fix: goal 0021 Phase 1 remainder — gaps 2-4 closed (DBOS pseudo-steps, input mapping, cycle naming)

Three verified fixes closing out goal 0021's Phase 1 gap log:

- Gap 2 (DBOS parking pseudo-steps in get_run): investigated and found
  already fixed by goal 0026's PR the same day it was logged (the
  "DBOS." step-name-prefix filter covers every DBOS system op, and the
  stepped/breakpoint flow parks through the identical parkForApproval
  mechanism as a plain guardrail ask). Added the missing proof for the
  stepped-flow case specifically via assertNoDBOSPseudoSteps in
  breakpoint_test.go.
- Gap 3 (per-step input field unverified): proved the mapping itself
  is correct on a real multi-step run with a real payload
  (TestGetRun_MultiStepInput_PopulatedAndChained). Found and fixed the
  actual cause of the dogfood confusion: RunStep.Input carried
  omitempty while sibling Output didn't, silently dropping the JSON
  key on a genuinely-empty first-step input. Removed the omitempty;
  regenerated bindings.
- Gap 4 (generic cycle-detection errors): both cycle shapes now name
  the actual looping node IDs. findRoot's zero-root case (a pure
  cycle) uses a new findAnyCycle DFS helper (graph_cycle.go, split out
  to respect the 500-line file limit). ExecuteWorkflow's runtime walk
  (a cycle downstream of a valid root -- a shape ValidateGraph's
  reachability check doesn't catch at all) now tracks traversal order
  instead of a bare seen-set to report the real loop too.

Phase 1 is now fully complete (4 gaps fixed/verified + 1
confirmed-by-design). Phase 2/3 stay open -- they need live
interactive probing, not code changes, per the goal's own scope.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018pkViCNAuZp2vBv2K9AbUh

Author: alicoding

docs/goals/0021-mcp-dogfood-gap-closure.md

@@ -8,7 +8,11 @@ MCP surface against the bank use cases (memory
 to a fix in priority order. Findings from live probing of the running
 instance, not code reading.
 
-## Phase 1 — read/introspection surface (2026-08-11, done)
+## Phase 1 — read/introspection surface (2026-08-11 probed, 2026-08-12
+gaps 2-4 closed — Phase 1 now fully complete: every gap either fixed or
+explicitly declined/confirmed-by-design. Phase 2/3 remain open below,
+unblocked but not yet run -- they need live interactive probing, not
+code changes.)
 
 What already works well, verified live: `list_node_types` is a real
 authoring vocabulary (full typed ConfigFields, defaults, options,
@@ -26,21 +30,68 @@ right terminal Decision.
    proven by `TestMCPRunWorkflow_PayloadFlowsIntoCaptureFile`, a
    real MCP client running a capture-file workflow against a real
    temp file). Trigger-fed workflows are now testable over MCP.
-2. **[MED] Parked/stepped runs leak DBOS parking machinery
+2. ~~**[MED] Parked/stepped runs leak DBOS parking machinery
    (`DBOS.setEvent`/`DBOS.recv`/`DBOS.sleep`) as pseudo-steps** in
    `get_run`'s step list (empty nodeTypeID) — noise on exactly the
-   runs a debugger inspects most. Filter (or label) them in
-   `GetRun`; check whether the UI Runs tab renders the same noise.
-3. **[LOW-MED] Per-step `input` (payload) field unverified** in
+   runs a debugger inspects most.~~ — **Already fixed, verified
+   2026-08-12.** Investigation found this was the exact same bug
+   goal 0026's PR fixed the same day this gap was logged (commit
+   `3e0433e`, "run detail no longer shows DBOS system steps as
+   blank rows"): the `"DBOS."` step-name-prefix filter in
+   `GetRun` (executionservice_getrun.go) already covers every DBOS
+   system op (verified against the installed `dbos-transact-golang`
+   source — every system step, not just setEvent/recv/sleep, uses
+   the uniform prefix), and the stepped/breakpoint flow
+   (`RunWorkflowStepped`) parks through the exact same
+   `parkForApproval` mechanism as an ordinary guardrail-policy ask,
+   so it was already covered too — confirmed by tracing
+   `executionservice_guardrail.go`, not assumed. What was missing:
+   a proof for the stepped-flow case specifically (only the
+   guardrail-approval-park case had a "no pseudo-step" assertion).
+   Added `assertNoDBOSPseudoSteps`, applied to
+   `TestStepMode_ParksBeforeEveryNode_StepThenContinue` at both the
+   first park and the final resolved-run `GetRun`
+   (breakpoint_test.go). The UI Runs tab calls the same `GetRun`
+   RPC, so no separate check needed there.
+3. ~~**[LOW-MED] Per-step `input` (payload) field unverified** in
    `get_run` — `inputAttributes` present on fresh runs, but no
    `input` key appeared (possibly omitempty + an empty first-step
-   input). Verify on a multi-step run with a real payload; fix the
-   mapping if genuinely missing.
-4. **[LOW] `validate_workflow` on a graph with a cycle reports only
+   input).~~ — **Fixed 2026-08-12.** Verified via a new real
+   multi-step run with a non-empty payload
+   (`TestGetRun_MultiStepInput_PopulatedAndChained`,
+   executionservice_getrun_test.go): the mapping itself was already
+   correct (each step's `Input` is the prior executed step's
+   `Output`, or the run's own seeded payload for the first step).
+   The actual bug was the suspected cause: `RunStep.Input` carried
+   `omitempty` while the sibling `Output` field didn't — an
+   asymmetry that silently dropped the JSON key on a genuinely-empty
+   first-step input, indistinguishable over MCP from a real mapping
+   failure. Removed `omitempty` from `Input`
+   (executionservice.go) so the key is always present, matching
+   `Output`'s own convention; bindings regenerated.
+4. ~~**[LOW] `validate_workflow` on a graph with a cycle reports only
    "must have exactly one starting node"** — true but unhelpful;
    naming the cycle would let an authoring agent fix it in one
    round trip instead of discovering it after removing the wrong
-   edge.
+   edge.~~ — **Fixed 2026-08-12.** Two distinct cycle shapes existed:
+   (a) a pure cycle with no root at all (`findRoot`'s zero-root
+   case, graph.go) — now names the actual looping node IDs via a
+   new `findAnyCycle` DFS helper (`graph_cycle.go`, split out to
+   stay under the 500-line file limit), e.g. "these nodes form a
+   cycle: a -> b -> c -> a"; and (b) a cycle reachable from a
+   perfectly valid single root but looping further downstream
+   (execute.go's runtime walk, previously a bare "workflow graph
+   contains a cycle") — `ValidateGraph`'s save-time reachability
+   check doesn't catch this shape at all (every looping node IS
+   reachable from the root, so nothing is flagged "unreachable");
+   only actual execution's traversal does, and its error now names
+   the real loop too (e.g. "a -> b -> a"), tracking each visited
+   node's position in traversal order instead of a bare seen-set.
+   Proven by two new tests in graph_test.go
+   (`TestFindRoot_PureCycle_NamesTheLoopingNodes`,
+   `TestExecuteWorkflow_CycleDownstreamOfARealRoot_NamesTheLoopingNodes`)
+   against real cyclic graphs run through `ExecuteWorkflow`/
+   `ValidateGraph`, not just unit-testing the helper in isolation.
 5. **Corrected finding (initially misread):** `run_workflow` IS
    gated by the writes toggle (`requireWriteEnabled`,
    millmcpservice_authoring.go:249) — it succeeded in probing

docs/goals/BACKLOG.md

@@ -145,7 +145,7 @@ live-review material, interleaved during owner reviews, not a lane.**
 
 **Standing**
 - [ ] [0001 — Authoring-surface overhaul](0001-authoring-surface-overhaul.md) (spacing audit + §3.8 prototype elements — live-review material)
-- [ ] [0021 — MCP dogfood gap closure](0021-mcp-dogfood-gap-closure.md) (owner-mandated 2026-08-11: orchestrator live-probes the MCP surface against the bank use cases, logs ranked gaps, fixes graduate out; phase 1 done — 4 gaps + 1 confirmed-by-design)
+- [ ] [0021 — MCP dogfood gap closure](0021-mcp-dogfood-gap-closure.md) (owner-mandated 2026-08-11: orchestrator live-probes the MCP surface against the bank use cases, logs ranked gaps, fixes graduate out; **Phase 1 fully complete 2026-08-12** — all 4 gaps fixed/verified + 1 confirmed-by-design; Phase 2/3 still open, need live interactive probing not code changes)
 - [ ] Workflow pins/favorites (tech debt, split from goal 0015's remainder 2026-08-12) — no pin/favorite concept exists anywhere in Mill today (grepped before scoping it out); needs its own small schema decision (which store owns a pin list, per-workflow or a plain ID set) before any build — deliberately not invented ad hoc under 0015's frecency-only ship. Quick Panel's workflow list sorts by frequency alone until this lands.
 - [ ] ⌘?/⌘/ multi-binding keybinding alias (tech debt, split from goal 0015's remainder 2026-08-12) — the owner's goal-0015 "bind ⌘? (and/or ⌘/) to open the palette too" ask needs a command to carry more than one `KeyCombo`; today's registry (`shared/commands.ts`) is 1:1 (`defaultBinding: KeyCombo | null`). Needs a real schema call (array vs. a small alias table) before it's buildable — real data-model infrastructure, not a self-contained UI change.
 - [ ] Copy-management migration — `app/` (tech debt, split from goal 0032 2026-08-12) — extract `app/`'s remaining hardcoded JSX copy (App.tsx's shell chrome, QuickPanel/QuickPanelApp, ApprovalPromptApp, workflowFrecency-adjacent UI, etc. — ~11 files carry inline strings) into `frontend/src/locales/en/app.json` (already scaffolded, currently `{}`) following `SettingsView.tsx`'s established pattern (`useTranslation()`/`t()`, namespace-per-bounded-context). DoR: read `docs/goals/0032-copy-management.md` for the locked i18n pattern before starting — no new library/schema decision needed, this is mechanical extraction. DoD: every `app/*.tsx` file free of inline user-facing string literals in JSX (aria-labels included), `app.json` populated, existing e2e specs touching `app/` still pass unchanged (translated text must match original English exactly).

frontend/bindings/github.com/alicoding/mill/internal/services/executionsvc/models.ts

@@ -209,9 +209,15 @@ export interface RunStep {
      * item 3): the immediately-preceding EXECUTED step's own recorded
      * Payload/Attributes, or the run's own seeded starting values for
      * the first executed step. Previously undiscoverable at all -- only
-     * a step's OUTPUT was ever surfaced.
+     * a step's OUTPUT was ever surfaced. Input deliberately has NO
+     * omitempty, matching Output below -- goal 0021 gap 3, caught live:
+     * a run started with no payload seeded a genuinely empty first-step
+     * Input, and omitempty dropped the JSON key entirely over MCP,
+     * reading as "the field doesn't exist" rather than "it's empty" --
+     * indistinguishable from a real mapping bug. An always-present key
+     * (empty string when there's truly nothing) is unambiguous.
      */
-    "input"?: string;
+    "input": string;
     "inputAttributes"?: { [_ in string]?: any } | null;
     "output": string;
 

internal/domain/composition/execute.go

@@ -2,6 +2,7 @@ package composition
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/alicoding/mill/internal/domain/guardrail"
 )
@@ -131,19 +132,31 @@ func executeWorkflow(nodes []Node, edges []Edge, attrs []AttributeDef, run StepR
 	if err != nil {
 		return "", err
 	}
-	root, err := findRoot(nodes, hasIncoming)
+	root, err := findRoot(nodes, outgoingEdges, hasIncoming)
 	if err != nil {
 		return "", err
 	}
 
 	ctx := ExecContext{Payload: opts.InitialPayload, Attributes: attributesEnv(attrs, opts.AttrValues), RunContext: opts.RunContext, Stepped: opts.Stepped}
-	visited := make(map[string]bool, len(nodes))
+	// visited records each node's position in path (traversal order) so
+	// a cycle hit below can report the actual loop -- e.g. "b -> c -> b"
+	// -- instead of a bare "contains a cycle" (goal 0021 gap 4: a
+	// generic message left an authoring agent to find the loop by
+	// process of elimination; a root-level cycle is already named by
+	// findRoot/findAnyCycle above, this is the sibling case where the
+	// graph has a valid single root but loops somewhere downstream of
+	// it, which ValidateGraph's reachability check doesn't catch since
+	// every node in the loop IS reachable from the root).
+	visited := make(map[string]int, len(nodes))
+	var path []string
 	current := root
 	for {
-		if visited[current] {
-			return "", fmt.Errorf("workflow graph contains a cycle")
+		if idx, ok := visited[current]; ok {
+			cycle := append(append([]string{}, path[idx:]...), current)
+			return "", fmt.Errorf("workflow graph contains a cycle: %s", strings.Join(cycle, " -> "))
 		}
-		visited[current] = true
+		visited[current] = len(path)
+		path = append(path, current)
 
 		node := byID[current]
 		// Trigger and Decision nodes carry no payload transformation --

internal/domain/composition/graph.go

@@ -51,8 +51,15 @@ func buildGraph(nodes []Node, edges []Edge) (byID map[string]Node, outgoingEdges
 
 // findRoot returns the single node with no incoming edge -- the
 // workflow's entry point, same definition internal/domain/trigger's
-// ExtractTrigger already relies on.
-func findRoot(nodes []Node, hasIncoming map[string]bool) (string, error) {
+// ExtractTrigger already relies on. Zero roots (every node has an
+// incoming edge) is exactly the case a pure cycle produces -- goal
+// 0021 gap 4's dogfood finding: the old message ("a workflow must have
+// exactly one starting node") was technically true but left an
+// authoring agent to discover which nodes loop by process of
+// elimination. When it's a cycle, name it -- findAnyCycle walks the
+// same outgoingEdges buildGraph already produced, so this costs
+// nothing extra to compute.
+func findRoot(nodes []Node, outgoingEdges map[string][]Edge, hasIncoming map[string]bool) (string, error) {
 	var root string
 	rootCount := 0
 	for _, n := range nodes {
@@ -61,6 +68,12 @@ func findRoot(nodes []Node, hasIncoming map[string]bool) (string, error) {
 			rootCount++
 		}
 	}
+	if rootCount == 0 {
+		if cycle := findAnyCycle(nodes, outgoingEdges); len(cycle) > 0 {
+			return "", fmt.Errorf("a workflow must have exactly one starting node (found none -- every node has an incoming edge because these nodes form a cycle: %s)", strings.Join(cycle, " -> "))
+		}
+		return "", fmt.Errorf("a workflow must have exactly one starting node")
+	}
 	if rootCount != 1 {
 		return "", fmt.Errorf("a workflow must have exactly one starting node")
 	}
@@ -231,7 +244,7 @@ func ValidateGraph(nodes []Node, edges []Edge, attrs []AttributeDef) []Issue {
 
 	var issues []Issue
 
-	root, rootErr := findRoot(nodes, hasIncoming)
+	root, rootErr := findRoot(nodes, outgoingEdges, hasIncoming)
 	if rootErr != nil {
 		issues = append(issues, errorIssue("", "", rootErr.Error()))
 	} else {

internal/domain/composition/graph_cycle.go

@@ -0,0 +1,64 @@
+package composition
+
+// findAnyCycle -- split out of graph.go once that file crossed the
+// 500-line limit (CLAUDE.md/§1.3), the same "split along a real seam"
+// discipline this package's other split files already established.
+// Naming an actual cycle is a genuinely separable diagnostic concern
+// from buildGraph/findRoot's core structural checks (goal 0021 gap 4:
+// the old "a workflow must have exactly one starting node" message was
+// technically true on a pure cycle but left an authoring agent to find
+// the loop by process of elimination).
+
+// findAnyCycle returns one real cycle's node IDs in traversal order
+// (the loop's first node repeated at the end, e.g. ["a", "b", "c",
+// "a"]), or nil if the graph is acyclic. Standard white/gray/black DFS
+// -- gray means "on the current recursion stack"; hitting a gray node
+// closes a cycle back to that node's position on the stack. Iterates
+// nodes/outgoingEdges in their given (deterministic, insertion-preserving
+// per buildGraph's own doc comment) order, so the same graph always
+// reports the same cycle rather than one that varies with map
+// iteration order.
+func findAnyCycle(nodes []Node, outgoingEdges map[string][]Edge) []string {
+	const (
+		white = iota
+		gray
+		black
+	)
+	color := make(map[string]int, len(nodes))
+	var stack []string
+	var cycle []string
+
+	var visit func(id string) bool
+	visit = func(id string) bool {
+		color[id] = gray
+		stack = append(stack, id)
+		for _, e := range outgoingEdges[id] {
+			switch color[e.Target] {
+			case white:
+				if visit(e.Target) {
+					return true
+				}
+			case gray:
+				for i, sid := range stack {
+					if sid == e.Target {
+						cycle = append(append([]string{}, stack[i:]...), e.Target)
+						break
+					}
+				}
+				return true
+			}
+		}
+		color[id] = black
+		stack = stack[:len(stack)-1]
+		return false
+	}
+
+	for _, n := range nodes {
+		if color[n.ID] == white {
+			if visit(n.ID) {
+				return cycle
+			}
+		}
+	}
+	return nil
+}

internal/domain/composition/graph_test.go

@@ -1,6 +1,7 @@
 package composition
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -66,6 +67,72 @@ func TestExecuteWorkflow_MultipleRootsMergingIntoOneNode_Rejected(t *testing.T)
 	}
 }
 
+// TestFindRoot_PureCycle_NamesTheLoopingNodes is goal 0021 gap 4's
+// repro: a graph where every node has an incoming edge (a pure cycle,
+// no Trigger at all) used to report only "a workflow must have exactly
+// one starting node" -- true, but it left an authoring agent to find
+// the loop by process of elimination. findRoot must now name the
+// actual node IDs that loop.
+func TestFindRoot_PureCycle_NamesTheLoopingNodes(t *testing.T) {
+	nodes := []Node{
+		{ID: "a", NodeTypeID: "process-inject-text"},
+		{ID: "b", NodeTypeID: "process-inject-text"},
+		{ID: "c", NodeTypeID: "process-inject-text"},
+	}
+	edges := []Edge{
+		{ID: "e1", Source: "a", Target: "b"},
+		{ID: "e2", Source: "b", Target: "c"},
+		{ID: "e3", Source: "c", Target: "a"},
+	}
+
+	_, err := ExecuteWorkflow(nodes, edges, nil)
+	if err == nil {
+		t.Fatal("ExecuteWorkflow on a pure cycle returned nil error, want an error")
+	}
+	if !strings.Contains(err.Error(), "a -> b -> c -> a") {
+		t.Fatalf("error = %q, want it to name the actual cycle (a -> b -> c -> a)", err.Error())
+	}
+
+	issues := ValidateGraph(nodes, edges, nil)
+	var found bool
+	for _, iss := range issues {
+		if strings.Contains(iss.Message, "a -> b -> c -> a") {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("ValidateGraph issues = %+v, want one naming the cycle (a -> b -> c -> a)", issues)
+	}
+}
+
+// TestExecuteWorkflow_CycleDownstreamOfARealRoot_NamesTheLoopingNodes
+// is the sibling case: a graph WITH a valid, unique Trigger root (so
+// findRoot succeeds and ValidateGraph's reachability walk sees every
+// node as reachable -- it doesn't check for cycles, only
+// unreachability) but that loops further downstream. Only actual
+// execution's own traversal catches this one, and its error must name
+// the loop too, not just say "contains a cycle".
+func TestExecuteWorkflow_CycleDownstreamOfARealRoot_NamesTheLoopingNodes(t *testing.T) {
+	nodes := []Node{
+		{ID: "t", NodeTypeID: "trigger-manual", Kind: KindTrigger},
+		{ID: "a", NodeTypeID: "process-inject-text"},
+		{ID: "b", NodeTypeID: "process-inject-text"},
+	}
+	edges := []Edge{
+		{ID: "e1", Source: "t", Target: "a"},
+		{ID: "e2", Source: "a", Target: "b"},
+		{ID: "e3", Source: "b", Target: "a"},
+	}
+
+	_, err := ExecuteWorkflow(nodes, edges, nil)
+	if err == nil {
+		t.Fatal("ExecuteWorkflow on a downstream cycle returned nil error, want an error")
+	}
+	if !strings.Contains(err.Error(), "a -> b -> a") {
+		t.Fatalf("error = %q, want it to name the actual loop (a -> b -> a)", err.Error())
+	}
+}
+
 func TestValidateGraph_DisconnectedIslandBehindACycle_Rejected(t *testing.T) {
 	// The specific trap a naive "root count + edge count" check misses:
 	// a 2-node cycle elsewhere in the graph "absorbs" exactly enough