Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/triage-context.md
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.
5 changes: 1 addition & 4 deletions src/praisonai-agents/praisonaiagents/tools/github_tools.py
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

Expand All @@ -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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The issue description mentions adding a debug log on successful branch creation. Please add a logger.debug call after this line to log the success, as it seems to be missing.

Suggested change
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)
logger.debug(f"Successfully created and checked out branch '{branch_name}'")

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching from git checkout -b to -B changes semantics: -B will reset an existing local branch to the current HEAD, which can discard local branch history/commits and is risky in developer environments. If the goal is to avoid failure when the branch already exists, a safer approach is to detect existence and then either (a) just check out the existing branch without resetting, or (b) choose a new branch name.

Copilot uses AI. Check for mistakes.
return f"Successfully created and checked out branch '{branch_name}'"
Comment on lines 14 to 18
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #1348 (as described in .github/triage-context.md) asks to add a logger.debug message on successful branch creation. This function currently returns success text but does not log on success. Consider adding logger.debug(...) immediately after the successful checkout so the requested behavior is implemented.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fix goal not implemented — missing logger.debug on success

Issue #1348 explicitly requests that github_create_branch emit a logger.debug message on successful branch creation. The PR claims to fix this issue, but no debug log is present in the success path. The logger object is already imported and used for errors, so the omission is clear.

Suggested change
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}'"

except subprocess.CalledProcessError as e:
logger.error(f"Failed to create branch: {e.stderr}")
Expand Down
9 changes: 9 additions & 0 deletions src/praisonai-agents/praisonaiagents/tools/shell_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Quote-stripping corrupts individually-quoted arguments

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 'git' 'status' → strips to git' 'statu (unbalanced quotes), causing shlex.split to raise ValueError: No closing quotation.

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)
Expand Down
130 changes: 99 additions & 31 deletions src/praisonai/praisonai/cli/commands/github.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import re
import sys
import json
import time
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand All @@ -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)

# ------------------------------------------------------------------
Expand All @@ -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
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch_url and compare_url interpolate self._branch_name without URL-encoding. Branch names commonly include / (e.g. feature/foo), which will break these URLs (especially the compare URL) or produce unintended paths. Encode the branch name (e.g., via urllib.parse.quote) when building GitHub URLs.

Copilot uses AI. Check for mistakes.

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)
Expand Down Expand Up @@ -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 {}

Expand All @@ -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:
Expand All @@ -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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This regular expression will fail to find the branch name from the github_commit_and_push tool's success message. The message is Successfully committed and pushed changes to branch '{branch_name}'. The regex should be updated to match the to branch '...' pattern.

Suggested change
m = re.search(r"branch '([^']+)'", result_str)
m = re.search(r"to branch '([^']+)'", result_str)

if m:
sticky.set_branch(m.group(1))
Comment on lines +364 to +367
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code reads sticky._branch_name from outside StickyComment, which couples the sink to StickyComment internals. To keep encapsulation (and make future refactors safer), expose a small accessor (e.g., has_branch() / branch_name property) and use that instead of reaching into a private attribute.

Copilot uses AI. Check for mistakes.
# 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
Expand Down Expand Up @@ -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


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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}")

Expand Down Expand Up @@ -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]")
Loading