feat(atlas): transform plan Commit fields when auto_commit disabled#2325
feat(atlas): transform plan Commit fields when auto_commit disabled#2325acamq wants to merge 9 commits intocode-yeongyu:devfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Safe transformation of plan files in-memory only when auto_commit is disabled. Guarded by config check, includes unit tests, and default behavior (auto_commit: true) is preserved.
The original implementation in tool-execute-before was transforming args.content for Read tools, but Read tools return content in output, not in input arguments. Move the transformation to tool-execute-after where it can properly access toolOutput.output.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/atlas/tool-execute-after.ts">
<violation number="1" location="src/hooks/atlas/tool-execute-after.ts:69">
P1: When `autoCommit` is enabled, the cleanup block for `read` tools is bypassed. This causes a memory leak as `callID` entries accumulate in the `pendingFilePaths` Map during orchestration. The map deletion should happen unconditionally for all read operations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The cleanup of pendingFilePaths entries was only executed when autoCommit was disabled, causing entries to accumulate indefinitely when autoCommit was enabled. Moved cleanup outside the autoCommit condition so it runs unconditionally for all read operations.
… module - Fix verification step numbering: both autoCommit modes now use Step 7 for commit-related instructions, Step 8 for proceed-to-next-task - Extract isPlanPath and transformPlanCommitFields to shared module to eliminate cross-phase coupling between before/after handlers - Remove unused autoCommit parameter from createToolExecuteBeforeHandler
- Add path boundary to isPlanPath regex to prevent substring false-positives (e.g., foo.sisyphus-backup/plans/x.md no longer matches) - Add word boundary to Commit regex to prevent matching ReCommit: YES - Key pendingFilePaths by sessionID:callID to prevent cross-session data leakage - Rename test file to match the module it tests (plan-commit-transform.test.ts) - Add user notification instructions when auto_commit is false
| The user has disabled auto-commits (\`start_work.auto_commit: false\`). | ||
| Changes will NOT be committed automatically. | ||
|
|
||
| **IMPORTANT: Inform the user now:** |
There was a problem hiding this comment.
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.
| if (!autoCommit && filePath && isPlanPath(filePath)) { | ||
| const output = toolOutput.output | ||
| if (output && typeof output === "string") { | ||
| toolOutput.output = transformPlanCommitFields(output) |
There was a problem hiding this comment.
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.
2 participants
Summary
Fixes an issue where the plan file still contains
Commit: YESinstructions even whenauto_commit: falseis configured, which could confuse Atlas into committing anyway.Related to #2198 — that PR added the
auto_commit: falseconfig option, but the plan file content wasn't being transformed to reflect this setting. The plan file is a stronger signal than the orchestrator reminder, so seeingCommit: YESin the plan could cause the model to commit regardless of the config.Changes
Plan content transformation (
tool-execute-before.ts): Whenauto_commit: false, Read tool output for.sisyphus/plans/*.mdfiles now transforms:Commit: YES→Commit: NO (user disabled auto-commits)Commit: NO→Commit: NO (user disabled auto-commits)Step 8 message (
verification-reminders.ts): Whenauto_commit: false, Step 8 now shows:Unit tests (
tool-execute-before.test.ts): 7 tests covering transformation logic including idempotency.Key Implementation Details
auto_commit: trueTest Plan
Summary by cubic
Honor
auto_commit: falseby transforming plan “Commit” fields to “NO” when reading.sisyphus/plans/*.md, and show a clear “commits disabled” reminder. Also fixes read-path cleanup and cross-session collisions inpendingFilePaths.tool.execute.after: “Commit: YES/NO” → “Commit: NO (user disabled auto-commits)”; idempotent, uses robust file-path extraction, handles Windows paths, and won’t match “ReCommit”.foo.sisyphus-backup/...).pendingFilePathsbysessionID:callIDand always clean up on Read to prevent memory leaks and cross-session data leaks.Written for commit a187c1b. Summary will update on new commits.