cwcli-inspect-benches

name	cwcli-inspect-benches
description	Incident-level internals of cwcli's inspect / cache / multi-bench layer - inspect's 3-tier freshness model (T1 serve-cache / T2 read-only partial drift-check / T3 full re-cache), the multi-bench addressing model (numeric vs durable user labels, the --bench selector, marker-file recovery), the shared --yes / auto-start / honest-exit-code contract, the cache secret-redaction whitelist that keeps secrets out of the SQLite cache, and the CWCLI_HOME on-disk footprint override. Use this whenever you edit or debug core/inspect.py, commands/inspect.py, commands/label.py, utils/bench_labels.py, utils/db_utils.py, utils/config_utils.py, the SQLite cache, or anything touching multi-bench selection / labels. Each note guards a real shipped bug - keep the root-cause "why".
metadata	{"internal":true}

cwcli inspect / cache / multi-bench internals (sharp edges)

Each note guards a real shipped bug. Keep the root-cause "why" so a later change does not silently re-break the fix. The one-line contracts live in the always-loaded AGENTS.md "Important Components" section; this skill is the deep detail.

inspect command: 3-tier freshness model (cache / partial / full)

inspect is cache-backed and used to be cache-first with no staleness check, so an app installed after the cache was written (it lands in the bench apps/ dir immediately, but the cache still held the old list) stayed invisible until inspect --update rebuilt the cache. open --app <name> read the same stale cache and errored "App not found". This was issue #27 ("need to inspect -uv on new projects to see installed apps"; -uv = --update --verbose, only --update matters).

The fix (commands/inspect.py) is a 3-tier model, chosen so inspect stays fast while serving fresh data:

• T1 - cache return (unchanged, instant): no container calls. Used with the --no-refresh flag, or automatically when the containers are not running (the running check is ensure_containers_running(..., prompt=False), which never prompts or starts - non-disruptive).
• T2 - partial inspect (partial_inspect_known_benches): the default for a cached-and-running instance, and a pure READ-ONLY drift detector that NEVER writes the cache. For each KNOWN bench path already in the cache it cheaply re-reads only ls apps (available_apps) and ls sites, plus a test -d bench check. It deliberately SKIPS _find_bench_instances (the find-based instance discovery), the deep per-site bench list-apps (which boots Frappe), AND any config re-read: the cached per-site installed_apps, the cached per-site site_config, and the cached common_site_config are all carried forward from the cached structures (so a transient unreadable/half-written config can never silently drop common_site_config/default_site). It returns (refreshed, drift). A freshly installed app shows up in apps/, so the cheap ls catches it.
• T3 - full inspect (unchanged): the existing --update / cache-miss path: find discovery + per-site bench list-apps + re-cache.

Escalate-on-drift: when T2 sees the on-disk available-apps/site set diverge from the cache (or a known bench vanished) it returns drift=True and inspect sets bench_instances_data = None to fall through to a full T3 inspect, so the deep per-site lists and any brand-new bench are refreshed AND persisted. No drift -> inspect serves the cached data UNCHANGED and does NOT write the cache (the former write-on-read that bumped last_updated on every cache hit is gone). T2 is wrapped so any failure degrades to serving the cached data (never worse than the old behavior). Guarded escalation: the drift-triggered full inspect is NOT allowed to hard-fail a previously-working read. Before falling through, inspect remembers the cached benches in drift_fallback_benches; if the full inspect's _find_bench_instances then discovers NO bench (e.g. a custom search path was removed via config paths remove, or the bench was never under the default search roots), inspect degrades to that cached data and serves it WITHOUT persisting, instead of raise typer.Exit(1). The hard "No Bench Instances found" error is preserved ONLY for the --update / cache-miss paths, which have no valid cache to fall back on (drift_fallback_benches is None). To keep the raise out of the spinner, the no-benches case is captured as a flag inside the TipSpinner and branched on after the with block, and cache_project_data is called only when benches were actually gathered. open --app reuses this via partial_inspect_known_benches(frappe_container, cached_data["bench_instances"], verbose=verbose) IN-MEMORY only - it does NOT persist (degrading to the cached available-apps on any error). It matches the refreshed entry to the chosen bench BY PATH (b["path"] == bench_instance["path"]), not by indexing refreshed[0], because the partial pass drops any vanished bench and so can be index-shifted relative to the cached list; on no match or an empty result it keeps the cached available-apps. Leaving the cache untouched is deliberate: the next plain cwcli inspect still self-heals via escalate-on-drift (which also refreshes the deep per-site installed lists), instead of open consuming the drift signal by writing fresh available_apps while carrying stale per-site installed_apps forward.

