From 45800c222d6a5229235fa7ba0c198612c703041a Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Fri, 21 Aug 2026 13:58:32 -0500 Subject: [PATCH 01/13] Add deterministic contribution IDs and stack lookup IDs for resolved 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) --- docs/reference/presets.md | 19 + extensions/EXTENSION-API-REFERENCE.md | 56 ++- src/specify_cli/_identifier.py | 178 ++++++++ src/specify_cli/extensions/__init__.py | 154 +++++++ src/specify_cli/presets/__init__.py | 51 +++ tests/test_contribution_ids.py | 552 +++++++++++++++++++++++++ 6 files changed, 1009 insertions(+), 1 deletion(-) create mode 100644 src/specify_cli/_identifier.py create mode 100644 tests/test_contribution_ids.py diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 1098abfb42..6f4a428908 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -205,6 +205,25 @@ specify preset add team-workflow --priority 10 For any file that both provide, `compliance` wins (priority 5 < 10). For files only one provides, that one is used. For files neither provides, the core default is used. +## Contribution Identifiers + +Every command, template, and script contributed by a preset (or an extension, or the core layer) is addressable at read time by a deterministic opaque identifier of the form: + +```text +{layer}:{sourceId}:{kind}:{name} +``` + +- `layer` is one of `core`, `preset`, or `extension`. +- `sourceId` is `_` for `core`, the preset pack id for `preset`, or the extension id for `extension`. +- `kind` is one of `command`, `template`, or `script`. +- `name` is the entry's declared `name` field. + +Identifiers are computed on demand from author-declared manifest content and are never persisted to `.specify/` or any cache. Copying a preset to another machine (or touching its files) does not change the identifiers it produces. + +`PresetResolver.collect_all_layers()` returns layer dicts that each include a `lookupId` field pointing back to the originating contribution's `id`. Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they carry a synthetic `project:_:{kind}:{name}` `lookupId` that intentionally does not match any manifest contribution. + +For the full grammar, including the hook name-component convention and the discriminator recipe used by extensions, see the [Extension API Reference — Contribution Identifiers](../../extensions/EXTENSION-API-REFERENCE.md#contribution-identifiers) section. + ## FAQ ### Can I use multiple presets at the same time? diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index a7bece0b89..475c3c8212 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -10,6 +10,7 @@ Technical reference for Spec Kit extension system APIs and manifest schema. 4. [Configuration Schema](#configuration-schema) 5. [Hook System](#hook-system) 6. [CLI Commands](#cli-commands) +7. [Contribution Identifiers](#contribution-identifiers) --- @@ -859,7 +860,60 @@ satisfied = version_satisfies("1.2.3", ">=1.0.0,<2.0.0") # bool --- -## File System Layout +## Contribution Identifiers + +Every command, template, script, and hook contributed by an extension (or a preset, or the core layer) is addressable at read time by a deterministic opaque identifier. Resolved artifact-stack layers carry a matching `lookupId` field that points back to the contribution the layer came from. Identifiers are **computed on demand from author-declared manifest content** and are **never persisted** to `.specify/` or to any cache file. + +### Grammar + +Named contributions (commands, templates, scripts) follow: + +```text +{layer}:{sourceId}:{kind}:{name} +``` + +- `layer` is one of `core`, `preset`, or `extension`. +- `sourceId` is `_` for `core`, the preset pack id for `preset`, or the extension id for `extension`. +- `kind` is one of `command`, `template`, `script`, or `hook`. +- `name` is the contribution's declared `name` field. + +Hook contributions use a compound name-component built from the event and command: + +```text +{layer}:{sourceId}:hook:{eventName}:{command} +``` + +When two or more hook entries within the same source share the same `(eventName, command)` pair, a 12-hex-character discriminator is appended: + +```text +{layer}:{sourceId}:hook:{eventName}:{command}:{discriminator} +``` + +The discriminator is the first 12 lowercase hex characters of `sha256(canonical_json(entry - {eventName, command}))`. Two hook entries with byte-identical declared fields (after removing `eventName` and `command`) are rejected at manifest load with a `ValidationError` naming both positions — there is no meaningful way to distinguish them at read time. + +### Reserved character + +`:` is reserved as the identifier component separator. It cannot appear inside any of `layer`, `sourceId`, `kind`, `name`, `eventName`, or `command`. Extension ids, command names, template names, and script names are already constrained by their existing regex patterns (`^[a-z0-9-]+$` and friends), which forbid `:`. Hook event names (mapping keys) and hook `command` values are additionally validated to reject `:` at manifest load. + +### The `project:` sentinel + +Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they have no backing manifest and cannot appear in `iter_contributions()`. Layers of that kind carry a synthetic `lookupId` of the form `project:_:{kind}:{name}` so consumers that reverse-lookup the id always see "not found", which is the intended behaviour: overrides are addressable at the stack level, not as first-class contributions. + +### Python API + +`ExtensionManifest.iter_contributions()` yields dicts of the form `{layer, sourceId, kind, name, id, ...author-declared fields}`; each entry's `id` is the computed identifier. `ExtensionManifest.contribution_id(kind, name)` returns the id for a single lookup, or `None` if no contribution matches. `PresetManifest` exposes the same two methods. + +`PresetResolver.collect_all_layers()` returns layer dicts that include a `lookupId` field for every layer type (`project override`, preset, extension, core, and bundled core). + +### Determinism guarantees + +Identifier derivation reads only the in-memory declared manifest content. No filesystem paths, no `os.environ`, no timestamps, and no file-content hashes contribute to any id. Copying an extension or preset to a different machine (or renaming its directory, or touching its files) does not change the identifiers it produces. + +### Opacity guidance + +Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Parse them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than by string-splitting on `:` — the discriminator suffix and future grammar extensions may otherwise catch you out. + + ```text .specify/ diff --git a/src/specify_cli/_identifier.py b/src/specify_cli/_identifier.py new file mode 100644 index 0000000000..4124157df5 --- /dev/null +++ b/src/specify_cli/_identifier.py @@ -0,0 +1,178 @@ +"""Deterministic identifiers for Spec Kit contributions and resolved stack layers. + +Every command, template, script, and hook contribution surfaced by a preset or +extension manifest carries a computed opaque ``id`` string, and every layer of a +resolved artifact stack carries a matching ``lookupId``. The identifier value is +derived only from author-declared manifest data — it never depends on file +contents, timestamps, archive hashes, installation directory paths, install-time +random values, or list positions. That is what makes identifiers portable +across machines, project locations, and reinstalls, and what lets consumers use +them as stable join keys. + +Grammar for named contributions (commands, templates, scripts):: + + id = "{layer}:{sourceId}:{kind}:{name}" + + layer ∈ {"core", "preset", "extension"} + sourceId = "_" when layer == "core"; the preset id or extension id otherwise + kind ∈ {"command", "template", "script", "hook"} + name = the contribution's declared ``name`` + +Hook identifiers use ``{eventName}:{command}`` as the name component:: + + id = "{layer}:{sourceId}:hook:{eventName}:{command}[:{discriminator}]" + +The 12-lowercase-hex discriminator is appended only when at least one sibling +hook in the same source shares the same ``(eventName, command)`` pair, and it is +computed by SHA-256 of a canonical JSON serialization of the hook entry's +declared fields (with ``eventName`` and ``command`` removed, since they already +appear in the identifier prefix). Two hook entries in the same source whose +declared fields produce byte-identical canonical JSON are rejected at manifest +load time — they are semantically identical listeners. + +The functions in this module are pure — inputs are strings or in-memory +mappings parsed from a manifest, outputs are strings. None of them read from +disk, look at ``os.environ``, call ``datetime``, or hash file contents. That +guarantee is what preserves portability, and it is enforced by inspection +rather than by runtime checks: any change here that adds an ambient input is a +change that breaks the identifier contract. +""" + +from __future__ import annotations + +import hashlib +import json +from typing import Any, Iterable, Mapping + + +PROJECT_OVERRIDE_LAYER = "project" +"""Resolver-only layer label for project-local override layers. + +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. +""" + +_DISCRIMINATOR_LENGTH = 12 + + +class IdentifierComponentError(ValueError): + """Raised when a manifest component would break identifier grammar.""" + + +def validate_component(value: Any, field_label: str) -> str: + """Return ``value`` unchanged if it is a non-empty ``:``-free string. + + Manifest components that appear in an identifier (``layer``, ``sourceId``, + ``kind``, ``name``, ``eventName``, ``command``) may not contain the ``:`` + delimiter — the grammar has no escape rule. This function is the guard used + by manifest validators to reject offending values at load time with a clear + message naming the field. + """ + if not isinstance(value, str): + raise IdentifierComponentError( + f"Invalid {field_label}: expected a string, got {type(value).__name__}" + ) + if not value: + raise IdentifierComponentError( + f"Invalid {field_label}: value must not be empty" + ) + if ":" in value: + raise IdentifierComponentError( + f"Invalid {field_label} '{value}': ':' is reserved as an identifier delimiter" + ) + return value + + +def derive_named_id(layer: str, source_id: str, kind: str, name: str) -> str: + """Build the identifier string for a named contribution kind. + + Callers are expected to have already validated each component with + :func:`validate_component` at manifest-load time; this function does not + revalidate — it is a pure string join so the identifier can be computed + cheaply on every read. + """ + return f"{layer}:{source_id}:{kind}:{name}" + + +def canonical_json(value: Any) -> bytes: + """Serialize ``value`` to a canonical UTF-8 JSON byte string. + + Mapping keys are sorted lexicographically at every depth, list order is + preserved (author intent), whitespace is stripped, and non-ASCII characters + are emitted verbatim. This is the byte string that the hook discriminator + hashes and that the manifest loader uses to detect byte-identical duplicate + hook entries. + """ + normalized = _normalize_for_canonical_json(value) + return json.dumps( + normalized, + sort_keys=True, + ensure_ascii=False, + separators=(",", ":"), + ).encode("utf-8") + + +def _normalize_for_canonical_json(value: Any) -> Any: + if isinstance(value, Mapping): + return {str(k): _normalize_for_canonical_json(v) for k, v in value.items()} + if isinstance(value, (list, tuple)): + return [_normalize_for_canonical_json(v) for v in value] + return value + + +def _has_hook_sibling_collision( + event_name: str, + command: str, + siblings: Iterable[Mapping[str, Any]], +) -> bool: + """Return True when at least one sibling shares the same event/command pair. + + ``siblings`` is the full same-source hook entry list including the entry + whose identifier is being derived. A collision therefore means at least two + entries share the pair. + """ + seen = 0 + for entry in siblings: + if entry.get("eventName") == event_name and entry.get("command") == command: + seen += 1 + if seen >= 2: + return True + return False + + +def hook_discriminator(declared_fields: Mapping[str, Any]) -> str: + """Compute the 12-hex-char SHA-256 discriminator for a hook entry. + + ``declared_fields`` is the entry as parsed from the manifest with + ``eventName`` and ``command`` removed — those two values already appear in + the identifier prefix, so hashing them would only reflect information the + consumer can already read. + """ + return hashlib.sha256(canonical_json(declared_fields)).hexdigest()[:_DISCRIMINATOR_LENGTH] + + +def derive_hook_id( + layer: str, + source_id: str, + event_name: str, + command: str, + siblings: Iterable[Mapping[str, Any]], + own_declared_fields: Mapping[str, Any], +) -> str: + """Build the identifier string for a hook contribution. + + The discriminator suffix is appended only when at least one sibling in the + same source shares the same ``(event_name, command)`` prefix. That keeps the + common case terse and the collision case unambiguous. ``siblings`` must + include every hook entry declared under this source (including the one + whose identifier is being derived); the function decides on its own whether + a collision exists. + """ + base = f"{layer}:{source_id}:hook:{event_name}:{command}" + if _has_hook_sibling_collision(event_name, command, siblings): + return f"{base}:{hook_discriminator(own_declared_fields)}" + return base diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 3968e4fcbe..9ab8283319 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -28,6 +28,13 @@ from packaging.specifiers import InvalidSpecifier, SpecifierSet from .._assets import _locate_core_pack, _repo_root +from .._identifier import ( + IdentifierComponentError, + canonical_json, + derive_hook_id, + derive_named_id, + validate_component, +) from .._download_security import ( archive_format_from_name, archive_suffix, @@ -415,6 +422,11 @@ def _validate(self): raise ValidationError( f"Invalid hook '{hook_name}': list must contain at least one entry" ) + try: + validate_component(hook_name, f"hook event name '{hook_name}'") + except IdentifierComponentError as exc: + raise ValidationError(str(exc)) from exc + event_entries: List[dict] = [] for entry in coerce_hook_entries(hook_config): if not isinstance(entry, dict): raise ValidationError( @@ -425,6 +437,13 @@ def _validate(self): raise ValidationError( f"Hook '{hook_name}' missing required 'command' field" ) + try: + validate_component( + entry["command"], + f"hook '{hook_name}' command", + ) + except IdentifierComponentError as exc: + raise ValidationError(str(exc)) from exc if "priority" in entry: priority = entry["priority"] if not isinstance(priority, int) or isinstance(priority, bool): @@ -437,6 +456,35 @@ def _validate(self): f"Hook '{hook_name}' has invalid 'priority': " "must be >= 1" ) + event_entries.append(entry) + + # Reject two hook entries under the same (event, command) whose + # declared fields (with eventName/command stripped) canonicalize + # to the same byte string — those are semantically identical + # listeners with no way to address them separately. + by_command: Dict[str, List[tuple[int, dict]]] = {} + for idx, entry in enumerate(event_entries): + by_command.setdefault(entry["command"], []).append((idx, entry)) + for command_value, group in by_command.items(): + if len(group) < 2: + continue + seen_canonical: Dict[bytes, int] = {} + for idx, entry in group: + stripped = { + k: v + for k, v in entry.items() + if k not in ("eventName", "command") + } + key = canonical_json(stripped) + if key in seen_canonical: + first_idx = seen_canonical[key] + raise ValidationError( + f"Duplicate hook entries for event '{hook_name}' " + f"command '{command_value}': entries at positions " + f"{first_idx} and {idx} have byte-identical declared " + "fields and cannot be uniquely identified" + ) + seen_canonical[key] = idx # Validate commands; track renames so hook references can be rewritten. rename_map: Dict[str, str] = {} @@ -725,6 +773,112 @@ def hooks(self) -> Dict[str, Any]: """Get hook definitions.""" return self.data.get("hooks", {}) + def iter_contributions(self) -> List[Dict[str, Any]]: + """Return an enriched, ordered list of every contribution this manifest declares. + + Each dict is a shallow copy of the underlying manifest entry with four + derived keys added: ``layer`` (always ``"extension"``), ``sourceId`` + (this manifest's ``id``), ``kind`` (``"command"`` / ``"template"`` / + ``"script"`` / ``"hook"``), and ``id`` (the deterministic identifier). + Hook entries also carry a synthesized ``name`` field of the form + ``"{eventName}:{command}"`` alongside the original ``eventName`` / + ``command`` values, so consumers can locate a hook by its identifier's + name component without re-splitting the string. + + The underlying ``self.data`` mapping is never mutated — the enriched + dicts are constructed fresh on every call so callers can safely rely on + the identifiers reflecting the current in-memory manifest state. + """ + source_id = self.id + contributions: List[Dict[str, Any]] = [] + + for cmd in self.commands: + enriched = dict(cmd) + name = cmd.get("name", "") + enriched.update( + layer="extension", + sourceId=source_id, + kind="command", + id=derive_named_id("extension", source_id, "command", name), + ) + contributions.append(enriched) + + for tmpl in self.templates: + enriched = dict(tmpl) + name = tmpl.get("name", "") + enriched.update( + layer="extension", + sourceId=source_id, + kind="template", + id=derive_named_id("extension", source_id, "template", name), + ) + contributions.append(enriched) + + for scr in self.scripts: + enriched = dict(scr) + name = scr.get("name", "") + enriched.update( + layer="extension", + sourceId=source_id, + kind="script", + id=derive_named_id("extension", source_id, "script", name), + ) + contributions.append(enriched) + + hooks = self.hooks or {} + # Flatten every hook entry across every event so the discriminator + # decision has visibility into the full same-source sibling set. + flattened: List[tuple[str, dict]] = [] + for event_name, hook_config in hooks.items(): + for entry in coerce_hook_entries(hook_config): + if isinstance(entry, dict): + normalized = dict(entry) + normalized.setdefault("eventName", event_name) + flattened.append((event_name, normalized)) + + siblings_for_id = [ + {"eventName": event, "command": entry.get("command", "")} + for event, entry in flattened + ] + + for event_name, entry in flattened: + command_value = entry.get("command", "") + declared_fields = { + k: v + for k, v in entry.items() + if k not in ("eventName", "command") + } + hook_id = derive_hook_id( + "extension", + source_id, + event_name, + command_value, + siblings_for_id, + declared_fields, + ) + enriched = dict(entry) + enriched.update( + layer="extension", + sourceId=source_id, + kind="hook", + name=f"{event_name}:{command_value}", + id=hook_id, + ) + contributions.append(enriched) + + return contributions + + def contribution_id(self, kind: str, name: str) -> Optional[str]: + """Return the computed identifier for a single contribution, if declared. + + ``name`` is the declared name for command/template/script kinds, or the + ``"{eventName}:{command}"`` compound for hook kinds. + """ + for entry in self.iter_contributions(): + if entry["kind"] == kind and entry.get("name") == name: + return entry["id"] + return None + def get_hash(self) -> str: """Calculate SHA256 hash of manifest file.""" h = hashlib.sha256() diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index a5cea4f958..95398e0d31 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -37,6 +37,10 @@ safe_extract_archive, ) from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority +from .._identifier import ( + PROJECT_OVERRIDE_LAYER, + derive_named_id, +) from .._init_options import ( MISSING_INIT_OPTIONS_FILE, is_ai_skills_enabled, @@ -539,6 +543,38 @@ def tags(self) -> List[str]: """Get preset tags.""" return self.data.get("tags", []) + def iter_contributions(self) -> List[Dict[str, Any]]: + """Return an enriched, ordered list of every contribution this preset declares. + + Each dict is a shallow copy of the underlying ``provides.templates[]`` + entry with four derived keys added: ``layer`` (always ``"preset"``), + ``sourceId`` (this preset's ``id``), ``kind`` (mirrors the entry's + ``type`` — one of ``"command"`` / ``"template"`` / ``"script"``), and + ``id`` (the deterministic identifier). The underlying manifest data is + not mutated. + """ + source_id = self.id + contributions: List[Dict[str, Any]] = [] + for entry in self.templates: + kind = entry.get("type", "") + name = entry.get("name", "") + enriched = dict(entry) + enriched.update( + layer="preset", + sourceId=source_id, + kind=kind, + id=derive_named_id("preset", source_id, kind, name), + ) + contributions.append(enriched) + return contributions + + def contribution_id(self, kind: str, name: str) -> Optional[str]: + """Return the computed identifier for a single contribution, if declared.""" + for entry in self.iter_contributions(): + if entry["kind"] == kind and entry.get("name") == name: + return entry["id"] + return None + def get_hash(self) -> str: """Calculate SHA256 hash of manifest file.""" h = hashlib.sha256() @@ -5527,6 +5563,9 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "path": override, "source": "project override", "strategy": "replace", + "lookupId": derive_named_id( + PROJECT_OVERRIDE_LAYER, "_", template_type, template_name + ), }) # Priority 2: Installed presets (sorted by priority — lower number = higher precedence) @@ -5583,6 +5622,9 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "path": candidate, "source": f"{pack_id} v{version}", "strategy": strategy, + "lookupId": derive_named_id( + "preset", pack_id, template_type, template_name + ), }) # Priority 3: Extension-provided templates (always "replace") @@ -5611,6 +5653,9 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "strategy": "replace", "extension_id": ext_id, "extension_dir": ext_dir, + "lookupId": derive_named_id( + "extension", ext_id, template_type, template_name + ), }) # Priority 4: Core templates (always "replace") @@ -5639,6 +5684,9 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "path": core, "source": "core", "strategy": "replace", + "lookupId": derive_named_id( + "core", "_", template_type, template_name + ), }) else: # Priority 5: Bundled core_pack (wheel install) or repo-root @@ -5649,6 +5697,9 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "path": bundled, "source": "core (bundled)", "strategy": "replace", + "lookupId": derive_named_id( + "core", "_", template_type, template_name + ), }) return layers diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py new file mode 100644 index 0000000000..e26a224c3b --- /dev/null +++ b/tests/test_contribution_ids.py @@ -0,0 +1,552 @@ +"""Tests for the deterministic contribution-id and stack lookup-id feature. + +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 +``layer x kind`` combination, the hook discriminator collision + rejection +rules, cross-process byte-stability, path/mtime independence, and the +additive-only shape guarantee for the enriched contribution dicts. +""" + +from __future__ import annotations + +import copy +import json +import os +import shutil +import subprocess +import sys +import textwrap +import time +from pathlib import Path + +import pytest +import yaml + +from specify_cli._identifier import ( + IdentifierComponentError, + PROJECT_OVERRIDE_LAYER, + canonical_json, + derive_hook_id, + derive_named_id, + hook_discriminator, + validate_component, +) +from specify_cli.extensions import ExtensionManifest, ValidationError +from specify_cli.presets import PresetManifest, PresetResolver + + +# --------------------------------------------------------------------------- +# Fixture builders (programmatic — no on-disk fixture tree) +# --------------------------------------------------------------------------- + + +def _preset_data(pack_id: str = "speckit-core") -> dict: + return { + "schema_version": "1.0", + "preset": { + "id": pack_id, + "name": pack_id, + "version": "1.0.0", + "description": "Fixture preset", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + {"type": "command", "name": "speckit.plan", "file": "commands/plan.md"}, + {"type": "template", "name": "spec-template", "file": "templates/spec.md"}, + {"type": "script", "name": "setup-plan", "file": "scripts/setup-plan.sh"}, + ] + }, + } + + +def _extension_data( + ext_id: str = "speckit-git", + hooks: dict | None = None, + with_commands: bool = True, + with_templates: bool = True, + with_scripts: bool = True, +) -> dict: + data = { + "schema_version": "1.0", + "extension": { + "id": ext_id, + "name": ext_id, + "version": "1.0.0", + "description": "Fixture extension", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {}, + } + if with_commands: + data["provides"]["commands"] = [ + { + "name": f"speckit.{ext_id.replace('-', '')}.branch", + "file": "commands/branch.md", + "description": "Fixture command", + } + ] + if with_templates: + data["provides"]["templates"] = [ + {"name": "pr-body", "file": "templates/pr-body.md"} + ] + if with_scripts: + data["provides"]["scripts"] = [ + {"name": "post-commit", "file": "scripts/post-commit.sh"} + ] + if hooks is not None: + data["hooks"] = hooks + return data + + +def _write_manifest(tmp_path: Path, data: dict, filename: str) -> Path: + manifest_path = tmp_path / filename + with open(manifest_path, "w", encoding="utf-8") as fh: + yaml.safe_dump(data, fh, sort_keys=False) + return manifest_path + + +# --------------------------------------------------------------------------- +# Identifier grammar — layer x kind derivation matrix +# --------------------------------------------------------------------------- + + +class TestIdentifierDerivation: + """Every layer x kind combination produces the expected grammar.""" + + @pytest.mark.parametrize( + "layer, source_id, kind, name, expected", + [ + ("core", "_", "command", "speckit.constitution", "core:_:command:speckit.constitution"), + ("core", "_", "template", "spec-template", "core:_:template:spec-template"), + ("core", "_", "script", "setup-plan", "core:_:script:setup-plan"), + ("preset", "speckit-core", "command", "speckit.plan", "preset:speckit-core:command:speckit.plan"), + ("preset", "speckit-core", "template", "spec-template", "preset:speckit-core:template:spec-template"), + ("preset", "speckit-core", "script", "setup-plan", "preset:speckit-core:script:setup-plan"), + ("extension", "speckit-git", "command", "speckit.git.branch", "extension:speckit-git:command:speckit.git.branch"), + ("extension", "speckit-git", "template", "pr-body", "extension:speckit-git:template:pr-body"), + ("extension", "speckit-git", "script", "post-commit", "extension:speckit-git:script:post-commit"), + ], + ) + def test_named_id_grammar(self, layer, source_id, kind, name, expected): + assert derive_named_id(layer, source_id, kind, name) == expected + + @pytest.mark.parametrize( + "layer, source_id, event, command, expected", + [ + ("core", "_", "before_specify", "speckit.constitution", "core:_:hook:before_specify:speckit.constitution"), + ("preset", "speckit-core", "before_plan", "speckit.plan", "preset:speckit-core:hook:before_plan:speckit.plan"), + ("extension", "speckit-git", "before_specify", "speckit.git.branch", "extension:speckit-git:hook:before_specify:speckit.git.branch"), + ], + ) + def test_hook_id_no_discriminator(self, layer, source_id, event, command, expected): + siblings = [{"eventName": event, "command": command}] + assert ( + derive_hook_id(layer, source_id, event, command, siblings, {}) + == expected + ) + + def test_named_id_stable_across_two_derivations(self): + a = derive_named_id("preset", "speckit-core", "command", "speckit.plan") + b = derive_named_id("preset", "speckit-core", "command", "speckit.plan") + assert a == b + + +# --------------------------------------------------------------------------- +# Canonical JSON +# --------------------------------------------------------------------------- + + +class TestCanonicalJson: + def test_sorts_mapping_keys_at_every_depth(self): + payload = {"z": 1, "a": {"y": 2, "x": [3, {"n": 4, "m": 5}]}} + assert canonical_json(payload) == b'{"a":{"x":[3,{"m":5,"n":4}],"y":2},"z":1}' + + def test_preserves_list_order(self): + assert canonical_json([3, 1, 2]) == b"[3,1,2]" + + def test_utf8_no_ensure_ascii(self): + assert canonical_json({"k": "café"}).decode("utf-8") == '{"k":"café"}' + + +# --------------------------------------------------------------------------- +# Hook discriminator behaviour +# --------------------------------------------------------------------------- + + +class TestHookDiscriminator: + def test_no_discriminator_when_unique(self, tmp_path): + data = _extension_data( + hooks={ + "before_specify": {"command": "speckit.speckitgit.branch"}, + } + ) + manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] + assert len(hooks) == 1 + assert hooks[0]["id"] == "extension:speckit-git:hook:before_specify:speckit.speckitgit.branch" + + def test_discriminator_when_colliding(self, tmp_path): + data = _extension_data( + hooks={ + "before_plan": [ + {"command": "speckit.speckitgit.branch", "priority": 10}, + {"command": "speckit.speckitgit.branch", "priority": 20}, + ] + } + ) + manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] + assert len(hooks) == 2 + prefixes = {"extension:speckit-git:hook:before_plan:speckit.speckitgit.branch"} + for h in hooks: + assert h["id"].startswith(next(iter(prefixes)) + ":") + suffix = h["id"].rsplit(":", 1)[-1] + assert len(suffix) == 12 + assert all(ch in "0123456789abcdef" for ch in suffix) + assert hooks[0]["id"] != hooks[1]["id"] + + def test_discriminator_stable_under_reordering(self, tmp_path): + entries_a = [ + {"command": "speckit.speckitgit.branch", "priority": 10}, + {"command": "speckit.speckitgit.branch", "priority": 20}, + ] + entries_b = list(reversed([copy.deepcopy(e) for e in entries_a])) + + dir_a = tmp_path / "a" + dir_a.mkdir() + dir_b = tmp_path / "b" + dir_b.mkdir() + manifest_a = ExtensionManifest( + _write_manifest(dir_a, _extension_data(hooks={"before_plan": entries_a}), "extension.yml") + ) + manifest_b = ExtensionManifest( + _write_manifest(dir_b, _extension_data(hooks={"before_plan": entries_b}), "extension.yml") + ) + + ids_a = { + (h["command"], h.get("priority")): h["id"] + for h in manifest_a.iter_contributions() + if h["kind"] == "hook" + } + ids_b = { + (h["command"], h.get("priority")): h["id"] + for h in manifest_b.iter_contributions() + if h["kind"] == "hook" + } + assert ids_a == ids_b + + def test_byte_identical_declared_fields_rejected_at_load(self, tmp_path): + data = _extension_data( + hooks={ + "after_tasks": [ + {"command": "speckit.speckitgit.branch", "priority": 10}, + {"command": "speckit.speckitgit.branch", "priority": 10}, + ] + } + ) + with pytest.raises(ValidationError) as exc_info: + ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + message = str(exc_info.value) + assert "Duplicate hook entries" in message + assert "after_tasks" in message + assert "positions 0 and 1" in message + + def test_hook_discriminator_helper_is_deterministic(self): + payload = {"priority": 10, "optional": True, "prompt": "Run?"} + a = hook_discriminator(payload) + b = hook_discriminator(dict(reversed(list(payload.items())))) + assert a == b + assert len(a) == 12 + + +# --------------------------------------------------------------------------- +# Manifest component `:` guard +# --------------------------------------------------------------------------- + + +class TestComponentGuard: + def test_validate_component_rejects_colon(self): + with pytest.raises(IdentifierComponentError) as exc_info: + validate_component("has:colon", "test field") + assert "':' is reserved" in str(exc_info.value) + + def test_validate_component_rejects_empty(self): + with pytest.raises(IdentifierComponentError): + validate_component("", "test field") + + def test_validate_component_rejects_non_string(self): + with pytest.raises(IdentifierComponentError): + validate_component(42, "test field") + + def test_extension_hook_event_name_with_colon_rejected(self, tmp_path): + data = _extension_data( + hooks={"before:plan": {"command": "speckit.speckitgit.branch"}} + ) + with pytest.raises(ValidationError) as exc_info: + ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + assert "':' is reserved" in str(exc_info.value) + + def test_extension_hook_command_with_colon_rejected(self, tmp_path): + data = _extension_data( + hooks={"before_plan": {"command": "speckit:bad:command"}} + ) + with pytest.raises(ValidationError) as exc_info: + ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + assert "':' is reserved" in str(exc_info.value) + + +# --------------------------------------------------------------------------- +# `iter_contributions` output surface +# --------------------------------------------------------------------------- + + +class TestContributionSurface: + def test_preset_iter_contributions_matrix(self, tmp_path): + manifest = PresetManifest(_write_manifest(tmp_path, _preset_data(), "preset.yml")) + entries = manifest.iter_contributions() + by_kind = {e["kind"]: e for e in entries} + assert by_kind["command"]["id"] == "preset:speckit-core:command:speckit.plan" + assert by_kind["template"]["id"] == "preset:speckit-core:template:spec-template" + assert by_kind["script"]["id"] == "preset:speckit-core:script:setup-plan" + for entry in entries: + assert entry["layer"] == "preset" + assert entry["sourceId"] == "speckit-core" + + def test_extension_iter_contributions_matrix(self, tmp_path): + data = _extension_data( + hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} + ) + manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + entries = manifest.iter_contributions() + kinds = {e["kind"]: e for e in entries} + assert kinds["command"]["id"] == "extension:speckit-git:command:speckit.speckitgit.branch" + assert kinds["template"]["id"] == "extension:speckit-git:template:pr-body" + assert kinds["script"]["id"] == "extension:speckit-git:script:post-commit" + assert kinds["hook"]["id"] == "extension:speckit-git:hook:before_specify:speckit.speckitgit.branch" + assert kinds["hook"]["name"] == "before_specify:speckit.speckitgit.branch" + + def test_contribution_id_lookup(self, tmp_path): + manifest = PresetManifest(_write_manifest(tmp_path, _preset_data(), "preset.yml")) + assert ( + manifest.contribution_id("command", "speckit.plan") + == "preset:speckit-core:command:speckit.plan" + ) + assert manifest.contribution_id("command", "does-not-exist") is None + + def test_representation_shape_is_additive_for_preset(self, tmp_path): + original = _preset_data() + manifest = PresetManifest(_write_manifest(tmp_path, original, "preset.yml")) + derived_keys = {"layer", "sourceId", "kind", "id"} + for src_entry, out_entry in zip(original["provides"]["templates"], manifest.iter_contributions()): + assert set(src_entry.keys()).issubset(out_entry.keys()) + assert derived_keys.issubset(out_entry.keys()) + + def test_representation_shape_is_additive_for_extension(self, tmp_path): + original = _extension_data( + hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} + ) + manifest = ExtensionManifest(_write_manifest(tmp_path, original, "extension.yml")) + entries = manifest.iter_contributions() + derived_named = {"layer", "sourceId", "kind", "id"} + + cmd_entry = original["provides"]["commands"][0] + cmd_out = next(e for e in entries if e["kind"] == "command") + assert set(cmd_entry.keys()).issubset(cmd_out.keys()) + assert derived_named.issubset(cmd_out.keys()) + + hook_entry = original["hooks"]["before_specify"] + hook_out = next(e for e in entries if e["kind"] == "hook") + assert set(hook_entry.keys()).issubset(hook_out.keys()) + assert derived_named.issubset(hook_out.keys()) + assert hook_out["name"] == "before_specify:speckit.speckitgit.branch" + + def test_underlying_data_not_mutated(self, tmp_path): + original = _preset_data() + original_snapshot = copy.deepcopy(original) + manifest = PresetManifest(_write_manifest(tmp_path, original, "preset.yml")) + _ = manifest.iter_contributions() + assert manifest.data == original_snapshot + + +# --------------------------------------------------------------------------- +# `lookupId` round-trip through the resolver +# --------------------------------------------------------------------------- + + +def _make_project(root: Path) -> Path: + """Create a minimal project layout the resolver understands.""" + (root / ".specify" / "presets").mkdir(parents=True) + (root / ".specify" / "extensions").mkdir(parents=True) + (root / ".specify" / "memory").mkdir(parents=True) + (root / "templates" / "commands").mkdir(parents=True) + (root / "templates" / "scripts").mkdir(parents=True) + return root + + +class TestLookupIdRoundTrip: + def test_project_override_layer_carries_sentinel_lookup_id(self, tmp_path): + project = _make_project(tmp_path) + overrides_dir = project / ".specify" / "templates" / "overrides" + overrides_dir.mkdir(parents=True) + (overrides_dir / "spec-template.md").write_text("override", encoding="utf-8") + resolver = PresetResolver(project) + layers = resolver.collect_all_layers("spec-template", "template") + override_layer = next(l for l in layers if l["source"] == "project override") + assert override_layer["lookupId"] == derive_named_id( + PROJECT_OVERRIDE_LAYER, "_", "template", "spec-template" + ) + + def test_core_layer_carries_core_lookup_id(self, tmp_path): + project = _make_project(tmp_path) + (project / "templates" / "spec-template.md").write_text("core", encoding="utf-8") + # PresetResolver reads templates from a bundled/repo path — point the + # resolver at the fixture project by monkey-patching the templates_dir. + resolver = PresetResolver(project) + resolver.templates_dir = project / "templates" + layers = resolver.collect_all_layers("spec-template", "template") + core_layer = next(l for l in layers if l["source"] == "core") + assert core_layer["lookupId"] == "core:_:template:spec-template" + + def test_preset_layer_lookup_id_matches_manifest_contribution_id(self, tmp_path): + project = _make_project(tmp_path) + pack_id = "speckit-fixture" + pack_dir = project / ".specify" / "presets" / pack_id + (pack_dir / "templates").mkdir(parents=True) + (pack_dir / "templates" / "spec-template.md").write_text("preset", encoding="utf-8") + _write_manifest( + pack_dir, + { + "schema_version": "1.0", + "preset": { + "id": pack_id, + "name": pack_id, + "version": "1.0.0", + "description": "Fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + } + ] + }, + }, + "preset.yml", + ) + registry = { + "schema_version": "1.0", + "presets": { + pack_id: {"version": "1.0.0", "priority": 10, "enabled": True} + }, + } + (project / ".specify" / "presets" / ".registry").write_text( + json.dumps(registry), encoding="utf-8" + ) + resolver = PresetResolver(project) + layers = resolver.collect_all_layers("spec-template", "template") + preset_layer = next(l for l in layers if l["source"].startswith(pack_id)) + manifest = PresetManifest(pack_dir / "preset.yml") + assert preset_layer["lookupId"] == manifest.contribution_id("template", "spec-template") + assert preset_layer["lookupId"] == f"preset:{pack_id}:template:spec-template" + + +# --------------------------------------------------------------------------- +# Determinism across environments +# --------------------------------------------------------------------------- + + +_SUBPROCESS_SCRIPT = textwrap.dedent( + """ + import sys, json + from specify_cli.extensions import ExtensionManifest + manifest = ExtensionManifest(sys.argv[1]) + ids = [c["id"] for c in manifest.iter_contributions()] + sys.stdout.write(json.dumps(ids)) + """ +) + + +class TestDeterminism: + def _fixture_manifest(self, tmp_path: Path) -> Path: + data = _extension_data( + hooks={ + "before_specify": {"command": "speckit.speckitgit.branch"}, + "before_plan": [ + {"command": "speckit.speckitgit.branch", "priority": 10}, + {"command": "speckit.speckitgit.branch", "priority": 20}, + ], + } + ) + return _write_manifest(tmp_path, data, "extension.yml") + + def test_identifiers_match_across_subprocesses(self, tmp_path): + manifest_path = self._fixture_manifest(tmp_path) + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join( + [str(Path(__file__).resolve().parent.parent / "src"), env.get("PYTHONPATH", "")] + ) + + def _run() -> str: + proc = subprocess.run( + [sys.executable, "-c", _SUBPROCESS_SCRIPT, str(manifest_path)], + capture_output=True, + text=True, + env=env, + check=True, + ) + return proc.stdout + + assert _run() == _run() + + def test_ids_independent_of_paths_and_mtimes(self, tmp_path): + original_dir = tmp_path / "orig" + copied_dir = tmp_path / "copy" + original_dir.mkdir() + manifest_path = self._fixture_manifest(original_dir) + original_ids = [c["id"] for c in ExtensionManifest(manifest_path).iter_contributions()] + + shutil.copytree(original_dir, copied_dir) + distant_past = time.time() - 3600 + os.utime(copied_dir / manifest_path.name, (distant_past, distant_past)) + copied_ids = [ + c["id"] for c in ExtensionManifest(copied_dir / manifest_path.name).iter_contributions() + ] + assert original_ids == copied_ids + + +# --------------------------------------------------------------------------- +# Identifiers never persisted +# --------------------------------------------------------------------------- + + +class TestNoPersistence: + def test_no_id_written_to_manifest_files(self, tmp_path): + data = _extension_data( + hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} + ) + manifest_path = _write_manifest(tmp_path, data, "extension.yml") + # Read identifiers to force the derivation code path. + manifest = ExtensionManifest(manifest_path) + ids = [c["id"] for c in manifest.iter_contributions()] + assert ids # sanity check — feature actually ran + on_disk = manifest_path.read_text(encoding="utf-8") + assert ":command:" not in on_disk + assert ":template:" not in on_disk + assert ":script:" not in on_disk + assert ":hook:" not in on_disk + + def test_no_id_written_to_preset_manifest_files(self, tmp_path): + preset_path = _write_manifest(tmp_path, _preset_data(), "preset.yml") + manifest = PresetManifest(preset_path) + _ = [c["id"] for c in manifest.iter_contributions()] + on_disk = preset_path.read_text(encoding="utf-8") + assert ":command:" not in on_disk + assert ":template:" not in on_disk + assert ":script:" not in on_disk + From c04e6d7fb8336f33351ef451eb86f3fd047c853f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:13:45 +0000 Subject: [PATCH 02/13] Remove trailing blank line Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- tests/test_contribution_ids.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index e26a224c3b..f9efe049c1 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -549,4 +549,3 @@ def test_no_id_written_to_preset_manifest_files(self, tmp_path): assert ":command:" not in on_disk assert ":template:" not in on_disk assert ":script:" not in on_disk - From ec2191d8ec4e97d7aa6afc281ed4ad19451f92a9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:15:39 +0000 Subject: [PATCH 03/13] Fix markdownlint blank lines Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- extensions/EXTENSION-API-REFERENCE.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index 475c3c8212..08a0eb3bda 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -913,8 +913,6 @@ Identifier derivation reads only the in-memory declared manifest content. No fil Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Parse them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than by string-splitting on `:` — the discriminator suffix and future grammar extensions may otherwise catch you out. - - ```text .specify/ ├── extensions/ From e29d289c6bd228d9df236820d494134ba65e1d01 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 17:28:47 +0000 Subject: [PATCH 04/13] Align hook contribution IDs with installation Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- extensions/EXTENSION-API-REFERENCE.md | 10 +- src/specify_cli/_identifier.py | 93 ++---------------- src/specify_cli/extensions/__init__.py | 67 ++++--------- tests/test_contribution_ids.py | 128 +++++-------------------- 4 files changed, 50 insertions(+), 248 deletions(-) diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index 08a0eb3bda..2500694e9c 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -883,13 +883,7 @@ Hook contributions use a compound name-component built from the event and comman {layer}:{sourceId}:hook:{eventName}:{command} ``` -When two or more hook entries within the same source share the same `(eventName, command)` pair, a 12-hex-character discriminator is appended: - -```text -{layer}:{sourceId}:hook:{eventName}:{command}:{discriminator} -``` - -The discriminator is the first 12 lowercase hex characters of `sha256(canonical_json(entry - {eventName, command}))`. Two hook entries with byte-identical declared fields (after removing `eventName` and `command`) are rejected at manifest load with a `ValidationError` naming both positions — there is no meaningful way to distinguish them at read time. +If an extension declares multiple hooks with the same `(eventName, command)` pair, the final declaration wins, matching hook installation. `iter_contributions()` emits only that final hook, so every emitted hook identifier corresponds to an installed hook. ### Reserved character @@ -911,7 +905,7 @@ Identifier derivation reads only the in-memory declared manifest content. No fil ### Opacity guidance -Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Parse them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than by string-splitting on `:` — the discriminator suffix and future grammar extensions may otherwise catch you out. +Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Parse them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than by string-splitting on `:` so future grammar extensions do not break consumers. ```text .specify/ diff --git a/src/specify_cli/_identifier.py b/src/specify_cli/_identifier.py index 4124157df5..2bd0540453 100644 --- a/src/specify_cli/_identifier.py +++ b/src/specify_cli/_identifier.py @@ -20,15 +20,11 @@ Hook identifiers use ``{eventName}:{command}`` as the name component:: - id = "{layer}:{sourceId}:hook:{eventName}:{command}[:{discriminator}]" + id = "{layer}:{sourceId}:hook:{eventName}:{command}" -The 12-lowercase-hex discriminator is appended only when at least one sibling -hook in the same source shares the same ``(eventName, command)`` pair, and it is -computed by SHA-256 of a canonical JSON serialization of the hook entry's -declared fields (with ``eventName`` and ``command`` removed, since they already -appear in the identifier prefix). Two hook entries in the same source whose -declared fields produce byte-identical canonical JSON are rejected at manifest -load time — they are semantically identical listeners. +Extension hook contributions use the same last-write-wins behavior as hook +installation, so at most one contribution exists for each +``(eventName, command)`` pair. The functions in this module are pure — inputs are strings or in-memory mappings parsed from a manifest, outputs are strings. None of them read from @@ -40,9 +36,7 @@ from __future__ import annotations -import hashlib -import json -from typing import Any, Iterable, Mapping +from typing import Any PROJECT_OVERRIDE_LAYER = "project" @@ -56,9 +50,6 @@ is the correct outcome for a layer with no originating manifest entry. """ -_DISCRIMINATOR_LENGTH = 12 - - class IdentifierComponentError(ValueError): """Raised when a manifest component would break identifier grammar.""" @@ -98,81 +89,11 @@ def derive_named_id(layer: str, source_id: str, kind: str, name: str) -> str: return f"{layer}:{source_id}:{kind}:{name}" -def canonical_json(value: Any) -> bytes: - """Serialize ``value`` to a canonical UTF-8 JSON byte string. - - Mapping keys are sorted lexicographically at every depth, list order is - preserved (author intent), whitespace is stripped, and non-ASCII characters - are emitted verbatim. This is the byte string that the hook discriminator - hashes and that the manifest loader uses to detect byte-identical duplicate - hook entries. - """ - normalized = _normalize_for_canonical_json(value) - return json.dumps( - normalized, - sort_keys=True, - ensure_ascii=False, - separators=(",", ":"), - ).encode("utf-8") - - -def _normalize_for_canonical_json(value: Any) -> Any: - if isinstance(value, Mapping): - return {str(k): _normalize_for_canonical_json(v) for k, v in value.items()} - if isinstance(value, (list, tuple)): - return [_normalize_for_canonical_json(v) for v in value] - return value - - -def _has_hook_sibling_collision( - event_name: str, - command: str, - siblings: Iterable[Mapping[str, Any]], -) -> bool: - """Return True when at least one sibling shares the same event/command pair. - - ``siblings`` is the full same-source hook entry list including the entry - whose identifier is being derived. A collision therefore means at least two - entries share the pair. - """ - seen = 0 - for entry in siblings: - if entry.get("eventName") == event_name and entry.get("command") == command: - seen += 1 - if seen >= 2: - return True - return False - - -def hook_discriminator(declared_fields: Mapping[str, Any]) -> str: - """Compute the 12-hex-char SHA-256 discriminator for a hook entry. - - ``declared_fields`` is the entry as parsed from the manifest with - ``eventName`` and ``command`` removed — those two values already appear in - the identifier prefix, so hashing them would only reflect information the - consumer can already read. - """ - return hashlib.sha256(canonical_json(declared_fields)).hexdigest()[:_DISCRIMINATOR_LENGTH] - - def derive_hook_id( layer: str, source_id: str, event_name: str, command: str, - siblings: Iterable[Mapping[str, Any]], - own_declared_fields: Mapping[str, Any], ) -> str: - """Build the identifier string for a hook contribution. - - The discriminator suffix is appended only when at least one sibling in the - same source shares the same ``(event_name, command)`` prefix. That keeps the - common case terse and the collision case unambiguous. ``siblings`` must - include every hook entry declared under this source (including the one - whose identifier is being derived); the function decides on its own whether - a collision exists. - """ - base = f"{layer}:{source_id}:hook:{event_name}:{command}" - if _has_hook_sibling_collision(event_name, command, siblings): - return f"{base}:{hook_discriminator(own_declared_fields)}" - return base + """Build the identifier string for a hook contribution.""" + return f"{layer}:{source_id}:hook:{event_name}:{command}" diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 9ab8283319..bf7910847c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -30,7 +30,6 @@ from .._assets import _locate_core_pack, _repo_root from .._identifier import ( IdentifierComponentError, - canonical_json, derive_hook_id, derive_named_id, validate_component, @@ -458,34 +457,6 @@ def _validate(self): ) event_entries.append(entry) - # Reject two hook entries under the same (event, command) whose - # declared fields (with eventName/command stripped) canonicalize - # to the same byte string — those are semantically identical - # listeners with no way to address them separately. - by_command: Dict[str, List[tuple[int, dict]]] = {} - for idx, entry in enumerate(event_entries): - by_command.setdefault(entry["command"], []).append((idx, entry)) - for command_value, group in by_command.items(): - if len(group) < 2: - continue - seen_canonical: Dict[bytes, int] = {} - for idx, entry in group: - stripped = { - k: v - for k, v in entry.items() - if k not in ("eventName", "command") - } - key = canonical_json(stripped) - if key in seen_canonical: - first_idx = seen_canonical[key] - raise ValidationError( - f"Duplicate hook entries for event '{hook_name}' " - f"command '{command_value}': entries at positions " - f"{first_idx} and {idx} have byte-identical declared " - "fields and cannot be uniquely identified" - ) - seen_canonical[key] = idx - # Validate commands; track renames so hook references can be rewritten. rename_map: Dict[str, str] = {} for cmd in commands: @@ -826,35 +797,33 @@ def iter_contributions(self) -> List[Dict[str, Any]]: contributions.append(enriched) hooks = self.hooks or {} - # Flatten every hook entry across every event so the discriminator - # decision has visibility into the full same-source sibling set. - flattened: List[tuple[str, dict]] = [] + # Mirror registration's last-write-wins behavior for duplicate commands + # in the same event, so IDs exist only for hooks installed on disk. + collapsed: List[tuple[str, dict]] = [] + seen: Dict[tuple[str, str], int] = {} for event_name, hook_config in hooks.items(): for entry in coerce_hook_entries(hook_config): - if isinstance(entry, dict): - normalized = dict(entry) - normalized.setdefault("eventName", event_name) - flattened.append((event_name, normalized)) - - siblings_for_id = [ - {"eventName": event, "command": entry.get("command", "")} - for event, entry in flattened - ] + if not isinstance(entry, dict): + continue + command_value = entry.get("command") + if not command_value: + continue + normalized = dict(entry) + normalized.setdefault("eventName", event_name) + key = (event_name, command_value) + if key in seen: + collapsed[seen[key]] = (event_name, normalized) + else: + seen[key] = len(collapsed) + collapsed.append((event_name, normalized)) - for event_name, entry in flattened: + for event_name, entry in collapsed: command_value = entry.get("command", "") - declared_fields = { - k: v - for k, v in entry.items() - if k not in ("eventName", "command") - } hook_id = derive_hook_id( "extension", source_id, event_name, command_value, - siblings_for_id, - declared_fields, ) enriched = dict(entry) enriched.update( diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index f9efe049c1..4b08df1030 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -4,9 +4,9 @@ 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 -``layer x kind`` combination, the hook discriminator collision + rejection -rules, cross-process byte-stability, path/mtime independence, and the -additive-only shape guarantee for the enriched contribution dicts. +``layer x kind`` combination, duplicate hook collapse behavior, cross-process +byte-stability, path/mtime independence, and the additive-only shape guarantee +for the enriched contribution dicts. """ from __future__ import annotations @@ -27,10 +27,8 @@ from specify_cli._identifier import ( IdentifierComponentError, PROJECT_OVERRIDE_LAYER, - canonical_json, derive_hook_id, derive_named_id, - hook_discriminator, validate_component, ) from specify_cli.extensions import ExtensionManifest, ValidationError @@ -141,12 +139,8 @@ def test_named_id_grammar(self, layer, source_id, kind, name, expected): ("extension", "speckit-git", "before_specify", "speckit.git.branch", "extension:speckit-git:hook:before_specify:speckit.git.branch"), ], ) - def test_hook_id_no_discriminator(self, layer, source_id, event, command, expected): - siblings = [{"eventName": event, "command": command}] - assert ( - derive_hook_id(layer, source_id, event, command, siblings, {}) - == expected - ) + def test_hook_id_grammar(self, layer, source_id, event, command, expected): + assert derive_hook_id(layer, source_id, event, command) == expected def test_named_id_stable_across_two_derivations(self): a = derive_named_id("preset", "speckit-core", "command", "speckit.plan") @@ -155,40 +149,12 @@ def test_named_id_stable_across_two_derivations(self): # --------------------------------------------------------------------------- -# Canonical JSON -# --------------------------------------------------------------------------- - - -class TestCanonicalJson: - def test_sorts_mapping_keys_at_every_depth(self): - payload = {"z": 1, "a": {"y": 2, "x": [3, {"n": 4, "m": 5}]}} - assert canonical_json(payload) == b'{"a":{"x":[3,{"m":5,"n":4}],"y":2},"z":1}' - - def test_preserves_list_order(self): - assert canonical_json([3, 1, 2]) == b"[3,1,2]" - - def test_utf8_no_ensure_ascii(self): - assert canonical_json({"k": "café"}).decode("utf-8") == '{"k":"café"}' - - -# --------------------------------------------------------------------------- -# Hook discriminator behaviour +# Duplicate hook behavior # --------------------------------------------------------------------------- -class TestHookDiscriminator: - def test_no_discriminator_when_unique(self, tmp_path): - data = _extension_data( - hooks={ - "before_specify": {"command": "speckit.speckitgit.branch"}, - } - ) - manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) - hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] - assert len(hooks) == 1 - assert hooks[0]["id"] == "extension:speckit-git:hook:before_specify:speckit.speckitgit.branch" - - def test_discriminator_when_colliding(self, tmp_path): +class TestDuplicateHooks: + def test_last_duplicate_hook_is_the_only_contribution(self, tmp_path): data = _extension_data( hooks={ "before_plan": [ @@ -199,67 +165,15 @@ def test_discriminator_when_colliding(self, tmp_path): ) manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] - assert len(hooks) == 2 - prefixes = {"extension:speckit-git:hook:before_plan:speckit.speckitgit.branch"} - for h in hooks: - assert h["id"].startswith(next(iter(prefixes)) + ":") - suffix = h["id"].rsplit(":", 1)[-1] - assert len(suffix) == 12 - assert all(ch in "0123456789abcdef" for ch in suffix) - assert hooks[0]["id"] != hooks[1]["id"] - - def test_discriminator_stable_under_reordering(self, tmp_path): - entries_a = [ - {"command": "speckit.speckitgit.branch", "priority": 10}, - {"command": "speckit.speckitgit.branch", "priority": 20}, - ] - entries_b = list(reversed([copy.deepcopy(e) for e in entries_a])) - - dir_a = tmp_path / "a" - dir_a.mkdir() - dir_b = tmp_path / "b" - dir_b.mkdir() - manifest_a = ExtensionManifest( - _write_manifest(dir_a, _extension_data(hooks={"before_plan": entries_a}), "extension.yml") - ) - manifest_b = ExtensionManifest( - _write_manifest(dir_b, _extension_data(hooks={"before_plan": entries_b}), "extension.yml") - ) - - ids_a = { - (h["command"], h.get("priority")): h["id"] - for h in manifest_a.iter_contributions() - if h["kind"] == "hook" - } - ids_b = { - (h["command"], h.get("priority")): h["id"] - for h in manifest_b.iter_contributions() - if h["kind"] == "hook" - } - assert ids_a == ids_b - - def test_byte_identical_declared_fields_rejected_at_load(self, tmp_path): - data = _extension_data( - hooks={ - "after_tasks": [ - {"command": "speckit.speckitgit.branch", "priority": 10}, - {"command": "speckit.speckitgit.branch", "priority": 10}, - ] - } + assert len(hooks) == 1 + assert hooks[0]["priority"] == 20 + assert hooks[0]["id"] == "extension:speckit-git:hook:before_plan:speckit.speckitgit.branch" + assert ( + manifest.contribution_id( + "hook", "before_plan:speckit.speckitgit.branch" + ) + == hooks[0]["id"] ) - with pytest.raises(ValidationError) as exc_info: - ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) - message = str(exc_info.value) - assert "Duplicate hook entries" in message - assert "after_tasks" in message - assert "positions 0 and 1" in message - - def test_hook_discriminator_helper_is_deterministic(self): - payload = {"priority": 10, "optional": True, "prompt": "Run?"} - a = hook_discriminator(payload) - b = hook_discriminator(dict(reversed(list(payload.items())))) - assert a == b - assert len(a) == 12 # --------------------------------------------------------------------------- @@ -394,7 +308,9 @@ def test_project_override_layer_carries_sentinel_lookup_id(self, tmp_path): (overrides_dir / "spec-template.md").write_text("override", encoding="utf-8") resolver = PresetResolver(project) layers = resolver.collect_all_layers("spec-template", "template") - override_layer = next(l for l in layers if l["source"] == "project override") + override_layer = next( + layer for layer in layers if layer["source"] == "project override" + ) assert override_layer["lookupId"] == derive_named_id( PROJECT_OVERRIDE_LAYER, "_", "template", "spec-template" ) @@ -407,7 +323,7 @@ def test_core_layer_carries_core_lookup_id(self, tmp_path): resolver = PresetResolver(project) resolver.templates_dir = project / "templates" layers = resolver.collect_all_layers("spec-template", "template") - core_layer = next(l for l in layers if l["source"] == "core") + core_layer = next(layer for layer in layers if layer["source"] == "core") assert core_layer["lookupId"] == "core:_:template:spec-template" def test_preset_layer_lookup_id_matches_manifest_contribution_id(self, tmp_path): @@ -450,7 +366,9 @@ def test_preset_layer_lookup_id_matches_manifest_contribution_id(self, tmp_path) ) resolver = PresetResolver(project) layers = resolver.collect_all_layers("spec-template", "template") - preset_layer = next(l for l in layers if l["source"].startswith(pack_id)) + preset_layer = next( + layer for layer in layers if layer["source"].startswith(pack_id) + ) manifest = PresetManifest(pack_dir / "preset.yml") assert preset_layer["lookupId"] == manifest.contribution_id("template", "spec-template") assert preset_layer["lookupId"] == f"preset:{pack_id}:template:spec-template" From f7a7395a586ed2e4038ba77b745c7c402296e40c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:32:53 +0000 Subject: [PATCH 05/13] Align hook contribution ordering with installer semantics Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- docs/reference/presets.md | 2 +- src/specify_cli/extensions/__init__.py | 88 ++++++++++++-------------- tests/test_contribution_ids.py | 21 ++++++ 3 files changed, 64 insertions(+), 47 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 6f4a428908..abeff253b9 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -222,7 +222,7 @@ Identifiers are computed on demand from author-declared manifest content and are `PresetResolver.collect_all_layers()` returns layer dicts that each include a `lookupId` field pointing back to the originating contribution's `id`. Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they carry a synthetic `project:_:{kind}:{name}` `lookupId` that intentionally does not match any manifest contribution. -For the full grammar, including the hook name-component convention and the discriminator recipe used by extensions, see the [Extension API Reference — Contribution Identifiers](../../extensions/EXTENSION-API-REFERENCE.md#contribution-identifiers) section. +For the full grammar, including the hook name-component convention and extension hook duplicate collapse semantics, see the [Extension API Reference — Contribution Identifiers](../../extensions/EXTENSION-API-REFERENCE.md#contribution-identifiers) section. ## FAQ diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index bf7910847c..444a959a15 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -219,6 +219,27 @@ def coerce_hook_entries(hook_config: Any) -> List[Any]: return hook_config if isinstance(hook_config, list) else [hook_config] +def collapse_hook_event_entries(event_name: str, hook_config: Any) -> List[Dict[str, Any]]: + """Collapse a hook event to final entries using installer last-write-wins. + + Duplicate commands are removed and re-inserted so the final declaration is + retained at the end of the event list, matching register-time ordering. + """ + collapsed: Dict[str, Dict[str, Any]] = {} + for entry in coerce_hook_entries(hook_config): + if not isinstance(entry, dict): + continue + command = entry.get("command") + if not command: + continue + normalized = dict(entry) + normalized.setdefault("eventName", event_name) + if command in collapsed: + del collapsed[command] + collapsed[command] = normalized + return list(collapsed.values()) + + @dataclass class CatalogEntry(BaseCatalogEntry): """Represents a single catalog entry in the catalog stack.""" @@ -797,43 +818,24 @@ def iter_contributions(self) -> List[Dict[str, Any]]: contributions.append(enriched) hooks = self.hooks or {} - # Mirror registration's last-write-wins behavior for duplicate commands - # in the same event, so IDs exist only for hooks installed on disk. - collapsed: List[tuple[str, dict]] = [] - seen: Dict[tuple[str, str], int] = {} for event_name, hook_config in hooks.items(): - for entry in coerce_hook_entries(hook_config): - if not isinstance(entry, dict): - continue - command_value = entry.get("command") - if not command_value: - continue - normalized = dict(entry) - normalized.setdefault("eventName", event_name) - key = (event_name, command_value) - if key in seen: - collapsed[seen[key]] = (event_name, normalized) - else: - seen[key] = len(collapsed) - collapsed.append((event_name, normalized)) - - for event_name, entry in collapsed: - command_value = entry.get("command", "") - hook_id = derive_hook_id( - "extension", - source_id, - event_name, - command_value, - ) - enriched = dict(entry) - enriched.update( - layer="extension", - sourceId=source_id, - kind="hook", - name=f"{event_name}:{command_value}", - id=hook_id, - ) - contributions.append(enriched) + for entry in collapse_hook_event_entries(event_name, hook_config): + command_value = entry.get("command", "") + hook_id = derive_hook_id( + "extension", + source_id, + event_name, + command_value, + ) + enriched = dict(entry) + enriched.update( + layer="extension", + sourceId=source_id, + kind="hook", + name=f"{event_name}:{command_value}", + id=hook_id, + ) + contributions.append(enriched) return contributions @@ -5152,17 +5154,11 @@ def register_hooks(self, manifest: ExtensionManifest): config["hooks"][hook_name] = [] changed = True - # Key by command to dedup within the manifest. Deleting before - # re-insert moves a duplicate to the end so "last wins" also breaks ties. + # Key by command after canonical last-write-wins collapse so order + # exactly matches iter_contributions() for duplicate declarations. new_entries: Dict[str, Dict[str, Any]] = {} - for entry in coerce_hook_entries(hook_config): - if not isinstance(entry, dict): - continue - command = entry.get("command") - if not command: - continue - if command in new_entries: - del new_entries[command] + for entry in collapse_hook_event_entries(hook_name, hook_config): + command = entry["command"] new_entries[command] = { "extension": manifest.id, "command": command, diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index 4b08df1030..bdfb211f4a 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -175,6 +175,27 @@ def test_last_duplicate_hook_is_the_only_contribution(self, tmp_path): == hooks[0]["id"] ) + def test_duplicate_hook_moves_to_end_like_installer(self, tmp_path): + data = _extension_data( + hooks={ + "before_plan": [ + {"command": "speckit.dup", "priority": 1}, + {"command": "speckit.other", "priority": 2}, + {"command": "speckit.dup", "priority": 3}, + ] + }, + with_commands=False, + with_templates=False, + with_scripts=False, + ) + manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] + assert [h["name"] for h in hooks] == [ + "before_plan:speckit.other", + "before_plan:speckit.dup", + ] + assert [h["priority"] for h in hooks] == [2, 3] + # --------------------------------------------------------------------------- # Manifest component `:` guard From 5343edd3c47b98aa1678133095f1480fc0b8bccc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:49:26 +0000 Subject: [PATCH 06/13] Validate identifier components for all named contribution names Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- extensions/EXTENSION-API-REFERENCE.md | 2 +- src/specify_cli/extensions/__init__.py | 18 ++++++++++++++++++ src/specify_cli/presets/__init__.py | 11 +++++++++++ tests/test_contribution_ids.py | 26 +++++++++++++++++++++++++- 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index 2500694e9c..5ed1b45f25 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -887,7 +887,7 @@ If an extension declares multiple hooks with the same `(eventName, command)` pai ### Reserved character -`:` is reserved as the identifier component separator. It cannot appear inside any of `layer`, `sourceId`, `kind`, `name`, `eventName`, or `command`. Extension ids, command names, template names, and script names are already constrained by their existing regex patterns (`^[a-z0-9-]+$` and friends), which forbid `:`. Hook event names (mapping keys) and hook `command` values are additionally validated to reject `:` at manifest load. +`:` is reserved as the identifier component separator. It cannot appear inside any of `layer`, `sourceId`, `kind`, `name`, `eventName`, or `command`. Extension ids, command names, template names, and script names are already constrained by their existing regex patterns (`^[a-z0-9-]+$` and friends), which forbid `:`. In addition, every value that appears in an identifier — contribution names (commands, templates, scripts) as well as hook event names (mapping keys) and hook `command` values — is explicitly validated to reject `:` at manifest load, so the guarantee holds uniformly. ### The `project:` sentinel diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 444a959a15..5c3dbace3d 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -527,6 +527,15 @@ def _validate(self): "must follow pattern 'speckit.{extension}.{command}'" ) + # The (possibly corrected) name is an identifier component, so it + # may not contain the ':' delimiter. EXTENSION_COMMAND_NAME_PATTERN + # already excludes it; the explicit guard keeps the guarantee + # uniform with the hook fields. + try: + validate_component(cmd["name"], f"command name '{cmd['name']}'") + except IdentifierComponentError as exc: + raise ValidationError(str(exc)) from exc + # Validate alias types; no pattern enforcement on aliases — they are # intentionally free-form to preserve community extension compatibility # (e.g. 'speckit.verify' short aliases used by existing extensions). @@ -648,6 +657,15 @@ def _validate_provided_artifacts(entries: List[Any], section: str, singular: str ) seen_names.add(name) + # The name is an identifier component, so it may not contain the + # ':' delimiter. VALID_EXTENSION_ARTIFACT_NAME_PATTERN already + # excludes it; the explicit guard keeps the guarantee uniform with + # the hook fields. + try: + validate_component(name, f"{singular} name '{name}'") + except IdentifierComponentError as exc: + raise ValidationError(str(exc)) from exc + file_value = entry["file"] reason = relative_extension_path_violation(file_value) if reason: diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 95398e0d31..ac20f85c1b 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -39,7 +39,9 @@ from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority from .._identifier import ( PROJECT_OVERRIDE_LAYER, + IdentifierComponentError, derive_named_id, + validate_component, ) from .._init_options import ( MISSING_INIT_OPTIONS_FILE, @@ -503,6 +505,15 @@ def _validate(self): "must be lowercase alphanumeric with hyphens only" ) + # The name is an identifier component, so it may not contain the + # ':' delimiter. The patterns above already exclude it; the explicit + # guard keeps the guarantee uniform with the hook fields and holds + # if those patterns are ever relaxed. + try: + validate_component(tmpl["name"], f"template name '{tmpl['name']}'") + except IdentifierComponentError as exc: + raise PresetValidationError(str(exc)) from exc + @property def id(self) -> str: """Get preset ID.""" diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index bdfb211f4a..91243b6b68 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -32,7 +32,7 @@ validate_component, ) from specify_cli.extensions import ExtensionManifest, ValidationError -from specify_cli.presets import PresetManifest, PresetResolver +from specify_cli.presets import PresetManifest, PresetResolver, PresetValidationError # --------------------------------------------------------------------------- @@ -232,6 +232,30 @@ def test_extension_hook_command_with_colon_rejected(self, tmp_path): ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) assert "':' is reserved" in str(exc_info.value) + @pytest.mark.parametrize("template_type", ["command", "template", "script"]) + def test_preset_template_name_with_colon_rejected(self, tmp_path, template_type): + data = _preset_data() + data["provides"]["templates"] = [ + {"type": template_type, "name": "bad:name", "file": "commands/x.md"} + ] + with pytest.raises(PresetValidationError): + PresetManifest(_write_manifest(tmp_path, data, "preset.yml")) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_extension_artifact_name_with_colon_rejected(self, tmp_path, section): + data = _extension_data() + data["provides"][section] = [{"name": "bad:name", "file": "x/y.md"}] + with pytest.raises(ValidationError): + ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + + def test_extension_command_name_with_colon_rejected(self, tmp_path): + data = _extension_data() + data["provides"]["commands"] = [ + {"name": "speckit.git:branch", "file": "commands/branch.md"} + ] + with pytest.raises(ValidationError): + ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) + # --------------------------------------------------------------------------- # `iter_contributions` output surface From 1dbd291e0b33213c25c9f7971c6350c20f224de1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:18:01 +0000 Subject: [PATCH 07/13] Address contribution ID docs feedback Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- docs/reference/presets.md | 10 +++ extensions/EXTENSION-API-REFERENCE.md | 15 +++- tests/test_contribution_ids.py | 102 -------------------------- 3 files changed, 24 insertions(+), 103 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index abeff253b9..3e8ad3b7ac 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -218,6 +218,16 @@ Every command, template, and script contributed by a preset (or an extension, or - `kind` is one of `command`, `template`, or `script`. - `name` is the entry's declared `name` field. +`sourceId` is the source's own stable system identifier: + +| Layer | `sourceId` value | Where it comes from | +| --- | --- | --- | +| `core` | `_` (literal underscore) | Placeholder because core has no manifest id | +| `preset` | The preset pack's `id` | The manifest's `preset.id` field — the same value used by `PresetManifest.id`, the install directory, registries, and resolver layer metadata | +| `extension` | The extension's `id` | The manifest's `extension.id` field — the same value used by `ExtensionManifest.id`, the install directory, registries, and hook metadata | + +Preset contribution identifiers cover named artifacts (`command`, `template`, and `script`). Extension hooks are treated separately because their lookup name is derived from the hook event and command instead of a single `name` field. + Identifiers are computed on demand from author-declared manifest content and are never persisted to `.specify/` or any cache. Copying a preset to another machine (or touching its files) does not change the identifiers it produces. `PresetResolver.collect_all_layers()` returns layer dicts that each include a `lookupId` field pointing back to the originating contribution's `id`. Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they carry a synthetic `project:_:{kind}:{name}` `lookupId` that intentionally does not match any manifest contribution. diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index 5ed1b45f25..02c751b6f0 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -877,6 +877,8 @@ Named contributions (commands, templates, scripts) follow: - `kind` is one of `command`, `template`, `script`, or `hook`. - `name` is the contribution's declared `name` field. +`sourceId` is the source's own stable system identifier: `_` for the manifest-less core layer, `preset.id` from `preset.yml` for presets, and `extension.id` from `extension.yml` for extensions. It is the same string used elsewhere to refer to that preset or extension (install directories, registries, resolver metadata, and hook metadata), which makes identifiers stable join keys back to their originating source. + Hook contributions use a compound name-component built from the event and command: ```text @@ -895,7 +897,18 @@ Project-local overrides in `.specify/templates/overrides/` are a resolver-only c ### Python API -`ExtensionManifest.iter_contributions()` yields dicts of the form `{layer, sourceId, kind, name, id, ...author-declared fields}`; each entry's `id` is the computed identifier. `ExtensionManifest.contribution_id(kind, name)` returns the id for a single lookup, or `None` if no contribution matches. `PresetManifest` exposes the same two methods. +```python +class ExtensionManifest: + def iter_contributions(self) -> list[dict]: ... + def contribution_id(self, kind: str, name: str) -> str | None: ... + + +class PresetManifest: + def iter_contributions(self) -> list[dict]: ... + def contribution_id(self, kind: str, name: str) -> str | None: ... +``` + +Each contribution dict carries `{layer, sourceId, kind, name, id, ...author-declared fields}`; `id` is the computed identifier. `contribution_id(kind, name)` returns the id for a single lookup, or `None` if no contribution matches. `PresetResolver.collect_all_layers()` returns layer dicts that include a `lookupId` field for every layer type (`project override`, preset, extension, core, and bundled core). diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index 91243b6b68..cd0cda3b24 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -13,12 +13,6 @@ import copy import json -import os -import shutil -import subprocess -import sys -import textwrap -import time from pathlib import Path import pytest @@ -142,11 +136,6 @@ def test_named_id_grammar(self, layer, source_id, kind, name, expected): def test_hook_id_grammar(self, layer, source_id, event, command, expected): assert derive_hook_id(layer, source_id, event, command) == expected - def test_named_id_stable_across_two_derivations(self): - a = derive_named_id("preset", "speckit-core", "command", "speckit.plan") - b = derive_named_id("preset", "speckit-core", "command", "speckit.plan") - assert a == b - # --------------------------------------------------------------------------- # Duplicate hook behavior @@ -295,33 +284,6 @@ def test_contribution_id_lookup(self, tmp_path): ) assert manifest.contribution_id("command", "does-not-exist") is None - def test_representation_shape_is_additive_for_preset(self, tmp_path): - original = _preset_data() - manifest = PresetManifest(_write_manifest(tmp_path, original, "preset.yml")) - derived_keys = {"layer", "sourceId", "kind", "id"} - for src_entry, out_entry in zip(original["provides"]["templates"], manifest.iter_contributions()): - assert set(src_entry.keys()).issubset(out_entry.keys()) - assert derived_keys.issubset(out_entry.keys()) - - def test_representation_shape_is_additive_for_extension(self, tmp_path): - original = _extension_data( - hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} - ) - manifest = ExtensionManifest(_write_manifest(tmp_path, original, "extension.yml")) - entries = manifest.iter_contributions() - derived_named = {"layer", "sourceId", "kind", "id"} - - cmd_entry = original["provides"]["commands"][0] - cmd_out = next(e for e in entries if e["kind"] == "command") - assert set(cmd_entry.keys()).issubset(cmd_out.keys()) - assert derived_named.issubset(cmd_out.keys()) - - hook_entry = original["hooks"]["before_specify"] - hook_out = next(e for e in entries if e["kind"] == "hook") - assert set(hook_entry.keys()).issubset(hook_out.keys()) - assert derived_named.issubset(hook_out.keys()) - assert hook_out["name"] == "before_specify:speckit.speckitgit.branch" - def test_underlying_data_not_mutated(self, tmp_path): original = _preset_data() original_snapshot = copy.deepcopy(original) @@ -419,70 +381,6 @@ def test_preset_layer_lookup_id_matches_manifest_contribution_id(self, tmp_path) assert preset_layer["lookupId"] == f"preset:{pack_id}:template:spec-template" -# --------------------------------------------------------------------------- -# Determinism across environments -# --------------------------------------------------------------------------- - - -_SUBPROCESS_SCRIPT = textwrap.dedent( - """ - import sys, json - from specify_cli.extensions import ExtensionManifest - manifest = ExtensionManifest(sys.argv[1]) - ids = [c["id"] for c in manifest.iter_contributions()] - sys.stdout.write(json.dumps(ids)) - """ -) - - -class TestDeterminism: - def _fixture_manifest(self, tmp_path: Path) -> Path: - data = _extension_data( - hooks={ - "before_specify": {"command": "speckit.speckitgit.branch"}, - "before_plan": [ - {"command": "speckit.speckitgit.branch", "priority": 10}, - {"command": "speckit.speckitgit.branch", "priority": 20}, - ], - } - ) - return _write_manifest(tmp_path, data, "extension.yml") - - def test_identifiers_match_across_subprocesses(self, tmp_path): - manifest_path = self._fixture_manifest(tmp_path) - env = os.environ.copy() - env["PYTHONPATH"] = os.pathsep.join( - [str(Path(__file__).resolve().parent.parent / "src"), env.get("PYTHONPATH", "")] - ) - - def _run() -> str: - proc = subprocess.run( - [sys.executable, "-c", _SUBPROCESS_SCRIPT, str(manifest_path)], - capture_output=True, - text=True, - env=env, - check=True, - ) - return proc.stdout - - assert _run() == _run() - - def test_ids_independent_of_paths_and_mtimes(self, tmp_path): - original_dir = tmp_path / "orig" - copied_dir = tmp_path / "copy" - original_dir.mkdir() - manifest_path = self._fixture_manifest(original_dir) - original_ids = [c["id"] for c in ExtensionManifest(manifest_path).iter_contributions()] - - shutil.copytree(original_dir, copied_dir) - distant_past = time.time() - 3600 - os.utime(copied_dir / manifest_path.name, (distant_past, distant_past)) - copied_ids = [ - c["id"] for c in ExtensionManifest(copied_dir / manifest_path.name).iter_contributions() - ] - assert original_ids == copied_ids - - # --------------------------------------------------------------------------- # Identifiers never persisted # --------------------------------------------------------------------------- From 164534107017135eea830214354432c94dd5871a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:25:06 +0000 Subject: [PATCH 08/13] Align contribution ID test coverage summary Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- tests/test_contribution_ids.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index cd0cda3b24..65f61030bf 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -4,9 +4,9 @@ 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 -``layer x kind`` combination, duplicate hook collapse behavior, cross-process -byte-stability, path/mtime independence, and the additive-only shape guarantee -for the enriched contribution dicts. +``layer x kind`` combination, duplicate hook collapse behavior, component +validation, contribution lookup, resolver ``lookupId`` round-trips, and +non-persistence of computed identifiers. """ from __future__ import annotations From b0015cf269160453bceb54c46b0f9d81b4febb77 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:38:37 +0000 Subject: [PATCH 09/13] docs: clarify preset kind hook note (Assisted-by: GitHub Copilot, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- docs/reference/presets.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 3e8ad3b7ac..deea79cef7 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -215,7 +215,7 @@ Every command, template, and script contributed by a preset (or an extension, or - `layer` is one of `core`, `preset`, or `extension`. - `sourceId` is `_` for `core`, the preset pack id for `preset`, or the extension id for `extension`. -- `kind` is one of `command`, `template`, or `script`. +- `kind` is one of `command`, `template`, or `script` (see below for hooks). - `name` is the entry's declared `name` field. `sourceId` is the source's own stable system identifier: From 388a96885c925e3ba25943e9723996b1b10d9adc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:53:09 +0000 Subject: [PATCH 10/13] fix: use manifest ID for extension lookup IDs Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- src/specify_cli/presets/__init__.py | 34 ++++++++++++++++------------- tests/test_contribution_ids.py | 25 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index ac20f85c1b..40520e5cf2 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -5118,12 +5118,13 @@ def _manifest_declared_template( def _extension_manifest_declared_template( self, ext_dir: Path, template_name: str, template_type: str - ) -> tuple[dict | None, Path | None]: + ) -> tuple[dict | None, Path | None, str | None]: """Resolve an extension's manifest-declared command/template/script entry and usable file. - Mirrors ``_manifest_declared_template`` (for presets): returns ``(entry, candidate)`` - where ``entry`` is the matching ``provides.`` mapping, or ``None`` if the - extension has no (valid) manifest or doesn't declare this ``(name, type)``. + Mirrors ``_manifest_declared_template`` (for presets): returns + ``(entry, candidate, manifest_id)`` where ``entry`` is the matching + ``provides.`` mapping, or ``None`` if the extension has no (valid) + manifest or doesn't declare this ``(name, type)``. ``candidate`` is the declared ``file:`` resolved under ``ext_dir`` IFF it is a regular file that stays within ``ext_dir`` (guards against path traversal via a malformed manifest, mirroring ``resolve_extension_command_via_manifest``); @@ -5136,16 +5137,16 @@ def _extension_manifest_declared_template( diverge (the divergence flagged in review on #4012). """ if template_type not in ("command", "template", "script"): - return None, None + return None, None, None ext_manifest_path = ext_dir / "extension.yml" if not ext_manifest_path.exists(): - return None, None + return None, None, None from ..extensions import ExtensionManifest, ValidationError as ExtValidationError try: ext_manifest = ExtensionManifest(ext_manifest_path) except (ExtValidationError, yaml.YAMLError, OSError, TypeError, AttributeError): - return None, None + return None, None, None if template_type == "command": entries = ext_manifest.commands elif template_type == "template": @@ -5157,10 +5158,10 @@ def _extension_manifest_declared_template( continue file_rel = entry.get("file") if not file_rel: - return entry, None + return entry, None, ext_manifest.id rel_path = Path(file_rel) if rel_path.is_absolute(): - return entry, None + return entry, None, ext_manifest.id candidate = ext_dir / rel_path try: # Resolve only for the containment check, not for the @@ -5170,9 +5171,9 @@ def _extension_manifest_declared_template( # lookup returns for the same directory. candidate.resolve().relative_to(ext_dir.resolve()) # raises ValueError if outside except (OSError, ValueError): - return entry, None - return entry, (candidate if candidate.is_file() else None) - return None, None + return entry, None, ext_manifest.id + return entry, (candidate if candidate.is_file() else None), ext_manifest.id + return None, None, ext_manifest.id def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: """Build unified list of registered and unregistered extensions sorted by priority. @@ -5324,7 +5325,7 @@ def resolve( # The extension manifest is authoritative, same as preset manifests # above: check it before convention-based lookup so a declared entry # at a non-conventional path wins over a stale conventional file. - entry, manifest_candidate = self._extension_manifest_declared_template( + entry, manifest_candidate, _manifest_id = self._extension_manifest_declared_template( ext_dir, template_name, template_type ) if manifest_candidate is not None: @@ -5647,7 +5648,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: # above: check it before convention-based lookup so a declared entry # at a non-conventional path wins over a stale conventional file, and # a declared-but-missing file isn't silently masked by convention. - entry, candidate = self._extension_manifest_declared_template( + entry, candidate, manifest_id = self._extension_manifest_declared_template( ext_dir, template_name, template_type ) if entry is None: @@ -5665,7 +5666,10 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: "extension_id": ext_id, "extension_dir": ext_dir, "lookupId": derive_named_id( - "extension", ext_id, template_type, template_name + "extension", + manifest_id if entry is not None else ext_id, + template_type, + template_name, ), }) diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index 65f61030bf..7e231302ad 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -380,6 +380,31 @@ def test_preset_layer_lookup_id_matches_manifest_contribution_id(self, tmp_path) assert preset_layer["lookupId"] == manifest.contribution_id("template", "spec-template") assert preset_layer["lookupId"] == f"preset:{pack_id}:template:spec-template" + def test_extension_layer_uses_manifest_id_when_directory_name_differs(self, tmp_path): + project = _make_project(tmp_path) + ext_dir = project / ".specify" / "extensions" / "local-copy" + (ext_dir / "templates").mkdir(parents=True) + (ext_dir / "templates" / "pr-body.md").write_text("extension", encoding="utf-8") + manifest_path = _write_manifest( + ext_dir, + _extension_data( + ext_id="real-id", + with_commands=False, + with_scripts=False, + ), + "extension.yml", + ) + + resolver = PresetResolver(project) + layers = resolver.collect_all_layers("pr-body", "template") + extension_layer = next( + layer for layer in layers if layer["source"] == "extension:local-copy (unregistered)" + ) + manifest = ExtensionManifest(manifest_path) + + assert extension_layer["lookupId"] == manifest.contribution_id("template", "pr-body") + assert extension_layer["lookupId"] == "extension:real-id:template:pr-body" + # --------------------------------------------------------------------------- # Identifiers never persisted From 09089e28e8e19b33f0ede261f3a6efc4f12250e6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:54:07 +0000 Subject: [PATCH 11/13] refactor: omit unused extension manifest ID Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- src/specify_cli/presets/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 40520e5cf2..0aeab6e834 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -5173,7 +5173,7 @@ def _extension_manifest_declared_template( except (OSError, ValueError): return entry, None, ext_manifest.id return entry, (candidate if candidate.is_file() else None), ext_manifest.id - return None, None, ext_manifest.id + return None, None, None def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: """Build unified list of registered and unregistered extensions sorted by priority. From 3bd2b1d4c0141b46e49a1a5413d050b4d50507ac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:04:23 +0000 Subject: [PATCH 12/13] docs: clarify extension contribution source IDs Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- docs/reference/presets.md | 2 +- extensions/EXTENSION-API-REFERENCE.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index deea79cef7..5093a424ff 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -224,7 +224,7 @@ Every command, template, and script contributed by a preset (or an extension, or | --- | --- | --- | | `core` | `_` (literal underscore) | Placeholder because core has no manifest id | | `preset` | The preset pack's `id` | The manifest's `preset.id` field — the same value used by `PresetManifest.id`, the install directory, registries, and resolver layer metadata | -| `extension` | The extension's `id` | The manifest's `extension.id` field — the same value used by `ExtensionManifest.id`, the install directory, registries, and hook metadata | +| `extension` | The extension's `id` | The manifest's `extension.id` field — the value used by `ExtensionManifest.id` and manifest-backed `lookupId` values; the extension directory or registry key may differ for unregistered local copies | Preset contribution identifiers cover named artifacts (`command`, `template`, and `script`). Extension hooks are treated separately because their lookup name is derived from the hook event and command instead of a single `name` field. diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index 02c751b6f0..66617d0e9c 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -877,7 +877,7 @@ Named contributions (commands, templates, scripts) follow: - `kind` is one of `command`, `template`, `script`, or `hook`. - `name` is the contribution's declared `name` field. -`sourceId` is the source's own stable system identifier: `_` for the manifest-less core layer, `preset.id` from `preset.yml` for presets, and `extension.id` from `extension.yml` for extensions. It is the same string used elsewhere to refer to that preset or extension (install directories, registries, resolver metadata, and hook metadata), which makes identifiers stable join keys back to their originating source. +`sourceId` is the source's own stable manifest identifier: `_` for the manifest-less core layer, `preset.id` from `preset.yml` for presets, and `extension.id` from `extension.yml` for extensions. Manifest-backed extension contributions and resolver `lookupId` values use `extension.id` even when an unregistered local copy has a different directory or registry key, which makes identifiers stable join keys back to their originating manifest. Hook contributions use a compound name-component built from the event and command: From d1dac36980e041b5c3902012a130d48125f52e6c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:22:19 +0000 Subject: [PATCH 13/13] fix: validate extension command collisions on load Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 129 +++++++++++++------------ tests/test_contribution_ids.py | 22 ++--- tests/test_extensions.py | 45 +++++++++ 3 files changed, 124 insertions(+), 72 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 5c3dbace3d..34a27bef7c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -240,6 +240,71 @@ def collapse_hook_event_entries(event_name: str, hook_config: Any) -> List[Dict[ return list(collapsed.values()) +def _collect_extension_command_names( + extension_id: str, commands: List[Dict[str, Any]] +) -> Dict[str, str]: + """Collect and validate command and alias names declared by a manifest.""" + if extension_id in CORE_COMMAND_NAMES: + raise ValidationError( + f"Extension ID '{extension_id}' conflicts with core command namespace '{extension_id}'" + ) + + declared_names: Dict[str, str] = {} + + for cmd in commands: + primary_name = cmd["name"] + aliases = cmd.get("aliases", []) + + if aliases is None: + aliases = [] + if not isinstance(aliases, list): + raise ValidationError( + f"Aliases for command '{primary_name}' must be a list" + ) + + for kind, name in [("command", primary_name)] + [ + ("alias", alias) for alias in aliases + ]: + if not isinstance(name, str): + raise ValidationError( + f"{kind.capitalize()} for command '{primary_name}' must be a string" + ) + + path_reason = relative_extension_path_violation(name) + if path_reason: + raise ValidationError(f"Invalid {kind} {name!r}: {path_reason}") + + # Enforce canonical pattern only for primary command names; + # aliases are free-form to preserve community extension compat. + if kind == "command": + match = EXTENSION_COMMAND_NAME_PATTERN.match(name) + if match is None: + raise ValidationError( + f"Invalid {kind} '{name}': " + "must follow pattern 'speckit.{extension}.{command}'" + ) + + namespace = match.group(1) + if namespace != extension_id: + raise ValidationError( + f"{kind.capitalize()} '{name}' must use extension namespace '{extension_id}'" + ) + + if namespace in CORE_COMMAND_NAMES: + raise ValidationError( + f"{kind.capitalize()} '{name}' conflicts with core command namespace '{namespace}'" + ) + + if name in declared_names: + raise ValidationError( + f"Duplicate command or alias '{name}' in extension manifest" + ) + + declared_names[name] = kind + + return declared_names + + @dataclass class CatalogEntry(BaseCatalogEntry): """Represents a single catalog entry in the catalog stack.""" @@ -559,6 +624,8 @@ def _validate(self): f"'{cmd['name']}': {alias_reason}" ) + _collect_extension_command_names(ext["id"], commands) + # Rewrite any hook command references that pointed at a renamed command or # an alias-form ref (ext.cmd → speckit.ext.cmd). Always emit a warning when # the reference is changed so extension authors know to update the manifest. @@ -1235,67 +1302,7 @@ def _collect_manifest_command_names(manifest: ExtensionManifest) -> Dict[str, st Raises: ValidationError: If any declared name is invalid """ - if manifest.id in CORE_COMMAND_NAMES: - raise ValidationError( - f"Extension ID '{manifest.id}' conflicts with core command namespace '{manifest.id}'" - ) - - declared_names: Dict[str, str] = {} - - for cmd in manifest.commands: - primary_name = cmd["name"] - aliases = cmd.get("aliases", []) - - if aliases is None: - aliases = [] - if not isinstance(aliases, list): - raise ValidationError( - f"Aliases for command '{primary_name}' must be a list" - ) - - for kind, name in [("command", primary_name)] + [ - ("alias", alias) for alias in aliases - ]: - if not isinstance(name, str): - raise ValidationError( - f"{kind.capitalize()} for command '{primary_name}' must be a string" - ) - - path_reason = relative_extension_path_violation(name) - if path_reason: - raise ValidationError( - f"Invalid {kind} {name!r}: {path_reason}" - ) - - # Enforce canonical pattern only for primary command names; - # aliases are free-form to preserve community extension compat. - if kind == "command": - match = EXTENSION_COMMAND_NAME_PATTERN.match(name) - if match is None: - raise ValidationError( - f"Invalid {kind} '{name}': " - "must follow pattern 'speckit.{extension}.{command}'" - ) - - namespace = match.group(1) - if namespace != manifest.id: - raise ValidationError( - f"{kind.capitalize()} '{name}' must use extension namespace '{manifest.id}'" - ) - - if namespace in CORE_COMMAND_NAMES: - raise ValidationError( - f"{kind.capitalize()} '{name}' conflicts with core command namespace '{namespace}'" - ) - - if name in declared_names: - raise ValidationError( - f"Duplicate command or alias '{name}' in extension manifest" - ) - - declared_names[name] = kind - - return declared_names + return _collect_extension_command_names(manifest.id, manifest.commands) def _get_installed_command_name_map( self, diff --git a/tests/test_contribution_ids.py b/tests/test_contribution_ids.py index 7e231302ad..52098d2980 100644 --- a/tests/test_contribution_ids.py +++ b/tests/test_contribution_ids.py @@ -75,7 +75,7 @@ def _extension_data( if with_commands: data["provides"]["commands"] = [ { - "name": f"speckit.{ext_id.replace('-', '')}.branch", + "name": f"speckit.{ext_id}.branch", "file": "commands/branch.md", "description": "Fixture command", } @@ -147,8 +147,8 @@ def test_last_duplicate_hook_is_the_only_contribution(self, tmp_path): data = _extension_data( hooks={ "before_plan": [ - {"command": "speckit.speckitgit.branch", "priority": 10}, - {"command": "speckit.speckitgit.branch", "priority": 20}, + {"command": "speckit.speckit-git.branch", "priority": 10}, + {"command": "speckit.speckit-git.branch", "priority": 20}, ] } ) @@ -156,10 +156,10 @@ def test_last_duplicate_hook_is_the_only_contribution(self, tmp_path): hooks = [c for c in manifest.iter_contributions() if c["kind"] == "hook"] assert len(hooks) == 1 assert hooks[0]["priority"] == 20 - assert hooks[0]["id"] == "extension:speckit-git:hook:before_plan:speckit.speckitgit.branch" + assert hooks[0]["id"] == "extension:speckit-git:hook:before_plan:speckit.speckit-git.branch" assert ( manifest.contribution_id( - "hook", "before_plan:speckit.speckitgit.branch" + "hook", "before_plan:speckit.speckit-git.branch" ) == hooks[0]["id"] ) @@ -207,7 +207,7 @@ def test_validate_component_rejects_non_string(self): def test_extension_hook_event_name_with_colon_rejected(self, tmp_path): data = _extension_data( - hooks={"before:plan": {"command": "speckit.speckitgit.branch"}} + hooks={"before:plan": {"command": "speckit.speckit-git.branch"}} ) with pytest.raises(ValidationError) as exc_info: ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) @@ -265,16 +265,16 @@ def test_preset_iter_contributions_matrix(self, tmp_path): def test_extension_iter_contributions_matrix(self, tmp_path): data = _extension_data( - hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} + hooks={"before_specify": {"command": "speckit.speckit-git.branch"}} ) manifest = ExtensionManifest(_write_manifest(tmp_path, data, "extension.yml")) entries = manifest.iter_contributions() kinds = {e["kind"]: e for e in entries} - assert kinds["command"]["id"] == "extension:speckit-git:command:speckit.speckitgit.branch" + assert kinds["command"]["id"] == "extension:speckit-git:command:speckit.speckit-git.branch" assert kinds["template"]["id"] == "extension:speckit-git:template:pr-body" assert kinds["script"]["id"] == "extension:speckit-git:script:post-commit" - assert kinds["hook"]["id"] == "extension:speckit-git:hook:before_specify:speckit.speckitgit.branch" - assert kinds["hook"]["name"] == "before_specify:speckit.speckitgit.branch" + assert kinds["hook"]["id"] == "extension:speckit-git:hook:before_specify:speckit.speckit-git.branch" + assert kinds["hook"]["name"] == "before_specify:speckit.speckit-git.branch" def test_contribution_id_lookup(self, tmp_path): manifest = PresetManifest(_write_manifest(tmp_path, _preset_data(), "preset.yml")) @@ -414,7 +414,7 @@ def test_extension_layer_uses_manifest_id_when_directory_name_differs(self, tmp_ class TestNoPersistence: def test_no_id_written_to_manifest_files(self, tmp_path): data = _extension_data( - hooks={"before_specify": {"command": "speckit.speckitgit.branch"}} + hooks={"before_specify": {"command": "speckit.speckit-git.branch"}} ) manifest_path = _write_manifest(tmp_path, data, "extension.yml") # Read identifiers to force the derivation code path. diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 6642da2b09..d1c2df8eb7 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -635,6 +635,51 @@ def test_alias_free_form_accepted(self, temp_dir, valid_manifest_data): assert manifest.commands[0]["aliases"] == ["speckit.hello"] assert manifest.warnings == [] + def test_duplicate_primary_command_rejected_at_manifest_load( + self, temp_dir, valid_manifest_data + ): + """Duplicate primary command names are rejected before install validation.""" + import yaml + + valid_manifest_data["provides"]["commands"].append( + { + "name": "speckit.test-ext.hello", + "file": "commands/hello-again.md", + } + ) + manifest_path = temp_dir / "extension.yml" + manifest_path.write_text(yaml.safe_dump(valid_manifest_data)) + + with pytest.raises( + ValidationError, + match="Duplicate command or alias 'speckit.test-ext.hello'", + ): + ExtensionManifest(manifest_path) + + def test_primary_command_alias_collision_rejected_at_manifest_load( + self, temp_dir, valid_manifest_data + ): + """A primary name cannot duplicate an alias from another command.""" + import yaml + + valid_manifest_data["provides"]["commands"][0]["aliases"] = [ + "speckit.test-ext.goodbye" + ] + valid_manifest_data["provides"]["commands"].append( + { + "name": "speckit.test-ext.goodbye", + "file": "commands/goodbye.md", + } + ) + manifest_path = temp_dir / "extension.yml" + manifest_path.write_text(yaml.safe_dump(valid_manifest_data)) + + with pytest.raises( + ValidationError, + match="Duplicate command or alias 'speckit.test-ext.goodbye'", + ): + ExtensionManifest(manifest_path) + def test_valid_command_name_has_no_warnings(self, temp_dir, valid_manifest_data): """Test that a correctly-named command produces no warnings.""" import yaml