chore(release): 0.6.0 — rollback never wipes non-automator files#3
Merged
Conversation
…d opt-in A failed in-place attempt previously ran `git reset --hard` + a blanket `git clean -fd` over the whole checkout, sparing only `.automator/` and two artifact subdirs. That could delete a project's `_bmad-output/` and any other untracked files (the cause of the reported notey data loss). The orchestrator now never runs a blanket `git clean`. New `verify.safe_rollback` reverts the attempt's tracked changes to baseline and removes only the untracked files THIS run created (diffed against a baseline-time snapshot, `StoryTask.baseline_untracked`); pre-existing untracked files and the whole `_bmad-output/` are preserved. The output folder is now exposed wholesale via `ProjectPaths.output_folder`. Auto-rollback is gated behind `[scm] rollback_on_failure` (default off). Off: the engine never touches the tree — it pauses with bold manual-recovery instructions (back up untracked files -> git reset --hard <baseline> -> restore). On: the safe rollback above runs and warns. Sweep migration uses a non-pausing `_safe_reset` for internal ledger restore. Worktree isolation sidesteps the path entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughVersion 0.6.0 replaces the previous unconditional ChangesSafe Rollback & Opt-in Policy
Sequence Diagram(s)sequenceDiagram
participant Engine
participant verify
participant StoryTask
participant Journal
Engine->>StoryTask: snapshot baseline_untracked = untracked_files(repo)
Note over Engine: attempt runs...
Engine->>Engine: attempt fails → _rollback_or_pause(task)
alt rollback_on_failure = True
Engine->>verify: safe_rollback(repo, baseline_commit, baseline_untracked)
verify->>verify: git reset --hard baseline
verify->>verify: compute new_files = current − baseline_untracked
verify->>verify: unlink new_files (skipping keep dirs)
verify->>verify: _prune_empty_parents
Engine->>Engine: task defers
else rollback_on_failure = False
Engine->>Journal: record rollback-manual-required entry
Engine->>Engine: emit notification with manual recovery instructions
Engine->>Engine: raise RunPaused
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: This release bumps bmad-auto to 0.6.0 and fixes a data-loss risk in in-place failure recovery by replacing blanket cleaning with a safe, snapshot-based rollback. Changes:
Technical Notes: Rollback now preserves pre-existing untracked files and protects BMAD output directories; worktree isolation continues to avoid in-place rollback paths entirely. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| repo = repo.resolve() | ||
| keep_roots = [(repo / k).resolve() for k in keep] | ||
| for rel in sorted(created): | ||
| path = (repo / rel).resolve() |
There was a problem hiding this comment.
src/automator/verify.py:160: path = (repo / rel).resolve() will follow symlinks, so if a run-created untracked path is a symlink the rollback may unlink() the symlink target (potentially outside the repo) rather than removing the symlink itself. Consider avoiding resolve() on deletion targets and ensuring rollback only deletes within the repo while unlinking the repo entry, not the resolved target.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/automator/engine.py (1)
1350-1356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not stash deferred artifacts before the rollback-off pause.
_stash_deferred_artifacts()moves the spec out of the artifact directory before_rollback_or_pause()decides to pause, so the defaultrollback_on_failure = falsepath still modifies the tree instead of leaving it exactly as-is. Gate the stash behind the auto-rollback path, or perform it only after manual recovery has been acknowledged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/automator/engine.py` around lines 1350 - 1356, The _stash_deferred_artifacts method is being called unconditionally before _rollback_or_pause, which modifies the tree even when rollback_on_failure is false and the task is paused instead of rolled back. Move the _stash_deferred_artifacts call so it only executes within the auto-rollback code path (when rollback_on_failure is true) rather than before _rollback_or_pause is called, ensuring the tree remains unmodified when pausing without auto-rollback enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/automator/engine.py`:
- Around line 650-651: The manual recovery pause logic lacks a check to
determine if recovery has already been acknowledged, causing unconditional
re-pausing on resume even when manual recovery is complete. Add a recovered or
acknowledged state check before calling `_pause_for_manual_recovery()` in the
`_finish_inflight()` and `_rollback_or_pause()` methods to skip the pause if
recovery has been verified. Additionally, in the `_defer()` method, defer
marking the task as terminal until after the recovery pause is completed, rather
than marking it terminal before the pause, so that resuming the workflow can
properly verify recovery status before continuing execution.
In `@src/automator/policy.py`:
- Line 428: The rollback_on_failure parameter assignment uses bool() conversion
which silently converts string values like "false" to True, enabling unintended
auto-rollback behavior. Instead of using bool(scm_d.get(...)), extract the value
and validate that it is actually a boolean type, raising a PolicyError if a
non-boolean value is provided. Only assign the rollback_on_failure parameter
when the value is confirmed to be a genuine boolean from the TOML configuration
or fall back to the ScmPolicy.rollback_on_failure default.
In `@src/automator/verify.py`:
- Around line 157-164: The issue is that path.resolve() is being called on line
160 before unlink(), which follows symlinks to their targets. This causes
unlink() to delete the target file instead of the symlink itself, and can allow
symlinks to bypass the keep_roots protection by resolving outside the designated
keep areas. Remove the .resolve() call on the path variable used in the unlink()
operation, so that comparisons with keep_roots and the unlink() call work on the
original repo-relative lexical path rather than the resolved target path. Keep
the comparison logic but ensure deletion operates on the unresolved path.
In `@tests/test_engine.py`:
- Around line 379-381: The test assertions are not sufficiently verifying
tracked-file rollback behavior. In the first test block around line 380-381, add
an explicit assertion that checks the content of src.txt to verify tracked edits
were properly reset (beyond just checking that the worktree is dirty and the
precious untracked file exists). Similarly, in the second test block around line
405-408, add an explicit assertion that checks the src.txt content to confirm
working-tree content matches expectations, rather than only verifying the commit
pointer position with HEAD. Both test sections need to include explicit src.txt
content verification to strengthen the assertions and properly validate the
tracked-file rollback behavior.
---
Outside diff comments:
In `@src/automator/engine.py`:
- Around line 1350-1356: The _stash_deferred_artifacts method is being called
unconditionally before _rollback_or_pause, which modifies the tree even when
rollback_on_failure is false and the task is paused instead of rolled back. Move
the _stash_deferred_artifacts call so it only executes within the auto-rollback
code path (when rollback_on_failure is true) rather than before
_rollback_or_pause is called, ensuring the tree remains unmodified when pausing
without auto-rollback enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06b63fc1-413c-40e4-a966-a770f7f49dbe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.claude-plugin/marketplace.jsonCHANGELOG.mdmodule.yamlpyproject.tomlsrc/automator/__init__.pysrc/automator/bmadconfig.pysrc/automator/data/settings/core.tomlsrc/automator/data/skills/bmad-auto-setup/assets/module.yamlsrc/automator/engine.pysrc/automator/model.pysrc/automator/policy.pysrc/automator/sweep.pysrc/automator/verify.pytests/test_engine.pytests/test_hook_bus.pytests/test_plugin_workflows.pytests/test_sweep.pytests/test_verify.py
Comment on lines
+650
to
+651
| if not self.policy.scm.rollback_on_failure: | ||
| self._pause_for_manual_recovery(task, task.baseline_commit or "") |
There was a problem hiding this comment.
Make the manual-rollback pause resumable before continuing.
With rollback disabled, _pause_for_manual_recovery() saves the current task state. On retry/resume, _finish_inflight() calls _rollback_or_pause() again and pauses unconditionally, even if the operator already reset manually. In _defer(), the task is already terminal before the pause, so a resume can skip recovery entirely and continue on the dirty tree. Add a recovered/acknowledged state check before re-pausing, and avoid marking the task terminal until recovery has been verified or completed.
Also applies to: 742-743, 1339-1356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/automator/engine.py` around lines 650 - 651, The manual recovery pause
logic lacks a check to determine if recovery has already been acknowledged,
causing unconditional re-pausing on resume even when manual recovery is
complete. Add a recovered or acknowledged state check before calling
`_pause_for_manual_recovery()` in the `_finish_inflight()` and
`_rollback_or_pause()` methods to skip the pause if recovery has been verified.
Additionally, in the `_defer()` method, defer marking the task as terminal until
after the recovery pause is completed, rather than marking it terminal before
the pause, so that resuming the workflow can properly verify recovery status
before continuing execution.
| merge_strategy=str(scm_d.get("merge_strategy", ScmPolicy.merge_strategy)), | ||
| delete_branch=bool(scm_d.get("delete_branch", ScmPolicy.delete_branch)), | ||
| keep_failed=bool(scm_d.get("keep_failed", ScmPolicy.keep_failed)), | ||
| rollback_on_failure=bool(scm_d.get("rollback_on_failure", ScmPolicy.rollback_on_failure)), |
There was a problem hiding this comment.
Reject non-boolean rollback_on_failure values instead of truthifying them.
bool(scm_d.get(...)) makes rollback_on_failure = "false" evaluate to True, silently opting into auto rollback. Since this flag controls whether the checkout is modified on failure, require a real TOML boolean and raise PolicyError otherwise.
Proposed fix
+ raw_rollback_on_failure = scm_d.get(
+ "rollback_on_failure", ScmPolicy.rollback_on_failure
+ )
+ if not isinstance(raw_rollback_on_failure, bool):
+ raise PolicyError(
+ "scm.rollback_on_failure must be a boolean: "
+ f"got {raw_rollback_on_failure!r}"
+ )
scm = ScmPolicy(
isolation=str(scm_d.get("isolation", ScmPolicy.isolation)),
branch_per=str(scm_d.get("branch_per", ScmPolicy.branch_per)),
target_branch=str(scm_d.get("target_branch", ScmPolicy.target_branch)),
merge_strategy=str(scm_d.get("merge_strategy", ScmPolicy.merge_strategy)),
delete_branch=bool(scm_d.get("delete_branch", ScmPolicy.delete_branch)),
keep_failed=bool(scm_d.get("keep_failed", ScmPolicy.keep_failed)),
- rollback_on_failure=bool(scm_d.get("rollback_on_failure", ScmPolicy.rollback_on_failure)),
+ rollback_on_failure=raw_rollback_on_failure,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rollback_on_failure=bool(scm_d.get("rollback_on_failure", ScmPolicy.rollback_on_failure)), | |
| raw_rollback_on_failure = scm_d.get( | |
| "rollback_on_failure", ScmPolicy.rollback_on_failure | |
| ) | |
| if not isinstance(raw_rollback_on_failure, bool): | |
| raise PolicyError( | |
| "scm.rollback_on_failure must be a boolean: " | |
| f"got {raw_rollback_on_failure!r}" | |
| ) | |
| scm = ScmPolicy( | |
| isolation=str(scm_d.get("isolation", ScmPolicy.isolation)), | |
| branch_per=str(scm_d.get("branch_per", ScmPolicy.branch_per)), | |
| target_branch=str(scm_d.get("target_branch", ScmPolicy.target_branch)), | |
| merge_strategy=str(scm_d.get("merge_strategy", ScmPolicy.merge_strategy)), | |
| delete_branch=bool(scm_d.get("delete_branch", ScmPolicy.delete_branch)), | |
| keep_failed=bool(scm_d.get("keep_failed", ScmPolicy.keep_failed)), | |
| rollback_on_failure=raw_rollback_on_failure, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/automator/policy.py` at line 428, The rollback_on_failure parameter
assignment uses bool() conversion which silently converts string values like
"false" to True, enabling unintended auto-rollback behavior. Instead of using
bool(scm_d.get(...)), extract the value and validate that it is actually a
boolean type, raising a PolicyError if a non-boolean value is provided. Only
assign the rollback_on_failure parameter when the value is confirmed to be a
genuine boolean from the TOML configuration or fall back to the
ScmPolicy.rollback_on_failure default.
Comment on lines
+157
to
+164
| repo = repo.resolve() | ||
| keep_roots = [(repo / k).resolve() for k in keep] | ||
| for rel in sorted(created): | ||
| path = (repo / rel).resolve() | ||
| if any(path == root or path.is_relative_to(root) for root in keep_roots): | ||
| continue | ||
| try: | ||
| path.unlink(missing_ok=True) |
There was a problem hiding this comment.
Do not resolve rollback candidates before unlinking.
Line 160 follows symlinks before unlink(). If the run creates junk -> /home/user/file, rollback deletes /home/user/file instead of the junk symlink; a symlink under _bmad-output can also resolve outside the keep root and bypass protection. Keep comparisons should be repo-relative/lexical, and deletion should unlink the original path.
Proposed fix
+from pathlib import PurePosixPath
+
def safe_rollback(
repo: Path,
baseline: str,
*,
baseline_untracked: list[str] | None,
@@
- repo = repo.resolve()
- keep_roots = [(repo / k).resolve() for k in keep]
+ repo = repo.resolve()
+ keep_roots = tuple(PurePosixPath(k) for k in keep)
for rel in sorted(created):
- path = (repo / rel).resolve()
- if any(path == root or path.is_relative_to(root) for root in keep_roots):
+ rel_path = PurePosixPath(rel)
+ if rel_path.is_absolute() or ".." in rel_path.parts:
+ continue
+ if any(rel_path == root or rel_path.is_relative_to(root) for root in keep_roots):
continue
+ path = repo.joinpath(*rel_path.parts)
try:
path.unlink(missing_ok=True)
except OSError:
continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/automator/verify.py` around lines 157 - 164, The issue is that
path.resolve() is being called on line 160 before unlink(), which follows
symlinks to their targets. This causes unlink() to delete the target file
instead of the symlink itself, and can allow symlinks to bypass the keep_roots
protection by resolving outside the designated keep areas. Remove the .resolve()
call on the path variable used in the unlink() operation, so that comparisons
with keep_roots and the unlink() call work on the original repo-relative lexical
path rather than the resolved target path. Keep the comparison logic but ensure
deletion operates on the unresolved path.
Comment on lines
+379
to
+381
| # the orchestrator left the tree exactly as-is — no reset, nothing deleted | ||
| assert not worktree_clean(project.project) | ||
| assert precious.read_text() == "precious\n" |
There was a problem hiding this comment.
Strengthen assertions to verify tracked-file rollback behavior.
Line 380 can stay dirty from the deliberate untracked file even if tracked edits were reset, and Line 407 only checks commit pointer (HEAD), not working-tree content. Add explicit src.txt content checks in both tests.
Suggested test-tightening patch
def test_rollback_off_pauses_with_manual_notice(project):
@@
- # the orchestrator left the tree exactly as-is — no reset, nothing deleted
+ # the orchestrator left tracked edits exactly as-is — no reset
+ assert "change for 1-1-a" in (project.project / "src.txt").read_text()
+ # and the tree is still dirty
assert not worktree_clean(project.project)
assert precious.read_text() == "precious\n"
@@
def test_rollback_on_preserves_preexisting_untracked(project):
@@
- assert rev_parse_head(project.project) == task.baseline_commit # tracked reverted
+ assert rev_parse_head(project.project) == task.baseline_commit
+ assert (project.project / "src.txt").read_text() == "original\n" # tracked reverted
assert precious.read_text() == "keep me\n" # pre-existing untracked survivesAlso applies to: 405-408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_engine.py` around lines 379 - 381, The test assertions are not
sufficiently verifying tracked-file rollback behavior. In the first test block
around line 380-381, add an explicit assertion that checks the content of
src.txt to verify tracked edits were properly reset (beyond just checking that
the worktree is dirty and the precious untracked file exists). Similarly, in the
second test block around line 405-408, add an explicit assertion that checks the
src.txt content to confirm working-tree content matches expectations, rather
than only verifying the commit pointer position with HEAD. Both test sections
need to include explicit src.txt content verification to strengthen the
assertions and properly validate the tracked-file rollback behavior.
dracic
added a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 24, 2026
…d string window_command validated env names but not env values; a newline/control char in a value could desync psmux command-string parsing before pwsh. Add _validate_env_value to reject control characters fail-safe (mirrors the existing _validate_env_key guard). POSIX path unchanged. Refs: _bmad-output/implementation-artifacts/deferred-work.md (deferred-work.md, Adversarial-review deferrals bmad-code-org#3) Claude-Session: https://claude.ai/code/session_019ohWeC44Mto4upsYNNaVh8
dracic
added a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 24, 2026
…e-aware Three Windows branches asserting a real-dir Library result held only on an unprivileged runner; on a Windows host with SeCreateSymbolicLinkPrivilege (Developer Mode/admin) the product symlinks and the asserts would fail. Probe the actual symlink capability and assert a symlink when held, else the real-dir fallback. The forced-denial host-agnostic test and the real-dir priming tests are unchanged. Refs: _bmad-output/implementation-artifacts/deferred-work.md (deferred-work.md, Windows test-suite greening residuals bmad-code-org#3) Claude-Session: https://claude.ai/code/session_019ohWeC44Mto4upsYNNaVh8
This was referenced Jun 25, 2026
Merged
Merged
This was referenced Jul 2, 2026
Merged
pbean
added a commit
that referenced
this pull request
Jul 23, 2026
…findings (#276) A second-model adversarial review of PR #277 surfaced 7 valid findings. This lands the fixes for six of them (the 7th, a repo-wide policy-coercion consistency issue, is deferred to a follow-up): - #1 (High) M2 transition now outranks the M1 hash gate. A clean review that round-trips done->in-review->done back to byte-identical launch bytes while omitting its marker no longer false-refuses; the hash gate refuses only when no transition was observed. Safe: the dead-window false positive never records a transition (status never leaves `done`). - #2 (High) The stories-mode folder+id read-back now applies the same launch-snapshot gate, closing the identical false completion it previously bypassed (it accepted on the mtime floor alone). Inert on a dev leg (no snapshot). - #3 (High) The three in-place spec rewriters (append/strip/reset) now write atomically (temp + atomic_replace), so an interrupted/disk-full repair leaves the original spec intact instead of truncated. - #4 (Med) Adds engine-level coverage of the synthesized-REVIEW repair path (including a follow-up review that rewrites the spec) plus regressions for #1/#2/#3/#5/#7 and a _snapshot_verdict truth-table. - #5 (Low) Snapshot/candidate identity is now by resolved filesystem path (_same_spec), not raw string spelling, so a `..`/symlink/case alias can't disable M1/M2. - #7 (Low) A spec ending in a bare `\r` no longer receives an invisible heading (completed to CRLF so the scan's `^` regex sees it). #1 and #2 share a single `_snapshot_verdict` helper so the "M2 outranks M1" rule can't drift between the two paths. All edits sit on `_DevSynthesisMixin`, covering both the tmux and OpenCode adapters.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
0.6.0 — data-loss fix + opt-in auto-rollback
Why
A user's project (
~/dev/notey) had its_bmad-output/wiped. Root cause: on a failed in-place attempt, the engine rangit reset --hard <baseline>+ a blanketgit clean -fdover the whole checkout, sparing only.automator/and two artifact subdirs. Any other untracked file (incl._bmad-output/when not git-tracked) could be deleted.Fixed
git cleanagain. Newverify.safe_rollbackreverts the attempt's tracked changes and removes only the untracked files that run created (diffed against a baseline-time snapshot,StoryTask.baseline_untracked). Pre-existing untracked files and the entire_bmad-output/tree are preserved (now exposed wholesale viaProjectPaths.output_folder).Changed
[scm] rollback_on_failure(default off).git reset --hard <baseline>→ restore).scm.isolation = "worktree") sidesteps the path entirely.Tests
safe_rollbackunit tests (preserve pre-existing untracked, remove run-created, keep-dir protection, none-snapshot no-op, empty-dir prune) + engine ON/OFF behavior tests. In-place continuation tests opt intorollback_on_failure=True. Full suite 862 passing,trunk checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes – Version 0.6.0
New Features
output_folderpath for output directory customization[scm] rollback_on_failure(default: disabled)Bug Fixes