Conversation
…dow panes Add ADR 0001 for bmad-code-org#730 and bmad-code-org#731: how the state root reaches a pane child when inheritance cannot carry it (PSMUX_BARE_ENV clears the env; a stale multiplexer server substitutes its own BMAD_LOOP_STATE_DIR). Accepted, in three stages: - Stage 1: a tmux-only inherited_env query plus a launcher warning when a pane would resolve a different state root. psmux stays Unknown, because its show-environment cannot see inherited values. - Stage 2: carry the state root to parked engine windows in argv (a hidden --state-root option), with no seam change, on every backend. - Stage 3: a versioned verb pair, specified but not scheduled. Also starts docs/adr/ with an index, linked from docs/README.md. Known gap: the round-3 review findings (the passwd fallback when HOME is absent, and absent vs empty HOME) were patched and checked against runs.state_root by hand, not re-reviewed. Refs bmad-code-org#730 bmad-code-org#731
No stage of ADR 0001 is implemented yet. Say so in the summary and above the staged plan, label Stages 1 and 2 as planned, and word the approver's answer on --state-root as the contract Stage 2 will introduce.
|
@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: ee698b824a
ℹ️ 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".
| - Add a hidden top-level `--state-root` option, applied in `cli.main` before `relay` dispatch and `_configure_mux`. | ||
| - It must be absolute. Otherwise `main` exits `USAGE` with one message, the same rule as `BMAD_LOOP_STATE_DIR`'s own validation. | ||
| - `tui.launch.start_detached` inserts it from `runs.state_root()`, and raises `LaunchError` on `StateRootError` (see Option D). The other `cli_argv` caller is a captured `subprocess.run` that inherits the launcher's own env directly, so it needs no flag. | ||
| - The bare-env warning's text, and Stage 1's, narrow to window-0 and other already-running shells. Per Stage 1, the wording says that a matching query cannot vouch for a shell that is already running. |
There was a problem hiding this comment.
Do not narrow this warning to window-0 and existing shells after Stage 2. When PSMUX_BARE_ENV=1 is enabled, Stage 2 transports only the state root to the parked engine process; subsequently created coding-CLI panes can still lose ambient credentials or configuration that are not explicitly included in their env dict. The ADR itself acknowledges this remaining limitation under Option D, so implementing this instruction would misleadingly imply that bare mode is safe everywhere except shells.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22d2196. The bare-env warning is no longer narrowed: Stage 2 carries only the state root, so a coding-CLI pane in bare mode can still miss other variables (Option D, Cons). Only Stage 1's stale-root warning narrows to window-0 shells; the bare-env warning drops just its parked-window clause.
Stage 2 carries only the state root, so a coding-CLI pane in bare mode can still miss variables its env dict does not name. Only Stage 1's stale-root warning narrows to window-0 shells; the bare-env warning stays and drops just the parked-window clause.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22d2196302
ℹ️ 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".
| - For **future panes**: `tmux set-environment -t =<ctl> BMAD_LOOP_STATE_DIR <root>`. The session scope matters, because a session value overrides the global one (§3.2), so a global-only fix can leave a stale session value in force. Add `-g` as well to cover new sessions, or use `tmux kill-server` to restart clean. | ||
| - For **already-running shells**, including window 0: no query can see them, and `set-environment` cannot change them. They need an in-shell `export BMAD_LOOP_STATE_DIR=<root>` or recreating. |
There was a problem hiding this comment.
In Stage 1's warning, render the root with shell-safe quoting in both suggested commands. The documented tmux syntax accepts a single value argument for set-environment (tmux manual), so a valid root such as /home/me/state dir is split into extra arguments and the remedy fails; shell metacharacters can also change what a copied command executes. The adjacent export BMAD_LOOP_STATE_DIR=<root> has the same problem, so the ADR and its warning tests should require quoting rather than interpolating the path verbatim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 14a1a3e: the ADR now requires both remedy commands to render the root (and the ctl target) with shlex.quote, never verbatim, and Stage 1's tests to assert it with a root containing a space and a metacharacter. The Stage 1 implementation (bmad-code-org#854) already shell-quotes the remedy.
A valid state root may contain spaces or shell metacharacters; set-environment takes a single value. The suggested commands must render the root with shlex.quote, and Stage 1's tests must assert it.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Stage 1's remedy now states that tmux's control session is shared by every project on the server, offers set-environment only where no other state root uses it, and names the cost of kill-server. Stage 2 also carries the registry root (--registry-root) for the honor_ambient_psmux_data_dir opt-in, demotes the stale-root warning to an informational note, and re-surfaces the bare-env warning in the TUI. The CLI, test and Stage 3 specs are corrected, the E2E gate moves to its own module, and section 8 names the approver and records every amendment after acceptance as re-approved. Refs bmad-code-org#730 bmad-code-org#731
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
…ntact Under the pre-7.3 PowerShell native-argument algorithm, a psmux parked argument that ends in a backslash and contains whitespace merges with the next one (measured on Windows PowerShell 5.1; pwsh 7.6.6 delivers it intact in both Windows and Legacy modes). Stage 2 now must either make the psmux transport safe for that case or refuse PowerShell older than 7.3 on every psmux launch path, with a live test for whichever route it takes. Refs bmad-code-org#730 bmad-code-org#731
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
Section 8 promised a test on the pre-7.3 runtime for whichever route Stage 2 takes, but the refusal route only faked the version probe. It now also points the probe at Windows PowerShell 5.1 and asserts the refusal, with the faked-probe unit test kept as a supplement. Refs bmad-code-org#730 bmad-code-org#731
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
Upstream PR merged; closing this review mirror. |
1 participant
Review mirror of bmad-code-org#850 for Codex review. Not for merge here.
Design record for bmad-code-org#730 and bmad-code-org#731, which bmad-code-org#537 left as
needs-design: how the state root reaches a pane child when environment inheritance cannot carry it. This happens in two ways:PSMUX_BARE_ENV=1clears the env, and a stale multiplexer server substitutes its ownBMAD_LOOP_STATE_DIR. Coding-CLI windows are already immune; the exposure is the TUI's parked engine windows and window-0 shells.Docs only. No file under
src/ortests/changes.What the ADR decides (
docs/adr/0001-mux-env-transport.md, status Accepted)BMAD_LOOP_STATE_DIRinto every window it spawns bmad-code-org/bmad-loop#731 interim. A non-abstractinherited_envquery, implemented on tmux only. The launcher compares the state root a new pane would resolve with its own, and warns once on a mismatch. psmux stays "unknown": itsshow-environmentshows onlyPSMUX*/TMUX*names, so an inheritedBMAD_LOOP_STATE_DIRis invisible to it (source-read atv3.3.8and mastere36bd85).--state-rootoption applied at the start ofcli.main. Stage 2 is the change a stale multiplexer server substitutes its ownBMAD_LOOP_STATE_DIRinto every window it spawns bmad-code-org/bmad-loop#731 waits for, and it settles the parked-window half of psmux: decide whether bmad-loop should supportPSMUX_BARE_ENVbmad-code-org/bmad-loop#730 on every backend, out-of-tree ones included, because no released signature changes.BMAD_LOOP_STATE_DIRinto every window it spawns bmad-code-org/bmad-loop#731 closes and psmux: decide whether bmad-loop should supportPSMUX_BARE_ENVbmad-code-org/bmad-loop#730 is re-scoped to "parked windows supported, window-0 shells warned".Evidence
v3.3.8and master.Also in this PR
docs/adr/with an index; the repo had no ADR convention.docs/README.md.Refs bmad-code-org#730 bmad-code-org#731 bmad-code-org#660
Summary by CodeRabbit