chore(ci): compute trunk merge-queue lanes from impacted targets - #74266
Conversation
The impacted-targets upload reported "ALL" for every PR, which overlaps every other PR's lane and left the queue effectively serial. Replace it with a computed target list so unrelated PRs can test and merge in parallel lanes. Targets are derived from the dependency graphs the repo already maintains: tach.toml for the Python product cascade (reusing turbo-discover's parser), backend:contract-check for product isolation, and the rust Cargo manifests for the crate graph. Path filters in ci-*.yml are deliberately not used as a source: they are tuned to over-run tests, and the frontend filter matches '**/*.md' and '**/*.yaml', so nearly every PR would land in one lane. Trunk runs two PRs in parallel only when their target sets are disjoint, so every rule is biased toward over-reporting. Unrecognized paths, unparseable graphs, and a missing merge base all fall back to "ALL", which restores the previous single-lane behavior rather than risking a silently narrow set. Measured on the current tree: a single-product backend change reports 1 target, a rust leaf crate 2, a shared rust crate 18, and a lockfile change still reports ALL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b964444 to
35f5433
Compare
Prompt To Fix All With AI### Issue 1
.github/scripts/trunk-impacted-targets.js:225-228
**Multiline dependencies lose graph edges**
When a workspace dependency uses a table such as `[dependencies.personhog-coordination]`, this parser reads the following `path` assignment as the dependency name and omits `personhog-stateright` from the reverse closure. Changes to these dependent crates then receive disjoint targets and can land together without their combined state being tested.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(ci): compute trunk merge-queue lan..." | Re-trigger Greptile |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
The rust crate graph only read dependency names from body keys, so a [dependencies.<name>] table dropped the edge entirely: the name lives in the header there, and the body holds path/version/features. Scanning that body could also invent an edge to any crate sharing a name with a Cargo attribute. rust/personhog-stateright declares personhog-coordination this way, so it was missing from that crate's reverse closure. A change to the coordination crate and a change to stateright got disjoint targets and could merge in parallel without their combined state being tested. Target-scoped tables ([target.'cfg(...)'.dependencies]) were skipped by the same header check. They only hold external crates today, so no edge was actually lost, but the shape now parses. [workspace.dependencies] stays excluded: it declares versions for every member rather than the crate's own edges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The impacted-targets job checked out the PR, ran its target script, and then handed TRUNK_API_TOKEN to a later step in the same job. Steps share a runner, so a same-repo PR could edit the script to prepend a directory to GITHUB_PATH and drop its own jq or curl there. The upload step then resolved those names to attacker-supplied executables and ran them with the token in their environment. Scoping the secret to a single step was never sufficient: GITHUB_PATH and GITHUB_ENV exist precisely to carry state across steps. Split into two jobs. `compute` checks out and runs the script with no secret in scope. `upload` gets the secret, never checks out, and never runs anything from the PR, so its jq and curl come from the runner image. Separate jobs get separate runners, so the only thing crossing the boundary is the JSON string in the job output, which is read through env: and jq --argjson rather than interpolated into a shell command. `upload` runs even when `compute` fails, falling back to ALL. A PR with no upload never enters the queue, so failing closed here would be worse than losing parallelism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tools/ was mapped wholesale to the backend targets on the claim that the suite imports it. That is only true for part of the tree. phrocs is Go, owned by ci-phrocs.yml and build-phrocs.yml, and nothing in the backend references it, so it was serializing against every backend PR for no reason. The same held for traffic-sim, hogbox-preview, infra-scripts, and the other standalone tools. Those now get a lane each. hogli and hogli-commands stay in the backend lanes because the coupling there is real: ci-backend.yml drives the suite through hogli, and posthog/conftest.py imports hogli_commands.quarantine.pytest_support on every pytest run. An unrecognized tool also defaults to the backend lanes, so a newly added one over-reports until someone classifies it. Two tools sit on the fe/py boundary and became tripwires instead: openapi-codegen generates the frontend API types from the backend serializers, and owners is read by the backend, frontend, and script suites alike. Leaving either in backend-only lanes would let a frontend PR merge in parallel with a change that rewrites its generated types. Files directly under tools/ are the CI-steering scripts (backend test selection, playwright spec selection, the selection verdict). They decide what every suite runs, so they widen fully. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
posthog-app/src/scenes/inbox/InboxScene.js |
684.7 KiB | 🔺 +4.8 KiB (+0.7%) |
posthog-app/_parent/products/managed_migrations/frontend/ManagedMigration.js |
27.4 KiB | 🔺 +3.3 KiB (+13.6%) |
posthog-app/src/scenes/onboarding/Onboarding.js |
814.4 KiB | 🔺 +3.0 KiB (+0.4%) |
render-query/src/render-query/render-query.js |
21.28 MiB | 🔺 +1.8 KiB (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.24 MiB · 22 files | 🔺 +330 B (+0.0%) | ███░░░░░░░ 27.6% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.09 MiB · 3,014 files | 🔺 +1.8 KiB (+0.0%) | ████████░░ 83.3% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.6 KiB | ../node_modules/.pnpm/posthog-js@1.407.5/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.5/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.2 KiB | src/lib/api.ts |
| 94.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.18 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.18 MiB · 17 files | 🔺 +330 B (+0.0%) | ████░░░░░░ 38.2% of 5.72 MiB |
| Deferred (lazy) | 2.07 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 717.2 KiB | dist/toolbar/toolbar-app-JFACJRNE.css |
| 546.0 KiB | dist/toolbar/chunk-chunk-ZPQNFZXD.js |
| 484.3 KiB | dist/toolbar/chunk-chunk-SMT7UAIH.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-V2BZAD6S.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-SJJ5SQBP.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-5LXGT3CN.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-EK265TQY.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +112.2 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1363.16 MiB · 🔺 +112.2 KiB (+0.0%)
84f5997 to
6e08e76
Compare
|
/trunk merge |
Master landed the merge-queue rollout (#74065, #74367) and the impacted-targets upload (#74266) in the meantime, so: - trunk-impacted-targets.yml now takes master's version wholesale; it supersedes this branch's standalone draft with a compute/upload split and real lane computation. - Kept master's removal of the labeled/unlabeled triggers on top of this branch's removal of ready_for_review. - Kept master's hogli-lint not-a-required-gate markers and the non-pytest-Python wording in ci-mcp.
Note
Stacked on #74065. Review that one first, this PR's base is
chore/merging-prs-skill.Problem
#74065 wires up the impacted-targets upload, but it reports
"ALL"for every PR."ALL"overlaps every other PR's lane, so the parallel merge queue stays effectively serial. That was the right starting point (it can never under-report), but it buys no parallelism.Trunk assigns two PRs to parallel lanes only when their target sets are disjoint. Lanes aren't configured as a list, they emerge from the target names we upload. So the real task is designing a target vocabulary plus the path to target mapping that produces it.
Changes
.github/scripts/trunk-impacted-targets.jsmaps a PR's changed files onto lane targets. The workflow now computes the list instead of hardcoding"ALL".The target vocabulary:
py:core,py:product:<name>backend:contract-checkfe:core,fe:product:<name>rust:crate:<name>node:ingestion,svc:<name>nodejs/,services/docs,agents.agents/Two design points worth review:
Targets come from the dependency graphs we already maintain, not a hand-written path table. A hand-maintained table is a second copy of the graph that drifts silently, and its failure mode is a broken master. So this reuses
turbo-discover's tach parser and itsbackend:contract-checkisolation signal.The
ci-*.ymlpath filters are deliberately not used as the source. They're tuned to over-run tests, where an over-broad match only wastes runner minutes.ci-frontend.yml'sfrontendfilter matches**/*.mdand**/*.yaml, so deriving lanes from it would put nearly every PR back in one lane while looking parallel.Everything is biased toward over-reporting, because reporting extra targets only costs parallelism while reporting too few merges conflicting PRs side by side. Unrecognized paths, unparseable graphs, and a missing merge base all fall back to
"ALL".Measured against the current tree:
products/metrics/backend/api.pyproducts/data_warehouse/frontend/Foo.tsxrust/capture/src/main.rscapture-logs)rust/common/types/src/lib.rsproducts/data_warehouse/backend/api.pyproducts/surveys/backend/api.pypnpm-lock.yamlALL(tripwire)terraform/main.tfALL(unmapped, fails safe)Sizing note: of the last 400 commits on master, 273 (68%) touch a single coarse domain, so most PRs are single-lane candidates.
Warning
Accepted risk:
fe:coreandpy:coreare disjoint, so a frontend PR and a backend PR can merge in parallel. The E2E suite exercises both together and runs on either kind of change, so a combination neither PR's own run covered can still break master. This is inherent to parallel queues rather than specific to these rules. If it shows up in practice, the knob is ane2etarget emitted for every change matchingci-e2e-playwright.yml's trigger paths, which puts all such PRs back in one lane. Documented in the script header.How did you test this code?
Added
.github/scripts/trunk-impacted-targets.test.js(18 cases, registered inci-scripts.yml). Synthetic products, crates, and graphs throughout, so the tests don't break when a product or crate is added.The regressions each group catches, none of which an existing test covered (this is new code):
ALL. The one failure mode that silently breaks master: an unclaimed path yielding an empty target set reads to Trunk as "overlaps nothing", so the PR merges in parallel with everything.ALL, including when mixed with otherwise-narrow files. Drop a tripwire and conflicting lockfile bumps merge in parallel.prost14 = { package = "prost" }shape, see below.Two bugs the smoke run against the real tree caught, both now covered:
prost14 = { package = "prost" }), which killed the whole graph and collapsed every rust PR toALL. It now resolves through the rename and only adds an edge when the real crate is in-workspace.Also ran:
hogli lint:workflows(6/6 across 106 workflows),hogli ci:preflight --strict(0 failures), and verified the jq payload construction produces valid requests for both the"ALL"and array shapes against the documented API schema.I could not run
actionlint(not on PATH in this environment), and I have not exercised this against a live Trunk queue. The first real signal will be the upload logs on this PR.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5) wrote this at my direction. I asked it to research how Trunk lanes work and recommend a set for this repo, then to implement the recommendation.
The research phase changed the design. My initial framing was "recommend a collection of lanes", but lanes are emergent from uploaded target names rather than configured, which reframes the whole thing as a safety problem: disjoint targets mean two PRs merge without ever being tested together. The TODO left in #74065 suggested deriving targets from
ci-paths-filter.yml; that got rejected once we looked at what those filters actually match. Findingturbo-discover.jsandrust/affected-serviceswas the turning point, since the repo already maintains the graphs needed and already fails safe to "test everything".Rust crate granularity was the main open call. Per-crate lanes need a dependency graph, and the guppy determinator that would give it properly needs a cargo build we don't want in a 5-minute job. Settled on parsing the Cargo manifests statically, degrading to "every rust crate" rather than to
ALLwhen that fails, since the whole rust tree is still soundly disjoint from py and fe.Skills invoked:
/authoring-ci-workflows,/writing-tests,/writing-code-comments.