Tech Debt Backlog

[dean0x]

Tech Debt Backlog

Auto-maintained by /code-review. Items are added when pre-existing issues are found during code reviews.

How This Works

• Issues found in code you didn't change are logged here
• Each item links to the review that found it
• Items are automatically removed when fixed
• Check items off manually as you address them

Risk Legend

• 🟢 Low risk - Safe, isolated change
• 🟡 Medium risk - Requires careful testing, may have side effects
• 🔴 High risk - Breaking changes possible, significant refactor, or risky dependencies
• ⏸️ Deferred - Intentionally delayed due to risk/reward tradeoff

---

🎯 Pre-v0.4.0 Priorities

Decision: These items should be fixed BEFORE starting v0.4.0 (Task Resumption + Scheduling) to avoid compounding technical debt.

Rationale: v0.4.0 adds new database tables, event handlers, and timer-based execution. Fixing these patterns now prevents copying bad patterns into new code.

Priority	Item	Why Now?	Status
P0	bootstrap() extraction	v0.4.0 adds 2+ handlers + timer setup; 417 lines will balloon	✅ Fixed in PR #42
P1	findAll() pagination	v0.4.0 adds 3 new tables; pattern needed before more unbounded queries	✅ Fixed in PR #43
P2	Timing-based test waits	Scheduling tests are inherently timing-heavy; flaky test debt compounds	✅ Fixed — replaced with waitForEvent() / flushEventLoop() helpers

Deferred (OK to skip for v0.4.0)

• EventBus any types - Large refactor, can live with it
• resource-monitor mutable state - Isolated concern
• Documentation line refs - Fix during v0.4.0 docs

---

Items

Architecture

• 🟡 [architecture] src/services/handlers/query-handler.ts:88 - QueryHandler uses findAllUnbounded() as workaround
  → [Review: 2025-12-18](PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43)
  → Context: QueryHandler.handleTaskStatusQuery() needs all tasks but pagination was added to findAll(). Currently uses findAllUnbounded() to maintain behavior.
  → Proper Solution: Add pagination support to TaskStatusQuery event and MCP TaskStatus tool. Allow limit/offset parameters to flow through the event system.
  → Why Deferred: Scope creep for PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43. Requires MCP interface changes and event schema updates.

• 🟢 [architecture] src/implementations/task-repository.ts:255 - findByStatus() lacks pagination
  → [Review: 2025-12-21](PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43)
  → Context: findByStatus() returns all matching tasks without pagination. Could return thousands of tasks for common statuses like 'completed'.
  → Proper Solution: Add limit/offset parameters like findAll(). Update findByStatus(status, limit?, offset?).
  → Why Deferred: Scope creep for PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43. Already documented in JSDoc that it's not paginated.

