Skip to content

fix(cli-tools): defer install execution until step completion is confirmed - #676

Open
canblmz1 wants to merge 4 commits into
op7418:mainfrom
canblmz1:fix/cli-install-execution-integrity
Open

fix(cli-tools): defer install execution until step completion is confirmed#676
canblmz1 wants to merge 4 commits into
op7418:mainfrom
canblmz1:fix/cli-install-execution-integrity

Conversation

@canblmz1

@canblmz1canblmz1 commented Aug 21, 2026

Copy link
Copy Markdown

Problem

codepilot_cli_tools_install executes execAsync(command, ...) as soon as the
AI SDK resolves that individual tool call.

At that point, the tool itself has no visibility into whether the surrounding
generation later terminates safely. A syntactically complete tool call can still
belong to a step that ends because of a token limit, provider error, or another
unsafe terminal condition.

The existing permission system does not currently carry stream-completion
state, so it cannot make an execution-integrity decision based on whether the
surrounding generation terminated safely.

Reproduction

src/__tests__/unit/execution-guard.test.ts covers a
codepilot_cli_tools_install call whose own arguments are complete, followed by
a step ending with finishReason: "length".

The contrast case demonstrates the previous execution pattern: once
execute() is entered, the real command runs without any knowledge of the
step's final termination state.

Fix

This patch uses
prefix-safe-json
(now the published 0.4.3 release — see "Dependency update" below) and its
createAiSdkExecutionGuard() API to gate the real side effect using the raw
fullStream parts already consumed in agent-loop.ts.

The real install command cannot wait inside the tool's execute() for the
stream's terminal state, because the stream cannot finish until in-flight tool
executions resolve.

Instead:

  1. codepilot_cli_tools_install.execute() registers the real install closure
    and immediately returns a queued result.
  2. agent-loop.ts feeds the step's raw fullStream events into the execution
    guard.
  3. Once the stream is fully consumed, each pending execution is resolved
    against the guard decision for its toolCallId.
  4. The real execAsync side effect runs only when the surrounding tool call is
    positively confirmed safe to execute.

The scope is intentionally narrow: only
codepilot_cli_tools_install, the directly shell-executing tool traced in this
patch, is converted. Other execute()-based tools may benefit from the same
pattern, but are intentionally out of scope here.

Safety semantics

A real install command executes only when the surrounding streamed tool call is
positively confirmed complete.

Truncated, errored, unknown, or otherwise unconfirmed terminal states discard
the pending side effect instead of executing it.

Follow-up fixes (turn isolation + model-history correctness)

Two issues found in review of the first commit, addressed in a second commit
on this branch:

Deferred state is isolated per Native turn, not by toolCallId alone.
toolCallId is provider-generated per call and not guaranteed unique across
concurrent turns — two overlapping turns can both produce e.g. "call_1"
(the same identity-isolation concern runtime/native-turn-registry.ts
already handles for abort controllers, for the same underlying reason). A
fresh executionScopeId is created once per runAgentLoop invocation and
threaded through the existing tool-assembly chain
(agent-loop.tsassembleToolsgetBuiltinTools
createCliToolsTools), so pending registrations are keyed by
scope + toolCallId and two turns can never resolve or discard each other's
pending commands even if their toolCallIds collide.

Abort/error cleanup discards pending closures fail-closed. Every step
already resolves its own scope's registrations; a new
discardPendingExecutions(scopeId) is additionally called from
agent-loop.ts's teardown finally block (which runs on every exit path —
success, abort, timeout, thrown error), so a turn that never reaches its own
step-level resolution can never leave a queued shell command available for a
later, unrelated turn to accidentally resolve. Any registration left
unmatched by a guard decision at step-resolution time is discarded the same
way, not carried forward.

The real deferred result now replaces the temporary "Queued…" placeholder
in the next model-step transcript, not just the UI.
The AI SDK bakes
execute()'s immediate "Queued…" return value into responseData.messages
as that call's tool-result — which is exactly what gets appended to the
conversation for the next step. Previously only a follow-up SSE event told
the UI the real outcome; the model's own history permanently kept
"Queued…" as the result. applyResolvedExecutionsToMessages() now replaces
that placeholder with the real outcome (the actual result text for an
executed call, or an explicit "Execution skipped: …" message — never a
claimed success — for a rejected one) before the messages are appended,
using the repository's existing ModelMessage tool-result shape from
tool-history-integrity.ts. Only the matching toolCallId's part is
touched; every other message and part is unchanged. The UI SSE and the
model transcript now share the exact same outcome text, so they can never
disagree about what happened.

