diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fa2248e0..51014e332 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,11 +9,14 @@ on: - "tests/**" - "scripts/**" - "gui/**" + - "assets/**" - ".gitattributes" - ".npmignore" - "package.json" - "bun.lock" - "tsconfig.json" + - "README.md" + - "LICENSE" - ".github/workflows/ci.yml" - ".github/workflows/release.yml" - ".github/workflows/enforce-pr-target.yml" @@ -26,11 +29,14 @@ on: - "tests/**" - "scripts/**" - "gui/**" + - "assets/**" - ".gitattributes" - ".npmignore" - "package.json" - "bun.lock" - "tsconfig.json" + - "README.md" + - "LICENSE" - ".github/workflows/ci.yml" - ".github/workflows/release.yml" - ".github/workflows/enforce-pr-target.yml" @@ -111,56 +117,151 @@ jobs: echo 'label=windows-latest' >> "$GITHUB_OUTPUT" fi + # Which areas this push actually touches. + # + # Deliberately a job-level filter rather than a wider workflow-level `paths:` + # one. A workflow skipped by path filtering leaves its checks Pending forever, + # so a PR requiring them can never merge; a skipped *job* reports success. + # That asymmetry is the whole reason this job exists instead of more entries + # in the `on:` block above. + changes: + name: changes + runs-on: ubuntu-latest + timeout-minutes: 5 + # The workflow grants only `contents: read`, and specifying any permission + # sets every unspecified one to `none`. paths-filter reads the PR's file list + # through the API on `pull_request`, so without this it fails outright — and + # a failed filter produces empty outputs, which every `== 'true'` condition + # below would read as "nothing changed, skip". + permissions: + contents: read + pull-requests: read + outputs: + gui: ${{ steps.filter.outputs.gui }} + packaging: ${{ steps.filter.outputs.packaging }} + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false + + - name: Detect changed areas + id: filter + uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 + with: + # Without this the action compares against the repository's DEFAULT + # branch, which is `main`. A push to `dev` would then be diffed against + # `main`, so every area touched since the last promotion keeps reading + # as "changed" — the scoped jobs would run on nearly every dev push and + # the saving would silently not happen while CI stayed green. + # + # On `pull_request` the action ignores this and uses the PR's own file + # list. On a branch push it means "compare against the previous commit + # on this branch", which is the intent. + base: ${{ github.ref }} + filters: | + gui: + - 'gui/**' + # Everything that ends up inside `npm pack`, or that decides what + # does. `src/**` belongs here because package.json ships `src` and + # bin/ocx.mjs executes it: without that entry an ordinary source PR + # would get no Windows verification at all, since the Windows suite + # now runs only at the shipping boundary. + packaging: + - 'package.json' + - 'bun.lock' + - 'src/**' + - 'bin/**' + - 'gui/**' + - 'assets/**' + - '.npmignore' + # `.gitattributes` decides how tracked package inputs are + # materialized on each runner, so an attribute change can put CRLF + # shebangs into the tarball without any source file moving. + - '.gitattributes' + - 'README.md' + - 'LICENSE' + - 'scripts/prepare-package.ts' + + # The suite, split by file across four Linux runners. + # + # `bun test --shard=i/N` sorts test files by path and deals them round-robin, + # so the split is deterministic: shard 3 of 4 covers the same files on every + # run, and the four shards together cover the suite exactly once. Measured on + # this suite: 120 files per shard, union 480, no file in two shards. + # + # Only the suite lives here. Typecheck, lint, build, and the scans run once in + # `gates` rather than four times — they are fixed cost, and paying it per shard + # would eat what the sharding saves. test: - name: ${{ matrix.name }} - needs: select-windows-runner - runs-on: ${{ matrix.runner }} - # Windows dominates this matrix: on run 30459554635 the same suite took - # ubuntu 4.6min / macos 5.6min / windows 11.8min. Against the previous - # 12-minute ceiling that left ~12s of headroom, so runner variance decided - # the result rather than the code under review — #711's rerun finished at - # 11.8min and passed while #653's was killed at 12.0min (issue #717). - # A cancelled job renders as `fail` in `gh pr checks`, so that flakiness - # reads as a broken PR. After the 2026-08-01 state-store admission merge, - # Windows under `bun test --isolate` on #827 hit the 20-minute kill while - # still green mid-suite (~19m of tests). 30 minutes is the margin for that - # tip — not a licence to absorb hung tests (see oauth mutation waitMs - # unref fix). Shrink the suite / close the platform gap rather than raising - # this again for ordinary variance. - timeout-minutes: 30 + name: test ${{ matrix.shard }}/4 + runs-on: ubuntu-latest + # A quarter of the suite. A shard that needs longer than this is wedged, not + # slow — the old 30-minute ceiling was margin for the Windows leg, which no + # longer runs here (see platform-windows). + timeout-minutes: 15 strategy: fail-fast: false matrix: - # `name` is spelled out rather than derived from `runner`: a runner given - # as a label array renders as "self-hosted Windows X64 ocx-home", so the - # check name would change with the routing and break any branch - # protection rule that names it. Keeping `windows` fixed means the - # required check stays the same whichever machine served it. - include: - - name: ubuntu - runner: ubuntu-latest - - name: windows - runner: ${{ fromJSON(needs.select-windows-runner.outputs.runner) }} - - name: macos - runner: macos-latest + shard: [1, 2, 3, 4] steps: - - name: Show selected runner - if: matrix.name == 'windows' - shell: bash - run: echo "windows leg on ${{ needs.select-windows-runner.outputs.label }}" + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false - # A self-hosted runner keeps its working directory between jobs. Without an - # explicit wipe, a file deleted in the commit under test survives on disk - # and the suite passes against a tree that no longer exists in git. - # `--ephemeral` registration de-registers the runner after each job but does - # not clean the workspace, so this step is what makes the checkout honest. - - name: Clean workspace (self-hosted only) - if: runner.environment == 'self-hosted' - shell: bash - run: git clean -xffd . || true + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + # The GUI install is NOT optional here, however unrelated it looks to a + # test shard. Several files under tests/ import JSX-bearing modules from + # gui/src (ProviderRail and friends), and React is declared only in + # gui/package.json. Without this the affected shards die on + # `Cannot find module 'react/jsx-dev-runtime'` while the other shards pass. + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + # Nor is the build. Tests that fetch the served dashboard read the session + # bootstrap meta tags out of `gui/dist/index.html`, so without a build the + # server has no index to serve and those assertions see an empty string. + # The old three-platform job happened to satisfy this because every leg ran + # the GUI build as part of the same job; splitting the suite away from the + # gates removed that coincidence, so the dependency has to be explicit. + - name: Build GUI + run: | + cd gui + bun run build + + - name: Test + run: bun test --isolate tests --shard=${{ matrix.shard }}/4 + # Everything that is not the suite: type safety, privacy, lint, build, smoke. + # One runner, once per push. Splitting these across the shards would repeat a + # fixed couple of minutes four times to save nothing. + gates: + name: gates + needs: changes + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false - name: Setup Bun uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 @@ -176,9 +277,6 @@ jobs: - name: Typecheck run: bun x tsc --noEmit - - name: Test - run: bun test --isolate tests - - name: GUI tests run: cd gui && bun test tests @@ -189,20 +287,155 @@ jobs: run: bun build scripts/release.ts --target=bun --outdir=.tmp/ci-release-script-check - name: GUI lint + if: needs.changes.outputs.gui == 'true' run: | cd gui bun run lint - name: GUI build + if: needs.changes.outputs.gui == 'true' + run: | + cd gui + bun run build + + - name: CLI help smoke + run: bun run src/cli/index.ts help + + # macOS runs on every pull request, and runs the WHOLE suite unsharded. + # + # That is the point of it. The four Linux shards each cover a quarter of the + # files, which quietly assumes no test depends on a sibling file having run in + # the same process pool. This leg is the control that would notice if that + # assumption ever broke. It is also the cheapest leg on the board — 5m23s on + # the baseline run, faster than the ubuntu leg it sits beside — so there was + # never a latency argument for touching it. + # + # It does not repeat the gates: typecheck, privacy, lint, and build are + # platform-independent and already ran once above. + platform-macos: + name: macos + runs-on: macos-latest + timeout-minutes: 30 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false + + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + # Same reason as the shards: the suite serves gui/dist and reads it back. + - name: Build GUI + run: | + cd gui + bun run build + + - name: Test + run: bun test --isolate tests + + - name: CLI help smoke + run: bun run src/cli/index.ts help + + # Windows runs when something is about to ship — promotion to main/preview, or + # an explicit dispatch — not on every pull request. + # + # It was 16m23s against a 6-minute Linux critical path on the baseline run, so + # it alone decided when a PR turned green. The history in this file records the + # ceiling being raised twice rather than the gap being closed (#711 vs #653, + # issue #717, then #827's 20-minute kill). The coverage is not dropped: it + # moves to the boundary where a maintainer acts on it, and release.yml already + # refuses to publish a commit without a successful run of this workflow. + # + # The trade is real and worth naming: a Windows-only regression can now reach + # `dev` and be caught at promotion instead of at review time. `dev` is not + # published; main and preview are, and both still gate on this job. + # + # The condition is spelled out positively rather than as `!= 'pull_request'`, + # which would also match every push to dev — most of the traffic this change + # exists to speed up. + platform-windows: + name: windows + needs: select-windows-runner + if: >- + github.event_name == 'workflow_dispatch' || + (github.event_name == 'push' && + (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/preview')) + runs-on: ${{ fromJSON(needs.select-windows-runner.outputs.runner) }} + # After the 2026-08-01 state-store admission merge, Windows under + # `bun test --isolate` on #827 hit the 20-minute kill while still green + # mid-suite (~19m of tests). 30 minutes is the margin for that tip — not a + # licence to absorb hung tests (see the oauth mutation waitMs unref fix). + timeout-minutes: 30 + steps: + - name: Show selected runner + shell: bash + run: echo "windows leg on ${{ needs.select-windows-runner.outputs.label }}" + + # A self-hosted runner keeps its working directory between jobs. Without an + # explicit wipe, a file deleted in the commit under test survives on disk + # and the suite passes against a tree that no longer exists in git. + # `--ephemeral` registration de-registers the runner after each job but does + # not clean the workspace, so this step is what makes the checkout honest. + - name: Clean workspace (self-hosted only) + if: runner.environment == 'self-hosted' + shell: bash + # `|| true` used to swallow this, which defeats the point: a clean that + # fails on permissions leaves the deleted files in place and the checkout + # below then validates a tree that no longer exists in git. Only the + # not-a-repository case is tolerated — that is the first run on a fresh + # box, where there is nothing to clean. + run: | + if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then + git clean -xffd . + fi + + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false + + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + # Same reason as the shards: the suite serves gui/dist and reads it back. + - name: Build GUI run: | cd gui bun run build + - name: Test + run: bun test --isolate tests + - name: CLI help smoke run: bun run src/cli/index.ts help npm-global-smoke: name: npm-global ${{ matrix.os }} + needs: changes + if: needs.changes.outputs.packaging == 'true' runs-on: ${{ matrix.os }} timeout-minutes: 8 strategy: @@ -216,6 +449,11 @@ jobs: steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + # No job here pushes, and the self-hosted box keeps its checkout + # between jobs, so leaving a usable token in .git/config is avoidable + # residue. Matches the convention already used by the other workflows. + persist-credentials: false # Deliberately NO setup-bun: prove `npm install -g` works without a # separately-installed Bun. The launcher uses the bundled `bun` dependency. @@ -243,3 +481,76 @@ jobs: - name: ocx help via bundled bun run: ocx help + + # The one check name that means "CI passed". + # + # Shard names move whenever the shard count changes, and the platform legs come + # and go by trigger. Neither is a stable thing to require in branch protection. + # This job is: it depends on every other job and asserts each result. `dev` has + # no branch protection configured today, so nothing has to be re-pointed — but + # whoever enables it has one obvious check to require. + # + # NOTE for that day: requiring this check also means dropping the + # workflow-level `paths:` filter above, or moving this job to an + # always-triggered workflow. A PR that touches only docs does not trigger this + # workflow at all, so no `ci` check would be created and the PR would sit + # pending forever. That is harmless while nothing is required and a trap + # afterwards. + # + # `if: always()` is load-bearing. Without it, a failed or skipped dependency + # skips this job too — and GitHub reports a skipped job as success, so the gate + # would go green precisely when something went wrong. + ci: + name: ci + if: always() + # Every producer, including the ones that only feed other jobs. `needs` holds + # direct dependencies only, so a failing `select-windows-runner` would + # otherwise reach this gate as nothing at all while its dependents report + # `skipped` — which the gate is required to read as a deliberate skip. + needs: [changes, select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Assert every needed job succeeded or was skipped + shell: bash + env: + RESULTS: ${{ toJSON(needs) }} + run: | + set -euo pipefail + echo "$RESULTS" | jq . + # Allowlist, not denylist. Anything that is not a known-good result + # fails the gate, so a result GitHub adds later cannot pass silently. + # + # `skipped` is a pass on purpose: that is how a trigger-scoped job + # (platform-windows on a pull request) reports when it is deliberately + # not run. + bad=$(echo "$RESULTS" | jq -r ' + to_entries + | map(select(.value.result != "success" and .value.result != "skipped")) + | .[] | "\(.key)=\(.value.result)"') + if [ -n "$bad" ]; then + echo "::error::needed job(s) did not pass: $bad" + exit 1 + fi + + # `skipped` passing is right for a job that is SUPPOSED to be skipped, and + # wrong everywhere else. The windows leg is the whole reason this workflow + # can be trusted at the shipping boundary, so on the events that ARE that + # boundary it has to have actually run. Without this, a one-character slip + # in its `if:` would silently skip it, the gate would go green, and the + # release helper — which only checks that this workflow succeeded — would + # publish a tip no Windows machine ever executed. + - name: Assert the windows leg ran where it is mandatory + if: >- + github.event_name == 'workflow_dispatch' || + (github.event_name == 'push' && + (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/preview')) + shell: bash + env: + WINDOWS_RESULT: ${{ needs.platform-windows.result }} + run: | + set -euo pipefail + if [ "$WINDOWS_RESULT" != "success" ]; then + echo "::error::the windows leg is mandatory on ${GITHUB_REF#refs/heads/} / workflow_dispatch but reported '${WINDOWS_RESULT}'. A skipped windows leg must never reach the release gate." + exit 1 + fi diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d9767d931..225b9e94a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -138,17 +138,27 @@ jobs: fi ci_url="$( + # shellcheck disable=SC2016 + # `$branch` is a jq variable bound by --arg, not a shell variable, so + # the filter must stay single-quoted. gh run list \ --workflow ci.yml \ --commit "$GITHUB_SHA" \ --status success \ --limit 10 \ - --json conclusion,headSha,url,workflowName \ - --jq '.[0].url // ""' + --json conclusion,headSha,url,workflowName,event,headBranch \ + --jq --arg branch "${GITHUB_REF#refs/heads/}" \ + 'map(select(.event == "push" and .headBranch == $branch)) | .[0].url // ""' )" if [ -z "$ci_url" ]; then - echo "::error::No successful Cross-platform CI run found for ${GITHUB_SHA}. Wait for CI to pass before releasing." + # Deliberately narrower than "any successful ci.yml run for this SHA". + # The Windows suite runs on promotion pushes to main/preview and on + # dispatch, not on pull requests — so a green PR run for the same SHA + # proves Linux and macOS and says nothing about Windows. Accepting it + # here would let a publish proceed while the promotion run that + # actually carries Windows was still pending, or had failed. + echo "::error::No successful Cross-platform CI run found for ${GITHUB_SHA} on ${GITHUB_REF#refs/heads/} (push event). A pull-request run does not qualify: it skips the Windows leg. Wait for the promotion run to pass before releasing." gh run list --workflow ci.yml --commit "$GITHUB_SHA" --limit 10 || true exit 1 fi diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/000_plan.md b/devlog/_plan/260803_ci_dev_lane_sharding/000_plan.md new file mode 100644 index 000000000..a0c9d6c72 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/000_plan.md @@ -0,0 +1,230 @@ +# dev CI lane: shard on Linux, move Windows off the PR path + +Opened 2026-08-03 against `dev@f9b9440c551e3d7f3e2041098caa2ee4de57698e` (v2.10.0). + +## The problem this unit solves + +`.github/workflows/ci.yml` runs one `test` job across a three-OS matrix, and +every leg runs the *whole* gate: install, typecheck, 480 test files under +`bun test --isolate`, GUI tests, privacy scan, release-helper build, GUI lint, +GUI build, CLI smoke. The three legs are not equal. + +Measured on run `30748690567` (PR #880, `feat/cline-pass-provider`, all green): + +| leg | wall clock | +|---|---| +| ubuntu | 5m 49s | +| macos | 5m 23s | +| windows | **16m 23s** | + +The Windows leg is roughly 3x the other two and it alone decides when a PR +turns green. That is not a new observation — the `timeout-minutes: 30` comment +in `ci.yml` records the history: a 12-minute ceiling once let runner variance +decide review outcomes (#711 passed at 11.8min, #653 was killed at 12.0min, +issue #717), and the ceiling has been raised twice since rather than the gap +being closed. + +Two consequences fall out of that: + +1. **Feedback latency.** A contributor waits ~16 minutes for a verdict that + Linux produced at minute 6. +2. **Concurrency.** Standard runners are free on public repositories, so the + scarce resource is not money — it is the 20 concurrent jobs the Free plan + allows, and the 5-job macOS cap. With ~20 pull requests open, six jobs per + push (3 test legs + 3 npm-global legs) saturates that budget and PRs queue + behind each other. + +Corrected after audit: a run is **seven** jobs, not six — `select-windows-runner` +is one of them. + +Jobs *created* per run is not the same as jobs *running at once*: the platform +legs wait on the selector, `gates` and the packaging smoke wait on `changes`, +and the gate waits on everything. Peak simultaneous runners, derived from the +dependency graph rather than by summing rows: + +| | jobs per run | peak simultaneous | +|---|---|---| +| before | 7 | 6 (3 suite + 3 npm-global, after the selector) | +| after — non-packaging PR (tests/workflows/docs) | 9 | 7 | +| after — source or packaging PR | 12 | up to 10 | +| after — promotion | 13 | up to 10 | + +"Peak" is the graph's maximum antichain, not the sum of the rows: `changes` and +`select-windows-runner` start alongside the four shards and macOS, while +`gates` and the packaging smoke wait on `changes`, and the gate waits on +everything. The upper bounds assume worst-case overlap; real overlap depends on +job durations. + +Note which row an ordinary `src/**` PR lands in: **the packaging row**, because +`src` ships inside the npm tarball and is therefore a packaging input. So the +common case trades more concurrent short jobs for a much shorter critical path. +That is the right trade against a 20-job budget, but it is a trade, and calling +the change strictly cheaper would be false. + +## What this unit changes + +The dev/PR lane becomes a Linux lane. Windows verification does not disappear — +it moves to where a maintainer actually consumes it: promotion to `main` and +`preview`, plus `workflow_dispatch` on demand. macOS stays on the PR lane and +keeps running the **whole suite, unsharded** — it is the control that would +notice if the four Linux shards ever stopped being independent. What it drops +is the platform-independent work it used to repeat: typecheck, privacy scan, +GUI lint and build, release-helper syntax. Those now run once, in `gates`. + +Stated as an invariant: **every platform that ships is still proven before it +ships; only the moment of proof moves.** A PR is proven on Linux, a promotion +is proven on every platform. + +## Constraints discovered before planning + +### `tests/ci-workflows.test.ts` is the real specification + +This is not a workflow edit with a test that happens to cover it. The suite +pins the workflow's shape deliberately, and its comments say why each pin +exists — usually because an audit round deleted that exact thing and the suite +stayed green. Any restructure must move these pins forward *intentionally*: + +- `ci.jobs["select-windows-runner"]["timeout-minutes"] === 2` +- `ci.jobs.test["timeout-minutes"] === 30` +- `ci.jobs["npm-global-smoke"]["timeout-minutes"] === 8` +- `count(workflow, "timeout-minutes:") === 3` — an exact count, so adding a job + without updating this test fails the suite +- `workflow` contains `bun test --isolate tests` +- pinned action SHAs for checkout / setup-bun / setup-node, and no `@vN` refs +- `pull_request.branches` sorted equals `["dev", "main"]` +- `Object.keys(pull_request)` sorted equals `["branches", "paths"]` +- `push.branches` sorted equals `["dev", "main", "preview"]` +- an exact 14-entry path list, asserted identical for `push` and `pull_request` +- `- name: GUI lint`, `bun run lint`, `- name: GUI build`, `bun run build` + +The path-list pin has its own history: "Round 16 dropped `src/**`, `tests/**`, +and both workflow self-references one at a time and the suite stayed green each +time." So the list is asserted element-by-element on purpose. + +**Design consequence:** the test file is edited in the same phase as the +workflow it pins, and every pin change is justified in the diff rather than +relaxed. A pin that becomes meaningless (an exact `timeout-minutes:` count +across a matrix that now has more jobs) is *replaced by a stronger pin*, not +deleted. + +### Path filtering must not sit at workflow level + +GitHub's documentation is explicit: "If a workflow is skipped due to path +filtering, branch filtering, or a commit message, then checks associated with +that workflow will remain in a 'Pending' state. A pull request that requires +those checks to be successful will be blocked from merging." +([workflow syntax](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax)) + +A *job* skipped by an `if:` condition behaves in the opposite way: "A job that +is skipped will report its status as 'Success'. It will not prevent a pull +request from merging, even if it is a required check." +([job conditions](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-jobs-with-conditions)) + +So affected-path scoping belongs on jobs, never on the workflow trigger. The +existing `on.pull_request.paths` list stays as a coarse "is this workflow +relevant at all" filter — it is already there and already pinned — and the new +per-area scoping happens inside the run. + +### Required-check names are exact strings + +"If you use branch protection rules that require specific status checks, make +sure that job names are unique across all workflows." +([protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches)) + +A sharded matrix produces `test (1)`, `test (2)`, … — names that change +whenever the shard count changes. The answer is a single aggregate job whose +name never moves, which `needs:` every real job and asserts their results. +Branch protection then names one stable check. + +Right now `dev` has **no** branch protection configured (`GET +/repos/.../branches/dev/protection` returns 404 "Branch not protected"), which +matches `AGENTS.md`: approval and CI requirements are "enforced by convention +until branch protection is configured". That makes this the cheap moment to +introduce a stable gate name — nothing has to be re-pointed today, and whoever +enables protection later has one obvious check to require. + +**Known coupling, recorded rather than fixed here.** The workflow keeps its +`on.pull_request.paths` filter (widened to 17 entries in phase 2), which means +a PR touching only `docs-site/**` or `devlog/**` does not trigger it at all — and +therefore creates no `ci` check. That is harmless today because nothing is +required. The moment `ci` becomes a required check, it stops being harmless: +a docs-only PR would sit pending forever. + +Whoever enables branch protection must therefore also either drop the +workflow-level `paths:` filter or move the gate into an always-triggered +workflow. This unit does not do it now because removing that pinned 14-entry +list is a separate decision with its own blast radius, and bundling it into a +CI-speed change is the drive-by scope expansion this repository's guidance +warns against. It is written down here and in the PR description so it cannot +be discovered the hard way. + +### Bun's sharding semantics, verified locally + +Bun 1.3.13 added `--shard=i/n`. From the release notes: "Test files are sorted +by path for determinism and distributed round-robin across shards, keeping each +shard balanced to within one file of each other. The shard index is 1-based." +([Bun v1.3.13](https://bun.com/blog/bun-v1.3.13), 2026-04-20) + +Verified on this tree with Bun 1.3.14 rather than taken on trust — see +`001_shard_evidence.md`. Four shards over the real suite give 120 files each, +union 480, zero overlap, zero loss. + +### The suite serializes itself locally, and that is local-only + +`scripts/test.ts` waits for other `bun test --isolate` runners on the same +machine before starting, because parallel worktrees on one developer box turned +a 210s suite into 13 minutes. That queue is keyed on `pgrep` of the local +machine; separate CI runners never see each other, so sharding does not +interact with it. CI already calls `bun test --isolate tests` directly, not the +wrapper script. + +## Phase map + +Dependency-ordered: each phase consumes the verified output of the previous one. + +| phase | doc | delivers | +|---|---|---| +| 1 | `010_linux_shard_matrix.md` + `011_platform_legs.md` | one matrix job becomes four Linux shards, a `gates` job, two platform jobs, and the aggregate `ci` check | +| 2 | `030_affected_scoping.md` | per-job change detection; packaging and GUI work skips when its area is untouched | +| 3 | `040_ship_and_review.md` | local gates, push, PR, live timing evidence, security + bot review | + +Phase 1 is deliberately **one commit covering two documents**. An earlier draft +split "shard Linux" and "move the platforms" into separate phases; that is not +implementable, because Windows and macOS are `include:` entries of the very +`test` job the sharding replaces. Splitting them would have deleted two +platforms in a commit whose own description claimed they were untouched. The +documents stay separate for readability; the delivery does not. + +Phase 2 follows because it only adds conditions to jobs phase 1 defines. Phase +3 is the only phase that touches the remote. + +## Scope boundary + +**IN:** `.github/workflows/ci.yml`, `tests/ci-workflows.test.ts`, this devlog +unit, and — only if sharding demands it — `package.json` scripts. + +**OUT:** `src/`, `gui/` source, release publishing, provider adapters, any +credential surface, `enforce-pr-target.yml` semantics. The self-hosted Windows +runner routing keeps its security reasoning verbatim: those comments explain +that the routing is a *cost* control and not a security boundary, and that the +fork-approval policy is what actually protects the box. Nothing here weakens +that. + +## Accept criteria + +1. `actionlint` exits 0 on every touched workflow. +2. Shards partition the suite: per-shard counts sum to the discovered file + count with no file in two shards. +3. `bun run typecheck` exits 0; the full suite passes, including the workflow + pins in `tests/ci-workflows.test.ts`. +4. The aggregate gate fails when any needed job fails, and succeeds when a + needed job is skipped by area filtering. Both directions are asserted, not + assumed — a gate that cannot fail is worse than no gate. +5. Live run evidence after push: dev-lane wall clock compared against the + 16m 23s baseline. +6. No local path, worktree name, or machine name appears in any pushed commit, + workflow, or PR body. +7. Every shard runs green with the GUI dependencies installed — specifically + the shard containing the JSX-importing tests, not an arbitrary one. +8. Windows still runs on `workflow_dispatch` and on promotion, proven by an + actual dispatch run rather than by reading the condition. diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/001_shard_evidence.md b/devlog/_plan/260803_ci_dev_lane_sharding/001_shard_evidence.md new file mode 100644 index 000000000..b7fea9c1d --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/001_shard_evidence.md @@ -0,0 +1,157 @@ +# Evidence: shard semantics, runner economics, required-check behavior + +Collected 2026-08-03. Every external claim below was opened at its primary +source; every local claim was run on this tree at +`dev@f9b9440c551e3d7f3e2041098caa2ee4de57698e` with Bun 1.3.14. + +## 1. Baseline timings (measured, not estimated) + +`gh run view 30748690567` — PR #880, all jobs `success`: + +| job | started | completed | wall | +|---|---|---|---| +| ubuntu | 12:51:19Z | 12:57:08Z | 5m 49s | +| macos | 12:51:19Z | 12:56:42Z | 5m 23s | +| windows | 12:51:19Z | 13:07:42Z | **16m 23s** | +| npm-global ubuntu | 12:51:07Z | 12:51:44Z | 0m 37s | +| npm-global macos | 12:51:06Z | 12:51:36Z | 0m 30s | +| npm-global windows | 12:51:06Z | 12:52:59Z | 1m 53s | +| select windows runner | 12:51:13Z | 12:51:17Z | 0m 4s | + +Total run wall clock is set by the Windows test leg: **16m 23s**, against a +Linux critical path of 5m 49s. This run used GitHub-hosted `windows-latest` +(the self-hosted route only applies to `push`/`workflow_dispatch`). + +## 2. Bun `--shard` semantics + +Primary source: [Bun v1.3.13 release notes](https://bun.com/blog/bun-v1.3.13), +2026-04-20. + +> "Test files are sorted by path for determinism and distributed round-robin +> across shards, keeping each shard balanced to within one file of each other. +> The shard index is 1-based (`1 <= index <= count`)." + +> "Workers automatically run with `--isolate` between files." + +Sharding is file-level, not test-level, and it composes with `--isolate` +without requiring it. + +### Verified locally + +A 10-file scratch directory under `tests/` (bunfig pins discovery to `./tests`, +so scratch files elsewhere are invisible to the runner — worth knowing before +trusting a shard experiment run outside that root): + +``` +shard 1/3: f01 f04 f07 f10 +shard 2/3: f02 f05 f08 +shard 3/3: f03 f06 f09 +``` + +Round-robin over path-sorted files, exactly as documented, disjoint and +complete. + +Against the real suite (480 discovered `*.test.ts` files): + +``` +shard 1/4: 120 files +shard 2/4: 120 files +shard 3/4: 120 files +shard 4/4: 120 files +union: 480 total, 480 unique +find tests -name '*.test.ts' | wc -l => 480 +``` + +No file appears twice; no file is lost. Four shards divide this suite exactly. + +### Shard count choice + +Four. The suite is 480 files over a ~5m 49s Linux leg, of which install + +typecheck + GUI build are fixed overhead that every shard repeats. Splitting +the *test* step four ways trims the variable part while paying that overhead +four times; beyond four the overhead dominates and the concurrency budget +(20 jobs, Free plan) starts to matter more than the wall clock saved. Four is +also the number that divides 480 evenly, which keeps the shards balanced +exactly rather than to within one file. + +## 3. Runner economics + +Primary sources: [Actions billing](https://docs.github.com/en/billing/concepts/product-billing/github-actions), +[runner pricing](https://docs.github.com/en/billing/reference/actions-runner-pricing), +[Actions limits](https://docs.github.com/en/actions/reference/limits), +[hosted runners reference](https://docs.github.com/en/actions/reference/runners/github-hosted-runners). + +> "GitHub Actions usage is free for self-hosted runners and for public +> repositories that use standard GitHub-hosted runners." + +This repository is public, so **minutes are not billed** and the 1x/2x/10x +multipliers do not apply as a cost argument. The honest justification for this +unit is latency and concurrency, not money. Stating it the other way round +would be a nicer story and a false one. + +The binding limits instead: + +> "Standard GitHub-hosted runner | Free | 20 | 5." + +20 concurrent jobs, of which at most 5 may be macOS. Current shape spends 6 +jobs per push (3 test legs + 3 npm-global legs); with ~20 open pull requests +that budget is the queue. + +Runner sizes, which explain the Windows gap only partially — Linux and Windows +are both 2 vCPU / 8 GB, so the 3x difference is filesystem and process-spawn +cost under `--isolate`, not a smaller machine: + +| label | vCPU | RAM | +|---|---|---| +| `ubuntu-latest` | 2 | 8 GB | +| `windows-latest` | 2 | 8 GB | +| `macos-latest` | 3 (M1) | 7 GB | + +## 4. Required checks and skipped jobs + +Primary sources: [workflow syntax](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax), +[job conditions](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-jobs-with-conditions), +[protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches). + +The two behaviors that decide the design, and they are opposites: + +> "If a workflow is skipped due to path filtering, branch filtering, or a +> commit message, then checks associated with that workflow will remain in a +> 'Pending' state. A pull request that requires those checks to be successful +> will be blocked from merging." + +> "A job that is skipped will report its status as 'Success'. It will not +> prevent a pull request from merging, even if it is a required check." + +Hence: filter **jobs**, never the workflow, for anything a required check +depends on. + +For the aggregate gate: + +> "If a job fails or is skipped, all jobs that need it are skipped unless the +> jobs use a conditional expression that causes the job to continue." + +> "If you would like a job to run even if a job it is dependent on did not +> succeed, use the `always()` conditional expression in `jobs..if`." + +So the gate needs `if: always()` or it inherits the very skipping it is +supposed to summarize. + +### Current protection state + +``` +GET /repos/lidge-jun/opencodex/branches/dev/protection +=> 404 "Branch not protected" +``` + +Nothing is required today, matching `AGENTS.md` ("enforced by convention until +branch protection is configured"). No existing required-check name can break, +which is precisely why introducing the stable gate name now costs nothing. + +## 5. Local test-runner interaction + +`scripts/test.ts` blocks a run while another `bun test --isolate` process +exists on the same machine, discovered via `pgrep`. That is a developer-box +concern (parallel worktrees fighting over one CPU turned a 210s suite into 13 +minutes). CI invokes `bun test --isolate tests` directly and each shard runs on +its own runner, so shards cannot see or serialize against each other. diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md b/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md new file mode 100644 index 000000000..950c3f2d9 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md @@ -0,0 +1,383 @@ +# Audit rounds — synthesis and dispositions + +## Round 1 + +Independent reviewer, read-only, 2026-08-03. Verdict: **FAIL**, 6 High + 2 +Medium. Every blocker was re-verified locally before being accepted; none is +taken on the reviewer's word alone. + +## Root cause across the blockers + +Three of the six High findings (1, 2, 5) are the same mistake wearing different +clothes: **the plan reasoned about each job in isolation and not about the +dependency graph the jobs form.** A gate that lists three `needs:` while five +jobs exist, a filter job with no permission to filter, a required check on a +path-filtered workflow — each is locally sensible and globally wrong. + +Blocker 3 has a different root cause and a worse one: an assumption stated as a +finding. "25 test files reference `gui/` but read source rather than importing +built artifacts" was written from a `rg` of import *paths* without running a +single one of those tests. + +## Dispositions + +| # | Sev | Finding | Disposition | +|---|---|---|---| +| 1 | High | Gate omits `changes`/`select-windows-runner`; skip-as-pass hides a failed producer | **ACCEPT** | +| 2 | High | `dorny/paths-filter` needs `pull-requests: read`; workflow grants only `contents: read` | **ACCEPT** | +| 3 | High | Dropping GUI install breaks shards — JSX tests need React | **ACCEPT** | +| 4 | High | Packaging filter omits `src/**` → src-only PRs get zero Windows signal | **ACCEPT** | +| 5 | High | PR-level `paths:` + required `ci` check = permanently pending docs-only PRs | **ACCEPT, deferred** | +| 6 | High | Windows `if:` as written also runs on `dev` pushes; `matrix` invalid in job `if:` | **ACCEPT** | +| 7 | Medium | `if: always()` parses as string, not boolean | **ACCEPT** | +| 8 | Medium | "once-only" claim false while platform legs duplicate gates; job count wrong | **ACCEPT** | + +### Verified locally, not accepted on report + +**Blocker 3** — ran the test rather than reading the import: + +``` +$ bun test tests/provider-workspace-rail.test.ts +error: Cannot find module 'react/jsx-dev-runtime' from + gui/src/components/provider-workspace/ProviderRail.tsx +0 pass, 1 fail +``` + +`ProviderRail.tsx:52` is JSX (`return (`), React lives +only in `gui/package.json`, and `gui/node_modules` is absent in a fresh +checkout. The shard job must keep the GUI install. **The plan's own +verification step would have caught this only by luck** — "run one shard +locally" would have passed on shards not containing these files. + +**Blocker 6** — ran actionlint on a minimal reproduction: + +``` +if: ${{ matrix.name != 'windows' }} +=> context "matrix" is not allowed here. available contexts are + "github", "inputs", "needs", "vars". +``` + +Confirms Option A (split jobs) is the only workable shape, as `020` suspected +but had not proven. + +**Blocker 7** — ran the parser: + +``` +$ bun -e 'console.log(JSON.stringify(Bun.YAML.parse("jobs:\n ci:\n if: always()\n")))' +{"jobs":{"ci":{"if":"always()"}}} +``` + +String, not boolean. The doc's pin was wrong. + +### The one finding the reviewer cleared + +I had flagged the gate's `grep '"result": "failure"'` as a possible silent +no-match if `toJSON` emitted compact JSON. The reviewer opened the GitHub +expressions documentation: `toJSON` "Returns a pretty-print JSON +representation", and the official `needs` example shows `"result": "failure"` +with that exact spacing. The concern was unfounded. + +It is still being changed. Depending on the whitespace of a pretty-printer for +whether CI can fail is a correct-by-coincidence design. The gate moves to `jq`, +which parses instead of pattern-matching — see below. + +### Blocker 5, and why it is accepted but deferred + +The reviewer is right that `on.pull_request.paths` and a *required* `ci` check +cannot coexist: a docs-only PR would never create the check and would hang +pending forever. + +It is deferred rather than fixed here because the conflict is not live. `dev` +has no branch protection (`404 Branch not protected`), so nothing is required +today. Removing the pinned 14-entry path list is a separate decision with its +own blast radius — those pins exist because an audit round deleted entries one +at a time and nothing went red — and bundling it into a CI-speed change would +be exactly the drive-by scope expansion the repo's own guidance warns against. + +What this unit does instead: **document the coupling at the point where it will +bite.** The PR description and `040` state that requiring `ci` in branch +protection must be accompanied by removing the workflow-level `paths:` filter, +or by moving the gate to an always-triggered workflow. Recorded as a known +follow-up, not silently left as a trap. + +## Amendments applied + +1. **`010`** — gate rewritten to `jq`-based result inspection with an explicit + allowlist (`success`/`skipped` pass, everything else fails), `needs:` covers + every producer, GUI install stays in the shard job, `if` pin corrected to + the string form. +2. **`020`** — Option A (split jobs) confirmed as the only valid shape with + actionlint evidence; Windows condition pinned to dispatch-or-main/preview + rather than not-pull-request; platform steps enumerated explicitly instead + of "unchanged"; the "eighth" arithmetic error corrected to "quarter". +3. **`030`** — `pull-requests: read` added to the `changes` job; packaging + filter widened to every input that reaches the published tarball, so + src-only PRs keep a Windows packaged-CLI signal. +4. **`000`** — job count corrected to seven; concurrency table added; + blocker-5 coupling recorded. +5. **`040`** — maintainer security review added as an explicit gate + (`MAINTAINERS.md`: GitHub Actions changes require it), alongside bot review. + +## What this round says about the plan's method + +The plan was strong on external evidence (Bun semantics, GitHub docs, measured +timings) and weak on *executing its own assumptions*. Blocker 3 was one `bun +test` away from being caught during planning. The lesson carried into the +implementation phases: **every claim of the form "this dependency is not +needed" gets run, not read.** + +--- + +## Round 2 + +Same reviewer, re-audit of the amendments. Verdict: **FAIL**, 2 High + 4 Medium + +2 Low. Round 1's blockers 2, 3, 4, 6, 7 confirmed closed; 5 confirmed +accurately deferred; 1 and 8 partially closed. + +The reviewer also confirmed the `jq` gate is now correct, having probed it +against every documented `needs.*.result` value (`success`, `failure`, +`cancelled`, `skipped`, an unknown value, and malformed JSON) — the allowlist +rejects everything it should and `set -euo pipefail` catches a jq parse +failure. `jq 1.7` ships in the current `ubuntu-latest` image. + +### Root cause: the phase boundary was fictional + +Round 1 was about the dependency graph between jobs. Round 2 is about the +dependency between *commits*, and it is the more embarrassing finding. + +Phase 1 said "Windows and macOS legs are untouched in this phase". Phase 1 also +replaced the `test` job. Those legs **are** `include:` entries of the `test` +job — so phase 1 as written deletes two platforms while asserting it does not. +The phase boundary described a state the workflow could never be in, and no +amount of care inside phase 2 could have fixed it. + +This is the same failure shape as round 1's blocker 3: a claim about the +existing code written without checking the existing code. There it was "these +tests don't need React"; here it was "these legs are separate from that job". + +### Dispositions + +| # | Sev | Finding | Disposition | +|---|---|---|---| +| 1 | High | Phase 1 cannot preserve Windows/macOS as written | **ACCEPT** — phases 1 and 2 merged into one commit (`010` + `011`) | +| 2 | High | Phase 2/3 tell the implementer not to update `ci.needs`, which the derived pin rejects | **ACCEPT** — each phase now states its complete `needs` list explicitly | +| 3 | Med | `assets/**`, `README.md`, `LICENSE` in the packaging filter but not in the workflow's own paths | **ACCEPT** — outer path list widened in the same commit, with the pinned `ciPaths` array extended | +| 4 | Med | Windows steps still deferred to "same shape as macOS" | **ACCEPT** — enumerated, including the self-hosted wipe, with pins | +| 5 | Med | `000` says macOS "stops re-running the full suite", `011` says it runs it | **ACCEPT** — `000` corrected; macOS drops the repeated gates, not the suite | +| 6 | Med | Concurrency table sums rows instead of deriving peak from the graph | **ACCEPT** — split into jobs-per-run and peak-simultaneous | +| 7 | Low | Filter pin samples 4 of 10 patterns | **ACCEPT** — whole-list comparison | +| 8 | Low | Stale claims: "eighth", `failure`/`cancelled` wording, "every push" | **ACCEPT** — all three corrected | + +### Renumbering + +Merging the platform work into phase 1 makes the old `020` a sub-document of +phase 1, so the unit renumbers: `020_platform_legs.md` → `011_platform_legs.md` +(phase 1's second document), and the affected-scoping doc keeps `030` as phase +2. Decade ranges still map to phases; phase 1 simply owns two documents. + +### What the two rounds together say + +Both rounds found the same class of defect: **a confident statement about code +that had not been executed or read at the point of the claim.** The external +research was sound throughout — Bun's shard semantics, GitHub's skip behavior, +the runner limits all held up. What failed twice was local grounding. + +That is worth recording because it is not what an audit is usually expected to +catch. The plan's weakest points were not its research; they were the sentences +that sounded too obvious to check. + +--- + +## Round 3 + +Same reviewer. Verdict: **FAIL**, 1 High + 4 Medium + 1 Low. Round 2's blockers +1, 2, 7, 8 confirmed closed; the `ci.needs` progression was independently +modelled at both commit boundaries and passes. + +### Dispositions + +| # | Sev | Finding | Disposition | +|---|---|---|---| +| 1 | High | Duplicate `steps:` key in the enumerated Windows job — fails actionlint | **ACCEPT** | +| 2 | Med | `paths-filter` with no `base:` diffs a `dev` push against `main` | **ACCEPT** | +| 3 | Med | Concurrency table puts src-only PRs in the wrong row and understates graph peak | **ACCEPT** | +| 4 | Med | The macOS contradiction in `000` was never actually removed | **ACCEPT** | +| 5 | Med | `030` says the outer path list "stays exactly as it is", then grows it | **ACCEPT** | +| 6 | Low | Renumbering left stale phase references | **ACCEPT** | + +### The two that matter + +**Blocker 1 was mine, freshly introduced.** The round-2 amendment that +enumerated the Windows steps left the original `steps:` line in place above the +new one. It is a one-character-class error that makes the workflow invalid, and +it survived because I had linted *fragments* — a job condition here, a `needs:` +list there — rather than an assembled workflow. + +Fixed, and the method fixed with it: the phase-1 verification list now requires +`actionlint` against the complete assembled workflow. Done, and it passes: + +``` +$ actionlint .github/workflows/ci.yml # full proposed shape, all 8 jobs +ACTIONLINT CLEAN +``` + +**Blocker 2 is the one no amount of re-reading my own plan would have found.** +`dorny/paths-filter` defaults `base` to the repository's default branch: + +``` +$ gh api repos/lidge-jun/opencodex --jq .default_branch +main +``` + +So a push to `dev` would be diffed against `main`, and every area touched since +the last promotion would keep reading as "changed" until `main` caught up. The +GUI and packaging jobs would run on nearly every dev push, and the per-push +saving this phase exists to produce would silently not happen — while the +workflow stayed green and looked correct. `base: ${{ github.ref }}` fixes it, +and the input is pinned in the suite. (See round 4: this fix was claimed here +one round before it actually reached the plan.) + +The action SHA was independently confirmed to be tag v3.0.2: + +``` +$ gh api repos/dorny/paths-filter/git/ref/tags/v3.0.2 --jq .object.sha +de90cc6fb38fc0963ad72b210f1f284cd68cea36 +``` + +### Gate logic, verified directly + +``` +all success/skipped -> (empty) => pass +one failure -> b=failure => fail +one cancelled -> a=cancelled => fail +unknown "neutral" -> a=neutral => fail +``` + +The allowlist rejects unknown result values rather than ignoring them, which is +the property a denylist would not have had. + +### Three rounds, one pattern + +Round 1: a claim about tests that had not been run. Round 2: a claim about job +structure that had not been read. Round 3: a claim about YAML validity that had +not been linted as a whole, and a third-party default that had not been looked +up. + +Every single blocker across three rounds was a **local, checkable fact stated +without checking it** — never a research failure, never a design disagreement. +The design survived all three rounds essentially unchanged. What kept failing +was the gap between "this is obviously true" and "I ran the thing that proves +it". + +--- + +## Round 4 + +Same reviewer. Verdict: **GO-WITH-FIXES (blockers=1)** — one Medium blocker +plus one non-blocking stale sentence. Round 3's items all confirmed closed, +including the assembled-workflow actionlint run, both `ci.needs` boundaries, +and the jq allowlist behavior. + +### The blocker + +**The `base:` fix existed only in this synthesis document, not in the plan it +described.** Round 3 recorded "`base: ${{ github.ref }}` fixes it, and the +input is now pinned in the suite" — but `030`'s `changes` job still had a bare +`with: filters:` and no test assertion. Implementing the plan verbatim would +have reproduced the exact sticky-diff bug round 3 had just identified. + +Cause: the edit meant to add `base:` to `030` did not apply, and the synthesis +was written as though it had. The same thing had already happened once in this +unit — round 3's blocker 4 was a macOS correction I believed I had made and had +not. + +Fixed: `base:` is in `030`'s job spec with the failure explained inline, and +the promised suite pin is written out. Verified by grep rather than by +recollection: + +``` +$ grep -n "base:" 030_affected_scoping.md +106: base: ${{ github.ref }} +``` + +Also fixed: `040`'s stale "7 → 9-12" concurrency wording, now consistent with +`000`'s runner-count / maximum-antichain distinction. + +### The lesson, sharpened + +Rounds 1-3 shared a pattern: a claim about the code made without running the +code. Round 4 is narrower and worse — **a claim about my own edit, made without +re-reading the file.** A patch tool reporting success proves only that it +matched context lines somewhere, not that the intended change is where you +believe it is. + +Standing rule adopted for the implementation phases: after any amendment a +later step depends on, grep for the changed token in the changed file before +writing a sentence that assumes it landed. + +--- + +## Round 5 — the first live CI run, and the review bots + +The pre-merge audit ran four rounds against the plan. This round is what +running it actually taught, which is a different thing. + +### The first sharded run went red, and it was my fault + +Shard 1/4 and macOS failed on `management-integration-routes.test.ts`: a test +that fetches the served dashboard and reads its session bootstrap out of the +meta tags. The token came back empty. + +Diagnosed rather than guessed. The test also fails on an untouched checkout of +`dev` when `gui/dist` is absent, and passes there after `bun run build` — so +the dependency is pre-existing and real. What my change removed was its +*accidental* satisfaction: the old three-platform job ran the suite and the GUI +build in the same job, so `gui/dist` always existed by the time tests ran. +Splitting the suite away from the gates broke that without anyone noticing, +and area-scoping the gates' build behind `gui/**` meant even that copy was +conditional. + +Every job that runs the root suite now builds the GUI unconditionally, and a +suite pin asserts it — driven red first. + +**This is the failure the four planning rounds could not have caught.** The +dependency was invisible in the workflow, invisible in the test file, and only +existed as a side effect of two things sharing a job. No amount of reading +finds that; running it does. + +### Review bots: 9 comments, dispositions + +| Source | Finding | Disposition | +|---|---|---| +| Codex P1 | `release.yml` accepts any successful `ci.yml` run for a SHA, so a green PR run — which skips Windows — could satisfy the publish gate | **FIXED** | +| Codex P2 | `.gitattributes` missing from the packaging filter | **FIXED** | +| CodeRabbit (Major) | `git clean -xffd . \|\| true` swallows a failed self-hosted wipe | **FIXED** | +| CodeRabbit (Minor) | `persist-credentials: false` missing on checkouts | **FIXED** | +| CodeRabbit (Minor) | Plan's shard/platform examples omit the GUI build | **FIXED** | +| CodeRabbit (Minor) | `040`'s gate sequence claims fail-fast without `set -e` | **FIXED** | +| CodeRabbit (Major) | `040`'s leak scan omits `devlog` and uses two different pattern sets | **FIXED** | +| CodeRabbit (Minor) | Devlog dates are "future-dated" (2026-08-03) | **REBUTTED** | + +**The Codex P1 was the best find of the entire review**, planning rounds +included. My change made the Windows leg conditional on the event, but +`release.yml` selects a CI run by SHA and status alone. After a promotion, the +PR run for that same commit is still there, still green, and still Windows-free +— so the publish gate could be satisfied by a run that proved nothing about +Windows, while the promotion run carrying Windows was still in flight or had +failed. That is precisely the coverage hole this unit promised not to open, and +it was outside the file I was editing. The gate now selects a `push` run on the +release branch specifically. + +**The date rebuttal.** The bot read CI timestamps in UTC (`2026-08-02T17:xx`) +and concluded the 2026-08-03 dates were in the future. The workspace runs in +Asia/Seoul, where those UTC timestamps are already the 3rd: + +``` +local: 2026-08-03 02:55 KST +utc: 2026-08-02 17:55 UTC +``` + +The dates are correct in the timezone they were written in, and the unit slug +matches them. Changing them to match a UTC reading would make the record less +accurate, not more. diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/010_linux_shard_matrix.md b/devlog/_plan/260803_ci_dev_lane_sharding/010_linux_shard_matrix.md new file mode 100644 index 000000000..ddd8ef58b --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/010_linux_shard_matrix.md @@ -0,0 +1,394 @@ +# Phase 1 — the job split: Linux shards, platform legs, aggregate gate + +Consumes `000_plan.md` and `001_shard_evidence.md`. Companion document: +`011_platform_legs.md`, which specifies the two platform jobs this phase +creates. **They are one commit**, not two phases. + +> **Amended after audit rounds 1 and 2** (`002_audit_synthesis.md`). +> Round 1: the GUI dependency install stays in the shard job (dropping it breaks +> the suite — proven, not argued), the gate parses results with `jq` instead of +> grepping pretty-printed JSON, and the `if` pin is a string. +> Round 2: the platform split merged into this phase, because splitting it was +> not implementable. + +## Why this is one commit and not two + +An earlier draft made "shard Linux" phase 1 and "move the platforms" phase 2, +on the reasoning that the aggregate gate should exist before any leg is +removed. That ordering cannot be implemented. + +The current `test` job *is* the three-platform matrix (`ci.yml:114-145`): +Ubuntu, Windows, and macOS are `include:` entries of the job phase 1 replaces. +Rewriting it into an Ubuntu shard matrix therefore deletes the Windows and +macOS legs in the same edit — while phase 1's own text claimed those legs were +"untouched in this phase". The phase boundary described a state the workflow +cannot be in. + +So the split is: **one commit turns one matrix job into five jobs** (four +shards + `gates`), **creates the two platform jobs**, and **adds the gate**. +Area scoping stays a genuinely separate phase, because it only adds conditions +to jobs that by then exist. + +The original concern still holds and is still met: the gate lands in the same +commit as the leg removal, so no window exists where a check name disappears +without a stable one replacing it. + +## File: `.github/workflows/ci.yml` — MODIFY + +### 1a. Split the single `test` job into shards plus `gates` + +The current job runs everything on all three OSes. The work divides into two +kinds: the *suite* (480 files, the expensive part, platform-independent in +practice) and the *gates* (typecheck, privacy scan, GUI lint/build, release +helper syntax, CLI smoke) which are fast and only need to run once. + +Running the gates inside every shard would multiply ~2 minutes of fixed cost by +the shard count. So: shards run the suite, and one job runs the gates. + +BEFORE (current `test` job header): + +```yaml + test: + name: ${{ matrix.name }} + needs: select-windows-runner + runs-on: ${{ matrix.runner }} + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + include: + - name: ubuntu + runner: ubuntu-latest + - name: windows + runner: ${{ fromJSON(needs.select-windows-runner.outputs.runner) }} + - name: macos + runner: macos-latest +``` + +AFTER: + +```yaml + # The suite, split by file across four Linux runners. + # + # `bun test --shard=i/N` sorts test files by path and deals them round-robin, + # so the split is deterministic: shard 3 of 4 covers the same files on every + # run, and the four shards together cover the suite exactly once. Verified on + # this suite at 120 files per shard, union 480, no overlap. + # + # Only the suite lives here. Typecheck, lint, build, and the scans run once in + # `gates` rather than four times — they are fixed cost, and paying it per shard + # would eat what the sharding saves. + test: + name: test ${{ matrix.shard }}/4 + runs-on: ubuntu-latest + timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3, 4] + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + # Each job gets its own workspace, so `gates` building the GUI does nothing + # for this one. Tests that fetch the served dashboard read their session + # bootstrap out of gui/dist/index.html, and with no build the server has no + # index to serve. The old three-platform job satisfied this by accident, + # because the suite and the GUI build shared a job. + - name: Build GUI + run: | + cd gui + bun run build + + - name: Test + run: bun test --isolate tests --shard=${{ matrix.shard }}/4 +``` + +Note `timeout-minutes: 15` rather than 30. A shard that needs longer than a +quarter hour to run a quarter of the suite is wedged, not slow. The 30-minute +ceiling existed for the Windows leg; carrying it onto a Linux shard would +re-import a limit that was always about the platform being removed. + +**The GUI install stays.** An earlier draft dropped it, reasoning that the 25 +test files touching `gui/` read source rather than built artifacts. That was an +assumption written from a grep of import paths, and it is false. Several of +those files import JSX-bearing components: + +``` +$ bun test tests/provider-workspace-rail.test.ts +error: Cannot find module 'react/jsx-dev-runtime' from + gui/src/components/provider-workspace/ProviderRail.tsx +0 pass, 1 fail +``` + +`gui/src/components/provider-workspace/ProviderRail.tsx:52` returns JSX, React +is declared only in `gui/package.json`, and a fresh checkout has no +`gui/node_modules`. Without the install, shards containing these files fail and +shards without them pass — an intermittent, shard-count-dependent failure, +which is the worst shape this could have taken. + +The cost is real: `cd gui && bun install` now runs in each of the four shards. +It is accepted rather than optimized away, because the alternative (splitting +GUI-importing tests into their own suite) is a test-layout change and belongs +in its own unit, not smuggled into a CI restructure. + +### 1b. New `gates` job — the once-only checks + +```yaml + # Everything that is not the suite: type safety, privacy, lint, build, smoke. + # One runner, once per push. Splitting these across shards would repeat a + # fixed ~2 minutes four times to save nothing. + gates: + name: gates + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + - name: Typecheck + run: bun x tsc --noEmit + + - name: GUI tests + run: cd gui && bun test tests + + - name: Privacy scan + run: bun run privacy:scan + + - name: Check release helper syntax + run: bun build scripts/release.ts --target=bun --outdir=.tmp/ci-release-script-check + + - name: GUI lint + run: | + cd gui + bun run lint + + - name: GUI build + run: | + cd gui + bun run build + + - name: CLI help smoke + run: bun run src/cli/index.ts help +``` + +### 1c. New `ci` aggregate gate + +```yaml + # The one check name that means "CI passed". + # + # Shard names change whenever the shard count changes, and platform jobs come + # and go by trigger. Neither is a stable thing to require in branch + # protection. This job is: it depends on everything and asserts each result. + # + # `if: always()` is required — without it, a skipped or failed dependency + # skips this job too, and a skipped job reports success. The gate would then + # go green precisely when something went wrong, which is worse than having no + # gate at all. + # + # `skipped` counts as a pass on purpose: that is how an area-filtered job + # reports when its paths were untouched (see phase 2). `failure` and + # `cancelled` do not. + ci: + name: ci + if: always() + # EVERY producer, including the ones that only feed other jobs. `needs` holds + # direct dependencies only — it "doesn't include implicitly dependent jobs" — + # so a failing `changes` or `select-windows-runner` would otherwise reach this + # gate as nothing at all, while the jobs downstream of it report `skipped` + # and the gate calls that a pass. + # + # PHASE-ORDERED: this list names only jobs that exist *at this phase*. + # `needs:` pointing at an undefined job is a hard workflow error — + # actionlint: `job "ci" needs job "does-not-exist" which does not exist in + # this workflow`. Phase 2 adds `changes` to this list when it creates that + # job. The derived pin below is what forces it. + needs: [select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Assert every needed job succeeded or was skipped + shell: bash + env: + RESULTS: ${{ toJSON(needs) }} + run: | + set -euo pipefail + echo "$RESULTS" | jq . + # Allowlist, not denylist: anything that is not a known-good result + # fails the gate. A denylist passes silently on any result GitHub adds + # later, and "the gate quietly stopped catching a new failure mode" is + # not a thing that should be possible here. + # + # `skipped` passes because that is how an area-filtered job (phase 2) + # and a trigger-scoped platform job (this phase) report when they are + # deliberately not run. + bad=$(echo "$RESULTS" | jq -r ' + to_entries + | map(select(.value.result != "success" and .value.result != "skipped")) + | .[] | "\(.key)=\(.value.result)"') + if [ -n "$bad" ]; then + echo "::error::needed job(s) did not pass: $bad" + exit 1 + fi +``` + +Reading results out of `toJSON(needs)` rather than naming each job means adding +a job to `needs:` automatically extends the assertion. The failure mode of the +name-by-name form is silent: someone adds a job, forgets the corresponding +`test "${{ needs.x.result }}" = success` line, and the gate stops covering it +while still looking thorough. + +An earlier draft grepped the JSON text for `'"result": "failure"'`. GitHub does +document `toJSON` as returning "a pretty-print JSON representation", so that +grep would in fact have matched — but a gate whose ability to fail depends on +the whitespace of a pretty-printer is correct by coincidence. `jq` parses the +structure instead, and `jq` is present on `ubuntu-latest`. + +## File: `tests/ci-workflows.test.ts` — MODIFY + +The existing pins describe the old shape and must move deliberately. + +### Pin 1 — per-job timeouts and their exact count + +BEFORE: + +```ts + expect(ci.jobs?.["select-windows-runner"]?.["timeout-minutes"]).toBe(2); + expect(ci.jobs?.test?.["timeout-minutes"]).toBe(30); + expect(ci.jobs?.["npm-global-smoke"]?.["timeout-minutes"]).toBe(8); + // Every job must stay bounded — an unbounded job can hang a queue for hours. + expect(count(workflow, "timeout-minutes:")).toBe(3); +``` + +AFTER: + +```ts + expect(ci.jobs?.test?.["timeout-minutes"]).toBe(15); + expect(ci.jobs?.gates?.["timeout-minutes"]).toBe(15); + expect(ci.jobs?.ci?.["timeout-minutes"]).toBe(5); + expect(ci.jobs?.["npm-global-smoke"]?.["timeout-minutes"]).toBe(8); + // Every job must stay bounded — an unbounded job can hang a queue for hours. + // Asserted structurally rather than by counting the string: a count passes + // if a new job is added and an old one loses its bound in the same edit. + for (const [name, job] of Object.entries(ci.jobs ?? {})) { + expect(`${name}:${typeof job?.["timeout-minutes"]}`).toBe(`${name}:number`); + } +``` + +The `count(...) === 3` assertion is *strengthened*, not dropped. Counting +occurrences of a string only proves three bounds exist somewhere; iterating the +parsed jobs proves every job has one, which is what the comment above it always +claimed. The `${name}:` prefix makes a failure name the offending job instead +of reporting `undefined !== number`. + +### Pin 2 — the test command + +BEFORE: + +```ts + expect(workflow).toContain("bun test --isolate tests"); +``` + +AFTER: unchanged. `bun test --isolate tests --shard=${{ matrix.shard }}/4` +still contains that substring, so the pin holds as written and keeps meaning +what it meant: the suite runs isolated. + +Add alongside it: + +```ts + // Sharding is only safe while the shards tile the suite exactly. If the + // matrix and the divisor drift apart, some files stop running and CI stays + // green — the worst available failure. Pin them to each other. + const shards = (ci.jobs?.test as { strategy?: { matrix?: { shard?: number[] } } }) + ?.strategy?.matrix?.shard ?? []; + expect(shards).toEqual([1, 2, 3, 4]); + expect(workflow).toContain(`--shard=\${{ matrix.shard }}/${shards.length}`); +``` + +That is the pin this phase actually needs. A matrix of `[1, 2, 3]` against +`/4` runs three quarters of the suite and reports success. + +### Pin 3 — the gate cannot be neutered + +New: + +```ts + // The aggregate gate is the check a human trusts. Three ways to break it + // silently: drop `if: always()` so it skips (and a skipped job reports + // success), shrink `needs:` so it stops covering a job, or let `needs` + // drift behind the job list so a new job is never gated. Pin all three. + const gate = ci.jobs?.ci as { if?: unknown; needs?: string[] } | undefined; + expect(gate?.if).toBe("always()"); + const gated = [...(gate?.needs ?? [])].sort(); + const everyOtherJob = Object.keys(ci.jobs ?? {}).filter(n => n !== "ci").sort(); + expect(gated).toEqual(everyOtherJob); +``` + +The derived comparison also solves the phase-ordering hazard. Each phase adds +jobs *and* must extend `needs:` in the same commit, because the assertion is +recomputed from whatever jobs the workflow currently defines — a phase that +adds a job without gating it fails the suite immediately, and a phase that +gates a job it has not defined fails actionlint. The two checks close from +opposite directions. + +`needs:` after each phase, stated so no phase has to infer it: + +| after | `ci.needs` | +|---|---| +| phase 1 (this doc + `011`) | `select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke` | +| phase 2 (`030`) | the above **plus `changes`** | + +`Bun.YAML.parse` leaves `if: always()` as the string `"always()"`, verified: + +``` +$ bun -e 'console.log(JSON.stringify(Bun.YAML.parse("jobs:\n ci:\n if: always()\n")))' +{"jobs":{"ci":{"if":"always()"}}} +``` + +The `everyOtherJob` comparison is the assertion that matters most in this file. +A hardcoded list rots the moment someone adds a job; deriving it from the +workflow means the suite fails the instant a job exists that the gate does not +cover. + +## Verification for this phase + +1. `actionlint .github/workflows/ci.yml` exits 0. +2. `bun test tests/ci-workflows.test.ts` passes. +3. Shard tiling re-proven after the edit: per-shard file counts sum to the + discovered total. +4. One shard executed locally end-to-end — specifically a shard containing the + JSX-importing GUI tests, since those are what an install mistake breaks. +5. `actionlint` run against the **assembled** workflow, not against fragments. + Round 3 of the audit caught a duplicate `steps:` key that fragment-level + checking could not see. + +## Out of scope here + +Area/path scoping. This phase adds no conditions based on *changed files* and +creates no `changes` job. The only conditions it introduces are the trigger +scoping on `platform-windows` and the `always()` on the gate — both about which +event is running, not about which files moved. That keeps this commit's blast +radius to "the same work, rearranged across jobs", so a bisect against a broken +dev lane has one variable rather than two. diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md b/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md new file mode 100644 index 000000000..b08790f39 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md @@ -0,0 +1,284 @@ +# Phase 1b — Windows off the PR lane, macOS as the platform control + +Companion to `010_linux_shard_matrix.md`. **Same commit, same phase** — the two +documents describe one workflow rewrite, split by topic for readability rather +than by delivery. `010` covers the shards, the `gates` job, and the aggregate +check; this one covers the two platform jobs that replace the Windows and macOS +legs of the old matrix. + +> **Amended after audit rounds 1 and 2** (`002_audit_synthesis.md`). +> Round 1: Option A is the only option (a job `if:` cannot read `matrix`, proven +> with actionlint), and the Windows condition is dispatch-or-main/preview rather +> than not-pull-request. +> Round 2: the Windows steps are actually enumerated here rather than deferred +> to "same shape as macOS", and the `ci.needs` update is stated as this phase's +> work rather than assumed done elsewhere. + +## The decision, stated plainly + +Windows stops running on `pull_request` and on `push` to `dev`. It keeps +running on `push` to `main` and `preview`, and on `workflow_dispatch`. + +macOS **stays as it is**: a full unsharded suite run on the PR lane. + +That second sentence is a deliberate reversal of an earlier draft of this unit, +which had macOS reduced to a build-and-smoke leg. Two reasons it is wrong to +reduce it: + +1. **Sharding needs a control.** Four shards each running a quarter of the + suite share an assumption: that no test depends on another test's file + having run in the same process pool. A single unsharded full-suite run is + the thing that would notice if that assumption broke. Removing Windows and + sharding Linux in the same unit leaves *no* unsharded full run — unless + macOS is it. +2. **macOS is the maintainer's own platform.** Local development happens there, + so a macOS regression is caught fastest and matters most immediately. + +macOS is also the cheapest of the three to keep in wall-clock terms: 5m 23s, +*faster* than the Linux leg it sits beside. There was never a latency argument +for touching it. The only argument was the 5-job macOS concurrency cap, and one +job per push does not threaten that. + +So the shape is: Linux answers fast and in parallel, macOS answers whole, +Windows answers before anything ships. + +## Why Windows can leave the PR lane specifically + +Not because Windows matters less — because of *when* its failures are +actionable. Windows-specific defects in this repository cluster in service +installation, path handling, and process lifetime: `service-lifecycle.yml` +already covers the first, and the rest surface at install/run time, which is +what promotion and release exercise. Meanwhile the Windows leg costs 16m 23s of +every contributor's feedback loop on changes that are, in the overwhelming +majority, platform-neutral TypeScript. + +The trade is explicit: a Windows-only regression can now land on `dev` and be +caught at promotion rather than at PR. That is a real regression in coverage +timing, and it is the price. What makes it acceptable is that `dev` is not +published — `main` and `preview` are, and both still gate on Windows before +they move. + +## File: `.github/workflows/ci.yml` — MODIFY + +### 1d. Platform jobs, trigger-scoped + +The `select-windows-runner` job and its comment block stay exactly as they are. +That comment explains that runner routing is a cost control and *not* a +security boundary, that a hostile PR can rewrite the routing, and that the +fork-approval policy is the only real protection. None of that reasoning +changes; it is copied forward verbatim. + +The conditional-matrix idea is dead on arrival. A job-level `if:` cannot read +the `matrix` context — verified rather than assumed: + +``` +$ actionlint # on a minimal repro with `if: ${{ matrix.name != 'windows' }}` +context "matrix" is not allowed here. available contexts are + "github", "inputs", "needs", "vars". +``` + +GitHub's context-availability table confirms it: `jobs..if` may use +`github`, `needs`, `vars`, and `inputs`. So the platform legs become two +ordinary jobs. + +```yaml + # macOS runs on every PR. The Linux shards each cover a quarter of the suite, + # so this is the only place the whole suite runs in one process pool. If shard + # independence ever breaks — a test that passes only because a sibling file ran + # first — this leg is what notices. + platform-macos: + name: macos + runs-on: macos-latest + timeout-minutes: 30 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + # The whole suite, unsharded and in one pool. Deliberately NOT the gates: + # typecheck, privacy scan, and lint are platform-independent and already + # run once in `gates`. Repeating them here is what made the old three-OS + # matrix pay for everything three times. + # + # The GUI *build* is the exception, and it is not optional: each job has its + # own workspace, so `gates` building the GUI does nothing for this one, and + # the root suite serves gui/dist and reads it back. + - name: Build GUI + run: | + cd gui + bun run build + + - name: Test + run: bun test --isolate tests + - name: CLI help smoke + run: bun run src/cli/index.ts help + + # Windows runs when something is about to ship — promotion to main/preview, or + # an explicit dispatch — not on every pull request. It is a 16-minute leg + # against a 6-minute Linux critical path, and it was deciding when every PR + # turned green. The coverage is not dropped, only moved to the boundary where + # a maintainer acts on it: release.yml refuses to publish a commit without a + # successful run of this workflow. + platform-windows: + name: windows + needs: select-windows-runner + if: >- + github.event_name == 'workflow_dispatch' || + (github.event_name == 'push' && + (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/preview')) + runs-on: ${{ fromJSON(needs.select-windows-runner.outputs.runner) }} + timeout-minutes: 30 + steps: + - name: Show selected runner + shell: bash + run: echo "windows leg on ${{ needs.select-windows-runner.outputs.label }}" + + # A self-hosted runner keeps its working directory between jobs. Without an + # explicit wipe, a file deleted in the commit under test survives on disk + # and the suite passes against a tree that no longer exists in git. + # `--ephemeral` registration de-registers the runner after each job but does + # not clean the workspace, so this step is what makes the checkout honest. + - name: Clean workspace (self-hosted only) + if: runner.environment == 'self-hosted' + shell: bash + run: git clean -xffd . || true + + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + + - name: Setup Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.14 + + - name: Install dependencies + run: | + bun install --frozen-lockfile + cd gui + bun install --frozen-lockfile + + - name: Build GUI + run: | + cd gui + bun run build + + - name: Test + run: bun test --isolate tests + + - name: CLI help smoke + run: bun run src/cli/index.ts help +``` + +Both the runner diagnostic and the self-hosted workspace wipe are carried over +verbatim from the current `test` job. They exist for reasons written in their +own comments, and neither has anything to do with this restructure — losing +them here would be exactly the silent deletion this unit's audit rounds keep +catching. + +The condition is spelled out positively — dispatch, or a push to `main` or +`preview` — rather than as `!= 'pull_request'`. The negative form is the bug the +audit caught: it also runs Windows on every `push` to `dev`, which is most of +the traffic this unit exists to speed up, so the change would have looked +correct and achieved close to nothing. + +The `select-windows-runner` job and its comment block are untouched. That +comment explains that runner routing is a cost control and *not* a security +boundary, that a hostile PR can rewrite the routing, and that the fork-approval +policy is the only real protection. All of it carries forward verbatim. + +### `needs:` for the gate + +Both jobs created here go into the gate's `needs:` in this same commit: + +```yaml + needs: [select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] +``` + +That is not optional bookkeeping. The suite pin derives the expected `needs` +list from the workflow's own job keys, so creating a job without gating it +fails the tests immediately. + +`platform-windows` is skipped on pull requests, and a skipped job reports +success — which is exactly why the gate's allowlist admits `skipped`. Without +that, every PR would fail its gate on the deliberately-absent Windows leg. + +### 1e. `npm-global-smoke` keeps all three OSes + +Untouched. Its own comment already explains the reasoning — it is an 8-minute +job that deliberately avoids the self-hosted box, and its Windows leg is 1m 53s. +There is nothing to win by moving it and real coverage to lose: it is the only +check that `npm install -g` works on Windows without a separate Bun. + +## File: `tests/ci-workflows.test.ts` — MODIFY + +The shard document replaced the timeout pins; this document extends them for the +adds the assertion that keeps this decision honest: + +```ts + expect(ci.jobs?.["select-windows-runner"]?.["timeout-minutes"]).toBe(2); + expect(ci.jobs?.["platform-macos"]?.["timeout-minutes"]).toBe(30); + expect(ci.jobs?.["platform-windows"]?.["timeout-minutes"]).toBe(30); +``` + +```ts + // Windows leaving the PR lane is a trade, not a deletion: it still runs + // before anything is published. Assert the positive condition, not the + // absence of `pull_request` — `!= 'pull_request'` also matches every push to + // dev, which would quietly restore the 16-minute leg to the busiest lane and + // still pass a loosely-worded test. + const windowsIf = String((ci.jobs?.["platform-windows"] as { if?: string })?.if ?? ""); + expect(windowsIf).toContain("github.event_name == 'workflow_dispatch'"); + expect(windowsIf).toContain("github.ref == 'refs/heads/main'"); + expect(windowsIf).toContain("github.ref == 'refs/heads/preview'"); + expect(windowsIf).not.toContain("refs/heads/dev"); + + // macOS is the unsharded control for the sharded Linux lane. If it stops + // running the whole suite, sharding loses the thing that would catch a + // cross-file dependency between shards. + const macosSteps = (ci.jobs?.["platform-macos"] as { steps?: { run?: string }[] })?.steps ?? []; + expect(macosSteps.some(s => s.run?.includes("bun test --isolate tests"))).toBe(true); + expect(macosSteps.some(s => s.run?.includes("--shard"))).toBe(false); + // Unconditional: a control that only sometimes runs is not a control. + expect(ci.jobs?.["platform-macos"]).not.toHaveProperty("if"); +``` + +```ts + // Windows must run the same full suite, and must keep the self-hosted + // workspace wipe. Without the wipe a deleted file survives on the runner's + // disk and the suite passes against a tree that no longer exists in git — + // the failure that step was added to prevent. + const winSteps = (ci.jobs?.["platform-windows"] as { steps?: { if?: string; run?: string }[] })?.steps ?? []; + expect(winSteps.some(s => s.run?.includes("bun test --isolate tests"))).toBe(true); + expect(winSteps.some(s => s.run?.includes("--shard"))).toBe(false); + expect(winSteps.some(s => s.if === "runner.environment == 'self-hosted'" + && s.run?.includes("git clean -xffd"))).toBe(true); +``` + +The last two assertions are the ones worth having: macOS must not be sharded +and must not be conditional, which is the entire reason it was kept. + +## Verification for this phase + +1. `actionlint` exits 0. +2. `bun test tests/ci-workflows.test.ts` passes. +3. Manual matrix expansion recorded for both events: what jobs a + `pull_request` produces, and what a `push` to `main` produces. This is the + claim that cannot be checked locally by running anything, so it is checked + by reading and written down. +4. Live confirmation in phase 3 (`040`): the PR run shows no Windows test leg, and a + `workflow_dispatch` run shows one. + +## Risk register + +| risk | mitigation | +|---|---| +| Windows-only regression lands on `dev` | Caught at promotion to `main`/`preview`; `dev` is not published | +| Someone requires the old `windows` check name in future branch protection | The stable `ci` gate is the name to require; `dev` has no protection configured today | +| Skipped Windows job read as a pass by a human skimming checks | The `ci` gate is one line and covers it; the PR description states the trade | diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/030_affected_scoping.md b/devlog/_plan/260803_ci_dev_lane_sharding/030_affected_scoping.md new file mode 100644 index 000000000..df690e269 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/030_affected_scoping.md @@ -0,0 +1,368 @@ +# Phase 2 — affected-path scoping, per job + +Consumes phase 1 (`010` + `011`), which made the lane parallel and moved the +slowest platform off it. This phase stops jobs running at all when nothing they +cover changed. It is the second and last workflow commit. + +> **Amended after audit rounds 1 and 2** (`002_audit_synthesis.md`). +> Round 1: the `changes` job gains `pull-requests: read` (without it the filter +> cannot read a PR's files at all), and the packaging filter covers `src/**` so +> an ordinary source PR keeps a Windows signal. +> Round 2: the workflow-level path list is widened in step with the packaging +> filter, the `ci.needs` update is stated as this phase's work, and the filter +> pin compares the whole pattern list instead of four samples. + +## The rule that shapes everything here + +Workflow-level `paths:` and job-level `if:` behave in opposite ways when a +required check is involved: + +- Workflow skipped by `paths:` → its checks stay **Pending** forever, and a PR + requiring them can never merge. +- Job skipped by `if:` → reports **Success**. + +So: the workflow-level `paths:` filter stays a *coarse* relevance filter and is +never used to scope a required check. All new scoping is job-level. + +That list is not frozen, though — it grows from 14 entries to 17 later in this +phase, because three packaging inputs were missing from it. What matters is +that it is edited deliberately and pinned element-by-element in the suite (its +pins exist because an audit round deleted entries one at a time and nothing +went red), not that it never changes. + +## What is worth scoping, and what is not + +Scoping has a cost: a filter job runs first, every downstream job waits on it, +and the whole thing is one more place to be wrong. It pays only where the job +being skipped is expensive and the area is genuinely separable. + +| job | scope it? | reasoning | +|---|---|---| +| `test` shards | **no** | Any `src/**` or `tests/**` change can affect any test. The correlation between changed path and affected test is not something a glob knows. | +| `gates` | **no** | Typecheck and privacy scan are whole-tree properties. ~2 minutes. | +| GUI lint / GUI build | **yes** | Only `gui/**` can break them, and they are a real chunk of the `gates` job. | +| `npm-global-smoke` | **yes** | 3 jobs of packaging proof, only meaningful when packaging inputs move: `package.json`, `bin/**`, `.npmignore`, `gui/**`, `scripts/prepare-package.ts`. | +| `platform-macos` | **no** | It is the unsharded control (phase 1). A control that only sometimes runs is not a control. | + +That table is the whole design. Two targets, both expensive, both cleanly +separable. Scoping the shards would be the obvious next idea and it is the one +to refuse: `bun test --changed` exists, but "which tests does this diff affect" +is exactly the question whose wrong answer is a green CI over untested code. + +### `npm-global-smoke` and the Windows signal + +Widening the packaging filter is an audit correction with a specific failure +behind it. The first draft filtered on `package.json`, `bun.lock`, `bin/**`, +`.npmignore`, `gui/**`, and `scripts/prepare-package.ts` — omitting `src/**`. + +But `package.json` ships `src` in its `files` array, and `bin/ocx.mjs` executes +that shipped source. So a PR touching only `src/router.ts` would have had: + +- no Windows suite (phase 1 moved it off PRs), +- no Windows service-lifecycle run (that workflow watches four specific source + paths, not `src/**`), +- and no Windows packaged-CLI smoke (filtered out by the omission). + +Zero Windows verification for the single most common kind of change in this +repository. The 1m 53s Windows smoke is the cheap signal that keeps that from +being true, and it only fires if `src/**` is in the filter. + +## File: `.github/workflows/ci.yml` — MODIFY + +### 2a. New `changes` job + +```yaml + # Which areas this push actually touches. + # + # Deliberately a job-level filter, not a workflow-level `paths:` one: a + # workflow skipped by path filtering leaves its checks Pending forever, which + # would block a PR that requires them. A skipped *job* reports success, which + # is what makes this safe. + changes: + name: changes + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + gui: ${{ steps.filter.outputs.gui }} + packaging: ${{ steps.filter.outputs.packaging }} + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + + - name: Detect changed areas + id: filter + uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 + with: + # WITHOUT THIS the action compares against the repository's DEFAULT + # branch, which is `main`. A push to `dev` would then be diffed against + # `main`, so every area touched since the last promotion keeps reading + # as "changed" — the GUI and packaging jobs would run on nearly every + # dev push until main caught up, and the per-push saving this phase + # exists to produce would silently not happen while CI stayed green. + # + # On `pull_request` the action ignores this and uses the PR's own file + # list. On a branch push it means "compare against the previous commit + # on this same branch", which is the intent. + base: ${{ github.ref }} + filters: | + gui: + - 'gui/**' + packaging: + - 'package.json' + - 'bun.lock' + - 'bin/**' + - '.npmignore' + - 'gui/**' + - 'scripts/prepare-package.ts' +``` + +corrected to cover everything that reaches the published tarball: + +```yaml + # Everything that ends up inside `npm pack`, or that decides what + # does. `src/**` belongs here because package.json ships `src` and + # bin/ocx.mjs executes it — without this entry an ordinary source PR + # gets no Windows verification at all once phase 1 moves the Windows + # suite off pull requests. + packaging: + - 'package.json' + - 'bun.lock' + - 'src/**' + - 'bin/**' + - 'gui/**' + - 'assets/**' + - '.npmignore' + - 'README.md' + - 'LICENSE' + - 'scripts/prepare-package.ts' +``` + +### The filter cannot see what the workflow never runs for + +Three of those entries — `assets/**`, `README.md`, `LICENSE` — are real tarball +inputs but do **not** appear in the workflow's own `on.*.paths` list. A PR that +changes only `README.md` never triggers this workflow, so `changes` never runs +and the packaging filter never gets the chance to fire. + +A per-job filter can only ever narrow what the workflow-level filter admits. +Widening the inner list past the outer one buys nothing and reads as coverage +that does not exist. + +So the outer list grows from 14 entries to 17, in the same commit: + +```yaml + paths: + # ... existing 14 entries ... + - "assets/**" + - "README.md" + - "LICENSE" +``` + +applied identically to `push` and `pull_request` — the suite asserts the two +lists are equal, and its comment explains why: otherwise "a change lands on +`dev` having been checked on one trigger and not the other". + +`tests/ci-workflows.test.ts`'s pinned `ciPaths` array grows by the same three +entries. That array is pinned element-by-element on purpose (an audit round +once deleted entries one at a time with the suite staying green), so extending +it is a deliberate edit, which is the intent. + +**Alternative considered and rejected:** drop the three entries from the +packaging filter instead, leaving the outer list alone. That is less code and +it is wrong — a README change genuinely does alter the published tarball, and +`npm-global-smoke` is the only check that packs it. + +This deliberately keeps `npm-global-smoke` running for most PRs. The saving is +narrower than the first draft implied — it now skips only for changes confined +to tests, workflows, or docs — and that is the honest scope. Claiming the wider +saving would have meant deleting a platform signal without saying so. + +The `changes` job also needs a permission the workflow does not currently +grant: + +```yaml + changes: + name: changes + runs-on: ubuntu-latest + timeout-minutes: 5 + # The workflow grants only `contents: read`, and specifying any permission + # sets every unspecified one to `none`. paths-filter reads the PR's file list + # through the API on `pull_request`, so without this it fails outright — and + # a failed filter means empty outputs, which reads as "nothing changed". + permissions: + contents: read + pull-requests: read +``` + +The action must be pinned to a full commit SHA: `tests/ci-workflows.test.ts` +asserts `expect(workflow).not.toMatch(/uses:\s+\S+@(?:v\d+|main|master)\b/)`, +and a mutable third-party action ref is a supply-chain hole `AGENTS.md` calls a +release blocker. The SHA above is resolved and re-verified during build rather +than trusted from this document. + +**Alternative considered:** computing the diff with `git diff --name-only` +against the merge base in a plain shell step, avoiding the third-party action +entirely. Cheaper in trust, more expensive in correctness — the merge-base +calculation differs between `push` and `pull_request`, needs `fetch-depth: 0`, +and gets subtly wrong on force-pushes. If review objects to the dependency, +this is the fallback and the tradeoff is a known one. + +### 2b. Conditioning the scoped work + +GUI steps inside `gates` become conditional rather than the whole job: + +```yaml + - name: GUI lint + if: needs.changes.outputs.gui == 'true' + run: | + cd gui + bun run lint + + - name: GUI build + if: needs.changes.outputs.gui == 'true' + run: | + cd gui + bun run build +``` + +`gates` gains `needs: changes`. Step-level rather than job-level because +`gates` also carries typecheck and the privacy scan, which always run. + +`npm-global-smoke` is skipped as a whole job: + +```yaml + npm-global-smoke: + name: npm-global ${{ matrix.os }} + needs: changes + if: needs.changes.outputs.packaging == 'true' +``` + +### 2c. Gate interaction + +The gate's *logic* needs no change: it already admits `skipped` and rejects +everything that is not `success` or `skipped`, which is precisely the behavior +this phase depends on. Worth restating because it is the load-bearing detail — +had the gate been written as `test "${{ needs.npm-global-smoke.result }}" = +success`, this phase would fail every PR that does not touch packaging. + +The gate's `needs:` **does** change, in this commit, because this phase creates +a job: + +```yaml + needs: [changes, select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] +``` + +`changes` must be gated directly. It is an upstream producer, so if it fails, +its dependents report `skipped` — which the gate reads as a deliberate skip. A +failed filter would otherwise pass the gate while silently disabling two jobs. + +## File: `tests/ci-workflows.test.ts` — MODIFY + +The GUI-gate pins from the original suite must survive in a form that still +means something. The current assertions are substring checks: + +```ts + expect(workflow).toContain("- name: GUI lint"); + expect(workflow).toContain("bun run lint"); + expect(workflow).toContain("- name: GUI build"); + expect(workflow).toContain("bun run build"); +``` + +These still pass after adding `if:` lines — which is the problem. Their comment +says they exist because "the GUI build gate was silently dropped once" (PR #97). +A gate that is present but permanently false is dropped in every sense that +matters. Strengthen: + +```ts + // PR #97 dropped the GUI build gate silently once, hence these pins. After + // area-scoping they must assert more than presence: a step conditioned on a + // filter that never fires is a dropped gate wearing the step's name. Pin + // the condition to the filter output that this phase defines. + const gateSteps = (ci.jobs?.gates as { steps?: { name?: string; if?: string; run?: string }[] })?.steps ?? []; + for (const stepName of ["GUI lint", "GUI build"]) { + const step = gateSteps.find(s => s.name === stepName); + expect(`${stepName}:${step === undefined}`).toBe(`${stepName}:false`); + expect(String(step?.if)).toBe("needs.changes.outputs.gui == 'true'"); + } +``` + +And a pin that the filter itself covers what it claims: + +```ts + // A filter with an empty or narrowed pattern list skips the job it guards + // on every run, and reports success while doing it. + const filters = String( + (ci.jobs?.changes as { steps?: { with?: { filters?: string } }[] }) + ?.steps?.find(s => s.with?.filters)?.with?.filters ?? "", + ); + expect(filters).toContain("'gui/**'"); + expect(filters).toContain("'package.json'"); + expect(filters).toContain("'bin/**'"); +``` + +Sampling four patterns is not enough: deleting `bun.lock`, `.npmignore`, +`assets/**`, `README.md`, `LICENSE`, or `scripts/prepare-package.ts` would keep +that green while quietly shrinking what gets packaging verification. Compare +the whole list: + +```ts + // Whole-list comparison, not samples. Every entry here is an input to the + // published tarball; dropping one silently stops packaging verification for + // that surface, which is the failure mode this pin exists to catch. + const packaging = filters + .split(/\n\s*packaging:\s*\n/)[1]?.split(/\n\s*\w+:\s*\n/)[0] ?? ""; + const patterns = [...packaging.matchAll(/-\s*'([^']+)'/g)].map(m => m[1]).sort(); + expect(patterns).toEqual([ + ".npmignore", + "LICENSE", + "README.md", + "assets/**", + "bin/**", + "bun.lock", + "gui/**", + "package.json", + "scripts/prepare-package.ts", + "src/**", + ]); +``` + +```ts + // `src/**` in the packaging filter is load-bearing for platform coverage, + // not a convenience: it is what keeps a source-only PR running the Windows + // packaged-CLI smoke after the Windows suite moved to promotion. + expect(filters).toContain("'src/**'"); + + // paths-filter cannot read a PR's file list without this, and a filter that + // errors produces empty outputs — which every `== 'true'` condition reads as + // "skip". The jobs would silently stop running. + expect((ci.jobs?.changes as { permissions?: Record })?.permissions) + .toEqual({ contents: "read", "pull-requests": "read" }); +``` + +```ts + // `base` is not cosmetic. Unset, paths-filter diffs a `dev` push against the + // repository default branch (`main`), so everything changed since the last + // promotion still reads as changed and the scoped jobs run anyway — the + // filter would look correct, stay green, and save nothing. Pinning it to the + // pushed ref is what makes the scoping per-push rather than per-release. + const filterStep = (ci.jobs?.changes as { steps?: { with?: Record }[] }) + ?.steps?.find(s => s.with?.filters); + expect(filterStep?.with?.base).toBe("${{ github.ref }}"); +``` + +## Verification for this phase + +1. `actionlint` exits 0. +2. `bun test tests/ci-workflows.test.ts` passes. +3. The action SHA resolves to the claimed version tag, checked against the + upstream repository rather than copied from this doc. +4. Live evidence in phase 3 (`040`): a commit touching only devlog files shows + `npm-global-smoke` skipped and the `ci` gate green — the exact combination + that would break under a naive gate. + +## What this phase deliberately does not do + +No `bun test --changed`. No per-shard test selection. No skipping the suite on +a docs-only diff. The suite is the thing CI exists to run; the savings here +come from not repeating *packaging and GUI* work that provably cannot be +affected, and stop there. diff --git a/devlog/_plan/260803_ci_dev_lane_sharding/040_ship_and_review.md b/devlog/_plan/260803_ci_dev_lane_sharding/040_ship_and_review.md new file mode 100644 index 000000000..63667f1c2 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/040_ship_and_review.md @@ -0,0 +1,175 @@ +# Phase 3 — local gates, push, PR, live evidence, review response + +Consumes phases 1 and 2. This is the only phase that touches the remote. + +> **Amended after audit round 1** (`002_audit_synthesis.md`): a maintainer +> security review is an explicit gate here, not just bot review — `MAINTAINERS.md` +> requires it for GitHub Actions changes. + +## 3a. Local gates before anything leaves the machine + +Run in this order, because each one's failure makes the next one's output +meaningless: + +```bash +set -euo pipefail +actionlint .github/workflows/ci.yml +bun x tsc --noEmit +bun test tests/ci-workflows.test.ts +bun run test +bun run privacy:scan +``` + +The full suite matters here specifically because this unit edits the file that +*tests the workflows*. A targeted run of `ci-workflows.test.ts` proves the new +pins pass; it does not prove nothing else read those workflows. + +Re-prove shard tiling against the final matrix, since the divisor is now +written in two places (matrix list and `--shard=i/N`): + +```bash +for s in 1 2 3 4; do + bun test --shard=$s/4 --test-name-pattern '____never_match____' 2>&1 \ + | grep -oE '^tests/[^:]+\.test\.ts' | sort -u +done | sort | uniq -d +``` + +Empty output means no file is in two shards. Compare the union count against +`find tests -name '*.test.ts' | wc -l` for the other direction. + +## 3b. Commit hygiene + +Commits are the public record. None of them may carry a local absolute path, a +worktree directory name, a machine name, or an internal session reference. +Check before pushing: + +```bash +# One pattern set for both checks, and the diff scan covers every path the +# branch touches — including devlog, which is exactly as public as the workflow +# files and was omitted from an earlier version of this command. +leak='/Users/|worktree|macmini|session|\.codex/' +git log origin/dev..HEAD --format='%B' | grep -nEi "$leak" || echo clean +git diff origin/dev..HEAD | grep -nEi "$leak" || echo clean +``` + +A hit is not automatically a leak — this plan legitimately contains the word +`worktree` when describing why the local test runner serializes. Read each hit +rather than trusting the exit code. + +The devlog files are tracked and public by design (`AGENTS.md`: the devlog is a +public directory in a public repository), so they are held to the same standard +as the workflow files — which is why this unit's docs cite run IDs and file +paths relative to the repository root and nothing else. + +## 3c. Push and PR + +Target `dev`. `enforce-pr-target.yml` rejects PRs whose base is not `dev`, and +also rejects "empty, thin, or malformed descriptions" — so the description is a +gate, not a courtesy. + +The description covers, in order: + +1. **What changes** — Linux shards, aggregate gate, Windows moved to + promotion/dispatch, macOS kept whole, packaging and GUI work area-scoped. +2. **The measured baseline** — run `30748690567`: ubuntu 5m 49s, macos 5m 23s, + windows 16m 23s. The Windows leg set the critical path. +3. **The honest cost** — a Windows-only regression can reach `dev` and be + caught at promotion instead of at PR. `dev` is not published; `main` and + `preview` still gate on Windows. +4. **Why not a cost argument** — standard runners are free on public + repositories. This is about feedback latency and the 20-job concurrency + budget, not minutes billed. Claiming savings that do not exist would be the + easier pitch and a false one. +5. **Required-check note** — `dev` currently has no branch protection + configured. The new stable `ci` check is the one to require if that changes; + individual shard names are not stable and are not meant to be required. + Requiring `ci` also means dropping the workflow-level `paths:` filter (or + moving the gate to an always-triggered workflow), because a docs-only PR + currently triggers no run and would therefore never create the check. +6. **Review pointers** — the two failure modes worth a reviewer's attention: + shard/divisor drift (some files silently stop running) and a gate that + cannot fail. +7. **Concurrency trade** — jobs created per run go up (7 → 9-13) and peak + simultaneous runners with them (6 → up to 10 for a source PR), while the + critical path goes down. The exact figures are in `000`. Stated plainly, + since "faster CI" usually implies "less CI" and here it does not. + +## 3c-bis. Security review is required, not optional + +`MAINTAINERS.md`: "Authentication, credential handling, GitHub Actions, release +automation, dependency installation, and other security-boundary changes +require explicit security review." + +This unit is squarely inside that: it edits GitHub Actions workflows and adds a +third-party action. The PR therefore requests maintainer security review +explicitly rather than treating green bots as sufficient. Points to raise for +that review: + +- `dorny/paths-filter` is a new third-party dependency, SHA-pinned; the + no-dependency fallback is documented in `030`. +- The `changes` job takes `pull-requests: read`, a permission the workflow did + not previously hold anywhere. It is scoped to that one job. +- `select-windows-runner` and its security commentary are unchanged; the + self-hosted routing is not touched by this unit. +- No job gains `contents: write`, and no secret is newly exposed. + +## 3d. Live evidence + +After the PR run completes: + +```bash +gh run list --workflow ci.yml --branch --limit 1 +gh run view --json jobs -q '.jobs[] | "\(.name) \(.conclusion) \(.startedAt) \(.completedAt)"' +``` + +What the evidence must show, stated before it is collected so it cannot be +read favourably after the fact: + +- Four `test i/4` jobs, all success. +- No Windows test leg on the PR event. +- `platform-macos` present and green. +- `ci` gate green. +- Critical path materially under the 16m 23s baseline. If it is not, the + restructure failed its own premise and the D summary says so rather than + finding a flattering subset of the numbers. +- `npm-global-smoke` present (this PR touches `.github/**` and `tests/**`, so + packaging is untouched — expect it **skipped**, and expect the gate green + anyway; that combination is the phase-3 proof). + +Then a `workflow_dispatch` run to prove the Windows leg still exists and still +passes. Without that, "coverage moved rather than dropped" is an assertion, not +a fact. + +## 3e. Review-bot response + +Codex and CodeRabbit both review this repository. Each comment gets one of two +dispositions, recorded: + +- **Fixed** — a follow-up commit, named. +- **Rebutted** — with the reason, in English, on the thread. + +Neither bot is authoritative about intent. Where a suggestion conflicts with a +decision recorded in `000_plan.md` or `011_platform_legs.md`, the answer cites +that reasoning instead of silently complying. Where a bot finds something those +documents got wrong, the document is updated too — a devlog that only records +the decisions that survived is a worse record than none. + +Likely objections, anticipated: + +| objection | response | +|---|---| +| "Dropping Windows from PRs reduces coverage" | Correct, and stated. Windows still runs on promotion and dispatch, and the Windows `npm-global-smoke` leg still runs on any PR touching a packaged input — which includes `src/**`. | +| "`dorny/paths-filter` is a third-party dependency" | Pinned to a full SHA; fallback to a plain `git diff` step is documented in `030`. | +| "The gate treats skipped as success" | Deliberate and required — an area-filtered job reports skipped, and GitHub reports skipped jobs as success anyway. | +| "Shard count is magic" | 480 files divide evenly by 4; the matrix and divisor are pinned to each other in the suite. | +| "A docs-only PR will hang on a required `ci` check" | True if `ci` is ever required while the workflow keeps `paths:`. Documented in `000` and in the PR body as a prerequisite for enabling protection. | +| "Peak concurrency went up" | Correct; the trade is stated in `000`'s table. Shorter critical path against a 20-job budget of short jobs. | + +## 3f. Close-out + +D summary records: the real terminal outcome, the measured before/after, what +did **not** improve, and which assumption would have to be wrong for this to be +the wrong call. The candidate for that last one is stated up front: if +Windows-only regressions turn out to be frequent on `dev`, the correct response +is to put Windows back on the PR lane, sharded, rather than to defend this +arrangement. diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 550a09ac7..e4b1adc6c 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -35,19 +35,109 @@ describe("GitHub Actions hardening", () => { }; // Job-scoped: a global count still passes if values are swapped between jobs. - // Pin ownership explicitly. `test` is 30m for Windows isolate margin on #827 - // after state-store admission — do not raise again; fix hung tests instead - // (unref'd oauth waitMs / shell kill-grace). Selector stays at 2; smoke at 8. + // Pin ownership explicitly. `platform-windows` keeps the 30m Windows isolate + // margin from #827 after state-store admission — do not raise again; fix hung + // tests instead (unref'd oauth waitMs / shell kill-grace). A `test` shard runs + // a quarter of the suite, so 15m there means wedged rather than slow. expect(ci.jobs?.["select-windows-runner"]?.["timeout-minutes"]).toBe(2); - expect(ci.jobs?.test?.["timeout-minutes"]).toBe(30); + expect(ci.jobs?.test?.["timeout-minutes"]).toBe(15); + expect(ci.jobs?.gates?.["timeout-minutes"]).toBe(15); + expect(ci.jobs?.["platform-macos"]?.["timeout-minutes"]).toBe(30); + expect(ci.jobs?.["platform-windows"]?.["timeout-minutes"]).toBe(30); expect(ci.jobs?.["npm-global-smoke"]?.["timeout-minutes"]).toBe(8); + expect(ci.jobs?.ci?.["timeout-minutes"]).toBe(5); // Every job must stay bounded — an unbounded job can hang a queue for hours. - expect(count(workflow, "timeout-minutes:")).toBe(3); + // Asserted structurally rather than by counting the string: a count passes if + // a job is added while another loses its bound in the same edit. Iterating the + // parsed jobs proves what the sentence above always claimed, and names the + // offending job when it fails. + for (const [name, job] of Object.entries(ci.jobs ?? {})) { + expect(`${name}:${typeof job?.["timeout-minutes"]}`).toBe(`${name}:number`); + } expect(workflow).toContain("actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0"); expect(workflow).toContain("oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6"); expect(workflow).toContain("actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e"); expect(workflow).toContain("bun test --isolate tests"); expect(workflow).not.toMatch(/uses:\s+\S+@(?:v\d+|main|master)\b/); + + // Sharding is only safe while the shards tile the suite exactly. If the + // matrix and the divisor drift apart, some files stop running and CI stays + // green — the worst failure available here. Pin them to each other. + const shards = (ci.jobs?.test as { strategy?: { matrix?: { shard?: number[] } } }) + ?.strategy?.matrix?.shard ?? []; + expect(shards).toEqual([1, 2, 3, 4]); + expect(workflow).toContain(`--shard=\${{ matrix.shard }}/${shards.length}`); + + // The aggregate gate is the check a human trusts. Three ways to break it + // silently: drop `if: always()` so it skips (and a skipped job reports + // success), shrink `needs:` so it stops covering a job, or add a job and + // forget to gate it. Deriving the expected list from the workflow's own job + // keys closes all three — a hardcoded list rots on the next job added. + const gate = ci.jobs?.ci as { if?: unknown; needs?: string[] } | undefined; + expect(gate?.if).toBe("always()"); + expect([...(gate?.needs ?? [])].sort()) + .toEqual(Object.keys(ci.jobs ?? {}).filter(name => name !== "ci").sort()); + + // macOS is the unsharded control for the sharded Linux lane: it is the only + // place the whole suite runs in one pool. Sharded or conditional, it stops + // being a control. + const macosSteps = (ci.jobs?.["platform-macos"] as { steps?: { run?: string }[] })?.steps ?? []; + expect(macosSteps.some(step => step.run?.includes("bun test --isolate tests"))).toBe(true); + expect(macosSteps.some(step => step.run?.includes("--shard"))).toBe(false); + expect(ci.jobs?.["platform-macos"]).not.toHaveProperty("if"); + + // Windows leaving the PR lane is a trade, not a deletion: it still runs + // before anything is published. Assert the positive condition rather than the + // absence of `pull_request` — `!= 'pull_request'` also matches every push to + // dev, which would restore the 16-minute leg to the busiest lane while still + // passing a loosely-worded test. + const windowsIf = String((ci.jobs?.["platform-windows"] as { if?: string })?.if ?? ""); + expect(windowsIf).toContain("github.event_name == 'workflow_dispatch'"); + expect(windowsIf).toContain("github.ref == 'refs/heads/main'"); + expect(windowsIf).toContain("github.ref == 'refs/heads/preview'"); + expect(windowsIf).not.toContain("refs/heads/dev"); + + // Windows runs the same full suite, and keeps the self-hosted workspace wipe. + // Without the wipe a deleted file survives on the runner's disk and the suite + // passes against a tree that no longer exists in git. + const winSteps = (ci.jobs?.["platform-windows"] as { steps?: { if?: string; run?: string }[] })?.steps ?? []; + expect(winSteps.some(step => step.run?.includes("bun test --isolate tests"))).toBe(true); + expect(winSteps.some(step => step.run?.includes("--shard"))).toBe(false); + expect(winSteps.some(step => step.if === "runner.environment == 'self-hosted'" + && step.run?.includes("git clean -xffd"))).toBe(true); + + // Every job that runs the root suite must build the GUI first, unconditionally. + // Tests that fetch the served dashboard read their session bootstrap out of + // `gui/dist/index.html`; with no build the server has no index to serve and the + // assertions read an empty string. The old three-platform job satisfied this by + // accident, because the same job also ran the GUI build — splitting the suite + // away from the gates removed that coincidence, and the shards went red on a + // pull request before this pin existed. + for (const jobName of ["test", "platform-macos", "platform-windows"]) { + const steps = (ci.jobs?.[jobName] as { steps?: { if?: string; run?: string }[] })?.steps ?? []; + const build = steps.find(step => step.run?.includes("bun run build")); + expect(`${jobName}:${build === undefined}`).toBe(`${jobName}:false`); + expect(`${jobName}:${build?.if ?? "unconditional"}`).toBe(`${jobName}:unconditional`); + } + + // No job in this workflow pushes, and the self-hosted runner keeps its + // checkout between jobs, so a persisted token is avoidable residue. The + // other workflows in this repository already set this; ci.yml was the gap. + const checkouts = Object.values(ci.jobs ?? {}) + .flatMap(job => (job as { steps?: { uses?: string; with?: Record }[] })?.steps ?? []) + .filter(step => step.uses?.startsWith("actions/checkout@")); + expect(checkouts.length).toBeGreaterThan(0); + for (const [index, step] of checkouts.entries()) { + expect(`checkout[${index}]:${step.with?.["persist-credentials"]}`).toBe(`checkout[${index}]:false`); + } + + // The self-hosted workspace wipe must not swallow its own failure. A clean + // that fails on permissions leaves deleted files on disk, and the checkout + // after it then validates a tree that no longer exists in git. + const wipe = ((ci.jobs?.["platform-windows"] as { steps?: { if?: string; run?: string }[] })?.steps ?? []) + .find(step => step.run?.includes("git clean -xffd")); + expect(wipe?.run).not.toContain("|| true"); + expect(wipe?.run).toContain("git rev-parse --is-inside-work-tree"); }); test("PR checks reach every branch the target gate accepts", async () => { @@ -106,6 +196,9 @@ describe("GitHub Actions hardening", () => { ".github/workflows/release.yml", ".github/workflows/stale-needs-info.yml", ".npmignore", + "LICENSE", + "README.md", + "assets/**", "bin/**", "bun.lock", "gui/**", @@ -130,6 +223,71 @@ describe("GitHub Actions hardening", () => { expect(workflow).toContain("bun run lint"); expect(workflow).toContain("- name: GUI build"); expect(workflow).toContain("bun run build"); + + // Presence is no longer enough. After area scoping, a step conditioned on a + // filter that never fires is a dropped gate wearing the step's name — the + // same outcome #97 hit, reached a different way. Pin the condition to the + // filter output, and pin the filter to patterns that can actually match. + const ci = Bun.YAML.parse(workflow) as { + jobs?: Record | undefined>; + }; + const gateSteps = (ci.jobs?.gates as { + steps?: { name?: string; if?: string }[]; + })?.steps ?? []; + for (const stepName of ["GUI lint", "GUI build"]) { + const step = gateSteps.find(candidate => candidate.name === stepName); + expect(`${stepName}:${step === undefined}`).toBe(`${stepName}:false`); + expect(step?.if).toBe("needs.changes.outputs.gui == 'true'"); + } + + const filterStep = (ci.jobs?.changes as { + steps?: { with?: Record }[]; + })?.steps?.find(step => step.with?.filters); + + // `base` is not cosmetic. Unset, paths-filter diffs a `dev` push against the + // repository default branch (`main`), so everything changed since the last + // promotion still reads as changed and the scoped jobs run anyway — the + // filter would look correct, stay green, and save nothing. + expect(filterStep?.with?.base).toBe("${{ github.ref }}"); + + // paths-filter cannot read a PR's file list without this, and a filter that + // errors produces empty outputs — which every `== 'true'` condition reads as + // "skip". The scoped jobs would silently stop running. + expect((ci.jobs?.changes as { permissions?: Record })?.permissions) + .toEqual({ contents: "read", "pull-requests": "read" }); + + // Whole-list comparison, not samples. Every entry is an input to the + // published tarball; dropping one silently stops packaging verification for + // that surface. `src/**` is the load-bearing one: it keeps a source-only PR + // running the Windows packaged-CLI smoke now that the Windows suite only + // runs at the shipping boundary. + const filters = String(filterStep?.with?.filters ?? ""); + const packagingBlock = filters.split(/\n\s*packaging:\s*\n/)[1] ?? ""; + const packaging = [...packagingBlock.matchAll(/-\s*'([^']+)'/g)].map(match => match[1]).sort(); + expect(packaging).toEqual([ + ".npmignore", + ".gitattributes", + "LICENSE", + "README.md", + "assets/**", + "bin/**", + "bun.lock", + "gui/**", + "package.json", + "scripts/prepare-package.ts", + "src/**", + ].sort()); + + // A per-job filter can only narrow what the workflow-level filter admits, so + // every packaging pattern that names a real path must also appear in the + // trigger's own path list. Otherwise the workflow never runs for that file + // and the filter entry is decoration. + const triggerPaths = (ci.on as { pull_request?: { paths?: string[] } } | undefined) + ?.pull_request?.paths ?? []; + for (const pattern of packaging) { + if (pattern === "scripts/prepare-package.ts") continue; // covered by scripts/** + expect(`${pattern}:${triggerPaths.includes(pattern)}`).toBe(`${pattern}:true`); + } }); test("stale needs-info workflow is schedule-only and least-privilege", async () => {