Add rollback-safe self-update replacement transaction

[AviBackToBlack]

Summary

• consume only the opaque, previously checksum-and-provenance-verified release result and re-hash staged bytes before mutation
• serialize replacement with registry and shim mutations
• discover only same-file or byte-identical managed shims, leaving unrelated executables untouched
• replace the management executable and complete proven shim set using same-volume temporary files
• run an exact bootstrap version smoke test and re-check every resulting digest
• restore the full prior managed set after any failure, preserving a private actionable recovery binary if rollback cannot complete

This is the Windows replacement transaction itself. The temporary helper that waits for the invoking process to exit and the user-facing apply command remain separate follow-up work, so cb self-update --check stays read-only.

Validation

• gofmt -l .
• go vet ./...
• go test -race ./...
• python3 -m unittest -v internal/registry/pipx_wrapper_test.py internal/cli/pipx_discovery_test.py
• Windows amd64 full-package cross-build
• Windows arm64 full-package cross-build

Roadmap: #2 (RM-31)

---

[devin-ai-integration]

Devin Review found 2 potential issues.

[CherylSnowVeil]

Reviewed head cd1c6d6 — the rollback-safe self-update replacement transaction (internal/selfupdate/apply.go, tests, README, package doc). Initial review; CI was inspected, including the Windows job logs.

Findings

[blocking] canonicalApplyFile rejects valid Windows paths — all five new apply tests fail on Windows CI (internal/selfupdate/apply.go:270-293)

The check !strings.EqualFold(resolved, path) requires the caller's path to equal its filepath.EvalSymlinks canonicalization. On Windows, EvalSymlinks normalizes components beyond mere casing — notably 8.3 short names (RUNNER~1 → runneradmin) and junctions. os.TempDir()/t.TempDir() on the GitHub Windows runners return exactly such paths, so every new test fails there: installed management executable path resolves to "C:\Users\runneradmin\AppData\Local\Temp\...\cb.exe" instead of itself. Both Test and build (Windows) and Test and build (Windows ARM64) are red on this head.

This is not only a test-environment quirk: in production, ApplyVerified (and applyFileDigest/requireApplyDigest, which wrap canonicalApplyFile for every pre-replace and rollback digest check) fail closed for any install path not already in resolved form. The sibling helpers make the intended convention explicit — canonicalVerificationFile (verify.go:198) and canonicalInstalledExecutableDir (stage.go:258) both return the resolved path instead of demanding textual equality, and the existing tests already normalize with EvalSymlinks before comparing (mustResolveTestPath in verify_test.go:415, resolvedDestination in stage_test.go:46). Fix: have canonicalApplyFile return resolved (and use it as installDir/staged), rather than rejecting non-canonical-but-valid caller strings.

[important] Case-sensitive directory comparison in the staging-layout check (internal/selfupdate/apply.go:74)

filepath.Dir(stageDir) != installDir compares the EvalSymlinks-canonicalized staged directory (from Verified.binaryPath, which Verify already resolves) against installDir, which keeps the caller's casing since canonicalApplyFile returns the input path verbatim. On Windows a correct staged layout is rejected whenever the caller's installedExecutable differs in case from the on-disk path (e.g. C:\TOOLS\cb.exe vs C:\Tools) — "verified executable is outside the exact private same-volume staging layout". The adjacent volume check already uses strings.EqualFold; the directory and filepath.Base(staged) != "cb.exe" comparisons should be case-insensitive too, or performed on canonicalized paths.

[important] Rollback restores shims as hardlinks to the private-DACL recovery binary (internal/selfupdate/apply.go:209, 315-338)

restrictStagingPath(rollbackBinary, false) gives the rollback copy a protected owner-only DACL (D:P(A;;FA;;;<current user>)). On rollback, tx.replace(rollbackBinary, shim, true) prefers os.Link, so each restored shim becomes a hardlink to that inode and permanently carries the private staging security descriptor — the inode survives os.RemoveAll(rollbackDir). Consequences: restored shims lose whatever install-dir-inherited ACL they had, and diverge from cb.exe itself (restored via copy with preferHardlink=false, so it gets the inherited ACL). On a shared/multi-principal install, principals that previously could execute the shims can no longer do so after a rollback. This also cuts against the RM-31 requirement to "preserve ACL expectations" (docs/roadmap-implementation-requirements.md:441). Consider restoring shims with preferHardlink=false (digest equality already guarantees discovery on future runs) or reapplying the directory-inherited ACL after linking.

