fix: harden provider account credential lifecycle - #432
Conversation
…safety-current-main
JasonYeYuhe
left a comment
JasonYeYuhe
left a comment
There was a problem hiding this comment.
Reviewed the full diff, the tests, CI, and ran the suite locally. The engineering here is strong — the transaction design is right, the failure modes are real ones, and nearly every structural risk I went looking for was already handled and had a dedicated test waiting for me. Details below.
One finding needs fixing before merge. It is not visible in CI, and it is caused by the isolation work rather than by the transaction work.
Blocking: test: isolate Claude helper state from XCTest wedges the offline suite on a developer Mac
swift test --package-path "CLI Pulse Bar/CLIPulseCore" on this branch hangs indefinitely on a machine that has Claude Code installed. Observed here: the run stopped at test 312 of ~2,715 and sat wedged for 17 hours until I killed it. SecurityAgent came up in the same minute as the xctest process and stayed up.
Hung test: ClaudeCollectorTests.testIsAvailableWithCLIBinary — untouched by this PR.
Mechanism
ClaudeCredentials.resolveTokenDetails tries sources in order (ClaudeSourceStrategy.swift):
4. env vars — absent
5. readCredentialsFile() — reads realHomeDir + "/.claude/.credentials.json"
6. readKeychainCredentials() — cross-app login-Keychain read
This PR adds an XCTest guard at line 249, so realHomeDir now returns a per-PID temp directory that is never created. But readKeychainCredentials() / SecItemCopyMatching at line 332 has no such guard.
So the isolation is half applied, and the half that landed makes things worse:
| step 5 | step 6 | |
|---|---|---|
| before | real ~/.claude/.credentials.json found → returns |
never reached |
| after | temp dir, file absent → falls through | cross-app Keychain read |
The function's own comment two lines above the call says it: "The sandboxed app cannot access cross-app keychain items without triggering a macOS authorization dialog." Removing the file that used to satisfy the lookup first is what routes execution onto that dialog.
Verified on this machine: ~/.claude/.credentials.json exists (509 bytes), and readCredentialsFile() reads it via realHomeDir.
Why CI is green
A headless runner has neither the credentials file nor a Claude Code Keychain item, so both step 5 and step 6 return nil immediately. The hang needs a real developer machine with Claude Code signed in — which is every machine that will actually run this suite locally.
This is the failure shape this repo keeps paying for: the check is green and the real environment is wedged. It also means the PR body's "CLIPulseCore offline suite: 2,715 tests, 4 skipped, 0 failures" is a CI result, not a local one.
Suggested fix
Guard the Keychain path the same way ClaudeHelperContract.appGroupHelperDir is already guarded in this PR:
public static func readKeychainCredentials(...) -> Creds? {
if isolatedTestHomeDirectory != nil { return nil }
...
}Worth adding a test that asserts the Keychain path is not consulted under XCTest — otherwise this reopens silently.
What I checked that turned out fine
Four things looked risky on the diff and are all correct — I am recording them so nobody re-derives them later:
| concern | finding |
|---|---|
save() holds the persistence lock and re-enters withMutationLock → deadlock? |
No. GeminiCredentialMutationLock is NSRecursiveLock + recursionDepth, and skips the file lock when depth > 0. The inner plain NSLock is never nested — the code inside it uses the unlocked private helpers. Covered by testAppSaveTransactionLockAllowsReentrantOwnerMutation. |
outbox Intent goes into a Set and enqueue defaults a fresh generation UUID → duplicate accumulation? |
No. enqueue filters same-(owner, accountID) records before inserting. Intent: Hashable includes generation, so markCompleted matches exactly — which is what makes a stale completion unable to clear a newer intent. |
commitProviderCredential has no paired rollback in the transaction |
Fine — the stated contract is that each primitive compensates itself, and commitAuthorization already carries epoch + operationID write markers and rollbackCredentialWrite. |
attribution count == 1 ? … : nil → multi-account users lose helper data? |
Only the per-account split is dropped; provider-level usage survives (providerResults.count == 1 is asserted). Graceful degradation, not data loss. |
release() returning true for kinds without a shared source is easy to read as a weakened check, but it is load-bearing: the same commit turns the call site into a guard, so without it every non-Claude/Gemini account deletion would fail. testReleaseForProviderWithoutSharedSourceIsSuccessfulNoOp pins it.
Tests are genuine rather than tautological — event-ordering assertions, and .failedRollbackIncomplete asserted distinctly from .failedRolledBack. The QARuntimeSideEffectPolicyTests change tightens the guard (deletedKeys.count == 2 → exact 4-key list). No pbxproj wiring risk: both new files are inside the SPM package.
The two red SwiftLint checks are genuinely pre-existing — same check fails on merged #425 and #427 (3,115 violations across 326 files repo-wide). Unrelated to this PR, but a permanently-red "warning-only" check can never report a new problem, which makes it as uninformative as a permanently-green one. Worth baselining separately.
I also independently confirmed the isolation claim that did land: nothing under ~/Library/Group Containers/group.yyh.CLI-Pulse/ was written during the run.
Non-blocking
- Ambiguous attribution empties the per-account view with no signal. Provider totals survive, so the user sees numbers — but their per-account breakdown vanishes with no explanation. A log line, or a hint in the UI, would keep this from reading as a bug.
- XCTest detection is inconsistent.
CookieResolveruses seven signals (XCTestConfigurationFilePath,XCTestBundlePath, bundle path, process name…);ClaudeCredentialsuses one (NSClassFromString("XCTestCase")). If seven are warranted in one place, one is a gap in the other — and the blocking issue above lives in exactly that gap. Suggest a single shared predicate. isolatedTestHomeDirectoryis per-PID, not per-test, so everything in one run shares an isolated home. Probably intentional, worth confirming.
Agreed on holding the signed-in provider-account smoke test before merge — with the Keychain guard added first, so the suite can actually finish locally.
Review prepared with Claude Code; findings verified against the code and reproduced locally.
|
Implemented the blocking XCTest isolation fix in
The three non-blocking suggestions are intentionally not mixed into this blocker-only fix. The signed-in provider-account smoke test remains pending before merge. This PR remains Draft; no merge, release, or deployment is requested. @JasonYeYuhe please re-review the blocking item when convenient. |
JasonYeYuhe
left a comment
JasonYeYuhe
left a comment
There was a problem hiding this comment.
0b8a62ca resolves my blocking finding. Verified locally, not just read — the guard is in the right place (ahead of the app-cache lookup, so it covers both the cache and the cross-app item), and the test is a real negative control: it seeds the app's own cache with a valid credential and asserts nil, so removing the guard fails the test rather than silently passing.
before 0b8a62ca |
after | |
|---|---|---|
| full offline suite | wedged 17 hours at test 312/2716, SecurityAgent up |
2,716 tests, 4 skipped, 0 failures, 12.669 s |
testIsAvailableWithCLIBinary |
hung indefinitely | passes in 0.139 s |
testKeychainCredentialReadIsDisabledUnderXCTest |
— | passes |
Two consecutive runs, both clean. Making keychainCacheKey internal rather than duplicating the string in the test is the right call.
One more isolation gap — in scope for this PR, but not introduced by it
To be clear up front: this is pre-existing on main and your diff does not touch the line responsible. I'm raising it here only because it sits directly under this PR's own acceptance criterion — "Offline tests must use isolated temporary state and leave the real App Group files unchanged."
Running the offline suite rewrites the developer's real shared App Group UserDefaults.
Measured on my machine with neither the app nor the helper running, so nothing else could be the writer:
~/Library/Preferences/group.yyh.CLI-Pulse.plist
sha1 970c2857… -> a456a708…
mtime 17:27:54 -> 17:28:34 (exactly the swift test run)
cli_pulse_provider_shared_credential_owner_Claude 82E0429E… -> D382AA80…
cli_pulse_provider_shared_credential_owner_Gemini B7069911… -> BAE20755…
Those new UUIDs are test-fixture account IDs that have overwritten the real owner records.
Cause. ProviderSharedCredentialOwner.defaults defaults to the production suite:
static var defaults: UserDefaults? = UserDefaults(suiteName: HelperIPC.suiteName) // "group.yyh.CLI-Pulse"and resolveTokenDetails calls ProviderSharedCredentialOwner.claim(...) before env, file, or Keychain. So merely resolving a Claude token under XCTest claims ownership in the developer's real shared defaults. It is injectable for focused tests, but the default path is live — the same shape as the realHomeDir gap, one layer over.
Why the PR's validation didn't catch it. "Real App Group snapshot/account/session checksums were unchanged" covers the files in the container (claude_snapshot.json, claude_account.json, …) — and that part I independently confirmed is true. The UserDefaults suite is App Group state too, and it lives in ~/Library/Preferences/, outside the checksum set.
Impact is transient but not nil. reconcile(configs:) repairs it: when the stored owner is not in eligibleIDs it reassigns to eligible.first or clears it, so the next app refresh heals the record. On a machine where the app isn't relaunched, it stays wrong, and Claude/Gemini shared-credential resolution can fail closed in the meantime.
Suggested fix, mirroring what you already did for realHomeDir:
static var defaults: UserDefaults? = {
if isRunningUnderXCTest {
return UserDefaults(suiteName: "clipulse-xctest-\(ProcessInfo.processInfo.processIdentifier)")
}
return UserDefaults(suiteName: HelperIPC.suiteName)
}()with a test asserting the production suite is untouched after a claim().
Your call whether that belongs in this PR or a follow-up — it is genuinely not your regression, and this PR is already large.
This also sharpens my earlier point about the XCTest predicate: there are now three independent isolation points (CookieResolver, ClaudeCredentials, and this one), using two different definitions of "am I under test". A single shared predicate would have made this third one obvious — the gap keeps appearing in whichever spot the current definition doesn't reach.
Everything else from my previous review stands unchanged; 3d4940bc is still an ancestor, so nothing was rewritten.
Verified with Claude Code: two full local suite runs plus a controlled before/after on the shared defaults with no app or helper process running.
JasonYeYuhe
left a comment
JasonYeYuhe
left a comment
There was a problem hiding this comment.
Ran the real-Keychain smoke we'd both flagged as the remaining gap. It found a regression introduced by this PR, with a reproduction. Recommend not merging until it's addressed.
The bug: an unreadable credential becomes a permanent brick
makeSecretPersistenceCheckpoint returns nil if any of the four reads comes back .failure, and readResult classifies every OSStatus that isn't errSecSuccess/errSecItemNotFound as .failure. Both saveSecrets(using:) and deleteSecrets(using:) now guard on that checkpoint before doing anything.
So one unreadable item means the account can be neither overwritten nor deleted.
This is new. Before this PR neither function read first:
// pre-PR saveSecrets // pre-PR deleteSecrets
persistSecret(apiKey, …) persistSecret(nil, "apiKey", …)
persistSecret(manualCookie…, …) persistSecret(nil, "cookie", …)Writing a fresh value over an unreadable item used to succeed (the read-back verifies the new value, which is fine), and deleting used to succeed. Now both fail closed, permanently — fail-closed has turned into fail-forever, with no path back for the user.
Reproduction (real login Keychain, not the in-memory double)
I planted a non-UTF8 payload under a throwaway account key via SecItemAdd, then drove the real code:
PASS planted a non-UTF8 credential item (OSStatus 0)
PASS readResult classifies it as .failure (not .missing)
PASS checkpoint returns nil on the corrupt item
saveSecrets -> false
deleteForRemoval -> false
Not editable, not removable. The only escape is Keychain Access.app.
How a real user gets there
Non-UTF8 is my synthetic trigger; it is not the likely one. The likely ones are anything that makes SecItemCopyMatching return a non-success status:
- ACL mismatch after a re-sign or a channel switch. An item written by the MAS build read from the Developer ID build (or vice versa) prompts; the user clicking Deny yields
errSecAuthFailed→.failure. errSecUserCanceled— the prompt is dismissed rather than answered.errSecMissingEntitlement— access group not present in the running build's entitlements.
One thing I want to explicitly retract on Gemini's behalf: it proposed "screen locks mid-save → errSecInteractionNotAllowed" as a trigger. I checked, and that one is weak here — kSecUseDataProtectionKeychain is never set in KeychainHelper, so these queries hit the legacy file-based login keychain, which is no-timeout and survives screen lock. (Same distinction CLAUDE.md records for codesign vs notarytool.) Don't fix for that scenario.
Second-order: the recovery gate can latch permanently
Same root cause, one layer up. If a save fails and rollback is incomplete, retainProviderAccountSaveRecovery stores a recovery, and recoverPendingProviderAccountSave then blocks every future save for that account until it succeeds. But restoreSecrets verifies each write with read(...) == .value(v), which cannot succeed against an unreadable item — so the editor sits on "Could not restore the previous provider configuration. Please retry." forever. (Gemini flagged this; it follows directly from the confirmed bug above.)
Suggested direction
Separate "I could not read the old value" from "I must not proceed."
- Delete must never need a checkpoint. There is nothing to roll back to — the whole point is removal. Drop the guard from the delete path.
- Save should degrade, not refuse. An unreadable prior value means there is nothing worth preserving; proceed with the write and mark the transaction as rollback-unavailable rather than aborting.
.failedRollbackIncompletealready exists to express exactly that.
What the same run confirmed is fine
Worth recording so nobody re-litigates it:
| check | result |
|---|---|
| write immediately readable in-process; overwrite visible | PASS |
delete of a non-existent key returns true |
PASS — so deleteAndConfirmMissing is sound |
| checkpoint → overwrite → restore returns the original apiKey and cookie | PASS |
| restoring a checkpoint whose entry was absent deletes it (no stale credential) | PASS |
| sibling account survives removal of the legacy-migration owner | PASS |
That last row settles the sibling question from my first review — I seeded the shared cli_pulse_provider_Synthetic_* legacy slots, removed the legacySecretMigrationEligible == true owner, and the sibling's apiKey and cookie were both intact and still loadable. Your account-scoped key design holds. Gemini independently reached the same conclusion by reading loadSecret.
Caveat on my harness: it resolved to access group group.yyh.CLI-Pulse.quarantine (unrecognised bundle id → quarantine runtime), so it exercised real SecItem semantics but not the production access group's ACLs. The ACL-mismatch trigger above is therefore reasoned from code, not reproduced.
The harness is a throwaway .executableTarget I can hand over if you want it — it is deliberately gated: it inventories the provider-scoped namespace first and refuses to run unless the probe kind is empty, and it cleans up every key it creates.
Findings from Gemini 3.7 Flash (via agy) plus my own real-Keychain harness; every claim above was re-verified against the code, and one of Gemini's was retracted. A Codex pass is still running and I'll add anything it turns up.
|
A Codex pass (GPT-5.6, high→medium effort) on the same code, given only the inlined functions so it couldn't wander. It independently agreed with the brick finding — "Correct. A non-UTF-8 item becomes Already fixed by #435Any read authorization error produces the same lockout. One honest limit: #435 removes the checkpoint blocker, not the verification blocker. If reads fail persistently for a key, Still open — not addressed by #4351. 2. "Restored but unverifiable" is indistinguishable from "not restored." 3. The checkpoint is four independent reads, not a snapshot. Concurrent mutation could make a later rollback restore a combination that never coexisted. Largely mitigated in practice — the editor holds 4. A valid-but-wrong access group fails silently. This is the one I'd flag hardest, because it's the opposite failure mode from everything above: if the queried group is accessible but is not the group holding the items, reads return I hit this for real without meaning to. My harness resolved to One Codex left undecided, which I can closeIt declined to call Process note, since it cuts against the numbers: Codex needed four attempts. Three died at the harness time limit (exit 144) with partial output, which I twice mis-read as the model flailing. It wasn't — a Gemini 3.7 Flash and Codex both reviewed independently; every claim above was checked against the code before being repeated, and the two I could not substantiate were dropped rather than passed along. |
`readResult` maps every OSStatus other than success/itemNotFound to `.failure`,
and `makeSecretPersistenceCheckpoint` returned nil if ANY of its four reads came
back that way. Both `saveSecrets` and `deleteSecrets` guarded on that checkpoint,
so a single unreadable entry left the account neither editable nor removable —
fail-closed had become fail-forever, with no route back for the user short of
Keychain Access.app.
Reproduced against the real login Keychain by planting a non-UTF8 payload:
readResult -> .failure
makeSecretPersistence… -> nil
saveSecrets -> false
deleteSecretsForAccount…-> false
This is a regression rather than a pre-existing gap: before the save transaction
landed, neither function read anything first, so overwriting an unreadable item
succeeded and so did deleting it.
The checkpoint now keeps the full `ProviderSecretReadResult` per entry instead of
collapsing to `String?`, so "unreadable" is recorded rather than fatal, and
`restoreSecrets` skips those entries. Skipping is the only honest option: writing
would invent a value we never saw and deleting would destroy one. It does not
count against the rollback, because we end up no worse off than before the
attempt. Readable entries are still rolled back exactly as before.
Same repro after the change: checkpoint is produced, save succeeds and the
replacement value is what is stored, and an account holding a corrupt credential
can be deleted.
`makeSecretPersistenceCheckpoint` keeps its optional return type on purpose, so
every existing `guard let` call site compiles unchanged and simply stops
tripping. The blast radius is one type and one function.
`testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails` pinned the promise
that caused this ("if any entry cannot be read, refuse to write at all") and is
replaced by two tests that pin the corrected contract. Both were negative-
controlled: with the guard restored they fail, with it removed they pass.
2,717 tests, 4 skipped, 0 failures.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
JasonYeYuhe
left a comment
JasonYeYuhe
left a comment
There was a problem hiding this comment.
Approving: the blocking regression I raised is fixed and verified, so my CHANGES_REQUESTED is stale and shouldn't sit on the PR.
412fa950 (#435) landed the tri-state checkpoint. Re-verified on this branch:
- real login Keychain — a corrupt credential can now be overwritten and the account deleted; both returned
falsebefore - offline suite green on CI, including the two replacement tests, both negative-controlled (revert the fix → they fail; restore it → they pass)
Everything else from my earlier reviews still stands as written: the transaction design is sound, the lock nesting is safe, the outbox generation matching is correct, and the sibling-account concern I raised first was disproved by experiment — your account-scoped key design holds.
Still open, none of them blocking, and two of them not yours:
ProviderSharedCredentialOwner.defaultsresolves to the production suite under XCTest, so running the offline suite rewrites the developer's real shared App Group defaults — pre-existing onmain- A valid-but-wrong access group makes reads return
.missingrather than.failure, so the code "succeeds" while real credentials sit orphaned in the other group — pre-existing onmain, and the one I'd prioritise, since MAS ↔ Developer ID switching changes the access group ProviderConfig.saveSecretsdiscardsrestoreSecrets' result, so the model layer can't distinguish a clean rollback from mixed state — the transaction layer can, so this is a narrower gaperrSecDuplicateItemretries the identicalSecItemUpdatethat just failed — dead branch, low severity, pre-existing
Two gates remain that are yours, not mine: this is still a Draft, and your own list has "a manual signed-in provider-account smoke test should be completed before merge". That smoke test has not been done — my harness covers the model layer against a real Keychain, but nothing has exercised the editor UI flow end to end.
|
@cyq1017 — approved, and it's yours to merge. Nothing on my side is holding it now; I deliberately did not touch the Draft flag or merge it for you. Where it stands
The one gate left is your ownYour PR body lists "a manual signed-in provider-account smoke test should be completed before merge", and that still hasn't been done. My harness covered the model layer against a real Keychain — save, checkpoint, restore, delete, and sibling isolation — but nothing has driven the editor UI flow end to end. Given Worth doing specifically: with two accounts of the same provider signed in, edit-and-save one, then delete the other, then relaunch and confirm the survivor still resolves. That's the path where a mistake would be expensive. Once you've done it, flip the Draft and merge — no need to wait on me. Open, none blocking, ordered by what I'd fix first
Credit where it's dueThe parts I went looking for and found already handled: the recursive-lock nesting, the outbox generation matching, and the per-primitive rollback contract. And the sibling-account destruction I flagged in my first review was wrong — I disproved it by experiment; your account-scoped key design holds. The throwaway real-Keychain harness is archived if you want it — it inventories the shared provider-scoped namespace first and refuses to run unless the probe kind is empty, then cleans up every key it creates. Say the word and I'll drop it in. |
JasonYeYuhe
left a comment
JasonYeYuhe
left a comment
There was a problem hiding this comment.
Re-reviewed at branch head 412fa950. Note that head is #435 (my own fix) merged into your branch — there are no new commits from you since the last round, so everything below is what still stands.
Verified working, not just read:
swift teston this branch: 2717 tests, 0 failures, 4 skipped.- The deletion-outbox generation scoping is correct. I checked the thing that would break it —
enqueuepurges every prior record for(owner, accountID)before inserting (ProviderAccountDeletionOutbox.swift:84), so at most one record per account exists and the generation-scopedrecords.remove(...)cannot leak stale entries. It also correctly refuses to let a stale in-flight response complete a newer retry, which was the point. Write-verified viasaved.contains(intent). ProviderAccountSaveTransactioncompensation ordering is right, and it deliberately assigns both rollback results tolets before&&so short-circuiting can't skip the secrets rollback. Easy to get wrong; you didn't.- The XCTest isolation genuinely addresses the wedge from last round, including the subtler half of it:
isolatedTestHomeDirectory != nil → return nilblocks the cross-app Keychain fallback rather than just redirecting the home, which is what made "isolate half of it" worse than not isolating at all. release(kind:accountID:)returningtruefor unsupported kinds is the right call — checked all three call sites (AppState.swift:1673,GeminiOAuthManager.swift:2450,2926); none treats it as "something was released".
1. The access-group item is not demonstrably closed — and I could not prove it either way
readResult maps only non-errSecItemNotFound statuses to .failure. So this closes the hard error case (denied auth prompt, missing entitlement, non-UTF8 payload) but leaves open the one I originally flagged: a valid-but-wrong access group where the item simply isn't there, which is a legitimate errSecItemNotFound → .missing → silent success → credential orphaned in the old group.
I tried to settle it against the real Keychain and the harness cannot answer the question, which is itself worth stating:
- Probing a nonexistent key under
group.yyh.CLI-Pulse,…quarantine,…qaand a deliberately bogus group all returned -25300errSecItemNotFound→.missing. - But then I planted an item in the default group and read it back through every one of those groups — all returned it successfully. My probe process is unsigned, so
kSecAttrAccessGroupis ignored entirely for it. That makes the first result meaningless as evidence about an entitled app.
So neither of us should claim this is fixed or broken yet. What would settle it: run that probe from a signed build carrying the production entitlements, in the actual MAS ↔ Developer ID shape (write under one channel's group, read under the other's). If that read comes back errSecItemNotFound, the .missing/.failure split does not help and the orphan needs a different mechanism — e.g. a fallback read across known previous groups before concluding "no credential".
2. deleteAndConfirmMissing can re-block account removal — same shape as #435
ProviderConfig.swift:464:
guard store.delete(key:accessGroup:) else { return false }
return store.read(key:accessGroup:) == .missingdelete has already succeeded. If the confirming read then returns .failure — keychain locked, auth prompt denied, i.e. errSecInteractionNotAllowed — the whole call reports failure, and deleteSecretsForAccountRemoval (:282, :293) aborts account removal.
That is fail-closed becoming fail-forever again, just narrower than the original. This project has a long history with exactly -25308 on a locked keychain, so it isn't hypothetical. Suggest .failure on the confirm read mean "could not confirm" — retryable, or trusted to the delete's own success — rather than "removal failed". The policy #435 settled on applies: an entry we cannot read gives the caller nothing to usefully refuse over.
3. makeSecretPersistenceCheckpoint is now optional but can never be nil
After #435 removed the early-out, ProviderConfig.swift:304 has no return nil path (the two matches in that range are inside #435's own comment). It is still declared -> SecretPersistenceCheckpoint?, and callers at :196, :244 and ProviderConfigEditor.swift:878 still guard let … else { return false }.
Those branches are dead, and worse, they advertise precisely the failure mode that caused the original bug — the next person has a ready-made place to reintroduce it. Please make it non-optional and delete the guards.
4. Three different definitions of "am I under XCTest", of unequal strength
KeychainHelper.swift:128—NSClassFromString("XCTestCase") != nilClaudeSourceStrategy.swift:229— same one signalCookieResolver.swift:36— seven signals (added by this PR)
Genuine question rather than a complaint: if one signal was sufficient, why did the cookie path need seven? If seven was needed, then the two guarding the most dangerous surfaces — the real Keychain and the real Claude credentials — are the weak ones. I checked and this is not currently failing (no swift-testing targets exist in the package yet, so XCTestCase is always linked), so this is drift-prevention rather than a live bug. One shared helper would remove the question.
5. Branch is 5 commits behind main
v1.49.0 shipped while this was open (first-run window, version 1.47.0 → 1.49.0, Android versionCode 66 → 67). GitHub still reports MERGEABLE and I saw no overlap with your files, but please merge main and re-run before landing — your 2717-test run does not include what's now on main.
6. Nit: two new user-visible strings are hardcoded English
ProviderConfigEditor.swift:854 and :866 set testState = .failure("…"), which renders through Text(msg) at :735 in an app shipping six locales. In fairness this matches the file's existing convention (:796 is already hardcoded), so it's not a regression — but these two are recovery instructions, which is the moment a non-English user most needs to understand what to do.
Overall this is careful work and the compensation design is sound. (1) and (2) are the ones I would want resolved before this lands; (3)–(6) are cleanups. Still yours to smoke-test and un-draft — I have not touched it.
Follow-on to the config in this branch's first commit. Three corrections the CI job log surfaced that a local `--quiet` run had hidden: 1. TWO RULE NAMES IN THE ALLOWLIST WERE INVALID. `nsnumber_init_as_function_reference` and `prefer_for_where` do not exist; the real name is `for_where`. SwiftLint only WARNS about an unknown identifier and carries on, so both rules were silently inactive — an allowlist that quietly ignores entries is its own trap. Fixed, and `for_where` immediately found 5 real sites. 2. force_try IS ERROR SEVERITY, and was the only thing making the job exit 2. All 6 sites are `try!` on NSRegularExpression built from compile-time string literals: they cannot fail at runtime, and making the properties optional would push a nil check onto every call site for no safety. Each now carries a reasoned `swiftlint:disable:next` — documentation of WHY it is safe, which is worth more than either silence or a blanket severity downgrade. The rule stays an error so an UNjustified `try!` still stops a merge. 3. I HAD BEEN READING THE WRONG CSV COLUMN. SwiftLint's CSV is file,line,char,severity,type,reason,rule_id — index 5 is the human message, index 6 is the rule id. Two annotation passes silently matched nothing and reported success. The earlier "715 distinct rules" was 715 distinct messages. Result: 3,104 violations -> 33 warnings, 0 errors, exit 0. So `continue-on-error: true` is removed and the job now BLOCKS. A red check that blocks nothing is what let 3,104 accumulate in the first place. VERIFIED THE GATE FIRES, not just that it passes: appending a deliberate `try!` to PrivacySettings.swift made the lint exit 2; restoring the file returned exit 0. The 33 remaining are warning-severity and do not fail the job. Zeroing them and adding --strict is the follow-up; one of them is in ProviderAccountDeletionOutbox and must wait for #432, which changes that initializer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…something (#442) * ci: give SwiftLint a config, so 3,104 violations become 35 that mean something There was no `.swiftlint.yml` anywhere. CI therefore ran SwiftLint's DEFAULT rule set and reported 3,104 violations across 248 files, every run, for months. A check that is always red carries exactly as much information as one that is always green. It cost real time too: v1.49's PR had to verify by hand that its new files were clean, because the check itself could not say. The defaults were not wrong so much as not this project's: 1,734 (56%) identifier_name — objecting to `s`, `v`, `c`, `k`, `d` in closures and math. A house-style opinion this codebase has declined 1,734 times. 376 line_length 213 trailing_comma 153 opening_brace ~700 size/complexity rules, which measure "this file is big" `only_rules` (an allowlist) rather than a long `disabled_rules`, for two reasons: it states what we believe instead of what we tolerate, and a SwiftLint upgrade cannot silently add a rule and redden every PR. before: 3,104 violations / 248 files after: 35 violations / 15 files Also PINS SwiftLint to 0.63.2. `brew install swiftlint` was unpinned, while ruff three files away carries a paragraph explaining why that is unacceptable. Same failure mode, same fix. WHAT IS DELIBERATELY NOT DONE HERE The remaining 35 are left, and the job stays `continue-on-error: true`, because two of them must not be fixed mechanically and this PR touches no Swift at all (so it cannot conflict with the 22 files in the open #432): * 6 x force_try are `try!` on NSRegularExpression built from COMPILE-TIME CONSTANT patterns. The rule is right in general and wrong here; making the property optional would complicate every call site for zero safety. They need a reasoned `swiftlint:disable:next` each, not a blind rewrite. * ProviderAccountDeletionOutbox.swift:19 unneeded_synthesized_initializer is correct on main and SELF-RESOLVES when #432 lands — that PR gives the init a `generation: UUID? = UUID()` default, which the synthesized memberwise init would not provide. Deleting the init today would be reverted tomorrow; deleting it after #432 would silently drop a default. Zeroing the 35 and flipping the job to blocking is the follow-up, once #432 is in. Only then does turning it red mean anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: make SwiftLint block, now that being red means something Follow-on to the config in this branch's first commit. Three corrections the CI job log surfaced that a local `--quiet` run had hidden: 1. TWO RULE NAMES IN THE ALLOWLIST WERE INVALID. `nsnumber_init_as_function_reference` and `prefer_for_where` do not exist; the real name is `for_where`. SwiftLint only WARNS about an unknown identifier and carries on, so both rules were silently inactive — an allowlist that quietly ignores entries is its own trap. Fixed, and `for_where` immediately found 5 real sites. 2. force_try IS ERROR SEVERITY, and was the only thing making the job exit 2. All 6 sites are `try!` on NSRegularExpression built from compile-time string literals: they cannot fail at runtime, and making the properties optional would push a nil check onto every call site for no safety. Each now carries a reasoned `swiftlint:disable:next` — documentation of WHY it is safe, which is worth more than either silence or a blanket severity downgrade. The rule stays an error so an UNjustified `try!` still stops a merge. 3. I HAD BEEN READING THE WRONG CSV COLUMN. SwiftLint's CSV is file,line,char,severity,type,reason,rule_id — index 5 is the human message, index 6 is the rule id. Two annotation passes silently matched nothing and reported success. The earlier "715 distinct rules" was 715 distinct messages. Result: 3,104 violations -> 33 warnings, 0 errors, exit 0. So `continue-on-error: true` is removed and the job now BLOCKS. A red check that blocks nothing is what let 3,104 accumulate in the first place. VERIFIED THE GATE FIRES, not just that it passes: appending a deliberate `try!` to PrivacySettings.swift made the lint exit 2; restoring the file returned exit 0. The 33 remaining are warning-severity and do not fail the job. Zeroing them and adding --strict is the follow-up; one of them is in ProviderAccountDeletionOutbox and must wait for #432, which changes that initializer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
Scope and acceptance criteria
Non-goals
Validation
git diff --check: passed.0b8a62ccompleted: 23 checks succeeded, 4 conditional checks were skipped, and 2 SwiftLint warning-only checks reported the existing repository-wide baseline. The required CI Gate succeeded.Risk and rollback
main.Agent involvement
Codex and bounded local coding workers assisted with implementation, test execution, review preparation, and evidence collection. Agents did not approve scope expansion, merge, release, or deployment.
Human ownership and review
Unfinished work
0b8a62c; the required CI Gate succeeded, with only the two known SwiftLint warning-only checks reporting the existing baseline.0b8a62c; re-review from the Human Reviewer is pending.