Skip to content
Open
75 changes: 75 additions & 0 deletions src/hooks/atlas/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down
156 changes: 156 additions & 0 deletions src/hooks/atlas/plan-commit-transform.test.ts
Original file line number Diff line number Diff line change
@@ -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.
`)
})
})
})
11 changes: 11 additions & 0 deletions src/hooks/atlas/plan-commit-transform.ts
Original file line number Diff line number Diff line change
@@ -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)")
}
57 changes: 53 additions & 4 deletions src/hooks/atlas/tool-execute-after.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,36 @@ 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<string, unknown>
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<string, string>
autoCommit: boolean
}): (toolInput: ToolExecuteAfterInput, toolOutput: ToolExecuteAfterOutput) => Promise<void> {
const { ctx, pendingFilePaths, autoCommit } = input
return async (toolInput, toolOutput): Promise<void> => {
// Guard against undefined output (e.g., from /review command - see issue #1035)
if (!toolOutput) {
return
}
Expand All @@ -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
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the Read result itself, so Atlas now sees synthetic plan content instead of the file's real contents. Because the PR intentionally keeps the file on disk unchanged, any later logic that re-reads the plan or tries to edit/match the Commit: line can diverge from what the model previously saw. That makes Read non-faithful and introduces a new inconsistency even though it fixes the signaling issue.

log(`[${HOOK_NAME}] Transformed plan Commit fields for read`, {
sessionID: toolInput.sessionID,
filePath,
})
}
}
return
}

if (toolInput.tool !== "task") {
return
}
Expand Down
14 changes: 12 additions & 2 deletions src/hooks/atlas/tool-execute-before.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -50,6 +59,7 @@ export function createToolExecuteBeforeHandler(input: {
sessionID: toolInput.sessionID,
})
}
return
}
}
}
14 changes: 12 additions & 2 deletions src/hooks/atlas/verification-reminders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:**
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These instructions now conflict with the next step. Step 7 tells the orchestrator to stop and ask the user about manual commit / re-enabling auto-commit, while Step 8 still says PROCEED TO NEXT TASK and DO NOT STOP. In Boulder flow that is a behavior change from "continue autonomously, just skip the commit" to "pause after each completed task," which looks like an unintended regression when autoCommit is disabled.

- 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 `
---
Expand Down