mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(agent-status): close three resolver contract holes found in review
**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.
This commit is contained in:
@@ -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')
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<A extends string = TuiAgent> = {
|
||||
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<A extends string = TuiAgent> = {
|
||||
evidence: readonly PaneAgentEvidence<A>[]
|
||||
/** 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<A extends string = TuiAgent> = {
|
||||
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<A extends string = TuiAgent>(
|
||||
input: PaneAgentIdentityInput<A>
|
||||
): PaneAgentIdentity<A> {
|
||||
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 }
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user