feat: add deterministic contribution IDs and specify artifact command - #4305
feat: add deterministic contribution IDs and specify artifact command#4305nicolehaugen wants to merge 93 commits into
specify artifact command#4305Conversation
…artifacts
Every command, template, script, and hook contribution returned by
preset and extension manifest surfaces now carries a computed opaque
identifier of the form {layer}:{sourceId}:{kind}:{name}, and every
resolved artifact-stack layer carries a matching lookupId derived from
the same recipe.
Identifiers are computed at read time from author-declared manifest
content only. No paths, timestamps, or file-content hashes contribute
to derivation, so identifiers are stable across machines, reinstalls,
and directory moves. Nothing is persisted to .specify/ or any cache.
Hooks that collide within a source on (eventName, command) get a
12-hex SHA-256 discriminator computed from the canonical JSON of the
entry's declared fields minus eventName/command. Two hook entries
with byte-identical remaining fields are rejected at manifest load
because there is no meaningful way to distinguish them.
The change is purely additive: all existing name-based resolution
behaviour is preserved, and no consumer keys off the new id or
lookupId fields.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: claude-opus-4.7, autonomous)
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>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@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>
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@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>
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>
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>
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
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>
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>
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
…g from parent dir 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>
Assisted-by: GitHub Copilot (model: Claude Sonnet 4.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Addressed in Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.4, autonomous). |
|
@copilot Two small changes in this PR's helper cluster around `_iter_core_candidates`. Keep it tight — do not modify pre-existing Spec Kit code.
File: Rename the classmethod and rewrite the docstring: Python Update the single caller in
The functions Above Python No comment needed on the other two — one anchor at
|
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:106
- The documented project sentinel is not enforced:
derive_named_id("project", "foo", ...)emitsproject:foo:..., andlayer_kind_from_lookup_id()accepts it even though project IDs requiresourceId == "_". Enforce the sentinel in both construction and full-shape parsing so malformed project IDs cannot be treated as authoritative.
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
validate_component(name, "name")
src/specify_cli/_identifier.py:47
- This parenthetical contradicts the intentional built-in contract stated above: built-in layers omit
lookupId, so not every layer carries one. Narrow the statement to non-built-in layers.
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
src/specify_cli/presets/init.py:5743
- The PR description still says
collect_all_layers()uses the on-disk extension directory assourceId, but this branch deliberately replaces it with the validated manifest ID for declared contributions. Update the PR description to match the implemented round-trip contract; the current text tells consumers the opposite identity to expect.
ext_manifest = ExtensionManifest(ext_manifest_path)
if isinstance(ext_manifest.id, str) and ext_manifest.id:
source_id_for_lookup = ext_manifest.id
src/specify_cli/_identifier.py:55
- The PR description still promises contribution IDs for every core contribution and a
lookupIdon each resolver layer, while this allow-list intentionally excludes core and the resolver omits built-in lookup IDs. Update the PR body to describe the source-agnostic built-in contract already implemented and documented here.
_LAYER_KINDS = frozenset({PROJECT_OVERRIDE_LAYER, "preset", "extension"})
_CONTRIBUTION_KINDS = frozenset({"command", "template", "script", "hook"})
_NAMED_CONTRIBUTION_KINDS = _CONTRIBUTION_KINDS - {"hook"}
_HOOK_LAYERS = frozenset({"preset", "extension"})
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: claude-opus-4.7, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Addressed in |
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.
src/specify_cli/_identifier.py:49
- “Every layer carries a lookupId” contradicts the intentional built-in contract in this module and the artifact docs: built-in layers omit
lookupId. Scope the statement to project overrides so the identifier documentation remains internally consistent.
This issue also appears in the following locations of the same file:
- line 99
- line 145
Project overrides are a resolver feature — they are not backed by any manifest
contribution. When a resolved artifact stack contains a project-override layer,
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
ever emit a matching ``id``, so consumers see "not found" for the lookup, which
is the correct outcome for a layer with no originating manifest entry.
src/specify_cli/artifacts/init.py:488
- An unreadable extension registry can escape this public catalog boundary as raw
OSError, becauseExtensionRegistry(...)reads it beforeis_corrupt()is reached. Wrap construction just as the preset validator must, so all registry read failures consistently becomeArtifactResolutionError.
This issue also appears on line 505 of the same file.
from ..extensions import ExtensionRegistry
if ExtensionRegistry(extensions_dir).is_corrupt():
raise ArtifactResolutionError()
src/specify_cli/_identifier.py:147
- The parser's “complete shape” validation still accepts impossible source/layer pairs such as
project:foo:template:xandpreset:_:template:x. Mirror the constructor's source-ID invariant here so malformed external lookup IDs returnNone.
layer = parts[0]
if layer not in _LAYER_KINDS:
return None
src/specify_cli/_identifier.py:106
- The constructor does not enforce the documented source-ID rule: it accepts
project:anything:...and also_for preset/extension layers. Those IDs do not represent valid sources, yetlayer_kind_from_lookup_id()can treat them as authoritative. Enforce_iff the layer isproject.
validate_component(layer, "layer")
validate_component(source_id, "sourceId")
validate_component(kind, "kind")
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
validate_component(name, "name")
src/specify_cli/artifacts/init.py:508
- This validator can leak
OSErrorinstead of the catalog'sArtifactResolutionError:PresetRegistry(...)calls_load()beforeis_corrupt(), and a directory at.registrydeterministically raisesIsADirectoryErrorthere. Wrap construction so directArtifactCatalogcallers receive the same logical error as malformed registries and the CLI envelope.
from ..presets import PresetRegistry
if PresetRegistry(presets_dir).is_corrupt():
raise ArtifactResolutionError()
docs/reference/artifacts.md:131
- This cause is stale: artifact content is no longer composed by
listorinfo, so composition failure does not produce this error. Document the implemented cases—corrupt registries, filesystem failures, and resolver layer-collection errors.
| `artifact resolution failed` | The preset/extension registries could not be read, or artifact content could not be composed |
src/specify_cli/presets/init.py:5743
- The current PR description says extension
sourceIduses the on-disk directory name, but this branch intentionally usesExtensionManifest.idfor declared entries and only falls back to the directory for convention-only entries. Update the PR description because it currently publishes the opposite lookup-ID contract.
ext_manifest = ExtensionManifest(ext_manifest_path)
if isinstance(ext_manifest.id, str) and ext_manifest.id:
source_id_for_lookup = ext_manifest.id
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: claude-opus-4.7, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:105
- The constructor does not enforce the documented project-layer source invariant:
derive_named_id("project", "anything", "command", "x")returnsproject:anything:command:x, andlayer_kind_from_lookup_id()accepts it even though project IDs requiresourceId == "_". Enforce this relation in both construction and parsing so every accepted ID conforms to the declared grammar.
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
tests/test_contribution_ids.py:6
- This module summary says every resolved layer has a
lookupId, but the new built-in-layer tests explicitly require core rows to omit it. Describe only project/preset/extension provenance layers as carrying lookup IDs so the test documentation matches the contract it asserts.
Every command / template / script / hook contribution surfaced by a preset or
extension manifest exposes a computed ``id`` derived from author-declared data
only, and every layer of a resolved artifact stack exposes a matching
``lookupId``. The scenarios below cover: the identifier grammar across every
docs/reference/artifacts.md:131
- This cause is not implemented: artifact inspection never composes content, and layer-backed artifacts remain visible even when content cannot be decoded or composed.
ArtifactResolutionErroris used for corrupt registries, filesystem failures, or resolver errors while collecting layers, so document those actual causes instead.
| `artifact resolution failed` | The preset/extension registries could not be read, or artifact content could not be composed |
src/specify_cli/presets/init.py:5743
- The current PR description says extension
sourceIduses the on-disk directory name, but this code deliberately replaces it withExtensionManifest.id; the renamed-directory tests and reference docs assert that manifest identity is the contract. Update the PR description so it does not publish the opposite lookup-ID behavior.
ext_manifest = ExtensionManifest(ext_manifest_path)
if isinstance(ext_manifest.id, str) and ext_manifest.id:
source_id_for_lookup = ext_manifest.id
src/specify_cli/_identifier.py:47
- The parenthetical contradicts this PR's intentional built-in contract: built-in stack layers omit
lookupId, so it is not true that every layer carries one. Limit the invariant to provenance-backed/project-override layers.
contribution. When a resolved artifact stack contains a project-override layer,
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@copilot Change `specify artifact list --json` to always include the `stack` array per artifact — matching the shape `bundle list --json` uses (full record per row, no summary/detail split, no opt-in flag). Why**Precedent — `bundle list --json` returns full records.** See `bundler/models/records.py:44-52` (`InstalledBundleRecord.to_dict()`) — one call, full record shape including `contributed_components`. There is no `bundle info` fallback needed for consumers that want provenance. **Real usage — wizard boot regression.** The Spec Kit Wizard canvas boot currently calls `list --json` then loops `info --json` per artifact — 68 shell-outs in a project with a few presets + an extension. Each shell-out pays Python cold-start (~400–900ms on Windows). Total boot cost: 27–60s. This regression is a direct consequence of `list`'s summary shape, and it disappears the moment `stack` is inlined. **No back-compat risk.** The `artifact` command is being introduced by this same PR. There are no pre-existing external callers to break. `list --json`'s response shape is being expanded, not renamed or narrowed. Change**File:** `src/specify_cli/artifacts/_commands.py` `artifact_list` (currently around lines 95-118): when `--json` is passed, each row must include `stack` — the same array shape `get_artifact_info` returns today, unchanged. Result per row: ```json That's exactly what Implementation — reuse, don't re-doDo not re-resolve layers per artifact after listing. Share the layer-serialization function between Keep
|
Assisted-by: GitHub Copilot (model: GPT-5 Codex, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Implemented in
Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5 Codex). |
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:105
- The documented grammar requires
sourceId == "_"for project layers, but this constructor only checks that it is non-empty and colon-free. As a result,derive_named_id("project", "other", "template", "x")succeeds andlayer_kind_from_lookup_id()also accepts the invalid ID. Enforce the project/source relationship in both construction and parsing so the full-shape validation contract remains symmetric.
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
docs/reference/artifacts.md:131
- This cause is not implemented: the artifact catalog no longer composes content when serving
listorinfo, so composition failure does not produce this error. Document the actual paths mapped toArtifactResolutionError: corrupt registries, filesystem access failures, or resolver errors while collecting layers.
| `artifact resolution failed` | The preset/extension registries could not be read, or artifact content could not be composed |
src/specify_cli/_identifier.py:49
- The “every layer carries a
lookupId” invariant is false under this module's intentional built-in contract: lines 25-26 and both resolver core branches omitlookupIdfor built-in layers. Reword this to describe the synthetic project-override identity without claiming it applies to every stack layer.
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
ever emit a matching ``id``, so consumers see "not found" for the lookup, which
is the correct outcome for a layer with no originating manifest entry.
src/specify_cli/presets/init.py:5729
- The current PR description says the on-disk extension directory is used as
sourceIdfor manifest-declared entries, but this implementation deliberately uses the validated manifest ID and reserves directory identity for convention-only contributions. Update the PR description so it does not publish the opposite opaque-ID contract.
# Manifest-declared entries use the manifest's validated ``id:``
# for the lookupId's sourceId, so ``lookupId`` joins directly to
# ``ExtensionManifest.iter_contributions()``'s ``id`` even when
# the installed directory (``ext_id``) was renamed. Convention-
# only contributions have no manifest to consult and fall back
# to the directory identity. The directory identity is retained
- Files reviewed: 22/22 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>
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:105
- The documented grammar requires project IDs to use the
_source sentinel, but this constructor accepts values such asproject:other:template:x, andlayer_kind_from_lookup_id()also classifies them as valid. Enforcesource_id == "_"for the project layer in both construction and parsing so the shared API cannot create or accept IDs outside its own grammar.
validate_component(layer, "layer")
validate_component(source_id, "sourceId")
validate_component(kind, "kind")
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
src/specify_cli/artifacts/init.py:303
- This returns a
manifestPathwhenever the pack happens to contain a manifest, even for a convention-only artifact that the manifest does not declare. That makes the JSON contradict its documented “declaring manifest” meaning and points consumers to a file with no matching contribution. Preserve whether the resolver match was manifest-declared (or verify the lookup ID againstiter_contributions) and returnnullfor undeclared convention rows.
if isinstance(pack_dir, Path):
manifest_path = pack_dir / manifest_name
elif pack_id:
manifest_path = project_root / ".specify" / tier_dir / pack_id / manifest_name
else:
return None
if not manifest_path.is_file():
src/specify_cli/presets/init.py:5743
- The current PR description still says extension
sourceIdcomes from the on-disk directory, but this branch deliberately emits the validated manifest ID for declared entries (and the renamed-extension tests assert that contract). Please update the description to reserve directory identity for convention-only contributions; otherwise it publishes the opposite lookup-ID behavior.
ext_manifest = ExtensionManifest(ext_manifest_path)
if isinstance(ext_manifest.id, str) and ext_manifest.id:
source_id_for_lookup = ext_manifest.id
src/specify_cli/artifacts/init.py:675
artifact listcallscollect_all_layers()once per candidate, while each call walks every extension and reparses eachextension.yml(and reparses the matching manifest again for its source ID). With A artifacts and E extensions this produces O(A×E) YAML parses. Cache extension manifests on the resolver, as presets already are, or build the stack index in one pass before listing.
if key not in layers_cache:
try:
layers_cache[key] = resolver.collect_all_layers(name, kind)
except (OSError, PresetError) as exc:
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@copilot Please extend this PR to also emit Field:
Scope:
Out of scope: full structured provenance (source URL, version, install time — that's the broader #4208 follow-up). This PR should ship only the |
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>
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>
Implemented across Posted on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6, autonomous). |
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:103
- The documented grammar requires
sourceId == "_"for theprojectlayer, butderive_named_id("project", "anything", ...)currently succeeds, and the full-shape parser accepts the same impossible ID. Enforce this layer/source relationship in both construction and parsing so malformed project provenance cannot be treated as authoritative.
validate_component(layer, "layer")
validate_component(source_id, "sourceId")
validate_component(kind, "kind")
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
src/specify_cli/presets/init.py:5743
- The PR body still states that
collect_all_layers()uses the on-disk extension directory assourceIdand that every layer gets alookupId. This code instead uses the manifest ID for declared extensions, while built-in layers intentionally omitlookupId; the tests and reference docs align with the code. Update those PR-body bullets so consumers are not given the opposite contract.
ext_manifest = ExtensionManifest(ext_manifest_path)
if isinstance(ext_manifest.id, str) and ext_manifest.id:
source_id_for_lookup = ext_manifest.id
docs/reference/artifacts.md:165
- This cause is no longer implemented: the artifact catalog does not compose artifact content, and
get_artifact_info()intentionally relies on layer presence.ArtifactResolutionErrorcurrently represents corrupt registries, filesystem access failures, or resolver errors while collecting layers, so documenting composition failure misleads callers.
| `artifact resolution failed` | The preset/extension registries could not be read, or artifact content could not be composed |
src/specify_cli/artifacts/init.py:264
- This extracts
sourceIdby splitting an identifier that the public guidance explicitly says to treat as opaque (extensions/EXTENSION-API-REFERENCE.md:921-923). It also makesartifactsa second owner of identifier grammar despite the PR's stated_identifier.pyboundary. Expose a shared validated parser in_identifier.py, or carrysourceIdexplicitly on resolver layers, instead of decoding it here.
layer_kind = layer_kind_from_lookup_id(lookup_id)
if layer_kind not in ("project", "preset", "extension"):
raise ArtifactResolutionError()
return layer_kind, lookup_id.split(":", 2)[1], lookup_id
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/_identifier.py:49
- This docstring still claims every resolved layer carries a
lookupId, but built-in layers intentionally omit it throughout this PR. Update the invariant so this constant's documentation matches the source-agnostic built-in contract.
Project overrides are a resolver feature — they are not backed by any manifest
contribution. When a resolved artifact stack contains a project-override layer,
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
ever emit a matching ``id``, so consumers see "not found" for the lookup, which
is the correct outcome for a layer with no originating manifest entry.
CHANGELOG.md:9
- The new Unreleased section records only
sourcePath, omitting the two primary user-facing additions in this PR: deterministic contribution IDs and the newspecify artifactcommand. This would make the release notes materially incomplete.
- feat(artifacts): include `sourcePath` on artifact stack layers for installed preset and extension contributions.
src/specify_cli/presets/init.py:5728
- The PR description currently says extension
sourceIdcomes from the on-disk directory name, but this implementation deliberately usesExtensionManifest.idfor manifest-declared layers and reserves the directory identity for convention-only contributions. The code matches the lookup round-trip contract and tests, so update the PR description to state the same behavior.
# Manifest-declared entries use the manifest's validated ``id:``
# for the lookupId's sourceId, so ``lookupId`` joins directly to
# ``ExtensionManifest.iter_contributions()``'s ``id`` even when
# the installed directory (``ext_id``) was renamed. Convention-
# only contributions have no manifest to consult and fall back
src/specify_cli/artifacts/init.py:264
- This still parses
sourceIdby splitting the opaque lookup ID, contrary to the API guidance inextensions/EXTENSION-API-REFERENCE.md:923and the PR's goal that_identifier.pyexclusively owns the grammar. A future grammar change can pass the shared classifier yet break this extraction; expose a shared structured parser or carrysourceIdexplicitly on resolver layers instead.
layer_kind = layer_kind_from_lookup_id(lookup_id)
if layer_kind not in ("project", "preset", "extension"):
raise ArtifactResolutionError()
return layer_kind, lookup_id.split(":", 2)[1], lookup_id
- Files reviewed: 24/24 changed files
- Comments generated: 1
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: claude-sonnet-4.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/_identifier.py:105
- The constructor does not enforce the documented project-source sentinel, so
derive_named_id("project", "other", ...)produces an ID outside the module's own grammar. Enforcesource_id == "_"for project IDs (and reject_for preset/extension IDs) at this shared derivation boundary.
if layer not in _LAYER_KINDS:
raise IdentifierComponentError(f"Invalid layer '{layer}'")
if kind not in _NAMED_CONTRIBUTION_KINDS:
raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
src/specify_cli/_identifier.py:49
- This comment contradicts the built-in-tier contract established above and in the artifact docs: core stack rows intentionally omit
lookupId. Reword this as a project-override-specific synthetic ID rather than claiming every layer has one.
its ``lookupId`` uses this label so the round-trip invariant (every layer
carries a ``lookupId``) still holds. No manifest ``iter_contributions()`` will
src/specify_cli/_identifier.py:148
- The parser's “complete shape” validation accepts
project:not-underscore:template:x(andpreset:_:template:x), even though_is reserved as the project source sentinel. Reject layer/source combinations that no constructor should produce, otherwise malformed IDs are classified as authoritative layers.
layer = parts[0]
if layer not in _LAYER_KINDS:
return None
if parts[2] not in _CONTRIBUTION_KINDS:
- Files reviewed: 24/24 changed files
- Comments generated: 1
- Review effort level: Balanced
| except ( | ||
| ExtValidationError, | ||
| yaml.YAMLError, | ||
| OSError, | ||
| TypeError, | ||
| AttributeError, | ||
| ): | ||
| # Fall back to the directory identity when the | ||
| # manifest can't be re-read — same recovery as | ||
| # ``_extension_manifest_declared_template``. |
Consolidates the work from #4261 and #4267 into a single PR against
main.Why consolidated
#4261 (contribution IDs) and #4267 (
specify artifactcommand) were originally opened as a stack, but the underlying identifier grammar and resolver changes evolved together through review — splitting them cleanly would leave both PRs with incomplete stories. This PR ships the full, cohesive story in one place.What's in it
Contribution IDs (from #4261):
{layer}:{source_id}:{kind}:{name}identifier grammar for every core / preset / extension / project-override contribution.PresetResolver.collect_all_layers()returns each layer with a stablelookupIdso the same ID round-trips through the resolver._identifier.pymodule owns the grammar; consumers never re-derive it via string prefixes.Artifact command (from #4267):
specify artifactcommand that surfaces composition stacks as JSON, keyed by the same lookup IDs above._script_variants.pymodule: single place that knows thebash/,powershell/,python/runtime subdir layout for core scripts._locate_core_asset_dirin_assets.py— one source of truth for locating coretemplates/,commands/, andscripts/across wheel-installed and source-checkout layouts.Fixes surfaced during review:
collect_all_layers()uses the on-disk directory name as the extensionsourceId, matching howiter_extensions_by_priority()and the artifact catalog enumerate extensions — closes a gap where a manifest with a declarediddifferent from its directory produced alookupIdthe resolver would not itself hand back.provides.templates, so an installed override likespeckit.constitutionno longer becomes an ambiguoustemplate:row.project:layer prefix instead of being labeled as a preset with sourceId_.Fixes
Fixes #4210
Fixes #4212
Supersedes
Supersedes #4261 (closed)
Supersedes #4267 (closed)
Testing
1132 tests passing on Windows (11 skipped, 127 deselected are pre-existing Windows symlink-privilege failures unrelated to these changes, confirmed by running the same tests against unmodified
origin/main).Posted on behalf of @nicolehaugen by GitHub Copilot (model: claude-opus-4.7, supervised).