refactor(update): decompose command by responsibility - #97634
cervantesh wants to merge 1 commit into
Conversation
9f1f78e to
cd38939
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head cd38939500c7aac9110a0cff0d68b244414d65f4, including the actual 20-file diff, the behavior-critical extracted modules/facade, compatibility and stale-module regressions, the public verification ledger, the closing issue, current open updater work, and the exact-head upstream workflows.
This is strong structural work. The decomposition is not just file motion with a hopeful re-export layer: update_cmd.py remains the historical patch surface, seeds the extracted consumers from _COMPAT_CONSUMERS, and installs a module-level __setattr__ bridge so later monkeypatches of historical globals propagate to the moved call sites. The regressions exercise that in more than one shape (_read_project_version, get_hermes_home, and a fleet-restart subprocess consumer), which addresses the highest-risk compatibility failure mode of this kind of extraction.
The other important side of this refactor is the running updater's generation boundary. update_runtime_refresh._STALE_PURGE_PROTECTED includes the full 15-module extracted updater graph, and test_purge_protects_executing_modules proves those exact module objects remain pinned while stale non-executing Hermes modules are evicted and rebuilt from the post-update checkout. That is the right invariant: do not let the process executing generation N accidentally import a generation N+1 sibling halfway through its own mutation/restart transaction.
I also checked the review-driven backup correction rather than relying on the PR narrative. The moved backup failure sites now resolve logging through the historical facade (_u().logger) instead of an unbound extracted-module name, while the existing fail-soft backup behavior remains intact. The public ledger is unusually good about its proof boundaries: 161/161 functions mapped, 155 AST-identical, six adaptations enumerated, 195/195 historical names present, zero missing propagation entries in the static audit, 56 focused tests, cross-platform focused runs, Linux updater and Windows-live before/after comparators, and native systemd/launchd/SCM restart-boundary equivalence. It also explicitly says what those witnesses do not prove rather than laundering them into a broader claim.
Exact-head CI for cd389395... is green on the tested composition: CI 33232923520, Docker 33232923216, and Nix 33232923194 all completed successfully. Within CI, Python tests, E2E, native Windows/macOS lanes, Ruff/ty, blocking Ruff, Windows footguns, attribution, OSV, supply-chain checks, and the final required-check aggregator are green.
Interlock / ownership check
I do not see this as superseding the open updater behavior work. It creates the ownership/routing boundary those PRs should compose through. In particular, #97547 (receipt completion classification), #97366 (multi-venv dependency/runtime ownership), #97350/#97450 (fleet-probe row expectations), #97056 (exact commit target semantics), #96853 (systemd generation boundary), and #96235 (aborted-restart generation recovery) are complementary behavior changes whose contributor credit and semantics must survive whichever merge order wins. #93042 is also still the provenance owner for the intentionally-unlanded canary/rollout transaction; this refactor must not turn that historical carrier into an implicit implementation claim.
The practical merge-order rule is straightforward: if one of those behavior PRs lands first, this decomposition must rebase and absorb the landed semantics into the corresponding extracted owner; if this lands first, those PRs should be rebased/ported onto the extracted modules rather than re-growing update_cmd.py. That is the other side of the shape here: a successful godfile extraction is incomplete if subsequent work keeps treating the compatibility facade as the implementation owner.
One release-gate blocker: the exact landing composition has moved
The branch and all of the cited exact-head evidence are pinned to base ee742fe1bc828f6456659f67d27bdbeacdebbdd4. The PR's current synthetic merge object is still 029f74da78f3485efdddeb22b480da5b80fde561, whose commit message is literally Merge cd389395... into ee742fe....
Current main is now 7eee066c30eda38b50bc8942a06a9c4b67b5d8ce. I compared ee742fe...7eee066... and found no direct path collision with this PR's hermes_cli/update_*, hermes_cli/_update_compat.py, or changed tests/hermes_cli/test_* paths, so I do not see evidence of a semantic conflict and I am not asking for a redesign. But the exact landing object has not been certified: the green workflows and before/after evidence prove ee742fe + cd389395, not 7eee066 + cd389395.
Required before merge: refresh/rebase onto current main (preserving Miguel Martinez / @cervantesh's three commits and attribution), then rerun the focused compatibility + stale-purge + backup gates and obtain fresh upstream CI/Docker/Nix on that resulting exact head/current-base composition. If the rebase remains mechanical, the existing deeper cross-platform evidence remains valuable provenance; the new exact-current object still needs its own green release receipt.
So: no code-semantic blocker found in the reviewed head; one exact-current-base verification blocker remains. The decomposition itself is careful, unusually well evidenced, and substantially improves the updater's ownership topology without pretending adjacent behavior work is duplicated. Nice work getting a 10k-line mutation/restart surface into reviewable owners while preserving the ugly compatibility reality it has accumulated. 🚀
cd38939 to
a654f00
Compare
|
Thanks for the careful exact-head review. I addressed the landing-composition gate in one bounded refresh.
The PR remains ready for review. I will avoid daily rebase churn and refresh again if relevant updater work lands near a maintainer review/merge window, or if maintainers request it. The SHAs above intentionally define the evidence frame even if unrelated |
a654f00 to
99daedd
Compare
|
Small evidence clarification after auditing the inherited local failures: the earlier A canonical clean-main audit receipt assigned the independently closable test-infrastructure defects to #98037, #98038, #98039, and #98040. None touches this PR diff or changes the behavior-preserving closure predicate. I updated the evidence package, manifest, and infographic accordingly. The product PR head remains |
99daedd to
ee40818
Compare
ee40818 to
49db219
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewing the materially new exact head 49db219ea01a265cd9b1c4f0ac58368c0eff9f52 against current main@593aa74c6182ce2e5e23bc102daaaae71710c05d. This is not a repeat of my earlier cd389395... review: the branch has been rebuilt on current main and now incorporates the later updater contracts that landed in the meantime.
The final tree is substantially stronger than the earlier frame. I checked the current 21-file diff, the behavior-critical extracted owners and facade, the compatibility propagation table, stale-module generation protection, the exact four-commit chain, the current issue/PR graph, and exact-head hosted CI. The head is mergeable, has no unresolved review threads, and the exact head is green across CI 33702564967, Docker 33702564514, and Nix 33702564513. The current decomposition also correctly follows the moved Git-network inspection sites and carries the newly-landed serve-recovery / reconciliation behavior into the new owner modules rather than silently reverting it.
I do have one release-blocking history/proof defect, plus two concrete follow-ups.
BLOCKER — the active commit chain is not self-consistent / exact-commit green
The first active commit, 7c23c6d8663de920c2c877849b01cf680c451c7c, is based directly on this PR's current base, but it does not carry the abort-recovery facade ABI that current main already requires.
This is not just “no CI ran on an intermediate commit.” There is a concrete broken object:
- Current-base
tests/hermes_cli/test_update_serve_generation_recovery.pycallsupdate_cmd._abort_recovery_is_complete(...)andupdate_cmd._surviving_pre_update_serve_runtimes(...)directly. - At exact commit
7c23c6d..., the newhermes_cli/update_cmd.pyfacade exports neither name. - The moved
hermes_cli/update_fleet_restart.pyat that same commit directly consumes_abort_recovery_is_complete,_surviving_pre_update_serve_runtimes,_recover_gateway_restart_after_abort, and_warn_stale_serve_runtimesthrough the compatibility-global model. - The later commit
17cdb78a4e558c8fcfc2499f194ba67aa421eae9is exactly the repair: it imports the abort-recovery names into the facade and adds the corresponding_COMPAT_CONSUMERSpropagation entries.
So the branch reaches a correct final tree by passing through an active commit that is not a valid current-main decomposition object. I also checked exact-object CI availability: 7c23c6d..., dc78338..., and 17cdb78... have no check-runs/workflow runs; only the final 49db219... object has the hosted green matrix. Final-head green cannot certify immutable earlier commits.
Required fix: rebuild the visible history so every active commit is self-consistent and receives an exact-commit green verdict. The smallest repair is to fold the 17cdb78... abort-recovery facade bridge into the initial extraction commit (preserving the superseded SHA/provenance in the PR record), then run the repository CI gates on every visible commit. I am not asking for a code redesign; this is a history-integrity / release-proof blocker exposed by a real intermediate-tree regression.
SHOULD-FIX — the generation-protection regression omits one executing owner
Production currently does the right thing: update_runtime_refresh._STALE_PURGE_PROTECTED includes hermes_cli.update_fleet_restart.
But tests/hermes_cli/test_update_stale_module_purge.py::test_purge_protects_executing_modules says it proves the “complete frozen execution graph” and its executing_names set omits hermes_cli.update_fleet_restart. That is exactly the sort of omission this decomposition needs tests to make impossible, because the failure mode is cross-generation module mixing in a live self-updater.
Please add hermes_cli.update_fleet_restart to that asserted frozen graph. The final implementation already satisfies the invariant; the durable regression should actually pin the whole claimed graph.
INTERLOCK / provenance — preserve the updater contracts this refactor is relocating
The rebase correctly absorbed newer behavior, but the PR's lineage text should name the canonical merged owners rather than leaving those changes as anonymous “current main” behavior:
- #96235 is the historical predecessor from JoaoMarcos44, closed unmerged.
- #100165 is the merged salvage of that serve-generation recovery, with authorship preserved.
- #100928 is the merged success-path/reconciliation follow-on, preserving the #100490 reconciliation work and #100493 survivor-warning work plus the field-report/root-cause lineage.
This PR does not supersede those contributions; it relocates their shipped behavior into bounded updater owners. Please say that explicitly so future archaeology does not confuse extraction ownership with behavioral authorship.
There are also two live FILE-LIST collisions that need an explicit merge-order rule:
- #92934 currently touches
hermes_cli/update_cmd.pyandtests/hermes_cli/test_update_stale_module_purge.pyto preservehermes_cli.update_receiptacross the purge. That is a complementary generation-safety defect, not duplicate work. If #97634 lands first, that fix belongs inupdate_runtime_refresh.py/ the relocated purge test rather than regrowing the facade. If #92934 lands first, this branch must rebase and absorb its protected-module contract into the extracted owner before recertifying. - #99450 is a much larger complementary updater-authority change and collides directly with
update_cmd.pyplus the stale-purge/update test surface. If it lands first, this decomposition must rebase and route its quiesce / restart / reconciliation semantics into the new owners; if this lands first, #99450 needs to port to those owners instead of treating the compatibility facade as the implementation surface.
Finally, #97633's issue body still names an older implementation head/base. The PR↔issue edge exists (Closes #97633), but the closure card should be refreshed to the current exact object after the history repair so the issue does not certify a superseded SHA.
Verdict
Final-tree semantics: no new blocker found. Active-history/release object: blocker remains.
The hard part of this rebase was done well: current-main behavior is actually composed rather than overwritten, the compatibility bridge is explicit, the extracted owners remain bounded, and exact-final-head CI is green. Fix the broken intermediate object, pin update_fleet_restart in the frozen-graph regression, and make the merged provenance/merge-order edges explicit; then this becomes a much cleaner landing object rather than merely a clean final snapshot. 🚀
c8d7493 to
3e695cd
Compare
Split the self-update implementation into bounded owner modules while preserving the update_cmd compatibility facade and current runtime behavior. Keep the shipped recovery and reconciliation contracts from NousResearch#100165 and NousResearch#100928 intact, and pin the complete frozen updater execution graph.
c8d7493 to
3e695cd
Compare
|
@andrexibiza, thank you for the exact-object review. I addressed each item in the current review frame. Ready-for-review receipt
The previous four-commit chain is superseded. The extraction, abort-recovery facade exports, compatibility propagation, relocated source-inspection tests, and frozen-graph regression now coexist in one visible commit, so there is no active intermediate ABI gap. The prior SHAs remain in the review record for archaeology, but are not presented as certified objects. The frozen execution-graph regression now explicitly includes The PR and #97633 now identify the shipped behavior provenance: #96235 as the historical unmerged predecessor, #100165 as the merged serve-recovery salvage, and #100928 as the merged success/reconciliation follow-on preserving the #100490 and #100493 lineage. They also state the merge-order rules for #92934 and #99450. Neither live PR's implementation is incorporated or duplicated here. Current exact-head evidence:
The PR body and issue closure card now point to this exact frame. With the requested history, regression, provenance, and interlock updates in place—and all required checks green—I am returning the PR to Ready for Review. |
Current-main supersession reviewI refreshed the integration frame before attempting another rebase. The correct disposition has changed: #102117 merged an independent whole-codebase decomposition and already implements this PR's structural objective on current Verified on that exact
I also reviewed what remains unique here. This branch propagates arbitrary reassignment of private facade globals into extracted owners through One non-blocking test-hardening opportunity remains: current production protects all eight split updater owners, while A rebase preview conflicts in six shared files and would retain both this PR's 13-owner topology and main's |
Note
Superseded on current
main. PR #102117 independently landed the same structural objective and is present inmain@1e69c12b64dba4090eddffeae27f5a147068d34b. Rebasing this branch would combine two incompatible decompositions rather than preserve a useful patch. This PR is retained as historical design, review, and evidence provenance; its architecture and exact-head receipts do not describe currentmain.What does this PR do?
Decomposes the 11,250-physical-line
hermes_cli/update_cmd.pyimplementation into focused modules by existing operational responsibility while retainingupdate_cmd.pyas a 504-line compatibility facade.This is a behavior-preserving refactor with two explicit safety contracts:
Related issue
Closes #97633
Current integration frame
main@48c0c3a873bc5adaf20c632b5b7630a4fac000b4d205375a3a63c440fefc14e0a777700f24a8d12ecervanteshThe previous four-commit review frame (
7c23c6d866,dc78338bb5,17cdb78a4e,49db219ea0) has been superseded by the single commit above. This folds the abort-recovery facade bridge into the extraction itself, so there is no longer an intermediate active object with an incomplete ABI. The earlier SHAs remain recorded in the review discussion for archaeology; no CI claim is made for them.Responsibility boundaries
update_orchestrator.pyupdate_source.pyupdate_zip.pyupdate_dependencies.pyupdate_runtime_refresh.pyupdate_process_guard.pyupdate_gateway_posix.pyupdate_gateway_windows.pyupdate_fleet_restart.pyupdate_reconciliation.pyupdate_backup.pyupdate_desktop.pyupdate_notices.pyFollowing andrexibiza's review on #96235, this refactor uses 2,000 physical lines as a review-specific maintainability guard, not as a claimed repository policy. Every extracted production module remains below that guard; the largest is
update_orchestrator.pyat 1,934 physical lines. The already-largetests/hermes_cli/test_backup.pyremains outside that production-module decomposition boundary.Behavioral authorship and refactor provenance
This PR authors the decomposition. It does not claim authorship of the updater behaviors that it relocates from
update_cmd.pyinto bounded owners. For the specific recovery and reconciliation behaviors called out in the review, the relevant reporting, implementation, and integration lineage is:hermes updateleaves running services on stalesys.moduleswhen the auto-restart phase aborts on an ImportError #92145.hermes updateleaves non-unitserveruntimes on pre-update code — and because they run cron tickers, every agent-mode cron job then fails with ImportError #100479.Those merged commits already exist in this branch's
mainancestry, so they should not be cherry-picked again. This PR preserves their shipped behavior while changing its module ownership. The single commit at this PR's head, authored by @cervantesh, represents the decomposition and compatibility work only.Live merge-order interlocksThese are complementary changes, not work absorbed by this refactor:
update_runtime_refresh.pyand the relocated purge test, not regrow the facade. If fix(update): preserve active receipt across module purge #92934 lands first, this branch must rebase, carryhermes_cli.update_receiptinto the extracted protected graph, and recertify.Neither PR's implementation is duplicated here.
Mechanical-equivalence audit
Compared with the exact current base:
_m,_holder_value_flags,_resolve_pre_update_backup_mode,_run_pre_update_backup,_check_and_apply_config_migration, and_cmd_update_impl;hermes_cli.update_cmd; and_cmd_update_implresolves tohermes_cli.update_orchestrator.Review-driven regressions
Independent review identified three concrete proof requirements, all represented in the current atomic object:
test_purge_protects_executing_modulesnow includeshermes_cli.update_fleet_restart, matching the complete production_STALE_PURGE_PROTECTEDexecution graph.The modified-skill notice remains evaluated per source file, and source-inspection tests follow Git network calls and runtime allowlists to their extracted owners.
Exact-head validation
154 passedlocallyd205375a3a181/181;175identical;6declared adaptations226/226; none missingcompileallexit0; diff-check exit0A broader local Windows diagnostic completed with
719 passed, 54 skipped, 24 failedafter updater tests spawned live gateway processes from the test worktree. The exact processes were inventoried and removed. A representative isolated failure reproduces unchanged on the exact basemain@48c0c3a873, so it is not attributed to this refactor and is not used as a green acceptance claim.Historical operational evidence
The earlier evidence package remains useful provenance for the design and before/after harnesses, but its SHAs are historical and are not presented as validation of the current head:
The infographic is a navigation aid; the current exact-head audits and hosted checks are authoritative for this refresh.
Checklist
mainupdater behavior is composed into the extracted owners.update_fleet_restartis pinned in the frozen execution-graph regression.