fix(trash): never sweep a note Obsidian trashed under a name the server recorded

[aliasunder]

What does this PR do?

The trash retention sweep deleted whatever file sat at an expired trash_entries row's path. Emptying .trash/ by hand while the server runs leaves the row behind (the orphan purge runs only at boot), so a note Obsidian later trashed under the same name was deleted once the row expired. The local deploy guide tells users to empty .trash/ by hand to reclaim space, and promises that anything Obsidian puts there is never touched.

The sweep now deletes a file only while it is still, unchanged, the file the server trashed. Every file it keeps is logged with a warning naming the path and the reason.

Type of change

• New MCP tool
• Bug fix
• Refactoring
• Documentation
• CI / workflow change
• Infrastructure (SST, Docker)
• Other:

Checklist

• The pre-commit hook passed: typecheck, knip, and ESLint, Prettier, markdownlint and ShellCheck on the staged files
• npm test passes (5321 passed, 2 skipped, macOS, Node 24.18.0)
• npm run build succeeds
• New MCP tools follow the naming and description conventions in AGENTS.md (no tool changes)
• README or ARCHITECTURE.md updated (ARCHITECTURE.md trash bullets and SECURITY.md's gate list; the README and deploy guide already promise that files Obsidian trashed are never touched)

What changed

• Recording (moveNoteToTrash): a recorded trash move stores the landed file's identity next to its path: inode number, size and modification time in nanoseconds, from a bigint lstat (readTrashFileState). A failed identity read takes the existing fail-open path: warn, clear any stale row, and the delete still succeeds.

• Sweep (sweepOneEntry): after the two containment gates, the sweep reads the file at the row's path under the trash lock and unlinks it only when both hold:

  1. its identity equals the recorded one
  2. its inode change time is no later than 60 seconds after the moment the row was recorded

  Anything else is kept and its row dropped, with a warning carrying reason: unrecorded (row from before this change), replaced (a different file at the path) or changed (renamed or attributes written after trashing). The sweep summary counts these as droppedUnmatched.

• Store (search-index.ts): trash_entries gains a nullable file_identity column, added to existing databases by the same idempotent ALTER TABLE pattern as the other columns.

• Trash folder race: moveNoteToTrash created its .trash/ folder before taking the lock it shares with the sweep, so a sweep pruning that empty folder in between made the delete fail with cannot move to trash. The folder is now created under the lock; a new test failed on the old code.

• Orphan purge re-read: the purge's in-lock re-read compared only the row's timestamp, so a row replaced within the same second (by a case-variant spelling or another file's record) counted as unchanged and could be dropped, leaving that file unrecorded. It now compares the whole row.

• Folder prune: the sweep's prune of the folders an unlink emptied now runs inside its own try/catch. It throws on a hidden path segment, which only a corrupted or hand-edited row can hold, and that throw used to end the sweep for every row after it.

• Failed row read or drop: a row re-read or delete that fails (an I/O error, or a full disk on a delete) used to end the sweep, or the boot-time orphan purge, for every row after it. It is now logged, the row is kept for the next run, and the run moves on.

• Unrelated doc fix: CONTRIBUTING.md lost an unfinished sentence about cli/templates/, a folder that no longer exists, which ran into the env-block sync rule after it. The review bot raised it on this PR; it is its own commit.

• Scheduler: startTrashSweepSchedule uses async/await with try/catch/finally instead of a .catch().finally() chain. Behaviour is unchanged; the existing re-arm test covers it.

Why these fields

Field	Why
Inode number	Tells apart two files with the same size and timestamps, such as a copy restored from a backup.
Size and modification time	ext4 reuses freed inode numbers, so a note created after the hand-emptying can get the old inode. Its modification time differs.
Inode change time, as a limit	Every rename and attribute write moves it, so it catches the server's own trashed note restored out of .trash/ by hand and put back later, which keeps the other three values.
Device number, left out	Inside a Docker Desktop bind mount it is the number Docker Desktop's VM assigned when it mounted the share (43 here, against 16777231 on the macOS host), so it can change when Docker Desktop restarts, and rows outlive restarts.

bigint keeps every digit of the inode number, which can exceed 2^53 for a file ID seen through a Windows bind mount.

Why the change time is a limit, not a stored value

A rename updates a file's change time but leaves its inode number, size and modification time alone. So of the four values, only the change time shows that a note was taken out of .trash/ and later put back.

Storing the change time and requiring an exact match would make the sweep keep every file on Docker Desktop for macOS, untouched ones included. Measured there:

• Right after the server moves a note into .trash/, the container keeps reporting the note's change time from before the move. It had not updated 8.5 seconds later, and opening the file or listing the folder did not update it. The Mac had the new value, and a container started later read that new value. So a change time stored at the move would differ from the one read after any container restart.
• Changes made on the Mac reach a running container within about a second. A file replaced under the same name showed its new inode number, and a file given a new extended attribute showed its new change time. So the sweep sees the changes it checks for.
• The container's clock was within 0.1 seconds of the Mac's, so the container's timestamps can be compared with the Mac's change times.

So the server stores no change time. The row already records the moment of the move, written right after the rename, and the sweep deletes a file only when its change time is no later than 60 seconds after that moment. An untouched note passes whichever change time the container reports, the stale one or the real one, because both come before that limit. A note restored and put back later carries the change time of that later move, so it is kept.

The 60-second allowance

The row's moment is stored in whole seconds, and the Mac's clock can run slightly ahead of the container's, so an untouched note's change time can land a little after the recorded moment. The 60 seconds absorb both. The cost is one gap: a note the server trashed, restored by hand and trashed again within 60 seconds of the server's delete still passes, and is deleted when its row expires. A test pins that behaviour so a change to it is deliberate.

Upgrade note

Rows recorded before this change have no identity. The sweep cannot show that such a row still points at the file the server trashed, so it keeps the file and drops the row with a warning (reason: unrecorded). Notes the server moved to .trash/ before upgrading are therefore no longer cleaned up automatically, including a backlog kept under TRASH_RETENTION_DAYS=none and switched to a number after upgrading; emptying .trash/ by hand removes them. The local deploy guide and deploy/local/.env.example say so for version 0.54.11 or earlier.

Tests

• Reproduced the bug first: the new recycled-name test failed on main with the sweep reporting purged: 1.
• Sweep tests now record rows at the real moment and run the sweep with only Date moved 31 days ahead, as production does, so each file's real change time meets the limit.
• New tests cover the recycled name, a row without an identity, a change time past the limit, the exact 60-second boundary and one nanosecond past it, the accepted restore-within-60-seconds gap, a failed identity read during the sweep and during recording, the warm-database migration, the orphan purge's whole-row re-read, a failed folder prune that must not end the sweep, a failed row drop on each sweep branch and in the purge, a failed row re-read in both, and each identity field.
• Mutation checks: removing the gate, any identity field, the limit, the boundary (>=), the allowance (59 or 61 s), the migration, the identity write, the fail-open placement, or the stale-row clear each fails the intended test.
• End to end on Docker Desktop for macOS, with the branch's local image and a fresh container whose clock runs 31 days ahead: an untouched control note was unlinked, a note Obsidian trashed under a recycled name was kept (replaced), and a note restored and re-trashed on the host 61 seconds later was kept (changed).

Live Test Results

Head: 9d1adc8

Image: ghcr.io/aliasunder/vault-cortex:test (revision label 9d1adc8e41bfa70df2886e4904e53cd28e462b53, image sha256:c65ee78e346e731cff0ecff1563acf8e13fc5e703e444dfdb2cf093e8ec5c16e), built by Test Deploy without skip_build. This head is the branch rebased onto main at b9e14e0, so the image runs this PR together with the tag changes main gained.

On the deployed instance (Obsidian Sync on, so trash bookkeeping is off there):

• Deploy: the container was re-created from :test at 2026-10-10 02:46 UTC with that revision label. It is healthy with 0 restarts, and its boot logs show no warnings or errors.
• Index migration: trash_entries on the instance's existing index still has trash_key, trash_path, trashed_at, file_identity with 0 rows, so the migration ran cleanly on an index that already had the column.
• Delete through the live connector: a probe note tagged Probe-665-Case was written and deleted. A lowercase probe-665-case tag search found it first. The server logged deleted note with trashOption: "none", the note is gone, and no .trash/ folder was created.

The retention sweep itself, with the same image on the instance host in a separate container with a throwaway vault and index, Obsidian Sync off:

• Three deletes returned Moved <note> to trash (.trash/<note>), and each row stored an identity, for example .trash/control.md 258729:24:1791600484434668468.
• Then .trash/recycled.md was emptied by hand and a new note was moved in under that name. A minute later .trash/restored.md was restored by hand and trashed again.
• A fresh server with its clock 31 days ahead logged trash retention sweep complete with expired: 3, purged: 1, droppedMissing: 0, droppedUnmatched: 2, and kept-file warnings for .trash/recycled.md (replaced) and .trash/restored.md (changed).
• After the sweep: .trash/control.md was purged, .trash/recycled.md was kept with its new content ("obsidian trashed this"), .trash/restored.md was kept, and 0 rows were left.

The commits after this head (413b538, a6318b2, 25c13c6, 18b9679, 14a6459, c958ff5) rewrite code comments in vault-crud-tools.ts and vault-filesystem.ts and trim and reword the trash sections of ARCHITECTURE.md and SECURITY.md; no line that runs changed.

🤖 Generated with Claude Code

[umm-actually]

Document that trash recorded before this change is no longer auto-cleaned
Low severity · subtle bugs · medium confidence

deploy/local/README.md:306 — beyond the diff's line ranges, in code the changes touch or depend on.

The upgrade transition this change creates — rows recorded before the identity column keep their files forever (the sweep drops each row with reason unrecorded) — is stated in the PR description's Upgrade note but in no committed user-facing doc: deploy/local/README.md still says the server cleans up the notes it put there after 30 days, and README.md's TRASH_RETENTION_DAYS row says only notes the server itself moved there are cleaned up. A user upgrading with an expired-trash backlog keeps those notes on disk indefinitely, with only the section's existing manual-emptying advice as recovery.

Failure scenario: A user upgrades a deployment that has notes sitting in .trash/ recorded by the previous version; when their retention window passes, the sweep logs reason unrecorded, keeps each file, and drops its row, so notes the README promises are cleaned after 30 days stay in .trash/ until the user empties the folder by hand.

Suggested fix

Add a one-line upgrade note to deploy/local/README.md's Deleting notes section (mirrored in README.md's TRASH_RETENTION_DAYS row): trash recorded by earlier versions is kept and its row dropped at the next sweep with reason "unrecorded" — empty .trash/ by hand to remove it.

---

umm-actually · z-ai/glm-5.3-flash

[umm-actually]

umm-actually re-reviewed at c958ff5

No new findings (16 tracked finding(s) across all runs).

Context notes

• Priority docs already in context: ARCHITECTURE.md, deploy/local/README.md
• Priority docs not included: deploy/railway/README.md, deploy/render/README.md, deploy/remote/README.md (missing, unreadable, or over budget)
• 15 related file(s) excluded by max_related_files cap: cli/src/__tests__/env.test.ts, cli/src/__tests__/templates.test.ts, scripts/__tests__/search-eval-plan.test.ts, src/vault-mcp/mcp-core/__tests__/mcp-router.test.ts, src/vault-mcp/mcp-core/__tests__/tool-definitions.test.ts, src/vault-mcp/mcp-core/prompts/__tests__/prompt-test-harness.ts, src/vault-mcp/search/__tests__/file-watcher.test.ts, src/vault-mcp/search/__tests__/hybrid-search.test.ts, src/vault-mcp/search/__tests__/memory-index.test.ts, src/vault-mcp/search/__tests__/memory-recall.test.ts, src/vault-mcp/search/__tests__/search-helpers.test.ts, src/vault-mcp/search/__tests__/task-queries.test.ts, src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts, src/vault-mcp/vault-operations/__tests__/note-mover.test.ts, src/vault-mcp/vault-operations/__tests__/vault-folder-config.test.ts

---

umm-actually · z-ai/glm-5.3-flash

[aliasunder]

Dispositions for the umm-actually comments outside inline threads at c904ddb:

• Issue comment 6085723956 (document that trash recorded before this change is no longer auto-cleaned): fixed in d670be4. Release notes carry only PR titles and BREAKING CHANGE: footers, so the PR description's upgrade note would not have reached anyone upgrading. deploy/local/README.md ("Deleting notes") now says notes the server moved to .trash with version 0.54.11 or earlier also stay, and points to emptying the folder by hand; deploy/local/.env.example says the same next to TRASH_RETENTION_DAYS, synced into cli/src/env.ts. The README table row and feature bullet are unchanged: they describe current behaviour, and the README keeps operational caveats in the deploy guides.
• Issue comment 6085724247 (status): noted that the correctness-security phase did not complete before the deadline. The fix commit d670be4 triggers a new review round.

---

🔍 ship-check · pr-monitor · claude-opus-5-5

[umm-actually]

Complete the truncated template-sync rule sentence
Low severity · conventions · medium confidence

CONTRIBUTING.md:105 — beyond the diff's line ranges, in code the changes touch or depend on.

The template-sync rule ends mid-clause (holds verbatim copies of) and runs into the next paragraph, so the sentence names no files and a contributor cannot learn what cli/templates/ must mirror. The dangling object is on main, alongside the env-block rule this PR just satisfied.

Failure scenario: A contributor editing deploy/local/docker-compose.yml or a deploy/*/.env.example reads the rule, cannot tell which files cli/templates/ must keep in sync, and ships the change without the mirrored copy — the two surfaces drift until a drift test or a user hits the stale template.

Suggested fix

Restore the missing object so the rule names the files it covers, e.g. "**Template sync rule:** `cli/templates/` holds verbatim copies of the quickstart files under `deploy/`; change the copy in the same PR as the source." — or delete the sentence if `cli/templates/` no longer exists.

---

umm-actually · deepseek/deepseek-v4.1-flash

[aliasunder]

Disposition for the umm-actually comment outside inline threads at ba1fccf:

• Issue comment 6086341009 (complete the truncated template-sync rule sentence, CONTRIBUTING.md:105): fixed in 4566f67, as its own docs-only commit. cli/templates/ no longer exists, so the unfinished sentence about it is deleted and the env-block sync sentence that followed now stands on its own.

---

🔍 ship-check · pr-monitor · claude-opus-5-5

[umm-actually]

Guard the orphan purge's row delete against store throws
Low severity · subtle bugs · medium confidence

src/vault-mcp/vault-operations/trash-sweeper.ts:380 — beyond the diff's line ranges, in code the changes touch or depend on.

Pre-existing: the boot-time orphan purge calls the store's row delete without the per-row containment this PR gave the retention sweep (dropTrashEntry), so one failing delete rejects the whole purge and the remaining orphan rows wait for the next restart instead of the next row. The sweep treats the identical failure per row and keeps going.

Failure scenario: The index database hits SQLITE_FULL when the boot purge's deleteTrashEntry fires: the error escapes purgeOrphanedTrashEntries (server.ts logs 'orphaned trash entry purge failed') and every orphan row after the failing one stays unpurged until the next container restart, while the sweep's equivalent drop survives the same failure row by row.

Suggested fix

Wrap the delete in a try/catch that logs the failure with describeError and counts the row as not purged so the loop continues to the remaining rows — the sweep's dropTrashEntry in this same file is the template.

---

umm-actually · z-ai/glm-5.3-flash

[aliasunder]

Disposition for the umm-actually comment outside inline threads at 7243c3a:

• Issue comment 6087563553 (guard the orphan purge's row delete against store throws, trash-sweeper.ts:380): fixed in 01bc22e. The purge now drops rows through dropTrashEntry, the same guard the sweep uses, so a failed delete is logged, that row waits for the next boot, and the purge moves on to the remaining rows. A new test has the store fail one of two drops. It checks that the other row is purged and that the summary reports checked: 2, purged: 1, and it fails when the purge goes back to the bare delete.

---

🔍 ship-check · pr-monitor · claude-opus-5-5

[umm-actually]

Correct the delete tool's unconditional retention claim
Low severity · conventions · medium confidence

src/vault-mcp/mcp-core/tools/vault-crud-tools.ts:1090 — beyond the diff's line ranges, in code the changes touch or depend on.

The delete tool's model-facing description still promises that a note the server moved to .trash/ �bis deleted after its retention period�, but the identity/change-time gate this PR adds keeps � and unrecords � any such file that was replaced, rewritten, or touched after the 60-second allowance (and every pre-identity row), so the promise the model repeats to users no longer holds.

Failure scenario: A user deletes a note under the default system-trash setting; a backup or sync process rewrites the copy in .trash/, so the sweep classifies it changed, keeps the file, and drops its row. An agent that read the delete tool's description tells the user the note will be cleaned up in 30 days, and the space is never reclaimed (the same happens for every note trashed before the identity column existed, which the local guide's manual-emptying advice covers but the tool description does not).

Suggested fix

State the condition where the description names the outcome, e.g. "Notes this server moved there are deleted after its retention period unless the copy in .trash/ was replaced or changed, in which case it is kept and no longer tracked."

---

umm-actually · deepseek/deepseek-v4.1-flash

[aliasunder]

Disposition for the umm-actually comment outside inline threads at 48e83ae:

• Issue comment 6089106307 (correct the delete tool's unconditional retention claim, vault-crud-tools.ts:1090): the description stays unchanged by design. The retention sentence describes what happens to the notes the server moves. The one exception, a copy changed in .trash/ more than a minute after the move, only ever keeps a file. Exceptions that keep a file are documented outside the tool text, here in ARCHITECTURE.md's trash section, so the description that every client loads stays short.

---

🔍 ship-check · pr-monitor · claude-opus-5-5