diff --git a/src/hooks/atlas/index.test.ts b/src/hooks/atlas/index.test.ts index cd01eb71ae..aa28480cef 100644 --- a/src/hooks/atlas/index.test.ts +++ b/src/hooks/atlas/index.test.ts @@ -748,6 +748,81 @@ describe("atlas hook", () => { }) }) }) + + describe("Plan file transformation with autoCommit", () => { + const ORCHESTRATOR_SESSION = "orchestrator-plan-transform-test" + + beforeEach(() => { + setupMessageStorage(ORCHESTRATOR_SESSION, "atlas") + }) + + afterEach(() => { + cleanupMessageStorage(ORCHESTRATOR_SESSION) + }) + + test("should transform Commit fields when autoCommit is false and reading plan file", async () => { + // given + const hook = createAtlasHook(createMockPluginInput(), { directory: TEST_DIR, autoCommit: false }) + const planContent = "# Plan\n\nCommit: YES\n\n- [ ] Task 1" + const output = { + title: "Read", + output: planContent, + metadata: { filePath: "/project/.sisyphus/plans/test-plan.md" }, + } + + // when + await hook["tool.execute.after"]( + { tool: "read", sessionID: ORCHESTRATOR_SESSION }, + output + ) + + // then + expect(output.output).toContain("Commit: NO (user disabled auto-commits)") + expect(output.output).not.toContain("Commit: YES") + }) + + test("should NOT transform Commit fields when autoCommit is true", async () => { + // given + const hook = createAtlasHook(createMockPluginInput(), { directory: TEST_DIR, autoCommit: true }) + const planContent = "# Plan\n\nCommit: YES\n\n- [ ] Task 1" + const output = { + title: "Read", + output: planContent, + metadata: { filePath: "/project/.sisyphus/plans/test-plan.md" }, + } + + // when + await hook["tool.execute.after"]( + { tool: "read", sessionID: ORCHESTRATOR_SESSION }, + output + ) + + // then + expect(output.output).toContain("Commit: YES") + expect(output.output).not.toContain("Commit: NO (user disabled auto-commits)") + }) + + test("should NOT transform when file is not a plan file", async () => { + // given + const hook = createAtlasHook(createMockPluginInput(), { directory: TEST_DIR, autoCommit: false }) + const content = "# Regular file\n\nCommit: YES\n\nSome content" + const output = { + title: "Read", + output: content, + metadata: { filePath: "/project/src/file.ts" }, + } + + // when + await hook["tool.execute.after"]( + { tool: "read", sessionID: ORCHESTRATOR_SESSION }, + output + ) + + // then + expect(output.output).toContain("Commit: YES") + expect(output.output).not.toContain("Commit: NO (user disabled auto-commits)") + }) + }) }) describe("session.idle handler (boulder continuation)", () => { diff --git a/src/hooks/atlas/plan-commit-transform.test.ts b/src/hooks/atlas/plan-commit-transform.test.ts new file mode 100644 index 0000000000..0c86a6a24e --- /dev/null +++ b/src/hooks/atlas/plan-commit-transform.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, test } from "bun:test" +import { isPlanPath, transformPlanCommitFields } from "./plan-commit-transform" + +describe("isPlanPath", () => { + describe("#given a valid plan path", () => { + test("#when path is .sisyphus/plans/test.md #then returns true", () => { + expect(isPlanPath(".sisyphus/plans/test.md")).toBe(true) + }) + + test("#when path has full path /project/.sisyphus/plans/test.md #then returns true", () => { + expect(isPlanPath("/project/.sisyphus/plans/test.md")).toBe(true) + }) + + test("#when path uses backslashes #then returns true", () => { + expect(isPlanPath("C:\\project\\.sisyphus\\plans\\test.md")).toBe(true) + }) + }) + + describe("#given a path with .sisyphus as substring (false-positive prevention)", () => { + test("#when path is foo.sisyphus-backup/plans/test.md #then returns false", () => { + expect(isPlanPath("foo.sisyphus-backup/plans/test.md")).toBe(false) + }) + + test("#when path is my.sisyphus/plans/test.md #then returns false", () => { + expect(isPlanPath("my.sisyphus/plans/test.md")).toBe(false) + }) + }) + + describe("#given undefined or empty path", () => { + test("#when path is undefined #then returns false", () => { + expect(isPlanPath(undefined)).toBe(false) + }) + }) +}) + +describe("transformPlanCommitFields", () => { + describe("#given content with Commit: YES", () => { + test("#when transformed #then becomes Commit: NO (user disabled auto-commits)", () => { + const content = "Commit: YES" + const result = transformPlanCommitFields(content) + expect(result).toBe("Commit: NO (user disabled auto-commits)") + }) + }) + + describe("#given content with Commit: NO", () => { + test("#when transformed #then becomes Commit: NO (user disabled auto-commits)", () => { + const content = "Commit: NO" + const result = transformPlanCommitFields(content) + expect(result).toBe("Commit: NO (user disabled auto-commits)") + }) + }) + + describe("#given content with Commit fields with extra whitespace", () => { + test("#when transformed #then handles whitespace variations", () => { + const content = "Commit: YES" + const result = transformPlanCommitFields(content) + expect(result).toBe("Commit: NO (user disabled auto-commits)") + }) + }) + + describe("#given content with multiple Commit fields", () => { + test("#when transformed #then all Commit fields are transformed", () => { + const content = ` +## Task 1 +Commit: YES + +## Task 2 +Commit: NO + +## Task 3 +Commit: YES +` + const result = transformPlanCommitFields(content) + expect(result).toBe(` +## Task 1 +Commit: NO (user disabled auto-commits) + +## Task 2 +Commit: NO (user disabled auto-commits) + +## Task 3 +Commit: NO (user disabled auto-commits) +`) + }) + }) + + describe("#given content with ReCommit: YES (word boundary check)", () => { + test("#when transformed #then ReCommit is NOT transformed", () => { + const content = "ReCommit: YES" + const result = transformPlanCommitFields(content) + expect(result).toBe("ReCommit: YES") + }) + }) + + describe("#given content without Commit fields", () => { + test("#when transformed #then content is unchanged", () => { + const content = ` +## Task +- [ ] Do something +- [ ] Do another thing +` + const result = transformPlanCommitFields(content) + expect(result).toBe(content) + }) + }) + + describe("#given already transformed content", () => { + test("#when transformed again #then remains idempotent", () => { + const originalContent = "Commit: YES" + const firstTransform = transformPlanCommitFields(originalContent) + const secondTransform = transformPlanCommitFields(firstTransform) + expect(secondTransform).toBe(firstTransform) + expect(secondTransform).toBe("Commit: NO (user disabled auto-commits)") + }) + }) + + describe("#given complex plan content with mixed fields", () => { + test("#when transformed #then only Commit fields are changed", () => { + const content = ` +# Plan + +## 1. TASK +Some task description + +## 2. EXPECTED OUTCOME +- [ ] Files created +- [ ] Tests pass + +## 3. REQUIRED TOOLS +- Write +- Bash + +## 4. Commit: YES +This should be committed after completion. +` + const result = transformPlanCommitFields(content) + expect(result).toBe(` +# Plan + +## 1. TASK +Some task description + +## 2. EXPECTED OUTCOME +- [ ] Files created +- [ ] Tests pass + +## 3. REQUIRED TOOLS +- Write +- Bash + +## 4. Commit: NO (user disabled auto-commits) +This should be committed after completion. +`) + }) + }) +}) diff --git a/src/hooks/atlas/plan-commit-transform.ts b/src/hooks/atlas/plan-commit-transform.ts new file mode 100644 index 0000000000..f615bb4592 --- /dev/null +++ b/src/hooks/atlas/plan-commit-transform.ts @@ -0,0 +1,11 @@ +export function isPlanPath(filePath: string | undefined): boolean { + if (!filePath) return false + return /(^|[/\\])\.sisyphus[/\\]plans[/\\].*\.md$/.test(filePath) +} + +/** + * Uses negative lookahead to ensure idempotency (won't re-transform already-transformed content). + */ +export function transformPlanCommitFields(content: string): string { + return content.replace(/\bCommit:\s*(YES|NO)(?!\s*\(user disabled auto-commits\))/g, "Commit: NO (user disabled auto-commits)") +} diff --git a/src/hooks/atlas/tool-execute-after.ts b/src/hooks/atlas/tool-execute-after.ts index fd8c1824cd..e8dee49c02 100644 --- a/src/hooks/atlas/tool-execute-after.ts +++ b/src/hooks/atlas/tool-execute-after.ts @@ -9,8 +9,29 @@ import { isSisyphusPath } from "./sisyphus-path" import { extractSessionIdFromOutput } from "./subagent-session-id" import { buildCompletionGate, buildOrchestratorReminder, buildStandaloneVerificationReminder } from "./verification-reminders" import { isWriteOrEditToolName } from "./write-edit-tool-policy" +import { isPlanPath, transformPlanCommitFields } from "./plan-commit-transform" import type { ToolExecuteAfterInput, ToolExecuteAfterOutput } from "./types" +/** + * Extract file path from metadata using multiple possible key names. + * Tries: filepath, filePath, path, file (in that order) + */ +function extractFilePath(metadata: unknown): string | undefined { + if (!metadata || typeof metadata !== "object") { + return undefined + } + + const objectMeta = metadata as Record + const candidates = [objectMeta.filepath, objectMeta.filePath, objectMeta.path, objectMeta.file] + for (const candidate of candidates) { + if (typeof candidate === "string" && candidate.length > 0) { + return candidate + } + } + + return undefined +} + export function createToolExecuteAfterHandler(input: { ctx: PluginInput pendingFilePaths: Map @@ -18,7 +39,6 @@ export function createToolExecuteAfterHandler(input: { }): (toolInput: ToolExecuteAfterInput, toolOutput: ToolExecuteAfterOutput) => Promise { const { ctx, pendingFilePaths, autoCommit } = input return async (toolInput, toolOutput): Promise => { - // Guard against undefined output (e.g., from /review command - see issue #1035) if (!toolOutput) { return } @@ -28,9 +48,12 @@ export function createToolExecuteAfterHandler(input: { } if (isWriteOrEditToolName(toolInput.tool)) { - let filePath = toolInput.callID ? pendingFilePaths.get(toolInput.callID) : undefined - if (toolInput.callID) { - pendingFilePaths.delete(toolInput.callID) + const mapKey = toolInput.sessionID && toolInput.callID + ? `${toolInput.sessionID}:${toolInput.callID}` + : toolInput.callID + let filePath = mapKey ? pendingFilePaths.get(mapKey) : undefined + if (mapKey) { + pendingFilePaths.delete(mapKey) } if (!filePath) { filePath = toolOutput.metadata?.filePath as string | undefined @@ -46,6 +69,32 @@ export function createToolExecuteAfterHandler(input: { return } + if (toolInput.tool === "read" || toolInput.tool === "Read") { + // Always cleanup pendingFilePaths to prevent memory leak + const mapKey = toolInput.sessionID && toolInput.callID + ? `${toolInput.sessionID}:${toolInput.callID}` + : toolInput.callID + let filePath = mapKey ? pendingFilePaths.get(mapKey) : undefined + if (mapKey) { + pendingFilePaths.delete(mapKey) + } + if (!filePath) { + filePath = extractFilePath(toolOutput.metadata) + } + // Transform plan files only when autoCommit is disabled + if (!autoCommit && filePath && isPlanPath(filePath)) { + const output = toolOutput.output + if (output && typeof output === "string") { + toolOutput.output = transformPlanCommitFields(output) + log(`[${HOOK_NAME}] Transformed plan Commit fields for read`, { + sessionID: toolInput.sessionID, + filePath, + }) + } + } + return + } + if (toolInput.tool !== "task") { return } diff --git a/src/hooks/atlas/tool-execute-before.ts b/src/hooks/atlas/tool-execute-before.ts index 51f670000b..e188d30f6a 100644 --- a/src/hooks/atlas/tool-execute-before.ts +++ b/src/hooks/atlas/tool-execute-before.ts @@ -27,8 +27,8 @@ export function createToolExecuteBeforeHandler(input: { const filePath = (toolOutput.args.filePath ?? toolOutput.args.path ?? toolOutput.args.file) as string | undefined if (filePath && !isSisyphusPath(filePath)) { // Store filePath for use in tool.execute.after - if (toolInput.callID) { - pendingFilePaths.set(toolInput.callID, filePath) + if (toolInput.callID && toolInput.sessionID) { + pendingFilePaths.set(`${toolInput.sessionID}:${toolInput.callID}`, filePath) } const warning = ORCHESTRATOR_DELEGATION_REQUIRED.replace("$FILE_PATH", filePath) toolOutput.message = (toolOutput.message || "") + warning @@ -41,6 +41,15 @@ export function createToolExecuteBeforeHandler(input: { return } + // Store filePath for Read tools (for plan transformation in tool.execute.after) + if (toolInput.tool === "read" || toolInput.tool === "Read") { + const filePath = (toolOutput.args.filePath ?? toolOutput.args.path ?? toolOutput.args.file) as string | undefined + if (filePath && toolInput.callID && toolInput.sessionID) { + pendingFilePaths.set(`${toolInput.sessionID}:${toolInput.callID}`, filePath) + } + return + } + // Check task - inject single-task directive if (toolInput.tool === "task") { const prompt = toolOutput.args.prompt as string | undefined @@ -50,6 +59,7 @@ export function createToolExecuteBeforeHandler(input: { sessionID: toolInput.sessionID, }) } + return } } } diff --git a/src/hooks/atlas/verification-reminders.ts b/src/hooks/atlas/verification-reminders.ts index 019e5e8699..e6dda221fc 100644 --- a/src/hooks/atlas/verification-reminders.ts +++ b/src/hooks/atlas/verification-reminders.ts @@ -57,9 +57,19 @@ export function buildOrchestratorReminder( - Stage ONLY the verified changes - Commit with clear message describing what was done ` - : "" + : ` +**STEP 7: COMMITS DISABLED** - const nextStepNumber = autoCommit ? 8 : 7 +The user has disabled auto-commits (\`start_work.auto_commit: false\`). +Changes will NOT be committed automatically. + +**IMPORTANT: Inform the user now:** +- State clearly that no commits were made +- Summarize what changes were completed +- Ask if they want to commit manually or enable auto-commit +` + + const nextStepNumber = 8 return ` ---