Forge Code Review — Medium & Low Severity Findings

Companion to the [Critical & High findings article]. These issues affect maintainability, correctness, and developer experience but are not immediate security risks.

---

Medium Severity Findings

MED-1: Inline Tool Definitions Bypass Registry

Files: src/agents/planner.ts, src/agents/implementer.ts, src/agents/reviewer.ts, src/agents/tester.ts

Every agent defines its tools inline with a local runTool() helper that directly invokes Bun.spawn or execAsync. The ToolRegistry in src/tools/index.ts is populated but never queried by agents.

typescript
// planner.ts:27 — local helper, not from registry
async function runTool(name: string, args: string[]): Promise<string> {
  const proc = Bun.spawn(args, { stdout: 'pipe', stderr: 'pipe' });
  // ...
}

Impact: No centralized sandboxing, auditing, or rate-limiting of tool calls. Adding a new tool requires modifying each agent.

Fix: Have agents request tools from the registry and execute through a sandboxed runner.

---

MED-2: Naive Keyword Matching in Planner

File: src/agents/planner.ts:170-220

The createSimplePlan() fallback uses hardcoded keyword arrays to determine task categories:

typescript
const keywords = {
  auth: ['auth', 'login', 'session', 'password', 'jwt', 'oauth'],
  api: ['api', 'endpoint', 'route', 'rest', 'graphql'],
  fix: ['fix', 'bug', 'error', 'crash', 'broken'],
};

Impact: A task like "fix the login API" matches all three categories. No disambiguation logic exists — first match wins.

Fix: Score all categories and pick highest, or defer to LLM classification.

---

MED-3: Fragile Regex in Implementer Failure Fixes

File: src/agents/implementer.ts:290

typescript
const fileMatch = failure.suggestedFix?.match(/in file `([^`]+)`/);

This expects the LLM's suggestedFix to contain the exact pattern in file `path`. If the LLM formats differently (e.g., "in path" or "file: path"), the fix silently fails.

Fix: Use structured output from the tester (file path as a dedicated field) instead of parsing natural language.

---

MED-4: Silent Default LLM Client

File: src/orchestrator/context.ts:167-185

typescript
class DefaultLLMClient implements LLMClient {
  async chat(messages, options?) {
    return {
      content: 'LLM not configured. Please configure a real LLM client.',
      // ...
    };
  }
}

The default LLM client returns a polite string instead of throwing. Agents proceed as if they got a real response, leading to nonsensical downstream outputs.

Fix: Throw a ConfigError so the pipeline fails fast with a clear message.

---

MED-5: Config Merge Bug Drops User Breaker Overrides

File: src/core/config.ts:90

typescript
function mergeConfig(defaults: ForgeConfig, overrides: Partial<ForgeConfig>): ForgeConfig {
  return {
    ...defaults,
    ...overrides,
    safety: {
      ...defaults.safety,
      ...overrides.safety,
      breakers: defaults.safety.breakers, // ← always uses defaults!
    },
  };
}

Impact: Users cannot customize circuit breaker thresholds via config files.

Fix: breakers: { ...defaults.safety.breakers, ...overrides.safety?.breakers }

---

MED-6: Keyword-Only Memory Similarity

File: src/orchestrator/context.ts:200-230

The DefaultMemoryStore.search() uses simple keyword overlap for similarity:

typescript
const words = query.toLowerCase().split(/\s+/);
return this.memories.filter(m =>
  words.some(w => m.content.toLowerCase().includes(w))
);

Impact: "fix authentication bug" matches any memory containing "fix", "authentication", or "bug" — producing noisy results.

Fix: Use TF-IDF scoring or embedding-based similarity when an LLM is available.

---

MED-7: Async Beads Registration Fire-and-Forget

File: src/tools/index.ts

Beads tools are registered asynchronously with no error handling:

typescript
isBeadsAvailable().then(available => {
  if (available) {
    allBeadsTools.forEach(t => registry.register(t));
  }
});

Impact: Race condition — if an agent requests beads tools before the async check completes, they won't be found.

Fix: Make registration synchronous or await it during pipeline initialization.

---

MED-8: Sort Direction Bug in Checkpoint Retrieval

File: src/events/bus.ts:242

typescript
getLatestCheckpoint(): Checkpoint | undefined {
  const rows = this.db.prepare(
    'SELECT * FROM checkpoints ORDER BY timestamp ASC LIMIT 1'
  ).all();
  return rows[0]; // Returns OLDEST, not latest!
}

Fix: Change ASC to DESC.

---

Low Severity Findings

LOW-1: Inconsistent Module Import Paths

Files: Various

Some files use .ts extensions, others use .js:

typescript
// cli/index.ts uses .js
import { loadConfig } from '../core/config.js';

// agents/pi-adapter.ts uses .ts
import { forgeToolsToPiTools } from './pi-tool-converter.ts';

Fix: Standardize on .ts imports (Bun supports them natively).

---

LOW-2: node:child_process Instead of Bun.$

File: src/agents/planner.ts

typescript
import { exec } from 'node:child_process';
const execAsync = promisify(exec);

Per CLAUDE.md: "Use Bun.$ instead of execa" — node:child_process should also be replaced.

Fix: Use Bun.$\command`orBun.spawn()` (as beads.ts does correctly).

---

LOW-3: Mixed ID Generation Strategies

Files: Multiple

Fix: Standardize on ULID (already a dependency) — it's sortable and unique.

---

LOW-4: Deprecated substr() Usage

File: src/cli/index.ts:382

typescript
Math.random().toString(36).substr(2, 5)

substr() is deprecated in favor of substring() or slice().

---

LOW-5: Deployer emit() Is a No-Op

File: src/agents/deployer.ts:666-668

typescript
private async emit(event: string, data: Record<string, unknown>): Promise<void> {
  // Empty — all observability from the deployer is silently dropped
}

Impact: No deployment events reach the EventBus. Monitoring and audit trails are blind to deploy activity.

Fix: Wire to ctx.bus.emit() like other agents do.

---

Summary Table