Skip to content

Show which rule required the approval - #191

Open
d-mo wants to merge 2 commits into
mainfrom
feat/approval-rule-context
Open

Show which rule required the approval#191
d-mo wants to merge 2 commits into
mainfrom
feat/approval-rule-context

Conversation

@d-mo

@d-mo d-mo commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

The gap

Reviewing a policy, you can read its CEL expression. Reviewing an actual
approval request, you saw the tool name and the argument values and nothing
else.

So a call sitting exactly on a threshold looked identical to one in the middle
of a band, and a mis-scoped rule was invisible at the one moment someone could
still catch it. The approver had the numbers but not the line they were being
measured against.

Origin: a LinkedIn thread with Dylan Merigaud, who shipped PO approval routing
at Pivot. His "amount exactly on the threshold" story is this exact blindness,
on our surface.

What changed

The matched rule is snapshotted onto the approval request when it is created:
rule id, name, expression, expression type, priority, the decision, and the ids
of any lower-priority rules that also matched (the mis-scoping signal).

Snapshotted, not recomputed at read time. Editing or deleting a rule tomorrow
must not rewrite the record of why a call was gated last week.

The console approval view gets a "Why this needs approval" block with the
expression verbatim in monospace, tags for its priority and the arguments it
inspects, and a link to review the rule. List rows and push payloads show the
rule name only: a clipped expression reads as a different rule.

Where there is no rule

Five things can gate a call and only one is a ToolAccessRule. The rule-less
cases say so plainly instead of inventing an expression:

  • default tool gating: "No access rule matched. This tool is configured to
    require approval for every call, whatever the arguments are."
  • a rule that could not be evaluated: says Preloop failed closed
  • the agent's own permission hook: says no Preloop rule was evaluated
  • the request_approval builtin: records nothing at all

No context means the block is not rendered. Historical rows stay blank rather
than get a fabricated reason.

The block reports what matched and nothing more. There is no risk score here
and no recommendation; a test asserts the copy does not claim one.

Compatibility

evaluate_policy() returns PolicyDecision, a tuple subclass that IS the
same 3-tuple and carries .rule_context on the side. Existing callers unpack
it unchanged; tests that patch the evaluator with a plain tuple still work,
because every consumer reads the snapshot with getattr(..., None) and a
missing one reads as "not recorded" rather than raising on the enforcement
path.

New column is nullable JSONB; the read-schema field is optional.

Tests

36 backend tests (including the boundary-amount case, winning-rule-by-priority,
also-matched rules, and all three rule-less paths) and 9 frontend tests.
Nine mutations were run against the implementation and each killed the intended
test; one initially survived, so a test was added for it. Details in
factory/briefs/2026-08-06-approval-rule-context-report.md.

Note

Migration is parented on 20260806_ai_model_updated_at (from #190), not on
20260801_stagger_email, to keep a single alembic head.

Note

Low Risk
Well-tested snapshot of rule context onto approval requests with backward-compatible tuple subclass design. No security concerns. One pre-existing code structure observation remains (duplicate sync/async eval loops).

Overview
Adds a rule_context JSONB column to ApprovalRequest capturing the policy rule that gated a call. The PolicyDecision tuple subclass preserves backward compatibility while carrying rule identity, expression, priority, and also-matched rules through the approval pipeline. A new frontend block renders "Why this needs approval" with the expression verbatim.

Written by Preloop PR Reviewer for commit on feat/approval-rule-context. Updates automatically on new commits.

An approver reviewing a policy could read its CEL expression, but an
approver reviewing an actual request saw only the tool name and the
argument values. So a call sitting exactly on a threshold looked
identical to one in the middle of a band, and a mis-scoped rule was
invisible at the one moment someone could catch it.

The matched rule is now snapshotted onto the approval request when it is
created: rule id, name, expression, its type, priority, the decision,
and the ids of any lower-priority rules that also matched. Snapshotted,
not recomputed, so editing or deleting a rule later cannot rewrite the
record of why a past call was gated.

Cases without a rule are stated plainly rather than dressed up: default
tool gating says the tool requires approval for every call, a rule that
failed to evaluate says Preloop failed closed, and the request_approval
builtin records nothing at all. Surfaces omit the block when there is no
context; historical rows stay blank rather than get a fabricated reason.

The console shows a "Why this needs approval" block with the expression
verbatim in monospace; list rows show the rule name only, since a
clipped expression reads as a different rule. Push payloads carry the
rule name for the same reason.
@cursor

cursor Bot commented Aug 6, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@preloop-staging preloop-staging Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💬 Preloop has suggestions for this PR.

See the summary comment below for details.

@preloop-staging

preloop-staging Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Preloop Code Review

Last Updated: 2026-08-06 (re-review pass)
Reviewing Commit: on feat/approval-rule-context
Review Status: Approved


Summary

This PR adds rule context snapshots to approval requests so approvers can see which policy rule gated a call. The implementation is clean: PolicyDecision carries backward-compatible tuple unpacking with a side-channel .rule_context, and the new frontend component renders it elegantly. The defensive getattr(..., None) pattern throughout ensures the enforcement path never breaks on a missing snapshot. 36 backend + 9 frontend tests cover the boundary-case, priority-ordering, and rule-less paths thoroughly.

What Looks Good

  • Backward-compatible PolicyDecision tuple subclass design working callers and test mocks are unaffected
  • All five rule-less gating sources produce truthful, non-fabricated explanations
  • "No risk score" assertion in the test guarantees the block stays factual
  • Migration is clean (nullable JSONB) and correctly parented
  • WebSocket broadcasts and push notifications pass rule_context/rule_name through the pipeline
  • Comprehensive test coverage with mutation testing
  • The #tool= hash parameter now enables deep-linking from the "Review this rule in Tools" link

Issues Found

Medium Priority

  • [Code Quality]: Duplicate rule-evaluation logic in sync and async paths policy_evaluator.py:517-677 and policy_evaluator.py:1098-1252. This predates the PR; the new _matched_rule_context() and _evaluate_rule_candidates() helpers are a step in the right direction, but the main evaluation loops remain structurally identical.

Resolved Issues

  • [Frontend]: "Review this rule in Tools" link does not deep-link to the specific tool config -- Fixed with #tool= hash parameter navigation in tools-view.ts

Progress: 1 of 2 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

Two things from the PR review.

The sync and async evaluators each built the rule snapshot inline with
identical code, which is how the two quietly start recording different
things about the same rule. Both now call _matched_rule_context().

"Review this rule in Tools" dropped the reviewer into the full tool
catalogue. There is no per-tool route to deep-link to yet, so the link
now carries the tool name in the hash and the Tools page narrows its
list to it. Without a tool name it falls back to the plain page.

@preloop-staging preloop-staging Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preloop has reviewed this PR.

The previous "Review this rule in Tools" link concern has been resolved with the #tool= hash parameter. One pre-existing code structure observation remains, noted in the summary below.

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.

1 participant