Skip to content

Commit b5bb1a7

Browse files
intel352claude
andauthored
orchestrator: nil-app guards + transitive-call audit (P2b PR1) (#26)
* refactor(orchestrator): nil-app guards for stateless subset (P2b-T2) Make the 3 stateless steps (step.lsp_diagnose, step.self_improve_validate, step.self_improve_diff) safe to execute with app=nil — the condition the gRPC legacy-bridge imposes (typed_contracts.go AgentPlugin.CreateStep calls factory(name, config, nil)). This unblocks serving them via the workflow-plugin-agent gRPC binary in a subsequent PR (Phase 2b adapter + fold-in). Two nil-guards (additive — only the app==nil branch is new; real-app path unchanged): - findGuardrailsModule (step_agent_execute.go): prepend `if app == nil { return nil }`. Reached transitively by self_improve_validate.Execute -> checkImmutability (:121). - resolveServices (services.go): prepend `if app == nil { return <null bundle> }` BEFORE the `reg := app.SvcRegistry()` panic site (:509). Reached transitively by self_improve_diff.Execute -> postToBlackboard (:118). The null bundle mirrors the all-Null defaults the function builds for an empty registry, so every caller's IsNull()/null check degrades gracefully. lookupLSPProvider (step_lsp_diagnose.go:108) was already guarded — no change. The transitive-call audit (D1/D10 — NOT a flat grep) is recorded as a comment header at the top of stateless_nilapp_test.go. A flat grep on the step files is false-clean: step_self_improve_diff has zero direct s.app derefs yet panics transitively via resolveServices. T1 fold-in compile+launch smoke (de-risk ADR 0053 unconditional fold-in) PASSED prior to this change: cross-compiled clean across the full goreleaser matrix (linux/darwin/windows x amd64/arm64, CGO_ENABLED=0 — no FAIL) and the launched binary served without startup panic (zero stderr, killed by timeout signal — the orchestrator package has no func init(), and its transitive deps don't fatal at startup). The throwaway cmd/foldin-smoke probe was removed; the durable regression test lands in a later task (T5). Evidence: - go test ./orchestrator/ -run TestStatelessNilApp -v -> PASS (all 3 subtests) - go test ./orchestrator/ ./executor/ -> green (real-app path unaffected; A5) - go test -race ./orchestrator/ -> green - go vet ./... -> clean - cd /Users/jon/workspace/ratchet && go build ./... -> clean (published-pin smoke) Co-Authored-By: Claude <noreply@anthropic.com> * docs(orchestrator): clarify audit line numbers are pre-guard sites (P2b-T2) The transitive-audit comment header in stateless_nilapp_test.go cited specific line numbers (findGuardrailsModule :734, resolveServices :509) and labelled them "current". They are the PRE-GUARD origin/master panic sites — the guards this PR adds sit above each cited line, so in the current tree the derefs are a few lines lower. Reworded the header so a future reader tracing "what was the bug" lands on the right code. Doc-only; no behavior change. Co-Authored-By: Claude <noreply@anthropic.com> * test(orchestrator): assert blackboard_warning in diff nil-app subtest Tightens TestStatelessNilApp/self_improve_diff: assert the blackboard_warning output key is present, proving the transitive postToBlackboard -> resolveServices(nil) path actually executed (not just 'no panic'). Regression guard against deleting the postToBlackboard branch. Review nit (PR1 two-stage review). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2584c98 commit b5bb1a7

3 files changed

Lines changed: 238 additions & 1 deletion

File tree

