mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
Fix workflow resume disabled-check bypass after project move/rename
RunState.installed_registry_root previously persisted the creation-time absolute project path unconditionally whenever a run belonged to an installed workflow. After the whole project directory was renamed or moved, workflow_resume would open a WorkflowRegistry at that now nonexistent path, get back an empty/default registry, and silently skip the disabled-workflow check -- a paused run for a disabled workflow could be resumed successfully from the new location. Fix persists installed_registry_root only when the owning root genuinely differs from the current project_root (true cross-project direct-file- source invocations). The common same-project case now persists None and is re-derived from the live project_root at resume time via a new _resolve_run_owner_root() helper, which also falls back to project_root if a stored root no longer exists on disk -- covering both the common case transparently surviving project moves and the cross-project case degrading safely if its owner project vanishes, rather than silently skipping the disabled check. Backward compatible: state files missing the new fields, and states with a still-existing distinct cross-project root, behave unchanged. Added regression tests: - resume blocked after project moved then disabled at new location - resume still works after project moved while workflow stays enabled - cross-project registry root is still correctly honored when it exists - resume falls back to current project's registry when a stored cross-project root no longer exists Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -77,6 +77,33 @@ def _open_workflow_registry(project_root: Path, out=None):
|
||||
raise typer.Exit(1)
|
||||
|
||||
|
||||
def _resolve_run_owner_root(
|
||||
installed_registry_root: str | None, project_root: Path
|
||||
) -> Path:
|
||||
"""Determine which project's registry gates resuming a run.
|
||||
|
||||
``installed_registry_root`` is only ever persisted when the run's
|
||||
installed workflow genuinely belongs to a *different* project than the
|
||||
one whose ``runs/`` directory holds this run's own state (a direct
|
||||
external workflow-file invocation) -- see ``workflow_run``. The common
|
||||
case (an installed workflow run from its own project) stores ``None``,
|
||||
so a later project rename/move is transparently picked up here by
|
||||
falling back to the *current* ``project_root`` instead of a stale
|
||||
absolute path baked in at run start.
|
||||
|
||||
A persisted cross-project root that itself no longer exists (that
|
||||
other project moved/was deleted) is not trusted either: silently
|
||||
skipping the disabled check because a stored path merely happens not
|
||||
to resolve would defeat the guard's purpose, so this also falls back
|
||||
to ``project_root`` rather than risk that.
|
||||
"""
|
||||
if installed_registry_root:
|
||||
candidate = Path(installed_registry_root)
|
||||
if candidate.is_dir():
|
||||
return candidate
|
||||
return project_root
|
||||
|
||||
|
||||
def _parse_input_values(
|
||||
input_values: list[str] | None, *, json_output: bool = False
|
||||
) -> dict[str, Any]:
|
||||
@@ -730,7 +757,20 @@ def workflow_run(
|
||||
definition,
|
||||
inputs,
|
||||
installed_workflow_id=registered_id,
|
||||
installed_registry_root=registry_root if registered_id else None,
|
||||
# Only persist an explicit root when the installed workflow
|
||||
# genuinely belongs to a *different* project than the one
|
||||
# whose runs/ directory holds this run's own state (a
|
||||
# direct external workflow-file invocation) -- the common
|
||||
# case (an installed workflow run from its own project)
|
||||
# leaves this None so resume re-derives the owning root
|
||||
# from wherever the project currently is, transparently
|
||||
# surviving a project rename/move instead of baking in a
|
||||
# stale absolute path at run start.
|
||||
installed_registry_root=(
|
||||
registry_root
|
||||
if registered_id and registry_root != project_root
|
||||
else None
|
||||
),
|
||||
)
|
||||
except ValueError as exc:
|
||||
err.print(f"[red]Error:[/red] {exc}")
|
||||
@@ -801,10 +841,8 @@ def workflow_resume(
|
||||
raise typer.Exit(1)
|
||||
|
||||
if pre_state.installed_workflow_id is not None:
|
||||
owner_root = (
|
||||
Path(pre_state.installed_registry_root)
|
||||
if pre_state.installed_registry_root
|
||||
else project_root
|
||||
owner_root = _resolve_run_owner_root(
|
||||
pre_state.installed_registry_root, project_root
|
||||
)
|
||||
installed_meta = _open_workflow_registry(owner_root, err).get(
|
||||
pre_state.installed_workflow_id
|
||||
|
||||
@@ -10038,6 +10038,156 @@ steps:
|
||||
result = runner.invoke(app, ["workflow", "resume", run_id, "--json"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
def test_resume_blocks_after_project_moved_following_disable(
|
||||
self, temp_dir, monkeypatch
|
||||
):
|
||||
"""Renaming/moving the entire project after starting a run must not
|
||||
let a subsequent disable-then-resume bypass the guard. Persisting
|
||||
the run's *creation-time absolute* project path would make resume
|
||||
open a now-nonexistent old root (WorkflowRegistry falls back to an
|
||||
empty default there), missing the disabled entry that actually
|
||||
lives in the *current* (moved) project's registry. The common,
|
||||
same-project case must instead re-derive the owning root from the
|
||||
project's current location on every resume."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
import shutil
|
||||
|
||||
project_v1 = temp_dir / "project-v1"
|
||||
(project_v1 / ".specify" / "workflows").mkdir(parents=True)
|
||||
monkeypatch.chdir(project_v1)
|
||||
runner = CliRunner()
|
||||
run_id = self._install_and_run_gated(runner, app, project_v1)
|
||||
|
||||
project_v2 = temp_dir / "project-v2"
|
||||
shutil.move(str(project_v1), str(project_v2))
|
||||
monkeypatch.chdir(project_v2)
|
||||
|
||||
result = runner.invoke(app, ["workflow", "disable", "gated-wf"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
result = runner.invoke(app, ["workflow", "resume", run_id])
|
||||
assert result.exit_code != 0
|
||||
assert "disabled" in result.output
|
||||
|
||||
def test_resume_after_project_moved_still_works_when_enabled(
|
||||
self, temp_dir, monkeypatch
|
||||
):
|
||||
"""The inverse of the move regression: an enabled workflow's run
|
||||
must still resume normally after the project is moved -- the
|
||||
current-project fallback must not itself block legitimate
|
||||
resumes."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
import shutil
|
||||
|
||||
project_v1 = temp_dir / "project-v1-ok"
|
||||
(project_v1 / ".specify" / "workflows").mkdir(parents=True)
|
||||
monkeypatch.chdir(project_v1)
|
||||
runner = CliRunner()
|
||||
run_id = self._install_and_run_gated(runner, app, project_v1)
|
||||
|
||||
project_v2 = temp_dir / "project-v2-ok"
|
||||
shutil.move(str(project_v1), str(project_v2))
|
||||
monkeypatch.chdir(project_v2)
|
||||
|
||||
result = runner.invoke(app, ["workflow", "resume", run_id, "--json"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
def test_resume_respects_cross_project_registry_root(
|
||||
self, temp_dir, monkeypatch
|
||||
):
|
||||
"""A run started via a direct workflow.yml path belonging to a
|
||||
different project than the cwd used for `workflow run`/`workflow
|
||||
resume` must still gate resuming on *that* owning project's
|
||||
registry, not the cwd project's (which has no entry for this ID
|
||||
at all). This is the genuine cross-project case that must remain
|
||||
unaffected by only special-casing the common same-project one."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
owner_project = temp_dir / "owner-project"
|
||||
(owner_project / ".specify" / "workflows").mkdir(parents=True)
|
||||
monkeypatch.chdir(owner_project)
|
||||
runner = CliRunner()
|
||||
src = owner_project / "gated-src"
|
||||
src.mkdir()
|
||||
(src / "workflow.yml").write_text(self._GATED_WORKFLOW_YAML, encoding="utf-8")
|
||||
result = runner.invoke(app, ["workflow", "add", str(src), "--dev"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
unrelated_cwd = temp_dir / "unrelated-cwd"
|
||||
unrelated_cwd.mkdir()
|
||||
monkeypatch.chdir(unrelated_cwd)
|
||||
|
||||
target = owner_project / ".specify" / "workflows" / "gated-wf" / "workflow.yml"
|
||||
result = runner.invoke(app, ["workflow", "run", str(target), "--json"])
|
||||
assert result.exit_code == 0, result.output
|
||||
run_id = json.loads(result.stdout)["run_id"]
|
||||
|
||||
monkeypatch.chdir(owner_project)
|
||||
result = runner.invoke(app, ["workflow", "disable", "gated-wf"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
# Resume must run from unrelated_cwd (where this run's own
|
||||
# state.json actually lives) yet still be blocked by the owner
|
||||
# project's disabled entry.
|
||||
monkeypatch.chdir(unrelated_cwd)
|
||||
result = runner.invoke(app, ["workflow", "resume", run_id])
|
||||
assert result.exit_code != 0
|
||||
assert "disabled" in result.output
|
||||
|
||||
def test_resume_falls_back_to_current_project_when_cross_project_root_vanishes(
|
||||
self, temp_dir, monkeypatch
|
||||
):
|
||||
"""A persisted cross-project owning root that no longer exists (that
|
||||
other project was itself deleted/moved away) must not be trusted as
|
||||
a safety signal -- silently skipping the disabled check just
|
||||
because the stored path happens not to resolve would defeat the
|
||||
guard. Falls back to the current project's own registry instead."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
import shutil
|
||||
|
||||
owner_project = temp_dir / "owner-project-2"
|
||||
(owner_project / ".specify" / "workflows").mkdir(parents=True)
|
||||
monkeypatch.chdir(owner_project)
|
||||
runner = CliRunner()
|
||||
src = owner_project / "gated-src"
|
||||
src.mkdir()
|
||||
(src / "workflow.yml").write_text(self._GATED_WORKFLOW_YAML, encoding="utf-8")
|
||||
result = runner.invoke(app, ["workflow", "add", str(src), "--dev"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
unrelated_cwd = temp_dir / "unrelated-cwd-2"
|
||||
unrelated_cwd.mkdir()
|
||||
monkeypatch.chdir(unrelated_cwd)
|
||||
|
||||
target = owner_project / ".specify" / "workflows" / "gated-wf" / "workflow.yml"
|
||||
result = runner.invoke(app, ["workflow", "run", str(target), "--json"])
|
||||
assert result.exit_code == 0, result.output
|
||||
run_id = json.loads(result.stdout)["run_id"]
|
||||
|
||||
# owner_project vanishes entirely -- its persisted absolute root
|
||||
# is now dangling.
|
||||
shutil.rmtree(owner_project)
|
||||
|
||||
# unrelated_cwd (where this run's own state.json lives) has its own
|
||||
# separately-installed, disabled workflow under the same ID.
|
||||
src2 = unrelated_cwd / "gated-src"
|
||||
src2.mkdir()
|
||||
(src2 / "workflow.yml").write_text(
|
||||
self._GATED_WORKFLOW_YAML, encoding="utf-8"
|
||||
)
|
||||
result = runner.invoke(app, ["workflow", "add", str(src2), "--dev"])
|
||||
assert result.exit_code == 0, result.output
|
||||
result = runner.invoke(app, ["workflow", "disable", "gated-wf"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
result = runner.invoke(app, ["workflow", "resume", run_id])
|
||||
assert result.exit_code != 0
|
||||
assert "disabled" in result.output
|
||||
|
||||
def test_disable_shows_marker_in_list(self, project_dir, monkeypatch):
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
Reference in New Issue
Block a user