Open
Conversation
…structure - Created CommandParser class with base structure - Added Command dataclass for parsed commands - Added CommandParseError exception class - Implemented parse() method signature and documentation - Added comprehensive docstrings and usage examples - Follows code patterns from gh_client.py - Module imports and verifies successfully
…rmed commands, special chars) Improved command parser with robust edge case handling: 1. Empty input handling - returns empty list for empty/whitespace-only strings 2. Malformed command detection - added MALFORMED_PATTERN to detect and log warnings 3. Special character handling - commands with trailing punctuation (e.g., /merge!, /merge.) are sanitized 4. Numeric command filtering - commands like /123 are skipped 5. Double slash prevention - negative lookbehind prevents matching //merge 6. Argument sanitization - trailing punctuation removed from arguments Changes: - Updated COMMAND_PATTERN regex to use \S+? (non-whitespace) instead of \w+ to capture special chars - Added negative lookbehind (?<!/) to prevent matching after another slash - Added MALFORMED_PATTERN for detecting potentially malformed commands - Added _sanitize_command_type() method to clean command names - Enhanced _parse_args() to sanitize arguments by removing trailing punctuation - Added debug logging for malformed patterns and skipped commands Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Comprehensive summary of edge case handling implementation: - Empty input handling - Malformed command detection - Special character sanitization - Numeric command filtering - Double slash prevention - Argument sanitization All 11 edge case tests passed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…e structure - Created CommandExecutor class with base structure - Added custom exceptions: CommandExecutionError, PermissionDeniedError - Added CommandResult dataclass for execution results - Implemented execute() and execute_all() methods - Added stub handlers for merge, resolve, and process commands - Added permission validation and feedback posting methods - Follows patterns from gh_client.py (async, logging, error handling) - Syntax verified with py_compile Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…lient.pr_merge - Parse optional merge method from command args (merge/squash/rebase) - Call gh_client.pr_merge() to execute the merge - Add comprehensive error handling for common merge failures: - Not mergeable (conflicts) - Merge conflicts - Failing CI checks - Requires approval - Draft PR state - Return structured CommandResult with merge status - Use logger for all output (no print statements)
…comment processing Implemented _handle_process() method that: - Fetches inline comments from PR via gh_client.get_inline_comments() - Generates summary with per-file breakdown of comments - Posts summary as PR comment via gh_client.pr_comment() - Returns structured CommandResult with comment count and details - Handles empty comments case gracefully - Comprehensive error handling for GHCommandError and generic exceptions Follows all patterns from gh_client.py: - Proper async/await pattern - Logging using logger (no print statements) - Structured error handling - Type hints with dict[str, list[dict]] - CommandResult with success/error/data fields Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Added imports for CommandParser, Command, CommandExecutor, and CommandResult - Added process_commands() method that integrates parser and executor - Method parses PR comment text, extracts commands, and executes them - Includes proper error handling, logging, and progress reporting - Returns list of CommandResult objects for each command executed - Follows existing orchestrator patterns (async/await, delegation, progress callbacks)
Implemented the _post_feedback method to post structured command results as PR comments using gh_client.pr_comment(). Features: - Formats command results with status indicators (✓/✗) - Includes command type, message, and error details - Provides contextual information based on command type - merge: merge method, who merged - resolve: package manager, command, output snippet - process: comment count, files affected - Markdown-formatted comments for readability - Automatic footer indicating auto-generation - Proper error handling for failed comment posting Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…mand flow Created comprehensive integration tests for the GitHub PR command system: - test_end_to_end_merge_command: Tests full /merge command flow from parsing to execution - test_end_to_end_resolve_command: Tests /resolve with package manager detection - test_end_to_end_process_command: Tests /process with inline comment handling - test_permission_denied_flow: Tests permission validation integration - test_multiple_commands_sequential_execution: Tests multiple commands in sequence - test_command_failure_stops_execution: Tests stop-on-failure behavior - test_no_commands_in_comment: Tests handling of comments without commands - test_unknown_command_ignored: Tests graceful handling of unknown commands - test_mixed_known_and_unknown_commands: Tests filtering of unknown commands - test_audit_trail_logging: Tests audit trail generation and logging All tests use mocked GHClient and permission checker for isolated testing. Tests verify the full integration between parser, executor, and feedback posting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updated implementation plan to mark subtask-4-3 (Write integration tests for end-to-end command flow) as completed. All 10 integration tests pass successfully, covering the full command flow from parsing to execution to feedback posting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added Session 9 entry documenting the completion of integration tests for end-to-end command flow. All 10 tests pass successfully, covering the full integration from command parsing through execution to feedback posting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRemoves numerous project specification and planning documentation files while introducing a complete GitHub PR command processing pipeline comprising a dedicated parser module, command executor with permission checks and feedback posting, orchestrator integration, and comprehensive integration tests. Changes
Sequence DiagramsequenceDiagram
participant User as PR Commenter
participant GHOrch as GitHubOrchestrator
participant Parser as CommandParser
participant Executor as CommandExecutor
participant PermChk as GitHubPermissionChecker
participant GHClient as GitHub API
participant Audit as Audit Log
User->>GHOrch: process_commands(comment_text, pr_number, username)
GHOrch->>Parser: parse(comment_text)
Parser->>Parser: regex match & sanitize
Parser-->>GHOrch: [Command, Command, ...]
alt Commands Found
GHOrch->>Executor: execute_all(commands, pr_number, username)
loop For Each Command
Executor->>PermChk: check_permission(username, command_type)
PermChk-->>Executor: bool
alt Permission Granted
Executor->>Executor: _handle_*() by command type
Executor->>GHClient: API call (merge/comment/etc)
GHClient-->>Executor: result
Executor->>Audit: _log_audit(attempt, success)
Executor-->>Executor: CommandResult(success=true)
else Permission Denied
Executor->>Audit: _log_audit(denied)
Executor-->>Executor: CommandResult(success=false, error)
end
Executor->>GHClient: _post_feedback(pr_comment)
end
Executor-->>GHOrch: [CommandResult, CommandResult, ...]
else No Commands
GHOrch-->>GHOrch: return []
end
GHOrch-->>User: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/runners/github/command_executor.py`:
- Around line 463-485: The permission check incorrectly treats command.type ==
"process" as read-only even though _handle_process and _post_feedback perform
writes (post PR comments); update _check_permissions to treat "/process" as a
write operation: call checker.get_user_role and enforce write-level roles (same
logic used for other write commands), remove the bypass that grants any repo
relationship for command.type == "process", and change the audit log's
additional_context "operation_type" from "read" to "write" when handling
"/process"; make the same change in the other duplicated permission blocks
referenced (around the other ranges) so all checks for "/process" consistently
require write permissions.
- Around line 823-835: The subprocess created in the install block (the variable
process from asyncio.create_subprocess_exec used with
install_command/install_args and cwd=self.project_dir) can leak when
asyncio.wait_for times out; catch asyncio.TimeoutError around the await
asyncio.wait_for(process.communicate(), ...) call, and on timeout synchronously
terminate the child (e.g., call process.kill() or process.terminate()), then
await process.wait() to reap it and finally attempt to collect/close
stdout/stderr (e.g., await process.communicate() or ensure pipes are closed)
before re-raising or returning a timeout error; apply the same pattern to the
other installer block referenced (the similar process handling at the 908-933
section) so no child processes remain running after a timeout.
- Around line 1004-1011: The hardcoded package-manager-to-file mapping in
command_executor.py can misclassify projects (e.g., mapping pyproject.toml to
multiple managers); replace this heuristic with a dynamic check by calling the
project detection in context/project_analyzer.py to determine the actual project
stack and allowed installers, and then consult core/security.py to fetch the
allowed command whitelist for that detected stack before selecting an installer;
locate the mapping/table in command_executor.py (the tuple list containing
"pipenv", "poetry", "hatch", "pdm", "uv", "conda", "pip") and remove/replace its
usage so installer selection flows from project_analyzer detection ->
core/security allowed commands.
- Around line 823-830: Replace the direct call to asyncio.create_subprocess_exec
and any manual use of self.project_dir when launching install_command with the
repository's platform abstraction utilities: call the platform execution helper
(e.g., Platform.execute or platform.run_subprocess) passing install_command and
install_args, and supply the working directory using the platform path-join
utility (e.g., platform.path.join(self.project_dir, ...)) instead of raw
strings; update error/stdout/stderr handling to use the execution helper's
returned objects. Locate the call site using symbols
asyncio.create_subprocess_exec, install_command, install_args, and
self.project_dir in command_executor.py and make the same replacement for the
similar invocation around the other referenced site (the code near the other
occurrence indicated by the review comment). Ensure imports reference the
platform abstraction module and remove direct asyncio subprocess imports if no
longer needed.
In `@apps/backend/runners/github/command_parser.py`:
- Around line 96-98: Remove the hardcoded SUPPORTED_COMMANDS list and instead
query the project stack via context/project_analyzer (e.g., call the stack
detection function used in the repo) and then ask core/security for the allowed
commands for that stack; replace usages of SUPPORTED_COMMANDS in
command_parser.py with the dynamic result (or call a core/security helper like
get_allowed_commands(stack) or is_command_allowed(command, stack)) so policy is
enforced centrally rather than baked into SUPPORTED_COMMANDS.
- Line 118: The constructor currently sets self.allowed_commands =
allowed_commands or self.SUPPORTED_COMMANDS which treats an empty list as falsy
and falls back to SUPPORTED_COMMANDS; change the logic to only use
SUPPORTED_COMMANDS when allowed_commands is None (e.g., check if
allowed_commands is None) and otherwise assign the provided allowed_commands so
that an explicit empty allowlist is respected; update the initialization around
allowed_commands / self.allowed_commands in the CommandParser (or relevant class
__init__) to implement this None-only fallback.
In `@apps/backend/runners/github/orchestrator.py`:
- Around line 1667-1668: The code currently instantiates CommandParser() with
embedded defaults; instead, obtain the repository/user-specific command
allowlist from the centralized security hooks: call the project stack detector
in context/project_analyzer.py to determine the project context, then fetch the
policy/allowlist from core/security.py and pass that policy into CommandParser
(or set it on the parser before calling parse(comment_text)); update the use
sites where CommandParser is constructed (the CommandParser() instantiation and
its parse method) to accept and use the policy-derived allowlist rather than
relying on defaults.
- Around line 1719-1722: The except block that currently logs and returns an
empty list should not swallow errors; change the handler in the except Exception
as e block so it logs the full exception (use logger.exception or include
traceback) and then re-raises the exception instead of returning []; i.e.,
replace logger.error(f"Failed to process commands for PR #{pr_number}: {e}") and
return [] with logger.exception("Failed to process commands for PR #%s",
pr_number) followed by raise so callers can distinguish operational failures
from legitimate empty-command results.
- Around line 1653-1656: The code calls logging.getLogger(...) inside
process_commands (creating variable logger) but logging is not imported; add an
import for the logging module at top of the module (e.g., import logging) or
reuse an existing module-level logger instead of calling logging directly so
that the call in process_commands (and the variable logger) resolves correctly;
update any tests or linter expectations if they assume a module-level logger
name (refer to the process_commands function and the logger variable created
with logging.getLogger(__name__)).
In `@apps/backend/tests/integration/test_github_commands.py`:
- Around line 675-680: The test mutates the logger for
'runners.github.command_executor' (executor_logger), clearing handlers, adding a
test handler, setting level and propagate, but never restores the original
state; capture the original executor_logger.handlers (copy list),
executor_logger.level, and executor_logger.propagate before changing them, then
ensure you restore those originals after the test (use a try/finally or test
teardown) and remove the temporary handler; apply the same capture/restore
around the other occurrences noted (lines ~697-698) so other tests are not
affected.
In `@SUBTASK_1_2_COMPLETION.md`:
- Around line 3-8: Add missing blank lines surrounding the markdown headings "##
Task" and "## Status" so they each have an empty line before and after the
heading (i.e., ensure a blank line precedes "## Task" and that both "## Task"
and "## Status" are followed and preceded by blank lines as appropriate) to
satisfy MD022.
In `@SUBTASK_1_3_COMPLETION_SUMMARY.md`:
- Around line 13-14: Update the COMMAND_PATTERN regex to the safer form and add
MALFORMED_PATTERN: replace the existing COMMAND_PATTERN with the new pattern
(?<!/)/(\S+?)(?:\s+([^\n]*?))?(?=\s|$|/) and add MALFORMED_PATTERN =
re.compile(r"/[^\w\s]|/\d+|//+") in command_parser.py to handle malformed and
edge-case inputs; also ensure Markdown fences and headings have surrounding
blank lines and change the fenced commit block language from ``` to ```text so
the commit block at the specified section declares a language.
- Around line 86-91: The claim "Double slashes: Prevents path traversal
attempts" overstates protection; update the summary block that lists edge case
handling (the bullet labeled "Double slashes") to either remove the assertion or
rephrase it to a precise statement (e.g., "Normalizes or rejects paths with
double slashes to reduce certain malformed path cases" or "May mitigate some
simple traversal patterns when combined with proper normalization and
validation") and ensure the surrounding header or list does not imply complete
path traversal protection without mentioning that proper path normalization and
validation are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d4170bd-1a1f-4bcb-b186-eed981af92c7
📒 Files selected for processing (21)
.auto-claude/specs/026-complete-platform-abstraction/audit_backend.md.auto-claude/specs/026-complete-platform-abstraction/build-progress.txt.auto-claude/specs/026-complete-platform-abstraction/implementation_plan.json.auto-claude/specs/078-batch-operations-quick-actions/VERIFICATION_SUBTASK_6_2.md.auto-claude/specs/078-batch-operations-quick-actions/build-progress.txt.auto-claude/specs/078-batch-operations-quick-actions/implementation_plan.json.auto-claude/specs/089-you-ve-hit-your-limit-resets-8pm-europe-saratov/implementation_plan.json.auto-claude/specs/096-transfer-february-commits-analysis/build-progress.txt.auto-claude/specs/096-transfer-february-commits-analysis/february_commits_analysis.md.auto-claude/specs/096-transfer-february-commits-analysis/implementation_plan.json.auto-claude/specs/131-adaptive-agent-personality-system/VERIFICATION_REPORT.md.auto-claude/specs/131-adaptive-agent-personality-system/build-progress.txt.auto-claude/specs/131-adaptive-agent-personality-system/implementation_plan.json.auto-claude/specs/145-comprehensive-documentation-portal/implementation_plan.jsonSUBTASK_1_2_COMPLETION.mdSUBTASK_1_3_COMPLETION_SUMMARY.mdapps/backend/runners/github/command_executor.pyapps/backend/runners/github/command_parser.pyapps/backend/runners/github/orchestrator.pyapps/backend/tests/integration/__init__.pyapps/backend/tests/integration/test_github_commands.py
💤 Files with no reviewable changes (14)
- .auto-claude/specs/078-batch-operations-quick-actions/build-progress.txt
- .auto-claude/specs/131-adaptive-agent-personality-system/VERIFICATION_REPORT.md
- .auto-claude/specs/078-batch-operations-quick-actions/implementation_plan.json
- .auto-claude/specs/145-comprehensive-documentation-portal/implementation_plan.json
- .auto-claude/specs/078-batch-operations-quick-actions/VERIFICATION_SUBTASK_6_2.md
- .auto-claude/specs/131-adaptive-agent-personality-system/build-progress.txt
- .auto-claude/specs/089-you-ve-hit-your-limit-resets-8pm-europe-saratov/implementation_plan.json
- .auto-claude/specs/096-transfer-february-commits-analysis/february_commits_analysis.md
- .auto-claude/specs/026-complete-platform-abstraction/implementation_plan.json
- .auto-claude/specs/026-complete-platform-abstraction/audit_backend.md
- .auto-claude/specs/131-adaptive-agent-personality-system/implementation_plan.json
- .auto-claude/specs/026-complete-platform-abstraction/build-progress.txt
- .auto-claude/specs/096-transfer-february-commits-analysis/implementation_plan.json
- .auto-claude/specs/096-transfer-february-commits-analysis/build-progress.txt
Comment on lines
+463
to
+485
| # Read operations (process) are allowed for anyone with repo access | ||
| # We still verify the user has at least read access | ||
| role = await checker.get_user_role(username) | ||
|
|
||
| # Allow if user has any relationship to the repo (even CONTRIBUTOR or NONE) | ||
| # We'll let the GitHub API itself reject if they truly can't access the repo | ||
| logger.info( | ||
| f"✓ User '{username}' (role: {role}) has permission to execute '/{command.type}'" | ||
| ) | ||
|
|
||
| # Log permission granted for read operation | ||
| self._log_audit( | ||
| event_type="permission_granted", | ||
| username=username, | ||
| command=command, | ||
| pr_number=pr_number, | ||
| additional_context={ | ||
| "user_role": role, | ||
| "operation_type": "read", | ||
| }, | ||
| ) | ||
|
|
||
| return True |
Contributor
There was a problem hiding this comment.
/process is treated as read-only but performs write operations.
/process posts comments (pr_comment) in _handle_process and _post_feedback, yet _check_permissions bypasses write-role enforcement for this command.
🔐 Proposed fix
- "command_requires_write": command.type in ["merge", "resolve"],
+ "command_requires_write": command.type in ["merge", "resolve", "process"],
...
- if command.type in ["merge", "resolve"]:
+ if command.type in ["merge", "resolve", "process"]:
...
- # Fail open for read operations, fail closed for write operations
- return command.type not in ["merge", "resolve"]
+ # Fail closed when permission verification errors occur
+ return FalseAlso applies to: 1129-1210, 1302-1330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/command_executor.py` around lines 463 - 485, The
permission check incorrectly treats command.type == "process" as read-only even
though _handle_process and _post_feedback perform writes (post PR comments);
update _check_permissions to treat "/process" as a write operation: call
checker.get_user_role and enforce write-level roles (same logic used for other
write commands), remove the bypass that grants any repo relationship for
command.type == "process", and change the audit log's additional_context
"operation_type" from "read" to "write" when handling "/process"; make the same
change in the other duplicated permission blocks referenced (around the other
ranges) so all checks for "/process" consistently require write permissions.
Comment on lines
+823
to
+835
| # Execute the install command using asyncio | ||
| process = await asyncio.create_subprocess_exec( | ||
| install_command, | ||
| *install_args, | ||
| cwd=self.project_dir, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
|
|
||
| stdout, stderr = await asyncio.wait_for( | ||
| process.communicate(), | ||
| timeout=120.0, # 2 minute timeout for package installs | ||
| ) |
Contributor
There was a problem hiding this comment.
Timeout path leaks child process.
On asyncio.TimeoutError, the subprocess is not terminated/waited, so dependency installers can continue running in the background.
🛡️ Proposed fix
+import contextlib
...
async def _handle_resolve(
self,
command: Command,
pr_number: int,
username: str,
) -> CommandResult:
...
+ process: asyncio.subprocess.Process | None = None
try:
...
- process = await asyncio.create_subprocess_exec(
+ process = await asyncio.create_subprocess_exec(
install_command,
*install_args,
cwd=self.project_dir,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
...
except asyncio.TimeoutError:
+ if process and process.returncode is None:
+ process.kill()
+ with contextlib.suppress(Exception):
+ await process.wait()
logger.error(f"Package install timed out after 120s")Also applies to: 908-933
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/command_executor.py` around lines 823 - 835, The
subprocess created in the install block (the variable process from
asyncio.create_subprocess_exec used with install_command/install_args and
cwd=self.project_dir) can leak when asyncio.wait_for times out; catch
asyncio.TimeoutError around the await asyncio.wait_for(process.communicate(),
...) call, and on timeout synchronously terminate the child (e.g., call
process.kill() or process.terminate()), then await process.wait() to reap it and
finally attempt to collect/close stdout/stderr (e.g., await
process.communicate() or ensure pipes are closed) before re-raising or returning
a timeout error; apply the same pattern to the other installer block referenced
(the similar process handling at the 908-933 section) so no child processes
remain running after a timeout.
Comment on lines
+823
to
+830
| # Execute the install command using asyncio | ||
| process = await asyncio.create_subprocess_exec( | ||
| install_command, | ||
| *install_args, | ||
| cwd=self.project_dir, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use platform abstraction utilities for execution/path operations.
Direct subprocess/glob usage here bypasses the centralized execution/path abstraction required by repo policy.
As per coding guidelines, "Use path joining utilities and execution utilities from the platform abstraction module instead of manual string concatenation for file paths and command execution".
Also applies to: 1033-1034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/command_executor.py` around lines 823 - 830,
Replace the direct call to asyncio.create_subprocess_exec and any manual use of
self.project_dir when launching install_command with the repository's platform
abstraction utilities: call the platform execution helper (e.g.,
Platform.execute or platform.run_subprocess) passing install_command and
install_args, and supply the working directory using the platform path-join
utility (e.g., platform.path.join(self.project_dir, ...)) instead of raw
strings; update error/stdout/stderr handling to use the execution helper's
returned objects. Locate the call site using symbols
asyncio.create_subprocess_exec, install_command, install_args, and
self.project_dir in command_executor.py and make the same replacement for the
similar invocation around the other referenced site (the code near the other
occurrence indicated by the review comment). Ensure imports reference the
platform abstraction module and remove direct asyncio subprocess imports if no
longer needed.
Comment on lines
+1004
to
+1011
| ("pipenv", ["Pipfile.lock", "Pipfile"]), | ||
| ("poetry", ["poetry.lock", "pyproject.toml"]), | ||
| ("hatch", ["pyproject.toml"]), # hatch uses pyproject.toml | ||
| ("pdm", ["pdm.lock", "pyproject.toml"]), | ||
| ("uv", ["uv.lock"]), | ||
| ("conda", ["environment.yml", "conda.yml"]), | ||
| ("pip", ["requirements.txt", "setup.py", "pyproject.toml"]), | ||
| # Rust |
Contributor
There was a problem hiding this comment.
Package manager detection is too heuristic and can misclassify pyproject.toml projects.
Mapping pyproject.toml to multiple managers can execute the wrong installer (for example, Poetry/Hatch/PDM on non-Poetry projects).
As per coding guidelines, "Dynamically detect project stack using context/project_analyzer.py to determine allowed commands. Never hardcode command allowlists - use core/security.py for security hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/command_executor.py` around lines 1004 - 1011,
The hardcoded package-manager-to-file mapping in command_executor.py can
misclassify projects (e.g., mapping pyproject.toml to multiple managers);
replace this heuristic with a dynamic check by calling the project detection in
context/project_analyzer.py to determine the actual project stack and allowed
installers, and then consult core/security.py to fetch the allowed command
whitelist for that detected stack before selecting an installer; locate the
mapping/table in command_executor.py (the tuple list containing "pipenv",
"poetry", "hatch", "pdm", "uv", "conda", "pip") and remove/replace its usage so
installer selection flows from project_analyzer detection -> core/security
allowed commands.
Comment on lines
+96
to
+98
| # Supported command types | ||
| SUPPORTED_COMMANDS = ["merge", "resolve", "process"] | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid hardcoded command allowlists in parser policy.
The parser bakes command policy in code (SUPPORTED_COMMANDS), which makes policy drift likely and bypasses central security hooks.
As per coding guidelines, "Dynamically detect project stack using context/project_analyzer.py to determine allowed commands. Never hardcode command allowlists - use core/security.py for security hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/command_parser.py` around lines 96 - 98, Remove
the hardcoded SUPPORTED_COMMANDS list and instead query the project stack via
context/project_analyzer (e.g., call the stack detection function used in the
repo) and then ask core/security for the allowed commands for that stack;
replace usages of SUPPORTED_COMMANDS in command_parser.py with the dynamic
result (or call a core/security helper like get_allowed_commands(stack) or
is_command_allowed(command, stack)) so policy is enforced centrally rather than
baked into SUPPORTED_COMMANDS.
Comment on lines
+1719
to
+1722
| except Exception as e: | ||
| logger.error(f"Failed to process commands for PR #{pr_number}: {e}") | ||
| # Return empty list on error (caller can handle) | ||
| return [] |
Contributor
There was a problem hiding this comment.
Do not silently convert processing failures into “no commands.”
Returning [] on exceptions makes operational failures indistinguishable from legitimate “no command found” cases.
🧭 Proposed fix
except Exception as e:
- logger.error(f"Failed to process commands for PR #{pr_number}: {e}")
- # Return empty list on error (caller can handle)
- return []
+ logger.exception(f"Failed to process commands for PR #{pr_number}")
+ return [
+ CommandResult(
+ success=False,
+ command_type="system",
+ message="✗ Failed to process PR commands",
+ error=str(e),
+ )
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/runners/github/orchestrator.py` around lines 1719 - 1722, The
except block that currently logs and returns an empty list should not swallow
errors; change the handler in the except Exception as e block so it logs the
full exception (use logger.exception or include traceback) and then re-raises
the exception instead of returning []; i.e., replace logger.error(f"Failed to
process commands for PR #{pr_number}: {e}") and return [] with
logger.exception("Failed to process commands for PR #%s", pr_number) followed by
raise so callers can distinguish operational failures from legitimate
empty-command results.
Comment on lines
+675
to
+680
| # Get the logger and clear any existing handlers | ||
| executor_logger = logging.getLogger('runners.github.command_executor') | ||
| executor_logger.handlers.clear() | ||
| executor_logger.addHandler(handler) | ||
| executor_logger.setLevel(logging.INFO) | ||
| executor_logger.propagate = False # Don't propagate to parent loggers |
Contributor
There was a problem hiding this comment.
Restore logger state after audit-log test.
This test clears/replaces handlers and propagate but does not restore previous logger state, which can affect later tests.
🧪 Proposed fix
- executor_logger = logging.getLogger('runners.github.command_executor')
- executor_logger.handlers.clear()
- executor_logger.addHandler(handler)
- executor_logger.setLevel(logging.INFO)
- executor_logger.propagate = False # Don't propagate to parent loggers
+ executor_logger = logging.getLogger("runners.github.command_executor")
+ old_handlers = list(executor_logger.handlers)
+ old_level = executor_logger.level
+ old_propagate = executor_logger.propagate
+ executor_logger.handlers = [handler]
+ executor_logger.setLevel(logging.INFO)
+ executor_logger.propagate = False # Don't propagate to parent loggers
...
- # Clean up
- executor_logger.removeHandler(handler)
+ # Clean up
+ executor_logger.handlers = old_handlers
+ executor_logger.setLevel(old_level)
+ executor_logger.propagate = old_propagateAlso applies to: 697-698
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/tests/integration/test_github_commands.py` around lines 675 -
680, The test mutates the logger for 'runners.github.command_executor'
(executor_logger), clearing handlers, adding a test handler, setting level and
propagate, but never restores the original state; capture the original
executor_logger.handlers (copy list), executor_logger.level, and
executor_logger.propagate before changing them, then ensure you restore those
originals after the test (use a try/finally or test teardown) and remove the
temporary handler; apply the same capture/restore around the other occurrences
noted (lines ~697-698) so other tests are not affected.
Comment on lines
+3
to
+8
| ## Task | ||
| Implement command extraction for /merge, /resolve, /process | ||
|
|
||
| ## Status | ||
| ✅ **COMPLETED** | ||
|
|
Contributor
There was a problem hiding this comment.
Fix markdown heading spacing to satisfy MD022.
Line 3 and Line 6 headings are missing required surrounding blank lines, which will keep markdownlint noisy.
🛠️ Proposed fix
## Task
+
Implement command extraction for /merge, /resolve, /process
## Status
+
✅ **COMPLETED**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Task | |
| Implement command extraction for /merge, /resolve, /process | |
| ## Status | |
| ✅ **COMPLETED** | |
| ## Task | |
| Implement command extraction for /merge, /resolve, /process | |
| ## Status | |
| ✅ **COMPLETED** | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SUBTASK_1_2_COMPLETION.md` around lines 3 - 8, Add missing blank lines
surrounding the markdown headings "## Task" and "## Status" so they each have an
empty line before and after the heading (i.e., ensure a blank line precedes "##
Task" and that both "## Task" and "## Status" are followed and preceded by blank
lines as appropriate) to satisfy MD022.
Comment on lines
+13
to
+14
| ### File Modified | ||
| - `apps/backend/runners/github/command_parser.py` |
Contributor
There was a problem hiding this comment.
Address markdownlint warnings (MD022/MD031/MD040).
Headings/fences need surrounding blank lines, and the fenced commit block at Line 108 should declare a language.
🧹 Proposed fix
### File Modified
+
- `apps/backend/runners/github/command_parser.py`
**Before:**
+
```python
COMMAND_PATTERN = re.compile(r"/(\w+)(?:\s+([^\n]*?))?(?=\s|$|/)")After:
+
COMMAND_PATTERN = re.compile(r"(?<!/)/(\S+?)(?:\s+([^\n]*?))?(?=\s|$|/)")
MALFORMED_PATTERN = re.compile(r"/[^\w\s]|/\d+|//+")- +text
f060316 - auto-claude: subtask-1-3 - Add edge case handling (empty input, malformed commands, special chars)
Also applies to: 50-58, 108-110
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SUBTASK_1_3_COMPLETION_SUMMARY.md` around lines 13 - 14, Update the
COMMAND_PATTERN regex to the safer form and add MALFORMED_PATTERN: replace the
existing COMMAND_PATTERN with the new pattern
(?<!/)/(\S+?)(?:\s+([^\n]*?))?(?=\s|$|/) and add MALFORMED_PATTERN =
re.compile(r"/[^\w\s]|/\d+|//+") in command_parser.py to handle malformed and
edge-case inputs; also ensure Markdown fences and headings have surrounding
blank lines and change the fenced commit block language from ``` to ```text so
the commit block at the specified section declares a language.
Comment on lines
+86
to
+91
| The edge case handling prevents several security issues: | ||
| - **Command injection**: Special chars in arguments are sanitized | ||
| - **Malformed commands**: Suspicious patterns are logged and skipped | ||
| - **Numeric commands**: Prevents confusion with other numbers | ||
| - **Double slashes**: Prevents path traversal attempts | ||
|
|
Contributor
There was a problem hiding this comment.
Security claim at Line 90 is inaccurate.
“Double slashes prevents path traversal attempts” is not a valid path traversal control in this context and overstates protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SUBTASK_1_3_COMPLETION_SUMMARY.md` around lines 86 - 91, The claim "Double
slashes: Prevents path traversal attempts" overstates protection; update the
summary block that lists edge case handling (the bullet labeled "Double
slashes") to either remove the assertion or rephrase it to a precise statement
(e.g., "Normalizes or rejects paths with double slashes to reduce certain
malformed path cases" or "May mitigate some simple traversal patterns when
combined with proper normalization and validation") and ensure the surrounding
header or list does not imply complete path traversal protection without
mentioning that proper path normalization and validation are required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This feature extends the existing GitHub PR workflow to support command execution capabilities alongside AI review. Currently, the system only performs AI review of pull requests. This enhancement will enable users to execute specific commands through PR comments, including merging branches, resolving dependencies, and processing comments. The implementation will build upon the existing GHClient infrastructure and security model.
Summary by CodeRabbit
New Features
/merge,/resolve, and/processoperations.Chores