[nit] Crash-time litter (internal/selfupdate/apply.go:316-328)

A hard crash inside replaceManagedFile leaves .cb.exe-update-*.tmp files (and crashed runs leave .container-bin-update-* staging dirs) in the install directory with no sweep on entry. Harmless, but a small cleanup pass or documented expectation would keep the directory tidy.

Summary

The transaction design itself is solid: re-hash of the verified bytes before mutation, serialization via the shared mutation lock, proven-shim discovery by os.SameFile/digest rather than name, per-file digest validation before and after each replacement, byte-aware handling of ambiguous replace results, and a recovery artifact preserved when rollback can't complete. The tests exercise the rollback paths well. The blocking item is the path-canonicalization check — it fails on the project's own Windows CI and would fail closed for legitimate installs; the fix (use the resolved path, as the sibling canonicalizers do) is small and consistent with existing conventions. Note ApplyVerified is intentionally unwired pending the helper slice, so --check remains read-only as documented.

[AviBackToBlack]

Windows x64 CI exposed that EvalSymlinks expands the runner temp directory from its 8.3 spelling, so the original self-path textual equality check rejected the same file before reaching replacement. Fixed in 87e8a24: canonicalization now re-stats and requires SameFile identity, then returns the resolved path. This preserves fail-closed identity checking without rejecting valid 8.3/case-normalized paths. I also built the Windows selfupdate test binary and executed it directly on this host: all transaction, rollback, MoveFileExW replace-existing, verifier, staging, and Windows authentication tests passed. The Linux race suite and vet pass as well.

[CherylSnowVeil]

Re-review of head ae2e4885 (previously reviewed cd1c6d6). Re-validated each earlier finding against the current tree and CI, then did a complete fresh pass over the full diff.

Validation of previous findings

• [blocking] canonicalApplyFile rejecting valid Windows paths — fixed in 87e8a24. The function now re-stats the EvalSymlinks result, requires os.SameFile(info, resolvedInfo) identity, and returns the resolved path (apply.go:285-312). Both Test and build (Windows) and Test and build (Windows ARM64) are green on this head, and the behavior now matches the sibling canonicalizers. The fail-closed identity property is preserved.
• [important] Case-sensitive staging-layout comparison — fixed. apply.go:74 now uses strings.EqualFold for both the cb.exe basename and filepath.Dir(stageDir) != installDir comparisons; both operands are canonicalized anyway, so this is correctly belt-and-suspenders.
• [important] Rollback hardlinking shims to the private-DACL recovery binary — fixed in ae2e4885. Shim rollback now uses preferHardlink=false (apply.go:224), so restored shims are copied through a fresh install-directory temp file and inherit the installation ACL. TestApplyTransactionRollsBackCompleteManagedSetAfterSmokeFailure asserts the restored shims are not os.SameFile with the recovery identity.
• [nit] Crash-time litter — still present, still a nit. .cb.exe-update-*.tmp files on hard crash, orphaned .container-bin-update-* staging dirs, and stale container-bin.mutation.lock from a killed helper have no sweep on entry (the lock already self-describes the manual recovery path, so this is consistent with existing design).

Also verified the 9d5dd6a hardening that pre-dated the fixes I reviewed: Verified now binds the installed path/version/digest/size via bindInstalledExecutable (hash + cb version smoke + post-hash), apply() re-checks that identity under the mutation lock (apply.go:94-98), and Windows replacement uses MoveFileExW with MOVEFILE_REPLACE_EXISTING|MOVEFILE_WRITE_THROUGH rather than os.Rename. All sound.

Fresh findings on ae2e4885

[suggestion] Inconclusive post-replace state on the management executable bypasses the recovery-artifact path (internal/selfupdate/apply.go:164-170)

If tx.replace(staged, installedExecutable, false) reports failure and the follow-up applyFileDigest(installedExecutable) itself errors — or returns bytes that are neither the old nor staged digest — apply returns a plain error and the deferred RemoveAll(rollbackDir) deletes the recovery copy. That is the only path where the post-mutation state is unknown yet the recovery artifact is discarded. Every other uncertain path goes through fail(), which preserves rollbackBinary when rollback cannot fully verify restoration. Impact is narrow: MoveFileExW is atomic so the common cases (clean failure → oldDigest, falsely-reported failure → stagedDigest) are handled correctly. But a transient read failure right after a reported replace failure (e.g., AV quarantine/lock on the destination) loses the artifact precisely when the state is least certain. Consider routing digestErr != nil and the neither-digest case through fail() — rollback already no-ops on oldDigest and keeps the recovery binary on anything it cannot restore.

