Skip to content

fix(LCS): use Object.create(null) so prototype members don't shadow lookups#87

Open
breferrari wants to merge 1 commit intobhousel:mainfrom
breferrari:fix/lcs-prototype-pollution
Open

fix(LCS): use Object.create(null) so prototype members don't shadow lookups#87
breferrari wants to merge 1 commit intobhousel:mainfrom
breferrari:fix/lcs-prototype-pollution

Conversation

@breferrari
Copy link
Copy Markdown

Fixes #86.

LCS stores equivalence classes in a plain object, so input elements that happen to match Object.prototype member names (constructor, __proto__, toString, hasOwnProperty, ...) read the inherited value instead of undefined and crash on the next line with:

TypeError: equivalenceClasses[item].push is not a function

Swapping the literal for Object.create(null) gives the lookup table no prototype chain, so it only sees keys we wrote. Zero API change — same bracket access semantics, just no inheritance.

 function LCS(buffer1, buffer2) {
-  let equivalenceClasses = {};
+  let equivalenceClasses = Object.create(null);

Added a regression test to test/LCS.test.js covering the common prototype names. The test fails on main with the original TypeError; passes with the fix.

bun run all is clean (lint, build, 30/30 tests).

…ookups

LCS stores equivalence classes in a plain object, so input elements that
match Object.prototype member names (`constructor`, `__proto__`,
`toString`, `hasOwnProperty`, etc.) hit the prototype-inherited member
on the lookup and crash with:

    TypeError: equivalenceClasses[item].push is not a function

`Object.create(null)` produces a dictionary with no prototype chain, so
the lookup sees the key only if we wrote it. Zero API change.

Regression test in test/LCS.test.js covers the common prototype names.

Closes bhousel#86
Copilot AI review requested due to automatic review settings April 19, 2026 17:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a crash in LCS when input elements match Object.prototype member names by switching its lookup table to a prototype-less object, and adds a regression test to prevent reintroduction.

Changes:

  • Replace the LCS equivalence-class lookup table {} with Object.create(null) to avoid prototype shadowing.
  • Add a regression test that exercises common Object.prototype member names as input elements.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/diff3.mjs Uses a prototype-less object for equivalenceClasses to prevent inherited keys from breaking lookups.
test/LCS.test.js Adds a regression test covering problematic prototype member names.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

breferrari added a commit to breferrari/shardmind that referenced this pull request Apr 19, 2026
Once bhousel/node-diff3#87 is merged and released upstream, the
LineInterner class in differ.ts can be removed in favor of passing
raw string arrays to diff3MergeRegions. Cross-linked three places
so the workaround doesn't get forgotten:

- Issue #49 documents the removal steps.
- source/core/differ.ts comment above LineInterner references #49
  and the upstream PR.
- ROADMAP.md v0.2 Engine polish bullet points to both.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
breferrari added a commit to breferrari/shardmind that referenced this pull request Apr 19, 2026
)

