bunfig.toml (1)

5-7: Consider making coverage opt-in rather than always-on.

Having coverage = true by default means every bun test invocation will generate coverage data, which adds overhead. Typically, coverage is run explicitly via bun test --coverage when needed. The current setup may slow down routine test runs during development.

If this is intentional for CI enforcement, consider documenting this choice. Otherwise, you could remove this line and rely on the test:coverage script in package.json for coverage reports.

Suggested change to make coverage opt-in

 [test]
 # Run tests from src and tests directories
 root = "."
 
 # Coverage settings
-coverage = true
 coverageDir = "./coverage"

.github/workflows/ci.yml (1)

10-70: Matrix runtime is currently misleading.
The node entry doesn’t influence any steps (tests still run via Bun), so this adds duplicate work and implies Node coverage that isn’t actually exercised. Consider either dropping the Node axis or adding Node-specific install/test steps gated by matrix.runtime.

tests/tui/theme.test.ts (1)

184-234: Minor DRY: lift shared TaskStatus list.
The same status array is declared twice; a single constant keeps the tests tidy.

♻️ Proposed refactor

 import { describe, test, expect } from 'bun:test';
 import {
   colors,
   statusIndicators,
   keyboardShortcuts,
   fullKeyboardShortcuts,
   layout,
   getTaskStatusColor,
   getTaskStatusIndicator,
   formatElapsedTime,
   type RalphStatus,
   type TaskStatus,
 } from '../../src/tui/theme.js';
 
