fix(config): make legacy migration durable - #3864
Conversation
Durability rationale and invariantsThe previous migration wrote This change separates two guarantees that ordinary rename-based code often conflates:
The state machine preserves these invariants:
Observed verification on commit
Focused coverage includes YAML parse parity, exact mode parity, existing destination and publication-seam races, source/destination symlinks, atomic-unavailable and malformed outcomes, partial-write cleanup, committed failure disposition, parent fsync success/EIO/EINVAL paths, exact operation ordering/call counts, bounded warning contents, temp inventory, JSON retention, and zero fallback/rollback/delete after commit. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the legacy config.json → config.yml migration path in packages/coding-agent to be safer under races and failure modes, using same-directory exclusive temps plus a native atomic no-replace publish, while preserving legacy files and permissions.
Changes:
- Reworked
migrateJsonToYmlto publish via an exclusive, no-follow temp file + native atomic no-replace rename, with post-publish parent-directory durability proof and bounded warnings. - Added focused migration tests covering mode preservation, symlink refusal, destination races, atomic-unavailability, cleanup behavior, and parent-fsync failure modes.
- Added a settings regression test ensuring legacy JSON configs still load correctly through the migration path, and documented the fix in the package changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/coding-agent/test/settings-manager.test.ts | Adds a regression test proving legacy JSON configs load via the retained YAML migration and preserve mode bits. |
| packages/coding-agent/test/config-file-migration.test.ts | Introduces a dedicated test suite validating atomic publication behavior, race outcomes, cleanup, and durability warnings. |
| packages/coding-agent/src/config/config-file.ts | Implements the hardened migration (exclusive temp + native no-replace publish + durability classification and parent fsync). |
| packages/coding-agent/CHANGELOG.md | Notes the hardened migration behavior under “Fixed”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| vi.spyOn(fs, "writeSync").mockImplementation(((...args: Parameters<typeof fs.writeSync>) => { | ||
| if (args[0] === tempFd) trace.push("write"); | ||
| return originalWrite(...args); | ||
| }) as typeof fs.writeSync); |
| function isStructuredPublishOutcome(value: NativeNoReplaceResult): boolean { | ||
| return ( | ||
| typeof value.ok === "boolean" && | ||
| typeof value.mutationState === "string" && | ||
| typeof value.durabilityState === "string" && | ||
| typeof value.reason === "string" && | ||
| typeof value.primitive === "string" && | ||
| value.primitive.length > 0 && | ||
| typeof value.phase === "string" && | ||
| typeof value.diagnostic === "object" && | ||
| value.diagnostic !== null | ||
| ); | ||
| } |
![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: 05e87450f6
ℹ️ 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 noFollow = fs.constants.O_NOFOLLOW; | ||
| if (typeof noFollow !== "number" || noFollow === 0) { | ||
| warnMigration("no_follow_unsupported"); | ||
| return; |
There was a problem hiding this comment.
On native Windows O_NOFOLLOW is unavailable, so this guard returns for every existing legacy JSON file before reading it; the existing exact MCP reader handles the same platform gap by omitting only that flag on win32 and relying on the lstat/fstat identity checks. Since ConfigFile then proceeds to load only the YAML path, Windows users upgrading with only legacy models.json or other JSON-backed ConfigFile configs fall back to defaults instead of migrating, so the no-follow hard requirement needs a Windows-safe path.
Useful? React with 👍 / 👎.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head adversarial review for 05e8745 against current dev.
Verdict: APPROVE (no implementation defect found)
Adversarial pass over the legacy JSON→YAML config migration durability change (config-file.ts + config-file-migration.test.ts + settings-manager.test.ts):
- Atomicity: same-directory temp created with
O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, fully written via a progress-checked write loop,fchmod'd to the source mode (umask-independent),fsync'd, then published via the nativerenameNoReplacePath(renameat2 RENAME_NOREPLACE / renameatx_np RENAME_EXCL) with parent-directoryfsyncafterward. A partial YAML can never appear at the destination; an existing destination is never overwritten and there is no fallbackrenameSync. Verified the syscall-order test (open→write→chmod→temp fsync→close→rename→parent fsync) and the destination-race test. - Crash/restart: crash before rename leaves only an inert, mode-protected, random-named
.tmp(never read by the app); re-migration on restart is idempotent (JSON retained, destination re-checked). Crash after rename but before parent fsync keeps a complete YAML; failure dispositions never trigger destructive rollback of a committed publication (explicitly tested — no unlink afterok:falsepost-commit). - Permissions: published YAML mode equals the legacy JSON mode (asserted at 0o640 in both unit and settings-manager integration tests). O_NOFOLLOW applied to source open, temp open, and parent-dir open; symlinked sources and destinations are refused rather than followed.
- Rollback: the legacy JSON is never modified or deleted in any outcome; removal of the YAML is the rollback path and re-migration stays possible.
- Stale/corrupt config: corrupt JSON fails closed with a bounded warning and no YAML; existing YAML is never clobbered; TOCTOU on the source is guarded by a post-open identity (
dev/ino) check. - No secret disclosure:
warningDetailslogs only a fixed message plus a regex-sanitized error name — raw error text (which can embed paths and config values) is never logged, and tests assert the warning excludes the directory path and the config value even when the underlying error message contains them. Strictly better than the old path-bearing log. - Contract boundary: the TS classifier mirrors the Rust
NativeNoReplaceResult::from_exactmapping exactly (committed/not_attempted/none/complete; not_committed/not_attempted for the certified reason set; unknown→indeterminate→fail-closed identity-checked cleanup). Theio_failureset entry is a harmless defensive superset. - CI: every completed check on the exact head is SUCCESS (path-affected skips only). Focused test jobs (
config-file-migration.test.ts,settings-manager.test.ts), ts-build, cli-smoke, native-build, and state gates all green. - Changelog/artifacts: entry correctly under
## [Unreleased]; no generated artifacts touched.
Non-blocking observation: a hard crash between temp write and rename leaves the random-named .tmp behind until manual cleanup (no startup sweep in this PR). It is mode-protected and never read, so this is a hygiene nit only, not a correctness or disclosure issue.
Review receipt: gajae.pr-review-verdict.v1 approve sha256:05e87450f6ee490669a108e04c03efec4f141ff2
|
Resolved the final G005 review blockers in follow-up commit Blocker and correction:
Focused regressions cover descriptor/pathname mode disagreement, certified race cleanup, malformed outcome retention, throwing publication retention, and bounded diagnostic sanitization. Verification:
|
Direct YAML writes could expose partial content and wider permissions during startup. Publish a synced same-directory temp through the native no-replace primitive, retain JSON, and distinguish namespace commit from parent durability. Lore-id: c7d18a42 Constraint: migration remains synchronous and never falls back to ordinary rename Constraint: committed publications and legacy JSON are always retained Rejected: check-then-rename | permits a concurrent destination overwrite Rejected: ordinary rename fallback | weakens the no-replace invariant Confidence: high Scope-risk: narrow Reversibility: easy Directive: fail closed on malformed publication outcomes Tested: bun test packages/coding-agent/test/config-file-migration.test.ts packages/coding-agent/test/settings-manager.test.ts Tested: bun --cwd=packages/coding-agent run check
Deleting the staged file after an indeterminate native outcome could remove the only recoverable publication evidence. Preserve identity-bound staging names unless non-commit is certified, report sanitized stage and error-code evidence, and derive permissions from the opened source descriptor. Lore-id: g011-g005-retention Constraint: ambiguous publication outcomes must preserve the identity-bound temp Constraint: migration diagnostics must remain bounded and sanitized Rejected: cleanup after every non-committed result | native throws and malformed results do not prove non-commit Confidence: high Scope-risk: narrow Reversibility: simple Tested: config migration and settings suites; coding-agent package check
8f55bb1 to
cd06c07
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd06c07070
ℹ️ 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".
| @@ -1,1717 +1 @@ | |||
| # Changelog | |||
|
|
|||
There was a problem hiding this comment.
Restore the deleted changelog rather than leaving this file blank. This removes every released entry plus the [Unreleased] section; consequently the embedded changelog parser returns no entries to users, and scripts/release.ts skips this package during releases because it cannot find [Unreleased]. It also rewrites immutable released sections contrary to the repository contract.
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
| if (isCertifiedNonCommit(publication)) { | ||
| if (publication.reason !== "destination_exists") { |
There was a problem hiding this comment.
On NFS, older Linux kernels, and filesystems that reject renameat2(RENAME_NOREPLACE), the native call returns the certified atomic_unavailable outcome, but this branch treats it as terminal and deletes the staging file. The repository's existing managed-file publisher handles exactly this outcome by retrying with linkNoReplacePath, which preserves the no-overwrite guarantee; without that fallback, users whose config directory is on such a filesystem never migrate their legacy JSON and ConfigFile subsequently loads only the absent YAML path.
Useful? React with 👍 / 👎.
| const current = fs.lstatSync(tempPath); | ||
| if (!current.isFile() || current.isSymbolicLink() || !sameIdentity(current, identity)) return; | ||
| fs.unlinkSync(tempPath); |
There was a problem hiding this comment.
When another process replaces the temp pathname after lstatSync returns but before unlinkSync executes, the identity comparison still describes the original inode and this code deletes the replacement file. Thus the claimed identity-bound cleanup has a pathname TOCTOU window and can remove data owned by a concurrent process; use the repository's exact identity-bound unlink primitive, or otherwise combine verification and deletion atomically.
Useful? React with 👍 / 👎.
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
|
@Yeachan-Heo 이 PR은 APPROVED 상태인데 현재 head에서 CHANGELOG가 파괴돼 있다. 지금 머지 버튼을 누르면 릴리스 이력이 날아간다.
$ git cat-file -s cd06c0707:packages/coding-agent/CHANGELOG.md
1GitHub의 원인은 내가 머지한 #3932다. #3942 에 전말을 정리했고, #3941 로 CI 가드를 올렸다. 12건이 같은 상태이며 그중 6건이 APPROVED 리뷰를 달고 있다. 복구: git checkout origin/dev -- packages/coding-agent/CHANGELOG.md |
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review of head cd06c0707.
Blocked only by the changelog
packages/coding-agent/CHANGELOG.md is 1 byte on this head (dev: 312,259). That is fallout from #3932, which I authored and merged — twelve open PRs took the same damage within ten minutes (#3942). Not your doing, but it has to be fixed before merge, and this PR is one of the two still showing reviewDecision: APPROVED, so the merge button is live over a destroyed changelog.
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md
# then re-add this PR's entry under ## [Unreleased]The code is the most careful publication path I have read in this repo
I went looking for the usual gaps and did not find them:
O_CREAT | O_EXCL | noFollowon the temp — cannot clobber, cannot be redirected through a symlink planted in the config directory.fstatSync(tempFd).isFile()— validated through the descriptor, not the path, so there is no TOCTOU window between open and check.sourceModeat open andfchmodSyncafter — open mode is subject to umask, so passing the mode alone would have silently widened or narrowed permissions depending on the user's environment. Re-applying through the fd is what actually delivers the stated goal of not exposing wider permissions.writeFullyrather than a barewriteSync— partial writes on a short write are real and produce a truncated config that still parses.- Full durability chain:
fsync(temp)→renameNoReplacePath→syncParentDirectory(ymlPath). The rename is a directory metadata operation and is not durable until the parent is synced; you do it, and you do it inside the committed branch with its ownparent_syncstage and a distinctpublished_parent_sync_faileddiagnostic. - Fail-closed on outcome classification: committed / certified-non-commit / everything else are handled separately,
destination_existsis treated as an expected non-event rather than a warning, and an unproven-but-committed outcome still warns (published_outcome_not_proven) instead of being silently accepted. removeCertifiedTemp(tempPath, tempIdentity)deletes by recorded identity, not by path — so a concurrent process that recreated the same path does not get its file removed.- Rejecting check-then-rename ("permits a concurrent destination overwrite") and ordinary rename fallback ("weakens the no-replace invariant") are both correct, and the second is the one most people give up under pressure.
Cross-reference for your own #3866
#3866 (keybinding migration, same author) implements the same shape — temp, fsync, rename — but omits the parent-directory fsync that this PR gets right at line 222. I flagged it there; the fix is literally the syncParentDirectory call you already wrote here. Worth lifting this file's helper rather than reimplementing.
Restore the changelog and I will convert this to an approving verdict immediately; nothing in the code needs to change.
gajae.pr-review-verdict.v1 merge-blocked sha256:cd06c0707057c048375f18bae63f56b549e677ae reviewer:architect evidence:git cat-file -s <head>:packages/coding-agent/CHANGELOG.md returns 1 vs 312259 on dev; code read of config-file.ts:110-267 at this head found no defects
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee961da3ed
ℹ️ 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".
| tempFd = undefined; | ||
|
|
||
| stage = "publication"; | ||
| const publication = native.renameNoReplacePath(tempPath, ymlPath); |
There was a problem hiding this comment.
When another process replaces the discoverable temp pathname after the descriptor is closed on line 213 but before this call, renameNoReplacePath publishes the replacement inode. Although tempIdentity was captured, it is only consulted during cleanup and the published destination is never checked against it, so migration can accept YAML that was not derived from the legacy JSON. Keep the staged descriptor authoritative through publication and verify the destination identity before treating the outcome as committed.
Useful? React with 👍 / 👎.
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
Summary
Verification
bun test packages/coding-agent/test/config-file-migration.test.ts packages/coding-agent/test/settings-manager.test.ts— 56 passedbun --cwd=packages/coding-agent run check— passed