Skip to content

feat(cli): add [cli] sandbox and approval_policy config defaults - #52

Merged
locez merged 8 commits into
locez:mainfrom
tsunheimat:feat/config-fully-trusted
Sep 15, 2026
Merged

locez merged 8 commits into
locez:mainfrom
tsunheimat:feat/config-fully-trusted

Conversation

@tsunheimat

@tsunheimat tsunheimat commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds a [cli] table to config.toml with two defaults applied to every merry invocation, so a preferred sandbox mode or approval policy does not need to be passed as a flag each time. The approval side replaces the earlier fully_trusted bool (and --fully-trusted) with an approval_policy whose values name who reviews a command, adds a deny policy, and makes model_then_human the default under every sandbox mode.

[cli]
sandbox = "no-sandbox"
approval_policy = "none"
merry --approval-policy human_only run "task"
Value Runtime mode Reviewer
none FullyTrusted nobody: configured actions run without an approval round (the old fully_trusted = true)
deny DenyAll (new) nobody: every permission request and high-risk action review is denied without asking; actions that need no approval still run
model_only Required the [models.approval_review] model; a model denial is final
model_then_human (default) ModelThenHostFallback the model first; when it denies or cannot decide, the TUI dialog or merry run prompt makes the final call
human_only HostDecisionOnly the TUI dialog or the merry run prompt
  • sandbox takes with-sandbox (outer+inner, the built-in default for TUI and run), no-sandbox, or inner-sandbox, named after the root flag each stands in for. Any other value, including a raw --no-sandbox, is a parse error that lists the accepted names.
  • approval_policy and --approval-policy <POLICY> share one ApprovalPolicy type (crates/merry-cli/src/coding/approval.rs, clap ValueEnum + serde, snake_case). --help lists the five values with a one-line description each. fully_trusted / --fully-trusted no longer parse.
  • sandbox and approval_policy are independent. The sandbox sets the execution boundary; the approval policy sets who reviews requests inside it. Every one of the 5×3 combinations is accepted in config and on the command line and resolves identically either way. Cli::apply_defaults merges each dimension on its own: a sandbox flag replaces only the configured sandbox, --approval-policy replaces only the configured approval_policy, and neither touches the other (so a configured none survives --with-sandbox, and --no-sandbox --approval-policy human_only works with approval_policy = "none" in config). MerryConfig::cli_defaults() is infallible; the CLI no longer derives or constrains one dimension from the other.
  • Runtime: new PermissionReviewMode::DenyAll, denied directly in review_permission_request and never satisfied by an existing grant. ModelThenHostFallback escalates a model denial (including an "approve" the model was not confident enough in) to the host instead of treating it as final; Required keeps a model denial final.
  • Typed host fallback reason. PermissionAdmissionContext / PermissionReviewRequest carry Option<HostFallbackReason> (ModelDenied(PermissionAdmissionReview), ReviewFailed { message }, ReviewModelUnavailable) instead of a review_failure string. The TUI dialog and the headless prompt print AI review denied: <rationale> for a model denial and AI review unavailable: … only for real failures; the original review metadata (source, risk, user authorization, rationale) travels with the request.
  • Coding layer: CodingTrustMode is replaced by CodingApprovalPolicy (default ModelThenHuman); CodingPermissionPolicy::for_process_boundary becomes for_approval_policy(policy, host_source). These types now live in merry-coding/src/runtime/permission_policy.rs. Host-reviewed policies still require a host admission source and fail with a typed error otherwise.
  • Removed: [permissions] no_sandbox_review, NoSandboxReviewMode, and CodingProcessBoundary. They only existed to derive a reviewer from the sandbox boundary, which the fixed default replaces; a config that still sets no_sandbox_review fails to parse with an unknown-field error.
  • examples/config.toml documents both keys; sandbox stays commented and approval_policy = "model_then_human" is spelled out as the shipped default (the example-config test asserts both). README has the reviewer table and the independence rule.

Human approval for a sandboxed merry run -

The outer bubblewrap child runs under --new-session, so it cannot open /dev/tty (ENXIO), and a - task has already consumed stdin. Previously a --with-sandbox … run - under model_then_human / human_only denied every request unanswered.

Now, only for that combination (Cli::hands_off_review_terminal: outer sandbox + run - + a policy that may ask a person), the parent resolves its controlling terminal's device node before the re-exec (tty_nr from /proc/self/stat, matched against /dev/pts and /dev by st_rdev) and adds --dev-bind <that node> /dev/merry-review-tty to the bwrap plan. The headless reviewer tries /dev/tty first and then that path, opened with O_NOCTTY. Properties:

  • --new-session is unchanged; /dev/tty stays unopenable inside, so the TIOCSTI/job-control isolation it provides is kept. The bound node is not the child's controlling terminal.
  • No file descriptor is inherited across the exec (the workspace forbids unsafe, and a bind needs no pre_exec/from_raw_fd), so there is nothing to leak to child processes.
  • Inner action sandboxes mount their own --dev /dev, so tool processes never see /dev/merry-review-tty.
  • --inner-sandbox / --no-sandbox runs keep using /dev/tty; policies that never ask a person get no bind at all.
  • A run with no terminal anywhere still denies each request explicitly; the stderr notice now names the ways out (task on argv, or --approval-policy model_only / deny).
  • Cancellable reads. The terminal is opened O_NONBLOCK | O_NOCTTY on a private file description and read through Tokio's reactor (AsyncFd) by a dedicated adapter, headless_review/terminal.rs. An unanswered prompt is a pending await that cancellation drops; nothing sits on the blocking pool, so HeadlessPermissionReviewTask::finish() followed by dropping the runtime returns promptly. A hung-up pseudo-terminal (EIO after the master closes) reads as end of input, which the reviewer turns into a denial. The operator's shell is unaffected because the non-blocking flag never touches an inherited description.
  • Platform gates. /proc/self/stat and device-number lookup are cfg(target_os = "linux") with a None branch elsewhere; the terminal adapter and its libc flags are cfg(unix) with a None branch elsewhere; the bound-device path is only tried on Linux; the PTY and bubblewrap tests (which use the Linux-only rustix dev-dependency) are cfg(all(test, target_os = "linux")).

