feat: handler unit tests + stale test cleanup (#54)

[dean0x]

Summary

• Add 21 new unit tests for 3 previously untested handlers: PersistenceHandler (8), QueueHandler (9), OutputHandler (4)
• Remove 3 stale it.skip tests for unimplemented threshold events (ResourceThresholdCrossed/ResourceThresholdRecovered) — no type definition, producer, or consumer exists in the codebase
• Update test:handlers script to include the 3 new test files

Completes Phase 2 of Issue #54. Phase 1 (18 edge-case tests for worker pool, resource monitor, agent adapters) was merged in 738e377.

Test plan

• npm run build — clean compile
• npm run test:handlers — 105 tests pass (84 existing + 21 new)
• npm run test:implementations — no regressions from skipped test removal
• npm run test:services — no regressions
• npx biome check — lint clean on all modified files

[greptile-apps]

Confidence Score: 5/5

• This PR is safe to merge — it adds and fixes tests only, with no production code changes.
• All changes are test-only. The new tests use real SQLite databases in temp directories with proper cleanup, fake timers with try/finally env restoration, and follow established patterns in the codebase. The single identified gap (missing duration assertion in TaskCompleted) is a test coverage omission, not a functional issue. No regressions are introduced.
• tests/unit/services/handlers/persistence-handler.test.ts — the TaskCompleted test is missing a duration assertion despite the test name and handler both referencing it.

Important Files Changed

Filename	Overview
tests/unit/services/handlers/persistence-handler.test.ts	New file with 8 tests covering all 6 handled events (TaskDelegated, TaskStarted, TaskCompleted, TaskFailed, TaskCancelled, TaskTimeout) plus error paths. One minor gap: the TaskCompleted test name promises to verify duration persistence but the assertion is missing.
tests/unit/services/handlers/queue-handler.test.ts	New file with 9 tests covering TaskPersisted (dependency blocking), TaskCancellationRequested, NextTaskQuery, RequeueTask, and TaskUnblocked (including the P0/P2 DB-vs-payload scenario to verify fresh DB fetch). Well structured with real SQLite backing and thorough assertions.
tests/unit/services/handlers/output-handler.test.ts	New file with 4 tests for LogsRequested and OutputCaptured events. Both stdout and stderr line counts are asserted, tail parameter forwarding is verified, and graceful handling of missing task output is covered.
tests/unit/implementations/event-driven-worker-pool.test.ts	Adds 5 new tests: WORKER_SPAWN_FAILED on missing agent, kill when process is already dead, killAll on empty pool, timeout-after-completion no-op, and warning on completion-for-unknown-worker. New tests include explicit spawn assertions before guard clauses, addressing previous review feedback.
tests/unit/implementations/system-resource-monitor.test.ts	Converts 5 it.skip stubs to real tests (interval emission, idempotent start, safe stop, no-eventBus guard, error recovery). Rewrites 3 settling-worker tests with proper env isolation (try/finally). Adds getThresholds test for CPU threshold calculation.
tests/unit/implementations/agent-adapters.test.ts	Adds 7 new tests: 5 for BaseAgentAdapter kill lifecycle (SIGTERM, SIGKILL escalation, dispose-clears-timeout, deduplicated timeout on double-kill, PROCESS_KILL_FAILED error) and 2 for GeminiAdapter GEMINI_SANDBOX env override behaviour.
package.json	Adds the 3 new handler test files to the test:handlers script. Straightforward update.

Sequence Diagram

sequenceDiagram
    participant Test
    participant InMemoryEventBus
    participant PersistenceHandler
    participant QueueHandler
    participant OutputHandler
    participant SQLiteTaskRepository
    participant PriorityTaskQueue

    Note over Test,PriorityTaskQueue: PersistenceHandler → QueueHandler flow
    Test->>InMemoryEventBus: emit('TaskDelegated', {task})
    InMemoryEventBus->>PersistenceHandler: handleTaskDelegated
    PersistenceHandler->>SQLiteTaskRepository: save(task)
    PersistenceHandler->>InMemoryEventBus: emit('TaskPersisted', {taskId, task})
    InMemoryEventBus->>QueueHandler: handleTaskPersisted
    QueueHandler->>SQLiteTaskRepository: dependencyRepo.isBlocked(taskId)
    alt not blocked
        QueueHandler->>PriorityTaskQueue: enqueue(task)
        QueueHandler->>InMemoryEventBus: emit('TaskQueued', {taskId})
    else blocked
        QueueHandler-->>InMemoryEventBus: (no enqueue)
    end

    Note over Test,PriorityTaskQueue: TaskUnblocked fresh-DB-fetch flow
    Test->>InMemoryEventBus: emit('TaskUnblocked', {taskId, task[P2]})
    InMemoryEventBus->>QueueHandler: handleTaskUnblocked
    QueueHandler->>SQLiteTaskRepository: findById(taskId) → task[P0]
    QueueHandler->>PriorityTaskQueue: enqueue(task[P0])
    QueueHandler->>InMemoryEventBus: emit('TaskQueued', {taskId})

    Note over Test,PriorityTaskQueue: OutputHandler LogsRequested flow
    Test->>InMemoryEventBus: emit('LogsRequested', {taskId, tail})
    InMemoryEventBus->>OutputHandler: handleLogsRequested
    OutputHandler->>OutputHandler: outputCapture.getOutput(taskId, tail)
    OutputHandler-->>Test: logger.debug('Task logs retrieved', {stdoutLines, stderrLines})

Loading

_{Last reviewed commit: 2eba8fd}

[greptile-apps]

Test name promises duration assertion that never appears

The test is named "should update task to COMPLETED with exitCode and duration" and emits the event with duration: 5000, but no assertion is made on the persisted duration field. The handler at persistence-handler.ts:124 explicitly persists duration: event.duration, so a regression there would go undetected by this test.

Since Task.duration is an optional field in the domain (domain.ts:110), it can be read back from the repository. Add an assertion to match the test's stated intent:

Suggested change

	it('should update task to COMPLETED with exitCode and duration', async () => {
	const task = createTask({ prompt: 'test task' });
	await taskRepo.save(task);
	await eventBus.emit('TaskCompleted', { taskId: task.id, exitCode: 0, duration: 5000 });
	await flushEventLoop();
	const findResult = await taskRepo.findById(task.id);
	expect(findResult.ok).toBe(true);
	expect(findResult.value!.status).toBe(TaskStatus.COMPLETED);
	expect(findResult.value!.exitCode).toBe(0);
	expect(findResult.value!.completedAt).toBeDefined();
	});
	it('should update task to COMPLETED with exitCode and duration', async () => {
	const task = createTask({ prompt: 'test task' });
	await taskRepo.save(task);
	await eventBus.emit('TaskCompleted', { taskId: task.id, exitCode: 0, duration: 5000 });
	await flushEventLoop();
	const findResult = await taskRepo.findById(task.id);
	expect(findResult.ok).toBe(true);
	expect(findResult.value!.status).toBe(TaskStatus.COMPLETED);
	expect(findResult.value!.exitCode).toBe(0);
	expect(findResult.value!.completedAt).toBeDefined();
	expect(findResult.value!.duration).toBe(5000);
	});