fix(update/sync): sync reports 'up to date' on content drift; update overwrites in-place customizations (constitution)

[admarble]

Summary

Dogfooding a manual sequant update in a downstream project surfaced two correctness bugs in the update/sync flow. Both lead a user to a wrong outcome on the default path: either silently miss real drift, or silently overwrite a file they customized in place.

Scope note: the headless/TTY ergonomics of update (crashes without a TTY, no --yes) are out of scope here — these two are the correctness bugs. Filing them together because they share one theme: the manual update flow can't be trusted to do the right thing by default.

---

Bug 1 — sync reports "up to date" while real content drift exists

sequant sync decides up-to-date status purely from the version marker:

src/commands/sync.ts:106
  if (!force && skillsVersion === packageVersion) {
    console.log(chalk.green("✔ Skills are already up to date!"));

Version equality ≠ content equality. On a tree where update (which diffs content) found 5 modified + 1 new file, sync printed ✔ Skills are already up to date! and exited a no-op — same 2.6.1 marker on both sides.

This matters because sync is the non-interactive / automation-recommended path (it even prints "Plugin users get auto-updates without running sync manually"). The path we point CI and scripts at is the one that declares success while leaving drift in place. The only escape is sync --force, which is undiscoverable from the "up to date" message.

Acceptance criteria

• When the version marker matches but bundled template content differs from installed content, sync does not claim "up to date" — it reports the drift (e.g. version current, but N files differ — run \update` or `sync --force``) and/or applies it.
• Truthful no-op: sync only prints "already up to date" when content is actually identical.

---

Bug 2 — In-place customizations (e.g. constitution.md) are classified modified and overwritten on the default Y

Override protection only fires when a separate .claude/.local/<file> exists:

src/commands/update.ts:151-159
  // Check if there's a local override
  const localOverridePath = localPath.replace(".claude/", ".claude/.local/");
  const hasLocalOverride = await fileExists(localOverridePath);
  if (hasLocalOverride) {
    changes.push({ path: localPath, status: "local-override" });
  } else {
    ... status: "modified"
  }

The constitution is the one file explicitly meant to be edited in place per project. A user who customizes it (custom principles/patterns) — without creating a parallel .claude/.local/ file they have no reason to know about — gets it listed under Modified, with Local overrides: 0, and the default Apply updates? (Y/n) overwrites their content. That's silent data loss of intentional customization.

Compounding it, the diff compares against the raw template:

src/commands/update.ts:148
  if (localContent === templateContent) { ... unchanged }

But {{PROJECT_NAME}} is only substituted at write time (update.ts:242). So constitution.md shows as modified for every initialized project even with zero user edits (installed # matcha-maps Constitution vs template # {{PROJECT_NAME}} Constitution). This trains users to hit Y on a "modified" constitution as routine — exactly when it might contain real edits.

Repro

1. sequant init in a project; edit .claude/memory/constitution.md in place (add a custom principle).
2. sequant update.
3. Observe constitution under Modified, Local overrides: 0; default Y reverts custom content to template (+ name substitution).

Acceptance criteria

• In-place divergence of a customizable/templated file (constitution at minimum) is detected as a protected local override — skipped by default, only overwritten with --force.
• Template-vs-installed comparison renders {{PROJECT_NAME}} (and other tokens) before diffing, so an unmodified constitution reads as unchanged, not modified.
• No silent loss of in-place customizations on the default (non---force) path.

---

Evidence (observed this session)

• sequant sync → ✔ Skills are already up to date! while sequant update --dry-run on the same tree reported New files: 1 / Modified: 5 / Local overrides: 0.
• update Modified list included .claude/memory/constitution.md, shown as # matcha-maps Constitution → # {{PROJECT_NAME}} Constitution. Had to restore from git after applying.

Suggested approach

• sync: fall back to (or always use) content comparison; treat version marker as a fast-path hint, not the source of truth.
• update: (a) render template tokens before the localContent === templateContent check; (b) treat a customizable file diverging in place as local-override (skip-by-default), not modified.

[admarble]

Plan Review vs Acceptance Criteria — #708

Complexity: Standard (5 ACs, 2 directories: src/commands, src/lib)

AC Quality Check

ACs are written as plain - [ ] checkboxes without explicit AC-N: IDs, so the parser extracted 0 — I've assigned IDs below for traceability. Minor recommendation: add **AC-1:**-style IDs so /exec and /qa can track them. One open-ended phrase to pin down:

• AC-1 "...reports the drift and/or applies it" — ambiguous on whether sync should mutate files on the version-equal path. See Open Questions (recommend report-only).
• AC-4 "customizable/templated file (constitution at minimum)" — "at minimum" leaves the set open. See Open Questions (recommend a small explicit allow-list, constitution only for now).

---

Scope Assessment

Non-Goals: Present and explicit — headless/TTY ergonomics of update (no-TTY crash, missing --yes) are out of scope. ✅

Metric	Value	Status
Feature count	1 (one update/sync content-truth fix)	✅
AC items	5	✅
Directory spread	2 (src/commands, src/lib)	✅

Verdict: ✅ SCOPE_OK — two bugs, one shared root cause; cohesive.

---

Acceptance Criteria

AC-1: sync reports drift instead of false "up to date" when content differs at equal version

Verification: Unit Test (mocked fs, mirrors existing sync.test.ts)
Scenario: Given installed version == package version but ≥1 template renders to different installed content → When syncCommand() runs without --force → Then it does not print "already up to date"; it prints version current, but N file(s) differ — run \update` or `sync --force`` and exits a no-op.

AC-2: Truthful no-op — "already up to date" only when content is identical

Verification: Unit Test
Scenario: Given version equal AND every template renders identical to installed content → When syncCommand() runs → Then it prints "already up to date" and skips copy.

AC-3: Render template tokens before the equality check

Verification: Unit Test
Scenario: Given an unmodified installed constitution.md (# matcha-maps Constitution, expanded STACK_NOTES) → When updateCommand() computes changes → Then it renders {{PROJECT_NAME}}/{{STACK_NOTES}} (using the same variable set as copyTemplates: getProjectName() + getStackNotes()) before comparing, and classifies the file unchanged, not modified.

AC-4: In-place divergence of a customizable file → local-override (skip-by-default, --force overwrites)

Verification: Unit Test
Scenario: Given a user-edited constitution.md that diverges after token rendering, with no parallel .claude/.local/ file → When updateCommand() runs → Then it is classified local-override (not modified), excluded from the default apply set; only --force includes it in the apply set.

AC-5: No silent loss of in-place customizations on the default path

Verification: Integration Test (real fs in a temp project)
Scenario: Given a customized constitution.md → When updateCommand({}) runs and the default Apply updates? (Y) is accepted → Then constitution.md is not overwritten (only --force overwrites it).

---

Implementation Plan

Phase 1: Extract a single source of truth for template variables + change detection (src/lib)

1. Add buildTemplateVariables(stack, tokens, opts?) in src/lib/templates.ts returning exactly what copyTemplates writes with: stackConfig.variables, tokens, PROJECT_NAME (from getProjectName()), STACK, STACK_NOTES (from getStackNotes() / getMultiStackNotes()). Refactor copyTemplates (templates.ts:244) to call it so the three call sites can't drift again.
2. Add computeTemplateChanges(stack, tokens, opts) in src/lib that iterates listTemplateFiles(), renders each via processTemplate(raw, vars) before comparing to installed content, and classifies new | unchanged | modified | local-override. This is the corrected version of update.ts:133-175.
3. Add CUSTOMIZABLE_FILES allow-list (.claude/memory/constitution.md) + isCustomizableFile(path). In computeTemplateChanges, a file that diverges in place AND is customizable (and has no .local override) → local-override.

Phase 2: Fix update (src/commands/update.ts) — AC-3, AC-4, AC-5

4. Replace the inline diff loop with computeTemplateChanges. Build the apply set as [...new, ...modified] by default and [...new, ...modified, ...localOverrides] only when --force.
5. Keep the existing diff rendering for the --dry-run view.

Phase 3: Fix sync (src/commands/sync.ts) — AC-1, AC-2

6. In the !force && skillsVersion === packageVersion branch (sync.ts:106), call computeTemplateChanges; if any new/modified exist, print the drift message and return (report-only, non-destructive); else print "already up to date".

---

Design Review

1. Where does this logic belong? src/lib/templates.ts (or a small src/lib/template-diff.ts) — both commands are thin consumers; the comparison + variable rules are library concerns. The bug exists because the rules were duplicated/incomplete in update and absent in sync.
2. Simplest correct approach? One shared computeTemplateChanges + buildTemplateVariables. Rejected: patching update.ts and sync.ts independently — re-creates the drift that caused this.
3. Existing pattern? Mirrors copyTemplates's variable assembly (templates.ts:244-250) — we are unifying to that exact set, not inventing one.
4. What would a senior reviewer challenge? "Does --force on update now clobber genuine .local/ overrides too?" — yes, by the AC's "only overwritten with --force" wording. Flag in Open Questions. Also: keep sync report-only so the equal-version fast path never silently overwrites an in-place-customized constitution (ties Bug 1 and Bug 2 together).

---

Feature Quality Planning

All standard checks pass. Notable items:

• Sibling site (follow-up, not silently widened): checkAndWarnSkillsOutdated (sync.ts:168) → areSkillsOutdated is also version-only and will under-report content drift to other commands' pre-flight warnings. Same root cause; recommend a follow-up issue rather than expanding this PR.
• Variable parity risk: update.ts:242 currently uses cwd basename for PROJECT_NAME and omits STACK_NOTES entirely — both must move to buildTemplateVariables or AC-3 only partially fixes the "always modified" symptom.

---

Open Questions

1. Should sync apply or only report on the equal-version drift path (AC-1 "and/or")? — Recommendation: report-only. Auto-applying would reintroduce constitution data-loss (Bug 2) on the "safe" fast path. Impact if wrong: minor message-vs-mutation change, cheap to revisit.
2. --force overwriting genuine .claude/.local/ overrides? — Recommendation: under --force, include local-override in the apply set (matches AC-4 wording). Impact: a forced update also refreshes parallel .local files; acceptable since --force = "take the template".
3. Customizable-file allow-list contents? — Recommendation: constitution.md only for now; expand later if other in-place-edited templates appear.

---

Recommended Workflow

Phases: spec → testgen → exec → qa
Quality Loop: enabled
Reasoning: ACs are Unit/Integration-verifiable (testgen stubs first). Data-loss-class correctness bug → enable the quality loop despite SCOPE_OK. No UI → no test phase.

---

Label Review

Current: bug, cli
Recommended: bug, cli (+ planned on this comment)
Reason: Accurate as-is; no UI/security/breaking surface.
Quality Loop: Manually enabled (data-loss risk), not auto-enabled (scope is green).

---

--- DRAFT GITHUB ISSUE COMMENT (PLAN) ---

Plan (spec): Both bugs share one root cause — divergent/incomplete template content-comparison. Fix by extracting a single buildTemplateVariables + computeTemplateChanges in src/lib (matching copyTemplates), then making update and sync thin consumers.

• AC-1 sync reports drift (not "up to date") when content differs at equal version — Unit
• AC-2 "already up to date" only when content is byte-identical — Unit
• AC-3 render {{PROJECT_NAME}}/{{STACK_NOTES}} (same vars as copyTemplates) before the equality check; unmodified constitution reads unchanged — Unit
• AC-4 in-place-diverged customizable file → local-override (skip by default, --force overwrites) — Unit
• AC-5 default Y path never overwrites a customized constitution — Integration

Key decisions: sync is report-only on the equal-version path (no auto-apply) so the fast path can't silently clobber an in-place-customized constitution. --force includes local-override files in the apply set.

Open questions: (1) sync report-only vs apply → recommend report-only; (2) --force overwriting .local/ overrides → recommend yes; (3) customizable allow-list = constitution only for now.

Follow-up (sibling): checkAndWarnSkillsOutdated/areSkillsOutdated (sync.ts:168) is also version-only — same root cause, recommend a separate issue.

Workflow: spec → testgen → exec → qa, quality loop on (data-loss-class bug).

[admarble]

Implementation Update (exec) — PR #710

Both bugs share one root cause (incomplete/divergent template content-comparison), fixed by extracting a single source of truth in src/lib/templates.ts: buildTemplateVariables + computeTemplateChanges, now also used by copyTemplates so the write and diff paths can't drift again.

AC Coverage

AC	Description	Status
AC-1	sync reports drift instead of false "up to date" at equal version	✅ Implemented
AC-2	"already up to date" only when content is byte-identical	✅ Implemented
AC-3	Render {{PROJECT_NAME}}/{{STACK_NOTES}} (same vars as copyTemplates) before equality check	✅ Implemented
AC-4	In-place-diverged customizable file → local-override (skip default, --force overwrites)	✅ Implemented
AC-5	Default Y path never overwrites a customized constitution	✅ Implemented

Key files changed

• src/lib/templates.ts — buildTemplateVariables, computeTemplateChanges, CUSTOMIZABLE_FILES/isCustomizableFile, SEQUANT_TEMPLATES_DIR override; copyTemplates refactored to share variables.
• src/commands/update.ts — replace inline diff loop with computeTemplateChanges; default apply set excludes local-override, --force includes it; writes pre-rendered content.
• src/commands/sync.ts — version-equal branch now content-verifies and reports drift (report-only).
• Tests: templates.test.ts, sync.test.ts, new update.integration.test.ts.

Quality Plan Alignment

Quality Dimension	Items Addressed	Notes
Error Handling	✅	.local/ skip + customizable allow-list preserve in-place edits
Test Coverage	✅	Unit (AC-1..4) + integration (AC-5); regression on copyTemplates consumers
Code Quality	✅	Single source of truth removes the duplicated/incomplete variable logic that caused the bug
Best Practices	✅	sync report-only (non-destructive fast path)

Derived ACs: None in spec comment.

Checks

• npm run build ✅ · npm run lint ✅ (0 warnings) · targeted vitest: 32 passed; regression set: 192 passed.

Open questions resolved (per /spec recommendations)

1. sync equal-version path → report-only (no auto-apply).
2. --force → includes local-override files in apply set.
3. Customizable allow-list → constitution.md only for now.

Follow-up (out of scope)

checkAndWarnSkillsOutdated / areSkillsOutdated are also version-only (same root cause) — recommend a separate issue rather than expanding this PR.

[admarble]

QA Review for Issue #708 — PR #710

AC Coverage

AC	Description	Status	Notes
AC-1	sync reports drift instead of false "up to date" at equal version	✅ MET	sync.ts:118-141 content-verifies on version match; prints Version current, but N file(s) differ — run \update` or `sync --force`` (verbatim match to AC); report-only, no mutation. Unit test confirms.
AC-2	"already up to date" only when content is byte-identical	✅ MET	Only printed when drifted.length === 0 (new+modified). Unit test "skips sync when versions match and content is identical".
AC-3	Render {{PROJECT_NAME}}/{{STACK_NOTES}} (same vars as copyTemplates) before equality check	✅ MET	buildTemplateVariables is now the single source shared by copyTemplates + computeTemplateChanges; tokens rendered before diff. Hermetic test: unmodified constitution → unchanged.
AC-4	In-place-diverged customizable file → local-override (skip default, --force overwrites)	✅ MET	CUSTOMIZABLE_FILES/isCustomizableFile allow-list; default apply set excludes overrides, --force includes them. Hermetic test confirms classification.
AC-5	Default Y path never overwrites a customized constitution	✅ MET	Real-fs integration test: default updateCommand({}) leaves custom content intact; {force:true} overwrites.

Coverage: 5/5 AC items fully met. Derived ACs: None in spec.

---

CI Status

Check	State	Impact
build (22.x)	SUCCESS	✅ MET
typecheck	SUCCESS	✅ MET
validate-skills	SUCCESS	✅ MET
validate-plugin	SUCCESS	✅ MET

CI Summary: 4 passed, 0 pending, 0 failed.

---

Quality Metrics

Metric	Value	Status
Type issues (any)	0	OK
Deleted tests	0	OK
Files changed	7	OK
Lines added	+558	-
Lines deleted	-108	-

---

Test Coverage Analysis

Changed File	Tier	Has Tests?
src/lib/templates.ts	Standard	✅ templates.test.ts (AC-3/AC-4 hermetic)
src/commands/sync.ts	Standard	✅ sync.test.ts (AC-1/AC-2)
src/commands/update.ts	Standard	✅ update.integration.test.ts (AC-5 real-fs)

Coverage: 3/3 changed source files have tests. Targeted suite: 32 passed. npm run build: ✅. No critical paths untested.

---

Code Review

Strengths:

• Root-cause fix, not a patch: a single buildTemplateVariables is now the only place template variables are assembled, consumed by both the write path (copyTemplates) and the diff path (computeTemplateChanges) — eliminating the duplicate/incomplete variable logic that caused the bug.
• AC-1 drift message is a verbatim match to the AC text.
• sync fast path is report-only (non-destructive), correctly avoiding reintroducing Bug 2 on the "safe" path.
• AC-5 is covered by a real-fs integration test (not just mocks) that exercises the actual data-loss scenario — exactly the right test for a data-loss-class bug.
• Graceful degradation verified: listTemplateFiles walk is wrapped in try/catch, so the new fs I/O on sync's version-equal fast path cannot crash on a missing templates dir (falls back to "up to date").

Issues: None blocking.

Suggestions (non-blocking):

• On the default update path, protected local overrides are communicated only via the Local overrides: N summary count, not listed by name (per-file listing appears only under --force). Minor discoverability nit.
• Multi-stack note: only constitution.md uses STACK_NOTES, and it's in the customizable allow-list, so the fact that update/sync don't thread additionalStacks is benign (no false "modified" elsewhere, no data loss). No action needed.

---

Test Quality Review

Category	Status	Notes
Behavior vs Implementation	✅ OK	Asserts on outputs (file contents, log messages, classification), not internals.
Coverage Depth	✅ OK	unchanged/modified/new/local-override + default-vs---force paths.
Mock Hygiene	✅ OK	Unit tests mock fs/lib boundaries; integration + templates tests use real fs in temp dirs via SEQUANT_TEMPLATES_DIR.
Test Reliability	✅ OK	Deterministic (temp dirs, pinned PROJECT_NAME via package.json).

---

Call-Site Review

New exported functions: buildTemplateVariables, computeTemplateChanges, isCustomizableFile (+ CUSTOMIZABLE_FILES, TemplateChange).

Function	Call Sites	Loop?	AC Match
computeTemplateChanges	sync.ts:118 (guarded by !force && version match), update.ts:121	No	✅ matches AC-1/AC-3/AC-4
buildTemplateVariables	templates.ts (copyTemplates, computeTemplateChanges)	No	✅ single source of truth (AC-3)
isCustomizableFile	templates.ts (computeTemplateChanges)	No	✅ matches AC-4

All call sites match AC constraints. No dead exports.

---

Execution Evidence

Test Type	Command	Result
Unit + integration	vitest run sync.test update.integration.test templates.test	✅ 32 passed
Build	npm run build	✅ pass

scripts/ not modified → standalone /verify not required. The real-fs integration test exercises updateCommand end-to-end against the actual data-loss path. Evidence status: Complete.

---

CHANGELOG Verification

Check	Status
CHANGELOG.md exists	✅
User-facing changes	✅ (bug fix)
[Unreleased] entry	✅ Present, references #708

---

Self-Evaluation

• Verified working: Yes — ran the targeted suite (32 pass) and build; read the core logic and traced the rendered-before-diff fix and the customizable allow-list end-to-end. The AC-5 integration test independently confirms the data-loss path is closed.
• Test efficacy: High — the integration test fails if the constitution is overwritten; the hermetic templates tests fail if token rendering or classification regresses.
• Likely failure mode: A future template (other than constitution) starts using STACK_NOTES while update/sync still don't thread additionalStacks → that file would read as modified for multi-stack projects. Not a current bug (only constitution uses it, and it's protected).
• Verdict confidence: High.

---

Verdict: READY_FOR_MERGE

All 5 ACs MET (AC-1 message is a verbatim match), CI green (4/4), no any/deleted tests, root-cause fix with strong unit + real-fs integration coverage. The two non-blocking suggestions above are optional polish.

Recommended follow-up (out of scope, per /spec): checkAndWarnSkillsOutdated / areSkillsOutdated (sync.ts:194) are still version-only and will under-report content drift in other commands' pre-flight warnings — same root cause. File as a separate issue.

Documentation

• CHANGELOG entry added (fix(update/sync): sync reports 'up to date' on content drift; update overwrites in-place customizations (constitution) #708)
• N/A — internal correctness fix; no README/docs surface change

Next Steps

1. Merge PR fix(#708): content-truth for update/sync; protect in-place customizations #710.
2. Optionally file the areSkillsOutdated content-truth follow-up.

[admarble]

QA Gap Fixes (commit b01823a)

Second-look adversarial review surfaced gaps where the ACs were literally met but the issue's intent wasn't fully addressed. Fixed:

1. sync automation path could still pass silently on drift.
The drift branch returned exit 0, and under --quiet printed nothing — so CI/scripts (the path Bug 1 is explicitly about) could still treat a drifted tree as success. Now sets process.exitCode = 1 on drift, which fires even under --quiet (the exit code is the machine signal the suppressible message is not). Still report-only / non-destructive. The preAction auto-sync hook only runs on version mismatch (copy path), so it never hits this branch and is unaffected.

2. Constitution protection was POSIX-only (Windows data-loss risk).
isCustomizableFile/computeTemplateChanges matched forward-slash paths, but listTemplateFiles builds paths with the OS separator — on Windows the allow-list match would miss and the constitution would revert to modified → overwritten on the default Y, reintroducing the AC-4/AC-5 data loss. Now normalizes separators before the allow-list and .local/ checks.

3. Test coverage for the above.

• sync: assert exit code 1 on drift, plus a --quiet drift case (suppressed message, still exit 1).
• templates: separator-normalization case for isCustomizableFile.
• afterEach resets process.exitCode so it can't leak across tests/into vitest.

Verification: npm run build ✅ · npm run lint ✅ (0 warnings) · full unit suite 2759 passed (1 expected-fail, 2 skipped) · targeted suite 34 passed · CI 4/4 green.

Verdict unchanged: READY_FOR_MERGE.