review: real metering on salvage paths; gate lastText on a delivered result record - #377
Conversation
…result record From the merged #362's review fold: the catch-path salvages returned usd 0, turns 0 for sessions the proxy really metered, and the Stop hook makes the error_max_turns ending common for free-text agents, so dispatch's perAgent and totalUsd systematically undercounted, polluting exactly the live-week artifacts PRA-45 watches. Non-success result records still carry total_cost_usd/num_turns; the runner captures them before throwing. The lastText salvage now requires a delivered result record (a hard failure's narration burned the malformed-output re-dispatch), and the Stop hook's reason distinguishes never-called from bounced-and-must-correct. Riders from other merged-PR folds: the cross-file hard-negative fixture asserts its calibration band, the ack guard's dead isBotLogin clause is documented as belt-and-suspenders with the test spelling annotated, the consumer-bump skill stops calling the now-guarded --repo footgun silent, and the repo-root test uses the file's check helper.
🦋 Changeset detectedLatest commit: 9ca0d43 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 live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // mid-investigation narration, which would fail the contract | ||
| // parse and burn the malformed-output re-dispatch, exactly like | ||
| // the timeout case above. | ||
| if (ended && lastText !== undefined) { |
There was a problem hiding this comment.
note (non-blocking): Gating lastText on ended also removes the second attempt, because a thrown run is shed rather than re-dispatched. I traced the null path: a throw makes dispatchAgent return null, and dispatch.ts's finder loop hits if (output === null) { shedDimension(); continue; } before parseWithRetry, so the malformed-output path this change stops burning was also the only retry a hard mid-stream failure got. Introduced by this change; the timeout branch above already accepts the same trade, so this may be the intended symmetry rather than a defect.
review details
found by correctness-reviewer | also flagged by holistic: Gating lastText on `ended` removes the only retry for a transient mid-stream death.…through openThreadScore The blocking catch: the mid-bounce reason branch had no test, and while adding one the first-principles sibling proved the branch itself wrong: provisional only covers prose-gate bounces (a contract bounce returns before provisional is set), so a contract-bounced agent still heard 'you have not delivered'. The reason now branches on a submitAttempts counter incremented before the contract check, covering both bounce kinds, with tests for each plus the never-called case. The band assertion scores through a newly exported openThreadScore instead of re-deriving its formula and threadProse's strip by hand, and the changeset stops claiming zeros remain only without a result record (the rethrow path is a second, pre-existing case).
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (4 files)
Common patterns2 files: Salvage-path metering: hardcoded - usd: 0,
- turns: 0,
+ usd: endedUsd,
+ turns: endedTurns,2 files: - !isBotLogin(comment.author) &&
+ // Belt-and-suspenders only: staged threads carry
+ // GraphQL's bare logins, which never end in `[bot]`, so
+ // this clause fires only if staging ever switches to the
+ // REST spelling.
+ !isBotLogin(comment.author) &&review detailsreview-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation |
There was a problem hiding this comment.
5 of 6 prior review threads resolved; 1 still unaddressed as of 9ca0d43:
1 non-blocking thread still open
- note (non-blocking)
workflows/review/lib/dispatch-runner.ts:345: Gating lastText onendedalso removes the second attempt, because a thrown run is shed rather than re-dispatched. I tr...
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| }); | ||
| reason: | ||
| submitAttempts === 0 | ||
| ? "You have not delivered your result yet. Call the submit_result tool ONCE now, passing the ENTIRE JSON object your output contract specifies as its `result` argument; do not paste the JSON as a message." |
There was a problem hiding this comment.
suggestion (non-blocking): The never-delivered Stop-hook reason is the one branch no test pins. grep "have not delivered" workflows/review/lib/*.test.ts returns only the two negative assertions in the new bounce tests (dispatch-runner.test.ts:491 and :511); the only test of the zero-attempt branch (dispatch-runner.test.ts:464) asserts toContain("submit_result"), a substring both reason strings share. So collapsing the new submitAttempts === 0 ternary to always emit the "was rejected" arm keeps the whole suite green while telling an agent that never called the tool to correct a submission it never made — the mirror of the falsehood this PR removes. (Inverting the ternary would be caught by the two new tests; the collapse is the mutation that escapes.) One positive assertion in the existing test closes the loop.
A sketch, not a committable replacement:
const first = await hook!();
expect(first).toMatchObject({decision: "block"});
expect(String(first["reason"])).toContain("submit_result");
// The agent never called the tool: this branch, not the
// mid-bounce "your submission was rejected" one.
expect(String(first["reason"])).toContain("have not delivered");
expect(String(first["reason"])).not.toContain("was rejected");
review details
found by test-adequacy
2 participants
Follow-up to #362's final review fold (9 collapsed observations), plus riders from the other merged PRs' folds. The one that made this timely rather than just valid: the catch-path salvages returned
usd: 0, turns: 0for sessions the metered proxy really charged, and the runner's own comment says the Stop hook makes the error_max_turns ending common for free-text agents, so dispatch'sperAgententries and thetotalUsdsummed over them systematically undercounted. PRA-45's done-when is a week of live artifacts; an undercounted cost surface pollutes exactly that data.total_cost_usd/num_turns, so the runner captures them before throwing and every salvage path (captured, provisional, lastText) reports them. Zeros remain only when the stream dies with no result record at all, which is the genuinely unknowable case.Promise.resolveidiom for the async form the sibling handler uses and reflows the one 99-column comment line.Riders: the adjudicated cross-file hard-negative fixture now asserts its calibration band (>=6 shared bigrams, >=0.35 overlap, jaccard < 0.2) with the exported primitives instead of only documenting it, so a fixture drifting out of band fails instead of pinning nothing; the ack guard's never-firing
isBotLoginclause is documented as belt-and-suspenders (staged threads carry GraphQL's bare logins; the early return and sameLogin are the real guards) and the test's impossible spelling is annotated; the consumer-bump skill stops describing the--repofootgun as silent now that #372 made the checker fail loudly; and the repo-root test uses the file'scheckhelper.1934 tests green locally.
KORE-2512