fix(platform): harden win32 atomic replaces and path segments#98
Conversation
os.replace over a file another process holds open fails on Windows with WinError 5 (Python readers take no FILE_SHARE_DELETE), which froze a live run when the TUI poller collided with the engine's state write. POSIX rename-over-open never raises, so none of this bites on Linux/macOS. - add atomic_replace: a win32-gated, jittered-backoff retry around os.replace (~5 s worst case, sized for AV/indexer handle holds), and route every rename-over-open site through it: state, decisions, sweep answers, TUI settings, run archives - add safe_segment: an injective Windows-filename sanitizer (illegal and control chars, reserved device names incl. COM0/superscripts, trailing dot/space, length cap, digest suffix on any change; identity for clean input) and apply it wherever story/unit keys become path or window segments: task ids, feedback files, resolve dirs, worktree and failed dirs, deferred artifacts - read agent MCP configs BOM-tolerantly and fail closed on undecodable content; explain identity-guard skips in unity teardown - gate os.kill usage outside the process host
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds Windows-safe atomic replacement and path-segment sanitization helpers, uses them in persistence and path construction code, tightens Unity config and teardown handling, and broadens the portability guard’s ChangesWindows path-safety and atomic-write hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmad_loop/engine.py (1)
1166-1191: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
_resumable_session'stask_idformula diverges from_run_session's, breaking resume for sanitized story keys.
_run_session(Lines 1996-1998) sanitizes the whole composed"{story_key}-{role}-{seq}"string, per its own comment ("Sanitize the whole composition, not the parts")._resumable_session(Line 1179) instead sanitizes onlystory_keyand appends-{role}-{seq}afterward. Sincesafe_segmentappends a hash digest of the exact input it sanitized, these two produce different strings whenever the story key actually needs sanitization (illegal chars, trailing dot/space, or length overflow) — exactly the scenario this PR is meant to harden. The result: on crash-resume,_resumable_sessionwill never match the persistedSessionRecord, so the engine falls through to the destructive resume-restart path (rollback + re-run) instead of resuming completed work.🐛 Proposed fix — mirror `_run_session`'s composition
- task_id = f"{safe_segment(task.story_key)}-{role}-{seq}" + task_id = safe_segment(f"{task.story_key}-{role}-{seq}")Also applies to: 1985-1998
🤖 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/bmad_loop/engine.py` around lines 1166 - 1191, The task/session lookup in _resumable_session uses a different task_id composition than _run_session, so sanitized story keys will not match persisted SessionRecord entries on resume. Update _resumable_session to build task_id the same way as _run_session does for the session record (sanitize the full composed story_key-role-seq string, not just story_key), using the same safe_segment-based logic so resumed sessions can be found reliably.
🧹 Nitpick comments (2)
src/bmad_loop/data/plugins/unity/unity_setup.py (1)
428-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for BOM-prefixed and undecodable config content.
The PR objectives call out "BOM-tolerant reading" and "fail-closed behavior on undecodable content," but no test exercises either path. A BOM-prefixed file would verify
utf-8-sigstripping works, and a binary/invalid-UTF-8 file would verify theUnicodeDecodeErrorbranch returnsFalse.🧪 Suggested test additions
# In tests/test_engine_plugin.py, extend test_verify_agent_isolation: def test_verify_agent_isolation_bom_prefixed_config(tmp_path): """A BOM-prefixed config file is read correctly (utf-8-sig strips the BOM).""" mod = _load_unity_setup() url = "http://localhost:23723" cdir = tmp_path / ".codex" cdir.mkdir() bom = b"\xef\xbb\xbf" content = '[mcp_servers.ai-game-developer]\nurl = "http://localhost:23723"\n' (cdir / "config.toml").write_bytes(bom + content.encode("utf-8")) assert mod._verify_agent_isolation("codex", tmp_path, url) is True def test_verify_agent_isolation_undecodable_config(tmp_path, capsys): """Undecodable config content triggers fail-closed (returns False).""" mod = _load_unity_setup() url = "http://localhost:23723" cdir = tmp_path / ".codex" cdir.mkdir() (cdir / "config.toml").write_bytes(b"\xff\xfe\x00\x01binary garbage") assert mod._verify_agent_isolation("codex", tmp_path, url) is False assert "missing or unreadable" in capsys.readouterr().err🤖 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/bmad_loop/data/plugins/unity/unity_setup.py` around lines 428 - 431, Add tests for the BOM-tolerant and fail-closed config read paths in _verify_agent_isolation. Extend the existing test coverage around unity_setup so one test writes a UTF-8 BOM-prefixed config and asserts the config is parsed successfully, and another writes invalid UTF-8/binary content and asserts the function returns False and emits the unreadable-config message. Use the _verify_agent_isolation helper and the config.toml setup in the existing plugin tests to locate the behavior.src/bmad_loop/platform_util.py (1)
122-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuff
S311will still fire despite the# noseccomment.
# nosec B311only suppresses Bandit; Ruff'sS311rule uses its own# noqasyntax, so this line will keep failing lint even though the randomness use (retry jitter, not crypto) is legitimate.🔧 Suggested fix
- time.sleep(delay + random.uniform(0, _REPLACE_BASE_S)) # nosec B311 - retry jitter + time.sleep(delay + random.uniform(0, _REPLACE_BASE_S)) # nosec B311 # noqa: S311 - retry jitter, not crypto🤖 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/bmad_loop/platform_util.py` around lines 122 - 123, The retry jitter in platform_util is still triggering Ruff S311 because the existing suppression only targets Bandit. Update the time.sleep call in the retry loop to use Ruff’s own noqa suppression on the random.uniform expression or line, and keep the context tied to the retry backoff logic in the same block so the legitimate non-crypto randomness is accepted by lint.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@src/bmad_loop/engine.py`:
- Around line 1166-1191: The task/session lookup in _resumable_session uses a
different task_id composition than _run_session, so sanitized story keys will
not match persisted SessionRecord entries on resume. Update _resumable_session
to build task_id the same way as _run_session does for the session record
(sanitize the full composed story_key-role-seq string, not just story_key),
using the same safe_segment-based logic so resumed sessions can be found
reliably.
---
Nitpick comments:
In `@src/bmad_loop/data/plugins/unity/unity_setup.py`:
- Around line 428-431: Add tests for the BOM-tolerant and fail-closed config
read paths in _verify_agent_isolation. Extend the existing test coverage around
unity_setup so one test writes a UTF-8 BOM-prefixed config and asserts the
config is parsed successfully, and another writes invalid UTF-8/binary content
and asserts the function returns False and emits the unreadable-config message.
Use the _verify_agent_isolation helper and the config.toml setup in the existing
plugin tests to locate the behavior.
In `@src/bmad_loop/platform_util.py`:
- Around line 122-123: The retry jitter in platform_util is still triggering
Ruff S311 because the existing suppression only targets Bandit. Update the
time.sleep call in the retry loop to use Ruff’s own noqa suppression on the
random.uniform expression or line, and keep the context tied to the retry
backoff logic in the same block so the legitimate non-crypto randomness is
accepted by lint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fe2299f-250b-42c5-bd3e-8be507614711
📒 Files selected for processing (17)
src/bmad_loop/data/plugins/unity/unity_setup.pysrc/bmad_loop/data/plugins/unity/unity_teardown.pysrc/bmad_loop/decisions.pysrc/bmad_loop/engine.pysrc/bmad_loop/journal.pysrc/bmad_loop/platform_util.pysrc/bmad_loop/resolve.pysrc/bmad_loop/runs.pysrc/bmad_loop/sweep.pysrc/bmad_loop/tui/settings.pysrc/bmad_loop/workspace.pytests/test_engine.pytests/test_engine_plugin.pytests/test_journal.pytests/test_platform_util.pytests/test_portability_guard.pytests/test_resolve.py
|
@pbean please see the outside diff comment... |
_run_session sanitizes the whole composed "{key}-{part}-{seq}" string while
_resumable_session sanitized only the story key and re-appended "-{role}-{seq}"
raw. Whenever safe_segment actually changes the key (illegal chars, or a clean
key whose composed id overflows the segment cap) the two ids differ — different
digest input, different truncation — so the stored SessionRecord never matches
and _finish_inflight falls through to the destructive resume-restart, defeating
the bmad-code-org#61 post-kill rescue for exactly the keys this PR hardens.
SessionRecord stores no seq, so exact-id matching is load-bearing; route both
sites through one _session_task_id helper instead of mirroring the formula
(composing inline is how they diverged), and pin the parity with a regression
test over a dirty key and a cap-overflowing clean key.
|
@dracic Confirmed — the outside-diff critical is real, and it's a good catch by the rabbit. I traced the consequence: for any story key that Rather than round-trip, I pushed the fix to your branch (c768e9d): both sites now route through one On the rest of the review sweep:
I also ran a full consumer audit of every Will approve once CI is green on the new head. Thanks for a genuinely thorough piece of work — the win32-gated retry sizing rationale and the reserved-name coverage (COM0/superscripts/CONIN$) are better than what most of the ecosystem ships. |
pbean
left a comment
pbean
left a comment
There was a problem hiding this comment.
Validated end to end: full consumer audit of every safe_segment site and every rename-over site came back clean (details in my comment above), the one critical (resume task-id divergence) is fixed on-branch with a red/green parity test, and CI is green on the new head including both Windows jobs. Follow-ups tracked in #101/#102.
…ames (#103) * fix(platform): overwrite-safe deferred stash + ref-safe unit branch names #101 — _stash_deferred_artifacts staged a re-defer's spec straight over the prior stash with shutil.move. Its os.rename raises FileExistsError on Windows, which move catches and papers over with a non-atomic copy2 + unlink: that tears the stash on a mid-copy crash, and re-fails outright when an AV/indexer handle turns the rename into a sharing violation and copy2 cannot open the locked target either. Stage a copy inside the destination dir, then route through platform_util.atomic_replace — one-step overwrite, #98's win32 retry, and the replace stays same-filesystem so move's cross-device tolerance is preserved. #102 — unit_branch_name built bmad-loop/<run_id>/<unit_key> from the raw ids, so a key or --run-id carrying `:`, `..`, `@{`, a space or a trailing `.lock` passed #98's directory sanitization only to die at `git worktree add` with "is not a valid branch name". New platform_util.safe_ref_segment mirrors safe_segment's contract (identity for clean input, -<hex8> digest suffix whenever anything changed, never raises) on git's alphabet rather than Windows' — the two rule sets are disjoint in both directions, so a consumer deriving both a directory and a branch from one key must run both. Applied to both segments; a `git check-ref-format` oracle test pins the agreement over a nasty corpus. * fix(platform): retry the source unlink in the deferred stash too Review catch on #103. Windows denies a *delete* against an open handle exactly as it denies a rename-over, so hardening only the replace left the staged move half-hardened: an AV/indexer handle on the just-written source spec fails `spec_path.unlink()` after the stash has already landed. `_defer` calls _stash_deferred_artifacts before the rollback and the `story-deferred` journal append, so that raise aborts the whole deferral. Suppressing the unlink is wrong — it silently leaves the stale in-review spec this function exists to remove. Instead extract atomic_replace's win32 backoff into _retry_on_sharing_violation and reuse it for a new retrying_unlink, giving both halves of the move the same transient-lock tolerance. A permanent failure still propagates, as before. Replace-then-unlink ordering is deliberate: a failure can leave a duplicate spec, never a hole where the work was. * refactor(engine): reuse safe_ref_segment for attempt-preserve slugs Both recovery-ref sites re-implemented ref sanitization inline (alnum/_- join, 64-char cap). Never illegal — but a second rule set to keep in agreement with git, and lossier than the real one: dots in a clean run id were mangled, and distinct exotic ids could collapse to the same slug. Route both through safe_ref_segment: identity for clean ids, digest suffix keeps dirty ids distinct, single source of truth. Characterization test only — no red-first guard is possible because the old inline slug also produced legal refs; the test pins the invariant (real git resolves the slugged ref) and the new digest-suffixed shape.
2 participants
Problem
os.replaceover a file another process holds open fails on Windows with WinError 5 (Python readers take noFILE_SHARE_DELETE), which froze a live run when the TUI poller collided with the engine's state write. POSIX rename-over-open never raises, so none of this bites on Linux/macOS. The same audit surfaced two more Windows defect classes with no cover: no filename sanitizer for user-derived path segments, and an unguarded force-kill.Changes
atomic_replace: win32-gated, jittered-backoff retry aroundos.replace(~5 s worst case, sized for AV/indexer handle holds); every rename-over-open site routes through it — state, decisions, sweep answers, TUI settings, run archives.safe_segment: Windows-filename sanitizer (illegal/control chars, reserved device names incl.COM0/superscripts/CONIN$/CONOUT$, trailing dot/space, length cap, digest suffix on any change for practical collision resistance; identity for clean input) applied wherever story/unit keys become path or window segments: task ids, feedback files, resolve dirs, worktree and failed dirs, deferred artifacts. Composed task ids (key + plugin label) are sanitized as one segment so two individually capped parts can't exceed the filename limit.os.killusage gated outside the process host (test tripwire).Testing
PermissionError.Closes #74
Summary by CodeRabbit
Bug Fixes
Tests
os.killusage.