feat(presets): resolve constitution templates at command time - #3984
feat(presets): resolve constitution templates at command time#3984mnriem wants to merge 16 commits into
Conversation
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
There was a problem hiding this comment.
🟡 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
/constitutionguidance 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.
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
|
Addressed the command-time composition review in Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
There was a problem hiding this comment.
🟡 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.
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
|
Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). Commit The change preserves the existing diagnostic |
There was a problem hiding this comment.
🟡 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 supplyconstitution-template, and registered extension priorities are not honored, so/constitutiondoes not actually use the same full stack asPresetResolver._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
PresetResolversearches bothtemplates/<name>.mdand the preset-root<name>.mdconvention, and the install-time constitution code explicitly recognizes both. This helper only checks thetemplates/convention, so/constitutionsilently 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.mdlocation. A constitution seeded from that location byPresetResolvercan therefore resolve to a different lower layer on the next/constitutionrun. 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.
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>
|
Addressed the review feedback in commit Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
There was a problem hiding this comment.
🟡 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. AddNameas 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 previousshutil.copy()and the Bash/PowerShell variants preserve the resolved content, making generatedplan.mddiffer 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.mddepend 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"and2trigger 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 inPresetRegistry.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>
|
Addressed review Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
There was a problem hiding this comment.
🟡 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/constitutionand 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 isreplace) 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 beforesetup_plan.pyorcreate_new_feature.pyre-encodes it withwrite_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>
|
Addressed review Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
There was a problem hiding this comment.
🟡 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>.mdwill 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 bothtemplates/<name>.mdand root-level<name>.mdas 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/constitutionand 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>.mdand 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
.registrypriorities 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
--templateis 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--templaterequires 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>
There was a problem hiding this comment.
🟡 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 0masks a failedcat(for example, if the override becomes unreadable or disappears afterTest-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
fileandstrategytypes 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
fileandstrategyonly 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
fileorstrategyon 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.
Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3158e06f-95df-4e3a-843f-f159a35aa30c
|
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). |
There was a problem hiding this comment.
🟡 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
PresetManifestexplicitly use UTF-8. Specifyencoding='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
typeis treated as a template, while an entry with nofile, a non-stringname/type, or an unsupportedtypeis silently skipped; unsupported strategy strings on unrelated entries are also never rejected. This diverges fromPresetManifest'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 requiredtype,name, andfilefields 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
typeis treated as a template, while missingfile, non-stringname/type, unsupported types, and unsupported strategy strings on unrelated entries can all pass without failing resolution. This diverges from the canonicalPresetManifestchecks (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
fileandstrategy. Missingtypedefaults totemplate; missingfile, non-stringname/type, unsupported types, and invalid strategy strings on unrelated entries can be silently ignored. That differs fromPresetManifest(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.
Force UTF-8 decoding for registry and manifest reads in the Bash and PowerShell embedded-Python parsers so resolution no longer depends on the process locale, and validate every manifest template entry's required fields, type, and strategy consistent with the canonical PresetManifest. 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
|
Addressed the latest review round in 425c6e8:
Validation: Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously. |
There was a problem hiding this comment.
🟡 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
PresetManifestrejects both (src/specify_cli/presets/__init__.py:289-375). A corrupted manifest can consequently fall back to convention lookup as areplacelayer 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/templatesto empty containers, so{}and an empty template list are accepted even though canonicalPresetManifestrejects them (src/specify_cli/presets/__init__.py:289-375). This can silently fall back to convention lookup withreplacesemantics 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
{}orprovides: {templates: []}and then falls back to a convention file, althoughPresetManifestrejects 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 areplacelayer 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
registeredbecomes 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.
Reject manifests missing the provides/templates sections or declaring an empty template list in all three runtime resolvers, matching the canonical PresetManifest which treats those as invalid instead of silently degrading a composing layer to a convention `replace` lookup. Make a corrupt or unreadable extension registry fail closed in Bash, PowerShell, and Python instead of swallowing the error and treating every on-disk extension directory as unregistered-and-enabled, which could activate a disabled extension. Read the preset and extension registries as explicit UTF-8 in the PowerShell resolver so priority/enabled-state decoding no longer depends on the process code page under Windows PowerShell 5.1. 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
|
Addressed the latest review round in d6f426f:
Added parity regressions for each: malformed-manifest cases ( Validation: full suite ( Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously. |
There was a problem hiding this comment.
🟡 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/.registryis 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
.registryas 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
|
Addressed review 4858629667 (fail-open on non-regular-file extension registry). The Bash and Python resolvers gated the extension Added directory- and broken-symlink parity regressions in Commit: f9732f9 Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously. |
There was a problem hiding this comment.
🟡 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.
| # Add unregistered directories with implicit priority=10 | ||
| for ext_dir in self.extensions_dir.iterdir(): | ||
| if not ext_dir.is_dir() or ext_dir.name.startswith("."): | ||
| if not ext_dir.is_dir() or not self._is_safe_registry_id(ext_dir.name): |
| $registeredNames = @() | ||
| $ranked = @() | ||
| $registryFile = Join-Path $ExtensionsDir '.registry' | ||
| if (Test-Path -LiteralPath $registryFile) { |
…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
|
Addressed review 4864849239 (two remaining fail-open registry states). Canonical PowerShell broken symlink: Added canonical corrupt- and directory-registry regressions in Commit: 1299c50 Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8), acting autonomously. |
There was a problem hiding this comment.
🟡 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
replacelayer has already won. As a result, init/constitution-syncmaterialization 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.
| if not self.registry_path.exists(): | ||
| return False |
Summary
constitution-templateat command time through the dedicated Bash, PowerShell, or Python resolver scriptprovides/templatessections or declaring an empty template list, consistent with the canonicalPresetManifestPresetResolver(used by init andconstitution-syncmaterialization) 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.registrysymlink under Windows PowerShell), instead of returning successful empty/partial content or silently treating every on-disk extension directory as unregistered-and-enabledspecify preset resolvewhile rejecting empty or traversal-like segmentsreplacebase so malformed lower layers cannot invalidate a winning override or presetspec.mdandplan.mdconstitution-syncis enabled and document the lifecycleTesting
.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 -quvx ruff@0.15.0 check src testsCloses #3950
Authored by GitHub Copilot (model: Claude Opus 4.8), acting autonomously on behalf of @mnriem.