Scope is unchanged from the first commit: only Native Runtime's
codepilot_cli_tools_install is affected, and cli-tools-mcp.ts (the SDK
Runtime's independent implementation) is not touched.

Dependency update: prefix-safe-json 0.4.3 (security release)

prefix-safe-json moved 0.0.1-alpha.40.2.0 → now the published exact
0.4.3 (package.json updated to "0.4.3"; package-lock.json regenerated
with npm install prefix-safe-json@0.4.3 --save-exact, not hand-edited).

0.4.3 is a security release (GHSA-3xpw-9694-2xxp) fixing three root causes in
the AI SDK adapter/gate this integration depends on:

  1. AiSdkStreamAdapter.push() used to silently drop every raw event once it
    had already observed its own terminal, so late/contradictory evidence for
    a call never reached the coordinator at all.
  2. takeDecision() used to read a decision snapshot frozen at finish()
    time instead of the coordinator's live diagnostics, so evidence recorded
    after finish() but before that call's authority was consumed was never
    consulted.
  3. A raw event carrying conflicting id/toolCallId used to silently
    prefer id instead of failing the stream closed.

No public API change: createAiSdkExecutionGuard() / push() / finish() /
takeDecision(), and the ExecuteDecision/NonExecutableDecision fields
this integration reads (action, toolCallId, reason), are unchanged —
confirmed directly against the installed package's .d.ts, not assumed.
0.4.3 still declares engines.node: >=18.0.0.

Does this integration's own code actually exercise the fix?
resolvePendingExecutions() in execution-guard.ts reads decisions
directly off guard.finish()'s own return value in the same statement — it
never calls takeDecision(). A fresh guard is created per step
(agent-loop.ts, inside the step loop) and is never reused once its
finish() is read, so there is no async gap between this integration's own
finish() call and its own decision consumption for GHSA-3xpw-9694-2xxp's
literal finish()-to-takeDecision() window to open in here. What is live is
root cause #1: whether a late/contradicting raw event arriving before a
step's own resolvePendingExecutions() call — anywhere in that step's
fullStream — correctly revokes authority instead of being silently
dropped by the adapter. New test:
src/__tests__/unit/execution-guard-post-terminal-authority.test.ts proves
this directly through this repo's real execution-guard.ts functions (the
exact call path agent-loop.ts uses), plus a control case confirming the
fix does not overcorrect a genuinely clean step, plus a second test against
the raw prefix-safe-json library directly (real push/finish/
takeDecision, no synthetic decision object) confirming the literal
advisory scenario is fixed at the library level.

Testing

  • src/__tests__/unit/execution-guard.test.ts: 16/16 passing against
    the real 0.4.3 install
    • safe tool-calls completion → executes exactly once
    • finishReason: "length" → does not execute
    • truncated arguments → does not execute
    • provider error → does not execute
    • unknown terminal state → does not execute
    • unrelated unregistered tool call → ignored
    • contrast case confirms the previous unconditional execution pattern would
      run in the same length scenario
    • two scopes registering the identical toolCallId never cross-resolve,
      even resolved in adversarial order
    • a discarded scope's closure never runs, even when a later, unrelated
      scope reuses the same toolCallId
    • a registration left unmatched by any guard decision is discarded, not
      carried forward
    • applyResolvedExecutionsToMessages: the safe case never leaves "Queued"
      in the transcript and shows the real result; the unsafe case never
      leaves "Queued" and never claims success; an unrelated tool-result
      message is returned byte-identical; only the matching toolCallId
      among multiple parts in one message is replaced
  • src/__tests__/unit/execution-guard-post-terminal-authority.test.ts
    (new): 3/3 passing — see "Dependency update" above
  • npx tsc --noEmit (full project, real 0.4.3 install): clean, 0 errors
  • npx eslint on every changed file: 0 errors, 0 warnings
  • npm run test:harness-boundary: passing
  • npm run build: passing
  • Targeted relevant existing suites — every agent-loop-*, permission-*,
    tool-history-integrity, cli-tools-mcp, and execution-guard* file
    (19 files, 354 cases): 316 passing, 38 failing. Proved, not assumed,
    that all 38 are pre-existing and unrelated to this change
    : ran the
    identical 19-file suite against this same branch with the dependency
    bump and the new test file stashed out (i.e. the unmodified prior commit),
    and diffed the two failure lists — byte-for-byte identical set of 38/50
    failing cases in both runs (diff empty). All failures cluster in
    permission-registry/permission-approval-token/permission-profile-*/DB
    persistence tests (HMAC token verification, SQLite roundtrips) — none in
    execution-guard*, cli-tools*, or agent-loop* proper — consistent
    with the native-binding/DB sandbox limitation this PR's main history
    already documented (see prior "Dependency update" note on 0.2.0,
    now folded into this section).

