fix(init): confine .bmad-loop, policy.toml and .gitignore writes (#771) - #829
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. Walkthrough
ChangesInit containment
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changed init behavior rejects escaping targets while preserving supported in-project links; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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. A rabbit checks each path with care Comment |
f8e96b3 to
2d27293
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d2729349b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| project, outside = tmp_path / "proj", tmp_path / "outside" | ||
| project.mkdir() | ||
| outside.mkdir() | ||
| (project / ".bmad-loop").symlink_to(outside, target_is_directory=True) |
There was a problem hiding this comment.
These new tests hand-roll project directories under tmp_path, and the remaining added cases repeat the same pattern. This bypasses the shared project fixture’s Git repository and BMAD-shaped baseline, so the tests can miss interactions with the initialized repository and silently drift as the canonical sandbox changes; adapt the fixture copy for each symlink/junction scenario instead.
AGENTS.md reference: AGENTS.md:L58-L60
Useful? React with 👍 / 👎.
2d27293 to
7e37359
Compare
7e37359 to
54a8d7d
Compare
install_into guarded only the hook-config and skill-tree writes. The .bmad-loop mkdir, the fallback policy.toml write and the .gitignore append went through whatever link sat at the name. A symlink or junction out of the project, or a dangling policy.toml link that fails is_file(), made init write outside the tree and still report success. All three destinations are now checked with _confined_to before init's first write. An escape prints FAIL naming the path and returns 1, with no hook config, skills, policy or gitignore change left behind. Strictly-below is the right predicate for each path: - .bmad-loop: a directory that resolves to the root would drop policy.toml at top level. The policy check alone misses that, because root/policy.toml is still below the root. - policy.toml and .gitignore: these name files, and the root is a directory no write should aim at. An in-project link still passes and is written through. The .gitignore append stays an append, now opened by its resolved name, so it keeps the file mode and leaves an in-project link a link. The new policy keeps write_text rather than atomic_write_text. That branch runs only when no regular file exists, so a torn write loses nothing the operator owns (#379's concern). atomic_write_text would also mint the file 0600 instead of the umask default.
54a8d7d to
9e209bb
Compare
…dbox The refusal rows now snapshot the sandbox's git status (ignored paths included) after planting the link and require it unchanged after the refused init, instead of probing a hand-rolled tmp_path tree.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #771.
Summary
install_intoguarded only the hook-config and skill-tree writes. The.bmad-loopmkdir, the fallbackpolicy.tomlwrite and the.gitignoreappend all followed whatever link was at that name. A symlink or junction pointing out of the project, or a danglingpolicy.tomllink (which failsis_file()), madeinitwrite outside the project tree and still report success._confined_tobeforeinit's first write. If one escapes,initprints FAIL naming the path and returns 1. No hook config, skills, policy or.gitignorechange is left behind..bmad-loop, a directory that resolves to the root itself would putpolicy.tomlat the top level, and checking only the policy path misses that.policy.tomland.gitignorename files, so the root directory itself is never a valid target. A link that stays inside the project still passes, and the write goes through it..gitignoreappend is still an append, now opened by its resolved name, so it keeps the file's mode and leaves an in-project link as a link. The new policy still useswrite_text, notatomic_write_text. That branch runs only when no regular file exists, so a torn write loses nothing the operator owns (Non-atomic read-modify-rewrite in ~10 places; a short write truncates ledgers, specs, and the board #379's concern).atomic_write_textwould also create the file as 0600 instead of the umask default.Notes for reviewers
initnow refuses when.bmad-loopor an existingpolicy.tomlresolves outside the project, even when it would write nothing there. Operators who keep either one outside the project tree (for example, symlinked to a shared location) will seeinitfail until they move it inside the project.test_init_refuses_a_bmad_loop_dir_junctioned_out_of_the_projectruns only on win32 and is skipped on Linux by design. The symlink cases are POSIX-only.Stack
Part of a stack; merge bottom-up. This is layer 4 of 8, on top of #828.
Stack order (bottom → top): #826 → #827 → #828 → #829 → #830 → #831 → #832 → #833.
Summary by CodeRabbit
Bug Fixes
initnow refuses to proceed when.bmad-loop,policy.toml, or.gitignoreresolves outside the project directory or to the project root..gitignoreupdates preserve existing content, permissions, and repeat-run behavior when using valid in-project paths.Documentation