Skip to content

ci: require L4 and cover installer upgrades - #159

Merged
fullstackjam merged 2 commits into
mainfrom
codex/complete-harness-required-check
Aug 1, 2026
Merged

ci: require L4 and cover installer upgrades#159
fullstackjam merged 2 commits into
mainfrom
codex/complete-harness-required-check

Conversation

@fullstackjam

Copy link
Copy Markdown
Member

What does this PR do?

Promotes the L4 VM suite to a required merge check and adds an L1 behavioural test for install.sh upgrading an existing installation.

Why?

Closes the two remaining gaps called out by docs/HARNESS.md: destructive VM coverage did not block merges, and the real Homebrew upgrade-over-existing branch had no behavioural test.

Testing

  • go vet ./... passes
  • Relevant tests added or updated
  • go test ./internal/...
  • go test -race -v -run TestIntegration_InstallScript_ExistingInstallUpgrade ./test/integration
  • Required-check names validated against workflow job names

Cross-repo checklist

  • Does this need a docs/content update in openboot.dev? — No.
  • Does this change the CLI ↔ server API contract? — No.

Notes for reviewer

  • Live main branch protection has already been updated and verified to require vm-e2e from GitHub Actions while preserving the existing strict checks.
  • The release-only L4 job now has a distinct display name so it cannot collide with the PR check context.

@github-actions github-actions Bot added tests Tests only ci CI/CD changes docs labels Aug 1, 2026
@fullstackjam

Copy link
Copy Markdown
Member Author

@claude please review this PR — focus on correctness, design, test coverage for the install.sh upgrade branches, branch-protection/check-name risk, and rollback.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude finished @fullstackjam's task in 3m 42s —— View job


Review: ci: require L4 and cover installer upgrades

Todo list

  • Gather context (read diff, changed files)
  • Review .github/required-checks.txt and workflow changes for check-name correctness
  • Review test/integration/install_script_test.go for upgrade-branch coverage
  • Review docs updates (docs/HARNESS.md, docs/MERGE_POLICY.md, CONTRIBUTING.md)
  • Assess rollback risk of making L4 a required check
  • Post final review

Overall this is a clean, well-scoped change — small diff, docs kept in sync with the actual gate, and the new test closes a real gap (the "already installed" branch of install.sh previously had zero behavioural coverage; curl-bash-smoke in test.yml only exercises dry-run against a mock server and is itself if: github.event_name != 'pull_request', so it never ran the real brew-upgrade path at all).

Correctness / design

  • test/integration/install_script_test.go feeds scripts/install.sh through bash -s -- with fake uname/xcode-select/brew/openboot on PATH, which correctly avoids touching real Homebrew or network. I traced the fake scripts against scripts/install.sh:184-207 by hand: brew list openboot → already-installed branch → brew update (swallowed via || true in the real script) → brew upgrade, then brew reinstall only on upgrade failure. Both subtests' wantCalls match this exactly, and the openboot install --help tail call correctly reflects that $1 (--help) is forwarded via exec openboot install "$@" (install.sh:222).
  • The job rename in release.yml:68 (name: release vm-e2e (L4)) is a good, deliberate fix — without it, release.yml's vm-e2e job id would produce the same GitHub check context (vm-e2e) as vm-e2e-spike.yml's job, and branch protection matches by name only. Since release.yml only triggers on push: tags / workflow_dispatch (never pull_request), the collision is currently latent rather than live, but renaming pre-emptively is the right call and matches the PR's own stated reasoning.
  • .github/required-checks.txt's new vm-e2e line matches vm-e2e-spike.yml's job id (no name: override there, so GitHub uses the id) — confirmed against the required-checks alignment (drift) sensor logic in harness.yml:129-155, which reads job.get('name', job_id) per job. That sensor is itself continue-on-error: true, so it can only warn, not enforce — correctness here rests on this PR getting the name right, which it does.
  • Docs (HARNESS.md, MERGE_POLICY.md, CONTRIBUTING.md) are consistent with each other and with what test.yml/vm-e2e-spike.yml actually do (verified curl-bash-smoke, contract, cli-compat are all if: github.event_name != 'pull_request', matching the corrected "push to main / dispatch" wording — this also fixes a pre-existing doc inaccuracy that said "every PR").

Test coverage gaps (minor)

  • Both subtests cover: upgrade succeeds, and upgrade fails → reinstall succeeds. There's no case for reinstall also failing — under set -euo pipefail the script would just abort with the raw brew reinstall error, which is probably fine behaviorally, but it's untested. Worth a follow-up subtest with BREW_UPGRADE_EXIT=1 + a BREW_REINSTALL_EXIT that fails, asserting the script exits non-zero and surfaces the brew error rather than silently continuing.
  • No coverage for brew list openboot itself failing for a reason other than "not installed" (e.g. brew missing entirely) — likely out of scope here since that's the fresh-install branch, not the upgrade branch this PR targets.

Branch-protection / check-name risk

  • The one real risk is sequencing, not the diff itself: per the PR notes, live branch protection on main was already updated to require vm-e2e before this PR (which documents that same requirement) merges. That means every other currently-open PR started needing a real macOS-14 VM run to pass as soon as protection changed, independent of this PR landing. If vm-e2e-spike.yml turns out flaky under load (destructive real-Homebrew installs on shared macOS runners), that blocks unrelated merges immediately, not just future ones touching installer code.
  • docs/MERGE_POLICY.md's own process (step 2) allows updating branch protection via UI or pasting the reproducible gh api command in the PR description "so the reviewer can reproduce it." This PR took the UI path and only asserts it was "verified," without the command or its output — a reviewer can't independently confirm the live contexts match required-checks.txt from the PR alone. Not a blocker, but pasting gh api repos/openbootdotdev/openboot/branches/main/protection --jq '.required_status_checks.contexts' output would close that gap per the doc's own step 3.

Rollback

  • Already covered symmetrically by existing policy: reverting is "edit required-checks.txt to drop the line + gh api -X PUT .../protection" (docs/MERGE_POLICY.md "How to change this policy"), and MERGE_POLICY.md's "Required ≠ blocking forever" gives an audited admin-override escape hatch if vm-e2e breaks upstream (e.g. GitHub Actions macOS outage) before someone reverts the required-check config properly. No new rollback mechanism was needed and none was invented — good, avoids gold-plating.

Note: I couldn't execute go vet/go test in this sandbox (command approval wasn't available), so the above is from static tracing of the script and test fakes rather than a live run — the PR description's own reported go test -race pass for the new test is consistent with what I traced by hand.

@fullstackjam

Copy link
Copy Markdown
Member Author

Live branch-protection verification (performed before merge):\n\n- strict: true\n- required contexts: lint, unit (L1), curl|bash smoke, old-cli compat, contract schema (L2), vm-e2e\n- vm-e2e app_id: 15368 (GitHub Actions)\n\nThe five pre-existing contexts and strict mode were preserved; this change only appended vm-e2e and bound it to GitHub Actions.

@fullstackjam
fullstackjam merged commit 4de8cd8 into main Aug 1, 2026
14 checks passed
@fullstackjam
fullstackjam deleted the codex/complete-harness-required-check branch August 1, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI/CD changes docs tests Tests only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant