fix(actuation): preserve original origin through confirmation re-entry#167
Merged
ai-hpc merged 1 commit intoMay 25, 2026
Merged
Conversation
Contributor
|
Reviewed and merged: #166 is valid, and this preserves the original actuation origin through confirmation while avoiding double-charge regressions covered by dispatcher tests. Merged at a46222b; thanks @galuis116. |
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
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
Preserve the originating channel (
Voice,Telegram,Dashboard,Api,Repl) when a pending sensitive home action is executed via the confirmation flow. Previously,ToolDispatcher::confirm_pending_home_action()rebuilt theToolExecutionContextwithrequest_origin: RequestOrigin::Confirmation, which silently bypassed[core.actuation_safety].max_actions_per_minute_by_origin, hid the originating channel insafety/actuation-audit.jsonl, and made[core.actuation_safety].allowed_originsoverrides all-or-nothing for the entire confirmation flow. Theconfirmed: trueflag (added in PR #141) is now the only signal the policy gate needs to know the action is pre-approved.Fixes #166 .
Changes
crates/genie-core/src/tools/dispatch.rs::confirm_pending_home_action(L786-823): passpending.requested_byas the re-entryrequest_origininstead of the hardcodedRequestOrigin::Confirmation. Added a multi-line comment explaining the invariant so future readers don't re-introduce the override.crates/genie-core/src/tools/dispatch.rs::exec_home_control_inner(L609-615): whenexec_ctx.confirmed == true, skipActuationRateLimiter::check_and_record. The original-origin bucket already paid one slot on the request that returnedConfirmationRequired; the confirmation re-entry must not double-charge the same logical action.crates/genie-core/src/tools/dispatch.rstests: added aSensitiveHomeProviderfixture (returnsvoice_safe: false,domain: "lock") plus two helpers (extract_confirmation_token,audit_events_matching) so the confirmation path can be exercised end-to-end inside#[tokio::test].testsmodule:confirm_preserves_original_origin_in_audit_log— reads back the actuation audit JSONL and asserts theExecutedrow'sorigin == "telegram"(was"confirmation").confirm_does_not_bypass_per_origin_rate_limit— withmax_actions_per_minute_by_origin = { telegram = 1 }, the second Telegram-initiated sensitive request inside the window is rejected with a "rate limit" message; only the first action reaches the HA provider.confirm_does_not_double_charge_when_already_paid— withmax_actions_per_minute_by_origin = { telegram = 2 }, request → confirm → request succeeds, proving the confirm did not consume the second slot.No production-config defaults change. The behaviour of direct (non-confirmation) actuations is unchanged.
Real Behavior Proof
#[tokio::test]against a mockHomeAutomationProvider. The equivalent verification path is the new in-process tests below plus the existinghome_control_rate_limits_by_origin,home_control_respects_configured_allowed_origins, andaction_history_hydrates_from_audit_logtests, all running green.What I ran
What I observed
Audit row now carries the originating channel. Inside
confirm_preserves_original_origin_in_audit_log, a Telegram-initiated sensitivehome_control { entity: "front door", action: "lock" }is issued, then confirmed viaconfirm_pending_home_action(token). The actuation audit JSONL contains exactly one"status": "executed"row and itsoriginis"telegram"— the value the test asserts. Before this patch the same setup producedorigin: "confirmation", which is what the bug report attached to this PR documents.Per-origin rate limit is now honoured through the confirmation flow. Inside
confirm_does_not_bypass_per_origin_rate_limit, withmax_actions_per_minute_by_origin = { telegram: 1 }:ConfirmationRequired(charges slot 1).confirmed-guard inexec_home_control_inner).output.to_lowercase().contains("rate limit"). Only one action reached theSensitiveHomeProvider::execute. Before this patch the second request succeeded because it would have been routed through theConfirmationbucket (default global 12/min).No double-charge on confirm. Inside
confirm_does_not_double_charge_when_already_paid, withtelegram = 2: request → confirm → request succeeds, proving the confirm did not push a second timestamp into the Telegram bucket. Before this patch the second request would have been rejected (bucket would be at 2/2 after the confirm: 1 for the issue, 1 for the re-entry).No direct-path regression. The pre-existing
home_control_rate_limits_by_origin(twoRequestOrigin::Dashboardcalls withmax_actions_per_minute_by_origin = { dashboard: 1 }, second must be rate-limited) still passes, as doeshome_control_respects_configured_allowed_originsandhome_control_records_action_history. Fullcargo testmatches the main-baseline 608 / 0 / 3.Test plan
A reviewer can re-verify on any Rust 1.85+ host (no Jetson, no Home Assistant, no audio needed):
cargo test -p genie-core --lib tools::dispatch::tests::confirm— three tests, ~1s, all green.cargo test -p genie-core --lib tools::dispatch::tests::— 29 tests, all green.genie-core:lock.*,cover.*garage, oralarm_control_panel.*).geniepod.toml, set[core.actuation_safety]withenabled = true,max_actions_per_minute_by_origin = { telegram = 1 }, defaultallowed_origins.genie-coreagainst your dev HA (HA_TOKEN=… cargo run --bin genie-core).Pending token: act-…back.curl -X POST "http://127.0.0.1:3000/api/actuation/confirm?token=act-…"— the action executes.tail -n 2 data/safety/actuation-audit.jsonl— the"status":"executed"row carries"origin":"telegram"(was"confirmation"on main).Confirmationbucket, not theTelegrambucket).Notes for reviewers
RequestOrigin::Confirmationis now unwritten in production code — the only writer was the hardcoded re-entry this PR removes. The variant is left in the enum for serde-backward-compat with any historical entries that may already be on disk insafety/actuation-audit.jsonlfrom operators running pre-fix builds. A follow-up PR to delete the variant + itsDisplay/as_policy_keybranches + the"confirmation"entry in defaultallowed_originscan land cleanly once we're comfortable that no one needs to read those historical rows back through the enum. Kept out of this PR to minimise rebase churn against test(home): regression for confirmed sensitive action at policy gate (closes #162) #163 and to keep the diff focused on the behavioural fix.test(home): regression for confirmed sensitive action at policy gate) merges. That PR scaffolds the gate-side test for the same flow; rebasing this PR on top of it is cheaper than the reverse and keeps the new dispatcher tests adjacent to test(home): regression for confirmed sensitive action at policy gate (closes #162) #163's gate test ingit log.origin: "confirmation"on rows their old build wrote; only NEW confirmed executions carry the original channel. If a "rewrite old rows" migration is wanted, it should be its own PR.[core.actuation_safety]overrides see no difference. Operators who did configuremax_actions_per_minute_by_originnow have those limits actually enforced through the confirmation flow — that is the bug fix, and is the only operator-visible behavioural change.