fix: persist Command Code browser sessions - #2564
Conversation
|
Exact test proof for commit
All credential persistence coverage used |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e303b80e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try await KeychainCacheStore.withServiceOverrideForTesting(service) { | ||
| try await KeychainCacheStore.withImplicitTestStoreForTesting { |
There was a problem hiding this comment.
When these new cache-aware tests run on a machine that already has a legacy commandcode-cookie.json, overriding only the Keychain service is not enough: CookieHeaderCache.load(provider:) falls back to the default Application Support legacy file before sessionLoader is used, then migrates/removes that real entry. That makes the recorder expectations depend on developer machine state and can touch real credentials; wrap these blocks in CookieHeaderCache.withLegacyBaseURLOverrideForTesting with a temp directory as well.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Codex review: needs changes before merge. Reviewed August 2, 2026, 8:21 AM ET / 12:21 UTC. ClawSweeper reviewWhat this changesThis PR makes Command Code reuse a validated cached browser session and persist a newly validated browser session so CLI refreshes and later local-service usage can authenticate. Merge readinessKeep this owner-authored PR open for explicit maintainer handling. The provider fix is focused, but the existing P2 blocker remains: its new cache-aware tests isolate the Keychain store but not the legacy cookie-file fallback, so they can read, migrate, or remove a developer’s real Command Code credential state. Priority: P2 Review scores
Verification
How this fits togetherCommand Code’s web fetch strategy receives either a manual cookie, a shared cached cookie, or an imported browser session, then validates it against the usage endpoint. A successful browser session is written through the shared cookie cache so CLI refresh and subsequent provider requests can reuse it. flowchart LR
A[Manual cookie or browser session] --> B[Command Code fetch]
C[Shared cookie cache] --> B
B --> D[Usage validation]
D -->|valid browser session| E[Stage or persist cache entry]
E --> C
D --> F[CLI and local service usage]
Before merge
Findings
Agent review detailsSecurityNeeds attention: The runtime change has no new supply-chain path, but its new credential-cache tests can access and mutate real legacy browser-session material without full test isolation. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the runtime persistence design, but wrap every new cache-aware Command Code test in a unique temporary legacy-cache base as well as the existing isolated Keychain store, then rerun the focused provider and CLI refresh suites. Do we have a high-confidence way to reproduce the issue? Yes, at source level: a Keychain test-store miss reaches the legacy-file migration path, while the new tests omit the available legacy-base override. The linked report also provides a concrete real-user CLI refresh and subsequent usage/service reproduction path. Is this the best way to solve the issue? No, not yet. The provider persistence approach matches existing shared-cache patterns, but the tests must isolate the legacy cookie cache before they safely prove the repair. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 41bc0141e5f1. LabelsLabel justifications:
EvidenceSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
* Port upstream 0.47.0: Low Power Mode (steipete#2518) * Port upstream 0.47.0: Notion AI provider (steipete#2552) * Port upstream 0.47.0: codexbar hooks watch (steipete#2536) * Port upstream 0.47.0: Cursor optional on-demand usage (steipete#2338) * Port upstream 0.47.0: Command Code persist browser sessions (steipete#2564) * Port upstream 0.47.0: OpenCode Go idle WAL read (steipete#2544) * Port upstream 0.47.0: real-calendar monthly pace (steipete#2552) * Port upstream 0.47.0: XAI provider * Port upstream 0.47.0: verified z.ai/Kimi/Grok window durations (steipete#2431) * Document upstream port procedure * Simplify port helpers: drop bespoke trait, unused param, duplicated aggregation
1 participant
Summary
Root cause
The Command Code automatic strategy imported and validated browser cookies directly, but never participated in
CookieHeaderCache. The CLI cookie-refresh transaction hides the existing cache while validation runs and requires exactly one staged cache replacement before it reports success. Command Code therefore validated successfully but always committed zero staged mutations, producing the reported "could not be saved" result and leaving CLI/service consumers without credentials.Fix
The automatic strategy now checks the shared cache before browser import, evicts a cached entry only after an explicit invalid-credentials response, and stores the first browser session that successfully validates. During explicit cookie refresh, that store is staged by the existing suppression gate and committed only after provider validation succeeds.
Proof
swift test --filter CommandCodeProviderTests— passed, 6 testsswift test --filter CLICookieRefreshTests— passed, 12 testsmake check— passedmake test— all completed groups passed except the documented loaded-machine wall-clock flake inKiroStatusProbeTests; an isolated rerun reproduced onlyaccepted pipe output cannot overrun the usage deadlineCloses #2541