feat: implement issue #612 — [Phase 1] Route pr_diff review through the artifact contract (behavior-preserving extraction) - #746
Conversation
…he artifact contract (behavior-preserving extraction)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthrough
ChangesRegistry-driven pr_diff dispatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. |
![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 refactors the PR review script to dynamically resolve the review rubric and output channel from a rubric registry helper instead of using hardcoded paths, and introduces a comprehensive BATS test suite to verify this dispatch wiring. The feedback suggests stripping trailing carriage returns from the resolved registry values to prevent execution failures on Windows environments.
![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 — waiting on PR blockers (intent: review-changes)PR: #746 |
|
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. |
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: LOW
Reviewed commit: a36c997b96285d2a3de01536fac6961b14d37dfa
Review mode: triage-approved (single reviewer)
Summary
Phase 1 of the artifact-contract extraction (epic #610, issue #612): scripts/review-one-pr.sh now resolves its {rubric, output_channel} from the rubric registry keyed on artifact_type=pr_diff instead of hard-coding the cascade. Behavior-preserving — the registry manifest resolves pr_diff to the existing triage->deep-review->synthesize prompts and scripts/post-pr-review.sh, so production review is identical. +120/-5 across 3 files.
Linked issue analysis
Closes #612. All acceptance criteria verified:
- AC#1: review-one-pr.sh sources lib/review-registry.sh and resolves rubric + output_channel via review_registry_lookup for artifact_type=pr_diff (replacing the hard-coded triage/deep prompt paths and post-pr-review.sh call sites).
- AC#2: content_ref is the PR URL; the metadata+diff prefetch is unchanged, so prompts consume the same inlined context.
- AC#3: registry resolves pr_diff output_channel to scripts/post-pr-review.sh (verified at PR head); post-pr-review.sh itself is untouched, so idempotency markers and decision format remain byte-identical.
- AC#4: lint.yml only ADDS tests/test_pr_diff_dispatch.bats to the bats list (no existing entries dropped, no assertions edited); bats CI is green.
- AC#5: DRY_RUN is still passed through to the resolved channel; no change to DRY_RUN/FORCE_REVIEW logic.
Findings
No blocking issues.
- Verified the registry dependency exists at the PR head: scripts/lib/review-registry.sh (from #611) and the versioned manifest scripts/lib/review-registry.tsv, which registers pr_diff -> rubric 'prompts/triage.md,prompts/deep-review.md,prompts/synthesize.md', output_channel 'scripts/post-pr-review.sh'.
- All four referenced targets (triage.md, deep-review.md, synthesize.md, post-pr-review.sh) exist on disk, so the dispatch cannot fail to resolve.
- 'bash "$REVIEW_OUTPUT_CHANNEL"' executes a value sourced only from the in-repo versioned manifest, not from PR/user input — no injection surface.
- Minor (non-blocking): the dispatch binds rubric entries [0]/[1] (triage, deep) but not [2] (synthesize), which is still invoked via its existing hard-coded path. This is consistent with the Phase-1 behavior-preserving intent and is not a regression.
- Tests in test_pr_diff_dispatch.bats are wiring/grep assertions plus two registry-resolution checks that lock in today's output channel and prompt order.
CI status
All checks green. SUCCESS: Lint, ShellCheck, shellcheck, bats, unit-tests, CodeQL (actions+python), SonarCloud, agent-shield, holdout-guard, Secret scan (gitleaks), Agent Security Scan, validate-agent-profiles, gh-aw-compile, review, CodeRabbit. SKIPPED (n/a): dependabot-automerge, dependency-audit ecosystem jobs, dev-lead/ci-relay. mergeStateStatus=BLOCKED only because reviewDecision=REVIEW_REQUIRED (this review).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 5c556072e21a6802927c64a51be2e77f84fbd8f2
Review mode: triage-approved (single reviewer)
Summary
Phase-1 artifact-contract refactor of scripts/review-one-pr.sh: the production PR path now dispatches through the rubric registry for artifact_type=pr_diff instead of hard-coding prompt/output paths. Resolves to identical behavior — triage→deep-review prompts and post-pr-review.sh output channel — verified against the versioned manifest. Also adds the new bats suite to lint.yml's test list and a wiring test file. Behavior-preserving, well-tested, all CI green.
Linked issue analysis
Closes #612 ([Phase 1] route pr_diff review through the rubric registry). The dispatch sources lib/review-registry.sh (added by merged #611), resolves {rubric, output_channel} for pr_diff, binds cascade entry 1→triage and entry 2→deep-review, and posts via the resolved channel. Confirmed scripts/lib/review-registry.tsv at the head SHA registers pr_diff → 'prompts/triage.md,prompts/deep-review.md,prompts/synthesize.md' and output_channel 'scripts/post-pr-review.sh', matching every call site and test assertion. AC #1 (registry-driven dispatch) and AC #3 (resolves to today's behavior) are met.
Findings
No blocking findings.
- Dispatch indirection is a no-op: resolved rubric/output_channel equal the previously hard-coded values, so the cascade behaves identically.
- bash "$REVIEW_OUTPUT_CHANNEL" sources its value from the repo-versioned manifest (not user input), so no command-injection exposure.
- gemini-code-assist's sole suggestion (strip trailing CR for Windows) is already implemented: both lookups pipe through 'tr -d \r'.
- Minor (non-blocking): the rubric's third entry (synthesize.md) is registered but not separately bound at the dispatch layer; consistent with the existing code and fine for Phase 1.
- New wiring tests lock the contract without running the live cascade; lint.yml correctly adds tests/test_pr_diff_dispatch.bats to the suite.
CI status
All required checks green: CodeQL, shellcheck/ShellCheck, bats, unit-tests, Lint, SonarCloud, Agent Security Scan, gitleaks, validate-agent-profiles, gh-aw-compile, holdout-guard, agent-shield. Conditional dependency-audit/dependabot jobs SKIPPED (expected). reviewDecision=APPROVED. CodeRabbit is a non-blocking advisory bot.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
Closes #612
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Chores
Refactor