Conversation
A recorded control-window id is a handle, not an identity (bmad-code-org#750). The record could readmit an untagged neighbour's window in two ways: a window id reused after a control-server restart, or a record forged by anything able to write under the project root (CWE-284, raised on bmad-code-org#749). Measured before choosing the remedy: neither psmux 3.3.8 nor tmux 3.4 reuses window ids within one server's life (psmux's next_win_id only increments), but both restart at @1/@0 after a server restart. #{pane_pid} works as a list-windows field on both backends, so no seam change is needed. - The ctl-window record is now "<window id>\n<pane pid>". The pid is read at mint through the same list_windows column the lookup uses; if it cannot be read, the record keeps the id alone (best-effort, never fails the launch). - ctl_window_id admits an untagged row only when both the id and the pid match the listing. A pid-less record (an older release's) or a malformed one still breaks ties among tagged rows, and never admits an untagged row: otherwise a forger would simply write the old format. - The lookup never writes the record or the tag. Limits, stated in the docstring: a writer with mux access to the ctl server can read the pid, but that access can already set the tag directly. Pid reuse after a restart is a residual. An active-pane change fails closed. Known gaps: - An untagged window minted before this change has a pid-less record, so it is unreachable by a/x until relaunched. Tagged windows are unaffected. - After the last test-only edit, only test_tui_launch.py was re-run on Windows (112 passed). WSL ran all three touched files after it (926 passed). The rest of the Linux suite is unverified locally. Closes bmad-code-org#750
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cf2360e9e
ℹ️ 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): | ||
| _forget_ctl_window(project, run_id) | ||
| return | ||
| payload = f"{win_id}\n{pane_pid}" if pane_pid else win_id |
There was a problem hiding this comment.
When this writes @7\n4242, a downgraded or concurrently used pre-change release reads the entire stripped payload as the window ID. If both a tagged parked run-RID window and a tagged live resume-RID window exist, that value matches neither row, so the old lookup falls back to the first window and attach/stop can target the stale one, recreating bmad-code-org#482. Preserve the original single-ID record format and store the PID separately, or otherwise make the persisted representation backward-compatible.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 78a7193. ctl-window is again the window id alone, byte-identical to the pre-change format, so an older release reads it exactly as before. The pane pid moved to a sibling file, ctl-window-pid, written pid-first with the same atomic writer and read with the same hardening. A torn or racing pair fails closed (a newer pid never matches an older id), and admission rules are unchanged.
The previous commit wrote "<window id>\n<pane pid>" into ctl-window. A pre-change release reads that whole payload as the window id, matches no row, and falls back to the first tagged window, so with a parked run-RID beside a live resume-RID it could attach to or stop the stale one (the bmad-code-org#482 failure). ctl-window is again the window id alone, byte-identical to the old format. The pane pid moves to a sibling file, ctl-window-pid, written by the same atomic writer and read with the same hardening (O_NOFOLLOW, O_NONBLOCK, S_ISREG on the descriptor, size cap). - Writes go pid first, record second; lookup reads record first, pid second. A crash between the writes, or a lookup racing a relaunch, leaves a newer pid beside an older id, which no row satisfies, so it fails closed. - Admission is unchanged: an untagged row needs the id and the pid to match; a missing or malformed pid never admits it; tagged tie-break uses the id. - Forgetting a ctl window removes both files. Known gaps: - Two concurrent launches of the same run can interleave into one launch's id beside the other's pid. That fails closed (no untagged admission, tagged rows unaffected, the next launch repairs it) and is documented rather than locked. - Tests were updated but not run locally; CI verifies them. Refs bmad-code-org#750
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a7193549
ℹ️ 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".
| and win_id == recorded | ||
| and recorded_pid is not None | ||
| and pane_pid.strip() == recorded_pid |
There was a problem hiding this comment.
On hosts where same-UID processes can inspect the process table, code with only project-write access can find a target bmad-loop ... --run-id RID process, walk its parent chain to the pane shell, and forge both ctl-window and ctl-window-pid. If that target is an untagged window with the same run ID, this equality admits it even though this project never minted it, so x can still kill a neighboring project's orchestrator. A pane PID is an observable identifier rather than proof of ownership; this needs a nonce or authentication secret unavailable to the workspace writer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2122962 by removing the pid proof instead of hardening it. Measured: a pane's environ exposes the tmux socket path, so on an unsandboxed host a process-table reader already has mux access, and any mint-time nonce travels in client argv (seen 15/20 by a naive /proc watcher). No mint-time secret reaches the mux-access bar without a seam change, so ctl_window_id now admits only rows carrying this project's tag (the tag's own bar: a mux write). The ctl-window record only breaks ties among tagged rows, and ctl-window-pid is gone. A window whose tag write failed is left alone until relaunched (fails closed).
A pane pid is an observable identifier, not proof of ownership. On a host where same-UID processes can read the process table, code with only project-write access could find a neighbour's pane pid and forge both ctl-window and ctl-window-pid. Measured before choosing: - A pane's own environment exposes the tmux socket path, so on an unsandboxed host process-table read already implies mux access. - Anything minted into the window at creation travels through the client's argv: a naive /proc watcher saw a window-name nonce in 15 of 20 mints. No mint-time secret reaches the mux-access bar without a seam or name-contract change. So ctl_window_id now admits only rows carrying this project's tag, the same bar as writing the tag itself (a mux write). The ctl-window record (still the window id alone, as older releases read it) only breaks ties among tagged rows. The ctl-window-pid sibling and all its machinery are removed. Cost, accepted: a window whose best-effort tag write failed is no longer reachable by a/x until relaunched. That fails closed, the price bmad-code-org#749 already accepts for recordless windows. Known gap: the ablation was not run locally; CI verifies the tests. The portability guard passes (446). Refs bmad-code-org#750
|
@codex review |
dracic
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2122962993
ℹ️ 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".
| # An empty tag is refused the same way (#750): see the docstring. | ||
| if tag in mine: | ||
| tagged.append(win_id) | ||
| elif not tag and win_id == recorded: | ||
| # untagged, but this project's own launch recorded this window — | ||
| # proof of the mint, not of the tag, so it only counts if nothing | ||
| # is tagged | ||
| untagged.append(win_id) | ||
| matches = tagged or untagged | ||
| if not matches: | ||
| if not tagged: | ||
| return None |
There was a problem hiding this comment.
When psmux's secondary show-options probe fails, PsmuxMultiplexer.list_windows deliberately substitutes "" for every option field (psmux_backend.py:632-659); this new tag-only gate therefore treats all valid recorded windows as foreign and returns None. In the TUI, psmux's stderr warning is captured invisibly, so x makes kill_ctl_window a no-op and _stop_run_worker still reports success while leaving the control window behind; attach can likewise silently fall through. Preserve the distinction between an unreadable tag listing and a genuinely unset tag, or surface the degraded result through the TUI rather than returning the normal no-window sentinel.
AGENTS.md reference: AGENTS.md:L82-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e1f2779. ctl_window_lookup now returns the admitted id plus the count of same-run rows refused for an empty tag, so a psmux option-probe fault no longer reads as a clean absence: x warns that the run stopped but a control window under its name was left running (naming both causes, unreadable or unset tag), and a says why the window is out of reach. Telling unreadable from unset would need an on_fault on list_windows (a seam change), so the notice names both rather than guessing.
…t no-op Tag-only admission made the project tag load-bearing for every control window lookup. When psmux's option probe fails, list_windows fills every option column with "", so all of this run's windows read as untagged, ctl_window_id answered "no window", `x` became a no-op that still reported success, and `a` fell through silently. - ctl_window_lookup returns the admitted window id plus the number of same-run rows it refused for an empty tag. The count is returned even beside a tagged answer, since an untagged row may be a relaunch whose tag write failed. ctl_window_id stays a thin wrapper. - `x` warns that the run stopped but a control window under its name was not closed, naming both causes (a tag that could not be read, or a tag write that failed) and asking the operator to check and close it by hand. - `a` says why an unproven window is out of reach, then still attaches to whatever is reachable. Known gaps: - Telling "unreadable" apart from "unset" would need an on_fault on list_windows (a seam change), so the notice names both causes. - The CLI attach path relies on psmux's own stderr warning. - Tests were written but not run locally; CI verifies them. The portability guard passes (446). Refs bmad-code-org#750
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1f2779abf
ℹ️ 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".
| Read-only throughout: this never writes the record and never touches the | ||
| tag — re-tagging on read is claiming, not proving, and would hand a | ||
| neighbour's window this project's tag.""" | ||
| return ctl_window_lookup(project, run_id)[0] |
There was a problem hiding this comment.
When a run's control window has an unset or unreadable tag, bmad-loop attach still reaches attach_plan, which calls this wrapper and loses the new unproven count. The degraded result therefore becomes an ordinary None: the CLI either reports “nothing to attach” when no agent is live, or silently attaches to the agent even when a decision is waiting in the unreachable control window. Propagate the count through attach_plan so the CLI can explain the refusal just as the TUI now does.
AGENTS.md reference: AGENTS.md:L82-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e988588. attach_plan now resolves through ctl_window_lookup and returns the unproven count with its plan; cmd_attach prints the same operator notice as the TUI on stderr, before attaching or before "nothing to attach", with exit codes unchanged. The remaining ctl_window_id wrapper caller (ctl_window_recorded) was checked and loses nothing an operator needs; no other source caller remains.
attach_plan resolved the control window through the ctl_window_id wrapper and dropped the count of same-run rows refused for an empty tag. So `bmad-loop attach` reported "nothing to attach", or silently attached to the agent, while a decision could be waiting in a control window it could not prove. attach_plan now uses ctl_window_lookup and returns the count with its plan. cmd_attach prints the same operator notice the TUI uses, on stderr, before attaching or before "nothing to attach". Exit codes are unchanged. Other callers of the ctl_window_id wrapper were checked: ctl_window_recorded is safe (it compares an id it just minted, so no refusal count is lost that an operator needs); TUI a and x already use ctl_window_lookup. No other source caller remains. Known gap: tests were written but not run locally; CI verifies them. The portability guard passes (446). Refs bmad-code-org#750
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e988588ffc
ℹ️ 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 tagged: | ||
| return None, unproven |
There was a problem hiding this comment.
When the primary list-windows query fails, the TerminalMultiplexer.list_windows contract returns []; unlike the fixed psmux secondary option-probe path, this reaches this return with unproven == 0. Consequently, x reports the run cleanly stopped while its control window may remain alive, and CLI/TUI attach reports an ordinary absence. Carry a degraded-listing signal separately from a genuinely empty listing so callers can surface this failure.
AGENTS.md reference: AGENTS.md:L82-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 069834d. An empty control-window listing is now confirmed through list_window_ids, which raises when a failed query proves nothing, so a primary listing failure surfaces in x, a, CLI attach and the prune instead of reading as a clean absence. The one residual (psmux option-probe failure inside the prune) is documented in _ctl_window_candidates; telling unreadable from unset there needs a list_windows seam change.
Several paths still read a failed control-window lookup as a clean absence. A failed primary listing returned [], so `x` reported a clean stop while the window could be alive, and attach reported nothing to attach. - Control-window listings that come back empty are confirmed through list_window_ids, which raises when a failed query proves nothing. The lookup, the kill, `x`, `a`, CLI attach and the ctl-window prune surface that failure instead of treating it as "no window". - The prune's has-session short-circuit only trusts a False answer when list_window_ids also proves the session empty, because a refused connect reads as False too. - Run and sweep launches warn when the window they just minted is not reachable by lookup (its tag write failed), as resume already did. - The unproven-window notice picks its pronouns by count. Residual, documented in _ctl_window_candidates: when psmux's option probe fails, every tag reads empty, so the prune skips an untagged window of ours whose run dir is gone without reporting it. Telling "unreadable" from "unset" needs an on_fault on list_windows (a seam change). Tests were written but not run locally; CI verifies them. The portability guard passes (446). Refs bmad-code-org#750
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 069834de7f
ℹ️ 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".
| window minted between the two reads also lands in the raise — loud, never | ||
| silent, and the next look answers it.""" | ||
| rows = mux.list_windows(session, fields) | ||
| if not rows and mux.list_window_ids(session): |
There was a problem hiding this comment.
This new confirmation probe makes the existing test_ctl_window_id_no_session_or_tmux fail: its fake returns rc 1 with stderr "no session" for every command, so list_windows() produces [], but list_window_ids() does not recognize that text as proof the session is gone and raises TmuxError instead of allowing the asserted None. Update that fixture to use a recognized absence response such as "can't find session"/"no server running", or change its expectation to the newly intended failure behavior, otherwise the test suite is red.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3aeede5: the double now answers tmux's "can't find session" wording, which _SESSION_GONE_STDERR accepts as proof the session is gone; test_tui_launch.py passes locally (115).
…n gone An empty control-window listing is now confirmed through list_window_ids, which only accepts the _SESSION_GONE_STDERR wordings as proof of absence. The double's bare "no session" proves nothing, so the lookup rightly raised; use tmux's "can't find session" wording instead.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Review mirror of bmad-code-org#853 for Codex review. Not for merge here.
Closes the residual bmad-code-org#749 deferred to bmad-code-org#750: a recorded control-window id is a handle, not an identity. The record could readmit an untagged neighbour's window in two ways:
Change: admit only tagged windows
ctl_window_idnow admits only rows carrying this project'sPROJECT_OPTIONtag. Thectl-windowrecord, still the window id alone and byte-identical to the format older releases read, only breaks ties among tagged rows (bmad-code-org#482). An untagged row is never admitted, whatever the record says. The lookup never writes the record or the tag.The forgery bar becomes the tag's own bar: setting a tag needs a mux write.
Why not a pane-pid proof (the first revision of this PR)
The first revision recorded the pane pid at mint and re-proved it at lookup. Review showed that a pid is an observable identifier, not proof of ownership. The follow-up measurements found that no mint-time secret reaches the mux-access bar without a seam or naming-contract change:
/proc/<pid>/environcarriesTMUX=<socket path>,…/procwatcher saw it in 15 of 20 mints@1/@0after a server restartSo the pid machinery was removed: net code removal.
Cost (accepted, fails closed)
A window whose best-effort tag write failed is no longer reachable by
a/xuntil relaunched. That is the same price bmad-code-org#749 already accepts for recordless windows, wherexreports the run stopped and leaves the window running.Tests
test_ctl_window_id_admits_a_record_naming_a_window_it_never_mintedflips from characterization to refusal.tui-guideand CHANGELOG are updated.Refs bmad-code-org#750