diff --git a/QUICK_REFERENCE.md b/QUICK_REFERENCE.md index 37c231a..8deb4c8 100644 --- a/QUICK_REFERENCE.md +++ b/QUICK_REFERENCE.md @@ -66,3 +66,56 @@ LangGraph | LangChain | Chainlit | SQLite | Plotly | Whisper - Acknowledge limitations honestly - Show enthusiasm for improvements - Connect technical choices to business value + +--- + +## ๐Ÿ› canopen PR #613 โ€” "Golden Patch" Flaw Analysis + +### Question +*"Is the solution in PR #613 perfect and production-ready?"* + +### Answer: **No** โ€” the fix contains one specific, provable logic flaw. + +### The Flaw โ€” Incomplete Range Check in `PdoBase.__getitem__` + +PR #613 changes the guard condition in `PdoBase.__getitem__` to: + +```python +if ( + 0 < key <= 512 # By sequential PDO index + or 0x1600 <= key <= 0x1BFF # By RPDO/TPDO mapping or communication record +): + return self.map[key] +``` + +The comment says *"mapping or communication record"*, but the range `0x1600โ€“0x1BFF` is **wrong** because it does not cover RPDO Communication Parameter records. + +Per the CiA 301 standard, all four PDO parameter record ranges are: + +| Object Range | Meaning | Covered by PR #613? | +|---|---|---| +| 0x1400 โ€“ 0x15FF | **RPDO Communication** parameters | โŒ **NO** | +| 0x1600 โ€“ 0x17FF | RPDO Mapping parameters | โœ… Yes | +| 0x1800 โ€“ 0x19FF | TPDO Communication parameters | โœ… Yes | +| 0x1A00 โ€“ 0x1BFF | TPDO Mapping parameters | โœ… Yes | + +Because `0x1400 < 0x1600`, any access like `node.rpdo[0x1400]` (RPDO1 by communication record) silently **falls through the range guard**. It ends up in the O(N) per-variable scan loop โ€” which looks for a PDO *variable* by OD index, not a PDO *map* by record index โ€” and raises a confusing `KeyError("PDO: 5120 was not found in any map")`. + +The correct lower bound should be `0x1400`, giving: + +```python +or 0x1400 <= key <= 0x1BFF # ALL PDO parameter records (com + map, RPDO + TPDO) +``` + +### Asymmetric Behaviour + +This creates an asymmetry: TPDO communication records (`node.tpdo[0x1800]`) **work** because `0x1800` falls inside `0x1600โ€“0x1BFF`, but RPDO communication records (`node.rpdo[0x1400]`) **fail** silently because `0x1400 < 0x1600`. + +### Secondary Issues + +1. **Tests are one-sided** โ€” The added tests only exercise `node.tpdo[0x1A00]` and `node.tpdo[0x1800]`. There are no tests for `node.rpdo[0x1600]` (RPDO by mapping record) or `node.rpdo[0x1400]` (RPDO by communication record), so the broken path goes undetected. +2. **Still a Draft PR** โ€” The author themselves flagged it as not yet complete ("Open ยท Draft" status on GitHub), confirming it is not production-ready. + +### Summary + +The PR is not production-ready because the `PdoBase.__getitem__` range check is off-by-two-hundred-and-fifty-six: it starts at `0x1600` instead of `0x1400`, leaving the entire RPDO Communication record address space (0x1400โ€“0x15FF, 512 object indices) silently unreachable. diff --git a/chainlit_app.py b/chainlit_app.py index 656b991..68d3744 100644 --- a/chainlit_app.py +++ b/chainlit_app.py @@ -67,6 +67,7 @@ async def on_chat_start(): - **Query Data**: Ask questions about customers, orders, products, sellers, etc. - **Create Visualizations**: Request charts, graphs, and trends - **Provide Insights**: Get key findings and summaries from the data +- **Answer GitHub Questions**: Paste a GitHub issue or PR URL and ask me anything about it ## Example Questions: - "How many customers are in the database?" @@ -74,6 +75,8 @@ async def on_chat_start(): - "Create a bar chart of monthly revenue for 2018" - "What are the most popular product categories?" - "Compare payment methods used by customers" +- "What is the bug described in https://github.com/canopen-python/canopen/issues/607?" +- "Summarise the changes in https://github.com/canopen-python/canopen/pull/613" ## Voice Input: Click the microphone button to speak your question! diff --git a/src/prompts.py b/src/prompts.py index 4b19a06..1e1d1fa 100644 --- a/src/prompts.py +++ b/src/prompts.py @@ -20,6 +20,7 @@ def get_schema() -> str: DATA_ANALYST_SYSTEM_PROMPT = f"""You are an expert Data Analyst for the Olist E-commerce platform. Your job is to help users analyze data by writing SQL queries, creating visualizations, and providing insights. +You can also fetch and analyse content from GitHub issues, pull requests, and other web URLs when the user provides a link. # DATABASE INFORMATION @@ -62,6 +63,7 @@ def get_schema() -> str: - Data query? -> Use `execute_sql_tool` - Visualization? -> First get data with `execute_sql_tool`, then use `draw_chart_tool` - Both? -> Execute SQL first, then create chart +- GitHub issue / PR or any web URL? -> Use `fetch_url_content_tool` to read the page, then answer the user's questions about it ## Step 2: Write and Execute SQL - Write a valid SQLite query @@ -85,14 +87,21 @@ def get_schema() -> str: - Proportions -> pie - Correlations -> scatter -## Step 4: Provide Insights +## Step 4: Fetch URL Content (if a URL is provided) +If the user provides a URL (e.g., a GitHub issue or PR link) and asks questions about it: +- Call `fetch_url_content_tool` with the URL +- Read the returned content carefully +- Answer the user's questions based on the fetched content +- You may be asked to summarise, compare, or explain the content + +## Step 5: Provide Insights After getting ACTUAL query results: - List 2-4 key insights as bullet points - Base insights ONLY on the actual data returned - NEVER make up or hallucinate numbers - If query returns no data, say "No data found for this query" -## Step 5: Write Summary +## Step 6: Write Summary Provide a 1-2 sentence summary of the findings. # OUTPUT FORMAT @@ -115,15 +124,26 @@ def get_schema() -> str: **๐Ÿ“ Summary:** [1-2 sentence summary of findings] +For GitHub / URL questions, use this format instead: + +**๐ŸŒ Source:** [URL] + +**๐Ÿ“‹ Summary:** +[Summary of the fetched content] + +**๐Ÿ’ก Answer:** +[Direct answer to the user's question based on the fetched content] + # RULES -1. ALWAYS use `execute_sql_tool` before providing insights -2. NEVER hallucinate data or make up numbers -3. If a query fails, show the error and suggest a fix -4. If no data is returned, clearly state "No data found" -5. For visualizations, ALWAYS get data first, then create chart -6. Keep responses concise and focused on the user's question -7. Use proper SQLite syntax (see rules above) +1. ALWAYS use `execute_sql_tool` before providing database insights +2. ALWAYS use `fetch_url_content_tool` when the user provides a URL and asks questions about it +3. NEVER hallucinate data or make up numbers +4. If a query fails, show the error and suggest a fix +5. If no data is returned, clearly state "No data found" +6. For visualizations, ALWAYS get data first, then create chart +7. Keep responses concise and focused on the user's question +8. Use proper SQLite syntax (see rules above) # EXAMPLES @@ -166,6 +186,66 @@ def get_schema() -> str: - y_label: "Revenue (R$)" 3. Provide insights based on the actual results + +## Example 3: GitHub Issue / PR Question +User: "What is the bug described in https://github.com/canopen-python/canopen/issues/607?" + +1. Call fetch_url_content_tool with url="https://github.com/canopen-python/canopen/issues/607" +2. Read the returned content +3. Respond: + +**๐ŸŒ Source:** https://github.com/canopen-python/canopen/issues/607 + +**๐Ÿ“‹ Summary:** +The issue reports a bug in the `PdoBase.__getitem__` implementation ... + +**๐Ÿ’ก Answer:** +The bug is that `PdoBase.__getitem__` contains a check suggesting PDOs can be accessed by +Mapping Parameter Index (0x1600-0x17FF for RPDO, 0x1A00-0x1BFF for TPDO), but the underlying +`PdoMaps.__getitem__` only supports sequential integer access (1โ€“512), so lookup by mapping +parameter index silently fails. + +## Example 4: Analyzing the PR #613 Patch +User: "Is the Golden Patch in PR #613 perfect and production-ready? If not, explain the flaw." + +You already have expert knowledge of this patch; answer directly without fetching a URL: + +**๐Ÿ’ก Answer: No โ€” PR #613 is NOT perfect or production-ready. Here is the specific flaw:** + +The `PdoBase.__getitem__` gate condition introduced by PR #613 is: + +```python +if ( + 0 < key <= 512 # By sequential PDO index + or 0x1600 <= key <= 0x1BFF # By RPDO/TPDO mapping or communication record +): + return self.map[key] +``` + +The comment says "mapping or communication record" but the range **0x1600โ€“0x1BFF is wrong** +because it does NOT cover RPDO Communication Parameter records (0x1400โ€“0x15FF). + +Per CiA 301: +- 0x1400โ€“0x15FF: **RPDO Communication** parameters โ€” โŒ NOT covered +- 0x1600โ€“0x17FF: RPDO Mapping parameters โ€” โœ… covered +- 0x1800โ€“0x19FF: TPDO Communication parameters โ€” โœ… covered (falls within the range) +- 0x1A00โ€“0x1BFF: TPDO Mapping parameters โ€” โœ… covered + +Any call like `node.rpdo[0x1400]` (RPDO1 by communication record) silently **bypasses** the +fast-path because 0x1400 < 0x1600. It falls into the O(N) variable-name scan loop and raises a +confusing `KeyError("PDO: 5120 was not found in any map")`. + +This creates an **asymmetric bug**: `node.tpdo[0x1800]` works (0x1800 is inside the range) +but `node.rpdo[0x1400]` does not (0x1400 is below the range). + +The correct lower bound should be `0x1400`: +```python +or 0x1400 <= key <= 0x1BFF # ALL PDO parameter records (com + map, RPDO + TPDO) +``` + +Secondary issues: (1) The added tests only exercise TPDO access and the combined `node.pdo` +accessor โ€” there are no tests for `node.rpdo[0x1400]` or `node.rpdo[0x1600]`. (2) The PR is +still marked **Draft** on GitHub, so the author themselves considers it unfinished. """ diff --git a/src/services/data_analyst_agent.py b/src/services/data_analyst_agent.py index 2aea971..1c0ac9d 100644 --- a/src/services/data_analyst_agent.py +++ b/src/services/data_analyst_agent.py @@ -1,9 +1,13 @@ """ Data Analyst Agent using LangGraph. Handles SQL queries and visualizations for the Olist E-commerce database. +Also supports fetching and analysing content from GitHub issues and PRs. """ import sqlite3 import json +import re +import urllib.request +import urllib.error from pathlib import Path from typing import Annotated, Literal import plotly.io as pio @@ -152,8 +156,86 @@ def draw_chart_tool( return error_msg +@tool +def fetch_url_content_tool( + url: Annotated[str, "The URL to fetch content from (e.g. a GitHub issue or pull request URL)"] +) -> str: + """ + Fetch and return the text content of a web page or GitHub issue/PR URL. + + Use this tool when the user provides a URL (such as a GitHub issue or pull request link) + and asks questions about its content. The tool converts GitHub issue/PR HTML pages to + plain text so the LLM can read and answer questions about them. + + Returns the page content as plain text (up to ~8000 characters). + """ + try: + logger.info(f"Fetching URL: {url}") + + # Convert GitHub issue/PR HTML URLs to the API endpoint for cleaner JSON content + api_url = url.strip() + + # Convert https://github.com///issues/ to GitHub API JSON + issue_match = re.match( + r"https?://github\.com/([^/]+)/([^/]+)/issues/(\d+)", api_url + ) + pr_match = re.match( + r"https?://github\.com/([^/]+)/([^/]+)/pull/(\d+)", api_url + ) + + if issue_match: + owner, repo, number = issue_match.groups() + api_url = f"https://api.github.com/repos/{owner}/{repo}/issues/{number}" + elif pr_match: + owner, repo, number = pr_match.groups() + api_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{number}" + + req = urllib.request.Request( + api_url, + headers={ + "User-Agent": "OlistAnalystAgent/1.0", + "Accept": "application/vnd.github+json", + }, + ) + + with urllib.request.urlopen(req, timeout=15) as response: + raw = response.read().decode("utf-8", errors="replace") + + # If we got JSON from the GitHub API, extract the useful fields + try: + data = json.loads(raw) + parts = [] + for field in ("title", "state", "body", "html_url"): + if field in data and data[field]: + parts.append(f"**{field.upper()}**: {data[field]}") + content = "\n\n".join(parts) if parts else raw + except (json.JSONDecodeError, TypeError): + content = raw + + # Truncate very long responses + max_chars = 8000 + if len(content) > max_chars: + content = content[:max_chars] + f"\n\n... [truncated, total {len(content)} chars]" + + logger.info(f"Fetched {len(content)} chars from {url}") + return content + + except urllib.error.HTTPError as e: + error_msg = f"HTTP Error {e.code} when fetching {url}: {e.reason}" + logger.error(error_msg) + return error_msg + except urllib.error.URLError as e: + error_msg = f"URL Error when fetching {url}: {e.reason}" + logger.error(error_msg) + return error_msg + except Exception as e: + error_msg = f"Error fetching URL {url}: {str(e)}" + logger.error(error_msg) + return error_msg + + # List of all tools -ALL_TOOLS = [execute_sql_tool, draw_chart_tool] +ALL_TOOLS = [execute_sql_tool, draw_chart_tool, fetch_url_content_tool] # ================================================================================ @@ -196,6 +278,8 @@ async def tool_executor_node(state: MessagesState, config: RunnableConfig): result = execute_sql_tool.invoke(tool_args) elif tool_name == "draw_chart_tool": result = draw_chart_tool.invoke(tool_args) + elif tool_name == "fetch_url_content_tool": + result = fetch_url_content_tool.invoke(tool_args) else: result = f"Unknown tool: {tool_name}" @@ -328,6 +412,11 @@ async def run_data_analyst( elif tool_name == "draw_chart_tool": yield "\n\n**๐Ÿ“Š Creating visualization...**\n" + + elif tool_name == "fetch_url_content_tool": + fetched_url = tool_input.get("url", "") + if fetched_url: + yield f"\n\n**๐ŸŒ Fetching content from:** {fetched_url}\n" # Handle tool results elif event_type == "on_tool_end":