diff --git a/src/main/automation/automation-run-completion.ts b/src/main/automation/automation-run-completion.ts index 67692216..4be1602a 100644 --- a/src/main/automation/automation-run-completion.ts +++ b/src/main/automation/automation-run-completion.ts @@ -177,6 +177,15 @@ function advanceToInReview( statusMessage: string, ): void { const { db, logger, publishDomainChanged } = deps; + // The tasks-status guard refuses an `in_review` transition for a + // top-level assignee (reports_to IS NULL) unless the task is + // flagged for user review. Auto-completed runs owned by the CEO + // routinely trip that path — pre-flag them so the transition + // succeeds and the human becomes the reviewer. + const agent = db.getAgent(run.agentId); + if (agent.reportsTo === null) { + db.setTaskRequiresUserReview(taskId, true); + } db.advanceTaskStatus(taskId, "in_review", statusMessage); callbacks.handleTaskStatusChange(taskId, run.companyId, "in_progress", "in_review"); const company = db.listSnapshot().companies.find((c) => c.id === run.companyId); @@ -184,7 +193,6 @@ function advanceToInReview( const importantDocs = findImportantDocsForRun(db, run.id, run.companyId); if (importantDocs.length > 0) { db.setTaskRequiresUserReview(taskId, true); - const agent = db.getAgent(run.agentId); if (agent.heartbeatEnabled) { db.setHeartbeat(agent.id, false, agent.heartbeatIntervalSec || 120); logger.info(`[deliverable-review] Paused heartbeat for ${agent.name} — deliverable flagged for review.`); diff --git a/src/main/database-base.ts b/src/main/database-base.ts index 57cfa22e..6fe17338 100644 --- a/src/main/database-base.ts +++ b/src/main/database-base.ts @@ -164,93 +164,17 @@ export class AppDatabaseBase { initializeDatabase(this.db, ra, tx); this.connectorsCrud.seedConnectors(); this.modelCapabilities.seedModelCapabilities(MODEL_CAPABILITY_SEED); + // Crash-recovery: a run still at `running` on boot means the worker + // died mid-turn. Flip it to `interrupted`, surface the related task + // as `blocked` for operator attention, and release workspace locks. + // This is the only mutation the startup path is permitted to make; + // all other "data-repair" passes have moved to explicit runtime + // state-machine guards (see `tasks-status.ts`) or maintenance IPC. this._lastInterruptedCount = this.interruptStaleRuns(); - this._lastOrphanResetCount = this.reconcileOrphanedTasks(); - this._lastUnreviewableResetCount = this.reconcileUnreviewableTasks(); this.settings.purgeExpiredRunLogs(); this.wakeup.purgeExpiredWakeupRequests(); } - /** - * Reset tasks stuck at `in_progress` with no live run. - * - * Invariant: `task.status = 'in_progress'` ⇒ there exists a run `r` - * where `r.status IN ('queued', 'running')` referenced by this task - * (via checkout_run_id or active_run_id). - * - * Whenever the invariant breaks — because a dispatch failed, a worker - * crashed, or a pre-fix finishRun cleared the run refs but not the - * status — the task becomes un-reclaimable: `checkoutTask`'s guard - * requires `status IN ('todo','backlog')`, and heartbeat loops forever - * logging "lost checkout race". One query at boot drains every such - * orphan, regardless of which of the two reference columns happens to - * be null. The runtime `finishRun` path enforces the same invariant - * going forward, so this should stabilise at zero rows changed. - */ - protected reconcileOrphanedTasks(): number { - const result = this.db - .prepare( - `UPDATE tasks - SET status = 'todo', - checkout_run_id = NULL, - execution_locked_at = NULL, - active_run_id = NULL, - updated_at = ? - WHERE status = 'in_progress' - AND NOT EXISTS ( - SELECT 1 FROM runs r - WHERE (r.id = tasks.checkout_run_id OR r.id = tasks.active_run_id) - AND r.status IN ('queued', 'running') - )`, - ) - .run(new Date().toISOString()); - const changes = Number(result.changes); - if (changes > 0) { - - console.info(`[init] Reconciled ${changes} orphaned in_progress task(s) → todo.`); - } - return changes; - } - - /** - * Reset tasks stuck at `in_review` with no reachable reviewer. - * - * A task enters `in_review` when an assignee submits work for approval. - * The reviewer is the assignee's manager (walk up via `reports_to`), - * or the human operator when the task is flagged - * `requires_user_review = 1`. When the assignee is a top-level agent - * (CEO/board, `reports_to IS NULL`) and the task does not require - * user review, *no one* can review it — the task becomes a dead - * letter, and the CEO heartbeat falls into a loop re-proposing its - * `approve_ceo_strategy` without ever recovering. - * - * This observed pattern cost us a full debugging session: CEO - * self-assigned a large task, moved it to `in_review`, then spent - * every heartbeat cycle re-emitting the same approval. Demoting the - * task to `todo` breaks the loop — the CEO sees un-reviewable work - * and correctly falls through to the hire path instead. - */ - protected reconcileUnreviewableTasks(): number { - const result = this.db - .prepare( - `UPDATE tasks - SET status = 'todo', updated_at = ? - WHERE status = 'in_review' - AND requires_user_review = 0 - AND assignee_agent_id IS NOT NULL - AND assignee_agent_id IN ( - SELECT id FROM agents WHERE reports_to IS NULL - )`, - ) - .run(new Date().toISOString()); - const changes = Number(result.changes); - if (changes > 0) { - - console.info(`[init] Reconciled ${changes} un-reviewable in_review task(s) → todo.`); - } - return changes; - } - close() { this.db.close(); } // ─── Settings ────────────────────────────────────────────────────────────── @@ -435,11 +359,7 @@ export class AppDatabaseBase { // ─── Internal infrastructure ─────────────────────────────────────────────── protected _lastInterruptedCount = 0; - protected _lastOrphanResetCount = 0; - protected _lastUnreviewableResetCount = 0; get lastInterruptedCount(): number { return this._lastInterruptedCount; } - get lastOrphanResetCount(): number { return this._lastOrphanResetCount; } - get lastUnreviewableResetCount(): number { return this._lastUnreviewableResetCount; } interruptStaleRuns(): number { const ts = nowIso(); diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 44971d76..baaec707 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -74,10 +74,19 @@ export function migrateSecretsTable(db: DatabaseSync, readAll: ReadAll) { db.prepare("create unique index if not exists secrets_company_name_idx on secrets(company_id, name)").run(); return; } - const fallbackCompanyId = - ((db.prepare("select value from settings where key = 'current_company_id'").get() as { value: string } | undefined)?.value) ?? - ((db.prepare("select id from companies order by created_at asc limit 1").get() as { id: string } | undefined)?.id) ?? - "00000000-0000-0000-0000-000000000000"; + // Legacy profiles predate the per-company secrets table. Rebind each + // row to the current company; if neither `current_company_id` nor any + // company row exists we refuse rather than attach secrets to a + // synthetic zero UUID that references nothing — that would leak + // ciphertext with no deletion path and no UI visibility. + const currentCompany = db.prepare("select value from settings where key = 'current_company_id'").get() as { value: string } | undefined; + const firstCompany = db.prepare("select id from companies order by created_at asc limit 1").get() as { id: string } | undefined; + const fallbackCompanyId = currentCompany?.value ?? firstCompany?.id; + if (!fallbackCompanyId) { + throw new Error( + "Cannot migrate legacy secrets table: no current_company_id is set and no company rows exist. Create a company before upgrading, or drop the legacy secrets table if the rows are stale.", + ); + } db.exec(` alter table secrets rename to secrets_legacy; create table secrets (id text primary key, company_id text not null, name text not null, description text not null, backend text not null, ciphertext blob not null, updated_at text not null, created_at text not null, unique(company_id, name)); diff --git a/src/main/database/tasks-status.ts b/src/main/database/tasks-status.ts index fe262ff9..9c7aa8d8 100644 --- a/src/main/database/tasks-status.ts +++ b/src/main/database/tasks-status.ts @@ -37,6 +37,37 @@ export function buildTaskStatusMethods( "cancelled", ]); + /** + * Guard against the "in_review dead letter": a task whose assignee is + * a top-level agent (reports_to IS NULL, the DB invariant CEO marker) + * and that does not require user review has no reachable reviewer — + * no manager to walk up to, and no human gate. Without this guard the + * CEO heartbeat loops forever re-proposing the same approval. + * + * The right root fix is to reject the transition at source: a caller + * that moves such a task into `in_review` must first set + * requires_user_review=1 (escalate to human) or route differently + * (advance to done, hire a reviewer, bounce to todo). Previous + * versions silently resurrected these tasks at every DB init; this + * replaces the startup side-effect with a clear runtime error. + */ + function guardInReviewHasReviewer(taskId: string): void { + const row = readAll<{ requiresUserReview: number; reportsTo: string | null }>( + `select t.requires_user_review as requiresUserReview, + a.reports_to as reportsTo + from tasks t + left join agents a on a.id = t.assignee_agent_id + where t.id = ?`, + taskId, + )[0]; + if (!row) return; + if (row.requiresUserReview === 1) return; + if (row.reportsTo !== null) return; + throw new Error( + `Task ${taskId} has a top-level assignee and no reviewer: cannot enter in_review without requires_user_review=1.`, + ); + } + return { advanceTaskStatus( taskId: string, @@ -52,6 +83,7 @@ export function buildTaskStatusMethods( if (!task) throw new Error(`Task not found: ${taskId}`); const previousStatus = task.status; if (previousStatus === newStatus) return { previousStatus }; + if (newStatus === "in_review") guardInReviewHasReviewer(taskId); const ts = nowIso(); // Status side-effect timestamps (Paperclip pattern) diff --git a/tests/unit/atomic-checkout.test.ts b/tests/unit/atomic-checkout.test.ts index 5780998f..88101177 100644 --- a/tests/unit/atomic-checkout.test.ts +++ b/tests/unit/atomic-checkout.test.ts @@ -420,124 +420,45 @@ describe("CAS checkout", () => { expect(db.getTask(taskId).status).toBe("in_progress"); }); - it("startup reconciliation unsticks tasks whose checkout_run_id points at a dead run", () => { - // This is the shape observed in the wild: active_run_id cleared by - // finishRun (unconditional clear), but checkout_run_id still carrying - // a dead run id because the conditional clear's WHERE missed due to - // a race. Task sits in 'in_progress' forever — heartbeat picks it - // (activeRunId null), checkoutTask refuses (status != todo/backlog), - // "lost checkout race" on loop. Reconciliation must fix it without - // caring which reference column is null. + it("advanceTaskStatus('in_review') rejects a CEO self-review that has no reviewer", () => { + // Regression guard for the CEO dead-letter case: a task whose + // assignee is a top-level agent (reports_to IS NULL) and that is + // not flagged for user review has no reachable reviewer. Previous + // versions silently resurrected these tasks at every DB init; the + // root fix rejects the transition at source so callers either + // escalate to the human or leave the task in a reclaimable state. const companyId = createTestCompany(db); createTestConnector(db); const workspaceId = createTestWorkspace(db, companyId); - const agentId = createTestAgent(db, companyId, workspaceId); - const taskId = createTestTask(db, companyId, null, workspaceId); - - const { id: deadRunId } = db.createRun({ - companyId, - taskId, - agentId, - workspaceId, - connectorId: "openai-codex", - }); - db.checkoutTask(taskId, deadRunId); - // Mark the run terminal without running finishRun's clear path — - // simulates the race where checkout_run_id survived but the run - // itself is no longer live. - (db as unknown as RawDb).db - .prepare("UPDATE runs SET status = 'cancelled' WHERE id = ?") - .run(deadRunId); - (db as unknown as RawDb).db - .prepare("UPDATE tasks SET active_run_id = NULL WHERE id = ?") - .run(taskId); - - // Pre-condition: task is stuck. - const stuck = (db as unknown as RawDb).db - .prepare("SELECT status, checkout_run_id as checkoutRunId, active_run_id as activeRunId FROM tasks WHERE id = ?") - .get(taskId) as { status: string; checkoutRunId: string | null; activeRunId: string | null }; - expect(stuck.status).toBe("in_progress"); - expect(stuck.checkoutRunId).toBe(deadRunId); - expect(stuck.activeRunId).toBeNull(); - - // Re-initialise: reconciliation should run. - db.close(); - db = new AppDatabase(tempDir); - db.init(); - - expect(db.lastOrphanResetCount).toBe(1); - const reclaimed = db.getTask(taskId); - expect(reclaimed.status).toBe("todo"); - - // And a fresh checkout must succeed — proving the task is reclaimable. - const { id: newRunId } = db.createRun({ - companyId, - taskId, - agentId, - workspaceId, - connectorId: "openai-codex", - }); - expect(db.checkoutTask(taskId, newRunId)).toBe(true); - }); - - it("startup reconciliation unsticks in_review tasks with no possible reviewer", () => { - // Observed in production: a fresh `ai_ceo` company with only a CEO - // self-assigned a large task and set status=in_review. The reviewer - // would be the assignee's manager (walk up reports_to), but the CEO - // has no manager. With `requires_user_review = 0` there is no human - // fallback either. The heartbeat falls into a loop re-proposing - // `approve_ceo_strategy` every cycle without ever hiring or moving - // the task. Demoting to 'todo' breaks the loop. - const companyId = createTestCompany(db); - createTestConnector(db); - const workspaceId = createTestWorkspace(db, companyId); - // Top-level agent (CEO): reports_to stays NULL. const ceoId = createTestAgent(db, companyId, workspaceId, "CEO"); const taskId = createTestTask(db, companyId, ceoId, workspaceId); - // Manually wedge the task into the dead-letter shape. - (db as unknown as RawDb).db - .prepare("UPDATE tasks SET status = 'in_review', requires_user_review = 0 WHERE id = ?") - .run(taskId); - expect(db.getTask(taskId).status).toBe("in_review"); - - // Re-init triggers reconciliation. - db.close(); - db = new AppDatabase(tempDir); - db.init(); - - expect(db.lastUnreviewableResetCount).toBe(1); + expect(() => db.advanceTaskStatus(taskId, "in_review")).toThrow(/no reviewer/); + // Task status must remain unchanged after the rejected transition. expect(db.getTask(taskId).status).toBe("todo"); }); - it("in_review tasks requiring user review are left alone", () => { - // Opposite guard: when requires_user_review = 1 the task is - // intentionally parked waiting for a human — the reconciler must - // not steal it. + it("advanceTaskStatus('in_review') accepts a CEO assignee flagged requires_user_review=1", () => { + // The same task becomes legitimate when the human is the reviewer. const companyId = createTestCompany(db); createTestConnector(db); const workspaceId = createTestWorkspace(db, companyId); const ceoId = createTestAgent(db, companyId, workspaceId, "CEO"); const taskId = createTestTask(db, companyId, ceoId, workspaceId); - (db as unknown as RawDb).db - .prepare("UPDATE tasks SET status = 'in_review', requires_user_review = 1 WHERE id = ?") - .run(taskId); + db.setTaskRequiresUserReview(taskId, true); - db.close(); - db = new AppDatabase(tempDir); - db.init(); - - expect(db.lastUnreviewableResetCount).toBe(0); + db.advanceTaskStatus(taskId, "in_review"); expect(db.getTask(taskId).status).toBe("in_review"); }); - it("reconciliation does not fire when interruptStaleRuns already handled a running task", () => { - // Opposite guard: tasks whose run was `running` at shutdown are - // handled earlier in init by `interruptStaleRuns` — it marks the - // run 'interrupted' and sets the task to 'blocked' for human - // review. By the time reconciliation runs, the task is no longer - // 'in_progress', so the cleanup's WHERE clause skips it. The user - // sees the task surfaced in the recovery UI, not silently reset. + it("interruptStaleRuns surfaces shutdown-era running runs without startup task rewrites", () => { + // `interruptStaleRuns` is the ONLY mutation the startup path may + // perform: any run at 'running' on boot means a crash, so the run + // flips to 'interrupted' and the task surfaces as 'blocked' for + // operator attention. No other reconciliation runs at init; the + // task-status machine (see guardInReviewHasReviewer) and the + // runtime `finishRun` guards are now responsible for every other + // data-repair concern. const companyId = createTestCompany(db); createTestConnector(db); const workspaceId = createTestWorkspace(db, companyId); @@ -558,10 +479,7 @@ describe("CAS checkout", () => { db = new AppDatabase(tempDir); db.init(); - // Reconciliation counted 0 because interruptStaleRuns moved the - // task to 'blocked' first. Both guards collaborate — neither - // steals a task out from under an actively-working run. - expect(db.lastOrphanResetCount).toBe(0); + expect(db.lastInterruptedCount).toBe(1); expect(db.getTask(taskId).status).toBe("blocked"); }); }); diff --git a/tests/unit/resolve-workspace.test.ts b/tests/unit/resolve-workspace.test.ts index 792222e9..90b2a90e 100644 --- a/tests/unit/resolve-workspace.test.ts +++ b/tests/unit/resolve-workspace.test.ts @@ -16,10 +16,12 @@ import { AppDatabase } from "../../src/main/database"; import { resolveWorkspaceWithLockMode } from "../../src/main/orchestration/resolve-workspace"; /** - * Regression tests for the workspace lock policy introduced in v0.4.168+: - * only `task.workspaceId` grants exclusive access. Agent-, project-, and - * company-scoped resolutions are all `shared` so teammates can run in - * parallel instead of queuing on a single workspace lock. + * Regression tests for the current workspace lock policy: every + * resolved workspace is held exclusively for the duration of the run. + * Pi's coding tools always include bash/write/edit, so there is no + * read-only category where concurrent runs are safe; teammates + * sharing one repo queue through the wakeup table instead of racing + * the filesystem. * * See `resolve-workspace.ts` for the full rationale. */ @@ -75,7 +77,7 @@ describe("resolveWorkspaceWithLockMode", () => { expect(result.workspace?.id).toBe(workspaceId); }); - it("agent-bound workspace → shared (normal case: teammates share a repo)", () => { + it("agent-bound workspace → exclusive (single writer per repo)", () => { const { companyId, workspaceId } = seedCompany(db); const result = resolveWorkspaceWithLockMode( db, @@ -84,10 +86,10 @@ describe("resolveWorkspaceWithLockMode", () => { companyId, ); expect(result.source).toBe("agent"); - expect(result.lockMode).toBe("shared"); + expect(result.lockMode).toBe("exclusive"); }); - it("project-bound workspace → shared (team collaboration space)", () => { + it("project-bound workspace → exclusive (single writer per repo)", () => { const { companyId } = seedCompany(db); const projectId = db.saveProject({ companyId, @@ -95,9 +97,6 @@ describe("resolveWorkspaceWithLockMode", () => { description: "", status: "active", }); - // Create a second workspace bound to the project so the resolver's - // project-primary branch actually fires (the default seeded workspace - // has no project_id set). db.saveWorkspace({ companyId, name: "Project repo", @@ -115,10 +114,10 @@ describe("resolveWorkspaceWithLockMode", () => { companyId, ); expect(result.source).toBe("project"); - expect(result.lockMode).toBe("shared"); + expect(result.lockMode).toBe("exclusive"); }); - it("company-fallback → shared (unchanged legacy behavior)", () => { + it("company-fallback → exclusive (single writer per repo)", () => { const { companyId } = seedCompany(db); const result = resolveWorkspaceWithLockMode( db, @@ -127,7 +126,7 @@ describe("resolveWorkspaceWithLockMode", () => { companyId, ); expect(result.source).toBe("company_fallback"); - expect(result.lockMode).toBe("shared"); + expect(result.lockMode).toBe("exclusive"); }); it("task workspace wins over agent workspace even when both are set", () => {