fix(tasks): cover syncProject wiring and allow deleting orphans after file removal

Review follow-up: - runtime.test.js: add syncProject wiring tests with a real temp-dir project and real project-config runtime — asserts reconcileLoopTasks is driven with the discovered loops when the project path is known (task created, nextRunAt computed) and that plain listing is used when the path cannot be resolved (reconcile not called). - service.js: DELETE on a loop-owned task is rejected with a 400 only while its loop file still exists on disk; once the file is gone the orphan task can be deleted directly instead of waiting for the next reconcile. Tests use real temp files for both branches. - DOCUMENTATION.md: delete semantics updated accordingly. - PR description refreshed for the final HEAD (test counts, reconciliation contract, evidence wording).
2026-08-06 09:49:31 +03:00
parent 59a6c1b70d
commit 9b6b90504c
4 changed files with 156 additions and 21 deletions
@@ -96,10 +96,11 @@ project write lock on every `syncProject` when the project path is known:
 - **UI edits** to a loop-sourced task are preserved in the config but the loop
  file remains authoritative: the next reconciliation re-applies the file's
  definition (including `enabled`). Use `enabled: false` in the file to
-  disable. Deleting a loop-sourced task through the API is rejected with a 400 —
-  the loop file is the removal surface. The scheduled-tasks UI marks loop tasks
-  as file-managed and disables their edit/enable/delete actions for the same
-  reason; `run now` remains available.
+  disable. Deleting a loop-sourced task through the API is rejected with a 400
+  while its loop file still exists on disk — the loop file is the removal
+  surface; once the file is gone, deleting the orphan task is allowed. The
+  scheduled-tasks UI marks loop tasks as file-managed and disables their
+  edit/enable/delete actions for the same reason; `run now` remains available.

 ## Public exports (runtime.js)

@@ -1,5 +1,15 @@
-import { describe, expect, it } from 'vitest';
-import { computeNextRunAt, expandCommandGoalObjective, formatScheduledSessionTitle, parseScheduledCommandPrompt } from './runtime.js';
+import { describe, expect, it, vi } from 'vitest';
+import os from 'os';
+import path from 'path';
+import { mkdtemp, rm, mkdir, writeFile } from 'fs/promises';
+import {
+  computeNextRunAt,
+  expandCommandGoalObjective,
+  formatScheduledSessionTitle,
+  parseScheduledCommandPrompt,
+  createScheduledTasksRuntime,
+} from './runtime.js';
+import { createProjectConfigRuntime } from '../projects/project-config.js';

 describe('scheduled-tasks runtime helpers', () => {
  it('computes next daily run in timezone', () => {
@@ -109,3 +119,90 @@ describe('scheduled-tasks runtime helpers', () => {
      .toBe('Review the requested scope.\n\nauth module');
  });
 });
+
+describe('scheduled-tasks runtime syncProject wiring', () => {
+  const createTempProject = async () => {
+    const tempRoot = await mkdtemp(path.join(os.tmpdir(), 'oc-runtime-loop-'));
+    const repoPath = path.join(tempRoot, 'repo');
+    await mkdir(path.join(repoPath, '.agents', 'loops'), { recursive: true });
+    return {
+      tempRoot,
+      repoPath,
+      cleanup: async () => {
+        await rm(tempRoot, { recursive: true, force: true });
+      },
+    };
+  };
+
+  const createProjectConfig = async (tempRoot) => createProjectConfigRuntime({
+    fsPromises: await import('fs/promises'),
+    path,
+    projectsDirPath: path.join(tempRoot, 'config'),
+    createTaskID: () => 'task-fixed-id',
+  });
+
+  const createRuntimeDeps = (overrides = {}) => ({
+    buildOpenCodeUrl: () => 'http://localhost',
+    getOpenCodeAuthHeaders: () => ({}),
+    waitForOpenCodeReady: async () => {},
+    ...overrides,
+  });
+
+  it('reconciles discovered loops when the project path is known', async () => {
+    const { tempRoot, repoPath, cleanup } = await createTempProject();
+    try {
+      await writeFile(path.join(repoPath, '.agents', 'loops', 'daily.md'), `---
+name: daily
+schedule: "0 9 * * *"
+enabled: true
+model: openai/gpt-5
+---
+Run daily.
+`, 'utf8');
+
+      const projectConfigRuntime = await createProjectConfig(tempRoot);
+      const runtime = createScheduledTasksRuntime({
+        ...createRuntimeDeps(),
+        projectConfigRuntime,
+        listProjects: async () => [{ id: 'proj', path: repoPath }],
+      });
+
+      await runtime.syncProject('proj');
+
+      const tasks = await projectConfigRuntime.listScheduledTasks('proj');
+      expect(tasks).toHaveLength(1);
+      expect(tasks[0].id).toBe('loop:project:daily');
+      expect(tasks[0].loopFile).toBe(path.join(repoPath, '.agents', 'loops', 'daily.md'));
+      // syncTaskSchedule computed and persisted the next run for the enabled task.
+      expect(tasks[0].state.nextRunAt).toBeGreaterThan(0);
+    } finally {
+      await cleanup();
+    }
+  });
+
+  it('falls back to plain listing when the project path cannot be resolved', async () => {
+    const { tempRoot, cleanup } = await createTempProject();
+    try {
+      const projectConfigRuntime = await createProjectConfig(tempRoot);
+      const reconcileSpy = vi.spyOn(projectConfigRuntime, 'reconcileLoopTasks');
+      const listSpy = vi.spyOn(projectConfigRuntime, 'listScheduledTasks');
+
+      const runtime = createScheduledTasksRuntime({
+        ...createRuntimeDeps(),
+        projectConfigRuntime,
+        // Project not registered -> ensureProjectPath cannot resolve a path.
+        listProjects: async () => [],
+      });
+
+      await runtime.syncProject('proj');
+
+      expect(reconcileSpy).not.toHaveBeenCalled();
+      expect(listSpy).toHaveBeenCalledWith('proj');
+      expect(await projectConfigRuntime.listScheduledTasks('proj')).toEqual([]);
+      reconcileSpy.mockRestore();
+      listSpy.mockRestore();
+    } finally {
+      await cleanup();
+    }
+  });
+});
@@ -1,3 +1,4 @@
+import fs from 'node:fs';
 import path from 'node:path';
 import { OpenChamberControlError } from '../openchamber-control/error.js';

@@ -80,10 +81,12 @@ export const createScheduledTaskService = (dependencies) => {
    if (!normalizedTaskID) throw new OpenChamberControlError('taskId is required', 400);
    const current = await projectConfigRuntime.listScheduledTasks(projectID);
    const existing = current.find((task) => task.id === normalizedTaskID) || null;
-    if (existing?.loopFile) {
+    if (existing?.loopFile && fs.existsSync(existing.loopFile)) {
      // Loop tasks are owned by their `.agents/loops` markdown file: deleting
-      // the JSON row would be silently undone by the next reconcile. The file
-      // itself is the removal surface.
+      // the JSON row would be silently undone by the next reconcile while the
+      // file exists. The file itself is the removal surface. Once the file is
+      // gone (the task is an orphan that the next sync would remove anyway),
+      // deleting the row is safe and allowed.
      throw new OpenChamberControlError(
        'Loop task is managed by its .agents/loops markdown file; delete the file to remove the task',
        400,
@@ -1,4 +1,7 @@
 import { describe, expect, it, vi } from 'vitest';
+import os from 'os';
+import path from 'path';
+import { mkdtemp, rm, writeFile } from 'fs/promises';
 import { createScheduledTaskService } from './service.js';

 const createService = (overrides = {}) => {
@@ -32,19 +35,50 @@ const loopTask = {
 };

 describe('scheduled-task service remove', () => {
-  it('rejects deleting a loop-sourced task without touching storage', async () => {
-    const { service, projectConfigRuntime, scheduledTasksRuntime } = createService({
-      projectConfigRuntime: {
-        listScheduledTasks: vi.fn(async () => [loopTask]),
-      },
-    });
+  it('rejects deleting a loop-sourced task while its loop file still exists', async () => {
+    const tempRoot = await mkdtemp(path.join(os.tmpdir(), 'oc-loop-delete-'));
+    try {
+      const loopFilePath = path.join(tempRoot, 'daily.md');
+      await writeFile(loopFilePath, '---\nname: daily-digest\n---\nRun.\n', 'utf8');

-    await expect(service.remove('project-test', loopTask.id)).rejects.toMatchObject({
-      statusCode: 400,
-      message: expect.stringContaining('delete the file to remove the task'),
-    });
-    expect(projectConfigRuntime.deleteScheduledTask).not.toHaveBeenCalled();
-    expect(scheduledTasksRuntime.syncProject).not.toHaveBeenCalled();
+      const { service, projectConfigRuntime, scheduledTasksRuntime } = createService({
+        projectConfigRuntime: {
+          listScheduledTasks: vi.fn(async () => [{ ...loopTask, loopFile: loopFilePath }]),
+        },
+      });
+
+      await expect(service.remove('project-test', loopTask.id)).rejects.toMatchObject({
+        statusCode: 400,
+        message: expect.stringContaining('delete the file to remove the task'),
+      });
+      expect(projectConfigRuntime.deleteScheduledTask).not.toHaveBeenCalled();
+      expect(scheduledTasksRuntime.syncProject).not.toHaveBeenCalled();
+    } finally {
+      await rm(tempRoot, { recursive: true, force: true });
+    }
+  });
+
+  it('allows deleting a loop-sourced task once its loop file is gone', async () => {
+    const tempRoot = await mkdtemp(path.join(os.tmpdir(), 'oc-loop-delete-'));
+    try {
+      // The loop file was removed from disk; the orphan task is allowed to be
+      // deleted directly instead of waiting for the next reconcile.
+      const loopFilePath = path.join(tempRoot, 'gone.md');
+
+      const { service, projectConfigRuntime, scheduledTasksRuntime } = createService({
+        projectConfigRuntime: {
+          listScheduledTasks: vi.fn(async () => [{ ...loopTask, loopFile: loopFilePath }]),
+        },
+      });
+
+      const tasks = await service.remove('project-test', loopTask.id);
+
+      expect(projectConfigRuntime.deleteScheduledTask).toHaveBeenCalledWith('project-test', loopTask.id);
+      expect(scheduledTasksRuntime.syncProject).toHaveBeenCalled();
+      expect(Array.isArray(tasks)).toBe(true);
+    } finally {
+      await rm(tempRoot, { recursive: true, force: true });
+    }
  });

  it('deletes JSON-configured tasks normally', async () => {