[mstsirkin]

Summary

An Enterprise Grid signs its members in twice: once at the org level - the session root-state.json lists, so stock slk offers the grid as a single workspace - and once per member workspace, whose per-workspace tokens the desktop app keeps in localConfig_v2. slk's --add-workspace desktop flow reads that store by team id, so a member workspace can be configured, but until now it could never connect: the grid rejects client.userBoot for member tokens outright, and that failure is fatal to bootstrap.Run.

This PR makes a member workspace boot from what the grid does serve its token, so a Grid user can add internal/external/member workspaces the way the desktop app presents them.

Background: the grid serves the two session classes different methods

Observed against a live Grid (one org-level workspace, two member workspaces):

method	org-level token	member-workspace token
client.userBoot / client.channels	OK	internal_error
client.counts / conversations.view / users.channelSections.list / client.shouldReload	OK	team_is_restricted
users.conversations / conversations.history / users.prefs.get / dnd.info / users.list	enterprise_is_restricted	OK
WebSocket (direct dial, agent_version set)	OK	OK

So neither token can run the full boot sequence, but between them everything is covered. The org-level session is what slk uses today (it is what root-state.json offers); this PR is about the member tokens.

What changes

• Client records enterprise_id from auth.test; IsGridMember() reports a session whose enterprise id differs from its team id.
• bootAdapter.UserBoot falls back to a synthesized boot result when userBoot fails and the session is a grid member:
  • Team record - from auth.test, which hydrateFirstSight needs for the workspace's parent cache row.
  • Mute prefs - via users.prefs.get, which the grid does serve member tokens.
  • Conversation lists stay empty on purpose: connectWorkspace builds the sidebar from users.conversations on every workspace (userBoot is only that call's fallback source), and an empty IsOpen makes dropClosedMPIMs a no-op.
• Off a grid, nothing changes: a userBoot failure still fails the workspace.
• The gate is IsGridMember rather than the error string, matching how a users.conversations failure is already treated on the org workspace (Enterprise App Token Support #5): keeping the session beats failing it.

Known costs on a member workspace

• No boot-time unread snapshot: client.counts is team_is_restricted for member tokens. bootstrap.Run already treats a counts failure as non-fatal, and WebSocket events keep the read state current, so only the boot-time badges are lost.
• conversations.view is rejected, so the open-channel path lands on its existing conversations.history fallback.
• Edge revalidation of member-team conversations fails (invalid_auth), which is the pre-existing behavior the org workspace already logs and retries.

Testing

• New tests: grid-member boot synthesizes the team record and mutes and skips userBoot; a users.prefs.get failure costs only the mute set; non-grid workspaces keep failing on userBoot; IsGridMember across org-level / member / non-grid / unconnected clients.
• go build ./..., go vet ./..., gofmt -l . clean; go test ./... passes.
• Verified against a live Grid with two member workspaces: both boot with their full sidebars (134 and 103 conversations, vs 64 for the org-level merged view, which also misses most DMs) and live websockets. Related: Failed to mint token: mint token: status 403 #111, Read xoxc tokens from desktop localConfig_v2 (fixes Enterprise Grid, #111) #115, Enterprise Grid parity: request-shape, call-pattern and fetch-scope (phase 1 of 3) #121, fix(slk): Enterprise Grid support — sidebar fallback + team-scoped edge revalidation #125, Enterprise Grid: usergroups.list called without team_id, member-workspace groups render as @group #214.

🤖 Generated with Claude Code

[greptile-apps]

Confidence Score: 4/5

[Medium impact] The changes affect Grid member boot and live events across connected workspace sessions.

The remaining helper-documentation requirement must be satisfied before merging.

Findings

1.  Removed reactions stay counted ▶
2.  New helpers are not listed ▶

Summary

This PR lets Enterprise Grid member workspaces connect when Slack rejects client.userBoot.

• Grid member workspaces connect when userBoot is blocked.
• Grid sessions share live events without repeating shared work.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Call client.userBoot] --> B{Succeeded?}
  B -->|Yes| C[Use returned boot result]
  B -->|No| D{Grid member and context still live?}
  D -->|No| E[Return error]
  D -->|Yes| F[Build team record and fetch mute preferences]
  F --> G[Continue workspace startup]
  C --> G

Loading

_{Reviews (14) · Last reviewed commit: "fix: process each Grid event once, not o..." · Reviewed by Greptile}

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Welcome, @mstsirkin, and thanks. This is a careful first PR. The method table in the description is very useful, and the fallback is tightly scoped: it only runs when userBoot has already failed and auth.test says the session is a grid member workspace. Off a grid, nothing changes, and a test pins that. The comment on gridMemberBoot says what is left at zero and why. I checked those claims against main: nothing in cmd/slk reads Self, DND, ChannelsPriority or EmojiCacheTS off the boot result, dropClosedMPIMs returns early when IsOpen is empty, bootstrap.Run already treats a client.counts failure as non-fatal, and the mute store reads res.Prefs through boot.Prefs.UnmarshalJSON, so the users.prefs.get body decodes into the same shape userBoot gives.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes. Adapter-level change in cmd/slk plus one accessor on slackclient.Client; internal/ui untouched	reading
CI	held: the title is feat:, so this needs a human look first (see verdict)	CI
Greptile	4/5, one P2: IsGridMember is not listed in AGENTS.md	Greptile
Tests cover the new behaviour	Yes. Grid-member boot (team record + mutes), a prefs failure that doesn't fail the boot, non-grid still failing, and IsGridMember across all four session shapes. The boot tests would fail without the change	reading

Findings (at 2ff263a)

Fix before merge:

1. internal/slack/client.go:291: list IsGridMember in AGENTS.md (I agree with Greptile). The repo's convention is that a reusable helper goes into the shared-code tables in the same commit. Future Grid-specific fallbacks will want this, and nothing else in the repo detects a member workspace today (I searched for enterprise_id/EnterpriseID). One row in "Text and rendering", next to the other slackclient entries, is enough. Something like: "Is this session an Enterprise Grid member workspace (not the org-level session)? | (*slackclient.Client).IsGridMember(); member tokens can't call client.userBoot/client.counts".

Optional:

2. cmd/slk/bootstrap_adapters.go:81: a cancelled context is swallowed too. If ctx is cancelled while userBoot is running on a member workspace, the error turns into a synthesized result. users.prefs.get, client.counts and users.conversations then fail on the same context, and connectWorkspace can carry on with an empty sidebar instead of returning the cancellation. Before this PR that path returned an error. Suggest if ctx.Err() != nil || !a.c.IsGridMember() { return nil, err }.
3. internal/bootstrap/bootstrap.go:349: Run's "What is fatal" doc still says userBoot is fatal because "there is no workspace without a self, a team and a conversation list". That's still literally true of Deps.Boot, but a reader won't learn that the grid adapter now degrades instead. One sentence pointing at gridMemberBoot would keep the two in step.
4. cmd/slk/bootstrap_adapters_test.go:1044: the name …KeepsOtherFailures doesn't match what the test checks, which is that a prefs failure does not fail the boot. Maybe TestBootAdapter_GridMemberPrefsFailureCostsOnlyMutes.

I couldn't verify one thing by reading: that the org-level session's auth.test always returns team_id == enterprise_id. If some grid returns a T… team id for its org-level token, IsGridMember would be true there too. That's harmless while userBoot succeeds for that token, but it would quietly downgrade a real userBoot outage there. Your live observation covers Red Hat's grid. It's worth a line in the IsGridMember doc saying that's where the rule comes from.

Verdict: needs minor changes. Adding the AGENTS.md row (finding 1) unblocks it. Flagged for @gammons: the feat: title adds user-visible functionality (Enterprise Grid member workspaces can now connect), so CI stays held until you've looked at it.

[mstsirkin]

Thanks for the thorough review - all findings addressed and force-pushed as a single updated commit (125aa17):

1. AGENTS.md row - added to the shared-code table, next to WaitOutRateLimit, naming IsGridMember() and pointing at gridMemberBoot.
2. Cancellation - bootAdapter.UserBoot now returns the error when ctx.Err() != nil before considering the fallback, exactly as suggested, and TestBootAdapter_GridMemberCancellationPropagates pins it.
3. Run's "What is fatal" doc - added a sentence noting the one degradation lives upstream of Run, pointing at gridMemberBoot in cmd/slk.
4. Test renamed to TestBootAdapter_GridMemberPrefsFailureCostsOnlyMutes.
5. IsGridMember doc - now states that the org-level session answering team_id == enterprise_id is an observation of a live grid rather than documented behavior, and what the cost would be if some grid answered a T… id for its org token.

Build, vet, gofmt and the full test suite pass locally.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks, @mstsirkin, for working through every point so quickly. I compared 125aa17 against 2ff263a: the rest of the change is the same, and all five follow-ups landed the way you described. The new cancellation test is a good one. Without the ctx.Err() check, the fallback would run and the prefs failure would be swallowed, so UserBoot would return no error and the test would fail. It pins the fix.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes. Adapter-level change in cmd/slk plus one accessor on slackclient.Client; internal/ui untouched. The new AGENTS.md row sits with the other slackclient helpers	reading
CI	held: needs a human look first (see verdict). GitGuardian passed	CI
Greptile	5/5, no new findings. Its earlier P2 (the AGENTS.md row) is fixed	Greptile
Tests cover the new behaviour	Yes. Grid-member boot, a prefs failure that costs only mutes, non-grid still failing, a cancelled context propagating, and IsGridMember across all four session shapes	reading

Findings (at 125aa17)

None. Earlier findings 1–4 are fixed (AGENTS.md row, cmd/slk/bootstrap_adapters.go:86 cancellation check, internal/bootstrap/bootstrap.go "What is fatal" note, test rename). The IsGridMember doc at internal/slack/client.go:279 now records that the org-level rule comes from observing a live grid.

Verdict: ready to merge. Nothing blocks it on my side. Flagged for @gammons: the feat: title adds user-visible functionality (Enterprise Grid member workspaces can now connect), and the PR now edits AGENTS.md, so CI stays held until you've looked at it.

[mstsirkin]

Follow-up commit f7e0210, because the feature in the first commit has a cost the live grid made obvious: the grid delivers every event to each of the user's connected sessions, so with the org workspace plus member workspaces connected, each message, invalidation and presence change ran the full handler pipeline once per socket (two to three times). On a busy grid that is a constant stream of redundant work - an idle slk measured 6-8% CPU on triple delivery, 0.2% after.

Sessions of one grid now share a byte-keyed frame dedupe (short TTL, distinct bytes always processed, hello/reconnect_url/user_typing exempt), with the active workspace's socket never gated since only its handler renders messages. Off a grid nothing changes - the gate is installed only when auth.test reported an enterprise id. Details and measurements are in the commit message.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks, @mstsirkin, for the follow-up and for measuring it. Running every event two or three times on a grid is real waste, and the commit message explains the design and its trade-offs well. Some parts are well judged: the dedupe key is the frame's exact bytes, so a frame that differs in any way always passes; the active socket is never gated; per-connection frames are exempt; and off a grid nothing changes. TestGridFrameGate_InactiveFirstActiveStillRenders covers the ordering that would have broken live rendering.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes in shape: the gate lives in cmd/slk, and slackclient only gets a hook and an accessor. internal/ui is untouched	reading
CI	held: needs a human look first (see verdict). GitGuardian passed	CI
Greptile	4/5, one P1: inactive workspaces miss updates. I agree, details below	Greptile
Tests cover the new behaviour	The dedupe and gate are well covered. Nothing checks that a second workspace's handler still applies its own state when a sibling's socket wins	reading

Findings (at f7e0210)

Fix before merge:

1. cmd/slk/grid_dedupe.go:97 (gridFrameGate): a dropped frame never reaches that workspace's handler, and many handlers write state that belongs to one workspace, not shared state. Each session has its own WorkspaceContext, and whichever socket wins the race is the only one updated:

   • OnPrefChange (cmd/slk/membership.go:16) updates h.wsCtx.MuteStore;
   • OnDNDChange and OnSelfPresenceChange (cmd/slk/presence.go:125, :142) set wsCtx.DNDEnabled/DNDEndTS/Presence, which the notifier reads in OnMessage (cmd/slk/rtm_handler.go:198);
   • OnConversationOpened (cmd/slk/conversations.go:18) appends to wsCtx.Channels and that workspace's finder;
   • the OnChannelSection* handlers (cmd/slk/sections.go:92 on) update wsCtx.SectionStore;
   • inactive OnMessage sends the ReadStateChangedMsg that refreshes that workspace's rail dot (cmd/slk/rtm_handler.go:362). The active handler doesn't send it, so when the active socket wins, an inactive sibling's dot never updates live.

   So with the org workspace active, a mute, a DND change or a new DM on that grid updates only the org session. Switch to the member workspace and its mutes, DND and conversation list are stale until the next boot. A stale DND flag there also means notifications that should be silenced aren't. Which workspace goes stale depends on a delivery race, so it will be hard to reproduce.

   Suggestion: turn the exempt list around and dedupe only frame types whose handlers write nothing but shared state (the SQLite cache and the UI), e.g. presence_change and reactions. Let anything that touches WorkspaceContext through on every socket: pref_change, dnd_updated*, manual_presence_change, the conversation-open events and channel_section*. message needs care because of the rail-dot message above. One option: dedupe its durable writes but still send ReadStateChangedMsg for every grid sibling. A test that sends one pref_change frame through two gated sessions and checks that both MuteStores change would pin this.

Optional:

2. cmd/slk/grid_dedupe.go:71: the dedupe also drops a byte-identical repeat on the same inactive socket within 5 s, not only a sibling's copy. A real state that flips and flips back inside the window (presence away → active → away, or DND on → off → on) has its third frame dropped there, so that workspace ends on the wrong value. Keying the entry by the socket that recorded it, and dropping only a copy from a different socket, would remove this.
3. internal/slack/client.go:286, :298: SetWSEventGate and EnterpriseID are new slackclient accessors. EnterpriseID would be worth a row in AGENTS.md next to IsGridMember, so the next Grid-specific change finds it.

Verdict: changes requested. Finding 1 unblocks it: per-workspace state has to keep updating on every grid session. Flagged for @gammons: the feat: title adds user-visible functionality (Enterprise Grid member workspaces can connect), the PR changes more than 300 lines, and it edits AGENTS.md, so CI stays held until you've looked at it.

[mstsirkin]

All findings from the dedup review addressed in the amended commit (4c09480):

Finding 1 (per-workspace state) - the exemption is now a denylist, as suggested, and it errs the safe way: a frame type not listed is at worst processed redundantly (the pre-dedupe behaviour), while a type wrongly treated as shared would silently go stale. Never deduped: pref_change, dnd_updated/dnd_updated_user, manual_presence_change, the four channel_section* types, mpim_open, and thread_subscribed (its UpsertThreadSubscription keys on workspace_id, so each session must persist its own row - same staleness class as the ones you listed). For message I took the option you described: the durable writes and the notification stay deduped (un-deduping it would triple desktop notifications, since the notify path runs before the active check), and whichever handler processed the frame now sends ReadStateChangedMsg for every sibling workspace - the active arm sends it for the inactive siblings (its own dot is owned by the NewMessageMsg path), the inactive arm for all of them. rtmEventHandler.gridSiblings is wired in main.go from router.All() filtered by EnterpriseID, read at event time so late-connecting siblings are included.

Finding 2 (same-socket repeat) - entries now record the socket that marked them, and only a copy from a different socket drops, so the away-active-away flip lands on its final value. TestGridEventDedupe_SameSocketRepeatPasses pins it.

Finding 3 (AGENTS.md) - EnterpriseID got its own row next to IsGridMember's, naming the dedupe and sibling fan-out that key on it.

Tests: TestGridEventDedupe_PerWorkspaceFramesPassEverywhere covers the denylist through two gated sessions (the MuteStore scenario you suggested, at the gate level - pref_change reaches both handlers, and the existing handler tests pin that OnPrefChange mutates the MuteStore); TestOnMessage_GridSiblingRailRefresh_{Inactive,Active} pin the dot fan-out in both arms; TestOnMessage_NoGrid_RefreshesOnlyOwnWorkspace pins the off-grid behaviour as unchanged. Full suite plus -race on cmd/slk pass.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks, @mstsirkin, for working through the last round so carefully. Turning the dedupe into a denylist is the right way round, and the comment on gridPerWorkspaceFrames says why. Keying entries by the socket that recorded them fixes the away-active-away case, and TestGridEventDedupe_SameSocketRepeatPasses pins it. Fanning ReadStateChangedMsg out to grid siblings is a neat answer to the rail-dot problem, and reading router.All() at event time catches siblings that connect late. This re-review covers the change since f7e0210.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes: the gate and fan-out stay in cmd/slk; internal/ui is untouched. The new AGENTS.md row for EnterpriseID is helpful	reading
CI	held: needs a human look first (see verdict). GitGuardian passed	CI
Greptile	3/5: P1 socket IDs can collide (I agree, finding 2), P1 inactive workspaces miss updates (partly still true, finding 1)	Greptile
Tests cover the new behaviour	Good for the dedupe, the same-socket rule and the sibling rail refresh. The per-workspace test covers the listed types only, so it can't catch the missing ones below	reading

Findings (at 4c09480)

Fix before merge:

1. cmd/slk/grid_dedupe.go:87 (gridPerWorkspaceFrames): some frame types that write per-workspace state are still deduped, because the denylist names one event type where the dispatcher maps several to the same handler (internal/slack/events.go):

   • im_created, im_open, group_joined and channel_joined all go to OnConversationOpened, like mpim_open, and append to wsCtx.Channels and that workspace's finder. Only mpim_open is listed, so a new DM or a joined channel shows up only in whichever session won the race.
   • thread_unsubscribed shares OnThreadSubscriptionChanged with thread_subscribed, and that writes a workspace_id-keyed row (cmd/slk/thread_subscriptions.go:263). The reason you give for listing thread_subscribed applies to it too: an unsubscribe would leave the sibling's row active.
   • member_joined_channel and member_left_channel update wsCtx.Membership (cmd/slk/membership.go:35, :42).
   • thread_marked writes thread read state keyed by h.workspaceID (cmd/slk/markread.go:272 on).

   Suggestion: add those eight types. To keep the list from drifting from the dispatcher, a test could take the case labels that reach a wsCtx-writing handler and check each one is in gridPerWorkspaceFrames. Even a hand-written list of every type in events.go that the test sorts into "shared" or "per-workspace" would make the next new event type a deliberate choice.

2. cmd/slk/grid_dedupe.go:62: socketSeq is a plain seq++ with no lock, and gridFrameGate calls it from each workspace's connect goroutine (cmd/slk/main.go:1578, :1638), which run in parallel. Two sessions of one grid can get the same socket ID, and then every sibling copy counts as a same-socket repeat and passes. That's only the old redundant work, not lost state, but it's a data race that -race would report if a test connected two sessions at once. sync/atomic (atomic.Uint64 on gridEventDedupe), or taking d.mu in socketSeq, fixes it.

Optional:

3. cmd/slk/grid_dedupe.go:150 (gridFrameGate): when an inactive sibling's copy of a message frame arrives first, it does the durable writes and the desktop notification, and then the active socket's copy passes too (it's never gated) and runs the notify path again. So in that order the user can still get two notifications for one message. It's no worse than before the dedupe, but if you want one notification per message, the active handler could skip the notify step when allow reported the frame as already seen.

Verdict: changes requested. Finding 1 unblocks it: every event type whose handler writes WorkspaceContext or workspace-keyed rows has to run on every grid session. Finding 2 is a two-line fix worth doing in the same push. Flagged for @gammons: the feat: title adds user-visible functionality (Enterprise Grid member workspaces can connect), the PR changes more than 300 lines, and it edits AGENTS.md, so CI stays held until you've looked at it.

[mstsirkin]

Both findings fixed, plus your optional #3, in the amended commit (03bd79f):

Finding 1 - all eight types added (im_created, im_open, group_joined, channel_joined, thread_unsubscribed, member_joined_channel, member_left_channel, thread_marked), plus two more the same rule catches: user_invalidated and dnd_invalidated both write wsCtx.PeerStatus (peer_status.go:414-423 - each workspace's refresher queue is per-workspace state). Their handling is a set insert, so un-deduping them costs nothing measurable. And I took the test you suggested: TestGridPerWorkspaceFrames_ClassifiesEveryDispatchedType lists every type events.go dispatches, sorted into per-workspace / shared / connection-exempt, and asserts the denylist is exactly the per-workspace column - a new event type now fails the test until someone classifies it.

Finding 2 - socketSeq is an atomic.Uint64 on the dedupe, Add(1) in gridFrameGate.

Finding 3 (optional, taken) - notifyOnce keys (channel, ts) process-wide with a one-minute TTL and gates the h.notifier.Notify call, so the inactive-first delivery order can no longer produce two notifications for one message. TestNotifyOnce pins it. The dedupe tests that used dnd_invalidated/user_invalidated frames as their examples moved to presence_change now that those types are per-workspace.

Full suite plus -race on cmd/slk and internal/slack pass.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks, @mstsirkin. This push fixes both blockers from last round. The denylist now covers every type the dispatcher maps to a per-workspace handler: I checked each case label in internal/slack/events.go against it. socketSeq is an atomic.Uint64. The classification test makes moving a type a deliberate choice, and notifyOnce handles my optional point about duplicate notifications. This re-review covers the change since 4c09480.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes: the gate, fan-out and notify guard stay in cmd/slk; internal/ui is untouched	reading
CI	held: needs a human look first (see verdict). GitGuardian passed	CI
Greptile	P1: counts can go up twice (I agree, finding 1). P1: race in notifyOnce (I agree, finding 2). P2: IsGridMember isn't in AGENTS.md (it is, at AGENTS.md:139)	Greptile
Tests cover the new behaviour	Good for the classification and notifyOnce itself. Nothing checks that the same message frame, delivered to an inactive and then the active session, writes counts once	reading

Findings (at 03bd79f)

Fix before merge:

1. cmd/slk/grid_dedupe.go:167: the active socket's copy always passes, so when an inactive sibling's copy arrives first, both handlers do the shared cache writes. notifyOnce covers the notification, but not these writes:

   • cmd/slk/rtm_handler.go:346: IncrementChannelMentionCount runs twice. One @-mention shows a badge of 2 until the next channel_marked or counts refresh.
   • cmd/slk/rtm_handler.go:527 (OnReactionAdded): the user is appended a second time and Count goes up by 2. OnReactionRemoved (:556) takes Count down twice the same way while other users remain.

   Before this PR a grid had only one session, so this is new. Suggestion: make the two writes idempotent, as notifyOnce does for notifications. Bump the mention count once per (channel, ts), and skip the reaction append when userID is already in r.UserIDs (and the remove when it isn't). A test that feeds one frame to an inactive gate and then to the active one, and checks the counts, would pin it.

2. cmd/slk/notify_once.go:27: notifyOnceState.now is read and assigned before the mutex is taken. Each workspace's socket goroutine calls this, so two first notifications at once are a data race. Initialise now where the state is declared, or move the read under mu.

Optional:

3. cmd/slk/grid_classification_test.go:13: the comment says a new event type "fails this test until someone sorts it", but the test never reads events.go. It only checks that gridPerWorkspaceFrames matches its own hand-written list, so a new case in the dispatcher passes without being sorted. To make the comment true, read the case "…" labels out of ../../internal/slack/events.go in the test (a regexp is enough) and require each one to be in one of the three lists. Also, message_changed and message_deleted are subtypes of message, not frame types, so they can come out of shared.

Verdict: changes requested. Findings 1 and 2 unblock it; both are small. Flagged for @gammons: the feat: title adds user-visible functionality (Enterprise Grid member workspaces can connect), the PR changes more than 300 lines, and it edits AGENTS.md, so CI stays held until you've looked at it.

[mstsirkin]

Both doc findings in the amended commit: the gridSiblings field doc, the main.go wiring comment and the AGENTS.md row now describe grid detection rather than a fan-out, and onceMap got its row (notifyOnce / mentionBumpedOnce, keyed by (channel, ts)). No code change; full suite passes. The grid reaction-removal gap stays as documented.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks @mstsirkin. af982a9 takes both optional findings from last time. The gridSiblings field doc (cmd/slk/rtm_handler.go:51-56) and the main.go:1647-1651 comment now say the closure only tells OnMessage whether it's on a grid. AGENTS.md has a row for onceMap / notifyOnce / mentionBumpedOnce, and I checked that they exist at cmd/slk/notify_once.go:20, :62 and :66. Only comments and AGENTS.md changed since 6834252; git range-diff shows no code change.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes; the grid logic stays in cmd/slk	reading
CI	held: PR carries needs human review (run 39910598740)	CI
Greptile	4/5. One new P1: a new group DM can stay missing on an inactive grid sibling. I confirmed it by reading (finding 1)	Greptile
Tests cover the new behaviour	Yes for the dedupe, the once-guards and the rail refresh. Nothing covers message-driven discovery on a sibling	reading

Findings

Fix before merge:

1. On a grid, a new group DM can be discovered only by the session whose copy of the message wins. This code was there at 6834252 too, and I missed it last pass; Greptile caught it this time. The path:

   • OnMessage starts with discoverConversation(channelID) (cmd/slk/rtm_handler.go:147). It comes from main, and its doc (:91-95) says it exists because a group DM another user creates mid-session can arrive only as messages, with no conversation-opened event.
   • message isn't in gridPerWorkspaceFrames, so allow dedupes it (cmd/slk/grid_dedupe.go:127).
   • gridFrameGate always passes the active session's copy (:163-171). So when the active socket records the frame first, every inactive sibling's copy is dropped. Their handlers never run discoverConversation, and the conversation never reaches their channelTypes, sidebar or finder.

   When the user switches to that sibling, I found nothing that adds the group DM. It shows up only once a later message in it reaches that session's handler, either while the session is active or by winning the race. It's limited to grid users, but it's the per-workspace staleness the denylist exists to prevent.

   There are two ways to fix it; your call which:

   • (a) Treat message as per-workspace. OnMessage's shared writes already look safe to repeat. UpsertMessage, SetChannelSyncedAt, AdvanceChannelLatestSyncedTS and UpdateChannelReadState(…, true) are upserts or monotonic, and the mention bump and notification go through mentionBumpedOnce / notifyOnce. Check each before relying on that. The cost: every sibling writes the cache once per message, and the inactive arm's ReadStateChangedMsg repeats per sibling (the refresh Greptile flagged on 6d5f3ef).
   • (b) Keep the dedupe, but let a dropped message frame still trigger discovery on that session. For example, give the gate a per-session hook that queues the channel ID for that handler's serialized owner. The PendingEventHandler wake-up already exists for this kind of local work.

   Either way, add a test: two handlers on one grid, the active one's copy first, and the sibling ends up with the new group DM in its conversation lists.

Optional:

2. As before: on a grid, when a user missing from a truncated UserIDs list removes a reaction, the count still doesn't go down. The code comment explains why, and the next history fetch corrects the count.

Verdict: needs minor changes. Make message-driven discovery reach every grid sibling, with a test, and this is ready. Flagged for @gammons: over 300 changed lines, a feat title, and it edits AGENTS.md.

[mstsirkin]

Finding 1 fixed with option (b) in the amended commit, keeping the dedupe: gridFrameGate takes an onDroppedMessage hook, and main.go wires it to that session's handler.discoverAndPublish - the discovery block extracted from OnMessage's start (discovery plus publish). A dropped message frame still parses its channel through the hook, so the session that lost the delivery race learns the conversation; the gate runs on the client's serialized event-owner goroutine, so the hook is ordered with every other handler callback, and the shared unread state the publish wants was already written by the winner.

Tests: TestGridFrameGate_DroppedMessageRunsDiscovery (hook runs for a dropped message frame, not for non-message frames, not on the active gate) and TestGridDroppedMessage_StillDiscoversOnSibling - the two-handlers-on-one-grid case you asked for: the active session's copy processes the message, the sibling's dropped copy still ends with the group DM in its workspace list and channelTypes. Full suite plus -race pass.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks @mstsirkin. This fixes last pass's blocker the way I'd hoped (option (b)), and splitting the PR into two commits makes it easier to read. gridFrameGate now takes an onDroppedMessage hook (cmd/slk/grid_dedupe.go:171-188). main.go:1706 wires it to that session's handler.discoverAndPublish (cmd/slk/rtm_handler.go:151), and that method runs the same two steps OnMessage uses. I checked that the gate runs on the client's dispatch goroutine, the same one that drains OnPendingEvents (internal/slack/client.go:543), so the hook is ordered with every other callback. Against af982a9's tree, only grid_dedupe.go, main.go, rtm_handler.go and two test files changed.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes; the grid logic stays in cmd/slk	reading
CI	held: PR carries needs human review (run 39917871251)	CI
Greptile	5/5. Two new P2s; I confirmed both by reading (findings 1 and 2)	Greptile
Tests cover the new behaviour	Yes. TestGridFrameGate_DroppedMessageRunsDiscovery covers the gate→hook path. TestGridDroppedMessage_StillDiscoversOnSibling is the two-handler case I asked for; it calls discoverAndPublish directly, so only reading confirms the main.go wiring	reading

Findings

No blockers. Optional:

1. The hook runs discovery for message frames that never reach OnMessage (cmd/slk/grid_dedupe.go:183). The dispatcher calls OnMessage only for subtypes "", bot_message, thread_broadcast, file_share and me_message when the message isn't ephemeral, and for non-ephemeral message_changed (internal/slack/events.go:539-562). The hook also fires for message_deleted, ephemerals and every other subtype. So a dropped copy can add a conversation to a sibling's sidebar that the normal path deliberately skips. It's rare in practice, since those conversations are usually already known. Suggestion: decode subtype and is_ephemeral (plus message.is_ephemeral for message_changed) and run the hook only for frames that reach OnMessage. Better still, put that predicate in one exported helper in internal/slack that the dispatcher also uses, so the two lists can't drift.
2. A byte-identical repeat from a second socket can be dropped after the first socket disconnects (cmd/slk/grid_dedupe.go:136). This was already in af982a9, and I didn't raise it then. Socket A delivers presence_change … away and then drops. The state goes active and back to away within 5 s, and socket B delivers both. B's away matches A's entry and is dropped, so on an inactive workspace the shared presence stays active. This needs a disconnect inside the TTL, so it's narrow. One option is to add presence_change to gridConnectionExempt; it's cheap to handle twice.
3. As before: on a grid, when a user missing from a truncated UserIDs list removes a reaction, the count still doesn't go down. The code comment explains why, and the next history fetch corrects the count.

Verdict: ready to merge. The optional findings above are worth a look but don't block. Flagged for @gammons: over 300 changed lines, a feat title, and it edits AGENTS.md.

[mstsirkin]

Both optional findings taken in the amended commit:

Finding 1 - the hook now consults slackclient.MessageFrameDrivesOnMessage (new in internal/slack/events.go, next to the dispatch switch it mirrors, with a doc note that the two move together): the gate decodes subtype plus the frame's and the embedded message's ephemeral flags, so a dropped message_deleted, ephemeral, or any other non-OnMessage frame no longer runs discovery - while a non-ephemeral message_changed still does, matching the dispatcher. The gate test pins both directions. (The classification test learned to skip the helper's own one-tab case labels, which the regexp now also sees.)

Finding 2 - presence_change moved to gridConnectionExempt with a comment explaining the socket-swap staleness it prevents; presence is a cheap set-value, so handling it per-session costs nothing. The dedupe tests moved their shared-frame example to user_change.

The truncated-reaction gap stays as documented. Full suite plus -race pass.

[greptile-apps]

New helpers are not listed

MessageFrameDrivesOnMessage adds a reusable exported helper, but the shared-code tables do not list it. AGENTS.md requires adding reusable helpers to those tables in the same commit. Add its purpose and when callers should use it before merging. SetWSEventGate and discoverAndPublish are also missing.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks @mstsirkin, you took both optionals from the last pass. The discovery hook now only fires for frames OnMessage would see (MessageFrameDrivesOnMessage), and presence_change is exempt from the dedupe, which removes the stale-presence case. The new hook test covers message_deleted, ephemerals and a shared frame (no hook), and message_changed (hook), and the dedupe tests moved to user_change. Good changes.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes; all in cmd/slk and internal/slack. One convention gap, see finding 1	reading
CI	held: PR needs human review (run 39923134996)	CI
Greptile	4/5 at f1c6c14. Three P2s: helpers not listed in AGENTS.md (partly out of date), no lockstep test for the message rules (confirmed, finding 1), removed reactions stay counted (an older thread; off a grid this is fixed, on a grid it's the documented conservative skip)	Greptile
Tests cover the new behaviour	Yes for the gate. Not for the predicate matching the dispatcher	reading

Findings

Fix before merge:

1. MessageFrameDrivesOnMessage is a second copy of the dispatcher's rules, and they already disagree in one case (internal/slack/events.go:531 at f1c6c14). The message arm of dispatchWebSocketEvent doesn't call the predicate. It repeats the switch, and only a comment keeps the two in step. AGENTS.md (Conventions, "Two implementations that must stay parallel?") asks for an interface or a lockstep test here, not a comment. There's already a gap: a message_changed frame with no embedded message stops at events.go:575 without calling OnMessage, but the predicate returns true for it. In grid_dedupe.go:197 the missing message only feeds the ephemeral flag, so the hook still runs discovery for that frame. That's harmless today, but it is exactly the drift the comment says can't happen. Suggestion: have the message arm decide with the predicate itself (add a hasEmbedded bool argument for the nil case). Or add a table test that runs dispatchWebSocketEvent with a recording handler for each subtype × ephemeral flag × embedded/absent message, and checks OnMessage was called exactly when the predicate says so.

Optional:

2. MessageFrameDrivesOnMessage and SetWSEventGate aren't in the AGENTS.md tables (Greptile P2). Greptile's other "not listed" findings are out of date: IsGridMember, the grid gates and onceMap/notifyOnce/mentionBumpedOnce are at AGENTS.md:139-141.

Verdict: needs minor changes. Making the dispatcher use the predicate, or adding the lockstep test, unblocks it. Flagged for @gammons: feat, user-visible (Enterprise Grid member workspaces), 1,575 lines, and it changes AGENTS.md.

[mstsirkin]

Finding 1 fixed both ways in the amended commit:

The gap: MessageFrameDrivesOnMessage now takes hasEmbedded and returns false for message_changed with no embedded message - matching the dispatcher's if msg.Message == nil { break }. The gate passes it (evt.Message != nil), so the hook no longer runs discovery for that frame either.

The lockstep guard: TestMessageFrameDrivesOnMessage_MatchesDispatcher (in internal/slack/events_test.go) builds each frame - subtype × frame-ephemeral × embedded present/absent/ephemeral - runs it through dispatchWebSocketEvent with a recording handler, and asserts OnMessage fired exactly when the predicate says. Ten cases, including the two the reviewer called out (changed no embedded, changed embedded ephemeral). I took the test rather than restructuring the dispatcher onto the predicate because the arm's payload selection differs per subtype (message_changed passes the embedded message's fields with edited=true), so a boolean gate would just move the switch, not remove it - the test pins the two together without pretending they're one thing.

Finding 2 (optional, taken): MessageFrameDrivesOnMessage and SetWSEventGate rows added to AGENTS.md.

Full suite plus -race pass.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks @mstsirkin. Last pass's blocker is fixed. MessageFrameDrivesOnMessage now takes hasEmbedded, so message_changed with no embedded message returns false, the same as the dispatcher's msg.Message == nil break (internal/slack/events.go:575). The gate passes evt.Message != nil. TestMessageFrameDrivesOnMessage_MatchesDispatcher runs each frame through dispatchWebSocketEvent and checks the predicate agrees, which is the lockstep test AGENTS.md asks for. Your reason for keeping the switch makes sense: the arms pick different payloads, so a boolean gate wouldn't remove it.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes; code is all in cmd/slk and internal/slack. The two new AGENTS.md rows are in the wrong place (finding 1)	reading
CI	held: PR needs human review (run 39933745806)	CI
Greptile	4/5 at 3a5ddb4. "Removed reactions stay counted" is the older thread (fixed off a grid, the documented conservative skip on one). "New helpers are not listed": they're listed now, but see finding 1	Greptile
Tests cover the new behaviour	Yes; the lockstep test covers the predicate against the dispatcher	reading

Findings

Fix before merge:

1. The two new AGENTS.md rows sit above the intro, outside any table (AGENTS.md:2-3 at 3a5ddb4). They're between # AGENTS.md and "Orientation for anyone…". With no header row they don't render as a table, so the file now opens with two lines of raw pipes. Suggestion: move them into the Text and rendering table next to the other grid rows (IsGridMember, EnterpriseID, onceMap, AGENTS.md:142-144) and drop the stray blank lines at the top.

Optional:

2. TestMessageFrameDrivesOnMessage_MatchesDispatcher declares wantOnMessage per case but never checks it (internal/slack/events_test.go, the got != want check). As written, it shows the predicate and dispatcher agree, not that either is right. Adding if got != tc.wantOnMessage { … } pins both with one line. thread_broadcast, file_share and me_message cases would cover the rest of the first arm cheaply.

Verdict: needs minor changes. Moving the two AGENTS.md rows into the table unblocks it. Flagged for @gammons: feat, user-visible (Enterprise Grid member workspaces), more than 300 lines, and it changes AGENTS.md.

[mstsirkin]

Both findings in the amended commit:

Finding 1 - the two rows moved into the Text and rendering table, next to IsGridMember / EnterpriseID / onceMap, and the stray lines at the top of the file are gone. (They landed above the intro because the insertion was verified by counting matches rather than reading the diff - which is also why the missing blank-line and the test's assertion nesting got caught only at review this round. Both habits are fixed.)

Finding 2 (optional, taken) - the test now checks got != tc.wantOnMessage as its own assertion, before the predicate agreement, so each case pins the expected behavior AND the dispatcher/predicate lockstep independently. thread_broadcast, file_share and me_message cases added; thirteen cases total.

Full suite passes.

[gammons]

🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code.

Thanks @mstsirkin. Both points from last pass are fixed. The two AGENTS.md rows for MessageFrameDrivesOnMessage and SetWSEventGate are now in the Text and rendering table with the other grid rows, and the top of the file matches main again. TestMessageFrameDrivesOnMessage_MatchesDispatcher now checks wantOnMessage as well as dispatcher-vs-predicate agreement, and covers thread_broadcast, file_share and me_message (internal/slack/events_test.go:1041-1043, 1072 at 780b380). Nothing else changed since 3a5ddb4.

Check	Result	Source
Fits AGENTS.md and the re-architecture plan	Yes; code is all in cmd/slk and internal/slack, and the new helpers are in the AGENTS.md tables	reading
CI	held: PR needs human review (run 39940349416)	CI
Greptile	4/5 at 780b380. Its two listed findings are older threads. "New helpers are not listed": they are all listed now (AGENTS.md:139-143). "Removed reactions stay counted": fixed off a grid; on a grid it's the documented conservative skip	Greptile
Tests cover the new behaviour	Yes; the lockstep test now pins both the predicate and the dispatcher to the expected result	reading

Findings

None blocking, and nothing new.

Verdict: ready to merge. Flagged for @gammons: feat, user-visible (Enterprise Grid member workspaces), more than 300 lines (1,631), and it changes AGENTS.md.