cwcli apps command group + update.py (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 contract lives in the always-loaded AGENTS.md "Important Components" section; this skill is the deep detail.
apps command group: multi-site fan-out, JSON purity, update deprecation (2026-07-11; onto the logic core 2026-07-15)
commands/apps.py is the first-class app manager (list/install/uninstall/update), registered as a Typer sub-app in main.py (add_typer(apps_cmd.app, name="apps")). As of batch 5 (migrate-apps-core) list/install/uninstall moved to core/apps.py (list_apps/install_apps/uninstall_apps, returning typed AppsListing/AppsReport); commands/apps.py is now a THIN RENDERER over the core for all four subcommands (update moved in batch 4, see below). It adds no new dependency or cache field. The original list/install/uninstall design was spec-driven via OpenSpec (openspec/changes/add-app-management/); the core migration's own artifacts live in openspec/changes/migrate-apps-core/.
Load-bearing decisions, each guarding a real constraint - the substance is unchanged from the original design (every flag, message, exit code and --json shape is preserved byte-for-byte by the migration), only the file/function each one lives in moved:
- Multi-site by DEFAULT for install/uninstall/update. No
--site = fan out to ALL sites from the canonical bench_sites.list_sites (NOT get_default_site - that single-site model was rejected by the captain); --site is repeatable and narrows (core/apps.py:_target_sites). There is deliberately NO active/disabled-site distinction - one site set, the same list_sites everything else uses. The fan-out is continue-and-report-all: run every (app, site) step, collect a per-step AppResult(app, site, action, ok), and exit non-zero if ANY failed - never a success banner over a partial failure. commands/apps.py:_report_and_exit renders the aggregate and reads the exit code off report.ok/listing.ok, NOT the envelope's result.status (the closed Status set has no ERROR member, so a partial failure is WARNING-shaped and every other verb maps WARNING to exit 0 - copying that pattern here would report success for a half-failed uninstall). Stop-on-first-failure was explicitly rejected.
- EVERY verb that changes app code on disk routes through ONE resynchronise step (
core.supervision.resync_after_code_change; core.apps._resync_after_mutation is its report adapter). bench get-app changes the bench environment after the interpreter started, so a successful install-app could return ok: true while site requests failed with ModuleNotFoundError; uninstall leaves the inverse stale-code risk; checkout leaves the bench serving the branch the developer just moved off (the QUIETEST form - nothing errors at all); and update's git pull is the same fault again. Four verbs each growing their own restart is the drift the install/uninstall fix already avoided between two, so the step lives in core.supervision - NOT core.apps - because core.update is a separate module that needs it and supervision already owns discovery, per-program restart and the site-routed probe. Callers pass only the thing they alone know: which sites they changed (install/uninstall: the sites the fan-out landed on; checkout: the sites with that app installed, via core.apps._sites_with_app_installed - "checkout has no target site" was true of its ARGUMENTS and false of its EFFECT; update: sites_to_migrate; the frappe bench-wide reset: every site on the bench). WHAT IS CYCLED: every code-bearing program, not just web (supervision.code_bearing_programs: web, schedule, and every worker spelling, keyed off the normalized label's base so worker_default/worker_short need no list). This closes the residual the first fix reported: workers and the scheduler import the same app code, so after an install they cannot import the new app, after an uninstall they act on tables that are gone, and after a checkout they silently run the old branch. socketio/watch (node) and redis_* are deliberately excluded - they import no Frappe app and cycling redis drops the cache and the job queue for nothing. The honest cost, disclosed rather than hidden: a worker restart interrupts a job in flight (RQ shuts down warm on SIGTERM inside supervisord's stop window); that is the same disturbance cwcli restart causes and the one the README already directed users to after a mutation, and a worker running code that no longer exists is not a safer state to leave behind. Three properties, each guarding something specific. (1) A stopped bench stays stopped: programs are cycled only when a required process read POSITIVELY establishes a live supervisord with a serving web, and only programs already RUNNING/STARTING are touched, so a deliberately-stopped worker is not started by an install; an unreadable read is a REPORTED failure, never "assume nothing is running" (core.where's fail-honest rule) - the required=True on BOTH discovery calls is what makes that true and a test pins its presence. A manager cwcli does not own (honcho, plain bench start) is probed but never restarted, and an unhealthy site there gets a manual-restart remedy. (2) It NEVER raises: the code change has already landed, so every failure comes back in ResyncOutcome; a raise would erase the record of a change that genuinely happened and an automation caller that retried would run the mutation twice. (3) Restarting is NOT proof - "supervisord reports RUNNING" is a bound-port-shaped claim, and serving stale code is exactly the class of defect such a claim misses - so every named site must answer /api/method/ping with HTTP 200 (bounded; each probe gets only the remaining budget, or a server that accepts and never answers hangs the loop past its own timeout) before the outcome is ok. A failed restart, unreadable process state or port, probe exception, or non-200 makes AppsReport.ok/UpdateReport.ok false and the command non-zero without erasing the landed mutation. The restart's exit code is read not in (0, None) like every other container read here - None means docker recorded no code, not failure, and the site probe is the real gate. The step is DISCLOSED: each program is ANNOUNCED as it is cycled (AppsAnnounce(phase="restart-processes", app=<program>) / UpdateStepStart(phase="resync")) and reported as its own row on the human, --json and axi surfaces - the defect is that a bench was mutated underneath running processes silently, and a fix that restarts silently repeats the habit on a command that can then sit in the probe wait for up to a minute. update's POSITION is load-bearing: the resync runs inside the finally AFTER the maintenance-mode disable, because a site in maintenance answers 503 and the proof could never be obtained; it is SKIPPED on an abort, where aborted: true already says the run is half-applied and a Ctrl-C should not gain a restart plus a bounded wait. UpdateReport grew restarted_processes/unserved_sites/resync_error and _build_report folds them into ok. No extra consent, unlike scale: the caller already chose an app mutation and this cycles the programs of the ONE bench they named, whose web may otherwise stay broken; a consent gate whose refusal leaves the site down would be the defect, not a guard. The failure is real but NOT reproducible on demand - do not read a non-reproduction as proof the guard is unnecessary. It was observed twice on real v16 benches (the operator's, and a 200 -> 500 on /api/method/ping straight after axi apps install), but a later deliberate attempt - raw bench install-app into a running bench, no restart - served 200 from the stale process on a freshly provisioned v16 bench. Whether the running interpreter trips depends on what the request path imports and whether the dev server's reloader happened to pick the change up, which is exactly why the resync is UNCONDITIONAL rather than gated on detecting the bad state: a condition that only sometimes appears is one a conditional guard will sometimes miss. The real-Docker guards are test_apps_resync_e2e.py (install -> checkout -> update off ONE install, asserting each code-bearing program got a NEW pid, that socketio/watch/redis_* kept theirs, and that the site serves 200 at every step - a statement about real supervisord pids that no mock can make) and test_axi_apps_install_permitted_then_refused_on_rerun, which proves HTTP 200 BEFORE checking installed-state assertions; the ordering is the point, because checking only that no error was raised is exactly what missed this.
--json stdout purity. console is stdout (see utils/console.py), so in JSON mode NOTHING but the final json.dumps may touch stdout. The core emits AppsOutput events tagged by stream; the frontend's _make_renderer picks the consumption mode - buffer-and-join to stderr-on-failure when json_output, direct sys.stdout.write when not (mirrors run.py) - because that choice is rendering, not logic. A destructive apps uninstall --json without --yes REFUSES (json implies non-interactive; a confirm_or_exit prompt would fight the JSON contract, and its --yes ack prints to stdout).
- Post-mutation refresh uses
cache.recache_project, NOT partial_inspect_known_benches/core.partial_refresh. The partial pass is READ-ONLY (never persists) and cannot refresh per-site installed_apps - exactly what install/uninstall change. Only a full recache (which routes through cache_project_data -> _redact_config_for_cache, so no secret is ever written) makes where/open/inspect honest. Degrades to a warning; the mutation already succeeded. This refresh is deliberately NOT in core/apps.py - see the batch 5 section below for why it stays a frontend epilogue here rather than following core/update.py's example of a mid-fan-out call.
<app> is a name OR a git URL passed straight to bench get-app; the per-site install-app name is derived from the actual apps/ before/after diff (the real dir bench get-app created), NOT parsed from the target string - core/apps.py:derive_app_name's URL-basename-minus-.git heuristic is only a FALLBACK for when that diff is ambiguous (zero or more than one new apps/ entry, e.g. the app was already present). uninstall-app is always invoked with bench's own --yes so a non-TTY exec never hangs on bench's internal confirm (cwcli's confirm_or_exit gate is the human confirmation). uninstall only removes the app from site(s); deleting its code from the bench is out of scope (that is bench remove-app, run manually).
apps's list/install/uninstall on the logic core: what a reviewer would flag as a mistake and isn't (batch 5, 2026-07-15)
core/apps.py owns resolution (container + bench only - no default-site, no site-name validation, no bench-dir probe: apps uses none of those today, and reaching for them because they exist would ADD failures it does not have), the container I/O, and the fan-outs; it RETURNS AppsListing/AppsReport and prints nothing. Five decisions guard real behaviour, and each looks like an inconsistency if you read only the diff:
- The recache stays a FRONTEND epilogue (
commands/apps.py:_refresh_cache, gated on any(r.ok for r in report.results)), NOT hoisted into the core the way core/update.py calls cache.recache_project mid-fan-out. That call exists in update only because its recache runs MID-fan-out and cannot be moved out; install/uninstall's recache is a post-mutation step gated on a condition already in the returned report, so an epilogue costs nothing. (Before migrate-inspect-core, update's mid-fan-out call was also the one core-module-imports-CLI-layer site in the codebase, since recache_project invoked the inspect Typer command; since that batch recache_project's body calls core.inspect instead, so this is now an ordinary core-to-core call, not a layering exception.)
uninstall's fused --yes ("skip the destructive confirmation AND auto-start containers") keeps that exact CLI meaning - a migration does not get to change the contract - but core.uninstall_apps takes auto_start and consent as SEPARATE parameters and returns NEEDS_CHOICE/confirm_uninstall for the destructive gate. The fusion is one frontend's UX choice; a core that must also serve axi and a future GUI must not inherit it. commands/apps.py:_uninstall closes over consent and is called TWICE on the interactive path - once with yes, and again with True after confirm_or_exit returns - which is the re-invoke shape every NEEDS_CHOICE frontend uses (--yes short-circuits it on the first call; tests/test_apps_characterization.py's interactive-confirm test is what proves the second call actually runs the uninstall rather than just passing).
AppsAnnounce and AppsCommand are two events, not one, because the pre-migration code interleaves them: it announces "Fetching x...", THEN reads apps/ (echoing that read under --verbose), THEN echoes the get-app command itself. Fusing the two into one event would reorder --verbose stderr.
- The
warnings field carries notes an agent should act on; the command trace rides the event channel. The original batch-5 proposal said list_apps needs no callback ("nothing to report progress about") - right about progress, wrong about the trace: routing $ ls -1 apps through warnings would put a debug echo inside axi apps list's structured TOON document. list_apps takes on_event like the other two verbs (and like core.update); axi passes nothing and the trace is discarded for free (_noop).
cwcli axi apps list ships; axi apps uninstall deliberately does NOT (captain-locked 2026-07-15).
bench uninstall-app drops the app's tables, and letting an agent do that is a product decision on its own evidence, not a side effect of moving code onto the core.
tests/test_axi_apps_list.py::test_uninstall_is_deliberately_not_a_verb asserts the absence so it cannot be misread as an oversight.
Decision 4 above made the eventual verb cheap: the core already exposes destructive consent separately from auto-start.
axi apps install SHIPPED 2026-07-20, reversing only the half of this deferral it never actually covered.
Its user-facing contract lives in README.md, while the safety invariant and regression references live in the AGENTS.md apps entry.
uninstall stays deferred under the unchanged rationale.
cwcli axi apps checkout SHIPPED 2026-07-20, reversing its own absence assertion - and HOW it was reversed is the durable lesson. The old assertion sat beside the install/uninstall one and read "apps checkout mutates the in-instance checkout, so LIKE install/uninstall it is a HUMAN verb only". That "like" is where the reasoning slipped: install/uninstall were held for the ONE threat named above (dropping an app's tables), and checkout was grouped with them for merely being a mutation, so a rationale it was never covered by silently became a block. Two things settled it, both verified rather than asserted: (1) core.checkout_app runs git fetch + git checkout -B inside apps/<app> - no bench command, no site, no SQL, no table - and migrate-apps-core/tasks.md §7.2 had already pre-authorized a different answer for non-deleting mutations; (2) "the agent surface is read-only" is not a rule this repo follows - most live axi verbs mutate (see the AGENTS.md apps checkout entry for the current count and enumeration), axi init provisions an entire instance and axi apps update runs schema migrations across live sites, so no read-only principle can be what withholds a git fetch. Generalize this, do not just remember the outcome: when you find an absence pinned by a test, read the rationale the test INHERITED and check it actually reaches the thing it is blocking. axi init is the same story (deferred, pinned, then shipped on its own evidence). The verb itself shipped as a thin renderer with zero core/ delta; its guards and the deferred resulting-commit read are in the AGENTS.md entry and openspec/changes/add-axi-apps-checkout-verb/.
- The dirty-tree guard is cwcli's own, and how it got there is the lesson.
axi apps checkout originally had NO dirty-tree pre-check by design, leaning on git's refusal of a checkout that would OVERWRITE a modified file, and the docs described that as "a dirty tree fails". CI on PR #129 proved the claim false: a dirty file the target ref does not touch has nothing to conflict with, so it rode through at exit 0. The obvious fix was to soften the docs; the captain ruled the opposite - fulfil the promise rather than scrub the doc, because a false SAFETY claim is acted on (a user leaves uncommitted work in an app dir believing it is protected), and these checkouts live in a SHARED dev instance where the work carried across may not be the runner's. core.apps._refuse_dirty_tree now refuses CONFLICT/app.dirty_tree before any fetch. Generalize this: when a doc and the code disagree about a SAFETY guarantee, the doc is not automatically the thing that is wrong. What counts as dirty is stated, not implied, and it was itself revised once - the first cut passed --untracked-files=no, reasoning that the guard should cover exactly what --reset destroys; the captain reversed that, so it is now plain git status --porcelain and untracked files refuse too, because a new module not yet git added is uncommitted work and the tool must not decide it is worthless. That widening is only safe because .gitignore keeps build residue out of git status entirely - a real-bench E2E asserts a freshly provisioned app is genuinely clean rather than assuming it. Keep the honest corollary attached: --reset does NOT delete untracked files (cwcli never runs git clean). A definition left vague here would recreate the very ambiguity the fix removed. Full rationale in the AGENTS.md apps checkout entry.
_checkout_narrate forwards raw git output to stderr; _init_narrate deliberately does not forward raw bench output. Do not "unify" them. They answer different questions. Init's InitOutput is thousands of lines of bench-build noise an agent will not parse, and cwcli logs serves it afterwards, so narrating it is pure cost. A checkout runs two or three short git commands, a git step is NOT a supervised process so nothing is logged anywhere afterwards, and the entire reason a step failed lives in those bytes - git's own "couldn't find remote ref" or its auth failure is what tells an agent whether to fix the ref or the credentials. Drop it and those reach the agent as a bare ok: false with no cause. (The dirty-tree case no longer depends on this: it is refused before the fetch as its own typed error carrying the paths and --reset. The narration still matters for every failure that IS a git step.) Both narrators write only to stderr, so stdout stays one TOON document either way.
A standing recon item was judged and DECLINED on the code, not assumed: unifying core/update.py's _sites_with_app with core/apps.py's _installed_apps/list_apps installed-apps read. They are INVERSE questions (app->sites vs site->apps) with deliberately opposite failure semantics - _sites_with_app drops an unreadable site because it feeds a migration filter, while list_apps must distinguish "read failed" (None) from "no apps" ([]) or its exit code lies - reading different data shapes (the cache's raw bench list-apps lines vs a live first-token parse). The one genuinely shared line (the first-token parse) stays duplicated on purpose.
Judging that surfaced a real defect, REPORTED and deliberately kept dead: core/update.py:291's cache branch does exact list-membership (app in installed_apps) against get_cached_project_data's RAW bench list-apps lines, which on a real v16 bench are frappe 16.26.3 - so the cache branch is dead and _sites_with_app always falls through to its live query. Fail-SAFE (correct answer, slower); it survived because the tests seed the cache with bare names and take the cache branch, while the live fallback production always runs was 0% covered until TestSitesWithAppLiveFallback (tests/test_core_update.py) exercised it directly with realistic versioned cached data. Fixing the branch itself would still change update's behaviour on the input to a migration fan-out, so the captain chose to keep it dead-but-now-covered rather than fix it (openspec/changes/migrate-apps-core/tasks.md §8). core/update.py itself remains untouched.
Regression coverage for the core migration: tests/test_apps_characterization.py (the green-before net, committed before anything moved so refactor-under-green is auditable - commands/apps.py 91.28% -> 99.49%), tests/test_core_apps.py (every branch only axi/a future GUI can reach: confirm_start, select_bench, the no-cache default, confirm_uninstall, plus that the core prints nothing and returns plain serializable data), tests/test_axi_apps_list.py (TOON rendering, exit 0/1/2, the null-vs-empty-list distinction on a failed site read, and the install/uninstall non-registration assertion).
cwcli update deprecation + frappe special-case (folded into commands/update.py):
cwcli update is now a DEPRECATED alias that prints a notice and delegates to the shared run_app_update, which both apps update and update call - one implementation, no drift. run_app_update validates >=1 app then calls _update_project.
_update_project gained sites_filter (the repeatable --site narrowing, applied once to all_affected_sites via _apply_site_filter in the shared discovery pass) and an early frappe special-case: if any named app is frappe (case-insensitive), it runs _run_frappe_update_reset (bench update --reset, whole-bench) and returns - the per-app git pull loop is skipped, and --site does not apply (bench update is bench-wide).
--site matching zero affected sites refuses, non-zero. _fail_if_site_filter_matched_nothing distinguishes "genuinely nothing to migrate" (no site has the app installed at all - exits 0, unchanged) from "--site named a site the app isn't actually on" (a typo/mismatch): when sites_filter is non-empty AND the unfiltered affected-site set is non-empty AND filtering narrows it to empty, the command errors naming both the requested and the actually-affected sites and exits 1, rather than silently completing having migrated nothing. Applies to both apps update and the deprecated update alias (shared _update_project).
update.py control flow + maintenance-mode safety (u4/b8, 2026-07-11/12)
_update_project used to mis-nest the verbose vs non-verbose logic: only the header was gated on if verbose, the "verbose" body ran ALWAYS, and the whole non-verbose progress-bar block was the else: of the trailing if all_affected_sites: (clear-locks) - so it only ran when the affected set was EMPTY, and re-did git pull + discovery there (double pull, dead progress UI). Now the pull + discovery happen in ONE shared pass (_pull_apps -> _recache_after_pull -> _discover_affected_sites), then a single top-level if verbose:/else: keyed on --verbose (NOT all_affected_sites) picks the migration presentation (_run_migrations_verbose streams; _run_migrations_quiet uses console.status). The old rich Progress/Live/Spinner machinery and its throwaway pre-count discovery pass are gone; non-verbose now uses the same console.status spinner pattern the rest of the file already uses.
Maintenance-mode safety (all in _update_project + helpers):
_enable_maintenance turns maintenance ON per site, recording each success in maintenance_sites AS it is enabled - so a mid-loop exec crash still lets the finally disable exactly the sites that were actually enabled (the old bulk-then-record could lose already-enabled sites on an exception).
sites_to_migrate is sorted(maintenance_sites) (or sorted(all_affected_sites) when --skip-maintenance), and migration, the optional cache/website-cache clears, and lock-clearing all run over that same set - a site that could not enter maintenance is never migrated, cache/lock-cleared, or otherwise touched as if the update had run there.
- That skip is honestly surfaced, not silent:
failed_maintenance_enable (all_affected_sites - maintenance_sites, only computed when maintenance isn't skipped) is reported in the "Update completed with errors" summary ("could not enter maintenance mode - not migrated") and folds into has_errors -> non-zero exit, same as every other partial-failure phase.
_disable_maintenance (called in finally) turns maintenance OFF per site, checks each result, warns on a failed disable, and records stuck sites in failed_maintenance_disable -> which feeds has_errors -> non-zero exit (a stuck site is never left silent).
- Every shell interpolation in
update.py is shlex.quoted (site/app/path); docker-py shlex.splits string commands, so quoting yields valid tokens. _dir_exists passes its test -d check in LIST form (["sh", "-c", script]) so docker-py execs it directly rather than re-shlex.splitting a sh -c "..." string - that keeps shlex.quote(path) robust even for a path containing a single quote (a sh -c "..." string wrapper would break the outer quoting on such input).
Regression coverage: tests/test_apps.py (a FakeFrappeContainer recording every exec + streaming via a fake client.api; covers both modes, multi-site fan-out aggregation, git-URL derivation, the destructive/non-TTY refusals, cache-refresh, the frappe reset branch, and the deprecated-update warn+delegate). The u4/b8 control-flow/maintenance-mode fix above is covered by test_update_pulls_and_discovers_once and test_update_empty_affected_set_performs_neither_second_pass (one pull + one discovery pass, both presentations), test_update_migrate_skipped_for_site_not_in_maintenance (failed-enable skip + honesty: non-zero exit, summary line, no cache/lock-clear for the skipped site), test_update_stuck_site_warns_and_exits_nonzero (failed disable), and test_update_shell_interpolations_are_shlex_quoted. The apps commands are @handle_docker_errors-decorated, so tests patch docker_utils.shutil.which + docker_utils.docker.from_env (in the wired fixture) and pass every Typer param explicitly.
The summary MUST be reported from inside the finally (r2, 2026-07-15)
Reporting used to live AFTER the try/finally, so any raise jumped clean over it.
Batch 3 (PR #80) re-pointed _stream_command onto core.exec_stream, which RAISES CwcliError when the exit code is unknowable (a dropped connection: exec_inspect -> {"ExitCode": None, "Running": True}); _stream_command turns that into typer.Exit(1), which unwound past the seven-way aggregation.
It disclosed the hang-to-typed-error change but not this consequence: the finally held (maintenance always disabled-attempted - the invariant that matters most), but the summary never ran, so a genuinely stuck site lost its actionable bench --site X set-maintenance-mode off remediation and kept only a bare inline warn; failures accumulated before the raise were dropped; the abandoned fan-out went unreported.
Net still an improvement over the unbounded poll it replaced (an infinite hang is strictly worse than a truncated report with an honest exit code) - a NEW gap on a path that previously could not be reached, not a regression against anything a user had.
The fix and the two things that pin its shape - do not "tidy" either back:
- Report from within the
finally, AFTER _disable_maintenance - not from an except. The obvious alternative (catch, summarise, re-raise) is WRONG here: Python runs the except clause BEFORE the finally, and the remediation data (failed_maintenance_disable) is produced BY _disable_maintenance in that finally - so an except-based report prints while the list is still empty and omits the very line the fix exists to restore. Making it work needs nested trys and a duplicated report call. The finally is the only placement where the data already exists and every raise is covered structurally, without enumerating exception types.
_report_summary is PRINT-ONLY; the caller owns the exit. Raising from a finally REPLACES the in-flight exception - it would swallow the real error and any traceback from an unexpected one. So it returns has_errors and the trailing if has_errors: raise typer.Exit(1) sits after the try (unreachable when aborted: that exception propagates out of the finally and already exits non-zero, carrying its own message).
aborted is gated on bool(sites_to_migrate) (hence its [] init before the try): an abort is only worth reporting once there was a fan-out to abandon. Raising earlier (a --site typo, a failed pull) leaves nothing half-done, so the raise's own error stands alone instead of being dressed up as an interrupted update - while any failure already accumulated still reports. aborted also suppresses the success banner (no "Successfully updated N app(s)" over an update that stopped halfway) and is set in an except BaseException: that ONLY records and re-raises untouched - BaseException, not Exception, so a Ctrl-C is reported the same way (it leaves sites in maintenance exactly as a stream loss does).
Regression coverage: test_update_stream_loss_mid_fanout_still_reports_stuck_site_remediation (parametrized over verbose) drives the exact dropped-connection shape into a 2-site fan-out with the disable failing too, and asserts the summary + BOTH remediation lines survive; test_update_site_filter_refusal_is_not_reported_as_an_interrupted_update pins the sites_to_migrate gate. Both fail against unfixed source - the pre-existing green did not cover this (tests/test_apps.py covered 69.95% of update.py and only 2 of the 7 aggregation branches), which is exactly how it shipped. Note rich hard-wraps to console width, so assertions on the long remediation line must collapse whitespace first.
Where this lives now (m5): the state machine moved to core/update.py and the report is a RETURNED UpdateReport, so an unwinding exception can no longer skip it at all - the placement discipline above became structural rather than a convention. The two pins still hold in their new form: _report_summary (now in commands/update.py, taking the report) is still PRINT-ONLY with the caller owning the exit, and aborted is still gated on bool(sites_to_migrate). The except BaseException: that records aborted and re-raises untouched is still BaseException, not Exception, for the same Ctrl-C reason. See the m5 section below for what a returned report could NOT do, and what UpdateAborted exists to fix.
update on the logic core: three decisions that must not be undone (m5, 2026-07-15)
The state machine now lives in core/update.py (core.update(...) -> Result[UpdateReport]); commands/update.py is a renderer over it, and both cwcli apps update and the deprecated cwcli update are thin frontends over that ONE implementation. cwcli apps update --json and cwcli axi apps update emit the same report. The full argument is openspec/changes/migrate-update-core/.
core.update is NOT a generator, deliberately, and against locked decision 4's letter. Decision 4 says "streaming operations return typed event iterators"; a later agent reading it alone WILL reach for update_stream(plan) -> Iterator[UpdateEvent] and reopen a data-safety hole with an elegant-looking refactor. Do not. A try/finally inside a generator does NOT run when a consumer breaks early while holding a reference, or when the generator lands in a reference cycle - cleanup is deferred to the garbage collector. Probed, five consumer shapes: exhaust -> ran; break (refcount drops) -> ran; break holding a reference -> DID NOT RUN; reference cycle -> DID NOT RUN; contextlib.closing -> ran. A GUI pumping events from an event loop holds the iterator on self and lives in a widget cycle - exactly cases 3 and 4 - so a window closed mid-update strands a site in maintenance until GC happens to run, and enabling that GUI is the rework's own goal. The reading: decision 4 governs genuine STREAMING ops cwcli itself streams - raw exec output, which core.exec_stream is and remains (logs was once the other expected case; it settled as a deliberate NON-consumer instead - core/logs.py is a plan-only resolve and its --follow bytes never enter the Python process, so decision 4 does not govern it); update is a state machine that emits progress and whose terminal value is a report. contextlib.closing was rejected because it relocates a safety-critical guarantee into every frontend's hands, forever, including frontends not yet written. run_stream has the same exposure and is fine: an abandoned run_stream leaks a socket, not a stuck site.
UpdateAborted is why the callback exists at all. A RETURNED report cannot survive an unwinding KeyboardInterrupt - there is no return. Since a Ctrl-C mid-update must still surface a stuck site's remediation (the property PR #81 restored, above), the finally builds the report either way and hands it to on_event(UpdateAborted(report=...)) when unwinding. Delete that and Ctrl-C silently loses the remediation again.
- A lost stream is UNKNOWN, never a failure, and the fan-out CONTINUES. Before m5, a migration that returned non-zero was recorded and the fan-out continued, while a migration whose stream was LOST aborted it: the same real-world event, two behaviours, decided by whether Docker happened to record an exit code. They are one behaviour now. But unknown is NOT folded into
failed_*, and must never be: a lost stream means the exit code is unknowable and the command may still be running, so an agent branching on axi apps update will retry a "failure" and retrying a live migration does real harm. unknown_apps/unknown_migrations/unknown_builds sit alongside their failed_* counterparts (three, not seven: only pull/migrate/build stream inside the state machine; the frappe reset rides failed_apps/unknown_apps as the app frappe). Every exec-stream CwcliError is unknown, including exec.start_failed - it arguably means "it never ran", but that is a confident claim derived from an API call whose own outcome is uncertain, and the safe direction for a retry decision is unknown.
- The exit code reads
report.ok, NOT result.status. Every other axi verb does 0 if result.status in (OK, WARNING) else 1. Copying that here ships a fail-open: a partial update failure is a WARNING-shaped envelope carrying ok=False, so it would report success for an update that half failed - on the exact surface this batch exists to make honest. The closed Status set has no ERROR member by design (hard failures raise; update's partial failures must not, because reporting them all IS the job), so the aggregate rides the DTO.
cwcli axi apps update has no --yes, and must not grow one. A first cut threaded auto_start=yes straight into core.update with no start prologue: resolvers.resolve_container_state(auto_start=True, ...) only REPORTS ContainerState(start_requested=True) back to a caller that is supposed to perform the real, UI-coupled .start() itself (see its docstring) - core.update never did, so the exec against a still-stopped container raised a raw docker.errors.APIError instead of a clean TOON error. commands/update.py:run_app_update is safe ONLY because its ensure_containers_running(auto_start=yes) prologue actually starts the container BEFORE calling core.update; the agent verb has no such prologue and must not gain one (starting stays UI-coupled, off the core). The fix is not "add a start": it is dropping --yes entirely, so auto_start defaults to False, resolve_container_state returns NEEDS_CHOICE/confirm_start, and axi_apps_update renders that through the SAME emit_axi_choice_as_usage_error path every other stopped-project fork uses - a TOON usage error (exit 2) whose help: line names cwcli start, exactly like axi backup/axi unlock. An agent composes cwcli axi start then this verb. Do not re-add --yes/auto_start to the axi verb without also adding a real start prologue - and even then, that would reopen the "starting is UI-coupled" boundary this core migration exists to keep.
_run_frappe_update_reset hardcoded verbose=True and wrote bench output to stdout whatever the caller asked - which is why apps update was the only apps subcommand with no --json: a structured surface cannot call it. Fixed; non-verbose now runs the reset under a spinner, mirroring _pull_apps. The three pre-existing frappe tests all pass verbose=True and the base fake returns "" for the reset, so with no bytes to stream, streaming and not streaming looked identical - that is why nothing caught it. Any new test of this property needs a fake that actually emits output. Its recache-before-exit-code-check is PRESERVED deliberately: a failed reset still recaches, then reports failure (a partially-applied reset genuinely changes the cache). Do not "fix" it without an argument of its own.
update keeps its OWN two-directory bench probe (apps/ AND sites/) in core/update.py. resolvers.require_bench_dir probes sites/ ONLY: reusing it silently drops the apps/ check, and widening it changes backup/unlock. Reported as a near-miss, not resolved by bending the primitive. core.update also deliberately does NOT call validate_site_name/resolve_default_site - update uses neither, and adding them would add failures it does not have.
- LAYERING SIGNAL, SETTLED by
migrate-inspect-core (batch 7). core/update.py was the first core module to call utils.cache.recache_project, which (until that batch) lazily imported the inspect COMMAND - a core module reaching into the CLI layer at runtime, the one site of its kind. It could not be hoisted to the frontend (it runs mid-fan-out, between the pull and the discovery that depends on it), so the fix needed inspect's own migration, not a local workaround. Since inspect moved onto the core, recache_project's body calls core.inspect(refresh="full", offer_choice=False) directly; core/update.py's call into it is now an ordinary core-to-core call, and no module under core/ imports commands/ at runtime any more. utils/auto_inspect.py's daemon fallback was re-pointed the same way in the same batch (it used to import and call the inspect Typer command directly).
- The two frontends are NOT interface-identical.
apps update <proj> erpnext takes apps POSITIONALLY; the deprecated cwcli update <proj> --app erpnext takes them as a repeatable --app/-a OPTION. Unifying the signatures breaks every existing cwcli update ... --app x invocation. Both are pinned by an E2E leg against the real binary.
Regression coverage for the above: tests/test_core_update.py (the envelope, the probe, unknown-vs-failed, the maintenance lifecycle, the finally under a raise, UpdateAborted, the frappe fork, the callback), tests/test_axi_apps_update.py (both structured surfaces, stdout purity, exit 0/1/2), tests/test_update_characterization.py (the seven-way aggregation and every flag, written BEFORE the migration and passing unchanged either side of it), and `tests/e2e/test_apps_update_
…(truncated)
1---2name: cwcli-apps-update3description: Incident-level internals of cwcli's apps command group (list/install/uninstall/update/checkout) and the update state machine - multi-site fan-out by default, --json stdout purity, post-mutation recache, git-URL app-name derivation, the deprecated `cwcli update` alias and its frappe special-case, list/install/uninstall on the logic core (core/apps.py: the untangled uninstall consent/auto_start, report.ok-driven exit codes, `cwcli axi apps list`, `axi apps checkout`, and `axi apps install` shipping while `axi apps uninstall` stays deferred, and how the checkout and install absence assertions were each reversed), and core/update.py's control flow plus maintenance-mode safety (why it is NOT a generator, why a lost stream is unknown-not-failed, per-site enable/disable, honest partial-failure exit codes, shlex-quoted interpolation). Use this whenever you edit or debug commands/apps.py, core/apps.py, commands/update.py, core/update.py, or `axi apps list`/`axi apps update`, or touch app install/uninstall/update/check4---56# cwcli apps command group + update.py (sharp edges)78Each 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 contract lives in the always-loaded `AGENTS.md` "Important Components" section; this skill is the deep detail.910### `apps` command group: multi-site fan-out, JSON purity, update deprecation (2026-07-11; onto the logic core 2026-07-15)1112`commands/apps.py` is the first-class app manager (`list`/`install`/`uninstall`/`update`), registered as a Typer sub-app in `main.py` (`add_typer(apps_cmd.app, name="apps")`). As of batch 5 (`migrate-apps-core`) `list`/`install`/`uninstall` moved to `core/apps.py` (`list_apps`/`install_apps`/`uninstall_apps`, returning typed `AppsListing`/`AppsReport`); `commands/apps.py` is now a THIN RENDERER over the core for all four subcommands (`update` moved in batch 4, see below). It adds no new dependency or cache field. The original `list`/`install`/`uninstall` design was spec-driven via OpenSpec (`openspec/changes/add-app-management/`); the core migration's own artifacts live in `openspec/changes/migrate-apps-core/`.1314Load-bearing decisions, each guarding a real constraint - the substance is unchanged from the original design (every flag, message, exit code and `--json` shape is preserved byte-for-byte by the migration), only the file/function each one lives in moved:1516- **Multi-site by DEFAULT for install/uninstall/update.** No `--site` = fan out to ALL sites from the canonical `bench_sites.list_sites` (NOT `get_default_site` - that single-site model was rejected by the captain); `--site` is repeatable and narrows (`core/apps.py:_target_sites`). There is deliberately NO active/disabled-site distinction - one site set, the same `list_sites` everything else uses. The fan-out is **continue-and-report-all**: run every `(app, site)` step, collect a per-step `AppResult(app, site, action, ok)`, and exit non-zero if ANY failed - never a success banner over a partial failure. `commands/apps.py:_report_and_exit` renders the aggregate and reads the exit code off `report.ok`/`listing.ok`, NOT the envelope's `result.status` (the closed `Status` set has no `ERROR` member, so a partial failure is `WARNING`-shaped and every other verb maps `WARNING` to exit 0 - copying that pattern here would report success for a half-failed uninstall). Stop-on-first-failure was explicitly rejected.17- **EVERY verb that changes app code on disk routes through ONE resynchronise step** (`core.supervision.resync_after_code_change`; `core.apps._resync_after_mutation` is its report adapter). `bench get-app` changes the bench environment after the interpreter started, so a successful `install-app` could return `ok: true` while site requests failed with `ModuleNotFoundError`; uninstall leaves the inverse stale-code risk; `checkout` leaves the bench serving the branch the developer just moved off (the QUIETEST form - nothing errors at all); and `update`'s `git pull` is the same fault again. Four verbs each growing their own restart is the drift the install/uninstall fix already avoided between two, so the step lives in `core.supervision` - NOT `core.apps` - because `core.update` is a separate module that needs it and `supervision` already owns discovery, per-program restart and the site-routed probe. Callers pass only the thing they alone know: which sites they changed (install/uninstall: the sites the fan-out landed on; `checkout`: the sites with that app installed, via `core.apps._sites_with_app_installed` - "checkout has no target site" was true of its ARGUMENTS and false of its EFFECT; `update`: `sites_to_migrate`; the frappe bench-wide reset: every site on the bench). **WHAT IS CYCLED: every code-bearing program, not just `web`** (`supervision.code_bearing_programs`: `web`, `schedule`, and every worker spelling, keyed off the normalized label's base so `worker_default`/`worker_short` need no list). This closes the residual the first fix reported: workers and the scheduler import the same app code, so after an install they cannot import the new app, after an uninstall they act on tables that are gone, and after a checkout they silently run the old branch. `socketio`/`watch` (node) and `redis_*` are deliberately excluded - they import no Frappe app and cycling redis drops the cache and the job queue for nothing. The honest cost, disclosed rather than hidden: a worker restart interrupts a job in flight (RQ shuts down warm on SIGTERM inside supervisord's stop window); that is the same disturbance `cwcli restart` causes and the one the README already directed users to after a mutation, and a worker running code that no longer exists is not a safer state to leave behind. **Three properties, each guarding something specific.** (1) A stopped bench stays stopped: programs are cycled only when a `required` process read POSITIVELY establishes a live supervisord with a serving `web`, and only programs already `RUNNING`/`STARTING` are touched, so a deliberately-stopped worker is not started by an install; an unreadable read is a REPORTED failure, never "assume nothing is running" (`core.where`'s fail-honest rule) - the `required=True` on BOTH discovery calls is what makes that true and a test pins its presence. A manager cwcli does not own (honcho, plain `bench start`) is probed but never restarted, and an unhealthy site there gets a manual-restart remedy. (2) It NEVER raises: the code change has already landed, so every failure comes back in `ResyncOutcome`; a raise would erase the record of a change that genuinely happened and an automation caller that retried would run the mutation twice. (3) Restarting is NOT proof - "supervisord reports RUNNING" is a bound-port-shaped claim, and serving stale code is exactly the class of defect such a claim misses - so every named site must answer `/api/method/ping` with HTTP 200 (bounded; each probe gets only the remaining budget, or a server that accepts and never answers hangs the loop past its own timeout) before the outcome is `ok`. A failed restart, unreadable process state or port, probe exception, or non-200 makes `AppsReport.ok`/`UpdateReport.ok` false and the command non-zero without erasing the landed mutation. The restart's exit code is read `not in (0, None)` like every other container read here - `None` means docker recorded no code, not failure, and the site probe is the real gate. The step is DISCLOSED: each program is ANNOUNCED as it is cycled (`AppsAnnounce(phase="restart-processes", app=<program>)` / `UpdateStepStart(phase="resync")`) and reported as its own row on the human, `--json` and `axi` surfaces - the defect is that a bench was mutated underneath running processes silently, and a fix that restarts silently repeats the habit on a command that can then sit in the probe wait for up to a minute. **`update`'s POSITION is load-bearing**: the resync runs inside the `finally` AFTER the maintenance-mode disable, because a site in maintenance answers 503 and the proof could never be obtained; it is SKIPPED on an abort, where `aborted: true` already says the run is half-applied and a Ctrl-C should not gain a restart plus a bounded wait. `UpdateReport` grew `restarted_processes`/`unserved_sites`/`resync_error` and `_build_report` folds them into `ok`. No extra consent, unlike `scale`: the caller already chose an app mutation and this cycles the programs of the ONE bench they named, whose web may otherwise stay broken; a consent gate whose refusal leaves the site down would be the defect, not a guard. **The failure is real but NOT reproducible on demand - do not read a non-reproduction as proof the guard is unnecessary.** It was observed twice on real v16 benches (the operator's, and a `200 -> 500` on `/api/method/ping` straight after `axi apps install`), but a later deliberate attempt - raw `bench install-app` into a running bench, no restart - served `200` from the stale process on a freshly provisioned v16 bench. Whether the running interpreter trips depends on what the request path imports and whether the dev server's reloader happened to pick the change up, which is exactly why the resync is UNCONDITIONAL rather than gated on detecting the bad state: a condition that only sometimes appears is one a conditional guard will sometimes miss. The real-Docker guards are `test_apps_resync_e2e.py` (install -> checkout -> update off ONE install, asserting each code-bearing program got a NEW pid, that `socketio`/`watch`/`redis_*` kept theirs, and that the site serves 200 at every step - a statement about real supervisord pids that no mock can make) and `test_axi_apps_install_permitted_then_refused_on_rerun`, which proves HTTP 200 BEFORE checking installed-state assertions; the ordering is the point, because checking only that no error was raised is exactly what missed this.18- **`--json` stdout purity.** `console` is stdout (see `utils/console.py`), so in JSON mode NOTHING but the final `json.dumps` may touch stdout. The core emits `AppsOutput` events tagged by stream; the frontend's `_make_renderer` picks the consumption mode - buffer-and-join to stderr-on-failure when `json_output`, direct `sys.stdout.write` when not (mirrors `run.py`) - because that choice is rendering, not logic. A destructive `apps uninstall --json` without `--yes` REFUSES (json implies non-interactive; a `confirm_or_exit` prompt would fight the JSON contract, and its `--yes` ack prints to stdout).19- **Post-mutation refresh uses `cache.recache_project`, NOT `partial_inspect_known_benches`/`core.partial_refresh`.** The partial pass is READ-ONLY (never persists) and cannot refresh per-site `installed_apps` - exactly what install/uninstall change. Only a full recache (which routes through `cache_project_data` -> `_redact_config_for_cache`, so no secret is ever written) makes `where`/`open`/`inspect` honest. Degrades to a warning; the mutation already succeeded. This refresh is deliberately NOT in `core/apps.py` - see the batch 5 section below for why it stays a frontend epilogue here rather than following `core/update.py`'s example of a mid-fan-out call.20- **`<app>` is a name OR a git URL** passed straight to `bench get-app`; the per-site `install-app` name is derived from the actual `apps/` before/after diff (the real dir `bench get-app` created), NOT parsed from the target string - `core/apps.py:derive_app_name`'s URL-basename-minus-`.git` heuristic is only a FALLBACK for when that diff is ambiguous (zero or more than one new `apps/` entry, e.g. the app was already present). `uninstall-app` is always invoked with bench's own `--yes` so a non-TTY exec never hangs on bench's internal confirm (cwcli's `confirm_or_exit` gate is the human confirmation). `uninstall` only removes the app from site(s); deleting its code from the bench is out of scope (that is `bench remove-app`, run manually).2122#### `apps`'s `list`/`install`/`uninstall` on the logic core: what a reviewer would flag as a mistake and isn't (batch 5, 2026-07-15)2324`core/apps.py` owns resolution (container + bench only - no default-site, no site-name validation, no bench-dir probe: `apps` uses none of those today, and reaching for them because they exist would ADD failures it does not have), the container I/O, and the fan-outs; it RETURNS `AppsListing`/`AppsReport` and prints nothing. Five decisions guard real behaviour, and each looks like an inconsistency if you read only the diff:2526- **The recache stays a FRONTEND epilogue** (`commands/apps.py:_refresh_cache`, gated on `any(r.ok for r in report.results)`), NOT hoisted into the core the way `core/update.py` calls `cache.recache_project` mid-fan-out. That call exists in `update` only because its recache runs MID-fan-out and cannot be moved out; `install`/`uninstall`'s recache is a post-mutation step gated on a condition already in the returned report, so an epilogue costs nothing. (Before `migrate-inspect-core`, `update`'s mid-fan-out call was also the one core-module-imports-CLI-layer site in the codebase, since `recache_project` invoked the `inspect` Typer command; since that batch `recache_project`'s body calls `core.inspect` instead, so this is now an ordinary core-to-core call, not a layering exception.)27- **`uninstall`'s fused `--yes`** ("skip the destructive confirmation AND auto-start containers") keeps that exact CLI meaning - a migration does not get to change the contract - but `core.uninstall_apps` takes `auto_start` and `consent` as SEPARATE parameters and returns `NEEDS_CHOICE`/`confirm_uninstall` for the destructive gate. The fusion is one frontend's UX choice; a core that must also serve `axi` and a future GUI must not inherit it. `commands/apps.py:_uninstall` closes over `consent` and is called TWICE on the interactive path - once with `yes`, and again with `True` after `confirm_or_exit` returns - which is the re-invoke shape every `NEEDS_CHOICE` frontend uses (`--yes` short-circuits it on the first call; `tests/test_apps_characterization.py`'s interactive-confirm test is what proves the second call actually runs the uninstall rather than just passing).28- **`AppsAnnounce` and `AppsCommand` are two events, not one**, because the pre-migration code interleaves them: it announces "Fetching x...", THEN reads `apps/` (echoing that read under `--verbose`), THEN echoes the get-app command itself. Fusing the two into one event would reorder `--verbose` stderr.29- **The `warnings` field carries notes an agent should act on; the command trace rides the event channel.** The original batch-5 proposal said `list_apps` needs no callback ("nothing to report progress about") - right about progress, wrong about the trace: routing `$ ls -1 apps` through `warnings` would put a debug echo inside `axi apps list`'s structured TOON document. `list_apps` takes `on_event` like the other two verbs (and like `core.update`); `axi` passes nothing and the trace is discarded for free (`_noop`).30- **`cwcli axi apps list` ships; `axi apps uninstall` deliberately does NOT** (captain-locked 2026-07-15).31 `bench uninstall-app` drops the app's tables, and letting an agent do that is a product decision on its own evidence, not a side effect of moving code onto the core.32 `tests/test_axi_apps_list.py::test_uninstall_is_deliberately_not_a_verb` asserts the absence so it cannot be misread as an oversight.33 Decision 4 above made the eventual verb cheap: the core already exposes destructive consent separately from auto-start.34 **`axi apps install` SHIPPED 2026-07-20**, reversing only the half of this deferral it never actually covered.35 Its user-facing contract lives in `README.md`, while the safety invariant and regression references live in the `AGENTS.md` `apps` entry.36 `uninstall` stays deferred under the unchanged rationale.37- **`cwcli axi apps checkout` SHIPPED 2026-07-20, reversing its own absence assertion - and HOW it was reversed is the durable lesson.** The old assertion sat beside the install/uninstall one and read "`apps checkout` mutates the in-instance checkout, so LIKE install/uninstall it is a HUMAN verb only". That "like" is where the reasoning slipped: install/uninstall were held for the ONE threat named above (dropping an app's tables), and `checkout` was grouped with them for merely being a mutation, so a rationale it was never covered by silently became a block. Two things settled it, both verified rather than asserted: (1) `core.checkout_app` runs `git fetch` + `git checkout -B` inside `apps/<app>` - no bench command, no site, no SQL, no table - and `migrate-apps-core/tasks.md` §7.2 had already pre-authorized a different answer for non-deleting mutations; (2) **"the agent surface is read-only" is not a rule this repo follows** - most live `axi` verbs mutate (see the `AGENTS.md` `apps checkout` entry for the current count and enumeration), `axi init` provisions an entire instance and `axi apps update` runs schema migrations across live sites, so no read-only principle can be what withholds a git fetch. Generalize this, do not just remember the outcome: when you find an absence pinned by a test, read the rationale the test INHERITED and check it actually reaches the thing it is blocking. `axi init` is the same story (deferred, pinned, then shipped on its own evidence). The verb itself shipped as a thin renderer with zero `core/` delta; its guards and the deferred resulting-commit read are in the `AGENTS.md` entry and `openspec/changes/add-axi-apps-checkout-verb/`.38- **The dirty-tree guard is cwcli's own, and how it got there is the lesson.** `axi apps checkout` originally had NO dirty-tree pre-check by design, leaning on git's refusal of a checkout that would OVERWRITE a modified file, and the docs described that as "a dirty tree fails". CI on PR #129 proved the claim false: a dirty file the target ref does not touch has nothing to conflict with, so it rode through at exit 0. The obvious fix was to soften the docs; the captain ruled the opposite - **fulfil the promise rather than scrub the doc**, because a false SAFETY claim is acted on (a user leaves uncommitted work in an app dir believing it is protected), and these checkouts live in a SHARED dev instance where the work carried across may not be the runner's. `core.apps._refuse_dirty_tree` now refuses `CONFLICT`/`app.dirty_tree` before any fetch. Generalize this: when a doc and the code disagree about a SAFETY guarantee, the doc is not automatically the thing that is wrong. **What counts as dirty is stated, not implied, and it was itself revised once** - the first cut passed `--untracked-files=no`, reasoning that the guard should cover exactly what `--reset` destroys; the captain reversed that, so it is now plain `git status --porcelain` and untracked files refuse too, because a new module not yet `git add`ed is uncommitted work and the tool must not decide it is worthless. That widening is only safe because `.gitignore` keeps build residue out of `git status` entirely - a real-bench E2E asserts a freshly provisioned app is genuinely clean rather than assuming it. Keep the honest corollary attached: `--reset` does NOT delete untracked files (cwcli never runs `git clean`). A definition left vague here would recreate the very ambiguity the fix removed. Full rationale in the `AGENTS.md` `apps checkout` entry.39- **`_checkout_narrate` forwards raw git output to stderr; `_init_narrate` deliberately does not forward raw bench output. Do not "unify" them.** They answer different questions. Init's `InitOutput` is thousands of lines of bench-build noise an agent will not parse, and `cwcli logs` serves it afterwards, so narrating it is pure cost. A checkout runs two or three short git commands, a git step is NOT a supervised process so nothing is logged anywhere afterwards, and the entire reason a step failed lives in those bytes - git's own "couldn't find remote ref" or its auth failure is what tells an agent whether to fix the ref or the credentials. Drop it and those reach the agent as a bare `ok: false` with no cause. (The dirty-tree case no longer depends on this: it is refused before the fetch as its own typed error carrying the paths and `--reset`. The narration still matters for every failure that IS a git step.) Both narrators write only to stderr, so stdout stays one TOON document either way.4041**A standing recon item was judged and DECLINED on the code, not assumed:** unifying `core/update.py`'s `_sites_with_app` with `core/apps.py`'s `_installed_apps`/`list_apps` installed-apps read. They are INVERSE questions (app->sites vs site->apps) with deliberately opposite failure semantics - `_sites_with_app` drops an unreadable site because it feeds a migration filter, while `list_apps` must distinguish "read failed" (`None`) from "no apps" (`[]`) or its exit code lies - reading different data shapes (the cache's raw `bench list-apps` lines vs a live first-token parse). The one genuinely shared line (the first-token parse) stays duplicated on purpose.4243**Judging that surfaced a real defect, REPORTED and deliberately kept dead:** `core/update.py:291`'s cache branch does exact list-membership (`app in installed_apps`) against `get_cached_project_data`'s RAW `bench list-apps` lines, which on a real v16 bench are `frappe 16.26.3` - so the cache branch is dead and `_sites_with_app` always falls through to its live query. Fail-SAFE (correct answer, slower); it survived because the tests seed the cache with bare names and take the cache branch, while the live fallback production always runs was 0% covered until `TestSitesWithAppLiveFallback` (`tests/test_core_update.py`) exercised it directly with realistic versioned cached data. Fixing the branch itself would still change `update`'s behaviour on the input to a migration fan-out, so the captain chose to keep it dead-but-now-covered rather than fix it (`openspec/changes/migrate-apps-core/tasks.md` §8). `core/update.py` itself remains untouched.4445Regression coverage for the core migration: `tests/test_apps_characterization.py` (the green-before net, committed before anything moved so refactor-under-green is auditable - `commands/apps.py` 91.28% -> 99.49%), `tests/test_core_apps.py` (every branch only `axi`/a future GUI can reach: `confirm_start`, `select_bench`, the no-cache default, `confirm_uninstall`, plus that the core prints nothing and returns plain serializable data), `tests/test_axi_apps_list.py` (TOON rendering, exit 0/1/2, the null-vs-empty-list distinction on a failed site read, and the install/uninstall non-registration assertion).4647`cwcli update` deprecation + frappe special-case (folded into `commands/update.py`):48- `cwcli update` is now a DEPRECATED alias that prints a notice and delegates to the shared `run_app_update`, which both `apps update` and `update` call - one implementation, no drift. `run_app_update` validates >=1 app then calls `_update_project`.49- `_update_project` gained `sites_filter` (the repeatable `--site` narrowing, applied once to `all_affected_sites` via `_apply_site_filter` in the shared discovery pass) and an early **frappe special-case**: if any named app is `frappe` (case-insensitive), it runs `_run_frappe_update_reset` (`bench update --reset`, whole-bench) and returns - the per-app `git pull` loop is skipped, and `--site` does not apply (bench update is bench-wide).50- **`--site` matching zero affected sites refuses, non-zero.** `_fail_if_site_filter_matched_nothing` distinguishes "genuinely nothing to migrate" (no site has the app installed at all - exits 0, unchanged) from "`--site` named a site the app isn't actually on" (a typo/mismatch): when `sites_filter` is non-empty AND the unfiltered affected-site set is non-empty AND filtering narrows it to empty, the command errors naming both the requested and the actually-affected sites and exits 1, rather than silently completing having migrated nothing. Applies to both `apps update` and the deprecated `update` alias (shared `_update_project`).5152#### `update.py` control flow + maintenance-mode safety (u4/b8, 2026-07-11/12)5354`_update_project` used to mis-nest the verbose vs non-verbose logic: only the header was gated on `if verbose`, the "verbose" body ran ALWAYS, and the whole non-verbose progress-bar block was the `else:` of the trailing `if all_affected_sites:` (clear-locks) - so it only ran when the affected set was EMPTY, and re-did `git pull` + discovery there (double pull, dead progress UI). Now the pull + discovery happen in ONE shared pass (`_pull_apps` -> `_recache_after_pull` -> `_discover_affected_sites`), then a single top-level `if verbose:`/`else:` keyed on `--verbose` (NOT `all_affected_sites`) picks the migration presentation (`_run_migrations_verbose` streams; `_run_migrations_quiet` uses `console.status`). The old `rich` `Progress`/`Live`/`Spinner` machinery and its throwaway pre-count discovery pass are gone; non-verbose now uses the same `console.status` spinner pattern the rest of the file already uses.5556Maintenance-mode safety (all in `_update_project` + helpers):57- `_enable_maintenance` turns maintenance ON **per site**, recording each success in `maintenance_sites` AS it is enabled - so a mid-loop exec crash still lets the `finally` disable exactly the sites that were actually enabled (the old bulk-then-record could lose already-enabled sites on an exception).58- `sites_to_migrate` is `sorted(maintenance_sites)` (or `sorted(all_affected_sites)` when `--skip-maintenance`), and migration, the optional cache/website-cache clears, and lock-clearing all run over that same set - **a site that could not enter maintenance is never migrated, cache/lock-cleared, or otherwise touched as if the update had run there.**59- That skip is honestly surfaced, not silent: `failed_maintenance_enable` (`all_affected_sites - maintenance_sites`, only computed when maintenance isn't skipped) is reported in the "Update completed with errors" summary ("could not enter maintenance mode - not migrated") and folds into `has_errors` -> **non-zero exit**, same as every other partial-failure phase.60- `_disable_maintenance` (called in `finally`) turns maintenance OFF per site, checks each result, warns on a failed disable, and records stuck sites in `failed_maintenance_disable` -> which feeds `has_errors` -> **non-zero exit** (a stuck site is never left silent).61- Every shell interpolation in `update.py` is `shlex.quote`d (site/app/path); docker-py `shlex.split`s string commands, so quoting yields valid tokens. `_dir_exists` passes its `test -d` check in LIST form (`["sh", "-c", script]`) so docker-py execs it directly rather than re-`shlex.split`ting a `sh -c "..."` string - that keeps `shlex.quote(path)` robust even for a path containing a single quote (a `sh -c "..."` string wrapper would break the outer quoting on such input).6263Regression coverage: `tests/test_apps.py` (a `FakeFrappeContainer` recording every exec + streaming via a fake `client.api`; covers both modes, multi-site fan-out aggregation, git-URL derivation, the destructive/non-TTY refusals, cache-refresh, the frappe reset branch, and the deprecated-`update` warn+delegate). The u4/b8 control-flow/maintenance-mode fix above is covered by `test_update_pulls_and_discovers_once` and `test_update_empty_affected_set_performs_neither_second_pass` (one pull + one discovery pass, both presentations), `test_update_migrate_skipped_for_site_not_in_maintenance` (failed-enable skip + honesty: non-zero exit, summary line, no cache/lock-clear for the skipped site), `test_update_stuck_site_warns_and_exits_nonzero` (failed disable), and `test_update_shell_interpolations_are_shlex_quoted`. The `apps` commands are `@handle_docker_errors`-decorated, so tests patch `docker_utils.shutil.which` + `docker_utils.docker.from_env` (in the `wired` fixture) and pass every Typer param explicitly.6465#### The summary MUST be reported from inside the `finally` (r2, 2026-07-15)6667**Reporting used to live AFTER the `try/finally`, so any raise jumped clean over it.**68Batch 3 (PR #80) re-pointed `_stream_command` onto `core.exec_stream`, which RAISES `CwcliError` when the exit code is unknowable (a dropped connection: `exec_inspect` -> `{"ExitCode": None, "Running": True}`); `_stream_command` turns that into `typer.Exit(1)`, which unwound past the seven-way aggregation.69It disclosed the hang-to-typed-error change but not this consequence: the `finally` held (maintenance always disabled-attempted - the invariant that matters most), but the summary never ran, so **a genuinely stuck site lost its actionable `bench --site X set-maintenance-mode off` remediation** and kept only a bare inline warn; failures accumulated before the raise were dropped; the abandoned fan-out went unreported.70Net still an improvement over the unbounded poll it replaced (an infinite hang is strictly worse than a truncated report with an honest exit code) - a NEW gap on a path that previously could not be reached, not a regression against anything a user had.7172The fix and the two things that pin its shape - **do not "tidy" either back**:7374- **Report from within the `finally`, AFTER `_disable_maintenance` - not from an `except`.** The obvious alternative (catch, summarise, re-raise) is WRONG here: Python runs the `except` clause BEFORE the `finally`, and the remediation data (`failed_maintenance_disable`) is produced BY `_disable_maintenance` in that `finally` - so an `except`-based report prints while the list is still empty and **omits the very line the fix exists to restore**. Making it work needs nested `try`s and a duplicated report call. The `finally` is the only placement where the data already exists and every raise is covered structurally, without enumerating exception types.75- **`_report_summary` is PRINT-ONLY; the caller owns the exit.** Raising from a `finally` REPLACES the in-flight exception - it would swallow the real error and any traceback from an unexpected one. So it returns `has_errors` and the trailing `if has_errors: raise typer.Exit(1)` sits after the try (unreachable when aborted: that exception propagates out of the `finally` and already exits non-zero, carrying its own message).76- **`aborted` is gated on `bool(sites_to_migrate)`** (hence its `[]` init before the `try`): an abort is only worth reporting once there was a fan-out to abandon. Raising earlier (a `--site` typo, a failed pull) leaves nothing half-done, so the raise's own error stands alone instead of being dressed up as an interrupted update - while any failure already accumulated still reports. `aborted` also suppresses the success banner (no "Successfully updated N app(s)" over an update that stopped halfway) and is set in an `except BaseException:` that ONLY records and re-raises untouched - `BaseException`, not `Exception`, so a Ctrl-C is reported the same way (it leaves sites in maintenance exactly as a stream loss does).7778Regression coverage: `test_update_stream_loss_mid_fanout_still_reports_stuck_site_remediation` (parametrized over `verbose`) drives the exact dropped-connection shape into a 2-site fan-out with the disable failing too, and asserts the summary + BOTH remediation lines survive; `test_update_site_filter_refusal_is_not_reported_as_an_interrupted_update` pins the `sites_to_migrate` gate. Both fail against unfixed source - **the pre-existing green did not cover this** (`tests/test_apps.py` covered 69.95% of `update.py` and only 2 of the 7 aggregation branches), which is exactly how it shipped. Note `rich` hard-wraps to console width, so assertions on the long remediation line must collapse whitespace first.7980**Where this lives now (m5):** the state machine moved to `core/update.py` and the report is a RETURNED `UpdateReport`, so an unwinding exception can no longer skip it at all - the placement discipline above became structural rather than a convention. The two pins still hold in their new form: `_report_summary` (now in `commands/update.py`, taking the report) is still PRINT-ONLY with the caller owning the exit, and `aborted` is still gated on `bool(sites_to_migrate)`. The `except BaseException:` that records `aborted` and re-raises untouched is still `BaseException`, not `Exception`, for the same Ctrl-C reason. See the m5 section below for what a returned report could NOT do, and what `UpdateAborted` exists to fix.8182#### `update` on the logic core: three decisions that must not be undone (m5, 2026-07-15)8384The state machine now lives in `core/update.py` (`core.update(...) -> Result[UpdateReport]`); `commands/update.py` is a renderer over it, and both `cwcli apps update` and the deprecated `cwcli update` are thin frontends over that ONE implementation. `cwcli apps update --json` and `cwcli axi apps update` emit the same report. The full argument is `openspec/changes/migrate-update-core/`.8586- **`core.update` is NOT a generator, deliberately, and against locked decision 4's letter.** Decision 4 says "streaming operations return typed event iterators"; a later agent reading it alone WILL reach for `update_stream(plan) -> Iterator[UpdateEvent]` and reopen a data-safety hole with an elegant-looking refactor. **Do not.** A `try/finally` inside a generator does NOT run when a consumer breaks early while holding a reference, or when the generator lands in a reference cycle - cleanup is deferred to the garbage collector. Probed, five consumer shapes: exhaust -> ran; break (refcount drops) -> ran; **break holding a reference -> DID NOT RUN**; **reference cycle -> DID NOT RUN**; `contextlib.closing` -> ran. A GUI pumping events from an event loop holds the iterator on `self` and lives in a widget cycle - exactly cases 3 and 4 - so a window closed mid-update strands a site in maintenance until GC happens to run, and enabling that GUI is the rework's own goal. The reading: decision 4 governs genuine STREAMING ops cwcli itself streams - raw exec output, which `core.exec_stream` is and remains (`logs` was once the other expected case; it settled as a deliberate NON-consumer instead - `core/logs.py` is a plan-only resolve and its `--follow` bytes never enter the Python process, so decision 4 does not govern it); `update` is a state machine that emits progress and whose terminal value is a report. `contextlib.closing` was rejected because it relocates a safety-critical guarantee into every frontend's hands, forever, including frontends not yet written. `run_stream` has the same exposure and is fine: an abandoned `run_stream` leaks a socket, not a stuck site.87- **`UpdateAborted` is why the callback exists at all.** A RETURNED report cannot survive an unwinding `KeyboardInterrupt` - there is no return. Since a Ctrl-C mid-update must still surface a stuck site's remediation (the property PR #81 restored, above), the `finally` builds the report either way and hands it to `on_event(UpdateAborted(report=...))` when unwinding. Delete that and Ctrl-C silently loses the remediation again.88- **A lost stream is UNKNOWN, never a failure, and the fan-out CONTINUES.** Before m5, a migration that returned non-zero was recorded and the fan-out continued, while a migration whose stream was LOST aborted it: the same real-world event, two behaviours, decided by whether Docker happened to record an exit code. They are one behaviour now. But unknown is NOT folded into `failed_*`, and must never be: a lost stream means the exit code is unknowable and **the command may still be running**, so an agent branching on `axi apps update` will retry a "failure" and retrying a live migration does real harm. `unknown_apps`/`unknown_migrations`/`unknown_builds` sit alongside their `failed_*` counterparts (three, not seven: only pull/migrate/build stream inside the state machine; the frappe reset rides `failed_apps`/`unknown_apps` as the app `frappe`). Every exec-stream `CwcliError` is unknown, **including `exec.start_failed`** - it arguably means "it never ran", but that is a confident claim derived from an API call whose own outcome is uncertain, and the safe direction for a retry decision is unknown.89- **The exit code reads `report.ok`, NOT `result.status`.** Every other axi verb does `0 if result.status in (OK, WARNING) else 1`. Copying that here ships a fail-open: a partial update failure is a `WARNING`-shaped envelope carrying `ok=False`, so it would report success for an update that half failed - on the exact surface this batch exists to make honest. The closed `Status` set has no `ERROR` member by design (hard failures raise; update's partial failures must not, because reporting them all IS the job), so the aggregate rides the DTO.90- **`cwcli axi apps update` has no `--yes`, and must not grow one.** A first cut threaded `auto_start=yes` straight into `core.update` with no start prologue: `resolvers.resolve_container_state(auto_start=True, ...)` only REPORTS `ContainerState(start_requested=True)` back to a caller that is supposed to perform the real, UI-coupled `.start()` itself (see its docstring) - `core.update` never did, so the exec against a still-stopped container raised a raw `docker.errors.APIError` instead of a clean TOON error. `commands/update.py:run_app_update` is safe ONLY because its `ensure_containers_running(auto_start=yes)` prologue actually starts the container BEFORE calling `core.update`; the agent verb has no such prologue and must not gain one (starting stays UI-coupled, off the core). The fix is not "add a start": it is dropping `--yes` entirely, so `auto_start` defaults to `False`, `resolve_container_state` returns `NEEDS_CHOICE`/`confirm_start`, and `axi_apps_update` renders that through the SAME `emit_axi_choice_as_usage_error` path every other stopped-project fork uses - a TOON usage error (exit 2) whose `help:` line names `cwcli start`, exactly like `axi backup`/`axi unlock`. An agent composes `cwcli axi start` then this verb. Do not re-add `--yes`/`auto_start` to the axi verb without also adding a real start prologue - and even then, that would reopen the "starting is UI-coupled" boundary this core migration exists to keep.91- **`_run_frappe_update_reset` hardcoded `verbose=True`** and wrote bench output to stdout whatever the caller asked - which is why `apps update` was the only `apps` subcommand with no `--json`: a structured surface cannot call it. Fixed; non-verbose now runs the reset under a spinner, mirroring `_pull_apps`. **The three pre-existing frappe tests all pass `verbose=True` and the base fake returns "" for the reset, so with no bytes to stream, streaming and not streaming looked identical - that is why nothing caught it.** Any new test of this property needs a fake that actually emits output. Its **recache-before-exit-code-check is PRESERVED deliberately**: a failed reset still recaches, then reports failure (a partially-applied reset genuinely changes the cache). Do not "fix" it without an argument of its own.92- **`update` keeps its OWN two-directory bench probe** (`apps/` AND `sites/`) in `core/update.py`. `resolvers.require_bench_dir` probes `sites/` ONLY: reusing it silently drops the `apps/` check, and widening it changes `backup`/`unlock`. Reported as a near-miss, not resolved by bending the primitive. `core.update` also deliberately does NOT call `validate_site_name`/`resolve_default_site` - `update` uses neither, and adding them would add failures it does not have.93- **LAYERING SIGNAL, SETTLED by `migrate-inspect-core` (batch 7).** `core/update.py` was the first core module to call `utils.cache.recache_project`, which (until that batch) lazily imported the `inspect` COMMAND - a core module reaching into the CLI layer at runtime, the one site of its kind. It could not be hoisted to the frontend (it runs mid-fan-out, between the pull and the discovery that depends on it), so the fix needed `inspect`'s own migration, not a local workaround. Since `inspect` moved onto the core, `recache_project`'s body calls `core.inspect(refresh="full", offer_choice=False)` directly; `core/update.py`'s call into it is now an ordinary core-to-core call, and no module under `core/` imports `commands/` at runtime any more. `utils/auto_inspect.py`'s daemon fallback was re-pointed the same way in the same batch (it used to import and call the `inspect` Typer command directly).94- **The two frontends are NOT interface-identical.** `apps update <proj> erpnext` takes apps POSITIONALLY; the deprecated `cwcli update <proj> --app erpnext` takes them as a repeatable `--app`/`-a` OPTION. Unifying the signatures breaks every existing `cwcli update ... --app x` invocation. Both are pinned by an E2E leg against the real binary.9596Regression coverage for the above: `tests/test_core_update.py` (the envelope, the probe, unknown-vs-failed, the maintenance lifecycle, the `finally` under a raise, `UpdateAborted`, the frappe fork, the callback), `tests/test_axi_apps_update.py` (both structured surfaces, stdout purity, exit 0/1/2), `tests/test_update_characterization.py` (the seven-way aggregation and every flag, written BEFORE the migration and passing unchanged either side of it), and `tests/e2e/test_apps_update_9798…(truncated)