fix(commands): bound pr-review and context-hub-setup with output and iteration limits#1671
Conversation
PR Validation ReportCaution ❌ Status: FAIL Description Validation
PR Standards
QA Validation
|
Session Protocol Compliance ReportCaution ❌ Overall Verdict: CRITICAL_FAIL All session protocol requirements satisfied. What is Session Protocol?Session logs document agent work sessions and must comply with RFC 2119 requirements:
See Compliance Summary
Detailed Validation ResultsClick each session to see the complete validation report with specific requirement failures. 📄 sessions-2026-04-18-session-01-issue-1668-output-constraints=== Session Validation === [FAIL] Validation errors:
✨ Zero-Token ValidationThis validation uses deterministic script analysis instead of AI:
Powered by 📊 Run Details
Powered by Session Protocol Validator workflow |
AI Quality Gate ReviewTip ✅ Final Verdict: PASS WalkthroughThis PR was reviewed by six AI agents in parallel, analyzing different aspects of the changes:
Review Summary
💡 Quick Access: Click on individual agent jobs (e.g., "🔒 security Review", "🧪 qa Review") in the workflow run to see detailed findings and step summaries. Security Review DetailsThe Now I have a complete picture. Let me review the security aspects: Security Review: PR #1671PR Category: CODE (Python)Changed files:
Findings
AnalysisCWE-22 Path Traversal Protection: [PASS]
Input Validation: [PASS]
YAML Parsing: [PASS]
Error Handling: [PASS]
Test Coverage: [PASS]
RecommendationsNone required. Verdict{
"verdict": "PASS",
"message": "CWE-22 path validation correctly implemented using Path.resolve() + relative_to(). Type guards and YAML safe_load prevent injection.",
"agent": "security",
"timestamp": "2026-04-27T00:00:28.441Z",
"findings": []
}QA Review DetailsBased on my analysis of the PR, here is my QA review: QA Review: PR #1671Test Coverage Assessment
New Function Coverage
Code Quality
Quality Concerns
Regression Risk Assessment
{
"verdict": "PASS",
"message": "Comprehensive test coverage for all new validation logic with proper edge case handling",
"agent": "qa",
"timestamp": "2026-04-27T00:02:49Z",
"findings": [
{
"severity": "low",
"category": "code-quality",
"description": "summary_required_columns validation triggers on None unlike other optional fields",
"location": "scripts/validate_pr_review_config.py:259-267",
"recommendation": "Intentional design choice for required field; no change needed"
}
]
}Analyst Review DetailsCode Quality Score
Overall: 5/5 Impact Assessment
Findings
Recommendations
Verdict{
"verdict": "PASS",
"message": "Well-structured CWE-22 hardening and schema validation with comprehensive test coverage (35 tests covering type guards, range checks, and path traversal).",
"agent": "analyst",
"timestamp": "2026-04-27T00:01:27.520Z",
"findings": [
{
"severity": "low",
"category": "consistency",
"description": "summary_required_columns validation checks cols as falsy rather than explicit None check before validation",
"location": "scripts/validate_pr_review_config.py:259-267",
"recommendation": "Optional: align with pattern used for max_tokens/allowed fields for consistency"
}
]
}Architect Review DetailsI have sufficient context for the architectural review. Let me provide my analysis. Architect Review: PR #1671Design Quality Assessment
Overall Design Score: 4.8/5 Architectural Concerns
Breaking Change Assessment
Technical Debt Analysis
ADR Assessment
Recommendations
Verdict{
"verdict": "PASS",
"message": "Design is sound. CWE-22 hardening and schema validation follow established patterns with comprehensive test coverage.",
"agent": "architect",
"timestamp": "2026-04-26T23:59:04.453Z",
"findings": [
{
"severity": "low",
"category": "design-pattern",
"description": "Validation helpers for invocation_limits and output_constraints share similar structure",
"location": "scripts/validate_pr_review_config.py:175-267",
"recommendation": "Consider extracting generic typed-field validator; non-blocking as current pattern is clear"
}
]
}DevOps Review DetailsNow let me check the test file and verify the path validation handles null bytes: Now I have a complete picture. Let me provide the DevOps review: PR #1671 DevOps ReviewPR Scope DetectionCategory: CODE (Python scripts in Pipeline Impact Assessment
CI/CD Quality Checks
Script Quality Assessment
Findings
Security AnalysisThe CWE-22 hardening is well-implemented:
Test Coverage
Automation Opportunities
Recommendations
Verdict{
"verdict": "PASS",
"message": "Script hardening and schema validation are well-implemented with comprehensive test coverage.",
"agent": "devops",
"timestamp": "2026-04-26T23:58:35.461Z",
"findings": [
{
"severity": "low",
"category": "shell-quality",
"description": "Null-byte injection test relies on OS rejection rather than explicit check in validate_safe_path",
"location": "tests/test_validate_pr_review_config.py:337-347",
"recommendation": "Consider adding explicit null-byte detection in validate_safe_path for defense-in-depth"
}
]
}Roadmap Review DetailsStrategic Alignment Assessment
Feature Completeness
Impact Analysis
Concerns
Recommendations
Verdict{
"verdict": "PASS",
"message": "Bug fix PR delivers CWE-22 path validation hardening and schema guards with 35 tests. Aligns with security gate objectives and infrastructure reliability goals.",
"agent": "roadmap",
"timestamp": "2026-04-27T00:00:24.629Z",
"findings": [
{
"severity": "low",
"category": "documentation",
"description": "PR notes pre-existing test failures on main (workspace-budget tests) that are out of scope",
"location": "PR description",
"recommendation": "Track workspace-budget cleanup in a separate issue if not already tracked"
}
]
}Run Details
Powered by AI Quality Gate workflow |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive completion criteria, task budgets, and output constraints for the context hub setup and PR review workflows. Key changes include capping the number of processed pull requests, defining retry limits for completion gates, and standardizing summary output formats. Review feedback identified a logical contradiction between immediate failure reporting and retry budgets in the setup documentation, as well as an inconsistency in the placeholder naming convention for pull request numbers.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConfig validation script adds optional top-level sections Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.claude/commands/pr-review.md (1)
36-36: Keep limits and summary schema single-sourced from config.Line 36 and Line 67 restate numeric defaults and explicit columns inline. If YAML changes, this prompt can drift from runtime behavior.
Suggested fix
-For `all-open`, query open PRs and cap the list at `invocation_limits.all_open_max_prs` (default 5). If additional PRs remain, record the skipped count and apply `invocation_limits.all_open_overflow_action` in Step 6. For each selected PR number, validate using `scripts.claude_code.get_pr_context` from config. +For `all-open`, query open PRs and cap the list at `invocation_limits.all_open_max_prs`. If additional PRs remain, record the skipped count and apply `invocation_limits.all_open_overflow_action` in Step 6. For each selected PR number, validate using `scripts.claude_code.get_pr_context` from config. -Report per-PR status using `output_constraints.summary_format` (table) with columns from `output_constraints.summary_required_columns`: PR, Branch, Comments, Acknowledged, Implemented, Commit, Status. Truncate per-PR agent output exceeding `output_constraints.per_pr_max_response_lines` (default 120) and persist full detail per `output_constraints.per_pr_overflow_action`. If `all-open` skipped PRs in Step 1, append a row noting the skipped count and direct the user to re-run. +Report per-PR status using `output_constraints.summary_format` with columns from `output_constraints.summary_required_columns`. Truncate per-PR agent output exceeding `output_constraints.per_pr_max_response_lines` and persist full detail per `output_constraints.per_pr_overflow_action`. If `all-open` skipped PRs in Step 1, append a row noting the skipped count and direct the user to re-run.Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/pr-review.md at line 36, The prompt duplicates numeric defaults and schema columns instead of reading them from the config; update the logic to single-source these values by referencing invocation_limits.all_open_max_prs and invocation_limits.all_open_overflow_action (instead of hardcoded defaults) and derive the summary schema/columns from the same config entry used at runtime (rather than embedding explicit columns inline), and ensure each PR number validation still calls scripts.claude_code.get_pr_context; in short, remove inline numeric/default values and explicit column lists and replace them with variables read from the config symbols invocation_limits.* and the config entry that defines the summary schema so the prompt always reflects runtime config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/context-hub-setup.md:
- Around line 167-168: The Step 3 status reporting text is contradictory: it
both mandates the three-line "Component: State" format with no extra commentary
and also shows a "Commands available" section; update the Step 3 instructions so
they are consistent by deciding one behavior and editing the relevant
lines—either remove the "Commands available" subsection from the Step 3 template
(so only three "Component: State" lines are shown) or change the Rule that
currently reads "Status block in Step 3 MUST use the three-line `Component:
State` format... No additional commentary." to allow the "Commands available"
lines; update all occurrences of "Step 3", "Status block", "Component: State",
and "Commands available" in the file to reflect the chosen behavior so the
instructions are non-contradictory.
In @.claude/commands/pr-review-config.yaml:
- Around line 111-129: Update scripts/validate_pr_review_config.py to validate
the newly added top-level sections invocation_limits and output_constraints:
ensure invocation_limits contains keys all_open_max_prs (integer >=1),
all_open_overflow_action (non-empty string), completion_gate_max_retries
(integer >=0), and completion_gate_overflow_action (non-empty string); and
ensure output_constraints contains per_pr_max_response_lines (integer >=1),
per_pr_overflow_action (non-empty string), summary_format (one of the allowed
formats, e.g., "table"), and summary_required_columns (non-empty list of strings
including the required set). Add schema checks, clear error messages naming the
failing key, and include these validations in the existing validate_config()
function so missing or mistyped fields cause validation to fail.
In @.claude/commands/pr-review.md:
- Line 75: The wording for invocation_limits.completion_gate_max_retries is
ambiguous about whether it counts total attempts or only retries after the first
attempt; update the text around completion_criteria and
invocation_limits.completion_gate_max_retries to explicitly define the semantics
(e.g., "max retries after the initial attempt" or "max total attempts including
the initial check") and ensure any references to completion_gate_max_retries
consistently use that term; also mention how completion_gate_overflow_action is
triggered when the cap (as now clarified) is reached and reference
failure_handling and error_recovery for post-cap behavior so there is no
off-by-one confusion.
---
Nitpick comments:
In @.claude/commands/pr-review.md:
- Line 36: The prompt duplicates numeric defaults and schema columns instead of
reading them from the config; update the logic to single-source these values by
referencing invocation_limits.all_open_max_prs and
invocation_limits.all_open_overflow_action (instead of hardcoded defaults) and
derive the summary schema/columns from the same config entry used at runtime
(rather than embedding explicit columns inline), and ensure each PR number
validation still calls scripts.claude_code.get_pr_context; in short, remove
inline numeric/default values and explicit column lists and replace them with
variables read from the config symbols invocation_limits.* and the config entry
that defines the summary schema so the prompt always reflects runtime config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a5d094a9-8f99-4319-bb56-4b8cf404d431
⛔ Files ignored due to path filters (1)
.agents/sessions/2026-04-18-session-01-issue-1668-output-constraints.jsonis excluded by!.agents/sessions/**
📒 Files selected for processing (3)
.claude/commands/context-hub-setup.md.claude/commands/pr-review-config.yaml.claude/commands/pr-review.md
Review Triage RequiredNote Priority: NORMAL - Human approval required before bot responds Review Summary
Next Steps
Powered by PR Maintenance workflow - Add triage:approved label |
rjmurillo
left a comment
There was a problem hiding this comment.
Approved: all checks passing, threads resolved
dfbe920
✅ Pass: Memory Validationadr/adr-007-augmentation-research: no citations 📊 Validation Details
|
Spec-to-Implementation ValidationTip ✅ Final Verdict: PASS What is Spec Validation?This validation ensures your implementation matches the specifications:
Validation Summary
Spec References
Requirements Traceability DetailsRequirements Coverage Matrix
Summary
Gaps
Verification of Scope ReductionThe PR author correctly identified that
Implementation Completeness DetailsNow let me check if the workflow files mentioned in the issue exist (workflow/0-init.md and workflow/2-impl.md): The workflow files (workflow/0-init.md and workflow/2-impl.md) do not exist - they were removed in PR #1611 as noted in the session log. Let me verify by checking if there's a workflow directory: Now I have all the context needed to evaluate the implementation against the acceptance criteria from issue #1668. Acceptance Criteria ChecklistBased on issue #1668's Recommended changes:
Additional Implementation Quality Checks
Missing FunctionalityNone critical. The workflow files mentioned in the issue (workflow/0-init.md, workflow/2-impl.md) were removed in PR #1611 and are correctly out of scope per the session decision log. Edge Cases Not Covered
Implementation Quality
Run Details
Powered by AI Spec Validator workflow |
rjmurillo
left a comment
There was a problem hiding this comment.
Eval dry-run PASS. Classification: 2 prompts, 1 YAML config. All files structurally valid. ADR-057 evidence satisfied.
dfbe920 to
6ba5428
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/validate_pr_review_config.py`:
- Around line 159-175: The current checks assume invocation_limits and
output_constraints are dicts and only verify presence of keys; change the logic
that handles "invocation_limits" and "output_constraints" so you first assert
their types (e.g., if "invocation_limits" in config: if not
isinstance(config["invocation_limits"], dict): errors.append(...)) before
iterating over INVOCATION_LIMIT_FIELDS/OUTPUT_CONSTRAINT_FIELDS, and then
validate each field's value/type/range (e.g., numeric fields are int/float and
within allowed bounds >=0, string fields are non-empty strings). For
output_constraints.summary_required_columns ensure cols is a non-empty list and
that every element is a non-empty string. Use the existing local names il and oc
and append clear validation errors when types or value ranges are invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e6d629e3-8206-4b4f-a9b6-97f1559041fa
⛔ Files ignored due to path filters (1)
.agents/sessions/2026-04-18-session-01-issue-1668-output-constraints.jsonis excluded by!.agents/sessions/**
📒 Files selected for processing (5)
.claude/commands/context-hub-setup.md.claude/commands/pr-review-config.yaml.claude/commands/pr-review.mdscripts/validate_pr_review_config.pytests/test_validate_pr_review_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/commands/pr-review.md
…iteration limits Adds invocation_limits and output_constraints to pr-review-config.yaml: - all-open caps at 5 PRs per invocation with overflow reporting - completion gate caps at 3 retry iterations before escalation - per-PR agent output caps at 120 lines with session-file persistence - summary uses required tabular columns Updates pr-review.md Step 1, Completion Gate, and Step 6 to reference the new config keys so agents enforce the limits at runtime. Adds Completion Criteria, Task Budget, and Output Constraints sections to context-hub-setup.md so setup has a verifiable done-when condition and bounded retries. Fixes #1668 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fix retry contradiction in context-hub-setup.md completion criteria
- Fix {pr} placeholder to {number} for consistency in config
- Clarify Step 3 output rules to allow Commands available block
- Clarify retries vs iterations semantics in completion gate
- Remove inline defaults from pr-review.md, single-source from config
- Add invocation_limits and output_constraints validation to schema
- Add tests for new validation rules
The session log used legacy field names (serenaInit, lintRun, commitCreated, etc.) that the current validate_session_json.py rejects. Renamed to the required schema fields: sessionStart adds serenaActivated, serenaInstructions, branchVerified, notOnMain; sessionEnd renames logComplete -> checklistComplete, lintRun -> markdownLintRun, commitCreated -> changesCommitted, jsonValid -> validationPassed, and adds handoffPreserved + serenaMemoryUpdated. Hardened scripts/validate_pr_review_config.py against CWE-22 path traversal by routing the user-supplied config_path through validate_safe_path against _PROJECT_ROOT before any open(). Matches the pattern already used in scripts/validate_session_json.py. Added 3 CLI tests covering relative traversal, absolute paths outside the repo, and the default-path happy path. Removed unused pytest/Path imports flagged by ruff. Python Security Checks (pip-audit CVE-2026-3219) is resolved by the rebase on origin/main, which now ignores the CVE per workflow update in #1779. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6ba5428 to
b7646d5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_validate_pr_review_config.py (1)
196-202: Add a timeout to the validator subprocess call.Line 197 runs a child process without
timeout. If the validator hangs, this test can stall the suite indefinitely.Suggested patch
def _run(self, *argv: str) -> subprocess.CompletedProcess[str]: return subprocess.run( [sys.executable, str(_VALIDATOR), *argv], capture_output=True, text=True, cwd=str(_REPO_ROOT), + timeout=15, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_validate_pr_review_config.py` around lines 196 - 202, The helper _run currently invokes subprocess.run without a timeout which can hang the test suite; update the subprocess.run call inside _run to include a reasonable timeout (e.g., timeout=30) and handle subprocess.TimeoutExpired by failing the test with a clear message (re-raise a unittest/pytest failure or raise RuntimeError) so timeouts don't hang indefinitely; ensure you modify the _run function where subprocess.run is called to pass the timeout parameter and add a try/except around the call to catch subprocess.TimeoutExpired and produce a deterministic test failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_validate_pr_review_config.py`:
- Around line 196-202: The helper _run currently invokes subprocess.run without
a timeout which can hang the test suite; update the subprocess.run call inside
_run to include a reasonable timeout (e.g., timeout=30) and handle
subprocess.TimeoutExpired by failing the test with a clear message (re-raise a
unittest/pytest failure or raise RuntimeError) so timeouts don't hang
indefinitely; ensure you modify the _run function where subprocess.run is called
to pass the timeout parameter and add a try/except around the call to catch
subprocess.TimeoutExpired and produce a deterministic test failure message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 73c81fd3-e203-4648-93e2-b62b75842294
⛔ Files ignored due to path filters (1)
.agents/sessions/2026-04-18-session-01-issue-1668-output-constraints.jsonis excluded by!.agents/sessions/**
📒 Files selected for processing (5)
.claude/commands/context-hub-setup.md.claude/commands/pr-review-config.yaml.claude/commands/pr-review.mdscripts/validate_pr_review_config.pytests/test_validate_pr_review_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/commands/pr-review-config.yaml
- .claude/commands/pr-review.md
- scripts/validate_pr_review_config.py
PR #1786 (merged to main) shipped the same `invocation_limits` and `output_constraints` config additions and the `context-hub-setup.md` completion criteria that this branch was adding. Resolution: - Take main's version of `pr-review.md`, `pr-review-config.yaml`, and `context-hub-setup.md` (all superseded by #1786). - Drop the stale per-issue session log (`.agents/sessions/2026-04-18-session-01-issue-1668-output-constraints.json`), which referenced the now-superseded shape. Keep the unique-and-still-valuable pieces from this branch and update them to match main's actual schema: - `scripts/validate_pr_review_config.py`: CWE-22 path-traversal guard on the CLI entry point; schema validation for `invocation_limits` and `output_constraints`. Schema constants now reflect main's keys (`per_pr_max_response_tokens`, `summary_format_allowed_values`). - Apply CodeRabbit review feedback (PR #1671): guard against non-mapping values (e.g. `null`), require positive integers for max counts and non-negative integers for retry counts, require non-empty strings for action and format fields, require list entries to be non-empty strings, and assert `summary_format` is a member of `summary_format_allowed_values`. - Tests: update `VALID_CONFIG` to main's schema and add cases covering every new error branch (12 new tests, 35 total, all pass). Refs #1668 Refs #1786 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_validate_pr_review_config.py`:
- Around line 299-323: Add unit tests exercising the new path-hardening for
null-byte and control-character inputs: extend TestCliPathSafety (or add a new
test class) to call the CLI via the existing _run helper (or directly call
validate_safe_path() if available) with inputs containing a trailing null byte
(e.g. "config.yaml\x00") and with control characters (e.g. "config\n.yaml" or
"\x07config.yaml"), asserting they return the same rejection behavior
(returncode == 2 and "Invalid config path" in stderr) as the existing
traversal/absolute tests; reference the existing TestCliPathSafety._run helper,
the CLI entry point symbol _VALIDATOR, or the validate_safe_path() function to
locate where to add these cases.
- Around line 196-296: Add regression tests that explicitly pass boolean values
to integer fields to ensure the validator rejects them: create tests like
test_all_open_max_prs_rejects_bool,
test_completion_gate_max_retries_rejects_bool, and
test_per_pr_max_response_tokens_rejects_bool which set
invocation_limits["all_open_max_prs"] = True,
invocation_limits["completion_gate_max_retries"] = False, and
output_constraints["per_pr_max_response_tokens"] = True respectively, call
validate_config(config) and assert the same error substrings already used (e.g.,
"all_open_max_prs must be an integer >= 1", "completion_gate_max_retries must be
an integer >= 0", "per_pr_max_response_tokens must be an integer >= 1") are
present in the returned errors.
- Around line 67-72: Add missing test cases to
tests/test_validate_pr_review_config.py to assert that boolean values are
rejected for integer fields in invocation_limits and output_constraints: add
cases passing True/False for fields like all_open_max_prs,
completion_gate_max_retries, and per_pr_max_response_tokens to mirror the
validator's isinstance(..., bool) rejection logic (refer to the validator checks
that explicitly reject bool for max_tokens and the other integer fields). Also
extend the path-validation tests in this file to include rejection cases for
null bytes and control characters (use patterns similar to
tests/test_path_validation.py:test_path_with_null_byte_rejected and
tests/test_validation_pr_description.py:test_safe_label_for_output_replaces_all_control_chars)
so path validation covers both '\x00' and control-character inputs. Ensure new
tests assert the validator raises the same validation error as existing
path/char tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3bc26e0c-92cc-44dc-b6cb-7424b83236b5
📒 Files selected for processing (2)
scripts/validate_pr_review_config.pytests/test_validate_pr_review_config.py
✅ Files skipped from review due to trivial changes (1)
- scripts/validate_pr_review_config.py
Address two CodeRabbit follow-ups on PR #1671: - Add explicit `bool` regression tests for the integer fields (all_open_max_prs, completion_gate_max_retries, per_pr_max_response_tokens). Python's `bool` is a subclass of `int`, so the validator's explicit `bool` exclusion needs a test to prevent silent regression. - Add a `TestPathValidationHardening` class that exercises the new `validate_safe_path` integration against null-byte injection, control- character injection, traversal, and absolute-outside-root inputs. Null bytes cannot reach the CLI through subprocess argv (the OS rejects them), so those cases call `validate_safe_path` directly. Control chars resolve to a non-existent path; the CLI then exits 2 with a "not found" message, which is acceptable rejection behavior. Tests: 43 passed (8 new), 0 failed. Refs #1668 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_validate_pr_review_config.py (1)
361-377:⚠️ Potential issue | 🟠 MajorControl-character hardening test is too permissive and can mask a regression.
Line 374-377 allows
"Config file not found", so this test can pass even if control characters are not rejected byvalidate_safe_path. That weakens the security hardening signal.Suggested fix
- def test_control_chars_rejected_by_cli(self) -> None: - """Control chars resolve to a path that does not exist; CLI returns 2.""" - # newline-bearing input: validate_safe_path resolves it, but the - # resulting file does not exist, so the CLI rejects with exit 2. - result = subprocess.run( - [sys.executable, str(_VALIDATOR), "config\r.yaml"], - capture_output=True, - text=True, - cwd=str(_REPO_ROOT), - ) - assert result.returncode == 2 - # Either "Invalid config path" (from validate_safe_path) or - # "Config file not found" is acceptable; both signal rejection. - assert ( - "Invalid config path" in result.stderr - or "Config file not found" in result.stderr - ) + def test_control_char_newline_rejected(self) -> None: + from scripts.utils.path_validation import validate_safe_path + + with pytest.raises(ValueError): + validate_safe_path("config\n.yaml", _REPO_ROOT) + + def test_control_char_bell_rejected(self) -> None: + from scripts.utils.path_validation import validate_safe_path + + with pytest.raises(ValueError): + validate_safe_path("\x07config.yaml", _REPO_ROOT)As per coding guidelines, "Create unit test cases for path validation functions that cover path traversal attempts, null byte injection, control character injection, and absolute path rejection before deploying to production".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_validate_pr_review_config.py` around lines 361 - 377, The test test_control_chars_rejected_by_cli is too permissive because it allows either "Invalid config path" or "Config file not found", which can mask a failure in validate_safe_path; update the test to assert only the specific rejection from validate_safe_path by requiring "Invalid config path" in result.stderr (or, better, add an explicit unit assertion that validate_safe_path("config\r.yaml") raises/returns the invalid-path error) so the CLI test fails if control characters are not rejected—refer to validate_safe_path and the CLI invocation that runs [sys.executable, str(_VALIDATOR), "config\r.yaml"] when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_validate_pr_review_config.py`:
- Around line 361-377: The test test_control_chars_rejected_by_cli is too
permissive because it allows either "Invalid config path" or "Config file not
found", which can mask a failure in validate_safe_path; update the test to
assert only the specific rejection from validate_safe_path by requiring "Invalid
config path" in result.stderr (or, better, add an explicit unit assertion that
validate_safe_path("config\r.yaml") raises/returns the invalid-path error) so
the CLI test fails if control characters are not rejected—refer to
validate_safe_path and the CLI invocation that runs [sys.executable,
str(_VALIDATOR), "config\r.yaml"] when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 786f66e2-76a0-4551-aee5-9369fd01900b
📒 Files selected for processing (1)
tests/test_validate_pr_review_config.py
Summary
This branch was originally proposed to add
invocation_limitsandoutput_constraintsconfig keys plus Completion Criteria sections (closes #1668). PR #1786 shipped that work first, so the branch has been rebased to drop the duplicated config and doc edits and now lands two unique pieces:validate_safe_path).invocation_limitsandoutput_constraintssections, aligned to main's actual keys (per_pr_max_response_tokens,summary_format_allowed_values).Specification References
Changes
Type of Change
Testing
Notes
CodeRabbit feedback on PR #1671 has been applied:
null) before iterating.all_open_max_prs >= 1,completion_gate_max_retries >= 0,per_pr_max_response_tokens >= 1. Booleans are excluded explicitly because Python'sboolis a subclass ofint.strip().summary_formatmust be a member of the allowed-values list.Two pre-existing test failures on
main(workspace-budget tests against the top-level workspace policy files) surface here because this branch touches Python paths and the workspace-budget tests are gated on Python changes. The most recent main run skipped those tests for the same reason. Fixing the budget overage is out of scope and should be addressed in a dedicated cleanup PR.