feat: implement issue #1092 — [Phase 3] Agentic iterative validation: confirm suspected bugs by running lint/tests before reporting - #1229
Conversation
… confirm suspected bugs by running lint/tests before reporting
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughDeep-review prompts now require verification outcomes for logic findings. A shell post-processor adjusts refuted findings, records verification events, and runs before synthesis. Unit and end-to-end tests cover transformations, logging, safe no-ops, and timeout enforcement. ChangesDeep finding verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeepReview
participant VerificationProcessor
participant TokenMetrics
participant ReviewSynthesis
DeepReview->>VerificationProcessor: write deep findings
VerificationProcessor->>VerificationProcessor: apply verification outcomes
VerificationProcessor->>TokenMetrics: emit finding_verification records
VerificationProcessor->>ReviewSynthesis: pass adjusted findings
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-07-14T11:28:25Z. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![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 implements agentic iterative validation (issue #1092) to confirm or refute suspected logic and correctness findings by running relevant lint or test tools within a bounded sandbox. It introduces the finding-verification.sh script to process verification results, downgrading or dropping refuted findings and logging verification records via token-metrics.sh. The deep review prompts are updated, the verification step is integrated into review-one-pr.sh, and comprehensive unit and E2E tests are added. Feedback suggests optimizing the loop in finding-verification.sh to avoid spawning multiple jq and printf processes per finding, and using $BATS_TEST_TMPDIR for temporary files in BATS tests to ensure proper cleanup.
There was a problem hiding this comment.
Pull request overview
Implements issue #1092’s “agentic iterative validation” loop by having the deep-review tier tag logic/correctness findings with a verification outcome, then post-processing those findings to downgrade/drop refuted items and emit kind:"finding_verification" telemetry records for FP-rate measurement.
Changes:
- Added
scripts/lib/finding-verification.shto apply the downgrade/drop discipline and emitfinding_verificationrecords viaemit_verification_record. - Wired the post-processor into
scripts/review-one-pr.shimmediately after the deep tier produces JSON output. - Updated deep-review prompts to require a
verificationfield for logic/correctness findings; added unit + e2e coverage for the post-processor and timeout bound.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/dev-lead/unit/test_finding_verification.bats | Adds unit tests for apply_finding_verification and emit_verification_record. |
| tests/dev-lead/e2e/scenarios/08-finding-verification-timeout.sh | Adds an e2e scenario asserting the deep-tier run_agentic timeout bounds a hung engine. |
| scripts/review-one-pr.sh | Sources and invokes apply_finding_verification after deep-tier output is produced. |
| scripts/lib/token-metrics.sh | Introduces emit_verification_record JSONL emission for FP-rate telemetry. |
| scripts/lib/finding-verification.sh | New post-processor implementing downgrade/drop + audit tagging + telemetry emission. |
| prompts/deep-review.md | Instructs deep tier to run a quick repro for logic/correctness and set verification. |
| prompts/deep-review-logic.md | Same as above for the logic-focused deep review variant. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1229 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: d048396b14d68284b36df9c303a583078b698a41
Review mode: triage-approved (single reviewer)
Summary
Implements issue #1092 (agentic iterative validation): the deep-review tier is now prompted to confirm suspected logic/correctness findings by running the repo's lint/test tools inside its existing Bash sandbox, tagging each with a verification outcome. A new pure-jq post-processor (scripts/lib/finding-verification.sh) downgrades refuted findings by one severity (drops refuted info findings), never touches unverifiable or non-logic findings (reward-hacking guard), and emits kind:finding_verification audit records via a new emit_verification_record() in token-metrics.sh. Wired into review-one-pr.sh after the deep tier. Covered by 13 bats unit tests and an e2e scenario proving the DEEP_TIMEOUT_SEC bound. All five bot-review threads were addressed and resolved; all CI checks are green including shellcheck, bats, unit-tests, holdout-guard, CodeQL, and gitleaks.
Linked issue analysis
Closes #1092. AC1 (validation trigger + repro step): implemented via prompt steps in deep-review.md/deep-review-logic.md, scoped to logic/correctness categories. AC2 (downgrade/drop + finding_verification records): implemented in apply_finding_verification with per-finding audit records. AC3 (no privilege escalation, time-bounded): the repro runs inside the existing run_agentic sandbox (Bash/Read/Grep/Glob, DEEP_TIMEOUT_SEC); the post-processor is jq-only; guard documented in the lib header and proven by e2e scenario 08. AC4 (holdout eval non-regression): the holdout-guard CI check passed; the unverifiable-never-downgrades rule guards against dropping true findings to game the FP rate. The diff substantively addresses the issue.
Findings
No blocking findings. Minor, non-blocking notes: (1) _fv_is_logic_category in finding-verification.sh appears unused — the classification is done inside the jq program — and could be removed; (2) in apply_finding_verification, the final jq 'del(._events)' "$tmp" > "$file" would leave $file truncated if jq failed mid-write, though this is effectively unreachable since jq just produced $tmp. Secret scan: the run_secret_scanning MCP tool is not available in this environment; the gitleaks CI check passed and no credential-like content appears in the diff.
CI status
All required checks green (shellcheck, ShellCheck, Lint, bats, unit, unit-tests, CodeQL, Analyze actions/python, SonarCloud, gitleaks secret scan, agent-shield, holdout-guard, prompt-coverage, validate-fixtures, guard, template-drift, and org stub/permission checks). Conditional checks (dependabot-automerge, language-specific dependency audits, dev-lead ci-relay) skipped as expected.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/lib/finding-verification.sh`:
- Around line 54-73: Remove the unused _fv_is_logic_category and
_fv_downgrade_severity functions from scripts/lib/finding-verification.sh, and
remove the corresponding _fv_downgrade_severity unit test from
tests/dev-lead/unit/test_finding_verification.bats; leave the jq logic in
apply_finding_verification unchanged.
- Line 146: Update the jq transformation in the finding-verification flow to
write to a separate temporary file first, and only atomically replace $file with
mv after jq succeeds. Preserve the original $file when jq fails, including the
existing failure-handling behavior, and ensure the temporary file is cleaned up
appropriately.
In `@tests/dev-lead/e2e/scenarios/08-finding-verification-timeout.sh`:
- Around line 63-82: Update the subshell invocation around run_agentic in the
timeout scenario to keep set -e enabled and capture failures via an || fallback
assignment to rc. Remove the set +e/set -e toggles, preserve the existing
environment setup and output suppression, and ensure successful subshell
execution leaves rc as 0 while failures retain the actual exit code.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88cb55f3-d071-45b8-bfe2-947051acd846
📒 Files selected for processing (7)
prompts/deep-review-logic.mdprompts/deep-review.mdscripts/lib/finding-verification.shscripts/lib/token-metrics.shscripts/review-one-pr.shtests/dev-lead/e2e/scenarios/08-finding-verification-timeout.shtests/dev-lead/unit/test_finding_verification.bats
| _fv_is_logic_category() { | ||
| case "${1:-}" in | ||
| logic | correctness) return 0 ;; | ||
| *) return 1 ;; | ||
| esac | ||
| } | ||
|
|
||
| # _fv_downgrade_severity <severity> | ||
| # Prints the next-lower deep-review severity. `info` prints empty (the caller | ||
| # drops the finding). An unrecognized severity prints itself (no change) so an | ||
| # unexpected value can never silently escalate or vanish. | ||
| _fv_downgrade_severity() { | ||
| case "${1:-}" in | ||
| critical) printf 'major' ;; | ||
| major) printf 'minor' ;; | ||
| minor) printf 'info' ;; | ||
| info) printf '' ;; | ||
| *) printf '%s' "${1:-}" ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove unused Bash functions and their corresponding tests.
The shell functions _fv_is_logic_category and _fv_downgrade_severity are dead code. Their logic was natively migrated into the jq payload inside apply_finding_verification (lines 96-103) and the shell implementations are never invoked.
scripts/lib/finding-verification.sh#L54-L73: Remove the unused functions.tests/dev-lead/unit/test_finding_verification.bats#L70-L77: Remove the now-dead unit test targeting_fv_downgrade_severity.
📍 Affects 2 files
scripts/lib/finding-verification.sh#L54-L73(this comment)tests/dev-lead/unit/test_finding_verification.bats#L70-L77
🤖 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 `@scripts/lib/finding-verification.sh` around lines 54 - 73, Remove the unused
_fv_is_logic_category and _fv_downgrade_severity functions from
scripts/lib/finding-verification.sh, and remove the corresponding
_fv_downgrade_severity unit test from
tests/dev-lead/unit/test_finding_verification.bats; leave the jq logic in
apply_finding_verification unchanged.
| emit_verification_record "$workflow" "$tier" "$outcome" "$sev_before" "$sev_after" "$idx" "$context" | ||
| done < <(jq -r '._events[]? | "\(.index)\t\(.outcome)\t\(.severity_before)\t\(.severity_after)"' "$tmp" 2>/dev/null) | ||
|
|
||
| jq 'del(._events)' "$tmp" > "$file" 2>/dev/null || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent file truncation if jq fails.
The redirection > "$file" truncates the target file to 0 bytes before jq executes. If jq fails (e.g., due to an unexpected input or memory limit), $file will be left empty rather than containing the unmodified original JSON. This empty file will then break downstream stages that expect valid JSON.
Redirect the output to a safe temporary file first, then use mv to atomically replace the original file.
💡 Proposed fix
- jq 'del(._events)' "$tmp" > "$file" 2>/dev/null || true
+ jq 'del(._events)' "$tmp" > "${tmp}.clean" 2>/dev/null && mv "${tmp}.clean" "$file" || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jq 'del(._events)' "$tmp" > "$file" 2>/dev/null || true | |
| jq 'del(._events)' "$tmp" > "${tmp}.clean" 2>/dev/null && mv "${tmp}.clean" "$file" || true |
🤖 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 `@scripts/lib/finding-verification.sh` at line 146, Update the jq
transformation in the finding-verification flow to write to a separate temporary
file first, and only atomically replace $file with mv after jq succeeds.
Preserve the original $file when jq fails, including the existing
failure-handling behavior, and ensure the temporary file is cleaned up
appropriately.
| set +e | ||
| ( | ||
| export PATH="${STUB_BIN_DIR}:${PATH}" | ||
| export REVIEW_ENGINE="claude" | ||
| export DEV_LEAD_DRY_RUN="false" | ||
| export GITHUB_ENV="${TMP_ENV}" | ||
| export GITHUB_OUTPUT="/dev/null" | ||
| export DEEP_TIMEOUT_SEC="${budget}" | ||
| export STUB_ENGINE_DELAY="${stub_delay}" | ||
| # Ensure token logging is off so the stub path stays simple and no usage | ||
| # sidecar work happens. | ||
| unset TOKEN_LOG_FILE | ||
| # shellcheck source=../../../../scripts/engine.sh | ||
| source "${ENGINE_SCRIPT}" | ||
| # Pass an explicit model (≠ the tier default) so the in-Claude fallback chain | ||
| # is a single element — the timeout fires once, no chain re-attempts. | ||
| run_agentic "${TMP_PROMPT}" "e2e-timeout-model" "deep" >/dev/null 2>&1 | ||
| ) | ||
| rc=$? | ||
| set -e |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Capture exit codes natively without toggling set +e.
Disabling errexit to capture a subshell failure is unnecessary and can mask unintended failures. You can capture the subshell's exit code securely while leaving set -e active by utilizing the || operator. If the subshell returns 0, the assignment is skipped (leaving rc=0); if it fails, rc receives the actual non-zero exit code.
💡 Proposed fix
- set +e
- (
+ (
export PATH="${STUB_BIN_DIR}:${PATH}"
export REVIEW_ENGINE="claude"
export DEV_LEAD_DRY_RUN="false"
export GITHUB_ENV="${TMP_ENV}"
export GITHUB_OUTPUT="/dev/null"
export DEEP_TIMEOUT_SEC="${budget}"
export STUB_ENGINE_DELAY="${stub_delay}"
# Ensure token logging is off so the stub path stays simple and no usage
# sidecar work happens.
unset TOKEN_LOG_FILE
# shellcheck source=../../../../scripts/engine.sh
source "${ENGINE_SCRIPT}"
# Pass an explicit model (≠ the tier default) so the in-Claude fallback chain
# is a single element — the timeout fires once, no chain re-attempts.
run_agentic "${TMP_PROMPT}" "e2e-timeout-model" "deep" >/dev/null 2>&1
- )
- rc=$?
- set -e
+ ) || rc=$?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set +e | |
| ( | |
| export PATH="${STUB_BIN_DIR}:${PATH}" | |
| export REVIEW_ENGINE="claude" | |
| export DEV_LEAD_DRY_RUN="false" | |
| export GITHUB_ENV="${TMP_ENV}" | |
| export GITHUB_OUTPUT="/dev/null" | |
| export DEEP_TIMEOUT_SEC="${budget}" | |
| export STUB_ENGINE_DELAY="${stub_delay}" | |
| # Ensure token logging is off so the stub path stays simple and no usage | |
| # sidecar work happens. | |
| unset TOKEN_LOG_FILE | |
| # shellcheck source=../../../../scripts/engine.sh | |
| source "${ENGINE_SCRIPT}" | |
| # Pass an explicit model (≠ the tier default) so the in-Claude fallback chain | |
| # is a single element — the timeout fires once, no chain re-attempts. | |
| run_agentic "${TMP_PROMPT}" "e2e-timeout-model" "deep" >/dev/null 2>&1 | |
| ) | |
| rc=$? | |
| set -e | |
| ( | |
| export PATH="${STUB_BIN_DIR}:${PATH}" | |
| export REVIEW_ENGINE="claude" | |
| export DEV_LEAD_DRY_RUN="false" | |
| export GITHUB_ENV="${TMP_ENV}" | |
| export GITHUB_OUTPUT="/dev/null" | |
| export DEEP_TIMEOUT_SEC="${budget}" | |
| export STUB_ENGINE_DELAY="${stub_delay}" | |
| # Ensure token logging is off so the stub path stays simple and no usage | |
| # sidecar work happens. | |
| unset TOKEN_LOG_FILE | |
| # shellcheck source=../../../../scripts/engine.sh | |
| source "${ENGINE_SCRIPT}" | |
| # Pass an explicit model (≠ the tier default) so the in-Claude fallback chain | |
| # is a single element — the timeout fires once, no chain re-attempts. | |
| run_agentic "${TMP_PROMPT}" "e2e-timeout-model" "deep" >/dev/null 2>&1 | |
| ) || rc=$? |
🤖 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/dev-lead/e2e/scenarios/08-finding-verification-timeout.sh` around lines
63 - 82, Update the subshell invocation around run_agentic in the timeout
scenario to keep set -e enabled and capture failures via an || fallback
assignment to rc. Remove the set +e/set -e toggles, preserve the existing
environment setup and output suppression, and ensure successful subshell
execution leaves rc as 0 while failures retain the actual exit code.
Source: Linters/SAST tools
Closes #1092
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
Bug Fixes
Tests