fix(artifacts): enforce identifier grammar and gate manifestPath on declared contributions

Three related correctness fixes for the artifact-stack pipeline surfaced during PR #4305 review:

1. _identifier.py: add source_id_from_lookup_id() helper mirroring layer_kind_from_lookup_id, and enforce the project/'_' sentinel in derive_named_id (project layer requires source_id == '_'; preset/extension layers reject '_'). Swap the split(':', 2)[1] call in artifacts/__init__.py to use the new helper so consumers no longer parse identifier grammar directly.

2. presets/__init__.py: thread a manifest_declared flag through collect_all_layers so downstream consumers can distinguish manifest-declared contributions from convention-only fallbacks.

3. artifacts/__init__.py: _derive_manifest_path returns None when the layer is not manifest-declared, so a stack row for a convention-only contribution no longer falsely reports a manifestPath pointing at a manifest that does not declare it.

Tests: compact param-based coverage for source_id_from_lookup_id and derive_named_id sentinel rules; one preset + one extension test proving lookupId uses the manifest's validated id when it differs from the on-disk directory name; one end-to-end extension test proving a convention-only contribution reports manifestPath: null. Existing TestManifestPathPortability fixtures updated to set manifest_declared: True.

Assisted-by: GitHub Copilot (model: claude-opus-4.7, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: nicolehaugen

src/specify_cli/_identifier.py

@@ -95,6 +95,15 @@ def derive_named_id(layer: str, source_id: str, kind: str, name: str) -> str:
     derivation boundary that must enforce the grammar. Callers passing raw
     strings should either pre-validate or handle
     :class:`IdentifierComponentError`.
+
+    The project-layer sentinel is also enforced here: the project layer uses
+    ``sourceId == "_"`` (see :data:`PROJECT_OVERRIDE_LAYER`) and no other
+    value; the preset and extension layers never use ``"_"``, which is
+    reserved for the project layer. Callers that mix these up would produce a
+    lookupId :func:`layer_kind_from_lookup_id` still parses but that no
+    manifest ever emits — a silent join-key mismatch. Rejecting the mix here
+    keeps the grammar's sentinel contract enforced at the single derivation
+    boundary rather than in each caller.
     """
     validate_component(layer, "layer")
     validate_component(source_id, "sourceId")
@@ -104,6 +113,14 @@ def derive_named_id(layer: str, source_id: str, kind: str, name: str) -> str:
     if kind not in _NAMED_CONTRIBUTION_KINDS:
         raise IdentifierComponentError(f"Invalid named contribution kind '{kind}'")
     validate_component(name, "name")
+    if layer == PROJECT_OVERRIDE_LAYER and source_id != "_":
+        raise IdentifierComponentError(
+            f"Invalid sourceId '{source_id}': project layer requires '_'"
+        )
+    if layer in {"preset", "extension"} and source_id == "_":
+        raise IdentifierComponentError(
+            "Invalid sourceId '_': reserved for project layer"
+        )
     return f"{layer}:{source_id}:{kind}:{name}"
 
 
@@ -171,6 +188,20 @@ def is_dotted_command_name(value: str) -> bool:
     )
 
 
+def source_id_from_lookup_id(lookup_id: str) -> str | None:
+    """Return the sourceId segment of a resolved-stack ``lookupId``, or ``None``.
+
+    Returns ``None`` for any value that :func:`layer_kind_from_lookup_id`
+    would reject — same validation, same grammar, single source of truth.
+    Consumers must not ``.split(":")`` a ``lookupId`` themselves: the
+    grammar's segmentation lives in this module, and any caller doing its
+    own split leaks the layout across the codebase.
+    """
+    if layer_kind_from_lookup_id(lookup_id) is None:
+        return None
+    return lookup_id.split(":", 2)[1]
+
+
 def derive_hook_id(
     layer: str,
     source_id: str,

src/specify_cli/artifacts/__init__.py

@@ -25,6 +25,7 @@
     derive_public_id,
     is_dotted_command_name,
     layer_kind_from_lookup_id,
+    source_id_from_lookup_id,
     validate_component,
 )
 from .._script_variants import canonical_script_name
@@ -261,7 +262,10 @@ def _public_layer_shape(
     layer_kind = layer_kind_from_lookup_id(lookup_id)
     if layer_kind not in ("project", "preset", "extension"):
         raise ArtifactResolutionError()
-    return layer_kind, lookup_id.split(":", 2)[1], lookup_id
+    source_id = source_id_from_lookup_id(lookup_id)
+    if source_id is None:
+        raise ArtifactResolutionError()
+    return layer_kind, source_id, lookup_id
 
 
 def _derive_manifest_path(layer: dict[str, Any], project_root: Path) -> str | None:
@@ -280,10 +284,22 @@ def _derive_manifest_path(layer: dict[str, Any], project_root: Path) -> str | No
     layers — which ``collect_all_layers()`` always sets alongside
     ``lookupId``. Missing provenance keys mean no manifest path is available.
 
+    Convention-only contributions are surfaced by the resolver even when the
+    pack's manifest does not declare them — the manifest file exists on disk
+    but does not list the artifact in ``provides``. Reporting the manifest
+    path in that case would be a false positive: consumers joining on the
+    reported path would find no matching contribution. ``collect_all_layers``
+    sets ``manifest_declared=True`` on layers that came from a manifest
+    ``provides`` entry, so those layers alone report a manifest path; a layer
+    without that flag falls through to ``None`` even when the manifest file
+    exists on disk.
+
     Uses ``as_posix()`` so the string is stable across Windows and POSIX — a
     caller comparing snapshots between operating systems gets the same value
     on both.
     """
+    if not layer.get("manifest_declared"):
+        return None
     lookup_id = layer.get("lookupId", "")
     layer_kind = layer_kind_from_lookup_id(lookup_id)
     if layer_kind == "preset":

src/specify_cli/presets/__init__.py

@@ -5997,6 +5997,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
                         "strategy": strategy,
                         "preset_id": pack_id,
                         "pack_dir": pack_dir,
+                        "manifest_declared": entry is not None,
                         "lookupId": derive_named_id(
                             "preset", source_id_for_lookup, template_type, template_name
                         ),
@@ -6059,6 +6060,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
                     "strategy": "replace",
                     "extension_id": ext_id,
                     "extension_dir": ext_dir,
+                    "manifest_declared": entry is not None,
                     "lookupId": derive_named_id(
                         "extension", source_id_for_lookup, template_type, template_name
                     ),

tests/test_artifact_command.py

@@ -1344,6 +1344,7 @@ def test_preset_manifest_path_is_repo_relative(self, tmp_path: Path):
             "path": pack_dir / "spec-template.md",
             "preset_id": "my-pack",
             "pack_dir": pack_dir,
+            "manifest_declared": True,
         }
         assert (
             _derive_manifest_path(layer, project_root)
@@ -1361,6 +1362,7 @@ def test_extension_manifest_path_is_repo_relative(self, tmp_path: Path):
             "path": ext_dir / "commands" / "speckit.my-ext.go.md",
             "extension_id": "my-ext",
             "extension_dir": ext_dir,
+            "manifest_declared": True,
         }
         assert (
             _derive_manifest_path(layer, project_root)
@@ -1384,6 +1386,7 @@ def test_renamed_pack_directory_wins_over_lookup_id_source(self, tmp_path: Path)
             "path": pack_dir / "spec-template.md",
             "preset_id": "renamed-on-disk",
             "pack_dir": pack_dir,
+            "manifest_declared": True,
         }
         assert (
             _derive_manifest_path(layer, project_root)
@@ -1400,6 +1403,7 @@ def test_missing_manifest_file_is_none(self, tmp_path: Path):
             "path": pack_dir / "spec-template.md",
             "preset_id": "my-pack",
             "pack_dir": pack_dir,
+            "manifest_declared": True,
         }
         assert _derive_manifest_path(layer, project_root) is None
 
