fix(fleet): accept an unnamed placement instead of killing the worker

src/fleet/relay-fleet-client.test.ts

@@ -325,13 +325,11 @@ describe('RelayFleetClient', () => {
     })
   })
 
-  it.each([
-    ['self', { node: 'self' }],
-    ['an empty node', { node: '' }],
-    ['an absent node', {}],
-  ])('fails closed when placement resolves to %s', async (_label, placement) => {
+  // `self` is a positive assertion that the work did NOT go remote, so it must
+  // still fail closed and tear the worker down.
+  it('fails closed when placement resolves to self', async () => {
     const messaging = new FakeMessaging()
-    messaging.placementAck = { placement }
+    messaging.placementAck = { placement: { node: 'self' } }
     const fleet = createClient(messaging)
 
     await expect(fleet.spawn({
@@ -353,6 +351,76 @@ describe('RelayFleetClient', () => {
     })
   })
 
+  // CONTRACT CHANGE, deliberate. These two cases previously shared the `self`
+  // assertion above and were expected to fail closed. That was correct while
+  // `placement.node` was REQUIRED: an absent name could only mean something had
+  // gone wrong. @agent-relay/sdk 11.8.5 (relay#1619) made it OPTIONAL and
+  // documents it as "absent when acknowledgment metadata is missing or not yet
+  // visible", so an unnamed placement is now an ordinary SUCCESSFUL spawn.
+  //
+  // Keeping the old expectation would make Factory release — i.e. kill — a
+  // worker it had just launched, every time the roster lagged. That is the
+  // production incident this change exists to prevent, so the guard must
+  // distinguish "no name yet" from "explicitly not remote".
+  it.each([
+    ['an empty node', { node: '' }],
+    ['an absent node', {}],
+  ])('accepts an unnamed placement (%s) without releasing the worker', async (_label, placement) => {
+    const messaging = new FakeMessaging()
+    messaging.placementAck = { placement }
+    const fleet = createClient(messaging)
+
+    const result = await fleet.spawn({
+      name: 'ar-1-impl',
+      capability: 'spawn:codex',
+      node: 'self',
+      repo: 'AgentWorkforce/factory',
+      task: 'do work',
+    })
+
+    expect(result.name).toBe('ar-1-impl')
+    // Accepted and tracked — the worker keeps running.
+    expect(fleet.trackedAgents().size).toBe(1)
+    // Never torn down.
+    expect(messaging.invokes).not.toContainEqual(
+      expect.objectContaining({ name: 'release' }),
+    )
+    // And no node name is invented for it: only the acknowledgement may name
+    // the node, and it did not.
+    expect(result.node).toBeUndefined()
+  })
+
+  it('does not inherit an action-output node when the acknowledgement is unnamed', async () => {
+    // The `self` guard exists because action output can name the node running
+    // the spawn handler. That output is equally untrustworthy when the
+    // acknowledgement is merely silent, so an unnamed placement must not quietly
+    // adopt it — otherwise "accepted but unidentified" would launder an
+    // unverified name into a trusted result.
+    const messaging = new FakeMessaging()
+    messaging.placementAck = { invocationId: 'unnamed-placement', status: 'pending', placement: {} }
+    messaging.invocations.set('unnamed-placement', [{
+      invocationId: 'unnamed-placement',
+      actionName: 'spawn',
+      status: 'completed',
+      output: { name: 'ar-1-impl', node: 'mac-mini' },
+    }])
+    const fleet = createClient(messaging)
+
+    const result = await fleet.spawn({
+      name: 'ar-1-impl',
+      capability: 'spawn:codex',
+      node: 'self',
+      repo: 'AgentWorkforce/factory',
+      task: 'do work',
+    })
+
+    expect(result.node).toBeUndefined()
+    expect(fleet.trackedAgents().get('ar-1-impl')?.node).toBeUndefined()
+    expect(messaging.invokes).not.toContainEqual(
+      expect.objectContaining({ name: 'release' }),
+    )
+  })
+
   it('rejects the acknowledgement node even when action output synthesizes a named node', async () => {
     const messaging = new FakeMessaging()
     messaging.placementAck = {
@@ -1097,6 +1165,62 @@ describe('RelayFleetClient', () => {
     expect(fleet.trackedAgents().has('ar-2-review')).toBe(true)
   })
 
+  // Guards the downstream half of the unnamed-placement change. A review raised
+  // the concern that a worker tracked WITHOUT a node would be skipped by the
+  // registration probe and eventually torn down. It is not: the probe keys on
+  // AGENT NAME (`onlineAgentNames.has(name)`), so an unnamed worker is checked
+  // exactly like a named one. The `!entry.node` skip bypasses only the
+  // node-offline check, which is meaningless without a node name and is safer
+  // skipped than guessed.
+  it('keeps reconciling a tracked worker that has no node, by name', async () => {
+    const messaging = new FakeMessaging()
+    // Online under its own name, and there is a live node it is NOT attributed to.
+    messaging.agentRows = [{ name: 'ar-1-impl', status: 'online' }]
+    messaging.nodeRows = [{ name: 'mac-mini', status: 'online', capabilities: [] }]
+    let nowMs = 1_000_000
+    const fleet = createClient(messaging, { now: () => nowMs })
+    const exits: Array<{ name: string; reason?: string }> = []
+    fleet.onAgentExit((name, reason) => exits.push({ name, reason }))
+
+    // An accepted placement whose acknowledgement carried no node name.
+    messaging.placementAck = { placement: {} }
+    const result = await fleet.spawn({ name: 'ar-1-impl', capability: 'spawn:codex' })
+    expect(result.node).toBeUndefined()
+    expect(fleet.trackedAgents().get('ar-1-impl')?.node).toBeUndefined()
+
+    // Well past the registration grace AND past the node-offline grace.
+    nowMs += 600_000
+    await fleet.reconcileTrackedAgents()
+    await fleet.reconcileTrackedAgents()
+
+    // Still tracked, never exited, never released. Being unnamed is not death.
+    expect(exits).toEqual([])
+    expect(fleet.trackedAgents().has('ar-1-impl')).toBe(true)
+    expect(messaging.invokes).not.toContainEqual(
+      expect.objectContaining({ name: 'release' }),
+    )
+  })
+
+  it('still exits an unnamed tracked worker when it actually leaves the roster', async () => {
+    // The complement of the test above: skipping the node check must not make
+    // an unnamed worker immortal. Name-based liveness still applies.
+    const messaging = new FakeMessaging()
+    messaging.agentRows = []
+    messaging.nodeRows = [{ name: 'mac-mini', status: 'online', capabilities: [] }]
+    let nowMs = 1_000_000
+    const fleet = createClient(messaging, { now: () => nowMs })
+    const exits: Array<{ name: string; reason?: string }> = []
+    fleet.onAgentExit((name, reason) => exits.push({ name, reason }))
+
+    messaging.placementAck = { placement: {} }
+    await fleet.spawn({ name: 'ar-1-impl', capability: 'spawn:codex' })
+
+    nowMs += 600_000
+    await fleet.reconcileTrackedAgents()
+
+    expect(exits).toEqual([{ name: 'ar-1-impl', reason: 'exited' }])
+  })
+
   it('synthesizes exits for offline roster rows and dead nodes after their grace windows', async () => {
     const messaging = new FakeMessaging()
     messaging.agentRows = [

src/fleet/relay-fleet-client.ts

@@ -373,14 +373,16 @@ export class RelayFleetClient implements FleetClient {
       throw error
     }
     const result = spawnResultFromInvocation(input.name, input.sessionRef, invocation, ack)
-    let acknowledgedNode: string
+    let acknowledgedNode: string | undefined
     try {
-      acknowledgedNode = assertNamedRemotePlacement(result, ack.placement?.node)
+      acknowledgedNode = assertRemotePlacement(result, ack.placement?.node)
     } catch (error) {
       // A completed placement invocation has already launched the worker. If
-      // Relay cannot prove that it ran on a named remote node, tear that worker
-      // down before refusing the result so a rejected spawn cannot keep acting
-      // outside Factory's lifecycle tracking.
+      // Relay positively acknowledged `self`, tear that worker down before
+      // refusing the result so a rejected spawn cannot keep acting outside
+      // Factory's lifecycle tracking. Reached only for `self` — an accepted
+      // placement whose node name is merely absent is NOT a failure and must
+      // never land here, or Factory would kill workers it just started.
       try {
         await this.release(result.name, 'unverified-placement')
       } catch (releaseError) {
@@ -398,8 +400,20 @@ export class RelayFleetClient implements FleetClient {
       }
       throw error
     }
-    const trustedResult = { ...result, node: acknowledgedNode }
-    this.#track(trustedResult.name, { invocationId: ack.invocationId, node: acknowledgedNode })
+    // Only the acknowledgement may name the node. `spawnResultFromInvocation`
+    // derives a node from action output, which is exactly the untrusted source
+    // the `self` guard above exists to defeat — so when the acknowledgement
+    // carries no name, strip it rather than inheriting that guess. An accepted
+    // placement with an unknown node is tracked without one; the roster
+    // reconciliation loop is what later attributes it.
+    const { node: _untrustedNode, ...resultWithoutNode } = result
+    const trustedResult: SpawnResult = acknowledgedNode
+      ? { ...result, node: acknowledgedNode }
+      : resultWithoutNode
+    this.#track(trustedResult.name, {
+      invocationId: ack.invocationId,
+      ...(acknowledgedNode ? { node: acknowledgedNode } : {}),
+    })
     return trustedResult
   }
 
@@ -469,7 +483,12 @@ export class RelayFleetClient implements FleetClient {
       log: this.#log,
     }))
     const invocation = await this.#awaitInvocation(ack.actionName || 'preview:tailscale-serve', ack, deadlineAtMs)
-    return previewReferenceFromInvocation(invocation, ack.placement?.node ?? ack.dispatchedNodeId)
+    // `dispatchedNodeId` is `string | null`, and `placement.node` became
+    // optional in @agent-relay/sdk 11.8.5, so this chain can yield `null`.
+    // Collapse it to `undefined` explicitly — the callee distinguishes only
+    // "have a node" from "do not", and a `null` masquerading as a value here
+    // is how an absent node turns into a bad preview reference.
+    return previewReferenceFromInvocation(invocation, ack.placement?.node ?? ack.dispatchedNodeId ?? undefined)
   }
 
   async removePreview(preview: PreviewReference): Promise<boolean> {
@@ -1690,12 +1709,31 @@ function spawnResultFromInvocation(
   }
 }
 
-function assertNamedRemotePlacement(result: SpawnResult, acknowledgedNode: string | undefined): string {
-  // The placement acknowledgement is authoritative. Action output may name
-  // the node executing the spawn handler even when Relay acknowledged `self`,
-  // so accepting the synthesized SpawnResult would let self-placement pass.
+/**
+ * Decide whether an accepted placement may be trusted as remote.
+ *
+ * Two outcomes that used to be one. `@agent-relay/sdk` 11.8.5 (relay#1619) made
+ * `placement.node` OPTIONAL, documenting it as "absent when acknowledgment
+ * metadata is missing or not yet visible". That was deliberate: an accepted
+ * placement must not become a failure merely because the roster could not be
+ * read. Before that change `placement.node` was required, so `!node` could only
+ * mean something was wrong; now it is an ordinary outcome of a SUCCESSFUL spawn.
+ *
+ * `'self'`, by contrast, is a positive assertion that the work did NOT go
+ * remote, and it must still be refused. Action output may name the node running
+ * the spawn handler even when Relay acknowledged `self`, so the acknowledgement
+ * — not the synthesized SpawnResult — remains the authority here.
+ *
+ * Returns the proven remote node name, or `undefined` when the placement was
+ * accepted but no node name is available yet. Throws only for `'self'`.
+ */
+function assertRemotePlacement(result: SpawnResult, acknowledgedNode: string | undefined): string | undefined {
   const node = acknowledgedNode?.trim()
-  if (!node || node === 'self') {
+  if (!node) {
+    // Accepted, but unidentified. The caller must not invent a name for it.
+    return undefined
+  }
+  if (node === 'self') {
     throw new Error(
       `Relay placement did not prove a named remote node for ${result.name}; ` +
       `refusing to accept the spawn result`,