-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(atlas): transform plan Commit fields when auto_commit disabled #2325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
54a545f
b85a34a
7ec9f36
7f26d45
fbc5426
09cd863
f9c165b
0c3bffc
a187c1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
| `) | ||
| }) | ||
| }) | ||
| }) |
| 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)") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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:** | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| - 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 ` | ||
| --- | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the
Readresult 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 theCommit:line can diverge from what the model previously saw. That makesReadnon-faithful and introduces a new inconsistency even though it fixes the signaling issue.