perf(agent): posture-escalation signals and controller (PR10a)#736
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe agent loop now supports opt-in posture policies that observe execution outcomes, escalate once when configured triggers fire, adjust runtime controls, record posture escalations, and attach sandbox risk classifications to executed tool results. ChangesAgent posture escalation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Run
participant executeToolCall
participant profileController
participant trace
Run->>executeToolCall: execute tool call
executeToolCall-->>Run: ToolResult with Risk
Run->>profileController: observeToolOutcome(outcome, ToolResult)
Run->>profileController: maybeEscalate()
profileController-->>Run: PostureEscalation
Run->>trace: increment posture escalation counter
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/agent/profile_controller_test.go (1)
97-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this trace test into
internal/trace/trace_test.go.This assertion directly tests
trace.OptionalEventKeys, not agent behavior. As per coding guidelines, keep Go tests next to the source file they test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/profile_controller_test.go` around lines 97 - 104, Move TestOptionalEventKeysIncludePostureEscalations from the agent profile controller test file into internal/trace/trace_test.go, preserving its assertion and test behavior unchanged so it remains colocated with the trace.OptionalEventKeys implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/loop.go`:
- Around line 828-844: Ensure the CompletionUncertain path invokes
posture.observeUncertain() and applies any resulting escalation before
continuing, reusing a shared helper with the existing posture.maybeEscalate()
handling near the escalation act block. Preserve the one-shot and unprofiled-run
behavior, and add a Run-level test proving an uncertainty observation can
trigger OnCompletionUncertain escalation.
In `@internal/agent/profile_controller_test.go`:
- Around line 197-202: Update the no-profile assertion around result.FinalAnswer
so it requires the documented “maximum number of turns” outcome. Remove the
exact “recovered after escalation” exception and fail whenever the final answer
does not contain the expected maximum-turns message, while preserving the
existing request-ceiling assertion.
In `@internal/agent/profile_controller.go`:
- Around line 51-53: Update the risk escalation logic in the controller method
containing c.riskTripped and the related handling around lines 98-113: evaluate
risky mutations regardless of whether the tool result is StatusOK, including
partial failures, and validate that c.policy.OnRiskyMutation is a recognized
non-empty threshold before comparing risk ranks. Preserve escalation only for
valid configured thresholds that the result meets or exceeds.
---
Nitpick comments:
In `@internal/agent/profile_controller_test.go`:
- Around line 97-104: Move TestOptionalEventKeysIncludePostureEscalations from
the agent profile controller test file into internal/trace/trace_test.go,
preserving its assertion and test behavior unchanged so it remains colocated
with the trace.OptionalEventKeys implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24bc2867-1352-43e1-a9c8-7095f7ec2d69
📒 Files selected for processing (5)
internal/agent/loop.gointernal/agent/profile_controller.gointernal/agent/profile_controller_test.gointernal/agent/types.gointernal/trace/trace.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed current head fb0b0e2. Focused agent/trace race tests, vet, formatting, and diff hygiene pass. Two blockers remain. 1. OnCompletionUncertain is not wired into the loop at all: observeUncertain appears only in profile_controller.go and its unit tests. The CompletionUncertain branch in loop.go immediately continues, so merely adding the observation there would still skip the sole maybeEscalate act point at the end-of-turn tail. Refactor/apply escalation before that continue (or through a shared act helper) and add a loop-level regression proving an uncertain completion changes the next request/ceiling as configured and increments posture_escalations. 2. riskRank claims unknown levels rank lowest so a malformed threshold cannot trip spuriously, but observeToolOutcome compares resultRank >= thresholdRank. A non-empty invalid OnRiskyMutation therefore ranks 0 and every successful classified result ranks >= 0, firing escalation. Disable/reject unknown thresholds before comparison and add an invalid-threshold regression. The nil-profile and other trigger paths otherwise look sound.
|
Both blockers fixed in e8afb7f, and you were right on both counts. The uncertain path was worse than unwired: my observation call had silently failed to apply during editing, and even if it had landed, the branch continues the turn loop before the end-of-turn act point, so escalation would never have applied from there. The fix extracts the act logic into a shared applyPosture helper used at the tail and invoked immediately after observeUncertain inside the uncertain branch, before the continue. The new loop-level regression TestPostureEscalationOnUncertainCompletion drives a cue turn under the completion gate and asserts the very next request carries the target effort and posture_escalations increments. The risk-rank inversion is fixed the way you suggested: the threshold is validated first (an unrecognized level ranks 0 and disables the signal instead of matching everything), with TestProfileControllerIgnoresInvalidRiskThreshold pinning a typo threshold against a critical result. While in there I also took the bot's related point: the executed check is now DenialReason == DenialNone rather than StatusOK, so a partial failure that actually ran the mutation counts, with a test case for that too. The doc comments now describe the real behavior. Also folded in the two smaller bot asks: the unprofiled-run test now requires the max-turns answer outright, and the optional-event-key test moved to the trace package where it belongs. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewed head e8afb7f. Both prior blockers are resolved: CompletionUncertain now observes and immediately applies posture through the shared helper before the loop continues, with a loop-level regression proving the next request changes and posture_escalations increments; invalid OnRiskyMutation thresholds now disable the signal instead of matching every classified result. The follow-up also correctly counts executed partial failures while excluding denied calls, strengthens the nil-profile regression, and colocates the trace-key test. PR-specific race tests, trace tests, go vet, and git diff --check pass. Approving.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE (code) — clean, correct, genuinely inert slice. One pre-merge requirement: rebase off the stacked PRs first.
Verified against the actual code, not just the description:
- Inert for existing callers:
Options.Profileis nil everywhere → nil-policy controller, everyobserve*early-returns,maybeEscalatereturns false, and the counter fires only inside theif firedblock. Transcript/model behavior is byte-identical. ✅ sandbox.Classifyis pure (classifyWithScope(request, nil), no I/O/prompting); sandboxed calls reusepreflightDecision.Risk(no double-eval). ✅maxTurnsis read dynamically in the loop condition andapplyPostureonly ever raises it — escalation can extend a run, never shorten one. ✅- Defensive controller: one-shot latch;
riskRankranks unknown levels 0 so a typo'd threshold disables the signal instead of matching everything; targets are the already-valid displaced values. ✅ - Counter registered in
OptionalEventKeys(); the dualapplyPosturesites are safe via the one-shot latch. 11 new tests incl. the no-op twin and the end-to-end act path. ✅
1. [Pre-merge, process] Rebase off the stacked #730/#732/#734. The branch carries their commits (dbd9443 #730, 0fae754 #734, 2cc6f43 #732), so the diff shows 17 files instead of the real 6, and merging as-is would pull those PRs' contents into main and duplicate/conflict when they land on their own. Rebase onto clean main (or merge only after they do) so #736 contains just the 6 posture commits.
2. [Minor] Risk stamp runs unconditionally. executeToolCall computes executedRisk regardless of Profile, so unprofiled runs now run sandbox.Classify on each executed unsandboxed call. Byte-identical output and Classify is cheap, so 'no perf change' holds in practice — but it's not literally zero new work. Optional: gate the unsandboxed-classify branch on options.Profile != nil for truly cost-free unprofiled runs (tradeoff: field unavailable to future non-profile consumers).
Nice work — the slice does exactly what it claims. Merge is kevin's call per the program gate, after the rebase.
3 participants
Summary
First slice of PR10 of the Zero Agent Performance Program (execution profiles): the run-policy signal plumbing and the posture-escalation controller, with no user surface and no behavior change. The profile catalog, the
--exec-profileselector, and the baseline-tuned knob values follow in the next slice once the Phase 0 numbers are settled; this slice is deliberately inert so it can land independently of the M1 discussion.Added
ToolResult.Risk: executed tool results now carry the sandbox risk classification the permission path already computes. Sandboxed calls reuse the preflight decision's risk (no second evaluation); unsandboxed calls run the same pure classifier the permission event path uses; unknown-tool and denied results keep the zero value. Pure observation: permissions, sandboxing, and prompting are untouched.feedback, _ :=); the loop now observes it. Feedback handling is unchanged.ProfilePolicy/PostureEscalationtypes onagent.Options: a one-shot, in-run escalation to stricter knob values (turn ceiling, reasoning effort, completion gate), with triggers for tool-failure streaks, uncertain completion evaluations, self-correct failures, and executed mutations at or above a risk threshold. Targets are the values a profile displaced at run start, so escalation can never introduce a value that was not already valid for the run.profileController: observes the signals at the loop's existing seams (the repeated-failure guard's own streak, the completion gate's uncertain branch, the self-correct call, executed results) and applies at most one escalation per run at the end-of-turn tail. It mutates only per-turn-read policy values; it never injects messages, never touches the model or session, and is fully independent of the model-initiatedescalate_modelpath.posture_escalationstrace counter (optional event key), deliberately distinct frommodel_switches.No behavior change
Options.Profileis nil for every existing caller, which makes every controller method a no-op; the loop is byte-identical for unprofiled runs (pinned byTestPostureEscalationAbsentWithoutProfileand the nil-policy test, and the full agent suite passes unchanged). The risk stamp adds a field to executed results that nothing consumes yet. No performance change expected.Verification
go build ./...,go vet,gofmtclean on touched filesinternal/agentandinternal/tracesuites greenDependencies and follow-ups
Builds on the completion policy (#719) and the turn-session seam (#720). Follow-ups per the program plan: the
internal/execprofilecatalog with--exec-profileand baseline-tuned values (next slice, with A/B evidence for the Fast profile gate), then the TUI/profilecommand.Summary by CodeRabbit