diff --git a/scripts/codemod/remove-appdatabase-passthrough.mjs b/scripts/codemod/remove-appdatabase-passthrough.mjs index 670749b0..be4ed448 100644 --- a/scripts/codemod/remove-appdatabase-passthrough.mjs +++ b/scripts/codemod/remove-appdatabase-passthrough.mjs @@ -322,7 +322,6 @@ const METHOD_TO_MODULE = { getWorkspace: "workspacesMod", getProjectPrimaryWorkspace: "workspacesMod", listWorkspaceLocalPaths: "workspacesMod", - getAnyCompanyWorkspace: "workspacesMod", deleteWorkspace: "workspacesMod", acquireWorkspaceLock: "workspacesMod", claimWorkspaceLock: "workspacesMod", diff --git a/src/main/database/workspaces.ts b/src/main/database/workspaces.ts index 0dc448c1..57f59687 100644 --- a/src/main/database/workspaces.ts +++ b/src/main/database/workspaces.ts @@ -68,15 +68,6 @@ export function buildWorkspaceMethods( return rows.map((r) => r.localPath).filter(Boolean); }, - getAnyCompanyWorkspace(companyId: string): WorkspaceRecord | null { - const row = getDb() - .prepare( - "select id, company_id as companyId, name, local_path as localPath, repo_url as repoUrl, repo_ref as repoRef, project_id as projectId, state, is_primary as isPrimary, created_at as createdAt, updated_at as updatedAt from workspaces where company_id = ? order by is_primary desc, created_at asc limit 1", - ) - .get(companyId) as WorkspaceRecord | undefined; - return row ? { ...row, isPrimary: toBoolean(row.isPrimary) } : null; - }, - deleteWorkspace(id: string, companyId: string) { transaction(() => { getDb().prepare("update agents set workspace_id = null where workspace_id = ? and company_id = ?").run(id, companyId); diff --git a/src/main/orchestration/resolve-workspace.ts b/src/main/orchestration/resolve-workspace.ts index 58bb4271..4283bd8b 100644 --- a/src/main/orchestration/resolve-workspace.ts +++ b/src/main/orchestration/resolve-workspace.ts @@ -1,61 +1,192 @@ import type { AppDatabase } from "../database"; +import type { AgentRecord, TaskRecord, WorkspaceRecord } from "@shared/types"; import type { WorkspaceLockMode } from "./types"; /** - * Resolve the workspace and lock semantics for a run context. + * Per-task workspace lock inference (v52+). * - * Lock policy: every resolved workspace is held exclusively for the - * duration of the run. Pi's `createCodingTools()` hands every agent - * the mutating `bash`, `write`, and `edit` tools, so there is no - * read-only category where concurrent runs are safe — two agents - * editing the same working tree step on each other's files and - * corrupt git state before the per-repo `index.lock` even gets - * touched. + * The core invariant is: `lockMode='exclusive'` is an *opt-in*, + * `lockMode='none'` is the default. Previously every resolved + * workspace was held exclusively, which folded four runnable + * agents onto a single company workspace lock and serialised the + * entire C-suite. The fix is not to pick a different fallback — it + * is to stop inferring filesystem use from the shape of the graph + * and ask the task row itself. * - * Contention is handled at dispatch time: `run-dispatch.ts` tries - * `claimWorkspaceLock` synchronously. A lost race finishes the run - * as `cancelled` (not `failed` — the Dashboard's "Needs attention" - * card ignores cancellations), and the next heartbeat retries on - * its own cadence. The caller must keep that rollback path alive; - * without it contention would leave tasks pinned at `in_progress` - * forever. + * `inferLockMode` returns the pair of decisions the dispatcher + * needs (mode + which workspace, if any). `resolveWorkspaceWithLockMode` + * is the thin outward-facing wrapper that loads the actual + * `WorkspaceRecord` for callers that need the path / companyId. * - * When no workspace can be resolved (headless heartbeat runs that - * have no repo to touch) we return `none`; the worker uses - * `process.cwd()` as a read-only cwd and the dispatcher skips the - * lock claim. + * Silent fallbacks removed in this rewrite: + * + * • `company_fallback` — previously the fourth fallback step + * returned *any* workspace row in the company as the lock target. + * That was silent corruption's favourite mechanism: four agents + * with no workspace binding would all land on the same row and + * fight for one mutex. A task that needs a filesystem and has no + * legitimate binding now throws `ConfigurationError` at resolve + * time so the caller decides (skip heartbeat, surface error, + * whatever) instead of the resolver guessing. + * + * • No `lockMode='exclusive'` ever returns a null workspace. The + * dispatcher assumed this invariant (it claims a lock on + * `workspace.id` when mode is exclusive). The types now enforce + * it: exclusive always comes with a resolved workspace, none + * always comes with `workspace=null`. + * + * Coding-role inference (rule 2) is a safety net for the one- + * migration transition window where legacy rows may not yet have a + * meaningful `requires_filesystem` column value set by the creator. + * Once `saveTask` is taught to set the column explicitly at insert + * time (follow-up commit), rule 2 becomes a no-op. + */ + +export type WorkspaceResolutionSource = + | "task" + | "agent" + | "project" + | "none"; + +export interface WorkspaceResolution { + workspace: WorkspaceRecord | null; + lockMode: WorkspaceLockMode; + source: WorkspaceResolutionSource; +} + +export class WorkspaceConfigurationError extends Error { + readonly code = "WORKSPACE_UNRESOLVED"; + constructor(message: string) { + super(message); + this.name = "WorkspaceConfigurationError"; + } +} + +const CODING_ROLE_PATTERN = /cto|engineer|developer|devops|sre/i; + +/** + * Accept the widened shapes call sites actually pass. `projectId` + * lives as `string | null | undefined` in several historical call + * sites (legacy inline types), and `role`/`department` are optional + * because taskless heartbeats supply only the runtime-needed subset. + */ +type TaskInput = { + workspaceId: TaskRecord["workspaceId"]; + projectId?: TaskRecord["projectId"] | undefined; + requiresFilesystem?: boolean; +}; + +type AgentInput = Pick & { + role?: string | null; + department?: AgentRecord["department"]; +}; + +/** + * Decide whether this (task, agent) pair should hold the workspace + * exclusive for the duration of the run. Returns the mode and the + * workspace-id the lock should sit on; callers load the row + * afterwards. + * + * Decision table: + * + * 1. `task.workspaceId` set → exclusive on that workspace + * (explicit opt-in by the task author, e.g. PR branch tasks). + * + * 2. Task signals filesystem need (`requiresFilesystem === true` + * OR agent role matches coding pattern OR department = + * engineering) → exclusive on agent.workspaceId / project + * primary. Throws if neither exists. + * + * 3. Otherwise → none / null. Pure cognitive work — strategy, + * planning, document drafting, financial analysis, PM decisions + * — runs with no workspace and no lock. + */ +function inferLockMode( + db: AppDatabase, + task: TaskInput | null, + agent: AgentInput, + companyId: string, +): { lockMode: WorkspaceLockMode; workspaceId: string | null; source: WorkspaceResolutionSource } { + // Rule 1: task-level explicit workspace binding is the strongest + // signal available — the task row literally names the workspace. + if (task?.workspaceId) { + return { lockMode: "exclusive", workspaceId: task.workspaceId, source: "task" }; + } + + const taskSignalsFilesystem = task?.requiresFilesystem === true; + const agentSignalsFilesystem = + (typeof agent.role === "string" && CODING_ROLE_PATTERN.test(agent.role)) || + agent.department === "engineering"; + + const needsFilesystem = taskSignalsFilesystem || agentSignalsFilesystem; + if (!needsFilesystem) { + return { lockMode: "none", workspaceId: null, source: "none" }; + } + + // Rule 2: filesystem-touching task → agent workspace first, then + // project primary. We do not fall back to `any company workspace` + // any more — that path was the cause of the single-lock pile-up. + if (agent.workspaceId) { + return { lockMode: "exclusive", workspaceId: agent.workspaceId, source: "agent" }; + } + + const projectWorkspace = db.workspacesMod.getProjectPrimaryWorkspace( + task?.projectId ?? null, + companyId, + ); + if (projectWorkspace) { + return { + lockMode: "exclusive", + workspaceId: projectWorkspace.id, + source: "project", + }; + } + + throw new WorkspaceConfigurationError( + `Task requires a filesystem-backed run but no workspace is bound to the task, agent, or project (companyId=${companyId}).`, + ); +} + +/** + * Resolve the workspace + lock mode for a run context. + * + * Contract: + * • lockMode 'exclusive' ⇒ workspace is a live WorkspaceRecord. + * • lockMode 'none' ⇒ workspace is null. + * + * Violations throw `WorkspaceConfigurationError` at resolve time so + * the caller chooses how to recover (skip heartbeat, cancel run, + * surface to operator). There is no silent mode degradation. */ export function resolveWorkspaceWithLockMode( db: AppDatabase, - task: { workspaceId: string | null; projectId?: string | null } | null, - agent: { workspaceId: string | null }, + task: TaskInput | null, + agent: AgentInput, companyId: string, -): { - workspace: ReturnType; - lockMode: WorkspaceLockMode; - source: "task" | "agent" | "project" | "company_fallback" | "none"; -} { - if (task) { - const taskWorkspace = db.workspacesMod.getWorkspace(task.workspaceId); - if (taskWorkspace) { - return { workspace: taskWorkspace, lockMode: "exclusive", source: "task" }; - } +): WorkspaceResolution { + const decision = inferLockMode(db, task, agent, companyId); + + if (decision.lockMode === "none") { + return { workspace: null, lockMode: "none", source: "none" }; } - const agentWorkspace = db.workspacesMod.getWorkspace(agent.workspaceId); - if (agentWorkspace) { - return { workspace: agentWorkspace, lockMode: "exclusive", source: "agent" }; + if (!decision.workspaceId) { + throw new WorkspaceConfigurationError( + `inferLockMode returned exclusive with no workspaceId (companyId=${companyId}) — lock mode/workspace invariant broken.`, + ); } - const projectWorkspace = db.workspacesMod.getProjectPrimaryWorkspace(task?.projectId ?? null, companyId); - if (projectWorkspace) { - return { workspace: projectWorkspace, lockMode: "exclusive", source: "project" }; + const workspace = db.workspacesMod.getWorkspace(decision.workspaceId); + if (!workspace) { + throw new WorkspaceConfigurationError( + `Resolved workspace ${decision.workspaceId} does not exist (companyId=${companyId}, source=${decision.source}).`, + ); + } + if (workspace.companyId !== companyId) { + throw new WorkspaceConfigurationError( + `Resolved workspace ${workspace.id} belongs to company ${workspace.companyId}, not ${companyId}.`, + ); } - const fallbackWorkspace = db.workspacesMod.getAnyCompanyWorkspace(companyId); - if (fallbackWorkspace) { - return { workspace: fallbackWorkspace, lockMode: "exclusive", source: "company_fallback" }; - } - - return { workspace: null, lockMode: "none", source: "none" }; + return { workspace, lockMode: "exclusive", source: decision.source }; } diff --git a/tests/unit/resolve-workspace.test.ts b/tests/unit/resolve-workspace.test.ts index ba63a9cc..2a920a87 100644 --- a/tests/unit/resolve-workspace.test.ts +++ b/tests/unit/resolve-workspace.test.ts @@ -13,17 +13,22 @@ vi.mock("electron", () => ({ })); import { AppDatabase } from "../../src/main/database"; -import { resolveWorkspaceWithLockMode } from "../../src/main/orchestration/resolve-workspace"; +import { + resolveWorkspaceWithLockMode, + WorkspaceConfigurationError, +} from "../../src/main/orchestration/resolve-workspace"; /** - * 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. + * Workspace lock policy (v52+). * - * See `resolve-workspace.ts` for the full rationale. + * `lockMode='exclusive'` is opt-in — either the task carries an + * explicit workspaceId, or the task's requiresFilesystem column is + * true, or the resolving agent is in a coding role. Every other + * case returns `lockMode='none'` and `workspace=null`, so four + * pure-cognitive agents run fully in parallel without queueing on + * a single workspace lock. The previous `company_fallback` step + * (which returned any workspace in the company as the lock target) + * has been removed and now throws `WorkspaceConfigurationError`. */ function seedCompany(db: AppDatabase) { @@ -64,12 +69,12 @@ describe("resolveWorkspaceWithLockMode", () => { await rm(tempDir, { recursive: true, force: true }); }); - it("task-bound workspace → exclusive (explicit opt-in)", () => { + it("task.workspaceId set → exclusive on that workspace", () => { const { companyId, workspaceId } = seedCompany(db); const result = resolveWorkspaceWithLockMode( db, { workspaceId, projectId: null }, - { workspaceId: null }, + { workspaceId: null, role: "CMO", department: "marketing" }, companyId, ); expect(result.source).toBe("task"); @@ -77,19 +82,20 @@ describe("resolveWorkspaceWithLockMode", () => { expect(result.workspace?.id).toBe(workspaceId); }); - it("agent-bound workspace → exclusive (single writer per repo)", () => { + it("coding-role agent + agent workspace → exclusive on agent workspace", () => { const { companyId, workspaceId } = seedCompany(db); const result = resolveWorkspaceWithLockMode( db, null, - { workspaceId }, + { workspaceId, role: "CTO", department: "engineering" }, companyId, ); expect(result.source).toBe("agent"); expect(result.lockMode).toBe("exclusive"); + expect(result.workspace?.id).toBe(workspaceId); }); - it("project-bound workspace → exclusive (single writer per repo)", () => { + it("coding-role agent + project primary workspace → exclusive on project workspace", () => { const { companyId } = seedCompany(db); const projectId = db.entityCrud.saveProject({ companyId, @@ -97,7 +103,7 @@ describe("resolveWorkspaceWithLockMode", () => { description: "", status: "active", }); - db.workspacesMod.saveWorkspace({ + const projectWsId = db.workspacesMod.saveWorkspace({ companyId, name: "Project repo", localPath: "/tmp/project-ws", @@ -106,30 +112,43 @@ describe("resolveWorkspaceWithLockMode", () => { projectId, isPrimary: true, }); - const result = resolveWorkspaceWithLockMode( db, - { workspaceId: null, projectId }, - { workspaceId: null }, + { workspaceId: null, projectId, requiresFilesystem: true }, + { workspaceId: null, role: "CEO", department: "executive" }, companyId, ); expect(result.source).toBe("project"); expect(result.lockMode).toBe("exclusive"); + expect(result.workspace?.id).toBe(projectWsId); }); - it("company-fallback → exclusive (single writer per repo)", () => { + it("task.requiresFilesystem signals exclusive even for non-coding role", () => { + const { companyId, workspaceId } = seedCompany(db); + const result = resolveWorkspaceWithLockMode( + db, + { workspaceId: null, projectId: null, requiresFilesystem: true }, + { workspaceId, role: "COO", department: "operations" }, + companyId, + ); + expect(result.lockMode).toBe("exclusive"); + expect(result.source).toBe("agent"); + }); + + it("pure cognitive task (no coding role, no requiresFilesystem) → none", () => { const { companyId } = seedCompany(db); const result = resolveWorkspaceWithLockMode( db, - null, - { workspaceId: null }, + { workspaceId: null, projectId: null }, + { workspaceId: null, role: "CMO", department: "marketing" }, companyId, ); - expect(result.source).toBe("company_fallback"); - expect(result.lockMode).toBe("exclusive"); + expect(result.source).toBe("none"); + expect(result.lockMode).toBe("none"); + expect(result.workspace).toBeNull(); }); - it("task workspace wins over agent workspace even when both are set", () => { + it("task.workspaceId wins over agent.workspaceId even when both are set", () => { const { companyId, workspaceId: agentWsId } = seedCompany(db); const taskWsId = db.workspacesMod.saveWorkspace({ companyId, @@ -142,7 +161,7 @@ describe("resolveWorkspaceWithLockMode", () => { const result = resolveWorkspaceWithLockMode( db, { workspaceId: taskWsId, projectId: null }, - { workspaceId: agentWsId }, + { workspaceId: agentWsId, role: "CTO", department: "engineering" }, companyId, ); expect(result.source).toBe("task"); @@ -150,7 +169,23 @@ describe("resolveWorkspaceWithLockMode", () => { expect(result.workspace?.id).toBe(taskWsId); }); - it("no workspace at all → none", () => { + it("filesystem-requiring task with no resolvable workspace → throws WorkspaceConfigurationError", () => { + const companyId = db.company.saveCompany({ + name: "Empty Co", + description: "", + status: "active", + }); + expect(() => + resolveWorkspaceWithLockMode( + db, + { workspaceId: null, projectId: null, requiresFilesystem: true }, + { workspaceId: null, role: "CTO", department: "engineering" }, + companyId, + ), + ).toThrow(WorkspaceConfigurationError); + }); + + it("non-coding role with no workspace bindings → none (never throws, never falls back to shared lock)", () => { const companyId = db.company.saveCompany({ name: "Empty Co", description: "", @@ -159,11 +194,27 @@ describe("resolveWorkspaceWithLockMode", () => { const result = resolveWorkspaceWithLockMode( db, null, - { workspaceId: null }, + { workspaceId: null, role: "CFO", department: "finance" }, companyId, ); - expect(result.source).toBe("none"); expect(result.lockMode).toBe("none"); expect(result.workspace).toBeNull(); }); + + it("four non-coding agents resolving in parallel all get lockMode=none (no shared lock collision)", () => { + const { companyId } = seedCompany(db); + const roles = [ + { role: "CEO", department: "executive" as const }, + { role: "CMO", department: "marketing" as const }, + { role: "CPO", department: "product" as const }, + { role: "COO", department: "operations" as const }, + ]; + const modes = roles.map((r) => + resolveWorkspaceWithLockMode(db, null, { workspaceId: null, ...r }, companyId), + ); + for (const m of modes) { + expect(m.lockMode).toBe("none"); + expect(m.workspace).toBeNull(); + } + }); });