Skip to content

feat(presets): resolve constitution templates at command time - #3984

Merged
mnriem merged 19 commits into
github:mainfrom
mnriem:mnriem-feat-3950-runtime-constitution-resolutio
Aug 10, 2026
Merged

feat(presets): resolve constitution templates at command time#3984
mnriem merged 19 commits into
github:mainfrom
mnriem:mnriem-feat-3950-runtime-constitution-resolutio

Conversation

@mnriem

@mnriem mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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 across the Bash, Python, and PowerShell runtime resolvers and the canonical PresetResolver (used by init and constitution-sync materialization) when a Bash template layer cannot be read, and when the extension registry is corrupt, unreadable, or not a readable regular file (a directory or broken symlink at the registry path, including a dangling .registry symlink under Windows PowerShell), 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.

Gate install-time constitution materialization behind the constitution-sync preset while preserving one-time init seeding and authored-file safeguards.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7dbce70f-80c6-4e14-a30d-78cb358bcb84
Copilot AI balanced review requested due to automatic review settings August 4, 2026 12:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread templates/commands/constitution.md Outdated
Add a machine-readable preset resolve mode backed by PresetResolver.resolve_content and require the constitution command to consume it.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7dbce70f-80c6-4e14-a30d-78cb358bcb84
Copilot AI review requested due to automatic review settings August 4, 2026 12:54
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread src/specify_cli/presets/_commands.py Outdated
Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7dbce70f-80c6-4e14-a30d-78cb358bcb84
Copilot AI review requested due to automatic review settings August 4, 2026 13:52
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread scripts/python/common.py
Comment thread scripts/powershell/common.ps1
Align runtime resolution across script variants, validate registry path components, and honor canonical extension ordering and convention paths.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 15:31
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Normalize and tie-break preset priorities consistently across script variants, and preserve template bytes when Python materializes generated files.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 16:18
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Avoid parsing irrelevant lower layers once resolution reaches a replace base, and decode raw bytes so Python preserves source line endings.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 16:29
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Support root-level extension templates across runtime resolvers, fail safely when Bash cannot parse an extension registry, and validate requested templates in every prerequisite output mode.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

The Bash and Python resolvers used is_file()/`-f` to gate reading the
extension `.registry`, which returns false for a directory or a broken
symlink at that path. In those cases the resolvers treated the registry
as absent and scanned every on-disk extension directory as unregistered
and enabled — a fail-open path. Detect any filesystem entry at the
registry path (including broken symlinks) and reject unless it is a
readable regular file. PowerShell now rejects a non-leaf entry explicitly
for parity. Adds directory- and broken-symlink parity regressions.

Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3158e06f-95df-4e3a-843f-f159a35aa30c
Copilot AI review requested due to automatic review settings August 5, 2026 13:21
@mnriem

mnriem commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 4858629667 (fail-open on non-regular-file extension registry).

The Bash and Python resolvers gated the extension .registry read with is_file()/-f, which is false for a directory or a broken symlink at that path — so those cases were treated as "no registry" and every on-disk extension directory was scanned as unregistered-and-enabled (fail-open). Both now detect any filesystem entry at the registry path (including broken symlinks) and reject unless it is a readable regular file. PowerShell rejects a non-leaf entry explicitly for parity.

Added directory- and broken-symlink parity regressions in tests/test_resolve_template_python_parity.py (the broken-symlink case is scoped to Bash + Python); each was verified to fail when its fix is reverted. Ran the parity suite (39 passed, PowerShell included), bash -n, a pwsh parser check, and ruff.

Commit: f9732f9

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Canonical and PowerShell resolution still permit fail-open behavior for specific corrupt extension-registry states.

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: 44/44 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.

Comment thread src/specify_cli/presets/__init__.py
Comment thread scripts/powershell/common.ps1 Outdated
…nd PowerShell

Two remaining fail-open paths for an invalid extension registry:

- The canonical PresetResolver enumerated extensions through
  ExtensionRegistry, whose _load() normalizes a corrupt or unreadable
  registry to an empty mapping. The directory scan then admitted every
  on-disk extension directory as unregistered-and-enabled, so a corrupt
  registry could still supply constitution content at init and through
  constitution-sync materialization. Add a non-invasive is_corrupt()
  probe (recovery behavior for install/enable/disable is unchanged) and
  raise from _get_all_extensions_by_priority() when the registry exists
  but is invalid. _load() now also recovers from OSError/UnicodeDecodeError
  so a directory or unreadable registry no longer crashes construction.

- The PowerShell resolver gated the registry read with Test-Path, which
  returns false for a dangling symlink on Windows, letting a broken
  .registry symlink bypass the guard and enable every on-disk extension.
  Detect the entry via directory enumeration (which observes a broken
  symlink) and reject it unless it is a readable regular file.

Adds canonical corrupt/directory-registry regressions and extends the
broken-symlink parity test to PowerShell.

Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3158e06f-95df-4e3a-843f-f159a35aa30c
Copilot AI review requested due to automatic review settings August 5, 2026 13:49
@mnriem

mnriem commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 4864849239 (two remaining fail-open registry states).

Canonical PresetResolver (init + constitution-sync): ExtensionRegistry._load() normalizes a corrupt/unreadable registry to an empty mapping, after which _get_all_extensions_by_priority() admitted every on-disk directory as an unregistered, enabled extension — so a corrupt registry could still supply constitution content. Added a non-invasive is_corrupt() probe (recovery behavior for install/enable/disable is unchanged) and made the resolver raise PresetValidationError when the registry exists but is invalid. _load() also now recovers from OSError/UnicodeDecodeError, so a directory or unreadable registry no longer crashes construction. reconcile_constitution already catches PresetValidationError and warns, so materialization fails closed.

PowerShell broken symlink: Test-Path follows the link and returns false for a dangling .registry symlink on Windows, bypassing the guard. Now the entry is detected via directory enumeration (which observes a broken symlink) and rejected unless it is a readable regular file.

Added canonical corrupt- and directory-registry regressions in tests/test_presets.py (each verified to fail when the guard is reverted) and extended the broken-symlink parity test to PowerShell. Full suite: 6558 passed / 8 skipped; ruff, bash -n, and a pwsh parser check all clean.

Commit: 1299c50

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Dangling registries can still fail open, and canonical resolution does not stop at the first effective replacement layer.

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 (1)

src/specify_cli/presets/init.py:5357

  • The canonical resolver still collects every lower preset and then extensions after a project override or higher-priority replace layer has already won. As a result, init/constitution-sync materialization can fail on a corrupt lower extension registry (or inspect malformed lower layers), while all three runtime resolvers now return immediately at the first effective base. Stop canonical resolution before enumerating lower tiers so these paths retain the promised priority parity.
            for pack_id, metadata in self._get_all_presets_by_priority():
                pack_dir = self.presets_dir / pack_id
                # Read strategy and manifest file path from preset manifest
                strategy = "replace"
                manifest_has_strategy = False
  • Files reviewed: 45/45 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.

Comment thread src/specify_cli/extensions/__init__.py Outdated
…s_corrupt

is_corrupt() gated on Path.exists(), which follows symlinks and returns
False for a dangling .registry symlink — so the canonical PresetResolver
treated it as an absent registry and fell back to scanning every on-disk
extension directory as unregistered-and-enabled, reopening the fail-open
path this guard closes. Detect lexical existence with os.path.lexists and
require a regular file before parsing, so a broken symlink (or directory)
is reported corrupt and resolution fails closed. Adds a canonical
broken-symlink regression alongside the directory case.

Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3158e06f-95df-4e3a-843f-f159a35aa30c
Copilot AI review requested due to automatic review settings August 10, 2026 13:28
@mnriem

