feat: core/drift.ts + core/differ.ts — three-way merge engine (#11)

[breferrari]

Closes #11. Milestone 3 of the ROADMAP — the update engine that makes ShardMind different from every other "stamp-out-a-template" tool.

Summary

• source/core/drift.ts — detectDrift() classifies every tracked file into managed / modified / volatile / missing / orphaned by sha256-comparing disk content against state.json.rendered_hash. Non-recursive orphan scan: parent directory of any tracked file = tracked directory; files in a tracked dir not in state.files = orphans. Engine scaffolding (shard-values.yaml, .shardmind/) and third-party metadata (.git/, .obsidian/) are excluded at the source. Reads are bounded by mapConcurrent(items, 32, fn) so large vaults can't exhaust file descriptors.
• source/core/differ.ts — computeMergeAction() per spec §4.9: skip when base === ours, overwrite silently for managed files, three-way merge for modified files. threeWayMerge() uses node-diff3's diff3MergeRegions so stable regions with buffer === 'a'|'b' can be classified as auto-merged vs 'o' unchanged, which flat diff3Merge can't discriminate. A LineInterner maps each unique line to an integer-named token before diff3 sees it — this sidesteps a latent upstream crash where lines matching Object.prototype member names (constructor, __proto__, toString, …) collide with the library's internal {} hash map. Filed upstream as bhousel/node-diff3#86 with fix PR #87; removal of our workaround tracked in Drop LineInterner workaround once node-diff3 ships the prototype-lookup fix #49.
• source/core/renderer.ts — exposes renderString() for in-memory template rendering (no disk I/O) via a lazy isolated nunjucks.Environment. renderFrontmatterSafely() auto-recovers when naive render produces YAML-hostile output (e.g. owner: {{ name }} with name = "Alice: AI researcher") by re-rendering with every string leaf JSON-encoded; non-string values keep their YAML type. A WeakMap seen-set guards the walker against circular references in values.
• source/runtime/errors.ts — typed ErrorCode string union (39 codes, grouped by domain). ShardMindError.code is now strictly typed; typos surface at compile time. Exported from shardmind/runtime for hook consumers.
• source/runtime/errno.ts — shared errnoCode / isEnoent with runtime typeof === 'string' guard. Collapsed 8 copies of the err instanceof Error && 'code' in err ? ... pattern across core + runtime.
• CRLF tolerance — threeWayMerge splits on /\r?\n/, so a user's Windows-saved file (CRLF theirs) doesn't fight LF base/ours rendered by the engine. Merged output is always LF.
• Cross-OS CI matrix — {ubuntu, windows, macos} × {node 22, node 24} with fail-fast: false and defaults.run.shell: bash. Windows caught the CRLF regression.
• Conflict markers use git vocabulary: <<<<<<< yours / ======= / >>>>>>> shard update.

Adversarial hardening

Deliberately pushed the engine against 16 attack categories. Four real bugs surfaced and were fixed:

1. node-diff3 crash on Object.prototype line content — fixed via LineInterner (and filed upstream).
2. YAML-hostile frontmatter values crashing render — fixed via auto-recovery.
3. Circular references in values stack-overflowing the recovery walker — fixed with WeakMap seen-set.
4. Stats over-counted by 1 on newline-terminated content — fixed by subtracting the trailing-empty token post-loop.

Twelve other categories held up: line endings (CRLF/LF/CR/mixed/no-trailing), conflict-marker injection (user content containing the marker strings), unicode/BOM/null bytes, size extremes (10K chars, 5K lines, many conflicts), idempotence & convergence, conflict boundaries (start/end/whole-file), drift races, exotic value types (Date/Function/Symbol), drift performance (500 files in <5s), token interning stress, whitespace-sensitive diffs, stats bookkeeping.

Test plan

• npm test — 300 tests pass across 26 files
• npm run typecheck — strict mode clean, zero any / as any / @ts-ignore
• npm run build — runtime bundle 7.66 KB (well under the 30 KB target)
• 20 fixture scenarios (17 spec + 3 edge cases: empty file, UTF-8 non-ASCII, frontmatter merge on modified ownership)
• 12 direct detectDrift unit tests covering every DriftReport bucket including 5 orphan scenarios
• 7 direct threeWayMerge unit tests pinning down stats accounting (caught a bug the fixture suite missed)
• 6 fast-check property invariants × 200 cases = 1 200 generative scenarios (caught the node-diff3 prototype crash)
• 60+ adversarial stress tests across 16 attack categories
• 2 CRLF-robustness tests
• Cross-OS CI matrix green on all 6 jobs
• ROADMAP.md Milestone 3 fully checked; issue core/drift.ts — orphan detection (v0.2) #47 (orphan detection) pulled forward and closed
• Spec docs (docs/ARCHITECTURE.md §17, docs/IMPLEMENTATION.md §4.8/§4.9) updated to match implementation
• CHANGELOG.md [Unreleased] section populated
• Two rounds of Copilot review addressed with disposition + commit refs on every thread

Tracking

• Upstream workaround removal: Drop LineInterner workaround once node-diff3 ships the prototype-lookup fix #49 (drop LineInterner once fix(LCS): use Object.create(null) so prototype members don't shadow lookups bhousel/node-diff3#87 ships).
• v0.2 Engine polish bullet in ROADMAP mirrors Drop LineInterner workaround once node-diff3 ships the prototype-lookup fix #49.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Implements ShardMind’s Milestone 3 “merge engine” by adding drift detection, a three-way merge planner, and an in-memory rendering entrypoint needed by the differ to render cached/new templates without touching disk.

Changes:

• Added detectDrift() to classify tracked files into managed/modified/volatile/missing (with orphaned stubbed for v0.2).
• Added computeMergeAction() + threeWayMerge() using node-diff3 merge regions and Git-style conflict markers.
• Exposed renderString() with a lazy, isolated Nunjucks environment and added/expanded unit tests for renderer + drift/differ behaviors.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/unit/renderer.test.ts	Adds unit coverage for renderString (frontmatter normalization, error behavior, custom env).
tests/unit/drift.test.ts	Unskips and updates fixture runner to dispatch orchestration-level cases and validate conflict markers.
tests/unit/drift-classification.test.ts	New direct unit tests covering all detectDrift buckets independently of merge fixtures.
source/core/renderer.ts	Adds renderString() and shared Nunjucks options with a lazily created isolated environment.
source/core/drift.ts	Introduces drift classification via sha256 comparison + volatile/missing handling; orphaned deferred.
source/core/differ.ts	Introduces merge-action computation and a region-based three-way merge with stats + conflict regions.
ROADMAP.md	Marks Milestone 3 complete and adds deferred orphan detection item (#47).

[Copilot]

threeWayMerge splits input on "\n" only. If the on-disk file uses CRLF ("\r\n"), each line in theirsLines will retain a trailing "\r", which can create spurious diffs/conflicts and inaccurate merge stats when compared against base/ours rendered with LF. Normalize line endings (e.g., convert CRLF→LF for all three inputs or split on /\r?\n/) before diff3 to make merges stable across platforms/editors.

[breferrari]

Good catch — valid concern, fixed in 31c9e04.

threeWayMerge now splits all three inputs on /\r?\n/, so a CRLF theirs on Windows no longer fights LF base / ours (renderer output is always LF). Merged output stays LF; a comment on the split documents the choice and flags that callers should convert to platform-native line endings at the write boundary if ever needed.

Covered by tests/unit/differ-line-endings.test.ts — one test for identity (CRLF theirs matching LF base/ours → no conflict) and one for a genuine conflict that only CRLF-normalization could reveal.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.

[Copilot]

detectDrift() uses Promise.all over every entry in state.files (and over trackedDirs for orphan scanning). In large vaults this can trigger EMFILE/too-many-open-files errors or cause big I/O bursts. Consider processing files with a small concurrency limit (or sequentially) so drift detection remains reliable under high file counts.

[breferrari]

EMFILE risk is real at scale. Fixed in df54469: added a mapConcurrent(items, 32, fn) helper that caps in-flight file handles at 32 while preserving input order. 32 stays well under typical OS fd limits (macOS defaults to ~256) while still saturating disk on realistic vaults.

[Copilot]

FileState.ownership uses the literal 'user' (see runtime/types.ts), while docs/IMPLEMENTATION.md §4.8 describes 'volatile', and this module maps 'user'→reported 'volatile'. This naming split is easy to miss for future contributors; consider aligning the persisted state value with the reporting vocabulary (or updating the spec/docs to explicitly call out the 'user' persisted value and why).

[breferrari]

Already addressed — the 'user' → 'volatile' naming gap is documented both in the inline comment at drift.ts:72-75 and in docs/IMPLEMENTATION.md §4.8 (updated in cabe217). Leaving both names as-is: 'user' in the persisted state because that's what the install engine writes; 'volatile' in the reporting layer because that's what the update command displays. Renaming either breaks backwards compatibility with existing state.json files.

[Copilot]

This PR implements orphan detection (detectOrphans/scanDirForOrphans) and adds orphan-focused unit tests, but the PR description + linked issue #47 state orphan detection is deferred and detectDrift returns orphaned: []. Please reconcile the intended scope by either updating the PR description/issues/docs to reflect the implemented behavior, or reverting orphan detection to the deferred stub if v0.1 shouldn’t surface orphans yet.

[breferrari]

Stale — orphan detection was pulled into v0.1 during review and the scope was reconciled. PR body was refreshed, ROADMAP.md Milestone 3 bullet tracks the v0.1 landing, and issue #47 is closed with the rationale (e5304dd). detectDrift now returns a populated orphaned array, which matches what the tests exercise.

[Copilot]

threeWayMerge() splits on /\r?\n/, which will produce a trailing "" element when the input ends with a newline. That extra sentinel line gets counted into stats and affects ConflictRegion lineStart/lineEnd, so line accounting can be off-by-one (and tests currently allow this via >= checks). Consider normalizing to a line array that strips the final empty element while separately preserving whether the original had a trailing newline, then re-append the newline when joining the merged output.

[breferrari]

Good catch on the stats inflation. Fixed in df54469 — when all three inputs end with \n, split(/\r?\n/) produces a trailing "" that diff3 emits as a stable region and inflates linesUnchanged by 1. After the merge loop, we now detect this and subtract one from linesUnchanged.

Kept the trailing "" flowing through diff3 itself rather than stripping it — a newline-status difference between theirs and ours is a legitimate auto-merge concern, not a pre-merge normalization. My first attempt stripped + re-appended based on ours' style, which broke the property test "theirs-only-changed ⇒ output equals theirs". The stats-only correction preserves merge semantics and fixes the off-by-one in reporting.

[Copilot]

Pull request overview

Copilot reviewed 44 out of 47 changed files in this pull request and generated 4 comments.

[Copilot]

In scanDirForOrphans, UNSCANNED_DIRS is meant to prevent scanning directories like .shardmind/, .git/, .obsidian/, but the check is currently UNSCANNED_DIRS.has(entry.name) after entry.isFile(), so it never matches (those are directories, and entry.name is just the filename). This also means a tracked directory that is one of these (e.g. if state ever contains a path under .git/) would still be scanned, contradicting the doc comment/spec.

Suggested fix: remove the entry.name check and instead short-circuit at the directory level (e.g. in scanDirForOrphans, if (UNSCANNED_DIRS.has(relDir.split('/')[0] ?? relDir)) return []; or check relDir === '.git' / startsWith('.git/') etc.).

[breferrari]

Correct — the UNSCANNED_DIRS.has(entry.name) check ran after entry.isFile() filtered out directories, so it was effectively dead code.

Fixed in df54469 by moving the check to detectOrphans where trackedDirs is derived: any directory whose top segment is .shardmind, .git, or .obsidian is dropped from the set before any readdir is issued. If a shard somehow tracks .git/HEAD, that whole directory is skipped at the source. Renamed the constant to UNSCANNED_DIR_NAMES to reflect the new role.

[Copilot]

getDefaultStringEnv constructs new nunjucks.Environment(null, ...). Passing null as the loader list is risky because Environment expects undefined, a loader, or an array of loaders; null can behave differently across Nunjucks versions and may throw if internal code assumes an array/object.

Suggested fix: pass undefined (omit the first argument) or [] as the loaders argument to explicitly represent “no loaders”, keeping the env truly string-only and avoiding potential runtime incompatibilities.

Suggested change

	defaultStringEnv = new nunjucks.Environment(null, NUNJUCKS_OPTS);
	defaultStringEnv = new nunjucks.Environment([], NUNJUCKS_OPTS);

[breferrari]

Fair — swapped to new nunjucks.Environment([], NUNJUCKS_OPTS) in df54469. Added a comment noting the empty loader array explicitly represents 'no filesystem resolution', which is the intent for in-memory string rendering.

[Copilot]

loadFiles treats expected-output.md as optional, but the catch { /* no-op */ } currently swallows all read errors (permissions, path mistakes, etc.), which can mask real fixture/test setup problems.

Suggested fix: only ignore ENOENT (file missing) and rethrow other errors so the test fails loudly when the fixtures directory is misconfigured.

[breferrari]

Right — the blanket catch was swallowing permissions/path errors too. Narrowed to ENOENT in df54469; any other read failure now throws so fixture-setup problems surface loudly.

[Copilot]

errnoCode is typed to return string | undefined, but Node’s ErrnoException.code can be string | number. The current implementation returns (err as { code?: string }).code without verifying the runtime type, so it can return a number while claiming it’s a string, which is an unsound contract and can lead to confusing error messages/comparisons.

Suggested fix: either (a) widen the return type to string | number | undefined, or (b) keep the string | undefined API but explicitly guard typeof code === 'string' before returning.

Suggested change

	return (err as { code?: string }).code;
	const code = (err as { code?: unknown }).code;
	return typeof code === 'string' ? code : undefined;

[breferrari]

Good defensive catch. Fixed in df54469 with the suggested typeof === 'string' guard. Node's types document code as string, but a non-string on a custom thrown object would otherwise silently violate our return type.

[Copilot]

Pull request overview

Copilot reviewed 49 out of 53 changed files in this pull request and generated 3 comments.

[Copilot]

detectOrphans() normalizes tracked directories to posix separators, but trackedPaths itself is left unnormalized. If state.files ever contains Windows-style keys (e.g. nested\\file.md), scanDirForOrphans() will generate nested/file.md and fail the trackedPaths.has(rel) check, incorrectly reporting a tracked file as an orphan. Normalize trackedPaths to a consistent separator (or maintain a parallel normalized Set for orphan detection) so tracked files never appear in orphaned even with mixed separators.

[breferrari]

Fixed in df54469 — trackedPaths is now normalized to forward slashes on construction, so the trackedPaths.has(rel) check inside scanDirForOrphans works whether state.files was written with posix or native separators. install-executor already emits posix, but the defensive normalization costs nothing and removes a silent-failure mode.

[Copilot]

The comment says “See the block comment on LINE_SPLIT for why this is load-bearing”, but the load-bearing rationale is the prototype-collision workaround (line interning), not the line-splitting regex. This is misleading when maintaining the code; consider updating the reference to point at the interning/prototype-collision comment instead.

Suggested change

	// Intern every unique line to an integer-named token. See the block
	// comment on LINE_SPLIT for why this is load-bearing.
	// Intern every unique line to an integer-named token. This is
	// load-bearing because the interning/prototype-collision workaround
	// depends on diffing tokens rather than raw line strings.

[breferrari]

Stale reference fixed in df54469. Updated the comment to point at the LineInterner/prototype-collision rationale (which is the load-bearing part); LINE_SPLIT is just about CRLF tolerance.

[Copilot]

The comment claims differ.ts prefixes every line with U+0001 and then strips it, but the current implementation uses integer token interning (no sentinel prefix). This makes the motivation for these tests inaccurate and harder to trust. Update the comment to match the actual interning approach (or drop the sentinel rationale) while keeping the round-trip assertions.

[breferrari]

Rewrote the describe block in df54469 to describe the current LineInterner-based approach. Kept the round-trip tests as a regression guard — they prove user content containing U+0001 survives the merge, which an earlier sentinel-prefix implementation would have mangled. Also dropped two .not.toContain('\u0001') assertions elsewhere that were sentinel-era leftovers.