Tests: a real bubblewrap + pseudo-terminal integration test (bound_terminal_answers_inside_a_new_session_sandbox_without_dev_tty) spawns bwrap --unshare-user --unshare-pid --new-session … --dev-bind <pts> /dev/merry-review-tty, confirms /dev/tty is closed inside, writes yes to the pty master, and reads answer=yes back through the bound device. cancelling_an_unanswered_terminal_review_denies_it_and_lets_the_runtime_stop runs the reviewer against a real PTY with a request outstanding and nobody answering, cancels it, asserts the request is denied, and asserts the runtime drops within a time bound (against the previous blocking read it fails at that assertion). The adapter tests cover typed lines followed by hangup and a pending read dropped by cancellation. Further tests cover tty_nr parsing, device lookup by number against a fresh pty, the plan (bind present only when handed off, after --dev /dev, --new-session intact, never /dev/tty), the terminal path fallback order, and the CLI predicate across all policies and modes.

Shell completions

merry completions <SHELL> (bash, zsh, fish, elvish, powershell) prints a completion script generated by clap_complete from the same clap definition that parses the flags, so --approval-policy and its five values, the sandbox flags, and every subcommand complete without a hand-kept list. main answers it right after parsing, before loading config.toml or touching the sandbox, so it works before Merry is configured or when the current config fails to load. README shows the install lines for zsh, bash, and fish.

Behavior notes

  • Default behaviour changes: previously the reviewer followed the sandbox mode (model under with-sandbox, model-then-host under inner-sandbox, host-only under no-sandbox). Now every mode defaults to model_then_human; set human_only to keep the old no-sandbox behaviour, model_only to keep the old with-sandbox behaviour.
  • Neither none nor deny changes any access ceiling. deny_paths, review_paths masking, and network = false apply exactly as before.
  • Config lives only in the user-trusted XDG config file. No project-local config path is involved.
  • A configured sandbox = "with-sandbox" also applies to merry debug, exactly as --with-sandbox would.
  • Breaking for existing configs: [cli] fully_trusted and [permissions] no_sandbox_review are now unknown-field parse errors. There is no deprecation alias in this revision.
  • File layout: cli.rs unit tests moved to cli/tests.rs (cli.rs is now ~230 lines); approval-policy types moved out of merry-coding/src/runtime.rs into runtime/permission_policy.rs.

Verification

  • cargo fmt --all
  • cargo check --workspace --all-targets and cargo clippy --workspace --all-targets --all-features -- -D warnings (only the pre-existing nonminimal_bool lint in tui/projector/tool_output.rs, untouched by this PR)
  • cargo test --workspace --no-fail-fast: every crate passes except the bubblewrap execution tests in merry-process and merry-cli that fail on this host with mount destination must be absolute (identical on the baseline). merry-runtime 728 passed, merry-coding 37 passed, merry-cli 615 passed / 5 known host-only failures.
  • Cross-target cargo check -p merry-cli for x86_64-pc-windows-gnu and aarch64-apple-darwin cannot complete on this host on the baseline either: it stops in the build scripts of ring and onig_sys (no cross C toolchain) before reaching CLI code. Platform gating was verified by inspection of every std::os::unix, libc, AsyncFd, /proc, rustix, and bubblewrap use in the touched files.
  • New/changed tests all pass, including the real bwrap + PTY tests above, config_and_flags_resolve_every_sandbox_and_approval_policy_combination_alike, cli_accepts_every_sandbox_and_approval_policy_combination, and the runtime assertions that the host receives HostFallbackReason::ModelDenied with the model's rationale and ReviewModelUnavailable when no review model is configured.
  • XDG_CONFIG_HOME=/nonexistent merry completions zsh prints a #compdef merry script listing --approval-policy with all five values.

Earlier revisions: https://claude.ai/code/session_01KdtdhgkdDC8Scm7hprRdzC

https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm

tsunheimat added a commit to tsunheimat/merry that referenced this pull request Sep 13, 2026
…ions

The first revision of locez#52 added a dedicated `[permissions] fully_trusted`
key. Replace it with a generic `[cli] default_options` setting so the same
mechanism also selects the sandbox mode, instead of growing one config
key per root flag.

