perf(keychain): memoize concurrent code-signature checks

[steipete]

Browser-cookie preflights repeatedly validate the invoking app's entire code signature for the same trusted-application/executable pair. This memo reuses those results and shares an in-flight result per key, so concurrent callers perform one validation while unrelated keys can progress independently.

The key includes the full trusted-application representation, executable path and filesystem identity, enclosing bundle path/version/directory identity, and the bundle's main executable path/identity. Info.plist is read directly so app updates invalidate entries without relying on Foundation's cached bundle metadata. The process-local memo holds at most 64 completed results, with 30-second success and five-minute confirmed-rejection lifetimes; transient errors are never cached. ACL/prompt-selector inspection and non-interactive secret-read policy are unchanged.

Rewritten from the investigation and proposal by @jeffloo886, with contributor credit preserved in the commit and changelog.

Validation:

• With memoization temporarily bypassed, swift test -j 2 --filter KeychainAccessValidationMemoTests failed with 13 expected issues: 100 serial requests performed 100 validations and 20 concurrent requests performed 20.
• With the implementation restored, swift test -j 2 --filter 'KeychainAccess|BrowserCookieAccessGate|KeychainPromptSafetyAudit|ProviderArchitectureGatekeeper|KeychainNoUIQuery|KeychainCacheApplicationPath' passed 101 tests in 10 suites. Counts were 100 serial requests → 1 validation and 20 concurrent requests → 1 validation, including proof that another key progresses while one is pending. Bundle/version/executable changes, expiry, capacity, distinct identities, missing metadata, and uncached transient statuses are covered.
• Test commands used CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1, CODEXBAR_TEST_CODEX_FILE_ISOLATION=1, and CODEXBAR_TEST_SESSION_FILE_ISOLATION=1, with CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS unset. Fixtures and preflights are synthetic; no app relaunch or live account probes were used.
• swift build -j 2 passed (debug).
• make check passed: 0 violations, 0 serious in 2549 files.
• Independent Codex autoreview passed with no actionable P0–P2 findings after fixing wholesale cache eviction; an added regression failed against that behavior and now passes. Overflow evicts one result, preserving other valid entries.

The call counts are synthetic validation-hook measurements; this PR does not claim a new end-to-end CPU measurement.

Fixes #3837
Closes #3838

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 22, 2026, 1:54 AM ET / 05:54 UTC.

ClawSweeper review

What this changes

The PR caches and coalesces application-signature checks used before Keychain reads, with bounded lifetimes, update-sensitive keys, tests, and documentation.

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains useful: main and the latest release still repeat signature validation. No discrete implementation defect was found, but the security-sensitive cache needs final-read boundary proof before merge.

Priority: P2
Reviewed head: dfccffeb1bef7ad457c5c7b1adb286fd54492596

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The implementation is focused and well covered synthetically, but the required authority-boundary evidence is absent.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: Authority-chain proof required: the current-head tests exercise the validation memo through an injected browser preflight, without reaching the native credential read. Show allowed access and rejection of mismatching or subsequently invalidated trust at that boundary, including background prompt suppression. The OWNER exemption still applies to unrelated performance proof; no stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: Authority-chain proof required: the current-head tests exercise the validation memo through an injected browser preflight, without reaching the native credential read. Show allowed access and rejection of mismatching or subsequently invalidated trust at that boundary, including background prompt suppression. The OWNER exemption still applies to unrelated performance proof; no stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	10 items	Policy and patch scope: Read the full root AGENTS.md and all six introduced file diffs. No nested AGENTS.md or maintainer-notes directory was found. Applied the no-prompt validation guidance; no builds, tests, or live Keychain operations were executed. Final git status was clean. Introduced implementation: The pinned base-to-head delta adds per-key in-flight sharing and bounded result caching. Validation runs outside the dictionary lock; missing identity metadata bypasses caching, and only success or confirmed signature rejection is retained. Still necessary on main and release: Inspected main at b99a916 and release v0.64.0: both directly invoke SecTrustedApplicationValidateWithPath on each call. Existing task-local preflight reuse does not provide this cross-call, per-signature coalescing.
Findings	None	None.
Security	Needs attention	Final-read safety during cached success remains unverified: A sealed-resource edit can leave the cache key unchanged and permit another secret-read attempt during the documented success lifetime. macOS authorization and no-UI controls remain, but supplied tests stop before that boundary and do not establish rejection or prompt suppression there.

How this fits together

CodexBar checks Keychain access before importing browser cookies or loading cached credentials. The changed cache reuses signature-validation results; live ACL inspection and downstream macOS authorization still control subsequent reads.

flowchart TD
 A[Cookie import or credential cache read] --> B[Read current Keychain ACL]
 B --> C[Signature validation cache]
 C --> D{Reusable result?}
 D -->|No| E[macOS signature validation]
 E --> F[Preflight decision]
 D -->|Yes| F
 F --> G[Skip or attempt protected Keychain read]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: Authority-chain proof required: the current-head tests exercise the validation memo through an injected browser preflight, without reaching the native credential read. Show allowed access and rejection of mismatching or subsequently invalidated trust at that boundary, including background prompt suppression. The OWNER exemption still applies to unrelated performance proof; no stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve security concern: Final-read safety during cached success remains unverified - A sealed-resource edit can leave the cache key unchanged and permit another secret-read attempt during the documented success lifetime. macOS authorization and no-UI controls remain, but supplied tests stop before that boundary and do not establish rejection or prompt suppression there.
