refactor: code quality cleanup pass

CLEANUP_ASSESSMENT.md

@@ -0,0 +1,76 @@
+# Code Quality Cleanup Assessment
+
+Scope: oneshot-cli (~2400 LOC, single-binary Bun CLI). One combined PR covering all 8 areas because of repo size.
+
+## 1. Dedup / DRY
+
+Applied (high confidence):
+
+- New `src/steps/shared.ts` extracts: `loadPromptTemplate`, `readClaudeMd`, `getPluginFlag`, `codexEffortConfig`, review model + effort helpers. Previously duplicated across `plan.ts`, `execute.ts`, `pr.ts`, `review.ts`.
+- Unified CLI arg builder: `buildRemoteCommandParts` and `buildLocalChildArgs` were near-identical (differ only in whether each value is shell-escaped). Collapsed to a single `buildPipelineArgs({ escape })` helper.
+
+Skipped:
+
+- `formatTime` exists in both `log.ts` and `stats.ts` but formats differently (ms vs seconds). Intentional.
+- ANSI color constants in `log.ts` and `stats.ts` — below the 3-line threshold and stats is the only other consumer.
+
+## 2. Type consolidation
+
+No duplicated type definitions. No action.
+
+## 3. Dead code
+
+Ran `knip`. Reviewed flagged items:
+
+- `ONESHOT_STEPS`, `OneshotStep`, `getStepShort`: the contract file `pipeline-steps.ts` is the documented source-of-truth that `oneshot-bot/src/services/oneshot-contract.ts` mirrors. KEEP.
+- Event type exports (`StartedEvent`, `StepEvent`, `CompletedEvent`, `ClassifiedEvent`, `OneshotEvent`): JSONL wire contract — the bot parses these files. Package publishes `src/**/*.ts` so these are public surface. KEEP.
+- `ExecResult`, `ErrorCode`, `StepTimeoutKey`: published helper types. KEEP.
+- `docs/assets/script.js`: static docs site asset. KEEP.
+
+No action — all flagged exports are load-bearing for the shipped source-published contract.
+
+## 4. Circular dependencies
+
+`madge --circular src/cli.ts` → none. No action.
+
+## 5. Weak types
+
+- Zero `any`, `as any`, `@ts-ignore`, or `@ts-expect-error` in source. Strict mode already on.
+- `stats.ts` had `e.step!`, `e.label!`, `e.elapsed!` after a predicate filter. Replaced with a proper type guard (`hasStepDetails`) so non-null assertions are gone and the narrowed type is visible.
+
+## 6. Defensive try/catch
+
+Reviewed every catch. All remaining ones are legitimate boundaries:
+
+- `pipeline.ts` history read/write, worktree teardown, Linear update, diffstat — best-effort, non-fatal.
+- `events.ts` file writes — warns once per path, suppresses thereafter.
+- `cli.ts` top-level CLI error handler.
+- `exec.ts` `killProcessTree` — process already dead.
+- `config.ts` JSON parse — wraps with path context.
+- `classify.ts` "fail safe" to `deep` — documented fallback when the haiku call fails.
+- `version.ts` package.json read — returns `"0.0.0"` rather than crashing the CLI at import time.
+- `stats.ts` — tolerates corrupted `/tmp` files.
+- `pr.ts` `getDiffStats` — display-only, non-fatal.
+
+No action.
+
+## 7. Legacy / deprecated
+
+None. No action.
+
+## 8. AI slop / comments
+
+Trimmed a few restating comments:
+
+- `pipeline.ts`: "Push review fixes (if any) and mark PR as ready" (restates code).
+- `pipeline.ts`: "Stage and commit the review fixes" (restates).
+- `pr.ts`: "Check if review made any changes" (restates).
+- `pr.ts`: "Mark the PR as ready (remove draft status)" (restates).
+
+Kept every real WHY comment: graceful-degradation note, rebase-race commentary, pipeline-steps contract header, plugin-dir JSDoc.
+
+## Verification
+
+- `bun run typecheck` — clean
+- `bun test` — 16 pass
+- `bun run build` — succeeds

src/cli.ts

