fix(workflows): reject a non-integer or out-of-range current_step_index in RunState resume by Noor-ul-ain001 · Pull Request #4325 · github/spec-kit

Noor-ul-ain001 · 2026-08-25T15:26:19Z

Summary

RunState.load() restores current_step_index via state_data.get("current_step_index", 0) with no shape check, unlike a few of the other fields it restores — workflow_id, installed_workflow_id, installed_registry_root, and inputs — which are validated and raise a clean Invalid run state: ... ValueError on a malformed value. (This loader does not shape-check every restored field: step_results, workflow_dir, current_step_id, created_at/updated_at, and error are still assigned directly with no isinstance check. This PR only addresses current_step_index, since it's the one resume() depends on for a safe list slice — it is not a loader-wide validation audit.)
resume() later slices definition.steps[state.current_step_index :] with no guard of its own, so a non-int current_step_index (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, instead of the same clean domain error the other validated fields already get. A negative value slices from the end of the step list instead of failing, silently resuming from the wrong step.
A follow-up commit in this PR also rejects an out-of-range positive index in resume() itself (RunState.load() can reject non-int/negative values but can't check the upper bound — the step count isn't known until the workflow definition loads, later, in resume()). Without this, an out-of-range index would silently slice to an empty list and complete the run without executing any step.
This is the same "validate cleanly vs. crash at runtime on the same bad value" shape already fixed repeatedly for step configs in this codebase (e.g. fix(workflows): require a cases block on switch steps #4144 for switch's cases, fix(workflows): reject mismatched run state IDs #3899 for run-state IDs) — here it shows up in RunState.load()'s field validation instead of a step's validate()/execute() pair.

Test plan

Added test_load_rejects_invalid_current_step_index (parametrized over a string, float, negative int, list, dict, and bool) to tests/test_workflows.py::TestRunState
Added test_resume_rejects_out_of_range_current_step_index covering the upper-bound case caught in resume()
Verified both new tests fail without their respective fix and pass with it (stashed only src/specify_cli/workflows/engine.py, confirmed the failure, then restored it and confirmed the pass)
Ran tests/test_workflows.py::TestRunState — passing, no regressions
Ran the full tests/test_workflows.py — pre-existing Windows-only symlink-guard/tmp-dir failures reproduced identically on main, unrelated to this change

AI Disclosure

I did use AI assistance (describe below)

This PR was authored with Claude Code (model: Claude Sonnet 5), used in its interactive, agentic CLI mode with full local tool access (file read/edit, running pytest, git). It identified the bug, wrote the source fix, the out-of-range follow-up fix, and the regression tests, and ran the verification described above, under my direction. I reviewed the diff and test output myself before opening this PR and understand the change being made.

Separately, GitHub Copilot reviewed this PR and left two inline findings pointing out that the new comment/docstring overclaimed loader-wide field validation (the same issue raised in review here). Its automated Copilot Autofix commit (bf9de58b, "Potential fix for pull request finding") addressed one of the two — trimming the inaccurate claim out of the test docstring in tests/test_workflows.py, no logic change — which I reviewed before it stayed on the branch. The matching finding on the engine.py source comment was not auto-fixed; that one is what this update (commit e61c1561) resolves.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

…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

Copilot

🟢 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.

+        # ``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>

Copilot

🟡 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

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6

Copilot

🟢 Approval recommended

The validation is correctly placed, preserves state on failure, and has focused regression coverage.

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Balanced

mnriem · 2026-09-15T17:27:11Z

The out-of-range-index review request is addressed. We’ll keep this as triage-can-wait: it provides defensive handling for malformed run-state files, but no failure from normal engine-generated state has been identified.

Please narrow the remaining claim that every other restored field is validated; several fields are still restored directly. This does not call for expanding the PR into a loader-wide validation audit.

Please also add a plain AI-disclosure statement covering the Claude assistance and Copilot Autofix contribution, including the tool/model, mode/settings, and extent of assistance.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

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 <noreply@anthropic.com>

Noor-ul-ain001 · 2026-09-15T17:47:00Z

Thanks! Addressed both remaining points:

Narrowed the overclaim: the engine.py comment near the current_step_index guard said this loader gives "every other malformed field" a clean error — untrue, since step_results, workflow_dir, current_step_id, created_at/updated_at, and error are still assigned directly with no isinstance check. Reworded (commit e61c1561) to name only the fields that actually are validated (workflow_id, installed_workflow_id, installed_registry_root, inputs) and to say explicitly this PR only addresses current_step_index, not a loader-wide audit. Comment-only change, no behavior touched. Mirrored the same fix into the updated PR summary.
Added an AI Disclosure section to the PR body: tool/model (Claude Code, Claude Sonnet 5), mode (interactive agentic CLI with local tool access), and extent (AI wrote the fixes and tests under my direction; I reviewed the diff/output before opening/updating the PR). It also covers the Copilot Autofix commit (bf9de58b) that trimmed the same overclaim out of the test docstring — noting that its sibling finding on the engine.py comment wasn't auto-fixed, which is what this update resolves.

Let me know if anything else needs adjusting.

Noor-ul-ain001 requested a review from mnriem as a code owner August 25, 2026 15:26

mnriem requested a balanced review from Copilot September 1, 2026 17:32

Copilot started reviewing on behalf of mnriem September 1, 2026 17:33 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Potential fix for pull request finding

bf9de58

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

mnriem requested a balanced review from Copilot September 1, 2026 22:00

Copilot started reviewing on behalf of mnriem September 1, 2026 22:01 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py

mnriem mentioned this pull request Sep 8, 2026

fix(workflows): namespace nested descendant step ids in loops/fan-out #4338

Open

7 tasks

mnriem requested a balanced review from Copilot September 15, 2026 17:20

Copilot started reviewing on behalf of mnriem September 15, 2026 17:21 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

mnriem added triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-awaiting Waiting on author response labels Sep 15, 2026

Noor-ul-ain001 changed the title ~~fix(workflows): reject a non-integer current_step_index in RunState.load()~~ fix(workflows): reject a non-integer or out-of-range current_step_index in RunState resume Sep 15, 2026

Navigation Menu

Uh oh!

fix(workflows): reject a non-integer or out-of-range current_step_index in RunState resume - #4325

fix(workflows): reject a non-integer or out-of-range current_step_index in RunState resume#4325
Noor-ul-ain001 wants to merge 4 commits into
github:mainfrom
Noor-ul-ain001:fix/runstate-current-step-index

Noor-ul-ain001 commented Aug 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

mnriem commented Sep 15, 2026

Uh oh!

Noor-ul-ain001 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Noor-ul-ain001 commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

mnriem commented Sep 15, 2026

Uh oh!

Noor-ul-ain001 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Noor-ul-ain001 commented Aug 25, 2026 •

edited

Loading