`[cli] default_options` accepts a whitespace-separated string or an array
of root `merry` options and applies them to every invocation as if they
were written before the subcommand. Supported options are --with-sandbox,
--no-sandbox, --inner-sandbox, and --fully-trusted. Validation mirrors
clap: unsupported entries, repeated options, blank array entries, and more
than one sandbox mode are config errors reported at startup for every
command.

Explicit command-line flags take precedence: a sandbox mode on the command
line replaces the configured one, because the three modes are mutually
exclusive. --fully-trusted has no negating flag, so a configured default
enables it for every session. Cli::apply_default_options mutates the
parsed Cli before the sandbox bootstrap decision, so main, the outer
re-exec, and cli_route all observe the same effective flags; the sandboxed
child re-derives them from the same config.

The `[permissions] fully_trusted` key and MerryConfig::fully_trusted are
removed. examples/config.toml and README document the new table.

Verification: cargo fmt --all -- --check; cargo clippy -p merry-cli
--all-targets (only the pre-existing nonminimal_bool warning in
tui/projector/tool_output.rs); cargo test -p merry-cli: 603 passed, 5
failed, all five the known host-only sandbox tests (nested bwrap
namespace and "mount destination must be absolute") that fail identically
on origin/main. Manual smoke with a temporary XDG_CONFIG_HOME confirmed
the validation messages and that a configured --with-sandbox makes
`merry debug` attempt the outer re-exec.

Refs: locez#52

Claude-Session: https://claude.ai/code/session_01XGvvrrRLJvE1KuFSbFBx84
@tsunheimat tsunheimat changed the title feat(cli): add [permissions] fully_trusted config setting feat(cli): add [cli] default_options config setting Sep 13, 2026
tsunheimat added a commit to tsunheimat/merry that referenced this pull request Sep 13, 2026
Follow-up on locez#52: the previous revision accepted raw `--flag` strings in
`[cli] default_options` and re-parsed them against an allowlist of root
flag names. Replace that with four typed values that map directly to
modes, so config never carries command-line syntax.

`default_options` now takes one value or an array of `with-sandbox`,
`no-sandbox`, `inner-sandbox`, and `fully-trusted`. `CliDefaultOption` is
a serde enum, so an unknown value (including a raw `--no-sandbox`) is a
parse error that lists the accepted names. `CliDefaultOptions` resolves
the list to an optional ProcessExecutionMode plus a fully_trusted bool;
listing more than one sandbox mode or repeating a value is a config
error. Cli::apply_default_options sets the matching flag fields from the
resolved mode, with command-line flags still taking precedence and
fully-trusted applying to every session.

The flag-name allowlist (`cli::DefaultOption`) and its round-trip test
are removed. examples/config.toml and README describe the four values.

Verification: cargo fmt --all -- --check; cargo clippy -p merry-cli
--all-targets (only the pre-existing nonminimal_bool warning in
tui/projector/tool_output.rs); cargo test -p merry-cli: 601 passed, 5
failed, all five the known host-only sandbox tests that fail identically
on origin/main. Manual smoke with a temporary XDG_CONFIG_HOME confirmed
the unknown-variant message for a raw flag, the sandbox-mode conflict
error, that a configured with-sandbox makes `merry debug` attempt the
outer re-exec, and that inner-sandbox + fully-trusted runs normally.

Refs: locez#52

Claude-Session: https://claude.ai/code/session_01XGvvrrRLJvE1KuFSbFBx84
Users who always want a particular sandbox mode or fully trusted review
previously had to pass --with-sandbox/--no-sandbox/--inner-sandbox or
--fully-trusted on every TUI or `merry run` invocation. Add
`[cli] default_options` to config.toml as the persistent equivalent.

`default_options` takes one value or an array of four typed values that
map to modes: with-sandbox (outer+inner bubblewrap), no-sandbox
(unrestricted host), inner-sandbox (inner action sandbox only), and
fully-trusted (skip model/host permission review). `CliDefaultOption` is
a serde enum, so an unknown value, including a raw `--no-sandbox`, is a
parse error that lists the accepted names. `CliDefaultOptions` resolves
the list to an optional ProcessExecutionMode plus a fully_trusted bool;
listing more than one sandbox mode or repeating a value is a config
error reported at startup for every command.

Cli::apply_default_options sets the matching flag fields right after
config validation and before the sandbox bootstrap decision, so main,
the outer re-exec, and cli_route all observe the same effective flags;
the sandboxed child re-derives them from the same config. Explicit
command-line flags take precedence: a sandbox mode on the command line
replaces the configured one because the three modes are mutually
exclusive. fully-trusted has no negating flag, so a configured default
enables it for every session.

Fully trusted review still only skips model and host permission review.
Access ceilings, deny_paths, review_paths masking, and network = false
are unaffected. The key lives in the user-trusted XDG config only.
examples/config.toml documents the four values (commented, nothing
preselected; the example-config test asserts this) and README explains
precedence.

