feat(pr-generator-core): add environment config parser, command executor, GitHub R…#28435
feat(pr-generator-core): add environment config parser, command executor, GitHub R…#28435joneba-google wants to merge 1 commit into
Conversation
…EST client, and preflight filter
|
📊 PR Size: size/L
|
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 establishes the foundational infrastructure for the Gemini CLI Issue-to-PR Code Generation Pipeline. It introduces modular utilities for managing environment configurations, executing system commands, interacting with the GitHub API, and filtering test outputs. These components are designed to work within a containerized environment, ensuring reliable orchestration of the code generation and evaluation workflow. 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 workflow package for the SSR Agent Orchestrator, adding configuration, command execution, GitHub API integration, and preflight test filtering. The review comments correctly identify several critical issues: a potential remote code execution (RCE) vulnerability from globally trusting workspaces, command injection risks from using shell=True, a missing timeout on GitHub API requests, and brittle regex patterns in the preflight filter that fail to handle monorepo paths and standard Jest/Vitest test summaries.
| os.environ["GEMINI_CLI_WORKSPACE_TRUSTED"] = "true" | ||
| os.environ["GEMINI_CLI_TRUST_WORKSPACE"] = "true" |
There was a problem hiding this comment.
Globally setting GEMINI_CLI_WORKSPACE_TRUSTED and GEMINI_CLI_TRUST_WORKSPACE to "true" instructs the Gemini CLI to trust any workspace it runs in. Since this orchestrator is designed to process untrusted pull requests and clone external repositories, trusting the workspace globally allows a malicious pull request to include crafted configuration files or hooks that the Gemini CLI will execute automatically, leading to Remote Code Execution (RCE) within the container.
| os.environ["GEMINI_CLI_WORKSPACE_TRUSTED"] = "true" | |
| os.environ["GEMINI_CLI_TRUST_WORKSPACE"] = "true" | |
| # Avoid globally trusting untrusted workspaces to prevent RCE from malicious PRs. |
| result = subprocess.run( | ||
| cmd, | ||
| shell=True, | ||
| cwd=cwd, | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| check=False | ||
| ) |
There was a problem hiding this comment.
CommandExecutor.run executes shell commands using subprocess.run with shell=True. While this supports shell features, executing commands with untrusted inputs (such as branch names, PR titles, or repository URLs) poses a security risk. Note that untrusted inputs must be sanitized, escaped, and wrapped at the ingestion service layer before being passed to downstream components like this orchestrator, rather than performing sanitization in this downstream orchestration logic. Additionally, consider avoiding shell=True to prevent command injection.
References
- Sanitize, escape, and wrap untrusted inputs at the ingestion service layer before publishing to Pub/Sub or passing to downstream components, rather than performing sanitization in the downstream orchestration or prompt construction logic.
| "Sending Pull Request creation request for branch: %s", branch_name | ||
| ) | ||
| try: | ||
| with urllib.request.urlopen(req) as response: |
There was a problem hiding this comment.
The urllib.request.urlopen call is made without a specified timeout. In a production environment like Cloud Run, network requests without a timeout can block indefinitely if the remote server hangs or experiences high latency. This can lead to resource exhaustion and container hangs.
Please specify a reasonable timeout (e.g., timeout=30) to ensure the request fails fast if the GitHub API is unresponsive.
| with urllib.request.urlopen(req) as response: | |
| with urllib.request.urlopen(req, timeout=30) as response: |
| # Regex search for failing files of format "FAIL src/..." | ||
| failing_files = set(re.findall(r"FAIL\s+(src/[^\s>]+)", clean_output)) | ||
| logging.info("Analyzing failing test files: %s", failing_files) | ||
|
|
||
| allowed_failures = { | ||
| "src/utils/sessionCleanup.test.ts", | ||
| "src/config/extension-manager-permissions.test.ts", | ||
| } | ||
|
|
||
| if not failing_files: | ||
| logging.info("No failing test files matched.") | ||
| return False | ||
|
|
||
| # If failures exist that are not inside our allowed list, we cannot ignore | ||
| if not failing_files.issubset(allowed_failures): | ||
| logging.warning( | ||
| "Unapproved test failures detected: %s", | ||
| failing_files - allowed_failures, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
In a monorepo structure, test files are typically located within sub-packages (e.g., packages/core/src/... or packages/cli/src/...). The current regex FAIL\s+(src/[^\s>]+) strictly expects the path to start with src/, which will fail to match any test failures when run from the monorepo root. This means allowed failures will not be detected, and the preflight bypass logic will fail.
Additionally, simply filtering out unapproved failures from the matched list can be dangerous if we only match a subset of failures. Instead, we should capture all failing paths and verify that every single one of them ends with one of the allowed failure paths.
# Regex search for failing files of format "FAIL [path]"
failing_files = set(re.findall(r"FAIL\s+(\S+)", clean_output))
logging.info("Analyzing failing test files: %s", failing_files)
allowed_failures = {
"src/utils/sessionCleanup.test.ts",
"src/config/extension-manager-permissions.test.ts",
}
if not failing_files:
logging.info("No failing test files matched.")
return False
# If failures exist that are not inside our allowed list, we cannot ignore
unapproved_failures = {
f for f in failing_files
if not any(f.endswith(allowed) for allowed in allowed_failures)
}
if unapproved_failures:
logging.warning(
"Unapproved test failures detected: %s",
unapproved_failures,
)
return False| return False | ||
|
|
||
| # Find total test failure count summary in JEST style output: e.g. "Tests: 3 failed, 4 passed" | ||
| match = re.search(r"Tests\s+(\d+)\s+failed", clean_output) |
There was a problem hiding this comment.
The regex Tests\s+(\d+)\s+failed expects whitespace immediately after Tests. However, standard Jest and Vitest test summaries format this as Tests: 3 failed, 4 passed (with a colon :). Because \s does not match a colon, this regex will fail to match standard test summaries, causing the bypass logic to always return False.
Updating the regex to Tests:?\s*(\d+)\s+failed makes the colon optional and robustly handles any whitespace variations.
| match = re.search(r"Tests\s+(\d+)\s+failed", clean_output) | |
| match = re.search(r"Tests:?\s*(\d+)\s+failed", clean_output) |
gemini-cli
Bot
added
the
status/need-issue
joneba-google
changed the title
1 participant
Overview
This PR introduces the foundational utility modules and package structure for the Gemini CLI SSR Pipeline. It packages configuration parsing, subprocess execution with structured logging, GitHub v3 REST API client integration, and ANSI preflight test output filtering.
──────
Summary of Changes
1. Package Initialization (init.py)
• Establishes the workflow package definition and documents the core module architecture.
2. Environment Configuration (config.py)
• Environment Parser (config.py): Parses and validates required GCP environment variables (REPO_URL, GIT_TOKEN, FIRESTORE_DOC, FIRESTORE_ID, EXECUTION_ID, GOOGLE_CLOUD_PROJECT, MAX_ATTEMPTS).
• Path Resolution: Dynamically configures temporary PR and evaluation workspace paths (/tmp/pr/, /tmp/eval/).
• Schema Validation (config.py): Ensures FIRESTORE_DOC contains valid JSON and raises structured config.py on failure.
3. Subprocess Execution Utility (command_executor.py)
• Safe Command Runner (command_executor.py): Executes system shell commands, logs execution parameters and working directories, and cleanly captures standard streams.
• Error Handling: Raises command_executor.py with exit codes, stdout, and stderr on command failures.
4. GitHub REST API Client (github_client.py)
• Lightweight REST Integration (github_client.py): Uses standard urllib to avoid heavy external HTTP client dependencies in the container.
• Automated PR Submission (github_client.py): Authenticates using Bearer tokens and submits POST requests to GitHub v3 REST API endpoints (/repos/{owner}/{repo}/pulls), returning the created PR URL.
5. Preflight Test & Lint Output Filter (preflight_filter.py)
• ANSI Sanitization (preflight_filter.py): Removes terminal escape sequences using a module-level precompiled regex constant (_ANSI_ESCAPE_RE).
• Failure Analysis (preflight_filter.py): Analyzes test output traces to selectively bypass known sandbox-level root privilege test failures while flagging unapproved regressions.