fix(merge): don't render copy-origin files in three-way merge (#132)

CHANGELOG.md

@@ -8,6 +8,16 @@ Between releases: see `git log` for merged work and [`ROADMAP.md`](ROADMAP.md) f
 
 ## [Unreleased]
 
+### Fixed (update merge rendered copy-origin files — #132)
+
+Closes [#132](https://github.com/breferrari/shardmind/issues/132); root cause **reported via [#129](https://github.com/breferrari/shardmind/issues/129)** (thanks!).
+
+- **`shardmind update` no longer crashes** when a modified copy-origin file (any non-`.njk` file — scripts, `.test.ts`, JSON) contains a literal `{{` that isn't a valid Nunjucks expression. The three-way merge (`source/core/differ.ts`) was rendering both sides of *every* modified file through Nunjucks; in v6 only `.njk` files are templates, so copy files were being rendered when they shouldn't be — which crashed on a literal `{{` and **silently substituted** any real `{{ expr }}` a copy file legitimately contained.
+
+- **Fix:** `computeMergeAction` takes a `literal` flag; `update-planner` sets it for copy-origin files (those carrying `copyFromSourcePath`). When set, the merge runs on the raw bytes (`base = oldTemplate`, `ours = newTemplate`, no render). The renderer stays strict, so genuine `.njk` template syntax errors still fail loudly with `RENDER_TEMPLATE_ERROR` — a renderer-level "swallow the parse error" approach would have masked them. Binary copy files through the utf-8 merge remain tracked by [#63](https://github.com/breferrari/shardmind/issues/63).
+
+- **Tests:** merge fixture 21 (`tests/fixtures/merge/21-copy-origin-literal-braces/`, authored RED-first) proving the copy file merges without rendering and both literals survive; unit tests for `literal` raw-merge + conflict and a regression guard that a broken `.njk` still throws; an end-to-end `update` integration test. **Docs:** `docs/IMPLEMENTATION.md §4.9`, `docs/SHARD-LAYOUT.md`.
+
 ### Added (adopt batch operations — #120)
 
 Closes [#120](https://github.com/breferrari/shardmind/issues/120). `shardmind adopt` against a divergent vault asked for a decision on *every* differing file (the flagship obsidian-mind run had 35). A top-level mode picker now resolves the whole set at once, with per-file prompting kept as a mode.

docs/IMPLEMENTATION.md

@@ -734,6 +734,7 @@ computeMergeAction(input: {
   newValues: Record<string, unknown>;
   actualContent: string;       // File on disk
   renderContext: RenderContext;
+  literal?: boolean;           // copy-origin file → merge raw bytes, don't render (#132)
 }): Promise<MergeAction>
 
 type MergeAction =
@@ -749,9 +750,12 @@ interface MergeStats {
 ```
 
 **Algorithm**:
-1. Render old template with old values → `base`
-2. Render new template with new values → `ours`
+1. Render old template with old values → `base` (**unless `literal`** — see below)
+2. Render new template with new values → `ours` (**unless `literal`**)
 3. `theirs` = `actualContent` (what's on disk)
+
+**Copy-origin files (`literal: true`, #132)**: in v6 only `.njk` files are templates; everything else is copied verbatim (`modules.ts`). The update planner sets `literal` for copy-origin files (those carrying `copyFromSourcePath`). When set, steps 1–2 **skip rendering** — `base = oldTemplate`, `ours = newTemplate` — and the three-way merge runs on the raw bytes. Rendering a copy file would (a) crash on a literal `{{` that isn't a valid expression and (b) silently substitute any real `{{ expr }}` it contains as data. The renderer itself stays strict, so genuine `.njk` authoring errors still throw `RENDER_TEMPLATE_ERROR`.
+
 4. If `sha256(base) === sha256(ours)` → no upstream change → `{ type: 'skip' }`
 5. If ownership is `managed` (base === theirs) → `{ type: 'overwrite', content: ours }`
 6. If ownership is `modified`:

docs/SHARD-LAYOUT.md

@@ -91,7 +91,7 @@ Three mechanisms.
 
 1. **Module / agent selection.** Wizard values gate which files ship. Default wizard state is **all modules enabled, all agent files shipped** — per VISION's "ships complete" posture and Invariant 1. User deselects what they don't want.
 
-2. **`.njk` Nunjucks rendering** (author-explicit opt-in by suffix). Any file ending in `.njk` is rendered with user values and the suffix is stripped on install. Author convention is to keep `.njk` to **dotfolder configs** the user doesn't see — `.claude/settings.json.njk`, `.mcp.json.njk` — so the clone-UX cost stays zero. The engine doesn't enforce that convention because iterator templates (`<dir>/_each.<ext>.njk`) and other legitimate uses produce vault-visible output. Vault-visible `{{ values.X }}` *without* the `.njk` suffix is the deferred `rendered_files` opt-in tracked under [#86](https://github.com/breferrari/shardmind/issues/86).
+2. **`.njk` Nunjucks rendering** (author-explicit opt-in by suffix). Any file ending in `.njk` is rendered with user values and the suffix is stripped on install. Author convention is to keep `.njk` to **dotfolder configs** the user doesn't see — `.claude/settings.json.njk`, `.mcp.json.njk` — so the clone-UX cost stays zero. The engine doesn't enforce that convention because iterator templates (`<dir>/_each.<ext>.njk`) and other legitimate uses produce vault-visible output. Vault-visible `{{ values.X }}` *without* the `.njk` suffix is the deferred `rendered_files` opt-in tracked under [#86](https://github.com/breferrari/shardmind/issues/86). The three-way merge on `update` honors the same split: copy-origin (non-`.njk`) files are merged on their raw bytes and are **never** re-rendered, so a literal `{{` in a script or test fixture neither crashes the merge nor gets substituted ([#132](https://github.com/breferrari/shardmind/issues/132)).
 
 3. **Lifecycle hooks.** Shard-author TypeScript split across three named slots with engine-enforced write boundaries — `bootstrap` (unmanaged-path setup: QMD bootstrap, `git init`), `personalize` (managed-file edits like `brain/North Star.md`, only when the user supplied non-default values), `post-update` (additive managed-file edits on update). Bound by Invariants 2 + 3 + 4 below. See [§Hook lifecycle](#hook-lifecycle-state-and-re-hash-semantics).
 

source/core/differ.ts

@@ -68,6 +68,16 @@ export interface ComputeMergeActionInput {
   readonly newValues: Record<string, unknown>;
   readonly actualContent: string;
   readonly renderContext: RenderContext;
+  /**
+   * Copy-origin file (not a `.njk` template): `oldTemplate` / `newTemplate`
+   * are verbatim bytes, NOT Nunjucks sources. Skip rendering them — a copy
+   * file is never templated, so rendering it (a) crashes on a literal `{{`
+   * that isn't a valid expression (e.g. `garbage{{` in a test fixture) and
+   * (b) silently substitutes any real `{{ expr }}` the file contains as
+   * data. The three-way merge runs on the raw bytes instead. Default
+   * `false` keeps the rendered path for `.njk` templates. See #132.
+   */
+  readonly literal?: boolean;
 }
 
 export interface ThreeWayMergeResult {
@@ -79,16 +89,15 @@ export interface ThreeWayMergeResult {
 export async function computeMergeAction(
   input: ComputeMergeActionInput,
 ): Promise<MergeAction> {
-  const base = renderString(
-    input.oldTemplate,
-    { ...input.renderContext, values: input.oldValues },
-    input.path,
-  );
-  const ours = renderString(
-    input.newTemplate,
-    { ...input.renderContext, values: input.newValues },
-    input.path,
-  );
+  // Copy-origin files are verbatim — never run Nunjucks over them (see
+  // `literal` on the input type). Template files render old/new values so the
+  // diff3 below sees only the user's manual edits, not value churn.
+  const sideContent = (template: string, values: Record<string, unknown>): string =>
+    input.literal
+      ? template
+      : renderString(template, { ...input.renderContext, values }, input.path);
+  const base = sideContent(input.oldTemplate, input.oldValues);
+  const ours = sideContent(input.newTemplate, input.newValues);
 
   if (sha256(base) === sha256(ours)) {
     return { type: 'skip', reason: 'no upstream change' };

source/core/update-planner.ts

@@ -451,6 +451,10 @@ export async function planUpdate(input: PlanUpdateInput): Promise<UpdatePlan> {
         newValues,
         actualContent,
         renderContext: newRenderContext,
+        // Copy-origin files (no `.njk` suffix) are verbatim, not templates.
+        // Rendering them through Nunjucks crashes on a literal `{{` and would
+        // substitute any real `{{ expr }}` as data, so merge raw bytes (#132).
+        literal: target.copyFromSourcePath !== undefined,
       });
 
       switch (mergeAction.type) {

tests/fixtures/merge/21-copy-origin-literal-braces/actual-file.md

@@ -0,0 +1,5 @@
+header
+const greeting = "Hi {{ user_name }}";
+const broken = "garbage{{";
+my personal line
+footer

tests/fixtures/merge/21-copy-origin-literal-braces/expected-output.md

@@ -0,0 +1,6 @@
+header
+shard added a line
+const greeting = "Hi {{ user_name }}";
+const broken = "garbage{{";
+my personal line
+footer

tests/fixtures/merge/21-copy-origin-literal-braces/new-template.md.njk

@@ -0,0 +1,5 @@
+header
+shard added a line
+const greeting = "Hi {{ user_name }}";
+const broken = "garbage{{";
+footer

tests/fixtures/merge/21-copy-origin-literal-braces/new-values.yaml

@@ -0,0 +1 @@
+user_name: Alice

tests/fixtures/merge/21-copy-origin-literal-braces/old-template.md.njk

@@ -0,0 +1,4 @@
+header
+const greeting = "Hi {{ user_name }}";
+const broken = "garbage{{";
+footer

tests/fixtures/merge/21-copy-origin-literal-braces/old-values.yaml

@@ -0,0 +1 @@
+user_name: Alice

tests/fixtures/merge/21-copy-origin-literal-braces/scenario.yaml

@@ -0,0 +1,17 @@
+name: "Copy-origin file with literal {{ — merged verbatim, never rendered"
+description: >
+  A modified copy-origin file (not a .njk template) containing a literal `{{`
+  that is not a valid Nunjucks expression ("garbage{{"), plus a real-looking
+  `{{ user_name }}`. The three-way merge must NOT render it: rendering would
+  crash on `garbage{{` and would substitute `{{ user_name }}` as data. With
+  copy_origin the merge runs on raw bytes — the user's add and the shard's add
+  combine non-conflicting, and both literals survive untouched.
+  Reported via #129; root cause #132.
+
+copy_origin: true
+ownership_before: modified
+user_edited: true
+template_changed: true
+values_changed: false
+conflict_expected: false
+expected_action: auto_merge

tests/integration/update.test.ts

@@ -1000,6 +1000,82 @@ describe('update pipeline (against examples/minimal-shard)', () => {
       await fsp.rm(shardDir, rmOpts);
     }
   }, 45_000);
+
+  it('merges a modified copy-origin file with a literal {{ without rendering it (#132)', async () => {
+    // A copy-origin file (no `.njk` suffix) carrying a literal `{{` that is
+    // not a valid Nunjucks expression, plus a real-looking `{{ user_name }}`.
+    // Before #132 the merge rendered both sides → crashed on `garbage{{` and
+    // would have substituted `{{ user_name }}`. Now it merges raw bytes.
+    const oldShard = path.join(os.tmpdir(), `shardmind-update-oldshard-${crypto.randomUUID()}`);
+    try {
+      await copyShard(MINIMAL_SHARD, oldShard);
+      const probeBase =
+        'line A\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\nline B\n';
+      await fsp.writeFile(path.join(oldShard, 'notes.txt'), probeBase, 'utf-8');
+
+      await installBaseline(vault, oldShard, 'sha-0.1.0');
+      // Sanity: the copy file installed verbatim (no substitution at install).
+      const installed = await fsp.readFile(path.join(vault, 'notes.txt'), 'utf-8');
+      expect(installed).toBe(probeBase);
+
+      // User edits the copy file → drift classifies it `modified`.
+      const probeUser =
+        'line A\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\nmy line\nline B\n';
+      await fsp.writeFile(path.join(vault, 'notes.txt'), probeUser, 'utf-8');
+
+      // New shard version edits a different region of the copy file.
+      await copyShard(oldShard, newShard);
+      await bumpVersion(newShard, '0.2.0');
+      const probeShard =
+        'line A\nshard line\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\nline B\n';
+      await fsp.writeFile(path.join(newShard, 'notes.txt'), probeShard, 'utf-8');
+
+      const state = (await readState(vault)) as ShardState;
+      const oldValues = parseYaml(
+        await fsp.readFile(path.join(vault, 'shard-values.yaml'), 'utf-8'),
+      ) as Record<string, unknown>;
+      const newManifest = await parseManifest(path.join(newShard, '.shardmind', 'shard.yaml'));
+      const newSchema = await parseSchema(path.join(newShard, '.shardmind', 'shard-schema.yaml'));
+
+      const migration = applyMigrations(oldValues, state.version, newManifest.version, newSchema.migrations);
+      const selections = mergeModuleSelections(state.modules, newSchema, {});
+      const drift = await detectDrift(vault, state);
+      const renderCtx = buildRenderContext(newManifest, migration.values, selections);
+
+      // Before #132 this call threw RENDER_TEMPLATE_ERROR on `garbage{{`.
+      const plan = await planUpdate({
+        vault: { root: vault, state, drift },
+        values: { old: oldValues, new: migration.values },
+        newShard: { schema: newSchema, selections, tempDir: newShard, renderContext: renderCtx },
+        removedFileDecisions: {},
+      });
+      expect(plan.pendingConflicts).toEqual([]);
+
+      const result = await runUpdate({
+        vaultRoot: vault,
+        plan,
+        conflictResolutions: {},
+        currentState: state,
+        newManifest,
+        newSchema,
+        newValues: migration.values,
+        newSelections: selections,
+        resolved: { ...RESOLVED, version: newManifest.version },
+        tarballSha256: 'sha-0.2.0',
+        newTempDir: newShard,
+      });
+      expect(result.state.version).toBe('0.2.0');
+
+      const merged = await fsp.readFile(path.join(vault, 'notes.txt'), 'utf-8');
+      expect(merged).toContain('garbage{{'); // no crash, literal intact
+      expect(merged).toContain('{{ user_name }}'); // NOT substituted
+      expect(merged).not.toContain('Alice'); // proof it was never rendered
+      expect(merged).toContain('my line'); // user edit kept
+      expect(merged).toContain('shard line'); // shard edit kept
+    } finally {
+      await fsp.rm(oldShard, { recursive: true, force: true });
+    }
+  }, 45_000);
 });
 
 // Shard copy helper for the hook integration test. Mirrors the one in

tests/unit/drift.test.ts

@@ -69,6 +69,12 @@ interface Scenario {
   partial_update?: boolean;
   module_change?: 'newly_included' | 'newly_excluded';
   module?: string;
+  /**
+   * Copy-origin file (not a `.njk` template). The merge must NOT render it —
+   * `oldTemplate`/`newTemplate` are verbatim bytes. Threads to
+   * `computeMergeAction`'s `literal` flag. See #132.
+   */
+  copy_origin?: boolean;
 }
 
 interface FixtureFiles {
@@ -154,7 +160,7 @@ function makeRenderContext(scenario: Scenario): RenderContext {
   };
 }
 
-const EXPECTED_FIXTURE_COUNT = 20;
+const EXPECTED_FIXTURE_COUNT = 21;
 
 describe('merge engine (fixture-driven)', () => {
   it(`discovers all ${EXPECTED_FIXTURE_COUNT} scenarios`, () => {
@@ -229,6 +235,7 @@ async function assertStandardMerge(
     newValues: files.newValues,
     actualContent: files.actualContent,
     renderContext: makeRenderContext(scenario),
+    literal: scenario.copy_origin ?? false,
   });
 
   expect(action.type).toBe(scenario.expected_action);