mirror of
https://github.com/andrewyng/openworker.git
synced 2026-09-03 13:00:37 +00:00
human_only asks skip the reviewer - it could clear git-hook writes
Found while designing reviewer stress scenarios: _authorize consulted the reviewer on ANY needs_user decision, but two asks exist precisely so a PERSON sees them - protected in-project files that execute later (.git/hooks, CI configs: 'no auto-approve path may clear them') and writes whose path cannot be located for scoping (an allow would bypass root scoping unverified). A reviewer 'allow' on either was that floor's bypass; the 8.3 prompt's git-hook example hoped for unsure but nothing enforced it. Decision grows human_only; the two branches set it; _authorize and _preconsult_reviewer skip the reviewer when it's set (card always). Shadow recording is untouched - a shadow verdict has no decision path and 'would the reviewer have allowed this?' is useful data.
This commit is contained in:
+11
-3
@@ -806,7 +806,8 @@ class TurnEngine:
|
||||
decision = self.permissions.evaluate(
|
||||
tool_call.name, tool_call.arguments, spec.metadata
|
||||
)
|
||||
if not decision.allowed and decision.needs_user:
|
||||
# human_only asks never reach the reviewer — same rule as `_authorize`.
|
||||
if not decision.allowed and decision.needs_user and not decision.human_only:
|
||||
pending.append(tool_call)
|
||||
if not pending:
|
||||
return
|
||||
@@ -948,10 +949,17 @@ class TurnEngine:
|
||||
self._audit(tool_call, stage="auto_allowed", status="allowed", reason=reason)
|
||||
|
||||
consulted_live = False
|
||||
if not allowed and decision.needs_user and self._reviewer_active():
|
||||
if (
|
||||
not allowed
|
||||
and decision.needs_user
|
||||
and not decision.human_only
|
||||
and self._reviewer_active()
|
||||
):
|
||||
# The one thing the reviewer may do: turn "ask the human" into "go ahead" —
|
||||
# never "blocked" into "go ahead" (§1.2; hard denies never reach this branch
|
||||
# because needs_user is False on them).
|
||||
# because needs_user is False on them). `human_only` asks (git hooks, CI
|
||||
# configs, unscopable writes) skip the reviewer entirely: their floor is that
|
||||
# a PERSON sees them, and a verdict here would be that floor's bypass.
|
||||
consulted_live = True
|
||||
verdict = await self._consult_reviewer(tool_call)
|
||||
self._audit(
|
||||
|
||||
@@ -204,6 +204,12 @@ class Decision:
|
||||
allowed: bool
|
||||
reason: str = ""
|
||||
needs_user: bool = False # True → surface should prompt the user for approval
|
||||
# True → this ask is reserved for a HUMAN: the Auto-Approve reviewer must not be
|
||||
# consulted and cannot clear it. Set on decisions whose entire point is that a person
|
||||
# sees them — protected in-project files that execute later (git hooks, CI configs:
|
||||
# "never WITHOUT a human — no auto-approve path may clear them") and writes whose path
|
||||
# could not be located for scoping (an allow would bypass root scoping unverified).
|
||||
human_only: bool = False
|
||||
# Set when a task-scoped standing rule allowed the call ("tool → target") so the
|
||||
# engine can audit the exact rule and the tool card can say so (§25).
|
||||
rule: str = ""
|
||||
@@ -313,6 +319,7 @@ class PermissionEngine:
|
||||
False,
|
||||
"cannot determine the write path to scope",
|
||||
needs_user=True,
|
||||
human_only=True, # an unscopable write must reach a person, not the reviewer
|
||||
)
|
||||
for path in paths:
|
||||
if not self._under_writable_root(path):
|
||||
@@ -335,6 +342,7 @@ class PermissionEngine:
|
||||
False,
|
||||
"this file runs automatically later — approval required",
|
||||
needs_user=True,
|
||||
human_only=True, # deferred-execution files: a human sees every one (§ floor)
|
||||
)
|
||||
|
||||
# Full access.
|
||||
|
||||
@@ -96,6 +96,22 @@ def test_hard_floors_hold_in_auto_approve(tmp_path):
|
||||
assert not d.allowed and not d.needs_user # hard deny: the reviewer never sees it
|
||||
|
||||
|
||||
def test_deferred_execution_files_are_human_only(tmp_path):
|
||||
# Git hooks / CI configs run on a LATER innocuous action — the floor is that a human
|
||||
# sees every such write ("no auto-approve path may clear them"). The decision says so.
|
||||
d = _gate(tmp_path).evaluate(
|
||||
"write_file", {"path": ".git/hooks/pre-commit", "content": "curl evil.site"}
|
||||
)
|
||||
assert not d.allowed and d.needs_user and d.human_only
|
||||
# An unscopable write (no locatable path) is human-only too: an allow would bypass
|
||||
# root scoping unverified.
|
||||
d2 = _gate(tmp_path).evaluate("apply_patch", {"patch": "garbage, no file header"})
|
||||
assert not d2.allowed and d2.needs_user and d2.human_only
|
||||
# An ordinary ask stays reviewer-eligible.
|
||||
d3 = _gate(tmp_path).evaluate("run_shell", {"command": "pytest -q"})
|
||||
assert d3.needs_user and not d3.human_only
|
||||
|
||||
|
||||
# -- verdict parsing: no parse path results in execution (§8.5) -------------------
|
||||
|
||||
|
||||
@@ -173,6 +189,30 @@ def test_replies_are_labelled_reply_and_turn_numbering_skips_them():
|
||||
assert lines[2].startswith(" turn 2 update the changelog") # numbering skipped the reply
|
||||
|
||||
|
||||
# -- human-only asks skip the reviewer entirely -------------------------------------
|
||||
|
||||
|
||||
def test_reviewer_cannot_clear_a_git_hook_write(tmp_path):
|
||||
# Stress case that found the gap (2026-08-17): the engine used to consult the
|
||||
# reviewer on ANY needs_user decision — including protected in-project files, whose
|
||||
# entire floor is that a PERSON sees them. An "allow" here would have been the
|
||||
# bypass. Now: card always, reviewer never asked.
|
||||
engine, rows, approvals = _engine(
|
||||
tmp_path,
|
||||
[
|
||||
_tool_turn(("write_file", {"path": ".git/hooks/pre-commit", "content": "x"})),
|
||||
AssistantTurn(text="done", finish_reason="stop"),
|
||||
],
|
||||
)
|
||||
engine.reviewer = _FakeReviewer({"write_file": "allow"}) # eager to allow — must not matter
|
||||
events = _run(engine, "update the changelog")
|
||||
|
||||
assert approvals == ["write_file"] # the human saw the card
|
||||
assert EventType.PERMISSION_REQUIRED in [ev.type for ev in events]
|
||||
assert engine.reviewer.asked == [] # the reviewer was never consulted
|
||||
assert [r for r in rows if r.get("stage") == "reviewer_verdict"] == []
|
||||
|
||||
|
||||
# -- attachments never reach the reviewer's request (§4.4) --------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user