+const TASK_STATUSES: TaskStatus[] = [
+  'done',
+  'active',
+  'actionable',
+  'pending',
+  'blocked',
+  'error',
+  'closed',
+];
+
 describe('theme', () => {
   describe('colors', () => {
@@
-  describe('getTaskStatusColor', () => {
-    const taskStatuses: TaskStatus[] = ['done', 'active', 'actionable', 'pending', 'blocked', 'error', 'closed'];
+  describe('getTaskStatusColor', () => {
     test('should return valid color for all task statuses', () => {
-      taskStatuses.forEach((status) => {
+      TASK_STATUSES.forEach((status) => {
         const color = getTaskStatusColor(status);
         expect(color).toBeDefined();
         expect(color).toMatch(/^#[0-9a-fA-F]{6}$/);
       });
     });
@@
-  describe('getTaskStatusIndicator', () => {
-    const taskStatuses: TaskStatus[] = ['done', 'active', 'actionable', 'pending', 'blocked', 'error', 'closed'];
+  describe('getTaskStatusIndicator', () => {
     test('should return indicator for all task statuses', () => {
-      taskStatuses.forEach((status) => {
+      TASK_STATUSES.forEach((status) => {
         const indicator = getTaskStatusIndicator(status);
         expect(indicator).toBeDefined();
         expect(indicator.length).toBeGreaterThan(0);
       });
     });

tests/plugins/beads-tracker.test.ts (1)

46-70: Initialization tests lack assertions to verify config was applied.

Several tests call initialize() but don't verify the configuration was actually stored. For example, accepts beadsDir config (lines 47-51) doesn't check that the internal beadsDir was set. Similarly for accepts labels as string, accepts labels as array, and accepts workingDir config.

Consider adding assertions or accessing internal state to verify the configuration took effect. Otherwise, these tests only confirm that initialize() doesn't throw.

♻️ Example assertion for beadsDir test

     test('accepts beadsDir config', async () => {
       await plugin.initialize({ beadsDir: '.custom-beads' });
-      // Note: isReady will be false if .beads doesn't exist
-      // but config should be accepted
+      // Verify config was stored (access private field for testing)
+      expect((plugin as unknown as { beadsDir: string }).beadsDir).toBe('.custom-beads');
     });

CONTRIBUTING.md (2)

200-219: Add language specifier to fenced code block.

The directory structure code block is missing a language specifier, which triggers a markdown lint warning. Add a language like text or leave empty lines to indicate it's plain text.

♻️ Suggested fix

-```
+```text
 tests/
 ├── commands/           # CLI command tests

---

322-328: Clarify coverage expectations versus CI threshold.

The guidance states "Aim for 80%+ line coverage on new code", but the PR summary indicates CI enforces a 40% threshold. This discrepancy could confuse contributors about the actual expectations.

Consider clarifying that 80% is the aspirational target whilst 40% is the enforced minimum, or align the documentation with the actual CI threshold.

♻️ Suggested clarification

 ### Coverage Requirements

-- Aim for **80%+ line coverage** on new code
+- CI enforces a **40% minimum** coverage threshold
+- Aim for **80%+ line coverage** on new code where practical
 - Critical paths (engine, plugins) should have higher coverage

tests/commands/run.test.ts (1)

1-7: LGTM with minor note.

The ABOUTME comment is compliant. The import of spyOn on line 6 appears unused in this file—consider removing it if not needed.

♻️ Remove unused import

-import { describe, test, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test';
+import { describe, test, expect, beforeEach, afterEach, mock } from 'bun:test';

tests/commands/status.test.ts (2)

141-227: Extract duplicated getExitCode helper function.

The getExitCode function is identically defined in each of the 5 tests. Consider extracting it to the describe block level to reduce duplication and improve maintainability.

♻️ Suggested refactor

   describe('exit code mapping', () => {
+    const getExitCode = (status: RalphStatus): StatusExitCode => {
+      switch (status) {
+        case 'completed':
+          return 0;
+        case 'running':
+        case 'paused':
+          return 1;
+        case 'failed':
+        case 'no-session':
+          return 2;
+      }
+    };
+
     test('completed status returns exit code 0', () => {
-      // Test the exit code logic directly
-      const getExitCode = (status: RalphStatus): StatusExitCode => {
-        switch (status) {
-          case 'completed':
-            return 0;
-          case 'running':
-          case 'paused':
-            return 1;
-          case 'failed':
-          case 'no-session':
-            return 2;
-        }
-      };
-
       expect(getExitCode('completed')).toBe(0);
     });

     test('running status returns exit code 1', () => {
-      const getExitCode = (status: RalphStatus): StatusExitCode => {
-        // ... same function definition
-      };
       expect(getExitCode('running')).toBe(1);
     });
     // ... similarly for other tests
   });

---

229-420: Consider consolidating duplicated interfaces and helper functions.

The LockCheckResult and PersistedSessionState interfaces are redefined in each test, along with variations of the determineStatus function. Whilst some variations exist in the switch statement, the core logic is largely duplicated.

Consider extracting the interface definitions to the describe block level. The determineStatus function variations could be either consolidated or kept separate if testing different behavioural aspects intentionally.

♻️ Suggested approach

describe('status determination logic', () => {
  interface LockCheckResult {
    isLocked: boolean;
    isStale: boolean;
    lock: { pid: number } | null;
  }

  interface PersistedSessionState {
    status: string;
  }

  const determineStatus = (
    session: PersistedSessionState | null,
    lockCheck: LockCheckResult
  ): RalphStatus => {
    // Full implementation here
  };

  test('running when lock is held', () => {
    const result = determineStatus(
      { status: 'running' },
      { isLocked: true, isStale: false, lock: { pid: 12345 } }
    );
    expect(result).toBe('running');
  });
  // ... other tests use shared function
});

tests/plugins/json-tracker.test.ts (2)

490-498: Consider using expect.fail() or throwing directly instead of expect(true).toBe(false).

The pattern expect(true).toBe(false) is an anti-pattern that produces unclear failure messages. Bun test provides better alternatives.

🔧 Suggested improvement

     try {
       validatePrdJsonSchema(data, 'test.json');
-      expect(true).toBe(false); // Should not reach here
+      throw new Error('Expected validatePrdJsonSchema to throw');
     } catch (err) {
       expect(err).toBeInstanceOf(PrdJsonSchemaError);
       const schemaErr = err as PrdJsonSchemaError;
       expect(schemaErr.details.some((d) => d.includes('status'))).toBe(true);
     }

Alternatively, use expect().toThrow() consistently as done in lines 387-438.

---

513-520: Same anti-pattern as above.

This block also uses expect(true).toBe(false) as a fail mechanism. Consider refactoring to match the cleaner pattern used elsewhere in this file.

🔧 Suggested improvement

     try {
       validatePrdJsonSchema(data, 'test.json');
-      expect(true).toBe(false);
+      throw new Error('Expected validatePrdJsonSchema to throw');
     } catch (err) {
       expect(err).toBeInstanceOf(PrdJsonSchemaError);
       const schemaErr = err as PrdJsonSchemaError;
       expect(schemaErr.details.some((d) => d.includes('subtasks'))).toBe(true);
     }

tests/tui/output-parser.test.ts (1)

222-232: Conditional assertion may mask test failures.

The conditional if (output.length > 50000) means the toContain('...output trimmed...') assertion only runs sometimes. If the implementation changes, this test might silently pass without verifying the expected behaviour.

🔧 Suggested improvement

Consider making the test deterministic by ensuring the input always triggers trimming:

     test('should trim output when exceeding max size', () => {
       // Create parser and push large amount of data
-      const largeChunk = ('x'.repeat(100) + '\n').repeat(1200);
+      // MAX_PARSED_OUTPUT_SIZE is 100KB, so push ~120KB to ensure trimming
+      const largeChunk = ('x'.repeat(100) + '\n').repeat(1200);
       parser.push(largeChunk);
       const output = parser.getOutput();
-      // Should be trimmed but still have content
-      expect(output.length).toBeLessThan(110_000);
-      if (output.length > 50000) {
-        expect(output).toContain('...output trimmed...');
-      }
+      // Output should be trimmed to MAX_PARSED_OUTPUT_SIZE (100KB) + trim marker
+      expect(output.length).toBeLessThan(110_000);
+      expect(output).toContain('...output trimmed...');
     });

tests/factories/agent-config.ts (1)

74-88: Consider allowing rateLimitHandling customisation.

Unlike createAgentConfigWithRateLimiting, this function doesn't allow customising the rateLimitHandling configuration. This might be intentional, but could limit test flexibility when you need fallbacks with specific rate limit settings.

🔧 Optional improvement

 export function createAgentConfigWithFallbacks(
   fallbackAgents: string[],
-  overrides: Partial<AgentPluginConfig> = {}
+  overrides: Partial<AgentPluginConfig> = {},
+  rateLimitHandling: Partial<RateLimitHandlingConfig> = {}
 ): AgentPluginConfig {
   return createAgentConfig({
     ...overrides,
     fallbackAgents,
     rateLimitHandling: {
       enabled: true,
       maxRetries: 3,
       baseBackoffMs: 5000,
       recoverPrimaryBetweenIterations: true,
+      ...rateLimitHandling,
     },
   });
 }

tests/factories/session-state.ts (1)

14-28: Prefer deterministic defaults for session factories.

Defaults depend on current time and process state (Lines 17–28, 97–103), which can make fixtures vary between runs and cause snapshot brittleness. Consider using fixed constants (or an injectable clock/host info) and only override when a test needs real-time values.

Possible deterministic defaults

+const DEFAULT_TIMESTAMP = new Date('2024-01-01T00:00:00.000Z').toISOString();
+const DEFAULT_CWD = '/test/project';
+const DEFAULT_PID = 12345;

 export const DEFAULT_SESSION_METADATA: SessionMetadata = {
   id: 'test-session-001',
   status: 'running',
-  startedAt: new Date().toISOString(),
-  updatedAt: new Date().toISOString(),
+  startedAt: DEFAULT_TIMESTAMP,
+  updatedAt: DEFAULT_TIMESTAMP,
   agentPlugin: 'claude',
   trackerPlugin: 'json',
   currentIteration: 1,
   maxIterations: 10,
   totalTasks: 5,
   tasksCompleted: 0,
-  cwd: process.cwd(),
+  cwd: DEFAULT_CWD,
 };

 export const DEFAULT_LOCK_FILE: LockFile = {
-  pid: process.pid,
+  pid: DEFAULT_PID,
   sessionId: 'test-session-001',
-  acquiredAt: new Date().toISOString(),
-  cwd: process.cwd(),
+  acquiredAt: DEFAULT_TIMESTAMP,
+  cwd: DEFAULT_CWD,
   hostname: 'test-host',
 };

Also applies to: 97-103

tests/factories/prd-data.ts (1)

61-73: Consider using a fixed timestamp for deterministic test defaults.

DEFAULT_GENERATED_PRD.createdAt uses new Date().toISOString() which produces a different value each time the module is loaded. This could cause flaky tests when comparing full objects or snapshot testing.

💡 Suggested fix

 export const DEFAULT_GENERATED_PRD: GeneratedPrd = {
   name: 'Test Feature',
   slug: 'test-feature',
   description: 'A test feature for unit testing',
   targetUsers: 'Developers and testers',
   problemStatement: 'Need to test PRD generation',
   solution: 'Create comprehensive test factories',
   successMetrics: 'All tests pass',
   constraints: 'Must be type-safe',
   userStories: [DEFAULT_USER_STORY],
   branchName: 'feature/test-feature',
-  createdAt: new Date().toISOString(),
+  createdAt: '2024-01-01T00:00:00.000Z',
 };

tests/factories/tracker-task.ts (2)

98-106: Consider using fixed timestamps for deterministic test defaults.

Similar to the PRD factory, several functions use new Date().toISOString() for updatedAt and syncedAt fields, which produces non-deterministic values. This could cause flaky tests when comparing objects.

💡 Suggested fix for createCompletedTask

 export function createCompletedTask(
   overrides: Partial<TrackerTask> = {}
 ): TrackerTask {
   return createTrackerTask({
     status: 'completed',
-    updatedAt: new Date().toISOString(),
+    updatedAt: '2024-01-01T00:00:00.000Z',
     ...overrides,
   });
 }

Also applies to: 165-176, 181-190

---

195-201: createTaskFilter provides no default values.

This function simply spreads the overrides into an empty object, which is effectively a no-op. Consider adding default values or documenting that it's intentionally minimal for type-safe partial construction.

 export function createTaskFilter(
   overrides: Partial<TaskFilter> = {}
 ): TaskFilter {
-  return {
-    ...overrides,
-  };
+  // Intentionally minimal - TaskFilter fields are all optional
+  return { ...overrides };
 }

tests/plugins/opencode-agent.test.ts (1)

16-18: Avoid double-dispose in this path.

afterEach already disposes the plugin, and the dispose test calls it again. If dispose is not idempotent, this can fail; consider guarding the teardown.

♻️ Suggested guard

-afterEach(async () => {
-  await plugin.dispose();
-});
+afterEach(async () => {
+  if (await plugin.isReady()) {
+    await plugin.dispose();
+  }
+});

Also applies to: 201-205

tests/plugins/tracker-registry.test.ts (2)

6-6: Unused import: spyOn

The spyOn function is imported but not used in this file. The tests use mock() directly instead. Consider removing the unused import.

🧹 Suggested fix

-import { describe, test, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test';
+import { describe, test, expect, beforeEach, afterEach, mock } from 'bun:test';

---

165-184: Test couples tightly to internal implementation detail.

The assertion expect(callCount).toBe(3) assumes that registerBuiltin internally calls the factory once to extract metadata. While this test verifies unique instance creation, it's fragile if the registry implementation changes. Consider either documenting this assumption more explicitly or testing only that instance1 !== instance2 (or their meta names differ) without asserting the exact call count.

tests/plugins/agent-registry.test.ts (2)

123-142: Same implementation coupling as tracker registry tests.

Similar to the tracker registry tests, the assertion expect(callCount).toBe(3) couples the test to internal implementation details. Consider aligning with a consistent approach across both registry test files.

---

215-238: Consider testing instance identity after disposal.

The test verifies disposal is called and that a new instance can be obtained, but line 237 checks expect(newInstance).toBe(mockPlugin) which passes because the factory returns the same mock object. This doesn't actually verify a new instance was created. For a more robust test, consider using a factory that creates distinct instances per call.

src/config/index.test.ts (1)

36-41: Consider hoisting dynamic import for efficiency.

The smol-toml import is performed dynamically inside the helper function, which means it runs on every call. Since this is a test file and the import is used frequently, consider hoisting it to module level.

🧹 Suggested improvement

+import { stringify } from 'smol-toml';
+
 // Helper to write TOML config file
 async function writeTomlConfig(path: string, config: StoredConfig): Promise<void> {
-  const { stringify } = await import('smol-toml');
   const content = stringify(config);
   await writeFile(path, content, 'utf-8');
 }

src/utils/retry.ts (2)

100-102: Consider consolidating with existing delay utility.

There's a similar delay method in src/engine/index.ts (line 1182-1184). While having a standalone sleep function in the utils module is reasonable for reusability, you might consider either:

1. Using this utility in the engine, or
2. Re-exporting this from a shared location

This is a minor observation; the duplication is minimal and the implementations are identical.

---

175-182: Type constraint may be too restrictive for withRetry.

The generic constraint T extends (...args: unknown[]) => Promise<unknown> may cause type inference issues when wrapping functions with specific parameter types. Using any[] for args would better preserve the original function's parameter types.

🧹 Suggested fix

-export function withRetry<T extends (...args: unknown[]) => Promise<unknown>>(
+export function withRetry<T extends (...args: any[]) => Promise<any>>(
   fn: T,
   options: RetryOptions = {}
 ): (...args: Parameters<T>) => Promise<RetryResult<Awaited<ReturnType<T>>>> {
   return async (...args: Parameters<T>) => {
     return retry(() => fn(...args) as Promise<Awaited<ReturnType<T>>>, options);
   };
 }

src/utils/logger.ts (2)

86-105: Consider edge case for negative durations.

The formatDuration function assumes non-negative input. If a negative value is passed (e.g., due to clock skew), it would produce -Xms. Consider adding a guard or documenting the expected input range.

💡 Optional defensive check

 export function formatDuration(ms: number): string {
+  if (ms < 0) {
+    return '0ms';
+  }
   if (ms < 1000) {
     return `${ms}ms`;
   }

---

110-118: Consider edge case for negative bytes.

Similar to formatDuration, negative byte values would produce unexpected output (e.g., NaN from Math.log(-1)). Consider adding a guard.

💡 Optional defensive check

 export function formatBytes(bytes: number): string {
   if (bytes === 0) return '0 B';
+  if (bytes < 0) return '0 B';
 
   const units = ['B', 'KB', 'MB', 'GB', 'TB'];

src/utils/files.ts (1)

101-121: Consider using path.relative() from Node.js.

The custom implementation uses hardcoded / separators, which may cause issues on Windows. Node.js provides path.relative(from, to) that handles cross-platform path separators correctly.

♻️ Suggested refactor

+import { relative } from 'node:path';
+
 /**
  * Get the relative path from one path to another
  */
 export function getRelativePath(from: string, to: string): string {
-  const fromParts = normalize(from).split('/').filter(Boolean);
-  const toParts = normalize(to).split('/').filter(Boolean);
-
-  // Find common prefix
-  let commonLength = 0;
-  while (
-    commonLength < fromParts.length &&
-    commonLength < toParts.length &&
-    fromParts[commonLength] === toParts[commonLength]
-  ) {
-    commonLength++;
-  }
-
-  // Build relative path
-  const upCount = fromParts.length - commonLength;
-  const downParts = toParts.slice(commonLength);
-
-  const relativeParts = [...Array(upCount).fill('..'), ...downParts];
-  return relativeParts.join('/') || '.';
+  return relative(from, to) || '.';
 }

src/utils/process.ts (1)

60-66: Consider escalating to SIGKILL if SIGTERM doesn't terminate the process.

When a timeout occurs, SIGTERM is sent, but some processes may ignore it. A more robust approach would be to escalate to SIGKILL after a grace period if the process doesn't terminate.

♻️ Optional enhancement for robust timeout handling

     // Set up timeout
     if (timeout > 0) {
       timeoutHandle = setTimeout(() => {
         killed = true;
         child.kill('SIGTERM');
+        // Escalate to SIGKILL after grace period
+        setTimeout(() => {
+          if (!child.killed) {
+            child.kill('SIGKILL');
+          }
+        }, 1000);
       }, timeout);
     }

src/utils/validation.ts (1)

222-227: Control character regex is intentional but could be clearer.

The static analysis tool flags the control character ranges (\x00-\x1F and \x7F) as suspicious, but this is a valid use case for sanitisation—removing ASCII control characters (NULL, tabs, newlines, DEL, etc.). Consider adding a comment to clarify intent, or use a more explicit pattern.

♻️ Optional: Add clarifying comment

 /**
  * Sanitize a string by trimming and removing control characters
  */
 export function sanitizeString(value: string): string {
+  // Remove ASCII control characters (0x00-0x1F) and DEL (0x7F)
   return value.trim().replace(/[\x00-\x1F\x7F]/g, '');
 }

tests/mocks/agent-responses.ts (2)

22-32: Timestamps in DEFAULT_EXECUTION_RESULT are set at module load time.

The startedAt and endedAt timestamps use new Date().toISOString() which captures the time when the module loads, not when the factory is called. This could cause unexpected behaviour if tests rely on timestamp ordering or freshness.

♻️ Optional: Generate timestamps dynamically

-export const DEFAULT_EXECUTION_RESULT: AgentExecutionResult = {
-  executionId: 'test-execution-001',
-  status: 'completed',
-  exitCode: 0,
-  stdout: 'Task completed successfully',
-  stderr: '',
-  durationMs: 1000,
-  interrupted: false,
-  startedAt: new Date().toISOString(),
-  endedAt: new Date().toISOString(),
-};
+const DEFAULT_EXECUTION_RESULT_BASE = {
+  executionId: 'test-execution-001',
+  status: 'completed' as const,
+  exitCode: 0,
+  stdout: 'Task completed successfully',
+  stderr: '',
+  durationMs: 1000,
+  interrupted: false,
+};
+
+export const DEFAULT_EXECUTION_RESULT: AgentExecutionResult = {
+  ...DEFAULT_EXECUTION_RESULT_BASE,
+  startedAt: new Date().toISOString(),
+  endedAt: new Date().toISOString(),
+};

// Then in createExecutionResult:
 export function createExecutionResult(
   overrides: Partial<AgentExecutionResult> = {}
 ): AgentExecutionResult {
+  const now = new Date().toISOString();
   return {
-    ...DEFAULT_EXECUTION_RESULT,
+    ...DEFAULT_EXECUTION_RESULT_BASE,
+    startedAt: now,
+    endedAt: now,
     ...overrides,
   };
 }

---

202-234: Consider calling onStart before the promise is created for accurate lifecycle simulation.

Currently, onStart is called at line 221 after the promise is created at line 210. In a real agent, onStart would typically be invoked before or at the very beginning of execution. This ordering might not affect most tests but could matter for lifecycle-sensitive tests.

♻️ Optional: Reorder onStart call

     execute(
       prompt: string,
       files?: AgentFileContext[],
       options?: AgentExecuteOptions
     ): AgentExecutionHandle {
       const executionId = `exec-${Date.now()}`;
       let interrupted = false;

+      options?.onStart?.(executionId);
+
       const promise = new Promise<AgentExecutionResult>((resolve) => {
         setTimeout(() => {
           if (interrupted) {
             resolve(createInterruptedExecution({ executionId }));
           } else {
             resolve({ ...executeResult, executionId });
           }
           options?.onEnd?.({ ...executeResult, executionId });
         }, 10);
       });

-      options?.onStart?.(executionId);
-
       const handle: AgentExecutionHandle = {

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T12:30:04.930Z
Learning: After making code changes, always run: `bun run typecheck && bun run build`

Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T12:29:45.887Z
Learning: After making code changes, always run: bun run typecheck && bun run build

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T12:30:04.930Z
Learning: After making code changes, always run: `bun run typecheck && bun run build`

Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T12:29:45.887Z
Learning: After making code changes, always run: bun run typecheck && bun run build

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}