diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index d17513cc0b..2dec16d909 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -851,7 +851,37 @@ def load(cls, run_id: str, project_root: Path) -> RunState: installed_origin_tracked=has_installed_workflow_id, ) state.status = RunStatus(state_data["status"]) - state.current_step_index = state_data.get("current_step_index", 0) + + # ``resume()`` slices ``definition.steps[state.current_step_index :]`` + # with no guard of its own -- unlike ``workflow_id`` / + # ``installed_workflow_id`` / ``installed_registry_root`` / ``inputs`` + # above, this field was never shape-checked here. A non-int value (a + # hand-edited or externally-written state.json, e.g. a string or + # float) reaches that slice and raises a raw, unhelpful + # ``TypeError: slice indices must be integers or None or have an + # __index__ method`` from deep inside ``resume()`` instead of the + # clean "Invalid run state: ..." error the sibling fields above + # already give when malformed. A negative value slices from the end + # instead of failing, silently resuming from the wrong step. Reject + # both here, consistent with those sibling checks (this loader does + # not shape-check every restored field -- e.g. ``step_results`` and + # ``workflow_dir`` below are still assigned directly -- only + # ``current_step_index`` is addressed here, since it is the one + # ``resume()`` depends on for a safe list slice). ``bool`` is an + # ``int`` subclass, so it is excluded explicitly (mirrors the + # ``max_iterations`` / ``continue_on_error`` bool guards elsewhere in + # this module). + current_step_index = state_data.get("current_step_index", 0) + if ( + isinstance(current_step_index, bool) + or not isinstance(current_step_index, int) + or current_step_index < 0 + ): + raise ValueError( + "Invalid run state: 'current_step_index' must be a " + f"non-negative integer, got {current_step_index!r}" + ) + state.current_step_index = current_step_index state.current_step_id = state_data.get("current_step_id") state.step_results = state_data.get("step_results", {}) state.workflow_dir = state_data.get("workflow_dir") @@ -1095,6 +1125,21 @@ def resume( else: definition = self.load_workflow(state.workflow_id) + # RunState.load() rejects a non-int/negative current_step_index but + # can't check the upper bound — the step count isn't known until the + # workflow definition is loaded, above. An out-of-range positive + # index (e.g. a hand-edited state.json) would otherwise slice + # definition.steps[state.current_step_index:] into an empty list + # below, silently completing the run without executing any step. + if state.current_step_index >= len(definition.steps): + msg = ( + "Invalid run state: 'current_step_index' " + f"({state.current_step_index}) is out of range for " + f"workflow {state.workflow_id!r} with {len(definition.steps)} " + "step(s)." + ) + raise ValueError(msg) + dispatch_default_errors = _dispatch_default_errors(definition) if dispatch_default_errors: raise ValueError(" ".join(dispatch_default_errors)) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d599f3c6a4..1811a72ce8 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -7376,6 +7376,40 @@ def test_load_rejects_stored_run_id_mismatch(self, project_dir): ): RunState.load("requested-run", project_dir) + @pytest.mark.parametrize( + "bad_current_step_index", + ["not-a-number", 1.5, -1, [0], {"index": 0}, True], + ) + def test_load_rejects_invalid_current_step_index( + self, project_dir, bad_current_step_index + ): + """Reject non-integer and negative resume indices at load time. + + ``bool`` is covered explicitly because it subclasses ``int``. + """ + from specify_cli.workflows.engine import RunState + + run_dir = ( + project_dir / ".specify" / "workflows" / "runs" / "bad-index-run" + ) + run_dir.mkdir(parents=True) + (run_dir / "state.json").write_text( + json.dumps( + { + "run_id": "bad-index-run", + "workflow_id": "test-workflow", + "status": "paused", + "current_step_index": bad_current_step_index, + } + ), + encoding="utf-8", + ) + + with pytest.raises( + ValueError, match="'current_step_index' must be a non-negative integer" + ): + RunState.load("bad-index-run", project_dir) + @pytest.mark.parametrize( ("installed_workflow_id", "installed_registry_root"), [ @@ -16678,6 +16712,40 @@ def test_resume_preload_rejects_malformed_state_cleanly( assert result.exception is None or isinstance(result.exception, SystemExit) assert "Invalid run state" in result.output + def test_resume_rejects_out_of_range_current_step_index( + self, project_dir, monkeypatch + ): + """An out-of-range positive index must fail cleanly, not silently + complete the run with no steps executed. + + ``resume()`` slices ``definition.steps[state.current_step_index:]``; + for any index >= len(steps) that slice is an empty list, so the run + would otherwise finish with status "completed" having executed + nothing. + """ + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + runner = CliRunner() + run_id = self._install_and_run_gated(runner, app, project_dir) + state_path = ( + project_dir / ".specify" / "workflows" / "runs" / run_id / "state.json" + ) + data = json.loads(state_path.read_text(encoding="utf-8")) + data["current_step_index"] = 5 + state_path.write_text(json.dumps(data), encoding="utf-8") + + result = runner.invoke(app, ["workflow", "resume", run_id]) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Invalid run state" in result.output + assert "out of range" in result.output + + reloaded = json.loads(state_path.read_text(encoding="utf-8")) + assert reloaded["status"] == "paused" + def test_resume_legacy_run_respects_current_disabled_state( self, project_dir, monkeypatch ):