fix(coding-agent): make keybinding migration failure-clean - #3866
fix(coding-agent): make keybinding migration failure-clean#3866kimdogyeom wants to merge 2 commits into
Conversation
Legacy migration overwrote the first backup and could leave stale temporary files after synchronous publication failures. Atomic fsynced staging now preserves recovery evidence and lets marker publication resume safely after the primary has committed. Lore-id: a93c6f10 Constraint: preserve the synchronous keybinding loading API and immediate migrated defaults Constraint: never overwrite the first keybindings.json.bak Rejected: asynchronous migration | delays immediate keybinding usability and changes lifecycle semantics Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/keybindings-config.test.ts packages/coding-agent/test/ux-adversarial.test.ts Tested: bun --cwd=packages/coding-agent run check
Migration invariants and failure rationaleThis stays synchronous deliberately. Keybindings are consumed during construction, so moving persistence to an async lifecycle would either delay immediate usability or introduce a second observable configuration state. The work here is about publication durability and atomic visibility, not latency: each primary/marker payload is written to a uniquely owned temporary file, fsynced, closed, and renamed before the next publication stage begins. The The marker is a completion receipt. It is published only after the migrated primary rename succeeds. A marker-stage failure therefore leaves a parseable canonical primary, the original first backup, and no false receipt; the next synchronous load recognizes that the primary is already canonical and retries only atomic marker publication. Once the marker exists, later loads perform no migration writes. The primary is never rolled back merely because receipt publication failed. Fault injection covers backup plus open/write/fsync/close/rename for both primary and marker. Every injected stage left zero Observed verification:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the coding-agent’s legacy keybindings migration so that it is failure-clean and more atomic, ensuring backups are preserved and partially-written migration artifacts don’t linger.
Changes:
- Preserve the first legacy backup via exclusive creation (
COPYFILE_EXCL) rather than overwriting. - Publish migrated primary config and the migration marker via per-run unique temp files, fsync, and atomic rename with cleanup on failures.
- Expand tests to cover atomic publication, idempotent resume behavior, and temporary-file cleanup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/coding-agent/src/config/keybindings.ts | Refactors keybinding migration to use exclusive backup creation and atomic temp-file publication with cleanup hooks. |
| packages/coding-agent/test/keybindings-config.test.ts | Adds regression tests for atomic migration publication, stage-by-stage failure cleanup, and idempotent resume. |
| packages/coding-agent/test/ux-adversarial.test.ts | Extends adversarial UX probe to assert marker creation and absence of leftover temp files. |
| packages/coding-agent/CHANGELOG.md | Documents the migration hardening under Unreleased fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| beforeStage(`${stagePrefix}-open`); | ||
| descriptor = fs.openSync(temporary, "wx"); | ||
| beforeStage(`${stagePrefix}-write`); | ||
| fs.writeFileSync(descriptor, content, "utf-8"); | ||
| beforeStage(`${stagePrefix}-fsync`); | ||
| fs.fsyncSync(descriptor); | ||
| beforeStage(`${stagePrefix}-close`); | ||
| fs.closeSync(descriptor); | ||
| descriptor = undefined; | ||
| beforeStage(`${stagePrefix}-rename`); | ||
| fs.renameSync(temporary, destination); | ||
| } finally { |
![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: 1c474b3d2a
ℹ️ 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".
| const ordered = orderKeybindingsConfig(migratedConfig); | ||
| publishKeybindingMigrationFile(filePath, `${JSON.stringify(ordered, null, 2)}\n`, "primary", beforeStage); | ||
| } | ||
| publishKeybindingMigrationFile(markerPath, "v1\n", "marker", beforeStage); |
There was a problem hiding this comment.
When keybindings.json already contains only namespaced keys, this still writes .migration-v1 even though migrated is false. If that user later adds or fixes an older unqualified binding, the marker path returns toKeybindingsConfig(rawConfig) without migrateKeybindingNames, and the TUI manager ignores keys that are not in the registry (packages/tui/src/keybindings.ts:224-225), so the documented legacy migration on load (docs/keybindings.md:66) no longer happens for that file. Only publish the marker after an actual migration, or keep translating legacy keys in memory when the marker exists.
Useful? React with 👍 / 👎.
Canonical-only configs incorrectly published the one-time marker, preventing legacy keys added later from being translated. Publish the marker only when legacy names were actually migrated and cover the delayed-legacy path. Lore-id: 6d45c3a1 Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/keybindings-config.test.ts packages/coding-agent/test/keybindings-migration.test.ts packages/coding-agent/test/ux-adversarial.test.ts Tested: bun --cwd=packages/coding-agent run check
|
Fixed discussion_r3719925576 in 6ebbbc6. Verified:
|
Summary
Verification
bun test packages/coding-agent/test/keybindings-config.test.ts packages/coding-agent/test/ux-adversarial.test.ts(12 pass)bun --cwd=packages/coding-agent run checkThe synchronous loading API and immediate migrated keybinding usability remain unchanged.