Skip to content

feat: add specify artifact command exposing composition stacks as JSON - #4267

Draft
nicolehaugen wants to merge 25 commits into
nicolehaugen-contribution-idsfrom
nicolehaugen-shiny-garbanzo
Draft

feat: add specify artifact command exposing composition stacks as JSON#4267
nicolehaugen wants to merge 25 commits into
nicolehaugen-contribution-idsfrom
nicolehaugen-shiny-garbanzo

Conversation

@nicolehaugen

Copy link
Copy Markdown

Description

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

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
Comment thread tests/test_artifact_command_parity.py Fixed
Comment thread tests/test_artifact_command.py Fixed
Comment thread tests/test_artifact_command.py Fixed
…rt' and 'import from''

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings August 24, 2026 15:14
…rt' and 'import from''

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

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.

Pull request overview

Adds JSON-based artifact inventory and composition-stack introspection to the Specify CLI.

Changes:

  • Adds artifact list and artifact info commands.
  • 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 list can advertise artifacts that artifact info immediately 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

Comment thread src/specify_cli/artifacts/__init__.py Outdated
Comment thread src/specify_cli/artifacts/__init__.py
Comment thread src/specify_cli/artifacts/__init__.py Outdated
Comment thread src/specify_cli/artifacts/_commands.py Outdated
Comment thread tests/test_artifact_command.py Outdated
Comment thread src/specify_cli/artifacts/__init__.py Outdated
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

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 (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 single provides.templates[] list whose entries carry type and file. PresetManifest therefore 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/metadata at the root and sectioned provides entries). Since PresetManifest rejects it, the parity test reaches the preset file only through convention fallback and never verifies manifest/resolver parity. Generate the canonical schema_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, and setup_plan.py). More importantly, get_artifact_info() passes that filename to PresetResolver.collect_all_layers(), which appends .sh and looks outside the runtime subdirectory, so these advertised list entries all resolve as unknown 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 raises typer.Exit for an invalid SPECIFY_INIT_DIR, so this call bypasses the artifact JSON error handler. In --json mode stderr is then plain Rich text rather than the promised {"error": ...} envelope. Add a quiet/project-resolution API that raises an ArtifactError, 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

  • StackLayer is unused, and the repository's Python lint job runs Ruff over tests, 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/scripts shape: all preset contributions live in provides.templates[], with type identifying 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 a project: lookup ID, but this catch-all converts every non-core/non-extension layer into a preset. A real .specify/templates/overrides/<name>.md therefore appears as layer: "preset", presetId: "_", which is false metadata. Handle the project: 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

