Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 12 of 13: everything else - #24
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 · Preserve refreshes from the model-fetch command 🔴 High · Retain a working discovery receive path on Windows 🟡 Medium · Atomic saves sever credential symlinks 🟡 Medium · Quota-cooled model hidden when another credential has a non-quota suspension 🟡 Medium · Explicit login cannot replace a corrupt credential file 🟡 Medium · ToolSearch escapes registry clone isolation 🔵 Low · Use suffixed errors instead of shadowing the parser error 🔵 Low · Check deferred probe socket close errors 🔵 Low · Check deferred credential-temp cleanup errors WalkthroughThis PR is part 12 of 13 of the review-only split 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 · Preserve refreshes from the model-fetch command
🔴 High · Retain a working discovery receive path on Windows
🟡 Medium · Atomic saves sever credential symlinks
🟡 Medium · Quota-cooled model hidden when another credential has a non-quota suspension
🟡 Medium · Explicit login cannot replace a corrupt credential file
🟡 Medium · ToolSearch escapes registry clone isolation
🔵 Low · Use suffixed errors instead of shadowing the parser error
🔵 Low · Check deferred probe socket close errors
🔵 Low · Check deferred credential-temp cleanup errors
Check
| cleanupTmp = false | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Atomic saves sever credential symlinks
For a credential accessed through a file symlink, canonicalLockPath locks the resolved target, but this rename replaces the symlink itself. Saving or refreshing then creates a separate credential copy while leaving the original target stale; other processes using that target continue using the old tokens. The previous os.Create-based save followed the symlink. Also reported for this defect: - Keep symlinked credentials on their canonical target (internal/auth/codex/token.go:94-96): FileTokenStore.List and resolveCodexAuthDiskPath accept and follow symlinked credential files, and WithFileLock locks their canonical target. This rename instead replaces the symlink itself. After a save or refresh, CPA and consumers of the original target have separate credential files, with the original retaining stale tokens; subsequent saves also stop sharing the same lock. This breaks previously working shared-credential setups.
Also affects:
| // A quota cooldown can mark the same client both quota-exceeded and | ||
| // suspended. Count that unavailable client only once. | ||
| effectiveClients := availableClients - expiredClients - otherSuspended + quotaAndOtherSuspended | ||
| effectiveClients := availableClients - expiredClients - cooldownSuspended - otherSuspended + quotaAndSuspended |
There was a problem hiding this comment.
🟡 Medium · Quota-cooled model hidden when another credential has a non-quota suspension
The new effectiveClients now subtracts cooldownSuspended, but the fallback available clause still requires otherSuspended == 0. The same change appears in GetAvailableModelsByProvider at line 1588. Scenario: - A model is served by credentials A and B. - A is suspended with reason "quota" and its 5-minute QuotaExceeded window has lapsed (long Claude/Codex cooldowns). - B is suspended for a non-quota reason, such as unauthorized. Old code: effective = 2-0-1 = 1, so the model was listed. New code: effective = 2-0-1-1+0 = 0, and otherSuspended == 1 fails the fallback clause, so the model is hidden. With A alone, the model stays listed. Adding a broken credential B therefore removes it. This contradicts the stated isQuotaSuspension intent that quota-cooled models stay listed and get a 429 instead of looking unknown. Consequence: GetAvailableModels (/v1/models and the Codex client catalog), GetAvailableModelInfos and GetAvailableModelsByProvider drop the model, while requests still route to it.
| if err := conn4.SetControlMessage(ipv4.FlagInterface|ipv4.FlagDst, true); err != nil { | ||
| err4 = err | ||
| if errClose := conn4.Close(); errClose != nil { | ||
| log.WithError(errClose).Debug("discovery: close unusable IPv4 browse socket") | ||
| } | ||
| conn4 = nil |
There was a problem hiding this comment.
🔴 High · Retain a working discovery receive path on Windows
In the pinned golang.org/x/net version, both ipv4.SetControlMessage and ipv6.SetControlMessage unconditionally return errNotImplemented on Windows. These new checks therefore close both successfully opened sockets, making every Windows browse fail with "open mDNS sockets" before sending a query. The previous zeroconf browser did not make unsupported control-message metadata fatal. Windows needs a supported receive-isolation implementation rather than unconditional socket rejection.
| } | ||
|
|
||
| if answerTypeEnum { | ||
| if err := builder.PTRResource(dnsmessage.ResourceHeader{ |
There was a problem hiding this comment.
🔵 Low · Use suffixed errors instead of shadowing the parser error
This new err binding shadows the parser error in buildServiceQueryReplyFiltered, violating AGENTS.md's explicit suffixed-variable rule. The identical shadowing pattern also occurs at lines 797, 807, 817, 825, and 841; these local error bindings need distinct suffixed names. Also reported for this defect: - Use operation-specific names for shadowed reply errors (internal/discovery/zeroconf.go:787-792): This declaration shadows the parser's existing err variable in buildServiceQueryReplyFiltered, violating AGENTS.md's mandatory operation-suffixed naming rule for shadowed variables. The same pattern occurs at lines 797, 807, 817, 825, and 841. Use operation-specific error names consistently at these sites.
Also affects:
| var conns []probeConn | ||
| defer func() { | ||
| for _, c := range conns { | ||
| _ = c.conn.Close() |
There was a problem hiding this comment.
🔵 Low · Check deferred probe socket close errors
The new deferred cleanup discards every socket Close error, contrary to AGENTS.md's requirement to check and log deferred errors. Unexpected cleanup failures therefore have no diagnostic. Expected already-closed errors can be distinguished from other failures. Also reported for this defect: - Check errors from deferred probe cleanup (internal/discovery/zeroconf.go:2183): The new deferred cleanup discards socket Close errors, contrary to AGENTS.md's mandatory requirement to check and log deferred cleanup errors. A cleanup failure is therefore silently suppressed. Check the returned error and log unexpected failures, excluding an expected already-closed socket where appropriate.
| if errUnmarshal := json.Unmarshal(existingBytes, &diskMap); errUnmarshal != nil || diskMap == nil { | ||
| return fmt.Errorf("invalid existing credential JSON") |
There was a problem hiding this comment.
🟡 Medium · Explicit login cannot replace a corrupt credential file
If an existing Codex credential file is empty or contains truncated JSON, saving a successful new login returns "invalid existing credential JSON" even when ReplaceCredentials is true. The validation runs before the explicit-replacement branch, so reauthentication cannot repair the file without manual deletion.
| cleanupTmp := true | ||
| defer func() { | ||
| if cleanupTmp { | ||
| _ = os.Remove(tmpPath) |
There was a problem hiding this comment.
🔵 Low · Check deferred credential-temp cleanup errors
After an unsuccessful atomic write, a failing os.Remove silently leaves the credential temporary file behind. AGENTS.md requires checking and logging deferred errors; assigning the removal error to the blank identifier violates that mandate.
| merged[key] = value | ||
| } else { | ||
| delete(merged, key) |
There was a problem hiding this comment.
🔴 Critical · Preserve refreshes from the model-fetch command
When cmd/fetch_codex_models.ensureAccessToken refreshes an expired credential, it updates Auth.Metadata and calls FileTokenStore.Save without a CodexRefreshPatch. That save reaches this merge, which replaces the newly rotated tokens with the old disk credentials and returns success. The new refresh token is consequently discarded while the persisted refresh token has already been consumed, causing subsequent refreshes to fail. The existing refresh writer needs to participate in the new versioned-patch contract.
| // ToolSearch reports whether every credential behind a Home route can take | ||
| // Codex tool_search as Anthropic native tool search. Home computes it per | ||
| // route; nil means unknown and is treated as unsupported. | ||
| ToolSearch *bool `json:"tool_search,omitempty"` |
There was a problem hiding this comment.
🟡 Medium · ToolSearch escapes registry clone isolation
The new ToolSearch pointer is shallow-copied by cloneModelInfo, which deep-copies only WebSearch. After RegisterClient, changing the original flag or a flag returned by GetModelsForClient also changes registry-owned metadata without a lock, publication, or generation update. This reproduces with a registered ToolSearch=true model and an output snapshot changed to false.
|
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.