From a37bc005bc0f4264feebb2a19ee5cd1b84094dc4 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 25 Aug 2026 20:26:29 +0500 Subject: [PATCH 1/5] fix(workflows): reject a non-integer current_step_index in RunState.load() RunState.load() shape-checks every other persisted field on resume -- workflow_id, installed_workflow_id, installed_registry_root, and inputs -- raising a clean "Invalid run state: ..." ValueError on a malformed value. current_step_index was the one field passed through unchecked. resume() later slices `definition.steps[state.current_step_index :]` with no guard of its own, so a non-int value (e.g. a hand-edited or externally-written state.json) reaches that slice and raises a raw `TypeError: slice indices must be integers or None or have an __index__ method` from deep inside resume() instead. A negative value slices from the end instead of failing, silently resuming from the wrong step. This mirrors the sibling field-validation pattern in RunState.load() (e.g. the workflow_id/installed_workflow_id checks) and the recurring "validate cleanly vs. crash at runtime" bug class already fixed across this codebase for step configs (e.g. #4144, #3899). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt --- src/specify_cli/workflows/engine.py | 27 +++++++++++++++++- tests/test_workflows.py | 43 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index d17513cc0b..c78fd62352 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -851,7 +851,32 @@ 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: ..." this loader already gives every + # other malformed field. A negative value slices from the end instead + # of failing, silently resuming from the wrong step. Reject both here, + # consistent with the sibling checks. ``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") diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d599f3c6a4..8a72d075cc 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -7376,6 +7376,49 @@ 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 + ): + """A malformed ``current_step_index`` must fail at load(), not resume(). + + ``resume()`` slices ``definition.steps[state.current_step_index :]`` + with no guard of its own. Every other field this loader restores + (``workflow_id``, ``installed_workflow_id``, ``installed_registry_root``, + ``inputs``) is shape-checked here and raises the same clean "Invalid + run state: ..." ``ValueError`` on a malformed value; ``current_step_index`` + was the one field silently passed through. A non-int value reaches the + slice and raises a raw, unhelpful ``TypeError`` from deep inside + ``resume()`` instead. ``True`` is included because ``bool`` is an + ``int`` subclass and would otherwise slip past a bare ``isinstance(..., + int)`` check. + """ + 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"), [ From bf9de58b4dd8db4df42c66dfcd4b19dde3fe9553 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Wed, 2 Sep 2026 00:20:37 +0500 Subject: [PATCH 2/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/test_workflows.py | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 8a72d075cc..a6152dca0e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -7383,18 +7383,9 @@ def test_load_rejects_stored_run_id_mismatch(self, project_dir): def test_load_rejects_invalid_current_step_index( self, project_dir, bad_current_step_index ): - """A malformed ``current_step_index`` must fail at load(), not resume(). - - ``resume()`` slices ``definition.steps[state.current_step_index :]`` - with no guard of its own. Every other field this loader restores - (``workflow_id``, ``installed_workflow_id``, ``installed_registry_root``, - ``inputs``) is shape-checked here and raises the same clean "Invalid - run state: ..." ``ValueError`` on a malformed value; ``current_step_index`` - was the one field silently passed through. A non-int value reaches the - slice and raises a raw, unhelpful ``TypeError`` from deep inside - ``resume()`` instead. ``True`` is included because ``bool`` is an - ``int`` subclass and would otherwise slip past a bare ``isinstance(..., - int)`` check. + """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 From 32e560cbe8c9ab93e88f14dabf16e9f8c4071d3f Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 13 Sep 2026 19:27:53 +0500 Subject: [PATCH 3/5] fix(workflows): reject out-of-range current_step_index in resume() RunState.load() checks current_step_index is a non-negative int but can't bound it against the step count, which is only known once the workflow definition is loaded in resume(). An out-of-range positive index (e.g. a hand-edited state.json) reached resume()'s definition.steps[state.current_step_index:] slice, which for any index >= len(steps) is an empty list -- so the run silently completed having executed no steps, instead of failing like every other malformed state field. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6 --- src/specify_cli/workflows/engine.py | 15 +++++++++++++ tests/test_workflows.py | 34 +++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index c78fd62352..10fff2a32b 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -1120,6 +1120,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 a6152dca0e..1811a72ce8 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -16712,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 ): From e61c156182318d59a5fc609e52649b8149c539eb Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 15 Sep 2026 22:45:01 +0500 Subject: [PATCH 4/5] docs(workflows): narrow the RunState.load() comment's validation claim The comment added for the current_step_index guard implied this loader shape-checks every field it restores. It doesn't: step_results, workflow_dir, current_step_id, created_at, updated_at, and error are all still assigned directly from state_data with no isinstance check. Only workflow_id, installed_workflow_id, installed_registry_root, and inputs are validated (pre-existing), plus current_step_index as of this PR. Reworded to name only those fields rather than claiming universal coverage. No behavior change. Co-Authored-By: Claude Sonnet 5 --- src/specify_cli/workflows/engine.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 10fff2a32b..2dec16d909 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -860,12 +860,17 @@ def load(cls, run_id: str, project_root: Path) -> RunState: # 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: ..." this loader already gives every - # other malformed field. A negative value slices from the end instead - # of failing, silently resuming from the wrong step. Reject both here, - # consistent with the sibling checks. ``bool`` is an ``int`` subclass, - # so it is excluded explicitly (mirrors the ``max_iterations`` / - # ``continue_on_error`` bool guards elsewhere in this module). + # 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) From 10f348096b996ac3ef6ca1e10af932ca1aee63c2 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Thu, 17 Sep 2026 19:49:18 +0500 Subject: [PATCH 5/5] Validate current_step_index before restoring state Add validation for current_step_index to prevent errors during resume. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/engine.py | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 2dec16d909..289351e2ae 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -852,25 +852,10 @@ def load(cls, run_id: str, project_root: Path) -> RunState: ) state.status = RunStatus(state_data["status"]) - # ``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). + # Validate the index shape before restoring it. The upper bound cannot + # be checked until resume() loads the workflow definition and is handled + # there. Reject bool explicitly because it subclasses int; otherwise a + # malformed value could fail during slicing or resume from the wrong step. current_step_index = state_data.get("current_step_index", 0) if ( isinstance(current_step_index, bool)