Claude Code Review

Summary

Clean, well-scoped implementation that correctly generalizes AskUserQuestion's HITL machinery to cover ExitPlanMode. The DRY refactor — extracting hitl-tools.ts to replace three independent copies of the same normalization logic — is the highlight. Backend Go tests adequately cover the new ExitPlanMode status detection. No blockers or criticals.

Findings

Blocker

None.

Critical

None.

Major

None.

Minor

1. Exact string match in adapter.py inconsistent with normalized approach everywhere else
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py — the enrichment guard:

if current_tool_display_name == "ExitPlanMode":

...uses an exact case-sensitive comparison while the backend (isHITLToolCall) and frontend (normalizeToolName) both strip non-alpha chars and lowercase before matching. If the SDK ever emits exitPlanMode or exit_plan_mode (unlikely, but it has happened with tool renames), the enrichment silently skips and the frontend renders a plan-review card with no plan content. The detection still fires because BUILTIN_FRONTEND_TOOLS uses exact match too, so the inconsistency is contained — but a helper that mirrors the normalization pattern would make this robust:

def _normalize_tool_name(name: str) -> str:
 return "".join(c for c in name.lower() if c.isalpha())
if _normalize_tool_name(current_tool_display_name) == "exitplanmode":

2. _read_plan_file is untested
The function is pure (no side effects beyond I/O) and has non-trivial behavior: mtime sort, 100 KB truncation at a byte boundary, UTF-8 decode with errors="ignore". The runner test suite covers other functions — adding a few table-driven tests for this one would prevent silent regressions from filesystem-layout assumptions. At minimum: empty plans dir, single file under limit, single file over limit, non-UTF-8 edge (though errors="ignore" handles it).

3. Removed comment in ask-user-question.tsx reduced signal
Line 35 previously had: // Handle simple { question: "..." } format (e.g. from Claude Code AskUserQuestion tool). This documented a non-obvious branch — the two-shape input protocol (AskUserQuestionInput[] vs bare { question: string }). The logic is preserved but the explanation is gone. Worth reinstating as a single-line comment since the shape switch is a subtlety a future reader would wonder about.

4. exit-plan-mode.tsx is exactly 200 lines (at the convention boundary)
Frontend conventions say "Components under 200 lines." The file counts 200 lines including the displayName footer. No refactor needed now, but note that any future additions push it over the stated limit.

Nit

• input.allowedPrompts as AllowedPrompt[] — the Array.isArray guard validates it's an array but doesn't validate element shapes. Fine for now given the source is Claude tool args, but a comment noting the trust boundary would help a reader.
• Buttons in the action row (Approve / Reject / Request Changes) don't show a spinner on isSubmitting. They disable correctly, so there's no functional bug, but user feedback on the submitting state is absent for Reject and Request Changes. Could be a follow-up.

Positive Highlights

• Excellent DRY refactoring. hitl-tools.ts consolidates tool-name normalization from three independent copies (stream-message, use-agent-status, ask-user-question) into one source of truth with five well-named exports. This is exactly the right move.
• Go test quality. The new RUN_FINISHED with same-run ExitPlanMode returns waiting_input test covers the most important status-derivation path, and the extended case-insensitive table now covers both tools symmetrically.
• Proper 100 KB plan file cap. _PLAN_FILE_MAX_BYTES is defined as a named constant and enforced before sending content over the wire — prevents runaway payload sizes without silently dropping the content.
• UI state machine is correct. disabled = alreadyAnswered || submitted || isSubmitting || !isNewest correctly covers every state where the user should not be able to interact. The finally block in handleDecision ensures isSubmitting always resets even on throw.
• Zero any types, no panic(), Shadcn UI used throughout, React Query not applicable (purely prop-driven component). Conventions are clean across the stack.

Recommendations

1. (Minor) Normalize current_tool_display_name before the ExitPlanMode enrichment guard — a one-line helper keeps it consistent with the rest of the system.
2. (Minor) Add unit tests for _read_plan_file covering the truncation and empty-directory cases.
3. (Nit) Reinstate the single-line comment on the bare { question } branch in ask-user-question.tsx.

Amber · standards loaded from CLAUDE.md, specs/standards/backend/, specs/standards/frontend/, specs/standards/security/

Amber Code Review — PR#1589: feat: implement ExitPlanMode HITL for Tool Permission Model

