Skip to content

Commit 95d8ed7

Browse files
committed
revert(anthropic): drop the streamed tool-argument validation, keep the rest
Two attempts at making the streaming path agree with the non-stream path about malformed tool arguments, both rejected by review for a real reason. Stopping at the third rather than pushing another patch at the same spot. Attempt one buffered `input_json_delta` fragments and validated at block close. That broke the streaming contract: `src/bridge.ts:628` turns each adapter delta into a client-visible `response.function_call_arguments.delta` frame, and `tests/responses-stream-tool-events.test.ts:30` pins that split fragments stay split. A started call would show empty arguments until block close. Attempt two kept fragments flowing and errored the turn when the assembled JSON did not parse. Also wrong: the bridge's `error` case closes an open tool call anyway (`src/bridge.ts:771`), emitting `response.function_call_arguments.done` and an `output_item.done` with `status:"completed"` BEFORE `response.failed`. The client is still told the call completed. The fix belongs in the bridge -- a terminal error mode that cancels rather than closes an open tool call. That is out of scope for an Anthropic adapter PR, affects every adapter's error path, and touches code whose comment records that `JSON.parse("")` once poisoned whole sessions. It deserves its own cycle with that history read first, not a third patch from this end. Kept, because both stand on their own: the non-stream `toolUseArguments` degradation to `{}`, which fixes the half of #765 that is actually fixable here, and the EOF stop-reason regression test that exposed the two existing EOF tests as vacuous. Reverted: every streaming-path change. That path now behaves exactly as it did before this round, so nothing regressed -- the known defect is simply still there, and #765 stays open for it. Reasoning recorded in devlog/_plan/260731_pr_merge_round/021.
1 parent 299cd2f commit 95d8ed7

3 files changed

Lines changed: 67 additions & 131 deletions

