Roll out reliable AI review evidence in Harper Pro - #748
Conversation
This comment has been minimized.
This comment has been minimized.
|
Reviewed; no blockers found. |
kriszyp's review on harper#2256: the validator was left on 224c2ad while the claude/gemini callers moved to 4b59dc0 — the #86 delta changes the workflow contract the validator checks callers against, so the pins move together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4VjVDt6egBUxmGiASuahs
kriszyp
left a comment
There was a problem hiding this comment.
🤖 Pure CI-plumbing PR: bumps the ai-review-prompts pin for the Claude review caller, the Gemini review caller, and the caller-workflow validator from 224c2ad (#80) to 4b59dc0 (#86, "reliable review evidence, context, and run binding"). No product code is touched. Both the uses: …@<sha> ref and the duplicated ai-review-prompts-ref: input were updated consistently in all three files (claude-review.yml:35, claude-review.yml:61, gemini-review.yml:53, gemini-review.yml:71, validate-caller-workflows.yml:24, validate-caller-workflows.yml:28), and the pin comments were updated in lockstep — the mechanical part is correct and complete.
🤖 Reviewed with Claude
| # `_claude-review.yml`'s authorize `if:`, not in this caller. | ||
| if: ${{ vars.CLAUDE_ALWAYS_ON == 'true' || github.event.action == 'labeled' }} | ||
| uses: HarperFast/ai-review-prompts/.github/workflows/_claude-review.yml@224c2adcfdb408d68c7ca48eb618623c990a96ad # main 2026-07-28 (#80 week-of-07-20 calibration: producing-side guard verification; on 82c9484) | ||
| uses: HarperFast/ai-review-prompts/.github/workflows/_claude-review.yml@4b59dc0ddb15aff517884127b58204f79193b4f2 # main 2026-08-20 (#86 reliable review evidence, context, and run binding) |
There was a problem hiding this comment.
Bumping the pin also re-opens the caller-side contract, and this is the one part of the upgrade that fails silently rather than loudly.
The permissions: block just below (contents: read, pull-requests: write, id-token: write) is documented as "union of what the reusable's authorize and review jobs declare" — i.e. it's a hand-maintained mirror of the old (#80) reusable. GitHub intersects the reusable's per-job requests with this caller ceiling and drops un-grantable scopes without erroring. So if _claude-review.yml@4b59dc0 added a scope for the new "run binding / evidence" behavior — actions: read is the obvious candidate if it now reads workflow-run metadata, job logs, or artifacts to bind the review to a run — the job will start fine and then fail (or quietly degrade to a review without the new context) at the step that needs it, which is exactly the failure mode this change is meant to eliminate.
Same question for secrets: at lines 85-89 and for the mirrored blocks in gemini-review.yml (permissions at 60-63, secrets at 95-99).
Could you diff _claude-review.yml/_gemini-review.yml between 224c2ad and 4b59dc0 for any added permissions: entry, secrets: entry, or newly-required inputs: and reconcile all four blocks? If nothing changed, a one-line note in the PR description saying the contract is unchanged at #86 would be enough to close this out.
| jobs: | ||
| validate: | ||
| uses: HarperFast/ai-review-prompts/.github/workflows/_validate-caller-workflows.yml@224c2adcfdb408d68c7ca48eb618623c990a96ad # main 2026-07-28 (#80 week-of-07-20 calibration: producing-side guard verification; on 82c9484) | ||
| uses: HarperFast/ai-review-prompts/.github/workflows/_validate-caller-workflows.yml@4b59dc0ddb15aff517884127b58204f79193b4f2 # main 2026-08-20 (#86 reliable review evidence, context, and run binding) |
There was a problem hiding this comment.
This bumps the validator to #86 while two other callers in this repo stay on the #79 pin: claude-mention.yml:17 and claude-issue-to-pr.yml:15 (both 54d9e61).
That asymmetry matters more here than for a normal caller, because per this file's header the validate job is meant to be a required status check on main and it runs on every PR with no paths: filter. The validator inspects caller files on disk, not the version they pin — so if #86 added any new caller-shape rule (say, a required permissions: block, or a pin-comment format), the two lagging files trip it and every PR in the repo goes red until they're fixed, including PRs that touch no workflows at all.
Two suggestions:
- Either bump
claude-mention.ymlandclaude-issue-to-pr.ymlto4b59dc0in this PR (keeps all five callers on one upstream version, which is also easier to reason about since layer files and bash scripts are checked out per-caller at each caller's own ref), or confirm from the Track GPU utilization alongside CPU in analytics profiler #86 diff that no new validator rule was introduced. - While here: the comment at lines 5-6 says the validator covers
.github/workflows/claude-*.yml, which doesn't globgemini-review.yml. If the upstream validator does check the Gemini caller, that line is stale and worth correcting alongside the pin.
| # the CALLER's ref in `workflow_call` context), and `uses: …@<ref>` | ||
| # is parsed literally so we can't interpolate a variable. | ||
| ai-review-prompts-ref: 224c2adcfdb408d68c7ca48eb618623c990a96ad | ||
| ai-review-prompts-ref: 4b59dc0ddb15aff517884127b58204f79193b4f2 |
There was a problem hiding this comment.
Adjacent to the pin you're bumping: the repo-specific-checks: block below (lines 82-84, and the identical copy at gemini-review.yml:92-94) tells both reviewers:
Tests. No unit-test split —
npm run test:integrationis the test surface.
That's no longer true. package.json:112 defines test:unit (mocha --require unitTests/unitTestSetup.cjs 'unitTests/**/*.test.mjs'), and AGENTS.md documents it as the fast, no-server test path that should be preferred over the slow integration suite. The same stale claim is in claude-mention.yml:30 and claude-issue-to-pr.yml:28-29 ("There is no npm test / test:unit script").
Net effect is that a PR whose change is unit-testable gets reviewed — and, in the mention/issue-to-pr workflows, validated — under the assumption that the only option is the expensive integration run, so the reviewer either skips test advice or pushes toward the slow path. Since this PR is the "reliable review context" roll-out, correcting the context the callers actually ship seems in scope:
- **Tests.** `npm run test:unit` (mocha, requires a built
`dist/` — run `npm run build` first) for fast unit coverage;
`npm run test:integration` for the integration surface. The
integration suite is slow — use it judiciously.
Keep the two review callers byte-identical here, per the sync note at gemini-review.yml:7-10.
Roll out the reliable review-evidence plumbing from HarperFast/ai-review-prompts#86 in Harper Pro by moving the Claude, Gemini, and caller-validator workflows to its verified merge commit. This keeps the review and validation paths on one immutable workflow, prompt, and script version.
For the human reviewer
No open judgment calls. The three caller pins move together to avoid validating the new review callers against a different upstream revision. Mention, issue-to-PR, permissions, triggers, secrets, layers, and repo-specific checks are unchanged.
Verification
git diff --checkpassed.4b59dc0ddb15aff517884127b58204f79193b4f2as the signed, verified merge commit for Make AI review calibration evidence reliable ai-review-prompts#86.1bbcad9…head and reported no blockers; all ordinary CI passed.Review coverage
Authored by Codex, with the validator alignment follow-up authored by Claude Fable 5 from Kris Zyp's canary feedback. Independent Claude review covered the initial pin bump; Gemini reviewed the final head with no findings. Cursor and further domain adjudication were pruned by the minimal low-risk policy. Receipt @ 1bbcad9.
Human-Review-Need: 2 @ 1bbcad9