Rebase

Rebased onto current main (26 commits ahead at rebase time, none touching
this PR's files — clean rebase, no conflicts).

@vercel

vercelBot commented Aug 21, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the op7418's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actionsgithub-actionsBot added area:tests PR/issue 影响面: tests pr:large PR 过大,建议拆分或在描述说明 labels Aug 21, 2026
@github-actions

github-actionsBot commented Aug 21, 2026

Copy link
Copy Markdown

⚠️这个 PR 较大(9 个文件 / 1113 行改动;阈值 25 文件 / 800 行)。

建议拆分成更小、聚焦的 PR,或在描述里说明为什么需要一次性改这么多——这样更好审查、风险更低。

这只是提醒,不阻塞合并。

@canblmz1
canblmz1 marked this pull request as ready for review August 21, 2026 07:28
@canblmz1

Copy link
Copy Markdown
Author

recheck

canblmz1 pushed a commit to canblmz1/prefix-safe-json that referenced this pull request Aug 23, 2026
The 0.1.0 entry said the package "had already been integrated against
three independent real-world codebases (Dyad, CodePilot, Apache Maka)
across separate PRs" - past tense, implying settled, completed
integrations. Checked all three directly against their live PRs rather
than repeating the claim:
- dyad-sh/dyad#4341 and op7418/CodePilot#676 are both real and
substantive - genuine PRs pinning prefix-safe-json@0.0.1-alpha.4 with
real integration code and specific, named test suites - but both are
still open, not merged.
- apache/maka#3434 solves the identical problem (gating tool execution
on raw stream completion, not just JSON validity) but its own PR
description states it "adds no new runtime package, no
prefix-safe-json dependency" - a Maka-owned native implementation.
It should never have been grouped with the other two as a dependency
adopter.
None of the three amount to "integrated" in past tense, and open PRs
are not package adoption regardless of how substantive the patch
behind them is.
Added a dated correction note directly under the original paragraph
rather than editing it - the original text stays exactly as published,
readable for what it said and when, with the correction attached
immediately after it and sourced to the specific PRs it's about.
@canblmz1

Copy link
Copy Markdown
Author

Updated the integration from the old alpha build to the published prefix-safe-json 0.2.0 release and reran the execution-integrity tests. No architecture change; this only removes the prerelease dependency.

