refactor(schema): make runs.task_id nullable, delete heartbeat workaround
The NOT NULL constraint forced heartbeat runs to synthesize a fake task row (`[Heartbeat] X — Check for work`) so the FK could resolve. Three layers of code — acquireHeartbeatTask, findReusableHeartbeatTask, cleanupDuplicateHeartbeatTasks, plus the is_heartbeat flag column + UI filter — existed to paper over one bad constraint. Remove the constraint and the whole workaround vanishes. SQLite table rebuild with data preservation. Live production DB, no backup — migration is atomic, idempotent, and verified before DROP. ## Migration v35 (rebuild runs with nullable task_id) Uses `fn:` form so it can: - Snapshot `COUNT(*) FROM runs` before copy - Walk `PRAGMA table_info(runs)` to detect drift columns (v34 declared a column set; any additive ALTER since v14 shows up here) - Assert `COUNT(*) FROM runs_new == beforeCount` before `DROP TABLE runs` - `PRAGMA defer_foreign_keys = ON` so run_events(run_id) FK defers to commit time, by which runs has the same id set under the new name - Recreate all 6 indexes on the new table If the assertion fails, the migration throws inside the existing `transaction()` wrapper. SQLite rolls back the whole migration, old `runs` table is untouched, `schema_migrations` never records v35. Next app start retries cleanly. ## Migration v36 (drop tasks.is_heartbeat + synthetic rows) Runs now carry `task_id = NULL` for heartbeat dispatches. The `is_heartbeat` flag is dead data going forward. 1. `UPDATE runs SET task_id = NULL WHERE task_id IN (SELECT id FROM tasks WHERE is_heartbeat = 1)` 2. `DELETE FROM tasks WHERE is_heartbeat = 1` (synthetic rows, not real work) 3. Rebuild `tasks` without the `is_heartbeat` column (same defer-FK + row-count-assert pattern as v35) ## Code cleanup - `RunRecord.taskId: string | null`; `createRun` / `cancelRunForDeletion` / `finishRun` guard every `UPDATE tasks` path with a null-check. - `TaskRecord.isHeartbeat` removed; every SELECT / filter / UI reference dropped. - Delete `acquireHeartbeatTask` / `findReusableHeartbeatTask` / `cleanupDuplicateHeartbeatTasks` — dead after the constraint lifts. - Delete `src/main/orchestration/heartbeat-task.ts` (pure workaround constant file). - `heartbeat.ts` creates runs with `taskId: null` when the agent has no assigned work. Turn context already supports nullable task (heartbeatFallbackSection); prompt surface unchanged. - `validateTaskCreation` drops the `isHeartbeat` parameter — user- authored + automation-authored tasks are never heartbeat. - Renderer: `run.taskId ? taskMap.get(...) : undefined` in Dashboard / RunCenter / RunDetail / RunQueue. - Delete `tests/unit/heartbeat-coalescing.test.ts` (12 tests that exercised the deleted DB methods). - Drop 1 isHeartbeat-bypass test from `task-quality-gates.test.ts`. ## Grep verification - `isHeartbeatTaskTitle src/` → 0 - `acquireHeartbeatTask src/` → migration comment only - `is_heartbeat src/` → schema.ts migration history only - `isHeartbeat src/` → 0 Tests: 578/578 (was 591 — delta is 13 tests that tested deleted code).
This commit is contained in:
ZacharyZhang-NY committed
1 parent
92795f7bfb
commit
7e1d4199ee
44 files changed
+385
-674
No files matched your search
@@ -1,317 +0,0 @@
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { mkdtemp, rm } from "node:fs/promises";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("electron", () => ({
|
||||
safeStorage: {
|
||||
isEncryptionAvailable: () => false,
|
||||
getSelectedStorageBackend: () => "basic_text",
|
||||
encryptString: (value: string) => Buffer.from(value, "utf8"),
|
||||
decryptString: (value: Buffer) => value.toString("utf8"),
|
||||
},
|
||||
}));
|
||||
|
||||
import { AppDatabase } from "../../src/main/database";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Helpers
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
function createTestCompany(db: AppDatabase): string {
|
||||
return db.saveCompany({
|
||||
name: "Test Company",
|
||||
description: "Heartbeat coalescing tests",
|
||||
status: "active",
|
||||
});
|
||||
}
|
||||
|
||||
function createTestConnector(db: AppDatabase): void {
|
||||
db.saveConnector({
|
||||
id: "openai-codex",
|
||||
command: "codex",
|
||||
model: "gpt-5",
|
||||
envBindingText: "",
|
||||
notes: "Test connector",
|
||||
});
|
||||
}
|
||||
|
||||
function createTestWorkspace(db: AppDatabase, companyId: string): string {
|
||||
return db.saveWorkspace({
|
||||
companyId,
|
||||
name: "Test Workspace",
|
||||
localPath: "/tmp/test-workspace",
|
||||
repoUrl: "https://github.com/example/test",
|
||||
repoRef: "main",
|
||||
isPrimary: true,
|
||||
});
|
||||
}
|
||||
|
||||
function createTestAgent(db: AppDatabase, companyId: string, workspaceId: string, name = "Agent"): string {
|
||||
return db.saveAgent({
|
||||
companyId,
|
||||
name,
|
||||
role: "IC",
|
||||
title: name,
|
||||
status: "active",
|
||||
reportsTo: null,
|
||||
connectorId: "openai-codex",
|
||||
workspaceId,
|
||||
model: null,
|
||||
capabilities: "Executes tasks",
|
||||
budgetMonthlyUsd: 100,
|
||||
});
|
||||
}
|
||||
|
||||
function createHeartbeatTask(
|
||||
db: AppDatabase,
|
||||
companyId: string,
|
||||
agentId: string,
|
||||
status: string,
|
||||
agentName = "Agent",
|
||||
): string {
|
||||
const id = db.saveTask({
|
||||
companyId,
|
||||
title: `[Heartbeat] ${agentName} — Check for work`,
|
||||
description: `Automated heartbeat run for ${agentName}.`,
|
||||
assigneeAgentId: agentId,
|
||||
status,
|
||||
priority: "medium",
|
||||
});
|
||||
// saveTask is a generic user-task write path and does not set the
|
||||
// heartbeat flag. Flip it directly so this fixture matches what
|
||||
// acquireHeartbeatTask would produce at runtime.
|
||||
db.queryAll("UPDATE tasks SET is_heartbeat = 1 WHERE id = ?", id);
|
||||
return id;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("Heartbeat task coalescing", () => {
|
||||
let tempDir: string;
|
||||
let db: AppDatabase;
|
||||
|
||||
beforeEach(async () => {
|
||||
tempDir = await mkdtemp(join(tmpdir(), "agentcompany-heartbeat-"));
|
||||
db = new AppDatabase(tempDir);
|
||||
db.init();
|
||||
createTestConnector(db);
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
db.close();
|
||||
await rm(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// findReusableHeartbeatTask
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
describe("findReusableHeartbeatTask", () => {
|
||||
it("returns the most recent done heartbeat task for the agent", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "CEO");
|
||||
|
||||
const taskId = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.id).toBe(taskId);
|
||||
expect(result!.status).toBe("done");
|
||||
});
|
||||
|
||||
it("returns null when no heartbeat tasks exist for the agent", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Worker");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when all heartbeat tasks are still in_progress", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Worker");
|
||||
|
||||
createHeartbeatTask(db, companyId, agentId, "in_progress", "Worker");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("returns the most recently updated heartbeat task when multiple done ones exist", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "CEO");
|
||||
|
||||
// Create two done heartbeat tasks
|
||||
const _oldTaskId = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
const newTaskId = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
|
||||
// Advance the newer one so its updated_at is more recent
|
||||
db.advanceTaskStatus(newTaskId, "in_review", "Testing");
|
||||
db.advanceTaskStatus(newTaskId, "done", "Testing done");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.id).toBe(newTaskId);
|
||||
});
|
||||
|
||||
it("returns in_review task if no done tasks exist", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Lead");
|
||||
|
||||
const taskId = createHeartbeatTask(db, companyId, agentId, "in_review", "Lead");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.id).toBe(taskId);
|
||||
});
|
||||
|
||||
it("does not return heartbeat tasks from other agents", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agent1 = createTestAgent(db, companyId, wsId, "Agent1");
|
||||
const agent2 = createTestAgent(db, companyId, wsId, "Agent2");
|
||||
|
||||
createHeartbeatTask(db, companyId, agent1, "done", "Agent1");
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agent2, companyId);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("does not return non-heartbeat tasks", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Worker");
|
||||
|
||||
// Create a regular done task (not a heartbeat)
|
||||
db.saveTask({
|
||||
companyId,
|
||||
title: "Regular completed task",
|
||||
description: "Not a heartbeat",
|
||||
assigneeAgentId: agentId,
|
||||
status: "done",
|
||||
priority: "medium",
|
||||
});
|
||||
|
||||
const result = db.findReusableHeartbeatTask(agentId, companyId);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// cleanupDuplicateHeartbeatTasks
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
describe("cleanupDuplicateHeartbeatTasks", () => {
|
||||
it("cancels duplicate heartbeat tasks, keeping only the most recent per agent", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "CEO");
|
||||
|
||||
// Create three done heartbeat tasks
|
||||
const _task1 = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
const _task2 = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
const task3 = createHeartbeatTask(db, companyId, agentId, "done", "CEO");
|
||||
|
||||
// Touch task3 so it becomes the most recent
|
||||
db.advanceTaskStatus(task3, "in_review", "Touch");
|
||||
db.advanceTaskStatus(task3, "done", "Touch done");
|
||||
|
||||
const cleaned = db.cleanupDuplicateHeartbeatTasks(companyId);
|
||||
expect(cleaned).toBe(2);
|
||||
|
||||
// The most recent one should still be done
|
||||
const remaining = db.getTask(task3);
|
||||
expect(remaining.status).toBe("done");
|
||||
|
||||
// The older ones should be cancelled
|
||||
const old1 = db.getTask(_task1);
|
||||
const old2 = db.getTask(_task2);
|
||||
expect(old1.status).toBe("cancelled");
|
||||
expect(old2.status).toBe("cancelled");
|
||||
});
|
||||
|
||||
it("returns the count of cancelled tasks", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Worker");
|
||||
|
||||
createHeartbeatTask(db, companyId, agentId, "done", "Worker");
|
||||
createHeartbeatTask(db, companyId, agentId, "todo", "Worker");
|
||||
const newest = createHeartbeatTask(db, companyId, agentId, "done", "Worker");
|
||||
// Touch newest so it has the latest updated_at
|
||||
db.advanceTaskStatus(newest, "in_review", "Touch");
|
||||
db.advanceTaskStatus(newest, "done", "Touch done");
|
||||
|
||||
const cleaned = db.cleanupDuplicateHeartbeatTasks(companyId);
|
||||
expect(cleaned).toBe(2);
|
||||
});
|
||||
|
||||
it("does not cancel in_progress heartbeat tasks", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Worker");
|
||||
|
||||
const activeTask = createHeartbeatTask(db, companyId, agentId, "in_progress", "Worker");
|
||||
const doneTask = createHeartbeatTask(db, companyId, agentId, "done", "Worker");
|
||||
// Touch doneTask to make it more recent
|
||||
db.advanceTaskStatus(doneTask, "in_review", "Touch");
|
||||
db.advanceTaskStatus(doneTask, "done", "Touch done");
|
||||
|
||||
const cleaned = db.cleanupDuplicateHeartbeatTasks(companyId);
|
||||
// Only 0 cancelled because in_progress cannot be cancelled and doneTask is the most recent
|
||||
expect(cleaned).toBe(0);
|
||||
|
||||
// in_progress task should be untouched
|
||||
const active = db.getTask(activeTask);
|
||||
expect(active.status).toBe("in_progress");
|
||||
});
|
||||
|
||||
it("keeps one heartbeat task per agent when company has multiple agents", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agent1 = createTestAgent(db, companyId, wsId, "Agent1");
|
||||
const agent2 = createTestAgent(db, companyId, wsId, "Agent2");
|
||||
|
||||
// Agent1: 3 heartbeat tasks
|
||||
createHeartbeatTask(db, companyId, agent1, "done", "Agent1");
|
||||
createHeartbeatTask(db, companyId, agent1, "done", "Agent1");
|
||||
const a1newest = createHeartbeatTask(db, companyId, agent1, "done", "Agent1");
|
||||
db.advanceTaskStatus(a1newest, "in_review", "Touch");
|
||||
db.advanceTaskStatus(a1newest, "done", "Touch done");
|
||||
|
||||
// Agent2: 2 heartbeat tasks
|
||||
createHeartbeatTask(db, companyId, agent2, "done", "Agent2");
|
||||
const a2newest = createHeartbeatTask(db, companyId, agent2, "done", "Agent2");
|
||||
db.advanceTaskStatus(a2newest, "in_review", "Touch");
|
||||
db.advanceTaskStatus(a2newest, "done", "Touch done");
|
||||
|
||||
const cleaned = db.cleanupDuplicateHeartbeatTasks(companyId);
|
||||
// 2 old from agent1 + 1 old from agent2 = 3
|
||||
expect(cleaned).toBe(3);
|
||||
|
||||
// Most recent per agent should survive
|
||||
expect(db.getTask(a1newest).status).toBe("done");
|
||||
expect(db.getTask(a2newest).status).toBe("done");
|
||||
});
|
||||
|
||||
it("returns 0 when no duplicate heartbeat tasks exist", () => {
|
||||
const companyId = createTestCompany(db);
|
||||
const wsId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, wsId, "Solo");
|
||||
|
||||
// Only one heartbeat task
|
||||
createHeartbeatTask(db, companyId, agentId, "done", "Solo");
|
||||
|
||||
const cleaned = db.cleanupDuplicateHeartbeatTasks(companyId);
|
||||
expect(cleaned).toBe(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -271,7 +271,6 @@ const fakeTask = (id: string, title = "Task"): TaskRecord => ({
|
||||
sessionJson: null,
|
||||
sessionDisplayId: null,
|
||||
requiresUserReview: false,
|
||||
isHeartbeat: false,
|
||||
startedAt: null,
|
||||
completedAt: null,
|
||||
cancelledAt: null,
|
||||
|
||||
@@ -11,14 +11,14 @@ import type { TaskValidationResult as _TaskValidationResult } from "../../src/ma
|
||||
|
||||
describe("validateTaskCreation", () => {
|
||||
it("rejects description with 5 chars when not heartbeat", () => {
|
||||
const result = validateTaskCreation({ title: "Fix bug", description: "Short" }, false);
|
||||
const result = validateTaskCreation({ title: "Fix bug", description: "Short" });
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.errors.some(e => e.includes("at least 50 characters"))).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects description with exactly 49 chars when not heartbeat", () => {
|
||||
const desc = "A".repeat(49);
|
||||
const result = validateTaskCreation({ title: "Fix bug", description: desc }, false);
|
||||
const result = validateTaskCreation({ title: "Fix bug", description: desc });
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.errors.some(e => e.includes("at least 50 characters"))).toBe(true);
|
||||
});
|
||||
@@ -29,20 +29,13 @@ describe("validateTaskCreation", () => {
|
||||
const padded = desc.slice(0, 50).padEnd(50, ".");
|
||||
expect(padded.length).toBe(50);
|
||||
// Must contain "done when"
|
||||
const result = validateTaskCreation({ title: "Build login page", description: padded }, false);
|
||||
const result = validateTaskCreation({ title: "Build login page", description: padded });
|
||||
expect(result.valid).toBe(true);
|
||||
expect(result.errors).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("always returns valid for heartbeat tasks regardless of description", () => {
|
||||
const result = validateTaskCreation({ title: "[Heartbeat] Check status", description: "Short" }, true);
|
||||
expect(result.valid).toBe(true);
|
||||
expect(result.errors).toHaveLength(0);
|
||||
expect(result.warnings).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("rejects empty description when not heartbeat", () => {
|
||||
const result = validateTaskCreation({ title: "Some task", description: "" }, false);
|
||||
const result = validateTaskCreation({ title: "Some task", description: "" });
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.errors.some(e => e.includes("at least 50 characters"))).toBe(true);
|
||||
});
|
||||
@@ -50,7 +43,7 @@ describe("validateTaskCreation", () => {
|
||||
it("rejects 60-char description without acceptance criteria keywords", () => {
|
||||
const desc = "This is a description that is long enough to pass length check";
|
||||
expect(desc.length).toBeGreaterThanOrEqual(50);
|
||||
const result = validateTaskCreation({ title: "A task", description: desc }, false);
|
||||
const result = validateTaskCreation({ title: "A task", description: desc });
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.errors.some(e => e.toLowerCase().includes("acceptance criteria"))).toBe(true);
|
||||
});
|
||||
@@ -58,13 +51,13 @@ describe("validateTaskCreation", () => {
|
||||
it("returns valid=true when description has both length and acceptance criteria", () => {
|
||||
const desc = "Implement the authentication flow for the login page. Done when: all tests pass and the form submits.";
|
||||
expect(desc.length).toBeGreaterThanOrEqual(50);
|
||||
const result = validateTaskCreation({ title: "Auth flow", description: desc }, false);
|
||||
const result = validateTaskCreation({ title: "Auth flow", description: desc });
|
||||
expect(result.valid).toBe(true);
|
||||
expect(result.errors).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("includes current character count in length error message", () => {
|
||||
const result = validateTaskCreation({ title: "Fix", description: "Short" }, false);
|
||||
const result = validateTaskCreation({ title: "Fix", description: "Short" });
|
||||
expect(result.errors.some(e => e.includes("currently 5"))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user