From 463e5b8375ef46ffd85d9fa945d6f9452a3da232 Mon Sep 17 00:00:00 2001 From: Matt Leaverton Date: Fri, 24 Apr 2026 17:30:00 -0500 Subject: [PATCH 1/2] fix: require condition on inbound-to-terminal edges from fallible predecessors (#3) --- .../attractor/validate/terminal_condition.go | 100 +++++++ .../validate/terminal_condition_test.go | 259 ++++++++++++++++++ internal/attractor/validate/validate.go | 1 + 3 files changed, 360 insertions(+) create mode 100644 internal/attractor/validate/terminal_condition.go create mode 100644 internal/attractor/validate/terminal_condition_test.go diff --git a/internal/attractor/validate/terminal_condition.go b/internal/attractor/validate/terminal_condition.go new file mode 100644 index 00000000..439eb6cd --- /dev/null +++ b/internal/attractor/validate/terminal_condition.go @@ -0,0 +1,100 @@ +package validate + +import ( + "fmt" + "strings" + + "github.com/danshapiro/kilroy/internal/attractor/model" +) + +// lintTerminalConditionEdge checks that every inbound edge to a terminal node +// whose predecessor is a fallible node (one that can produce a non-success +// outcome) carries an explicit condition= attribute. +// +// Rule: terminal_condition_edge (ERROR) +// +// Rationale: if a fallible node (e.g., an agent or tool) fails and has an +// unconditional edge to a terminal node, the engine follows the edge, reaches +// the terminal, and reports status=success — even though the predecessor failed. +// Requiring an explicit condition forces the graph author to declare their +// routing intent rather than relying on an implicit "always proceed to done". +// +// Success-only predecessors (start, conditional/diamond, loop.begin/end, +// concurrent.split/join) are exempt: they never execute user code and cannot +// produce a non-success outcome on their own, so an unconditional edge from +// them to a terminal node is safe. +func lintTerminalConditionEdge(g *model.Graph) []Diagnostic { + exitSet := make(map[string]bool) + for _, id := range findAllExitNodeIDs(g) { + exitSet[id] = true + } + + var diags []Diagnostic + for _, e := range g.Edges { + if e == nil { + continue + } + // Only care about edges whose target is a terminal node. + if !exitSet[e.To] { + continue + } + // Edge already has an explicit condition — graph author was deliberate. + if strings.TrimSpace(e.Condition()) != "" { + continue + } + // Look up the source node. + fromNode := g.Nodes[e.From] + if fromNode == nil { + // Missing node — caught by edge_target_exists rule. + continue + } + // Success-only predecessors are exempt: they cannot fail. + if isSuccessOnlyPredecessor(fromNode, e.From) { + continue + } + // Fallible predecessor with an unconditional edge to a terminal node. + diags = append(diags, Diagnostic{ + Rule: "terminal_condition_edge", + Severity: SeverityError, + Message: fmt.Sprintf( + "edge %s → %s: unconditional edge to terminal node from fallible predecessor %q; add an explicit condition= so routing intent is clear (e.g. condition=\"outcome=success\" to proceed only on success, or condition=\"outcome!=success\" for a failure fallback)", + e.From, e.To, e.From, + ), + EdgeFrom: e.From, + EdgeTo: e.To, + Fix: fmt.Sprintf( + "add condition=\"outcome=success\" (or another explicit condition) to the %s → %s edge", + e.From, e.To, + ), + }) + } + return diags +} + +// isSuccessOnlyPredecessor returns true when n is a node type that cannot +// produce a non-success outcome on its own (i.e., it performs no user-code +// execution and its routing is fully deterministic). These nodes are exempt +// from the terminal_condition_edge rule. +// +// The set covers: +// - Start nodes shape=Mdiamond, shape=circle, or id="start" (by convention) +// - Conditional shape=diamond (pure pass-through router) +// - Loop sentinels shape=trapezium (loop.begin), shape=invtrapezium (loop.end) +// - Concurrent shape=pentagon (split), shape=cylinder (join) +func isSuccessOnlyPredecessor(n *model.Node, id string) bool { + switch n.Shape() { + case "Mdiamond", "circle": + return true // start node shapes + case "diamond": + return true // conditional pass-through — never executes user code + case "trapezium", "invtrapezium": + return true // loop.begin / loop.end sentinels + case "pentagon", "cylinder": + return true // concurrent.split / concurrent.join + } + // Also honour the "start" node ID convention (spec §6.1). + if strings.EqualFold(id, "start") { + return true + } + return false +} diff --git a/internal/attractor/validate/terminal_condition_test.go b/internal/attractor/validate/terminal_condition_test.go new file mode 100644 index 00000000..eca97292 --- /dev/null +++ b/internal/attractor/validate/terminal_condition_test.go @@ -0,0 +1,259 @@ +package validate + +import ( + "strings" + "testing" + + "github.com/danshapiro/kilroy/internal/attractor/dot" +) + +// TestTerminalConditionEdge_FalliblePredecessor_NoCondition verifies that the +// rule fires when a fallible (agent or tool) node has an unconditional edge to +// a terminal node — the bug class described in fix #3. +func TestTerminalConditionEdge_FalliblePredecessor_NoCondition(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + agent [shape=box, llm_provider=anthropic, llm_model=claude-sonnet-4.6, + prompt="Do work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> agent -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertHasRule(t, diags, "terminal_condition_edge", SeverityError) +} + +// TestTerminalConditionEdge_FallibleToolPredecessor_NoCondition verifies that +// the rule fires for tool (parallelogram) nodes as well as agent (box) nodes. +func TestTerminalConditionEdge_FallibleToolPredecessor_NoCondition(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + setup [shape=parallelogram, tool_command="sh setup.sh"] + start -> setup -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertHasRule(t, diags, "terminal_condition_edge", SeverityError) +} + +// TestTerminalConditionEdge_ErrorMessageContainsNodeIDs verifies that the +// diagnostic message includes both the source and target node IDs so the graph +// author can locate the offending edge (acceptance criterion #2). +func TestTerminalConditionEdge_ErrorMessageContainsNodeIDs(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + done [shape=Msquare] + worker [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Do work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> worker -> done +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + + var found *Diagnostic + for i := range diags { + if diags[i].Rule == "terminal_condition_edge" { + found = &diags[i] + break + } + } + if found == nil { + t.Fatal("expected terminal_condition_edge diagnostic, got none") + } + if found.EdgeFrom == "" { + t.Error("terminal_condition_edge diagnostic must include EdgeFrom") + } + if found.EdgeTo == "" { + t.Error("terminal_condition_edge diagnostic must include EdgeTo") + } + if !strings.Contains(found.Message, "worker") { + t.Errorf("expected message to contain source node id %q; got: %s", "worker", found.Message) + } + if !strings.Contains(found.Message, "done") { + t.Errorf("expected message to contain target node id %q; got: %s", "done", found.Message) + } +} + +// TestTerminalConditionEdge_SuccessOnlyStart_NoConditionAllowed verifies that +// an unconditional edge from a start node to a terminal does NOT trigger the +// rule (acceptance criterion #3: "start -> exit is still valid"). +func TestTerminalConditionEdge_SuccessOnlyStart_NoConditionAllowed(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertNoRule(t, diags, "terminal_condition_edge") +} + +// TestTerminalConditionEdge_WithCondition_NoError verifies that a fallible +// predecessor whose edge to the terminal carries an explicit condition passes +// (acceptance criterion #4: "agent -> exit [condition=...] passes"). +func TestTerminalConditionEdge_WithCondition_NoError(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + agent [shape=box, llm_provider=anthropic, llm_model=claude-sonnet-4.6, + prompt="Do work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> agent + agent -> exit [condition="outcome=success"] +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertNoRule(t, diags, "terminal_condition_edge") +} + +// TestTerminalConditionEdge_AgentWithoutCondition_Error is the canonical example +// from acceptance criterion #5: "start -> agent -> exit without condition FAILS". +func TestTerminalConditionEdge_AgentWithoutCondition_Error(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + agent [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Do work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> agent -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertHasRule(t, diags, "terminal_condition_edge", SeverityError) +} + +// TestTerminalConditionEdge_ConditionalPredecessor_Exempt verifies that a +// diamond (conditional) node is treated as success-only: no condition required +// on its outgoing edge to a terminal. +func TestTerminalConditionEdge_ConditionalPredecessor_Exempt(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + agent [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Do work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + router [shape=diamond] + start -> agent -> router + router -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + // The router -> exit edge has no condition but router is a diamond (success-only). + assertNoRule(t, diags, "terminal_condition_edge") +} + +// TestTerminalConditionEdge_LoopBeginAndEnd_Exempt verifies that loop.begin +// (trapezium) and loop.end (invtrapezium) are treated as success-only. +func TestTerminalConditionEdge_LoopBeginAndEnd_Exempt(t *testing.T) { + // Loop end routing to terminal with no condition should be allowed because + // loop.end is a deterministic sentinel. + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + loop_begin [shape=trapezium, loop_id=main, loop_max=3] + loop_end [shape=invtrapezium, loop_id=main, loop_max=3] + start -> loop_begin -> loop_end -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertNoRule(t, diags, "terminal_condition_edge") +} + +// TestTerminalConditionEdge_DoublecircleTerminal_AlsoCaught verifies that the +// rule applies to doublecircle-shaped terminal nodes, not just Msquare. +func TestTerminalConditionEdge_DoublecircleTerminal_AlsoCaught(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + done [shape=doublecircle] + agent [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> agent -> done +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + assertHasRule(t, diags, "terminal_condition_edge", SeverityError) +} + +// TestTerminalConditionEdge_IntegratedInValidate verifies that the rule is +// wired into the top-level Validate function (not just the lint helper). +func TestTerminalConditionEdge_IntegratedInValidate(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + agent [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + start -> agent -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := Validate(g) // top-level: should include terminal_condition_edge + assertHasRule(t, diags, "terminal_condition_edge", SeverityError) +} + +// TestTerminalConditionEdge_MultipleOffendingEdges verifies that every +// unconditional inbound-to-terminal edge from a fallible predecessor is reported +// (not just the first one found). +func TestTerminalConditionEdge_MultipleOffendingEdges(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + step_a [shape=box, llm_provider=openai, llm_model=gpt-5.4, + prompt="Work. Write status to $KILROY_STAGE_STATUS_PATH or $KILROY_STAGE_STATUS_FALLBACK_PATH if that fails."] + step_b [shape=parallelogram, tool_command="sh b.sh"] + start -> step_a -> step_b + step_a -> exit + step_b -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintTerminalConditionEdge(g) + count := 0 + for _, d := range diags { + if d.Rule == "terminal_condition_edge" { + count++ + } + } + if count < 2 { + t.Fatalf("expected at least 2 terminal_condition_edge diagnostics (one per offending edge); got %d", count) + } +} diff --git a/internal/attractor/validate/validate.go b/internal/attractor/validate/validate.go index 8b69d373..73f3a401 100644 --- a/internal/attractor/validate/validate.go +++ b/internal/attractor/validate/validate.go @@ -96,6 +96,7 @@ func ValidateWithOptions(g *model.Graph, opts ValidateOptions, extraRules ...Lin diags = append(diags, lintConcurrentSplitHasJoin(g)...) diags = append(diags, lintNoNestedConcurrentRegions(g)...) diags = append(diags, lintNoLoopsInConcurrentRegions(g)...) + diags = append(diags, lintTerminalConditionEdge(g)...) // Run custom lint rules (spec §7.3: extra_rules appended after built-in rules). for _, rule := range extraRules { From ce6ae4824777050bdfc6af20d412916d6aee2002 Mon Sep 17 00:00:00 2001 From: Matt Leaverton Date: Mon, 27 Apr 2026 12:56:04 -0500 Subject: [PATCH 2/2] fix: reconcile all_conditional_edges with terminal_condition_edge; fix existing graphs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new terminal_condition_edge rule and the existing all_conditional_edges rule were in tension: the natural fix for the new rule (adding condition="outcome=success" to edges flagged by terminal_condition_edge) triggered all_conditional_edges, which fires when a node has only conditional outgoing edges with no fallback. all_conditional_edges now exempts two patterns: 1. Every outgoing edge targets a terminal node — "no condition matched" terminates the run by intent; terminal_condition_edge governs those edges instead. 2. Conditions are exhaustive via an outcome=X / outcome!=X pair — the pair covers the full outcome space, so there is no runtime routing gap. Also adds explicit conditions to the inbound-to-terminal edges in the three existing workflow graphs (build-test, coding-loop, multi-tool-exercise) so all three now validate clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/attractor/validate/validate.go | 67 +++++++++++++++++--- internal/attractor/validate/validate_test.go | 53 +++++++++++++++- workflows/build-test/graph.dot | 6 +- workflows/coding-loop/graph.dot | 2 +- workflows/multi-tool-exercise/graph.dot | 4 +- 5 files changed, 114 insertions(+), 18 deletions(-) diff --git a/internal/attractor/validate/validate.go b/internal/attractor/validate/validate.go index 73f3a401..4cbc9887 100644 --- a/internal/attractor/validate/validate.go +++ b/internal/attractor/validate/validate.go @@ -1376,25 +1376,74 @@ func lintAllConditionalEdges(g *model.Graph) []Diagnostic { continue // no outgoing edges — other lint rules handle this } allConditional := true + allTargetsTerminal := true for _, e := range edges { if strings.TrimSpace(e.Condition()) == "" { allConditional = false - break + } + if !exitIDs[strings.TrimSpace(e.To)] { + allTargetsTerminal = false } } - if allConditional { - diags = append(diags, Diagnostic{ - Rule: "all_conditional_edges", - Severity: SeverityError, - NodeID: id, - Message: fmt.Sprintf("node %q has %d outgoing edge(s) but all are conditional; add an unconditional fallback edge to avoid routing gaps", id, len(edges)), - Fix: "Add an unconditional edge (no condition attribute) as a fallback route", - }) + if !allConditional { + continue + } + // Exemption 1: every outgoing edge targets a terminal node. "No condition + // matched" terminates the run by intent — terminal_condition_edge governs + // those edges instead. + if allTargetsTerminal { + continue + } + // Exemption 2: conditions are exhaustive. A pair of edges with conditions + // `outcome=X` and `outcome!=X` (any value of X) covers the full outcome + // space, so there is no runtime routing gap. + if hasExhaustiveOutcomePair(edges) { + continue } + diags = append(diags, Diagnostic{ + Rule: "all_conditional_edges", + Severity: SeverityError, + NodeID: id, + Message: fmt.Sprintf("node %q has %d outgoing edge(s) but all are conditional; add an unconditional fallback edge to avoid routing gaps", id, len(edges)), + Fix: "Add an unconditional edge (no condition attribute) as a fallback route", + }) } return diags } +// hasExhaustiveOutcomePair reports whether the edges collectively cover the +// full outcome space via a complementary `outcome=X` / `outcome!=X` pair. +// Whitespace around tokens is tolerated; no other condition shapes are +// recognised here. +func hasExhaustiveOutcomePair(edges []*model.Edge) bool { + positive := make(map[string]bool) + negative := make(map[string]bool) + for _, e := range edges { + cond := strings.TrimSpace(e.Condition()) + if cond == "" { + continue + } + if v, ok := strings.CutPrefix(cond, "outcome!="); ok { + negative[strings.TrimSpace(v)] = true + continue + } + if v, ok := strings.CutPrefix(cond, "outcome ="); ok { + positive[strings.TrimSpace(v)] = true + continue + } + if v, ok := strings.CutPrefix(cond, "outcome="); ok { + positive[strings.TrimSpace(v)] = true + continue + } + } + for v := range positive { + if negative[v] { + return true + } + } + return false +} + func lintTemplatePostmortemRecoveryRouting(g *model.Graph) []Diagnostic { if g == nil { return nil diff --git a/internal/attractor/validate/validate_test.go b/internal/attractor/validate/validate_test.go index ade682d4..44fc871f 100644 --- a/internal/attractor/validate/validate_test.go +++ b/internal/attractor/validate/validate_test.go @@ -1506,9 +1506,11 @@ digraph G { start [shape=Mdiamond] exit [shape=Msquare] a [shape=box, llm_provider=openai, llm_model=gpt-5.4, prompt="x"] + b [shape=box, llm_provider=openai, llm_model=gpt-5.4, prompt="y"] start -> a - a -> exit [condition="outcome=success"] - a -> exit [condition="outcome=fail"] + a -> b [condition="outcome=success"] + a -> b [condition="outcome=fail"] + b -> exit } `)) if err != nil { @@ -1520,6 +1522,52 @@ digraph G { func TestValidate_AllConditionalEdges_PassesWithUnconditionalFallback(t *testing.T) { g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + a [shape=box, llm_provider=openai, llm_model=gpt-5.4, prompt="x"] + b [shape=box, llm_provider=openai, llm_model=gpt-5.4, prompt="y"] + start -> a + a -> b [condition="outcome=success"] + a -> b [condition="outcome=fail"] + a -> b + b -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintAllConditionalEdges(g) + assertNoRule(t, diags, "all_conditional_edges") +} + +// A complementary `outcome=X` / `outcome!=X` pair covers the full outcome +// space, so there is no routing gap even though every edge is conditional. +func TestValidate_AllConditionalEdges_ExemptOnExhaustivePair(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + a [shape=parallelogram, tool_command="echo hi"] + b [shape=parallelogram, tool_command="echo bye"] + start -> a + a -> b [condition="outcome=success"] + a -> exit [condition="outcome!=success"] + b -> exit [condition="outcome=success"] +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := lintAllConditionalEdges(g) + assertNoRule(t, diags, "all_conditional_edges") +} + +// When every outgoing edge from a node targets a terminal node, the engine +// terminates the run on a missed condition by intent — the routing-gap concern +// doesn't apply. terminal_condition_edge governs those edges instead. +func TestValidate_AllConditionalEdges_ExemptWhenAllTargetsTerminal(t *testing.T) { + g, err := dot.Parse([]byte(` digraph G { start [shape=Mdiamond] exit [shape=Msquare] @@ -1527,7 +1575,6 @@ digraph G { start -> a a -> exit [condition="outcome=success"] a -> exit [condition="outcome=fail"] - a -> exit } `)) if err != nil { diff --git a/workflows/build-test/graph.dot b/workflows/build-test/graph.dot index 4a33cb75..95faf789 100644 --- a/workflows/build-test/graph.dot +++ b/workflows/build-test/graph.dot @@ -61,7 +61,7 @@ digraph build_test { check_test -> report_test_fail [condition="outcome=fail"] check_test -> report_test_fail - report_success -> done - report_build_fail -> done - report_test_fail -> done + report_success -> done [condition="outcome=success"] + report_build_fail -> done [condition="outcome=success"] + report_test_fail -> done [condition="outcome=success"] } diff --git a/workflows/coding-loop/graph.dot b/workflows/coding-loop/graph.dot index ae1c80a2..036e8adf 100644 --- a/workflows/coding-loop/graph.dot +++ b/workflows/coding-loop/graph.dot @@ -202,5 +202,5 @@ When finished, write {\"status\":\"success\"} to $KILROY_STAGE_STATUS_PATH or $K reviewer -> done_gate done_gate -> loop_end loop_end -> report - report -> done + report -> done [condition="outcome=success"] } diff --git a/workflows/multi-tool-exercise/graph.dot b/workflows/multi-tool-exercise/graph.dot index ab7b43db..8c9dedaa 100644 --- a/workflows/multi-tool-exercise/graph.dot +++ b/workflows/multi-tool-exercise/graph.dot @@ -68,12 +68,12 @@ digraph multi_tool_exercise { start -> prepare prepare -> claude_write [condition="outcome=success"] - prepare -> done + prepare -> done [condition="outcome!=success"] claude_write -> codex_write codex_write -> opencode_write opencode_write -> combine combine -> validate [condition="outcome=success"] - combine -> done + combine -> done [condition="outcome!=success"] validate -> check_validate check_validate -> done [condition="outcome=success"] check_validate -> done [condition="outcome=fail"]