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
WalkthroughThis PR introduces ChangesWindows path-safety and atomic-write hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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... |
1 participant
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