Continue agents after workflow transitions - #402
Conversation
tildesrc
left a comment
tildesrc
left a comment
There was a problem hiding this comment.
Code review — 8 findings (4 confirmed, then design edges and cleanups). Seven are inline below; one can't be anchored because the file isn't in the diff:
[CONFIRMED] Docs still teach the old contract this PR abolishes. AGENTS.md:267, docs/tasks.md:107, and terminal/dashboard.py:29/2292 all still say "advance starts a new agentic turn", while this PR's tests assert that wording is gone from the rendered skills and the new directive says the opposite ("continue in this same turn"). The operating manual CLAUDE.md binds in-repo agents to now contradicts the shipped behavior — a future adapter or the turn-flip-hook work built from the glossary would silently revert this fix. The ROADMAP definition-of-done calls for updating the manual with each slice.
🤖 Review by Claude Code
| A transition happens mid-model-turn, after the user-prompt hook emitted the old phase's | ||
| briefing. Returning the new one here closes that context gap for every MCP-capable agent CLI. | ||
| """ | ||
| briefing = await service.briefing_for(task) |
There was a problem hiding this comment.
[CONFIRMED] Fallible render after the commit → a retry double-advances. This await runs after the transition has persisted, and briefing_for → workflow.briefing → _briefing_extras reaches the filesystem artifact store. A transient OSError (or a bug in a path-discovered workflow's briefing extras) makes the tool return an error for a transition that actually succeeded — and the natural agent response, retrying advance, now resolves from the new state: a silent second hop (e.g. ITERATING→MERGING) or a baffling IllegalTransition. Before this PR nothing fallible ran post-commit on this path.
Suggestion: wrap the briefing render so a failure degrades to the bare task dict (no briefing key) instead of erroring the whole tool call.
There was a problem hiding this comment.
Fixed in f712fbc. now catches briefing-render exceptions, logs them, and returns the already-persisted bare task result without a key. Added an MCP regression test proving the tool succeeds and the transition remains persisted when raises.
| briefing. Returning the new one here closes that context gap for every MCP-capable agent CLI. | ||
| """ | ||
| briefing = await service.briefing_for(task) | ||
| if task.turn is Actor.AGENT: |
There was a problem hiding this comment.
[PLAUSIBLE] Gate on terminality too, not turn alone. The rendered skill text promises the directive fires when "the new phase is nonterminal and you hold its turn", but this condition checks only task.turn. Terminality is enforced only by the coincidence that TerminalState.turn_on_enter defaults to USER — it's an overridable ClassVar (core/state.py:52), workflow validation doesn't forbid a terminal state with turn_on_enter=AGENT, and discovery loads third-party workflows. Such a state would emit "it's finished; there's nothing to do" immediately followed by "continue its work immediately".
Workflow.is_terminal already exists — add and not workflow.is_terminal(task.state) (or validate terminal states' turn_on_enter at workflow build time).
There was a problem hiding this comment.
Fixed in f712fbc. The continuation predicate now explicitly checks via the task's active workflow, rather than relying on terminal states conventionally seeding .
| briefing = await service.briefing_for(task) | ||
| if task.turn is Actor.AGENT: | ||
| briefing += ( | ||
| "\n\nYou now hold the turn in this phase. Continue its work immediately in this " |
There was a problem hiding this comment.
[PLAUSIBLE] The directive misaddresses cross-task callers. "You now hold the turn… continue its work immediately" speaks to the tool caller, but apply_operation/set_state accept any task_id with no caller scoping (only the orchestration tools are gated to the acting orchestrator). An Orchestrator moving a child back with set_state(child_id, "ITERATING") (turn_on_enter=AGENT) gets told to do the child's coding work in its own container — which has neither the child's branch nor its workspace.
Cheapest fix: phrase it about the task, not the caller ("the task's agent now holds the turn — if that's you, continue…"); fuller fix is caller scoping on the transition tools.
There was a problem hiding this comment.
Add an optional acting_task to resolve this issue. This will be useful when access controls are added later anyway.
There was a problem hiding this comment.
Fixed in f712fbc. and now accept optional ; rendered operations pass their own task ID. The continue directive is emitted only when the acting task is omitted for backward compatibility or matches the transitioned task. Added cross-task coverage.
There was a problem hiding this comment.
Implemented in f712fbc. Both transition MCP tools expose optional , and generated operations send the current task ID in that field. It now controls whether the caller receives same-task continuation guidance and leaves a seam for later access-control enforcement.
| if task.turn is Actor.AGENT: | ||
| briefing += ( | ||
| "\n\nYou now hold the turn in this phase. Continue its work immediately in this " | ||
| "same turn; do not stop merely to wait for another user prompt." |
There was a problem hiding this comment.
[PLAUSIBLE] Second prose source for the same guidance. The continue-after-transition contract now lives in two places that must agree: this adapter-composed directive and its static paraphrase baked into every rendered skill file (skills.py) — and they already state different predicates (skill: "nonterminal and you hold its turn"; here: turn only). All other briefing prose composes in core Workflow.briefing. A post_transition=True variant on the core briefing render would keep one source; alternatively let the skill text say only "follow the returned briefing" and keep the conditional here.
There was a problem hiding this comment.
Fixed in f712fbc. The rendered skill now only tells the agent to follow the briefing returned by the tool. The conditional continuation guidance lives solely in , so there is one dynamic prose source and one predicate.
| f"don't edit the state directly. It's gated on the current state's responsibilities and " | ||
| f"starts a new turn.\n" | ||
| f"returns the entered phase's briefing. If the new phase is nonterminal and you hold its " | ||
| f"turn, follow that briefing and continue immediately.\n" |
There was a problem hiding this comment.
[CONFIRMED] This template also renders /drop, where both claims are wrong. write_operation_commands always includes drop (core/workflow.py:246), and the gate explicitly exempts it (core/workflow.py:555 — "always allowed, never gated"). So the rendered /drop says "It's gated on the current state's responsibilities" (false — an agent told to abandon a task may burn a turn resolving, or falsely marking MET, pending responsibilities first), and the new "If the new phase is nonterminal…" sentence is dead text since DROPPED is always terminal. The drop operation deserves its own text.
There was a problem hiding this comment.
Fixed in f712fbc. The shared operation renderer now gives dedicated text stating that it is always allowed and bypasses outstanding responsibilities; it contains neither gated nor continuation/briefing guidance. Added a regression test.
| f"don't edit the state directly. It's gated on the current state's responsibilities and " | ||
| f"starts a new turn.\n" | ||
| f"returns the entered phase's briefing. If the new phase is nonterminal and you hold its " | ||
| f"turn, follow that briefing and continue immediately.\n" |
There was a problem hiding this comment.
[CONFIRMED] Duplication grew: identical body pasted into both renderers. render_operation and render_agent_operation are byte-identical after the frontmatter line, and this PR applied the same two-line wording change to both plus a second test to guard the second copy. A missed copy next time silently gives claude and codex divergent operation instructions. The in-file precedent exists (_task_id_note shared by both) — extract a shared _operation_body(name, target_state, task_id) and have each renderer prepend its own frontmatter.
There was a problem hiding this comment.
Fixed in f712fbc. Extracted , shared byte-for-byte by the Claude and Codex renderers, and added a test asserting their procedure bodies remain identical.
| assert result.structuredContent["state"] == "ITERATING" # existing flat task shape remains | ||
| briefing = result.structuredContent["briefing"] | ||
| assert "**ITERATING**" in briefing | ||
| assert "[pending] plan-implemented" in briefing |
There was a problem hiding this comment.
[CONFIRMED] Re-pins briefing content owned by other tests. The [pending] plan-implemented line is already pinned by the briefing fixture (tests/fixtures/briefing/…ITERATING.md via test_briefing.py), and the plan.md artifact URI by test_github_self_reviewed.py:136 and test_service.py:608. A briefing-format tweak now breaks 3+ files. Keep the URI assertion if you want the artifact-store-threads-through-MCP integration proof, but drop the content pin — this test should assert only what mcp.py adds (the briefing key, the state header, the continue directive).
There was a problem hiding this comment.
Fixed in f712fbc. Removed the redundant responsibility-content assertion. The MCP integration test now focuses on the state header, MCP-added directive, and artifact URI threading.
|
Addressed the unanchored documentation finding in f712fbc. |
1 participant
Implements the task plan.md artifact.
Workflow transition tools return the entered phase briefing from the exact post-transition task. Same-task, agent-held nonterminal phases include an explicit instruction to continue immediately; optional acting-task identity prevents cross-task callers from receiving that directive. Briefing-render failures safely degrade to the persisted bare transition result.
Claude and Codex share one operation procedure, with correct ungated handling for drop. The operating manual and task/dashboard documentation describe the updated transition contract.