fix(adjudicate): declare a verdict tool and stop forcing tool_choice:none - #137
Conversation
|
Reviewed against main Good news first, since the findings below are heavy: the sweep works when it fires. Forced 1. Blocking — the verdict tool is injected on every request of every preset, including ones that contain no sweep
The rationale at if h.sweepEnabled && provider == bschemas.Anthropic { body, _ = adjudicate.Inject(...) }The Cost note: because the injection happens after 2. Blocking — the mechanism did not reproduce live. 0 of 5 real asks used the declared tool.This is the PR's central claim, and two independent live attempts could not observe it. In every case the verdicts came back through the pre-existing prose parser this PR does not Caveats stated fairly: n=5 across two agents, and one used a synthetic transcript. But the PR's own Please land a paired A/B — same transcript and model, declared vs not, n≥10, one timeline — or 3. Blocking — the injected tool's
|
|
Thank you — this was a genuinely load-bearing review. Three blockers are fixed in The three-way A/BBoth prior measurements — the PR body's and yours — compared
Fisher two-tailed on unparseable: 1 vs 3 Your finding 2, refuted — and the counter that settles it43 of 77 replies (55.8%) came back as a I want to be clear that the methodological half of that finding was correct and is what produced Arm 2 is the finding, and it inverts the "drop the tool" optionRemoving So this PR's own headline was wrong, in the more interesting direction. The comment it deleted — Freed to call something and offered only the agent's own tools plus (a) and (b) are therefore one change, not two, and reducing this PR to (a) would ship a What reproduces, and what does not
The blockers1 — injected where it cannot be used. Fixed as you proposed: 3 — the
2 — measurability. Docs. Declined, with reasons
Benchmark traffic went to the plain gateway, not through Context Guru. Total cost $59.38. Not merging or closing — over to you. |
feb82a9 to
7f278bf
Compare
|
Rebased onto The rebase found a real defect in this PR, and main had just fixed the same bug twice.
Now sourced from And main's new Worth noting the limit the test itself concedes: a Verified on the rebased tree: |
OsherElhadad
left a comment
OsherElhadad
left a comment
There was a problem hiding this comment.
Reviewed, built, and ran locally against fake upstreams (no live provider spend). Summary: the core change is correct and well-gated, and it does not repeat #161's tool-leak mistake — context_guru_adjudicate is correctly absent from every non-sweep pipeline I tried, including live requests against the built binary. All statistical claims in the PR body reproduce exactly (recomputed the Fisher p-values from the raw counts: 0.0245 / 2e-06 / 0.0755, all matching). Six of the vacuity-check table's mutations plus one I added myself were applied by hand and reverted; every named test failed and came back green as claimed.
Two things I'd want before merging, then a handful of cheap cleanups.
Before merge
1. The "withheld on both wire paths, answered in band" claim has a real hole — the co-called-client-tool case.
proxy/proxy.go:1572:
if (len(calls) == 0 && len(strays) == 0) || otherTools {
bail() // hands `withheld` back to the clientWhen the model calls context_guru_adjudicate and a client tool in the same assistant turn, otherTools is true, the loop bails, and the adjudicate tool_use streams to the client raw. Reproduced live: one upstream round, client receives the block verbatim, no in-band answer that turn. AnswerStrayCalls correctly repairs it on the next turn (verified: "not found" replaced, is_error cleared, client's own tool_result left intact, counter incremented) — so fail-open holds and the cost is one agent turn, not a broken session. But this is a path the response loop does see and deliberately defers to the backstop, which is the opposite of what "answered in band on both paths, backstop only for a path the loop doesn't see" says. No test covers the co-call case, and given the PR's own precedent for what a leaked proxy tool_use costs an agent, it deserves one pinning the known behavior rather than leaving it to be "fixed" wrongly later. As a minor side effect, bail()'s RecordSSEExpandAfterStream() call conflates an adjudicate leak with an expand leak in that metric.
2. Commit 3's stray-counter fix ships with no regression guard.
Reverted proxy/promexport.go:439 to float64(s.AdjudicateStray) and dropped its notExportedWhy entry — the entire ./proxy suite stayed green (only the unrelated TestCtlGetCampaignAggregatesPredictedAndRealPerTenant flake failed, and that's pre-existing/unrelated — see below). The series would silently export a permanent 0 on every scrape with nothing complaining. The precedent this commit cites, TestExpandUnresolvedSeriesRender (proxy/promexport_coverage_test.go:152-178), asserts both that the line renders and that the value moves when the counter increments — the PR adopted the exemption half of that pattern but not the guard half, and the vacuity table lists no mutation for commit 3 at all, consistent with there being no test that would catch it. A ~10-line TestAdjudicateStraySeriesRender closes this.
Cleanups
3. The byte-stability premise is stated too strongly. proxy/proxy.go:1172-1173 and internal/adjudicate/tool.go:36-37 say pipeline membership is "fixed at config load," but proxy/tenancy.go:561-587 rebuilds a tenant's *Pipeline when the config document changes — mid-session — by its own design. The conclusion survives (anything that flips this gate already invalidates the prefix for much bigger reasons), but "fixed at config load" should read "fixed per config document" so the claim matches the code.
4. Two test comments still cite the numbers the PR body itself retracts. proxy/adjudicatetool_test.go:112 and proxy/prefixask_test.go:127-130 both still say "0 of 6 verdicts" / "6 of 6," which the body corrects to "main returns verdicts on 71.5% of the items it asks about." Worth fixing since these comments are what a future reader trusts.
5. internal/adjudicate/tool.go:176-190 — AnswerStrayCalls's doc comment runs straight into // ResponseCallIDs returns… with no blank line, so godoc attributes the whole 14-line block to the wrong function and AnswerStrayCalls ends up undocumented. One blank line fixes it.
6. proxy/ssepeek.go:179 — pass(body io.Reader, expandTool string) became pass(body io.Reader, proxyTools ...string). An empty variadic silently disables the whole leak defense (found=false) and still compiles, where the old signature made that impossible. A len(proxyTools) == 0 guard (or keeping one required name) would preserve the old compile-time safety.
7. Worth a line acknowledging: asks-attempted-per-request is 0.171 / 0.124 / 0.429 across the three arms — arm 3 attempts asks 2.5x more often per request than main. Doesn't invalidate the headline (the unparseable rate is a per-ask proportion and Fisher on proportions is still the right test), but something beyond the two intended changes differed between arms, and the PR disclaims cost comparability without disclaiming this.
Not this PR's problem
go test ./... has one failure, TestCtlGetCampaignAggregatesPredictedAndRealPerTenant (proxy/campaign_test.go:627) — not touched by this branch, passed 3/3 in isolation and on a full re-run. The branch is ~7 commits behind main; rebase will likely make this moot.
Fail-open holds throughout otherwise: adjudicate.Inject sits inside the existing recover() block and returns the original body on any trouble, AnswerStrayCalls is byte-identical when uninvolved, and the one response-corruption path (finding 1) degrades to the pre-existing expand behavior and self-repairs. One counter-accuracy nit: NoteAnsweredInBand (proxy.go:1585-1588) increments before expand.Continuation can fail, so a Continuation failure could double-count that stray alongside the request-path backstop — narrow, accuracy-only.
Solid, unusually well-tested change. Requesting changes for 1-2; 3-7 can ride along in the same push.
…ct both claims Responds to the second review on #137. Rebased onto main first, which moots the unrelated TestCtlGetCampaignAggregatesPredictedAndRealPerTenant failure the review saw: the full suite is green on the rebase alone, before any change here. FINDING 1 — the co-called-client-tool hole. When the model calls context_guru_adjudicate AND a client tool in the same assistant turn, otherTools is true, the response loop bail()s, and our tool_use streams to the client raw. AnswerStrayCalls repairs it on the next request, so fail-open holds and the price is one agent turn. Documented and PINNED, not changed: the loop cannot continue a turn whose other tool_use only the CLIENT can execute without inventing a result for the client's tool or dropping its call, and both are worse than one lost turn. - TestAdjudicateStrayCoCalledWithClientToolLeaks asserts what happens today: the leak this turn, the repair next turn, the client's own tool_result untouched, and the stray counted exactly once. - Comments corrected wherever they overclaimed. The old wording said the tool was answered in band with the backstop only for "a path this loop does not see" — false for this path, which the loop DOES see and defers deliberately. Fixed in proxy.go (both the response loop and the request-path repair), in internal/adjudicate/tool.go (ResponseCallIDs and AnswerStrayCalls), in docs/reference/routes.md (which claimed every stray "costs the agent nothing"), and in docs/components/extract_llm_sweep.md. FINDING 2 — the stray counter shipped with no regression guard. Commit 3 changed cg_adjudicate_stray_total to read adjudicate.StrayAnswered() because Snapshot.AdjudicateStray is filled by the /stats handler after renderMetrics takes its snapshot, so a promLine off `s` exports a permanent 0. The review reverted that and the whole ./proxy suite stayed green. TestAdjudicateStraySeriesRender adds the missing half of the TestExpandUnresolvedSeriesRender pattern: the line renders at zero AND the value moves when the counter does. Baseline-relative, because strayAnswered is process-wide and shared across the test binary. CLEANUPS 3. "fixed at config load" -> "fixed per config DOCUMENT" in proxy.go, internal/adjudicate/tool.go and docs/components/extract_llm_sweep.md. tenancy.go rebuilds a tenant's *Pipeline when the config document changes, mid-session. The conclusion survives; the premise now matches the code. 4. proxy/adjudicatetool_test.go and proxy/prefixask_test.go no longer cite the "0 of 6" / "6 of 6" verdict counts the PR body retracts. Replaced with the three-arm measurement (9.1% vs 30.0% unparseable, Fisher p = 0.0245, 55.8% of replies carrying a tool_use) and the note that main answers 71.5% of the items it asks about. The 8,378-vs-8,268 cache-key finding is un-retracted and kept. 5. AnswerStrayCalls's doc comment was sitting above ResponseCallIDs, so godoc attributed it to the wrong function. MOVED to its own declaration rather than just separated by a blank line, which the review suggested: a blank line alone would have left AnswerStrayCalls undocumented and the block floating. 6. sseSplicer.pass takes one required tool name plus a variadic tail. A bare `proxyTools ...string` compiled with NO names and silently disabled the entire leak defence (found stays false, every proxy tool_use streams through). ALSO - NoteAnsweredInBand moved after expand.Continuation succeeds. It previously incremented before Continuation could fail, so a failure bailed, the tool_use reached the client, and the request-path repair counted the same stray a second time. Now exactly one count per stray however it was answered. - bail()'s RecordSSEExpandAfterStream conflation is documented rather than split, with the reasons in place: the field is /stats-only and reaches no dashboard or alert, two of the three bail sites cannot attribute the leak because nothing has parsed the turn yet, and a leak can be both kinds at once — so the honest fix is two counters and a new exported family with its own render and vacuity guard, which is a metrics change rather than this PR's subject. VACUITY CHECK — every mutation asserted to have LANDED before running. promexport.go back to float64(s.AdjudicateStray), adjudicate import dropped, notExportedWhy entry KEPT: TestAdjudicateStraySeriesRender FAILS — still reads "cg_adjudicate_stray_total 0" after two answered strays — the series is not reading adjudicate.StrayAnswered() (Snapshot.AdjudicateStray is filled only by /stats, so a promLine off `s` exports a permanent 0) Same, plus the notExportedWhy entry dropped (the review's exact revert): TestAdjudicateStraySeriesRender FAILS, and is the ONLY failure in ./proxy — where before this test the entire suite stayed green under that revert. The otherTools deferral removed from the response loop's bail condition: TestAdjudicateStrayCoCalledWithClientToolLeaks FAILS — expected the loop to bail on otherTools after ONE round, got 4 — the co-call path no longer defers, so this test's premise is gone AnswerStrayCalls neutered to `return body, 0`: TestAdjudicateStrayCoCalledWithClientToolLeaks FAILS on all four repair assertions — refusal forwarded unchanged, no substitute answer, is_error still set, and "the leaked stray was counted 0 times, want exactly 1". sp.pass called with an empty withhold set: compile error, "not enough arguments in call to sp.pass" — no longer representable, which is the property the two-argument signature used to give. The double-count fix carries NO test: reaching it needs expand.Continuation to fail on a response that already parsed as containing a tool_use, and there is no fixture seam for that at the handler boundary. Stated rather than papered over. gofmt clean, go build ./... clean, go vet ./... clean, full go test ./... green (Go 1.26.4, CGO_ENABLED=1). No benchmarks and no live provider spend. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
7f278bf to
55f2a62
Compare
|
Thanks — the two before-merge items were both right, and finding 2 in particular caught a real gap in my own vacuity discipline. All seven are addressed in one push ( Step 0 — rebaseRebased onto 1 — the co-called-client-tool holePinned, and the comments corrected. Behaviour unchanged, which I think is right and not just conservative: answering in band means continuing the turn upstream, and the loop cannot continue a turn whose other
The 2 — the stray-counter guardYou were right and the diagnosis was exactly right: I took the
Verified against your exact revert, both ways:
The failure message under the revert: 3-73. "fixed at config load" → "fixed per config document", in 4. Both comments fixed. Replaced with the three-arm numbers — 9.1% unparseable (7 of 77) against 30.0% (6 of 20), Fisher p = 0.0245, 55.8% of replies carrying a 5. One correction to the ask: a blank line alone would not have fixed this. The comment block sits above 6. Restored as a required name plus a variadic tail — 7. Added to the body, with the arithmetic: 20/117, 29/234, 78/182 = 0.171 / 0.124 / 0.429, arm 3 attempting an ask 2.5× more often per request. It says the headline survives (per-replied-ask proportion, Fisher on proportions is right regardless of ask volume) and that the divergence is nonetheless real and unexplained by the diff — likeliest trajectory drift changing how often the sweep's pre-expiry trigger fires, which is upstream of anything here. Also added the missing commit-3 row to the vacuity table, and the four new mutations. Closing nit —
|
| mutation (each asserted to have landed first) | test | result |
|---|---|---|
promexport.go → float64(s.AdjudicateStray), import dropped, exemption kept |
TestAdjudicateStraySeriesRender |
FAILS |
same + notExportedWhy entry dropped (your exact revert) |
TestAdjudicateStraySeriesRender |
FAILS, and the only failure in ./proxy |
otherTools deferral removed from the bail condition |
TestAdjudicateStrayCoCalledWithClientToolLeaks |
FAILS — "expected the loop to bail on otherTools after ONE round, got 4" |
AnswerStrayCalls neutered to return body, 0 |
TestAdjudicateStrayCoCalledWithClientToolLeaks |
FAILS on all four repair assertions, incl. "counted 0 times, want exactly 1" |
sp.pass called with an empty withhold set |
compile-time | not enough arguments in call to sp.pass |
The 4-round result on the third row is worth noting on its own: without the deferral a co-called turn doesn't just answer in band, it spins the loop to maxExpandRounds — the deferral is load-bearing, not incidental.
gofmt clean, go build ./... clean, go vet ./... clean, full go test ./... green on Go 1.26.4 with CGO_ENABLED=1. No benchmarks, no live provider spend — fake upstreams throughout, as you did.
Not merging; over to you.
…none
CompletePrefixed set tool_choice to {"type":"none"} on every prefix ask, with a comment
claiming that was "free (not in the cache key) and required, or the model answers with a
tool_use". Both halves were wrong, and the measurement is the same prefix with only
tool_choice varying:
tool_choice reply shape cache verdicts returned
{"type":"none"} prose / thinking only read 8,268 (free) 0 of 6
{"type":"tool",name} tool_use MISS, wrote 8,378 6 of 6
(omitted) tool_use read 8,268 (free) 6 of 6, on 4 of 4 trials
So `none` is what DROVE the model into prose. A sampled reply reasoned correctly under the
criterion and simply said so in sentences ("the task is not yet complete, and no summary of
this raw data has been recorded elsewhere") -- which the contract itself calls a valid
answer -- and the caller then scored it as an unparseable failure. Forcing a NAMED tool is
not free either: it wrote a second cache entry, so tool_choice does participate in the key
when it names a tool even though "none" does not. Merely DECLARING a structured-answer tool,
with no tool_choice at all, gets a schema-shaped answer for the whole batch at cache-read
price.
internal/adjudicate declares that tool. Its verdict labels are small INTEGERS, never opaque
tool_use ids: asked for ids the model regularised them (toolu_01..07 for toolu_probe_00..07),
because reproducing a random identifier from thousands of tokens back is a copying task
rather than a judgement; with integers it was 0 bad labels across 40+ trials. Its field names
are extract.Verdict's own JSON tags, so the existing parser reads a tool input unchanged.
The tool is injected on EVERY request, next to expand.Inject, not only when the sweep is
about to ask: `tools` hashes before system and messages, so a tool that appears on the turn a
sweep fires and disappears on the next invalidates the prefix from position zero -- the flap
expand's `always` mode exists to prevent.
Stray calls the AGENT makes are answered on the request path, mirroring
expand.RepairToolResults. The client cannot execute a tool the proxy injected, so it answers
"not found" and the agent loses a turn to a dead end. Not defensive: a model was directly
observed calling context_guru_expand at step 2 of a run. Measured at 0 strays across ~4,900
requests with the "do not call this yourself" description, so /stats publishes
adjudicate_stray -- that counter is the only thing that can say the description stopped
working.
Additive, deliberately: extract.ParseVerdicts and extract.BuildFallbackAsk are untouched and
a model that answers in prose anyway is read exactly as before. This changes which reply
shape is PREFERRED, not which ones are accepted.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…w answers arrive
Addresses the review's three blockers. The measurement it asked for was run, and it
changed what this PR claims: the tool is NOT inert, and removing tool_choice:none
WITHOUT it is worse than main.
Both prior measurements compared main against (a)+(b) together, so neither could tell
which half worked. Same transcript, same ask model (aws/claude-sonnet-5, LOCA S2L,
cg_128k_3), three passes per arm, run sequentially, passes interleaved by arm so any
drift over the run lands on all three equally.
| arm | build | asks replied | timed out | unusable | via tool_use | coverage |
|---|---|---|---|---|---|---|
| 1 | origin/main | 20 | 0 | 6 (30.0%) | 0 | 71.5% |
| 2 | (a) alone: tool_choice removed, NO tool | 24 | 5 | 14 (58.3%) | 0 | 41.2% |
| 3 | this PR, (a)+(b) | 77 | 1 | 7 (9.1%) | 43 (55.8%) | 90.9% |
Fisher two-tailed: 1 vs 3 p = 0.0245, 2 vs 3 p = 0.0000, 1 vs 2 p = 0.0755.
Two claims are refuted and one is confirmed:
- The review's "0 of 5 asks used the declared tool" does NOT hold at n=77: 43 replies
(55.8%) came back as a tool_use. n=5 was the artefact. The counter added here is what
makes this observable at all.
- This PR's own "tool_choice:none is what produced prose, and both halves of the old
comment were wrong" is ALSO wrong, in the more interesting direction. Arm 2 is the
worst of the three. The old comment ("or the model answers with a tool_use instead of
the verdicts") was RIGHT about the mechanism and wrong only about the remedy: freed to
call a tool and offered only the agent's own plus context_guru_expand, the model calls
one of those. Logging every reply's content blocks caught it directly, 5 of 20 replies
in a dedicated pass, as `thinking,tool_use:context_guru_expand` with NO text block --
which the text-only extraction reads as "" and files as unusable.
- main's rate (30.0%) and this branch's (9.1%) reproduce the PR body's 26.1% -> 7.3%,
and coverage 71.5% -> 90.9% reproduces 74.2% -> 92.6%.
So (a) and (b) are one change, not two, and the PR is NOT reducible to (a).
The PR body's timeout claim does not reproduce: main lost 0 asks to the 90 s
llmCallTimeout here, not 9. The only arm that lost asks that way is (a)-alone, at 5.
Now gated on `provider == Anthropic && tn.Pipe.Has("extract_llm_sweep")`. Neither
condition varies per turn -- pipeline membership is fixed at config load, the provider by
the route -- so the prefix stays byte-stable and the cache-flap argument is satisfied;
that argument only ever forbade gating on something per-turn. Injecting unconditionally
cost a measured 946 bytes at the head of the cacheable prefix of every preset, including
`off`, the control arm of every published comparison in this repo.
TestAdjudicateToolAdvertisedOnEveryTurn was asserting exactly that defect (it built with
`pipeline: []`); it is rewritten to keep the every-turn property on a pipeline that can
actually adjudicate.
The SSE splicer took one tool name; it now takes the withhold SET, and proxy.go passes
both proxy-injected tools. `advertised` covers both, or a request advertising only the
adjudication tool was never inspected. expand.ResponseCalls gained a variadic list of
proxy-owned names so a second one is not misclassified as a CLIENT tool -- that
misclassification was what made the loop bail and hand the call over. Stray calls are now
answered IN BAND on the response path, before the client is written to;
adjudicate.AnswerStrayCalls stays as the backstop it was described as.
components.PrefixUsage.ViaTool, plus sweep_answered_via_tool / _via_prose. Without this a
working sweep and a silently-prose-answering one are identical in every counter, because
extract.ParseVerdicts reads a tool_use input and a JSON array in text the same way. This
is the only reason the two conflicting readings above could be settled.
- docs/components/extract_llm_sweep.md stated the INVERSE of what is measured. Replaced
with the three-arm table, the mechanism, and a section on where the tool is injected.
- docs/reference/routes.md gains the `adjudicate_stray` row, and the counter is now
exported as cg_adjudicate_stray_total so /metrics matches what routes.md:14 promises.
Every new test was re-run with its subject reverted and had to FAIL. All 7 mutations did,
and each mutation was asserted to have landed before running:
| mutation | test | result |
|---|---|---|
| gate removed, inject unconditionally | ...NotAdvertisedWhenThePipelineCannotAdjudicate | FAILS |
| gate removed, inject unconditionally | ...NotAdvertisedOnANonAnthropicRoute | FAILS |
| splicer withholds expand only | ...DoesNotReachTheClientOnTheSSEPath | FAILS |
| in-band answering removed | ...DoesNotReachTheClientOnTheJSONPath | FAILS |
| our tool counted as a CLIENT tool again | ...DoesNotReachTheClient (both) | FAILS |
| sweep stops recording the reply shape | TestSweepCountsWhetherTheAnswerCame... | FAILS |
| fallback IS attributed a reply shape | TestSweepDoesNotAttributeAReplyShape... | FAILS |
One gap the mutation run itself exposed and closed: dropping `u.ViaTool = true` from
CompletePrefixed broke nothing, because no test asserted the wiring. Both cheapmodel
prefix-ask tests now assert the reported shape, in each direction.
The SSE leak test initially failed for the wrong reason -- its fixture answered a
stream:true request with JSON on round 2, a documented anomaly path that cannot splice and
so bails and hands the withheld events back. The fixture now streams both rounds, and says
why.
`gofmt` clean, `go build ./...` and the full `go test ./...` clean on the eval box
(Go 1.26.4, CGO_ENABLED=1).
Not folded in, recorded as follow-ups: the empty-reply failure mode (a thinking block and
no answer, present in every arm) and the fabricated-quote rate, which moved in the
OPPOSITE direction to the PR body's report here (main 24.4% of verdicts against this
branch's 8.4%, versus the 15.8% -> 24.9% recorded earlier) and so is workload-dependent
rather than a regression this change causes.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…apshot Rebasing onto main surfaced that main had just fixed, twice, the exact bug this change had introduced a third instance of. cg_adjudicate_stray_total read float64(s.AdjudicateStray). But renderMetrics holds the bare aggregator snapshot, and Snapshot.AdjudicateStray is filled only by the /stats handler afterwards, so that promLine would have exported a hard-wired 0 on every scrape however often the agent called the tool. Main documents the class in promexport.go while fixing cg_expand_unresolved_total and cg_frozen_decisions_total, the latter having shipped with the defect. Sourced now from adjudicate.StrayAnswered(), as main sources those. Main's new TestEverySnapshotFieldIsExportedOrExempt caught it, and its header names this PR as expected-to-fail. Resolved the way its second group prescribes rather than by weakening the test: the field is listed in notExportedWhy with the series it feeds and where the value comes from. Worth noting the test cannot catch this class on its own -- a promLine off `s` passes reflection whatever the value -- which is why the exemption records the source rather than merely asserting an export exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…ct both claims Responds to the second review on #137. Rebased onto main first, which moots the unrelated TestCtlGetCampaignAggregatesPredictedAndRealPerTenant failure the review saw: the full suite is green on the rebase alone, before any change here. FINDING 1 — the co-called-client-tool hole. When the model calls context_guru_adjudicate AND a client tool in the same assistant turn, otherTools is true, the response loop bail()s, and our tool_use streams to the client raw. AnswerStrayCalls repairs it on the next request, so fail-open holds and the price is one agent turn. Documented and PINNED, not changed: the loop cannot continue a turn whose other tool_use only the CLIENT can execute without inventing a result for the client's tool or dropping its call, and both are worse than one lost turn. - TestAdjudicateStrayCoCalledWithClientToolLeaks asserts what happens today: the leak this turn, the repair next turn, the client's own tool_result untouched, and the stray counted exactly once. - Comments corrected wherever they overclaimed. The old wording said the tool was answered in band with the backstop only for "a path this loop does not see" — false for this path, which the loop DOES see and defers deliberately. Fixed in proxy.go (both the response loop and the request-path repair), in internal/adjudicate/tool.go (ResponseCallIDs and AnswerStrayCalls), in docs/reference/routes.md (which claimed every stray "costs the agent nothing"), and in docs/components/extract_llm_sweep.md. FINDING 2 — the stray counter shipped with no regression guard. Commit 3 changed cg_adjudicate_stray_total to read adjudicate.StrayAnswered() because Snapshot.AdjudicateStray is filled by the /stats handler after renderMetrics takes its snapshot, so a promLine off `s` exports a permanent 0. The review reverted that and the whole ./proxy suite stayed green. TestAdjudicateStraySeriesRender adds the missing half of the TestExpandUnresolvedSeriesRender pattern: the line renders at zero AND the value moves when the counter does. Baseline-relative, because strayAnswered is process-wide and shared across the test binary. CLEANUPS 3. "fixed at config load" -> "fixed per config DOCUMENT" in proxy.go, internal/adjudicate/tool.go and docs/components/extract_llm_sweep.md. tenancy.go rebuilds a tenant's *Pipeline when the config document changes, mid-session. The conclusion survives; the premise now matches the code. 4. proxy/adjudicatetool_test.go and proxy/prefixask_test.go no longer cite the "0 of 6" / "6 of 6" verdict counts the PR body retracts. Replaced with the three-arm measurement (9.1% vs 30.0% unparseable, Fisher p = 0.0245, 55.8% of replies carrying a tool_use) and the note that main answers 71.5% of the items it asks about. The 8,378-vs-8,268 cache-key finding is un-retracted and kept. 5. AnswerStrayCalls's doc comment was sitting above ResponseCallIDs, so godoc attributed it to the wrong function. MOVED to its own declaration rather than just separated by a blank line, which the review suggested: a blank line alone would have left AnswerStrayCalls undocumented and the block floating. 6. sseSplicer.pass takes one required tool name plus a variadic tail. A bare `proxyTools ...string` compiled with NO names and silently disabled the entire leak defence (found stays false, every proxy tool_use streams through). ALSO - NoteAnsweredInBand moved after expand.Continuation succeeds. It previously incremented before Continuation could fail, so a failure bailed, the tool_use reached the client, and the request-path repair counted the same stray a second time. Now exactly one count per stray however it was answered. - bail()'s RecordSSEExpandAfterStream conflation is documented rather than split, with the reasons in place: the field is /stats-only and reaches no dashboard or alert, two of the three bail sites cannot attribute the leak because nothing has parsed the turn yet, and a leak can be both kinds at once — so the honest fix is two counters and a new exported family with its own render and vacuity guard, which is a metrics change rather than this PR's subject. VACUITY CHECK — every mutation asserted to have LANDED before running. promexport.go back to float64(s.AdjudicateStray), adjudicate import dropped, notExportedWhy entry KEPT: TestAdjudicateStraySeriesRender FAILS — still reads "cg_adjudicate_stray_total 0" after two answered strays — the series is not reading adjudicate.StrayAnswered() (Snapshot.AdjudicateStray is filled only by /stats, so a promLine off `s` exports a permanent 0) Same, plus the notExportedWhy entry dropped (the review's exact revert): TestAdjudicateStraySeriesRender FAILS, and is the ONLY failure in ./proxy — where before this test the entire suite stayed green under that revert. The otherTools deferral removed from the response loop's bail condition: TestAdjudicateStrayCoCalledWithClientToolLeaks FAILS — expected the loop to bail on otherTools after ONE round, got 4 — the co-call path no longer defers, so this test's premise is gone AnswerStrayCalls neutered to `return body, 0`: TestAdjudicateStrayCoCalledWithClientToolLeaks FAILS on all four repair assertions — refusal forwarded unchanged, no substitute answer, is_error still set, and "the leaked stray was counted 0 times, want exactly 1". sp.pass called with an empty withhold set: compile error, "not enough arguments in call to sp.pass" — no longer representable, which is the property the two-argument signature used to give. The double-count fix carries NO test: reaching it needs expand.Continuation to fail on a response that already parsed as containing a tool_use, and there is no fixture seam for that at the handler boundary. Stated rather than papered over. gofmt clean, go build ./... clean, go vet ./... clean, full go test ./... green (Go 1.26.4, CGO_ENABLED=1). No benchmarks and no live provider spend. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
55f2a62 to
fc25a85
Compare
|
Rebased onto The conflict was with #161, which is the sibling of this PR's own fix. Main gated Resolution detail worth recording, since two commits touched the same lines: for each conflicted hunk I Re-verified on the rebased tree: No functional change from the review response. This is the rebase and the conflict resolution only. |
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
…atency claim does not 45 runs, three arms, 64k, and the first iteration in this series with a clean error column: 0 errored runs and 0 HTML 400s in every arm. That matters because errored runs score 0 in ITT and had been the dominant noise term. THE MECHANISM GATE PASSED, and that is this iteration's real product. The pre-flight had both econ counters at 0/0 -- the trigger was never reached. After amendment 1 it was evaluated 30 times and repaid 30 times, with 0 declines, 30 real adjudications, 0 unparseable replies, and an index record for every candidate. coref acted 391 times, so min_batch_frac clears at 64k. The offline feasibility estimate (3-4x margin) was not merely directionally right. THE LATENCY CLAIM DOES NOT REPLICATE, and this is the endpoint iteration 021 could not produce. Turns per run, split by outcome: arm B is 18.9 against A's 20.7 overall, but among SOLVED runs it is 20.1 -> 19.8, a 1.5% difference inside noise, while among UNSOLVED runs it is 21.1 -> 18.5. B also solved fewer tasks. So the shape of B's efficiency is giving up sooner on tasks it fails, not solving faster. Iteration 021's -28% requests was read as a latency win; its own text attributed it to "fewer runaway sessions" and it could not distinguish the two. Measured with the split it lacked, on a run with ZERO errors, the gain is on the failure path. On wall clock the direction is worse rather than absent: B adds 1,396 ms per request against A's 203 ms -- about 1.2 s on every request -- to buy 1.5% fewer turns on the runs that matter, at 10x its own model spend. Arm C went the other way: +26% turns and 62% more LOCA cost. REWARD IS NULL BOTH WAYS and the pre-registered harm gate blocks both arms: B is -2 solves (p = 0.6250, bound 48.1%), C is +1 (p = 1.0000, bound 31.9%), against a 25% blocking threshold declared before the run. The structural fact this exposes is more useful than either p-value: at n=15 with one seed the best achievable bound is 21.8%, with ZERO worsened pairs. This design can essentially never license a positive claim, which is the argument for five seeds rather than more tasks. One defect filed rather than footnoted: expand_unresolved_missing = 60 in Cp1 against 0 everywhere else -- 60 of that pass's 112 expand calls unresolvable, in the pass that was also arm C's worst. An unresolvable expand is the one failure the reversibility invariant exists to prevent, and the correlation is unexplained. Also recorded, both firsts: extract_llm_sweep as SHIPPED never fires under continuous load (its only trigger needs an idle gap, and 8 workers never leave one), so arm A's sweep is inert and B-A is "a sweep that runs" versus "a sweep that cannot"; and sweep_answered_via_tool is 0 against via_prose 30 -- the verdict tool #137 added is offered every time and never used. The index row for iterations 016-021 is still missing from experiments/README.md; that gap predates this commit and is not addressed here. Signed-off-by: David Amid <david.amid@il.ibm.com> Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
3 participants
The defect, restated after measurement
internal/cheapmodel.CompletePrefixedsettool_choiceto{"type":"none"}on every prefix ask,with this comment:
An earlier version of this PR said both halves of that were wrong. That was itself wrong, and a
three-way A/B run for the review shows how. The second half was right about the mechanism and wrong
only about the remedy. The fix is not to suppress the tool call — it is to declare a tool worth
calling. The two changes below are therefore one change, and this PR is not reducible to either
half.
The three-way A/B
Prior measurements compared
mainagainst both changes together, so neither could separate them.The missing arm is
tool_choiceremoved with NO tool declared. Same transcript, same ask model(
aws/claude-sonnet-5, LOCA S2Lcg_128k_3), same config, three passes per arm, run sequentially,passes interleaved by arm so drift over the run lands on all three equally.
origin/mainsweep_unparseablesweep_reply_truncatedtool_usesweep_prefix_cache_read_ZEROadjudicate_strayFisher two-tailed on unparseable: 1 vs 3
p = 0.0245, 2 vs 3p = 0.0000, 1 vs 2p = 0.0755.Arm 2 is the result. Removing
tool_choice: noneon its own is worse thanmain, and it is theonly arm that loses asks to
llmCallTimeout. The mechanism was measured directly, not inferred, bylogging every reply's content blocks on a dedicated pass with
tool_choiceremoved and no verdicttool declared:
The ask appends to a prefix carrying the agent's own tools — they are in the cache key, so stripping
them costs the read. Freed to call something and offered only those plus
context_guru_expand, themodel calls one of them.
CompletePrefixedreturns the first non-emptytext, so those replies readas
""and are filed unusable.Separately: forcing a named tool is not free either — it wrote a second cache entry (8,378 against
the 8,268 already cached), so
tool_choicedoes participate in the key when it names a tool, eventhough
nonedoes not. Omitting it entirely reads the prefix for free.What this changes
internal/adjudicate(new) declarescontext_guru_adjudicate. Verdict labels are smallintegers, never opaque
tool_useids: asked for ids the model regularised them(
toolu_01..07fortoolu_probe_00..07), because reproducing a random identifier from thousands oftokens back is a copying task rather than a judgement; with integers it was 0 bad labels across 40+
trials. The schema's field names are
extract.Verdict's own JSON tags, so the existing parser readsa tool input unchanged.
extract_llm_sweep, andnowhere else. Every-request matters because
toolshashes beforesystemandmessages, so a toolthat appears when the sweep fires and vanishes next turn invalidates the prefix from position zero —
the flap
expand'salwaysmode exists to prevent. But that argument only forbids gating onsomething per-turn: pipeline membership is fixed at config load and the provider by the route, so
both are byte-stable for a session. Injecting without those two conditions cost a measured 946
bytes at the head of the cacheable prefix of every preset — including
off, the control arm ofevery published comparison in this repo, and including presets with no sweep at all. Also skipped
under a forcing
tool_choice, on a request with no tools, in observe mode, and on a bypassedrequest — the same gates
expand.Injectrespects.CompletePrefixedno longer setstool_choiceand prefers our tool'sinputover text. Onlyour tool by name: the prefix carries the agent's own tools, and returning a
Readcall's argumentswould replace a usable prose answer with something that cannot parse.
tool_useis withheld from the client on both wire paths, and answered in band. The SSEsplicer takes a withhold set rather than one name;
advertisedcovers both proxy-injected tools,or a request advertising only this one was never inspected;
expand.ResponseCallstakes theproxy-owned names so ours is not classified as a CLIENT tool, which is what made the loop
bail()and hand the call over.
adjudicate.AnswerStrayCallsremains as the backstop — for two distinctpaths, and an earlier draft of this bullet named only the second. (a) A turn that calls this tool
alongside a client tool:
otherToolsis true, the response loopbail()s, and ourtool_usereaches the client raw. That is a deliberate deferral, not a gap — the loop cannot continue a turn whose
other
tool_useonly the client can execute without inventing a result for the client's tool ordropping its call, and both are worse than one lost turn. The next request's repair fixes it: substitute
answer in,
is_errorcleared, the client's owntool_resultuntouched, stray counted once. Cost is oneagent turn, not a broken session, and it degrades to exactly the pre-existing expand behaviour. Pinned
by
TestAdjudicateStrayCoCalledWithClientToolLeaks, which asserts the leak rather than wishing it away.(b) A round the loop genuinely cannot see: SSE aggregation failed, or
maxExpandRoundsis spent.sweep_answered_via_tool/_via_prose. Without these, a working sweep and a silentlyprose-answering one are identical in every counter, because
extract.ParseVerdictsreads atool_useinput and a JSON array in text the same way. This ambiguity is what left two independentlive measurements — "0 of 5 asks used the tool" and "6 of 6" — unable to be judged against each
other. Both were consistent with everything published at the time.
/statspublishesadjudicate_stray, now also exported ascg_adjudicate_stray_totalso/metricsmatches whatroutes.md:14promises. Measured 0 across all nine benchmark passes.extract.ParseVerdictsandextract.BuildFallbackAskare untouched, and a model thatanswers in prose anyway is read exactly as before. This changes which reply shape is preferred, not
which are accepted — 44.2% of arm 3's replies still arrived as prose and parsed fine.
Docs
docs/components/extract_llm_sweep.mdstated the inverse of what is measured abouttool_choice: none; replaced with the three-arm table, the measured mechanism, and a section on wherethe tool is injected and why both gate conditions are byte-stable.
docs/reference/routes.mdgains themissing
adjudicate_strayrow.Corrections to this PR's own earlier numbers
about. The defect is that ~30% of its asks come back unusable, not that all do.
9 asks to the 90 s
llmCallTimeoutagainst this branch's 0. Here main lost 0. The only arm thatloses asks that way is
tool_choice-removed-without-the-tool, at 5.dominant residual mode in every arm is a reply with no text block. Filed as extract_llm_sweep: empty and wrong-tool prefix-ask replies are both filed as sweep_unparseable #164.
sweep_reply_truncatednever fired — 0 of 121 replied asks across all three arms.sweep_prefix_cache_read_ZEROwas 0 in all nine passes.different amounts of task work (117 / 234 / 182 requests), so no cost claim is made.
changes. 20/117, 29/234, 78/182 = 0.171 / 0.124 / 0.429 — arm 3 attempted an ask 2.5x more often
per request than main. The headline survives:
sweep_unparseableis a proportion per replied ask,the arms are compared on that proportion, and Fisher on proportions is the right test regardless of how
many asks each arm made. But the divergence is real and unexplained by the diff — the likeliest cause is
trajectory drift changing how often the sweep's pre-expiry trigger fires, which is upstream of anything
this PR touches. Recorded because the body already disclaims cost comparability and this deserves the
same treatment: something beyond the two intended changes differed between arms.
Vacuity check
Standing repo rule: each new test re-run with the code it covers reverted or mutated, and required to
FAIL. Each mutation was also asserted to have landed before running, since a
sedthat silentlyfails to match produces a clean "ok" that looks like a pass.
Injectloses the idempotency guardTestInjectIsByteStableAndIdempotentInjectno longer appends lastTestInjectIsByteStableAndIdempotentInjectdrops the forcing-tool_choice/no-tools guardsTestInjectRefusesWhenItWouldPerturbSelectionAnswerStrayCallsno longer restricts to our own toolTestAnswerStrayCalls*AnswerStrayCallsstops countingTestAnswerStrayCallsLeavesRealResultsAloneAnswerStrayCallsleavesis_errorsetTestAnswerStrayCallsLeavesRealResultsAloneHasToolreads the wrong dialect fieldTestHasToolTestAdjudicateToolNotAdvertisedOnAnAgentCompactionTestAdjudicateStrayCallIsAnsweredOnTheRequestPathtool_choice:noneput back (the original defect)TestCompletePrefixedAppendsWithoutDisturbingThePrefixtool_usepreference removedTestCompletePrefixedPrefersOurToolInputOverTexttool_usepreferred, including the agent's ownTestCompletePrefixedPrefersOurToolInputOverTextadjudicate_strayremoved from/statsTestStatsShapeIsUnchangedTestAdjudicateToolNotAdvertisedWhenThePipelineCannotAdjudicateTestAdjudicateToolNotAdvertisedOnANonAnthropicRouteTestAdjudicateStrayCallDoesNotReachTheClientOnTheSSEPathTestAdjudicateStrayCallDoesNotReachTheClientOnTheJSONPathTestAdjudicateStrayCallDoesNotReachTheClient(both)TestSweepCountsWhetherTheAnswerCameViaTheToolOrProseTestSweepDoesNotAttributeAReplyShapeToTheFallbackCompletePrefixedstops settingViaToolTestCompletePrefixedPrefersOurToolInputOverTextpromexport.goback tofloat64(s.AdjudicateStray),adjudicateimport dropped, exemption KEPTTestAdjudicateStraySeriesRendernotExportedWhyentry dropped (the reviewer's exact revert)TestAdjudicateStraySeriesRender./proxy, where before this test the whole suite stayed greenotherToolsdeferral removed from the response loop's bailTestAdjudicateStrayCoCalledWithClientToolLeaksmaxExpandRounds, 4 rounds, instead of 1)AnswerStrayCallsneutered toreturn body, 0TestAdjudicateStrayCoCalledWithClientToolLeaksis_errorstill set, counted 0 not 1)pass(body)called with an empty withhold setnot enough arguments in call to sp.pass— no longer representableFour honest notes on that table:
The row for commit 3 was missing entirely, and that was the tell. The table shipped with no
mutation for the stray-counter fix because no test would have caught it: this PR took the
notExportedWhyexemption half of theTestExpandUnresolvedSeriesRenderpattern and skipped theguard half.
TestAdjudicateStraySeriesRenderadds it — the line renders at zero and the valuemoves when
adjudicate.StrayAnswered()does, baseline-relative because the counter is process-wide andshared across the test binary.
TestAdjudicateToolAdvertisedOnEveryTurnwas asserting the defect — it built withpipeline: []and passed because the tool reached a pipeline that could never adjudicate. Rewritten to keep the
every-turn property on a sweep-bearing Anthropic route.
The last row is a gap the mutation run itself exposed: dropping
u.ViaTool = trueoriginally brokenothing, because no test asserted the wiring. Both cheapmodel prefix-ask tests now assert the
reported shape, in each direction.
TestCompletePrefixedStillReadsTextWhenNoToolWasCalledis a deliberate control, not a vacuityfailure: it guards the RETAINED text path, so it passes with the fix removed. That is the point of it.
The SSE leak test initially failed for the wrong reason — its fixture answered a
stream: truerequestwith JSON on round 2, a documented anomaly path that cannot splice and therefore bails and hands the
withheld events back. The fixture now streams both rounds and says why.
gofmtclean,go build ./...and the fullgo test ./...clean on the eval box (Go 1.26.4,CGO_ENABLED=1).Run setup, and the one knob not left at its default
cg_128k_3.json— 3 LOCA S2L tasks,aws/claude-sonnet-5, 128k window,--max-workers 8, three passes per arm, pooled.pre_expiry_seconds: 7200, against a default of 60.sweeping()fires only while0 < (CacheTTLMs − IdleMs) <= pre_expiry. A benchmark agent turning every few seconds keepsIdleMsin single digits, so the default 60 s window never opens. This measures the parse rate of asks,
deliberately not how often the window opens in production. Worth flagging on its own: at the
shipped default this component is close to inert on an actively-turning agent.
block_fallback: true.fallbackAsk()callsmodel.Complete(), notCompletePrefixed(), so itcannot differ between arms and would only dilute them.
min_tokens: 400(default 1000), a sample-size fix only:maxAskItemscaps every ask at 12 itemsregardless, so reply length and truncation rate are unchanged.
Follow-ups, deliberately not folded in
sweep_unparseable. Thedominant residual failure mode, present in
mainat the same rate.sweep_quote_fabricatedswings 3x between runs and inverted between arms(main 24.4% against this branch's 8.4%, where an earlier run recorded 15.8% → 24.9%). It cannot
currently gate a PR in either direction, which is why no claim is made from it here.