feat(presets): resolve constitution templates at command time

[mnriem]

Summary

• stop preset stack changes from rewriting the live constitution by default
• resolve the composed constitution-template at command time through the dedicated Bash, PowerShell, or Python resolver script
• keep runtime resolution consistent with canonical preset and extension priority, enabled-state, ID tie-breaking, case-sensitive registry membership, and root/template-directory conventions
• reject structurally malformed preset manifests consistently across Bash, PowerShell, and Python runtime resolution: validate every template entry's required fields, type, and strategy, and reject manifests missing the provides/templates sections or declaring an empty template list, consistent with the canonical PresetManifest
• force UTF-8 decoding of preset registries and manifests in the Bash and PowerShell runtime resolvers (including the PowerShell native registry reads under Windows PowerShell 5.1) so composition no longer depends on the process locale or code page
• fail closed when a Bash template layer cannot be read, and when a corrupt or unreadable extension registry is encountered, instead of returning successful empty/partial content or silently treating every on-disk extension directory as unregistered-and-enabled
• support safe dotted command identifiers in specify preset resolve while rejecting empty or traversal-like segments
• stop resolution at the first effective replace base so malformed lower layers cannot invalidate a winning override or preset
• reject unsafe template and registry path components before filesystem resolution, and fail explicitly when Bash cannot parse an installed extension registry
• preserve source line endings and resolved template bytes when Python materializes spec.md and plan.md
• resolve feature templates before creating their directories so composition errors leave no orphan directory and clean retries keep the same feature number
• validate requested prerequisite templates consistently in JSON and text output modes
• restore guarded install-time materialization when constitution-sync is enabled and document the lifecycle

Testing

• .venv/bin/python -m pytest tests/test_presets.py tests/test_resolve_template_python_parity.py tests/test_setup_tasks.py -q
• .venv/bin/python -m pytest tests/test_check_prerequisites_python_parity.py tests/test_setup_plan_python_parity.py tests/test_create_new_feature_python_parity.py tests/test_setup_tasks_python_parity.py -q
• uvx ruff@0.15.0 check src tests

Closes #3950

Authored by GitHub Copilot (model: Claude Opus 4.8), acting autonomously on behalf of @mnriem.

[Copilot]

🟡 Not ready to approve

The referenced CLI command does not provide composed template content for /constitution to consume.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Moves constitution templates to runtime resolution while retaining optional materialization through constitution-sync.

Changes:

• Gates install-time constitution reconciliation behind constitution-sync.
• Updates /constitution guidance and lifecycle documentation.
• Expands tests for default and opt-in behavior.

File summaries

File	Description
src/specify_cli/presets/__init__.py	Gates constitution materialization.
templates/commands/constitution.md	Adds runtime-resolution workflow.
tests/test_presets.py	Covers lifecycle behavior.
presets/README.md	Documents default behavior.
presets/ARCHITECTURE.md	Describes constitution lifecycle.
presets/constitution-sync/README.md	Documents opt-in materialization.
presets/constitution-sync/preset.yml	Updates preset description.
presets/catalog.json	Updates catalog metadata.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the command-time composition review in f0d5acc4. specify preset resolve <name> --content now emits only PresetResolver.resolve_content() output and exits non-zero when resolution fails; /constitution executes that mode and refuses to continue with a single layer. Added CLI coverage for composed output and failure behavior, plus reference documentation.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

The new content endpoint permits path traversal through an unvalidated template name.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

Commit 2eaef9bd replaces the constitution-specific preset resolve --content endpoint with the same project-local composed-template mechanism used by the workflow scripts. The shared resolver now has Bash, PowerShell, and Python parity for replace, prepend, append, and wrap; spec, plan, tasks, checklist, and constitution all consume composed content; and the removed endpoint eliminates the reported path-traversal surface.

The change preserves the existing diagnostic preset resolve command, legacy TASKS_TEMPLATE output, and missing-template fallbacks. Positive and negative cross-runtime tests cover composition, malformed manifests, missing bases/placeholders, traversal, missing PyYAML, registry fallback, and Unicode. The complete suite passes: 6,517 passed, 8 skipped.

