Conversation
…e state root A tmux server is long-lived, and its panes inherit the environment it was started with. A server cold-started under BMAD_LOOP_STATE_DIR=S1, then reused by a launch under S2, hands S1 to the parked engine window and every window-0 shell. The engine then writes its control plane where the launcher never looks, and the live run reads as gone (bmad-code-org#731). This is the interim from the bmad-code-org#731 design: detect it and say so; carrying the root explicitly is a later change. - TerminalMultiplexer.inherited_env(session, name, *, on_fault=None) is a new non-abstract query answering what a new pane will inherit: None (unknown, the seam default), the UNSET sentinel (known absent), or the value ("" included). It never raises; a failed query reports through on_fault instead of folding into unknown. - TmuxMultiplexer implements it with show-environment -t =S NAME, plus the -g fallback on an exact "unknown variable" miss. It sits on TmuxMultiplexer, not BaseTmuxBackend, so psmux keeps "unknown": its show-environment cannot see inherited values. - runs.state_root is now runs.resolve_state_root(os.environ, passwd_home), byte-identical over 2560 env combinations on Windows and Linux. The passwd home is looked up lazily and used only when HOME is absent: absent and empty HOME are different inputs. - _ensure_ctl_session compares the root a new pane would resolve with the launcher's own, after both the create and the reuse arm, and warns once per process through the TUI with a shell-quoted remedy. Unknown answers are silent, faults are reported, and the launch is never blocked. Out-of-tree backends written against today's seam are unaffected: a StubMux implementing only the released abstract set completes both arms with no warning. Known gaps: - Windows test_runs has 4 environmental symlink WinError 1314 failures. - WSL ran the 6 touched test files only (1586 passed). The rest of the Linux suite is unverified locally. - trunk check could not run locally (a broken ruff plugin); ruff, prettier and pyright were run directly. Refs bmad-code-org#731
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. WalkthroughThe TUI now checks whether new tmux windows would inherit a different state root. It warns once when the roots differ or the inherited root cannot be resolved, but continues launching. The warning includes the roots and shell-quoted commands to update the tmux environment. ChangesTmux state-root warning
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant BmadLoopApp
participant launch._ensure_ctl_session
participant TmuxMultiplexer
participant runs.resolve_state_root
BmadLoopApp->>launch._ensure_ctl_session: Ensure control session
launch._ensure_ctl_session->>TmuxMultiplexer: Query inherited state-root inputs
TmuxMultiplexer->>launch._ensure_ctl_session: Return values, UNSET, or unknown
launch._ensure_ctl_session->>runs.resolve_state_root: Resolve inherited state root
launch._ensure_ctl_session->>BmadLoopApp: Send warning through warning sink when roots differ
BmadLoopApp->>BmadLoopApp: Display warning notification
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The state-root warning is non-blocking, and no issue requiring a fix before merge was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change observes inherited configuration without changing privileges, state ownership, or launch destinations. It warns about an existing state-root mismatch rather than preventing it. Unknown configuration and warning-delivery failures limit the assurance provided by this diagnostic. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 10 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the tmux trail, Comment |
|
CI note on
I cannot re-run the job (it needs repository admin rights). Could a maintainer re-run the failed job? If it reproduces, I will dig in. |
1 participant
The interim for #731: detect a reused tmux server that would plant a stale
BMAD_LOOP_STATE_DIRin new panes, and say so. This is Stage 1 of the design in #850. Carrying the root explicitly in the parked window's argv is Stage 2, a separate change.Problem
A tmux server is long-lived, and its panes inherit the environment it was started with. A server cold-started under
BMAD_LOOP_STATE_DIR=S1, then reused by a launch under S2, hands S1 to the parked engine window and every window-0 shell. The engine writes its control plane where the launcher never looks, and the live run reads as gone.Change
TerminalMultiplexer.inherited_env(session, name, *, on_fault=None): a new non-abstract query for what a new pane will inherit. It answersNone(unknown, the seam default), theUNSETsentinel (known absent), or the value,""included. It never raises; a query that was attempted and failed reports throughon_faultrather than folding into "unknown".TmuxMultiplexer, notBaseTmuxBackend. It runsshow-environment -t =S NAME, plus a-gfallback on an exactunknown variablemiss. Reply shapes were measured on tmux 3.4:NAME=v,NAME=, the-NAMEremoval marker, misses, and a missing session.show-environmentshows onlyPSMUX*/TMUX*names, so it cannot see an inheritedBMAD_LOOP_STATE_DIR; the per-project registry already closes the ordinary path there.runs.resolve_state_root(env, passwd_home): the state-root cascade as a pure function.runs.state_root()delegates to it and is byte-identical: a differential over 2560 env combinations on both Windows and Linux found 0 mismatches. The passwd home is looked up lazily and only used whenHOMEis absent, because absent and emptyHOMEare different inputs._ensure_ctl_session: after both the create and the reuse arm, it compares the root a new pane would resolve with the launcher's own. On a mismatch it warns once per process through the TUI, naming both roots and giving a shell-quoted remedy.docs/multiplexer-backends.md: a new subsection with the operator remedy. CHANGELOGFixed.Compatibility
A
StubMuxthat implements only the released abstract set completes both_ensure_ctl_sessionarms with no warning and no error. Out-of-tree backends need no change.Tests
11 gates were ablated, each confirmed to fail its test without the gate. These include: the fault path, placing the implementation on the base class (psmux test), passwd use on an empty
HOME, eager passwd lookup, a raw-override comparison instead of resolved roots, the looseunknown variablematch, and the remedy quoting.Local results
test_runs, all environmental symlinkWinError 1314.Refs #731 #850
Summary by CodeRabbit