Skip to content

fix: warn when permission mode is full or the sandbox is off - #837

Open
ZuyiZhou wants to merge 1 commit into
mainfrom
fix/doctor_permission_sandbox_warn
Open

ZuyiZhou wants to merge 1 commit into
mainfrom
fix/doctor_permission_sandbox_warn

Conversation

@ZuyiZhou

@ZuyiZhou ZuyiZhou commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Summary

raven doctor was silent when permissions.mode is full and when tools.sandbox.backend is none. Full skips the ask tier only; built-in denials and user deny rules still hold. none runs commands on the host with no isolation. That is the default, and it is the same fact the startup log already records. The report now names both settings. Neither warning changes the exit code. --json carries features.permission_mode and features.sandbox_backend.

Doctor reads the global config file. A conversation can override the mode for its own turns; that override is not in this report.

The identity prompt's Platform Policy, on both Windows and POSIX, now tells the agent to install a package into the project's virtual environment, or a temporary one, and to leave the global environment alone. Both policies share one sentence.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • uv run --frozen --python 3.12 --extra dev ruff check raven/cli/doctor_commands.py raven/context_engine/segments/render.py tests/test_cli_doctor_commands.py tests/test_segments.py -- all checks passed.
  • uv run --frozen --python 3.12 --extra dev ruff format --check on those four files -- already formatted.
  • COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q tests/test_cli_doctor_commands.py::test_doctor_warns_for_the_default_sandbox_and_not_for_smart_mode tests/test_cli_doctor_commands.py::test_doctor_warns_for_full_permission_and_not_for_an_enabled_sandbox tests/test_cli_doctor_commands.py::test_doctor_is_quiet_when_permission_mode_asks_and_sandbox_is_boxlite tests/test_cli_doctor_commands.py::test_the_safety_settings_reach_the_json_output tests/test_segments.py::TestIdentityBootstrap::test_identity_installs_packages_into_a_virtual_environment -- 5 passed in 3.19s.
  • Restoring raven/cli/doctor_commands.py from origin/main made those four doctor tests fail. Restoring raven/context_engine/segments/render.py made the identity test fail (count of the new sentence was 0). Both files were put back before the commit.
  • PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD -- exit 0.
  • make check-commits -- exit 0.
  • PR_TITLE="fix: warn when permission mode is full or the sandbox is off" make check-pr-title -- exit 0.
  • git diff --check origin/main...HEAD -- exit 0.
  • git fetch origin main && git merge-tree --write-tree HEAD origin/main -- clean. Base is e84152c52.

The full suite, the type checker, and coverage were not run locally. CI runs the type checker in the python lint job, and the unit shards plus coverage gates cover the rest.

The whole tests/test_cli_doctor_commands.py file was not the gate. In this terminal Rich injects color into numbers and wraps help text, so existing assertions such as --probe, 42 tokens, and 9.9.9 fail here on their own.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Ruff passed, as listed above. The type checker was not run locally. CHANGELOG.md Unreleased Fixed has the note. docs-site was left as it is, because the meaning of the two settings did not change.

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

The permission gate and the sandbox executor are unchanged. The new lines are advisory. A default install now shows the sandbox warning and still exits 0. The footer still says the configuration looks healthy, which is how the other yellow rows already behave.

--json gains two fields. A reader that ignores unknown keys is unaffected.

Rollback is to revert this change.

Related Issues

N/A

Doctor names permissions.mode full and tools.sandbox.backend none.
Neither warning changes the exit code. The identity prompt tells the
agent to install a package into the project virtual environment, or a
temporary one, and to leave the global environment alone.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

Reviewed the refreshed github/main...HEAD diff and the relevant doctor/config, permission-gate, sandbox-factory, and identity-prompt callers and history. The warnings accurately describe the shipped behavior: full bypasses only ask-tier decisions while denials remain in force, and none selects direct host execution. The global-versus-session scope is explicit, the additive JSON fields follow existing doctor-report precedent, and the prompt policy is shared across both platform branches. I also checked backward compatibility, that tests were not weakened, and the applicable AGENTS.md/CONTEXT constraints.

Verification: COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q tests/test_cli_doctor_commands.py tests/test_segments.py passed (144 tests); git diff --check github/main...HEAD passed. Current CI had all completed checks green, with the unit shards and kernel wheel smoke still pending when reviewed.

@0xKT

0xKT commented Oct 1, 2026

Copy link
Copy Markdown
Member

Not a blocker -- two things I reproduced on this head. Neither changes runtime behaviour, and neither holds the merge; both are worth a look because the first is about what the new line promises and the second is about what the new tests hold.

Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

1. The Permissions line is true of the config file, and can be false of the running conversation

doctor_commands.py:847 renders config.permissions.mode with prose about running behaviour ("ask-tier calls run without asking"). That field is not what the gate reads. permissions/gate.py:107:

mode = PermissionMode(session_mode(current_turn().conversation_id) or cfg.mode)

The conversation's own mode comes first; the config field is the fallback. permissions/session.py:54-66 makes it durable -- an unknown conversation is restored from its session record, so this survives a restart. Two real writers put a mode there: rpc/methods/config.py (a config.set carrying a session_id) and raven agent --permission-mode.

So the line can mislead in both directions: print Permissions: smart with no warning while a conversation on that machine runs full, or print the warning while the conversation in front of the reader asks for everything.

The caveat is already in the source -- doctor_commands.py:69-71, "A conversation can override the mode for its own turns; that override is not here" -- it just does not reach the user, who sees an unqualified line. This file sets its own standard for exactly that at :628-630: "doctor reporting a configured number that no longer exists would be reporting a setting, not the behaviour."

Worth saying: the Sandbox half of the same function does not have this problem. There is no per-conversation sandbox override, so that line means what it says. Only the Permissions half overclaims, and a few words ("the default every conversation starts on") would settle it.

2. The one combination the change exists for is never rendered-tested

The three rendering tests cover (smart, none), (full, auto) and (ask, boxlite). (full, none) -- the case where BOTH warnings should fire, and the most dangerous one -- appears only in test_the_safety_settings_reach_the_json_output, which goes through --json and asserts the two raw values. The --json path renders no warnings at all, so nothing checks that both warnings appear together.

Reproduced with a mutant that silences the permissions warning in exactly that combination:

-    if features.permission_mode == "full":
+    if features.permission_mode == "full" and features.sandbox_backend != "none":

bash pyt.sh tests/test_cli_doctor_commands.py tests/test_segments.py
144 passed in 11.69s

Restored, 144 passed again, tree clean. So a future edit can make either warning conditional on the other -- turning the pair off in the one configuration they are most needed for -- and ship green. One rendering test at (full, none) asserting both warning strings closes it.

This branch has not been deployed

No deployments
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.

3 participants