Let one Bot hand work to another, and reach a person when no Bot will do - #266
Let one Bot hand work to another, and reach a person when no Bot will do#266davidmckayv wants to merge 23 commits into
Conversation
Issue #192. This is what decides a hop, not what delivers one. Resolving who is being addressed, refusing when it should, writing the row that says what happened, and putting durable work on the queue #216 shipped. The runner that claims that work and runs the other Bot comes next, and the split is the point: deciding happens inside somebody's run and has to be fast and fail closed, while delivering is a whole agent turn that has to survive the pod it started on. THE ENVELOPE IS TYPED, WHICH IS THE ONE DEPARTURE FROM THE ISSUE. It proposed `message_bot(target, message)`. Free text is the commonest way a multi-agent system goes quietly wrong: the receiving Bot infers the intent, re-derives the constraints and guesses what shape of answer was wanted, and when it guesses wrong it does not fail, it returns something else confidently. Naming the task, its constraints and what a good answer looks like costs the asking model a little effort and removes most of that. THE GRANT IS AN ORDINARY GRANT. `plugin_grants` gains a `bot` kind rather than getting a table of its own, because an administrator already understands "this Bot may use that" and a fork's policy layer already applies to grants. That widening also caught a ternary labelling everything that was not an MCP tool a skill, which would have filed a Bot grant in the trail as one. DEPTH AND THE CONVERSATION TRAVEL IN THE SIGNED ASSERTION. A chain is three runs on up to three pods, so a counter in a variable stops applying the moment the second hop lands elsewhere, which is also when a loop starts costing money. And where an answer lands cannot come from the model, or one Bot could drop a turn into a conversation it was never part of. BOTH CAPS FAIL CLOSED AND ARE COUNTED FROM ROWS. The fan-out cap counts the hops this run has already offered, because counting in a process counts one pod and a run whose hops land on several is exactly what it exists to bound. They are configuration rather than constants, and mean by default: one level deep and three per run. A Bot is refused, in the same words, whether the Bot it named does not exist or is one its person cannot see, so this cannot be used to enumerate the roster. Every refusal is a sentence the Bot can say rather than an exception, because a throw ends the run with nothing said and reads to the person as the Bot ignoring them. And every outcome leaves an audit row: the refused one matters more, since a hop that happened is visible in the transcript and one that was refused is invisible everywhere else.
The decision half is wired in. A Bot that has been granted another is offered `message_bot`; one that has not is offered nothing, which is the correct default and better than a tool whose every call is refused. MADE PER RUN, NOT PER REQUEST, and that is the whole reason this touches the runtime. The tool has to know how deep the chain already is and which conversation an answer belongs in, and both are facts about the run rather than the request: a request is earlier, with a Bot and a person and no message. The per-run wrapper that already existed for narrowing tools is exactly that seam, so it does both now and is named for what it does rather than for one of its reasons. Depth comes from the assertion this deployment signed, never from the Bot id the runtime happens to be building. On a hop those agree; taking it from the signed value rather than the build is what stops a stale assertion aiming the next hop at another Bot's grants. The grant is read on every run and every hop rather than held, so one made a minute ago counts and one revoked a minute ago stops counting. A read that fails is treated as no grant: failing closed costs a hop, failing open would let a Bot address one nobody gave it because the database blinked. Driven against Postgres rather than fakes, because three of the four properties are the database's own: whether a second offer of the same hop collides, whether the fan-out count sees rows another replica wrote, and whether a grant read now reflects one written a moment ago. A fake answers all three the way its author expected, which is the wrong witness for the questions worth asking. Booted the server too: every wiring bug in this shape lives in module construction, where no unit test goes.
… of it The other half of #192. Deciding a hop happens inside somebody's run and has to be quick and fail closed; delivering one is a whole agent turn against a model. They are separated by the queue rather than by a function call, which is what lets any replica take any hop: on a cluster the Bot being addressed is very unlikely to be on the pod that addressed it. THE LEASE IS RENEWED FOR AS LONG AS THE RUN TAKES. A run is minutes and a lease that lapses mid-answer hands the same hop to a second replica, which runs the same Bot again and bills for it twice. That is the failure this queue exists to prevent and the one it is easiest to reintroduce by forgetting a heartbeat. THROUGH THE PLATFORM'S OWN RUNNER, not by calling the agent and writing the answer somewhere. The runner is what persists a turn to a thread, so a delivered answer is the same kind of object as one a person's run produced: in the transcript, in the history the next run reads, and surviving whichever pod made it. Calling `agent.run` directly would produce an answer nothing recorded, which is the failure nobody can debug. The addressed Bot reads the conversation before the ask, because it is joining something already in progress: one handed only the task answers a question whose other half was settled three messages ago. Who is asking is stamped from the row this deployment wrote, never from anything a model produced, or a Bot could claim to be another. And the parts stay parts: the asking model was made to name the task, its constraints and what a good answer looks like precisely so this one need not infer them, and flattening them into prose at the last step would throw that away. A run that ended in an error is not a delivery. Treating it as one finishes the work and leaves the person waiting for an answer that will never come. A run that completed IS one, whatever the Bot said: "I could not find that" is an answer, and retrying spends another model call on the same non-answer. A second attempt says so in the trail before it runs anything, because it may already have run that Bot and posted an answer before its owner died, and somebody looking at two similar answers should be able to tell a duplicate from a mystery.
Slice two of #192 is connected. A Bot calls the tool, a row lands on the queue, and whichever replica gets there first runs the addressed Bot and lets its answer into the conversation. THE ADDRESSED BOT IS BUILT BY THE RUNTIME MOUNT, not by wiring assembled beside it. `agentFor` and `history` are handed out from where the runtime already knows how to make a Bot for a person, because "built exactly the way a person's run builds it" is worth guaranteeing structurally: a Bot assembled by parallel wiring drifts the first time one of those arguments changes, and the drift is invisible. It runs, and quietly holds different tools or a different role from the one the person is talking to. One Intelligence client serves both, so a hop reads the history a person's run would read rather than a second view of it that could disagree. A LOOP RATHER THAN A SCHEDULE. A hop is somebody waiting for an answer, not housekeeping, so the culler's minute-granularity CronJob would be an unexplainable pause in a conversation. Every replica sweeps and the queue decides who gets what, so a replica added is delivery capacity rather than contention. It does not run at all where the depth cap is zero: a deployment that has switched the capability off has no hop to find, and polling for work that cannot exist is a query a second for nothing. The end-to-end test is the one worth having. The two halves never speak: deciding happens in one run and delivering in another process, and the only thing between them is a row. Unit tests on either side pass while the row they agree on is written by one and unreadable by the other. Only the model call is faked, because running a real one is slow, expensive and non-deterministic for a question the files either side already answer. History is passed through untouched rather than converted. The platform holds a thread's messages in its own shape and takes them back in the same one, so a stricter type in the middle would mean inventing a conversion between two things that already agree, and a conversion is a place to lose a message.
The caps are chart values and documented variables rather than folklore, and always rendered including the zeroes, so a deployment that has switched the capability off says so rather than relying on the image's default staying what it is today. AND THE HOP IS DRAWN IN THE TRANSCRIPT. Without this the call still appears, as a generic tool call named `message_bot` with its arguments as JSON: technically visible and practically not. What the issue asks for is that a person can see their Bot bringing in another one and read what it asked for, because a conversation that quietly fans out to four Bots and bills for all of them is the thing to avoid. The renderer registers no tool. `message_bot` runs on the server, where the grant, the caps and the audit row are, and a frontend registration would be a second place that decides. It draws a refusal differently from a handoff, since one is a Bot bringing in help and the other is a boundary holding, and drawing them alike would make a working cap look like a working handoff. The parts stay parts there too: what was asked, what bounded it, and what was wanted back.
Two found by driving this on a real cluster rather than by reading it. THE CULLER HAD NO CEILING ON ONE SWEEP. With `concurrencyPolicy: Forbid` above it, a sweep that hangs holds the lock for ever and Kubernetes never starts the next one, so culling simply ceases: no error, no restart, no alert, and the first sign is a bill for a fleet of browsers nobody has used in a fortnight. That is the failure the feature exists to prevent, arriving through the mechanism meant to prevent overlapping runs. Guido flagged it; it was on the follow-up list and should not have been. AND THE TRANSCRIPT DREW EVERY SUCCESSFUL HANDOFF AS BLOCKED. A server-side tool's result reaches the surface as a tool message whose content is JSON-encoded, so the renderer saw `"Handed to Knowledge…"` with the quotes and matched none of them. Worse than not drawing it: a working boundary and a working handoff looked identical, and the reassuring one was the wrong one. The prefix the two sides agree on is now named in one place, which is not a contract to be proud of but does stop them drifting apart in silence. The runner is also built from the client's own address and token rather than from configuration, the way the runtime builds it. That was a real bug and not the one it looked like: it has not fixed the delivery failure, which is recorded on the PR.
A delivery presented the lock's join token as the runner socket's credential. That token is what a browser presents to watch a conversation; the runner's socket has its own. Overridden with it the socket was refused, and because the runner treats a socket that will not connect as something to retry rather than as a failed run, nothing was emitted and nothing ever completed. Every hop hung in total silence. Taking the lock is what makes the run legitimate; the gateway checks the run id on every event, and nothing else needs presenting. Three more faults sat behind it, each of which would have been the next one: A hop was unbounded. Nobody watches a hop, so a run that stalls holds the conversation's lock and its place on the queue for as long as the process lives. It now has a deadline, and says how far the Bot got before it passed. The history handed across carried the asking Bot's tool traffic. A thread's stored history is what a person is shown, not a prompt: the assistant message that made a tool call is not kept, so its result is stored alone with a toolCallId matching nothing. The asking Bot's last act is always the call that handed the work on, so every hop carried one. What crosses now is what was said. The lock was released on the conversation that asked rather than the one it was taken on, and each attempt made a fresh conversation to answer in, so a retried hop left a row of empty channels behind it. The rest is what a person sees. The addressed Bot answers in the conversation they already have with it, which now moves up the roster when it does, because the browser is what writes that and no browser is watching. Its transcript keeps one line saying who asked and what for, rather than the whole prompt. And a hop that fails for good sends the asking Bot back to say so, so a question handed on and never answered stops being silence. Asking a person is now a tool of its own, sitting beside the one for handing work sideways and competing with it. A model with no named way to stop takes the one it has: it guesses, or it asks a Bot that cannot settle it either. Who "a person" is, is a seam; this template answers the person in the conversation.
`runAgent` takes runId, tools, context and forwardedProps. AG-UI keeps the messages and the thread on the agent itself and builds the run's input from them, so a messages array passed as a run parameter is ignored in silence. Nothing failed. The Bot ran, read an empty conversation, and answered "how can I help?" to a question printed directly above its reply. Told to answer with one particular word, it asked what it could help with instead, which is how this was finally pinned down.
The cap counted the run's hops and then wrote one, which holds only while nothing else is writing. The case it exists for is the opposite: a model asked to do several things emits several tool calls in one turn and they run at once, each reading a count taken before any of the others had committed. Five hops passed a cap of three, every time, on a single pod, with no unusual timing. The count and the write are one step now, in the queue where the rows are, under an advisory lock on the run's own prefix. A key already on the queue still counts as offered rather than as refused: a retried offer of queued work is not a new hop. The integration test drives five concurrent offers against a real PostgreSQL, because a stub that awaits one call at a time cannot fail the way this did.
…apart `ask_person` appeared in the transcript as a raw tool call with its arguments as JSON. A Bot that decided it could not settle something and stopped to ask has done the right thing; drawn that way it reads as a malfunction. Both lines read their outcome out of the tool's own prose, which is what a server-side tool leaves available, and both now decode through `asText` rather than stripping quotes by hand. Matched against the raw value the prefix never matches, which is how every accepted handoff came to be drawn as Blocked.
…cript A Bot going back to its own conversation to report a failed hop had the instruction that made it speak persisted alongside its answer, in a bubble that looks like something the person typed and then had read back to them. Its own sentence is the whole message.
# Conflicts: # server/src/work/queue.ts
|
CI here is red on |
|
Known limitation worth naming before review: a hop that fails and is retried leaves one "asked …" line per attempt in the addressed Bot's conversation, with no answer under any of them. There is no clean fix on this side. Every attempt pushes Persisting the ask only on the first attempt was considered and rejected: if that attempt loses the lock race the line is never written at all, and a later successful answer then appears with nothing saying why the Bot spoke. A duplicate on a failure path is the better of the two. Cosmetic, and only on repeated failure, which the asking conversation now explains in words. A proper fix needs the platform to discard a run that produced no assistant message, or to allow a message to be withdrawn. |
…voke one Three faults found by review and reproduced against a real PostgreSQL before being touched. The suite was green through all of them, which is the point: two are about time passing, and every stub of this queue answers whatever it is told. A claim leases the whole batch from one moment and the batch is delivered one at a time, so a heartbeat covering only the hop in flight left the rest on a lease quietly running out. A delivery is minutes and a lease is one: the tail of every batch expired, was claimed by another replica, and was delivered by both. Two model calls, two answers in somebody's conversation, and both replicas reporting success, because `finish` returns a boolean saying the lease had gone and nothing read it. Every claimed hop is renewed now, and each is renewed once more immediately before its delivery starts. That renewal is the question and the answer at once: consulting the heartbeat's own bookkeeping would only catch a refusal it had already seen, and a process paused long enough to lose the lease never asked. `finish` answering false no longer reads as success. `ChannelStore.direct` looked and then made, which is not find-or-create. Two hops delivered at the same moment each found nothing and each made a conversation, so one person had two channels with that Bot and their answers split between them. A Bot asked for several things in one turn produces exactly that. Find and make now share one transaction, serialised on the person and the Bot. The grant table learned a `bot` kind and the API did not. Revoke rejected it outright, so the capability could only be enabled by writing a row by hand and could not be turned off at all, while the design rests on a revoked grant applying to the very next hop. Both endpoints take it, one Bot reaching another is an administrator's decision like an MCP tool rather than a skill somebody attaches to their own Bot, and `kind` is checked against the three that exist rather than trusted from a JSON body. A fan-out cap of zero switched the capability off everywhere except the model's tool list, so every call was refused and the Bot told the person it had tried. Both zeros close the same door now.
|
All four confirmed, reproduced against a real PostgreSQL before anything was touched, and fixed in 1395278. 1. The tail of a batch was delivered twice. A claim leases the whole batch from one moment and the batch is delivered one at a time, so a heartbeat covering only the hop in flight left the rest on a lease quietly running out. Reproduced with two replicas: bot-1 was safe, bot-2 and bot-3 expired, replica B took and delivered them, replica A delivered them again, and A reported all three as delivered because Every claimed hop is renewed now, and each is renewed once more immediately before its own delivery starts. That second renewal is deliberately the check as well: consulting the heartbeat's bookkeeping would only catch a refusal it had already seen, and a process paused long enough to lose its lease never asked. My first attempt at this got it wrong and the second regression test caught it. 2. Duplicate conversations. Confirmed: two concurrent 3. Administrators could not configure it. Worse than described: 4. Each of the four has a regression test that fails without its fix — I checked by reverting each one in turn. The two races needed integration tests against a real database and a heartbeat interval that could be injected; a stub answers whatever it is told, which is exactly why 1,781 green tests said nothing. |
# Conflicts: # server/src/app.ts # server/src/attention/routes.ts # server/src/audit.ts # server/src/index.ts # server/tests/attention-view.test.ts
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
This PR got a deep review: 8 independent finder passes over the full branch, ~30 candidate findings deduplicated to 16, each then adversarially verified against the checked-out branch (the two Helm findings were reproduced empirically with helm template).
Overall the feature is well-built — the queue's claim/renew/release semantics, the signed depth assertions, the cap enforcement ordering, and the big code moves in index.ts/channels/routes.ts all held up under adversarial verification, and the PR reuses existing infrastructure (signed values, audit events, the work-queue primitives) well.
Requesting changes for three small, contained fixes that bite real deployments:
- Helm
--reuse-valuesupgrade fails the entire render (_helpers.tpl:184) — plus its silent sibling, the emptyactiveDeadlineSecondsin the culler cronjob. - The unguarded 2-second sweep interval stacks concurrent deliveries without bound under a backlog (
index.ts). - Failure-notice rows consume the asking run's fan-out budget, producing wrong "already asked N Bots" refusals (
handoff.ts).
Everything else is inline as non-blocking (a couple of near-blockers worth doing while you're in there: the notice-key collision and forwarding maxAttempts to claim).
| relying on the image's default staying what it is today. | ||
| */}} | ||
| - name: BOT_HANDOFF_MAX_DEPTH | ||
| value: {{ .Values.config.handoff.maxDepth | quote }} |
There was a problem hiding this comment.
[blocking] .Values.config.handoff.maxDepth is dereferenced with no nil guard, and config.handoff is a brand-new values key. Helm 3's --reuse-values takes the old release's computed values instead of merging the new chart defaults, so upgrading from any pre-PR release with --reuse-values fails the entire render (the server deployment includes commonEnv), not just the handoff feature.
Reproduced: helm template . --set config.handoff=null → nil pointer evaluating interface {}.maxDepth at _helpers.tpl:184:19.
Suggested fix: guard both values, e.g. {{ .Values.config.handoff.maxDepth | default 1 | quote }} (the chart already uses this convention elsewhere, e.g. the sandbox namespace default), or {{ (.Values.config.handoff).maxDepth | default 1 | quote }} to survive the whole map being absent. Same for maxPerRun below.
There was a problem hiding this comment.
Fixed in 9735e2b, and confirmed the premise before touching it: this is helm/helm#9000, --reuse-values does not merge new chart defaults.
Guarded both with (.Values.config.handoff).maxDepth | default 1, so the whole map being absent is survivable rather than only a missing leaf.
Driven rather than reasoned about: I installed main's chart into a local kind cluster and ran a real helm upgrade --reuse-values to this branch. Unguarded it fails exactly as you said — UPGRADE FAILED ... nil pointer evaluating interface {}.maxDepth. Guarded, the same upgrade succeeds and renders "1" / "3".
| }; | ||
|
|
||
| // Unref'd so this never holds the process open on its own. A pod draining should drain. | ||
| setInterval(sweep, 2_000).unref(); |
There was a problem hiding this comment.
[blocking] This is a bare setInterval(sweep, 2_000) with no re-entrancy guard, and sweep claims via FOR UPDATE SKIP LOCKED, so overlapping sweeps claim disjoint batches. A single delivery can legitimately run up to 5 minutes (DEFAULT_DELIVERY_DEADLINE_MS), during which ~150 more sweeps fire — each claiming up to 5 more rows and starting its own heartbeat interval. Under a backlog, one replica's concurrent agent deliveries grow ~5 every 2s, bounded only by backlog size. The per-thread NX lock only serializes hops in the same conversation.
Suggested fix: self-schedule with setTimeout after each sweep completes (or a simple in-flight boolean), so a replica runs at most one sweep at a time. That also caps concurrency at the claim limit and stops idle deployments paying a claim transaction every 2s.
There was a problem hiding this comment.
Fixed in 9735e2b. Self-scheduling, as you suggested.
I pulled it out into server/src/work/loop.ts (repeatAfterEach) rather than inlining a boolean, because the property could not be tested where it was — the loop lives in the composition root. server/tests/work-loop.test.ts drives it with a fake clock and asserts nothing ever starts while something is in flight.
Writing that turned up a fault of its own worth knowing about: void work().finally(next) re-raises what it caught, so every failed sweep would have left an unhandled rejection, which on Bun ends the process. It is .then(next, next) now, with the reason written down.
| * several pods is exactly what this exists to bound: every hop this run has offered is a row | ||
| * under its own prefix, so the rows are the count. | ||
| */ | ||
| atMost: { keyPrefix: `${from.runId}:`, max: caps.maxPerRun }, |
There was a problem hiding this comment.
[blocking] The fan-out cap counts every row under keyPrefix: ${from.runId}:`` for this kind — but tell() in handoff-runner.ts writes failure notices under the same kind with key `${work.runId}:notice:${work.toBotId}`, which starts with this prefix. The count doesn't discriminate by status or payload, so a dead hop's notice consumes the asking run's budget.
Concrete: maxPerRun=3, a long run hands to A and B; the hop to A fails for good (~4+ minutes with 5 attempts × 60s delays) while the run is still going; the notice is the third row under the prefix, and the run's third legitimate message_bot call is refused with "This turn has already asked 3 Bots" after asking only 2.
Suggested fix: give notices their own kind, or exclude :notice: keys from the counted prefix.
There was a problem hiding this comment.
Fixed in 9735e2b. The notice key is now notice:${hopKey} — outside the run's prefix entirely, so it costs no fan-out budget.
Your concrete scenario is the one I tested: server/tests/agent-handoff-runner.test.ts asserts the key does not start with the run prefix.
| queue.offer({ | ||
| kind: HANDOFF_KIND, | ||
| // Distinct from the hop's own key, or `offer` would treat this as the same work and drop it. | ||
| key: `${work.runId}:notice:${work.toBotId}`, |
There was a problem hiding this comment.
The notice key ${work.runId}:notice:${work.toBotId} carries no hash of the failed hop, while hop keys hash [target, task, constraints, expecting]. One run can legally queue two different hops to the same bot under the cap of 3; if both fail for good, the second tell()'s offer hits onConflictDoNothing on the identical key and is silently dropped. And since queue.purge is never invoked for this kind in production wiring, the first notice row blocks the second forever, not just within a window. The person hears about only one of their two lost questions, with only the first failure's reason.
Suggested fix: include the failed hop's key (or its hash suffix) in the notice key.
There was a problem hiding this comment.
Fixed in the same change, and you were right that this is close to blocking — with nothing purging the kind, the dropped notice is lost for good rather than for a window.
The key carries the failed hop's own key now, so two lost questions to one Bot leave two notices. Tested in agent-handoff-runner.test.ts; both new tests fail against the old key.
Separately, purge is now wired for this kind (runner.reap(), hourly, one-day retention) — see the reply on the queue thread.
| return { | ||
| /** Deliver whatever this replica can claim. */ | ||
| async sweep(): Promise<HandoffRunReport> { | ||
| const claimed = await queue.claim({ |
There was a problem hiding this comment.
This queue.claim call omits maxAttempts, so the queue's SQL cutoff silently uses DEFAULT_MAX_ATTEMPTS=5 while the runner's own maxAttempts option gates the "failed for good" notice — the option's doc says "Must match what claim is told", but nothing tells claim. Latent today (the one production caller uses matching defaults), but: with maxAttempts: 7 the claim filter stops serving the row at attempt 5, item.attempts >= 7 is never true, and the failure notice is never sent — the silent stop the feature exists to prevent. With maxAttempts: 3 the notice fires while the queue keeps re-serving the hop until 5, so the bot can still answer after the person was told it failed for good.
Suggested fix: forward maxAttempts into the claim call and delete the must-match comment.
There was a problem hiding this comment.
Fixed in 9735e2b: maxAttempts is forwarded into claim, and the must-match comment is gone in favour of one that says it is told to both.
Worth recording that your reasoning about which direction breaks how was right on both counts — higher here and the notice never fires, which is the silent stop the feature exists to prevent.
| // Read now rather than at boot, so a grant made a minute ago counts and one revoked a | ||
| // minute ago stops counting. | ||
| hasSomebodyToAsk: | ||
| (await pluginStore.botsReachableFrom(botId).catch(() => [] as string[])) |
There was a problem hiding this comment.
[non-blocking] This botsReachableFrom query runs on every run of every built-in Bot, before any of handoffTool's short-circuits (maxDepth <= 0, maxPerRun <= 0, at-cap) that would discard the result — the maxDepth > 0 gate below only guards the delivery runner, not this closure. So a deployment that sets BOT_HANDOFF_MAX_DEPTH=0 to switch the feature off still pays one grants query per run. And because ask_person is always returned, the passing.length === 0 allocation-reuse fast path in copilot.ts is now dead for every built-in bot. When a hop is sent, mayAddress re-reads the same grants seconds later inside desk.send.
Suggested: check the caps (and the run's depth) before querying, and skip wiring handoffForActor entirely when maxDepth <= 0 — escalation would need its own (cheap) path, which also untangles it from riding the handoff parameter.
There was a problem hiding this comment.
Fixed in de0a3d1. The caps and the run's depth are checked before the query, so a deployment with BOT_HANDOFF_MAX_DEPTH=0 pays nothing per run and a run at the cap pays nothing either.
I did not take the second half — untangling escalation from the handoff parameter so handoffForActor can be skipped entirely. ask_person is deliberately offered whether or not a Bot has been granted anybody, so it needs a path regardless, and the closure is now cheap when the caps are off. Worth doing as its own change; noted rather than quietly dropped.
| botId: string; | ||
| }): Promise<AbstractAgent | null> => { | ||
| const actor: AgentActor = { id: input.actorId, role: "user" }; | ||
| const agents = await resolveRuntimeAgents( |
There was a problem hiding this comment.
[non-blocking] agentFor resolves the actor's entire roster — loadAgents, vendor listServers, and per-bot granted-tool queries for every registered agent — only to return agents[input.botId]. This runs once per hop delivery on the sweeping replica, and again on every retry (up to 5 per hop). With K bots that's K sets of grant queries and K agent constructions per delivered hop, K−1 of them discarded.
Suggested: accept a botId filter in resolveRuntimeAgents (or filter the registered list before building) so only the addressed Bot is built.
There was a problem hiding this comment.
Fixed in de0a3d1: resolveRuntimeAgents takes an optional onlyBotId, and agentFor passes it.
The roster is still LOADED in full deliberately — which Bots exist for this person is what decides whether the one addressed is theirs to see at all — but only the addressed Bot is constructed and only its grants are read. That removes K−1 agent builds and K−1 grant queries per delivery, and again per retry.
| return Boolean(released); | ||
| }, | ||
|
|
||
| async count({ kind, keyPrefix }) { |
There was a problem hiding this comment.
[non-blocking] count has no call sites anywhere in src or tests — the fan-out cap uses the transactional atMost inside offer instead — yet every WorkQueue implementer, including the test fake in agent-handoff.test.ts, must carry it. It also misdirects: readers infer callers enforce the cap by counting, when the real mechanism is atMost. Suggest deleting the method from the interface and both implementations.
There was a problem hiding this comment.
Deleted in de0a3d1, interface and implementation, plus the test fake. You were right that it misdirects — the fake in agent-handoff.test.ts now models atMost instead, which is what actually enforces the cap.
| agentId: string; | ||
| }) => { | ||
| try { | ||
| const held = await intelligenceClient.ɵacquireThreadLock(input); |
There was a problem hiding this comment.
[non-blocking] The thread lock and runner connection — the "whole of the ceremony" legitimizing a delivered run — ride five ɵ-prefixed private Intelligence-client APIs (ɵgetRunnerWsUrl, ɵgetRunnerAuthToken, ɵacquireThreadLock, ɵrenewThreadLock, ɵcleanupThreadLock) plus a THREAD_LOCK_TTL_SECONDS that duplicates the platform default by copy. A routine @copilotkit bump that renames one makes every acquire throw, which acquire() maps to null ("busy") — every hop retries to exhaustion and every handoff reports "it never answered", surfacing only as contention-looking warnings.
Worth at least: pin the package version this was verified against, isolate the ɵ surface behind one small adapter, and distinguish "threw" from "busy" in acquire so an upstream break is loud. Longer-term this wants a public server-run/lock API upstream.
There was a problem hiding this comment.
Half of this is already true and half is fixed in de0a3d1.
Already: @copilotkit/runtime is pinned exactly at 1.69.0 in server/package.json, no caret — so "pin the version this was verified against" is satisfied.
Fixed: acquire now returns null ONLY for a 409 and raises everything else, so a renamed ɵ API is loud. Your description of the failure was exact, and I hit it during development for a different reason — the catch collapsed "busy" and "failed" and cost hours. The runner writes the real reason onto agent.handoff_failed now.
Not done: isolating the ɵ surface behind a further adapter. threadLock and runnerConnection already are that seam, and a second wrapper would add a layer without adding a guarantee. The upstream ask for a public server-run/lock API is the real fix and is worth raising there.
| # `maxDepth: 0` switches the capability off entirely: no Bot is offered the tool and the delivery | ||
| # loop does not run. | ||
| handoff: | ||
| maxDepth: 1 |
There was a problem hiding this comment.
[non-blocking] These defaults now live in three places: here, the config.ts fallbacks, and the docs/configuration.md table — and since _helpers.tpl always renders the env vars, the code default is dead in Helm deployments while the docs describe the wrong source. The next PR that bumps one leaves the others behind, and operators debug fan-out refusals against a number their deployment doesn't use.
Suggested: omit the env vars when the values are unset so the code default rules, or add a test asserting values.yaml and the docs match handoffCaps()'s fallbacks.
There was a problem hiding this comment.
Fixed in de0a3d1, and it was worse than three places by the time the nil-guard landed: values.yaml, the code fallback, the docs table, and now the template's own | default.
I did not take the "omit the env vars when unset" option, because the existing comment argues for always setting them so a deployment that switched this off says so rather than inheriting whatever the image does next release. So it is your second suggestion: server/tests/handoff-caps-defaults.test.ts reads all four and asserts they agree. I checked it fails by changing values.yaml alone.
Three findings from review, each reproduced before it was touched.
`config.handoff` is a values key this chart did not have. `helm upgrade
--reuse-values` takes the previous release's computed values instead of merging
the new chart's defaults, so on every deployment that already exists the map is
absent and reaching through it is a nil dereference. It fails the whole render,
not the feature: the helper is included by the server deployment. Driven against
a kind cluster by installing main's chart and upgrading to this one, which fails
with `nil pointer evaluating interface {}.maxDepth` and succeeds once guarded.
The culler's `activeDeadlineSeconds` had the same shape with a quieter ending.
The key is newer than the culler around it, so an existing release still renders
the CronJob and emits an empty scalar. That is null, which Kubernetes reads as
unset, so the ceiling on a hung sweep silently stops existing on exactly the
deployments old enough to have one. CI now renders each new key absent and
refuses both a failed render and an empty value.
The sweep ran on a bare interval. Claims are taken with `skip locked`, so
overlapping sweeps do not contend for a row, they take different ones: during a
five-minute delivery an interval starts a hundred and fifty more sweeps, each
claiming another batch and starting its own agent runs, and the concurrency is
bounded by the backlog rather than by the limit asked for. It self-schedules now,
through `repeatAfterEach`, which exists so the property can be tested at all.
Writing it turned up a fault of its own: `finally` re-raises, so the obvious
`void work().finally(next)` leaves an unhandled rejection after every failed
sweep, which on Bun ends the process.
A failure notice was keyed inside the asking run's prefix, so the message saying
a hop was lost spent that run's fan-out budget and the next legitimate ask was
refused with "this turn has already asked 3 Bots" after asking two. It also
carried no hop identity, so a run that lost two questions to the same Bot filed
one notice and dropped the other on conflict, for good, since nothing purges this
kind. The key is now outside the prefix and carries the hop it is about.
`claim` was never told the `maxAttempts` the notice is gated on, so the two could
disagree: higher here and the row stops being served before the notice can fire,
which is the silent stop this feature exists to prevent.
…aste The transcript's copies of the two marker phrases were commented as shared with the server. They never were: three separate literals, no test touching both, and the drift they guard against is the bug their own comments recount. They are one file now, with a test that reads the server's copies, and one matcher for both renderers, because they disagreed about a result that is neither a string nor absent — one drew it as success and the other as a refusal, for the same case. A message is not always a string. AG-UI takes an array of parts and the platform types thread content as unknown, so the day attachments ship every message carrying one would have vanished from the conversation handed across a hop, in silence. The text is taken out of the parts now. `agents.name` has no unique constraint and duplicating a Bot makes a second with the same name, so a hop addressed by name went to whichever sorted first — or was refused as ungranted because the other twin was the granted one. Two matches are now refused by naming the ids to choose between. Neither tool is offered to a Bot that runs at its own endpoint: they execute here, and the callback path executes MCP refs only. That was true and unsaid, the docs claimed the opposite, and a `bot` grant naming such a Bot was stored dead. Said at the branch, corrected in the docs, refused at the door. The grants query behind the tool ran on every run of every Bot, before the caps that would discard it, so a deployment with the feature switched off still paid for it. And building the addressed Bot resolved the whole roster to return one, on every delivery and again on every retry. Nothing reaped this kind, so hop rows accumulated for ever and the fan-out cap's prefix count paid for the growth on every offer. They are dropped after a day, which is far past the point where the key still stops a duplicate. Taking the conversation's lock returned "busy" for every failure, so a renamed underscored API would have looked exactly like ordinary contention while every hop retried to exhaustion. Only a conflict means busy now. `count` had no callers and misdirected about how the cap works; it is gone. The caps' defaults live in four places that cannot be merged, so a test holds them together.
Deploying this branch to the live cluster with `helm upgrade --reuse-values` failed on `.Values.routines.enabled`: the same fault review had just found in `config.handoff`, in a key that came from somewhere else, one release later. A key added by the release being installed is absent on every deployment that already exists, and reaching through it unguarded fails the whole render. So the check is no longer a list somebody remembers to extend. It diffs this chart's values against the last released chart, or against main where the chart has not shipped, and renders once per key that is new — refusing both a failed render and a value that comes out empty. Empty is measured against a baseline render, because the bundled PostgreSQL emits an empty `annotations:` of its own and a check that cried about that would teach everyone to ignore it. Writing the detector honestly took three passes: `key:` with nothing after it is also how YAML opens a mapping, and a block sequence may sit at the same indentation as the key it belongs to. It now asks whether anything belongs under the key rather than what the line looks like.
`| default` substitutes whenever a value is EMPTY, and in Go templates zero is
empty. So the guard added last round for an absent `config.handoff` rewrote
`maxDepth: 0` to `1`: a deployment that had switched handing work between Bots
off got it switched back on, silently, by a fix for something else. The
values.yaml comment right above it says "always set, including the zeroes", and
it had stopped being true. A guard that defeats an off switch is worse than the
nil dereference it was added for.
`kindIs "invalid"` asks the question actually being asked — whether anybody said
anything at all — and the same treatment goes to the culler's deadline and the
routines schedule. That schedule was also wrong on its own terms: it fell back to
every minute where everything else documents every five.
The values-key script had the same shape of fault. `path.slice(0,
path.lastIndexOf("."))` chops the last character of a dotless key, so `routines`
became `routine`, no values file has that, and every NEW TOP-LEVEL KEY was
filtered out of the render check — the exact case the script was written for. It
now also asserts that a zero renders as a zero, which is what would have caught
the bug above; both were verified by putting each fault back.
Three more from the same review. A `bot` grant to a Bot at its own endpoint was
refused on the way in AND on the way out, so a dead row could never be deleted:
taking something away is always allowed now. `botsReachableFrom` did not filter
on agent type, so grants made before that check stayed configured and inert. And
a routine's turn still resolved the whole roster, despite the docstring on
`onlyBotId` naming it as one of the two callers it was added for.
The two reuse items were right as well. Text extraction from structured content
existed twice; `textOf` is one copy. The markers now live in
`shared/handoff-markers.ts` and are read from both sides — I had argued this
risked the browser bundle, and it does not: it typechecks and builds. What
replaces the copies-match test is one that holds each SENTENCE to its marker,
which is the drift that remains possible.
|
Round 3, all seven confirmed and fixed in 65b62db. Each was reproduced before it was touched. The off-switch defeat is the one that matters, and it was mine. `| default` substitutes on EMPTY and zero is empty, so the nil-guard I added last round rewrote `maxDepth: 0` to `1` — switching the capability back on for a deployment that had switched it off, with the values.yaml comment directly above it still claiming "always set, including the zeroes". A guard that defeats an off switch is worse than the nil dereference it was added for. It uses `kindIs "invalid"` now, which asks whether anybody said anything at all. Verified across all five cases: 0 stays 0, absent map defaults, absent leaf defaults, explicit values pass through. The script bug you found by simulation is real and was the worst kind of irony — The script now also asserts a zero renders as a zero. I checked both new assertions by putting each fault back: with Also fixed: the routines schedule fell back to every minute where everything documents every five; the routine turn now passes Both reuse items too. |
The bot-grant checks ran before the admin gate on a route that only needs a signed-in user, so any of them got three distinguishable 403s: whether an id exists, and whether it is built-in or remote, for Bots including other people's private ones. The comment two blocks below says a refusal must not become a way to probe the skill table, and `handoff.ts` in this same feature collapses exactly this on purpose. The role is checked first now, and everybody who is not an administrator gets one sentence. The fan-out prefix was built from a runId that arrives on the request. A run calling itself `notice` took the prefix every failure notice is keyed under, so one turn's budget of three was spent by dead hops belonging to other people. The run is hashed into the key now, which removes every character a caller chooses while keeping it stable for the run, which is all the cap needs. `offer` answered true both for work it queued and for a key already there, so a repeated ask was reported to the model as handed over while the row it named had long since been delivered and finished: nothing queued, nobody going to run it, and a Bot promising the person an answer twice. It answers `queued`, `already` or `refused`, and the desk says the honest thing for the middle one. The desk resolved the roster as `role: "user"`. An administrator's hop to a Bot they can see and chat with in the UI was refused as "no such Bot" — the failure `index.ts` warns about for a routine's owner, one file over. Asked for now, per hop, and the same for the conversation a hop answers in. Also: a grant's target was never checked to exist, so a typo stored happily and every hop then refused as not-granted; the delivery loop swept every two seconds for a deployment that had set the fan-out cap to zero; the off-switch assertion in CI sat under the added-keys check and would have stopped running once these keys shipped; and the caps test matched the template's source, so `| default` coming back or the two variables being swapped both passed. It renders and reads the value now, and I checked it catches both. Smaller, all from the same review: the notice-key comment claimed nothing purges this kind, which `reap` in the same file contradicts; a failed hop's reason went into a person-facing sentence verbatim, platform response body and all; the ambiguity fallback used a list one line out of step with the check guarding it; and the docs said a grant naming a remote Bot is refused without saying it means the grantee, or that a Bot made through the UI is always remote — so on a deployment with no tenant package nothing can hold `message_bot` at all.
|
Round 4, fixed in d18b6f3. Both must fix items were real, and #1 is the one I am least happy about. 1. Enumeration. Confirmed, and the ordering was defended by a comment I wrote. Two blocks below it another comment of mine says a refusal must not become a way to probe the skill table — I understood the property and then broke it one block up. The role is checked before anything is looked up now; everybody who is not an administrator gets one sentence for all three cases, asserted by a test that collects the refusals for an existing Bot, a remote Bot and an unregistered one and requires the set to have exactly one member. 2. Client-supplied runId. Confirmed, both halves.
Should fix, all four: the desk resolves the role per hop (and so do the two #6 was right and the fix was worse than you said. Matching the template source meant Non-blocking taken: #9 (platform response bodies no longer reach a person-facing sentence — status and a generic phrase, body stays in the trail), #10 (the "nothing purges this kind" comment is false and load-bearing), #13 (docs now say grantee and state the real scope: a Bot made through the UI is remote, so with no tenant package nothing can hold Not taken, with reasons: #8, #11, #12. Happy to do them — #8 in particular is a real gap since the lock wrapper has no tests — but they are the three where I would rather not make more untested changes to this branch tonight. Say the word. 1586 server tests, 179 app tests, 13/13 CI, and the new-values-key check clean on all five targets. |
The caps test shelled out to `helm template`, and the suite it lives in runs in a job with no Helm binary. A missing binary does not fail that test: `spawnSync` returns a non-zero exit, the helper answers undefined, and undefined is compared to a number. It passed locally, where Helm is installed, and failed in CI — which is the right way round, but only by luck. The two halves are split along where Helm exists. The chart job's script holds the rendered fallback to values.yaml and holds a zero to zero; the suite holds values.yaml to the code default and to the docs. Together they still chain from what a container receives back to what is written down, and neither half depends on a tool its job does not have.
|
Follow-up: CI caught something my local run did not, and it is worth naming because it is the failure mode I keep arguing against. The caps test shelled out to So the same test I had just rewritten to stop asserting the wrong thing was asserting nothing at all in CI. Fixed in e4b8d97 by splitting along where Helm exists: the chart script holds the rendered fallback to values.yaml and holds a zero to zero, and the suite holds values.yaml to the code default and the docs. The chain from what a container receives back to what is written down is unbroken, and neither half depends on a tool its job does not have. 13/13 green now. |
…am throwing Round 4 taught the desk to resolve the real role and left the delivery building the same person as an ordinary user. The two then disagreed in the worst direction: an administrator's hop to a Bot only they can see was accepted, the model was told it had been handed over, and every delivery attempt failed to build that Bot until the person was told it never answered. A refusal that failed closed became a lie that failed slowly. `agentFor` takes the resolved person now, so the Bot the desk agreed to is the Bot that gets built. The seam that resolves them throws when a role cannot be established. Everything in this module answers with a sentence — the file's opening paragraph is about exactly why — so a revoked role or a database blink ended the run with nothing said, through a seam added to fix something else. `mayAddress` beside it catches for the same reason. It returns null now and the hop is refused in words. On the delivery side the throw survives, because there the hop should fail, but the sentence a person eventually reads is no longer "A routine requires an authorized owner." Reaping was inside the on/off gate, so switching the capability off froze it: the rows made while it was on stayed at the head of the queue, and switching it back on delivered a month-old question to somebody who had stopped waiting. It is housekeeping about the past and runs regardless. Also: a Bot could be granted itself, which the desk refuses as a self-hop, so the row was dead when written; and the duplicate-ask refusal left no audit row while every other refusal leaves one, and claimed "in this turn" when the run id it infers that from arrives on the request. Both fixed, and the duplicate now has the test whose absence would have let a revert pass.
|
Round 5 validated: both new bugs were real, and both were mine from round 4. Fixed in 2cc833c. 1. The half-threaded role. Confirmed at 2. The throwing seam. Confirmed, and the contract is stated in the file's own opening paragraph ( All four smaller items taken:
Also added a test for the null-actor path, since that seam is new and its whole point is not throwing. 1587 server tests, 179 app/shared, chart check clean on all five targets. |
2 participants
Closes #192.
One Bot can address another and the addressed one answers for itself, with its own role, tools and grants, as the same person. Every hop is a claimed row on the queue #216 shipped, so it survives the pod it started on and lands on whichever replica gets to it.
What a person sees
A Bot with a
botgrant is offeredmessage_bot. It names the task, anything that bounds it and what a good answer looks like, and the deployment decides everything else: who is being addressed (against the roster that person may see), who is asking, where the answer lands, and how deep the chain has gone.The answer lands in the addressed Bot's own conversation with that person. Not the one that asked, and this is the platform rather than a choice: an Intelligence thread is owned by exactly one agent, and
assertThreadAgentOwnershipis unconditional. So the conversation that asked says where the work went, and the one that answers moves to the top of the roster with an unread mark. Its transcript keeps one line saying who asked and what for, not the envelope: the model needs the constraints and the shape of a good answer, a person scrolling needs to know why that Bot suddenly spoke.A hop that fails for good sends the asking Bot back into the conversation the person is watching to say plainly that nothing came back. Otherwise a question handed on and never answered looks exactly like a slow one.
ask_personsits besidemessage_botand competes with it. A Bot that needs judgement should stop rather than guess or hand the question to a Bot that cannot settle it either, and a model with no named way to stop takes one of the two it has. It is offered to every run, granted anybody or not: asking the person already in the conversation costs nothing, and a deployment able to switch off the safe exit while keeping the expensive one would be backwards. Who "a person" is is a seam.What driving it on a cluster found
Every one of these was silent, and each would have been the next one:
POST /api/threads/:id/lockhands one back and it reads like the thing to present. It is what a browser presents; the runner's socket has its own. Passed in, the socket is refused, and the runner treats a socket that will not connect as something to retry rather than as a failed run: nothing is emitted, nothing completes, and every hop hangs for ever in total silence.runAgenttakesrunId,tools,contextandforwardedProps. Amessagesarray passed as a run parameter is ignored without error, so the Bot ran against an empty conversation and answered "how can I help?" to a question printed directly above its reply. Told to answer with one particular word, it asked what it could help with, which is how this was finally pinned down.toolCallIdmatching nothing. The asking Bot's last act is always the call that handed the work on, so every hop carried one.Driven
On EKS, in a browser, against the real cluster:
ask_personchosen over guessing on a request only a person could settle, with the reason on the record;The queue's new prefix cap has an integration test that drives five concurrent offers against a real PostgreSQL. It fails against the old
offer.After review
Four more, all found by review, all reproduced against a real PostgreSQL before anything was touched, all fixed in 1395278. The suite was green through every one of them.
bot-1was safe,bot-2andbot-3expired, replica B took and delivered them, replica A delivered them again — and A reported all three as delivered, becausefinishreturns a boolean saying the lease had gone and nothing read it. Every claimed hop is renewed now, and each is renewed once more immediately before its own delivery starts. That second renewal is deliberately the check as well: consulting the heartbeat's own bookkeeping would only catch a refusal it had already seen, and a process paused long enough to lose its lease never asked.ChannelStore.directlooked and then made, which is not find-or-create: each of two concurrent deliveries found nothing and made a channel, so one person had two conversations with that Bot and their answers split between them. A Bot asked for several things in one turn produces exactly that. Find and make share one transaction now, serialised on the person and the Bot. An advisory lock rather than a unique constraint, because what has to be unique is not a column: it is "this person's channel whose whole roster is this one Bot", a count over another table.botkind and the API did not. Worse,kindwas never checked at runtime, so an admin could grant one by accident of the missing check while revoke rejected it outright: enable by hand, no way to turn off, against a design that rests on a revoked grant applying to the very next hop. Both endpoints take it, one Bot reaching another is an administrator's decision like an MCP tool rather than a skill somebody attaches to a Bot they own, andkindis checked against the three that exist.BOT_HANDOFF_MAX_PER_RUN=0switched the capability off everywhere except the model's tool list, so every call was refused and the Bot told the person it had tried. Both zeros close the same door now.Each has a regression test that fails without its fix, checked by reverting each in turn. The two races needed integration tests against a real database and a heartbeat interval that could be injected: a stub answers whatever it is told, which is why the green suite said nothing.
Driven again on the cluster afterwards: a hop delivered end to end, and a
botgrant revoked and re-granted through the API rather than by hand, attributed to the administrator who did it.Known limitation
A hop that fails and is retried leaves one "asked …" line per attempt in the addressed Bot's conversation, with no answer under any of them. Every attempt pushes
RUN_STARTEDcarrying the ask before the failure happens inside the agent, and the transcript is replayed per run, so no message id collapses them and a thread message cannot be withdrawn. Persisting the ask only on the first attempt was considered and rejected: if that attempt loses the lock race the line is never written at all, and a later successful answer then appears with nothing saying why the Bot spoke. Cosmetic, only on repeated failure, and the asking conversation now explains it in words. A proper fix needs the platform to discard a run that produced no assistant message.