‎orchestrator/services.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,30 @@ type serviceBundle struct {
506506
// Execute, replacing the scattered app.SvcRegistry()["..."].(*ConcreteType)
507507
// casts.
508508
func resolveServices(app modular.Application) serviceBundle {
509+
// app==nil early return: the gRPC legacy-bridge path constructs steps with
510+
// app=nil (typed_contracts.go AgentPlugin.CreateStep -> factory(name,
511+
// config, nil)). The real-app path below dereferences app.SvcRegistry()
512+
// (the next statement), which would panic. Instead return the same
513+
// all-Null bundle the function builds for an empty registry: every
514+
// required-stateful caller checks IsNull(svc) and degrades gracefully, and
515+
// every truly-optional caller already tolerates the Null default. See
516+
// stateless_nilapp_test.go (transitive-call audit) — step_self_improve_diff
517+
// reaches this path transitively via postToBlackboard.
518+
if app == nil {
519+
return serviceBundle{
520+
Blackboard: NullBlackboard{},
521+
ToolRegistry: NullToolRegistry{},
522+
SecretGuard: NullSecretGuard{},
523+
Approval: NullApproval{},
524+
HumanRequest: NullHumanRequest{},
525+
SubAgent: NullSubAgent{},
526+
Skill: NullSkill{},
527+
Container: NullContainer{},
528+
Webhook: NullWebhook{},
529+
Memory: NullMemoryStore{},
530+
Transcript: NullTranscript{},
531+
}
532+
}
509533
reg := app.SvcRegistry()
510534
b := serviceBundle{
511535
Blackboard: NullBlackboard{},
Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
package orchestrator
2+
3+
// Transitive-call audit for the 3 stateless steps under app=nil (P2b-T2, D1/D10).
4+
//
5+
// The gRPC legacy-bridge path (typed_contracts.go AgentPlugin.CreateStep ->
6+
// factory(name, config, nil)) constructs each step with app=nil. For a step to
7+
// be serve-able it must tolerate app=nil: every path from Execute that reaches
8+
// app.SvcRegistry() / s.app.* — DIRECTLY or TRANSITIVELY through helpers in
9+
// services.go / step_agent_execute.go — must be nil-safe (guarded, behind an
10+
// app!=nil check, or in a guarded branch). A flat grep on the step files is
11+
// INSUFFICIENT: step_self_improve_diff has zero direct s.app derefs yet still
12+
// panics transitively via resolveServices.
13+
//
14+
// Audit (line numbers are the PRE-GUARD origin/master sites — i.e. the historical
15+
// unguarded-deref locations. The guards this task adds sit ABOVE each cited line,
16+
// so in the current tree the derefs are a few lines lower; the citations mark the
17+
// panic sites a future reader tracing "what was the bug" needs):
18+
//
19+
// step.lsp_diagnose (step_lsp_diagnose.go)
20+
// Execute:47 -> s.findLSPProvider() -> lookupLSPProvider(s.app) (:103->:107)
21+
// lookupLSPProvider:108 `if app == nil { return nil }` -> GUARDED ✓
22+
// (no other s.app deref in the file)
23+
// DISPOSITION: already nil-safe.
24+
//
25+
// step.self_improve_validate (step_self_improve_validate.go)
26+
// Execute:86 -> lookupLSPProvider(s.app) -> GUARDED ✓ (above)
27+
// Execute:79 (only when currentYAML != "") -> s.checkImmutability
28+
// checkImmutability:121 -> findGuardrailsModule(s.app)
29+
// findGuardrailsModule (step_agent_execute.go:733)
30+
// :734 `for _, svc := range app.SvcRegistry()` -> UNGUARDED ✗
31+
// Execute:104 -> s.runMCPValidation
32+
// runMCPValidation:152 `if s.app == nil { return ... }` -> GUARDED ✓ (pre-check)
33+
// (Execute itself never directly derefs s.app)
34+
// DISPOSITION: needs `if app == nil { return nil }` guard on findGuardrailsModule.
35+
//
36+
// step.self_improve_diff (step_self_improve_diff.go)
37+
// Execute:102 (only when outputToBlackboard && hasChanges) -> s.postToBlackboard
38+
// postToBlackboard:118 -> resolveServices(s.app).Blackboard
39+
// resolveServices (services.go:508)
40+
// :509 `reg := app.SvcRegistry()` -> UNGUARDED ✗
41+
// postToBlackboard:119 `if IsNull(bb) { return err }` runs AFTER resolveServices,
42+
// so it cannot save us from the :509 panic.
43+
// (the step file has NO direct s.app deref — flat grep is false-clean)
44+
// DISPOSITION: needs `if app == nil { return <null bundle> }` guard at the TOP of
45+
// resolveServices, BEFORE the `reg := app.SvcRegistry()` line.
46+
//
47+
// After the two guards above are applied, all three steps are nil-app-safe.
48+
49+
import (
50+
"context"
51+
"testing"
52+
53+
"github.com/GoCodeAlone/modular"
54+
"github.com/GoCodeAlone/workflow/module"
55+
"github.com/GoCodeAlone/workflow/plugin"
56+
)
57+
58+
// TestStatelessNilApp proves the 3 served (stateless) steps tolerate app=nil,
59+
// the condition the gRPC legacy-bridge imposes (factory(name, config, nil)).
60+
// Each subtest constructs the step via its real plugin.StepFactory with app=nil
61+
// — mirroring AgentPlugin.CreateStep's call — and asserts Execute returns a
62+
// graceful result rather than panicking.
63+
func TestStatelessNilApp(t *testing.T) {
64+
t.Run("lsp_diagnose", func(t *testing.T) {
65+
// Factory mirrors AgentPlugin.CreateStep: factory(name, config, nil).
66+
factory := newLSPDiagnoseFactory()
67+
stepAny, err := factory("step.lsp_diagnose", map[string]any{}, nil)
68+
if err != nil {
69+
t.Fatalf("factory returned error: %v", err)
70+
}
71+
step := stepAny.(*LSPDiagnoseStep)
72+
if step.app != nil {
73+
t.Fatalf("expected app==nil on the constructed step (bridge contract)")
74+
}
75+
76+
// Provide yaml content so the step reaches the lookupLSPProvider path
77+
// (the app=nil hazard) rather than the early empty-content return.
78+
pc := &module.PipelineContext{Current: map[string]any{
79+
"yaml": "name: test\n",
80+
}}
81+
82+
result, err := step.Execute(context.Background(), pc)
83+
if err != nil {
84+
t.Fatalf("Execute with app=nil returned error (expected graceful): %v", err)
85+
}
86+
if result == nil {
87+
t.Fatal("Execute returned nil result")
88+
}
89+
// lookupLSPProvider(nil) returns nil -> step returns empty diagnostics.
90+
count, _ := result.Output["count"].(int)
91+
if count != 0 {
92+
t.Errorf("expected 0 diagnostics (no provider when app=nil), got %d", count)
93+
}
94+
})
95+
96+
t.Run("self_improve_validate", func(t *testing.T) {
97+
// Factory mirrors AgentPlugin.CreateStep: factory(name, config, nil).
98+
factory := newSelfImproveValidateFactory()
99+
stepAny, err := factory("step.self_improve_validate", map[string]any{}, nil)
100+
if err != nil {
101+
t.Fatalf("factory returned error: %v", err)
102+
}
103+
step := stepAny.(*SelfImproveValidateStep)
104+
if step.app != nil {
105+
t.Fatalf("expected app==nil on the constructed step (bridge contract)")
106+
}
107+
108+
// Provide BOTH proposed + current YAML so checkImmutability runs (the
109+
// branch that hits findGuardrailsModule(s.app) — the app=nil hazard).
110+
pc := &module.PipelineContext{Current: map[string]any{
111+
"proposed_yaml": "name: foo\nversion: \"2\"\n",
112+
"current_yaml": "name: bar\nversion: \"1\"\n",
113+
}}
114+
115+
result, err := step.Execute(context.Background(), pc)
116+
if err != nil {
117+
t.Fatalf("Execute with app=nil returned error (expected graceful): %v", err)
118+
}
119+
if result == nil {
120+
t.Fatal("Execute returned nil result")
121+
}
122+
// The YAML-schema path runs pre-checkImmutability and is app-free, so
123+
// `valid`/`errors` must still be computed. With well-formed YAML and
124+
// no guardrails module (findGuardrailsModule(nil)->nil), valid==true.
125+
valid, _ := result.Output["valid"].(bool)
126+
if !valid {
127+
t.Errorf("expected valid=true (well-formed YAML, no guardrails under app=nil), got valid=%v, output=%v", valid, result.Output)
128+
}
129+
errs, _ := result.Output["errors"].([]string)
130+
if errs == nil {
131+
t.Errorf("expected errors slice to be present (computed pre-checkImmutability), got nil; output=%v", result.Output)
132+
}
133+
})
134+
135+
t.Run("self_improve_diff", func(t *testing.T) {
136+
// Factory mirrors AgentPlugin.CreateStep: factory(name, config, nil).
137+
// output_to_blackboard=true forces the postToBlackboard path that
138+
// transitively hits resolveServices(s.app) — the app=nil hazard.
139+
factory := newSelfImproveDiffFactory()
140+
stepAny, err := factory("step.self_improve_diff", map[string]any{
141+
"output_to_blackboard": true,
142+
}, nil)
143+
if err != nil {
144+
t.Fatalf("factory returned error: %v", err)
145+
}
146+
step := stepAny.(*SelfImproveDiffStep)
147+
if step.app != nil {
148+
t.Fatalf("expected app==nil on the constructed step (bridge contract)")
149+
}
150+
151+
// Differing proposed/current so hasChanges=true -> postToBlackboard runs.
152+
pc := &module.PipelineContext{Current: map[string]any{
153+
"proposed_yaml": "name: foo\n",
154+
"current_yaml": "name: bar\n",
155+
}}
156+
157+
result, err := step.Execute(context.Background(), pc)
158+
if err != nil {
159+
t.Fatalf("Execute with app=nil returned error (expected graceful): %v", err)
160+
}
161+
if result == nil {
162+
t.Fatal("Execute returned nil result")
163+
}
164+
// The diff itself is computed app-free; only the Blackboard post is
165+
// gated. resolveServices(nil) returns a null bundle whose Blackboard
166+
// IsNull()s -> postToBlackboard records a non-fatal warning instead of
167+
// panicking. The diff output must survive.
168+
diff, _ := result.Output["diff"].(string)
169+
if diff == "" {
170+
t.Errorf("expected non-empty diff (content differs), got empty; output=%v", result.Output)
171+
}
172+
hasChanges, _ := result.Output["has_changes"].(bool)
173+
if !hasChanges {
174+
t.Errorf("expected has_changes=true, got false; output=%v", result.Output)
175+
}
176+
// postToBlackboard ran (output_to_blackboard=true + hasChanges) against a
177+
// null-bundle Blackboard -> it records a non-fatal blackboard_warning.
178+
// Asserting its presence proves the transitive resolveServices(nil) path
179+
// executed — a regression guard: deleting the postToBlackboard branch
180+
// would drop this key and fail the test.
181+
if _, ok := result.Output["blackboard_warning"]; !ok {
182+
t.Errorf("expected blackboard_warning (proof postToBlackboard ran the transitive resolveServices(nil) path), got output=%v", result.Output)
183+
}
184+
})
185+
}
186+
187+
// Compile-time guard: the 3 served steps' factories are plugin.StepFactory
188+
// (the type AgentPlugin.CreateStep dispatches through). If a factory signature
189+
// drifts, this fails to compile.
190+
var _ plugin.StepFactory = newLSPDiagnoseFactory()
191+
var _ plugin.StepFactory = newSelfImproveValidateFactory()
192+
var _ plugin.StepFactory = newSelfImproveDiffFactory()
193+
194+
// Compile-time guard: the step types implement the Execute signature that
195+
// legacyStepInstance wraps (typed_contracts.go:169).
196+
var _ interface {
197+
Execute(context.Context, *module.PipelineContext) (*module.StepResult, error)
198+
} = (*LSPDiagnoseStep)(nil)
199+
var _ interface {
200+
Execute(context.Context, *module.PipelineContext) (*module.StepResult, error)
201+
} = (*SelfImproveValidateStep)(nil)
202+
var _ interface {
203+
Execute(context.Context, *module.PipelineContext) (*module.StepResult, error)
204+
} = (*SelfImproveDiffStep)(nil)
205+
206+
// silence unused-import for modular in case the compile-time guards above are
207+
// the only references (they aren't, but keeps the file robust to edits).
208+
var _ modular.Application

‎orchestrator/step_agent_execute.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -729,8 +729,13 @@ func findSSEHub(app modular.Application) *SSEHub {
729729
}
730730

731731
// findGuardrailsModule searches the service registry for a GuardrailsModule instance.
732-
// Returns nil if no guardrails module is registered.
732+
// Returns nil if no guardrails module is registered, or if app is nil (the gRPC
733+
// legacy-bridge path constructs steps with app=nil — see stateless_nilapp_test.go
734+
// and the transitive-call audit header).
733735
func findGuardrailsModule(app modular.Application) *GuardrailsModule {
736+
if app == nil {
737+
return nil
738+
}
734739
for _, svc := range app.SvcRegistry() {
735740
if gm, ok := svc.(*GuardrailsModule); ok {
736741
return gm

0 commit comments

Comments
 (0)