feat(pr-generator-orchestrator): implement iterative bug-fixing state machine and container worker entrypoint - #28433
Conversation
|
📊 PR Size: size/XL
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the complete orchestration framework for an automated code generation pipeline, designed to transform issues into pull requests autonomously. It encompasses the entire workflow from initial issue intake and concurrency management to iterative AI-driven code development, rigorous evaluation, and final GitHub PR submission. The system is built to be deployed as a Cloud Run Job, leveraging Google Cloud services and AI agents to streamline the development process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the caretaker-agent orchestrator, which automates bug-fixing and pull request generation using Google Antigravity AI agents, Firestore synchronization, and GitHub integration. The code review highlights critical security vulnerabilities, primarily centered around command injection risks in orchestrator.py and command_executor.py due to direct shell interpolation of untrusted inputs (such as issue numbers, repository URLs, and LLM-generated commit messages). To address these, the reviewer recommends refactoring CommandExecutor.run to support executing commands as argument lists with shell=False. Other feedback includes sanitizing file paths against path traversal, restricting agent policies to mitigate prompt injection, and centralizing environment variable normalization to avoid scattered logic.
…container worker entrypoint
75972b8 to
82a8800
Compare
joneba-google
changed the title
joneba-google
changed the title
gemini-cli
Bot
added
the
status/need-issue
| from command_executor import ( | ||
| CommandExecutor, | ||
| CommandExecutionError, | ||
| sanitize_identifier, | ||
| sanitize_relative_path, | ||
| ) |
There was a problem hiding this comment.
Importing non-existent functions sanitize_identifier and sanitize_relative_path from command_executor will cause a fatal ImportError on startup.
Recommended fix:
Remove them from the import statement.
There was a problem hiding this comment.
I forgot to push the change for that file! I recently updated pr-generator-core with the change.
| from db import ( | ||
| acquire_lock, | ||
| release_lock, | ||
| mark_pr_created, | ||
| mark_needs_human, | ||
| ClaimAction, | ||
| IssueStatus, | ||
| ) |
There was a problem hiding this comment.
Importing directly from db will fail with an ImportError because the db directory has no __init__.py exposing the symbols. Imports must be from db.db_interface.
Recommended fix:
from db.db_interface import (
acquire_lock,
release_lock,
mark_pr_created,
mark_needs_human,
ClaimAction,
IssueStatus,
)| try: | ||
| CommandExecutor.run(["git", "checkout", "-B", branch_name, "origin/main"], self.config.pr_repo_path) | ||
| except CommandExecutionError as e: | ||
| raise OrchestrationError(f"Failed to checkout feature branch {branch_name}: {e}") from e |
There was a problem hiding this comment.
Nit: Add a TODO in the code for responding to user comments.
# TODO: Add logic to fetch and checkout the existing branch if responding to user feedback
CommandExecutor.run(["git", "checkout", "-B", branch_name, "origin/main"], self.config.pr_repo_path)| if claim_action == ClaimAction.NEEDS_HUMAN: | ||
| logging.warning( | ||
| "Triage attempts exceeded maximum allowed limit. Issue moved to NEEDS_HUMAN. Exiting." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Exiting with sys.exit(1) when the lock claim fails due to max attempts reached causes the Cloud Run job to report failure, triggering unnecessary workflow retries and overwriting the Firestore state.
Recommended fix:
if claim_action == ClaimAction.NEEDS_HUMAN:
logging.warning(
"Triage attempts exceeded maximum allowed limit. Issue moved to NEEDS_HUMAN. Exiting."
)
return| if commit_line_count > 500: | ||
| logging.error( | ||
| "Verdict: APPROVED but modified line size (%s) exceeds 500 limit. Moving to NEEDS_HUMAN.", | ||
| commit_line_count, | ||
| ) | ||
| try: | ||
| mark_needs_human( | ||
| lock_holder=execution_id, | ||
| reason=f"Commit modifications ({commit_line_count} lines) exceed 500 lines limit.", | ||
| doc_id=doc_id, | ||
| owner=owner, | ||
| repo=repo, | ||
| issue_number=issue_num, | ||
| ) | ||
| except Exception as e: | ||
| logging.error("Failed to update Firestore status to NEEDS_HUMAN: %s", e) | ||
| sys.exit(2) |
There was a problem hiding this comment.
Exiting with sys.exit(2) when the commit line count exceeds 500 causes the workflow to trigger failure handling, which overwrites the specific NEEDS_HUMAN status and reason in Firestore.
Recommended fix:
if commit_line_count > 500:
logging.error(
"Verdict: APPROVED but modified line size (%s) exceeds 500 limit. Moving to NEEDS_HUMAN.",
commit_line_count,
)
try:
mark_needs_human(
lock_holder=execution_id,
reason=f"Commit modifications ({commit_line_count} lines) exceed 500 lines limit.",
doc_id=doc_id,
owner=owner,
repo=repo,
issue_number=issue_num,
)
except Exception as e:
logging.error("Failed to update Firestore status to NEEDS_HUMAN: %s", e)
return| eslint_cmd = ["npx", "eslint"] + changed_files + ["--max-warnings", "0"] | ||
| eslint_env = {**os.environ, "NODE_OPTIONS": "--max-old-space-size=4096"} | ||
| try: | ||
| lint_result = CommandExecutor.run( | ||
| eslint_cmd, self.config.eval_repo_path, env=eslint_env | ||
| ) |
There was a problem hiding this comment.
Calling CommandExecutor.run with a list eslint_cmd while it executes with shell=True will execute only the first item in the list, causing the command to fail or behave unexpectedly.
Recommended fix:
eslint_files = " ".join(changed_files)
eslint_cmd = f"npx eslint --max-warnings 0 --no-error-on-unmatched-pattern --no-warn-ignored {eslint_files}"
eslint_env = {**os.environ, "NODE_OPTIONS": "--max-old-space-size=4096"}
try:
lint_result = CommandExecutor.run(
eslint_cmd, self.config.eval_repo_path, env=eslint_env
)There was a problem hiding this comment.
Thank you for catching this. A recent change to CommandExecutor set use_shell to isinstance(cmd, str), so it should execute properly now. I implemented your recommended flags.
| except CommandExecutionError as e: | ||
| logging.error("Failed to push git branch: %s", e) | ||
| sys.exit(3) |
There was a problem hiding this comment.
Using sys.exit(3) inside business logic helper methods bypasses structured exception handling and leaves locks uncleaned. It should raise OrchestrationError.
Recommended fix:
except CommandExecutionError as e:
logging.error("Failed to push git branch: %s", e)
raise OrchestrationError(f"Failed to push git branch: {e}") from e| except GitHubClientError as e: | ||
| logging.error("Pull request submission failed: %s", e) | ||
| sys.exit(3) |
There was a problem hiding this comment.
Using sys.exit(3) on PR creation failure bypasses exception handling and leaves locks uncleaned. It should raise OrchestrationError.
Recommended fix:
except GitHubClientError as e:
logging.error("Pull request submission failed: %s", e)
raise OrchestrationError(f"Pull request submission failed: {e}") from e| def setup_logging() -> None: | ||
| """Sets up the root logger with a standardized format.""" | ||
| logging.basicConfig( | ||
| level=logging.INFO, | ||
| format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", | ||
| handlers=[logging.StreamHandler(sys.stdout)], | ||
| ) | ||
| logging.getLogger().addFilter(IgnoreRawWsMsgFilter()) |
There was a problem hiding this comment.
In Python logging, filters added to a logger (including the root logger) are bypassed for records that propagate from child loggers. Only filters added directly to the logging handlers will filter all propagated records. As a result, logs generated via child loggers (e.g. inside orchestrator.py or config.py) containing 'RAW WS MSG' will not be filtered.
Recommended fix:
def setup_logging() -> None:
"""Sets up the root logger with a standardized format."""
handler = logging.StreamHandler(sys.stdout)
handler.addFilter(IgnoreRawWsMsgFilter())
logging.basicConfig(
level=logging.INFO,
format="%(asctime)s [%(levelname)s] %(name)s: %(message)s",
handlers=[handler],
)| class IgnoreRawWsMsgFilter(logging.Filter): | ||
| def filter(self, record: logging.LogRecord) -> bool: | ||
| return "RAW WS MSG" not in record.getMessage() |
There was a problem hiding this comment.
Class 'IgnoreRawWsMsgFilter' is missing a docstring.
Recommended fix:
class IgnoreRawWsMsgFilter(logging.Filter):
"""Filter to ignore raw websocket messages in the log output."""
def filter(self, record: logging.LogRecord) -> bool:
return "RAW WS MSG" not in record.getMessage()| diff_stat = CommandExecutor.run("git diff --stat origin/main", self.config.pr_repo_path) | ||
| logging.info("Diff Stat summary:\n%s", diff_stat) | ||
| lines = diff_stat.split("\n") | ||
| last_line = lines[-1] if lines else "" |
There was a problem hiding this comment.
Executing diff_stat.split("\n") on git diff --stat output with a trailing newline makes lines[-1] an empty string "". Regex matching for insertions/deletions on lines[-1] fails, resulting in commit_line_count = 0 and completely bypassing the 500-line safety cap (if commit_line_count > 500:).
Suggested Fix:
lines = diff_stat.strip().split("\n")
last_line = lines[-1] if lines else ""
insertions = re.search(r"(\d+)\s+insertion", last_line)
deletions = re.search(r"(\d+)\s+deletion", last_line)|
|
||
| # Parse recommended PR Description (case-insensitive) | ||
| desc_match = re.search( | ||
| r"##\s*PR\s*Description\r?\n\s*(.+)", |
There was a problem hiding this comment.
desc_match uses (.+) with re.DOTALL without a lookahead delimiter. If ## PR Description appears before other headers in pr_details.md, desc_match captures the rest of the file.
Suggested Fix:
desc_match = re.search(
r"##\s*PR\s*Description\r?\n\s*(.+?)(?=\r?\n##|$)",
details_content,
re.IGNORECASE | re.DOTALL,
)
adamfweidman
left a comment
adamfweidman
left a comment
There was a problem hiding this comment.
Overall looks good but please add unit tests
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
joneba-google
added
the
help wanted
| logging.info("Cleaning up evaluation repository path: %s", self.config.eval_dir) | ||
| if os.path.exists(self.config.eval_dir): | ||
| try: | ||
| shutil.rmtree(self.config.eval_dir) |
There was a problem hiding this comment.
If shutil.rmtree failed with an OSError, would stale evaluation files leak across runs since os.makedirs with exist_ok=True keeps existing contents?
2 participants
Overview
This PR implements the main application orchestration layer and asynchronous container entrypoint for the Gemini CLI SSR Pipeline. It coordinates Firestore concurrency locking, iterative AI agent coding and evaluation loops, ESLint static analysis, diff limit verification, and automated GitHub Pull Request submissions.
──────
Summary of Changes
1. Container Worker Entrypoint (worker.py)
• Asynchronous Main Runner (worker.py): Initializes the config.py object and executes the asynchronous orchestrator.py state machine.
• Centralized Logging: Establishes standardized stream logging with a custom log filter (worker.py) to eliminate raw WebSocket noise from container stdout.
• Structured Exit Codes: Maps orchestrator.py and unhandled exceptions to standardized exit codes for Cloud Run monitoring.
2. Orchestrator State Machine (orchestrator.py)
• Firestore Dual-Lock Validation: Invokes db_interface.py to claim issue leases (status: COMMIT_GENERATION), exiting cleanly if locked by another active workflow or escalating to NEEDS_HUMAN when exceeding
retry thresholds.
• Git Workspace Management: Clones/syncs target repositories, checks out feature branches (ssr-agent-<issue_num>), and configures git exclude rules to protect internal state files (firestore_doc.json,
changes.diff, verdict.json).
• Iterative Dual-Agent Loop:
• Coding Agent Phase: Prompts the Antigravity Coding Agent to implement code fixes and test assertions based on the Firestore issue specification or previous loop feedback (pr_feedback.md).
• Evaluation Agent Phase: Syncs workspace modifications into an isolated evaluation environment, runs ESLint checks, and assesses test verdicts.
• Evaluation Performance Optimization: Reuses node_modules from the PR workspace via filesystem symlinks (orchestrator.py), bypassing redundant 1–3+ minute npm ci installs on every loop iteration.
• Diff Limits & PR Publishing:
• Enforces a 500-line modified code limit; patches exceeding 500 lines are routed to NEEDS_HUMAN.
• Injects in-memory HTTP Basic Authorization headers (GIT_CONFIG_COUNT, GIT_CONFIG_KEY_0, GIT_CONFIG_VALUE_0) during git push to prevent plaintext token exposure in .git/config.
• Submits Pull Requests via github_client.py and marks Firestore documents as PR_EVALUATION_PENDING.