#42: verify every listed manifest, and lock rollback

Question

Two safety gaps in RollbackService become reachable the moment manifest ids are shown to a user. Kriday's decision during charting: fix them inside #42, ship them together, do not ship #42 and leave the hole open in between.

Gap 1: listManifests verifies nothing.

src/core/organize/rollback.ts:346 reads and parses every .json in the rollback directory with no signature or hash verification. getManifest (:276) does verify, via manifestIntegrityService.verifyManifest (src/core/organize/manifest-integrity.ts:121), which checks version, hash match, and HMAC-SHA256 with timingSafeEqual.

The doc block at rollback.ts:331 states this plainly: callers acting on a listed manifest's paths must verify first. It is survivable today because manifest ids are internal. The pass manifest ticket makes them user-visible, so an unsigned manifest file becomes a value a user can paste back in.

Every caller that acts on a listed id must route through getManifest. Audit the current callers:

src/tools/rollback.ts:78 takes listManifests()[0].id when no manifest_id is given, then passes it to rollback(). rollback() calls getManifest first (rollback.ts:463), so this path is already safe. Confirm it, do not assume it.
QuarantineService.latestQuarantineId() (src/core/organize/quarantine.ts:379) reads the latest id from the list. Check whether it then reads contents without verifying.
QuarantineService.readManifest(id) (quarantine.ts:390) does verify. Good precedent, follow it.

What to decide. Whether listManifests itself should skip unverifiable manifests, or keep listing them and push verification onto every caller. Skipping in the list means a tampered file becomes invisible rather than reported, which is arguably better and hides evidence. Reporting it means the list carries a verified: false flag, which is more honest and more work. Both are defensible; the choice is a policy one, not an implementation one.

Gap 2: RollbackService takes no lock at all.

AGENTS.md records this as a known gap. HistoryLoggerService serializes every writer through operations.lock (src/services/history-logger.service.ts:85, :171), and that is the only lock in the server. RollbackService reads and unlinks manifest files unguarded (rollback.ts:464, :729), so two processes undoing at once race on the same manifest file.

This was theoretical while only the newest manifest was reachable by id. Listing ids to a user makes it real.

Do not bolt the history lock onto RollbackService. That serializes two different resources and makes the two files contend. If this gets fixed, the separate shared state comes first.

Implementation detail.

Add a manifest lock in the rollback directory, sibling to operations.lock in the history directory. Reuse the token-and-staleness pattern from history-logger.service.ts:188 rather than inventing a second one.
manifest_id has no format validation at the Zod layer (src/schemas/organize.ts:259 is a bare z.string().optional()). getManifest checks MANIFEST_ID_PATTERN internally (rollback.ts:280). Hoist that to the schema so a malformed id fails as a validation error rather than a filesystem miss.
Keep listManifests' signature stable if you choose the flag approach. If you choose to filter, note that QuarantineService.latestQuarantineId depends on listing order, which currently sorts by timestamp (rollback.ts:373). Filtering changes which manifest is newest.

Tests that must fail without this change.

A manifest file whose signature is corrupted is not acted on by any listed id.
A manifest file whose signature is corrupted still appears in a list that reports it as unverified, if the flag approach is chosen. Assert the chosen behaviour, not both.
Two concurrent undo_last_operation calls on the same id: exactly one succeeds, the other reports a clean refusal. Never a torn manifest.
undo_last_operation with manifest_id: "not-a-uuid" returns a validation error, not a lookup failure.

Proof. Drive it through node $C call file_organizer_undo_last_operation in the verify sandbox, then corrupt a manifest on disk and drive it again.

Relates to. The retention ticket introduces manifest deletion, which is a new writer on the same unguarded resource. Gap 2 has to be closed before pruning ships, or pruning becomes a second race.

fix(rollback): flag every listed manifest, and lock the directory it …

Decision

Close both gaps. Verify at list time with a flag, and give RollbackService its own lock.

The lock is not new machinery. HistoryLoggerService already implements the exact pattern needed: acquireLock at src/services/history-logger.service.ts:171, tryAcquireLock at :188, releaseLock at :217. Token-checked release, staleness at 2x the timeout. Rollback gets a sibling of that pattern against its own lock file.

