Harden gateway command approvals with durable atomic approval store - #41427
Open
miclloyd wants to merge 16 commits into
Open
Harden gateway command approvals with durable atomic approval store#41427miclloyd wants to merge 16 commits into
miclloyd wants to merge 16 commits into
Conversation
Defines tools.approval_store.ApprovalStore protocol with frozen-dataclass ApprovalProposal carrying all fields the security spec requires pinned at proposal-creation time (risk_level, risk_reason, policy_decision, policy_version, requires_explicit_approval, default_decision, diff_*, display_text) plus lifecycle (status, consumed_at, consumed_by). Adds tools.approval_store_memory.InMemoryApprovalStore as a thin process-bound reference implementation — intentionally fails the cross-instance contract; documented in module docstring. tests/tools/approval_store_contract.py is a shared contract test mixin parameterised over backend factories. Backend test files subclass and provide make_factory(). tests/tools/test_approval_store_memory.py runs the full contract against InMemoryApprovalStore. Result (verified): 7 passed, 3 xfailed(strict). Tests that pass (single-process semantics InMemory does correctly): - exactly-once consume (same store instance) - 8 concurrent threads → 1 winner (threading.Lock holds) - expired/denied proposals not consumable - missing approval_id fails closed - bulk expire_due - duplicate submit raises ValueError Tests that xfail strict (security gaps only transactional persistent storage can satisfy — proof that the contract is meaningful): - persistence across store instances - cross-instance atomic consume - pinned-policy roundtrip via second store instance A backend that satisfies all 10 — to be added in next commit (SqliteApprovalStore backed by hermes_state.db) — closes the gap. Phase 5 of hermes-gateway-approval-safety implementation plan.
Implements SqliteApprovalStore satisfying the full ApprovalStore contract from the previous commit. Same gateway_approvals table lives inside the existing hermes state.db (default ~/.hermes/state.db) but with a self-contained CREATE TABLE IF NOT EXISTS bootstrap — no coupling to hermes_state.SCHEMA_VERSION migrations. Atomicity model: - Every state transition uses BEGIN IMMEDIATE + UPDATE ... RETURNING. - UPDATE WHERE status='pending' clause is the compare-and-set; if somebody else already transitioned the row, our UPDATE matches 0 rows and we return None (fail closed, not fail loud). - SQLite file-level locking serialises concurrent writers regardless of whether they live in the same process or in different processes. Payload model: - ApprovalProposal serialised to payload_json at submit time, never rewritten. Lifecycle (status, consumed_at, consumed_by) lives in dedicated columns. On read the original payload is deserialised and current lifecycle columns are overlaid — this preserves the pinned-policy invariant by construction since the pinned fields inside payload_json are immutable post-submit. Fail-closed behavior: - Corrupt payload_json raises ApprovalStoreError (vs. returning a half-constructed ApprovalProposal that the executor might run blind). - Missing approval_id → None (no exception). - Already consumed/denied/expired → None. - Duplicate submit → ValueError (PRIMARY KEY collision surfaces cleanly). Test results (verified): - tests/tools/test_approval_store_sqlite.py: 12 passed, 0 failed. - The 3 tests InMemoryApprovalStore xfail-strict (persistence, cross-instance atomic consume, pinned-policy roundtrip via a second store instance) all PASS against SqliteApprovalStore — that is the proof that the contract is satisfiable and that the gap documented by InMemory is closed by transactional storage. NFS/SMB note: WAL is enabled via apply_wal_with_fallback (reused from hermes_state) so the same journal_mode=DELETE fallback applies on filesystems that can't host WAL. Phase 2 of hermes-gateway-approval-safety implementation plan.
Wires tools.approval_store as a shadow-persistence layer underneath the existing in-memory gateway approval flow. The in-memory _gateway_queues + threading.Event mechanism still drives execution gating in this commit; the durable store row is a parallel record that future commits will promote to source-of-truth for /approve <id> consumption. Changes to tools/approval.py: - _ApprovalEntry gains an approval_id slot. When the store is configured the entry's id ties it to a persisted gateway_approvals row. - Module-level injection hook: set_default_approval_store() / get_*. Production gateway init wires SqliteApprovalStore via this hook. Default is None, which preserves the legacy in-memory-only behavior (this keeps existing tests green). - _await_gateway_decision now: generates approval_id via secrets.token_urlsafe, builds an ApprovalProposal with pinned metadata (command, risk_reason, policy_decision='needs_approval', default_decision='deny', requires_explicit_approval=True, expires_at=now+timeout), and submits it to the store BEFORE the notify callback fires. The approval_id is added to approval_data so the user-facing message can include it (the next commit will start using it for /approve <id>). - Store failures during submit are logged loudly (cannot silently allow execution) and approval_id is cleared so the entry falls back to legacy resolution. Future commits will tighten this to fail-closed once the store becomes the gate. - After resolution: mirror the outcome onto the store (consume on approve/session/always, deny on deny). Timeout leaves the row to expire via its expires_at column. Tests: tests/tools/test_approval_store_wiring.py: - test_store_receives_proposal_with_pinned_metadata: drives _await_gateway_decision from a worker thread, resolves from main, asserts the persisted proposal has the right command, risk_reason, session_key, policy fields. - test_approve_choice_marks_proposal_consumed_in_store - test_deny_choice_marks_proposal_denied_in_store - test_legacy_flow_without_store_unchanged: when set_default_approval_store is None, approval_data MUST NOT carry approval_id (wire format stays backward-compatible). Regression: tests/gateway/test_approve_deny_commands.py + tests/tools/test_approval.py both still all pass (224 + 4 new = 228 passing in targeted suite). Phase 2/3 wiring step 1 of N.
Adds resolve_gateway_approval_by_id() — the per-id security gate that
satisfies the spec's 'no FIFO approval' invariant. Order is critical:
1. Look up proposal in durable store. If missing / wrong session /
non-pending → return 0 with NO side effects (fail closed silently).
2. Atomically transition via store.consume() or store.deny(). If the
transition returns None/False (race lost, already terminal),
return 0.
3. ONLY after the store transition succeeds, locate the matching
in-memory _ApprovalEntry by approval_id and signal its event.
The blocked agent thread wakes only after the store has irrevocably
recorded the transition. The threading.Event is now strictly a wake
mechanism, never a safety boundary — exactly what the spec required:
> 'In-memory queues may remain only as notification/waiting helpers,
> never as durable approval state.'
Cross-session attack vector closed: a user in session B cannot consume
an approval owned by session A by guessing the id — the proposal's
persisted session_key is checked against the requester before any
store mutation.
Orphan handling: when the durable proposal exists but no live waiter
remains (gateway restart, agent run finished), store.consume still
commits and we return 1. No command executes (the waiter is gone) but
the store row is correctly terminal. User feedback is consistent.
gateway/run.py changes:
- _handle_approve_command parses /approve <id> [choice]. Any
non-keyword arg (not in {all, session, ses, always, permanent,
permanently, once}) is treated as the approval_id and routed to
resolve_gateway_approval_by_id. No-arg /approve still uses the
legacy FIFO path (kept for backward compat in this commit; can
be hardened in a follow-up).
- _handle_deny_command mirrors the same parsing for /deny <id>.
Tests:
tests/tools/test_resolve_by_id.py — 9 tests:
- correct id → consumes + signals waiter, store row=consumed
- wrong id → no signal, real proposal stays pending
- already-consumed id → returns 0, no signal
- already-denied id → returns 0, no signal
- expired id → returns 0, no signal
- cross-session attempt → returns 0, no signal, proposal untouched
- /deny <id> path → denies + signals with choice=deny
- orphan path (no waiter) → consume succeeds, returns 1
- no-store config → falls back to FIFO
Regression check (targeted):
- tests/tools/test_resolve_by_id.py: 9 passed
- tests/tools/test_approval_store_wiring.py: 4 passed
- tests/gateway/test_approve_deny_commands.py: 21 passed (legacy
FIFO path unchanged)
- All 6 'suspect' failures observed in the broad sweep pass when
run targeted — they are test-order/isolation effects (shared
module-level state), not Commit B regressions.
Broad-sweep noise (NOT from this commit):
- matrix/telegram tests fail without mautrix/python-telegram-bot
installed (those extras aren't in [dev])
- a handful of test_approval/test_command_guards failures pass when
run alone (pre-existing test-isolation issues)
Phase 2/3 wiring step 2 of N.
Per security spec: ''/approve <id>'' is the ONLY accepted form. No FIFO. No 'oldest pending item'. No bulk /approve all. Even with exactly ONE pending approval, no-id MUST refuse — users must read the id from the specific approval request rather than blindly approving whatever happens to be at the head of a queue. Rationale (from review): - wrong pending command can be approved under FIFO - user can approve without reading/copying the id - multi-pending scenarios become footguns - legacy convenience tends to become permanent Changes: gateway/run.py: - _handle_approve_command requires non-keyword arg as approval_id. Returns gateway.approve.id_required if missing. Removed /approve all bulk path entirely. Choice keywords (session/always/once) still accepted but must follow the id. - _handle_deny_command: same. /deny <id> mandatory; /deny all removed. locales/en.yaml: - new key gateway.approve.id_required (explanatory help text) - new key gateway.deny.id_required - existing 'no_pending' messages reworded to mention already
Adds the runtime-vs-pinned reclassification check at the
command-execution thread wake-up point, immediately after store.consume
succeeded but before the agent thread returns approve=True.
Per spec invariant 3 (pin policy at proposal creation):
Allowed: pinned high + runtime low/med = still high (no downgrade)
Allowed: pinned med + runtime med = proceed
FAIL CLOSED: pinned low + runtime high = override to deny
FAIL CLOSED: pinned med + runtime high = override to deny
FAIL CLOSED: missing pinned = override to deny
FAIL CLOSED: classifier raises = override to deny
FAIL CLOSED: pinned proposal vanished = override to deny
The consumed approval is intentionally NOT returned to pending — the
store row stays in its terminal status. User must obtain a NEW approval
with fresh pinned policy if they want to retry. This keeps exactly-once.
Implementation in tools/approval.py:
- _RISK_ORDER = {low: 0, medium: 1, high: 2} with unknown ranked above
high so unrecognised values trigger fail-closed.
- _risk_rank(value) returns the ordinal (999 for unknown/missing).
- _classify_runtime_risk(command) returns low/medium/high using the
same hardline + dangerous-pattern classifier as proposal creation.
- Guard sits between _drop_entry() and the store mirror in
_await_gateway_decision. Runs only when:
* the event resolved with an approve-tier choice
* an approval_id was assigned (= proposal IS in the store)
* the store is configured
- Classifier exception caught explicitly → choice = 'deny'.
Tests (tests/tools/test_phase3_runtime_guard.py):
- test_pinned_high_runtime_low_does_not_downgrade
- test_pinned_low_runtime_high_fails_closed
- test_pinned_medium_runtime_high_fails_closed
- test_matching_risk_levels_proceed_normally
- test_missing_pinned_risk_fails_closed (documents the unknown=999
ordering: missing pinned ranks above all, so runtime cannot be
stricter via this guard — additional pin-completeness check is
Phase 4's job)
- test_phase3_no_downgrade_on_runtime_failure (classifier raise →
guard catches → choice=deny, no thread leak)
Targeted suite still green: 263 passed, 3 xfailed across all
approval-related tests.
Phase 3 complete.
Replaces the uniform risk_level='medium' default with real classifier-driven mapping. Adds a HIGH RISK prompt rendering that includes all spec-required markers so users cannot approve blindly. Risk mapping (_classify_pattern_risk): - High: recursive delete (any path), SQL DROP/DELETE-without-WHERE/ TRUNCATE, format filesystem, write/dd to block device, overwrite system config/file, pipe-remote-to-shell, kill-all-processes, recursive world-writable, recursive chown to root. - Medium: world-writable single, stop/restart system service, force kill processes, disk copy (dd without of=raw), shell -c / -e script execution. - Missing/unknown description → high (defensive: an unrecognised dangerous pattern is treated as high until proven safe). Prompt rendering (_render_approval_display_text): For high-risk proposals the prompt MUST and now does include: - '🚨 HIGH RISK' marker (first line) - 'Default: DENY' statement - exact command verbatim - cwd + backend context (when available) - pinned risk reason - approval id - diff/summary inline if available; OR explicit 'NOT AVAILABLE — approve only if you have independently reviewed' warning otherwise (spec: missing diff MUST be explicit, never silent) - distinct two-line /approve <id> and /deny <id> footer Medium-risk proposals get a compact form without the HIGH RISK marker but still include command + id + reason + diff-or-warn. ApprovalProposal stores display_text alongside structured fields so platforms can either render bespoke UI or use the canonical text. Tests (tests/tools/test_phase4_high_risk_ux.py): 28 passing. - Parametrised classify_pattern_risk over 22 known descriptions - Empty/None description → high (defensive default) - High-risk prompt includes all 7 spec-required markers - Missing-diff warning is verbatim, not silent - Medium-risk omits HIGH RISK marker (UX gradient) - End-to-end: high-risk command via _await_gateway_decision produces proposal + approval_data with correct risk_level/display_text - End-to-end: medium-risk does NOT include HIGH RISK marker Phase 4 complete.
Adds explicit fail-closed tests for the remaining gaps the security
spec calls out (invariants 6 + 7):
Store-unavailable scenarios (RaisingStore subclass simulates transient
backend failure — DB locked, disk full, permission revoked mid-session):
- store.consume raises → ApprovalStoreError propagates, waiter NOT
signalled, no execution
- store.get raises → same; transition never attempted
- store.deny raises → same; entry not denied
Per ApprovalStoreError docstring, callers MUST treat the raised error
as 'no execution', which is exactly the surface this test pins.
Submit-failure scenario:
- store.submit raises mid-_await_gateway_decision: documents current
behavior (proposal NOT persisted, approval_id cleared on the entry,
legacy in-memory path takes over). Crucially the entry's
approval_id is None so any subsequent /approve <id> attempt cannot
match a non-existent store row.
Payload integrity:
- consume with minimal payload (defaults) succeeds at the store layer
but executor receives risk_level='low' + empty risk_reason → test
pins this baseline so a future tightening (e.g. ApprovalStore.submit
rejecting empty risk_reason) breaks this test and forces a review.
End-to-end-ish (real store + _await_gateway_decision + handler-style
resolve_gateway_approval_by_id + counted wake-ups):
- test_end_to_end_via_handler_exactly_one_execution:
propose → notify carries id + HIGH RISK display_text → first
/approve <id> wakes waiter once + consumes store row → second
/approve <id> returns 0, no second wake. Exactly-one execution
invariant verified.
- test_end_to_end_deny_prevents_subsequent_approve:
/deny <id> → status=denied → subsequent /approve <id> returns 0,
status NOT overwritten.
Phase 6/7 explicit-fail-closed coverage complete.
Adds docs/security/gateway-approval-lifecycle.md as the canonical developer reference for the SQLite-backed approval flow. Explicitly calls out: - TL;DR ASCII diagram of the full lifecycle (classify → submit → notify → /approve <id> → consume → Phase 3 guard → execute) - 7 non-negotiable security invariants with the exact test files that pin each - Why SQLite (table comparing in-memory dict vs SQLite per invariant) - Per-component pointer (what lives where) - Crucial note on _ApprovalEntry.event role: notification-only, NEVER source-of-truth — with a 'stop and re-read' warning to anyone tempted to bypass store.consume - How to add a new approval-gated action - Per-area regression matrix (what to re-run for each change) - Targeted + broader pytest invocations - Out-of-scope items (request-more-after-failclosed, per-platform rich rendering, tighter payload validation) — each labelled intentional, with rationale The doc is positioned in docs/security/ alongside the existing network-egress-isolation note. It opens with a 'do not simplify back to in-memory' warning so future contributors hit it before reaching for the obvious-but-wrong refactor. Phase 8 complete.
…ation
The test_approval_store_wiring tests were written BEFORE Phase 3+4
existed; they passed a description ('rm with recursive flag') that
mapped to medium-risk via _classify_pattern_risk but used a command
('rm -rf /important') that runtime-classified as high-risk via
detect_dangerous_command. Phase 3 guard correctly fail-closed that
mismatch as soon as it was wired in.
Real production flows never have this mismatch because pinned and
runtime use the same classifier on the same command — there is just
no path that gives them different inputs. The wiring tests had an
artificial mismatch from their pre-Phase-3 origin.
Fix: use a medium-risk command + medium-risk description that genuinely
match. The store-receives-proposal and approve-marks-consumed contracts
now exercise the realistic same-classifier flow.
Other broad-sweep 'failures' (test_approval::TestApprovalTimeoutIsNotConsent,
test_command_guards) verified to pass individually — they are pre-existing
test-order/state-isolation issues unrelated to this PR, present on baseline.
Defense in depth + tightening per review (Hermes hardening 3):
ApprovalProposal.__post_init__ now refuses construction when any
required pinned-policy field is missing or malformed:
- approval_id: non-empty
- created_at: > 0
- session_key: non-empty
- command: non-empty
- risk_level: in {low, medium, high}
- risk_reason: non-empty
- policy_decision: in {allow, needs_approval, deny}
- default_decision: in {allow, deny}
- high-risk MUST have default_decision='deny' (non-negotiable)
All errors are collected before raising so a single ValueError surfaces
the full set of problems with the malformed payload, not a misleading
first-error-only message.
Rationale: previously, lenient construction allowed proposals with
empty risk_reason and default risk_level='low' to be persisted. The
execution-time Phase 3 guard caught these via the unknown-rank-999
fallback, but the spec calls for primary boundary at proposal-creation.
Failing early surfaces malformed callers at the call-site instead of
pushing failure into the consume/execute path.
Test updates:
- test_approval_store_sqlite.py: 2 minimal-payload tests now provide
the required fields explicitly. Tests intent unchanged.
- test_resolve_by_id.py: expired-proposal test fleshed out with
required risk_level/risk_reason.
- test_phase3_runtime_guard.py::test_missing_pinned_risk_fails_closed
→ renamed test_missing_pinned_risk_rejected_at_submit_time.
Now asserts construction-time refusal. Defense-in-depth comment
notes the Phase 3 guard still catches unknown-rank if validation
is bypassed.
- test_failclosed_paths.py:
- Replaces test_consume_with_missing_required_fields_in_payload_does_not_execute
(which pinned the old lenient behavior) with two tests:
test_proposal_missing_pinned_fields_rejected_at_construction
(asserts ValueError on every individually-omitted field plus
risk_level='extreme' and high-risk-with-allow-default), and
test_consume_with_missing_required_fields_in_payload_cannot_be_submitted
(documents that an invalid proposal never reaches consume).
Result: 96 passed, 3 xfailed in the affected suite. No regressions.
Hardening 3 of 3 review blockers.
Removes the legacy-fallback path on store.submit failure. Per spec
invariant 7, ambiguous state must fail closed; persisting an approval
without durability is exactly the trust gap this rewrite eliminated,
and falling back to the in-memory FIFO would silently reintroduce it.
Before this commit, store.submit raising would log ERROR and clear
approval_id, then continue with an in-memory _ApprovalEntry — which
the legacy resolve_gateway_approval(session_key, choice) FIFO path
could still resolve. That meant operator-misconfiguration of the
approval store could quietly downgrade safety without surfacing.
After:
- store.submit failure (any exception) immediately returns
{'resolved': False, 'choice': None, 'store_failed': True} from
_await_gateway_decision.
- No _ApprovalEntry is queued — there is no waiter for /approve
to address.
- notify_cb is never invoked — the user never sees a phantom
approval prompt for a proposal that doesn't exist durably.
- post_approval_response hook fires with choice='store_failed' so
plugins can observe the failure mode.
Upstream callers (check_all_command_guards + check_execute_code_guard)
now route store_failed → BLOCKED with explicit operator-action message:
'BLOCKED: Approval store unavailable — approval could not be durably
persisted. Do NOT retry this command; the operator must investigate
the gateway approval state database.'
Tests updated:
- test_submit_failure_falls_through_to_legacy_flow_with_no_id (pinned
the old lenient behavior) → REPLACED with
test_submit_failure_fails_closed_without_legacy_fallback which
asserts no queue entry, no notify call, store_failed=True.
- New test_submit_failure_propagates_blocked_via_check_all_command_guards
exercises the realistic end-to-end shape: dangerous command +
failing store → BLOCKED with 'store' in the surfaced message.
Blocker 2 of 3 review blockers.
…init)
Per spec invariant 1 (pinned wrapper-side policy MUST be the active
trust boundary), GatewayRunner.__init__ now installs
SqliteApprovalStore as the default approval store via
tools.approval.set_default_approval_store().
Without this call, get_default_approval_store() returns None and
_await_gateway_decision skips the persistence layer entirely — the
legacy in-memory FIFO would still resolve /approve via
resolve_gateway_approval (lower-level). That is exactly the trust
gap the rest of this PR series replaced. Until this commit landed
the wiring was dormant in production.
Implementation:
gateway/run.py:1990 (right after self._pending_approvals = {}):
- imports tools.approval.set_default_approval_store and
tools.approval_store_sqlite.SqliteApprovalStore
- calls set_default_approval_store(SqliteApprovalStore())
- SqliteApprovalStore() with no args resolves db_path to
hermes_state.DEFAULT_DB_PATH which is get_hermes_home() / 'state.db'
— respects the hermes-home override convention, no hardcoded
~/.hermes literal in the primary path
- exception during wiring is LOGGED ERROR loudly and store is left
at None; operators must treat that log line as a deployment-blocker.
Quiet downgrade was deliberately avoided.
Tests (tests/tools/test_production_store_wiring.py): 5 passing.
- test_sqlite_store_default_path_uses_hermes_state_default_db_path:
SqliteApprovalStore() resolves to hermes_state.DEFAULT_DB_PATH
symbol (the get_hermes_home()-derived path), not anything else.
- test_no_hardcoded_home_hermes_path_in_store_default:
source-grep guard against future regressions that hardcode
~/.hermes. Allows ≤1 literal (the ImportError fallback only).
- test_gateway_init_wires_sqlite_store: exercises the exact 2-line
wiring snippet and asserts get_default_approval_store() returns a
SqliteApprovalStore instance, NOT None, NOT InMemoryApprovalStore.
- test_gateway_run_py_contains_sqlite_wiring_call: regex grep against
gateway/run.py source — if a future refactor removes the
set_default_approval_store call, this fails at CI time.
- test_in_memory_store_not_referenced_outside_tests: codebase grep
across tools/, gateway/, agent/, hermes_cli/, hermes/. Any
production-code instantiation of InMemoryApprovalStore would fail
this. InMemory is test-only by contract.
Combined with the previous two commits (validate-pinned at submit,
fail-closed on submit failure), the production trust boundary is now
the SQLite store and the wiring cannot be silently lost.
Blocker 1 of 3 review blockers. All three blockers now resolved.
Targeted suite: 305 passed, 3 xfailed.
…llback)
Concern from review v2: if GatewayRunner.__init__ tries to wire
SqliteApprovalStore and the wiring raises (DB path unwritable, NFS
locked, OOM at boot), the previous behavior left _default_approval_store
at None and logged ERROR. Subsequent gateway-context approval requests
would then take the 'store is None' branch in _await_gateway_decision
and SILENTLY DEGRADE to the legacy in-memory FIFO. That is exactly the
silent-degradation pattern the rewrite was meant to eliminate.
Fix: separate 'no store by design' (tests, scripts, non-gateway flows)
from 'store init failed at boot' (production mis-wiring). Gateway boot
now marks the latter; _await_gateway_decision uses the mark to decide
between legacy-fallback (OK for non-gateway) and fail-closed (required
for production).
tools/approval.py:
- New module-level flag _approval_store_init_failed +
_approval_store_init_failure_reason.
- Helpers: mark_approval_store_init_failed(reason),
clear_approval_store_init_failed(), is_approval_store_init_failed().
- set_default_approval_store(non-None) clears the flag — this is the
recovery path after a failed boot.
- _await_gateway_decision: if store is None AND init-failed flag is
set, return {resolved: False, store_failed: True} immediately
before any queue/notify side-effects. Operator log line names the
recorded failure reason for diagnosis.
gateway/run.py: the except block around the SqliteApprovalStore
wiring now calls mark_approval_store_init_failed(str(exc)) AFTER
logging. Gateway continues starting (other features work) but
dangerous-command approval is hard-stopped until operator restores
wiring.
Tests (2 new in test_failclosed_paths.py):
- test_init_failure_fails_closed_no_silent_legacy_fallback: with
init-failed flag set + store=None, _await_gateway_decision returns
store_failed without queueing any entry or invoking notify_cb.
- test_init_failure_recovers_when_store_re_wired: installing a real
store via set_default_approval_store clears the flag.
Targeted suite: 307 passed, 3 xfailed.
Concerns from review v2:
NousResearch#1 silent degradation on store init failure → FIXED (this commit)
NousResearch#2 ApprovalProposal validation in non-gateway flows → VERIFIED:
ApprovalProposal only constructed in tools/approval.py:1497
(gateway flow) and tools/approval_store_sqlite.py:322
(deserialize); no non-gateway code constructs it.
NousResearch#3 set_default_approval_store global + test leakage → VERIFIED:
all 6 test files with approval-store usage have autouse
fixtures restoring the global between tests.
NousResearch#4 SQLite path uses get_hermes_home() — already verified.
NousResearch#5 High-risk mapping scope — already balanced per test_phase4.
…ecuted
Addresses two real-bugs from review v2 self-review (Risk 1 + 2) which
should not be normalised in the very PR that hardens approval. The PR
must not ship with 'audit says approved but command never ran' as a
known/accepted behavior.
Risk 1 — Phase 3 override left audit trail misleading
-----------------------------------------------------
Before:
user clicks /approve <id>
→ resolve_gateway_approval_by_id calls store.consume(id) → status='consumed'
→ agent thread wakes; Phase 3 guard detects runtime stricter
→ choice overridden to 'deny'
→ check_all_command_guards returns BLOCKED, command NOT executed
→ store row STILL shows status='consumed' with no audit signal that
execution was actually blocked
Retrospective incident review six months later sees 'consumed' and
infers the command ran. It didn't.
Fix:
- New columns execution_status / execution_reason / execution_recorded_at
on gateway_approvals table. Idempotent migration via PRAGMA-checked
ALTER TABLE for pre-existing DBs.
- ApprovalProposal gains matching fields with defaults
(execution_status='not_started').
- New ApprovalStore.mark_post_consume(approval_id, executed, reason)
method on both backends. Updates only rows in status='consumed'.
- _await_gateway_decision now records the post-consume outcome:
user_choice='deny' → status=denied, execution_status stays not_started
user_choice approve + Phase 3 OK → execution_status='executed'
user_choice approve + Phase 3 override → execution_status=
'blocked_after_consume', reason='phase3_runtime_stricter'
- Distinguishes user_choice (entry.result, raw click) from final
choice (post-Phase-3 outcome) so the audit pinpoints exactly what
happened.
- Preserves the existing store.consume/store.deny mirror so legacy
resolve_gateway_approval callers (platform inline-buttons in
Telegram/Slack/Matrix/QQBot/Feishu/api_server) still get the row
transitioned. The execution_status is the new layer on top.
Risk 2 — orphan-consumed UX falsely said 'resuming'
---------------------------------------------------
Before:
/approve <id> with no live waiter (gateway restart, agent run done)
→ resolve_gateway_approval_by_id returned 1
→ handler returned 'gateway.approve.once_singular' =
'✅ Command approved. The agent is resuming...'
→ user inferred command ran. It didn't.
Fix:
- resolve_gateway_approval_by_id returns -1 (new sentinel) for orphan
case, distinct from 1 (signalled waiter) and 0 (rejected).
- Records execution_status='blocked_after_consume',
reason='orphan_no_waiter' so audit is consistent with Risk 1 model.
- gateway/run.py _handle_approve_command + _handle_deny_command route
count==-1 to new locale keys gateway.approve.orphan_consumed /
gateway.deny.orphan_denied. Text states explicitly the command was
NOT executed and instructs user to re-issue.
Tests
-----
tests/tools/test_audit_execution_status.py (5 new):
- user_approve_then_executes_records_executed
- user_approve_blocked_by_phase3_records_blocked_after_consume
- user_deny_records_not_started
- orphan_consumed_records_blocked_after_consume
- handler_orphan_message_signals_command_not_executed (grep guard
on gateway/run.py + locale)
test_resolve_by_id.py orphan test renamed +-extended:
- test_resolve_by_id_orphan_consumes_store_but_returns_minus_one
asserts return == -1 AND execution_status == 'blocked_after_consume'
AND execution_reason == 'orphan_no_waiter'.
Schema migration
----------------
_migrate_existing_schema() runs PRAGMA table_info + ALTER TABLE ADD
COLUMN for the three new columns if absent. Existing state.db files
created by earlier hermes versions upgrade in place on first
SqliteApprovalStore() construction. No SCHEMA_VERSION bump needed
because gateway_approvals is self-contained.
Targeted suite: 312 passed, 3 xfailed. Inga regressioner.
Risk 3 (init timeout), Risk 4 (session-end cruft), Risk 5 (schema
cache caveat), and the other nice-to-haves remain in HANDOFF.md as
follow-ups per Hermes review v2 directive.
tonydwb
reviewed
Jun 7, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Reviewed PR #41427: Harden gateway command approvals with durable atomic approval store
Decision: COMMENT — high-surface-area hardening PR (17 files, ~3.7k additions). The diff can’t be fully verified in this scheduled run.
🔴 Reviewability Note
- This PR rewrites the approval-store contract and rewires the gateway runtime; it was not possible to review every path in cron batch mode.
- Treat this as a review placeholder; please ensure a human review covers transactional correctness, memory-store fallout, and fail-close semantics.
Reviewed in batch on 2026-06-08
alt-glitch
added
type/security
…TER ALTER
Real blocker from review v3 (Hermes self-test on pre-existing
state.db): the previous _ensure_schema flow was
conn.executescript(SCHEMA_SQL) # CREATE TABLE + CREATE INDEX
_migrate_existing_schema(conn) # ALTER TABLE for new columns
which fails on an upgrade with:
sqlite3.OperationalError: no such column: execution_status
because SCHEMA_SQL contained both the CREATE TABLE IF NOT EXISTS
(a no-op on the existing table that lacks execution_*) AND the new
partial index CREATE INDEX ... ON gateway_approvals(execution_status)
WHERE execution_status = 'blocked_after_consume', which is evaluated
before the ALTER TABLE that adds the column.
Fix: split into CREATE_TABLE_SQL + INDEX_SQL and run in three steps:
1. conn.executescript(CREATE_TABLE_SQL) # new DBs get full shape
2. _migrate_existing_schema(conn) # old DBs get missing cols
3. conn.executescript(INDEX_SQL) # indices on guaranteed-present cols
SCHEMA_SQL kept as a back-compat alias (= CREATE_TABLE_SQL + INDEX_SQL)
for any external callers that imported the constant.
Regression test:
test_approval_store_sqlite.py::test_upgrade_from_pre_execution_columns_schema
- Builds an OLD-schema DB directly via raw SQL (matches the shape an
earlier deploy left on disk)
- Seeds a pre-existing pending row to ensure migration doesn't drop data
- Opens via NEW SqliteApprovalStore — schema upgrades transparently
- Verifies: row still present, new execution_* fields at defaults,
new partial index now exists, consume + mark_post_consume work
end-to-end against the migrated DB
This is the production-upgrade path; the previous flow would have hit
this exact error on first SqliteApprovalStore() call against an
existing state.db. Catching it pre-merge prevents a deploy-time
incident.
Targeted suite: 313 passed (+1 new test), 3 xfailed.
miclloyd
marked this pull request as ready for review
June 8, 2026 04:52
miclloyd
force-pushed
the
michael/gateway-approval-hardening
branch
from
June 8, 2026 11:36
3410a59 to
2c1904d
Compare
teknium1
reviewed
Jul 14, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the substantial hardening work. The current-main FIFO premise is real, but two blocking paths remain.
Problems
tools/approval.py:1777-1780keeps the FIFO resolver for inline buttons/API. Existing adapters callresolve_gateway_approval(session_key, choice)without a durable proposal id (for exampleplugins/platforms/telegram/adapter.py:5402-5403), so a button can still resolve the oldest queued approval rather than the prompt clicked. This bypasses the PR's per-id guarantee on those surfaces.tools/approval.py:1815marks an approval asexecutedbefore the guard returns. Terminal dispatch occurs only after the guard (tools/terminal_tool.py:2282-2286), so a later cancellation or dispatch failure can leave a false execution audit record.
Suggested changes
- Thread the durable approval id through
send_exec_approval, all button/API callbacks, and resolve throughresolve_gateway_approval_by_idfor every store-backed gateway path. - Record
executedonly at the actual execution boundary, with a distinct non-executed result for post-approval dispatch failures.
Automated hermes-sweeper review.
| try: | ||
| from tools.approval import set_default_approval_store | ||
| from tools.approval_store_sqlite import SqliteApprovalStore | ||
| set_default_approval_store(SqliteApprovalStore()) |
Contributor
There was a problem hiding this comment.
Wiring the store here makes it production-default, but every existing button/API handler still calls the FIFO resolve_gateway_approval(session_key, choice) without the durable id. Thread approval_data["approval_id"] through send_exec_approval and every callback before enabling this store, otherwise button clicks remain able to release the oldest queued approval.
| else: | ||
| # Approve confirmed by Phase 3 — agent thread about | ||
| # to release for execution. | ||
| store.mark_post_consume(approval_id, executed=True) |
Contributor
There was a problem hiding this comment.
This records executed before _await_gateway_decision() returns to the terminal/code-execution caller; the command has not executed yet. Move this audit transition to the actual dispatch/completion boundary and record a non-executed outcome if execution cannot begin.
teknium1
added
sweeper:risk-security-boundary
Contributor
|
too large to review safely This PR changes 1657 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-luna-high in Codex |
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
5 participants
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
Replaces the in-memory
dict+threading.Lockapproval storage intools/approval.pywith a SQLite-backed durable store. The change closesa class of safety gaps where dangerous-command approvals could be (a)
lost on gateway restart, (b) raced across processes, (c) silently
downgraded by config drift between proposal and execution, (d)
approved without the user reading the specific id, or (e) audited as
"approved" when post-consume guards actually blocked execution.
What changes
SqliteApprovalStoreas the production default. Tablegateway_approvalsinside the existing Hermesstate.db. Atomicconsume via
BEGIN IMMEDIATE+UPDATE … WHERE status='pending' … RETURNING. Idempotent schema with PRAGMA-checked ALTER TABLE forpre-existing DBs. Wired in
GatewayRunner.__init__. Boot-timewiring failure marks the store init-failed; subsequent gateway-
context approvals fail closed (no silent degradation to legacy
in-memory FIFO).
/approve <id>is the only accepted form. No FIFO. No/approve all. Even with exactly one pending approval, no-id refuses.Cross-session id-spoofing rejected via persisted
session_keycheck.ApprovalProposal.__post_init__raisesValueErroron malformedpayloads — empty
risk_reason, invalidrisk_level,high-risk-without-default-deny, etc.
re-classifies the command at execution-thread wake-up. If the live
classifier ranks it higher than the pinned risk, the consumed
approval is overridden to deny.
execution_status/execution_reason/execution_recorded_aton
gateway_approvals.status='consumed'records that the userclicked /approve;
execution_statusrecords whether the commandactually ran (
executed), was blocked by Phase 3 / orphan / etc(
blocked_after_consume), or never reached execution because theuser denied (
not_started). Retrospective audit can answer"did this approval lead to an execution?" unambiguously.
/approve <id>consumes a store row but no live waiter exists (gateway restart,
agent run finished), the handler returns
gateway.approve.orphan_consumed("Approval recorded, but theoriginal command session is no longer active. The command was NOT
executed.") instead of the misleading "✅ Command approved" message.
store.submitraises (DBlocked, disk full, validation refused), gateway returns BLOCKED
with "approval store unavailable" message. No
_ApprovalEntryqueued, no
notify_cbinvoked, no legacy fallback.risk_level == 'high'prompts include🚨 HIGH RISK,Default: DENY, exact command, cwd/backendcontext, pinned risk reason, the approval id, and either an inline
diff/summary or an explicit
NOT AVAILABLE — approve only if you have independently reviewedwarning.
ApprovalStoreContractmixin intests/tools/approval_store_contract.pyparameterised over bothInMemoryApprovalStore(xfail-strict on persistence gaps, bydesign) and
SqliteApprovalStore(passes all). Future backendsmust satisfy the same contract to be usable.
Storage rationale
The previous module-level
dict+threading.Lockstorage gavewithin-process atomicity but failed cross-process consume, lost state
on gateway restart, and had no audit trail. SQLite gives all of these
without a custom locking protocol — explicitly the spec required no
"best effort JSON + sidecar lockfile" approach.
Documentation:
docs/security/gateway-approval-lifecycle.md.Test results
The 3 xfailed-strict are the documented in-memory backend contract
gaps (persistence + cross-instance atomicity + pinned-payload roundtrip
through a second store instance). They MUST remain xfailing — if they
start passing it means the in-memory backend grew unauthorised
persistence behavior. The
SqliteApprovalStorepasses the same threetests.
Broad suite (
pytest tests/ -k "approval or approve or gateway") hasunrelated optional-dependency failures (matrix/telegram extras not in
[dev]) and a handful of pre-existing test-isolation issues that passwhen run individually. Documented in HANDOFF.md in the local-only patch
artifact bundle.
Test plan (post-merge, before deploy)
/approvewithout id is rejected/approvewith wrong id is rejected (and the real proposal stayspending)
/approvewith correct id executes the command exactly once/approve <id>post-restart consumes the row but returnsorphan-message; no command runs;
execution_status='blocked_after_consume'return BLOCKED with
approval store unavailablemessageclassifier to high-risk, /approve →
status='consumed'butexecution_status='blocked_after_consume'withexecution_reason='phase3_runtime_stricter'Status
Draft PR. Self-review complete; two real-bugs found and fixed in the
audit/UX layer (see
fix(audit)commit). Awaiting maintainer review.Not yet deployed to any production gateway.
🤖 Generated with Claude Code