agents: show draft cards only for drafts that exist on disk - #176
agents: show draft cards only for drafts that exist on disk#176morgmart wants to merge 6 commits into
Conversation
morgmart
changed the title
0467a75 to
85babd0
Compare
The Agents gallery used to build draft cards from open build-agent chat sessions plus an in-memory cache of draft metadata. When a draft file was moved or deleted out from under the app, the card stayed behind and Delete failed with `Source "…" not found`, leaving a card that could never be removed. Drafts are now read from disk like finished agents: `listAgentGallery()` splits a single `listAgentSources()` call into personas and drafts, the agent store keeps `draftSources`, and `AgentsView` renders a card per draft file, joining it to its builder session when one is still open. File gone -> card gone on the next refresh. Deleting a draft whose chat is gone discards the file directly instead of going through a session. Untouched drafts no longer pile up or prompt. `modelProviderId` is seeded on every new draft and was being counted as user content, so leaving a fresh "New agent" draft asked "Save this agent draft?" and kept an `untitled-agent-*.md` around. It is now exempt from the placeholder check, and the navigation guard silently discards a draft with no user content (navigating first so closing the empty chat does not redirect home). Editing an existing agent without changes still just navigates away. `findAgentBuilderSource` drops a cached draft from the in-memory cache when its file is no longer listed by the backend and cannot be read, so a missing file can't deadlock delete again. Also removes a duplicate `useVoiceConversationStore` import in the AppShell navigation test that was failing lint on main. Co-Authored-By: Claude <noreply@anthropic.com>
Navigation guard: the "is this draft untouched?" decision was made, then another lookup awaited, then the draft deleted — anything typed in that gap was discarded silently. `discardUntouchedDraftAgentSession` now owns re-check-then-discard: the user-content check is the last step before the file goes, and a draft that picked up content returns "kept" so the caller shows the save/discard prompt instead. Both the Back guard and the New agent button use it; `isDiscardableAgentBuilderSession` is folded in. Gallery refresh: `usePersonas` fenced stale disk listings behind a private mutation counter, but draft deletion in AgentsView and the promotion writes in AgentBuilderCapability bypassed it, so a focus/interval refresh that began before Delete could land afterwards and repaint the deleted card. The fence now lives in the agent store as `refreshGallery` / `mutateGallery`; every gallery writer goes through one of the two. Tests: deferred-lookup race for the guard (type while pending → kept, no delete), store fence semantics (stale snapshot dropped, in-flight mutation blocks apply, fence released on throw), and an AgentsView test where a refresh started before Delete resolves afterwards and the card stays gone. Co-Authored-By: Claude <noreply@anthropic.com>
…ion refresh Review of efb8993 found two gaps in the race fixes. The "final" user-content check still awaited a disk read after its in-memory look, so text typed during that read was invisible to it and the draft was still deleted. The in-memory look is now its own synchronous helper (`hasLocalAgentBuilderUserContent`) and `discardUntouchedDraftAgentSession` runs it once more with no await between it and the delete. Test holds the read inside the content check, types during it, and asserts "kept" with no delete/navigate/close; it fails without the re-check. `completeBuilder` started its disk refresh before the seeding mutation had released the gallery fence, so the fence (correctly) dropped every post-promotion refresh and the gallery stayed on the optimistic copy until the next timed refresh. The refresh now chains after the mutation. A capability test drives a real save through the real store and asserts the listing from disk is applied; the ChatRightRail store mock now models the fence instead of always applying, so it can no longer mask ordering bugs. Co-Authored-By: Claude <noreply@anthropic.com>
The New agent path discarded the untouched draft first and started the new builder afterwards, while the Back path navigated first. Both now use the helper's `onBeforeDiscard` transition, so the old chat is no longer the active session while its file is being deleted and closing it cannot redirect home. The helper test pins the order: navigate, delete, close. Co-Authored-By: Claude <noreply@anthropic.com>
…gallery a4dd9af passed `startBuilderSession` as the discard transition so the old chat would leave the screen before its file was deleted. Starting a builder is async (it resolves a provider/model before creating the chat), so the transition only began a navigation attempt and could not guarantee the new chat was active before the delete or close ran. This path is only reachable from the Agents view, where the old draft's editor is not on screen, so nothing is typed into it during the discard. Start the replacement after the untouched draft is discarded, as before. The helper's navigate -> delete -> close contract still holds for the Back guard, whose transition is a synchronous view change. Co-Authored-By: Claude <noreply@anthropic.com>
136575b to
88f9339
Compare
| // The check above awaited a disk read after its in-memory look. Anything | ||
| // typed during that read is invisible to it, so look once more — with no | ||
| // await between here and the delete. | ||
| if (hasLocalAgentBuilderUserContent(sessionId)) { |
There was a problem hiding this comment.
🤖 An edit can be in saveStatus === "saving" while localEditSessionIds is false. During that window the disk and local-source checks can still see the old placeholder, allowing navigation to delete the draft before its write lands. The in-flight save does not appear to be represented in the discard guard.
| * text, queued messages, sent messages. Synchronous on purpose — callers that | ||
| * are about to delete something re-run this with no await in between. | ||
| */ | ||
| export function hasLocalAgentBuilderUserContent(sessionId: string): boolean { |
There was a problem hiding this comment.
🤖 This content check handles queued text but not attachment-only composer or queue state. ChatInput accepts attachment-only messages, so navigation can classify that session as untouched, close it, and discard draftAttachmentsBySession[sessionId] or queued payload.attachments.
| key !== "draft" && | ||
| key !== "builderSessionId" && | ||
| key !== "provider" && | ||
| key !== "modelProviderId" && |
There was a problem hiding this comment.
🤖 These setup keys are ignored regardless of their values. A user who changes only the avatar, provider, or model can therefore have the persisted draft classified as an untouched placeholder, hidden from the gallery, and silently deleted on navigation. Setup-only edits are indistinguishable from the seeded values here.
| const source = await findCurrentBuilderSource(sessionId); | ||
| const isDraft = source === undefined || source.properties?.draft === true; | ||
|
|
||
| if (await hasAgentBuilderSessionUserContent(sessionId)) { |
There was a problem hiding this comment.
🤖 If the fresh source read fails while the backend still lists the file, the lookup can return stale placeholder metadata and this branch proceeds to deletion. A temporarily unreadable, user-modified draft can therefore be treated as confirmed empty.
| if (sessionId) { | ||
| await deleteDraftAgentSession(sessionId, { | ||
| closeSession: onDeleteDraftSession, | ||
| }); |
There was a problem hiding this comment.
🤖 If two draft files share a builderSessionId (for example, an external copy preserving frontmatter), both cards can bind to the same session. Deleting the non-target card then follows the session target and removes a different file than the selected source.path.
| } | ||
| } catch (error) { | ||
| console.warn("Failed to delete agent builder draft during discard:", error); | ||
| } finally { |
There was a problem hiding this comment.
🤖 A real deletion failure is swallowed here, but the finally block still clears builder state, closes the session, and returns "discarded". Because untouched placeholders are hidden from the gallery, the surviving file becomes an invisible orphan with no retry path, and a non-not-found error is indistinguishable from successful discard.
| // The draft just became this agent; drop its card without waiting | ||
| // for the disk refresh so the gallery never shows both at once. | ||
| for (const draft of current.draftSources) { | ||
| if (draft.properties?.builderSessionId === session.id) { |
There was a problem hiding this comment.
🤖 An orphan draft reopened without its original chat gets a new session ID while its frontmatter can retain the vanished ID. Promotion then misses the original draft in this loop, so the store temporarily retains both it and the promoted persona—and retains it indefinitely if refresh fails. The optimistic removal is keyed only by the current session ID rather than the promoted draft path.
| // The refresh must start after the mutation releases the fence, or the | ||
| // fence would (correctly) reject it as having begun mid-mutation. | ||
| void seeded | ||
| .then(() => agentStore.refreshGallery(listAgentGallery)) |
There was a problem hiding this comment.
🤖 promotePersonaSource can create the destination while failing to remove the original draft. This immediate refresh then lists and re-adds that orphan beside the promoted agent, restoring the duplicate card after the optimistic mutation removed it.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
Additional non-overlapping P1/P2 findings from review.
| closeSession: onDeleteDraftSession, | ||
| }); | ||
| } else { | ||
| await discardAgentBuilderSource(source.path); |
There was a problem hiding this comment.
P1 — Validate a sessionless card's target before deleting.
draftSources is a prior disk snapshot. If a sessionless draft at P.md is moved and a finished agent is created at P.md before the next refresh, this sends sources/delete for the replacement without rereading it or checking properties.draft. A stale Draft card can therefore delete a finished agent. Re-read and validate the selected source (ideally its identity/version too) immediately before deletion.
| @@ -143,12 +145,12 @@ export async function findAgentBuilderSource( | |||
| ); | |||
|
|
|||
| if (foundByPath && !isEmptyPlaceholderDraft(foundByPath)) { | |||
There was a problem hiding this comment.
P2 — Retry the backend-listed moved source after evicting a stale cache entry.
An edited draft keeps cached path A; after an external rename the backend lists B with the same builderSessionId. This branch selects stale A; its failed read evicts A and returns undefined without trying the already-computed movedNonPlaceholder (B). Delete then clears the session/card while B remains and reappears on refresh. Prefer the backend-listed match or retry it after eviction, with a test that primes the edited cache before renaming.
| (draft: GalleryDraft) => { | ||
| // Starting by path reopens the live builder chat when there is one and | ||
| // otherwise opens a fresh builder on the same file. | ||
| onStartAgentBuilderSession?.({ |
There was a problem hiding this comment.
P2 — Continue opens a second builder after a filename move.
The gallery has already identified the original live session as draft.sessionId, but Continue discards it and starts only by the new path/slug. The original session still has the old path, so findLiveBuilderSession() misses it and creates another builder for the same source; reconciliation later rebinds the original session too. Reuse/rebind draft.sessionId, or update the source's owner metadata when intentionally creating a replacement.
| console.warn("Failed to delete agent builder draft during discard:", error); | ||
| } finally { | ||
| clearBuilderSessionState(sessionId); | ||
| await deps.closeSession?.(sessionId); |
There was a problem hiding this comment.
P2 — Resolve the provisional session ID before closing.
A new builder begins under a client-side draft ID and is asynchronously promoted to the ACP ID. If the user leaves while creation is pending, this archives the captured provisional ID; after promotion no archive reaches the backend session, leaving the empty chat behind. Resolve the live builder ID immediately before close/archive (and handle promotion racing that resolution).
| return "nothing-to-discard"; | ||
| } | ||
|
|
||
| deps.onBeforeDiscard?.(); |
There was a problem hiding this comment.
P2 — Silent discard redirects Settings to Home.
Navigating first only works for routes that clear activeSessionId. openSettings() preserves it, so after this opens Settings the archive still sees the builder as active and unconditionally changes the view to Home. New agent → no edits → Settings briefly opens Settings, then lands on Home. Detach the builder before archival or make archival preserve an already-selected non-chat route.
| draftSources: state.draftSources.filter((draft) => draft.path !== path), | ||
| })), | ||
|
|
||
| refreshGallery: async (fetchGallery) => { |
There was a problem hiding this comment.
P2 — The gallery fence does not make disk snapshots latest-wins.
The revision changes only through mutateGallery(). Navigation-driven discards bypass it, so a listing that started before the delete can still apply afterward. Independently, two refreshes at the same revision can resolve out of order after an external file removal: a newer no-draft listing applies, then an older listing restores the card. Advance the fence for every draft lifecycle delete and track a latest-issued refresh generation so older snapshots cannot overwrite newer ones.
…etes found in PR review Untouched-draft detection: - The rail reports "saving" as a local edit; the write has not landed. - Any rail edit marks the session touched for its lifetime, so a draft whose only change was an avatar or model pick is kept, not discarded. - Attachment-only composer state and queued attachments count as content. - A listed-but-unreadable file is treated as having content, not as empty. - The close reaches the live backend session ID, not the provisional one. Deleting from the gallery: - discardAgentBuilderSource re-reads the path and refuses when it no longer holds a draft (AgentBuilderSourceNotDraftError); the card's delete is keyed by the card's path (deleteDraftAgentSource), never by the file its session resolves to. A stale card refreshes from disk instead of deleting a finished agent. Gallery fence: - Delete and promote run as gallery mutations inside the lifecycle helpers, so navigation-driven discards are fenced too. - refreshGallery is latest-wins: an older listing resolving after a newer one is dropped. - findAgentBuilderSource prefers the backend's moved file over a cache entry whose file is gone, and falls through after evicting one. Navigation: - Archiving the active chat only redirects Home when the chat view was showing; Settings and other surfaces stay put. Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
Category
Bug fix / UX behavior (Agents gallery, agent builder)
User Impact
Source "…" not found. The gallery now shows exactly the drafts that exist on disk — file gone, card gone on the next refresh.untitled-agent-*.mdfile (and an empty chat) behind. Untouched drafts are discarded silently.Editing an existing agent without changes still shows the save/discard modal — that is intentionally out of scope here and will be a separate PR.
Problem
Draft cards were built from open
build-agentchat sessions plus an in-memory cache of draft metadata (localDraftSourcesByPath). When the draft file disappeared out from under the app, the card survived on the cache, and Delete went through Goose'ssources/delete, which fails when the file can't be canonicalized. The lookup infindAgentBuilderSourcekept returning the cached entry, so the card could never be removed.Separately, every new draft is seeded with
modelProviderId, and the placeholder check treated that as user content. So a fresh "New agent" draft was never considered empty: leaving it prompted to save, and declining still left the file on disk. Over time this piled upuntitled-agent-*files — the exact files that become stuck cards when cleaned up by hand.Solution
Gallery drafts come from disk.
listAgentGallery()makes a singlelistAgentSources()call and splits it intopersonasanddraftsbyproperties.draft === true. The agent store gainsdraftSources;usePersonasandAgentBuilderCapability.refreshPersonaspopulate both lists from that one call.AgentsViewrenders one card per draft file (hiding untouched placeholders) and joins each to its builder session by path orbuilderSessionIdwhen one is still open.PersonaDraftCardrenders from the source entry rather than a session.Placeholder rule.
modelProviderIdis exempt fromisPlaceholderDraftForSession, so a draft with only seeded metadata counts as untouched.Silent discard of untouched drafts.
guardNavigationchecksisDiscardableAgentBuilderSession(draft or no file) when there's no user content, runs the navigation first, then discards the draft and closes its chat. Navigating first matters: closing the active chat redirects home, which would stomp on where the user was going. Existing-agent edits with no changes just navigate away.Lookup no longer deadlocks.
findAgentBuilderSourcecompares against the backend's listed paths; a cached draft that isn't listed and can't be read is dropped from the cache and treated as missing, so delete/discard paths can complete.Delete without a session.
handleDeleteDraftusesdeleteDraftAgentSessionwhen a chat exists, otherwisediscardAgentBuilderSource(path), then removes the draft from the store immediately.Also removes a duplicate
useVoiceConversationStoreimport inAppShell.navigation.test.tsxthat was failing lint onmain.Review follow-up: two timing races (second commit)
Review surfaced two gaps where the promises above could break under timing:
A draft could be deleted based on a stale "it's empty" check. The guard decided "untouched", awaited one more lookup, then deleted — anything typed in that gap was lost silently.
discardUntouchedDraftAgentSessionnow owns re-check-then-discard: the user-content check is the last step before the file goes, and a draft that picked up content returns"kept"so the save/discard prompt shows instead. Both the Back guard and the New agent button use it (isDiscardableAgentBuilderSessionfolded in).A slow gallery refresh could repaint a just-deleted card.
usePersonasalready fenced stale disk listings behind a mutation counter, but that fence was private to the hook; draft deletion inAgentsViewand promotion writes inAgentBuilderCapabilitybypassed it. The fence now lives in the agent store asrefreshGallery(fetch)/mutateGallery(work), and every gallery writer goes through one of the two. A refresh that started before a delete or promotion is dropped when it lands.Review follow-up 2 (third commit)
hasLocalAgentBuilderUserContent), re-run with no await between it and the delete. Test types during the held read inside the content check and asserts"kept".completeBuilderstarted its disk refresh before the seeding mutation released the fence, so the fence dropped every post-promotion refresh. The refresh now chains after the mutation; a capability test drives a real save through the real store and asserts the disk listing is applied. TheChatRightRailstore mock now models the fence instead of always applying.Review follow-up 3 (fourth and fifth commits)
start()is only reachable from the Agents view, so there is no active chat to redirect from; the reorder bought nothing and made the helper's order contract inconsistent with the Back guard. The fifth commit reverts it: discard first, then start.Testing
just checkpasses.pnpm vitest run: 576 files, 6822 passed, 1 skipped."kept", nothing deleted), store fence semantics (stale snapshot dropped, in-flight mutation blocks apply, fence released on throw), and anAgentsViewtest where a refresh started before Delete resolves afterwards and the card stays gone.just devwas done against the earlier version of this branch (moved a draft file out of~/.agents/agents, confirmed Delete worked). The reworked behavior (card disappears on refresh, untouched draft leaves no prompt/file) is covered by the tests above; a fresh manual pass is still worth doing before merge.File changes
src/shared/api/agents.ts—AgentGalleryListing,listAgentGallery(),refreshAgentGallery();listPersonas()delegates to the gallery call.src/features/agents/stores/agentStore.ts—draftSources,setDraftSources,removeDraftSource; gallery fence (galleryRevision,galleryMutationsInFlight,refreshGallery,mutateGallery).src/features/agents/hooks/usePersonas.ts— loads personas and drafts from one gallery fetch through the store fence (private mutation refs removed).src/features/agents/capabilities/AgentBuilderCapability.tsx—completeBuilderwrites and its follow-up refresh go through the store fence.src/features/agents/ui/PersonaGallery.tsx—GalleryDrafttype;PersonaDraftCardrenders from the source entry.src/features/agents/ui/AgentsView.tsx— drafts derived fromdraftSources, joined to sessions; continue/delete handlers work with or without a session; delete runs as a gallery mutation.src/features/agents/lib/agentBuilderIdentity.ts—modelProviderIdexempt from placeholder detection.src/features/agents/lib/agentBuilderSession.ts—discardUntouchedDraftAgentSession(re-check-then-discard).src/features/agents/lib/agentBuilderSourceLifecycle.ts—findAgentBuilderSourcedrops unlisted, unreadable cached drafts.src/features/agents/hooks/useAgentBuilderCoordinator.ts—guardNavigationandstartusediscardUntouchedDraftAgentSession.AppShell.navigation.test.tsx,usePersonas.test.ts,AgentBuilderCapability.test.tsx,AgentsView.entry.test.tsx,agentBuilderSession.test.ts,agentStore.test.ts,ChatRightRail.test.tsx.