[bug-fix] Fix constitution-preset-template-sync: remove template propagation; rely on runtime resolution (#3737) - #3790
Conversation
There was a problem hiding this comment.
Pull request overview
Expands the constitution workflow to synchronize guidance across project and preset template layers.
Changes:
- Adds template-layer discovery instructions.
- Requires preserving provider-specific content.
- Expands Sync Impact Report statuses and provenance.
Show a summary per file
| File | Description |
|---|---|
templates/commands/constitution.md |
Updates template synchronization and reporting instructions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (5)
templates/commands/constitution.md:105
- The effective project override lives under
.specify/templates/overrides/, as used byPresetResolver.collect_all_layers()and the setup scripts. Checking.specify/templates/<template-name>.mdinstead skips the real override (and misclassifies the installed core copy), so a constitution update can still leave the effective template stale.
1. **Project-local override**: Check if `.specify/templates/<template-name>.md` exists. If so, include it.
templates/commands/constitution.md:109
- This scan does not match the effective preset stack: it includes disabled preset directories, ignores registry priority, and requires the optional
replacesfield even though the resolver selects manifest entries bytypeandname(including composingappend/prepend/wrapentries). Consequently it can edit inactive presets while omitting active layers; use.specify/presets/.registryand the same manifest matching contract asPresetResolver.collect_all_layers()(src/specify_cli/presets/__init__.py:618-648, 5137-5161).
2. **Active preset-provided replacements**: List every subdirectory of `.specify/presets/` (if the directory exists).
For each subdirectory, read `preset.yml` and look for entries under `provides.templates` where the `replaces`
field matches `<template-name>`. For each matching entry, the preset-provided file path is
`.specify/presets/<preset-id>/<file>` (the `file` field from `preset.yml`). Include all such files.
templates/commands/constitution.md:104
- The list labeled “all active layers” omits enabled extension-provided templates, although
PresetResolver.collect_all_layers()places them between presets and core (src/specify_cli/presets/__init__.py:5194-5232) and issue #3737 requires the same effective stack. An extension replacement will therefore remain stale and the Sync Impact Report will falsely appear complete.
a. **Discover all active layers** (in priority order):
templates/commands/constitution.md:111
- The editable installed core layer is
.specify/templates/<template-name>.md; only the bundled package fallback is read-only (PresetResolver.collect_all_layers(),src/specify_cli/presets/__init__.py:5234-5269). Treating core only as an inaccessible package reference means the built-in layer is either mislabeled via step 1 or omitted once the override path is corrected, contrary to the requirement to synchronize both built-in and preset templates.
3. **Core/built-in reference**: Note that the core template in the package itself is not directly editable
by the agent; treat it as the canonical reference for constitutional guidance.
templates/commands/constitution.md:139
- This collapses “already compliant” into “updated,” despite step 4 defining them separately and issue #3737 explicitly requiring the report to distinguish updated, already-compliant, and pending files. A file that was reviewed but unchanged would still be inaccurately reported as updated.
- Templates requiring updates (✅ updated / ⚠ pending / ✗ not found) — list **every** resolved template layer
from step 4 (project-local overrides and preset-provided replacements) with its file path and provider
(e.g., "project override", "preset: <preset-id>"). A template must only be marked ✅ updated if it was
actually reviewed and either modified or explicitly verified as already compliant during this run. Templates
that were not reviewed in this run must be marked ⚠ pending.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
Issue #3737 asked the /constitution command to synchronize constitutional guidance into every effective task/plan/spec template, including active preset-provided replacements. This changes the fix's direction: rather than teach the command to discover and edit more template layers, it removes the template-propagation behavior entirely. Why this is the correct fix: - The governed templates do not embed constitutional content. plan-template carries a runtime placeholder ("[Gates determined based on constitution file]") and spec-template/tasks-template reference no principles at all. - The consuming commands read .specify/memory/constitution.md at runtime and derive their Constitution Check gates live (plan, tasks), and analyze is the dedicated drift checker that validates spec/plan/tasks against the constitution. Enforcement is therefore already automatic and always current. - Statically editing template files fights the preset/override composition system: a replace preset shadows an edited core template entirely, and a hand-edited versioned preset file is clobbered on its next update. Presets and extensions are formalized, versioned artifacts the command must not mutate. So the original bug (constitution edits missing active preset templates) is resolved by not propagating at all: the runtime read is the single source of truth. The /constitution command is scoped to its own artifact — it drafts and writes the constitution and reports a Sync Impact Report changelog, and no longer reads, edits, or reports on plan/spec/tasks/preset/extension templates. Refs #3737 Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5b80589c-74e8-42e5-b2cb-7a7e0d69a964
3e54352 to
7e7a296
Compare
|
Revised this PR to pivot the approach (force-pushed Rather than extend propagation to discover and edit more template layers, this now removes template propagation from Rationale:
So the underlying bug — constitution edits missing the active preset template — is fixed by not propagating at all. The command is now scoped to drafting and writing the constitution plus a Sync Impact Report changelog (net +10/−18, single file). Full reasoning is in the updated PR description. Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8). This change was directed and reviewed by @mnriem before commit. |
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
templates/commands/constitution.md:20
- The linked #3737 acceptance criteria require discovering active preset templates, updating both built-in and replacement templates, and reporting every resolved source. These lines explicitly prohibit that behavior, so this change cannot satisfy any of the issue's synchronization/validation criteria. Please either revise the issue's accepted contract to the runtime-resolution model (with corresponding validation) or implement the agreed criteria before presenting this as the #3737 fix.
This command's own work is limited to updating the project constitution itself. Dependent templates
and commands read the constitution at runtime and are not modified here.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Medium
Bug fix — #3737, revised approach
This supersedes the original propagation-based fix. Instead of teaching
/constitutionto discover and edit more template layers, it removes template propagation from the command entirely. The runtime-resolution model already keeps templates aligned with the constitution, so propagation is redundant and actively harmful.Why the pivot — and why this is the correct fix
1. The governed templates don't embed constitutional content.
plan-template.mdcarries a runtime placeholder:[Gates determined based on constitution file].spec-template.mdandtasks-template.mdreference no principles at all.There is nothing to "sync" into them — they hold a pointer, not a copy.
2. The consuming commands read the constitution at runtime, every run.
plan.md→ "Fill Constitution Check section from constitution" (re-evaluated post-design).tasks.md→ loads/memory/constitution.mdfor principles and governance constraints.analyze.mdis the dedicated drift checker: it treats/memory/constitution.mdas the non-negotiable live authority, extracts principle MUST/SHOULD at runtime, and flags violations as CRITICAL.Enforcement is therefore already automatic and always current. A "consistency check" inside
/constitutionwould be a stale snapshot duplicatinganalyze.3. Statically editing template files fights the preset/override composition system.
replacepreset shadows an edited core template entirely — editing core changes nothing the runtime actually uses (this is exactly the bug in [Bug]: Constitution updates do not synchronize active preset-provided task templates #3737).append/wrap) have no single file to edit — the effective template is a computed string.What #3737 asked vs. what this does
#3737 asked the command to propagate guidance into every effective template layer, including active preset replacements. Extending propagation that way is the anti-pattern: it duplicates the single source of truth and mutates artifacts the command doesn't own. The correct resolution of the underlying bug (constitution edits missing the active preset template) is to not propagate at all — the runtime read is the single source of truth.
Changes
templates/commands/constitution.md/constitutionnow scopes to its own artifact: draft, validate, and write.specify/memory/constitution.md, plus a Sync Impact Report changelog. Renumbered remaining steps; frontmatter, Scope Guard, and Outline no longer claim propagation.Net: +10 / −18, single file. The command no longer reads, edits, or reports on
plan/spec/tasks/preset/extension templates.Tests
No code paths change (agent prompt only). The runtime resolution this fix relies on is already covered by
TestCollectAllLayersand the resolver-integration tests intests/. There is intentionally nothing new for/constitutionto test — its downstream surface was removed.Refs #3737