From 24d132e347ad7cdd273fb950f28b0e40a0e43a26 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 10:45:58 +1000 Subject: [PATCH 01/16] feat(factory): a sessions layer with its projection store provided The server has the project store, the launch and management services and git in scope where the factory is merged in, but not the projection store. `layerLive` is `layer` with that store provided, for the factory's root layer. Issue: #16 --- apps/server/src/factory/t3/sessions.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/server/src/factory/t3/sessions.ts b/apps/server/src/factory/t3/sessions.ts index 309e8366d400..e4ada0233130 100644 --- a/apps/server/src/factory/t3/sessions.ts +++ b/apps/server/src/factory/t3/sessions.ts @@ -492,3 +492,11 @@ export const make = Effect.gen(function* () { }); export const layer = Layer.effect(FactorySessions, make); + +/** + * `layer` with the projection store it reads provided: `server.ts` has the + * project store, the launch and management services and git in scope where + * the factory is merged in, but not the projection store, which upstream + * provides to each worker that reads it. + */ +export const layerLive = layer.pipe(Layer.provide(ProjectionStore.layer)); From 844104c4a9230c9ca915a29368f7c7a2c85ee1aa Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 10:57:35 +1000 Subject: [PATCH 02/16] test: port softfac's start tests for the launch Fails until the launch lands: start() has no sessions, launch message, text re-check or started outcome yet (type errors and failing assertions in start, messages, pass and index tests). Issue: #16 --- apps/server/src/factory/index.test.ts | 16 +- apps/server/src/factory/pass.test.ts | 8 +- .../src/factory/runner/messages.test.ts | 60 ++ apps/server/src/factory/runner/start.test.ts | 667 ++++++++++++++++-- 4 files changed, 696 insertions(+), 55 deletions(-) create mode 100644 apps/server/src/factory/runner/messages.test.ts diff --git a/apps/server/src/factory/index.test.ts b/apps/server/src/factory/index.test.ts index b32f2bc50cf1..012787b69d36 100644 --- a/apps/server/src/factory/index.test.ts +++ b/apps/server/src/factory/index.test.ts @@ -18,6 +18,7 @@ import { utcSecond } from "./runner/start.ts"; import { MARKER } from "./tracker/github.ts"; import { FactoryProcess, RunSpawnError, type RunInput, type RunResult } from "./process.ts"; import * as Paths from "./t3/paths.ts"; +import * as Sessions from "./t3/sessions.ts"; /** A process runner on a machine with no `gh`: every call fails to start. */ const noGh = Layer.succeed( @@ -27,6 +28,18 @@ const noGh = Layer.succeed( }), ); +/** A FactorySessions that finds no T3 project and dies on anything else. */ +const stubSessions = Sessions.FactorySessions.of({ + findProject: () => Effect.succeed(null), + launch: () => Effect.die("not launched"), + status: () => Effect.die("no status"), + thread: () => Effect.die("no thread"), + send: () => Effect.die("not sent"), + archive: () => Effect.die("not archived"), + removeWorktree: () => Effect.die("not removed"), +}); +const noSessions = Layer.succeed(Sessions.FactorySessions, stubSessions); + const messageText = (message: unknown): string => { const first = Array.isArray(message) ? message[0] : message; return typeof first === "string" ? first : String(first); @@ -76,6 +89,7 @@ const logsFor = ( }), ), Layer.provide(gh), + Layer.provide(noSessions), ), ), ).pipe(Effect.provide(Logger.layer([capture], { mergeWithExisting: false }))); @@ -205,7 +219,7 @@ describe("liveWorld", () => { }; const worldOf = (process: FactoryProcess["Service"]) => - liveWorld(machine, "/factory-test/data", process, () => NOW, "mac-mini"); + liveWorld(machine, "/factory-test/data", process, stubSessions, () => NOW, "mac-mini"); const repo: Repo = { watched: machine.repos[0]!, diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 667c24d7e5c0..03b1752e6cd5 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -61,8 +61,9 @@ const githubFailure = (): StartError => new GitHubFailed({ args: ["issue", "list"], status: 1, stderr: "boom" }); const started = (issue: number, repo = REPO): StartOutcome => ({ - kind: "claimed", + kind: "started", run: rid(issue, repo), + threadId: "thread-1", }); type Answers = Record; @@ -192,7 +193,7 @@ class Fake implements PassWorld { confirmed: true, }); } - if ("kind" in answer && answer.kind === "claimed") { + if ("kind" in answer && answer.kind === "started") { this.runs.push({ id: answer.run, state: "triage", unreadable: false, confirmed: true }); return Effect.succeed(answer); } @@ -337,7 +338,7 @@ describe("claiming new issues", () => { const { lines } = yield* pass(fake, machine([REPO], 1)); assert.equal( runLine(lines, "rawphp/softfac#2 "), - "rawphp/softfac#2 ready: claimed in run rawphp__softfac__2__1", + "rawphp/softfac#2 ready: claimed, session started in run rawphp__softfac__2__1", ); }), ); @@ -370,6 +371,7 @@ describe("claiming new issues", () => { assert.equal(report.claimed, 0); const line = runLine(lines, "rawphp/softfac#12 "); assert.ok(line.includes("stuck"), line); + assert.ok(line.includes("claimed, but the launch failed"), line); assert.ok(line.includes("rawphp__softfac__12__1"), line); assert.ok(!line.includes("secret detail"), line); }), diff --git a/apps/server/src/factory/runner/messages.test.ts b/apps/server/src/factory/runner/messages.test.ts new file mode 100644 index 000000000000..63d7e04fb095 --- /dev/null +++ b/apps/server/src/factory/runner/messages.test.ts @@ -0,0 +1,60 @@ +import { assert, it } from "@effect/vitest"; +import { describe } from "vite-plus/test"; + +import { launchMessage, sessionMayExistNote } from "./messages.ts"; + +/** The paragraph that names the GitHub identity, for the login `rawphp` and the `gh` directory `/gh/rawphp`. */ +const IDENTITY_WORDS = + 'You act on GitHub as rawphp. Your T3 server should have been started with GH_CONFIG_DIR=/gh/rawphp. Before anything else run gh api user --jq .login exactly as written, in the environment you were started with. Never set GH_CONFIG_DIR, GH_TOKEN or GH_HOST, and never use gh auth token or gh auth switch, on any command. If it prints any other login, end with FACTORY-STOP: stuck and make the first line of your message "the session\'s gh account is , not rawphp".'; + +describe("launchMessage", () => { + it("invokes /factory and says nobody is in the chat", () => { + assert.strictEqual( + launchMessage("rawphp/softfac", 10, "rawphp", "/gh/rawphp"), + `/factory rawphp/softfac#10\n\nThe factory daemon launched this session unattended, in a worktree it created for you. Nobody is in the chat. The daemon owns the issue's labels and comments and runs its own gate after your draft PR.\n\n${IDENTITY_WORDS}`, + ); + }); + + it("names the login and the gh config dir the session acts as", () => { + const message = launchMessage("acme/widgets", 7, "alice", "/Users/me/.config/softfac/gh-alice"); + assert.ok( + message.endsWith( + '\n\nYou act on GitHub as alice. Your T3 server should have been started with GH_CONFIG_DIR=/Users/me/.config/softfac/gh-alice. Before anything else run gh api user --jq .login exactly as written, in the environment you were started with. Never set GH_CONFIG_DIR, GH_TOKEN or GH_HOST, and never use gh auth token or gh auth switch, on any command. If it prints any other login, end with FACTORY-STOP: stuck and make the first line of your message "the session\'s gh account is , not alice".', + ), + message, + ); + assert.strictEqual(message.split("You act on GitHub as").length - 1, 1); + }); + + it("forbids every way of choosing the account and names the dir once", () => { + const message = launchMessage("acme/widgets", 42, "rawphp", "/factory/gh"); + const paragraph = message.slice(message.lastIndexOf("\n\n") + 2); + assert.ok( + paragraph.includes( + "run gh api user --jq .login exactly as written, in the environment you were started with.", + ), + paragraph, + ); + assert.ok( + paragraph.includes( + "Never set GH_CONFIG_DIR, GH_TOKEN or GH_HOST, and never use gh auth token or gh auth switch, on any command.", + ), + paragraph, + ); + assert.ok( + paragraph.includes("Your T3 server should have been started with GH_CONFIG_DIR=/factory/gh."), + paragraph, + ); + // The variable's name with a value appears only in the clause for the server. + assert.strictEqual(paragraph.split("GH_CONFIG_DIR=").length - 1, 1); + }); +}); + +describe("sessionMayExistNote", () => { + it("says a launch attempt may have started a session, and names the command id", () => { + assert.strictEqual( + sessionMayExistNote("the run passed its time limit", "softfac:rawphp__softfac__7__1:launch"), + "the run passed its time limit; a launch attempt may have started a session (launch command id softfac:rawphp__softfac__7__1:launch)", + ); + }); +}); diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index bd4ca8f0bad4..25474e25feb0 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -8,16 +8,23 @@ import * as Schema from "effect/Schema"; import { afterEach, describe } from "vite-plus/test"; import { timeLimitOnlyProjectConfig, type ProjectConfig } from "../config.ts"; -import type { Approval } from "../domain/approval.ts"; +import type { Approval, TextCheck } from "../domain/approval.ts"; import { unapprovedComment } from "../domain/approval.ts"; import type { State } from "../domain/lifecycle.ts"; import { gateLockFile } from "../gate.ts"; -import { makeCause } from "../ledger/events.ts"; +import { makeCause, type Cause, type RunEvent } from "../ledger/events.ts"; import { Ledger } from "../ledger/ledger.ts"; import { scanMachineLog } from "../ledger/machineLog.ts"; import { readRun } from "../ledger/read.ts"; -import { runPath, type RunId } from "../ledger/runId.ts"; +import { parseRunId, runPath, type RunId } from "../ledger/runId.ts"; import { T0, useTempDirs } from "../ledger/testSupport.ts"; +import { + SessionsError, + type LaunchRequest, + type Launched, + type Model, + type Thread, +} from "../t3/sessions.ts"; import { GitHubFailed, type ClaimOutcome, @@ -25,8 +32,24 @@ import { type GitHubError, type IssueDetail, } from "../tracker/github.ts"; -import { CANNOT_CONFIRM, CUT_SHORT_COMMENT, statusBody, stuckComment } from "./messages.ts"; -import { GateRunningError, start, type StartOutcome, type Tracker } from "./start.ts"; +import { + CANNOT_CONFIRM, + CUT_SHORT_COMMENT, + LAUNCH_FAILED, + TEXT_CHANGED, + launchMessage, + sessionMayExistNote, + statusBody, + stuckComment, +} from "./messages.ts"; +import { + GateRunningError, + start, + type Sessions, + type StartContext, + type StartOutcome, + type Tracker, +} from "./start.ts"; const tempDir = useTempDirs(); @@ -71,11 +94,14 @@ const fakeTracker = (script: { claims?: Array; assignFails?: boolean; setStateFails?: boolean; + unassignFails?: boolean; newestComment?: FactoryComment | null; + textChecks?: Array; }) => { const calls: Array = []; const issues = [...(script.issues ?? [detail("ready")])]; const claims = [...(script.claims ?? [{ kind: "claimed" } as const])]; + const textChecks = [...(script.textChecks ?? [])]; const answer = (queue: Array, fallback: A) => { const next = queue.length > 1 ? queue.shift()! : (queue[0] ?? fallback); return next instanceof Object && "_tag" in next && typeof next._tag === "string" @@ -107,7 +133,7 @@ const fakeTracker = (script: { }, unassign: (n, user) => { calls.push(`unassign ${n} ${user}`); - return Effect.void; + return script.unassignFails ? Effect.fail(ghFailure()) : Effect.void; }, postComment: (n, body) => { calls.push(`comment ${n} ${body}`); @@ -121,26 +147,196 @@ const fakeTracker = (script: { calls.push(`newest ${n}`); return Effect.succeed(script.newestComment ?? null); }, + textCheck: (n, since, instructors) => { + calls.push(`textCheck ${n} ${since} ${instructors.join(",")}`); + return answer(textChecks, "unchanged"); + }, }; return { tracker, calls }; }; +const isSessionsError = Schema.is(SessionsError); + +const MODEL: Model = { instanceId: "claudeAgent", model: "claude-opus-5-5" }; +const LAUNCH_ID = "softfac:rawphp__softfac__7__1:launch"; +const WORKTREE = "/t3/wt"; + +/** + * Scripted answers for the T3 sessions the claim calls. Each method pops its next answer, or + * repeats the last one. `launch` fails the test when the run's ledger does not yet hold the + * `launch-sending` step: the step is written before the call, as softfac's own hook checks. + */ +const fakeSessions = ( + dataDir: string, + script: { + project?: string | null | SessionsError; + launches?: Array; + archive?: Array; + thread?: Thread; + }, +) => { + const calls: Array = []; + const launchRequests: Array = []; + const launches = [...(script.launches ?? [])]; + const archives = [...(script.archive ?? [])]; + const popped = (queue: Array): A | undefined => + queue.length > 1 ? queue.shift() : queue[0]; + const result = (answer: A | SessionsError | undefined, fallback: A) => + isSessionsError(answer) + ? Effect.fail(answer) + : Effect.succeed(answer === undefined ? fallback : answer); + const sessions: Sessions = { + findProject: (root) => { + calls.push(`findProject ${root}`); + return result(script.project === undefined ? "proj-1" : script.project, "proj-1"); + }, + launch: (request) => { + calls.push(`launch ${request.commandId}`); + launchRequests.push(request); + const ledgerFile = runPath( + dataDir, + parseRunId(request.commandId.slice("softfac:".length, -":launch".length)), + ); + if (!NodeFS.readFileSync(ledgerFile, "utf8").includes('"step":"launch-sending"')) { + return Effect.die(new Error("launch was called before the launch-sending step")); + } + return result(popped(launches), { + threadId: "thread-1", + worktreePath: WORKTREE, + branch: "t3code/abc", + }); + }, + thread: (threadId) => { + calls.push(`thread ${threadId}`); + return Effect.succeed( + script.thread ?? { worktreePath: WORKTREE, branch: "t3code/abc", archived: false }, + ); + }, + archive: (threadId) => { + calls.push(`archive ${threadId}`); + const answer = popped(archives); + return isSessionsError(answer) ? Effect.fail(answer) : Effect.void; + }, + removeWorktree: (path) => { + calls.push(`removeWorktree ${path}`); + return Effect.void; + }, + }; + return { sessions, calls, launchRequests }; +}; + const run7: RunId = { repo: REPO, issue: "7", attempt: 1 }; const isGateRunning = Schema.is(GateRunningError); -const ctxFor = (dataDir: string, tracker: Tracker, clock: () => number = () => T0) => ({ +const ctxFor = ( + dataDir: string, + tracker: Tracker, + sessions: Sessions, + extra: Partial = {}, +): StartContext => ({ repo: REPO, trackerUser: USER, machine: "mac-mini", project: project(), dataDir, tracker, - clock, + repoPath: "/work/softfac", + ghConfigDir: "/gh/rawphp", + model: MODEL, + sessions, + clock: () => T0, + ...extra, }); -const startIn = (dataDir: string, tracker: Tracker, clock?: () => number) => - start(ctxFor(dataDir, tracker, clock), 7); +const startIn = ( + dataDir: string, + tracker: Tracker, + sessions: Sessions = fakeSessions(dataDir, {}).sessions, + extra: Partial = {}, +) => start(ctxFor(dataDir, tracker, sessions, extra), 7); + +const cause = (reason: string): Cause => makeCause("start", reason); +const BUILDING = statusBody("building", "mac-mini", "thread-1", null); +const FINISHED = { + outcome: "stuck", + total_ms: 0, + gate_attempts: 0, + review_cycles: null, + pr_url: null, + pr_end: null, +}; + +/** The lines of a run that was started, in order: what the ledger holds when the issue is `building`. */ +const startedLines = (): Array<{ line: RunEvent; cause: Cause }> => [ + { + line: { event: "step_changed", data: { step: `approved ${LABELED}` } }, + cause: makeCause("start", "approved", { actor: "tomkaczocha" }), + }, + { + line: { event: "claimed", data: { machine: "mac-mini", github_user: USER } }, + cause: cause("claimed"), + }, + { + line: { event: "state_changed", data: { from: "ready", to: "triage" } }, + cause: cause("claimed"), + }, + { + line: { event: "step_changed", data: { step: "launch-sending" } }, + cause: cause("launch-sending"), + }, + { + line: { + event: "session_launched", + data: { + thread_id: "thread-1", + worktree: WORKTREE, + branch: "t3code/abc", + model_instance: "claudeAgent", + model: "claude-opus-5-5", + resumed_from: null, + }, + }, + cause: cause("session-launched"), + }, + { + line: { event: "step_changed", data: { step: "approval-rechecked" } }, + cause: cause("approval-rechecked"), + }, + { + line: { event: "state_changed", data: { from: "triage", to: "building" } }, + cause: cause("session-started"), + }, +]; + +const startedEvents = () => + startedLines().map(({ line, cause: why }) => ({ + event: line.event, + data: line.data, + cause: why, + })); + +/** A run file holding the first `lines` lines of `startedLines()`, as an earlier pass left it. */ +const seedRun = (dataDir: string, lines: ReadonlyArray<{ line: RunEvent; cause: Cause }>) => { + const ledger = Ledger.create(dataDir, run7, { clock: () => T0 - 1000 }); + handles.push(ledger); + for (const { line, cause: why } of lines) ledger.append(line, why); +}; + +/** The lines a run ends with when it goes stuck from `triage` on the stuck line `reason`. */ +const stuckTail = (reason: string, code: string) => [ + { event: "stuck", data: { reason }, cause: cause(code) }, + { event: "state_changed", data: { from: "triage", to: "stuck" }, cause: cause(code) }, + { event: "finished", data: FINISHED, cause: cause(code) }, +]; + +/** What a stuck run does to the issue, after the session is stopped: the public comment, the label, the release, the status. */ +const stuckCalls = (publicReason: string, thread: string | null) => [ + `comment 7 ${stuckComment(publicReason)}`, + "setState 7 stuck", + "unassign 7 rawphp", + `status 7 rawphp ${statusBody("stuck", "mac-mini", thread, null)}`, +]; const entriesOf = (dataDir: string, id: RunId = run7) => readRun(dataDir, id).entries.map((e) => ({ event: e.event, data: e.data, cause: e.cause })); @@ -170,34 +366,76 @@ const olderRun = ( }; describe("start: a fresh ready issue", () => { - it.effect("checks the approval, claims, and records the approval, the claim and the state", () => + it.effect("claims, launches the session and moves the issue to building", () => Effect.gen(function* () { const dataDir = tempDir(); const { tracker, calls } = fakeTracker({}); - const outcome = yield* startIn(dataDir, tracker); - assert.deepStrictEqual(outcome, { kind: "claimed", run: run7 } satisfies StartOutcome); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { + kind: "started", + run: run7, + threadId: "thread-1", + } satisfies StartOutcome); assert.deepStrictEqual(calls, [ "issue 7", "approval 7 tomkaczocha", "claim 7 rawphp ready->triage", + `textCheck 7 ${LABELED} tomkaczocha`, + "setState 7 building", + `status 7 rawphp ${BUILDING}`, ]); - assert.deepStrictEqual(entriesOf(dataDir), [ - { - event: "step_changed", - data: { step: `approved ${LABELED}` }, - cause: { step: "start", reason: "approved", actor: "tomkaczocha" }, - }, + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac", `launch ${LAUNCH_ID}`]); + assert.deepStrictEqual(t3.launchRequests, [ { - event: "claimed", - data: { machine: "mac-mini", github_user: "rawphp" }, - cause: { step: "start", reason: "claimed" }, - }, - { - event: "state_changed", - data: { from: "ready", to: "triage" }, - cause: { step: "start", reason: "claimed" }, + commandId: LAUNCH_ID, + projectId: "proj-1", + title: "rawphp/softfac#7 Build step", + baseRef: "main", + startFromOrigin: false, + message: launchMessage(REPO, 7, USER, "/gh/rawphp"), + model: MODEL, }, ]); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + }), + ); + + it.effect("records the model the launch call sent", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker } = fakeTracker({}); + const t3 = fakeSessions(dataDir, {}); + const model = { instanceId: "codex", model: "gpt-5.5" }; + yield* startIn(dataDir, tracker, t3.sessions, { model }); + assert.deepStrictEqual(t3.launchRequests[0]!.model, model); + assert.deepStrictEqual(entriesOf(dataDir)[4]!.data, { + thread_id: "thread-1", + worktree: WORKTREE, + branch: "t3code/abc", + model_instance: "codex", + model: "gpt-5.5", + resumed_from: null, + }); + }), + ); + + it.effect("records a launch that has no worktree yet", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker } = fakeTracker({}); + const t3 = fakeSessions(dataDir, { + launches: [{ threadId: "thread-1", worktreePath: null, branch: null }], + }); + yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(entriesOf(dataDir)[4]!.data, { + thread_id: "thread-1", + worktree: null, + branch: null, + model_instance: "claudeAgent", + model: "claude-opus-5-5", + resumed_from: null, + }); }), ); @@ -283,9 +521,11 @@ describe("start: a fresh ready issue", () => { ] as const) { const dataDir = tempDir(); const { tracker, calls } = fakeTracker({ issues: [issue] }); - const outcome = yield* startIn(dataDir, tracker); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); assert.deepStrictEqual(outcome, { kind: "skipped", reason }); assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(t3.calls, []); assert.strictEqual(NodeFS.existsSync(NodePath.join(dataDir, "runs")), false); } }), @@ -295,11 +535,14 @@ describe("start: a fresh ready issue", () => { Effect.gen(function* () { const dataDir = tempDir(); const { tracker } = fakeTracker({ claims: [{ kind: "skipped-assigned", assignees: ["a"] }] }); - const outcome = yield* startIn(dataDir, tracker); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); assert.deepStrictEqual(outcome, { kind: "skipped", reason: "issue #7 is already assigned to a", }); + // T3 is asked before the claim; nothing is launched for a claim someone else won. + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac"]); assert.strictEqual(NodeFS.existsSync(runPath(dataDir, run7)), false); }), ); @@ -314,8 +557,8 @@ describe("start: a claim call that fails", () => { claims: [ghFailure()], }); const outcome = yield* startIn(dataDir, tracker); - assert.deepStrictEqual(outcome, { kind: "claimed", run: run7 }); - assert.deepStrictEqual(calls, [ + assert.deepStrictEqual(outcome, { kind: "started", run: run7, threadId: "thread-1" }); + assert.deepStrictEqual(calls.slice(0, 4), [ "issue 7", "approval 7 tomkaczocha", "claim 7 rawphp ready->triage", @@ -325,7 +568,7 @@ describe("start: a claim call that fails", () => { scanMachineLog(dataDir).map((e) => [e.event, e.data]), [["claim_repaired", { repo: REPO, issue: 7, outcome: "finished" }]], ); - assert.deepStrictEqual(stepsOf(dataDir), ["step_changed", "claimed", "state_changed"]); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); }), ); @@ -336,8 +579,12 @@ describe("start: a claim call that fails", () => { issues: [detail("ready"), detail("triage", [])], claims: [ghFailure()], }); - assert.deepStrictEqual(yield* startIn(dataDir, tracker), { kind: "claimed", run: run7 }); - assert.deepStrictEqual(calls.slice(3), ["issue 7", "assign 7 rawphp"]); + assert.deepStrictEqual(yield* startIn(dataDir, tracker), { + kind: "started", + run: run7, + threadId: "thread-1", + }); + assert.deepStrictEqual(calls.slice(3, 5), ["issue 7", "assign 7 rawphp"]); }), ); @@ -348,9 +595,12 @@ describe("start: a claim call that fails", () => { issues: [detail("ready"), detail("ready", [])], claims: [ghFailure()], }); - const error = yield* Effect.flip(startIn(dataDir, tracker)); + const t3 = fakeSessions(dataDir, {}); + const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); assert.strictEqual(error._tag, "GitHubFailed"); assert.deepStrictEqual(calls.slice(3), ["issue 7"]); + // Nothing was launched, and no run file exists to hold a launch. + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac"]); assert.strictEqual(NodeFS.existsSync(runPath(dataDir, run7)), false); assert.deepStrictEqual(scanMachineLog(dataDir), []); }), @@ -388,8 +638,12 @@ describe("start: a claim call that fails", () => { issues: [detail("ready"), detail("ready", [USER])], claims: [ghFailure()], }); - assert.deepStrictEqual(yield* startIn(dataDir, tracker), { kind: "claimed", run: run7 }); - assert.deepStrictEqual(calls.slice(3), ["issue 7", "setState 7 triage"]); + assert.deepStrictEqual(yield* startIn(dataDir, tracker), { + kind: "started", + run: run7, + threadId: "thread-1", + }); + assert.deepStrictEqual(calls.slice(3, 5), ["issue 7", "setState 7 triage"]); assert.deepStrictEqual( scanMachineLog(dataDir).map((e) => [e.event, e.data]), [["claim_repaired", { repo: REPO, issue: 7, outcome: "finished" }]], @@ -404,8 +658,16 @@ describe("start: a claim call that fails", () => { issues: [detail("ready"), detail(null, [])], claims: [ghFailure()], }); - assert.deepStrictEqual(yield* startIn(dataDir, tracker), { kind: "claimed", run: run7 }); - assert.deepStrictEqual(calls.slice(3), ["issue 7", "assign 7 rawphp", "setState 7 triage"]); + assert.deepStrictEqual(yield* startIn(dataDir, tracker), { + kind: "started", + run: run7, + threadId: "thread-1", + }); + assert.deepStrictEqual(calls.slice(3, 6), [ + "issue 7", + "assign 7 rawphp", + "setState 7 triage", + ]); assert.deepStrictEqual( scanMachineLog(dataDir).map((e) => [e.event, e.data]), [["claim_repaired", { repo: REPO, issue: 7, outcome: "finished" }]], @@ -446,15 +708,12 @@ describe("start: an issue in triage that is ours", () => { Effect.gen(function* () { const dataDir = tempDir(); const { tracker, calls } = fakeTracker({ issues: [detail("triage", [USER])] }); - const outcome = yield* startIn(dataDir, tracker); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: CANNOT_CONFIRM }); - assert.deepStrictEqual(calls, [ - "issue 7", - `comment 7 ${stuckComment(CANNOT_CONFIRM)}`, - "setState 7 stuck", - "unassign 7 rawphp", - `status 7 rawphp ${statusBody("stuck", "mac-mini", null, null)}`, - ]); + assert.deepStrictEqual(calls, ["issue 7", ...stuckCalls(CANNOT_CONFIRM, null)]); + // No session was launched, so no T3 call at all. + assert.deepStrictEqual(t3.calls, []); const code = { step: "start", reason: "approval-unconfirmed" }; assert.deepStrictEqual(entriesOf(dataDir), [ { @@ -502,14 +761,16 @@ describe("start: an issue in triage that is ours", () => { }), ); - it.effect("with an unfinished run that was approved is left to that run", () => + it.effect("with an unfinished run in another state is left to the step that owns it", () => Effect.gen(function* () { const dataDir = tempDir(); - olderRun(dataDir, 1, "triage", false, true); + olderRun(dataDir, 1, "building", false, true); const { tracker, calls } = fakeTracker({ issues: [detail("triage", [USER])] }); - const outcome = yield* startIn(dataDir, tracker); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); assert.strictEqual(outcome.kind, "skipped"); assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(t3.calls, []); assert.deepStrictEqual(stepsOf(dataDir), ["step_changed", "claimed", "state_changed"]); }), ); @@ -576,7 +837,11 @@ describe("start: older runs of the issue", () => { olderRun(dataDir, 4, "stuck", true); const { tracker } = fakeTracker({}); const outcome = yield* startIn(dataDir, tracker); - assert.deepStrictEqual(outcome, { kind: "claimed", run: { ...run7, attempt: 5 } }); + assert.deepStrictEqual(outcome, { + kind: "started", + run: { ...run7, attempt: 5 }, + threadId: "thread-1", + }); for (const [id, expected] of [ [a, "needs-info"], [b, "pr-open"], @@ -612,3 +877,303 @@ describe("start: older runs of the issue", () => { }), ); }); + +const NO_PROJECT = "no T3 project for this repo on this machine"; +const REFUSED = "Thread launch failed during resolve-project."; +const sessionsError = (operation: "archive" | "find-project" | "launch", message: string) => + new SessionsError({ operation, cause: new Error(message) }); + +describe("start: the launch", () => { + it.effect("a repo with no T3 project goes stuck before any launch is sent", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({}); + const t3 = fakeSessions(dataDir, { project: null }); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: NO_PROJECT }); + assert.deepStrictEqual(calls, [ + "issue 7", + "approval 7 tomkaczocha", + "claim 7 rawphp ready->triage", + ...stuckCalls(LAUNCH_FAILED, null), + ]); + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac"]); + assert.deepStrictEqual(entriesOf(dataDir), [ + ...startedEvents().slice(0, 3), + ...stuckTail(NO_PROJECT, "launch-failed"), + ]); + }), + ); + + it.effect("a T3 that cannot be reached when the project is looked up claims nothing", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({}); + const t3 = fakeSessions(dataDir, { project: sessionsError("find-project", "T3 is down") }); + const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); + assert.strictEqual(error._tag, "FactorySessionsError"); + assert.deepStrictEqual(calls, ["issue 7", "approval 7 tomkaczocha"]); + assert.strictEqual(NodeFS.existsSync(runPath(dataDir, run7)), false); + }), + ); + + it.effect("a launch T3 refuses goes stuck with the detail in the ledger only", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({}); + const t3 = fakeSessions(dataDir, { launches: [sessionsError("launch", REFUSED)] }); + const detailText = sessionMayExistNote(REFUSED, LAUNCH_ID); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: detailText }); + // The public comment never carries T3's words. + assert.deepStrictEqual(calls.slice(3), stuckCalls(LAUNCH_FAILED, null)); + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac", `launch ${LAUNCH_ID}`]); + assert.deepStrictEqual(entriesOf(dataDir), [ + ...startedEvents().slice(0, 4), + ...stuckTail(detailText, "launch-failed"), + ]); + }), + ); + + it.effect("a second attempt gets the next number and its own command id", () => + Effect.gen(function* () { + const dataDir = tempDir(); + olderRun(dataDir, 1, "stuck", true); + const { tracker } = fakeTracker({}); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + const second = { ...run7, attempt: 2 }; + assert.deepStrictEqual(outcome, { kind: "started", run: second, threadId: "thread-1" }); + assert.deepStrictEqual(t3.calls, [ + "findProject /work/softfac", + "launch softfac:rawphp__softfac__7__2:launch", + ]); + assert.deepStrictEqual(entriesOf(dataDir, second), startedEvents()); + assert.deepStrictEqual(stepsOf(dataDir), ["claimed", "state_changed", "finished"]); + }), + ); + + it.effect("a text that changed before the second look archives the session and goes stuck", () => + Effect.gen(function* () { + for (const check of ["changed", "unjudged"] as const) { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({ textChecks: [check] }); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: TEXT_CHANGED }); + assert.deepStrictEqual(calls, [ + "issue 7", + "approval 7 tomkaczocha", + "claim 7 rawphp ready->triage", + `textCheck 7 ${LABELED} tomkaczocha`, + ...stuckCalls(TEXT_CHANGED, "thread-1"), + ]); + assert.deepStrictEqual(t3.calls, [ + "findProject /work/softfac", + `launch ${LAUNCH_ID}`, + "archive thread-1", + "thread thread-1", + `removeWorktree ${WORKTREE}`, + ]); + assert.deepStrictEqual(entriesOf(dataDir), [ + ...startedEvents().slice(0, 5), + { + event: "worktree_removed", + data: { path: WORKTREE }, + cause: cause("worktree-removed"), + }, + ...stuckTail(TEXT_CHANGED, "text-changed-before-start"), + ]); + } + }), + ); + + it.effect( + "an archive that fails is an error before anything is written, and the next start retries", + () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({ + issues: [detail("ready"), detail("triage", [USER])], + textChecks: ["changed", "changed"], + }); + const t3 = fakeSessions(dataDir, { + archive: [sessionsError("archive", "T3 is busy"), undefined], + }); + const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); + assert.strictEqual(error._tag, "FactorySessionsError"); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 5)); + assert.strictEqual(calls.at(-1), `textCheck 7 ${LABELED} tomkaczocha`); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: TEXT_CHANGED }); + assert.strictEqual(t3.calls.filter((c) => c.startsWith("launch ")).length, 1); + assert.strictEqual(t3.calls.filter((c) => c.startsWith("archive ")).length, 2); + }), + ); + + it.effect("the second look comes after the launch is recorded and before the label", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const first = fakeTracker({ textChecks: [ghFailure()] }); + const t3 = fakeSessions(dataDir, {}); + const error = yield* Effect.flip(startIn(dataDir, first.tracker, t3.sessions)); + assert.strictEqual(error._tag, "GitHubFailed"); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 5)); + assert.strictEqual(first.calls.at(-1), `textCheck 7 ${LABELED} tomkaczocha`); + // The next start finds the thread in the ledger and does not launch again. + const second = fakeTracker({ issues: [detail("triage", [USER])] }); + const outcome = yield* startIn(dataDir, second.tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "started", run: run7, threadId: "thread-1" }); + assert.strictEqual(t3.calls.filter((c) => c.startsWith("launch ")).length, 1); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + }), + ); + + it.effect("a label change that fails after the launch is a runner error with no rollback", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({ setStateFails: true }); + const t3 = fakeSessions(dataDir, {}); + const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); + assert.strictEqual(error._tag, "GitHubFailed"); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 6)); + assert.strictEqual(calls.length, 5); + assert.strictEqual( + t3.calls.some((c) => c.startsWith("archive")), + false, + ); + }), + ); + + it.effect("an unassign that fails in the stuck path is a runner error", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker } = fakeTracker({ unassignFails: true }); + const t3 = fakeSessions(dataDir, { project: null }); + const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); + assert.strictEqual(error._tag, "GitHubFailed"); + const events = stepsOf(dataDir); + assert.strictEqual(events.length, 4); + assert.notStrictEqual(events.at(-1), "finished"); + }), + ); +}); + +describe("start: a launch that an earlier pass may have sent", () => { + it.effect("writes the launch-sending step once and sends under the same command id", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 4)); + const { tracker } = fakeTracker({ issues: [detail("triage", [USER])] }); + const t3 = fakeSessions(dataDir, {}); + yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac", `launch ${LAUNCH_ID}`]); + }), + ); + + it.effect( + "a launch that cannot be sent after an earlier one may have been says a session may exist", + () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 4)); + const { tracker, calls } = fakeTracker({ issues: [detail("triage", [USER])] }); + const t3 = fakeSessions(dataDir, { project: null }); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.strictEqual(outcome.kind, "stuck"); + const reason = outcome.kind === "stuck" ? outcome.reason : ""; + assert.ok(reason.includes("may have started a session"), reason); + assert.ok(reason.includes(LAUNCH_ID), reason); + assert.strictEqual(reason, sessionMayExistNote(NO_PROJECT, LAUNCH_ID)); + assert.strictEqual(calls[1], `comment 7 ${stuckComment(LAUNCH_FAILED)}`); + }), + ); + + it.effect("a first launch that never went out has no such note", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker } = fakeTracker({}); + const t3 = fakeSessions(dataDir, { project: null }); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + const reason = outcome.kind === "stuck" ? outcome.reason : ""; + assert.ok(!reason.includes("may have started"), reason); + }), + ); +}); + +describe("start: a resume", () => { + const textCheckCall = `textCheck 7 ${LABELED} tomkaczocha`; + const triage = () => fakeTracker({ issues: [detail("triage", [USER])] }); + + it.effect("with no session launches and then looks at the text once", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 3)); + const { tracker, calls } = triage(); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "started", run: run7, threadId: "thread-1" }); + assert.deepStrictEqual(t3.calls, ["findProject /work/softfac", `launch ${LAUNCH_ID}`]); + assert.deepStrictEqual(calls, [ + "issue 7", + textCheckCall, + "setState 7 building", + `status 7 rawphp ${BUILDING}`, + ]); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + }), + ); + + it.effect("with a session that was not looked at again does not launch again", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 5)); + const { tracker, calls } = triage(); + const t3 = fakeSessions(dataDir, {}); + yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(calls, [ + "issue 7", + textCheckCall, + "setState 7 building", + `status 7 rawphp ${BUILDING}`, + ]); + assert.deepStrictEqual(t3.calls, []); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + }), + ); + + it.effect("with a second look that passed does not repeat it", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 6)); + const { tracker, calls } = triage(); + const t3 = fakeSessions(dataDir, {}); + yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(calls, [ + "issue 7", + "setState 7 building", + `status 7 rawphp ${BUILDING}`, + ]); + assert.deepStrictEqual(t3.calls, []); + }), + ); + + it.effect("with a session and no recorded approval archives it and goes stuck", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const lines = startedLines(); + seedRun(dataDir, [lines[1]!, lines[2]!, lines[4]!]); + const { tracker, calls } = triage(); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: CANNOT_CONFIRM }); + assert.deepStrictEqual(calls, ["issue 7", ...stuckCalls(CANNOT_CONFIRM, "thread-1")]); + assert.deepStrictEqual(t3.calls, [ + "archive thread-1", + "thread thread-1", + `removeWorktree ${WORKTREE}`, + ]); + }), + ); +}); From cb7c29f67a37491293f599529cf546991d5fd9e5 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 10:57:37 +1000 Subject: [PATCH 03/16] feat(factory): launch the /factory session for a claimed issue Issue: #16 --- apps/server/src/factory/index.ts | 16 +- apps/server/src/factory/pass.ts | 6 +- apps/server/src/factory/runner/messages.ts | 28 ++ apps/server/src/factory/runner/progress.ts | 41 ++- apps/server/src/factory/runner/start.ts | 370 ++++++++++++++++++--- 5 files changed, 414 insertions(+), 47 deletions(-) diff --git a/apps/server/src/factory/index.ts b/apps/server/src/factory/index.ts index 482458aa7c54..351b4aded9e3 100644 --- a/apps/server/src/factory/index.ts +++ b/apps/server/src/factory/index.ts @@ -30,8 +30,9 @@ import { runPass, type PassError, type PassWorld, type Repo } from "./pass.ts"; import * as Process from "./process.ts"; import { FactoryProcess } from "./process.ts"; import { CUT_SHORT_COMMENT } from "./runner/messages.ts"; -import { start, utcSecond, type Tracker } from "./runner/start.ts"; +import { start, utcSecond, type Sessions, type Tracker } from "./runner/start.ts"; import * as Paths from "./t3/paths.ts"; +import * as T3Sessions from "./t3/sessions.ts"; import { bindProcess, makeGitHub, type GitHubError, type IssueSummary } from "./tracker/github.ts"; /** The running factory, for the code that wakes it (a thread's run changing, a person's request). */ @@ -108,6 +109,7 @@ export const liveWorld = ( machine: MachineConfig, dataDir: string, process: FactoryProcess["Service"], + sessions: Sessions, clock: () => number, host: string, ): PassWorld => { @@ -173,6 +175,10 @@ export const liveWorld = ( project: repo.project, dataDir, tracker: trackerFor(repo.watched), + repoPath: repo.watched.path, + ghConfigDir: machine.ghConfigDir, + model: { instanceId: machine.modelInstance, model: machine.model }, + sessions, clock, }, issue, @@ -231,7 +237,8 @@ const startFactory = Effect.gen(function* () { const machine = loaded.success.value; yield* Effect.logInfo("factory started"); const process = yield* FactoryProcess; - const world = liveWorld(machine, factoryDir, process, () => Date.now(), hostName()); + const sessions = yield* T3Sessions.FactorySessions; + const world = liveWorld(machine, factoryDir, process, sessions, () => Date.now(), hostName()); const onePass = Effect.gen(function* () { // Each line is logged as it is decided, so a pass that hangs or dies has said what it did. const context = yield* Effect.context(); @@ -242,8 +249,8 @@ const startFactory = Effect.gen(function* () { }); /** - * Needs FactoryPaths, FactoryProcess and FileSystem; what index.test.ts builds with a test - * FactoryPaths and a fake FactoryProcess. + * Needs FactoryPaths, FactoryProcess, FactorySessions and FileSystem; what index.test.ts builds + * with a test FactoryPaths, a fake FactoryProcess and a stub FactorySessions. */ export const FactoryRuntime = Layer.effect(FactoryLoop, startFactory); @@ -251,4 +258,5 @@ export const FactoryRuntime = Layer.effect(FactoryLoop, startFactory); export const FactoryLayer = FactoryRuntime.pipe( Layer.provide(Paths.layer), Layer.provide(Process.layer), + Layer.provide(T3Sessions.layerLive), ); diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 0c44713fba79..e5a83f03584c 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -561,7 +561,7 @@ const noteStart = ( return madeSince(newestRun(known.of(name, issue)), before); } switch (outcome.success.kind) { - case "claimed": { + case "started": { const run = runIdToString(outcome.success.run); notes.parts.push(words.claimed(run)); if (notes.state === "ready") pass.report.claimed += 1; @@ -716,9 +716,9 @@ const claim = ( continue; } const run = yield* noteStart(world, pass, known, notes, repo, issue, { - claimed: (run) => `claimed in run ${run}`, + claimed: (run) => `claimed, session started in run ${run}`, notTaken: "not claimed", - stuck: "claimed, then", + stuck: "claimed, but the launch failed", }); yield* pass.flush(notes, run); if (known.lost) return; diff --git a/apps/server/src/factory/runner/messages.ts b/apps/server/src/factory/runner/messages.ts index 6f0715c5628b..b82559b51913 100644 --- a/apps/server/src/factory/runner/messages.ts +++ b/apps/server/src/factory/runner/messages.ts @@ -21,6 +21,34 @@ export const CUT_SHORT_COMMENT = export const stuckComment = (reason: string): string => `The factory could not finish this issue and has released it.\n\nReason: ${reason}\n\nFix the cause, then set the label back to \`factory:ready\`.`; +/** The public reason when the issue text changed between the approval and the session. */ +export const TEXT_CHANGED = "the issue text was changed while the factory was starting"; + +/** The public reason when the launch was refused. The detail stays in the ledger. */ +export const LAUNCH_FAILED = "the factory could not launch a session"; + +/** The words a stuck run's ledger reason carries when a launch attempt may have started a session. */ +export const SESSION_MAY_EXIST = "a launch attempt may have started a session"; + +export const sessionMayExistNote = (what: string, commandId: string): string => + `${what}; ${SESSION_MAY_EXIST} (launch command id ${commandId})`; + +/** How the first line of a stuck session's message begins when gh acts as another account. */ +export const WRONG_ACCOUNT_PREFIX = "the session's gh account is "; + +/** + * The first message of a session for a GitHub issue: invokes `/factory`, says nobody is in the + * chat, and names the GitHub identity the session acts as (`login`, and the `ghConfigDir` its T3 + * server should have been started with) with what to do when `gh` says otherwise. + */ +export const launchMessage = ( + repo: string, + issue: number, + login: string, + ghConfigDir: string, +): string => + `/factory ${repo}#${issue}\n\nThe factory daemon launched this session unattended, in a worktree it created for you. Nobody is in the chat. The daemon owns the issue's labels and comments and runs its own gate after your draft PR.\n\nYou act on GitHub as ${login}. Your T3 server should have been started with GH_CONFIG_DIR=${ghConfigDir}. Before anything else run gh api user --jq .login exactly as written, in the environment you were started with. Never set GH_CONFIG_DIR, GH_TOKEN or GH_HOST, and never use gh auth token or gh auth switch, on any command. If it prints any other login, end with FACTORY-STOP: stuck and make the first line of your message "${WRONG_ACCOUNT_PREFIX}, not ${login}".`; + /** The issue's single status comment. */ export const statusBody = ( state: State, diff --git a/apps/server/src/factory/runner/progress.ts b/apps/server/src/factory/runner/progress.ts index c01706eb28ce..2de38df96e00 100644 --- a/apps/server/src/factory/runner/progress.ts +++ b/apps/server/src/factory/runner/progress.ts @@ -14,11 +14,21 @@ export interface Seen { readonly state: State | null; /** The `session_launched` line's thread. */ readonly threadId: string | null; + /** The `launch-sending` step: a launch call was about to be sent, so a session may exist. */ + readonly launchSent: boolean; + /** The `approval-rechecked` step: the issue text was checked again after the launch. */ + readonly rechecked: boolean; + /** The `session_launched` line's worktree path. */ + readonly worktree: string | null; + /** A `worktree_removed` or `worktree_remove_failed` line follows the last `session_launched`. */ + readonly worktreeDone: boolean; /** The reason on the `stuck` line: the comment is on the issue. */ readonly stuck: string | null; readonly finished: boolean; } +const LAUNCH_SENDING = "launch-sending"; + const field = (data: unknown, key: string): unknown => typeof data === "object" && data !== null ? (data as Record)[key] : undefined; @@ -27,6 +37,10 @@ export const readProgress = (entries: ReadonlyArray): Seen => { let approvedAt: string | null = null; let state: State | null = null; let threadId: string | null = null; + let launchSent = false; + let rechecked = false; + let worktree: string | null = null; + let worktreeDone = false; let stuck: string | null = null; let finished = false; for (const entry of entries) { @@ -39,6 +53,13 @@ export const readProgress = (entries: ReadonlyArray): Seen => { if (approvedAt === null && typeof step === "string" && step.startsWith("approved ")) { approvedAt = step.slice("approved ".length); } + if (step === "approval-rechecked") rechecked = true; + if ( + typeof step === "string" && + (step === LAUNCH_SENDING || step.startsWith(`${LAUNCH_SENDING} resumed-from `)) + ) { + launchSent = true; + } break; } case "state_changed": { @@ -49,8 +70,15 @@ export const readProgress = (entries: ReadonlyArray): Seen => { case "session_launched": { const id = field(entry.data, "thread_id"); if (typeof id === "string") threadId = id; + const path = field(entry.data, "worktree"); + worktree = typeof path === "string" ? path : null; + worktreeDone = false; break; } + case "worktree_removed": + case "worktree_remove_failed": + worktreeDone = true; + break; case "stuck": { const reason = field(entry.data, "reason"); stuck = typeof reason === "string" ? reason : ""; @@ -61,5 +89,16 @@ export const readProgress = (entries: ReadonlyArray): Seen => { break; } } - return { claimed, approvedAt, state, threadId, stuck, finished }; + return { + claimed, + approvedAt, + state, + threadId, + launchSent, + rechecked, + worktree, + worktreeDone, + stuck, + finished, + }; }; diff --git a/apps/server/src/factory/runner/start.ts b/apps/server/src/factory/runner/start.ts index 25bfd7e5a257..3b9269182751 100644 --- a/apps/server/src/factory/runner/start.ts +++ b/apps/server/src/factory/runner/start.ts @@ -1,7 +1,8 @@ // @effect-diagnostics nodeBuiltinImport:off - a gate lock is a file, checked the way gate.ts makes it. /** * The claim: takes a `ready` issue to `factory:triage` with a run file. Ported from softfac's - * `runner/start.rs` up to the launch, which issue #16 adds. + * `runner/start.rs`, through the launch of the `/factory` session and the move to + * `factory:building`. * * Before anything is claimed, a fresh start asks GitHub who applied the `factory:ready` label and * who edited the issue text since, and builds only when an instructor applied the label and @@ -14,9 +15,15 @@ * A new attempt first finishes every older unfinished run of the issue in the ledger, so a run * a person has moved on from stops being polled. * + * The session is launched under a command id derived from the run, after the ledger says a + * launch is about to be sent, so a launch that fails on our side is found again by sending the + * same id. The issue text is checked again once the session exists: a change stops the run and + * archives the session. + * * Calling `start` again resumes. An issue in `triage` whose only assignee is the factory's own - * user is an interrupted start: a run with no record of what approved it cannot be confirmed, so - * it fails closed and goes `stuck` (no session was launched, as the record is written first). + * user is an interrupted start, and every step goes on from what the ledger records. A run with + * no record of what approved it cannot be confirmed, so it fails closed and goes `stuck` (no + * session was launched, as the record is written first). */ import * as NodeFS from "node:fs"; @@ -35,8 +42,19 @@ import { readRun } from "../ledger/read.ts"; import { newRunId, nextAttempt, runIdToString, type RunId } from "../ledger/runId.ts"; import { formatTs } from "../ledger/time.ts"; import { isUtcSecond } from "../tracker/approvalHistory.ts"; -import type { BoundGitHub, GitHubError } from "../tracker/github.ts"; -import { CANNOT_CONFIRM, CUT_SHORT_COMMENT, statusBody, stuckComment } from "./messages.ts"; +import type { BoundGitHub, GitHubError, IssueDetail } from "../tracker/github.ts"; +import { ThreadNotFoundError } from "../t3/sessions.ts"; +import type { FactorySessions, Model, SessionsError } from "../t3/sessions.ts"; +import { + CANNOT_CONFIRM, + CUT_SHORT_COMMENT, + LAUNCH_FAILED, + TEXT_CHANGED, + launchMessage, + sessionMayExistNote, + statusBody, + stuckComment, +} from "./messages.ts"; import { readProgress, type Seen } from "./progress.ts"; /** @@ -55,6 +73,13 @@ export type Tracker = Pick< | "postComment" | "upsertStatusComment" | "newestFactoryComment" + | "textCheck" +>; + +/** The subset of `FactorySessions` the claim calls. */ +export type Sessions = Pick< + FactorySessions["Service"], + "findProject" | "launch" | "thread" | "archive" | "removeWorktree" >; export interface StartContext { @@ -67,14 +92,20 @@ export interface StartContext { readonly project: ProjectConfig; readonly dataDir: string; readonly tracker: Tracker; + /** The local clone the T3 project is rooted at: `WatchedRepo.path`. */ + readonly repoPath: string; + /** The `GH_CONFIG_DIR` the launch message names. */ + readonly ghConfigDir: string; + readonly model: Model; + readonly sessions: Sessions; /** Unix epoch milliseconds. */ readonly clock: () => number; } /** What `start` did. */ export type StartOutcome = - /** The issue is claimed and its run file records it. Issue #16 launches the session from here. */ - | { readonly kind: "claimed"; readonly run: RunId } + /** The issue is claimed, its session is launched and the issue is `factory:building`. */ + | { readonly kind: "started"; readonly run: RunId; readonly threadId: string } /** The issue was not ours to take. Nothing was written anywhere. */ | { readonly kind: "skipped"; readonly reason: string } /** The issue was `ready` but not approved, so it went back to `factory:proposed` with one fixed comment, and no run was made. */ @@ -91,7 +122,12 @@ export class GateRunningError extends Schema.TaggedError()("Ga } } -export type StartError = GitHubError | LedgerError | InvalidTransition | GateRunningError; +export type StartError = + | GitHubError + | LedgerError + | InvalidTransition + | GateRunningError + | SessionsError; const mention = (issue: number) => `#${issue}`; @@ -116,20 +152,27 @@ export const start = ( } const ours = isOurs(detail.assignees, ctx.trackerUser); if (detail.state === "ready" && detail.assignees.length === 0) { - return yield* startFresh(ctx, issue); + return yield* startFresh(ctx, issue, detail); } if (detail.state === "ready") return skipped(issue, detail.assignees); - if (detail.state === "triage" && ours) return yield* resumeStart(ctx, issue); + if (detail.state === "triage" && ours) return yield* resumeStart(ctx, issue, detail); return { kind: "skipped", reason: `issue ${mention(issue)} is not labelled factory:ready` }; }); -const startFresh = (ctx: StartContext, issue: number): Effect.Effect => +const startFresh = ( + ctx: StartContext, + issue: number, + detail: IssueDetail, +): Effect.Effect => Effect.gen(function* () { // Before the claim: nothing is written for an issue that is not approved except the // explanation. const approval = yield* ctx.tracker.approval(issue, ctx.project.instructors); if (approval.kind === "refused") return yield* refuseStart(ctx, issue, approval.refusal); const { labeledAt, labeledBy } = approval; + // Also before the claim: with T3 out of reach nothing is claimed, so no issue is left in + // `triage` for a launch that cannot be sent. + const project = yield* ctx.sessions.findProject(ctx.repoPath); const run = newRunId( ctx.repo, String(issue), @@ -152,7 +195,7 @@ const startFresh = (ctx: StartContext, issue: number): Effect.Effect => +const resumeStart = ( + ctx: StartContext, + issue: number, + detail: IssueDetail, +): Effect.Effect => Effect.gen(function* () { const next = nextAttempt(ctx.dataDir, ctx.repo, String(issue)); if (next > 1) { const newest = newRunId(ctx.repo, String(issue), next - 1); const seen = readProgress((yield* ledger(() => readRun(ctx.dataDir, newest))).entries); - // A run already in `stuck` is over; it is finished below with the others. A run cut short - // before it recorded an approval or a session never launched one, so `begin` goes on from - // it to `stuck`: a stuck path that failed half way is finished in its own run. Any other - // unfinished run is left to the step that owns its state: launching a claimed run is - // issue #16's. + // A run already in `stuck` is over; it is finished below with the others. A run still in + // `triage` (or not yet in any state) was cut short, so `begin` goes on from what its + // ledger records, whatever it was approved on. Any other unfinished run is left to the + // step that owns its state. if (!seen.finished && seen.state !== "stuck") { - const unconfirmed = seen.approvedAt === null && seen.threadId === null; - if (unconfirmed && (seen.state === null || seen.state === "triage")) { + if (seen.state === null || seen.state === "triage") { const out = yield* ledger(() => Ledger.open(ctx.dataDir, newest, { clock: ctx.clock })); try { - return yield* begin(ctx, issue, newest, out); + return yield* begin(ctx, issue, detail, newest, out, undefined); } finally { out.close(); } @@ -317,7 +362,7 @@ const resumeStart = (ctx: StartContext, issue: number): Effect.Effect Ledger.create(ctx.dataDir, run, { clock: ctx.clock })); try { - return yield* begin(ctx, issue, run, out); + return yield* begin(ctx, issue, detail, run, out, undefined); } finally { out.close(); } @@ -325,15 +370,19 @@ const resumeStart = (ctx: StartContext, issue: number): Effect.Effect => Effect.gen(function* () { const seen = readProgress((yield* ledger(() => readRun(ctx.dataDir, run))).entries); @@ -352,26 +401,265 @@ const begin = ( out.append({ event: "state_changed", data: { from: "ready", to: claimed.next } }, claim), ); } - const at: Progress = { state: seen.state ?? claimed.next, threadId: seen.threadId, seen }; + const at: Progress = { + state: seen.state ?? claimed.next, + threadId: seen.threadId, + worktree: seen.worktree, + worktreeDone: seen.worktreeDone, + seen, + }; + // A decision to give up that is already in the ledger stands. + if (seen.stuck !== null) { + yield* goStuck(ctx, issue, out, at, "stuck-resumed", { reason: seen.stuck }); + return { kind: "stuck", run, reason: seen.stuck }; + } + // The record is written before any launch: with no session either, nothing was sent, so + // nothing is launched here (a session on unconfirmed text, only to archive it). if (seen.approvedAt === null && seen.threadId === null) { - yield* goStuck(ctx, issue, out, at, "approval-unconfirmed", CANNOT_CONFIRM); + yield* goStuck(ctx, issue, out, at, "approval-unconfirmed", { reason: CANNOT_CONFIRM }); return { kind: "stuck", run, reason: CANNOT_CONFIRM }; } - return { kind: "claimed", run }; + // A launch that is not in the ledger may still have started a thread: the call can fail on + // our side after T3 acted. Launching again under the same command id either starts the + // session or returns the thread T3 already started. The text is looked at after the launch + // and never before it: skipping the launch for a changed text could leave such a thread + // running with nobody to stop it. + let threadId = seen.threadId; + if (threadId === null) { + const launched = yield* launchSession(ctx, issue, detail, run, out, seen, project); + if (!launched.ok) { + yield* goStuck(ctx, issue, out, at, "launch-failed", { + reason: LAUNCH_FAILED, + detail: launched.detail, + }); + return { kind: "stuck", run, reason: launched.detail }; + } + threadId = launched.threadId; + // What the ledger now records, for a stop that has to remove the worktree while T3 may not + // report its path yet. + at.worktree = launched.worktree; + } + at.threadId = threadId; + if (seen.approvedAt === null) { + yield* goStuck(ctx, issue, out, at, "approval-unconfirmed", { reason: CANNOT_CONFIRM }); + return { kind: "stuck", run, reason: CANNOT_CONFIRM }; + } + // The session reads the issue live a few seconds after the approval, and the claim has told + // anyone watching the labels that it is starting. Check the text again now that the session + // exists: a change by someone who is not an instructor stops the run. A failed read is an + // error, and the next call checks again; the step says the check passed. + if (!seen.rechecked) { + const check = yield* ctx.tracker.textCheck(issue, seen.approvedAt, ctx.project.instructors); + // A history that cannot be judged counts as changed. + if (check !== "unchanged") { + yield* goStuck(ctx, issue, out, at, "text-changed-before-start", { reason: TEXT_CHANGED }); + return { kind: "stuck", run, reason: TEXT_CHANGED }; + } + yield* ledger(() => + out.append( + { event: "step_changed", data: { step: "approval-rechecked" } }, + makeCause("start", "approval-rechecked"), + ), + ); + } + if (at.state !== "building") yield* finishBuilding(ctx, issue, out, at, threadId); + return { kind: "started", run, threadId }; }); -/** Where the issue stands while the stuck actions are applied. */ +/** + * Where the issue stands while the stuck actions are applied. `threadId`, `worktree` and + * `worktreeDone` start from the ledger and move as this call launches and stops the session. + */ interface Progress { - readonly state: State; - readonly threadId: string | null; + state: State; + threadId: string | null; + worktree: string | null; + worktreeDone: boolean; readonly seen: Seen; } +/** The words for what a failed call said: its message when it has one. */ +const describe = (cause: unknown): string => + typeof cause === "object" && + cause !== null && + "message" in cause && + typeof cause.message === "string" + ? cause.message + : String(cause); + +type Launch = + | { readonly ok: true; readonly threadId: string; readonly worktree: string | null } + | { readonly ok: false; readonly detail: string }; + /** - * The stuck path: the actions `lifecycle.transition` asks for on a failure, in order, then the - * `state_changed` line and the summary line. A comment is skipped when the ledger already - * records it; the label, the release and the status comment are repeated safely. `code` is the - * reason of every line it writes. + * Sends the launch: `launch-sending` is written once per run, before the first call, so a + * launch that fails on our side is known to have been sent. The command id comes from the run, + * so sending it again returns the thread T3 already made. In-process, every failure of the call + * is T3's answer after the request went out: it is final, and a session may exist. A launch that + * was never sent (no project) says so only when an earlier pass did send one. + */ +const launchSession = ( + ctx: StartContext, + issue: number, + detail: IssueDetail, + run: RunId, + out: Ledger, + seen: Seen, + project: string | null | undefined, +): Effect.Effect => + Effect.gen(function* () { + const commandId = `softfac:${runIdToString(run)}:launch`; + const projectId = + project === undefined ? yield* ctx.sessions.findProject(ctx.repoPath) : project; + if (projectId === null) { + const missing = "no T3 project for this repo on this machine"; + return { + ok: false, + detail: seen.launchSent ? sessionMayExistNote(missing, commandId) : missing, + }; + } + if (!seen.launchSent) { + yield* ledger(() => + out.append( + { event: "step_changed", data: { step: "launch-sending" } }, + makeCause("start", "launch-sending"), + ), + ); + } + const launched = yield* Effect.result( + ctx.sessions.launch({ + commandId, + projectId, + title: `${ctx.repo}#${issue} ${detail.title}`, + baseRef: ctx.project.base, + startFromOrigin: false, + message: launchMessage(ctx.repo, issue, ctx.trackerUser, ctx.ghConfigDir), + model: ctx.model, + }), + ); + if (launched._tag === "Failure") { + return { + ok: false, + detail: sessionMayExistNote(describe(launched.failure.cause), commandId), + }; + } + const { threadId, worktreePath, branch } = launched.success; + yield* ledger(() => + out.append( + { + event: "session_launched", + data: { + thread_id: threadId, + worktree: worktreePath, + branch, + model_instance: ctx.model.instanceId, + model: ctx.model.model, + resumed_from: null, + }, + }, + makeCause("start", "session-launched"), + ), + ); + return { ok: true, threadId, worktree: worktreePath }; + }); + +/** The label, the status comment and the ledger line that follow a launch. Safe to repeat. */ +const finishBuilding = ( + ctx: StartContext, + issue: number, + out: Ledger, + at: Progress, + threadId: string, +): Effect.Effect => + Effect.gen(function* () { + const building = yield* Effect.fromResult(transition(at.state, { kind: "session-starting" })); + for (const action of building.actions) { + switch (action.kind) { + case "set-label": + yield* ctx.tracker.setState(issue, action.state); + break; + case "launch-session": + // Launched already. + break; + case "update-status-comment": + yield* ctx.tracker.upsertStatusComment( + issue, + ctx.trackerUser, + statusBody("building", ctx.machine, threadId, null), + ); + break; + default: + return yield* new InvalidTransition({ state: at.state, event: "session-starting" }); + } + } + yield* ledger(() => + out.append( + { event: "state_changed", data: { from: at.state, to: building.next } }, + makeCause("start", "session-started"), + ), + ); + at.state = building.next; + }); + +/** + * Stops the session before the run goes stuck: archives the thread, then removes its worktree + * unless the ledger says that is done. An archive or a read of the thread that fails fails the + * start with nothing written, and the next pass archives again. A worktree that cannot be + * removed is written down and does not stop the run. + */ +const stopSession = ( + ctx: StartContext, + out: Ledger, + at: Progress, +): Effect.Effect => + Effect.gen(function* () { + if (at.threadId === null) return; + yield* ctx.sessions.archive(at.threadId); + if (at.worktreeDone) return; + const reported = yield* ctx.sessions.thread(at.threadId).pipe( + Effect.map((thread) => thread.worktreePath), + Effect.catchIf(isThreadNotFound, () => Effect.succeed(null)), + ); + const path = reported ?? at.worktree; + if (path !== null) { + const removed = yield* Effect.result(ctx.sessions.removeWorktree(path)); + if (removed._tag === "Success") { + yield* ledger(() => + out.append( + { event: "worktree_removed", data: { path } }, + makeCause("start", "worktree-removed"), + ), + ); + } else { + yield* ledger(() => + out.append( + { + event: "worktree_remove_failed", + data: { path, error: describe(removed.failure.cause) }, + }, + makeCause("start", "worktree-remove-failed"), + ), + ); + } + } + at.worktreeDone = true; + }); + +const isThreadNotFound = Schema.is(ThreadNotFoundError); + +/** Why a run goes stuck: `reason` is the public one; `detail`, when there is one, stays in the ledger. */ +interface Stop { + readonly reason: string; + readonly detail?: string; +} + +/** + * The stuck path. The session is stopped first (see `stopSession`), so a failure of that leaves + * the issue as it was and the next pass tries again. Then the actions `lifecycle.transition` + * asks for on a failure, in order, then the `state_changed` line and the summary line. The + * comment says `stop.reason`; the ledger's `stuck` line says `stop.detail` when there is one. A + * comment is skipped when the ledger already records it; the label, the release and the status + * comment are repeated safely. `code` is the reason of every line it writes. */ const goStuck = ( ctx: StartContext, @@ -379,7 +667,7 @@ const goStuck = ( out: Ledger, at: Progress, code: string, - reason: string, + stop: Stop, ): Effect.Effect => Effect.gen(function* () { const cause = makeCause("start", code); @@ -387,15 +675,19 @@ const goStuck = ( yield* ledger(() => out.finish("stuck", null, null, cause)); return; } - const failed = yield* Effect.fromResult(transition(at.state, { kind: "failed", reason })); + yield* stopSession(ctx, out, at); + const recorded = stop.detail ?? stop.reason; + const failed = yield* Effect.fromResult( + transition(at.state, { kind: "failed", reason: recorded }), + ); let movedTo: State | null = null; let stuckPosted = at.seen.stuck !== null; for (const action of failed.actions) { switch (action.kind) { case "post-stuck-comment": if (!stuckPosted) { - yield* ctx.tracker.postComment(issue, stuckComment(reason)); - yield* ledger(() => out.append({ event: "stuck", data: { reason } }, cause)); + yield* ctx.tracker.postComment(issue, stuckComment(stop.reason)); + yield* ledger(() => out.append({ event: "stuck", data: { reason: recorded } }, cause)); stuckPosted = true; } break; From d38718702c848179d9a1b788813e3201f971d3de Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:08:05 +1000 Subject: [PATCH 04/16] test(factory): a confirmed triage run is started again by the next pass Fails: the pass never calls start for a run that is already confirmed in triage. Issue: #16 --- apps/server/src/factory/pass.test.ts | 94 +++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 3 deletions(-) diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 03b1752e6cd5..94dbbe45fc25 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -194,7 +194,11 @@ class Fake implements PassWorld { }); } if ("kind" in answer && answer.kind === "started") { - this.runs.push({ id: answer.run, state: "triage", unreadable: false, confirmed: true }); + const existing = this.runs.find((r) => runIdToString(r.id) === runIdToString(answer.run)); + // A run the pass already knew moves on to building; a new claim leaves a triage run. + if (existing !== undefined) + this.runs[this.runs.indexOf(existing)] = { ...existing, state: "building" }; + else this.runs.push({ id: answer.run, state: "triage", unreadable: false, confirmed: true }); return Effect.succeed(answer); } if ("kind" in answer) return Effect.succeed(answer); @@ -630,6 +634,90 @@ describe("which repo gets a free slot", () => { ); }); +// ---- runs that are confirmed and still in triage ---- + +describe("runs that are confirmed and still in triage", () => { + it.effect("a confirmed triage run gets start on the next pass, and the line says so", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.pushStart(`${REPO}#12`, started(12)); + const { report, lines } = yield* pass(fake, machine([REPO], 3)); + assert.deepEqual(starts(fake), [`start ${REPO}#12`]); + const line = runLine(lines, "rawphp/softfac#12 triage:"); + assert.ok(line.includes("session started, the issue is building"), line); + assert.equal(report.claimed, 0); + }), + ); + + it.effect("a confirmed run with no state yet is started too", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runs.push({ id: rid(13), state: null, unreadable: false, confirmed: true }); + fake.pushStart(`${REPO}#13`, started(13)); + yield* pass(fake, machine([REPO], 3)); + assert.deepEqual(starts(fake), [`start ${REPO}#13`]); + }), + ); + + it.effect("a triage run whose start fails records the error on that run", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.pushStart(`${REPO}#12`, githubFailure()); + const { report } = yield* pass(fake, machine([REPO], 3)); + assert.equal(report.errors, 1); + assert.equal(fake.runErrors.length, 1); + assert.deepEqual(fake.runErrors[0]!.run, rid(12)); + assert.equal(fake.runErrors[0]!.step, "start"); + }), + ); + + it.effect("a triage run that went stuck says so and names the run", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.pushStart(`${REPO}#12`, { kind: "stuck", run: rid(12), reason: "secret detail" }); + const { lines } = yield* pass(fake, machine([REPO], 3)); + const line = runLine(lines, "rawphp/softfac#12 "); + assert.ok(line.includes("stuck"), line); + assert.ok(line.includes("rawphp__softfac__12__1"), line); + assert.ok(!line.includes("secret detail"), line); + }), + ); + + it.effect("a triage run is worked before new issues are claimed", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [3]; + fake.pushStart(`${REPO}#12`, started(12)); + fake.pushStart(`${REPO}#3`, started(3)); + yield* pass(fake, machine([REPO], 5)); + assert.deepEqual(starts(fake), [`start ${REPO}#12`, `start ${REPO}#3`]); + }), + ); + + it.effect("a building run is left alone", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(40, BUILDING); + yield* pass(fake, machine([REPO], 3)); + assert.deepEqual(starts(fake), []); + }), + ); + + it.effect("a run of a repo that is not loaded is left alone", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage", "acme/other"); + const { report } = yield* pass(fake, machine([REPO], 3)); + assert.deepEqual(starts(fake), []); + assert.equal(report.errors, 0); + }), + ); +}); + // ---- claims that have no run in the ledger ---- describe("claims that have no run in the ledger", () => { @@ -647,13 +735,13 @@ describe("claims that have no run in the ledger", () => { }), ); - it.effect("a triage issue that has an unfinished run is left to that run", () => + it.effect("a triage issue that has an unfinished run is started by the run step alone", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, "triage"); fake.triage[REPO] = [12]; yield* pass(fake, machine([REPO], 3)); - assert.deepEqual(starts(fake), []); + assert.deepEqual(starts(fake), [`start ${REPO}#12`]); }), ); From 76c9b365980b18396c6204d58ee4a328c9dc1efa Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:08:25 +1000 Subject: [PATCH 05/16] fix(factory): the pass starts a confirmed triage run again Issue: #16 --- apps/server/src/factory/pass.ts | 58 ++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index e5a83f03584c..66b1beec0085 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -287,9 +287,10 @@ export const runPass = ( yield* pass.record({ event: "pass_started", data: {} }); const repos = yield* loadRepos(world, machine, pass); // Without the ledger there is no telling how many runs are active, so nothing is claimed. - const infos = yield* runs(world, repos, pass); + const infos = yield* runs(world, pass); if (infos !== null) { const known = new Known(infos); + yield* workRuns(world, repos, known, pass); yield* untrackedClaims(world, repos, known, pass); yield* unassignedClaims(world, repos, known, pass); yield* claim(world, repos, known, machine, pass); @@ -349,11 +350,7 @@ const loadRepos = ( }); /** Every unfinished run, counted; a run whose file cannot be read is an error line and counts as active. */ -const runs = ( - world: PassWorld, - repos: ReadonlyArray, - pass: Pass, -): Effect.Effect | null> => +const runs = (world: PassWorld, pass: Pass): Effect.Effect | null> => Effect.gen(function* () { const listed = yield* Effect.result(world.unfinishedRuns()); if (listed._tag === "Failure") { @@ -379,12 +376,57 @@ const runs = ( ); continue; } - // The steps that follow a run of a loaded repo (issues #16 to #20) go here. - void repos; } return listed.success; }); +/** + * Calls `start` again for each confirmed run still in `triage` (or with no state yet): `start` + * resumes it from its ledger, so a run cut short after the launch was sent is finished and its + * session never runs on untracked. A run of a repo that is not loaded is left alone. The other + * states are worked by later steps. It is not a new claim, so it is not counted as one. + */ +const workRuns = ( + world: PassWorld, + repos: ReadonlyArray, + known: Known, + pass: Pass, +): Effect.Effect => + Effect.gen(function* () { + for (const info of [...known.infos]) { + if (info.unreadable || !info.confirmed) continue; + if (info.state !== null && info.state !== "triage") continue; + const loaded = repos.find( + (l) => l.kind === "ready" && sameRepo(l.repo.watched.repo, info.id.repo), + ); + if (loaded === undefined || loaded.kind !== "ready") continue; + const repo = loaded.repo; + const name = repo.watched.repo; + const issue = Number(info.id.issue); + const notes = pass.notes(`${name}${mention(issue)}`, info.state); + const outcome = yield* Effect.result(world.start(repo, issue)); + if (outcome._tag === "Failure") { + if (outcome.failure._tag === "GateRunningError") notes.parts.push(GATE_RUNNING); + else pass.failed(notes, "start", outcome.failure); + } else { + switch (outcome.success.kind) { + case "started": + notes.parts.push("session started, the issue is building"); + break; + case "stuck": + pass.stuck(notes, runIdToString(outcome.success.run)); + break; + case "skipped": + case "refused": + notes.parts.push("not started, left to a later pass"); + break; + } + } + yield* known.refresh(world, pass, name, issue); + yield* pass.flush(notes, info.id); + } + }); + /** * The unfinished runs as this pass knows them: listed once at the start of the pass, then the * runs of an issue read again after each `start` for it, so the ledger is not listed whole again. From c20e7a48ab5e8ec8894382e02930f3c14a79a222 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:08:34 +1000 Subject: [PATCH 06/16] test(factory): a start that fails at T3 is logged as a t3 error Fails: the error is recorded as kind ledger. Issue: #16 --- apps/server/src/factory/pass.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 94dbbe45fc25..e7d4385599e9 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -25,6 +25,7 @@ import { type Report, } from "./pass.ts"; import { GateRunningError, type StartError, type StartOutcome } from "./runner/start.ts"; +import { SessionsError } from "./t3/sessions.ts"; import { GitHubFailed, type Blocker } from "./tracker/github.ts"; const REPO = "rawphp/softfac"; @@ -716,6 +717,19 @@ describe("runs that are confirmed and still in triage", () => { assert.equal(report.errors, 0); }), ); + + it.effect("a start that fails at T3 is recorded as a t3 error", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.pushStart( + `${REPO}#12`, + new SessionsError({ operation: "find-project", cause: new Error("down") }), + ); + yield* pass(fake, machine([REPO], 3)); + assert.equal(fake.runErrors[0]!.kind, "t3"); + }), + ); }); // ---- claims that have no run in the ledger ---- From 13207bed2b61a3cd08623cd59c6377ccf8afed15 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:08:38 +1000 Subject: [PATCH 07/16] fix(factory): a failed T3 call is logged as kind t3 Issue: #16 --- apps/server/src/factory/pass.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 66b1beec0085..e5242cc7e093 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -140,6 +140,8 @@ const errorKind = (error: StartError): ErrorKind => { return "tracker"; case "GateRunningError": return "gate"; + case "FactorySessionsError": + return "t3"; default: // The run's own record disagrees with what the step expected. return "ledger"; From 8e61acc4c70e9629da7af1a173e967c9bd34ffbb Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:10:25 +1000 Subject: [PATCH 08/16] refactor(factory): move the run's stop and stuck path to runner/run.ts A pure move with no behaviour change, so there is no red phase. Progress gains the step it runs for, so #17, #19 and #20 reuse the stop path under their own cause step. The ledger step names launch-sending and approval-rechecked are exported from progress.ts and used by start.ts. Issue: #16 --- apps/server/src/factory/runner/progress.ts | 7 +- apps/server/src/factory/runner/run.ts | 202 +++++++++++++++++++++ apps/server/src/factory/runner/start.ts | 182 +------------------ 3 files changed, 212 insertions(+), 179 deletions(-) create mode 100644 apps/server/src/factory/runner/run.ts diff --git a/apps/server/src/factory/runner/progress.ts b/apps/server/src/factory/runner/progress.ts index 2de38df96e00..cdec8c808814 100644 --- a/apps/server/src/factory/runner/progress.ts +++ b/apps/server/src/factory/runner/progress.ts @@ -27,7 +27,10 @@ export interface Seen { readonly finished: boolean; } -const LAUNCH_SENDING = "launch-sending"; +/** The ledger step that says a launch call is about to be sent. */ +export const LAUNCH_SENDING = "launch-sending"; +/** The ledger step that says the issue text was checked again after the launch. */ +export const APPROVAL_RECHECKED = "approval-rechecked"; const field = (data: unknown, key: string): unknown => typeof data === "object" && data !== null ? (data as Record)[key] : undefined; @@ -53,7 +56,7 @@ export const readProgress = (entries: ReadonlyArray): Seen => { if (approvedAt === null && typeof step === "string" && step.startsWith("approved ")) { approvedAt = step.slice("approved ".length); } - if (step === "approval-rechecked") rechecked = true; + if (step === APPROVAL_RECHECKED) rechecked = true; if ( typeof step === "string" && (step === LAUNCH_SENDING || step.startsWith(`${LAUNCH_SENDING} resumed-from `)) diff --git a/apps/server/src/factory/runner/run.ts b/apps/server/src/factory/runner/run.ts new file mode 100644 index 000000000000..09c4ee156de7 --- /dev/null +++ b/apps/server/src/factory/runner/run.ts @@ -0,0 +1,202 @@ +/** + * What every step that stops or finishes a run shares, ported from softfac's `runner.rs`: the + * stuck path with its session stop, and the move to `factory:building`. Each step passes its own + * `Progress.step`, which names it in the ledger's causes. + */ +import * as Effect from "effect/Effect"; +import * as Schema from "effect/Schema"; + +import { InvalidTransition, transition, type State } from "../domain/lifecycle.ts"; +import { ledgerCall as ledger, type LedgerError } from "../ledger/errors.ts"; +import { makeCause, type CauseStep } from "../ledger/events.ts"; +import type { Ledger } from "../ledger/ledger.ts"; +import type { BoundGitHub, GitHubError } from "../tracker/github.ts"; +import { ThreadNotFoundError } from "../t3/sessions.ts"; +import type { FactorySessions, SessionsError } from "../t3/sessions.ts"; +import { statusBody, stuckComment } from "./messages.ts"; +import type { Seen } from "./progress.ts"; + +/** What the stop and finish paths can fail with. */ +export type RunError = GitHubError | LedgerError | InvalidTransition | SessionsError; + +/** What the stop and finish paths call: the tracker and the sessions, whichever step runs them. */ +export interface RunContext { + readonly trackerUser: string; + readonly machine: string; + readonly tracker: Pick< + BoundGitHub, + "setState" | "unassign" | "postComment" | "upsertStatusComment" + >; + readonly sessions: Pick; +} + +/** + * Where the issue stands while the stuck actions are applied. `threadId`, `worktree` and + * `worktreeDone` start from the ledger and move as this call launches and stops the session. + */ +export interface Progress { + /** The step running, for the cause of every line written here. */ + readonly step: CauseStep; + state: State; + threadId: string | null; + worktree: string | null; + worktreeDone: boolean; + readonly seen: Seen; +} + +/** The words for what a failed call said: its message when it has one. */ +export const describe = (cause: unknown): string => + typeof cause === "object" && + cause !== null && + "message" in cause && + typeof cause.message === "string" + ? cause.message + : String(cause); + +/** The label, the status comment and the ledger line that follow a launch. Safe to repeat. */ +export const finishBuilding = ( + ctx: RunContext, + issue: number, + out: Ledger, + at: Progress, + threadId: string, +): Effect.Effect => + Effect.gen(function* () { + const building = yield* Effect.fromResult(transition(at.state, { kind: "session-starting" })); + for (const action of building.actions) { + switch (action.kind) { + case "set-label": + yield* ctx.tracker.setState(issue, action.state); + break; + case "launch-session": + // Launched already. + break; + case "update-status-comment": + yield* ctx.tracker.upsertStatusComment( + issue, + ctx.trackerUser, + statusBody("building", ctx.machine, threadId, null), + ); + break; + default: + return yield* new InvalidTransition({ state: at.state, event: "session-starting" }); + } + } + yield* ledger(() => + out.append( + { event: "state_changed", data: { from: at.state, to: building.next } }, + makeCause(at.step, "session-started"), + ), + ); + at.state = building.next; + }); + +/** + * Stops the session before the run goes stuck: archives the thread, then removes its worktree + * unless the ledger says that is done. An archive or a read of the thread that fails fails the + * start with nothing written, and the next pass archives again. A worktree that cannot be + * removed is written down and does not stop the run. + */ +const stopSession = (ctx: RunContext, out: Ledger, at: Progress): Effect.Effect => + Effect.gen(function* () { + if (at.threadId === null) return; + yield* ctx.sessions.archive(at.threadId); + if (at.worktreeDone) return; + const reported = yield* ctx.sessions.thread(at.threadId).pipe( + Effect.map((thread) => thread.worktreePath), + Effect.catchIf(isThreadNotFound, () => Effect.succeed(null)), + ); + const path = reported ?? at.worktree; + if (path !== null) { + const removed = yield* Effect.result(ctx.sessions.removeWorktree(path)); + if (removed._tag === "Success") { + yield* ledger(() => + out.append( + { event: "worktree_removed", data: { path } }, + makeCause(at.step, "worktree-removed"), + ), + ); + } else { + yield* ledger(() => + out.append( + { + event: "worktree_remove_failed", + data: { path, error: describe(removed.failure.cause) }, + }, + makeCause(at.step, "worktree-remove-failed"), + ), + ); + } + } + at.worktreeDone = true; + }); + +const isThreadNotFound = Schema.is(ThreadNotFoundError); + +/** Why a run goes stuck: `reason` is the public one; `detail`, when there is one, stays in the ledger. */ +export interface Stop { + readonly reason: string; + readonly detail?: string; +} + +/** + * The stuck path. The session is stopped first (see `stopSession`), so a failure of that leaves + * the issue as it was and the next pass tries again. Then the actions `lifecycle.transition` + * asks for on a failure, in order, then the `state_changed` line and the summary line. The + * comment says `stop.reason`; the ledger's `stuck` line says `stop.detail` when there is one. A + * comment is skipped when the ledger already records it; the label, the release and the status + * comment are repeated safely. `code` is the reason of every line it writes. + */ +export const goStuck = ( + ctx: RunContext, + issue: number, + out: Ledger, + at: Progress, + code: string, + stop: Stop, +): Effect.Effect => + Effect.gen(function* () { + const cause = makeCause(at.step, code); + if (at.state === "stuck") { + yield* ledger(() => out.finish("stuck", null, null, cause)); + return; + } + yield* stopSession(ctx, out, at); + const recorded = stop.detail ?? stop.reason; + const failed = yield* Effect.fromResult( + transition(at.state, { kind: "failed", reason: recorded }), + ); + let movedTo: State | null = null; + let stuckPosted = at.seen.stuck !== null; + for (const action of failed.actions) { + switch (action.kind) { + case "post-stuck-comment": + if (!stuckPosted) { + yield* ctx.tracker.postComment(issue, stuckComment(stop.reason)); + yield* ledger(() => out.append({ event: "stuck", data: { reason: recorded } }, cause)); + stuckPosted = true; + } + break; + case "set-label": + yield* ctx.tracker.setState(issue, action.state); + movedTo = action.state; + break; + case "unassign": + yield* ctx.tracker.unassign(issue, ctx.trackerUser); + break; + case "update-status-comment": + yield* ctx.tracker.upsertStatusComment( + issue, + ctx.trackerUser, + statusBody(movedTo ?? at.state, ctx.machine, at.threadId, null), + ); + break; + default: + return yield* new InvalidTransition({ state: at.state, event: "failed" }); + } + } + yield* ledger(() => + out.append({ event: "state_changed", data: { from: at.state, to: failed.next } }, cause), + ); + yield* ledger(() => out.finish("stuck", null, null, cause)); + }); diff --git a/apps/server/src/factory/runner/start.ts b/apps/server/src/factory/runner/start.ts index 3b9269182751..59d5efa4b91f 100644 --- a/apps/server/src/factory/runner/start.ts +++ b/apps/server/src/factory/runner/start.ts @@ -43,7 +43,6 @@ import { newRunId, nextAttempt, runIdToString, type RunId } from "../ledger/runI import { formatTs } from "../ledger/time.ts"; import { isUtcSecond } from "../tracker/approvalHistory.ts"; import type { BoundGitHub, GitHubError, IssueDetail } from "../tracker/github.ts"; -import { ThreadNotFoundError } from "../t3/sessions.ts"; import type { FactorySessions, Model, SessionsError } from "../t3/sessions.ts"; import { CANNOT_CONFIRM, @@ -55,7 +54,8 @@ import { statusBody, stuckComment, } from "./messages.ts"; -import { readProgress, type Seen } from "./progress.ts"; +import { APPROVAL_RECHECKED, LAUNCH_SENDING, readProgress, type Seen } from "./progress.ts"; +import { describe, finishBuilding, goStuck, type Progress } from "./run.ts"; /** * What the claim asks of the tracker: the subset of `makeGitHub` it calls, with the process @@ -402,6 +402,7 @@ const begin = ( ); } const at: Progress = { + step: "start", state: seen.state ?? claimed.next, threadId: seen.threadId, worktree: seen.worktree, @@ -457,7 +458,7 @@ const begin = ( } yield* ledger(() => out.append( - { event: "step_changed", data: { step: "approval-rechecked" } }, + { event: "step_changed", data: { step: APPROVAL_RECHECKED } }, makeCause("start", "approval-rechecked"), ), ); @@ -466,27 +467,6 @@ const begin = ( return { kind: "started", run, threadId }; }); -/** - * Where the issue stands while the stuck actions are applied. `threadId`, `worktree` and - * `worktreeDone` start from the ledger and move as this call launches and stops the session. - */ -interface Progress { - state: State; - threadId: string | null; - worktree: string | null; - worktreeDone: boolean; - readonly seen: Seen; -} - -/** The words for what a failed call said: its message when it has one. */ -const describe = (cause: unknown): string => - typeof cause === "object" && - cause !== null && - "message" in cause && - typeof cause.message === "string" - ? cause.message - : String(cause); - type Launch = | { readonly ok: true; readonly threadId: string; readonly worktree: string | null } | { readonly ok: false; readonly detail: string }; @@ -521,7 +501,7 @@ const launchSession = ( if (!seen.launchSent) { yield* ledger(() => out.append( - { event: "step_changed", data: { step: "launch-sending" } }, + { event: "step_changed", data: { step: LAUNCH_SENDING } }, makeCause("start", "launch-sending"), ), ); @@ -563,158 +543,6 @@ const launchSession = ( return { ok: true, threadId, worktree: worktreePath }; }); -/** The label, the status comment and the ledger line that follow a launch. Safe to repeat. */ -const finishBuilding = ( - ctx: StartContext, - issue: number, - out: Ledger, - at: Progress, - threadId: string, -): Effect.Effect => - Effect.gen(function* () { - const building = yield* Effect.fromResult(transition(at.state, { kind: "session-starting" })); - for (const action of building.actions) { - switch (action.kind) { - case "set-label": - yield* ctx.tracker.setState(issue, action.state); - break; - case "launch-session": - // Launched already. - break; - case "update-status-comment": - yield* ctx.tracker.upsertStatusComment( - issue, - ctx.trackerUser, - statusBody("building", ctx.machine, threadId, null), - ); - break; - default: - return yield* new InvalidTransition({ state: at.state, event: "session-starting" }); - } - } - yield* ledger(() => - out.append( - { event: "state_changed", data: { from: at.state, to: building.next } }, - makeCause("start", "session-started"), - ), - ); - at.state = building.next; - }); - -/** - * Stops the session before the run goes stuck: archives the thread, then removes its worktree - * unless the ledger says that is done. An archive or a read of the thread that fails fails the - * start with nothing written, and the next pass archives again. A worktree that cannot be - * removed is written down and does not stop the run. - */ -const stopSession = ( - ctx: StartContext, - out: Ledger, - at: Progress, -): Effect.Effect => - Effect.gen(function* () { - if (at.threadId === null) return; - yield* ctx.sessions.archive(at.threadId); - if (at.worktreeDone) return; - const reported = yield* ctx.sessions.thread(at.threadId).pipe( - Effect.map((thread) => thread.worktreePath), - Effect.catchIf(isThreadNotFound, () => Effect.succeed(null)), - ); - const path = reported ?? at.worktree; - if (path !== null) { - const removed = yield* Effect.result(ctx.sessions.removeWorktree(path)); - if (removed._tag === "Success") { - yield* ledger(() => - out.append( - { event: "worktree_removed", data: { path } }, - makeCause("start", "worktree-removed"), - ), - ); - } else { - yield* ledger(() => - out.append( - { - event: "worktree_remove_failed", - data: { path, error: describe(removed.failure.cause) }, - }, - makeCause("start", "worktree-remove-failed"), - ), - ); - } - } - at.worktreeDone = true; - }); - -const isThreadNotFound = Schema.is(ThreadNotFoundError); - -/** Why a run goes stuck: `reason` is the public one; `detail`, when there is one, stays in the ledger. */ -interface Stop { - readonly reason: string; - readonly detail?: string; -} - -/** - * The stuck path. The session is stopped first (see `stopSession`), so a failure of that leaves - * the issue as it was and the next pass tries again. Then the actions `lifecycle.transition` - * asks for on a failure, in order, then the `state_changed` line and the summary line. The - * comment says `stop.reason`; the ledger's `stuck` line says `stop.detail` when there is one. A - * comment is skipped when the ledger already records it; the label, the release and the status - * comment are repeated safely. `code` is the reason of every line it writes. - */ -const goStuck = ( - ctx: StartContext, - issue: number, - out: Ledger, - at: Progress, - code: string, - stop: Stop, -): Effect.Effect => - Effect.gen(function* () { - const cause = makeCause("start", code); - if (at.state === "stuck") { - yield* ledger(() => out.finish("stuck", null, null, cause)); - return; - } - yield* stopSession(ctx, out, at); - const recorded = stop.detail ?? stop.reason; - const failed = yield* Effect.fromResult( - transition(at.state, { kind: "failed", reason: recorded }), - ); - let movedTo: State | null = null; - let stuckPosted = at.seen.stuck !== null; - for (const action of failed.actions) { - switch (action.kind) { - case "post-stuck-comment": - if (!stuckPosted) { - yield* ctx.tracker.postComment(issue, stuckComment(stop.reason)); - yield* ledger(() => out.append({ event: "stuck", data: { reason: recorded } }, cause)); - stuckPosted = true; - } - break; - case "set-label": - yield* ctx.tracker.setState(issue, action.state); - movedTo = action.state; - break; - case "unassign": - yield* ctx.tracker.unassign(issue, ctx.trackerUser); - break; - case "update-status-comment": - yield* ctx.tracker.upsertStatusComment( - issue, - ctx.trackerUser, - statusBody(movedTo ?? at.state, ctx.machine, at.threadId, null), - ); - break; - default: - return yield* new InvalidTransition({ state: at.state, event: "failed" }); - } - } - yield* ledger(() => - out.append({ event: "state_changed", data: { from: at.state, to: failed.next } }, cause), - ); - yield* ledger(() => out.finish("stuck", null, null, cause)); - }); - /** * Finishes every unfinished run of the issue before attempt `below`, by its last state, so it is * not polled again and cannot write to an issue a newer attempt now owns. While the gate of any From 9f46619cd9326779846228a5a979e36c462df10e Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:19:26 +1000 Subject: [PATCH 09/16] test(factory): pin the whole outcome in two stuck-path start tests Two review nits: the no-note test now asserts the stuck outcome and its reason, and the failing-unassign test asserts the exact ledger lines. Both pass already; no behaviour changes. Issue: #16 --- apps/server/src/factory/runner/start.test.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index 25474e25feb0..cc2ae256a1ad 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -1052,9 +1052,13 @@ describe("start: the launch", () => { const t3 = fakeSessions(dataDir, { project: null }); const error = yield* Effect.flip(startIn(dataDir, tracker, t3.sessions)); assert.strictEqual(error._tag, "GitHubFailed"); - const events = stepsOf(dataDir); - assert.strictEqual(events.length, 4); - assert.notStrictEqual(events.at(-1), "finished"); + // The comment and the label went out and are on record; the summary line is not written. + assert.deepStrictEqual(stepsOf(dataDir), [ + "step_changed", + "claimed", + "state_changed", + "stuck", + ]); }), ); }); @@ -1096,8 +1100,7 @@ describe("start: a launch that an earlier pass may have sent", () => { const { tracker } = fakeTracker({}); const t3 = fakeSessions(dataDir, { project: null }); const outcome = yield* startIn(dataDir, tracker, t3.sessions); - const reason = outcome.kind === "stuck" ? outcome.reason : ""; - assert.ok(!reason.includes("may have started"), reason); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: NO_PROJECT }); }), ); }); From dafc3d7b5c2f053f34edcc1f25058f2eb6495a3f Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:40:12 +1000 Subject: [PATCH 10/16] fix(factory): review-gate round 2: the run steps resume and never claim The pass's run and untracked-claim steps now call `resume`, which goes on with a claim already made and never takes the fresh-claim branch: an issue put back to factory:ready is left to the claim step, which checks the pause, the limit and the blockers (f1, f2). `resume` also finishes a stuck path cut short after its label moved: an issue labelled factory:stuck whose newest run is unfinished in triage with a stuck line in its ledger. Before, start skipped it every pass and the run held a slot forever (f10). A pass that runs past 30 minutes is stopped and logged, so one hung call (an in-process T3 launch, say) no longer stops every later pass. The bound is above any single call's own limit (gh 60 s, worktree removal 10 min); a launch cut short resumes on the next pass under the same command id (f11). Tests for the stop path's worktree-remove failure and a thread T3 no longer knows (f5). Drops imports the move to run.ts left unused. --- apps/server/src/factory/index.ts | 41 +++--- apps/server/src/factory/loop.test.ts | 40 +++++- apps/server/src/factory/loop.ts | 17 ++- apps/server/src/factory/pass.test.ts | 51 ++++--- apps/server/src/factory/pass.ts | 30 +++-- apps/server/src/factory/runner/start.test.ts | 135 ++++++++++++++++++- apps/server/src/factory/runner/start.ts | 75 +++++++++-- 7 files changed, 318 insertions(+), 71 deletions(-) diff --git a/apps/server/src/factory/index.ts b/apps/server/src/factory/index.ts index 351b4aded9e3..c2ace0518b24 100644 --- a/apps/server/src/factory/index.ts +++ b/apps/server/src/factory/index.ts @@ -30,7 +30,14 @@ import { runPass, type PassError, type PassWorld, type Repo } from "./pass.ts"; import * as Process from "./process.ts"; import { FactoryProcess } from "./process.ts"; import { CUT_SHORT_COMMENT } from "./runner/messages.ts"; -import { start, utcSecond, type Sessions, type Tracker } from "./runner/start.ts"; +import { + resume, + start, + utcSecond, + type Sessions, + type StartContext, + type Tracker, +} from "./runner/start.ts"; import * as Paths from "./t3/paths.ts"; import * as T3Sessions from "./t3/sessions.ts"; import { bindProcess, makeGitHub, type GitHubError, type IssueSummary } from "./tracker/github.ts"; @@ -120,6 +127,19 @@ export const liveWorld = ( const trackerFor = (watched: WatchedRepo): Tracker => bindProcess(githubFor(watched), process); const tracker = (effect: Effect.Effect) => withProcess(effect).pipe(Effect.mapError(asPassError("tracker"))); + const startContext = (repo: Repo): StartContext => ({ + repo: repo.watched.repo, + trackerUser: repo.trackerUser, + machine: host, + project: repo.project, + dataDir, + tracker: trackerFor(repo.watched), + repoPath: repo.watched.path, + ghConfigDir: machine.ghConfigDir, + model: { instanceId: machine.modelInstance, model: machine.model }, + sessions, + clock, + }); return { project: (watched) => withProcess(projectFromBase(githubFor(watched))), trackerUser: (watched) => @@ -166,23 +186,8 @@ export const liveWorld = ( withProcess(githubFor(repo.watched).textBlockers(issue)).pipe( Effect.mapError(asPassError("tracker")), ), - start: (repo: Repo, issue: number) => - start( - { - repo: repo.watched.repo, - trackerUser: repo.trackerUser, - machine: host, - project: repo.project, - dataDir, - tracker: trackerFor(repo.watched), - repoPath: repo.watched.path, - ghConfigDir: machine.ghConfigDir, - model: { instanceId: machine.modelInstance, model: machine.model }, - sessions, - clock, - }, - issue, - ), + start: (repo: Repo, issue: number) => start(startContext(repo), issue), + resume: (repo: Repo, issue: number) => resume(startContext(repo), issue), gateRunning: (run) => NodeFS.existsSync(gateLockFile(dataDir, run)), recordError: (run, step, kind, message) => ledger(() => { diff --git a/apps/server/src/factory/loop.test.ts b/apps/server/src/factory/loop.test.ts index c0ac9bc0cc48..031205503753 100644 --- a/apps/server/src/factory/loop.test.ts +++ b/apps/server/src/factory/loop.test.ts @@ -1,11 +1,12 @@ import { assert, it } from "@effect/vitest"; import * as Deferred from "effect/Deferred"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; import * as TestClock from "effect/testing/TestClock"; import { describe } from "vite-plus/test"; -import { runLoop } from "./loop.ts"; +import { PASS_TIMEOUT, runLoop } from "./loop.ts"; import type { Report } from "./pass.ts"; const report = (n: number): Report => ({ runs: n, claimed: 0, errors: 0, attention: 0 }); @@ -100,6 +101,43 @@ describe("the pass loop", () => { ), ); + it.effect("a pass that hangs is stopped at the pass timeout and the loop goes on", () => + Effect.scoped( + Effect.gen(function* () { + const never = yield* Deferred.make(); + const { calls, onePass } = counting({ at: 1, on: never }); + const loop = yield* runLoop(onePass, 60); + const first = yield* Effect.forkChild(loop.awaitPass, { startImmediately: true }); + yield* TestClock.adjust(Duration.subtract(PASS_TIMEOUT, Duration.seconds(1))); + assert.strictEqual(calls.n, 1, "the hung pass is still running"); + yield* TestClock.adjust("1 second"); + assert.strictEqual(yield* Fiber.join(first), null); + const second = yield* Effect.forkChild(loop.awaitPass, { startImmediately: true }); + yield* TestClock.adjust("60 seconds"); + assert.deepStrictEqual(yield* Fiber.join(second), report(2)); + }), + ), + ); + + it.effect("a long pass that finishes before the pass timeout keeps its report", () => + Effect.scoped( + Effect.gen(function* () { + const on = yield* Deferred.make(); + const { calls, onePass } = counting({ at: 1, on }); + const loop = yield* runLoop(onePass, 60); + const first = yield* Effect.forkChild(loop.awaitPass, { startImmediately: true }); + yield* TestClock.adjust(Duration.subtract(PASS_TIMEOUT, Duration.seconds(1))); + yield* Deferred.succeed(on, undefined); + assert.deepStrictEqual(yield* Fiber.join(first), report(1)); + // The timeout that would have fired is gone: the next pass comes on the poll alone. + yield* TestClock.adjust("59 seconds"); + assert.strictEqual(calls.n, 1); + yield* TestClock.adjust("1 second"); + assert.strictEqual(calls.n, 2); + }), + ), + ); + it.effect("the loop stops with its scope", () => Effect.gen(function* () { const { calls, onePass } = counting(); diff --git a/apps/server/src/factory/loop.ts b/apps/server/src/factory/loop.ts index 35898f785c99..a6451d32cd5c 100644 --- a/apps/server/src/factory/loop.ts +++ b/apps/server/src/factory/loop.ts @@ -2,7 +2,8 @@ * The pass loop: one pass when the factory starts and one every `poll_seconds` after, with a * wake that starts the next pass now. Passes never overlap: one fiber runs them one after the * other, and a wake that arrives during a pass makes the next pass follow it at once, however - * many wakes arrived. + * many wakes arrived. A pass that runs past `PASS_TIMEOUT` is stopped and logged, so one hung + * call cannot stop every later pass; the next pass goes on from the ledger. */ import * as Deferred from "effect/Deferred"; import * as Duration from "effect/Duration"; @@ -15,10 +16,16 @@ import type { Report } from "./pass.ts"; export interface Loop { /** Starts the next pass now when none is running, or right after the one that is. */ readonly wake: Effect.Effect; - /** The report of the next pass to finish; null when that pass died. */ + /** The report of the next pass to finish; null when that pass died or timed out. */ readonly awaitPass: Effect.Effect; } +/** + * Longer than any one call a pass makes is allowed (a `gh` call 60 seconds, a worktree removal + * 10 minutes), so only a pass that is hung is stopped. + */ +export const PASS_TIMEOUT = Duration.minutes(30); + /** Runs `onePass` on the loop's schedule in a fiber of the scope. The first pass starts at once. */ export const runLoop = ( onePass: Effect.Effect, @@ -31,6 +38,12 @@ export const runLoop = ( for (;;) { yield* latch.close; const report = yield* onePass.pipe( + Effect.timeoutOption(PASS_TIMEOUT), + Effect.flatMap((done) => + done._tag === "Some" + ? Effect.succeed(done.value) + : Effect.logError("factory pass timed out and was stopped").pipe(Effect.as(null)), + ), Effect.catchCause((cause) => Effect.logError("factory pass failed", cause).pipe(Effect.as(null)), ), diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index e7d4385599e9..6e7944e6ab3f 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -183,8 +183,17 @@ class Fake implements PassWorld { } start(repo: Repo, issue: number) { + return this.answerStart("start", repo, issue); + } + + resume(repo: Repo, issue: number) { + return this.answerStart("resume", repo, issue); + } + + /** Both calls share one script per issue; `calls` says which was made. */ + private answerStart(call: "start" | "resume", repo: Repo, issue: number) { const key = `${repo.watched.repo}#${issue}`; - this.calls.push(`start ${key}`); + this.calls.push(`${call} ${key}`); const answer = this.starts[key]?.shift() ?? { kind: "skipped", reason: "scripted" }; if (this.startLeavesRun.includes(issue)) { this.runs.push({ @@ -249,7 +258,9 @@ const pass = ( return { report, lines }; }); -const starts = (fake: Fake) => fake.calls.filter((c) => c.startsWith("start ")); +/** The `start` and `resume` calls, in order. */ +const starts = (fake: Fake) => + fake.calls.filter((c) => c.startsWith("start ") || c.startsWith("resume ")); const releases = (fake: Fake) => fake.calls.filter((c) => c.startsWith("release ")); const runLine = (lines: ReadonlyArray, prefix: string): string => { @@ -638,26 +649,26 @@ describe("which repo gets a free slot", () => { // ---- runs that are confirmed and still in triage ---- describe("runs that are confirmed and still in triage", () => { - it.effect("a confirmed triage run gets start on the next pass, and the line says so", () => + it.effect("a confirmed triage run is resumed on the next pass, and the line says so", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, "triage"); fake.pushStart(`${REPO}#12`, started(12)); const { report, lines } = yield* pass(fake, machine([REPO], 3)); - assert.deepEqual(starts(fake), [`start ${REPO}#12`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); const line = runLine(lines, "rawphp/softfac#12 triage:"); assert.ok(line.includes("session started, the issue is building"), line); assert.equal(report.claimed, 0); }), ); - it.effect("a confirmed run with no state yet is started too", () => + it.effect("a confirmed run with no state yet is resumed too", () => Effect.gen(function* () { const fake = new Fake(); fake.runs.push({ id: rid(13), state: null, unreadable: false, confirmed: true }); fake.pushStart(`${REPO}#13`, started(13)); yield* pass(fake, machine([REPO], 3)); - assert.deepEqual(starts(fake), [`start ${REPO}#13`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#13`]); }), ); @@ -695,7 +706,7 @@ describe("runs that are confirmed and still in triage", () => { fake.pushStart(`${REPO}#12`, started(12)); fake.pushStart(`${REPO}#3`, started(3)); yield* pass(fake, machine([REPO], 5)); - assert.deepEqual(starts(fake), [`start ${REPO}#12`, `start ${REPO}#3`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#3`]); }), ); @@ -735,13 +746,13 @@ describe("runs that are confirmed and still in triage", () => { // ---- claims that have no run in the ledger ---- describe("claims that have no run in the ledger", () => { - it.effect("a triage issue of ours with no run gets start, and is not a new claim", () => + it.effect("a triage issue of ours with no run is resumed, and is not a new claim", () => Effect.gen(function* () { const fake = new Fake(); fake.triage[REPO] = [7]; fake.pushStart(`${REPO}#7`, { kind: "stuck", run: rid(7), reason: "could not confirm" }); const { report, lines } = yield* pass(fake, machine([REPO], 1)); - assert.deepEqual(starts(fake), [`start ${REPO}#7`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#7`]); const line = runLine(lines, "rawphp/softfac#7 "); assert.ok(line.startsWith("rawphp/softfac#7 triage: no run on record, stuck"), line); assert.ok(line.includes("rawphp__softfac__7__1"), line); @@ -749,17 +760,17 @@ describe("claims that have no run in the ledger", () => { }), ); - it.effect("a triage issue that has an unfinished run is started by the run step alone", () => + it.effect("a triage issue that has an unfinished run is resumed by the run step alone", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, "triage"); fake.triage[REPO] = [12]; yield* pass(fake, machine([REPO], 3)); - assert.deepEqual(starts(fake), [`start ${REPO}#12`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); }), ); - it.effect("a triage issue whose only run is not confirmed gets start again", () => + it.effect("a triage issue whose only run is not confirmed is resumed again", () => Effect.gen(function* () { // A stuck path that failed half way leaves a run with no approval behind; start finishes it. const fake = new Fake(); @@ -767,7 +778,7 @@ describe("claims that have no run in the ledger", () => { fake.triage[REPO] = [12]; fake.pushStart(`${REPO}#12`, { kind: "stuck", run: rid(12), reason: "could not confirm" }); const { lines } = yield* pass(fake, machine([REPO], 3)); - assert.deepEqual(starts(fake), [`start ${REPO}#12`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); assert.ok(runLine(lines, "rawphp/softfac#12 ").includes("stuck"), lines.join("\n")); }), ); @@ -783,17 +794,17 @@ describe("claims that have no run in the ledger", () => { }), ); - it.effect("a triage issue with no run is started even when every slot is taken", () => + it.effect("a triage issue with no run is resumed even when every slot is taken", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(40, BUILDING); fake.triage[REPO] = [7]; yield* pass(fake, machine([REPO], 1)); - assert.deepEqual(starts(fake), [`start ${REPO}#7`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#7`]); }), ); - it.effect("a triage issue is started before new issues are claimed", () => + it.effect("a triage issue is resumed before new issues are claimed", () => Effect.gen(function* () { const fake = new Fake(); fake.triage[REPO] = [7]; @@ -801,7 +812,7 @@ describe("claims that have no run in the ledger", () => { fake.pushStart(`${REPO}#7`, started(7)); fake.pushStart(`${REPO}#3`, started(3)); yield* pass(fake, machine([REPO], 5)); - assert.deepEqual(starts(fake), [`start ${REPO}#7`, `start ${REPO}#3`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#7`, `start ${REPO}#3`]); }), ); @@ -827,7 +838,7 @@ describe("claims that have no run in the ledger", () => { fake.triage[REPO] = "error"; fake.triage[OTHER] = [4]; const { report, lines } = yield* pass(fake, machine([REPO, OTHER], 1)); - assert.deepEqual(starts(fake), [`start ${OTHER}#4`]); + assert.deepEqual(starts(fake), [`resume ${OTHER}#4`]); assert.equal(report.errors, 1); assert.ok( lines.some((l) => l.startsWith("error: rawphp/softfac: cannot list the triage issues")), @@ -1281,7 +1292,7 @@ describe("the hand pause", () => { fake.unassigned[REPO] = [{ issue: 8, state: "triage" }]; pausedByHand(fake); yield* pass(fake, machine([REPO], 5)); - assert.deepEqual(starts(fake), [`start ${REPO}#7`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#7`]); assert.deepEqual(releases(fake), [`release ${REPO}#8 triage`]); }), ); @@ -1530,7 +1541,7 @@ describe("reading the ledger", () => { return listed(only); }; const { report, lines } = yield* pass(fake, machine([REPO], 5)); - assert.deepEqual(starts(fake), [`start ${REPO}#7`, `start ${REPO}#8`]); + assert.deepEqual(starts(fake), [`resume ${REPO}#7`, `resume ${REPO}#8`]); assert.equal( lines.filter((l) => l.startsWith("error: cannot read the run ledger")).length, 1, diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index e5242cc7e093..4618f7ba9ae0 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -74,6 +74,8 @@ export interface PassWorld { ) => Effect.Effect, PassError>; /** The claim: `runner/start.ts`. */ readonly start: (repo: Repo, issue: number) => Effect.Effect; + /** The claim gone on with, never made: `resume` in `runner/start.ts`. */ + readonly resume: (repo: Repo, issue: number) => Effect.Effect; /** Whether the run's gate is running now. */ readonly gateRunning: (run: RunId) => boolean; /** Appends an `error` line to the run's own ledger. */ @@ -383,10 +385,11 @@ const runs = (world: PassWorld, pass: Pass): Effect.Effect i.unreadable || i.confirmed)) continue; const notes = pass.notes(`${name}${mention(issue)}`, "triage"); - const run = yield* noteStart(world, pass, known, notes, repo, issue, { + const run = yield* noteStart(world, pass, known, notes, repo, issue, "resume", { claimed: (run) => `no run on record, claimed in run ${run}`, notTaken: "no run on record, not ours to resume", stuck: "no run on record", @@ -576,10 +579,10 @@ interface StartWords { } /** - * Calls `start` for the issue and puts what it did on `notes`: the run it names, when it made - * one, is the run the notes' errors belong to. The issue's runs are read again into `known` - * after the call. A refused claim is a `claim_skipped`; a claim counts only when the issue was - * `ready`. + * Calls `call` (`start`, or `resume` for a claim already made) for the issue and puts what it + * did on `notes`: the run it names, when it made one, is the run the notes' errors belong to. + * The issue's runs are read again into `known` after the call. A refused claim is a + * `claim_skipped`; a claim counts only when the issue was `ready`. */ const noteStart = ( world: PassWorld, @@ -588,12 +591,13 @@ const noteStart = ( notes: Notes, repo: Repo, issue: number, + call: "start" | "resume", words: StartWords, ): Effect.Effect => Effect.gen(function* () { const name = repo.watched.repo; const before = newestRun(known.of(name, issue)); - const outcome = yield* Effect.result(world.start(repo, issue)); + const outcome = yield* Effect.result(world[call](repo, issue)); if (outcome._tag === "Failure" && outcome.failure._tag === "GateRunningError") { // Nothing was called or written. notes.parts.push(GATE_RUNNING); @@ -759,7 +763,7 @@ const claim = ( }); continue; } - const run = yield* noteStart(world, pass, known, notes, repo, issue, { + const run = yield* noteStart(world, pass, known, notes, repo, issue, "start", { claimed: (run) => `claimed, session started in run ${run}`, notTaken: "not claimed", stuck: "claimed, but the launch failed", diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index cc2ae256a1ad..2270b50f4a37 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -20,6 +20,7 @@ import { parseRunId, runPath, type RunId } from "../ledger/runId.ts"; import { T0, useTempDirs } from "../ledger/testSupport.ts"; import { SessionsError, + ThreadNotFoundError, type LaunchRequest, type Launched, type Model, @@ -44,6 +45,7 @@ import { } from "./messages.ts"; import { GateRunningError, + resume, start, type Sessions, type StartContext, @@ -156,6 +158,7 @@ const fakeTracker = (script: { }; const isSessionsError = Schema.is(SessionsError); +const isThreadNotFound = Schema.is(ThreadNotFoundError); const MODEL: Model = { instanceId: "claudeAgent", model: "claude-opus-5-5" }; const LAUNCH_ID = "softfac:rawphp__softfac__7__1:launch"; @@ -172,7 +175,8 @@ const fakeSessions = ( project?: string | null | SessionsError; launches?: Array; archive?: Array; - thread?: Thread; + thread?: Thread | ThreadNotFoundError; + removeWorktree?: SessionsError; }, ) => { const calls: Array = []; @@ -208,9 +212,11 @@ const fakeSessions = ( }, thread: (threadId) => { calls.push(`thread ${threadId}`); - return Effect.succeed( - script.thread ?? { worktreePath: WORKTREE, branch: "t3code/abc", archived: false }, - ); + return isThreadNotFound(script.thread) + ? Effect.fail(script.thread) + : Effect.succeed( + script.thread ?? { worktreePath: WORKTREE, branch: "t3code/abc", archived: false }, + ); }, archive: (threadId) => { calls.push(`archive ${threadId}`); @@ -219,7 +225,7 @@ const fakeSessions = ( }, removeWorktree: (path) => { calls.push(`removeWorktree ${path}`); - return Effect.void; + return script.removeWorktree === undefined ? Effect.void : Effect.fail(script.removeWorktree); }, }; return { sessions, calls, launchRequests }; @@ -1180,3 +1186,122 @@ describe("start: a resume", () => { }), ); }); + +describe("start: stopping a session that goes stuck", () => { + it.effect("a worktree that cannot be removed is written down and the run still goes stuck", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const { tracker, calls } = fakeTracker({ textChecks: ["changed"] }); + const t3 = fakeSessions(dataDir, { + removeWorktree: new SessionsError({ + operation: "remove-worktree", + cause: new Error("disk busy"), + }), + }); + const outcome = yield* startIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: TEXT_CHANGED }); + assert.deepStrictEqual(calls.slice(4), stuckCalls(TEXT_CHANGED, "thread-1")); + assert.deepStrictEqual(entriesOf(dataDir), [ + ...startedEvents().slice(0, 5), + { + event: "worktree_remove_failed", + data: { path: WORKTREE, error: "disk busy" }, + cause: cause("worktree-remove-failed"), + }, + ...stuckTail(TEXT_CHANGED, "text-changed-before-start"), + ]); + }), + ); + + it.effect("a thread T3 no longer knows is stopped at the worktree the ledger records", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 5)); + const { tracker } = fakeTracker({ + issues: [detail("triage", [USER])], + textChecks: ["changed"], + }); + const t3 = fakeSessions(dataDir, { + thread: new ThreadNotFoundError({ threadId: "thread-1" }), + }); + const outcome = yield* resume(ctxFor(dataDir, tracker, t3.sessions), 7); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: TEXT_CHANGED }); + assert.deepStrictEqual(t3.calls, [ + "archive thread-1", + "thread thread-1", + `removeWorktree ${WORKTREE}`, + ]); + assert.deepStrictEqual(entriesOf(dataDir).slice(5, 6), [ + { event: "worktree_removed", data: { path: WORKTREE }, cause: cause("worktree-removed") }, + ]); + }), + ); +}); + +describe("resume", () => { + const resumeIn = (dataDir: string, tracker: Tracker, sessions?: Sessions) => + resume(ctxFor(dataDir, tracker, sessions ?? fakeSessions(dataDir, {}).sessions), 7); + + it.effect("goes on with a triage run of ours as start does", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 3)); + const { tracker } = fakeTracker({ issues: [detail("triage", [USER])] }); + const outcome = yield* resumeIn(dataDir, tracker); + assert.deepStrictEqual(outcome, { kind: "started", run: run7, threadId: "thread-1" }); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents()); + }), + ); + + it.effect("never claims an issue put back to ready: that is the claim step's", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 3)); + const { tracker, calls } = fakeTracker({ issues: [detail("ready")] }); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* resumeIn(dataDir, tracker, t3.sessions); + assert.strictEqual(outcome.kind, "skipped"); + assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(t3.calls, []); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 3)); + assert.strictEqual(NodeFS.existsSync(runPath(dataDir, { ...run7, attempt: 2 })), false); + }), + ); + + it.effect("finishes a stuck path cut short after its label moved", () => + Effect.gen(function* () { + const dataDir = tempDir(); + const first = fakeTracker({ unassignFails: true }); + const t3 = fakeSessions(dataDir, { project: null }); + yield* Effect.flip(startIn(dataDir, first.tracker, t3.sessions)); + // The label is `factory:stuck` and the factory is still assigned. + const { tracker, calls } = fakeTracker({ issues: [detail("stuck", [USER])] }); + const outcome = yield* resumeIn(dataDir, tracker, t3.sessions); + assert.deepStrictEqual(outcome, { kind: "stuck", run: run7, reason: NO_PROJECT }); + // The comment went out the first time and is not repeated. + assert.deepStrictEqual(calls, ["issue 7", ...stuckCalls(LAUNCH_FAILED, null).slice(1)]); + assert.deepStrictEqual(entriesOf(dataDir), [ + ...startedEvents().slice(0, 3), + ...stuckTail(NO_PROJECT, "launch-failed").slice(0, 1), + ...stuckTail(NO_PROJECT, "stuck-resumed").slice(1), + ]); + }), + ); + + it.effect("leaves a stuck issue alone when its run is finished, or it has none", () => + Effect.gen(function* () { + const none = tempDir(); + const noRun = fakeTracker({ issues: [detail("stuck", [USER])] }); + assert.strictEqual((yield* resumeIn(none, noRun.tracker)).kind, "skipped"); + assert.deepStrictEqual(noRun.calls, ["issue 7"]); + assert.strictEqual(NodeFS.existsSync(runPath(none, run7)), false); + + const finished = tempDir(); + olderRun(finished, 1, "stuck", true); + const done = fakeTracker({ issues: [detail("stuck", [USER])] }); + assert.strictEqual((yield* resumeIn(finished, done.tracker)).kind, "skipped"); + assert.deepStrictEqual(done.calls, ["issue 7"]); + assert.strictEqual(NodeFS.existsSync(runPath(finished, { ...run7, attempt: 2 })), false); + }), + ); +}); diff --git a/apps/server/src/factory/runner/start.ts b/apps/server/src/factory/runner/start.ts index 59d5efa4b91f..e09afc43ecfc 100644 --- a/apps/server/src/factory/runner/start.ts +++ b/apps/server/src/factory/runner/start.ts @@ -20,10 +20,10 @@ * same id. The issue text is checked again once the session exists: a change stops the run and * archives the session. * - * Calling `start` again resumes. An issue in `triage` whose only assignee is the factory's own - * user is an interrupted start, and every step goes on from what the ledger records. A run with - * no record of what approved it cannot be confirmed, so it fails closed and goes `stuck` (no - * session was launched, as the record is written first). + * Calling `start` again resumes; `resume` does only that. An issue in `triage` whose only + * assignee is the factory's own user is an interrupted start, and every step goes on from what + * the ledger records. A run with no record of what approved it cannot be confirmed, so it fails + * closed and goes `stuck` (no session was launched, as the record is written first). */ import * as NodeFS from "node:fs"; @@ -32,7 +32,7 @@ import * as Schema from "effect/Schema"; import type { ProjectConfig } from "../config.ts"; import { refusalReason, unapprovedComment, type Refusal } from "../domain/approval.ts"; -import { InvalidTransition, stateLabel, transition, type State } from "../domain/lifecycle.ts"; +import { InvalidTransition, stateLabel, transition } from "../domain/lifecycle.ts"; import { gateLockFile } from "../gate.ts"; import { ledgerCall as ledger, type LedgerError } from "../ledger/errors.ts"; import { makeCause, type Cause, type ClaimRepair, type Outcome } from "../ledger/events.ts"; @@ -51,8 +51,6 @@ import { TEXT_CHANGED, launchMessage, sessionMayExistNote, - statusBody, - stuckComment, } from "./messages.ts"; import { APPROVAL_RECHECKED, LAUNCH_SENDING, readProgress, type Seen } from "./progress.ts"; import { describe, finishBuilding, goStuck, type Progress } from "./run.ts"; @@ -147,18 +145,71 @@ export const start = ( const detail = yield* ctx.tracker.issue(issue); // A closed issue is never claimed, whatever its label says. Issue #20 ports what softfac // does with its unfinished runs; until then they are left alone. - if (detail.closed) { - return { kind: "skipped", reason: `issue ${mention(issue)} is closed` }; - } - const ours = isOurs(detail.assignees, ctx.trackerUser); + if (detail.closed) return closed(issue); if (detail.state === "ready" && detail.assignees.length === 0) { return yield* startFresh(ctx, issue, detail); } if (detail.state === "ready") return skipped(issue, detail.assignees); - if (detail.state === "triage" && ours) return yield* resumeStart(ctx, issue, detail); + return yield* goOn(ctx, issue, detail); + }); + +/** + * `start` for a claim already made, which never makes one: what the pass calls for a run it + * knows of and for a `triage` issue of ours with no run. An issue someone put back to + * `factory:ready` is left to the claim step, which checks the pause, the limit and the blockers + * first. + */ +export const resume = ( + ctx: StartContext, + issue: number, +): Effect.Effect => + Effect.gen(function* () { + const detail = yield* ctx.tracker.issue(issue); + if (detail.closed) return closed(issue); + if (detail.state === "ready") { + return { kind: "skipped", reason: `issue ${mention(issue)} is ready again, a new claim` }; + } + return yield* goOn(ctx, issue, detail); + }); + +const closed = (issue: number): StartOutcome => ({ + kind: "skipped", + reason: `issue ${mention(issue)} is closed`, +}); + +/** + * Goes on from the ledger with an issue that is not `ready`: a `triage` issue of ours, or a + * `stuck` issue whose newest run gave up and was cut short before its summary line (the label + * moves before the release and the status comment, so a failure in between leaves the run + * unfinished in `triage`), which finishes the stuck path. + */ +const goOn = ( + ctx: StartContext, + issue: number, + detail: IssueDetail, +): Effect.Effect => + Effect.gen(function* () { + if (detail.state === "triage" && isOurs(detail.assignees, ctx.trackerUser)) { + return yield* resumeStart(ctx, issue, detail); + } + if (detail.state === "stuck" && (yield* stuckCutShort(ctx, issue))) { + return yield* resumeStart(ctx, issue, detail); + } return { kind: "skipped", reason: `issue ${mention(issue)} is not labelled factory:ready` }; }); +/** Whether the issue's newest run is unfinished, still in `triage`, and its ledger says it went stuck. */ +const stuckCutShort = (ctx: StartContext, issue: number): Effect.Effect => + Effect.gen(function* () { + const next = nextAttempt(ctx.dataDir, ctx.repo, String(issue)); + if (next === 1) return false; + const newest = newRunId(ctx.repo, String(issue), next - 1); + const seen = readProgress((yield* ledger(() => readRun(ctx.dataDir, newest))).entries); + return ( + !seen.finished && seen.stuck !== null && (seen.state === null || seen.state === "triage") + ); + }); + const startFresh = ( ctx: StartContext, issue: number, From 17be34c2eee2f20fc5e81661ca69344f4937dac7 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:50:06 +1000 Subject: [PATCH 11/16] fix(factory): review-gate round 3: a re-readied issue is not held back by its own run Since the run steps only resume, a run whose issue an instructor labelled factory:ready again stayed in triage and held a slot; with every slot taken the claim step never reached the issue, so its claim never finished the old run (f10). The claim step now leaves an issue's own runs out of the limit, since a claim finishes them first. Another issue's run still holds its slot. PASS_TIMEOUT's comment no longer claims only a hung pass is stopped (f13). A test pins that resume leaves a stuck issue alone when its run never went stuck (f14). --- apps/server/src/factory/loop.ts | 6 +++-- apps/server/src/factory/pass.test.ts | 26 ++++++++++++++++++++ apps/server/src/factory/pass.ts | 7 ++++-- apps/server/src/factory/runner/start.test.ts | 17 +++++++++++++ 4 files changed, 52 insertions(+), 4 deletions(-) diff --git a/apps/server/src/factory/loop.ts b/apps/server/src/factory/loop.ts index a6451d32cd5c..235036990511 100644 --- a/apps/server/src/factory/loop.ts +++ b/apps/server/src/factory/loop.ts @@ -21,8 +21,10 @@ export interface Loop { } /** - * Longer than any one call a pass makes is allowed (a `gh` call 60 seconds, a worktree removal - * 10 minutes), so only a pass that is hung is stopped. + * Well past what a pass takes, though not a sum of every call's own limit (a `gh` call 60 + * seconds, a worktree removal 10 minutes): a very slow pass is stopped too. That is safe, as + * every step goes on from the ledger on the next pass, and a claim cut short before its run + * file is written goes stuck, as it cannot be confirmed. */ export const PASS_TIMEOUT = Duration.minutes(30); diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 6e7944e6ab3f..2fefa0abbfbb 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -710,6 +710,32 @@ describe("runs that are confirmed and still in triage", () => { }), ); + it.effect("an issue put back to ready is claimed past the slot its own run holds", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + yield* pass(fake, machine([REPO], 1)); + // `resume` leaves a ready issue alone; the claim finishes the old run and starts afresh. + assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#12`]); + }), + ); + + it.effect("an issue put back to ready still waits for a slot another run holds", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(40, BUILDING); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + const { lines } = yield* pass(fake, machine([REPO], 1)); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + assert.ok( + lines.some((l) => l.startsWith("limit reached: 1 ready issue")), + lines.join("\n"), + ); + }), + ); + it.effect("a building run is left alone", () => Effect.gen(function* () { const fake = new Fake(); diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 4618f7ba9ae0..fc52bdf8265b 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -715,8 +715,11 @@ const claim = ( // the machine's, said once after the loop. if (paused !== null) continue; // Before the blockers: with no slot free nothing is claimed, so they are not read. Every - // issue left is counted as left over, blocked or not. - if (active >= machine.maxConcurrentRuns) { + // issue left is counted as left over, blocked or not. The issue's own runs do not count: a + // claim finishes them first, so a run an instructor retried by labelling the issue + // `factory:ready` again does not hold the slot its new attempt needs. + const own = known.of(name, issue).filter(holdsSlot).length; + if (active - own >= machine.maxConcurrentRuns) { left += 1; yield* pass.record({ event: "claim_skipped", diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index 2270b50f4a37..345eb46f5601 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -1288,6 +1288,23 @@ describe("resume", () => { }), ); + it.effect("leaves alone a stuck issue whose run never went stuck: a person stopped it", () => + Effect.gen(function* () { + for (const lines of [startedLines().slice(0, 3), startedLines()]) { + const dataDir = tempDir(); + seedRun(dataDir, lines); + const before = entriesOf(dataDir); + const { tracker, calls } = fakeTracker({ issues: [detail("stuck", [USER])] }); + const t3 = fakeSessions(dataDir, {}); + const outcome = yield* resumeIn(dataDir, tracker, t3.sessions); + assert.strictEqual(outcome.kind, "skipped"); + assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(t3.calls, []); + assert.deepStrictEqual(entriesOf(dataDir), before); + } + }), + ); + it.effect("leaves a stuck issue alone when its run is finished, or it has none", () => Effect.gen(function* () { const none = tempDir(); From 8265cc9f8e25d6a057562afec6533263244c504f Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 11:58:23 +1000 Subject: [PATCH 12/16] fix(factory): review-gate round 4: a stalled triage run says so and keeps its slot Round 3 left an issue's own runs out of the claim limit, so an issue put back to factory:ready while its old session still ran could start a second session past max_concurrent_runs: finishOlderRuns ends the old run in the ledger only until #20 ports the stop (f15). That exemption is gone. As softfac's `stalled` does, a confirmed triage run that `resume` will not take (a person relabelled, reassigned or closed its issue) now prints an ATTENTION line saying it is not progressing and holds a slot, and how to let the start finish, instead of a quiet "left to a later pass" (f10). The run keeps its slot, as in softfac; abandoning a closed issue's run is #20's. --- apps/server/src/factory/pass.test.ts | 28 ++++++++++++---------------- apps/server/src/factory/pass.ts | 24 ++++++++++++++++++------ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 2fefa0abbfbb..dfcfae4c5123 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -710,27 +710,23 @@ describe("runs that are confirmed and still in triage", () => { }), ); - it.effect("an issue put back to ready is claimed past the slot its own run holds", () => + it.effect("a triage run whose issue a person moved is an attention line and keeps its slot", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, "triage"); - fake.ready[REPO] = [12]; - yield* pass(fake, machine([REPO], 1)); - // `resume` leaves a ready issue alone; the claim finishes the old run and starts afresh. - assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#12`]); - }), - ); - - it.effect("an issue put back to ready still waits for a slot another run holds", () => - Effect.gen(function* () { - const fake = new Fake(); - fake.runIn(40, BUILDING); - fake.runIn(12, "triage"); - fake.ready[REPO] = [12]; - const { lines } = yield* pass(fake, machine([REPO], 1)); + fake.ready[REPO] = [12, 3]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "issue #12 is ready again" }); + const { report, lines } = yield* pass(fake, machine([REPO], 1)); + // `resume` never claims; the run holds the only slot, so neither issue is claimed. assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + assert.equal(report.attention, 1); + const line = runLine(lines, "ATTENTION: rawphp/softfac#12 "); + assert.ok( + line.includes("run rawphp__softfac__12__1 is not progressing and holds a slot"), + line, + ); assert.ok( - lines.some((l) => l.startsWith("limit reached: 1 ready issue")), + lines.some((l) => l.startsWith("limit reached: 2 ready issues")), lines.join("\n"), ); }), diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index fc52bdf8265b..058ad8926a95 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -203,6 +203,12 @@ class Pass { this.out(`error: ${text}`); } + /** A line a person has to act on. */ + attention(text: string): void { + this.report.attention += 1; + this.out(`ATTENTION: ${text}`); + } + /** * Writes `event` to the machine log. A log that cannot be written is an error line once per * pass and nothing else: it is not an event, which could not be written either. @@ -423,7 +429,7 @@ const workRuns = ( break; case "skipped": case "refused": - notes.parts.push("not started, left to a later pass"); + stalled(pass, notes, runIdToString(info.id)); break; } } @@ -432,6 +438,15 @@ const workRuns = ( } }); +/** + * A confirmed run in `triage` that `resume` would not take: a person moved its issue, so nothing + * in the pass will move it, and it holds a slot. softfac's words, as its `stalled` says them. + */ +const stalled = (pass: Pass, notes: Notes, run: string): void => + pass.attention( + `${notes.label} run ${run} is not progressing and holds a slot: its issue is not \`factory:triage\` assigned to the factory's user alone, so \`start\` will not take it. If you moved the issue by hand, put it back to \`factory:triage\` assigned to the factory's user so \`start\` can finish it`, + ); + /** * The unfinished runs as this pass knows them: listed once at the start of the pass, then the * runs of an issue read again after each `start` for it, so the ledger is not listed whole again. @@ -715,11 +730,8 @@ const claim = ( // the machine's, said once after the loop. if (paused !== null) continue; // Before the blockers: with no slot free nothing is claimed, so they are not read. Every - // issue left is counted as left over, blocked or not. The issue's own runs do not count: a - // claim finishes them first, so a run an instructor retried by labelling the issue - // `factory:ready` again does not hold the slot its new attempt needs. - const own = known.of(name, issue).filter(holdsSlot).length; - if (active - own >= machine.maxConcurrentRuns) { + // issue left is counted as left over, blocked or not. + if (active >= machine.maxConcurrentRuns) { left += 1; yield* pass.record({ event: "claim_skipped", From 857d1bd80c069469583a07df06bc5e400c0ae9ff Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 12:07:02 +1000 Subject: [PATCH 13/16] fix(factory): review-gate round 5: a stalled run's line says why and is logged The stalled ATTENTION line now carries resume's reason, and a skip says what a person did: a closed issue's line says to reopen it (nothing finishes a closed issue's run until #20), and an issue put back to ready with nobody assigned gets a plain note instead, as the claim step takes it when a slot is free (f10, f16). Each attention line also writes the machine log's `attention` event with `not-progressing`, as softfac does (f18). Tests pin a re-readied issue with and without a free slot (f19); comments say its run holds a slot until a claim finishes it (f17). A claim_repaired line the machine log cannot take is logged as a warning, not dropped (f9). `mention` and the sole-assignee check have one definition each, still case-folded with toLowerCase: a Linear user can be a display name (f8). --- apps/server/src/factory/index.ts | 6 +- apps/server/src/factory/pass.test.ts | 72 +++++++++++++++++--- apps/server/src/factory/pass.ts | 54 +++++++++++---- apps/server/src/factory/runner/start.test.ts | 64 ++++++++++++++--- apps/server/src/factory/runner/start.ts | 56 ++++++++++----- 5 files changed, 200 insertions(+), 52 deletions(-) diff --git a/apps/server/src/factory/index.ts b/apps/server/src/factory/index.ts index c2ace0518b24..ede133af9142 100644 --- a/apps/server/src/factory/index.ts +++ b/apps/server/src/factory/index.ts @@ -31,6 +31,7 @@ import * as Process from "./process.ts"; import { FactoryProcess } from "./process.ts"; import { CUT_SHORT_COMMENT } from "./runner/messages.ts"; import { + isOurs, resume, start, utcSecond, @@ -74,10 +75,7 @@ const unassignedByNumber = (items: ReadonlyArray) => /** The numbers of the issues `user` is the only assignee of, by number. */ const assignedOnlyTo = (items: ReadonlyArray, user: string) => items - .filter( - (item) => - item.assignees.length === 1 && item.assignees[0]!.toLowerCase() === user.toLowerCase(), - ) + .filter((item) => isOurs(item.assignees, user)) .map((item) => item.number) .sort(byNumber); diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index dfcfae4c5123..d36e0fc6eaf1 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -710,24 +710,80 @@ describe("runs that are confirmed and still in triage", () => { }), ); - it.effect("a triage run whose issue a person moved is an attention line and keeps its slot", () => + it.effect( + "a triage run whose issue is ready again waits for the claim step, with no attention", + () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + const { report, lines } = yield* pass(fake, machine([REPO], 1)); + // `resume` never claims, and the run holds the only slot. + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + assert.equal(report.attention, 0); + const line = runLine(lines, "rawphp/softfac#12 triage:"); + assert.ok(line.includes("ready again: the claim step takes it when a slot is free"), line); + assert.ok( + lines.some((l) => l.startsWith("limit reached: 1 ready issue")), + lines.join("\n"), + ); + }), + ); + + it.effect("a triage run whose issue is ready again is claimed afresh when a slot is free", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + const { report } = yield* pass(fake, machine([REPO], 2)); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#12`]); + assert.equal(report.attention, 0); + }), + ); + + it.effect("a triage run whose issue a person moved is an attention line, logged, with why", () => Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, "triage"); - fake.ready[REPO] = [12, 3]; - fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "issue #12 is ready again" }); + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "issue #12 is assigned to bob" }); const { report, lines } = yield* pass(fake, machine([REPO], 1)); - // `resume` never claims; the run holds the only slot, so neither issue is claimed. - assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); assert.equal(report.attention, 1); const line = runLine(lines, "ATTENTION: rawphp/softfac#12 "); assert.ok( - line.includes("run rawphp__softfac__12__1 is not progressing and holds a slot"), + line.includes( + "run rawphp__softfac__12__1 is not progressing and holds a slot: issue #12 is assigned to bob. If you moved the issue by hand", + ), line, ); + assert.deepEqual( + fake.events.filter((e) => e.event === "attention"), + [ + { + event: "attention", + data: { run: "rawphp__softfac__12__1", reason: "not-progressing" }, + }, + ], + ); + }), + ); + + it.effect("a triage run whose issue was closed says to reopen it", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.pushStart(`${REPO}#12`, { + kind: "skipped", + reason: "issue #12 is closed", + moved: "closed", + }); + const { report, lines } = yield* pass(fake, machine([REPO], 1)); + assert.equal(report.attention, 1); + const line = runLine(lines, "ATTENTION: rawphp/softfac#12 "); assert.ok( - lines.some((l) => l.startsWith("limit reached: 2 ready issues")), - lines.join("\n"), + line.includes("holds a slot: issue #12 is closed. If it was closed by mistake, reopen it"), + line, ); }), ); diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 058ad8926a95..51086f000d9d 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -19,7 +19,7 @@ import type { ErrorKind, MachineEvent } from "./ledger/events.ts"; import { holdsSlot, type RunInfo } from "./ledger/listRuns.ts"; import { runIdToString, type RunId } from "./ledger/runId.ts"; import { duration, formatTs } from "./ledger/time.ts"; -import type { StartError, StartOutcome } from "./runner/start.ts"; +import { mention, type StartError, type StartOutcome } from "./runner/start.ts"; import { LIST_LIMIT, type Blocker, type Listing, type UnassignedClaim } from "./tracker/github.ts"; /** An error of the pass's own: what kind it is for the machine log, and what it says. */ @@ -150,8 +150,6 @@ const errorKind = (error: StartError): ErrorKind => { } }; -const mention = (issue: number) => `#${issue}`; - const sameRepo = (a: string, b: string) => a.toLowerCase() === b.toLowerCase(); const GATE_RUNNING = "gate running"; @@ -203,10 +201,14 @@ class Pass { this.out(`error: ${text}`); } - /** A line a person has to act on. */ - attention(text: string): void { + /** + * A line a person has to act on about `run`, and the machine log's `attention` event for it + * with the ledger.md `reason` code. + */ + attention(text: string, run: string, reason: string): Effect.Effect { this.report.attention += 1; this.out(`ATTENTION: ${text}`); + return this.record({ event: "attention", data: { run, reason } }); } /** @@ -394,8 +396,8 @@ const runs = (world: PassWorld, pass: Pass): Effect.Effect - pass.attention( - `${notes.label} run ${run} is not progressing and holds a slot: its issue is not \`factory:triage\` assigned to the factory's user alone, so \`start\` will not take it. If you moved the issue by hand, put it back to \`factory:triage\` assigned to the factory's user so \`start\` can finish it`, +const stalled = ( + pass: Pass, + notes: Notes, + run: string, + why: string, + closed: boolean, +): Effect.Effect => { + const back = + "put it back to `factory:triage` assigned to the factory's user so `start` can finish it"; + const advice = closed + ? `If it was closed by mistake, reopen it and ${back}` + : `If you moved the issue by hand, ${back}`; + return pass.attention( + `${notes.label} run ${run} is not progressing and holds a slot: ${plain(why)}. ${advice}`, + run, + "not-progressing", ); +}; /** * The unfinished runs as this pass knows them: listed once at the start of the pass, then the diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index 345eb46f5601..d02f151b5cb1 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -4,6 +4,7 @@ import * as NodePath from "node:path"; import { assert, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; +import * as Logger from "effect/Logger"; import * as Schema from "effect/Schema"; import { afterEach, describe } from "vite-plus/test"; @@ -14,7 +15,7 @@ import type { State } from "../domain/lifecycle.ts"; import { gateLockFile } from "../gate.ts"; import { makeCause, type Cause, type RunEvent } from "../ledger/events.ts"; import { Ledger } from "../ledger/ledger.ts"; -import { scanMachineLog } from "../ledger/machineLog.ts"; +import { MACHINE_LOG, scanMachineLog } from "../ledger/machineLog.ts"; import { readRun } from "../ledger/read.ts"; import { parseRunId, runPath, type RunId } from "../ledger/runId.ts"; import { T0, useTempDirs } from "../ledger/testSupport.ts"; @@ -518,18 +519,19 @@ describe("start: a fresh ready issue", () => { it.effect("an issue that is assigned, closed or not ready is skipped with nothing written", () => Effect.gen(function* () { - for (const [issue, reason] of [ - [detail("ready", ["a", "b"]), "issue #7 is already assigned to a, b"], - [detail("ready", [], true), "issue #7 is closed"], - [detail("building", [USER]), "issue #7 is not labelled factory:ready"], - [detail("triage", ["eve"]), "issue #7 is not labelled factory:ready"], - [detail(null), "issue #7 is not labelled factory:ready"], + const notOurs = "issue #7 is not factory:triage assigned to rawphp alone"; + for (const [issue, expected] of [ + [detail("ready", ["a", "b"]), { reason: "issue #7 is already assigned to a, b" }], + [detail("ready", [], true), { reason: "issue #7 is closed", moved: "closed" }], + [detail("building", [USER]), { reason: notOurs }], + [detail("triage", ["eve"]), { reason: notOurs }], + [detail(null), { reason: notOurs }], ] as const) { const dataDir = tempDir(); const { tracker, calls } = fakeTracker({ issues: [issue] }); const t3 = fakeSessions(dataDir, {}); const outcome = yield* startIn(dataDir, tracker, t3.sessions); - assert.deepStrictEqual(outcome, { kind: "skipped", reason }); + assert.deepStrictEqual(outcome, { kind: "skipped", ...expected }); assert.deepStrictEqual(calls, ["issue 7"]); assert.deepStrictEqual(t3.calls, []); assert.strictEqual(NodeFS.existsSync(NodePath.join(dataDir, "runs")), false); @@ -555,6 +557,30 @@ describe("start: a fresh ready issue", () => { }); describe("start: a claim call that fails", () => { + it.effect("is finished even when the machine log cannot be written, which is logged", () => + Effect.gen(function* () { + const dataDir = tempDir(); + // A directory where the log file goes: the machine log cannot be opened. + NodeFS.mkdirSync(NodePath.join(dataDir, MACHINE_LOG), { recursive: true }); + const { tracker } = fakeTracker({ + issues: [detail("ready"), detail("triage", [USER])], + claims: [ghFailure()], + }); + const warnings: Array = []; + const capture = Logger.make(({ logLevel, message }) => { + if (logLevel === "Warn") + warnings.push(String(Array.isArray(message) ? message[0] : message)); + }); + const outcome = yield* startIn(dataDir, tracker).pipe( + Effect.provide(Logger.layer([capture], { mergeWithExisting: false })), + ); + assert.deepStrictEqual(outcome, { kind: "started", run: run7, threadId: "thread-1" }); + assert.deepStrictEqual(warnings, [ + "claim_repaired (finished) for rawphp/softfac#7 could not be written to the machine log", + ]); + }), + ); + it.effect("is finished when GitHub applied it, and says so in the machine log", () => Effect.gen(function* () { const dataDir = tempDir(); @@ -1260,7 +1286,11 @@ describe("resume", () => { const { tracker, calls } = fakeTracker({ issues: [detail("ready")] }); const t3 = fakeSessions(dataDir, {}); const outcome = yield* resumeIn(dataDir, tracker, t3.sessions); - assert.strictEqual(outcome.kind, "skipped"); + assert.deepStrictEqual(outcome, { + kind: "skipped", + reason: "issue #7 is ready again", + moved: "ready", + }); assert.deepStrictEqual(calls, ["issue 7"]); assert.deepStrictEqual(t3.calls, []); assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 3)); @@ -1268,6 +1298,22 @@ describe("resume", () => { }), ); + it.effect("says a closed issue was closed, and touches nothing", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 3)); + const { tracker, calls } = fakeTracker({ issues: [detail("triage", [USER], true)] }); + const outcome = yield* resumeIn(dataDir, tracker); + assert.deepStrictEqual(outcome, { + kind: "skipped", + reason: "issue #7 is closed", + moved: "closed", + }); + assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 3)); + }), + ); + it.effect("finishes a stuck path cut short after its label moved", () => Effect.gen(function* () { const dataDir = tempDir(); diff --git a/apps/server/src/factory/runner/start.ts b/apps/server/src/factory/runner/start.ts index e09afc43ecfc..6d4236c80a22 100644 --- a/apps/server/src/factory/runner/start.ts +++ b/apps/server/src/factory/runner/start.ts @@ -104,8 +104,11 @@ export interface StartContext { export type StartOutcome = /** The issue is claimed, its session is launched and the issue is `factory:building`. */ | { readonly kind: "started"; readonly run: RunId; readonly threadId: string } - /** The issue was not ours to take. Nothing was written anywhere. */ - | { readonly kind: "skipped"; readonly reason: string } + /** + * The issue was not ours to take. Nothing was written anywhere. `moved` says a person closed + * the issue, or put it back to `factory:ready` with nobody assigned, which the claim step takes. + */ + | { readonly kind: "skipped"; readonly reason: string; readonly moved?: "closed" | "ready" } /** The issue was `ready` but not approved, so it went back to `factory:proposed` with one fixed comment, and no run was made. */ | { readonly kind: "refused"; readonly reason: string } /** The issue went to `stuck`. */ @@ -127,14 +130,16 @@ export type StartError = | GateRunningError | SessionsError; -const mention = (issue: number) => `#${issue}`; +/** An issue as the pass's lines and the skip reasons name it. */ +export const mention = (issue: number) => `#${issue}`; const skipped = (issue: number, assignees: ReadonlyArray): StartOutcome => ({ kind: "skipped", reason: `issue ${mention(issue)} is already assigned to ${assignees.join(", ")}`, }); -const isOurs = (assignees: ReadonlyArray, user: string) => +/** Whether `user` is the only assignee. Not `asciiLower`: a Linear user can be a display name. */ +export const isOurs = (assignees: ReadonlyArray, user: string) => assignees.length === 1 && assignees[0]!.toLowerCase() === user.toLowerCase(); export const start = ( @@ -157,7 +162,8 @@ export const start = ( * `start` for a claim already made, which never makes one: what the pass calls for a run it * knows of and for a `triage` issue of ours with no run. An issue someone put back to * `factory:ready` is left to the claim step, which checks the pause, the limit and the blockers - * first. + * first. Until it claims the issue, which finishes the older runs, an unfinished run of the + * issue holds its slot. */ export const resume = ( ctx: StartContext, @@ -166,15 +172,17 @@ export const resume = ( Effect.gen(function* () { const detail = yield* ctx.tracker.issue(issue); if (detail.closed) return closed(issue); - if (detail.state === "ready") { - return { kind: "skipped", reason: `issue ${mention(issue)} is ready again, a new claim` }; + if (detail.state === "ready" && detail.assignees.length === 0) { + return { kind: "skipped", reason: `issue ${mention(issue)} is ready again`, moved: "ready" }; } + if (detail.state === "ready") return skipped(issue, detail.assignees); return yield* goOn(ctx, issue, detail); }); const closed = (issue: number): StartOutcome => ({ kind: "skipped", reason: `issue ${mention(issue)} is closed`, + moved: "closed", }); /** @@ -195,7 +203,10 @@ const goOn = ( if (detail.state === "stuck" && (yield* stuckCutShort(ctx, issue))) { return yield* resumeStart(ctx, issue, detail); } - return { kind: "skipped", reason: `issue ${mention(issue)} is not labelled factory:ready` }; + return { + kind: "skipped", + reason: `issue ${mention(issue)} is not factory:triage assigned to ${ctx.trackerUser} alone`, + }; }); /** Whether the issue's newest run is unfinished, still in `triage`, and its ledger says it went stuck. */ @@ -292,7 +303,7 @@ const repairClaim = ( const labelled = assigned && (seen.state === "triage" || (yield* ok(ctx.tracker.setState(issue, "triage")))); if (assigned && labelled) { - recordClaimRepair(ctx, issue, "finished"); + yield* recordClaimRepair(ctx, issue, "finished"); return { kind: "claimed" }; } // Both are tried, so a label that will not move does not leave our assignee on the issue. @@ -300,14 +311,21 @@ const repairClaim = ( const moved = yield* ok(ctx.tracker.setState(issue, "proposed")); if (off && moved) { yield* ok(ctx.tracker.postComment(issue, CUT_SHORT_COMMENT)); - recordClaimRepair(ctx, issue, "released"); + yield* recordClaimRepair(ctx, issue, "released"); } return yield* error; }); -/** Writes a `claim_repaired` line to the machine log. A log that cannot be written is not worth failing the claim for. */ -const recordClaimRepair = (ctx: StartContext, issue: number, outcome: ClaimRepair): void => { - try { +/** + * Writes a `claim_repaired` line to the machine log. A log that cannot be written is not worth + * failing the claim for, so it is logged as a warning that names the issue. + */ +const recordClaimRepair = ( + ctx: StartContext, + issue: number, + outcome: ClaimRepair, +): Effect.Effect => + ledger(() => { const log = MachineLog.open(ctx.dataDir, { clock: ctx.clock }); try { log.append({ @@ -317,10 +335,14 @@ const recordClaimRepair = (ctx: StartContext, issue: number, outcome: ClaimRepai } finally { log.close(); } - } catch { - // Not worth failing the claim for. - } -}; + }).pipe( + Effect.catchCause((cause) => + Effect.logWarning( + `claim_repaired (${outcome}) for ${ctx.repo}${mention(issue)} could not be written to the machine log`, + cause, + ), + ), + ); /** How long a refusal comment that has no label time to be compared with counts as recent. */ const RECENT_MS = 60 * 60 * 1000; From 193715692905ddb8a0386562703d155aac14eec5 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 12:10:55 +1000 Subject: [PATCH 14/16] fix(factory): review-gate round 6: a retry that waits on its own run says so An issue put back to factory:ready while its own unfinished run holds the last slot can never be claimed: only that claim would finish the run. The claim step now raises an ATTENTION line and a `not-progressing` attention event for it, naming the run and how to free it, instead of only a limit line (f10). The limit still counts the run, so no second session starts past max_concurrent_runs. pass.ts matches repos with runId.ts's asciiLower, as run ids do (f8). A test pins that resume says nothing moved for a ready issue someone is assigned to, which stalls its run (f19). --- apps/server/src/factory/pass.test.ts | 54 +++++++++++++------- apps/server/src/factory/pass.ts | 19 ++++++- apps/server/src/factory/runner/start.test.ts | 16 ++++++ 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index d36e0fc6eaf1..09823916197b 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -710,25 +710,41 @@ describe("runs that are confirmed and still in triage", () => { }), ); - it.effect( - "a triage run whose issue is ready again waits for the claim step, with no attention", - () => - Effect.gen(function* () { - const fake = new Fake(); - fake.runIn(12, "triage"); - fake.ready[REPO] = [12]; - fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); - const { report, lines } = yield* pass(fake, machine([REPO], 1)); - // `resume` never claims, and the run holds the only slot. - assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); - assert.equal(report.attention, 0); - const line = runLine(lines, "rawphp/softfac#12 triage:"); - assert.ok(line.includes("ready again: the claim step takes it when a slot is free"), line); - assert.ok( - lines.some((l) => l.startsWith("limit reached: 1 ready issue")), - lines.join("\n"), - ); - }), + it.effect("a ready-again issue whose own run holds the only slot is an attention line", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + const { report, lines } = yield* pass(fake, machine([REPO], 1)); + // `resume` never claims, and the run holds the only slot. + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + const note = runLine(lines, "rawphp/softfac#12 triage:"); + assert.ok(note.includes("ready again: the claim step takes it when a slot is free"), note); + assert.equal(report.attention, 1); + const line = runLine(lines, "ATTENTION: rawphp/softfac#12 "); + assert.ok(line.includes("the new claim waits for a slot this run holds"), line); + assert.deepEqual( + fake.events.filter((e) => e.event === "attention"), + [ + { + event: "attention", + data: { run: "rawphp__softfac__12__1", reason: "not-progressing" }, + }, + ], + ); + }), + ); + + it.effect("a ready issue the limit holds back by other runs raises no attention", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(40, BUILDING); + fake.ready[REPO] = [3]; + const { report } = yield* pass(fake, machine([REPO], 1)); + assert.deepEqual(starts(fake), []); + assert.equal(report.attention, 0); + }), ); it.effect("a triage run whose issue is ready again is claimed afresh when a slot is free", () => diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 51086f000d9d..9663c8a51f91 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -17,7 +17,7 @@ import { sanitize } from "./gate.ts"; import type { LedgerError } from "./ledger/errors.ts"; import type { ErrorKind, MachineEvent } from "./ledger/events.ts"; import { holdsSlot, type RunInfo } from "./ledger/listRuns.ts"; -import { runIdToString, type RunId } from "./ledger/runId.ts"; +import { asciiLower, runIdToString, type RunId } from "./ledger/runId.ts"; import { duration, formatTs } from "./ledger/time.ts"; import { mention, type StartError, type StartOutcome } from "./runner/start.ts"; import { LIST_LIMIT, type Blocker, type Listing, type UnassignedClaim } from "./tracker/github.ts"; @@ -150,7 +150,8 @@ const errorKind = (error: StartError): ErrorKind => { } }; -const sameRepo = (a: string, b: string) => a.toLowerCase() === b.toLowerCase(); +/** Repos match as run ids do: `owner/name` folded with `asciiLower`. */ +const sameRepo = (a: string, b: string) => asciiLower(a) === asciiLower(b); const GATE_RUNNING = "gate running"; @@ -446,6 +447,17 @@ const workRuns = ( } }); +/** + * A ready issue the limit holds back while its own unfinished run holds a slot: the claim would + * finish that run (`finishOlderRuns`), so with no other slot freeing, nothing moves. + */ +const waitsOnItself = (pass: Pass, notes: Notes, run: string): Effect.Effect => + pass.attention( + `${notes.label} run ${run} is not progressing and holds a slot: its issue is ready again, and the new claim waits for a slot this run holds. Put the issue back to \`factory:triage\` assigned to the factory's user so \`start\` can finish this run, or free a slot`, + run, + "not-progressing", + ); + const READY_AGAIN = "its issue is ready again: the claim step takes it when a slot is free, and this run holds one until then"; @@ -763,6 +775,9 @@ const claim = ( event: "claim_skipped", data: { repo: name, issue: String(issue), reason: "limit" }, }); + // Only this claim would finish the issue's own run, so a slot it holds never frees. + const own = known.of(name, issue).find(holdsSlot); + if (own !== undefined) yield* waitsOnItself(pass, notes, runIdToString(own.id)); continue; } // A blocked issue takes no slot. Nothing is written for it, and a read that fails holds it diff --git a/apps/server/src/factory/runner/start.test.ts b/apps/server/src/factory/runner/start.test.ts index d02f151b5cb1..94b583cda8bc 100644 --- a/apps/server/src/factory/runner/start.test.ts +++ b/apps/server/src/factory/runner/start.test.ts @@ -1298,6 +1298,22 @@ describe("resume", () => { }), ); + it.effect("says nothing moved for a ready issue someone is assigned to", () => + Effect.gen(function* () { + const dataDir = tempDir(); + seedRun(dataDir, startedLines().slice(0, 3)); + const { tracker, calls } = fakeTracker({ issues: [detail("ready", ["bob"])] }); + const outcome = yield* resumeIn(dataDir, tracker); + // Not `moved: "ready"`: the claim step never takes an assigned issue, so the run stalls. + assert.deepStrictEqual(outcome, { + kind: "skipped", + reason: "issue #7 is already assigned to bob", + }); + assert.deepStrictEqual(calls, ["issue 7"]); + assert.deepStrictEqual(entriesOf(dataDir), startedEvents().slice(0, 3)); + }), + ); + it.effect("says a closed issue was closed, and touches nothing", () => Effect.gen(function* () { const dataDir = tempDir(); From 167ccbf47ea4f0776fe565e07269a706aaf7fcdc Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 12:15:50 +1000 Subject: [PATCH 15/16] fix(factory): review-gate round 7: a retry claims past its own sessionless run An issue put back to factory:ready while its own triage run held the last slot waited forever: only that claim would finish the run (f10). Runs now say whether a session may be running (a launched session, or a launch that was sent). The claim step leaves the issue's own confirmed triage runs with no session out of the limit, since finishOlderRuns ends them and nothing is left running. One whose session may be running keeps its slot, so no second session starts past max_concurrent_runs, and only when such runs fill the limit by themselves does the claim raise its ATTENTION line (f20). --- .../src/factory/ledger/listRuns.test.ts | 56 ++++++++++-- apps/server/src/factory/ledger/listRuns.ts | 10 +- apps/server/src/factory/pass.test.ts | 91 ++++++++++++++++--- apps/server/src/factory/pass.ts | 29 ++++-- 4 files changed, 153 insertions(+), 33 deletions(-) diff --git a/apps/server/src/factory/ledger/listRuns.test.ts b/apps/server/src/factory/ledger/listRuns.test.ts index 444d80554380..eb563d8a4063 100644 --- a/apps/server/src/factory/ledger/listRuns.test.ts +++ b/apps/server/src/factory/ledger/listRuns.test.ts @@ -60,9 +60,9 @@ describe("listUnfinishedRuns", () => { runAt(dataDir, rid(2), T0 + 10_000, "pr-open", true); runAt(dataDir, rid(4), T0 + 30_000, null); expect(listUnfinishedRuns(dataDir)).toEqual([ - { id: rid(1), state: "triage", unreadable: false, confirmed: true }, - { id: rid(3), state: "building", unreadable: false, confirmed: true }, - { id: rid(4), state: null, unreadable: false, confirmed: true }, + { id: rid(1), state: "triage", unreadable: false, confirmed: true, session: false }, + { id: rid(3), state: "building", unreadable: false, confirmed: true, session: false }, + { id: rid(4), state: null, unreadable: false, confirmed: true, session: false }, ]); }); @@ -78,8 +78,8 @@ describe("listUnfinishedRuns", () => { // A directory named like a run file is a run whose file cannot be read. NodeFS.mkdirSync(NodePath.join(dataDir, RUNS_DIR, runIdFileName(rid(6)))); expect(listUnfinishedRuns(dataDir)).toEqual([ - { id: rid(7), state: "building", unreadable: false, confirmed: true }, - { id: rid(6), state: null, unreadable: true, confirmed: false }, + { id: rid(7), state: "building", unreadable: false, confirmed: true, session: false }, + { id: rid(6), state: null, unreadable: true, confirmed: false, session: false }, ]); }); @@ -99,7 +99,7 @@ describe("listUnfinishedRuns", () => { `{"ts":"2026-10-04T10:13:16.123Z","ms":${T0},"event":"state_changed","data":{"from":"ready","to":"later"}}\n`, ); expect(listUnfinishedRuns(dataDir)).toEqual([ - { id: rid(9), state: null, unreadable: false, confirmed: false }, + { id: rid(9), state: null, unreadable: false, confirmed: false, session: false }, ]); }); @@ -107,7 +107,7 @@ describe("listUnfinishedRuns", () => { const dataDir = tempDir(); runAt(dataDir, rid(8), T0, "triage", false, false); expect(listUnfinishedRuns(dataDir)).toEqual([ - { id: rid(8), state: "triage", unreadable: false, confirmed: false }, + { id: rid(8), state: "triage", unreadable: false, confirmed: false, session: false }, ]); }); @@ -120,8 +120,45 @@ describe("listUnfinishedRuns", () => { // A file of another issue that cannot be read is not opened, so it is not listed. NodeFS.mkdirSync(NodePath.join(dataDir, RUNS_DIR, runIdFileName(rid(3)))); expect(listUnfinishedRuns(dataDir, { repo: "RawPhp/Softfac", issue: 1 })).toEqual([ - { id: rid(1), state: "triage", unreadable: false, confirmed: true }, - { id: rid(1, "rawphp/softfac", 2), state: "building", unreadable: false, confirmed: true }, + { id: rid(1), state: "triage", unreadable: false, confirmed: true, session: false }, + { + id: rid(1, "rawphp/softfac", 2), + state: "building", + unreadable: false, + confirmed: true, + session: false, + }, + ]); + }); +}); + +describe("listUnfinishedRuns: whether a session may be running", () => { + it("is set by a launched session or a launch that was sent, and not by the claim alone", () => { + const dataDir = tempDir(); + runAt(dataDir, rid(1), T0, "triage"); + runAt(dataDir, rid(2), T0 + 10_000, "triage"); + runAt(dataDir, rid(3), T0 + 20_000, "triage"); + const step = (id: RunId, line: Parameters[0]) => { + const ledger = Ledger.open(dataDir, id, { clock: ticking(T0 + 30_000, 1000) }); + handles.push(ledger); + ledger.append(line, cause); + }; + step(rid(2), { event: "step_changed", data: { step: "launch-sending" } }); + step(rid(3), { + event: "session_launched", + data: { + thread_id: "t-3", + worktree: "/wt", + branch: "b", + model_instance: "m", + model: "x", + resumed_from: null, + }, + }); + expect(listUnfinishedRuns(dataDir).map((r) => [r.id.issue, r.session])).toEqual([ + ["1", false], + ["2", true], + ["3", true], ]); }); }); @@ -132,6 +169,7 @@ describe("holdsSlot", () => { state, unreadable, confirmed, + session: false, }); it("every confirmed unfinished run holds a slot but one waiting in pr-open", () => { diff --git a/apps/server/src/factory/ledger/listRuns.ts b/apps/server/src/factory/ledger/listRuns.ts index 5344e967dfbb..cf260579b2e6 100644 --- a/apps/server/src/factory/ledger/listRuns.ts +++ b/apps/server/src/factory/ledger/listRuns.ts @@ -20,6 +20,11 @@ export interface RunInfo { * launch one: `start` sends it to `stuck` when it is called for the issue again. */ readonly confirmed: boolean; + /** + * A session may be running for it: the run records a launched session, or a launch that was + * sent. False for a file that cannot be read. + */ + readonly session: boolean; } /** @@ -68,9 +73,10 @@ export const listUnfinishedRuns = ( const seen = readProgress(record.entries); if (seen.finished) continue; const confirmed = seen.approvedAt !== null || seen.threadId !== null; - info = { id, state: seen.state, unreadable: false, confirmed }; + const session = seen.threadId !== null || seen.launchSent; + info = { id, state: seen.state, unreadable: false, confirmed, session }; } catch { - info = { id, state: null, unreadable: true, confirmed: false }; + info = { id, state: null, unreadable: true, confirmed: false, session: false }; } const rank = info.unreadable ? 2 : firstMs === null ? 1 : 0; runs.push({ firstMs, info, rank }); diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index 09823916197b..c5041778df4c 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -98,8 +98,8 @@ class Fake implements PassWorld { calls: Array = []; events: Array = []; - runIn(issue: number, state: State | null, repo = REPO) { - this.runs.push({ id: rid(issue, repo), state, unreadable: false, confirmed: true }); + runIn(issue: number, state: State | null, repo = REPO, session = false) { + this.runs.push({ id: rid(issue, repo), state, unreadable: false, confirmed: true, session }); } pushStart(key: string, answer: StartOutcome | StartError) { @@ -201,6 +201,7 @@ class Fake implements PassWorld { state: "triage", unreadable: false, confirmed: true, + session: false, }); } if ("kind" in answer && answer.kind === "started") { @@ -208,7 +209,14 @@ class Fake implements PassWorld { // A run the pass already knew moves on to building; a new claim leaves a triage run. if (existing !== undefined) this.runs[this.runs.indexOf(existing)] = { ...existing, state: "building" }; - else this.runs.push({ id: answer.run, state: "triage", unreadable: false, confirmed: true }); + else + this.runs.push({ + id: answer.run, + state: "triage", + unreadable: false, + confirmed: true, + session: false, + }); return Effect.succeed(answer); } if ("kind" in answer) return Effect.succeed(answer); @@ -500,7 +508,13 @@ describe("claiming new issues", () => { it.effect("a run whose file cannot be read is an error line and counts as active", () => Effect.gen(function* () { const fake = new Fake(); - fake.runs.push({ id: rid(40), state: null, unreadable: true, confirmed: false }); + fake.runs.push({ + id: rid(40), + state: null, + unreadable: true, + confirmed: false, + session: false, + }); fake.ready[REPO] = [1]; fake.pushStart(`${REPO}#1`, started(1)); const { report, lines } = yield* pass(fake, machine([REPO], 1)); @@ -665,7 +679,13 @@ describe("runs that are confirmed and still in triage", () => { it.effect("a confirmed run with no state yet is resumed too", () => Effect.gen(function* () { const fake = new Fake(); - fake.runs.push({ id: rid(13), state: null, unreadable: false, confirmed: true }); + fake.runs.push({ + id: rid(13), + state: null, + unreadable: false, + confirmed: true, + session: false, + }); fake.pushStart(`${REPO}#13`, started(13)); yield* pass(fake, machine([REPO], 3)); assert.deepEqual(starts(fake), [`resume ${REPO}#13`]); @@ -710,17 +730,31 @@ describe("runs that are confirmed and still in triage", () => { }), ); - it.effect("a ready-again issue whose own run holds the only slot is an attention line", () => + it.effect( + "a ready-again issue whose own run has no session is claimed past that run's slot", + () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + const { report, lines } = yield* pass(fake, machine([REPO], 1)); + // The claim finishes the old run, which left nothing running, before it claims. + assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#12`]); + assert.ok(runLine(lines, "rawphp/softfac#12 triage:").includes("left to the claim step")); + assert.equal(report.attention, 0); + }), + ); + + it.effect("a ready-again issue whose own run's session holds the only slot is attention", () => Effect.gen(function* () { const fake = new Fake(); - fake.runIn(12, "triage"); + fake.runIn(12, "triage", REPO, true); fake.ready[REPO] = [12]; fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); const { report, lines } = yield* pass(fake, machine([REPO], 1)); - // `resume` never claims, and the run holds the only slot. + // A second session would run past the limit, so the claim waits. assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); - const note = runLine(lines, "rawphp/softfac#12 triage:"); - assert.ok(note.includes("ready again: the claim step takes it when a slot is free"), note); assert.equal(report.attention, 1); const line = runLine(lines, "ATTENTION: rawphp/softfac#12 "); assert.ok(line.includes("the new claim waits for a slot this run holds"), line); @@ -736,6 +770,19 @@ describe("runs that are confirmed and still in triage", () => { }), ); + it.effect("a ready-again issue that waits on another run's slot too raises no attention", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(40, BUILDING); + fake.runIn(12, "triage", REPO, true); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + const { report } = yield* pass(fake, machine([REPO], 2)); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + assert.equal(report.attention, 0); + }), + ); + it.effect("a ready issue the limit holds back by other runs raises no attention", () => Effect.gen(function* () { const fake = new Fake(); @@ -868,7 +915,13 @@ describe("claims that have no run in the ledger", () => { Effect.gen(function* () { // A stuck path that failed half way leaves a run with no approval behind; start finishes it. const fake = new Fake(); - fake.runs.push({ id: rid(12), state: "triage", unreadable: false, confirmed: false }); + fake.runs.push({ + id: rid(12), + state: "triage", + unreadable: false, + confirmed: false, + session: false, + }); fake.triage[REPO] = [12]; fake.pushStart(`${REPO}#12`, { kind: "stuck", run: rid(12), reason: "could not confirm" }); const { lines } = yield* pass(fake, machine([REPO], 3)); @@ -880,7 +933,13 @@ describe("claims that have no run in the ledger", () => { it.effect("a run that is not confirmed takes no slot", () => Effect.gen(function* () { const fake = new Fake(); - fake.runs.push({ id: rid(12), state: "triage", unreadable: false, confirmed: false }); + fake.runs.push({ + id: rid(12), + state: "triage", + unreadable: false, + confirmed: false, + session: false, + }); fake.ready[REPO] = [3]; fake.pushStart(`${REPO}#3`, started(3)); yield* pass(fake, machine([REPO], 1)); @@ -1414,7 +1473,13 @@ describe("what the pass records", () => { Effect.gen(function* () { const fake = new Fake(); fake.runIn(12, BUILDING); - fake.runs.push({ id: rid(13), state: null, unreadable: true, confirmed: false }); + fake.runs.push({ + id: rid(13), + state: null, + unreadable: true, + confirmed: false, + session: false, + }); const { report, lines } = yield* pass(fake, machine([REPO], 1)); assert.deepEqual(fake.events[0], { event: "pass_started", data: {} }); assert.deepEqual(fake.events.at(-1), { diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 9663c8a51f91..3f7884a96a1b 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -448,8 +448,8 @@ const workRuns = ( }); /** - * A ready issue the limit holds back while its own unfinished run holds a slot: the claim would - * finish that run (`finishOlderRuns`), so with no other slot freeing, nothing moves. + * A ready issue the limit holds back only because of its own triage run whose session may be + * running: only the claim would finish that run (`finishOlderRuns`), so the slot never frees. */ const waitsOnItself = (pass: Pass, notes: Notes, run: string): Effect.Effect => pass.attention( @@ -458,8 +458,11 @@ const waitsOnItself = (pass: Pass, notes: Notes, run: string): Effect.Effect + !run.unreadable && run.confirmed && (run.state === null || run.state === "triage"); /** * A confirmed run in `triage` that `resume` would not take, for `why`: a person moved or closed @@ -768,16 +771,24 @@ const claim = ( // the machine's, said once after the loop. if (paused !== null) continue; // Before the blockers: with no slot free nothing is claimed, so they are not read. Every - // issue left is counted as left over, blocked or not. - if (active >= machine.maxConcurrentRuns) { + // issue left is counted as left over, blocked or not. The issue's own triage runs end with + // this claim (`finishOlderRuns`). One with no session leaves nothing running, so its slot is + // the claim's; one whose session may be running keeps its slot, so no second session + // starts past the limit. + const own = known.of(name, issue).filter(inTriage); + const freed = own.filter((run) => !run.session).length; + if (active - freed >= machine.maxConcurrentRuns) { left += 1; yield* pass.record({ event: "claim_skipped", data: { repo: name, issue: String(issue), reason: "limit" }, }); - // Only this claim would finish the issue's own run, so a slot it holds never frees. - const own = known.of(name, issue).find(holdsSlot); - if (own !== undefined) yield* waitsOnItself(pass, notes, runIdToString(own.id)); + // Its own running sessions fill the limit alone: when every other run frees its slot, + // the claim still waits, and only the claim would end them. + const running = own.filter((run) => run.session); + if (running.length > 0 && running.length >= machine.maxConcurrentRuns) { + yield* waitsOnItself(pass, notes, runIdToString(running[0]!.id)); + } continue; } // A blocked issue takes no slot. Nothing is written for it, and a read that fails holds it From ea835ff80fffe2e9001784b02ba368a48b137132 Mon Sep 17 00:00:00 2001 From: Tom Kaczocha Date: Fri, 16 Oct 2026 12:19:11 +1000 Subject: [PATCH 16/16] test(factory): review-gate round 8: pin the limit around a re-readied issue's run Two pass tests: a re-readied issue whose own run has no session still waits when other runs fill the limit, and once claimed past its own run it takes the slot the next ready issue would have had (f21). workRuns picks its runs with the claim step's `inTriage`, so the two cannot drift, and the docs no longer say the run holds its slot until the claim (f22). --- apps/server/src/factory/pass.test.ts | 32 +++++++++++++++++++++++++ apps/server/src/factory/pass.ts | 5 ++-- apps/server/src/factory/runner/start.ts | 3 +-- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/apps/server/src/factory/pass.test.ts b/apps/server/src/factory/pass.test.ts index c5041778df4c..8fe0372fa1dd 100644 --- a/apps/server/src/factory/pass.test.ts +++ b/apps/server/src/factory/pass.test.ts @@ -746,6 +746,38 @@ describe("runs that are confirmed and still in triage", () => { }), ); + it.effect("a ready-again issue's freed slot does not beat the limit other runs fill", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(40, BUILDING); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + yield* pass(fake, machine([REPO], 1)); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`]); + assert.deepEqual( + fake.events.filter((e) => e.event === "claim_skipped").map((e) => e.data), + [{ repo: REPO, issue: "12", reason: "limit" }], + ); + }), + ); + + it.effect("a ready-again issue claimed past its own run takes the slot from the next issue", () => + Effect.gen(function* () { + const fake = new Fake(); + fake.runIn(12, "triage"); + fake.ready[REPO] = [12, 13]; + fake.pushStart(`${REPO}#12`, { kind: "skipped", reason: "ready again", moved: "ready" }); + fake.pushStart(`${REPO}#12`, started(12)); + yield* pass(fake, machine([REPO], 1)); + assert.deepEqual(starts(fake), [`resume ${REPO}#12`, `start ${REPO}#12`]); + assert.deepEqual( + fake.events.filter((e) => e.event === "claim_skipped").map((e) => e.data), + [{ repo: REPO, issue: "13", reason: "limit" }], + ); + }), + ); + it.effect("a ready-again issue whose own run's session holds the only slot is attention", () => Effect.gen(function* () { const fake = new Fake(); diff --git a/apps/server/src/factory/pass.ts b/apps/server/src/factory/pass.ts index 3f7884a96a1b..d0a952e9ee30 100644 --- a/apps/server/src/factory/pass.ts +++ b/apps/server/src/factory/pass.ts @@ -398,7 +398,7 @@ const runs = (world: PassWorld, pass: Pass): Effect.Effect => Effect.gen(function* () { for (const info of [...known.infos]) { - if (info.unreadable || !info.confirmed) continue; - if (info.state !== null && info.state !== "triage") continue; + if (!inTriage(info)) continue; const loaded = repos.find( (l) => l.kind === "ready" && sameRepo(l.repo.watched.repo, info.id.repo), ); diff --git a/apps/server/src/factory/runner/start.ts b/apps/server/src/factory/runner/start.ts index 6d4236c80a22..a2596150fe35 100644 --- a/apps/server/src/factory/runner/start.ts +++ b/apps/server/src/factory/runner/start.ts @@ -162,8 +162,7 @@ export const start = ( * `start` for a claim already made, which never makes one: what the pass calls for a run it * knows of and for a `triage` issue of ours with no run. An issue someone put back to * `factory:ready` is left to the claim step, which checks the pause, the limit and the blockers - * first. Until it claims the issue, which finishes the older runs, an unfinished run of the - * issue holds its slot. + * first, and which finishes the issue's older runs when it claims it. */ export const resume = ( ctx: StartContext,