[suggestion] Rollback-restored shims are now copies — cb doctor will warn (internal/selfupdate/apply.go:224, internal/diag/diag.go:289)

The ACL fix makes restored shims fresh files, so after any rollback every restored registered shim trips doctor's "shim %s is a copy, not a hardlink" warning until the next cb install. Hardlinking to the restored cb.exe (which carries a normal inherited install-dir ACL) would preserve both properties — though the current copy-from-rollbackBinary approach is arguably the more defensive choice since it restores correct bytes even when the management-executable restore itself failed. If keeping copies, a one-line note that post-rollback doctor warnings are expected/cleared by cb install would avoid confusion.

[nit] Docs now describe shipped phases as future work (docs/architecture.md:352, docs/roadmap-implementation-requirements.md:75, docs/roadmap-decisions.md:24)

architecture.md still says "Attestation/checksum verification and installed-file replacement remain separate later phases" — the verification half was already stale from #82, and the replacement transaction now exists internally (the user-facing apply command genuinely remains). The RM-31 status rows have the same drift. The repo reconciles roadmap docs per milestone, so a follow-up sweep is fine.

Summary

The previously flagged issues are all resolved, and the resolution approach is consistent with the codebase's conventions. The transaction itself reads correctly end to end: staged bytes are re-hashed before mutation, the Verified-bound installed identity is re-checked under the shared mutation lock, proven shims are discovered by os.SameFile/digest only, per-file digests are validated before and after each replacement, the ambiguous "reported failure after changing bytes" case is detected and rolled back, downgrade authorization is enforced at verification time and can't be forged through the opaque Verified, and rollback refuses to overwrite foreign changes while preserving an actionable recovery binary. Test coverage of forward, rollback, stale-install, changed-staged-bytes, ambiguous-failure, and recovery-preservation paths is good. No blocking or important issues found on this head — only the optional hardening and hygiene items above.

[AviBackToBlack]

Addressed CherylSnowVeil's latest re-review in signed commit a100de0. Reported replacement failures whose destination is unreadable or contains neither known digest now enter rollback and preserve the private recovery executable whenever restoration cannot be proved; both cases have regressions. Successful rollback now re-hardlinks updated shims to the verified restored cb.exe, preserving normal install ACLs and doctor-clean identity, while falling back to copied recovery bytes if cb.exe itself cannot be proved restored. RM-31 architecture and roadmap text is reconciled. Crash leftovers are documented rather than wildcard-deleted because a matching name alone does not prove ownership. Full race tests, vet, release-style version checks, and the complete compiled native Windows self-update suite pass.

[AviBackToBlack]

Conflict resolved in signed merge commit 2595cf5 after PRs #88 and #89 advanced main. The resolution keeps the merged WSL/image-trust roadmap updates and the RM-31 transaction status from a100de0. Full race tests, vet, version checks, and both Python suites pass on the merged tree.

[AviBackToBlack]

Main advanced again when PR #91 merged, so I resolved the new conflict in signed merge commit 52e3392. The merged verifier now authenticates and exactly extracts the ARM64 archive, then binds the installed cb.exe identity before the rollback-safe transaction—preserving both PRs' fail-closed guarantees. The roadmap text now records #91 as merged. Full race tests, vet, all 11 Python wrapper/discovery tests, the Windows ARM64 cross-build, and the complete native Windows self-update suite pass.

[CherylSnowVeil]

Re-review of head 52e3392 (previously reviewed ae2e4885). Re-validated each prior finding against the current tree, then did a complete fresh pass over the full PR, including the merged ARM64 verification code and the two merge commits. All CI legs are green; no Go toolchain exists in my environment, so dynamic verification rests on CI plus direct inspection.

Validation of previous findings

