fix: gate tool execution on raw stream completion, not just JSON validity - #3434
fix: gate tool execution on raw stream completion, not just JSON validity#3434canblmz1 wants to merge 1 commit into
Conversation
927e6a2 to
43e9a63
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving the tool-execution decision onto raw stream evidence and for adding a broad settlement matrix. The final safety boundary still has two fail-open/production-readiness issues and one duplicated payload authority. I left the required final state inline; the core simplification is that every streamed call must execute only the identity and value that the guard actually proved.\n\nAI-assisted review disclosure: Codex delegated independent Runtime and test reviews; I verified the exact-head control flow, dependency declaration, and live PR state before posting.
43e9a63 to
452ff41
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the defect you found is real and it is the good kind of finding: settleModelStepOutcome classifying finishReason: "length" as completed is correct for its own purpose (continuation and retry bookkeeping) and wrong as an execution gate, and nothing in the type system was ever going to tell anyone that those two questions had been conflated. A mutating call reaching ToolRuntime because its arguments happened to be syntactically complete, while the generation that produced it was cut off mid-thought, is exactly the failure worth closing.
The second half of the argument is the stronger one, and I want to say so explicitly because it is easy to miss: tool-call.input is post-processed data that repairToolCall may have coerced, whereas the raw tool-input-delta bytes are evidence about what the provider actually streamed. Verifying against the bytes rather than the SDK's conclusion is the right authority.
The handling of atomic delivery is the part I went in expecting to find a hole in and did not. Treating "no raw evidence for this id" as safe-by-omission would have been the obvious mistake; instead the absence is only allowed to mean "genuinely atomic" when hadRawArgumentEvidence is false for the whole request, because once any call in the request streamed real bytes, another call's missing decision is indistinguishable from an id mismatch. That distinction is subtle and it is drawn correctly.
The tests are proportionate to what they protect. 25 behavioural cases asserting execution counts — zero times / exactly once — across a safety matrix and a red-team set, including abort mid-stream, a missing terminal event, and two concurrent runs sharing toolCallId: "call_1". These fail if the gate regresses, rather than restating it.
So: no P0, and nothing wrong with the mechanism.
My one blocking concern is not about the code at all — it is about the dependency, and I do not think it can be settled inside this PR.
This adds [email protected] and makes it the authority that decides whether tool calls with real side effects — filesystem writes, shell commands, apply_patch, SQL, dependency installs, by your own list — are allowed to execute. Per THIRD_PARTY_NOTICES.txt, that package's repository is github.com/canblmz1/prefix-safe-json, which is your own account.
I want to be clear about what I am and am not saying. I am not suggesting anything improper, the license is clean (MIT OR Apache-2.0, Apache-2.0 selected), pinning the exact version rather than a range is the right call, and writing the hard part as a reusable library is a defensible engineering decision. What I am saying is that an Apache project taking a security-critical runtime dependency on a pre-1.0 package owned by an individual outside the project's control is a decision the project has to make deliberately, and right now it is a single line in a bugfix PR whose description does not mention it. Someone reviewing the Problem/Fix sections would not learn that this happened.
The questions I would want answered before this lands, none of which I can answer for you:
- What happens to this gate if the package is unmaintained, unpublished, or its npm publish rights are compromised? The blast radius is "tool calls execute when they should not", which is the thing this PR exists to prevent.
- Is donating the code to the project — vendoring it under
packages/, with the same tests — on the table? The consumed surface here is one factory and its verdicts; the completeness parser is the substance. That would keep the design and remove the external ownership question entirely. - Has an ASF-side dependency review been done? I do not know this project's threshold for new runtime dependencies, and that genuinely is a question for a maintainer rather than for me.
If the answer is "vendor it", nothing about your design has to change, which is why I think this is worth raising rather than working around.
On CI: test and package are red on this head, and neither is your fault. Both fail on pi-tui-runner.ts missing midTurn, a file this PR does not touch. This run was created at 11:41:11Z; the commit that added midTurn: 'local' landed on main at 11:49:56Z. A pull_request run tests the merge commit computed at event time, so this one predates the fix, and a re-run replays the same SHA. A rebase onto current main should clear it — please do not go looking for something to fix there.
Review assisted by AI (Claude Opus 5). Findings were verified against the files, the lockfile, the third-party notices and the workflow-run timestamps at this head; the reviewer is accountable for them.
| "image-dimensions": "^2.5.1", | ||
| "linkedom": "^0.18.13", | ||
| "node-pty": "^1.2.0-beta.15", | ||
| "prefix-safe-json": "0.1.1", |
There was a problem hiding this comment.
[P1] See the review body for the full reasoning; anchoring it here because this one line is the whole of it.
[email protected] becomes the authority deciding whether side-effecting tool calls execute, and THIRD_PARTY_NOTICES.txt records its repository as github.com/canblmz1/prefix-safe-json — the same account as this PR's author.
Not an objection to the package or to you. The license is clean and the exact-version pin is right. The objection is that an ASF project acquiring a security-critical dependency owned by an individual outside the project should be an explicit, disclosed decision, and this PR's description does not mention that it adds a dependency at all.
Vendoring it into packages/ with its tests would preserve every design decision in this PR and remove the question. That is the outcome I would suggest, but the call belongs to a maintainer, not to me.
It also pulls ajv@^8.20.0 transitively, which is worth noting for the same review.
| */ | ||
| import { createAiSdkExecutionGuard, type AiSdkExecutionGuard } from 'prefix-safe-json'; | ||
| import type { | ||
| ModelStepOutcome, |
There was a problem hiding this comment.
Not a finding — the opposite.
This file's header comment is the best piece of writing I have read in this repository today. It states why the module exists, names the specific function whose classification it is deliberately not reusing and why that function is right for its own purpose, documents the atomic-delivery case with the concrete provider that exhibits it, explains why absence of evidence is only allowed to mean "atomic" at whole-request scope, and closes the concurrency question about colliding toolCallIds before anyone has to ask it.
Most comments explain what the code does. This one explains what a reader would otherwise get wrong. Please keep it intact through any rework.
a9da82b to
671468f
Compare
671468f to
39214a7
Compare
Problem
Maka deliberately keeps tool execution outside the Vercel AI SDK and settles
returned tool calls through its own
ToolRuntimeafter each provider step.Before this change, the final execution gate relied on the provider step being
classified as
completed.However,
settleModelStepOutcome()also classifiesfinishReason: "length"as completed.That means a mutating tool call can have syntactically complete arguments,
reach Maka's returned-tool settlement path, and still belong to a provider
generation that was cut off by a token limit.
A complete tool-call payload is not, by itself, proof that the surrounding
provider step terminated safely.
There is a second integrity boundary as well: the AI SDK's final
tool-callinput is already parsed/post-processed data. Where providers expose raw
tool-input-deltachunks, those raw bytes are stronger evidence of whether thearguments were actually streamed to structural completion.
Fix
Add a narrow tool-execution safety layer around each physical provider request.
For incrementally streamed tool arguments:
tool-input-start,tool-input-delta,tool-input-end, and terminalstream events;
[email protected]'screateAiSdkExecutionGuard()to derive a per-tool-call execution verdict;ToolRuntime.For providers that deliver tool arguments atomically and expose no raw argument
deltas, no raw-JSON completeness claim is made.
Those calls instead require the provider step itself to finish with an
explicitly execution-safe reason:
stoptool-callsOther terminal states, including
length, are not execution-safe.This intentionally leaves
settleModelStepOutcome()unchanged because itslength -> completedbehavior has broader continuation/bookkeeping semantics.The stricter rule exists only at the irreversible tool-execution boundary.
Guard-proved execution authority
The guard's verdict is not reduced to a boolean gate. For a call whose raw
bytes were observed, the tracked decision carries the guard's own proved tool
name and parsed value, not just an execute/retry/reject action.
For an incrementally streamed call:
toolCall.toolName;ToolRuntimeis the guard-proved value, decoded fromthat call's own raw bytes -- never the SDK-projected
toolCall.input, whicha later repair/coercion step could have altered after the raw bytes were
already proved complete;
toolCallId (case-insensitively, since existing repair logic legitimately
corrects a mis-cased name), the call fails closed rather than executing
under either name.
The SDK-projected
toolCall.inputis only ever used as a fallback for callsthe guard has no raw-byte evidence for at all (the atomic-delivery case
below), where no guard-proved value exists to begin with.
Atomic provider delivery
Some real provider paths do not stream argument bytes incrementally. They may
emit:
with the actual parsed arguments appearing only in the final
tool-callevent.
The integration does not fabricate raw-byte evidence for that case.
"No raw delta evidence" means exactly that: raw argument completeness is
unknown. It does not mean the call is safe.
Execution is therefore allowed only when the provider step also has a
positively safe terminal reason.
Request-level raw evidence
The atomic-delivery fallback above is scoped to the entire physical provider
request, not evaluated call-by-call.
If a request contains zero raw tool-argument evidence anywhere, the fallback
applies as described: an execution-safe terminal reason is sufficient for a
call the guard has no raw bytes for.
If a request contains raw evidence for any call, that request is no longer
eligible for the fallback. Every other call in it must have its own explicit,
matching guard decision:
back to the step's terminal reason -- a missing decision in a request that
already proved it can stream raw bytes is indistinguishable from an id
mismatch between the raw stream and the SDK's resolved tool call;
delivered call does not let the atomic call inherit the fallback merely
because its own id has no decision;
existing, unchanged fallback behavior.
Scope
This change is intentionally limited to the existing tool-settlement path.
Unchanged:
ToolRuntime.settleToolCall()settleModelStepOutcome()Rejected tool calls continue through Maka's existing settlement/result
mechanism rather than introducing a new placeholder transcript state.
Concurrency
The execution guard is scoped to one physical provider request.
There is no process-global state keyed only by
toolCallId, so concurrentprovider requests may safely reuse identifiers such as
call_1withoutcross-resolving each other's decisions.
Production-path coverage includes concurrent calls sharing the same tool-call
id with different safety outcomes.
Dependency
Adds the exact dependency:
This is the published stable release, not a prerelease -- the version is
pinned exactly rather than using a prerelease or caret range. The public API
this integration depends on (
createAiSdkExecutionGuard()) has not changedacross any version used on this branch.
The package is used only where raw AI SDK stream evidence is available and does
not replace Maka's existing tool/runtime architecture.
Upstream is dual-licensed
MIT OR Apache-2.0; that declaration is unchanged.Maka selects the
Apache-2.0option through the existing third-party-licenseresolution mechanism (
LICENSE_SELECTIONSinscripts/generate-third-party-notices.mjs), following the same patternalready used for two other dual-licensed dependencies with an SPDX
ORdeclaration. No entry was added to the global license allowlist -- both
options in the declared expression were already independently approved.
check:third-party-noticesandcheck:cli-third-party-noticesboth passagainst the regenerated notices.
Tests
Added focused guard tests and end-to-end production-path tests through:
Covered cases include:
stop-> executestool-calls-> executeslength-> withheldstop-> executestool-calls-> executeslength-> withheldobject is delivered to
ToolRuntime, never the divergent oneunder either name
Validation on the final branch:
tool-call-execution-guard.test.ts,length-cutoff-tool-execution-repro.test.ts,ai-sdk-backend.test.ts): 257/257 passingnpm run lint: cleannpm run format:check: cleancheck:third-party-notices/check:cli-third-party-notices: pass@maka/storagemodule-resolution errors observed on pristine upstream, unchanged by this
patch (0 errors in any file this PR touches)
check:releasewas not completed locally -- the local checkout used tovalidate this branch does not have the monorepo's other packages built
(
dist/missing for@maka/storage,@maka/runtime,@maka/ui,@maka/desktop), which its first step (check:stale) requires. This is nota claim that the full release check passed.
GitHub's own CI has not run on the current head: all three checks (CI,
Release Windows check, Dependency audit) are
action_required, pending amaintainer approving workflow execution for this fork PR. This is not a test
failure.