Skip to content

Commit d9f7431

Browse files
authored
docs: realign branch-protection sources of truth with live protection (#163)
* docs: realign branch-protection sources of truth with live protection PR #161 made `contract schema (L2)`, `curl|bash smoke`, and `old-cli compat` run on pull_request and added them to branch protection on main, but left the in-repo files describing the previous three-check world. Live protection requires six contexts; the repo claimed three. The maintainer confirmed the six-check state is intended, so the in-repo files catch up to protection rather than the reverse. Live protection is unchanged by this commit. - .github/required-checks.txt: add the three missing contexts, so the file matches `.required_status_checks.contexts` exactly. - docs/MERGE_POLICY.md: drop the pre-merge/post-merge split, which no longer describes anything real — every job in test.yml fires on push, pull_request, and both dispatch events with no job-level `if:`, and vm-e2e on push and pull_request. "Why these three" becomes "Why these six". Adds the external-state trade-off these required checks carry, and notes the drift sensor's blind spot: it compares the file against workflow job names only, never against live protection, which is how this drift went unnoticed. - docs/HARNESS.md: the curl|bash smoke row said "push to main / dispatch" and the L2 contract row said "CI"; both now say "every PR". * docs: correct CI claims found by review fact-check A fact-check of the previous commit against the actual workflows found three wrong claims in the new text. All verified by reading the files. - `vm-e2e` does not run on push to `main`. vm-e2e-spike.yml scopes its push trigger to the `test/vm-e2e-speed` spike branch, so the claim "all six run on every PR and again on push to main" was false. It is PR-only, now stated as such. - `curl|bash smoke` never executes `scripts/install.sh`. The job curls `localhost:18888/testuser/test-config/install`, which mock-server.py serves as a synthetic stub that execs the freshly built binary in dry-run mode; no workflow references scripts/install.sh at all. The real installer is covered in L1 by install_script_test.go, which pipes it through `/bin/bash -s`. Both the table row (pre-existing error) and the new "no Go test exercises it end to end" bullet were wrong. - The network-dependency section said "three" but named two, and claimed a GitHub API blip or yanked asset blocks all PRs. `old-cli compat` ends its lookup with `|| true` and gates every later step on a non-empty version, so it passes green having tested nothing. Rewritten to name the real asymmetry: L2 blocks, cli-compat fails open. Also corrects "fails on PRs" for the drift sensor, which is continue-on-error, and notes its second blind spot: it only checks that listed checks have jobs, never that required contexts are listed.
1 parent b47d976 commit d9f7431

3 files changed

Lines changed: 58 additions & 25 deletions

File tree

.github/required-checks.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,7 @@
1010

1111
lint
1212
unit (L1)
13+
contract schema (L2)
14+
curl|bash smoke
15+
old-cli compat
1316
vm-e2e

docs/HARNESS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,12 @@ Three regulation categories:
4949
| Arch. | `fmtprint` — UI output via `ui.*` helpers, not raw `fmt.Print*` | L1 | `internal/archtest/fmtprint_test.go` |
5050
| Arch. | `install.sh` must not prompt on stdin — under `curl \| bash` stdin is the script | L1 | `internal/archtest/installsh_test.go` |
5151
| Behav. | L1 unit + integration + contract (faked runners *and* real brew/git/npm in temp dirs) | pre-push, CI | `make test-unit` |
52-
| Behav. | L2 contract schema (against openboot-contract repo) | CI | `.github/workflows/test.yml` `contract` job |
52+
| Behav. | L2 contract schema (against openboot-contract repo) | every PR | `.github/workflows/test.yml` `contract` job |
5353
| Behav. | L3 e2e binary | release | `make test-e2e` |
5454
| Behav. | L4 VM e2e (`vm`) — required full destructive suite on a clean macOS host | every PR | `.github/workflows/vm-e2e-spike.yml` (`vm-e2e` required check on a macos-14 runner) |
5555
| Behav. | `install.sh` upgrade over an existing install — tap refresh, upgrade, reinstall fallback, and resolved-version reporting with fake Homebrew/OpenBoot commands | L1 | `test/integration/install_script_test.go` |
5656
| Behav. | Install-wizard TUI on a real pty — L3: launch/quit smoke + full keyboard choreography (stops before confirm, installs nothing); L4: same key sequence through a real install via `expect(1)`, asserting brew/git system state | L3 at release, L4 every PR | `test/e2e/install_wizard_e2e_test.go`, `test/e2e/install_wizard_vm_test.go` |
57-
| Behav. | curl\|bash smoke (install.sh + mock server) | push to main / dispatch | `.github/workflows/test.yml` `curl-bash-smoke` job |
57+
| Behav. | curl\|bash smoke — mock-served install script piped into `bash`, driving the built binary through a config install (**not** `scripts/install.sh`; that's the L1 row above) | every PR | `.github/workflows/test.yml` `curl-bash-smoke` job |
5858
| Behav. | Auto-release sensor — patch fast lane (`fix:`-only) auto-tags + dispatches `release.yml`; feat threshold opens a `release-ready` issue (check L4 CI green, then tag manually) | push to `main` | `.github/workflows/auto-release.yml` |
5959
| Behav. | Release notes — Conventional Commits since previous tag, grouped by type (Features / Bug Fixes / etc) + Full Changelog link, appended to the install-instructions template | tag push or `workflow_dispatch` | `.github/workflows/release.yml` (`Write release notes` step) |
6060
| Behav. | Old-CLI compat (previous release × current mock server) | every PR | `.github/workflows/test.yml` `cli-compat` job |

docs/MERGE_POLICY.md

Lines changed: 53 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,32 +9,26 @@ branch protection rules configured at:
99
If the two ever drift, **GitHub is authoritative** — file a PR against
1010
this doc to bring it back in sync.
1111

12-
## CI stages
12+
## Required checks (block merge)
1313

14-
CI is split into two stages to keep PR feedback fast.
14+
All six run on every PR. There is no post-merge-only tier: every job in
15+
`.github/workflows/test.yml` fires on `push` (to `main`/`master`),
16+
`pull_request`, `repository_dispatch` (`contract-updated`), and
17+
`workflow_dispatch` alike, with no job-level `if:` gating.
1518

16-
### Pre-merge (required — blocks merge)
17-
18-
Runs on every PR. Must pass before merge.
19+
`vm-e2e` is the exception in the other direction — it is **PR-only**. Its
20+
workflow's `push` trigger is scoped to the `test/vm-e2e-speed` spike
21+
branch, so it does not run on `main` at all.
1922

2023
| Check | Workflow | Why required |
2124
|---|---|---|
2225
| `lint` | Test | Catches gofmt / gosec / staticcheck issues that block release builds. |
2326
| `unit (L1)` | Test | Unit + integration + contract: faked-runner Go tests *and* real `brew` / `git` / `npm` against temp dirs. Includes `internal/archtest` fitness rules. |
27+
| `contract schema (L2)` | Test | Validates remote-config / snapshot JSON against the `openboot-contract` schemas, and asserts the CLI decoders consume the canonical fixtures losslessly. |
28+
| `curl\|bash smoke` | Test | Builds the binary, starts `scripts/mock-server.py`, and pipes a served install script into `bash`, driving `openboot install -s -u <slug>` end to end against a mock API. Despite the name it does **not** exercise `scripts/install.sh` — that is covered in L1 by `test/integration/install_script_test.go`. |
29+
| `old-cli compat` | Test | Runs the previous release binary against the current mock server. Catches server-side changes that would break already-shipped CLIs. |
2430
| `vm-e2e` | vm-e2e-spike | Exercises the destructive install paths and TUI choreography on a fresh Apple Silicon macOS VM. |
2531

26-
### Post-merge (runs on push to `main`)
27-
28-
Does not block merge. Catches regressions on the merged state before
29-
the auto-release sensor can tag. Runs on `workflow_dispatch` and
30-
`repository_dispatch` too.
31-
32-
| Check | Workflow | Why post-merge |
33-
|---|---|---|
34-
| `contract schema (L2)` | Test | Clones external repo + pip install — too slow for every PR. Validates remote-config / snapshot JSON against the `openboot-contract` schemas. |
35-
| `curl\|bash smoke` | Test | Builds binary + starts mock server — too slow for every PR. Confirms `scripts/install.sh` still bootstraps the CLI. |
36-
| `old-cli compat` | Test | Downloads previous release from GitHub — too slow and network-dependent for every PR. Catches server-side changes that would break already-shipped CLIs. |
37-
3832
### Not required (and why)
3933

4034
| Check | Status | Reason |
@@ -56,7 +50,7 @@ the auto-release sensor can tag. Runs on `workflow_dispatch` and
5650
add it to this list. Promote a check to required by editing this doc
5751
and updating branch protection in the same PR.
5852

59-
## Why these three
53+
## Why these six
6054

6155
Each required check covers a class of regression that has shipped to
6256
users in past commits:
@@ -65,22 +59,58 @@ users in past commits:
6559
- `unit (L1)` is the broadest behaviour check — covers both faked-runner
6660
unit logic and real-subprocess integration drift (brew flag changes,
6761
`git` exit-code shifts between macOS versions).
62+
- `contract schema (L2)` catches CLI ↔ server wire drift. Tolerant
63+
decoders like `UnmarshalRemoteConfigFlexible` will silently repair,
64+
move, or drop fields; only a canonical-fixture comparison notices.
65+
- `curl|bash smoke` is the only check that drives a real built binary
66+
through a config install against a live HTTP API, catching wiring
67+
breakage that faked-runner tests structurally cannot reach.
68+
- `old-cli compat` catches server-side changes that break CLIs already
69+
on users' machines — the one regression class the current binary's
70+
own tests structurally cannot see.
6871
- `vm-e2e` (L4) covers the destructive and terminal-dependent paths that
6972
cannot safely run inside L1, including real Homebrew installs and the
7073
install-wizard choreography on a fresh macOS VM.
7174

72-
The three heavier checks (`contract schema (L2)`, `curl|bash smoke`,
73-
`old-cli compat`) still run on every merge to `main` — they just don't
74-
block PRs, because they're too slow or network-dependent to require on
75-
every push to a feature branch.
75+
### The cost of requiring the two network-dependent checks
76+
77+
`contract schema (L2)` checks out `openboot-contract@main` and
78+
`old-cli compat` downloads the previous GitHub release, so requiring
79+
both ties every merge to state outside this repo. They fail in opposite
80+
directions, and the difference is worth knowing before you trust either
81+
badge:
82+
83+
- `contract schema (L2)` **blocks**. A red or mid-edit `openboot-contract`
84+
main stops every PR, including ones that touch neither the contract nor
85+
the decoders. That is the deliberate trade: tolerant decoders like
86+
`UnmarshalRemoteConfigFlexible` hide wire drift from every other check,
87+
so the alternative to a blocked PR is a user bug report.
88+
- `old-cli compat` **passes silently**. Its release lookup ends in
89+
`|| true` and writes a possibly-empty `version=`, and every step after
90+
it is gated on `steps.prev.outputs.version != ''`. A GitHub API blip,
91+
or no stable release still carrying the arch asset, yields a green
92+
check that ran no compat test at all. It cannot block a PR — but a
93+
green tick is therefore not evidence that compat was verified.
94+
95+
If external flakiness does start blocking unrelated work, the escape
96+
hatch is the documented bypass under *Operating principles*, not quietly
97+
dropping the contexts from protection while this doc still lists them —
98+
that is the exact drift this file exists to prevent.
7699

77100
## How to change this policy
78101

79102
The required-checks list has an in-repo source of truth:
80103
[`.github/required-checks.txt`](../.github/required-checks.txt). The
81104
`required-checks alignment (drift)` sensor in
82105
[`.github/workflows/harness.yml`](../.github/workflows/harness.yml)
83-
fails on PRs that desync it from the workflow `name:` values.
106+
flags PRs that desync it from the workflow `name:` values. It is
107+
`continue-on-error: true`, so it annotates rather than blocks.
108+
109+
It also has two blind spots. It never reads live branch protection, so a
110+
context added or removed in the GitHub UI alone drifts silently; and it
111+
only checks one direction — every line in the file must map to a job,
112+
but a required context missing from the file is not flagged. Step 3
113+
below is the only thing that catches either.
84114

85115
1. Open a PR that edits this file **and** `.github/required-checks.txt`
86116
with the proposed change.

0 commit comments

Comments
 (0)