fix(desktop): resolve agent profiles through one archive-aware selector - #5706
fix(desktop): resolve agent profiles through one archive-aware selector#5706wpfleger96 wants to merge 14 commits into
Conversation
The profile panel's Instances list and primary managed-agent resolution read local managed-agents.json with no relay-archive join, so an archived agent still rendered as a live instance — and could win the nav click as the "primary" profile when its record sorted first in file order. Filter archived pubkeys through the existing useIsArchivedPredicate: skip them in the persona-fallback resolution and hide them from the instance list, falling back to the full set when every sibling is archived so a persona is never stranded. Fail-open while the relay snapshot loads. Stopgap: this whole surface is superseded by #centralize-personas-and-agents. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes Pfleger's behalf.
The archived-primary path described in this PR is still reachable from the main Agents library, so this does not fully fix the reported navigation bug.
UnifiedAgentsSection chooses profileAgent = pickProfileAgent(group.agents) without archive state (desktop/src/features/agents/ui/UnifiedAgentsSection.tsx:130-159). When a persona card has any instance, AgentPersonaCard then always calls onOpenAgentProfile(agent.pubkey) (:313-321). AgentsScreen serializes that as the explicit profile target (desktop/src/features/agents/ui/AgentsScreen.tsx:69-76). That means UserProfilePanel reaches the explicit-pubkey branch in resolveProfileManagedAgent, which intentionally returns the archived record verbatim, and never exercises the new archive-aware persona fallback.
Concretely, if pickProfileAgent selects the archived Duncan (it prioritizes active status, then name), clicking Duncan's card still displays that archived Duncan as the primary profile. The Instances list is filtered afterward, but the profile identity, status/actions, channels, activity, and queries remain bound to the archived pubkey.
Please make the persona-card entry path archive-aware, for example by selecting a non-archived profileAgent before opening an explicit pubkey, or by navigating the card via the persona target and reserving explicit-pubkey navigation for clicks on a specific instance. Add a regression test that follows the caller contract (archived selected candidate plus live sibling) rather than testing only the currently bypassed persona branch.
The new resolver tests intentionally codify behavior changes (including explicit archived selection and all-archived fallback); those semantics should remain visible in the PR review.
|
Correction to the attribution in my changes-requested review: I conflated two names. Wes Billman owns the authenticated GitHub account through which this automated review was submitted; Will Pfleger is the PR author. There is no “Wes Pfleger” involved. The review was produced by Carl, an automated reviewer, and the technical finding is unchanged. Sorry for the confusion. |
Persona resolution and the Instances list previously fell back to archived records when every sibling was archived, and the main Agents library card + standalone buckets were archive-blind — so an archived identity was still reachable as a primary profile from multiple nav paths. Persona resolution now returns undefined and Instances returns [] when all siblings are archived (the panel renders from the persona prop, the Instances section self-omits). pickProfileAgent filters archived candidates before sorting, so an all-archived group opens the persona profile instead of an archived record. buildUnifiedGroups drops archived agents from the ungrouped/unknown standalone buckets. Explicit-pubkey navigation still resolves an archived record verbatim, preserving the deliberate manage/unarchive access path. Fail-open on unloaded snapshot. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
changed the title
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-reviewed exact head 2e19767a88328ab66320b5580f3af626f0138f5d. My prior blocker is resolved.
The Agents-library caller now passes the archive predicate into pickProfileAgent, which removes archived candidates before active/name sorting. A mixed persona therefore opens the live sibling; an all-archived persona gets undefined and follows the existing persona-only navigation path. Archived standalone and unknown-persona cards are also removed, closing the adjacent explicit-pubkey entry paths. Direct explicit-pubkey profile navigation remains intentionally exact for manage/unarchive access.
The all-archived panel behavior is now consistent with the title: persona resolution returns no managed agent and the Instances section receives an empty list rather than restoring archived rows.
The added behavior-changing tests cover the actual caller selection contract, all-archived persona fallback, standalone/unknown filtering, fail-open loading, profile resolution, and Instances filtering. Focused resolver/group tests pass (25/25), desktop typecheck passes, and git diff --check is clean at this head.
I found no remaining material code issue. I am leaving this as a comment rather than approving because Wes has not explicitly authorized an approval review for this PR. Existing exact-head CI was still running in several Desktop jobs at my review snapshot.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes after a fresh lifecycle review. Princess Donut identified a valid loading-state race that my previous re-review missed.
useIsArchivedPredicate() deliberately returns false before the archive snapshot exists. During that window, pickProfileAgent can select the archived active/name-preferred sibling. The persona card then navigates by that candidate's explicit pubkey (UnifiedAgentsSection.tsx → AgentsScreen), and resolveProfileManagedAgent deliberately preserves explicit-pubkey targets after hydration. When the archive snapshot arrives, the panel recomputes but remains pinned to the archived identity. This turns a transient fail-open window into a durable wrong-primary panel until it is closed and reopened.
Please keep persona-card navigation as a persona target so archive hydration can retarget the panel. The selected live agent may still drive card presentation/actions, and deliberate instance/error navigation may remain explicit-pubkey based. Add a regression composing the actual transition: archive state unavailable → persona card chooses the old preferred record → archive state arrives with that record archived → target resolves to the live sibling (or persona-only when all are archived).
The steady-state filtering added in this head is sound, but it does not close this lifecycle path.
The persona card's main click serialized pickProfileAgent()'s pubkey into a durable profile=<pubkey> URL target. During the archive-query fail-open loading window pickProfileAgent can select an archived sibling, and resolveProfileManagedAgent preserves explicit-pubkey targets verbatim — so after hydration the open panel stayed stranded on the archived identity, reachable from the main Agents nav. The card now opens a persona target, which re-resolves every render and self-corrects to a live sibling (or persona-only when all siblings are archived). The persona-open callback carries the optional tab arg so the open-Runtime-tab-on-stopped-error behavior survives. Explicit-pubkey navigation (instance rows, the avatar error affordance, channel-members) is unchanged and still reaches archived profiles for manage/unarchive. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Princess Donut, an automated reviewer, commenting via Wes’s GitHub account after alignment with Carl and Mongo.
Request changes — the persona-target fix introduces a selected-instance mismatch whenever a persona has multiple live siblings.
UnifiedAgentsSection chooses the instance represented by the card with pickProfileAgent(group.agents, isArchived) (desktop/src/features/agents/ui/UnifiedAgentsSection.tsx:135-156). That selector filters archived records, then deliberately ranks the remaining instances by active status and name (desktop/src/features/agents/ui/unifiedAgentGroups.ts:51-63). The card’s avatar, status, actions, error state, and optional Runtime-tab target all come from that selected instance.
The main click now records only the persona (UnifiedAgentsSection.tsx:321-332). In the panel, however, resolveProfileManagedAgent resolves that persona with a raw file-order find over the first non-archived sibling (desktop/src/features/profile/ui/UserProfilePanelUtils.ts:181-184). It does not apply the card’s active/name policy.
Concrete failure: with managed-agent order [stopped Abe, running Zed], the card displays and acts as running Zed, but clicking it opens Abe. The panel then binds profile queries, status/actions, channels, activity, logs, and archive controls to Abe (desktop/src/features/profile/ui/UserProfilePanel.tsx:192-234). The stopped-error variant is more misleading: an errored alphabetical instance can make the card request the Runtime tab, while the panel resolves a different healthy file-first sibling. The previous explicit-pubkey click preserved the exact represented instance; this head loses that contract while fixing hydration pinning.
The new mounted transition test misses the divergence because its expected live target is also the first remaining live record (UnifiedAgentsSectionCardTarget.test.mjs:226-273). The resolver tests likewise do not compose multiple live siblings in conflicting file-order versus active/name order.
Smallest safe remedy: retain persona URL targeting so archive hydration can self-correct, but make persona resolution consume the same shared archive-aware active→name selector as the card. Do not duplicate the policy. Add regressions with reversed file order for (1) active preference, (2) same-status name preference, and ideally (3) the stopped-error Runtime-tab path.
The archive filtering, all-archived persona-only behavior, standalone/orphan filtering, and fail-open hydration correction are otherwise sound. Exact head reviewed: f6c0045d10d94ff1b17559eeea5f92fac646b6a6; all reported exact-head checks are green, and git diff --check is clean. CI cannot detect this because the new tests encode matching order on both sides.
…stances The profile panel resolved a persona's primary managed agent by raw file-order find of the first non-archived sibling, while the persona card chose its face via pickProfileAgent (archive-filter, then active status, then name). With two or more live siblings in conflicting orders the card displayed one instance but the click opened another, binding status, actions, logs, and channels to the wrong live instance. Persona resolution now reuses pickProfileAgent, so card and panel share one selection policy and always land on the same live instance; all-archived still resolves to undefined and renders persona-only. Archived siblings return to the Instances list under a labeled Archived subsection instead of being filtered out of existence. Archived rows keep their explicit-pubkey click, restoring a UI-reachable unarchive path for channel-less agents while main Agents navigation still cannot durably surface an archived identity. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
changed the title
…ep the Archived subsection resolvePersonaInstances derived its grouping key from the nullable primary managedAgent, which resolves to undefined when every sibling is archived — so an all-archived persona showed no Instances section and its channel-less identities stayed UI-unreachable for unarchive. Key the resolver by an explicit target persona ID (persona?.id ?? managedAgent?.personaId) so set membership is independent of primary-instance selection. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…test guards the caller The all-archived seam regression duplicated the panel's persona-ID fallback instead of exercising it, so a caller-only mutation could reintroduce the bug while every test stayed green. Extract resolveManagedProfileState — the single composition of primary selection and instance bucketing — have the panel consume it, and have the regression call that same function. The persona?.id fallback now has exactly one home, so mutating it turns the regression red. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The behavioral composition+section test fed ProfileInstancesSection directly,
so it could not catch UserProfilePanel dropping a composed bucket on the way
to the section — replacing archivedInstances={personaInstances.archived} with
={[]} killed the Archived subsection while the full suite stayed green. Add a
narrow source-contract guard pinning the panel's call to resolveManagedProfileState
and both bucket->prop mappings, and narrow the behavioral test's wording so it
no longer claims to cover that bridge.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…d instances The archived-instances bridge is a three-hop prop chain (UserProfilePanel -> ProfileSummaryView -> ProfileRuntimeTabContent -> ProfileInstancesSection). The prior contract pinned only hop 1, so a one-line drop at either downstream hop erased every Archived row while all tests stayed green. Pin both live and archived props at each hop and correct the header to describe the full chain it now guards. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ived instances Correct prop forwarding is only reachable if the visibility gates stay archived-aware. Dropping the archivedInstances clause from showRuntimeTab (ProfileSummaryView) or hasInstances (ProfileRuntimeTabContent) strands an all-archived persona while every test stayed green. Pin both clauses; the contract now covers the complete archived-aware expression set in the chain. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The round-8 showRuntimeTab pin used /[^;]*archivedInstances.length > 0/, which proves the text appears before the semicolon but not that the archived clause is still a live gate operand. An operator swap (|| -> && diagnosticsFields.length > 0), an inerting && false, or a commented-out clause all keep the text present while stranding all-archived personas — every one survived the old regex green. Replace both gate pins with a parser: strip comments, extract the named initializer up to its terminating semicolon, split the || chain, and require archivedInstances.length > 0 to survive as a whole operand. Operator swaps, inerting, deletion, and comment-hiding now fail; a legitimate operand reorder or parenthesisation stays green. The behavioral mount Thufir preferred is unreachable from a test-file-only change: importing either gate module transitively loads AgentSessionTranscriptList, which reads import.meta.env at module evaluation and throws under the node test harness. Shimming that needs the shared loader or that component, both outside this boundary. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The round-9 operand normalizer sliced from the last paren and split on every textual ||, so a clause trapped in a dead conjunction (false && (clause), (clause || false) && false) read as present while evaluating to a hidden UI for all-archived personas. Replace it with an evaluator that substitutes each gate leaf with its truth value for an all-archived owner-bot persona and evaluates under real JS precedence, asserting the user-visible property: the archived bucket alone shows the Runtime tab / Instances section. Syntactic reordering and whole-operand parens stay green; every precedence trap is now red. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The surrogate-language evaluator was not fail-closed: negated unknown leaves flipped to true, and the string-blind extractor truncated at a semicolon inside a string literal — both hid an all-archived persona while the contract stayed green. Extract each gate initializer's exact text via ts.createSourceFile and execute it under real precedence with concrete all-archived bindings; any unlisted identifier throws a ReferenceError and fails the test. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Replace the source-analysis proxy for the showRuntimeTab and hasInstances gates with rendered-behavior tests. Mounting ProfileSummaryView and ProfileRuntimeTabContent with a realistic all-archived owner-bot persona asserts the Runtime tab and Instances section actually render for one and for two archived rows and self-omit for none, so the guard no longer has a source-analysis-vs-rendered-behavior gap: decoy declarations, fixture shape, operand order, and precedence are all irrelevant when the assertion is the real UI. The mounts import the gate modules, which transitively read import.meta.env.VITE_* at module-evaluation time; the shared node test loader now shims import.meta.env to the vite build shape so any such module imports cleanly under the node harness. The six prop-handoff source pins stay unchanged. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…mounts The behavioral gate mounts held one real gate input constant in each component, so a gate conditioned on that input regressed the UI while every mounted case stayed green: - The summary mount fixed isSelf: true, but a real all-archived persona target has no primary managed agent, so production computes isSelf: false. A gate prefixed `isSelf && (...)` hid the Runtime tab for every persona target yet passed all mounts. - The runtime mount fixed currentPubkey: null everywhere, so a gate suffixed `(...) && currentPubkey === null` hid the Instances section on the explicit-pubkey path (the unarchive route) while passing. Model the persona-target shape (isSelf: false, pubkey: null — still light: !isSelf && pubkey is false, so no action objects are consumed) and add one runtime case on the explicit-pubkey path (currentPubkey set to an archived row). Both mutants, and the two applied together, now fail; the existing currentPubkey: null cases stay to keep both navigation shapes represented. Mount mechanism unchanged. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
2 participants
Relay-archived agent identities were still reachable as a primary profile from several paths in the desktop Agents surface, and the profile panel and persona card could disagree about which live instance a persona represents. The reported repros: an archived Duncan that sorted first in the local
managed-agents.jsonfile order won the nav click over the live Duncan; and with two or more live siblings the card displayed one instance (e.g. runningZed) while clicking it opened another (stoppedAbe), binding status, actions, logs, and channels to the wrong live instance.The root cause is that the profile panel and the Agents library read local managed-agent records with no join against the relay's archive snapshot, and resolved a persona's instance by two different policies. This wires the existing fail-open
useIsArchivedPredicate()(fromidentity-archive/hooks.ts) through every nav path that resolves a managed agent, and collapses the two selection policies into one.Changed resolver semantics
resolveProfileManagedAgent's pubkey branch is unchanged. This is the only way an archived identity surfaces as a primary profile, and only on an intentional click.resolveProfileManagedAgent's persona branch now callspickProfileAgent— the same archive-filter → active-status → name selector the persona card uses to choose its face — so the panel opens the exact live instance the card displays. Previously the panel used a raw file-orderfind, which could pick a different live sibling than the card; that divergence is gone. All siblings archived still resolves toundefined, and the panel renders from thepersonaprop.resolvePersonaInstancesreturns{ live, archived }. The primary Instances list is unchanged for live rows; archived siblings render below a labeled "Archived" subsection instead of being filtered out of existence. Archived rows keep the explicit-pubkey click, restoring a UI-reachable unarchive path for channel-less agents. The Instances section renders when live OR archived instances exist — an all-archived persona shows only the Archived subsection.useIsArchivedPredicatetreats every identity as live) could serialize an archived sibling's pubkey, and the explicit-pubkey branch preserves it verbatim — stranding the open panel on the archived identity after hydration. A persona target re-resolves every render through the persona branch above, so it self-corrects to a live sibling (or persona-only when all siblings are archived). The persona-open callback carries the optionaltabargument so the open-Runtime-tab-on-stopped-error behavior survives.buildUnifiedGroupsdrops archived agents from theungroupedandunknownbuckets, which have no persona fallback.Archived identities remain reachable only through an explicit pubkey click — the channel-members archived section, an instance row (including the new Archived subsection rows), and the card's avatar error affordance (open Runtime tab on a stopped, errored agent) — which is intentional. Main Agents navigation still cannot durably surface an archived identity.
Fail-open
While the relay archive snapshot is loading or errored, the predicate returns
false, so a cold start resolves exactly as it did before this filter existed and never hides an identity on missing data. The predicate also self-exempts the current user by construction.Scope
desktop/src/**plus one shared test-infra file:test-loader-hooks.mjs— the shared node test loader now shimsimport.meta.envto the vite build shape so a module that reads aVITE_*key at module-evaluation time imports cleanly under the node harness; this lets the profile-bridge contract test mount the two archived-visibility gates rather than analyze their source. Inert for the rest of the suite.features/profile/ui/UserProfilePanelUtils.ts—resolveProfileManagedAgent(now reusespickProfileAgent),resolvePersonaInstances(keyed by an explicit persona ID, returns{ live, archived }), andresolveManagedProfileState(the single composition of primary selection + instance bucketing the panel consumes, so an all-archived persona keeps its Archived bucket)features/profile/ui/ProfileInstancesSection.tsx— extracted Instances section with the labeled Archived subsectionfeatures/profile/ui/UserProfilePanel.tsx,features/profile/ui/UserProfilePanelSections.tsx,features/profile/ui/UserProfilePanelTabs.tsx— thread the archived bucket through to the sectionfeatures/agents/ui/unifiedAgentGroups.ts—pickProfileAgent,buildUnifiedGroupsfeatures/agents/ui/UnifiedAgentsSection.tsx— threads the predicate into both helpers; routes the persona card's main click through a persona targetfeatures/agents/ui/AgentsScreen.tsx,features/agents/ui/AgentsView.tsx,shared/context/ProfilePanelContext.tsx— carry the optionaltabargument through the persona-open callbackThis whole surface is a stopgap superseded later by #centralize-personas-and-agents.
Deferred coverage
A future E2E should assert that a stopped-error persona card produces
profilePersona=<id>&profileTab=runtimethroughAgentsScreen— the production types and direct source trace are verified correct today, but unit coverage does not exercise theAgentsViewoption-forwarding hop.