Repository navigation
A second lens: a second reviewer account reads every head the review approves - #207
Conversation
…approves A caller on a self-hosted runner that passes second-reviewer-login gets a second read of each approved head in the same job, as that account, with a second env file and rubric the host keeps for it. The post caller posts that verdict with the second-reviewer-token secret. The second lens has its own artifact (redline-verdict-second), history and round cap. Its step cannot fail or stall the job, so the job's result stays the first review's verdict. A pull request that only moves pins is approved without a model call once every commit it pins is on ci-repo's main. Off by default: a caller that passes nothing new runs as before.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Verdict: request changes. The second verdict is separated from the first in review history and posting, but the pin fast path accepts more than pin moves, and a second-artifact upload failure can still change the first review's result. The added tests cover the normal paths but not these failure cases.
1. Blocking: scripts/redline/review.mjs:1242
if (!pinOnly(files) || shas.length === 0) return { ok: false, shas, why: 'the change is not only pin moves' };
pinOnly checks each added and removed line independently, not whether the action or workflow stays the same. For example, replacing uses: askalf/ci/actions/checkout-with-retry@<sha> with uses: askalf/ci/actions/drift-report@<sha> in a workflow passes this check. If the SHA is on main, pinApproval returns ok and the second reviewer approves without reading a change that replaces the executed action, rather than merely moving its pin. Deleting an action line alongside another pin bump also passes.
Suggested fix:
Use a stricter predicate for deterministic approval that pairs removed and added lines and preserves action/workflow paths, indentation, and structure while allowing only revision changes. Add tests for action replacement and deletion alongside a valid pin bump.
2. Blocking: .github/workflows/redline-review.yml:555
- name: Upload the second lens's verdict if: always() && steps.second.outcome != 'skipped' && hashFiles('redline-verdict-second/verdict.json') != '' uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2
Only the second review step has continue-on-error. If the first review approves and its artifact uploads successfully, but uploading the second artifact fails, this step fails the whole job. The first APPROVE artifact then disagrees with the run's failure conclusion and post.mjs refuses to post it; the second post also skips the failed run. Thus an optional second-review upload can suppress the first verdict and fail its required check.
Suggested fix:
Make the second artifact upload non-fatal as well, and cover that property in the workflow tests so second-review infrastructure failures preserve the first verdict.
rule:none
…cannot fail the job The approval without a model call took any change whose lines were all pins, so a pin swapped for another action's, or deleted next to a real bump, passed. pinMoves now pairs every removed pin with the same pin at another commit, line for line, and pinApproval also asks that each new commit is ahead of the one it replaces, so a pin taken back to an older commit is read by the model. The upload of the second verdict has continue-on-error like the step before it: a failed upload failed the job, which then disagreed with the first review's approval and left it unposted.
sprayberry-redline
left a comment
There was a problem hiding this comment.
Verdict: approve. The pin approval now requires matching in-place replacements and forward commits on main, with regression tests for swapped, added, deleted and backward pins. Both earlier findings are resolved: the second artifact upload is also non-fatal, while the separate artifact, reviewer identity and posting checks keep the two verdicts independent.
…dread (#10) The Redline caller and the post caller move to askalf/ci at f972757 (askalf/ci#207) and turn the second lens on: every head Redline approves is read once more in the same job, as sprayberry-secondread, with a rubric the runner host keeps for it, and the post job posts that verdict with REDLINE_SECOND_REVIEWER_TOKEN.
A second reviewer account can now read every head the review approves, in the same job. It is off unless a caller turns it on: a caller that passes nothing new runs exactly as before.
Why
A ruleset that requires two approving reviews needs two reviewers. One model reading the same change twice adds little; a second reader with a narrower rubric, on a model the host picks for it, adds a different read. This gives a caller that second reader without a second workflow, a second pin or a second dispatch.
What changes
redline-review.ymlsecond-reviewer-login(empty by default),second-env-fileandsecond-prompt-file(host paths, next to the first two).Second lensstep runsreview.mjsagain after the review step passed, as that account, withREDLINE_LENS=secondand the second env file and rubric. Its verdict is uploaded asredline-verdict-second.continue-on-error, and the step has its own timeout, so the job's result stays the first review's verdict. Everything that reads a run's conclusion keeps reading what it reads today.review.mjsLENS(fromREDLINE_LENS, empty orsecond) names the verdict artifact, so each lens has its own history and round cap.REDLINE_PIN_APPROVAL=1, which the second-lens step sets, a pull request that only moves pins is approved without a model call once three things are seen: every changed line is a pin replaced in place by the same pin at another commit (pinMoves), every commit it pins is onci-repo's main, and each is ahead of the commit it replaces. The approval opens with "Deterministic approval" and says no model read the head. A pin swapped for another action's, added, deleted or taken back to an older commit, a commit that is not on main, or a compare that cannot be read, sends the head to the model.redline-post-verdict.ymlandpost.mjssecond-reviewer-loginand optional secretsecond-reviewer-token. With the login set, the post script runs once more for the second verdict, as the second account.Tests
review.test.mjs: the lens constant in a fresh process, which patches are pins moved in place (a swapped, added, deleted, traded, re-indented or unchanged pin is not), the pin approval end to end against the stubbed GitHub (approved and posted, left for the post, dry run, off main, moved backwards, unreadable compares, a swapped or deleted pin, off by default, mixed change, re-run), and the workflow's new steps, order, timeouts, the two steps that may fail without failing the job, and cleanup, with the runner check's shell run for the hosted refusal.post.test.mjs: rule 4 for the second lens, and the post workflow's second step, its secret and its inputs.node test-all.mjspasses here apart from two checks inreview.test.mjsthat fail on this Windows machine before the change as well (the hosted-mode output paths).actionlintis clean on both workflows.