feat: unified per-platform proactive-push opt-out gate (cron + review + kanban + restart)

[sam7894604]

Unified per-platform proactive-push gate

Lets a platform opt out of ALL unsolicited / background pushes with one setting,
enforced in code at every background delivery choke point — instead of each
subsystem inventing its own switch (previously: memory notifications had a
per-platform key; cron only had per-job deliver; kanban/restart had none).

Config

display:
  platforms:
    line:
      proactive_push: false   # LINE receives NO background push (cron, review,
                              # kanban, restart) — interactive replies unaffected
  # or globally: display.proactive_push: false

What's gated (all consult platform_accepts_proactive_push())

• cron delivery — _resolve_delivery_targets drops opted-out platforms
  (covers deliver=origin / all / explicit platform:chat — a platform-level
  "do not disturb" wins over a job's routing intent). Job still runs + saves
  last_output.
• background-review memory notifications — effective_memory_notifications()
  layers the two keys: delivered only when proactive_push != false AND
  memory_notifications != off. memory_notifications is now registered as a
  per-platform overrideable key (was global-only).
• kanban notifier — terminal-event pushes skipped for opted-out platforms.
• restart notice + home-channel startup broadcast — gated (unifies with the
  existing gateway_restart_notification flag).

Every skip is logged; all gates are fail-open (a config read error delivers
as before rather than silently dropping).

NOT gated (by design)

• Interactive replies (user message → agent response, slash output,
  streaming) — never consult the gate; it lives only in background paths.
• Background agent-task delivery — the result of a task the user explicitly
  requested (a delayed interactive reply), not an unsolicited push. Gating it
  would suppress an answer the user is waiting for. Left untouched, flagged.

Commits

1. feat(display) — add proactive_push key + register memory_notifications + platform_accepts_proactive_push() helper.
2. feat(cron) — gate cron delivery.
3. feat(gateway) — gate background-review via effective_memory_notifications().
4. feat(gateway) — gate kanban + restart notifications.

Tests

102 green across display_config (proactive_push resolution/normalisation, layered memory_notifications), cron gate (opt-out drops platform / keeps others / deliver=all still respects / global / fail-open / logged / integration), restart-notice suppression. No regression in restart (28) / kanban notifier (10) suites.

⚠️ HOLD upstream — fork feature branch. Deploy to toothless after review (set display.platforms.line.proactive_push: false, restore the sync job's deliver: origin, verify LINE silent + cron still runs).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added per-platform controls for proactive push notifications.
  • Added configurable memory notification modes: off, standard, and detailed.
  • Startup, restart, scheduled delivery, and background notifications now respect platform opt-out settings.

• Bug Fixes

  • Opted-out notifications and delivery targets are skipped cleanly, preventing repeated retries.
  • Memory notifications are automatically disabled when proactive pushes are turned off.

• Tests

  • Expanded coverage for platform-specific and global settings, fallback behavior, and opt-outs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds per-platform proactive-push controls for cron, kanban, restart, startup, and memory notifications. Adds shared configuration helpers and tests. Also adds a duplicate Cloudflare AI Gateway BYOK handling block.

Changes

Proactive-push opt-out feature

Layer / File(s)	Summary
Display config settings and helper functions gateway/display_config.py, tests/gateway/test_display_config.py	Adds defaults, normalization, precedence, and effective memory-notification resolution for proactive_push and memory_notifications.
Cron scheduler delivery target filtering cron/scheduler.py, tests/cron/test_proactive_push_gate.py	Filters opted-out targets before delivery selection. The filter fails open and logs skipped targets.
Kanban and notification delivery gates gateway/kanban_watchers.py, gateway/run.py, tests/gateway/test_restart_notification.py	Skips opted-out deliveries, advances skipped kanban cursors, and resolves memory notification modes per platform.
Cloudflare AI Gateway BYOK handling agent/agent_runtime_helpers.py	Adds a duplicate block that sets the Cloudflare authorization header and clears the API key for gateway URLs.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CronScheduler
  participant ProactivePushFilter
  participant ConfigLoader
  participant DeliveryTargets

  CronScheduler->>ProactivePushFilter: resolved targets and job
  ProactivePushFilter->>ConfigLoader: load_config()
  ConfigLoader-->>ProactivePushFilter: user configuration
  ProactivePushFilter->>DeliveryTargets: filtered targets

Loading

sequenceDiagram
  participant GatewayWatcher
  participant DisplayConfig
  participant SubscriptionCursor
  participant NotificationAdapter

  GatewayWatcher->>DisplayConfig: check proactive-push acceptance
  DisplayConfig-->>GatewayWatcher: accepted or opted out
  alt opted out
    GatewayWatcher->>SubscriptionCursor: advance skipped event
  else accepted
    GatewayWatcher->>NotificationAdapter: send notification
  end

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: a unified per-platform proactive-push opt-out gate across the listed delivery paths.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/per-platform-proactive-push-gate

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (5)

gateway/kanban_watchers.py (1)

293-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct fail-open gate, but no accompanying test coverage.

The gate logic is sound: config loaded once per tick, fail-open via _push_cfg = None (short-circuit in the boolean check on line 307 protects against referencing platform_accepts_proactive_push if the import itself failed), skip is logged, and the cursor is advanced so an opted-out event isn't replayed every tick — consistent with the existing unknown-platform-skip pattern at lines 320-327.

Per the PR stack, the sibling cron layer ships tests/cron/test_proactive_push_gate.py and the restart layer ships test_send_restart_notification_suppressed_by_proactive_optout, but this kanban gate has no corresponding test in this cohort. Given the multi-board tick loop and the cursor-advance-on-skip side effect, a regression here (e.g. gate silently not firing, or cursor not advancing) would be easy to miss without a targeted test.

Want me to draft a test exercising: (1) an opted-out platform's subscription is skipped and its cursor still advances, and (2) a config-load failure fails open and still delivers?

🤖 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 `@gateway/kanban_watchers.py` around lines 293 - 318, Add targeted test
coverage for the kanban proactive-push gate in kanban_watchers.py, focusing on
the delivery loop that uses _load_push_cfg, platform_accepts_proactive_push, and
_kanban_advance. Write a test that verifies an opted-out platform is skipped,
the skip is logged, and the cursor is still advanced so the event is not
replayed; also add a fail-open case where config loading raises and the code
still proceeds with delivery rather than blocking. Use the existing kanban
notifier flow and the multi-board tick behavior to locate the right branch.

cron/scheduler.py (1)

645-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid fail-open implementation.

Fail-open on config-load exceptions, per-target logging, and platform-level opt-out overriding routing intent (deliver=all, explicit platform:chat) all match the documented contract and are covered by tests (test_optout_platform_dropped_others_kept, test_deliver_all_still_respects_optout, test_global_optout_drops_all, test_fail_open_on_config_error, test_skip_is_logged).

One minor observation: this same try: from hermes_cli.config import load_config; from gateway.display_config import platform_accepts_proactive_push; ... except Exception: <fail-open> wrapper is duplicated in gateway/kanban_watchers.py (lines 296-301) and, per the PR stack description, presumably again in gateway/run.py for restart/startup notifications. Consider extracting a small shared helper (e.g. display_config.try_load_config_for_push_gate() -> Optional[dict]) to keep the fail-open contract consistent across all three call sites instead of re-implementing it each time.

♻️ Suggested shared helper (illustrative)

+# gateway/display_config.py
+def try_load_display_config() -> Optional[dict]:
+    """Load config for a proactive-push gate check; fail-open (return None) on error."""
+    try:
+        from hermes_cli.config import load_config
+        return load_config()
+    except Exception:
+        return None

🤖 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 `@cron/scheduler.py` around lines 645 - 677, The proactive-push opt-out gate is
implemented with the same fail-open wrapper in multiple places, including
_filter_proactive_push_optout and the similar logic in
gateway/kanban_watchers.py and gateway/run.py. Extract the shared
config-load/check flow into a small helper (for example in
gateway/display_config or a related utility) that returns the loaded config or a
fail-open result, and have each call site use that helper while keeping the
existing logging and platform_accepts_proactive_push behavior unchanged.

gateway/display_config.py (1)

255-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Opportunity to fold cleanup_progress into the shared boolean-normalise set.

proactive_push was correctly added to the shared boolean-coercion branch (Lines 249-259), but the adjacent cleanup_progress branch (Lines 268-271) still duplicates identical logic in a separate if. Since this area is already being touched, consider consolidating.

♻️ Optional consolidation

     if setting in {
         "show_reasoning",
         "streaming",
         "interim_assistant_messages",
         "long_running_notifications",
         "busy_ack_detail",
         "proactive_push",
+        "cleanup_progress",
     }:
         if isinstance(value, str):
             return value.lower() in {"true", "1", "yes", "on"}
         return bool(value)
     if setting == "memory_notifications":
         ...
-    if setting == "cleanup_progress":
-        if isinstance(value, str):
-            return value.lower() in {"true", "1", "yes", "on"}
-        return bool(value)

🤖 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 `@gateway/display_config.py` around lines 255 - 271, The boolean-normalization
logic for cleanup_progress is duplicated outside the shared coercion branch, so
consolidate it with the existing proactive_push handling in display_config
normalization. Update the shared setting check in the display_config function to
include cleanup_progress alongside proactive_push, and keep the same
string-to-bool and fallback bool(value) behavior so there is a single source of
truth for both settings.

tests/gateway/test_restart_notification.py (1)

693-711: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid, focused suppression test — consider a companion test for the home-channel path.

This test correctly isolates the new restart-notification proactive-push gate. The same PR layer adds an analogous gate to _send_home_channel_startup_notifications (gateway/run.py lines 13018-13031), which isn't covered by a direct test here — only the underlying platform_accepts_proactive_push/effective_memory_notifications helpers are unit-tested elsewhere. A copy-pasted gate is exactly the kind of code that can silently diverge; a mirrored test would catch that early.

async def test_send_home_channel_startup_notifications_suppressed_by_proactive_optout(tmp_path, monkeypatch):
    monkeypatch.setattr(
        "hermes_cli.config.load_config",
        lambda: {"display": {"platforms": {"telegram": {"proactive_push": False}}}},
    )
    runner, adapter = make_restart_runner()
    adapter.send = AsyncMock()
    delivered = await runner._send_home_channel_startup_notifications(skip_targets=None)
    assert delivered == set()
    adapter.send.assert_not_called()

🤖 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 `@tests/gateway/test_restart_notification.py` around lines 693 - 711, Add a
companion async test for the home-channel proactive-push suppression path,
mirroring test_send_restart_notification_suppressed_by_proactive_optout. In
test_send_home_channel_startup_notifications_suppressed_by_proactive_optout, set
proactive_push to False via hermes_cli.config.load_config, invoke
RestartRunner._send_home_channel_startup_notifications with skip_targets=None,
and assert it returns an empty set and does not call adapter.send. This should
cover the analogous gate in gateway/run.py and keep it from diverging from the
restart notification behavior.

gateway/run.py (1)

12937-12951: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated proactive-push gate — extract a shared helper.

Both blocks re-implement the identical try/except/import/log pattern for platform_accepts_proactive_push. Given the PR's stated goal of a single source of truth for this gate, having it copy-pasted here (and likely again in cron/kanban per the PR stack) risks drift if the gate logic, log wording, or exception handling needs to change later.

♻️ Suggested consolidation

+    def _proactive_push_gate(self, platform: Platform, *, context: str) -> bool:
+        """Fail-open check for whether `platform` accepts a proactive push.
+
+        Returns True (allow) on any config-read error.
+        """
+        try:
+            from hermes_cli.config import load_config as _load_push_cfg
+            from gateway.display_config import platform_accepts_proactive_push
+            if not platform_accepts_proactive_push(_load_push_cfg(), _platform_config_key(platform)):
+                logger.info(
+                    "%s suppressed: %s opted out of proactive push",
+                    context, platform.value,
+                )
+                return False
+        except Exception:
+            pass  # fail-open
+        return True

Then at each call site:

-            try:
-                from hermes_cli.config import load_config as _load_push_cfg
-                from gateway.display_config import platform_accepts_proactive_push
-                if not platform_accepts_proactive_push(_load_push_cfg(), _platform_config_key(platform)):
-                    logger.info(
-                        "Restart notification suppressed: %s opted out of proactive push",
-                        platform_str,
-                    )
-                    return None
-            except Exception:
-                pass  # fail-open
+            if not self._proactive_push_gate(platform, context="Restart notification"):
+                return None

Also worth reusing the existing _load_gateway_config() alias used throughout this file for display-config reads, instead of a fresh local import of hermes_cli.config.load_config, for consistency.

Also applies to: 13018-13031

🤖 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 `@gateway/run.py` around lines 12937 - 12951, The proactive-push check is
duplicated, so extract the repeated try/except/import/log logic into a shared
helper and reuse it at each call site instead of copy-pasting it in the
restart-notice path and the other matching block. Update the helper to use the
existing _load_gateway_config() alias for config reads, and keep
platform_accepts_proactive_push as the single gate implementation so the log
wording and exception handling stay consistent.

🤖 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.

Inline comments:
In `@gateway/display_config.py`:
- Around line 56-67: The comments for proactive push gating overstate scope by
including background-task results, which are not actually controlled by this
setting. Update the `_GLOBAL_DEFAULTS["proactive_push"]` documentation and the
`platform_accepts_proactive_push` docstring to describe only the paths that
truly consult this gate (cron, background-review memory notifications, kanban
notifiers, restart/home-channel notices), and explicitly exclude agent-task
delivery so future changes don’t assume it is already gated.

---

Nitpick comments:
In `@cron/scheduler.py`:
- Around line 645-677: The proactive-push opt-out gate is implemented with the
same fail-open wrapper in multiple places, including
_filter_proactive_push_optout and the similar logic in
gateway/kanban_watchers.py and gateway/run.py. Extract the shared
config-load/check flow into a small helper (for example in
gateway/display_config or a related utility) that returns the loaded config or a
fail-open result, and have each call site use that helper while keeping the
existing logging and platform_accepts_proactive_push behavior unchanged.

In `@gateway/display_config.py`:
- Around line 255-271: The boolean-normalization logic for cleanup_progress is
duplicated outside the shared coercion branch, so consolidate it with the
existing proactive_push handling in display_config normalization. Update the
shared setting check in the display_config function to include cleanup_progress
alongside proactive_push, and keep the same string-to-bool and fallback
bool(value) behavior so there is a single source of truth for both settings.

In `@gateway/kanban_watchers.py`:
- Around line 293-318: Add targeted test coverage for the kanban proactive-push
gate in kanban_watchers.py, focusing on the delivery loop that uses
_load_push_cfg, platform_accepts_proactive_push, and _kanban_advance. Write a
test that verifies an opted-out platform is skipped, the skip is logged, and the
cursor is still advanced so the event is not replayed; also add a fail-open case
where config loading raises and the code still proceeds with delivery rather
than blocking. Use the existing kanban notifier flow and the multi-board tick
behavior to locate the right branch.

In `@gateway/run.py`:
- Around line 12937-12951: The proactive-push check is duplicated, so extract
the repeated try/except/import/log logic into a shared helper and reuse it at
each call site instead of copy-pasting it in the restart-notice path and the
other matching block. Update the helper to use the existing
_load_gateway_config() alias for config reads, and keep
platform_accepts_proactive_push as the single gate implementation so the log
wording and exception handling stay consistent.

In `@tests/gateway/test_restart_notification.py`:
- Around line 693-711: Add a companion async test for the home-channel
proactive-push suppression path, mirroring
test_send_restart_notification_suppressed_by_proactive_optout. In
test_send_home_channel_startup_notifications_suppressed_by_proactive_optout, set
proactive_push to False via hermes_cli.config.load_config, invoke
RestartRunner._send_home_channel_startup_notifications with skip_targets=None,
and assert it returns an empty set and does not call adapter.send. This should
cover the analogous gate in gateway/run.py and keep it from diverging from the
restart notification behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a3d5263e-580c-4210-9df0-4e0a0899ae32

📥 Commits

Reviewing files that changed from the base of the PR and between dd5e290638b56a49344e96ee4c99fe4b7e90c543 and dc3092c3d50de1b35fd62c76ea562b2dc1aefa8b.

📒 Files selected for processing (7)

• cron/scheduler.py
• gateway/display_config.py
• gateway/kanban_watchers.py
• gateway/run.py
• tests/cron/test_proactive_push_gate.py
• tests/gateway/test_display_config.py
• tests/gateway/test_restart_notification.py

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Docstrings overstate coverage: "background-task results" isn't actually gated.

Both the _GLOBAL_DEFAULTS["proactive_push"] comment (Line 58) and the platform_accepts_proactive_push docstring (Line 291) list "background-task results" as one of the paths that consults this gate. Per the PR objectives, background agent-task delivery is explicitly left untouched by design — only cron, background-review memory notifications, kanban notifier, and restart/home-channel notices are gated. Leaving this text in place risks a future contributor assuming task-result delivery is already covered and skipping it, or double-gating incorrectly.

📝 Suggested doc fix

-    # Per-platform master switch for UNSOLICITED / background pushes (cron job
-    # responses, background-review memory notifications, kanban notifiers,
-    # background-task results, restart notices). Default on for back-compat.
+    # Per-platform master switch for UNSOLICITED / background pushes (cron job
+    # responses, background-review memory notifications, kanban notifiers,
+    # restart/startup notices). Default on for back-compat. NOTE: background
+    # agent-task result delivery intentionally does NOT consult this gate yet.

-    Single source of truth for the per-platform proactive-push gate. Every
-    background delivery path (cron ``_deliver_result``, background-review
-    memory notifications, kanban notifiers, background-task results, restart
-    notices) consults this before delivering, so a platform opted out via
+    Single source of truth for the per-platform proactive-push gate. Every
+    background delivery path (cron ``_deliver_result``, background-review
+    memory notifications, kanban notifiers, restart notices) consults this
+    before delivering, so a platform opted out via

As per PR objectives: "Background agent-task delivery is also left unchanged by design."

Also applies to: 284-313

🤖 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 `@gateway/display_config.py` around lines 56 - 67, The comments for proactive
push gating overstate scope by including background-task results, which are not
actually controlled by this setting. Update the
`_GLOBAL_DEFAULTS["proactive_push"]` documentation and the
`platform_accepts_proactive_push` docstring to describe only the paths that
truly consult this gate (cron, background-review memory notifications, kanban
notifiers, restart/home-channel notices), and explicitly exclude agent-task
delivery so future changes don’t assume it is already gated.

[coderabbitai]

🧹 Nitpick comments (1)

agent/agent_runtime_helpers.py (1)

2225-2242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicate Cloudflare BYOK block.

create_openai_client already performs this exact lookup and mutation at Lines 2207-2224. The added block repeats the environment read and header/API-key updates without adding behavior. Delete Lines 2225-2242 and keep one implementation.

Proposed fix

-    # Cloudflare AI Gateway BYOK: when a primary client's base_url is routed
-    # through the gateway (e.g. GEMINI_BASE_URL / XAI_BASE_URL), CF rejects the
-    # request (401 AiGatewayError) unless the cf-aig-authorization header is
-    # present. Inject it from CF_AIG_TOKEN and clear api_key so CF supplies the
-    # stored provider key. Direct provider URLs are untouched. Token read from
-    # env only, never logged. (Mirrored by tools/transcription_tools for STT.)
-    _cf_base_url = str(client_kwargs.get("base_url", "") or "")
-    if "gateway.ai.cloudflare.com" in _cf_base_url:
-        try:
-            from hermes_cli.config import get_env_value
-            _aig_token = (get_env_value("CF_AIG_TOKEN") or "").strip()
-            if _aig_token:
-                _dh = dict(client_kwargs.get("default_headers") or {})
-                _dh["cf-aig-authorization"] = f"Bearer {_aig_token}"
-                client_kwargs["default_headers"] = _dh
-                client_kwargs["api_key"] = ""
-        except Exception:
-            pass

🤖 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 `@agent/agent_runtime_helpers.py` around lines 2225 - 2242, Remove the
duplicate Cloudflare BYOK lookup and mutation block immediately following the
existing implementation in create_openai_client. Keep the earlier CF_AIG_TOKEN
handling at the start of create_openai_client unchanged so only one
implementation remains.

🤖 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 `@agent/agent_runtime_helpers.py`:
- Around line 2225-2242: Remove the duplicate Cloudflare BYOK lookup and
mutation block immediately following the existing implementation in
create_openai_client. Keep the earlier CF_AIG_TOKEN handling at the start of
create_openai_client unchanged so only one implementation remains.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 76ca8329-6a29-43cf-83ae-3bd592d63f5b

📥 Commits

Reviewing files that changed from the base of the PR and between dc3092c3d50de1b35fd62c76ea562b2dc1aefa8b and 9cc49a4.

📒 Files selected for processing (1)

• agent/agent_runtime_helpers.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

gateway/run.py (1)

21032-21097: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

LibreOffice conversion subprocess inherits the full gateway environment, including provider API keys.

_office_via_libreoffice spawns soffice with env={**os.environ, "HOME": outdir}. This passes the complete parent environment — LLM provider API keys, tokens, and other secrets held in os.environ — to a third-party document-conversion binary that needs none of them.

Elsewhere in this file, subprocess spawns that could otherwise inherit secrets are explicitly sanitized. The quick-command exec path builds its subprocess environment with build_subprocess_env() specifically because "quick commands run in the gateway process which has all API keys in os.environ." The same reasoning applies here: soffice runs in this same gateway process and should not receive its credentials.

🔒 Proposed fix

+        from tools.environments.local import build_subprocess_env
+        sanitized_env = build_subprocess_env()
+        sanitized_env["HOME"] = outdir
         try:
             proc = await asyncio.create_subprocess_exec(
                 soffice, "--headless", "--nologo", "--nofirststartwizard",
                 "--convert-to", target, "--outdir", outdir, real_path,
                 stdout=asyncio.subprocess.DEVNULL,
                 stderr=asyncio.subprocess.DEVNULL,
-                # Isolated HOME so concurrent conversions don't fight over the
-                # single-user LibreOffice profile lock.
-                env={**os.environ, "HOME": outdir},
+                # Isolated HOME so concurrent conversions don't fight over the
+                # single-user LibreOffice profile lock; scrubbed of secrets.
+                env=sanitized_env,
             )

🤖 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 `@gateway/run.py` around lines 21032 - 21097, Sanitize the environment passed
to the LibreOffice subprocess in _office_via_libreoffice instead of copying
os.environ wholesale. Reuse the existing build_subprocess_env() helper used by
other gateway subprocess paths, while preserving the isolated HOME=outdir
setting required for concurrent conversions.

🧹 Nitpick comments (1)

gateway/run.py (1)

20538-20552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the proactive-push gate into a shared helper and use the same config loader as other display settings.

This block duplicates the exact try/except scaffolding at Lines 20630-20643 in _send_home_channel_startup_notifications. Extract a single helper, for example _platform_accepts_proactive_push(self, platform) -> bool, and call it from both sites.

Also, every other per-platform display-setting read in this file goes through the module-level _load_gateway_config() (see _resolve_gateway_display_bool, the busy_ack_detail, reasoning_style, and streaming reads). This block instead imports hermes_cli.config.load_config locally. _load_gateway_config() has an mtime-keyed cache and explicitly honors get_hermes_home_override() for profile scoping. Confirm that hermes_cli.config.load_config() provides equivalent profile-scoping and caching under gateway.multiplex_profiles, or switch to _load_gateway_config() for consistency and to avoid a second full config parse on this codepath.

♻️ Proposed refactor sketch

+    def _platform_accepts_proactive_push(self, platform: Platform) -> bool:
+        """Fail-open per-platform proactive_push gate for unsolicited pushes."""
+        try:
+            from gateway.display_config import platform_accepts_proactive_push
+            return platform_accepts_proactive_push(
+                _load_gateway_config(), _platform_config_key(platform)
+            )
+        except Exception:
+            return True  # fail-open on config read errors
+
     async def _send_restart_notification(self) -> Optional[tuple[str, str, Optional[str]]]:
         ...
-            try:
-                from hermes_cli.config import load_config as _load_push_cfg
-                from gateway.display_config import platform_accepts_proactive_push
-                if not platform_accepts_proactive_push(_load_push_cfg(), _platform_config_key(platform)):
-                    logger.info(
-                        "Restart notification suppressed: %s opted out of proactive push",
-                        platform_str,
-                    )
-                    return None
-            except Exception:
-                pass  # fail-open
+            if not self._platform_accepts_proactive_push(platform):
+                logger.info(
+                    "Restart notification suppressed: %s opted out of proactive push",
+                    platform_str,
+                )
+                return None

🤖 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 `@gateway/run.py` around lines 20538 - 20552, Extract the duplicated
proactive-push check into a shared `_platform_accepts_proactive_push(self,
platform)` helper and call it from both the restart-notification path and
`_send_home_channel_startup_notifications`. Within the helper, use the
module-level `_load_gateway_config()` rather than importing
`hermes_cli.config.load_config`, preserve the existing platform key resolution,
logging/return behavior, and fail-open exception handling.

🤖 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.

Outside diff comments:
In `@gateway/run.py`:
- Around line 21032-21097: Sanitize the environment passed to the LibreOffice
subprocess in _office_via_libreoffice instead of copying os.environ wholesale.
Reuse the existing build_subprocess_env() helper used by other gateway
subprocess paths, while preserving the isolated HOME=outdir setting required for
concurrent conversions.

---

Nitpick comments:
In `@gateway/run.py`:
- Around line 20538-20552: Extract the duplicated proactive-push check into a
shared `_platform_accepts_proactive_push(self, platform)` helper and call it
from both the restart-notification path and
`_send_home_channel_startup_notifications`. Within the helper, use the
module-level `_load_gateway_config()` rather than importing
`hermes_cli.config.load_config`, preserve the existing platform key resolution,
logging/return behavior, and fail-open exception handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9d1bbf72-a53f-4c7a-b376-d45fa196be0a

📥 Commits

Reviewing files that changed from the base of the PR and between 9cc49a4 and 02ffc1a0b45fe7473bd7f4255608f2c5bc95005d.

📒 Files selected for processing (5)

• agent/agent_runtime_helpers.py
• cron/scheduler.py
• gateway/display_config.py
• gateway/kanban_watchers.py
• gateway/run.py

🚧 Files skipped from review as they are similar to previous changes (4)

• gateway/kanban_watchers.py
• cron/scheduler.py
• agent/agent_runtime_helpers.py
• gateway/display_config.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

agent/agent_runtime_helpers.py (3)

1499-1503: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Resolve the named custom-provider pool key before loading it.

When primary_provider is custom, a named endpoint uses a pool key such as custom:<name>. load_pool() selects the named custom-provider path only for that prefixed key. The calls at Lines [1499]-[1503] and [1623]-[1625] pass plain primary_provider. A fallback restore can therefore attach the generic pool instead of the pool for the primary base URL. The reuse at Lines [1618]-[1621] preserves that pool, which can disable rotation or select an unrelated credential.

Resolve get_custom_provider_pool_key() from the primary base URL once, then use that key in both load_pool() calls. Add a regression test with two named custom providers that share a gateway base URL.

Also applies to: 1618-1625

🤖 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 `@agent/agent_runtime_helpers.py` around lines 1499 - 1503, Update the
primary-provider pool handling around prefetched_primary_pool and the later
load_pool call to resolve get_custom_provider_pool_key() from the primary base
URL once, then pass that resolved key to both load_pool calls instead of plain
primary_provider. Preserve reuse of the resolved pool, and add a regression test
covering two named custom providers sharing a gateway base URL.

---

2349-2377: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate the Cloudflare hostname before sending CF_AIG_TOKEN.

The substring check at Line [2349] also accepts untrusted hosts, such as gateway.ai.cloudflare.com.attacker.example, and URLs that contain the hostname in a path or query. This can send CF_AIG_TOKEN to a non-Cloudflare endpoint. The same BYOK block is duplicated at Lines [2360]-[2377]. Keep one block and require an exact hostname match before injecting the header.

Proposed fix

-    if "gateway.ai.cloudflare.com" in _cf_base_url:
+    if base_url_hostname(_cf_base_url) == "gateway.ai.cloudflare.com":

🤖 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 `@agent/agent_runtime_helpers.py` around lines 2349 - 2377, Consolidate the
duplicated Cloudflare BYOK logic into one block and replace the substring check
on _cf_base_url with URL parsing that requires the hostname to equal
gateway.ai.cloudflare.com exactly, excluding matching paths, queries,
subdomains, and attacker-controlled suffixes. Only inject cf-aig-authorization
and clear api_key after this validated host check.

---

2210-2224: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Gate OpenCode Qwen cache rewards to chat completions.

OpenCode opencode-zen and opencode-go route Qwen models via opencode_model_api_mode() to anthropic_messages. That makes this branch hit before is_anthropic_wire, so provider_is_alibaba_family + model_is_qwen can return (True, False) with a Messages transport and apply the OpenAI-wire cache layout to an Anthropic-wire request. Require the OpenAI wire (not is_anthropic_wire) for this branch, or return the native Messages cache layout for the supported anthropic OpenCode Qwen routes.

🤖 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 `@agent/agent_runtime_helpers.py` around lines 2210 - 2224, Update the Qwen
branch guarded by provider_is_alibaba_family and model_is_qwen to apply only
when the request uses the OpenAI wire, by also requiring not is_anthropic_wire.
Preserve the existing native Anthropic Messages handling for supported OpenCode
Qwen routes and prevent the OpenAI-wire cache layout from being returned for
anthropic_messages transport.

🤖 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.

Outside diff comments:
In `@agent/agent_runtime_helpers.py`:
- Around line 1499-1503: Update the primary-provider pool handling around
prefetched_primary_pool and the later load_pool call to resolve
get_custom_provider_pool_key() from the primary base URL once, then pass that
resolved key to both load_pool calls instead of plain primary_provider. Preserve
reuse of the resolved pool, and add a regression test covering two named custom
providers sharing a gateway base URL.
- Around line 2349-2377: Consolidate the duplicated Cloudflare BYOK logic into
one block and replace the substring check on _cf_base_url with URL parsing that
requires the hostname to equal gateway.ai.cloudflare.com exactly, excluding
matching paths, queries, subdomains, and attacker-controlled suffixes. Only
inject cf-aig-authorization and clear api_key after this validated host check.
- Around line 2210-2224: Update the Qwen branch guarded by
provider_is_alibaba_family and model_is_qwen to apply only when the request uses
the OpenAI wire, by also requiring not is_anthropic_wire. Preserve the existing
native Anthropic Messages handling for supported OpenCode Qwen routes and
prevent the OpenAI-wire cache layout from being returned for anthropic_messages
transport.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 64672cd2-751e-41b0-b9c0-937374722348

📥 Commits

Reviewing files that changed from the base of the PR and between 02ffc1a0b45fe7473bd7f4255608f2c5bc95005d and 1a0c3c4.

📒 Files selected for processing (5)

• agent/agent_runtime_helpers.py
• cron/scheduler.py
• gateway/display_config.py
• gateway/kanban_watchers.py
• gateway/run.py

🚧 Files skipped from review as they are similar to previous changes (3)

• gateway/kanban_watchers.py
• cron/scheduler.py
• gateway/display_config.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

gateway/run.py (3)

6285-6309: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Fix the crash path when gateway_tokens_display.json has a non-dict chats value.

At line 6302, chats = data.get("chats") or {} only guards against a falsy chats value. If the on-disk file contains {"chats": "something"} or any other truthy non-dict value, chats becomes that non-dict value. The dict comprehension at line 6307 then calls chats.items(), which raises AttributeError. _load_tokens_display runs unguarded from GatewayRunner.__init__, so a malformed or manually edited state file crashes gateway startup entirely instead of degrading gracefully like the outer isinstance(data, dict) check already does for the top-level structure.

🛡️ Proposed fix

         if "chats" in data or "global" in data:
             self._tokens_display_global = bool(data.get("global", False))
-            chats = data.get("chats") or {}
+            chats = data.get("chats")
+            if not isinstance(chats, dict):
+                chats = {}
         else:
             chats = data  # legacy flat format

🤖 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 `@gateway/run.py` around lines 6285 - 6309, Update _load_tokens_display so the
chats value is validated as a dictionary before iterating over it; treat
missing, null, or any non-dict chats value as an empty mapping. Preserve the
existing global preference handling and legacy flat-format migration, while
ensuring malformed nested state returns {} without raising.

---

21490-21507: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Move the XLSX size cap inside the per-row loop so it bounds a single worksheet.

The intended cap (_MAX = 20000 chars) is checked at line 21501, but that check sits outside the inner for row in ws.iter_rows(...) loop (lines 21497-21500). For a single worksheet with a very large row count, the inner loop runs to completion — building the full out list in memory and paying the per-cell string-conversion cost for every row — before the size check ever runs. The break at line 21502 only stops processing additional worksheets; it does not bound the cost of the current one. Any user who can send a document attachment can trigger this by uploading a spreadsheet with a large number of rows in one sheet, even though the code's stated intent is to cap extraction near 20 KB.

⚡ Proposed fix

                     for ws in wb.worksheets:
                         out.append(f"# Sheet: {ws.title}")
+                        truncated = False
                         for row in ws.iter_rows(values_only=True):
                             cells = ["" if c is None else str(c) for c in row]
                             if any(cells):
                                 out.append("\t".join(cells))
-                        if sum(len(x) for x in out) > _MAX:
-                            break
+                            if sum(len(x) for x in out) > _MAX:
+                                truncated = True
+                                break
+                        if truncated:
+                            break

🤖 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 `@gateway/run.py` around lines 21490 - 21507, Move the _MAX length check from
the worksheet loop into the inner row loop within the XLSX extraction block, so
processing stops as soon as the current worksheet’s accumulated output reaches
the cap. Preserve the existing worksheet headers, row conversion, logging,
wrapping, and return behavior while ensuring large single-sheet files do not
process every remaining row.

---

21524-21559: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Security And Privacy (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor

Pass the sanitized subprocess environment to soffice.

_office_via_libreoffice processes untrusted documents with env={**os.environ, "HOME": outdir}, exposing the full gateway environment to a third-party binary. Reuse tools.environments.local.build_subprocess_env() here and overwrite HOME, so provider/secret env vars are not inherited by the conversion helper. The current --convert-to path does not execute embedded macros by default, but this still removes a possible secret-leak surface if LibreOffice parses vulnerable content.

🤖 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 `@gateway/run.py` around lines 21524 - 21559, Update _office_via_libreoffice to
obtain the subprocess environment from
tools.environments.local.build_subprocess_env() instead of copying os.environ,
then override HOME with outdir while preserving the existing isolated-profile
behavior. Pass this sanitized environment to asyncio.create_subprocess_exec for
the soffice conversion.

🧹 Nitpick comments (1)

gateway/run.py (1)

21030-21044: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate the proactive-push gate and reuse _load_gateway_config() instead of hermes_cli.config.load_config. Both sites implement the identical try/except gate around platform_accepts_proactive_push, and both import hermes_cli.config.load_config fresh rather than reusing this file's own _load_gateway_config() helper, which every other display-setting read in this file already uses (cached, managed-scope-aware, fail-open).

• gateway/run.py#L21030-L21044: extract the gate into a small shared helper (e.g. self._platform_accepts_proactive_push(platform)) that calls _load_gateway_config() and platform_accepts_proactive_push, then call it here.
• gateway/run.py#L21122-L21135: call the same shared helper here instead of re-importing load_config and re-implementing the try/except.

🤖 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 `@gateway/run.py` around lines 21030 - 21044, The proactive-push gate is
duplicated and bypasses the file’s cached, managed-scope-aware configuration
helper. In gateway/run.py lines 21030-21044, extract the try/except logic into a
shared helper such as _platform_accepts_proactive_push(platform) that uses
_load_gateway_config() and platform_accepts_proactive_push with fail-open
behavior, then call it from the existing restart-notification flow. In
gateway/run.py lines 21122-21135, replace the duplicated import and gate with
the same helper call; both sites should retain their existing suppression
behavior and logging.

🤖 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.

Outside diff comments:
In `@gateway/run.py`:
- Around line 6285-6309: Update _load_tokens_display so the chats value is
validated as a dictionary before iterating over it; treat missing, null, or any
non-dict chats value as an empty mapping. Preserve the existing global
preference handling and legacy flat-format migration, while ensuring malformed
nested state returns {} without raising.
- Around line 21490-21507: Move the _MAX length check from the worksheet loop
into the inner row loop within the XLSX extraction block, so processing stops as
soon as the current worksheet’s accumulated output reaches the cap. Preserve the
existing worksheet headers, row conversion, logging, wrapping, and return
behavior while ensuring large single-sheet files do not process every remaining
row.
- Around line 21524-21559: Update _office_via_libreoffice to obtain the
subprocess environment from tools.environments.local.build_subprocess_env()
instead of copying os.environ, then override HOME with outdir while preserving
the existing isolated-profile behavior. Pass this sanitized environment to
asyncio.create_subprocess_exec for the soffice conversion.

---

Nitpick comments:
In `@gateway/run.py`:
- Around line 21030-21044: The proactive-push gate is duplicated and bypasses
the file’s cached, managed-scope-aware configuration helper. In gateway/run.py
lines 21030-21044, extract the try/except logic into a shared helper such as
_platform_accepts_proactive_push(platform) that uses _load_gateway_config() and
platform_accepts_proactive_push with fail-open behavior, then call it from the
existing restart-notification flow. In gateway/run.py lines 21122-21135, replace
the duplicated import and gate with the same helper call; both sites should
retain their existing suppression behavior and logging.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: efef9e03-2a49-40d0-af22-514fc74cf149

📥 Commits

Reviewing files that changed from the base of the PR and between 1a0c3c4 and 25e8b11.

📒 Files selected for processing (8)

• agent/agent_runtime_helpers.py
• cron/scheduler.py
• gateway/display_config.py
• gateway/kanban_watchers.py
• gateway/run.py
• tests/cron/test_proactive_push_gate.py
• tests/gateway/test_display_config.py
• tests/gateway/test_restart_notification.py

🚧 Files skipped from review as they are similar to previous changes (6)

• cron/scheduler.py
• gateway/display_config.py
• agent/agent_runtime_helpers.py
• tests/gateway/test_display_config.py
• tests/cron/test_proactive_push_gate.py
• gateway/kanban_watchers.py