workflow: rejecting a draft and replaying it with feedback works again (#870)

CHANGELOG.md

@@ -4,6 +4,33 @@
 
 ### Fixes
 
+- **#870: rejecting a draft and replaying it with feedback works again.**
+  `resume_workflow(id, "before_draft", {"feedback": "..."})` on a waiting run
+  replays from that checkpoint with the payload visible to
+  `workflow_resume_payload()`, and parks at the wait again. Post-wait steps do
+  **not** run, so a side-effecting `publish` cannot fire on a rejection.
+
+  #482 (5.5.0) refused it, on the stated grounds that *"the rollback re-armed
+  the wait and the payload was silently discarded"*. The payload was not
+  discarded — measured on 4.0.8, where the spelling worked, it reached the
+  replayed step. What it did not do was **satisfy the wait**, which is a
+  different thing and is exactly what a reviewer rejecting a draft wants. The
+  5.5.0 entry is annotated rather than rewritten, since it is what that release
+  shipped.
+
+  The capability was never actually removed, only its spelling: the refusal's
+  condition includes `event_type is None`, so passing that **verification**
+  argument to the Python runner has skipped it the whole time. Relying on an
+  accident was the state of the art here, and the two-phase workaround people
+  found instead is worse — two calls, and it runs every post-wait step.
+
+  **`resume_workflow(id, "checkpoint")` with no payload is still refused**, and
+  that refusal is what #482 earned: a rollback re-enters the waiting step, which
+  waits again, so nothing changes behind a healthy-looking result. Its message
+  now names both recipes rather than only the advancing one. A checkpoint the
+  run has not reached, and a typo'd one, already failed with `Checkpoint not
+  found` and still do.
+
 - **#869: a run parked at `workflow_wait` no longer ages out of existence.**
   `LocalWorkflowStore` skipped any file whose mtime was older than
   `terminal_max_age_days` (default 30) *before opening it* — the only way to
@@ -5035,6 +5062,14 @@ odus-a2a-wire` "the local worktree" of the wire repo. That directory's
   administratively over a graph that ran past the wait (a stale registration)
   still resumes and clears the mark.
 
+  > **Corrected by #870.** "The payload was silently discarded" is wrong, and
+  > the error message repeated it. The payload *did* reach
+  > `workflow_resume_payload()` in the replayed step; what it did not do was
+  > **satisfy the wait**. On that false premise the refusal also covered
+  > `checkpoint + payload`, which is reject-and-revise and works — so this
+  > entry describes removing a no-op and in fact removed a working pattern too.
+  > Left as written, with this note, because it is what 5.5.0 shipped.
+
 - **#486: resuming from a mid-step checkpoint no longer double-counts folded
   state, and the re-entry rule is documented.** A resume re-enters the step
   that recorded the checkpoint from the top — that is the decided semantics,

docs/guide/real-world-integration.md

@@ -176,12 +176,45 @@ already written changes. Declaring no schema accepts anything, as before.
 
 > **A waiting run is advanced by a payload, not by a checkpoint.**
 > `resume_workflow(id, {payload})` satisfies the wait and the run moves on.
-> `resume_workflow(id, "checkpoint")` on a waiting run is **refused** with an
-> error naming the event the run is waiting on (#482) — a checkpoint rollback
-> re-enters the waiting step from the top, which re-arms the wait, so it could
-> only ever no-op (and with a payload alongside, discard it). Before the
-> refusal existed, that call returned a healthy-looking `"status": "waiting"`
-> result and silently did nothing.
+> `resume_workflow(id, "checkpoint")` — a checkpoint *alone* — is **refused**
+> (#482): a rollback re-enters the waiting step from the top, which re-arms the
+> wait, so the call could only ever no-op. Before the refusal existed it
+> returned a healthy-looking `"status": "waiting"` result and silently did
+> nothing.
+
+### Rejecting a draft: replay with feedback
+
+The other thing a reviewer wants is not "advance" but **"go back and try
+again"**. Pass the checkpoint *and* a payload:
+
+```python
+resume_nd(graph_id, "before_draft", {"feedback": "too formal, try again"})
+```
+
+The run replays from that checkpoint, the replayed step reads the feedback
+through `workflow_resume_payload()`, and the flow parks at its wait again ready
+for the next review. Post-wait steps do **not** run, so a side-effecting
+`publish` cannot fire on a rejection.
+
+The two calls answer different questions, and the checkpoint argument is what
+distinguishes them:
+
+| Call | Meaning |
+|---|---|
+| `resume_workflow(id, {...})` | approve: satisfy the wait, run the rest |
+| `resume_workflow(id, "cp", {...})` | reject: replay from `cp` with feedback, park again |
+| `resume_workflow(id, "cp")` | refused — nothing would change |
+
+A checkpoint the run has not reached yet, or a typo, fails with
+`Checkpoint not found` rather than doing something surprising.
+
+> This was refused between 5.5.0 and 5.14.0 (#870). The refusal's stated reason
+> was that the payload would be "silently discarded", which was not what
+> happened — the payload reached the replayed step; what it did not do was
+> satisfy the wait. If you are on one of those versions, the workaround is two
+> calls: satisfy the wait with a payload your post-wait steps gate on, then roll
+> back. Note that this runs every post-wait step, which is why it is a
+> workaround and not the recipe.
 
 The `gate` step is a pure orchestration step — it does nothing except park the
 flow until the external signal arrives. The `execute` step reads the payload

src/nodus_lang_workflow/runner.py

@@ -1219,13 +1219,29 @@ def resume_workflow(
             # re-enters the waiting step, which hits its `workflow_wait` again --
             # the run goes straight back to `waiting` and the result looks
             # healthy, so a caller checking for an error sees success while
-            # nothing happened. With a payload it is worse: the rollback re-arms
-            # the wait and the payload is silently discarded (a wait is a
-            # sentinel the engine pauses on; nothing consults the pending payload
-            # when a re-run step re-arms it). The call that advances a waiting
-            # run delivers a payload *without* a checkpoint. Refuse both no-op
-            # combinations with the real reason, the way #399 and #425 replaced
-            # misleading answers on this same path.
+            # nothing happened. That is a real no-op and is still refused.
+            #
+            # #870: but the same refusal covered `checkpoint` **with** a payload,
+            # on the stated grounds that "the payload is silently discarded". It
+            # is not, and was not: the payload reaches
+            # `workflow_resume_payload()` in the replayed step. What it does not
+            # do is *satisfy the wait* -- the replay re-arms it -- which is a
+            # different thing and is exactly what a reviewer rejecting a draft
+            # wants: replay from before the draft with the feedback, then park
+            # for approval again.
+            #
+            # Measured on 4.0.8, where the spelling worked, and on 5.14.0 through
+            # `event_type=` (which skips this branch and always has), the run
+            # replays with the payload visible, post-wait steps do NOT fire, and
+            # the run re-parks. So #482 removed a spelling and left the
+            # capability reachable only by passing a *verification* argument --
+            # an accident, not a design. The two-phase workaround people found
+            # instead is worse: it takes two calls and runs every post-wait step,
+            # so a side-effecting step fires on a rejection unless it gates.
+            #
+            # The neighbouring mistakes are already refused elsewhere and do not
+            # need this branch: a checkpoint the run has not reached, and a
+            # typo'd one, both fail with "Checkpoint not found".
             #
             # "Genuinely" is the persisted graph state agreeing the run is
             # waiting. A record marked waiting administratively
@@ -1234,29 +1250,23 @@ def resume_workflow(
             # doing so.
             if (
                 checkpoint is not None
+                and resume_payload is None
                 and event_type is None
                 and record is not None
                 and record.status == RUN_STATUS_WAITING
                 and isinstance(state, dict)
                 and state.get("status") == "waiting"
             ):
                 wait_event = record.wait.event_type if record.wait is not None else None
-                if resume_payload is None:
-                    detail = (
-                        f"pass a payload to satisfy it -- "
-                        f"resume_workflow(graph_id, {{...}}) -- and the run "
-                        f"will advance. Resuming from checkpoint "
-                        f"'{checkpoint}' alone re-enters the waiting step, "
-                        f"which waits again."
-                    )
-                else:
-                    detail = (
-                        f"drop the checkpoint argument -- "
-                        f"resume_workflow(graph_id, {{...}}) -- and the payload "
-                        f"will satisfy it. Rolling back to checkpoint "
-                        f"'{checkpoint}' re-enters the waiting step, which "
-                        f"re-arms the wait and discards the payload."
-                    )
+                detail = (
+                    f"pass a payload to satisfy it -- "
+                    f"resume_workflow(graph_id, {{...}}) -- and the run "
+                    f"will advance. Resuming from checkpoint "
+                    f"'{checkpoint}' alone re-enters the waiting step, "
+                    f"which waits again. To reject and replay instead, pass "
+                    f"the feedback with the checkpoint -- "
+                    f"resume_workflow(graph_id, '{checkpoint}', {{...}})."
+                )
                 return {
                     "ok": False,
                     "error": (

tests/eval/release_claims_probe.py

@@ -734,18 +734,72 @@ def probe_topology_validation():
     return "matching shape passes; a drifted one names the real cause, not a cycle"
 
 
-@probe("5.4.0: a checkpoint resume of a waiting run is refused")
+@probe("5.4.0: a checkpoint resume of a waiting run is refused when it would no-op")
 def probe_waiting_resume_refused():
     from nodus_lang_workflow import runner as runner_module
 
     source = inspect_source(runner_module.WorkflowFrameworkRunner.resume_workflow)
     assert "waiting_run_checkpoint_resume" in source, "the refusal is gone"
-    assert "discards the payload" in source, "the payload-eating case is unguarded"
     assert 'state.get("status") == "waiting"' in source, (
         "the refusal no longer consults the persisted state, so a stale "
         "administrative wait would be refused"
     )
-    return "refused on genuinely-waiting runs only, both argument shapes"
+    # #870: the refusal is scoped to the argument shape that really does
+    # nothing. It used to cover `checkpoint + payload` too, on the stated
+    # grounds that the rollback "discards the payload" -- which it does not: the
+    # payload reaches the replayed step, it simply does not satisfy the wait.
+    # This probe asserted that false sentence was PRESENT in the source, so the
+    # premise was pinned in three places at once (changelog, error message,
+    # here).
+    assert "and resume_payload is None" in source, (
+        "the refusal is not scoped to the no-payload case, so reject-and-revise "
+        "is blocked again (#870)"
+    )
+    assert "discards the payload" not in source, (
+        "the refusal still claims the rollback discards the payload; it reaches "
+        "the replayed step (#870)"
+    )
+    return "refused only with no payload; checkpoint+payload replays (#870)"
+
+
+@probe("5.15.0: rejecting a draft replays it with the feedback (#870)")
+def probe_reject_and_revise():
+    # The behavioural half. The probe above reads the source, and a source
+    # assertion cannot tell you the capability works -- which is how the false
+    # "discards the payload" premise survived: it was asserted as a *string*.
+    source = (
+        "workflow w {\n"
+        '    step draft { checkpoint "before_draft"; '
+        "let p = workflow_resume_payload(); return {\"fb\": str(p)} }\n"
+        '    step gate after draft { return workflow_wait("approve", "k1", {}) }\n'
+        '    step publish after gate { return {"published": true} }\n'
+        "}\n"
+        "fn main() {\n"
+        "    let r = run_workflow(w)\n"
+        "    let g = r[\"graph_id\"]\n"
+        '    let back = resume_workflow(g, "before_draft", {"feedback": "redo"})\n'
+        '    let st = back["status"]\n'
+        '    let steps = back["steps"]\n'
+        '    let d = steps["draft"]\n'
+        '    print("STATUS=\\(st) DRAFT=\\(d)")\n'
+        '    let done = resume_workflow(g, {"approved": true})\n'
+        '    let ds = done["steps"]\n'
+        '    let pub = ds["publish"]\n'
+        '    print("PUB=\\(pub)")\n'
+        "}"
+    )
+    out = run_nd(source)
+    assert out.get("ok"), f"reject-and-revise failed: {out.get('error')}"
+    text = out.get("stdout") or ""
+    assert "STATUS=waiting" in text, f"the run did not park again: {text[:200]}"
+    assert "redo" in text, (
+        f"the replayed step did not see the feedback -- the exact claim #482 "
+        f"was refused on: {text[:200]}"
+    )
+    assert "PUB=" in text and "published" in text, (
+        f"the run could not be approved after a rejection: {text[:200]}"
+    )
+    return "replays with the feedback, parks again, then approves"
 
 
 @probe("5.4.0: a persist failure names the cell, and durable:false protects it")
@@ -3243,6 +3297,7 @@ def main() -> int:
     probe_graph_does_not_execute(args.repo)
     probe_topology_validation()
     probe_waiting_resume_refused()
+    probe_reject_and_revise()
     probe_persist_naming()
     probe_goal_waypoint()
     probe_check_enters_steps()