Add deterministic contribution IDs and stack lookup IDs for resolved artifacts - #4261
Add deterministic contribution IDs and stack lookup IDs for resolved artifacts#4261nicolehaugen wants to merge 1 commit into
Conversation
…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)
There was a problem hiding this comment.
Pull request overview
Adds deterministic IDs for manifest contributions and lookup IDs for resolved artifact layers.
Changes:
- Adds identifier derivation and hook discrimination.
- Enriches preset/extension contribution and resolver APIs.
- Adds tests and identifier documentation.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_identifier.py |
Implements identifier derivation. |
src/specify_cli/extensions/__init__.py |
Adds extension contribution IDs and validation. |
src/specify_cli/presets/__init__.py |
Adds preset IDs and resolver lookup IDs. |
tests/test_contribution_ids.py |
Tests determinism and lookup behavior. |
extensions/EXTENSION-API-REFERENCE.md |
Documents the identifier contract. |
docs/reference/presets.md |
Documents preset contribution IDs. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
extensions/EXTENSION-API-REFERENCE.md:917
- The previous
## File System Layoutheading was removed when this section was inserted, so the existing tree below is now an orphaned code block under “Opacity guidance.” Restore the heading before the tree to preserve the document structure.
- Files reviewed: 6/6 changed files
- Comments generated: 7
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
extensions/EXTENSION-API-REFERENCE.md:918
- The previous
File System Layoutheading was replaced when this section was inserted, leaving the directory tree below as an orphaned code block. Restore the heading before the tree.
src/specify_cli/extensions/init.py:467
- Duplicate detection groups the raw hook command before the later canonicalization pass. An event containing both
test-ext.helloandspeckit.test-ext.hellowith identical remaining fields passes this check, then both references are rewritten to the same command anditer_contributions()emits the same discriminator and ID for both. Group using the same canonical command form so normalization cannot create duplicate IDs.
)
if "name" not in cmd or "file" not in cmd:
raise ValidationError("Command missing 'name' or 'file'")
src/specify_cli/presets/init.py:5628
- When
entry is None,candidatecan be a conventionally discovered preset file rather than a manifest contribution. This still emits a manifest-shapedpreset:...ID, butPresetManifest.contribution_id()returnsNonefor it, so the documented lookup round trip fails and project overrides are no longer the only non-matching layer. Represent and document undeclared preset fallbacks explicitly, as is attempted for extensions, or expose a corresponding synthetic contribution.
"source": f"{pack_id} v{version}",
"strategy": strategy,
"lookupId": derive_named_id(
src/specify_cli/extensions/init.py:879
- For colliding hooks, every entry has the same synthesized
name(eventName:command), so this method always returns the first ID and provides no way to retrieve any discriminator-suffixed sibling. Make the hook lookup unambiguous—for example by accepting discriminator/declared fields or returning all matching IDs—instead of presenting this as a single-contribution lookup.
``name`` is the declared name for command/template/script kinds, or the
``"{eventName}:{command}"`` compound for hook kinds.
extensions/EXTENSION-API-REFERENCE.md:914
- These helpers construct identifiers; neither parses one. Directing consumers to use them for parsing is therefore not actionable and contradicts the opacity guidance. Tell consumers not to parse IDs and to retain/use the structured contribution fields instead.
Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Construct and compare them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than assembling or parsing them by string-splitting on `:` — the discriminator suffix and future grammar extensions may otherwise catch you out.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/extensions/init.py:883
- Valid colliding hooks have the same
kindand synthesizednameand differ only by discriminator, so this returns whichever hook appears first. Reordering those hooks changes this method's result and the other hook's ID is unreachable through this lookup API, despite the discriminator being introduced to distinguish both. Make ambiguous hook lookup explicit, such as by accepting discriminator/declared fields, looking up a full ID, or rejecting multiple matches.
matches = []
for entry in self.iter_contributions():
if entry["kind"] == kind and entry.get("name") == name:
src/specify_cli/presets/init.py:5663
ext_idhere is the registry/directory key, not necessarily the loaded manifest'sextension.id._get_all_extensions_by_priority()explicitly admits unregistered directories, while_extension_manifest_declared_template()discards the manifest object after returning the entry. Thus an unregistered directoryfolder/whose valid manifest declaresid: actualgetsextension:folder:..., butExtensionManifest.contribution_id()returnsextension:actual:..., breaking the required round-trip. Carry the manifest source ID through this resolution path, or reject/treat mismatched directory IDs as non-manifest fallbacks.
"lookupId": derive_named_id(
"extension" if entry is not None else EXTENSION_FALLBACK_LAYER,
ext_id,
template_type,
template_name,
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/specify_cli/presets/init.py:5663
- For a manifest-backed extension layer, the source component must come from
ExtensionManifest.id, notext_id, which here is the installation directory/registry key. Unregistered extension directories are explicitly supported, and a directory rename leaves the manifest ID unchanged but changes thislookupId; it then no longer equalsExtensionManifest.contribution_id()and violates the documented directory-move stability guarantee. Return the loaded manifest ID from_extension_manifest_declared_template()and use it whenentryis present.
"lookupId": derive_named_id(
"extension" if entry is not None else EXTENSION_FALLBACK_LAYER,
ext_id,
template_type,
template_name,
extensions/EXTENSION-API-REFERENCE.md:865
- This statement is false for convention-discovered preset and extension layers: the resolver now emits
preset-convention:...andextension-fallback:..., neither of which points to a manifest contribution. Qualify the round-trip guarantee to manifest-backed layers and document both resolver-only sentinel grammars alongsideproject:; otherwise consumers following the documentedcore|preset|extensiongrammar cannot interpret actualcollect_all_layers()output.
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.
docs/reference/presets.md:223
- This round-trip claim omits convention fallbacks.
collect_all_layers()now emitspreset-convention:{sourceId}:{kind}:{name}andextension-fallback:{sourceId}:{kind}:{name}for undeclared files, and those intentionally match no contribution just like the project sentinel. Document these two outcomes here so the preset reference matches the resolver output.
`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.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/extensions/init.py:803
ExtensionManifeststill accepts multipleprovides.commandsentries with the same canonical name, so this loop emits the same supposedly unique ID for each entry andcontribution_id()silently returns the first one. The duplicate-name check only runs later inExtensionManager._collect_manifest_command_names()(lines 1242-1316), which does not protect this new public manifest API. Reject duplicate canonical command names during manifest validation, as is already done for preset entries and extension templates/scripts.
for cmd in self.commands:
enriched = dict(cmd)
name = cmd.get("name", "")
enriched.update(
layer="extension",
src/specify_cli/presets/init.py:5663
- For supported unregistered extension directories,
ext_idis the directory name, not necessarily the loaded manifest'sextension.id._extension_manifest_declared_template()can therefore return a manifest-backed entry whose contribution ID uses the manifest ID, while thislookupIduses the directory ID and cannot round-trip. Propagate the loaded manifest ID from the helper and use it assourceIdwheneverentryis present.
"path": candidate,
"source": source,
"strategy": "replace",
"extension_id": ext_id,
"extension_dir": ext_dir,
extensions/EXTENSION-API-REFERENCE.md:918
- The existing
## File System Layoutheading was removed when this section was inserted, leaving the following.specify/tree attached to “Opacity guidance” and eliminating the layout section. Restore the heading before the code block.
```text
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:5632
- This derives a manifest-backed lookup ID from the registry/directory key rather than the preset manifest's own
id. Because_manifest_declared_template()does not require those values to match, a renamed/mismatched installed directory can producepreset:<registry-id>:...whilePresetManifest.contribution_id()producespreset:<manifest-id>:..., breaking the promised exact round-trip and directory-move stability. Return/use the matched manifest's source ID (or contribution ID) here and cover the mismatched-directory case.
This issue also appears on line 5664 of the same file.
"lookupId": derive_named_id(
lookup_layer, pack_id, template_type, template_name
),
src/specify_cli/presets/init.py:5666
- This uses
ext_idfrom the registry/directory scan, not the matchedExtensionManifest.id._extension_manifest_declared_template()accepts a valid manifest without checking those IDs match, so an unregistered extension placed in a renamed directory gets alookupIdthat cannot equal that manifest'scontribution_id()and changes when the directory moves. Propagate the actual manifest source/contribution ID from the helper for declared entries and add a renamed-directory round-trip test.
"lookupId": derive_named_id(
lookup_layer, ext_id, template_type, template_name
),
extensions/EXTENSION-API-REFERENCE.md:914
- The previous
## File System Layoutheading was replaced by this new section but never reinserted, leaving the existing.specify/tree below as unheaded content under Contribution Identifiers. Restore the heading before that code block.
### Opacity guidance
Identifiers are stable, but treat them as **opaque strings** in stored data (registries, cache files, external tooling). Construct and compare them with the helpers in `specify_cli._identifier` (`derive_named_id`, `derive_hook_id`) rather than assembling or parsing them by string-splitting on `:` — the discriminator suffix and future grammar extensions may otherwise catch you out.
extensions/EXTENSION-API-REFERENCE.md:904
- This return contract omits the valid ambiguous-hook case: when multiple discriminated hooks share the requested
{eventName}:{command}name,ExtensionManifest.contribution_id()raisesValueErrorrather than returning an ID orNone. Document that exception so API consumers do not treat every valid lookup as nullable-only.
`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.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/specify_cli/extensions/init.py:885
ExtensionManifest._validate()does not reject duplicate primary command names; that check currently occurs only later inExtensionManager._collect_manifest_command_names(). Therefore a directly loaded or convention-discovered manifest can reach this loop with two commands having the same name, and both receive the same supposedly addressable contribution ID. Reject duplicate normalized command names during manifest validation, as is already done for templates, scripts, presets, and indistinguishable hooks.
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),
src/specify_cli/presets/init.py:5666
- For a manifest-backed extension layer, this uses the directory/registry key (
ext_id) rather than the loaded manifest'sextension.id. Unregistered extension directories are intentionally scanned, so renaming such a directory while leavingextension.ymlunchanged makes this emitextension:<directory>:..., whereasExtensionManifest.contribution_id()emitsextension:<manifest-id>:.... The documented round-trip and directory-move stability guarantees then fail. Carry the loaded manifest ID out of_extension_manifest_declared_template()and use it for declared entries; keepext_idonly for the fallback sentinel.
"lookupId": derive_named_id(
lookup_layer, ext_id, template_type, template_name
),
extensions/EXTENSION-API-REFERENCE.md:904
- This API description omits the
ValueErrorraised byExtensionManifest.contribution_id()when valid colliding hooks share the same compound name. Document that unique matches return an ID, absent matches returnNone, and ambiguous hook lookups raiseValueError, so callers know they must handle that case or scaniter_contributions()by full ID.
`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. For hooks whose compound name matches multiple discriminated entries, it raises `ValueError`; callers must use `iter_contributions()` to select the desired full id. `PresetManifest` exposes the same two methods.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/specify_cli/presets/init.py:5666
- Manifest-backed lookup IDs use the extension directory name (
ext_id) rather than the manifest's declared ID. Unregistered extension directories are intentionally discovered by directory name, so renaming/copying a directory whoseextension.iddiffers makes this emitextension:<directory>:..., whileExtensionManifest.contribution_id()emitsextension:<manifest-id>:.... The advertised exact join and directory-move stability therefore fail. Propagate the loaded manifest ID/contribution ID from_extension_manifest_declared_template()for declared entries; retainext_idonly for the convention-fallback sentinel.
"lookupId": derive_named_id(
lookup_layer, ext_id, template_type, template_name
),
src/specify_cli/extensions/init.py:669
- This treats all floats as JSON-compatible, but YAML
.nan/.infvalues become non-finite floats andjson.dumps()emits non-standardNaN/Infinitytokens by default. Such a discriminator is not canonical JSON and cannot be reproduced by conforming JSON implementations, undermining the documented portable hash recipe. Reject non-finite floats here and setallow_nan=Falseincanonical_json()as defense in depth.
if value is None or isinstance(value, (str, bool, int, float)):
return
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/presets/init.py:5632
- For a manifest-backed preset this builds the join key from the registry/directory
pack_id, not the preset ID thatPresetManifest.contribution_id()uses._manifest_declared_template()accepts any valid manifest and does not enforcemanifest.id == pack_id, so an edited or manually registered preset can emitpreset:<directory-id>:...while its contribution ispreset:<manifest-id>:...; the promised exact lookup then fails. Carry the loaded manifest ID (or the computed contribution ID) out of the helper and use that for manifest-backed layers.
This issue also appears on line 5664 of the same file.
"lookupId": derive_named_id(
lookup_layer, pack_id, template_type, template_name
),
src/specify_cli/presets/init.py:5666
- Manifest-backed extension layers have the same source-ID mismatch: this uses the extension directory name, while
ExtensionManifest.contribution_id()derives fromextension.id._get_all_extensions_by_priority()intentionally supports unregistered directories, and_extension_manifest_declared_template()does not require their names to equal the manifest ID. Thus.specify/extensions/local-copy/extension.ymlwithid: actual-idemitsextension:local-copy:..., which cannot join toextension:actual-id:.... Return the loaded manifest ID/contribution ID from the helper for this branch.
"lookupId": derive_named_id(
lookup_layer, ext_id, template_type, template_name
),
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
582e6c7 to
6da4330
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (4)
src/specify_cli/presets/init.py:5658
- For supported unregistered extension directories (
_get_all_extensions_by_priority()at lines 5166-5218),ext_idis the directory name, not necessarily the ID declared inextension.yml. Renaming such a directory therefore changes thislookupId, and a valid declared contribution can produceextension:<directory>:...here whileExtensionManifest.contribution_id()producesextension:<manifest-id>:.... Carry the loaded manifest ID through_extension_manifest_declared_template()for declared entries; reserve a non-manifest sentinel for convention-only entries.
"lookupId": derive_named_id(
"extension", ext_id, template_type, template_name
),
src/specify_cli/extensions/init.py:837
- Because hook entries may contain extra fields, an entry can declare an
eventNamedifferent from its enclosing mapping key.setdefault()preserves that value in the surfaced contribution, while sibling matching and the generated ID use the mapping key, so consumers receive aneventNamethat contradicts the ID. Always set the synthesized field fromevent_name.
normalized = dict(entry)
normalized.setdefault("eventName", event_name)
flattened.append((event_name, normalized))
extensions/EXTENSION-API-REFERENCE.md:914
- These helpers derive identifiers; they cannot parse an existing opaque ID. Directing consumers to use them for parsing is therefore unusable guidance. State that IDs must not be parsed, and describe the helpers only as constructors when the original coordinates are available.
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.
extensions/EXTENSION-API-REFERENCE.md:916
- The inserted section replaced the existing
## File System Layoutheading, so the.specify/tree beginning below is now an unlabeled continuation of “Opacity guidance.” Restore the section boundary and heading before that code block.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Fixes #4210
Summary
Every command, template, script, and hook contribution returned by preset and extension manifest surfaces now carries a computed opaque
id, and every resolved artifact-stack layer carries a matchinglookupIdderived from the same recipe.Identifier contract
{layer}:{sourceId}:{kind}:{name}—layer∈core|preset|extension,sourceId=_for core / preset id / extension id,kind∈command|template|script|hook,name= the declared name.{layer}:{sourceId}:hook:{eventName}:{command}, with a:{12-hex}discriminator appended when two entries in the same source share the same(eventName, command)pair. Discriminator = first 12 lowercase hex chars ofsha256(canonical_json(entry − {eventName, command})). Two hook entries whose remaining declared fields are byte-identical are rejected at manifest load — there is no meaningful way to distinguish them.:is reserved as the component separator and is now guarded on hookeventName/command. All other id-component fields are already regex-constrained to forbid it.os.environ, or file-content hashes contribute — so they are stable across machines, reinstalls, and directory moves. Nothing is persisted.project:_:{kind}:{name}lookupIdthat intentionally does not match any manifest contribution.The change is purely additive: existing
name-based resolution is preserved, and no production call-site keys off the new fields.Modules touched
src/specify_cli/_identifier.py— new pure derivation module (derive_named_id,derive_hook_id,hook_discriminator,canonical_json,validate_component,PROJECT_OVERRIDE_LAYER).src/specify_cli/extensions/__init__.py— hook:guards oneventNameandcommand, byte-identical hook duplicate rejection,ExtensionManifest.iter_contributions()andExtensionManifest.contribution_id().src/specify_cli/presets/__init__.py—PresetManifest.iter_contributions()andPresetManifest.contribution_id(),lookupIdon every layer emitted byPresetResolver.collect_all_layers().Tests added
tests/test_contribution_ids.py— 39 tests covering the layer×kind derivation matrix, canonical JSON semantics, hook discriminator (no-collision, collision-with-suffix, reorder-stability, byte-identical rejection),:component guards, contribution-surface shape (additive, underlying data un-mutated), resolverlookupIdround-trip forproject/core/presetlayers, cross-subprocess determinism, mtime independence, and non-persistence.Docs updated
extensions/EXTENSION-API-REFERENCE.md— new "Contribution Identifiers" section (grammar, hook convention, discriminator recipe, reserved character,project:sentinel, Python API, determinism guarantees, opacity guidance) and TOC entry.docs/reference/presets.md— new "Contribution Identifiers" section that cross-links to the extension reference for the full grammar.Validation
pytest -qon the affected suites is clean apart from 10 pre-existing symlink-related failures caused by Windows privilege limitations (they fail identically onmainwithout these changes). All 39 new tests pass; all extensions/presets/hooks/unit tests unaffected by symlinks pass.Deviations from tasks.md
The scratch task list was drafted against an idealized layout that does not match the repo. Concrete adjustments applied in place:
src/specify_cli/_identifier.py(top-level helper, matching the_download_security.py/_utils.pypattern) rather than an idealizedsrc/specify_cli/manifests/_identifier.py. There is no sharedmanifests/package here._validate()methods onExtensionManifest(and, where relevant,PresetManifest) rather than in a separatemanifests/validation.py/manifests/loader.py.iter_contributions()/contribution_id()methods returning dicts rather than as typedContributionRef/HookContribution/ResolvedStackLayerdataclasses. Existing manifest storage stays untyped and byte-for-byte unchanged.PresetResolver.collect_all_layers()gain alookupIdkey. There is noResolvedStackLayertype to add a field to.tests/test_extensions.py/tests/test_presets.py.