mnriem commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the three open Copilot review threads (all extension-registry fail-open follow-ups).

  • Canonical PresetResolver (src/specify_cli/presets/__init__.py): a corrupt registry normalized to {} and admitted every on-disk extension. Added a non-invasive ExtensionRegistry.is_corrupt() probe and made enumeration raise PresetValidationError when the registry exists but is invalid. — 1299c508
  • PowerShell (scripts/powershell/common.ps1): Test-Path follows a dangling symlink and reports false on Windows. Now the .registry entry is detected via directory enumeration and rejected unless a readable regular file. — 1299c508
  • ExtensionRegistry.is_corrupt() (src/specify_cli/extensions/__init__.py): still gated on Path.exists(), which follows a dangling symlink and mistook it for an absent registry. Switched to os.path.lexists() + require a regular file. — e453c508

Added canonical corrupt-, directory-, and broken-symlink regressions and extended the broken-symlink parity test to PowerShell; each guard verified to fail on revert. Full suite: 6559 passed / 8 skipped; ruff, bash -n, and a pwsh parser check all clean.

Latest commit: e453c50

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

src/specify_cli/presets/init.py:4982

  • collect_all_layers() calls this unconditionally after collecting overrides and presets, so a corrupt extension registry still aborts canonical constitution materialization even when a higher-priority replace override/preset has already won. For example, init with an override plus a corrupt .specify/extensions/.registry reaches this exception and leaves memory/constitution.md unseeded, contrary to the new “stop at the first effective replace base” behavior implemented by the runtime scripts. Short-circuit canonical layer collection once the winning replace base is found, while retaining this fail-closed check when extension enumeration is actually needed.
            return None, None
        for tmpl in manifest.templates:
            if tmpl.get("name") == template_name and tmpl.get("type") == template_type:
                file_path = tmpl.get("file")
                if file_path:

scripts/powershell/common.ps1:364

  • PowerShell comparison operators are case-insensitive by default, so on a case-sensitive filesystem this treats an unrelated .REGISTRY entry as .registry; the subsequent exact-path Test-Path fails and resolution rejects a state that Bash and Python treat as having no registry. Use a case-sensitive comparison to keep registry-path behavior consistent across variants.
        Where-Object { $_.Name -eq '.registry' } |
  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 10, 2026 13:36
@mnriem

mnriem commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Updated the branch — main had advanced ~30 commits and the PR went CONFLICTING. Merged main in; the one conflict was in ExtensionRegistry._load() between this branch's fail-closed work and the upstream registry UTF-8/OSError changes (#3998, #3834).

Resolved in favor of upstream's narrower except (json.JSONDecodeError, UnicodeDecodeError, FileNotFoundError) — upstream deliberately does not catch OSError so a temporarily-unreadable-but-intact registry isn't wiped by a later _save(). To keep a directory at .registry from crashing construction (the reason this branch had briefly caught OSError), _load() now guards is_file() before opening and recovers to empty. The fail-closed signal for resolution paths stays in the read-only is_corrupt() probe (which still treats a directory, dangling symlink, or unreadable file as corrupt).

Full suite green after the merge: 6694 passed / 8 skipped; ruff clean.

Latest commit: 86adcbe

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

