Agent plan preapprove#133
Conversation
8aa8af9 to
9a192b8
Compare
hunner
left a comment
There was a problem hiding this comment.
Solid, carefully-engineered implementation — the fail-closed plumbing (parse failures → denied/human, LLM errors → human review or normal gating, missing charter → deny), the status-poll fast pass with its adversarial test matrix, and the overall test discipline are genuinely good. The concerns below are at the policy-design layer, not the implementation.
The through-line of the inline comments: the plan path is not judge-equivalent to unplanned execution. The plan judge necessarily sees agent-authored summaries rather than concrete arguments (inherent to preapproval), so the adherence judge is the mechanism that holds execution to the approved intent — but it currently only runs for AI-approved plans, its verdict set can't catch charter violations that follow the plan, and the judge/reviewer are deciding under a one-shot mental model of a pass that is actually unlimited-use until expiry.
Summary of the comments:
Fixes requested
- Run the adherence judge on human- and auto-approved plans too (
approvedPlanPass) - Give the adherence judge a charter-violation path (
planAdherenceSystemPrompt) - Default an omitted
action.serverto the submitting source instead of matching any upstream - Persist
TTLSecondsinPlansRepo.Update - Test coverage for the
Invokeplan-pass branch (and dedupe withSubmit)
Discussion starters
6. Should approved-plan passes be consumable/counted rather than unlimited-use within TTL? (planJudgeSystemPrompt)
7. Should invocation-scoped auto_deny rules stay authoritative over plan passes?
🤖 Automated reply
|
I talked to @hunner about this in a meeting and am putting it back in draft while i revise it |
9a192b8 to
0eebf64
Compare
dd15fb2 to
8449cdc
Compare
There was a problem hiding this comment.
I know @nibalizer had brought this up with one of his demos that Invocations were not displaying in real-time, so I'm assuming the same is true for Plans
Invocations → SSE (server-push). useApprovalNotifications opens a streaming connection to /api/v1/admin/invocations/stream?status=pending_approval with Accept: text/event-stream, reads it via response.body.getReader(), and reacts to named invocations events as the server pushes them. Pending invocations therefore surface in near real-time.
Plans → polling (client-pull). There's no plan stream endpoint. Two independent 5-second setTimeout loops fetch /api/v1/admin/plans?status=pending_approval:
one in useApprovalNotifications (for the desktop notification), and
one in usePlans via react-query's refetchInterval (for the Plans page).
The hook author even flags it explicitly in usePlans.ts: // Plans have no SSE stream — poll every 5s so pending submissions show up.
Excellent feedback, thanks |
4d5ba0d to
c4d0a1e
Compare
e8c48b8 to
dcbd3ad
Compare
hunner
left a comment
There was a problem hiding this comment.
🗓️
Nice work!
The only interesting thing I saw is that if a plan has actions that are 100% covered by auto_approve rules, plan_service still runs the evaluateMandatoryPlanCharter against the plan. This means an agent without a charter can run auto-approved commands, but would get a plan denied for the lack of a charter. Just a weird gap that doesn't really matter in real scenarios I don't think.
We would like to give agents the option to submit plans for preapproval. Similar to normal invocations rules can be applied to plans.
Summary
Adds agent plan preapproval, allowing agents to submit plans for approval before executing their individual tool calls.
Plan approval uses the existing invocation rules. Each planned action is evaluated against relevant rules in order, while a shared LLM judge reviews the entire plan for charter
compliance at most once.
What changed
Plan approval behavior
For each action in a submitted plan, relevant invocation rules run from top to bottom:
A mandatory LLM judge reviews the entire plan for charter compliance:
Plan execution behavior
When an approved plan runs:
The implementation supports both authenticated and no-auth deployments, including managed-agent identities and aliases.
UI
Testing
Added coverage for:
#90


Note that because claude managed agents doesn't have a harness it won't get the hook telling it about this feature.