Recover leader preferences with retained rule transactions - #1291
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Review note: diff was truncated after 60000 bytes in what I was given, so this covers everything up through Likely data-loss bug: leader-key preference migration
static var canonicalDefaults: UserDefaults {
TestEnvironment.isRunningTests
? .standard
: UserDefaults(suiteName: "com.keypath.KeyPath")!
}
if let data = leaderDefaults.data(forKey: Keys.leaderKeyPreference),
let stored = try? JSONDecoder().decode(LeaderKeyPreference.self, from: data)Unless Also a minor nit: Everything elseThe new preference-journal machinery in |
![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: 458eacd75c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Set(preferenceChanges.map(\.role)).count == preferenceChanges.count | ||
| else { throw Failure.invalidJournal } | ||
| let preferenceEntries = try preferenceChanges.map { change in | ||
| let before = try readPreference(change.role, from: preferences) |
There was a problem hiding this comment.
If another writer changes the leader preference after the manager captures leaderPreferenceBefore but before this asynchronous staging step, this line records that newer value as the transaction's before revision and then overwrites it with the stale candidate. A successful reload therefore silently loses the intervening preference update rather than triggering the intended third-revision conflict; pass the manager's expected preimage into staging and compare it before creating the journal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 65f1c34. RuleCollectionsManager now captures the exact durable leader preference Data with its admitted in-memory value, and RecoverableRuleWrite requires that expected preimage before creating the journal. The existing second comparison still catches changes during file I/O. Added a deterministic manager-level race test that mutates UserDefaults in onBeforeSave, after the manager snapshot and before stage; it verifies zero reloads, the intervening preference survives, files remain unchanged, and no journal is created. Focused suites pass: DurableConfigPreferenceRecoveryTests 10/10, CollectionLeaderRecoveryTests 8/8, RecoverableRuleWriteTests 11/11.
|
Confirmed: the canonical suite is the GUI's existing app domain, not a new migration domain. The force unwrap is for this fixed, valid Foundation suite identifier; tests inject temporary suites and route the default test path to the test process's |
458eacd to
65f1c34
Compare
|
@codex review |
|
Reviewed the diff (durable leader-key-preference journaling alongside rule-file recovery). The new
Nothing else in the diff looked incorrect — the snapshot/rollback plumbing through |
|
Thanks. Both points match the intentionally bounded contract documented in this PR:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65f1c34e50
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static var canonicalDefaults: UserDefaults { | ||
| TestEnvironment.isRunningTests | ||
| ? .standard | ||
| : UserDefaults(suiteName: "com.keypath.KeyPath")! |
There was a problem hiding this comment.
When the installed keypath-cli config apply reconciles a Leader collection, ConfigFacade.applyReconciledLeaderKeyPreference persists PreferencesService.shared.leaderKeyPreference before saveConfiguration validates and writes the config, and its non-dry-run error path never restores the prior value. By redirecting that shared service to the app's defaults domain here, a validation or file-write failure now leaves the app's durable leader preference advanced while the rule files remain unchanged—the split revision this transaction is intended to prevent. Route CLI reconciliation through the retained preference/file transaction or restore the preference on every precommit failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a99d7d0. ConfigFacade no longer mutates PreferencesService during reconciliation. It captures the exact canonical defaults preimage, generates from a local candidate, and for a real apply stages the generated config plus fixed leader preference role in the retained .rawConfig journal. Raw startup recovery now receives canonical defaults. Dry runs use the candidate without any defaults write. Added a failed non-dry CLI apply regression test and an interrupted CLI raw journal recovery test through another defaults instance. Focused passes: ConfigFacadeTests 10/10, DurableConfigPreferenceRecoveryTests 11/11, ConfigFacadeAdmissionTests 4/4, RawConfigurationRecoveryTests 12/12.
65f1c34 to
a99d7d0
Compare
|
@codex review |
|
Review finding: staged leader-key preference can be silently dropped
But in let preparedLeaderPreference = pendingLeaderKeyPreference ?? preferencesService.leaderKeyPreference
...
if leaderPreferenceBefore != nil {
try preferenceChanges.append(.leader(...))
}
let result = await SaveCoordinator(...).saveRuleState(
...,
leaderKeyPreference: leaderPreferenceBefore == nil ? nil : preparedLeaderPreference,
...
)
pendingLeaderKeyPreference = nil // <-- always reset, regardless of whether it was consumedIf a caller sets
This diff was truncated before I could see the tail of the toggle/enable-disable function to confirm whether it passes Minor: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a99d7d0b6b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `selectedOutput` were silently ignored by `keypath apply`. Reconcile both, mirroring | ||
| // the in-process reconcile the app does on load (RuleCollectionsManager). See #889. | ||
| let reconcile = await applyReconciledLeaderKeyPreference(from: collections) | ||
| let reconcile = await reconciledLeaderKeyPreference(from: collections) |
There was a problem hiding this comment.
If the CLI previously crashed after staging a leader/config revision but before committing its raw journal, this reads the staged preference before recovery. When the collection already matches that value, reconcile is nil and the non-transactional saveConfiguration branch leaves the old journal in place, so a later startup recovery can undo this apparently successful apply; when it differs, the transactional overload recovers to the older preference and then rejects the now-stale preimage. Recover pending raw writes before loading the preference and deriving reconciliation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da11ea7. ConfigFacade now calls retained-journal recovery with canonical defaults immediately after gate admission, reloads the shared preference cache, and only then loads sources and derives the leader candidate. Added a deterministic interrupted raw-journal apply test that verifies the recovered revision is reconciled and recommitted, the journal is removed, and the final config/preference both use tab.
| guard let decoded = try? JSONDecoder().decode(LeaderKeyPreference.self, from: previousData) else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When KeyPath.LeaderKey.Preference contains Data that no longer decodes, returning nil treats reconciliation as unnecessary. The subsequent ordinary save generates from PreferencesService.shared's default fallback, so an enabled Leader collection with an explicit non-default output is silently ignored while the malformed preference remains durable. Preserve the exact malformed preimage for the journal and reconcile from the default fallback, or fail the apply explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da11ea7. Malformed stored Data now supplies the exact journal preimage while reconciliation derives from the same default fallback PreferencesService uses; it no longer returns nil. Added a regression test that starts with malformed canonical Data and verifies CLI apply commits the collection-selected leader.
a99d7d0 to
da11ea7
Compare
|
@codex review |
|
Correctness: pending leader-key preference can be silently dropped
let preparedLeaderPreference = pendingLeaderKeyPreference ?? preferencesService.leaderKeyPreference
...
if leaderPreferenceBefore != nil {
try preferenceChanges.append(.leader(before: leaderPreferenceBefore?.data, after: JSONEncoder().encode(preparedLeaderPreference)))
}
...
let result = await SaveCoordinator(...).saveRuleState(
...
preferenceChanges: preferenceChanges,
leaderKeyPreference: leaderPreferenceBefore == nil ? nil : preparedLeaderPreference,
...
)
pendingLeaderKeyPreference = nilWhether a pending leader-key change is journaled/persisted and whether it's fed into config generation for this save is gated on the caller-supplied Suggest deriving the "should this save include a leader-preference change" decision directly from Minor/lower-confidence notes, not blocking:
Note: the diff supplied for review was truncated (after ~60000 bytes, partway through |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da11ea7346
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try await service.recoverPendingRuleWrite( | ||
| mutationPermit: permit, | ||
| preferenceDefaults: PreferencesService.canonicalDefaults | ||
| ) |
There was a problem hiding this comment.
When a previous app save crashed after Kanata accepted the staged rules but before the journal was committed, this call restores the files/preferences and sets needsRecoveredRuntimeRefresh, but the CLI never calls applyRecoveredRuntimeIfNeeded. A dry run returns without reloading, and generation or validation can also throw before the final reload, leaving Kanata on the attempted revision after the journal has been removed; because the marker is only in this service instance, it is lost when the CLI exits. Apply the recovered runtime before proceeding, or retain durable recovery evidence until it is reloaded.
AGENTS.md reference: AGENTS.md:L344-L344
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 547a231. Immediately after retained-journal recovery, ConfigFacade now calls applyRecoveredRuntimeIfNeeded through the same injected/TCP reload path before refreshing preferences, loading sources, or generating/validating a new candidate. A failed recovery reload aborts while the service still owns the recovery requirement. The interrupted CLI test now asserts exactly two reloads: restored revision first, newly committed apply second. ConfigFacadeTests 12/12 and ConfigFacadeAdmissionTests 4/4 pass.
da11ea7 to
547a231
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 547a2311c2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let rawRecovered = try RecoverableRuleWrite.recover( | ||
| files: rawFiles, directory: directory, scope: .rawConfig, | ||
| preferences: sendablePreferences?.value | ||
| ) |
There was a problem hiding this comment.
When an interrupted leader transaction is first encountered by an installed CLI pack operation, PacksFacade.withPackOperation constructs a manager—initializing PreferencesService.shared from the staged preference—before PackInstaller.recoverAndValidateState reaches this recovery call. Recovery restores the old preference directly in UserDefaults, but that pack path never reloads the manager's cached preference; the subsequent SaveCoordinator.stageRuleState therefore generates and commits the pack revision using the stale attempted leader while defaults contain the recovered leader, recreating the split revision. Return preference-recovery information or explicitly reload manager.preferencesService before the pack mutation proceeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 12c93cc. Pack recovery now uses the manager’s persistence defaults and immediately reloads that manager’s cached leader before recovered-source refresh or any pack mutation. Added a regression that constructs a fresh manager while the attempted preference is staged, runs pack recovery, and verifies its cache returns to the recovered prior leader. I also audited every recovery entry and downstream leader consumer: rule/bootstrap already reload injected stores; CLI recovers/reloads before candidate derivation; SaveCoordinator raw/app editors now reload canonical shared preferences; ConfigurationService rule/app/ordinary generation decodes the durable post-recovery leader directly. Backup/restore-only paths do not generate a leader-dependent candidate. PackRuleTransactionTests 27/27, ConfigurationServiceSavePipelineTests 41/41, AppKeymapSaveTests 19/19, RawConfigurationRecoveryTests 12/12 pass.
Review notesPreference durability relies on deprecated The whole point of this change is to journal the leader-key preference write alongside the config file so a crash mid-write can't leave them out of sync (see try writePreference(change.after, role: change.role, to: preferences)
guard synchronizePreferences(PreferenceDefaults(preferences)) else {
throw Failure.systemCall("preference synchronize", EIO)
}
try requireAfterPreferences(preferenceEntries, in: preferences)
Contrast that with the config file write, which goes through Worth either accepting this as a known residual risk (much narrower than before, since it's now scoped to the sync-flush window rather than the whole save), or replacing the Minor: force-unwrap in static var canonicalDefaults: UserDefaults {
TestEnvironment.isRunningTests
? .standard
: UserDefaults(suiteName: "com.keypath.KeyPath")!
}
No other correctness issues found in the reviewed portion of the diff (note: diff was truncated after the |
547a231 to
12c93cc
Compare
|
@codex review |
|
Correctness: In if let preferences, !journal.preferences.isEmpty {
for entry in journal.preferences where try readPreference(entry.role, from: preferences) == entry.after {
if let before = entry.before {
try writePreference(before, role: entry.role, to: preferences)
} else {
preferences.removeObject(forKey: entry.role.key)
}
}
...
}This condition — "if the on-disk value currently equals Concretely: Suggested fix: mirror the file-entry logic — only restore to (Note: reviewed only the supplied diff, which was truncated after ~60000 bytes near |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Resolution of the remaining top-level review notes:
|
1 participant
Leader-key edits previously persisted
UserDefaultsbefore the retained rule journal existed, so a process exit or rejected reload could leave the next launch generating from a preference that did not match the recovered rule files.This change keeps the leader candidate local until
ConfigurationServicestages a versioned journal containing the fixed leader preference role and the config/source files. It verifies the canonical app defaults domain before reload, commits applied and pending runtime outcomes, restores rejected/interrupted revisions together, and fails closed when either a tracked file or the leader preference has a third external revision. Bootstrap and normal manager mutations refresh the durable preference before snapshotting. Installed CLI reconciliation now generates from a local candidate and retains the canonical preference with the raw generated config; failed applies and dry runs do not advance GUI defaults. Version 1 journals remain readable, and rule saves with no preference entry avoid the defaults synchronization barrier.Scope is intentionally leader-only. Logical keymap selection, Context HUD config inputs, device selection, and the global handwritten-file ownership guard remain documented follow-ups.
Validation:
swift build --jobs 4with pinned Xcode 26.6: passedDurableConfigPreferenceRecoveryTests: 11/11 passedCollectionLeaderRecoveryTests: 8/8 passedRecoverableRuleWriteTests: 11/11 passedConfigFacadeTests: 12/12 passedConfigFacadeAdmissionTests: 4/4 passedRawConfigurationRecoveryTests: 12/12 passedPackRuleTransactionTests: 27/27 passedConfigurationServiceSavePipelineTests: 41/41 passedAppKeymapSaveTests: 19/19 passedpython3 Scripts/check-accessibility.py: passed, 379 files checkedhrm-fast-typing,hrm-per-finger-sliders,hrm-typing-feel-slider,settings-repair-tab). One stale leader conflict assertion exposed by the gate was updated to the required fail-closed contract and its full focused class then passed.Unknown command: /thermo-nuclear-swift-review; GitHubclaude-reviewpassed on the current commit.