A separate lock file, not a shared one. AGENTS.md is explicit: do not bolt the history lock onto RollbackService. History and rollback are different resources. Sharing one lock would make operations.jsonl and the manifest files contend, and pruning from #102 would become a second writer on the history lock. Separate resources, separate locks.

New file: manifests.lock in the rollback directory, sibling to operations.lock in the history directory.

Gap 1: listManifests verifies

Report unverified manifests, do not hide them. listManifests() gets a verified flag and keeps listing everything.

A tampered manifest file is a security event. Filtering it out means nobody ever learns it happened. findNewerConflicts already takes this position at src/core/organize/rollback.ts:404: an unverified manifest counts as a conflict rather than being ignored, because ignoring it would let a forged manifest hide a real overlap, and the failure mode of not ignoring it is a refused undo, not a damaged one.

New return type, not a field on RollbackManifest.

interface ListedManifest {
  manifest: RollbackManifest;
  verified: boolean;
  error?: string;
}

async listManifests(): Promise<ListedManifest[]>

RollbackManifest stays the on-disk shape. This matters: its hash covers actions and timestamp (src/core/organize/manifest-integrity.ts:83), and a verified field on the object that gets signed and hashed would be a footgun.

Nothing is filtered out, so the sort by timestamp at :373 is unchanged. That matters for QuarantineService.latestQuarantineId() (src/core/organize/quarantine.ts:379), which reads the list and depends on that order.

Every caller updates. This is a signature change, so it breaks at compile time rather than silently:

findNewerConflicts at :395, unpacks .manifest and uses the existing verify call it already does at :402.
QuarantineService.latestQuarantineId() at quarantine.ts:379.
findNewerConflicts callers.
Anything in src/tools/ and src/core/organize/quarantine.ts that lists.

The verification itself is manifestIntegrityService.verifyManifest (manifest-integrity.ts:121), which checks version, hash match, and HMAC-SHA256 with timingSafeEqual. No new verification code.

Gap 2: rollback takes a lock

Hold the lock for the whole rollback() call. Acquire at the top after ensureStorage, release in a finally.

Fine-grained locking would let two processes start the same undo. That is the exact disaster. Duration is bounded by file moves, not by minutes of work, and operations.lock sets the precedent.

Lock file: <rollbackDir>/manifests.lock
Timeout: 5000ms, matching operations.lock
Staleness: 2x timeout = 10s, matching :196
Poll: LOCK_RETRY_MS capped at timeout/4, matching :183

The reasoning for not lengthening the timeout: a waiter that cannot acquire within 5s is looking at a crashed process, not a slow one. Change the number only when there is a real undo that takes over 5 seconds to acquire.

createManifest takes the same lock. It writes a manifest file into the same directory, so it is the same resource.

This is what unblocks #102. Pruning deletes manifest files. Without this lock, a prune can unlink a manifest that an undo in flight is reading, and rollback unlinks on success at :729. Pruning would turn a rare race into a scheduled one.

Gap 3: hoist the manifest id pattern

manifest_id is a bare z.string().optional() at src/schemas/organize.ts:259. The format is only checked inside getManifest at rollback.ts:280, so a malformed id fails as a filesystem miss rather than a validation error.

Move MANIFEST_ID_PATTERN into the Zod schema so not-a-uuid is rejected at the boundary with a clear message. Import the pattern from one place, do not copy the string.

Also update the hand-written JSON inputSchema at src/tools/rollback.ts:32 to describe the format. Two sources of truth, nothing keeps them in step. AGENTS.md.

Tests that must fail without this change

A manifest file with a corrupted signature appears in listManifests() with verified: false and a non-empty error. Assert the flag, not both behaviours.
A corrupted manifest is never used as a conflict-free overlap source. findNewerConflicts still counts it.
Two concurrent rollback() calls on the same id: exactly one succeeds, the other reports a clean refusal. Never a torn manifest.
The lock file is released when rollback() throws mid-batch, so a retry can acquire it.
A stale manifests.lock older than 10s is reclaimed.
undo_last_operation with manifest_id: "not-a-uuid" returns a validation error, not a lookup failure.
QuarantineService.latestQuarantineId() still returns the newest manifest when an older one is corrupt.

Proof

