Talk to an OpenBot coworker from Slack, as yourself - #297
Conversation
242b5d3 to
e56dfbc
Compare
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
Reviewed the two Slack commits (efe35a6..HEAD) — I skipped #296's commit per the merge notes. Four things inline, one that has no line to hang off, and a set of things I checked and cleared that I want to write down so the next reader doesn't have to redo the work.
No changelog entry for any of this
Against this PR's actual base (fb0c797) CHANGELOG.md is byte-identical — so the entire managed-Slack surface ships with no entry. Two separate things are going on:
- Within the branch, the second commit removes the two
Unreleasedentries the first commit added (the named-coworker routing and the connector-read refusal). Since #296 lands first, that reads as a silent revert of #296's changelog on merge. - Nothing is added for Slack itself.
Both look like rebase fallout rather than a decision.
Checked and cleared
Writing these down because each one reads like a bug until you chase it, and two of them are load-bearing invariants that live in a dependency:
- Per-turn threadId. The self-hosted
channels-slackconversation store mints a fresh random threadId per turn, which would breakgetByChannelsThreadIdon every follow-up and re-bind the thread each time. The managedchannels-intelligencedelivery adapter — the one this deployment uses — passes the stableconversationKey, sochannelsThreadId === conversationKeyand the binding holds. Correct as written, but the whole append-only-binding design rests on a property of the adapter you happen to be on. Worth a comment at the binding site naming that. - The double
protectinOpenBotChannelAgent.run. It re-protects the execution into a nested copy beforeresolve()setsagentId, which reads like every computer tool must fail withSlackComputerContextError. It doesn't —runAgentLoopinvokes tool handlers afterawait agent.runAgent(...), so they run in the outer context whose objectresolve()mutated. Also worth a comment; a later refactor that moves handler invocation inside the await would break every Slack computer tool with an error that points nowhere near the cause. pendingExecutionForreturningqueue[0](oldest) rather than the active execution is only reachable if a thread operation detaches the async context.trackOperationruns the operation in-context, so this is defensive only.- zod v4 in
shared/computer-tool-contracts.tsis fine — channels-core'stoJsonSchemapreferstoJSONSchema()for v4 and only falls back tozod-to-json-schemafor v3. GovernedBuiltInAgent.clone()not callingsuper.clone()dropsthreadId/messages/state, but both consumers (runtimehandle-run, channelsisolateAgentInstance+ conversation store) assign those after cloning.- The TTL interplay (
ASSISTANCE_TTL_MSvsHELP_REQUEST_TTL_MS, both 10 min) resolves in the intended order — the poll deadline fires just before the control plane expires the request, so the friendly "Nobody took control" outcome is actually reachable rather than being shadowed by a hard expiry.
Minor
waitForAssistance in server/src/slack/assistance.ts is exported and carries ~90 lines of tests, but waitForExactAssistance replaced it and nothing in production calls it. pinnedFirst in app-sidebar.tsx is likewise no longer used by the sidebar.
On the umbrella-package question in your description: yes, please narrow @copilotkit/channels to the Slack sub-packages. Carrying the Discord, Telegram, Teams and WhatsApp adapters to use none of them is four dependency surfaces for nothing.
4fcac1d to
5d4b540
Compare
|
Thank you — this was a careful review, and the "checked and cleared" section saved the next reader real work. Everything is answered in 5d4b540, the fourth commit, and each inline thread has a reply with specifics. The branch is also rebased onto current The changelogYou read it right: rebase fallout, not a decision. Both halves are fixed. The two One structural note, because it caused four rebases in a row: almost every merge to The two invariants you chased downBoth were worth writing down, and one of them turned out to be worth removing rather than documenting. Per-turn threadId. Named at the binding site ( The double
The other two you cleared — MinorBoth removed, and their coverage moved rather than dropped.
The first two are in While there: The umbrella packageDone. One trap for anyone doing this again: it typechecked locally and failed CI. VerificationFull suite against a live PostgreSQL: 2900 tests across 233 files, 3 failures, all in files this change does not touch — |
5d4b540 to
67e1f94
Compare
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
Second pass, on 67e1f94. Everything from the first round is addressed, and the rebase onto current main resolves the staleness I raised.
The protect change deserves calling out: I'd flagged the double-establish as "works by accident" and expected a comment back. Making protect idempotent instead fixed a latent bug I'd written off as unreachable — on the detached managed-delivery path, resolve() wrote agentId to the original while computer tools read a copy, and runAgentLoop's ordering wasn't saving that path the way it saves the primary one. The load-bearing detail is that runWithPendingExecution queues currentSlackExecution() rather than the raw object, so executionForRun also hands back something already protected. Worth keeping that in mind if the queue is ever refactored.
Three must-fix items inline. All three are copy or parse, none are structural. Two of them (the unlink advice and the conflict fallback) are new surfaces created by fixing the 409 message, which is the normal cost of acting on that note rather than a criticism of how it was done.
Non-blocking — worth doing, not required for approval
None of the following should hold up a merge. Flagging them so they're recorded rather than rediscovered.
Worth fixing in this PR if you're touching it anyway:
server/src/channels/routes.ts:140— theROSTER_ORDERinvariant comment still points atpinnedFirstinapp-sidebar.tsx, deleted in this commit. The sibling reference inuse-channel-events.tswas updated; this one was missed. Seconds to fix, no runtime effect.app/tests/sidebar-roster.test.ts:100— the title-invariance test puts one row in each pin group with identical timestamps, so the native rows are never compared against each other. A sort that became summary-sensitive would still pass it. It's a false-confidence test rather than a broken one, but it's asserting less than its name claims.
Follow-up, if you want to track it:
- Deleting the
waitForAssistancesuite (which I asked for — it was genuinely dead) took with it the only coverage ofwaitForExactAssistance's hung-poll branch andsettleOperation's late-rejection consumption. The two replacement tests cover expiry-by-clock and abort-mid-wait, but not a status poll that never settles. That branch still ships, so this is a coverage regression on live code rather than dead code — my note should have said "check what these tests uniquely cover before deleting them", and it didn't. app/src/components/channels/external-thread-chat.tsx:19— therestoring/unreadable/currenttriad reimplementsuseQuery'sisPending/isError/cancellation, and aqueryOptionsfactory would supply the retry the copy currently substitutes "reload" for. This is also what the repo's own data-access convention calls for — every read aqueryOptionsfactory, consumed throughuseQuery. Correct as written now, so this is convention debt, and converting it changes how the screen loads data, which doesn't belong in a fix-up round.
One correction to my first review, since it shaped code you wrote: I asserted the transcript read is all-or-nothing and that unreadable should therefore be a boolean. I inferred that from the component instead of reading the function, and the inline comment on queries.ts explains why the premise was wrong. The boolean is still the right shape once that read throws — but the reasoning in the new comment is mine and is inaccurate as written.
Four callers now build a Bot for a person — a chat request, a routine's headless turn, a hop delivered to another Bot, and the boundary's own lookup — and each passed the same eleven collaborators positionally. One of them getting an argument wrong is a Bot that runs and quietly holds different tools or a different role from the one the person is talking to. ActorAgentResolver binds them once. Choosing a coworker moves out of the HTTP route for the same reason: it was the routing model call, the visibility rule, and the channel.routed row all written inside a Hono handler, so nothing that is not an HTTP request could route. CoworkerRoutingService owns the decision, and the route turns its outcome into status codes. That move makes an explicit name cheap enough to honour: a message that names exactly one coworker on the asking person's roster no longer pays a model call to be told what the person already said. Two matches are refused with both names rather than guessed at.
A person mentions @openBot in a Slack thread and names or describes the coworker they want. The thread is pinned to that coworker and replies continue with it, without another mention. Channels SDK owns Slack ingress, delivery, streaming and files. This deployment stays the authority for everything that decides what may happen: every turn re-resolves the Slack speaker to an OpenBot user and reloads THAT person's roster, grants, policy and audit identity. A second person in the same thread who cannot see the pinned coworker is refused rather than run as the person who started it. The coworker is built by the same resolver a browser turn uses, so a Slack turn holds the same tools, the same standing role, the same signed run assertion and the same stall guard. Its computer runs through the same gateway, which means the same boundary decides, and the same audit row is written. Secrets, sign-in control and 2FA are never asked for in Slack. The thread gets an expiring link to this deployment's own screen, and the bounded assistance wait resumes when control is released there. An unlinked Slack user is told so and handed a signed, expiring link; the agent does not run. An exact match between a verified Slack email and one active OpenBot account may create the first link. Nothing already linked is ever silently reassigned. State lives in Postgres, not in the process: the thread binding, the transcript, the identity link and the approval decisions are all tables, so a reply delivered to a second replica finds the same conversation. The bindings table is append-only by trigger.
The Slack side of a conversation was only in Slack: a person could not read what their coworker had done, and the account link and the secure prompt a Slack turn sends somebody to had nowhere to land. Three surfaces, all behind the existing session guard. Confirming a Slack account is theirs happens on a page that reads the signed link token and binds only to the OpenBot user completing the flow, with a sign-in return that comes back to the same confirmation rather than the roster. Taking the wheel or answering a secure prompt happens on the coworker's own screen, reached from the expiring link in the thread. And a Slack thread appears in the conversation sidebar, labelled, next to the channels it already lists, opening a read-only transcript of the turns as they were stored. The computer tools a Slack turn calls are declared once, in shared, so the browser and the channel offer the same contract rather than two drifting copies of it.
Four things from review, and a dependency narrowing that was asked for. The 409 from linking a Slack account said one sentence for two opposite conflicts. The store already knew which key the insert lost to: the Slack identity belonging to another OpenBot account, or the caller's own account already linked to a different Slack user in the same workspace. It threw the same string for both, so somebody re-linking under a new Slack id was told their identity belonged to another account -- a false claim about their own account, with no action attached. The conflict now travels as a code, and the confirmation page says the true one. GET and POST on the link route did not send `Cache-Control: no-store`, which every sibling route in the file does. The request URL carries the token and the response is the identity claim decoded from it, so an intermediary keying on that URL would hold a decoded claim beside the credential that produced it. The read-only Slack transcript had no rejection handler: a failed `/messages` left the view on its restoring skeleton for as long as somebody left it open, and rejected with nobody listening. The `unreadable` counter it should have fed was unreachable -- the read is all-or-nothing -- so it is a fact about the read now, and says the conversation could not be read. A Slack turn established its private execution context twice, and protecting copied every time, so a turn had two executions: the run wrote `agentId` to one and a computer tool reading the other would have found none and refused. It only worked because someone else's agent loop happens to invoke tool handlers after the run returns. Protecting an already-protected execution now returns it unchanged, which holds the invariant here rather than in a dependency, and there is a test for it. The stable-threadId property the append-only binding rests on is named at the binding site, because it is a property of managed delivery rather than of Channels. `@copilotkit/channels` was the umbrella package, so the Discord, Telegram, Teams and WhatsApp adapters came with it to be used by nothing. Narrowed to `channels-core` and `channels-ui`, with `channels-slack` a devDependency for the one test that asserts rendered Block Kit. `waitForAssistance` was replaced by `waitForExactAssistance` and called by nothing while keeping ninety lines of tests, and `pinnedFirst` was superseded by `conversationRoster`. Both removed, and what their tests uniquely covered -- the bounded wait expiring after the link is posted, a turn cancelled mid-wait, and a title never moving a row -- is now asserted on the paths that ship.
…e link it is Second review round. Three must-fix items, all in surfaces the previous commit created or exposed. `readExternalThreadMessages` coerced any unexpected body to `[]`, so a 200 of the wrong shape reached the screen as a transcript that had finished loading with nothing in it, and the reader concluded their coworker never answered. Nothing rejected, so the failure notice added in the last commit could not fire. It validates and throws now, the way every other reader in that module already did -- the notice has something to catch, and "empty" and "unreadable" stop being the same sentence about a record whose whole claim is that it is the canonical one. The 409 pair had no safe default. Both members assert who owns what, and the case with no `conflict` code is the one case where the page does not know which -- a new app against a server that does not send it yet sends every 409 down that path. Falling back to `provider_identity_linked` re-emitted the exact false claim about somebody else's account that the code was added to remove. There is a third, claimless variant now, so the invariant is that this page never guesses about account ownership rather than that it guesses conservatively. And the message told people to unlink something. There is no unlink route, no unlink screen, and no delete against `external_user_links` anywhere in the server: a link is made once and never reassigned. So that sentence named an action the deployment does not have, which is the same dead end as naming something untrue. Both conflicts now name an administrator, who is who resolves it. Not blocking, taken anyway: - The `ROSTER_ORDER` invariant comment in `server/src/channels/routes.ts` still pointed at `pinnedFirst`, deleted last commit. Its sibling in `use-channel-events.ts` was updated and this one was missed. - The title-invariance test put one row in each pin group with equal timestamps, so the partition ordered them and the comparator was never asked. A sort that started reading the summary passed it. It now has several rows per group with titles running opposite to activity, and fails when the sort is made summary-sensitive. - Deleting the `waitForAssistance` suite took the only coverage of a status poll that never settles with it, which ships. A test drives it through `computer_request_help`: the wait ends at the deadline, the request is cleared, and the abandoned read's later rejection is consumed rather than surfacing unhandled. The comment on the failed-read handler explained itself with a claim about the endpoint that was not true of the code as written. It names the validation that makes it true instead.
67e1f94 to
44be4cc
Compare
|
Second round is in The three blockersNothing to argue with in any of them. The two you attributed to the cost of acting on your 409 note were exactly that, and both were real: the fallback re-emitted the claim the change existed to remove, and the new sentence named an action the deployment does not have. Details are on the threads. The transcript coercion is the one I am most glad you blocked on. Non-blocking, taken
Follow-upsThe coverage regression is fixed, not tracked. You were generous about whose note it was, but deleting tests is my edit and the branch that ships is The On your correctionAppreciated, and it mattered: the comment I wrote asserted the endpoint's behaviour rather than the code's, and the code was doing the opposite three lines away in another file. The boolean is still right, and the comment now names the validation that makes it true instead of claiming it as a property of VerificationFull suite against a live PostgreSQL: 2943 pass, 20 skip, 3 fail, 2966 tests across 242 files. The three are in files this change does not touch — One note for whoever merges: the Slack migration is |
2 participants
The problem
A coworker only exists where OpenBot is open. The work it is for happens somewhere else: the thread
where somebody asks whether the filing is clean, the channel where a question about a customer lands
at eleven at night. Getting a coworker to answer there means a person reading the question, opening
OpenBot, retyping it, and pasting the answer back — which is a person doing the routing, badly, and
the reason most of these questions never reach a coworker at all.
The obvious way to close that is a bot that posts into Slack on the deployment's behalf. It is also
the wrong one, and the reason is the whole of this change. A shared bot answers as itself. It cannot
say which person asked, so it cannot read that person's roster, apply that person's grants, refuse a
private coworker they cannot see, or put their name on the audit row. Every safety property this
deployment has is a property of knowing who is asking, and a bridge that forgets loses all of them
at once while still looking like it works.
The approach
Channels SDK owns Slack; this deployment owns everything that decides. One
createChanneldeclaration named
openbotis handed to the sameCopilotRuntimethe browser talks to. Ingress,subscriptions, delivery, streaming, files, deduplication and reconnects are the SDK's. Which coworker
runs, whether this person may run it, which tools it holds and what gets written down are not.
Every turn re-resolves the speaker.
SlackIdentityLinkermaps the Slack workspace and user to anOpenBot user through
external_user_links, and the run is built for that actor by the sameActorAgentResolvera browser request uses. The conversation is shared by the thread; authorizationis not. A second person in the thread who cannot see the pinned coworker is refused with a plain
sentence — not run as the person who started it, not silently rerouted, and not told what the
coworker is configured to do.
The thread is pinned to one coworker, once.
external_thread_bindingsis keyed by the canonicalChannels thread id and is append-only, enforced by a trigger rather than by the code that writes it,
because a binding that can be edited is a conversation that can be aimed somewhere else after the
fact. Wanting a different coworker means a new top-level mention, which is a new thread.
An unlinked person is told so, and the agent does not run. They get a signed, expiring link to a
confirmation page behind their own OpenBot session, which binds only to the user completing the flow.
An exact match between a verified Slack email and one active OpenBot account may create the first
link. Ambiguous or conflicting matches require the explicit flow, and an existing link is never
reassigned by a later email match.
Secrets are never asked for in Slack. When a coworker needs a sign-in, a code or a card number,
the thread gets a sentence and an expiring link to that coworker's own screen in OpenBot. The bounded
assistance wait continues on the server and resumes when control is released there, or ends cleanly
when it is cancelled or expires.
The computer is the same computer. Slack turns call the tools declared in
shared/computer-tool-contracts.ts— the same contract the browser registers — and they executethrough
ComputerGateway. The same policy decides, the same refusal comes back, the same audit rowis written. There is no second, quieter path to an acting call.
Where it runs
external_user_links,external_thread_bindings,external_thread_messages,approval_decisions— in migration0029_slack_channels.sql.by canonical thread id, and answers into the same Slack thread.
(provider, tenant, conversation, thread)for bindings and on(thread, message_id)for transcript rows, so a redelivered Slack event cannot bind twice or storea message twice. Same-thread turns are configured serial.
SlackIngressRegistryholds identity facts for one delivery, keyed bySlack event id, one-use, 30-second TTL, and returns nothing unless exactly one entry matches. Both
halves — the SDK's
identifyUsercallback and the agent factory — run in the same process on thesame delivery, so this is not cross-request state. It fails closed: no match means no run.
nothing new is exposed through the ingress.
startManagedChannelHoststarts HTTP first so setupand health stay reachable while attachment settles, and
/api/capabilitiesprojects three fieldsof channel status — never the snapshot, which carries the provider's own token.
What is not covered
Slack app and a credential lifecycle each.
intentional Slack representation; everything else degrades to text.
every person links explicitly.
its turns. Managed delivery gives that; the self-hosted
channels-slackconversation store doesnot, so moving off managed delivery means keying the binding by the conversation key explicitly.
Named at the binding site.
Verification
Rebased onto
mainatd12300b. Full suite against a live PostgreSQL: 2943 pass, 20 skip, 3fail, 2966 tests across 242 files. All three failures are in files this change does not touch:
db-client-address.test.tsdials a hard-coded127.0.0.1:5432, and twosupervisorDockerintegration tests pull real images and time out at sixty seconds here.
mainin the sameenvironment fails the first as well, and five more besides. Each commit was also run on its own
database when the branch was first rebased: 2502 tests on the first, 2806 on the second, 2840 on
the third.
bun run format:check,bun run lint,bun run typecheck, theagent-computerandsupervisortypechecks,
bun run buildandbun install --frozen-lockfileare clean.drizzle-kit checkreports no collision, and the unwritten-migration probe finds nothing to generate.
The recording is the deployment we run this on, on 28 August: a mention in a Slack channel, the
coworker browsing a page and answering in-thread with what it read, the deep link back, and then the
same conversation in the OpenBot sidebar with its stored transcript. It is the fork's build of this
change; the branch has since been rebased onto current
main, which is what the numbers above are.Reviewed twice and answered in the fourth and fifth commits. The second round: the transcript read
validates instead of coercing a bad body to an empty conversation, the 409 pair gained a claimless
third variant so the page never guesses about account ownership, and its copy names an
administrator rather than an unlink flow the deployment does not have.
The first round: the two 409 conflicts now say which one happened,
GETandPOSTon the link route sendno-store, the read-only transcript handles a failed readinstead of sitting on its skeleton, and a turn's private execution is one object however many times
the context is established — held here rather than resting on when somebody else's agent loop
invokes tool handlers.
@copilotkit/channelsis narrowed tochannels-core+channels-ui, withchannels-slacka devDependency for the one test that asserts rendered Block Kit.waitForAssistanceand
pinnedFirstwere superseded and are gone; what their tests uniquely covered is asserted on thepaths that ship.
New test files, one line each:
slack-channel.integration.test.tsx— mention, reply, binding, and the refusals: an unlinkedspeaker, a second speaker who cannot see the coworker, a coworker deleted after binding.
slack-identity-linker.test.ts— email matching, ambiguity, and that a link is never reassigned.slack-computer-tools.test.ts— every computer tool through the gateway, and its refusal.slack-assistance.test.ts— the assistance link, the bounded wait, resume, cancel and expiry.slack-channel-agent.test.ts— binding, delegation, and that private context never reaches a prompt.slack-ingress-registry.test.ts— one-use, TTL, and refusing an ambiguous match.slack-approval-authorizer.test.ts,slack-approval-store.integration.test.ts— who may decide anapproval, and that a decision is recorded once.
slack-tenant-context.test.ts,slack-turn-phase.test.ts,slack-execution-context.test.ts—canonical tenant, turn phase, and the per-run context boundary.
slack-lifecycle.test.ts— HTTP up before attachment, and still up when attachment fails.external-link-store.integration.test.ts,external-link-token.test.ts,external-link-routes.test.ts— the link table, the signed token, and the confirmation routes.external-thread-store.integration.test.ts— bindings, transcript ordering, and the append-onlytrigger.
app/tests/*— the link page, the assist route, the sign-in return, the sidebar rows and theread-only thread view.
Merge notes
This is based on #296, so that change's commit is in this branch too and its diff shows here as
well; review from the second commit. #296 lands first.
createAppandmountCopilotRuntimebothtake new trailing arguments, and
mountCopilotRuntimetakes the resolver in place of its elevencollaborators, which is the shared contract most likely to collide with another branch in flight.
Four places where this change and
mainmet in the same lines, and how:resolveRuntimeAgentsgrewloadInstructionsonmain, soActorAgentResolvercarriesloadInstructionsForActoras a dependency rather thanmountCopilotRuntimecarrying it as anargument. One binding, so a routine's headless turn and a Slack reply get the same standing
instructions a browser turn gets.
onRunBusystays an argument: it is told about runs, not aboutcoworkers.
mainaddedBuiltInAgentWithSaneHistoryto drop a dangling tool call beforeBuiltInAgent.runconverts the messages.
GovernedBuiltInAgentnow extends it rather than sitting beside it, so abuilt-in Bot cannot be governed and unsanitised at the same time, and the remote composition
applies the same guard.
mainaddedmatchingChannels, which searches a channel's name, its summary and its lastmessage. The sidebar now filters one roster of channels and Slack threads, so that search moved
into
matchingRoster— summary included — andchannel-search.test.tsasserts it there.0028, regenerated againstmain's0027snapshot, with the append-onlytrigger on
external_thread_bindingshand-appended as before.