fix(runs,tui): tag project ownership with a transportable digest - #523
Conversation
The project tag stored the resolved path, and psmux refuses a value that cannot cross its CLI->server control line verbatim. Filter those refusal shapes through Path.resolve() on Windows and one survives: a spaced UNC share, \srv\share name\proj. There every session went untagged, and untagged is weak ownership twice over — it dies with the run dir, so an orphaned session leaked once `clean` removed the directory, and it proves ownership by run-id collision on disk rather than by identity, so a reused --run-id let one project prune another's session. project_tag now returns a sha256 prefix of the resolved path, which clears the transport gate by construction. Read sites compare against accepted_tags so a tag written by an earlier release still proves ownership: the ctl session is long-lived and shared across projects, and without that its windows would read as another project's after an upgrade. The gate itself is unchanged — it remains the general contract for `@` session options, it is simply no longer the project tag's expected path. Closes bmad-code-org#419
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughProject ownership tags now use transport-safe 16-character SHA-256 digests. Session and window pruning accepts both digest tags and legacy path tags. Untagged-state fallback behavior remains unchanged. ChangesProject ownership tagging
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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/adapters/psmux_backend.py (1)
519-527: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRaise when session-option cleanup fails.
_write_scoped()only prints a warning whenset-option -ufails. The existing value can then remain set. Line 523 incorrectly states that the option reads as unset. A stale non-empty project tag prevents the untagged ownership fallback and can leave a session unprunable.
src/bmad_loop/adapters/psmux_backend.py#L519-L527: Propagate a typedTmuxErrorwhen the unset write fails. State that the option reads as unset only after a successful cleanup.docs/multiplexer-backends.md#L79-L88: Document the successful-cleanup behavior and the loud failure path.As per coding guidelines, “At I/O boundaries, fail loudly: use typed escalation rather than bare
except; observation may degrade, but repair writes must raise.”🤖 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/adapters/psmux_backend.py` around lines 519 - 527, The cleanup path in _write_scoped must propagate a typed TmuxError when unsetting the non-transportable session option fails, and only report the option as unset after successful cleanup; update src/bmad_loop/adapters/psmux_backend.py lines 519-527 accordingly. Document this successful-cleanup behavior and loud failure path in docs/multiplexer-backends.md lines 79-88.Source: Coding guidelines
🤖 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/adapters/psmux_backend.py`:
- Around line 519-527: The cleanup path in _write_scoped must propagate a typed
TmuxError when unsetting the non-transportable session option fails, and only
report the option as unset after successful cleanup; update
src/bmad_loop/adapters/psmux_backend.py lines 519-527 accordingly. Document this
successful-cleanup behavior and loud failure path in
docs/multiplexer-backends.md lines 79-88.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f896a50b-5761-42d6-887b-6f6f1b59e301
📒 Files selected for processing (8)
CHANGELOG.mddocs/multiplexer-backends.mdsrc/bmad_loop/adapters/psmux_backend.pysrc/bmad_loop/runs.pysrc/bmad_loop/tui/launch.pytests/test_psmux_backend.pytests/test_runs.pytests/test_tui_launch.py
|
@coderabbitai The
The one fair nit — the refusal warning states "the option reads as unset" before the free has succeeded — is contradicted visibly by the second warning whenever the free fails, so we're leaving the wording as is. |
|
Tip For best results, initiate chat on the files or code changes.
A failed cleanup emits a second warning. It can only preserve a stale foreign tag, which makes pruning skip the session. It cannot cause an incorrect kill. The digest-based I withdraw the finding. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
@codex review |
|
Bookkeeping, no change requested here: I filed #526 to carry forward the two pieces of #419
Neither is a defect in this diff — direction (1) is what this PR set out to do, and #419 itself |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
bmad-code-org#518 landed on main and rewrote `runs.project_tag` for the same class of defect from the other end: a project path the *listing* cannot carry was conditionally percent-encoded, while every transportable path stayed byte-identical so stored tags kept comparing equal. The digest subsumes that. Encoding answered only the listing round trip, so a path the listing carries but psmux's control line refuses — the spaced UNC share this branch exists for — still went untagged. A 16-hex digest clears both transports by construction, and the compatibility objection encoding was shaped around is answered on the read side by `accepted_tags`. So the encoding half of bmad-code-org#518 is removed, not merged alongside: `_survives_listing`, `_TAG_ENCODED_PREFIX` and the now-dead `sys`/`quote` imports go with it. bmad-code-org#518's other halves are independent and untouched — the bounded field split in `BaseTmuxBackend.list_windows`, the whole-run-id compare, and the bmad-code-org#482 control window identity work. Test coverage is ported rather than dropped: bmad-code-org#518's line-separator family now asserts the digest shape and a single-row round trip against the same `_SEP_VALUES` table, and the surrogate case was already covered by this branch's transportability test. The `tmux_base` bounded-split comment no longer claims PROJECT_OPTION holds a path, and states the bound as the seam's standing contract, since no field a caller requests today can hold a tab. CHANGELOG: the two Unreleased entries describing the same tag are folded into one, since both are unreleased and the second described a mechanism this merge removes.
|
Merged The two designs. #518 returns the resolved path byte-identically when it survives a listing round trip, and percent-encodes ( Why the digest wins. Encoding answers only the listing half. The path this branch exists for — a spaced UNC share — passes The session therefore still went untagged after #518. A digest clears both transports by construction — no separator, no tab, no byte outside ASCII, and it satisfies the control-line gate. The compatibility objection encoding was shaped around (rewriting every tag strands the ones already stored on live sessions and windows) is answered on the read side by What was removed: Coverage is ported, not dropped. #518's line-separator family now asserts the digest shape and a single-row round trip over the same
The two Unreleased CHANGELOG entries describing this same tag are folded into one, since both are unreleased and the second described the mechanism this merge removes. The Post-merge on this head: full suite 4946 passed / 0 failed, |
|
@codex review |
![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: aa617fcdf9
ℹ️ 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".
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 (2)
tests/test_tui_launch.py (1)
895-897: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo docstrings still describe the superseded percent-encoding design.
runs.project_tagnow hashes the resolved path and does not encode it. Its own docstring states that the digest "subsumes the conditional percent-encoding this function briefly applied".Two claims here contradict that:
- Line 896-897 states that the separators are carried "by project_tag's encoding", and that "two different mechanisms carry them". Under the digest there is one mechanism: no spelling of the project path reaches the listing at all.
- Line 917 states "project_tag encodes the tag instead".
Both tests still pass, because a hex digest survives the listing. Only the stated reason is wrong. The parametrization at lines 879-883 stays valuable as a path-input matrix, so keep it.
📝 Proposed docstring correction
- Parametrized over all six on purpose: two different mechanisms carry them — - the tab by the backends' bounded field split, the separators by project_tag's - encoding — so one spelling passing says nothing about another.""" + Parametrized over all six on purpose: each is a byte a resolved project path + can legally hold, and project_tag hashes the path rather than carrying it, so + none of them reaches the listing. The matrix pins that the digest is the + single mechanism, rather than one spelling happening to survive."""- Reach and scoping are not a trade: project_tag encodes the tag instead, so - the comparison stays exact and this row is simply not ours. + Reach and scoping are not a trade: project_tag hashes the resolved path, so + the tag is listing-safe by construction, the comparison stays exact, and this + row is simply not ours.Also applies to: 917-918
🤖 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_tui_launch.py` around lines 895 - 897, Update the docstrings for the parametrized tests around the path-input matrix and the `project_tag` behavior to remove references to percent-encoding or multiple mechanisms. Explain that `project_tag` hashes the resolved path into a digest, so no path spelling or separators reach the listing; preserve the existing parametrization and test behavior.src/bmad_loop/tui/launch.py (1)
382-382: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
runs.accepted_tags(project)inctl_window_id
_ctl_window_candidatesaccepts both current digest and legacy resolved-path tags, butctl_window_idcompares only the current digest. A legacy-tagged control window is therefore prunable but unreachable byattach_planandkill_ctl_window. Usetag in runs.accepted_tags(project).🤖 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/tui/launch.py` at line 382, Update ctl_window_id to use runs.accepted_tags(project) when matching control-window tags, allowing both current digest and legacy resolved-path tags; replace the single-tag comparison while preserving the existing attach_plan and kill_ctl_window lookup behavior.
🧹 Nitpick comments (2)
tests/test_runs.py (1)
648-649: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep Windows coverage for valid separator values.
The function-level
win32skip disables every parameter case. If the supported Windows filesystem accepts any of\x85,\u2028, or\u2029, this skips valid regression coverage. Confirm the supported filename rules, then skip only invalid parameter values.The PR objective requires transport coverage for Windows paths.
🤖 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_runs.py` around lines 648 - 649, Replace the function-level Windows skip on the parametrized separator test with per-parameter skipping based on the supported Windows filename rules, retaining coverage for valid values such as \x85, \u2028, or \u2029. Preserve the existing _SEP_VALUES and _SEP_IDS parameterization and ensure Windows transport-path coverage remains enabled for accepted separators.CHANGELOG.md (1)
191-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the entry and use an imperative lead.
As per coding guidelines,
CHANGELOG.mdentries must remain “terse, scannable, and imperative.” This entry is a long implementation narrative and starts with a declarative sentence. Keep it underUnreleased, but summarize the digest behavior, legacy-tag compatibility, and pruning result in a short release note.Suggested shorter entry
- **A project path the multiplexer cannot carry no longer strands the scans over it (`#419`).** - ... + **Use a 16-character SHA-256 prefix for project ownership tags (`#419`).** + Preserve legacy path tags during pruning. Prevent transport failures from leaving + sessions unowned or crossing project boundaries.🤖 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 `@CHANGELOG.md` around lines 191 - 206, Rewrite the changelog entry for issue `#419` under Unreleased as a short, scannable imperative release note. Summarize that path tags now use a 16-hex digest, legacy path-shaped tags remain supported, and pruning correctly retains and removes owned sessions/windows without the current implementation narrative.Source: Coding guidelines
🤖 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/tui/launch.py`:
- Line 382: Update ctl_window_id to use runs.accepted_tags(project) when
matching control-window tags, allowing both current digest and legacy
resolved-path tags; replace the single-tag comparison while preserving the
existing attach_plan and kill_ctl_window lookup behavior.
In `@tests/test_tui_launch.py`:
- Around line 895-897: Update the docstrings for the parametrized tests around
the path-input matrix and the `project_tag` behavior to remove references to
percent-encoding or multiple mechanisms. Explain that `project_tag` hashes the
resolved path into a digest, so no path spelling or separators reach the
listing; preserve the existing parametrization and test behavior.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 191-206: Rewrite the changelog entry for issue `#419` under
Unreleased as a short, scannable imperative release note. Summarize that path
tags now use a 16-hex digest, legacy path-shaped tags remain supported, and
pruning correctly retains and removes owned sessions/windows without the current
implementation narrative.
In `@tests/test_runs.py`:
- Around line 648-649: Replace the function-level Windows skip on the
parametrized separator test with per-parameter skipping based on the supported
Windows filename rules, retaining coverage for valid values such as \x85,
\u2028, or \u2029. Preserve the existing _SEP_VALUES and _SEP_IDS
parameterization and ensure Windows transport-path coverage remains enabled for
accepted separators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae3bac30-e86d-497c-a5f5-5ede661d4e3f
📒 Files selected for processing (8)
CHANGELOG.mdsrc/bmad_loop/adapters/psmux_backend.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/runs.pysrc/bmad_loop/tui/launch.pytests/test_psmux_backend.pytests/test_runs.pytests/test_tui_launch.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_psmux_backend.py
- src/bmad_loop/adapters/psmux_backend.py
- src/bmad_loop/runs.py
`ctl_window_id` compared the stored window tag against `project_tag(project)` alone, so a control window tagged before the digest read as foreign and `a` and `x` could no longer reach this project's own orchestrator — while `_ctl_window_candidates`, which accepts the legacy tag, would happily prune the same window. Reachable and pruned-but-unreachable are the wrong pair. The site is one main added (bmad-code-org#482 window identity) after this branch forked, so the branch's read-side update never covered it and the merge did not conflict. Both readers now go through `accepted_tags`; the writers keep minting the current digest. Scoping is unchanged: a nonempty tag outside the accepted set is still foreign, so a stop cannot cross a project boundary. Found by codex review on the merge commit.
|
@codex review |
|
@coderabbitai full review |
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Two test docstrings still explained the tag's safety as percent-encoding, and one credited the tab case to the backends' bounded split. Under the digest neither spelling of the path reaches the listing, so there is one mechanism, not two. Both tests passed either way — only the stated reason was wrong. The bmad-code-org#419 CHANGELOG entry is cut roughly in half: the transports and the user-facing consequence stay, the blow-by-blow of which byte defeats which parse goes. Headline keeps its noun-phrase form, which is the section's convention (Unreleased opens 54x "A", 22x "The", 7x "An", 0x imperative). Also narrows a skip reason that overclaimed: NEL, U+2028 and U+2029 are not illegal in win32 names, so the skip rests on the property being a POSIX-name concern rather than on a filesystem rule. Found by CodeRabbit review on the merge commit.
|
@coderabbitai Triaged all four findings from the 1. 2. Stale percent-encoding docstrings (Minor) — valid, fixed. Good catch, and it found real drift my own sweep missed: I had grepped for the identifiers ( 3. CHANGELOG length (Trivial) — the length half is valid, the imperative half is declined. Entry is cut roughly in half; the transports and the user-facing consequence stay, the byte-by-byte parse narrative goes. The imperative-headline part is a false positive against this repo's actual convention. Measured over the Rewriting this one headline to "Use a 16-character SHA-256 prefix…" would make it the sole outlier in ~90 entries. 4. Per-parameter Windows skip (Trivial) — declined, with one correction taken. You are right that Not splitting the skip per parameter, though: it would encode an assertion about which of these bytes a Windows filesystem accepts, which I cannot validate from this machine, and a wrong guess turns a green Windows job red for a reason unrelated to the tag. The sibling test on On |
|
@codex review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== relevant files =='
fd -a -t f '^(runs|launch|test_runs|test_tui_launch|AGENTS|CHANGELOG)\.(py|md)$|^AGENTS\.md$|^CHANGELOG\.md$' . \
| sort
printf '%s\n' '== recent relevant commits =='
git show --no-patch --oneline 351ffba 468709f aa617fc 2>&1 || true
printf '%s\n' '== outlines =='
for file in \
src/bmad_loop/runs.py \
src/bmad_loop/tui/launch.py \
tests/test_runs.py \
tests/test_tui_launch.py
do
if [ -f "$file" ]; then
echo "-- $file"
ast-grep outline "$file" --items all --type function --match 'accepted_tags|project_tag|prunable_sessions|ctl_window_id|test_project_tag|test_prunable|test_a_delimiter' || true
fi
done
printf '%s\n' '== ownership readers/writers and stale encoding prose =='
rg -n -C 4 --glob '*.py' \
'accepted_tags\s*\(|project_tag\s*\(|bmad_project|PROJECT_OPTION|encoding|encodes|percent' \
src/bmad_loop tests || true
printf '%s\n' '== changelog unreleased and guidance =='
if [ -f AGENTS.md ]; then sed -n '1,220p' AGENTS.md; fi
if [ -f CHANGELOG.md ]; then
sed -n '1,180p' CHANGELOG.md
fiLength of output: 174438 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== relevant commit summaries =='
git show --no-patch --format='%h %s' 351ffba 468709f aa617fc
printf '%s\n' '== runs ownership implementation =='
sed -n '330,410p' src/bmad_loop/runs.py
printf '%s\n' '== TUI ownership readers and writer =='
sed -n '350,400p' src/bmad_loop/tui/launch.py
sed -n '635,680p' src/bmad_loop/tui/launch.py
sed -n '745,780p' src/bmad_loop/tui/launch.py
printf '%s\n' '== all accepted_tags call sites =='
rg -n -C 3 'accepted_tags\s*\(' src/bmad_loop/runs.py src/bmad_loop/tui/launch.py
printf '%s\n' '== revised test documentation and Windows skip =='
sed -n '620,735p' tests/test_runs.py
rg -n -C 5 'skipif|skip\(|POSIX.*name|delimiter.*project.*path' tests/test_runs.py tests/test_tui_launch.py
printf '%s\n' '== digest changelog entry and nearby headline convention =='
rg -n -C 4 -i 'digest|psmux.*tag|project.*tag|ownership tag' CHANGELOG.md
sed -n '1,90p' CHANGELOG.md
printf '%s\n' '== diff of applied follow-up commit =='
git show --format=fuller --stat --find-renames 468709f
git show --format= --unified=5 468709f -- \
src/bmad_loop/runs.py src/bmad_loop/tui/launch.py tests/test_runs.py tests/test_tui_launch.py CHANGELOG.mdLength of output: 57395
I verified the source and documentation changes. I did not independently run the reported test, ✏️ Learnings added
You are interacting with an AI system. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
2 participants
The project tag stored the resolved path, and psmux refuses a value that cannot cross its CLI->server control line verbatim (#418). Filter those refusal shapes through
Path.resolve()on Windows and one survives: a spaced UNC share,\srv\share name\proj. There every session went untagged, and untagged is weak ownership twice over (#419) — it dies with the run dir, so an orphaned session leaked oncecleanremoved the directory, and it proves ownership by run-id collision on disk rather than by identity, so a reused--run-idlet one project prune another's session.project_tagnow returns a 16-hex sha256 prefix of the resolved path, which clears the transport gate by construction. Both read sites (runs.prunable_sessions, the TUI's ctl-window prune) compare againstaccepted_tags— digest plus the legacy resolved-path form — so a tag written by an earlier release still proves ownership: the ctl session is long-lived and shared across projects, and without legacy acceptance its windows would read as another project's after an upgrade.The transport gate itself is unchanged — it remains the general contract for
@session options; it is simply no longer the project tag's expected path. The untagged run-dir fallback also stays, for state whose tag write failed or predates a working one.Tests: tag shape is hex whatever the path (with the refused UNC literal asserted as the premise) and clears the psmux gate; legacy path tags stay owned and foreign paths/digests stay foreign on both read sites; existing prune-partition and ctl-window tests pass unedited.
Closes #419
Summary by CodeRabbit