Drive through the verify sandbox, then corrupt a manifest on disk and drive it again.

C=.opencode/skills/verify-file-organizer/scripts/control-file-organizer.mjs
node $C call organize_files --directory /tmp/... --dry_run false --json
node $C call undo_last_operation --json
# corrupt a manifest's signature, then
node $C call undo_last_operation --manifest_id <that id> --json

Never a unit test alone. The concurrency case needs two real processes.

Ordering

Ships with #42 so the cascade does not land on an unlocked, unverified manifest set. #102 cannot start until this is done.

Correction: this landed differently, and better

Shipped in PR #103 and #105. Two things in the comment above are now wrong. Read this before implementing anything from it.

The flag is `signatureValid`, not `verified`

I specified verified: boolean. The implementation named it signatureValid, and the reason is correct and I did not think of it.

IntegrityReport.verified already exists in src/core/organize/manifest-integrity.ts and means the opposite thing: whether the files are still where the manifest put them. Not whether the manifest is this machine's own record.

Two flags named verified in the same subsystem, meaning different things, is a trap. signatureValid wins.

The lock renews, because a whole-undo critical section is not bounded

I specified a 5s timeout with staleness at 2x, copied from operations.lock. That is wrong for rollback and the implementation fixed it.

HistoryLoggerService can copy that protocol because its critical section is one appendFile. applyRollback moves every file a manifest names and takes exactly as long as that takes. Under a fixed 5s staleness threshold, a legitimate holder looks abandoned partway through a large undo, a second undo of the same id reclaims the directory, and both spend the batch twice.

What shipped instead:

ManifestLockService at src/core/organize/manifest-lock.ts, holding manifests.lock.
rollback and removeManifest run inside runExclusive, so read-check-apply-delete is one critical section.
A holder renews on a timer at lockTimeoutMs / 4 and receives a ManifestLockLease to check between files, so one reclaimed anyway stops before the next file rather than at the end.
Renewal goes through the FileHandle the lock was created with, so it touches the inode and never refreshes a replacement that took the path.
Reclaiming renames to a private tombstone rather than unlinking, so two waiters judging the same dead lock cannot both delete it and both acquire.

That is a stricter fix than the one I specified, and the tombstone rename is the part I would not have thought of.

What this ticket still owes

Nothing. Both gaps are closed. #102 is now unblocked and can hook its prune into runExclusive.

One consequence for #102: the prune must run inside runExclusive, and it must be a lease-aware operation like applyRollback if it can take more than one lock acquisition. Prune reads the directory and unlinks a bounded set of files, so a single acquisition is fine.

Both gaps shipped. listManifests reports a signatureValid flag per entry rather than filtering, so an unverified manifest stays visible as evidence, and getManifest is still the only path that hands paths to a caller. ManifestLockService holds a manifests.lock of its own in the rollback directory, with renewal for a holder whose undo outlives the window and a rename-to-tombstone reclaim so two waiters cannot both take one dead lock. manifest_id is validated as a UUID at the schema layer in both undo_last_operation and verify_integrity, so a typo is a validation error rather than a filesystem miss. The four tests the issue asked for are in tests/unit/core/organize/manifest-verification-and-lock.test.ts.

Navigation Menu

Repository navigation

#42: verify every listed manifest, and lock rollback #97

Question

Decision

Gap 1: listManifests verifies

Gap 2: rollback takes a lock

Gap 3: hoist the manifest id pattern

Tests that must fail without this change

Proof

Ordering

Correction: this landed differently, and better

The flag is `signatureValid`, not `verified`

The lock renews, because a whole-undo critical section is not bounded

What this ticket still owes

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

#42: verify every listed manifest, and lock rollback #97

Description

Question

Activity

kridaydave commented on Oct 11, 2026

Decision

Gap 1: listManifests verifies

Gap 2: rollback takes a lock

Gap 3: hoist the manifest id pattern

Tests that must fail without this change

Proof

Ordering

kridaydave commented on Oct 11, 2026

Correction: this landed differently, and better

The flag is signatureValid, not verified

The lock renews, because a whole-undo critical section is not bounded

What this ticket still owes

kridaydave commented on Oct 12, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

The flag is `signatureValid`, not `verified`