@@ -1414,6 +1418,7 @@ def test_missing_provenance_keys_is_none(self, tmp_path: Path):
         layer = {
             "lookupId": "preset:my-pack:template:spec-template",
             "path": pack_dir / "spec-template.md",
+            "manifest_declared": True,
         }
         assert _derive_manifest_path(layer, project_root) is None
 
@@ -1426,6 +1431,30 @@ def test_builtin_and_project_layers_have_no_manifest(self, tmp_path: Path):
         assert _derive_manifest_path(builtin_layer, project_root) is None
         assert _derive_manifest_path(project_layer, project_root) is None
 
+    def test_convention_only_extension_layer_reports_no_manifest_path(
+        self, tmp_path: Path
+    ):
+        """A contribution the manifest does not declare in ``provides`` — a
+        "convention-only" contribution — must NOT report the manifest as its
+        source, even when the manifest file exists on disk. Joining on the
+        reported path would find no matching contribution. One extension test
+        covers both the preset and extension branches: ``_derive_manifest_path``
+        gates on the layer's ``manifest_declared`` flag before dispatching by
+        layer kind."""
+        project_root = tmp_path / "proj"
+        ext_dir = project_root / ".specify" / "extensions" / "foo"
+        ext_dir.mkdir(parents=True)
+        (ext_dir / "extension.yml").write_text("id: foo\n", encoding="utf-8")
+
+        layer = {
+            "lookupId": "extension:foo:command:speckit.baz",
+            "path": ext_dir / "commands" / "baz.md",
+            "extension_id": "foo",
+            "extension_dir": ext_dir,
+            "manifest_declared": False,
+        }
+        assert _derive_manifest_path(layer, project_root) is None
+
 
 class TestPresetDisplayName:
     """`_preset_display_name` delegates to the validated `PresetManifest.name`."""

tests/test_contribution_ids.py

@@ -31,6 +31,7 @@
     derive_named_id,
     derive_public_id,
     layer_kind_from_lookup_id,
+    source_id_from_lookup_id,
     validate_component,
 )
 from specify_cli.extensions import ExtensionManifest, ValidationError
