chore: tech debt sweep — #96, #97, #98, #99, #102

[dean0x]

Summary

Post-v0.6.0 tech debt cleanup. Closes 5 issues; 3 were already resolved, 2 required code changes.

• tech-debt: WorkerRepository missing Zod row validation schema #96 (WorkerRepository Zod validation): Already implemented — closed with comment
• tech-debt: WorkerRepository missing operationErrorHandler #97 (WorkerRepository operationErrorHandler): Already implemented — closed with comment
• tech-debt: worker-repository.test.ts not excluded from test:implementations #99 (worker-repository.test.ts grouping): By design — closed with comment
• tech-debt: EventDrivenWorkerPool.spawn() at 99 lines — extract post-spawn setup #98: Extract registerWorker() from spawn() — reduces method from ~99 to ~65 lines
• Extract exitOnError CLI helper to reduce boilerplate #102: Extract exitOnError/exitOnNull CLI helpers — eliminates 13 repeated guard blocks across status.ts, logs.ts, schedule.ts (net -41 lines)

Also updated #31 tech debt backlog: marked 3 stale/fixed entries, updated open count from 14 → 4.

Closes #96, closes #97, closes #98, closes #99, closes #102

Test plan

• npm run build — clean typecheck
• npm run test:implementations — 314 tests pass (covers tech-debt: EventDrivenWorkerPool.spawn() at 99 lines — extract post-spawn setup #98)
• npm run test:cli — 157 tests pass (covers Extract exitOnError CLI helper to reduce boilerplate #102)
• npx biome check — clean
• Snyk SAST — 0 issues

[greptile-apps]

Confidence Score: 5/5

• This PR is safe to merge — it is a pure refactor with no logic changes, full test coverage, and clean typecheck.
• All changes are mechanical deduplication of existing patterns. The new exitOnError/exitOnNull helpers are well-tested, the worker pool extraction preserves the exact rollback logic from the original code, and the PR description confirms 314 + 157 tests pass with a clean build and SAST scan. The one flagged item (dual Date.now() calls in registerWorker) is a pre-existing minor inconsistency, not a regression.
• No files require special attention.

Important Files Changed

Filename	Overview
src/cli/services.ts	Introduces exitOnError and exitOnNull guard helpers and refactors withReadOnlyContext/withServices to use them — clean, well-typed reduction of boilerplate.
src/cli/commands/logs.ts	Guard calls replace inline if/exit blocks; return value of exitOnNull is intentionally discarded (only used as an existence check, discussed in previous threads).
src/cli/commands/status.ts	Clean adoption of guard helpers; exitOnError return value is correctly captured in all paths.
src/cli/commands/schedule.ts	Largest consumer of the new guards; cancel/pause/resume correctly discard exitOnError return values (services return Result<void>). Added explicit Promise<void> return types to previously untyped functions.
src/implementations/event-driven-worker-pool.ts	Worker registration logic cleanly extracted to registerWorker(); minor pre-existing inconsistency where in-memory startedAt and DB-registered startedAt use separate Date.now() calls — the refactoring is a good moment to unify them.
tests/unit/cli-services.test.ts	New unit test suite with good edge-case coverage (falsy-but-not-null zero/empty-string values, spinner stop messages, prefix formatting). Correctly mocks process.exit and ui.error.
package.json	Adds cli-services.test.ts to the test:cli script so the new tests are included in the standard CLI test run.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI Command] --> B{exitOnError}
    B -->|result.ok = false| C[s?.stop stopMsg]
    C --> D[ui.error prefix + message]
    D --> E[process.exit 1]
    B -->|result.ok = true| F[return result.value]
    F --> G{exitOnNull}
    G -->|value == null| H[s?.stop Not found]
    H --> I[ui.error msg]
    I --> J[process.exit 1]
    G -->|value != null| K[return narrowed T]
    K --> L[Continue command logic]

Loading

_{Last reviewed commit: "fix: biome import or..."}

[dean0x]

⚠️ Missing Unit Tests (HIGH - Blocking)

The new exitOnError and exitOnNull helpers (lines 15-37) are now the single point of error handling for the entire CLI layer (used in 4 command files, ~15 call sites), but they have no unit tests.

Problem:

• If either function regresses (wrong exit code, missing spinner stop, broken type narrowing), all CLI commands fail silently
• These call process.exit(1) — getting them wrong means critical behavior breaks

Suggested Fix:
Add unit tests in tests/unit/cli/services.test.ts covering:

• (1) success path returns unwrapped value
• (2) error path calls process.exit(1)
• (3) error path stops spinner with correct message
• (4) exitOnNull handles both null and undefined

---

Feedback via Claude Code • /code-review

[dean0x]

⚠️ UX Regression: Spinner Stop Message (MEDIUM - Should Fix)

The spinner stop message changed from descriptive "Initialization failed" to generic "Failed" in exitOnError (line 17).

Problem:

• Original: withReadOnlyContext() and withServices() both called s?.stop('Initialization failed')
• New: exitOnError hardcodes s?.stop('Failed')
• This loses context about what failed on initialization errors

Suggested Fix:
Add a stopMsg parameter to exitOnError (like exitOnNull already has):

export function exitOnError<T>(
  result: Result<T>,
  s?: Spinner,
  prefix?: string,
  stopMsg = 'Failed',
): T {
  if (!result.ok) {
    s?.stop(stopMsg);
    // ...
  }
  return result.value;
}

Then restore context:

export function withServices(...) {
  const container = exitOnError(
    await bootstrap({ mode: 'cli' }),
    s,
    'Bootstrap failed',
    'Initialization failed'  // <-- restore this
  );
}

---

Feedback via Claude Code • /code-review

[dean0x]

ℹ️ Duplicate Date.now() Calls (MEDIUM - Pre-existing)

The registerWorker method calls Date.now() twice independently at lines 222 and 238, creating two separate timestamps that should be identical.

Problem:

• Line 222: const startedAt: Date.now() for WorkerState.startedAt
• Line 238: startedAt: Date.now() for database registration
• These can differ by microseconds despite being conceptually the same value
• Semantic inconsistency between in-memory and DB timestamps could cause confusing diagnostics

Suggested Fix:
Capture once and reuse:

private registerWorker(...): Result<WorkerState> {
  const now = Date.now();  // Capture once
  const worker: WorkerState = { ..., startedAt: now, ... };
  // ...
  this.workerRepository.register({ ..., startedAt: now, ... });  // Reuse
}

Note: This is a pre-existing issue moved as-is during the extract refactor. Not introduced by this PR.

---

Feedback via Claude Code • /code-review

[dean0x]

Code Review Summary — PR #109 (chore/tech-debt-sweep)

Analysis Complete

8 reviewers across 8 reports (Architecture, Performance, Tests, Regression, TypeScript, Security, Complexity, Consistency)

High-confidence (≥80%) issues: 3 inline comments ☝️

• Missing unit tests for exitOnError/exitOnNull (95%, HIGH)
• Spinner message regression (90%, MEDIUM)
• Duplicate Date.now() in registerWorker (82%, MEDIUM pre-existing)

---

Lower-Confidence Suggestions (60-79%)

These don't warrant inline comments but are worth considering:

Architecture (65% confidence):

• exitOnError return type relies on process.exit being typed never. Adding throw after process.exit(1) would clarify control flow.

TypeScript (65% confidence):

• exitOnNull return value discarded in logs.ts:17 but used in status.ts:17. Capturing the return would be more consistent.
• Asymmetry: exitOnNull has stopMsg parameter but exitOnError hardcodes 'Failed'.

Complexity (65% confidence):

• Duplicate detail-formatting pattern in schedule.ts:199-207 and schedule.ts:242-247. A shared formatScheduleDetails helper could consolidate.

---

Pre-existing Issues (Not Blocking)

Architecture — Medium (82%):
Incomplete exitOnError/exitOnNull adoption: 6 CLI files still use old pattern (cancel.ts, resume.ts, pipeline.ts, run.ts, agents.ts, init.ts). Not introduced by this PR; consider follow-up sweep.

Tests — Medium (85%):
CLI tests use simulation functions instead of testing actual command code. Tests validate "what code should do" rather than "what code actually does." Pre-existing architectural pattern; no action needed for this PR.

---

Overall Assessment

✅ Clean architectural refactoring with strong DRY improvements

• registerWorker extraction: method extraction (40 → 6 lines)
• exitOnError/exitOnNull: DRY helper (eliminates ~60 lines boilerplate)
• Self-dogfooding: Uses own helpers in withReadOnlyContext/withServices

✅ No regressions introduced (aside from minor spinner message simplification)

✅ Type safety maintained (strict TypeScript, no any types, proper narrowing)

✅ Security unchanged (pure refactor, no new inputs, no changed control flow)

---

Next Steps

1. Add unit tests for exitOnError/exitOnNull (blocker)
2. Consider stopMsg parameter on exitOnError to restore context
3. Document or plan follow-up for remaining 6 CLI files (nice-to-have)

---

Review completed by Claude Code at 2026-03-20 17:59 UTC
Automated via /code-review operation

[greptile-apps]

Dual Date.now() calls may produce mismatched startedAt timestamps

worker.startedAt and the DB-registered startedAt are sourced from two separate Date.now() calls. While the drift is usually a few microseconds (both calls are synchronous), the in-memory WorkerState and the persisted record could carry different timestamps — which matters if any cross-process coordination logic uses startedAt for ordering or deduplication.

The extraction into registerWorker creates a clean opportunity to fix this by reusing the worker's own timestamp:

Suggested change

	memoryUsage: 0,
	process: childProcess,
	task,
	};
	this.workers.set(workerId, worker);
	this.taskToWorker.set(task.id, workerId);
	const regResult = this.workerRepository.register({
	workerId,
	taskId: task.id,
	pid,
	ownerPid: process.pid,
	agent: agentProvider,
	startedAt: Date.now(),
	});
	if (!regResult.ok) {
	const workerId = WorkerId(`worker-${pid}`);
	const worker: WorkerState = {
	id: workerId,
	taskId: task.id,
	pid,
	startedAt: Date.now(),
	cpuUsage: 0,
	memoryUsage: 0,
	process: childProcess,
	task,
	};
	this.workers.set(workerId, worker);
	this.taskToWorker.set(task.id, workerId);
	const regResult = this.workerRepository.register({
	workerId,
	taskId: task.id,
	pid,
	ownerPid: process.pid,
	agent: agentProvider,
	startedAt: worker.startedAt,
	});