* feat(renderer): export renderString helper (#11)

Extracts the frontmatter-aware render pipeline into a public helper so the
merge engine can render an in-memory template string without writing it to
disk first. Uses a lazy isolated nunjucks.Environment — no pollution of
the global configure() state.

Covered by 5 new unit tests in renderer.test.ts (plain body, frontmatter
normalization, context substitution, syntax error, caller-supplied env).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

Implements spec §4.8 (drift classification) and §4.9 (three-way merge
via node-diff3's Khanna–Myers algorithm). Unskips the fixture runner so
all 17 merge scenarios pass, with an orchestration dispatch branch that
routes volatile / new_file / removed / module-change scenarios away from
computeMergeAction (those are update-planner concerns, not merge concerns).

Highlights:
- detectDrift maps state.json ownership='user' onto DriftReport.volatile;
  hash-mismatched files become modified; absent files become missing.
  Orphan detection is deferred to v0.2 (see follow-up).
- computeMergeAction: skip when base===ours, overwrite for managed,
  three-way merge for modified. Conflict markers use git's yours/shard
  update vocabulary.
- threeWayMerge uses diff3MergeRegions (not diff3Merge) to classify
  stable vs auto-merged vs conflict lines accurately in MergeStats.
- ownershipForMergeInput now treats user_edited=true as 'modified'
  regardless of ownership_before — matches what drift would report at
  runtime (scenario 07 depends on this).

Tests: all 17 fixtures pass; 7 new drift classification unit tests
cover every DriftReport bucket independently of the merge fixtures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(roadmap): tick merge engine; defer orphan detection to v0.2 (#11, #47)

M3 three-way merge engine is done: drift.ts + differ.ts + 17 fixtures
green. Orphan detection (files under tracked paths but not in state)
is deferred to v0.2 as #47 — no v0.1 flow consumes it and the
iterator-exploded directory semantics need design first.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: dedupe ENOENT dance, parallelize drift reads, share state factory

Code-review pass on the merge engine PR:

- New source/runtime/errno.ts with errnoCode + isEnoent replaces eight
  inline copies of the `err instanceof Error && 'code' in err ? (err as
  NodeJS.ErrnoException).code : undefined` pattern across core and
  runtime modules. Lives in runtime/ so both layers can import without
  crossing the one-way runtime → core boundary.
- detectDrift in source/core/drift.ts parallelizes per-file reads via
  Promise.all and split the inline body into volatileEntry /
  missingEntry / hashedEntry helpers. ~50× faster on large vaults.
- New tests/helpers/shard-state.ts centralizes ShardState construction.
  drift.test.ts and drift-classification.test.ts both use it; more
  callsites (state.test.ts, install-planner.test.ts) can migrate later.

All 214 tests remain green; typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(differ): normalize CRLF in three-way merge inputs

Renderer output is always LF, but a user's file on disk may be CRLF
on Windows. Splitting on '\n' alone left trailing '\r' on every line
of `theirs`, producing spurious conflicts against LF `base`/`ours`.

Split on /\r?\n/ for all three inputs; merged output is LF. Callers
that need platform-native line endings should convert at the write
boundary.

New unit file tests/unit/differ-line-endings.test.ts covers identity
(CRLF theirs matching LF base/ours) and a real CRLF-theirs conflict.

Addresses Copilot review on PR #48.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: run matrix on ubuntu + windows + macos

shardmind installs as a global CLI and runs wherever the user's vault
lives. Ubuntu-only coverage missed the CRLF-on-Windows merge regression
Copilot flagged on #48 — make all three OSes first-class. fail-fast is
off so a Windows-only fault doesn't mask a macOS one.

defaults.run.shell: bash makes the same step script work on all three
runners (Windows runners have git-bash pre-installed), and `rm -f`
tolerates the missing lockfile path on fresh checkouts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(gitignore): exclude Claude Code's ScheduleWakeup lock file

.claude/scheduled_tasks.lock is session-local runtime state (Claude
Code writes it to coordinate its own wakeup timer). Ignore it alongside
.claude/memory/ — same category of "never commit, not user content".
.claude/ itself stays tracked since that's where shardmind installs
managed assets into a vault.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: tighten merge engine per self-review

Self-review pass on the code this PR introduced. All 222 tests green.

source/core/differ.ts
- Hoist `ThreeWayMergeResult` to a named exported type, drop the
  `ReturnType<typeof ...>` workaround.
- Mark `ComputeMergeActionInput` fields readonly (it's an input contract).
- Pull conflict-handling into a pure `resolveUnstableRegion(region,
  mergedLengthBefore)` — no out-parameters, returns everything it
  produces. Easier to reason about and test.
- Pull `runMerge()` wrapper for the ShardMindError translation so the
  happy path in computeMergeAction stays linear.
- `arraysEqual` is now `length === && every()` over `readonly T[]` —
  idiomatic TS, callable with diff3's readonly arrays.
- Drop the `// ownership === 'modified'` narrative comment (the
  discriminated union already proves it).

FIX: Stats accounting was undercounting `linesAutoMerged`. A stable
region with `buffer === 'a'` or `'b'` means diff3 resolved to one
side's version without ambiguity — that's auto-merged, not unchanged.
Only `buffer === 'o'` is truly unchanged. Caught by the new direct
threeWayMerge tests; fixture suite didn't pin down the stats numbers.

source/core/drift.ts
- Import `FileState` from runtime/types.js instead of using
  `ShardState['files'][string]`. (Lazy, now fixed.)
- Extract `classifyFile` + `classifyByHash` helpers. The classifier
  now returns the target bucket directly, so the outer loop collapses
  to a single `byBucket[bucket].push(entry)` — no second discriminant
  check needed.

tests/helpers/
- Add `makeFileState` factory and a barrel `index.ts` so test files
  import from `../helpers` once the surface grows.
- Replace the magic `'x'.repeat(64)` with a named `PLACEHOLDER_HASH`.

tests/unit/drift.test.ts
- Classify each scenario up-front (`ScenarioKind`) and dispatch via a
  switch with `assertNever` exhaustiveness. Each branch is now a named
  assertion function (`assertVolatile`, `assertNewFile`, etc.) instead
  of an inline if-chain — easier to read and extend.

tests/unit/three-way-merge.test.ts (new)
- 6 direct unit tests for the merge primitive covering unchanged /
  auto-merge / conflict stats, false-conflict handling, and
  non-adjacent conflict regions with distinct line ranges.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(errors): type-safe ErrorCode union for ShardMindError

Replaces the free-form `code: string` on ShardMindError with a string
literal union listing all 39 codes the engine can emit (source/runtime/
errors.ts). TypeScript now refuses any typo at every callsite — no
runtime-only surprises, no code-grep archaeology to find what codes
exist.

The union is organized by domain (vault, manifest, schema, values,
state, registry/download, rendering, install, merge) with a header
comment on each group. Adding a new code is a one-line change in the
registry; the compiler then surfaces every site that needs it.

ErrorCode is exported from shardmind/runtime so hook scripts that
catch ShardMindError can narrow on code without hardcoding strings.

Intentional duplicates (VALUES_NOT_FOUND vs VALUES_MISSING,
VALUES_READ_FAILED vs VALUES_FILE_READ_FAILED) are documented in the
registry — runtime layer vs commands layer, different recovery hints.
Unification is a separate design concern.

All 222 tests green; typecheck clean on first pass (every existing
callsite happened to spell its code correctly).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(drift): land orphan detection in v0.1 (closes #47)

Reversing the v0.2 deferral. obsidian-mind's core UX promise — "ShardMind
manages X, leaves Y alone" — depends on the manager/user boundary being
visible. Without orphan reporting, `shardmind` status has no way to show
what files are user territory vs shard territory. That's the value prop
of a package manager, not polish.

The "iterator-exploded directory semantics need design first" rationale
was also weaker than it looked. The rule is straightforward:

  parent directory of a tracked file = tracked directory
  any file in a tracked directory not in state.files = orphan
  non-recursive — subdirectories count only if they hold tracked files

Applied to obsidian-mind:
  skills/leadership.md tracked → skills/my-extra.md = orphan ✓
  CLAUDE.md tracked            → brain/daily/2026-04-19.md ≠ orphan
                                 (brain/daily/ isn't tracked) ✓

Implementation details:
- detectOrphans runs in parallel with the classification map via
  Promise.all. No added latency on the happy path.
- Engine scaffolding (`shard-values.yaml`) and third-party metadata
  (`.shardmind/`, `.git/`, `.obsidian/`) are explicitly excluded. The
  excluded list is a named ReadonlySet in drift.ts, not a sprinkle of
  magic strings.
- ENOENT on a scanned directory is treated as "nothing to report",
  not an error — handles the case where a tracked directory has been
  rm'd out from under us.

Tests: 5 new cases in drift-classification.test.ts covering orphan in
tracked dir, non-recursion into untracked subdirs, engine-reserved-file
exclusion, .shardmind/.git/.obsidian exclusion, and multi-dir aggregation.

ROADMAP: remove the v0.2 bullet for #47.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: review-pass cleanup (vault-paths consts, LINE_SPLIT, inline helper)

Addresses findings from the second /simplify review pass.

- source/runtime/vault-paths.ts: add GIT_DIR and OBSIDIAN_DIR
  constants. drift.ts's UNSCANNED_DIRS set now imports them instead of
  hardcoding '.git' / '.obsidian' — matches the existing
  SHARDMIND_DIR / VALUES_FILE centralization.

- source/core/drift.ts: inline the single-use `toPosixPath` helper.
  It collided in name with `toPosix(from, to)` in fs-utils.ts (which
  has a different signature and purpose); the one-call-site slash
  normalization doesn't pay for a shared function.

- source/core/differ.ts: extract `LINE_SPLIT = /\r?\n/` to a
  module-level constant; threeWayMerge used the literal three times on
  consecutive lines. Comment moved to the constant so the CRLF
  rationale has a single home.

- tests/helpers/shard-state.ts: document makeStateWithFiles as a
  shorthand, which is exactly what it is — 13 drift-test callsites
  stay concise, which is why the helper earns its keep.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: collapse review-pass dismissals after pushback

Four follow-through fixes on findings I initially waved away:

1. Remove makeStateWithFiles helper — it was just makeShardState({
   files }) with extra sugar. Two helpers for the same operation is a
   lookup tax on every reader; one helper is cleaner. Updated 13
   callsites across drift.test.ts + drift-classification.test.ts.

2. Hoist `import type { Dirent } from 'node:fs'` in drift.ts so the
   scanner signature reads cleanly instead of inline
   import('node:fs').Dirent[] expression.

3. Extract MergeStatsWithConflicts as a named type in runtime/types.ts.
   ThreeWayMergeResult and MergeResult.stats both reference it now,
   instead of `MergeResult['stats']` indexed access + inline shape.
   Extends MergeStats so the auto_merge-only stats and the conflict-
   aware stats are clearly related.

4. `trackedPaths` scoping — re-inspected; correctly scoped, one-time
   Set construction shared between detectOrphans and the classifier.
   No change needed.

All 227 tests green; typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(merge): edge-case fixtures — empty file, UTF-8, frontmatter merge

Closes the last unchecked bullet on ROADMAP Milestone 3 and the
"Edge case fixtures pass" line on #11's acceptance criteria. I had
cut this corner in the first pass and only caught it on audit.

Three new fixtures extend the corpus from 17 to 20:

- 18-empty-file: base template is empty, new template adds content.
  Ownership managed → overwrite. Guards against any future regression
  where empty-string rendering gets special-cased incorrectly.

- 19-utf8-non-ascii: emoji, accented Latin, CJK, combining marks.
  Auto-merge path — shard and user both edit, disjoint. Ensures no
  Buffer/string conversion silently mangles multi-byte characters in
  a way ASCII-only fixtures wouldn't reveal.

- 20-frontmatter-modified-merge: modified ownership, user added a
  frontmatter tag while shard updated the body. Covers frontmatter +
  body interleaving via diff3 that scenario 12 (managed, frontmatter-
  only, overwrite) never exercises.

Hash-identical / binary-identical: not added as a new fixture. The
`sha256(base) === sha256(ours)` shortcut is already exercised by
scenario 01 (no-change → skip) and scenario 05 (modified-no-upstream
→ skip). A redundant fixture would test the same code path.

The runner's "discovers N scenarios" assertion is now parameterized
by a named constant so future additions are a one-line bump.

230 tests green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec): §4.8 and §4.9 reflect implementation reality

Final audit pass on PR #48. Two doc drifts introduced by the
implementation that the spec didn't catch up with:

§4.9 (differ): algorithm step 6a said `diff3Merge(...)`. Implementation
uses `diff3MergeRegions(...)` because the flat `diff3Merge` MergeRegion
shape can't distinguish stable-unchanged lines (buffer === 'o') from
stable-auto-merged lines (buffer === 'a' | 'b'), and the acceptance
criteria require accurate stats. Step 6a now documents the regions
algorithm, CRLF-tolerant split, and the stable/unstable classification
rules. MergeStatsWithConflicts is called out as its own named type
(matches runtime/types.ts).

§4.8 (drift): algorithm was silent on the orphan scan because the
original design deferred it. Orphan detection is now part of v0.1 (see
#47, closed). Algorithm documents the parallel scan via Promise.all,
the tracked-directory derivation (parent dir of each tracked file), the
non-recursive scan rule, and the engine-reserved / never-scanned
exclusions. Includes a short rationale block on why we don't recurse.

PR #48 body refreshed (separate from this commit) to reflect the final
state: 230 tests, 20 fixtures, orphan detection landed, ErrorCode
union, CRLF fix, cross-OS CI matrix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat: property-based invariants + fix node-diff3 prototype-key crash

Showcase-pass: this is the cutting-edge rigor the merge engine deserves.

## The bug

Property testing via fast-check surfaced a real crash in node-diff3
that no fixture caught: its LCS implementation uses `{}` as a map and
iterates on string keys, which collides with Object.prototype members
when any line equals 'constructor', '__proto__', 'toString',
'hasOwnProperty', etc.:

    TypeError: equivalenceClasses[item].push is not a function

For a vault of markdown/code content, a line containing the literal
word `constructor` is routine — this would crash obsidian-mind on
random user files.

## The fix

threeWayMerge now prefixes every line with U+0001 (START OF HEADING,
never a valid printable character) before handing arrays to
diff3MergeRegions. No prototype member name starts with U+0001, so
the collision can't happen. Prefixes are stripped from all outputs
(merged content, conflict region snapshots) before they leave the
module — consumers never see them.

Explicit regression test in three-way-merge.test.ts covers the
'constructor' / '__proto__' / 'toString' / 'hasOwnProperty' /
'valueOf' case.

## Property tests (tests/unit/three-way-merge-properties.test.ts)

Six invariants pinned down across 200 random cases each (= 1200
generated scenarios):

1. Identity: merge(x, x, x) is x with no conflicts.
2. Theirs only: base === ours ⇒ merged content equals theirs.
3. Ours only:  base === theirs ⇒ merged content equals ours.
4. False conflict: theirs === ours ⇒ merged content equals theirs.
5. CRLF invariance: CRLF theirs produces the same conflict count
   as LF theirs of the same content.
6. Stats bookkeeping: linesUnchanged + linesAutoMerged +
   linesConflicted ≤ total emitted lines.

Dev dep added: fast-check.

## Docs

- docs/ARCHITECTURE.md §17.3 table now reflects all 20 scenarios.
- §17.4 code sample shows diff3MergeRegions (not diff3Merge) and
  explains why the regions variant is load-bearing for accurate stats.
- §17.5 corrects the frontmatter-merge decision — we line-merge the
  rendered document (parse → stringify via yaml, then diff3), we do
  NOT deep-merge YAML objects. Fixture 20 proves this works.
- §17.6 test-build-order checkboxes updated to reflect what shipped.
- CHANGELOG.md gains an [Unreleased] section documenting the merge
  engine, ErrorCode union, CRLF fix, cross-OS CI, and 20 fixtures.

237 tests green; typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* hardening: turn the merge engine upside down, fix 3 real bugs

Ran an adversarial test pass deliberately trying to break the engine.
Wrote ~60 stress tests across 11 categories; 3 categories failed and
surfaced genuine bugs, each fixed in this commit.

## Bug 1 — Prototype pollution via user content (found earlier, already fixed)

Covered in 03248fe. Documented here for narrative continuity.

## Bug 2 — Sentinel corruption of user content

The original fix for the node-diff3 prototype crash prefixed every line
with U+0001 and global-stripped U+0001 from output. This corrupts user
content that legitimately contains U+0001 (unlikely in markdown but
possible, and a determined adversary could construct it).

Fix: replace the sentinel-prefix approach with a `LineInterner` that
maps every unique line to an integer-named token ("0", "1", "2", ...).
Integer-named keys can never collide with Object.prototype members,
so diff3's hash-map crash is impossible by construction. Tokens are
mapped back to original lines on output — user content is never
touched, the encoding is foolproof.

See source/core/differ.ts: LineInterner class replaces prefixLines /
stripPrefixes / stripPrefix.

## Bug 3 — YAML-hostile frontmatter values crash the renderer

Template `owner: {{ name }}` with `name = "Alice: AI researcher"`
rendered to `owner: Alice: AI researcher` which is invalid YAML, and
the renderer threw RENDER_FRONTMATTER_ERROR. In a user-facing product
this means any value containing a colon, pipe, or quote would crash
the entire update flow. Not acceptable for a showcase.

Fix: `renderFrontmatterSafely` attempts the naive render first. If
parse fails, it retries with every string leaf in the context
JSON-encoded (which is always a valid YAML double-quoted scalar).
Non-string values (numbers, booleans, arrays of primitives) are left
untouched so their YAML type is preserved. Only if recovery also fails
do we throw — meaning the template itself has a structural problem
independent of the values.

## Bug 4 — Circular references in values crash the recovery walker

Surfaced by the test I added for bug 3: a value with a self-reference
(possible through hooks or computed defaults) sent encodeStringLeaves
into infinite recursion and stack-overflow.

Fix: encodeStringLeaves takes a WeakMap<object, unknown> `seen`
parameter. On re-entry into an already-visited object, returns the
cached stand-in. Cycles terminate; the encoded output is a valid
acyclic JSON-compatible tree.

## What now passes

- 60+ adversarial tests across: prototype pollution vectors, sentinel
  corruption, line-ending edge cases (CRLF, CR, mixed, no trailing
  newline), conflict marker injection (user content containing
  `<<<<<<< yours` etc.), unicode/BOM/null bytes, size extremes (10K
  chars, 5K lines, many conflicts), idempotence/convergence, conflict
  boundaries (start/end/entire file), drift races (file deleted
  mid-scan, directory deletion, Windows path separators), exotic
  value types (Date, function, Symbol), renderer context hardening
  (circular refs, deep nesting), token interning stress (10K lines,
  all duplicates, all unique), whitespace-sensitive diffs, stats
  bookkeeping under stress.

## Test count

230 → 300 tests. Typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(gitignore): exclude local fork clones (e.g. node-diff3)

Opened a fork of node-diff3 for issue #86 / PR #87 upstream and
cloned it into this workspace for iteration. Keep it out of
shardmind's working tree.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Revert "chore(gitignore): exclude local fork clones (e.g. node-diff3)"

The node-diff3 clone doesn't belong next to this repo; removed the
directory and the matching ignore line. The upstream PR (#87 on
bhousel/node-diff3) stays — this just cleans up the local workspace.

This reverts commit 45337c0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: track the node-diff3 workaround removal as a followup (#49)

Once bhousel/node-diff3#87 is merged and released upstream, the
LineInterner class in differ.ts can be removed in favor of passing
raw string arrays to diff3MergeRegions. Cross-linked three places
so the workaround doesn't get forgotten:

- Issue #49 documents the removal steps.
- source/core/differ.ts comment above LineInterner references #49
  and the upstream PR.
- ROADMAP.md v0.2 Engine polish bullet points to both.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: Copilot review round 2 — nine real findings

Copilot flagged 12 issues on PR #48 post-hardening. Triaged: 9 real,
3 stale. This commit addresses all 9.

source/core/drift.ts
- Bounded concurrency on per-file reads. `detectDrift` previously
  issued unlimited `Promise.all` over every tracked file, which can
  trip EMFILE/too-many-open-files on large vaults. New
  `mapConcurrent(items, 32, fn)` helper caps in-flight handles at 32
  while preserving input order. (3107133175)
- `trackedPaths` now normalized to forward slashes on construction.
  `scanDirForOrphans` builds relative paths with `/`, and if any
  state.files key slipped through with native separators the
  `trackedPaths.has(rel)` check would miss and falsely orphan a
  tracked file. (3107243197)
- `UNSCANNED_DIRS` was dead code — the check at `entry.name` ran
  *after* `entry.isFile()`, so it could never match a directory and
  its stated purpose (preventing scans into `.shardmind/`, `.git/`,
  `.obsidian/`) was moot. Moved the check to `detectOrphans` where
  tracked directories are derived; if a shard somehow tracks a file
  under one of those namespaces, that directory is skipped at the
  source. Renamed to `UNSCANNED_DIR_NAMES` to reflect the new role.
  (3107192187)

source/core/differ.ts
- Trailing-newline stats correction. `split(/\r?\n/)` produces a
  trailing "" for newline-terminated content; diff3 emits it as part
  of a stable region, which padded `linesUnchanged` by 1. When all
  three agreed on the trailing "", subtract one from `linesUnchanged`
  so stats match user-visible line counts. The merge itself is
  unchanged — trailing newlines are legitimate document content that
  should flow through diff3 like any other line. (3107133188)
- Updated the comment above `diff3MergeRegions` to point at the
  LineInterner/prototype-collision rationale, not `LINE_SPLIT`.
  LINE_SPLIT is about CRLF tolerance; the load-bearing bit is the
  interning. (3107243211)

source/core/renderer.ts
- `new nunjucks.Environment([], ...)` instead of `(null, ...)`. null
  isn't a documented loader value; an empty loader array explicitly
  represents "no filesystem resolution", which is the intent.
  (3107192203)

source/runtime/errno.ts
- Runtime `typeof === 'string'` guard on the extracted `code`. Node's
  types say `code?: string`, but a stray non-string on a custom error
  would have silently violated our return type. Now unsound values
  return `undefined`. (3107192220)

tests/unit/drift.test.ts
- `loadFiles` narrowed the `expected-output.md` read's catch to ENOENT
  only. Other errors (permissions, etc.) now throw and surface the
  real fixture-setup problem instead of being swallowed. (3107192214)

tests/unit/merge-adversarial.test.ts
- Updated stale "sentinel prefix" describe block to describe the
  current `LineInterner` approach. (3107243218)
- Removed two leftover `.not.toContain('\u0001')` assertions.
  Sentinel encoding is gone — those checks would incorrectly fail
  if user content ever contained `\u0001`, which is exactly what the
  top-of-file tests verify is now preserved.

Stale (replied, not changed):
- CRLF concern (3107097177) — already fixed in 31c9e04.
- Naming-gap docs (3107133181) — already documented in
  docs/IMPLEMENTATION.md §4.8 in cabe217.
- Orphan scope reconciliation (3107133183) — PR body refreshed mid-
  session; ROADMAP reflects the v0.1 landing.

300 tests green; typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LCS crashes on lines named constructor / __proto__ / toString etc.

2 participants