Repository navigation
Report Claude subagent activity and keep automatic session replies out of channels - #3587
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
trissim
enabled auto-merge
October 29, 2026 05:53
|
debt-src at 1d9c6b2, every finding: |
trissim
force-pushed
the
work/slot-activity--agent-comms
branch
from
October 29, 2026 06:17
04d82b3 to
4b6e590
Compare
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
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.
4 commits, 13 files, +152/-34: Claude slots report work; automatic replies stay in DMs
Two observed failures in one Claude-session repair. Subagent
PreToolUseevents previously vanished atSubagentCall.judge; an occupied slot therefore looked ready while working. Separately,ReceivingMonitor.confirmtreated every delivered destination as an automatic reply route, broadcasting ordinary final text to mentioned channels.Activity: use the existing phase and activity owners
Before:
SubagentHook(SubagentSeats)alone owned the agent-id/seat mapping.SubagentCall(GatedCall)discarded every tool call.ThreadOutput.enterprojected phases into activity.After:
SeatedSubagent(SubagentSeats)owns the existing seat lookup and activity publication. BothSubagentHook(SeatedSubagent)andSubagentCall(GatedCall, SeatedSubagent)inherit it; commander forks use the existing agent-id-derived fork name.ModelWaitPhase; PreToolUse entersToolRunningPhase(tool_name).AgentActivity.enter_phaseowns phase-to-activity projection for native turns, bridge outputs and subagents, using the existing ActivityStore.ThreadOutput.enterconsumes it rather than maintaining a second projection.ThreadExecution.holds_activityretains the existing native-turn lifetime.SubagentExecutionoverrides it using the occupied registration, so both slots and commander forks stay current until stop/session-end.SubagentCaller.endentersIdlePhasebefore the existing unregister/archive operation. No additional activity store, per-slot file, event-name switch, exception swallowing or process mirror.The hook uses
client_wireagainst its existing WatchRecord root, retaining Core's published-route client lifetime and refusal behavior.Bound callbacks receive the record from its owner
The initial implementation copied
*state.bindings()into its new activity callbacks. That supplied an unstated exactly-one-record assumption outsideRecordedWatch, and mypy correctly found six errors. The fix changes the existingWatchState.by_rolecontract: fleet and commander callbacks takeWatchRecord;RecordedWatchinvokes the selected callback withself.record. The unbound implementation never invokes those callbacks. All five production callers migrate together: activity, subagent start, stop, session-end, and deck selection. The start/stop/end callers now pass their bound methods directly instead of tuple-splat lambdas. No callback binding cast, overload, fallback or extra state is added.bindings()remains only for the actual loops inqueue_bypass.py:216andskill.py:248. Searchrg -n 'by_role\\(|bindings\\(' src/agent_comms_hooksshows all five migrated callers and the two remaining iteration sites;*state.bindings()has zero matches in forks.py. Retained source map:/home/ts/.cache/agent-scratch/slot-activity/by-role-ast-before.log, 11 hook modules, zero parse omissions; dynamic callback resolution is not inferred by that AST map.Targeted mypy on all three changed hook modules succeeds: no issues found in 3 source files, exit 0. The current runtime does not include mypy; the first attempt failed with
No module named mypy, and the first cached invocation lacked librt. The completed check reused existing cached Python-3.14-compatible mypy 2.3.1, mypy_extensions, pathspec, librt and ast_serialize through PYTHONPATH without installing/changing runtime packages. Command:MYPYPATH=src timeout -s INT 300 python -m mypy --explicit-package-bases --follow-imports=silent --ignore-missing-imports src/agent_comms_hooks/forks.py src/agent_comms_hooks/watch.py src/agent_comms_hooks/skill.py. Full result:/home/ts/.cache/agent-scratch/slot-activity/by-role-mypy-result.log. The four behavior selectors still need the integration owner's run on the final head; no unit pytest was launched.Rebased this owned branch onto main 86ced58 before the owner correction. Earlier test receipts continue to identify their original pre-rebase heads, not the final head.
Replies: destination types own automatic eligibility
Before: complete monitor confirmation returned
(message.exact_reply_target,)for every destination.After:
NamedTarget.automatic_reply_targetsreturns no routes.ThreadTargetalone supplies its sender route.ReceivingMonitor.confirmasks that existing target family; channel sends remain explicit. It also preserves P924/#3581's independentstarts_turn_forresponse-policy gate so informational/notice messages cannot acquire automatic replies merely by being consumed.No
startswith('#'), channel-name roster, message flag, new reply store or change to explicit channel sends. The existing stagedReplyingThreadOutputstill publishes at actual completion, with its original clearing/idempotence behavior.Source evidence
Retained AST map before editing at acdac6e: refactor-audit
Package.load(Repository(...), 'HEAD', root)oversrc/agent_comms(588 modules) andsrc/agent_comms_hooks(11 modules), zero parse omissions. Declarations, inheritance, relevant imports and reads/writes/calls were retained at/home/ts/.cache/agent-scratch/slot-activity/ast-before.log. Sites were read semantically through slot start/seat/stop/session-end, ActivityStore and registry projection, monitor confirmation, and staged final publication. Dynamic callback resolution is not proved by this AST evidence.Before/after derivation searches:
rg -n 'enter_phase|holds_activity|automatic_reply_targets|class SeatedSubagent|class SubagentCall' srcandrg -n 'exact_reply_target|delivered_targets|reply_targets' src.IMPL-4 / fact: a subagent's tool phase must reach activity; at: old
read_gate.py:110no-op; derive: phase-owned activity through AgentActivity; by:SubagentCall(SeatedSubagent)and native/bridgeThreadOutputsharingenter_phase.BOUND-8 / fact: which destination accepts an automatic final; at: old
harness_delivery_states.py:118flattened all destinations into one route; derive: existing target-family behavior; by:NamedTargetempty default andThreadTargetoverride.Proof and remaining acceptance
Completed source checks: all 11 changed Python files compile in memory;
git diff --checkpasses; production imports of SubagentCall and ReceivingMonitor succeed. Branch push was verified againstgit ls-remote; exact head 69054ca.Tests added/extended through real entrypoints, with no patches of product code:
tests/test_commander_forks.py::test_a_subagent_start_registers_its_fork_and_a_resume_reuses_the_archived_thread: real start, PreToolUse and stop for a commander fork, checking thinking/tool/idle activity.tests/test_commander_forks.py::test_a_unit_sessions_subagents_take_role_slots_and_an_unbound_sessions_take_none: real hook start/PreToolUse/stop for an occupied seat, registry activity projection and a deliberately old event proving a long-running tool does not become idle.tests/test_claude_automatic_reply.py::test_channel_mention_and_dm_share_a_turn_without_broadcasting_the_final: real monitor socket/probe/confirmation for an outstanding #comms mention plus DM, Claude assistant/system record decode, final publication only to the DM, repeated-completion control and explicit channel-send control.The existing read-gate test now creates the child its simulated PreToolUse belongs to; it no longer asks a bound session to report activity for an agent that never started.
The integration owner ran the three selectors on initial head
69054ca2cwith the installed runtime: 2 passed, 1 failed. Both real hook lifecycle tests passed. The bridge control counted viewer's setup-generated channel join/leave announcements. The second commit tried to record a setup cut usingmessage_history()[-1]; that was my error becauseBus.message_historyat bus.py:234 returns an Iterator. Onbe1bfe48a, the owner's expanded four-case run was 3 passed, 1 failed: both lifecycle controls and the existing read-gate control passed; the bridge control crashed on that subscript before exercising final publication.The third commit removes that history lookup entirely. It uses
channel.seq, already returned by the setup's committed send, and counts only viewer sends after that cut. No body filter or materialization of history is added. Production source is unchanged by both test corrections. The latest corrected head still needs the owner's rerun; compile checks are not claimed as execution proof.Unit pytest is prohibited by standing instructions. The integration owner owns the combined run with
PYTHONPATH=$PWD/src:$PWD/tools timeout -s INT 300 python -m pytest -p no:xdist -o faulthandler_timeout=60 .... No tests, full suite or ratchets were run by this unit. Installation and live Toad acceptance remain with the integration owner.Integration
Read open-PR file inventory before editing. #3581/P924 repairs notice reply policy and overlaps the ReceivingMonitor return; its old mentioned-channel expectation conflicts with Tristan's newer no-channel-auto-reply instruction. Its owner and claude-code were told. The commander's decision is #3581 lands unchanged first; this PR then integrates current main, preserves the policy gate and changes that mentioned-channel expectation to no channel publication. At the test-correction fetch, #3581 is still open and its test is not yet on main. This PR owns the destination-type restriction and slot activity, not a second notice implementation. P918's nominal path migration may require import-path rebasing; no competing nominal migration is included.
Assumed: Claude emits SubagentStart before that agent's PreToolUse and SubagentStop or SessionEnd retires its registration; the existing WatchClaim role and seat mapping are authoritative; configured slot/fork root binding does not change during a tool event; integration owner runs the focused controls and verifies actual Toad activity plus a real answered DM after install.