Can added 4 commits August 28, 2026 22:17
…irmed
Root cause: codepilot_cli_tools_install's execute() called execAsync(command)
directly and unconditionally, the moment the AI SDK finished parsing that
one tool call's arguments. It had no awareness of finishReason or whether
the *step* containing it (which may include other tool calls or trailing
text) ever reached a safe terminal state. A tool call can be syntactically
complete and still belong to a generation that was cut off by a token
limit, a provider error, or a content filter immediately after — the
existing permission system (permission-checker.ts) is a pure name/pattern
allow-list with no visibility into stream completion, so nothing in the
current codebase catches this. In "trust" mode, or for any future tool
without an explicit ask rule, a truncated/unconfirmed generation could
already have run a real shell command by the time anything downstream knew
the response wasn't finished.
execute() cannot simply await proof of that itself: the step's fullStream
can't reach its own finish part until every in-flight execute() call for
that step has already resolved, so waiting inside execute() would deadlock.
Fix: src/lib/execution-guard.ts defers the real side effect. execute()
registers it and returns an immediate "queued" result instead of running
the command; agent-loop.ts — which already iterates the step's fullStream
and already learns finishReason once the step ends — feeds every event
into a prefix-safe-json (npm) execution guard and, once the step is over,
resolves each pending registration against the guard's decision for that
exact toolCallId. Only a call whose surrounding step positively confirmed
completion ever actually runs; everything else is discarded before taking
effect. The confirmed/rejected outcome is surfaced as a follow-up
tool_result SSE event, reusing the existing event shape.
Files:
- src/lib/execution-guard.ts (new): registerDeferredExecution / createStepGuard
/ resolvePendingExecutions.
- src/lib/builtin-tools/cli-tools.ts: codepilot_cli_tools_install's execute
now registers via execution-guard instead of running execAsync directly;
no other tool or behavior changed.
- src/lib/agent-loop.ts: push every fullStream event into a per-step guard
(additive — these event types were previously unhandled, falling to the
existing `default: break`), resolve pending executions once the step's
finishReason is known.
- package.json / package-lock.json: add prefix-safe-json@0.0.1-alpha.4.
- src/__tests__/unit/execution-guard.test.ts (new): 7 tests — safe
completion executes exactly once; four unsafe terminal states (length,
truncated arguments, provider error, unknown) never execute; an
unregistered tool call is ignored; a contrast test demonstrating the
pre-fix unconditional-execute pattern for comparison.
Scope: only codepilot_cli_tools_install (the tool with the clearest,
directly-verifiable shell-execution side effect) is converted in this
patch. Other execute()-based tools (file writes, other MCP-backed tools)
have the same architectural exposure and could adopt the same
registerDeferredExecution pattern, but are left out here to keep this
patch reviewable.
Two follow-up fixes to the deferred-execution pattern from f783133.
1. Scope deferred execution state per turn, not by toolCallId alone.
execution-guard.ts's pending map was keyed only by toolCallId, a
provider-generated id with no uniqueness guarantee across concurrent
Native turns (two overlapping turns can both produce e.g. "call_1") —
the same identity-isolation concern runtime/native-turn-registry.ts
already handles for abort controllers, and for the same underlying
reason: module-local state shared across concurrent turns. A late
resolution from one turn could in principle have matched a different
turn's registration for the same toolCallId, or a thrown/aborted turn
could leave a registration in the map indefinitely with no path to
remove it.
Fix: a fresh `executionScopeId` (randomUUID()) is created once per
runAgentLoop invocation and threaded through the existing tool assembly
chain (agent-loop.ts -> assembleTools -> getBuiltinTools ->
createCliToolsTools), so the pending map is keyed by scope + toolCallId.
registerDeferredExecution fails closed (refuses to register, returns an
explicit error) if no scope is available. resolvePendingExecutions now
also fail-closed-discards any registration in its scope left unmatched
by a guard decision, instead of leaving it to linger. A new
discardPendingExecutions(scopeId) is called from agent-loop.ts's
teardown `finally` (which already runs on every exit path — success,
abort, timeout, thrown error), so a turn that never reaches its own
step-level resolution can never leave a queued shell command for a
later, unrelated turn to accidentally pick up.
2. Feed the real deferred result back into the model's own history, not
just the UI.
The tool's execute() returns an immediate "Queued…" placeholder, which
the AI SDK bakes into responseData.messages as that call's tool-result
— and responseData.messages is exactly what gets appended to the
conversation for the next step. Previously only the UI (via a follow-up
SSE tool_result) ever learned the real outcome; the model's own
transcript permanently kept "Queued…" as the result, so a later step
had no way to know whether the command actually ran.
Fix: ResolvedExecution now carries a single `outcomeText` (the real
result text, or an explicit "Execution skipped: ..." message for a
rejected call) computed once in resolvePendingExecutions, so the UI
SSE and the model transcript can never disagree about what happened.
applyResolvedExecutionsToMessages(responseData.messages,
resolvedExecutions) replaces the queued placeholder tool-result part
for each resolved toolCallId before the messages get appended to the
loop's `messages` array — using the repository's existing
ModelMessage tool-result shape (role: "tool", content: [{ type:
"tool-result", toolCallId, toolName, output: { type: "text", value }
}], matching tool-history-integrity.ts). Only the matching part is
replaced; every other message and part is returned unchanged (same
reference, not cloned).
Scope unchanged from f783133: only Native Runtime's
codepilot_cli_tools_install is affected. cli-tools-mcp.ts (the SDK
Runtime's independent implementation) is not touched. prefix-safe-json
stays pinned at 0.0.1-alpha.4.
Tests: 16 in execution-guard.test.ts (10 from f783133 plus 6 new) —
two scopes registering the identical toolCallId never cross-resolve in
adversarial resolution order; a discarded scope's closure never runs
even when a later, unrelated scope reuses the same toolCallId; an
unmatched registration is discarded, not carried forward; and
applyResolvedExecutionsToMessages never leaves "Queued" in the transcript
for either the safe (real result) or unsafe (explicit skip, no false
success) case, leaves unrelated tool-result parts byte-identical, and
only replaces the matching toolCallId among multiple parts in the same
message.
…release
prefix-safe-json 0.0.1-alpha.4 -> 0.2.0 (exact). The API this integration
uses (createAiSdkExecutionGuard, AiSdkExecutionGuard.push/finish) is
unchanged between the two, so no source changes were needed. 0.2.0 still
declares engines.node >=18, which matches this repo's Node 20 build/release
workflow (a future prefix-safe-json release narrowing to Node >=22 would
not).
Verified: resolved version is exactly 0.2.0, typecheck clean, and the
execution-guard/agent-loop/permission/tool-history-integrity suites all
still pass unchanged.
…lease
prefix-safe-json 0.2.0 -> 0.4.3 (exact). 0.4.3 is a security patch
(GHSA-3xpw-9694-2xxp) fixing three root causes in the AI SDK adapter/gate
this integration depends on:
1. AiSdkStreamAdapter.push() used to silently drop every raw event once
it had already observed its own terminal, so late/contradictory
evidence for a call never reached the coordinator at all.
2. takeDecision() used to read a decision snapshot frozen at finish()
time instead of the coordinator's live diagnostics, so evidence
recorded after finish() but before that call's authority was consumed
was never consulted.
3. A raw event carrying conflicting id/toolCallId used to silently
prefer id instead of failing the stream closed.
No public API change - createAiSdkExecutionGuard()/push()/finish()/
takeDecision() and the ExecuteDecision/NonExecutableDecision shapes this
integration reads (action, toolCallId, reason) are unchanged, confirmed
directly against the installed package's .d.ts, not assumed.
Verified against the real 0.4.3 install (resolved version, and lockfile
integrity hash, both confirmed exactly 0.4.3):
- npx tsc --noEmit (full project): clean, 0 errors.
- src/__tests__/unit/execution-guard.test.ts: 16/16 passing, identical
to the 0.2.0 result.
Adds src/__tests__/unit/execution-guard-post-terminal-authority.test.ts,
answering the actual question this bump exists to answer: does this
integration's own code exercise the fix, not just "does the library
still pass its own tests." resolvePendingExecutions() in
execution-guard.ts reads `decisions` directly off guard.finish()'s own
return value in the same statement - it never calls takeDecision().
A fresh guard is created per step and is never reused once its finish()
is read, so there is no async gap between this integration's own
finish() call and its own decision consumption for GHSA-3xpw-9694-2xxp's
literal finish()-to-takeDecision() window to open in here.
What is live in this integration is root cause op7418#1: whether a
late/contradicting raw event that arrives before a step's own
resolvePendingExecutions() call - anywhere in that step's fullStream -
correctly revokes authority instead of being silently dropped by the
adapter. The new test proves that directly, through this repo's real
execution-guard.ts functions (createStepGuard/registerDeferredExecution/
resolvePendingExecutions - the exact call path agent-loop.ts uses), plus
a control case confirming the fix does not overcorrect a genuinely clean
step. A second test exercises the raw prefix-safe-json library
(createAiSdkExecutionGuard directly, real push/finish/takeDecision, no
synthetic decision object) to confirm the literal advisory scenario is
fixed at the library level.
package-lock.json regenerated with `npm install prefix-safe-json@0.4.3
--save-exact`, not hand-edited.
@canblmz1
canblmz1force-pushed the fix/cli-install-execution-integrity branch from b817aae to cc8ac63CompareAugust 28, 2026 19:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:testsPR/issue 影响面: testspr:largePR 过大,建议拆分或在描述说明

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@canblmz1