Conversation
Add [mux] honor_ambient_psmux_data_dir (default off). With it on, bmad-loop keeps an operator's absolute PSMUX_DATA_DIR as the psmux registry instead of overriding it with the derived per-project root. The setting is a boolean, never a path: policy.toml is writable by driven sessions, so a policy-sourced root could aim the cleanup path at a registry of the session's choosing. The outer process and every pane child read the same per-project file, which closes both horns measured on bmad-code-org#729 (transient pin vs persistent pin). - runs.resolve_psmux_registry_root is the pure decision. It honours the ambient value only when the flag is on, the value is absolute, and the value is not shaped like a derived root (a 16-hex project tag parent with _mux), so an inherited derived root is never adopted. - runs.export_psmux_registry_root stays the single place the root is settled. On honour, the derived root is handed to the displaced- registry sweep, so sessions started before the flip stay reachable. - runs.ctl_session_for folds the settled root into the control-session digest when it differs from the derived one, so the control session does not collide on psmux's name mutex with one still alive in the derived registry. The name is unchanged otherwise. - bmad-loop mux says whether the registry was derived or honoured. Known gaps: - The final Windows runs were single-file. The pre-review multi-file run had 4 symlink WinError 1314 failures (environmental). - WSL ran only the 4 touched test files (1 environmental failure: no bare python on PATH). The rest of the Linux suite is unverified locally. - ruff format --check reports pre-existing drift in cli.py, runs.py, test_cli.py and test_policy.py that also exists on main; it is left out of this diff. Closes bmad-code-org#729
|
@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: 09576696e9
ℹ️ 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 root != derived: | ||
| # Honoured: what is displaced now is the derived root, which holds any | ||
| # session started before the operator turned the flag on. Same sweep, | ||
| # same reason as the override arm below. | ||
| displaced = derived |
There was a problem hiding this comment.
When this setting changes the registry after an interrupted or stopped run leaves its agent session alive, the displaced registry is recorded only for cleanup. Resume still calls runs.kill_session(run_dir.name) against the newly selected registry (runsetup.py:1531-1534), leaving the old same-named server alive; because psmux's session-name mutex spans registries, launching the resumed session in the new registry is then rejected. Kill the tagged stale session in the displaced registry during resume, rather than requiring the operator to run cleanup manually first.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6e5b07f. Resume now also calls runs.kill_displaced_session, which walks the legacy-registry set (psmux's default plus the displaced root) and kills the run's same-named session only when its project tag is one this project accepts (cleanup's require_tag rule, never by name alone). Unlistable registries, tag-unproven survivors and failed kills are journaled as displaced-session-not-cleared and warned on stderr. Note: the gap predates this branch (reachable since 0.12.0 via bmad-code-org#537's displaced registry), so it lands as a Fixed entry; this PR only added a trigger.
…istry Resume dropped a lingering agent session with runs.kill_session, which reaches only the registry this process exported. A session left in an older registry stayed alive, and because psmux's session-name mutex ignores PSMUX_DATA_DIR (psmux/psmux#599, unfixed in 3.3.8), the resumed session's create was then rejected. The gap predates this branch: it has been reachable since 0.12.0 (bmad-code-org#537) for a run interrupted before an upgrade, whose session sits in psmux's default registry or a displaced ambient root. The honor_ambient_psmux_data_dir opt-in adds one more trigger: the derived root becomes the displaced one. - runs.kill_displaced_session walks the existing legacy-registry set (psmux's default plus the displaced root). In each, it matches the run's session through the registry's own name key (psmux folds case) and kills it only when its project tag is one this project accepts: cleanup's require_tag rule, never a kill by name alone. - It does not reuse prunable_sessions: that check reads the run as alive once resume has written its own pid, and would spare exactly the session this targets. - Every registry it could not list, every same-named session left standing for lack of a tag proof, and every session that survived its kill is returned. compose_resume journals each as displaced-session-not-cleared and warns on stderr, pointing at bmad-loop cleanup. Known gaps: - Tests were written but not run locally; CI verifies them. - stop_run's backstop kill keeps its single-registry scope; a session left there blocks nothing until a resume, which now clears it. Refs bmad-code-org#729 bmad-code-org#537
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e5b07f799
ℹ️ 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".
| pin = Path(ambient) | ||
| if pin.name == MUX_REGISTRY_DIR and _PROJECT_TAG_RE.fullmatch(pin.parent.name): | ||
| return derived | ||
| return ambient |
There was a problem hiding this comment.
When two projects honor the same profile-provided PSMUX_DATA_DIR, this returns a registry shared across projects, but direct session operations still assume the primary registry proves ownership. For example, if project A has a stopped/resumable run r1 with no session and project B currently owns tagged bmad-loop-r1 in the shared registry, resuming A first calls the unconditional runs.kill_session(run_dir.name) in compose_resume, killing B's coding process; _stop_run_once has the same unsafe backstop, and attach_plan can attach to the foreign session. These by-name operations need the same project-tag ownership check already added to the primary cleanup path before the ambient registry can safely be shared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e857ee. When the settled registry is not this project's derived root, every by-name operation now requires this project's tag proof: kill_session (covering resume, the stop backstop and the engine's five teardown paths), _session_liveness, attach_plan/session_exists, and the generic adapter's _ensure_session, which refuses to adopt a foreign same-named session and fails the launch visibly. The project is recorded per process, so no signatures change. Refusals warn on stderr and are toasted in the TUI. The derived per-project registry is unchanged. 18 gates ablated; Windows 4666 and WSL 4929 passing, with environmental failures only.
The resume fix journals a new kind; the journal-kind inventory guard requires its row. Its only field, detail, is already in _JOURNAL_DROP_FIELDS, so no diagnostics routing row is needed.
… a shared registry With [mux] honor_ambient_psmux_data_dir on, two projects can share one psmux registry, and a session name (bmad-loop-<run id>) no longer says which project owns it. Run ids collide across projects only by chance (same second, 1/65536) or on purpose (the hidden --run-id), but the consequence was killing, adopting or attaching to another project's coding session. When the settled registry is not this project's derived root, every by-name operation against it now needs this project's tag proof. The project is recorded per process when the registry is exported, so no signature changes: - runs.kill_session refuses to kill an unproven same-named session. This covers resume, the stop backstop and the engine's five teardown paths. - runs._session_liveness and the attach resolution (attach_plan, session_exists) stop treating a foreign session as this run's. - The generic adapter's _ensure_session refuses to adopt an existing same-named session it cannot prove, and fails the launch with a clear MultiplexerError instead of opening windows inside it. - Every refusal is visible: a stderr warning, plus a drained queue the TUI turns into toasts on attach, stop and cleanup, because Textual captures stderr. Behaviour in a derived per-project registry is unchanged: there the name already proves ownership. Tests: 18 gates, each ablated and confirmed to fail without it. Windows (9 touched or affected files): 4666 passed, 6 environmental failures (5 symlink WinError 1314, 1 host codex.CMD path). WSL, the same files: 4929 passed, 1 environmental failure (no bare python). Known gaps: - The refused-kill queue is drained only by the TUI; in a CLI or engine process it holds at most one entry per refused kill until the process exits. - Review round 3 left two small findings (the stop worker drain, and toast markup rendering in a test); they were applied without a 4th round and checked by hand. Refs bmad-code-org#729
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e857eef6e
ℹ️ 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 (honor_ambient and ambient and Path(ambient).is_absolute()): | ||
| return derived |
There was a problem hiding this comment.
In the TUI settings flow, if the process started with this option enabled, its backend remains pointed at the honored root, but switching the option off and launching or resuming a run causes the detached child to read the updated policy here and switch to the derived root. The parent TUI continues querying the old root, so it cannot see or attach to the new agent session and may report that no session exists. Either prevent this live edit, require/reliably prompt a TUI restart, or ensure detached children inherit the parent's settled registry until the parent is reconfigured.
AGENTS.md reference: AGENTS.md:L82-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab6a5d: start_detached resolves the registry the detached child would settle and refuses the launch when it differs from the TUI's own, with an error asking for a TUI restart, so a live policy flip can no longer split parent and child across registries.
…ip gate The ownership gate added for a shared registry sat inside the generic session_exists, which prune_ctl_windows also uses to find the control session. A control session is per-project by name (its digest), but it is not tagged like an agent session. So in any non-derived registry the gate refused it, and the prune found nothing to prune. The psmux live gate caught it (test_prune_kills_only_the_owning_projects_window, isolated PSMUX_DATA_DIR). The gated check is now agent_session_exists, used only by the agent attach and liveness paths. The control-session lookup keeps the ungated session_exists. Live gate after the fix: two full runs of tests/test_psmux_live.py, 15 passed, 0 failed each. A first run overlapping the reviewer's startup failed 2 rows (the prune row and the pipe-pane row); the prune row is known to flake under load (it failed the same way on unmodified main while three lanes loaded the box), and neither failure recurred. Unit files: 905 passed. Ablation: 4/4 gates fail when deleted. Refs bmad-code-org#729
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0dd1211a8
ℹ️ 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".
| self.mux.new_session(self.session_name, cwd, PANE_COLUMNS, PANE_LINES) | ||
| # Tag the session with its project so a cleanup in another project | ||
| # never prunes this run (run_dir = <project>/.bmad-loop/runs/<id>). |
There was a problem hiding this comment.
When the ambient registry is shared, if new_session succeeds but set_session_option raises (for example, after a transient psmux error), this leaves a live untagged session behind. On the next attempt, foreign_session_refusal treats that same session as foreign, while kill_session and cleanup also refuse or skip it, so the run ID remains blocked until the operator manually removes the session. Tear down the newly created session when tagging fails, while preserving the original error, or otherwise make the create-and-tag sequence recoverable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab6a5d: when the ownership tag cannot be written, _ensure_session tears down the session this call just created (by its exact name, outside the ownership gate, read back), then re-raises the original error, chained with the teardown fault if that also fails. A session the call found rather than created is never touched.
…gable session Two gaps around honor_ambient_psmux_data_dir: - A TUI started with the opt-in on keeps querying the honoured registry. Flipping the setting off live and then launching or resuming made the detached child settle the derived registry, so the TUI could no longer see or attach to the run's session. start_detached now resolves the registry the child would settle and refuses the launch when it differs from the TUI's own, with an error that says to restart the TUI. Nothing is inherited implicitly. - In _ensure_session, a session that new_session created but whose ownership tag could not be written stayed alive and untagged. In a shared registry it then read as foreign: kill and cleanup refused it, so the run id stayed blocked. The session this call just minted is now torn down by its exact name, without the ownership gate (it is untagged by construction). The teardown is read back, and the original error is re-raised, chained with the teardown fault when that fails too. A session the call found, rather than created, is never touched. Tests (single files, -n 0): test_tui_launch 112 passed; test_generic_tmux 410 passed, 1 environmental failure (codex.CMD host path); the portability guard 446 passed. Ablation: 6/6 gates fail when deleted. Refs bmad-code-org#729
|
@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". |
Under PSMUX_BARE_ENV a pane child inherits no PSMUX_DATA_DIR, so it derives the registry instead of honouring the operator's root. bmad-loop does not support that mode and already warns. Say so where the inheritance is assumed (_registry_drift, resolve_psmux_registry_root, and the operator docs), so ADR 0001 and this change agree that Stage 2 carries the registry root explicitly. Refs bmad-code-org#729
|
@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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, 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". |
…able tag in the resume sweep
…nreadable policy in the drift refusal
… on under a running TUI
…say a running TUI must be restarted after a switch flip
1 participant
Review mirror of bmad-code-org#851 for Codex review. Not for merge here.
Implements the mechanism bmad-code-org#729 proposes for an operator's own
PSMUX_DATA_DIR: a[mux]policy boolean, default off.Why a boolean
Two operators can produce byte-identical environments that need opposite answers (the transient pin vs the persistent pin, measured on bmad-code-org#729). The missing fact is whether the pin is persistent, and that fact is not in the process environment. Moving it into per-project policy means the outer process and every pane child read the same answer.
It is deliberately a whether, never a where.
policy.tomlis writable by the sessions the orchestrator drives, so a policy-sourced path would let a driven session aim the cleanup path's kills at a registry of its choosing (the bmad-code-org#498 / bmad-code-org#571 class).What changes
[mux] honor_ambient_psmux_data_dir(bool, default false), with itscore.tomlentry.runs.resolve_psmux_registry_root(derived, ambient, *, honor_ambient): the pure decision. It honours the ambient value only when the flag is on, the value is absolute, and the value is not shaped like a derived root (a 16-hex project-tag parent with_mux). A pane child that merely inherited the outer process's derived root therefore never adopts it as a pin.runs.export_psmux_registry_rootstays the only place the root is settled. On honour, the derived root goes to the displaced-registry sweep, so sessions started before the flag was flipped stay reachable for cleanup.runs.ctl_session_forfolds the settled root into the control-session digest when it differs from the derived one. Otherwise the control session would collide, on psmux's name-keyed mutex (the single-server name mutex ignores PSMUX_DATA_DIR, so two registries cannot both hold a session of the same name — including __warm__ (3.3.8/master) psmux/psmux#599), with one still alive in the derived registry. Names are byte-identical when nothing is honoured.bmad-loop muxstates which source won.--jsonoutput is unchanged.docs/multiplexer-backends.md,docs/FEATURES.md, and a CHANGELOGAddedentry.Tests
test_runs.py:PSMUX_DATA_DIRshould mean bmad-code-org/bmad-loop#729's measured table (outer, pane child moved to another state root, clean with or without the pin) for each flag value.values are still overriddentest_cli.py: the opt-in throughmain, and thebmad-loop muxwording.test_policy.py: defaults, parsing, and the boolean-field registry.Every new gate was ablated and confirmed to fail without it. The one exception is the
Path.is_absolutevsos.path.isabsrow: those two diverge only on Windows Python before 3.13, so it could not fail locally.Local results
test_runs.py471 passed, 4 failed. All 4 are environmental symlinkWinError 1314.pythonon PATH) and in reverify code this PR does not touch.ruff format --checkreports drift incli.py,runs.py,test_cli.pyandtest_policy.pythat already exists onmain; it is left out of this diff.Review follow-ups
Resume clears a stale session in a displaced registry (
runs.kill_displaced_session). psmux's session-name mutex ignoresPSMUX_DATA_DIR(the single-server name mutex ignores PSMUX_DATA_DIR, so two registries cannot both hold a session of the same name — including __warm__ (3.3.8/master) psmux/psmux#599), so a same-named session left in an older registry blocked the resumed create. Only tag-proven sessions are killed, never by name alone, and unclearable ones are journaled asdisplaced-session-not-cleared. This gap predates the branch: it has been reachable since 0.12.0 via psmux: adopt PSMUX_DATA_DIR as a per-project registry root — seam-wide, not a create-call env tweak bmad-code-org/bmad-loop#537's displaced registry.Ownership proof in a shared registry. With the opt-in on, two projects can share one registry, and a session name no longer proves ownership. When the settled registry is not the project's derived root, every by-name operation now requires this project's tag:
kill_session, which covers resume, the stop backstop and the engine's teardown paths;_ensure_session, which refuses to adopt a foreign same-named session and fails the launch visibly.Refusals warn on stderr and are toasted in the TUI. The derived per-project registry is unchanged. The project is recorded per process, so no signatures change. 18 gates were ablated.
Refs bmad-code-org#729
Summary by CodeRabbit
PSMUX_DATA_DIRas the psmux session registry instead of the project-derived registry. The setting takes effect on the nextbmad-loopinvocation and is off by default.bmad-loop muxoutput identifies which registry root is being used.