• [suggestion] Inconclusive post-replace state bypassed the recovery-artifact path — fixed. apply.go:164-184 now routes the digestErr != nil and neither-known-digest cases through fail() with changed = true, so rollback runs and the private recovery executable is preserved whenever restoration cannot be proved. TestApplyTransactionPreservesRecoveryAfterInconclusiveReplacementFailure covers both sub-cases.
• [suggestion] Rollback-restored shims were private-DACL copies that cb doctor would flag — fixed. rollback() (apply.go:247-256) now prefers tx.replace(installedExecutable, shim, preferHardlink=true) once the management executable is verifiably restored, so restored shims regain a normal install-inherited ACL and hardlink identity (diag.go:289 stays quiet); the private rollbackBinary copy is used only as the defensive fallback when restoration is unprovable, in which case rollback already reports failure and preserves the recovery artifact. TestApplyTransactionRollsBackCompleteManagedSetAfterSmokeFailure asserts os.SameFile on the restored set.
• [nit] Docs describing shipped phases as future work — fixed. docs/architecture.md:365-372, docs/roadmap-decisions.md, docs/roadmap-implementation-requirements.md, selfupdate.go package doc, and README now describe staging/verification/ARM64 selection/the transaction as implemented internals, with only the wait-helper and user-facing apply remaining.
• [nit] Crash-time litter — resolved via documentation. README now explicitly documents that .container-bin-update-* staging/rollback dirs and .cb.exe-update-*.tmp files can survive a hard crash and why they are not wildcard-deleted. Reasonable resolution.
• Round-1 findings re-spot-checked and still fixed: canonicalApplyFile returns the resolved path with an os.SameFile identity check (apply.go:317-344), and the staging-layout comparisons use strings.EqualFold (apply.go:74).

Fresh findings on 52e3392

[important] Stale dependency-graph invariants in docs/architecture.md (lines 307, 326, 332-334, 364)

This PR changes the package's import edges — apply.go now imports internal/mutationlock and internal/registry — but the doc still states the pre-change graph verbatim:

• internal/selfupdate ... (leaf) (line 307) and selfupdate in the (leaves) list (line 326);
• "mutationlock is reached only from main" (line 333) — it is now also reached from selfupdate;
• "internal/selfupdate has no project imports" (line 364), directly contradicted by the new imports; line 307's "canonical release selection and read-only plan" description is also stale now that the package contains the replacement transaction.

The coupling itself is deliberate and required by RM-31 (serialization with registry/shim mutations; ValidToolName/ReservedToolName for managed-shim scoping), and the no-cycle property still holds. Only the documented invariant is wrong — but this doc presents the graph as generated truth (go list output) and it now misstates a load-bearing boundary. Needs a small update to the edges list and the selfupdate paragraph.

[suggestion] Two rollback branches lack direct test coverage (apply.go:243-245, 250-256)

• The per-shim foreign-change refusal ("managed shim %s changed outside the update transaction; refusing to overwrite it during rollback") is never exercised — the management-executable equivalent is covered by the inconclusive-replacement tests, but no test mutates a shim mid-transaction.
• The successful managementRestored == false copy-fallback (source = rollbackBinary, preferHardlink = false) is unreached: TestApplyTransactionPreservesRecoveryArtifactWhenRollbackFails fails the shim restore itself rather than letting the fallback succeed while management restoration fails. A stubbed replace that fails only for installedExecutable while succeeding for shims would cover it.

Summary

The transaction still reads correctly end to end on this head: staged bytes are re-hashed before mutation, the Verified-bound installed path/version/digest/size is re-checked under the shared mutation lock, proven shims are discovered by os.SameFile/digest only, per-file digests are validated before and after each replacement, the ambiguous and inconclusive replacement outcomes enter rollback, foreign changes are refused rather than overwritten, and rollback preserves an actionable private recovery binary. The Verify changes (installed-executable binding via bindInstalledExecutable and plan-level downgrade-authorization checks in validateVerificationPlan) are fail-closed and consistent with the identical logic in stage.go, and the merge with #91's ARM64 extraction composes correctly — apply's staged-layout and digest checks hold for the extracted cb.exe too. ApplyVerified remains intentionally unwired, so --check stays read-only as documented. No blocking issues; the stale architecture-doc dependency claims should be corrected before merge.

[AviBackToBlack]

Addressed CherylSnowVeil's review in signed commit 4dc027f. docs/architecture.md now reflects the live project import graph: selfupdate depends on mutationlock and registry for the unexposed transaction, is no longer listed as a leaf, and its package description covers staging/verification/replacement; verification against go list also exposed and fixed the pre-existing omitted cli -> dockervol edge. Added direct rollback regressions for refusing a concurrently changed shim without overwriting it and for successfully restoring shims from the private recovery copy when management restoration cannot be proved. Full race tests, vet, all 11 Python tests, and the complete native Windows self-update suite pass.