File tree

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# 021 — 스트림 tool 인자 검증을 이번 라운드에서 뺀 이유
2+
3+
#765를 태우면서 파생된 문제 하나를 세 번 고치려다 실패했다. 세 번째에서
4+
멈추고 되돌렸다. 기록으로 남긴다 — 다음 사람이 같은 길을 다시 걷지 않도록.
5+
6+
## 발단
7+
8+
`toolUseArguments()`가 파싱 불가능한 문자열 input을 JSON **문자열**로 재인코딩했다
9+
(`src/adapters/anthropic.ts:269`). tool 계약은 객체를 요구하므로
10+
`"not json at all"`이 그대로 전달된다. #765가 보고한 이중 인코딩이다.
11+
`{}`로 떨어뜨리는 것으로 고쳤고, 이건 남아 있다.
12+
13+
그런데 리뷰어가 짚었다: **스트리밍 경로는 그대로다.** `input_json_delta`
14+
`partial_json`을 그대로 흘려보내므로(`:836`) 같은 깨진 페이로드가
15+
`parseResponse`로는 `{}`, `parseStream`으로는 `"not json"`이 된다. 한 결함에
16+
두 답.
17+
18+
## 세 번의 시도
19+
20+
**1차 — 조각을 버퍼링해서 블록 종료 시 검증.** 두 경로가 같은 헬퍼를 타게 만들었다.
21+
리뷰어가 거부: `src/bridge.ts:628`이 어댑터 delta 하나하나를 클라이언트가 보는
22+
`response.function_call_arguments.delta` 프레임으로 바꾼다. 조각을 붙들면
23+
시작된 함수 호출이 블록이 끝날 때까지 빈 인자로 보인다.
24+
`tests/responses-stream-tool-events.test.ts:30`이 쪼개진 프레임 유지를 고정하고 있다.
25+
일관성을 얻으려고 프로토콜을 깬 것이다.
26+
27+
**2차 — 증분 전달은 유지하고, 조립본이 파싱 안 되면 턴을 에러로.** 계약은 복구됐다.
28+
리뷰어가 다시 거부: 어댑터가 `tool_call_end` 없이 `error`를 내도
29+
`src/bridge.ts:771`의 error 케이스가 열린 tool call을 닫는다.
30+
`closeCurrentToolCall()``response.function_call_arguments.done`
31+
`status:"completed"``response.output_item.done``response.failed` **앞에**
32+
내보낸다. 즉 클라이언트는 여전히 "완료된 호출"을 본다.
33+
34+
**3차 — 없음.** 여기서 멈췄다.
35+
36+
## 왜 멈췄나
37+
38+
실제 수정 지점이 `src/bridge.ts`다. 열린 tool call을 닫지 말고 취소하는
39+
터미널 에러 모드를 새로 만들어야 하는데, 그건:
40+
41+
- 이 PR(#765, Anthropic 어댑터)의 범위 밖이다.
42+
- 모든 어댑터의 error 경로에 영향을 준다. Anthropic만의 문제가 아니다.
43+
- Codex가 다음 턴에 호출을 돌려보낼 때의 동작을 바꾼다.
44+
`closeCurrentToolCall()`의 주석이 왜 빈 인자를 `"{}"`로 직렬화하는지 설명한다 —
45+
`JSON.parse("")`가 세션 전체를 400으로 오염시킨 이력이 있다. 이 경로는
46+
건드리기 전에 그 이력을 먼저 이해해야 한다.
47+
48+
같은 결함을 두 번 연속 고치지 못했으면 패치를 멈추고 근본 원인으로 가라는 게
49+
규칙이다. 세 번째는 계획을 바꾸는 자리지 세 번째 패치를 미는 자리가 아니다.
50+
51+
## 지금 상태
52+
53+
- **유지**: non-stream `toolUseArguments``{}` 수정과 그 테스트.
54+
독립적으로 옳고, #765가 보고한 절반을 실제로 고친다.
55+
- **유지**: EOF stop-reason 회귀 테스트. 기존 EOF 테스트 둘 다 stop reason을
56+
안 보내서 fallback을 되돌려도 통과한다 — 무력한 테스트였다. 새 테스트는
57+
fallback을 끄면 실패한다(14 pass / 1 fail 확인).
58+
- **되돌림**: 스트리밍 경로 변경 전부. 지금 동작은 이 라운드 이전과 같다.
59+
60+
**남은 결함**: 스트리밍 경로는 여전히 깨진 `partial_json`을 그대로 흘린다.
61+
#765는 그래서 열어둔다. 다음 사이클의 단위는 "Anthropic 어댑터"가 아니라
62+
**"bridge의 터미널 에러가 열린 tool call을 어떻게 처리해야 하는가"**다.
63+
어댑터부터 손대면 또 같은 벽에 부딪힌다.

src/adapters/anthropic.ts

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -284,23 +284,6 @@ function toolUseArguments(input: unknown): string {
284284
return JSON.stringify(input ?? {});
285285
}
286286

287-
/**
288-
* Whether the arguments assembled from a stream's `input_json_delta` fragments are usable.
289-
* A tool block that sent no fragments at all is fine -- that is a no-argument call. Anything
290-
* else has to parse, because unlike the non-stream path the fragments have already been
291-
* forwarded to the client and cannot be repaired after the fact.
292-
*/
293-
function streamedToolArgumentsParse(assembled: string): boolean {
294-
const trimmed = assembled.trim();
295-
if (!trimmed) return true;
296-
try {
297-
JSON.parse(trimmed);
298-
return true;
299-
} catch {
300-
return false;
301-
}
302-
}
303-
304287
function anthropicKeyUsesBearer(provider: OcxProviderConfig): boolean {
305288
return provider.apiKeyTransport === "bearer";
306289
}
@@ -782,7 +765,6 @@ export function createAnthropicAdapter(provider: OcxProviderConfig, cacheRetenti
782765
let currentBlockType = "";
783766
let currentToolCallId = "";
784767
let currentToolCallName = "";
785-
let currentToolCallJson = "";
786768
let pendingUsage: Record<string, number> | undefined;
787769
let pendingStopReason: string | undefined;
788770
let emittedDone = false;
@@ -826,7 +808,6 @@ export function createAnthropicAdapter(provider: OcxProviderConfig, cacheRetenti
826808
if (block.type === "tool_use") {
827809
currentToolCallId = block.id ?? synthesizeToolUseId();
828810
currentToolCallName = toolNames.fromWire(block.name ?? "");
829-
currentToolCallJson = "";
830811
yield { type: "tool_call_start", id: currentToolCallId, name: currentToolCallName };
831812
}
832813
if (block.type === "redacted_thinking" && typeof block.data === "string") {
@@ -852,31 +833,14 @@ export function createAnthropicAdapter(provider: OcxProviderConfig, cacheRetenti
852833
// so a stray signature on a non-thinking block can never be captured.
853834
yield { type: "thinking_signature", signature: delta.signature };
854835
} else if (delta.type === "input_json_delta" && typeof delta.partial_json === "string" && currentBlockType === "tool_use") {
855-
// Forwarded immediately: the bridge maps each delta to a client-visible
856-
// response.function_call_arguments.delta frame, so withholding fragments until
857-
// block close would leave a started call showing empty arguments. A copy is kept
858-
// to validate the assembled payload at content_block_stop.
859-
currentToolCallJson += delta.partial_json;
860836
yield { type: "tool_call_delta", arguments: delta.partial_json };
861837
}
862838
break;
863839
}
864840
case "content_block_stop": {
865841
if (currentBlockType === "tool_use") {
866-
// The non-stream path repairs an unparseable payload in toolUseArguments(); the
867-
// stream cannot, because the fragments are already downstream. Fail the turn
868-
// instead of ending a tool call whose arguments will not parse -- a client that
869-
// received `not json` must not be told the call completed normally.
870-
if (!streamedToolArgumentsParse(currentToolCallJson)) {
871-
yield {
872-
type: "error",
873-
message: "Anthropic stream sent malformed tool_use arguments (invalid JSON)",
874-
};
875-
return;
876-
}
877842
yield { type: "tool_call_end" };
878843
currentToolCallId = "";
879-
currentToolCallJson = "";
880844
}
881845
currentBlockType = "";
882846
break;

tests/anthropic-stream-hardening.test.ts

Lines changed: 4 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -104,102 +104,11 @@ describe("anthropic non-stream tool_use input", () => {
104104
});
105105
});
106106

107-
test("malformed streamed tool arguments fail the turn instead of completing the call", async () => {
108-
// The stream forwards fragments as they arrive (the bridge turns each into a client-visible
109-
// frame), so a malformed payload cannot be repaired the way the non-stream path repairs it.
110-
// It must not end the call normally either: a client that already received `not json` would
111-
// be told the tool call completed. The turn errors instead.
112-
const response = new Response([
113-
"event: content_block_start\n",
114-
'data: {"type":"content_block_start","content_block":{"type":"tool_use","id":"toolu_9","name":"get_weather"}}\n\n',
115-
"event: content_block_delta\n",
116-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"not "}}\n\n',
117-
"event: content_block_delta\n",
118-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"json"}}\n\n',
119-
"event: content_block_stop\n",
120-
'data: {"type":"content_block_stop"}\n\n',
121-
"event: message_stop\n",
122-
'data: {"type":"message_stop"}\n\n',
123-
].join(""));
124-
const events = await collect(createAnthropicAdapter(provider).parseStream(response));
125-
expect(events.at(-1)?.type).toBe("error");
126-
expect(events.some(e => e.type === "tool_call_end")).toBe(false);
127-
expect(events.some(e => e.type === "done")).toBe(false);
128-
});
129-
130-
test("valid streamed tool arguments still arrive as incremental deltas", async () => {
131-
// The bridge maps each adapter delta to response.function_call_arguments.delta, so the
132-
// fragments must keep flowing as they arrive rather than being withheld until block close.
133-
// Both fragments, in order, and the call ends normally.
134-
const response = new Response([
135-
"event: content_block_start\n",
136-
'data: {"type":"content_block_start","content_block":{"type":"tool_use","id":"toolu_10","name":"get_weather"}}\n\n',
137-
"event: content_block_delta\n",
138-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"{\\"city\\":"}}\n\n',
139-
"event: content_block_delta\n",
140-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"\\"Paris\\"}"}}\n\n',
141-
"event: content_block_stop\n",
142-
'data: {"type":"content_block_stop"}\n\n',
143-
"event: message_stop\n",
144-
'data: {"type":"message_stop"}\n\n',
145-
].join(""));
146-
const events = await collect(createAnthropicAdapter(provider).parseStream(response));
147-
expect(events.filter(e => e.type === "tool_call_delta")).toMatchObject([
148-
{ type: "tool_call_delta", arguments: '{"city":' },
149-
{ type: "tool_call_delta", arguments: '"Paris"}' },
150-
]);
151-
expect(events.some(e => e.type === "tool_call_end")).toBe(true);
152-
expect(events.at(-1)?.type).toBe("done");
153-
});
154-
155-
test("two tool_use blocks in one message keep separate argument buffers", async () => {
156-
// The validation buffer is per block; a leak would make the second block inherit the first
157-
// block's JSON and could fail a well-formed call.
158-
const response = new Response([
159-
"event: content_block_start\n",
160-
'data: {"type":"content_block_start","content_block":{"type":"tool_use","id":"toolu_a","name":"one"}}\n\n',
161-
"event: content_block_delta\n",
162-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"{\\"one\\":1}"}}\n\n',
163-
"event: content_block_stop\n",
164-
'data: {"type":"content_block_stop"}\n\n',
165-
"event: content_block_start\n",
166-
'data: {"type":"content_block_start","content_block":{"type":"tool_use","id":"toolu_b","name":"two"}}\n\n',
167-
"event: content_block_delta\n",
168-
'data: {"type":"content_block_delta","delta":{"type":"input_json_delta","partial_json":"{\\"two\\":2}"}}\n\n',
169-
"event: content_block_stop\n",
170-
'data: {"type":"content_block_stop"}\n\n',
171-
"event: message_stop\n",
172-
'data: {"type":"message_stop"}\n\n',
173-
].join(""));
174-
const events = await collect(createAnthropicAdapter(provider).parseStream(response));
175-
expect(events.filter(e => e.type === "tool_call_delta")).toMatchObject([
176-
{ type: "tool_call_delta", arguments: '{"one":1}' },
177-
{ type: "tool_call_delta", arguments: '{"two":2}' },
178-
]);
179-
expect(events.filter(e => e.type === "tool_call_end")).toHaveLength(2);
180-
expect(events.at(-1)?.type).toBe("done");
181-
});
182-
183-
test("a tool_use block with no argument fragments is not treated as malformed", async () => {
184-
// A no-argument tool call sends no input_json_delta at all. An empty buffer must stay valid.
185-
const response = new Response([
186-
"event: content_block_start\n",
187-
'data: {"type":"content_block_start","content_block":{"type":"tool_use","id":"toolu_c","name":"now"}}\n\n',
188-
"event: content_block_stop\n",
189-
'data: {"type":"content_block_stop"}\n\n',
190-
"event: message_stop\n",
191-
'data: {"type":"message_stop"}\n\n',
192-
].join(""));
193-
const events = await collect(createAnthropicAdapter(provider).parseStream(response));
194-
expect(events.some(e => e.type === "error")).toBe(false);
195-
expect(events.some(e => e.type === "tool_call_end")).toBe(true);
196-
expect(events.at(-1)?.type).toBe("done");
197-
});
198-
199107
test("EOF after message_delta.stop_reason settles instead of erroring", async () => {
200-
// The blocker the other EOF tests could not see: they assert the pre-existing error path, so
201-
// reverting the stop-reason fallback leaves them green. This drives the fallback itself -- a
202-
// stream that reported why it stopped but never sent message_stop.
108+
// The other two EOF tests assert the pre-existing error path and never send a stop reason,
109+
// so they stay green with this fallback reverted -- they do not test the change they shipped
110+
// with. This drives the fallback itself: a stream that reported why it stopped but never
111+
// sent message_stop.
203112
const response = new Response([
204113
"event: content_block_start\n",
205114
'data: {"type":"content_block_start","content_block":{"type":"text","text":""}}\n\n',

0 commit comments

Comments
 (0)