De-overfit the provider bridge kit: unwind frames, split the session registry, repatriate single-consumer helpers - #1742
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I started the security, quality, performance, architecture, and end-to-end review. I will post one final review after I combine the results. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| } | ||
| return true; | ||
| }, | ||
| resolvePendingToolCalls: (scope, message) => { |
There was a problem hiding this comment.
🚨 slopcop/review — Low: Scope cleanup can become quadratic.
Each scope cleanup scans the full map. Closing many sessions can cost O(S × P) instead of work local to each session.
A benchmark took 264 ms for 10,000 scopes and 1,041 ms for 20,000 scopes. Please add a per-scope request index.
| requestIdCounter += 1; | ||
| const requestId = requestIdCounter; | ||
| pendingToolCalls.set(requestId, { resolve, scope: args.scope }); | ||
| options.sendToolCall({ |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: A send error breaks the stated promise contract.
forwardToolCall states that it never rejects. A synchronous sendToolCall error rejects the promise and retains the pending entry.
Please catch the error, delete the entry, and resolve an error result. Add a test with a sender that throws.
| buildEditDiff, | ||
| buildShellEnvOverrides, | ||
| buildShellEnvironmentPolicyConfig, | ||
| buildFileChangeItem, |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: The new public helper returns an empty item ID.
The exported helper returns id: "". Its result cannot pass the file-change parser without a caller replacement.
Please add callId to the helper arguments and assign it to id. Add a direct helper test for this contract.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This pull request removes a large shared helper that managed provider sessions and tool calls.
It gives each provider control of its session lifecycle. It also moves single-provider helpers back to their providers and reduces the public kit.
Findings
-
Medium — The new public
buildFileChangeItemexport returns an empty ID.First-party callers replace the ID. External plugin authors can emit an item that the thread view rejects.
Require
callId, assign it toid, and add a direct contract test. -
Medium —
forwardToolCallcan reject after a synchronoussendToolCallerror.This result conflicts with its no-rejection contract and leaves stale pending state.
Catch the error, delete the entry, and resolve an
isError: trueresult. -
Low — Scope cleanup scans all pending calls for each closing session.
The cost can grow with the square of the session count.
A benchmark took 264 ms for 10,000 scopes and 1,041 ms for 20,000 scopes. Add a per-scope request index.
Architecture and security
Pi and Claude repeat some session-close code. Their stop behavior differs, so I do not recommend a shared helper yet.
I found no security issue.
Validation
- All 1,056 affected-package tests passed.
- All 13 Turbo test and typecheck tasks passed.
git diff --checkpassed.- A local Claude bridge request returned
bridge-ok. - The development browser showed the prompt and response.
- The GPT-5.6 final check confirmed all three findings.
This review uses a comment only. I did not approve the pull request or request changes.
06a32ed to
65b325a
Compare
…ver rejects Two findings from the #1742 review: - buildFileChangeItem returned id: "" and relied on every caller spreading its own id over the result — a trap now that the helper is published. callId is a required argument and becomes the item id; both first-party switches pass it directly. - forwardToolCall's contract says it never rejects, but a synchronous sendToolCall throw rejected the promise and stranded the pending map entry. The sender call is now guarded: on throw the entry is removed and the call resolves as an isError result. Covered by a throwing- sender test that also proves the entry does not linger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The kit's buildToolUseItem router (command/fileChange/special parser options) existed only so two consumers could feed it callbacks. Each consumer now writes its own explicit switch over its tool names, calling the plain constructors (buildFileChangeItem, new buildGenericToolCallItem) directly, preserving exact behavior: parser-returns-null falls back to the generic toolCall with raw args, parentToolCallId threads on every branch, and claude's web-tool special case stays last. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The kit keeps only what both bridges genuinely share: a pending-tool-call tracker that mints item/tool/call requests, matches responses by request id directly (no per-response all-sessions scan), and error-resolves a session's calls on close. The sessions map, close dedup (closingSessions + finally cleanup), and close-all move into the two consumers, which also absorb the single-consumer options (nextToolCallRequestId, resolveAdditionalPendingWork, stopSession) and the TSession/TCloseResult generics. Claude-code's interactive requests now mint their own string-prefixed ids so they cannot collide with the tracker's numeric ids on the shared channel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decodeNativeProviderToolCallRequest and its schema had zero consumers — deleted. The runtime adapter's inline duplicate of the normalized decode now delegates to decodeNormalizedProviderToolCallRequest (the test fake already used it), and providerToolCallResponseSchema moves into bridge-tool-calls.ts as a kit-private const, its only consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
buildShellEnvironmentPolicyConfig moves into provider-codex: its shell_environment_policy.set.* namespace is codex-native and codex is its only real consumer. Claude-code stops borrowing that namespace: its plugin-internal session config bag now carries a plain env map under its own envVars key (still filtered through buildShellEnvOverrides), and the kit's extractEnvOverrides decode is deleted with it. diffCumulativeText and its two interfaces move into agent-runtime's pi provider, their only consumer, tests along with them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the core - Delete finishOpenProviderTurn + FinishOpenProviderTurnArgs (only caller was a claude-code test, which now inlines the three lines). - Make contentWrapperSchema kit-private (adapter-utils is its only user) and inline the NormalizeProviderCommandOutputArgs one-off type. - Drop preserveUndefinedToolCallFields from completeStartedToolItem's public args; buildToolResultItem keeps the historical output shape via a private variant. - Drop completeWebItems from buildToolResultItem; claude-code (the only provider that passed true) completes started web items at its own call site with completeStartedToolItem. - Fold the identical drainAcceptedUserMessages call out of all three providers' onTurnStart hooks into the turn-state registry core (ProviderTurnState now carries pendingAcceptedUserMessages), shrinking the hooks to provider-only resets and un-publishing the drain helper. - Rename CounterScopedItemIdState's fields to openScopedItemIdsByScope / scopedItemCounter project-wide: compaction ids flow through them too, so the reasoning-only names lied. - Regenerate the SDK's bundled provider-bridge types and record this de-overfitting pass in docs/api_to_audit.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ver rejects Two findings from the #1742 review: - buildFileChangeItem returned id: "" and relied on every caller spreading its own id over the result — a trap now that the helper is published. callId is a required argument and becomes the item id; both first-party switches pass it directly. - forwardToolCall's contract says it never rejects, but a synchronous sendToolCall throw rejected the promise and stranded the pending map entry. The sender call is now guarded: on throw the entry is removed and the call resolves as an isError result. Covered by a throwing- sender test that also proves the entry does not linger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
71a6d11 to
efc4304
Compare
1 participant
De-overfits
bridge-kitfollowing a two-reviewer adversarial audit of every module and export, calibrated on one ruling: GOOD = code encoding bb-side invariants (canonical event/item semantics, wire contracts both sides validate); BAD = a frame hosting per-provider knowledge injected through generics, callbacks, or config objects. ~80% of the kit survived the audit as genuine invariants (notablyturn-state, whose suspect generic-plus-hooks shape is driven identically by all three consumers); this PR removes the part that didn't. One commit per finding group:buildToolUseItem— the router was an if-chain each bridge could write itself, inverted into the kit via name-sets + parser callbacks + an escape hatch supplied by exactly one provider. The invariant constructors stay (buildFileChangeItem, newbuildGenericToolCallItemfallback); claude-code and pi own explicit switches, preserving null-parse→generic fallback and parent-id threading.bridge-session-registry— only 2 of 4 bridges used it, and over half its option surface existed for one consumer each (nextToolCallRequestId,resolveAdditionalPendingWork,stopSessionclaude-code-only;TCloseResultpi-only). The genuinely shared kernel survives ascreatePendingToolCallTracker(flat requestId→resolver map — also removes the per-response all-sessions scan); session orchestration inlined into the two bridges. Reviewer note: claude-code's interactive requests previously shared the tool-call id counter; they now use string-prefixed ids (interaction-N), keeping the id spaces collision-free. Wire-compatible: JSON-RPC ids arestring | numberand both ends treat them opaquely.decodeNativeProviderToolCallRequesthad zero consumers anywhere (deleted);bridge-protocol-adapterreimplementeddecodeNormalizedProviderToolCallRequestinline (now delegates);providerToolCallResponseSchemaprivatized.buildShellEnvironmentPolicyConfig→ provider-codex (itsshell_environment_policy.set.*namespace is codex-native config); claude-code stops round-tripping env vars through codex-style keys and carries a plain map under its own bag key (both ends verified inside the plugin — nothing crosses the daemon wire schema).diffCumulativeText→ pi, its only consumer.finishOpenProviderTurn(only caller was a test) deleted; two per-provider policy booleans removed from the good constructors (completeWebItems,preserveUndefinedToolCallFields); the universally-identicaldrainAcceptedUserMessagescall folded into the turn-state core;CounterScopedItemIdState's lyingreasoning*field names renamed (compaction ids flow through them too);contentWrapperSchemaand dead arg interfaces un-exported. Includes thedocs/api_to_audit.mdrecord and regenerated bundled types.Numbers
Verification
🤖 Generated with Claude Code