fix(factory): skip the orphan-recovery context on a dry-run sweep (#348)

src/orchestrator/factory.test.ts

@@ -5033,6 +5033,147 @@ describe('FactoryLoop', () => {
     }
   })
 
+  // factory#348. `#343` gave read-only CLI commands a fleet client that refuses
+  // to mint a workspace identity, and `#githubOrphanRecoveryContext` read
+  // `fleet.roster()` regardless of `dryRun` — so a GitHub-backed
+  // `run-once --dry-run` (the container's start-disabled pre-cutover gate) hit
+  // the refusal and logged a safety-context failure on every sweep. The context
+  // was provably unused on that path: `mayRecoverGithubOrphan` is `!dryRun` and
+  // `#reconcileOrphanedGithubInProgress` refuses under `dryRun` before reading
+  // it. Deciding what a sweep WOULD do must not need an identity to do it with.
+  describe('orphan-recovery safety context under a read-only fleet client', () => {
+    // Counts the roster read without changing its answer, so the must-not-fire
+    // arm below measures the guard and not a refusal.
+    class RosterCountingFleet extends FakeFleetClient {
+      rosterCalls = 0
+
+      override async roster(): ReturnType<FakeFleetClient['roster']> {
+        this.rosterCalls += 1
+        return await super.roster()
+      }
+    }
+
+    // The production shape of #343's refusal: fleet reads are fine once an
+    // identity exists, but minting one is refused — and `roster()` mints on
+    // demand.
+    class ReadOnlyRosterFleet extends FakeFleetClient {
+      rosterCalls = 0
+
+      override async roster(): Promise<never> {
+        this.rosterCalls += 1
+        throw new Error(
+          'Refusing to register relay agent "factory-cloud-1c88bf23": this fleet client is read-only.',
+        )
+      }
+    }
+
+    // One orphan-shaped issue: `factory:in-progress`, no live agent, a stranded
+    // in-flight dispatch row. Live, this is exactly what orphan recovery exists
+    // to release; dry, it is what gets preserved.
+    const orphanFixture = async (): Promise<{
+      root: string
+      mount: FakeMountClient
+      stateStore: InMemoryStateStore
+    }> => {
+      const root = await mkdtemp(join(tmpdir(), 'factory-348-dryrun-'))
+      const path = githubIssuePath('AgentWorkforce', 'pear', 348)
+      const payload = githubIssueFile(348, { labels: ['factory', 'pear', 'factory:in-progress'] })
+      const mount = new FakeMountClient({ [path]: payload })
+      mount.setSubRoot('/linear/issues', 'absent')
+      const stateStore = new InMemoryStateStore({ batchSize: 4 })
+      const issue = parseGithubFactoryIssue(path, payload)
+      await stateStore.recordDispatchAttempt('factory-test', issueKey(issue), {
+        attempts: 1,
+        inFlight: true,
+        terminal: false,
+        backoffUntilMs: 0,
+      })
+      return { root, mount, stateStore }
+    }
+
+    const factoryFor = (
+      root: string,
+      mount: FakeMountClient,
+      fleet: FakeFleetClient,
+      stateStore: InMemoryStateStore,
+    ): ReturnType<typeof createFactory> =>
+      createFactory(config({
+        issueSource: 'github',
+        loop: { registryPath: join(root, 'registry.json') },
+      }), {
+        mount,
+        fleet,
+        stateStore,
+        triage: new StaticTriage(),
+        githubWriteback: new RecordingGithubWriteback(),
+        probePrGhRunner: async () => ({ stdout: '[]' }),
+      })
+
+    // MUST NOT FIRE. The roster read is skipped because the sweep is dry, not
+    // because the client refused it: this fleet answers `roster()` normally.
+    it('does not read the roster on a dry-run sweep', async () => {
+      const { root, mount, stateStore } = await orphanFixture()
+      const fleet = new RosterCountingFleet()
+      try {
+        const factory = factoryFor(root, mount, fleet, stateStore)
+
+        const report = await factory.runOnce({ dryRun: true })
+
+        expect(fleet.rosterCalls).toBe(0)
+        expect(report.orphanRecoveryDegraded).toBe('dry-run')
+        // A deliberate skip, not a failure: the live-path failure counter must
+        // stay clear so a real degradation is still distinguishable from this.
+        expect(factory.status().counters.githubOrphanRecoveryContextFailures).toBeUndefined()
+        expect(factory.status().counters.githubOrphanRecoveryContextSkippedDryRun).toBe(1)
+      } finally {
+        await rm(root, { recursive: true, force: true })
+      }
+    })
+
+    // MUST FIRE, and the CONTROL for the arm above. Same fixture, same fleet
+    // class, same issue — only `dryRun` differs. Without it, the zero above
+    // could mean the fixture never reaches the orphan-recovery call site at
+    // all, and would prove nothing. Swapping the two arms fails both: this one
+    // would stop recovering, that one would start reading the roster.
+    it('CONTROL: still reads the roster and recovers the orphan on a live sweep', async () => {
+      const { root, mount, stateStore } = await orphanFixture()
+      const fleet = new RosterCountingFleet()
+      try {
+        const factory = factoryFor(root, mount, fleet, stateStore)
+
+        const report = await factory.runOnce()
+
+        expect(fleet.rosterCalls).toBeGreaterThan(0)
+        expect(report.orphanRecoveryDegraded).toBeUndefined()
+        expect(report.dispatched.map((result) => result.issue.key)).toEqual(['348'])
+        expect(factory.status().counters.githubOrphanedInProgressRecovered).toBe(1)
+        expect(factory.status().counters.githubOrphanRecoveryContextSkippedDryRun).toBeUndefined()
+      } finally {
+        await rm(root, { recursive: true, force: true })
+      }
+    })
+
+    // The regression itself, in its production shape: the container's
+    // start-disabled `run-once --dry-run` gate, whose fleet client refuses to
+    // mint an identity. Before the guard this logged a safety-context failure
+    // every sweep; now the sweep never asks.
+    it('completes a dry-run sweep against a client that refuses to mint an identity', async () => {
+      const { root, mount, stateStore } = await orphanFixture()
+      const fleet = new ReadOnlyRosterFleet()
+      try {
+        const factory = factoryFor(root, mount, fleet, stateStore)
+
+        const report = await factory.runOnce({ dryRun: true })
+
+        expect(fleet.rosterCalls).toBe(0)
+        expect(report.orphanRecoveryDegraded).toBe('dry-run')
+        expect(factory.status().counters.githubOrphanRecoveryContextFailures).toBeUndefined()
+      } finally {
+        await rm(root, { recursive: true, force: true })
+      }
+    })
+  })
+
   // Factory admits an issue to dispatch on EITHER the title prefix or the
   // scope label, but orphan recovery demanded the label alone. An issue
   // admitted by title could therefore be dispatched and then never un-stuck.

