fix: tell a refused tool call what to do next, not just what went wrong - #1885
Conversation
f48dc1b to
5bc7bd0
Compare
|
Pushed fixes for findings 7 through 10, and acted on 11. One sub-point of finding 10 does not hold and I left that code alone; details below. Finding 7, confirmed and it is the most serious of the five. Verified the gate: activateGoal and mutateGoal return undefined for five distinct conditions, and only one of them is a race. A turn that was never registered declines permanently, which covers every turn started with goalBoundary 'none' — the graph supervisor at boot.ts:917 and both paths at session-execution-ipc-main.ts, where beginObservedTurn never runs — and every turn whose session was closed and removed. The loop is exactly as described and the PR's own test file reproduces it: in "GoalSet after permanent removal" the session is gone, GoalSet says call GoalStatus and retry only if there is still no active goal, GoalStatus says "No goal set for this session", the condition holds, the model retries, identical refusal, no exit. The old opaque text asked for nothing, so this was a regression. I split the message by cause rather than only removing the retry. The coordinator now exposes activationStanding and mutationStanding, and the two mutators are implemented on top of them, so there is one copy of the predicate rather than a second that can drift. GoalStatus is prescribed where reading can change what happens next — a real concurrent change, and the case where this turn already armed a goal, since there GoalStatus tells the model something it does not know. For the causes that are settled facts about the turn the sentence says the turn cannot do this and asks the model to say so instead. One thing turned up while writing it. The reason I had first called turn_superseded cannot mean that: the lane is looked up out of the map and then checked for currency, so the check reduces to "the coordinator is disposed". It is named coordinator_disposed now and says that. Negative control: collapsing the five causes back into one retry message turns fifteen assertions red, including both real-coordinator scenarios, which fail with the exact loop text quoted above. Finding 8, confirmed. That branch fires when the worker returns a well-formed result of a different operation; real failures throw, as every error path in filesystem-worker/client.ts does. So a worker that writes the file and labels the result 'glob' produced "nothing was written to disk", or for Edit "the file is unchanged, a different old_string will not help", about a file nothing in that path looked at. It is two helpers now. A write says Maka cannot tell whether the change landed, that the file is in an unknown state, and to read it before writing again. A read says no result came back. Finding 9, confirmed, and the fix is in the read helper above. "Grep could not be completed inside Maka, so no matches were produced" is a successful empty search as far as a skimming model is concerned. It now says no search result came back and that this does not mean the pattern is absent. Glob at the other site had the same shape — less confusable, as you said, but the same sentence — and is fixed with it. Negative control: restoring either wording turns the three E3 tests red. Finding 10, confirmed in part. The prescription is the real problem: reservedShellRuns and reservedPtyRuns are manager-wide while StopBackgroundTask takes a session-scoped ref, so the session that hits the cap may own none of the runs holding it. The message now offers that move as a condition, says the limit is shared across sessions, and names waiting, which is always available. The second half of the finding does not hold. The printed number is maxLiveShellRuns, but the throw fires at reserved >= max and reserved is only incremented after that check passes, so reserved can never exceed max and the two are equal at the moment the message is built. maxLiveShellRuns is also readonly and constructor-set. The number is accurate, so I left it. Negative control on the wording: reverting to the flat "Stop one with StopBackgroundTask" turns the shell-run-manager capacity test red. Finding 11: confirmed unreachable. tool-runtime.ts injects askUserQuestion and requestSandboxBoundary unconditionally, with no spread guard, unlike listChildAgents and readChildAgentOutput a few lines above. I did not make the injection conditional, because there is nothing to condition it on — both are methods on ToolRuntime itself and are always present, and availability is in fact decided when the tool set is assembled, which is why the CLI adds both only on the TUI surface. The guards stay, since MakaToolContext declares the callbacks optional and an embedder can build one without them, but the two tests now say in their names and a comment that they cover that type contract and not a production path. Test delta, against a baseline taken on clean origin/main on this machine. Node here has no node:sqlite, so @maka/runtime already carries 64 failing test names. After the change: 2754 tests, and the only new failures are the three E5 cases in this PR's own test file, all of them "No such built-in module: node:sqlite" from createSqliteShellRunStore. They will run in CI. A handful of Bash and workspace-executor timeouts also moved around between runs; load average was above 35 on this box and all of them pass 79/79 when run in isolation. biome check clean on the nine touched files, check-console passes. Also rebased onto current main. |
|
Second-round review addressed. Five findings fixed, one rejected with evidence, one partly reframed. The prescribed fallback is gone.
GoalResume is finding 3, and it survived contact — for a wider reason than the report gives. The test stub could not see any of this, and now can. The fake answered Next door: One rejection. "Goal authority is unavailable." is not transient. Its only producer is The worker-protocol violation still reaches an operator: it travels as the Error Two things the body claims that the diff no longer does, both now corrected there: the Edit/Write row said "the file is unchanged", which the third commit deliberately replaced with "cannot tell whether" — that branch fires on a mislabelled success, so the disk state is unknown — and the verification line counted a partial run. Negative controls were run, not asserted. Reverting each production change in turn and rebuilding produced, verbatim: Test delta: |
170e744 to
39777bf
Compare
|
Third-round review findings addressed. Rebased onto current main. Finding 1, the shell-slot refusal naming a tool the receiving agent does not The assertion is at the seam that decides it: the test asks the real builder Finding 2, goal_changed inviting a retry that is permanent within the turn. goal_already_armed is the same shape from GoalSet. It is reachable only once Finding 3, the merge conflict with #1935. Resolved toward upstream. The branch On the non-blocking items. The agent_swarm resume advice is a real defect, not a defensive corner. The dead branches are documented rather than removed, and one of them turned Information loss for operators is restored the way builtin-tools.ts does it. The unguarded sentence now has a guard. "This is an internal failure, not a Every new assertion was negative-controlled by reverting the production change, |
A refusal that names only its cause is a refusal a model re-sends. That was measured, not assumed: across recorded runs, refusals with no next move came back as identical retries, and refusals that named an internal noun the model has no verb for came back as retries of the same call with different arguments. Every sentence changed here answers "so what do I do now": - `agent_spawn` / `agent_list` / `agent_output` said the capability "is unavailable in this runtime context". A model cannot make a runtime context available. They now say retrying will fail the same way and to do the task with the tools already in hand. - The four Goal refusals said "this turn no longer owns Goal activation". Ownership is a host state machine label. They now name `GoalStatus` as the call that shows what the goal actually is. - `agent_output` locator rejections said "its matching identity fields", leaving the model to guess which of four optional id fields to add. They now name the fields for that locator. - `Edit` and `Write` reported an internal filesystem-identity mismatch through the argument-error path, so it read as a complaint about `old_string`. They now say the file is unchanged, that the arguments are not the problem, and what to do. - A malformed background-task ref got its own text echoed back. It now gets the canonical form. - A full shell or PTY slot did not say what frees one. It names `StopBackgroundTask`. The test walks each refusal and asserts on the sentence a model would read. Three of its cases need `node:sqlite` and are skipped on runtimes without it.
CI caught nine tests pinned to wording this change replaces. Every one of them asserted the half of the old sentence that had to go — the host noun the model could not act on — so they had to move rather than be relaxed. They now pin what the new sentence is for: - the six Goal declines assert `Call GoalStatus`, the call that shows what the goal actually is, instead of `no longer owns Goal activation` - `agent_swarm` asserts it names the tool and what to do instead, rather than `spawnChildSession capability is unavailable` - the two capacity refusals assert they name `StopBackgroundTask`, which is the thing that frees a slot and which the old wording never mentioned Relaxing the patterns instead would have left tests that pass whatever the refusal says, which is how a sentence stops being anybody's contract.
…annot Review found that three of these rewrites traded an opaque sentence for a confident wrong one. A refusal is read by a model, so the cost of a claim it cannot support is a decision made on a false picture. The four Goal declines are the worst of it. They asserted one cause — "the session goal changed while this turn was running" — and prescribed a retry. But the gate returns undefined for five reasons and only one of them is a race. A turn that was never registered under a Goal boundary declines permanently: every turn started with goalBoundary 'none', which is the graph supervisor and both session-execution IPC paths, and every turn whose session was closed and removed. The PR's own test file demonstrates the loop. In "GoalSet after permanent removal" the session is gone, so GoalSet says call GoalStatus and retry "only if there is still no active goal", GoalStatus says "No goal set for this session", the condition holds, the model retries, and it receives the identical refusal. Forever. The old text was opaque but it asked for nothing. So the coordinator now says why. `activationStanding` and `mutationStanding` hold the same predicates the two mutators use — one source of truth, not a second copy that can drift — and each names a cause. GoalStatus is prescribed where reading can change what happens next, and where it cannot the sentence says so and asks the model to report instead. One of the five turned out to be misnamed while I was writing it: the lane is looked up from the map before it is checked for currency, so that branch can only fire when the coordinator is disposed, and it is called that now rather than pretending a later turn took over. `internalFilesystemFailure` asserted disk state it never looked at. That branch fires when the worker returns a well-formed result of the wrong operation; real failures throw. A worker that writes the file and then labels the result 'glob' produced "nothing was written to disk", or for Edit "the file is unchanged … a different old_string will not help", and the model went on from there. It is now two helpers. A write says Maka cannot tell whether the change landed and sends the model to read the file. A read says no result came back — and says what that does not mean, because "Grep could not be completed inside Maka, so no matches were produced" reads as a search that ran and found nothing, and a model that takes it that way concludes the pattern is absent from the repository. Glob had the same shape and is fixed with it. The capacity message promised a move that can be unavailable: the reserved-run counters are manager-wide while StopBackgroundTask takes a session-scoped ref, so with several sessions live the one that hits the cap may own none of the runs holding it. It now offers that move as a condition and names waiting, which always works. The review also flagged the printed number as the cap rather than the reserved count; it is both — the throw fires at `reserved >= max` and reserved is only ever incremented after that check passes, so the two are equal here. Left as is. Not changed, but no longer covered silently: the AskUserQuestion and request_sandbox_boundary guards cannot fire in production. ToolRuntime injects both callbacks unconditionally, unlike listChildAgents and readChildAgentOutput beside them, which are spread only when present. Availability is decided when the tool set is assembled — the CLI adds both only on the TUI surface. The guards stay because the context type declares the callbacks optional and an embedder may build one without them, and the tests now say that is the contract they cover rather than implying a reachable path. Every new assertion was negative-controlled. Collapsing the Goal causes back into one retry message turns fifteen of them red, including the two real- coordinator scenarios that reproduce the loop.
Five of the sentences this branch rewrote still asserted or prescribed
something the code cannot back.
The filesystem-worker refusals ended "use Bash to do the same work". A
local_read child is given Read, Glob and Grep and nothing else, so the
caller most likely to be running a bare Grep was handed an instruction it
cannot carry out. The shell is now offered on a condition the model can
check for itself, and the sentence ends on a move every caller has. The
worker-protocol violation still reaches an operator: it travels as the
Error cause, out of the model's sight and in every stack.
coordinator_disposed could not be produced. dispose() sets the flag and
clears the lane map in one step, so the lane lookup above the isCurrent
check already failed and the turn was told it "does not run under the
session goal boundary" — false about a turn that had armed the goal it
was trying to clear. Shutdown is now asked about first, and the carefully
worded shutdown sentence can be shown.
GoalResume goes through the activation gate, so it could be told it had
"already armed a goal" and "cannot arm a second one" in reply to a call
that asked to arm nothing. The advice now knows which tool it answers.
Nor is "already armed" the right noun: beginObservedTurn binds the lease
by itself when a goal is already active, and that turn armed nothing.
"Goal authority is unavailable." reads as a passing condition. Its only
producer is beginDrain, which is one-way, so it now says the door does
not reopen.
request_sandbox_boundary was rewritten on a path ToolRuntime cannot
reach, because ToolRuntime injects the callback unconditionally; the
sentence a model actually gets was left alone. Both now share one
constant. The retry branch in agent_swarm and the ref message on the
Read({ref}) path, which is where a ref is most likely mistyped, get the
same treatment.
Tests: the fake coordinator answered both authorization gates with the
same reason, so a tool wired to the wrong gate stayed green. The two
gates now answer separately, and every decline cause is produced by the
real coordinator.
Three of the sentences this branch rewrote still handed the model a move
it could not make, and the tests could not tell, because they call
tool.impl with a hand-built context: nothing there knows which agent
receives the message, or whether a state survives the turn.
The shell-slot refusals hedged on the wrong thing. "Stop one of yours
with StopBackgroundTask if you started any" hedges on owning a run; the
gap is owning the tool. Built with the real buildChildAgentTools over a
parent set carrying the shell tools, a child agent receives Read, Glob,
Grep, WebSearch, Write, Edit, Bash and ArchiveRead — Bash yes,
StopBackgroundTask and WriteStdin no, because
buildToolsForAgentDefinition is a strict name allowlist. So the caller
that most often reaches the manager-wide cap is precisely the one that
would go looking for a schema entry it does not have. Both sentences now
describe the move instead of naming a tool, and the PTY one leads with
dropping `pty`, which is a parameter every caller already has. The test
asks the real builder which names a child never receives and asserts the
refusal contains none of them, so the next sentence that reaches for one
turns it red.
goal_changed invited a retry that cannot succeed. registration.
observedControlLease is written once at beginObservedTurn and refreshed
only by a mutation that succeeds, so once the lease has moved the turn
declines the same way until it ends. Driven against the real coordinator
— one turn registers, a second arms a goal and clears it — GoalSet
declines goal_changed on the first call and on the fourth, and no call
available to that turn changes the answer. The GoalSet arm of
goal_already_armed is the same shape: reachable only once the armed goal
has left `active`, and by then the controlLease cannot be released,
because releasing it needs a mutation the moved lease also declines.
Both now end by closing the retry. GoalStatus is still offered, because
reading is still worth doing; it is the last step rather than a step
back to the call that just failed.
The agent_swarm resume refusal prescribed spawning. ToolRuntime's
buildChildAgentContext returns {} wholesale when getCurrentRunId() is
falsy, so spawn and resume leave together, and "drop resume_run_ids and
send that work as new items instead" walks the model into the spawn
refusal, which tells it retrying fails the same way. The advice now asks
whether spawning is available before prescribing it — an embedder can
supply one without the other, so it is a real question — and the test
follows the advice rather than reading it.
Also restores what an operator lost. The rejected background-task ref,
and the three capability names the agent-swarm and subagent refusals
stopped mentioning, travel as the Error cause the way builtin-tools.ts
already carries its worker-protocol violations: out of the transcript,
in every stack. And "This is an internal failure, not a problem with
your arguments" could have been deleted from all five filesystem
messages without turning a test red; now it cannot.
Rebased onto main. The tool-runtime.ts conflict is apache#1935's hosted-run
routing against this branch's message: upstream's !hostedRun guard is
kept, so hosted runs still route to the host instead of throwing, and
only the message changes. The comment there now records that this guard,
unlike the one in sandbox-boundary-tool.ts, does fire in production —
the CLI supplies the sandbox store callbacks only on the tui surface.
The two branches that are genuinely unreachable through ToolRuntime say
so where they are.
39777bf to
489fc92
Compare
1 participant
The measurement
A refusal that names only its cause is a refusal a model re-sends. Across recorded runs:
What changed
agent_spawn/agent_list/agent_output/agent_swarmagent_outputlocatorRead/Glob/Grepon an internal fs mismatchEdit/Writeon an internal fs mismatchold_stringStopBackgroundTask, and says the limit is sharedrequest_sandbox_boundaryA model cannot make a runtime context available, and it has no verb for owning a turn. Those sentences were describing host state machines to something that can only make tool calls.
What a refusal may not do
Two rules the second pass enforces, because the first pass broke both.
It may not prescribe a move the caller does not have.
local_readchildren get Read, Glob and Grep and nothing else, so a Grep failure ending "use Bash to do the same work" is a dead end dressed as a way out. The shell is offered on a condition the model can check for itself, and the sentence ends on something every caller can do.It may not assert what the code cannot observe. The filesystem branch fires on a mislabelled success, so it does not know whether the write landed and does not say. Only
goal_changeddescribes a race, so onlygoal_changedasks the model to look again; the other four causes say the turn cannot do this and to move on. The worker-protocol violation still reaches an operator, as the Errorcause.Reachability
Three of the five Goal decline causes had no test that the real coordinator can produce them, and one of the three could not:
coordinator_disposedsat below a lane lookup thatdispose()had already made fail, so the shutdown sentence was unreachable and the turn read a sentence that was false about it. Every cause is now produced by a realGoalContinuationCoordinator, and the two authorization gates answer separately in the stub so a tool wired to the wrong one fails.Not included on purpose
automation-tools.tshas the same defect — "not found, not owned, or not active" collapses three independent causes into one — but its authority interface went async and session-scoped upstream since this was written. Porting it is a rewrite rather than a hunk, so it comes separately.tool-runtime.ts:776writes the Computer Use approval summary into the model-facingtool_callrecord on the AI SDK path. Same family of defect, different surface, and it feeds the durable args hash as well; it needs its own change.