diff --git a/desktop/src/features/onboarding/welcomeKickoff.test.mjs b/desktop/src/features/onboarding/welcomeKickoff.test.mjs index e10dc8835..30f60c078 100644 --- a/desktop/src/features/onboarding/welcomeKickoff.test.mjs +++ b/desktop/src/features/onboarding/welcomeKickoff.test.mjs @@ -13,6 +13,7 @@ import { selectWelcomeKickoffIntroTeammates, waitForWelcomeKickoffBeat, waitForWelcomeTeammatesOnline, + welcomeKickoffAlreadyFinished, welcomeTeammateNeedsRestart, } from "./welcomeKickoff.ts"; @@ -168,8 +169,18 @@ test("kickoff coordinator preserves one task across rerenders and cancels on nav assert.ok(coordinator.begin("welcome")); }); +test("a clean kickoff posts no scripted closer", () => { + // The intros @mention the lead, which is the mandatory callback in + // base_prompt.md — so the lead is *guaranteed* to wake and reply in-thread. + // Scripting a CTA on top of that gave the user two closers, and the scripted + // one raced the intros and lost. The lead's live reply is the close now; the + // scripted closer is only for reporting a problem. + // See docs/welcome-kickoff-silent-failures.md. + assert.equal(buildWelcomeKickoffCloser([]), null); + assert.equal(buildWelcomeKickoffCloser([], []), null); +}); + test("closer degrades coherently for partial and total startup failure", () => { - assert.match(buildWelcomeKickoffCloser([]), /What can we help you build/); assert.match(buildWelcomeKickoffCloser(["Honey"]), /Honey is having trouble/); assert.match( buildWelcomeKickoffCloser(["Honey", "Bumble"]), @@ -429,6 +440,81 @@ test("intro replies reach the closer classification without the user opening the ); }); +// A clean kickoff posts no closer, so "already finished" can no longer be a +// single marker lookup — the marker only exists when something went wrong. If +// this regresses, the start effect re-arms on every revisit and restarts the +// whole Welcome team each time the channel is opened. +test("a clean kickoff reads as finished from the intros, with no closer marker", async () => { + const agentSet = { lead: fizz, teammates: [honey, bumble] }; + const finished = await welcomeKickoffAlreadyFinished( + "channel-1", + kickoffOpener, + agentSet, + { + closerExists: async () => false, + fetchReplies: async () => ({ + events: [ + introReply("honey-intro", honey.pubkey, kickoffOpener.id), + introReply("bumble-intro", bumble.pubkey, kickoffOpener.id), + ], + nextCursor: null, + }), + }, + ); + + assert.equal(finished, true); +}); + +test("a kickoff missing one intro is not finished", async () => { + const agentSet = { lead: fizz, teammates: [honey, bumble] }; + const finished = await welcomeKickoffAlreadyFinished( + "channel-1", + kickoffOpener, + agentSet, + { + closerExists: async () => false, + fetchReplies: async () => ({ + events: [introReply("honey-intro", honey.pubkey, kickoffOpener.id)], + nextCursor: null, + }), + }, + ); + + assert.equal(finished, false); +}); + +test("a closer marker still short-circuits the finished check", async () => { + // The failure paths and the solo opener still carry the marker, so that + // evidence must keep working without a thread fetch at all. + let fetched = false; + const finished = await welcomeKickoffAlreadyFinished( + "channel-1", + kickoffOpener, + { lead: fizz, teammates: [honey, bumble] }, + { + closerExists: async () => true, + fetchReplies: async () => { + fetched = true; + return { events: [], nextCursor: null }; + }, + }, + ); + + assert.equal(finished, true); + assert.equal(fetched, false); +}); + +test("no opener means the kickoff has not finished", async () => { + const finished = await welcomeKickoffAlreadyFinished( + "channel-1", + null, + { lead: fizz, teammates: [honey, bumble] }, + { closerExists: async () => false }, + ); + + assert.equal(finished, false); +}); + test("merging the opener subtree never double-counts an already-visible reply", () => { const honeyIntro = introReply("honey-intro", honey.pubkey, kickoffOpener.id); // An open thread feeds the same replies in through both sources. diff --git a/desktop/src/features/onboarding/welcomeKickoff.ts b/desktop/src/features/onboarding/welcomeKickoff.ts index 2588410f1..858173bb6 100644 --- a/desktop/src/features/onboarding/welcomeKickoff.ts +++ b/desktop/src/features/onboarding/welcomeKickoff.ts @@ -24,13 +24,25 @@ import { } from "@/shared/api/tauriManagedAgents"; import { hasManagedAgentChannelMessageMarker } from "@/shared/api/tauriManagedAgentMessageMarkers"; import { sendManagedAgentChannelMessage } from "@/shared/api/tauriManagedAgentMessages"; -import { getPresence, listManagedAgents } from "@/shared/api/tauri"; +import { + getPresence, + getThreadReplies, + listManagedAgents, +} from "@/shared/api/tauri"; import { getProfile } from "@/shared/api/tauriProfiles"; import type { Channel, ManagedAgent, RelayEvent } from "@/shared/api/types"; import { normalizePubkey } from "@/shared/lib/pubkey"; import { useQueryClient } from "@tanstack/react-query"; export const WELCOME_KICKOFF_OPENER_MARKER = "buzz-welcome-kickoff.opener.v1"; +// Despite the name, presence does NOT mean "the kickoff finished": a clean team +// kickoff posts no closer at all and is closed by the lead's own callback reply. +// This marks only the paths that had something to say — a failed or delayed +// teammate, or no team to wait for (the solo opener carries it too; see +// `additionalMarkers` below). Ask `welcomeKickoffAlreadyFinished` whether a +// kickoff is done: a marker needs a message to exist, so absence proves nothing. +// The string stays as-is — renaming it strands the markers already on relays for +// kickoffs that failed, and the dedupe below would let them report twice. export const WELCOME_KICKOFF_CLOSER_MARKER = "buzz-welcome-kickoff.closer.v1"; export const WELCOME_KICKOFF_PROVIDER_MARKER = "buzz-welcome-kickoff.provider-required.v1"; @@ -87,8 +99,19 @@ const kickoffCoordinator = createWelcomeKickoffCoordinator(); const closerInFlight = new Set(); const TEAMMATE_READY_POLL_MS = 250; const TEAMMATE_READY_WAIT_MS = 60_000; -const TEAMMATE_INTRO_WAIT_MS = 15_000; +// How long a teammate that is alive but silent gets before the closer reports it +// as delayed. This budget has to cover a whole agent turn — wake on the p tag, +// fetch context, run inference, shell out to `buzz messages send` — so it is +// tens of seconds, not a UI beat. At 15s the closer reliably lost the race and +// announced "taking longer than expected" seconds before the intros landed, +// contradicting itself in front of a brand new user. A *crashed* teammate does +// not wait this out: `failed` is read from agent status and closes immediately, +// so this budget only covers the case where patience is the right answer. +const TEAMMATE_INTRO_WAIT_MS = 60_000; const CLOSER_BEAT_MS = 3_000; +// The intros are the first replies to the opener; this only has to be deep +// enough to see them, not to page a real conversation. +const INTRO_LOOKUP_LIMIT = 200; const closerAbortControllers = new Map(); const closerTimeouts = new Map< string, @@ -238,12 +261,24 @@ export async function waitForWelcomeKickoffBeat(options: { }); } +/** + * Build the closer, or `null` when nothing needs to be said. + * + * The scripted closer exists to report a *problem*: a teammate that crashed or + * hasn't spoken yet. A clean kickoff returns `null` and posts nothing, because + * the lead already closes the conversation itself — the intros `@mention` the + * lead (the mandatory callback in `buzz-acp/src/base_prompt.md`), which wakes it + * and it replies in-thread. Scripting a second CTA on top of that gave the user + * two closers, and the scripted one — fired on a timer, before the intros were + * in — was the one that could be wrong. See + * docs/welcome-kickoff-silent-failures.md. + */ export function buildWelcomeKickoffCloser( failedNames: readonly string[], delayedNames: readonly string[] = [], -) { +): string | null { if (failedNames.length === 0 && delayedNames.length === 0) { - return WELCOME_KICKOFF_CTA; + return null; } if (failedNames.length === 1 && delayedNames.length === 0) { return `${failedNames[0]} is having trouble starting — you can check on them in Agents.\n\n${WELCOME_KICKOFF_CTA}`; @@ -339,6 +374,48 @@ async function markerExists(channelId: string, marker: string) { }); } +/** + * Has the kickoff already run to completion in this channel? + * + * This used to be a single `closerMarker` lookup, which worked only because + * every kickoff ended in a scripted message we could tag. A clean kickoff now + * posts nothing, so there is no marker to find and the marker check alone would + * report "not finished" forever — re-arming the start effect and restarting the + * Welcome team on every single revisit. + * + * So fall back to the other durable, relay-side evidence that it finished: the + * opener's thread holds an intro from each teammate. Both signals are + * server-side, which is what makes this safe across app restarts and devices — + * a local latch would not be. + */ +export async function welcomeKickoffAlreadyFinished( + channelId: string, + opener: RelayEvent | null, + agentSet: WelcomeAgentSet, + options: { + closerExists?: (channelId: string) => Promise; + fetchReplies?: typeof getThreadReplies; + } = {}, +) { + const closerExists = + options.closerExists ?? ((id: string) => markerExists(id, closerMarker)); + const fetchReplies = options.fetchReplies ?? getThreadReplies; + if (await closerExists(channelId)) return true; + if (!opener) return false; + const { events } = await fetchReplies(opener.id, channelId, { + limit: INTRO_LOOKUP_LIMIT, + cursor: null, + }); + const introAuthors = introAuthorsAfterOpener( + events, + opener, + agentSet.teammates, + ); + return agentSet.teammates.every((agent) => + introAuthors.has(normalizePubkey(agent.pubkey)), + ); +} + export function welcomeTeammateNeedsRestart( agent: ManagedAgent, leadPubkey: string, @@ -453,9 +530,14 @@ async function sendWelcomeKickoffCloser({ }: { agentSet: WelcomeAgentSet; channelId: string; - content: string; + content: string | null; opener: RelayEvent; }) { + // A clean kickoff has nothing to report, so it posts nothing and the lead's + // own callback reply closes the thread. Enforced here rather than at the call + // sites so the delayed-teammate timer can't sneak a bare CTA out after the + // intros land and re-classify the kickoff as clean. + if (content === null) return; if (await markerExists(channelId, closerMarker)) return; await sendManagedAgentChannelMessage({ @@ -491,17 +573,24 @@ export function useWelcomeKickoff( () => markerEvent(channelEvents, openerMarker) ?? null, [channelEvents], ); - // Retire the watch once the closer exists: the kickoff is resolved, so - // revisits to Welcome shouldn't keep refetching the subtree forever. + const agentSet = React.useMemo( + () => + resolveWelcomeAgentSetForRelay( + managedAgentsQuery.data ?? [], + activeCommunity?.relayUrl, + ), + [activeCommunity?.relayUrl, managedAgentsQuery.data], + ); + // Retire the watch once the kickoff is resolved, so revisits to Welcome don't + // keep refetching the subtree forever. // - // This has to be a latch rather than a plain derivation. The closer is a - // *thread reply* to the opener (see sendWelcomeKickoffCloser), so it never - // appears in `channelEvents` unless the user happened to open the thread — - // deriving from `channelEvents` meant this never retired at all. Deriving - // from `kickoffEvents` instead is self-referential: it gates the query that - // feeds it, so retiring would drop the evidence that justified retiring and - // (on a cache eviction) flip the query back on. Latching per channel keeps - // the decision one-way and stable. + // This has to be a latch rather than a plain derivation. The evidence lives in + // the opener's *thread*, so it never appears in `channelEvents` unless the user + // happened to open the thread — deriving from `channelEvents` meant this never + // retired at all. Deriving from `kickoffEvents` instead is self-referential: it + // gates the query that feeds it, so retiring would drop the evidence that + // justified retiring and (on a cache eviction) flip the query back on. Latching + // per channel keeps the decision one-way and stable. const [resolvedChannelId, setResolvedChannelId] = React.useState< string | null >(null); @@ -516,19 +605,33 @@ export function useWelcomeKickoff( ); React.useEffect(() => { if (!channelId || kickoffResolved) return; - if (markerEvent(kickoffEvents, closerMarker) == null) return; + // A closer only exists when something went wrong, so it can't be the only + // thing we latch on any more — a clean kickoff would never retire. + if (markerEvent(kickoffEvents, closerMarker) != null) { + setResolvedChannelId(channelId); + return; + } + if (!openerEvent || !agentSet) return; + const { failed, unresolved } = classifyWelcomeKickoffResolution( + kickoffEvents, + openerEvent, + agentSet, + ); + // Every teammate introduced itself and none crashed: the choreography is + // done and the lead's callback reply is the close. Nothing further to post. + if (failed.length > 0 || unresolved.length > 0) return; + // Drop any pending delayed-teammate timer. `sendWelcomeKickoffCloser` would + // refuse to post a clean closer anyway, but leaving a timer armed against a + // finished kickoff is a trap for the next reader. + const pending = closerTimeouts.get(channelId); + if (pending) { + globalThis.clearTimeout(pending); + closerTimeouts.delete(channelId); + } setResolvedChannelId(channelId); - }, [channelId, kickoffEvents, kickoffResolved]); + }, [agentSet, channelId, kickoffEvents, kickoffResolved, openerEvent]); const channelEventsRef = React.useRef(kickoffEvents); channelEventsRef.current = kickoffEvents; - const agentSet = React.useMemo( - () => - resolveWelcomeAgentSetForRelay( - managedAgentsQuery.data ?? [], - activeCommunity?.relayUrl, - ), - [activeCommunity?.relayUrl, managedAgentsQuery.data], - ); const readiness = React.useMemo( () => resolveAgentReadiness(runtimesQuery.data ?? [], globalConfig), [globalConfig, runtimesQuery.data], @@ -562,7 +665,13 @@ export function useWelcomeKickoff( teammates: [welcomeTeam[1], welcomeTeam[2]], }; - if (await markerExists(channelId, closerMarker)) { + if ( + await welcomeKickoffAlreadyFinished( + channelId, + markerEvent(channelEventsRef.current, openerMarker) ?? null, + resolvedAgentSet, + ) + ) { return; } if (!readiness.ready) { diff --git a/docs/welcome-kickoff-silent-failures.md b/docs/welcome-kickoff-silent-failures.md index 46e82ce12..7958ecffe 100644 --- a/docs/welcome-kickoff-silent-failures.md +++ b/docs/welcome-kickoff-silent-failures.md @@ -3,11 +3,14 @@ Status: **partly open.** The *perception* gap is handled (see below); the silent paths themselves are not. The [intro loop](#the-intro-loop-a-loud-failure) is mitigated as of 2026-07-18 by three bullets in the base prompt's Callback -Mentions section — **not yet confirmed live against Codex.** Prompt is the only -lever available: a structural cap was considered and rejected (see that section). +Mentions section, and **confirmed live against Codex** — the chain terminated on +its own at four replies. Prompt is the only lever available: a structural cap was +considered and rejected (see that section). The +[double closer](#the-closer-was-racing-a-message-it-could-not-beat) that the same +run exposed is fixed as of 2026-07-18. Context: the Welcome-channel kickoff choreography (`desktop/src/features/onboarding/welcomeKickoff.ts`) where Fizz posts an -opener, teammates introduce themselves in-thread, and Fizz posts a closer. +opener and teammates introduce themselves in-thread. Two failure shapes live here: the **silent** ones (nobody speaks) and the **loud** one (nobody stops). They have opposite symptoms and the same root @@ -51,7 +54,8 @@ that runs away. | 1 | Provider fallback ("connect to an AI provider in Settings…") | Readiness check fails before kickoff | Fizz (marker: `provider-required.v1`) | | 2 | Happy-path opener (mentions teammates, asks them to introduce themselves) | Team online | Fizz (marker: `opener.v1`) | | 3 | Degraded opener ("I'm here with Honey and Bumble…") | Fizz online, zero teammates online within 60s | Fizz (opener + closer markers, self-contained) | -| 4 | Closer variants (clean / failed / slow teammate wording) | 3s beat after intros resolve, or 15s intro timeout | Fizz (marker: `closer.v1`) | +| 4 | Closer variants (failed / slow teammate wording only — a clean kickoff posts nothing) | 3s beat after a teammate is seen crashed, or 60s intro timeout | Fizz (marker: `closer.v1`) | +| 4b | The actual close on a clean kickoff | The intros `@mention` Fizz, waking it | Fizz, **LLM-generated** (no marker) | | 5 | Setup-mode nudge ("here's what you still need to configure") | Agent process spawns but its requirements check fails (e.g. missing API key) | The agent process itself (backend, buzz-acp setup-listener mode) | ## Silent paths (what the user CANNOT be told today) @@ -293,14 +297,18 @@ new work — which needs a judgment call the harness can't currently make. ### Verification status -- **Not verified live.** The prompt change is behavioral; only a real Codex - session on a fresh Welcome channel can confirm it. Prompt edits have no test - that can fail on a literal-minded model, so the desktop/Rust suites say - nothing about whether this works. -- One run on 2026-07-18 went from 21+ replies to **2** (the two intros, no - loop) — but that run had the opener waiver in it too, so it is **confounded - and does not count as evidence for the base prompt**. It is the reason the - opener was reverted: the next run needs to isolate the variable. +- **Verified live against Codex on 2026-07-18, with the base prompt as the only + variable** (the opener waiver was reverted first, precisely so this run would + isolate it). The chain terminated on its own: intros tagged `@Fizz` (the + legitimate `:44` callback), Fizz woke and replied once using plain narrative + names and **no tags**, so nobody woke and the chain ended at four replies. The + designed exit path is the one that fired. +- An earlier run went from 21+ replies to 2, but it had the opener waiver in it + and is **confounded** — it does not count as evidence for the base prompt. It + is the reason the opener was reverted. +- Prompt edits have no test that can fail on a literal-minded model, so the + desktop/Rust suites say nothing about whether this holds. One live run is + encouraging, not proof: n=1 on a sampled model. - Unit-tested only: the solo opener mentions no agent at all (pins the degraded path, which cannot loop by construction). @@ -326,6 +334,95 @@ new work — which needs a judgment call the harness can't currently make. off", which reads terminal — but the boundary is a model judgment, and reading it too broadly converts this loud failure into the silent kind. +## The closer was racing a message it could not beat + +Found in the same 2026-07-18 Codex run that cleared the intro loop. The user saw +Fizz say *"Honey and Bumble are taking longer than expected"* — and then both +intros landed one minute later, followed by Fizz closing again, properly. Two +CTAs, and the wrong one came first. + +Neither symptom was a failure state. They were **one design conflict**: + +1. **The closer raced the intros on a 15s stopwatch.** `TEAMMATE_INTRO_WAIT_MS` + was `15_000`, plus a `CLOSER_BEAT_MS` 3s beat — about 18s for a teammate to + have an intro *published*. That has to cover a whole agent turn: wake on the p + tag, fetch context, run inference, shell out to `buzz messages send`. Real + turns run tens of seconds, so the closer reliably lost and announced a delay + that wasn't real. PR #2066 tightened this beat, which made it more likely, not + less. +2. **The scripted closer could never be the last word.** The choreography assumed + it was. But the intros `@mention` Fizz — that is the *mandatory* callback in + `base_prompt.md:44`, since Fizz delegated the intros — so Fizz is **guaranteed** + to wake and reply after them. The choreography and the base prompt were + fighting: one scripted a final message, the other guaranteed a message after + it. + +Worth noting which one was better: Fizz's live reply ("bring us a project, bug, +question, or half-formed idea and I'll route it or start building") beat the +scripted CTA. It was contextual, and it was right about the state of the world. + +### The fix (landed 2026-07-18) + +**Let the guaranteed message be the close, and keep the script for problems only.** + +1. `buildWelcomeKickoffCloser` returns **`null`** for a clean kickoff — the + success state posts nothing. The failure variants (crashed teammate, slow + teammate) are unchanged and still carry the CTA, because there the user needs + both the bad news and a way forward. +2. The `null` is enforced inside `sendWelcomeKickoffCloser`, not at the call + sites. There are two callers, and the delayed-teammate timer can fire *after* + the intros land and re-classify the kickoff as clean — the choke point stops + that stray timer from emitting a bare CTA. +3. `TEAMMATE_INTRO_WAIT_MS` `15_000` → `60_000`, matching the presence wait. This + budget now only covers a teammate that is *alive but silent*, where patience is + the correct answer — a **crashed** teammate is read from agent status + (`failed`) and closes immediately, so real breakage is still reported fast. + +### The latch this broke, and why it needed a second signal + +The closer marker was doing quiet double duty: it was also the durable *"kickoff +already finished"* signal, in two places — retiring the opener-thread watch, and +the guard in the start effect that stops the choreography re-arming on revisit. +A marker only exists if a **message** exists (`markerExists` resolves it by +querying the relay for a tagged message), so "success posts nothing" silently +means "success never latches". Left alone that would have: + +- refetched the opener subtree forever on every Welcome revisit — the exact bug + PR #2066 fixed; and worse, +- **restarted the whole Welcome team every time the channel was opened**, because + the start effect's guard would never trip. + +So completion needs a second piece of evidence, and it has to be **relay-side** — +a local latch would re-run on another device or after a reinstall. +`welcomeKickoffAlreadyFinished` now reads: *closer marker exists* **OR** *the +opener's thread holds an intro from every teammate*. Both are durable server-side +facts. The marker still short-circuits first, so the failure paths and the solo +opener (which carries both markers on one message) are untouched and cost no +extra fetch. + +The in-memory latch stays a latch for the reason the original comment gives: the +evidence lives in the subtree that the latch itself gates, so deriving it fresh +each render is self-referential. + +### Residual risk + +- **The CTA is now model-generated on the happy path.** If Fizz wakes and says + something that doesn't invite the user in, the kickoff ends softer than the + script did. The mitigation is `base_prompt.md:44` making the wake itself + guaranteed — but *what Fizz says* is not. This is the deliberate bet of the + change, and the thing to watch across runs. +- **If Fizz doesn't reply at all, nothing closes.** The intros still landed, so + the channel is a usable welcome rather than an empty one — but there is no CTA + and no scripted backstop. A watchdog (wait N seconds for the lead's callback, + then post the scripted CTA) was considered and deliberately left out: it re-adds + the timer race this change removed. Revisit only if a run shows the lead going + quiet. +- Unit tests pin the contract (`buildWelcomeKickoffCloser([])` is `null`; the + finished-check reads the intros with no marker; a marker short-circuits without + a fetch; a missing intro is not finished). What they **cannot** pin is whether a + real Fizz reliably produces a good close — same limitation as the intro-loop + fix. + ## Constraints for the silent-path fix - **Fizz cannot be the messenger** for these paths: she is the thing that @@ -391,3 +488,28 @@ new work — which needs a judgment call the harness can't currently make. - The two failure classes interact: silent-path work adds messages, and every new message is a potential wake. Any fallback that `@mentions` an agent to recover from a failure can itself seed a loop. +- **Open thread panes can go stale while the reply count keeps climbing** — a + general bug, not a kickoff bug, but it shows up here because the intros are + thread replies. Observed repeatedly, including in the 2026-07-18 run. The cause + of the *asymmetry* is confirmed: the count and the list read from different + sources. The count is the relay's server-computed recount pushed as kind 39005 + (`hooks.ts`, `channelWindowStore.ts` `mergeLiveThreadSummary`) — authoritative + and independent of the reply event itself. The list is a client-maintained cache + (`["thread-replies", channelId, rootId]`) appended to by parsing live events. + One is robust, the other is fragile, so they can disagree. + + The specific trigger is **not** confirmed. Leading hypothesis: a refetch + overwrite race in `useThreadReplies.ts` — the query is `staleTime: 0`, and on + refetch it replaces the cache with the server's answer while preserving only + replies that arrived *during* the fetch (`receivedInFlight` is diffed against + `idsAtStart`). A live reply that landed just *before* a refetch, and that the + server's answer doesn't yet include, is dropped. Welcome refetches unusually + often because there are two observers on that cache entry (the pane's, plus + `welcomeKickoff.ts`'s opener-subtree watch). Fits every symptom, including why + close+reopen fixes it (the later fetch catches up). Ruled out for this case: + malformed `e` tags (the CLI/SDK emit proper NIP-10 markers) and root-key + divergence (the opener *is* the NIP-10 root). + + Note the live-append branch that feeds the pane has **no test coverage at all**, + which is why this can regress quietly. Needs a repro test before a fix — don't + fix on the hypothesis.