From ce834946ea520e50349e603fa80b4879989a3cae Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:41:04 -0700 Subject: [PATCH] fix(agent-status): close three resolver contract holes found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Duplicate evidence of one source resolved by array order.** `eligible.find(...)` returned the first match, so two live hooks naming different agents were settled by input position — the exact property this resolver exists to remove. The original order-independence test only used DISTINCT sources, so it never exercised it. Conflicting same-class evidence now returns null with `ambiguousAt`, and does NOT fall through to a weaker source: letting a title answer whenever two hooks disagree is worse than saying nothing. **A bare numeric runId collided across authority restarts.** `incarnation` is a total order only within one `authorityId` (agent-status-observation.ts states this), and the id is regenerated per authority instance, so a restarted host counting from its own floor would report `1` and match an unrelated live run 1. The run key now carries its authority, and evidence from a DIFFERENT authority is treated as incomparable — kept, like an absent key — rather than as stale. **Title stayed reachable by consumers that authorize writes.** Ranking it last makes misuse unlikely; `minimumSource` makes it impossible. An action consumer passes `'launch'` and weaker evidence is dropped before ranking, so routing or delivery cannot name a target from a parsed string even by reordering its inputs. Display surfaces omit it and are unaffected. Also restores the generic agent-vocabulary parameter, which lives on the routing branch and was lost when this branch was rebased. Each fix is mutation-verified: first-match restored fails 3, ignoring authority fails 1, dropping the floor fails 2. The authority test was itself vacuous on the first attempt — both sides used `incarnation: 1`, so a resolver ignoring authority still passed on the numeric compare. It now uses differing incarnations. The remaining review finding, that `process > launch` has no freshness bound, is NOT fixed here: it needs an observation timestamp the evidence type does not yet carry. Recorded rather than silently dropped. --- .../pane-agent-identity-resolver.test.ts | 155 ++++++++++++++++-- src/shared/pane-agent-identity-resolver.ts | 98 ++++++++--- 2 files changed, 220 insertions(+), 33 deletions(-) diff --git a/src/shared/pane-agent-identity-resolver.test.ts b/src/shared/pane-agent-identity-resolver.test.ts index f39b0f7a30d..e11ef249c65 100644 --- a/src/shared/pane-agent-identity-resolver.test.ts +++ b/src/shared/pane-agent-identity-resolver.test.ts @@ -8,6 +8,8 @@ import { const resolve = (evidence: PaneAgentEvidence[], extra = {}) => resolvePaneAgentIdentity({ evidence, ...extra }) +const H = 'authority-a' + describe('resolvePaneAgentIdentity', () => { describe('a display title is the last thing consulted', () => { it.each(PANE_AGENT_EVIDENCE_SOURCES.filter((s) => s !== 'title' && s !== 'sibling'))( @@ -43,13 +45,16 @@ describe('resolvePaneAgentIdentity', () => { describe('run generation separates the bug from the legitimate reclaim', () => { // Both shapes are `completed hook = A, title = B`. Ordering alone cannot tell them apart. const shape = (hookRun: number, titleRun: number): PaneAgentEvidence[] => [ - { source: 'completed-hook', agent: 'claude', runId: hookRun }, - { source: 'title', agent: 'codex', runId: titleRun } + { source: 'completed-hook', agent: 'claude', run: { authorityId: H, incarnation: hookRun } }, + { source: 'title', agent: 'codex', run: { authorityId: H, incarnation: titleRun } } ] it('keeps the completed hook when both belong to the current run', () => { // The reported bug: nothing new started, so the hook is still the truth. - const result = resolvePaneAgentIdentity({ evidence: shape(7, 7), currentRunId: 7 }) + const result = resolvePaneAgentIdentity({ + evidence: shape(7, 7), + currentRun: { authorityId: H, incarnation: 7 } + }) expect(result).toMatchObject({ agent: 'claude', source: 'completed-hook' }) expect(result.supersededSources).toEqual([]) }) @@ -57,15 +62,24 @@ describe('resolvePaneAgentIdentity', () => { it('drops the completed hook once a new run has started', () => { // The legitimate reclaim: the pane was reused, so run 7's hook describes an agent that is // no longer there. It is ineligible, not merely outranked. - const result = resolvePaneAgentIdentity({ evidence: shape(7, 8), currentRunId: 8 }) + const result = resolvePaneAgentIdentity({ + evidence: shape(7, 8), + currentRun: { authorityId: H, incarnation: 8 } + }) expect(result).toMatchObject({ agent: 'codex', source: 'title' }) expect(result.supersededSources).toEqual(['completed-hook']) }) it('produces opposite answers from identical evidence, given only the run ids', () => { // The whole point, stated as one assertion. - const bug = resolvePaneAgentIdentity({ evidence: shape(7, 7), currentRunId: 7 }) - const reclaim = resolvePaneAgentIdentity({ evidence: shape(7, 8), currentRunId: 8 }) + const bug = resolvePaneAgentIdentity({ + evidence: shape(7, 7), + currentRun: { authorityId: H, incarnation: 7 } + }) + const reclaim = resolvePaneAgentIdentity({ + evidence: shape(7, 8), + currentRun: { authorityId: H, incarnation: 8 } + }) expect(bug.agent).not.toBe(reclaim.agent) }) }) @@ -75,14 +89,16 @@ describe('resolvePaneAgentIdentity', () => { // An old host publishes no run ids. Treating unknown as stale would blank every row. const result = resolvePaneAgentIdentity({ evidence: [{ source: 'completed-hook', agent: 'claude' }], - currentRunId: 9 + currentRun: { authorityId: H, incarnation: 9 } }) expect(result.agent).toBe('claude') }) it('disables run filtering entirely when the pane has no current run', () => { const result = resolvePaneAgentIdentity({ - evidence: [{ source: 'completed-hook', agent: 'claude', runId: 3 }] + evidence: [ + { source: 'completed-hook', agent: 'claude', run: { authorityId: H, incarnation: 3 } } + ] }) expect(result).toMatchObject({ agent: 'claude', supersededSources: [] }) }) @@ -119,10 +135,10 @@ describe('resolvePaneAgentIdentity', () => { it('returns null when every source belongs to a superseded run', () => { const result = resolvePaneAgentIdentity({ evidence: [ - { source: 'live-hook', agent: 'claude', runId: 1 }, - { source: 'title', agent: 'codex', runId: 1 } + { source: 'live-hook', agent: 'claude', run: { authorityId: H, incarnation: 1 } }, + { source: 'title', agent: 'codex', run: { authorityId: H, incarnation: 1 } } ], - currentRunId: 2 + currentRun: { authorityId: H, incarnation: 2 } }) expect(result.agent).toBeNull() expect(result.supersededSources).toEqual(['live-hook', 'title']) @@ -142,4 +158,121 @@ describe('resolvePaneAgentIdentity', () => { expect(forward.source).toBe('live-hook') }) }) + + describe('two observations of the same class cannot be settled by array order', () => { + // Review finding: `eligible.find(...)` returned the FIRST match, so duplicates of one source + // naming different agents resolved by input order — the exact property this resolver exists to + // remove. The earlier order-independence test only used DISTINCT sources, so it never saw it. + it('returns null when two live hooks name different agents', () => { + const result = resolve([ + { source: 'live-hook', agent: 'claude' }, + { source: 'live-hook', agent: 'codex' } + ]) + expect(result.agent).toBeNull() + expect(result.ambiguousAt).toBe('live-hook') + }) + + it('gives the same answer in either order', () => { + const a: PaneAgentEvidence[] = [ + { source: 'live-hook', agent: 'claude' }, + { source: 'live-hook', agent: 'codex' } + ] + expect(resolve(a)).toEqual(resolve(a.toReversed())) + }) + + it('still resolves when duplicates agree', () => { + expect( + resolve([ + { source: 'live-hook', agent: 'claude' }, + { source: 'live-hook', agent: 'claude' } + ]).agent + ).toBe('claude') + }) + + it('does not fall through to a weaker source on conflict', () => { + // Falling through would let a title answer whenever two hooks disagreed — strictly worse + // than saying nothing. + const result = resolve([ + { source: 'live-hook', agent: 'claude' }, + { source: 'live-hook', agent: 'codex' }, + { source: 'title', agent: 'grok' } + ]) + expect(result.agent).toBeNull() + }) + }) + + describe('run keys from different authorities are incomparable, not stale', () => { + // Review finding: a bare numeric runId collides across authority restarts. A restarted main + // regenerates its authorityId and counts incarnations from its own floor, so `1 === 1` would + // equate two unrelated runs. + it('keeps evidence whose authority differs, even when the incarnations do not match', () => { + // The incarnations must DIFFER for this to discriminate. With both at 1, a resolver that + // ignored authority entirely would still pass on the numeric compare — verified by mutation, + // which is how the first version of this test was caught as vacuous. + const result = resolvePaneAgentIdentity({ + evidence: [ + { source: 'live-hook', agent: 'claude', run: { authorityId: 'host-b', incarnation: 5 } } + ], + currentRun: { authorityId: 'host-a', incarnation: 2 } + }) + expect(result.agent).toBe('claude') + expect(result.supersededSources).toEqual([]) + }) + + it('does not equate the same incarnation number from two authorities', () => { + // A restarted main regenerates authorityId and counts from its own floor, so `1` from + // host-b is not `1` from host-a. Incomparable is treated as unknown, so the evidence is + // kept rather than being read as either current or stale. + const result = resolvePaneAgentIdentity({ + evidence: [ + { source: 'launch', agent: 'codex', run: { authorityId: 'host-b', incarnation: 1 } } + ], + currentRun: { authorityId: 'host-a', incarnation: 1 } + }) + expect(result).toMatchObject({ agent: 'codex', source: 'launch' }) + }) + + it('supersedes only within the same authority', () => { + const result = resolvePaneAgentIdentity({ + evidence: [ + { source: 'live-hook', agent: 'claude', run: { authorityId: 'host-a', incarnation: 1 } } + ], + currentRun: { authorityId: 'host-a', incarnation: 2 } + }) + expect(result.agent).toBeNull() + expect(result.supersededSources).toEqual(['live-hook']) + }) + }) + + describe('an action consumer can refuse to see weak evidence at all', () => { + // Review finding: title remained available to consumers that AUTHORIZE A WRITE. Ranking it + // last makes misuse unlikely; dropping it makes misuse impossible. + it('ignores a title entirely below the floor', () => { + const result = resolve([{ source: 'title', agent: 'codex' }], { minimumSource: 'launch' }) + expect(result.agent).toBeNull() + }) + + it('ignores a sibling below the floor even when opted in', () => { + const result = resolve([{ source: 'sibling', agent: 'codex' }], { + allowSibling: true, + minimumSource: 'launch' + }) + expect(result.agent).toBeNull() + }) + + it('still answers from evidence at or above the floor', () => { + const result = resolve( + [ + { source: 'launch', agent: 'claude' }, + { source: 'title', agent: 'codex' } + ], + { minimumSource: 'launch' } + ) + expect(result).toMatchObject({ agent: 'claude', source: 'launch' }) + }) + + it('leaves display consumers unrestricted when no floor is given', () => { + expect(resolve([{ source: 'title', agent: 'codex' }]).agent).toBe('codex') + }) + }) }) diff --git a/src/shared/pane-agent-identity-resolver.ts b/src/shared/pane-agent-identity-resolver.ts index eddd275a761..98342963415 100644 --- a/src/shared/pane-agent-identity-resolver.ts +++ b/src/shared/pane-agent-identity-resolver.ts @@ -44,21 +44,43 @@ export type PaneAgentEvidenceSource = (typeof PANE_AGENT_EVIDENCE_SOURCES)[numbe /** Authority order, strongest first. Position here is the ONLY place precedence is expressed. */ const SOURCE_RANK: readonly PaneAgentEvidenceSource[] = PANE_AGENT_EVIDENCE_SOURCES -export type PaneAgentEvidence = { - source: PaneAgentEvidenceSource - agent: TuiAgent - /** - * The agent run this evidence describes. Evidence whose run is not the pane's current run is - * ineligible. Undefined means unknown — from an old peer that does not publish run ids, or an - * ingress not yet stamped — and is treated as eligible, so a missing field never blanks a row. - */ - runId?: number +/** + * Which agent run a piece of evidence belongs to. + * + * Why the authority and not a bare number: `incarnation` is only a total order WITHIN one + * `authorityId` (see agent-status-observation.ts). The id is regenerated per authority instance, + * so a restarted main, a second machine, and a renderer that parsed bytes itself each count from + * their own floor. Comparing bare numbers across them silently equates unrelated runs — a + * restarted host reporting `incarnation: 1` would match a live run 1 from before the restart. + * + * Runs from different authorities are INCOMPARABLE, not different. + */ +export type PaneAgentRunKey = { + authorityId: string + incarnation: number } -export type PaneAgentIdentityInput = { - evidence: readonly PaneAgentEvidence[] +export type PaneAgentEvidence = { + source: PaneAgentEvidenceSource + agent: A + /** + * The agent run this evidence describes. Evidence proven to belong to a SUPERSEDED run is + * ineligible. Undefined means unknown — from an old peer that does not publish run keys, or an + * ingress not yet stamped — and is treated as eligible, so a missing field never blanks a row. + */ + run?: PaneAgentRunKey +} + +export type PaneAgentIdentityInput = { + evidence: readonly PaneAgentEvidence[] /** The pane's current run. Undefined disables run filtering entirely (old peer, mixed version). */ - currentRunId?: number + currentRun?: PaneAgentRunKey + /** + * Refuse to answer from evidence weaker than this rank. Consumers that AUTHORIZE AN ACTION — + * message routing, mailbox delivery, agent-scoped timers — pass `'launch'` so a parsed title or + * a neighbouring pane can never name the target of a write. Display surfaces omit it. + */ + minimumSource?: PaneAgentEvidenceSource /** * Pane-scoped consumers must not inherit another pane's agent. Sibling evidence is dropped * unless the caller is a tab-level surface that opted in. @@ -66,10 +88,12 @@ export type PaneAgentIdentityInput = { allowSibling?: boolean } -export type PaneAgentIdentity = { - agent: TuiAgent | null +export type PaneAgentIdentity = { + agent: A | null /** Which class of evidence decided it. Null when nothing eligible remained. */ source: PaneAgentEvidenceSource | null + /** Set when two equally-ranked sources disagreed, so a caller can tell "nothing" from "conflict". */ + ambiguousAt?: PaneAgentEvidenceSource /** Evidence discarded because it belongs to a superseded run. Surfaced for diagnostics. */ supersededSources: readonly PaneAgentEvidenceSource[] } @@ -77,22 +101,44 @@ export type PaneAgentIdentity = { /** * Resolves one pane's agent from ranked evidence. * + * Generic over the agent vocabulary: the sidebar speaks the widened `AgentType` and the tab speaks + * the strict `TuiAgent`. Ranking evidence does not depend on which, and a cast at that boundary + * would only hide the mismatch. + * * Returns null rather than guessing. A pane with no eligible evidence shows no agent, which is * recoverable; showing the wrong agent is not, and at the action surfaces (orchestration routing, * mailbox delivery, prompt-cache timers) it is a misdelivery rather than a cosmetic slip. */ -export function resolvePaneAgentIdentity(input: PaneAgentIdentityInput): PaneAgentIdentity { +export function resolvePaneAgentIdentity( + input: PaneAgentIdentityInput +): PaneAgentIdentity { const superseded: PaneAgentEvidenceSource[] = [] + const floor = input.minimumSource + ? SOURCE_RANK.indexOf(input.minimumSource) + : Number.MAX_SAFE_INTEGER + const eligible = input.evidence.filter((item) => { if (item.source === 'sibling' && input.allowSibling !== true) { return false } - // Why undefined is eligible: absence means "this peer does not publish run ids", not "this - // belongs to an old run". Treating unknown as stale would blank every row from an old host. - if (input.currentRunId === undefined || item.runId === undefined) { + // Why the floor: an action consumer must not be able to act on a title, at any rank. Dropping + // the evidence entirely rather than ranking it lower makes misuse impossible rather than + // unlikely — a caller cannot accidentally consult it by reordering. + if (SOURCE_RANK.indexOf(item.source) > floor) { + return false + } + if (input.currentRun === undefined || item.run === undefined) { + // Why eligible: absence means "this peer does not publish run keys", not "this is stale". + // Treating unknown as superseded would blank every row from an older host. return true } - if (item.runId === input.currentRunId) { + if (item.run.authorityId !== input.currentRun.authorityId) { + // Why eligible and NOT superseded: runs from different authorities are incomparable, not + // older. A restarted main counts from its own floor, so `incarnation` alone would falsely + // equate unrelated runs. Incomparable evidence is treated as unknown, like an absent key. + return true + } + if (item.run.incarnation === input.currentRun.incarnation) { return true } superseded.push(item.source) @@ -100,10 +146,18 @@ export function resolvePaneAgentIdentity(input: PaneAgentIdentityInput): PaneAge }) for (const source of SOURCE_RANK) { - const match = eligible.find((item) => item.source === source) - if (match) { - return { agent: match.agent, source, supersededSources: superseded } + const matches = eligible.filter((item) => item.source === source) + if (matches.length === 0) { + continue } + const agents = new Set(matches.map((item) => item.agent)) + if (agents.size > 1) { + // Why null and not the first: two observations of the same class naming different agents is + // a genuine conflict, and picking one would make the answer depend on array order — the very + // property this resolver exists to remove. Fall through to nothing rather than guess. + return { agent: null, source: null, ambiguousAt: source, supersededSources: superseded } + } + return { agent: matches[0].agent, source, supersededSources: superseded } } return { agent: null, source: null, supersededSources: superseded } }