From 4765683b86b42e5e3de033db6c0dfac79cc7f28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Thu, 8 Oct 2026 17:34:40 -0400 Subject: [PATCH] Q107: a task approval covers the content it was given; edits need a fresh one Co-Authored-By: Claude Opus 5.5 --- .../085-approval-subject-snapshot.ts | 12 ++++ src/main/database/schema.ts | 2 + src/main/services/organization-service.ts | 66 ++++++++++--------- src/main/services/task-approval.ts | 4 ++ src/main/services/task-mutations.ts | 8 ++- src/main/services/task-run-preparation.ts | 20 ++---- src/renderer/src/i18n/zh.ts | 1 + src/renderer/src/pages/ApprovalsPage.tsx | 7 +- src/shared/domain.ts | 3 +- tests/unit/domain-services.test.ts | 2 +- tests/unit/execution-modes.test.ts | 39 ++++++++++- tests/unit/task-review.test.ts | 2 +- 12 files changed, 114 insertions(+), 52 deletions(-) create mode 100644 src/main/database/migrations/085-approval-subject-snapshot.ts create mode 100644 src/main/services/task-approval.ts diff --git a/src/main/database/migrations/085-approval-subject-snapshot.ts b/src/main/database/migrations/085-approval-subject-snapshot.ts new file mode 100644 index 00000000..b87230e3 --- /dev/null +++ b/src/main/database/migrations/085-approval-subject-snapshot.ts @@ -0,0 +1,12 @@ +// Existing task approvals are taken to cover the task as it stands now; from here on an edit needs a fresh one. +export const approvalSubjectSnapshotSchema = ` + alter table approvals add column subject_snapshot text; + update approvals set subject_snapshot = ( + select json_object('title', t.title, 'description', t.description, 'assignee', t.assignee_agent_id, 'risk', t.risk, 'workspace', t.workspace_id, 'filesystem', t.requires_filesystem) + from tasks t where t.id = approvals.source_entity_id + ) + where source_entity_type = 'task' and kind in ('task_risk', 'task_execution'); + + insert into schema_migrations(version, name, applied_at) + values (85, 'approval_subject_snapshot', datetime('now')); +`; diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 4ac8b315..1076e675 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -83,6 +83,7 @@ import { connectorExecutionsSchema } from "./migrations/081-connector-executions import { connectorPendingAccountSchema } from "./migrations/082-connector-pending-account"; import { connectorAccountLabelsSchema } from "./migrations/083-connector-account-labels"; import { connectorGrantLimitsSchema } from "./migrations/084-connector-grant-limits"; +import { approvalSubjectSnapshotSchema } from "./migrations/085-approval-subject-snapshot"; interface Migration { version: number; @@ -175,6 +176,7 @@ const MIGRATIONS: Migration[] = [ { version: 82, name: "connector_pending_account", sql: connectorPendingAccountSchema }, { version: 83, name: "connector_account_labels", sql: connectorAccountLabelsSchema }, { version: 84, name: "connector_grant_limits", sql: connectorGrantLimitsSchema }, + { version: 85, name: "approval_subject_snapshot", sql: approvalSubjectSnapshotSchema }, ]; const SCHEMA_VERSION = MIGRATIONS[MIGRATIONS.length - 1].version; diff --git a/src/main/services/organization-service.ts b/src/main/services/organization-service.ts index 15b6a00e..94f33bd7 100644 --- a/src/main/services/organization-service.ts +++ b/src/main/services/organization-service.ts @@ -1,5 +1,5 @@ import type { DatabaseSync } from "node:sqlite"; -import { APPROVAL_STATE, RISK_LEVEL, TASK_STATUS, WRITING_MODE } from "../../shared/domain"; +import { APPROVAL_STATE, RISK_LEVEL, TASK_STATUS, WRITING_MODE, type RiskLevel } from "../../shared/domain"; import { createId } from "../../shared/ids"; import type { CreateAgentInput, CreateGoalInput, CreateOrganizationInput, CreateProjectInput, CreateTaskInput, CreateWorkspaceInput, DeleteTaskInput, UpdateAgentInput, UpdateEmployeeProfileInput, UpdateTaskInput } from "../../shared/commands"; import { AuditService } from "./audit-service"; @@ -12,6 +12,7 @@ import { TaskMutations } from "./task-mutations"; import { normalizeAvatarColor, normalizeAvatarImage } from "./avatar-values"; import { approvalDecision, autonomy } from "./org-approvals"; import { admitAgent } from "./agent-admission"; +import { TASK_APPROVAL_KINDS_SQL, TASK_SNAPSHOT_SQL } from "./task-approval"; export class OrganizationService { private readonly audit: AuditService; @@ -23,7 +24,7 @@ export class OrganizationService { this.audit = new AuditService(db); this.policy = new PolicyService(db); this.governance = new OrgGovernanceService(db); - this.tasks = new TaskMutations(db, (input) => this.createTask(input)); + this.tasks = new TaskMutations(db, (input) => this.createTask(input), (orgId, taskId) => this.requestTaskApproval(orgId, taskId)); } createOrganization(input: CreateOrganizationInput): string { @@ -262,35 +263,7 @@ export class OrganizationService { now, ); const policy = this.policy.evaluateTaskCreation(input.orgId, input.risk); - const approvalRequired = policy.approvalRequired || input.requiresFilesystem; - if (approvalRequired) { - const approvalId = createId("app"); - const approvalKind = policy.approvalRequired ? "task_risk" : "task_execution"; - const autoApprove = autonomy(this.db, input.orgId).executionMode === "ai_ceo"; - const decision = approvalDecision(this.db, input.orgId, autoApprove, now); - this.db.prepare(` - insert into approvals( - id, org_id, kind, title, details, risk, requested_by_type, - requested_by_id, source_entity_type, source_entity_id, state, decided_by_employee_id, created_at, decided_at - ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - `).run( - approvalId, - input.orgId, - approvalKind, - input.title.trim(), - input.description.trim(), - RISK_LEVEL.HIGH, - "system", - null, - "task", - id, - decision.state, - decision.decidedBy, - now, - decision.decidedAt, - ); - this.settleApproval(input.orgId, approvalId, approvalKind, RISK_LEVEL.HIGH, autoApprove); - } + this.requestTaskApproval(input.orgId, id); this.audit.record({ orgId: input.orgId, entityType: "task", @@ -303,6 +276,37 @@ export class OrganizationService { return id; } + /** Asks for approval of a task's current content when policy or file access needs it and no review already covers that content. */ + requestTaskApproval(orgId: string, taskId: string): void { + const task = this.db.prepare(`select t.title, t.description, t.risk, t.requires_filesystem, ${TASK_SNAPSHOT_SQL} as snapshot from tasks t where t.id = ? and t.org_id = ?`) + .get(taskId, orgId) as { title: string; description: string; risk: RiskLevel; requires_filesystem: number; snapshot: string }; + const policy = this.policy.evaluateTaskCreation(orgId, task.risk); + if (!policy.approvalRequired && task.requires_filesystem !== 1) return; + const latest = this.db.prepare(` + select subject_snapshot from approvals + where org_id = ? and source_entity_type = 'task' and source_entity_id = ? and kind in ${TASK_APPROVAL_KINDS_SQL} + order by rowid desc limit 1 + `).get(orgId, taskId) as { subject_snapshot: string | null } | undefined; + if (latest?.subject_snapshot === task.snapshot) return; + const superseded = this.db.prepare(` + update approvals set state = ? where org_id = ? and source_entity_type = 'task' and source_entity_id = ? and kind in ${TASK_APPROVAL_KINDS_SQL} and state = ? + `).run(APPROVAL_STATE.SUPERSEDED, orgId, taskId, APPROVAL_STATE.PENDING); + if (superseded.changes > 0) console.info(`[approvals] task ${taskId} changed; its pending review was superseded`); + const now = new Date().toISOString(); + const approvalId = createId("app"); + const approvalKind = policy.approvalRequired ? "task_risk" : "task_execution"; + const autoApprove = autonomy(this.db, orgId).executionMode === "ai_ceo"; + const decision = approvalDecision(this.db, orgId, autoApprove, now); + this.db.prepare(` + insert into approvals( + id, org_id, kind, title, details, risk, requested_by_type, requested_by_id, source_entity_type, source_entity_id, + state, decided_by_employee_id, created_at, decided_at, subject_snapshot + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `).run(approvalId, orgId, approvalKind, task.title, task.description, RISK_LEVEL.HIGH, "system", null, "task", taskId, + decision.state, decision.decidedBy, now, decision.decidedAt, task.snapshot); + this.settleApproval(orgId, approvalId, approvalKind, RISK_LEVEL.HIGH, autoApprove); + } + /** Routes a pending approval to its decider, or records the auto-approval in the audit trail. */ private settleApproval(orgId: string, approvalId: string, kind: string, risk: typeof RISK_LEVEL[keyof typeof RISK_LEVEL], autoApproved: boolean): void { if (!autoApproved) { diff --git a/src/main/services/task-approval.ts b/src/main/services/task-approval.ts new file mode 100644 index 00000000..2542436d --- /dev/null +++ b/src/main/services/task-approval.ts @@ -0,0 +1,4 @@ +/** The task content an approval covers, as SQL over a `tasks t` row; one expression for recording and checking keeps them comparable. */ +export const TASK_SNAPSHOT_SQL = "json_object('title', t.title, 'description', t.description, 'assignee', t.assignee_agent_id, 'risk', t.risk, 'workspace', t.workspace_id, 'filesystem', t.requires_filesystem)"; + +export const TASK_APPROVAL_KINDS_SQL = "('task_risk', 'task_execution')"; diff --git a/src/main/services/task-mutations.ts b/src/main/services/task-mutations.ts index a211a1eb..7d2c6b8c 100644 --- a/src/main/services/task-mutations.ts +++ b/src/main/services/task-mutations.ts @@ -23,7 +23,12 @@ interface TaskRow { export class TaskMutations { private readonly audit: AuditService; - constructor(private readonly db: DatabaseSync, private readonly createTask: (input: CreateTaskInput) => string) { + constructor( + private readonly db: DatabaseSync, + private readonly createTask: (input: CreateTaskInput) => string, + /** Asks for a fresh approval when an edit changed content an approval covered. */ + private readonly requestApproval: (orgId: string, taskId: string) => void, + ) { this.audit = new AuditService(db); } @@ -48,6 +53,7 @@ export class TaskMutations { where id = ? and org_id = ? `).run(input.title.trim(), input.description.trim(), input.assigneeAgentId, input.risk, input.priority, input.parentTaskId, now, input.taskId, input.orgId); if (result.changes === 0) throw new Error("Task was not found."); + this.requestApproval(input.orgId, input.taskId); this.audit.record({ orgId: input.orgId, entityType: "task", diff --git a/src/main/services/task-run-preparation.ts b/src/main/services/task-run-preparation.ts index 8b7facbe..71d0e597 100644 --- a/src/main/services/task-run-preparation.ts +++ b/src/main/services/task-run-preparation.ts @@ -1,3 +1,4 @@ +import { TASK_APPROVAL_KINDS_SQL, TASK_SNAPSHOT_SQL } from "./task-approval"; import type { DatabaseSync } from "node:sqlite"; import { APPROVAL_STATE, RISK_LEVEL } from "../../shared/domain"; import { createId } from "../../shared/ids"; @@ -11,11 +12,6 @@ import type { WorkspaceRow, } from "./task-prompts"; -interface TaskApprovalRow { - id: string; - state: string; -} - export function requireTask(db: DatabaseSync, orgId: string, taskId: string): TaskExecutionTaskRow { const task = db.prepare("select * from tasks where id = ? and org_id = ?").get(taskId, orgId) as TaskExecutionTaskRow | undefined; if (!task) throw new Error("Task was not found."); @@ -45,16 +41,14 @@ export function assertExecutionApproval(db: DatabaseSync, orgId: string, task: T export function executionApprovalProblem(db: DatabaseSync, orgId: string, task: TaskExecutionTaskRow): string | null { if (task.risk !== RISK_LEVEL.HIGH && task.requires_filesystem !== 1) return null; const approval = db.prepare(` - select id, state - from approvals - where org_id = ? - and source_entity_type = 'task' - and source_entity_id = ? - and kind in ('task_risk', 'task_execution') - order by created_at desc + select a.state, a.subject_snapshot = (select ${TASK_SNAPSHOT_SQL} from tasks t where t.id = a.source_entity_id) as covers_current + from approvals a + where a.org_id = ? and a.source_entity_type = 'task' and a.source_entity_id = ? and a.kind in ${TASK_APPROVAL_KINDS_SQL} + order by a.rowid desc limit 1 - `).get(orgId, task.id) as TaskApprovalRow | undefined; + `).get(orgId, task.id) as { state: string; covers_current: number | null } | undefined; if (!approval || approval.state !== APPROVAL_STATE.APPROVED) return "Approved task execution approval is required."; + if (approval.covers_current !== 1) return "The task changed after it was approved; the changed task needs a fresh approval."; return null; } diff --git a/src/renderer/src/i18n/zh.ts b/src/renderer/src/i18n/zh.ts index fb4619aa..7d2c0f9e 100644 --- a/src/renderer/src/i18n/zh.ts +++ b/src/renderer/src/i18n/zh.ts @@ -387,6 +387,7 @@ export const zh: Record = { "Account label, e.g. sales": "账号标签，例如 sales", "Add account": "添加账号", "limited": "受限", + "Replaced by an edit": "已因编辑被替换", "Revoke {name}": "撤销 {name}", "Grant {name} to": "把 {name} 授予", "Grant to…": "授予给…", diff --git a/src/renderer/src/pages/ApprovalsPage.tsx b/src/renderer/src/pages/ApprovalsPage.tsx index e4f2dae8..920e504e 100644 --- a/src/renderer/src/pages/ApprovalsPage.tsx +++ b/src/renderer/src/pages/ApprovalsPage.tsx @@ -276,7 +276,8 @@ function DecidedRow({ last: boolean; }) { const approved = approval.state === APPROVAL_STATE.APPROVED; - const decidedLabel = approved ? t("Approved") : t("Declined"); + const superseded = approval.state === APPROVAL_STATE.SUPERSEDED; + const decidedLabel = approved ? t("Approved") : superseded ? t("Replaced by an edit") : t("Declined"); return (
- +
{approval.title}
- {decidedLabel} · {formatRelative(approval.decidedAt ?? approval.createdAt)} · {decider.split(" ")[0]} + {decidedLabel} · {formatRelative(approval.decidedAt ?? approval.createdAt)}{superseded ? "" : ` · ${decider.split(" ")[0]}`}
); diff --git a/src/shared/domain.ts b/src/shared/domain.ts index 5ff04b54..40ea0c37 100644 --- a/src/shared/domain.ts +++ b/src/shared/domain.ts @@ -16,7 +16,8 @@ export { SEARCH_ENTITY_TYPES, type SearchEntityType, type SearchResultRecord } f export type { BrowserActionPolicyRecord, BrowserActionRecord, BrowserSessionContainerRecord, SocialAccountGrantRecord, SocialAccountRecord, SocialApprovalRouteRecord, SocialAuditRecord } from "./social-domain"; export type { AppSnapshot, UiLanguage, UiPreferences, UiTheme } from "./snapshot-domain"; export const TASK_STATUS = { BACKLOG: "backlog", IN_PROGRESS: "in_progress", REVIEW: "review", DONE: "done" } as const; -export const APPROVAL_STATE = { PENDING: "pending", APPROVED: "approved", DECLINED: "declined" } as const; +/** Superseded: a pending review replaced by one for newer content. */ +export const APPROVAL_STATE = { PENDING: "pending", APPROVED: "approved", DECLINED: "declined", SUPERSEDED: "superseded" } as const; export const RISK_LEVEL = { LOW: "low", MEDIUM: "medium", HIGH: "high" } as const; export const TASK_PRIORITY = { LOW: "low", NORMAL: "normal", HIGH: "high" } as const; /** How a task's output should read: conversational prose carries house style; technical and structured do not. */ diff --git a/tests/unit/domain-services.test.ts b/tests/unit/domain-services.test.ts index 0ff8d667..427a143b 100644 --- a/tests/unit/domain-services.test.ts +++ b/tests/unit/domain-services.test.ts @@ -24,7 +24,7 @@ describe("domain services", () => { const connection = createConnection(); const rows = connection.db.prepare("select version from schema_migrations order by version asc").all() as Array<{ version: number }>; - expect(rows.map((row) => row.version)).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84]); + expect(rows.map((row) => row.version)).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85]); connection.close(); }); diff --git a/tests/unit/execution-modes.test.ts b/tests/unit/execution-modes.test.ts index b0963445..ab308719 100644 --- a/tests/unit/execution-modes.test.ts +++ b/tests/unit/execution-modes.test.ts @@ -8,7 +8,8 @@ import { OrganizationService } from "../../src/main/services/organization-servic import { ChatService } from "../../src/main/services/chat-service"; import { dispatchAgentTool, type AgentToolContext } from "../../src/main/services/agent-tools"; import { ApprovalService } from "../../src/main/services/approval-service"; -import { requireAssignedAgent } from "../../src/main/services/task-run-preparation"; +import { executionApprovalProblem, requireAssignedAgent } from "../../src/main/services/task-run-preparation"; +import { TASK_PRIORITY } from "../../src/shared/domain"; import type { TaskExecutionTaskRow } from "../../src/main/services/task-prompts"; function call(name: string, args: Record) { @@ -153,4 +154,40 @@ describe("execution modes", () => { expect([lifecycle(bo), identities(bo), memberships(bo)]).toEqual(["declined", 0, 0]); expect(() => runAs(bo)).toThrow("Bo's hire was declined; reassign the task."); }); + + test("a task approval covers the content it was given; an edit needs a fresh one", () => { + const h = harness("ai_cofounder"); + const db = h.connection.db; + const employeeId = (db.prepare("select id from employees where org_id = ?").get(h.orgId) as { id: string }).id; + const approvals = new ApprovalService(db); + const taskId = h.connection.transaction(() => h.deps.organization.createTask({ + orgId: h.orgId, title: "Wipe staging", description: "Drop the staging tables.", status: "backlog", + assigneeAgentId: h.ceoId, risk: "high", projectId: null, goalId: null, workspaceId: null, priority: TASK_PRIORITY.NORMAL, requiresFilesystem: false, + })); + const task = () => db.prepare("select * from tasks where id = ?").get(taskId) as unknown as Parameters[2]; + const taskApprovals = () => db.prepare("select id, state, details from approvals where source_entity_id = ? order by rowid").all(taskId) as Array<{ id: string; state: string; details: string }>; + const edit = (description: string) => h.connection.transaction(() => h.deps.organization.updateTask({ + orgId: h.orgId, taskId, title: "Wipe staging", description, assigneeAgentId: h.ceoId, risk: "high", priority: TASK_PRIORITY.NORMAL, parentTaskId: null, + })); + const decide = (id: string) => h.connection.transaction(() => approvals.decide({ orgId: h.orgId, approvalId: id, state: "approved", decidedByEmployeeId: employeeId })); + + // Editing while the review waits replaces it with one for the new content. + edit("Drop the staging tables, then reseed."); + expect(taskApprovals().map((row) => [row.state, row.details])).toEqual([ + ["superseded", "Drop the staging tables."], + ["pending", "Drop the staging tables, then reseed."], + ]); + decide(taskApprovals()[1].id); + expect(executionApprovalProblem(db, h.orgId, task())).toBeNull(); + + // An approved task edited afterwards is not covered by the old approval. + edit("Drop the production tables."); + expect(executionApprovalProblem(db, h.orgId, task())).toBe("Approved task execution approval is required."); + expect(taskApprovals().map((row) => row.state)).toEqual(["superseded", "approved", "pending"]); + // A save that changes nothing approved keeps the approval. + decide(taskApprovals()[2].id); + edit("Drop the production tables."); + expect(taskApprovals()).toHaveLength(3); + expect(executionApprovalProblem(db, h.orgId, task())).toBeNull(); + }); }); diff --git a/tests/unit/task-review.test.ts b/tests/unit/task-review.test.ts index e9e7931c..60bfa41f 100644 --- a/tests/unit/task-review.test.ts +++ b/tests/unit/task-review.test.ts @@ -67,7 +67,7 @@ describe("task review", () => { test("a failed review hand-off leaves the task where it was; deleting reviewed work closes its review", () => { const h = setup(); const taskId = h.task(h.devId, "Login page"); - const failing = new TaskMutations(h.connection.db, () => { throw new Error("review task could not be created"); }); + const failing = new TaskMutations(h.connection.db, () => { throw new Error("review task could not be created"); }, () => undefined); expect(() => failing.updateStatus(h.orgId, taskId, TASK_STATUS.REVIEW)).toThrow("review task could not be created"); expect(h.connection.db.prepare("select status from tasks where id = ?").get(taskId)).toEqual({ status: "in_progress" });