• Resolve merge risk (P1) - Final-read behavior remains unproven when a cached success outlives a sealed-resource change: demonstrate that an unauthorized or revoked identity cannot obtain credentials or trigger background prompts through the production read boundary.
• Complete next step (P2) - Provide the scoped final-read proof before merge using isolated credentials and redacted terminal output or logs; screenshots or recordings may supplement visible prompt behavior. Remove secrets and private identifiers before posting. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review.

Findings

• [medium] Final-read safety during cached success remains unverified — Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:54

Agent review details

Security

Needs attention: No credential-disclosure bypass was established, but retained preflight success requires evidence that final authorization and prompt suppression remain effective.

Review metrics

Metric	Value	Why it matters
Production and test growth	production +172/-1; tests +258/-1	Production growth implements bounded coalescing and update-sensitive identity checks, with focused concurrency and invalidation coverage.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3837
Summary: This PR is an explicit candidate fix for the repeated signature-validation issue and overlaps the earlier open proposal.

Members:

• canonical: Background Chromium cookie gate re-runs a full app-bundle code-signature validation on every refresh: ~11.6% of a core at idle, 190-217% CPU bursts every 60s #3837 - The issue documents the measured problem and is explicitly targeted by this PR.
• same_root_cause: fix(keychain): keep completed signature validations fresh #3838 - The earlier open proposal caches the same validation chokepoint; this rewrite adds concurrent coalescing and broader update invalidation.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. Verify final-read enforcement (recommended)
   Provide isolated macOS production-path evidence for allowed access, a mismatching signing requirement, and invalidation during the success-cache window, including credential rejection and background prompt suppression.

Technical review

Best possible solution:

Retain the bounded shared cache with demonstrated authorization and prompt suppression at the final credential-read boundary.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: repeated browser preflights on current main invoke the native signature validator again; the linked report supplies a concrete macOS profiling path. This review did not execute that path.

Is this the best way to solve the issue?

Yes, the shared validation chokepoint is a focused solution and preserves live ACL inspection; confidence in its security-sensitive final-read behavior still requires targeted proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b99a91694d78.

Labels

Label changes:

• add P2: This addresses measured background CPU overhead in a bounded Keychain preflight path.
• add merge-risk: 🚨 security-boundary: The owner-selected 30-second freshness window retains preflight success across some resource changes; final credential-read enforcement during that window still needs proof.
• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: the current-head tests exercise the validation memo through an injected browser preflight, without reaching the native credential read. Show allowed access and rejection of mismatching or subsequently invalidated trust at that boundary, including background prompt suppression. The OWNER exemption still applies to unrelated performance proof; no stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

• P2: This addresses measured background CPU overhead in a bounded Keychain preflight path.
• merge-risk: 🚨 security-boundary: The owner-selected 30-second freshness window retains preflight success across some resource changes; final credential-read enforcement during that window still needs proof.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: the current-head tests exercise the validation memo through an injected browser preflight, without reaching the native credential read. Show allowed access and rejection of mismatching or subsequently invalidated trust at that boundary, including background prompt suppression. The OWNER exemption still applies to unrelated performance proof; no stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

• [medium] Final-read safety during cached success remains unverified — Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:54
  A sealed-resource edit can leave the cache key unchanged and permit another secret-read attempt during the documented success lifetime. macOS authorization and no-UI controls remain, but supplied tests stop before that boundary and do not establish rejection or prompt suppression there.
  Confidence: 0.94

What I checked:

• Policy and patch scope: Read the full root AGENTS.md and all six introduced file diffs. No nested AGENTS.md or maintainer-notes directory was found. Applied the no-prompt validation guidance; no builds, tests, or live Keychain operations were executed. Final git status was clean. (AGENTS.md:1, dfccffeb1bef)
• Introduced implementation: The pinned base-to-head delta adds per-key in-flight sharing and bounded result caching. Validation runs outside the dictionary lock; missing identity metadata bypasses caching, and only success or confirmed signature rejection is retained. (Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:46, dfccffeb1bef)
• Still necessary on main and release: Inspected main at b99a916 and release v0.64.0: both directly invoke SecTrustedApplicationValidateWithPath on each call. Existing task-local preflight reuse does not provide this cross-call, per-signature coalescing. (Sources/CodexBarCore/KeychainAccessPreflight.swift:410, 3703a72b6b66)
• Owner-selected freshness tradeoff: The OWNER-authored patch explicitly documents that other sealed-resource changes are detected when the 30-second success lifetime expires. This deliberately retains bounded staleness; it is not being raised again as an unaccepted design choice. That choice does not establish final-read safety. (docs/keychain-prompts.md:41, dfccffeb1bef)
• Authority reaches the credential-read boundary: An allowed preflight lets KeychainCacheStore.load issue its secret-data query. Its existing comment records that legacy prompts have occurred despite UI-fail flags. Browser imports separately retain the non-interactive SweetCookieKit scope. Thus stale preflight success is observable before final I/O, while unauthorized disclosure is not established. (Sources/CodexBarCore/KeychainCacheStore.swift:121, dfccffeb1bef)
• Authoritative signature contract: Dependency inspection applies because the changed production function calls Security.framework's trusted-application validator. Apple's TrustedApplication.verifyToDisk calls SecStaticCodeCheckValidity with default flags; its documented result covers sealed resources and remains valid only while the code is unchanged. (OSX/libsecurity_codesigning/lib/SecStaticCode.h:127, db15acbe6a7f)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• ProspectOre: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add redacted final-read evidence for an allowed identity, a mismatching requirement, and trust invalidated while success remains cached, including absence of unauthorized credential access and background prompts.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.