-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Issue 1348 #1349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Issue 1348 #1349
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| # GitHub Issue #1337: Big Task: Document CLI Router Architecture | ||
| # GitHub Issue #1348: Add logging to github_create_branch on success | ||
|
|
||
| Please thoroughly trace how src/praisonai/praisonai/cli/main.py interfaces with typer across all command modules. Produce a detailed architecture document in a new file docs/cli_architecture_test.md. | ||
| The github_create_branch function in src/praisonai-agents/praisonaiagents/tools/github_tools.py should log a debug message on successful branch creation using logger.debug. |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,4 @@ | ||||||||||
| import os | ||||||||||
| import subprocess | ||||||||||
| from pydantic import BaseModel, Field | ||||||||||
| from typing import Dict, Any, Optional | ||||||||||
| import logging | ||||||||||
| from .decorator import tool | ||||||||||
|
|
||||||||||
|
|
@@ -17,7 +14,7 @@ def github_create_branch(branch_name: str) -> str: | |||||||||
| try: | ||||||||||
| # Check if we are in a git repository | ||||||||||
| subprocess.run(["git", "rev-parse", "--is-inside-work-tree"], check=True, capture_output=True) | ||||||||||
| subprocess.run(["git", "checkout", "-b", branch_name], check=True, capture_output=True, text=True) | ||||||||||
| subprocess.run(["git", "checkout", "-B", branch_name], check=True, capture_output=True, text=True) | ||||||||||
|
||||||||||
| return f"Successfully created and checked out branch '{branch_name}'" | ||||||||||
|
Comment on lines
14
to
18
|
||||||||||
| return f"Successfully created and checked out branch '{branch_name}'" | |
| subprocess.run(["git", "checkout", "-B", branch_name], check=True, capture_output=True, text=True) | |
| logger.debug(f"Successfully created and checked out branch '{branch_name}'") | |
| return f"Successfully created and checked out branch '{branch_name}'" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,13 +51,22 @@ def execute_command( | |
| Dictionary with execution results | ||
| """ | ||
| try: | ||
| # Strip wrapping quotes the LLM sometimes adds around the whole command string | ||
| if command and len(command) >= 2 and command[0] == command[-1] and command[0] in ("'", '"'): | ||
| command = command[1:-1] | ||
|
Comment on lines
+55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The heuristic strips the first and last character when both are the same quote character. This fires on legitimate commands where multiple arguments happen to be individually shell-quoted — for example A safer approach validates the stripped string before committing: # Strip wrapping quotes the LLM sometimes adds around the whole command string
if command and len(command) >= 2 and command[0] == command[-1] and command[0] in ("'", '"'):
stripped = command[1:-1]
try:
shlex.split(stripped) # validate before committing to strip
command = stripped
except ValueError:
pass # keep original if stripping breaks parsing |
||
| # Treat empty-string cwd same as None to avoid subprocess failure | ||
| if not cwd: | ||
| cwd = None | ||
| # Always split command for safety (no shell execution) | ||
| # Use shlex.split with appropriate posix flag | ||
| if platform.system() == 'Windows': | ||
| # Use shlex with posix=False for Windows to handle quotes properly | ||
| command = shlex.split(command, posix=False) | ||
| else: | ||
| command = shlex.split(command) | ||
| # Guard against empty command list (e.g. LLM passed empty string) | ||
| if not command: | ||
| return {"error": "Empty command", "stdout": "", "stderr": "", "exit_code": 1} | ||
|
|
||
| # Expand tilde and environment variables in command arguments | ||
| # (shell=False means the shell won't do this for us) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||
| import os | ||||||
| import re | ||||||
| import sys | ||||||
| import json | ||||||
| import time | ||||||
|
|
@@ -59,15 +60,23 @@ class StickyComment: | |||||
| WORKING = "🔄" | ||||||
|
|
||||||
| def __init__(self, api_base: str, issue: int, token: str, | ||||||
| task_type: str, title: str, run_url: str): | ||||||
| task_type: str, title: str, run_url: str, | ||||||
| repo_name: str = ""): | ||||||
| self._api_base = api_base | ||||||
| self._issue = issue | ||||||
| self._token = token | ||||||
| self._task_type = task_type | ||||||
| self._title = title | ||||||
| self._run_url = run_url | ||||||
| self._repo_name = repo_name | ||||||
| self._comment_id: Optional[int] = None | ||||||
|
|
||||||
| # Branch and PR tracking | ||||||
| self._branch_name: Optional[str] = None | ||||||
| self._pr_url: Optional[str] = None | ||||||
| self._pr_number: Optional[int] = None | ||||||
| self._summary_lines: List[str] = [] | ||||||
|
|
||||||
| # Todo list: list of dicts {content, status} status ∈ pending/in_progress/completed | ||||||
| self._todos: List[Dict[str, Any]] = [] | ||||||
| # Free-form activity log shown below the todo list (last N lines) | ||||||
|
|
@@ -154,6 +163,25 @@ def on_tool_end(self, agent_name: str, tool_name: str) -> None: | |||||
| self._current_tool = None | ||||||
| # no extra log for tool end – keeps comment clean | ||||||
|
|
||||||
| def set_branch(self, branch_name: str) -> None: | ||||||
| """Called when a branch is successfully created.""" | ||||||
| with self._lock: | ||||||
| self._branch_name = branch_name | ||||||
| self._schedule_push() | ||||||
|
|
||||||
| def set_pr(self, pr_url: str, pr_number: Optional[int] = None) -> None: | ||||||
| """Called when a PR is explicitly created.""" | ||||||
| with self._lock: | ||||||
| self._pr_url = pr_url | ||||||
| self._pr_number = pr_number | ||||||
| self._schedule_push() | ||||||
|
|
||||||
| def add_summary_line(self, line: str) -> None: | ||||||
| """Append a line to the Summary section.""" | ||||||
| with self._lock: | ||||||
| self._summary_lines.append(line) | ||||||
| self._schedule_push() | ||||||
|
|
||||||
| def finalize(self, success: bool, summary: str) -> None: | ||||||
| """Push the final state synchronously.""" | ||||||
| if self._timer: | ||||||
|
|
@@ -164,7 +192,8 @@ def finalize(self, success: bool, summary: str) -> None: | |||||
| for t in self._todos: | ||||||
| if t["status"] == "in_progress": | ||||||
| t["status"] = "completed" | ||||||
| self._logs.append(summary) | ||||||
| if summary: | ||||||
| self._summary_lines.append(summary) | ||||||
| self._push_now(final=success) | ||||||
|
|
||||||
| # ------------------------------------------------------------------ | ||||||
|
|
@@ -190,51 +219,64 @@ def _tool_label(tool_name: str, tool_args: dict) -> str: | |||||
| return f"`{tool_name}`" | ||||||
|
|
||||||
| def _build_body(self, final: Optional[bool] = None) -> str: | ||||||
| """Render the full Markdown comment body.""" | ||||||
| """Render the full Markdown comment body (Claude Code style).""" | ||||||
| if final is True: | ||||||
| icon = self.DONE | ||||||
| status_icon = self.DONE | ||||||
| verb = "finished" | ||||||
| elif final is False: | ||||||
| icon = self.FAIL | ||||||
| status_icon = self.FAIL | ||||||
| verb = "failed on" | ||||||
| else: | ||||||
| icon = self.SPINNER | ||||||
| status_icon = self.SPINNER | ||||||
| verb = "is working on" | ||||||
|
|
||||||
| lines: List[str] = [] | ||||||
|
|
||||||
| # Title line | ||||||
| lines.append(f"**{self._task_type} #{self._issue}: {self._title}** {icon}") | ||||||
| lines.append("") | ||||||
|
|
||||||
| # Job link | ||||||
| # --- Header line (Claude Code style) --- | ||||||
| meta_parts: List[str] = [] | ||||||
| if self._run_url: | ||||||
| lines.append(f"[View job run]({self._run_url})") | ||||||
| lines.append("") | ||||||
| meta_parts.append(f"[View job]({self._run_url})") | ||||||
| if self._branch_name and self._repo_name: | ||||||
| branch_url = f"https://github.com/{self._repo_name}/tree/{self._branch_name}" | ||||||
| meta_parts.append(f"[`{self._branch_name}`]({branch_url})") | ||||||
| if self._pr_url: | ||||||
| meta_parts.append(f"[View PR →]({self._pr_url})") | ||||||
| else: | ||||||
| compare_url = f"https://github.com/{self._repo_name}/compare/main...{self._branch_name}?quick_pull=1" | ||||||
| meta_parts.append(f"[Create PR →]({compare_url})") | ||||||
|
Comment on lines
+239
to
+246
|
||||||
|
|
||||||
| header = f"**PraisonAI {verb} #{self._issue}** {status_icon}" | ||||||
| lines.append(" · ".join([header] + meta_parts) if meta_parts else header) | ||||||
| lines.append("") | ||||||
| lines.append(f"**{self._task_type} #{self._issue}: {self._title}**") | ||||||
| lines.append("") | ||||||
|
|
||||||
| # --- Todo list --- | ||||||
| if self._todos: | ||||||
| lines.append("**Todo List**") | ||||||
| lines.append("") | ||||||
| for t in self._todos: | ||||||
| s = t["status"] | ||||||
| if s == "completed": | ||||||
| box = "- [x]" | ||||||
| elif s == "in_progress": | ||||||
| box = "- [x]" # GitHub renders in_progress same as done; use ✅ marker | ||||||
| else: | ||||||
| box = "- [ ]" | ||||||
| box = "- [x]" if s == "completed" else "- [ ]" | ||||||
| content = t["content"] | ||||||
| if s == "in_progress": | ||||||
| content = f"{content} 🔄" | ||||||
| elif s == "completed": | ||||||
| content = f"~~{content}~~" if final else content | ||||||
| lines.append(f"{box} {content}") | ||||||
| lines.append("") | ||||||
|
|
||||||
| # --- Activity log (last 15 items) --- | ||||||
| recent = self._logs[-15:] | ||||||
| if recent: | ||||||
| # --- Summary section --- | ||||||
| if self._summary_lines: | ||||||
| lines.append("**Summary**") | ||||||
| lines.append("") | ||||||
| for entry in self._summary_lines[-10:]: | ||||||
| lines.append(f"{entry}") | ||||||
| lines.append("") | ||||||
|
|
||||||
| # --- Activity log (only while running, last 8 items) --- | ||||||
| if final is None and self._logs: | ||||||
| lines.append("**Activity**") | ||||||
| lines.append("") | ||||||
| for entry in recent: | ||||||
| for entry in self._logs[-8:]: | ||||||
| lines.append(f"- {entry}") | ||||||
|
|
||||||
| return "\n".join(lines) | ||||||
|
|
@@ -283,7 +325,6 @@ class GithubContextSink(ContextTraceSinkProtocol): | |||||
| def emit(self, event) -> None: | ||||||
| try: | ||||||
| et = event.event_type | ||||||
| # Normalise enum vs string | ||||||
| et_val = et.value if hasattr(et, "value") else str(et) | ||||||
| data = event.data or {} | ||||||
|
|
||||||
|
|
@@ -298,7 +339,6 @@ def emit(self, event) -> None: | |||||
| tool_args = data.get("tool_args") or {} | ||||||
| agent_name = event.agent_name or "" | ||||||
|
|
||||||
| # TodoWrite → extract plan and build checkbox list | ||||||
| if tool_name == "TodoWrite": | ||||||
| todos = tool_args.get("todos", []) | ||||||
| if todos: | ||||||
|
|
@@ -308,11 +348,35 @@ def emit(self, event) -> None: | |||||
| sticky.set_todos(todos) | ||||||
| else: | ||||||
| sticky.on_tool_start(agent_name, tool_name, tool_args) | ||||||
| # Capture branch name eagerly from args | ||||||
| if tool_name == "github_create_branch": | ||||||
| bn = tool_args.get("branch_name", "") | ||||||
| if bn: | ||||||
| sticky.set_branch(bn) | ||||||
|
|
||||||
| elif et_val == "tool_call_end": | ||||||
| tool_name = data.get("tool_name", "") | ||||||
| tool_result = data.get("result") or data.get("tool_result") or {} | ||||||
| result_str = str(tool_result) | ||||||
| if tool_name != "TodoWrite": | ||||||
| sticky.on_tool_end(event.agent_name or "", tool_name) | ||||||
| # Capture branch from push result (fallback) | ||||||
| if tool_name == "github_commit_and_push" and not sticky._branch_name: | ||||||
| m = re.search(r"branch '([^']+)'", result_str) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regular expression will fail to find the branch name from the
Suggested change
|
||||||
| if m: | ||||||
| sticky.set_branch(m.group(1)) | ||||||
|
Comment on lines
+364
to
+367
|
||||||
| # Capture PR URL only when explicitly created | ||||||
| if tool_name == "github_create_pull_request": | ||||||
| m = re.search(r'https://github\.com/[^\s"\)]+/pull/\d+', result_str) | ||||||
| if m: | ||||||
| pr_url = m.group(0) | ||||||
| pr_num = re.search(r'/pull/(\d+)', pr_url) | ||||||
| sticky.set_pr(pr_url, int(pr_num.group(1)) if pr_num else None) | ||||||
| if isinstance(tool_result, dict): | ||||||
| url = tool_result.get("url") or tool_result.get("html_url", "") | ||||||
| if url and "/pull/" in url: | ||||||
| pr_num = re.search(r'/pull/(\d+)', url) | ||||||
| sticky.set_pr(url, int(pr_num.group(1)) if pr_num else None) | ||||||
|
|
||||||
| except Exception: | ||||||
| pass | ||||||
|
|
@@ -370,15 +434,18 @@ def _load_yaml_steps_as_todos(agent_file: str, sticky: StickyComment) -> None: | |||||
| with sticky._lock: | ||||||
| sticky._todos = todos | ||||||
|
|
||||||
| # Build a reverse map: agent role name → step index for the sink to use | ||||||
| # Build a reverse map: role name / step name / agent key → step index | ||||||
| sticky._step_agent_map = {} | ||||||
| for i, step in enumerate(steps): | ||||||
| if not isinstance(step, dict): | ||||||
| continue | ||||||
| agent_key = step.get("agent", "") | ||||||
| step_name = step.get("name", "") | ||||||
| role_cfg = roles.get(agent_key, {}) | ||||||
| role_name = role_cfg.get("role", agent_key) | ||||||
| sticky._step_agent_map[role_name.lower()] = i | ||||||
| for name in (role_name, step_name, agent_key): | ||||||
| if name: | ||||||
| sticky._step_agent_map[name.lower()] = i | ||||||
|
|
||||||
|
|
||||||
| # --------------------------------------------------------------------------- | ||||||
|
|
@@ -444,6 +511,7 @@ def github_triage( | |||||
| sticky = StickyComment( | ||||||
| api_base=api_base, issue=issue, token=token, | ||||||
| task_type=task_type, title=ctx_title, run_url=run_url, | ||||||
| repo_name=repo_name, | ||||||
| ) | ||||||
| sticky._logs.append(f"Context fetched for {task_type} #{issue}") | ||||||
|
|
||||||
|
|
@@ -505,9 +573,9 @@ def github_triage( | |||||
| pass | ||||||
|
|
||||||
| if success: | ||||||
| summary = f"✅ **Completed** – {task_type} #{issue} triaged successfully." | ||||||
| summary = f"✅ Completed – {task_type} #{issue} triaged successfully." | ||||||
| else: | ||||||
| summary = f"❌ **Failed** – {error_msg[:200]}" | ||||||
| summary = f"❌ Failed – {error_msg[:200]}" | ||||||
|
|
||||||
| sticky.finalize(success=success, summary=summary) | ||||||
| print("[green]✅ Final sticky comment updated.[/green]") | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue description mentions adding a debug log on successful branch creation. Please add a
logger.debugcall after this line to log the success, as it seems to be missing.