feat: add specify artifact command exposing composition stacks as JSON - #4267
feat: add specify artifact command exposing composition stacks as JSON#4267nicolehaugen wants to merge 25 commits into
specify artifact command exposing composition stacks as JSON#4267Conversation
Adds a new `specify artifact` command group with two subcommands:
* `specify artifact list --json` — flat inventory of every command,
template, and script SpecKit exposes for the current project. Each row
carries a stable `id` (`{kind}:{name}`), an author-declared
`name`, its `kind`, and a `description` string that is never
omitted (empty string when the author declared none).
* `specify artifact info <name> --json` — the same row plus its full
ordered composition `stack`: highest-priority contributor first, with
`active` marking the winner `PresetResolver.resolve_content` would
return and `hidden` marking rows shadowed by a higher-priority
`replace`. Each stack entry carries a portable POSIX `manifestPath`
(or `null` for the core baseline) and a `lookupId` from the
contribution-id grammar so the output round-trips against
`specify preset info` and `specify extension info`.
The two commands share one strict JSON error envelope on stderr
(`{ "error": "..." }`) with exit code 1 for the three logical errors
(unknown artifact, ambiguous artifact, not a Spec Kit project) and exit
code 2 for the "`--json` is required" usage error. stdout is always
empty on error, so the two streams stay independently parseable.
Implementation lives in a new `src/specify_cli/artifacts/` subpackage
that mirrors the existing `presets/` and `extensions/` layout — pure
logic in `__init__.py` and thin Typer wiring in `_commands.py`. The
subpackage reuses `PresetResolver.collect_all_layers` for the actual
composition math and only reshapes each layer into a `StackLayer` JSON
row, so `active` and `hidden` stay in lockstep with the resolver's
winner-selection logic.
Skills (`.github/skills/**/SKILL.md`) are intentionally excluded from
the inventory — they are integration-specific installation output, not a
shipped asset family. The command still surfaces the underlying command
that a skill was generated from.
Tests:
* `tests/test_artifact_command.py` — 32 tests: contract shape, sort
order, empty-inventory behavior, kind-hint parsing, ambiguous-name
error, unknown-artifact error, not-a-project error, skills exclusion,
CLI wiring end-to-end (`--json` required, JSON envelope shape,
stderr-only errors, empty stdout on error, UTF-8 with no BOM), and
preset-replace hiding the core layer.
* `tests/test_artifact_command_parity.py` — 6 tests: `manifestPath`
uses forward slashes on every OS and is never absolute, the `active`
row corresponds to the resolver's actual winner, and the pretty-printed
JSON has no trailing whitespace and ends in exactly one newline.
All 38 new tests pass. Full presets + extensions regression suite is
green modulo pre-existing Windows-symlink-privilege failures that
predate this branch.
Assisted-by: GitHub Copilot (model: claude-opus-4.7, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4a40fb96-1bbe-4fb2-99d8-411170046cb0
…rt' and 'import from'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…rt' and 'import from'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds JSON-based artifact inventory and composition-stack introspection to the Specify CLI.
Changes:
- Adds
artifact listandartifact infocommands. - Implements artifact discovery and stack serialization.
- Adds CLI, resolver-parity, and cross-platform tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/__init__.py |
Registers the artifact command group. |
src/specify_cli/artifacts/__init__.py |
Implements inventory and stack logic. |
src/specify_cli/artifacts/_commands.py |
Provides Typer CLI and JSON output. |
tests/test_artifact_command.py |
Tests contracts and CLI behavior. |
tests/test_artifact_command_parity.py |
Tests resolver and path parity. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
src/specify_cli/artifacts/init.py:669
- This directory scan includes disabled presets and unregistered preset directories, while
PresetResolver._get_all_presets_by_priority()only composes enabled registry entries (src/specify_cli/presets/__init__.py:5067-5073). As a result,artifact listcan advertise artifacts thatartifact infoimmediately reports as unknown. Enumerate the same enabled source set as the resolver; retain the resolver's intentional unregistered-extension behavior separately.
for tier in ("presets", "extensions"):
tier_dir = specify_dir / tier
if not tier_dir.is_dir():
continue
for pack_dir in sorted(tier_dir.iterdir(), key=lambda p: p.name):
if not pack_dir.is_dir():
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_artifact_command.py:60
- This helper does not create a valid preset manifest: production presets require
schema_version,preset,requires, and a singleprovides.templates[]list whose entries carrytypeandfile.PresetManifesttherefore rejects every fixture created here, and the tests accidentally exercise convention fallback instead of manifest-backed preset composition. Build fixtures in the real schema so these tests can catch the production parsing bug.
"id": pack_id,
tests/test_artifact_command_parity.py:23
- This duplicated helper writes the same invalid preset schema (
id/metadataat the root and sectionedprovidesentries). SincePresetManifestrejects it, the parity test reaches the preset file only through convention fallback and never verifies manifest/resolver parity. Generate the canonicalschema_version/preset/requires/provides.templates[type,file]shape instead.
manifest = {
src/specify_cli/artifacts/init.py:282
- Core scripts are listed by physical filename, so each Bash/PowerShell/Python variant becomes a different artifact (for example,
setup-plan.sh,setup-plan.ps1, andsetup_plan.py). More importantly,get_artifact_info()passes that filename toPresetResolver.collect_all_layers(), which appends.shand looks outside the runtime subdirectory, so these advertised list entries all resolve asunknown artifact. Normalize variants to a logical script name and make stack lookup use the actual runtime paths before exposing them.
name = entry.name
src/specify_cli/artifacts/_commands.py:53
_resolve_init_dir_override()itself prints Rich errors and raisestyper.Exitfor an invalidSPECIFY_INIT_DIR, so this call bypasses the artifact JSON error handler. In--jsonmode stderr is then plain Rich text rather than the promised{"error": ...}envelope. Add a quiet/project-resolution API that raises anArtifactError, or translate validation without letting the shared helper emit first.
from .._project import _resolve_init_dir_override
override = _resolve_init_dir_override()
cwd = override if override is not None else Path.cwd()
if not (cwd / ".specify").is_dir():
raise NotASpecKitProjectError()
tests/test_artifact_command.py:24
StackLayeris unused, and the repository's Python lint job runs Ruff overtests, so this import triggers F401 and blocks CI. Remove it from the import list.
)
src/specify_cli/artifacts/init.py:689
- Preset manifests do not share the extension
provides.commands/templates/scriptsshape: all preset contributions live inprovides.templates[], withtypeidentifying command/template/script (PresetManifest.iter_contributions()at src/specify_cli/presets/init.py:546-568). Consequently real preset commands and scripts are omitted, and preset scripts are misclassified as templates. Parse each tier through its manifest model instead of applying the extension shape to both.
Both preset and extension manifests use the same ``provides`` shape:
src/specify_cli/artifacts/init.py:486
collect_all_layers()emits project overrides with aproject:lookup ID, but this catch-all converts every non-core/non-extension layer into a preset. A real.specify/templates/overrides/<name>.mdtherefore appears aslayer: "preset",presetId: "_", which is false metadata. Handle theproject:layer explicitly (and update the public layer contract accordingly) rather than falling through to preset handling.
layer="preset",
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (8)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_artifact_command.py:63
- This helper writes an invalid preset manifest: the real schema requires
preset,requires, and a mixedprovides.templateslist with atypeper entry.PresetManifesttherefore rejects this fixture, and the stack tests pass through convention fallback instead of exercising manifest-declared presets, masking the production parser mismatch.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
tests/test_artifact_command_parity.py:27
- This duplicated helper also writes the extension-style
providesshape instead of a valid preset (preset/requiresplus typed entries underprovides.templates). The resolver rejects it and the parity test exercises convention fallback, so it does not verify parity for a real manifest-declared preset.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
src/specify_cli/artifacts/init.py:672
- This enumerates raw on-disk declarations rather than the resolver-visible inventory. Disabled extensions, disabled/unregistered presets, and entries whose declared file is missing can therefore appear in
artifact list, whileartifact inforeturnsunknown artifactfor the same ID. Build the list from the same enabled registry/resolver sources used bycollect_all_layers()(and only retain names with a non-empty stack).
for pack_dir in sorted(tier_dir.iterdir(), key=lambda p: p.name):
if not pack_dir.is_dir():
continue
manifest_name = "preset.yml" if tier == "presets" else "extension.yml"
manifest = pack_dir / manifest_name
src/specify_cli/artifacts/init.py:707
- Preset manifests do not use these three sibling lists: valid presets put commands, templates, and scripts together under
provides.templates, distinguished by each entry'stype(seepresets/lean/preset.yml:15-18). Consequently valid preset contributions are omitted or classified as templates. Parse presets viaPresetManifest.iter_contributions()and extensions viaExtensionManifest.iter_contributions()instead of applying the extension shape to both.
for kind_key, kind_value in (
("commands", "command"),
("templates", "template"),
("scripts", "script"),
src/specify_cli/artifacts/init.py:284
- Using the physical filename as the artifact name exposes separate
.sh,.ps1, and.pyrows (including underscore-vs-hyphen variants), whereas script contribution IDs use logical names such assetup-plan(tests/test_contribution_ids.py:124-127). These listed names cannot be resolved:collect_all_layers()appends.sh, soscript:setup-plan.shsearches forsetup-plan.sh.sh, and bundled scripts live in runtime subdirectories. Normalize runtime variants to one logical script artifact and make stack lookup resolve that logical name to the actual runtime files.
name = entry.name
if name in seen:
continue
src/specify_cli/artifacts/init.py:484
- A project override has a
project:_:{kind}:{name}lookup ID, so it falls through both branches and is emitted as a preset with ID/name_. Project overrides are the documented highest-precedence resolver tier (docs/reference/presets.md:148-153) and need an explicitprojectlayer representation with null preset/manifest fields rather than being mislabeled.
pack_id = _extract_lookup_pack_id(lookup_id) or ""
pack_dir = project_root / ".specify" / "presets" / pack_id
display = _preset_display_name(pack_dir, pack_id) if pack_id else pack_id
manifest_path = _derive_manifest_path(layer, project_root)
rows.append(
src/specify_cli/artifacts/init.py:395
- Valid preset manifests store the human-readable name at
preset.name, not at top-levelname(ormetadata.name). As written, every normal preset stack reports the pack ID aspresetNameinstead of its display name.
display = data.get("name")
if isinstance(display, str) and display:
return display
src/specify_cli/artifacts/_commands.py:50
_resolve_init_dir_override()prints a Rich error and raisestyper.ExitwhenSPECIFY_INIT_DIRis invalid (src/specify_cli/_project.py:43-52). That bypasses the JSON error handler, so a--jsoninvocation can emit non-JSON stderr despite this module's strict envelope contract. Add a non-emitting resolution path that converts these failures toNotASpecKitProjectErrorbefore serialization.
from .._project import _resolve_init_dir_override
override = _resolve_init_dir_override()
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (5)
src/specify_cli/artifacts/init.py:293
- Core scripts are surfaced with their physical filename (for example,
setup-plan.sh) rather than the extension-free logical name accepted by preset/extension manifests.get_artifact_info()then passes that name toPresetResolver.collect_all_layers(..., "script"), which appends.shand does not search the runtime subdirectory, so script rows returned byartifact listcannot be retrieved byartifact info. Normalize scripts to logical names and make core script lookup use the runtime directories consistently.
name = entry.name
if name in seen:
continue
try:
text = entry.read_text(encoding="utf-8")
except (OSError, UnicodeDecodeError):
text = ""
seen[name] = _CoreBaselineRow(
name=name,
kind="script",
path=entry,
description=_extract_script_description(text),
src/specify_cli/artifacts/init.py:395
- Valid preset manifests store their display name at
preset.name(seepresets/lean/preset.yml:3-6), but this readsmetadata.nameor a top-levelname. Consequently real preset stack rows report the preset ID aspresetNameinstead of the human-friendly name.
metadata = data.get("metadata")
if isinstance(metadata, dict):
display = metadata.get("name")
if isinstance(display, str) and display:
return display
display = data.get("name")
if isinstance(display, str) and display:
return display
src/specify_cli/artifacts/init.py:449
PresetResolveremits project overrides with aproject:_...lookup ID, but this classifier only recognizes core and extension prefixes, so an override falls through and is falsely serialized as a preset with no ID/name. This also makes the emitted lookup ID violate this module's declared layer/lookup grammar. Represent project overrides explicitly (and update the JSON contract), or exclude them without mislabeling them.
# Layer classification: prefer lookupId prefix (authoritative) with a
# source-string fallback for defensive parsing.
if lookup_id.startswith("core:") or source.startswith("core"):
src/specify_cli/artifacts/init.py:674
- This directory/manifest scan does not use the resolver's eligibility rules. It includes every preset directory even though
PresetResolveronly composes presets registered in.registry, while omitting registered preset and extension artifacts that resolve through the supported convention fallback when no manifest entry exists. As a result,artifact listcan both advertise artifacts thatartifact inforejects and omit artifacts thatartifact info --kindresolves. Build the inventory from the same registry, enabled-state, and fallback semantics as the resolver.
for pack_dir in sorted(tier_dir.iterdir(), key=lambda p: p.name):
if not pack_dir.is_dir():
continue
manifest_name = "preset.yml" if tier == "presets" else "extension.yml"
manifest = pack_dir / manifest_name
if not manifest.is_file():
continue
src/specify_cli/artifacts/_commands.py:53
_resolve_init_dir_override()itself prints Rich errors and raisestyper.Exitfor an invalidSPECIFY_INIT_DIR. Because that exception bypasses theArtifactErrorhandlers, these JSON-only commands emit non-JSON stderr despite this function's stated strict-envelope purpose. Use a non-emitting project resolver or convert override validation failures into the artifact error envelope.
from .._project import _resolve_init_dir_override
override = _resolve_init_dir_override()
cwd = override if override is not None else Path.cwd()
if not (cwd / ".specify").is_dir():
raise NotASpecKitProjectError()
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:5350
- Core scripts are installed under
.specify/scripts/<runtime>/(shared_infra.py:510-535), but this probes.specify/templates/scripts. As a result,resolve()skips the project's installed (and potentially customized) runtime variant and falls through to the bundled copy; the new tests also write to the non-installed path, so they do not cover a real initialized project. Search.specify/scriptsusing the persisted script selection, while retaining the flat templates path only as a legacy fallback.
This issue also appears on line 5689 of the same file.
core = next(
(path for path in script_variant_paths(self.templates_dir / "scripts", template_name) if path.exists()),
None,
src/specify_cli/artifacts/init.py:608
- Descriptions are selected independently of resolver precedence: core rows are inserted first and a non-empty core description is never replaced, while artifacts without a core row use whichever pack directory sorts first rather than the highest-priority layer. Therefore
artifact list/infocan describe a hidden lower layer even when the stack marks another preset active. Choose the description from the resolver's effective highest-priority contribution (with an explicit fallback policy for empty descriptions).
for kind, name, description in self._iter_contribution_artifacts():
key = (kind, name)
if key not in seen:
seen[key] = Artifact(
id=f"{kind}:{name}",
name=name,
kind=kind,
description=description,
)
elif description and not seen[key].description:
seen[key] = Artifact(
id=seen[key].id,
name=seen[key].name,
kind=seen[key].kind,
description=description,
)
src/specify_cli/presets/init.py:5691
collect_all_layers()repeats the same non-installed script root: current projects contain core variants in.specify/scripts/<runtime>/, not.specify/templates/scripts/<runtime>/(shared_infra.py:510-535). This makes the composition stack ignore the project's selected/customized script and report the bundled fallback instead. Align this lookup with the correctedresolve()behavior and keep only the flat templates location as a legacy fallback.
c = next(
(path for path in script_variant_paths(self.templates_dir / "scripts", template_name) if path.exists()),
None,
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Applied the bounded Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous). |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/specify_cli/artifacts/init.py:606
- Description selection is independent of composition precedence: core rows are inserted first, and this condition only replaces an empty description. Consequently, an active preset such as
leanoverridingspeckit.constitutionstill reports the hidden core command's description; for non-core collisions, alphabetic directory order wins instead of resolver priority. Select the first non-empty description in resolver stack order solistandinfodescribe the effective artifact.
)
src/specify_cli/artifacts/init.py:731
- This raw
yaml.safe_loadbypassesExtensionManifest's supported command-name canonicalization. For example, a valid legacy entry namedspeckit.runis normalized tospeckit.<extension-id>.runbyExtensionManifest(seesrc/specify_cli/extensions/__init__.py:521-531), so the resolver exposes the canonical lookup ID, while this scan tests the unnormalized ID and omits the artifact—making bareartifact inforeport it as unknown. Build manifest rows from the validated manifest APIs/iter_contributions()so inventory names match resolver names.
data: Any = None
if manifest.is_file():
try:
data = yaml.safe_load(manifest.read_text(encoding="utf-8"))
except (OSError, UnicodeDecodeError, yaml.YAMLError):
tests/test_artifact_command.py:66
- This fixture does not create a valid preset manifest: it uses
metadatainstead ofpresetand omitsschema_versionandrequires; several callers also omit requiredtype/filefields.PresetManifesttherefore rejects it and the resolver treats the files as convention-only, so tests such astest_preset_command_uses_entry_typedo not exercise manifest type projection and would still pass if that behavior regressed. Use a schema-valid preset fixture for manifest-specific tests.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@copilot address these points in this PR (do not open a new PR): @copilot
|
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_artifact_command.py:66
- This helper does not create a valid preset manifest:
PresetManifestrequires top-levelschema_version,preset,requires, andprovidessections (src/specify_cli/presets/__init__.py:312-415), while this uses legacy-looking top-levelid/metadatafields. Consequently the resolver rejects every fixture manifest and these tests exercise convention fallback instead of valid manifest-declared files, priorities, strategies, and display names. Update the helper to emit the current schema and adjust its callers to include requiredtype/fileentries so the intended preset paths are actually covered.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
tests/test_artifact_command_parity.py:27
- This duplicated preset fixture also writes an invalid manifest: the current schema requires
schema_version, apresetmapping,requires.speckit_version, andprovides.templatesentries withtype,name, andfile(src/specify_cli/presets/__init__.py:312-425). The parity test therefore reaches the preset only through convention fallback, so it does not verify parity for a real manifest-declared override. Use the valid preset schema here and convert the callers'commandsdata to validprovides.templatesentries.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
src/specify_cli/artifacts/init.py:584
- The inventory baseline is loaded only from the CLI's bundled/source assets, so it never enumerates core files installed under this project's
.specify/templates. This makes the new resolver behavior inconsistent with this command: for example, theps-only-helper.ps1andpy_only_helper.pycases added intests/test_presets.py:12925-12969resolve successfully, butartifact listomits them and bareartifact info ps-only-helperreports an unknown artifact because_find_matches()depends on this list. Merge project-installed core commands/templates/script variants (including the legacy flat script layout) into the baseline using the same naming rules asPresetResolver.
baseline = self._get_baseline()
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Point 1 was already applied — Point 2 is now addressed in 43cf9bc: Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous). |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/artifacts/init.py:743
- Parsing raw YAML here bypasses
ExtensionManifest's accepted legacy-name canonicalization (extensions/__init__.py:489-532). An installed command declared asspeckit.hellois resolved asspeckit.<extension-id>.hello, but this inventory derives and gates the raw lookup ID; with a non-conventionalfile:path the command is omitted entirely, and with a conventional path the obsolete name is listed instead. Build manifest contributions from the validatedPresetManifest/ExtensionManifestviews (theiriter_contributions()output) while retaining convention fallback for invalid or undeclared entries.
if manifest.is_file():
try:
data = yaml.safe_load(manifest.read_text(encoding="utf-8"))
except (OSError, UnicodeDecodeError, yaml.YAMLError):
data = None
if isinstance(data, dict):
for kind, name, description in _iter_manifest_contributions(
data, is_preset=tier == "presets"
):
lookup_id = derive_named_id(layer, pack_dir.name, kind, name)
if lookup_id in _lookup_ids(kind, name):
yield kind, name, description
src/specify_cli/presets/init.py:5351
- Runtime-specific project scripts are installed under
.specify/scripts/<runtime>/(shared_infra.py:510-519anddocs/installation.md:131-135), but this searches.specify/templates/scripts/<runtime>/. As a result,resolve()ignores the project's selected/customized script and falls through to the currently installed CLI's bundled copy. Keep the legacy flat path, but search runtime variants from.specify/scripts.
core = next(
(path for path in script_variant_paths(self.templates_dir / "scripts", template_name) if path.exists()),
None,
)
src/specify_cli/presets/init.py:5692
- This repeats the runtime-directory mismatch from
resolve():collect_all_layers()searches.specify/templates/scripts/<runtime>/, while initialization installs variants under.specify/scripts/<runtime>/(shared_infra.py:510-519). The stack can therefore report a bundled script instead of the project-installed/customized winner. Preserve the legacy flat candidate and search runtime variants from.specify/scriptsso both resolver paths stay aligned.
c = next(
(path for path in script_variant_paths(self.templates_dir / "scripts", template_name) if path.exists()),
None,
)
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@copilot address this code fix, use this same PR: ❌ Thread #21 (outdated=false) — resolved but not fixed Location: Reviewer's concern: The baseline is loaded only from the wheel/source checkout, so resolver-visible core files that exist only in the project's Current state at HEAD:
Impact: if a project drops a bespoke core template (e.g. |
| yield kind, entry.stem | ||
|
|
||
|
|
||
| def _iter_manifest_contributions( |
There was a problem hiding this comment.
@copilot 1. _iter_manifest_contributions (lines 812-875) — reinvents Manifest.iter_contributions()
The artifact code re-parses provides dicts by hand, branching on preset vs extension shape:
Python
provides = data.get("provides")
if is_preset:
entries = provides.get("templates")
for entry in entries:
kind_value = entry.get("type") # ← re-doing what iter_contributions does
But both manifest classes already yield the same info with derived layer, sourceId, kind, and a pre-computed id:
PresetManifest.iter_contributions()—presets/__init__.py:547ExtensionManifest.iter_contributions()—extensions/__init__.py:776
| return artifact.description | ||
| return "" | ||
|
|
||
| def _iter_contribution_artifacts( |
There was a problem hiding this comment.
@copilot _iter_contribution_artifacts (lines 686-752) — reinvents registry enumeration
Walks .specify/presets/ and .specify/extensions/ with raw .iterdir(). This picks up any orphan directories from failed installs (which the registry would not) and then has to filter after-the-fact by lookup-ID membership. The right primitives already exist:
PresetManager.list_installed()—presets/__init__.py:4104ExtensionManager.list_installed()—extensions/__init__.py:3630- Per-pack manifest via
PresetManager.get_pack(pack_id)/ExtensionManager.get_extension(ext_id)
Going through the registry would also naturally filter disabled packs upfront, removing the need for the _lookup_ids(...) post-filter (which is currently the only defense against surfacing disabled/orphan entries).
| return None | ||
|
|
||
|
|
||
| def _preset_display_name(pack_dir: Path, pack_id: str) -> str: |
There was a problem hiding this comment.
@copilot _preset_display_name (lines 390-414) — reinvents PresetManifest.name
Hand-rolls YAML loading + defensive parsing of data["preset"]["name"] / data["name"]. PresetManifest.name is a validated property that returns this exact value. Same for PresetManifest.description if that ever gets surfaced. The fallback that thread #8's follow-up added (top-level name) is only needed because the artifact code bypassed the manifest class in the first place.
| description: str | ||
|
|
||
|
|
||
| def _core_asset_root(subdir: str) -> Path | None: |
There was a problem hiding this comment.
@copilot _enumerate_core_* + _core_asset_root (lines 135-335) — reinvents _find_bundled_core
Three enumerators re-derive where the core pack lives; _core_asset_root is now the third implementation of "find the core assets" alongside PresetResolver._find_bundled_core (5720) and _load_core_command_names. The docstring on line 141 even admits this: "Mirrors the two-tier resolution … so all three code paths agree on what 'core' means." That agreement is manual — a divergence would go undetected. Related: this codepath omits tier-4 (.specify/templates/), which is the underlying unfixed issue in thread #21.
| else: | ||
| hidden = idx > first_replace_idx | ||
|
|
||
| # Layer classification: prefer lookupId prefix (authoritative) with a |
There was a problem hiding this comment.
@copilot fix . _build_stack layer classification (lines 465-528)
Classifies each layer by string-prefix-matching lookupId ("core:", "project:", "extension:") rather than asking the resolver. This works because _identifier.py fixes the grammar, but it also means artifacts is coupled to the string layout rather than a stable API. A layer_kind_from_lookup_id(lookup_id) helper in _identifier.py would remove the fragility.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _derive_manifest_path(layer: dict[str, Any], project_root: Path) -> str | None: |
There was a problem hiding this comment.
@copilot fix ** _derive_manifest_path / _find_enclosing_manifest (lines 343-387)**
Walks parent dirs to find preset.yml/extension.yml. But for registered packs, the resolver already knows the location — pack_dir = project_root / ".specify" / <tier> / <sourceId>. The pack_id is already parsed at line 513 (_extract_lookup_pack_id), so the manifest path is derivable without a filesystem walk. The walk only matters for convention-only unregistered contributions, and for those the resolver itself returns no manifest.
|
@copilot In |
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Implemented project-local core command, template, and script enumeration in ca42671, with regression coverage for inventory and stack lookup. Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous). |
|
@copilot please address the six inline review comments I posted at 18:16 UTC in review #5011143037 — they cover reusing |
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
tests/test_artifact_command.py:67
- This fixture is not a valid
PresetManifest: the requiredschema_version,preset, andrequiressections are missing, andmetadatais not the preset metadata section. The resolver therefore catches validation and treats every fixture pack as manifestless, so tests such astest_preset_command_uses_entry_typeonly exercise convention fallback rather than the manifest projection they claim to cover. Build the canonical preset schema here and update command/script callers to useprovides.templatesentries with an explicittype.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
}
tests/test_artifact_command_parity.py:28
- This helper also writes an invalid preset schema (
PresetManifestrequiresschema_version,preset,requires, andprovides). As a result, the parity test's preset is resolved only through convention fallback, so it does not verify parity for a manifest-declared override. Use the canonical preset schema and put the command underprovides.templateswithtype: command.
manifest = {
"id": pack_id,
"version": "1.0.0",
"metadata": {"name": f"Test preset {pack_id}"},
"provides": provides,
}
src/specify_cli/artifacts/init.py:790
- Reading raw YAML here loses
ExtensionManifest's supported legacy command-name canonicalization. For example, an extension declaringspeckit.hellois normalized tospeckit.<extension-id>.helloby the resolver, but this iterator probes only the raw name; the canonical artifact is then omitted fromlist, and bareinforeports it unknown even though the resolver can resolve it. Project extension entries from the validatedExtensionManifest.iter_contributions()output instead.
if isinstance(data, dict):
for kind, name, description in _iter_manifest_contributions(
data, is_preset=tier == "presets"
):
lookup_id = derive_named_id(layer, pack_dir.name, kind, name)
if lookup_id in _lookup_ids(kind, name):
yield kind, name, description
src/specify_cli/artifacts/init.py:647
- Baseline rows are inserted without the colon check applied to manifest and convention contributions. On POSIX, a project-local file such as
.specify/templates/bad:name.mdtherefore emitstemplate:bad:name, violating this command's ID contract and creating OS-dependent inventory output. Skip baseline rows whose logical name contains the reserved delimiter.
for row in (*baseline.commands, *baseline.templates, *baseline.scripts):
key = (row.kind, row.name)
if key not in seen:
seen[key] = Artifact(
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| text = "" | ||
| rows.append( | ||
| _CoreBaselineRow( | ||
| name=f"speckit.{stem}", |
| if ":" in name: | ||
| prefix, _, bare = name.partition(":") | ||
| if prefix in ("command", "template", "script"): | ||
| resolved: ArtifactKind = prefix # type: ignore[assignment] | ||
| if kind is not None and kind != resolved: | ||
| raise ArtifactNotFoundError(name) | ||
| return bare, resolved | ||
| return name, kind |
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
|
@copilot in |
| fallback, and the artifact command's core-baseline enumeration) shares.""" | ||
|
|
||
| def test_prefers_wheel_core_pack_over_repo_checkout(self, tmp_path, monkeypatch): | ||
| import specify_cli._assets as assets |
| assert _locate_core_asset_dir("commands") == core_pack / "commands" | ||
|
|
||
| def test_falls_back_to_repo_checkout_when_no_wheel_bundle(self, tmp_path, monkeypatch): | ||
| import specify_cli._assets as assets |
| assert _locate_core_asset_dir("scripts") == repo_root / "scripts" | ||
|
|
||
| def test_returns_none_when_directory_missing(self, tmp_path, monkeypatch): | ||
| import specify_cli._assets as assets |
| assert _locate_core_asset_dir("commands") is None | ||
|
|
||
| def test_returns_none_for_unknown_subdir(self, tmp_path, monkeypatch): | ||
| import specify_cli._assets as assets |
|
@copilot fix all the import issues in the tests |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/specify_cli/artifacts/init.py:265
- Project-local command files already named
speckit.<name>.mdare emitted asspeckit.speckit.<name>. This makes the newtest_includes_project_local_core_assetsexpectation fail and causes bareartifact info speckit.local-commandto report the resolvable command as unknown. Preserve an existing namespace prefix.
name=f"speckit.{stem}",
src/specify_cli/artifacts/init.py:588
- A supplied
--kind(orkind:nameshorthand) bypasses inventory matching, so the bare name is passed directly to resolver path joins. Values such astemplate:../outsidecan therefore escape the intended artifact subdirectory and produce IDs outside the documented grammar. Validate the parsed name with the established command versus template/script name grammars before building the stack.
if ":" in name:
prefix, _, bare = name.partition(":")
if prefix in ("command", "template", "script"):
resolved: ArtifactKind = prefix # type: ignore[assignment]
if kind is not None and kind != resolved:
raise ArtifactNotFoundError(name)
return bare, resolved
return name, kind
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
| if manifest.is_file(): | ||
| try: | ||
| data = yaml.safe_load(manifest.read_text(encoding="utf-8")) | ||
| except (OSError, UnicodeDecodeError, yaml.YAMLError): | ||
| data = None | ||
| if isinstance(data, dict): | ||
| for kind, name, description in _iter_manifest_contributions( | ||
| data, is_preset=tier == "presets" | ||
| ): | ||
| lookup_id = derive_named_id(layer, pack_dir.name, kind, name) | ||
| if lookup_id in _lookup_ids(kind, name): | ||
| yield kind, name, description |
There was a problem hiding this comment.
@copilot Address this issue - here's why: Never re-parse a file format when a validated in-memory class already exists for it.
The class encodes rules that the file format doesn't. Bypassing the class means you're reading a different version of the data than the rest of the system uses, and any invariant that class enforces becomes something you have to re-enforce (or silently violate) in your code.
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Description
Testing
uv run specify --helpuv sync && uv run pytestAI Disclosure