From ba133ba59e30c25e70a8ef016c533b4d6ea99e62 Mon Sep 17 00:00:00 2001 From: ProtocolWarden <32967198+ProtocolWarden@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:43:00 -0400 Subject: [PATCH] fix(observer): stop the CLI lying about flags it ignores Acting on a vulture triage that filed "8 observer CLI flags do nothing". The premise did not survive contact: 4 of the 5 implicated commands (observe-and-validate, compare, import, cleanup) are stubs that print "not yet implemented" and exit, so 6 of the 8 are ONE fact -- unimplemented commands -- not six defects. Wiring them is impossible without building the commands; tracked in .console/backlog.md instead of faked. The investigation surfaced worse defects, on commands that DO work: - cleanup exited 0 while doing nothing, so `cleanup --no-dry-run` reported success and no caller or scheduled job could tell retention never ran. A test asserted EXIT_SUCCESS, so the bug was pinned by its own coverage. Now exits non-zero like every sibling stub. - show/export accepted --backend and ignored it, serving LOCAL data as though it came from the requested backend -- silently wrong data, not a missing feature. list already had the guard they lacked; they now match it. - list --format csv was advertised in --help with no branch to handle it, and a typo'd --format fell through every arm. Both exited 0 printing nothing, which reads as "no snapshots" rather than "format not handled". csv implemented, unknown formats rejected. - list --filter parsed and was ignored, returning an unfiltered list. Removed rather than stubbed: nothing caches per-snapshot validation status to filter on, and an unknown-option error is honest where a quiet unfiltered result is not. Also guards typer.Exit from cmd_list's broad except, which would otherwise swallow the deliberate exit (click's Exit subclasses RuntimeError). Separately corrects docs/operator/setup.md, which claimed setup verifies with `team-executor --help`. TeamExecutor declares no [project.scripts], so that binary never exists and OC consumes it as a library; setup still runs the probe, so the doc described a step that cannot pass. 70 tests green in test_snapshot_cli.py (6 new); ruff clean; all five behaviours smoke-tested through the real CLI. The 18 other failures in tests/unit/observer/ reproduce with these changes stashed -- pre-existing Windows tempfile PermissionError, unrelated. Co-Authored-By: Claude Opus 5 --- .console/backlog.md | 62 +++++++++++++++++++ .console/log.md | 39 ++++++++++++ docs/operator/setup.md | 27 +++++--- src/operations_center/observer/cli.py | 79 ++++++++++++++++++------ tests/unit/observer/test_snapshot_cli.py | 70 ++++++++++++++++++++- 5 files changed, 247 insertions(+), 30 deletions(-) diff --git a/.console/backlog.md b/.console/backlog.md index d1357f1cb..7117c3862 100644 --- a/.console/backlog.md +++ b/.console/backlog.md @@ -2,8 +2,70 @@ _Durable work inventory. Update after each meaningful chunk of progress._ +## Up Next + +### Implement the 4 stub observer commands +- `observe-and-validate`, `compare`, `import` and `cleanup` in + `src/operations_center/observer/cli.py` are stubs: they print "not yet implemented" + and exit. Their options (`--skip-validation`, `--signals`, `--validate-after`, + `--keep-count`, …) are therefore declared and never read. Wiring the flags is not + possible without building the commands. +- `observe-and-validate` needs RepoObserver integration (its own stub message says so); + `cleanup` needs retention logic honouring `--days`/`--keep-count`/`--dry-run`; + `compare` and `import` need snapshot diffing and ingest respectively. +- All four now exit non-zero, so nothing can mistake a stub for a result. +- Vulture will keep reporting these params. That is correct — do not whitelist them; the + finding disappears when the commands are implemented. + +### Vulture gate is false-green — ~620 real findings land on the next Custodian pin bump +- OC's pinned Custodian (`d6ba8ab`) has a vulture adapter that appends tests/whitelist + paths AFTER `--min-confidence=`; vulture's argparse rejects that, exits 2 with empty + stdout, and the adapter reports clean. **Vulture has never actually run in CI.** +- Fixed upstream in Custodian (paths before flags + returncode check). Once OC bumps the + pin, ~620 findings appear at once. +- Triaged: 426 in `src/`, 195 in `tests/`; 32 at 100% confidence, 589 at 60%. By kind: + 302 variable, 127 attribute, 99 method, 86 function, 4 class, 3 property. The + 100%-confidence set splits into framework-signature false positives (`exc_val` in + `__exit__`, `exitstatus` in pytest hooks), dead test locals, and the real CLI defects + fixed on 2026-08-04. +- **Blanket-whitelisting would bury real defects.** Triage by confidence, not wholesale. +- Options when the pin moves: burn down, add a `.vulture_whitelist.py` (the adapter + already looks for one at repo root), or raise `vulture_min_confidence` in + `.custodian/config.yaml`. Do NOT raise the threshold merely to unblock a push. + +### `oc setup` probes a `team-executor` CLI that cannot exist +- `ensure_executor_installed`/`verify_executor` (`entrypoints/setup/main.py:192,219`, + called at `:1210-1211`) probe PATH for a `team-executor` console script and verify it + with `--help`. TeamExecutor declares no `[project.scripts]`, so that binary does not + exist and the check can never pass. OC consumes it as a library. +- Replace the PATH/CLI probe with an importability check of the three backends OC loads, + mirroring `ensure_executor_backends()` in `scripts/operations-center.sh`. +- Remove the "Known stale step" note in `docs/operator/setup.md` once fixed. + ## Done +### 2026-08-04: Observer CLI flags that lied (✅ COMPLETE) +- **Objective**: fix the flags the vulture triage exposed as declared-but-never-read. +- **Finding that reframed it**: 4 of the 5 implicated commands are unimplemented stubs, so + 6 of the 8 "dead flags" are a symptom of that, not separate bugs (moved to Up Next). + Investigating them surfaced worse defects on the commands that DO work. +- **Fixed**: + - `cleanup` exited **0** while doing nothing, so `cleanup --no-dry-run` reported success + and a scheduled job could not tell retention had never run. Now exits non-zero, like + every sibling stub. A test asserted the old behaviour and was pinning the bug. + - `show`/`export` accepted `--backend` and ignored it, serving LOCAL data as though it + came from the requested backend. Now rejected, matching `list`'s existing guard. + - `list --filter` parsed and was ignored, returning an unfiltered list. Removed: nothing + caches per-snapshot validation status to filter on, so an unknown-option error is + honest where a quietly unfiltered result is not. + - `list --format csv` was advertised in `--help` with no branch — exited 0 printing + nothing, indistinguishable from an empty store. Implemented. + - `list --format ` fell through every arm and exited 0 silently. Now rejected. +- **Verification**: 70 tests in `test_snapshot_cli.py` green (6 new); `ruff check .` clean; + all five behaviours smoke-tested through the real CLI. The 18 remaining failures in + `tests/unit/observer/` reproduce with these changes stashed — pre-existing Windows + `PermissionError: [WinError 32]` in tempfile handling, unrelated. + ### 2026-07-15: Stage 4 — Refactor existing code to use the new shared helper (✅ COMPLETE) - **Objective**: Independently re-verify Stage 2's migration against the "refactor existing code" acceptance bar (identified/updated all relevant callsites, replaced redundant diff --git a/.console/log.md b/.console/log.md index aae1f04da..42e9aecd3 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,42 @@ +## 2026-08-04 — fix(observer): stop the CLI lying about flags it ignores + +Acting on a vulture triage that filed "8 observer CLI flags do nothing". The +premise did not survive contact: 4 of the 5 implicated commands +(`observe-and-validate`, `compare`, `import`, `cleanup`) are stubs that print +"not yet implemented" and exit, so 6 of the 8 are ONE fact — unimplemented +commands — not six defects. Wiring them is impossible without building the +commands, so that became backlog rather than being faked. + +What the investigation DID surface is worse than the original filing, because it +sits on commands that work. `cleanup` exited **0** while doing nothing: a +scheduled `cleanup --no-dry-run` reported success and no caller could tell +retention had never run. A test asserted `EXIT_SUCCESS`, so the bug was pinned by +its own coverage. `show` and `export` accepted `--backend` and ignored it, +serving LOCAL data as though it came from the requested backend — silently wrong +data, not a missing feature, and `list` already had the guard they lacked. +`list --format csv` was advertised in `--help` with no branch, and a typo'd +`--format` fell through every arm; both exited 0 printing nothing, which reads as +"no snapshots" rather than "I did not understand you". `--filter` was removed +rather than stubbed: nothing caches per-snapshot validation status, and an +unknown-option error is honest where a quietly unfiltered list is not. + +The through-line is one failure mode — a CLI that successfully answers a question +the user did not ask. Exit codes and explicit rejection are the fix; each change +carries a test, and the `cleanup` test now documents why it inverted. + +Also corrects `docs/operator/setup.md`, which claimed setup "verifies the install +with `team-executor --help`". TeamExecutor declares no `[project.scripts]`, so no +such binary is ever produced and OC consumes it purely as a library. Setup STILL +runs that probe (`entrypoints/setup/main.py:1210-1211`), so the doc described a +step that can never pass; the section now describes the real import-based +mechanism and flags the dead probe as a known-stale step (backlog). + +This branch originally also carried self-heal and CI-pin fixes. Both landed +independently on main as #491 and #492 with better implementations — a +data-driven `EXECUTOR_BACKENDS` list, and `pip install -e ".[dev]"` taking the +pin from pyproject instead of a second version literal. Dropped rather than +merged: duplicating them would have re-introduced the drift #492 removed. + ## 2026-08-03 — fix(hooks): pre-push resolved the wrong workspace root inside a git worktree `.hooks/pre-push` locates the boundary disclosure artifact by globbing sibling diff --git a/docs/operator/setup.md b/docs/operator/setup.md index 0285a6df7..a42d3ae1c 100644 --- a/docs/operator/setup.md +++ b/docs/operator/setup.md @@ -71,9 +71,9 @@ source .env.operations-center.local ### Executor (TeamExecutor) TeamExecutor is the multi-agent coding engine OperationsCenter uses for task execution. -See `src/operations_center/backends/team_executor/` for the adapter implementation. +OperationsCenter consumes it as a Python library (`import team_executor`) — see +`src/operations_center/backends/team_executor/` for the adapter implementation. -- install/verify `team-executor` CLI - configure orchestrator defaults - persist local execution settings @@ -111,14 +111,21 @@ so they work regardless of which venv was activated during bootstrap. ## Executor Install Behavior -Setup: - -- checks whether `team-executor` is on `PATH` -- installs `uv` if needed -- installs TeamExecutor if missing -- verifies the install with `team-executor --help` - -Setup is intended to be idempotent: it does not reinstall the executor when the current install already works. +The execute backends (TeamExecutor, DAGExecutor, CritiqueExecutor) are sibling checkouts, +not declared OperationsCenter dependencies. `ensure_executor_backends` in +`scripts/operations-center.sh` installs any that are missing editable into the +OperationsCenter venv on launch, so a `uv sync` or venv recreate that drops them recovers +on the next fleet start. All three are loaded by import, not by CLI. + +The check is idempotent: it only installs when a backend is not importable. + +> **Known stale step:** `oc setup` still probes for a `team-executor` console script on +> `PATH` and verifies it with `team-executor --help` +> (`ensure_executor_installed` / `verify_executor` in +> `src/operations_center/entrypoints/setup/main.py`). TeamExecutor's `pyproject.toml` +> declares no `[project.scripts]`, so that binary does not exist and the check cannot +> pass. Of the three backends only DAGExecutor ships a console script (`dag-executor`), +> and OperationsCenter does not use it. ## Advanced Mode diff --git a/src/operations_center/observer/cli.py b/src/operations_center/observer/cli.py index c2fea338d..35340b1e4 100644 --- a/src/operations_center/observer/cli.py +++ b/src/operations_center/observer/cli.py @@ -12,9 +12,11 @@ from __future__ import annotations +import csv import json import logging import os +import sys from pathlib import Path import typer @@ -420,6 +422,19 @@ def cmd_observe_and_validate( raise typer.Exit(EXIT_CONFIG_ERROR) +def _snapshot_size(snapshot_dir: Path) -> str: + """Human-readable size of a snapshot's payload, or "" when it is absent.""" + json_file = snapshot_dir / "repo_state_snapshot.json" + if not json_file.exists(): + return "" + size_bytes = json_file.stat().st_size + if size_bytes < 1024: + return f"{size_bytes} B" + if size_bytes < 1024 * 1024: + return f"{size_bytes / 1024:.1f} KB" + return f"{size_bytes / (1024 * 1024):.1f} MB" + + @app.command("list") def cmd_list( limit: int = typer.Option( @@ -432,11 +447,6 @@ def cmd_list( "--order", help="Sort order: recent|oldest|name", ), - filter_status: str | None = typer.Option( - None, - "--filter", - help="Filter: valid|invalid (if validation cached)", - ), format_str: str = typer.Option( "table", "--format", @@ -498,18 +508,7 @@ def cmd_list( table.add_column("size", style="yellow") for snapshot_dir in snapshot_dirs: - json_file = snapshot_dir / "repo_state_snapshot.json" - size = "" - if json_file.exists(): - size_bytes = json_file.stat().st_size - if size_bytes < 1024: - size = f"{size_bytes} B" - elif size_bytes < 1024 * 1024: - size = f"{size_bytes / 1024:.1f} KB" - else: - size = f"{size_bytes / (1024 * 1024):.1f} MB" - - table.add_row(snapshot_dir.name, "—", size) + table.add_row(snapshot_dir.name, "—", _snapshot_size(snapshot_dir)) if not quiet: console.print(table) @@ -519,6 +518,30 @@ def cmd_list( if not quiet: print_structured(console, snapshots) + elif format_str == "csv": + # `--help` has always advertised csv, but there was no branch for it: + # the command exited 0 having printed nothing, which reads as "no + # snapshots" rather than "format not handled". + if not quiet: + writer = csv.writer(sys.stdout, lineterminator="\n") + writer.writerow(["run_id", "observed_at", "size"]) + for snapshot_dir in snapshot_dirs: + writer.writerow([snapshot_dir.name, "", _snapshot_size(snapshot_dir)]) + + else: + # Previously an unknown --format fell through every branch and exited + # 0 with no output, so a typo looked like an empty snapshot store. + if not quiet: + console.print( + f"[red]Error: unknown --format '{format_str}' (expected table|json|csv)[/red]" + ) + raise typer.Exit(EXIT_CONFIG_ERROR) + + # `typer.Exit` subclasses RuntimeError, so the broad handler below would + # otherwise swallow the deliberate exit above and relabel it as an + # unexpected listing failure. Matches the pattern in cmd_validate. + except typer.Exit: + raise except Exception as e: if not quiet: console.print(f"[red]Error listing snapshots: {e}[/red]") @@ -560,6 +583,15 @@ def cmd_show( ), ) -> None: """Display snapshot contents.""" + # `--backend` was accepted and ignored, so `--backend s3` silently read the + # LOCAL store and presented it as the requested one. `list` already rejects + # non-local backends; refusing here keeps the whole CLI honest rather than + # answering a question the user did not ask. + if backend != "local": + if not quiet: + console.print("[red]Error: Non-local backends not yet supported[/red]") + raise typer.Exit(EXIT_CONFIG_ERROR) + try: loader = SnapshotLoader() snapshot = loader.load(snapshot_path) @@ -678,6 +710,13 @@ def cmd_export( ), ) -> None: """Export snapshot to file.""" + # See cmd_show: an ignored `--backend` silently exported from the local + # store regardless of what was requested. + if backend != "local": + if not quiet: + console.print("[red]Error: Non-local backends not yet supported[/red]") + raise typer.Exit(EXIT_CONFIG_ERROR) + try: loader = SnapshotLoader() snapshot = loader.load(snapshot_id) @@ -812,7 +851,11 @@ def cmd_cleanup( """Remove old snapshots.""" if not quiet: console.print("[cyan]cleanup[/cyan] command not yet implemented") - raise typer.Exit(EXIT_SUCCESS) + # EXIT_CONFIG_ERROR, not EXIT_SUCCESS. This stub used to exit 0, so + # `cleanup --no-dry-run` reported success while deleting nothing — a caller + # or scheduled job could not tell retention had silently never run. Every + # other unimplemented command in this CLI already exits non-zero. + raise typer.Exit(EXIT_CONFIG_ERROR) @app.command("query-flaky-tests") diff --git a/tests/unit/observer/test_snapshot_cli.py b/tests/unit/observer/test_snapshot_cli.py index 9727e4b4c..64699f4be 100644 --- a/tests/unit/observer/test_snapshot_cli.py +++ b/tests/unit/observer/test_snapshot_cli.py @@ -209,6 +209,62 @@ def test_list_invalid_backend(self) -> None: result = runner.invoke(app, ["list", "--backend", "s3"]) assert result.exit_code == EXIT_CONFIG_ERROR + def test_list_format_csv_emits_rows(self) -> None: + """csv has always been advertised in --help but had no branch. + + It fell through every format arm and exited 0 having printed nothing, + which is indistinguishable from an empty snapshot store. + """ + with tempfile.TemporaryDirectory() as tmpdir: + snapshot_dir = Path(tmpdir) / "obs_test_123_abc" + snapshot_dir.mkdir() + (snapshot_dir / "repo_state_snapshot.json").write_text("{}", encoding="utf-8") + + result = runner.invoke(app, ["list", "--storage-root", tmpdir, "--format", "csv"]) + assert result.exit_code == EXIT_SUCCESS + lines = [ln for ln in result.stdout.splitlines() if ln.strip()] + assert lines[0] == "run_id,observed_at,size" + assert any(ln.startswith("obs_test_123_abc,") for ln in lines[1:]) + + def test_list_unknown_format_is_rejected_not_silently_empty(self) -> None: + """A typo'd --format used to exit 0 with no output, reading as 'no snapshots'.""" + with tempfile.TemporaryDirectory() as tmpdir: + snapshot_dir = Path(tmpdir) / "obs_test_123_abc" + snapshot_dir.mkdir() + (snapshot_dir / "repo_state_snapshot.json").write_text("{}", encoding="utf-8") + + result = runner.invoke(app, ["list", "--storage-root", tmpdir, "--format", "tabel"]) + assert result.exit_code == EXIT_CONFIG_ERROR + assert "unknown --format" in result.stdout + + def test_show_rejects_non_local_backend(self) -> None: + """`--backend` was ignored, so `show --backend s3` served LOCAL data as if remote.""" + result = runner.invoke(app, ["show", "some-run-id", "--backend", "s3"]) + assert result.exit_code == EXIT_CONFIG_ERROR + assert "Non-local backends not yet supported" in result.stdout + + def test_export_rejects_non_local_backend(self) -> None: + """Same ignored-`--backend` bug on export: it exported from local regardless.""" + with tempfile.TemporaryDirectory() as tmpdir: + out = Path(tmpdir) / "out.json" + result = runner.invoke( + app, ["export", "some-run-id", str(out), "--backend", "s3"] + ) + assert result.exit_code == EXIT_CONFIG_ERROR + + def test_list_no_longer_accepts_dead_filter_flag(self) -> None: + """`--filter` parsed and was then ignored, so it always returned unfiltered rows. + + Nothing caches per-snapshot validation status for it to filter on, so the + option is removed rather than left silently inert: an unknown-option error + is honest, a quietly unfiltered list is not. + """ + with tempfile.TemporaryDirectory() as tmpdir: + result = runner.invoke( + app, ["list", "--storage-root", tmpdir, "--filter", "valid"] + ) + assert result.exit_code != EXIT_SUCCESS + class TestShowCommand: """Tests for show command.""" @@ -286,11 +342,21 @@ def test_import_not_implemented(self) -> None: assert "not yet implemented" in result.stdout def test_cleanup_not_implemented(self) -> None: - """Test cleanup command.""" + """An unimplemented cleanup must NOT report success. + + This previously asserted EXIT_SUCCESS, pinning a bug: the stub exited 0, + so `cleanup --no-dry-run` looked like retention had run when nothing was + deleted. Every sibling stub already exits non-zero. + """ result = runner.invoke(app, ["cleanup"]) - assert result.exit_code == EXIT_SUCCESS + assert result.exit_code == EXIT_CONFIG_ERROR assert "not yet implemented" in result.stdout + def test_cleanup_no_dry_run_still_does_not_report_success(self) -> None: + """The dangerous invocation specifically must not exit 0.""" + result = runner.invoke(app, ["cleanup", "--no-dry-run"]) + assert result.exit_code != EXIT_SUCCESS + class TestGlobalOptions: """Tests for global options."""