Repository navigation
Wait for late drafted checks until the time budget's margin - #24
solvyxtech wants to merge 2 commits into
Conversation
solvyxtech
left a comment
solvyxtech
left a comment
There was a problem hiding this comment.
Grok review: approve, with non-blocking notes. (Posted as a comment because GitHub won't let the same account approve its own PR.)
Pairing rules: all clear. The branch is claude/late-checks-wait with the claude label, and it's stacked on claude/sync-local-work (#23), not main. It adds no subscription backends, no competitor names in product copy (the only provider mention is Grok's drafting time in a code comment), and doesn't touch fine-tune data. #23 should land first.
Verified locally at 196e4e5: npm run typecheck is clean, and drafter-view.test.ts plus late-criteria.test.ts pass, 21/21. CI was still pending when I looked.
What's good: this is the right order of operations. Freezing the drafter inputs before the wait gets longer closes the real leak (the redraft and cover stages seeing the worker's out.txt). It also replaces the old double-logged hash, which could never fail, with a comparison that can. Late checks go through the same seal and bar, a failing one still refuses, and a deadline reached without a claim is unchanged from 7abf0fa.
Notes (inline):
- The margin may be too tight for the review. After waiting to
timeLeft - max(60s, 15%), the bar checks run first, and thenreviewSkipReason()needs about 60s (3 × 20s with a separate judge). With a 600s budget that leaves roughly 30s for the checks before the review gets skipped and the claim ends unreviewed, which is the same kind of mislabel this PR is fixing. draftsIndependent()returns true whenused()is empty. That's fine today becausedraftCriteriareports synchronously, but requiringused.length > 0would make it fail closed.- The ACP isolation only changes the working folder.
projectViewstill putsWorking directory: <abs path>in every prompt, so an ACP agent with file tools could open the work folder by absolute path. Either soften theaskCwddoc comment ("cannot open" overstates it) or deny filesystem permission requests outsideaskCwd. - For the record: in the snapshot path, each stage's hash is computed over the snapshot's own fields. So the comparison catches a stage called without the snapshot, which is the regression that matters, but it doesn't cover the appended review/cover task text. That's fine as designed. It just might be worth one sentence in the doc comment so nobody reads it as covering the full prompt.
Nothing here blocks the merge.
| export function claimWaitForDraftsMs(timeLeftMs: number | undefined, budgetMs: number): number { | ||
| const floor = lateCriteriaWaitMs(timeLeftMs); | ||
| if (timeLeftMs === undefined || !budgetMs) return floor; | ||
| const margin = Math.max(60_000, Math.round(budgetMs * 0.15)); |
There was a problem hiding this comment.
After this wait, the bar checks still have to run, and then reviewSkipReason() wants ~60s (3 × 20s for a separate judge). At 600s budget the margin is 90s, so checks slower than ~30s push the review out and the claim ends unreviewed. Consider folding the review's need into the margin, e.g. max(60s, 15%) + reviewNeed, or reusing the same constant.
| * drafter stage so far used exactly those. | ||
| */ | ||
| const draftsIndependent = (): boolean => | ||
| opts.draftInputs !== undefined && opts.draftInputs.used().every((h) => h === draftInputs); |
There was a problem hiding this comment.
Nit: [].every(...) is true, so no stage having reported yet counts as independent. It's safe today because draftCriteria calls onInputs synchronously before its first await, but used.length > 0 && would make it fail closed if that ordering ever changes.
| * Where a subprocess (ACP) drafter is started. Kept apart from the work | ||
| * folder so the agent answering cannot open the worker's files. | ||
| */ | ||
| askCwd?: string; |
There was a problem hiding this comment.
The cwd change helps, but projectView still puts Working directory: <abs path> in the prompt, so an ACP agent with file tools could still read the work folder by absolute path. I'd soften "cannot open the worker's files" here, or deny ACP filesystem permission requests outside askCwd.
A claim of done waited a fixed 120 s for checks still being drafted. A separate judge drafting for 170-300 s (a local model past 6 min) finished after that, and correct work was judged without task checks and labelled plain unverified. Before waiting longer, the drafts had to be independent of the work, and they were not: every drafter stage re-read the work folder. A redraft or cover step running after the cut saw the worker's output (a task naming out.txt got "out.txt ... first line: 3" in its prompt), and the second draft attempt re-read scripts, commands and lessons live. - The drafter's inputs (project view, tree, commands, scripts, lessons) are frozen once before the first step and used by every stage: first draft, lint redraft, critic, cover step, redraft, and the second try. - drafterInputsHash covers that snapshot. Each stage reports the hash of what it actually put in its prompt; the join compares them with the turn-start hash and refuses drafts built from anything else. - A subprocess (ACP) drafter starts in an empty temp folder, not the work. - With frozen inputs and a time budget, a claim waits for late drafts until what is left less max(60 s, 15% of the budget), never less than before. No budget, or inputs that differ: the old wait. Only after a claim; the deadline without a claim is unchanged. - Late checks join the same seal and bar; a failing one refuses the claim. Journal and info lines say how long it waited and whether the drafts arrived.
…eout test On Node 22 AbortSignal.timeout's timer does not hold the event loop open and the mock stream has no socket, so the loop emptied before the 30 ms timeout and the runner cancelled the rest of local-ask.test.ts. That is why check was red on every PR in the stack, #23 included.
196e4e5 to
4118f29
Compare
|
Rebased onto the new The stack below #27 is now linear (#24 → #25 → #26 → #27 → …). This branch is the bottom of it, so it also carries one CI commit (4118f29):
|
1 participant
What
drafterSnapshot). Every drafter stage reads that snapshot: the first draft, the lint redraft, the critic, the cover step, the redraft after review, and theautoDraftsecond try.drafterInputsHashcovers the snapshot. Each stage reports the hash of what it actually put in its prompt (onInputs). The engine journals the snapshot hash at turn start, and at the join compares it with the hashes the stages reported. Drafts built from other inputs are not joined, and that is journalled (late-checks…NOT joined) and said in an info line.max(60 s, 15% of budget). It never waits less than the old rule (claimWaitForDraftsMs). With no budget, or with inputs that don't match, the old wait (at most 120 s) still applies.waited Nsand whether the drafts arrived (arrived,waitedMs).Why
Measured on 2026-10-07 across 6 bench lanes and 46 runs: the separate judge (Grok at about 170–300 s, a local NUC model past 6 min) was still drafting when the worker claimed done. After 120 s the claim was judged without task checks, so correct work was labelled plain "unverified".
Extending the wait was only safe once the drafts were independent of the work, and they were not.
draftCriteriacalledprojectViewagainst the live folder on every call, and the redraft and cover stages run after the cut, while the worker is already writing. With the task "Write the word count of input.txt to out.txt", the redraft prompt containedout.txt … first line: 3, so the worker's answer could have been sealed as the check. The old "inputs hash journalled twice" was also the same variable logged twice, so it could not fail.The deadline behaviour from 7abf0fa does not come back: the late wait only happens after a claim of done. A deadline reached without a claim is handled exactly as before.
How tested
npm run checkis green, with e2e and self-check run with~/.local/binremoved from PATH: 1950 tests, 0 failures, e2e PASS, self-check PASS.New tests in
test/drafter-view.test.ts:draftCriteriato the liveprojectView: 3 of the new tests fail.New tests in
test/late-criteria.test.tsrun the engine on a virtual clock (mockedsetTimeoutandDate):Expected numbers
With a 600 s budget, a claim at about 10 s now waits up to about 500 s, where it used to wait 120 s. In the 46-run bench, the runs whose judge drafted for 170–300 s should now carry task checks instead of plain "unverified". Drafts from a local model that needs more than about 8 minutes will still miss a 600 s budget. Interactive runs with no budget are unchanged.