perf(ci): keep JS-only products out of the backend merge-queue lanes - #76481
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Reviews (1): Last reviewed commit: "perf(ci): narrow merge-queue lanes for p..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR optimizes Trunk merge-queue target selection for products that vendor a nested pnpm workspace (e.g. products/desktop), so changes within declared JS package subtrees no longer unnecessarily claim backend (Python) lanes.
Changes:
- Add support for reading
pnpm-workspace.yamlpackages:globs per product and compiling them into a workspace matcher. - Update product file classification so “unclassified” files inside declared workspace packages narrow to the product’s frontend lane (while
.pystill claims backend lanes). - Extend the unit test suite with new cases covering narrowing behavior, negated globs, and parsing boundaries around
catalog:.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| .github/scripts/trunk-impacted-targets.js | Loads per-product pnpm workspace declarations and uses them to avoid widening backend lanes for files inside declared JS package subtrees. |
| .github/scripts/trunk-impacted-targets.test.js | Adds focused tests to prevent regressions in workspace parsing and lane narrowing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bd2986f to
c9a2289
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/scripts/trunk-impacted-targets.js:360
- compileWorkspaceMatcher can fail to apply common negated globs like
!packages/legacy/**. Because globToRegExp anchors the pattern,packages/legacy/**won't match the ancestor dirpackages/legacy(no trailing slash), so excluded package roots can still be treated as “in workspace” and incorrectly narrow targets.
// The globs name package directories, so a file is inside the workspace
// when one of its ancestor directories matches. Testing the file path
// itself would miss everything below the package root.
return (relativePath) => {
const segments = relativePath.split('/')
for (let depth = 1; depth < segments.length; depth++) {
const dir = segments.slice(0, depth).join('/')
if (include.some((re) => re.test(dir)) && !exclude.some((re) => re.test(dir))) {
return true
.github/scripts/trunk-impacted-targets.js:427
- parsePytestIgnores scans the entire pytest.ini text, including comment lines and non-addopts settings. A commented-out
--ignore=...(or another config field containing that substring) could accidentally mark a product as backend-detached, which is the risky narrowing direction. Consider restricting parsing to theaddoptsvalue and stripping ini comments first.
// Reads the --ignore paths out of pytest's addopts. Nothing matching yields an
// empty list, which leaves every product on the old widening.
function parsePytestIgnores(text) {
return [...text.matchAll(/--ignore[= ](\S+)/g)].map((match) => match[1].replace(/\/+$/, ''))
}
.github/scripts/trunk-impacted-targets.test.js:510
- The negated-glob test only covers
!packages/legacybut not the more common pnpm form!packages/legacy/**(which currently won’t exclude the package root unless the matcher accounts for trailing slashes). Adding an assertion for the/**form will lock in the intended behavior and prevent accidental narrowing.
test('a negated workspace glob excludes its subtree from the narrowing', () => {
const matcher = compileWorkspaceMatcher(['packages/*', '!packages/legacy'])
assert.equal(matcher('packages/agent/package.json'), true)
assert.equal(matcher('packages/legacy/package.json'), false)
})
c9a2289 to
eafdd59
Compare
eafdd59 to
cda04c1
Compare
…aces products/desktop vendors its own pnpm workspace, so its manifests, configs, and assets are neither .py nor .tsx nor under backend/. They fell into the "could be either domain" case and claimed every backend lane, serializing TypeScript-only PRs against all of Python. Files inside a subtree the product's own pnpm-workspace.yaml declares as a package now claim only the product's frontend lane. Everything else, including the product root manifests and any .py under a package, keeps widening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anes products/desktop/pnpm-workspace.yaml and products/desktop/pnpm-lock.yaml sit at the product root, outside every package glob the declaration lists, so the narrowing did not reach them and each still claimed all 82 backend lanes. A dependency bump in the vendored workspace serialized against every Python PR in the queue. A pnpm-workspace.yaml makes its directory a workspace root rather than a member of the repo-root one, so the lockfile beside it resolves only that workspace's packages. Neither file is importable from Python. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
products/desktop is an app imported from another repository. pytest.ini ignores the subtree, ci-backend.yml excludes it from its path filter, tach.toml never declares it, and no Python here imports it. Its vendored .py files under tools/ and every config at its root still read as backend to the layout rules and claimed all 82 backend lanes, so either one serialized against every Python PR in the queue. A product that pytest ignores and tach does not declare now claims its own two lanes instead of all of them. Both declarations are already tripwires, so a PR that detaches a product cannot itself run beside anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cda04c1 to
0ff22f3
Compare
|
This pull request was merged into |
Problem
products/desktopis an app imported from another repository. It vendors its own pnpm workspace, so it has anapps/+packages/layout instead of thebackend/+frontend/split the lane rules assume, and the product classifier keys off extension and directory:A
package.jsonunderpackages/, asnapshots.ymlunderapps/, an icon, a font, the two pnpm files at the product root,postcss.config.mjs, the 32 vendored.pyfiles undertools/: every one of them claimed all 82 Python lanes. That is 889 of desktop's 4,806 tracked files, 18% of the tree, each serializing against every backend PR in the queue.Measured against the open PRs touching
products/desktopwhen this was written, 3 of 14 were widened purely by this, with a TypeScript-only footprint otherwise:products/desktop/packages/agent/package.jsonproducts/desktop/apps/code/snapshots.ymlproducts/desktop/packages/harness/package.jsonReplaying the last 500 merged PRs finds two more: #76339 (a 188-file desktop resync) and #76340, both at 83 lanes for a change no Python suite runs on.
Changes
Two rules, each keyed off a declaration the repo already enforces.
1. A file inside a subtree the product's own
pnpm-workspace.yamldeclares as a package is frontend-only, along with the two files pnpm itself owns at the workspace root (pnpm-workspace.yaml,pnpm-lock.yaml). The declaration is the signal rather than an extension allowlist, so a path narrows only where the product itself says a JS package lives. Apnpm-workspace.yamlmakes its directory a workspace root rather than a member of the repo-root one (the root file excludesproducts/desktopexplicitly), so the lockfile beside it resolves that workspace's packages and nothing else. The repo-rootpnpm-lock.yamlstays a tripwire in its own right.2. A product with no backend surface claims its own lane instead of all of them. Two enforced declarations have to hold together:
pytest.iniignores the subtree (--ignore=products/desktop), so no backend test collects a file under it.ci-backend.ymlcarries the same exclusion in its path filter, but a filter tuned to over-run is not a safe source for lane assignment, while an--ignorestates the suite does not cover the path at all.tach.toml, the enforced Python module graph, so no declared module may import it.products/desktopis the only product satisfying both today. It also has nomanifest.tsx, nobackend/, and no entry infrontend/src/products.json, and nothing in this repo's Python imports it. Its.pyfiles are a vendored copy of the upstream repository's own tooling.What deliberately keeps the old widening:
.pyinside a declared workspace package of a product that is not backend-detached. The workspace says a directory holds a JS package, not that Python cannot be checked into it.products/<p>/package.jsonandturbo.json, which decide isolation and contract surface, soCONTRACT_DECLARATIONShas to keep seeing them.pnpm-workspace.yaml, an unparseable one, an unreadablepytest.ini, or an unavailable tach graph all leave the product exactly where it was.Note
Narrowing is the dangerous direction here: a backend lane that stops being claimed lets Trunk run two conflicting PRs in parallel. Both rules are gated on declarations that CI enforces, and
pytest.ini,tach.toml, and the repo-rootpnpm-lock.yamlare all tripwires, so a PR that changes what these rules read cannot itself run beside anything.How did you test this code?
node --test .github/scripts/trunk-impacted-targets.test.js— 45 pass, 9 new.The new cases each guard a distinct regression rather than restating the implementation: the workspace narrowing itself; the two root pnpm files, plus the boundary that a product without a declaration keeps widening (which a basename-only rule would lose);
.pyinside a declared package still claiming backend lanes for a product that has one (the unsafe direction); files outside the declared globs still widening; the detached product keeping its own lane while a non-detached product with the same file shapes still widens; and parser cases for both readers, sincepnpm-workspace.yamlcarries acatalog:block right afterpackages:that a looser reader would swallow, andpytest.inispells its ignores inside one longaddoptsline where a line-anchored reader finds nothing.Whole-tree differential against master. Both versions run over every tracked file under
products/, target sets diffed:No product other than desktop changes by a single file, and nothing widens anywhere. Every desktop file that claimed
py:coreunder master now claims onlyfe:product:desktop/py:product:desktop; the count of desktop files still reaching a backend lane is zero. #76407, which genuinely touchesposthog/api/comments.pyalongside desktop files, still claims all 86 lanes.Against the last 500 merged PRs. Replaying each PR's file list through both versions, two move: #76339 from 83 lanes to 1, and #76340 from 83 to 2. No PR gains a lane.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
Not applicable. The rationale lives in the script's own header comments, which this PR extends.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
This started as asking Claude (Claude Code) whether the existing fallback was too aggressive. It measured the open desktop PRs first, and the answer moved from "probably" to a specific number, which is what shaped the first rule. The second came from replaying the last 500 merged PRs and asking why desktop was still widening at all, which surfaced that the product has no backend surface in this repository whatsoever.
Decisions worth a reviewer's eye:
.json/.yml/asset extensions toisFrontend. Rejected: a product could keep a Python-consumed YAML at its root outsidebackend/, and that fix would silently narrow it. Both rules here key off a declaration instead, so neither can reach a product that has not made the statement.--ignorealone would narrow a product whose Python another product still imports; requiring absence fromtach.tomlcloses that.products/desktop/package.jsonandturbo.jsonstill widen, because those feedCONTRACT_DECLARATIONSand untangling the two is a bigger change than this should be. They are the only desktop paths left that reach beyond the product's own lanes, and rule 2 means they no longer reach the backend ones.Earlier revisions of this description quoted 87 products and 88 lanes. That came from a local tree where six stale directories under
products/(build artifacts left by deleted products) were read as products. The numbers here are from a clean checkout: 81 products, so 82 backend lanes./writing-testsand/writing-code-commentsinvoked before the test and comment edits.