@@ -110,19 +110,23 @@ export const parseArgs = (args: string[]): ParsedArgs => {
   };
 };
 
-export const buildRemoteCommandParts = (parsed: ParsedArgs): string[] => {
-  const parts = ["--local", shellEscape(parsed.repo)];
-  if (parsed.task) parts.push(shellEscape(parsed.task));
-  if (parsed.model) parts.push("--model", shellEscape(parsed.model));
-  if (parsed.branch) parts.push("--branch", shellEscape(parsed.branch));
-  if (parsed.basePath) parts.push("--base-path", shellEscape(parsed.basePath));
-  if (parsed.mode) parts.push("--mode", shellEscape(parsed.mode));
-  if (parsed.eventsFile) parts.push("--events-file", shellEscape(parsed.eventsFile));
+const buildPipelineArgs = (parsed: ParsedArgs, escape: boolean): string[] => {
+  const quote = (value: string) => (escape ? shellEscape(value) : value);
+  const parts = ["--local", quote(parsed.repo)];
+  if (parsed.task) parts.push(quote(parsed.task));
+  if (parsed.model) parts.push("--model", quote(parsed.model));
+  if (parsed.branch) parts.push("--branch", quote(parsed.branch));
+  if (parsed.basePath) parts.push("--base-path", quote(parsed.basePath));
+  if (parsed.mode) parts.push("--mode", quote(parsed.mode));
+  if (parsed.eventsFile) parts.push("--events-file", quote(parsed.eventsFile));
   if (parsed.dryRun) parts.push("--dry-run");
   if (parsed.deepReview) parts.push("--deep-review");
   return parts;
 };
 
+export const buildRemoteCommandParts = (parsed: ParsedArgs): string[] =>
+  buildPipelineArgs(parsed, true);
+
 const REMOTE_ONESHOT_BIN_SETUP = [
   'oneshot_bin="${ONESHOT_BIN:-}"',
   'if [ -z "$oneshot_bin" ]; then oneshot_bin="$(command -v oneshot 2>/dev/null || true)"; fi',
@@ -163,18 +167,8 @@ const writeRemoteConfig = (proc: Bun.Subprocess<"pipe", "inherit" | "pipe", "inh
   proc.stdin.end();
 };
 
-export const buildLocalChildArgs = (parsed: ParsedArgs): string[] => {
-  const args = ["--local", parsed.repo];
-  if (parsed.task) args.push(parsed.task);
-  if (parsed.model) args.push("--model", parsed.model);
-  if (parsed.branch) args.push("--branch", parsed.branch);
-  if (parsed.basePath) args.push("--base-path", parsed.basePath);
-  if (parsed.mode) args.push("--mode", parsed.mode);
-  if (parsed.eventsFile) args.push("--events-file", parsed.eventsFile);
-  if (parsed.dryRun) args.push("--dry-run");
-  if (parsed.deepReview) args.push("--deep-review");
-  return args;
-};
+export const buildLocalChildArgs = (parsed: ParsedArgs): string[] =>
+  buildPipelineArgs(parsed, false);
 
 const printUsage = () => {
   console.log(`

src/pipeline.ts

@@ -143,7 +143,6 @@ export const runPipeline = async (config: OneshotConfig, options: OneshotOptions
       }
     }
 
-    // Push review fixes (if any) and mark PR as ready
     if (shouldFinalizePr) {
       await runStep(8, events, stepTimings, () => finalizeAfterReview(ctx));
     }

src/stats.ts

@@ -45,6 +45,16 @@ interface EventLine {
   filesChanged?: number;
 }
 
+type StepEventLine = EventLine & { step: number; label: string; elapsed: number };
+
+const isStepDone = (e: EventLine): e is StepEventLine =>
+  e.type === "step" && e.status === "done" && e.step != null && e.label != null && e.elapsed != null;
+
+type FailedStepLine = EventLine & { step: number; label: string };
+
+const isStepFailed = (e: EventLine): e is FailedStepLine =>
+  e.type === "step" && e.status === "failed" && e.step != null && e.label != null;
+
 const formatTime = (ms: number): string => {
   const seconds = Math.round(ms / 1000);
   if (seconds < 60) return `${seconds}s`;
@@ -100,10 +110,10 @@ function scanRecentRuns(): CompletedRun[] {
                 : "unknown";
 
         const stepTimings = events
-          .filter(e => e.type === "step" && e.status === "done" && e.step != null && e.label != null && e.elapsed != null)
-          .map(e => ({ step: e.step!, label: e.label!, elapsed: e.elapsed! }));
+          .filter(isStepDone)
+          .map(e => ({ step: e.step, label: e.label, elapsed: e.elapsed }));
 
-        const failed = events.find(e => e.type === "step" && e.status === "failed" && e.step != null && e.label != null);
+        const failed = events.find(isStepFailed);
 
         runs.push({
           runId: started?.runId ?? "unknown",
@@ -115,7 +125,7 @@ function scanRecentRuns(): CompletedRun[] {
           elapsed: completed.elapsed ?? 0,
           timestamp: completed.timestamp ?? file.mtime,
           stepTimings,
-          failedStep: failed ? { step: failed.step!, label: failed.label! } : undefined,
+          failedStep: failed ? { step: failed.step, label: failed.label } : undefined,
         });
       } catch {
         // skip corrupted files

src/steps/execute.ts

@@ -1,30 +1,23 @@
-import { readFileSync } from "fs";
-import { join } from "path";
 import type { PipelineContext } from "../config";
-import { exec, execOrThrow } from "../exec";
+import { execOrThrow } from "../exec";
 import { getStepTimeout } from "../config";
 import { shellEscape } from "../shell";
-import { PROMPTS_DIR } from "../paths";
-
-const loadPromptTemplate = (): string => {
-  return readFileSync(join(PROMPTS_DIR, "execute.txt"), "utf-8");
-};
+import { codexEffortConfig, loadPromptTemplate, readClaudeMd } from "./shared";
 
 export const execute = async (ctx: PipelineContext): Promise<void> => {
   const { config, options, worktreePath } = ctx;
 
-  const claudeMd = await exec(`cat "${worktreePath}/CLAUDE.md" 2>/dev/null || echo "No CLAUDE.md found"`);
+  const claudeMd = await readClaudeMd(worktreePath);
 
-  const prompt = loadPromptTemplate()
+  const prompt = loadPromptTemplate("execute.txt")
     .replace("{{task}}", options.task)
     .replace("{{plan}}", ctx.plan)
-    .replace("{{claudeMd}}", claudeMd.stdout.trim());
+    .replace("{{claudeMd}}", claudeMd);
 
   const timeoutMs = getStepTimeout(config, "executeMinutes");
-  const effortConfig = `model_reasoning_effort="${config.codex.reasoningEffort}"`;
 
   await execOrThrow(
-    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(config.codex.model)} -c ${shellEscape(effortConfig)}`,
+    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(config.codex.model)} -c ${shellEscape(codexEffortConfig(config.codex.reasoningEffort))}`,
     { timeoutMs, stream: true }
   );
 };

src/steps/plan.ts

@@ -1,33 +1,23 @@
-import { readFileSync } from "fs";
-import { join } from "path";
 import type { PipelineContext } from "../config";
-import { exec, execOrThrow } from "../exec";
+import { execOrThrow } from "../exec";
 import { getStepTimeout } from "../config";
 import { shellEscape } from "../shell";
-import { PROMPTS_DIR, CLAUDE_PLUGIN_DIR } from "../paths";
-
-const loadPromptTemplate = (): string => {
-  return readFileSync(join(PROMPTS_DIR, "plan.txt"), "utf-8");
-};
-
-const pluginFlag = CLAUDE_PLUGIN_DIR
-  ? `--plugin-dir ${shellEscape(CLAUDE_PLUGIN_DIR)} `
-  : "";
+import { getPluginFlag, loadPromptTemplate, readClaudeMd } from "./shared";
 
 export const plan = async (ctx: PipelineContext): Promise<string> => {
   const { config, options, worktreePath } = ctx;
 
-  const claudeMd = await exec(`cat "${worktreePath}/CLAUDE.md" 2>/dev/null || echo "No CLAUDE.md found"`);
+  const claudeMd = await readClaudeMd(worktreePath);
 
-  const prompt = loadPromptTemplate()
+  const prompt = loadPromptTemplate("plan.txt")
     .replace("{{task}}", options.task)
-    .replace("{{claudeMd}}", claudeMd.stdout.trim());
+    .replace("{{claudeMd}}", claudeMd);
 
   const model = options.model ?? config.claude.model;
   const timeoutMs = getStepTimeout(config, "planMinutes");
 
   const result = await execOrThrow(
-    `cd ${shellEscape(worktreePath)} && claude -p ${shellEscape(prompt)} ${pluginFlag}--model ${shellEscape(model)} --no-session-persistence`,
+    `cd ${shellEscape(worktreePath)} && claude -p ${shellEscape(prompt)} ${getPluginFlag()}--model ${shellEscape(model)} --no-session-persistence`,
     { timeoutMs, stream: true }
   );
 

src/steps/pr.ts

@@ -1,18 +1,8 @@
-import { readFileSync } from "fs";
-import { join } from "path";
 import type { PipelineContext } from "../config";
 import { execOrThrow, exec, OneshotError } from "../exec";
 import { getStepTimeout } from "../config";
 import { shellEscape } from "../shell";
-import { PROMPTS_DIR, CLAUDE_PLUGIN_DIR } from "../paths";
-
-const pluginFlag = CLAUDE_PLUGIN_DIR
-  ? `--plugin-dir ${shellEscape(CLAUDE_PLUGIN_DIR)} `
-  : "";
-
-const loadPromptTemplate = (): string => {
-  return readFileSync(join(PROMPTS_DIR, "pr.txt"), "utf-8");
-};
+import { getPluginFlag, loadPromptTemplate } from "./shared";
 
 export const getPrModel = (ctx: PipelineContext): string =>
   ctx.options.model ?? ctx.config.claude.model;
@@ -32,15 +22,15 @@ export const createDraftPr = async (ctx: PipelineContext): Promise<string> => {
   const taskSummary = options.taskSummary ?? options.task;
 
   const model = getPrModel(ctx);
-  const prompt = loadPromptTemplate()
+  const prompt = loadPromptTemplate("pr.txt")
     .replace("{{task}}", taskSummary)
     .replace("{{branchName}}", branchName)
     .replace(/\{\{baseBranch\}\}/g, baseBranch);
 
   const timeoutMs = getStepTimeout(config, "prMinutes");
 
   const result = await execOrThrow(
-    `cd ${shellEscape(worktreePath)} && claude -p ${shellEscape(prompt)} ${pluginFlag}--dangerously-skip-permissions --model ${shellEscape(model)} --no-session-persistence`,
+    `cd ${shellEscape(worktreePath)} && claude -p ${shellEscape(prompt)} ${getPluginFlag()}--dangerously-skip-permissions --model ${shellEscape(model)} --no-session-persistence`,
     { timeoutMs, stream: true }
   );
 
@@ -66,15 +56,13 @@ export const createDraftPr = async (ctx: PipelineContext): Promise<string> => {
 export const finalizeAfterReview = async (ctx: PipelineContext): Promise<void> => {
   const { worktreePath, prUrl } = ctx;
 
-  // Check if review made any changes
   const diffCheck = await exec(`cd ${shellEscape(worktreePath)} && git diff --stat`);
   const untracked = await exec(
     `cd ${shellEscape(worktreePath)} && git ls-files --others --exclude-standard`
   );
   const hasChanges = !!(diffCheck.stdout.trim() || untracked.stdout.trim());
 
   if (hasChanges) {
-    // Stage and commit the review fixes
     await execOrThrow(`cd ${shellEscape(worktreePath)} && git add -A`);
     await execOrThrow(`cd ${shellEscape(worktreePath)} && git commit -m "fix: address review findings"`);
 
@@ -116,10 +104,7 @@ export const finalizeAfterReview = async (ctx: PipelineContext): Promise<void> =
         // Abort so the worktree is clean for teardown. Swallow any failure
         // from the abort itself — if there's nothing to abort git will just
         // complain and we don't want to mask the original rebase error.
-        const abortResult = await exec(
-          `cd ${shellEscape(worktreePath)} && git rebase --abort`
-        );
-        void abortResult;
+        await exec(`cd ${shellEscape(worktreePath)} && git rebase --abort`);
         const detail = rebaseErr instanceof Error ? rebaseErr.message : String(rebaseErr);
         throw new OneshotError(
           `review fix commit could not be rebased onto ${branchName} — another process pushed conflicting changes while the review was running. PR left in draft.`,
@@ -136,7 +121,6 @@ export const finalizeAfterReview = async (ctx: PipelineContext): Promise<void> =
     );
   }
 
-  // Mark the PR as ready (remove draft status)
   const prNumber = prUrl.match(/\/pull\/(\d+)/)?.[1];
   if (!prNumber) throw new Error(`could not extract PR number from URL: ${prUrl}`);
   await execOrThrow(`cd ${shellEscape(worktreePath)} && gh pr ready ${shellEscape(prNumber)}`);

src/steps/review.ts

@@ -1,20 +1,8 @@
-import { readFileSync } from "fs";
-import { join } from "path";
 import type { PipelineContext } from "../config";
 import { execOrThrow, OneshotError } from "../exec";
 import { getStepTimeout } from "../config";
 import { shellEscape } from "../shell";
-import { PROMPTS_DIR } from "../paths";
-
-const loadPromptTemplate = (): string => {
-  return readFileSync(join(PROMPTS_DIR, "review.txt"), "utf-8");
-};
-
-const getReviewModel = (ctx: PipelineContext): string =>
-  ctx.config.codex.reviewModel ?? "gpt-5.4-mini";
-
-const getReviewEffort = (ctx: PipelineContext): string =>
-  ctx.config.codex.reviewReasoningEffort ?? "xhigh";
+import { codexEffortConfig, getReviewEffort, getReviewModel, loadPromptTemplate } from "./shared";
 
 export const review = async (ctx: PipelineContext): Promise<void> => {
   const { options, worktreePath } = ctx;
@@ -40,15 +28,12 @@ export const review = async (ctx: PipelineContext): Promise<void> => {
 const standardReview = async (ctx: PipelineContext): Promise<void> => {
   const { config, worktreePath, options } = ctx;
   const baseBranch = options.branch ?? "main";
-  const prompt = loadPromptTemplate()
+  const prompt = loadPromptTemplate("review.txt")
     .replace("{{task}}", options.task)
     .replace(/\{\{baseBranch\}\}/g, baseBranch);
   const timeoutMs = getStepTimeout(config, "reviewMinutes");
-  const model = getReviewModel(ctx);
-  const effort = getReviewEffort(ctx);
-  const effortConfig = `model_reasoning_effort="${effort}"`;
   await execOrThrow(
-    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(model)} -c ${shellEscape(effortConfig)}`,
+    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(getReviewModel(ctx))} -c ${shellEscape(codexEffortConfig(getReviewEffort(ctx)))}`,
     { timeoutMs, stream: true }
   );
 };
@@ -57,8 +42,6 @@ const deepReview = async (ctx: PipelineContext): Promise<void> => {
   const { config, worktreePath, options } = ctx;
   const baseBranch = options.branch ?? "main";
   const timeoutMs = getStepTimeout(config, "deepReviewMinutes");
-  const model = getReviewModel(ctx);
-  const effort = getReviewEffort(ctx);
 
   const prompt = `You are reviewing code changes for a task.
 
@@ -72,9 +55,8 @@ Review ALL changes in this branch against origin/${baseBranch} (use git diff ori
 
 Fix any issues directly. Run typecheck and build to verify. Do NOT create commits.`;
 
-  const effortConfig = `model_reasoning_effort="${effort}"`;
   await execOrThrow(
-    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(model)} -c ${shellEscape(effortConfig)}`,
+    `cd ${shellEscape(worktreePath)} && codex exec ${shellEscape(prompt)} --dangerously-bypass-approvals-and-sandbox -m ${shellEscape(getReviewModel(ctx))} -c ${shellEscape(codexEffortConfig(getReviewEffort(ctx)))}`,
     { timeoutMs, stream: true }
   );
 };