Verification: cargo fmt --all -- --check; cargo clippy -p merry-cli
--all-targets (only the pre-existing nonminimal_bool warning in
tui/projector/tool_output.rs); cargo test -p merry-cli: 601 passed, 5
failed, all five the known host-only sandbox tests (nested bwrap
namespace and "mount destination must be absolute") that fail
identically on origin/main. Manual smoke with a temporary
XDG_CONFIG_HOME confirmed the unknown-variant message for a raw flag,
the sandbox-mode conflict error, that a configured with-sandbox makes
`merry debug` attempt the outer re-exec, and that inner-sandbox +
fully-trusted runs normally.

Claude-Session: https://claude.ai/code/session_01XGvvrrRLJvE1KuFSbFBx84
@tsunheimat
tsunheimat force-pushed the feat/config-fully-trusted branch from 0e1c74a to 19669aa Compare September 13, 2026 16:32
Follow-up on locez#52: the combined `default_options` list mixed two
independent axes, the sandbox mode and the review mode, so it needed
conflict and repeat validation and an array for the common case of
setting both. Split it into one scalar, typed key per axis.

`[cli] sandbox` takes "with-sandbox", "no-sandbox", or "inner-sandbox",
named after the root flag each stands in for, and maps to
ProcessExecutionMode through a serde enum; any other value, including a
raw `--no-sandbox`, is a parse error listing the accepted names.
`[cli] fully_trusted` is a bool matching `--fully-trusted`. The `[cli]`
table still rejects unknown keys, so the old `default_options` key fails
to parse with a message naming the two supported keys.

MerryConfig::cli_defaults() is now infallible and returns CliDefaults;
Cli::apply_defaults sets the matching flag fields, with command-line
sandbox flags still taking precedence and fully_trusted applying to
every session. examples/config.toml and README describe both keys.

Verification: cargo fmt --all -- --check; cargo clippy -p merry-cli
--all-targets (only the pre-existing nonminimal_bool warning in
tui/projector/tool_output.rs); cargo test -p merry-cli: 600 passed, 5
failed, all five the known host-only sandbox tests that fail identically
on origin/main. Manual smoke with a temporary XDG_CONFIG_HOME confirmed
the unknown-variant message for a raw flag, the unknown-field message
for the old default_options key, that sandbox = "with-sandbox" makes
`merry debug` attempt the outer re-exec, and that inner-sandbox plus
fully_trusted = true runs normally.

Refs: locez#52

Claude-Session: https://claude.ai/code/session_01KdtdhgkdDC8Scm7hprRdzC
@tsunheimat tsunheimat changed the title feat(cli): add [cli] default_options config setting feat(cli): add [cli] sandbox and fully_trusted config defaults Sep 13, 2026
`[cli] fully_trusted` and `--fully-trusted` were hard to read: a bool named
after trust said nothing about who reviews commands otherwise. Replace both
with `approval_policy` / `--approval-policy <POLICY>`, one value per
reviewer:

- auto (default): derive the reviewer from the sandbox mode as before;
  `[permissions] no_sandbox_review` now applies only here
- model: model-backed review without a human fallback
- human: the TUI dialog or `merry run` prompt decides
- model-then-human: model first, human when the model cannot decide
- trusted: no approval round (the old fully trusted mode)
- none: reject every permission request without asking

`none` needs a runtime mode that neither approves nor consults a source, so
add `PermissionReviewMode::DenyAll`; it denies in `review_permission_request`
and never reuses an existing grant. The coding layer replaces
`CodingTrustMode` with `CodingApprovalPolicy` and dispatches on it in
`CodingPermissionPolicy::for_process_boundary`; explicit reviewer policies
ignore the boundary, `auto` keeps the previous derivation.

The `[cli]` invariants carry over: `approval_policy = "trusted"` still
requires `sandbox = "no-sandbox"` at load time, and a configured `trusted`
still yields to `--with-sandbox` / `--inner-sandbox` on the command line.
Every other configured policy applies under any sandbox mode, and an explicit
flag always wins.

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
@tsunheimat tsunheimat changed the title feat(cli): add [cli] sandbox and fully_trusted config defaults feat(cli): add [cli] sandbox and approval_policy config defaults Sep 14, 2026
… to model_then_human

Rename the approval policy values so each one says who reviews, using the
same snake_case spelling in config and on the command line:

- none             no approval needed; configured actions run unreviewed
                   (was "trusted")
- deny             reject every permission request (was "none")
- model_only       model review only (was "model")
- model_then_human model first, human when the model denies or cannot decide
                   (was "model-then-human")
- human_only       human review only (was "human")

Drop the "auto" value together with `[permissions] no_sandbox_review`: the
default is now `model_then_human` under every sandbox mode instead of a
reviewer derived from the sandbox boundary, so the derivation and its
no-sandbox knob have nothing left to decide. `CodingPermissionPolicy::
for_process_boundary` becomes `for_approval_policy`, and `CodingProcessBoundary`
and `NoSandboxReviewMode` are removed from the coding API.

`model_then_human` now also escalates a model denial to the host instead of
treating it as final: the model is a first pass under that policy, and the
person at the terminal makes the final call whenever the model does not
approve. The host prompt shows the model's rationale as the fallback reason.
`model_only` keeps a model denial final.

The `none` policy keeps the old `trusted` invariants: it requires
`sandbox = "no-sandbox"` in config and a configured `none` applies only
while the effective mode is unrestricted. `examples/config.toml` now spells
out `approval_policy = "model_then_human"` as the shipped default.

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
Generate bash, zsh, fish, elvish, and powershell completion scripts from the
same clap definition that parses `merry` arguments, so subcommands, flags,
and value lists such as `--approval-policy` stay in sync without a hand-kept
list.