Comment thread src/specify_cli/artifacts/__init__.py Outdated
Copilot AI review requested due to automatic review settings August 24, 2026 15:22

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 (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 mixed provides.templates list with a type per entry. PresetManifest therefore 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 provides shape instead of a valid preset (preset/requires plus typed entries under provides.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, while artifact info returns unknown artifact for the same ID. Build the list from the same enabled registry/resolver sources used by collect_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's type (see presets/lean/preset.yml:15-18). Consequently valid preset contributions are omitted or classified as templates. Parse presets via PresetManifest.iter_contributions() and extensions via ExtensionManifest.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 .py rows (including underscore-vs-hyphen variants), whereas script contribution IDs use logical names such as setup-plan (tests/test_contribution_ids.py:124-127). These listed names cannot be resolved: collect_all_layers() appends .sh, so script:setup-plan.sh searches for setup-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 explicit project layer 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-level name (or metadata.name). As written, every normal preset stack reports the pack ID as presetName instead 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 raises typer.Exit when SPECIFY_INIT_DIR is invalid (src/specify_cli/_project.py:43-52). That bypasses the JSON error handler, so a --json invocation can emit non-JSON stderr despite this module's strict envelope contract. Add a non-emitting resolution path that converts these failures to NotASpecKitProjectError before 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>

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 (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 to PresetResolver.collect_all_layers(..., "script"), which appends .sh and does not search the runtime subdirectory, so script rows returned by artifact list cannot be retrieved by artifact 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 (see presets/lean/preset.yml:3-6), but this reads metadata.name or a top-level name. Consequently real preset stack rows report the preset ID as presetName instead 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

  • PresetResolver emits project overrides with a project:_... 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 PresetResolver only 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 list can both advertise artifacts that artifact info rejects and omit artifacts that artifact info --kind resolves. 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 raises typer.Exit for an invalid SPECIFY_INIT_DIR. Because that exception bypasses the ArtifactError handlers, 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

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

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 24, 2026 15:40
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous)

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>

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 (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/scripts using 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/info can 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 corrected resolve() 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

@nicolehaugen

Copy link
Copy Markdown
Author

@copilot

  • Create extensions folder for Gemini CLI #17's belt-and-braces suggestion I made earlier — bounding _find_enclosing_manifest at project_root — was not applied. The reviewer only asked for the None-return path, which is what the fix does, so the thread is fair to resolve. But the unbounded parent walk still runs and just gets discarded now.
  • chore: Fix typo #8's fix reads only data["preset"]["name"]. If a preset manifest ever puts the display name at top-level name (as some earlier drafts of this repo did), it will fall back to the pack id. That's the reviewer's asked-for behavior — worth knowing if you later see a pack id where you expected a friendly name.

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

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 24, 2026 17:10
Copilot AI and others added 2 commits August 24, 2026 17:10
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>

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot

Applied the bounded _find_enclosing_manifest walk at project_root and added regression coverage for outside-parent, project-root manifest, and directory-input cases in 27f1481, 45338b6, and ac09641. I left the preset display-name behavior unchanged because your note described it as the reviewer-requested behavior.

Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous).

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 (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 lean overriding speckit.constitution still 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 so list and info describe the effective artifact.
                )

src/specify_cli/artifacts/init.py:731

  • This raw yaml.safe_load bypasses ExtensionManifest's supported command-name canonicalization. For example, a valid legacy entry named speckit.run is normalized to speckit.<extension-id>.run by ExtensionManifest (see src/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 bare artifact info report 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 metadata instead of preset and omits schema_version and requires; several callers also omit required type/file fields. PresetManifest therefore rejects it and the resolver treats the files as convention-only, so tests such as test_preset_command_uses_entry_type do 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 AI review requested due to automatic review settings August 24, 2026 17:16
@nicolehaugen

Copy link
Copy Markdown
Author

@copilot address these points in this PR (do not open a new PR): @copilot

  • #17's belt-and-braces suggestion I made earlier — bounding _find_enclosing_manifest at project_root — was not applied. The reviewer only asked for the None-return path, which is what the fix does, so the thread is fair to resolve. But the unbounded parent walk still runs and just gets discarded now.
  • #8's fix reads only data["preset"]["name"]. If a preset manifest ever puts the display name at top-level name (as some earlier drafts of this repo did), it will fall back to the pack id. That's the reviewer's asked-for behavior — worth knowing if you later see a pack id where you expected a friendly name.

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 (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: PresetManifest requires top-level schema_version, preset, requires, and provides sections (src/specify_cli/presets/__init__.py:312-415), while this uses legacy-looking top-level id/metadata fields. 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 required type/file entries 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, a preset mapping, requires.speckit_version, and provides.templates entries with type, name, and file (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' commands data to valid provides.templates entries.
    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, the ps-only-helper.ps1 and py_only_helper.py cases added in tests/test_presets.py:12925-12969 resolve successfully, but artifact list omits them and bare artifact info ps-only-helper reports 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 as PresetResolver.
        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>

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot address these points in this PR (do not open a new PR): @copilot

  • #17's belt-and-braces suggestion I made earlier — bounding `_fi...

Point 1 was already applied — _find_enclosing_manifest bounds the parent walk at project_root and breaks out as soon as a parent escapes it (27f1481, 45338b6, ac09641), so no unbounded walking happens.

Point 2 is now addressed in 43cf9bc: _preset_display_name reads preset.name first and falls back to a top-level name key before falling back to the pack id, with regression tests for the nested, flat, and no-name manifest layouts.

Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous).

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 (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 as speckit.hello is resolved as speckit.<extension-id>.hello, but this inventory derives and gates the raw lookup ID; with a non-conventional file: path the command is omitted entirely, and with a conventional path the obsolete name is listed instead. Build manifest contributions from the validated PresetManifest/ExtensionManifest views (their iter_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-519 and docs/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/scripts so 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

@nicolehaugen

Copy link
Copy Markdown
Author

@copilot address this code fix, use this same PR: ❌ Thread #21 (outdated=false) — resolved but not fixed

Location: src/specify_cli/artifacts/__init__.py:667 (originally)

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 .specify/templates/ tree are absent from artifact list (and therefore from bare-name artifact info). For example, .specify/templates/legacy-template.md produces a core layer in collect_all_layers(), but no catalog row unless the current package also ships that name. Build the baseline from the union of the project-local core tree and the bundled fallback, matching resolver tiers 4/5.

Current state at HEAD:

  • _core_asset_root (lines 135-156) only checks the wheel-installed core_pack, then falls back to the source-checkout _repo_root() / "templates". It never looks at project_root / ".specify" / "templates" — exactly the tree the reviewer said should be unioned in.
  • _iter_contribution_artifacts walks .specify/presets/ and .specify/extensions/ only.
  • _iter_project_override_artifacts walks .specify/templates/overrides/ only.
  • Nothing enumerates .specify/templates/*.md (root) or .specify/templates/scripts/* for candidate names.

Impact: if a project drops a bespoke core template (e.g. .specify/templates/legacy-template.md) that isn't in the shipped baseline and isn't a preset/extension/override, PresetResolver.collect_all_layers() will resolve it as a core: layer, but specify artifact list --json won't include it, and specify artifact info legacy-template --json (bare name, no template: prefix) will return "unknown artifact". Only specify artifact info template:legacy-template --json would find it, via the on-demand stack build.

Comment thread src/specify_cli/artifacts/__init__.py Outdated
yield kind, entry.stem


def _iter_manifest_contributions(

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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:547
  • ExtensionManifest.iter_contributions()extensions/__init__.py:776

return artifact.description
return ""

def _iter_contribution_artifacts(

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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:4104
  • ExtensionManager.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:

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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:

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment thread src/specify_cli/artifacts/__init__.py Outdated
else:
hidden = idx > first_replace_idx

# Layer classification: prefer lookupId prefix (authoritative) with a

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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:

@nicolehaugen nicolehaugen Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@nicolehaugen

Copy link
Copy Markdown
Author

@copilot In src/specify_cli/artifacts/__init__.py, extend _enumerate_core_commands, _enumerate_core_templates, and _enumerate_core_scripts to also enumerate .specify/templates/ in the project root (tier 4 in PresetResolver.resolve()), unioned with the existing bundled/repo-checkout enumeration. This addresses the "baseline is loaded only from the wheel/source checkout" review comment on the PR.

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot In src/specify_cli/artifacts/__init__.py, extend _enumerate_core_commands, _enumerate_core_templates, and _enumerate_core_scripts to also enumerate .specify/templates/ in the projec...

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

@nicolehaugen

Copy link
Copy Markdown
Author

@copilot please address the six inline review comments I posted at 18:16 UTC in review #5011143037 — they cover reusing Manifest.iter_contributions(), registry enumeration via list_installed(), PresetManifest.name, _find_bundled_core, _build_stack layer classification via lookupId, and simplifying _derive_manifest_path for registered packs.

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 (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 required schema_version, preset, and requires sections are missing, and metadata is not the preset metadata section. The resolver therefore catches validation and treats every fixture pack as manifestless, so tests such as test_preset_command_uses_entry_type only exercise convention fallback rather than the manifest projection they claim to cover. Build the canonical preset schema here and update command/script callers to use provides.templates entries with an explicit type.
    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 (PresetManifest requires schema_version, preset, requires, and provides). 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 under provides.templates with type: 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 declaring speckit.hello is normalized to speckit.<extension-id>.hello by the resolver, but this iterator probes only the raw name; the canonical artifact is then omitted from list, and bare info reports it unknown even though the resolver can resolve it. Project extension entries from the validated ExtensionManifest.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.md therefore emits template: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}",

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot address this issue

Comment on lines +604 to +611
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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot address the issue

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
@nicolehaugen

Copy link
Copy Markdown
Author

@copilot in src/specify_cli/artifacts/__init__.py, do the actual code refactors (not docstring updates) for the six inline review comments I posted: (a) replace _iter_manifest_contributions with calls to PresetManifest.iter_contributions() / ExtensionManifest.iter_contributions(); (b) replace the .iterdir() scan in _iter_contribution_artifacts with PresetManager.list_installed() / ExtensionManager.list_installed(); (c) replace _enumerate_core_* bodies with calls that reuse PresetResolver._find_bundled_core for locating core assets; (d) drop the _derive_manifest_path parent-walk for registered packs — use self.presets_dir / pack_id / "preset.yml" / self.extensions_dir / ext_id / "extension.yml" directly; (e) rewrite _build_stack layer classification to key off a small helper in _identifier.py (e.g., layer_kind_from_lookup_id) instead of prefix matching; (f) also fix the two newer issues — the speckit. double-prefix bug in the core enumerators, and the missing name validation when --kind is supplied (route the parsed bare name through validate_component before _build_stack

Comment thread tests/test_assets.py
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
Comment thread tests/test_assets.py
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
Comment thread tests/test_assets.py
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
Comment thread tests/test_assets.py
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
Comment thread tests/test_assets.py Outdated
@nicolehaugen

Copy link
Copy Markdown
Author

@copilot fix all the import issues in the tests

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/artifacts/init.py:265

  • Project-local command files already named speckit.<name>.md are emitted as speckit.speckit.<name>. This makes the new test_includes_project_local_core_assets expectation fail and causes bare artifact info speckit.local-command to 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 (or kind:name shorthand) bypasses inventory matching, so the bare name is passed directly to resolver path joins. Values such as template:../outside can 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

Comment thread src/specify_cli/artifacts/__init__.py Outdated
Comment on lines +756 to +767
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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

3 participants