Skip to content

feat(harness): default-off risk evaluator in the permission engine - #257

Open
rifkir23 wants to merge 1 commit into
HKUDS:mainfrom
rifkir23:feat/risk-evaluator-permission-engine
Open

rifkir23 wants to merge 1 commit into
HKUDS:mainfrom
rifkir23:feat/risk-evaluator-permission-engine

Conversation

@rifkir23

Copy link
Copy Markdown
Contributor

Description

Implements the shape agreed in #245: one strictness-only risk evaluator wired into the permission engine, default off, rules only.

The evaluator runs only on calls the mode default (step 4) would allow, and its only move is allow → ask. That ask goes through _apply_approval_policy like every other one, so a never approval policy or a headless run ends in a deny that carries the evaluator's reason. Sensitive-path protection, the read-only upper bound, and explicit rules all decide first and stay final in either direction. The runner's fail-closed path is untouched.

Related Issues

Closes #245. (#189's heuristics can join later as extra rules on top of this, each with its own test, as suggested in the thread.)

Changes Made

  • core/harness/risk_evaluator.py (new): a RiskEvaluator protocol ((tool_name, arguments) -> reason | None, so it returns an opinion and the engine decides) and CommandGuardRiskEvaluator, which reuses command_guard.screen_all. That means the destructive-command screen always applies, and the egress/install screens apply under DEEPCODE_COMMAND_SCREEN=strict, the same as at the shell front door. There is no new keyword scorer and no model call.
  • core/harness/permissions.py: the three step-4 allow returns (plan read-only, full_auto, default read-only) go through a single _mode_default_allow hook. Opinions are cached per call signature. An evaluator that raises or returns an unusable value fails closed to ask and is not cached, so a transient error doesn't stick to a signature for the rest of the session. make_engine gains an optional risk_evaluator argument.
  • core/harness/policy.py: build_permission_engine wires the built-in evaluator in when DEEPCODE_RISK_EVALUATOR=1.
  • docs/P1_SECURITY_BASE.md: one bullet documenting the flag.

Tests (tests/test_harness_risk_evaluator.py)

These map to the five tests proposed in the issue, plus the parity test that was asked for:

  1. Cannot loosen anything: sensitive-path DENY and plan-mode DENY survive an allow opinion, and explicit allow/deny rules stay final (the evaluator is never consulted).
  2. Intended path: in full_auto with the flag on, rm -rf → ask with the screen's reason. With never, it's a deny that carries both reasons.
  3. Failure contained: raising, non-string, and empty opinions → ask (fail-closed), never a crash. Failures are not cached.
  4. No waste: zero calls when steps 1–3 (or the default-mode mutating ask) already decided, and one call per signature (cache hit asserted).
  5. Evasion fixtures: whitespace, split flags, long flags, chained commands, and 0777 are flagged. A base64-encoded payload is shown passing, as a pinned, documented limit: this is advisory, not a security boundary. The sandbox is.
  6. Feature off: across default/plan/full_auto × on_request/never, make_engine gives decisions and reasons identical to a plain engine. build_permission_engine follows the flag too.

Mutation check: removing the hook fails 7 tests, and treating evaluator failure as allow fails the fail-closed test.

Checklist

  • Changes tested locally: the new file plus every test that touches the permission engine / policy / command guard (302 passed). ruff check and ruff format --check are clean with the pre-commit-pinned ruff v0.15.21.
  • Code reviewed (self)
  • Documentation updated
  • Unit tests added

Additional Notes

One behaviour worth a decision from you: with the Full Access preset and DEEPCODE_RISK_EVALUATOR=1, the evaluator still asks, because I read the flag as explicit operator intent. If you'd rather Full Access ignore it (the way it bypasses MCP origin approval), that's a one-line change in build_permission_engine and I'm happy to make it.

…KUDS#245)

Wire one strictness-only evaluator into PermissionEngine, in the shape
agreed in HKUDS#245 (action triage, Option A, rules only):

- It runs only on calls the mode default (step 4) would allow, and its one
  move is allow -> ask, routed through _apply_approval_policy so `never`
  and headless runs end in a deny that carries the evaluator's reason.
  Sensitive-path protection, the read-only upper bound and explicit rules
  decide first and stay final.
- The built-in evaluator reuses command_guard.screen_all (destructive
  screen always; egress/install under DEEPCODE_COMMAND_SCREEN=strict)
  instead of a new keyword scorer.
- Default off. DEEPCODE_RISK_EVALUATOR=1 opts in via make_engine and
  build_permission_engine; with it off every decision and reason is
  unchanged.
- Opinions are cached per call signature. An evaluator that raises or
  returns an unusable opinion fails closed to ask and is not cached.
@rifkir23

Copy link
Copy Markdown
Contributor Author

FYI on the red Windows job: it fails in "Install package and test tools" at pip check, before any test runs. The cause is the runner's preinstalled pipx 1.17.11, which wants newer filelock/packaging/platformdirs than scripts/ci/requirements.lock pins:

pipx 1.17.11 requires filelock>=4.0.9, but you have filelock 3.32.6
pipx 1.17.11 requires packaging>=26.3, but you have packaging 26.2
pipx 1.17.11 requires platformdirs>=4.12.2, but you have platformdirs 4.11.8

The same step fails the same way on run 37660317969 (an unrelated PR from 2026-10-07), and the last main run (2026-09-28) passed it, so this looks like a runner-image change. This PR does not touch CI or dependencies, and all test (3.12/3.13/3.14) jobs pass. Happy to open a separate small PR (a venv for the Windows job, or a lock bump) if you would like.

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.

[Design] Risk triage for tool calls: what does it classify, and where does it sit in the permission engine?

1 participant