A sharp edge: T2 cannot cheaply refresh per-site installed_apps (that needs the deep bench list-apps); it relies on escalate-on-drift to bring those up to date when apps/ or the site set changes. A pure install-app of an app already present in apps/ (no new dir) would not trip drift, but in practice a newly installed app appears in apps/ first, which is exactly the reported case.

The migration (batch 7, openspec migrate-inspect-core, 2026-07-16): the tier machine, the discovery/gather fan-out, AND the cache write now live in core/inspect.py; commands/inspect.py is a renderer. core.inspect_raw(...) -> Result[RawInspect] returns the CACHE-SHAPED dicts the human --json/tree/-i renderers need for byte-identity (key order differs between a gathered dict and a cache-read dict - current_site/label swap - and the configs ride in full); core.inspect(...) -> Result[InspectReport] is the typed, secrets-free surface every logic consumer and axi uses (site configs cross it only as has_site_config, never content). The T2 run-state check is resolve_container_state(auto_start=False, offer_choice=False) BY CONSTRUCTION - passive even when the caller passed auto_start=True. Two disclosed hardenings landed with the move: the probe decode is errors="replace" (a single non-UTF-8 byte used to crash the whole inspect), and a raw docker/transport exception escaping the T3 fan-out is now CwcliError(DOCKER) with the cache untouched (was an unhandled traceback; still crash-without-corruption). The old private helpers have public core spellings: _find_bench_instances -> discover_benches (same sorted(set(...)) numeric-label order), partial_inspect_known_benches -> partial_refresh (same carry-forward/drop-vanished/never-write contract).

