diff --git a/run/critic.go b/run/critic.go index c563550..5b7d2a3 100644 --- a/run/critic.go +++ b/run/critic.go @@ -33,7 +33,7 @@ func (e *Executor) startCritic(runCtx context.Context, cancel context.CancelFunc } soft := e.cfg.Defaults.CriticSoftTimeout if soft <= 0 { - return nil, noop + soft = 90 * time.Second // defensive: withFallbacks normally guarantees >0 } h := e.cfg.Ports.Critic.Monitor(runCtx, info, soft) if h == nil { @@ -41,6 +41,10 @@ func (e *Executor) startCritic(runCtx context.Context, cancel context.CancelFunc } done := make(chan struct{}) go func() { + // A host CriticHandle.Deadline() that panics must not crash the process + // (this runs on its own goroutine, so the executor's top-level recover + // can't catch it). Log-free best-effort: just stop watching. + defer func() { _ = recover() }() t := time.NewTicker(criticDeadlineCheck) defer t.Stop() for { diff --git a/run/delivery_test.go b/run/delivery_test.go index eafda1e..b7218ad 100644 --- a/run/delivery_test.go +++ b/run/delivery_test.go @@ -67,22 +67,48 @@ func TestNoDeliveryWithoutTarget(t *testing.T) { } } -func TestDeliveryErrorPath(t *testing.T) { +// TestNoDeliveryOnEarlyResolveError: an error BEFORE the run starts (model +// resolve) returns before delivery is reached — neither Deliver nor DeliverError +// fires. (Delivery covers run OUTCOMES, not pre-run setup failures.) +func TestNoDeliveryOnEarlyResolveError(t *testing.T) { d := &recordingDelivery{} ex := run.New(run.Config{ Registry: tool.NewRegistry(), Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { - return ctx, nil, errors.New("resolve boom") // forces a run error + return ctx, nil, errors.New("resolve boom") }, Ports: run.Ports{Delivery: d}, }) ex.Run(context.Background(), run.RunnableAgent{Name: "x", ModelTier: "m"}, tool.Invocation{RunID: "r", DeliveryKind: "channel", DeliveryID: "chan-9"}, "go") - // A model-resolve error returns before the run context exists, so delivery - // isn't reached — assert no spurious Deliver. (DeliverError on in-loop errors - // is exercised by the wiring; this guards the early-return path.) - if d.delivers != 0 { - t.Errorf("early failure should not Deliver, got %d", d.delivers) + if d.delivers != 0 || d.errored != nil { + t.Errorf("early resolve failure should neither Deliver nor DeliverError: delivers=%d errored=%v", d.delivers, d.errored) + } +} + +// TestDeliverErrorOnRunFailure: an in-loop run failure (the model errors) routes +// through DeliverError with the run error. +func TestDeliverErrorOnRunFailure(t *testing.T) { + d := &recordingDelivery{} + fp := fake.New("fake") + fp.Enqueue("m", fake.Step{Err: errors.New("model boom")}) // model errors mid-run + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + Ports: run.Ports{Delivery: d}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m"}, + tool.Invocation{RunID: "r", DeliveryKind: "channel", DeliveryID: "chan-9"}, "go") + if res.Err == nil { + t.Fatal("expected a run error") + } + if d.delivers != 0 { + t.Errorf("a failed run should not Deliver (success path), got %d", d.delivers) + } + if d.errored == nil || d.target.ID != "chan-9" { + t.Errorf("a failed run with a target should DeliverError to chan-9, got errored=%v target=%+v", d.errored, d.target) } } diff --git a/run/executor.go b/run/executor.go index 7ac8d72..2dab0bd 100644 --- a/run/executor.go +++ b/run/executor.go @@ -104,10 +104,19 @@ type Result struct { } // Run executes ra with the given invocation + input and returns the Result. It -// never propagates a panic; failures surface in Result.Err. -func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocation, input string) Result { +// never propagates a panic; failures surface in Result.Err (a top-level recover +// converts any panic — including from a host Port — into a run error). +func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocation, input string) (res Result) { started := time.Now() - res := Result{RunID: inv.RunID} + res = Result{RunID: inv.RunID} + // Enforce the no-panic contract: a panic anywhere in the run (incl. a host + // Critic/Audit/Palette callback on the main goroutine) becomes Result.Err + // rather than unwinding into the caller. + defer func() { + if r := recover(); r != nil { + res.Err = fmt.Errorf("run.Executor: recovered panic: %v", r) + } + }() tier := ra.ModelTier if tier == "" { diff --git a/run/ports.go b/run/ports.go index e18ac74..0a5dfe9 100644 --- a/run/ports.go +++ b/run/ports.go @@ -113,6 +113,11 @@ type Critic interface { } // CriticHandle is the executor's live link to a run's critic. +// +// Concurrency: the executor calls RecordStep/RecordToolStart/Steer from the run +// goroutine while a separate watch goroutine polls Deadline() and the run's end +// calls Stop() — so implementations MUST be safe for concurrent use across these +// methods (the critic battery's handle guards its state with a mutex). type CriticHandle interface { // RecordStep / RecordToolStart keep the critic's activity clock fresh so a // healthy-but-slow run is not mistaken for a hang.