Replace flow validation step-index sentinels with named API#382
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a8b0c3d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,19 @@ | |||
| """Named step-index sentinels for flow-level validation records.""" | |||
There was a problem hiding this comment.
Adding chainweaver/step_index.py is an add-module architecture change, but the AGENTS.md repo map still omits it and docs/agent-context/architecture.md has no module-boundary entry for the new public helper. AGENTS.md §10 explicitly requires add/remove/rename modules to update both the AGENTS.md repo map and architecture.md in the same PR, so leaving these stale breaks the repository’s documented source of truth for future agents.
Useful? React with 👍 / 👎.
dgenio
left a comment
dgenio
left a comment
There was a problem hiding this comment.
AI Code Review — REQUEST_CHANGES
Solid, well-scoped refactor that matches issue #339's intent — the named API is a clean 1:1 replacement with no behavior change, and the public API snapshot is correctly updated. Two blockers before merge:
1. mypy fails CI (12 errors). The _FlowLike Protocol's steps: Sized is a mutable (invariant) member, so list[FlowStep] / list[DAGFlowStep] don't satisfy it. One-line fix — make steps a read-only @property (verified locally: clears all errors). This also breaks the repo invariant that new code must pass mypy. See inline comment on step_index.py.
2. New module is undocumented. chainweaver/step_index.py isn't in the AGENTS.md repo map or architecture.md, which the documentation-governance rule requires for module adds. See inline comment.
Not blocking: issue #339's "add a test asserting validation records carry the named constants" is satisfied by the updated assertions in test_data_integrity / test_flow_execution / test_step_contracts — no separate test needed.
Once mypy is green and the docs are updated, this is good to go. Nice clean change otherwise.
| Requirement (#339) | Status |
|---|---|
| No remaining literal sentinels | ✅ Met |
| Constants exported + snapshotted | ✅ Met |
| Constants documented | |
| All validation passes (incl. mypy) | ❌ Not met |
| Test asserts named constants | ✅ Met (via updated tests) |
Generated by Claude Code
| class _FlowLike(Protocol): | ||
| steps: Sized |
There was a problem hiding this comment.
Blocker — fails mypy (12 errors in CI).
The _FlowLike Protocol declares steps: Sized as a mutable attribute, which mypy checks invariantly — so Flow.steps: list[FlowStep] (a subtype of Sized, not an exact match) does not satisfy the protocol. This breaks all six flow_output_step_index(flow) call sites, violating the "must pass mypy" invariant.
Fix — make the member read-only so it's covariant:
class _FlowLike(Protocol):
@property
def steps(self) -> Sized: ...Verified: this clears all 12 errors (Success: no issues found). Suggest adding mypy to your local verification — the PR description ran ruff + pytest but not mypy, which is why this slipped through.
Generated by Claude Code
There was a problem hiding this comment.
Blocker — documentation governance.
Adding chainweaver/step_index.py is a module-add. AGENTS.md requires add/remove/rename-module changes to update the repo-map tree and docs/agent-context/architecture.md in the same PR. Neither was updated (Codex flagged this too).
Please add a repo-map line, e.g.:
├── step_index.py FLOW_INPUT_STEP_INDEX + flow_output_step_index(flow): named
sentinels for flow-level validation StepRecords (#339)
and a matching module-boundary entry in architecture.md. The StepRecord table update on L283 is correct — this is only about the repo map + architecture doc.
Generated by Claude Code
2 participants
Closes #339
Summary
FLOW_INPUT_STEP_INDEXandflow_output_step_index(flow)API for flow-level validation recordsVerification
git diff --checkuvx --with '.[dev]' ruff format --check chainweaver/step_index.py chainweaver/__init__.py chainweaver/compiler.py chainweaver/executor.py chainweaver/export/callable.py chainweaver/mcp/server.py chainweaver/tools.py tests/test_data_integrity.py tests/test_flow_execution.py tests/test_step_contracts.pyuvx --with '.[dev]' ruff check chainweaver/step_index.py chainweaver/__init__.py chainweaver/compiler.py chainweaver/executor.py chainweaver/export/callable.py chainweaver/mcp/server.py chainweaver/tools.py tests/test_data_integrity.py tests/test_flow_execution.py tests/test_step_contracts.pyuvx --with '.[dev]' pytest --no-cov tests/test_flow_execution.py tests/test_data_integrity.py tests/test_step_contracts.py tests/test_public_api_snapshot.pyuvx --with '.[dev]' pytest --no-covNote: the same targeted pytest run without
--no-covcollected 81 passing tests but failed the repository-wide coverage gate because coverage collected no data in theuvxisolated environment.