The old Typer-default trap - calling the inspect COMMAND as a function left any omitted parameter as its truthy typer.Option(...) default OBJECT, so every logic caller had to pass all nine params explicitly - is GONE for inspect's callers: core.inspect is keyword-only with real defaults, and NOTHING invokes the command as a function any more (the seven old edges - recache_project, auto_inspect, update's and restore's fallbacks, open's two, rm's discovery - all call the core; main.py is the command's only importer). The trap still applies to any OTHER un-migrated Typer command invoked as a function (a prior version of this note also listed start among inspect's callers; commands/start.py never referenced inspect - corrected in batch 7). Regression coverage: tests/test_inspect_characterization.py (byte-identical --json per tier + the persisted cache shape, pinned green BEFORE the migration), tests/test_core_inspect.py (the envelope at the core), tests/test_inspect_partial_refresh.py (uses a FakeFrappeContainer that records every exec_run to assert which tier ran, and asserts T2 no-drift performs no cache write).

Multi-bench UX: labels, --bench selector, marker-file recovery

An instance is one docker-compose deployment; its frappe container can hold more than one bench directory (a multi-bench instance). The addressing model lives in utils/bench_labels.py and the shared resolver commands/utils.py:resolve_bench_path.

Label model

• Numeric label = position. _find_bench_instances returns sorted(set(...)) (deterministic). A bench's numeric label is its index into that stable sorted-by-path order (0, 1, 2, ...). Positions can shift when a bench is added/removed, so numeric labels are NOT durable - a user label is.
• User label = durable handle. Optional per-bench string. --bench <selector> resolves a user label FIRST, then a numeric index (bench_labels.resolve_bench). The two namespaces can't overlap because a user label may NOT be purely numeric (validate_user_label rejects ^\d+$, enforces charset [A-Za-z0-9._-], max 64). Duplicate labels within an instance are rejected by core.label (set_label for the label command, set_labels for inspect -i), which has the sibling benches to check against - not by either frontend.
• get_cached_project_data returns benches ordered by Bench.id (== insertion == sorted discovery order), so bench_instances[i] is always numeric label i. Do not reorder it.

Storage + marker-file recovery (the load-bearing contract)

• The user label is persisted in BOTH the SQLite cache AND a per-bench marker file <bench-root>/.cwcli/.bench-label (JSON {"schema": 1, "label": "..."}, room for future fields). The marker lives INSIDE the container/bench, so it survives a cache wipe. This is the recovery mechanism: if the SQLite DB is lost, a full inspect (--update / cache-miss) rebuilds each label from its marker (_gather_bench_data calls bench_labels.read_label_marker) and re-derives the rest of the bench config live, then re-persists. The marker is the source of truth for labels; T1/T2 serve the DB label (kept in sync by every writer), and partial_inspect_known_benches carries the cached label forward (it does NOT re-read the marker).
• DB schema: Bench.label is a nullable column added AFTER the initial schema. create_tables(safe=True) never adds a column to an existing table, so initialize_database() runs _migrate_bench_label_column() (a PRAGMA table_info(bench) check + ALTER TABLE bench ADD COLUMN label VARCHAR). This is idempotent and keeps old (label-less) caches working - existing rows get NULL. cache_project_data stores bench_data.get("label") or None; set_bench_label(project, path, label) updates one bench by path without rewriting the whole instance.
• Marker I/O is container-side. write_label_marker/read_label_marker/clear_label_marker go through container.exec_run with LIST-form commands (["cat", ...], ["rm","-f", ...], ["sh","-c", script]). The write base64-encodes the JSON on the host and base64 -ds it in the container, so no label content is ever interpolated into a shell command. Reads fail SAFE (missing/garbage marker -> None, never an error). Tests use tests/bench_fakes.py:MarkerFakeContainer, which really base64-decodes the write script into an in-memory fs.

--bench selector and the shared resolver

• resolve_bench_path(project, bench_selector, path_override, *, on_ambiguous="error") is the single replacement for the old copy-pasted "use --path if given, else bench_instances[0], else /workspace/frappe-bench" logic (which silently guessed in multi-bench instances). Precedence: --path wins (escape hatch; --bench+--path together is an error); else --bench resolves against the cache; else the DEFAULT bench: single-bench -> that bench; multi-bench -> error listing the benches (on_ambiguous="error", the data-op default) OR first-bench-plus-note (on_ambiguous="first"); no cache at all -> returns None so the caller keeps its inspect/default fallback.
• Commands with --bench: run, backup, update, open, unlock, restore (all use on_ambiguous="error" - a multi-bench op with no selector errors instead of guessing), and start (uses on_ambiguous="first" per captain decision: cwcli start KEEPS WORKING on multi-bench by starting bench start in the first sorted bench and printing a note; --bench picks another). run's --path default is None so the resolver runs. start recovers -y/--yes, -v, and --bench <val> from its variadic project-name argument (the arg greedily eats trailing options), same trick as rm's _recover_trailing_flags.
• Setting labels: cwcli label <project> [<selector> [<new-label>]] [--clear] (commands/label.py). Bare cwcli label <project> lists benches (read-only, no container). Setting/clearing writes BOTH stores and REQUIRES a running frappe container (the marker lives inside the bench; it does NOT auto-start - a label change should not spin up a stopped instance). BOTH the set and the clear paths write the MARKER FIRST and only touch the DB on marker success: because the marker is the source of truth for recovery, clearing the DB while the marker survives would let a later full inspect resurrect the just-cleared label, and writing the DB while the marker write failed would drop the durable handle on the next cache wipe. A marker failure prints an error and exits non-zero, leaving the DB unchanged. inspect -i prompts for a label per bench (frontend-only) and hands the collected answers to core.label.set_labels, the batched sibling of set_label - so the validate/uniqueness/marker/cache rule has one owner instead of being re-implemented inline; see the cwcli-core-axi skill's "label on the core" section for set_labels's own contract (the container-unavailable cache-only degrade, per-assignment outcomes, first-wins uniqueness within the batch).

--yes/-y contract

• Shared commands/utils.py:confirm_or_exit(prompt, *, assume_yes, refuse_message) mirrors the restore/rm pattern: --yes proceeds; a non-TTY WITHOUT --yes on a destructive op refuses and exits non-zero; an interactive decline exits non-zero.
• commands/utils.py:ensure_containers_running mirrors that same contract on its interactive not-running branch: on a non-TTY without auto_start it refuses (prints a message naming --yes, exits 1) instead of hanging on questionary or crashing on EOF, and an interactive decline or Ctrl-C exits 1 (was Exit(0)). This one root fix cures the inherited non-TTY defect across every caller (run/backup/update/open/unlock/logs/inspect). The prompt=False path is unchanged and load-bearing: it still returns False silently (no TTY check, no prompt, no Exit) for inspect Tier-2 and the rm recache path.
• Destructive confirm gaining --yes: config cache clear --all (uses confirm_or_exit).
• Auto-start commands gaining --yes: run, backup, update, open, unlock, logs, inspect thread --yes into ensure_containers_running(auto_start=yes), so a stopped instance auto-starts without the "start the containers?" prompt (non-destructive; enables non-interactive driving). For inspect this auto-start is scoped to the Tier 3 full-inspect path (--update / cache-miss / drift-escalation), which persists fresh data: since batch 7 the frontend resolves the core's confirm_start choice via ensure_containers_running(prompt=prompt_to_start, auto_start=yes) OUTSIDE the spinner and re-invokes the core once (capped). The Tier 2 cache-hit freshness pass stays passive inside the core itself (resolve_container_state(auto_start=False, offer_choice=False) hardcoded, even under --yes), so a plain cache-hit inspect never starts a stopped instance (preserving the T2 READ-ONLY contract above).
• start --yes: auto-confirms stopping conflicting Frappe instances to free their ports (_check_port_conflicts(assume_yes=...)). That port-conflict confirm also guards sys.stdin.isatty() before prompting: a non-TTY without --yes refuses (exit 1) rather than hanging, and Ctrl-C exits 1.
• Honest exit codes (issue #39): start/stop/restart exit 1 on a nonexistent instance (never printing "started"/"stopped" for it); multi-instance loops process every name then exit 1 if any failed (the rm failures-collector pattern - _start_project raises Exit(1) on not-found; cwcli stop itself catches core.stop's typed NOT_FOUND directly; _restart_project still signals not-found with a None return distinct from a legitimate zero count, sourced from commands/stop.py:stop_project_best_effort - a frontend adapter over core.stop mapping its typed NOT_FOUND back to the None its two remaining callers (restart's stop-then-start, start's port-conflict resolution) were written against). config's frozen deprecated aliases (auto-inspect start/restart/set-interval/install-startup/uninstall-startup) keep their pre-rework bodies verbatim and still raise typer.Exit(1) on their error paths, with the same idempotent-success (exit 0) carve-out for already-in-the-requested-state no-ops; the current primary surface's exit-code contract (USAGE -> 2, e.g. an invalid interval or a contradictory/missing cache clear target) is owned by the cwcli-core-axi skill's "config onto the core" section. update folds a maintenance-mode-disable failure into has_errors and names the stuck site (bench --site X set-maintenance-mode off). open's editor-select refuses (exit 1) on a non-TTY with multiple editors and no --code/--code-insiders/--cursor/--docker flag, and an interactive cancel exits 1.
• status issues no confirmation and needs no auto-start, so it has no --yes. restore/rm already had --yes (unchanged).

Regression coverage: tests/test_bench_labels.py (validation/resolution/marker I/O), tests/test_bench_selector.py (resolver cases incl. ambiguous/not-found), tests/test_bench_label_db_and_command.py (migration, set_bench_label, label command), tests/test_inspect_label_recovery.py (DB-loss recovery from markers via full inspect), tests/test_yes_flag.py (confirm_or_exit, config cache clear --all, start, auto-start).

Cache never stores secrets

The SQLite cache (~/.cwcli/cache/cwc-cache.db) must never persist Frappe secrets (DB passwords, per-site encryption_key, admin/root passwords, Redis URLs). Nothing ever reads secret values back from the cache: every credential consumer reads live from the container or from CLI flags/prompts (restore.py reads encryption_key live from a file in the container; MariaDB creds come from _prompt_mariadb_credentials), so a cached secret is pure write-only risk.

The single enforcement point is _redact_config_for_cache in utils/db_utils.py, applied at the ONLY write chokepoint (cache_project_data) before _validate_config_json and json.dumps, for BOTH common_site_config and per-site site_config. It is a whitelist, not a blacklist: only _COMMON_CONFIG_CACHE_KEYS / _SITE_CONFIG_CACHE_KEYS are kept, everything else is dropped, so a future unknown Frappe secret key fails closed. default_site MUST stay in the common whitelist or restore/backup/unlock lose default-site resolution (get_default_site). Redact ONLY here, never in core/inspect.py (redacting at the inspect layer would strip in-memory dicts the same run may use and leave the write path fail-open).

Old caches written before this shipped still hold secrets, so initialize_database() runs a one-shot _scrub_cached_config_secrets() (idempotent, warn-and-continue, never raises) that re-filters every config row through the same whitelist, rewrites only rows that change, touches only config_json (never bumps last_updated), and replaces unparseable JSON with "{}".

Rule: any NEW cached config field must be added to the relevant whitelist deliberately. Filesystem permissions (0700 dir / 0600 file, _set_secure_db_permissions) remain as defense-in-depth, not the primary control. Regression coverage: tests/test_db_security.py (TestCacheRedaction proves secrets stripped at write, scrub cleans an old row + is a no-op on clean rows, and get_default_site still resolves; test_models_document_redaction locks in that the old encryption TODO stays paid).

CWCLI_HOME override: relocating cwcli's on-disk state

config_utils.cwcli_home() is the single helper every footprint path resolves through: CONFIG_DIR/PROJECTS_DIR (config_utils.py), CACHE_DIR/DB_PATH (db_utils.py, which imports the helper instead of duplicating its own APP_NAME/Path.home() logic), the auto-inspect PID_DIR (auto_inspect.py), and rm's pre-deletion archive base (core/rm.py, both call sites, migrated off commands/rm.py by migrate-rm-core). It reads os.environ.get("CWCLI_HOME"): a non-empty value relocates the whole footprint there (.expanduser()d); unset OR empty (falsy) falls back to the existing Path.home() / ".cwcli". It never repoints the process HOME itself, so git/ssh/other HOME-derived tooling are unaffected, and relocated dirs keep the same 0700 dir / 0600 db-file permissions as the default location.

Sharp edge (why this needs its own entry): the feature shipped in two passes - the first pass (fb02a9e) routed config_utils/db_utils/auto_inspect but missed rm's two archive_base = Path.home() / ".cwcli" / "archive" call sites, which a no-mistakes review caught and fixed in a follow-up commit (cfa6d92). Until that fix, a CWCLI_HOME override was split-brain: every other path moved, but rm's live pre-deletion backups and config archives still landed in the real ~/.cwcli/archive. The lesson for future work: any NEW ~/.cwcli-adjacent path must be routed through cwcli_home() from the start, never a fresh Path.home() / ".cwcli" (or Path.home() / APP_NAME) literal, or it silently reintroduces this gap.

Regression coverage: tests/test_cwcli_home.py is deliberately mock-free - it sets a real CWCLI_HOME env var and asserts real filesystem results, resolving CONFIG_DIR/PROJECTS_DIR/CACHE_DIR/DB_PATH/PID_DIR in a fresh subprocess (exactly how a real cwcli process resolves them at import time) and calling cwcli_home() directly for the pure-logic (unset/empty/set) cases; it also proves the relocated cache keeps 0700/0600 permissions. It does NOT cover rm's archive routing (rm's own suites are still the existing mocked fakes, which never set CWCLI_HOME) - a mock-free assertion that rm --no-volumes/full rm writes its archive under $CWCLI_HOME/archive is not yet covered by any suite.

label on the logic core: the two-store write (batch 2)

commands/label.py is now a thin frontend over core/label.py (list_benches / set_label / clear_label). The behavior below is unchanged by that migration - it is restated here because the migration is exactly the kind of change that could silently break it.

• The clear path writes the MARKER first, the cache second, and fails closed on both. This ordering is load-bearing and must never be "tidied". The marker inside the bench is the source of truth for label recovery (core/inspect.py's _gather_bench_data reads it to rebuild labels after a cache wipe), so clearing the cache while the marker survives would let a later full inspect resurrect a label the user just deleted. A failed marker removal therefore raises and leaves the cache label intact; a failed cache update after a successful marker removal raises rather than reporting success. CLAUDE.md's captain standard names this incident class directly ("label --clear DB/marker consistency"). Pinned by tests/test_core_label.py (including a test that asserts the call ORDER directly, not just its failure modes), by the two pre-migration tests in tests/test_bench_label_db_and_command.py:171,190 whose assertions survived the migration unchanged, and by tests/e2e/test_label_e2e.py against a real container - a two-store property is precisely what unit fakes cannot prove, because the fakes can only agree with themselves.
• The SET path deliberately does NOT check its cache write, and this asymmetry is CORRECT. Do not fix it. core.set_label ignores db_utils.set_bench_label's return value while clear_label checks it. The reason is the recovery direction: on CLEAR, marker-survives-cache-cleared is a resurrection (silent data restoration), so it must fail closed. On SET, marker-written-cache-missed is self-healing in the same direction - the marker is the source of truth, so the next inspect recovers exactly the label the user asked for - and it is immediately visible, because the command prints the refreshed list straight from the cache. This rationale is now RECORDED at the site (core.set_label, task cwcli-label-setpath-db-check-a3, settled 2026-07-17): the comment above the db_utils.set_bench_label call spells out the recovery-direction argument, and test_cache_write_failure_is_tolerated_because_marker_is_source_of_truth in tests/test_core_label.py pins that a missed cache write on SET does NOT raise (the mirror of the clear path's test_cache_failure_after_marker_removal_is_surfaced). The decision: the asymmetry is intentional and correct - do NOT make the two paths symmetric "for consistency", which would either turn a self-healing case into a hard failure or weaken the clear-path guard to match the set path.
• label NEVER auto-starts a stopped project. It is the first production caller of resolvers.resolve_container_state(offer_choice=False): a stopped container is a hard NOT_RUNNING refusal, never a confirm_start choice. A label change is not a reason to spin up an instance, and the marker lives inside the bench so the container must genuinely be reachable. The refusal's hint is supplied by label via not_running_hint (the primitive's default names --yes, which label has no such flag for).
• The duplicate-label check keys on PATH, not on dict identity. A bench IS its path: db_utils.set_bench_label keys on it and the marker lives at it. (Pre-migration this compared dict identity, which was an artifact of having the dict in hand.)
• cwcli label --verbose was a dead flag and is now honest. It fed only the verbose params of the three bench_labels marker functions, which accepted and never read them, so the flag did literally nothing. The dead params are gone (inspect's own --verbose is real and untouched) and the flag now reports the envelope's notes and the resolved marker path. It was kept rather than removed: dropping a public flag would break existing invocations for no gain.