review: hold for a human instead of approving when core lenses produced no output - #338
Conversation
…roving when core lenses produced no output PR 328's re-run (31124365377 attempt 2) approved after every core lens died on an API auth error: submission.ts hardcoded all dimensions 'assessed', so computeVerdict's HOLD_FOR_HUMAN gate was unreachable and the plan resolved to APPROVE over seven 'not assessed' note lines. The plan CLI now feeds the dispatcher's real skippedDimensions into computeVerdict. A hold submits no review: the body (hold head, note lines, folded claims, unstuck instructions; never stamped) posts as one standalone PR comment. The conformance gate's plan-match rule gains the hold shape (no review event, no inline comments, no resolutions, exactly the hold comment queued), split into dispatch-gate-plan.ts at the max-lines cap. The cache writer leaves prior fingerprints standing on a hold (the next run reviews in full) and drops only risksPatternsKey, because posting the hold comment collapses the standing guidance comment via hide-older-comments. Blocking findings still win: REQUEST_CHANGES posts with the dead lens disclosed in a note line. Tests: submission-hold.test.ts (plan, gate, and sanitizer-drift shapes, including a replay of the incident's dispatch-result), cache-record hold cases. Full suite 72 files / 1669 tests; lint and typecheck clean.
🦋 Changeset detectedLatest commit: 6c679c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review Guidancegithub-actions (6 files)
Common patterns2 files: Rule 7 (submission-plan conformance check) extracted from - const planStaged = typeof input.submissionPlan === "object" && ...
- if (planStaged !== undefined && submit === undefined) { ... }
- if (planStaged !== undefined && submit !== undefined) { ... }
+ violations.push(
+ ...submissionPlanViolations({
+ items: input.items,
+ submissionPlan: input.submissionPlan,
+ submit,
+ verdictEvent,
+ body,
+ commentCount,
+ }),
+ ); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // carries no `skippedDimensions` and reads as all-assessed, the | ||
| // pre-hold behavior. | ||
| const skippedDimensionNames = new Set( | ||
| (Array.isArray(dispatch.skippedDimensions) |
There was a problem hiding this comment.
question (non-blocking): Is the 'older dispatcher' premise real? Absent skippedDimensions fails open to all-assessed. The dispatcher and this plan CLI ship from the same repo checkout and run within the same workflow run, so a dispatch-result.json staged by an 'older dispatcher' arguably cannot occur in production; the tolerance (explicitly tested as 'reads as all-assessed') mostly buys a silent fail-open path for the exact failure class this PR closes. The whole PR's philosophy elsewhere is fail-closed (gate violations are red runs, cache refuses on holds).
A sketch, not a committable replacement:
Fail closed instead: treat a dispatch-result with no skippedDimensions array as core-dimensions-unavailable (hold), or have the plan CLI refuse the stage file as malformed — and drop the backward-compat test unless mixed-version runs are actually possible.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Regressions (baseline caught, candidate missed)
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
… suggestions Five of the six non-blocking suggestions from the bot's own review of this PR, plus a comment-premise fix for the sixth: - The hold body now folds the #329 blocking-only collapsed pr-level bucket too; composing only prLevelLines + anchored dropped those claims from the hold comment (real gap introduced rebasing over #329). - The dimension-name mapping imports DEFAULT_FINDERS and the new TRIAGE_DIMENSION from dispatch-roster.ts instead of restating the strings, so a rename cannot silently decouple the hold from the dispatcher. - cache-record's hold branch refuses loudly (::warning) on an unreadable queue, mirroring the review-event path; only a readable queue with no hold comment is the benign skip. - dispatch-gate-plan's doc claims exactly what the code enforces (review submission, inline comments, resolutions, hold comment) instead of 'any other posting item'. - New gate test for a present-but-spliced hold comment body. - The 'older dispatcher' comment premise was wrong (the dispatcher always writes skippedDimensions); reworded for the real case, hand-staged eval fixtures.
There was a problem hiding this comment.
Approved — no blocking issues found.
5 of 6 prior review threads resolved; 1 still unaddressed as of 6c679c8:
1 non-blocking thread still open
- question (non-blocking)
workflows/review/lib/submission.ts:522: Is the 'older dispatcher' premise real? Absent skippedDimensions fails open to all-assessed. The dispatcher and this pla...
Non-blocking observations (2)
workflows/review/lib/submission.ts:535suggestion (non-blocking): The rename-safety claim rests on positional indices (DEFAULT_FINDERS[0]/[1]) that reordering or insertion silently breaks.workflows/review/lib/submission.ts:571question (non-blocking): Does a hold have enough teeth as a green run plus one standalone comment?
…sion-footer Conflict was import-block only: main's hold-for-human (#338) added the DimensionStatus/VerdictReason type import beside this branch's version-footer import.
2 participants
What
PR #328's re-run (31124365377 attempt 2) submitted "Approved; no blocking issues found" after every core lens died on an API auth error, with seven "not assessed this run" note lines as the only trace. The verdict layer treated "lens failed to run" the same as "lens ran and found nothing":
submission.tshardcoded every dimension asassessed, socomputeVerdict's HOLD_FOR_HUMAN gate (designed for exactly this) was unreachable, guarded by a throw whose comment says dimension availability "must not feed this CLI without a hold path". This PR builds the hold path.submission.tsfeeds the dispatcher's realskippedDimensionsintocomputeVerdict(correctness-reviewer-> correctness,skill-auditor-> skill-severity,pattern triage-> note-and-continue). A hold plan stages the hold body (head, note lines, folded claims, unstuck instructions; never stamped), no inline comments, no resolutions. Blocking findings still win: with a validated blocking claim the verdict stays REQUEST_CHANGES and the dead lens is disclosed in a note line.add-comment) and submits no review, so the PR shows neither an approval nor a block.dispatch-gate-plan.ts(rule 7, split out ofdispatch-gate.tsat the max-lines cap, rules unchanged) gains the hold shape: no review submission, no inline comments, no resolutions, exactly the plan body queued as one PR comment. A queued review event over a hold plan, or a withheld hold comment, is a blocked red run.cache-record.ts: a hold records no fingerprints (the prior record stands, so the next run reviews in full). It drops onlyrisksPatternsKey, because posting the hold comment collapses the standing guidance comment viahide-older-comments, and the next approving run must repost it.review.mdSteps 3/4/6 describe the hold branch; Steps 7-9 are already APPROVE-gated or code-owned.Sibling of #339, which fixes this incident's trigger (the api-proxy cache-miss guard). This PR is the fail-safe for the class: any future lens-death cause (rate limits, model outage, SDK crash) otherwise auto-approves the same way.
Tests
submission-hold.test.tscovers the plan, gate, and sanitizer-drift shapes, including a replay of the incident's actualdispatch-result.json(now resolves to HOLD_FOR_HUMAN with the correct comment body). Cache-writer hold cases incache-record.test.ts. Full suite 75 files / 1685 tests; lint and typecheck clean.