Skip to content

fix(tui): make mode authority decide approval prompts#3795

Merged
Hmbown merged 1 commit into
mainfrom
codex/0866-3790-mode-single-authority
Jun 29, 2026
Merged

fix(tui): make mode authority decide approval prompts#3795
Hmbown merged 1 commit into
mainfrom
codex/0866-3790-mode-single-authority

Conversation

@Hmbown

@Hmbown Hmbown commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Why

Fixes #3790. The selected mode should be the single authority for permission prompts: YOLO means zero approval prompts, while Agent and Plan keep the normal prompt or block behavior. The old auto-review safety floor could force prompts past YOLO for publish-like shell and MCP actions, and MCP/code-execution/hook ask paths still had mode-blind approval gates.

Change

  • Remove the auto-review safety floor that force-held publish, background destructive, and MCP actions past the active mode.
  • Gate MCP actions, code execution, and hook ask approvals on auto_approve, so YOLO does not emit approval requests for those paths.
  • Keep explicit deny/block rules as hard prohibitions in every mode.
  • Distinguish mode auto-approval from user approval in the approval decision/result stamp so model-visible output does not claim a user approved a prompt they never saw.

Validation

  • cargo fmt
  • cargo test -p codewhale-tui --bin codewhale-tui --locked yolo_mode_
  • cargo test -p codewhale-tui --bin codewhale-tui --locked auto_review
  • cargo test -p codewhale-tui --bin codewhale-tui --locked approval

Remove the auto-review safety-floor paths that forced prompts past the selected mode, including publish-like shell and MCP action holds. Gate MCP/code-execution/hook ask approvals on auto_approve so YOLO follows the zero-prompt contract while hard deny/block rules still refuse work in every mode.

Stamp mode auto-approvals separately from user approvals so tool results no longer claim the user approved a call they were never shown.

Fixes #3790.

Tested: cargo fmt; cargo test -p codewhale-tui --bin codewhale-tui --locked yolo_mode_does_not_prompt_for_mcp_action; cargo test -p codewhale-tui --bin codewhale-tui --locked yolo_mode_; cargo test -p codewhale-tui --bin codewhale-tui --locked auto_review; cargo test -p codewhale-tui --bin codewhale-tui --locked approval
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 6m 16s —— View job


PR Review: fix(tui): make mode authority decide approval prompts

  • Gather context and understand PR scope
  • Read changed files
  • Review for correctness bugs
  • Review for reuse/simplification opportunities
  • Review test coverage
  • Post final findings

Summary

This PR correctly unifies the authority for approval prompts behind the selected mode. The logic is sound, the new honest-stamping distinction (ApprovedByMode vs Approved) is propagated cleanly through the TUI event loop, and all safety-floor removals are correctly reflected in the tests. Below are findings in priority order.


P1 — Correctness

Non-bypassable tool stamping gap (main.rs, runtime_threads.rs)

rlm_eval is the only tool that returns true from registered_tool_requires_non_bypassable_approval, which means it emits Event::ApprovalRequired even when auto_approve = true. Two callers still use approve_tool_call (not approve_tool_call_by_mode) under auto_approve = true:

  • main.rs:7374 — headless event loop
  • runtime_threads.rs:3324 — API runtime auto-approve path
// main.rs:7372 – stamps ApprovedByUser even when auto-approving on mode authority
Event::ApprovalRequired { id, .. } => {
    if auto_approve {
        let _ = engine_handle.approve_tool_call(id).await;  // should be approve_tool_call_by_mode
    }
}

This contradicts the PR's honest-stamping goal: the tool result would claim "approved by the user" when it was never shown to anyone. The TUI path correctly calls approve_tool_call_by_mode; these two paths don't. Since rlm_eval is the only non-bypassable tool this affects today, the blast radius is narrow — but it's the exact case the PR was designed to fix.

Fix this →

Headless fail-fast regression (design note, not a panic)

Previously, ApprovalMode::Never + background destructive shell would Block at the auto-review step — an immediate failure returned to the model. Now it emits ApprovalRequired and the headless runner auto-denies via engine_handle.deny_tool_call. The model still sees a denial, but the failure now goes through the approval channel rather than the auto-review short-circuit. This is functionally equivalent in most contexts and well-documented in the test renames. Flagging so the change is deliberate.


P3 — Reuse / simplification

auto_review_force_prompt_overrides_auto_approve removal is clean. No dead code or duplicated logic introduced. The ToolApprovalStamp::AutoApprovedByModemodel_description() string is clear and specific — good choice.


P4 — Tests

Weak assertion in mcp_read_allows_and_mcp_action_is_not_held_by_policy (auto_review.rs:648)

assert_ne!(
    policy.evaluate(&action_ctx).action,
    AutoReviewAction::HoldForReview,
    "MCP actions are no longer held by the policy; the mode governs prompting"
);

This passes even if the result is Allow (which would silently skip the mode-based approval gate in the engine). The fallback for a destructive MCP action should now be AskUser. A tighter assertion:

assert_eq!(policy.evaluate(&action_ctx).action, AutoReviewAction::AskUser);

Fix this →

Coverage that's genuinely good:

  • auto_approved_by_mode_stamp_is_honest_and_not_attributed_to_the_user — exactly the right assertions (checks !contains("approved by the user"), contains("auto-approved"), and the metadata field).
  • yolo_mode_does_not_prompt_for_mcp_action — correctly panics on Event::ApprovalRequired, waits for ToolCallComplete, and lets the tool fail post-gate rather than pre-gate.
  • yolo_mode_runs_publish_like_shell_without_a_prompt — the switch from matching ApprovalRequired to asserting !saw_approval_prompt is the right inversion.

P5 — Security / trust model note

Removing the publish floor means YOLO now runs git push, cargo publish, npm publish, and gh release without any prompt. The PR body and comment chain document this explicitly (deferred to 0.8.67). Confirming this is intentional for Hunter's sign-off since it changes the YOLO threat model.

Block (deny) rules remain hard prohibitions in every mode — that invariant is preserved and tested.


Overall: Well-structured change with solid test coverage. The two approve_tool_callapprove_tool_call_by_mode gaps in the headless/runtime paths are the only unfinished seam relative to the stated goal. Everything else looks correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permissions are over-abstracted: make the MODE the single authority (rip out auto-review/Auto; defer to 0.8.67)

1 participant