fix(review-gate): require review_completed attestation on empty-findings payloads (1b76)#693
Conversation
…pty-findings payloads (1b76) validate-review-output.sh accepted a code-review-dispatch payload with empty findings + a >=10-char summary as SCHEMA_VALID — byte-equivalent to a truncated/ garbled payload — so a stripped reviewer response could pass as a clean no-issues review and merge un-reviewed code (588e panel O5-C2; fail-open gating the 3ee4 enforce-flip). Fix: add a top-level boolean `review_completed` attestation. - validate-review-output.sh: allowlist review_completed; require it === true when findings is an empty list AND the payload is non-synthetic (synthetic infra payloads keep their positive `type` channel; non-empty findings are themselves the positive signal). Schema hash cb48a66f -> 35dd40c9. - reviewer-base.md (shared base): reviewers now emit review_completed:true as a required top-level key; the 12 code-reviewer-*.md agents (+ ci variants) regenerated via build-review-agents.sh. - dso_ci_review/findings.py: merge_findings() sets review_completed:true (the runner's positive completion attestation for aggregated clean reviews); runner.py schema-hash constant + dispatch.py doc comment updated in lockstep. - contract doc + tests updated; new RED tests cover empty-without-attestation (reject), empty-with-attestation (pass), non-empty (unaffected), synthetic (exempt). Suite 100 -> 103; full dso_ci_review pytest 898 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 24 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR introduces a ChangesSchema enforcement and validation
Agent documentation updates
Test coverage
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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)
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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if isinstance(findings, list): | ||
| _synthetic_types = {"specialist_error", "fallback_exhausted", "parse_error"} | ||
| _is_synthetic_payload = any( | ||
| isinstance(f, dict) and f.get("type") in _synthetic_types for f in findings | ||
| ) | ||
| if not findings and not _is_synthetic_payload: | ||
| if data.get("review_completed") is not True: | ||
| errors.append( | ||
| "clean review (empty findings) must set review_completed: true — a positive " | ||
| "review-completed attestation is required to distinguish a completed no-issues " | ||
| "review from a truncated/garbled payload (1b76)" | ||
| ) |
There was a problem hiding this comment.
💡 Quality: Redundant synthetic-payload check in empty-findings enforcement
In validate-review-output.sh, the empty-findings enforcement computes _is_synthetic_payload = any(... for f in findings) and then gates on if not findings and not _is_synthetic_payload. When not findings is true the list is empty, so any(...) over it is always False and _is_synthetic_payload can never be True — the synthetic-payload term is dead code in this branch. A synthetic payload always carries at least one finding entry (with type), making findings non-empty, so it is already excluded by not findings. The extra computation is harmless but misleading: it suggests an empty synthetic payload would be exempted when in fact no such case exists. Consider dropping the _is_synthetic_payload term here (or adding a comment that it is intentionally defensive) to avoid implying behavior that the condition cannot exercise.
Was this helpful? React with 👍 / 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
plugins/dso/agents/ci/code-reviewer-deep-arch.md (1)
719-720:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove stale “exactly 2 top-level keys” statements to avoid schema-contract contradiction.
This file now requires
review_completed, but these lines still say the schema enforces exactly two top-level keys (findings,summary). That contradiction can produce inconsistent agent outputs and unnecessary re-dispatches.Also applies to: 762-762
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-deep-arch.md` around lines 719 - 720, Update the documentation text that claims the JSON schema requires "exactly 2 top-level keys (findings, summary)" by removing or replacing that stale assertion and any wording that contradicts the current schema; specifically remove the phrase "exactly 2 top-level keys" and update references about the deprecated "scores" key so the doc reflects the current required top-level keys including "review_completed" (and still "findings" and "summary") to avoid schema-contract contradictions and re-dispatch loops.plugins/dso/agents/ci/code-reviewer-deep-correctness.md (1)
753-753:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate stale validator note to include
review_completed.Line 753 still says validation enforces 2 required top-level keys, which conflicts with the updated schema section requiring
review_completed. Please align this line to prevent instruction ambiguity for the CI reviewer agent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-deep-correctness.md` at line 753, Update the stale sentence that claims validate-review-output.sh enforces only 2 required top-level keys to reflect the current schema by including the now-required review_completed key; specifically edit the line that mentions "2 required top-level keys (findings, summary)" to state the required keys are findings, summary, and review_completed (and note that scores is deprecated) so the guidance and CI validation expectations (validate-review-output.sh) are consistent with the schema; ensure the example summary text guidance (e.g., "security_overlay_warranted: no, performance_overlay_warranted: yes") remains unchanged.plugins/dso/agents/ci/code-reviewer-deep-hygiene.md (1)
647-648:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix leftover “2 required top-level keys” guidance in this CI agent prompt.
These lines contradict the new schema requirement that includes
review_completed. Keeping both rules in one prompt can cause malformed reviewer output contracts.Also applies to: 704-704
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-deep-hygiene.md` around lines 647 - 648, Update the prompt text that still instructs "2 required top-level keys (findings, summary)" and references the deprecated "scores" key: remove or replace that sentence so the guidance matches the current schema which requires the top-level keys findings, summary, and review_completed; also remove references to the deprecated "scores" key and ensure the finding categories enum description still lists the five allowed values (and nothing contradictory). Apply this change to both occurrences of the outdated guidance (the block containing the literal text "2 required top-level keys" and any nearby mention of "scores" / finding category enum) so the CI agent prompt and reviewer output contract are consistent with the new schema.plugins/dso/agents/ci/code-reviewer-deep-verification.md (1)
648-648:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign this validator sentence with the new
review_completedrequirement.Line 648 still documents a 2-key requirement, which conflicts with the updated schema contract in this same file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-deep-verification.md` at line 648, Update the validator sentence that currently asserts a 2-key requirement (findings, summary) to reflect the new schema by adding the required review_completed key: state that the output MUST include findings, summary and review_completed (summary field text should still contain items like "security_overlay_warranted: no, performance_overlay_warranted: yes"), note that scores is deprecated, and keep the reference to validate-review-output.sh enforcing the contract; edit the sentence mentioning "summary field" on line 648 to list these three required top-level keys (findings, summary, review_completed) instead of two.plugins/dso/agents/ci/code-reviewer-performance.md (1)
473-500:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate CI JSON example/checklist to include
review_completed.The Step 3 payload example and Step 4 required-fields checklist still model a two-key contract, conflicting with the updated schema rules above. This inconsistency will cause avoidable re-dispatch loops when the example is followed literally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-performance.md` around lines 473 - 500, Update the example JSON and the Step 4 checklist in plugins/dso/agents/ci/code-reviewer-performance.md to include the new required top-level boolean field review_completed alongside summary and findings; specifically add "review_completed": true/false to the example object and update the textual checklist to require review_completed, and ensure validate-review-output.sh (the dispatcher validator) is aware/consistent with the new field name so the example and checklist no longer contradict the schema.plugins/dso/agents/ci/code-reviewer-security-red-team.md (1)
563-566:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRed-team output-schema section still documents the legacy two-key contract.
The delta says only
findingsandsummaryare required, but the file’s base contract now requiresreview_completed: true. This mismatch is a functional contract defect in reviewer instructions and should be corrected in the delta block too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-security-red-team.md` around lines 563 - 566, The Output Schema delta currently states only findings and summary are required but the base contract also requires review_completed: true; update the "Output Schema" section so the required top-level keys include review_completed (e.g., "Your output MUST conform... (3 required top-level keys: findings, summary, review_completed)"), and mention that each output must set review_completed: true alongside findings and summary to align the delta with the base contract and prevent a contract mismatch.plugins/dso/agents/ci/code-reviewer-security-blue-team.md (1)
569-572:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlue-team delta schema is now out of sync with base contract.
This section still defines the legacy two-key top-level schema, which conflicts with the required
review_completed: truecontract defined earlier in the same file. Please align the delta Output Schema text with the new validator contract to prevent invalid reviewer payloads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-security-blue-team.md` around lines 569 - 572, Update the "Output Schema" delta to remove the outdated "two-key top-level schema" wording and explicitly match the validator contract that requires review_completed: true; specifically, change the text under "Output Schema" to state the required top-level keys are findings, summary and review_completed (and that the legacy scores key is deprecated and must not be emitted), and ensure the per-finding allowlist note remains unchanged so that each finding uses only the five permitted fields; reference the "Output Schema" section and the earlier "review_completed: true" contract when editing the wording.plugins/dso/agents/ci/code-reviewer-light.md (1)
630-656:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve stale “two-key schema” instructions in the final output section.
This section still tells the agent to emit only
findingsandsummary, which now contradicts the requiredreview_completed: truecontract earlier in the same file. In CI mode this creates deterministic validator failures on empty-findings outputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-light.md` around lines 630 - 656, The "Final Output Reminder" text incorrectly insists only on top-level keys "findings" and "summary", which conflicts with the earlier required "review_completed: true" contract and causes CI validator failures; update the final-output guidance to require the exact top-level keys expected by CI (include "review_completed": true alongside "findings" and "summary"), remove or reword the outdated "two-key schema" directive, and add a short note that emitting any other top-level keys (except an allowed deprecated "scores") will fail validate-review-output.sh so authors must include "review_completed: true" when appropriate.plugins/dso/agents/ci/code-reviewer-standard.md (1)
469-500:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix CI JSON example/checklist to include
review_completedStep 3’s canonical JSON and Step 4 required-fields checklist still describe a 2-key payload (
summary,findings) and omitreview_completed, which now contradicts the schema rules above and the validator behavior. This can drive invalid agent output and re-dispatch loops.Suggested doc fix
{ "summary": "<one-paragraph overall assessment>", + "review_completed": true, "findings": [ @@ -2. **Did you include all required schema fields?** `summary` is required; each finding requires `severity`, `category`, `file`, `description`. Field names match Step 3's schema exactly — do NOT substitute `dim` for `category`, `title` for `description`, or `line` for `cited_lines`. +2. **Did you include all required schema fields?** `summary` and `review_completed` are required; each finding requires `severity`, `category`, `file`, `description`. Field names match Step 3's schema exactly — do NOT substitute `dim` for `category`, `title` for `description`, or `line` for `cited_lines`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-standard.md` around lines 469 - 500, The CI JSON example and final-checklist in plugins/dso/agents/ci/code-reviewer-standard.md still show only "summary" and "findings" and omit the required "review_completed" field; update the canonical JSON example in Step 3 and the Step 4 required-fields checklist to include "review_completed" (boolean) and show it in the single-object example, adjust any prose that lists required fields (e.g., the Step 4 checks and the "Field-name discipline" paragraph) to mention "review_completed", and add one brief example/value for "review_completed" in the example JSON so the validator examples match actual validator expectations.plugins/dso/agents/code-reviewer-deep-arch.md (1)
755-756:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove stale “exactly 2 top-level keys” language
These lines still claim only
findings/summaryare required, which now conflicts with the updatedreview_completedrequirement in the same file and validator contract. Please align both references to the 3-key requirement.Also applies to: 798-799
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/code-reviewer-deep-arch.md` around lines 755 - 756, Replace the stale phrase "exactly 2 top-level keys (findings, summary)" with the correct 3-key requirement by stating something like "exactly 3 top-level keys (findings, summary, review_completed)"; update the same wording elsewhere in the file where "exactly 2 top-level keys" appears (the later occurrence around the other paragraph) so both references match the validator/contract and explicitly list the three keys.plugins/dso/agents/ci/code-reviewer-test-quality.md (1)
654-657:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate test-quality Output Schema to 3 required top-level keys
This section still instructs a 2-key payload, conflicting with the updated required
review_completedcontract. Keeping this stale text will cause schema-invalid outputs from this agent.Suggested doc fix
-Your output MUST conform to the standard reviewer-findings.json schema (2 required top-level keys: findings, summary). +Your output MUST conform to the standard reviewer-findings.json schema (3 required top-level keys: findings, summary, review_completed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dso/agents/ci/code-reviewer-test-quality.md` around lines 654 - 657, The "Output Schema" section still says the reviewer-findings.json requires 2 top-level keys; update the prose under the "## Output Schema" heading to reflect the new contract requiring 3 required top-level keys (findings, summary, review_completed), and explicitly note that review_completed is required and must follow the updated contract so generated outputs remain schema-valid; ensure any examples or wording that assert "2 required top-level keys" are changed to say "3 required top-level keys" and add a brief sentence about the expected type/format of review_completed.plugins/dso/agents/code-reviewer-light.md (1)
714-721:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the light-tier final-output reminder to include
review_completed.Line 714 onward still instructs agents to emit only
findingsandsummary, which conflicts with the updated required schema in Line 166 (review_completedmandatory). This will produce invalid payloads for clean reviews and fail validation.Proposed fix
-Your JSON output MUST use these exact top-level key names: +Your JSON output MUST use these exact top-level key names: ```json -{ "findings": [...], "summary": "..." } +{ "findings": [...], "summary": "...", "review_completed": true }-Required top-level keys are
findingsandsummary. The legacyscoreskey is deprecated — do not emit it. The validator toleratesscoreswith a deprecation warning but rejects any other top-level key name (e.g., "dimensions", "ratings").
+Required top-level keys arefindings,summary, andreview_completed(alwaystrue). The legacyscoreskey is deprecated — do not emit it. The validator toleratesscoreswith a deprecation warning but rejects any other top-level key name (e.g., "dimensions", "ratings").</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/dso/agents/code-reviewer-light.mdaround lines 714 - 721, Update the
light-tier final-output reminder so the required top-level JSON keys include
"review_completed" (always true) in addition to "findings" and "summary"; change
the example output to { "findings": [...], "summary": "...", "review_completed":
true } and update the descriptive sentence to say "Required top-level keys are
findings,summary, andreview_completed(alwaystrue)." Keep the note
about the deprecatedscoreskey tolerated with a warning and ensure no other
top-level keys (e.g., "dimensions", "ratings") are permitted.</details> <!-- cr-comment:v1:955f3f29d5d972b64b7f89d1 --> </blockquote></details> <details> <summary>plugins/dso/agents/code-reviewer-security-blue-team.md (1)</summary><blockquote> `607-618`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Tier-local schema block is stale and contradicts the enforced contract.** This section still says “2 required top-level keys” and a reduced finding allowlist, which conflicts with the updated base contract in this same file (and validator behavior) that requires `review_completed` semantics. Please align this block to the current schema so blue-team agents don’t emit conflicting payload shapes. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/dso/agents/code-reviewer-security-blue-team.mdaround lines 607 -
618, Update the stale tier-local schema block so it matches the current enforced
contract: replace the old statement about "2 required top-level keys
(findings,summary)" and the reduced allowlist with the active schema that
includesreview_completedsemantics and the full required payload shape;
ensure the allowlisted finding fields and requiredcited_linesrule remain
consistent with the base validator and reference the same keys used elsewhere in
this file (e.g.,findings,summary,review_completed,cited_lines) so
blue-team agents emit the correct JSON shape.</details> <!-- cr-comment:v1:826b84c555f5f203b7389879 --> </blockquote></details> <details> <summary>plugins/dso/agents/code-reviewer-security-red-team.md (1)</summary><blockquote> `601-602`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Red-team output schema section still documents the old 2-key contract.** This local section conflicts with the updated base schema in the same file (which now includes `review_completed` requirements). Please update this block to the current contract to prevent instruction ambiguity and invalid reviewer payloads. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/dso/agents/code-reviewer-security-red-team.mdaround lines 601 -
602, The documentation block that enforces the "2 required top-level keys:
findings, summary" in plugins/dso/agents/code-reviewer-security-red-team.md must
be updated to match the current reviewer-findings.json contract — modify the
text that describes the required top-level keys so it includes the additional
required key review_completed and any updated allowed fields/constraints (retain
the allowlist restriction for finding fields: severity, category, description,
file, cited_lines and the prefix requirement for descriptions), remove or revise
the outdated sentence about the "2-key contract", and ensure the block
references the exact schema keys findings, summary, and review_completed to
avoid producing invalid reviewer payloads.</details> <!-- cr-comment:v1:a9a8c1d461239cfb73e05335 --> </blockquote></details> <details> <summary>plugins/dso/agents/code-reviewer-standard.md (1)</summary><blockquote> `924-925`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Schema references in later sections still mention a 2-key top-level contract.** These lines now contradict the updated schema block above that requires `review_completed`. Please update these references to avoid conflicting guidance inside the same agent definition. Also applies to: 973-974 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/dso/agents/code-reviewer-standard.mdaround lines 924 - 925, Update
the text that currently reads "top-level scoring dimension — the JSON schema
enforces 2 required top-level keys (findings,summary); the legacyscores
key is deprecated." (and the similar reference later) so it reflects the new
contract: list the required top-level keysfindings,summary, and
review_completed, note thatscoresis deprecated, and remove any assertation
that only two keys are required; keep the same wording style and adjust both
occurrences mentioned in the comment to avoid the conflicting guidance.</details> <!-- cr-comment:v1:dfe713d2c0ebfb97aa2f67b2 --> </blockquote></details> <details> <summary>tests/hooks/test-validate-review-output.sh (2)</summary><blockquote> `1479-1504`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Avoid masking `escalate_review` validation errors behind missing attestation.** Both malformed `escalate_review` fixtures use empty findings without `review_completed`. That can short-circuit to attestation errors and make these assertions brittle/non-specific. <details> <summary>Suggested fix</summary> ```diff _ESC_STRING_FILE=$(write_fixture "escalate_review_string.json" '{ "findings": [], "summary": "All dimensions clean but escalate_review is malformed as a string.", + "review_completed": true, "escalate_review": "uncertain severity" }') _ESC_MISSING_IDX_FILE=$(write_fixture "escalate_review_missing_index.json" '{ "findings": [], "summary": "All dimensions clean but escalate_review element is missing finding_index.", + "review_completed": true, "escalate_review": [{"reason": "uncertain severity"}] }')As per coding guidelines, “Tests must include edge-case and error-path coverage.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hooks/test-validate-review-output.sh` around lines 1479 - 1504, The tests are masking escalate_review validation errors because both fixtures use an empty "findings" array so attestation/`review_completed` checks short-circuit; update the fixtures created by write_fixture for _ESC_STRING_FILE and _ESC_MISSING_IDX_FILE to include at least one minimal finding object (e.g., include a finding with a finding_index like 0 and an explicit review_completed value) so the code path reaches escalate_review validation (affecting the asserts that read _ESC_STRING_OUTPUT and the _snapshot_fail check) and the specific type/missing-field errors surface.Source: Coding guidelines
1362-1368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIsolate
review_tierenum failure from attestation failure.This negative test can pass for the wrong reason because
findingsis empty andreview_completedis omitted. If enum validation regresses, the test may still stay green due to the attestation gate.Suggested fix
_REVIEW_TIER_INVALID_FILE=$(write_fixture "review_tier_invalid.json" '{ "findings": [], "summary": "No issues found. Code is well-structured and tests are adequate.", - "review_tier": "ultra_deep" + "review_tier": "ultra_deep", + "review_completed": true }')As per coding guidelines, “Tests must exercise observable behavior of new code paths” and “Tests must include edge-case and error-path coverage.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hooks/test-validate-review-output.sh` around lines 1362 - 1368, The test fixture review_tier_invalid.json currently has empty "findings" and omits "review_completed", so the negative test (run_script code-review-dispatch -> _REVIEW_TIER_INVALID_EXIT) can fail due to attestation gate instead of the invalid review_tier enum; update the fixture used by test_review_tier_invalid_value_fails to include a non-empty "findings" array and set "review_completed": true (or otherwise ensure attestation passes) so the script reaches enum validation and the invalid "review_tier": "ultra_deep" is the observable failure being asserted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/docs/test-reviewer-base-file-constraint.sh`:
- Around line 311-323: The current check uses keys_match and attest_match
separately and can misfire; change the logic to assert that the required-key
clause explicitly names findings, summary and review_completed together (e.g., a
single regex/search that matches
"required.*(findings).*?(summary).*?(review_completed)" or a pattern that finds
those three tokens in the same clause/line), replace the two-step keys_match +
attest_match approach with a single combined assertion, and update the condition
that sets "three_key_found" to only be true when that combined pattern matches
(references: keys_match, attest_match, and the schema_section handling that
prints/inspects "three_key_found").
---
Outside diff comments:
In `@plugins/dso/agents/ci/code-reviewer-deep-arch.md`:
- Around line 719-720: Update the documentation text that claims the JSON schema
requires "exactly 2 top-level keys (findings, summary)" by removing or replacing
that stale assertion and any wording that contradicts the current schema;
specifically remove the phrase "exactly 2 top-level keys" and update references
about the deprecated "scores" key so the doc reflects the current required
top-level keys including "review_completed" (and still "findings" and "summary")
to avoid schema-contract contradictions and re-dispatch loops.
In `@plugins/dso/agents/ci/code-reviewer-deep-correctness.md`:
- Line 753: Update the stale sentence that claims validate-review-output.sh
enforces only 2 required top-level keys to reflect the current schema by
including the now-required review_completed key; specifically edit the line that
mentions "2 required top-level keys (findings, summary)" to state the required
keys are findings, summary, and review_completed (and note that scores is
deprecated) so the guidance and CI validation expectations
(validate-review-output.sh) are consistent with the schema; ensure the example
summary text guidance (e.g., "security_overlay_warranted: no,
performance_overlay_warranted: yes") remains unchanged.
In `@plugins/dso/agents/ci/code-reviewer-deep-hygiene.md`:
- Around line 647-648: Update the prompt text that still instructs "2 required
top-level keys (findings, summary)" and references the deprecated "scores" key:
remove or replace that sentence so the guidance matches the current schema which
requires the top-level keys findings, summary, and review_completed; also remove
references to the deprecated "scores" key and ensure the finding categories enum
description still lists the five allowed values (and nothing contradictory).
Apply this change to both occurrences of the outdated guidance (the block
containing the literal text "2 required top-level keys" and any nearby mention
of "scores" / finding category enum) so the CI agent prompt and reviewer output
contract are consistent with the new schema.
In `@plugins/dso/agents/ci/code-reviewer-deep-verification.md`:
- Line 648: Update the validator sentence that currently asserts a 2-key
requirement (findings, summary) to reflect the new schema by adding the required
review_completed key: state that the output MUST include findings, summary and
review_completed (summary field text should still contain items like
"security_overlay_warranted: no, performance_overlay_warranted: yes"), note that
scores is deprecated, and keep the reference to validate-review-output.sh
enforcing the contract; edit the sentence mentioning "summary field" on line 648
to list these three required top-level keys (findings, summary,
review_completed) instead of two.
In `@plugins/dso/agents/ci/code-reviewer-light.md`:
- Around line 630-656: The "Final Output Reminder" text incorrectly insists only
on top-level keys "findings" and "summary", which conflicts with the earlier
required "review_completed: true" contract and causes CI validator failures;
update the final-output guidance to require the exact top-level keys expected by
CI (include "review_completed": true alongside "findings" and "summary"), remove
or reword the outdated "two-key schema" directive, and add a short note that
emitting any other top-level keys (except an allowed deprecated "scores") will
fail validate-review-output.sh so authors must include "review_completed: true"
when appropriate.
In `@plugins/dso/agents/ci/code-reviewer-performance.md`:
- Around line 473-500: Update the example JSON and the Step 4 checklist in
plugins/dso/agents/ci/code-reviewer-performance.md to include the new required
top-level boolean field review_completed alongside summary and findings;
specifically add "review_completed": true/false to the example object and update
the textual checklist to require review_completed, and ensure
validate-review-output.sh (the dispatcher validator) is aware/consistent with
the new field name so the example and checklist no longer contradict the schema.
In `@plugins/dso/agents/ci/code-reviewer-security-blue-team.md`:
- Around line 569-572: Update the "Output Schema" delta to remove the outdated
"two-key top-level schema" wording and explicitly match the validator contract
that requires review_completed: true; specifically, change the text under
"Output Schema" to state the required top-level keys are findings, summary and
review_completed (and that the legacy scores key is deprecated and must not be
emitted), and ensure the per-finding allowlist note remains unchanged so that
each finding uses only the five permitted fields; reference the "Output Schema"
section and the earlier "review_completed: true" contract when editing the
wording.
In `@plugins/dso/agents/ci/code-reviewer-security-red-team.md`:
- Around line 563-566: The Output Schema delta currently states only findings
and summary are required but the base contract also requires review_completed:
true; update the "Output Schema" section so the required top-level keys include
review_completed (e.g., "Your output MUST conform... (3 required top-level keys:
findings, summary, review_completed)"), and mention that each output must set
review_completed: true alongside findings and summary to align the delta with
the base contract and prevent a contract mismatch.
In `@plugins/dso/agents/ci/code-reviewer-standard.md`:
- Around line 469-500: The CI JSON example and final-checklist in
plugins/dso/agents/ci/code-reviewer-standard.md still show only "summary" and
"findings" and omit the required "review_completed" field; update the canonical
JSON example in Step 3 and the Step 4 required-fields checklist to include
"review_completed" (boolean) and show it in the single-object example, adjust
any prose that lists required fields (e.g., the Step 4 checks and the
"Field-name discipline" paragraph) to mention "review_completed", and add one
brief example/value for "review_completed" in the example JSON so the validator
examples match actual validator expectations.
In `@plugins/dso/agents/ci/code-reviewer-test-quality.md`:
- Around line 654-657: The "Output Schema" section still says the
reviewer-findings.json requires 2 top-level keys; update the prose under the "##
Output Schema" heading to reflect the new contract requiring 3 required
top-level keys (findings, summary, review_completed), and explicitly note that
review_completed is required and must follow the updated contract so generated
outputs remain schema-valid; ensure any examples or wording that assert "2
required top-level keys" are changed to say "3 required top-level keys" and add
a brief sentence about the expected type/format of review_completed.
In `@plugins/dso/agents/code-reviewer-deep-arch.md`:
- Around line 755-756: Replace the stale phrase "exactly 2 top-level keys
(findings, summary)" with the correct 3-key requirement by stating something
like "exactly 3 top-level keys (findings, summary, review_completed)"; update
the same wording elsewhere in the file where "exactly 2 top-level keys" appears
(the later occurrence around the other paragraph) so both references match the
validator/contract and explicitly list the three keys.
In `@plugins/dso/agents/code-reviewer-light.md`:
- Around line 714-721: Update the light-tier final-output reminder so the
required top-level JSON keys include "review_completed" (always true) in
addition to "findings" and "summary"; change the example output to { "findings":
[...], "summary": "...", "review_completed": true } and update the descriptive
sentence to say "Required top-level keys are `findings`, `summary`, and
`review_completed` (always `true`)." Keep the note about the deprecated `scores`
key tolerated with a warning and ensure no other top-level keys (e.g.,
"dimensions", "ratings") are permitted.
In `@plugins/dso/agents/code-reviewer-security-blue-team.md`:
- Around line 607-618: Update the stale tier-local schema block so it matches
the current enforced contract: replace the old statement about "2 required
top-level keys (`findings`, `summary`)" and the reduced allowlist with the
active schema that includes `review_completed` semantics and the full required
payload shape; ensure the allowlisted finding fields and required `cited_lines`
rule remain consistent with the base validator and reference the same keys used
elsewhere in this file (e.g., `findings`, `summary`, `review_completed`,
`cited_lines`) so blue-team agents emit the correct JSON shape.
In `@plugins/dso/agents/code-reviewer-security-red-team.md`:
- Around line 601-602: The documentation block that enforces the "2 required
top-level keys: findings, summary" in
plugins/dso/agents/code-reviewer-security-red-team.md must be updated to match
the current reviewer-findings.json contract — modify the text that describes the
required top-level keys so it includes the additional required key
review_completed and any updated allowed fields/constraints (retain the
allowlist restriction for finding fields: severity, category, description, file,
cited_lines and the prefix requirement for descriptions), remove or revise the
outdated sentence about the "2-key contract", and ensure the block references
the exact schema keys findings, summary, and review_completed to avoid producing
invalid reviewer payloads.
In `@plugins/dso/agents/code-reviewer-standard.md`:
- Around line 924-925: Update the text that currently reads "top-level scoring
dimension — the JSON schema enforces 2 required top-level keys (`findings`,
`summary`); the legacy `scores` key is deprecated." (and the similar reference
later) so it reflects the new contract: list the required top-level keys
`findings`, `summary`, and `review_completed`, note that `scores` is deprecated,
and remove any assertation that only two keys are required; keep the same
wording style and adjust both occurrences mentioned in the comment to avoid the
conflicting guidance.
In `@tests/hooks/test-validate-review-output.sh`:
- Around line 1479-1504: The tests are masking escalate_review validation errors
because both fixtures use an empty "findings" array so
attestation/`review_completed` checks short-circuit; update the fixtures created
by write_fixture for _ESC_STRING_FILE and _ESC_MISSING_IDX_FILE to include at
least one minimal finding object (e.g., include a finding with a finding_index
like 0 and an explicit review_completed value) so the code path reaches
escalate_review validation (affecting the asserts that read _ESC_STRING_OUTPUT
and the _snapshot_fail check) and the specific type/missing-field errors
surface.
- Around line 1362-1368: The test fixture review_tier_invalid.json currently has
empty "findings" and omits "review_completed", so the negative test (run_script
code-review-dispatch -> _REVIEW_TIER_INVALID_EXIT) can fail due to attestation
gate instead of the invalid review_tier enum; update the fixture used by
test_review_tier_invalid_value_fails to include a non-empty "findings" array and
set "review_completed": true (or otherwise ensure attestation passes) so the
script reaches enum validation and the invalid "review_tier": "ultra_deep" is
the observable failure being asserted.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 514070bb-c19d-4edc-88af-b27c4ad58b38
📒 Files selected for processing (29)
plugins/dso/agents/ci/code-reviewer-deep-arch.mdplugins/dso/agents/ci/code-reviewer-deep-correctness.mdplugins/dso/agents/ci/code-reviewer-deep-hygiene.mdplugins/dso/agents/ci/code-reviewer-deep-verification.mdplugins/dso/agents/ci/code-reviewer-light.mdplugins/dso/agents/ci/code-reviewer-performance.mdplugins/dso/agents/ci/code-reviewer-security-blue-team.mdplugins/dso/agents/ci/code-reviewer-security-red-team.mdplugins/dso/agents/ci/code-reviewer-standard.mdplugins/dso/agents/ci/code-reviewer-test-quality.mdplugins/dso/agents/code-reviewer-deep-arch.mdplugins/dso/agents/code-reviewer-deep-correctness.mdplugins/dso/agents/code-reviewer-deep-hygiene.mdplugins/dso/agents/code-reviewer-deep-verification.mdplugins/dso/agents/code-reviewer-light.mdplugins/dso/agents/code-reviewer-performance.mdplugins/dso/agents/code-reviewer-security-blue-team.mdplugins/dso/agents/code-reviewer-security-red-team.mdplugins/dso/agents/code-reviewer-standard.mdplugins/dso/agents/code-reviewer-test-quality.mdplugins/dso/docs/contracts/review-findings-schema.mdplugins/dso/docs/workflows/prompts/reviewer-base.mdplugins/dso/scripts/dso_ci_review/dispatch.pyplugins/dso/scripts/dso_ci_review/findings.pyplugins/dso/scripts/dso_ci_review/runner.pyplugins/dso/scripts/validate-review-output.shtests/docs/test-reviewer-base-file-constraint.shtests/hooks/test-validate-review-output.shtests/scripts/test-write-reviewer-findings.sh
| keys_match = re.search(r'three required top-level keys|EXACTLY three top-level keys|required.*three.*keys', content, re.IGNORECASE) | ||
| attest_match = re.search(r'review_completed', content) | ||
| if keys_match and attest_match: | ||
| print("three_key_found") | ||
| PYEOF | ||
| )" | ||
| if [[ "$schema_section" == *"two_key_found"* ]]; then | ||
| assert_eq "JSON schema specifies 2-key schema (EXACTLY two top-level keys: findings + summary)" "present" "present" | ||
| if [[ "$schema_section" == *"three_key_found"* ]]; then | ||
| assert_eq "JSON schema specifies 3-key schema (findings + summary + review_completed)" "present" "present" | ||
| else | ||
| assert_eq "JSON schema specifies 2-key schema (findings + summary, no scores)" \ | ||
| "2-key schema specification (findings + summary)" \ | ||
| "2-key schema language not found — still using 3-key schema?" | ||
| assert_eq "JSON schema specifies 3-key schema (findings + summary + review_completed)" \ | ||
| "3-key schema specification (findings + summary + review_completed)" \ | ||
| "3-key schema language not found — review_completed attestation missing (bug 1b76)?" | ||
| fi |
There was a problem hiding this comment.
Tighten the 3-key contract assertion to avoid false positives.
Current logic accepts any file containing “three required ... keys” plus any separate mention of review_completed. It does not prove the required-key clause actually names findings, summary, and review_completed together.
Suggested fix
-keys_match = re.search(r'three required top-level keys|EXACTLY three top-level keys|required.*three.*keys', content, re.IGNORECASE)
-attest_match = re.search(r'review_completed', content)
-if keys_match and attest_match:
+lines = content.splitlines()
+keys_line = next(
+ (ln for ln in lines if re.search(r'(three|3).*required.*top-level keys', ln, re.IGNORECASE)),
+ ""
+)
+if all(k in keys_line for k in ('findings', 'summary', 'review_completed')):
print("three_key_found")As per coding guidelines, “Tests must exercise observable behavior of new code paths.” Based on learnings, the upstream contract in plugins/dso/docs/workflows/prompts/reviewer-base.md defines these exact three required top-level keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/docs/test-reviewer-base-file-constraint.sh` around lines 311 - 323, The
current check uses keys_match and attest_match separately and can misfire;
change the logic to assert that the required-key clause explicitly names
findings, summary and review_completed together (e.g., a single regex/search
that matches "required.*(findings).*?(summary).*?(review_completed)" or a
pattern that finds those three tokens in the same clause/line), replace the
two-step keys_match + attest_match approach with a single combined assertion,
and update the condition that sets "three_key_found" to only be true when that
combined pattern matches (references: keys_match, attest_match, and the
schema_section handling that prints/inspects "three_key_found").
Source: Coding guidelines
… arch_result review_completed injection Neutral opus pre-merge review of #693 surfaced an important latent fail-CLOSED introduced by the 1b76 empty-findings rule: the deep-tier path (runner.py `merged = arch_result`) uses the raw arch-agent output, which carries review_completed only if the LLM emitted it. A clean deep review omitting the key failed the new empty-findings rule and routed into dispatch_schema_correction, which is structurally unable to add the key (reviewer-schema-correction.md forbids adding keys) — re-failing into a synthetic schema_error that blocked an otherwise clean PR. Fix: inject review_completed=True onto a non-synthetic arch_result with empty findings before validation (copy-not-mutate), mirroring the merge_findings guard. Synthetic arch results are unaffected (short-circuited to the merge_findings dict, which already carries the key); non-empty results are their own positive signal. Adds tests/skills/dso_ci_review/test_empty_findings_review_completed_integration.py (4 tests): merge_findings→real-validator end-to-end, the empty-without-attestation control (rule still enforced), the deep-tier arch-replacement RED/GREEN, and the synthetic-edge guard. dso_ci_review suite 902 passed; validator suite 103 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view attestation (1b76) Re-review found the prior per-path injection incomplete: the runner produces the validated `merged` dict from several branches (single-pass merge_findings, deep arch-replacement, Strategy-F cluster-aggregation, rechunk). Only two were patched; the cluster-aggregation and rechunk clean-review dicts still reached the validator without review_completed (and, for cluster, with non-allowed keys + no summary) — re-introducing the 1b76 fail-CLOSED via those paths. Replace per-path injection with a single authoritative normalization (_normalize_empty_clean_review) applied immediately before the sole main-pipeline _validate_findings_schema(merged) call. For a NON-synthetic empty-findings merged it stamps review_completed:True, synthesizes a summary, and drops non-allowed top-level keys — normalizing the empty clean-review dict to the validator contract so no clean-review branch (current or future) fails closed. The redundant arch per-path injection is removed; merge_findings()'s own injection is kept (reusable public contract). The other four _validate_findings_schema calls live inside dispatch_schema_correction (downstream of the choke point) and are unaffected. Non-empty / synthetic flows are untouched. Adds cluster-aggregation + rechunk clean-review integration tests (RED before the choke point, GREEN after). tests/skills/dso_ci_review/ 904 passed; validator 103. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| # When validate-review-output.sh changes its schema, update this constant and | ||
| # the corresponding test in test_runner_smoke.py. | ||
| _VALIDATE_REVIEW_SCHEMA_HASH = "cb48a66fc3292083" | ||
| _VALIDATE_REVIEW_SCHEMA_HASH = "35dd40c9e6e83207" |
…t (1b76 re-review minor) Cycle-3 neutral review minor: runner.py Step 7b comment referenced the pre-1b76 schema hash 214949ee476be6d0 (from PR #124, never updated). Documentation-only; the real lockstep is _VALIDATE_REVIEW_SCHEMA_HASH + the drift test (both already 35dd40c9e6e83207). Comment updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI failed: The CI build failed because the PR is targeting the 'main' branch directly from a 'fix/' branch, violating the required 'staged-*' naming convention enforced by the project's two-tier promotion workflow.OverviewThe FailuresBranch Naming Constraint Violation (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
2 participants
1b76 — require positive
review_completedattestation on empty-findings payloadsFixes bug 1b76-0a0f-9779-4eda (588e panel O5-C2). Closes a fail-open where
validate-review-output.shaccepted{findings:[], summary>=10chars}as a clean review — byte-equivalent to a truncated/garbled payload — letting un-reviewed code merge. This fail-open gates the 3ee4 enforce-flip.Fix: top-level boolean
review_completedattestation; validator requires it=== truewhenfindingsis empty AND the payload is non-synthetic (synthetic infra payloads keep theirtypechannel; non-empty findings are themselves the positive signal). Emitted via the sharedreviewer-base.md(12 agents + ci variants regenerated viabuild-review-agents.sh);merge_findings()sets it for the runner's aggregated clean reviews. Contract doc + tests updated (suite 100→103; fulldso_ci_reviewpytest 898 passed).Why direct PR + manual bypass-merge
Per epic 588e handoff: the two-tier staged flow livelocks (review-sub-pr/llm-review FP-amplifies). Landing recipe = direct PR to
main+ a neutral local opus review before merge (the real gate — especially apt here since this change touches the review machinery itself) + human web-UI bypass-merge with all other required checks green. Approved by maintainer (Fix A via shared base). The neutral opus review is run pre-merge and attached as the clearance artifact.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests