feat: add require_approval action with async approval workflow#2
Closed
feat: add require_approval action with async approval workflow#2
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…l action - Add ApprovalsConfig struct and Approvals field to Config - Add ApprovalTimeout field to Rule - Accept require_approval as third action type in validation - Reject rate_limit + require_approval and state + require_approval - Validate approval_timeout and top-level approvals durations - Prevent rate_limit desugaring when action is require_approval - Add test fixtures and comprehensive tests
…roval - Add DecisionType enum (allow, deny, require_approval) - Add Type and RuleHash fields to Decision - Handle require_approval in Evaluate with condition support - Preserve deny precedence over require_approval - Add ruleHash function (SHA-256 fingerprint) - Set Type on all existing return paths for backward compat - Comprehensive engine tests for new decision type
…rprinting - Store interface with full lifecycle: create, find, approve, deny, consume, rollback, expire - SQLite backend with WAL mode, atomic consumption via UPDATE WHERE - Deterministic fingerprinting: canonical JSON with sorted keys + SHA-256 - 21 tests covering lifecycle, dedup, expiry, one-time consumption, rollback
…nd events - Handler creates pending approvals, deduplicates retries, consumes approved records - Rolls back both approval consumption and state counters on upstream failure - JSON-RPC error response (-32003) with approval_id, tool, expiry, retryable, human_instructions - Events: approval_requested, approval_reused, approval_consumed - tool_call events emit result="approval_required" for approval decisions - Nil approval store falls back to deny (backward compatible) - CLI wires SQLite approval store in cmd/root.go - 10 new proxy handler tests for full approval lifecycle
- list, show, approve, deny, expire subcommands under 'intercept approvals' - list shows pending approvals in PRD format - approve/deny accept --actor/--reason flags - expire --all-stale cleans up expired records - 9 tests covering all subcommands
- GET /api/pending, GET /api/pending/{id}, POST /api/approve/{id}, POST /api/deny/{id}
- approve/deny are idempotent, expired records return 409
- --enable-admin-api flag starts server, defaults to 127.0.0.1:9111
- Admin API Flags group in help output
- 10 tests covering all endpoints, idempotency, expiry, loopback binding
- NotifyConfig/WebhookConfig structs in ApprovalsConfig - Validation: reject incomplete webhook config, require http(s) URL - Notifier with HMAC-SHA256 signing over timestamp.body - 5s default timeout, non-2xx returns error - 5 webhook tests, 5 config webhook tests
- WithWebhookNotifier option for Handler - sendWebhook fires async goroutine on approval_requested - safePreview extracts top-5 keys, truncates strings, redacts nested - Webhook wired in cmd/root.go from config - 5 handler tests: fires on approval, no-panic without webhook, failure doesn't affect response, preview safe subset, long strings
- Cache computePolicyRevision (no per-call JSON marshal) - Unify scanRecord/scanRecordRows via scannable interface - Deduplicate handleApprove/handleDeny into handleDecision - Cache HashArgs (compute once, pass through) - Bound webhook goroutines with semaphore (cap 10) - Use interceptSubdir helper in CLI - Include approval CLI command in error message - Add approval rules to POLICY.md and USAGE.md
L1AD
force-pushed
the
feat/require-approval
branch
from
65ad1ae to
9dcbb0b
Compare
- MarkApproved/MarkDenied now include WHERE status='pending', preventing re-arming of consumed/denied/expired records from CLI or admin API - FindByFingerprint accepts dedupeWindow param, adds created_at bound so dedupe_window config is actually respected - Fix POLICY.md docs: webhook config uses notify.webhook.url not webhook_url
- FindByFingerprint: dedupe_window restricts pending record reuse only; approved records remain valid until their full expires_at lifetime - USAGE.md: fix approval ID prefix (appr_ -> apr_)
Contributor
Author
|
Closing to address review findings. Will reopen with fixes. |
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
1 participant
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.
Summary
Adds
require_approvalas a third action type for Intercept policy rules. When a tool call matches a require_approval rule, the proxy returns a structured error. A human approves via CLI or local HTTP API, then the agent retries and the call goes through exactly once.Design: async model (no hold-open), fingerprint-based dedup, one-time consumption with rollback on upstream failure.
What's included
action: require_approval,approval_timeout, top-levelapprovals:block with webhook configDecisionRequireApprovaltype withRuleHashfor fingerprintinginternal/approvals/-- Store interface, SQLite backend, canonical JSON fingerprinting, dedup, one-time atomic consumption, rollbackhandleApproval, JSON-RPC-32003error response withhuman_instructionsfield, bounded webhook goroutinesintercept approvals list/show/approve/deny/expire--enable-admin-api, loopback-onlyapproval_requested, fire-and-forgetWhat's NOT included
Key design decisions
--enable-admin-apiflag, loopback-only; even localhost is an attack surfaceTesting
go test ./...)Test plan
make testpassesmake buildsucceedsinternal/approvals/sqlite.gofor correctness (atomic consumption, rollback)internal/proxy/handler.gohandleApprovalflowinternal/approvals/fingerprint.go(canonical JSON key ordering)