From 36a3e4ed763448075e5fb695d423cdbff55fac90 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 15 Apr 2026 06:15:52 +0000 Subject: [PATCH] refactor: code quality cleanup pass Combined cleanup covering the 8 quality areas (repo is ~2400 LOC so a single PR keeps the diff reviewable). - dedup: extract loadPromptTemplate, readClaudeMd, getPluginFlag, codexEffortConfig, review model/effort helpers into steps/shared.ts (previously duplicated across plan, execute, pr, review) - dedup: collapse buildRemoteCommandParts and buildLocalChildArgs into a single buildPipelineArgs({ escape }) helper - weak types: replace e.step! / e.label! / e.elapsed! in stats.ts with isStepDone / isStepFailed type guards - ai slop: drop a handful of restating comments in pipeline.ts and pr.ts; keep all WHY-comments (graceful degradation, rebase race, contract header) Assessment doc at CLEANUP_ASSESSMENT.md. No behaviour changes, no public API changes, knip surface preserved (published as source-level contract for oneshot-bot). Gate green: typecheck + 16 tests + build. --- CLEANUP_ASSESSMENT.md | 76 +++++++++++++++++++++++++++++++++++++++++++ src/cli.ts | 34 ++++++++----------- src/pipeline.ts | 1 - src/stats.ts | 18 +++++++--- src/steps/execute.ts | 19 ++++------- src/steps/plan.ts | 22 ++++--------- src/steps/pr.ts | 24 +++----------- src/steps/review.ts | 26 +++------------ src/steps/shared.ts | 27 +++++++++++++++ 9 files changed, 151 insertions(+), 96 deletions(-) create mode 100644 CLEANUP_ASSESSMENT.md create mode 100644 src/steps/shared.ts diff --git a/CLEANUP_ASSESSMENT.md b/CLEANUP_ASSESSMENT.md new file mode 100644 index 0000000..1ee3437 --- /dev/null +++ b/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 diff --git a/src/cli.ts b/src/cli.ts index 47b071f..6ab1c52 100755 --- a/src/cli.ts +++ b/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(` diff --git a/src/pipeline.ts b/src/pipeline.ts index 5dc2ca9..9dc4383 100644 --- a/src/pipeline.ts +++ b/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)); } diff --git a/src/stats.ts b/src/stats.ts index 7fdeb58..077c7c6 100644 --- a/src/stats.ts +++ b/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 diff --git a/src/steps/execute.ts b/src/steps/execute.ts index 4114beb..cd6f353 100644 --- a/src/steps/execute.ts +++ b/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 => { 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 } ); }; diff --git a/src/steps/plan.ts b/src/steps/plan.ts index 2359db2..392b4f7 100644 --- a/src/steps/plan.ts +++ b/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 => { 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 } ); diff --git a/src/steps/pr.ts b/src/steps/pr.ts index 9a18e27..9e34190 100644 --- a/src/steps/pr.ts +++ b/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,7 +22,7 @@ export const createDraftPr = async (ctx: PipelineContext): Promise => { 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); @@ -40,7 +30,7 @@ export const createDraftPr = async (ctx: PipelineContext): Promise => { 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,7 +56,6 @@ export const createDraftPr = async (ctx: PipelineContext): Promise => { export const finalizeAfterReview = async (ctx: PipelineContext): Promise => { 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` @@ -74,7 +63,6 @@ export const finalizeAfterReview = async (ctx: PipelineContext): Promise = 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 = // 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 = ); } - // 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)}`); diff --git a/src/steps/review.ts b/src/steps/review.ts index 41cad3a..e0985bf 100644 --- a/src/steps/review.ts +++ b/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 => { const { options, worktreePath } = ctx; @@ -40,15 +28,12 @@ export const review = async (ctx: PipelineContext): Promise => { const standardReview = async (ctx: PipelineContext): Promise => { 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 => { 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 } ); }; diff --git a/src/steps/shared.ts b/src/steps/shared.ts new file mode 100644 index 0000000..f0ee654 --- /dev/null +++ b/src/steps/shared.ts @@ -0,0 +1,27 @@ +import { readFileSync } from "fs"; +import { join } from "path"; +import type { PipelineContext } from "../config"; +import { exec } from "../exec"; +import { shellEscape } from "../shell"; +import { PROMPTS_DIR, CLAUDE_PLUGIN_DIR } from "../paths"; + +export const loadPromptTemplate = (name: string): string => + readFileSync(join(PROMPTS_DIR, name), "utf-8"); + +export const readClaudeMd = async (worktreePath: string): Promise => { + const result = await exec(`cat "${worktreePath}/CLAUDE.md" 2>/dev/null || echo "No CLAUDE.md found"`); + return result.stdout.trim(); +}; + +/** Trailing-space-delimited `--plugin-dir …` fragment when configured, else "". */ +export const getPluginFlag = (): string => + CLAUDE_PLUGIN_DIR ? `--plugin-dir ${shellEscape(CLAUDE_PLUGIN_DIR)} ` : ""; + +export const codexEffortConfig = (effort: string): string => + `model_reasoning_effort="${effort}"`; + +export const getReviewModel = (ctx: PipelineContext): string => + ctx.config.codex.reviewModel ?? "gpt-5.4-mini"; + +export const getReviewEffort = (ctx: PipelineContext): string => + ctx.config.codex.reviewReasoningEffort ?? "xhigh";