Repository navigation
fix(gateway): stop synthetic goal/heartbeat turns from stomping channel_pin - #126155
chelsealong wants to merge 3 commits into
Conversation
…el_pin Goal continuation, heartbeat due-prompts and /goal resume events are built non-internal (they must still pass authorization) with channel_prompt=None. _pinned_channel_inputs() only reused the pin for internal events, so these synthetic turns recorded (None, parent_chat_id) as the new channel_pin, dropping the channel prompt and channel_overrides for that turn and flipping them back on the next human turn (NousResearch#125763/NousResearch#124179-style A->B->A: shorter ephemeral prompt, changed _agent_config_signature, cached-agent rebuild, prefix-cache miss). Add MessageEvent.inherit_channel_pin, independent of `internal` (which also gates authorization bypass and touch_activity), and set it on the three producers that carry no channel inputs of their own. _pinned_channel_inputs now reuses the pin for internal OR pin-inheriting events. Fixes NousResearch#126109
ehz0ah
left a comment
There was a problem hiding this comment.
I reviewed this at abe539ac. The focused suites pass as submitted: 72 tests passed across the pin-wiring and goal-dispatch files. The channel-pin change is directionally correct, but the full prompt still changes on the synthetic turn. A one-line byte-equality assertion in the new real-handler regression fails on this head. Details are inline. This head also conflicts with current main in the pin-wiring test and has no hosted checks.
| _turn_channel_prompt, _turn_source = self._pinned_channel_inputs( | ||
| session_key, event.channel_prompt, source, internal=event.internal, | ||
| session_key, event.channel_prompt, source, | ||
| reuse_pin=event.internal or getattr(event, "inherit_channel_pin", False), |
There was a problem hiding this comment.
Blocking: this reuses only the channel pin. Earlier in the same handler, _hmwa_prepare_turn() still calls _pinned_session_context_prompt(..., internal=event.internal). _synthetic_prompt_event() removes message_id but keeps the event non-internal, and Discord includes message-ID presence in _ephemeral_change_key(). The synthetic turn therefore rebuilds a shorter session-context block, and the next human turn changes it back. I added only assert eph[0] == eph[1] == eph[2] to this PR’s new real-handler test; it failed with 2 passing tests and 1 failure because the middle prompt lost the “Triggering message” line. Please apply the same inheritance signal to the session-context pin and keep that full byte-equality assertion. That closes the A→B→A invariant rather than only preserving channel_prompt.
| # heartbeat due-prompts, /goal resume) so the turn reuses the session's pinned channel | ||
| # prompt/parent instead of overwriting it with this event's own None (#126109). Independent | ||
| # of ``internal``: these events still go through user authorization. | ||
| inherit_channel_pin: bool = False |
There was a problem hiding this comment.
Please append this field after the existing init fields instead of inserting it before metadata. MessageEvent is the normalized adapter boundary, and its own field-order comment requires new fields to be appended for positional compatibility. This placement shifts metadata, timestamp, allow_gateway_control, and reply_expected for external adapters or plugins that still construct events positionally.
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #126155 (head abe539acdd303cdff13bddc4bd8a739dfa03bf58)
Verdict: request changes — the diagnosis is right and the channel_pin half is fixed, but the fix is partial and the head conflicts with main. Merge decision is Teknium's.
Context. Synthetic goal-continuation / heartbeat / /goal resume turns are non-internal and carry channel_prompt=None; _pinned_channel_inputs treated every non-internal event as a human turn and overwrote the pin with (None, parent) (#126109). Your inherit_channel_pin flag on the three producers fixes that half; verified by driving the real handler on your tree merged onto origin/main @ 188a1a5: channel_prompt == ['Channel hint.'] * 3 (45 tests passed across your test file + goal/heartbeat/queue siblings).
Blocking — the session-context pin still flips (ehz0ah's inline finding, confirmed live). run_turn.py::_hmwa_prepare_turn still calls _pinned_session_context_prompt(..., internal=event.internal). _synthetic_prompt_event strips source.message_id, and on Discord _ephemeral_change_key keys on message-id presence, so the synthetic turn re-renders the session-context block and the next human turn re-renders it back. Same probe on your tree: context bytes 1238 → 1122 → 1238 while the channel prompt stays stable; the whole point of the pin is byte-stable system bytes, so half the A→B→A (and the agent rebuild) survives. The issue triage lists both _pinned_session_context_prompt and _pinned_channel_inputs as the two seams; feed the same internal or inherit_channel_pin decision to both (#126167 does this and its byte-equality test passes; your test should assert ephemeral[0] == ephemeral[1] == ephemeral[2] too, which is what makes it red on main for the right reason — the current red-on-base is a TypeError on the missing kwarg, not the symptom).
Also needed:
gateway/platforms/event.py:MessageEvent's own rule (comment aboveallow_gateway_control) is that new fields append afterreply_expectedfor positional compat;inherit_channel_pinis inserted beforemetadata.- Rebase:
tests/gateway/test_internal_event_pin_wiring.pyconflicts with main (#126092 added tests in the same slot — keep both). GitHub dispatches zero workflows on a conflicting PR, which is why the branch shows no checks. - Minor: the
getattr(event, "inherit_channel_pin", False)at the two call sites is only there forSimpleNamespacedoubles in two tests; fine, but if you take the_event_preserves_prompt_pins-style helper it lives in one place.
Overlap: #126167 (filed six minutes after this one) covers both pins and passes the full byte-equality probe; you found the channel half first and should be credited whichever lands. If you update this PR to cover the context pin as above, the two are equivalent and the choice is the maintainer's.
| # heartbeat due-prompts, /goal resume) so the turn reuses the session's pinned channel | ||
| # prompt/parent instead of overwriting it with this event's own None (#126109). Independent | ||
| # of ``internal``: these events still go through user authorization. | ||
| inherit_channel_pin: bool = False |
There was a problem hiding this comment.
MessageEvent's own rule (comment above allow_gateway_control): new fields append after reply_expected for positional compat. Move inherit_channel_pin to the end of the init fields.
| if not session_key: | ||
| return channel_prompt, source | ||
| if not internal: | ||
| if not reuse_pin: |
There was a problem hiding this comment.
This fixes the channel half only. _hmwa_prepare_turn still calls _pinned_session_context_prompt(..., internal=event.internal), and because _synthetic_prompt_event strips message_id (a _ephemeral_change_key input on Discord), the session-context pin still flips. Live on this head merged onto main: context bytes 1238 → 1122 → 1238 across human → synthetic → human while channel_prompt stays 'Channel hint.'. Pass the same internal or inherit_channel_pin decision to _pinned_session_context_prompt, and assert full ephemeral byte-equality in the test.
Feed the same internal or inherit_channel_pin decision into _pinned_session_context_prompt as _pinned_channel_inputs, so a synthetic goal/heartbeat/resume turn (which strips message_id, a _ephemeral_change_key input) doesn't flip the session-context block and back on the next human turn. Also moves inherit_channel_pin to append after reply_expected for MessageEvent positional compat.
…c-channel-pin # Conflicts: # tests/gateway/test_internal_event_pin_wiring.py
|
Addressed both blocking points at
|
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head 10df6fd2b5361e4a8db144c836b6b6950ce76b9f.
The new commits resolve both blockers from my earlier review. Synthetic goal, heartbeat, and resume turns now reuse the session-context pin as well as the channel pin, so dropping message_id no longer changes the full ephemeral prompt. The new MessageEvent field is also appended after the existing init fields, which preserves positional compatibility.
I traced the producer and consumer paths. The affected events remain non-internal, so authorization, emergency-stop handling, and session activity updates are unchanged. The effective-prompt regression now checks byte-identical prompts across a human turn, a synthetic turn, and the next human turn.
Local verification passed: 108 tests across the pin-wiring, goal continuation and resume, heartbeat, queue, and goal-dispatch suites. GitHub currently reports no hosted checks.
One non-blocking coverage improvement would be to assert inherit_channel_pin directly in the heartbeat, goal-continuation, and resume producer tests. The current end-to-end regression constructs the flag explicitly, so those assertions would catch a future producer-wiring regression closer to its source.
I found no further code-level blocker on this head. GitHub currently reports a merge conflict with main, so the branch still needs to be updated before it can merge.
|
Merged current |
|
Salvaged into #139865, merged as 8eecd1c, with your commit and authorship kept. The |
Fixes #126109.
Root cause
Gateway turns injected from synthetic events (goal continuation, heartbeat
due-prompts,
/goalresume) are built as non-internalMessageEvents(they must still pass user authorization) with
channel_prompt=None._pinned_channel_inputs()(gateway/run_agent_cache.py) only reused thesession's pinned channel inputs for
internalevents — every non-internalevent, synthetic or not, recorded
(channel_prompt, source.parent_chat_id)as the new
channel_pin. Since these synthetic events never carry their ownchannel_prompt, that recorded(None, parent_chat_id), dropping thechannel prompt /
channel_overridesprompt for that turn and then flippingit back on the next human turn — the same A→B→A pattern as #125763 and
#124179 (shorter ephemeral system prompt, changed
_agent_config_signature,cached-agent rebuild, prefix-cache miss).
Producers, all confirmed on current
main:gateway/run_goals.py::_synthetic_prompt_event— used by goalcontinuation (
_post_turn_goal_continuation) and the heartbeat due-promptpoller (
_heartbeat_poll_watch)gateway/slash_commands_goals.py::_enqueue_goal_turn—/goalresumecontinuation (
channel_prompt=event.channel_prompt if kickoff else None)Fix
internalwas overloaded as the proxy for "reuse the channel pin", but italso gates authorization bypass (and
touch_activity) — heartbeat and goalevents deliberately stay non-internal so those checks still apply. Added a
separate
MessageEvent.inherit_channel_pinflag and set it on the threesynthetic producers above (the
/goalresume case only, not the kickoffcase, since kickoff carries its own real
channel_prompt). Both call sitesof
_pinned_channel_inputsnow passreuse_pin=event.internal or event.inherit_channel_pin.Test plan
Added
test_synthetic_non_internal_event_reuses_channel_pintotests/gateway/test_internal_event_pin_wiring.py, which drives the real_handle_message_with_agenthandler through human → synthetic (non-internal,inherit_channel_pin=True,channel_prompt=None) → human turns on oneDiscord thread session with a
channel_overridesparent persona configured,and asserts the resolved
channel_promptpassed to_run_agentis"Channel hint."on all three turns (never flips toNone), and that thecombined ephemeral prompt keeps both
"Channel hint."and"Parent persona."throughout.Confirmed the test fails without the fix (reverted the 5 source files,
left the test in place):
With the fix restored, the new test and the rest of the suite pass:
Also ran the full set of gateway tests that exercise
MessageEvent,channel_prompt/channel_pin, goal continuation, or heartbeats (to catchany test double missing the new field), plus
tests/hermes_cli/test_goal_dispatch.py:This first pass caught two existing tests (
test_queued_final_ledger.py,test_discord_triggering_note_persistence.py) that buildpending_eventas a bare
SimpleNamespacewithout the new field; switched both_pinned_channel_inputscall sites togetattr(event, "inherit_channel_pin", False)(matching the existing defensivegetattr(event, "internal", False)pattern used elsewhere) so the new field is optional for callers/mocks, and
reran — all green.
ruff checkpasses on all changed files.AI assistance disclosure
This PR was written by an autonomous AI coding agent (Claude, Anthropic)
operating against this repository's own issue tracker and test suite, with
no human-authored code changes.
🤖 Generated with Claude Code