fix(tui): resolve a run's control window by identity, not listing order - #518
Conversation
Control-session windows are named <kind>-<run_id> over four kinds, so the name is not unique and ctl_window_id answered the first matching row. A resume launched over a still-parked run window therefore drove attach, the @bmad_return_pane stamp and the kill to the dead window while the live one kept running. Each launch now records the window id it minted in the run dir, and the lookup prefers that id only after re-proving it against the live listing: a record that was killed or pruned, or whose id now carries another run's name, is ignored rather than replayed as a target that no longer resolves (an unresolvable -t lands on the active window). A launch that cannot record its own window forgets the previous record instead of leaving a superseded id authoritative. With no record the answer is unchanged. The re-prove pairs new_parked_window's id with the window_id column of list_windows, which the seam previously left free to diverge; both bundled backends already agree, and one that does not degrades to the ambiguous by-name resolve rather than mistargeting. The seam docstring and the adapter authoring guide now say so. Closes bmad-code-org#482
Review follow-ups on the bmad-code-org#482 fix: - Write the record atomically: it is read cross-process (bmad-loop attach), and on win32 an AV/indexer holding the previous record failed a plain overwrite into the forget path — atomic_replace retries exactly that violation, turning most real-world failures into successes, and a torn read can no longer degrade the lookup. - Forget the record with a plain unlink, not retrying_unlink: launches run on the Textual event loop, and dropping a best-effort hint is not worth ~5s of blocked win32 backoff. - resume_detached returns its window id and the TUI warns when it was not captured: resume is the launch that mints a second window under the run id, so a silently lost record re-created the bmad-code-org#482 symptoms behind a success toast (the resolve path already surfaced this). - Narrow _read_ctl_window's except to the UnicodeDecodeError its docstring documents, so an unrelated ValueError cannot hide as a degraded lookup. - Prose: the unresolvable -t hazard is psmux-specific (psmux/psmux#545; tmux merely errors), set_window_option is contractually best-effort, and new_parked_window's docstring now cross-references its required id form. - Docs: the run-dir inventories in FEATURES.md and the TUI guide list the ctl-window sidecar. - Tests: pin the is_run guard against a run-dir-shaped non-run (the missing-dir case alone also passes via the OSError swallow), record-before-tag ordering under a raising tag, trailing-newline records, the session:@n shape psmux actually emits, the resume warning, and the CLI attach project value.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 47 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 (16)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change persists detached control-window IDs in per-run sidecar files, validates them against live windows, and updates TUI attach, resume, and stop flows to use project-scoped resolution. Window transport and adapter ID contracts now preserve exact targeting. ChangesControl-window targeting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TUIApp
participant Launch
participant Multiplexer
TUIApp->>Launch: resume_detached(project, run_id)
Launch->>Multiplexer: create detached control window
Multiplexer-->>Launch: return window ID
Launch->>Launch: persist ctl-window
TUIApp->>Launch: ctl_window_id(project, run_id)
Launch->>Multiplexer: list live windows
Multiplexer-->>Launch: return matching window
Launch-->>TUIApp: return validated window ID
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.
Actionable comments posted: 4
🤖 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 `@CHANGELOG.md`:
- Around line 163-178: Condense the CHANGELOG entry describing live
control-window targeting into one concise, imperative action statement,
preserving the core outcome that attach, return-stamp, and kill now use each
run’s recorded live window. Remove the causal narrative, implementation details,
adapter guidance, and fallback behavior already covered elsewhere.
In `@docs/adapter-authoring-guide.md`:
- Around line 139-143: Remove the stray closing parenthesis after issue
reference (`#482`) in the paragraph describing new_parked_window and list_windows,
leaving the sentence ending with the reference’s required punctuation only.
In `@tests/test_tui_launch.py`:
- Around line 260-271: Apply the existing force_tmux_backend fixture to every
test using _ctl_listing for tmux argv assertions, including the cases around
lines 347-354, 357-368, and 371-377. Ensure those tests explicitly select the
tmux backend before exercising the command-building logic, without changing
_ctl_listing itself.
- Around line 533-535: In the fake helper, replace the redundant conditional
assignment to rc with a direct zero value, preserving the existing
CompletedProcess behavior.
🪄 Autofix
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 Plus
Run ID: 870cb7a1-00ee-47cf-8886-38631359cece
📒 Files selected for processing (10)
CHANGELOG.mddocs/FEATURES.mddocs/adapter-authoring-guide.mddocs/tui-guide.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_cli.pytests/test_tui_app.pytests/test_tui_launch.py
…, drop a useless conditional
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
_record_ctl_window's docstring promises that a failed write must not fail the launch — the window is already running by then, and the lookup has a documented degrade to the name scan. `except OSError` did not hold that promise: atomic_write_text resolves the path before its own try, and below 3.13 Path.resolve reports a symlink loop as RuntimeError, so a run dir reached through a looping link crashed the launch on the 3.11/3.12 legs. Widen to `except Exception`, keeping the _forget_ctl_window call in the arm — the same widening, for the same resolve-before-try reason, the engine's deferred-close rollback already carries. Not BaseException: a genuine KeyboardInterrupt still gets out. _forget_ctl_window's own except stays OSError; unlink never resolves. The test injects the fault rather than building a real symlink loop: 3.13+ resolves loops without raising, so a loop-based version would be green on the interpreter the suite usually runs and red only in CI.
|
Pushed one maintainer commit ( The gap: The new test injects the No CHANGELOG entry — the record-write path is introduced by this same unreleased PR, so the existing #482 entry already describes the behaviour and it is unchanged. |
|
@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: 50bab0e11b
ℹ️ 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".
| if not runs.is_run(run_dir): | ||
| return | ||
| try: | ||
| atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id) |
There was a problem hiding this comment.
When a previous coding session creates .bmad-loop/runs/<id>/ctl-window as a symlink, the next resume or resolve reaches this call after launching the window and overwrites the symlink target. atomic_write_text explicitly resolves and follows symlinks, and coding sessions have the same workspace permissions as the orchestrator, so a session can redirect this new host-side write to a project configuration, source file, or other user-writable path; because the write succeeds, the broad exception handler does not protect against it. Reject redirected records or use a no-follow, directory-anchored atomic write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d31f08b — thanks, this one was real.
Verified the premise first: atomic_write_text follows a symlink deliberately, and says so — src/bmad_loop/platform_util.py:253, "Symlinks are followed. path.resolve() first, so a ledger symlinked into the repo keeps being a symlink and the real file is what gets rewritten" — pinned by tests/test_platform_util.py:303. That contract is right for a user-curated ledger; this sidecar is the opposite (machine-minted, per-run, disposable), and runs.RUNS_DIR is .bmad-loop/runs (src/bmad_loop/runs.py:29), i.e. under the project root every coding session can write.
I reproduced it rather than reasoning about it. Ablating the new guard and running the new test writes @7 straight into a symlinked pyproject.toml:
> assert outside.read_text(encoding="utf-8") == "[project]\n"
E AssertionError: assert '@7' == '[project]\n'
One calibration on the impact, not a dispute: the payload is a tmux window id, not attacker-chosen content, so the primitive is truncation of a user-writable file rather than injection into it. And the writer runs as the same uid as the session, so under an unsandboxed adapter it grants nothing new. What makes it worth fixing is the sandboxed case — an adapter confining a session to workspace-write, where redirecting a host-side write is exactly the confinement being stepped around.
The fix refuses rather than following, and drops the link (Path.unlink never follows one, so the target is untouched): the launch still succeeds and the lookup degrades to the name scan, the same fallback every other record failure takes. Same refusal shape as the session-supplied worktree config path at src/bmad_loop/worktree_flow.py:645-651, which already treats this threat as real.
Residual, documented in the docstring rather than papered over: this is a check-then-write, so a session that re-plants between the probe and the replace still wins the race. Closing that needs an O_NOFOLLOW write, which does not port to the win32 leg this record is atomic for. What the guard removes is the standing redirect.
…ough it atomic_write_text resolves the path first — a documented contract that serves a user-curated ledger, not a machine-minted per-run sidecar. The run dir lives under the project root every coding session can write, so a planted link aimed this host-side write at any user-writable path, reach a workspace-confined adapter otherwise denies. The payload is a window id, so the primitive is truncation, not injection. Refuse and drop the link; the launch still succeeds and the lookup degrades to the name scan, exactly as the other record failures do.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31f08b877
ℹ️ 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".
| try: | ||
| redirected = record.is_symlink() | ||
| except OSError: | ||
| redirected = True | ||
| if redirected: | ||
| _forget_ctl_window(project, run_id) |
There was a problem hiding this comment.
When a workspace process replaces ctl-window with a symlink after this is_symlink() check but before atomic_write_text, that helper resolves and follows the newly planted link, so a coding session can still redirect the host-side overwrite to another user-writable path. Fresh evidence beyond the prior comment is that the current implementation's own docstring explicitly acknowledges that this replanting race still wins; use a no-follow or directory-anchored atomic replacement instead of a preflight check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken — fixed properly in d5acf47. You're right, and my previous docstring was wrong in a way worth naming: it dismissed the race citing only the O_NOFOLLOW route (genuinely not portable to the win32 leg) and never addressed your second suggestion, the directory-anchored replace. That one is portable, and it closes the race outright.
The check is gone entirely. atomic_write_text grew a keyword-only follow_symlinks=False, which skips exactly one line — the path.resolve() at src/bmad_loop/platform_util.py:314 that made it follow in the first place. os.replace does not dereference its destination, so a link planted at any moment, including one planted after a check would have run, is clobbered rather than written through. No preflight, so no window to win.
Additive and default-preserving, so every existing ledger caller keeps the follow-the-link contract it was built for; the sidecar is the only opt-out. I put it in the shared helper rather than forking a temp-write-and-replace into the TUI module so the fsync ordering, temp naming, win32 sharing-violation retry, and cleanup-on-failure stay in one place.
One thing your comment didn't raise that fell out of it: with the resolve skipped, target.exists() would still have followed the link to copy its target's mode onto the new record — letting a planted link choose the record's permissions. Not inherited any more (_is_plain_file); a fresh record keeps mkstemp's 0600.
Replacing also self-heals where refusing did not — the record ends up a plain file again, instead of leaving the planted link in place for the next launch.
Ablated all three gates singly: dropping follow_symlinks=False at the call site reddens the launch test; ignoring it inside _atomic_write reddens both platform_util tests; removing only the mode guard reddens only the mode test.
| except launch.LaunchError as e: | ||
| self.notify(str(e), severity="error") | ||
| return | ||
| if not win_id: |
There was a problem hiding this comment.
When the sidecar write fails because of permissions, disk errors, or symlink refusal, _record_ctl_window removes the record but start_detached still returns the truthy minted ID, so this condition emits no warning even though subsequent attach/stop operations fall back to the older first-match behavior that this change fixes. Propagate the persistence outcome or verify the record before reporting an unqualified successful resume.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in d5acf47. The coherence argument holds: the warning exists precisely to say "the disambiguation is gone, a/x may hit an older window", and an unwritten record leaves the operator in exactly the place an uncaptured id does — so reporting only one of the two was arbitrary. My symlink fix in the previous round made this worse, not better, by adding another silent way to get there.
Took the second of your two remedies (verify the record) rather than the first (propagate the outcome), because propagating means changing start_detached's return contract, which start_resolve_detached and its tests also depend on — a wider blast radius than this warrants. resume_detached now re-reads the record and returns None unless it names the window just minted:
win_id = start_detached(project, ["resume", ...], run_id, "resume")
if win_id and _read_ctl_window(project, run_id) != win_id:
return None
return win_idThat has the side benefit of asking the question the consumers actually ask — will ctl_window_id prefer this window — of the same file they read, rather than a proxy for it. So it also covers a run dir pruned mid-launch, which neither the write's return value nor the captured id would have caught.
app.py's toast moved from "window id was not captured" to "not recorded", since it now covers both causes. Two tests pin the halves (returns None when the write fails, returns the id when it lands); removing the check reddens the first and nothing else.
On the AGENTS.md reference — for what it's worth, the lines cited (L74-77) are the Testing section rather than the fail-loud doctrine, but the underlying point stands on its own and I've treated it as valid.
There was a problem hiding this comment.
Correction to my own last paragraph: the AGENTS.md citation was accurate and my quibble was not. AGENTS.md:70 starts "## Engineering doctrine", which puts L77 at "Fail loud at boundaries: typed escalation over bare except; observation may degrade, repair writes must raise" — precisely the rule the finding invoked. I misread the section offset. The fix in d5acf47 is unchanged.
Supersedes the preflight is_symlink check: a check leaves a window between itself and the write that a re-planting session wins. os.replace does not dereference its destination, so an atomic_write_text growing follow_symlinks=False clobbers a link planted at any moment. Mode and xattrs stop being inherited there too — a name being replaced rather than updated should carry nothing of what it used to point at. Also fold the unwritten record into resume's existing warning: an uncaptured window id and a record that did not survive both leave ctl_window_id on the ambiguous scan, so reporting only the first hid the rest behind an unqualified success toast.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5acf47cd7
ℹ️ 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".
| minted would answer a different window than the CLI does.""" | ||
| record = runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE | ||
| try: | ||
| text = record.read_text(encoding="utf-8") |
There was a problem hiding this comment.
When a coding session replaces its workspace-writable ctl-window with a FIFO or a symlink to an endless source such as /dev/zero, this synchronous read_text() blocks indefinitely or consumes memory until failure. action_attach performs this read on Textual's event loop, so merely pressing a can freeze the dashboard; CLI attach and stop are affected as well. Open only a regular, non-symlink record and cap the bytes read so malformed hints degrade to the name scan instead of hanging the observer.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d203f4e. Both halves reproduce, and the second one is worse than described.
FIFO: with a 5s alarm around _read_ctl_window, the process died to SIGALRM rather than returning — the open blocks indefinitely, exactly as you said, and action_attach (src/bmad_loop/tui/app.py:410) calls it inline on the event loop, so a freezes the dashboard.
/dev/zero: MemoryError after 0.32s under a 2GB ulimit -v. The part worth flagging is that MemoryError is not an OSError, so it was not merely a slow failure — it escaped _read_ctl_window's except (OSError, UnicodeDecodeError) and its documented "Never raises", straight out through action_attach, which has no handler at all. That is a crash, not a degrade.
The read now opens with O_RDONLY | O_NOFOLLOW | O_NONBLOCK, checks S_ISREG on the opened descriptor — fstat describes the object actually opened, so unlike a path check it cannot be raced — and reads at most 256 bytes. The POSIX-only flags degrade to 0 on win32, where the cap and the regular-file check carry alone. I chose the cap over widening the except to Exception deliberately: it removes the condition rather than absorbing it, and a blanket catch there would mask real bugs.
Worth reporting because it changed the patch: my first two tests for this were vacuous, and only ablation caught them.
- The FIFO test raised
TimeoutErrorfrom the alarm handler — which is a subclass ofOSError, so_read_ctl_window's ownexcept OSErrorswallowed it and the ablated build still returnedNone. It passed against the bug, five seconds slower. It raises a bare customExceptionnow. - The
/dev/zerotest never exercisedS_ISREGat all —O_NOFOLLOWrefused the link first, so deleting theS_ISREGcheck left every test green. It is pinned now by a FIFO with a writer holding bytes queued: the open does not block and the path is not a link, so neither other guard fires, and without the check the queued@2is simply read — a session forging the record through a pipe it controls.
Each of the four guards (O_NONBLOCK, O_NOFOLLOW, S_ISREG, the cap) now reddens exactly one test when removed singly.
| atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id, follow_symlinks=False) | ||
| except Exception: | ||
| _forget_ctl_window(project, run_id) |
There was a problem hiding this comment.
When this write fails during start_resolve_detached because the run directory is read-only, full, or concurrently pruned, the exception is swallowed and the minted ID is still returned, so _launch_resolve reports no degradation and attaches successfully. After cmd_resolve detaches the client and resumes the engine in that window, later attach/stop operations fall back to the older ambiguous match. Fresh evidence after the prior persistence-warning fix is that only resume_detached rereads the record; the resolve path still treats capture as proof of persistence, so propagate a separate persistence outcome and warn without losing the captured ID needed for the initial attach.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in d203f4e. The asymmetry was real and it was mine — last round I fixed the resume path's silent degradation and left _launch_resolve with the same gap, so the fix was half-applied.
Took your framing directly: propagate a persistence outcome without losing the captured id, since the resolve path needs that id for its immediate attach. Split out of resume_detached's return into a shared helper both launchers use:
def ctl_window_recorded(project: Path, run_id: str, win_id: str) -> bool:
return _read_ctl_window(project, run_id) == win_id_launch_resolve now warns and attaches anyway — the window it minted is reached by the id in hand, so only the later verbs are degraded, which is what the toast says. resume_detached keeps folding it into the return instead, because resume launches and leaves: it has no use for an id it cannot record. Same signal, different shape, and the difference is now stated in both docstrings rather than implied.
Two tests: the degraded path warns and still attaches to @7; the healthy path asserts no warning fires (that existing test previously wrote no record at all, so it would have started warning spuriously — it now records the window, which is what a real launch does). Removing the check reddens the first and nothing else.
One scope note, flagged rather than acted on: cmd_resolve in the CLI reaches the same launcher outside the TUI, so it has no toast to carry this. That is pre-existing rather than introduced here, and wiring an operator-visible signal through the CLI path is a wider change than this PR should absorb — happy to file it as a follow-up issue if you'd rather it not ride along.
There was a problem hiding this comment.
Correcting my own last paragraph before it becomes a spurious follow-up: the CLI scope note was wrong. cmd_resolve does not reach start_resolve_detached — it runs the resolve session in the foreground of the invoking terminal and never mints a ctl window. Grepping the tree, both launchers have exactly one caller each, and both are in the TUI:
src/bmad_loop/tui/app.py:518: win_id = launch.start_resolve_detached(self.project, run_id)
src/bmad_loop/tui/app.py:722: win_id = launch.resume_detached(self.project, run_id)
So there is no uncovered CLI path and nothing to file. Every launcher that mints a ctl window now reports the record outcome. The fix in d203f4e is unchanged; only my note about it was wrong.
The record sits under the project root every coding session can write, and action_attach reads it on Textual's event loop, so the shape at the path has to be settled before any bytes are consumed. A FIFO planted there blocks the open indefinitely — a keypress freezes the dashboard — and a link to an endless source raises MemoryError, which is not an OSError and so escaped the "never raises" promise entirely, out through a caller with no handler. O_NONBLOCK + S_ISREG on the opened descriptor (fstat cannot be raced), O_NOFOLLOW, and a 256-byte cap. Also give the resolve path the record signal resume already had, via a shared ctl_window_recorded: it keeps the captured id it attaches with and warns only that later attach/stop are back on the ambiguous scan.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bmad_loop/platform_util.py`:
- Around line 341-344: Close the no-follow inheritance race in the
target-handling flow by replacing the separate _is_plain_file probe and
shutil.copymode call with one no-follow stat that captures regular-file
permission bits, then apply those bits to the temporary file descriptor without
re-resolving target. Update _copy_xattrs to use no-follow listxattr/getxattr
behavior, or skip xattr inheritance when no-follow semantics cannot be
guaranteed; preserve the documented behavior that a replaced symlink carries
nothing from its former target.
In `@tests/test_tui_launch.py`:
- Around line 387-398: Update test_read_record_rejects_a_non_regular_file’s
comment to describe coverage of the O_NOFOLLOW guard when opening a symlink
targeting a device, removing claims about S_ISREG and unreachable MemoryError
behavior. Keep the test setup and assertion unchanged, since
test_read_record_rejects_a_fifo_that_already_has_data already covers the
opened-descriptor S_ISREG check.
🪄 Autofix
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 Plus
Run ID: 7d34ae0a-8f4a-43f1-ad06-9ffcc179202b
📒 Files selected for processing (6)
src/bmad_loop/platform_util.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_platform_util.pytests/test_tui_app.pytests/test_tui_launch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bmad_loop/tui/launch.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d203f4ee65
ℹ️ 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".
| should report it rather than let an unqualified success toast imply the | ||
| targeting is sound. Split out from resume_detached's return so the resolve | ||
| path can warn while still keeping the captured id it attaches with.""" | ||
| return _read_ctl_window(project, run_id) == win_id |
There was a problem hiding this comment.
For a registered external backend whose new_parked_window ID differs from the window_id returned by list_windows—a compatibility case the updated seam explicitly says degrades to name lookup—this returns true merely because the sidecar round-tripped. resume_detached and the resolve launcher therefore omit the degradation warning, even though ctl_window_id rejects that ID and may select the older same-run window. Check the same live-listing/name condition used by ctl_window_id, not only file equality.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c2fdffe. This one was self-inflicted: the helper's own docstring claimed "whether ctl_window_id will now prefer win_id", and then it compared the file instead — a weaker claim wearing the stronger one's description. The divergence you name is the case where the two come apart, and it is documented as tolerated by this very PR (src/bmad_loop/adapters/multiplexer.py, the list_window_ids note: a backend that diverges "remains usable, but falls back to the ambiguous by-name lookup"). So the record round-trips, ctl_window_id rejects it against the listing, and the warning that exists for precisely that ambiguity stayed silent.
It now asks the real question, which also made it shorter:
try:
return ctl_window_id(project, run_id) == win_id
except MultiplexerError:
return FalseTwo consequences worth flagging, since both change behaviour beyond the finding:
- No warning when only one window carries the run id. With nothing to disambiguate the scan is already right, so a missing record is not a degradation. File equality would have warned on every such launch — crying wolf where targeting is sound. New test pins it.
- An unreachable listing counts as not recorded.
ctl_window_idcan raiseMultiplexerError, and neither_do_resumenor_launch_resolvehas a handler for it, so propagating would crash the TUI after a launch that already succeeded. Degrading matches the toast's own hedge ("may target an older window") and the repo's observation-degrades doctrine. Also pinned — removing the catch reddens that test alone.
Cost is one extra list-windows per resume/resolve launch. I updated test_existing_ctl_session_reused, which pins the exact verb sequence, rather than leaving the new call unasserted.
Test fakes were strengthened alongside: FakeRun now scripts list-windows instead of returning empty, and the resume signal tests model #482's real shape — a parked run-RID listed ahead of the live resume-RID — so "degraded" means the scan actually answers the corpse rather than merely that a file is missing.
ctl_window_recorded compared the sidecar to the id just minted, but a round-tripped record is a weaker claim than the one its docstring makes. A backend whose new_parked_window id is shaped differently from its list_windows window_id column — a divergence the seam tolerates — reads the record back intact while ctl_window_id rejects it and falls through to the first match, so both launchers stayed silent in exactly the case the warning exists for. Ask ctl_window_id instead. Consequences: no warning when only one window carries the run id (the scan is right, so there is nothing to report), and an unreachable listing counts as not recorded rather than raising into launchers that have no handler for it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2fdffeffa
ℹ️ 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".
| if not mux_available(): | ||
| return None | ||
| matches: list[str] = [] | ||
| for win_id, name in get_multiplexer().list_windows(CTL_SESSION, ["window_id", "window_name"]): |
There was a problem hiding this comment.
When two projects have the same caller-supplied run ID in the shared control session, a stale or workspace-writable ctl-window record in project A can name project B's window; this listing checks only the name suffix, so the recorded ID passes the re-proof and attach/stop can select or kill B's orchestrator window. Include runs.PROJECT_OPTION in this lookup and require the recorded window's tag to match runs.project_tag(project), as the prune path already does for this shared session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken — fixed in b2a0b81, with one honest caveat about attribution below.
Verified the mechanism you pointed at: _ctl_window_candidates already lists ["window_id", "window_name", runs.PROJECT_OPTION] and scopes by runs.project_tag(project) (src/bmad_loop/tui/launch.py:464-480), and start_detached already stamps that tag on every window it mints. So this was one column and one predicate, mirroring a convention 40 lines away rather than inventing one.
I copied its untagged rule verbatim, and it matters here for a reason worth naming: the tag is written by a set_window_option the seam declares best-effort, and this PR has a test (test_record_survives_a_raising_window_tag) for a backend that raises from it. A bare tag == mine filter would have made that window unreachable to a/x — trading a cross-project bug for a same-project one. So: tagged-elsewhere is skipped, untagged is admitted exactly when this project holds the run dir.
Attribution caveat, since I would rather state it than let the fix imply otherwise: the cross-project collision is pre-existing, not introduced here. Before this PR the scan already matched on name suffix alone and returned matches[0], so a neighbouring project's window with a lower window index was already a possible answer. What this PR changes is that the record makes the selection deterministic rather than index-order luck. I fixed it anyway on two grounds: ctl_window_id's docstring in this PR sells an "unambiguous lookup", and cross-project is an ambiguity it did not resolve; and x killing a live orchestrator next door is bad enough that a six-line adjacent fix beats filing it.
Four tests, each ablated singly: a neighbour's window is skipped; a record naming the neighbour's window is ignored rather than replayed; untagged + local run dir is admitted; untagged without one is refused (removing just the elif reddens that last one alone).
Test fakes updated too — _ctl_listing now tags rows for the project by default, since that is the state a real launch leaves, and takes an explicit third field to script another project's window or the untagged case.
The control session is shared across projects and --run-id is caller-supplied, so a same-run_id window next door was a legal match for a name-suffix scan — and for kill_ctl_window that means closing a live orchestrator in another project. Filter on the PROJECT_OPTION tag start_detached already stamps, on _ctl_window_candidates' rule: a window tagged elsewhere is skipped, an untagged one is admitted only when this project holds the run dir, so a failed (best-effort) tag write leaves the window reachable by its own project rather than by nobody. The record is re-proved against the scoped matches, so it cannot smuggle a neighbour's window back in either.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
The new test asserts tmux argv through the multiplexer seam, so the win32 legs drove psmux and reddened on the backend name. Same force_tmux_backend marker its sibling test_resolve_escalation_launches_and_attaches already carries. Reproduced locally with BMAD_LOOP_MUX_BACKEND=psmux: red without the marker, green with it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd64a935db
ℹ️ 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".
| if not runs.is_run(run_dir): | ||
| return | ||
| try: | ||
| atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id, follow_symlinks=False) |
There was a problem hiding this comment.
When a workspace process replaces .bmad-loop/runs/<run_id> (or another ancestor below the project) with a symlink to an external directory containing state.json, runs.is_run follows it and this write does too: follow_symlinks=False protects only the final ctl-window component. The host process can therefore create or overwrite ctl-window outside the confined workspace—for example, corrupting another project's run record. Fresh evidence beyond the earlier final-component symlink fixes is that the current helper passes the unresolved parent path to both mkstemp and os.replace; reject symlinked ancestors or perform a directory-anchored no-follow traversal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f1c60e1. Reproduced it before believing it — an ancestor link with a state.json behind it:
is_run says: True
escaped write: True '@7'
So runs.is_run follows the link, mkstemp/os.replace take the unresolved parent, and the record lands in the linked-to directory. _run_dir_is_confined now lstat-checks every component below the project before the write.
Two calibrations, offered as scoping rather than pushback:
- The reach is narrower than the final-component escape. The name written is always
ctl-window, so this clobbers another project's record — as you say — rather than an arbitrary file. Still worth closing; it is the same shape. projectitself is deliberately not checked. The operator chooses where the project lives and may legitimately keep it behind a symlink; everything below it is session-writable. Same boundaryworktree_flowdraws at the worktree.
On the remedy choice: I took "reject symlinked ancestors" over "directory-anchored no-follow traversal". The race-free walk needs dir_fd, which POSIX has and win32 does not — and this record is atomic specifically for the win32 leg. So unlike the final-component case, where you correctly showed me a portable race-free option existed and I took it, here I have not found one, and I would rather say that plainly than imply the check is airtight. It removes the standing redirect (plant a link, wait for a launch); a session re-planting inside the check/write window still wins. Documented in the docstring, bounded by the same two facts: same uid as the writer, fixed filename, window-id payload.
The read path is left following ancestors on purpose: a foreign ctl-window read that way still has to survive recorded in matches, so it cannot become a target — only a value that gets discarded.
| if tag: | ||
| if tag != mine: | ||
| continue # another project's window | ||
| elif not local: | ||
| continue # untagged and no run dir here — ownership unprovable |
There was a problem hiding this comment.
When projects A and B share a caller-supplied run ID, B has a legacy or failed-tag untagged window, and A has a correctly tagged window, this branch admits B's window merely because A has a local run directory. If B's row is listed first and A has no usable record—as happens for a fresh run, for which recording is intentionally skipped—A's attach or stop resolves B's window and can kill B's orchestrator despite A's tagged candidate being available. Fresh evidence after the earlier project-scope fix is that tagged and untagged matches are appended to the same ordered list; use untagged candidates only when no candidate explicitly tagged to this project exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in f1c60e1 — this was a genuine hole in my own previous round's fix. I admitted untagged windows on local, which is a fact about this project's run dir and says nothing about whose window the untagged row is; merged into one listing-ordered list they then competed on index. Your run example is the sharp end of it: recording is deliberately skipped there, so the record cannot break the tie.
Untagged is now a fallback rather than a peer — matches = tagged or untagged, so an untagged row is consulted only when nothing carries this project's tag.
Worth reporting, because it changed what I trust: my first ablation of this did not bite. I ablated to tagged + untagged, which still puts tagged first, and the test passed — so it briefly looked verified when it was not. The real pre-fix shape is a single list appended in listing order; reverting to that reddens the test. Same lesson as the vacuous FIFO test two rounds ago: the ablation has to reconstruct the actual prior behaviour, not merely a plausible-looking mutation.
One process note, offered for your judgement rather than as a disagreement. This is round 7, and rounds 5–7 have each found a real defect in the previous round's fix — project scope, then untagged-vs-tagged ordering, then ancestor symlinks. Each fix has been small and each finding legitimate, but the pattern says the sidecar's threat surface is broader than a targeting bugfix set out to cover. If the maintainers would rather keep #482 to the identity fix, the workspace-writability hardening could reasonably be split into a follow-up issue and this PR held to the lookup change. Happy either way — I have kept everything landed and green, so there is nothing blocking if you would prefer it stays together.
|
@coderabbitai — all four items from the review on Taken
Also from your earlier reviews, now cleared: Declined — The stated mechanism does not hold, and the proposed fix would introduce a worse bug. So the handler never sees a live descriptor for any failure It raises A leak is reachable only for an exception raised before Full run: |
|
@codex review |
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
I agree with the decline. Your The remaining asynchronous pre-ownership window is not a deterministic issue that warrants added wrapper construction in this PR. The finding is withdrawn. The accepted fixes and the full verification results look good. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/bmad_loop/platform_util.py --items all --type function --match 'atomic_write_text_at|atomic_write_text'
ast-grep outline tests/test_tui_launch.py --items all --type function --match '_fail_the_record|test_confinement_check_refuses_an_unprobeable_ancestor|test_resume_reports_a_record_that_did_not_survive|test_resume_returns_the_id_when_the_record_survives'
ast-grep outline tests/test_runs.py --items all --type function --match 'transportable|separator'
printf '\n--- platform implementation matches ---\n'
ast-grep run --lang python --pattern $'def atomic_write_text_at($$$):\n $$$' src/bmad_loop/platform_util.py
printf '\n--- targeted test and documentation matches ---\n'
rg -n -C 6 \
-e 'def _fail_the_record\b' \
-e 'def test_confinement_check_refuses_an_unprobeable_ancestor\b' \
-e 'def test_resume_reports_a_record_that_did_not_survive\b' \
-e 'def test_resume_returns_the_id_when_the_record_survives\b' \
-e '2028|2029|\\u2028|\\u2029' \
tests/test_tui_launch.py tests/test_runs.py
printf '\n--- adapter-guide issue-reference matches ---\n'
rg -n -C 2 '`#482`' docs/adapter-authoring-guide.mdLength of output: 2362 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/bmad_loop/platform_util.py: atomic_write_text_at ---'
sed -n '390,480p' src/bmad_loop/platform_util.py
printf '%s\n' '--- tests/test_tui_launch.py: confinement and record helpers ---'
sed -n '1000,1135p' tests/test_tui_launch.py
printf '%s\n' '--- tests/test_runs.py: line-separator parametrization ---'
sed -n '625,705p' tests/test_runs.py
printf '%s\n' '--- docs/adapter-authoring-guide.md: `#482` reference ---'
rg -n -C 2 '`#482`' docs/adapter-authoring-guide.md
printf '%s\n' '--- changed diff for inspected files ---'
git diff --unified=12 origin/main...HEAD -- \
src/bmad_loop/platform_util.py \
tests/test_tui_launch.py \
tests/test_runs.py \
docs/adapter-authoring-guide.mdLength of output: 93996
The remaining pre-ownership asynchronous-exception window does not justify that change. I also verified the applied fixes for the escaped line separators, the delimiter-test documentation, the ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f765f9e3c8
ℹ️ 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".
| # documented fallback in _ctl_window_candidates — so even a | ||
| # non-conforming backend raising from the (contractually best-effort) | ||
| # set_window_option must not cost the record. | ||
| _record_ctl_window(project, run_id, win_id) |
There was a problem hiding this comment.
When two TUI instances resume or resolve the same paused run concurrently, both can pass the liveness check and mint windows before either detached child establishes engine liveness. These unsynchronized writes can then complete out of mint order, so the earlier window overwrites the later window's ctl-window identity; a failed earlier write can likewise unlink the later record via its cleanup path. Attach and stop will consequently target a parked or failed launcher while the other window drives the run. Serialize detached launches per run or make the record update generation-aware so an older launch cannot overwrite or erase a newer identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and reproduced — but split out to #530 rather than fixed here, because the root cause is not in this PR.
Both halves of what you describe reproduce exactly:
launch._record_ctl_window(project, "RID", "@7") # the LATER window
launch._record_ctl_window(project, "RID", "@5") # the earlier one lands after
ctl_window_id(project, "RID") -> "@5" # the OLDER window wins
launch._record_ctl_window(project, "RID", "@7")
launch._forget_ctl_window(project, "RID") # a failed launch's cleanup
record exists: False
ctl_window_id(project, "RID") -> "@5" # back to listing order
Where I differ is on which defect this is. The reason two launches exist at all is that cmd_resume gates on runs.engine_liveness(run_dir) and then launches with no lock across the two steps, while engine.pid is not written until the detached child starts — so both resumes read a non-alive liveness and both proceed. That is an unlocked check-then-act, it is on main, and this PR does not touch cli.py at all (git diff origin/main...HEAD -- src/bmad_loop/cli.py is empty). The gate's own message is the tell:
run <id> is still live — resuming would double-drive it; stop it first
So the first-order consequence of your trigger is two engines driving one run dir, and the stale record is downstream of that.
That is why I am not taking either remedy you offer here. Making the record generation-aware would give correct window targeting for one of two engines that are both writing the same run dir — a worse state than an ambiguous attach, because it looks right. And mint order cannot be recovered after the fact without either a generation source or the lock that is missing to begin with. Serializing launches is the correct fix, but it belongs where the check-then-act is, not in the record.
This is the same call I made on #528: a real defect, in code this PR does not touch, filed rather than folded. For contrast, the P2 you raised last round was fixed here (fdbb684) because it lived entirely in this PR's own new code.
#530 has the reproduction, the root cause, and a suggested shape — an exclusive per-run lock spanning the liveness check and the launch, using platform_util.file_lock, which install.py already uses this way.
One note for the next pass: if you re-report this on an unchanged head, it is a known deferral to #530 rather than a new finding.
|
@coderabbitai full review |
|
@codex review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f765f9e3c8
ℹ️ 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".
| rows = get_multiplexer().list_windows( | ||
| CTL_SESSION, ["window_id", "window_name", runs.PROJECT_OPTION] | ||
| ) |
There was a problem hiding this comment.
On POSIX/tmux, when the project path contains any non-UTF-8 filename byte, str(project.resolve()) contains a surrogateescape character and project_tag() leaves it raw because _survives_listing() checks only line separators. This new option column makes tmux emit that original byte, but BaseTmuxBackend._run() decodes captured output with the locale's strict error handler, so ctl_window_id() raises UnicodeDecodeError; TUI attach and stop therefore crash for these otherwise valid project paths. Encode filesystem bytes (for example via os.fsencode() plus byte-oriented quoting) before storing the tag, or decode listings with a reversible error handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2cfd10a — and it was worse than you described, in a way that was entirely my own doing.
Reproduced the whole chain first, on a real proj\xff directory:
surrogates in resolved str: ['0xdcff']
_survives_listing(raw): True <- left it RAW
quote(raw, safe=""): UnicodeEncodeError
strict decode of a listing: UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 16
Line 2 and line 4 are your finding, exactly as stated. Line 3 is one you did not mention and it is the sharper half: quote(raw, safe="") defaults to strict UTF-8, so a path holding both a separator and a non-UTF-8 byte reached my new encoder and crashed inside project_tag itself — the function added to prevent a mismatch would instead raise. That is a defect I introduced in the previous round's fix, one round earlier.
Both halves fixed:
_survives_listingnow asks both questions the transport asks — does the row split, and can the value be encoded at all — instead of only the first. Same rule as last round: ask the transport's own question rather than enumerate characters. Naming only separators is how a surrogate got reported as safe.quote(..., errors="surrogateescape"), which turns the surrogate back into the original byte before percent-encoding it. The tag is now pure ASCII for these paths, so nothing undecodable is written into the listing at all.
project_tag: %enc%%2Ftmp%2F.../proj%FF ascii: True survives: True
sep+badbyte: %enc%%2Ftmp%2F...%2Fp%0Aq%FF ascii: True (previously raised)
Ablations, singly: drop the encodability check -> 2 tests redden; drop errors="surrogateescape" -> 1 reddens, and only that one, so neither gate masks the other.
On the second remedy you offer — "decode listings with a reversible error handler" — that half is out of scope here and already tracked. My fix stops us writing an undecodable tag; it cannot help a tag some other version already stored raw, which still meets a strict decode. But that decode is BaseTmuxBackend._run's _ERRORS = None on POSIX, it is on main, and this PR does not touch it (git diff origin/main...HEAD -- src/bmad_loop/adapters/tmux_base.py shows no change to the codec settings). It is #380, which already names this precise case — including @bmad_project storing str(project.resolve()) — and marks it pre-existing. Same call as #530 last round: fix what this PR's own code got wrong, leave the pre-existing half with its issue.
4844 passed, pyright clean, trunk check --no-fix clean. The one unrelated red in my local run was test_decision_modal_scrolls_when_content_long, the known flake #360 — it passes on a rerun.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bmad_loop/runs.py`:
- Around line 298-328: Update project_tag to pass errors="surrogatepass" when
calling quote for paths that do not survive listing, preserving the existing
safe="" encoding and returning a stable transport-safe tag even when raw
contains surrogate escapes and splitlines separators.
In `@src/bmad_loop/tui/app.py`:
- Line 418: Update the control-window lookup at src/bmad_loop/tui/app.py:418 to
call ctl_window_id through _mux_guarded, and at src/bmad_loop/tui/app.py:899
catch MultiplexerError separately after runs.stop_run() so cleanup failures are
reported distinctly from run-stop failures; add regression coverage for both
paths.
In `@tests/test_tui_launch.py`:
- Around line 956-963: Fix the run_dir binding in the test around _write_record:
use the returned record path only for writing, and bind run_dir to the parent
run directory before checking runs.is_run and the control-window file. Update
the assertions to verify that the real run directory lacks state.json and that
its _CTL_WINDOW_FILE is removed, while retaining the `@7` lookup assertion.
Confirm the test fails if _forget_ctl_window is removed from
_record_ctl_window’s is_run early-return path.
🪄 Autofix
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 Plus
Run ID: 810783bb-01d1-48e3-9a0e-e5b84d69556e
📒 Files selected for processing (16)
CHANGELOG.mddocs/FEATURES.mddocs/adapter-authoring-guide.mddocs/tui-guide.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/platform_util.pysrc/bmad_loop/runs.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_cli.pytests/test_multiplexer.pytests/test_platform_util.pytests/test_runs.pytests/test_tui_app.pytests/test_tui_launch.py
| return | ||
| session = runs.session_name(run_id) | ||
| win_id = launch.ctl_window_id(run_id) | ||
| win_id = launch.ctl_window_id(self.project, run_id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle MultiplexerError for control-window lookup and cleanup.
ctl_window_id() performs list_windows(). kill_ctl_window() resolves a window and calls kill_window(). Both operations can raise MultiplexerError.
At Line 418, route the lookup through _mux_guarded. Otherwise, a failed listing escapes the Textual action handler.
At Line 899, catch MultiplexerError separately after runs.stop_run(). Do not report that the run stop failed when only control-window cleanup failed. Add regression coverage for both paths.
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.”
📍 Affects 1 file
src/bmad_loop/tui/app.py#L418-L418(this comment)src/bmad_loop/tui/app.py#L899-L899
🤖 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/app.py` at line 418, Update the control-window lookup at
src/bmad_loop/tui/app.py:418 to call ctl_window_id through _mux_guarded, and at
src/bmad_loop/tui/app.py:899 catch MultiplexerError separately after
runs.stop_run() so cleanup failures are reported distinctly from run-stop
failures; add regression coverage for both paths.
Source: Coding guidelines
The tag encoding asked only whether a value survived splitlines(), so a POSIX path holding a byte that is not valid in the filesystem encoding was still tagged raw. os.fsdecode leaves that byte as a lone surrogate, tmux emits the original byte, and the backend decodes captured output strictly - so the listing read raises UnicodeDecodeError and attach and stop crash outright rather than mismatching. _survives_listing now asks both questions the transport asks: does the row split, and can the value be encoded at all. quote() also gains errors="surrogateescape", without which the encoder itself raised UnicodeEncodeError on exactly the paths it exists to carry - a path holding both a separator and a bad byte reached it. Reading a tag another version stored raw is the decode half and stays with bmad-code-org#380.
|
@codex review |
|
@coderabbitai full review |
|
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cfd10a7b0
ℹ️ 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".
| _record_ctl_window(project, run_id, win_id) | ||
| # Tag the window with its project so a cleanup in another project never | ||
| # closes it (the ctl session is shared across projects). | ||
| mux.set_window_option(win_id, runs.PROJECT_OPTION, runs.project_tag(project)) |
There was a problem hiding this comment.
On native Windows, when the project path contains a valid filename character such as an apostrophe, semicolon, or non-breaking space, project_tag() leaves the path raw because it survives the listing parser, but PsmuxMultiplexer._transportable() rejects it and this option remains unset. If two such projects use the same explicit run ID, fresh TUI launches have neither a tag nor a ctl-window record, so attach/stop and control-window pruning can select the other project's window by listing order. Fresh evidence beyond the earlier separator/non-UTF-8 tag fixes is that psmux applies this additional transportability predicate; encode values it refuses before setting the project option.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real mechanism, already tracked — not folded in. Two of your three examples are also wrong, which narrows it further.
The examples. Measured against PsmuxMultiplexer._transportable rather than reasoned about:
PASSES apostrophe C:\Users\O'Brien Files\proj
PASSES mid-token semicolon C:\Users\a; b\proj
REFUSED NBSP C:\Users\a\xa0b\proj
REFUSED standalone ; token C:\Users\a ; b\proj
REFUSED spaced UNC \\srv\share name\proj
An apostrophe and a mid-token ; both pass — the predicate's own comment says so directly: "a spaced O'Brien Files or a; b path passes". Inside the client's double quotes ' is literal and a mid-token ; survives; only the whitespace-delimited ; token is cut. Your NBSP example is right, and it is the useful part of the report.
The refusal is silent, as you say. set_window_option warns to stderr and returns without raising (psmux_backend.py:567-576), freeing the key so a stale value cannot be replayed. So the window really is untagged and ctl_window_id really does fall to ownership-by-run-dir.
But this is #419, and psmux's own code comment says so. The session-scope twin of this branch carries the line "Both edges of that fallback are bounded in #419" (psmux_backend.py:517). #419 is open, names this exact fallback, and its closing section — "The fallback's other edge: ownership by run-id collision, not by identity" — is the collision you describe.
Three reasons it stays there rather than riding this PR:
- Pre-existing and untouched.
_transportableand the silent-refusal path are onmain, andgit diff origin/main...HEAD -- src/bmad_loop/adapters/psmux_backend.pyis empty. - Not a regression. Before this PR,
ctl_window_idtook no project argument at all — no tag, no run-dir gate, first name match from any project. The untagged path is strictly narrower than what it replaced. - The fix is a seam change, and Untagged sessions are weak ownership: they leak once their run dir is gone, and can be pruned by the wrong project on a run-id collision (the fallback behind #320) #419 already scoped it. Its direction (1) is "store a digest of the resolved path… both sides already route through
runs.project_tag… this is the direction I would take, and it is why the work did not ride fix(adapters): gate the psmux session project tag on transportability (#320) #418."project_taglives inruns.pyand cannot askPsmuxMultiplexer._transportablewithout inverting the dependency, so closing this means a new seam method the backend answers — adapter contract, docs, both backends. That is Untagged sessions are weak ownership: they leak once their run dir is gone, and can be pruned by the wrong project on a run-id collision (the fallback behind #320) #419's work, not a control-window targeting bugfix's.
Worth noting the convergence: this PR has already taken the first step of that direction. project_tag now percent-encodes behind a %enc% prefix when a value cannot ride the listing — a splitlines() separator, or a surrogateescaped byte. Conditionally, so a transportable path stays byte-identical and nothing stored is stranded, which sidesteps the read-side transition #419 flags as direction (1)'s cost. What is left is exactly the psmux control-line predicate.
I have added both increments your report surfaced to #419: NBSP as a reachable refusal shape its analysis had not listed, and ctl_window_id as a second, targeting consumer of the fallback.
Same rule as the last two rounds — fix what this PR's own code got wrong, leave the pre-existing half with its issue. Finding 19 was fixed here; the concurrency one went to #530; the decode half of the surrogate finding stayed with #380.
|
@codex 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". |
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.
2 participants
Closes #482
The defect
Control-session windows are named
<kind>-<run_id>over four kinds, so the name is not unique.ctl_window_idreturned the first matching row — on tmux the lowest-index one. The plain operator sequence in the issue (run → park →e) leaves a deadrun-<rid>window in front of the liveresume-<rid>, and all three consequences follow:aattaches to the parked corpse,set_return_panestamps the return target on it (leaving the live window with no way back to the operator's origin pane), andxkills it while the live one keeps running.The fix
Each launch records the window id it minted in the run dir (
ctl-window), and the lookup prefers that id — but only after re-proving it against the live listing. The record must be present in the listing and its row name must still end in-{run_id}, so a record that was killed or pruned, or whose id now names another run, is ignored rather than replayed. That matters because an unresolvable-tis not a harmless miss on psmux: it lands on the active window (psmux/psmux#545). With no record, the answer is byte-for-byte the previous first-match scan.This is the issue's second fix shape. The other two were considered and rejected:
<kind>-<run_id>— forces every call site to answer "which kind is current?", which is exactly the unknown being solved.Reaping the stale window at launch was left alone deliberately — the park exists to keep the exit status inspectable, and closing it on resume destroys that without being asked.
Seam contract
The re-prove pairs
new_parked_window's id with thewindow_idcolumn oflist_windows. The seam explicitly left those free to diverge ("nothing membership-tests a parked id"), so that licence is narrowed here, in the docstring and in the adapter authoring guide. Both bundled backends already agree (psmux qualifies both, #291). A backend that diverges is not broken — the re-prove simply never matches and the lookup degrades to the ambiguous by-name resolve, never to a mistarget.Degradation, stated
The record is a hint, never a target on its own. It is written atomically (a torn or overwrite-blocked record would silently degrade the lookup), reading it never raises — including
UnicodeDecodeError, which is not anOSErrorand which neitheraction_attachnor_stop_run_workerwould have caught — and a launch that cannot record the window it just minted forgets the previous record rather than leaving a superseded id authoritative. When that removal fails too, the surviving id still has to pass the re-prove, so the ceiling is the pre-fix by-name answer. A resume whose window id was not captured now warns instead of hiding the degraded lookup behind the success toast.Tests
Every row of the edge-case matrix is covered in
tests/test_tui_launch.py, plus the two call-site seams the previous tests stubbed out:test_attach_uses_the_recorded_ctl_window(TUI attach, does not monkeypatch the lookup), the strengthenedtest_story_checkpoint_stop_marks_stopped(TUI stop), andtest_attach_records_return_pane_inside_tmux(CLI attach) now assert the value of the project argument, not just its arity.Per the repo's ablation rule, each negative assertion was verified to FAIL with its guard removed: the listing re-prove, the name re-prove, the
is_runguard, the record-before-tag ordering, the decode catch, and the project argument at both TUI call sites.Verification
uv run pytest tests/test_tui_launch.py tests/test_tui_app.py tests/test_cli.py tests/test_psmux_backend.py— 723 passeduv run pyright— no new findings (the twoos.setxattr/getxattrerrors are pre-existing and Windows-only)trunk check— cleanKnown limitation, not fixed here
The name scan is still not scoped to the calling project — the ctl session is shared, so two projects holding windows for the same run id can resolve to each other's. Pre-existing, and this change narrows it (the record itself is project-scoped, so only the no-record path is exposed). Accidental collision needs the same second and the same 4 hex digits of
runs.new_run_id. Worth its own issue rather than widening this one.Summary by CodeRabbit