Skip to content

Add read-only self-update release check - #76

Merged
AviBackToBlack merged 6 commits into
mainfrom
codex/add-self-update-check
Sep 24, 2026
Merged

AviBackToBlack merged 6 commits into
mainfrom
codex/add-self-update-check

Conversation

@AviBackToBlack

Copy link
Copy Markdown
Owner

Summary

  • add cb self-update --check as the read-only selection/planning slice of RM-31
  • select stable, prerelease, or exact releases with explicit downgrade authorization
  • validate canonical GitHub release metadata, exact Windows/amd64 assets, response/asset bounds, and the future checksum + provenance policy
  • keep the command bootstrap-safe and document its no-download/no-write boundary

This intentionally does not download, verify, stage, or replace installed files. Those remain separate roadmap slices, so RM-31 is not complete.

Validation

  • go test -race ./...
  • go vet ./...
  • release-style Windows/amd64 build with main.version=v1.1.0
  • live stable check against v1.1.0 (CURRENT)
  • live explicit v1.0.0 downgrade rejection (exit 120)
  • live explicit authorized v1.0.0 downgrade plan
  • signed commit verified locally

@CherylSnowVeil CherylSnowVeil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: cb self-update --check read-only release selection

Reviewed the full diff at fb7e8b9 (internal/selfupdate, main.go dispatch, tests, docs). The implementation faithfully matches the RM-31 selection-slice contract in docs/roadmap-implementation-requirements.md: canonical-repo-only HTTPS queries, bounded responses, exact asset names/URLs/sizes with duplicate rejection, no downloads, no file writes, and fail-closed handling for dev builds, unsupported platforms, drafts, and unauthorized downgrades. The required-asset contract (cb.exe, container-bin-<tag>-windows-amd64.zip, SHA256SUMS) matches what .github/workflows/release.yml actually publishes, and X-GitHub-Api-Version: 2026-03-10 is a real supported API version. Dispatch before registry I/O is correctly limited to management invocations — shim invocations still fall through to tool dispatch. Test coverage is strong, including the no-network-before-local-rejection check.

Findings

🟢 [suggestion] internal/selfupdate/selfupdate.go — --prerelease listing can outgrow the 1 MiB response bound

selectRelease requests releases?per_page=100 while getJSON caps bodies at maxResponseSize (1 MiB). GitHub release objects are heavy — each embeds the author object, per-asset uploader objects, URL fields, and the release-notes body, typically ~8–15 KB each. 100 entries land at roughly 0.8–1.5 MiB, i.e. right around the cap. Once the repository accumulates enough releases with notes, cb self-update --check --prerelease will permanently fail with "GitHub release response exceeded the 1 MiB limit" instead of degrading gracefully. Suggest a smaller page (e.g. per_page=30) or paginating until the byte bound, so the limit protects against abuse without becoming a time bomb. Not urgent today with few releases, but cheap to fix now.

🟢 [nit] internal/selfupdate/selfupdate.go ParseArgs — --version consumes the next token even when it is another flag

cb self-update --check --version --prerelease treats --prerelease as the version value, so the error is "version "--prerelease" must be canonical…" rather than "--version requires a value", and --prerelease is silently swallowed. The command still fails correctly, so this is purely diagnostic polish — optionally reject a following token that starts with - or validate immediately.

Overall

No blocking or important issues found. The read-only boundary is enforced honestly (no download, no writes, plan-only output), the security posture is fail-closed throughout, and docs/README/usage are consistently updated per the repo's documentation rules. LGTM with the two optional notes above.

@AviBackToBlack

Copy link
Copy Markdown
Owner Author

Addressed both review notes in signed commit b63e02d: prerelease discovery now requests the 30 most recent releases (and documentation/tests match), reducing pressure on the 1 MiB response cap; --version now rejects a following flag as a missing value instead of consuming it. Revalidated with go test -race ./... and go vet ./....

@CherylSnowVeil CherylSnowVeil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at b63e02d

Validation of previous findings

Both notes from the prior round are verified fixed against the current head, not just per the author's comment:

  • 🟢 per_page vs the 1 MiB bound — fixed. selectRelease now requests releases?per_page=30 (internal/selfupdate/selfupdate.go), the README documents "the 30 most recent published releases", and TestPlanPrereleaseSelectsHighestCanonicalPublishedCandidate asserts the 30-entry URL. 30 release objects sit comfortably under maxResponseSize, so the bound now protects against abuse rather than becoming a time bomb.
  • 🟢 --version swallowing a following flag — fixed. ParseArgs now rejects strings.HasPrefix(args[i+1], "-"), so cb self-update --check --version --prerelease fails with "--version requires one canonical version" instead of consuming the flag as the value. New test case missing exact before flag covers it. Rejecting --prefixed values is safe since canonical versions must start with v.

Fresh review of the current head

Re-reviewed the complete diff (all 8 files) rather than only the fix commit:

  • Selection paths. Stable (releases/latest), prerelease (max canonical semver among the 30 most recent, requiring both the GitHub prerelease flag and semver prerelease identifiers, drafts excluded), and exact (releases/tags/{tag} with Draft/tag-name verification) all fail closed. The exact-tag URL is safe: parseVersion restricts the tag to [v0-9A-Za-z.-] so no query/path injection is possible.
  • Fail-closed ordering. Plan rejects dev builds (dev or a dev prerelease identifier, case-insensitive) and non-windows/amd64 platforms before any network call, and the test proves the transport is never invoked.
  • Metadata validation. Exact html_url, browser_download_url, asset names (cb.exe, container-bin-<tag>-windows-amd64.zip, SHA256SUMS — matching release.yml), size bounds, and duplicate rejection are all enforced. Redirects are refused (ErrUseLastResponse), responses are bounded at 1 MiB post-decompression, and the pinned X-GitHub-Api-Version: 2026-03-10 is accepted by the live API (verified: HTTP 200).
  • Dispatch. self-update runs before registry I/O only for management invocations (cb/container-bin/cb-vX…), so a tool shim still receives self-update as a regular argv — the no-registry-load property is covered by TestSelfUpdateCheckDoesNotLoadRegistry. Unauthenticated-only queries keep the command bootstrap-safe and stdlib-only.
  • Semver. compare implements numeric-vs-alphanumeric precedence, leading-zero rejection, and arbitrary-length numeric identifiers correctly; the ordered test vector is solid.
  • Docs. README, docs/security-model.md, and docs/architecture.md (including the selfupdate leaf and the statearchive edge, which matches the real import graph) are consistent with the implementation.

New findings

🟢 [nit] internal/selfupdate/selfupdate.go getJSON — error messages don't identify the endpoint. cb self-update --check --version v9.9.9 reports only "GitHub release query returned HTTP 404" with no indication of which release/endpoint was queried. The message is interpretable in context, but including the endpoint or tag would make "no such release" self-explanatory. Purely diagnostic polish; not blocking.

Overall

Both previous findings are resolved correctly, CI is green across Linux/Windows test, vet, CodeQL, zizmor, govulncheck, and the release-bundle reproduction. No new blocking or important issues found — LGTM.

Copilot AI lite review requested due to automatic review settings September 23, 2026 18:19
@AviBackToBlack

Copy link
Copy Markdown
Owner Author

Addressed the fresh-review diagnostic nit in signed commit a882668. getJSON now includes the canonical request URI in network and non-200 errors, so an exact lookup reports the requested /repos/AviBackToBlack/container-bin/releases/tags/<tag> endpoint instead of an anonymous HTTP status. The label comes from req.URL.RequestURI(), so it carries no credentials or mutable external host data. The HTTP regression test asserts the endpoint-bearing error. Validation: go test -race ./..., go vet ./..., and git diff --check pass.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Devin Review

Comment thread internal/selfupdate/selfupdate.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Address the two moderate selector-validation issues involving prerelease tags and empty explicit versions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds read-only cb self-update --check support for selecting and validating Windows/amd64 releases without downloading or modifying files.

Changes:

  • Adds stable, prerelease, exact-version, and downgrade selection.
  • Validates release metadata, assets, URLs, and size limits.
  • Documents bootstrap behavior and security boundaries.
File Summary
README.md Documents the self-update check command.
main.go Adds bootstrap-safe command dispatch.
main_test.go Tests registry-independent dispatch.
internal/​selfupdate/​version.go Parses and compares versions.
internal/​selfupdate/​selfupdate.go Implements release selection and planning.
internal/​selfupdate/​selfupdate_test.go Tests selection, validation, and bounds.
docs/​security-model.md Documents fail-closed update selection.
docs/​architecture.md Documents package and dispatch architecture.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/selfupdate/selfupdate.go Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 18:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

An empty --version value can fall back to stable selection instead of being rejected.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject empty --version values instead of falling back to stable

internal/​selfupdate/​selfupdate.go:95

An empty argument is accepted here because strings.HasPrefix("", "-") is false. Thus cb self-update --check --version "" silently falls back to stable selection (and can also bypass duplicate --version detection) instead of rejecting a non-canonical exact version. Reject an empty value before assigning it.

Copilot AI review requested due to automatic review settings September 23, 2026 19:04
@AviBackToBlack

Copy link
Copy Markdown
Owner Author

Addressed the newly reported empty --version edge case in signed commit 7ad0e70. ParseArgs now rejects an empty exact-version operand before assignment, preventing fallback to stable selection and preserving duplicate-option detection. A regression test was added; the full race suite and vet pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The update-selection and release-validation changes warrant final human review.

Review effort: Lite
Findings: None

@AviBackToBlack
AviBackToBlack enabled auto-merge (squash) September 23, 2026 23:50
…-check

# Conflicts:
#	docs/architecture.md
#	main.go
Copilot AI review requested due to automatic review settings September 24, 2026 00:00
@AviBackToBlack
AviBackToBlack merged commit 126793b into main Sep 24, 2026
16 checks passed
@AviBackToBlack
AviBackToBlack deleted the codex/add-self-update-check branch September 24, 2026 00:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Release tag validation must reject non-canonical tags that the self-update command cannot select.

Review effort: Lite
Findings: None

AviBackToBlack added a commit that referenced this pull request Sep 25, 2026
## Summary

- add machine-policy schema 2 with mandatory detached Ed25519
authentication for the exact `container-bin.toml` bytes
- define strict signer identities, canonical raw public keys,
activation/expiry windows, explicit revocation, and overlapping key
rotation
- verify the bounded regular-file `container-bin.toml.sig` envelope
before registry parsing, built-in fallback, backup recovery, shim
reconciliation, or Docker work
- require every production registry load and reload to pass the active
policy authenticator
- fail closed on missing, malformed, unauthorized, revoked, inactive, or
mismatched signatures with stable policy codes
- keep signed registry bytes read-only to `cb` mutation commands while
allowing `setup` / `install` to reconcile shims without creating or
upgrading the registry
- preserve and re-authenticate detached signatures in backups;
authenticate signed restore previews before parsing while leaving signed
apply to administrator provisioning
- document the schema, exact signing message, envelope, key lifecycle,
recovery boundary, diagnostics, and downgrade behavior

This starts from current `main` after the enterprise-policy foundation
merged in #76. It is independent of open project-overlay, self-update,
and WSL work.

## Validation

- `gofmt` / `git diff --check`
- `go vet ./...`
- `go test -race ./...`
- `python -m unittest -v internal/registry/pipx_wrapper_test.py
internal/cli/pipx_discovery_test.py`
- `go list -deps ./...`
- Windows/amd64 release-style cross-build with injected `v0.0.0-citest`
- native Windows/amd64 execution of the complete `internal/policy` test
binary

Roadmap: #2 (registry-signature enterprise policy). The roadmap remains
authoritative and this item is not complete until the PR is merged.
<!-- devin-review-badge-begin -->

---

<a
href="https://app.devin.ai/review/avibacktoblack/container-bin/pull/84"
target="_blank"><picture><source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img
src="https://static.devin.ai/assets/gh-devin-review-light.svg?v=4"
alt="Devin Review"></picture></a>
<!-- devin-review-badge-end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants