chore: v0.6.0 tech debt cleanup (#101, #104, #83)

[dean0x]

Summary

Tech debt consolidation for v0.6.0, addressing three architectural and design improvements for maintainability and code organization.

Changes

#101: OutputRepository Interface in Core

• Move OutputRepository interface to core/interfaces.ts for DIP (Dependency Inversion Principle) compliance
• Consolidate all repository interfaces in core layer
• Remove unused Config adapter and getConfig() function from bootstrap

#104: BootstrapMode Enum

• Replace boolean flags in BootstrapOptions with BootstrapMode enum
• Improves clarity and prevents invalid flag combinations
• Cleaner API surface

#83: ScheduleExecutor Transaction Safety

• Wrap ScheduleExecutor missed-run FAIL policy in database transaction
• Ensures atomicity when handling cascading task failures
• Prevents partial state updates

Also Closes

• P2: totalSize not updated after tail-slicing in TaskManager.getOutput #95 (already fixed in PR fix: v0.6.0 correctness bugs (#84, #82, #95) #106)

Testing

• All existing tests pass (597 total)
• Coverage verified for affected components
• No breaking changes to public APIs

Related Issues

• Closes Move OutputRepository interface to core/interfaces.ts #101
• Closes Replace BootstrapOptions boolean flags with mode enum #104
• Closes refactor: wrap ScheduleExecutor handleMissedRun in transaction #83
• Related to P2: totalSize not updated after tail-slicing in TaskManager.getOutput #95 (v0.6.0 correctness fixes)

[greptile-apps]

Confidence Score: 5/5

• This PR is safe to merge — all changes are mechanical refactors or targeted correctness improvements with full test coverage.
• All three goals are well-executed: the interface relocation is purely mechanical, BootstrapMode eliminates an invalid-flag-combination class of bugs with a pure testable function, and the transaction wrapping directly closes a data-integrity gap. No breaking changes to public APIs, all 18 changed files reviewed. The two inline comments are suggestions for future improvement, not blockers.
• src/services/schedule-executor.ts — the SKIP missed-run path has a pre-existing emit-before-persist ordering that is now visually inconsistent with the newly hardened FAIL path.

Important Files Changed

Filename	Overview
src/bootstrap.ts	Introduces BootstrapMode string-union type and deriveModeFlags() to replace ad-hoc boolean flags; removes obsolete Config adapter; wires TransactionRunner into ScheduleExecutor factory. Clean and consistent refactor.
src/services/schedule-executor.ts	Wraps the MissedRunPolicy.FAIL path in a runInTransaction() call for atomicity. The SKIP path still emits ScheduleMissed before updateNextRun, creating a minor ordering inconsistency (pre-existing, not introduced here).
src/core/interfaces.ts	Removes Config interface and adds OutputRepository interface to core, completing DIP compliance for the repository layer; no interface changes, only relocation.
tests/unit/services/schedule-executor.test.ts	Adds database parameter to ScheduleExecutor.create() calls and a new rollback test that sabotages recordExecutionSync mid-transaction and verifies schedule stays ACTIVE with no audit record and no event emitted.
tests/integration/service-initialization.test.ts	Replaces skipResourceMonitoring:true with resourceMonitor:new TestResourceMonitor() and adds deriveModeFlags() unit tests covering all three modes.
tests/integration/task-scheduling.test.ts	Removes redundant skipResourceMonitoring flag and adds database as required third argument to all ScheduleExecutor.create() calls within tests.

Sequence Diagram

sequenceDiagram
    participant SE as ScheduleExecutor
    participant DB as Database (TransactionRunner)
    participant SR as SQLiteScheduleRepository
    participant EB as EventBus

    SE->>SE: tick() — finds due schedule
    SE->>SE: handleMissedRun(schedule, now)
    note over SE: MissedRunPolicy.FAIL

    SE->>DB: runInTransaction(() => { ... })
    activate DB
    DB->>SR: updateSync(id, { status: CANCELLED })
    DB->>SR: recordExecutionSync({ status: 'missed', ... })
    alt recordExecutionSync throws
        SR-->>DB: throws Error
        DB-->>SE: err(BackbeatError) — rolled back
        SE->>SE: log error, break (no event emitted)
    else both ops succeed
        DB-->>SE: ok(undefined)
        deactivate DB
        SE->>EB: emit('ScheduleMissed', { policy: FAIL })
        SE->>SE: log "Schedule failed due to missed run"
    end

Loading

Comments Outside Diff (2)

1. src/services/schedule-executor.ts, line 363-374 (link)

   SKIP policy emits event before persisting next-run update

   For MissedRunPolicy.FAIL (the focus of this PR) the ScheduleMissed event is correctly emitted only after a successful transaction. However, the SKIP path emits the event before calling updateNextRun(schedule):

   await this.eventBus.emit('ScheduleMissed', { ... });  // event first
   await this.updateNextRun(schedule);                    // persist second

   If updateNextRun fails (e.g. DB write error), the ScheduleMissed event has already been dispatched to all subscribers but the schedule's nextRunAt was never advanced — so the same run will be treated as missed again on the next tick. Consider persisting the state change before emitting the event, consistent with the FAIL path's approach.

2. src/bootstrap.ts, line 58-65 (link)

   mode flag silently has no effect when resourceMonitor is injected

   When a caller provides both mode: 'run' (or any mode with skipResourceMonitoring: true) and a custom resourceMonitor, the skip flag derived from the mode is effectively ignored. The factory returns the injected monitor immediately before reaching the startMonitoring() branch:

   container.registerSingleton('resourceMonitor', () => {
     if (options.resourceMonitor) {
       return options.resourceMonitor;  // exits here — mode flag never checked
     }
     // ... skipResourceMonitoring only applies below this point
   });

   This is fine in practice (a custom monitor has no startMonitoring() side effects anyway), but the interaction isn't obvious. A short note in the BootstrapOptions JSDoc or deriveModeFlags documentation clarifying that skipResourceMonitoring only affects the built-in SystemResourceMonitor would help future contributors avoid confusion.

_{Last reviewed commit: "chore: fix biome for..."}

[dean0x]

Code Review Summary

Nine reviewers analyzed this PR across architecture, complexity, consistency, documentation, performance, regression, security, tests, and TypeScript. Here's the consolidated feedback:

Blocking Issues (Must Fix Before Merge)

1. Dead Config interface left in core/interfaces.ts (95-90% confidence)

File: src/core/interfaces.ts:351-359
Reviewers: Architecture, Consistency, TypeScript, Regression

The getConfig() adapter in bootstrap.ts was correctly removed, along with its Config import (commit c1c2861). However, the Config interface itself remains exported but has zero consumers. It's dead code in the core interfaces module.

Fix: Delete lines 351-359:

/**
 * Configuration
 */
export interface Config {
  readonly maxOutputBuffer: number;
  readonly taskTimeout: number;
  readonly cpuCoresReserved: number;
  readonly memoryReserve: number;
  readonly logLevel: 'debug' | 'info' | 'warn' | 'error';
  readonly maxListenersPerEvent?: number;
  readonly maxTotalSubscriptions?: number;
}

---

2. BootstrapMode test re-implements logic instead of testing behavior (85% confidence)

File: tests/integration/service-initialization.test.ts:387-398
Reviewers: Tests, TypeScript

The test duplicates the mode-to-flag derivation logic from src/bootstrap.ts:119-122 verbatim, then asserts against hardcoded values. This tests the test's own logic, not the actual bootstrap() function. If someone changes the mapping in bootstrap(), this test will silently pass with stale expectations.

Fix Options:

• (A) Extract derivation function — Export deriveModeFlags(mode: BootstrapMode) from bootstrap.ts and test that pure function directly (simplest, fastest)
• (B) Integration test — Call bootstrap({ mode: 'cli' }) and verify actual behavior (e.g., no scheduleExecutor in container)

Recommendation: Use Option A for speed and clarity.

// In src/bootstrap.ts
export function deriveModeFlags(mode: BootstrapMode) {
  return {
    skipResourceMonitoring: mode === 'run',
    skipScheduleExecutor: mode === 'cli' || mode === 'run',
    skipRecovery: mode === 'cli',
  };
}

// In test: import and test the real function
import { deriveModeFlags } from '../../src/bootstrap.js';

it.each([...])('mode "%s" produces correct flags', (mode, expected) => {
  expect(deriveModeFlags(mode)).toEqual(expected);
});

---

Should-Fix Issues (Improve Code Quality)

3. Rollback test missing ScheduleMissed event verification (82% confidence)

File: tests/unit/services/schedule-executor.test.ts:367-394
Reviewer: Tests

The transaction rollback test correctly verifies schedule and execution history state, but does NOT assert that ScheduleMissed event was suppressed. The source code (schedule-executor.ts:405-409) correctly breaks before emitting the event on failure, but the test doesn't validate this invariant. A future refactor could easily break this by reordering the code.

Fix: Subscribe to ScheduleMissed and assert zero events:

const missedEvents: unknown[] = [];
eventBus.subscribe('ScheduleMissed', async (event: unknown) => {
  missedEvents.push(event);
});

await executor.triggerTick();
await flushEventLoop();

// ...existing assertions...

// No ScheduleMissed event — transaction failed, so no side effects
expect(missedEvents).toHaveLength(0);

---

4. BootstrapOptions JSDoc removed without equivalent documentation (85% confidence)

File: src/bootstrap.ts:39-43
Reviewer: Documentation

The original BootstrapOptions interface had per-property JSDoc explaining each field's purpose (e.g., "Custom ProcessSpawner implementation (e.g., NoOpProcessSpawner for tests)"). The refactoring removed these hints. While the BootstrapMode type alias has good top-level documentation, the remaining processSpawner and resourceMonitor fields lost their JSDoc hints.

Fix: Add brief JSDoc to DI fields:

export interface BootstrapOptions {
  mode?: BootstrapMode;
  /** Custom ProcessSpawner (e.g., NoOpProcessSpawner for tests) */
  processSpawner?: ProcessSpawner;
  /** Custom ResourceMonitor (e.g., TestResourceMonitor for tests) */
  resourceMonitor?: ResourceMonitor;
}

---

5. OutputRepository methods lack per-method JSDoc (82% confidence)

File: src/core/interfaces.ts:471-476
Reviewer: Documentation

The OutputRepository interface was moved from the implementation file to core/interfaces.ts as part of issue #101. The move is an opportunity to add documentation since it's now a first-class core contract alongside well-documented repository interfaces. All four methods (save, append, get, delete) are currently undocumented.

Fix: Add method-level JSDoc:

export interface OutputRepository {
  /** Persist full output snapshot (stdout + stderr) */
  save(taskId: TaskId, output: TaskOutput): Promise<Result<void>>;
  /** Append incremental data to a stream */
  append(taskId: TaskId, stream: 'stdout' | 'stderr', data: string): Promise<Result<void>>;
  /** Retrieve stored output for a task */
  get(taskId: TaskId): Promise<Result<TaskOutput | null>>;
  /** Remove stored output for a task */
  delete(taskId: TaskId): Promise<Result<void>>;
}

---

Pre-Existing Issues (Not Blocking, Context Only)

bootstrap() function length (82% confidence, pre-existing)

• File: src/bootstrap.ts:118-469 (351 lines)
• The function exceeds recommended length, but is an accepted exception as a DI composition root with linear, sequential structure and low cyclomatic complexity. No action needed for this PR.

interfaces.ts file size (82% confidence, pre-existing)

• File: src/core/interfaces.ts (504 lines)
• Slightly above 500-line threshold, but acceptable for a "barrel" interfaces file. No action needed for this PR.

ROADMAP.md needs update (85% confidence, pre-existing)

• File: docs/ROADMAP.md:73-79
• Items refactor: wrap ScheduleExecutor handleMissedRun in transaction #83, Move OutputRepository interface to core/interfaces.ts #101, Replace BootstrapOptions boolean flags with mode enum #104 should move from "Remaining" to "Features (merged)" once this PR lands. Defer to v0.6.0 release preparation.

---

Lower-Confidence Suggestions (60-79%)

Several reviewers flagged lower-confidence observations that don't block merge but document patterns worth considering:

• Sequential schedule processing (65%) — intentional to avoid thundering herd
• clearRunningScheduleByTask linear scan (60%) — acceptable due to small active schedule counts
• Integration tests for ScheduleExecutor signature change (65%) — TypeScript already enforces types
• CLAUDE.md ScheduleExecutor description refinement (65%) — note about transactional wrapping is still accurate

---

Scores Summary

Category	Score	Status
Architecture	9/10	APPROVED_WITH_CONDITIONS
Complexity	9/10	APPROVED
Consistency	9/10	APPROVED_WITH_CONDITIONS
Documentation	7/10	APPROVED_WITH_CONDITIONS
Performance	9/10	APPROVED
Regression	9/10	APPROVED
Security	9/10	APPROVED
Tests	8/10	CHANGES_REQUESTED
TypeScript	9/10	APPROVED_WITH_CONDITIONS

---

Reviewer Consensus

Architecture: All three tech debt cleanups execute correctly:

• OutputRepository interface move follows established pattern ✓
• BootstrapMode enum eliminates invalid flag combinations (8 → 3 valid states) ✓
• ScheduleExecutor transaction wrapping fixes atomicity bug ✓

Quality verdict: These changes reduce complexity, improve type safety, and fix a real correctness issue. Address the 5 issues above (1 blocking + 4 should-fix) before merge. All suggestions are clear and actionable.

---

Reviewed by: Architecture, Complexity, Consistency, Documentation, Performance, Regression, Security, Tests, TypeScript reviewers
PR Recommendation: APPROVED_WITH_CONDITIONS (fix blocking issue + 4 should-fix items)