Add model-tidy: safe plan/apply tool for idle LLM model cleanup

[ThinkOffApp]

What it does

Status: two rounds of independent static review (codexmb) found real issues, both fixed on this branch — see the PR comments for the full gap -> test mapping. Round 1 (commit 0f6b682): fail-open process/docker checks, an apply crash window, hardlink scan scope. Round 2 (commit e45642e): a hardlink partner living outside every discovered root, and a hard-kill crash window that the round-1 fix's try/catch couldn't reach — replaced with the journal-based recovery described below. Still in draft; not merge-ready until the lead says so.

model-tidy moves idle local LLM model directories off a full internal GPU-box
drive (asus1/asus2: single 931 GB NVMe, 91%/95% full) onto another mount
(external SSD / NAS share), leaving a symlink behind so every existing path —
including a docker bind mount — keeps working.

• plan (default): read-only, prints what WOULD move and why every other
  candidate is skipped, with real GiB sizes (bytes / 2^30) and a
  free-before/free-after estimate. Writes a JSON record per run.
• apply: only with the explicit --apply flag AND --target </mount>,
  which must exist, be writable, and be on a different filesystem device
  than --home — checked via stat().dev, refused otherwise.

Language: Node.js, matching the repo (README: "No dependencies. Node.js ≥ 18
only."), following the existing src/<name>.mjs + bin/<name>.mjs +
test/<name>.test.mjs shape used by session-keepalive.mjs,
room-automation.mjs, etc. No systemd pattern existed in this repo (it ships
macOS launchd .plists for its own daemons); the target hosts are Ubuntu, so
systemd/model-tidy.{service,timer} are new, self-contained units — not
wired into the macOS installer.

One deliberate deviation from the brief: the copy step in apply is a
small hand-written pure-Node recursive copy (copyUnitPureNode in
src/model-tidy.mjs), not a shelled-out rsync. Reasons in
docs/model-tidy.md, short version: keeps the zero-dependency Node-only
posture the rest of the repo has, and rsync -H only preserves hardlinks
between paths named in the same invocation — the pure-Node version tracks
dev:ino -> target path explicitly across a whole hardlink unit and
fs.linkSyncs repeats, which is the same safety property but verified
directly by a test instead of relying on rsync flag behavior across
machines. Byte-for-byte verification (size + SHA-256 + symlink targets +
file-count) still happens after copy and before any deletion.

Selection rules (in order, each with an explicit reason string)

1. KEEP list match (--keep-file, path/glob, ~ expanded, # comments) — always skipped.
2. In use by a process: any process with the file open under /proc/*/fd, or a
   recognized serving process's (vllm, llama-server, llama.cpp, sglang,
   exllama, tabby, ollama, mlx, text-generation) command line
   referencing the path.
3. Bind-mounted into a RUNNING docker container (docker inspect on running
   containers' Mounts). Fail-safe: if docker is unreadable, ALL of
   ~/.cache/huggingface is treated as in-use and the run says so.
4. Newest mtime within --min-idle-days (default 14).
5. Already a symlink (previously tidied).
6. Hardlink sets move as ONE unit: candidates sharing a dev:ino across
   different roots are grouped; the group is selected only if every member
   independently cleared rules 1-5, otherwise the whole group is skipped with
   a reason naming which member failed and why.

Remaining candidates sort by size (desc), capped by --max-gb (whole units
only, deferred to next run if over budget).

Safety invariants (updated after two rounds of codexmb review — see PR comments for the full gap -> test mapping)

• plan never writes to disk, full stop — including for a unit left mid-swap by an interrupted apply. It only DETECTS that (via a journal) and reports it as interrupted move found: ...; it never mutates. Only recover (a new subcommand) and apply itself (once, at its own start) ever run recovery.
• apply requires BOTH --apply and --target; --target must be an absolute, existing, writable path on a different filesystem device — enforced by stat().dev, not path string matching.
• A source directory's copy is only ever removed after it's verified byte-for-byte (size + SHA-256) at the target, and only via the journaled swap sequence below — no code path deletes first.
• The exact safety claim, as specified: the original data is preserved and recoverable at every step; the original path is unavailable for the instant between the two renames, and until recovery if a crash lands there. It is not continuously available. Before touching a unit at all, apply writes a journal record (JSON, one file per unit, fsynced) with the source/staged/temp-link/target paths and the per-file size+SHA-256 manifest; it's updated after every step. Recovery is journal-driven, not naming-driven: a directory merely named like a leftover with no matching (and still-valid) journal record is reported and never touched.
• A hardlink set moves as a unit or not at all, and the scan covers every discovered model root regardless of its own status. It also refuses a candidate whose st_nlink exceeds the number of links actually found under the scanned roots — a partner link can sit entirely outside every discovered root, which no amount of wider scanning closes in general.
• Deletion uses fs.rmSync (Node), never a shell rm -rf.
• KEEP-listed paths are never touched by any mode.
• --report-to-room and any nightly apply are opt-in, off by default (the shipped systemd service runs plan only).

Install the timer (not installed by this PR)

mkdir -p ~/.config/systemd/user && cp systemd/model-tidy.service systemd/model-tidy.timer ~/.config/systemd/user/ && systemctl --user daemon-reload && systemctl --user enable --now model-tidy.timer

Test output (verbatim, node --test test/model-tidy.test.mjs)

▶ discoverCandidates
  ✔ finds hf-cache, models-dir entries
✔ discoverCandidates
▶ loadKeepList
  ✔ parses non-blank, non-comment lines
  ✔ returns [] for a missing file
✔ loadKeepList
▶ computeHardlinkGroups
  ✔ groups candidates that share an inode across different roots
✔ computeHardlinkGroups
▶ planRun
  ✔ selects exactly the idle dir and the complete hardlink set, with correct skip reasons for everything else
  ✔ skips a whole hardlink set if either member is not idle
  ✔ treats ~/.cache/huggingface as in-use when docker is unreadable
  ✔ skips a dir with an open file handle (simulated process check)
  ✔ skips a dir bind-mounted into a running container
  ✔ caps total moved bytes by --max-gb, deferring smaller units
✔ planRun
▶ validateTarget
  ✔ refuses a target on the same filesystem as the source (real check, no mocking)
  ✔ refuses a target that does not exist
✔ validateTarget
▶ applyRun
  ✔ copies, verifies, and replaces the source with a symlink for every selected unit
  ✔ negative control: a corrupted target file makes apply refuse and leaves the source untouched
✔ applyRun
ℹ tests 14
ℹ suites 6
ℹ pass 14
ℹ fail 0

Full repo suite (npm test): 672/673 pass. The 1 failure
(test/session-send.test.mjs idle-guard timing test) is pre-existing,
untouched by this diff, and passes in isolation — it only flakes under
full-suite parallel load on this machine.

Not verified

• Never run against a real machine except --dry-run-remote in plan mode. No apply has executed outside test fixtures.
• Ran --dry-run-remote against both asus1 and asus2 (read-only, plan-mode-only, per the task's step 8): neither host has a node binary, so the helper correctly refused before copying or running anything and printed an install instruction. It did not, and could not, run real plan logic on either box.
• The ad-hoc project-directory discovery heuristic (marker files at top level or one level deep, e.g. .../model) matches the two examples in the brief but hasn't been run against the real layout on either box.
• /proc/*/fd + cmdline process-in-use check only works meaningfully on Linux; degrades to checked:false off Linux (this dev machine is macOS) rather than silently reporting idle.
• docker inspect's Mounts[].Source format was assumed from documented docker behavior, not confirmed against the real vLLM/docker-compose setup on either box.
• No real hardlinked pair between ~/models/* and the HF cache has been inspected on either box; the test fixture constructs a synthetic one via fs.linkSync.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-21T09:34:20.542521Z	1da9d2b	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[ThinkOffApp]

Lead review of the apply path, read from the branch, not from the report.

Safety ordering holds: verifyUnit (same relative path set, same symlink targets, size AND sha256 per regular file) runs before any mutation; the only removal of a source (rmSync(src, {recursive:true})) is after that verification passes; the symlink is created next and re-verified; same-filesystem targets are refused up front. The negative-control test (corrupted target -> refuse -> original bytes read back intact) is the right test and is present.

One hardening suggestion, not a blocker: between rmSync(src) and symlinkSync(dst, src) there is a window where a crash leaves the source gone and no symlink. The copy is verified at the target so nothing is lost, but every path referencing the model breaks until someone recreates the link by hand. Safer sequence: renameSync(src, src + '.tidy-moving') -> symlinkSync(dst, src) -> post-symlink verify -> rmSync(src + '.tidy-moving'). Same-filesystem rename is atomic, so a crash at any point leaves either the original or a working symlink.

Confirmed from the report and worth keeping visible in the PR: this has never executed apply outside fixtures, and it has never produced a plan on asus1/asus2 because neither has Node. Decision on installing Node there is with petrus.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 104d7c9cf5

ℹ️ 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".

[chatgpt-codex-connector]

Stream checksums instead of reading model shards whole

For typical multi-GiB .safetensors or .gguf shards, readFileSync materializes the entire file in one Buffer; under supported Node 18 versions, sufficiently large shards exceed the Buffer limit, while smaller ones can still exhaust available memory. The copy then succeeds but verification throws, so affected model units can never be applied; compute SHA-256 incrementally with a read stream instead.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve the source until symlink replacement succeeds

When symlink creation or post-link verification fails, the source has already been recursively deleted, contradicting the failure guarantee and leaving a missing or dangling source path. This is reproducible with an existing cross-filesystem target passed as a relative path: validation accepts it, but symlinkSync(dst, src) interprets dst relative to the source's parent, and realpathSync(src) then fails after deletion. Normalize paths and use a reversible rename/rollback or staged atomic replacement.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Treat nested Docker bind sources as in use

If a running container bind-mounts a subdirectory of a candidate, such as ~/models/foo/weights while the candidate is ~/models/foo, this containment test returns false because it only checks whether the candidate is below the mount source. The parent can consequently be selected and removed while its child is actively mounted; the check must also recognize m.Source.startsWith(path + sep).

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject invalid idle-day values before planning

When --min-idle-days is mistyped, for example --min-idle-days fourteen, Number(...) produces NaN; every ageDays < minIdleDays comparison is then false, silently bypassing the freshness guard and allowing recently modified models to be moved during an apply. Validate that this option is finite and nonnegative before constructing the plan.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Fail closed when process usage cannot be checked

When /proc is unavailable or cannot be listed, findProcessUsers returns {checked:false, users:[]}, but this code treats the empty list exactly like a successful check and can select a model that is currently serving. This affects non-Linux systems and Linux hosts with restricted proc access; skip the candidate or abort apply whenever checked is false rather than silently continuing.

Useful? React with 👍 / 👎.

[ThinkOffApp]

Fixed the three safety defects from codexmb's static review (commit 104d7c9) in 0f6b682.

1. FAIL-OPEN in plan (most serious). findProcessUsers swallowed per-pid permission failures and still returned checked:true; an unreadable docker only fail-safed ~/.cache/huggingface, not ~/models or ad-hoc dirs. Now any candidate whose in-use status can't be fully verified — an unreadable pid, /proc unreadable, docker present but not inspectable — is skipped with "in-use status unverified: <why>", never treated as idle for lack of evidence. Unreadable docker now taints every candidate on the box. Summary line reports "N unverified".
Covered by: fails closed for EVERY candidate on the box when docker is unreadable, not just the HF cache, fails closed when the process check cannot be fully completed (e.g. EACCES on one pid), and the positive control with a readable process list and docker, the idle dir is still selected (test/model-tidy.test.mjs).

2. CRASH WINDOW in apply. Old sequence was rmSync(src) then symlinkSync(dst, src) — a crash between them left the source gone with no symlink. Now: renameSync(src, src+'.tidy-moving') → symlinkSync(dst, src) → re-verify → rmSync(staging). Any failure after the rename removes the half-made symlink and renames staging back to src. A leftover <src>.tidy-moving from a prior interrupted run is detected up front and that unit is refused, untouched. Safety claim updated in docs/model-tidy.md and this PR body: a failure at any step leaves a unit's source EITHER with its original in place OR with a working symlink to a verified copy — never neither.
Covered by: crash-window negative control: if symlink creation fails after the source was staged, the source is restored byte-identical and refuses a unit with a leftover *.tidy-moving from a previous crash, without touching it.

3. HARDLINK SCAN SCOPE. computeHardlinkGroups already scanned every discovered candidate (KEEP-listed/recent/in-use dirs included, not just the plain-idle subset) — verified this by writing the exact regression test requested rather than assuming. Fixed the skip message, which previously wrapped even the failing member's own reason in a confusing self-referential "hardlink set with itself" string; it now reads "hardlinked to <path>, which is not moving (<path>'s own reason)".
Covered by: skips an idle dir hardlinked to a KEEP-listed dir, even though both are otherwise idle-eligible.

Also fixed while touching the same code, from the same review but outside the three defects: docker bind-mount detection now also catches a mount source nested under a candidate (not just the reverse), --target must be an absolute path, and the CLI rejects a non-finite/negative --min-idle-days instead of silently disabling the freshness guard via NaN.

Test results: new tests pass standalone (--test-name-pattern) and in the full model-tidy run: 19/19. Full repo suite: 678/678.

Not done in this pass (same review, out of scope for the three defects, documented in docs/model-tidy.md's "Not verified"): verifyUnit's checksum step reads each file whole via readFileSync rather than streaming, which could exhaust memory on real multi-GiB shards; there's no lsof fallback for the process-in-use check. Still never run apply outside test fixtures, and still never run on asus1/asus2 (no Node there).

Leaving this in draft per the lead's instruction.

🤖 Generated with Claude Code

[ThinkOffApp]

Fixed the two remaining gaps from codexmb's re-review of 0f6b682, in e45642e. Read the probe (/tmp/codex-pr128-review.9e6lTY/probe.mjs) and reproduced both before fixing.

GAP 1: hardlink to a file outside every discovered root. computeHardlinkGroups only ever saw candidates it had itself discovered, so a second link placed directly under the fixture root (above home/models) was invisible no matter how wide the scan went — confirmed by running the probe unmodified against the pre-fix code (externalLinkSelected: true). Fixed by comparing each file's st_nlink against how many links this scan actually observed under the scanned roots; a shortfall skips the whole candidate with "hardlinked N times, only M links found under scanned roots; refusing incomplete set", checked before every other rule.
Covered by: GAP 1 regression: refuses a candidate hardlinked to a file OUTSIDE every discovered root (codexmb probe, reproduced exactly) and the positive control GAP 1 positive control: an idle dir whose hardlinks are ALL inside the scanned tree is still selected as a unit.

GAP 2: hard crash between rename and symlink. The probe's symlinkFn: () => process.exit(77) in a child process confirmed the original path was gone with no symlink and no in-process rollback ever ran (a process.exit can't be caught by try/catch). Per the lead's amendment to the original fix request, this is now journal-based rather than a bigger in-process rollback:

• plan is read-only — it never recovers anything, only detects an interrupted move (via the journal) and reports it per unit as "interrupted move found: ...", refusing to select that unit.
• A new recover subcommand (and apply's own start, only reached because --apply was given) is the only place that mutates. Before touching a unit, apply writes a journal record (JSON, one file per unit, fsynced) with the source/staged/temp-link/target paths and the already-computed per-file size+SHA-256 manifest; it's rewritten after every step (pending → linked → staged → swapped → removed on cleanup).
• Recovery is journal-driven, not naming-driven: a directory literally named *.tidy-moving or *.tidy-link with no matching journal record is reported and left alone, regardless of its name. A journaled unit is only acted on after re-verifying its manifest against whatever currently exists on disk; a mismatch is also left alone and reported.
• Exact safety claim (docs/model-tidy.md and PR body updated to say this, not more): "the original data is preserved and recoverable at every step; the original path is unavailable for the instant between the two renames, and until recovery runs if a crash lands there. It is not continuously available."

Covered by (all crash a real child process via process.exit, matching the probe's technique):

• codexmb probe, kept verbatim: a hard crash during the symlink step leaves the original intact, and the invariant holds after one recovery pass
• a hard crash right after the link is verified, before either rename, is healed with the original untouched
• a hard crash between the two renames is healed by completing the pending symlink swap (the gap the amendment specifically called out)
• a hard crash after the second rename (before cleanup) is healed by finishing the cleanup
• AMENDMENT: plan is read-only — an interrupted move is reported per unit, and every byte on disk (full tree, sizes and mtimes) is untouched by plan (before/after full-tree snapshot comparison)
• AMENDMENT: recovery acts only on journaled units — a directory literally named *.tidy-moving with NO journal record is never touched
• AMENDMENT: recovery refuses a journaled unit whose on-disk content does not match the journaled manifest
• refuses a unit with an UNJOURNALED leftover *.tidy-moving, without touching it (apply's own recovery pass only heals journaled units)
• recovery is idempotent and a no-op on a healthy tree

Test results: 11/11 new tests in isolation (--test-name-pattern), 29/29 full model-tidy suite, 688/688 full repo suite.

Not done, noted in docs/model-tidy.md: the streaming-checksum and lsof-fallback findings from round 1 remain out of scope (and now also apply to the journal's own manifest computation, which shares the same whole-file-readFileSync property). The journal-based recovery has only been exercised against synthetic fixtures and a hard-killed child process on this dev machine — never against asus1/asus2, a real multi-GiB model, or a genuinely full disk.

Leaving this in draft per the lead's instruction.

🤖 Generated with Claude Code

[ThinkOffApp]

Fixed the data-loss path codexmb reproduced in recovery, in 8df7bd0. Read the probe (/tmp/codex-pr128-round2.pjlier/probe.mjs, read-only) and reproduced it exactly before fixing.

Confirmed bug: source GOOD, apply interrupted between the two renames (a thrown exception during afterStage, journal step 'staged', source missing, staged original + pending link both present), target then corrupted to BADD. recoverInterruptedMoves reported completed-swap-and-cleaned and deleted the last good original. Running the unmodified probe against pre-fix code confirmed it: sourceBytes: "BADD", stagedOriginalExists: false.

Root cause: recovery validated the staged original's own manifest and checked that realpathSync returned something that existed, but never re-verified the target's actual content against the journal manifest, and never checked that the symlink's realpath was exactly the journal's recorded target path. A link resolving to something was being treated as proof of what it resolves to — it isn't, and the target can degrade after the link was created and verified.

Fix: one guarded function, finalizeSwapOrRestore, is now the only place in the codebase allowed to delete a staged original — called by both apply's own final step (same run) and recoverInterruptedMoves (a later run). It requires all three, every time, even for a link already verified once at creation:

1. every file under the journal's target path matches the manifest by size + SHA-256 exactly, no extra or missing paths;
2. the source path is a symlink whose realpath resolves to exactly the recorded target path;
3. the staged original itself, if it still exists, still matches the manifest exactly.

Any failure: remove a bad/half symlink at the source if present, restore the staged original to the source path if one exists, mark the journal record step: 'failed' with the reason, and report — never delete on the strength of "a link resolves."

Defect -> test mapping:

• codexmb's probe kept verbatim (direct port, same scenario): DATA-LOSS FIX, codexmb's round-2 probe kept verbatim: target corrupted after staging must not cost the last good original — asserts source reads GOOD (never BADD), and the unit is reported failed (never completed-swap-and-cleaned) with the target flagged.
• Mirror case (target fine, source symlink swapped to point elsewhere): mirror case: target is fine but the source symlink points elsewhere — recover must not delete the staged original — asserts recovery refuses and the good staged content survives either at the source or the staging path.
• Positive control (target and link both genuinely correct): positive control: target and symlink both correct — recover completes the swap and cleans up.
• Same corrupt-target scenario through apply's own final cleanup path, not only a later recover: the SAME corrupt-target scenario against apply's OWN final cleanup path, not only recover — corrupts the target via the afterSwap hook inside the same applyRun call and asserts apply reports the unit failed with the original preserved (never silently "moved").

Also fixed two pre-existing tests whose expectations predated this function (the old two-step completed-swap-from-link / finished-cleanup split is now correctly unified into a single verified completed-swap-and-cleaned, or a restore-on-failure when verification fails).

Test results: 4/4 new tests in isolation, 33/33 full model-tidy suite, 692/692 full repo suite.

Leaving this in draft per the lead's instruction. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[ThinkOffApp]

Fixed the mirror data-loss case codexmb's round-3 probe found, in 05588ee. Read the probe (/tmp/codex-pr128-round3.pHg5hh/probe.mjs, read-only) and reproduced both its scenarios before fixing.

Confirmed bug: apply interrupted afterSwap (live symlink already in place, staged original still pending cleanup), one file removed from the STAGED original only (destination GOOD, live symlink valid). The round-2 fix's single pass/fail treated "staged doesn't match" the same as any other failure and fell through to "restore" — which unlinked the correct, working symlink and replaced it with the known-damaged backup. Confirmed against pre-fix code: sourceHasWeights: false while targetBytes: "GOOD" — a live, working path destroyed in favor of a known-bad one.

Fix: finalizeSwapOrRestore is now an explicit 4-row decision table over three independently re-checked facts (targetOk, linkOk, stagedState — absent / matching / damaged, no longer a single boolean):

#	targetOk && linkOk	stagedState	Action
1	true	absent or matching	complete: finish any pending rename, delete staged if it matched, clear journal (existing)
2	true	damaged	preserve the live path untouched, quarantine the damaged backup to <sourcePath>.tidy-quarantine-<journalId>, journal completed-partial-staging-quarantined — nothing deleted
3	false	matching	restore: remove bad symlink, rename staged back, re-verify, journal failed (existing behavior, now only reachable here)
4	false	absent or damaged	touch nothing — not even a still-pending rename — journal failed-both-copies-damaged with per-check reasons, report every path

While implementing row 4 I found a related bug of my own: recoverInterruptedMoves used to complete a pending rename (temp-link -> source) speculatively before calling finalizeSwapOrRestore, reasoning "that alone never deletes anything" — but it still violates row 4's "rename nothing" requirement whenever the final decision turns out to be row 4. Moved that rename entirely inside finalizeSwapOrRestore, only performed within the row-1/row-2 branches that have already decided it's warranted.

Row -> test mapping:

• Row 2 (rewrote the existing test, since it turned out to actually exercise row 2, not row 3): AMENDMENT / decision-table row 2: ... it quarantines rather than deletes or restores over the good symlink — asserts the live symlink survives untouched with correct content, and the tampered staged copy ends up at a quarantine path, never deleted.
• Row 3: decision-table row 3, reproducing codexmb's round-3 probe's own 'bad-target' scenario (afterSwap crash, not afterStage): a corrupted target still triggers restore-from-staged, never quarantine — proves linkOk (a path check) does not imply target content is correct, and that this case still restores rather than quarantines.
• Row 4 (new): decision-table row 4: nothing verified good anywhere (target corrupted AND staging partial) — recovery deletes nothing, renames nothing, and leaves every path exactly as found — full before/after tree snapshot (sizes + SHA-256) across two consecutive recovery passes, asserting byte-for-byte identity and that the report names every path.
• Row 1 still holds: existing positive control: target and symlink both correct — recover completes the swap and cleans up.
• Round-2's corrupt-target probe (DATA-LOSS FIX, codexmb's round-2 probe kept verbatim) and the round-3 probe's own two scenarios were all re-run against this fix and pass (verified directly with the unmodified probe files too, not just the ported tests).

Also fixed two smaller issues found while wiring this up: a quarantined path (and its owning journal record) was being double-reported as an unrelated orphan stray by the general stray-scan — excluded it once its journal record actually carries quarantinePath; and discoverCandidates now excludes .tidy-quarantine-* names so a quarantine directory is never mistaken for a brand-new candidate model.

Test results: 3/3 new/changed tests in isolation, 35/35 full model-tidy suite, 694/694 full repo suite.

Leaving this in draft per the lead's instruction. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[ThinkOffApp]

Fixed both round-4 blockers, in 18e9144. Read the probe (/tmp/codex-pr128-round4.HAIFHo/probe.mjs, read-only, same fixture style as round 3) before implementing.

BLOCKER 1: never recursive-delete a path assumed to be a symlink. Three sites did rmSync(path, {recursive:true}) on a path only believed to be a symlink, without re-verifying that belief right before removal: finalizeSwapOrRestore row 3's sourcePath obstacle, its pending-link obstacle, and recoverInterruptedMoves's pre-swap stray-link branch. If a real directory ever sat there instead, or the symlink pointed somewhere unexpected, a recursive remove could destroy a whole tree that was never ours.

Replaced all three with one helper, unlinkOwnedSymlink(path, expectedTargetPath): lstatSync first — not a symlink, or missing, refuse, touch nothing; is a symlink but its realpath doesn't resolve to exactly the journal's recorded target, refuse, touch nothing; only then unlinkSync — never rmSync, never recursive. Every caller now treats a refusal as "left alone, reported, journal step: 'failed'".

Tests (sentinel-file style, one per site, plus the wrong-target case):

• site 1 (finalizeSwapOrRestore row 3, sourcePath obstacle): a REAL DIRECTORY with a sentinel file sitting where a symlink was expected is never touched
• site 2 (finalizeSwapOrRestore row 3, pending-link obstacle): ... refused, not recursively removed
• site 3 (recoverInterruptedMoves stray-link branch): ... refused, original untouched source survives
• a symlink at the pending-link path pointing somewhere OTHER than the recorded target must be refused, never unlinked

Each asserts the sentinel file (or the wrong-target symlink itself) survives, the unit is reported refused, and a full before/after tree snapshot (sizes + SHA-256, not just mtimes) proves nothing else on disk changed.

BLOCKER 2: journal durability. writeJournalRecordSync did open(file,'w') + write + fsync — a crash between the truncating open and the write leaves an empty or partial record at the path readers expect, making a real in-flight unit look like nothing needs recovering.

Changed to write-temp-fsync-rename: write the full record to <file>.tmp-<pid>-<random> in the same directory, fsync and close that temp file, renameSync it atomically over the real path, then fsync the journal directory itself (best-effort; some platforms can't fsync a directory fd, swallowed since the rename is still atomic there). Readers (listJournalRecords, used by both detectInterruptedMoves and recoverInterruptedMoves) now treat a record that's missing, empty, truncated, unparseable, or schema-invalid as journal-unreadable — reported, never acted on by plan, apply, or recover. A stray <hash>.json.tmp-* left over from an interrupted journal write is recognized by name and reported the same way, never parsed as a record.

Tests:

• (a) a journal record truncated to HALF its bytes is reported unreadable by plan and recover, and every path is left unchanged
• (a) ... truncated to ZERO bytes (empty file) ...
• (b) a stray .tmp-* journal file ... is never parsed as a record, and is reported
• (c) positive control: a complete, valid journal record still drives recovery normally

Test results: 8/8 new tests in isolation, 43/43 full model-tidy suite, 702/702 full repo suite.

Leaving this in draft per the lead's instruction. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[ThinkOffApp]

Round 6, all three items done, in e03d4dc.

ITEM 1: findProcessUsers error boundary. The inner per-fd catch around readlink/realpath swallowed every error, treating a permission failure the same as the fd simply vanishing mid-scan. Now: ENOENT is ignored and counted as such; any other error (EACCES, EPERM, anything else) marks that pid's check unverified and the candidate is skipped with in-use status unverified: <pid> <code>. Added an injectable realpathSync option to findProcessUsers so tests can throw at the exact read boundary rather than only mocking the whole listProcessUsers function.

Tests: ENOENT at the fd-resolution boundary (fd vanished mid-scan) is ignorable — checked stays true, candidate remains selectable; EACCES at the fd-resolution boundary marks that pid unverified and fails closed, naming the pid and the code; positive control: with no read-boundary errors at all, the check completes normally; plus an end-to-end planRun check that the skip reason names the pid and code.

ITEM 2: sha256File memory. Replaced whole-file readFileSync with bounded-memory streaming: readSync into one reused 8 MiB buffer, hashed incrementally. Every hashing call site (manifest computation, manifest verification, copy verification) shares this one function, so the fix is uniform — no other site needed touching. Exported sha256File and HASH_CHUNK_BYTES for direct testing.

Tests: hashes a file LARGER than the chunk size identically to a whole-buffer digest (correctness at multi-chunk scale); never allocates a buffer larger than the chunk size while hashing — Buffer.allocUnsafe/Buffer.alloc instrumented to throw if any single allocation exceeds the chunk size, proving bounded memory rather than a hidden whole-file read.

ITEM 3: swapToSymlink's pre-swap rmSync. The pre-swap verification-failure branch (~line 1364 pre-fix) did rmSync(link, {recursive:true}) on a path only assumed to be the symlink it had just asked to be created — the same class of bug fixed at three other sites in round 4, missed there since this site runs before any journal step describes a "target" to verify against. Routed through unlinkOwnedSymlink(link, dst) the same way; on refusal the path is left untouched, the journal record is marked step: 'failed' with the reason, and the thrown error names it.

Test: sentinel test: symlinkFn creates a REAL DIRECTORY at the link path instead of a symlink — it and a sentinel file inside it survive, apply fails, and the journal record says failed.

Test results: 7/7 new tests in isolation, 50/50 full model-tidy suite, 709/709 full repo suite.

Keeping this in draft — the lead said it should be complete after this round, but draft stays until they say otherwise. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[ThinkOffApp]

Round 7, both gaps done, in 360d730. Read the probe (/tmp/codex-pr128-round6.bcpiZx/journal-probe.mjs, read-only) and reproduced GAP 1 before fixing.

GAP 1: unreadable journal records did not protect their unit. Confirmed with the probe unmodified: interrupt beforeLink, empty the journal record, rerun plan — it correctly reported journal-unreadable with path:null, but that finding was filtered out of interruptedByPath (which only keeps findings with a resolvable path), so the source stayed selected. Pre-fix probe output: "selected":["/.../models/idle"].

Fixed by recovering ownership from the journal filename: journalFilePath always names a record <journalKey(sourcePath)>.json, and that 64-hex-char key survives even when the content doesn't. plan now builds a candidate→journalKey map and matches every unreadable/stray record's filename against it:

• a match refuses exactly that candidate: interrupted move with unreadable journal record: <file>;
• no match at all (unparseable name, or a key with no current candidate) makes plan refuse to select anything this run (globalJournalBlockReason, surfaced in the summary line as REFUSING TO SELECT ANYTHING THIS RUN), rather than guess.

apply enforces this even harder: after its own recovery pass, any remaining unreadable/stray journal file blocks the entire run before any mutation (before even --target validation) — unresolved journal state: <files>; run recover, or resolve by hand. recover still only reports such files and leaves them, and every path, alone.

Re-ran the unmodified probe against the fix: "selected":[].

Tests: codexmb's probe verbatim: interrupt beforeLink, empty the journal record, rerun plan — the source must NOT be selected, and the reason names the record; a stray journal file with an UNPARSEABLE name: plan selects nothing, and apply refuses before any mutation (full tree snapshot before/after); positive control: a healthy, journal-free run still selects the idle dir.

GAP 2: directory-fsync failure was swallowed. writeJournalRecordSync's directory-fsync catch caught and continued on any error, including EIO. Now: on the supported target (process.platform === 'linux', injectable for tests), any error from the temp-file fsync, the rename into place, or the directory fsync throws — uncaught by swapToSymlink, so it aborts the whole unit before whatever mutation that write was guarding, and no durability claim is made. Off Linux, only ENOTSUP/EINVAL from the directory fsync specifically is tolerated, attaching an explicit durability degraded: <code> note to the result; EIO and everything else still abort there too. The fsync/rename functions and the platform are both injectable through applyRun/recoverInterruptedMoves, so this is tested without branching on the real process.platform.

Tests: EIO on the journal-directory fsync, on a SIMULATED Linux platform: apply aborts, source untouched (full tree identical), report names the error; ENOTSUP on the journal-directory fsync, on a non-Linux platform: proceeds with a "durability degraded" note; EIO on the journal TEMP-FILE fsync aborts, on any platform.

Test results: 6/6 new tests in isolation, 56/56 full model-tidy suite, 715/715 full repo suite.

Keeping this in draft. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[ThinkOffApp]

Round 8, the ordering gap, fixed in 1da9d2b. Read the probe (/tmp/codex-pr128-round7.gO0idm/order-probe.mjs, read-only) and reproduced it before fixing.

Confirmed bug: applyRun called the mutating recoverInterruptedMoves before checking for remaining unreadable journal state. Fixture: one valid afterStage interruption plus an empty unknown.json. Pre-fix probe output: apply correctly returned the unresolved-journal failure (ok:false), but before/after showed the valid unit had already been promoted — before: {sourceExists:false, stagingExists:true} → after: {sourceExists:true, sourceSymlink:true, stagingExists:false}. No data loss (the promotion itself was fully verified), but "refusing" is supposed to mean nothing happened, not "something happened, and by the way we're refusing."

Fix: the preflight (unreadable/stray journal check) now runs strictly before recoverFn is ever called. recoverInterruptedMoves never touches an unreadable/corrupt record either way — it always reports and leaves those alone — so the set of files this check sees is identical whether it runs before or after recovery; only when apply is allowed to act on that information changed. If anything is unresolved, apply returns immediately with recovered: [] and touches nothing.

Re-ran the probe against the fix: before and after are now byte-identical ({sourceExists:false, stagingExists:true} both times), recovered:[].

The explicit recover subcommand is intentionally unchanged: it still heals every valid journaled unit in one run while reporting (never touching) any unreadable one alongside them. Documented as one sentence in docs/model-tidy.md: "apply refuses globally on any unreadable journal; recover heals what it can and reports the rest."

Tests:

• codexmb's probe verbatim: apply on a mixed fixture ... refuses with the unresolved-journal error, and the full tree is byte-identical before/after — the valid unit's staging is still present and its source is not yet a symlink
• the lone-corrupt case (no valid interrupted unit at all) still refuses
• positive control: a valid interruption with NO unreadable records still recovers and proceeds normally
• recover (the explicit subcommand's underlying function) on the mixed fixture heals the valid unit and reports the unreadable one

Test results: 4/4 new tests in isolation, 60/60 full model-tidy suite, 719/719 full repo suite.

Keeping this in draft. Nothing run on asus1/asus2.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1da9d2b72d

ℹ️ 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".

[chatgpt-codex-connector]

Reject targets nested inside selected source directories

When the cross-filesystem target is mounted beneath a selected candidate—particularly --target ~/models/archive, since every entry under ~/models is itself a candidate—this validation succeeds. The copy destination for that candidate is then created inside its own source, and copyDir recursively discovers and copies the directory it just created, potentially filling the target filesystem or running into path-length limits. Reject source/target containment in either direction, or exclude the target hierarchy from the plan.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Abort apply when automatic recovery remains unresolved

When a valid journal describes damaged or otherwise unrecognized on-disk state, recoverInterruptedMoves returns actions such as left-alone-nothing-verified-good, left-alone-after-failed-verification, or error; this result is never inspected, so apply continues copying and swapping unrelated selected units. That contradicts the documented whole-run refusal for an interrupted unit recovery cannot resolve and makes further mutations while manual recovery is still required. Check the recovery outcomes and return an error before processing the plan if any journaled unit remains unresolved.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate the target before running mutating recovery

If --target is nonexistent, unwritable, relative, or on the source filesystem, this ordering still runs recovery first. Recovery can promote a pending link and delete its verified staged original before validation returns the target error, so an apply that claims to refuse an invalid target has already mutated the model tree. Run target validation before recoverFn so invalid apply invocations remain non-mutating.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep plan usable without a writable log directory

Every plan invocation calls writeRunLog before entering the plan branch, despite the command's read-only contract. Besides creating files during a purportedly non-mutating run, an absent or unwritable --log-dir makes mkdirSync/appendFileSync throw before the plan is printed, preventing diagnostics on restricted or read-only hosts. Logging should be opt-in or best-effort for plan mode rather than a mandatory write.

Useful? React with 👍 / 👎.