[Improve] Check finished coding turns against the request in about a second instead of a minutes-long judge pass - #3071
Conversation
…second instead of a minutes-long judge pass
|
1 issue outstanding. See task
Reviewed 8559bef |
…s review feedback
…rom slipping past the completion check
…oof claims, evident defects, and clipped diffs
…validation run stale
…ing the model to discount them
| * What formatters and pre-commit hooks rewrite: whitespace, quote style, | ||
| * trailing commas, semicolons, and wrapping parentheses. | ||
| */ | ||
| const FORMATTING_ONLY_CHARACTERS = /[\s'"`;,()]/g; |
There was a problem hiding this comment.
Removing parentheses is not formatting-only in JavaScript/TypeScript. For example, a test can finish with const value = (a + b) * c, then the source can change to const value = a + b * c; both normalize to the same fingerprint even though behavior changed. The earlier validation is therefore sent as current evidence. Do not strip semantically meaningful syntax such as parentheses when deciding whether validation is stale.
|
|
||
| for (const file of [...new Set(files)] | ||
| .sort() | ||
| .slice(0, MAX_FINGERPRINT_FILES)) { |
There was a problem hiding this comment.
The file/content caps make this fingerprint non-authoritative: after 300 changed paths, edits to a later path never enter the hash; likewise, a changed file over 2 MB is represented only by its byte length. In either case a prior passing test can survive a real source edit and suppress the validation checks. Include all changed content, or make validation evidence stale whenever the fingerprint has to omit or coarsen a file.
There was a problem hiding this comment.
This still leaves the validation evidence non-authoritative for files handled by statIdentity: a same-size edit that preserves the mtime (for example, a copy preserving timestamps) hashes identically, so the earlier passing command is retained for different shipped source. The new regression only changes the mtime, so it cannot catch that case. Hash the content or treat capped files as stale evidence.
| * the worse mistake. When a reformat does move parentheses after a test run, | ||
| * that run reads as stale and the agent is asked to run it again. | ||
| */ | ||
| const FORMATTING_ONLY_CHARACTERS = /[\s'"`;,]/g; |
There was a problem hiding this comment.
Removing backticks, semicolons, and all whitespace is still not behavior-preserving JavaScript normalization. For example, const value = '${token}' and ``const value = ${token}``` normalize to the same fingerprint even though the latter interpolates; return\n{ value: 1 }` versus `return { value: 1 }` has the same issue through ASI. A test before either edit is therefore retained as validation for different source. Keep syntax that can affect parsing/semantics, or conservatively mark such changes stale.
| // Every changed file is read, so an edit anywhere changes the result. | ||
| // Past the caps the bytes are hashed as they are: normalizing them is the | ||
| // expensive part, and skipping it only errs toward calling a run stale. | ||
| const content = await readFile(join(repoPath, file)).then( |
There was a problem hiding this comment.
This still buffers every capped file before hashing it. A task that changes more than 300 files can put an arbitrarily large generated/binary file after that boundary; readFile allocates the entire file, so the completion path can stall or exhaust the worker rather than taking the intended quick/skipped path. Hash capped files with a stream, or retain a bounded fallback that marks earlier validation stale without loading their content.
Problem
After implementation, the coding agent hands a "completion and sanity check" to the hidden
judgesubagent. The judge has read/grep/glob and no step limit, so on a small, no-UI change it can spend minutes and ~100 tool calls re-reading the repository to answer three questions: did the diff do what was asked, does the report match the diff, did anything slip in. A second pass follows any fix. The pull request then gets reviewed anyway.Change
Those questions become a typed, sub-second check that the harness runs itself when a turn ends. The agent is only involved when the check finds something.
Flow
POST /api/mcp/tasks/runs/:runId/completion_check(run-token auth, run must match).clearorskipped: the turn completes as before.flagged(probability >= 0.8): the turn is reopened once with a hidden prompt naming the points, the same mechanism the chat-closeout stop hook uses. The prompt says the check can be wrong and that "change nothing and say why" is a valid answer. The original report is kept and joined with the short follow-up so the task report does not collapse to the correction.The judgment model key stays on the API; only a
ROOMOTE_COMPLETION_GATE=trueflag reaches the sandbox, set at dequeue when a hosted judgment model is configured (an operator-set value is discarded). Any failure anywhere isskippedand never holds a turn.Hosted judgment model only. The check passes
highVolume: true, which rules out the helper-model fallback. A live probe of that fallback (same system prompt, prompt, and answer schema, five recommended small models, 9 turns x 3 questions x 2 runs) showed it is not usable here: one model false-flagged 18 of 54 judgments and another 20 of 54, mostly by answering ~0.99 to everything, and median latency ran 3-16 s with a 43 s tail. The best model still got 3 of 54 wrong. Deployments without a judgment model therefore keep today's judge pass unchanged.Parity with the judge pass
requestUnaddressed, judged over the diff, the recorded commands, and the report. Still asked on a clipped diff: the model is told the diff is clipped and that the diff stat lists every fileplanIncomplete: the agent's latest todo list, read from the transcript; asked only when one existsvalidationContradictedandvalidationMissing. The harness records the parent agent's last 12 shell commands with exit code and output tail as OpenCode reported them, so "tests pass" is compared with what ran, not trusted. Each command also gets a fingerprint of the changed code taken right after it finished; a run counts only while that fingerprint still matches what ships. Nothing is inferred from command text, so an edit made with an editor tool,sed -i, a heredoc script, or a git operation is treated the same, in this turn or a later one. The fingerprint is the changed files' content with formatting-only characters removed, so a formatter, a pre-commit hook, orgit commitafter the tests does not make them stale. Stale runs are dropped server-side rather than handed to the model with a markerproofClaimDoubtful: an interface change with proof waved off or unmentioned. Captured proof and a specific blocker are both acceptedevidentDefect, limited to what the changed lines show on their own: inverted conditions, a removed guard, error check, orawait, a call left on an old signature, a test weakened to passreportOverclaimsleftoverArtifactsDeliberately not ported: defects and edge cases that only show with code outside the diff. That open-ended repository reading is what made the judge pass take minutes, and pull request review covers it. When proof kept images, the judge pass still runs with its original prompt.
What "this task changed" means
Diffing from the fork point is wrong for work on an existing pull request: earlier work on the branch would be judged as this task's, and a requested removal of that earlier work nets to no change at all. The base is therefore the first checkout of the current branch from the HEAD reflog, falling back to the fork point when history was rewritten or the default branch was merged in afterwards. Untracked files count as additions; lockfiles, snapshots, and minified bundles are left out of the patch. Large diffs are clipped per file, largest first, so every file stays represented; with a clipped diff the two "is it in the diff" questions are not asked.
Judge scope
Where the check is on, the
judgesubagent stays for the one thing the decision model cannot do: opening proof images. The runtime judge instructions switch to a proof-only variant that spawns it only whencapture-visual-proofkept screenshots or keyframes, andimplement-changesdefers to them. Where the check is off, the original judge instructions are used as before.Validation
proofClaimDoubtfulflags, all on pull requests that changed interface files without mentioning proof. p50 199 ms, p95 405 ms.validationContradicteduses its own 0.65 threshold; nothing that should stay quiet scored above 0.47 on it.pnpm lint:fast && pnpm check-types:fast && pnpm knippass. Worker harness suite passes.Open before merge
noulmeans, which likely explains the ~0.99-to-everything answers. Worth fixing for the other surfaces that do fall back to it.