feat: implement ExitPlanMode HITL for Tool Permission Model#1
Closed
quay-devel wants to merge 2 commits into
Closed
feat: implement ExitPlanMode HITL for Tool Permission Model#1quay-devel wants to merge 2 commits into
quay-devel wants to merge 2 commits into
Conversation
Add ExitPlanMode as a human-in-the-loop tool alongside AskUserQuestion, enabling plan approval workflows in ACP sessions. This implements the spec from PR ambient-code#1586 (closes ambient-code#1583). Runner: - Add ExitPlanMode to BUILTIN_FRONTEND_TOOLS halt set - Enrich ExitPlanMode tool args with plan file content from .claude/plans/ - Complete Tier 1 tool allowlist (NotebookEdit, WebFetch, TodoWrite, etc.) Backend: - Generalize isAskUserQuestionToolCall → isHITLToolCall to detect both AskUserQuestion and ExitPlanMode for status derivation and compaction - Add ExitPlanMode test cases for waiting_input detection Frontend: - Generalize HITL detection in use-agent-status and stream-message - Add ExitPlanModeMessage component with approve/reject/request-changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds ExitPlanMode HITL support: generalized HITL name normalization/predicates, backend status/compaction updates, a new ExitPlanModeMessage React component and stream wiring, Claude adapter plan-file injection, and an expanded default tool allowlist. ChangesExitPlanMode HITL Tool Feature
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🤖 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 `@components/frontend/src/components/session/exit-plan-mode.tsx`:
- Around line 42-43: Validate and coerce the incoming payload before rendering:
ensure planContent is a string (e.g., check typeof input.planContent ===
"string" and fallback to "" for ReactMarkdown) and ensure allowedPrompts is an
array (use Array.isArray(input.allowedPrompts) ? input.allowedPrompts as
AllowedPrompt[] : []), then use that sanitized values for rendering and mapping;
update usages in this component (references: planContent, allowedPrompts,
ReactMarkdown and the map over allowedPrompts) so malformed payloads don’t cause
runtime errors.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 84-104: The _read_plan_file function can crash or return unsafe
content because plan_files sorting uses p.stat() which can raise on
broken/unreadable files and symlinked *.md can escape .claude/plans; modify
_read_plan_file to: 1) iterate over plans_dir.glob("*.md") and build a safe list
by catching OSError when calling p.stat() (skip files that error), 2) for each
candidate ensure p.is_file() and that
p.resolve().is_relative_to(plans_dir.resolve()) or compare commonpath to prevent
symlink escape, 3) sort the filtered safe list by mtime (use try/except when
reading mtime), and 4) catch exceptions around read_text (OSError) and return
None on failure—refer to symbols _read_plan_file, plans_dir, plan_files,
p.stat(), p.resolve(), and read_text to locate changes.
🪄 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: 47a7b2ed-0ad3-4ad3-a9fb-8dc4143b2125
📒 Files selected for processing (8)
components/backend/websocket/agui_proxy.gocomponents/backend/websocket/agui_store.gocomponents/backend/websocket/agui_store_test.gocomponents/frontend/src/components/session/exit-plan-mode.tsxcomponents/frontend/src/components/ui/stream-message.tsxcomponents/frontend/src/hooks/use-agent-status.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py
Comment on lines
+42
to
+43
| const planContent = (input.planContent as string) || ""; | ||
| const allowedPrompts = (input.allowedPrompts as AllowedPrompt[]) || []; |
There was a problem hiding this comment.
Validate tool payload shapes before rendering
Lines 42-43 assume planContent is string and allowedPrompts is array. If payload shape is malformed, this can crash at Line 117 (map on non-array) or pass invalid input into ReactMarkdown at Line 106.
Proposed fix
- const planContent = (input.planContent as string) || "";
- const allowedPrompts = (input.allowedPrompts as AllowedPrompt[]) || [];
+ const planContent =
+ typeof input.planContent === "string" ? input.planContent : "";
+ const allowedPrompts: AllowedPrompt[] = Array.isArray(input.allowedPrompts)
+ ? input.allowedPrompts.filter(
+ (p): p is AllowedPrompt =>
+ !!p &&
+ typeof p === "object" &&
+ typeof (p as { tool?: unknown }).tool === "string" &&
+ typeof (p as { prompt?: unknown }).prompt === "string",
+ )
+ : [];Also applies to: 104-109, 117-123
🤖 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 `@components/frontend/src/components/session/exit-plan-mode.tsx` around lines
42 - 43, Validate and coerce the incoming payload before rendering: ensure
planContent is a string (e.g., check typeof input.planContent === "string" and
fallback to "" for ReactMarkdown) and ensure allowedPrompts is an array (use
Array.isArray(input.allowedPrompts) ? input.allowedPrompts as AllowedPrompt[] :
[]), then use that sanitized values for rendering and mapping; update usages in
this component (references: planContent, allowedPrompts, ReactMarkdown and the
map over allowedPrompts) so malformed payloads don’t cause runtime errors.
Comment on lines
+84
to
+104
| def _read_plan_file(options: Any) -> str | None: | ||
| """Read the most recent plan file from .claude/plans/ in the cwd.""" | ||
| cwd = None | ||
| if isinstance(options, dict): | ||
| cwd = options.get("cwd") | ||
| elif hasattr(options, "cwd"): | ||
| cwd = options.cwd | ||
| if not cwd: | ||
| return None | ||
| plans_dir = Path(cwd) / ".claude" / "plans" | ||
| if not plans_dir.is_dir(): | ||
| return None | ||
| plan_files = sorted( | ||
| plans_dir.glob("*.md"), key=lambda p: p.stat().st_mtime, reverse=True | ||
| ) | ||
| if not plan_files: | ||
| return None | ||
| try: | ||
| return plan_files[0].read_text(encoding="utf-8") | ||
| except OSError: | ||
| return None |
There was a problem hiding this comment.
Harden plan-file discovery to prevent run failures and file exfiltration
Line 97 can raise on broken/unreadable files (p.stat() in sort key), which can fail the stream. Also, symlinked *.md entries can escape .claude/plans and inject arbitrary file contents into planContent (Line 1083).
Proposed fix
def _read_plan_file(options: Any) -> str | None:
"""Read the most recent plan file from .claude/plans/ in the cwd."""
@@
- plan_files = sorted(
- plans_dir.glob("*.md"), key=lambda p: p.stat().st_mtime, reverse=True
- )
- if not plan_files:
+ root = plans_dir.resolve()
+ candidates: list[tuple[float, Path]] = []
+ for p in plans_dir.glob("*.md"):
+ try:
+ # Do not follow symlinks outside plans dir
+ resolved = p.resolve()
+ if root not in resolved.parents:
+ continue
+ if not p.is_file() or p.is_symlink():
+ continue
+ candidates.append((p.stat().st_mtime, p))
+ except OSError:
+ continue
+
+ if not candidates:
return None
+ candidates.sort(key=lambda t: t[0], reverse=True)
+ latest = candidates[0][1]
try:
- return plan_files[0].read_text(encoding="utf-8")
+ return latest.read_text(encoding="utf-8")
except OSError:
return NoneAlso applies to: 1076-1086
🤖 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 `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines
84 - 104, The _read_plan_file function can crash or return unsafe content
because plan_files sorting uses p.stat() which can raise on broken/unreadable
files and symlinked *.md can escape .claude/plans; modify _read_plan_file to: 1)
iterate over plans_dir.glob("*.md") and build a safe list by catching OSError
when calling p.stat() (skip files that error), 2) for each candidate ensure
p.is_file() and that p.resolve().is_relative_to(plans_dir.resolve()) or compare
commonpath to prevent symlink escape, 3) sort the filtered safe list by mtime
(use try/except when reading mtime), and 4) catch exceptions around read_text
(OSError) and return None on failure—refer to symbols _read_plan_file,
plans_dir, plan_files, p.stat(), p.resolve(), and read_text to locate changes.
- Extract shared hitl-tools.ts with normalizeToolName, isHITLTool, isAskUserQuestionTool, isExitPlanModeTool, and hasToolResult helpers - Remove duplicated hasResult and tool detection functions from ask-user-question.tsx, exit-plan-mode.tsx, stream-message.tsx, and use-agent-status.ts - Add 100KB size guard to _read_plan_file to prevent oversized events - Log JSON errors during ExitPlanMode plan enrichment instead of silently swallowing them - Use stable composite key for allowedPrompts list rendering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the Tool Permission Model spec from PR ambient-code#1586, adding
ExitPlanModeas a HITL (human-in-the-loop) tool that halts the event stream and waits for user approval — the same mechanism already used byAskUserQuestion.ExitPlanModeadded toBUILTIN_FRONTEND_TOOLShalt set; plan file content injected into tool args; Tier 1 allowlist completed with all missing toolsisAskUserQuestionToolCall→isHITLToolCallto detect both tools for status derivation and snapshot compaction; new test cases for ExitPlanModeuse-agent-status.tsandstream-message.tsx; newExitPlanModeMessagecomponent with approve/reject/request-changes actionsCloses ambient-code#1583
Spec: ambient-code#1586
Test plan
go test ./websocket/...)npm run build)npx vitest run)panic()in Go code, noanytypes in TypeScript🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests