fix(ce-code-review): bound the validator wait on an on-disk verdicts file (EveryInc#1688)

Author: tmchow

docs/solutions/skill-design/anti-poll-scope-and-async-subagent-dispatch.md

@@ -48,7 +48,7 @@ A portable rule classifies what dispatch actually returned:
 - **Launch identifier or asynchronous receipt:** a launch receipt means the reviewer is uncollected. Use the host's blocking collection capability until the launch reaches a terminal outcome. That outcome may arrive as the call's own return, as a blocking wait's return, or as a host-delivered terminal message that names the launch and carries its final payload; a status or progress update is none of these.
 - **No reliable blocking collector:** the host offers no in-turn way to reach a terminal outcome for the launch. Stop the launched work and take the workflow's failure or degraded path. Fail closed only after discharging lifecycle obligations for detached work already started. Never end the turn to wait, emit progress-only output, or synthesize a partial roster.
 
-The same classification governs both reviewer batches and later validator batches. A foreground request is an intent, not evidence that a result arrived. Judge a collector by its live contract, not its name: a wait counts only when it blocks until terminal, and a message counts only when it identifies the launch and carries the payload. Issue #1654 showed why the rule must be the condition rather than a tool shape: Codex delivers a subagent's final answer as a host message tagged with the launch's task name while `wait_agent` reports status, so a rule demanding one ID-addressed collector that returns the outcome failed closed on a host that could collect.
+The same classification governs both reviewer batches and later validator batches, with one addition for the validator (#1679): its wait has a bound, the validator writes its verdicts to a file in the run directory, and a verdicts file missing at the bound is infrastructure failure rather than "still running". A blocking call that cannot be bounded is not a collector for that batch. A foreground request is an intent, not evidence that a result arrived. Judge a collector by its live contract, not its name: a wait counts only when it blocks until terminal, and a message counts only when it identifies the launch and carries the payload. Issue #1654 showed why the rule must be the condition rather than a tool shape: Codex delivers a subagent's final answer as a host message tagged with the launch's task name while `wait_agent` reports status, so a rule demanding one ID-addressed collector that returns the outcome failed closed on a host that could collect.
 
 For an asynchronous primitive, the rule still needs three explicit clauses:
 

skills/ce-code-review/references/finish-review.md

@@ -59,9 +59,9 @@ This stage is an optional second check on the findings. Independent verification
 1. Skip a validator only when the finding has `first_evidence` and both an ordinary reviewer plus an `adversarial-<provider>` reviewer whose artifact records `independence_verified:true`. Same-model corroboration never licenses this shortcut. Record the skip count in Coverage.
 2. Select every remaining P0/P1 and every remaining actionable finding. Single-source P0s are always selected. P2/P3 advisory items should already have moved to the soft buckets; do not validate them merely to keep them primary.
 3. Put the selected set into **one** deterministic validator batch, ordered by severity and stable `#`, using `references/validator-batch-template.md`. Eight findings is the normal cap. When more than eight P0/P1 survive, expand that same batch past the normal cap to include every surviving P0/P1 rather than silently omitting a blocker; never split the work into another batch. The validator returns one independent verdict per finding. Cost, elapsed time, confidence, or a finding appearing mechanically obvious never licenses an additional skip; when the required validator cannot run, keep P0/P1 as validation-degraded and say so.
-4. Run the validator batch foreground with background execution off, then classify the observed return rather than trusting that request as a wait guarantee. Collection completes when the successful launch reaches a terminal outcome (the validator has finished, with a verdict or an error). Consume a valid compact validator verdict whether returned in-band or collected asynchronously; classify a terminal tool error or malformed output as validator infrastructure failure under step 5. A launch receipt (the host's acknowledgement that the validator started) is uncollected, not a validator return: use the host's blocking collection capability until the validator reaches a terminal outcome. That outcome may arrive as the call's own return, a blocking wait's return, or a host-delivered terminal message that names the validator launch and carries its verdict; a progress or status update is not one. If no reliable blocking collection exists because the host offers no in-turn way to reach that terminal outcome, stop the launched validator and treat it as validator infrastructure failure under step 5. Never use shell no-ops (`echo waiting`, `noop`, `yield turn`, `end turn`, `true`, or sleeps), detached status polls, scheduled wakeups, or narrated "still waiting" turns.
+4. Launch the validator batch and collect it with a wait that has an end. The verdicts file the template names (`$RUN_DIR/validator-verdicts.json`) is the fact; the call's own return, a blocking wait's return, or a host-delivered terminal message that names the validator launch and carries its verdict is one rendering of it. Consume a valid compact verdict from whichever arrives first, whether returned in-band or collected asynchronously. A launch receipt (the host's acknowledgement that the validator started) is uncollected, not a validator return, and a progress or status update is not a terminal outcome. Use the host's blocking collection capability with a bound: blocking waits for the validator's terminal outcome or for the verdicts file, each as long as the host allows, repeated back to back with nothing between them until verdicts arrive or the aggregate wall-clock limit the template states has passed since launch. A host whose single wait is short (tens of seconds) reaches the limit by repeating the wait, not by treating one short return as the deadline. Where the host's only in-turn collector cannot be bounded, launch the validator so that a bounded blocking wait exists (background execution plus the host's bounded wait) rather than an unbounded foreground call; a wait that cannot be bounded is not a collector for this batch. When no bounded wait exists by any means, do not launch the validator and treat the batch as validator infrastructure failure under step 5. When the bound passes with no valid verdicts file and no terminal outcome, stop the launched validator and treat the batch as validator infrastructure failure as well; classify a terminal tool error, or malformed output with no valid verdicts file, the same way. Never use shell no-ops (`echo waiting`, `noop`, `yield turn`, `end turn`, `true`, or sleeps), detached status polls, scheduled wakeups, or narrated "still waiting" turns; one bounded blocking wait is none of those.
    After collection or stopping the validator, apply the skill's agent lifecycle rule before leaving this stage.
-5. A valid `validated:false` drops that finding and records the reason. On malformed output or validator infrastructure failure, drop affected P2/P3 findings; keep affected P0/P1 as validation-degraded. Prune triage groups after drops and record the batch, per-finding verdicts, failures, and degraded blockers in Coverage.
+5. A valid `validated:false` drops that finding and records the reason. A `validated:"uninspected"` verdict means the validator's budget ran out before it reached that finding; it is validator infrastructure failure for that finding alone. On malformed output or validator infrastructure failure, drop affected P2/P3 findings; keep affected P0/P1 as validation-degraded. Prune triage groups after drops and record the batch, per-finding verdicts, failures, and degraded blockers in Coverage.
 
 ### Stage 5c: Act on findings (explicit local apply only)
 
@@ -136,7 +136,7 @@ Write the human-readable findings through the `ce-noslop` skill. Preserve exact
 7. **Learnings & Past Solutions.** Show the `learnings-researcher` results that are not findings: relevant past solutions, and matched pack rules with no violating line, as "Known Pattern" notes with links to `<root>/solutions/` files or the `(pack: <id>, <path within the pack>)` citation. A pack rule the diff contradicts entered the finding set in Stage 5 and appears in the findings tables, not here; one violated only by unchanged code sits in the pre-existing section, not here.
 8. **Agent-Native Gaps.** Show `agent-native-reviewer` local-prompt results. Omit section if no gaps found.
 9. **Deployment Notes.** If the `deployment-verification-agent` local prompt ran, show the key Go/No-Go items: blocking pre-deploy checks, the most important verification queries, rollback caveats, and monitoring focus areas. Keep the checklist actionable rather than dropping it into Coverage. Schema drift appears in the findings tables as `data-migration` P1 rows; do not add a separate Schema Drift section.
-10. **Coverage.** Applied count (when Stage 5c ran), suppressed count by anchor (e.g., "N findings suppressed at anchor 50, M at anchor 25"), mode-aware demotion count, validator drop count and reasons (when Stage 5b ran), any P0/P1 with degraded validation (kept on validator infrastructure failure), residual risks, testing gaps, failed/timed-out reviewers, and inferred-intent uncertainty when applicable. When the Stage 3c (Small-diff fast path) lite roster ran, state it and the reduced reviewer set (so the narrower coverage is visible). When Stage 5b skipped validators for quote-anchored cross-model-corroborated findings, state how many and name that evidence basis; also state the one-batch result for every remaining selected finding. When the Stage 5 step 3 quote-the-line check demoted any 75/100 finding for missing `first_evidence`, record that count alongside `first_evidence_backfilled` (including zero). When no plan was discovered in Stage 2b (or discovery was ambiguous and skipped), note that settlement suppression was not evaluated. When the plan was `plan_source: inferred` and Stage 5 step 2 (Settled decisions) discarded findings, note the inferred source and count of preference findings suppressed. **Removable surface (only when deletion-oriented maintainability findings exist):** one line giving the approximate net lines/files those findings would remove if applied (e.g., "Removable surface: ~120 lines / 2 files across findings #4, #7"). This is a dead-weight signal, **not** a reduction target: never lower the bar for a finding or invent deletions to grow the number, and omit the line entirely when no finding proposes a deletion.
+10. **Coverage.** Applied count (when Stage 5c ran), suppressed count by anchor (e.g., "N findings suppressed at anchor 50, M at anchor 25"), mode-aware demotion count, validator drop count and reasons (when Stage 5b ran), any P0/P1 with degraded validation (kept on validator infrastructure failure), residual risks, testing gaps, failed/timed-out reviewers, and inferred-intent uncertainty when applicable. When the Stage 3c (Small-diff fast path) lite roster ran, state it and the reduced reviewer set (so the narrower coverage is visible). When Stage 5b skipped validators for quote-anchored cross-model-corroborated findings, state how many and name that evidence basis; also state the one-batch result for every remaining selected finding, and whether the verdicts landed or the wait's bound was exceeded. When the Stage 5 step 3 quote-the-line check demoted any 75/100 finding for missing `first_evidence`, record that count alongside `first_evidence_backfilled` (including zero). When no plan was discovered in Stage 2b (or discovery was ambiguous and skipped), note that settlement suppression was not evaluated. When the plan was `plan_source: inferred` and Stage 5 step 2 (Settled decisions) discarded findings, note the inferred source and count of preference findings suppressed. **Removable surface (only when deletion-oriented maintainability findings exist):** one line giving the approximate net lines/files those findings would remove if applied (e.g., "Removable surface: ~120 lines / 2 files across findings #4, #7"). This is a dead-weight signal, **not** a reduction target: never lower the bar for a finding or invent deletions to grow the number, and omit the line entirely when no finding proposes a deletion.
 11. **Verdict.** Ready to merge / Ready with fixes / Not ready. Fix order if applicable. When an `explicit` plan has unaddressed requirements or implementation units, the verdict must reflect it: a PR that's code-clean but missing planned requirements is "Not ready" unless the omission is intentional. When an `inferred` plan has unaddressed requirements or implementation units, note it in the verdict reasoning but do not block on it alone.
 
 Do not include time estimates.

skills/ce-code-review/references/validator-batch-template.md

@@ -1,6 +1,6 @@
 # Validator Batch Prompt Template
 
-Use one fresh validator subagent for one batch of already-merged findings. Eight findings is the normal cap. When more than eight P0/P1 findings survive, expand that same batch to include every surviving P0/P1; never omit a blocker or split the work into another batch. The validator is independent of the originating reviewers and the orchestrator.
+Use one fresh validator subagent for one batch of already-merged findings. Eight findings is the normal cap. When more than eight P0/P1 findings survive, expand that same batch to include every surviving P0/P1; never omit a blocker or split the work into another batch. The validator is independent of the originating reviewers and the orchestrator. Fill `{run_dir}` with the review's run directory; the orchestrator waits on the verdicts file there, so the file, not the in-band return, is the contract.
 
 ```
 You are the independent validation gate for the code-review findings below. Evaluate each finding separately under fresh inspection. False positives are common; reject a finding when the cited code does not prove it, it predates and is unaffected by this diff, surrounding code handles it, or it is only an unsupported preference.
@@ -21,16 +21,18 @@ Do not let one valid or invalid finding influence another. Do not invent new fin
 
 For local-aligned scope, inspect the cited files, callers, guards, project contracts, and targeted history with read-only tools. For pr-remote or branch-remote scope, use the provided diff and reviewed head ref, never the unrelated workspace copy.
 
-Return exactly one JSON object:
+Budget: the batch has 15 minutes of wall clock and about five tool calls per finding. Inspect findings in the order given. When the budget runs out, stop inspecting and give every remaining finding `"validated": "uninspected"` with the reason `budget exhausted`; never guess a verdict you did not inspect.
+
+Write the JSON object below to `{run_dir}/validator-verdicts.json` before you return, then return the same object:
 {
   "verdicts": [
     {
       "#": <input stable number>,
-      "validated": true | false,
+      "validated": true | false | "uninspected",
       "reason": "<one sentence grounded in inspected evidence>"
     }
   ]
 }
 
-Return one verdict for every input # exactly once. No prose outside JSON. Do not edit, commit, push, or mutate files.
+Return one verdict for every input # exactly once. No prose outside JSON. Writing the verdicts file above is the one permitted write; do not edit project files, commit, push, or otherwise mutate the checkout.
 ```

tests/review-skill-contract.test.ts

@@ -635,7 +635,16 @@ describe("ce-code-review contract", () => {
     expect(content).toMatch(/Eight findings is the normal cap/i)
     expect(content).toMatch(/expand that same batch.*include every surviving P0\/P1/i)
     expect(content).toMatch(/never split the work into another batch/i)
-    expect(content).toMatch(/Run the validator batch foreground/i)
+    // #1679: a foreground-only collector has no end on hosts whose blocking call
+    // cannot be bounded, so the contract is a bounded wait on the verdicts file.
+    expect(content).not.toMatch(/Run the validator batch foreground/i)
+    expect(content).toMatch(/wait that has an end/i)
+    expect(content).toMatch(/validator-verdicts\.json/)
+    // Codex's wait_agent caps a single wait at ~30s (PR #1688 review): the bound is aggregate, not per wait.
+    expect(content).toMatch(/repeated back to back.*aggregate wall-clock limit/i)
+    expect(content).toMatch(/no bounded wait exists.*do not launch the validator/i)
+    expect(content).toMatch(/bound passes.*validator infrastructure failure/i)
+    expect(content).toMatch(/uninspected.*validator infrastructure failure for that finding/i)
     expect(content).toMatch(/Cost, elapsed time, confidence.*never licenses an additional skip/i)
 
     // Foreground is a request, not proof that the host returned a verdict in-band.
@@ -659,6 +668,13 @@ describe("ce-code-review contract", () => {
     expect(validatorTemplate).toMatch(/surrounding code handles it/i)
     expect(validatorTemplate).toMatch(/one verdict for every input # exactly once/i)
     expect(validatorTemplate).toMatch(/Do not invent new findings/i)
+    // #1679: the validator states its own budget and writes verdicts to disk.
+    expect(validatorTemplate).toMatch(/\d+ minutes of wall clock/i)
+    expect(validatorTemplate).toMatch(/tool calls per finding/i)
+    expect(validatorTemplate).toMatch(/validator-verdicts\.json.*before you return/i)
+    expect(validatorTemplate).toContain('"validated": true | false | "uninspected"')
+    // The read-only rule must carve out the one write the bounded wait depends on.
+    expect(validatorTemplate).toMatch(/one permitted write/i)
   })
 
   test("Stage 5c requires explicit local-apply authority and mode:agent is always report-only", async () => {