refactor(tools): fix empty catches, remove AI slop from code comments

This commit is contained in:
YeonGyu-Kim
2026-04-03 19:17:09 +09:00
parent 3689ecd5b0
commit 22283fca6d
10 changed files with 38 additions and 35 deletions
@@ -141,8 +141,6 @@ Create the work plan directly - that's your job as the planning agent.`,
categoryModel = variantToUse ? { ...normalized, variant: variantToUse } : normalized categoryModel = variantToUse ? { ...normalized, variant: variantToUse } : normalized
} }
} else if (resolutionSkipped && (agentOverride?.model ?? agentCategoryModel)) { } else if (resolutionSkipped && (agentOverride?.model ?? agentCategoryModel)) {
// Cold cache: resolution was skipped but user explicitly configured a model.
// Honor the user override directly — don't fall through to hardcoded fallback chain.
const normalized = normalizeModelFormat((agentOverride?.model ?? agentCategoryModel)!) const normalized = normalizeModelFormat((agentOverride?.model ?? agentCategoryModel)!)
if (normalized) { if (normalized) {
const agentCategoryVariant = agentOverride?.category const agentCategoryVariant = agentOverride?.category
@@ -164,8 +162,6 @@ Create the work plan directly - that's your job as the planning agent.`,
normalizedAgentFallbackModels, normalizedAgentFallbackModels,
defaultProviderID, defaultProviderID,
) )
// Don't assign hardcoded fallback chain when resolution was skipped (cold cache)
// — the chain may contain model IDs that don't exist in the provider yet.
fallbackChain = configuredFallbackChain ?? (resolutionSkipped ? undefined : agentRequirement?.fallbackChain) fallbackChain = configuredFallbackChain ?? (resolutionSkipped ? undefined : agentRequirement?.fallbackChain)
// Only promote fallback-only settings when resolution actually selected a fallback model. // Only promote fallback-only settings when resolution actually selected a fallback model.
@@ -227,10 +227,10 @@ describe("hashline edit operations", () => {
}) })
it("preserves blank lines and indentation in range replace (no false unwrap)", () => { it("preserves blank lines and indentation in range replace (no false unwrap)", () => {
//#given reproduces the 애국가 bug where blank+indented lines collapse //#given, reproduces the 애국가 bug where blank+indented lines collapse
const lines = ["", "동해물과 백두산이 마르고 닳도록", "하느님이 보우하사 우리나라 만세", "", "무궁화 삼천리 화려강산", "대한사람 대한으로 길이 보전하세", ""] const lines = ["", "동해물과 백두산이 마르고 닳도록", "하느님이 보우하사 우리나라 만세", "", "무궁화 삼천리 화려강산", "대한사람 대한으로 길이 보전하세", ""]
//#when replace the range with indented version (blank lines preserved) //#when, replace the range with indented version (blank lines preserved)
const result = applyReplaceLines( const result = applyReplaceLines(
lines, lines,
anchorFor(lines, 1), anchorFor(lines, 1),
@@ -238,7 +238,7 @@ describe("hashline edit operations", () => {
["", " 동해물과 백두산이 마르고 닳도록", " 하느님이 보우하사 우리나라 만세", "", " 무궁화 삼천리 화려강산", " 대한사람 대한으로 길이 보전하세", ""] ["", " 동해물과 백두산이 마르고 닳도록", " 하느님이 보우하사 우리나라 만세", "", " 무궁화 삼천리 화려강산", " 대한사람 대한으로 길이 보전하세", ""]
) )
//#then all 7 lines preserved with indentation, not collapsed to 3 //#then, all 7 lines preserved with indentation, not collapsed to 3
expect(result).toEqual(["", " 동해물과 백두산이 마르고 닳도록", " 하느님이 보우하사 우리나라 만세", "", " 무궁화 삼천리 화려강산", " 대한사람 대한으로 길이 보전하세", ""]) expect(result).toEqual(["", " 동해물과 백두산이 마르고 닳도록", " 하느님이 보우하사 우리나라 만세", "", " 무궁화 삼천리 화려강산", " 대한사람 대한으로 길이 보전하세", ""])
}) })
@@ -350,10 +350,10 @@ describe("runFormattersForFile", () => {
}, },
}) })
//#when run for a .go file, but only .ts formatters registered //#when, run for a .go file, but only .ts formatters registered
await runFormattersForFile(client, "/project", "/src/main.go") await runFormattersForFile(client, "/project", "/src/main.go")
//#then no error thrown //#then, no error thrown
}) })
it("runs formatter for matching extension", async () => { it("runs formatter for matching extension", async () => {
@@ -367,10 +367,10 @@ describe("runFormattersForFile", () => {
}, },
}) })
//#when echo is a safe no-op command //#when, echo is a safe no-op command
await runFormattersForFile(client, "/tmp", "/tmp/test.ts") await runFormattersForFile(client, "/tmp", "/tmp/test.ts")
//#then should complete without error //#then, should complete without error
expect(client.config.get).toHaveBeenCalledTimes(1) expect(client.config.get).toHaveBeenCalledTimes(1)
}) })
}) })
+6 -6
View File
@@ -23,10 +23,10 @@ describe("parseLineRef", () => {
}) })
it("gives specific hint when literal text is used instead of line number", () => { it("gives specific hint when literal text is used instead of line number", () => {
//#given model sends "LINE#HK" instead of "1#HK" //#given, model sends "LINE#HK" instead of "1#HK"
const ref = "LINE#HK" const ref = "LINE#HK"
//#when / #then error should mention that LINE is not a valid number //#when / #then, error should mention that LINE is not a valid number
expect(() => parseLineRef(ref)).toThrow(/not a line number/i) expect(() => parseLineRef(ref)).toThrow(/not a line number/i)
}) })
@@ -39,10 +39,10 @@ describe("parseLineRef", () => {
}) })
it("extracts valid line number from mixed prefix like LINE42 without throwing", () => { it("extracts valid line number from mixed prefix like LINE42 without throwing", () => {
//#given normalizeLineRef extracts 42#VK from LINE42#VK //#given, normalizeLineRef extracts 42#VK from LINE42#VK
const ref = "LINE42#VK" const ref = "LINE42#VK"
//#when / #then should parse successfully as line 42 //#when / #then, should parse successfully as line 42
const result = parseLineRef(ref) const result = parseLineRef(ref)
expect(result.line).toBe(42) expect(result.line).toBe(42)
expect(result.hash).toBe("VK") expect(result.hash).toBe("VK")
@@ -144,11 +144,11 @@ describe("validateLineRef", () => {
}) })
it("suggests correct line number when hash matches a file line", () => { it("suggests correct line number when hash matches a file line", () => {
//#given model sends LINE#XX where XX is the actual hash for line 1 //#given, model sends LINE#XX where XX is the actual hash for line 1
const lines = ["function hello() {", " return 42", "}"] const lines = ["function hello() {", " return 42", "}"]
const hash = computeLineHash(1, lines[0]) const hash = computeLineHash(1, lines[0])
//#when / #then error should suggest the correct reference //#when / #then, error should suggest the correct reference
expect(() => validateLineRefs(lines, [`LINE#${hash}`])).toThrow(new RegExp(`1#${hash}`)) expect(() => validateLineRefs(lines, [`LINE#${hash}`])).toThrow(new RegExp(`1#${hash}`))
}) })
}) })
-1
View File
@@ -48,7 +48,6 @@ export function parseLineRef(ref: string): LineRef {
hash: match[2], hash: match[2],
} }
} }
// normalized equals ref.trim() in all error paths — extraction only succeeds for valid refs
const hashIdx = normalized.indexOf('#') const hashIdx = normalized.indexOf('#')
if (hashIdx > 0) { if (hashIdx > 0) {
const prefix = normalized.slice(0, hashIdx) const prefix = normalized.slice(0, hashIdx)
+1 -2
View File
@@ -20,8 +20,7 @@ describe("isServerInstalled", () => {
afterEach(() => { afterEach(() => {
try { try {
rmSync(tempDir, { recursive: true, force: true }) rmSync(tempDir, { recursive: true, force: true })
} catch (e) { } catch {
// cleanup failed — ignored
} }
if (process.platform === "win32") { if (process.platform === "win32") {
+20 -8
View File
@@ -1,3 +1,5 @@
import { log } from "../../shared/logger"
type ManagedClientForCleanup = { type ManagedClientForCleanup = {
client: { client: {
stop: () => Promise<void>; stop: () => Promise<void>;
@@ -22,23 +24,32 @@ export type LspProcessCleanupHandle = {
export function registerLspManagerProcessCleanup(options: ProcessCleanupOptions): LspProcessCleanupHandle { export function registerLspManagerProcessCleanup(options: ProcessCleanupOptions): LspProcessCleanupHandle {
const handlers: RegisteredHandler[] = []; const handlers: RegisteredHandler[] = [];
// Synchronous cleanup for 'exit' event (cannot await) const logCleanupError = (phase: string, error: unknown): void => {
log(`[lsp-manager-process-cleanup] ${phase}`, {
error: error instanceof Error ? error.message : String(error),
});
};
const syncCleanup = () => { const syncCleanup = () => {
for (const [, managed] of options.getClients()) { for (const [, managed] of options.getClients()) {
try { try {
// Fire-and-forget during sync exit - process is terminating void managed.client.stop().catch((error) => {
void managed.client.stop().catch(() => {}); logCleanupError("stop failed during exit cleanup", error);
} catch {} });
} catch (error) {
logCleanupError("failed to schedule exit cleanup", error);
}
} }
options.clearClients(); options.clearClients();
options.clearCleanupInterval(); options.clearCleanupInterval();
}; };
// Async cleanup for signal handlers - properly await all stops
const asyncCleanup = async () => { const asyncCleanup = async () => {
const stopPromises: Promise<void>[] = []; const stopPromises: Promise<void>[] = [];
for (const [, managed] of options.getClients()) { for (const [, managed] of options.getClients()) {
stopPromises.push(managed.client.stop().catch(() => {})); stopPromises.push(managed.client.stop().catch((error) => {
logCleanupError("stop failed during signal cleanup", error);
}));
} }
await Promise.allSettled(stopPromises); await Promise.allSettled(stopPromises);
options.clearClients(); options.clearClients();
@@ -52,8 +63,9 @@ export function registerLspManagerProcessCleanup(options: ProcessCleanupOptions)
registerHandler("exit", syncCleanup); registerHandler("exit", syncCleanup);
// Don't call process.exit() here; other handlers (background-agent manager) handle final exit. const signalCleanup = () => void asyncCleanup().catch((error) => {
const signalCleanup = () => void asyncCleanup().catch(() => {}); logCleanupError("signal cleanup failed", error);
});
registerHandler("SIGINT", signalCleanup); registerHandler("SIGINT", signalCleanup);
registerHandler("SIGTERM", signalCleanup); registerHandler("SIGTERM", signalCleanup);
if (process.platform === "win32") { if (process.platform === "win32") {
-3
View File
@@ -2,11 +2,9 @@ import { spawn as bunSpawn } from "bun"
import { spawn as nodeSpawn, type ChildProcess } from "node:child_process" import { spawn as nodeSpawn, type ChildProcess } from "node:child_process"
import { existsSync, statSync } from "fs" import { existsSync, statSync } from "fs"
import { log } from "../../shared/logger" import { log } from "../../shared/logger"
// Bun spawn segfaults on Windows (oven-sh/bun#25798) — unfixed as of v1.3.8+
function shouldUseNodeSpawn(): boolean { function shouldUseNodeSpawn(): boolean {
return process.platform === "win32" return process.platform === "win32"
} }
// Prevents segfaults when libuv gets a non-existent cwd (oven-sh/bun#25798)
export function validateCwd(cwd: string): { valid: boolean; error?: string } { export function validateCwd(cwd: string): { valid: boolean; error?: string } {
try { try {
if (!existsSync(cwd)) { if (!existsSync(cwd)) {
@@ -24,7 +22,6 @@ export function validateCwd(cwd: string): { valid: boolean; error?: string } {
interface StreamReader { interface StreamReader {
read(): Promise<{ done: boolean; value: Uint8Array | undefined }> read(): Promise<{ done: boolean; value: Uint8Array | undefined }>
} }
// Bridges Bun Subprocess and Node.js ChildProcess under a common API
export interface UnifiedProcess { export interface UnifiedProcess {
stdin: { write(chunk: Uint8Array | string): void } stdin: { write(chunk: Uint8Array | string): void }
stdout: { getReader(): StreamReader } stdout: { getReader(): StreamReader }
+3 -3
View File
@@ -732,14 +732,14 @@ describe("skill tool - short name resolution", () => {
] ]
const tool = createSkillTool({ skills: loadedSkills }) const tool = createSkillTool({ skills: loadedSkills })
// when / then should not resolve (ambiguous), should suggest both // when / then, should not resolve (ambiguous), should suggest both
await expect(tool.execute({ name: "debugging" }, mockContext)).rejects.toThrow( await expect(tool.execute({ name: "debugging" }, mockContext)).rejects.toThrow(
"not found" "not found"
) )
}) })
it("prefers exact match over short name match", async () => { it("prefers exact match over short name match", async () => {
// given "debugging" exists as both exact and as part of a namespace // given, "debugging" exists as both exact and as part of a namespace
const loadedSkills = [ const loadedSkills = [
createMockSkill("debugging"), createMockSkill("debugging"),
createMockSkill("superpowers/debugging"), createMockSkill("superpowers/debugging"),
@@ -749,7 +749,7 @@ describe("skill tool - short name resolution", () => {
// when // when
const result = await tool.execute({ name: "debugging" }, mockContext) const result = await tool.execute({ name: "debugging" }, mockContext)
// then should match "debugging" exactly, not "superpowers/debugging" // then, should match "debugging" exactly, not "superpowers/debugging"
expect(result).toContain("## Skill: debugging") expect(result).toContain("## Skill: debugging")
}) })
}) })
+1 -1
View File
@@ -535,7 +535,7 @@ describe("syncAllTasksToTodos", () => {
// when // when
await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer); await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer);
// then no duplicates // then, no duplicates
const matching = writtenTodos.filter((t: TodoInfo) => t.content === "Task 1 (updated)"); const matching = writtenTodos.filter((t: TodoInfo) => t.content === "Task 1 (updated)");
expect(matching.length).toBe(1); expect(matching.length).toBe(1);
expect(matching[0].status).toBe("in_progress"); expect(matching[0].status).toBe("in_progress");