`main` answers the subcommand right after parsing, before loading
`config.toml` or touching the sandbox, so the script prints even when Merry
is not configured yet or the current config fails to load. The route keeps a
matching arm so it stays total if that short-circuit ever moves.

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
Comment thread crates/merry-cli/src/cli.rs Outdated
///
/// Explicit command-line flags win. The three sandbox modes are mutually
/// exclusive, so a mode given on the command line keeps a configured
/// `sandbox` from applying. A configured `approval_policy = "none"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里不对吧, approval_policy 不是不与 sandbox 强相关么? approval 的语义不是在自己的授权范围内申请上限么?

@locez

locez commented Sep 14, 2026 •

Copy link
Copy Markdown
Owner

审查版本:262bdcb3ba4d22107fe1c29854f8ee7590c4b2cc,基线:e94f21a5de7fd7958a65a50b17d69270b3195d03。

整体分层方向合理,但建议修复下面两处问题后再合并。

  1. [P2] 审批策略与沙箱边界重新耦合,配置和命令行行为不一致。

    config/cli.rs:87–98 强制 approval_policy = "none" 搭配 sandbox = "no-sandbox";cli.rs:131–136 又在通过参数切换沙箱时丢弃配置中的 none,回到 model_then_human。

    实测:同样的 inner-sandbox + none,写进配置会报错,通过命令行则成功。另外,仅配置 approval_policy = "none" 时,即使显式传入 --no-sandbox --approval-policy human_only,也会在应用覆盖值之前被配置校验拒绝。

    这使 CLI 重新承担了 coding 层已经取消的策略推导。建议两个维度分别合并默认值与命令行覆盖:审批决定谁审核,沙箱决定执行边界。删除这两处跨维度约束,并同步修正说明及测试;补充五种策略与三种沙箱的组合测试,验证配置和参数等价、覆盖只影响对应维度。

Locez: 这里直接按照2个参数互相独立

  1. [P2] 外层沙箱中的 run - 无法完成新增的人工审批。

    run.rs:255–260 现在允许所有沙箱模式选择人工审批。任务从 stdin 读取后,审批依赖 headless_review.rs:98–105 打开 /dev/tty,但外层启动使用的 --new-session 会断开控制终端。

    PTY 实验确认:父进程能打开 /dev/tty;bubblewrap 不使用 --new-session 时子进程也能打开,使用该参数后返回 ENXIO。结合现有输入选择路径,merry --with-sandbox --approval-policy human_only run - 中需要审批的请求会因无输入渠道被拒绝,默认 model_then_human 的人工回退也受影响。此前外层沙箱固定使用 model_only,本次新增的策略组合使这条人工输入路径需要真正可用。

Locez: run 这里看看是否可以解决,如果不可以,感觉显示拒绝就好了

建议在宿主侧建立专用审批通道,或在外层 re-exec 前打开并受控传递审批所需的文件描述符,保留 --new-session 的隔离行为;增加“stdin 任务 + 外层沙箱 + 人工审批”的真实 PTY 集成测试。

职责与代码复用

  • 配置和参数共享 serde/clap 类型;策略映射由 merry-coding 负责,审批执行由 runtime 负责,方向合理。
  • DenyAll 接入已有审批入口;父子 runtime 继续复用同一策略组合。
  • 没有新增反向依赖,也没有改变模型可见的工具定义与顺序。
  • CLI 的 ApprovalPolicy 与 coding 层枚举属于合理的边界转换,无需为了减少枚举而把 clap 引入 coding 层。

文件大小与可维护性

本次修改的 Rust 文件均低于仓库的 1000 行约束,建议按职责拆分。

文件 基线 → 当前行数 建议
merry-cli/src/cli.rs 498 → 750 约 523 行内联测试,适合移到 cli/tests.rs
merry-coding/src/runtime.rs 761 → 753 审批策略、错误及映射可以独立成模块
merry-runtime/src/permission.rs 816 → 824 合同与 channel 桥接可后续拆分,主要是既有结构
merry-runtime/src/runtime/permission_execution.rs 446 → 464 职责集中,目前无需拆分
新增审批、配置、补全模块 73 / 105 / 69 大小合适

另一个非阻断改进点是使用明确类型表示人工回退原因。permission_execution.rs:411–415 把模型拒绝装进 review_failure 字符串,导致 headless 提示 仍显示 AI review unavailable。建议区分“模型拒绝”和“审核失败”,并保留原始审核信息。

Locez:这个处理一下

验证结果

  • cargo fmt --all --check、cargo clippy --locked --offline --all-targets --all-features -- -D warnings、Cargo metadata、git diff --check 均通过。
  • cargo test --locked --offline --all:2132 通过、0 失败、9 项按现有标记忽略。其中 7 项为回环 TCP 测试,2 项需真实模型;没有额外启用这些忽略项。
  • CLI 单元测试 614 项通过。初次受限环境中的 4 项失败涉及 /var/tmp 只读及 NETLINK_ROUTE 限制,在主机环境重跑后全部通过。
  • 配置/参数行为与 PTY 终端实验结果如上;错误配置下的 completions bash 也已验证正常输出。
  • Python 本地未重跑;已核对该 PR 的六组 Python CI 均通过。

上述测试通过并不能覆盖前两项发现:现有配置测试固化了当前耦合规则,PTY 审批组合则缺少对应集成覆盖。

…al for sandboxed run -, type the host fallback reason

Review follow-up for PR locez#52.

approval_policy and sandbox are now independent dimensions. `[cli]`
config no longer rejects `approval_policy = "none"` without
`sandbox = "no-sandbox"`, `MerryConfig::cli_defaults` is infallible, and
`Cli::apply_defaults` applies a configured policy under every sandbox
mode; a flag on one dimension replaces only that dimension's default.
Config and flags resolve every 5x3 combination identically (new matrix
tests in cli/tests.rs and config/tests/cli.rs).

A sandboxed `merry run -` can now complete human approval. The outer
bubblewrap child runs under `--new-session`, so it cannot open
`/dev/tty`, and a `-` task has already consumed stdin. When such a run
uses `model_then_human` or `human_only`, the parent resolves its
controlling terminal's device node (tty_nr from /proc/self/stat) before
the re-exec and `--dev-bind`s only that node at /dev/merry-review-tty;
the headless reviewer opens it with O_NOCTTY after /dev/tty fails.
`--new-session` is kept, no descriptor is inherited, and inner action
sandboxes mount their own /dev, so tool processes never see the node.
A real bubblewrap+PTY test proves /dev/tty stays closed while the answer
arrives through the bound device. Runs with no terminal at all still
deny each request, now with a notice naming the alternatives.

The host fallback reason is a typed `HostFallbackReason`
(`ModelDenied(PermissionAdmissionReview)`, `ReviewFailed`,
`ReviewModelUnavailable`) instead of a string, so the TUI and headless
prompts say "AI review denied: <rationale>" for a model denial and keep
"AI review unavailable" for actual failures; the original review
metadata travels with the request.

Also moves the cli.rs unit tests to cli/tests.rs and the coding
approval-policy types to merry-coding/src/runtime/permission_policy.rs.

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
@tsunheimat

Copy link
Copy Markdown
Contributor Author

Addressed in 601ef61 (all three points, plus the file-layout suggestions).

1. approval_policy 与 sandbox 完全独立。 删除了 config/cli.rs 的 none ⇒ no-sandbox 校验(cli_defaults() 现在不返回错误)和 Cli::apply_defaults 里丢弃 none 的跨维度分支。现在配置和参数各自只合并自己的维度:sandbox 参数只替换配置中的 sandbox,--approval-policy 只替换配置中的 approval_policy。新增 5 种策略 × 3 种沙箱的组合测试:config_and_flags_resolve_every_sandbox_and_approval_policy_combination_alike(配置与参数等价)、command_line_sandbox_flag_leaves_the_configured_approval_policy_alone、explicit_approval_policy_flag_leaves_the_configured_sandbox_alone(覆盖只影响对应维度)、cli_accepts_every_sandbox_and_approval_policy_combination(配置层)。README / examples/config.toml 已同步。

2. 外层沙箱中的 run - 人工审批:可以解决,已实现。 保留 --new-session。仅当「外层沙箱 + run - + model_then_human/human_only」时,父进程在 re-exec 前通过 /proc/self/stat 的 tty_nr 解析出控制终端的设备节点,然后只把这一个节点 --dev-bind 到沙箱内的 /dev/merry-review-tty;子进程 /dev/tty 打不开后改用该路径(O_NOCTTY)读取回答。没有采用传递 fd 的方式:workspace forbid(unsafe_code),而 pre_exec/from_raw_fd 都需要 unsafe,且 bind 方式没有 fd 泄漏到子进程的问题。内层 action sandbox 自己挂 --dev /dev,所以工具进程看不到该节点;/dev/tty 在沙箱内仍不可打开,--new-session 的隔离效果不变。新增真实 bubblewrap + PTY 集成测试 bound_terminal_answers_inside_a_new_session_sandbox_without_dev_tty(验证 /dev/tty 关闭、答案经 bind 设备到达),以及 plan / 设备查找 / CLI 判定的单元测试。完全没有终端时仍按请求显式拒绝,stderr 提示现在给出替代方案(任务放 argv,或 model_only / deny)。

3. 人工回退原因改为明确类型。 新增 merry_runtime::HostFallbackReason { ModelDenied(PermissionAdmissionReview), ReviewFailed { message }, ReviewModelUnavailable },替换 review_failure: Option<String>;模型拒绝时保留完整的 PermissionAdmissionReview。TUI 对话框与 headless 提示对模型拒绝显示 AI review denied: <rationale>,只有真正失败才显示 AI review unavailable。runtime 测试断言宿主收到 ModelDenied 及原始 rationale,以及未配置审批模型时收到 ReviewModelUnavailable。

文件拆分: cli.rs 的测试移到 cli/tests.rs(cli.rs 约 230 行);CodingApprovalPolicy / CodingPermissionPolicy / 其错误类型移到 merry-coding/src/runtime/permission_policy.rs。

PR 描述已更新。

Brings in locez#51 (fail closed when bubblewrap cannot nest).

The only conflict was in `crates/merry-cli/src/main.rs`, where both
sides inserted a block between the bubblewrap availability check and
`sandbox::maybe_reexec`. Both are kept: the nested-sandbox probe runs
first, so a host that cannot nest stops startup before anything else,
and the review terminal is resolved after it, right before the re-exec
that needs it.

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
@locez

locez commented Sep 15, 2026

Copy link
Copy Markdown
Owner

复审版本:2f5b680f2d18f4c012f52bf367ca64dc256a55fc,当前基线:d7144104459f92ce9fb283630e9eaadd766dae10。已检查本次修复提交及相对当前 main 的完整差异。

上次要求的配置解耦、类型化回退原因和模块拆分已落实。整体分层方向合理,建议补齐下面两处问题后合并。

  1. [P2] 新增终端通道仍使用不可取消的阻塞文件读取,reviewer 结束后进程可能无法退出。

    headless_review.rs:122 返回 tokio::fs::File,随后通过 BufReader::read_line() 等待终端输入。终端保持打开、操作者尚未回答时,取消分支 只能放弃等待 future,不能停止 Tokio blocking pool 中实际执行的终端读取。因此 HeadlessPermissionReviewTask::finish() 可以返回,但 main 销毁 Tokio runtime 时仍会等待该读取完成。运行在等待审批期间取消或因输出错误提前收尾,就可能卡在退出阶段。

    验证使用直接引用生产 headless_review.rs 的独立探针:创建 PTY,经 bwrap --new-session --dev-bind 暴露给 reviewer,stdin 关闭,发起审批后保持不回答,再取消并调用 finish()。观测顺序为 reviewer_finish_returned → before_runtime_drop,进程仍未退出;向 PTY 写入换行后才出现 runtime_drop_completed 并正常退出。探针只替换了绑定路径常量,没有改动生产读取或关闭逻辑。

    建议在 CLI 的终端适配层拥有可取消的非阻塞读取,例如通过安全的 FD API 配合 AsyncFd,并保持 O_NOCTTY。补充真实 PTY 的“等待输入期间取消 → reviewer 收尾 → 进程在限定时间内退出”测试,以及终端关闭路径。当前新增测试先写入 yes 再读取,覆盖不到此问题。基线的 /dev/tty 读取也有相同缺陷;本次增加的绑定终端路径继续沿用该模型,将问题带入了新打通的外层沙箱人工审批场景。

  2. [P2] Linux 专用终端实现和测试缺少平台边界。

    sandbox/review_terminal.rs:19 无条件导入 std::os::unix,模块也被 sandbox.rs 无条件编译;headless_review.rs:126 无条件引用仅在 Unix 声明的 libc。这给 Windows CLI 构建引入了不可用的依赖和 API,即使不启用沙箱也无法避开编译。

    此外,review_terminal.rs:87 的测试模块只有 cfg(test),却使用仅在 Linux dev-dependencies 中声明的 rustix,并直接执行 Linux bubblewrap 测试;macOS 编译 CLI 测试也会遇到缺失依赖。

    建议将 /proc、设备查找及 bubblewrap/PTY 测试限制在 Linux 实现中,为其他平台提供明确的不可用分支;Unix 终端打开逻辑也应加对应条件。检查 cargo check -p merry-cli 的平台构建与 macOS 的 CLI 测试编译。此项依据源码与 Cargo 目标依赖确认;本地仅有 Linux Rust 标准库,未实际运行跨平台编译。现有 macOS/Windows CI 构建的是 Python binding,不能覆盖 CLI 的这些错误。

职责、复用和 Rust 实践的复查结果:

  • sandbox 与 approval_policy 现在分别合并配置默认值和命令行覆盖,五种审批策略与三种沙箱组合有对应测试,符合两个维度独立的要求。
  • 审批策略组合留在 merry-coding,admission 执行与状态留在 runtime,终端设备适配留在 CLI;没有新增反向依赖或第二套权限状态。CLI 的 clap/serde 枚举与 coding 枚举之间的显式转换是合理边界。
  • HostFallbackReason 区分模型拒绝、审核失败和模型缺失,并保留原始审核信息;TUI 与 headless 复用其展示,消除了上次的字符串语义混用。父子 runtime 继续复用同一策略组合。
  • 本次没有修改模型可见的工具定义或顺序,没有发现因本次策略切换产生的额外缓存前缀变化。

本次修改的 36 个 Rust 文件均低于 1000 行;关键模块的规模与职责基本匹配:

文件 当前行数 判断
merry-cli/src/cli.rs / cli/tests.rs 238 / 578 参数入口与测试已分开,合理
merry-coding/src/runtime.rs / runtime/permission_policy.rs 612 / 156 策略拥有独立模块,职责更清楚
merry-cli/src/sandbox/review_terminal.rs 219 范围集中;需要完善平台与资源生命周期边界
merry-cli/src/headless_review.rs 504 规模可接受;可取消终端读取应由专用适配器承担
merry-runtime/src/permission.rs 867 最接近上限;后续扩展时可将审核合同与 channel 桥接按职责拆分,目前不作为阻断项

没有发现需要为了减少几行重复而新增跨层抽象的情况。后续拆分应围绕终端资源管理、审核合同、channel 通信这些真实职责展开。

验证:

  • cargo fmt --all --check、cargo clippy --locked --offline --all-targets --all-features -- -D warnings、Cargo metadata、git diff --check 均通过。
  • cargo test --locked --offline --all:2143 通过、0 失败、9 项按现有标记忽略;其中 CLI 单独验证 622 项通过。忽略项包括 7 项回环 TCP 测试及 2 项真实模型测试,未额外启用。
  • 新增 bubblewrap/PTY 正向测试通过;上述取消与 runtime 退出探针确认了测试套件未覆盖的生命周期缺陷。
  • 最新提交的 8 项 GitHub CI 均通过。Python/Web 本地未重跑;Rust 构建复用了现有 Web 产物,已核对本次没有 Web 源码改动。
  • 审查在独立临时 checkout 中完成,未修改生产源码;原工作区保持干净。本轮形成了可复现的合并前问题与已解决项证据,未变更 ROADMAP 或推进其他交付里程碑。

…e review terminal

Second-round review follow-up for PR locez#52.

Terminal answers for a stdin task were read through tokio::fs::File,
whose reads run on the blocking pool. Cancelling the reviewer while an
answer was outstanding dropped the future but not the read, so dropping
the runtime in main waited for a keystroke that never came. The
terminal is now opened O_NONBLOCK | O_NOCTTY on a private file
description and read through tokio's reactor (AsyncFd) by a dedicated
adapter in headless_review/terminal.rs; an unanswered read is a pending
await that cancellation simply drops. A hung-up pseudo-terminal (EIO
after the master closes) reads as end of input, which the reviewer
already turns into a denial. New tests cover typed lines followed by
hangup, a pending read dropped by cancellation, and the full lifecycle
on a real PTY: request outstanding, reviewer cancelled, request denied,
runtime dropped within a time bound. Against the previous blocking read
that last test fails at the shutdown assertion.

The Linux-only pieces are gated: /proc and device-number lookup in
sandbox/review_terminal.rs are cfg(target_os = "linux") with a None
branch elsewhere, the terminal adapter and its libc flags are cfg(unix)
with a None branch elsewhere, the bound-device path is only tried on
Linux, and the PTY/bubblewrap tests (which need the Linux-only rustix
dev-dependency) are cfg(all(test, target_os = "linux")).

Claude-Session: https://claude.ai/code/session_016kj3E1CjQtLCASyAW8WKzm
@tsunheimat

Copy link
Copy Markdown
Contributor Author

Both points addressed in 6315886.

1. 终端读取改为可取消的非阻塞读取。 新增专用适配器 crates/merry-cli/src/headless_review/terminal.rs:以 O_NONBLOCK | O_NOCTTY 打开终端(新的私有 file description,不影响操作者 shell 的 stdin),通过 Tokio reactor(AsyncFd<File>,安全 API,无 unsafe)实现 AsyncRead。未回答的读取只是一个 pending await,取消时直接丢弃,blocking pool 上不再有阻塞的终端读取,finish() 后 drop(runtime) 立即返回。终端关闭路径:pty master 关闭后 slave 返回 EIO,映射为 EOF → reviewer 走既有 InputEnded 拒绝分支。

新增真实 PTY 测试 cancelling_an_unanswered_terminal_review_denies_it_and_lets_the_runtime_stop:起 reviewer 指向 pty slave,发起审批且不回答,finish() 取消 → 断言请求被拒绝(rationale headless permission review was cancelled)→ 在独立线程 drop runtime,断言 10 秒内完成。已验证该测试对旧的 tokio::fs::File 实现失败(正是在 runtime 退出断言处超时),对新实现通过。另有适配器单元测试:逐行读取后 master 挂断读到 EOF;等待期间 timeout 丢弃 pending 读取。Stdin 通道(argv 任务)仍用 tokio::io::stdin(),这是基线行为,且 stdin 是继承的 description,不能对其设置 O_NONBLOCK,本次未改动。

2. 平台边界。 sandbox/review_terminal.rs 中 /proc/self/stat 解析与设备号查找为 cfg(target_os = "linux"),其他平台 resolve_controlling_terminal() 返回 None;终端适配器及 libc 常量为 cfg(unix),非 Unix 平台 open_review_terminal() 返回 None(按请求显式拒绝);/dev/merry-review-tty 候选路径仅在 Linux 加入;PTY/bubblewrap 测试(依赖仅 Linux 的 rustix dev-dependency)为 cfg(all(test, target_os = "linux"))。

跨平台编译说明:本机为 Windows/macOS target 跑 cargo check -p merry-cli 在基线(origin/main)上就停在 ring / onig_sys 的 build script(缺跨平台 C 工具链),到不了 CLI 代码,无法给出实际编译证据;已逐项核对改动文件中所有 std::os::unix / libc / AsyncFd / /proc / rustix / bwrap 引用均在对应 cfg 之下。

本地:fmt / clippy(仅既有 nonminimal_bool)通过;merry-cli 620 通过、5 项为既有的主机相关失败。PR 描述已更新。

@locez
locez merged commit d08884a into locez:main Sep 15, 2026
8 checks passed
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.

2 participants