Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P1
|
Codex review: needs real behavior proof before merge. Reviewed September 27, 2026, 11:40 PM ET / September 28, 2026, 03:40 UTC. ClawSweeper reviewWhat this changesThe branch limits how long Keychain signature validation can delay refreshes and lets Claude reuse and persist a valid in-memory credential after cache cleanup. Merge readiness⛔ Blocked before merge - 7 items remain Keep this PR open. Its fixes remain useful and are absent from current main, but the new cache write can preserve a Claude credential after the user revokes consent to read it. Priority: P1 Review scores
Verification
How this fits togetherCodexBar checks Keychain access while refreshing provider usage. Claude credential selection then reads its cache, memory, or other permitted sources before requesting usage. flowchart LR
A[Provider refresh] --> B[Keychain preflight]
B --> C{Validation completes?}
C -->|Yes| D[Cache lookup]
C -->|Timeout| E[Inconclusive access]
D --> F[Claude memory recovery]
E --> F
F --> G[Cache write]
G --> H[Usage result]
Before merge
Findings
Agent review detailsSecurityNeeds attention: The new Claude cache write can outlive revocation of the consent that authorized its source credential. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Bind each in-memory Claude credential to its acquisition consent epoch and commit cache writes only while that epoch remains authorized; retain the bounded validation behavior. Do we have a high-confidence way to reproduce the issue? No live PR-path reproduction was run. Source establishes an interleaving: select the memory credential, revoke consent and clear caches, then let the introduced save persist that old credential. Is this the best way to solve the issue? No, not yet. Bounded validation and memory recovery fit the reported failures, but repersistence must honor the consent epoch at the final cache write. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 579f68406855. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A stalled native code-signature check can hold the cookie-cache locks and prevent the provider refresh cycle from finishing. Preflight now waits at most two seconds per validation, returns an inconclusive result on timeout, and limits unfinished native checks to four workers. Late results do not authorize reads. The disabled-access startup ordering discussed in #3249 was already corrected by #3258; this change addresses the later process trace of blocked native validation.
Claude also lost a valid credential after a cache write failed: the next refresh cleared the stale-cache marker but did not reconsider memory. It now reuses and persists a fresh, unexpired credential after cleanup succeeds, preserving its profile, owner, history binding, consent, and prompt-policy boundaries. Four identical delegated-refresh cache writes share one helper. Production delta against the starting base: 70 insertions, 74 deletions; net −4 lines.
The original expired-file/fresh-Keychain selection in #3390 is already covered by
79d146d8c273andd847ebcd9eb1(#3807); the existing 18-case freshness matrix passes. The remaining external-login discovery in #3395 and Claude-owned partition-ACL replacement in #3798 remain open. Their evidence and the credential precedence rule are documented; this patch does not enable additional foreign-Keychain reads.Validation used isolated macOS stores and synthetic credentials with real Keychain access suppressed. In one before/after invocation, the two regression bodies failed on the original production files at
491ae688a89a(2 tests, 3 expected issues), then the candidate passed 579 tests in 65 suites. The original initializer shape was used only to compile the unrelated coalescing test against the baseline.Baseline:
Test run with 2 tests in 2 suites failed ... with 3 issues.The rejected-write case returnednotFound; the stalled validator returned success after five seconds instead of timing out.Candidate:
Test run with 579 tests in 65 suites passed after 40.902 seconds.make checkpassed:0/2671 files require formattingandFound 0 violations, 0 serious in 2670 files. Independent Codex P2 review found no actionable findings. No app relaunch, account change, or live credential read was performed.Fixes #3249
Closes #3390
Refs #3395
Refs #3798