@@ -529,3 +530,115 @@ 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
+
+
+# ---------------------------------------------------------------------------
+# `_identifier.py` review-round nits — sourceId accessor + derive_named_id
+# sentinel enforcement. Consumers must not ``.split(":")`` a lookupId
+# themselves, and the ``project``/``_`` pairing is enforced at the single
+# derivation boundary rather than at each caller.
+# ---------------------------------------------------------------------------
+
+
+class TestSourceIdFromLookupId:
+    @pytest.mark.parametrize(
+        "lookup_id, expected",
+        [
+            ("preset:speckit-core:command:speckit.plan", "speckit-core"),
+            (
+                "extension:speckit-git:hook:before_specify:speckit.git.branch",
+                "speckit-git",
+            ),
+            ("", None),
+            ("preset:foo", None),
+            ("unknown:foo:command:bar", None),
+            ("project:_:hook:evt:cmd", None),
+        ],
+    )
+    def test_extracts_source_id_or_none(self, lookup_id, expected):
+        assert source_id_from_lookup_id(lookup_id) == expected
+
+
+class TestDeriveNamedIdSentinel:
+    @pytest.mark.parametrize(
+        "layer, source_id",
+        [
+            (PROJECT_OVERRIDE_LAYER, "other"),
+            ("preset", "_"),
+            ("extension", "_"),
+        ],
+    )
+    def test_rejects_invalid_layer_source_pairs(self, layer, source_id):
+        with pytest.raises(IdentifierComponentError):
+            derive_named_id(layer, source_id, "command", "n")
+
+    def test_project_layer_accepts_underscore_source(self):
+        assert (
+            derive_named_id(PROJECT_OVERRIDE_LAYER, "_", "command", "n")
+            == f"{PROJECT_OVERRIDE_LAYER}:_:command:n"
+        )
+
+
+# ---------------------------------------------------------------------------
+# Manifest-declared id wins over installed-directory name — one preset test
+# and one extension test because the two branches of ``collect_all_layers``
+# could diverge independently. Each proves ``lookupId`` on the resolved layer
+# equals the manifest contribution ``id``.
+# ---------------------------------------------------------------------------
+
+
+def _write_registry(project: Path, tier: str, pack_id: str) -> None:
+    registry = {
+        "schema_version": "1.0",
+        tier: {pack_id: {"version": "1.0.0", "priority": 10, "enabled": True}},
+    }
+    (project / ".specify" / tier / ".registry").write_text(
+        json.dumps(registry), encoding="utf-8"
+    )
+
+
+class TestManifestIdWinsOverDirectoryName:
+    def test_preset_lookup_id_uses_manifest_id_when_directory_renamed(self, tmp_path):
+        project = _make_project(tmp_path)
+        dir_name, manifest_id = "renamed-preset", "original-preset"
+        pack_dir = project / ".specify" / "presets" / dir_name
+        (pack_dir / "templates").mkdir(parents=True)
+        (pack_dir / "templates" / "spec-template.md").write_text("p", encoding="utf-8")
+        data = _preset_data(manifest_id)
+        data["provides"] = {
+            "templates": [
+                {"type": "template", "name": "spec-template", "file": "templates/spec-template.md"}
+            ]
+        }
+        _write_manifest(pack_dir, data, "preset.yml")
+        _write_registry(project, "presets", dir_name)
+
+        layers = PresetResolver(project).collect_all_layers("spec-template", "template")
+        layer = next(L for L in layers if L["source"].startswith(dir_name))
+        manifest = PresetManifest(pack_dir / "preset.yml")
+        assert layer["lookupId"] == manifest.contribution_id("template", "spec-template")
+        assert layer["lookupId"] == f"preset:{manifest_id}:template:spec-template"
+
+    def test_extension_lookup_id_uses_manifest_id_when_directory_renamed(self, tmp_path):
+        project = _make_project(tmp_path)
+        dir_name, manifest_id = "renamed-ext", "original-ext"
+        # Extension commands are auto-namespaced under speckit.<manifest_id>
+        namespaced = f"speckit.{manifest_id}.branch"
+        ext_dir = project / ".specify" / "extensions" / dir_name
+        (ext_dir / "commands").mkdir(parents=True)
+        (ext_dir / "commands" / "branch.md").write_text("e", encoding="utf-8")
+        data = _extension_data(manifest_id, with_templates=False, with_scripts=False)
+        data["provides"]["commands"] = [
+            {"name": "speckit.branch", "file": "commands/branch.md", "description": "F"}
+        ]
+        _write_manifest(ext_dir, data, "extension.yml")
+        _write_registry(project, "extensions", dir_name)
+
+        layers = PresetResolver(project).collect_all_layers(namespaced, "command")
+        layer = next(L for L in layers if L.get("extension_id") == dir_name)
+        manifest = ExtensionManifest(ext_dir / "extension.yml")
+        assert layer["lookupId"] == manifest.contribution_id("command", namespaced)
+        assert layer["lookupId"] == f"extension:{manifest_id}:command:{namespaced}"
+
+
+