From 9544ca027de7bc3d71db409f3eb0a051cb5d1ff3 Mon Sep 17 00:00:00 2001 From: Arsalan Shahid Date: Mon, 21 Sep 2026 13:41:25 +0100 Subject: [PATCH] SPECIFICATION 8.1: say where control.resume leaves the task, and read the To column The control.resume To cell said in_progress, which is the result only where no prior state was captured. Since #144 the task returns to the state it held at the pause, so the cell was false for every task paused from anywhere else. It was safe to leave wrong because the lifecycle test read the From column for every method and the To column for task.update alone: substituting nonsense into that cell left all 23 tests passing. The cell now names the outcomes: the state held at the pause, one of created, in_progress, review_requested, abstained or escalated, and in_progress where none was captured. The test reads the To column for every method. A state named in a cell must be one the method produces, and a state a method produces must be named. Outcomes that a starting state alone does not settle are driven by their own scenarios: a quorum rule not yet satisfied, a rejection sent back for revision, a completion that opens a review, and a resume from each state a pause can be entered from. Reverting the cell to in_progress now fails in both references, as does dropping the alternative outcome from the decide.approve cell. control.md 2 gains the same statement in prose. --- SPECIFICATION.md | 12 +- .../tests/test_spec_lifecycle_table.py | 108 ++++++++++++++++-- .../tests/spec_lifecycle_table.test.ts | 99 +++++++++++++++- profiles/control.md | 7 ++ 4 files changed, 206 insertions(+), 20 deletions(-) diff --git a/SPECIFICATION.md b/SPECIFICATION.md index 7cdd890..76c31c0 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -663,7 +663,7 @@ categories, because several methods carry preconditions of their own. | review_requested | `abstain.declare` | abstained | | created, in_progress, review_requested, declined, abstained, escalated, paused | `escalate.raise` | escalated | | created, in_progress, review_requested, abstained, escalated, paused | `control.pause` | paused | -| paused | `control.resume` | in_progress | +| paused | `control.resume` | the state held at the pause: created, in_progress, review_requested, abstained or escalated; in_progress where none was captured | | created, in_progress, review_requested, abstained, escalated, paused | `control.cancel` | cancelled | | any state | `control.supersede` | superseded | @@ -688,11 +688,11 @@ Preconditions that are narrower or wider than "non-terminal": the workspace's own profile set could not lift. - **`control.pause` on a paused task succeeds and changes nothing.** - **`control.resume` restores the state captured at the corresponding - `control.pause`.** The `To` column above shows `in_progress`, which is the - result when no prior state was captured (for example a task resumed from a - snapshot written before this was recorded); otherwise the task returns to the - state it held when it was paused, so a review paused mid-flight is actionable - again on resume. + `control.pause`.** A review paused mid-flight is therefore actionable again + on resume. `in_progress` is the result only where no prior state was + captured, for example a task resumed from a snapshot written before this was + recorded. A second `control.pause` on an already paused task captures + nothing, so the state held at the first pause is the one restored. - **`task.update` MUST NOT complete a task that requires review.** `in_progress → completed` is otherwise legal, so without this the review could be skipped: the task would finish carrying no artefact and no diff --git a/packages/coordinator-py/tests/test_spec_lifecycle_table.py b/packages/coordinator-py/tests/test_spec_lifecycle_table.py index 8903c5e..f8685e5 100644 --- a/packages/coordinator-py/tests/test_spec_lifecycle_table.py +++ b/packages/coordinator-py/tests/test_spec_lifecycle_table.py @@ -8,7 +8,17 @@ This test reads it. It drives a task into every state, attempts every state-changing method from each, and requires the result to match the table exactly: a transition the table permits must work, and one it does not list -must be refused. The same check runs against the TypeScript coordinator in +must be refused. + +Both columns are read. The From column says where a method may be called, and +the To column says where it leaves the task. A state named in a To cell must be +one the method actually produces, and a state a method produces must be named, +so a cell cannot describe an outcome the coordinator does not have. Rows whose +outcome depends on more than the starting state, a review rule not yet +satisfied, a rejection sent back for revision, the state a pause captured, are +driven by the scenarios in OUTCOMES below. + +The same check runs against the TypeScript coordinator in packages/coordinator/tests/spec_lifecycle_table.test.ts, so the two implementations and the specification move together or not at all. """ @@ -46,9 +56,19 @@ def _table_rows(): and not set(r[0]) <= set("-| ")] +def _states_named_in(cell: str) -> set[str]: + """The states a To cell names, whatever prose surrounds them. + + A cell may carry an explanation beside the outcome, as + "completed once the review rule is satisfied, otherwise review_requested" + does. What is normative is which states it names. + """ + return {s for s in STATES if re.search(rf"\b{s}\b", cell)} + + def spec_machine(): - """(permitted, update_targets) as the specification describes them.""" - permitted, update_targets = {}, {} + """(permitted, update_targets, outcomes) as the specification describes them.""" + permitted, update_targets, outcomes = {}, {}, {} for froms, method_cell, to_cell in _table_rows(): method = re.search(r"`([a-z_]+\.[a-z_]+)`", method_cell).group(1) refused = to_cell.strip().startswith("refused") @@ -66,7 +86,10 @@ def spec_machine(): t.strip() for t in to_cell.split(",")) else: permitted.setdefault(method, set()).add(state) - return permitted, update_targets + named = _states_named_in(to_cell) + assert named, f"§8.1 gives {method} a To cell that names no state: {to_cell}" + outcomes.setdefault(method, set()).update(named) + return permitted, update_targets, outcomes # ------------------------------------------------------------ the coordinator @@ -136,8 +159,52 @@ def _attempt(send, method, tid): return calls[method]() +# Outcomes a starting state alone does not settle: a review rule not yet +# satisfied, a rejection sent back for revision, the state a pause captured. +# Each entry drives the scenario and returns the method it exercised, so the +# state the task lands in is attributed to that method. +def _outcome_scenarios(): + def review_rule_unsatisfied(send): + tid = send("task.create", kind="k", input={}, assignee=AGENT)["result"]["task_id"] + send("review.request", task_id=tid, artefact=ARTEFACT, to=[HUMAN, OTHER], + rule="quorum:2") + send("decide.approve", actor=HUMAN, task_id=tid, comment="ok", rationale="ok") + return "decide.approve", tid + + def rejection_sent_back(send): + tid = send("task.create", kind="k", input={}, assignee=AGENT)["result"]["task_id"] + send("review.request", task_id=tid, artefact=ARTEFACT, to=HUMAN) + send("decide.reject", actor=HUMAN, task_id=tid, comment="no", rationale="no", + request_revision=True) + return "decide.reject", tid + + def completion_opening_a_review(send): + tid = send("task.create", kind="k", input={}, assignee=AGENT, + review_required=True)["result"]["task_id"] + send("task.complete", task_id=tid, output=ARTEFACT) + return "task.complete", tid + + def resume_from(origin): + def run(send): + tid = _drive(send, origin) + send("control.pause", actor=HUMAN, task_id=tid, reason="hold") + send("control.resume", actor=HUMAN, task_id=tid) + return "control.resume", tid + run.__name__ = f"resume_from_{origin}" + return run + + scenarios = [review_rule_unsatisfied, rejection_sent_back, + completion_opening_a_review] + # control.pause names the states a pause can be entered from, so those are + # the states a resume can restore. + scenarios += [resume_from(origin) for origin in + ("created", "in_progress", "review_requested", "abstained", + "escalated")] + return scenarios + + def implemented_machine(): - permitted, update_targets = {}, {} + permitted, update_targets, outcomes = {}, {}, {} for state in STATES: c, send = _ready() tid = _drive(send, state) @@ -148,12 +215,19 @@ def implemented_machine(): tid2 = _drive(send2, state) if "error" not in _attempt(send2, method, tid2): permitted.setdefault(method, set()).add(state) + outcomes.setdefault(method, set()).add( + c2.get_workspace("w").tasks[tid2].state) for target in STATES: c3, send3 = _ready() tid3 = _drive(send3, state) if "error" not in send3("task.update", task_id=tid3, state=target): update_targets.setdefault(state, set()).add(target) - return permitted, update_targets + for scenario in _outcome_scenarios(): + c4, send4 = _ready() + method, tid4 = scenario(send4) + outcomes.setdefault(method, set()).add( + c4.get_workspace("w").tasks[tid4].state) + return permitted, update_targets, outcomes # ------------------------------------------------------------------ the check @@ -165,14 +239,15 @@ def machines(): def test_the_table_is_not_empty(machines): # Without this the comparisons below would pass by having nothing to compare. - (spec_permitted, spec_updates), _ = machines + (spec_permitted, spec_updates, spec_outcomes), _ = machines assert len(spec_permitted) >= 8, "SPECIFICATION.md 8.1 parsed to almost nothing" assert spec_updates, "no task.update rows found in 8.1" + assert len(spec_outcomes) >= 8, "the To column of 8.1 parsed to almost nothing" @pytest.mark.parametrize("method", METHODS) def test_the_table_matches_the_coordinator(method, machines): - (spec_permitted, _), (real_permitted, _) = machines + (spec_permitted, _, _), (real_permitted, _, _) = machines documented = spec_permitted.get(method, set()) actual = real_permitted.get(method, set()) assert documented == actual, ( @@ -186,7 +261,7 @@ def test_the_table_matches_the_coordinator(method, machines): @pytest.mark.parametrize("state", STATES) def test_the_task_update_rows_match_the_transition_map(state, machines): - (_, spec_updates), (_, real_updates) = machines + (_, spec_updates, _), (_, real_updates, _) = machines assert spec_updates.get(state, set()) == real_updates.get(state, set()), ( f"SPECIFICATION.md 8.1 and task.update disagree about {state}.\n" f" the table says : {sorted(spec_updates.get(state, set())) or 'nothing'}\n" @@ -194,6 +269,21 @@ def test_the_task_update_rows_match_the_transition_map(state, machines): ) +@pytest.mark.parametrize("method", METHODS) +def test_the_to_column_matches_where_the_coordinator_leaves_the_task(method, machines): + (_, _, documented), (_, _, actual) = machines + named = documented.get(method, set()) + reached = actual.get(method, set()) + assert named == reached, ( + f"SPECIFICATION.md 8.1 and the coordinator disagree about where " + f"{method} leaves the task.\n" + f" the To column names : {sorted(named) or 'nothing'}\n" + f" the coordinator reaches: {sorted(reached) or 'nothing'}\n" + f" named but unreachable: {sorted(named - reached) or 'none'}\n" + f" reached but unnamed : {sorted(reached - named) or 'none'}" + ) + + def test_the_table_names_no_method_the_coordinator_lacks(): # The table named task.assign, task.accept and task.start, which no # coordinator implements. A named method must be dispatchable. diff --git a/packages/coordinator/tests/spec_lifecycle_table.test.ts b/packages/coordinator/tests/spec_lifecycle_table.test.ts index b7349d8..3047777 100644 --- a/packages/coordinator/tests/spec_lifecycle_table.test.ts +++ b/packages/coordinator/tests/spec_lifecycle_table.test.ts @@ -7,8 +7,17 @@ * * This test reads it. It drives a task into every state, attempts every * state-changing method from each, and requires the result to match the table - * exactly. Mirrors packages/coordinator-py/tests/test_spec_lifecycle_table.py, - * so the two implementations and the specification move together or not at all. + * exactly. + * + * Both columns are read. The From column says where a method may be called, + * and the To column says where it leaves the task. A state named in a To cell + * must be one the method actually produces, and a state a method produces must + * be named. Rows whose outcome depends on more than the starting state, a + * review rule not yet satisfied, a rejection sent back for revision, the state + * a pause captured, are driven by outcomeScenarios below. + * + * Mirrors packages/coordinator-py/tests/test_spec_lifecycle_table.py, so the + * two implementations and the specification move together or not at all. */ import { test } from "node:test"; import assert from "node:assert/strict"; @@ -44,9 +53,20 @@ function tableRows(): [string, string, string][] { return rows; } +/** + * The states a To cell names, whatever prose surrounds them. A cell may carry + * an explanation beside the outcome, as "completed once the review rule is + * satisfied, otherwise review_requested" does. What is normative is which + * states it names. + */ +function statesNamedIn(cell: string): string[] { + return STATES.filter(s => new RegExp(`\\b${s}\\b`).test(cell)); +} + function specMachine() { const permitted = new Map>(); const updateTargets = new Map>(); + const outcomes = new Map>(); for (const [froms, methodCell, toCell] of tableRows()) { const method = /`([a-z_]+\.[a-z_]+)`/.exec(methodCell)![1]; const refused = toCell.trim().startsWith("refused"); @@ -65,10 +85,16 @@ function specMachine() { updateTargets.set(state, set); } else { permitted.set(method, (permitted.get(method) ?? new Set()).add(state)); + const named = statesNamedIn(toCell); + assert.ok(named.length > 0, + `§8.1 gives ${method} a To cell that names no state: ${toCell}`); + const set = outcomes.get(method) ?? new Set(); + for (const n of named) set.add(n); + outcomes.set(method, set); } } } - return { permitted, updateTargets }; + return { permitted, updateTargets, outcomes }; } // ------------------------------------------------------------ the coordinator @@ -131,9 +157,51 @@ function attempt(send: any, method: string, id: string) { } } +/** + * Outcomes a starting state alone does not settle: a review rule not yet + * satisfied, a rejection sent back for revision, the state a pause captured. + * Each scenario drives itself and names the method it exercised, so the state + * the task lands in is attributed to that method. + */ +function outcomeScenarios(): ((send: any) => [string, string])[] { + const resumeFrom = (origin: string) => (send: any): [string, string] => { + const id = drive(send, origin); + send("control.pause", { task_id: id, reason: "hold" }, HUMAN); + send("control.resume", { task_id: id }, HUMAN); + return ["control.resume", id]; + }; + return [ + (send): [string, string] => { + const id = send("task.create", { kind: "k", input: {}, assignee: AGENT }).result.task_id; + send("review.request", { task_id: id, artefact: ARTEFACT, to: [HUMAN, OTHER], + rule: "quorum:2" }); + send("decide.approve", { task_id: id, comment: "ok", rationale: "ok" }, HUMAN); + return ["decide.approve", id]; + }, + (send): [string, string] => { + const id = send("task.create", { kind: "k", input: {}, assignee: AGENT }).result.task_id; + send("review.request", { task_id: id, artefact: ARTEFACT, to: HUMAN }); + send("decide.reject", { task_id: id, comment: "no", rationale: "no", + request_revision: true }, HUMAN); + return ["decide.reject", id]; + }, + (send): [string, string] => { + const id = send("task.create", { kind: "k", input: {}, assignee: AGENT, + review_required: true }).result.task_id; + send("task.complete", { task_id: id, output: ARTEFACT }); + return ["task.complete", id]; + }, + // control.pause names the states a pause can be entered from, so those are + // the states a resume can restore. + ...["created", "in_progress", "review_requested", "abstained", "escalated"] + .map(origin => resumeFrom(origin)), + ]; +} + function implementedMachine() { const permitted = new Map>(); const updateTargets = new Map>(); + const outcomes = new Map>(); for (const state of STATES) { { const { c, send } = ready(); @@ -142,10 +210,12 @@ function implementedMachine() { `the fixture for ${state} did not reach it`); } for (const method of METHODS) { - const { send } = ready(); + const { c, send } = ready(); const id = drive(send, state); if (attempt(send, method, id).error === undefined) { permitted.set(method, (permitted.get(method) ?? new Set()).add(state)); + outcomes.set(method, (outcomes.get(method) ?? new Set()) + .add((c as any).workspaces.get("w").tasks.get(id).state)); } } for (const target of STATES) { @@ -156,7 +226,13 @@ function implementedMachine() { } } } - return { permitted, updateTargets }; + for (const scenario of outcomeScenarios()) { + const { c, send } = ready(); + const [method, id] = scenario(send); + outcomes.set(method, (outcomes.get(method) ?? new Set()) + .add((c as any).workspaces.get("w").tasks.get(id).state)); + } + return { permitted, updateTargets, outcomes }; } const spec = specMachine(); @@ -169,6 +245,7 @@ test("the table is not empty", () => { // Without this the comparisons below would pass by having nothing to compare. assert.ok(spec.permitted.size >= 8, "SPECIFICATION.md 8.1 parsed to almost nothing"); assert.ok(spec.updateTargets.size > 0, "no task.update rows found in 8.1"); + assert.ok(spec.outcomes.size >= 8, "the To column of 8.1 parsed to almost nothing"); }); for (const method of METHODS) { @@ -182,6 +259,18 @@ for (const method of METHODS) { }); } +for (const method of METHODS) { + test(`the To column matches where ${method} leaves the task`, () => { + const named = sorted(spec.outcomes.get(method)); + const reached = sorted(real.outcomes.get(method)); + assert.deepEqual(named, reached, + `SPECIFICATION.md 8.1 and the coordinator disagree about where ${method} ` + + `leaves the task.\n` + + ` the To column names : ${named.join(", ") || "nothing"}\n` + + ` the coordinator reaches: ${reached.join(", ") || "nothing"}`); + }); +} + for (const state of STATES) { test(`the task.update row for ${state} matches the transition map`, () => { assert.deepEqual(sorted(spec.updateTargets.get(state)), sorted(real.updateTargets.get(state)), diff --git a/profiles/control.md b/profiles/control.md index 832a5a5..a5b1910 100644 --- a/profiles/control.md +++ b/profiles/control.md @@ -32,6 +32,13 @@ log as a first-class entry. | `participant` | A specific participant stops being assigned new tasks; in-flight tasks complete by default. | | `workspace` | The whole workspace stops accepting new tasks. | +At task scope, `control.resume` returns the task to the state it held at the +`control.pause`, so a review paused mid-flight is actionable again rather than +arriving back as `in_progress` with the review stranded. A second pause on an +already paused task captures nothing, so one resume clears it. Where no state +was captured, a task restored from a store written before this was recorded, +the task resumes to `in_progress`. SPECIFICATION 8.1 carries the table. + ```json { "method": "control.pause",