fix(merge): don't render copy-origin files in three-way merge (#132)#133
Merged
Conversation
`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>
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>
- 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>
…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>
/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>
There was a problem hiding this comment.
Pull request overview
Fixes a crash and data-corruption risk in shardmind update by ensuring the three-way merge does not Nunjucks-render copy-origin (non-.njk) files, preserving literal {{ content and preventing unintended substitutions.
Changes:
- Add an opt-in
literalmode tocomputeMergeAction()to merge copy-origin files as raw content (no render). - Thread the copy-origin signal (
copyFromSourcePath) from the update planner into the merge decision (literal: true). - Add fixture, unit, and integration coverage; update docs + changelog to reflect the corrected merge behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
source/core/differ.ts |
Adds literal?: boolean to skip rendering old/new templates and merge raw strings for copy-origin files. |
source/core/update-planner.ts |
Sets literal for modified files when the target is copy-origin (copyFromSourcePath present). |
tests/unit/three-way-merge.test.ts |
Adds unit tests validating literal raw-merge behavior, CRLF preservation, and .njk syntax errors still throwing. |
tests/unit/drift.test.ts |
Extends fixture runner to pass literal based on new copy_origin scenario flag; bumps fixture count. |
tests/integration/update.test.ts |
Adds end-to-end regression test ensuring modified copy-origin files with literal {{ update without rendering/crashing. |
tests/fixtures/merge/21-copy-origin-literal-braces/* |
New fixture scenario validating raw merge for copy-origin content containing {{. |
docs/SHARD-LAYOUT.md |
Documents that update merge honors the .njk render vs copy-origin split (raw merge for non-.njk). |
docs/IMPLEMENTATION.md |
Updates merge spec to describe literal behavior for copy-origin files. |
CHANGELOG.md |
Adds an Unreleased fix entry describing the behavior change and test coverage. |
Comment on lines
+182
to
+187
| // base→ours changed b→c, user unchanged → managed-style overwrite to ours. | ||
| expect(['overwrite', 'auto_merge']).toContain(action.type); | ||
| if (action.type === 'overwrite' || action.type === 'auto_merge') { | ||
| expect(action.content.endsWith('\n')).toBe(false); | ||
| } | ||
| }); |
Owner
Author
There was a problem hiding this comment.
Fixed in fac9522. You're right — ownership: 'modified' never hits the managed overwrite branch, so the result is always auto_merge (user unchanged, shard diverged → clean resolve to ours). Tightened the assertion to auto_merge and kept the no-trailing-newline check.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #132. Root cause reported via #129 — thanks @e3322-pcsk9 for surfacing a real crash.
Summary
shardmind updatecrashed when a modified copy-origin file (any non-.njkfile — scripts,.test.ts, JSON) contained a literal{{that isn't a valid Nunjucks expression. The three-way merge (computeMergeAction,source/core/differ.ts) rendered both sides of every modified file through Nunjucks — but in v6 only.njkfiles are templates; everything else is copied verbatim (modules.ts). Rendering a copy file (a) crashed on a literal{{and (b) silently substituted any real{{ expr }}it legitimately contained.Fix
computeMergeActiontakes aliteralflag;update-plannersets it for copy-origin files (those already carryingcopyFromSourcePath). When set, the merge runs on the raw bytes (base = oldTemplate,ours = newTemplate, no render). The renderer itself stays strict, so genuine.njktemplate syntax errors keep failing loudly withRENDER_TEMPLATE_ERROR.Why not the renderer-level approach (re: #129)
#129 fixed the symptom by catching the parse error inside
renderTemplateand returning the raw source when a regex decided the content "isn't a template". Verified empirically that approach is unsound:renderStringconst x="garbage{{"(copy){{ user_name }}.njktypo{{ user_name }The renderer-level catch is wrong-altitude (weakens every render path for a merge-specific problem), incomplete (copy-file substitution untouched), and masks genuine
.njkauthoring errors. Fixing it at the merge — where the copy/render distinction is already known — addresses all three.Quality gate
npm run typecheckclean.npm test— 1125 passed | 30 skipped (was 1117).npm run buildclean.Acceptance (#132)
{{updates without crashing; literal preserved.{{ expr }}is not substituted during merge..njktemplate syntax error still throwsRENDER_TEMPLATE_ERROR(no masking).Harden Audit
Reference bar: #11 (merge engine — fixtures-first).
Rounds
tests/fixtures/merge/21-copy-origin-literal-braces/) authored and confirmed RED (RENDER_TEMPLATE_ERRORongarbage{{) before the fix; GREEN after.literal ? tpl : renderString(...)branches into one local helper; agents confirmed the seam, thecopyFromSourcePathsignal, and thestatus.tsdeferral are the right altitude.copyFromSourcePathis set only oncopy[], neverrender[](so a.njkfile is never wrongly skipped); add/restore/overwrite byte-copy copy files (never rendered);.njkerrors still throw; line-ending handling holds on raw bytes. Added CRLF + no-trailing-newline literal tests it flagged.Tests added
{{+{{ user_name }}→ auto-merge, both literals survive).computeMergeAction): literal raw-merge + no-substitution, literal conflict, CRLF preservation, no-trailing-newline, and a.njk-still-throws regression guard.updatepipeline with a modified copy file containinggarbage{{.Deferrals (tracked on #132)
status.ts(verbose drift base) renders copy templates too — same root cause, but try/catch-guarded so it never crashes (display-only inaccuracy). Follow-up; needs a small separate change to thread copy-origin intoDriftEntry.