Summary

Solid, well-structured implementation of ExitPlanMode as a second HITL tool. The abstraction into hitl-tools.ts is exactly the right move — it eliminates three duplicate detection functions and gives a clean single source of truth. The backend status-derivation and compaction logic correctly handles the ExitPlanMode lifecycle. One critical behavioral uncertainty (tool result format), one major finding (unguarded Tier-1 tool expansion), and four minor findings.

Findings

Blocker

None.

Critical

1. ExitPlanMode tool result format not verified against Claude Code CLI expectations
exit-plan-mode.tsx:63–68 — The component sends JSON.stringify({decision: "approve" | "reject" | "request_changes", feedback?: "..."}) as the tool result. Whether Claude Code CLI's ExitPlanMode actually expects this structured JSON — or expects natural-language text (e.g. "Yes, proceed") — is unspecified in the diff. If the format is wrong, Claude receives a malformed tool result and may behave unexpectedly after user approval.

The test plan has [ ] (unchecked) for "verify approve/reject/request-changes sends correct tool result." This is the exact gap. The result contract with Claude Code CLI must be established before merge.

Fix: Document the expected tool result schema (from the Claude Code SDK docs or spec PR#1586). Add a test that mocks onSubmitAnswer and asserts the correct JSON structure is produced for each button path (approve, reject, request_changes).

Major

2. Tier-1 allowlist expansion in mcp.py without feature flags
components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py:29–51 — Fourteen new tools added to DEFAULT_ALLOWED_TOOLS, several with meaningful blast radius:

• CronCreate — allows the runner to schedule persistent jobs that fire independent of session lifecycle.
• EnterWorktree / ExitWorktree — creates isolated git worktrees; side effects persist after session ends if cleanup fails.
• WebFetch — opens server-side SSRF risk if not already mitigated elsewhere.

Per CLAUDE.md: "Feature flags strongly recommended — gate new features behind Unleash flags." No flag here means a bad behavior from any of these tools has no kill switch.

If these tools were specified by the Tier-1 allowlist in PR#1586 and their addition is intentional/non-new, add a code comment pointing at the spec. Consider a single exitplanmode-hitl flag gating the expansion. At minimum, CronCreate and EnterWorktree/ExitWorktree warrant individual flags or a clear rationale for unflagged rollout.

Minor

3. border-l-3 is not a standard Tailwind CSS class
exit-plan-mode.tsx:88, 92 — Tailwind's built-in border-left width scale is 0, 1, 2, 4, 8 — no border-l-3. In standard Tailwind v3 JIT, this class compiles to nothing (the accent border won't render). If the existing AskUserQuestionMessage uses the same class and it works, there's a custom config in play — worth confirming. If not, replace with border-l-2 or border-l-4.

4. handleDecision silently drops submission errors
exit-plan-mode.tsx:55–68 — The try/finally block resets isSubmitting after an onSubmitAnswer failure, but no error is surfaced to the user. They can retry (since submitted stays false), but get no indication anything went wrong. Add an error state and display an inline message or toast on failure.

5. debug log level for ExitPlanMode plan injection failure
adapter.py:1092 — logger.debug("Failed to enrich ExitPlanMode with plan content: %s", e) — if this fires, the user sees the plan approval card with no plan content. That's confusing enough to warrant logger.warning(...) so it appears in production logs without requiring debug verbosity. Optionally add a logger.debug inside _read_plan_file when the plans directory doesn't exist, to aid onboarding debugging.

6. Removed non-obvious compatibility comment in ask-user-question.tsx
ask-user-question.tsx:34 — The deleted comment # Handle simple { question: "..." } format (e.g. from Claude Code AskUserQuestion tool) was the only explanation for why this second format path exists. A future maintainer won't know this is a backward-compat branch without it. Please restore.

Positive Highlights

• hitl-tools.ts consolidation is exactly right. Three files had duplicate detection functions; one shared library with named exports is clean and extensible.
• Backend compaction and status derivation is logically sound. compactFinishedRun correctly preserves only ToolCallStart (not ToolCallFinished) for HITL tools, and DeriveAgentStatus correctly scopes to the most recent run so compacted events from answered prompts don't produce false waiting_input statuses.
• _read_plan_file has sensible guardrails: 100KB cap, errors="ignore" on truncated UTF-8, OSError catch, graceful None return when no plans dir exists.
• isNewest guard on all HITL components correctly prevents users from re-submitting answers to stale prompts from prior runs.
• Test coverage for ExitPlanMode mirrors the existing AskUserQuestion cases exactly in agui_store_test.go — right pattern.
• Go naming consistency: isHITLToolCall in Go mirrors isHITLTool in TypeScript — good cross-layer coherence.

