Repository navigation
Add published self-update qualification harness - #126
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Process execution can hang indefinitely, and the documented release workflow does not enforce or record the new per-candidate gate.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds a Windows/amd64 harness for qualifying published self-update transactions and records the release gate.
Changes:
- Adds isolated end-to-end self-update qualification.
- Verifies replacement, shims, provenance, and cleanup.
- Updates roadmap and release documentation.
| File | Description |
|---|---|
scripts/qualify-self-update-published.ps1 |
Adds the qualification harness. |
docs/roadmap-implementation-requirements.md |
Updates self-update completion requirements. |
docs/roadmap-decisions.md |
Records the qualification decision. |
docs/release-matrix.md |
Documents execution and evidence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Review: Add published self-update qualification harness
Initial review of 20d0134 (COMMENT only; checks were still pending at review time, but this change is docs + a manual PowerShell harness, so Go CI coverage is unaffected).
What I verified
- The harness's exercised contract matches the implementation:
cb.exe self-update --apply --version V --gh-executable Pparses viainternal/selfupdate/selfupdate.go::ParseArgs;cb.exe versionis a bootstrap command requiring no registry/host-frontend (main.go), so the temp install works. - Shim reconciliation coverage is real:
discoverManagedShims(internal/selfupdate/apply.go) scans the install dir for regular.exefiles whose name is a valid non-reserved tool name and whose bytes match the old digest.go/gofmtqualify; staged/helper/rollbackcb.execopies live in subdirectories and are correctly ignored. - The staged/helper/rollback private directory prefixes all match the harness's
.container-bin-update-*sweep; the helper's deferred staged cleanup and the detached PowerShell helper-dir cleanup explain why the 1-minute drain loop is needed. - Helper stdout/stderr are inherited from the parent (
startSelfUpdateHelper), so the apply invocation's redirected-pipe read effectively waits for the helper to finish — the subsequent version poll is a belt-and-suspenders confirmation rather than the primary signal.launchHelperCleanupcorrectly detaches (nil std handles), so it does not hold the pipe. - Qualification evidence is accurate: published
v1.1.0exists withcb.exe= 3,383,808 bytes, amd64 archive andSHA256SUMS, matchingdocs/release-matrix.md. - Build flags (
-trimpath -buildvcs=false -ldflags "-s -w -X main.version=...") matchrelease.ymlexactly; fail-closed temp-path validation, re-validation beforeRemove-Item, and never printing the token are all consistent with the repo's stated security posture. - Docs edits are internally consistent: remaining "E2E" statements now correctly refer to Windows-on-Arm + Docker Desktop qualification, and the roadmap retains the rerun-per-release-candidate gate.
Findings
No blocking or important issues. Two optional observations:
- 🟢 [nit]
scripts/qualify-self-update-published.ps1:174— the private-artifact sweep only matches.container-bin-update-*. Other private update files can outlive the transaction without matching:.{name}-update-*.tmpreplacement temps created byreplaceManagedFile(e.g..go.exe-update-*.tmp), andcontainer-bin.mutation.lock, whichmutationlock's unlock deliberately leaves behind if its re-read fails or the token mismatches. If a transaction completes but leaves one of those, the harness would still emitprivate_artifacts = 0and the "no private update artifact remained" evidence claim would be overstated. Consider asserting an exact allowlist of expected survivors (cb.exe,go.exe,gofmt.exe) — it is a stronger statement for a release-candidate gate. - 🟢 [nit]
Invoke-ExactProcesshas no timeout:WaitForExit()and theReadToEndAsyncresults block indefinitely. For the apply call this means the script already waits for the full helper transaction (inherited pipe), so a wedged helper — or a hunggoshim build — stalls qualification with no diagnostic. A bounded wait + kill on expiry would make hangs reportable as failures instead.
Summary
A careful, fail-closed harness that genuinely exercises the complete published-release transaction end to end — exact selection, canonical bounded staging, checksum + provenance, private helper handoff, transactional replacement, hardlink and byte-identical shim reconciliation, and private-artifact cleanup — with the surrounding roadmap/release-matrix docs updated accurately and the 2026-10-04 evidence verifiable against the real v1.1.0 release. Good to merge once CI settles; the two notes above are optional hardening for future reruns.
|
Addressed both Cheryl optional hardening notes, plus the two overlapping Copilot findings, in
The hardened harness passed again against canonical published v1.1.0 assets with both shims hardlinked and no unexpected survivor. PowerShell parsing and Please re-review the current head when convenient. |
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review: published self-update qualification harness (d9e1a3c)
Validation of previous findings
Verified each prior finding against the current head and the actual self-update implementation:
- 🟢 [nit] Private-artifact sweep too narrow — FIXED. The post-transaction check now asserts an exact survivor allowlist (
cb.exe,go.exe,gofmt.exe) viaGet-ChildItem -Force ... -notin $expectedSurvivors, with a 1-minute drain for the detached helper-dir cleanup. This genuinely covers everything the old.container-bin-update-*glob missed:replaceManagedFile's.{name}-update-*.tmptemps (apply.go:367),container-bin.mutation.lockresidue thatmutationlock's unlock deliberately leaves on re-read mismatch/failure (mutationlock.go:65-75), and the staging/helper/rollback directories. It also catches anything else unexpected — a strictly stronger gate than the fix I suggested. - 🟢 [nit] Unbounded
Invoke-ExactProcesswait — FIXED.TimeoutSecondsis now mandatory (ValidateRange(1,3600)), every call site is bounded (300/30/900/30/30s), and the timeout covers both process exit and inherited-pipe drain within one deadline — important becausestartSelfUpdateHelperpassesos.Stdout/os.Stderrthrough to the helper, soReadToEndAsynconly completes when the helper exits. On expiry the script kills the direct tree viaKill($true)and sweeps executables underOwnedProcessRoot, which correctly catches a wedgedcb-update-helper.exeliving in.container-bin-update-helper-*under the qualification root. It correctly does not sweep the detached cleanuppowershell.exe(System32 path,DevNullstd handles — doesn't hold the pipe). - Copilot: release workflow lacked per-candidate gate — FIXED.
docs/release-matrix.mdnow gives parameterized previous-release → published-candidate instructions and.github/ISSUE_TEMPLATE/release-qualification.ymladds a required "Published-release self-update E2E" field demanding the retained JSON.
Fresh review of current head
- Timeout model is correct:
WaitForExit(ms)plusWhenAll(...).Wait(remaining)share one deadline, so an exited-but-pipe-holding transaction (parent exits, helper inherits the redirected pipes) is still bounded; the post-kill 10s task drain is best-effort and preserves the timeout as the primary error. OwnedProcessRootprefix matching is case-insensitive with proper trailing-separator handling and per-processPathaccess guarded — scoped to the unique GUID temp dir, so it cannot kill unrelated processes.- The documented candidate flow is implementable: exact
--versionselection accepts prerelease tags likev2.0.0-rc.1(only drafts are rejected,selfupdate.go:317), the tag parses under the sameValidatePatternused forFromVersion/TargetVersion, andExpectedRef/ReleaseURLcanonicalization works for prerelease tags. - The allowlist assertion can't false-positive on this fixture:
ApplyVerified's mutation lock is removed on the normal path,Staged.Cleanup/helper-dir cleanup are fail-closed about what they delete, andgofmt.exe's copy becomes a real hardlink viapreferHardlink— matching thefsutil hardlink listassertion for both shims. - Docs are internally consistent: the evidence paragraph, the "bounded child-process execution / exact survivor allowlist" description, the roadmap's rerun-per-candidate gate, and the issue-template field all match the implementation. CI is green.
Findings
No blocking or important issues. Two optional diagnostics notes:
- 🟢 [nit]
scripts/qualify-self-update-published.ps1:262-270— if thetryblock throws and thefinallyRemove-Item -Recurse -Forcethen hits a terminating error (e.g., a file still locked by a just-killed helper in delete-pending state, or a racing detached cleanup), the removal error replaces the original failure — losing the more useful diagnostic (e.g., which survivor artifact was found). Best-effort cleanup (-ErrorAction Continue+ warning) or wrapping the removal in try/catch that appends rather than replaces would keep the primary failure visible. - 🟢 [nit]
scripts/qualify-self-update-published.ps1:212-214— the version-pollcatchdiscards each attempt's failure reason, so a run where every poll times out reportslast output: <unavailable>without indicating the failure mode (timeout vs. non-zero exit vs. wrong version). Recording$_.Exception.Messageon failure would make a persistently wedged post-update executable distinguishable in the qualification evidence.
Summary
The hardening round landed cleanly and is verified against the real transaction code rather than just the claims — the bounded-wait design correctly handles the inherited-pipe helper case, and the exact allowlist is a meaningfully stronger post-transaction invariant than the prefix sweep it replaces. Docs, the issue-template gate, and the recorded evidence are all consistent with the implementation. Good to merge; the two notes above are optional.
|
Addressed both optional re-review diagnostics notes in a8a349a: version polling now preserves the final timeout/non-zero failure message, and best-effort qualification-root cleanup warns instead of replacing the primary failure. Revalidated PowerShell parsing, the live published v1.0.0 to v1.1.0 transaction (PASS; 2 shims; 0 private artifacts), go test -race ./..., go vet ./..., and git diff --check. The commit is intentionally unsigned because signing is disabled. |
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review: published self-update qualification harness (a8a349a)
Validation of previous findings
Both diagnostics notes from the d9e1a3c round are verified fixed in the current head:
- 🟢 Best-effort root cleanup — FIXED. The
finallyblock now wrapsRemove-Item -Recurse -Force -ErrorAction Stopin try/catch that downgrades removal failures toWrite-Warning(scripts/qualify-self-update-published.ps1:276-281). A locked or racing file can no longer replace the primary qualification failure. The fail-closed re-validation of the resolved path before deletion is preserved. - 🟢 Version-poll failure diagnostics — FIXED. The poll loop now records
$lastAttemptError—$_ .Exception.Messageon timeout/exception andexit code N; stderr: ...on non-zero exit — and the failure message reportslast erroralongsidelast output(scripts/qualify-self-update-published.ps1:204-225). A persistently wedged post-update executable is now distinguishable in the retained evidence (timeout vs. non-zero exit vs. wrong version).
Fresh review of the current PR
Re-checked the complete diff (20d0134..a8a349a → script, three docs, issue template) against the implementation rather than the delta alone:
- Contract still matches
internal/selfupdate:--applyrequirescb.exebasename (selfupdate.go:215), exact--versionselection,--gh-executableabsolute path;versionis a bootstrap command; managed-shim discovery and the helper handoff semantics are unchanged. - Evidence claims verified against the live release:
v1.1.0publishescb.exe(3,383,808 bytes), the amd64 archive andSHA256SUMS, matchingdocs/release-matrix.md;e625ab6is the recorded base commit. - The documented "go may be a native executable or a ContainerBin shim" claim holds: the default
go/golangtool profile whitelistsGOOS/GOARCHinenv_names(internal/registry/registry.go:267), so the injected build-target env propagates through a shim into the container. Worst case under an unshared temp path is a fail-closed build error, not a false pass. - Issue-template gate is valid YAML and consistent with the new required per-candidate step; docs edits in
release-matrix.md,roadmap-decisions.mdandroadmap-implementation-requirements.mdare internally consistent. git diff --checkclean. No pwsh locally to re-parse, but the delta is small and structurally simple.
Findings
No blocking or important issues. One residual nit in the new diagnostics code:
- 🟢 [nit]
scripts/qualify-self-update-published.ps1:213-215— the non-zero-exitelsebranch records$lastAttemptErrorbut does not reset$after, while thecatchbranch resets it to$null. If a poll returns exit 0 with the wrong version and a later attempt exits non-zero, the final message pairslast outputfrom the earlier attempt withlast errorfrom the latest one — two diagnostics labeled "last" that came from different attempts, which could be confusing when correlating a failed run. Setting$after = $nullin theelsebranch (or labeling the fieldslast successful output/last attempt error) would keep the pair consistent. Gate correctness is unaffected — the loop condition still requires a matching version to pass.
Summary
Both prior findings are properly fixed and the rest of the harness, docs and issue-template gate remain verified against the actual transaction code and published assets. Good to merge; the note above is optional.
|
Addressed the residual diagnostics nit in 5ed46f3: a non-zero version probe now clears the previous successful-but-wrong result, so last output and last error always describe the same latest attempt. Revalidated PowerShell parsing, the live published v1.0.0 to v1.1.0 transaction (PASS; 2 shims; 0 private artifacts), go test -race ./..., go vet ./..., and git diff --check. The commit is intentionally unsigned because signing is disabled. |
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review: published self-update qualification harness (5ed46f3)
Validation of previous findings
The residual nit from the a8a349a round is verified fixed against the current head:
- 🟢 Poll diagnostics pairing — FIXED. The non-zero-exit
elsebranch now sets$after = $nullalongside$lastAttemptError(scripts/qualify-self-update-published.ps1:214). In all three paths — exit 0, non-zero exit, and thrown exception/timeout —$afterand$lastAttemptErrornow describe the same latest attempt, so the finallast output/last errorpair can no longer mix diagnostics across attempts. Gate semantics unchanged: the loop still requires an exit-0 probe reporting the exact target version.
Fresh review of the current PR
Re-checked the complete diff against the implementation rather than the one-line delta:
- Contract still matches
internal/selfupdate:--applyrequires acb.exebasename (selfupdate.go:215) and an absolute--gh-executable; exact--versionselection requires the exact published tag and rejects drafts (selfupdate.go:317), so the documentedv2.0.0-rc.1candidate flow is implementable; the harness'sValidatePatternaccepts the same canonical-with-prerelease form. versionis a bootstrap command returning before registry/policy load (main.go:416, proven bymain_test.go), output formatcontainer-bin Xmatches both assertions, and the transaction itself already runs a bounded smokeversioncheck (apply.go:410-422) — the outer 2-minute poll is a correct belt-and-suspenders layer.- Shim reconciliation coverage holds:
discoverManagedShims(apply.go:273) acceptsos.SameFilehardlinks and size+digest-identical regular files with valid non-reserved names;go/gofmtqualify (registry.go:923-953), staging/helper/rollback copies live in subdirectories and are ignored. - The exact survivor allowlist still covers every private-artifact class in the implementation:
.container-bin-update-*staging,.container-bin-update-helper-*,.container-bin-update-rollback-*,.{name}-update-*.tmpreplacement temps (apply.go:367), andcontainer-bin.mutation.lockresidue (mutationlock.go:26). - Token handling is sound: the verifier passes
GH_TOKEN/GITHUB_TOKENtoghthrough an env whitelist only (verify.go:653-661), and nothing in the script or output path prints it. - Timeout/kill model,
OwnedProcessRootprefix matching, post-kill drain, warn-on-failure root cleanup, and fail-closed path revalidation are unchanged from the previously verified state. git diff --checkclean; CI is fully green on the current head (format/vet/test, Windows and ARM64 builds, release-bundle build + independent reproduction, CodeQL, govulncheck, zizmor, dependency review).
Findings
No blocking or important issues. One trivial docs nit:
- 🟢 [nit]
docs/release-matrix.md— the harness description sentence ends with a colon ("...updates that private installation to an exact canonical published release:") but is followed by a prose paragraph rather than the content the colon introduces; the second colon correctly leads into the command block. Ending the first sentence with a period would read correctly.
Summary
The last-round diagnostics fix is correct and complete; re-verification of the full harness against the transaction code, the published-asset contract, and the docs found nothing else of substance. CI is green. Good to merge.
|
Addressed the final trivial docs nit in 241c8c5: the harness-description sentence now ends with a period rather than a colon before the following prose paragraph. Revalidated go test -race ./..., go vet ./..., and git diff --check. The commit is intentionally unsigned because signing is disabled. |


Summary
Validation
scripts/qualify-self-update-published.ps1 -FromVersion v1.0.0 -TargetVersion v1.1.0 ...— PASS against canonical published v1.1.0 assets; two managed shims reconciled; zero private artifactsgo test -race ./...go vet ./...v2.0.0-citestcb.exe versionsmoke testgit diff --checkThe commit is intentionally unsigned because repository commit signing is currently disabled. No runtime behavior changes are included.