Agent runtime: ask_user interception, context compaction, guardrails

[gnanam1990]

Agent runtime — ask_user, compaction, guardrails

Ports the agent-runtime slices from the reskin branch onto main, minus the blueprint's own sub-agent/task interception — main already has the merged specialist Task implementation (anandh8x's), which this PR preserves rather than duplicating.

Built as a selective 3-way merge: started from main's loop.go/types.go, brought in the agent features, kept main's specialist drift, and excised the blueprint task path (executeTask, taskDisplaySummary, maxTaskDepth, Options.Provider, the task interception, and the one task usage-propagation test).

What's in it

• ask_user (S6) — AskUserRequest/AskUserResponse/AskUserQuestion + Options.OnAskUser. The loop intercepts ask_user and routes to the interactive front-end, degrading gracefully to the non-interactive fallback when no handler is set. Answers go through the redaction boundary.
• Compaction (S8) — Options.ContextWindow + CompactionPreserveLast. Proactive compaction when estimated history crosses a fraction of the window; reactive recovery on context-limit errors (summarizes the old middle, keeps system + last N). ContextWindow=0 disables it, so every existing caller/test is unchanged.
• Guardrails — empty-turn stop, repeated-tool-failure hint/stop, stale plan reminder, aborted-placeholder on guard stop; confirmation_policy.md embedded into the system prompt.

Preserved from main (specialist)

