Skip to content

bug: update renders copy-origin files through Nunjucks in three-way merge (crash on literal {{, silent substitution) #132

@breferrari

Description

@breferrari

Summary

shardmind update renders copy-origin files (anything without a .njk suffix — scripts, .test.ts, JSON, etc.) through Nunjucks during the three-way merge. Copy files are meant to be verbatim, so rendering them is wrong on two counts:

  1. Crash — a literal {{ that isn't a valid expression (e.g. const x = "garbage{{" in a test fixture) throws expected variable endRENDER_TEMPLATE_ERROR, aborting the update.
  2. Silent substitution — a copy file that legitimately contains {{ user_name }} (e.g. test data) has it replaced with the value during the merge, corrupting the merge base/ours and the user's content.

Root cause

source/core/update-planner.ts runs computeMergeAction for every modified managed file (drift.modified), with no copy-vs-render distinction:

// update-planner.ts (modified-files loop)
const newTemplate = await fsp.readFile(target.entry.sourcePath, 'utf-8');
const mergeAction = await computeMergeAction({ ownership: 'modified', oldTemplate, newTemplate, ... });

computeMergeAction (source/core/differ.ts) then renders both sides:

const base = renderString(input.oldTemplate, { ...renderContext, values: oldValues }, path);
const ours = renderString(input.newTemplate, { ...renderContext, values: newValues }, path);

oldTemplate is the cached template (.shardmind/templates/<key> — raw bytes for a copy file) and newTemplate is the new shard's source file (raw bytes for a copy file). Both get Nunjucks-rendered even though copy files carry a copyFromSourcePath marker indicating they are verbatim.

In v6, only .njk files are rendered; everything else is copied (modules.ts: relPath.endsWith('.njk')). The merge path never honors that distinction.

Reproduction

  1. A shard ships a non-.njk file containing a literal {{ (e.g. .claude/scripts/foo.test.ts with const s = "garbage{{";).
  2. Install, then edit that file (so drift classifies it modified).
  3. Publish a new shard version and run shardmind update → crashes with RENDER_TEMPLATE_ERROR.

Empirical behavior of the three relevant cases:

Input renderString
const x = "garbage{{" (literal, copy file) throws → update crashes
copy file with {{ user_name }} renders → silently substituted to the value
.njk template typo {{ user_name } (missing brace) throws (correct — a real authoring error)

status.ts's verbose-drift base already wraps renderString in try/catch (reason: 'render-failed'), so it degrades gracefully; the crash is specific to the update merge path. drift classification compares hashes (no render), so it is unaffected.

Relationship to #129

PR #129 ("fix(renderer): pass through non-Nunjucks content on parse error") targets the same crash but at the wrong layer — it catches the parse error inside renderTemplate and returns the raw source when a regex (/%\}|{%|{{.*?}}/) decides the content "isn't a template". This is unsound:

  • Incomplete — it only helps when the file has no valid-looking {{ }}. A copy file with {{ user_name }} still renders and is silently substituted (case 2 above is untouched).
  • Regression — a genuine .njk template typo like {{ user_name } (no closing brace) doesn't match the regex, so the error is swallowed and the broken literal is emitted into the user's vault instead of failing loudly. This breaks the engine's "loud typed errors on authoring mistakes" contract.
  • Wrong altitude — it weakens every render path (install/update/adopt output + drift base) to fix a problem that is specific to merging copy-origin files.

Security note: #129's regex is ReDoS-safe and does not expand the template-injection surface (it reduces rendering on failure). But the underlying behavior it leaves in place — evaluating verbatim files through Nunjucks (autoescape: false) during merge — is a latent injection/corruption vector for any copy file containing {{ … }}.

Proposed fix

Pass a literal flag to computeMergeAction for copy-origin files (target.copyFromSourcePath !== undefined); when set, skip renderString and merge the raw bytes. Keep renderTemplate strict so real .njk authoring errors stay fatal.

Acceptance

  • A modified copy-origin file with a literal {{ updates without crashing; the literal is preserved.
  • A copy-origin file containing {{ expr }} is not substituted during merge.
  • A genuine .njk template syntax error still throws RENDER_TEMPLATE_ERROR (no masking).
  • Regression test + integration test against the modified-copy-file update path.

Binary copy files going through the (utf-8) three-way merge remain tracked separately by #63.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions