fix(workflows): reject a non-integer current_step_index in RunState.load() - #4325
Open
Noor-ul-ain001 wants to merge 2 commits into
Open
fix(workflows): reject a non-integer current_step_index in RunState.load()#4325Noor-ul-ain001 wants to merge 2 commits into
Noor-ul-ain001 wants to merge 2 commits into
Conversation
…oad() 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. github#4144, github#3899). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The validation is correct and well covered; only minor inaccurate explanatory wording remains.
Pull request overview
Adds validation for persisted workflow resume indices to prevent malformed state from causing runtime errors or incorrect resumption.
Changes:
- Rejects non-integer, boolean, and negative step indices.
- Adds parameterized regression coverage.
File summaries
| File | Description |
|---|---|
src/specify_cli/workflows/engine.py |
Validates current_step_index during state loading. |
tests/test_workflows.py |
Tests malformed index values. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+855
to
+868
| # ``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). |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Out-of-range positive indices can still silently skip all remaining steps and mark the run completed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+870
to
+874
| if ( | ||
| isinstance(current_step_index, bool) | ||
| or not isinstance(current_step_index, int) | ||
| or current_step_index < 0 | ||
| ): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RunState.load()shape-checks every other persisted field it restores on resume —workflow_id,installed_workflow_id,installed_registry_root,inputs— raising a cleanInvalid run state: ...ValueErroron a malformed value.current_step_indexwas the one field passed through with no check at all (state_data.get("current_step_index", 0)).resume()later slicesdefinition.steps[state.current_step_index :]with no guard of its own, so a non-intcurrent_step_index(e.g. a hand-edited or externally-writtenstate.json) reaches that slice and raises a rawTypeError: slice indices must be integers or None or have an __index__ methodfrom deep insideresume(), instead of the same clean domain error every sibling field already gets. A negative value slices from the end of the step list instead of failing, silently resuming from the wrong step.casesblock on switch steps #4144 for switch'scases, fix(workflows): reject mismatched run state IDs #3899 for run-state IDs) — here it shows up inRunState.load()'s field validation instead of a step'svalidate()/execute()pair.Test plan
test_load_rejects_invalid_current_step_index(parametrized over a string, float, negative int, list, dict, and bool) totests/test_workflows.py::TestRunStateDID NOT RAISE <class 'ValueError'>for all 6 cases — and passes with it (stashed onlysrc/specify_cli/workflows/engine.py, kept the test)tests/test_workflows.py::TestRunState— 31 passed, no regressionstests/test_workflows.py— 851 passed; the 17 failed / 82 errored are pre-existing Windows-only symlink-guard tests (need Developer Mode elevation) andPermissionError: [WinError 5]on the sharedpytest-of-Etmp dir, both unrelated to this change (confirmed identical onmain)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt