Skip to content

Commit 1042db1

Browse files
committed
Report submit_output from complete() and drop autoAdvance
The handler reconstructed the cursor before the director mutated on tool.done, so parallel submit_output could both claim an advance. complete() now returns already-complete vs not-current and the handler reports that result. autoAdvance is removed from the plugin contract because the coordinator no longer reads it.
1 parent dafa078 commit 1042db1

14 files changed

Lines changed: 117 additions & 68 deletions

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,13 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
1313

1414
## [Unreleased]
1515

16+
### Changed
17+
18+
- Completing a workflow step is a `submit_output` tagged with that step's id.
19+
`advance_workflow` is gone. Already-complete and not-current ids are
20+
acknowledged without advancing. The unused `autoAdvance` workflow field is
21+
removed.
22+
1623
### Fixed
1724

1825
- Failed sessions with an `error` string in `run.json` are valid resume

docs/ARCHITECTURE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ Compaction replaces older turns with a structured, workflow-aware summary rather
173173

174174
- `ask_operator` — Pauses for a clarifying question with a list of options.
175175
- `present` — Renders structured UI from a JSON view spec instead of pasting tables into chat.
176-
- `submit_output` — Completes a workflow step when `step` is set (observed by the workflow coordinator). The step id is compared atomically against the current step; duplicate or stale ids are acknowledged without advancing. Always advertised so activating a workflow does not grow the tools array.
176+
- `submit_output` — Completes a workflow step when `step` is set. The step id is compared atomically against the current step (`complete()`); already-complete ids (behind the cursor) and not-current ids (future or unknown) are acknowledged without advancing. Always advertised so activating a workflow does not grow the tools array.
177177

178178
Core agent tools (advertised in every chat turn) include `manage_tasks`, `tool_search`, `use_skill`, **`task`** (spawn a sub-agent), and **`search_agents`** when sub-agent profiles are available — see Sub-agents below.
179179

@@ -185,7 +185,7 @@ Workflows are named, ordered recipes the agent follows step by step — a thin l
185185
- `capabilities.ts``detectCapabilities` maps the live tool surface to abstract capabilities (`ticket-tracker`, `code-host`, `doc-search`) by name pattern; `resolveStep` decides whether a step runs. A capability override set forces integrations off per run. Adding a capability is a data edit, not a logic change.
186186
- `runtime.ts``WorkflowRuntime` drives execution on a call stack: it skips capability-unsatisfied steps, descends into sub-workflow references, emits step lifecycle events, and snapshots `WorkflowState`. `state.ts` persists that snapshot atomically to `workflow.json` under the session state root for resume.
187187

188-
- `coordinator.ts` — bridges runtime and director: produces the `[WORKFLOW STEP i/total: label]` directive injected into each turn's system prompt, and compare-and-advances the runtime when a `submit_output` tagged `{ step }` completes. Shared by both directors. Fresh and resumed runs share one listener path.
188+
- `coordinator.ts` — bridges runtime and director: produces the `[WORKFLOW STEP i/total: label]` directive injected into each turn's system prompt, and compare-and-advances the runtime when a `submit_output` tagged `{ step }` completes. Already-complete and not-current ids are acknowledged without moving the cursor. Shared by both directors. Fresh and resumed runs share one listener path.
189189
- The built-in recipes: the atomics `update-ticket`, `improve-docs`, `write-tests`, `triage-bug`, `code-review`, `scope-project`, and the `build-feature` composite that chains them.
190190

