Relay handoff answers home, show working channels, and open the app to the keyboard - #290
Relay handoff answers home, show working channels, and open the app to the keyboard#290guidovizoso wants to merge 17 commits into
Conversation
Two columns on users say where somebody is in first-run onboarding and when they finished; /api/me carries the status and one POST moves it. The app redirects an unfinished user to /onboarding — an animated three-step wizard — and an address the build does not know now goes home through the same gate instead of a bare 404.
# Conflicts: # bun.lock # server/drizzle/meta/0015_snapshot.json # server/drizzle/meta/_journal.json # server/src/app.ts # server/src/index.ts
… next poll The handoff queue was swept every two seconds, and a person is waiting through every hop, so each leg of a handoff cost up to a full interval doing nothing. Offering work now fires pg_notify with the kind as payload, inside the offering transaction where there is one so it fires on commit and never before, and any replica can listen on a dedicated connection and kick its sweep immediately. A notification is a latency optimisation, never a delivery mechanism: one lost in transit costs up to one poll interval, not the work, and the queue's tables stay the only truth.
A transient busy flag on the channel activity event, announced and never written: busy is a moment, not a fact about the channel, and a missed signal costs at most a stuck-looking dot until the next real event, never data. Two ways in. The server signals by thread (signalBusy) for the runs it can see — the runtime's lock acquire and release now carry an onRunBusy seam, so every run the platform processes lights its channel, including one whose tab has navigated away — and a scratch thread maps to no channel and signals nowhere, which is the point of a scratch thread. The browser signals by channel (POST /:channelId/busy) for the one thing the server cannot see, a person's own turn beginning, with a membership check so belonging to a channel is not something an outsider can probe for.
A forward hop used to answer in the addressed Bot's own channel with the person: two conversations for one question, and the answer landed somewhere they never asked anything. Now the hop runs in a scratch thread of the addressed Bot's own — minted per hop, never mapped to a channel, never shown — and what it said comes home through a second queued hop that has the asking Bot relay the answer, attributed, in the conversation the person is watching. The delivery gathers the Bot's words from the stream as it goes past, because the runner publishes the turn to the platform and the events are the one chance to hear it. The relay rides the same durable queue as the turn that produced it, so a pod dying between the two loses the relay to a retry rather than for ever; the answerIn marker that stops a failure notice recursing stops a relay relaying. Answers are clipped at 12k characters so a Bot that comes back with a book cannot swamp the relaying run's prompt, and a backwards hop reads only the tail of the asking conversation so relaying never grows slower with the channel. Wired in index.ts: the scratch thread replaces the direct-channel answerIn, the roster announcement resolves thread to channel and happens only when the turn said something, the asking channel is lit while a forward hop runs, and the queue's new offered-work notification kicks the sweep so a person is not waiting out a poll interval per leg.
Three bouncing dots badged on the channel's avatar, driven by the busy flag on the activity socket. Socket-only and transient on the summary type: the roster query never returns it, so it is undefined until a busy event arrives and drops whenever the roster refetches — the acceptable failure for a hint about a moment. The event patcher flips only the busy field: the spread that serves ordinary activity would carry the event's null message onto the row and wipe the preview, and busy is not activity, so the row does not re-sort either. The channel reports its own turns over the new busy endpoint, fire-and-forget, and deliberately does not clear on unmount: a turn keeps running server-side after the person leaves the channel, and the server clears it when the run's lock is released.
A relayed handoff answer runs on the server and lands in the thread with no browser attached; the transcript restored history once, on mount, and would show the new turn only after leaving and coming back. The chat now watches the roster's own channel-list cache — the sidebar updating and the transcript refreshing are one signal and cannot drift apart — and when this channel's lastMessageAt advances to a moment a Bot authored, the durable history is read again and messages whose ids the transcript has never seen are appended. Appended by id, not compared by length, because the stored read keeps only what the platform can parse and can be shorter than the screen while still holding the news. Retried briefly, since the roster is patched when the turn is on record with the runner and the platform's read can be a beat behind. The chat also reports its own turns to the busy endpoint, keyed on whether a turn is in flight, so the roster's working dots cover the one run the server cannot see begin.
A small hotkey registry, one place on purpose: the binding a listener matches against and the combo the settings page shows are the same record, so the list under Preferences is what the keys actually do rather than what somebody remembered they did — each key drawn as its own keycap, symbols on a Mac and names elsewhere. Matching is exact rather than at-least, so Shift+N does not fire on Cmd+Shift+N and shadow whatever the browser means by it, and a plain Shift+letter combo is left alone while the focus is anywhere editable. Bound in _authed rather than _app, so a person on settings or admin can start a chat without first clicking back into the app frame. The new-channel composer's recipient picker takes focus on arrival, so Shift+N then typing a name is one motion.
The wizard shipped unformatted; biome's own output, no hand edits.
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
Reviewed and driven on a real EKS deployment (single user, sandbox computers), not just read. The headline change works: I asked General Assistant to put a question to Knowledge and the answer came home into the asking conversation as "Knowledge replied: CORMORANT", attributed, under the ▸ Asked Knowledge marker. Confirmed in a fresh channel and in a 65-message one, and confirmed durable by reading the thread back from the platform rather than trusting the screen.
Also verified on the deployment: the busy badge appears on the roster avatar while a hop runs and clears afterwards; the roster bumps and previews the relayed answer; the onboarding gate redirects to /onboarding and holds against a direct navigation to /admin/audit; migration 0025 correctly stamps an existing user as onboarded, so an upgrade shows no wizard (checked in the database); and Shift+N works while correctly not firing from inside an input, which is the part that matters for a plain Shift+letter combo.
Suite green here too: 2067 tests, typecheck and lint clean.
What I would fix before merging
No changelog entry, and this reverses what 0.0.5 shipped saying. This is the one I would hold on. The released 0.0.5 notes say, in these words:
The asking Bot does not relay text on its behalf, so what you read is the answer that Bot actually gave rather than another Bot's summary of it.
This PR makes the asking Bot relay exactly that, as a summary, in its own voice. Somebody who read the 0.0.5 notes and upgrades gets the opposite of what they were told. There are five user-visible changes in here (relay, busy channels, transcript catch-up, onboarding, Shift+N) and no entry for any of them, so the fix is an Unreleased section that also corrects the 0.0.5 paragraph rather than leaving it standing.
app/src/routes/_authed/onboarding.tsx:104 has max-w-lgoverflow-hidden. Two classes run together, so neither max-w-lg nor overflow-hidden applies. It is visible: the roster cards render wider than every other step of the wizard.
Lower confidence, worth a look
- Empty roster after finishing onboarding. I landed at
/with no channels in the sidebar. The API had all five, and a reload restored them, so nothing was lost. Seen once and I could not cleanly re-run it, so a lead rather than a confirmed defect.invalidateCurrentUseronly invalidates the current-user key, which may be the thread to pull. - One relay rendered truncated. In the long channel the transcript showed
Knowledge: ALBATand stayed that way, while the stored message was the completeKnowledge replied: ALBATROSS. Render only, and it did not reproduce in a fresh channel. onboarding_stepis currently dead weight. The column, the migration,setStep, the endpoint field andadvanceOnboardingMutationOptionsare all unused.onboarding.tsx:146says that is deliberate while the wizard is being designed, which is fair, butpeople/onboarding.tsdocumentsstepas "where the wizard resumes if they left halfway through" and it never resumes. I watched it stay0through a full run. Either drop the claim from that comment or wire it up.- Placeholder agents are indistinguishable from real ones. "Support Agent" sat beside General Assistant and Knowledge in identical styling on a deployment that has no such Bot. The comment says the invented names illustrate a roster "without claiming any of these exist here", but nothing on screen marks them as illustrations, so it does read as a claim.
One note on my own testing
My browser harness cannot deliver a shift-modified keystroke (pressing it into a focused textarea typed nothing), so Shift+N looked broken twice before I dispatched a real KeyboardEvent and saw it navigate. Flagging it so nobody repeats the false alarm.
Design reading: relaying through the queue with answerIn doing double duty as the loop guard, the answer clipped, and a relay failure logged rather than failing the hop into a second turn, all look right to me. The scratch thread explanation for why the addressed Bot cannot speak in the asking thread is convincing, and the busy signal covering only the forward leg (because the backwards leg already lights through the thread lock) is the detail I would have got wrong.
|
Reading the relay half of this, and the scratch thread makes a lot of sense — a leaked lock on a thread nobody is ever shown costs nothing. One thing about the backward leg I wanted to raise, because I think this change moves it from rare to common, and I would rather ask than assume I have read it right.
For a forward hop that costs nothing, as above: For a relay it is the conversation the person is sitting in.
Before this PR the Would hoisting the read above the |
The history read is the one call in a delivery that throws on a platform error, and it sat after the lock was acquired but before the try whose finally gives the lock back: a 500 from the platform leaked the lock for its full TTL. On a forward hop that lock is a scratch thread and the leak costs nothing; on a relay it is the asking conversation itself, so the person could not type for two minutes, the retry a minute later collided with the hop's own leftover hold and spent an attempt on it, and a relay that ran out of attempts vanished without a notice. The read is of the conversation that asked, which the lock — taken on the conversation being answered in — never protected. Hoisted above the acquire it fails before anything is held, and the happy path holds the lock for less of the turn too. Raised by kevin9327 on #290, who had the same reordering up as #305 against main; a test now pins the ordering.
Finishing the wizard navigates straight into the app, and the channel list — cached from before the wizard, or from a fetch that ran while it held the screen — could greet the person with an empty sidebar their reload then fixed. Seen once on a real deployment during review of #290 and not cleanly reproduced, so this is a hardening of the seam rather than a confirmed fix: both invalidations use refetchType "all" for the same reason, nothing is observing these queries while the wizard is up.
Two review findings on the wizard's roster step. The invented agents that top up a sparse deployment rendered identically to real ones, so "Support Agent" read as a Bot the deployment has; they are dimmed and labelled Example now. And max-w-lg had run together with overflow-hidden into a class that applied neither, which is why the roster cards drew wider than every other step. Also stops the onboarding store's comment promising that step is where the wizard resumes: it is stored and served but nothing resumes from it yet, and the comment now says so instead of describing a behaviour that does not exist.
…stance Five user-visible changes were on this branch with no entry: the relay, the working indicator, the transcript catch-up, the onboarding wizard, and Shift+N. Worse, 0.0.5's notes promised the asking Bot does not relay text on the addressed Bot's behalf, and this branch makes it do exactly that — so somebody upgrading off those notes got the opposite of what they were told. The Unreleased section now names the reversal outright, and the 0.0.5 paragraph carries a pointer forward rather than standing uncorrected.
|
@kevin9327 You read it right, and it was in fact one notch worse than "heartbeat never armed": the read sat after Carried the hoist here as 56700ad: the history read now runs before |
|
@davidmckayv Thanks for driving it on a real deployment — the busy-badge and migration checks especially. All addressed as of a15b934: The two holds:
The leads:
Also carried kevin9327's lock-ordering point in 56700ad with a pinning test. Suite, typecheck and lint green here including the new test. |
# Conflicts: # CHANGELOG.md
`startWorkOfferedListener` holds a Postgres connection for the life of the process and returns a `stop()` for giving it back, and the return value was dropped on the floor. The shutdown beside it stops the two listeners either side and says why in its own comment: "so a watch-mode restart does not leave two behind on every reload". This one was the third, and it was the one not stopped, so every reload of `bun run dev` on a deployment with handing work between Bots switched on left a connection behind. Held at module scope because the listener is started inside the gate that may never run, and the shutdown is at the top level: undefined there means a deployment with the capability off, which never started one. Checked rather than assumed: `pg_stat_activity` goes from one row to two when the listener starts and back to one after `stop()`.
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
Re-reviewed at 5e40b6b. Everything I raised is addressed, and the changelog fix went further than I asked: it says plainly what the new behaviour costs ("the asking Bot's account of the answer rather than the answer verbatim; very long answers are clipped"), and it annotates the standing 0.0.5 paragraph in place rather than leaving a released note contradicting the code. That is the right shape.
Checked each one:
- Changelog. Four entries in
Unreleased, and the 0.0.5 paragraph now carries a forward pointer. max-w-lgoverflow-hidden. Now two classes.- Placeholder agents. Dimmed and labelled
Example, so an invented name cannot read as a Bot the deployment has. - Empty roster after onboarding.
completeOnboardingMutationOptionsnow invalidateschannelKeys.list()alongside the user, withrefetchType: "all"for the same reason. The comment describes the exact symptom I hit. onboarding_step. Not wired, which is fine, and the store comment now says so outright instead of claiming the wizard resumes. That was the actual defect: the claim, not the column.
On the lock-ordering fix
Good catch by @kevin9327, and the fix is right. I confirmed the test pins it: reverting just the hoist fails a history read that fails > takes no lock, so nothing is leaked for the retry to collide with, and it passes with it.
One thing to watch when this and #305 both land. #305 is still open against main with the same fix, touching the same three files, but it hoists the read outside the try, while this PR hoists it to the top of the try. Against main that difference is invisible. Against this branch it is not: this PR adds setBusy(true) before the try and clears it in the outer finally, so a read that throws from outside that try would leave the asking channel lit as busy with nothing to turn it off. The placement here is the correct one for this branch.
So if #305 merges to main first, the rebase conflict in handoff-delivery.ts needs resolving toward this branch's placement (read inside the try), not toward #305's. Both PRs also touch CHANGELOG.md, so expect a conflict there too.
Pushed one fix
startWorkOfferedListener holds a Postgres connection for the life of the process and returns a stop() for giving it back, and the return value was dropped. The shutdown handler a few hundred lines below stops the two listeners either side of it and says why in its own comment: "so a watch-mode restart does not leave two behind on every reload". This was the third listener and the one not stopped, so every reload of bun run dev with handing work between Bots switched on left a connection behind. Pushed as 61e2065: held at module scope because the listener starts inside a gate that may never run.
Checked rather than assumed: pg_stat_activity goes from one row to two when the listener starts and back to one after stop().
What I checked
Re-deployed to a real single-user EKS cluster and drove it:
- The relay still works after the lock hoist: "Knowledge replied: KESTREL" landed in the asking conversation.
- The roster does repopulate after finishing the wizard. Worth saying plainly because I nearly filed this as still broken: my first screenshot after "Get started" showed an empty sidebar and the channels only appeared on a later frame. It arrives, just not instantly.
- The wizard's example labelling and the class fix both render as intended: cards constrained, "Support Agent" dimmed with an
Exampleline under it, the two real Bots not.
Also ran here: typecheck, biome format and lint clean; full suite 2113 tests, 0 fail once the local dev worker is stopped (the bot.message queue collision the PR body warns about, so that note is worth keeping).
The framing existed only in a code comment ("OpenBot exists to be forked"), which is the wrong place
for the first thing somebody needs to know: that there is nothing to sign up for, nothing to install
as a dependency, and that the example tenant package is a worked example to replace rather than a
default to keep. Read without it, "Quick start" looks like the way you use OpenBot rather than the
way you try the starting point before making it yours.
Placed above the alpha and laptop callouts because it frames both.
|
Also added a README callout, since this came up separately: the repository never said it is a template to clone. That framing only existed in a code comment in It now sits above the alpha and laptop callouts, because it frames both: there is no hosted version to sign up for, nothing is published as a package to depend on (every workspace here is Without it, "Quick start" reads as the way you use OpenBot rather than the way you try the starting point before making it yours. |
…om the README Reported as hard to set up even with an assistant helping, and the reason is legible in `.env.example`: it ships ten empty keys, and only three of them are a person's to fill. The start script defaults two and generates the third, so an assistant reading the file alone will walk somebody through seven values that fill themselves. `prompt.txt` says which three, what each start-up refusal means in the words the server actually prints, and what an assistant must not do on somebody's behalf: run the browser sign-in, echo their keys back, or change the Intelligence URLs, which are correct as shipped and not self-serve to replace. Every claim in it was checked against this repository rather than written from memory: the pinned Bun version, the port defaults, the five refusal messages quoted verbatim, and that TENANT_PACKAGE_DIR is relative to `server/` rather than the root, which is why the default carries a `../` that is easy to drop.
|
Added The reason is visible in So the prompt names the three ( Checked against the repository rather than written from memory: the pinned Bun version, the port defaults, all five refusal messages verbatim, and that |
|
All checks are green and auto-merge (squash) is armed. Branch protection needs one approving review from someone other than @davidmckayv (last pusher — his three follow-up commits, including the wake-up listener shutdown fix, all look right to me) and other than me as author. First approval merges it. |
3 participants
A batch of experience work: handoff answers stop landing in a second conversation, channels show when a Bot is working in them, open transcripts pick up turns that ran with no browser attached, first sign-in gets an onboarding wizard, and Shift+N starts a chat from anywhere.
Handoff answers come home
A forward hop used to answer in the addressed Bot's own channel with the person — two conversations for one question, and the answer landed somewhere they never asked anything. Now the hop runs in a scratch thread minted per hop, never mapped to a channel and never shown, and what the Bot said is relayed back into the conversation that asked, in the asking Bot's voice, attributed. The relay rides the same durable queue as the turn that produced it, answers are clipped so a Bot that returns a book cannot swamp the relaying run's prompt, and the
answerInmarker that already stopped failure notices recursing stops a relay relaying.Because a person now waits through every leg, offering work also fires
pg_notifyso a sweep starts immediately instead of at its next two-second poll. The poll stays as the backstop; a lost notification costs one interval, never the work.Channels show work happening
A transient
busyflag on the activity socket — announced, never persisted. The server signals it from the runtime's thread-lock acquire/release (covering headless hops and runs whose tab navigated away), and the browser reports its own turns over a new membership-checkedPOST /:channelId/busy. The roster badges the channel's avatar with three bouncing dots, patching only the busy field so the preview and ordering are untouched.An open transcript also picks up turns nobody streamed: it watches the roster's own cache, and when a channel's
lastMessageAtadvances to a Bot-authored moment it re-reads durable history and appends messages by id — so a relayed answer appears without leaving and coming back.Onboarding and keyboard
Verification
bun run typecheck, biome format and lint: clean.bun run test:ci) locally: all handoff unit and integration tests pass, including the new relay, batching, and event-patch cases. (The handoff integration tests share the queue'sbot.messagekind with a running dev server; locally they need the dev server stopped, which is expected.)