96 lines
4.0 KiB
Markdown
96 lines
4.0 KiB
Markdown
# T3.4 — `StepId` stability checks
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Phase | P3 — Workflow as data |
|
|
| Size | S — under 1 day |
|
|
| Status | Not started |
|
|
| Flags | — |
|
|
| Spec | inlined below |
|
|
| Blocks | — |
|
|
|
|
## Goal
|
|
|
|
Report added, removed and retained `StepId`s across a version bump, and reject a
|
|
bump that retains nothing unless it is explicitly marked a rewrite.
|
|
|
|
## Facts (inlined — no spec read needed)
|
|
|
|
- Credit assignment attributes outcomes to steps **across workflow versions**.
|
|
That requires a step identity surviving edits: insert a step at position 2 and
|
|
every positional index shifts, but `StepId` does not.
|
|
- **The framework cannot infer this.** It is a documented obligation on the
|
|
workflow author, enforced by three load-time checks:
|
|
1. `StepId` unique within a version.
|
|
2. On a version bump, report added, removed and retained ids. A version
|
|
retaining **no** ids from its parent is almost certainly a renumbering
|
|
accident and is rejected unless explicitly marked as a rewrite.
|
|
3. `StepId` is opaque to the framework — never parsed, never ordered, never
|
|
assumed numeric.
|
|
- This is why the stability contract is load-bearing rather than cosmetic: per-step
|
|
credit (T5.10) and the whole attribution path depend on it.
|
|
|
|
## Steps
|
|
|
|
1. Take parent and child `WorkflowDef`s; compute the three sets: added, removed,
|
|
retained.
|
|
2. Emit the report as structured data, not a log line — it belongs on the version
|
|
record so it is auditable later.
|
|
3. Reject when `retained.is_empty()` and the child is not marked as a rewrite.
|
|
Name the count of added and removed ids in the error.
|
|
4. Add the rewrite marker as an explicit field on the workflow definition, so
|
|
marking it is a deliberate edit in the user's own file.
|
|
5. Audit for any code that parses, sorts numerically, or ranges over `StepId`.
|
|
Delete it — ordering comes from `transitions`, not from the id.
|
|
6. Test both directions: renumbering every id rejected by default, accepted with
|
|
the marker.
|
|
|
|
## Acceptance
|
|
|
|
- Renumbering every step id is **rejected by default** and **accepted with the
|
|
explicit marker**.
|
|
|
|
## Verify
|
|
|
|
**Harness:** version pairs committed as fixtures — parent and child — covering
|
|
each diff shape.
|
|
|
|
**Integration test** — `tests/it_stepid_stability.rs`:
|
|
1. **Renumber everything, no marker:** assert **rejected**, and that the error
|
|
names the added/removed counts.
|
|
2. **Renumber everything, marker set:** assert **accepted**.
|
|
3. **Insert a step at position 2, ids unchanged:** assert accepted, and that the
|
|
report shows 1 added, 0 removed, N retained. This is the case the whole
|
|
contract exists for.
|
|
4. **Rename one of five ids:** assert accepted with 1 added, 1 removed, 4
|
|
retained — a partial rename must not trip the all-or-nothing rule.
|
|
5. Duplicate id within one version: rejected.
|
|
6. Assert the report is persisted **on the version record**, readable after a
|
|
restart — not emitted only as a log line.
|
|
7. Opacity audit: grep the codebase for numeric parsing, sorting or ranging over
|
|
`StepId`; assert none. Add a `StepId` in a fixture that is non-numeric and
|
|
contains a `.`, and assert nothing breaks.
|
|
|
|
**Command:** `cargo test -p workflow stepid_stability`
|
|
|
|
**False pass:**
|
|
- Only testing steps 1 and 2. The rule "reject when retained is empty" is
|
|
trivially satisfiable by an implementation that computes nothing else — steps
|
|
3 and 4 are what prove the diff is real.
|
|
- Asserting the report exists in stdout. Credit assignment reads it later from
|
|
storage; step 6 is the check that matters.
|
|
- Fixtures whose ids are `step1`, `step2`, … so accidental numeric ordering is
|
|
indistinguishable from correct behaviour. Step 7's odd id is the guard.
|
|
|
|
## Traps
|
|
|
|
- Sorting steps by `StepId` for display and then depending on that order.
|
|
- Auto-generating `StepId` from position when the author omits one. That
|
|
guarantees the renumbering accident the check exists to catch.
|
|
|
|
---
|
|
|
|
Background (not required to do this task):
|
|
[rust-agentic-sys.md](../../../rust-agentic-sys.md) §4.3, §11.7 ·
|
|
[rust-agentic-task.md](../../../rust-agentic-task.md)
|