191191
Invocation: workflows are **not** top-level slash commands. Recipe definitions load into the `WORKFLOWS` registry from **enabled workflow/command plugins** at startup; command surfaces on those plugins (e.g. a workflow plugin's command prefix such as `/mywf scope`). Slash commands may also be authored as data-only markdown (`commands/*.md`, no `index.ts`); see PLUGINS.md. The model never suggests or auto-starts workflows from ordinary chat. Skills (bundled `corbits-skills`, enabled plugins, or `.agents/skills/`) load on demand via `use_skill` or as `/<skill-name>` slash commands when `user-invocable` is not `false` (see Skills below). The TUI surfaces state via `src/tui/workflow-controller.ts` (lifecycle, capability overrides, resume) — the header shows step progress (`⟳ name · step/total label`).

src/agent/director.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,8 @@ export const submitOutputDefinition: ToolDefinition = {
247247
name: "submit_output",
248248
description:
249249
"Call this when the task is fully complete (include summary) or to complete " +
250-
"a workflow step (step id is required to advance; duplicate or stale step " +
251-
"ids are acknowledged without advancing).",
250+
"a workflow step (step id is required to advance; already-complete and " +
251+
"not-current step ids are acknowledged without advancing).",
252252
inputSchema: {
253253
type: "object",
254254
properties: {

src/agent/tools.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import { sessionModeEnablesSubAgents } from "../config/session-mode.js";
3434
import { advertisedToolNamesForSessionMode, type ToolAvailability } from "./tool-search.js";
3535
import type { ProviderCatalogEntry } from "../config/index.js";
3636
import type { AgentProfile } from "./profiles.js";
37+
import type { WorkflowCompleteResult } from "../workflows/types.js";
3738
import {
3839
createTaskTool,
3940
runSubAgent,
@@ -136,12 +137,10 @@ export interface AgentToolsetArgs {
136137
// every turn (workflow or not), so the model can call it with nothing active;
137138
// this lets its handler report an honest no-op instead of a false advance.
138139
isWorkflowActive?: () => boolean;
139-
// Current workflow step id, read live so the handler can distinguish a
140-
// matching complete from a duplicate, stale, or future id without advancing.
141-
getCurrentWorkflowStepId?: () => string | null;
142-
// True when `stepId` is behind the cursor in the active workflow frame.
143-
// Omitted (tests, exec) treats a non-current id as unknown/future, not stale.
144-
isPastWorkflowStep?: (stepId: string) => boolean;
140+
// Compare-and-advance the live workflow. The handler reports this result
141+
// instead of reconstructing the cursor; omitted (exec, tests) never claims
142+
// an advance.
143+
completeWorkflowStep?: (stepId: string) => WorkflowCompleteResult;
145144
// Primary session mode (always orchestrator; kept for call-site wiring).
146145
sessionMode?: SessionMode;
147146
// Session-start facts gating lsp advertisement. Omitted callers (tests,
@@ -467,11 +466,11 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise<AgentT
467466
}),
468467
stringTool({
469468
definition: submitOutputDefinition,
470-
// The director observes this call and compare-and-advances the workflow
471-
// runtime; the handler only acknowledges so the model gets a clean tool
472-
// result. Duplicate, stale, and future step ids succeed without claiming
473-
// an advance. Copy distinguishes those cases so a future id is not
474-
// reported as already complete.
469+
// The director also observes this call on tool.done; complete() is
470+
// compare-and-advance so a second pass is a no-op. The handler reports
471+
// complete()'s result so parallel submit_output cannot both claim an
472+
// advance. Already-complete and not-current ids succeed without
473+
// claiming one.
475474
handler: async (rawArgs: Record<string, unknown>): Promise<string> => {
476475
const parsed = SubmitOutputArgs(rawArgs);
477476
const step = parsed instanceof type.errors ? undefined : parsed.step;
@@ -481,12 +480,12 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise<AgentT
481480
if (step === undefined || step.length === 0) {
482481
return "Error: workflow completion requires a step identifier.";
483482
}
484-
const current = args.getCurrentWorkflowStepId?.() ?? null;
485-
if (current === step) {
483+
const result = args.completeWorkflowStep?.(step) ?? "not-current";
484+
if (result === "advanced") {
486485
const note = summary !== undefined && summary.length > 0 ? ` (${summary})` : "";
487486
return `Workflow step marked complete${note}. Advancing to the next step.`;
488487
}
489-
if (args.isPastWorkflowStep?.(step) === true) {
488+
if (result === "already-complete") {
490489
return "This workflow step is already complete. No advance.";
491490
}
492491
return "This workflow step is not current. No advance.";

src/director.test.ts

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -825,8 +825,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
825825
describe("submit_output workflow handler", () => {
826826
const buildToolset = (opts: {
827827
isWorkflowActive: () => boolean;
828-
getCurrentWorkflowStepId?: () => string | null;
829-
isPastWorkflowStep?: (stepId: string) => boolean;
828+
completeWorkflowStep?: (stepId: string) => "advanced" | "already-complete" | "not-current";
830829
}) =>
831830
createAgentToolset({
832831
cwd: process.cwd(),
@@ -837,11 +836,8 @@ describe("submit_output workflow handler", () => {
837836
}),
838837
onOperatorGate: async () => ({ kind: "cancel" }),
839838
isWorkflowActive: opts.isWorkflowActive,
840-
...(opts.getCurrentWorkflowStepId !== undefined
841-
? { getCurrentWorkflowStepId: opts.getCurrentWorkflowStepId }
842-
: {}),
843-
...(opts.isPastWorkflowStep !== undefined
844-
? { isPastWorkflowStep: opts.isPastWorkflowStep }
839+
...(opts.completeWorkflowStep !== undefined
840+
? { completeWorkflowStep: opts.completeWorkflowStep }
845841
: {}),
846842
});
847843

@@ -869,51 +865,86 @@ describe("submit_output workflow handler", () => {
869865
const content = await runSubmit(
870866
await buildToolset({
871867
isWorkflowActive: () => true,
872-
getCurrentWorkflowStepId: () => "a",
868+
completeWorkflowStep: () => "advanced",
873869
}),
874870
{ summary: "done" },
875871
);
876872
expect(content).toContain("requires a step identifier");
877873
expect(content).not.toContain("Advancing");
878874
});
879875

880-
test("acknowledges advancement when the step matches the current step", async () => {
876+
test("reports complete() when the step advances", async () => {
881877
const content = await runSubmit(
882878
await buildToolset({
883879
isWorkflowActive: () => true,
884-
getCurrentWorkflowStepId: () => "a",
880+
completeWorkflowStep: (id) => (id === "a" ? "advanced" : "not-current"),
885881
}),
886882
{ step: "a" },
887883
);
888884
expect(content).toContain("Advancing to the next step");
889885
});
890886

891-
test("acknowledges duplicate or stale completions without claiming an advance", async () => {
887+
test("reports already-complete without claiming an advance", async () => {
892888
const content = await runSubmit(
893889
await buildToolset({
894890
isWorkflowActive: () => true,
895-
getCurrentWorkflowStepId: () => "b",
896-
isPastWorkflowStep: (id) => id === "a",
891+
completeWorkflowStep: () => "already-complete",
897892
}),
898893
{ step: "a" },
899894
);
900895
expect(content).toContain("already complete");
901896
expect(content).not.toContain("Advancing");
902897
});
903898

904-
test("does not report a future step as already complete", async () => {
899+
test("does not report a not-current step as already complete", async () => {
905900
const content = await runSubmit(
906901
await buildToolset({
907902
isWorkflowActive: () => true,
908-
getCurrentWorkflowStepId: () => "a",
909-
isPastWorkflowStep: () => false,
903+
completeWorkflowStep: () => "not-current",
910904
}),
911905
{ step: "b" },
912906
);
913907
expect(content).toContain("not current");
914908
expect(content).not.toContain("already complete");
915909
expect(content).not.toContain("Advancing");
916910
});
911+
912+
test("omitted completeWorkflowStep does not claim an advance", async () => {
913+
const content = await runSubmit(await buildToolset({ isWorkflowActive: () => true }), {
914+
step: "a",
915+
});
916+
expect(content).toContain("not current");
917+
expect(content).not.toContain("Advancing");
918+
});
919+
920+
test("parallel submit_output only one reports Advancing", async () => {
921+
const { WorkflowRuntime } = await import("./workflows/runtime.js");
922+
const workflow = {
923+
name: "simple",
924+
description: "two steps",
925+
steps: [
926+
{ id: "a", label: "A" },
927+
{ id: "b", label: "B" },
928+
],
929+
};
930+
const runtime = new WorkflowRuntime(new Map(), () => workflow);
931+
runtime.start(workflow);
932+
const toolset = await buildToolset({
933+
isWorkflowActive: () => true,
934+
completeWorkflowStep: (stepId) => runtime.complete(stepId),
935+
});
936+
const run = (id: string, step: string) =>
937+
toolset.dynamicRunner.run(
938+
{ id, name: "submit_output", arguments: { step } },
939+
new AbortController().signal,
940+
);
941+
const [first, second] = await Promise.all([run("so-1", "a"), run("so-2", "a")]);
942+
await toolset.dispose();
943+
const contents = [String(first.content), String(second.content)];
944+
expect(contents.filter((c) => c.includes("Advancing"))).toHaveLength(1);
945+
expect(contents.filter((c) => c.includes("already complete"))).toHaveLength(1);
946+
expect(runtime.currentStep()?.id).toBe("b");
947+
});
917948
});
918949

919950
describe("transient nudges", () => {

src/tui/runner.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,7 +1258,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
12581258
toolAvailability,
12591259
);
12601260
// The workflow controller is built below, after the toolset; the holder lets
1261-
// submit_output's handler read live workflow-active state without a
1261+
// submit_output's handler complete the live workflow without a
12621262
// construction-order cycle.
12631263
const workflowControllerHolder: { instance?: WorkflowController } = {};
12641264

@@ -1282,9 +1282,8 @@ export async function runTUI(initialConfig: Config): Promise<number> {
12821282
getContextDir: () => workdir,
12831283

12841284
isWorkflowActive: () => workflowControllerHolder.instance?.isActive() === true,
1285-
getCurrentWorkflowStepId: () => workflowControllerHolder.instance?.currentStepId() ?? null,
1286-
isPastWorkflowStep: (stepId) =>
1287-
workflowControllerHolder.instance?.isPastStep(stepId) === true,
1285+
completeWorkflowStep: (stepId) =>
1286+
workflowControllerHolder.instance?.complete(stepId) ?? "not-current",
12881287
...(extraToolPlugins.length > 0 ? { extraToolPlugins } : {}),
12891288
onOperatorGate: (question, options) =>
12901289
new Promise<OperatorResult>((resolve) => {

src/tui/workflow-controller.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ import {
1212
saveWorkflowState,
1313
warnWorkflowPersistenceFailure,
1414
} from "../workflows/state.js";
15-
import type { CapabilityName, StepStatus, Workflow } from "../workflows/types.js";
15+
import type {
16+
CapabilityName,
17+
StepStatus,
18+
Workflow,
19+
WorkflowCompleteResult,
20+
} from "../workflows/types.js";
1621
import type { WorkflowEvent } from "../workflows/runtime.js";
1722

1823
export interface CapabilityStatus {
@@ -105,12 +110,8 @@ export class WorkflowController {
105110
return this.runtime?.isActive() === true;
106111
}
107112

108-
currentStepId(): string | null {
109-
return this.coordinator?.currentStepId() ?? null;
110-
}
111-
112-
isPastStep(stepId: string): boolean {
113-
return this.coordinator?.isPastStep(stepId) === true;
113+
complete(stepId: string): WorkflowCompleteResult {
114+
return this.coordinator?.complete(stepId) ?? "not-current";
114115
}
115116

116117
list(): { name: string; description: string }[] {

src/workflows/coordinator.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { WorkflowRuntime } from "./runtime.js";
2-
import type { WorkflowStep } from "./types.js";
2+
import type { WorkflowCompleteResult, WorkflowStep } from "./types.js";
33

44
// Bridges the workflow runtime and a director. A director consults the
55
// coordinator for the directive to inject into each turn's system prompt and
@@ -78,16 +78,23 @@ export class WorkflowCoordinator {
7878
// the runtime, and only via compare-and-advance against the current step.
7979
// Returns true when the runtime advanced (used by tests; the directors
8080
// already reset their idle counters on any tool call, so a workflow
81-
// advance is never seen as a stall). Duplicate or stale completions are
82-
// acknowledged here without moving the cursor.
81+
// advance is never seen as a stall). Already-complete and not-current
82+
// completions are acknowledged here without moving the cursor.
8383
handleToolDone(name: string | undefined, args: unknown, isError: boolean): boolean {
8484
if (isError || !this.runtime.isActive()) return false;
8585
if (name !== "submit_output") return false;
8686
const stepId = stepIdOf(args);
8787
if (stepId === null) return false;
88-
if (this.runtime.complete(stepId) !== "advanced") return false;
89-
this.persist();
90-
return true;
88+
return this.complete(stepId) === "advanced";
89+
}
90+
91+
// Compare-and-advance, persist on a real move, and return the complete()
92+
// result so callers (submit_output's handler) report it instead of
93+
// reconstructing the cursor.
94+
complete(stepId: string): WorkflowCompleteResult {
95+
const result = this.runtime.complete(stepId);
96+
if (result === "advanced") this.persist();
97+
return result;
9198
}
9299
}
93100

src/workflows/definition.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ export interface Workflow {
2323
description: string;
2424
autoInvoke?: string;
2525
stepThrough?: boolean;
26-
autoAdvance?: boolean;
2726
steps: WorkflowStep[];
2827
}
2928

src/workflows/runtime.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
type WorkflowFrame,
99
type WorkflowState,
1010
type WorkflowStep,
11+
type WorkflowCompleteResult,
1112
} from "./types.js";
1213

1314
export type WorkflowEvent =
@@ -96,16 +97,22 @@ export class WorkflowRuntime {
9697
}
9798

9899
// Compare-and-advance against the current step. Matching `stepId` advances
99-
// atomically (check and move happen in this call). Any other id — duplicate
100-
// of a step already left behind, a future step, or no active step — is
101-
// acknowledged without moving the cursor, so a retry cannot skip ahead.
102-
complete(stepId: string): "advanced" | "acknowledged" {
100+
// atomically (check and move happen in this call). A step already behind the
101+
// cursor is already-complete; a future, unknown, or inactive id is
102+
// not-current. Neither acknowledged case moves the cursor, so a retry cannot
103+
// skip ahead.
104+
complete(stepId: string): WorkflowCompleteResult {
103105
const current = this.currentStep();
104106
if (current !== null && current.id === stepId) {
105107
this.advance();
106108
return "advanced";
107109
}
108-
return "acknowledged";
110+
const view = this.view();
111+
if (view !== null) {
112+
const idx = view.steps.findIndex((s) => s.step.id === stepId);
113+
if (idx !== -1 && idx < view.stepIndex) return "already-complete";
114+
}
115+
return "not-current";
109116
}
110117

111118
// Mark the current step complete and move to the next runnable step. Pops

0 commit comments

Comments
 (0)