gateway: admit completion events by the session store's key; bound permanent route mismatches (#348) - #352
Conversation
…rmanent route mismatches (#348) A finished background delegation could be refused forever: the adapter rebuilt the session key from its own per-platform thread policy while the key on the event had been minted by the session store from the top-level config, and a refused admission refunded its delivery attempt, so the 8-attempt cap never fired and the watcher retried every 2 s. - base.py: when the adapter-derived key differs from the expected key, accept the event if the session store's own key for the source matches; a true mismatch is flagged on the event. - run_notifications.py: a flagged permanent mismatch is not re-raised as WakeNotAccepted, so the caller's release path spends an attempt and the existing cap parks the row as dropped with the existing single warning. Busy / pending-slot / missing-handler refusals keep their refund. - tests: T1 (store-key admission under a per-platform policy split) and T2 (permanent mismatch exhausts the 8 attempts).
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: electricsheephq/evaOS-hermes-desktop-app-adapter/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
૮ >ﻌ< ა ci reviewran on 20e5fab — gateway: a permanent route mismatch on the non-durable watch
|
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #352 - gateway: admit completion events by the session store's key; bound permanent route mismatches (#348)
Head: 83ea96c3196fdac62f8367490304708e6647b7f9 into release/runtime-r33.3. Review event: COMMENT.
Estimated review effort: 1/5 (~16 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
gateway/platforms/base.py |
modified | +9/-3 | Changed file | Low |
gateway/run_notifications.py |
modified | +1/-1 | Changed file | Low |
tests/gateway/test_completion_admission.py |
modified | +82/-0 | Test coverage | Low |
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Internally routed events whose adapter-derived session key differs from the expected key are now admitted when the session store independently derives the expected key from the event source.
- A confirmed route mismatch no longer refunds a durable delivery claim; repeated attempts can reach the existing eight-attempt terminal drop state.
- Added coverage for adapter/store thread-policy disagreement and permanent route-mismatch exhaustion.
Affected invariants:
- Route admission remains fail-closed unless either the adapter-derived key or the session-store-derived key matches the expected session key.
- Ordinary WakeNotAccepted outcomes still propagate for durable callers; only events explicitly marked as route mismatches are converted to failed attempts.
- Permanently misrouted completions are not delivered and are eventually bounded by the durable attempt limit.
Evidence:
- gateway/platforms/base.py:3519
- gateway/run_notifications.py:1088
- tests/gateway/test_completion_admission.py:124
- tests/gateway/test_completion_admission.py:166
Limitations:
- Per instruction, no tests, builds, package scripts, app commands, shell commands, or arbitrary PR code were executed.
- Review was limited to the supplied diff; surrounding implementations and CI results were not independently inspected.
No-finding rationale: The fallback comparison is fail-closed, uses the same event source after topic recovery, and marks only events that disagree with both key derivations. The notification change consumes attempts only for that explicit mismatch, while the added tests exercise successful store-key admission, single delivery, terminal exhaustion, and non-delivery. No validated correctness, security, data-loss, CI, release, or high-signal coverage defect is demonstrated by the provided diff.
Risk Taxonomy
No finding categories.
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #348.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83ea96c319
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…erminal drop for mismatched non-durable notices (#348 review round) - base.py: the session-store key fallback applies only to internal events. Heartbeat prompts are not internal and their bookkeeping keys on the store key, so admitting them under the adapter lane made them re-fire every poll on a host with a per-platform thread policy split. The route-mismatch flag is now reset on entry and the drop warning also prints the store key. - event.py: declare _gateway_route_mismatch like _gateway_accepted. - run_notifications.py: a flagged permanent mismatch returns a sentinel; a durable claim still spends its attempt (release path, existing cap), a non-durable interim notice is dropped instead of being requeued every 2 s, and a group whose final row is dropped is not requeued. - tests: heartbeat split regression (3 polls: 0 fires on the split host, 1 in the control); the existing wrong-route negatives now run with the real session store; T1 asserts the runner's key re-check; a shared thread created by one user and delegated by another queues behind the busy store session instead of starting a concurrent turn; a mismatched non-durable notice is not requeued once its final row is dropped.
There was a problem hiding this comment.
Walkthrough
PR: #352 - gateway: admit completion events by the session store's key; bound permanent route mismatches (#348)
Head: 51fcdd55284dbd81c05d110884863261e5cf1fc4 into release/runtime-r33.3. Review event: COMMENT.
Estimated review effort: 3/5 (~42 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
gateway/platforms/base.py |
modified | +10/-3 | Changed file | Low |
gateway/platforms/event.py |
modified | +1/-0 | Changed file | Low |
gateway/run_notifications.py |
modified | +5/-0 | Changed file | Low |
tests/gateway/test_completion_admission.py |
modified | +174/-1 | Test coverage | Low |
tests/gateway/test_heartbeat_split_admission.py |
added | +58/-0 | Test coverage | Low |
tests/gateway/test_plugin_message_injection.py |
modified | +2/-0 | Test coverage | Low |
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Internally routed events whose adapter-derived key differs from the expected key are now admitted when the session store derives the expected key from the same source.
- Rejected completion notifications now distinguish permanent route mismatches: durable claims consume the existing bounded attempt path, while non-durable notices are not requeued.
- Non-internal events, including heartbeat injections, cannot use the session-store fallback to cross an adapter-specific session split.
Affected invariants:
- Internal events remain fail-closed when neither the adapter nor session store derives the expected session key.
- Admission is acknowledged only after the real adapter accepts the synthetic event.
- A permanently invalid completion route cannot retry indefinitely or reach the message handler.
- Busy shared sessions remain serialized under the session-store key.
Evidence:
- gateway/platforms/base.py:3508 initializes per-admission receipts and lines 3521-3529 validate the store-derived key before accepting a divergent adapter route.
- gateway/run_notifications.py:1087-1091 classifies only adapter-reported route mismatches, and lines 1304-1307 map them into durable versus non-durable delivery outcomes.
- tests/gateway/test_completion_admission.py adds coverage for divergent thread policy, busy shared-session queuing, non-durable disposal, and eight-attempt durable exhaustion.
- tests/gateway/test_heartbeat_split_admission.py verifies that the fallback remains limited to internal events.
- tests/gateway/test_plugin_message_injection.py retains explicit rejection coverage with the session store wired.
Limitations:
- Review was limited to the supplied checkout diff; surrounding implementations were not inspected through shell commands.
- Per instruction, no tests, builds, package scripts, app commands, or arbitrary PR code were executed.
No-finding rationale: No validated correctness, security, data-loss, CI, release, or Unity regression was established from the supplied diff. The added tests target the principal high-risk boundaries introduced by the change, and the fallback remains restricted to internal events whose expected key independently matches the session store.
Risk Taxonomy
No finding categories.
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #348.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51fcdd5528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…on (#348 review round 3) On a host with a per-platform thread policy split, an admitted completion arrives on a different, idle adapter lane, so the adapter's own 'internal events never interrupt' rule (run_busy) is skipped and the inbound busy path routed it by the busy-input mode — steering or interrupting the other user's live turn. Internal events now always queue behind the running turn under the store key. Test: the shared-thread completion case is covered with a stub RUNNING agent (no interrupt/redirect/steer call; the event is queued as internal) in addition to the pending-sentinel variant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95308e9545
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Walkthrough
PR: #352 - gateway: admit completion events by the session store's key; bound permanent route mismatches (#348)
Head: 95308e9545bf60af911500116dba235498e6143e into release/runtime-r33.3. Review event: COMMENT.
Estimated review effort: 3/5 (~44 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
gateway/platforms/base.py |
modified | +10/-3 | Changed file | Moderate: validated P2 finding |
gateway/platforms/event.py |
modified | +1/-0 | Changed file | Low |
gateway/run_inbound.py |
modified | +1/-1 | Changed file | Low |
gateway/run_notifications.py |
modified | +5/-0 | Changed file | Low |
tests/gateway/test_completion_admission.py |
modified | +239/-1 | Test coverage | Elevated: large change |
tests/gateway/test_heartbeat_split_admission.py |
added | +58/-0 | Test coverage | Low |
tests/gateway/test_plugin_message_injection.py |
modified | +2/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Internal events whose adapter-derived route differs may now be admitted when the session store derives the expected key.
- Busy internal events are queued instead of interrupting, redirecting, or steering an active turn.
- Permanent completion-route mismatches now consume bounded durable delivery attempts and non-durable mismatches are discarded.
Affected invariants:
- One logical store session must use one serialization and bookkeeping key across adapter admission and runner execution.
- Completion acknowledgement must occur only after the intended session accepts the event.
- Permanent routing failures must not retry indefinitely.
Evidence:
- gateway/platforms/base.py:3518-3529 admits by
store_keybut retains the mismatchingsession_keyfor_active_sessions. - tests/gateway/test_completion_admission.py covers one split-policy event and already-busy sessions, but not concurrent events with distinct adapter-derived keys.
Limitations:
- Review was limited to the supplied diff; no commands, tests, builds, or additional repository inspection were performed.
No-finding rationale: No other validated correctness, security, data-loss, CI, release, or high-signal test defect was established from the supplied diff.
Risk Taxonomy
- Runtime correctness: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #348.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
…dropped, not requeued _inject_watch_notification recognised the permanent route mismatch only on the durable (raise_not_accepted) path. Non-durable watch_match and watch_disabled events fell through to 'return False', which the drain requeues, so a permanently mismatched watch event was re-injected and re-dropped on every tick. Check the permanent flag first for both paths: the durable caller still gets the sentinel; the watch caller gets None, which the drain drops with the existing warning.
There was a problem hiding this comment.
Walkthrough
PR: #352 - gateway: admit completion events by the session store's key; bound permanent route mismatches (#348)
Head: 20e5fab98f45db26f5e2078ca77391ce3b7c09bd into release/runtime-r33.3. Review event: COMMENT.
Estimated review effort: 3/5 (~44 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
gateway/platforms/base.py |
modified | +10/-3 | Changed file | Moderate: validated P2 finding |
gateway/platforms/event.py |
modified | +1/-0 | Changed file | Low |
gateway/run_inbound.py |
modified | +1/-1 | Changed file | Low |
gateway/run_notifications.py |
modified | +6/-1 | Changed file | Low |
tests/gateway/test_completion_admission.py |
modified | +269/-3 | Test coverage | Elevated: large change |
tests/gateway/test_heartbeat_split_admission.py |
added | +58/-0 | Test coverage | Low |
tests/gateway/test_plugin_message_injection.py |
modified | +2/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Internal events whose adapter-derived session key differs may now be admitted when the session store derives the expected key.
- Busy internal events are always queued instead of interrupting, redirecting, or steering the active agent.
- Confirmed route mismatches are classified separately so watch notifications are dropped and durable completion delivery is bounded by its attempt limit.
Affected invariants:
- Internal events must be admitted only to the session identified by durable routing metadata.
- Temporary admission failures must remain retryable; only proven permanent mismatches may be discarded.
- Completion events must not interrupt an active turn.
Evidence:
- gateway/platforms/base.py:3520-3525 collapses all store-key generation exceptions into the permanent-mismatch path.
- gateway/run_notifications.py:1088-1092 converts that flag into terminal behavior for ordinary watch delivery.
- gateway/run_notifications.py:1307-1308 returns failure for claimed durable delivery, allowing attempts to exhaust and the row to be dropped.
Limitations:
- Review was limited to the supplied checkout diff; no commands, tests, builds, or arbitrary repository code were executed.
- Runtime behavior of surrounding queue-drain and durable-claim code was not independently exercised.
No-finding rationale: No other correctness or security defect was validated from the supplied diff. The added tests cover store-key admission, busy-session queuing, ordinary mismatch dropping, and durable-attempt exhaustion, but they do not cover failure of store-key generation.
Risk Taxonomy
- Data loss: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #348.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
| return | ||
| try: | ||
| store_key = self._session_store._generate_session_key(event.source) if event.internal else None | ||
| except Exception: |
There was a problem hiding this comment.
P2: Store-key failures are treated as permanent route mismatches
Any exception while generating the session-store key is converted to None, after which _gateway_route_mismatch is set. The notification layer interprets that flag as permanent: ordinary watch notifications are discarded immediately and durable completions eventually exhaust their attempts and become dropped. An unavailable or unexpectedly failing session store therefore causes notification loss without proving a route mismatch. Only set the permanent flag after key generation succeeds and the generated key actually differs; preserve the retryable WakeNotAccepted behavior on exceptions, with an exception-path test.
Category: Data loss
Why this matters: A transient admission dependency failure can silently discard process-completion notifications that were previously retryable.
1 participant
Fixes #348 on the r33.x release line (base: the r33.6 tag
2d10969e).What was wrong
A finished background delegation's result could be refused forever. Two faults together:
BasePlatformAdapter.handle_messagerebuilt the session key from the adapter's per-platformthread_sessions_per_userpolicy, while the key stamped on the event had been minted bySessionStore._generate_session_keyfrom the top-level config. With a policy split (platform sectiontrue, top level unset) the keys differ and the event is refused (WakeNotAccepted).defer_completion_delivery), so the existing 8-attempt cap can never fire and_async_delegation_watcherretries every 2 s (measured: ~1,780 attempts/hour, attempts column pinned at 0, result delivered 0 times, gateway log retention collapsed).Change (≈14 non-test lines)
gateway/platforms/base.py: when the adapter-derived key differs fromexpected_session_key, accept the event if the session store's own key forevent.sourceequals the expected key; a true mismatch setsevent._gateway_route_mismatchand keeps the existing warning + return.gateway/run_notifications.py: a flagged permanent mismatch is not re-raised, so the caller's release path spends an attempt and the existing cap parks the row asdroppedafter ≤8 tries with the one existing "exhausted" WARNING. Busy / pending-slot / missing-handler refusals keep their refund.run_turn) still enforces routing, so this widens nothing beyond what the runner already accepts. Same path also stops heartbeat and plugin-injection events being dropped on threads with this policy split.Tests
tests/gateway/test_completion_admission.py:test_completion_accepts_session_store_key_when_adapter_thread_policy_differs— RED on the base (20× False, row pending, attempts 0, 20 route-drop warnings), GREEN now (delivered once, handler received the summary).test_permanent_route_mismatch_exhausts_delivery_attempts— RED on the base (pending forever, attempts 0), GREEN now (dropped, attempts 8, next call None, exactly one "exhausted" warning).Local acceptance bundle (admission, plugin injection incl.
test_base_adapter_rejects_derived_session_mismatch, completion delivery, heartbeat watch lifecycle, kanban wake acceptance, stop-thread sibling): 72 passed, 1 failed — the failure istest_push_receipt_requires_real_admission_without_displacing_user[True], pre-existing on the unmodified base in this venv (missing optional Raft platform library), unrelated.Rollout note (for the release visit that carries this)
Any
pendingdelegation row younger than 48 h will be delivered by the fixed gateway after its restart; query the fleet read-only forstate != 'running' AND delivery_state = 'pending'first, and let a row older than 48 h age out through the existing replay cap rather than delivering a stale result.Claim class:
tests_green_local. Not proven: behaviour on a live gateway (no box run).