Skip to content

Commit bb16911

Browse files
jawwad-aliclaude
andcommitted
fix(bundler): check an immutable built-in step set, not the mutable registry
Review catch: `STEP_REGISTRY` is not limited to bundled steps. `load_custom_steps` adds project-installed ids to that process-global mapping and never removes them, so in a long-lived process a community step loaded while working on project A would be accepted as "bundled" when validating a bundle for project B — before B's own StepRegistry is consulted. Snapshot the shipped ids into `BUILTIN_STEP_TYPES` immediately after `_register_builtin_steps()`, before `load_custom_steps` is even defined, and check that frozenset instead. Verified: with the check on STEP_REGISTRY the new cross-project test fails (a leaked community id resolves as bundled); with BUILTIN_STEP_TYPES it passes. 1 failed, 5 passed -> 6 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 22ab8a8 commit bb16911

3 files changed

Lines changed: 58 additions & 7 deletions

File tree

src/specify_cli/bundler/services/references.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,16 +40,20 @@ def _resolved_locally(root: Path, component: ComponentRef) -> bool:
4040
return True
4141
return WorkflowRegistry(root).is_installed(component.id)
4242
if kind == "steps":
43-
from ...workflows import STEP_REGISTRY
43+
from ...workflows import BUILTIN_STEP_TYPES
4444
from ...workflows.catalog import StepRegistry
4545

4646
# Step types ship with Spec Kit as built-ins (shell, gate, if, ...)
4747
# rather than as an on-disk asset directory, so there is no
4848
# ``_locate_bundled_step`` to mirror the three lookups above.
49-
# ``STEP_REGISTRY`` is the bundled-with-Spec-Kit check for this kind
50-
# -- it is what ``specify workflow step info`` reports as
51-
# "built-in". Without it every built-in step type looked unresolved.
52-
if component.id in STEP_REGISTRY:
49+
# ``BUILTIN_STEP_TYPES`` is the bundled-with-Spec-Kit check for this
50+
# kind. Deliberately NOT ``STEP_REGISTRY``: ``load_custom_steps``
51+
# adds project-installed ids to that process-global mapping and
52+
# never removes them, so in a long-lived process a community step
53+
# loaded for one project would be accepted as "bundled" when
54+
# validating another. Without any bundled check at all, every
55+
# built-in step type looked unresolved.
56+
if component.id in BUILTIN_STEP_TYPES:
5357
return True
5458
return StepRegistry(root).is_installed(component.id)
5559
except Exception: # noqa: BLE001 - resolution is best-effort

src/specify_cli/workflows/__init__.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,14 @@ def _register_builtin_steps() -> None:
7171

7272
_register_builtin_steps()
7373

74+
# The step types Spec Kit ships, snapshotted before any community step can be
75+
# loaded. ``load_custom_steps`` adds project-installed ids to the process-global
76+
# ``STEP_REGISTRY`` and never removes them, so ``STEP_REGISTRY`` cannot answer
77+
# "is this bundled with Spec Kit?" in a long-lived process: a step loaded for one
78+
# project would look built-in for the next. Callers that need the immutable set
79+
# (e.g. the bundler's reference checker) must use this instead.
80+
BUILTIN_STEP_TYPES: frozenset[str] = frozenset(STEP_REGISTRY)
81+
7482

7583
def load_custom_steps(project_root: Path) -> list[str]:
7684
"""Load community-installed custom step types into STEP_REGISTRY.

tests/unit/test_bundler_references.py

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,57 @@ def test_builtin_step_type_resolves(tmp_path: Path):
3434
types installed under ``.specify/workflows/steps/`` — so every built-in step
3535
type was reported as an unresolved reference.
3636
"""
37-
from specify_cli.workflows import STEP_REGISTRY
37+
from specify_cli.workflows import BUILTIN_STEP_TYPES
3838

3939
root = make_project(tmp_path)
4040
warnings: list[str] = []
4141
check = make_reference_checker(root, allow_network=True, warnings=warnings)
4242

4343
for step_id in ("shell", "gate", "command", "if"):
44-
assert step_id in STEP_REGISTRY, step_id
44+
assert step_id in BUILTIN_STEP_TYPES, step_id
4545
assert check(_ref("steps", step_id)) is None, step_id
4646
assert warnings == []
4747

4848

49+
def test_community_step_is_not_treated_as_bundled(tmp_path: Path):
50+
"""A community step loaded for one project must not resolve for another.
51+
52+
`load_custom_steps` adds project-installed ids to the process-global
53+
`STEP_REGISTRY` and never removes them, so checking `STEP_REGISTRY` here
54+
would accept project A's community step as "bundled" while validating
55+
project B. `BUILTIN_STEP_TYPES` is snapshotted before any custom step can
56+
load, which is why the check uses it instead.
57+
"""
58+
from specify_cli.workflows import (
59+
BUILTIN_STEP_TYPES,
60+
STEP_REGISTRY,
61+
_register_step,
62+
)
63+
from specify_cli.workflows.base import StepBase, StepResult, StepStatus
64+
65+
class _CommunityStep(StepBase):
66+
type_key = "community-only-step"
67+
68+
def execute(self, config, context): # pragma: no cover - never run
69+
return StepResult(status=StepStatus.COMPLETED)
70+
71+
# Simulate project A having loaded a community step into the global registry.
72+
_register_step(_CommunityStep())
73+
try:
74+
assert "community-only-step" in STEP_REGISTRY
75+
assert "community-only-step" not in BUILTIN_STEP_TYPES
76+
77+
# Project B does not have it installed, so it must NOT resolve locally.
78+
root = make_project(tmp_path)
79+
warnings: list[str] = []
80+
check = make_reference_checker(root, allow_network=True, warnings=warnings)
81+
problem = check(_ref("steps", "community-only-step"))
82+
assert problem is not None, "leaked community step resolved as bundled"
83+
assert "community-only-step" in problem
84+
finally:
85+
STEP_REGISTRY.pop("community-only-step", None)
86+
87+
4988
def test_unknown_step_type_still_errors_online(tmp_path: Path):
5089
"""The guard must not make every step id resolve."""
5190
root = make_project(tmp_path)

0 commit comments

Comments
 (0)