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",