Skip to content

fix(renderer): pass through non-Nunjucks content on parse error#129

Closed
e3322-pcsk9 wants to merge 2 commits into
breferrari:mainfrom
e3322-pcsk9:fix/renderer-literal-braces
Closed

fix(renderer): pass through non-Nunjucks content on parse error#129
e3322-pcsk9 wants to merge 2 commits into
breferrari:mainfrom
e3322-pcsk9:fix/renderer-literal-braces

Conversation

@e3322-pcsk9
Copy link
Copy Markdown

Problem

shardmind update crashes with RENDER_TEMPLATE_ERROR when the vault contains non-template files with literal {{ strings (e.g., test fixtures like "garbage{{" in .claude/scripts/tests/stop-checklist.test.ts).

The differ (source/core/differ.ts) renders both old and new template content through renderString() for three-way merge comparison. When Nunjucks encounters {{ that isn't a valid variable expression, it throws expected variable end.

Root Cause

renderTemplate() in source/core/renderer.ts treats every Nunjucks parse failure as fatal. Files that aren't Nunjucks templates (no {{ expr }}, {% block %}, or %} syntax) should pass through unchanged rather than crash.

Fix

In renderTemplate(), before throwing RENDER_TEMPLATE_ERROR, check if the source contains any actual Nunjucks syntax. If not, return the raw source — the parse error is from accidental {{ in non-template content, not a template authoring mistake.

function renderTemplate(source, context, env, filePath): string {
  try {
    return env.renderString(source, context);
  } catch (err) {
    if (!containsNunjucksSyntax(source)) {
      return source;
    }
    throw new ShardMindError(...);
  }
}

function containsNunjucksSyntax(source: string): boolean {
  return /%\}|{%|{{.*?}}/.test(source);
}

Test Plan

  • RED: Two regression tests proving renderString crashes on "garbage{{" and "data{{"
  • GREEN: Fix makes both tests pass
  • Regression: All 1114 existing tests pass (78 test files)

Commits

  1. test(renderer): add regression tests for literal {{ in non-template content — failing tests
  2. fix(renderer): pass through non-Nunjucks content on parse error — minimal fix

Ultraworked with Sisyphus

e3322-pcsk9 and others added 2 commits May 31, 2026 19:52
…ontent

Files containing literal {{ (e.g., test fixtures with "garbage{{") crash shardmind update with RENDER_TEMPLATE_ERROR when the differ renders them through Nunjucks. These tests capture the expected behavior: non-template content should pass through unchanged.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
When Nunjucks fails to parse a file and the source contains no actual Nunjucks syntax ({{ expr }}, {% block %}, %}), return the raw source instead of throwing RENDER_TEMPLATE_ERROR. This prevents shardmind update from crashing on non-template files like .test.ts that happen to contain literal {{ strings.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
@breferrari
Copy link
Copy Markdown
Owner

Thanks for this — you surfaced a real crash in shardmind update that wasn't on our radar, and the diagnosis (the differ renders content that isn't a template) is spot on. I dug into the root cause and opened #132 with the full writeup.

One thing the renderer-level approach here runs into: in v6 the merge renders every modified file, but only .njk files are actually templates — everything else is copied verbatim. So the deeper fix is to stop the merge from rendering copy-origin files at all (merge their raw bytes), rather than catching the parse error in renderTemplate. Two concrete issues with catching it in the renderer:

  1. It still substitutes a valid {{ user_name }} that a copy file legitimately contains (e.g. test data) — renderString succeeds and replaces it, so that path isn't caught.
  2. A genuine .njk template typo like {{ user_name } (missing brace) also throws expected variable end, and the regex doesn't match it — so it would get swallowed and the broken literal emitted into the user's vault, instead of failing loudly.

I've put up #133 taking the merge-layer approach (closes #132), with your report credited in the issue, the PR, and the changelog. Would love your eyes on it — and thank you again for catching this.

@breferrari breferrari closed this Jun 1, 2026
breferrari added a commit that referenced this pull request Jun 1, 2026
…133)

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

`shardmind update` ran computeMergeAction over every modified managed file
and rendered both sides through Nunjucks (differ.ts). But in v6 only `.njk`
files are templates; copy-origin files (scripts, .test.ts, JSON) are
verbatim. Rendering them (a) crashes on a literal `{{` that isn't a valid
expression (the reported crash) and (b) silently substitutes any real
`{{ expr }}` they contain as data.

Add a `literal` flag to ComputeMergeActionInput; when set the merge runs on
the raw bytes (base=oldTemplate, ours=newTemplate, no renderString).
update-planner passes literal=copyFromSourcePath!==undefined — copy-origin
actions already carry that marker, so no new plumbing. The renderer stays
strict, so genuine `.njk` authoring errors keep failing loudly (unlike a
renderer-level catch, which would mask them).

Fixtures-first: new merge fixture 21 (copy-origin file with `garbage{{` +
`{{ user_name }}`) authored + confirmed RED (rendered → RENDER_TEMPLATE_ERROR)
before the fix; the harness threads scenario.copy_origin → literal and bumps
the fixture count to 21. Unit tests: literal merges raw + preserves both
literals, literal still detects real conflicts, and a `.njk` syntax error
STILL throws RENDER_TEMPLATE_ERROR (regression guard).

Root cause reported via #129. Binary copy files through the utf-8 merge
remain tracked by #63.

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

* test(update): copy-origin file with literal {{ merges end-to-end (#132)

Integration test through the real install → modify → detect drift → plan →
runUpdate pipeline: a shard ships a non-`.njk` copy file containing
`garbage{{` + `{{ user_name }}`; the user edits it; the new shard version
edits a different region. planUpdate no longer crashes (it did before #132),
the file merges on raw bytes, and both literals survive — `{{ user_name }}`
is NOT substituted to "Alice", proving the merge never rendered it.

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

* docs(merge): document copy-origin literal merge (#132)

- IMPLEMENTATION §4.9: `literal` input + a "Copy-origin files" note (merge
  raw bytes, skip render; renderer stays strict).
- SHARD-LAYOUT: the update three-way merge honors the .njk/copy split —
  copy files merged on raw bytes, never re-rendered.
- CHANGELOG: Fixed entry, crediting the #129 report.

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

* refactor(merge): simplify pass — share the render-or-literal branch (#132)

/simplify: collapse the two parallel `literal ? template : renderString(...)`
branches into one local `sideContent` helper. Agents confirmed the seam
(literal on computeMergeAction), the copyFromSourcePath signal, and the
status.ts deferral are all the right altitude. Skipped: extracting a shared
test RenderContext factory (test-infra, outside this diff); reusing
setUpUpdate in the integration test (explicit setup reads clearer for a
one-off copy-file scenario).

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

* test(merge): harden literal path — CRLF + no-trailing-newline (#132)

/harden (focused adversarial pass on the merge boundary; it confirmed the
fix correct — copyFromSourcePath is set ONLY on copy[] never render[], so a
.njk file is never wrongly skipped; add/restore/overwrite byte-copy copy
files; .njk errors still throw). Add the two cheap raw-byte edge cases it
flagged: literal merge preserves the user file's CRLF line endings, and a
copy file with no trailing newline merges without inventing one.

status.ts:573 renders templates for its drift base too (same root cause,
but try/catch-guarded so it degrades gracefully) — tracked as a follow-up
on #132, out of scope for this crash fix.

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

* test(merge): tighten no-trailing-newline assertion to auto_merge (#132)

Copilot review: with ownership 'modified' the action can never be
'overwrite' (that's the managed-only branch), so accepting it weakened the
test. Assert auto_merge specifically.

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

---------

Co-authored-by: Claude Opus 4.8 (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.

2 participants