From 3a63a8b205df2e56899124070a5b5d5f8d147177 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Mon, 18 May 2026 12:44:27 +0900 Subject: [PATCH] test(shared): add audit test forbidding direct output.args mutation + fix 9th site Add replace-tool-args.audit.test.ts that scans src/**/*.ts for direct output.args property assignments and Object.assign(output.args, ...) outside the helper. Also fix the 9th mutation site discovered by the audit in compaction-todo-preserver/hook.ts. Add replace-tool-args.test.ts with 12 regression tests covering both mutable and Object.freeze'd output.args scenarios for all hook patterns. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- src/hooks/compaction-todo-preserver/hook.ts | 3 +- src/shared/replace-tool-args.audit.test.ts | 60 ++++++++ src/shared/replace-tool-args.test.ts | 148 ++++++++++++++++++++ 3 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 src/shared/replace-tool-args.audit.test.ts create mode 100644 src/shared/replace-tool-args.test.ts diff --git a/src/hooks/compaction-todo-preserver/hook.ts b/src/hooks/compaction-todo-preserver/hook.ts index 63e744c9c..dd2ea5c82 100644 --- a/src/hooks/compaction-todo-preserver/hook.ts +++ b/src/hooks/compaction-todo-preserver/hook.ts @@ -1,6 +1,7 @@ import type { PluginInput } from "@opencode-ai/plugin" import { resolveSessionEventID } from "../../shared/event-session-id" import { log } from "../../shared/logger" +import { replaceToolArgs } from "../../shared/replace-tool-args" interface TodoSnapshot { id?: string @@ -233,7 +234,7 @@ export function createCompactionTodoPreserverHook( return } - output.args.todos = snapshot + replaceToolArgs(output, { todos: snapshot }) log(`[${HOOK_NAME}] Replaced late Atlas bootstrap todowrite with restored snapshot`, { sessionID: input.sessionID, count: snapshot.length, diff --git a/src/shared/replace-tool-args.audit.test.ts b/src/shared/replace-tool-args.audit.test.ts new file mode 100644 index 000000000..4c14faf34 --- /dev/null +++ b/src/shared/replace-tool-args.audit.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect } from "bun:test" +import { readdir, readFile } from "node:fs/promises" +import { join, relative } from "node:path" + +const SRC_DIR = join(import.meta.dir, "..") + +async function collectTsFiles(dir: string): Promise { + const results: string[] = [] + const entries = await readdir(dir, { withFileTypes: true }) + + for (const entry of entries) { + const fullPath = join(dir, entry.name) + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "dist") continue + results.push(...(await collectTsFiles(fullPath))) + } else if (entry.name.endsWith(".ts") && !entry.name.endsWith(".test.ts") && !entry.name.endsWith(".audit.test.ts")) { + results.push(fullPath) + } + } + + return results +} + +const HELPER_FILE = "shared/replace-tool-args.ts" + +const DIRECT_MUTATION_PATTERN = /output\.args\.\w+\s*=[^=]/g +const OBJECT_ASSIGN_PATTERN = /Object\.assign\(\s*output\.args/g + +describe("replace-tool-args audit", () => { + it("#given src/**/*.ts files #when scanning for direct output.args mutation #then no matches found outside the helper", async () => { + // given + const files = await collectTsFiles(SRC_DIR) + const violations: string[] = [] + + // when + for (const file of files) { + const relPath = relative(SRC_DIR, file) + if (relPath === HELPER_FILE) continue + + const content = await readFile(file, "utf-8") + const lines = content.split("\n") + + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + if (DIRECT_MUTATION_PATTERN.test(line)) { + violations.push(`${relPath}:${i + 1}: ${line.trim()}`) + } + DIRECT_MUTATION_PATTERN.lastIndex = 0 + + if (OBJECT_ASSIGN_PATTERN.test(line)) { + violations.push(`${relPath}:${i + 1}: ${line.trim()}`) + } + OBJECT_ASSIGN_PATTERN.lastIndex = 0 + } + } + + // then + expect(violations).toEqual([]) + }) +}) diff --git a/src/shared/replace-tool-args.test.ts b/src/shared/replace-tool-args.test.ts new file mode 100644 index 000000000..113d8ab65 --- /dev/null +++ b/src/shared/replace-tool-args.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect } from "bun:test" +import { replaceToolArgs } from "./replace-tool-args" + +describe("replaceToolArgs", () => { + describe("#given a mutable output.args object", () => { + it("#when patching a single property #then the output.args contains the patched value", () => { + // given + const output = { args: { command: "git status", timeout: 30 } as Record } + + // when + replaceToolArgs(output, { command: "git log" }) + + // then + expect(output.args.command).toBe("git log") + expect(output.args.timeout).toBe(30) + }) + + it("#when patching multiple properties #then all patched values are present", () => { + // given + const output = { args: { url: "http://old.com", format: "text" } as Record } + + // when + replaceToolArgs(output, { url: "http://new.com", format: "markdown" }) + + // then + expect(output.args.url).toBe("http://new.com") + expect(output.args.format).toBe("markdown") + }) + + it("#when patching #then the original args object is not the same reference", () => { + // given + const originalArgs = { command: "echo hi" } as Record + const output = { args: originalArgs } + + // when + replaceToolArgs(output, { command: "echo bye" }) + + // then + expect(output.args).not.toBe(originalArgs) + expect(originalArgs.command).toBe("echo hi") + }) + }) + + describe("#given a frozen output.args object", () => { + it("#when patching a single property #then no TypeError is thrown and the value is updated", () => { + // given + const output = { args: Object.freeze({ command: "git status", timeout: 30 }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { command: "git log" })).not.toThrow() + expect(output.args.command).toBe("git log") + expect(output.args.timeout).toBe(30) + }) + + it("#when patching with Object-typed value #then no TypeError is thrown", () => { + // given + const output = { args: Object.freeze({ todos: "[]" }) as Record } + const parsed = [{ id: "1", content: "test", status: "pending" }] + + // when / then + expect(() => replaceToolArgs(output, { todos: parsed })).not.toThrow() + expect(output.args.todos).toEqual(parsed) + }) + + it("#when patching url on frozen webfetch args #then no TypeError is thrown", () => { + // given + const output = { args: Object.freeze({ url: "http://old.com", format: "markdown" }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { url: "http://redirected.com" })).not.toThrow() + expect(output.args.url).toBe("http://redirected.com") + expect(output.args.format).toBe("markdown") + }) + + it("#when patching command with env prefix on frozen bash args #then no TypeError is thrown", () => { + // given + const output = { args: Object.freeze({ command: "git rebase --continue" }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { command: "GIT_EDITOR=: git rebase --continue" })).not.toThrow() + expect(output.args.command).toBe("GIT_EDITOR=: git rebase --continue") + }) + + it("#when patching prompt on frozen task args #then no TypeError is thrown", () => { + // given + const output = { args: Object.freeze({ prompt: "Do the thing", category: "quick" }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { prompt: "[DIRECTIVE] Do the thing" })).not.toThrow() + expect(output.args.prompt).toBe("[DIRECTIVE] Do the thing") + expect(output.args.category).toBe("quick") + }) + + it("#when stripping null bytes from frozen command #then no TypeError is thrown", () => { + // given + const frozenCommand = "echo \x00hello" + const output = { args: Object.freeze({ command: frozenCommand }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { command: "echo hello" })).not.toThrow() + expect(output.args.command).toBe("echo hello") + }) + + it("#when replacing truncated question labels on frozen args #then no TypeError is thrown", () => { + // given + const output = { + args: Object.freeze({ + questions: [{ question: "Pick", options: [{ label: "A very long label that should be truncated" }] }], + }) as Record, + } + const truncated = { + questions: [{ question: "Pick", options: [{ label: "A very long label that sho..." }] }], + } + + // when / then + expect(() => replaceToolArgs(output, truncated)).not.toThrow() + expect((output.args.questions as Array<{ options: Array<{ label: string }> }>)[0].options[0].label).toBe( + "A very long label that sho...", + ) + }) + + it("#when replacing modifiedInput from PreToolUse hook on frozen args #then no TypeError is thrown", () => { + // given + const output = { args: Object.freeze({ filePath: "/old/path.ts" }) as Record } + + // when / then + expect(() => replaceToolArgs(output, { filePath: "/new/path.ts" })).not.toThrow() + expect(output.args.filePath).toBe("/new/path.ts") + }) + + it("#when replacing todo snapshot on frozen args #then no TypeError is thrown", () => { + // given + const output = { + args: Object.freeze({ + todos: [{ content: "bootstrap", status: "pending" }], + }) as Record, + } + const snapshot = [ + { content: "Real task 1", status: "in_progress" }, + { content: "Real task 2", status: "pending" }, + ] + + // when / then + expect(() => replaceToolArgs(output, { todos: snapshot })).not.toThrow() + expect(output.args.todos).toEqual(snapshot) + }) + }) +})