Skip to content

fix(responses): read-only proxy tools no longer drive the compress loop (炸锅) - #90

Open
ranxianglei wants to merge 3 commits into
masterfrom
fix/responses-readonly-proxy-loop
Open

fix(responses): read-only proxy tools no longer drive the compress loop (炸锅)#90
ranxianglei wants to merge 3 commits into
masterfrom
fix/responses-readonly-proxy-loop

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Problem

Reported in dog/billion-context-pi#22 (log: `bili (5).log`).

When the model called acp_status (or search_context), the proxy treated it like compress/decompress: it executed it server-side and re-requested the upstream. A model that called acp_status round after round hit the round cap:

```
[acp-diag] round 5 allCalls=[acp_status] ...
[acp-proxy] compress loop limit (5) reached, forwarding completion as-is
```

and the whole turn was discarded — the client received an empty completion (炸锅).

Root cause

PROXY_TOOL_NAMES lumps all 4 proxy tools into one set, but they are not equivalent:

tool kind should loop?
compress mutating ✅ re-request
decompress mutating ✅ re-request
search_context read-only ❌ surface + end turn
acp_status read-only ❌ surface + end turn

A read-only round has no state change to feed back to the model, so re-requesting just makes the model repeat itself until the cap fires.

Fix

Split PROXY_TOOL_NAMES into:

  • `MUTATING_PROXY_TOOLS = { compress, decompress }` — drive the re-request loop

  • `READONLY_PROXY_TOOLS = { search_context, acp_status }` — execute once, surface the result to the client, end the turn

  • Only a round containing a mutating tool triggers an upstream re-request.

  • A read-only-only round executes the tool(s), emits the visibility marker, and completes.

  • A mixed round (e.g. compress + acp_status) still re-requests, because a mutating tool is present.

This mirrors the Pi adapter (billion-context-pi), where the host executes acp_status once and drives the next turn itself — so it can never loop or discard.

Scope

This PR fixes the Responses path (src/compress-loop-responses.ts), which is what the failing log uses. The Anthropic (src/compress-loop-anthropic.ts) and OpenAI-chat (src/compress-loop.ts) loop files have the same structural bug and should get the same split in a follow-up.

Tests

Two new regression tests in `tests/proxy-responses-stream.test.ts`:

  1. read-only-only round — model calls only acp_status → asserts `fetchCalls === 0` (no re-request), `[ACP]` marker present, `response.completed` present, no `compress loop limit`.
  2. mixed round — `compress` + `acp_status` → asserts exactly one re-request (mutating tool still drives the loop).

```
ℹ tests 214 (+2)
ℹ pass 214
ℹ fail 0
```

npm run typecheck

Not fixed here (separate issues from the log)

  1. Anthropic / OpenAI-chat loop files — same readonly-loop bug, follow-up.
  2. Tag echo (~57×) — model regurgitates input <acp tokens=…> ref tags in output; `server.ts` detects+warns only, no strip on the responses path. Cosmetic.
  3. reasoning preamble accumulating to 50 — likely v0.1.27 residue (master routes reasoning as a tracked BiliMessage via PR#75); verify on v0.1.31.

🤖 PR opened by agent. Merge is human-only.

acp_status and search_context are read-only: they execute server-side once
and surface the result to the client. Treating them like compress/decompress
(mutation) forced a re-request every time, so a model that called acp_status
looped until the round cap (5) was hit and the whole turn was discarded.

Now only MUTATING proxy tools (compress/decompress) drive the re-request
loop; READONLY tools (acp_status/search_context) execute once and end the
turn. A mixed round (compress + acp_status) still re-requests because a
mutating tool is present.

Adds two regression tests covering the read-only-only and mixed cases.

Fixes dog/billion-context-pi#22 (bili (5).log)
Address oracle review (PR #90, merge-after-nits):

1. Q2 (should-fix): JSON path dropped read-only markers when a real tool
   accompanied them; stream path surfaced them. Drop the realCalls===0 gate
   on surfaceReadonlyJson so both paths behave identically.

2. Nit: wrap read-only tool execution in try/catch (both stream + JSON) so
   one failing acp_status/search_context emits a FAILED marker instead of
   killing the whole turn's stream.

3. Nit: add markers.length suffix to surfaceReadonlyJson message id to
   match the stream path's defensive id scheme.

Tests (+7, 212→219):
- stream: search_context-only (no re-request)
- stream: decompress-only STILL drives the loop (locks no-behavior-change)
- stream: real tool + acp_status surfaces marker, forwards call, no re-request
- stream: mixed-round assertion tightened (count [ACP] >= 2)
- JSON: acp_status-only surfaces marker, no re-request
- JSON: real tool + acp_status surfaces marker AND preserves call (Q2 parity)
…ress loop

Apply the same MUTATING/READONLY split from PR #90 (responses path) to
the OpenAI chat-completions loop (compress-loop.ts) and the Anthropic
loop (compress-loop-anthropic.ts).

acp_status/search_context now execute once, surface their result to the
client as a visibility marker, and end the turn WITHOUT a re-request.
Only compress/decompress drive the re-request loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant