fix: salvage 4 standalone fixes from superseded memnotif branch (tokens/config/plugins/tui) - #4
Closed
sam7894604 wants to merge 4 commits into
Closed
fix: salvage 4 standalone fixes from superseded memnotif branch (tokens/config/plugins/tui)#4sam7894604 wants to merge 4 commits into
sam7894604 wants to merge 4 commits into
Conversation
…e_token_counts The 0.17 feature port set billing_provider/base_url/mode via COALESCE(?, billing_field) (last-writer-wins), clobbering upstream 0.18's COALESCE(billing_field, ?) (first-writer-wins) which fixes dashboard cost misattribution on mid-session provider switch (NousResearch#48248). Session-split sets the new billing route on the fresh row via split_session, so token accumulation must not overwrite it. Restores upstream semantics; the explicit-overwrite path remains update_session_billing_route. Fixes test_update_session_billing_route_overwrites_after_switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hermes config set only coerced bool/int/float; a list or mapping literal
(e.g. platform_toolsets.line set to a JSON-style list) was stored as a
raw STRING with no warning. Every reader gated on isinstance(..., list)
— _get_platform_tools, _get_enabled_set, _get_disabled_set — then
silently ignored the value and fell back to its default, so the setting
looked saved but never took effect (observed in the wild: a platform
running on the wrong toolset bundle for weeks, and a plugins.enabled
entry that never enabled anything).
Values starting with '[' or '{' are now parsed with yaml.safe_load;
non-list/dict results and YAML errors warn on stderr and keep the
legacy string behavior.
…o the real switch
`hermes plugins enable gemini` printed a green success message and wrote
model-providers/gemini into plugins.enabled — but nothing ever reads that
flag for model providers: the general loader explicitly skips
kind: model-provider (handled by providers/__init__.py's own discovery,
selected via `hermes model` / model.provider) and kind: exclusive
(activated via `<category>.provider`). Same for disable: the entry lands
in plugins.disabled, the loader records it for introspection, and the
provider registers anyway.
The success message misleads users into believing they switched a
provider on or off. Found in the wild: a config with
model-providers/gemini in BOTH plugins.enabled (as a stray string) and
plugins.disabled, while the gemini provider had been registered and
usable the whole time.
enable/disable now detect the manifest kind and print what actually
controls the plugin, changing nothing:
! model-providers/gemini is a model provider — it is not controlled by
plugins.enabled/disabled (providers register automatically at startup).
To use it: run `hermes model` and pick it, or set model.provider.
To stop using it: select a different provider; remove its API key.
Nothing was changed.
_load_memory_notifications() read display.memory_notifications directly, bypassing per-platform overrides. Route it through resolve_display_setting (default platform_key "cli", the TUI's single surface tier) so it honors display.platforms.<platform>.memory_notifications the same way the messaging gateway now does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR bundles four independent changes: config value parsing now interprets bracketed/brace-prefixed strings as YAML lists/dicts with fallback warnings; plugin enable/disable commands refuse to act on "passive" plugin kinds (model-provider, exclusive); token count updates preserve existing non-NULL billing fields; and memory notification display resolution now supports per-platform overrides. ChangesConfig List/Map Literal Parsing
Passive Plugin Kind Handling
Billing Field Preservation in Token Updates
Per-Platform Memory Notification Display Setting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant cmd_enable
participant _plugin_kind
participant _print_passive_kind_hint
User->>cmd_enable: enable plugin key
cmd_enable->>_plugin_kind: read manifest kind
_plugin_kind-->>cmd_enable: kind (e.g., model-provider)
alt kind is passive
cmd_enable->>_print_passive_kind_hint: print guidance
cmd_enable-->>User: return without change
else standalone
cmd_enable-->>User: enable succeeds
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hermes_cli/plugins_cmd.py (2)
885-889: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the duplicated passive-kind early-exit.
The identical guard appears verbatim in
cmd_enableandcmd_disable. A small helper keeps the two paths from drifting.♻️ Suggested helper
def _refuse_if_passive(console, key: str) -> bool: kind = _plugin_kind(key) if kind in _PASSIVE_PLUGIN_KINDS: _print_passive_kind_hint(console, key, kind) return True return FalseThen each command becomes:
if _refuse_if_passive(console, key): returnAlso applies to: 969-973
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hermes_cli/plugins_cmd.py` around lines 885 - 889, Extract the duplicated passive-plugin early-exit logic from cmd_enable and cmd_disable into a shared helper, such as _refuse_if_passive, that wraps _plugin_kind, checks _PASSIVE_PLUGIN_KINDS, and calls _print_passive_kind_hint before returning whether to exit. Then update both command paths to call this helper and return early when it says to stop, so the behavior stays identical and won’t drift between the two functions.
823-839: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
_plugin_kindre-scans all plugins and swallows manifest errors.Both callers (
cmd_enablevia_resolve_plugin_key_and_source,cmd_disablevia_resolve_plugin_key) already trigger a full_discover_all_plugins()scan;_plugin_kindruns a third scan to re-find the same entry. Passing the already-resolveddir_pathwould avoid the extra filesystem walk.Separately, the broad
except Exception: return "standalone"silently downgrades a malformed manifest tostandalone, which would let a passive plugin fall through and write the exact deadplugins.enabled/disabledentry this change aims to prevent. Not blocking for a one-shot CLI, but worth a narrower catch (e.g.yaml.YAMLError,OSError) plus a warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hermes_cli/plugins_cmd.py` around lines 823 - 839, The `_plugin_kind` helper is doing a redundant full plugin rescan and silently masking manifest parse failures. Update `_plugin_kind` to accept the already-resolved plugin directory from `_resolve_plugin_key_and_source` / `_resolve_plugin_key` instead of calling `_discover_all_plugins()` again, and narrow the manifest exception handling around the `yaml.safe_load` path so malformed `plugin.yaml`/`plugin.yml` files are warned about rather than silently defaulting to `standalone`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hermes_cli/plugins_cmd.py`:
- Around line 885-889: Extract the duplicated passive-plugin early-exit logic
from cmd_enable and cmd_disable into a shared helper, such as
_refuse_if_passive, that wraps _plugin_kind, checks _PASSIVE_PLUGIN_KINDS, and
calls _print_passive_kind_hint before returning whether to exit. Then update
both command paths to call this helper and return early when it says to stop, so
the behavior stays identical and won’t drift between the two functions.
- Around line 823-839: The `_plugin_kind` helper is doing a redundant full
plugin rescan and silently masking manifest parse failures. Update
`_plugin_kind` to accept the already-resolved plugin directory from
`_resolve_plugin_key_and_source` / `_resolve_plugin_key` instead of calling
`_discover_all_plugins()` again, and narrow the manifest exception handling
around the `yaml.safe_load` path so malformed `plugin.yaml`/`plugin.yml` files
are warned about rather than silently defaulting to `standalone`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33c94e92-bfc5-447b-a641-06fe26010842
📒 Files selected for processing (6)
hermes_cli/config.pyhermes_cli/plugins_cmd.pyhermes_state.pytests/hermes_cli/test_config_set_list_values.pytests/hermes_cli/test_plugins_enable_passive_kinds.pytui_gateway/server.py
Owner
Author
Superseded — closingThe four fixes bundled here have each been routed to their proper destination, so this omnibus branch is no longer needed:
Closing as superseded. The branch |
sam7894604
pushed a commit
that referenced
this pull request
Aug 11, 2026
…on delegation callbacks (NousResearch#82592) * fix(gateway): stop frozen-preview finals and dropped idle-session delegation callbacks Two relay-plane delivery losses from the 2026-08-09 staging incident: 1. stream_consumer: the skip-redundant-finalize branch recorded _accumulated as the delivered turn-final payload even when the last ACKED edit was an earlier throttled preview snapshot, so delivered_final_matches reconciled True and the gateway suppressed the corrective final send — the user was left with a cut-off message ending in the streaming cursor. Extracted _mark_skip_redundant_finalize(): records the last acked wire payload (cursor-stripped), so a preview/final mismatch now returns False and the normal final send fires. 2. run.py: _classify_completion_target classified every ended parent session terminal unless it ended by compression. Idle/timeout session ends are the norm on scale-to-zero relay deployments and the chat route remains valid; completed async delegation results were terminally dropped. Ended parents now classify deliver unless the end was an explicit user boundary (session_reset / user_exit / session_switch). * fix(relay): drain in-flight outbound frames before transport teardown disconnect() failed every pending outbound future immediately with 'relay transport closed', so a trailing finalize edit racing turn teardown was lost even though the connector socket could still serve it. Bounded drain grace (5s) lets in-flight requests resolve; silent connectors still tear down promptly. asyncio.wait (not gather+wait_for) so a timeout doesn't cancel futures owned by the fail-remaining loop. * fix(gateway): route completion injection through the alias-aware transport resolver Third relay-plane delivery loss from the 2026-08-09 staging incidents: a delegation batch completed while the gateway was up, the watcher drained the event, and delivery vanished with no log line. _inject_watch_notification resolved its adapter with a literal p.value == platform_name scan of self.adapters — a relay-fronted gateway registers ONE adapter under Platform.RELAY fronting N logical platforms, so 'slack' never matched and the injection returned None ('no gateway route'), silently dropping the completion. The handoff path already documents this exact trap and uses resolve_delivery_transport; the injection path now does the same (native wins; relay eligible only when it fronts the logical platform), with the literal scan kept as fallback for stub runners and exotic platforms. * fix(relay): clamp disconnect drain grace to the runner's adapter-disconnect budget Review finding (JoaoMarcos44, NousResearch#82592): a fixed 5.0s drain in front of the three 1.0s sequential teardown awaits gives an 8.0s worst case inside the runner's 5.0s asyncio.wait_for(adapter.disconnect()) — tripping it cancels teardown mid-drain, skips the fail-pending loop, and leaves outbound callers blocked until _OUTBOUND_TIMEOUT_S (30s). The effective grace is now budget - 3*TEARDOWN - margin (env-aware via the same HERMES_GATEWAY_ADAPTER_DISCONNECT_TIMEOUT the runner reads), so the drain can never push teardown past its caller's budget; a budget too small for any drain disables it cleanly. * test(gateway): pin the final-send suppression contract across a behaviour matrix The gateway skips its own final send when the stream consumer claims the turn final already reached the user. Every incident in that family — NousResearch#71643 (stale finalize snapshot), NousResearch#78541 (payload-less multi-message split), NousResearch#82656 (frozen preview left with a visible cursor) — is the same failure: the consumer claimed delivery for text the platform never rendered, so the corrective send was suppressed and the answer was lost with no retry. Each was fixed with a scenario test pinned to one branch of GatewayStreamConsumer.run(). The got_done handler now has five sibling branches that each set the suppression flags and record a turn-final payload, and nothing checks them as a group: a new branch, or a new early `return True` in _send_or_edit, can reintroduce the class without failing a test. Pin the invariant instead of the branch — if the consumer offers the gateway any signal it would trust, the complete final text must have reached the wire — and assert it across {edit always / dies / never / lies} x {send always / never} x {fresh-final on / off} x {clean / interrupted stream}. The adapter records only frames that actually rendered, so an ACK the platform drops does not count as delivery. 24 honest-transport scenarios hold the invariant as a hard assertion. The 16 lying-transport scenarios are checked too; the single combination that still violates it is reported as an expected failure documenting the open exposure rather than asserting it away. Refs NousResearch#82656 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gateway,relay): prime relay egress routing for synthetic injections + cap stale completion replay Defect #4 from the 2026-08-09 staging incidents (upgrade-robustness): after every gateway restart the durable async-delegation replay injected completions correctly (post-741663cf1) but their replies bounced at the connector — 'slack egress declined: target not routed to an onboarded tenant'. The relay adapter re-attaches tenant discriminators (metadata.scope_id / metadata.user_id) from per-chat caches warmed ONLY by inbound traffic; synthetic turns race those cold caches on every deploy, scale-to-zero wake, and crash recovery. - relay adapter: prime_routing_cache() — feeds a synthetic event's session-store origin through the same _capture_scope used for real inbound (never raises). - run.py injection path: prime the resolved adapter before handle_message (duck-typed; native adapters unaffected). - async_delegation: 48h staleness cap in restore_undelivered_completions — a pending completion older than the cap is terminally dropped (payload stays queryable) instead of re-run as a fresh full-context turn; the post-restart replay of a July session burned a 102K-token context. Also carried: JoaoMarcos44's suppression behaviour-matrix harness (cherry-picked from NousResearch#82676, authorship preserved) — 39 passed + 1 xfail (the documented ACK-then-drop transport-honesty residue). * test: use recent timestamps in restored-ownership fixtures test_restore_stamps_restored_flag persisted its completion with epoch-era toy timestamps (dispatched_at=1.0), which the new 48h replay staleness cap correctly classifies as stale — the fixture then exercised the cap instead of the restored-flag contract (CI slice 4 failure). Timestamps are now now-relative; the staleness behavior itself is pinned separately in test_relay_injection_egress_priming.py. * fix(gateway,relay): close four review findings on the relay delivery fixes Review follow-ups on this branch (NousResearch#82592): 1. HIGH — classifier/resolver mismatch (falsely-acknowledged loss). _classify_completion_target now returns "deliver" for idle-ended parents, but _resolve_async_delegation_session still dropped every non-compression-ended pin: the durable row was acked at adapter acceptance, then the injection died inside the pipeline with no retry — strictly worse than the honest terminal drop on main, and the delivery leg defect #2's fix depends on did not exist. The resolver now retargets non-user-boundary ends (idle/timeout/ lifecycle) to the chat's current session — session_entry already IS the routing key's current session for the same chat — while user boundaries (session_reset / new_session / user_exit / session_switch) stay fail-closed. Both sides share one module-level _USER_BOUNDARY_END_REASONS so the verdict and the routing decision cannot drift again; a coherence test asserts deliver-verdicts resolve non-None across representative end reasons. 2. HIGH — drain clamp missed adapter-level spend. The effective drain grace budgeted drain + 3x teardown, but RelayAdapter.disconnect spends revocation-monitor teardown + go_idle time BEFORE the transport drain inside the same runner wait_for; worst case still blew the budget and cancelled teardown mid-drain (skipping the fail-pending loop). The adapter now measures its own elapsed time and threads the REMAINING budget into transport.disconnect(budget_s=...); legacy/stub transports without the keyword fall back to the no-arg signature. 3. P1 — _request_response racing disconnect() could register a future after the fail-pending loop already ran, stranding the caller for the full _OUTBOUND_TIMEOUT_S (30s). Fail fast with the same "relay transport closed" error once _closing is set. 4. P1 — _build_process_event_source's last-resort reconstruction dropped scope_id, so a scoped relay completion whose session-store origin was unavailable primed no tenant discriminator and could still bounce off the connector's fail-closed egress guard. scope_id now threads through the reconstructed SessionSource, with a warning when a scoped chat reconstructs without one. All four: RED reproduced with the fix reverted, GREEN after; relay/ delegation delivery families pass (43 + 71 + 179 across the touched suites); full tests/gateway run shows only failures already failing identically on merge base 2446c8b (env/dep issues). * fix(gateway,relay): make pending-frame failure cancellation-safe; persist completion routing origin Two remaining review findings on this branch (NousResearch#82592): 1. Cancellation could strand outbound waiters past the fail-pending loop. transport.disconnect() failed pending futures only at the END of the drain + three teardown awaits; a cancellation landing mid-drain (the runner's wait_for budget, an outer cleanup deadline) skipped the loop entirely and left registered futures unresolved — their callers blocked until _OUTBOUND_TIMEOUT_S (30s). The budget threading added earlier shrinks the window but is not a hard guarantee. The fail-pending loop (and the going_idle ack failure) now run in a `finally`, so no exit path — normal, error, or cancelled — can leave a registered future unresolved. Idempotent: done futures are skipped, a second disconnect() pass is a no-op. 2. Durable completions did not persist their routing origin, so the scope_id threading in the fallback SessionSource reconstruction had nothing to carry on the exact path it exists for (restart replay with session store + source cache gone): the async-delegation event producers never populated scope_id and the durable rows never stored it. Dispatch now snapshots the originating turn's scope_id/user_id/user_name from the session context (_capture_routing_origin — a new HERMES_SESSION_SCOPE_ID contextvar bound by the gateway at session-bind time alongside the existing vars), stores them in the existing task_json payload (no schema migration), and re-attaches them to all three completion-event shapes (live single, live batch, crash-recovery rebuild). The gateway's fallback reconstruction then primes both discriminators after a restart. Tests: cancellation mid-drain -> every pending future resolves with "relay transport closed" (mutation: moving the loop out of the finally goes RED); second-pass disconnect idempotence; end-to-end dispatch -> owner-death recovery -> event carries scope_id -> fallback SessionSource primes it (mutations: dropping the dispatch capture or the task_json persistence both go RED); live completion event carries the origin. 94 passed + 1 xfailed across the delivery/delegation suites; tests/tools delegation family 73 passed (2 collection errors pre-existing on merge base 2446c8b). --------- Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Ben Barclay <ben@nousresearch.com>
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.
Cherry-picks the genuinely-still-needed fixes out of the older
fix/memory-notifications-per-platformbranch onto the freshly-rebased upstream-alignedmain, dropping everything that is now redundant or superseded.What's included (4 commits, each verified still needed on the new upstream HEAD)
fix(tokens)— restore first-writer-wins billing inupdate_token_counts(hermes_state.py). New main currently hasCOALESCE(?, billing_provider)(last-writer-wins); this restoresCOALESCE(billing_provider, ?).fix(config)— parse list/mapping literals inhermes config set(hermes_cli/config.py). Absent from new main. +dedicated test.fix(plugins)— refuse enable/disable on passive plugin kinds with a pointer to the real switch (hermes_cli/plugins_cmd.py). Absent from new main. +dedicated test.fix(tui)— resolvememory_notificationsper-platform for parity with the gateway (tui_gateway/server.py). The gateway resolve lives in PR feat: unified per-platform proactive-push opt-out gate (cron + review + kanban + restart) #3 (ppg); this is the TUI half, which ppg does not touch.What was intentionally dropped
port: rebase fork feature set onto upstream 0.18.0— redundant;mainnow replays the 37 fork commits individually onto a newer upstream HEAD.memory_notificationsregister/gateway-resolve commits — superseded by PR feat: unified per-platform proactive-push opt-out gate (cron + review + kanban + restart) #3 (per-platform proactive-push gate), which carries a more complete version (proactive_pushkey + 3-mode + layered gate +effective_memory_notifications).fix(toolsets)static bundles for 8 platforms — verified REDUNDANT: on the new upstream HEAD,toolsets.resolve_toolset()has an auto-generation fallback that returns_HERMES_CORE_TOOLSfor any registered plugin platform. Confirmed decisively: after registering a platform inplatform_registry,resolve_toolset('hermes-line')returns the 49 core tools — identical to what the static bundle would give.The original
fix/memory-notifications-per-platformbranch is left untouched on the fork as historical record (and it remains deployed on the production host).Validation
test_config_set_list_values+test_plugins_enable_passive_kinds+test_display_config(memory_notifications per-platform).Summary by CodeRabbit
New Features
Bug Fixes