[bug-fix] Fix preset-constitution-not-installed: use PresetResolver in constitution setup#3297
Draft
github-actions[bot] wants to merge 1 commit into
Draft
Conversation
…tion setup Apply the remediation from the bug assessment on issue #3272. Changes: 1. Modify ensure_constitution_from_template (init.py) to resolve the constitution-template through the preset priority stack via PresetResolver, instead of hardcoding the core template path. This ensures a preset's replacement constitution-template is used when seeding .specify/memory/constitution.md. 2. Reorder init flow: move ensure_constitution_from_template to after the preset installation block so that 'specify init --preset' seeds the memory file from the already-resolved template stack, not from the generic template that existed before the preset arrived. 3. Add _maybe_reseed_constitution to PresetManager (presets/__init__.py): a post-install hook that re-seeds .specify/memory/constitution.md from the preset's constitution-template during 'specify preset add' on an existing project, but only when the memory file still contains generic placeholder tokens ([PROJECT_NAME] or [PRINCIPLE_1_NAME]). Legitimately authored constitutions (no placeholder tokens) are never overwritten. 4. Add regression tests covering both code paths (TestConstitutionReseedOnPresetInstall and TestEnsureConstitutionFromTemplate in tests/test_presets.py). Refs #3272 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes preset-aware constitution seeding during initialization and preset installation.
Changes:
- Resolves constitution templates through
PresetResolver. - Reorders initialization and adds guarded post-install reseeding.
- Adds constitution seeding and preservation tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/commands/init.py |
Adds preset-aware constitution resolution and reorders seeding. |
src/specify_cli/presets/__init__.py |
Adds post-install constitution reseeding. |
tests/test_presets.py |
Tests constitution resolution and overwrite guards. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Medium
Comment on lines
+53
to
+55
| template_constitution = _PresetResolver(project_path).resolve( | ||
| "constitution-template", "template" | ||
| ) |
Comment on lines
+1658
to
+1659
| resolver = PresetResolver(self.project_root) | ||
| resolved = resolver.resolve("constitution-template", "template") |
Comment on lines
+1649
to
+1652
| try: | ||
| content = memory_constitution.read_text(encoding="utf-8") | ||
| except OSError: | ||
| return |
| return | ||
|
|
||
| try: | ||
| shutil.copy2(resolved, memory_constitution) |
| continuing="Continuing without the optional preset.", | ||
| ) | ||
|
|
||
| ensure_constitution_from_template(project_path, tracker=tracker) |
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.
Bug fix — preset-constitution-not-installed
Proposed fix for issue #3272, applying the remediation from the bug assessment.
Verdict: Valid · Severity: High
Summary
ensure_constitution_from_templatehardcoded a read from the generic core template path, bypassing the preset resolution stack entirely, so a preset'sconstitution-template(withstrategy: replace) was never used when seeding.specify/memory/constitution.md. Additionally, forspecify init --preset, the function ran before the preset was installed, so even a resolver-aware call would have missed the preset. This PR fixes both issues and adds a post-install re-seed hook forspecify preset addon existing projects.Changes
src/specify_cli/commands/init.pyensure_constitution_from_templatenow resolves template viaPresetResolver; call moved to after preset installation blocksrc/specify_cli/presets/__init__.py_maybe_reseed_constitutionhelper; called frominstall_from_directorypost-installtests/test_presets.pyTestConstitutionReseedOnPresetInstallandTestEnsureConstitutionFromTemplateclassesTests Added or Updated
TestConstitutionReseedOnPresetInstall::test_install_reseeds_generic_constitution_memory— pins thatinstall_from_directoryre-seeds memory from preset when[PROJECT_NAME]is presentTestConstitutionReseedOnPresetInstall::test_install_reseeds_on_principle_placeholder— pins re-seed when[PRINCIPLE_1_NAME]is present even without[PROJECT_NAME]TestConstitutionReseedOnPresetInstall::test_install_does_not_overwrite_authored_constitution— pins the safety guard against overwriting authored constitutionsTestConstitutionReseedOnPresetInstall::test_install_skips_reseed_when_no_constitution_template— pins no-op when preset has noconstitution-templateentryTestConstitutionReseedOnPresetInstall::test_install_skips_reseed_when_memory_absent— pins thatinstall_from_directorydoes not create the memory fileTestEnsureConstitutionFromTemplate::test_uses_preset_constitution_when_available— pins that the function picks the preset's template over the generic coreTestEnsureConstitutionFromTemplate::test_falls_back_to_core_template_without_preset— pins fallback to core template when no preset is installedTestEnsureConstitutionFromTemplate::test_skips_when_memory_already_exists— pins the existing-file guardLocal Verification
PresetResolvercontract and the existingTestSelfTestPresetfixture patterns._register_commands/PresetResolver.resolvecall patterns already exercised by the test suite.Deviations from Assessment
None. The implementation follows the preferred three-part fix exactly:
ensure_constitution_from_templateusesPresetResolver.install_from_directory(skips when memory is absent or has been authored).The two NEEDS CLARIFICATION open questions were non-blocking:
speckit.constitutionpreset-awareness): not in scope; the second assessment comment confirmed this is a separate concern.Risks & Review Notes
[PROJECT_NAME]/[PRINCIPLE_1_NAME]detection) must cover both tokens since a partially-filled constitution might have the project name set but still carry[PRINCIPLE_1_NAME]. Both checks are independent (either triggers a re-seed).ensure_constitution_from_templateafter preset installation is a no-op for projects without--preset(the common case)._maybe_reseed_constitutionis best-effort: anOSErroron the finalshutil.copy2is silently swallowed so it cannot fail the overall preset installation.Refs #3272 · cc
@PechiSW