From 357d907a72777ffcbf28f4a24066c1598caeccb6 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 01:46:46 +0900 Subject: [PATCH 1/6] docs(devlog): plan the dev CI lane restructure before touching a workflow The Windows leg decides when a pull request turns green. On the last all-green run the suite took 5m49s on ubuntu, 5m23s on macos, and 16m23s on windows, so a contributor waits roughly three times longer than the verdict actually took to produce. The 30-minute ceiling on that job, and the two increases before it, are the same problem being paid for rather than fixed. This unit plans the change: four Linux shards for the suite, one job for the platform-independent gates, macOS kept whole and unsharded as the control that would notice if the shards stopped being independent, and Windows moved to promotion and dispatch where a maintainer consumes it. One aggregate check named `ci` covers whatever the job graph happens to be, so shard counts and platform legs can move without a required check name moving with them. No workflow is edited here. The docs carry the diff-level specification and the evidence behind it: shard tiling proven on this suite (four shards, 120 files each, union 480, no overlap), the runner and required-check behavior quoted from the official documentation, and the measured baseline the result will be compared against. An independent review ran four rounds against this plan and failed it three times. Every blocker was a local fact asserted without being checked: tests said not to need React that import JSX, a phase boundary that would have deleted two platforms in a commit claiming it did not, a duplicated YAML key, and a changed-files action left on its default base that would have diffed dev against main and saved nothing. The rounds and their dispositions are recorded with the plan, since the corrections are more useful to the next person than a clean-looking document. --- .../260803_ci_dev_lane_sharding/000_plan.md | 230 +++++++++++ .../001_shard_evidence.md | 157 +++++++ .../002_audit_synthesis.md | 317 +++++++++++++++ .../010_linux_shard_matrix.md | 384 ++++++++++++++++++ .../011_platform_legs.md | 270 ++++++++++++ .../030_affected_scoping.md | 368 +++++++++++++++++ .../040_ship_and_review.md | 166 ++++++++ 7 files changed, 1892 insertions(+) create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/000_plan.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/001_shard_evidence.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/010_linux_shard_matrix.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/030_affected_scoping.md create mode 100644 devlog/_plan/260803_ci_dev_lane_sharding/040_ship_and_review.md 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..eac3a5918 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md @@ -0,0 +1,317 @@ +# 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. 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..1ae51237b --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/010_linux_shard_matrix.md @@ -0,0 +1,384 @@ +# 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 + + - 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..bc3d567b6 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md @@ -0,0 +1,270 @@ +# 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, lint, and GUI build 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. + - 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: 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..819272a47 --- /dev/null +++ b/devlog/_plan/260803_ci_dev_lane_sharding/040_ship_and_review.md @@ -0,0 +1,166 @@ +# 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 +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 +git log origin/dev..HEAD --format='%B' | grep -nEi '/Users/|worktree|macmini|session' || echo clean +git diff origin/dev..HEAD -- .github tests | grep -nEi '/Users/|macmini' || echo clean +``` + +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. From 850affb64650e7b21e66c67dce12e40f9fdf77d7 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 02:13:22 +0900 Subject: [PATCH 2/6] ci: shard the suite on Linux and move Windows to the shipping boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One matrix job ran the whole gate on three platforms, and the Windows leg took 16m23s against ubuntu's 5m49s and macos's 5m23s on the last all-green run. It decided when every pull request turned green. The comment above its timeout records the ceiling being raised twice rather than the gap being closed, most recently to 30 minutes. The suite now runs as four Linux shards. `bun test --shard=i/4` sorts files by path and deals them round-robin, so the split is deterministic and the four shards cover the suite exactly once: 120 files each, union 480, no file in two shards. Everything that is not the suite — typecheck, privacy scan, GUI tests, lint, build, release-helper syntax, CLI smoke — moves to a single `gates` job instead of being repeated per platform. macOS keeps running the whole suite, unsharded. That is deliberate: the shards each cover a quarter of the files, which assumes no test depends on a sibling having run in the same pool, and this leg is the control that would notice if that assumption broke. It is also the fastest leg on the board, so there was never a reason to trim it. Windows moves to promotion and dispatch. Its coverage is not dropped — release.yml already refuses to publish a commit without a successful run of this workflow, so main and preview still gate on it, and the Windows npm-global smoke still runs on every push. The trade is that a Windows-only regression can now reach dev and be caught at promotion rather than at review. dev is not published; that is what makes the trade acceptable rather than free. A new `ci` job is the one check that means "CI passed". Shard names move with the shard count and the platform legs come and go by trigger, so neither is a stable thing to require in branch protection. It depends on every other job and asserts each result with an allowlist: success and skipped pass, anything else fails, so a result type added later cannot slip through. `if: always()` is what keeps it from being skipped along with a failed dependency and reporting success by omission. The suite's workflow pins move with it. The exact `timeout-minutes:` occurrence count is replaced by iterating the parsed jobs, which is what its comment always claimed to check; the shard matrix and the `--shard` divisor are pinned to each other, because drift between them would stop running files while CI stayed green; and the gate's `needs` is compared against the workflow's own job list rather than a hardcoded one, so adding a job without gating it fails immediately. Both new pins were driven red before being kept. --- .github/workflows/ci.yml | 241 ++++++++++++++++++++++++++++++------- tests/ci-workflows.test.ts | 67 ++++++++++- 2 files changed, 259 insertions(+), 49 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fa2248e0..01c77e047 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,54 +111,58 @@ jobs: echo 'label=windows-latest' >> "$GITHUB_OUTPUT" fi + # 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 - # 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 + + - 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 + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 @@ -176,9 +180,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 @@ -201,6 +202,107 @@ jobs: - 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 + + - 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: 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 + 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: 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 }} runs-on: ${{ matrix.os }} @@ -243,3 +345,54 @@ 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: [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 diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 550a09ac7..294117ca3 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -35,19 +35,76 @@ 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); }); test("PR checks reach every branch the target gate accepts", async () => { From 54a9c9b7ba1bad504bd05579c596886eb1e6b312 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 02:21:11 +0900 Subject: [PATCH 3/6] ci: run the packaging and GUI jobs only when their inputs move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sharded lane still paid for three npm-global smoke jobs and a GUI lint and build on every run, including pushes that touched neither. This scopes them to the areas that can actually break them. The filtering is per job, never on the workflow trigger. A workflow skipped by path filtering leaves its checks Pending forever, so a pull request requiring them can never merge, while a job skipped by a condition reports success. That asymmetry is why the coarse `paths:` list in the `on:` block stays coarse and a `changes` job does the real work. `base: ${{ github.ref }}` is doing more than it looks. Left unset, the action compares against the repository default branch, which is `main` — so a push to `dev` would be diffed against `main` and every area touched since the last promotion would keep reading as changed. The scoped jobs would run on nearly every push, the saving would not happen, and nothing would look wrong. The packaging filter covers `src/**` deliberately. package.json ships `src` and bin/ocx.mjs executes it, so a source change is a packaging change; without that entry an ordinary source pull request would get no Windows verification at all now that the Windows suite runs only at the shipping boundary. That keeps the 1m53s Windows smoke on the common path while the 16-minute suite stays off it. Three tarball inputs — assets, README, LICENSE — were named by the filter but absent from the trigger's own path list, which meant a change to one of them never started this workflow and the filter never got to see it. The trigger list grows from 14 entries to 17 to match, on both push and pull_request, since checking one and not the other is how a change lands on dev having been verified on the wrong trigger. The pins move with the behavior. The GUI lint and build assertions became substring checks that a permanently-false condition would still satisfy, so they now assert the condition itself: a gate that cannot fire is dropped in every sense that matters, which is the outcome PR #97 already hit once by a different route. The packaging filter is compared as a whole list rather than sampled, and every pattern in it is checked against the trigger's path list so an unreachable entry fails the suite. Each new pin was driven red before being kept. --- .github/workflows/ci.yml | 73 +++++++++++++++++++++++++++++++++++++- tests/ci-workflows.test.ts | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01c77e047..17e12a17c 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,6 +117,66 @@ 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 + + - 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' + - '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, @@ -160,6 +226,7 @@ jobs: # fixed couple of minutes four times to save nothing. gates: name: gates + needs: changes runs-on: ubuntu-latest timeout-minutes: 15 steps: @@ -190,11 +257,13 @@ 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 @@ -305,6 +374,8 @@ jobs: npm-global-smoke: name: npm-global ${{ matrix.os }} + needs: changes + if: needs.changes.outputs.packaging == 'true' runs-on: ${{ matrix.os }} timeout-minutes: 8 strategy: @@ -371,7 +442,7 @@ jobs: # 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: [select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] + needs: [changes, select-windows-runner, test, gates, platform-macos, platform-windows, npm-global-smoke] runs-on: ubuntu-latest timeout-minutes: 5 steps: diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 294117ca3..b69da1226 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -163,6 +163,9 @@ describe("GitHub Actions hardening", () => { ".github/workflows/release.yml", ".github/workflows/stale-needs-info.yml", ".npmignore", + "LICENSE", + "README.md", + "assets/**", "bin/**", "bun.lock", "gui/**", @@ -187,6 +190,70 @@ 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", + "LICENSE", + "README.md", + "assets/**", + "bin/**", + "bun.lock", + "gui/**", + "package.json", + "scripts/prepare-package.ts", + "src/**", + ]); + + // 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 () => { From 32c56a82952aee344f95f9268a0846b5b1a43b7b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 02:39:43 +0900 Subject: [PATCH 4/6] ci: build the GUI in every job that runs the root suite The first sharded run went red on shard 1/4 and macos, and the failure was mine rather than flaky: tests that fetch the served dashboard read their session bootstrap out of gui/dist/index.html, and nothing in those jobs built it. The old three-platform job satisfied this by accident. It ran the suite and the GUI build in the same job, so gui/dist always existed by the time the tests ran. Splitting the suite away from the gates removed the coincidence without removing the dependency, and area-scoping the gates' build behind `gui/**` meant even that copy was conditional. Reproduced on an untouched checkout of dev to be sure the split was the cause: the same test fails there with no gui/dist present and passes after `bun run build`. So the dependency is real and pre-existing; only its accidental satisfaction disappeared. The suite pin that would have caught this now exists: every job running the root suite must have an unconditional GUI build step. It was driven red before being kept. --- .github/workflows/ci.yml | 23 +++++++++++++++++++++++ tests/ci-workflows.test.ts | 14 ++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 17e12a17c..28b53262e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -218,6 +218,17 @@ jobs: 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 @@ -301,6 +312,12 @@ jobs: 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 @@ -366,6 +383,12 @@ jobs: 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 diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index b69da1226..184513d9d 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -105,6 +105,20 @@ describe("GitHub Actions hardening", () => { 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`); + } }); test("PR checks reach every branch the target gate accepts", async () => { From fd5ca50fa21e1975d2432bed75033b57dd00bf19 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 03:02:02 +0900 Subject: [PATCH 5/6] ci: close the Windows gap the release gate could still walk through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a hole this change opened outside the file it edited. release.yml accepts any successful ci.yml run for the release SHA. After a promotion, the pull-request run for that same commit is still there and still green — and now deliberately Windows-free, since the Windows leg is scoped to promotion and dispatch. So a publish could be satisfied by a run that proved nothing about Windows while the promotion run carrying it was still in flight, or had failed. The gate now selects a `push` run on the release branch specifically. A pull-request run for the same SHA no longer qualifies, and the error says why so the next person does not read it as flakiness. Also from review: - `.gitattributes` joins the packaging filter. It 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. - The self-hosted workspace wipe stops swallowing its own failure. A clean that fails on permissions left deleted files on disk, and the checkout after it then validated a tree that no longer exists in git. Only the no-repository case — first run on a fresh box — is tolerated. - Every checkout sets `persist-credentials: false`, matching what the other workflows here already do. No job in this one pushes, and the self-hosted box keeps its checkout between jobs. The plan documents carried the same GUI-build omission the first live run found, so they are corrected too rather than left describing a workflow that fails. One review point is rebutted rather than applied: the devlog dates are not future-dated. They read 2026-08-03 because that is the date in the authoring timezone; the same moment is 2026-08-02 in UTC, which is what the CI timestamps show. The reasoning is recorded with the audit trail. --- .github/workflows/ci.yml | 44 ++++++++++++- .github/workflows/release.yml | 16 ++++- .../002_audit_synthesis.md | 66 +++++++++++++++++++ .../010_linux_shard_matrix.md | 10 +++ .../011_platform_legs.md | 20 +++++- .../040_ship_and_review.md | 13 +++- tests/ci-workflows.test.ts | 22 ++++++- 7 files changed, 181 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 28b53262e..4879d4bd7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -142,6 +142,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 - name: Detect changed areas id: filter @@ -173,6 +178,10 @@ jobs: - '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' @@ -201,6 +210,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 - name: Setup Bun uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 @@ -243,6 +257,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 - name: Setup Bun uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 @@ -300,6 +319,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 - name: Setup Bun uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 @@ -367,10 +391,23 @@ jobs: - name: Clean workspace (self-hosted only) if: runner.environment == 'self-hosted' shell: bash - run: git clean -xffd . || true + # `|| 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 @@ -412,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. 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/002_audit_synthesis.md b/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md index eac3a5918..950c3f2d9 100644 --- a/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md +++ b/devlog/_plan/260803_ci_dev_lane_sharding/002_audit_synthesis.md @@ -315,3 +315,69 @@ 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 index 1ae51237b..ddd8ef58b 100644 --- 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 @@ -101,6 +101,16 @@ AFTER: 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 ``` 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 index bc3d567b6..b08790f39 100644 --- a/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md +++ b/devlog/_plan/260803_ci_dev_lane_sharding/011_platform_legs.md @@ -103,9 +103,18 @@ ordinary jobs. cd gui bun install --frozen-lockfile # The whole suite, unsharded and in one pool. Deliberately NOT the gates: - # typecheck, privacy scan, lint, and GUI build 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. + # 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 @@ -155,6 +164,11 @@ ordinary jobs. cd gui bun install --frozen-lockfile + - name: Build GUI + run: | + cd gui + bun run build + - name: Test run: bun test --isolate tests 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 index 819272a47..63667f1c2 100644 --- 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 @@ -12,6 +12,7 @@ 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 @@ -43,10 +44,18 @@ worktree directory name, a machine name, or an internal session reference. Check before pushing: ```bash -git log origin/dev..HEAD --format='%B' | grep -nEi '/Users/|worktree|macmini|session' || echo clean -git diff origin/dev..HEAD -- .github tests | grep -nEi '/Users/|macmini' || echo clean +# 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 diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 184513d9d..e4b1adc6c 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -119,6 +119,25 @@ describe("GitHub Actions hardening", () => { 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 () => { @@ -247,6 +266,7 @@ describe("GitHub Actions hardening", () => { const packaging = [...packagingBlock.matchAll(/-\s*'([^']+)'/g)].map(match => match[1]).sort(); expect(packaging).toEqual([ ".npmignore", + ".gitattributes", "LICENSE", "README.md", "assets/**", @@ -256,7 +276,7 @@ describe("GitHub Actions hardening", () => { "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 From 6b03796900d9040150c5dbc10792101d36dbf1c3 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 11:37:11 +0900 Subject: [PATCH 6/6] ci: make the aggregate gate refuse a silently skipped windows leg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skipped` counting as a pass is right for a job that is SUPPOSED to be skipped — that is how `platform-windows` reports on a pull request, and the whole point of moving it to the shipping boundary. It is wrong everywhere else, and the gate could not tell the two apart. The failure mode that leaves: a one-character slip in the windows `if:` condition skips the leg on a `main`/`preview` push, every needed job reports `success` or `skipped`, `ci` goes green, and the release helper — which only asks whether this workflow succeeded for the exact SHA — publishes a tip no Windows machine ever executed. Precisely the gap the previous commit closed on the release side, reachable again through the aggregate. The gate now asserts the windows result explicitly on the events where it is mandatory (`main`/`preview` pushes and `workflow_dispatch`), and keeps accepting `skipped` on pull requests and `dev` pushes, where it is the intended behavior. --- .github/workflows/ci.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4879d4bd7..51014e332 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -532,3 +532,25 @@ jobs: 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