diff --git a/CHANGELOG.md b/CHANGELOG.md index f49cad39..cc632f3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,33 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Added +- **The hook-event channel moves out of the project tree (#494).** A run's session-completion + signals now land under a user-scoped state root — `$XDG_STATE_HOME/bmad-loop` (else + `~/.local/state/bmad-loop`), `%LOCALAPPDATA%\bmad-loop\state` on Windows, or wherever + `BMAD_LOOP_STATE_DIR` points (absolute paths only — the orchestrator and the session it + launches read the root from different working directories) — keyed + `///events/`, so a branch switch, a + worktree mount or a rollback can no longer take a live run's control plane away. + + - **Older relays keep working.** Sessions are told the directory via `BMAD_LOOP_EVENTS_DIR`; both + relays fall back to the in-tree `/events` and the orchestrator polls both locations. + `init` copies the relay into the project, so an upgraded orchestrator regularly drives sessions + whose relay predates the move — re-run `bmad-loop init` to refresh it. + - **`bmad-loop relay `** writes a session event without the copied-in script, on the same + contract (nothing on stdout, rc 0 always, silent no-op outside a driven session). `init` does + not point hooks at it yet — that retargeting is #461 Phase 2 — so it changes no run today. It + is backed + by a new `events.py`, whose write path an AST parity test holds byte-identical to the + stdlib-only relay's, and dispatches ahead of the shared error handler so a broken + `policy.toml` cannot fail a hook. + - **`validate` gains `hooks.relay-stale`** — the installed relay compared against the packaged + one, a warning that never moves the exit code (the fallback keeps a stale relay working). + `diagnose`'s `events` group now counts both locations; payload and schema unchanged. + - **`delete`/`archive`/`clean` collect the out-of-tree dir** with the run, and `clean` sweeps + orphans whose run dir is already gone (`--json`: `state_dirs_swept`, an additive field). An + archived tarball therefore no longer contains `events/` — consumed transient signals. + - `run`/`sweep` `--dry-run` previews the events directory a session would use. + - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own adapter _class_ forced a name-branch in the run bootstrap. A profile's new `adapter` field names a diff --git a/README.md b/README.md index 8a514aac..73e1ed59 100644 --- a/README.md +++ b/README.md @@ -563,19 +563,25 @@ For `per_worktree`, set `editor_mode = "per_worktree"` with `[scm] isolation = " ## Environment variables -A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking precedence over the policy file. Most operators only ever touch `BMAD_LOOP_MUX_BACKEND`; the other two are override/test hooks. +A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking precedence over the policy file. Most operators only ever touch `BMAD_LOOP_MUX_BACKEND` and, on a host with an unusual home directory, `BMAD_LOOP_STATE_DIR`; the rest are override/test hooks. -| Variable | Value | Effect | -| ----------------------------- | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | -| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | -| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | +| Variable | Value | Effect | +| ----------------------------- | ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | +| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | +| `BMAD_LOOP_STATE_DIR` | absolute directory path | Overrides the user-scoped **state root** — the out-of-tree home of per-run control-plane state, keyed `///`. Used as the root itself, so nothing is appended to it. Must be **absolute**: the root is read both by the orchestrator and by the session it launches, which run from different working directories, so a relative value names two different places — bmad-loop refuses it rather than picking one. Unset ⇒ `$XDG_STATE_HOME/bmad-loop` when that names an absolute path, else `~/.local/state/bmad-loop`; on Windows `%LOCALAPPDATA%\bmad-loop\state`, else `%USERPROFILE%\AppData\Local\bmad-loop\state`. Set this when none of those is derivable or writable (a home on a network share, a locked-down service account). | +| `BMAD_LOOP_EVENTS_DIR` | directory path | **Session protocol, not an operator knob.** The orchestrator exports it into every session it drives, naming that run's hook-event directory under the state root; the hook relay writes there, falling back to the legacy in-tree `/events` when it is absent. Setting it yourself in a shell has no effect on a run (the engine overwrites it per session) and only misdirects a hand-invoked relay. | +| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` set documented in the [Game Engine MCP guide](docs/game-engine-mcp-guide.md). ## Run state -Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `events/` (hook signals), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a graceful stop is pending — `stop-request.json` (the control file the engine consumes at the next item boundary). +Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a graceful stop is pending — `stop-request.json` (the control file the engine consumes at the next item boundary). + +One piece deliberately lives elsewhere: the **hook-event channel** (the session completion signals the orchestrator waits on) sits under the user-scoped state root at `///events/`, outside the project tree — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. See `BMAD_LOOP_STATE_DIR` above for where that root resolves. The orchestrator also keeps polling the old in-tree `events/` location, so a project whose installed hook relay predates the move still completes its sessions; re-run `bmad-loop init` to refresh the relay. + +That out-of-tree directory is collected with the run: `delete`, `archive` and `clean` remove it alongside the run dir, and `clean` also sweeps this project's orphans there — control planes whose run dir is already gone, e.g. from a hand-removed run (`clean --dry-run` previews the count; `--json` reports it as `state_dirs_swept`). Two consequences worth knowing: an archived run's tarball no longer contains `events/` (transient completion signals, consumed while the run was live), and a project that is deleted, moved or renamed leaves its old subtree behind — the key is derived from the project's resolved path, so after a move the project itself now keys somewhere new and nothing can name the old key to sweep it. Remove it by hand if you care; it is events-sized, not run-sized. A run can be stopped two ways. A **hard stop** (`bmad-loop stop`, TUI `x`, Ctrl+C) SIGTERMs the engine mid-item and always kills the agent session. A **graceful stop** (`bmad-loop stop --graceful`, TUI `S`) instead writes `stop-request.json` — no signal, so it works on every platform and multiplexer backend — which the engine consumes at the next item boundary: the in-flight story (or sweep bundle) finishes through commit — or, mid-triage, the sweep's triage session completes and no bundles start — the run finalizes as `stopped` (not `finished`) and stays resumable, and pending auto-sweeps are suppressed. Its session teardown follows `cleanup_session_on_finish` like a normal finish, rather than the hard stop's unconditional kill; a hard stop always supersedes a pending graceful request. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 087e2df6..f4e18b27 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -102,12 +102,15 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). +- One piece deliberately lives **outside** that directory: the hook-event channel (#494) is at `///events/` under the user-scoped state root (`BMAD_LOOP_STATE_DIR`, see the [transport section](#hook-based-transport-no-pane-scraping) below and the README's env-var table), not `/events/`. The orchestrator still polls the legacy in-tree location, so a project whose installed relay predates the move keeps completing its sessions. `delete`, `archive` and `clean` remove the out-of-tree counterpart along with the run dir, and `clean` sweeps counterparts whose run dir is already gone; an **archived** run's tarball therefore no longer contains `events/` — those files are transient completion signals, consumed while the run was live, and everything an archive is read for later is in the run dir. - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) - Coding-agent hooks (`Stop` / `SessionStart` / `SessionEnd` / `PreCompact`) write structured event files the orchestrator watches; skills write a machine-readable `result.json`. +- The event channel lives **outside the project tree** (#494), at `///events/` — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. Each session is told where to write via `BMAD_LOOP_EVENTS_DIR`; the state root itself resolves per `BMAD_LOOP_STATE_DIR` (see the env-var table in the README). The relay falls back to the legacy in-tree `/events` when that variable is absent, and the orchestrator keeps polling that location too — the hook script is copied into the project by `init`, so an upgraded orchestrator regularly drives sessions whose relay predates the move, and without both halves every such session would stall to `session_timeout_min`. +- What the move is and is not worth: it eliminates the whole in-tree redirect class — nothing an agent writes inside the project can any longer point the completion channel somewhere else — and it is a hard boundary for a sandboxed session, which cannot reach outside the tree at all. It is **not** a boundary against a session running with permissions bypassed: that session is told the directory by `BMAD_LOOP_EVENTS_DIR`, so it can reach it by construction. This is why the relay keeps its own `_is_link_like` refusal on the write path (#493) as belt-and-braces rather than retiring it as redundant. ### Deferred-work sweeps @@ -178,8 +181,9 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Disk reclamation (`[cleanup]`) - `bmad-loop clean` reclaims **disk** (distinct from `cleanup`, which is only tmux). It tears down git worktrees a mid-flight stop left mounted — the main accumulation source: each carries a real Unity `Library/` (incl. the MCP-server build), which `git worktree remove` cannot reach once the engine was killed before teardown. It then trims the heavy `worktrees/` tree from runs kept for history (the run still lists in the dashboard — discovery reads `state.json`, not the worktree), and archives or deletes runs past the retention window. +- It also collects the **out-of-tree** half of a run. Removing a run dir no longer removes everything the run owns (#494), so `delete`/`archive`/`clean` remove the run's control-plane dir under the state root too, and `clean` additionally sweeps this project's orphans there — subtrees whose run dir is gone, from a hand-removed run or a delete that predates this. The sweep keys on the run directory _existing_, not on its `state.json` parsing, so a corrupt run an operator is trying to recover keeps its control plane; a trimmed run keeps its own for the same reason (it is still resumable). Their bytes are not in the reclaim estimate — a state dir holds consumed event files and little else. Known limit: the state root is keyed by the project's resolved path, so a project that is deleted, moved or renamed leaves its old subtree unsweepable — after a move the project keys somewhere new, and no project can name the old key. - Safe by construction: only **finished or stopped** runs are touched; running, unknown-host, paused and interrupted (resumable) runs are never reclaimed. `--keep ` protects a specific run (e.g. a finished one whose Editor is still live), `--dry-run` previews, `--retain N`/`--hard` tune the window and archive-vs-delete. -- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the effective retention policy, `freed_bytes` as a raw integer, and the paths and run ids under `worktrees`/`trimmed`/`archived`/`deleted`/`protected`) instead of the text. Plan and outcome share one schema, with `dry_run` saying which one you are holding, so a script can pre-flight a reclaim and compare it against what happened — though values are each invocation's own sample, not a promise the two agree. It names every item the text only counts or renders, and the unverifiable-pid warning text mode writes to stderr becomes `unverifiable_pid` in the document, leaving stderr empty. +- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the effective retention policy, `freed_bytes` as a raw integer, and the paths and run ids under `worktrees`/`trimmed`/`archived`/`deleted`/`protected`, and `state_dirs_swept` as a count) instead of the text. Plan and outcome share one schema, with `dry_run` saying which one you are holding, so a script can pre-flight a reclaim and compare it against what happened — though values are each invocation's own sample, not a promise the two agree. It names every item the text only counts or renders, and the unverifiable-pid warning text mode writes to stderr becomes `unverifiable_pid` in the document, leaving stderr empty. - Prevention is automatic: every `run`/`sweep` start reconciles worktrees leaked by a prior **finished** run (`[cleanup] auto_clean_on_finish`), and the Unity plugin's `post_run` hook removes the IvanMurzak MCP server's downloaded `/tmp///*.zip` and truncates its unbounded editor log (`[cleanup] clean_tmp`). For recurring housekeeping of stopped runs, schedule `bmad-loop clean`. ### Setup & install @@ -206,11 +210,11 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop diagnose []` (`diag`) — emit a sanitized diagnostic dump of a run/sweep to hand maintainers (histograms, counts, env, file sizes — no code/spec/prompts/paths/PII); a stray pseudonymized identifier is auto-substituted with its alias (disclosed in the report), while PII/secret hits still refuse to emit; defaults to the latest run (`--all`, `--out`, `--max-journal-entries`). `--json` emits the same dump as a stable machine-readable document per the [contract below](#machine-readable-output---json) instead of the markdown report. - `bmad-loop attach []` — tmux-attach to a run's live agent session. - `bmad-loop stop ` — stop a live run. The default is a **hard stop**: SIGTERM the engine mid-item and kill its agent session. `--graceful` instead requests a **graceful stop** — the engine finishes the in-flight item (a story through commit, a sweep bundle through commit, or an in-progress sweep triage — after which no bundles start), then finalizes cleanly and stops as a resumable `stopped` run, suppressing any pending auto-sweeps; `--cancel-graceful` withdraws a pending request. Delivery is a `stop-request.json` control file consumed at the next item boundary (no signal, so it works on every platform and multiplexer backend); a hard stop always supersedes a pending graceful one. The TUI surfaces the same pair: `x` hard-stops, `S` requests a graceful stop. -- `bmad-loop delete ` — delete a run directory (`--force` stops it first if live). -- `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it (`--force` stops it first if live). +- `bmad-loop delete ` — delete a run directory and its out-of-tree control-plane dir (`--force` stops it first if live). +- `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it, control-plane dir included (`--force` stops it first if live). The tarball holds the run dir, so it carries no `events/`. - Removal refuses while a matching agent session is live that the project cannot prove is another one's, even when the engine is dead: for an untagged session the run dir is the last ownership proof `cleanup` can read, so removing it would leak the session ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)). A session tagged to another project carries its own proof and never blocks. Run `cleanup` first, having confirmed the session is this project's (`attach`): for an _untagged_ session `cleanup` proves ownership by that same run dir, so two projects sharing a run id can prune each other's. Or pass `--force`, which removes anyway and kills nothing. `clean` leaves such a run untouched and reports it as protected. - `bmad-loop cleanup` — remove leftover tmux artifacts for finished/stopped runs. `--json` emits the sessions and ctl windows removed (or, with `--dry-run`, that would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json). -- `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window (`--dry-run`, `--keep`, `--retain N`, `--hard`). `--json` emits what was reclaimed (or would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. +- `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window, and sweep orphaned run control-plane dirs from the out-of-tree state root (`--dry-run`, `--keep`, `--retain N`, `--hard`). `--json` emits what was reclaimed (or would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. - `bmad-loop tui` — the interactive dashboard (`--low-frame-rate` for slow/SSH links). - `bmad-loop probe-adapter ` (`collect-adapter-data`) — collect + sanitize adapter-finalization data for a CLI profile; default zero-launch scan, opt-in `--probe` live capture. - Every command takes `--project ` (default: current directory). Any `` accepts a partial — the tail after the last `-`, shortened to any unique prefix. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 8e9b6501..5ac4fbca 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -699,6 +699,21 @@ Two seam facts worth internalizing: on disk; every other role builds `plain`. Both variants of a family share the `(*args, paths, **kwargs)` dev `__init__` contract, so honoring it is all an out-of-tree class must do to slot into that machinery. +- **The bootstrap's keyword set grows.** Accept `**kwargs` in both variants. + `runsetup.make_adapters` builds every class with `cls(**build_kwargs)`, and that + dict is a _description of the run_ — `run_dir`, `policy`, `profile`, the usage and + nudge settings, `events_dir` (the run's hook-event channel), plus `mux` for a + `needs_mux=True` kind and `paths` for the `dev` variant. Core adds to it as the run + gains things worth describing; `events_dir` arrived that way (#494). A class with a + closed signature raises `TypeError` on the first keyword it has not heard of, + before the session starts. + + The bundled families carry closed signatures instead, which is not a second + pattern to copy: they are edited in the same commit that adds the keyword, and an + out-of-tree class cannot be. What they do model is the other half of the + obligation — accept what you have no use for rather than refusing it. + `opencode_http` takes `events_dir` and immediately `del`s it, because that family + observes over SSE and fires no hooks, so there is no channel for it to point at. An entry-point profile is held to the same invariants a TOML profile is (hook dialect, path containment, `env_fault_patterns` compilation, …): it is validated on diff --git a/docs/setup-guide.md b/docs/setup-guide.md index ca10d4d6..99811ca3 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -331,7 +331,9 @@ bmad-loop cleanup --project # kill leftover tmux sessions ``` `clean --hard` permanently deletes runs instead of archiving them (we're removing the tool, so -there's nothing to keep). See the disk-reclamation coverage in +there's nothing to keep). Running it **first** also matters for the out-of-tree half: each run's +control-plane directory lives under the user-scoped state root, and `clean` is what collects it +(step 2 covers what is left over). See the disk-reclamation coverage in [docs/FEATURES.md](FEATURES.md) and the [command reference](../README.md#command-reference) for what each command touches. Make sure no run is still live (Editor open, session attached) first. @@ -345,6 +347,19 @@ Delete the `.bmad-loop/` directory. This removes the hook relay script rm -rf .bmad-loop/ ``` +One piece is **not** under `.bmad-loop/`: each run's hook-event channel lives in a user-scoped +state root outside the project (#494) — `$XDG_STATE_HOME/bmad-loop`, else +`~/.local/state/bmad-loop`; `%LOCALAPPDATA%\bmad-loop\state` on Windows; or whatever +`BMAD_LOOP_STATE_DIR` names. Step 1 collected this project's runs from it, leaving an empty +per-project directory whose name is a digest of the project path — not something to pick out by +hand. That root is shared by **every** project on this machine, so delete the whole thing only if +you are removing bmad-loop everywhere: + +```bash +# all projects on this machine — same cascade the orchestrator resolves +rm -rf "${BMAD_LOOP_STATE_DIR:-${XDG_STATE_HOME:-$HOME/.local/state}/bmad-loop}" +``` + ### 3. Remove the bundled skills `init` installed the three bundled `bmad-loop-*` skill directories — delete only those. **Leave diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index 94a82deb..8f98c1a9 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/src/bmad_loop/adapters/generic.py @@ -410,6 +410,7 @@ def __init__( usage_grace_s: float | None = None, stop_without_result_nudges: int | None = None, mux: TerminalMultiplexer | None = None, + events_dir: Path | None = None, ): self.run_dir = run_dir self.policy = policy @@ -441,7 +442,19 @@ def __init__( self.name = f"{profile.name}-tmux" self.binary = binary or profile.binary self.session_name = f"bmad-loop-{run_dir.name}" - self.watcher = SignalWatcher(run_dir / "events") + # The run's hook-event channel (#494): the out-of-tree directory the run + # bootstrap resolved, plus the legacy in-tree one kept under poll so a + # project whose installed relay predates the move still completes its + # sessions. `events_dir` is handed in rather than derived here because + # deriving it needs the PROJECT, and the only project this class can + # reach is `run_dir.parents[2]` — a shape real run dirs have and test run + # dirs do not, so a derivation would key the watcher off a directory that + # is not the project (see `_ensure_session`, which accepts exactly that + # weakness for a session tag but must not for the completion channel). + # Defaulting to the legacy dir keeps direct construction (tests, any + # caller outside `runsetup.make_adapters`) working unchanged; the + # bootstrap always passes one, pinned by a test. + self.watcher = SignalWatcher(events_dir or run_dir / "events", run_dir / "events") self.tasks_dir = run_dir / "tasks" self.logs_dir = run_dir / LOGS_DIR self.tasks_dir.mkdir(parents=True, exist_ok=True) diff --git a/src/bmad_loop/adapters/opencode_http.py b/src/bmad_loop/adapters/opencode_http.py index 28bb4968..09d9c9e5 100644 --- a/src/bmad_loop/adapters/opencode_http.py +++ b/src/bmad_loop/adapters/opencode_http.py @@ -390,7 +390,14 @@ def __init__( extra_args: tuple[str, ...] | None = None, usage_grace_s: float | None = None, stop_without_result_nudges: int | None = None, + events_dir: Path | None = None, ): + # `events_dir` is accepted and unused: this family observes over SSE and + # fires no hooks, so it has no event channel to point at. It is part of + # the run description `runsetup.make_adapters` hands every family (#494), + # and refusing the kwarg here would make the bootstrap branch per family + # on a value that costs nothing to carry. + del events_dir self._httpx = _require_httpx() self.run_dir = run_dir self.policy = policy diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 4077272a..0fb60268 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -64,6 +64,7 @@ "hooks.config-parse", "hooks.registered", "hooks.relay-present", + "hooks.relay-stale", "mux.backend", "mux.preflight", "mux.backends-detected", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 0db5f723..6cdb87e4 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -21,6 +21,7 @@ deferredwork, devcontract, envvars, + events, frontmatter, gates, install, @@ -495,10 +496,13 @@ def cmd_validate(args: argparse.Namespace) -> int: # A distinct id, not a repurposed `hooks.registered` — the two answer different # questions and an operator needs to see which one failed. # - # COUPLING (#461 Phase 2): Phase 2 moves the relay to - # ` -m bmad_loop.hookrelay` and retires HOOK_SCRIPT_REL. It must - # RETARGET this check to stat the registered interpreter (`hooks.interpreter`), - # not drop it — the stall it guards against survives the move. + # COUPLING (#461 Phase 2): Phase 2 moves the relay to the installed console + # script — `bmad-loop relay ` (cmd_relay / events.py), NOT the + # ` -m bmad_loop.hookrelay` spelling this once anticipated — and + # retires HOOK_SCRIPT_REL. It must RETARGET this check to stat what the + # registration actually points at (the resolved `bmad-loop` executable), not + # drop it — the stall it guards against survives the move: an entry point that + # is gone or unreadable strands every hook event exactly like a missing script. if any_hooks_registered: relay = project / install.HOOK_SCRIPT_REL # Existence is not enough: `is_file()` stays True for a mode-000 file, and @@ -537,6 +541,38 @@ def cmd_validate(args: argparse.Namespace) -> int: {"path": str(relay)}, ) + # #494 Phase 4: present-and-readable is not current. The relay is COPIED + # into the project by `init`, so an upgraded orchestrator routinely drives + # sessions through a relay written by an older wheel — and the #494 move + # is exactly the kind of change that skew hides: a pre-move relay writes + # its events to the in-tree `/events` while the operator believes + # the channel left the project tree, so a branch switch can still take the + # control plane away mid-run. + # + # A WARNING, never a problem, and validate's exit code must not move: + # Phase 3's fallback pair keeps a stale relay FUNCTIONAL (it writes the + # legacy directory, which SignalWatcher still polls), so the run completes + # — the operator is losing the property, not the loop. `passed` counts + # only problems, so `warn` is what says "degraded but working". + stale = install.hook_script_current(project) + if stale is False: + report.warn( + "hooks.relay-stale", + f"the installed hook relay {relay} differs from this bmad-loop's " + f"— it is from another version, or was edited. Events may still be " + f"written inside the project tree; run `bmad-loop init` to refresh it", + {"path": str(relay)}, + ) + elif stale is True: + report.ok( + "hooks.relay-stale", + f"hook relay script up to date: {relay}", + {"path": str(relay)}, + ) + # `None` (unreadable/undecodable on either side) reports nothing: the + # relay-present block above already spoke for the cases an operator can + # act on, and "I could not compare" is not a finding about their project. + # Adapter-kind validity is enforced against the LIVE registry, never a # hardcoded set: a profile.adapter naming no registered kind is a config error # (a typo, or an uninstalled plugin package). External adapter/profile packages @@ -1738,6 +1774,24 @@ def _render_invocation(pol, project: Path, role: str, prompt: str) -> str: return " ".join(argv) +def _events_dir_preview(project: Path) -> str | None: + """``BMAD_LOOP_EVENTS_DIR`` as a real run would set it, with a ```` + placeholder standing in for the id no dry run has (a preview creates nothing, + so the placeholder never reaches a filesystem). Printed once per preview + rather than on every story's ``env:`` line: only the run id varies, and the + path is long. + + ``None`` — plus the state root's own error on stderr — when no state root can + be resolved. A real run resolves the same path while building its adapters, so + silently dropping the line would turn a preview into a promise the run cannot + keep.""" + try: + return str(runs.events_dir_for(project, "")) + except runs.StateRootError as e: + print(f"warning: {e}", file=sys.stderr) + return None + + def _dry_run( paths: bmadconfig.ProjectPaths, pol, @@ -1766,6 +1820,8 @@ def render(role: str, prompt: str) -> str: print("no actionable stories") return 0 print(f"would process {len(queue)} stories (gates={pol.gates.mode}):") + if (events_dir := _events_dir_preview(paths.project)) is not None: + print(f" env (every session): BMAD_LOOP_EVENTS_DIR={events_dir}") dev_skill = _dev_skill_for_role(pol, paths.project, "dev") review_skill = _dev_skill_for_role(pol, paths.project, "review") for story in queue: @@ -1810,6 +1866,8 @@ def _dry_run_stories( f"stories mode: {len(rows)} stories from {folder}/stories.yaml " f"(gates={pol.gates.mode}){spec_ok}" ) + if (events_dir := _events_dir_preview(paths.project)) is not None: + print(f" env (every session): BMAD_LOOP_EVENTS_DIR={events_dir}") print("linear schedule (list order — no depends_on, strictly serial):") dev_skill = _dev_skill_for_role(pol, paths.project, "dev") for row in rows: @@ -3327,6 +3385,13 @@ def cmd_clean(args: argparse.Namespace) -> int: freed += wt_bytes trimmed.append(run_dir.name) + # After the loop, so the counterparts the removals above already took are gone + # from the enumeration rather than counted twice. Their bytes stay out of + # `freed` on purpose: a state dir holds consumed event files and nothing else, + # so sizing every one of them would buy kilobytes of accuracy for a walk of a + # second tree. The count is the honest report of what went. + swept = len(runs.reconcile_orphan_state_dirs(project, dry_run=dry)) + if args.json: machine.emit( clean_document( @@ -3340,12 +3405,12 @@ def cmd_clean(args: argparse.Namespace) -> int: deleted=deleted, protected=protected, unverifiable_pid=unverifiable, + state_dirs_swept=swept, ) ) return 0 - if not worktrees and not trimmed and not archived and not deleted: - print("nothing to reclaim") - else: + reclaimed = bool(worktrees or trimmed or archived or deleted) + if reclaimed: head = "would reclaim" if dry else "reclaimed" print( f"{head} ~{_human_bytes(freed)}: {len(worktrees)} worktree(s), " @@ -3355,6 +3420,12 @@ def cmd_clean(args: argparse.Namespace) -> int: print(f" archived {name} -> .bmad-loop/archive/{name}.tar.gz") for name in deleted: print(f" deleted {name}") + if swept: + # its own line rather than a fifth count in the summary above, which is a + # reclaimed-bytes report these dirs are deliberately outside of + print(f"{'would sweep' if dry else 'swept'} {swept} orphaned run state dir(s)") + if not reclaimed and not swept: + print("nothing to reclaim") if protected: print(f"left {len(protected)} live/resumable run(s) untouched") return 0 @@ -3639,6 +3710,41 @@ def cmd_init(args: argparse.Namespace) -> int: return install_into(project, clis=clis, skills=args.skills, force_skills=args.force_skills) +def cmd_relay(args: argparse.Namespace) -> int: + """``bmad-loop relay `` — the hook relay as an installed console script. + + **Nothing points at it yet.** ``init`` still registers the copied workspace + relay (``install._hook_command`` emits `` /.bmad-loop/ + bmad_loop_hook.py ``), so no installed hook reaches this handler today; + it is the target #461 Phase 2 retargets those registrations to, and that move + carries its own obligation — see the COUPLING note on ``hooks.relay-present``, + which must be retargeted rather than dropped in the same change. Said here + because a console script that exists and is documented reads as the live path, + and an operator debugging a lost Stop needs to know which relay actually ran. + + Total by contract, unlike every other handler: a coding CLI runs this INSIDE + the session whose completion it reports, and several of them surface a + non-zero hook exit as a failed tool call in that session. So nothing here + escalates. :func:`events.relay` already swallows the relay-level failures + (unset env, garbage stdin, a hostile events dir); the backstop below covers + the rest — an unexpected exception is a bug in this file, and a bug must not + be the reason a run's Stop signal turns into a broken session. It reports on + stderr, never stdout: hook stdout is parsed by the host. + + Dispatched from ``main()`` BEFORE its shared ``try``/``except`` on purpose — + see the comment there. + """ + try: + return events.relay(args.event, sys.stdin) + except Exception as e: + # Deliberately broad, and deliberately not re-raised: the alternative to a + # diagnostic line here is a traceback on the host's hook channel plus a + # non-zero rc. Narrower than the BaseException it could be — a Ctrl+C or a + # SystemExit is not a relay-level problem and keeps its own exit path. + print(f"bmad-loop relay: {type(e).__name__}: {e}", file=sys.stderr) + return ExitCode.OK + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( prog="bmad-loop", @@ -3978,7 +4084,34 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: "fixes repaint tearing over slow/SSH links; also settable via [tui] low_frame_rate", ) + # Registered directly rather than through `add()`: `relay` takes no --project. + # It is handed a run directory by the engine (via the session env) and must + # work with no project state at all — see the dispatch below. + relay_p = sub.add_parser( + "relay", + help="write one session event file from a coding-CLI hook payload on stdin " + "(a hook target for machines, not a command to run by hand)", + ) + relay_p.add_argument( + "event", + nargs="?", + default="Unknown", + help="canonical event name (Stop, SessionStart, …); the hooks always pass one", + ) + relay_p.set_defaults(func=cmd_relay) + args = parser.parse_args(argv) + # `relay` dispatches HERE, ahead of everything below, and the placement is the + # contract rather than an optimization. A coding CLI runs `bmad-loop relay Stop` + # inside the session whose completion it reports, and a hook that exits non-zero + # is surfaced by several hosts as a failed tool call in that session — so the + # `except` arms below, which print `error: …` and return 1 (or 130), are exactly + # the outcome the hook contract forbids. `_configure_mux(_project(args))` is + # skipped for the same reason and one more: relay touches neither mux nor policy, + # and a project whose policy.toml is broken must still be able to report that its + # session stopped. `cmd_relay` is total, so nothing is lost by not wrapping it. + if args.func is cmd_relay: + return cmd_relay(args) try: # Install the policy [mux] backend choice before dispatch: several # handlers (probe/diagnose/attach/stop/cleanup/tui) reach the mux diff --git a/src/bmad_loop/data/bmad_loop_hook.py b/src/bmad_loop/data/bmad_loop_hook.py index a1eac048..7b22db52 100644 --- a/src/bmad_loop/data/bmad_loop_hook.py +++ b/src/bmad_loop/data/bmad_loop_hook.py @@ -10,9 +10,23 @@ field extraction below tries each. agy alone carries no cwd, sending the workspacePaths list instead. Reads the hook payload from stdin and writes one event file -into the orchestrator's run directory. No-ops (exit 0) unless the session was +into the orchestrator's events directory. No-ops (exit 0) unless the session was spawned by bmad-loop (detected via env vars set on the tmux window), so normal interactive sessions are unaffected. + +Where that directory is: $BMAD_LOOP_EVENTS_DIR when the orchestrator names one, +else the legacy $BMAD_LOOP_RUN_DIR/events. The channel moved out of the project +tree in #494 so a branch switch or a worktree mount cannot take a live run's +control plane away, and this script is COPIED into the target project at init +time — so an upgraded orchestrator regularly drives sessions whose installed +copy is this file's older self, which knows only the legacy path. That pairing +is what the orchestrator's own dual poll (signals.SignalWatcher) covers. + +$BMAD_LOOP_EVENTS_DIR is deliberately NOT part of the no-op detector above: the +mirror-image skew — an older orchestrator that sets only RUN_DIR/TASK_ID driving +a session whose installed relay is this newer file — must still write its +events, and requiring the new variable would silently stall every one of those +runs instead. """ import json @@ -195,8 +209,13 @@ def main() -> int: # agy sends no cwd — it sends workspacePaths, a list of workspace roots. "cwd": payload.get("cwd") or _first_workspace(payload), } + # The orchestrator's own events dir when it named one, else the legacy + # in-tree location this file's older selves are still installed at (see the + # module docstring). `or`, not a presence test: an exported-but-empty value + # names the launch cwd, which is not a control plane. + events_dir = os.environ.get("BMAD_LOOP_EVENTS_DIR") or os.path.join(run_dir, "events") try: - _write_event(os.path.join(run_dir, "events"), f"{ts}-{task_id}-{event_name}.json", event) + _write_event(events_dir, f"{ts}-{task_id}-{event_name}.json", event) except OSError: # A hostile or broken events dir must degrade to the orchestrator's # normal session_timeout_min path, never surface as a hook failure that diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index fab18aad..c4571a3e 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/src/bmad_loop/data/settings/core.toml @@ -374,7 +374,7 @@ description = "⚠ ON: capture failed-unit diffs with NO size limit (overrides t [[section]] name = "cleanup" -description = "disk reclamation for .bmad-loop/runs (terminal runs only)" +description = "disk reclamation for .bmad-loop/runs, plus each run's out-of-tree control-plane dir (terminal runs only)" [[section.field]] key = "run_retention" kind = "int" diff --git a/src/bmad_loop/diagnostics.py b/src/bmad_loop/diagnostics.py index dcb76b01..edc0cb4c 100644 --- a/src/bmad_loop/diagnostics.py +++ b/src/bmad_loop/diagnostics.py @@ -63,10 +63,14 @@ SCHEMA_VERSION = 1 DEFAULT_JOURNAL_CAP = 200 -# Run-dir subdirectories whose mere existence/size is diagnostic but whose -# CONTENTS are off-limits (raw tmux panes = code, prompts, feedback prose, -# patches, full worktree checkouts). We stat them; we never read into output. +# Subdirectories whose mere existence/size is diagnostic but whose CONTENTS are +# off-limits (raw tmux panes = code, prompts, feedback prose, patches, full +# worktree checkouts). We stat them; we never read into output. +# +# All are run-dir-relative EXCEPT "events", which since #494 lives out of the +# project tree at the user state root — see `_category_roots`. _FILE_CATEGORIES = ("logs", "tasks", "feedback", "bundles", "failed", "worktrees", "events") +_EVENTS_CATEGORY = "events" # Journal fields that name a proprietary identifier — pseudonymized, not dropped, # so events stay correlatable. Maps field name -> alias namespace. @@ -297,30 +301,62 @@ def collect_env(project: Path) -> EnvInfo: ) -def summarize_files(run_dir: Path) -> list[FileGroup]: - """Counts/sizes only — file contents are NEVER opened into the output.""" +def _category_roots(category: str, run_dir: Path, events_dir: Path | None) -> list[Path]: + """Where a category's files live. One directory for all but ``events``. + + The events channel has TWO live roots since #494 (Phase 3): the primary is + out of the project tree under the user state root, and the legacy in-tree + ``/events`` is still both written and polled. It has to be, because + the hook relay is COPIED into the project by ``init`` — an upgraded + orchestrator routinely drives sessions whose relay predates the move and + still writes in-tree, so the watcher dual-polls. Counting only one root would + report zero events for precisely the runs whose event routing is what a + maintainer is reading the dump to understand. + + Both are summed into ONE ``FileGroup`` named ``events``: the payload shape is + the schema, and splitting the category (or adding a field) would be a break + for a v1 consumer. Which root the events came from is not what the count is + for — "did the hooks fire at all" is. + """ + if category != _EVENTS_CATEGORY: + return [run_dir / category] + legacy = run_dir / _EVENTS_CATEGORY + # Deduped by spelling so a state root deliberately pointed inside the run dir + # (BMAD_LOOP_STATE_DIR is honoured as spelled) cannot double-count. + if events_dir is None or events_dir == legacy: + return [legacy] + return [events_dir, legacy] + + +def summarize_files(run_dir: Path, *, events_dir: Path | None = None) -> list[FileGroup]: + """Counts/sizes only — file contents are NEVER opened into the output. + + ``events_dir`` is this run's out-of-tree event channel (#494); ``None`` when + the caller could not derive one, which degrades to the legacy in-tree + location alone rather than dropping the category. + """ groups: list[FileGroup] = [] for category in _FILE_CATEGORIES: - root = run_dir / category - if not root.is_dir(): - continue count = 0 total_bytes = 0 total_lines = 0 - for p in root.rglob("*"): - if not p.is_file(): + for root in _category_roots(category, run_dir, events_dir): + if not root.is_dir(): continue - count += 1 - try: - total_bytes += p.stat().st_size - except OSError: - pass - if category == "logs": + for p in root.rglob("*"): + if not p.is_file(): + continue + count += 1 try: - with p.open("rb") as f: - total_lines += sum(1 for _ in f) + total_bytes += p.stat().st_size except OSError: pass + if category == "logs": + try: + with p.open("rb") as f: + total_lines += sum(1 for _ in f) + except OSError: + pass if count: groups.append( FileGroup( @@ -500,6 +536,33 @@ def _coarsen_date(started_at: str | None) -> str | None: return head if sanitize.looks_like_identifier(head.replace("-", "0")) else None +def _events_dir(state: RunState) -> Path | None: + """This run's out-of-tree event channel, or ``None`` if it is not derivable. + + Derived from the run's OWN recorded project and id rather than threaded down + from ``cmd_diagnose``'s ``--project``, which is the smaller change and the + truer one: ``--all`` dumps every run under a project, and a run carries the + project it was started against. ``run_dir`` cannot answer this — the state + root is keyed by a digest of the resolved project, not by the run dir's + ancestry. + + Every failure mode is observation, so every one of them degrades. The + derivation resolves the project (``runs.project_tag``) and consults the host + for a state root, and a dump is routinely read on a machine that is not the + one that produced it: an unresolvable project, or a host with no derivable + state root, must cost the events count and nothing else. + """ + from . import runs + + try: + return runs.events_dir_for(Path(state.project), state.run_id) + except Exception: + # No `# nosec`: bandit's B110/B112 are about `pass`/`continue` bodies, and + # a directive naming a rule that never fires here would read as a waived + # finding. The breadth is deliberate and stated above, not silenced. + return None + + def collect_run(run_dir: Path, *, pseudo: sanitize.Pseudonymizer, cap: int) -> RunDiag: state: RunState = load_state(run_dir) tasks = list(state.tasks.values()) @@ -541,7 +604,7 @@ def collect_run(run_dir: Path, *, pseudo: sanitize.Pseudonymizer, cap: int) -> R session_tally=_session_tally(tasks), tasks=[_task_diag(t, pseudo, weight) for t in tasks], journal=summarize_journal(Journal(run_dir).entries(), pseudo, epic_by_key, cap=cap), - files=summarize_files(run_dir), + files=summarize_files(run_dir, events_dir=_events_dir(state)), ) diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 58d66f3a..2c14a09f 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -454,6 +454,7 @@ def clean_document( deleted: list[str], protected: list[str], unverifiable_pid: list[str], + state_dirs_swept: int, ) -> dict[str, object]: """The `clean --json` document: the disk this invocation reclaimed, or — under ``--dry-run`` — would reclaim. @@ -483,6 +484,15 @@ def clean_document( `[cleanup] run_retention`. The other three are the configured policy as loaded. Note `--hard` overrides `archive_old` for this invocation only, so it does not change the reported value; the outcome shows in `deleted`. + + `state_dirs_swept` counts the orphaned out-of-tree control-plane dirs the + invocation reclaimed (#494) — run state dirs under the user-scoped state root + with no run dir left to own them. A count rather than a list, because that is + exactly what the text mode reports and the paths name a location outside the + project that no caller acts on per-item. It is an additive field on the + existing schema version: a v1 consumer reads every field it already knew. + Those dirs hold only consumed event files, so their bytes are not in + `freed_bytes` — an accepted under-count of a few kilobytes at most. """ return { "schema_version": CLEAN_SCHEMA_VERSION, @@ -500,4 +510,5 @@ def clean_document( "deleted": list(deleted), "protected": list(protected), "unverifiable_pid": list(unverifiable_pid), + "state_dirs_swept": state_dirs_swept, } diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 9579a5c7..1d99dc28 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -54,7 +54,7 @@ from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .recovery_flow import RecoveryFlow -from .runs import clear_graceful_stop, graceful_stop_requested, kill_session +from .runs import clear_graceful_stop, events_dir_for, graceful_stop_requested, kill_session from .sprintstatus import ACTIONABLE_STATUSES from .sprintstatus import advance as sprint_advance from .sprintstatus import load as load_sprint_status @@ -4057,6 +4057,21 @@ def _run_session( env = { "BMAD_LOOP_MODE": "1", "BMAD_LOOP_RUN_DIR": str(self.run_dir), + # Where this session's hook relay writes its events (#494). The one + # required producer site: every engine-driven session — dev/review, + # sweep bundles, stories, injected plugin workflows — is dispatched + # through this dict. The deliberate non-sites all fail closed without + # it: `resolve.py` sets no BMAD_LOOP_TASK_ID, so the relay no-ops and + # an interactive resolve session produces no events at all; `probe.py` + # captures through BMAD_LOOP_PROBE_CAPTURE_DIR and its own probe relay; + # `plugins/bus.py` spawns plain shells with no task id; the Unity + # plugin's helper scripts are not CLI sessions. + # + # Keyed on the run's project and id, not on `self.run_dir`, so the + # value cannot drift from the directory `runsetup.make_adapters` + # pointed this run's SignalWatcher at — the producer and the consumer + # of one channel, derived by one function from the same two inputs. + "BMAD_LOOP_EVENTS_DIR": str(events_dir_for(self.paths.project, self.run_dir.name)), "BMAD_LOOP_TASK_ID": task_id, "BMAD_LOOP_STORY_KEY": task.story_key, } diff --git a/src/bmad_loop/envvars.py b/src/bmad_loop/envvars.py index e67d1c02..c3a20e12 100644 --- a/src/bmad_loop/envvars.py +++ b/src/bmad_loop/envvars.py @@ -8,9 +8,14 @@ Each reader preserves its call site's exact semantics — same parse, same fallback — so routing a site through here changes nothing observable. Only the -three core vars belong here; the `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` -family read by the bundled Unity plugin's stand-alone helper scripts is that -plugin's own contract (documented in the game-engine guide) and stays with it. +core operator/test knobs belong here (the count is deliberately not stated — it +has already changed once); the `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` family +read by the bundled Unity plugin's stand-alone helper scripts is that plugin's +own contract (documented in the game-engine guide) and stays with it. Nor do the +session-protocol vars the engine *injects* into a child session +(`BMAD_LOOP_RUN_DIR`, `BMAD_LOOP_TASK_ID`, …): those have a producing side inside +the orchestrator, and the stdlib-only relays that read them back cannot import +this module at all. """ from __future__ import annotations @@ -24,6 +29,9 @@ MUX_BACKEND = "BMAD_LOOP_MUX_BACKEND" #: Forces the process-host implementation by registered name (test / override). PROCESS_HOST = "BMAD_LOOP_PROCESS_HOST" +#: Overrides the user-scoped state root that per-run control-plane state lives +#: under (see :func:`runs.state_root`), replacing the whole platform cascade. +STATE_DIR = "BMAD_LOOP_STATE_DIR" def session_timeout_s() -> float | None: @@ -69,3 +77,29 @@ def mux_backend() -> str | None: def process_host() -> str | None: """The forced process-host name, or ``None`` when unset.""" return os.environ.get(PROCESS_HOST) + + +def state_dir() -> str | None: + """The overriding bmad-loop state root, or ``None`` when unset. + + Verbatim like the two name readers above — :func:`runs.state_root` uses the + value as the state root itself, so an operator who names a directory gets that + directory. Silently ignoring a stated override in favour of the platform + cascade would be the same failure :func:`mux_backend` refuses: a loud + misconfiguration turned into a quiet auto-select, discoverable only by + noticing where a run's events did *not* appear. + + Reading verbatim is not the same as accepting anything: this reader reports + what is set, and :func:`runs.state_root` judges it. A **relative** value is + refused there rather than resolved, because the root is read by two processes + with different working directories — see that function for the full reason. + The split is deliberate; a reader that silently rewrote its variable would + make the refusal impossible to state. + + The one value not passed through is the empty string, which reads as unset. + ``export BMAD_LOOP_STATE_DIR=`` is what an unset-looking export leaves behind, + and it is not a directory an operator can have meant: ``Path("")`` is the + *current directory*, so honouring it would silently root the control plane at + whatever cwd the loop happened to be launched from. + """ + return os.environ.get(STATE_DIR) or None diff --git a/src/bmad_loop/events.py b/src/bmad_loop/events.py new file mode 100644 index 00000000..273439f9 --- /dev/null +++ b/src/bmad_loop/events.py @@ -0,0 +1,266 @@ +"""Importable twin of the hardened event write in ``data/bmad_loop_hook.py``. + +The hook script is COPIED into every target project and runs inside the coding +CLI's process under whatever interpreter the host has, so it is stdlib-only by +contract (its docstring says so) and cannot import ``bmad_loop`` to reach this +module — and this module cannot import it back, since it ships as package DATA +rather than as an importable module. Hence a twin rather than shared code: +``_LINK_REPARSE_TAGS``, ``_first_workspace``, ``_is_link_like``, ``_write_all`` +and ``_write_event`` below are byte-identical copies of the hook's, pinned that +way by ``tests/test_events.py::test_the_twinned_source_is_identical`` — which +AST-extracts both sides and compares the source segments, so a fix applied to one +writer of the events control plane and not the other cannot pass review silently. + +Because they are byte-identical, their docstrings and comments are written from +the hook script's vantage point ("this relay runs under whatever interpreter the +host has") and stay that way on purpose: rewording either side to suit its own +file breaks parity, and diverging is exactly what two separately-hardened writers +of one control plane must not do. What the shared text says about the attack, the +platform branches, and the residual Windows windows holds for both. + +The rest of the module is this side's own: the payload shaping the hook does +inline in its ``main()``, and :func:`relay`, which backs ``bmad-loop relay +`` — the #461 Phase 2 hook target, an installed console script instead of +a file path inside the workspace that a branch switch can take away. +""" + +from __future__ import annotations + +import json +import os +import stat +import time +from typing import IO, Any + +# Windows reparse tags that make a directory entry REDIRECT somewhere else, +# compared against os.lstat().st_reparse_tag (Windows, 3.8+). Deliberately not +# os.path.isjunction(), which is 3.12+ — this relay runs under whatever +# interpreter the host has, not under the orchestrator's. Deliberately not "any +# reparse tag" either: cloud placeholders (OneDrive) and dedup stubs are reparse +# points too, and refusing those would stall a legitimate run. Empty on POSIX. +_LINK_REPARSE_TAGS = tuple( + tag + for tag in ( + getattr(stat, "IO_REPARSE_TAG_SYMLINK", None), + getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None), + ) + if tag is not None +) + + +def _first_workspace(payload): + paths = payload.get("workspacePaths") + if isinstance(paths, list) and paths and isinstance(paths[0], str): + return paths[0] + return None + + +def _is_link_like(path): + """True when `path` redirects elsewhere: a POSIX symlink, or a Windows + symlink OR DIRECTORY JUNCTION. + + `os.path.islink()` is False for a junction — junctions are a distinct + reparse kind, which is why `os.path.isjunction()` exists at all. On Windows + the junction is the arm that matters: `mklink /J` needs no elevation, while + a directory symlink needs SeCreateSymbolicLinkPrivilege or Developer Mode — + so the unprivileged attack is exactly the one `islink()` misses. + """ + if os.path.islink(path): + return True + try: + return getattr(os.lstat(path), "st_reparse_tag", 0) in _LINK_REPARSE_TAGS + except OSError: + return False + + +def _write_all(fd, data): + """Write every byte of `data` to `fd`. + + `os.write()` may write FEWER bytes than asked and simply return the count. A + truncated event file is not merely retried, it is lost: `SignalWatcher.poll` + adds a filename to its consumed set BEFORE parsing it (signals.py), so + malformed JSON is skipped and never re-read — the session's Stop signal is + gone for good and the run waits out `session_timeout_min`. The buffered + `open()` this replaced looped internally; the raw fd needed for + O_NOFOLLOW/dir_fd does not, so loop here. + """ + view = memoryview(data) + while view: + written = os.write(fd, view) + if written <= 0: # not observed in practice; a spinning hook is worse + raise OSError("short write to the event file") + view = view[written:] + + +def _write_event(events_dir, name, event): + """Write one event file into `events_dir`, refusing to follow a redirect. + + The events dir is the orchestrator's control plane. A driven session has + write access to the project, so it could plant `/events` as a + symlink (or, on Windows, a junction) and redirect — or swallow — the + completion signal, stalling the run to `session_timeout_min` instead of + completing. `os.makedirs(exist_ok=True)` `isdir()`-checks THROUGH such a + link, so the refusal has to come before it. That refusal works on every + platform. + + Where the platform has them, the create+replace is anchored to a dir_fd + opened O_NOFOLLOW: every later operation goes through that fd, so a swap + after the check cannot reach the write. Windows has neither + O_NOFOLLOW/O_DIRECTORY nor a handle-relative open (`os.supports_dir_fd` is + empty — dir_fd is implemented with the POSIX `*at` calls), so its fallback + re-resolves the path and the check-to-write window stays open there. It is + NARROWED, not closed: the redirect check runs again after the payload is + written and before it is published, so a swap still in place is refused and + the temp file removed. Two windows stay open on that path (#494), both + measured: a swap-and-restore around the create is undetectable from stdlib + Python, and a swap landing after the second check leaves the path-based + publish unable to find the temp file it wrote — it either raises or renames + a file the attacker planted inside the attacker's own directory. Neither + redirects the payload, and both end where a refusal ends: no event, so the + run waits out session_timeout_min. That is the same outcome an attacker gets + for free by leaving a redirect in place, which is refused without any race — + winning the race buys no capability, which is why the residual is accepted + rather than chased into ctypes/NtCreateFile inside a stdlib-only relay. + + Mode is 0o600 (narrowed from the umask-derived mode an ordinary `open()` + produced): only the operator running the loop reads these. + + Raises OSError on any refusal or failure; the caller degrades to a no-op. + """ + if _is_link_like(events_dir): + raise OSError(f"refusing to write events into a redirected directory: {events_dir}") + os.makedirs(events_dir, exist_ok=True) + data = json.dumps(event).encode("utf-8") + tmp = name + ".tmp" + o_nofollow = getattr(os, "O_NOFOLLOW", 0) + o_directory = getattr(os, "O_DIRECTORY", 0) + # O_BINARY is a no-op flag on POSIX; on Windows it stops the fd from + # newline-translating what os.write() puts through it. + create = os.O_WRONLY | os.O_CREAT | os.O_EXCL | o_nofollow | getattr(os, "O_BINARY", 0) + # Probe os.rename, not os.replace: CPython omits os.replace from + # supports_dir_fd on Linux even though it accepts src_dir_fd/dst_dir_fd, so + # probing it would leave this whole branch dead everywhere. This branch is + # POSIX-only by construction, and there rename(2) IS the atomic-replace + # primitive os.replace wraps — probe the function actually called. + if o_nofollow and o_directory and {os.open, os.rename} <= os.supports_dir_fd: + dir_fd = os.open(events_dir, os.O_RDONLY | o_directory | o_nofollow) + try: + fd = os.open(tmp, create, 0o600, dir_fd=dir_fd) + try: + _write_all(fd, data) + finally: + os.close(fd) + os.rename(tmp, name, src_dir_fd=dir_fd, dst_dir_fd=dir_fd) + finally: + os.close(dir_fd) + return + # Fallback (Windows): no dir_fd to anchor to, so the create below re-resolves + # events_dir by path. A swap into a junction between the check above and this + # create would have put the temp file inside the attacker's directory. Check + # again before publishing, so a swap that is still in place is refused rather + # than followed — the realistic shape, since a junction has to persist to + # capture the events the attacker is after. + tmp_path = os.path.join(events_dir, tmp) + fd = os.open(tmp_path, create, 0o600) + try: + _write_all(fd, data) + finally: + os.close(fd) + if _is_link_like(events_dir): + try: + os.unlink(tmp_path) + except OSError: + pass + raise OSError(f"events directory was redirected mid-write: {events_dir}") + os.replace(tmp_path, os.path.join(events_dir, name)) + + +# --------------------------------------------------------------- this side only + + +def event_file_name(ts: int, task_id: str, event_name: str) -> str: + """The event file's name. Sorted-by-time by construction (``ts`` first, fixed + width in practice), and carrying the task id so ``SignalWatcher`` can attribute + a file without opening it. Mirrors the hook's f-string exactly; the twin above + stops at the write, so this and :func:`shape_event` are pinned behaviorally + instead (``test_relay_and_hook_produce_the_same_event``).""" + return f"{ts}-{task_id}-{event_name}.json" + + +def shape_event(ts: int, event_name: str, task_id: str, payload: dict[str, Any]) -> dict[str, Any]: + """The event record the orchestrator consumes, built from one hook payload.""" + return { + "ts": ts, + "event": event_name, + "task_id": task_id, + # Payload keys vary by CLI: snake_case (claude/codex), conversation_id + # (cursor), or camelCase (copilot's sessionId/transcriptPath, agy's + # conversationId). Try each. + "session_id": ( + payload.get("session_id") + or payload.get("conversation_id") + or payload.get("sessionId") + or payload.get("conversationId") + ), + "transcript_path": payload.get("transcript_path") or payload.get("transcriptPath"), + # agy sends no cwd — it sends workspacePaths, a list of workspace roots. + "cwd": payload.get("cwd") or _first_workspace(payload), + } + + +def _read_payload(stdin: IO[str]) -> dict[str, Any]: + """The hook payload, or an empty dict for anything unreadable. + + A hook that fires with nothing on stdin, half a JSON document, undecodable + bytes, or a bare list is not an error the operator can act on — the event still + has to be written, because the run's completion signal rides on it. Every + non-dict outcome collapses to ``{}`` and the shaped event simply carries nulls. + ``UnicodeDecodeError`` and ``json.JSONDecodeError`` are both ``ValueError`` + subclasses and ride the same arm; ``OSError`` covers a closed or unreadable + descriptor. + """ + try: + payload = json.load(stdin) + except (ValueError, OSError): + return {} + return payload if isinstance(payload, dict) else {} + + +def relay(event_name: str, stdin: IO[str]) -> int: + """Write one event file for the session this process was spawned inside. + + The contract is the hook script's, because the hook config points at one or + the other and the orchestrator must not be able to tell which ran: a silent + no-op when the session was not spawned by bmad-loop (the env vars are the + detector), garbage stdin tolerated, and any ``OSError`` from a hostile or + broken events dir degrading to rc 0. Never anything on stdout — the CLI hosts + parse hook stdout — and never a non-zero rc, which several of them surface as + a failed tool call inside the very session whose completion this reports. + + Returns 0 unconditionally. Nothing here is worth failing a session over: the + orchestrator's fallback for a missing event is ``session_timeout_min``, and an + attacker who can suppress the event can already get that outcome by planting + the redirect ``_write_event`` refuses. + """ + run_dir = os.environ.get("BMAD_LOOP_RUN_DIR") + task_id = os.environ.get("BMAD_LOOP_TASK_ID") + if not run_dir or not task_id: + return 0 + ts = time.time_ns() + event = shape_event(ts, event_name, task_id, _read_payload(stdin)) + # $BMAD_LOOP_EVENTS_DIR when the orchestrator names one (#494 moved the + # channel out of the project tree), else the legacy in-tree location — the + # same preference, spelled the same way, as the copied hook script's `main()`. + # `or`, not a presence test: an exported-but-empty value names the launch cwd. + # The no-op detector above stays RUN_DIR + TASK_ID for the reason the hook's + # docstring gives: an older orchestrator sets neither the new variable nor any + # expectation that this relay needs it, and its sessions must still complete. + events_dir = os.environ.get("BMAD_LOOP_EVENTS_DIR") or os.path.join(run_dir, "events") + try: + _write_event(events_dir, event_file_name(ts, task_id, event_name), event) + except OSError: + # A hostile or broken events dir must degrade to the orchestrator's normal + # session_timeout_min path, never surface as a hook failure that fails the + # CLI window (mirrors the hook script's own write wrap). + return 0 + return 0 diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 885c33fb..311bb163 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1010,6 +1010,44 @@ def _review_findings(project: Path, tree: str) -> list[Finding]: return findings +def hook_script_current(project: Path) -> bool | None: + """Does the project's installed relay match the one this wheel would write? + + ``True`` yes, ``False`` stale (or otherwise divergent), ``None`` unknowable — + the installed copy or the packaged source could not be read as text. The + unknown arm is a third state and not a coerced ``False`` on purpose: the sole + caller (``cmd_validate``'s ``hooks.relay-stale``) reports what it knows, and + "I could not look" is not "your relay is out of date". + + Lives here, beside :func:`install_into`'s write of the same two paths, so the + reader and the writer of the relay stay in one module and one reviewer's view + — a comparison that resolved the source differently from the writer would + answer a different question. + + Compared as TEXT read with universal newlines, not as raw bytes. That is + precisely the round trip ``install_into`` performs (``read_text`` then + ``write_text``), and ``write_text`` translates ``\\n`` to ``os.linesep`` — so + on Windows every freshly-installed relay differs from the packaged source + byte-for-byte while being exactly what ``init`` writes. A byte compare would + call those installs permanently stale. + """ + try: + installed = (project / HOOK_SCRIPT_REL).read_text(encoding="utf-8") + packaged = ( + resources.files("bmad_loop.data") + .joinpath("bmad_loop_hook.py") + .read_text(encoding="utf-8") + ) + except (OSError, ValueError): + # ValueError covers UnicodeDecodeError (a relay overwritten with non-UTF-8 + # bytes); OSError covers missing/unreadable on either side. Observation + # degrades — the missing and unreadable cases already have their own + # finding (`hooks.relay-present`), and a packaged source this process + # cannot read is a broken wheel, not a stale project. + return None + return installed == packaged + + def _hook_command(project: Path, profile: CLIProfile, canonical_event: str) -> str: host = get_process_host() interp = host.hook_interpreter() diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index e70150d4..2fda7d43 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -9,11 +9,12 @@ import re import secrets import shutil +import sys import tarfile import time from pathlib import Path -from . import devcontract, verify +from . import devcontract, envvars, verify from .adapters.multiplexer import MultiplexerError, get_multiplexer from .journal import STATE_FILE, Journal, load_state, save_state from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask @@ -151,6 +152,185 @@ def attach_argv(run_id: str) -> list[str]: return attach_target_argv(session_target(run_id)) +# ------------------------------------------------------- user-scoped state root + + +class StateRootError(Exception): + """No user-scoped state root could be derived from this environment — every + candidate base was unset, empty, relative, or named the filesystem root. The + control plane has nowhere to live, and the caller must fail rather than guess + (see :func:`state_root`).""" + + +def _state_base(value: str | None) -> Path | None: + """``value`` as a usable base directory, or ``None`` when it cannot be one. + + The single rule every *derived* candidate below is held to, so the POSIX and + win32 branches cannot drift into judging their inputs differently. A base is + rejected when it is unset, empty, relative, or names the filesystem root + itself. The last three are the answers a broken environment gives *instead* of + raising, which is what makes them worth naming: + + - **empty**: ``os.path.expanduser("~")`` answers ``""`` on Windows for a + set-but-empty ``USERPROFILE``, and ``Path("")`` is the current directory. + - **relative**: including ``"~"`` itself, which is what ``expanduser`` returns + when it cannot expand at all. The state root would then move with the + launch cwd, and a run whose control plane it cannot find again is a run + that stalls to ``session_timeout_min`` rather than one that fails. + - **the root**: ``expanduser("~")`` answers ``"/"`` on POSIX for a set-but-empty + ``HOME`` (``posixpath`` folds the empty prefix to the root), which would put + ``/.local/state/bmad-loop`` on the filesystem root — a permission error for + an ordinary user and, for a containerised root, a silent write to ``/``. + ``base == base.parent`` is the root test on both flavours. + + ``os.path.isabs`` rather than :func:`platform_util.is_absolute_path`: the + latter is purpose-built for "must stay inside the project" guards and is + strictly broader — it calls the drive-*relative* ``C:foo`` absolute, which is + exactly the value that must not become a state root. The question here is the + platform's own, and each branch below only ever runs on its own platform. + """ + if not value or not os.path.isabs(value): + return None + base = Path(value) + return None if base == base.parent else base + + +def state_root() -> Path: + """The bmad-loop state root for this user: the out-of-tree home of per-run + control-plane state — the events channel (#494) and, later, the config digest + (#498). Outside the project tree because a branch switch, a worktree mount or + a rollback must not be able to take a live run's control plane away. + + Resolution, first answer wins: + + 1. ``BMAD_LOOP_STATE_DIR``, used as the state root **itself** — no + ``bmad-loop`` segment is appended, because the variable names our root + rather than a base to build one under. It is honoured as spelled (see + :func:`envvars.state_dir`) and is not passed through ``_state_base``: + *skipping* a stated override would be a silent countermand, where skipping + a derived base only moves on to the next guess. + + It must still be **absolute**, and a relative spelling raises rather than + being resolved for the operator. Absoluteness is not a matter of taste + here — the root is read by two processes with different working + directories. The engine exports it to the session as + ``BMAD_LOOP_EVENTS_DIR`` and the multiplexer launches that session at + ``spec.cwd`` (a worktree under isolation), while the watcher polls it from + the orchestrator's own cwd. A relative root therefore names two different + directories at once: the relay writes its Stop where nothing is watching, + and the run waits out ``session_timeout_min`` — the exact silent stall + ``_state_base`` rejects relative *derived* bases to avoid, and the one + this whole channel was moved out of the tree to prevent. + + Raising is not the countermand the paragraph above refuses: it names the + variable and the fix, where absolutizing against whichever cwd this + process happens to have would be the guess. The not-the-root half of + ``_state_base``'s rule is deliberately *not* applied — that half exists to + stop a broken environment's ``""`` from landing a guess at ``/``, and an + override is not a guess. + 2. POSIX — ``$XDG_STATE_HOME/bmad-loop`` when that variable names an absolute + path, else ``~/.local/state/bmad-loop``. A relative ``XDG_STATE_HOME`` is + *ignored*, which the XDG base-directory spec requires of its consumers. + (``install._shield_inherited_excludes`` resolves a relative + ``XDG_CONFIG_HOME`` instead of ignoring it — the opposite call for the + opposite reason: there we reproduce *git's* reading of the variable, here + we are the spec's own consumer.) + 3. win32 — ``%LOCALAPPDATA%\\bmad-loop\\state``, else + ``%USERPROFILE%\\AppData\\Local\\bmad-loop\\state``. ``LOCALAPPDATA`` names + the per-user, per-machine, non-roaming store Windows intends for exactly + this, and the second form is its documented default location. + + **Never** ``Path.home()`` on the win32 arm. It is ``ntpath.expanduser("~")``, + which prefers ``USERPROFILE`` and then falls back to ``HOMEDRIVE`` + + ``HOMEPATH`` — a pair that on a domain-joined machine may name a network home + share. A control plane whose atomic renames and ``O_NOFOLLOW``-anchored writes + live on an SMB share is not the local directory this needs, and the derivation + also disagrees with the one git uses for its own ``$HOME`` + (``install._shield_home_git_ignore`` documents that split in full). Reading + ``LOCALAPPDATA``/``USERPROFILE`` directly asks for the store by name instead of + inferring it from a home. + + Raises :class:`StateRootError` when no candidate answers. This is a write + path, so it raises rather than degrading to a plausible-looking default: + ``platform_util.resolve_or_lexical`` states the doctrine (observation may + degrade, repair writes must raise), and the degraded outcomes here are all + silent — a control plane at the cwd, or at ``/``, that the *next* process to + ask resolves somewhere else. + """ + override = envvars.state_dir() + if override: + # `os.path.isabs` on the raw string, matching `_state_base` exactly rather + # than `Path.is_absolute` — the rule and its reason are stated there. + if not os.path.isabs(override): + raise StateRootError( + f"{envvars.STATE_DIR} must name an absolute directory: {override!r} is " + "relative, and the state root is read by both this process and the " + "session it launches — which run from different working directories, " + "so a relative root names two different places and the run's " + "completion signal is written where nothing is watching" + ) + return Path(override) + if sys.platform == "win32": + local = _state_base(os.environ.get("LOCALAPPDATA")) + if local: + return local / "bmad-loop" / "state" + profile = _state_base(os.environ.get("USERPROFILE")) + if profile: + return profile / "AppData" / "Local" / "bmad-loop" / "state" + else: + xdg = _state_base(os.environ.get("XDG_STATE_HOME")) + if xdg: + return xdg / "bmad-loop" + home = _state_base(os.path.expanduser("~")) + if home: + return home / ".local" / "state" / "bmad-loop" + raise StateRootError( + "cannot locate a state directory for bmad-loop's run control plane: " + + ( + "neither %LOCALAPPDATA% nor %USERPROFILE% names an absolute directory" + if sys.platform == "win32" + else "neither $XDG_STATE_HOME nor $HOME names an absolute directory" + ) + + f" — set {envvars.STATE_DIR} to the directory it should live in" + ) + + +def project_state_root(project: Path) -> Path: + """The subtree of :func:`state_root` holding every run of this project: + ``/``. Split out from :func:`state_dir_for` because + the GC reads it as a *directory to enumerate* rather than composing one run's + path — see :func:`reconcile_orphan_state_dirs`, whose whole job is the entries + under here that no longer have a run dir.""" + return state_root() / project_tag(project) + + +def state_dir_for(project: Path, run_id: str) -> Path: + """This run's control-plane directory: ``//``. + + The project key is :func:`project_tag`, reused verbatim rather than re-derived: + it already resolves the project before digesting it, so the two spellings of + one project a caller can arrive with — a symlinked path, a relative one — key + to the same directory. They must, or a run started through one spelling would + write its events where a poll through the other never looks, and the run would + wait out ``session_timeout_min`` with the completion signal sitting on disk. + Its ``resolve()`` raising on a project the OS cannot canonicalize is correct + here for the same reason: an unknowable location cannot be keyed at all, and + guessing one is the wrong-directory write the tag exists to prevent. + + ``run_id`` needs no sanitizing — the id contract (see :data:`RUN_ID_RE`) is + already "a legal path segment on every platform", pinned by + :func:`is_valid_run_id`, and an id from outside is rejected there rather than + coerced here. + """ + return project_state_root(project) / run_id + + +def events_dir_for(project: Path, run_id: str) -> Path: + """The run's hook-event channel: the directory the relay writes a session's + events into and ``SignalWatcher`` polls for them.""" + return state_dir_for(project, run_id) / "events" + + # ---------------------------------------------------- run resolution / liveness @@ -634,6 +814,40 @@ def _refuse_live_session(project: Path, run_id: str, verb: str) -> None: ) +def _discard_state_dir(project: Path, run_id: str) -> None: + """Remove the run's out-of-tree control-plane counterpart, best-effort. + + The events channel (#494) lives outside the project tree, so removing a run + dir no longer removes everything the run owns: without this every + delete/archive would leak ``///`` forever. It + lives here rather than in the CLI so every caller inherits it — `delete`, + `archive`, `clean`, the TUI's removal actions and the engine's own + finish-time reclamation alike. + + A **never-raise tail**, per the teardown doctrine (#139): the run dir is + already gone by the time this runs, and failing the operator's delete over an + unreachable state root would report a removal that in fact happened. Every + catchable outcome is "the counterpart could not even be named" — + :class:`StateRootError` for an environment with no derivable root, and + ``OSError``/``RuntimeError`` for a project path the OS cannot canonicalize + (:func:`project_tag` resolves before digesting). ``RuntimeError`` is not + optional there: below 3.13 ``Path.resolve`` reports a symlink loop that way + rather than as ``OSError`` (measured — 3.11 and 3.12 raise, 3.13 and 3.14 + return the unresolved path), so on two supported interpreters an ``OSError`` + -only guard lets a loop escape and breaks the promise in this paragraph. + Removal failures are absorbed by ``ignore_errors``. Either way the orphan + sweep in :func:`reconcile_orphan_state_dirs` is the backstop. + + Deliberately not called by :func:`trim_run_dir`: a trimmed run is still live + on disk and resumable, and its control plane must outlive the scaffolding. + """ + try: + target = state_dir_for(project, run_id) + except (StateRootError, OSError, RuntimeError): + return + shutil.rmtree(target, ignore_errors=True) + + def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: """Permanently remove a run directory. Callers enforce the engine-liveness guard; the session guard is enforced here (see :func:`_refuse_live_session`), @@ -647,6 +861,9 @@ def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: if not force: _refuse_live_session(project, run_dir.name, "delete") shutil.rmtree(run_dir) + # after the run dir, never before: a raise above leaves the run whole, and a + # whole run keeps its control plane (see _discard_state_dir). + _discard_state_dir(project, run_dir.name) def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: @@ -655,7 +872,13 @@ def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: place so a partial archive never appears. Callers enforce the engine-liveness guard; the session guard is enforced here (see :func:`_refuse_live_session`, and :func:`delete_run` for ``force``) and runs before the tarball is written, - so a refusal leaves nothing behind.""" + so a refusal leaves nothing behind. + + The tarball holds the run dir only, so since #494 an archive no longer carries + the run's ``events/``: the channel moved out of the tree, and its files are + transient completion signals the watcher has already consumed — the recorded + decision accepts losing them from the archive. Everything an archive is read + for later (state, journal, tasks, logs) is in the run dir and unaffected.""" if not force: _refuse_live_session(project, run_dir.name, "archive") archive_dir = project / ARCHIVE_DIR @@ -666,6 +889,7 @@ def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: tar.add(run_dir, arcname=run_dir.name) atomic_replace(tmp, dest) shutil.rmtree(run_dir) + _discard_state_dir(project, run_dir.name) # same tail as delete_run return dest @@ -755,10 +979,111 @@ def reconcile_stale_worktrees(repo: Path, project: Path, *, dry_run: bool = Fals return handled +def _run_dir_names(project: Path) -> set[str] | None: + """Every *directory name* under the runs dir, or ``None`` when that listing + could not be taken. + + Deliberately not :func:`list_run_dirs`, which is ``state.json``-gated: this + answers "does a run dir by this name exist", and a run whose ``state.json`` is + missing or corrupt still owns its control plane. Gating on state.json would + sweep the counterpart out from under exactly the run an operator is trying to + recover. + + The two failures are distinguished because they mean opposite things. A + *missing* runs dir is a real answer — no runs, so nothing is live — while an + unreadable one answers nothing at all, and a sweep run against "no live names" + would remove every state dir this project has. ``None`` is that second case. + """ + try: + return {entry.name for entry in os.scandir(project / RUNS_DIR) if entry.is_dir()} + except FileNotFoundError: + return set() + except OSError: + return None + + +def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list[Path]: + """Remove this project's out-of-tree control-plane dirs whose run dir is gone. + + The GC backstop for the events channel (#494). :func:`_discard_state_dir` + removes the counterpart on every ordinary delete/archive, so this catches what + that path could not: a run dir removed by hand or by an `rm -rf .bmad-loop`, + a delete that ran before this version existed, and any tail that failed + quietly. Without it the state root accumulates one dead subtree per run + forever, on a path outside the project that no operator thinks to look at. + + Shaped like :func:`reconcile_orphan_worktrees`: enumerate on-disk truth, + containment-test each path, remove with failures tolerated. Returns what was + removed (or, under ``dry_run``, what would be). + + Every path is built from an entry name this function itself enumerated — + never from a caller-supplied ref, which is what :func:`_is_path_escape` + refuses on the ref-resolution path. Entries that are not real directories are + skipped, symlinks included: a link is not a state dir we created, and + reporting one swept would be a false count even where ``rmtree`` refuses it. + The containment test then covers what ``is_symlink`` cannot — a Windows + *junction* reads as a plain directory while ``resolve()`` follows it, so + without the test ``rmtree`` would empty a target sitting outside the root. + That case is POSIX-invisible, and the tests say so rather than claim it. + + Degrades to no-op rather than raising, in either direction: an underivable + state root, an unreadable root, or an unreadable runs dir all sweep nothing. + This is reclamation, not repair — leaving disk behind is the cheap outcome, + and removing a live run's control plane is not. + + Both guards hold ``RuntimeError`` alongside ``OSError`` for the same reason + :func:`_discard_state_dir` does: every path here is resolved (the project by + :func:`project_tag`, then the root, then each entry), and below 3.13 + ``Path.resolve`` reports a symlink loop as ``RuntimeError``. A loop planted + among the entries would otherwise escape a sweep whose whole contract is to + degrade, and take the operator's ``clean`` down with it after its real work + was already done. + + **The two reads are ordered, and the order is the whole race guard.** State + entries are enumerated *before* the live run-dir names, because a run creates + its run dir strictly before its state dir — ``compose_run`` builds the + ``Journal`` (which mkdirs the run dir) and only then calls ``make_adapters``, + whose ``SignalWatcher`` mkdirs the events dir. Reading entries first makes + that ordering carry the guarantee: anything in ``entries`` had its state dir + on disk at the first read, so its run dir was on disk *before* that, so the + later ``live`` read is certain to contain it. Read the other way round, a run + starting in the gap is missing from ``live`` and present in ``entries``, and + an operator's ``clean`` deletes the control plane of a run that is starting + right now — whose watcher then polls a primary that no longer exists, or + simply never sees the Stop. A run dir that disappears *between* the reads is + the opposite case and correctly swept: it is a real orphan by then. + """ + try: + root = project_state_root(project) + entries = sorted(root.iterdir()) + root_res = root.resolve() + except (StateRootError, OSError, RuntimeError): + return [] + live = _run_dir_names(project) + if live is None: + return [] + handled: list[Path] = [] + for entry in entries: + if entry.name in live or entry.is_symlink() or not entry.is_dir(): + continue + try: + entry.resolve().relative_to(root_res) + except (OSError, RuntimeError, ValueError): + continue + handled.append(entry) + if not dry_run: + shutil.rmtree(entry, ignore_errors=True) + return handled + + def trim_run_dir(run_dir: Path, *, dry_run: bool = False) -> list[Path]: """Delete heavy scaffolding (the ``worktrees/`` tree) from a concluded run dir, preserving its TUI-visible core so the run still appears in the - dashboard with full status/journal/logs. Returns the paths removed.""" + dashboard with full status/journal/logs. Returns the paths removed. + + The run's out-of-tree control plane is deliberately left alone (see + :func:`_discard_state_dir`): a trimmed run still exists and is still + resumable, so its state dir has to outlive its scaffolding.""" removed: list[Path] = [] for name in _HEAVY_RUN_ENTRIES: p = run_dir / name diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 603edc4e..4c46accf 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -412,7 +412,10 @@ def make_adapters( """Build the per-role adapters. ``profiles`` is an already-resolved mapping from :func:`resolve_profiles`; when given, no profile is re-read from disk, so a caller that gated on :func:`config_digest` launches the *same* bytes it - validated (#461 point 4). Omitted, each role resolves fresh as before.""" + validated (#461 point 4). Omitted, each role resolves fresh as before. + + Also the single resolution point for this run's out-of-tree events directory + (#494), handed to every family it builds — see the ``events_dir`` note below.""" from .adapters.multiplexer import fold_version, get_multiplexer, mux_usable from .adapters.profile import ProfileError, get_profile from .adapters.registry import AdapterError, get_adapter_kind @@ -483,6 +486,15 @@ def make_adapters( extra_args=cfg.extra_args, usage_grace_s=cfg.usage_grace_s, stop_without_result_nudges=cfg.stop_without_result_nudges, + # The run's out-of-tree hook-event channel (#494). Resolved HERE, + # from the `project` this function is handed, because it is the + # only layer that holds both halves of the key — the adapter sees + # a run dir and nothing else. Handed to every family rather than + # gated like `mux`: this is a description of the run, not a + # capability, and unlike resolving a multiplexer it costs no probe + # and can refuse no host. The engine derives the same value from + # the same two inputs for the producing side. + events_dir=runs.events_dir_for(project, run_dir.name), ) if kind.needs_mux: # Resolve and probe the shared multiplexer only when a kind diff --git a/src/bmad_loop/signals.py b/src/bmad_loop/signals.py index b9cad325..2a547630 100644 --- a/src/bmad_loop/signals.py +++ b/src/bmad_loop/signals.py @@ -1,8 +1,19 @@ """Watch the per-run events directory for hook-written event files. -The hook script (hooks/bmad_loop_hook.py) writes one JSON file per event, -atomically (tmp + rename), named "--.json". Plain -polling of a near-empty directory is cheap and crash-safe; no inotify. +The hook script (data/bmad_loop_hook.py) and its importable twin (:mod:`events`, +behind ``bmad-loop relay``) write one JSON file per event, atomically (tmp + +rename), named "--.json". Plain polling of a near-empty +directory is cheap and crash-safe; no inotify. + +Two directories, not one (#494). The channel now lives out of the project tree, +under the user-scoped state root (``runs.events_dir_for``), because a branch +switch, a worktree mount or a rollback must not be able to take a live run's +control plane away. The *legacy* in-tree ``/events`` stays polled as +well, and that is the whole version-skew guard: the relay a target project has +installed is a COPY taken at init time, so an upgraded orchestrator routinely +drives sessions whose hook only knows the old location. Without the second poll +that pairing loses every Stop event and every session stalls to +``session_timeout_min`` — the loudest possible regression, delivered silently. """ from __future__ import annotations @@ -25,35 +36,84 @@ class HookEvent: class SignalWatcher: - def __init__(self, events_dir: Path): + """Poll one or two event directories for a run's hook events. + + ``events_dir`` is the primary — the out-of-tree channel this orchestrator + directs its sessions to via ``BMAD_LOOP_EVENTS_DIR``, and the only one + created here. ``legacy_dir`` is the pre-#494 in-tree ``/events``, + polled when given so a project carrying an older installed relay still + completes its sessions (see the module docstring). It is deliberately NOT + created: an orchestrator that recreated the in-tree directory would undo the + move for the operator's `git status` while gaining nothing — a legacy relay + that writes there makes the directory itself. + + The single-positional-argument form is unchanged, and stays the shape the + probe (``probe.py``, watching its own capture dir) and the unit tests use. + """ + + def __init__(self, events_dir: Path, legacy_dir: Path | None = None): self.events_dir = events_dir - self._consumed: set[str] = set() + self.legacy_dir = legacy_dir + # Keyed by (directory, filename), not by filename: one name identifies an + # event only WITHIN a directory, and the two dirs are written by two + # independent relays. Keying on the name alone would let a file consumed + # from one dir mask a different event of the same name in the other, and a + # masked event here is a lost Stop — the run then waits out + # session_timeout_min with its completion signal sitting on disk. + self._consumed: set[tuple[str, str]] = set() self._pending: list[HookEvent] = [] # polled but not yet delivered via wait_for events_dir.mkdir(parents=True, exist_ok=True) + def _dirs(self) -> list[Path]: + """Primary first, then the legacy dir when there is a distinct one. The + equality check keeps a caller that passes the same path twice from + double-scanning; it cannot produce duplicate events either way (the + ``_consumed`` key would repeat), but the second scan would be pure waste.""" + if self.legacy_dir is None or self.legacy_dir == self.events_dir: + return [self.events_dir] + return [self.events_dir, self.legacy_dir] + def poll(self) -> list[HookEvent]: - """Return new, well-formed events since the last poll, oldest first.""" + """Return new, well-formed events since the last poll, oldest first. + + Ordering is by the parsed ``ts`` across BOTH directories, so which relay + wrote an event never affects where it lands in the sequence. + + A missing *legacy* directory is the normal case (nothing installed writes + there any more) and is skipped silently. A missing *primary* still raises, + as it always has: this watcher created it, so its absence means something + removed the live control plane out from under the run. + """ events: list[HookEvent] = [] - for entry in self.events_dir.iterdir(): - if entry.name in self._consumed or entry.suffix != ".json": - continue - self._consumed.add(entry.name) + dirs = self._dirs() + for directory in dirs: try: - data = json.loads(entry.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError): - continue - if not isinstance(data, dict) or "event" not in data or "task_id" not in data: - continue - events.append( - HookEvent( - ts=int(data.get("ts", 0)), - event=str(data["event"]), - task_id=str(data["task_id"]), - session_id=data.get("session_id"), - transcript_path=data.get("transcript_path"), - path=entry, + entries = list(directory.iterdir()) + except OSError: + if directory == self.events_dir: + raise + continue # legacy dir absent — the ordinary case + for entry in entries: + key = (str(directory), entry.name) + if key in self._consumed or entry.suffix != ".json": + continue + self._consumed.add(key) + try: + data = json.loads(entry.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + continue + if not isinstance(data, dict) or "event" not in data or "task_id" not in data: + continue + events.append( + HookEvent( + ts=int(data.get("ts", 0)), + event=str(data["event"]), + task_id=str(data["task_id"]), + session_id=data.get("session_id"), + transcript_path=data.get("transcript_path"), + path=entry, + ) ) - ) events.sort(key=lambda e: e.ts) return events diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index fbeabb4d..1ec985a6 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -483,8 +483,12 @@ def provision_worktree( project that commits its own skill tree (e.g. .agents/) or config keeps it untouched (no diff merged back); - the hook points at the MAIN repo's already-installed relay via an absolute - path (the relay locates the run dir from $BMAD_LOOP_RUN_DIR, not its own - location), so nothing is written into the worktree's .bmad-loop/; + path (the relay locates its events directory from $BMAD_LOOP_EVENTS_DIR, + falling back to $BMAD_LOOP_RUN_DIR/events — never from its own location), + so nothing is written into the worktree's .bmad-loop/. Since #494 the + primary channel is out of the project tree entirely, so a worktree cannot + carry a run's control plane at all — but the fallback still resolves under + the MAIN run dir, so the guarantee holds for an older installed relay too; - everything we wrote is excluded from git, in a file private to THIS worktree (`.git/worktrees//info/exclude`, activated per-worktree) that dies with it when the worktree is removed. It is never the repository-wide diff --git a/tests/conftest.py b/tests/conftest.py index 429285e7..a0f29e2d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,7 +13,7 @@ import pytest import yaml -from bmad_loop import cli, documents, platform_util +from bmad_loop import cli, documents, envvars, platform_util from bmad_loop.adapters.base import SessionResult, SessionSpec from bmad_loop.bmadconfig import ProjectPaths from bmad_loop.checks import ValidationReport @@ -283,6 +283,33 @@ def _isolate_ambient_git_ignores(tmp_path_factory: pytest.TempPathFactory): mp.undo() +@pytest.fixture(autouse=True) +def _isolate_state_root(tmp_path_factory: pytest.TempPathFactory, monkeypatch): + """Point the user-scoped state root at a per-test temp dir, for every test. + + `runs.state_root()` resolves to `~/.local/state/bmad-loop` (POSIX) or + `%LOCALAPPDATA%\\bmad-loop\\state` (win32) when nothing overrides it, and the + run control plane does not merely *read* that location — it mkdirs into it. + Without this every test that constructs an adapter would write into the + developer's (or the CI runner's) real state directory and leave it there, one + stray tree per run id, on a path no fixture cleans up. + + One variable is enough: `BMAD_LOOP_STATE_DIR` is checked before the platform + cascade and outranks all of it, so this cannot be defeated by whatever + XDG/LOCALAPPDATA the host happens to export. + + Deliberately NOT a blanket reset of HOME / USERPROFILE / LOCALAPPDATA / + XDG_STATE_HOME. Those are not ours: git reads HOME (`install`'s shield + probes it), the coding CLIs discover their own config under them, and + `sanitize.redact_home` measures against the real one — shadowing them + suite-wide would change what unrelated tests measure, which is the argument + `_isolate_ambient_git_ignores` makes for shadowing only the two it must. + Tests that grade the cascade itself `delenv` this variable and monkeypatch + the ones they need, and share this fixture's monkeypatch instance, so the + override comes off cleanly for exactly that test.""" + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path_factory.mktemp("state-root"))) + + @pytest.fixture(scope="session") def _project_template( tmp_path_factory: pytest.TempPathFactory, _isolate_ambient_git_ignores: None diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index 596cd69d..c50505e4 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -693,6 +693,70 @@ def test_make_adapters_unknown_kind_systemexit_names_profile(fresh_adapter_regis runsetup.make_adapters(project.project, _run_dir(project.project), pol) +def test_make_adapters_hands_every_kind_the_run_events_dir( + fresh_adapter_registry, project, monkeypatch +): + """#494: the events channel lives outside the project tree now, so an adapter + can no longer derive it — the only path it holds is a run dir, and the key + needs the PROJECT too. `make_adapters` is the one layer with both, so it + resolves the directory and hands it down. + + To EVERY kind, unlike `mux`: a path costs nothing to carry, probes no host and + can refuse no run, so gating it would make the bootstrap branch per family over + a value it can always compute. Both variants are checked because they take + separate `__init__` paths (the dev one threads `paths` as well).""" + from bmad_loop import runs + + registry = fresh_adapter_registry + registry.register_adapter("noxport", needs_mux=False, load=lambda: _stub_builder()) + monkeypatch.setattr(mux_mod, "get_multiplexer", lambda: pytest.fail("no mux for this kind")) + install_bmad_config(project) + _write_profile(project.project, "noxport", adapter="noxport") + _write_policy(project.project, '[adapter]\nname = "noxport"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + run_dir = _run_dir(project.project) + adapters = runsetup.make_adapters(project.project, run_dir, pol) + + expected = runs.events_dir_for(project.project, run_dir.name) + assert adapters["dev"].kwargs["events_dir"] == expected # dev variant + assert adapters["triage"].kwargs["events_dir"] == expected # plain variant + # and it is genuinely out of the project tree — the whole point of the move + assert not expected.is_relative_to(project.project) + + +def test_the_generic_watcher_polls_the_state_root_first_and_the_legacy_dir_too( + fresh_adapter_registry, project, monkeypatch +): + """The consumer half, on the real builtin family: the watcher's PRIMARY is the + out-of-tree channel this run's sessions are pointed at, and the pre-#494 in-tree + dir stays under poll so a project whose installed relay predates the move still + completes its sessions. + + This is also the producer/consumer agreement. Both sides derive the primary + from `runs.events_dir_for(project, run_id)` — this one and + `test_session_env_names_the_out_of_tree_events_dir` in test_engine.py, which + asserts the engine exports that same call's result. A run whose two halves + disagreed would poll a directory nothing writes to and stall every session to + `session_timeout_min` with the Stop sitting on disk. + + Ablation guard: drop `events_dir` from `make_adapters`' kwargs and the primary + falls back to the in-tree dir — the first assertion fails.""" + from bmad_loop import runs + + monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) + install_bmad_config(project) + run_dir = _run_dir(project.project) + + adapters = runsetup.make_adapters(project.project, run_dir, policy_mod.load(None)) + + watcher = adapters["dev"].watcher + assert watcher.events_dir == runs.events_dir_for(project.project, run_dir.name) + assert watcher.legacy_dir == run_dir / "events" + assert watcher.events_dir.is_dir() # created for the relay to write into + assert not (run_dir / "events").exists() # never re-created in the project tree + + def test_make_adapters_generic_shares_synthesizing_but_not_triage( fresh_adapter_registry, project, monkeypatch ): diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py index 6895238d..269a228d 100644 --- a/tests/test_cleanup.py +++ b/tests/test_cleanup.py @@ -526,5 +526,101 @@ def test_cmd_clean_json_nothing_to_reclaim_is_a_valid_empty_document(project, ca assert doc["schema_version"] == cli.CLEAN_SCHEMA_VERSION assert doc["freed_bytes"] == 0 + assert doc["state_dirs_swept"] == 0 for key in ("worktrees", "trimmed", "archived", "deleted", "protected", "unverifiable_pid"): assert doc[key] == [], key + + +# ------------------------------------------- cmd_clean: out-of-tree state (#494) + + +def _seed_state_dir(project, run_id): + events = runs.events_dir_for(project, run_id) + events.mkdir(parents=True) + (events / "1700000000-t1-Stop.json").write_text("{}") + return runs.state_dir_for(project, run_id) + + +def test_cmd_clean_removes_the_state_counterpart_of_a_deleted_run(project): + """The whole reason `clean` needed changing: past the retention window it + removes the run dir, and since #494 the run's control plane is no longer + inside it. Asserted through the command rather than `delete_run` alone, since + `clean` is the path an operator schedules and the only one that runs + unattended.""" + install_bmad_config(project) + repo = project.project + run_dir = repo / ".bmad-loop" / "runs" / "20260101-000000-aaaa" + save_state(run_dir, RunState(run_id="r", project=str(repo), started_at="x", finished=True)) + state_dir = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo, retain=0, hard=True)) == 0 + + assert not run_dir.exists() + assert not state_dir.exists() + + +def test_cmd_clean_keeps_the_state_counterpart_of_a_run_it_only_trimmed(project): + """A trimmed run is still on disk and still resumable, so its control plane + has to survive its scaffolding. Two paths could take it: `trim_run_dir` (which + deliberately does not) and the orphan sweep (whose live-name check is what + keeps it). This grades them together, at the level where a mistake in either + would strand a resumable run's completion channel.""" + install_bmad_config(project) + repo = project.project + run_dir = repo / ".bmad-loop" / "runs" / "20260101-000000-aaaa" + (run_dir / "worktrees" / "u").mkdir(parents=True) + save_state(run_dir, RunState(run_id="r", project=str(repo), started_at="x", stopped=True)) + state_dir = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo)) == 0 + + assert run_dir.is_dir() and not (run_dir / "worktrees").exists() # trimmed, not removed + assert state_dir.is_dir() + + +def test_cmd_clean_sweeps_an_orphaned_state_dir_and_reports_it(project, capsys): + """The backstop reaching a leak `clean` did not create: a run dir removed by + hand leaves a control plane nothing else will ever collect. The text mode says + so on its own line — the reclaim summary above it is a byte estimate these + dirs are deliberately outside of.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo)) == 0 + + assert not orphan.exists() + out = capsys.readouterr().out + assert "swept 1 orphaned run state dir(s)" in out + assert "nothing to reclaim" not in out # something WAS reclaimed + + +def test_cmd_clean_dry_run_plans_the_sweep_without_removing(project, capsys): + """Plan and outcome share one shape: the preview names the same work the real + run would do, and provably leaves the disk alone.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo, dry_run=True)) == 0 + + assert orphan.is_dir() + assert "would sweep 1 orphaned run state dir(s)" in capsys.readouterr().out + + +def test_cmd_clean_json_carries_the_sweep_count_in_both_modes(project, capsys): + """The additive field, on the existing schema version (nothing a v1 consumer + already read changed shape). Populated under `--dry-run` too, or a caller + pre-flighting a reclaim would see the sweep appear only after committing.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + plan = _clean_json(repo, capsys, "--dry-run") + assert plan["schema_version"] == cli.CLEAN_SCHEMA_VERSION + assert plan["state_dirs_swept"] == 1 + assert orphan.is_dir() + + outcome = _clean_json(repo, capsys) + assert outcome["state_dirs_swept"] == 1 + assert not orphan.exists() diff --git a/tests/test_cli.py b/tests/test_cli.py index 719edca5..f46f86ed 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4834,6 +4834,120 @@ def test_validate_flags_registered_hooks_with_an_unreadable_relay_script(project relay.chmod(0o644) # so the sandbox tears down cleanly +def _validate_json(project_dir, capsys): + """`validate --json` as (rc, document). Not `machine_json`: that helper takes + the expected rc as an input, and these tests are ABOUT the rc — the sandbox's + baseline verdict comes from findings that have nothing to do with the relay, + so it must be observed, never asserted.""" + rc = cli.main(["validate", "--project", str(project_dir), "--json"]) + out, err = capsys.readouterr() + assert err == "" # the machine.py purity contract still holds + return rc, json.loads(out) + + +def test_validate_reports_a_fresh_relay_as_up_to_date(project, capsys): + """#494 Phase 4: a relay `init` just wrote matches the packaged source. + + Pinned separately from the stale case because the two arms fail for opposite + reasons — a comparison wrong in the *other* direction (a raw byte compare + against a `write_text`-translated file, which on Windows calls every fresh + install stale) reddens here and nowhere else.""" + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + args = argparse.Namespace(project=str(project.project), spec=None) + + cli.cmd_validate(args) + text = _validate_output(capsys) + assert "hook relay script up to date" in text + assert "differs from this bmad-loop" not in text + + +def test_validate_warns_when_the_installed_relay_is_stale(project, capsys): + """#494 Phase 4: the relay is COPIED into the project by `init`, so an + upgraded orchestrator drives sessions through whatever version the project + happens to hold — and a pre-#494 relay still writes its events INSIDE the + project tree. `hooks.relay-present` cannot see that: the file is there and + readable, so it reads green. + + The verdict must not move — the dual-poll fallback keeps a stale relay + working, so this reports a lost property, not a broken run. Asserted as + "unchanged from the same project one edit earlier" rather than a literal 0: + the sandbox's baseline comes from unrelated findings, and hardcoding it would + pin this test to those instead of to the relay. + + Ablation guard: deleting the `hook_script_current` block in cmd_validate (or + forcing it to `True`) makes this FAIL on the `hooks.relay-stale` lookup.""" + from bmad_loop import install as install_mod + + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + capsys.readouterr() # drop init's chatter; _validate_json parses the WHOLE stream + + baseline_rc, baseline = _validate_json(project.project, capsys) + + relay = project.project / install_mod.HOOK_SCRIPT_REL + relay.write_text( + relay.read_text(encoding="utf-8") + "\n# an older wheel wrote this\n", encoding="utf-8" + ) + rc, doc = _validate_json(project.project, capsys) + + # The id is the matchable identity (checks.py) — match on it, not the prose. + stale = [f for f in doc["findings"] if f["check"] == "hooks.relay-stale"] + assert len(stale) == 1 + assert stale[0]["severity"] == "warning" + assert stale[0]["detail"]["path"] == str(relay) + assert "bmad-loop init" in stale[0]["message"] + + # The verdict and exit code are untouched by a warning. + assert (rc, doc["ok"]) == (baseline_rc, baseline["ok"]) + # And the two checks it must not be confused with still read exactly as before. + by_id = {f["check"]: f["severity"] for f in doc["findings"]} + assert by_id["hooks.registered"] == "ok" + assert by_id["hooks.relay-present"] == "ok" + + +def test_validate_is_silent_about_relay_staleness_when_hooks_are_unregistered(project, capsys): + """The check hangs off `any_hooks_registered`, like `hooks.relay-present`: a + project not routing events through any relay is not owed a note about which + version of one it holds — it is owed the registration FAIL, and nothing on + top of it. + + The relay is left INSTALLED and STALE on purpose. A test that simply skips + `init` proves nothing: with no relay on disk the comparison degrades to + "unknowable" and emits no finding whether it is gated or not, so the gate + could be deleted outright and such a test would stay green (it was, and it + did). This shape is the one that separates them. + + Ablation guard: hoisting the `hooks.relay-stale` block out of the + `if any_hooks_registered:` block makes this FAIL.""" + from bmad_loop import install as install_mod + from bmad_loop.adapters.profile import load_profiles + + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + + # Unregister: keep the relay, drop every registration that points at it. + for profile in load_profiles(project.project).values(): + config = project.project / profile.hooks.config_path + if config.is_file(): + config.write_text("{}\n", encoding="utf-8") + relay = project.project / install_mod.HOOK_SCRIPT_REL + relay.write_text(relay.read_text(encoding="utf-8") + "\n# stale\n", encoding="utf-8") + capsys.readouterr() + + _rc, doc = _validate_json(project.project, capsys) + # The premise, asserted rather than assumed — a still-registered project would + # make the absence below prove nothing. + assert any( + f["check"] == "hooks.registered" and f["severity"] == "problem" for f in doc["findings"] + ) + assert relay.is_file() # and the stale artifact really is still there + assert not [f for f in doc["findings"] if f["check"] == "hooks.relay-stale"] + + def test_validate_sprint_mode_still_gates_on_sprint_status(project, capsys): """Item 8 regression: the default (sprint) mode still requires sprint-status.""" install_bmad_config(project) diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index a86c8e56..3b2817ac 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -657,6 +657,158 @@ def test_non_ascii_survives_the_utf8_round_trip(tmp_path, monkeypatch): assert json.loads(path.read_text(encoding="utf-8"))["note"] == "café — naïve ✓" +# --------------------------------------------- the out-of-tree events channel + + +def _seed_bare_run(project_dir, run_id="20260812-090000-bbbb"): + """A run dir with just enough state to collect: this test family is about + WHERE `summarize_files` looks, not about the canary sweep.""" + run_dir = project_dir / ".bmad-loop" / "runs" / run_id + save_state( + run_dir, + RunState( + run_id=run_id, + project=str(project_dir), + started_at="2026-08-12T09:00:00", + run_type="story", + ), + ) + return run_dir + + +def _events_group(run_dir, project_dir): + diag = diagnostics.collect( + [run_dir], pseudo=sanitize.Pseudonymizer(), project=Path(project_dir) + ) + return next((g for g in diag.runs[0].files if g.category == "events"), None) + + +def _write_events(events_dir, n, *, prefix=""): + events_dir.mkdir(parents=True, exist_ok=True) + for i in range(n): + (events_dir / f"{prefix}17550000{i}-t-Stop.json").write_text("{}", encoding="utf-8") + + +def test_events_are_counted_at_the_out_of_tree_state_root(project, tmp_path, monkeypatch): + """#494: the events channel left the project tree for the user state root, and + `_FILE_CATEGORIES` listed "events" as a run-dir-relative name. That made the + group VANISH silently rather than redden — the `is_dir()` guard turns a + category with no directory into a no-op, so the dump simply stopped mentioning + events at all, on exactly the runs a maintainer reads a dump to understand. + + The count is derived through the real `collect` path, not by handing + `summarize_files` the directory: what has to hold is that a collector given + only a run dir finds the channel, and the derivation (from the run's OWN + recorded project and id) is the part that can break. + + Ablation guard: making `_events_dir` return None, or dropping the primary from + `_category_roots`, makes this FAIL on the count.""" + from bmad_loop import envvars, runs + + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "state")) + run_dir = _seed_bare_run(project.project) + _write_events(runs.events_dir_for(project.project, run_dir.name), 3) + + group = _events_group(run_dir, project.project) + assert group is not None and group.count == 3 + assert group.total_bytes > 0 + # The in-tree location is genuinely empty — the count above came from the + # state root, not from a legacy directory a fixture happened to create. + assert not (run_dir / "events").exists() + + +def test_legacy_in_tree_events_still_counted_and_summed_with_the_primary( + project, tmp_path, monkeypatch +): + """Both roots are live at once. The hook relay is COPIED into the project by + `init`, so an upgraded orchestrator routinely drives sessions whose relay + predates the move and still writes in-tree; `SignalWatcher` dual-polls for + exactly that reason. A dump that counted only the primary would report zero + for such a session, which is the same blind spot in a different place. + + Ablation guard: dropping the legacy root from `_category_roots` makes this + FAIL (2 instead of 5).""" + from bmad_loop import envvars, runs + + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "state")) + run_dir = _seed_bare_run(project.project) + _write_events(runs.events_dir_for(project.project, run_dir.name), 3, prefix="new-") + _write_events(run_dir / "events", 2, prefix="old-") + + group = _events_group(run_dir, project.project) + assert group is not None + # ONE group, summed — the payload shape is the v1 schema and does not split. + assert group.count == 5 + + +def test_events_degrade_to_the_legacy_root_when_the_state_root_is_underivable( + project, tmp_path, monkeypatch +): + """A dump is routinely read on a machine that did not produce the run, and the + state root is a HOST fact. `runs.state_root()` raises rather than guessing when + no candidate answers (it is a write path); observation must not inherit that — + the events count is worth losing, the dump is not. + + Ablation guard: narrowing `_events_dir`'s except clause so the raise escapes + makes this FAIL — and the way it fails is the point. `collect` catches per + run, so the escape does not crash the dump; it demotes the WHOLE run to + `_unreadable_run`, losing its tasks, journal and every other file group to a + host fact that has nothing to do with the run. Silent, and far worse than the + count this degradation gives up.""" + from bmad_loop import envvars, runs + + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.setattr(runs, "state_root", _raise_no_state_root) + run_dir = _seed_bare_run(project.project) + _write_events(run_dir / "events", 2) + + group = _events_group(run_dir, project.project) + assert group is not None and group.count == 2 + + +def _raise_no_state_root(): + from bmad_loop.runs import StateRootError + + raise StateRootError("no state root on this host") + + +def test_state_root_path_is_redacted_in_the_dump(project, tmp_path, monkeypatch): + """#494 put a control-plane directory under the user's home, and every home is + an egress hazard: `/home//.local/state/bmad-loop/...` and + `C:\\Users\\\\AppData\\Local\\bmad-loop\\state\\...` both name a real + person. The dump only ever *stats* that directory, so no field carries it + today — this pins the two backstops that must hold if one ever does. + + Both spellings are asserted on ONE host on purpose: `_ABS_HOME_RE` is + platform-independent by construction (a Windows-shaped path is diagnosed on + POSIX whenever a Windows run's dump is read on Linux), and a rule that only + fired on its native platform would pass CI on the runner that never sees it. + Neither uses the host's own home, which `redact_home` would rewrite to `~` + before the rule was ever consulted — that would prove the wrong mechanism.""" + monkeypatch.setenv("BMAD_LOOP_STATE_DIR", str(tmp_path / "state")) + posix_root = "/home/canaryoperator/.local/state/bmad-loop/9f1c2d3e4a5b6c7d/r/events" + win_root = r"C:\Users\canaryoperator\AppData\Local\bmad-loop\state\9f1c2d3e4a5b6c7d\r\events" + + # The realistic vector: an unknown journal field. Unknown fields fall to + # `scrub_json`, so this half proves per-field routing catches the path. + for i, planted in enumerate((posix_root, win_root)): + run_dir = _seed_bare_run(project.project, run_id=f"20260812-09000{i}-cccc") + Journal(run_dir).append("events-routed", events_dir=planted) + _diag, _pseudo, combined = _render_all([run_dir]) + assert planted not in combined + assert "canaryoperator" not in combined + + # And this half proves the egress guard would have refused the path anyway — + # which is what actually covers a field nobody has written yet. It runs LAST + # and in its own loop: `_render_json_over` leaves `_to_jsonable` monkeypatched + # for the rest of the test, so a later `_render_all` would re-render this + # payload instead of its own run. + for planted in (posix_root, win_root): + with pytest.raises(diagnostics.LeakDetected) as exc: + _render_json_over(monkeypatch, {"events_dir": planted}) + assert "absolute-home-path" in exc.value.rules + + # The pure guard-mechanics tests (hard-rule refusal, repair tally, cyclic # termination) live in tests/test_sanitize.py since #199 made guard shared API; # this file keeps the integration surface: real collectors, real renders. diff --git a/tests/test_engine.py b/tests/test_engine.py index 0c7876e3..94519ec8 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2012,6 +2012,38 @@ def test_happy_path(project): assert adapter.sessions[1].prompt.startswith("/bmad-dev-auto ") +def test_session_env_names_the_out_of_tree_events_dir(project): + """#494 producer side: every engine-driven session is told where to write its + hook events, and the answer is the out-of-tree channel — not `/events`, + which a branch switch, a worktree mount or a rollback can take away mid-run. + + `Engine._run_session`'s env dict is the ONE required producer site: dev, + review, sweep bundles, stories and injected plugin-workflow sessions are all + dispatched through it, so both roles below carry the variable from one edit. + + The value is `runs.events_dir_for(project, run_id)` — the same call + `runsetup.make_adapters` points this run's SignalWatcher at (see + `test_the_generic_watcher_polls_the_state_root_first_and_the_legacy_dir_too`). + That agreement is the invariant: a producer and consumer that disagreed would + leave every Stop unobserved and stall the run to `session_timeout_min`. + + Ablation guard: delete the env entry and this fails; point it at + `self.run_dir / "events"` and it fails on the value.""" + from bmad_loop import runs + + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + engine.run() + + expected = str(runs.events_dir_for(project.project, engine.run_dir.name)) + assert [s.role for s in adapter.sessions] == ["dev", "review"] + assert {s.env["BMAD_LOOP_EVENTS_DIR"] for s in adapter.sessions} == {expected} + assert not Path(expected).is_relative_to(project.project) + + def test_post_kill_rescued_result_flows_and_journals(project): """A result rescued by the adapter's post-kill reconcile (#61) reaches the engine as an ordinary completed result — it must flow the completed path diff --git a/tests/test_envvars.py b/tests/test_envvars.py index 8907e05b..93caa81f 100644 --- a/tests/test_envvars.py +++ b/tests/test_envvars.py @@ -1,12 +1,15 @@ -"""Registry tests for the three core `BMAD_LOOP_*` runtime overrides. +"""Registry tests for the core `BMAD_LOOP_*` runtime overrides. `envvars` is the one place each core var is named, typed, and given a reader, so what is pinned here is the *contract* the call sites (`engine`, -`adapters.multiplexer`, `cli`, `process_host`) and the README's "Environment -variables" table both depend on: the literal names, and each reader's parse and -fallback. The two name readers pass their value through verbatim on purpose — -validation lives downstream in the registry that resolves the name — so a test -asserting rejection here would be asserting the wrong module's job. +`adapters.multiplexer`, `cli`, `process_host`, `runs`) and the README's +"Environment variables" table both depend on: the literal names, and each +reader's parse and fallback. The two name readers pass their value through +verbatim on purpose — validation lives downstream in the registry that resolves +the name — so a test asserting rejection here would be asserting the wrong +module's job. `state_dir` is verbatim for a related reason (a stated override +must not be silently swapped for a guess) with one exception, the empty string, +which is graded here because `runs.state_root` trusts what this reader returns. Contract parity: `test_engine.py::test_session_timeout_s_env_override` pins the same rejection set one layer up, through `Engine._session_timeout_s` (does the @@ -33,6 +36,7 @@ def test_constants_are_the_literal_env_var_names(): assert envvars.SESSION_TIMEOUT_S == "BMAD_LOOP_SESSION_TIMEOUT_S" assert envvars.MUX_BACKEND == "BMAD_LOOP_MUX_BACKEND" assert envvars.PROCESS_HOST == "BMAD_LOOP_PROCESS_HOST" + assert envvars.STATE_DIR == "BMAD_LOOP_STATE_DIR" def test_session_timeout_s_is_none_when_unset(monkeypatch): @@ -154,3 +158,33 @@ def test_process_host_is_a_verbatim_passthrough(monkeypatch): monkeypatch.setenv(envvars.PROCESS_HOST, "bogus") assert envvars.process_host() == "bogus" + + +def test_state_dir_is_a_verbatim_passthrough_except_for_the_empty_string(monkeypatch): + """`runs.state_root` uses this value as the state root itself, so the reader + hands back what the operator wrote — a relative spelling included. Filtering it + would put the loop in the position `mux_backend` refuses: an override the + operator can see they exported, silently swapped for the platform guess, and + detectable only by noticing where a run's events did *not* appear. + + The empty string is the exception, and it is a value this reader must hold + rather than the caller: `export BMAD_LOOP_STATE_DIR=` is what an unset-looking + export leaves behind, `Path("")` is the *current directory*, and rooting the + control plane at the launch cwd is neither what was meant nor something a + later process would resolve the same way. Reading it as unset falls through to + the platform cascade, which is the same thing not exporting it at all does. + + Ablation target: spell the reader `os.environ.get(STATE_DIR)` and only the + empty row fails — the unset and value rows pass under it, so neither pins the + rule on its own.""" + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + assert envvars.state_dir() is None + + monkeypatch.setenv(envvars.STATE_DIR, "") + assert envvars.state_dir() is None + + monkeypatch.setenv(envvars.STATE_DIR, "/var/lib/bmad-loop") + assert envvars.state_dir() == "/var/lib/bmad-loop" + + monkeypatch.setenv(envvars.STATE_DIR, "relative/state") + assert envvars.state_dir() == "relative/state" diff --git a/tests/test_events.py b/tests/test_events.py new file mode 100644 index 00000000..9656ac2c --- /dev/null +++ b/tests/test_events.py @@ -0,0 +1,671 @@ +"""`events.py` — the importable twin of the stdlib-only hook relay, and the +`bmad-loop relay` command built on it. + +Two things are under test. The twin's *source parity* with +`data/bmad_loop_hook.py`, because two separately-maintained writers of one +control plane is how the hardening in one silently stops applying to the other. +And the twin's *behavior*, mirrored from the #493 hardening tests in +test_hook_script.py — a copy that is byte-identical today can still be edited on +both sides at once, and these are what say the shape still holds. +""" + +from __future__ import annotations + +import ast +import io +import json +import os +import stat +import subprocess +import sys +from pathlib import Path + +import pytest + +from bmad_loop import cli, events +from bmad_loop import policy as policy_mod + +HOOK = Path(events.__file__).resolve().parent / "data" / "bmad_loop_hook.py" +EVENTS = Path(events.__file__).resolve() + +# Everything `events.py` copies verbatim out of the hook script. The write path and +# the two helpers it stands on; the payload SHAPING is not here because the hook +# does it inline in its `main()` and there is no source segment to compare — it is +# pinned behaviorally instead, by test_relay_and_the_hook_shape_the_same_event. +TWINNED = ( + "_LINK_REPARSE_TAGS", + "_first_workspace", + "_is_link_like", + "_write_all", + "_write_event", +) + + +def _top_level_sources(path: Path) -> dict[str, str]: + """`name -> the exact source segment that defines it`, for the module's + top-level defs and plain assignments.""" + src = path.read_text(encoding="utf-8") + found: dict[str, str] = {} + for node in ast.parse(src).body: + if isinstance(node, ast.FunctionDef): + found[node.name] = ast.get_source_segment(src, node) or "" + elif isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name): + found[target.id] = ast.get_source_segment(src, node) or "" + return found + + +def _relay( + event: str, + payload, + monkeypatch, + run_dir: Path | None, + task_id: str = "t1", + events_dir: Path | str | None = None, +) -> int: + """Drive `bmad-loop relay ` in-process with `payload` on stdin. + + ``events_dir`` sets (or, as None, explicitly CLEARS) BMAD_LOOP_EVENTS_DIR: + cleared by default so an operator shell that happens to export it cannot + redirect a test's events out from under its assertions.""" + if run_dir is None: + monkeypatch.delenv("BMAD_LOOP_RUN_DIR", raising=False) + monkeypatch.delenv("BMAD_LOOP_TASK_ID", raising=False) + else: + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", task_id) + if events_dir is None: + monkeypatch.delenv("BMAD_LOOP_EVENTS_DIR", raising=False) + else: + monkeypatch.setenv("BMAD_LOOP_EVENTS_DIR", str(events_dir)) + text = payload if isinstance(payload, str) else json.dumps(payload) + monkeypatch.setattr(sys, "stdin", io.StringIO(text)) + return cli.main(["relay", event]) + + +# ------------------------------------------------------------------- parity + + +def test_the_twinned_source_is_identical(): + """The hook ships as package DATA and is stdlib-only by contract, so it cannot + import `events.py` and `events.py` cannot import it: the hardened write exists + twice on purpose. A fix applied to one copy and not the other leaves one writer + of the events control plane unhardened while every behavioral test stays green + — both copies pass their own suites either way. Compare the source directly. + + Ablation guard: change so much as a comment in either copy's `_write_event` + and this fails. The `missing` assertion is the second half: rename a twinned + function on one side and the comparison loop would otherwise have nothing to + compare and pass vacuously.""" + hook = _top_level_sources(HOOK) + twin = _top_level_sources(EVENTS) + + missing = {name: (name in hook, name in twin) for name in TWINNED} + assert all( + all(present) for present in missing.values() + ), f"a twinned name is gone from one side (name: in-hook, in-events): {missing}" + for name in TWINNED: + assert twin[name] == hook[name], ( + f"{name} has drifted between {HOOK.name} and {EVENTS.name} — " + f"fix both copies, or the two writers of the events control plane " + f"stop being hardened the same way" + ) + + +def test_relay_and_the_hook_shape_the_same_event(tmp_path, monkeypatch): + """The payload shaping and the event file name are the half of the twin that + `ast.get_source_segment` cannot reach — the hook does both inline in `main()`. + Pin them the only way left: feed one payload to both writers and compare what + lands. Every CLI's key spelling is in the payload, so a fallback dropped from + one side shows up as a null on that side only. + + Ablation guard: drop any `or payload.get(...)` arm from `events.shape_event`, + or reorder the event file name's fields, and this fails.""" + payload = { + "conversationId": "agy-3", + "transcriptPath": "/ws/transcript.jsonl", + "workspacePaths": ["/ws"], + } + hook_run, relay_run = tmp_path / "hook", tmp_path / "relay" + + proc = subprocess.run( + [sys.executable, str(HOOK), "Stop"], + input=json.dumps(payload), + # `env=` REPLACES the environment rather than extending it, and on Windows + # a Python child started without SYSTEMROOT can fail to load its + # side-by-side assemblies — a start failure that would read here as the + # relay misbehaving. Kept minimal otherwise: the relay is stdlib-only and + # must need nothing else. + env={ + "PATH": os.environ.get("PATH", ""), + **({"SYSTEMROOT": os.environ.get("SYSTEMROOT", "")} if os.name == "nt" else {}), + "BMAD_LOOP_RUN_DIR": str(hook_run), + "BMAD_LOOP_TASK_ID": "1-1-a-dev-1", + }, + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 0, proc.stderr + assert _relay("Stop", payload, monkeypatch, relay_run, task_id="1-1-a-dev-1") == 0 + + from_hook = next((hook_run / "events").glob("*.json")) + from_relay = next((relay_run / "events").glob("*.json")) + # `ts` is a timestamp and the name is built from it; compare the rest. + assert json.loads(from_relay.read_text()) | {"ts": 0} == ( + json.loads(from_hook.read_text()) | {"ts": 0} + ) + assert from_relay.name.split("-", 1)[1] == from_hook.name.split("-", 1)[1] + + +# -------------------------------------------------------- hardening (twinned) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_symlinked_events_dir_writes_nothing_and_exits_zero(tmp_path, monkeypatch, capsys): + """#461 (Low): a driven session can write inside the run dir, so it can plant + `events/` as a symlink and redirect the orchestrator's control-plane event + stream — swallowing the Stop signal and stalling the run to timeout. The relay + must refuse the link and degrade to a no-op, never write through it. + + This is the OUTCOME, which is what the operator has: rc 0, nothing said, + nothing anywhere. Two independent layers produce it on POSIX and deleting + either one alone leaves this green — the `_is_link_like` pre-check, and the + `O_NOFOLLOW` on the anchored dir open, which makes `os.open` of a symlinked + final component fail ELOOP on its own. So each layer is pinned where it is the + only thing standing: the pre-check by + `test_the_precheck_refuses_before_makedirs_on_the_fallback_path` (the fallback + has no dir open to lean on), and `O_NOFOLLOW`'s branch by + `test_the_anchored_branch_is_actually_taken_on_posix`. + + Ablation guard: delete the pre-`makedirs` `_is_link_like` refusal AND drop + `o_nofollow` from the `os.open(events_dir, …)` flags — with both layers gone + the payload lands in the attacker's directory and this fails. Verified; + removing only one does not redden it, which is why the two tests above + exist.""" + target = tmp_path / "attacker" + target.mkdir() + run_dir = tmp_path / "run" + run_dir.mkdir() + (run_dir / "events").symlink_to(target, target_is_directory=True) + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir) == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + assert list((run_dir / "events").iterdir()) == [] + + +class _ReparseStat: + """Stand-in for the os.lstat() result of a Windows junction: a DIRECTORY + mode (which is why os.path.islink() answers False) carrying a reparse tag.""" + + st_mode = stat.S_IFDIR | 0o755 + st_reparse_tag = 0xA0000003 # IO_REPARSE_TAG_MOUNT_POINT + + +def test_is_link_like_refuses_a_reparse_tagged_dir(tmp_path, monkeypatch): + """A Windows directory junction is a reparse point but NOT a symlink, so + `os.path.islink` is False for it while `os.makedirs`/`os.open` follow it — + and `mklink /J` needs no elevation, unlike a directory symlink, so it is the + cheaper attack. The refusal keys on the reparse tag instead. That branch is + reachable only on Windows; drive its logic here so it is not shipped + unexercised (the `stat.IO_REPARSE_TAG_*` constants do not exist on POSIX, + hence the substituted tuple). + + Ablation guard: dropping the `st_reparse_tag` arm of `_is_link_like` makes + the last assertion fail.""" + plain = tmp_path / "events" + plain.mkdir() + assert events._is_link_like(plain) is False + + real_lstat = os.lstat + monkeypatch.setattr(events, "_LINK_REPARSE_TAGS", (_ReparseStat.st_reparse_tag,)) + monkeypatch.setattr( + os, + "lstat", + lambda p, *a, **k: _ReparseStat() if str(p) == str(plain) else real_lstat(p), + ) + assert events._is_link_like(plain) is True + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_the_precheck_refuses_before_makedirs_on_the_fallback_path(tmp_path, monkeypatch): + """What the pre-check is for, in the one place it is load-bearing. On POSIX the + anchored branch's `O_NOFOLLOW` dir open refuses a symlinked `events/` by + itself, so the pre-check's absence is invisible there. The Windows fallback has + no dir open at all: `O_NOFOLLOW` on the create applies to the temp file's own + (non-link) name, so the create happily resolves THROUGH the redirected + directory and the payload is written into the attacker's tree before the + post-write re-check unlinks it again. + + Hence the two assertions. The refusal is the pre-check's, identified by its + message rather than by "an OSError happened" — the post-write re-check raises + one too and would mask this entirely. And `os.makedirs` never runs, which is + the docstring's own claim (`makedirs(exist_ok=True)` `isdir()`-checks THROUGH + the link, so the refusal has to come before it) stated as behavior. + + Ablation guard: delete the pre-`makedirs` `_is_link_like` refusal and this + fails on both counts — the message becomes `redirected mid-write` and + `makedirs` has run.""" + target = tmp_path / "attacker" + target.mkdir() + events_dir = tmp_path / "events" + events_dir.symlink_to(target, target_is_directory=True) + + made = [] + real_makedirs = os.makedirs + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) # force the fallback + monkeypatch.setattr(os, "makedirs", lambda p, **kw: (made.append(p), real_makedirs(p, **kw))[1]) + + with pytest.raises(OSError, match="refusing to write events into a redirected directory"): + events._write_event(str(events_dir), "1-t1-Stop.json", {"event": "Stop"}) + + assert made == [], "the refusal has to come BEFORE makedirs, which follows the link" + assert list(target.iterdir()) == [] + + +def test_fallback_refuses_a_redirect_that_appears_mid_write(tmp_path, monkeypatch): + """The Windows fallback has no dir_fd to anchor to, so it re-resolves + `events_dir` by path and a swap between the check and the create lands the + temp file in the attacker's directory. The post-write re-check catches a + swap that is still in place. Driven here with the dir_fd branch disabled, + because on POSIX that branch is always taken and the fallback would ship + unexercised. + + Ablation guard: deleting the post-write `_is_link_like` block makes this + fail — the event gets published instead of refused.""" + events_dir = tmp_path / "events" + calls = [] + real = events._is_link_like + + def swapped_after_the_check(path): + calls.append(path) + return len(calls) > 1 and real(path) is False # clean at check, dirty after + + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) # force the fallback + monkeypatch.setattr(events, "_is_link_like", swapped_after_the_check) + + with pytest.raises(OSError, match="redirected mid-write"): + events._write_event(str(events_dir), "1-t1-Stop.json", {"event": "Stop", "task_id": "t1"}) + + assert len(calls) == 2 # the check ran on both sides of the write + assert list(events_dir.iterdir()) == [] # nothing published, no .tmp left behind + + +@pytest.mark.skipif(os.name == "nt", reason="dir_fd is implemented with the POSIX *at() calls") +def test_the_anchored_branch_is_actually_taken_on_posix(tmp_path, monkeypatch): + """The capability probe must resolve to True where the capability exists, or + the TOCTOU-closing layer ships dead while every other test stays green — the + `islink` refusal covers the same cases, so nothing else would redden. + + This is not hypothetical: `os.replace` is NOT in `os.supports_dir_fd` on + Linux (only `os.rename` is) even though it accepts src_dir_fd/dst_dir_fd, so + probing `os.replace` — the function the code used to call — made the branch + unreachable on every platform. Observe the anchoring behaviorally rather + than re-deriving the probe, so editing the probe reddens this. + + Ablation guard: change the probe to `os.replace` (or drop the branch) and + this fails; no other test notices.""" + real_open, real_supports = os.open, os.supports_dir_fd + # The premise, asserted rather than assumed: were a future CPython to drop + # rename from the set, the branch would go dead and this says so directly. + assert {real_open, os.rename} <= real_supports + anchored = [] + + def spy(path, flags, mode=0o777, *, dir_fd=None): + anchored.append(dir_fd) + if dir_fd is None: + return real_open(path, flags, mode) + return real_open(path, flags, mode, dir_fd=dir_fd) + + monkeypatch.setattr(os, "open", spy) + # The probe tests os.open by IDENTITY, so the spy has to be in the capability + # set too or the probe answers False and this measures the fallback instead + # of the branch it exists to pin — which is how it first went red. + monkeypatch.setattr(os, "supports_dir_fd", real_supports | {spy}) + events._write_event(str(tmp_path / "events"), "1-t1-Stop.json", {"event": "Stop"}) + monkeypatch.undo() + + assert any(fd is not None for fd in anchored), "the create was never anchored to a dir_fd" + + +@pytest.mark.parametrize("forced_fallback", [False, True]) +def test_a_short_os_write_still_publishes_the_whole_payload(tmp_path, monkeypatch, forced_fallback): + """`os.write` may write FEWER bytes than asked and just return the count. The + buffered `open()` this replaced looped internally; the raw fd needed for + O_NOFOLLOW/dir_fd does not. A truncated event file is not retried but LOST: + `SignalWatcher.poll` adds a name to `_consumed` before parsing it, so + malformed JSON is skipped and never re-read — the Stop signal is gone and the + run waits out `session_timeout_min`. + + Both branches are driven: the loop is shared, but the fallback is the only + path Windows takes and POSIX would otherwise never exercise it. + + Ablation guard: collapsing `_write_all` back to a single `os.write` makes + this fail. Nothing else in the suite catches that — a real `os.write` + returns the full count, so the normal-path tests pass either way.""" + events_dir = tmp_path / "events" + real_write = os.write + + def a_byte_at_a_time(fd, data): + return real_write(fd, bytes(data)[:1]) # a legal short write + + if forced_fallback: + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) + event = {"event": "Stop", "task_id": "t1", "session_id": "s" * 300} + monkeypatch.setattr(os, "write", a_byte_at_a_time) + events._write_event(str(events_dir), "1-t1-Stop.json", event) + monkeypatch.undo() + + published = list(events_dir.glob("*.json")) + assert len(published) == 1 + assert json.loads(published[0].read_text()) == event + assert list(events_dir.glob("*.tmp")) == [] + + +def test_a_zero_length_write_raises_instead_of_spinning(tmp_path, monkeypatch): + """`_write_all` loops on short writes, so a descriptor that always accepts 0 + bytes would spin forever. Refuse instead: the caller degrades to a no-op and + the run takes the timeout path, which beats a hook process that never exits. + + Ablation guard: dropping the `written <= 0` arm hangs this test.""" + monkeypatch.setattr(os, "write", lambda fd, data: 0) + with pytest.raises(OSError, match="short write"): + events._write_event(str(tmp_path / "events"), "1-t1-Stop.json", {"event": "Stop"}) + + +@pytest.mark.skipif(os.name != "nt", reason="directory junctions are Windows-only") +def test_junctioned_events_dir_writes_nothing_and_exits_zero(tmp_path, monkeypatch, capsys): + """The Windows half of the symlink test — Windows CI is its only oracle, a + junction cannot be created on POSIX.""" + # If either tag constant were misnamed the tuple is empty and the refusal + # silently never fires. Assert it directly rather than inferring from below. + assert events._LINK_REPARSE_TAGS + + target = tmp_path / "attacker" + target.mkdir() + run_dir = tmp_path / "run" + run_dir.mkdir() + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(run_dir / "events"), str(target)], + check=True, + capture_output=True, + ) + # The premise, asserted rather than assumed: were a future CPython to start + # reporting junctions as links, this says so directly instead of going green + # for the wrong reason. + assert os.path.islink(run_dir / "events") is False + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir) == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes") +def test_event_file_mode_is_0600(tmp_path, monkeypatch): + """Event files carry the orchestrator's control plane; nothing but the + operator running the loop needs to read them (narrowed from the umask-derived + 0644 a plain `open()` produced). + + Ablation guard: drop the `0o600` argument from either `os.open` call and this + fails (the default 0o777 lands as 0o755 under the usual umask).""" + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + written = next((tmp_path / "events").glob("*.json")) + assert stat.S_IMODE(written.stat().st_mode) == 0o600 + + +# ------------------------------------------------------------- the relay command + + +def test_relay_writes_the_event_and_says_nothing(tmp_path, monkeypatch, capsys): + """The happy path, and the stdout invariant that rides on every path with it: + the hosts parse hook stdout, so a single stray line there is a protocol + violation even when the event itself landed.""" + payload = { + "session_id": "abc-123", + "transcript_path": "/home/u/.claude/projects/x/abc-123.jsonl", + "cwd": "/proj", + } + assert _relay("Stop", payload, monkeypatch, tmp_path, task_id="1-1-a-dev-1") == 0 + assert capsys.readouterr() == ("", "") + + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert "1-1-a-dev-1" in files[0].name and "Stop" in files[0].name + event = json.loads(files[0].read_text()) + assert event["event"] == "Stop" + assert event["task_id"] == "1-1-a-dev-1" + assert event["session_id"] == "abc-123" + assert event["transcript_path"].endswith("abc-123.jsonl") + assert not list((tmp_path / "events").glob("*.tmp")) + + +def test_relay_is_a_silent_noop_outside_a_driven_session(tmp_path, monkeypatch, capsys): + """An operator (or a stray hook config) can invoke `bmad-loop relay` in a + session bmad-loop never spawned. The session-protocol env is the detector, and + the answer is to write nothing and say nothing at rc 0 — a normal interactive + session must be unaffected by having the hooks installed.""" + monkeypatch.chdir(tmp_path) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, None) == 0 + assert capsys.readouterr() == ("", "") + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.parametrize( + "garbage", + ["", " ", "{not json", "[1, 2, 3]", '"a bare string"', "null"], + ids=["empty", "blank", "truncated", "list", "string", "null"], +) +def test_relay_tolerates_garbage_on_stdin(tmp_path, monkeypatch, capsys, garbage): + """A hook that fires with nothing (or something non-dict) on stdin still has to + produce the event: the run's completion signal rides on the file, not on the + payload. Every unusable payload collapses to nulls, never to a refusal. + + Ablation guard: delete the `isinstance(payload, dict)` collapse and the + list/string/null rows fail on the `.get` that follows. Every row here is a + `json.JSONDecodeError`, so the rest of `_read_payload`'s `except` is pinned + separately by `test_relay_tolerates_an_unreadable_stdin`.""" + assert _relay("SessionEnd", garbage, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["session_id"] is None + + +class _UnreadableStream: + """A stdin `json.load` cannot read. `json.load` calls `fp.read()`, so the + failure surfaces from there exactly as a real one would.""" + + def __init__(self, exc: BaseException): + self._exc = exc + + def read(self, *_a): + raise self._exc + + +@pytest.mark.parametrize( + "exc", + [ + OSError("broken pipe"), + UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte"), + ], + ids=["oserror", "undecodable"], +) +def test_relay_tolerates_an_unreadable_stdin(tmp_path, monkeypatch, capsys, exc): + """Beyond malformed JSON, the read itself can fail: the host can hand the hook + a closed descriptor, or bytes that are not valid UTF-8 (a transcript path from + a differently-encoded filesystem is the realistic source). Neither is + actionable and neither may cost the event — the run's completion signal rides + on the file landing. + + These are the two arms the malformed-JSON rows do NOT reach: + `UnicodeDecodeError` is a `ValueError` but not a `json.JSONDecodeError`, and + `OSError` is neither. + + Ablation guard: narrow `_read_payload`'s `except (ValueError, OSError)` to + `json.JSONDecodeError` and both rows fail — the exception escapes to + `cmd_relay`'s backstop, so no event is written and stderr is no longer + empty.""" + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(tmp_path)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", "t1") + monkeypatch.setattr(sys, "stdin", _UnreadableStream(exc)) + + assert cli.main(["relay", "Stop"]) == 0 + assert capsys.readouterr() == ("", "") + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["session_id"] is None + + +def test_relay_degrades_to_zero_when_the_write_fails(tmp_path, monkeypatch, capsys): + """Any OSError out of the write — a full disk, a read-only run dir, the + redirect refusals above — degrades to the orchestrator's normal + `session_timeout_min` path. A non-zero rc here is surfaced by several hosts as + a failed tool call inside the very session whose completion this reports. + + Ablation guard: delete the `except OSError` arm in `events.relay` and this + fails with the OSError escaping to the caller.""" + + def boom(*_a, **_k): + raise OSError("no space left on device") + + monkeypatch.setattr(events, "_write_event", boom) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + + +def test_relay_survives_an_unexpected_exception(tmp_path, monkeypatch, capsys): + """The backstop for a bug in this code path rather than a hostile events dir. + Same reason as the OSError arm — a hook that exits non-zero breaks the session + — but it reports on stderr, which the hosts do not parse, instead of failing + silently. + + Ablation guard: delete `cmd_relay`'s `except Exception` and this fails with + the RuntimeError escaping `main()`.""" + + def boom(*_a, **_k): + raise RuntimeError("a bug, not a hostile events dir") + + monkeypatch.setattr(events, "relay", boom) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + out, err = capsys.readouterr() + assert out == "" + assert "RuntimeError" in err + + +def test_relay_runs_without_the_mux_or_a_readable_policy(tmp_path, monkeypatch, capsys): + """`main()` configures the mux backend from policy before dispatch, for the + handlers that reach the mux without ever loading policy. Relay reaches neither, + and a project whose policy.toml is unparseable must still be able to report + that its session stopped — so relay dispatches ahead of that call entirely. + + Ablation guard: move the relay dispatch below `_configure_mux(_project(args))` + and this fails — `main()`'s `except PolicyError` arm prints `error: …` and + returns 1, which is exactly the CLI-window failure the hook contract forbids.""" + + def explode(_project): + raise policy_mod.PolicyError("policy.toml is not valid TOML") + + monkeypatch.setattr(cli, "_configure_mux", explode) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + assert len(list((tmp_path / "events").glob("*.json"))) == 1 + + +def test_relay_dispatches_outside_the_shared_error_handler(tmp_path, monkeypatch): + """The placement of the dispatch, stated directly rather than inferred: relay + is not wrapped by `main()`'s shared try/except, whose arms print `error: …` + and return 1 or 130. `cmd_relay` is total, so in production nothing reaches + this — the test forces the question by making the handler itself raise. + + Ablation guard: delete the early dispatch and this fails; the raise is caught + by `main()`'s typed arm and turned into rc 1.""" + + def explode(_args): + raise policy_mod.PolicyError("would be swallowed by main()'s handler") + + monkeypatch.setattr(cli, "cmd_relay", explode) + monkeypatch.setattr(sys, "stdin", io.StringIO("{}")) + with pytest.raises(policy_mod.PolicyError): + cli.main(["relay", "Stop"]) + + +def test_relay_defaults_the_event_name_like_the_hook(tmp_path, monkeypatch, capsys): + """The hook script reads `sys.argv[1] if len(sys.argv) > 1 else "Unknown"`, so + a misconfigured registration that forgets the event name still produces a file + the operator can see. argparse would otherwise turn that into a usage error at + rc 2, before any handler runs — nothing `cmd_relay` does could take it back.""" + assert _relay_no_event({"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + assert "Unknown" in next((tmp_path / "events").glob("*.json")).name + + +def _relay_no_event(payload, monkeypatch, run_dir: Path) -> int: + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", "t1") + monkeypatch.delenv("BMAD_LOOP_EVENTS_DIR", raising=False) + monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload))) + return cli.main(["relay"]) + + +# ------------------------------------------- where the event is written (#494) + + +def test_relay_prefers_the_events_dir_env(tmp_path, monkeypatch, capsys): + """The relay is the OTHER writer of this control plane, so it resolves the + directory exactly as the copied hook script does — the two are pointed at one + channel by the same variable, and a preference that held on only one of them + would split the channel in half depending on which target a project's hook + config happens to name.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + # Not `events` — that name is the imported module for the rest of this file. + events_dir = tmp_path / "state" / "runs" / "RID" / "events" + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=events_dir) == 0 + assert capsys.readouterr() == ("", "") + + files = list(events_dir.glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + assert not (run_dir / "events").exists() + + +@pytest.mark.parametrize("value", [None, ""], ids=["unset", "empty"]) +def test_relay_falls_back_to_the_run_dir(tmp_path, monkeypatch, capsys, value): + """An orchestrator predating #494 names no events dir; its sessions must still + write where it polls. Empty is the same case in disguise — `export + BMAD_LOOP_EVENTS_DIR=` leaves an empty value behind, and an empty path names + the launch cwd rather than a control plane. + + Ablation guard: swap the `or` for a presence test and the empty case writes + into the cwd — this fails.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=value) == 0 + assert capsys.readouterr() == ("", "") + + files = list((run_dir / "events").glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_relay_refuses_a_symlinked_env_directed_events_dir(tmp_path, monkeypatch, capsys): + """The #493 hardening is a property of the directory the relay is pointed at, + not of how that directory was derived — so it must hold for an env-named one, + with the same silent rc 0 degrade.""" + target = tmp_path / "attacker" + target.mkdir() + state = tmp_path / "state" + state.mkdir() + (state / "events").symlink_to(target, target_is_directory=True) + run_dir = tmp_path / "run" + run_dir.mkdir() + + rc = _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=state / "events") + assert rc == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + assert list((state / "events").iterdir()) == [] + assert not (run_dir / "events").exists() # no silent fallback to the legacy dir diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 66e0936e..9f905b35 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -40,21 +40,33 @@ # raises UnicodeDecodeError — a ValueError, NOT an OSError. _BAD_UTF8 = b"\xff\xfe\x00\x01 not utf-8 \x80\x81" +# The line that decides where the fake relay writes its events. Swapped below to +# build the version-skew twin, so the two scripts differ in nothing else. +_EVENTS_LINE = 'ed="$BMAD_LOOP_EVENTS_DIR"' +_LEGACY_EVENTS_LINE = 'ed="$BMAD_LOOP_RUN_DIR/events"' + FAKE_CLI = """#!/bin/bash # fake CLI: last positional arg is the prompt; env comes from tmux -e prompt="${@: -1}" ts=$(date +%s%N) -mkdir -p "$BMAD_LOOP_RUN_DIR/events" "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \\ - "$ts" "$BMAD_LOOP_TASK_ID" > "$BMAD_LOOP_RUN_DIR/events/$ts-$BMAD_LOOP_TASK_ID-SessionStart.json" + "$ts" "$BMAD_LOOP_TASK_ID" > "$ed/$ts-$BMAD_LOOP_TASK_ID-SessionStart.json" echo "{\\"workflow\\": \\"auto-dev\\", \\"prompt\\": \\"$prompt\\"}" \\ > "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID/result.json" ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \\ - "$ts2" "$BMAD_LOOP_TASK_ID" > "$BMAD_LOOP_RUN_DIR/events/$ts2-$BMAD_LOOP_TASK_ID-Stop.json" + "$ts2" "$BMAD_LOOP_TASK_ID" > "$ed/$ts2-$BMAD_LOOP_TASK_ID-Stop.json" sleep 60 # stay alive like an idle interactive session """ +# What a relay installed before #494 knows: only the in-tree location. Pairing it +# with a current orchestrator is the version-skew case the dual poll covers, and +# it is the ordinary state of any project whose `.bmad-loop/bmad_loop_hook.py` +# copy predates the move. +LEGACY_EVENTS_FAKE_CLI = FAKE_CLI.replace(_EVENTS_LINE, _LEGACY_EVENTS_LINE) + def make_adapter( tmp_path, profile_name="claude", binary=None, extra_args=None, mux=None, **policy_kw @@ -73,6 +85,12 @@ def make_adapter( binary=binary, extra_args=extra_args, mux=mux, + # As `runsetup.make_adapters` does: the primary channel is out of the + # project tree (#494), keyed by run. Under `tmp_path` rather than the real + # state root only because these are unit tests; what matters is that it is + # NOT `run_dir / "events"`, so every test here drives the production shape + # (out-of-tree primary, in-tree legacy still under poll). + events_dir=tmp_path / "state" / run_dir.name / "events", ) @@ -2976,9 +2994,9 @@ def test_read_usage_none_without_transcript(tmp_path): assert adapter.read_usage(SessionResult(status="completed")) is None -def _write_fake_cli(tmp_path): +def _write_fake_cli(tmp_path, script: str = FAKE_CLI): fake = tmp_path / "fake-cli" - fake.write_text(FAKE_CLI) + fake.write_text(script) fake.chmod(0o755) return fake @@ -2995,6 +3013,7 @@ def test_tmux_end_to_end_with_fake_cli(tmp_path, profile_name): spec_env = { "BMAD_LOOP_MODE": "1", "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), "BMAD_LOOP_TASK_ID": "t-int-1", } spec = SessionSpec( @@ -3042,7 +3061,11 @@ def test_tmux_reused_task_id_ignores_stale_artifacts(tmp_path): role="dev", prompt="/bmad-dev-auto 1-1-a", cwd=tmp_path, - env={"BMAD_LOOP_RUN_DIR": str(adapter.run_dir), "BMAD_LOOP_TASK_ID": task_id}, + env={ + "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), + "BMAD_LOOP_TASK_ID": task_id, + }, timeout_s=30.0, ) try: @@ -3055,6 +3078,49 @@ def test_tmux_reused_task_id_ignores_stale_artifacts(tmp_path): assert result.session_id == "fake-1" # fresh session, not "old" +@pytest.mark.skipif(not HAVE_TMUX, reason="tmux not available") +def test_tmux_end_to_end_with_a_relay_that_only_knows_the_legacy_dir(tmp_path): + """The version-skew guard, end to end through real tmux: a CURRENT + orchestrator (it exports BMAD_LOOP_EVENTS_DIR and waits on the out-of-tree + channel) driving a session whose relay is an OLD copy that writes only to + `/events`. That pairing is not exotic — the relay is copied into the + target project at init, so every project not re-inited after an upgrade is in + it, and without the watcher's legacy poll EVERY such session stalls to + `session_timeout_min` instead of completing. + + Ablation guard: drop `legacy_dir` from `SignalWatcher._dirs()` and this fails + (as a 30s timeout, not an assertion — which is precisely the production + symptom).""" + assert "$BMAD_LOOP_EVENTS_DIR" not in LEGACY_EVENTS_FAKE_CLI, "the twin still reads the new var" + assert LEGACY_EVENTS_FAKE_CLI != FAKE_CLI, "the swap did not take" + + fake = _write_fake_cli(tmp_path, LEGACY_EVENTS_FAKE_CLI) + adapter = make_adapter(tmp_path, binary=str(fake), extra_args=()) + spec = SessionSpec( + task_id="t-legacy-1", + role="dev", + prompt="/bmad-dev-auto 1-1-a", + cwd=tmp_path, + env={ + "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), + "BMAD_LOOP_TASK_ID": "t-legacy-1", + }, + timeout_s=30.0, + ) + try: + result = adapter.run(spec) + finally: + subprocess.run(["tmux", "kill-session", "-t", adapter.session_name], capture_output=True) + + assert result.status == "completed" + assert result.session_id == "fake-1" + # the premise, asserted rather than assumed: the events really did land in the + # legacy location and nowhere else, so the completion came through the fallback + assert list((adapter.run_dir / "events").glob("*.json")) + assert not list(adapter.watcher.events_dir.glob("*.json")) + + @pytest.mark.skipif(not HAVE_TMUX, reason="tmux not available") def test_tmux_crash_detected(tmp_path): """A session that dies without writing result.json -> crashed. Also the diff --git a/tests/test_hook_script.py b/tests/test_hook_script.py index 96e70138..3c81351a 100644 --- a/tests/test_hook_script.py +++ b/tests/test_hook_script.py @@ -331,6 +331,85 @@ def test_event_file_mode_is_0600(tmp_path): assert stat.S_IMODE(written.stat().st_mode) == 0o600 +# --------------------------------------------- where the event is written (#494) + + +def test_the_events_dir_env_wins_over_the_run_dir(tmp_path): + """#494: the channel moved out of the project tree, and this variable is how + the orchestrator names it. Nothing may land in the legacy in-tree location + when it is set — an event written there is only found by the orchestrator's + compatibility poll, and the whole point is that a branch switch or a worktree + mount cannot take the live channel away.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + events = tmp_path / "state" / "runs" / "RID" / "events" + env = { + "BMAD_LOOP_RUN_DIR": str(run_dir), + "BMAD_LOOP_EVENTS_DIR": str(events), + "BMAD_LOOP_TASK_ID": "t1", + } + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + files = list(events.glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + assert not (run_dir / "events").exists() + + +@pytest.mark.parametrize("value", [None, ""], ids=["unset", "empty"]) +def test_the_events_dir_falls_back_to_the_run_dir(tmp_path, value): + """The version-skew half, from the producing side: an orchestrator that + predates #494 sets no BMAD_LOOP_EVENTS_DIR, and its sessions must still write + their events somewhere the orchestrator polls — the legacy location it knows. + + The empty case is not hypothetical: `export BMAD_LOOP_EVENTS_DIR=` is what an + unset-looking export leaves behind, and an empty path names the launch cwd, + which is not a control plane. + + Ablation guard: change the `or` to a presence test (`is not None`) and the + empty case writes into the CLI's working directory instead — this fails.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + env = {"BMAD_LOOP_RUN_DIR": str(run_dir), "BMAD_LOOP_TASK_ID": "t1"} + if value is not None: + env["BMAD_LOOP_EVENTS_DIR"] = value + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + files = list((run_dir / "events").glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_a_symlinked_env_directed_events_dir_writes_nothing_and_exits_zero(tmp_path): + """The #493 hardening is about the DIRECTORY the relay is pointed at, so it + has to hold for the one an env var names just as it did for the one derived + from the run dir. Under isolation a driven session can write into the project + but not into the state root — yet the variable itself is inherited env, and a + session that can plant a link at the named path must still not redirect the + control plane through it.""" + target = tmp_path / "attacker" + target.mkdir() + state = tmp_path / "state" + state.mkdir() + (state / "events").symlink_to(target, target_is_directory=True) + run_dir = tmp_path / "run" + run_dir.mkdir() + + env = { + "BMAD_LOOP_RUN_DIR": str(run_dir), + "BMAD_LOOP_EVENTS_DIR": str(state / "events"), + "BMAD_LOOP_TASK_ID": "t1", + } + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + assert list(target.iterdir()) == [] + assert list((state / "events").iterdir()) == [] + # and it did not silently fall back to the legacy location either + assert not (run_dir / "events").exists() + + def test_installed_copy_matches_source(tmp_path): from bmad_loop.install import install_into diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 92b06d4a..86ffc144 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -154,6 +154,9 @@ def test_generic_adapter_drives_only_the_mux(tmp_path, no_tmux): policy=Policy(limits=LimitsPolicy()), profile=get_profile("claude"), mux=stub, + # out of the project tree, as `runsetup.make_adapters` resolves it (#494), + # so the seeded Stop below has to be observed on the PRIMARY channel + events_dir=tmp_path / "state" / "events", ) spec = _spec(tmp_path) diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index bf4339df..4ae1bc44 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -131,6 +131,7 @@ # and these scripts read back what was handed to them. SESSION_PROTOCOL_ENV = ( "BMAD_LOOP_RUN_DIR", + "BMAD_LOOP_EVENTS_DIR", "BMAD_LOOP_TASK_ID", "BMAD_LOOP_WORKTREE", "BMAD_LOOP_REPO_ROOT", @@ -164,6 +165,16 @@ # producing side is what these readers consume, not a second source of truth. ENV_READ_ALLOW = { "envvars.py": tuple(REGISTRY_NAMES.values()), + # `events.py` is the ONE in-package entry here, and the "cannot import + # bmad_loop" justification above does not reach it — it obviously can. It is + # exempt as the importable PARITY TWIN of the stdlib-only hook relay: the same + # session-protocol vars, read at the same points in the same protocol, by + # the code the hook config points at when it points at `bmad-loop relay` + # instead of the copied script. Routing one twin through `envvars` and leaving + # the other on `os.environ` would put the reads out of parity, and parity is + # what the AST test on those two files exists to keep. Family-scoped like the + # rest, so a core knob read inline here is still an offender. + "events.py": SESSION_PROTOCOL_ENV, "data/bmad_loop_hook.py": SESSION_PROTOCOL_ENV, "data/bmad_loop_probe_hook.py": SESSION_PROTOCOL_ENV, "data/plugins/unity/unity_cleanup.py": UNITY_ENV, diff --git a/tests/test_runs.py b/tests/test_runs.py index f20c7ef8..b5224ba7 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -6,11 +6,12 @@ import subprocess import sys import tarfile +from pathlib import Path import pytest -from conftest import escalated_run, git +from conftest import escalated_run, git, refuse_to_resolve -from bmad_loop import platform_util, runs, verify +from bmad_loop import envvars, platform_util, runs, verify from bmad_loop.adapters import tmux_base from bmad_loop.adapters.multiplexer import MultiplexerError from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer @@ -727,6 +728,239 @@ def test_project_tag_is_transportable_whatever_the_path(tmp_path): assert len({tag, runs.project_tag(tmp_path / "other")}) == 2 +# ------------------------------------------------- user-scoped state root (#494) +# +# Every row here clears the suite-wide `_isolate_state_root` override first: that +# fixture exists so no test writes into the real state directory, and it is the +# first thing `state_root` consults, so a cascade row that left it set would grade +# nothing. `sys.platform` is faked per branch (the house idiom — see +# test_journal.py), and the fake home is written to HOME *and* USERPROFILE because +# `expanduser` reads the first on POSIX and the second on Windows, so a row must +# set both to mean the same thing on either host (tests/test_diagnostics.py:630). + + +def _fake_home(monkeypatch, home) -> None: + """Point `expanduser("~")` at `home` on whichever host is running, and clear + everything else `state_root` would answer from first.""" + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.delenv("XDG_STATE_HOME", raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + + +def test_state_root_precedence_override_then_xdg_then_home(tmp_path, monkeypatch): + """Three answers, ranked, and the ranking is what each assertion removes. + + The override outranks a *set* XDG_STATE_HOME and not merely an unset one — + graded by leaving XDG set throughout — because it is the operator's stated + answer and the suite's own isolation depends on it winning against whatever a + host exports. + + The asymmetry in the middle is deliberate and pinned here rather than left to + the reader: `XDG_STATE_HOME` is a *base* to build under, so `bmad-loop` is + appended to it, while `BMAD_LOOP_STATE_DIR` names our root itself and is used + as spelled. Appending to the override would silently move every path a + phase-3 hook computes from that same variable.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + home = tmp_path / "home" + _fake_home(monkeypatch, home) + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "xdg")) + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "override")) + + assert runs.state_root() == tmp_path / "override" # verbatim: no "bmad-loop" tail + + monkeypatch.delenv(envvars.STATE_DIR) + assert runs.state_root() == tmp_path / "xdg" / "bmad-loop" + + monkeypatch.delenv("XDG_STATE_HOME") + assert runs.state_root() == home / ".local" / "state" / "bmad-loop" + + +@pytest.mark.parametrize("value", ["state", "./state", "~/state"], ids=repr) +def test_state_root_refuses_a_relative_override(tmp_path, monkeypatch, value): + """`BMAD_LOOP_STATE_DIR` is honoured as spelled, but a relative spelling is + not a root — it is two roots. The engine exports this path to the session as + `BMAD_LOOP_EVENTS_DIR` and the multiplexer launches that session at + `spec.cwd` (a worktree, under isolation), while the watcher polls from the + orchestrator's cwd. So the relay writes its Stop into one directory and + nothing watches it, and the run waits out `session_timeout_min` — silent, and + exactly the stall moving this channel out of the tree was meant to prevent. + + It RAISES rather than falling through to the cascade, which is the split from + the sibling XDG test above: a derived base that fails its check is a guess we + move on from, an override is a statement we cannot honour and must not + silently replace. It equally does not absolutize — resolving against whichever + cwd this process happens to have is the guess, and picking one of the two + directories at random is how the stall gets harder to see rather than gone. + + `~/state` is here for the same reason it is in the XDG rows: nothing expands + it, so it stays relative. The empty string is deliberately NOT a row — empty + reads as *unset* and falls through to the cascade, which + `test_state_root_precedence_override_then_xdg_then_home` already grades. + + Ablation target: drop the `os.path.isabs` guard from the override arm and all + three rows fail — each returning a cwd-relative root instead of raising.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, tmp_path / "home") + monkeypatch.setenv(envvars.STATE_DIR, value) + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + +@pytest.mark.parametrize("value", ["state", "./state", "~/state", ""], ids=repr) +def test_state_root_ignores_an_xdg_state_home_that_is_not_absolute(tmp_path, monkeypatch, value): + """The XDG base-directory spec says a relative value "must be ignored", and + ignoring it means falling through to the home default — not resolving it + against the cwd, which is what `Path(value) / "bmad-loop"` would do. + + That distinction is the whole test: a cwd-relative control plane is not a + failure anyone sees, it is a run whose events land somewhere the next process + to ask does not look, and a run that finds no completion signal waits out + `session_timeout_min`. `~/state` is here because expansion is not this + reader's job either — nothing expands it, so it stays relative. The empty + string is the same rule reached from the other side: set-but-empty is how an + unset-looking export reads, and `Path("")` is the cwd. + + Ablation target: drop the `os.path.isabs` half of `_state_base` and the three + relative rows fail together, each on a cwd-relative root. The empty row is + held by BOTH halves — `os.path.isabs("")` is already False — so no single + ablation reddens it here, and it is kept as the spelling an operator produces + rather than as an independent gate. What the emptiness half holds alone is the + *unset* variable, where `os.path.isabs(None)` raises; the refusal rows below + are what grade it.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + home = tmp_path / "home" + _fake_home(monkeypatch, home) + monkeypatch.setenv("XDG_STATE_HOME", value) + + assert runs.state_root() == home / ".local" / "state" / "bmad-loop" + + +def test_state_root_on_win32_prefers_localappdata_over_the_user_profile(tmp_path, monkeypatch): + """Windows keeps this class of per-user, per-machine state under + `%LOCALAPPDATA%`, and `%USERPROFILE%\\AppData\\Local` is that variable's + documented default location — the fallback, not a second opinion. + + XDG_STATE_HOME stays set across both assertions: it is a POSIX variable, and a + branch that consulted it on Windows would answer from an operator's WSL or + MSYS environment instead of the local store.""" + monkeypatch.setattr(runs.sys, "platform", "win32") + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "xdg")) + monkeypatch.setenv("LOCALAPPDATA", str(tmp_path / "local")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "profile")) + + assert runs.state_root() == tmp_path / "local" / "bmad-loop" / "state" + + monkeypatch.delenv("LOCALAPPDATA") + expected = tmp_path / "profile" / "AppData" / "Local" / "bmad-loop" / "state" + assert runs.state_root() == expected + + +def test_state_root_on_win32_refuses_a_home_derived_from_homedrive(tmp_path, monkeypatch): + """With neither `%LOCALAPPDATA%` nor `%USERPROFILE%` set, refuse — do not fall + back to a home directory. + + `Path.home()` is `ntpath.expanduser("~")`, which prefers `USERPROFILE` and then + `HOMEDRIVE` + `HOMEPATH`; on a domain-joined machine that pair can name a + network home share, and the control plane's `O_NOFOLLOW`-anchored writes and + atomic renames are not something to move onto SMB by inference. So the + variables are read by name and an absent store raises. + + Ablation target: replace the `USERPROFILE` read with `Path.home()` and this + row fails — on Windows it derives `Z:\\users\\x`, and on a POSIX host running + the faked branch `expanduser` falls back to the passwd entry. Both answer + where the guard refuses to.""" + monkeypatch.setattr(runs.sys, "platform", "win32") + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.delenv("USERPROFILE", raising=False) + monkeypatch.delenv("HOME", raising=False) + monkeypatch.setenv("HOMEDRIVE", "Z:") + monkeypatch.setenv("HOMEPATH", "\\users\\x") + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + +@pytest.mark.parametrize("home", ["", "relative-home", "~"], ids=repr) +def test_state_root_refuses_a_home_that_cannot_root_a_control_plane(monkeypatch, home): + """A write path fails loud rather than picking a plausible-looking directory. + + Each row is an answer `expanduser` really gives: `""` for a set-but-empty + `USERPROFILE` on Windows — and, on POSIX, `"/"`, since `posixpath` folds the + empty prefix to the root; `"relative-home"` for a `HOME` that is not a path at + all; and `"~"` for the input handed back when nothing can expand it. All three + would otherwise mkdir the control plane somewhere silently wrong — the launch + cwd, or `/.local/state`, which is a permission error for an ordinary user and + a real write to `/` for a containerised root. + + The message names the override, because that is the one remedy an operator + always has.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, home) + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + +def test_state_dir_for_is_keyed_on_project_identity_not_spelling(tmp_path, monkeypatch): + """One project reached by two spellings must key to ONE control plane. + + It is the same requirement `project_tag` was written for, and it reaches here + because the run and the hook that signals its completion can arrive with + different spellings of the project: the engine holds a resolved path, a relay + computes from what it was handed. Two keys would mean the poller watching one + directory while the events land in the other — no completion signal, and a run + that waits out `session_timeout_min` with the signal sitting on disk. + + Distinctness is asserted alongside identity: a key that collapsed every + project would satisfy the first half by making all runs share one plane.""" + project = tmp_path / "proj" + project.mkdir() + monkeypatch.chdir(tmp_path) + + absolute = runs.state_dir_for(project, "20260812-101500-ab12") + relative = runs.state_dir_for(Path("proj"), "20260812-101500-ab12") + assert absolute == relative + assert absolute == runs.state_root() / runs.project_tag(project) / "20260812-101500-ab12" + assert runs.events_dir_for(project, "20260812-101500-ab12") == absolute / "events" + + assert runs.state_dir_for(project, "20260812-101500-cd34") != absolute + assert runs.state_dir_for(tmp_path / "other", "20260812-101500-ab12") != absolute + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_state_dir_for_follows_a_symlinked_project_to_one_key(tmp_path): + """The symlink half of the spelling problem, and the one a lexical comparison + of the two paths would never catch — they share no component.""" + project = tmp_path / "proj" + project.mkdir() + link = tmp_path / "link" + link.symlink_to(project) + + assert runs.state_dir_for(link, "r1") == runs.state_dir_for(project, "r1") + + +def test_state_dir_for_raises_when_the_project_cannot_be_canonicalized(tmp_path, monkeypatch): + """A project the OS refuses to canonicalize (#552: a registered-but-not-serving + WSL UNC provider) has no knowable identity, so it gets no key. + + `project_tag`'s bare `resolve()` raising is the correct behaviour to inherit + rather than soften. Degrading to the lexical spelling would hand two spellings + of the one project two different control planes — the failure the test above + exists to prevent — and this is a write path, where the doctrine is to raise + (`platform_util.resolve_or_lexical`).""" + project = tmp_path / "proj" + project.mkdir() + refuse_to_resolve(monkeypatch, project) + + with pytest.raises(OSError): + runs.state_dir_for(project, "r1") + + def test_prunable_sessions_accepts_legacy_path_tag(tmp_path, monkeypatch): """A pre-digest tag stays ours; another project's path or digest stays foreign.""" legacy = str(tmp_path.resolve()) @@ -849,12 +1083,79 @@ def test_prune_sessions_returns_unknown_from_same_sample(tmp_path, monkeypatch): assert killed == ["odd-1"] +def _seed_state_dir(project, run_id) -> Path: + """The out-of-tree control plane a driven run leaves behind: its events dir + holding one already-consumed completion signal.""" + events = runs.events_dir_for(project, run_id) + events.mkdir(parents=True) + (events / "1700000000-t1-Stop.json").write_text("{}") + return runs.state_dir_for(project, run_id) + + +def _raising(exc: Exception): + def _fail(*_args, **_kwargs): + raise exc + + return _fail + + def test_delete_run(tmp_path): run_dir = _make_state_run(tmp_path, "r1") runs.delete_run(tmp_path, run_dir) assert not run_dir.exists() +def test_delete_run_removes_the_out_of_tree_state_counterpart(tmp_path): + """#494 moved the events channel out of the project tree, so removing the run + dir stopped removing everything the run owns. Without this tail every delete + leaks a subtree under the user-scoped state root — outside the project, where + no operator thinks to look — one per run, for the life of the machine.""" + run_dir = _make_state_run(tmp_path, "r1") + state_dir = _seed_state_dir(tmp_path, "r1") + + runs.delete_run(tmp_path, run_dir) + + assert not run_dir.exists() + assert not state_dir.exists() + + +@pytest.mark.parametrize( + "attr, exc", + [ + ("state_root", runs.StateRootError("no root")), + ("project_tag", OSError("cannot canonicalize")), + ("project_tag", RuntimeError("Symlink loop from '/p'")), + ], + ids=["no-derivable-state-root", "unresolvable-project", "symlink-loop-project"], +) +def test_delete_run_survives_a_counterpart_it_cannot_name(tmp_path, monkeypatch, attr, exc): + """The counterpart removal is a never-raise tail (#139 teardown doctrine). + + Every row is the counterpart being *unnameable*, which is the only failure + that can escape: an environment with no derivable state root, and a project + the OS refuses to canonicalize (#552). Removal failures are absorbed + separately, by `ignore_errors`. + + The `RuntimeError` row is not a hypothetical type: `project_tag` resolves + before digesting, and below 3.13 `Path.resolve` reports a symlink loop as + `RuntimeError` rather than `OSError` — measured across the support matrix, + 3.11 and 3.12 raise it where 3.13 and 3.14 return the unresolved path. So on + two supported interpreters this is the live arm, and it is injected here + rather than built from real symlinks because the loop would have to sit on + the *project* path, which the sandbox fixtures own. + + Raising would be worse than the leak it reports. The run dir is already gone + by this point, so the exception would fail a delete that in fact happened and + send the operator to retry a removal that can only fail the same way — while + `reconcile_orphan_state_dirs` already backstops the leak.""" + run_dir = _make_state_run(tmp_path, "r1") + monkeypatch.setattr(runs, attr, _raising(exc)) + + runs.delete_run(tmp_path, run_dir) + + assert not run_dir.exists() + + def test_delete_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypatch): """The #419 backstop: every caller's guard is keyed on engine pid liveness, so an orphan (engine dead, session alive) reaches here. For an untagged session the run @@ -1206,3 +1507,252 @@ def test_archive_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypat runs.archive_run(tmp_path, run_dir) assert run_dir.exists() assert not (tmp_path / ".bmad-loop" / "archive").exists() + + +def test_archive_run_removes_the_out_of_tree_state_counterpart(tmp_path): + """Archive inherits delete's tail — it removes the run dir just the same, so + it would leak the same subtree. + + The ordering is what the assertions pin: the tarball is complete before the + counterpart goes. Since #494 that tarball no longer carries the run's + `events/` — the channel is out of the tree and never enters the tar — which + the recorded decision accepts: those files are transient completion signals + the watcher consumed while the run was live, and everything an archive is + read for later (state, journal, tasks, logs) is in the run dir.""" + run_dir = _make_state_run(tmp_path, "20260611-100000-aaaa") + state_dir = _seed_state_dir(tmp_path, "20260611-100000-aaaa") + + dest = runs.archive_run(tmp_path, run_dir) + + with tarfile.open(dest) as tar: + assert "20260611-100000-aaaa/state.json" in tar.getnames() + assert not state_dir.exists() + + +# ------------------------------------------------- orphan state-dir sweep (#494) + + +def test_reconcile_orphan_state_dirs_removes_only_what_has_no_run_dir(tmp_path): + """The GC backstop for everything `_discard_state_dir` cannot reach: a run dir + removed by hand, an `rm -rf .bmad-loop`, a delete from before that tail + existed. Distinctness is the load-bearing half — a sweep that took the live + run's control plane too would strand a resumable run's completion channel.""" + _make_state_run(tmp_path, "live-1") + kept = _seed_state_dir(tmp_path, "live-1") + orphan = _seed_state_dir(tmp_path, "gone-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_keeps_a_run_dir_with_no_state_json(tmp_path): + """Existence of the *directory* is the test, not `list_run_dirs`, which is + state.json-gated. + + A run whose state.json is missing or corrupt is exactly the run an operator is + trying to recover, and it still owns its control plane. Reading liveness from + the gated listing would sweep the counterpart out from under it — deleting + state on the strength of state being unreadable.""" + _make_run(tmp_path, "corrupt-1", with_state=False) + kept = _seed_state_dir(tmp_path, "corrupt-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_dry_run_reports_without_removing(tmp_path): + """`clean --dry-run` promises a preview: the plan must name the work and leave + the disk alone, so the count a caller pre-flights is the count they get.""" + orphan = _seed_state_dir(tmp_path, "gone-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path, dry_run=True) == [orphan] + + assert orphan.is_dir() + + +def test_reconcile_orphan_state_dirs_sweeps_a_project_whose_runs_dir_is_gone(tmp_path): + """`rm -rf .bmad-loop` is the leak this exists for, and it is the case a + missing runs dir has to answer *as an answer*: no runs exist, so every state + dir under this project's key is an orphan. Reading it as "cannot tell" would + leave the whole subtree behind permanently — nothing will ever re-create the + runs dir with those ids in it.""" + orphan = _seed_state_dir(tmp_path, "gone-1") + assert not (tmp_path / ".bmad-loop" / "runs").exists() + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + + +def test_reconcile_orphan_state_dirs_sweeps_nothing_when_the_runs_dir_cannot_be_read( + tmp_path, monkeypatch +): + """The mirror of the test above, and the reason the two failures are told + apart. An unreadable runs dir answers nothing at all — treating it like the + missing one would sweep every control plane this project has, live runs + included, on the strength of a transient permission error. + + The fault is scoped to the runs dir on purpose. A blanket `os.scandir` raise + also takes out the state-root enumeration below it (and `rmtree`), so the + sweep returns `[]` whatever the arm under test does — measured: with the + degradation ablated to `set()` the test still passed, which is the negative + assertion holding for a reason that has nothing to do with the gate.""" + _make_state_run(tmp_path, "live-1") + kept = _seed_state_dir(tmp_path, "live-1") + runs_dir = tmp_path / ".bmad-loop" / "runs" + real_scandir = runs.os.scandir + + def _refuse_only_the_runs_dir(path, *args, **kwargs): + if Path(path) == runs_dir: + raise PermissionError("nope") + return real_scandir(path, *args, **kwargs) + + monkeypatch.setattr(runs.os, "scandir", _refuse_only_the_runs_dir) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_leaves_another_projects_subtree_alone(tmp_path): + """One state root holds every project's control planes, keyed by project + identity. The sweep enumerates its own key's subtree only — a sweep from the + root would let one project's `clean` delete another project's live runs, and + the two need not even be on the same disk.""" + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + (mine / ".bmad-loop" / "runs").mkdir(parents=True) + _make_state_run(theirs, "live-1") + foreign = _seed_state_dir(theirs, "live-1") + # my own orphan, so the sweep provably enumerates rather than finding nothing + orphan = _seed_state_dir(mine, "gone-1") + + assert runs.reconcile_orphan_state_dirs(mine) == [orphan] + + assert not orphan.exists() + assert foreign.is_dir() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_reconcile_orphan_state_dirs_never_removes_through_a_symlink(tmp_path): + """A symlink is not a state dir we created, so it is skipped rather than + followed — and this row points *inside* the root, where the containment test + below it cannot help. + + Reporting it would be a false count even where the removal fails harmlessly + (`rmtree` refuses a symlink): `clean` would claim a sweep that never happened + and go on claiming it every run. On Windows the containment test carries the + case this one cannot: a **junction** reads as a plain directory + (`is_symlink()` is False) but `resolve()` follows it, so without the + containment test `rmtree` would delete the target's contents outside the + root. That row is POSIX-invisible and is not graded here.""" + _make_state_run(tmp_path, "live-1") + target = _seed_state_dir(tmp_path, "live-1") + link = runs.project_state_root(tmp_path) / "ghost-1" + link.symlink_to(target) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert link.is_symlink() # not followed, not removed + assert target.is_dir() + + +@pytest.mark.parametrize( + "attr, exc", + [ + ("state_root", runs.StateRootError("no root")), + ("project_tag", OSError("cannot canonicalize")), + ("project_tag", RuntimeError("Symlink loop from '/p'")), + ], + ids=["no-derivable-state-root", "unresolvable-project", "symlink-loop-project"], +) +def test_reconcile_orphan_state_dirs_degrades_when_the_root_cannot_be_named( + tmp_path, monkeypatch, attr, exc +): + """Reclamation, not repair: a sweep that cannot name its root sweeps nothing + and says so, rather than failing the whole `clean` around it. Leaving disk + behind is the cheap outcome here — the caller's real work (worktrees, trims, + archives) has already been done by the time this runs. + + `RuntimeError` is the below-3.13 spelling of a symlink loop out of + `Path.resolve`, which `project_tag` calls; see the sibling delete test for + the measured version split.""" + monkeypatch.setattr(runs, attr, _raising(exc)) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + +def test_reconcile_orphan_state_dirs_keeps_a_run_that_starts_mid_sweep(tmp_path, monkeypatch): + """`clean` is an operator command with no lock against a run starting, and the + two reads it makes are of different trees. A run creates its run dir strictly + before its state dir (`compose_run` builds the `Journal`, which mkdirs the run + dir, and only then calls `make_adapters`, whose `SignalWatcher` mkdirs the + events dir) — so reading state entries FIRST is what makes the ordering carry + the guarantee: an entry seen there had its run dir on disk even earlier, and + the later `live` read cannot miss it. + + Read the other way round, a run that starts in the gap is absent from `live` + and present in `entries`, and `clean` deletes the control plane of a run that + is starting right now. The cost is not a lost directory — it is the run, which + then polls a primary that no longer exists or never sees its Stop and waits + out `session_timeout_min`. + + The gap is simulated where it actually lives, by starting a run *inside* the + `live` read rather than by patching the sweep: whichever read runs second is + the one that sees `racer`, which is exactly what a real interleaving does. + + Ablation guard: move the `live = _run_dir_names(project)` read back above the + `entries` enumeration and this fails, sweeping `racer` mid-startup.""" + _make_state_run(tmp_path, "live-1") + _seed_state_dir(tmp_path, "live-1") + orphan = _seed_state_dir(tmp_path, "ghost-1") + + real_names = runs._run_dir_names + racer: list[Path] = [] + + def _names_then_a_new_run(project: Path): + names = real_names(project) # the snapshot, taken before `racer` exists + _make_state_run(project, "racer") + racer.append(_seed_state_dir(project, "racer")) + return names + + monkeypatch.setattr(runs, "_run_dir_names", _names_then_a_new_run) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + assert racer[0].is_dir(), "swept the control plane of a run that was starting" + + +def test_reconcile_orphan_state_dirs_skips_an_entry_it_cannot_resolve(tmp_path, monkeypatch): + """The containment test resolves each candidate, so it inherits the same + below-3.13 `RuntimeError` a symlink loop raises — and here it lands *per + entry*, mid-sweep, after earlier entries have already been removed. An + unguarded loop would abort `clean` half-done and report none of what it had + just deleted. + + Skipping is the safe arm rather than sweeping: an entry that cannot be + resolved cannot be proven inside the root, and that proof is the only thing + standing between `rmtree` and a Windows junction's target. + + Ablation guard: drop `RuntimeError` from the containment guard and this + raises instead of returning the resolvable orphan.""" + _make_state_run(tmp_path, "live-1") + _seed_state_dir(tmp_path, "live-1") + good = _seed_state_dir(tmp_path, "ghost-good") + bad = _seed_state_dir(tmp_path, "ghost-loop") + + real_resolve = Path.resolve + + def _resolve(self: Path, *args, **kwargs): + if self == bad: + raise RuntimeError(f"Symlink loop from {str(self)!r}") + return real_resolve(self, *args, **kwargs) + + monkeypatch.setattr(Path, "resolve", _resolve) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [good] + assert not good.exists() and bad.is_dir() diff --git a/tests/test_signals.py b/tests/test_signals.py index 588e5fd5..ce6a7620 100644 --- a/tests/test_signals.py +++ b/tests/test_signals.py @@ -1,10 +1,13 @@ import json +import pytest + from bmad_loop.signals import SignalWatcher def write_event(events_dir, ts, task_id, event, **extra): payload = {"ts": ts, "event": event, "task_id": task_id, **extra} + events_dir.mkdir(parents=True, exist_ok=True) (events_dir / f"{ts}-{task_id}-{event}.json").write_text(json.dumps(payload)) @@ -89,3 +92,90 @@ def sleep(seconds): assert watcher.wait_for("t1", {"Stop"}, timeout_s=10, clock=clock, sleep=sleep) is None assert now["t"] >= 10 + + +# ------------------------------------------------------ dual poll (#494 skew guard) + + +def test_poll_sees_an_event_written_only_to_the_legacy_dir(tmp_path): + """THE version-skew case, and the reason the legacy dir is polled at all. + + The relay a target project runs is a COPY taken at init time, so an upgraded + orchestrator routinely drives sessions whose hook knows only the pre-#494 + in-tree `/events`. Nothing in the new location, everything in the old + one, and the Stop must still be observed — the alternative is that EVERY + session under such a project stalls to `session_timeout_min`. + + Ablation guard: drop `legacy_dir` from `_dirs()` and this fails.""" + watcher = SignalWatcher(tmp_path / "state" / "events", tmp_path / "run" / "events") + write_event(tmp_path / "run" / "events", 1, "t1", "Stop", session_id="legacy") + + event = watcher.wait_for("t1", {"Stop"}, timeout_s=1) + assert event is not None and event.session_id == "legacy" + + +def test_poll_orders_both_dirs_by_ts(tmp_path): + """Ordering is by the event's own `ts`, not by which directory it came from — + a run mid-upgrade could take a SessionStart from one relay and a Stop from + another, and `wait_for`'s buffering hands them out in poll order.""" + primary = tmp_path / "state" / "events" + legacy = tmp_path / "run" / "events" + watcher = SignalWatcher(primary, legacy) + write_event(legacy, 3, "t1", "Stop") + write_event(primary, 2, "t1", "PreCompact") + write_event(legacy, 1, "t1", "SessionStart") + + assert [e.event for e in watcher.poll()] == ["SessionStart", "PreCompact", "Stop"] + + +def test_poll_tolerates_a_missing_legacy_dir(tmp_path): + """The ordinary case once every relay is current: nothing ever creates the + in-tree dir, so it simply is not there. That must not raise — and must not be + papered over by creating it either (see the next test).""" + primary = tmp_path / "state" / "events" + watcher = SignalWatcher(primary, tmp_path / "run" / "events") + write_event(primary, 1, "t1", "Stop", session_id="s1") + + assert [e.session_id for e in watcher.poll()] == ["s1"] + + +def test_only_the_primary_dir_is_created(tmp_path): + """The whole point of #494 is that the run's control plane stops living in the + project tree. An orchestrator that re-created `/events` to poll it + would put the directory back in the operator's `git status` for nothing: a + legacy relay makes it itself, and a current one never writes there.""" + legacy = tmp_path / "run" / "events" + SignalWatcher(tmp_path / "state" / "events", legacy) + + assert (tmp_path / "state" / "events").is_dir() + assert not legacy.exists() + + +def test_the_same_file_name_in_both_dirs_yields_both_events(tmp_path): + """`_consumed` is keyed by (dir, name), so consuming a name from one directory + cannot mask a different event of that name in the other. The names collide on + (ts, task_id, event), which two independent relays can produce; a masked event + here would be a lost Stop. + + Ablation guard: key `_consumed` on `entry.name` alone and this fails.""" + primary = tmp_path / "state" / "events" + legacy = tmp_path / "run" / "events" + watcher = SignalWatcher(primary, legacy) + write_event(primary, 1, "t1", "Stop", session_id="from-primary") + write_event(legacy, 1, "t1", "Stop", session_id="from-legacy") + + assert sorted(e.session_id for e in watcher.poll()) == ["from-legacy", "from-primary"] + assert watcher.poll() == [] # both consumed + + +def test_poll_still_raises_when_the_primary_dir_is_gone(tmp_path): + """The legacy dir's absence is expected; the primary's is not — this watcher + created it, so something removed a live run's control plane out from under it. + Unchanged behavior, pinned here so the tolerance added for the legacy dir is + not quietly widened to both.""" + primary = tmp_path / "state" / "events" + watcher = SignalWatcher(primary, tmp_path / "run" / "events") + primary.rmdir() + + with pytest.raises(OSError): + watcher.poll() diff --git a/tests/test_stories_e2e.py b/tests/test_stories_e2e.py index a2ca2f71..8b81feb4 100644 --- a/tests/test_stories_e2e.py +++ b/tests/test_stories_e2e.py @@ -62,6 +62,7 @@ install_dev_base_skills, ) +from bmad_loop import runs from bmad_loop.install import ( BMAD_SCRIPTS_SEED_REL, CENTRAL_CONFIG_REL, @@ -88,9 +89,10 @@ story="$BMAD_LOOP_STORY_KEY"; folder="$BMAD_LOOP_SPEC_FOLDER" prompt="${1:-}" ts=$(date +%s%N) -mkdir -p "$rd/events" "$rd/tasks/$tid" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" "$rd/tasks/$tid" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" # argv as it ARRIVED, after profile render + tmux quoting — the orchestrator's own # tasks//prompt.txt records the pre-render prompt, so only this file can prove a # dispatched skill NAME actually reached the binary. @@ -109,7 +111,7 @@ > "$tdir/result.json" ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 exit 0 fi @@ -158,7 +160,7 @@ fi ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 exit 0 fi @@ -196,10 +198,16 @@ ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 """ +# The same script as a relay installed BEFORE #494 would be: it knows only the +# in-tree `/events` and ignores the variable the orchestrator now exports. +# Built by swapping the one line that names the directory, so it can differ from +# FAKE_CLI in nothing else. +LEGACY_EVENTS_FAKE_CLI = FAKE_CLI.replace('ed="$BMAD_LOOP_EVENTS_DIR"', 'ed="$rd/events"') + PROFILE_TOML = """\ name = "fakestories" binary = "{binary}" @@ -225,9 +233,10 @@ set -e rd="$BMAD_LOOP_RUN_DIR"; tid="$BMAD_LOOP_TASK_ID" ts=$(date +%s%N) -mkdir -p "$rd/events" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" # Background + wait keeps the same process group as a foreground sleep, but # records the child's pid so the test can prove teardown reaped descendants, # not just this shell (whose cmdline is all the pgrep check can see). @@ -247,9 +256,10 @@ set -e rd="$BMAD_LOOP_RUN_DIR"; tid="$BMAD_LOOP_TASK_ID"; story="$BMAD_LOOP_STORY_KEY" ts=$(date +%s%N) -mkdir -p "$rd/events" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" baseline=$(git rev-parse HEAD) # Detach a straggler into a NEW session (setsid): $! is the setsid'd process itself @@ -269,7 +279,7 @@ ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" # Stay alive like an idle interactive session so the pane shell is still live when # the engine kills it: the harvest sees the detached child as our descendant only # while we (its parent) are still around. Long enough to outlast Stop -> kill_window @@ -1014,6 +1024,41 @@ def test_e2e_sweep_intent_gap_patch_restore(tmp_path): assert any(str(spec) in p for p in prompts) +def test_e2e_a_relay_that_only_knows_the_legacy_events_dir_still_completes(tmp_path): + """The #494 version-skew guard, through the real CLI and real tmux. + + `bmad_loop_hook.py` is COPIED into the target project by `init`, so a project + that upgraded bmad-loop without re-initing runs a relay that has never heard of + BMAD_LOOP_EVENTS_DIR and writes only to `/events`. The orchestrator + exports the variable and waits on the out-of-tree channel regardless — so + without the watcher's second poll, that pairing observes no Stop at all and + EVERY session in the project stalls to `session_timeout_min`. It would not fail + a test suite; it would hang a user's overnight run. + + Asserted on the outcome (the story reaches `done` and commits), plus the + premise: the events really did land only in the legacy location, so the + completion cannot have come through the primary channel. + + Ablation guard: drop `legacy_dir` from `SignalWatcher._dirs()` and this fails — + slowly, as the session timeout, which is exactly the production symptom.""" + assert "$BMAD_LOOP_EVENTS_DIR" not in LEGACY_EVENTS_FAKE_CLI, "the twin still reads the new var" + assert LEGACY_EVENTS_FAKE_CLI != FAKE_CLI, "the swap did not take" + + root = tmp_path / "sbx" + story = "1-1-thing" + _scaffold_sprint(root, story, fake_cli=LEGACY_EVENTS_FAKE_CLI) + base = _commit_count(root) + + proc = _run(root, "run") + assert proc.returncode == 0, proc.stderr or proc.stdout + assert _sprint_status(root, story) == "done" + assert _commit_count(root) == base + 1 + + run_id = _run_id(root) + assert list((root / ".bmad-loop" / "runs" / run_id / "events").glob("*.json")) + assert not list(runs.events_dir_for(root, run_id).glob("*.json")) + + def test_e2e_sprint_mode_regression(tmp_path): # Scenario 6 (audit MAJOR-2): the new folder+id-capable bmad-dev-auto skill is # installed, but this is a plain SPRINT-mode run. It must drive dev → verify →