• 🟢 [security] src/implementations/*-repository.ts - No input validation for pagination parameters
  → [Review: 2025-12-21](PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43)
  → Context: findAll(limit, offset) accepts any number without bounds validation. Could pass Number.MAX_SAFE_INTEGER or negative values.
  → Proper Solution: Add MAX_LIMIT cap (e.g., 1000) and validate offset >= 0. Return error Result for invalid inputs.
  → Why Deferred: Low priority - internal API, SQLite handles edge cases gracefully.

Documentation

• 🟡 [documentation] docs/architecture/TASK_ARCHITECTURE.md - References outdated line numbers in source files
  → Review: 2025-12-13
  → Recommendation: Use function/class names instead of line numbers as references to avoid drift with code changes.
• 🟡 [documentation] docs/TASK-DEPENDENCIES.md - Missing v3 migration documentation, outdated cycle detection reference
  → Review: 2025-12-13
  → Recommendation: Update database schema section for CHECK constraints. Correct cycle detection implementation reference.
• ℹ️ [documentation] src/core/interfaces.ts:106 - Missing JSDoc for findByStatus() pagination consideration
  → ✅ Fixed in PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43 - Added JSDoc explaining findByStatus() is not paginated
• ℹ️ [documentation] src/implementations/dependency-repository.ts:489 - Stale findAll() JSDoc example
  → ✅ Fixed in PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43 - Updated JSDoc to accurately describe pagination behavior

Tests

• 🟡 [tests] tests/integration/*.test.ts - 40+ timing-based waits using setTimeout (flaky test risk)
  → ✅ Fixed — All setTimeout waits replaced with event-driven waitForEvent() / flushEventLoop() helpers across integration tests
• 🟢 [tests] tests/unit/implementations/task-repository.test.ts - No unit test file for task-repository
  → ✅ Fixed in PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43 - Added 10 unit tests for pagination, count, and unbounded methods
• ℹ️ [tests] tests/fixtures/test-doubles.ts:289 - TestTaskRepository ordering differs from production
  → ✅ Fixed in PR feat: add pagination to findAll() methods (P1 pre-v0.4.0) #43 - Added sort by created_at DESC to match production behavior

---

Summary

Items Added This Review: 1 new architecture item from PR #43

• Architecture: findByStatus() pagination (deferred, documented in JSDoc)

Items Fixed This Review: 5 items

• P1: findAll() pagination ✅
• Documentation: findByStatus() JSDoc ✅
• Documentation: findAll() stale JSDoc ✅
• Tests: task-repository unit tests ✅
• Tests: TestTaskRepository ordering ✅

Current Open Items: 5 (3 architecture, 2 documentation)

---

Last updated: 2026-03-09
Updated: timing-based test waits verified fixed (zero setTimeout in integration tests)
Original: Issues #25, #28; ongoing code reviews

[dean0x]

Items Resolved in PR #32

The following items were fixed in the recent merge:

Architecture

• src/implementations/resource-monitor.ts:325-436 - TestResourceMonitor lacks recordSpawn() implementation → Added in commit ea410d3

Documentation

• docs/TASK-DEPENDENCIES.md:706 - resolveDependencies line reference outdated (says 199, should be 344) → Fixed in commit d0407d2

Dependencies (from Issue #25)

• Dev dependency vulnerabilities (glob, vite, body-parser) → Fixed via npm audit fix in commit dbeaecc

---

Remaining items are still open for future work.

[dean0x]

Tech Debt Updates - v0.3.2 Cleanup

The following PRs address items from this backlog:

PRs Created

1. PR fix: increase spawn delay from 1s to 10s for stability #35 - fix: increase spawn delay from 1s to 10s

   • Not in this list, but improves stability

2. PR refactor: move baseline schema into migration v1 #36 - refactor: move baseline schema into migration v1

   • Closes Issue refactor: Move baseline schema into migration v1 for single source of truth #21
   • Makes migrations single source of truth for schema

3. PR docs: fix outdated line references and architecture status #37 - docs: fix outdated line references and architecture status

   • Addresses: docs/TASK-DEPENDENCIES.md line references
   • Addresses: docs/architecture/TASK_ARCHITECTURE.md "Phase 4 80% complete" → "complete"

Items Resolved by These PRs

Documentation:

• docs/TASK-DEPENDENCIES.md:706 - resolveDependencies line reference (344 → 352)
• docs/architecture/TASK_ARCHITECTURE.md - Updated status from "80% complete" to "complete"

Closed as Won't Fix

• Issue perf: Replace in-memory cycle detection with database-native recursive CTE #20 (Recursive CTE) - Addressed differently via caching/batching in v0.3.1. See issue for full explanation.

Items Still Outstanding

Most remaining items are:

• TypeScript as any casts - Documented as TypeScript limitations, not bugs
• Complexity - processNextTask(), handleTaskDelegated need refactoring (lower priority)
• Performance micro-optimizations - Not causing actual issues

These can be addressed opportunistically or in future releases.

[dean0x]

P2 Complete ✅

Timing-based test waits resolved in commit 7f6e7f6 (squash merged from PR #44).

Changes

• Replaced 40+ setTimeout calls with event-driven synchronization
• Added waitFor(), once(), removeListener() to TestEventBus
• Fixed memory leaks in handler lifecycle
• Added ResourceMonitor dependency injection to bootstrap

Pre-v0.4.0 Status

Priority	Item	Status
P0	bootstrap() extraction	✅ PR #42
P1	findAll() pagination	✅ PR #43
P2	Timing-based test waits	✅ 7f6e7f6

All pre-v0.4.0 priorities complete. Codebase is ready for v0.4.0 (Task Resumption + Scheduling).

[dean0x]

v0.4.0 Tech Debt Review (2026-02-20)

✅ No new tech debt introduced - v0.4.0 (Task Scheduling + Task Resumption) did not add items to existing tracked list.

Current Status:

• All 5 existing items remain low/medium severity
• No new architectural patterns that contradict existing standards
• New code follows established patterns: Result types, dependency injection, event-driven design

Most Impactful Open Item:

• 🟡 P2/MEDIUM - Timing-based test waits (40+ setTimeout in integration tests) - remains highest-impact item for v0.5.0+

Tracked for v0.5.0+:

1. EventBus any types - large refactor, can coexist with current system
2. Timing-based test waits - impacts test reliability, not critical for feature functionality
3. findByStatus() pagination - deferred, low priority
4. Documentation line number refs - fix during next doc overhaul
5. Pagination input validation - low priority, SQLite handles gracefully

Assessment: Technical debt remains manageable. Recommended focus: timing-based tests after v0.4.0 stabilizes in production.

[dean0x]

Correction to previous comment (2026-02-21)

The v0.4.0 tech debt review above incorrectly listed timing-based test waits as the most impactful open item. PR #44 (commit 7f6e7f6) resolved the original 40+ setTimeout calls. However, v0.4.0's new test files (task-scheduling.test.ts, task-resumption.test.ts) reintroduced 14 setTimeout waits.

These have now been fixed in commit fbfda1d — replaced with waitForEvent() from event-helpers.ts for deterministic event-driven synchronization. Also fixed a broken EventBus type import in event-helpers.ts that went unnoticed because Vitest/esbuild doesn't type-check.

Updated backlog status:

• Timing-based test waits → Fully resolved (PR fix: replace timing-based test waits with deterministic synchronization #44 + fbfda1d)
• Remaining 4 items unchanged (EventBus any types, findByStatus() pagination, doc line refs, pagination input validation)

[dean0x]

PersistenceHandler double-save on schedule-triggered tasks

Location: src/services/handlers/schedule-handler.ts (save inside transaction) + src/services/handlers/persistence-handler.ts (save on TaskDelegated)

Problem: Tasks created by the schedule handler are saved inside the transaction, then TaskDelegated is emitted. PersistenceHandler receives the event and calls taskRepo.save() again. The save uses INSERT OR IGNORE, so it's a harmless no-op, but it's a wasted write.

Options: (1) Check if task exists before saving, (2) Emit different event type, or (3) Accept the redundancy.

Risk: None — purely a performance micro-optimization. Not worth fixing unless profiling shows it matters.

Reference: Identified during Issue #81 planning (transaction atomicity refactor).

[dean0x]

Deferred from PR #85 Resolution

1. Extract TransactionRunner interface (DIP violation)

• Location: src/core/interfaces.ts, src/implementations/database.ts, src/services/handlers/schedule-handler.ts, src/services/handler-setup.ts
• Problem: ScheduleHandler depends on concrete Database class from implementations layer. Service layer should depend on an interface.
• Proposed fix: Extract TransactionRunner interface with single runInTransaction<T>(fn: () => T): Result<T> method. Have Database implement it. Update ScheduleHandler and HandlerDependencies to depend on the interface.
• Risk: 🟡 Low-medium. Touches 4 files, changes HandlerDependencies public interface. Straightforward but architectural pattern change.
• Why deferred: Affects shared data structures across multiple modules. Should be a deliberate, focused PR.

2. Error recovery cancellation semantics

• Location: src/services/handlers/schedule-handler.ts (~line 432-434)
• Problem: When step-0 TaskDelegated emission fails after a pipeline transaction commits, the cleanup loop cancels tasks sequentially with await taskRepo.update(). Could be wrapped in runInTransaction() with updateSync.
• Risk: 🟡 Medium. Current sequential pattern is intentionally resilient — if one cancellation fails, others still proceed. Wrapping in transaction makes it all-or-nothing, which changes error recovery semantics.
• Why deferred: Design tradeoff requiring deliberate architectural decision, not a clear improvement.