From 9f0adfd156e6e7749031777cae919d5c56ba7044 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 26 Oct 2026 12:29:59 +0200 Subject: [PATCH 1/4] fix(guardrails): stop yolo sessions waiting on the guardrail checker Yolo-mode sessions demoted guardrail review to advisory, but the engine still ran every review inline: each matching Read, Grep, Shell, Edit and web call waited for a full checker-model run whose verdict was then discarded. Two paths (terminal evidence failures and principal-revision changes) could still fail closed in yolo, contradicting the documented posture. Yolo sessions now skip the checker by default. Operators can opt in with guardrails.yoloAdvisory: true, which runs matching action and inbound reviews in the background: the tool never waits, is never blocked or held, checker failures never fail closed, and verdicts are emitted as advisory guardrail events. Run end joins in-flight reviews for up to 10s (plus 1s grace) so their usage is recorded, then cancels the rest. Narrower delegated children of a yolo parent keep enforcing. Adds agent.YoloReviewPolicyProvider (engine API, Added) and records the yolo behavior change in the engine changelog. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/governance.md | 4 +- engine/CHANGELOG.md | 17 ++ engine/agent/actionreview.go | 8 +- engine/agent/backgroundreview.go | 194 +++++++++++++++++ engine/agent/dispatch.go | 9 +- engine/agent/inboundreview.go | 50 ++++- engine/agent/loop.go | 4 + engine/agent/review.go | 15 +- engine/agent/yolo_review_test.go | 205 ++++++++++++++++++ engine/api/agent.txt | 2 + .../adapter/permconfig/guardrails_test.go | 22 ++ internal/adapter/permconfig/schema.go | 7 + internal/app/build.go | 8 + .../app/contextual_build_evidence_test.go | 5 +- internal/app/guardrails.go | 11 +- .../app/guardrails_approve_once_e2e_test.go | 8 +- internal/app/guardrails_yolo_test.go | 75 +++++++ internal/app/permission_mode.go | 7 +- internal/configgen/settings.skeleton.yaml | 6 + .../permissions-and-posture.md | 16 +- user-docs/reference/configuration.md | 1 + 21 files changed, 640 insertions(+), 34 deletions(-) create mode 100644 engine/agent/backgroundreview.go create mode 100644 engine/agent/yolo_review_test.go create mode 100644 internal/app/guardrails_yolo_test.go diff --git a/docs/architecture/governance.md b/docs/architecture/governance.md index bb8586f6b2..d1e56a7798 100644 --- a/docs/architecture/governance.md +++ b/docs/architecture/governance.md @@ -76,7 +76,9 @@ child prompt-injection defense. `Service.admitMode` (`internal/adapter/server/service.go`) refuses a created or switched mode above the ceiling, and `auto` without an enforcing guardrails checker; a server whose default mode is `auto` refuses to start without one. Unchecked autonomy is the -explicit `yolo` choice, where a configured checker is advisory. `admitStoredMode` +explicit `yolo` choice, where a configured checker never blocks: the engine skips it +unless the reviewer opts in (`YoloReviewPolicyProvider`), and then reviews off the +dispatch path and joins in-flight reviews, bounded, before the run ends. `admitStoredMode` re-checks a persisted session's mode at run, approval resume, fork, and clear, so a mode admitted under a looser deployment fails with `FailedPrecondition` until the owner switches it; history is untouched. A session parked awaiting an approval in such a mode diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index 3c04699626..cf18db5d63 100644 --- a/engine/CHANGELOG.md +++ b/engine/CHANGELOG.md @@ -13,6 +13,14 @@ The covered surface is the eight core packages (`session`, `governance`, `learni ### Added +- **Yolo advisory review opt-in** — adds `agent.YoloReviewPolicyProvider`. A + `ToolReviewer` that implements it and returns true from `GuardrailYoloReviews` + has its applicable action and inbound reviews in yolo-mode sessions run in the + background: the tool call neither waits for the verdict nor fails closed, and + the verdict is emitted as an advisory guardrail `EvHook` after the tool result + and before the run's terminal event. Run end waits up to 10s (plus a 1s grace) + for in-flight reviews, then cancels them. Added (minor). + - **Provider stop detail** — adds `port.Chunk.StopDetail`, the provider's own terminal label (for example `max_tokens`, `refusal`, `length`) that an adapter collapsed into `StopError` on `ChunkDone`. A stop-only `StopError` run now names @@ -455,6 +463,15 @@ The covered surface is the eight core packages (`session`, `governance`, `learni ### Changed +- **Yolo-mode sessions skip contextual review by default** — a yolo-mode session + no longer calls the `ToolReviewer` for action or inbound review unless the + reviewer implements `YoloReviewPolicyProvider` and opts in. Previously review + ran inline and the tool waited for an advisory verdict. A terminal review + failure (for example evidence preparation) no longer fails closed in a yolo + session. Narrower delegated children of a yolo parent keep enforcing. + Changed (breaking for embedders that relied on inline yolo review; pre-v1 + minor bump). + - **Agent-bound authority enforcement** — `Session.BindAuthority` requires a ceiling when `AgentDefinitionName` is set and rejects tools outside that ceiling. `GrantToolAuthority` and `CompleteWorkspaceEnrollment` reject tool grants beyond diff --git a/engine/agent/actionreview.go b/engine/agent/actionreview.go index 6c22d99a97..09f9fbfa65 100644 --- a/engine/agent/actionreview.go +++ b/engine/agent/actionreview.go @@ -818,11 +818,8 @@ func (e *Engine) resolveActionAssessment(ctx context.Context, r *Run, sess *sess result.Assessment = ReviewUnresolved } _, enforce := reviewPolicy(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction, assessment.err != nil) - if assessment.err != nil { - var terminal GuardrailReviewTerminalFailure - if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { - enforce = true - } + if assessment.err != nil && terminalReviewEnforces(sess.Mode, assessment.err) { + enforce = true } r.reviewRoot.record(reviewFact(call, assessment.action.request.Target, decision)) r.publishActionDetail(ctx, sess.ID, assessment.action.request.ReviewID, assessment.action.request, result, assessment.err) @@ -944,6 +941,7 @@ func (e *Engine) reviewActionWithTail(ctx context.Context, r *Run, sess *session e.publishToolResult(r, session.Event{Type: session.EvToolResult, Turn: turnIdx, ToolResult: ptr(res)}) return res, nil, false, false } + e.startBackgroundActionReview(ctx, r, sess, env, turnIdx, call) return session.ToolResult{}, nil, false, true } assessment := e.prepareActionAssessment(ctx, r, sess, env, call) diff --git a/engine/agent/backgroundreview.go b/engine/agent/backgroundreview.go new file mode 100644 index 0000000000..b616f05c06 --- /dev/null +++ b/engine/agent/backgroundreview.go @@ -0,0 +1,194 @@ +package agent + +import ( + "context" + "sync" + "time" + + "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/port" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" +) + +// backgroundReviewDrainCap bounds how long run end waits for in-flight yolo +// advisory reviews before cancelling them, and backgroundReviewDrainGrace how +// long it then waits for cancelled reviews to unwind. Vars only as TEST seams, +// like childDrainCap. +var ( + backgroundReviewDrainCap = 10 * time.Second + backgroundReviewDrainGrace = 1 * time.Second +) + +// backgroundReviews is a run's set of opted-in yolo advisory reviews. Reviews +// start on the dispatching goroutine after their request is snapshotted, run +// the checker off the dispatch path, and publish only before drain seals the set, +// so no advisory event follows the run's terminal event. +type backgroundReviews struct { + mu sync.Mutex + wg sync.WaitGroup + ctx context.Context + cancel context.CancelFunc + closed bool + sealed bool +} + +// context returns the set's review context, detached from the run's +// cancellation so a review outlives the tool call it observes until drain. +func (b *backgroundReviews) context(parent context.Context) (context.Context, bool) { + b.mu.Lock() + defer b.mu.Unlock() + if b.closed { + return nil, false + } + if b.ctx == nil { + b.ctx, b.cancel = context.WithCancel(context.WithoutCancel(parent)) + } + return b.ctx, true +} + +func (b *backgroundReviews) start(fn func()) bool { + b.mu.Lock() + defer b.mu.Unlock() + if b.closed { + return false + } + b.wg.Add(1) + go func() { + defer b.wg.Done() + fn() + }() + return true +} + +// publish runs fn unless drain has sealed the set or cancelled its context. +func (b *backgroundReviews) publish(fn func()) { + b.mu.Lock() + defer b.mu.Unlock() + if b.sealed || b.ctx.Err() != nil { + return + } + fn() +} + +// drain stops new reviews, waits up to the cap for in-flight ones, cancels the +// rest, and seals publication. It reports whether any review failed to unwind. +func (b *backgroundReviews) drain() bool { + b.mu.Lock() + b.closed = true + cancel := b.cancel + b.mu.Unlock() + abandoned := false + if cancel != nil { + if !waitGroupWithin(&b.wg, backgroundReviewDrainCap) { + cancel() + abandoned = !waitGroupWithin(&b.wg, backgroundReviewDrainGrace) + } + cancel() + } + b.mu.Lock() + b.sealed = true + b.mu.Unlock() + return abandoned +} + +func waitGroupWithin(wg *sync.WaitGroup, d time.Duration) bool { + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-done: + return true + case <-timer.C: + return false + } +} + +func (r *Run) drainBackgroundReviews(ctx context.Context) { + if r.backgroundReviews.drain() { + r.diag.Log(ctx, port.LevelWarn, "abandoning yolo advisory guardrail reviews that did not stop within the run-end drain cap", + "cap", (backgroundReviewDrainCap + backgroundReviewDrainGrace).String()) + } +} + +func advisoryDisposition(result ToolReviewResult, err error, clean string) string { + if err != nil || result.Assessment != ReviewAcceptable { + return "pass_advisory" + } + return clean +} + +// startBackgroundActionReview reviews an opted-in yolo action without delaying +// it. The call has already been authorized and proceeds regardless of verdict. +func (e *Engine) startBackgroundActionReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall) { + if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { + return + } + bg, ok := r.backgroundReviews.context(ctx) + if !ok { + return + } + assessment := e.prepareActionAssessment(bg, r, sess, env, call) + sessID := sess.ID + started := r.backgroundReviews.start(func() { + if assessment.action.close != nil { + defer assessment.action.close() + } + assessAction(bg, r, &assessment) + r.enqueueAuxiliaryUsageWhileActive(bg, RemapAuxiliaryUsage(bg, r.diag, session.UsageKindGuardrail, assessment.usage)) + result := assessment.result + decision := string(result.Assessment) + if assessment.err != nil { + decision = "unresolved" + result.Assessment = ReviewUnresolved + } + r.backgroundReviews.publish(func() { + r.reviewRoot.record(reviewFact(call, assessment.action.request.Target, decision)) + r.publishActionDetail(bg, sessID, assessment.action.request.ReviewID, assessment.action.request, result, assessment.err) + disposition := advisoryDisposition(result, assessment.err, "execute") + e.emit(r, session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePreToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: reviewMachinePayload(r.reviewRoot.reviewer, assessment.action.request, result, assessment.err, disposition)}}) + }) + }) + if !started { + assessment.cancel() + if assessment.action.close != nil { + assessment.action.close() + } + } +} + +// startBackgroundInboundReview inspects an opted-in yolo result that has +// already been released to the model; the verdict is advisory only. +func (e *Engine) startBackgroundInboundReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall, result session.ToolResult) { + if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { + return + } + bg, ok := r.backgroundReviews.context(ctx) + if !ok { + return + } + assessment := inboundAssessment{applies: true, mode: sess.Mode} + e.prepareInboundRequest(bg, r, sess, env, call, result, &assessment) + sessID := sess.ID + started := r.backgroundReviews.start(func() { + if assessment.close != nil { + defer assessment.close() + } + assessInbound(bg, r, &assessment) + r.enqueueAuxiliaryUsageWhileActive(bg, RemapAuxiliaryUsage(bg, r.diag, session.UsageKindGuardrail, assessment.usage)) + r.backgroundReviews.publish(func() { + r.publishInboundDetail(bg, sessID, assessment) + e.emitInboundReview(r, turnIdx, call, assessment, advisoryDisposition(assessment.result, assessment.err, "release_result")) + }) + }) + if !started { + assessment.cancel() + if assessment.close != nil { + assessment.close() + } + } +} diff --git a/engine/agent/dispatch.go b/engine/agent/dispatch.go index 81371f7a1d..54e888b24c 100644 --- a/engine/agent/dispatch.go +++ b/engine/agent/dispatch.go @@ -361,7 +361,7 @@ func (e *Engine) runReadBatch(ctx context.Context, r *Run, sess *session.Session // Human permission and pre-hook waits are complete. Prepare each sibling // serially, launching its private checker before preparing the next. - e.fanOutReadBatchActionAssessments(ctx, r, sess, env, prepared) + e.fanOutReadBatchActionAssessments(ctx, r, sess, env, turnIdx, prepared) if ctx.Err() != nil { r.recordCompletedActionUsage(ctx, sess, prepared) return out, true @@ -422,7 +422,7 @@ func (e *Engine) runReadBatch(ctx context.Context, r *Run, sess *session.Session // fanOutReadBatchActionAssessments starts every eligible private action review, // then joins them before the ordered decision drain begins. -func (e *Engine) fanOutReadBatchActionAssessments(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, prepared []readBatchPending) { +func (e *Engine) fanOutReadBatchActionAssessments(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, turnIdx int, prepared []readBatchPending) { var reviewWG sync.WaitGroup for i := range prepared { if ctx.Err() != nil { @@ -440,6 +440,8 @@ func (e *Engine) fanOutReadBatchActionAssessments(ctx context.Context, r *Run, s defer reviewWG.Done() assessAction(ctx, r, a) }(p.assessment) + } else { + e.startBackgroundActionReview(ctx, r, sess, env, turnIdx, p.call) } } reviewWG.Wait() @@ -1754,6 +1756,9 @@ func (e *Engine) executePrivate(ctx context.Context, r *Run, sess *session.Sessi if r.reviewRoot != nil && r.reviewRoot.reviewer != nil { assessment = e.prepareInboundAssessment(ctx, r, sess, env, c, res) assessInbound(ctx, r, &assessment) + if !assessment.applies { + e.startBackgroundInboundReview(ctx, r, sess, env, turnIdx, c, res) + } } return executionRecord{ result: res, queued: queued, duration: dur, assessment: assessment, postEvents: postEvents, diff --git a/engine/agent/inboundreview.go b/engine/agent/inboundreview.go index 365966f221..e529ba0482 100644 --- a/engine/agent/inboundreview.go +++ b/engine/agent/inboundreview.go @@ -48,10 +48,18 @@ type inboundAssessment struct { // reviewPolicy resolves the reviewer's configured applicability and enforcement // for the EXECUTING session's mode. A shared reviewRoot carries the operator's -// undemoted rules, so a yolo session (no enforcing checker; advisory monitoring -// only) demotes enforcement here while a narrower delegated child of that yolo -// parent keeps it. +// undemoted rules, so a yolo session never reviews inline here while a narrower +// delegated child of that yolo parent keeps enforcing. Opted-in yolo review runs +// through yoloBackgroundReview instead. func reviewPolicy(reviewer ToolReviewer, mode session.PermissionMode, toolName string, job ReviewJob, operationalFailure bool) (bool, bool) { + if mode == session.ModeYolo { + return false, false + } + return configuredReviewPolicy(reviewer, toolName, job, operationalFailure) +} + +// configuredReviewPolicy is the operator's policy independent of permission mode. +func configuredReviewPolicy(reviewer ToolReviewer, toolName string, job ReviewJob, operationalFailure bool) (bool, bool) { applies, enforce := job == ReviewJobAction, true if policy, ok := reviewer.(ReviewPolicyProvider); ok { applies, enforce = policy.GuardrailReviewPolicy(toolName, job, operationalFailure) @@ -59,7 +67,27 @@ func reviewPolicy(reviewer ToolReviewer, mode session.PermissionMode, toolName s // ToolReviewer predated inbound escrow. Reviewers that do not explicitly // advertise policy retain action-only behavior rather than unexpectedly // inspecting and holding every tool result. - return applies, enforce && mode != session.ModeYolo + return applies, enforce +} + +// terminalReviewEnforces reports whether err is an authority-relevant terminal +// failure that must fail closed. Yolo never fails closed on checker problems. +func terminalReviewEnforces(mode session.PermissionMode, err error) bool { + var terminal GuardrailReviewTerminalFailure + return mode != session.ModeYolo && errors.As(err, &terminal) && terminal.GuardrailReviewTerminalFailure() +} + +// yoloBackgroundReview reports whether a yolo session runs this review as +// advisory background work: only when the reviewer opts in and a rule applies. +func yoloBackgroundReview(reviewer ToolReviewer, mode session.PermissionMode, toolName string, job ReviewJob) bool { + if mode != session.ModeYolo || reviewer == nil { + return false + } + if opt, ok := reviewer.(YoloReviewPolicyProvider); !ok || !opt.GuardrailYoloReviews() { + return false + } + applies, _ := configuredReviewPolicy(reviewer, toolName, job, false) + return applies } func (e *Engine) prepareInboundAssessment(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall, result session.ToolResult) inboundAssessment { @@ -71,6 +99,13 @@ func (e *Engine) prepareInboundAssessment(ctx context.Context, r *Run, sess *ses if !applies { return assessment } + e.prepareInboundRequest(ctx, r, sess, env, call, result, &assessment) + return assessment +} + +// prepareInboundRequest snapshots the review request and evidence for an +// applicable inbound assessment. It must run on the dispatching goroutine. +func (e *Engine) prepareInboundRequest(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall, result session.ToolResult, assessment *inboundAssessment) { reviewCtx, cancel := context.WithTimeoutCause(ctx, reviewAssessmentTimeout, reviewFailure(ReviewFailureTimeout)) assessment.ctx, assessment.cancel = reviewCtx, cancel e.establishReviewPrincipal(r) @@ -104,7 +139,6 @@ func (e *Engine) prepareInboundAssessment(ctx context.Context, r *Run, sess *ses } assessment.request.Evidence, assessment.request.EvidenceComplete = prepared.Evidence, prepared.Complete assessment.source, assessment.close, assessment.err = prepared.Source, prepared.Close, prepareErr - return assessment } func assessInbound(parent context.Context, r *Run, assessment *inboundAssessment) { @@ -123,8 +157,7 @@ func assessInbound(parent context.Context, r *Run, assessment *inboundAssessment assessment.result.Assessment = ReviewUnresolved recordReviewFailure(r.reviewRoot.reviewer, assessment.result, assessment.err) _, assessment.enforce = reviewPolicy(r.reviewRoot.reviewer, assessment.mode, assessment.request.EffectiveCall.Name, ReviewJobInbound, true) - var terminal GuardrailReviewTerminalFailure - if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { + if terminalReviewEnforces(assessment.mode, assessment.err) { assessment.enforce = true } return @@ -144,8 +177,7 @@ func assessInbound(parent context.Context, r *Run, assessment *inboundAssessment } if assessment.err != nil { _, assessment.enforce = reviewPolicy(r.reviewRoot.reviewer, assessment.mode, assessment.request.EffectiveCall.Name, ReviewJobInbound, true) - var terminal GuardrailReviewTerminalFailure - if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { + if terminalReviewEnforces(assessment.mode, assessment.err) { assessment.enforce = true } } diff --git a/engine/agent/loop.go b/engine/agent/loop.go index d3f88c2069..6a3b221b76 100644 --- a/engine/agent/loop.go +++ b/engine/agent/loop.go @@ -857,6 +857,9 @@ type Run struct { // conversation content crosses that seam. reviewRoot *reviewRoot ownsReviewRoot bool + // backgroundReviews tracks opted-in yolo advisory reviews still running off + // the dispatch path; drainChildren joins them before the terminal event. + backgroundReviews backgroundReviews // currentPrompt is the accepted genuine prompt for this run. Completion locates // it in the final history; if compaction removed it, automatic admission gets an // invalid span and fails closed. @@ -3690,6 +3693,7 @@ var childDrainGrace = 1 * time.Second // BEFORE its clean-terminal calls — by the time this hook runs, the children it // would ask about are already cancelled and the registry sealed. func (e *Engine) drainChildren(ctx context.Context, r *Run) { + r.drainBackgroundReviews(ctx) joins := r.children.cancelLiveBackground() if len(joins) > 0 { if pending := joinChildren(joins, childDrainCap); len(pending) > 0 { diff --git a/engine/agent/review.go b/engine/agent/review.go index 5c37057719..43bd52401b 100644 --- a/engine/agent/review.go +++ b/engine/agent/review.go @@ -158,13 +158,22 @@ type ReviewEvidencePreparer interface { // checker-down posture. The returns report whether review applies and whether a // finding or unresolved assessment is enforced. A ToolReviewer without this // capability retains action-only enforcement. Report the operator's configured -// enforcement independent of permission mode: the engine demotes it to advisory -// for each executing yolo-mode session, including delegated children that share -// the reviewer. +// enforcement independent of permission mode: the engine never enforces review +// in an executing yolo-mode session, including delegated children that share the +// reviewer, and skips it there unless YoloReviewPolicyProvider opts in. type ReviewPolicyProvider interface { GuardrailReviewPolicy(toolName string, job ReviewJob, operationalFailure bool) (applies, enforce bool) } +// YoloReviewPolicyProvider reports whether the operator opted in to advisory +// review of yolo-mode sessions. Without it, or when it returns false, a yolo +// session calls no checker. When it returns true, each applicable review runs +// in the background: the tool call neither waits for the verdict nor fails +// closed, and the verdict is published as an advisory guardrail event. +type YoloReviewPolicyProvider interface { + GuardrailYoloReviews() bool +} + // PermissionReviewPolicyProvider reports whether an exact child Shell call has // enabled, enforcing, matching action-review coverage. Advisory, skipped, and // unmatched rules must return false. The engine never consults it for a diff --git a/engine/agent/yolo_review_test.go b/engine/agent/yolo_review_test.go new file mode 100644 index 0000000000..db8574d410 --- /dev/null +++ b/engine/agent/yolo_review_test.go @@ -0,0 +1,205 @@ +package agent + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stacklok/mecatl/engine/adapter/mockllm" + "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" +) + +// yoloReviewer reviews both jobs. optIn controls GuardrailYoloReviews; gate, when +// set, parks every Review until it is closed or the review context ends. +type yoloReviewer struct { + optIn bool + gate chan struct{} + result ToolReviewResult + err error + + mu sync.Mutex + reviews int + cancelled int +} + +func (r *yoloReviewer) Review(ctx context.Context, _ ToolReviewRequest, _ ReviewEvidenceSource) (ToolReviewResult, session.AuxiliaryUsage, error) { + r.mu.Lock() + r.reviews++ + r.mu.Unlock() + if r.gate != nil { + select { + case <-r.gate: + case <-ctx.Done(): + r.mu.Lock() + r.cancelled++ + r.mu.Unlock() + return ToolReviewResult{Assessment: ReviewUnresolved}, session.AuxiliaryUsage{}, ctx.Err() + } + } + usage := session.Usage{InputTokens: 5, OutputTokens: 1} + return r.result, session.AuxiliaryUsage{Buckets: map[session.UsageKind]session.TokenUsage{ + session.UsageKindGuardrail: {Total: usage, Models: map[string]session.Usage{"checker/model": usage}}, + }}, r.err +} + +func (*yoloReviewer) GuardrailReviewPolicy(string, ReviewJob, bool) (bool, bool) { return true, true } +func (r *yoloReviewer) GuardrailYoloReviews() bool { return r.optIn } + +func (r *yoloReviewer) counts() (int, int) { + r.mu.Lock() + defer r.mu.Unlock() + return r.reviews, r.cancelled +} + +func runYoloReview(t *testing.T, reviewer *yoloReviewer, preparer ReviewEvidencePreparer, onEvent func(session.Event)) ([]session.Event, int, *session.Session) { + t.Helper() + executions := 0 + catalog := tool.NewCatalog() + catalog.MustRegister(revisionActionTool{executions: &executions}) + engine := NewEngine(Deps{ + LLM: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("call", "Act", []byte(`{}`))), mockllm.TextTurn("done")), + Catalog: catalog, + Policy: terminalFailurePolicy{job: ReviewJobAction}, + ToolReviewer: reviewer, + ReviewEvidencePreparer: preparer, + }) + env := memEnv("/ws") + sess := session.New("yolo-review", session.ModeYolo, env.Ref(), session.Limits{}, time.Unix(0, 0)) + var events []session.Event + for ev := range engine.Run(context.Background(), sess, env, RunRequest{Text: "act"}).Events() { + events = append(events, ev) + if onEvent != nil { + onEvent(ev) + } + } + return events, executions, sess +} + +func guardrailHooks(events []session.Event) []session.Event { + var hooks []session.Event + for _, ev := range events { + if ev.Hook != nil && ev.Hook.Guardrail != nil { + hooks = append(hooks, ev) + } + } + return hooks +} + +func TestYoloSkipsGuardrailReviewWithoutOptIn(t *testing.T) { + reviewer := &yoloReviewer{result: ToolReviewResult{Assessment: ReviewProhibited}} + events, executions, _ := runYoloReview(t, reviewer, nil, nil) + if reviews, _ := reviewer.counts(); reviews != 0 { + t.Fatalf("reviews = %d, want 0: yolo without opt-in must not call the checker", reviews) + } + if executions != 1 { + t.Fatalf("executions = %d, want 1", executions) + } + if hooks := guardrailHooks(events); len(hooks) != 0 { + t.Fatalf("guardrail events = %+v, want none", hooks) + } +} + +func TestYoloOptInReviewsRunInBackgroundWithoutDelayingTheTool(t *testing.T) { + gate := make(chan struct{}) + reviewer := &yoloReviewer{optIn: true, gate: gate, result: ToolReviewResult{Assessment: ReviewProhibited}} + var once sync.Once + // Safety release: a blocking review would otherwise hang the run; releasing it + // late turns that regression into the ordering failure asserted below. + safety := time.AfterFunc(5*time.Second, func() { once.Do(func() { close(gate) }) }) + defer safety.Stop() + events, executions, sess := runYoloReview(t, reviewer, nil, func(ev session.Event) { + // The checker is still parked when the canonical result arrives; only then + // is it released, so a blocking review would deadlock this run. + if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.CallID == "call" { + once.Do(func() { close(gate) }) + } + }) + if executions != 1 { + t.Fatalf("executions = %d, want 1", executions) + } + resultAt, terminalAt := -1, -1 + for i, ev := range events { + if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.CallID == "call" { + if ev.ToolResult.IsError || ev.ToolResult.Content != "executed" { + t.Fatalf("tool result = %+v, want the released original", ev.ToolResult) + } + resultAt = i + } + if ev.Type == session.EvResult { + terminalAt = i + } + } + if resultAt < 0 || terminalAt < 0 { + t.Fatalf("missing tool result (%d) or terminal event (%d)", resultAt, terminalAt) + } + phases := map[string]string{} + for i, ev := range events { + if ev.Hook == nil || ev.Hook.Guardrail == nil { + continue + } + if i < resultAt || i > terminalAt { + t.Fatalf("guardrail event at %d, want between tool result %d and terminal %d", i, resultAt, terminalAt) + } + phases[ev.Hook.Phase] = ev.Hook.Guardrail.Disposition + } + if phases[string(governance.PhasePreToolUse)] != "pass_advisory" || phases[string(governance.PhasePostToolUse)] != "pass_advisory" { + t.Fatalf("advisory dispositions = %v, want pass_advisory for both phases", phases) + } + if reviews, _ := reviewer.counts(); reviews != 2 { + t.Fatalf("reviews = %d, want action and inbound", reviews) + } + if got := sess.TokenUsageSnapshot()[session.UsageKindGuardrail].Total; got.InputTokens != 10 { + t.Fatalf("guardrail usage = %+v, want both background reviews recorded", got) + } +} + +func TestYoloOptInTerminalEvidenceFailureNeverFailsClosed(t *testing.T) { + reviewer := &yoloReviewer{optIn: true, result: ToolReviewResult{Assessment: ReviewAcceptable}} + events, executions, _ := runYoloReview(t, reviewer, failingEvidencePreparer{err: errors.New("evidence unavailable")}, nil) + if executions != 1 { + t.Fatalf("executions = %d, want 1: yolo must not fail closed", executions) + } + for _, ev := range events { + if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.CallID == "call" && (ev.ToolResult.IsError || ev.ToolResult.Content != "executed") { + t.Fatalf("tool result = %+v, want the released original", ev.ToolResult) + } + if ev.Type == session.EvPermissionAsk { + t.Fatal("yolo must not surface a guardrail ask") + } + } + hooks := guardrailHooks(events) + if len(hooks) != 2 { + t.Fatalf("guardrail events = %d, want an advisory failure per phase", len(hooks)) + } + for _, ev := range hooks { + if ev.Hook.Guardrail.ReasonCode != string(ReviewFailureEvidenceFailure) || ev.Hook.Guardrail.Disposition != "pass_advisory" { + t.Fatalf("guardrail payload = %+v, want advisory evidence failure", ev.Hook.Guardrail) + } + } +} + +func TestYoloBackgroundReviewDrainIsCapped(t *testing.T) { + prevCap, prevGrace := backgroundReviewDrainCap, backgroundReviewDrainGrace + backgroundReviewDrainCap, backgroundReviewDrainGrace = 20*time.Millisecond, time.Second + t.Cleanup(func() { backgroundReviewDrainCap, backgroundReviewDrainGrace = prevCap, prevGrace }) + + reviewer := &yoloReviewer{optIn: true, gate: make(chan struct{}), result: ToolReviewResult{Assessment: ReviewAcceptable}} + start := time.Now() + events, executions, _ := runYoloReview(t, reviewer, nil, nil) + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("run took %s, want the drain cap to bound it", elapsed) + } + if executions != 1 { + t.Fatalf("executions = %d, want 1", executions) + } + if reviews, cancelled := reviewer.counts(); reviews != 2 || cancelled != 2 { + t.Fatalf("reviews=%d cancelled=%d, want both parked reviews cancelled at run end", reviews, cancelled) + } + if hooks := guardrailHooks(events); len(hooks) != 0 { + t.Fatalf("guardrail events = %+v, want none from cancelled reviews", hooks) + } +} diff --git a/engine/api/agent.txt b/engine/api/agent.txt index 0796c61941..181c843163 100644 --- a/engine/api/agent.txt +++ b/engine/api/agent.txt @@ -19,6 +19,7 @@ ifacemethod func (TokenCounter).Count(text string) int ifacemethod func (TokenCounter).CountMessages(msgs []session.Message) int ifacemethod func (ToolReviewer).Review(context.Context, ToolReviewRequest, ReviewEvidenceSource) (ToolReviewResult, session.AuxiliaryUsage, error) + ifacemethod func (YoloReviewPolicyProvider).GuardrailYoloReviews() bool method func (*Engine).Capabilities() port.ProviderCapabilities method func (*Engine).CompactSession(ctx context.Context, sess *session.Session) (ManualCompactionResult, error) method func (*Engine).ContextWindow() int @@ -359,6 +360,7 @@ type ToolReviewRequest struct{ReviewID string; Job ReviewJob; Event governance.H type ToolReviewResult struct{Assessment ReviewAssessment; Concerns []ReviewConcern; Evidence []ReviewEvidenceUse; Missing []ReviewMissingEvidence} type ToolReviewer interface{Review(context.Context, ToolReviewRequest, ReviewEvidenceSource) (ToolReviewResult, session.AuxiliaryUsage, error)} type UserModelReviewer struct{} +type YoloReviewPolicyProvider interface{GuardrailYoloReviews() bool} var ErrApprovalGrantIneligible error var ErrApprovalIntentMismatch error var ErrApprovalNotPending error diff --git a/internal/adapter/permconfig/guardrails_test.go b/internal/adapter/permconfig/guardrails_test.go index efc1be17b3..123f7ebfa3 100644 --- a/internal/adapter/permconfig/guardrails_test.go +++ b/internal/adapter/permconfig/guardrails_test.go @@ -156,3 +156,25 @@ guardrails: t.Fatalf("CLI guardrails must out-rank user-global; got %+v", g) } } + +func TestOperatorGuardrailsYoloAdvisoryParsed(t *testing.T) { + for _, tc := range []struct { + name string + yaml string + want bool + }{ + {name: "default_off", yaml: "\nguardrails:\n model: \"gpt-5\"\n", want: false}, + {name: "opt_in", yaml: "\nguardrails:\n model: \"gpt-5\"\n yoloAdvisory: true\n", want: true}, + } { + t.Run(tc.name, func(t *testing.T) { + env := envWithExplicit("/etc/mecatl/guardrails.yaml", tc.yaml) + g := newWithEnv(Options{ExplicitFiles: []string{"/etc/mecatl/guardrails.yaml"}}, env).OperatorGuardrails() + if g == nil { + t.Fatal("operator guardrails not parsed") + } + if g.YoloAdvisory != tc.want { + t.Fatalf("YoloAdvisory = %v, want %v", g.YoloAdvisory, tc.want) + } + }) + } +} diff --git a/internal/adapter/permconfig/schema.go b/internal/adapter/permconfig/schema.go index b468406699..074c8c1ff7 100644 --- a/internal/adapter/permconfig/schema.go +++ b/internal/adapter/permconfig/schema.go @@ -2059,6 +2059,12 @@ type GuardrailsSection struct { // guardrail checker (an unsafe verdict denies; a checker error fails closed // to the write-escape Ask). Default false = the un-routed posture table. Escape bool `yaml:"escape"` + // YoloAdvisory opts yolo-mode sessions in to advisory guardrail review. By + // default a yolo-mode session calls no checker. When true, each matching + // review runs in the background: the tool never waits for the verdict, is + // never blocked or held, and a checker failure never fails closed. Findings + // appear as advisory guardrail events. Other modes are unaffected. + YoloAdvisory bool `yaml:"yoloAdvisory"` // Rules is the guardrail rule list. Rules []GuardrailRuleSpec `yaml:"rules"` } @@ -2108,6 +2114,7 @@ func (g *GuardrailsSection) strictFields() map[string]any { "defaultMode": &g.DefaultMode, "taskWindow": &g.TaskWindow, "escape": &g.Escape, + "yoloAdvisory": &g.YoloAdvisory, "rules": &g.Rules, } } diff --git a/internal/app/build.go b/internal/app/build.go index 39c968fcce..d9884d521a 100644 --- a/internal/app/build.go +++ b/internal/app/build.go @@ -842,6 +842,9 @@ type Config struct { // fails closed to the write-escape Ask). Default false = the un-routed // posture table (auto read-allow / write-ask). GuardrailsEscape bool + // GuardrailsYoloAdvisory opts yolo-mode sessions in to background advisory + // review (guardrails.yoloAdvisory). Default false: yolo calls no checker. + GuardrailsYoloAdvisory bool // ModelAliases maps a short alias (e.g. "sonnet"/"opus"/"haiku"/"fast") to a // concrete provider model id. Resolved only here; the domain/agent always @@ -5476,6 +5479,11 @@ func guardrailsPostureLine(cfg Config, model string, src guardrailSource, specs // reflects the demoted specs for explicit rules; the note states WHY it is advisory. if guardrailsDemoted(cfg) { out += " — YOLO: AI safety checks are advisory only." + if cfg.GuardrailsYoloAdvisory { + out += " They run in the background and never block." + } else { + out += " Yolo-mode sessions skip them; set guardrails.yoloAdvisory to run them in the background." + } } return out } diff --git a/internal/app/contextual_build_evidence_test.go b/internal/app/contextual_build_evidence_test.go index bcd1b323e3..035dee0b76 100644 --- a/internal/app/contextual_build_evidence_test.go +++ b/internal/app/contextual_build_evidence_test.go @@ -432,6 +432,9 @@ func TestBuildDoesNotDiscloseEvidenceWithoutReadPermission(t *testing.T) { func TestBuildWiresFiniteScriptEvidenceToContextualReviewer(t *testing.T) { cfg := guardrailE2ECfg(t, false, PostureYolo, "./review-script.sh") + // Yolo skips the checker unless opted in; opt in so the review runs (in the + // background) and must still receive the finite script evidence. + cfg.GuardrailsYoloAdvisory = true script := "#!/bin/sh\n# SCRIPT_EVIDENCE_MARKER\nexit 0\n" + strings.Repeat("# pad line\n", 2_500) + "# SCRIPT_EVIDENCE_TAIL\n" if err := os.WriteFile(cfg.Workspace+"/review-script.sh", []byte(script), 0o700); err != nil { t.Fatal(err) @@ -444,7 +447,7 @@ func TestBuildWiresFiniteScriptEvidenceToContextualReviewer(t *testing.T) { } defer built.Close() // ModeYolo: Shell is auto-allowed (allow-all rules injected by Policy.Evaluate), - // so the guardrails block rule fires and routes the call to the contextual reviewer. + // and guardrails.yoloAdvisory routes the call to the contextual reviewer. // ModeDefault would floor Shell at Ask, hanging the test without a human approver. sess, err := built.Service.CreateSession(context.Background(), session.ModeYolo, session.Limits{MaxTurns: 2}) if err != nil { diff --git a/internal/app/guardrails.go b/internal/app/guardrails.go index 7b3693357d..871b702d5c 100644 --- a/internal/app/guardrails.go +++ b/internal/app/guardrails.go @@ -64,6 +64,10 @@ func foldOperatorGuardrails(cfg Config) Config { if g.Escape { cfg.GuardrailsEscape = true } + // YoloAdvisory: YAML-only (no flag); OR-folded like Escape. + if g.YoloAdvisory { + cfg.GuardrailsYoloAdvisory = true + } // Rules: YAML is the sole source. Map the on-disk specs to app.GuardrailRule. if len(cfg.GuardrailsRules) == 0 && len(g.Rules) > 0 { rules := make([]GuardrailRule, 0, len(g.Rules)) @@ -199,12 +203,17 @@ type guardrailActionReviewer struct { base agent.ToolReviewer rules []modelhook.CompiledRule failClosed bool + yoloAdvisory bool grants *modelhook.WaiverHolder providerID, modelID string ruleOrigin string health *guardrailRouteHealth } +// GuardrailYoloReviews implements agent.YoloReviewPolicyProvider: yolo-mode +// sessions review in the background only when the operator opted in. +func (r *guardrailActionReviewer) GuardrailYoloReviews() bool { return r.yoloAdvisory } + // GuardrailReviewPolicy implements agent.ReviewPolicyProvider so configured // matching and enforcement remain explicit on the primary reviewer. func (r *guardrailActionReviewer) GuardrailReviewPolicy(toolName string, job agent.ReviewJob, operationalFailure bool) (applies, enforce bool) { @@ -339,7 +348,7 @@ func buildGuardrailsActionReviewer(cfg Config, provReg *providerRegistry, provid if routed, ok := base.(interface{ GuardrailCheckerRoute() (string, string) }); ok { providerID, modelID = routed.GuardrailCheckerRoute() } - return &guardrailActionReviewer{base: base, rules: rules, failClosed: guardrailFailClosed(cfg.GuardrailsOnCheckerDown), grants: grants, providerID: providerID, modelID: modelID, ruleOrigin: origin, health: cfg.guardrailHealth} + return &guardrailActionReviewer{base: base, rules: rules, failClosed: guardrailFailClosed(cfg.GuardrailsOnCheckerDown), yoloAdvisory: cfg.GuardrailsYoloAdvisory, grants: grants, providerID: providerID, modelID: modelID, ruleOrigin: origin, health: cfg.guardrailHealth} } func attachGuardrailReviewer(deps *agent.Deps, reviewer agent.ToolReviewer, details agent.ReviewDetailSink) { diff --git a/internal/app/guardrails_approve_once_e2e_test.go b/internal/app/guardrails_approve_once_e2e_test.go index 3edd01597c..e1ba8c1161 100644 --- a/internal/app/guardrails_approve_once_e2e_test.go +++ b/internal/app/guardrails_approve_once_e2e_test.go @@ -418,7 +418,7 @@ func runDenyingTurn(t *testing.T, built *Built, id session.SessionID) modeTurnOu // TestGuardrailE2EExecutedYoloAutoDefaultTransitions runs ONE session through // three completed turns on a yolo server with a rejecting checker, switching // mode between runs so the engine transitions live: yolo allows the mutation -// with the checker advisory; auto lets the same checker block it while a +// without consulting the checker (guardrails.yoloAdvisory is off); auto lets the same checker block it while a // configured Deny still denies; default asks for it instead of auto-allowing. func TestGuardrailE2EExecutedYoloAutoDefaultTransitions(t *testing.T) { ctx := context.Background() @@ -461,14 +461,14 @@ func TestGuardrailE2EExecutedYoloAutoDefaultTransitions(t *testing.T) { return err == nil } - // yolo: the mutation is auto-allowed and the rejecting checker is advisory. + // yolo: the mutation is auto-allowed and the checker is skipped by default. yolo := runDenyingTurn(t, built, sess.ID) if len(yolo.policyAsks)+len(yolo.guardrailAsks) != 0 || len(yolo.results) != 1 || yolo.results[0].IsError || !exists("yolo_marker") { t.Fatalf("yolo turn: asks %v/%v results %+v marker %t; want the Shell call to run unasked", yolo.policyAsks, yolo.guardrailAsks, yolo.results, exists("yolo_marker")) } - if reviewer.Calls() == 0 { - t.Fatal("yolo turn: the checker was never consulted, so its advisory verdict proves nothing") + if calls := reviewer.Calls(); calls != 0 { + t.Fatalf("yolo turn: the checker was consulted %d times; yolo must skip it without guardrails.yoloAdvisory", calls) } if _, err := built.Service.SetMode(ctx, sess.ID, session.ModeAuto); err != nil { t.Fatalf("SetMode(auto) after a completed yolo run: %v", err) diff --git a/internal/app/guardrails_yolo_test.go b/internal/app/guardrails_yolo_test.go new file mode 100644 index 0000000000..cb474004b3 --- /dev/null +++ b/internal/app/guardrails_yolo_test.go @@ -0,0 +1,75 @@ +package app + +import ( + "context" + "testing" + "time" + + "github.com/stacklok/mecatl/engine/adapter/mockllm" + "github.com/stacklok/mecatl/engine/adapter/permpolicy" + "github.com/stacklok/mecatl/engine/agent" + "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" +) + +// runYoloGuardedGrep drives one guarded Grep through the production reviewer in a +// yolo session and returns the checker's call count and the guardrail events. +func runYoloGuardedGrep(t *testing.T, yoloAdvisory bool) (int, []session.Event) { + t.Helper() + verdict := `{"assessment":"prohibited","concerns":[{"ref":"c1","category":"redirection","rationale":"r","source_ref":""}],"evidence":[],"missing_evidence":[]}` + checker := mockllm.New(mockllm.TextTurn(verdict), mockllm.TextTurn(verdict), mockllm.TextTurn(verdict), mockllm.TextTurn(verdict)) + cfg := Config{ + UseMock: true, + GuardrailsModel: "checker-model", + GuardrailsRules: []GuardrailRule{{Match: "Grep", Phases: []string{"pre", "post"}, Mode: "block"}}, + GuardrailsYoloAdvisory: yoloAdvisory, + } + reviewer := buildGuardrailsActionReviewer(cfg, nil, checker, providerMock, nil) + if reviewer == nil { + t.Fatal("configured guardrail factory returned nil reviewer") + } + catalog := tool.NewCatalog() + catalog.MustRegister(stubTool{name: "Grep"}) + deps := agent.Deps{ + LLM: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("guarded", "Grep", []byte(`{}`))), mockllm.TextTurn("done")), + Catalog: catalog, Model: "parent-model", + Policy: permpolicy.NewPolicy([]governance.Rule{{Effect: governance.Allow}}, nil), + } + attachGuardrailReviewer(&deps, reviewer, nil) + sess := session.New("yolo", session.ModeYolo, + session.EnvironmentRef{Kind: session.EnvKindLocal, ID: "/ws", Revision: "in-tree-v1"}, + session.Limits{}, time.Unix(1, 0)) + var hooks []session.Event + for ev := range agent.NewEngine(deps).Run(context.Background(), sess, memEnvironment("/ws"), agent.RunRequest{Text: "go"}).Events() { + if ev.Hook != nil && ev.Hook.Guardrail != nil { + hooks = append(hooks, ev) + } + if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.IsError { + t.Fatalf("yolo tool result was blocked or withheld: %+v", ev.ToolResult) + } + } + return checker.Calls(), hooks +} + +func TestGuardrailsYoloSkipsCheckerByDefault(t *testing.T) { + calls, hooks := runYoloGuardedGrep(t, false) + if calls != 0 || len(hooks) != 0 { + t.Fatalf("checker calls=%d guardrail events=%d, want none without guardrails.yoloAdvisory", calls, len(hooks)) + } +} + +func TestGuardrailsYoloAdvisoryReviewsInBackground(t *testing.T) { + calls, hooks := runYoloGuardedGrep(t, true) + if calls == 0 { + t.Fatal("guardrails.yoloAdvisory did not reach the checker") + } + if len(hooks) != 2 { + t.Fatalf("guardrail events = %d, want action and inbound advisories", len(hooks)) + } + for _, ev := range hooks { + if ev.Hook.Guardrail.Disposition != "pass_advisory" { + t.Fatalf("disposition = %q, want pass_advisory", ev.Hook.Guardrail.Disposition) + } + } +} diff --git a/internal/app/permission_mode.go b/internal/app/permission_mode.go index 0682443f51..156c37b31f 100644 --- a/internal/app/permission_mode.go +++ b/internal/app/permission_mode.go @@ -210,7 +210,7 @@ func checkerRefusal(cfg Config, checkerConfigured bool) error { // yoloCheckerAdvisoryNote states what a configured checker does NOT do at the // gate-free tier, where demoteForPosture makes it advisory. Shared by the // refusal and the startup line so they cannot disagree. -const yoloCheckerAdvisoryNote = "In yolo-mode sessions a configured checker is observability-only: it has no pre-tool veto, no approve-once human ask, and no fail-closed on checker failure, so a checker outage looks the same as a clean result. Sessions switched to another mode, including auto, keep it enforcing" +const yoloCheckerAdvisoryNote = "In yolo-mode sessions a configured checker never blocks: it does not run unless guardrails.yoloAdvisory is set, and then it reviews in the background with no pre-tool veto, no approve-once human ask, and no fail-closed on checker failure, so a checker outage looks the same as a clean result. Sessions switched to another mode, including auto, keep it enforcing" // headlessTrustRefusal is the headless admission gate. // It runs AFTER resolveTrust, so any legitimate trust source satisfies it, and @@ -273,7 +273,10 @@ func checkerState(cfg Config, configured bool) (state, reason string) { case !compiled: return checkerDisabled, noGuardrailRuleCompiled case cfg.MaxAllowedMode == session.ModeYolo: - return checkerAdvisory, "demoted in yolo-mode sessions; other modes keep it enforcing" + if cfg.GuardrailsYoloAdvisory { + return checkerAdvisory, "background-only in yolo-mode sessions; other modes keep it enforcing" + } + return checkerAdvisory, "skipped in yolo-mode sessions (guardrails.yoloAdvisory is off); other modes keep it enforcing" case !enforcing: return checkerAdvisory, "configured advisory" } diff --git a/internal/configgen/settings.skeleton.yaml b/internal/configgen/settings.skeleton.yaml index eba9a20786..6a9a6a70e7 100644 --- a/internal/configgen/settings.skeleton.yaml +++ b/internal/configgen/settings.skeleton.yaml @@ -88,6 +88,12 @@ # # unsafe verdict denies; a checker error fails closed to the write-escape Ask). # # Default false = the un-routed posture table. # escape: false +# # YoloAdvisory opts yolo-mode sessions in to advisory guardrail review. By default a +# # yolo-mode session calls no checker. When true, each matching review runs in the +# # background: the tool never waits for the verdict, is never blocked or held, and a +# # checker failure never fails closed. Findings appear as advisory guardrail events. +# # Other modes are unaffected. +# yoloAdvisory: false # # Rules is the guardrail rule list. # rules: # - match: "" diff --git a/user-docs/features/security-and-execution/permissions-and-posture.md b/user-docs/features/security-and-execution/permissions-and-posture.md index 73573a1392..e350497877 100644 --- a/user-docs/features/security-and-execution/permissions-and-posture.md +++ b/user-docs/features/security-and-execution/permissions-and-posture.md @@ -49,7 +49,7 @@ Pass `--permission-mode` when you start `mecated`, `mecatui`, `mecak8s`, or |`default`|Ask|Ask|Ask|Optional| |`accept-edits`|Allowed|Ask|Ask|Optional| |`auto`|Allowed|Allowed|Ask; headless subagent requests go to a reviewer model|Required, enforcing| -|`yolo`|Allowed|Allowed|Allowed|Optional, advisory only| +|`yolo`|Allowed|Allowed|Allowed|Skipped unless opted in, then advisory only| Whether Mecatl loads project instructions, rules, and allow rules is the separate [project trust](#project-trust) decision; a permission mode never @@ -201,11 +201,15 @@ flag by default, so the checker configured in settings applies. Each can also name a checker model or pass the kill-switch. See [Guardrails for mecak8s](/operating/mecak8s.md#choose-a-guardrails-checker) and the [`mecatequi` action inputs](/building/ci/mecatequi.md#headless-posture-and-permission-asks). -In a session running in `yolo` mode, a configured checker is advisory only. It -has no pre-tool veto, no approve-once human ask, and no fail-closed behavior when -the checker fails, so a checker outage looks the same as a clean result. The -startup line reports it as `advisory`. Choose `auto` when you want the checker to -block findings. On a server started with `yolo`, a session you switch to `auto` +In a session running in `yolo` mode, a configured checker does not run, so tool +calls never wait for it. To keep its findings as warnings, opt in with +`guardrails.yoloAdvisory: true` in your user-global `settings.yaml`. The checker +then reviews each matching call in the background: the tool runs and its result +reaches the model without waiting, findings appear as advisory guardrail events, +and a checker failure never blocks. When a run ends, Mecatl waits up to about 10 +seconds for reviews still in progress, then cancels them. The startup line +reports the checker as `advisory` and states whether `yolo` skips it. Choose +`auto` when you want the checker to block findings. On a server started with `yolo`, a session you switch to `auto` or a lower mode keeps the checker enforcing. ## Headless roots and project trust diff --git a/user-docs/reference/configuration.md b/user-docs/reference/configuration.md index 598e22fbf3..40ceb1f794 100644 --- a/user-docs/reference/configuration.md +++ b/user-docs/reference/configuration.md @@ -67,6 +67,7 @@ OPERATOR-TIER LLM content-checker (issue #27). Parsed strictly. A project-tier g | `guardrails.defaultMode` | `string` | `(empty)` | DefaultMode sets the enforcement mode for the built-in default rules when no explicit rules are configured: "block" (default) or "advisory". An explicit rules list replaces the defaults entirely (this key is ignored). | | `guardrails.taskWindow` | `int` | `1` | TaskWindow selects the last K explicitly authenticated root prompts (default 1, clamped 1..3). | | `guardrails.escape` | `bool` | `false` | Escape is the guardrail escape knob: when true AND a checker model is configured, an out-of-root FS escape at posture auto routes through the guardrail checker (an unsafe verdict denies; a checker error fails closed to the write-escape Ask). Default false = the un-routed posture table. | +| `guardrails.yoloAdvisory` | `bool` | `false` | YoloAdvisory opts yolo-mode sessions in to advisory guardrail review. By default a yolo-mode session calls no checker. When true, each matching review runs in the background: the tool never waits for the verdict, is never blocked or held, and a checker failure never fails closed. Findings appear as advisory guardrail events. Other modes are unaffected. | | `guardrails.rules` | `[]guardrailrulespec` | `(absent)` | Rules is the guardrail rule list. | | `guardrails.rules[].match` | `string` | `(empty)` | Match is the tool-name matcher (exact / "prefix*" / "*"). | | `guardrails.rules[].phases` | `[]string` | `(absent)` | Phases lists "pre"/"post"; empty = both. | From c260ac99a0f0845759c70e52886fc5a1c9fac718 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 26 Oct 2026 13:15:13 +0200 Subject: [PATCH 2/4] refactor(guardrails): simplify yolo background reviews and fix run-end edges Publish advisory review events through the child registry's sealed, abortable emit instead of holding a private lock across the send, so a stalled consumer can no longer wedge run end. Drain background reviews from the run goroutine's teardown as well as drainChildren, so no review outlives the run on any exit path. Simplify the review set: create its context once per run, drop the sealed flag, cancel in-flight reviews at run end instead of waiting up to 10s, and share one launcher between action and inbound reviews. Revert the yolo guards on the inline terminal-failure path, which yolo no longer reaches. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/governance.md | 2 +- engine/CHANGELOG.md | 4 +- engine/agent/actionreview.go | 9 +- engine/agent/backgroundreview.go | 190 +++++++----------- engine/agent/dispatch.go | 4 +- engine/agent/inboundreview.go | 19 +- engine/agent/loop.go | 2 + engine/agent/yolo_review_test.go | 55 +++-- .../app/contextual_build_evidence_test.go | 9 +- internal/app/guardrails_yolo_test.go | 31 ++- .../permissions-and-posture.md | 4 +- 11 files changed, 173 insertions(+), 156 deletions(-) diff --git a/docs/architecture/governance.md b/docs/architecture/governance.md index d1e56a7798..6f26f6fbbc 100644 --- a/docs/architecture/governance.md +++ b/docs/architecture/governance.md @@ -78,7 +78,7 @@ mode above the ceiling, and `auto` without an enforcing guardrails checker; a se whose default mode is `auto` refuses to start without one. Unchecked autonomy is the explicit `yolo` choice, where a configured checker never blocks: the engine skips it unless the reviewer opts in (`YoloReviewPolicyProvider`), and then reviews off the -dispatch path and joins in-flight reviews, bounded, before the run ends. `admitStoredMode` +dispatch path, cancelling reviews still in flight when the run ends. `admitStoredMode` re-checks a persisted session's mode at run, approval resume, fork, and clear, so a mode admitted under a looser deployment fails with `FailedPrecondition` until the owner switches it; history is untouched. A session parked awaiting an approval in such a mode diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index cf18db5d63..b981180a28 100644 --- a/engine/CHANGELOG.md +++ b/engine/CHANGELOG.md @@ -18,8 +18,8 @@ The covered surface is the eight core packages (`session`, `governance`, `learni has its applicable action and inbound reviews in yolo-mode sessions run in the background: the tool call neither waits for the verdict nor fails closed, and the verdict is emitted as an advisory guardrail `EvHook` after the tool result - and before the run's terminal event. Run end waits up to 10s (plus a 1s grace) - for in-flight reviews, then cancels them. Added (minor). + and before the run's terminal event. Run end cancels reviews still in flight + and drops their verdicts. Added (minor). - **Provider stop detail** — adds `port.Chunk.StopDetail`, the provider's own terminal label (for example `max_tokens`, `refusal`, `length`) that an adapter diff --git a/engine/agent/actionreview.go b/engine/agent/actionreview.go index 09f9fbfa65..54f67a4f07 100644 --- a/engine/agent/actionreview.go +++ b/engine/agent/actionreview.go @@ -818,8 +818,11 @@ func (e *Engine) resolveActionAssessment(ctx context.Context, r *Run, sess *sess result.Assessment = ReviewUnresolved } _, enforce := reviewPolicy(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction, assessment.err != nil) - if assessment.err != nil && terminalReviewEnforces(sess.Mode, assessment.err) { - enforce = true + if assessment.err != nil { + var terminal GuardrailReviewTerminalFailure + if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { + enforce = true + } } r.reviewRoot.record(reviewFact(call, assessment.action.request.Target, decision)) r.publishActionDetail(ctx, sess.ID, assessment.action.request.ReviewID, assessment.action.request, result, assessment.err) @@ -941,7 +944,7 @@ func (e *Engine) reviewActionWithTail(ctx context.Context, r *Run, sess *session e.publishToolResult(r, session.Event{Type: session.EvToolResult, Turn: turnIdx, ToolResult: ptr(res)}) return res, nil, false, false } - e.startBackgroundActionReview(ctx, r, sess, env, turnIdx, call) + e.startBackgroundActionReview(r, sess, env, turnIdx, call) return session.ToolResult{}, nil, false, true } assessment := e.prepareActionAssessment(ctx, r, sess, env, call) diff --git a/engine/agent/backgroundreview.go b/engine/agent/backgroundreview.go index b616f05c06..3fcbc92c13 100644 --- a/engine/agent/backgroundreview.go +++ b/engine/agent/backgroundreview.go @@ -11,94 +11,57 @@ import ( "github.com/stacklok/mecatl/engine/tool" ) -// backgroundReviewDrainCap bounds how long run end waits for in-flight yolo -// advisory reviews before cancelling them, and backgroundReviewDrainGrace how -// long it then waits for cancelled reviews to unwind. Vars only as TEST seams, -// like childDrainCap. -var ( - backgroundReviewDrainCap = 10 * time.Second - backgroundReviewDrainGrace = 1 * time.Second -) +// backgroundReviewDrainGrace bounds how long run end waits for cancelled yolo +// advisory reviews to unwind. A var only as a TEST seam, like childDrainCap. +var backgroundReviewDrainGrace = 1 * time.Second -// backgroundReviews is a run's set of opted-in yolo advisory reviews. Reviews -// start on the dispatching goroutine after their request is snapshotted, run -// the checker off the dispatch path, and publish only before drain seals the set, -// so no advisory event follows the run's terminal event. +// backgroundReviews is a run's set of opted-in yolo advisory reviews. Each +// review snapshots its request on the dispatching goroutine, then runs the +// checker off the dispatch path on a context detached from the run's own +// cancellation. Run end cancels them; their advisory events go out through the +// child registry's sealed emit, so none follows the run's terminal event. type backgroundReviews struct { mu sync.Mutex wg sync.WaitGroup - ctx context.Context + ctx context.Context //nolint:containedctx // run-scoped review lifetime, cancelled at run end cancel context.CancelFunc closed bool - sealed bool } -// context returns the set's review context, detached from the run's -// cancellation so a review outlives the tool call it observes until drain. -func (b *backgroundReviews) context(parent context.Context) (context.Context, bool) { - b.mu.Lock() - defer b.mu.Unlock() - if b.closed { - return nil, false - } - if b.ctx == nil { - b.ctx, b.cancel = context.WithCancel(context.WithoutCancel(parent)) - } - return b.ctx, true +func (b *backgroundReviews) init(parent context.Context) { + b.ctx, b.cancel = context.WithCancel(context.WithoutCancel(parent)) } -func (b *backgroundReviews) start(fn func()) bool { +func (b *backgroundReviews) start(review func()) bool { b.mu.Lock() defer b.mu.Unlock() - if b.closed { + if b.closed || b.ctx == nil { return false } b.wg.Add(1) go func() { defer b.wg.Done() - fn() + review() }() return true } -// publish runs fn unless drain has sealed the set or cancelled its context. -func (b *backgroundReviews) publish(fn func()) { - b.mu.Lock() - defer b.mu.Unlock() - if b.sealed || b.ctx.Err() != nil { - return - } - fn() -} - -// drain stops new reviews, waits up to the cap for in-flight ones, cancels the -// rest, and seals publication. It reports whether any review failed to unwind. +// drain stops new reviews, cancels in-flight ones, and waits up to the grace for +// them to unwind. It is idempotent and reports whether every review unwound. func (b *backgroundReviews) drain() bool { b.mu.Lock() b.closed = true - cancel := b.cancel b.mu.Unlock() - abandoned := false - if cancel != nil { - if !waitGroupWithin(&b.wg, backgroundReviewDrainCap) { - cancel() - abandoned = !waitGroupWithin(&b.wg, backgroundReviewDrainGrace) - } - cancel() + if b.cancel == nil { + return true } - b.mu.Lock() - b.sealed = true - b.mu.Unlock() - return abandoned -} - -func waitGroupWithin(wg *sync.WaitGroup, d time.Duration) bool { + b.cancel() done := make(chan struct{}) go func() { - wg.Wait() + b.wg.Wait() close(done) }() - timer := time.NewTimer(d) + timer := time.NewTimer(backgroundReviewDrainGrace) defer timer.Stop() select { case <-done: @@ -109,9 +72,9 @@ func waitGroupWithin(wg *sync.WaitGroup, d time.Duration) bool { } func (r *Run) drainBackgroundReviews(ctx context.Context) { - if r.backgroundReviews.drain() { - r.diag.Log(ctx, port.LevelWarn, "abandoning yolo advisory guardrail reviews that did not stop within the run-end drain cap", - "cap", (backgroundReviewDrainCap + backgroundReviewDrainGrace).String()) + if !r.backgroundReviews.drain() { + r.diag.Log(ctx, port.LevelWarn, "abandoning yolo advisory guardrail reviews that did not stop within the run-end grace", + "grace", backgroundReviewDrainGrace.String()) } } @@ -122,73 +85,70 @@ func advisoryDisposition(result ToolReviewResult, err error, clean string) strin return clean } +// launchBackgroundReview runs assess off the dispatch path, records its checker +// usage, and publishes the advisory event built by publish unless run end +// cancelled the review first. release always runs once. +func (r *Run) launchBackgroundReview(assess func() session.AuxiliaryUsage, publish func() session.Event, release func()) { + ctx := r.backgroundReviews.ctx + if !r.backgroundReviews.start(func() { + defer release() + usage := assess() + r.enqueueAuxiliaryUsageWhileActive(ctx, RemapAuxiliaryUsage(ctx, r.diag, session.UsageKindGuardrail, usage)) + if ctx.Err() == nil { + r.children.safeEmit(publish()) + } + }) { + release() + } +} + // startBackgroundActionReview reviews an opted-in yolo action without delaying // it. The call has already been authorized and proceeds regardless of verdict. -func (e *Engine) startBackgroundActionReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall) { - if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { - return - } - bg, ok := r.backgroundReviews.context(ctx) - if !ok { +func (e *Engine) startBackgroundActionReview(r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall) { + if r.reviewRoot == nil || r.backgroundReviews.ctx == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { return } - assessment := e.prepareActionAssessment(bg, r, sess, env, call) - sessID := sess.ID - started := r.backgroundReviews.start(func() { - if assessment.action.close != nil { - defer assessment.action.close() + bg, sessID := r.backgroundReviews.ctx, sess.ID + a := e.prepareActionAssessment(bg, r, sess, env, call) + r.launchBackgroundReview(func() session.AuxiliaryUsage { + assessAction(bg, r, &a) + return a.usage + }, func() session.Event { + result, decision := a.result, string(a.result.Assessment) + if a.err != nil { + result.Assessment, decision = ReviewUnresolved, "unresolved" } - assessAction(bg, r, &assessment) - r.enqueueAuxiliaryUsageWhileActive(bg, RemapAuxiliaryUsage(bg, r.diag, session.UsageKindGuardrail, assessment.usage)) - result := assessment.result - decision := string(result.Assessment) - if assessment.err != nil { - decision = "unresolved" - result.Assessment = ReviewUnresolved + r.reviewRoot.record(reviewFact(call, a.action.request.Target, decision)) + r.publishActionDetail(bg, sessID, a.action.request.ReviewID, a.action.request, result, a.err) + guardrail := reviewMachinePayload(r.reviewRoot.reviewer, a.action.request, result, a.err, advisoryDisposition(result, a.err, "execute")) + return session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePreToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: guardrail}} + }, func() { + a.cancel() + if a.action.close != nil { + a.action.close() } - r.backgroundReviews.publish(func() { - r.reviewRoot.record(reviewFact(call, assessment.action.request.Target, decision)) - r.publishActionDetail(bg, sessID, assessment.action.request.ReviewID, assessment.action.request, result, assessment.err) - disposition := advisoryDisposition(result, assessment.err, "execute") - e.emit(r, session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePreToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: reviewMachinePayload(r.reviewRoot.reviewer, assessment.action.request, result, assessment.err, disposition)}}) - }) }) - if !started { - assessment.cancel() - if assessment.action.close != nil { - assessment.action.close() - } - } } // startBackgroundInboundReview inspects an opted-in yolo result that has // already been released to the model; the verdict is advisory only. -func (e *Engine) startBackgroundInboundReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall, result session.ToolResult) { - if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { +func (e *Engine) startBackgroundInboundReview(r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall, result session.ToolResult) { + if r.reviewRoot == nil || r.backgroundReviews.ctx == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { return } - bg, ok := r.backgroundReviews.context(ctx) - if !ok { - return - } - assessment := inboundAssessment{applies: true, mode: sess.Mode} - e.prepareInboundRequest(bg, r, sess, env, call, result, &assessment) - sessID := sess.ID - started := r.backgroundReviews.start(func() { - if assessment.close != nil { - defer assessment.close() + bg, sessID := r.backgroundReviews.ctx, sess.ID + a := inboundAssessment{applies: true, mode: sess.Mode} + e.prepareInboundRequest(bg, r, sess, env, call, result, &a) + r.launchBackgroundReview(func() session.AuxiliaryUsage { + assessInbound(bg, r, &a) + return a.usage + }, func() session.Event { + r.publishInboundDetail(bg, sessID, a) + return inboundReviewEvent(r, turnIdx, call, a, advisoryDisposition(a.result, a.err, "release_result")) + }, func() { + a.cancel() + if a.close != nil { + a.close() } - assessInbound(bg, r, &assessment) - r.enqueueAuxiliaryUsageWhileActive(bg, RemapAuxiliaryUsage(bg, r.diag, session.UsageKindGuardrail, assessment.usage)) - r.backgroundReviews.publish(func() { - r.publishInboundDetail(bg, sessID, assessment) - e.emitInboundReview(r, turnIdx, call, assessment, advisoryDisposition(assessment.result, assessment.err, "release_result")) - }) }) - if !started { - assessment.cancel() - if assessment.close != nil { - assessment.close() - } - } } diff --git a/engine/agent/dispatch.go b/engine/agent/dispatch.go index 54e888b24c..d21b231bcc 100644 --- a/engine/agent/dispatch.go +++ b/engine/agent/dispatch.go @@ -441,7 +441,7 @@ func (e *Engine) fanOutReadBatchActionAssessments(ctx context.Context, r *Run, s assessAction(ctx, r, a) }(p.assessment) } else { - e.startBackgroundActionReview(ctx, r, sess, env, turnIdx, p.call) + e.startBackgroundActionReview(r, sess, env, turnIdx, p.call) } } reviewWG.Wait() @@ -1757,7 +1757,7 @@ func (e *Engine) executePrivate(ctx context.Context, r *Run, sess *session.Sessi assessment = e.prepareInboundAssessment(ctx, r, sess, env, c, res) assessInbound(ctx, r, &assessment) if !assessment.applies { - e.startBackgroundInboundReview(ctx, r, sess, env, turnIdx, c, res) + e.startBackgroundInboundReview(r, sess, env, turnIdx, c, res) } } return executionRecord{ diff --git a/engine/agent/inboundreview.go b/engine/agent/inboundreview.go index e529ba0482..8000fb7784 100644 --- a/engine/agent/inboundreview.go +++ b/engine/agent/inboundreview.go @@ -70,13 +70,6 @@ func configuredReviewPolicy(reviewer ToolReviewer, toolName string, job ReviewJo return applies, enforce } -// terminalReviewEnforces reports whether err is an authority-relevant terminal -// failure that must fail closed. Yolo never fails closed on checker problems. -func terminalReviewEnforces(mode session.PermissionMode, err error) bool { - var terminal GuardrailReviewTerminalFailure - return mode != session.ModeYolo && errors.As(err, &terminal) && terminal.GuardrailReviewTerminalFailure() -} - // yoloBackgroundReview reports whether a yolo session runs this review as // advisory background work: only when the reviewer opts in and a rule applies. func yoloBackgroundReview(reviewer ToolReviewer, mode session.PermissionMode, toolName string, job ReviewJob) bool { @@ -157,7 +150,8 @@ func assessInbound(parent context.Context, r *Run, assessment *inboundAssessment assessment.result.Assessment = ReviewUnresolved recordReviewFailure(r.reviewRoot.reviewer, assessment.result, assessment.err) _, assessment.enforce = reviewPolicy(r.reviewRoot.reviewer, assessment.mode, assessment.request.EffectiveCall.Name, ReviewJobInbound, true) - if terminalReviewEnforces(assessment.mode, assessment.err) { + var terminal GuardrailReviewTerminalFailure + if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { assessment.enforce = true } return @@ -177,7 +171,8 @@ func assessInbound(parent context.Context, r *Run, assessment *inboundAssessment } if assessment.err != nil { _, assessment.enforce = reviewPolicy(r.reviewRoot.reviewer, assessment.mode, assessment.request.EffectiveCall.Name, ReviewJobInbound, true) - if terminalReviewEnforces(assessment.mode, assessment.err) { + var terminal GuardrailReviewTerminalFailure + if errors.As(assessment.err, &terminal) && terminal.GuardrailReviewTerminalFailure() { assessment.enforce = true } } @@ -298,8 +293,12 @@ func (e *Engine) emitInboundReview(r *Run, turnIdx int, call session.ToolCall, a if !assessment.applies { return } + e.emit(r, inboundReviewEvent(r, turnIdx, call, assessment, disposition)) +} + +func inboundReviewEvent(r *Run, turnIdx int, call session.ToolCall, assessment inboundAssessment, disposition string) session.Event { machine := reviewMachinePayload(r.reviewRoot.reviewer, assessment.request, assessment.result, assessment.err, disposition) - e.emit(r, session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePostToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: machine}}) + return session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePostToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: machine}} } // resolveInbound returns whether the effective tool result was released, distinct diff --git a/engine/agent/loop.go b/engine/agent/loop.go index 6a3b221b76..aef4dbde9c 100644 --- a/engine/agent/loop.go +++ b/engine/agent/loop.go @@ -1771,6 +1771,7 @@ func (e *Engine) prepareRun(ctx context.Context, sess *session.Session, req RunR // through registry.safeEmit over this binding (A4c); a delivered event mirrors // to the sink like every loop emit. r.children = newChildRunRegistry() + r.backgroundReviews.init(ctx) r.children.liveness = e.deps.SessionLiveness r.children.emit = func(ev session.Event) { if sequenced, delivered := r.emitOrAbortSequenced(ev, r.children.emitAbort); delivered { @@ -1801,6 +1802,7 @@ func (e *Engine) prepareRun(ctx context.Context, sess *session.Session, req RunR // drain+seal already; this defer covers them idempotently.) defer close(r.events) defer r.children.seal() + defer r.backgroundReviews.drain() defer cancel() defer r.closeAuxiliaryUsageOwnership() body(ctx, r) diff --git a/engine/agent/yolo_review_test.go b/engine/agent/yolo_review_test.go index db8574d410..3907c1e73c 100644 --- a/engine/agent/yolo_review_test.go +++ b/engine/agent/yolo_review_test.go @@ -3,16 +3,37 @@ package agent import ( "context" "errors" + "iter" "sync" "testing" "time" "github.com/stacklok/mecatl/engine/adapter/mockllm" "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/port" "github.com/stacklok/mecatl/engine/session" "github.com/stacklok/mecatl/engine/tool" ) +// heldFinalTurn delays the model's second call until release closes (or a +// safety timeout), so background reviews can publish before run end cancels them. +type heldFinalTurn struct { + port.LLMProvider + release chan struct{} + calls int +} + +func (h *heldFinalTurn) Stream(ctx context.Context, req port.LLMRequest) (iter.Seq2[port.Chunk, error], error) { + h.calls++ + if h.calls == 2 { + select { + case <-h.release: + case <-time.After(5 * time.Second): + } + } + return h.LLMProvider.Stream(ctx, req) +} + // yoloReviewer reviews both jobs. optIn controls GuardrailYoloReviews; gate, when // set, parks every Review until it is closed or the review context ends. type yoloReviewer struct { @@ -55,13 +76,19 @@ func (r *yoloReviewer) counts() (int, int) { return r.reviews, r.cancelled } -func runYoloReview(t *testing.T, reviewer *yoloReviewer, preparer ReviewEvidencePreparer, onEvent func(session.Event)) ([]session.Event, int, *session.Session) { +// runYoloReview drives one Act call in a yolo session. When hooks > 0 the final +// model turn waits until that many guardrail events have been delivered. +func runYoloReview(t *testing.T, reviewer *yoloReviewer, preparer ReviewEvidencePreparer, hooks int, onEvent func(session.Event)) ([]session.Event, int, *session.Session) { t.Helper() executions := 0 catalog := tool.NewCatalog() catalog.MustRegister(revisionActionTool{executions: &executions}) + llm := &heldFinalTurn{LLMProvider: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("call", "Act", []byte(`{}`))), mockllm.TextTurn("done")), release: make(chan struct{})} + if hooks == 0 { + close(llm.release) + } engine := NewEngine(Deps{ - LLM: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("call", "Act", []byte(`{}`))), mockllm.TextTurn("done")), + LLM: llm, Catalog: catalog, Policy: terminalFailurePolicy{job: ReviewJobAction}, ToolReviewer: reviewer, @@ -70,8 +97,14 @@ func runYoloReview(t *testing.T, reviewer *yoloReviewer, preparer ReviewEvidence env := memEnv("/ws") sess := session.New("yolo-review", session.ModeYolo, env.Ref(), session.Limits{}, time.Unix(0, 0)) var events []session.Event + seen := 0 for ev := range engine.Run(context.Background(), sess, env, RunRequest{Text: "act"}).Events() { events = append(events, ev) + if ev.Hook != nil && ev.Hook.Guardrail != nil { + if seen++; seen == hooks { + close(llm.release) + } + } if onEvent != nil { onEvent(ev) } @@ -91,7 +124,7 @@ func guardrailHooks(events []session.Event) []session.Event { func TestYoloSkipsGuardrailReviewWithoutOptIn(t *testing.T) { reviewer := &yoloReviewer{result: ToolReviewResult{Assessment: ReviewProhibited}} - events, executions, _ := runYoloReview(t, reviewer, nil, nil) + events, executions, _ := runYoloReview(t, reviewer, nil, 0, nil) if reviews, _ := reviewer.counts(); reviews != 0 { t.Fatalf("reviews = %d, want 0: yolo without opt-in must not call the checker", reviews) } @@ -111,7 +144,7 @@ func TestYoloOptInReviewsRunInBackgroundWithoutDelayingTheTool(t *testing.T) { // late turns that regression into the ordering failure asserted below. safety := time.AfterFunc(5*time.Second, func() { once.Do(func() { close(gate) }) }) defer safety.Stop() - events, executions, sess := runYoloReview(t, reviewer, nil, func(ev session.Event) { + events, executions, sess := runYoloReview(t, reviewer, nil, 2, func(ev session.Event) { // The checker is still parked when the canonical result arrives; only then // is it released, so a blocking review would deadlock this run. if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.CallID == "call" { @@ -159,7 +192,7 @@ func TestYoloOptInReviewsRunInBackgroundWithoutDelayingTheTool(t *testing.T) { func TestYoloOptInTerminalEvidenceFailureNeverFailsClosed(t *testing.T) { reviewer := &yoloReviewer{optIn: true, result: ToolReviewResult{Assessment: ReviewAcceptable}} - events, executions, _ := runYoloReview(t, reviewer, failingEvidencePreparer{err: errors.New("evidence unavailable")}, nil) + events, executions, _ := runYoloReview(t, reviewer, failingEvidencePreparer{err: errors.New("evidence unavailable")}, 2, nil) if executions != 1 { t.Fatalf("executions = %d, want 1: yolo must not fail closed", executions) } @@ -182,16 +215,12 @@ func TestYoloOptInTerminalEvidenceFailureNeverFailsClosed(t *testing.T) { } } -func TestYoloBackgroundReviewDrainIsCapped(t *testing.T) { - prevCap, prevGrace := backgroundReviewDrainCap, backgroundReviewDrainGrace - backgroundReviewDrainCap, backgroundReviewDrainGrace = 20*time.Millisecond, time.Second - t.Cleanup(func() { backgroundReviewDrainCap, backgroundReviewDrainGrace = prevCap, prevGrace }) - +func TestYoloBackgroundReviewsAreCancelledAtRunEnd(t *testing.T) { reviewer := &yoloReviewer{optIn: true, gate: make(chan struct{}), result: ToolReviewResult{Assessment: ReviewAcceptable}} start := time.Now() - events, executions, _ := runYoloReview(t, reviewer, nil, nil) - if elapsed := time.Since(start); elapsed > 5*time.Second { - t.Fatalf("run took %s, want the drain cap to bound it", elapsed) + events, executions, _ := runYoloReview(t, reviewer, nil, 0, nil) + if elapsed := time.Since(start); elapsed > backgroundReviewDrainGrace { + t.Fatalf("run took %s, want run end to cancel parked reviews at once", elapsed) } if executions != 1 { t.Fatalf("executions = %d, want 1", executions) diff --git a/internal/app/contextual_build_evidence_test.go b/internal/app/contextual_build_evidence_test.go index 035dee0b76..69ed0205a9 100644 --- a/internal/app/contextual_build_evidence_test.go +++ b/internal/app/contextual_build_evidence_test.go @@ -432,9 +432,6 @@ func TestBuildDoesNotDiscloseEvidenceWithoutReadPermission(t *testing.T) { func TestBuildWiresFiniteScriptEvidenceToContextualReviewer(t *testing.T) { cfg := guardrailE2ECfg(t, false, PostureYolo, "./review-script.sh") - // Yolo skips the checker unless opted in; opt in so the review runs (in the - // background) and must still receive the finite script evidence. - cfg.GuardrailsYoloAdvisory = true script := "#!/bin/sh\n# SCRIPT_EVIDENCE_MARKER\nexit 0\n" + strings.Repeat("# pad line\n", 2_500) + "# SCRIPT_EVIDENCE_TAIL\n" if err := os.WriteFile(cfg.Workspace+"/review-script.sh", []byte(script), 0o700); err != nil { t.Fatal(err) @@ -446,10 +443,10 @@ func TestBuildWiresFiniteScriptEvidenceToContextualReviewer(t *testing.T) { t.Fatalf("Build: %v", err) } defer built.Close() - // ModeYolo: Shell is auto-allowed (allow-all rules injected by Policy.Evaluate), - // and guardrails.yoloAdvisory routes the call to the contextual reviewer. + // ModeAuto: Shell is auto-allowed and the enforcing guardrails block rule routes + // the call to the contextual reviewer inline (yolo would skip the checker). // ModeDefault would floor Shell at Ask, hanging the test without a human approver. - sess, err := built.Service.CreateSession(context.Background(), session.ModeYolo, session.Limits{MaxTurns: 2}) + sess, err := built.Service.CreateSession(context.Background(), session.ModeAuto, session.Limits{MaxTurns: 2}) if err != nil { t.Fatalf("CreateSession: %v", err) } diff --git a/internal/app/guardrails_yolo_test.go b/internal/app/guardrails_yolo_test.go index cb474004b3..d56ddac7d6 100644 --- a/internal/app/guardrails_yolo_test.go +++ b/internal/app/guardrails_yolo_test.go @@ -2,6 +2,7 @@ package app import ( "context" + "iter" "testing" "time" @@ -9,10 +10,30 @@ import ( "github.com/stacklok/mecatl/engine/adapter/permpolicy" "github.com/stacklok/mecatl/engine/agent" "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/port" "github.com/stacklok/mecatl/engine/session" "github.com/stacklok/mecatl/engine/tool" ) +// heldFinalTurn delays the parent's second model call until release closes (or +// a safety timeout), so background reviews publish before run end cancels them. +type heldFinalTurn struct { + port.LLMProvider + release chan struct{} + calls int +} + +func (h *heldFinalTurn) Stream(ctx context.Context, req port.LLMRequest) (iter.Seq2[port.Chunk, error], error) { + h.calls++ + if h.calls == 2 { + select { + case <-h.release: + case <-time.After(5 * time.Second): + } + } + return h.LLMProvider.Stream(ctx, req) +} + // runYoloGuardedGrep drives one guarded Grep through the production reviewer in a // yolo session and returns the checker's call count and the guardrail events. func runYoloGuardedGrep(t *testing.T, yoloAdvisory bool) (int, []session.Event) { @@ -31,8 +52,12 @@ func runYoloGuardedGrep(t *testing.T, yoloAdvisory bool) (int, []session.Event) } catalog := tool.NewCatalog() catalog.MustRegister(stubTool{name: "Grep"}) + parent := &heldFinalTurn{LLMProvider: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("guarded", "Grep", []byte(`{}`))), mockllm.TextTurn("done")), release: make(chan struct{})} + if !yoloAdvisory { + close(parent.release) + } deps := agent.Deps{ - LLM: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("guarded", "Grep", []byte(`{}`))), mockllm.TextTurn("done")), + LLM: parent, Catalog: catalog, Model: "parent-model", Policy: permpolicy.NewPolicy([]governance.Rule{{Effect: governance.Allow}}, nil), } @@ -43,7 +68,9 @@ func runYoloGuardedGrep(t *testing.T, yoloAdvisory bool) (int, []session.Event) var hooks []session.Event for ev := range agent.NewEngine(deps).Run(context.Background(), sess, memEnvironment("/ws"), agent.RunRequest{Text: "go"}).Events() { if ev.Hook != nil && ev.Hook.Guardrail != nil { - hooks = append(hooks, ev) + if hooks = append(hooks, ev); len(hooks) == 2 { + close(parent.release) + } } if ev.Type == session.EvToolResult && ev.ToolResult != nil && ev.ToolResult.IsError { t.Fatalf("yolo tool result was blocked or withheld: %+v", ev.ToolResult) diff --git a/user-docs/features/security-and-execution/permissions-and-posture.md b/user-docs/features/security-and-execution/permissions-and-posture.md index e350497877..02cc31f581 100644 --- a/user-docs/features/security-and-execution/permissions-and-posture.md +++ b/user-docs/features/security-and-execution/permissions-and-posture.md @@ -206,8 +206,8 @@ calls never wait for it. To keep its findings as warnings, opt in with `guardrails.yoloAdvisory: true` in your user-global `settings.yaml`. The checker then reviews each matching call in the background: the tool runs and its result reaches the model without waiting, findings appear as advisory guardrail events, -and a checker failure never blocks. When a run ends, Mecatl waits up to about 10 -seconds for reviews still in progress, then cancels them. The startup line +and a checker failure never blocks. Reviews still in progress when a run ends +are cancelled, so their findings are not reported. The startup line reports the checker as `advisory` and states whether `yolo` skips it. Choose `auto` when you want the checker to block findings. On a server started with `yolo`, a session you switch to `auto` or a lower mode keeps the checker enforcing. From 7acd9a26cfeec602739f2f42f338a1d432968a38 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 26 Oct 2026 13:29:05 +0200 Subject: [PATCH 3/4] fix(guardrails): keep yolo review set allocation-free and de-flake cancel test Create the background review context on first use again: the eager per-run context and run-end drain added 13 allocations to every turn in the allocs gate even when no review runs. Drain now returns without allocating when nothing started. The run-end cancel test could end the run before either review reached the checker; it now lets the run finish only after both reviews park. Co-Authored-By: Claude Opus 5.5 --- engine/agent/backgroundreview.go | 51 +++++++++++++++++++++++--------- engine/agent/loop.go | 1 - engine/agent/yolo_review_test.go | 21 ++++++++++--- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/engine/agent/backgroundreview.go b/engine/agent/backgroundreview.go index 3fcbc92c13..5ccad04a58 100644 --- a/engine/agent/backgroundreview.go +++ b/engine/agent/backgroundreview.go @@ -19,7 +19,9 @@ var backgroundReviewDrainGrace = 1 * time.Second // review snapshots its request on the dispatching goroutine, then runs the // checker off the dispatch path on a context detached from the run's own // cancellation. Run end cancels them; their advisory events go out through the -// child registry's sealed emit, so none follows the run's terminal event. +// child registry's sealed emit, so none follows the run's terminal event. The +// context is created on first use, so a run without background reviews pays +// no allocations for the set. type backgroundReviews struct { mu sync.Mutex wg sync.WaitGroup @@ -28,14 +30,27 @@ type backgroundReviews struct { closed bool } -func (b *backgroundReviews) init(parent context.Context) { - b.ctx, b.cancel = context.WithCancel(context.WithoutCancel(parent)) +// context returns the review context, creating it from parent on first use. +// It reports false once run end has begun draining. +func (b *backgroundReviews) context(parent context.Context) (context.Context, bool) { + b.mu.Lock() + defer b.mu.Unlock() + if b.closed { + return nil, false + } + if b.ctx == nil { + if parent == nil { + parent = context.Background() + } + b.ctx, b.cancel = context.WithCancel(context.WithoutCancel(parent)) + } + return b.ctx, true } func (b *backgroundReviews) start(review func()) bool { b.mu.Lock() defer b.mu.Unlock() - if b.closed || b.ctx == nil { + if b.closed { return false } b.wg.Add(1) @@ -51,11 +66,12 @@ func (b *backgroundReviews) start(review func()) bool { func (b *backgroundReviews) drain() bool { b.mu.Lock() b.closed = true + cancel := b.cancel b.mu.Unlock() - if b.cancel == nil { + if cancel == nil { return true } - b.cancel() + cancel() done := make(chan struct{}) go func() { b.wg.Wait() @@ -88,8 +104,7 @@ func advisoryDisposition(result ToolReviewResult, err error, clean string) strin // launchBackgroundReview runs assess off the dispatch path, records its checker // usage, and publishes the advisory event built by publish unless run end // cancelled the review first. release always runs once. -func (r *Run) launchBackgroundReview(assess func() session.AuxiliaryUsage, publish func() session.Event, release func()) { - ctx := r.backgroundReviews.ctx +func (r *Run) launchBackgroundReview(ctx context.Context, assess func() session.AuxiliaryUsage, publish func() session.Event, release func()) { if !r.backgroundReviews.start(func() { defer release() usage := assess() @@ -105,12 +120,16 @@ func (r *Run) launchBackgroundReview(assess func() session.AuxiliaryUsage, publi // startBackgroundActionReview reviews an opted-in yolo action without delaying // it. The call has already been authorized and proceeds regardless of verdict. func (e *Engine) startBackgroundActionReview(r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall) { - if r.reviewRoot == nil || r.backgroundReviews.ctx == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { + if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { return } - bg, sessID := r.backgroundReviews.ctx, sess.ID + bg, ok := r.backgroundReviews.context(r.ctx) + if !ok { + return + } + sessID := sess.ID a := e.prepareActionAssessment(bg, r, sess, env, call) - r.launchBackgroundReview(func() session.AuxiliaryUsage { + r.launchBackgroundReview(bg, func() session.AuxiliaryUsage { assessAction(bg, r, &a) return a.usage }, func() session.Event { @@ -133,13 +152,17 @@ func (e *Engine) startBackgroundActionReview(r *Run, sess *session.Session, env // startBackgroundInboundReview inspects an opted-in yolo result that has // already been released to the model; the verdict is advisory only. func (e *Engine) startBackgroundInboundReview(r *Run, sess *session.Session, env tool.Environment, turnIdx int, call session.ToolCall, result session.ToolResult) { - if r.reviewRoot == nil || r.backgroundReviews.ctx == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { + if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { + return + } + bg, ok := r.backgroundReviews.context(r.ctx) + if !ok { return } - bg, sessID := r.backgroundReviews.ctx, sess.ID + sessID := sess.ID a := inboundAssessment{applies: true, mode: sess.Mode} e.prepareInboundRequest(bg, r, sess, env, call, result, &a) - r.launchBackgroundReview(func() session.AuxiliaryUsage { + r.launchBackgroundReview(bg, func() session.AuxiliaryUsage { assessInbound(bg, r, &a) return a.usage }, func() session.Event { diff --git a/engine/agent/loop.go b/engine/agent/loop.go index aef4dbde9c..a670abfef2 100644 --- a/engine/agent/loop.go +++ b/engine/agent/loop.go @@ -1771,7 +1771,6 @@ func (e *Engine) prepareRun(ctx context.Context, sess *session.Session, req RunR // through registry.safeEmit over this binding (A4c); a delivered event mirrors // to the sink like every loop emit. r.children = newChildRunRegistry() - r.backgroundReviews.init(ctx) r.children.liveness = e.deps.SessionLiveness r.children.emit = func(ev session.Event) { if sequenced, delivered := r.emitOrAbortSequenced(ev, r.children.emitAbort); delivered { diff --git a/engine/agent/yolo_review_test.go b/engine/agent/yolo_review_test.go index 3907c1e73c..25146f5cdf 100644 --- a/engine/agent/yolo_review_test.go +++ b/engine/agent/yolo_review_test.go @@ -35,10 +35,12 @@ func (h *heldFinalTurn) Stream(ctx context.Context, req port.LLMRequest) (iter.S } // yoloReviewer reviews both jobs. optIn controls GuardrailYoloReviews; gate, when -// set, parks every Review until it is closed or the review context ends. +// set, parks every Review until it is closed or the review context ends, and +// parked (when set) receives once per review that reached the gate. type yoloReviewer struct { optIn bool gate chan struct{} + parked chan struct{} result ToolReviewResult err error @@ -52,6 +54,9 @@ func (r *yoloReviewer) Review(ctx context.Context, _ ToolReviewRequest, _ Review r.reviews++ r.mu.Unlock() if r.gate != nil { + if r.parked != nil { + r.parked <- struct{}{} + } select { case <-r.gate: case <-ctx.Done(): @@ -84,7 +89,15 @@ func runYoloReview(t *testing.T, reviewer *yoloReviewer, preparer ReviewEvidence catalog := tool.NewCatalog() catalog.MustRegister(revisionActionTool{executions: &executions}) llm := &heldFinalTurn{LLMProvider: mockllm.New(mockllm.ToolCallTurn(session.NewToolCall("call", "Act", []byte(`{}`))), mockllm.TextTurn("done")), release: make(chan struct{})} - if hooks == 0 { + switch { + case reviewer.parked != nil: + // Let the run end only once both reviews are parked in the checker. + go func() { + <-reviewer.parked + <-reviewer.parked + close(llm.release) + }() + case hooks == 0: close(llm.release) } engine := NewEngine(Deps{ @@ -216,10 +229,10 @@ func TestYoloOptInTerminalEvidenceFailureNeverFailsClosed(t *testing.T) { } func TestYoloBackgroundReviewsAreCancelledAtRunEnd(t *testing.T) { - reviewer := &yoloReviewer{optIn: true, gate: make(chan struct{}), result: ToolReviewResult{Assessment: ReviewAcceptable}} + reviewer := &yoloReviewer{optIn: true, gate: make(chan struct{}), parked: make(chan struct{}, 2), result: ToolReviewResult{Assessment: ReviewAcceptable}} start := time.Now() events, executions, _ := runYoloReview(t, reviewer, nil, 0, nil) - if elapsed := time.Since(start); elapsed > backgroundReviewDrainGrace { + if elapsed := time.Since(start); elapsed > 4*time.Second { t.Fatalf("run took %s, want run end to cancel parked reviews at once", elapsed) } if executions != 1 { From d1f3b213062bcf043fc8dbb9f42d04d190a1c560 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 26 Oct 2026 18:31:49 +0200 Subject: [PATCH 4/4] fix(guardrails): snapshot evidence, cap and order yolo background reviews A background action review runs while the reviewed call executes, so a lazily read file could already be changed by Edit, Write or Shell and the verdict degrade to unresolved. Background reviews now set ReviewEvidencePreparation.Snapshot and the preparer serves the bounded first read it already performs from memory, reporting larger files as incomplete. Preparation cost is unchanged. Cap concurrent background reviews at four per run; calls over the cap are not reviewed. Stop recording advisory verdicts in the shared review trajectory, so enforcing reviews of delegated children see facts in dispatch order rather than in checker completion order. Co-Authored-By: Claude Opus 5.5 --- engine/CHANGELOG.md | 8 ++- engine/agent/actionreview.go | 16 +++--- engine/agent/backgroundreview.go | 47 ++++++++++++------ engine/agent/dispatch.go | 2 +- engine/agent/inboundreview.go | 3 +- engine/agent/review.go | 6 +++ engine/agent/review_deadline_test.go | 4 +- engine/agent/yolo_review_test.go | 48 ++++++++++++++++++ engine/api/agent.txt | 2 +- .../app/contextual_build_evidence_test.go | 49 +++++++++++++++++++ internal/app/contextual_evidence.go | 10 +++- .../permissions-and-posture.md | 5 +- 12 files changed, 168 insertions(+), 32 deletions(-) diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index b981180a28..29f09cf9cf 100644 --- a/engine/CHANGELOG.md +++ b/engine/CHANGELOG.md @@ -18,8 +18,12 @@ The covered surface is the eight core packages (`session`, `governance`, `learni has its applicable action and inbound reviews in yolo-mode sessions run in the background: the tool call neither waits for the verdict nor fails closed, and the verdict is emitted as an advisory guardrail `EvHook` after the tool result - and before the run's terminal event. Run end cancels reviews still in flight - and drops their verdicts. Added (minor). + and before the run's terminal event. At most four run at once; calls beyond + that are not reviewed. Run end cancels reviews still in flight and drops their + verdicts. Advisory verdicts are not recorded in the review trajectory. Adds + `ReviewEvidencePreparation.Snapshot`, set for these reviews, asking the + preparer to capture workspace evidence at preparation time because the + reviewed call may change it before the checker reads it. Added (minor). - **Provider stop detail** — adds `port.Chunk.StopDetail`, the provider's own terminal label (for example `max_tokens`, `refusal`, `length`) that an adapter diff --git a/engine/agent/actionreview.go b/engine/agent/actionreview.go index 54f67a4f07..bf7b1c2d87 100644 --- a/engine/agent/actionreview.go +++ b/engine/agent/actionreview.go @@ -334,7 +334,7 @@ func (e *Engine) establishReviewPrincipal(r *Run) { r.reviewRoot.establishInstructions(instructions) } -func (e *Engine) prepareActionReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall) actionReview { +func (e *Engine) prepareActionReview(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall, snapshot bool) actionReview { e.establishReviewPrincipal(r) target, paths := reviewTarget(call) facts, complete, principalRevision := r.reviewRoot.principalSnapshotWithRevision() @@ -382,7 +382,7 @@ func (e *Engine) prepareActionReview(ctx context.Context, r *Run, sess *session. var prepared PreparedReviewEvidence var prepareErr error if ctx.Err() == nil { - prepared, prepareErr = e.prepareReviewEvidence(ctx, r, sess, env, req, nil) + prepared, prepareErr = e.prepareReviewEvidence(ctx, r, sess, env, req, nil, snapshot) } else { prepareErr = reviewFailure(ReviewFailureTimeout) } @@ -428,12 +428,12 @@ func (e *Engine) authorizeReviewEvidenceRead(ctx context.Context, r *Run, sess * return nil } -func (e *Engine) prepareReviewEvidence(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, req ToolReviewRequest, result *session.ToolResult) (PreparedReviewEvidence, error) { +func (e *Engine) prepareReviewEvidence(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, req ToolReviewRequest, result *session.ToolResult, snapshot bool) (PreparedReviewEvidence, error) { if r.reviewRoot.preparer == nil { return PreparedReviewEvidence{}, nil } prepared, err := r.reviewRoot.preparer.PrepareReviewEvidence(ctx, ReviewEvidencePreparation{ - Request: req, Environment: env, Owner: sess.Owner.Clone(), Result: result, + Request: req, Environment: env, Owner: sess.Owner.Clone(), Result: result, Snapshot: snapshot, Authorize: func(authCtx context.Context, call session.ToolCall) error { paths := tool.LocalFileOperands(call.Name, call.Args) if call.Name != "Read" || len(paths) != 1 { @@ -662,9 +662,9 @@ type actionReviewAssessment struct { grantHit bool } -func (e *Engine) prepareActionAssessment(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall) actionReviewAssessment { +func (e *Engine) prepareActionAssessment(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, call session.ToolCall, snapshot bool) actionReviewAssessment { reviewCtx, cancel := context.WithTimeoutCause(ctx, reviewAssessmentTimeout, reviewFailure(ReviewFailureTimeout)) - action := e.prepareActionReview(reviewCtx, r, sess, env, call) + action := e.prepareActionReview(reviewCtx, r, sess, env, call, snapshot) assessment := actionReviewAssessment{action: action, ctx: reviewCtx, cancel: cancel} if action.prepareErr == nil && reviewCtx.Err() == nil { if issuer, ok := r.reviewRoot.reviewer.(ReviewGrantStore); ok && action.repeat { @@ -947,7 +947,7 @@ func (e *Engine) reviewActionWithTail(ctx context.Context, r *Run, sess *session e.startBackgroundActionReview(r, sess, env, turnIdx, call) return session.ToolResult{}, nil, false, true } - assessment := e.prepareActionAssessment(ctx, r, sess, env, call) + assessment := e.prepareActionAssessment(ctx, r, sess, env, call, false) assessAction(ctx, r, &assessment) if ctx.Err() != nil { r.recordGuardrailUsageWhileActive(ctx, sess, assessment.usage) @@ -991,7 +991,7 @@ func (e *Engine) armPostActionGrant(ctx context.Context, r *Run, sess *session.S if !repeat || !hasIssuer || park != nil || cancelled || result.IsError { return } - post := e.prepareActionReview(ctx, r, sess, env, call) + post := e.prepareActionReview(ctx, r, sess, env, call, false) if post.close != nil { defer post.close() } diff --git a/engine/agent/backgroundreview.go b/engine/agent/backgroundreview.go index 5ccad04a58..1461250a61 100644 --- a/engine/agent/backgroundreview.go +++ b/engine/agent/backgroundreview.go @@ -15,10 +15,16 @@ import ( // advisory reviews to unwind. A var only as a TEST seam, like childDrainCap. var backgroundReviewDrainGrace = 1 * time.Second +// maxBackgroundReviews caps concurrent advisory checker runs per run, bounding +// provider load, cost, and held evidence on read-heavy runs. +const maxBackgroundReviews = 4 + // backgroundReviews is a run's set of opted-in yolo advisory reviews. Each -// review snapshots its request on the dispatching goroutine, then runs the -// checker off the dispatch path on a context detached from the run's own -// cancellation. Run end cancels them; their advisory events go out through the +// review snapshots its request and evidence on the dispatching goroutine, then +// runs the checker off the dispatch path on a context detached from the run's +// own cancellation. At most maxBackgroundReviews run at once; a review over the +// cap is skipped. Advisory verdicts never feed the shared trajectory, so +// enforcing reviews of delegated children see the same facts in dispatch order. Run end cancels them; their advisory events go out through the // child registry's sealed emit, so none follows the run's terminal event. The // context is created on first use, so a run without background reviews pays // no allocations for the set. @@ -28,16 +34,20 @@ type backgroundReviews struct { ctx context.Context //nolint:containedctx // run-scoped review lifetime, cancelled at run end cancel context.CancelFunc closed bool + active int } -// context returns the review context, creating it from parent on first use. -// It reports false once run end has begun draining. -func (b *backgroundReviews) context(parent context.Context) (context.Context, bool) { +// reserve claims a review slot and returns the review context, creating it +// from parent on first use. It reports false once run end has begun draining +// or when maxBackgroundReviews are already running; a true return must be +// followed by start, which releases the slot. +func (b *backgroundReviews) reserve(parent context.Context) (context.Context, bool) { b.mu.Lock() defer b.mu.Unlock() - if b.closed { + if b.closed || b.active >= maxBackgroundReviews { return nil, false } + b.active++ if b.ctx == nil { if parent == nil { parent = context.Background() @@ -47,20 +57,30 @@ func (b *backgroundReviews) context(parent context.Context) (context.Context, bo return b.ctx, true } +// start runs a reserved review and releases its slot when it ends. It reports +// false, releasing the slot at once, when run end began after the reservation. func (b *backgroundReviews) start(review func()) bool { b.mu.Lock() defer b.mu.Unlock() if b.closed { + b.active-- return false } b.wg.Add(1) go func() { defer b.wg.Done() + defer b.releaseSlot() review() }() return true } +func (b *backgroundReviews) releaseSlot() { + b.mu.Lock() + b.active-- + b.mu.Unlock() +} + // drain stops new reviews, cancels in-flight ones, and waits up to the grace for // them to unwind. It is idempotent and reports whether every review unwound. func (b *backgroundReviews) drain() bool { @@ -123,21 +143,20 @@ func (e *Engine) startBackgroundActionReview(r *Run, sess *session.Session, env if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobAction) { return } - bg, ok := r.backgroundReviews.context(r.ctx) + bg, ok := r.backgroundReviews.reserve(r.ctx) if !ok { return } sessID := sess.ID - a := e.prepareActionAssessment(bg, r, sess, env, call) + a := e.prepareActionAssessment(bg, r, sess, env, call, true) r.launchBackgroundReview(bg, func() session.AuxiliaryUsage { assessAction(bg, r, &a) return a.usage }, func() session.Event { - result, decision := a.result, string(a.result.Assessment) + result := a.result if a.err != nil { - result.Assessment, decision = ReviewUnresolved, "unresolved" + result.Assessment = ReviewUnresolved } - r.reviewRoot.record(reviewFact(call, a.action.request.Target, decision)) r.publishActionDetail(bg, sessID, a.action.request.ReviewID, a.action.request, result, a.err) guardrail := reviewMachinePayload(r.reviewRoot.reviewer, a.action.request, result, a.err, advisoryDisposition(result, a.err, "execute")) return session.Event{Type: session.EvHook, Turn: turnIdx, Hook: &session.HookPayload{Phase: string(governance.PhasePreToolUse), Tool: call.Name, Decision: session.HookInfo, CallID: call.ID, Guardrail: guardrail}} @@ -155,12 +174,12 @@ func (e *Engine) startBackgroundInboundReview(r *Run, sess *session.Session, env if r.reviewRoot == nil || !yoloBackgroundReview(r.reviewRoot.reviewer, sess.Mode, call.Name, ReviewJobInbound) { return } - bg, ok := r.backgroundReviews.context(r.ctx) + bg, ok := r.backgroundReviews.reserve(r.ctx) if !ok { return } sessID := sess.ID - a := inboundAssessment{applies: true, mode: sess.Mode} + a := inboundAssessment{applies: true, mode: sess.Mode, snapshot: true} e.prepareInboundRequest(bg, r, sess, env, call, result, &a) r.launchBackgroundReview(bg, func() session.AuxiliaryUsage { assessInbound(bg, r, &a) diff --git a/engine/agent/dispatch.go b/engine/agent/dispatch.go index d21b231bcc..43f890ade1 100644 --- a/engine/agent/dispatch.go +++ b/engine/agent/dispatch.go @@ -433,7 +433,7 @@ func (e *Engine) fanOutReadBatchActionAssessments(ctx context.Context, r *Run, s continue } if applies, _ := reviewPolicy(r.reviewRoot.reviewer, sess.Mode, p.call.Name, ReviewJobAction, false); applies { - assessment := e.prepareActionAssessment(ctx, r, sess, env, p.call) + assessment := e.prepareActionAssessment(ctx, r, sess, env, p.call, false) p.assessment = &assessment reviewWG.Add(1) go func(a *actionReviewAssessment) { diff --git a/engine/agent/inboundreview.go b/engine/agent/inboundreview.go index 8000fb7784..590e0e5d16 100644 --- a/engine/agent/inboundreview.go +++ b/engine/agent/inboundreview.go @@ -44,6 +44,7 @@ type inboundAssessment struct { enforce bool mode session.PermissionMode principalRevision uint64 + snapshot bool } // reviewPolicy resolves the reviewer's configured applicability and enforcement @@ -123,7 +124,7 @@ func (e *Engine) prepareInboundRequest(ctx context.Context, r *Run, sess *sessio var prepared PreparedReviewEvidence var prepareErr error if reviewCtx.Err() == nil { - prepared, prepareErr = e.prepareReviewEvidence(reviewCtx, r, sess, env, assessment.request, &result) + prepared, prepareErr = e.prepareReviewEvidence(reviewCtx, r, sess, env, assessment.request, &result, assessment.snapshot) } else { prepareErr = reviewFailure(ReviewFailureTimeout) } diff --git a/engine/agent/review.go b/engine/agent/review.go index 43bd52401b..7ffe905469 100644 --- a/engine/agent/review.go +++ b/engine/agent/review.go @@ -135,6 +135,12 @@ type ReviewEvidencePreparation struct { // effective catalog, mode, and bound authority. Preparers must call it before // touching backend metadata or content; model/reviewer input cannot replace it. Authorize func(context.Context, session.ToolCall) error + // Snapshot asks the preparer to capture workspace evidence content now, + // within its bounded first read, instead of reading it lazily during the + // review. The Engine sets it for advisory background reviews, which run + // concurrently with the reviewed call and later calls that may change the + // files. Content beyond the captured read is reported incomplete. + Snapshot bool } // PreparedReviewEvidence owns one review-local finite capability set. Close is diff --git a/engine/agent/review_deadline_test.go b/engine/agent/review_deadline_test.go index fc1fcb01bc..68a1c5ae2c 100644 --- a/engine/agent/review_deadline_test.go +++ b/engine/agent/review_deadline_test.go @@ -124,7 +124,7 @@ func TestActionPreparationFailureRecordedBeforeAssessment(t *testing.T) { r := &Run{reviewRoot: newReviewRoot(reviewer, preparer, nil)} env := memEnv("/ws") sess := session.New("prep-health", session.ModeDefault, env.Ref(), session.Limits{}, time.Now()) - assessment := NewEngine(Deps{}).prepareActionAssessment(ctx, r, sess, env, session.NewToolCall("call", "Act", []byte(`{}`))) + assessment := NewEngine(Deps{}).prepareActionAssessment(ctx, r, sess, env, session.NewToolCall("call", "Act", []byte(`{}`)), false) defer assessment.cancel() wantRecords := 1 if stage == "before_checker" || stage == "parent_cancelled" { @@ -168,7 +168,7 @@ func TestPreparationTerminalContextErrorIsNotDowngraded(t *testing.T) { preparer := &deadlineEvidencePreparer{delay: 91 * time.Second, err: terminalContextEvidenceError{cause}} env := memEnv("/ws") sess := session.New("terminal-context", session.ModeDefault, env.Ref(), session.Limits{}, time.Now()) - _, err := NewEngine(Deps{}).prepareReviewEvidence(ctx, &Run{reviewRoot: newReviewRoot(nil, preparer, nil)}, sess, env, ToolReviewRequest{}, nil) + _, err := NewEngine(Deps{}).prepareReviewEvidence(ctx, &Run{reviewRoot: newReviewRoot(nil, preparer, nil)}, sess, env, ToolReviewRequest{}, nil, false) var terminal GuardrailReviewTerminalFailure if !errors.As(err, &terminal) || !terminal.GuardrailReviewTerminalFailure() { t.Fatalf("terminal error downgraded: %v", err) diff --git a/engine/agent/yolo_review_test.go b/engine/agent/yolo_review_test.go index 25146f5cdf..9a12da94ba 100644 --- a/engine/agent/yolo_review_test.go +++ b/engine/agent/yolo_review_test.go @@ -245,3 +245,51 @@ func TestYoloBackgroundReviewsAreCancelledAtRunEnd(t *testing.T) { t.Fatalf("guardrail events = %+v, want none from cancelled reviews", hooks) } } + +func TestBackgroundReviewsCapConcurrency(t *testing.T) { + var b backgroundReviews + release := make(chan struct{}) + for i := range maxBackgroundReviews { + if _, ok := b.reserve(context.Background()); !ok { + t.Fatalf("reserve %d refused below the cap", i) + } + b.start(func() { <-release }) + } + if _, ok := b.reserve(context.Background()); ok { + t.Fatal("reserve succeeded above maxBackgroundReviews") + } + close(release) + b.wg.Wait() + if _, ok := b.reserve(context.Background()); !ok { + t.Fatal("finished reviews did not release their slots") + } + if b.start(func() {}); !b.drain() { + t.Fatal("drain did not join the last review") + } +} + +type snapshotRecordingPreparer struct { + mu sync.Mutex + snapshots []bool +} + +func (p *snapshotRecordingPreparer) PrepareReviewEvidence(_ context.Context, prep ReviewEvidencePreparation) (PreparedReviewEvidence, error) { + p.mu.Lock() + p.snapshots = append(p.snapshots, prep.Snapshot) + p.mu.Unlock() + return PreparedReviewEvidence{Complete: true}, nil +} + +func TestYoloBackgroundReviewsSnapshotEvidence(t *testing.T) { + reviewer := &yoloReviewer{optIn: true, result: ToolReviewResult{Assessment: ReviewProhibited}} + preparer := &snapshotRecordingPreparer{} + events, _, _ := runYoloReview(t, reviewer, preparer, 2, nil) + if len(guardrailHooks(events)) != 2 { + t.Fatalf("guardrail events = %d, want both advisories", len(guardrailHooks(events))) + } + preparer.mu.Lock() + defer preparer.mu.Unlock() + if len(preparer.snapshots) != 2 || !preparer.snapshots[0] || !preparer.snapshots[1] { + t.Fatalf("snapshot flags = %v, want both background preparations to snapshot", preparer.snapshots) + } +} diff --git a/engine/api/agent.txt b/engine/api/agent.txt index 181c843163..c26db22153 100644 --- a/engine/api/agent.txt +++ b/engine/api/agent.txt @@ -319,7 +319,7 @@ type ReviewDetail struct{RootSessionID session.SessionID; SessionID session.Sess type ReviewDetailSink interface{PublishReviewDetail(context.Context, ReviewDetail)} type ReviewEvidence struct{Handle string; Kind string; Version string; Continuation string; Complete bool; Content string} type ReviewEvidenceMeta struct{Handle string; Kind string; Display string; Version string; Continuation string; Complete bool} -type ReviewEvidencePreparation struct{Request ToolReviewRequest; Environment tool.Environment; Owner *session.Principal; Result *session.ToolResult; Authorize func(context.Context, session.ToolCall) error} +type ReviewEvidencePreparation struct{Request ToolReviewRequest; Environment tool.Environment; Owner *session.Principal; Result *session.ToolResult; Authorize func(context.Context, session.ToolCall) error; Snapshot bool} type ReviewEvidencePreparer interface{PrepareReviewEvidence(context.Context, ReviewEvidencePreparation) (PreparedReviewEvidence, error)} type ReviewEvidenceRequest struct{ReviewID string; Handle string; Version string} type ReviewEvidenceSource interface{ReadReviewEvidence(context.Context, ReviewEvidenceRequest) (ReviewEvidence, error)} diff --git a/internal/app/contextual_build_evidence_test.go b/internal/app/contextual_build_evidence_test.go index 69ed0205a9..6a02026a8a 100644 --- a/internal/app/contextual_build_evidence_test.go +++ b/internal/app/contextual_build_evidence_test.go @@ -724,3 +724,52 @@ func TestBuildContextualEvidenceMatchesBuiltinReadPathDecoding(t *testing.T) { }) } } + +func TestSnapshotWorkspaceEvidenceSurvivesLaterChange(t *testing.T) { + ctx := context.Background() + workspace := memfs.NewWorkspace("/review") + if err := workspace.Write(ctx, "review-script.sh", []byte("SCRIPT_EVIDENCE_MARKER")); err != nil { + t.Fatal(err) + } + if err := workspace.Write(ctx, "big.sh", []byte(strings.Repeat("line\n", 8_000))); err != nil { + t.Fatal(err) + } + ref := session.EnvironmentRef{Kind: session.EnvKindMem, ID: "review", Revision: "r1"} + env := tool.MustEnvironment(ref, workspace, reviewerReadLedger{}, nil) + prepare := func(script string) agent.PreparedReviewEvidence { + t.Helper() + req := agent.ToolReviewRequest{ + ReviewID: "review-" + script, Job: agent.ReviewJobAction, + Event: sessionHookEvent("review-" + script), + EffectiveCall: session.NewToolCall("call-1", "Shell", json.RawMessage(`{"command":"./`+script+`"}`)), + Caller: agent.ReviewCaller{Role: "main", Capabilities: []string{"Shell"}}, Environment: ref, + Capacity: agent.ReviewCapacity{MaxEvidenceHandles: maxReviewEvidenceHandles, MaxEvidenceBytes: maxReviewEvidenceBytes}, + } + prepared, err := (&guardrailActionReviewer{providerID: "provider", modelID: "model"}).PrepareReviewEvidence(ctx, agent.ReviewEvidencePreparation{ + Request: req, Environment: env, Snapshot: true, Authorize: func(context.Context, session.ToolCall) error { return nil }, + }) + if err != nil || len(prepared.Evidence) == 0 || prepared.Source == nil { + t.Fatalf("prepare %s = %+v, err=%v", script, prepared, err) + } + t.Cleanup(prepared.Close) + return prepared + } + + small := prepare("review-script.sh") + if !small.Complete { + t.Fatal("a file within the first read must be fully captured") + } + // The reviewed call (or a later one) changes the file before the checker reads it. + if err := workspace.Write(ctx, "review-script.sh", []byte("changed")); err != nil { + t.Fatal(err) + } + meta := small.Evidence[0] + evidence, err := small.Source.ReadReviewEvidence(ctx, agent.ReviewEvidenceRequest{ReviewID: "review-review-script.sh", Handle: meta.Handle, Version: meta.Version}) + if err != nil || evidence.Content != "SCRIPT_EVIDENCE_MARKER" { + t.Fatalf("snapshot evidence=%+v err=%v, want the content captured at preparation", evidence, err) + } + + if big := prepare("big.sh"); big.Complete { + t.Fatal("a file beyond the first read must be reported incomplete under snapshot") + } +} diff --git a/internal/app/contextual_evidence.go b/internal/app/contextual_evidence.go index 21764dbef6..e942e9bee7 100644 --- a/internal/app/contextual_evidence.go +++ b/internal/app/contextual_evidence.go @@ -128,7 +128,15 @@ func (r *guardrailActionReviewer) PrepareReviewEvidence(ctx context.Context, pre complete = false continue } - _ = content // The bounded first page establishes version and total size. + if prep.Snapshot { + // A background review may run after the reviewed call changed the + // file, so serve the bounded first read instead of the live file. + captured := int64(len(content)) >= total + complete = complete && captured + candidates = append(candidates, reviewEvidenceCandidate{Kind: "text_file", Display: path, Version: encoded, Complete: captured, Authorized: true, Binding: binding, Backend: memoryReviewEvidenceBackend{content: string(content)}}) + continue + } + // The bounded first page establishes version and total size. candidates = append(candidates, reviewEvidenceCandidate{Kind: "text_file", Display: path, Version: encoded, Complete: true, Authorized: true, Binding: binding, Backend: workspaceReviewEvidenceBackend{reader: reader, path: path, size: total, version: encoded, authorize: authorize}}) } } diff --git a/user-docs/features/security-and-execution/permissions-and-posture.md b/user-docs/features/security-and-execution/permissions-and-posture.md index 02cc31f581..6e0ed45ffb 100644 --- a/user-docs/features/security-and-execution/permissions-and-posture.md +++ b/user-docs/features/security-and-execution/permissions-and-posture.md @@ -206,8 +206,9 @@ calls never wait for it. To keep its findings as warnings, opt in with `guardrails.yoloAdvisory: true` in your user-global `settings.yaml`. The checker then reviews each matching call in the background: the tool runs and its result reaches the model without waiting, findings appear as advisory guardrail events, -and a checker failure never blocks. Reviews still in progress when a run ends -are cancelled, so their findings are not reported. The startup line +and a checker failure never blocks. At most four reviews run at once, and calls +beyond that are not reviewed. Reviews still in progress when a run ends are +cancelled, so their findings are not reported. The startup line reports the checker as `advisory` and states whether `yolo` skips it. Choose `auto` when you want the checker to block findings. On a server started with `yolo`, a session you switch to `auto` or a lower mode keeps the checker enforcing.