diff --git a/.gitignore b/.gitignore index 8601b2a..239d598 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,6 @@ wheels/ graphify_output/ .understand-anything/ graphify-out/ + +# Project-local Bach task artifacts (the repo dogfoods itself) +.bach/ diff --git a/AGENTS.md b/AGENTS.md index 4d82874..f499b5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,11 +17,17 @@ Follow the same constraints as `CLAUDE.md`. The short version: - `bach sessions adopt [--force]` — link a discovered session ID to a Bach task artifact - `bach sessions resume ` — resume a session in iTerm (prints fallback command if iTerm unavailable) - `bach sessions watch ` — run the foreground session observer (polls transcript, calls judge, applies status via observer authority) -- `bach hooks install claude-code [--project-dir PATH]` — idempotent Claude Code hook install (SessionEnd + Stop + PostToolUse → `bach internal hook-event` / `bach internal session-stop`) -- `bach hooks install codex [--codex-home PATH]` — idempotent Codex hook install (sessionStop → `bach internal hook-event`) -- `bach hooks status [--project-dir PATH]` — show which runtimes have Bach hooks installed +- `bach hooks install claude-code [--project ] [--project-dir PATH]` — idempotent Claude Code hook install (SessionEnd + Stop + PostToolUse → `bach internal hook-event` / `bach internal session-stop`); `--project` resolves a registered project name to its filesystem path via ProjectRegistry +- `bach hooks install codex [--project ] [--codex-home PATH]` — idempotent Codex hook install (sessionStop → `bach internal hook-event`); `--project` resolves a registered project name +- `bach hooks status [--project ] [--project-dir PATH]` — show which runtimes have Bach hooks installed; `--project` resolves a registered project name - `bach afk run ` — headless multi-turn task runner; claims `afk_queue` → `executing`, drives turns, never sets `done` +## PRD#8 Commands (Board × Radar fusion — observer enablement) + +- `bach config set ` — set an observer knob in `~/.bach/config.yaml`; allowlisted keys (hyphenated): `watch-on-launch`, `observer-moves`, `judge-confidence-threshold`, `judge-interval-seconds`, `liveness-running-seconds`, `liveness-idle-minutes`, `afk-max-turns`, `afk-time-budget-minutes`; bools accept `on`/`off`/`true`/`false`/`yes`/`no` +- Board `/board` (REPL) — kanban cards in `executing`/`qa` columns now show liveness dot prefix (● running, ◐ idle, ✓ ended), truncated observer summary line, and `⚑ needs you` marker when observer flags needs_human +- Show `/show ` (REPL) — task detail appends an `── Observer ──` section (summary, confidence %, needs_human, session state) when the observer has produced a verdict + # GitNexus — Code Intelligence diff --git a/CLAUDE.md b/CLAUDE.md index 373c1aa..89e8336 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,11 +24,16 @@ Bach is a CLI-first personal agentic development session launcher that turns dai - `uv run bach sessions adopt [--force]` - Link a discovered session to a task artifact - `uv run bach sessions resume ` - Resume a session via iTerm (fallback command printed on failure) - `uv run bach sessions watch ` - Run the foreground session watcher (observer mode) -- `uv run bach hooks install claude-code [--project-dir PATH]` - Idempotent install of Claude Code hooks (SessionEnd/Stop/PostToolUse) -- `uv run bach hooks install codex [--codex-home PATH]` - Idempotent install of Codex hooks (sessionStop) -- `uv run bach hooks status [--project-dir PATH]` - Report which runtimes have Bach hooks installed +- `uv run bach hooks install claude-code [--project ] [--project-dir PATH]` - Idempotent install of Claude Code hooks (SessionEnd/Stop/PostToolUse); `--project` resolves a registered project name to its path +- `uv run bach hooks install codex [--project ] [--codex-home PATH]` - Idempotent install of Codex hooks (sessionStop); `--project` resolves a registered project name +- `uv run bach hooks status [--project ] [--project-dir PATH]` - Report which runtimes have Bach hooks installed; `--project` resolves a registered project name - `uv run bach afk run ` - Run a task headlessly (AFK mode) from `afk_queue` +### PRD#8 — Board × Radar fusion (observer enablement) +- `uv run bach config set ` - Set an observer knob (e.g. `watch-on-launch on`); allowlisted keys: `watch-on-launch`, `observer-moves`, `judge-confidence-threshold`, `judge-interval-seconds`, `liveness-running-seconds`, `liveness-idle-minutes`, `afk-max-turns`, `afk-time-budget-minutes` +- `/board` (REPL) - Kanban board now shows liveness dots (●/◐/✓) and observer summary lines on executing/qa cards; `⚑ needs you` marker when observer flags needs_human +- `/show ` (REPL) - Task detail now includes an Observer section (summary, confidence, needs_human, session state) when the observer has data + ## File References - `docs/internal/DISCOVERY.md` - real goal and scope - `docs/internal/PRD.md` - requirements diff --git a/docs/how-to/enable-observer.md b/docs/how-to/enable-observer.md new file mode 100644 index 0000000..3255bed --- /dev/null +++ b/docs/how-to/enable-observer.md @@ -0,0 +1,180 @@ +# Enable the Observer (Board × Radar fusion) + +Set up Bach's session observer in about 2 minutes: hooks for both runtimes, +one config knob, and you'll see live liveness dots on the `/board` kanban. + +--- + +## Prerequisites + +- Bach installed and `uv run bach --help` works. +- At least one project registered (`bach project add `). +- Claude Code and/or Codex installed. + +--- + +## Step-by-step + +### 1. Install hooks for both runtimes + +Run one command per runtime, targeting your project by name: + +```bash +# Claude Code — installs SessionEnd + Stop + PostToolUse hooks +# into /.claude/settings.json +bach hooks install claude-code --project + +# Codex — installs sessionStop hook into ~/.codex/hooks.json +bach hooks install codex --project +``` + +The `--project ` flag resolves to the project's registered path via +`ProjectRegistry`. Without the flag, the current working directory is used +(same as before Phase 17 — backward compatible). + +Both installs are **idempotent** — re-running never duplicates entries and +never removes unrelated user hooks. Every change made is printed to stdout. + +Verify the install: + +```bash +bach hooks status --project +``` + +You should see a `✓` next to both `claude-code` and `codex`. + +--- + +### 2. Enable watch-on-launch + +Tell Bach to automatically spawn a background session watcher every time you +launch a task: + +```bash +bach config set watch-on-launch on +``` + +Confirm the setting was written: + +```bash +bach config show +``` + +Look for `Watch on launch: True` in the output. + +**Other observer knobs you can tune** (all via `bach config set `): + +| Key | Default | Accepts | Effect | +|-----|---------|---------|--------| +| `watch-on-launch` | `off` | `on`/`off` | Auto-spawn watcher on task launch | +| `observer-moves` | `on` | `on`/`off` | Let observer write status changes (off = log-only) | +| `judge-confidence-threshold` | `0.7` | float 0–1 | Minimum confidence before a verdict is applied | +| `judge-interval-seconds` | `300` | positive int | Minimum gap between mid-session judge calls | +| `liveness-running-seconds` | `60` | positive int | Transcript mtime within N seconds → running | +| `liveness-idle-minutes` | `5` | positive int | Process alive but transcript stale N min → idle | +| `afk-max-turns` | `10` | positive int | AFK hard turn budget | +| `afk-time-budget-minutes` | `60` | positive int | AFK hard time budget | + +--- + +### 3. Launch a task + +```bash +# From the REPL — stable-ID reference (e.g. t12) +bach +bach > /open t12 + +# Or directly from the CLI +bach task launch +``` + +When `watch-on-launch` is on, Bach prints a confirmation line immediately +after spawning the watcher: + +``` +Observer watching this session (watch_on_launch=on). +``` + +If the watcher fails to spawn (process table issue, bad PATH, etc.) Bach logs +a WARNING but the task launch continues — the watcher is bonus, not blocking. + +--- + +### 4. What you'll see on the board + +Open the kanban board: + +```bash +bach +bach > /board +# or +bach > /b +``` + +Each task card in the `executing` and `qa` columns gains a **liveness dot** +in the top-left corner: + +| Dot | Color | Meaning | +|-----|-------|---------| +| `●` | green | Session is **running** (transcript updated recently) | +| `◐` | yellow | Session is **idle** (process alive but transcript stale) | +| `✓` | dim | Session has **ended** | +| _(none)_ | — | No session linked yet | + +When the session watcher has produced an observer verdict, a truncated +**summary line** appears below the task title in the card (executing and qa +columns only). + +If the observer flagged `needs_human`, the card shows: + +``` +⚑ needs you +``` + +in red — meaning the agent is blocked and needs your input. + +--- + +### 5. Task detail (Observer section) + +Open a task's detail view in the REPL: + +```bash +bach > /show t12 +``` + +When the observer has data, a dedicated `── Observer ──` section appears +after the artifact text: + +``` +── Observer ── + Summary: Tests passing, waiting on type errors in module X + Confidence: 82% + ⚑ needs you + Session: idle +``` + +This section only appears when the observer has written at least one verdict +(`observer_suggestion` or `observer_status_written` event in the artifact log). + +--- + +## Troubleshooting + +| Symptom | Check | +|---------|-------| +| No dots on the board | Run `bach hooks status --project ` — both hooks must show `✓` | +| No "Observer watching" line after launch | Confirm `Watch on launch: True` in `bach config show` | +| `✓` dot but no summary line | The watcher ran but found no observer events yet; wait for the next judge cycle | +| Observer moved status unexpectedly | Set `observer-moves off` to make the observer log-only while you audit | +| No `⚑ needs you` even when agent is blocked | The judge confidence was below threshold — lower `judge-confidence-threshold` | + +--- + +## Related docs + +- `wire-up-session-hook.md` — full hook reference, manual setup, loop-gate +- `configure-user-settings.md` — all user config knobs +- `use-the-repl.md` — board view, /show, /open, /done commands +- `../adr/015-session-observability-codex-parity.md` — observer architecture +- `../adr/016-loop-control.md` — loop gate and AFK bounds diff --git a/docs/internal/progress.txt b/docs/internal/progress.txt index ccdf461..034e103 100644 --- a/docs/internal/progress.txt +++ b/docs/internal/progress.txt @@ -4,6 +4,19 @@ - Repo scaffolded; core docs created; Python CLI skeleton; basic project/task services in place. +- **PRD#8 (Board × Radar fusion) complete** (board liveness join: + `board_liveness.py` with `TaskLiveness` frozen dataclass + `LivenessMap` + + TTL cache + stale-on-failure fallback; observer knob config (`bach + config set `) with 8 allowlisted keys in `settings.py`; + `--project ` flag on `hooks install` and `hooks status` resolves + registered project names to paths via `ProjectRegistry`; board + rendering gains liveness dot prefix ●/◐/✓ per `SessionLiveness`, + truncated observer summary line, `⚑ needs you` marker; `/show` task + detail appends Observer section; watcher spawn confirmation line when + `watch_on_launch=on`; board legend updated in `help_content.py`; + `docs/how-to/enable-observer.md` documents the 2-minute enablement + path; CLAUDE.md + AGENTS.md command lists updated; 902 baseline + tests preserved). - **Phase 16 complete** (session observability & loop control: `DiscoveredSession`, `SessionEvent`, `JudgeVerdict` domain types; `session_discovery.py` scans Claude Code + Codex transcript directories with mtime-based liveness; diff --git a/src/bach/cli/app.py b/src/bach/cli/app.py index 6aa6def..24576d3 100644 --- a/src/bach/cli/app.py +++ b/src/bach/cli/app.py @@ -13,7 +13,12 @@ from bach.cli.sessions import sessions_app from bach.config.paths import bach_home from bach.config.projects import ProjectRegistry -from bach.config.settings import is_valid_user_name, load_config, write_config_field +from bach.config.settings import ( + is_valid_user_name, + load_config, + set_observer_key, + write_config_field, +) from bach.config.themes import THEMES, ThemePalette, get_theme, list_themes from bach.domain.models import AgentRuntime, SessionMode from bach.logging_setup import configure_logging @@ -449,6 +454,15 @@ def config_show() -> None: console.print(f"[bold]Normalize model:[/bold] {config.normalize_model}") console.print(f"[bold]Theme:[/bold] {config.theme}") console.print(f"[bold]User name:[/bold] {config.user_name}") + # Phase 17 — observer knobs (8 allowlisted keys from PRD#8) + console.print(f"[bold]Watch on launch:[/bold] {config.watch_on_launch}") + console.print(f"[bold]Observer moves:[/bold] {config.observer_moves}") + console.print(f"[bold]Judge confidence:[/bold] {config.judge_confidence_threshold}") + console.print(f"[bold]Judge interval s:[/bold] {config.judge_interval_seconds}") + console.print(f"[bold]Liveness run s:[/bold] {config.liveness_running_seconds}") + console.print(f"[bold]Liveness idle m:[/bold] {config.liveness_idle_minutes}") + console.print(f"[bold]AFK max turns:[/bold] {config.afk_max_turns}") + console.print(f"[bold]AFK budget min:[/bold] {config.afk_time_budget_minutes}") # Soft-warn if the configured model is not in the current codex catalog. # We never raise here — stale config must not block `bach config show`, @@ -567,6 +581,35 @@ def config_set_name( console.print(f"[{t.success}]Set user_name = {stripped}[/{t.success}]") +@config_app.command("set") +def config_set( + key: Annotated[ + str, + typer.Argument(help="Observer knob to set (e.g. watch-on-launch)."), + ], + value: Annotated[ + str, + typer.Argument(help="Value to assign. Bools: on/off/true/false/yes/no."), + ], +) -> None: + """Set an observer knob in ~/.bach/config.yaml. + + Allowlisted keys (hyphenated CLI spelling): + watch-on-launch, observer-moves, judge-confidence-threshold, + judge-interval-seconds, liveness-running-seconds, + liveness-idle-minutes, afk-max-turns, afk-time-budget-minutes + + Use `bach config show` to inspect current values. + """ + t = _cli_theme() + try: + path = set_observer_key(key, value) + except ValueError as exc: + console.print(f"[{t.error}]{exc}[/{t.error}]") + raise typer.Exit(code=1) from None + console.print(f"[{t.success}]Set {key} = {value}[/{t.success}] ({path})") + + @config_app.command("list-themes") def config_list_themes() -> None: """List all built-in color themes. diff --git a/src/bach/cli/hooks_cmd.py b/src/bach/cli/hooks_cmd.py index 6fc28b9..9986834 100644 --- a/src/bach/cli/hooks_cmd.py +++ b/src/bach/cli/hooks_cmd.py @@ -24,6 +24,7 @@ import typer from rich.console import Console +from bach.config.projects import ProjectRegistry from bach.config.settings import load_config from bach.config.themes import THEMES, ThemePalette, get_theme from bach.services.hooks_service import ( @@ -47,6 +48,53 @@ def _theme() -> ThemePalette: # --------------------------------------------------------------------------- +def _resolve_project_dir( + project_name: str | None, + project_dir: Path | None, + t: ThemePalette, +) -> Path | None: + """Resolve --project to a filesystem path via ProjectRegistry. + + If --project is given, look up the name in the registry: + - known → return project.path + - unknown → print error listing registered names, raise typer.Exit(1) + If --project is omitted, return project_dir as-is (may be None → caller + falls back to cwd). + + Keeping resolution here (not in the service layer) is intentional: the + service layer only knows about paths, not about registry names — that is + a CLI / UX concern. + """ + if project_name is None: + # No --project flag → caller decides the fallback (cwd or --project-dir). + return project_dir + + registry = ProjectRegistry.default() + try: + project = registry.get(project_name) + logger.info( + "event=hooks_project_resolved name=%s path=%s", + project_name, + project.path, + ) + return project.path + except KeyError: + # List registered names to help the user recover quickly. + data = registry.load() + registered = sorted(p.name for p in data.projects.values()) + names_str = ", ".join(registered) if registered else "(none registered)" + _console.print( + f"[{t.error}]Unknown project:[/{t.error}] {project_name!r}. " + f"Registered projects: {names_str}" + ) + logger.warning( + "event=hooks_unknown_project name=%s registered=%s", + project_name, + names_str, + ) + raise typer.Exit(code=1) from None + + @hooks_app.command("install") def hooks_install( runtime: Annotated[ @@ -70,6 +118,14 @@ def hooks_install( help="Codex home directory (default: ~/.codex). Only used for codex installs.", ), ] = None, + project: Annotated[ + str | None, + typer.Option( + "--project", + help="Registered project name or key. Resolves to the project's path. " + "Overrides --project-dir when both are given.", + ), + ] = None, ) -> None: """Install Bach hooks for the specified runtime. @@ -86,7 +142,9 @@ def hooks_install( normalized = runtime.lower().strip() if normalized in ("claude-code", "claude"): - target_dir = project_dir or Path.cwd() + # --project resolves to a registered path; falls through to --project-dir / cwd. + resolved_dir = _resolve_project_dir(project, project_dir, t) + target_dir = resolved_dir or Path.cwd() result = install_claude_hooks(project_dir=target_dir) if result.added: for event_name, cmd in result.added: @@ -143,6 +201,14 @@ def hooks_status( help="Codex home directory to check (default: ~/.codex).", ), ] = None, + project: Annotated[ + str | None, + typer.Option( + "--project", + help="Registered project name or key. Resolves to the project's path. " + "Overrides --project-dir when both are given.", + ), + ] = None, ) -> None: """Report which runtimes have Bach hooks installed. @@ -150,7 +216,9 @@ def hooks_status( Claude Code hooks, and ~/.codex/hooks.json for Codex hooks. """ t = _theme() - target_dir = project_dir or Path.cwd() + # --project resolves to a registered path; falls through to --project-dir / cwd. + resolved_dir = _resolve_project_dir(project, project_dir, t) + target_dir = resolved_dir or Path.cwd() target_codex_home = codex_home or (Path.home() / ".codex") status = get_hooks_status( diff --git a/src/bach/cli/sessions.py b/src/bach/cli/sessions.py index 1ac7216..7e88060 100644 --- a/src/bach/cli/sessions.py +++ b/src/bach/cli/sessions.py @@ -27,6 +27,7 @@ from rich.table import Table from bach.config.paths import bach_home +from bach.config.projects import ProjectRegistry from bach.config.settings import load_config from bach.config.themes import THEMES, ThemePalette, get_theme from bach.domain.models import DiscoveredSession @@ -46,7 +47,7 @@ find_artifact_for_ref, list_sessions, resume_session, - scan_artifact_paths, + scan_registry_artifact_paths, ) logger = logging.getLogger(__name__) @@ -80,9 +81,14 @@ def _default_discover_fn() -> list[DiscoveredSession]: ) -def _default_artifacts_dir() -> Path: - """Return the bach home directory for artifact scanning.""" - return bach_home() +def _default_artifact_paths() -> list[Path]: + """Return artifact paths from every registered project's .bach/ dir. + + Per ADR-002, task artifacts are project-local (/.bach/runs/…). + Scanning bach_home() alone never finds them — we must iterate the registry. + """ + registry = ProjectRegistry.default() + return scan_registry_artifact_paths(registry) # --------------------------------------------------------------------------- @@ -118,14 +124,19 @@ def sessions_list( try: rows = list_sessions( - sessions_dir=_default_artifacts_dir() / "sessions", - artifacts_dir=_default_artifacts_dir(), + sessions_dir=bach_home() / "sessions", + # artifacts_dir is unused when artifact_paths is supplied; pass + # a sentinel so the signature stays satisfied without scanning + # the wrong directory. + artifacts_dir=bach_home(), discover_fn=_default_discover_fn, read_events_fn=read_tail_events, project=project, runtime=runtime, state=state, max_ended_age_s=None if show_all else DEFAULT_ENDED_WINDOW_S, + # Inject project-local paths per ADR-002 so the Task column links. + artifact_paths=_default_artifact_paths(), ) except Exception as exc: _console.print(f"[{t.error}]error[/{t.error}] {exc}") @@ -238,7 +249,8 @@ def sessions_adopt( a live session unless --force is given. """ t = _theme() - artifact_paths = scan_artifact_paths(_default_artifacts_dir()) + # Per ADR-002: artifacts live under each project's .bach/ dir, not bach_home(). + artifact_paths = _default_artifact_paths() try: artifact = adopt_session( @@ -273,7 +285,8 @@ def sessions_resume( """ t = _theme() cfg = load_config() - artifact_paths = scan_artifact_paths(_default_artifacts_dir()) + # Per ADR-002: scan project-local .bach dirs, not bach_home(). + artifact_paths = _default_artifact_paths() from bach.runtimes.iterm import launch_in_iterm # noqa: PLC0415 @@ -325,7 +338,8 @@ def sessions_watch( # Resolve the artifact path via the public service function so the CLI # handler stays thin and does not reach into private service internals. - artifact_paths = scan_artifact_paths(_default_artifacts_dir()) + # Per ADR-002: scan project-local .bach dirs, not bach_home(). + artifact_paths = _default_artifact_paths() artifact: Path | None = find_artifact_for_ref(task_or_session, artifact_paths) if artifact is None: diff --git a/src/bach/config/settings.py b/src/bach/config/settings.py index 03020d5..8d91ad8 100644 --- a/src/bach/config/settings.py +++ b/src/bach/config/settings.py @@ -20,6 +20,7 @@ """ import logging +from collections.abc import Callable from dataclasses import dataclass from enum import StrEnum from pathlib import Path @@ -447,3 +448,139 @@ def _coerce_positive_int(value: Any, field: str, *, default: int) -> int: ) return default return value + + +# --------------------------------------------------------------------------- +# A2 — `bach config set` observer knob allowlist (PRD#8, Board × Radar fusion) +# --------------------------------------------------------------------------- + +# Parsers/validators for the CLI string input. Each callable receives the +# raw string from `bach config set ` and must return the typed +# Python value to persist in the YAML file, or raise ValueError with a +# human-readable message on bad input. +# Keys use the CLI-spelling (hyphenated); field names use underscore form. + + +def _parse_bool_flag(raw: str) -> bool: + """Accept on/off/true/false/yes/no (case-insensitive) → Python bool. + + Only these 6 forms are accepted. Bare integers (0/1) or arbitrary + strings are rejected so the user gets a clear error rather than + silently mis-setting the knob. + """ + normalised = raw.strip().lower() + if normalised in ("on", "true", "yes"): + return True + if normalised in ("off", "false", "no"): + return False + raise ValueError( + f"Invalid boolean value {raw!r}. Accepted: on/off/true/false/yes/no (case-insensitive)." + ) + + +def _parse_confidence(raw: str) -> float: + """Accept a float in [0.0, 1.0]; raise ValueError otherwise.""" + try: + val = float(raw.strip()) + except ValueError: + raise ValueError( + f"Invalid float value {raw!r} for judge-confidence-threshold. " + "Expected a number in [0.0, 1.0]." + ) from None + if val < 0.0 or val > 1.0: + raise ValueError( + f"Value {val!r} is outside the valid range [0.0, 1.0] for judge-confidence-threshold." + ) + return val + + +def _parse_positive_int(raw: str) -> int: + """Accept a positive (>0) integer string; raise ValueError otherwise.""" + try: + val = int(raw.strip()) + except ValueError: + raise ValueError(f"Invalid integer value {raw!r}. Expected a positive integer.") from None + if val < 1: + raise ValueError(f"Value {val!r} is not positive. Must be >= 1.") + return val + + +# CLI-spelling (hyphen) → (parser, YAML field name) +# The parsers raise ValueError with a human message on bad input so that +# set_observer_key can re-raise cleanly without wrapping. +OBSERVER_SETTABLE_KEYS: dict[str, Callable[[str], Any]] = { + "watch-on-launch": _parse_bool_flag, + "observer-moves": _parse_bool_flag, + "judge-confidence-threshold": _parse_confidence, + "judge-interval-seconds": _parse_positive_int, + "liveness-running-seconds": _parse_positive_int, + "liveness-idle-minutes": _parse_positive_int, + "afk-max-turns": _parse_positive_int, + "afk-time-budget-minutes": _parse_positive_int, +} + +# Map CLI-spelling (hyphen) → YAML/dataclass field name (underscore). +# Kept separate so OBSERVER_SETTABLE_KEYS stays type-clean (only parsers). +_OBSERVER_KEY_TO_FIELD: dict[str, str] = {k: k.replace("-", "_") for k in OBSERVER_SETTABLE_KEYS} + + +def set_observer_key(key: str, raw_value: str) -> Path: + """Validate `raw_value` for `key`, write to config, return the path written. + + Raises ValueError with a human message for: + - unknown key (lists sorted valid keys) + - invalid value (parser raises with a human message) + - cross-field invariant violation: liveness_running_seconds must be + strictly less than liveness_idle_minutes * 60. If the invariant is + broken, the board's "◐ idle" dot becomes unreachable dead code + (running threshold would exceed the idle threshold, so a session + jumps from running to ended without passing through idle). + + Writes via write_config_field so every field-set goes through the + same code path (logging, mkdir, yaml round-trip). + """ + if key not in OBSERVER_SETTABLE_KEYS: + valid = ", ".join(sorted(OBSERVER_SETTABLE_KEYS.keys())) + raise ValueError(f"Unknown observer key {key!r}. Valid keys: {valid}.") + parser = OBSERVER_SETTABLE_KEYS[key] + # Raises ValueError with a human message if the raw value is invalid. + typed_value = parser(raw_value) + + # Cross-field validation for liveness thresholds. Read the OTHER field's + # current value from config so we can enforce the invariant: + # running_seconds < idle_minutes * 60 + # Without this guard, idle state becomes unreachable and the board's ◐ + # signal silently disappears. + if key in ("liveness-running-seconds", "liveness-idle-minutes"): + current_cfg = load_config() + if key == "liveness-running-seconds": + new_running_s: int = int(typed_value) + idle_minutes = current_cfg.liveness_idle_minutes + idle_s = idle_minutes * 60 + if new_running_s >= idle_s: + raise ValueError( + f"liveness-running-seconds ({new_running_s}s) must be less than " + f"liveness-idle-minutes * 60 ({idle_minutes} min × 60 = {idle_s}s). " + "Otherwise idle state is unreachable and the ◐ board signal vanishes." + ) + else: # liveness-idle-minutes + new_idle_minutes: int = int(typed_value) + running_s = current_cfg.liveness_running_seconds + idle_s = new_idle_minutes * 60 + if running_s >= idle_s: + raise ValueError( + f"liveness-idle-minutes ({new_idle_minutes} min × 60 = {idle_s}s) would be " + f"≤ liveness-running-seconds ({running_s}s). " + "Otherwise idle state is unreachable and the ◐ board signal vanishes." + ) + + field_name = _OBSERVER_KEY_TO_FIELD[key] + path = write_config_field(field_name, typed_value) + logger.info( + "event=observer_key_set key=%s field=%s value=%r path=%s", + key, + field_name, + typed_value, + path, + ) + return path diff --git a/src/bach/repl/app.py b/src/bach/repl/app.py index c6d2058..2728172 100644 --- a/src/bach/repl/app.py +++ b/src/bach/repl/app.py @@ -38,6 +38,7 @@ import difflib import logging +import re import subprocess from collections.abc import Callable from datetime import datetime @@ -54,7 +55,11 @@ from bach.config.settings import load_config, write_config_field from bach.config.themes import THEMES, ThemePalette, get_theme from bach.domain.models import AgentRuntime -from bach.repl.board import cmd_board, render_kanban_board # Phase 11 wiring +from bach.repl.board import ( # Phase 11 + Phase 16 B1 wiring + _liveness_dot_prefix, + cmd_board, + render_kanban_board, +) from bach.repl.help_content import ( CATEGORY_LABELS, COMMAND_HELP, @@ -70,6 +75,7 @@ ) from bach.repl.sessions_view import cmd_sessions # Phase 16 wiring from bach.runtimes.codex_discovery import list_codex_models +from bach.services.board_liveness import liveness_for_tasks # Phase 16 B1 from bach.services.daily_index_service import ( DailyTask, discover_all_tasks, @@ -319,9 +325,31 @@ def _format_skip_banner(skipped: dict[str, str], warning_color: str) -> str: Example expected output (what the user picked in the design Q): ⚠ Skipped 1 project: skillia (permission denied) """ - # TODO(jordi): implement banner formatting per the decisions above. - # 5-10 lines. Return "" when skipped is empty. - raise NotImplementedError("Banner format pending UX decision") + if not skipped: + return "" + + # Translate OSError class names to human-readable reasons. + # PermissionError is the overwhelmingly common case (macOS TCC denials). + # Any other class name is split on CamelCase word boundaries, lowercased, + # and joined with spaces for a sensible default: + # "FileNotFoundError" → "file not found error" + # "OSError" → "os error" + def _reason(class_name: str) -> str: + if class_name == "PermissionError": + return "permission denied" + # Split on CamelCase word boundaries so the result reads as prose. + # Two passes: lower→upper ("FileNot" → "File Not") then + # ALL_CAPS run followed by TitleCase ("OSError" → "OS Error"). + words = re.sub(r"([a-z])([A-Z])", r"\1 \2", class_name) + words = re.sub(r"([A-Z]+)([A-Z][a-z])", r"\1 \2", words) + return words.replace("_", " ").lower() + + count = len(skipped) + noun = "project" if count == 1 else "projects" + # List every skipped project — registries are small (~10 typical). + project_list = ", ".join(f"{name} ({_reason(cls_name)})" for name, cls_name in skipped.items()) + line = f"⚠ Skipped {count} {noun}: {project_list}" + return f"[{warning_color}]{line}[/{warning_color}]" def _render_dashboard(app: BachRepl) -> None: @@ -363,6 +391,9 @@ def _render_date_tasks_table(app: BachRepl, date_str: str) -> None: Columns: # (positional, view-relative) · ID (stable, global) · Status · Task · Project. The two reference columns coexist by design — see Phase 9 in ARCHITECTURE.md. + + Phase 16 (B1): fetches a LivenessMap and prepends a dot to each Task + cell. Minimal lines added here — liveness dot logic lives in board.py. """ tasks = discover_tasks_for_date(date_str, app.registry) if not tasks: @@ -373,6 +404,12 @@ def _render_date_tasks_table(app: BachRepl, date_str: str) -> None: ) return theme = app.current_theme + # Phase 16: fetch liveness so dots can prefix task rows. + live_map = liveness_for_tasks( + tasks, + registry=app.registry, + read_frontmatter_fn=TaskArtifactStore.read_frontmatter_at, + ) table = Table( title=f"Bach — Daily Run {date_str} ({len(tasks)} tasks)", title_style="bold", @@ -386,11 +423,14 @@ def _render_date_tasks_table(app: BachRepl, date_str: str) -> None: table.add_column("Project", style=theme.project) for i, task in enumerate(tasks, start=1): color = theme.status_color(task.status) + # Prepend liveness dot when available — reuses board.py's helper. + live = live_map.get(task.task_id) if task.task_id else None + dot = _liveness_dot_prefix(live.liveness if live else None, theme) table.add_row( str(i), _format_task_id(task.task_id), f"[{color}]{task.status}[/{color}]", - _format_title_with_hierarchy(task, theme), + f"{dot}{_format_title_with_hierarchy(task, theme)}", task.project_name, ) app.console.print(table) @@ -403,6 +443,9 @@ def _render_all_tasks_table(app: BachRepl) -> None: newest-first (delegated to the discoverer). Index 1..N matches the table order; `` in any subsequent command uses that index. Stable IDs (`tN`) work here too — they're view-independent. + + Phase 16 (B1): fetches a LivenessMap and prepends a dot to Task rows. + Minimal lines — reuses _liveness_dot_prefix from board.py. """ tasks = discover_all_tasks(app.registry) if not tasks: @@ -413,6 +456,12 @@ def _render_all_tasks_table(app: BachRepl) -> None: ) return theme = app.current_theme + # Phase 16: fetch liveness so dots can prefix task rows. + live_map = liveness_for_tasks( + tasks, + registry=app.registry, + read_frontmatter_fn=TaskArtifactStore.read_frontmatter_at, + ) table = Table( title=f"Bach — All Tasks ({len(tasks)} total)", title_style="bold", @@ -427,12 +476,16 @@ def _render_all_tasks_table(app: BachRepl) -> None: table.add_column("Project", style=theme.project) for i, task in enumerate(tasks, start=1): color = theme.status_color(task.status) + # Prepend liveness dot when available — _liveness_dot_prefix is + # imported at module level (line ~58). + live = live_map.get(task.task_id) if task.task_id else None + dot = _liveness_dot_prefix(live.liveness if live else None, theme) table.add_row( str(i), _format_task_id(task.task_id), task.date, f"[{color}]{task.status}[/{color}]", - _format_title_with_hierarchy(task, theme), + f"{dot}{_format_title_with_hierarchy(task, theme)}", task.project_name, ) app.console.print(table) @@ -838,6 +891,10 @@ def _cmd_show(app: BachRepl, arg_str: str) -> None: Cap output at 4000 chars so a huge grill transcript doesn't drown the rest of the REPL output. User can open the file directly for full content. + + Phase 16 (B1): appends an Observer section when the task has a + liveness entry with a summary. Reads from the LivenessMap so the + renderer never touches discovery/artifact internals directly. """ task = _resolve_task(app, arg_str) if task is None: @@ -848,6 +905,38 @@ def _cmd_show(app: BachRepl, arg_str: str) -> None: app.console.print(f"[bold]── {task.artifact_path} ──[/bold]") app.console.print(text) + # --- Observer section (Phase 16 B1) ------------------------------------- + # Only render when the task has a stable ID and the liveness map has + # a summary to show. Failures in liveness_for_tasks return {} (never + # raises), so this section is always safe to call. + if not task.task_id: + return + # Use the same task list the dashboard just rendered so liveness_for_tasks + # hits the shared N-task cache key. A singleton [task] call would produce + # a frozenset({task_id}) that never matches the dashboard's key, forcing a + # fresh `ps` subprocess on every /show. Resolve via the active view. + if app.current_view == "all": + view_tasks = discover_all_tasks(app.registry) + else: + date_str = app.current_date_str or datetime.now().strftime("%Y-%m-%d") + view_tasks = discover_tasks_for_date(date_str, app.registry) + live_map = liveness_for_tasks( + view_tasks, + registry=app.registry, + read_frontmatter_fn=TaskArtifactStore.read_frontmatter_at, + ) + live = live_map.get(task.task_id) + if live and live.summary: + t = app.current_theme + app.console.print("\n[bold]── Observer ──[/bold]") + app.console.print(f" Summary: {live.summary}") + if live.confidence is not None: + app.console.print(f" Confidence: {live.confidence:.0%}") + if live.needs_human: + app.console.print(f" [{t.error}]⚑ needs you[/{t.error}]") + if live.liveness is not None: + app.console.print(f" [{t.muted}]Session: {live.liveness.value}[/{t.muted}]") + # --- PROJECTS --------------------------------------------------------------- diff --git a/src/bach/repl/board.py b/src/bach/repl/board.py index 92c8c20..248edb9 100644 --- a/src/bach/repl/board.py +++ b/src/bach/repl/board.py @@ -15,17 +15,30 @@ place to grow. What this module exposes (consumed by `repl/app.py`): - - `cmd_board(app, arg_str)` — the slash-command handler - - `render_kanban_board(app)` — the dispatch-time renderer - - `parse_board_args(...)` — exposed for unit tests - - `truncate_title(...)` — exposed for unit tests - - `render_task_card(...)` — exposed for unit tests - - `build_kanban_table(...)` — exposed for unit tests + - `cmd_board(app, arg_str)` — the slash-command handler + - `render_kanban_board(app)` — the dispatch-time renderer + - `parse_board_args(...)` — exposed for unit tests + - `truncate_title(...)` — exposed for unit tests + - `render_task_card(...)` — exposed for unit tests + - `render_task_card_with_liveness(...)` — liveness-aware card (B1) + - `build_kanban_table(...)` — exposed for unit tests Logging convention (key=value to stderr, `bach.repl.board` namespace): event=board_view_enter project= archive= task_count= event=board_unknown_project arg= event=board_render task_count= columns= narrow_hint= + +Phase 16 (B1) — liveness dot prefix on board cards: + Renderers now accept an optional LivenessMap (from + services/board_liveness.py). When present, each card gains a colored + dot prefix based on session liveness: + ● green (success) — session is running + ◐ yellow (warning) — session is idle + ✓ dim (muted) — session ended + For executing/qa columns, ONE truncated (~40 chars) summary line is + added below the title when TaskLiveness.summary is set. The + needs_human flag renders a ⚑ needs you marker on the card. + Renderers consume LivenessMap ONLY — no discovery/artifact knowledge. """ from __future__ import annotations @@ -38,7 +51,10 @@ from bach.config.projects import ProjectRegistry from bach.config.themes import ThemePalette +from bach.domain.models import SessionLiveness +from bach.services.board_liveness import LivenessMap, liveness_for_tasks from bach.services.daily_index_service import DailyTask, discover_all_tasks +from bach.storage.artifacts import TaskArtifactStore if TYPE_CHECKING: # Avoid a circular import at runtime — board.py is imported lazily @@ -153,8 +169,7 @@ def parse_board_args(arg_str: str, registry: ProjectRegistry) -> BoardArgs: # alternative (silently pick the first or last) would # bury a probable user typo. raise BoardArgsError( - f"Multiple project filters not supported: " - f"{project_filter!r} and {token!r}" + f"Multiple project filters not supported: {project_filter!r} and {token!r}" ) project_filter = token continue @@ -226,6 +241,105 @@ def render_task_card(task: DailyTask, multi_project: bool, theme: ThemePalette) return line1 +# --------------------------------------------------------------------------- +# Liveness dot helpers (Phase 16 B1) +# --------------------------------------------------------------------------- + +# Columns where the observer summary line appears — only active stages where +# the watcher is likely producing useful context. Keeping it narrow avoids +# visual noise on quieter stages (inbox, prd, etc.). +_LIVENESS_SUMMARY_COLUMNS: frozenset[str] = frozenset({"executing", "qa"}) + +# Truncation cap for the summary line. Shorter than _TITLE_MAX_CHARS because +# the summary sits BELOW the title — it should visually recede, not compete. +_SUMMARY_MAX_CHARS = 40 + +# Dot symbols for liveness states. Unicode single-char so they take one cell +# of horizontal space and don't disturb column alignment. +_LIVENESS_DOTS: dict[SessionLiveness, str] = { + SessionLiveness.running: "●", + SessionLiveness.idle: "◐", + SessionLiveness.ended: "✓", +} + + +def _liveness_dot_prefix( + liveness: SessionLiveness | None, + theme: ThemePalette, +) -> str: + """Return a Rich-markup dot prefix for the given liveness value. + + None → empty string (no session ever linked — no dot shown). + Each live state maps to a dot symbol wrapped in the appropriate theme + color: running=success, idle=warning, ended=muted. + """ + if liveness is None: + return "" + dot = _LIVENESS_DOTS.get(liveness, "") + if not dot: + return "" + + # Map liveness → theme color slot. Uses theme.success/warning/muted so + # dot color tracks the active theme without hardcoding hex values here. + if liveness == SessionLiveness.running: + color = theme.success + elif liveness == SessionLiveness.idle: + color = theme.warning + else: + # ended — visually recede with muted color + color = theme.muted + return f"[{color}]{dot}[/{color}] " + + +def render_task_card_with_liveness( + task: DailyTask, + multi_project: bool, + theme: ThemePalette, + liveness_map: LivenessMap, +) -> str: + """Format a DailyTask card augmented with liveness data from LivenessMap. + + Extends render_task_card with: + - Dot prefix on line 1 based on SessionLiveness (●/◐/✓). + - ONE truncated summary line (dim) for executing/qa columns when + TaskLiveness.summary is set. + - ⚑ needs you marker (in error color) when needs_human=True. + + Renderers ONLY consume the LivenessMap — no disk reads happen here. + When the task_id is absent from the liveness_map, falls back to + render_task_card (backward-compatible, no dot). + """ + task_id = task.task_id or "—" + live = liveness_map.get(task.task_id) if task.task_id else None + + # --- build the base card (same layout as render_task_card) -------------- + parent_badge = ( + f" [{theme.muted}]↳{task.parent_task}[/{theme.muted}]" if task.parent_task else "" + ) + title = truncate_title(task.title) + dot_prefix = _liveness_dot_prefix(live.liveness if live else None, theme) + line1 = f"{dot_prefix}[bold]{task_id}[/bold]{parent_badge} {title}" + + # --- needs_human marker -------------------------------------------------- + needs_human_marker = "" + if live and live.needs_human: + # Render in error color so it stands out clearly from status text. + needs_human_marker = f" [{theme.error}]⚑ needs you[/{theme.error}]" + + # --- summary line (executing / qa only) ---------------------------------- + summary_line = "" + if live and live.summary and task.status in _LIVENESS_SUMMARY_COLUMNS: + truncated = truncate_title(live.summary, max_chars=_SUMMARY_MAX_CHARS) + summary_line = f"\n[{theme.muted}]{truncated}[/{theme.muted}]" + + # --- project line (multi-project boards) --------------------------------- + if multi_project: + p = theme.project + return f"{line1}{needs_human_marker}{summary_line}\n[{p}]{task.project_name}[/{p}]" + + return f"{line1}{needs_human_marker}{summary_line}" + + # --------------------------------------------------------------------------- # Table assembly # --------------------------------------------------------------------------- @@ -292,6 +406,7 @@ def build_kanban_table( project_count: int, title: str, theme: ThemePalette, + liveness_map: LivenessMap | None = None, ) -> Table: """Build the Rich Table for the board. @@ -303,6 +418,11 @@ def build_kanban_table( `project_count` controls whether cards include the project line (Q7: suppress when only one project is present in the source set). `theme` drives column header colors and card project-line color. + + Phase 16 (B1): `liveness_map` is optional. When provided, cards are + rendered with liveness dot prefix + summary + needs_human marker via + render_task_card_with_liveness. When absent or empty, falls back to + render_task_card (backward compatible). """ visible_columns = _select_columns(include_archive) buckets = _bucket_tasks_by_status(tasks, visible_columns) @@ -311,10 +431,10 @@ def build_kanban_table( table = Table( title=title, title_style="bold", - show_lines=True, # row separators help cards read as distinct units + show_lines=True, # row separators help cards read as distinct units show_header=True, pad_edge=False, - expand=True, # use the full terminal width when available + expand=True, # use the full terminal width when available ) for status in visible_columns: count = len(buckets[status]) @@ -329,12 +449,18 @@ def build_kanban_table( overflow="fold", ) + # Choose the card renderer: liveness-aware when a map was provided, + # plain otherwise. This keeps the default path unchanged. + def _render_card(task: DailyTask) -> str: + if liveness_map is not None: + return render_task_card_with_liveness(task, multi_project, theme, liveness_map) + return render_task_card(task, multi_project, theme) + # Render one card per bucket entry, padding shorter columns with "". # zip_longest yields rows where each row has one cell per column; # rows past a column's bucket end get "" for that column. column_cells: list[list[str]] = [ - [render_task_card(task, multi_project, theme) for task in buckets[status]] - for status in visible_columns + [_render_card(task) for task in buckets[status]] for status in visible_columns ] for row in zip_longest(*column_cells, fillvalue=""): table.add_row(*row) @@ -356,6 +482,11 @@ def render_kanban_board(app: BachRepl) -> None: - current_view_include_archive (bool) - console (rich.Console for output) + Phase 16 (B1): fetches a LivenessMap via liveness_for_tasks and passes + it to build_kanban_table so cards can show liveness dots, summary lines, + and needs_human markers. The fetch is best-effort — any failure returns + {} from liveness_for_tasks (logged at WARNING there; never crashes here). + Logs a `board_render` event with the visible task count and a narrow-terminal flag so post-hoc debugging can correlate "the board looked ugly" with terminal width. @@ -402,16 +533,28 @@ def render_kanban_board(app: BachRepl) -> None: ) logger.info( "event=board_render task_count=0 filter=%s archive=%s", - app.current_view_filter or "none", include_archive, + app.current_view_filter or "none", + include_archive, ) return + # Fetch liveness for the visible tasks. liveness_for_tasks never raises; + # returns {} on failure (already logged at WARNING by the service). + # read_frontmatter_fn is a free function that reads any artifact path — + # we use TaskArtifactStore's classmethod so no project binding is needed. + liveness_map = liveness_for_tasks( + tasks, + registry=app.registry, + read_frontmatter_fn=TaskArtifactStore.read_frontmatter_at, + ) + table = build_kanban_table( tasks=tasks, include_archive=include_archive, project_count=project_count, title=title, theme=app.current_theme, + liveness_map=liveness_map, ) app.console.print(table) @@ -426,8 +569,7 @@ def render_kanban_board(app: BachRepl) -> None: ) logger.info( - "event=board_render task_count=%d columns=%d narrow_hint=%s " - "filter=%s archive=%s", + "event=board_render task_count=%d columns=%d narrow_hint=%s filter=%s archive=%s", len(tasks), len(_select_columns(include_archive)), narrow, diff --git a/src/bach/repl/help_content.py b/src/bach/repl/help_content.py index f552e90..77ae7fc 100644 --- a/src/bach/repl/help_content.py +++ b/src/bach/repl/help_content.py @@ -865,11 +865,17 @@ class CommandHelp: ), what=( "Switches to the third dashboard view: a Rich-rendered kanban " - "with one column per workflow status. Default shows 7 WIP " + "with one column per workflow status. Default shows 9 WIP " "columns (hides done + carried_forward — see ADR-009 for why). " "Sticky: stays active until /list or /day exits.\n" "Inside board view, only stable `tN` refs work — positional " - " fails with a redirect (cards have no Nth row by design)." + " fails with a redirect (cards have no Nth row by design).\n" + "\nBoard legend (liveness dots — Phase 16):\n" + " ● green session is running (observer active)\n" + " ◐ yellow session is idle (agent waiting or hung)\n" + " ✓ dim session ended\n" + " (no dot) no session linked yet\n" + " ⚑ needs you observer flagged human action required" ), when=( "Daily WIP visibility: 'where is everything in the pipeline?' " @@ -879,7 +885,9 @@ class CommandHelp: "bach > /board\n" "(renders kanban: inbox(2) | research(0) | prototype(0) | prd(1) | …)\n" "bach > /board skillia all\n" - "(filters to skillia + shows archive columns)" + "(filters to skillia + shows archive columns)\n" + "● t47 Fix OAuth bug\n" + " Implementing token refresh… ← observer summary (executing/qa only)" ), related=("list", "day", "move"), aliases=("b",), diff --git a/src/bach/repl/sessions_view.py b/src/bach/repl/sessions_view.py index e223ae1..eb1c257 100644 --- a/src/bach/repl/sessions_view.py +++ b/src/bach/repl/sessions_view.py @@ -145,10 +145,11 @@ def cmd_sessions(app: Any, args: str) -> None: from datetime import UTC, datetime from bach.config.paths import bach_home + from bach.config.projects import ProjectRegistry from bach.config.settings import load_config from bach.runtimes.session_discovery import default_process_probe, discover_sessions from bach.runtimes.transcript import read_tail_events - from bach.services.sessions_service import list_sessions + from bach.services.sessions_service import list_sessions, scan_registry_artifact_paths cfg = load_config() # app is typed as Any to avoid a circular import with repl/app.py. @@ -180,9 +181,15 @@ def _discover() -> list[DiscoveredSession]: project, runtime, state, show_all = _parse_sessions_args(args) + # Per ADR-002: artifacts live under each project's .bach/ dir, not bach_home(). + registry = ProjectRegistry.default() + artifact_paths = scan_registry_artifact_paths(registry) + try: rows = list_sessions( sessions_dir=bach_home() / "sessions", + # artifacts_dir is unused when artifact_paths is injected; pass + # bach_home() as a safe sentinel to satisfy the signature. artifacts_dir=bach_home(), discover_fn=_discover, read_events_fn=read_tail_events, @@ -190,6 +197,7 @@ def _discover() -> list[DiscoveredSession]: runtime=runtime, state=state, max_ended_age_s=None if show_all else DEFAULT_ENDED_WINDOW_S, + artifact_paths=artifact_paths, ) except Exception as exc: logger.warning("event=repl_sessions_error reason=%s", exc) diff --git a/src/bach/runtimes/llm_call.py b/src/bach/runtimes/llm_call.py index efc0ce1..327b818 100644 --- a/src/bach/runtimes/llm_call.py +++ b/src/bach/runtimes/llm_call.py @@ -92,22 +92,35 @@ def run_codex_json( len(schema.get("properties", {})), ) - result = subprocess.run( - [ - "codex", "exec", - "--cd", _CODEX_CWD, - "--skip-git-repo-check", - "--model", model, - "--sandbox", "read-only", - "--output-schema", str(schema_path), - "-o", str(output_path), - prompt, - ], - capture_output=True, - text=True, - check=True, - timeout=timeout, - ) + try: + result = subprocess.run( + [ + "codex", "exec", + "--cd", _CODEX_CWD, + "--skip-git-repo-check", + "--model", model, + "--sandbox", "read-only", + "--output-schema", str(schema_path), + "-o", str(output_path), + prompt, + ], + capture_output=True, + text=True, + check=True, + timeout=timeout, + ) + except subprocess.CalledProcessError as exc: + # check=True buries codex's own diagnostics inside the exception; + # without this log the only visible failure is "exit status 1" + # (dogfooding PRD #8: a judge failure was undiagnosable). + logger.warning( + "event=llm_call_failed runtime=codex model=%s rc=%d stderr=%r stdout_tail=%r", + model, + exc.returncode, + (exc.stderr or "")[-500:], + (exc.output or "")[-200:], + ) + raise if not output_path.exists(): # Defensive — codex normally writes the output file on diff --git a/src/bach/runtimes/transcript.py b/src/bach/runtimes/transcript.py index 705909e..3e602df 100644 --- a/src/bach/runtimes/transcript.py +++ b/src/bach/runtimes/transcript.py @@ -154,7 +154,8 @@ def follow_events( *, poll_seconds: float, should_stop: Callable[[], bool], -) -> Iterator[SessionEvent]: + heartbeat: bool = False, +) -> Iterator[SessionEvent | None]: """Tail-follow a transcript file, yielding new events as they appear. Reads from the last known position and yields any new complete lines. @@ -169,11 +170,20 @@ def follow_events( Missing file: if the file does not exist yet, polling continues (the session may not have written its first line yet). Once should_stop fires the generator returns cleanly. + + heartbeat=True yields None once per poll cycle that produced no events. + Without it, a consumer driving side work from this generator (e.g. the + session watcher draining its hook spool) starves the moment the + transcript goes quiet — a closed session writes nothing, so a bare + for-loop body would never run again (the zombie-watcher bug found + while dogfooding PRD #8). """ offset = 0 # byte offset into the file; advances as lines are consumed while not should_stop(): if not path.exists(): + if heartbeat: + yield None time.sleep(poll_seconds) continue @@ -184,6 +194,8 @@ def follow_events( new_offset = fh.tell() except OSError as exc: logger.warning("event=transcript_follow_error path=%s reason=%s", path, exc) + if heartbeat: + yield None time.sleep(poll_seconds) continue @@ -205,10 +217,16 @@ def follow_events( incomplete = lines[-1] offset = new_offset - len(incomplete.encode("utf-8")) + yielded_any = False for line in complete_lines: if not line.strip(): continue + yielded_any = True yield parse_line(line, runtime) + if heartbeat and not yielded_any: + yield None + elif heartbeat: + yield None time.sleep(poll_seconds) diff --git a/src/bach/services/board_liveness.py b/src/bach/services/board_liveness.py new file mode 100644 index 0000000..9b7ffd1 --- /dev/null +++ b/src/bach/services/board_liveness.py @@ -0,0 +1,389 @@ +"""Board liveness join — maps DailyTask records to session liveness data. + +This module is the single place that knows how to: + 1. Read each task artifact's agent.runtime_session_id. + 2. Join that session_id against a list of DiscoveredSessions from the + session discovery layer. + 3. Extract the latest observer event (observer_suggestion / + observer_status_written) from the artifact log to surface + summary, confidence, and needs_human to the board renderer. + 4. Cache results with a TTL so the board refresh cycle (which calls + this on every render) doesn't hammer the filesystem and process + table on every keypress. + +Contract: + - Never raises to the renderer. Every exception is caught and logged. + - On scan failure with a prior cached result → return stale (log WARNING). + - On scan failure with no cache → return {}. + - Tasks with an empty task_id (legacy artifacts) are skipped — they + have no stable board key to index on. + - Tasks with no runtime_session_id → TaskLiveness(liveness=None, ...). + - Tasks with a session_id not found in discovery → liveness=ended. +""" + +import logging +import time +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from bach.config.projects import ProjectRegistry +from bach.domain.models import DiscoveredSession, SessionLiveness +from bach.services.daily_index_service import DailyTask + +logger = logging.getLogger(__name__) + +# --------------------------------------------------------------------------- +# Domain types +# --------------------------------------------------------------------------- + +# Observer event names that carry summary + confidence data to the board. +# Both events originate in session_watcher._apply_verdict: +# observer_suggestion — verdict logged but not applied (below threshold +# or observer_moves disabled) +# observer_status_written — verdict applied and status was changed +_OBSERVER_EVENT_NAMES: frozenset[str] = frozenset( + {"observer_suggestion", "observer_status_written"} +) + + +@dataclass(frozen=True) +class TaskLiveness: + """Board-ready liveness snapshot for a single task. + + Produced by liveness_for_tasks; consumed only by renderers. + Frozen so the renderer cannot accidentally mutate shared cache state. + + Fields: + liveness — None when no runtime_session_id exists for the task; + a SessionLiveness value when a session was (or was + expected to be) linked. + summary — Latest observer event's summary text, or None. + needs_human — True if the latest observer event flagged needs_human. + confidence — Latest observer event's confidence float, or None. + """ + + liveness: SessionLiveness | None # None = no linked session + summary: str | None # latest observer event summary, if any + needs_human: bool + confidence: float | None + + +# Type alias: stable task id → TaskLiveness snapshot. +LivenessMap = dict[str, TaskLiveness] + + +# --------------------------------------------------------------------------- +# Module-level TTL cache +# +# Keyed by frozenset of task_ids so different board views (today vs all) +# each have their own entry without interfering. Each entry is a tuple of +# (stored_timestamp, result_map). Stale results are kept alongside the +# timestamp so a subsequent scan failure can return them as a fallback. +# --------------------------------------------------------------------------- + +_CacheEntry = tuple[float, LivenessMap] +_cache: dict[frozenset[str], _CacheEntry] = {} + + +def _cache_clear() -> None: + """Discard all cached entries. Tests MUST call this in setUp/teardown.""" + _cache.clear() + + +# --------------------------------------------------------------------------- +# Observer event extraction +# --------------------------------------------------------------------------- + + +def _extract_latest_observer_event(log: list[Any]) -> dict[str, Any] | None: + """Return the last log entry whose event name is an observer event type. + + Iterates the log list in reverse to find the most recent observer event + in O(k) where k is the number of observer events — usually very small. + Returns None when there are no observer events in the log. + + Defensive: non-dict entries and entries without an 'event' key are + silently skipped. The observer events we care about + (observer_suggestion, observer_status_written) always carry summary, + confidence, and needs_human — callers should .get() with defaults. + """ + if not isinstance(log, list): + return None + for entry in reversed(log): + if not isinstance(entry, dict): + continue + if entry.get("event") in _OBSERVER_EVENT_NAMES: + return entry + return None + + +# --------------------------------------------------------------------------- +# Liveness join helpers +# --------------------------------------------------------------------------- + + +def _session_id_for_task(payload: dict[str, Any]) -> str | None: + """Extract the runtime_session_id from an artifact payload. + + Returns None when the field is absent or empty — callers treat this + as "no linked session" and set liveness=None in the result. + """ + agent = payload.get("agent") or {} + raw = agent.get("runtime_session_id") + if not raw: + return None + return str(raw) + + +def _build_session_index( + sessions: list[DiscoveredSession], +) -> dict[str, DiscoveredSession]: + """Build a {session_id → DiscoveredSession} lookup from the discovery result. + + When multiple sessions share a session_id (unexpected but defensive), + the last one wins. Discovery returns a consistent snapshot so ordering + within that snapshot is stable. + """ + return {s.session_id: s for s in sessions} + + +def _liveness_for_session_id( + session_id: str, + index: dict[str, DiscoveredSession], +) -> SessionLiveness: + """Resolve liveness for a known session_id against the discovery index. + + A session_id that is present in the artifact but absent from the + discovery snapshot → SessionLiveness.ended. "Ended" here means "not + found in the current discovery scan" — the most common reason is that + the transcript has been pruned from the runtime's session store (Claude + Code or Codex rolled over the project listing). It does NOT mean the + process was confirmed dead via `ps`; discovery simply has no record of + it. This is a conservative "probably done" signal, not a definitive + termination confirmation. + + This differs from liveness=None (no session_id at all). Renderers can + distinguish "no session ever started" (None) from "session ended or + pruned" (ended). + """ + discovered = index.get(session_id) + if discovered is None: + return SessionLiveness.ended + return discovered.liveness + + +# --------------------------------------------------------------------------- +# Public entry point +# --------------------------------------------------------------------------- + + +def liveness_for_tasks( + tasks: Sequence[DailyTask], + *, + registry: ProjectRegistry, + discover_fn: Callable[[], list[DiscoveredSession]] | None = None, + read_frontmatter_fn: Callable[[Path], dict[str, Any]], + now_fn: Callable[[], float] = time.monotonic, + ttl_s: float = 5.0, +) -> LivenessMap: + """Build a LivenessMap for the given tasks. + + Steps: + 1. Check the TTL cache keyed by frozenset(task_ids). Return cached + result when still fresh. + 2. Call discover_fn (or the scoped default) to get a snapshot of + all live/recent sessions. + 3. For each task with a non-empty task_id, read its artifact + frontmatter (via read_frontmatter_fn), join against the + session snapshot, and extract the latest observer event. + 4. Store in cache and return. + + On scan failure (discover_fn raises or read_frontmatter_fn raises for + all tasks): + - If a prior cached entry exists for this task set → return it + (log WARNING event=board_liveness_stale). + - Else return {} — never raises to the renderer. + + Args: + tasks: The DailyTask list to compute liveness for. + registry: The ProjectRegistry (used for scoped discovery default). + discover_fn: Callable that returns DiscoveredSession list. When + None, a scoped default is built from the registered project + paths (preferred: filter output, not the deep module). + read_frontmatter_fn: Callable that reads and returns a task + artifact's YAML frontmatter dict. Injected so tests don't + touch real disk paths. + now_fn: Callable returning a monotonic timestamp. Injected for + deterministic cache tests. + ttl_s: Cache TTL in seconds. Defaults to 5.0 seconds. + + Returns: + A LivenessMap (dict[task_id → TaskLiveness]). Empty dict on total + failure with no prior cache. + """ + # Build cache key from the stable task_ids (skip legacy empty-string ids). + task_ids: frozenset[str] = frozenset(t.task_id for t in tasks if t.task_id) + + now = now_fn() + + # --- Cache check -------------------------------------------------------- + if task_ids in _cache: + cached_ts, cached_result = _cache[task_ids] + if (now - cached_ts) < ttl_s: + # Fresh cache hit — return without scanning. + logger.debug( + "event=board_liveness_cache_hit task_count=%d age_s=%.2f", + len(task_ids), + now - cached_ts, + ) + return cached_result + + # --- Discover sessions -------------------------------------------------- + # Build the effective discover_fn. When None is provided, use a scoped + # default that discovers sessions and filters to registered project paths. + # Preferred approach: filter the OUTPUT of discover_sessions rather than + # touching the deep module (see contract in dag.md). + effective_discover_fn = discover_fn or _build_scoped_discover_fn(registry) + + sessions: list[DiscoveredSession] | None = None + try: + sessions = effective_discover_fn() + except Exception as exc: # noqa: BLE001 — discovery must never crash the board + logger.warning( + "event=board_liveness_scan_failed task_count=%d reason=%s", + len(task_ids), + type(exc).__name__, + ) + # Return stale cache if available; else return empty (safe default). + if task_ids in _cache: + logger.warning( + "event=board_liveness_stale task_count=%d", + len(task_ids), + ) + return _cache[task_ids][1] + return {} + + # --- Build session lookup index ----------------------------------------- + session_index = _build_session_index(sessions) + + # --- Build result map --------------------------------------------------- + result: LivenessMap = {} + + for task in tasks: + # Skip legacy artifacts without a stable task_id — they have no + # board key to index on (board renders "—" for them already). + if not task.task_id: + continue + + try: + payload = read_frontmatter_fn(task.artifact_path) + except Exception: # noqa: BLE001 — one bad artifact must not crash board + logger.warning( + "event=board_liveness_artifact_read_error task_id=%s path=%s", + task.task_id, + task.artifact_path, + ) + continue + + # Join: resolve session liveness. + session_id = _session_id_for_task(payload) + if session_id is None: + # No runtime session linked (task created but never launched, + # or Codex task before writeback completed). + liveness: SessionLiveness | None = None + else: + liveness = _liveness_for_session_id(session_id, session_index) + + # Extract latest observer event for summary / confidence / needs_human. + log = payload.get("log") or [] + obs_entry = _extract_latest_observer_event(log) + if obs_entry is not None: + summary: str | None = obs_entry.get("summary") or None + confidence: float | None = obs_entry.get("confidence") + needs_human: bool = bool(obs_entry.get("needs_human", False)) + else: + summary = None + confidence = None + needs_human = False + + result[task.task_id] = TaskLiveness( + liveness=liveness, + summary=summary, + needs_human=needs_human, + confidence=confidence, + ) + + # --- Store in cache with bounded size ----------------------------------- + # /show-style singleton keys (frozenset of one task_id) accumulate a new + # entry per unique task, so without a bound the cache grows unboundedly. + # Cap at 16 entries: drop the oldest by stored timestamp before inserting + # a new key. 16 covers any realistic simultaneous board+show workload + # without needing an LRU import. + _MAX_CACHE_ENTRIES = 16 + if task_ids not in _cache and len(_cache) >= _MAX_CACHE_ENTRIES: + # Find and evict the entry with the smallest stored timestamp. + oldest_key = min(_cache, key=lambda k: _cache[k][0]) + del _cache[oldest_key] + _cache[task_ids] = (now, result) + logger.debug( + "event=board_liveness_computed task_count=%d session_count=%d", + len(result), + len(sessions), + ) + return result + + +# --------------------------------------------------------------------------- +# Scoped discovery default +# --------------------------------------------------------------------------- + + +def _build_scoped_discover_fn( + registry: ProjectRegistry, +) -> Callable[[], list[DiscoveredSession]]: + """Build a discover_fn that scans sessions and filters to registered projects. + + Scoped discovery strategy (per dag.md contract): + - Call the real discover_sessions with default paths. + - Filter the OUTPUT to sessions whose project_dir matches one of + the registered project paths. + - This avoids touching the deep module's internals while still + narrowing the result to relevant sessions only. + + The filter is best-effort: sessions with project_dir=None are included + (they have no path to filter against; better to over-include than miss + a valid session for a task the user just launched). + """ + import datetime + + from bach.runtimes.session_discovery import ( + default_process_probe, + discover_sessions, + ) + + def _discover() -> list[DiscoveredSession]: + # Resolve registered project paths for filtering. + registry_data = registry.load() + registered_paths: set[Path] = {p.path for p in registry_data.projects.values()} + + from bach.config.settings import load_config + + cfg = load_config() + all_sessions = discover_sessions( + claude_projects_dir=Path.home() / ".claude" / "projects", + codex_home=Path.home() / ".codex", + process_probe=default_process_probe, + now=datetime.datetime.now(datetime.UTC), + running_threshold_s=cfg.liveness_running_seconds, + idle_threshold_s=cfg.liveness_idle_minutes * 60, + ) + + # Filter to sessions whose project_dir is in the registered set. + # Sessions with project_dir=None pass through (over-include is safer). + return [ + s for s in all_sessions if s.project_dir is None or s.project_dir in registered_paths + ] + + return _discover diff --git a/src/bach/services/hooks_service.py b/src/bach/services/hooks_service.py index 3a534f3..c351436 100644 --- a/src/bach/services/hooks_service.py +++ b/src/bach/services/hooks_service.py @@ -8,48 +8,85 @@ - NEVER clobber pre-existing unrelated user hooks. Parse-merge-rewrite only: read existing JSON, splice in Bach entries if absent, write back. - Idempotent: running install twice is a no-op — detected by matching - the command string, not by position. + the *command string suffix* (e.g. "internal hook-event") so absolute + paths from different venvs still deduplicate correctly. - Never raise on missing parent dirs — create on demand. - `HookInstallResult` reports what changed so the CLI can print it. + - On install: MIGRATE any old wrong-schema Bach entries (the flat + {"command": "bach ...", "run": "always"} shape) by removing them before + writing the correct nested shape. -Claude Code hook format (settings.json): +Claude Code hook format (settings.json) — REAL documented schema: { "hooks": { - "SessionEnd": [{"command": "...", "run": "always"}, ...], - "Stop": [{"command": "...", "run": "always"}, ...], - "PostToolUse": [{"command": "...", "run": "always"}, ...] + "SessionEnd": [ + { + "matcher": "", + "hooks": [{"type": "command", "command": " internal session-ended"}] + }, + { + "matcher": "", + "hooks": [{"type": "command", "command": " internal hook-event"}] + } + ], + "Stop": [ + { + "matcher": "", + "hooks": [{"type": "command", "command": " internal hook-event"}] + }, + { + "matcher": "", + "hooks": [{"type": "command", "command": " internal session-stop"}] + } + ], + "PostToolUse": [ + { + "matcher": "", + "hooks": [{"type": "command", "command": " internal hook-event"}] + } + ] } } - Bach adds entries that call `bach internal hook-event` piping stdin. - The Stop hook additionally calls `bach internal hook-event` with - the stop payload (the session-stop gate in a later phase will be - added separately by L1 — we only add hook-event routing here). -Codex hook format (~/.codex/hooks.json): + Matcher is empty string for Stop/SessionEnd/PostToolUse (fires on every + occurrence). Per the Claude Code docs: matcher value of "" or omitted + matches all. + + Command MUST be an absolute path — hook subprocesses do not inherit + the project venv PATH. Resolved at install time via _resolve_bach_exe(). + +Codex hook format (~/.codex/hooks.json) — verified from codex docs/--help: { - "sessionStop": [{"command": "..."}, ...], - "sessionStart": [{"command": "..."}, ...] + "sessionStop": [{"command": "..."}] } - Bach adds a sessionStop entry that pipes stdin to `bach internal hook-event`. + Bach adds a sessionStop entry that pipes stdin to + `bach internal hook-event`. This format is NOT the Claude Code nested + schema; Codex uses a simpler flat list. We leave Codex as-is (its + schema was correct before). """ import json import logging +import shutil +import sys from dataclasses import dataclass from pathlib import Path from typing import Any logger = logging.getLogger(__name__) -# The exact command Bach injects into Claude Code hooks. Uses shell pipe -# so the hook payload (on stdin) flows into the bach command. -_CLAUDE_HOOK_CMD = "bach internal hook-event" +# Suffix tokens that identify Bach-owned command entries. Matching on the +# suffix (not the full path) means re-running install from a different venv +# still deduplicates correctly against an entry written by a previous venv. +_SUFFIX_HOOK_EVENT = "internal hook-event" +_SUFFIX_SESSION_ENDED = "internal session-ended" +_SUFFIX_SESSION_STOP = "internal session-stop" # The equivalent command for Codex hooks (same target, same stdin pipe). -_CODEX_HOOK_CMD = "bach internal hook-event" +_CODEX_HOOK_CMD_SUFFIX = "internal hook-event" -# Claude Code event names Bach cares about. SessionEnd is the existing -# integration point; Stop and PostToolUse are Phase 16 additions. +# Claude Code event names Bach cares about. Order matches +# wire-up-session-hook.md: SessionEnd, Stop, PostToolUse. _CLAUDE_HOOK_EVENTS = ("SessionEnd", "Stop", "PostToolUse") @@ -66,12 +103,134 @@ class HookInstallResult: already_present: tuple[tuple[str, str], ...] = () +def _resolve_bach_exe() -> str: + """Return the absolute path to the `bach` console script. + + Resolution order (first that exists wins): + 1. Path(sys.executable).parent / "bach" — venv console script + 2. shutil.which("bach") — system PATH fallback + + Raises RuntimeError with a clear message if neither resolves, so the + user knows to install bach properly before hooks can be wired. + + Hook subprocesses do NOT inherit the project venv PATH, so an absolute + path is mandatory — a relative "bach" would silently no-op. + """ + # Prefer the venv-local script that lives alongside the current interpreter. + venv_script = Path(sys.executable).parent / "bach" + if venv_script.exists(): + return str(venv_script) + + # Fall back to whatever is on PATH at install time (e.g. pipx install). + which_result = shutil.which("bach") + if which_result: + return which_result + + raise RuntimeError( + "Cannot install Claude Code hooks: the `bach` executable was not found.\n" + "Expected it at: " + str(venv_script) + "\n" + "Also tried: shutil.which('bach') — not found.\n" + "Make sure bach is installed in the active venv (`uv sync` or `pip install bach`)." + ) + + +def _make_hook_group(abs_cmd: str) -> dict[str, Any]: + """Build one matcher-group entry in the REAL Claude Code hook schema. + + The documented Claude Code settings.json schema (verified at + https://code.claude.com/docs/en/hooks) is: + {"matcher": "", "hooks": [{"type": "command", "command": ""}]} + + Empty string matcher means "fire on every occurrence" for Stop, + SessionEnd, and PostToolUse — which is what Bach needs. + """ + return { + "matcher": "", + "hooks": [{"type": "command", "command": abs_cmd}], + } + + +def _cmd_suffix_matches(entry: Any, suffix: str) -> bool: + """Return True if `entry` is a valid hook group whose command ends with `suffix`. + + Handles both the NEW schema ({"matcher": "", "hooks": [{"type": "command", + "command": "..."}]}) and the OLD wrong schema ({"command": "...", "run": + "always"}) so old entries can be detected for migration. + """ + if not isinstance(entry, dict): + return False + # New schema: {"matcher": ..., "hooks": [...]} + inner_hooks = entry.get("hooks") + if isinstance(inner_hooks, list): + for h in inner_hooks: + if isinstance(h, dict) and h.get("command", "").endswith(suffix): + return True + # Old wrong schema: {"command": "bach internal ...", "run": "always"} + cmd = entry.get("command", "") + if isinstance(cmd, str) and cmd.endswith(suffix): + return True + return False + + +def _is_old_schema_bach_entry(entry: Any) -> bool: + """Return True if `entry` is an old-schema Bach hook ({"command": "bach ...", "run": "always"}). + + These are written by versions of Bach before the schema fix. They must be + removed on install to avoid accumulating dead entries that Claude Code + silently ignores. + """ + if not isinstance(entry, dict): + return False + # Old schema has a top-level "command" key (flat format). + cmd = entry.get("command", "") + if not isinstance(cmd, str): + return False + # Must be a Bach-owned entry (any internal sub-command). + bach_internal_suffixes = ( + _SUFFIX_HOOK_EVENT, + _SUFFIX_SESSION_ENDED, + _SUFFIX_SESSION_STOP, + ) + return any(cmd.endswith(s) for s in bach_internal_suffixes) + + +def _build_event_commands(event_name: str, abs_exe: str) -> list[tuple[str, str]]: + """Return the list of (suffix, full_command) pairs Bach installs for an event. + + Wiring as specified in wire-up-session-hook.md and ADR-015: + SessionEnd → session-ended (existing status/sidecar consumer) + + hook-event (spool for the watcher) + Stop → hook-event (spool) + + session-stop (loop gate; exits 2 for gated tasks) + PostToolUse → hook-event (spool) + """ + if event_name == "SessionEnd": + return [ + (_SUFFIX_SESSION_ENDED, f"{abs_exe} {_SUFFIX_SESSION_ENDED}"), + (_SUFFIX_HOOK_EVENT, f"{abs_exe} {_SUFFIX_HOOK_EVENT}"), + ] + if event_name == "Stop": + return [ + (_SUFFIX_HOOK_EVENT, f"{abs_exe} {_SUFFIX_HOOK_EVENT}"), + (_SUFFIX_SESSION_STOP, f"{abs_exe} {_SUFFIX_SESSION_STOP}"), + ] + if event_name == "PostToolUse": + return [ + (_SUFFIX_HOOK_EVENT, f"{abs_exe} {_SUFFIX_HOOK_EVENT}"), + ] + # Unknown event — no-op (future-proofing). + return [] + + def install_claude_hooks(*, project_dir: Path) -> HookInstallResult: """Parse-merge-rewrite .claude/settings.json under `project_dir`. - For each Bach-owned hook event (SessionEnd, Stop, PostToolUse), add - a `bach internal hook-event` entry if one is not already present. - Pre-existing user entries are preserved verbatim — we only append. + For each Bach-owned hook event (SessionEnd, Stop, PostToolUse): + - Remove any old-schema Bach entries ({"command": "...", "run": "always"}). + - Add the correct new-schema hook groups if not already present. + + Pre-existing unrelated user entries are preserved verbatim — we only + append/remove Bach-owned entries. `project_dir` is injected so tests use tmp_path. Production callers pass the CWD of the target project (where `.claude/` lives). @@ -107,6 +266,10 @@ def install_claude_hooks(*, project_dir: Path) -> HookInstallResult: hooks = {} existing["hooks"] = hooks + # Resolve the absolute path to the bach executable ONCE before looping. + # This raises RuntimeError with a clear message if bach is not found. + abs_exe = _resolve_bach_exe() + added: list[tuple[str, str]] = [] already_present: list[tuple[str, str]] = [] @@ -116,20 +279,32 @@ def install_claude_hooks(*, project_dir: Path) -> HookInstallResult: entries = [] hooks[event_name] = entries - # Check whether a Bach hook-event entry is already present. - # Match on command string — run/other fields may differ. - existing_cmds = {e.get("command", "") for e in entries if isinstance(e, dict)} - if _CLAUDE_HOOK_CMD in existing_cmds: - already_present.append((event_name, _CLAUDE_HOOK_CMD)) - else: - entries.append({"command": _CLAUDE_HOOK_CMD, "run": "always"}) - added.append((event_name, _CLAUDE_HOOK_CMD)) + # MIGRATE: remove any old-schema Bach entries (flat {"command","run"} shape) + # that Claude Code silently ignores. User hooks (non-Bach) are preserved. + old_count = len(entries) + entries[:] = [e for e in entries if not _is_old_schema_bach_entry(e)] + removed = old_count - len(entries) + if removed: logger.info( - "event=claude_hook_added event_name=%s command=%r", + "event=claude_hook_old_schema_removed event_name=%s count=%d", event_name, - _CLAUDE_HOOK_CMD, + removed, ) + # Install each command Bach wants for this event. + for suffix, full_cmd in _build_event_commands(event_name, abs_exe): + # Check if an entry with this suffix already exists (idempotency). + if any(_cmd_suffix_matches(e, suffix) for e in entries): + already_present.append((event_name, full_cmd)) + else: + entries.append(_make_hook_group(full_cmd)) + added.append((event_name, full_cmd)) + logger.info( + "event=claude_hook_added event_name=%s command=%r", + event_name, + full_cmd, + ) + settings_path.write_text( json.dumps(existing, indent=2, ensure_ascii=False) + "\n", encoding="utf-8", @@ -146,6 +321,10 @@ def install_codex_hooks(*, codex_home: Path) -> HookInstallResult: Adds a `sessionStop` entry that pipes the stop payload to `bach internal hook-event`. Pre-existing entries survive untouched. + Codex uses a simpler flat schema ({"command": "..."}) NOT the Claude Code + nested matcher-group schema. We verified this is the correct Codex format + and leave it unchanged. + `codex_home` is injected (default: ~/.codex) so tests use tmp_path. """ hooks_path = codex_home / "hooks.json" @@ -171,6 +350,10 @@ def install_codex_hooks(*, codex_home: Path) -> HookInstallResult: added: list[tuple[str, str]] = [] already_present: list[tuple[str, str]] = [] + # Resolve absolute path for the Codex hook command too. + abs_exe = _resolve_bach_exe() + codex_cmd = f"{abs_exe} {_CODEX_HOOK_CMD_SUFFIX}" + # Codex uses "sessionStop" as the event name for session end. event_name = "sessionStop" entries: list[Any] = existing.setdefault(event_name, []) @@ -178,23 +361,23 @@ def install_codex_hooks(*, codex_home: Path) -> HookInstallResult: entries = [] existing[event_name] = entries - # Detect existing Bach entry by matching the command substring. + # Detect existing Bach entry by matching the command suffix. def _is_bach(entry: Any) -> bool: if isinstance(entry, dict): - return _CODEX_HOOK_CMD in entry.get("command", "") + return str(entry.get("command", "")).endswith(_CODEX_HOOK_CMD_SUFFIX) if isinstance(entry, str): - return _CODEX_HOOK_CMD in entry + return entry.endswith(_CODEX_HOOK_CMD_SUFFIX) return False if any(_is_bach(e) for e in entries): - already_present.append((event_name, _CODEX_HOOK_CMD)) + already_present.append((event_name, codex_cmd)) else: - entries.append({"command": _CODEX_HOOK_CMD}) - added.append((event_name, _CODEX_HOOK_CMD)) + entries.append({"command": codex_cmd}) + added.append((event_name, codex_cmd)) logger.info( "event=codex_hook_added event_name=%s command=%r", event_name, - _CODEX_HOOK_CMD, + codex_cmd, ) hooks_path.write_text( @@ -211,11 +394,14 @@ def get_hooks_status(*, project_dir: Path, codex_home: Path) -> dict[str, Any]: """Inspect whether Bach hooks are installed in both runtimes. Returns a dict with keys: - claude_installed: bool — at least one Bach entry in each event + claude_installed: bool — Bach entries present in all expected events codex_installed: bool — Bach entry in codex sessionStop claude_detail: list of event names where Bach hooks are present codex_detail: list of event names where Bach hooks are present + Detects the NEW schema (matcher-group objects). Old-schema flat entries + are NOT counted as installed (they are migrated away by install). + All paths injected so tests run without touching real config. """ claude_installed = False @@ -233,8 +419,18 @@ def get_hooks_status(*, project_dir: Path, codex_home: Path) -> dict[str, Any]: if isinstance(hooks, dict): for event_name in _CLAUDE_HOOK_EVENTS: entries = hooks.get(event_name, []) - cmds = {e.get("command", "") for e in entries if isinstance(e, dict)} - if _CLAUDE_HOOK_CMD in cmds: + # For each event, verify all expected suffixes are present. + expected_suffixes = [ + s for s, _ in _build_event_commands( + event_name, + # Absolute path doesn't matter for suffix matching. + abs_exe="", + ) + ] + if all( + any(_cmd_suffix_matches(e, suf) for e in entries) + for suf in expected_suffixes + ): claude_detail.append(event_name) claude_installed = len(claude_detail) == len(_CLAUDE_HOOK_EVENTS) except (json.JSONDecodeError, OSError) as exc: @@ -256,9 +452,9 @@ def get_hooks_status(*, project_dir: Path, codex_home: Path) -> dict[str, Any]: def _is_bach(entry: Any) -> bool: if isinstance(entry, dict): - return _CODEX_HOOK_CMD in entry.get("command", "") + return str(entry.get("command", "")).endswith(_CODEX_HOOK_CMD_SUFFIX) if isinstance(entry, str): - return _CODEX_HOOK_CMD in entry + return entry.endswith(_CODEX_HOOK_CMD_SUFFIX) return False if any(_is_bach(e) for e in entries): diff --git a/src/bach/services/session_watcher.py b/src/bach/services/session_watcher.py index 17d89c0..3cbbf1f 100644 --- a/src/bach/services/session_watcher.py +++ b/src/bach/services/session_watcher.py @@ -44,6 +44,7 @@ SessionEventKind, ) from bach.runtimes.transcript import follow_events as _default_follow_events +from bach.runtimes.transcript import read_tail_events as _read_tail_events from bach.services.status_service import OBSERVER_SOURCE, set_task_status from bach.storage.artifacts import TaskArtifactStore @@ -51,7 +52,7 @@ # Spool event type strings that signal session termination. # These trigger a final judge call regardless of the judge interval. -_STOP_SPOOL_TYPES: frozenset[str] = frozenset({"stop", "session_end"}) +_STOP_SPOOL_TYPES: frozenset[str] = frozenset({"stop", "sessionend"}) # --------------------------------------------------------------------------- @@ -229,11 +230,35 @@ def _apply_verdict( # Status service may raise InvalidStatusError (observer authority violation) # or OSError (disk issue). Both are non-fatal — log and continue. logger.warning( - "event=observer_status_write_error artifact=%s target=%r reason=%s", + "event=observer_status_write_error session_id=%s artifact=%s target=%r reason=%s", + session_id, artifact, verdict.suggested_status, type(exc).__name__, ) + # The verdict must stay visible even when the move is rejected — + # the board summary line and /show read the artifact log, and an + # authority rejection is information the human wants ("judge thinks + # this needs me, but it's in a protected stage"). + try: + _append_log_event( + artifact, + { + "timestamp": datetime.now(UTC).isoformat(), + "event": "observer_suggestion", + "suggested_status": verdict.suggested_status, + "confidence": verdict.confidence, + "needs_human": verdict.needs_human, + "summary": verdict.summary, + "reason": f"write_rejected:{type(exc).__name__}", + }, + ) + except Exception: # noqa: BLE001 — best-effort; primary error already logged + logger.warning( + "event=observer_suggestion_fallback_failed session_id=%s artifact=%s", + session_id, + artifact, + ) def _run_judge( @@ -284,8 +309,17 @@ def _run_judge( def _is_stop_or_end_spool_event(event: dict[str, Any]) -> bool: - """True if this spool event signals session termination.""" - return event.get("type", "") in _STOP_SPOOL_TYPES + """True if this spool event signals session termination. + + Real spool lines (written by `bach internal hook-event`) carry the + runtime's own field: hook_event_name with values like "Stop" / + "SessionEnd". The legacy "type" spelling is kept for any synthetic + events. Normalization (lowercase, underscores stripped) makes both + runtimes' spellings match — dogfooding found the watcher blind to + real SessionEnd events because it only checked "type". + """ + raw = str(event.get("hook_event_name") or event.get("type") or "") + return raw.lower().replace("_", "") in _STOP_SPOOL_TYPES # --------------------------------------------------------------------------- @@ -456,7 +490,12 @@ def watch_session( accumulated_events: list[SessionEvent] = [] spool_offset = 0 - last_judge_time: float = 0.0 # epoch seconds; 0 means never judged + # Interval clock starts at loop entry: monotonic() is a huge number, so a + # 0.0 sentinel would make "interval elapsed" true on the very first event + # and fire a judge call after one transcript line (noise + cost). + judge_started_at = time.monotonic() + last_judge_time: float = judge_started_at + judged_at_least_once = False should_exit = False force_judge = False # True when a stop/end spool event arrives @@ -466,16 +505,22 @@ def _should_stop() -> bool: # Consume transcript events from follow_events_fn. # The generator yields events until should_stop() returns True. + # heartbeat=True: the generator yields None on quiet poll cycles so the + # spool drain + judge cadence below keep running after the transcript + # goes silent (a closed session writes nothing — without heartbeats this + # loop body would never execute again and the watcher zombies forever). for event in follow_events_fn( transcript_path, runtime, poll_seconds=1.0, should_stop=_should_stop, + heartbeat=True, ): - accumulated_events.append(event) + if event is not None: + accumulated_events.append(event) # Transcript end event → final judge and exit. - if event.kind is SessionEventKind.end: + if event is not None and event.kind is SessionEventKind.end: logger.info( "event=observer_transcript_end artifact=%s accumulated=%d", artifact_path, @@ -518,6 +563,21 @@ def _should_stop() -> bool: should_judge = force_judge or (interval_elapsed and len(accumulated_events) > 0) if should_judge: + # One-shot: a forced judge (stop/end) must not re-fire on every + # subsequent heartbeat tick before the loop winds down. + force_judge = False + judged_at_least_once = True + # Cold-start: a stop event already sitting in the spool can force + # a judge before the follower has read the transcript (dogfooding + # found the judge fired on a 1-event window). Backfill from the + # transcript tail so the verdict sees real session content. + if len(accumulated_events) < 5 and transcript_path != artifact_path: + try: + tail = _read_tail_events(transcript_path, runtime, max_events=50) + if len(tail) > len(accumulated_events): + accumulated_events = tail + except Exception: # noqa: BLE001 — window quality is best-effort + logger.warning("event=observer_tail_backfill_failed session_id=%s", session_id) # Re-read the payload before judging to get the freshest status. fresh_payload = _read_artifact_payload(artifact_path) or payload verdict = _run_judge( @@ -565,7 +625,7 @@ def _should_stop() -> bool: # If a stop event was seen but we haven't judged yet (e.g. transcript # was empty and only the spool had events), judge now. - if force_judge and last_judge_time == 0.0: + if force_judge and not judged_at_least_once: fresh_payload = _read_artifact_payload(artifact_path) or payload verdict = _run_judge( accumulated_events, diff --git a/src/bach/services/sessions_service.py b/src/bach/services/sessions_service.py index a5c7005..a3b41e8 100644 --- a/src/bach/services/sessions_service.py +++ b/src/bach/services/sessions_service.py @@ -10,6 +10,7 @@ adopt_session — link a session to a task artifact (writes runtime_session_id) resume_session — return/launch the resume command for a session scan_artifact_paths — recursively find .md artifacts under a directory + scan_registry_artifact_paths — collect artifacts from all registered projects (ADR-002) find_artifact_for_ref — find artifact Path for a task-id or session-id ref Design notes: @@ -31,7 +32,7 @@ from dataclasses import dataclass from datetime import UTC, datetime, timedelta from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from bach.domain.models import ( AgentRuntime, @@ -41,6 +42,13 @@ SessionLiveness, ) +if TYPE_CHECKING: + # ProjectRegistry is only needed for the type annotation on + # scan_registry_artifact_paths; importing it at the top level would + # create a circular dependency chain. TYPE_CHECKING imports are erased + # at runtime so they are safe here. + from bach.config.projects import ProjectRegistry + logger = logging.getLogger(__name__) @@ -262,6 +270,44 @@ def scan_artifact_paths(artifacts_dir: Path) -> list[Path]: return _scan_artifact_paths(artifacts_dir) +def scan_registry_artifact_paths(registry: "ProjectRegistry") -> list[Path]: + """Collect artifact paths from every registered project's local .bach dir. + + Per ADR-002, task artifacts are PROJECT-LOCAL: each project stores its + runs under /.bach/runs/YYYY-MM-DD/task-*.md. Scanning only + bach_home() therefore misses all of them — this function is the correct + source of truth for the sessions radar join. + + Projects whose .bach directory does not yet exist are silently skipped so + that a partially-initialised registry never crashes the session list. + + Args: + registry: a loaded or default ProjectRegistry instance. + + Returns: + Flat list of all .md paths found under every project's .bach/ tree. + """ + # Import inline to avoid a module-level circular dep: + # sessions_service → ProjectRegistry → (no dep on sessions_service) is fine, + # but keeping the import here mirrors the pattern used for TaskArtifactStore + # throughout this file and makes the dependency explicit. + + data = registry.load() + paths: list[Path] = [] + for project in data.projects.values(): + bach_dir = project.path / ".bach" + if not bach_dir.exists(): + # Skip projects that have never been initialised — common for newly + # registered projects that have not yet run `bach project init`. + logger.debug("event=scan_registry_skip_missing_bach project=%s", project.path) + continue + found = list(bach_dir.rglob("*.md")) + logger.debug("event=scan_registry_project project=%s found=%d", project.path, len(found)) + paths.extend(found) + logger.debug("event=scan_registry_artifact_paths total=%d", len(paths)) + return paths + + def find_artifact_for_ref( ref: str, artifact_paths: list[Path], diff --git a/src/bach/services/status_service.py b/src/bach/services/status_service.py index e686d16..36b5797 100644 --- a/src/bach/services/status_service.py +++ b/src/bach/services/status_service.py @@ -185,7 +185,11 @@ def set_task_status( # may only perform the two safe transitions defined in the PRD architecture # rule. Checked after reading old_status so the error message can name # both ends of the rejected move. - if source == OBSERVER_SOURCE and (old_status, new_status) not in _OBSERVER_ALLOWED_TRANSITIONS: + if ( + source == OBSERVER_SOURCE + and old_status != new_status # same→same is a benign no-op, not a violation + and (old_status, new_status) not in _OBSERVER_ALLOWED_TRANSITIONS + ): raise InvalidStatusError( f"observer source may not transition {old_status!r} → {new_status!r}. " f"Allowed observer moves: executing→qa, " diff --git a/src/bach/services/task_service.py b/src/bach/services/task_service.py index 2fcf3c9..04d27eb 100644 --- a/src/bach/services/task_service.py +++ b/src/bach/services/task_service.py @@ -21,6 +21,7 @@ import typer +from bach.config.paths import bach_home from bach.config.projects import ProjectRegistry from bach.config.settings import load_config from bach.domain.models import AgentRuntime, Project, SessionMode, TaskArtifact @@ -217,41 +218,76 @@ def launch_task( # session watcher. The watcher follows transcript events and the spool # to drive status updates via the judge. # Detached (no stdin/stdout, no wait) so it outlives this process. + # B1: print ONE confirmation line so the user knows a watcher started. if config.watch_on_launch: - self._spawn_session_watcher(artifact) - - def _spawn_session_watcher(self, artifact: Path) -> None: + spawned, log_path = self._spawn_session_watcher(artifact) + if spawned: + # Include the log path so the user knows where to look if the + # watcher behaves unexpectedly — it's the only trace of a + # detached process that has no terminal. + output(f"Observer watching this session (watch_on_launch=on). Log: {log_path}") + else: + # User explicitly opted in to watching — silence here would make + # them believe the session is watched when it isn't. + output("Observer spawn failed — session running unwatched.") + + def _spawn_session_watcher(self, artifact: Path) -> tuple[bool, Path | None]: """Spawn `bach internal session-watch ` as a detached child. Uses the same Python interpreter so the watcher inherits the same environment (venv, PATH, etc.) without requiring `bach` to be on PATH. - subprocess.Popen with close_fds=True + stdout/stderr DEVNULL detaches - the child from the parent terminal. We do NOT wait for it. + subprocess.Popen with close_fds=True + redirected stdout/stderr + detaches the child from the parent terminal. We do NOT wait for it. + + stdout and stderr are redirected to an append-mode log file at: + bach_home() / "logs" / "watcher-.log" + This is the ONLY trace of a detached process — DEVNULL would make + watcher crashes completely invisible and unrecoverable to diagnose. - Best-effort: a spawn failure logs a warning but never blocks the - primary launch. The user has their iTerm tab; the watcher is bonus. + Best-effort: a spawn failure logs a warning but NEVER blocks the + primary launch (warn-not-fail). The user has their iTerm tab; the + watcher is bonus. + + Returns (True, log_path) on successful spawn, (False, None) on + failure. The caller decides whether to print a confirmation line — + no output happens here so the output channel stays with launch_task. """ + # Build the log file path: bach_home/logs/watcher-.log + # Using the artifact stem makes the log easy to correlate to the task. + # Fallback to a runtime timestamp if the artifact path is unusually short. + stem = artifact.stem if artifact.stem else artifact.name + logs_dir = bach_home() / "logs" + logs_dir.mkdir(parents=True, exist_ok=True) # create on demand + log_path = logs_dir / f"watcher-{stem}.log" + try: # Construct the same invocation as `bach internal session-watch` # by re-using the current interpreter and module entry point. cmd = [sys.executable, "-m", "bach", "internal", "session-watch", str(artifact)] + # Open in append mode so successive watchers for the same artifact + # (e.g. re-launch after crash) accumulate in one file rather than + # overwriting the previous run's output. + log_fh = log_path.open("a", encoding="utf-8") subprocess.Popen( cmd, stdin=subprocess.DEVNULL, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, + stdout=log_fh, + stderr=log_fh, close_fds=True, ) logger.info( - "event=session_watcher_spawned artifact=%s", + "event=session_watcher_spawned artifact=%s log=%s", artifact, + log_path, ) - except Exception as exc: # noqa: BLE001 — best-effort, never block launch + return True, log_path + except Exception as exc: # noqa: BLE001 — warn-not-fail, never block launch logger.warning( "event=session_watcher_spawn_failed artifact=%s reason=%s", artifact, type(exc).__name__, ) + return False, None def validate_task(self, artifact: Path) -> list[str]: """Return a list of validation errors for the artifact, empty if OK.""" diff --git a/src/bach/storage/artifacts.py b/src/bach/storage/artifacts.py index fd0432e..27a156d 100644 --- a/src/bach/storage/artifacts.py +++ b/src/bach/storage/artifacts.py @@ -66,11 +66,21 @@ _ENUM_VALIDATORS: dict[str, frozenset[str]] = { # research + prototype are the pre-PRD investigation stages. Mirrors # `services/status_service.VALID_STATUSES` (the canonical set). - "status": frozenset({ - "inbox", "research", "prototype", "prd", - "kanban_issues", "afk_queue", "hitl_queue", - "executing", "qa", "done", "carried_forward", - }), + "status": frozenset( + { + "inbox", + "research", + "prototype", + "prd", + "kanban_issues", + "afk_queue", + "hitl_queue", + "executing", + "qa", + "done", + "carried_forward", + } + ), "agent.runtime": frozenset({"claude-code", "codex"}), # Phase 14: the `skill.name` field stores the SessionMode value. Two # new values join the original grill pair: @@ -91,10 +101,17 @@ # sections + launch_command by now". Used to make some checks contextual: # we do NOT flag a missing "Final Task Contract" while the agent is still # in the early stages, only once the work has been classified or started. -_STATUSES_PAST_PRD = frozenset({ - "kanban_issues", "afk_queue", "hitl_queue", "executing", - "qa", "done", "carried_forward", -}) +_STATUSES_PAST_PRD = frozenset( + { + "kanban_issues", + "afk_queue", + "hitl_queue", + "executing", + "qa", + "done", + "carried_forward", + } +) # Phase 9 → Phase 10 status migration. Applied in-memory at every read # (`_read_frontmatter_static`). Files are NOT auto-rewritten; the @@ -295,6 +312,17 @@ def read_frontmatter(self, artifact: Path) -> dict[str, Any]: """Parse and return the artifact's YAML frontmatter.""" return self._read_frontmatter_static(artifact) + @classmethod + def read_frontmatter_at(cls, artifact_path: Path) -> dict[str, Any]: + """Public seam for display-layer reads without a project-bound store. + + Wraps _read_frontmatter_static so callers outside the storage layer + (board renderers, liveness service) don't need to reference the + private method directly. Identical behaviour — status migration + and all other transformations are applied via the private reader. + """ + return cls._read_frontmatter_static(artifact_path) + def update_launch_artifacts( self, artifact: Path, @@ -424,10 +452,7 @@ def _check_enums(cls, payload: dict[str, Any]) -> list[str]: # Treat null/missing as "for the required-field check, not me". continue if value not in allowed: - errors.append( - f"invalid value for {dotted}: {value!r} " - f"(allowed: {sorted(allowed)})" - ) + errors.append(f"invalid value for {dotted}: {value!r} (allowed: {sorted(allowed)})") return errors @staticmethod @@ -519,14 +544,10 @@ def _check_post_grill_context(payload: dict[str, Any], body: str) -> list[str]: errors: list[str] = [] launch_command = payload.get("agent", {}).get("launch_command") if not launch_command: - errors.append( - f"status is {status!r} but agent.launch_command is empty" - ) + errors.append(f"status is {status!r} but agent.launch_command is empty") for section in _REQUIRED_BODY_SECTIONS: if section not in body: - errors.append( - f"status is {status!r} but body missing section: {section!r}" - ) + errors.append(f"status is {status!r} but body missing section: {section!r}") return errors @staticmethod @@ -608,5 +629,7 @@ def _apply_status_migration_in_place(payload: dict[str, Any], artifact: Path) -> payload["status"] = migrated logger.info( "event=status_migration_applied path=%s old=%s new=%s", - artifact, raw_status, migrated, + artifact, + raw_status, + migrated, ) diff --git a/tests/unit/test_board.py b/tests/unit/test_board.py index b3f477e..8907560 100644 --- a/tests/unit/test_board.py +++ b/tests/unit/test_board.py @@ -11,6 +11,11 @@ we can assert on the rendered string without coupling to the terminal at test time. - DailyTask records are constructed directly (frozen dataclass). + +Phase 16 (B1) additions: + - render_task_card_with_liveness: liveness dot prefix, summary line, + needs_human marker. + - build_kanban_table_with_liveness: passes LivenessMap through to cards. """ from __future__ import annotations @@ -23,7 +28,7 @@ from bach.config.projects import ProjectRegistry from bach.config.themes import THEMES -from bach.domain.models import Project +from bach.domain.models import Project, SessionLiveness from bach.repl.board import ( BoardArgs, BoardArgsError, @@ -32,8 +37,10 @@ build_kanban_table, parse_board_args, render_task_card, + render_task_card_with_liveness, truncate_title, ) +from bach.services.board_liveness import LivenessMap, TaskLiveness from bach.services.daily_index_service import DailyTask # --------------------------------------------------------------------------- @@ -72,7 +79,9 @@ def _registry_with_projects(tmp_path: Path, names: list[str]) -> ProjectRegistry for name in names: key = name.lower() data.projects[key] = Project( - key=key, name=name, path=tmp_path / name, + key=key, + name=name, + path=tmp_path / name, ) registry.save(data) return registry @@ -148,7 +157,7 @@ def test_card_single_project_omits_project_line() -> None: assert "[bold]t1[/bold]" in out assert "Fix" in out assert "skillia" not in out # project suppressed - assert "\n" not in out # single-line card + assert "\n" not in out # single-line card def test_card_multi_project_renders_project_line() -> None: @@ -213,9 +222,7 @@ def test_parse_args_project_name_sets_filter(tmp_path: Path) -> None: def test_parse_args_order_independent(tmp_path: Path) -> None: """`skillia all` and `all skillia` produce identical BoardArgs.""" registry = _registry_with_projects(tmp_path, ["skillia"]) - assert parse_board_args("skillia all", registry) == parse_board_args( - "all skillia", registry - ) + assert parse_board_args("skillia all", registry) == parse_board_args("all skillia", registry) def test_parse_args_unknown_project_raises(tmp_path: Path) -> None: @@ -287,8 +294,17 @@ def test_build_table_renders_all_default_column_headers() -> None: ) out = _render(table) # Phase 13: research + prototype inserted between inbox and prd. - for col in ("inbox", "research", "prototype", "prd", "kanban_issues", - "afk_queue", "hitl_queue", "executing", "qa"): + for col in ( + "inbox", + "research", + "prototype", + "prd", + "kanban_issues", + "afk_queue", + "hitl_queue", + "executing", + "qa", + ): assert col in out assert "done" not in out assert "carried_forward" not in out @@ -345,8 +361,8 @@ def test_build_table_sorts_tasks_oldest_first_per_column() -> None: table = build_kanban_table( tasks=[ _task(task_id="t10", title="Newest", status="inbox"), - _task(task_id="t1", title="Oldest", status="inbox"), - _task(task_id="t5", title="Middle", status="inbox"), + _task(task_id="t1", title="Oldest", status="inbox"), + _task(task_id="t5", title="Middle", status="inbox"), ], include_archive=False, project_count=1, @@ -358,3 +374,173 @@ def test_build_table_sorts_tasks_oldest_first_per_column() -> None: # top-to-bottom. So the substring "Oldest" appears before "Middle" # before "Newest". assert out.index("Oldest") < out.index("Middle") < out.index("Newest") + + +# --------------------------------------------------------------------------- +# B1: render_task_card_with_liveness — liveness dot prefix + summary + marker +# --------------------------------------------------------------------------- +# These tests use fake LivenessMaps so no real disk I/O or discovery runs. + + +def _liveness( + liveness: SessionLiveness | None = None, + summary: str | None = None, + needs_human: bool = False, + confidence: float | None = None, +) -> TaskLiveness: + """Convenience factory for TaskLiveness instances in tests.""" + return TaskLiveness( + liveness=liveness, + summary=summary, + needs_human=needs_human, + confidence=confidence, + ) + + +def test_card_with_liveness_running_has_green_dot() -> None: + """A running session renders the green ● dot prefix.""" + task = _task(task_id="t1", status="executing") + live_map: LivenessMap = {"t1": _liveness(liveness=SessionLiveness.running)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + # The dot character should appear somewhere in the output. + assert "●" in out + + +def test_card_with_liveness_idle_has_half_dot() -> None: + """An idle session renders the ◐ half-dot prefix (warning color).""" + task = _task(task_id="t2", status="executing") + live_map: LivenessMap = {"t2": _liveness(liveness=SessionLiveness.idle)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + assert "◐" in out + + +def test_card_with_liveness_ended_has_check_dot() -> None: + """An ended session renders the ✓ check prefix (muted color).""" + task = _task(task_id="t3", status="qa") + live_map: LivenessMap = {"t3": _liveness(liveness=SessionLiveness.ended)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + assert "✓" in out + + +def test_card_with_liveness_none_has_no_dot() -> None: + """No session linked (liveness=None) → no dot prefix, card unchanged.""" + task = _task(task_id="t4", status="inbox") + live_map: LivenessMap = {"t4": _liveness(liveness=None)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + # None liveness → no dot characters + assert "●" not in out + assert "◐" not in out + + +def test_card_with_liveness_missing_from_map_renders_normally() -> None: + """A task not in the LivenessMap renders without a dot (silent no-op).""" + task = _task(task_id="t5", status="inbox") + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map={} + ) + # No dot but should still contain the task ID and title. + assert "t5" in out + assert "Demo task" in out + assert "●" not in out + + +def test_card_with_liveness_shows_summary_for_executing_column() -> None: + """executing column cards get the truncated summary line under the title.""" + task = _task(task_id="t6", status="executing") + summary_text = "Working on authentication module refactoring long text here" + live_map: LivenessMap = { + "t6": _liveness(liveness=SessionLiveness.running, summary=summary_text) + } + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + # Summary should appear (possibly truncated at ~40 chars). + assert "Working on authentication" in out + + +def test_card_with_liveness_summary_truncated_to_40_chars() -> None: + """Summary line is truncated to ~40 characters to keep cards dense.""" + task = _task(task_id="t7", status="executing") + long_summary = "A" * 80 + live_map: LivenessMap = { + "t7": _liveness(liveness=SessionLiveness.running, summary=long_summary) + } + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + # Truncated with ellipsis — full 80-char string should NOT appear. + assert "A" * 80 not in out + assert "…" in out + + +def test_card_with_liveness_no_summary_for_inbox_column() -> None: + """inbox column cards do NOT get the summary line (only executing/qa).""" + task = _task(task_id="t8", status="inbox") + live_map: LivenessMap = {"t8": _liveness(liveness=None, summary="some summary text here now")} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + # Summary should NOT appear for non-executing/qa columns. + assert "some summary text here now" not in out + + +def test_card_with_liveness_needs_human_marker_shown() -> None: + """needs_human=True → '⚑ needs you' marker visible on the card.""" + task = _task(task_id="t9", status="executing") + live_map: LivenessMap = {"t9": _liveness(liveness=SessionLiveness.running, needs_human=True)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + assert "⚑" in out + assert "needs you" in out + + +def test_card_with_liveness_needs_human_false_no_marker() -> None: + """needs_human=False → no marker on the card.""" + task = _task(task_id="t10", status="executing") + live_map: LivenessMap = {"t10": _liveness(liveness=SessionLiveness.running, needs_human=False)} + out = render_task_card_with_liveness( + task, multi_project=False, theme=THEMES["default"], liveness_map=live_map + ) + assert "⚑" not in out + + +def test_build_kanban_table_with_liveness_passes_dots() -> None: + """build_kanban_table with a liveness_map renders dots in the output.""" + task = _task(task_id="t11", status="executing") + live_map: LivenessMap = {"t11": _liveness(liveness=SessionLiveness.running)} + table = build_kanban_table( + tasks=[task], + include_archive=False, + project_count=1, + title="test", + theme=THEMES["default"], + liveness_map=live_map, + ) + out = _render(table) + # The running-dot should appear in the rendered table. + assert "●" in out + + +def test_build_kanban_table_no_liveness_map_renders_normally() -> None: + """build_kanban_table without liveness_map still works (backward compat).""" + task = _task(task_id="t12", status="inbox") + table = build_kanban_table( + tasks=[task], + include_archive=False, + project_count=1, + title="test", + theme=THEMES["default"], + ) + out = _render(table) + assert "t12" in out + # No dots when no liveness info provided. + assert "●" not in out diff --git a/tests/unit/test_board_liveness.py b/tests/unit/test_board_liveness.py new file mode 100644 index 0000000..3082f23 --- /dev/null +++ b/tests/unit/test_board_liveness.py @@ -0,0 +1,775 @@ +"""Tests for services/board_liveness.py — liveness_for_tasks. + +Strategy: + - Use tmp_path to build realistic fake task artifacts with controlled + frontmatter (including observer log entries). + - Inject discover_fn, read_frontmatter_fn, now_fn so no real + ~/.claude, ~/.codex, or ~/.bach directories are touched. + - Use a fake clock (monotonic counter) to exercise TTL cache logic. + - Verify: basic join, observer event extraction, needs_human flag, + TTL cache hit/miss, scan failure → cached fallback, never raises. + +All filesystem writes stay inside tmp_path — never real user directories. +""" + +from pathlib import Path +from typing import Any + +import pytest +import yaml + +from bach.config.projects import ProjectRegistry +from bach.domain.models import ( + AgentRuntime, + DiscoveredSession, + SessionLiveness, +) +from bach.services.board_liveness import ( + TaskLiveness, + _cache_clear, + liveness_for_tasks, +) +from bach.services.daily_index_service import DailyTask + +# --------------------------------------------------------------------------- +# Helpers — fake task artifacts +# --------------------------------------------------------------------------- + + +def _write_artifact( + tmp_path: Path, + *, + task_id: str = "t1", + runtime_session_id: str | None = "uuid-abc-123", + log: list[dict[str, Any]] | None = None, + project_name: str = "myproj", +) -> Path: + """Write a minimal valid task artifact at a predictable tmp_path location.""" + date_dir = tmp_path / project_name / ".bach" / "runs" / "2026-06-11" + date_dir.mkdir(parents=True, exist_ok=True) + + artifact = date_dir / f"task-100000-{task_id}.md" + payload: dict[str, Any] = { + "id": f"task-100000-{task_id}", + "task_id": task_id, + "date": "2026-06-11", + "status": "executing", + "project": {"name": project_name, "path": str(tmp_path / project_name)}, + "agent": { + "runtime": "claude-code", + "bach_session_id": f"bach-test-{task_id}", + "runtime_session_id": runtime_session_id, + "runtime_session_name": f"bach-test-{project_name}", + "launch_command": None, + "resume_command": None, + "restart_command": None, + }, + "skill": {"name": "grill-me", "status": "not_started"}, + "task": { + "title": f"Task {task_id}", + "original_description": f"Task {task_id}", + "objective": None, + "non_goals": [], + "constraints": [], + "likely_files_or_areas": [], + "acceptance_checks": [], + "risks_or_unknowns": [], + }, + "post_grill": {"readiness": "not_ready", "next_action": "undecided"}, + "log": log or [{"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}], + } + body = "\n## Grill Transcript / Notes\n\n## Final Task Contract\n\n## Carry Forward\n" + artifact.write_text(f"---\n{yaml.safe_dump(payload, sort_keys=False)}---\n{body}") + return artifact + + +def _make_task( + artifact_path: Path, + *, + task_id: str = "t1", + project_name: str = "myproj", + project_path: Path | None = None, +) -> DailyTask: + """Build a DailyTask pointing at the given artifact.""" + return DailyTask( + artifact_path=artifact_path, + project_name=project_name, + project_path=project_path or artifact_path.parents[3], + bach_session_id=f"bach-test-{task_id}", + runtime="claude-code", + status="executing", + title=f"Task {task_id}", + resume_command=None, + date="2026-06-11", + task_id=task_id, + ) + + +def _make_session(session_id: str, liveness: SessionLiveness) -> DiscoveredSession: + """Build a minimal DiscoveredSession for testing the join.""" + return DiscoveredSession( + runtime=AgentRuntime.claude_code, + session_id=session_id, + transcript_path=Path("/fake/transcript.jsonl"), + project_dir=None, + liveness=liveness, + last_activity=None, + ) + + +def _make_registry(tmp_path: Path) -> ProjectRegistry: + """Return a real ProjectRegistry backed by an EMPTY projects.yaml in tmp_path.""" + registry_path = tmp_path / "projects.yaml" + registry_path.write_text("projects: {}\n") + return ProjectRegistry(registry_path) + + +# --------------------------------------------------------------------------- +# Fake read_frontmatter_fn (reads real YAML from artifact path) +# --------------------------------------------------------------------------- + + +def _real_read_frontmatter(artifact: Path) -> dict[str, Any]: + """Parse YAML frontmatter from a test artifact.""" + text = artifact.read_text() + _start, yaml_part, _body = text.split("---", 2) + result: dict[str, Any] = yaml.safe_load(yaml_part) or {} + return result + + +# --------------------------------------------------------------------------- +# Monotonic fake clock +# --------------------------------------------------------------------------- + + +class FakeClock: + """Controllable monotonic clock for TTL cache tests.""" + + def __init__(self, start: float = 0.0) -> None: + self._t = start + + def __call__(self) -> float: + return self._t + + def advance(self, seconds: float) -> None: + self._t += seconds + + +# --------------------------------------------------------------------------- +# Tests — basic liveness join +# --------------------------------------------------------------------------- + + +def test_no_tasks_returns_empty(tmp_path: Path) -> None: + """Empty task list → empty LivenessMap, never raises.""" + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + assert result == {} + + +def test_task_with_no_session_id_yields_none_liveness(tmp_path: Path) -> None: + """A task whose artifact has no runtime_session_id → liveness=None, summary=None.""" + artifact = _write_artifact(tmp_path, task_id="t1", runtime_session_id=None) + task = _make_task(artifact, task_id="t1") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + assert "t1" in result + entry = result["t1"] + assert entry.liveness is None + assert entry.summary is None + assert entry.needs_human is False + assert entry.confidence is None + + +def test_task_matched_to_running_session(tmp_path: Path) -> None: + """Task whose session_id matches a running DiscoveredSession → liveness=running.""" + artifact = _write_artifact(tmp_path, task_id="t2", runtime_session_id="sess-running") + task = _make_task(artifact, task_id="t2") + running_session = _make_session("sess-running", SessionLiveness.running) + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [running_session], + read_frontmatter_fn=_real_read_frontmatter, + ) + + assert result["t2"].liveness == SessionLiveness.running + + +def test_task_matched_to_idle_session(tmp_path: Path) -> None: + """Task linked to an idle session → liveness=idle.""" + artifact = _write_artifact(tmp_path, task_id="t3", runtime_session_id="sess-idle") + task = _make_task(artifact, task_id="t3") + idle_session = _make_session("sess-idle", SessionLiveness.idle) + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [idle_session], + read_frontmatter_fn=_real_read_frontmatter, + ) + + assert result["t3"].liveness == SessionLiveness.idle + + +def test_task_with_session_id_not_in_discovered(tmp_path: Path) -> None: + """Task has a session_id but no DiscoveredSession matches → liveness=ended.""" + artifact = _write_artifact(tmp_path, task_id="t4", runtime_session_id="sess-gone") + task = _make_task(artifact, task_id="t4") + # Discover returns a different session, no match. + other_session = _make_session("sess-other", SessionLiveness.running) + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [other_session], + read_frontmatter_fn=_real_read_frontmatter, + ) + + # session_id present but unmatched → liveness defaults to ended (session gone) + # The contract: liveness is None only when there is NO linked session. + # A missing-from-discovery result means ended SessionLiveness. + assert result["t4"].liveness == SessionLiveness.ended + + +# --------------------------------------------------------------------------- +# Tests — observer event extraction +# --------------------------------------------------------------------------- + + +def test_observer_suggestion_extracts_summary(tmp_path: Path) -> None: + """A log entry with event=observer_suggestion → summary + confidence extracted.""" + log: list[dict[str, Any]] = [ + {"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}, + { + "timestamp": "2026-06-11T01:00:00+00:00", + "event": "observer_suggestion", + "summary": "Agent is grilling the requirements", + "confidence": 0.82, + "needs_human": False, + }, + ] + artifact = _write_artifact(tmp_path, task_id="t5", log=log) + task = _make_task(artifact, task_id="t5") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + entry = result["t5"] + assert entry.summary == "Agent is grilling the requirements" + assert entry.confidence == pytest.approx(0.82) + assert entry.needs_human is False + + +def test_observer_status_written_extracts_summary(tmp_path: Path) -> None: + """event=observer_status_written also yields summary (latest observer event).""" + log: list[dict[str, Any]] = [ + {"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}, + { + "timestamp": "2026-06-11T01:30:00+00:00", + "event": "observer_status_written", + "summary": "Task is done, moving to QA", + "confidence": 0.95, + "needs_human": False, + }, + ] + artifact = _write_artifact(tmp_path, task_id="t6", log=log) + task = _make_task(artifact, task_id="t6") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + entry = result["t6"] + assert entry.summary == "Task is done, moving to QA" + assert entry.confidence == pytest.approx(0.95) + + +def test_needs_human_flag_extracted(tmp_path: Path) -> None: + """needs_human=True in the latest observer event bubbles up correctly.""" + log: list[dict[str, Any]] = [ + {"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}, + { + "timestamp": "2026-06-11T02:00:00+00:00", + "event": "observer_suggestion", + "summary": "Blocked on a design question", + "confidence": 0.9, + "needs_human": True, + }, + ] + artifact = _write_artifact(tmp_path, task_id="t7", log=log) + task = _make_task(artifact, task_id="t7") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + assert result["t7"].needs_human is True + + +def test_latest_observer_event_wins(tmp_path: Path) -> None: + """When multiple observer events exist, the LAST one wins.""" + log: list[dict[str, Any]] = [ + {"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}, + { + "timestamp": "2026-06-11T01:00:00+00:00", + "event": "observer_suggestion", + "summary": "First observer note", + "confidence": 0.6, + "needs_human": False, + }, + { + "timestamp": "2026-06-11T02:00:00+00:00", + "event": "observer_suggestion", + "summary": "Latest observer note", + "confidence": 0.88, + "needs_human": True, + }, + ] + artifact = _write_artifact(tmp_path, task_id="t8", log=log) + task = _make_task(artifact, task_id="t8") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + entry = result["t8"] + assert entry.summary == "Latest observer note" + assert entry.needs_human is True + assert entry.confidence == pytest.approx(0.88) + + +def test_no_observer_events_yields_none_summary(tmp_path: Path) -> None: + """Artifact with only non-observer log entries → summary=None, confidence=None.""" + log: list[dict[str, Any]] = [ + {"timestamp": "2026-06-11T00:00:00+00:00", "event": "created"}, + {"timestamp": "2026-06-11T01:00:00+00:00", "event": "launched"}, + ] + artifact = _write_artifact(tmp_path, task_id="t9", log=log) + task = _make_task(artifact, task_id="t9") + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + entry = result["t9"] + assert entry.summary is None + assert entry.confidence is None + assert entry.needs_human is False + + +# --------------------------------------------------------------------------- +# Tests — TTL cache +# --------------------------------------------------------------------------- + + +def test_ttl_cache_returns_cached_result_within_ttl(tmp_path: Path) -> None: + """Second call within TTL uses cached result — discover_fn not called again.""" + artifact = _write_artifact(tmp_path, task_id="t10", runtime_session_id="sess-t10") + task = _make_task(artifact, task_id="t10") + session = _make_session("sess-t10", SessionLiveness.running) + registry = _make_registry(tmp_path) + clock = FakeClock(start=100.0) + _cache_clear() + + call_count = 0 + + def counting_discover() -> list[DiscoveredSession]: + nonlocal call_count + call_count += 1 + return [session] + + # First call — populates cache. + result1 = liveness_for_tasks( + [task], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=10.0, + ) + + # Second call within TTL window — should reuse cache. + clock.advance(5.0) + result2 = liveness_for_tasks( + [task], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=10.0, + ) + + # discover_fn should only have been called once. + assert call_count == 1 + assert result1 == result2 + + +def test_ttl_cache_refreshes_after_expiry(tmp_path: Path) -> None: + """After TTL expires, discover_fn is called again and result is fresh.""" + artifact = _write_artifact(tmp_path, task_id="t11", runtime_session_id="sess-t11") + task = _make_task(artifact, task_id="t11") + registry = _make_registry(tmp_path) + clock = FakeClock(start=200.0) + _cache_clear() + + sessions: list[DiscoveredSession] = [_make_session("sess-t11", SessionLiveness.running)] + call_count = 0 + + def counting_discover() -> list[DiscoveredSession]: + nonlocal call_count + call_count += 1 + return sessions + + # First call. + liveness_for_tasks( + [task], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=5.0, + ) + + # Advance past TTL. + clock.advance(10.0) + + liveness_for_tasks( + [task], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=5.0, + ) + + assert call_count == 2 + + +def test_different_task_sets_bypass_cache(tmp_path: Path) -> None: + """Different frozensets of task ids each get their own cache entry.""" + art1 = _write_artifact(tmp_path, task_id="t12", runtime_session_id="sess-12") + art2 = _write_artifact(tmp_path, task_id="t13", runtime_session_id="sess-13") + task1 = _make_task(art1, task_id="t12") + task2 = _make_task(art2, task_id="t13") + registry = _make_registry(tmp_path) + clock = FakeClock(start=0.0) + _cache_clear() + + call_count = 0 + + def counting_discover() -> list[DiscoveredSession]: + nonlocal call_count + call_count += 1 + return [] + + liveness_for_tasks( + [task1], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=60.0, + ) + liveness_for_tasks( + [task2], + registry=registry, + discover_fn=counting_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=60.0, + ) + + # Two different task sets → two separate cache entries → two discover calls. + assert call_count == 2 + + +# --------------------------------------------------------------------------- +# Tests — scan failure resilience +# --------------------------------------------------------------------------- + + +def test_discover_fn_failure_returns_last_cached(tmp_path: Path) -> None: + """If discover_fn raises, last cached result is returned (not empty dict).""" + artifact = _write_artifact(tmp_path, task_id="t14", runtime_session_id="sess-t14") + task = _make_task(artifact, task_id="t14") + registry = _make_registry(tmp_path) + clock = FakeClock(start=0.0) + _cache_clear() + + good_session = _make_session("sess-t14", SessionLiveness.running) + + call_count = 0 + + def flaky_discover() -> list[DiscoveredSession]: + nonlocal call_count + call_count += 1 + if call_count == 1: + return [good_session] + raise OSError("disk error") + + # First call succeeds → populates cache. + result_first = liveness_for_tasks( + [task], + registry=registry, + discover_fn=flaky_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=1.0, + ) + + # Advance past TTL so next call triggers fresh scan. + clock.advance(5.0) + + # Second call: discover_fn raises → returns stale cached result. + result_second = liveness_for_tasks( + [task], + registry=registry, + discover_fn=flaky_discover, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=1.0, + ) + + assert result_first == result_second + assert result_second["t14"].liveness == SessionLiveness.running + + +def test_discover_fn_failure_no_cache_returns_empty(tmp_path: Path) -> None: + """If discover_fn raises on first call (no prior cache), returns {}.""" + artifact = _write_artifact(tmp_path, task_id="t15", runtime_session_id="sess-t15") + task = _make_task(artifact, task_id="t15") + registry = _make_registry(tmp_path) + clock = FakeClock(start=0.0) + _cache_clear() + + def always_fail() -> list[DiscoveredSession]: + raise OSError("no disk access") + + # Must NOT raise — returns empty dict as fallback. + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=always_fail, + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=5.0, + ) + + assert result == {} + + +def test_read_frontmatter_failure_skips_task(tmp_path: Path) -> None: + """If read_frontmatter_fn raises for a task, that task is skipped (not raised).""" + artifact = _write_artifact(tmp_path, task_id="t16", runtime_session_id="sess-t16") + task = _make_task(artifact, task_id="t16") + registry = _make_registry(tmp_path) + _cache_clear() + + def bad_reader(path: Path) -> dict[str, Any]: + raise ValueError("corrupt YAML") + + # Must NOT raise — task is skipped gracefully. + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=bad_reader, + ) + + # t16 not in result — skipped rather than crashed. + assert "t16" not in result + + +# --------------------------------------------------------------------------- +# Tests — TaskLiveness is a frozen dataclass +# --------------------------------------------------------------------------- + + +def test_task_liveness_is_frozen() -> None: + """TaskLiveness instances are immutable (frozen dataclass).""" + tl = TaskLiveness( + liveness=SessionLiveness.running, + summary="doing stuff", + needs_human=False, + confidence=0.9, + ) + with pytest.raises((AttributeError, TypeError)): + tl.needs_human = True # type: ignore[misc] + + +def test_task_liveness_equality() -> None: + """Two TaskLiveness with equal fields compare equal.""" + a = TaskLiveness(liveness=None, summary=None, needs_human=False, confidence=None) + b = TaskLiveness(liveness=None, summary=None, needs_human=False, confidence=None) + assert a == b + + +# --------------------------------------------------------------------------- +# Tests — multiple tasks processed in a single call +# --------------------------------------------------------------------------- + + +def test_multiple_tasks_each_get_entry(tmp_path: Path) -> None: + """All tasks in the input list appear as keys in the result.""" + ids = ["t20", "t21", "t22"] + tasks = [] + sessions = [] + for tid in ids: + art = _write_artifact(tmp_path, task_id=tid, runtime_session_id=f"sess-{tid}") + tasks.append(_make_task(art, task_id=tid)) + sessions.append(_make_session(f"sess-{tid}", SessionLiveness.running)) + + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + tasks, + registry=registry, + discover_fn=lambda: sessions, + read_frontmatter_fn=_real_read_frontmatter, + ) + + for tid in ids: + assert tid in result + assert result[tid].liveness == SessionLiveness.running + + +def test_task_with_empty_task_id_skipped(tmp_path: Path) -> None: + """A DailyTask with task_id='' is not added to the result map.""" + artifact = _write_artifact(tmp_path, task_id="legacy") + # Build task with empty task_id (legacy artifact without stable id) + task = DailyTask( + artifact_path=artifact, + project_name="myproj", + project_path=tmp_path / "myproj", + bach_session_id="bach-legacy", + runtime="claude-code", + status="executing", + title="Legacy task", + resume_command=None, + date="2026-06-11", + task_id="", # <-- legacy artifact + ) + registry = _make_registry(tmp_path) + _cache_clear() + + result = liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + ) + + # Empty task_id should be skipped — no entry in result. + assert "" not in result + + +# --------------------------------------------------------------------------- +# Tests — module cache bound (finding #5) +# --------------------------------------------------------------------------- + + +def test_cache_evicts_oldest_when_full(tmp_path: Path) -> None: + """Cache evicts the oldest entry when the 16-entry cap is reached. + + Singleton keys (frozenset of one task_id) accumulate quickly when + /show is called for many different tasks. Without the bound the + cache grows unboundedly. With the bound, inserting a 17th unique key + should keep total size at 16. + """ + from bach.services.board_liveness import _cache + + _cache_clear() + registry = _make_registry(tmp_path) + + # Populate 16 distinct singleton keys (one task per call). + clock = FakeClock(start=0.0) + for n in range(16): + tid = f"singleton-{n}" + art = _write_artifact(tmp_path, task_id=tid, runtime_session_id=None) + task = _make_task(art, task_id=tid) + # Each key gets a slightly different timestamp so ordering is deterministic. + clock.advance(1.0) + liveness_for_tasks( + [task], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=9999.0, # never expire during this test + ) + + assert len(_cache) == 16 + + # Insert a 17th key — oldest should be evicted, size stays at 16. + clock.advance(1.0) + tid_new = "singleton-16" + art_new = _write_artifact(tmp_path, task_id=tid_new, runtime_session_id=None) + task_new = _make_task(art_new, task_id=tid_new) + liveness_for_tasks( + [task_new], + registry=registry, + discover_fn=lambda: [], + read_frontmatter_fn=_real_read_frontmatter, + now_fn=clock, + ttl_s=9999.0, + ) + + # Size must not exceed 16. + assert len(_cache) <= 16 + # The new key must be present. + new_key = frozenset({tid_new}) + assert new_key in _cache + # The oldest key (singleton-0, added first at t=1.0) must have been evicted. + oldest_key = frozenset({"singleton-0"}) + assert oldest_key not in _cache diff --git a/tests/unit/test_config_set.py b/tests/unit/test_config_set.py new file mode 100644 index 0000000..dc2a707 --- /dev/null +++ b/tests/unit/test_config_set.py @@ -0,0 +1,479 @@ +"""Tests for `bach config set ` — A2 (Board × Radar fusion PRD#8). + +TDD: these tests were written BEFORE the implementation. +Coverage: +- OBSERVER_SETTABLE_KEYS is populated with the 8 allowlisted keys +- set_observer_key succeeds for every valid key + valid value +- set_observer_key raises ValueError with a human message for: + * invalid bool values + * threshold out of [0, 1] + * non-positive int values + * unknown key (error message lists sorted valid keys) +- round-trip: set_observer_key writes → load_config reads back the value +- CLI: `bach config set ` exits 0 on success, 1 on error +- `bach config show` output includes all 8 observer knobs +""" + +from pathlib import Path +from unittest.mock import patch + +import pytest +from typer.testing import CliRunner + +from bach.cli.app import app +from bach.config.settings import ( + OBSERVER_SETTABLE_KEYS, + load_config, + set_observer_key, +) + +_runner = CliRunner() + + +# --------------------------------------------------------------------------- +# OBSERVER_SETTABLE_KEYS catalog +# --------------------------------------------------------------------------- + + +def test_observer_settable_keys_has_eight_entries() -> None: + """The allowlist must contain exactly the 8 observer knobs specified in the DAG.""" + assert len(OBSERVER_SETTABLE_KEYS) == 8 + + +def test_observer_settable_keys_contains_expected_keys() -> None: + """Every hyphenated key name from the spec must be present.""" + expected = { + "watch-on-launch", + "observer-moves", + "judge-confidence-threshold", + "judge-interval-seconds", + "liveness-running-seconds", + "liveness-idle-minutes", + "afk-max-turns", + "afk-time-budget-minutes", + } + assert set(OBSERVER_SETTABLE_KEYS.keys()) == expected + + +def test_observer_settable_keys_all_callable() -> None: + """Each value must be callable (the parser/validator function).""" + for key, parser in OBSERVER_SETTABLE_KEYS.items(): + assert callable(parser), f"parser for '{key}' is not callable" + + +# --------------------------------------------------------------------------- +# set_observer_key — valid inputs (happy path) +# --------------------------------------------------------------------------- + + +def test_set_observer_key_watch_on_launch_on(tmp_path: Path) -> None: + """watch-on-launch accepts 'on' and writes True.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("watch-on-launch", "on") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.watch_on_launch is True + finally: + s._default_config_path = original + + +def test_set_observer_key_watch_on_launch_off(tmp_path: Path) -> None: + """watch-on-launch accepts 'off' and writes False.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("watch-on-launch", "off") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.watch_on_launch is False + finally: + s._default_config_path = original + + +def test_set_observer_key_bool_true_variants(tmp_path: Path) -> None: + """Bool knobs accept: on/off/true/false/yes/no (case-insensitive).""" + import bach.config.settings as s + + for truthy in ("on", "true", "yes", "ON", "TRUE", "YES"): + target = tmp_path / f"cfg_{truthy}.yaml" + original = s._default_config_path + s._default_config_path = lambda t=target: t # type: ignore[misc] + try: + set_observer_key("watch-on-launch", truthy) + cfg = load_config(path=target) + assert cfg.watch_on_launch is True, f"Expected True for '{truthy}'" + finally: + s._default_config_path = original + + +def test_set_observer_key_bool_false_variants(tmp_path: Path) -> None: + """Bool knobs accept: on/off/true/false/yes/no (case-insensitive).""" + import bach.config.settings as s + + for falsy in ("off", "false", "no", "OFF", "FALSE", "NO"): + target = tmp_path / f"cfg_{falsy}.yaml" + original = s._default_config_path + s._default_config_path = lambda t=target: t # type: ignore[misc] + try: + set_observer_key("watch-on-launch", falsy) + cfg = load_config(path=target) + assert cfg.watch_on_launch is False, f"Expected False for '{falsy}'" + finally: + s._default_config_path = original + + +def test_set_observer_key_observer_moves(tmp_path: Path) -> None: + """observer-moves is a bool knob; False round-trips correctly.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("observer-moves", "false") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.observer_moves is False + finally: + s._default_config_path = original + + +def test_set_observer_key_judge_confidence_threshold(tmp_path: Path) -> None: + """judge-confidence-threshold accepts a float in [0, 1].""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("judge-confidence-threshold", "0.85") + cfg = load_config(path=tmp_path / "config.yaml") + assert pytest.approx(cfg.judge_confidence_threshold) == 0.85 + finally: + s._default_config_path = original + + +def test_set_observer_key_judge_confidence_threshold_boundary_values(tmp_path: Path) -> None: + """Boundary values 0.0 and 1.0 are valid for threshold.""" + import bach.config.settings as s + + for val, _expected in (("0.0", 0.0), ("1.0", 1.0)): + target = tmp_path / f"cfg_{val.replace('.', '_')}.yaml" + original = s._default_config_path + s._default_config_path = lambda t=target: t # type: ignore[misc] + try: + set_observer_key("judge-confidence-threshold", val) + cfg = load_config(path=target) + assert pytest.approx(cfg.judge_confidence_threshold) == float(val) + finally: + s._default_config_path = original + + +def test_set_observer_key_judge_interval_seconds(tmp_path: Path) -> None: + """judge-interval-seconds accepts a positive int.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("judge-interval-seconds", "120") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.judge_interval_seconds == 120 + finally: + s._default_config_path = original + + +def test_set_observer_key_liveness_running_seconds(tmp_path: Path) -> None: + """liveness-running-seconds accepts a positive int.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("liveness-running-seconds", "30") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.liveness_running_seconds == 30 + finally: + s._default_config_path = original + + +def test_set_observer_key_liveness_idle_minutes(tmp_path: Path) -> None: + """liveness-idle-minutes accepts a positive int.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("liveness-idle-minutes", "10") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.liveness_idle_minutes == 10 + finally: + s._default_config_path = original + + +def test_set_observer_key_afk_max_turns(tmp_path: Path) -> None: + """afk-max-turns accepts a positive int.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("afk-max-turns", "5") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.afk_max_turns == 5 + finally: + s._default_config_path = original + + +def test_set_observer_key_afk_time_budget_minutes(tmp_path: Path) -> None: + """afk-time-budget-minutes accepts a positive int.""" + import bach.config.settings as s + + original = s._default_config_path + s._default_config_path = lambda: tmp_path / "config.yaml" + try: + set_observer_key("afk-time-budget-minutes", "90") + cfg = load_config(path=tmp_path / "config.yaml") + assert cfg.afk_time_budget_minutes == 90 + finally: + s._default_config_path = original + + +def test_set_observer_key_returns_path(tmp_path: Path) -> None: + """set_observer_key returns the path of the config file written.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + result = set_observer_key("afk-max-turns", "3") + assert result == target + finally: + s._default_config_path = original + + +# --------------------------------------------------------------------------- +# set_observer_key — rejection (sad path) +# --------------------------------------------------------------------------- + + +def test_set_observer_key_unknown_key_raises_value_error() -> None: + """An unknown key raises ValueError with a human message listing valid keys.""" + with pytest.raises(ValueError) as exc_info: + set_observer_key("does-not-exist", "on") + msg = str(exc_info.value) + # The error must list all valid keys so the user knows what to type. + assert "watch-on-launch" in msg + assert "does-not-exist" in msg + + +def test_set_observer_key_unknown_key_lists_sorted_valid_keys() -> None: + """Valid keys in the error message must appear in sorted order.""" + with pytest.raises(ValueError) as exc_info: + set_observer_key("nope", "val") + msg = str(exc_info.value) + valid_sorted = sorted(OBSERVER_SETTABLE_KEYS.keys()) + for key in valid_sorted: + assert key in msg + + +def test_set_observer_key_invalid_bool_raises_value_error() -> None: + """A non-boolean string for a bool knob must raise ValueError.""" + with pytest.raises(ValueError) as exc_info: + set_observer_key("watch-on-launch", "maybe") + msg = str(exc_info.value).lower() + assert "maybe" in msg or "bool" in msg or "on/off" in msg or "true" in msg + + +def test_set_observer_key_threshold_above_1_raises(tmp_path: Path) -> None: + """A threshold > 1.0 must raise ValueError (out of [0, 1]).""" + with pytest.raises(ValueError): + set_observer_key("judge-confidence-threshold", "1.5") + + +def test_set_observer_key_threshold_below_0_raises(tmp_path: Path) -> None: + """A threshold < 0.0 must raise ValueError.""" + with pytest.raises(ValueError): + set_observer_key("judge-confidence-threshold", "-0.1") + + +def test_set_observer_key_threshold_non_numeric_raises() -> None: + """A non-numeric threshold must raise ValueError.""" + with pytest.raises(ValueError): + set_observer_key("judge-confidence-threshold", "high") + + +def test_set_observer_key_int_zero_raises() -> None: + """A positive-int knob with value 0 must raise ValueError.""" + with pytest.raises(ValueError): + set_observer_key("judge-interval-seconds", "0") + + +def test_set_observer_key_int_negative_raises() -> None: + """A positive-int knob with a negative value must raise ValueError.""" + with pytest.raises(ValueError): + set_observer_key("liveness-running-seconds", "-5") + + +def test_set_observer_key_int_non_int_raises() -> None: + """A non-integer string for an int knob must raise ValueError.""" + with pytest.raises(ValueError): + set_observer_key("afk-max-turns", "ten") + + +# --------------------------------------------------------------------------- +# CLI: `bach config set ` +# --------------------------------------------------------------------------- + + +def test_cli_config_set_exits_0_on_success(tmp_path: Path) -> None: + """A valid key + value must exit with code 0 and print confirmation.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + result = _runner.invoke(app, ["config", "set", "afk-max-turns", "7"]) + assert result.exit_code == 0, result.output + assert "afk-max-turns" in result.output + finally: + s._default_config_path = original + + +def test_cli_config_set_exits_1_on_invalid_value(tmp_path: Path) -> None: + """An invalid value (e.g., zero for a positive-int knob) must exit 1 with an error. + + Note: we use '0' rather than '-1' because click/typer treats leading-dash + arguments as flags, and this test is about our validator rejecting the value, + not about click argument parsing. + """ + result = _runner.invoke(app, ["config", "set", "afk-max-turns", "0"]) + assert result.exit_code == 1 + assert result.output # must print something + + +def test_cli_config_set_exits_1_on_unknown_key() -> None: + """An unknown key must exit 1 and list valid keys.""" + result = _runner.invoke(app, ["config", "set", "no-such-key", "on"]) + assert result.exit_code == 1 + assert "watch-on-launch" in result.output + + +def test_cli_config_set_bool_key_written_and_readable(tmp_path: Path) -> None: + """End-to-end: `bach config set watch-on-launch on` → load_config sees True.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + result = _runner.invoke(app, ["config", "set", "watch-on-launch", "on"]) + assert result.exit_code == 0 + cfg = load_config(path=target) + assert cfg.watch_on_launch is True + finally: + s._default_config_path = original + + +def test_cli_config_set_float_key_written_and_readable(tmp_path: Path) -> None: + """End-to-end: `bach config set judge-confidence-threshold 0.9` round-trips.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + result = _runner.invoke(app, ["config", "set", "judge-confidence-threshold", "0.9"]) + assert result.exit_code == 0 + cfg = load_config(path=target) + assert pytest.approx(cfg.judge_confidence_threshold) == 0.9 + finally: + s._default_config_path = original + + +# --------------------------------------------------------------------------- +# `bach config show` — observer knobs section +# --------------------------------------------------------------------------- + + +def test_config_show_includes_watch_on_launch(tmp_path: Path) -> None: + """`bach config show` must display watch_on_launch.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + with patch("bach.cli.app.bach_home", return_value=tmp_path): + result = _runner.invoke(app, ["config", "show"]) + assert result.exit_code == 0 + assert "watch" in result.output.lower() + finally: + s._default_config_path = original + + +def test_config_show_includes_observer_moves(tmp_path: Path) -> None: + """`bach config show` must display observer_moves.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + with patch("bach.cli.app.bach_home", return_value=tmp_path): + result = _runner.invoke(app, ["config", "show"]) + assert result.exit_code == 0 + assert "observer" in result.output.lower() + finally: + s._default_config_path = original + + +def test_config_show_includes_judge_confidence(tmp_path: Path) -> None: + """`bach config show` must display judge_confidence_threshold.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + with patch("bach.cli.app.bach_home", return_value=tmp_path): + result = _runner.invoke(app, ["config", "show"]) + assert result.exit_code == 0 + assert "judge" in result.output.lower() or "confidence" in result.output.lower() + finally: + s._default_config_path = original + + +def test_config_show_includes_afk_fields(tmp_path: Path) -> None: + """`bach config show` must display afk_max_turns and afk_time_budget_minutes.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + with patch("bach.cli.app.bach_home", return_value=tmp_path): + result = _runner.invoke(app, ["config", "show"]) + assert result.exit_code == 0 + assert "afk" in result.output.lower() + finally: + s._default_config_path = original + + +def test_config_show_includes_liveness_fields(tmp_path: Path) -> None: + """`bach config show` must display liveness_running_seconds and liveness_idle_minutes.""" + import bach.config.settings as s + + target = tmp_path / "config.yaml" + original = s._default_config_path + s._default_config_path = lambda: target + try: + with patch("bach.cli.app.bach_home", return_value=tmp_path): + result = _runner.invoke(app, ["config", "show"]) + assert result.exit_code == 0 + assert "liveness" in result.output.lower() + finally: + s._default_config_path = original diff --git a/tests/unit/test_enable_observer_docs.py b/tests/unit/test_enable_observer_docs.py new file mode 100644 index 0000000..f12d314 --- /dev/null +++ b/tests/unit/test_enable_observer_docs.py @@ -0,0 +1,171 @@ +"""Tests for the enable-observer how-to doc and related config file updates. + +C1 owns: docs/how-to/enable-observer.md (new), + CLAUDE.md (command list update), + AGENTS.md (command list update), + docs/internal/progress.txt (append entry). + +These tests verify that the required documents exist, contain the +load-bearing sections mandated by the DAG spec, and that the +CLAUDE.md/AGENTS.md command lists reflect the new PRD#8 commands. +They do NOT test content accuracy (code review concern) — they guard +against accidental deletion or structural omission. +""" + +from pathlib import Path + +# Locate repo root relative to this test file so the tests are +# independent of the working directory. +_REPO_ROOT = Path(__file__).parent.parent.parent +_HOW_TO_DIR = _REPO_ROOT / "docs" / "how-to" +_ENABLE_OBSERVER_DOC = _HOW_TO_DIR / "enable-observer.md" +_CLAUDE_MD = _REPO_ROOT / "CLAUDE.md" +_AGENTS_MD = _REPO_ROOT / "AGENTS.md" +_PROGRESS_TXT = _REPO_ROOT / "docs" / "internal" / "progress.txt" + + +# --------------------------------------------------------------------------- +# enable-observer.md existence and structure +# --------------------------------------------------------------------------- + + +def test_enable_observer_doc_exists() -> None: + """docs/how-to/enable-observer.md must exist.""" + assert _ENABLE_OBSERVER_DOC.exists(), f"Missing: {_ENABLE_OBSERVER_DOC}" + + +def test_enable_observer_doc_has_hooks_install_claude_code() -> None: + """Doc must show the hooks install claude-code command.""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "hooks install claude-code" in text + + +def test_enable_observer_doc_has_hooks_install_codex() -> None: + """Doc must show the hooks install codex command.""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "hooks install codex" in text + + +def test_enable_observer_doc_has_project_flag() -> None: + """Doc must document the --project flag (A3 addition).""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "--project" in text + + +def test_enable_observer_doc_has_watch_on_launch() -> None: + """Doc must document the watch-on-launch config knob (A2 addition).""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "watch-on-launch" in text + + +def test_enable_observer_doc_has_config_set_command() -> None: + """Doc must show the `bach config set` command (A2 addition).""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "config set" in text + + +def test_enable_observer_doc_has_task_launch_reference() -> None: + """Doc must reference task launch (the step that starts a watched session).""" + text = _ENABLE_OBSERVER_DOC.read_text() + # Accept either 'task launch' or 'bach task launch' or '/open' (REPL). + assert "task launch" in text or "/open" in text or "launch" in text.lower() + + +def test_enable_observer_doc_describes_board_dots() -> None: + """Doc must explain what the liveness dots on the board mean.""" + text = _ENABLE_OBSERVER_DOC.read_text() + # The three dot characters used in the board view. + has_running_dot = "●" in text or "running" in text.lower() + has_idle_dot = "◐" in text or "idle" in text.lower() + has_ended_dot = "✓" in text or "ended" in text.lower() + assert has_running_dot + assert has_idle_dot + assert has_ended_dot + + +def test_enable_observer_doc_mentions_needs_human() -> None: + """Doc must mention the needs_human / needs you indicator.""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "needs" in text.lower() and ("human" in text.lower() or "you" in text.lower()) + + +def test_enable_observer_doc_has_quick_steps_section() -> None: + """Doc must have a numbered or clear step sequence (the 2-minute path).""" + text = _ENABLE_OBSERVER_DOC.read_text() + # Accept either a numbered list '1.' or explicit step headings '## Step'. + assert "1." in text or "## Step" in text or "## 1" in text + + +def test_enable_observer_doc_mentions_hooks_status() -> None: + """Doc must tell the user how to verify installation (hooks status).""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "hooks status" in text + + +def test_enable_observer_doc_mentions_config_show() -> None: + """Doc must tell the user how to inspect current config (config show).""" + text = _ENABLE_OBSERVER_DOC.read_text() + assert "config show" in text + + +# --------------------------------------------------------------------------- +# CLAUDE.md — new PRD#8 commands in the command list +# --------------------------------------------------------------------------- + + +def test_claude_md_has_config_set_command() -> None: + """CLAUDE.md command list must include `bach config set`.""" + text = _CLAUDE_MD.read_text() + assert "config set" in text + + +def test_claude_md_has_hooks_install_with_project() -> None: + """CLAUDE.md command list must document --project flag on hooks install.""" + text = _CLAUDE_MD.read_text() + assert "--project" in text + + +# --------------------------------------------------------------------------- +# AGENTS.md — new PRD#8 commands in the command list +# --------------------------------------------------------------------------- + + +def test_agents_md_has_config_set_command() -> None: + """AGENTS.md command list must include `bach config set`.""" + text = _AGENTS_MD.read_text() + assert "config set" in text + + +def test_agents_md_has_hooks_install_with_project() -> None: + """AGENTS.md command list must document --project flag on hooks install.""" + text = _AGENTS_MD.read_text() + assert "--project" in text + + +# --------------------------------------------------------------------------- +# progress.txt — PRD#8 entry appended +# --------------------------------------------------------------------------- + + +def test_progress_txt_has_prd8_entry() -> None: + """docs/internal/progress.txt must have a PRD#8 / Board-Radar entry.""" + text = _PROGRESS_TXT.read_text() + # Accept any of: 'PRD#8', 'board-radar', 'Board × Radar', 'Phase 17'. + has_marker = any( + marker in text + for marker in ( + "PRD#8", + "board-radar", + "Board × Radar", + "Board Radar", + "Phase 17", + "board_liveness", + ) + ) + assert has_marker, "progress.txt must include a PRD#8 / Board-Radar progress entry" + + +def test_progress_txt_mentions_observer_enablement() -> None: + """progress.txt PRD#8 entry must mention observer enablement docs.""" + text = _PROGRESS_TXT.read_text() + assert "observer" in text.lower() or "enable-observer" in text.lower() diff --git a/tests/unit/test_hooks_cmd_project.py b/tests/unit/test_hooks_cmd_project.py new file mode 100644 index 0000000..75895ab --- /dev/null +++ b/tests/unit/test_hooks_cmd_project.py @@ -0,0 +1,298 @@ +"""Tests for `--project ` resolution in `bach hooks install` and `bach hooks status`. + +The service layer (hooks_service.py) is unchanged — these tests verify only +the CLI wiring: project name → ProjectRegistry → project.path → project_dir. + +Strategy: + - Inject a fake ProjectRegistry via unittest.mock.patch so tests never + touch the real ~/.bach/projects.yaml. + - Patch _resolve_bach_exe so tests never need a real bach binary on disk. + - Use typer.testing.CliRunner for CLI invocation (no subprocess, no tty). + - One assertion per test. +""" + +import json +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from typer.testing import CliRunner + +from bach.cli.app import app +from bach.domain.models import Project + +_runner = CliRunner() + +# Fake absolute path returned by _resolve_bach_exe in all CLI tests. +_FAKE_EXE = "/fake/venv/bin/bach" +_RESOLVE_PATCH = "bach.services.hooks_service._resolve_bach_exe" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_registry(projects: dict[str, Project]) -> MagicMock: + """Return a mock ProjectRegistry that serves the given project dict.""" + registry = MagicMock() + data = MagicMock() + data.projects = projects + registry.load.return_value = data + + def _get(key_or_name: str) -> Project: + # Mirror the real ProjectRegistry.get: try slug then name. + for proj in projects.values(): + if proj.key == key_or_name or proj.name.lower() == key_or_name.lower(): + return proj + raise KeyError(f"Unknown Bach project: {key_or_name}") + + registry.get.side_effect = _get + return registry + + +def _write_claude_settings(project_dir: Path, data: dict[str, Any]) -> None: + """Write .claude/settings.json under project_dir.""" + settings_path = project_dir / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text(json.dumps(data)) + + +def _all_commands_in_event(settings: dict[str, Any], event_name: str) -> list[str]: + """Extract all command strings from nested hook groups for an event. + + Handles the REAL Claude Code schema (matcher-group objects). + """ + cmds: list[str] = [] + for group in settings.get("hooks", {}).get(event_name, []): + if not isinstance(group, dict): + continue + for h in group.get("hooks", []): + if isinstance(h, dict) and "command" in h: + cmds.append(h["command"]) + return cmds + + +# --------------------------------------------------------------------------- +# `bach hooks install claude-code --project ` — known project +# --------------------------------------------------------------------------- + + +def test_install_with_project_resolves_to_project_path(tmp_path: Path) -> None: + """--project should resolve to the registered project's path.""" + proj = Project(key="myproj", name="MyProj", path=tmp_path) + registry = _make_registry({"myproj": proj}) + + with ( + patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls, + patch(_RESOLVE_PATCH, return_value=_FAKE_EXE), + ): + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "myproj"]) + + # Install should succeed (exit 0). + assert result.exit_code == 0, result.output + # The .claude/settings.json must have been written under the project path. + assert (tmp_path / ".claude" / "settings.json").exists() + + +def test_install_with_project_name_case_insensitive(tmp_path: Path) -> None: + """Project lookup is case-insensitive (mirrors ProjectRegistry.get behaviour).""" + proj = Project(key="myproj", name="MyProj", path=tmp_path) + registry = _make_registry({"myproj": proj}) + + with ( + patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls, + patch(_RESOLVE_PATCH, return_value=_FAKE_EXE), + ): + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "MYPROJ"]) + + assert result.exit_code == 0, result.output + + +def test_install_with_project_is_idempotent(tmp_path: Path) -> None: + """Running install twice via --project must not duplicate hook entries.""" + proj = Project(key="myproj", name="MyProj", path=tmp_path) + registry = _make_registry({"myproj": proj}) + + for _ in range(2): + with ( + patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls, + patch(_RESOLVE_PATCH, return_value=_FAKE_EXE), + ): + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "myproj"]) + assert result.exit_code == 0, result.output + + # No duplicate commands in the nested hook groups. + settings = json.loads((tmp_path / ".claude" / "settings.json").read_text()) + for event_name in ("SessionEnd", "Stop", "PostToolUse"): + cmds = _all_commands_in_event(settings, event_name) + assert len(cmds) == len(set(cmds)), ( + f"Duplicate command found under {event_name} after idempotent install: {cmds}" + ) + + +# --------------------------------------------------------------------------- +# `bach hooks install claude-code --project ` — unknown project +# --------------------------------------------------------------------------- + + +def test_install_unknown_project_exits_nonzero(tmp_path: Path) -> None: + """--project with an unregistered name must exit with code 1.""" + registry = _make_registry({}) # empty registry + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "ghost"]) + + assert result.exit_code == 1 + + +def test_install_unknown_project_prints_error_message(tmp_path: Path) -> None: + """Unknown --project prints an error containing the name.""" + registry = _make_registry({}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "ghost"]) + + assert "ghost" in result.output + + +def test_install_unknown_project_lists_registered_projects(tmp_path: Path) -> None: + """Unknown --project error must list the registered projects by name.""" + proj = Project(key="realproj", name="RealProj", path=tmp_path) + registry = _make_registry({"realproj": proj}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "ghost"]) + + assert result.exit_code == 1 + # The registered project name should appear in the error output. + assert "RealProj" in result.output or "realproj" in result.output + + +def test_install_unknown_project_does_not_touch_disk(tmp_path: Path) -> None: + """No .claude/settings.json must be created for an unknown project name.""" + registry = _make_registry({}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + _runner.invoke(app, ["hooks", "install", "claude-code", "--project", "ghost"]) + + assert not (tmp_path / ".claude" / "settings.json").exists() + + +# --------------------------------------------------------------------------- +# `bach hooks install claude-code` — no --project flag keeps cwd default +# --------------------------------------------------------------------------- + + +def test_install_without_project_flag_uses_cwd_default(tmp_path: Path) -> None: + """Without --project, install defaults to cwd (--project-dir behaviour unchanged).""" + # We pass --project-dir explicitly so cwd is well-defined in the test. + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + result = _runner.invoke( + app, + ["hooks", "install", "claude-code", "--project-dir", str(tmp_path)], + ) + assert result.exit_code == 0, result.output + assert (tmp_path / ".claude" / "settings.json").exists() + + +# --------------------------------------------------------------------------- +# `bach hooks status --project ` — known project +# --------------------------------------------------------------------------- + + +def test_status_with_project_resolves_to_project_path(tmp_path: Path) -> None: + """--project on status should check the registered project's path.""" + _write_claude_settings(tmp_path, {}) # empty settings, so not installed + proj = Project(key="myproj", name="MyProj", path=tmp_path) + registry = _make_registry({"myproj": proj}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "status", "--project", "myproj"]) + + assert result.exit_code == 0, result.output + # Output should reference the project's .claude/settings.json path. + assert ".claude" in result.output + + +def test_status_with_project_shows_installed_when_hooks_present(tmp_path: Path) -> None: + """Status reports installed when Bach hooks are present in the project dir.""" + # Install first via service so the hooks are actually present. + from bach.services.hooks_service import install_claude_hooks + + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + + proj = Project(key="myproj", name="MyProj", path=tmp_path) + registry = _make_registry({"myproj": proj}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "status", "--project", "myproj"]) + + assert result.exit_code == 0, result.output + # Rich's checkmark ✓ for claude-code installed. + assert "✓" in result.output + + +# --------------------------------------------------------------------------- +# `bach hooks status --project ` — unknown project +# --------------------------------------------------------------------------- + + +def test_status_unknown_project_exits_nonzero(tmp_path: Path) -> None: + """Unknown project name on status must exit with code 1.""" + registry = _make_registry({}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "status", "--project", "ghost"]) + + assert result.exit_code == 1 + + +def test_status_unknown_project_prints_error_message(tmp_path: Path) -> None: + """Unknown --project on status prints an error containing the name.""" + registry = _make_registry({}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "status", "--project", "ghost"]) + + assert "ghost" in result.output + + +def test_status_unknown_project_lists_registered_projects(tmp_path: Path) -> None: + """Unknown --project error on status lists the registered projects.""" + proj = Project(key="realproj", name="RealProj", path=tmp_path) + registry = _make_registry({"realproj": proj}) + + with patch("bach.cli.hooks_cmd.ProjectRegistry") as mock_reg_cls: + mock_reg_cls.default.return_value = registry + result = _runner.invoke(app, ["hooks", "status", "--project", "ghost"]) + + assert result.exit_code == 1 + assert "RealProj" in result.output or "realproj" in result.output + + +# --------------------------------------------------------------------------- +# `bach hooks status` — no --project flag keeps cwd default +# --------------------------------------------------------------------------- + + +def test_status_without_project_flag_runs_without_error(tmp_path: Path) -> None: + """Without --project, status uses --project-dir (cwd default unchanged).""" + result = _runner.invoke( + app, + ["hooks", "status", "--project-dir", str(tmp_path)], + ) + # Even with no .claude/ dir present, status should exit 0 (shows ✗). + assert result.exit_code == 0, result.output diff --git a/tests/unit/test_hooks_service.py b/tests/unit/test_hooks_service.py index 866efa6..25e2179 100644 --- a/tests/unit/test_hooks_service.py +++ b/tests/unit/test_hooks_service.py @@ -6,11 +6,15 @@ - Idempotency: running install twice must not duplicate entries. - No-clobber: pre-existing unrelated hooks must survive intact. - Tests assert ONE behaviour per test. + - Schema assertions use the REAL Claude Code settings.json shape: + {"matcher": "", "hooks": [{"type": "command", "command": " internal ..."}]} + NOT the old wrong shape {"command": "...", "run": "always"}. """ import json from pathlib import Path from typing import Any +from unittest.mock import patch from bach.services.hooks_service import ( HookInstallResult, @@ -19,6 +23,13 @@ install_codex_hooks, ) +# Fake absolute path used across tests so _resolve_bach_exe() never hits disk. +_FAKE_EXE = "/fake/venv/bin/bach" + +# Patch target: _resolve_bach_exe lives inside hooks_service module. +_RESOLVE_PATCH = "bach.services.hooks_service._resolve_bach_exe" + + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -47,6 +58,21 @@ def _read_codex_hooks(codex_home: Path) -> dict[str, Any]: return data +def _all_commands_in_event(settings: dict[str, Any], event_name: str) -> list[str]: + """Extract all 'command' strings from the nested hook groups for an event. + + The real Claude Code schema (nested matcher-group objects). + """ + cmds: list[str] = [] + for group in settings.get("hooks", {}).get(event_name, []): + if not isinstance(group, dict): + continue + for h in group.get("hooks", []): + if isinstance(h, dict) and "command" in h: + cmds.append(h["command"]) + return cmds + + # --------------------------------------------------------------------------- # install_claude_hooks — basic install # --------------------------------------------------------------------------- @@ -54,73 +80,173 @@ def _read_codex_hooks(codex_home: Path) -> dict[str, Any]: def test_install_claude_hooks_creates_settings_file(tmp_path: Path) -> None: """With no pre-existing settings.json, the file is created.""" - result = install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + result = install_claude_hooks(project_dir=tmp_path) assert (tmp_path / ".claude" / "settings.json").exists() assert isinstance(result, HookInstallResult) +def test_install_claude_hooks_uses_real_schema(tmp_path: Path) -> None: + """The written entries must use the REAL Claude Code nested schema. + + Each event's entries must be matcher-group objects: + {"matcher": "...", "hooks": [{"type": "command", "command": "..."}]} + NOT the old flat schema {"command": "...", "run": "always"}. + """ + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + settings = _read_claude_settings(tmp_path) + for event_name in ("SessionEnd", "Stop", "PostToolUse"): + for group in settings["hooks"][event_name]: + # Every Bach-owned entry must be a matcher-group. + assert "hooks" in group, ( + f"Entry in {event_name} is not a matcher-group: {group}" + ) + assert "run" not in group, ( + f"Old-schema 'run' key found in {event_name}: {group}" + ) + for h in group["hooks"]: + assert h.get("type") == "command", ( + f"Inner hook type is not 'command' in {event_name}: {h}" + ) + + +def test_install_claude_hooks_command_uses_absolute_path(tmp_path: Path) -> None: + """All installed commands must use the absolute path returned by _resolve_bach_exe.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + settings = _read_claude_settings(tmp_path) + for event_name in ("SessionEnd", "Stop", "PostToolUse"): + cmds = _all_commands_in_event(settings, event_name) + for cmd in cmds: + assert cmd.startswith(_FAKE_EXE), ( + f"Command in {event_name} does not start with absolute exe: {cmd}" + ) + + def test_install_claude_hooks_adds_session_end_entry(tmp_path: Path) -> None: - """SessionEnd hook must be present after install.""" - install_claude_hooks(project_dir=tmp_path) + """SessionEnd hook must have both session-ended and hook-event commands.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) settings = _read_claude_settings(tmp_path) - hooks = settings.get("hooks", {}) - session_end = hooks.get("SessionEnd", []) - # At least one entry in SessionEnd calls bach internal hook-event. - commands = [ - entry.get("command", "") if isinstance(entry, dict) else "" for entry in session_end - ] - assert any("bach internal hook-event" in cmd for cmd in commands) + cmds = _all_commands_in_event(settings, "SessionEnd") + assert any("internal session-ended" in c for c in cmds), ( + f"session-ended not found in SessionEnd commands: {cmds}" + ) + assert any("internal hook-event" in c for c in cmds), ( + f"hook-event not found in SessionEnd commands: {cmds}" + ) def test_install_claude_hooks_adds_stop_entry(tmp_path: Path) -> None: - """Stop hook must be present after install.""" - install_claude_hooks(project_dir=tmp_path) + """Stop hook must have both hook-event and session-stop commands.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + settings = _read_claude_settings(tmp_path) + cmds = _all_commands_in_event(settings, "Stop") + assert any("internal hook-event" in c for c in cmds), ( + f"hook-event not found in Stop commands: {cmds}" + ) + assert any("internal session-stop" in c for c in cmds), ( + f"session-stop not found in Stop commands: {cmds}" + ) + + +def test_install_claude_hooks_adds_post_tool_use_entry(tmp_path: Path) -> None: + """PostToolUse hook must have hook-event command.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) settings = _read_claude_settings(tmp_path) - hooks = settings.get("hooks", {}) - stop = hooks.get("Stop", []) - commands = [entry.get("command", "") if isinstance(entry, dict) else "" for entry in stop] - assert any("bach internal hook-event" in cmd for cmd in commands) + cmds = _all_commands_in_event(settings, "PostToolUse") + assert any("internal hook-event" in c for c in cmds), ( + f"hook-event not found in PostToolUse commands: {cmds}" + ) def test_install_claude_hooks_is_idempotent(tmp_path: Path) -> None: """Running install twice must not duplicate hook entries.""" - install_claude_hooks(project_dir=tmp_path) - install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + install_claude_hooks(project_dir=tmp_path) settings = _read_claude_settings(tmp_path) - hooks = settings.get("hooks", {}) - # No list should have duplicate bach entries. - for event_name, entries in hooks.items(): - bach_cmds = [ - e.get("command", "") - for e in entries - if isinstance(e, dict) and "bach" in e.get("command", "") - ] - assert len(bach_cmds) == len(set(bach_cmds)), ( - f"Duplicate bach hook command found under {event_name}" + # No event should have duplicate command strings. + for event_name in ("SessionEnd", "Stop", "PostToolUse"): + cmds = _all_commands_in_event(settings, event_name) + assert len(cmds) == len(set(cmds)), ( + f"Duplicate command found under {event_name}: {cmds}" ) def test_install_claude_hooks_preserves_unrelated_hooks(tmp_path: Path) -> None: - """Pre-existing user hooks must survive — never clobber unrelated hooks.""" + """Pre-existing user hooks in the NEW schema must survive — never clobber.""" + # A user hook written in the correct nested schema. + user_group = {"matcher": "", "hooks": [{"type": "command", "command": "echo user_hook"}]} _write_claude_settings( - tmp_path, {"hooks": {"SessionEnd": [{"command": "echo user_hook", "run": "always"}]}} + tmp_path, {"hooks": {"SessionEnd": [user_group]}} ) - install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) settings = _read_claude_settings(tmp_path) session_end = settings["hooks"]["SessionEnd"] - commands = [e.get("command", "") for e in session_end if isinstance(e, dict)] - assert "echo user_hook" in commands, "User hook was removed" + all_cmds: list[str] = [] + for group in session_end: + if isinstance(group, dict): + for h in group.get("hooks", []): + if isinstance(h, dict): + all_cmds.append(h.get("command", "")) + assert "echo user_hook" in all_cmds, f"User hook was removed. Commands: {all_cmds}" def test_install_claude_hooks_works_with_empty_settings(tmp_path: Path) -> None: """An empty settings.json ({}) is a valid base — don't fail on it.""" _write_claude_settings(tmp_path, {}) - result = install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + result = install_claude_hooks(project_dir=tmp_path) assert isinstance(result, HookInstallResult) settings = _read_claude_settings(tmp_path) assert "hooks" in settings +def test_install_claude_hooks_migrates_old_schema_entries(tmp_path: Path) -> None: + """Old-schema Bach entries (flat {"command", "run"}) must be removed on install. + + Claude Code silently ignores the old format. Running install must + replace them with correctly-shaped matcher-group entries. + """ + old_entry = {"command": "/old/venv/bin/bach internal hook-event", "run": "always"} + _write_claude_settings( + tmp_path, {"hooks": {"PostToolUse": [old_entry]}} + ) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + settings = _read_claude_settings(tmp_path) + post_tool = settings["hooks"]["PostToolUse"] + # Old entry must be gone. + for entry in post_tool: + assert "run" not in entry, f"Old-schema entry survived: {entry}" + # New correct entry must be present. + cmds = _all_commands_in_event(settings, "PostToolUse") + assert any("internal hook-event" in c for c in cmds) + + +def test_install_claude_hooks_result_reports_added(tmp_path: Path) -> None: + """HookInstallResult.added should list all newly written entries.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + result = install_claude_hooks(project_dir=tmp_path) + # SessionEnd(2) + Stop(2) + PostToolUse(1) = 5 total commands. + assert len(result.added) == 5 + assert result.already_present == () + + +def test_install_claude_hooks_result_reports_already_present_on_rerun(tmp_path: Path) -> None: + """Second install must report all entries as already_present, nothing added.""" + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) + result2 = install_claude_hooks(project_dir=tmp_path) + assert result2.added == () + assert len(result2.already_present) == 5 + + # --------------------------------------------------------------------------- # install_codex_hooks — basic install # --------------------------------------------------------------------------- @@ -129,7 +255,8 @@ def test_install_claude_hooks_works_with_empty_settings(tmp_path: Path) -> None: def test_install_codex_hooks_creates_hooks_file(tmp_path: Path) -> None: """With no pre-existing hooks.json, the file is created.""" codex_home = tmp_path / ".codex" - result = install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + result = install_codex_hooks(codex_home=codex_home) assert (codex_home / "hooks.json").exists() assert isinstance(result, HookInstallResult) @@ -137,10 +264,9 @@ def test_install_codex_hooks_creates_hooks_file(tmp_path: Path) -> None: def test_install_codex_hooks_adds_session_stop_entry(tmp_path: Path) -> None: """A sessionStop entry calling bach internal hook-event is added.""" codex_home = tmp_path / ".codex" - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_codex_hooks(codex_home=codex_home) data = _read_codex_hooks(codex_home) - # Codex hooks.json: top-level keys are event names, values are lists of - # command strings or dicts with command fields. entries = data.get("sessionStop", data.get("stop", [])) cmds = [e.get("command", e) if isinstance(e, dict) else e for e in entries] assert any("bach internal hook-event" in str(c) for c in cmds) @@ -149,8 +275,9 @@ def test_install_codex_hooks_adds_session_stop_entry(tmp_path: Path) -> None: def test_install_codex_hooks_is_idempotent(tmp_path: Path) -> None: """Running install twice must not duplicate entries in hooks.json.""" codex_home = tmp_path / ".codex" - install_codex_hooks(codex_home=codex_home) - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_codex_hooks(codex_home=codex_home) + install_codex_hooks(codex_home=codex_home) data = _read_codex_hooks(codex_home) for event_name, entries in data.items(): bach_entries = [ @@ -166,7 +293,8 @@ def test_install_codex_hooks_preserves_unrelated_hooks(tmp_path: Path) -> None: """Pre-existing user hooks survive codex install — never clobber.""" codex_home = tmp_path / ".codex" _write_codex_hooks(codex_home, {"sessionStop": [{"command": "echo user_hook"}]}) - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_codex_hooks(codex_home=codex_home) data = _read_codex_hooks(codex_home) entries = data.get("sessionStop", data.get("stop", [])) cmds = [e.get("command", e) if isinstance(e, dict) else e for e in entries] @@ -189,14 +317,14 @@ def test_get_hooks_status_shows_not_installed_before_install(tmp_path: Path) -> """Before install, neither runtime reports installed.""" codex_home = tmp_path / ".codex" status = get_hooks_status(project_dir=tmp_path, codex_home=codex_home) - # At minimum, the keys are present with falsy values. assert not status.get("claude_installed", True) assert not status.get("codex_installed", True) def test_get_hooks_status_shows_installed_after_install(tmp_path: Path) -> None: """After install, the relevant key becomes truthy.""" - install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_claude_hooks(project_dir=tmp_path) codex_home = tmp_path / ".codex" status = get_hooks_status(project_dir=tmp_path, codex_home=codex_home) assert status.get("claude_installed") @@ -205,11 +333,34 @@ def test_get_hooks_status_shows_installed_after_install(tmp_path: Path) -> None: def test_get_hooks_status_codex_installed_after_install(tmp_path: Path) -> None: """After codex install, codex_installed is truthy.""" codex_home = tmp_path / ".codex" - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + install_codex_hooks(codex_home=codex_home) status = get_hooks_status(project_dir=tmp_path, codex_home=codex_home) assert status.get("codex_installed") +def test_get_hooks_status_old_schema_not_counted_as_installed(tmp_path: Path) -> None: + """Old-schema flat entries must NOT be counted as installed by get_hooks_status. + + The old {"command": "...", "run": "always"} format is silently ignored by + Claude Code. Reporting them as installed would give false confidence. + """ + # Write old-schema entries for all three events. + old_hooks: dict[str, Any] = { + "hooks": { + ev: [{"command": "/old/bach internal hook-event", "run": "always"}] + for ev in ("SessionEnd", "Stop", "PostToolUse") + } + } + _write_claude_settings(tmp_path, old_hooks) + codex_home = tmp_path / ".codex" + status = get_hooks_status(project_dir=tmp_path, codex_home=codex_home) + # Old schema entries must NOT satisfy the install check. + assert not status.get("claude_installed"), ( + "Old-schema entries were incorrectly counted as installed" + ) + + # --------------------------------------------------------------------------- # install_claude_hooks — abort on malformed settings.json (WATCH finding) # --------------------------------------------------------------------------- @@ -228,8 +379,9 @@ def test_install_claude_hooks_raises_on_malformed_json(tmp_path: Path) -> None: settings_path.parent.mkdir(parents=True, exist_ok=True) settings_path.write_text("this is not valid json {{{") - with pytest.raises(RuntimeError, match="settings.json"): - install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + with pytest.raises(RuntimeError, match="settings.json"): + install_claude_hooks(project_dir=tmp_path) # The malformed file must NOT have been overwritten. assert "this is not valid json" in settings_path.read_text() @@ -244,8 +396,9 @@ def test_install_claude_hooks_does_not_write_on_parse_failure(tmp_path: Path) -> original_content = "not json at all" settings_path.write_text(original_content) - with pytest.raises(RuntimeError): - install_claude_hooks(project_dir=tmp_path) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + with pytest.raises(RuntimeError): + install_claude_hooks(project_dir=tmp_path) # Content must be exactly unchanged. assert settings_path.read_text() == original_content @@ -265,8 +418,9 @@ def test_install_codex_hooks_raises_on_malformed_json(tmp_path: Path) -> None: hooks_path = codex_home / "hooks.json" hooks_path.write_text("{ broken json !") - with pytest.raises(RuntimeError, match="hooks.json"): - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + with pytest.raises(RuntimeError, match="hooks.json"): + install_codex_hooks(codex_home=codex_home) # The malformed file must NOT have been overwritten. assert "broken json" in hooks_path.read_text() @@ -282,8 +436,9 @@ def test_install_codex_hooks_does_not_write_on_parse_failure(tmp_path: Path) -> original_content = "not json" hooks_path.write_text(original_content) - with pytest.raises(RuntimeError): - install_codex_hooks(codex_home=codex_home) + with patch(_RESOLVE_PATCH, return_value=_FAKE_EXE): + with pytest.raises(RuntimeError): + install_codex_hooks(codex_home=codex_home) assert hooks_path.read_text() == original_content diff --git a/tests/unit/test_repl_app.py b/tests/unit/test_repl_app.py index 5e4b49a..b226492 100644 --- a/tests/unit/test_repl_app.py +++ b/tests/unit/test_repl_app.py @@ -32,7 +32,9 @@ def _registry_with_project(tmp_path: Path, name: str = "Demo") -> ProjectRegistr registry = ProjectRegistry(tmp_path / "registry.yaml") data = registry.load() data.projects[name.lower()] = Project( - key=name.lower(), name=name, path=project_path, + key=name.lower(), + name=name, + path=project_path, ) registry.save(data) return registry @@ -150,9 +152,7 @@ def test_add_happy_path_calls_task_service(tmp_path: Path) -> None: # Patch TaskService so we don't actually write artifacts during the test. with patch("bach.repl.app.TaskService") as ServiceCls: - ServiceCls.return_value.add_task.return_value.artifact_path = Path( - "/tmp/fake/task.md" - ) + ServiceCls.return_value.add_task.return_value.artifact_path = Path("/tmp/fake/task.md") app.run() ServiceCls.return_value.add_task.assert_called_once() kwargs = ServiceCls.return_value.add_task.call_args.kwargs @@ -281,9 +281,7 @@ def test_start_command_dispatches_on_first_iteration(tmp_path: Path) -> None: start_command="/add", ) with patch("bach.repl.app.TaskService") as ServiceCls: - ServiceCls.return_value.add_task.return_value.artifact_path = Path( - "/tmp/fake/task.md" - ) + ServiceCls.return_value.add_task.return_value.artifact_path = Path("/tmp/fake/task.md") app.run() ServiceCls.return_value.add_task.assert_called_once() @@ -335,12 +333,14 @@ def test_alias_question_mark_resolves_to_help( def test_help_command_detail_renders_synopsis( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/help normalize prints the synopsis line of the normalize entry.""" registry = _registry_with_project(tmp_path) app = BachRepl( - registry, input_fn=_scripted_input(["/help normalize", "/quit"]), + registry, + input_fn=_scripted_input(["/help normalize", "/quit"]), ) app.run() out = capsys.readouterr().out @@ -353,12 +353,14 @@ def test_help_command_detail_renders_synopsis( def test_help_command_detail_with_leading_slash( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/help /normalize works identically to /help normalize.""" registry = _registry_with_project(tmp_path) app = BachRepl( - registry, input_fn=_scripted_input(["/help /normalize", "/quit"]), + registry, + input_fn=_scripted_input(["/help /normalize", "/quit"]), ) app.run() out = capsys.readouterr().out @@ -366,7 +368,8 @@ def test_help_command_detail_with_leading_slash( def test_help_alias_resolves_to_canonical_detail( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/help n (the alias) shows the normalize detail page.""" registry = _registry_with_project(tmp_path) @@ -377,12 +380,14 @@ def test_help_alias_resolves_to_canonical_detail( def test_help_two_token_form_last_token_wins( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/help tasks normalize falls through to the normalize detail page.""" registry = _registry_with_project(tmp_path) app = BachRepl( - registry, input_fn=_scripted_input(["/help tasks normalize", "/quit"]), + registry, + input_fn=_scripted_input(["/help tasks normalize", "/quit"]), ) app.run() out = capsys.readouterr().out @@ -390,7 +395,8 @@ def test_help_two_token_form_last_token_wins( def test_help_unknown_topic_prints_friendly_error( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/help nonexistent points the user back at /help.""" registry = _registry_with_project(tmp_path) @@ -404,7 +410,8 @@ def test_help_unknown_topic_prints_friendly_error( def test_help_no_arg_still_renders_compact_overview( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """`/help` with no args must still show the compact overview (no regression).""" registry = _registry_with_project(tmp_path) @@ -419,12 +426,14 @@ def test_help_no_arg_still_renders_compact_overview( def test_help_existing_category_handler_still_fires( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """`/help tasks` uses the legacy _HELP_TOPIC_FNS handler (not detail page).""" registry = _registry_with_project(tmp_path) app = BachRepl( - registry, input_fn=_scripted_input(["/help tasks", "/quit"]), + registry, + input_fn=_scripted_input(["/help tasks", "/quit"]), ) app.run() out = capsys.readouterr().out @@ -440,16 +449,13 @@ def test_every_registered_command_has_a_help_entry() -> None: from bach.repl.app import _HANDLERS from bach.repl.help_content import COMMAND_HELP - missing = [ - name.lstrip("/") - for name in _HANDLERS - if name.lstrip("/") not in COMMAND_HELP - ] + missing = [name.lstrip("/") for name in _HANDLERS if name.lstrip("/") not in COMMAND_HELP] assert not missing, f"Commands missing help_content entries: {missing}" def test_workflow_cheatsheet_renders_stages_and_obsidian_link( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/workflow prints stage rows + the Obsidian deep-link. @@ -491,7 +497,8 @@ def test_workflow_cheatsheet_renders_stages_and_obsidian_link( def test_workflow_alias_w_routes_to_workflow( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/w is the documented alias for /workflow.""" registry = _registry_with_project(tmp_path) @@ -584,9 +591,7 @@ def test_config_handler_prints_effective_settings( assert "Normalize model:" in text -def test_models_handler_lists_catalog( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: +def test_models_handler_lists_catalog(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: """/models prints each model slug from the catalog.""" from bach.runtimes.codex_discovery import CodexModel @@ -621,10 +626,13 @@ def test_set_model_with_invalid_slug_rejects(tmp_path: Path) -> None: registry, input_fn=_scripted_input(["/set-model bogus-model", "/quit"]), ) - with patch( - "bach.repl.app.list_codex_models", - return_value=[CodexModel(slug="gpt-5.5", display_name="X", description="")], - ), patch("bach.repl.app.write_config_field") as writer: + with ( + patch( + "bach.repl.app.list_codex_models", + return_value=[CodexModel(slug="gpt-5.5", display_name="X", description="")], + ), + patch("bach.repl.app.write_config_field") as writer, + ): app.run() writer.assert_not_called() @@ -639,9 +647,10 @@ def test_set_model_with_valid_slug_writes(tmp_path: Path) -> None: input_fn=_scripted_input(["/set-model gpt-5.5", "/quit"]), ) catalog = [CodexModel(slug="gpt-5.5", display_name="X", description="")] - with patch( - "bach.repl.app.list_codex_models", return_value=catalog - ), patch("bach.repl.app.write_config_field") as writer: + with ( + patch("bach.repl.app.list_codex_models", return_value=catalog), + patch("bach.repl.app.write_config_field") as writer, + ): app.run() writer.assert_called_once_with("normalize_model", "gpt-5.5") @@ -653,9 +662,10 @@ def test_sm_alias_resolves_to_set_model(tmp_path: Path) -> None: registry = _registry_with_project(tmp_path) app = BachRepl(registry, input_fn=_scripted_input(["/sm gpt-5.5", "/quit"])) catalog = [CodexModel(slug="gpt-5.5", display_name="X", description="")] - with patch( - "bach.repl.app.list_codex_models", return_value=catalog - ), patch("bach.repl.app.write_config_field") as writer: + with ( + patch("bach.repl.app.list_codex_models", return_value=catalog), + patch("bach.repl.app.write_config_field") as writer, + ): app.run() writer.assert_called_once() @@ -756,9 +766,7 @@ def test_day_with_no_arg_resets_to_today_from_all_view(tmp_path: Path) -> None: from datetime import datetime as _dt registry = _registry_with_project(tmp_path) - app = BachRepl( - registry, input_fn=_scripted_input(["/list", "/day", "/quit"]) - ) + app = BachRepl(registry, input_fn=_scripted_input(["/list", "/day", "/quit"])) app.run() assert app.current_view == "today" assert app.current_date_str == _dt.now().strftime("%Y-%m-%d") @@ -785,6 +793,7 @@ def test_delete_handler_confirmation_y_deletes_file(tmp_path: Path) -> None: # Create a real artifact on disk so /delete has something to delete. project_path = list(registry.load().projects.values())[0].path from datetime import datetime as _dt + today = _dt.now().strftime("%Y-%m-%d") runs_dir = project_path / ".bach" / "runs" / today runs_dir.mkdir(parents=True, exist_ok=True) @@ -807,6 +816,7 @@ def test_delete_handler_confirmation_n_cancels(tmp_path: Path) -> None: registry = _registry_with_project(tmp_path) project_path = list(registry.load().projects.values())[0].path from datetime import datetime as _dt + today = _dt.now().strftime("%Y-%m-%d") runs_dir = project_path / ".bach" / "runs" / today runs_dir.mkdir(parents=True, exist_ok=True) @@ -826,6 +836,7 @@ def test_edit_handler_invokes_open_in_editor(tmp_path: Path) -> None: registry = _registry_with_project(tmp_path) project_path = list(registry.load().projects.values())[0].path from datetime import datetime as _dt + today = _dt.now().strftime("%Y-%m-%d") runs_dir = project_path / ".bach" / "runs" / today runs_dir.mkdir(parents=True, exist_ok=True) @@ -843,6 +854,7 @@ def test_edit_handler_handles_no_editor_gracefully(tmp_path: Path) -> None: registry = _registry_with_project(tmp_path) project_path = list(registry.load().projects.values())[0].path from datetime import datetime as _dt + today = _dt.now().strftime("%Y-%m-%d") runs_dir = project_path / ".bach" / "runs" / today runs_dir.mkdir(parents=True, exist_ok=True) @@ -851,6 +863,7 @@ def test_edit_handler_handles_no_editor_gracefully(tmp_path: Path) -> None: app = BachRepl(registry, input_fn=_scripted_input(["/edit 1", "/quit"])) from bach.utils.editor import EDITOR_NOT_FOUND + with patch("bach.repl.app.open_in_editor", return_value=EDITOR_NOT_FOUND): app.run() # Must not raise. @@ -864,6 +877,7 @@ def _minimal_artifact_yaml(date_str: str, title: str) -> str: files is its own anti-pattern). """ import yaml as _yaml + payload = { "id": "task-100000-test", "date": date_str, @@ -880,9 +894,13 @@ def _minimal_artifact_yaml(date_str: str, title: str) -> str: }, "skill": {"name": "grill-me", "status": "not_started"}, "task": { - "title": title, "original_description": title, - "objective": None, "non_goals": [], "constraints": [], - "likely_files_or_areas": [], "acceptance_checks": [], + "title": title, + "original_description": title, + "objective": None, + "non_goals": [], + "constraints": [], + "likely_files_or_areas": [], + "acceptance_checks": [], "risks_or_unknowns": [], }, "post_grill": {"readiness": "not_ready", "next_action": "undecided"}, @@ -920,6 +938,7 @@ def test_resolver_accepts_stable_id_form(tmp_path: Path) -> None: from io import StringIO from rich.console import Console + registry = _registry_with_project(tmp_path) buf = StringIO() app = BachRepl(registry, input_fn=_scripted_input(["/open t99", "/quit"])) @@ -935,6 +954,7 @@ def test_resolver_rejects_invalid_format(tmp_path: Path) -> None: from io import StringIO from rich.console import Console + registry = _registry_with_project(tmp_path) buf = StringIO() app = BachRepl(registry, input_fn=_scripted_input(["/open foo", "/quit"])) @@ -948,6 +968,7 @@ def test_resolver_missing_arg_friendly_error(tmp_path: Path) -> None: from io import StringIO from rich.console import Console + registry = _registry_with_project(tmp_path) buf = StringIO() app = BachRepl(registry, input_fn=_scripted_input(["/open", "/quit"])) @@ -972,6 +993,7 @@ def _make_task_artifact(tmp_path: Path, status: str = "grilling") -> Path: from datetime import datetime as _dt import yaml as _yaml + project_dir = tmp_path / "DemoProject" project_dir.mkdir(exist_ok=True) (project_dir / ".bach").mkdir(exist_ok=True) @@ -983,34 +1005,42 @@ def _make_task_artifact(tmp_path: Path, status: str = "grilling") -> Path: date_dir.mkdir(parents=True, exist_ok=True) artifact = date_dir / "task-100000-x.md" payload = { - "id": "task-100000-x", "date": today, "status": status, + "id": "task-100000-x", + "date": today, + "status": status, "project": {"name": "DemoProject", "path": str(project_dir)}, "agent": { - "runtime": "claude-code", "bach_session_id": "bach-x", - "runtime_session_id": "uuid-x", "runtime_session_name": "bach-x", - "launch_command": None, "resume_command": None, + "runtime": "claude-code", + "bach_session_id": "bach-x", + "runtime_session_id": "uuid-x", + "runtime_session_name": "bach-x", + "launch_command": None, + "resume_command": None, "restart_command": None, }, "skill": {"name": "grill-me", "status": "not_started"}, "task": { - "title": "Demo task", "original_description": "Demo task", - "objective": None, "non_goals": [], "constraints": [], - "likely_files_or_areas": [], "acceptance_checks": [], + "title": "Demo task", + "original_description": "Demo task", + "objective": None, + "non_goals": [], + "constraints": [], + "likely_files_or_areas": [], + "acceptance_checks": [], "risks_or_unknowns": [], }, "post_grill": {"readiness": "not_ready", "next_action": "undecided"}, "log": [{"timestamp": f"{today}T00:00:00+00:00", "event": "created"}], } body = "\n## Grill Transcript\n\n## Final Task Contract\n\n## Carry Forward\n" - artifact.write_text( - f"---\n{_yaml.safe_dump(payload, sort_keys=False)}---\n{body}" - ) + artifact.write_text(f"---\n{_yaml.safe_dump(payload, sort_keys=False)}---\n{body}") return artifact def _read_status_field(artifact: Path) -> str: """Helper: parse + return current status.""" import yaml as _yaml + return str(_yaml.safe_load(artifact.read_text().split("---")[1])["status"]) @@ -1054,6 +1084,7 @@ def test_status_handler_no_op_when_already_target(tmp_path: Path) -> None: registry.save(data) before_mtime = artifact.stat().st_mtime import time + time.sleep(0.01) app = BachRepl(registry, input_fn=_scripted_input(["/done 1", "/quit"])) app.run() @@ -1240,9 +1271,7 @@ def test_esc_esc_handler_calls_buffer_reset() -> None: kb = _build_key_bindings() assert kb is not None # Pull out the handler for the Esc-Esc binding. - esc_esc = next( - b for b in kb.bindings if tuple(b.keys) == (Keys.Escape, Keys.Escape) - ) + esc_esc = next(b for b in kb.bindings if tuple(b.keys) == (Keys.Escape, Keys.Escape)) fake_event = MagicMock() esc_esc.handler(fake_event) fake_event.current_buffer.reset.assert_called_once() @@ -1321,9 +1350,7 @@ def test_board_project_filter_stashed_on_app(tmp_path: Path) -> None: def test_board_unknown_project_keeps_view_unchanged(tmp_path: Path) -> None: """Bad arg → friendly error, view stays where it was (no surprise transition).""" registry = _registry_with_project(tmp_path) - app = BachRepl( - registry, input_fn=_scripted_input(["/board nosuchproject", "/quit"]) - ) + app = BachRepl(registry, input_fn=_scripted_input(["/board nosuchproject", "/quit"])) pre_view = app.current_view app.run() assert app.current_view == pre_view # didn't enter board view @@ -1332,9 +1359,7 @@ def test_board_unknown_project_keeps_view_unchanged(tmp_path: Path) -> None: def test_list_exits_board_view(tmp_path: Path) -> None: """/board → /list returns to the all-time list view.""" registry = _registry_with_project(tmp_path) - app = BachRepl( - registry, input_fn=_scripted_input(["/board", "/list", "/quit"]) - ) + app = BachRepl(registry, input_fn=_scripted_input(["/board", "/list", "/quit"])) app.run() assert app.current_view == "all" @@ -1342,9 +1367,7 @@ def test_list_exits_board_view(tmp_path: Path) -> None: def test_day_exits_board_view(tmp_path: Path) -> None: """/board → /day returns to today's date view.""" registry = _registry_with_project(tmp_path) - app = BachRepl( - registry, input_fn=_scripted_input(["/board", "/day", "/quit"]) - ) + app = BachRepl(registry, input_fn=_scripted_input(["/board", "/day", "/quit"])) app.run() assert app.current_view == "today" @@ -1360,7 +1383,8 @@ def test_positional_ref_rejected_in_board_view(tmp_path: Path) -> None: registry = ProjectRegistry(tmp_path / "registry.yaml") data = registry.load() data.projects["demoproject"] = Project( - key="demoproject", name="DemoProject", + key="demoproject", + name="DemoProject", path=artifact.parent.parent.parent.parent, ) registry.save(data) @@ -1396,7 +1420,8 @@ def test_stable_id_ref_works_in_board_view(tmp_path: Path) -> None: registry = ProjectRegistry(tmp_path / "registry.yaml") data = registry.load() data.projects["demoproject"] = Project( - key="demoproject", name="DemoProject", + key="demoproject", + name="DemoProject", path=artifact.parent.parent.parent.parent, ) registry.save(data) @@ -1418,9 +1443,7 @@ def test_board_view_persists_across_loop_iterations(tmp_path: Path) -> None: if view-state weren't sticky, we'd land somewhere else. """ registry = _registry_with_project(tmp_path) - app = BachRepl( - registry, input_fn=_scripted_input(["/board", "/help", "/quit"]) - ) + app = BachRepl(registry, input_fn=_scripted_input(["/board", "/help", "/quit"])) app.run() assert app.current_view == "board" @@ -1431,7 +1454,8 @@ def test_board_view_persists_across_loop_iterations(tmp_path: Path) -> None: def test_clear_command_emits_ansi_wipe( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/clear writes the ANSI viewport-and-scrollback wipe to stdout. @@ -1447,7 +1471,8 @@ def test_clear_command_emits_ansi_wipe( def test_clear_alias_c_routes_to_clear( - tmp_path: Path, capsys: pytest.CaptureFixture[str], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """/c is the documented one-letter alias for /clear.""" registry = _registry_with_project(tmp_path) @@ -1490,3 +1515,154 @@ def test_repl_bare_unknown_command_still_errors(tmp_path: Path) -> None: input_fn=_scripted_input(["definitelynotacommand", "quit"]), ) app.run() + + +# =========================================================================== +# Phase 16 (B1) — liveness dots in dashboard list rows and /show observer +# section. These tests use fake LivenessMaps injected via monkeypatch so +# no real discovery or disk I/O runs. +# =========================================================================== + + +def _make_project_with_task(tmp_path: Path, project_name: str, task_id: str) -> ProjectRegistry: + """Build a project + task artifact on disk so /show can discover it. + + Artifacts live at .bach/runs//task-*.md — the date dir + must exist for discover_all_tasks to find the file. Uses a fixed date + (2026-06-11) so the directory path is predictable in tests. + """ + project_path = tmp_path / project_name + date_dir = project_path / ".bach" / "runs" / "2026-06-11" + date_dir.mkdir(parents=True) + artifact = date_dir / f"task-00001-{task_id}.md" + artifact.write_text( + f"---\n" + f"task_id: {task_id}\n" + "status: executing\n" + "title: Demo task\n" + "agent:\n" + " runtime: claude-code\n" + " bach_session_id: bach-x\n" + "---\n" + ) + registry = ProjectRegistry(tmp_path / "registry.yaml") + data = registry.load() + key = project_name.lower() + data.projects[key] = Project(key=key, name=project_name, path=project_path) + registry.save(data) + return registry + + +def test_show_observer_section_when_liveness_has_summary( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """/show with observer data prints an 'Observer' section. + + We monkeypatch liveness_for_tasks to return a fixed LivenessMap so the + test doesn't touch real session discovery paths. + """ + from unittest.mock import patch + + from bach.domain.models import SessionLiveness + from bach.services.board_liveness import TaskLiveness + + registry = _make_project_with_task(tmp_path, "Demo", "t1") + + fake_liveness: dict[str, TaskLiveness] = { + "t1": TaskLiveness( + liveness=SessionLiveness.running, + summary="Implementing auth module", + needs_human=False, + confidence=0.85, + ) + } + + app = BachRepl( + registry, + input_fn=_scripted_input(["/show t1", "/quit"]), + ) + with patch("bach.repl.app.liveness_for_tasks", return_value=fake_liveness): + app.run() + + out = capsys.readouterr().out + # The Observer section heading + summary text should appear in /show output. + assert "Observer" in out + assert "Implementing auth module" in out + + +def test_show_no_observer_section_when_no_summary( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """/show without observer data prints NO Observer section.""" + from unittest.mock import patch + + from bach.domain.models import SessionLiveness + from bach.services.board_liveness import TaskLiveness + + registry = _make_project_with_task(tmp_path, "Demo2", "t2") + + # Liveness exists but no summary → no Observer section. + fake_liveness: dict[str, TaskLiveness] = { + "t2": TaskLiveness( + liveness=SessionLiveness.running, + summary=None, + needs_human=False, + confidence=None, + ) + } + + app = BachRepl( + registry, + input_fn=_scripted_input(["/show t2", "/quit"]), + ) + with patch("bach.repl.app.liveness_for_tasks", return_value=fake_liveness): + app.run() + + out = capsys.readouterr().out + # No observer summary → no Observer heading. + assert "Observer" not in out + + +# --------------------------------------------------------------------------- +# _format_skip_banner — finding #2 regression tests +# --------------------------------------------------------------------------- + + +def test_format_skip_banner_empty_dict_returns_empty_string() -> None: + """Empty skipped dict → '' so no banner is printed.""" + from bach.repl.app import _format_skip_banner + + result = _format_skip_banner({}, "yellow") + assert result == "" + + +def test_format_skip_banner_single_permission_error() -> None: + """One PermissionError project → correct human-readable banner.""" + from bach.repl.app import _format_skip_banner + + result = _format_skip_banner({"skillia": "PermissionError"}, "yellow") + # Must contain the structured pieces, wrapped in the theme color. + assert "⚠" in result + assert "Skipped 1 project:" in result + assert "skillia" in result + assert "permission denied" in result + assert "[yellow]" in result + assert "[/yellow]" in result + + +def test_format_skip_banner_two_projects() -> None: + """Two skipped projects → pluralised noun and both projects listed.""" + from bach.repl.app import _format_skip_banner + + result = _format_skip_banner( + {"alpha": "PermissionError", "beta": "OSError"}, + "#f0c060", + ) + assert "Skipped 2 projects:" in result + assert "alpha" in result + assert "permission denied" in result + assert "beta" in result + # OSError → "os error" (lowercased, no underscores) + assert "os error" in result diff --git a/tests/unit/test_session_watcher.py b/tests/unit/test_session_watcher.py index a869b29..1b501be 100644 --- a/tests/unit/test_session_watcher.py +++ b/tests/unit/test_session_watcher.py @@ -35,7 +35,7 @@ SessionEvent, SessionEventKind, ) -from bach.services.session_watcher import _transcript_path_for, watch_session +from bach.services.session_watcher import _apply_verdict, _transcript_path_for, watch_session # --------------------------------------------------------------------------- # Fixtures and helpers @@ -129,7 +129,7 @@ def _null_judge(**_kwargs: Any) -> None: def _events_iterator( event_batches: list[list[SessionEvent]], -) -> Callable[..., Iterator[SessionEvent]]: +) -> Callable[..., Iterator[SessionEvent | None]]: """ Return a follow_events replacement that yields each batch then stops. @@ -143,11 +143,16 @@ def _follow( *, poll_seconds: float, should_stop: Callable[[], bool], - ) -> Iterator[SessionEvent]: + heartbeat: bool = False, + ) -> Iterator[SessionEvent | None]: for batch in event_batches: if should_stop(): return yield from batch + # Mirror the real generator: a heartbeat tick after each poll + # cycle so spool drain runs even between event batches. + if heartbeat: + yield None return _follow @@ -491,10 +496,12 @@ def _spool_with_late_stop( session_id: str, *, sessions_dir: Path, from_offset: int = 0 ) -> tuple[list[dict[str, Any]], int]: call_count[0] += 1 - # Return stop on the 4th drain call. - if call_count[0] >= 4: + # Return stop exactly once (the real reader is offset-aware and + # never re-delivers consumed events; >= here would double-deliver + # under the heartbeat cadence and fake a second judge trigger). + if call_count[0] == 4 and from_offset == 0: return [{"type": "stop"}], 1 - return [], 0 + return [], from_offset code = watch_session( artifact, @@ -789,3 +796,84 @@ def _high_conf_judge(**_kwargs: Any) -> JudgeVerdict: # And the log entry must have been appended. log = _read_log(artifact) assert any(e.get("event") == "observer_suggestion" for e in log) + + +def test_apply_verdict_rejected_write_still_logs_suggestion(tmp_path: Path) -> None: + """Authority-rejected moves leave a visible observer_suggestion event. + + Regression from dogfooding: a rejected write produced only a WARNING + log line — the verdict vanished from the artifact, so the board and + /show had nothing to display. + """ + artifact = _write_artifact(tmp_path) + + def _rejecting_set_status(*_a: object, **_k: object) -> None: + raise ValueError("observer source may not transition") + + verdict = JudgeVerdict( + summary="agent waiting on human input", + suggested_status="hitl_queue", + needs_human=True, + confidence=0.95, + ) + _apply_verdict( + artifact, + verdict, + config=BachConfig(), + current_status="inbox", + session_id="sess-1", + set_task_status_fn=_rejecting_set_status, + ) + + text = artifact.read_text() + assert "observer_suggestion" in text + assert "write_rejected" in text + + +def test_watcher_drains_spool_when_transcript_is_quiet(tmp_path: Path) -> None: + """A SessionEnd spool event must trigger judge+exit even when the + transcript never grows — regression for the zombie-watcher bug found + while dogfooding (closed session writes nothing; spool starved).""" + artifact = _write_artifact(tmp_path, status="executing") + transcript = tmp_path / "quiet.jsonl" + transcript.write_text("") + + spool_calls = {"n": 0} + + def _spool( + _sid: str, *, sessions_dir: Path, from_offset: int = 0 + ) -> tuple[list[dict[str, Any]], int]: + spool_calls["n"] += 1 + if spool_calls["n"] == 2: + return ([{"hook_event_name": "SessionEnd"}], 1) + return ([], from_offset) + + judged = {"n": 0} + + def _judge(*_a: object, **_k: object) -> JudgeVerdict: + judged["n"] += 1 + return JudgeVerdict( + summary="session ended while grilling", + suggested_status="hitl_queue", + needs_human=True, + confidence=0.9, + ) + + applied: list[tuple[str, str]] = [] + + def _set_status(_artifact: Path, new: str, source: str) -> None: + applied.append((new, source)) + + rc = watch_session( + artifact, + config=BachConfig(), + sessions_dir=tmp_path / "sessions", + judge_fn=_judge, + transcript_path_fn=lambda *a, **k: transcript, + read_spool_events_fn=_spool, + set_task_status_fn=_set_status, + ) + + assert rc == 0 + assert judged["n"] >= 1 + assert ("hitl_queue", "observer") in applied diff --git a/tests/unit/test_sessions_service.py b/tests/unit/test_sessions_service.py index b5ed6f3..7e83d25 100644 --- a/tests/unit/test_sessions_service.py +++ b/tests/unit/test_sessions_service.py @@ -41,6 +41,7 @@ import pytest import yaml +from bach.config.projects import ProjectRegistry from bach.domain.models import ( AgentRuntime, DiscoveredSession, @@ -55,6 +56,7 @@ list_sessions, resume_session, scan_artifact_paths, + scan_registry_artifact_paths, ) # --------------------------------------------------------------------------- @@ -915,3 +917,120 @@ def test_close_session_no_process_found() -> None: result = _close(session, lambda _s: [], killed) assert result.reason == "no_process_found" assert killed == [] + + +# --------------------------------------------------------------------------- +# scan_registry_artifact_paths — ADR-002 registry scan +# --------------------------------------------------------------------------- + + +def _make_registry(tmp_path: Path, projects: dict[str, Path]) -> ProjectRegistry: + """Build a real ProjectRegistry YAML file under tmp_path and return it. + + Each entry in `projects` maps a human name to an absolute project path. + The YAML is written directly to avoid touching the real ~/.bach registry. + """ + registry_path = tmp_path / "registry.yaml" + # Build the YAML by hand to avoid upsert side-effects on a real ~/.bach. + raw: dict[str, Any] = { + "projects": { + name.lower().replace(" ", "-"): {"name": name, "path": str(path)} + for name, path in projects.items() + } + } + registry_path.write_text(yaml.safe_dump(raw, sort_keys=False)) + return ProjectRegistry(registry_path) + + +def test_scan_registry_artifact_paths_finds_across_projects(tmp_path: Path) -> None: + """scan_registry_artifact_paths collects .md artifacts from multiple projects. + + Two registered projects each have their own .bach/ layout; the function + must return all found artifacts regardless of which project they belong to. + """ + # Project A: two artifacts nested under .bach/runs/… + proj_a = tmp_path / "proj_a" + runs_a = proj_a / ".bach" / "runs" / "2026-06-11" + runs_a.mkdir(parents=True) + art_a1 = runs_a / "task-103255-foo.md" + art_a2 = runs_a / "task-103256-bar.md" + art_a1.write_text("# a1") + art_a2.write_text("# a2") + + # Project B: one artifact + proj_b = tmp_path / "proj_b" + runs_b = proj_b / ".bach" / "runs" / "2026-06-11" + runs_b.mkdir(parents=True) + art_b1 = runs_b / "task-203000-baz.md" + art_b1.write_text("# b1") + + registry = _make_registry(tmp_path, {"proj-a": proj_a, "proj-b": proj_b}) + result = scan_registry_artifact_paths(registry) + + found_names = {p.name for p in result} + assert "task-103255-foo.md" in found_names + assert "task-103256-bar.md" in found_names + assert "task-203000-baz.md" in found_names + assert len(result) == 3 + + +def test_scan_registry_artifact_paths_skips_missing_bach_dir(tmp_path: Path) -> None: + """Projects that have never run `bach project init` have no .bach/; skip silently.""" + # Project with a .bach/ dir that has an artifact. + proj_with = tmp_path / "proj_with" + runs = proj_with / ".bach" / "runs" / "2026-06-11" + runs.mkdir(parents=True) + art = runs / "task-001.md" + art.write_text("# art") + + # Project that has never been initialised — no .bach/ at all. + proj_without = tmp_path / "proj_without" + proj_without.mkdir(parents=True) + + registry = _make_registry(tmp_path, {"proj-with": proj_with, "proj-without": proj_without}) + result = scan_registry_artifact_paths(registry) + + # Only the one artifact from the initialised project is returned. + assert len(result) == 1 + assert result[0].name == "task-001.md" + + +def test_list_sessions_with_registry_artifact_paths_links_task_id(tmp_path: Path) -> None: + """Regression: list_sessions with artifact_paths from a project-local .bach links task_id. + + This is the core ADR-002 wiring fix: when artifact_paths comes from + scan_registry_artifact_paths, a session whose runtime_session_id matches + an artifact stored under /.bach/… must have task_id populated. + Previously, scanning bach_home() alone left task_id as None. + """ + sid = str(uuid.uuid4()) + + # Place the artifact under a project-local .bach tree (ADR-002 layout). + proj = tmp_path / "myproject" + runs = proj / ".bach" / "runs" / "2026-06-11" + runs.mkdir(parents=True) + artifact = runs / "task-103255-wiring.md" + # Write a minimal valid artifact using _make_artifact's YAML format. + artifact.write_text( + _make_artifact( + runs, + runtime_session_id=sid, + task_id="t103", + ).read_text() + ) + + registry = _make_registry(tmp_path, {"myproject": proj}) + artifact_paths = scan_registry_artifact_paths(registry) + + session = _make_session(session_id=sid, project_dir=proj) + rows = list_sessions( + sessions_dir=tmp_path / "sessions", + artifacts_dir=tmp_path / "nonexistent", # must NOT be scanned — artifact_paths injected + discover_fn=lambda: [session], + read_events_fn=lambda _p, _r: [], + artifact_paths=artifact_paths, + ) + + assert len(rows) == 1 + assert rows[0].task_id == "t103" + assert rows[0].artifact_path is not None diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index e471a45..acfe2e5 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -534,3 +534,77 @@ def test_load_config_all_new_fields_in_one_file(tmp_path: Path) -> None: assert config.liveness_idle_minutes == 8 assert config.afk_max_turns == 7 assert config.afk_time_budget_minutes == 120 + + +# --------------------------------------------------------------------------- +# set_observer_key — cross-field liveness validation (finding #7) +# --------------------------------------------------------------------------- + + +def _make_config_file(tmp_path: Path, **fields: object) -> Path: + """Write a minimal config file with the given fields and return the path.""" + import yaml as _yaml + + config_path = tmp_path / "config.yaml" + config_path.write_text(_yaml.safe_dump(dict(fields))) + return config_path + + +def test_set_observer_key_liveness_cross_field_passing_case(tmp_path: Path) -> None: + """Setting liveness-running-seconds to a value below idle threshold succeeds.""" + import bach.config.settings as settings_mod + from bach.config.settings import set_observer_key + + # Default: running=60s, idle=5 min (300s). Setting running to 30s passes. + config_path = _make_config_file(tmp_path, liveness_running_seconds=60, liveness_idle_minutes=5) + orig = settings_mod._default_config_path + settings_mod._default_config_path = lambda: config_path + try: + set_observer_key("liveness-running-seconds", "30") + # Reload and check the value was written. + cfg = load_config(path=config_path) + assert cfg.liveness_running_seconds == 30 + finally: + settings_mod._default_config_path = orig + + +def test_set_observer_key_liveness_running_exceeds_idle_raises(tmp_path: Path) -> None: + """Setting liveness-running-seconds >= idle threshold raises ValueError. + + With running=300s and idle=5 min (300s), running >= idle*60, so the + idle state is unreachable. ValueError must be raised before writing. + """ + import bach.config.settings as settings_mod + from bach.config.settings import set_observer_key + + config_path = _make_config_file(tmp_path, liveness_running_seconds=60, liveness_idle_minutes=5) + orig = settings_mod._default_config_path + settings_mod._default_config_path = lambda: config_path + try: + import pytest as _pytest + + with _pytest.raises(ValueError, match="idle state is unreachable"): + set_observer_key("liveness-running-seconds", "300") # 300s == 5min*60 + finally: + settings_mod._default_config_path = orig + + +def test_set_observer_key_liveness_idle_too_small_raises(tmp_path: Path) -> None: + """Setting liveness-idle-minutes so that idle*60 <= running raises ValueError. + + With running=120s and idle=1 min (60s), idle threshold <= running, so + idle state is unreachable. + """ + import bach.config.settings as settings_mod + from bach.config.settings import set_observer_key + + config_path = _make_config_file(tmp_path, liveness_running_seconds=120, liveness_idle_minutes=5) + orig = settings_mod._default_config_path + settings_mod._default_config_path = lambda: config_path + try: + import pytest as _pytest + + with _pytest.raises(ValueError, match="idle state is unreachable"): + set_observer_key("liveness-idle-minutes", "1") # 1 min = 60s < 120s + finally: + settings_mod._default_config_path = orig diff --git a/tests/unit/test_status_service.py b/tests/unit/test_status_service.py index 83041b0..b6b8f6a 100644 --- a/tests/unit/test_status_service.py +++ b/tests/unit/test_status_service.py @@ -391,3 +391,14 @@ def test_human_source_unrestricted_backward(tmp_path: Path) -> None: artifact = _write_minimal_artifact(tmp_path, initial_status="qa") result = set_task_status(artifact, "inbox", source="repl") assert result.new == "inbox" + + +def test_observer_same_status_noop_is_allowed(tmp_path: Path) -> None: + """Observer re-suggesting the current status is benign, not a violation. + + Regression from dogfooding: judge suggested hitl_queue while the task + was already hitl_queue → InvalidStatusError instead of a no-op. + """ + artifact = _write_minimal_artifact(tmp_path, initial_status="hitl_queue") + result = set_task_status(artifact, "hitl_queue", source="observer") + assert result.old == result.new == "hitl_queue" diff --git a/tests/unit/test_task_service.py b/tests/unit/test_task_service.py index 4438225..362ab3d 100644 --- a/tests/unit/test_task_service.py +++ b/tests/unit/test_task_service.py @@ -5,6 +5,9 @@ worth pinning is the `output` callback parameter on `launch_task` — which is what lets the TUI route messages into its log instead of stdout. + +Phase 16 addition: _spawn_session_watcher must redirect stdout/stderr to an +append-mode log file (never DEVNULL), so watcher crashes are diagnosable. """ from pathlib import Path @@ -12,6 +15,7 @@ from unittest.mock import patch from bach.config.projects import ProjectRegistry +from bach.config.settings import BachConfig from bach.domain.models import AgentRuntime, Project, SessionMode from bach.services.task_service import TaskService @@ -22,9 +26,7 @@ def _registry_with_one_project(tmp_path: Path) -> ProjectRegistry: (project_path / ".bach" / "runs").mkdir(parents=True) registry = ProjectRegistry(tmp_path / "registry.yaml") data = registry.load() - data.projects["demoproj"] = Project( - key="demoproj", name="Demoproj", path=project_path - ) + data.projects["demoproj"] = Project(key="demoproj", name="Demoproj", path=project_path) registry.save(data) return registry @@ -48,8 +50,11 @@ def test_launch_task_routes_fallback_message_through_output_callback( # Force launch_in_iterm to fail so we hit the fallback path. captured: list[str] = [] - with patch( - "bach.services.task_service.launch_in_iterm", return_value=False + with ( + patch("bach.services.task_service.launch_in_iterm", return_value=False), + # Pin defaults so a real ~/.bach watch_on_launch=on can't spawn a + # watcher subprocess from a unit test. + patch("bach.services.task_service.load_config", return_value=BachConfig()), ): service.launch_task(task.artifact_path, output=captured.append) @@ -73,11 +78,13 @@ def test_launch_task_default_output_is_typer_echo(tmp_path: Path) -> None: # Patch typer.echo to capture default-callback output. captured: list[str] = [] - with patch( - "bach.services.task_service.launch_in_iterm", return_value=False - ), patch( - "bach.services.task_service.typer.echo", - side_effect=lambda s, *a, **k: captured.append(s), + with ( + patch("bach.services.task_service.launch_in_iterm", return_value=False), + patch("bach.services.task_service.load_config", return_value=BachConfig()), + patch( + "bach.services.task_service.typer.echo", + side_effect=lambda s, *a, **k: captured.append(s), + ), ): # No output= argument — default kicks in. service.launch_task(task.artifact_path) @@ -99,10 +106,146 @@ def test_launch_task_iterm_success_does_not_invoke_output( ) captured: list[Any] = [] - with patch( - "bach.services.task_service.launch_in_iterm", return_value=True + with ( + patch("bach.services.task_service.launch_in_iterm", return_value=True), + # Pin defaults: the real ~/.bach/config.yaml may have watch_on_launch + # on, which would spawn a watcher and print — unit tests never read + # the real home (house rule). + patch("bach.services.task_service.load_config", return_value=BachConfig()), ): service.launch_task(task.artifact_path, output=captured.append) # Successful launch is silent (event logs happen at logger, not output). assert captured == [] + + +def test_watcher_spawn_failure_notifies_user_via_output( + tmp_path: Path, +) -> None: + """When watch_on_launch is on and _spawn_session_watcher fails, the + output callback receives a clear warning — silence would make the user + believe the session is watched when it isn't (finding #8). + """ + registry = _registry_with_one_project(tmp_path) + service = TaskService(registry) + task = service.add_task( + description="Demo task", + project_key="demoproj", + runtime=AgentRuntime.claude_code, + mode=SessionMode.grill_me, + ) + + captured: list[Any] = [] + # Config with watch_on_launch=True so the watcher branch runs. + from bach.config.settings import ItermLayout + from bach.domain.models import AgentRuntime as AR + + cfg = BachConfig( + watch_on_launch=True, + iterm_layout=ItermLayout.tabs, + runtime_default=AR.claude_code, + ) + with ( + patch("bach.services.task_service.launch_in_iterm", return_value=True), + patch("bach.services.task_service.load_config", return_value=cfg), + # _spawn_session_watcher now returns (bool, Path|None); False → (False, None) + patch.object(service, "_spawn_session_watcher", return_value=(False, None)), + ): + service.launch_task(task.artifact_path, output=captured.append) + + # A failure message must have been sent through the output channel. + assert any("spawn failed" in line.lower() or "unwatched" in line.lower() for line in captured) + + +def test_spawn_session_watcher_uses_log_file_not_devnull( + tmp_path: Path, +) -> None: + """_spawn_session_watcher must redirect stdout/stderr to a real file handle. + + Previously both were subprocess.DEVNULL, making watcher crashes completely + invisible. The log file at bach_home()/logs/watcher-.log is the ONLY + trace of the detached process — it must never be silenced. + + Pattern: capture the kwargs passed to subprocess.Popen via a fake Popen. + """ + import subprocess + + registry = _registry_with_one_project(tmp_path) + service = TaskService(registry) + task = service.add_task( + description="Demo task", + project_key="demoproj", + runtime=AgentRuntime.claude_code, + mode=SessionMode.grill_me, + ) + + captured_kwargs: dict[str, Any] = {} + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + captured_kwargs.update(kwargs) + + # Patch bach_home so the log dir lands in tmp_path, not the real ~/.bach. + fake_bach_home = tmp_path / "bach_home" + with ( + patch("bach.services.task_service.bach_home", return_value=fake_bach_home), + patch("bach.services.task_service.subprocess.Popen", _FakePopen), + ): + ok, log_path = service._spawn_session_watcher(task.artifact_path) + + # Spawn must succeed. + assert ok is True + assert log_path is not None + + # stdout and stderr must NOT be DEVNULL — they must be real file handles. + assert captured_kwargs.get("stdout") is not subprocess.DEVNULL, ( + "stdout was DEVNULL — watcher logs would be lost" + ) + assert captured_kwargs.get("stderr") is not subprocess.DEVNULL, ( + "stderr was DEVNULL — watcher logs would be lost" + ) + # Both should be the same file handle (opened in append mode). + assert captured_kwargs.get("stdout") is captured_kwargs.get("stderr"), ( + "stdout and stderr should share one file handle" + ) + # The log path must be under bach_home/logs/. + assert str(log_path).startswith(str(fake_bach_home / "logs")), ( + f"Log path {log_path} is not under bach_home/logs/" + ) + + +def test_spawn_session_watcher_log_path_included_in_output( + tmp_path: Path, +) -> None: + """When watcher spawns successfully, the output line must include the log path.""" + from bach.config.settings import ItermLayout + from bach.domain.models import AgentRuntime as AR + + registry = _registry_with_one_project(tmp_path) + service = TaskService(registry) + task = service.add_task( + description="Demo task", + project_key="demoproj", + runtime=AgentRuntime.claude_code, + mode=SessionMode.grill_me, + ) + + cfg = BachConfig( + watch_on_launch=True, + iterm_layout=ItermLayout.tabs, + runtime_default=AR.claude_code, + ) + + fake_log = tmp_path / "logs" / "watcher-task.log" + captured: list[str] = [] + with ( + patch("bach.services.task_service.launch_in_iterm", return_value=True), + patch("bach.services.task_service.load_config", return_value=cfg), + patch.object(service, "_spawn_session_watcher", return_value=(True, fake_log)), + ): + service.launch_task(task.artifact_path, output=captured.append) + + # The confirmation message must include the log file path. + assert any(str(fake_log) in line for line in captured), ( + f"Log path not in output. Got: {captured}" + ) diff --git a/tests/unit/test_transcript.py b/tests/unit/test_transcript.py index f972d1f..19d60dd 100644 --- a/tests/unit/test_transcript.py +++ b/tests/unit/test_transcript.py @@ -494,7 +494,7 @@ def should_stop() -> bool: ) ) # At least the initial line should have been yielded - assert any(e.kind is SessionEventKind.user_turn for e in events) + assert any(e is not None and e.kind is SessionEventKind.user_turn for e in events) def test_follow_events_stops_when_should_stop_true(tmp_path: Path) -> None: @@ -552,7 +552,8 @@ def _write_line() -> None: for event in follow_events( transcript, AgentRuntime.claude_code, poll_seconds=0.02, should_stop=should_stop ): - emitted.append(event) + if event is not None: + emitted.append(event) writer.join(timeout=2.0) assert any(e.kind is SessionEventKind.assistant_turn for e in emitted) @@ -585,3 +586,46 @@ def test_follow_events_returns_iterator() -> None: should_stop=lambda: True, ) assert isinstance(result, Iterator) + + +def test_follow_events_heartbeat_yields_none_when_quiet(tmp_path: Path) -> None: + """heartbeat=True emits None ticks on quiet polls so consumers can do + side work (spool drain) — regression for the zombie-watcher bug.""" + path = tmp_path / "t.jsonl" + path.write_text(_claude_line("user") + "\n") + + ticks = {"n": 0} + + def _stop() -> bool: + ticks["n"] += 1 + return ticks["n"] > 3 + + got = list( + follow_events( + path, + AgentRuntime.claude_code, + poll_seconds=0.01, + should_stop=_stop, + heartbeat=True, + ) + ) + # First poll yields the real event; later quiet polls yield None ticks. + assert any(e is not None for e in got) + assert any(e is None for e in got) + + +def test_follow_events_no_heartbeat_by_default(tmp_path: Path) -> None: + """Default behavior unchanged: no None ticks for existing consumers.""" + path = tmp_path / "t.jsonl" + path.write_text(_claude_line("user") + "\n") + + ticks = {"n": 0} + + def _stop() -> bool: + ticks["n"] += 1 + return ticks["n"] > 3 + + got = list( + follow_events(path, AgentRuntime.claude_code, poll_seconds=0.01, should_stop=_stop) + ) + assert all(e is not None for e in got)