[Copilot]

🟡 Not ready to approve

Runtime resolution has path-traversal risks and diverges from the canonical extension and convention stack.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

scripts/python/common.py:343

• This extension tier is ordered alphabetically and includes every directory, ignoring .specify/extensions/.registry. Consequently a disabled extension can still supply constitution-template, and registered extension priorities are not honored, so /constitution does not actually use the same full stack as PresetResolver._get_all_extensions_by_priority(). All three runtime resolvers need registry-aware extension filtering and ordering.

    extensions_dir = repo_root / ".specify" / "extensions"
    try:
        extension_dirs = sorted(
            path
            for path in extensions_dir.iterdir()
            if path.is_dir() and not path.name.startswith(".")

scripts/python/common.py:267

• The canonical PresetResolver searches both templates/<name>.md and the preset-root <name>.md convention, and the install-time constitution code explicitly recognizes both. This helper only checks the templates/ convention, so /constitution silently falls through past a valid root-level preset template. Preserve both convention paths in every runtime resolver.

This issue also appears on line 338 of the same file.

    manifest_path = preset_dir / "preset.yml"
    conventional = preset_dir / "templates" / f"{template_name}.md"

scripts/bash/common.sh:612

• This fallback only checks templates/$TemplateName.md, while the canonical resolver and constitution seeding also support a preset-root $TemplateName.md. Such a convention-based layer will be ignored at command time, making the Bash result differ from initialization. Add the root-level fallback and keep all script variants aligned.

                if [ -z "$candidate" ] && [ "$manifest_declared" = false ]; then
                    local cf="$presets_dir/$preset_id/templates/${template_name}.md"
                    [ -f "$cf" ] && candidate="$cf"

scripts/powershell/common.ps1:594

• This convention fallback omits the supported preset-root $TemplateName.md location. A constitution seeded from that location by PresetResolver can therefore resolve to a different lower layer on the next /constitution run. Check both convention locations consistently across script variants.

                if (-not $candidate -and -not $manifestDeclared) {
                    $cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md"
                    if (Test-Path $cf) { $candidate = $cf }

• Files reviewed: 43/43 changed files
• Comments generated: 2
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the review feedback in commit b30a63da: runtime template resolution now uses the dedicated composed-content scripts, validates template and registry path components, honors preset root conventions, and applies extension enabled state and priority consistently across Bash, PowerShell, Python, and the canonical resolver. The redundant --content CLI endpoint is not part of the final design.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Runtime priority ordering can diverge from the canonical resolver, and Python file generation changes line endings on Windows.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (6)

scripts/powershell/common.ps1:569

• The canonical registry sorts equal priorities alphabetically by preset ID, but this sort uses priority alone. Since priority 10 is the default, installing the same presets in different orders can produce a different winning/composed runtime template than PresetResolver. Add Name as the secondary sort key.

                        Sort-Object { & $priorityFor $_ } |

scripts/python/setup_plan.py:71

• Path.write_text() performs platform newline translation, so on Windows the Python variant changes LF template content to CRLF. The previous shutil.copy() and the Bash/PowerShell variants preserve the resolved content, making generated plan.md differ by script type. Write encoded bytes to preserve parity.

            paths.impl_plan.write_text(template_content, encoding="utf-8")

scripts/powershell/common.ps1:561

• Mixed priority representations are deliberately treated as a parse failure here, after which lines 578-582 scan every preset directory and ignore registry disabled state. Canonical registry resolution normalizes numeric strings, booleans, and invalid values instead; normalize each entry locally so one corrupted priority cannot activate disabled presets.

This issue also appears on line 569 of the same file.

                    if ($presetEntries.Count -gt 1) {
                        $allNumeric = $true
                        $allStrings = $true
                        foreach ($entry in $presetEntries) {
                            $priority = & $priorityFor $entry

scripts/python/create_new_feature.py:395

• Writing through text mode translates LF to CRLF on Windows, unlike the prior byte-preserving copy and the Bash/PowerShell implementations. This makes spec.md depend on the selected script variant; write UTF-8 bytes directly instead.

                spec_file.write_text(template_content, encoding="utf-8")

scripts/python/common.py:223

• Preset priorities are sorted before normalization and without the canonical ID tie-breaker. Mixed values such as "1" and 2 trigger the directory fallback, which re-enables disabled presets; equal priorities (the default for most installs) instead retain install order, so a different layer can win than in PresetRegistry.list_by_priority(). Sort by normalized priority and preset ID.

                    key=lambda kv: kv[1].get("priority", 10)
                    if isinstance(kv[1], dict)
                    else 10,
                )
                if (

scripts/bash/common.sh:608

• This raw priority sort neither normalizes registry values nor applies the canonical preset-ID tie-breaker. Mixed values can fall back to scanning every directory (including disabled presets), while equal default priorities retain install order and can select a different layer than PresetRegistry.list_by_priority(). Sort by normalized priority and ID.

    for pid, meta in sorted(presets.items(), key=lambda x: x[1].get('priority', 10) if isinstance(x[1], dict) else 10):
        if isinstance(meta, dict) and bool(meta.get('enabled', True)) and re.fullmatch(r'[a-z0-9-]+', pid):

• Files reviewed: 43/43 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed review 4856318595 in commit 5e777eb2: preset priorities now use canonical normalization and ID tie-breaking across Bash, PowerShell, and Python, including mixed, floating-point, large-integer, and invalid string values. Python template materialization now writes UTF-8 bytes so generated spec.md and plan.md preserve LF content on Windows.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Python newline conversion breaks byte preservation, and irrelevant lower layers can incorrectly invalidate a winning override.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

scripts/python/common.py:406

• A project override is an unconditional highest-priority replace, but the resolver still parses every lower preset before returning it. Consequently, an unrelated malformed preset manifest—or missing PyYAML—makes /constitution and other template consumers fail even though none of those layers can contribute. Short-circuit on the override in all three resolver variants (and similarly stop once the effective top layer is replace) so lower irrelevant layers cannot invalidate a valid resolution.

    if override.is_file():
        layers.append((override, "replace"))

scripts/python/common.py:440

• Path.read_text() performs universal-newline conversion, so a CRLF template is normalized to LF before setup_plan.py or create_new_feature.py re-encodes it with write_bytes(). This defeats the byte-preservation change and diverges from the Bash and PowerShell resolvers. Read and decode the raw bytes for every layer so existing line endings survive composition.

        content = layers[base_index][0].read_text(encoding="utf-8")
        for path, strategy in reversed(layers[:base_index]):
            layer_content = path.read_text(encoding="utf-8")

• Files reviewed: 43/43 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed review 4856706470 in commit b4196db5: all runtime resolvers now stop at a winning project override or the first effective replace base, so malformed lower layers cannot invalidate the result. Python now decodes raw layer bytes rather than using universal-newline text reads, preserving CRLF content through materialization.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Runtime resolvers currently diverge from canonical extension conventions and cross-variant behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (5)

scripts/bash/common.sh:746

• This omits the root-level extension template convention supported by the canonical resolver (src/specify_cli/presets/__init__.py:5035-5038,5091-5102). Projects with .specify/extensions/<id>/<template>.md will resolve that layer during init but Bash command-time resolution silently falls through to a different layer.

            local ext="$ext_dir/$extension_id"
            local candidate="$ext/templates/${template_name}.md"
            if [ -f "$candidate" ]; then
                layer_paths+=("$candidate")
                layer_strategies+=("replace")
                effective_base_found=true
                break
            fi

scripts/python/common.py:449

• Runtime composition only checks an extension's templates/ directory, but the canonical resolver treats both templates/<name>.md and root-level <name>.md as valid template conventions (src/specify_cli/presets/__init__.py:5035-5038,5091-5102). A root-level extension template therefore wins during init/canonical resolution but is skipped by the new command-time resolver, causing /constitution and the other migrated commands to use a lower layer instead.

        candidate = extension_dir / "templates" / f"{template_name}.md"
        if candidate.is_file():
            layers.append((candidate, "replace"))
            return compose_from_base()

scripts/powershell/common.ps1:671

• The canonical extension lookup supports both templates/<name>.md and root-level <name>.md (src/specify_cli/presets/__init__.py:5035-5038,5091-5102), while this runtime path checks only the former. PowerShell command-time resolution can therefore disagree with init and skip the effective extension layer.

        foreach ($extensionId in Get-SortedExtensionIds -ExtensionsDir $extDir) {
            $candidate = Join-Path $extDir "$extensionId/templates/$TemplateName.md"
            if (Test-Path $candidate) {
                $layerPaths += $candidate
                $layerStrategies += 'replace'
                $effectiveBaseFound = $true
                break
            }

scripts/bash/common.sh:465

• When no Python executable is available, this fallback emits every extension directory alphabetically and ignores .registry priorities and enabled state. The Bash resolver can consequently select content from a disabled extension or the wrong priority layer, contradicting the new cross-variant resolution contract. The fallback should either parse the registry with another available JSON tool or fail explicitly rather than silently changing semantics.

This issue also appears on line 739 of the same file.

    local ext extension_id
    for ext in "$ext_dir"/*/; do
        [ -d "$ext" ] || continue
        extension_id=$(basename "$ext")
        case "$extension_id" in *[!a-z0-9-]*) continue ;; esac
        printf '%s\n' "$extension_id"
    done

scripts/python/check_prerequisites.py:230

• --template is ignored unless Python is also given --json, whereas the new Bash and PowerShell implementations resolve it before choosing output mode and fail if it is missing or malformed. Thus the same non-JSON invocation has different exit status across supported script variants. Either validate that --template requires JSON consistently or resolve it in Python before the output-mode branch.

    if args.json_mode:
        payload: dict[str, object] = {
            "FEATURE_DIR": str(paths.feature_dir),
            "AVAILABLE_DOCS": docs,
        }
        if args.template_name:

• Files reviewed: 43/43 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The three runtime variants currently handle structurally malformed preset manifests inconsistently.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

scripts/python/common.py:356

• An empty preset.yml is accepted here because safe_load() returning None is coerced to {}; the conventional template then resolves successfully. Bash and PowerShell reject the same manifest as a non-mapping root, so runtime behavior depends on the selected script variant. Let the existing mapping check reject None instead.

            manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) or {}

scripts/python/common.py:371

• Non-mapping template entries are silently skipped, so a malformed active manifest such as templates: [null] can still fall back to and resolve a conventional template under Python. The Bash/PowerShell parsers fail this shape, and PresetManifest explicitly rejects it (tests/test_presets.py:342-353), so this breaks script-variant parity.

            for entry in templates:
                if (
                    not isinstance(entry, dict)
                    or entry.get("name") != template_name
                    or entry.get("type", "template") != "template"
                ):
                    continue

scripts/bash/common.sh:680

• The embedded parser does not validate the provides.templates container. A malformed but empty non-list such as templates: {} iterates zero times, is reported as absent, and allows a conventional template to resolve; the Python variant rejects that shape. Validate the nested containers before iteration so malformed effective manifests fail consistently.

    if not isinstance(data, dict):
        raise ValueError('manifest root must be a mapping')
    for t in data.get('provides', {}).get('templates', []):
        if t.get('name') == os.environ['SPECKIT_TMPL'] and t.get('type', 'template') == 'template':

scripts/powershell/common.ps1:606

• The embedded parser does not validate the provides.templates container. A malformed but empty non-list such as templates: {} iterates zero times, is reported as absent, and allows a conventional template to resolve; the Python variant rejects that shape. Validate the nested containers before iteration so malformed effective manifests fail consistently.

    if not isinstance(data, dict):
        raise ValueError('manifest root must be a mapping')
    for t in data.get('provides', {}).get('templates', []):
        if t.get('name') == sys.argv[2] and t.get('type', 'template') == 'template':

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the latest review by enforcing the same preset-manifest structure in the Bash, PowerShell, and Python runtime resolvers. Empty documents, non-list provides.templates values, and non-mapping template entries now fail consistently instead of falling through to conventional templates; parity coverage includes every malformed shape. Pushed as 8d1f99b.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Malformed manifest handling and extension-ID matching still diverge across runtime variants.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

scripts/python/common.py:383

• Non-string file values are coerced into path text here, so a malformed entry such as file: null is silently ignored (or can resolve a file literally named True/123) in the Python variant. The Bash and PowerShell manifest probes fail on the same value, and PresetManifest also rejects it, so runtime resolution loses the promised cross-variant malformed-manifest parity. Validate file and strategy as strings before constructing the path.

                relative = Path(str(entry.get("file", "")))
                if (
                    not relative
                    or relative.is_absolute()
                    or ".." in relative.parts
                ):
                    return None
                candidate = preset_dir / relative
                if not candidate.is_file():
                    return None
                return candidate, str(entry.get("strategy", "replace")).lower()

scripts/powershell/common.ps1:391

• PowerShell's -notin comparison is case-insensitive, so an invalid registered ID such as FOO suppresses a valid unregistered foo/ directory. Bash, Python, and the canonical resolver use case-sensitive ID membership and would include foo, producing a different winning extension. Use the case-sensitive membership operator here.

    foreach ($directory in Get-ChildItem -Path $ExtensionsDir -Directory -ErrorAction SilentlyContinue) {
        if ($directory.Name -cmatch '^[a-z0-9-]+$' -and $directory.Name -notin $registeredNames) {
            $ranked += [PSCustomObject]@{ Priority = 10; Id = $directory.Name }
        }

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the latest review by validating preset template file and strategy values as strings in all runtime variants and by making PowerShell extension-registry membership case-sensitive. Added parity regressions for malformed fields and case-colliding extension IDs. Pushed as 9151bfe.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Runtime validators can overlook malformed unrelated manifest entries, and Bash can mask an override read failure as successful resolution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

scripts/bash/common.sh:602

• This explicit return 0 masks a failed cat (for example, if the override becomes unreadable or disappears after Test-Path). Bash then reports successful resolution with empty or partial content, unlike the Python and PowerShell variants. Propagate the read failure as a composition error instead.

        cat "$override"
        return 0

scripts/bash/common.sh:694

• Only the matching entry's file and strategy types are validated. A malformed value on another manifest entry is silently ignored, so Bash does not consistently reject structurally malformed manifests. Move both type checks before the requested-template condition, matching the full manifest validation contract.

    for t in templates:
        if not isinstance(t, dict):
            raise ValueError('manifest template entries must be mappings')
        if t.get('name') == os.environ['SPECKIT_TMPL'] and t.get('type', 'template') == 'template':
            file_value = t.get('file', '')
            strategy = t.get('strategy', 'replace')
            if not isinstance(file_value, str):
                raise ValueError('manifest template file must be a string')
            if not isinstance(strategy, str):
                raise ValueError('manifest template strategy must be a string')

scripts/powershell/common.ps1:620

• The parser validates file and strategy only for the requested template entry. Thus the same malformed manifest can fail in one resolution and be accepted in another when the bad entry has a different name, breaking the promised structural-validation parity. Validate every entry's field types before filtering by name/type.

    for t in templates:
        if not isinstance(t, dict):
            raise ValueError('manifest template entries must be mappings')
        if t.get('name') == sys.argv[2] and t.get('type', 'template') == 'template':
            file_value = t.get('file', '')
            strategy = t.get('strategy', 'replace')
            if not isinstance(file_value, str):
                raise ValueError('manifest template file must be a string')
            if not isinstance(strategy, str):
                raise ValueError('manifest template strategy must be a string')

scripts/python/common.py:378

• The type checks occur only after filtering to the requested template, so a manifest containing a non-string file or strategy on any other template entry is accepted. That leaves structurally malformed installed manifests behaving differently depending on which template is resolved, contrary to the cross-runtime malformed-manifest validation added here. Validate these fields for every mapping before the name/type filter.

            for entry in templates:
                if not isinstance(entry, dict):
                    raise ValueError("manifest template entries must be mappings")
                if (
                    entry.get("name") != template_name
                    or entry.get("type", "template") != "template"
                ):
                    continue
                file_value = entry.get("file", "")
                strategy = entry.get("strategy", "replace")
                if not isinstance(file_value, str):
                    raise ValueError("manifest template file must be a string")
                if not isinstance(strategy, str):
                    raise ValueError("manifest template strategy must be a string")

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the latest review by validating every preset manifest entry before selecting the requested template and by propagating failures from every Bash template-layer read. Added regressions for malformed entries after a valid match and for override read failures. Pushed as 11d33aa.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

🟡 Not ready to approve

Runtime manifest validation remains incomplete, and locale-dependent decoding can break valid UTF-8 registries and manifests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (7)

scripts/bash/common.sh:679

• This subprocess opens a UTF-8 preset manifest with the interpreter's locale encoding. On Windows or a non-UTF-8 shell locale, a valid manifest containing non-ASCII metadata can fail decoding, while the Python resolver and canonical PresetManifest explicitly use UTF-8. Specify encoding='utf-8' to keep runtime parity.

    with open(os.environ['SPECKIT_MANIFEST']) as f:
        data = yaml.safe_load(f)

scripts/bash/common.sh:626

• The registry is defined and written as UTF-8, but this subprocess reads it with the locale encoding. On a non-UTF-8 locale, unrelated Unicode metadata can trigger the exception path and silently replace registry priority/enabled-state resolution with a directory scan. Open the registry with encoding='utf-8' so the Bash path preserves canonical ordering.

    with open(os.environ['SPECKIT_REGISTRY']) as f:
        data = json.load(f)

scripts/python/common.py:373

• The new per-entry validation still accepts structurally malformed entries. For example, an entry for this template with no type is treated as a template, while an entry with no file, a non-string name/type, or an unsupported type is silently skipped; unsupported strategy strings on unrelated entries are also never rejected. This diverges from PresetManifest's required-field/type/value checks (src/specify_cli/presets/__init__.py:382-430) and from the PR's guarantee that every template entry is validated. Validate the required type, name, and file fields and allowed type/strategy values for every entry before searching for the match.

            for entry in templates:
                if not isinstance(entry, dict):
                    raise ValueError("manifest template entries must be mappings")
                file_value = entry.get("file", "")
                strategy = entry.get("strategy", "replace")
                if not isinstance(file_value, str):
                    raise ValueError("manifest template file must be a string")
                if not isinstance(strategy, str):
                    raise ValueError("manifest template strategy must be a string")

scripts/bash/common.sh:696

• The new per-entry validation still accepts structurally malformed entries. An entry missing type is treated as a template, while missing file, non-string name/type, unsupported types, and unsupported strategy strings on unrelated entries can all pass without failing resolution. This diverges from the canonical PresetManifest checks (src/specify_cli/presets/__init__.py:382-430) and the stated every-entry validation guarantee. Apply those required-field, type, and allowed-value checks before looking for the requested template.

    for t in templates:
        if not isinstance(t, dict):
            raise ValueError('manifest template entries must be mappings')
        file_value = t.get('file', '')
        strategy = t.get('strategy', 'replace')
        if not isinstance(file_value, str):
            raise ValueError('manifest template file must be a string')
        if not isinstance(strategy, str):
            raise ValueError('manifest template strategy must be a string')

scripts/powershell/common.ps1:619

• The embedded parser still accepts structurally malformed entries because it only type-checks file and strategy. Missing type defaults to template; missing file, non-string name/type, unsupported types, and invalid strategy strings on unrelated entries can be silently ignored. That differs from PresetManifest (src/specify_cli/presets/__init__.py:382-430) and the PR's every-entry validation guarantee. Validate required fields, their types, and allowed type/strategy values before matching the requested template.

    for t in templates:
        if not isinstance(t, dict):
            raise ValueError('manifest template entries must be mappings')
        file_value = t.get('file', '')
        strategy = t.get('strategy', 'replace')
        if not isinstance(file_value, str):
            raise ValueError('manifest template file must be a string')
        if not isinstance(strategy, str):
            raise ValueError('manifest template strategy must be a string')

scripts/powershell/common.ps1:602

• This reads the UTF-8 preset manifest using Python's locale-dependent default encoding. A valid manifest with non-ASCII metadata can therefore fail on Windows/non-UTF-8 locales even though the canonical and Python runtime resolvers accept it. Open it explicitly as UTF-8.

    with open(sys.argv[1]) as f:
        data = yaml.safe_load(f)

scripts/bash/common.sh:435

• This registry read is also locale-dependent. If valid UTF-8 extension metadata contains characters the shell locale cannot decode, the broad exception handler treats the installed registry as empty and loses its enabled-state and priority decisions. Read it with encoding='utf-8', matching the dedicated Python resolver.

This issue also appears in the following locations of the same file:

• line 625
• line 678

if registry.is_file():
    try:
        data = json.loads(registry.read_text())
        value = data.get('extensions', {}) if isinstance(data, dict) else {}
        registered = value if isinstance(value, dict) else {}
    except Exception:
        registered = {}

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the latest review round in 425c6e8:

• Forced UTF-8 decoding for preset registry and manifest reads in the Bash and PowerShell embedded-Python parsers, so runtime composition no longer depends on the process locale (matching the Python runtime resolver, which already read UTF-8 explicitly).
• Expanded per-entry manifest validation in all three runtime resolvers to check required fields, allowed template types, and allowed strategies (including the script-type strategy restriction), aligning them with the canonical PresetManifest contract.
• Added parity regressions: structural-validation cases (missing type/file, unsupported type, unsupported strategy on an unrelated entry) and a UTF-8-under-C-locale test that fails without the encoding fix.

Validation: pytest on the parity/preset suites (654 passed), ruff check src tests, bash -n, and a PowerShell parser check all pass.

Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously.

[Copilot]

🟡 Not ready to approve

Manifest validation and registry decoding still diverge from the promised fail-closed, UTF-8-consistent behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (6)

scripts/powershell/common.ps1:610

• The embedded validator accepts missing required sections and an empty template list, while canonical PresetManifest rejects both (src/specify_cli/presets/__init__.py:289-375). A corrupted manifest can consequently fall back to convention lookup as a replace layer instead of failing closed. Apply the full structural checks consistently in all three runtime resolvers.

    if not isinstance(data, dict):
        raise ValueError('manifest root must be a mapping')
    provides = data.get('provides', {})
    if not isinstance(provides, dict):
        raise ValueError('manifest provides must be a mapping')
    templates = provides.get('templates', [])
    if not isinstance(templates, list):
        raise ValueError('manifest templates must be a list')

scripts/bash/common.sh:687

• The embedded validator defaults missing provides/templates to empty containers, so {} and an empty template list are accepted even though canonical PresetManifest rejects them (src/specify_cli/presets/__init__.py:289-375). This can silently fall back to convention lookup with replace semantics after manifest corruption. Apply the full structural checks consistently in all three runtime resolvers.

    provides = data.get('provides', {})
    if not isinstance(provides, dict):
        raise ValueError('manifest provides must be a mapping')
    templates = provides.get('templates', [])
    if not isinstance(templates, list):
        raise ValueError('manifest templates must be a list')

scripts/powershell/common.ps1:534

• This still decodes the preset registry with PowerShell's process-default encoding. Windows PowerShell 5.1 defaults to the active ANSI code page, so this does not provide the locale-independent UTF-8 registry decoding claimed by the PR. Read the file explicitly as UTF-8 before ConvertFrom-Json.

                $registryData = Get-Content $registryFile -Raw | ConvertFrom-Json

scripts/python/common.py:394

• This accepts {} or provides: {templates: []} and then falls back to a convention file, although PresetManifest rejects missing required sections and an empty template list (src/specify_cli/presets/__init__.py:289-375). A post-install manifest corruption can therefore silently turn a declared composing layer into a replace layer instead of failing closed, contrary to the stated canonical validation. Apply the full structural checks consistently in all three runtime resolvers.

            provides = manifest.get("provides", {})
            if not isinstance(provides, dict):
                raise ValueError("manifest provides must be a mapping")
            templates = provides.get("templates", [])
            if not isinstance(templates, list):
                raise ValueError("manifest templates must be a list")

scripts/powershell/common.ps1:360

• The extension registry is likewise decoded using PowerShell's process-default encoding. This leaves extension priority/enabled-state resolution locale-dependent under Windows PowerShell 5.1 despite the PR's UTF-8 guarantee. Read it explicitly as UTF-8 before parsing.

This issue also appears in the following locations of the same file:

• line 534
• line 603

            $data = Get-Content $registryFile -Raw | ConvertFrom-Json

scripts/bash/common.sh:435

• Parse failures are swallowed here and registered becomes empty, after which every on-disk extension directory is treated as unregistered and enabled. A malformed or unreadable installed registry can therefore activate a disabled extension and contradicts the stated fail-closed Bash behavior. Let registry read/shape errors make this helper return nonzero instead of scanning directories.

This issue also appears on line 682 of the same file.

if registry.is_file():
    try:
        data = json.loads(registry.read_text(encoding='utf-8'))
        value = data.get('extensions', {}) if isinstance(data, dict) else {}
        registered = value if isinstance(value, dict) else {}
    except Exception:
        registered = {}

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mnriem]

Addressed the latest review round in d6f426f:

• Manifest fail-closed: all three runtime resolvers (Bash, PowerShell, Python) now reject manifests that are missing the provides/templates sections or declare an empty template list, matching the canonical PresetManifest. Previously a corrupted manifest could silently fall back to a convention replace lookup instead of failing closed.
• Extension registry fail-closed: a corrupt or unreadable extension registry now fails closed in all three variants instead of swallowing the error and treating every on-disk extension directory as unregistered-and-enabled (which could activate a disabled extension). A missing extensions key is still treated as an empty registry.
• PowerShell UTF-8: the native preset and extension registry reads now decode explicitly as UTF-8, so priority/enabled-state resolution no longer depends on the active code page under Windows PowerShell 5.1.

Added parity regressions for each: malformed-manifest cases (missing_provides, empty_templates) and a test_all_variants_fail_for_malformed_extension_registry case (invalid JSON, non-mapping root, non-mapping extensions). I verified each new test fails when its fix is reverted.

Validation: full suite (pytest -q, 6552 passed / 8 skipped), ruff check src tests, bash -n, and a PowerShell parser check all pass.

Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously.

[Copilot]

🟡 Not ready to approve

Bash and Python still fail open when the extension registry path is a directory or broken symlink.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

scripts/python/common.py:247

• A non-file registry still fails open here. If .specify/extensions/.registry is a directory or broken symlink, is_file() is false, so every safe on-disk extension directory is treated as unregistered and its template can be served. This contradicts the new fail-closed behavior for corrupt/unreadable extension registries; detect an existing directory entry first and reject it unless it is a readable file.

    if registry.is_file():

scripts/bash/common.sh:431

• This check also treats a directory or broken symlink at .registry as if no registry existed, then ranks all extension directories as unregistered. The no-Python branch below does the same via -f, so a corrupt registry can bypass the intended fail-closed behavior and expose extension templates. Detect any filesystem entry at .registry (including symlinks), and reject it unless it is a readable regular file before either branch scans directories.

if registry.is_file():

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.