Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 4 of 13: claude executor, thinking, signatures - #17
ericlitman wants to merge 1 commit into
Conversation
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
🛑 9 blocking findings🔴 Critical · Reject truncated summaries before sealing compaction 🟡 Medium · Validate the finalized count-token body 🟡 Medium · Keep response parsing aligned with the configured stream value 🟡 Medium · Do not overwrite a parent message's alias state 🟡 Medium · Lift token-count betas after payload finalization 🟡 Medium · Validate token-count model compatibility after payload rules 🟡 Medium · Reset Gemini function-call groups across server-tool parts 🟡 Medium · Do not delete prompt-cache configuration after payload rules 🔵 Low · Place alias-cache support under helps WalkthroughThis review-only PR is part 4 of 13 of the upstream
Since the last review
Repair promptRead the
Details
First review of this pull request. @unfret review · @unfret status · @unfret help |
![unfret[bot]](https://avatars.githubusercontent.com/in/4933690?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛑 9 blocking findings
🔴 Critical · Reject truncated summaries before sealing compaction
🟡 Medium · Validate the finalized count-token body
🟡 Medium · Keep response parsing aligned with the configured stream value
🟡 Medium · Do not overwrite a parent message's alias state
🟡 Medium · Lift token-count betas after payload finalization
🟡 Medium · Validate token-count model compatibility after payload rules
🟡 Medium · Reset Gemini function-call groups across server-tool parts
🟡 Medium · Do not delete prompt-cache configuration after payload rules
🔵 Low · Place alias-cache support under helps
Check
| type claudeOAuthToolAliasStore struct { | ||
| mu sync.Mutex | ||
| entries map[string]map[string]string | ||
| order []string | ||
| } |
There was a problem hiding this comment.
🔵 Low · Place alias-cache support under helps
AGENTS.md requires executor/ to contain executors and their unit tests only, with helper/supporting files under executor/helps/. This new file adds a supporting alias-cache implementation, including storage, locking, cloning, and eviction, directly under executor/. Move the cache-support portion under helps to satisfy the mandatory directory rule. Also reported for this defect: - Place the new alias-store support code under helps (internal/runtime/executor/claude_executor_tool_state.go:18-22): AGENTS.md requires helper/supporting files to live under internal/runtime/executor/helps/, reserving the executor directory for executors and their unit tests. This new file introduces alias-cache storage, cloning, key construction, and error helpers directly in the executor directory. Move the supporting implementation under helps while retaining executor-specific integration in executor files.
| capsule, errSeal := helps.SealAntigravityCompaction(summaryText, baseModel) | ||
| if errSeal != nil { | ||
| return resp, fmt.Errorf("seal compaction capsule: %w", errSeal) | ||
| } | ||
|
|
||
| inputTokens, outputTokens, totalTokens, cachedTokens := claudeCompactionResponsesUsage(summaryResp.Payload) | ||
| respPayload := helps.BuildAntigravityCompactionResponse(baseModel, capsule, inputTokens, outputTokens, totalTokens) |
There was a problem hiding this comment.
🔴 Critical · Reject truncated summaries before sealing compaction
An HTTP 200 summary response can contain nonempty text with stop_reason="max_tokens". This path seals that partial text and returns status="completed" without checking the stop reason. Reproducing with max_output_tokens=1 produced a successful capsule containing only "The". A client replacing its history with that capsule loses the conversation context. Both compaction entrypoints use this path. Also reported for this defect: - Reject truncated summaries before sealing compaction (internal/runtime/executor/claude_executor_compaction.go:63-69): A compaction request retaining max_output_tokens:1 sends max_tokens:1 to Claude. If Claude returns nonempty text with stop_reason:"max_tokens", this code seals that truncated prefix and reports the compaction as completed. Reproduced with a capsule containing only "The". A client replacing its history with the successful compaction result loses the conversation context. Check summary completion before sealing.
| @@ -217,6 +256,8 @@ func (e *ClaudeExecutor) countTokensUpstream(ctx context.Context, auth *cliproxy | |||
| if errMidSystem := validateClaudeMidSystemMessageModel(body, confirmedClaudeCode, directAnthropic); errMidSystem != nil { | |||
| return cliproxyexecutor.Response{}, errMidSystem | |||
| } | |||
There was a problem hiding this comment.
🟡 Medium · Validate the finalized count-token body
This finalizer runs after validateClaudeMidSystemMessageModel, so the guard does not inspect the body actually sent. For example, a payload rule that changes a cloaked request from a modern model to a legacy model leaves the inserted mid-conversation system turn unvalidated, and Anthropic returns an upstream 400. Read-only validation needs to run after the final payload rules.
| bodyForUpstream, touchedPayloadPaths = helps.ApplyPayloadConfigWithTrackedPaths( | ||
| e.cfg, baseModel, to.String(), from.String(), "", bodyForUpstream, stripTranslatedDefaultMaxTokens(from, originalPayload, originalTranslated), | ||
| helps.PayloadRequestedModel(opts, req.Model), helps.PayloadRequestPath(opts), opts.Headers, | ||
| "diagnostics", | ||
| ) |
There was a problem hiding this comment.
🟡 Medium · Keep response parsing aligned with the configured stream value
Moving payload rules after stream selection lets a stream:false override reach upstream while Execute still uses upstreamStream=true for Chat/Responses response translation. Anthropic then returns JSON, but validateClaudeStreamingResponse parses it as SSE and returns a 502 "upstream returned empty stream response". This was reproduced with a successful JSON upstream response. Preserve payload-rule authority while selecting the parser from the actual upstream response mode. Also reported for this defect: - Keep response parsing aligned with configured stream mode (internal/runtime/executor/claude_executor_execute.go:249-253): For Chat or Responses requests, Execute fixes upstreamStream=true before applying payload rules. Moving these rules after the stream-setting guard lets an override such as stream=false reach upstream, but headers and response parsing still use upstreamStream=true. In a reproduction, upstream returned a valid JSON Messages response and Execute rejected it with HTTP 502, "upstream returned empty stream response".
| keys := claudeOAuthToolAliasKeys(payload, messageID) | ||
| if len(keys) == 0 { | ||
| return | ||
| } | ||
| e.claudeOAuthToolAliasStore().save(keys, aliases) |
There was a problem hiding this comment.
🟡 Medium · Do not overwrite a parent message's alias state
The save keys include thread.previous_message_id as well as the newly returned message ID. If parent m1 declares Read and a continuation replaces its tools with Write, completing that continuation overwrites m1's alias map with Write's map. A subsequent branch from m1 without declarations then loads the wrong map and forwards Read's generated MCP name instead of restoring Read.
| body = helps.NewPayloadFinalizer(e.cfg, e.Identifier(), baseModel, to.String(), "", originalTranslatedForPayload, req, opts)(body) | ||
| body = stripPromptCacheOptions(body) |
There was a problem hiding this comment.
🟡 Medium · Lift token-count betas after payload finalization
countTokensUpstream extracts betas before this newly added finalizer. A payload rule that supplies betas therefore leaves the proxy-only field in the outgoing JSON while extraBetas retains the earlier value. For example, overriding betas with advisor-tool-2026-03-01 sends that array in the body without the corresponding header beta, causing an upstream 400 for the unsupported betas field. The Messages paths perform this transport extraction after payload rules. Also reported for this defect: - Extract count-token betas after payload rules (internal/runtime/executor/claude_executor_tokens.go:258): The count-token path extracts betas before this newly added payload finalizer. If a matching payload rule adds or overrides betas, the value remains as an unsupported JSON body field and is never copied to Anthropic-Beta, so Anthropic rejects the count request instead of enabling the configured beta. - Extract token-count betas after applying payload rules (internal/runtime/executor/claude_executor_tokens.go:259-260): countTokensUpstream extracts and removes betas at line 220, before this newly added finalizer. A payload rule setting betas therefore leaves the array in the outgoing JSON and omits its values from Anthropic-Beta. Direct Anthropic rejects the extra body field instead of counting tokens. The Messages paths correctly extract betas after their final barrier; token counting needs the same ordering.
Also affects:
| if errMidSystem := validateClaudeMidSystemMessageModel(body, confirmedClaudeCode, directAnthropic); errMidSystem != nil { | ||
| return cliproxyexecutor.Response{}, errMidSystem | ||
| } | ||
| body = helps.NewPayloadFinalizer(e.cfg, e.Identifier(), baseModel, to.String(), "", originalTranslatedForPayload, req, opts)(body) |
There was a problem hiding this comment.
🟡 Medium · Validate token-count model compatibility after payload rules
The new finalizer runs after validateClaudeMidSystemMessageModel, so that guard no longer checks the body being sent. A rule changing a modern model with a system turn to a legacy model bypasses local request-scoped rejection and sends a guaranteed upstream 400. Conversely, a rule filtering incompatible system turns cannot repair the request because validation returns before the rule executes; this premature rejection was reproduced. Run the read-only guard on the configured body.
| firstFunctionCallSeen = true | ||
| // A text part, even an empty signature carrier, ends the parallel group: | ||
| // the Gemini API rejects an unsigned functionCall after it. Thought parts do not. | ||
| if hasFunctionCall || !part.Get("thought").Bool() { |
There was a problem hiding this comment.
🟡 Medium · Reset Gemini function-call groups across server-tool parts
The new group-reset condition is bypassed by the earlier toolCall/toolResponse early return. For a model turn containing a signed functionCall, then a server-tool part, then another unsigned functionCall, the sanitizer leaves the second call unsigned, while the updated validator treats the non-thought server-tool part as a separator and requires a signature. The sanitized payload therefore remains invalid upstream.
| diagnosticsState = claudeDiagnosticsRequestState{} | ||
| } | ||
| extraBetas, bodyForUpstream = extractAndRemoveBetas(bodyForUpstream) | ||
| bodyForUpstream = stripPromptCacheOptions(bodyForUpstream) |
There was a problem hiding this comment.
🟡 Medium · Do not delete prompt-cache configuration after payload rules
This cleanup runs after ApplyPayloadConfigWithTrackedPaths, bypassing the required final semantic barrier. A payload rule setting prompt_cache_options.mode=explicit is applied too late to suppress the automatic cache breakpoints and is then deleted here, so the outgoing request uses automatic rather than explicitly configured placement. The streaming and token-count paths contain the same post-rule deletion.
|
Closing: the review scope changed. Eric (2026-10-27): "i don't want to fix anything that's an issue in upstream - our only goal here is to validate our changes". Upstream code is out of scope. Our fork's own changes on v8.0.44 get one review PR instead. Findings from this part are recorded in the HOUSE-79 adjudication record. None was a defect in our code. |
Review only. Do not merge. This PR is one part of the review of #13, the merge of upstream
v8.0.44into the fork. That PR's diff (966 files) is too large for one review round.c88d701f7bc0894cde517ca7f931f5c05749cd74). The base is that tree with only this part's files restored tomain(c265779f), so the diff is this part's files alone and the surrounding code is the real final code.go.sumare in no part's diff.Linear: HOUSE-79
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.