src/orchestrator/factory.ts

@@ -2862,7 +2862,13 @@ export class FactoryLoop implements Factory {
       }
       const paths = await this.#readyIssuePaths()
       const orphanRecovery = issueSource === 'github'
-        ? await this.#githubOrphanRecoveryContext()
+        ? await this.#githubOrphanRecoveryContext(dryRun)
+        : undefined
+      // Preserved-not-reconciled is a fact the caller needs either way, but the
+      // two causes are not the same event: a dry run skips the context by
+      // design, a live sweep that has none failed to build one.
+      const orphanRecoveryDegraded = issueSource === 'github' && orphanRecovery === undefined
+        ? (dryRun ? 'dry-run' as const : 'context-unavailable' as const)
         : undefined
       const pulled: IssueRef[] = []
       const triaged: TriageDecision[] = []
@@ -3124,7 +3130,15 @@ export class FactoryLoop implements Factory {
         }
       }
 
-      report = { pulled, triaged, dispatched, skipped, dryRun, slackDegraded: this.#slackDegraded }
+      report = {
+        pulled,
+        triaged,
+        dispatched,
+        skipped,
+        dryRun,
+        slackDegraded: this.#slackDegraded,
+        ...(orphanRecoveryDegraded ? { orphanRecoveryDegraded } : {}),
+      }
       return report
     } catch (error) {
       this.#logger.warn?.('[factory] run-once failed', {
@@ -3143,6 +3157,7 @@ export class FactoryLoop implements Factory {
           dispatched: report.dispatched.length,
           skipped: report.skipped.length,
           slackDegraded: report.slackDegraded ?? false,
+          orphanRecoveryDegraded: report.orphanRecoveryDegraded,
           relayfileWaitWarnings: (this.#counters.relayfileOperationWaitWarnings ?? 0) - relayfileWaitWarningsAtStart,
           relayfileSlowOperations: (this.#counters.relayfileSlowOperations ?? 0) - relayfileSlowOperationsAtStart,
           relayfileOperationFailures: (this.#counters.relayfileOperationFailures ?? 0) - relayfileOperationFailuresAtStart,
@@ -3516,7 +3531,34 @@ export class FactoryLoop implements Factory {
     }
   }
 
-  async #githubOrphanRecoveryContext(): Promise<GithubOrphanRecoveryContext | undefined> {
+  /**
+   * The safety context orphan recovery needs before it may release a leaked
+   * `factory:in-progress` claim: who is online, which issues are actively
+   * owned, and which durable lifecycle rows look abandoned.
+   *
+   * A dry run never builds it. `#reconcileOrphanedGithubInProgress` refuses to
+   * release a claim under `dryRun` before it so much as looks at the context,
+   * and `mayRecoverGithubOrphan` in `#performRunOnce` is itself `!dryRun` —
+   * so the context was already provably unused on that path. Gathering it
+   * anyway was not free: `fleet.roster()` mints this process's workspace
+   * identity on demand, which is exactly what a read-only client refuses
+   * (#343) and exactly what a dry run must not need. Deciding what a sweep
+   * WOULD do must not require an identity to do it with.
+   *
+   * Returning `undefined` is therefore the honest answer for a dry run, not a
+   * failure: the caller reports the sweep as `orphanRecoveryDegraded:
+   * 'dry-run'` and preserves in-progress issues, which is what a dry run does
+   * regardless. The failure counter and the warn below stay for the live path,
+   * where an absent context IS a degradation.
+   */
+  async #githubOrphanRecoveryContext(dryRun: boolean): Promise<GithubOrphanRecoveryContext | undefined> {
+    if (dryRun) {
+      this.#increment('githubOrphanRecoveryContextSkippedDryRun')
+      this.#logger.info?.('[factory] dry run skipped the orphan-recovery safety context; in-progress issues are preserved', {
+        reason: 'a dry run never releases an in-progress claim, so it needs no workspace identity to build one',
+      })
+      return undefined
+    }
     try {
       const [registry, roster, lifecycles, waitingClarifications] = await Promise.all([
         readFactoryInFlightRegistry(this.#config.loop.registryPath),

src/types.ts

@@ -532,6 +532,17 @@ export interface IterationReport {
   skipped: Array<{ issue: IssueRef; reason: string }>
   dryRun: boolean
   slackDegraded?: boolean
+  /**
+   * Orphan recovery did not run for this sweep, so `factory:in-progress`
+   * claims were preserved rather than reconciled.
+   *
+   * `dry-run` is expected and benign: a dry run never releases a claim, so it
+   * deliberately skips building the safety context — which on a read-only
+   * fleet client would mean minting a workspace identity just to decide what
+   * the sweep WOULD do. `context-unavailable` is the real degradation: a live
+   * sweep tried to build the context and could not.
+   */
+  orphanRecoveryDegraded?: 'dry-run' | 'context-unavailable'
   /** A cross-process owner was already enumerating this workspace. */
   discoveryDeferred?: 'sweep-in-flight'
   error?: { message: string; stack?: string }