fix(gateway): close 3 watch-event gaps left by the completion-notification hardening - #86261
fix(gateway): close 3 watch-event gaps left by the completion-notification hardening#86261pierrenode wants to merge 1 commit into
Conversation
fix(gateway): close 3 watch-event gaps left by the completion-notification hardening
|
9e6cc3f to
836f2f3
Compare
|
CI caught a real regression in the last commit, now fixed in What broke: item 3 of this PR (title/compression filters) made Fix: async-delegation completions are now excluded from Full neighbor sweep (273 tests across the touched files + |
…ation hardening This week's completion-notification hardening series (b9e7bea, c0d2048, 84b4fb9, a96cd10, 7619564, 8dc9401) forced secret redaction, a spawning-session-boundary pre-flight, and title/compression filtering onto completion/async_delegation notifications. watch_match/watch_disabled/ watch_overflow_* events were left on the old, weaker behavior in three places — plus a fourth, related gap found while implementing the above: watch-event notifications weren't attributed to the subagent that owned the watched process. 1. Redaction: _format_gateway_process_notification (gateway/run.py) and the shared format_process_notification (tools/process_registry.py, also used by the TUI gateway) rendered watch_match's output/command from the producer-side, non-forced _redact_process_result pass only. A user with security.redact_secrets: false would get a raw secret sent straight to the chat platform if a watch pattern matched a line containing one. Both formatters now apply the same forced, unconditional redaction floor the completion path already has. 2. Session-boundary gate: _drain_watch_notifications called _inject_watch_notification directly, bypassing _classify_completion_target entirely. A watch_match/watch_disabled event from a process spawned in session A could still land in session B's chat after /new closed A. ProcessSession.parent_session_id (already stamped at spawn time) is now also carried on watch_match/watch_disabled events and checked before injection. Unstamped/global events (the cross-session overflow summaries) keep delivering unconditionally, matching completion's own legacy fallback. Watch events have no watcher to re-poll them later, so a "retry" (transient DB uncertainty) verdict fails open and delivers rather than losing the match outright. 3. Title/compression filters: title_generator._is_real_user_turn and context_compressor._is_synthetic_compression_user_turn didn't recognize any of the "[IMPORTANT: ...]"/"[ASYNC DELEGATION ...]" notification shapes — only the unrelated compaction/continuation/model-switch markers. Both now check message.get("display_kind") == "internal_notification" (the structural marker gateway/run.py stamps at persist time) as the primary signal, plus explicit text-prefix entries for the raw-string call paths that don't have a message dict to check. Async-delegation completions are excluded from the compression side of this: unlike watch/background-process bookkeeping, _format_async_delegation's own docstring says the block carries "the complete result summary" — genuine actionable content a real user turn would also carry — so treating it as synthetic would let compaction blank out a delegation's actual result (see test_completion_survives_compaction_verbatim_after_blank_echo, bc48241). They're still excluded from titling, where the boilerplate wrapper text would make a bad title regardless of the payload. 4. Subagent attribution: completion/async_delegation events resolve their task_id (via tools/delegate_tool.py's _active_subagents registry) into a "Started by subagent ... of delegation ... Task: ..." provenance line. watch_match already carried this; watch_disabled never did, in either formatter. Fixed by stamping task_id on the watch_disabled event dict and adding the same attribution lookup to both formatters — gateway/run.py's kept additive (not delegated to the shared formatter) to avoid silently swapping its _redact_gateway_user_facing_secrets guarantee for redact_terminal_output. Mutation-verified throughout, including the async-delegation exclusion against both the original wrong code and each half of the fix independently — all reproduce test_completion_survives_compaction_verbatim_after_blank_echo failing. Full neighbor sweep green (tests/gateway/, tests/agent/ compress/compaction suite, tests/tools/test_watch_patterns.py, tests/tools/test_process_registry.py, tests/tools/test_async_delegation.py). ruff clean. Adjacent open PRs checked, no semantic overlap: - NousResearch#75719 restructures the same gateway/run.py formatter for an unrelated concern (a "supersession context" note on delayed notifications) — textual proximity only. - NousResearch#61719 adds a different field (origin_ui_session_id) to the same watch_match/watch_disabled dict literals, for TUI/WebUI tab ownership — complementary, not overlapping. - NousResearch#73469 is an alternative architecture for the same-tick completion-coalescing race this week's series already solved differently; different function region.
daa6c88 to
5e857b3
Compare
What does this PR do?
This week's completion-notification hardening series (b9e7bea, c0d2048, 84b4fb9, a96cd10, 7619564, 8dc9401) forced secret redaction, a spawning-session-boundary pre-flight, and title/compression filtering onto
completion/async_delegationnotifications.watch_match/watch_disabled/watch_overflow_*events were left on the old, weaker behavior in three places. This closes all three gaps for the watch-event family — plus a fourth, related gap found while implementing the above: watch-event notifications weren't attributed to the subagent that owned the watched process.1. Redaction
_format_gateway_process_notification(gateway/run.py) and the sharedformat_process_notification(tools/process_registry.py, also used by the TUI gateway) renderedwatch_match's output/command from the producer-side, non-forced_redact_process_resultpass only. A user withsecurity.redact_secrets: falsewould get a raw secret sent straight to the chat platform if a watch pattern matched a line containing one.Both formatters now apply the same forced, unconditional redaction floor the completion path already has (
_redact_gateway_user_facing_secrets/redact_terminal_output(..., force=True)).2. Session-boundary gate
_drain_watch_notificationscalled_inject_watch_notificationdirectly, bypassing_classify_completion_targetentirely. Awatch_match/watch_disabledevent from a process spawned in session A could still land in session B's chat after/newclosed A — the exact bug #70300's fix closed forcompletionevents, left open for watch events.ProcessSession.parent_session_id(already stamped at spawn time regardless of notification mode) is now also carried onwatch_match/watch_disabledevents and checked before injection. Unstamped/global events (the cross-session overflow summaries, which have no single owning session) keep delivering unconditionally, matchingcompletion's own legacy fallback. Watch events have no watcher to re-poll them later, so a"retry"(transient DB uncertainty) verdict fails open and delivers rather than losing the match outright — narrower than completion's retry handling, by design.3. Title/compression filters
title_generator._is_real_user_turnandcontext_compressor._is_synthetic_compression_user_turndidn't recognize any of the"[IMPORTANT: ...]"/"[ASYNC DELEGATION ...]"notification shapes — only the unrelated compaction/continuation/model-switch markers. A session whose opening turn was a background notification would title itself after it, and a mid-conversation notification could get selected as a compaction anchor.Both now check
message.get("display_kind") == "internal_notification"(the structural marker gateway/run.py already stamps at persist time, #82888) as the primary, text-independent signal, plus explicit text-prefix entries for the raw-string call paths that don't have a message dict to check (is_titleable_user_message's direct callers;context_compressor's existing SessionDB-projected-row test pattern, which calls the recognizer with a bare{"role": "user", "content": ...}dict).4. Subagent attribution (added)
completionandasync_delegationevents carry atask_id, which_delegation_attribution_line()resolves (via the_active_subagentsregistry intools/delegate_tool.py) into a"Started by subagent ... of delegation ... Task: ..."provenance line — so a user watching a delegated worker's background process knows which subagent it belongs to.watch_matchalready carried this attribution;watch_disabled(the "I gave up watching, here's the tail" notification fired when the strike limit trips) never did, in both formatters (tools/process_registry.py::format_process_notificationandgateway/run.py::_format_gateway_process_notification).Fixed by stamping
task_idon thewatch_disabledevent dict (mirroringwatch_match's existing field) in_check_watch_patterns, and adding the same attribution-line lookup toformat_process_notification'swatch_disabledbranch.gateway/run.py's separate formatter gets the same fix via a lazy import of_delegation_attribution_linefor both itswatch_matchandwatch_disabledbranches, deliberately kept additive (not delegated to the shared formatter) to avoid silently swapping this function's_redact_gateway_user_facing_secretsguarantee forredact_terminal_output.Testing
tests/gateway/test_background_process_notifications.py,tests/gateway/test_completion_session_boundary.py,tests/agent/test_title_generator.py,tests/agent/test_context_compressor_zero_user_provenance.py,tests/tools/test_watch_patterns.py.tests/gateway/,tests/agent/test_context_compressor*.py,tests/tools/test_process_registry.py,tests/tools/test_watch_patterns.py,tests/tools/test_async_delegation.py,tests/tools/test_delegate_control_actions.py,tests/agent/test_title_generator.py— 270 passed, 5 pre-existing/unrelated skips, 0 failures.ruff checkclean on all changed files.Adjacent open PRs (checked, no semantic overlap)
_format_gateway_process_notification(delegatingwatch_match/watch_disabledto the sharedtools/process_registry.py::format_process_notification) but for an unrelated concern (a "supersession context" note on delayed notifications). Textual proximity only — this PR'stools/process_registry.pyredaction/attribution additions are correct and necessary regardless of which lands first, since they're a defense-in-depth layer on the same shared function fix(process): add supersession context to delayed notifications #75719 also touches.origin_ui_session_id) to the samewatch_match/watch_disableddict literals in_check_watch_patterns, for TUI/WebUI tab ownership filtering — not the gateway's/new-boundaryparent_session_idclassification or thetask_idattribution field this PR adds to the same literals. Complementary, not overlapping; will need a small rebase against whichever lands second.gateway/run.pybroadly but a different function region, no overlap.Type of Change