RunWithOptions tool-context fields (ToolCallID/SessionID/Model/ReasoningEffort/Depth/Cwd), propertyToRuntimeMap nested Items, ToolAdvertised AdvertiseInAuto. Dropped Options.Provider (blueprint-task-only; compaction uses Run's provider arg).

Testing

54 agent tests pass (incl. the new compaction/guardrails/ask_user Run-level tests). go build ./..., go vet, go test ./..., go test -race ./internal/agent/, GOOS=windows go build ./... all green. No new deps.

Unblocks the TUI shell and cli wiring PRs. Part of decomposing #101.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Interactive user-question prompts during runs; agent can request and accept structured answers.
  • Automatic conversation compaction to keep long sessions concise.

• Improvements

  • Stronger guardrails to prevent runaway behavior and repeated-failure loops.
  • Secrets in tool/handler output are redacted; malformed tool calls receive clearer handling and retries.
  • Confirmation policy is embedded into system prompts.

• Documentation

  • Added confirmation policy guidance.

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: ba6cf26d9389
Changed files (9): internal/agent/ask_user_test.go, internal/agent/compaction.go, internal/agent/compaction_test.go, internal/agent/confirmation_policy.md, internal/agent/guardrails.go, internal/agent/guardrails_test.go, internal/agent/loop.go, internal/agent/loop_test.go, internal/agent/types.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c01bfac-b548-4a55-a704-a52f47bcd53f

📥 Commits

Reviewing files that changed from the base of the PR and between cbaca4a and ba6cf26.

📒 Files selected for processing (5)

• internal/agent/ask_user_test.go
• internal/agent/compaction_test.go
• internal/agent/guardrails_test.go
• internal/agent/loop.go
• internal/agent/loop_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/agent/loop_test.go
• internal/agent/compaction_test.go

---

Walkthrough

Adds confirmation-policy embedding, session-history compaction (proactive/reactive), run-loop guardrails, and ask_user interception with redaction; integrates these into Run with retry/mid-turn-abort semantics and extensive unit and integration tests.

Changes

Agent Runtime Loop Enhancements

Layer / File(s)	Summary
Types & confirmation policy internal/agent/types.go, internal/agent/confirmation_policy.md, internal/agent/loop.go	Adds ask_user types and Options fields (ContextWindow, CompactionPreserveLast, OnAskUser); extends ToolResult with Redacted, ChangedFiles, Display; embeds confirmation policy into system prompt.
Compaction primitives internal/agent/compaction.go	Implements Compact, token-estimation heuristic, safe-suffix logic, compactionState, proactive maybeCompact, reactive recover, summarizer closure, and transcript rendering.
Compaction tests & Run integration internal/agent/compaction_test.go	Unit tests for Compact invariants and token estimates; integration tests exercising proactive/reactive compaction and retry behavior in Run.
Guardrails core internal/agent/guardrails.go	Adds guardState, empty-turn stopping, plan-reminder and stale-plan one-shot logic, normalized failure signatures, and hint/stop message builders for repeated tool failures.
Guardrails tests internal/agent/guardrails_test.go	Tests empty-turn stop/reset behavior, dropped-call exemptions, plan-not-called/stale reminders (one-shot), and repeated failing-tool halting with hint injection.
Run loop integration & ask_user internal/agent/loop.go	Integrates compaction into Run (proactive/reactive) with retry; detects dropped tool calls and injects user notices; implements repeated-failure mid-turn halting with aborted placeholder tool_results; intercepts ask_user via executeAskUser with OnAskUser routing, cancellation/abort handling, and redaction for intercepted outputs.
Loop tests (prompt, retries, redaction) internal/agent/loop_test.go, internal/agent/ask_user_test.go	Adds tests for confirmation-policy embedding, dropped-tool-call retry/mixed-call handling, aborted placeholders for unexecuted calls, ask_user handler flows (answers, missing questions, errors, cancellation), and secret scrubbing/redaction verification.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• Deep audit: 43 confirmed findings (9 high) — bugs, wiring & security gaps in PR #101 #102: The changes touch ask_user secret-scrubbing and loop-interception code paths (loop.go/types.go) referenced by the issue.

Possibly related PRs

• Gitlawb/zero#51: Introduces agent runtime core that this PR extends with compaction, guardrails, confirmation-policy, and ask_user handling.

Suggested reviewers

• Vasanthdev2004
• anandh8x

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.80% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the three major features added in this PR: ask_user interception, context compaction, and guardrails—all core runtime enhancements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent-runtime

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (2)

internal/agent/compaction_test.go (1)

529-542: ⚡ Quick win

Remove unused variable.

Line 532 declares called := false but it's never set to true anywhere, and line 538 ignores it with _ = called. This is dead code. Either:

• Remove the called variable entirely (the test already verifies non-invocation by checking retried and err), OR
• Have the mock provider set called = true to explicitly verify the provider was not invoked

Currently, called serves no purpose.

🤖 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/compaction_test.go` around lines 529 - 542, In
TestRecoverDisabledIsNoop, remove the dead local variable `called` (and its
no-op assignment `_ = called`) since it's never mutated; update the test to rely
on the existing assertions (retried, err, len(got)) or alternatively have the
mockProvider set a flag if you prefer explicit invocation checks—locate the
variable declaration `called := false` in TestRecoverDisabledIsNoop and delete
it and its later unused reference.

internal/agent/confirmation_policy.md (1)

1-84: ⚡ Quick win

Trust-model policy addition requires maintainer alignment.

This file introduces a comprehensive confirmation policy that will be embedded in the system prompt, defining when the agent blocks, confirms, or auto-approves actions with side effects. The policy appears well-designed (appropriate BLOCKED actions, reasonable ALWAYS CONFIRM thresholds, sensible pre-approval conditions, and good hygiene rules including blocking interactive programs that would hang).

However, as a trust-model and permission-policy change, explicit maintainer alignment is recommended before merge. Based on learnings: verify that trust-model and permission-policy changes receive explicit maintainer review.

The PR description clearly documents this addition, so it is not hidden—this comment is for awareness and alignment verification.

Based on learnings from similar PRs about trust-model and permission-policy changes requiring explicit maintainer alignment.

🤖 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/confirmation_policy.md` around lines 1 - 84, This policy
introduces trust-model/permission changes and therefore needs explicit
maintainer alignment before merge; update the PR and the policy to require
maintainer sign-off by (1) adding a clear "Requires maintainer approval" notice
at the top of the "Confirmation Policy" document (near the "Confirmation Modes"
/ "Confirmation Hygiene Rules" headings), (2) updating the PR description to
request and tag a maintainer review and add a maintainer-approval checklist
item, and (3) (optional) ensure a CODEOWNERS or PR template entry enforces
maintainer review for changes to this policy so merges cannot proceed without
their explicit sign-off.

🤖 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/compaction_test.go`:
- Around line 412-418: The test comment and assertion disagree: the code
(strings.Count(joined, "recovered") != 0) and the error text ("must NOT be
re-streamed") require zero occurrences, but the comment says "at most once."
Update the comment above the joined := strings.Join(deltas, "") line to state
that the retried turn's text must NOT be streamed to OnText (i.e., zero
occurrences), keeping the existing assertion using strings.Count(joined,
"recovered") and the current failure message.

In `@internal/agent/loop.go`:
- Around line 426-429: The current logic converts any error from
options.OnAskUser into a synthetic fallback via askUserFallbackResult; change it
so that if the error is a cancellation or timeout (check errors.Is(err,
context.Canceled) || errors.Is(err, context.DeadlineExceeded) after calling
options.OnAskUser in the function containing response, err :=
options.OnAskUser(ctx, request)), you propagate/return the cancellation error
immediately instead of calling askUserFallbackResult; for all other errors keep
the existing fallback path. Ensure you import/use the errors package for
errors.Is.
- Around line 215-231: The guard currently uses tools.StatusError broadly;
change the logic so schema hints are only injected for
retriable/argument/execution failures (not for disabled, permission_denied, or
sandbox_violation). Either update guards.observeToolResult to set
outcome.InjectHint=false for non-retriable error categories returned by
executeToolCall, or keep observeToolResult as-is and change the call site to
check the toolResult error kind before using outcome.InjectHint (e.g., add an
isRetriableToolError(toolResult) predicate and require it in the condition that
calls toolFailureHint and toolSchemaJSON). Ensure you reference the error
classification values returned by executeToolCall rather than treating every
tools.StatusError the same.
- Around line 416-418: The headless ask_user path uses registry.Run (via
askUserFallbackResult) which drops RunWithOptions metadata (ToolCallID,
SessionID, Model, ReasoningEffort, Depth, Cwd and returned metadata/display
fields) and thus violates the RunWithOptions contract; update
askUserFallbackResult (and the similar call sites at the other locations
referenced) to invoke registry.RunWithOptions (or otherwise forward the original
RunOptions) so the same RunOptions passed into RunWithOptions are preserved and
returned for ask_user, ensuring
ToolCallID/SessionID/Model/ReasoningEffort/Depth/Cwd and any metadata/display
fields are passed through unchanged.

---

Nitpick comments:
In `@internal/agent/compaction_test.go`:
- Around line 529-542: In TestRecoverDisabledIsNoop, remove the dead local
variable `called` (and its no-op assignment `_ = called`) since it's never
mutated; update the test to rely on the existing assertions (retried, err,
len(got)) or alternatively have the mockProvider set a flag if you prefer
explicit invocation checks—locate the variable declaration `called := false` in
TestRecoverDisabledIsNoop and delete it and its later unused reference.

In `@internal/agent/confirmation_policy.md`:
- Around line 1-84: This policy introduces trust-model/permission changes and
therefore needs explicit maintainer alignment before merge; update the PR and
the policy to require maintainer sign-off by (1) adding a clear "Requires
maintainer approval" notice at the top of the "Confirmation Policy" document
(near the "Confirmation Modes" / "Confirmation Hygiene Rules" headings), (2)
updating the PR description to request and tag a maintainer review and add a
maintainer-approval checklist item, and (3) (optional) ensure a CODEOWNERS or PR
template entry enforces maintainer review for changes to this policy so merges
cannot proceed without their explicit sign-off.

🪄 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 Plus

Run ID: a9b97bba-7383-45a4-bf19-47f176633355

📥 Commits

Reviewing files that changed from the base of the PR and between 0da2ecf and cbaca4a.

📒 Files selected for processing (9)

• internal/agent/ask_user_test.go
• internal/agent/compaction.go
• internal/agent/compaction_test.go
• internal/agent/confirmation_policy.md
• internal/agent/guardrails.go
• internal/agent/guardrails_test.go
• internal/agent/loop.go
• internal/agent/loop_test.go
• internal/agent/types.go

[Vasanthdev2004]

Verdict: request changes.

CI is green, but the agent-loop behavior issues below should be fixed before merge. They affect cancellation semantics, the RunWithOptions contract for ask_user fallback, and repeated-failure hinting for non-retriable errors.

[gnanam1990]

Addressed all four (thanks both):

1. Repeated-failure hint scoped to retriable errors — new isRetriableToolError (keys off the permission_action meta + the known disabled/permission/sandbox messages); policy refusals no longer drive the schema hint or the repeated-failure stop. Test TestIsRetriableToolError.
2. Headless ask_user fallback preserves the tool contract — now via registry.RunWithOptions with the full run context, copying Meta/ChangedFiles/Display.
3. Canceled ask_user aborts — context.Canceled/DeadlineExceeded from OnAskUser now returns that error from Run (executeToolCall/executeAskUser thread an abort error; remaining advertised calls are closed out for replay validity) instead of a synthetic answer. Test TestRunAskUserCancellationAbortsRun.
4. compaction_test comment aligned with the assertion.

build/vet/-race/full-suite + windows green. @coderabbitai review

[coderabbitai]

@gnanam1990 Thanks for the thorough follow-through on all four items — the isRetriableToolError scoping, the headless fallback using registry.RunWithOptions, the cancellation abort path, and the comment alignment all look like solid fixes. Kicking off the review now.

(∩^ω^)⊃━☆

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[gnanam1990]

@Vasanthdev2004 re-review please — all four items are fixed on the current head (ba6cf26): retriable-only failure hint (isRetriableToolError), headless ask_user via RunWithOptions with full fields, canceled ask_user now aborts the run with the error (not a synthetic answer), and the compaction_test comment. Tests added (TestIsRetriableToolError, TestRunAskUserCancellationAbortsRun); build/vet/-race/full-suite + windows green.

[Vasanthdev2004]

Re-reviewed current head ba6cf26.

Previous blockers are addressed:

• ask_user cancellation now aborts instead of fabricating a fallback answer.
• headless ask_user fallback now goes through RunWithOptions and preserves result fields.
• repeated-failure schema hints are limited to retriable tool failures.
• compaction test comment mismatch is fixed.

Validation passed locally:

• go test ./...
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• git diff --check origin/main...origin/pr-125

CI/CodeRabbit are green. Approved.