diff --git a/CHANGELOG.md b/CHANGELOG.md index fbabfe1..515b7d2 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index 8d26005..9ff03fa 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -734,6 +734,7 @@ computeMergeAction(input: { newValues: Record; actualContent: string; // File on disk renderContext: RenderContext; + literal?: boolean; // copy-origin file → merge raw bytes, don't render (#132) }): Promise 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`: diff --git a/docs/SHARD-LAYOUT.md b/docs/SHARD-LAYOUT.md index 9911a1f..4c05611 100644 --- a/docs/SHARD-LAYOUT.md +++ b/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 (`/_each..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 (`/_each..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). diff --git a/source/core/differ.ts b/source/core/differ.ts index f97fc0a..345e7b8 100644 --- a/source/core/differ.ts +++ b/source/core/differ.ts @@ -68,6 +68,16 @@ export interface ComputeMergeActionInput { readonly newValues: Record; 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 { - 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 => + 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' }; diff --git a/source/core/update-planner.ts b/source/core/update-planner.ts index 8616e24..adb46b2 100644 --- a/source/core/update-planner.ts +++ b/source/core/update-planner.ts @@ -451,6 +451,10 @@ export async function planUpdate(input: PlanUpdateInput): Promise { 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) { diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/actual-file.md b/tests/fixtures/merge/21-copy-origin-literal-braces/actual-file.md new file mode 100644 index 0000000..8a43d6b --- /dev/null +++ b/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 diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/expected-output.md b/tests/fixtures/merge/21-copy-origin-literal-braces/expected-output.md new file mode 100644 index 0000000..91402dc --- /dev/null +++ b/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 diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/new-template.md.njk b/tests/fixtures/merge/21-copy-origin-literal-braces/new-template.md.njk new file mode 100644 index 0000000..1aae36a --- /dev/null +++ b/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 diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/new-values.yaml b/tests/fixtures/merge/21-copy-origin-literal-braces/new-values.yaml new file mode 100644 index 0000000..8059494 --- /dev/null +++ b/tests/fixtures/merge/21-copy-origin-literal-braces/new-values.yaml @@ -0,0 +1 @@ +user_name: Alice diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/old-template.md.njk b/tests/fixtures/merge/21-copy-origin-literal-braces/old-template.md.njk new file mode 100644 index 0000000..b2f5433 --- /dev/null +++ b/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 diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/old-values.yaml b/tests/fixtures/merge/21-copy-origin-literal-braces/old-values.yaml new file mode 100644 index 0000000..8059494 --- /dev/null +++ b/tests/fixtures/merge/21-copy-origin-literal-braces/old-values.yaml @@ -0,0 +1 @@ +user_name: Alice diff --git a/tests/fixtures/merge/21-copy-origin-literal-braces/scenario.yaml b/tests/fixtures/merge/21-copy-origin-literal-braces/scenario.yaml new file mode 100644 index 0000000..f91b3ff --- /dev/null +++ b/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 diff --git a/tests/integration/update.test.ts b/tests/integration/update.test.ts index 7ad1de3..fddc753 100644 --- a/tests/integration/update.test.ts +++ b/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; + 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 diff --git a/tests/unit/drift.test.ts b/tests/unit/drift.test.ts index 2146ca4..66d297f 100644 --- a/tests/unit/drift.test.ts +++ b/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); diff --git a/tests/unit/three-way-merge.test.ts b/tests/unit/three-way-merge.test.ts index 9ecd611..f7aaa86 100644 --- a/tests/unit/three-way-merge.test.ts +++ b/tests/unit/three-way-merge.test.ts @@ -6,7 +6,16 @@ */ import { describe, it, expect } from 'vitest'; -import { threeWayMerge } from '../../source/core/differ.js'; +import { threeWayMerge, computeMergeAction } from '../../source/core/differ.js'; +import type { RenderContext } from '../../source/runtime/types.js'; + +const RENDER_CTX: RenderContext = { + values: {}, + included_modules: [], + shard: { name: 'test-shard', version: '0.1.0' }, + install_date: '2026-04-01', + year: '2026', +}; describe('threeWayMerge — stats accounting', () => { it('counts every unchanged line when base === theirs === ours', () => { @@ -107,3 +116,126 @@ describe('threeWayMerge — stats accounting', () => { expect(result.content).toContain('user added'); }); }); + +describe('computeMergeAction — literal (copy-origin) inputs (#132)', () => { + it('literal: merges raw bytes without rendering — literal {{ survives, no crash', async () => { + // A copy file containing a literal `{{` that is not a valid Nunjucks + // expression AND a real-looking `{{ user_name }}`. Rendering (the bug) + // would crash on the former and substitute the latter. literal=true + // merges the raw bytes; both survive. + const old = 'a\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\n'; + const neu = 'a\nshard line\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\n'; + const actual = 'a\nconst s = "garbage{{";\nconst g = "{{ user_name }}";\nmine\n'; + const action = await computeMergeAction({ + path: 'scripts/foo.test.ts', + ownership: 'modified', + oldTemplate: old, + newTemplate: neu, + oldValues: { user_name: 'Alice' }, + newValues: { user_name: 'Alice' }, + actualContent: actual, + renderContext: RENDER_CTX, + literal: true, + }); + expect(action.type).toBe('auto_merge'); + if (action.type === 'auto_merge') { + expect(action.content).toContain('garbage{{'); // not crashed + expect(action.content).toContain('{{ user_name }}'); // NOT substituted to "Alice" + expect(action.content).not.toContain('Alice'); + expect(action.content).toContain('mine'); // user edit kept + expect(action.content).toContain('shard line'); // shard edit kept + } + }); + + it('literal: preserves the user file\'s CRLF line endings on raw merge', async () => { + const action = await computeMergeAction({ + path: 'win.txt', + ownership: 'modified', + oldTemplate: 'a\nb\n', + newTemplate: 'a\nshard\nb\n', + oldValues: {}, + newValues: {}, + actualContent: 'a\r\nb\r\nmine\r\n', // user file is CRLF + renderContext: RENDER_CTX, + literal: true, + }); + expect(action.type).toBe('auto_merge'); + if (action.type === 'auto_merge') { + expect(action.content).toContain('\r\n'); // user's CRLF honored + expect(action.content).toContain('shard'); + expect(action.content).toContain('mine'); + } + }); + + it('literal: copy file with no trailing newline merges without inventing one', async () => { + const action = await computeMergeAction({ + path: 'no-eol.txt', + ownership: 'modified', + oldTemplate: 'a\nb', + newTemplate: 'a\nc', + oldValues: {}, + newValues: {}, + actualContent: 'a\nb', + renderContext: RENDER_CTX, + literal: true, + }); + // ownership 'modified' → three-way merge (never the managed overwrite + // branch). base→ours changed b→c, user unchanged → clean auto_merge to ours. + expect(action.type).toBe('auto_merge'); + if (action.type === 'auto_merge') { + expect(action.content.endsWith('\n')).toBe(false); + } + }); + + it('literal: a genuine divergence still produces a conflict on raw bytes', async () => { + const action = await computeMergeAction({ + path: 'data.json', + ownership: 'modified', + oldTemplate: 'a\nbase\nc\n', + newTemplate: 'a\nshard{{\nc\n', + oldValues: {}, + newValues: {}, + actualContent: 'a\nmine{{\nc\n', + renderContext: RENDER_CTX, + literal: true, + }); + expect(action.type).toBe('conflict'); + }); + + it('non-literal (.njk template) with a real syntax error STILL throws — no #129 masking', async () => { + // Regression guard: the fix must NOT make `.njk` authoring errors silent. + // A broken template (`{{ user_name }` — missing brace) on the rendered + // path must still surface RENDER_TEMPLATE_ERROR, not pass through. + await expect( + computeMergeAction({ + path: 'note.md.njk', + ownership: 'modified', + oldTemplate: 'ok\n', + newTemplate: 'Hi {{ user_name }\n', // genuine Nunjucks syntax error + oldValues: { user_name: 'Alice' }, + newValues: { user_name: 'Alice' }, + actualContent: 'ok\nedited\n', + renderContext: RENDER_CTX, + literal: false, + }), + ).rejects.toMatchObject({ code: 'RENDER_TEMPLATE_ERROR' }); + }); + + it('non-literal default: omitting literal renders as before (back-compat)', async () => { + // No `literal` field → renders, substituting `{{ user_name }}`. + const action = await computeMergeAction({ + path: 'note.md.njk', + ownership: 'managed', + oldTemplate: 'Hi {{ user_name }}\n', + newTemplate: 'Hello {{ user_name }}\n', + oldValues: { user_name: 'Alice' }, + newValues: { user_name: 'Alice' }, + actualContent: 'Hi Alice\n', + renderContext: RENDER_CTX, + }); + expect(action.type).toBe('overwrite'); + if (action.type === 'overwrite') { + expect(action.content).toBe('Hello Alice\n'); // substituted + } + }); +});