feat(runtime): per-agent file_policy with deny/prompt/read/write tiers across all read+write tool sites#1183
Closed
benhoverter wants to merge 27 commits into
Closed
Conversation
Standalone crate exposing OpenFang's tool surface to MCP clients (primarily
Claude Code subprocesses) over stdio. Per architectural decision in ANAI-22:
not folded into openfang-runtime — keeps the protocol adapter out of the
kernel/compactor blast radius and the dep graph clean.
This commit is scaffolding only:
* Cargo manifest with rmcp 1.x (server, transport-io, macros)
* lib.rs: ToolDispatcher seam trait (runtime-implements, bridge-consumes,
one-way dep), ToolDispatchError enum, Bridge struct wrapping an
Optional<Arc<dyn ToolDispatcher>>, single stub `ping` tool
* main.rs: stdio MCP server entrypoint, tracing -> stderr (stdout is the
transport), no dispatcher attached
* Workspace members updated
Identity is bound at Bridge construction time, not per-call — the security
invariant tracked by ANAI-31. Real tool surface mapping lands in ANAI-30.
cargo check -p openfang-mcp-bridge: clean.
cargo check --workspace: clean (pre-existing imap-proto future-incompat
warning unrelated).
Refs: ANAI-22, ANAI-29
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the daemon-side foundation for the MCP bridge per the ANAI-30 plan
(topology 1b: daemon → CC → bridge → unix socket → daemon dispatcher).
- New `protocol` module in openfang-mcp-bridge: Frame/Hello/HelloAck/
CallRequest/CallResponse types with length-prefixed JSON framing
(1 MiB cap, 4-byte BE length prefix). Gated by `ipc-codec` feature
so type-only consumers can drop the tokio io traits.
- New `bridge_ipc` module in openfang-api: BridgeIpcServer binds
<home_dir>/run/bridge.sock (0600), accept loop with graceful
shutdown via Notify, per-connection Hello validation and CallRequest
→ CallResponse loop.
- run_daemon spawns the listener; failure is non-fatal (HTTP keeps
serving; bridge just unavailable). Socket file removed on shutdown.
Step 1 stub: the dispatcher returns CallResult::Error
("not yet wired"). Step 2 replaces this with a call into
openfang_runtime::tool_runner::execute_tool, scoped to the four-tool
allowlist (file_read, file_list, agent_list, channel_send). Identity
binding + token-table auth land in ANAI-31.
Tests: 3 protocol roundtrip tests + 4 IPC handler tests
(handshake/dispatch end-to-end via tempfile socket, version mismatch
rejection, empty-token rejection).
Refs ANAI-30, ANAI-22.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the step-1 stub in `BridgeIpcServer` with a real call into
`openfang_runtime::tool_runner::execute_tool`, mirroring the argument
bundle used by the HTTP /mcp endpoint in routes.rs.
- Added ALLOWED_TOOLS allowlist: file_read, file_list, agent_list,
channel_send. Rejection happens at the protocol layer (CallResult::Error)
before any kernel touch.
- Added dispatch_call(): snapshots the skill registry, builds a
KernelHandle from Arc<OpenFangKernel>, and invokes execute_tool.
- ToolResult mapped to CallResult::Ok { content, is_error }, preserving
the Ok/Error distinction (Error = bridge couldn't dispatch; Ok with
is_error = tool ran but returned an error).
- Identity stub: caller_agent_id taken at face value from
CallRequest::agent_id. Real per-spawn token-bound identity lands in
ANAI-31.
Test: ipc_handshake_and_allowlist_gate verifies wire shape end-to-end:
disallowed tool gets allowlist Error, allowed tool gets Ok response. Real
execute_tool integration tests come once the daemon spawns the bridge
for real (ANAI-31).
…l surface
Replaces the stub `ping` tool with the four ANAI-30 tools (file_read,
file_list, agent_list, channel_send) and wires the bridge binary to forward
each `tools/call` over the daemon IPC socket established in step 1.
Library (lib.rs):
- ToolDispatcher::call now returns DispatchOk { content, is_error }
preserving the tool-error-vs-dispatch-error distinction across the seam
- built_in_tools() declares the four-tool slice; schemas mirror
runtime::tool_runner::builtin_tool_definitions() (kept in lockstep)
- Bridge: manual ServerHandler impl (drops the #[tool_router] macro). Filters
advertised tools by intersecting built_in_tools() with
ToolDispatcher::allowed_tools(); double-checks before dispatch
- Bridge::new now requires a dispatcher (was Option<_>)
Binary (main.rs):
- Reads OPENFANG_BRIDGE_SOCKET / TOKEN / AGENT_ID env vars (last is stub for
ANAI-30; ANAI-31 derives identity from token)
- Connects to daemon, performs Hello/HelloAck handshake, exits on rejection
- IpcDispatcher: bridge-side ToolDispatcher impl. Forwards each call via mpsc
to an actor task that owns the stream; correlation-by-request_id with a
PendingMap<u64, oneshot> so concurrent tools/call invocations don't
serialize at the dispatcher layer
- Reader task drains pending oneshots with an error on connection close so
in-flight calls don't hang; production path exits the process so CC
notices and tears down (gated behind cfg(not(test)))
Tests:
- lib: built_in_tools_has_anai30_slice, permitted_tools_intersects_with_dispatcher_allowed
- main: ipc_dispatcher_round_trip_and_correlation — fake daemon listener,
full handshake, two concurrent calls, verifies per-id correlation and the
NotPermitted gate
Workspace check clean. Daemon-side bridge_ipc tests still pass (4/4).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
End-to-end topology now exists at the type level:
daemon → claude (per-prompt) → openfang-mcp-bridge → IPC → daemon
- Add `caller_agent_id: Option<String>` to CompletionRequest. Plumbed
through all construction sites; agent_loop populates it with
session.agent_id, everywhere else passes None.
- Daemon (`server.rs::run_daemon`): after BridgeIpcServer starts,
publish OPENFANG_BRIDGE_SOCKET and OPENFANG_BRIDGE_BIN as process env
for subprocess drivers to discover. Bridge bin defaults to a sibling
of current_exe; operators can override with OPENFANG_BRIDGE_BIN. Both
set with `unsafe` (edition 2024) but only during single-threaded
daemon startup, before any subprocess spawns.
- BridgeIpcServer gains `socket_path()` accessor.
- ClaudeCodeDriver: per-spawn `try_build_bridge_mcp_config`. When
caller_agent_id is set AND both discovery env vars are present,
generate a UUID token, write `<home>/run/cc-mcp-<uuid>.json` (0600),
and add `--mcp-config <path> --strict-mcp-config` to the claude args.
RAII guard removes the file on drop so per-spawn token lifetime is
bounded by the CC subprocess.
- apply_env_filter extended to strip OPENFANG_BRIDGE_* from CC's child
env. Bridge gets these only via the explicit `env` map in the
mcp-config — CC inheriting them would risk a stray bridge picking up
the daemon socket without a fresh per-spawn token.
- Tests:
- test_build_bridge_mcp_config_shape — verifies wire shape claude
expects: mcpServers.openfang.{command,args,env} with exactly the
three discovery vars in env (no extras to leak state).
- test_apply_env_filter_strips_bridge_discovery_vars — confirms
filter removes all four bridge vars from CC's child env.
- test_bridge_mcp_config_drop_removes_file — RAII cleanup invariant.
Stub points still flagged: token validated as non-empty (ANAI-31
replaces with daemon-issued per-spawn token table); agent_id taken
in-band from CallRequest (ANAI-31 derives from token).
11 CC driver tests pass. bridge_ipc (4) and bridge crate (6) tests
unchanged. Workspace check clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…NABLED
Default-off kill switch so we can deploy the bridge code path without
inlining it into every CC invocation. When the gate is unset or not in
{1, true}, try_build_bridge_mcp_config returns None and CC is spawned
exactly as it was pre-step-4 — no --mcp-config, no temp file, no bridge
child. Validation flow: deploy with gate off (sanity), launchctl setenv
OPENFANG_BRIDGE_ENABLED 1, bounce daemon, observe; if anything regresses,
flip back to 0 and bounce for instant recovery.
Daemon still starts the IPC listener and publishes BRIDGE_SOCKET/BIN env
unconditionally — both are harmless without a bridge child connecting.
Pure additive switch; zero behavior change when off.
Test exercises the full truth table for bridge_enabled() (unset, truthy
variants, falsy/garbage variants) and confirms the gate suppresses
config generation regardless of other env. Single test owns the global
env var so no serial_test infra needed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bridge IPC handshake works standalone (bridge binary connects + Hello/HelloAck ok against the live socket), and the daemon-side `wired CC --mcp-config for OpenFang bridge` debug line confirms the flag is being passed to claude. But no `bridge IPC accepted connection` events ever fire — meaning claude is launched with `--mcp-config` but isn't spawning the MCP server subprocess. Without `--debug`, claude swallows MCP launch errors silently. And we drop CC's stderr on success spawns, so any silent rejection is invisible. Add (both spawn paths): - `--debug` flag when bridge config is wired, so MCP errors print to stderr. - Always log a 4 KB tail of CC stderr at info when bridge_wired, regardless of success/failure. Streaming path now drains stderr concurrently to avoid pipe deadlock under chatty --debug output. Existing 12/12 claude_code unit tests still pass; workspace check clean. Diagnostic only — once the cause is identified we'll pare back to bounded on-demand logging.
- bridge_ipc: promote handshake/dispatch events to INFO and add an `accepted connection` log on accept. Operators can now observe the full bridge lifecycle from daemon stderr without crawling through ~/.claude/debug/<uuid>.txt. - claude_code driver: gate --debug + the 4 KB CC-stderr-tail diagnostic behind a new OPENFANG_BRIDGE_DEBUG env var (off by default). With proper INFO logs daemon-side, the noisy --debug output and the per-spawn ~/.claude/debug/ files are no longer load-bearing. - server: validate operator-supplied OPENFANG_BRIDGE_BIN path at boot and log the resolution outcome (override vs. probe). Catches deploy ordering bugs where the env points at a binary that doesn't exist. Stderr is still drained concurrently in the streaming path — required whenever --debug might be on, cheap when it isn't.
Relocate builtin_tool_definitions() from runtime::tool_runner to
openfang_types::tool::registry as the single source of truth. Bridge
now derives its advertised surface from the registry, filtered by a
substrate-level BRIDGE_DENY allowlist (currently empty).
CC sees the full kernel surface; per-agent gating remains agent.toml.
web_fetch and web_search are no longer carved out — treat CC as an
API model: tools route through OpenFang, not hidden channels.
- openfang-types::tool: tool.rs → tool/{mod,registry}.rs
- tool_runner re-exports builtin_tool_definitions for callsite stability
- openfang-mcp-bridge: adds openfang-types dep (types-only, runtime-free
invariant preserved); built_in_tools() is now ~7 lines
- Tests: drift sentinel for BRIDGE_DENY, full-surface assertion,
ANAI-32 canonical-nine sanity (8/8 passing)
Validated end-to-end against live daemon: file_list, file_read,
agent_list, memory_recall, web_fetch, file_write all round-trip.
Lift the pure-syntactic shell validators -- metacharacter denylist and exec_policy allowlist -- out of the per-tool match arm in execute_tool and run them BEFORE the approval gate. Without this, denied commands were sent for human approval, approved, and only then rejected by the metachar denylist inside the per-tool arm. That wasted operator attention on commands guaranteed to fail. Validators remain inside the shell_exec arm as defense-in-depth. Scoped to shell_exec only -- not all is_shell_tool entries. process_start has a different input shape and its own validators. Widening the pre-gate is a separate change. For the deferred path -- commands that clear the pre-gate but still need human approval -- to reach a human, add the missing push surface. ApprovalManager. Tokio broadcast of Submitted / Resolved / TimedOut lifecycle events, plus a subscribe API. Lag-tolerant. Slow subscribers get RecvError::Lagged and resync via list_pending. Tracing now includes agent_id, tool_name, risk, and decided_by on every lifecycle line. channel_bridge. Spawn an approval surfacer that consumes those events, resolves the agent bindings via the registry, and pushes a formatted prompt to the most-specific bound channel and channel_id. Submission prompts include short id, agent, tool, risk, action summary -- truncated -- and timeout, with /approve and /reject hints. Resolved and TimedOut events post a follow-up so the prompt is not left dangling. Tests added on the approval side cover Submitted+TimedOut and Resolved event delivery, plus UTF-8-safe log truncation. Validated live on the daemon -- tests A through F. Metachar denial is synchronous, no approval burned. Allowlist match on argv0 basename clears without prompting. Approval surfacer delivers prompts to the bound channel for commands that fall through to approval.
…cy (dependency: ApprovalManager + capability gate)
Introduces a unified [file_policy] block in OpenFangConfig with optional
per-agent override on AgentManifest. This is the foundation for gating
all filesystem access (MCP file tools, shell commands) through one
policy regardless of vector.
This commit lands the schema, serde wiring, and evaluator only — no
enforcement seams yet. MCP tools and the shell extractor follow in
subsequent commits.
- New crates/openfang-types/src/file_policy.rs:
* FilePolicy (config) and CompiledFilePolicy (runtime)
* DefaultTier { Deny, Prompt, ReadOnly }
* FileOp { Read, Write }, FileDecision { Allow, Prompt, Deny }
* Precedence: deny > prompt > write > read > workspace > default
* Tilde expansion at compile time (dirs::home_dir)
* Field-by-field merged_over for per-agent override
* 12 unit tests covering precedence, tier behavior, glob errors,
workspace boundary (substring-safe), tilde expansion
- Wired file_policy into OpenFangConfig (global) and AgentManifest
(per-agent override).
- globset added to workspace deps and openfang-types.
Branch base: origin/main with feat/anai-32-capability-enforcement
merged in as the dependency carrier (ApprovalManager etc.).
Must-fix: - Read-paths inside workspace no longer silently allow writes. When a path matches read_paths and op = Write, evaluator skips the workspace fallback and falls through to the default tier. Regression test added. API tightening (cheaper before step 3 multiplies call sites): - Workspace bound at compile time. FilePolicy::compile(workspace) returns CompiledFilePolicy. Evaluate signature is now (path, op), no per-call workspace footgun. - default field is Option<DefaultTier>. None means inherit, fallback to Deny. Some(_) is an explicit override. Lets an overlay downgrade a base default (e.g. base = Prompt, overlay = Some(Deny)), previously impossible. - merged_over(base, overlay) replaced with overlay.layered_over(&base) as an instance method so the type system catches argument flips. - Tilde expansion. Forward-slash normalize on Windows, globset::escape on the home portion so a HOME containing glob meta does not become wildcards. - Glob matching. Drop the os_str + lossy double is_match. globset::is_match takes &Path directly. The OsStr/lossy split could give wrong answers on non-UTF8 paths. Nits: - Split GlobBuildError into Pattern + Build variants. FilePolicyError gains a GlobSetBuild variant. No more empty-string pattern in errors. - Replace bespoke path_starts_with with Path::starts_with (already component-wise, no allocs). - non_exhaustive on FilePolicyError. Tests: - 15 passing (was 12). New: read_paths_inside_workspace_blocks_writes, write_beats_read_on_overlap, layered_over_can_downgrade_default. - debug_assert path.is_absolute() inside evaluate to surface caller contract violations during dev builds. Workspace cargo check clean across all 13 crates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 3 of the unified [file_policy] rollout. Replaces the workspace hard-lock in file_read / file_write / file_list with policy evaluation when an agent ships a [file_policy] block. - tool_runner::resolve_file_path now takes Option<&CompiledFilePolicy> and FileOp. With a policy: canonicalize first (so symlinks evaluate by target), reject `..` outright, then call CompiledFilePolicy::evaluate. Without a policy: legacy workspace_sandbox / naive validation. The legacy fallback preserves today's behavior for agents that haven't opted into [file_policy] yet. - channel_send (local-file attachments) and speech_to_text route through the same seam — same paths must obey the same gate regardless of which tool reads them. - agent_loop and bridge_ipc compile the per-agent FilePolicy against the workspace root once per call. Compile errors log and fall back to None (legacy lock) — fail closed against unsafe configs rather than open. - HTTP /mcp surface in routes.rs passes None: that endpoint has no per-agent manifest binding to compile against. - FileDecision::Prompt fails closed pre-step-5 with an error explaining the gap (per-path approval surfacing lands in step 5). Fills in `file_policy: None` on six AgentManifest test constructors (kernel/registry/heartbeat) that were missed when the field was added in ea9896d — only surfaced now because step 3 made the workspace builds cover those test files. Doc fix: agent.rs::AgentManifest::file_policy comment referenced FilePolicy::merged_over; renamed to layered_over in the fixup commit. Tests: 4 new tool_runner cases (allow-outside, deny-on-read-only, prompt-fails-closed, workspace-implicit-allow). 8/8 file-tool tests pass; 961/961 openfang-runtime lib tests pass; cargo check clean.
Adds a small command->arg-extractor table that recognizes the common file-touching shell commands (cat/rg/grep/head/tail/less/wc/ls/find for reads; rm/mkdir/cp/mv/tee for writes) and pulls path arguments out of parsed argv. Both tool_shell_exec and tool_process_start now run extracted paths through CompiledFilePolicy::evaluate before spawn: - Allow -> proceed - Deny -> reject with reason (op surfaced) - Prompt -> soft-deny pre-step-5 (per-path approval surfacer not yet wired, matches the MCP file-tool degradation already in place) Unknown commands fall through to exec_policy unchanged - file_policy does not gate them in v1, per the brief. Implementation notes: - shell_path_extractor.rs is a pure module: argv in, (path, op) pairs out. No I/O, no globbing, no canonicalization. Caller anchors relative paths against the agent CWD (workspace) and lexically normalizes before evaluation. - Gate is applied in both Allowlist and Full exec_policy modes - file_policy is orthogonal to exec_policy and must hold even when shell access is unrestricted. - Shell metacharacters are rejected upstream, so redirect operators (>, >>, |, etc.) cannot reach the extractor and don't need handling. - tool_process_start gets workspace_root + file_policy added to its signature, mirroring tool_shell_exec. Tests: 17 extractor unit tests + 7 gate integration tests + lexical normalize regression. All 71 tool_runner tests pass; workspace cargo check clean across all 13 crates.
Must-fix:
- lexically_normalize no longer escapes root. Pre-fixup, /ws/sub/../../../etc
collapsed to /../etc, which then string-bypassed deny_paths = ["/etc/**"]
glob matching. Now /.. is a no-op (POSIX) and we land at /etc — which DOES
match the deny rule. Regression tests at the normalizer and gate level.
Should-fix:
- Extractor table grown for week-one destructive commands: touch (write),
sed -i / -i.bak / --in-place= (write — flag-aware), dd if=/of= (key=value
parse), and find -delete / -fprint{,0,f} / -fls (find primaries that
promote roots to Write or emit explicit Write sinks). find -delete was
previously classified Read — false negative on a destructive op.
- gate_shell_argv_against_file_policy fails closed when a policy is
configured but workspace_root is None and the path is relative. Pre-fixup
this was a silent `continue`; a future caller forgetting to thread the
anchor would have bypassed the gate.
- Gate takes a tool_label parameter; tool_process_start no longer monkey-
patches errors via .replace("shell_exec", "process_start"). Brittle and
could corrupt user-controlled strings containing "shell_exec".
- Bare `-` after `--` is now treated as a filename per POSIX (was skipped
unconditionally as stdin/stdout).
- Deny / Prompt errors now surface raw token AND resolved path so the
agent can see exactly what was extracted vs what was evaluated.
Nits:
- Module doc note: file_policy enforcement on the shell vector requires
exec_policy.mode = "allowlist" to be meaningful. In Full mode, an agent
can rename binaries to escape the basename-keyed extractor; the MCP
file_* tools remain fully gated regardless.
- is_flag("--") now returns false structurally, not via short-circuit luck
in the caller. Test pins the contract.
Tests: 28 extractor (was 17) + 11 gate-level integration (was 7) +
2 lexical-normalize. 75/75 tool_runner tests pass; workspace cargo check
clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Manager A `FileDecision::Prompt` from `CompiledFilePolicy::evaluate` no longer fails closed unconditionally — instead it surfaces a human approval request via `KernelHandle::request_approval` (the same seam ANAI-32 landed for shell_exec). Approved → access proceeds; denied/timed-out → tool returns Err with the decision shape. Plumbed `kernel` + `caller_agent_id` into: - `resolve_file_path` / `resolve_with_policy` (MCP file_read/write/list, channel_send file attachments, speech_to_text) - `gate_shell_argv_against_file_policy` (shell_exec, process_start) New helper `request_file_policy_approval` builds a summary including the operation, resolved path, and policy reason so the approver has enough context without inspecting raw input. Fail-closed posture preserved: when no kernel handle is threaded through (unit-test path, or a future caller forgets), Prompt still errors with an explanatory message — never silently allow. Tests: +5 (5/5 new pass; 80 tool_runner total, was 75). - test_file_read_prompt_path_approved_proceeds - test_file_write_prompt_path_denied_blocks - test_shell_gate_prompt_path_approved_passes - test_shell_gate_prompt_path_denied_blocks - test_file_policy_deny_does_not_invoke_approver Existing prompt-fails-closed tests renamed to *_prompt_path_no_kernel to reflect the new no-kernel-handle semantics. ApprovalStubKernel inside the test module records request_approval calls and returns a canned decision — keeps tests free of the live ApprovalManager + kernel wiring. Workspace `cargo check --tests` clean. 1006/1006 openfang-runtime lib tests pass.
Must-promoted: file_policy Prompt now fails closed when caller_agent_id is None (was: log "unknown" attribution). Without an agent_id we'd be writing fake-looking audit entries on a security-relevant gate; refuse instead. Should-fix: - Batch multi-path argv into a single approval call. `cp a b c` with three prompt-tier paths now costs one prompt, not three sequential ones. The shell gate scans all paths first, short-circuits on hard Deny *before* surfacing siblings, then issues one batched approval covering every Prompt item. - Stub kernel ApprovalStubKernel: `last: Option` → `calls: Vec`, with new `requires_approval` toggle so step-5 tests can exercise the combined up-front + in-tool double-gate. - Approval summary: drop redundant tool_label (already passed as the second arg to request_approval), add raw token alongside resolved path so the operator sees what the agent actually requested. - Drop "risk classification" wording from helper doc — no classification exists. Tests (+4, 84/84 in tool_runner module): - test_shell_gate_batches_multipath_prompt_into_one_request — pins single approval call for two-path `cp` argv with a Prompt-tier `/var/log/**`. - test_shell_gate_deny_short_circuits_before_prompt_batch — pins zero surfacer calls when a Deny sibling is present alongside a Prompt path. - test_file_policy_prompt_fails_closed_when_agent_id_missing — pins the must-fix attribution refusal. - test_combined_gates_double_prompt_under_requires_approval — pins v1 behavior under both gates active (two prompts per user action). If dedup lands later, this test flips to assert one and gets retitled. Existing approved-prompt test extended to assert raw token in summary and absence of duplicated tool_label. 1010/1010 openfang-runtime lib tests pass.
Feature-level code review surfaced three real bypasses that the per-step reviews could not have caught — they live between components. Must-fix #1: KernelConfig.file_policy was dead code. The agent_loop only compiled manifest.file_policy; the global [file_policy] block in config.toml was documented in the schema but never read. Added KernelHandle::global_file_policy() (default returns FilePolicy::default(); OpenFangKernel overrides) and a new compile_effective_file_policy helper in agent_loop that layers the overlay over the global before compiling. Replaces the inline match at both run_agent_loop and run_agent_loop_streaming call sites. The pure layer_and_compile_file_policy split lets tests exercise overlay- inheritance without needing a KernelHandle stub. 4 new tests pin: global applies when overlay is None, overlay layers per-field over global (non-empty replaces, omitted inherits), both-default returns None (legacy behavior preserved), and missing workspace returns None. Must-fix #2: tool_apply_patch was unhooked from file_policy. The highest-fidelity write tool went straight to apply_patch::apply_patch with no per-target evaluation. New gate_apply_patch_against_file_policy enumerates AddFile, UpdateFile (incl. move_to destination), and DeleteFile as Write ops, scans-then-prompts (deny short-circuits before any approval surfaces, all Prompt-tier paths batch into one approval). Mirrors the shell-gate two-pass design. New canonicalize_for_policy helper extracted from resolve_with_policy so apply_patch and MCP file tools share resolution logic. 4 new tests pin: deny path blocks before disk touch, multi-path prompt batches into one approval, deny sibling short-circuits the whole patch, and legacy no-policy path is unchanged. Must-fix RightNow-AI#3: tool_media_describe / tool_media_transcribe bypassed file_policy. Both called tokio::fs::read with only validate_path (..-rejection). Sibling tool_speech_to_text was wired correctly in step 3; these two were missed. Routed through resolve_file_path with FileOp::Read. Gate fires *before* the media engine availability check so a deny path returns a policy error even when the engine is missing — testable without constructing a fake MediaEngine. 2 new tests. Tests: 1020/1020 openfang-runtime lib tests pass (was 1010, +10). - 4 new in agent_loop::tests for layered_over correctness. - 4 new in tool_runner::tests for apply_patch gate. - 2 new in tool_runner::tests for media_describe/transcribe gate. Workspace cargo check clean. Should-fix items RightNow-AI#4 (resolver asymmetry), RightNow-AI#5 (image_generate temp writes), and RightNow-AI#6 (extractor command-table gaps) deferred to ANAI-40 follow-ups; documented in projects/openfang-fork/issues/anai-40-followups.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Review-7 must-fix #1: tool_image_analyze called tokio::fs::read with no file_policy, no workspace_root, and no validate_path either — the sibling of media_describe/media_transcribe (wired in the review-6 fixup), missed because it had no validate_path call to grep for. The bypass was total: a deny_paths glob outside the workspace was silently ignored, and an agent could read /etc/passwd even with deny_paths = ["/etc/**"] configured. Threaded workspace_root, file_policy, kernel, and caller_agent_id through tool_image_analyze and routed through resolve_file_path with FileOp::Read and tool_label = "image_analyze". The gate fires before the disk read so a deny path returns a policy error rather than a read error. Existing JSON output schema preserved (path field still echoes the user-provided raw input). New test test_image_analyze_deny_path_blocks_before_read mirrors test_media_describe_deny_path_blocks_before_engine_check: writes a real PNG outside the workspace, applies a deny_paths glob covering the outside dir, asserts the call returns a "denied by file_policy" error and that the approval stub recorded zero calls (deny short- circuits before the surfacer fires). Tests: 1021/1021 openfang-runtime lib tests pass (was 1020, +1). Should-fix items RightNow-AI#4 (canonicalize_for_policy error precedence), RightNow-AI#5 (UpdateFile.move_to source-Read semantics), and RightNow-AI#6 (no end-to-end manifest+global layering test through a real tool), plus three nits, deferred to ANAI-40 follow-ups; documented in projects/openfang-fork/issues/anai-40-followups.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Apply rustfmt to shell_path_extractor.rs introduced by this branch's work. Upstream-drift files left untouched (separate concern).
Apply rustfmt to files introduced or modified by this branch. Upstream-drift files (kernel, agent_loop, channels, anthropic/openai drivers, host_functions, model_catalog, types message) intentionally left untouched as a separate concern.
Reflow the trailing paragraph in extract_sed's doc comment so clippy doesn't read it as a continuation of the bullet list. No behavior change.
Both bridge-MCP-config wiring sites in the Claude Code driver were
using .map(|cfg| { side-effects; cfg }) on Option<NamedTempFile>,
which clippy flags as manual_inspect. .inspect() expresses intent
directly. No behavior change.
The MCP bridge IPC is unix-domain-socket-only by construction (daemon
listens on a unix socket; bridge subprocess connects to it). The bridge
crate and the daemon-side `bridge_ipc` module unconditionally imported
`tokio::net::{UnixStream, UnixListener}`, which broke Windows CI with
E0432 unresolved-import errors in `openfang-mcp-bridge::main` and
`openfang-api::bridge_ipc`.
Gates:
- `openfang-mcp-bridge::main` — entire body cfg-gated to `unix`; on
non-unix the binary is a no-op stub that prints a clear message and
exits non-zero. Tests gated `cfg(all(test, unix))`.
- `openfang-api::lib` — `pub mod bridge_ipc` gated to `unix`.
- `openfang-api::server::run_daemon` — `BridgeIpcServer::start` call
gated to `unix`; non-unix logs a single info line and proceeds without
bridge IPC. The CC driver's existing missing-socket fallthrough means
CC subprocesses spawn without `--mcp-config` on Windows, matching the
bridge-disabled path.
No behavioral change on Linux/macOS. Windows users get a daemon that
boots without bridge support; MCP-routed tools are unavailable until a
Windows-native transport (named pipes / TCP loopback) lands as a
follow-up.
Verified: cargo check --workspace, cargo check --workspace --tests,
cargo test -p openfang-mcp-bridge -p openfang-api --lib, cargo fmt
--check, and cargo clippy all clean on macOS.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brings in the Windows cfg-gate for the MCP bridge (cbb1105).
Contributor
Author
|
Superseded by ANAI-45 — closing unmerged. This PR is being closed in favor of ANAI-45 (trust pipeline unification). The four-tier path schema here ( [file_policy.defaults]
read = "prompt"
write = "deny"
delete = "deny"
[file_policy.read]
silent = ["~/Documents/GitHub/Repos/**"]
deny = ["~/.ssh/**", "**/.env"]
[file_policy.write]
silent = ["~/Documents/GitHub/Repos/openfang/**"]
prompt = ["~/Documents/GitHub/Repos/**"]
deny = ["~/.ssh/**", "**/.env"]Three ops (read/write/delete), three tiers per op (silent/prompt/deny), most-specific-glob wins with strictest-tier on tie. The blanket-tier UX failure that motivated this rework — no per-op granularity per path — is the core thing fixed. ANAI-45 also routes Zero reviews here and zero installed users make a clean replacement the simpler move — no legacy-tier desugaring to write, no two-release deprecation window. Branch |
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.
Closes #1181.
Summary
[file_policy]declarative tier system (deny/prompt/read/write) with glob-matched path lists, layered global ↔ per-agent overlay, compiled once per agent at boot.resolve_file_path(path, FileOp, tool_label)chokepoint: MCP file tools,shell_exec(via per-command path extractor),tool_apply_patch, and all four media tools (media_describe,media_transcribe,speech_to_text,image_analyze).Prompt-tier hits surface through theApprovalManagerpush surface added in feat(runtime,bridge,channels): MCP bridge for CC subprocesses + shell pre-gate uplift + approval push surface #1182. Multi-path commands batch allPromptpaths into a single approval;Denyshort-circuits before any approval surfaces."unknown"attribution on a security-relevant gate.Schema
Tier precedence: Deny > Prompt > Read > Write. Most-specific glob wins within a tier; cross-tier the higher-restriction tier wins.
<workspace>and~resolve at compile time.Layering
A global
[file_policy]block inconfig.tomlprovides the operator-default policy. Per-agentagent.tomlmay include a[file_policy]overlay that layers per-field over the global (non-empty replaces, omitted inherits, both-default returnsNoneto preserve legacy behavior). Compiled once per agent at boot viaKernelHandle::global_file_policy()+compile_effective_file_policyinagent_loop. Bothrun_agent_loopandrun_agent_loop_streamingcall sites.Notable invariants
caller_agent_id = Nonewith a kernel attached → policy gate fails closed (no fake"unknown"). Pinned by test.policy.evaluateonly; if Deny, return policy error before any disk touch.FileOp::Readbefore the engine availability check, so a deny path returns a policy error even when the engine is unconfigured (testable without constructing a fake engine).Known gaps (deferred follow-ups)
These were surfaced in feature-level review and deferred. Documented in the issue (#1181) for triage:
canonicalize-based) and shell (lexicalPOSIX-correcting..). Same path string can succeed via shell and fail via MCP. Recommended fix: align both on the lexical resolver.tool_image_generatewrites to/tmp/openfang_uploads/ungated. UUID filenames; low impact, but adefault = "deny"policy is silently violated. Likely correct fix: document as kernel-managed cache.tool_text_to_speech/tool_canvas_presentwrite to<workspace>/output/ungated — same class asimage_generate.xargs,awk -i inplace,chmod,chown,truncate. Mechanical extension.canonicalize_for_policyerror precedence —apply_patchAddFile to a non-existent parent surfaces"Failed to resolve parent directory"instead of a clean policy-error message.UpdateFile.move_tosource classified as Write only — surprising under aread_pathscarve-out on the source dir. Doc-comment fix.run_agent_loop/run_agent_loop_streamingaren't pinned end-to-end.policy_explainUI exists to make resolved paths visible to operators.approval_policy.requires_approval = trueAND aprompt_pathshit. The combined-gate test pins the count at 2 so a future dedup landing flips the assertion to 1.Test plan
cargo check --workspaceclean.cargo test -p openfang-runtime --lib— 1021/1021 passing (was 1010 pre-feature, +11 from cross-component fixups, +6 from media-tool fixups, etc.).<workspace>and~expansion, layered overlay correctness (4 dedicated tests forglobal_applies/overlay_replaces_per_field/both_default_returns_None/missing_workspace).read_paths_inside_workspace_blocks_writes,test_combined_gates_double_prompt_under_requires_approval,test_file_policy_prompt_fails_closed_when_agent_id_missing,test_apply_patch_deny_blocks_before_disk_touch,test_apply_patch_multi_path_prompt_batches_into_one_approval,test_apply_patch_deny_sibling_short_circuits_whole_patch,test_media_describe_deny_path_blocks_before_engine_check,test_media_transcribe_deny_path_blocks_before_engine_check,test_image_analyze_deny_path_blocks_before_read.is_flag_rejects_double_dash_structurallyregression that caught a real bypass during step-4 review.🤖 Generated with Claude Code