ci: give check:affected real Apple ownership rules and route ios.yml on them (#1781 A9-2) - #1857
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
|
Not ready at exact The body claims the dependency-closure fix is in At the actual head, the routing P1 remains: any ignored The Coverage failure is inherited main-side ratchet skew tracked by #1860, not caused by this patch. The body is also stale: it describes the unpushed fix and says 14 touched files while the exact PR contains 13. |
…1857) The exact-name .github exemption was unbounded: naming the lane's own setup-apple-runner-build or boot-ios-test-simulator action skipped the lane that runs them and the manifest stayed green. Lane now carries the transitive composite-action closure plus its own workflow file (Lane.uses, same walk declaredGates does), and the exemption refuses anything in it. Also: an unowned path under an ignored root (a non-TS fixture under a family root) asked for the ignore entry to be removed, which would un-route every sibling in that tree; it now asks for a selector owner. Both cases pinned, both proven red against the pre-fix code. Documents GitHub's 300-changed-file path-filter limit in docs/agents/testing.md.
4992259 to
1f4ed03
Compare
|
Re-reviewed GitHub exact head The body is materially stale: it describes the unpushed fix and says 14 touched files, while GitHub reports 13. The PR base |
…1857) The exact-name .github exemption was unbounded: naming the lane's own setup-apple-runner-build or boot-ios-test-simulator action skipped the lane that runs them and the manifest stayed green. Lane now carries the transitive composite-action closure plus its own workflow file (Lane.uses, same walk declaredGates does), and the exemption refuses anything in it. Also: an unowned path under an ignored root (a non-TS fixture under a family root) asked for the ignore entry to be removed, which would un-route every sibling in that tree; it now asks for a selector owner. Both cases pinned, both proven red against the pre-fix code. Documents GitHub's 300-changed-file path-filter limit in docs/agents/testing.md.
1f4ed03 to
354126c
Compare
|
The re-review above read head What is now at the head, all of it reviewable:
Local evidence on the rebased tree: Note for whoever merges — this is ratchet-class, as the body says: the routing assertion runs over every tracked path, so a path added on |
|
Re-reviewed exact Before readiness: update the stale body reference from unreachable |
|
Out of draft. Head The iOS lane started on this PR rather than being skipped — that is the fail-open direction proving itself in production: the diff touches Local evidence on this tree: Merge note (ratchet-class, repeated from the body): the routing assertion runs over every tracked path, so a path landing on |
…1857) The exact-name .github exemption was unbounded: naming the lane's own setup-apple-runner-build or boot-ios-test-simulator action skipped the lane that runs them and the manifest stayed green. Lane now carries the transitive composite-action closure plus its own workflow file (Lane.uses, same walk declaredGates does), and the exemption refuses anything in it. Also: an unowned path under an ignored root (a non-TS fixture under a family root) asked for the ignore entry to be removed, which would un-route every sibling in that tree; it now asks for a selector owner. Both cases pinned, both proven red against the pre-fix code. Documents GitHub's 300-changed-file path-filter limit in docs/agents/testing.md.
354126c to
a600673
Compare
|
Pre-merge rebase done — the ratchet condition this PR's body warns about arrived for real. Head is now Did the rename require a change here? No — and confirmed rather than assumed. The rebase was clean because this branch never touches Does the renamed host-XCTest job need an ownership rule? No. It declares exactly one gate, Re-run against main at The merge condition in the body still stands for whoever merges: re-run |
|
Re-reviewed exact |
…on them (#1781 A9-2) Device-lane ownership by platform family in the affected selector (scripts/check-affected/device-lanes.ts): a TypeScript-only Apple change now carries replay-ios/replay-ios-device/replay-macos in a narrow plan, other families own only their own lanes, shared runtime surface owns every lane, unit tests own none. Golden tables (contracts/fixtures) own the parity unit test and both runner builds instead of failing open. ios.yml pull_request paths-ignore is routed on that ownership; the gate manifest asserts the list against the selector over every tracked path both ways (scripts/gate/routing.ts, ROUTED_LANES). push to main is unfiltered. Path coverage exempts declared manual-only checks the way owned does.
…1857) The exact-name .github exemption was unbounded: naming the lane's own setup-apple-runner-build or boot-ios-test-simulator action skipped the lane that runs them and the manifest stayed green. Lane now carries the transitive composite-action closure plus its own workflow file (Lane.uses, same walk declaredGates does), and the exemption refuses anything in it. Also: an unowned path under an ignored root (a non-TS fixture under a family root) asked for the ignore entry to be removed, which would un-route every sibling in that tree; it now asks for a selector owner. Both cases pinned, both proven red against the pre-fix code. Documents GitHub's 300-changed-file path-filter limit in docs/agents/testing.md.
Lane.uses recorded only each composite action's action.yml, so a support file the descriptor executes was exemptible as if it were an unrelated sibling workflow: ios.yml uses setup-fixture-app, whose action.yml runs "$GITHUB_ACTION_PATH/fetch-artifact.sh", and that script runs its siblings resolve-artifact-name.sh and trusted-artifact.mjs — references that exist only inside shell, one level past anything YAML parsing sees. The closure unit is the action's directory now. It needs no shell model and cannot miss a file however deep the reference chain runs; the coarseness is harmless because a file in an action's own directory belongs to that action. All three files pinned, red against the descriptor-only closure.
a600673 to
17a762f
Compare
|
Second bypass closed. Head is Which fix, and why that one. I extended the closure to every file under a composite action's directory, rather than narrowing the exemption or parsing Planted witness ( Pinned as a test over all three files — the descriptor-named one and the two named only inside the shell script — and the test is non-vacuous: reverting the closure to descriptors-only fails exactly it ( Is anything else path-referenced? I checked the three shapes you named, and the answer is no, for a structural reason worth stating: the exemption only ever applies to Merge condition, corrected. The body no longer names a fixed base: it now says to re-run Local on this head: |
|
Independent re-review of |
Summary
pnpm check:affectedgains device-lane ownership by platform family, andios.ymlis routed on it atpull_requesttime via apaths-ignorelist that the gate manifest holds to the selector over every tracked path, both ways. Part of #1781 (A9-2).Before: a TypeScript-only Apple change (
src/platforms/apple/**,packages/platform-apple/**) produced a plan with no iOS check at all —swift-runner-iosfired only for.swift/apple/runner/**, andreplay-ioswas reachable only through a full fail-open.ios.ymlran on every non-docs PR, including Android-only and unit-test-only ones.After:
pushtomainis unfiltered: main runs every lane unconditionally.The four lines
check:affectedtell the truth about Apple changes (TS-only Apple PRs such as fix(ios): preserve final-probe xcodebuild diagnostics #1776/refactor: narrow perf API to actionable evidence #1731/fix(ios): use current devicectl capture-screenshot syntax for physical devices #1769 now carryreplay-iosinstead of a plan that omits iOS entirely).ios.ymlPR runs of the last 30 days → 0 hits) — 871 merged PRs, 3,286pull_requestios.ymlruns; would-skip-and-really-broke-the-runner = 0. Planted-red proofs:scripts/check-affected/device-lanes.test.ts(Apple file →replay-ios; Android-only + any tooling file → full fail-open) andscripts/gate/routing.test.ts(an ignore hidingsrc/platforms/apple/**or.github/actions/**fails the manifest; droppingsrc/platforms/android/**or the unit-test ignore fails it the other way).paths-ignorebefore it allocates a runner; no plan job on the critical path, noif:on the 12 smoke steps); macOS runner occupancy −61 runs / −691 min per 90 days over what A9-1 already saved (1.9% of the lane; last 30 days: −31 runs / −422 min of 16,490); red-lane attention 0 (no new lane); harness ≈ 165 LOC selector + 100 LOC manifest assertion + 23 YAML lines, all under existing gates (check:affected:test,check:gate-manifest:test).maingoes red onios.ymlfor a change a routed PR skipped (a cross-family lifecycle call, see residual risk), drop the routing entries fromios.ymland theROUTED_LANESrecord in the same commit; the ownership rules stay.Design note
What enumerates the Apple surface? Not the import graph. The daemon value-imports every family:
src/platform-runtime.tscomposes all six platform packages,src/core/interactors/*import each family's actions, andsrc/daemon/session-teardown.ts/src/daemon/server/daemon-runtime.tscall Android and web lifecycle helpers on every session close and daemon start. Reachability fromsrc/bin.tsis therefore the whole tree, and the layering graph additionally does not resolve relative imports insidepackages/(intra-package edges are invisible;packages/contracts/src/facades/*fan out to nothing). Transitive ownership derived from that graph would either classify everything as Apple or miss what matters.What the repo does enforce is the platform partition:
CANONICAL_PLATFORM_FAMILIES(scripts/layering/platform-package-policy.ts) names the families, layering R13 pins each family's runtime topackages/platform-<family>/andsrc/platforms/<family>/, and the remaining family-owned trees are named by a family or Apple-leaf directory segment (android/,linux/,test/integration/replays/<leaf>/,src/daemon/snapshot-presentation/ios/) or, undertest/integration/, by the lane prefix of the smoke file. Soscripts/check-affected/device-lanes.ts— the single declaration site — says:apple→ replay-ios, replay-ios-device, replay-macos ·ios→ replay-ios, replay-ios-device ·macos→ replay-macos ·android→ replay-android ·linux→ replay-linux ·web→ web-smoke ·harmonyos/vega→ noneexamples/test-app/**src/,packages/*/src/src/*.json, a.pngundertest/integration/)Filename tags in shared directories are deliberately not used (
src/daemon/android-system-dialog.tsstays shared) — a naming convention is not a boundary.contracts/fixtures/*.json(TS/Swift golden tables) get their own build rule:unit+swift-runner-ios+swift-runner-macosinstead of fail-open.Decide before allocating macOS. Three mechanisms were compared: (a) an ubuntu plan job the smoke job
needs— adds ~1 min serial latency to all 3,286 iOS runs to skip 61, on the lane that is usually a PR's critical path; (b) computing the plan inside the macOS job and skipping the rest — the runner is still allocated for ~2 min, and 12 steps need anif:; (c)paths-ignore— GitHub evaluates it before creating the job, so it costs nothing and adds no latency, but it is a hand-written glob list next to a derived selector. (c) is shipped, made honest byscripts/gate/routing.ts: for every tracked path, the lane must start when the selector fails open on it or routes it to one of the lane's declared gates (swift-runner-ios) or sampled checks (replay-ios,replay-ios-device, declared once inROUTED_LANES), and must not start on a path the selector classifies as another family's or as a unit test. Docs paths are exempt; a.github/**path is exempt only when ignored by its exact name (deploy/docs-preview workflows the lane does not use — a glob there would fail).Fail-open on tooling. Unchanged and now asserted:
.github/**,scripts/**,package.json,pnpm-lock.yaml,tsconfig*.json,vitest*.ts,packages/*/package.json|tsconfig.json, and the selector's own sources fail open to the full set, so any PR touching one of them startsios.ymlregardless of what else it touches (routing.test.tsplants.github/actions/**andpackage.jsoninto the ignore list and expects the manifest to fail).Residual risk — large PRs. GitHub's path filters examine only the first 300 changed files. A PR whose first 300 paths all match the ignore list skips the lane even if file 301 is Apple code (in-window examples of PRs past the limit: #1499 at 511 files, #1494 at 865). Inherent to
paths-ignoreand shared with A9-1, but this list matches many more files than A9-1's did, so the exposure is larger. Backstop is the unfilteredpushtrigger onmain. Recorded indocs/agents/testing.md.Residual risk (stated, not hidden). A family-tagged production file can execute on another family's lane through a cross-family lifecycle call — 50 files under other-family roots are directly value/dynamic-imported by shared code today (
pnpm depgraph), e.g.session-teardown.ts → src/platforms/android/perf.ts,daemon-runtime.ts → src/platforms/web/agent-browser-lifecycle.ts. Backstops:ci.yml's node-integration lanes start a real daemon (import-time and startup breakage), the unit/provider suites, and the unfilteredpushtrigger onmain. The 90-day replay found no instance. A seam-based refinement (treat those 50 as shared, derived from the layering graph) is available if this ever bites; it was not shipped because it would needpaths:with 50 negations and churn on every new cross-import.Also fixed on the way:
check:gate-manifest's path-coverage assertion now exempts declared manual-only checks the way itsownedassertion already did (the parkedreplay-ios/replay-androidlanes are printed by name on every run; repeating the gap once per path that selects them buried the real findings).Retrospective replay (90 days, ran before routing)
Method: every PR merged since 2026-05-20 (GraphQL, files + commit oids), every
pull_requestios.ymlrun since 2026-05-15 (REST, 4,455 runs), matched by head SHA with a branch-name fallback inside the PR's lifetime; each PR's file list run through the new selector in-process, and separately through the newpaths-ignorelist.paths-ignoreas of main (A9-1, docs-only)ios.ymlrunIncremental would-skip by class: Android-only 14 (e.g. #1808 #1737 #1734 #1561 #1513 #1378 #1314 #1285 #1090 #903 #796 #788 #671 #1531), unit-tests-only 20 (e.g. #1790 #1642 #1552 #1113 #595), web-only 1 (#835). Their 61 runs: 53 green, 7 cancelled, 1 red.
The one red: #796 (
src/platforms/android/snapshot-helper-session.ts+ its test), run 27434816426, step "Prepare iOS runner" —Daemon request timed outat the then-300 sprepare ios-runnerbudget (a cold xcodebuild). The same step failed the same day on two unrelated PRs (runs 27420294661, 27407004126); the PR's follow-up commit changed two Android lines plus a test and went green; the file set cannot reachxcodebuild. Classified infra flake. (Under the selector-only view a second red appears, #1347, docs-onlyCONTEXT.md+docs/adr— "Boot iOS test simulator" failed; docs cannot break the runner and A9-1 already skips it.)Selector vs YAML agreement over the 871 PRs: the YAML would-skip set equals the selector's except four PRs the YAML skips and the selector fails open on — #1836/#1547 (
docs/agents/testing.md, selector-owning prose) and #607/#603 (docs-preview workflows ignored by exact name), the two documented exemptions.Review round (independent adversarial review — routing approved)
Findings addressed in the two commits at the head of this branch (
ci: bound the routed-lane exemption to sibling workflowsandci: close the routed-lane exemption over composite-action support files):Real hole, fixed in code. The exact-name
.githubexemption was unbounded: the reviewer plantedsetup-apple-runner-build/action.ymlandboot-ios-test-simulator/action.yml— the lane's own build and boot actions — intopaths-ignoreandcheck:gate-manifeststayed green.Lanenow carriesuses: the transitive local composite-action closure plus the workflow's own file, from the same walkdeclaredGatesperforms; the exemption refuses anything in it. Both planted cases (plusrun-gate/action.yml, reached only through a composite action, andios.ymlitself) are pinned inrouting.test.tsand proven red against the pre-fix code.1b. The same hole one level deeper, also fixed. A follow-up review found the closure stopped at the YAML boundary: it recorded each action's
action.ymlbut not the files that descriptor executes, so.github/actions/setup-fixture-app/fetch-artifact.sh— genuine iOS-lane machinery, run asbash "$GITHUB_ACTION_PATH/fetch-artifact.sh"— was still exemptible. The closure's unit is the action's directory now. That is deliberate over parsing$GITHUB_ACTION_PATH/...out of the YAML:fetch-artifact.shitself runsresolve-artifact-name.shandtrusted-artifact.mjs, references that exist only inside shell, so a YAML-parsing fix would have repeated the same one-level-short failure. A directory needs no shell model and cannot miss a file however deep the chain runs; the coarseness is harmless because a file in an action's own directory belongs to that action. All three files are pinned and red against the descriptor-only closure.Wrong remedy in a correct failure, fixed. A tracked non-TS file under an ignored family root (
src/platforms/android/probe-fixture.json) correctly failed, but told the reader to remove the ignore entry — which would un-route every sibling.tsin that tree. Fail-open causes are now split:unknown-path/ambiguous-pathask for a selector owner, everything else still asks for the entry's removal. Pinned, also red against the pre-fix code.300-file limit, documented. See the residual-risk note above and
docs/agents/testing.md.Merge condition (ratchet-class — do not skip)
The routing assertion runs over every tracked path and reads every workflow, so it behaves like the layering/coverage ratchets: an in-flight PR that adds a family-tagged path, a non-TS file under an ignored root, or a new composite-action support file is green on its own base and turns
check:gate-manifestred onmainafter both land (the #1825 × #1779 failure mode). Re-runpnpm check:gate-manifestagainst whatevermainis at merge time — not against the base this PR was last rebased onto, which goes stale within hours — and merge after any in-flight PR that adds paths undersrc/platforms/*,packages/platform-*,android/,linux/,test/integration/replays/*, or.github/actions/*.Validation
check:affected:test(47),check:gate-manifest:test(45, incl. the 8 new routing witnesses and the path-coverage exemption witness),depgraph:test(22) green;pnpm check:gate-manifestgreen (46 checks wired across 33 lanes, manual-only: replay-android, replay-ios, replay-ios-device).src/platforms/apple/**toios.ymlpaths-ignore→routing: ios.yml ignores src/platforms/apple/core/apps.ts, but the selector routes it to "replay-ios", "replay-ios-device", "replay-macos"; adding.github/actions/**→… fails open on it (workflow-tooling); removingsrc/platforms/android/**→ios.yml starts on src/platforms/android/adb-executor.ts, which the selector classifies as android-owned (lanes: replay-android). Plus the two review plants above. All are pinned inscripts/gate/routing.test.ts; the two review cases were re-run against the pre-fix code and fail there..github/**,scripts/check-affected/**,package.json,docs/agents/testing.md), soios.ymlruns on it — CI on this PR is the live proof that the fail-open direction still starts the lane.ios.yml. That is GitHub'spaths-ignoresemantics ("skip when every changed file matches"), the same mechanism A9-1 relies on; the first Android-only or test-only PR after merge will show it.Docs:
docs/agents/testing.md(selector section + gate-manifest section) updated; skills untouched.Touched: 14 files (4 new). Scope stayed in
scripts/check-affected,scripts/gate,.github/workflows/ios.yml, testing docs.