Resolve 26 spec items from 2026-05-17 multi-agent review by curtyo18 · Pull Request #79 · curtyo18/FileOrganizer

curtyo18 · 2026-05-19T11:47:54Z

Summary

Closes all 26 open issues raised by the 2026-05-17 multi-agent code review (8 blockers, 16 majors, 1 tooling, 1 nit).
26 commits on branch, one per issue (T05 bundles B4: [Blocker] No fsync before quarantine — cross-drive copy can violate power-loss invariant #54+B5: [Blocker] Non-mismatch copy failures leave partial destination on disk (extends B2) #55 because they touch identical lines in move-cross-drive.ts); final commit is a code-review polish bundle.
Net +77 tests (engine 353→400, shared unchanged, UI unchanged); typecheck + lint clean across all workspaces.

What changed (by domain)

Correctness (B-class blockers)

post_hash now written on every completed move/copy/dedupe op (B3: [Blocker] post_hash never written — reconcile completed-branch and undo verification both dead #53) — fixes the silent-corruption reconcile path
Cross-drive copy now fsyncs destination + parent dir before quarantining source (B4: [Blocker] No fsync before quarantine — cross-drive copy can violate power-loss invariant #54)
Pipeline failures (EIO/ENOSPC/EACCES/disconnect) now unlink partial dest (B5: [Blocker] Non-mismatch copy failures leave partial destination on disk (extends B2) #55)
Startup PRAGMA integrity_check; engine refuses to start on a corrupt catalog (B6: [Blocker] Missing startup PRAGMA integrity_check — spec §12.3 contract not delivered #56)
Scan refuses to start on volume serial mismatch — catches drive-letter remaps (B7: [Blocker] Volume serial mismatch refusal missing at scan start (spec §5.1) #57)
NTFS hardlinks excluded from duplicate proposals (B8: [Blocker] NTFS hardlink dedup exclusion missing (spec §8.4) #58); scan now writes stat.ino as ntfs_file_id

Engine internals (M-class majors)

API input hardening + app.onError centralised; zod-validated settings/role bodies (M21: [Major] API input hardening — NaN limits, JSON parse, settings/role/rule validation, error shape #68)
API POST /api/scans no longer hangs on pre-onStart rejection (M13: [Major] POST /api/scans hangs forever if runScan rejects before onStart fires #60)
Reconcile decisions branch on op.kind with explicit ambiguous field; hash-read errors warn-logged (M16: [Major] Reconcile decisions ignore op.kind; partial-write hash mismatch silently swallowed (spec §12.3) #63)
Migration FK check moved inside the transaction; FK-violating migrations no longer permanently advance schema_version (M12: [Major] Migration FK violations leave schema_version committed; test exercises copy of guard (follow-up to M10) #59)
Dedupe applier respects throttle profile (M14: [Major] Dedup applier hardcoded chunkBytes ignores throttle profile (spec §6.3) #61); throttle scheduler aligns to interval boundary, catches up after sleep (M15: [Major] Throttle scheduler drifts under load, misses boundaries after sleep/hibernate (spec §5.5) #62)
Failed-undo records inverse kind via shared inverseUndoKind helper (M18: [Major] Failed-undo op records original op.kind instead of inverse-undo kind #65); runScan extracted into processOneFile + finishScan (M19: [Major] runScan in scan/orchestrator.ts mixes setup/walk/finalize across ~165 lines #66)
Rules matcher: compile-reuse, Windows path normalisation, RuleError('INVALID_GLOB'), deterministic tiebreaker, date-invariant JSDoc (M20: [Major] Rules matcher hardening — picomatch reuse, Windows paths, glob guard, tiebreaker, date compare #67)
Path-under-root check now case-insensitive on Windows (M17: [Major] Path-under-root checks case-sensitive; breaks on Windows primary target #64); silent scan catches now structured warn logs (M23: [Major] Silent catches across scan code make file/perm errors invisible #70); CLI hardening (--flag=value, --version, unknown-cmd help, mediainfo path, shutdown logging) (M22: [Major] CLI hardening — --flag=value, --version, unknown-command help, mediainfo path, shutdown swallow #69)
Settings schema validated; optimizer reads/writes through SettingsRepo with injectable clock (M25: [Major] Settings/optimizer cleanup — schema validation, settings-table sharing, injectable clocks #72)

Infrastructure (T-class tooling)

fetch-binaries now SHA-256 verifies MediaInfo download (.partial → rename), refuses on mismatch (T1: [Security] fetch-binaries.ts has no SHA-256 verification of downloaded MediaInfo binary #75)
CI: npm cache, drop lockfile fallback, pin node 22.x, noImplicitOverride, type-checked ESLint (no-floating-promises + no-misused-promises) (T2: [Tooling] CI hardening — npm cache, drop lockfile fallback, pin node, type-checked eslint, noImplicitOverride #76)
Schema hardening migration 0006 (CHECK constraints, ON DELETE FK behaviour, partial state index, scan_id index) (M24: [Major] Catalog schema hardening — missing scan_id index, CHECK constraints, ON DELETE; idx_files_state revisit #71); rules.created_at column for tiebreaker (migration 0007)
Code hygiene sweep: hoisted magic numbers, structured logger consistency, identifier names (N4: [Minor] Engine code hygiene — magic numbers, single-letter names, log field override, console.error vs structured logger #78)
toFileRecord mapper + QUARANTINE_DIR_NAME constant extracted (M27: [Major] Code duplication — FileRecord mapper in 3 places, _FileOrganizer_quarantine literal in 2 files #74)
Dependency audit: piexifjs removed (sharp covers EXIF write); exifr stays with documented rationale; vitest 4 migration deferred (M26: [Major] Dependency audit — exifr abandoned (only EXIF reader), piexifjs frozen, vite/jsdom/vitest major drift #73)
Test infrastructure: silentLogger / waitForScanStatus / drain helpers; hostile-filename test (N3: [Minor] Test infrastructure — silentLogger / poll-loop / drain helpers + missing test cases #77)

Closes

#53, #54, #55, #56, #57, #58, #59, #60, #61, #62, #63, #64, #65, #66, #67, #68, #69, #70, #71, #72, #73, #74, #75, #76, #77, #78

Test plan (manual verification)

The full test suite (430 tests) and code review have run; this is a checklist for behaviour you may want to eyeball before merging.

Scans

Start a scan from the UI; verify it completes with files indexed (smoke test)
Open catalog.db in a SQL viewer after the scan; files.ntfs_file_id is populated (non-NULL) on Windows; matches OS file ID for sample files
Power-cycle the laptop mid-scan if practical; on next boot the engine completes startup and reconcileOnStartup runs without error
Start a scan, change the drive letter mapping in Disk Management while paused, resume: scan refuses with VOLUME_SERIAL_MISMATCH (HTTP 409 surfaced as a UI error)

Organize / Undo

Apply an organize batch with a cross-drive move; verify the dest file matches the source bytes; History row shows completed
Disconnect the dest drive mid-copy on a real NAS path; verify the batch fails with DRIVE_DISCONNECTED (UI shows 503) and no partial file remains at the dest
Tamper with a moved file's content; click Undo for that batch; verify the undo is refused (status failed, reason mentions post_hash); source is not restored

Dedupe

Create two hardlinks of the same file on Windows (mklink /H from cmd): scan, open Duplicates page; the hardlinked pair does NOT appear as a duplicate
With three copies (one real, two hardlinks), the group shows count=2, reclaimable bytes = 1×size, samePhysicalFile: true

Reconcile (crash recovery)

Manually taskkill the engine mid-organize; restart; engine logs reconcile-* events and the half-completed op is classified as completed (if dest matches post_hash) or failed (if source still present alongside completed dest)
Place an unrecognised file in <drive>\_FileOrganizer_quarantine\ manually; restart; engine warn-logs reconcile-quarantine-orphan

CLI

npx tsx packages/engine/src/cli/index.ts --version → prints package version, exits 0
npx tsx packages/engine/src/cli/index.ts init --catalog=/tmp/x.db → works (the =value form)
npx tsx packages/engine/src/cli/index.ts bogus → prints "unknown command" + help to stderr, exits 2
Run npx tsx packages/engine/src/cli/index.ts serve then Ctrl+C → shutdown messages appear with structured log lines

API

curl http://127.0.0.1:<port>/api/files?limit=abc → 200 with default-limited results (no NaN SQL crash)
curl -X POST -H 'Content-Type: application/json' -d 'not json' http://127.0.0.1:<port>/api/rules → 400 { "error": "invalid-json" }
curl -X PUT -H 'Content-Type: application/json' -d '{"throttleProfiles":"junk"}' http://127.0.0.1:<port>/api/settings → 400 with the zod path named

Catalog integrity

With the engine stopped, corrupt a few bytes inside the freelist trunk pointer of catalog.db (or use Get-FileHash-different copy from node); start engine; refuses to start, stderr contains CATALOG_CORRUPT + the wiki link
After running npm run fetch-binaries, modify any bit of mediainfo binary; re-run fetch — re-download happens (not validated for the extracted binary; only the zip is hashed) — verify with a tampered MEDIAINFO_ZIP_SHA256 constant that fetch refuses

Throttle

Configure a schedule that crosses a profile boundary in the next 5 minutes; observe a throttle-changed event published within ~60s of the boundary, not at the old setInterval-style drift

Known follow-ups (from the code-review audit)

These were surfaced by the post-implementation audit and deferred so this PR could land:

Test gaps (Important): parent-dir fsync warn log branch, reconcile restore failed branch, three onError typed-error branches (CatalogError/QuarantineError/IntegrityError), walker-realpath-error log assertion
Code quality (Minor): insertOp/insertOpFull duplication in reconcile.test.ts, three as any casts in move-cross-drive.test.ts (already documented), zod 4 changelog reference in PR body (this section), delete kind stub in POST_CONDITIONS could use a TODO link

The branch test count is 430 passing + 1 skipped (metadata-video shim test on win32) across all workspaces.

Stats

Commits: 26 (25 issue-aligned + 1 review-polish)
Tests: 353 → 430 (+77 over baseline, +4 in polish commit)
Schema migrations: 2 new (0006 hardening, 0007 rules.created_at)
New deps: zod@4 (engine, for input validation), typescript-eslint@8 (root, for type-checked rules)
Removed deps: piexifjs (replaced by sharp's withMetadata EXIF write)

… constant Three sites had the same 20-field row→FileRecord mapping; one constant was duplicated under two names. Extract toFileRecord from files-repo and import it from both planners. Export QUARANTINE_DIR_NAME from quarantine and use it from organize/applier. Closes #74

- Add idx_files_scan_id (markMissing now uses index, not scan) - CHECK constraints on batches.status, operations.status - ON DELETE CASCADE/SET NULL on operations.batch_id, operations.file_id, quarantine.batch_id - idx_files_state rebuilt as partial index WHERE state != 'indexed' - Spec §4.1 updated for the partial index Closes #71

…uctured logger consistency Sweep of small style/clarity items: named constants for repeated literals (chunk size, cache-control, eviction fraction, scheduler interval), single-letter renames in rules/scan code, dirname() replacing path.slice, structured logger in undo.ts where console.error was used, shared DEFAULT_FILL_THRESHOLD_PERCENT. No behaviour changes. Closes #78

Threads the verified destination hash into updateOperationStatus so the startup reconcile dest-matches-post_hash branch and undo verification can operate against the hash captured at move time rather than the catalog's current files.sha256 row (which may have been re-scanned independently). - moveSameDrive / moveCrossDrive: MoveOutcome now carries postHash (renamed from verifiedHash to match DB column name and remove ambiguity) - organize applier: passes outcome.postHash as postHash - dedupe applier: passes liveHash as postHash - undo: prefers op.postHash for the drift check; error text mentions post_hash so callers can distinguish from legacy "hash drift" - legacy ops with null post_hash fall back to files.sha256 unchanged - added symmetric cross-drive applier test to mirror same-drive test Closes #53

…dest on copy failure Cross-drive copies now follow open → pipeline → handle.sync() → close → re-open for hash, with parent-dir fsync on POSIX. Bytes are durable before the source is renamed into quarantine, delivering spec §12.3's power-loss invariant. Pipeline failures (EIO, ENOSPC, EACCES) now unlink the partial destination before rethrowing, extending closed #20's hash-mismatch cleanup to all copy error paths. DriveError disconnects attempt the unlink and swallow the secondary failure (option 1 from B5's discussion). Closes #54 Closes #55

…mbiguous field Per-kind post-conditions replace the destination-only classification: - move: dest matches AND source absent → completed; dest matches AND source present → failed (anomalous, alert user) - copy: dest matches → completed regardless of source - quarantine/restore: per-kind path checks Decision shape gains explicit ambiguous: boolean so the counter no longer uses .includes('ambiguous'). hashFile errors during reconcile are warn-logged with op id; previously they fell through silently and were indistinguishable from hash mismatch. Closes #63

…lper inverseUndoKind(op) centralises the 'move' / 'restore' selection used by all three undo paths (success cross-drive, success same-drive, failure). The failure path previously recorded op.kind (original kind, e.g. 'copy') instead of the inverse, under-counting failed undos in any query keyed by operations.kind. batches-repo.ts's RecordOperationInput now accepts optional errorMessage so the failure path records the op-with-error in one DB write instead of a record-then-updateStatus pair. Closes #65

Spec §12.3's startup integrity_check contract was missing — a corrupt catalog file was opened and used silently. assertCatalogHealthy now runs after migrate and throws CatalogError('CATALOG_CORRUPT') on any non-'ok' result. CLI surfaces the error cleanly (named catalog path + rebuild docs link), exits non-zero. Closes #56

Pre-flight in scan/orchestrator.ts now reads the live serial of the drive letter and compares against the catalog's stored volume_serial. Mismatch throws ScanError('VOLUME_SERIAL_MISMATCH'); API returns 409. This catches the drive-letter-remapped case where the catalog and the physical drive disagree, before the scan corrupts state. The check is skipped for synth- serials (POSIX path-derived) because synthSerial hashes the full path, not the mount point, so detectVolume(mountPath) and detectVolume(originalPath) produce different values even on the same filesystem. Only real OS-issued serials (Windows volume UniqueId) can be compared against a mountPath re-detection with reliable semantics. The API translates the pre-start rejection via Promise.race so the handler does not hang waiting for onStart, which never fires when runScan throws before scansRepo.start(). Closes #57

…8.4) Hardlinked copies share physical bytes; quarantining a non-keeper reclaims zero space and confuses the user. detectDuplicates now collapses copies sharing (driveId, ntfsFileId) within each group, drops groups with <2 distinct physical copies, and recomputes reclaimableBytes. samePhysicalFile is surfaced on the duplicates API response. Sub-fix: scan now writes stat.ino as ntfs_file_id; previously the column was always null in production so the hardlink collapse never fired. POSIX inode works as a hardlink-equivalence key here too. Closes #58

Dedup applier read chunk size hardcoded to 1 MB regardless of the user's current throttle profile. ApplyDedupeInput now accepts chunkBytes/sleepMs; API reads them from the shared ThrottleManager and threads them through. Idle profile dedup now reads at 256 KB, full-send at 4 MB, per spec §5.5. Closes #61

…spec §5.5) setInterval was best-effort and drifted under busy event loops; missed all intermediate ticks during sleep/hibernate. Replaced with self-rescheduling setTimeout aligned to the next interval boundary. On wake (long-gap detection), an immediate re-evaluation runs so the active profile matches the schedule within the next tick window after wake. Injectable now() clock removes dependency on vi.useFakeTimers in tests. Closes #62

Six catch-and-swallow sites in metadata-image, metadata-video, and walker now log at warn level with { path, err } (or kind for stat). Scan continues unchanged; the only difference is visibility for an operator investigating "why isn't this file/folder in the catalog?" Closes #70

runScan was ~165 lines mixing pre-flight, per-file loop, progress, and three terminal branches. Extracted processOneFile (per-file business logic) and finishScan (terminal status writes). runScan body now ≤60 lines. Behaviour unchanged; existing tests pass without modification. Closes #66

Background scan's .catch only logged; onStartFired was never resolved on pre-onStart errors, so the handler's await hung forever and clients timed out with no error surfaced. T09 wired the Promise.race; this commit completes the error mapping so any pre-onStart ScanError or DriveError lands as a structured 4xx/5xx within 100 ms (DRIVE_NOT_FOUND → 404, DRIVE_DISCONNECTED → 503, others → 500). activeScans map stays clean (registeredId null → finally skip works). Closes #60

D) app.onError maps typed errors (Rule/Drive/Quarantine/Integrity/Catalog/ Scan) to structured statuses; unknown errors → 500 { error: 'internal' } with the original logged. Removed the (err as Error).message leaks at the three previously scattered sites. B) parseJsonBody helper. All POST/PUT/PATCH handlers go through it; malformed JSON → 400 { error: 'invalid-json' }. C) zod validators for Settings, CreateRoleInput, UpdateRoleInput in packages/engine/src/api/validators.ts (kept out of shared/ to avoid widening the dep surface). POST /api/rules wraps rules.create() so RuleError surfaces as 400 via onError. A) Number.isFinite guards on /api/files and /api/batches limits. Closes #68

… help, mediainfo path, shutdown logging - parseArgs splits on first '=' so --catalog=/foo works - --version / -V prints package version, exits 0 - Unknown command prints help on stderr after the error line - mediainfoPath default resolved from import.meta.url (works outside cwd) - shutdown() try/catches each teardown step and logs failures Closes #69

…on test) foreign_key_check was running AFTER tx() committed, so an FK-violating migration left schema_version permanently advanced and silently skipped the bad migration on next boot. Moved the check inside the transaction so a violation rolls back both the migration and the schema_version insert. Replaced the inline-copy test with one that runs migrate() against a fixture migrations dir holding a deliberately-broken migration. Closes #59

quarantine and cleanup guards used case-sensitive string comparison on a case-insensitive NTFS filesystem (README primary target). Centralised in drives/paths.ts. On win32 both sides lowercase before compare; POSIX unchanged. Quarantine guard, empty-dir sweep, and api server's isPathUnderAny all use the same helper now. Closes #64

…ard, tiebreaker, date invariant - picomatch compiled once per enabled rule at firstMatch call site (was once per (file,rule) pair); globs hoisted into a compiled[] map before the loop - file.path backslashes always normalised to '/' before picomatch evaluation so a single pattern works on both POSIX and Windows paths - Malformed pathGlob (strictBrackets: true) throws RuleError('INVALID_GLOB') at precompile time; bubbles to 400 via API onError handler - Migration 0007 adds created_at to rules; repo ORDER BY now priority ASC, created_at ASC, name ASC for deterministic equal-priority tiebreaker - JSDoc'd dateBefore/dateAfter as ISO-8601 lex-compare invariant; pathGlob documents forward-slash convention and normalisation behaviour - Boundary test: fileDate === dateBefore → no match; fileDate < dateBefore → match Closes #67

… injectable clock - SettingsRepo.load() validates the row against zod SettingsSchema; malformed fields fall back to defaults with a warn log - Optimizer stores lastOptimizedAt inside the Settings JSON blob, removing the second writer on the settings table - Optimizer.shouldRun accepts an optional now() clock so tests don't need vi.useFakeTimers for this concern Closes #72

…itest 4 deferral noted - exifr: pinned, code comment documents the stay-on-exifr decision and alternatives considered (split-package @exifr/parse, subprocess-heavy exiftool-vendored, unmaintained node-exif). Last npm release: 2022-05-01T21:24:18.198Z. - piexifjs: removed in favour of sharp's withMetadata({ exif }) — spike succeeded; fixture builder now uses sharp's native EXIF write path. DateTimeOriginal round-trips correctly via exifr.parse({ reviveValues: false }). piexifjs.d.ts also deleted. - vitest: comment in vitest.workspace.ts noting defineWorkspace → projects migration deferred to its own effort. Pinned at ^2.1.0 (resolves to 2.1.9), latest is 4.1.6. - vite/jsdom: excluded from scope per spec. Closes #73

…le→rename The MediaInfo binary fetched from mediaarea.net was used without integrity verification — a future origin compromise (or MITM on a proxy that re-signs TLS) would land arbitrary code in packages/engine/bin/mediainfo. Now pins MEDIAINFO_ZIP_SHA256, verifies after download, refuses to install on mismatch with expected/actual printed. Zip downloads as .partial and renames only after verification. SHA-256 captured 2026-05-19 from MediaInfo_CLI_24.06_Windows_x64.zip. Closes #75

…ype-checked ESLint, noImplicitOverride - actions/setup-node@v4 now has cache: 'npm' - Lockfile fallback dropped — missing lockfile fails loudly - node-version pinned to '22.x' - tsconfig: noImplicitOverride: true - ESLint: projectService + no-floating-promises + no-misused-promises. Full recommendedTypeChecked not enabled — no-unsafe-* produces excessive noise on legitimate untyped external libs (exifr, better-sqlite3 raw rows). checksVoidReturn.attributes:false suppresses Preact onClick false positives. Targeted fixes applied: void-operator on 14 floating promises, fix setTimeout(asyncFn) in scans, type-alias two empty interfaces. Closes #76

…ows-skip + hostile-filename test Three test-helpers replace duplicated boilerplate: - silentLogger() replaces writes:string[] = [] shim sites - waitForScanStatus(db, scanId, target) replaces polling loops - drain(asyncIterable) replaces for-await/void patterns Plus: - metadata-video shim test skipped on win32 - walker test collects entries + asserts length (was 'pass with zero assertions' on empty iterable) - template.test.ts adds hostile filename cases (slashes / backslashes) Closes #77

…gaps Code-review feedback batch: - Extracted SettingsSchema out of api/validators.ts into catalog/settings-schema.ts; catalog/settings-repo.ts no longer imports upward - Removed no-op try/catch wrapper around Promise.race in POST /api/scans (Hono's onError already handles propagation) - app.onError fallback now writes structured JSON to stderr instead of console.error, matching the rest of the engine's log shape - finishScan emits log.info(`scan-${status}`) instead of if/else, eliminating a 'failed' status being logged as 'scan-completed' - Added 4 missing tests: reconcile quarantine_path-missing → failed, scan writes stat.ino as ntfs_file_id, dateAfter boundary symmetric to dateBefore (2 cases)

…r extraction - Remove unimported test-helper waitForScanStatus (was speculative) - Remove unread lastTickAt field on ThrottleScheduler (write-only state) - Document why moveSameDrive uses row.sha256 as post_hash (rename atomicity preserves bytes; cross-drive path differs because pipeline copies can corrupt mid-stream) - Narrow parseJsonBody catch to SyntaxError so non-parse exceptions propagate to onError instead of being mis-mapped to invalid-json - Extract makeCapturingLogger to test-helpers; metadata-image.test.ts and metadata-video.test.ts now import the shared helper

SQLite forbids non-constant defaults in ALTER TABLE ADD COLUMN. The DEFAULT CURRENT_TIMESTAMP form passed on the Linux SQLite shipped by better-sqlite3 (3.53.0) but failed on the stricter Windows-prebuilt SQLite ("Cannot add a column with non-constant default"), wedging the first run on Windows before the server could start. The migration now uses a constant '' default and backfills real ISO timestamps in the same statement. RulesRepo.create() supplies created_at explicitly on every insert, so the placeholder default is never observed in practice — the column is effectively always populated with a real timestamp at row creation time. Adds a regression test asserting RulesRepo.create() lands a non-null created_at within wall-clock bounds of the call.

…ecovery Double-click launcher for non-terminal users on Windows. First run does npm install, fetch-binaries, build; subsequent runs are fast. The script handles two cross-OS development landmines: * Marker file (.installed-by-start-bat) stamped after a successful setup. If absent on next launch — typically because a WSL build on the shared bind mount repopulated node_modules — the script wipes and reinstalls cleanly for Windows. * Per-package recovery (:ensure_native_modules) catches the lighter cases that don't blow the marker away: better-sqlite3's compiled .node from the wrong OS, and npm bug #4828 leaving the rollup platform-binding optional dep uninstalled.

curtyo18 added 29 commits May 18, 2026 23:16

Navigation Menu

Resolve 26 spec items from 2026-05-17 multi-agent review - #79

Resolve 26 spec items from 2026-05-17 multi-agent review#79
curtyo18 wants to merge 29 commits into
mainfrom
chore/2026-05-issue-batch

curtyo18 commented May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

curtyo18 commented May 19, 2026

Summary

What changed (by domain)

Closes

Test plan (manual verification)

Scans

Organize / Undo

Dedupe

Reconcile (crash recovery)

CLI

API

Catalog integrity

Throttle

Known follow-ups (from the code-review audit)

Stats

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant