Repository navigation
fix(*): a command the host denies stays denied once it is handed to a sub-agent - #698
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: redact the denied command before writing it to retained logs.
I reviewed the PR commit and the surrounding permission gate, ACP dialect/collector paths, live-config readers, product launchers, relevant history, tests, docs, and the repository rules in AGENTS.md, CONTEXT-MAP.md, CONTEXT.md, and agents/README.md. I also checked backward compatibility, the dual enforcement architecture, and that existing tests were not weakened.
Verification on this checkout:
uv run pytest -q tests/test_config_product_render.py tests/test_agents_code_launcher.py tests/test_subagent_acp.py tests/test_acp_permissions.py: 418 passed, 1 skipped (case-sensitive filesystem)uv run pytest -q -n0 -m integration tests/integration/test_subagent_host_deny_e2e.py: 2 passedgit diff HEAD^..HEAD --check: clean- Direct log probe: a denied curl command containing a fake Authorization bearer value remained verbatim in the INFO record
The denial behavior itself is well covered and worked in both the unit and end-to-end paths. The inline credential-leak finding is the only blocker I found.
80d6231 to
4f8e0af
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the revision delta and resulting PR diff, including the shared-redactor move, all import callers, the permission paths and product-render enforcement reviewed previously, relevant history, backward compatibility, tests, and the repository architecture/rules in AGENTS.md, CONTEXT-MAP.md, CONTEXT.md, and agents/README.md. No tests were weakened.
The prior credential-log blocker is fixed, replied to, and its thread is resolved. I found no new issue.
Verification:
uv run pytest -q tests/test_config_product_render.py tests/test_agents_code_launcher.py tests/test_subagent_acp.py tests/test_acp_permissions.py tests/test_acp_redact.py: 449 passed, 1 skipped (case-sensitive filesystem)uv run pytest -q -n0 -m integration tests/integration/test_subagent_host_deny_e2e.py: 2 passeduv run --frozen --python 3.12 lint-imports: 10 contracts kept, 0 brokengit diff github/main...HEAD --check: clean
make is not installed in this environment, so I invoked the lint-imports target command directly.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This head only merges current main into the previously clean revision. I checked the merge delta, the resulting PR file set and diff, the existing resolved thread, and the previously reviewed permission/redaction paths; the feature code and tests are unchanged and no test was weakened.
Verification at 30a99cb3dffc:
uv run pytest -q tests/test_config_product_render.py tests/test_agents_code_launcher.py tests/test_subagent_acp.py tests/test_acp_permissions.py tests/test_acp_redact.py: 449 passed, 1 skipped (case-sensitive filesystem)uv run pytest -q -n0 -m integration tests/integration/test_subagent_host_deny_e2e.py: 2 passedgit diff --check 215c445a1443...HEAD: clean
The prior credential-log thread remains resolved.
30a99cb to
4f8e0af
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
c5c1083afc53 has the exact same tree (0331f4af966e05adf432a262d9ef635ee95f9536) as the last clean revision 30a99cb3dffc; only the merge commit object changed. I compared the head trees and PR diff, confirmed the prior thread remains resolved, and found no implementation or test change to reassess.
The identical tree was verified in the preceding review with 449 focused tests passed, 1 filesystem-dependent skip, 2 end-to-end tests passed, and a clean diff check.
… sub-agent The host answered every session/request_permission from an ACP sub-agent with the most permissive option, and a product launcher rendered its own config without the host's permissions. So a host deny rule bound the host's own tool calls and nothing it dispatched: with curl denied on the host, a curl handed to Raven-Code ran (measured end to end). Refusals now travel on both sides, since a refusal needs nobody to answer: - product_render.write_rendered merges the host's deny entries of permissions.tools and tools.exec.extraDenyPatterns into every product render, strictest wins, so the product's own gate refuses without asking. Every launcher ends in that call. - the host's approver reads the command a request names (rawInput.command, codex commandActions) against the host's builtin deny list and deny rules, live, and answers a match with the agent's reject option. - raven's ACP server now sends a shell call's command as rawInput.command, redacted, so a raven host can read it. The ask tier and the mode are still not carried: the approver keeps approving what no deny rule names. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
The approver logged a denied request's command verbatim, so a denied curl -H "Authorization: Bearer ..." kept the bearer value in the retained host log. Both of its log lines now go through the ACP redactor; the debug approval line had the same shape with the agent's title. The redactor moves from raven.acp to raven.security, because the outbound client may not import the server package and a second, smaller pattern table beside the real one is what drifts. The call record's docstring no longer cites the import rule as its reason for a smaller scrub. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
c5c1083 to
703c7ce
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision rebases the same two reviewed feature commits onto main at 3a463d6cd; GitHub reports the same PR file set, and the new base change is confined to the agent-loop answerless-turn work. I checked the rewritten diff, relevant permission/redaction callers and tests, repository rules/architecture already covered in the substantive review, test integrity, and the resolved thread. No new defect or compatibility change was introduced.
Verification at 703c7ceaff92:
uv run pytest -q tests/test_config_product_render.py tests/test_agents_code_launcher.py tests/test_subagent_acp.py tests/test_acp_permissions.py tests/test_acp_redact.py: 449 passed, 1 skipped (case-sensitive filesystem)uv run pytest -q -n0 -m integration tests/integration/test_subagent_host_deny_e2e.py: 2 passedgit diff --check 3a463d6cd252...HEAD: clean
The prior credential-log thread remains resolved.
|
No blockers in the reviewed scope. Scope: my review covers the I examined: the outbound approver and its command extraction, the broker change that puts Observations, none of them blocking:
Verification:
Checked and deliberately not reported:
|
… plugin with it (#703) ## Summary One plugin that failed to activate dropped every plugin. `PluginRegistry.activate` raised on the first plugin whose factory would not import (or whose contribution name collided), and `build_plugin_registry` (`raven/core/plugin_stack.py`) answered by returning an empty registry. A single broken user plugin therefore silently removed research-flow, ppt-engine, design-engine, the memory backend and the bundled playbook tools, with one stderr warning as the only trace. Reproduced with two mock plugins under a scratch `RAVEN_HOME`: `aa-broken` (factory raises `ImportError`) beside `zz-good`; before this change the registry came back with `activated: []`, after it with `design-engine, everos-memory, playbook, ppt-engine, zz-good` activated and `aa-broken` named as failed. Changes: - **Per-plugin activation** (`raven/plugins/registry.py`). Each admitted plugin activates on its own. One that fails is rolled back whole (no partial contribution in any table, and the `sys.path` entry `_ensure_importable` appended for it is withdrawn), recorded as a `PluginActivationFailure` (`activation_failures()`), and the plugins after it still activate. In a name conflict the plugin activated first (discovery order, by id) keeps the name and the other is recorded as failed. A module `__getattr__` that raises something other than `AttributeError` is now a recorded failure rather than an uncaught crash. - **Said to the user** (`plugin_stack.build_plugin_registry`, `runtime.build_runtime`). Each failure is one notice naming the plugin, its cause and the `plugins.disabled` escape, through the host's notifier (`HostWiring.notify`), else stderr - the same channel the missing-memory-backend notice already uses. When the configured memory backend belongs to a plugin that failed, the memory notice names that plugin and its cause instead of "no installed plugin provides it". - **`raven plugins`** shows a `failed` status and the cause under the table (the command called `activate` directly and previously crashed on such a plugin). - **Agents smoke check** (`raven/cli/agents_commands.py`) matched the old log line "plugin activation failed", which no longer exists; it now reads the shared `PLUGIN_FAILURE_MARKER`, and its test prints the notice the real producer writes, so the two cannot drift. - **Sub-agents inherit the host's opt-outs** (`product_render.write_rendered`, `inherit_plugin_opt_outs`). A product engine inherits `RAVEN_HOME`, so it scans the host's `<home>/plugins` and the shared interpreter's entry points, but it read `plugins.disabled` only from its own rendered config; a plugin disabled on the host came back in every product. Every render now merges the host's list after the product's own. The product's own engine plugin never travels (`own_plugins=`, passed by all five launchers and the scaffold): the PPT agent is `ppt-engine`, and a host turning it off for its own agent is not a request to run the PPT agent without it. A contract test reads every launcher's `write_rendered` call and requires it to name its `*_PLUGIN_ID`. - `CONTEXT.md` (Plugin Registry) and `docs-site/docs/building-plugin{,.zh}.md` describe the new behaviour. Not in this PR: the Web UI plugin list (`ext.list`) is discovery-only and does not show a failed plugin; adding it is a wire change (`ExtListResult`, openrpc, generated TS, the page) and is left for a follow-up. The TUI, gateway and `raven agent` hosts show the notice through their notifiers; the rpc stack (serve/acp) prints it to stderr, which is also what the agents smoke check reads. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification **Rebased**: head 7cd000d on origin/main 8ee5ae0, without conflicts. `git range-diff` shows every commit replayed unchanged. The branch's own test files rerun at this head: 395 passed. Numbers elsewhere in this description are from earlier heads. Head b4d51ae, rebased onto origin/main 6551f82 after #698 and #708 merged. #698 conflicted in `raven/config/product_render.py` and `tests/test_config_product_render.py`: both PRs added a function at the same spot (`inherit_host_denials` there, `inherit_plugin_opt_outs` here) and a call in `write_rendered`. The resolution keeps both functions and all their tests; `write_rendered` now reads `host_config()` once and passes it to both. #708 touches none of these files. At this head: ``` COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider 25948 passed, 109 skipped uv run --frozen --python 3.12 --all-extras pytest -q tests/test_config_product_render.py 44 passed ruff check / ruff format --check / make lint-imports lint-deps lint-types / git diff --check origin/main...HEAD check_source_language.py origin/main...HEAD / make check-commits / check-large-files all clean cd ui-tui && npm run lint:rpc && npm run type-check && npm test 144 files, 2106 passed ``` The earlier `TUI checks` failure (run 35834019105, head 5fc3740) was `newInstancePicker.test.tsx > quick-selects by the number it printed`. This PR changes nothing under `ui-tui/`; the test passed 5 of 5 runs locally at this head and in the full ui-tui run above. The numbers below are from the earlier heads and are kept for the record. ``` uv run --frozen --python 3.12 --extra dev ruff check raven evolver agents plugins-dist tests scripts All checks passed uv run --frozen --python 3.12 --extra dev ruff format --check raven evolver agents plugins-dist tests scripts 2083 files already formatted make lint-imports && make lint-deps && make lint-types Contracts: 10 kept, 0 broken / No dependency issues found / All checks passed git diff --check origin/main...HEAD clean COLORTERM=truecolor make coverage (full suite, head before rebasing onto 7fdf296) 25847 passed, 109 skipped COVERAGE_BASE_REF=origin/main make coverage-diff Diff coverage: 100.00% (71/71 executable changed lines) PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD exit 0 make check-commits pass git merge-tree --write-tree HEAD origin/main clean ``` After rebasing onto 7fdf296 (its two new commits touch none of these files), the touched suites at this head: ``` COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q tests/test_plugin_*.py tests/test_core_plugin_stack.py tests/test_cli_plugin_commands.py tests/test_cli_agents_commands.py tests/test_cli_tui_commands.py tests/test_config_product_render.py tests/test_agents_*launcher*.py tests/test_everos_plugin_discovery.py tests/test_release_plugin_list.py tests/test_cli_gateway_commands.py 727 passed uv run --frozen --python 3.12 --all-extras pytest -q tests/test_subagent_vendored_agents.py tests/test_rpc_subagents.py tests/test_subagent_acp.py 452 passed ``` Revert-to-red, one production file restored to origin/main at a time with the tests kept: ``` raven/plugins/registry.py -> 2 errors (plugin_stack imports PluginActivationFailure) raven/core/plugin_stack.py -> 6 failed raven/core/runtime.py -> 1 failed (build_runtime hands the host's notifier to the registry) raven/config/product_render.py -> 42 failed raven/cli/agents_commands.py -> 1 failed (smoke reds a handshake whose plugin did not load) raven/cli/plugin_commands.py -> 1 failed (failed row and escaped cause) agents/raven-ppt/run.py -> 2 failed (engine exemption, launcher contract) ``` Ten existing tests pinned "activation raises" (`test_plugin_registry`, `test_plugin_bootstrap`, and the conflict tests for tools, hooks, tool gates and session observers). They now assert the new contract instead: the later plugin is recorded with the same reason text, the earlier one keeps the name, and nothing of the loser is registered. Two stubs in `test_cli_tui_commands.py` took `build_plugin_registry(cfg)` and now accept the `notify` keyword. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed ## Risk - A plugin conflict used to disable all plugins; it now keeps the plugin that activates first and skips the other with a notice. Two plugins contributing the same name therefore run with one of them instead of none. - A product sub-agent now starts with the host's `plugins.disabled` merged into its config (its own engine excepted). A plugin that was disabled on the host but relied on inside a product stops loading there; the remedy is to enable it on the host. - `PluginRegistry.activate` no longer raises `PluginConflictError` / `PluginFactoryImportError`; the only in-tree callers were `assemble_plugin_registry` and `raven plugins`. The exception classes stay exported. - Security: the change only narrows what loads. A failed plugin's directory leaves sys.path, and the inherited opt-outs can only remove plugins from a product, never add one. - Rollback: revert the squash commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
Summary
A host deny rule bound the host's own tool calls and nothing it dispatched.
The host answered every
session/request_permissionfrom an ACP sub-agentwith the most permissive option (
raven/acp_client/permissions.py), and aproduct launcher rendered its own config without the host's permissions
(Raven-Code pins
mode: askand carries no rules). Measured end to endbefore this change: with
permissions.tools.exec {"curl *": "deny"}andtools.exec.extraDenyPatterns ["\\bcurl\\b"]on the host, a curl handed toRaven-Code asked the host, was answered "Allow for this session", and hit a
loopback sink.
Refusals now travel, on both sides, because a refusal needs nobody to answer:
Launcher side.
product_render.write_renderedmerges the host'sdenyentries of
permissions.toolsandtools.exec.extraDenyPatternsinto everyproduct render (strictest wins; a product exec tier written as one string
becomes the
*fallback beside the host's patterns; a pattern that does notcompile is left behind, as the host's own live reader leaves it). Every
launcher and the scaffold end in that call, and a test pins that. This is
the half that matters for raven's own products: a call their own gate
allows never produces a request the host could refuse.
Host side. The approver reads the command a request names
(
toolCall.rawInput.command, as sent and unquoted, and every codexcommandActionsentry) against the host's builtin deny list withextraDenyPatternsand itsdenyrules, read live, and answers a matchwith the agent's reject option (cancel only when none was offered). A check
that raises refuses. A parse error is not a refusal. This also covers
third-party agents, for what they ask about.
Server side. Raven's own ACP broker now sends a shell call's command as
rawInput.command(redacted like the title), so a raven host can read it.Logs. Both approver log lines (the INFO refusal, and the DEBUG
approval, which logged the agent's own title) go through the ACP redactor,
since the command is the sub-agent's to author and the log is retained. The
redactor moves from
raven/acp/redact.pytoraven/security/redact.py(content unchanged but one docstring paragraph), because
raven.acp_clientmay not import
raven.acp, and one table beats a second copy.Deliberately not changed: the ask tier and the mode. What no deny rule names
is still approved, as before; routing the ask tier to the host's human is a
separate decision. A rule added on the host reaches an already running raven
product at its next launch (the host-side check is live, the render is not).
Agents launched with a never-ask setting never send a request, so only their
own configuration binds them. Docs (permissions, protocol backends, both
languages) and the
Unattended Approvalentry inCONTEXT.mdsay this.Type
Verification
Head 703c7ce, rebased onto main 3a463d6 (
git range-diffshows bothcommits
=against the reviewed 4f8e0af; the head is linear again, the two"Update branch" merge commits are gone). At this head: the six focused suites
480 passed, the e2e 2 passed,
git diff --checkclean,make lint-imports10 kept. The full suite was run at the same two commits on earlier bases:
25878 passed / 109 skipped at ca5e5dc15 (shown below), 25881 passed / 110
skipped at 4f8e0af, diff coverage 100% at both. Main's newer commits touch
CONTEXT.mdin a different entry and none of the other files here:Revert-to-red, one production file at a time restored to origin/main (the
focused suites plus the e2e where it applies; measured on the first commit
before the rebases, which touched none of these files):
The first row is the two halves covering each other: with the launcher half
reverted the host half still stops the command. For the second commit, each
log line's redaction reverted alone turns
test_the_logged_refusal_and_approval_carry_no_credentialred (1 failed each).Risk
User-visible: an ACP sub-agent (Raven products, Claude Code, codex) now has a
command refused when it matches the host's deny rules or the builtin
catastrophe list, where it used to run. Hosts with no deny rules see no
difference except the builtin list. Rollback: revert this commit.
Related Issues
N/A