Recommendations (prioritized)

1. (Critical — before merge) Confirm tool result schema against Claude Code CLI's ExitPlanMode expectations and add an automated test for each decision path.
2. (Major) Add an Unleash feature flag gating at minimum CronCreate, EnterWorktree, ExitWorktree additions to DEFAULT_ALLOWED_TOOLS.
3. (Minor) Replace border-l-3 with border-l-4 (or confirm it resolves correctly in the project's Tailwind config).
4. (Minor) Add error state display to handleDecision for onSubmitAnswer failures.
5. (Minor) Raise plan injection log level to warning.
6. (Minor) Restore the {question: "..."} backward-compat comment in parseQuestions.

🤖 Amber — code review agent | ambient-code/platform

🤖 Amber Review — PR #1589: feat: implement ExitPlanMode HITL for Tool Permission Model

Summary

Clean implementation of ExitPlanMode as a second HITL tool alongside AskUserQuestion. The change is well-scoped: the runner halts on ExitPlanMode, the backend generalizes its HITL detection, and the frontend gains a plan approval component with approve/reject/request-changes actions. Code quality is high and the PR follows established patterns throughout.

Findings

Blocker

None.

Critical

None.

Major

None.

Minor

1. hitl-tools.ts:14-17 — isHITLTool duplicates normalized string comparisons

   // Current
   export function isHITLTool(name: string): boolean {
    const normalized = normalizeToolName(name);
    return normalized === "askuserquestion" || normalized === "exitplanmode";
   }
   // Simpler — delegates to already-defined functions, can't drift out of sync
   export function isHITLTool(name: string): boolean {
    return isAskUserQuestionTool(name) || isExitPlanModeTool(name);
   }

   Not a functional bug, but if a third HITL tool is added later, this is a two-place update instead of one. Style nit.

2. adapter.py _read_plan_file — picks most-recent .md by mtime without session scoping
   If a session workspace is ever reused or if multiple plan files accumulate, this could inject a stale plan. Low risk given current session isolation, but worth naming. A note in the function docstring ("assumes CWD is an isolated session workspace") would make the assumption explicit.

3. ask-user-question.tsx:40 — removed inline comment explaining fallback format
   The deleted comment // Handle simple { question: "..." } format (e.g. from Claude Code AskUserQuestion tool) explained why the fallback branch exists. The code still works; the why is now gone. Minor documentation loss.

4. exit-plan-mode.tsx — stale feedback input on cancel
   When a user opens the Request Changes input, types something, then clicks Cancel, feedback state is not cleared. Re-clicking Request Changes shows the stale text. This matches the UX of the surrounding component (AskUserQuestion has a similar gap) so it's a nit, not a regression.

Positive Highlights

• hitl-tools.ts is a clean extraction — eliminates three independent copies of the normalization logic that had drifted into ask-user-question.tsx, stream-message.tsx, and use-agent-status.ts.
• Correct UTF-8 byte truncation in _read_plan_file: slicing encoded bytes rather than characters handles multi-byte sequences properly. The prior revision that fixed this was the right call.
• Backend test coverage is solid — the new ExitPlanMode test case mirrors the existing AskUserQuestion pattern exactly, and case-insensitive variants are exercised for both tools.
• Runtime type guards in ExitPlanModeMessage (typeof input.planContent === "string", Array.isArray(input.allowedPrompts)) prevent crashes from unexpected backend payloads — consistent with defensive frontend patterns.
• JSON error logging in the adapter's plan-enrichment path (instead of a silent swallow) is an improvement surfaced by the review iteration.

Recommendations

1. Address the isHITLTool DRY nit if convenient — purely optional before merge.
2. Add a docstring note to _read_plan_file about the isolated-workspace assumption.

Confidence: High — backend and frontend changes follow established patterns; runner change is isolated to non-auth path.
CI: Mergify summary passing. Manual UI verification items remain open (plan approval flow end-to-end).