src/specify_cli/presets/init.py:5012

  • collect_all_layers() always reaches this check even when it has already collected a project override or a preset with strategy: replace. Consequently, a corrupt extension registry below that effective base makes canonical PresetResolver.resolve_content() fail, while the new Bash/PowerShell/Python runtime resolvers return the winning replace layer without inspecting lower tiers. This breaks the stated first-replace short-circuit and affects init/constitution-sync; stop layer collection before enumerating extensions once an effective replace base has been found.
        return None, None

    def _extension_manifest_declared_template(
        self, ext_dir: Path, template_name: str, template_type: str

scripts/python/common.py:368

  • This runtime validator checks only that name and file are strings, so entries with an invalid name or an absolute/parent-traversing file path pass even though canonical PresetManifest rejects them (src/specify_cli/presets/__init__.py:425-469). For a matching unsafe file, _preset_template_layer() merely returns None, allowing resolution to succeed from a lower/core layer instead of rejecting the malformed manifest. Add the canonical name-format and normalized relative-path checks for every entry in all three runtime validators.
    for field in ("type", "name", "file"):
        if not isinstance(entry[field], str):
            raise ValueError(f"manifest template {field} must be a string")
  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 10, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (6)

scripts/powershell/common.ps1:742

  • Encoding.UTF8 replaces invalid template bytes instead of throwing, so a malformed winning layer is returned successfully with U+FFFD characters while the Bash and Python resolvers fail the read. Apply a strict UTF8Encoding(..., throwOnInvalidBytes: true) to every layer read in this function so template resolution fails closed consistently.
        return [System.IO.File]::ReadAllText($layerPaths[0], [System.Text.Encoding]::UTF8)

scripts/powershell/common.ps1:550

  • This UTF-8 decoder silently replaces malformed byte sequences. Consequently, a preset registry with invalid UTF-8 in unrelated metadata can still parse and honor its priorities in PowerShell, while Bash/Python reject it and fall back to directory ordering, potentially selecting a different preset. Decode with throwOnInvalidBytes so registry normalization remains cross-language consistent.
                $registryData = [System.IO.File]::ReadAllText($registryFile, [System.Text.Encoding]::UTF8) | ConvertFrom-Json

scripts/bash/common.sh:812

  • This extension lookup bypasses extension.yml, so a manifest-declared template whose filename is non-conventional is omitted (or a stale conventional file wins). That disagrees with the canonical extension resolution contract in src/specify_cli/presets/__init__.py:5527-5541 and its regression tests at tests/test_presets.py:11398-11511. Consult the authoritative manifest entry before falling back to these paths.
            local candidate="$ext/templates/${template_name}.md"
            [ -f "$candidate" ] || candidate="$ext/${template_name}.md"
            if [ -f "$candidate" ]; then

scripts/python/common.py:502

  • Runtime resolution ignores extension.yml and checks only the two conventional filenames. A registered extension that declares constitution-template (or another requested template) at a custom path therefore falls through to a lower layer, even though the canonical resolver treats that manifest entry as authoritative (src/specify_cli/presets/__init__.py:5527-5541; regression coverage at tests/test_presets.py:11398-11430). Resolve the extension manifest entry before applying the convention fallback.
    for extension_id in _sorted_extension_ids(extensions_dir):
        extension_dir = extensions_dir / extension_id
        candidate = _conventional_template(extension_dir, template_name)
        if candidate is not None:
            layers.append((candidate, "replace"))

scripts/powershell/common.ps1:721

  • Only probing conventional paths means this resolver cannot consume extension templates declared at custom paths in extension.yml, and it can select a stale conventional file despite the manifest. The canonical resolver explicitly makes extension manifests authoritative (src/specify_cli/presets/__init__.py:5527-5541; tests at tests/test_presets.py:11398-11511). Resolve the manifest entry first, then use conventions only when it is undeclared.
            $candidate = Join-Path $extDir "$extensionId/templates/$TemplateName.md"
            if (-not (Test-Path $candidate)) {
                $candidate = Join-Path $extDir "$extensionId/$TemplateName.md"
            }
            if (Test-Path $candidate) {

scripts/powershell/common.ps1:371

  • Encoding.UTF8 uses replacement fallback, so invalid UTF-8 bytes inside otherwise valid registry JSON are converted to U+FFFD and can parse successfully. Bash and Python decode strictly and reject the same corrupt extension registry, so PowerShell can violate the promised fail-closed parity. Use a strict UTF-8 decoder that throws on invalid bytes.

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

  • line 550
  • line 742
            $data = [System.IO.File]::ReadAllText($registryFile, [System.Text.Encoding]::UTF8) | ConvertFrom-Json
  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@mnriem
mnriem merged commit 16cfab7 into github:main Aug 10, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Resolve constitution-template at /constitution command time; gate install-time seeding behind constitution-sync

2 participants