Repository navigation
Conversation
…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.
Contributor
Author
|
FYI on the red Windows job: it fails in "Install package and test tools" at The same step fails the same way on run 37660317969 (an unrelated PR from 2026-10-07), and the last |
This branch has not been deployed
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
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.
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. Thataskgoes through_apply_approval_policylike every other one, so aneverapproval 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): aRiskEvaluatorprotocol ((tool_name, arguments) -> reason | None, so it returns an opinion and the engine decides) andCommandGuardRiskEvaluator, which reusescommand_guard.screen_all. That means the destructive-command screen always applies, and the egress/install screens apply underDEEPCODE_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-4allowreturns (plan read-only,full_auto, default read-only) go through a single_mode_default_allowhook. Opinions are cached per call signature. An evaluator that raises or returns an unusable value fails closed toaskand is not cached, so a transient error doesn't stick to a signature for the rest of the session.make_enginegains an optionalrisk_evaluatorargument.core/harness/policy.py:build_permission_enginewires the built-in evaluator in whenDEEPCODE_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:
full_autowith the flag on,rm -rf→askwith the screen's reason. Withnever, it's a deny that carries both reasons.ask(fail-closed), never a crash. Failures are not cached.ask) already decided, and one call per signature (cache hit asserted).0777are flagged. A base64-encoded payload is shown passing, as a pinned, documented limit: this is advisory, not a security boundary. The sandbox is.make_enginegives decisions and reasons identical to a plain engine.build_permission_enginefollows the flag too.Mutation check: removing the hook fails 7 tests, and treating evaluator failure as allow fails the fail-closed test.
Checklist
ruff checkandruff format --checkare clean with the pre-commit-pinned ruff v0.15.21.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 inbuild_permission_engineand I'm happy to make it.