diff --git a/.github/scripts/trunk-impacted-targets.js b/.github/scripts/trunk-impacted-targets.js index 7502f0a861bb..7df9c9dcd64e 100644 --- a/.github/scripts/trunk-impacted-targets.js +++ b/.github/scripts/trunk-impacted-targets.js @@ -284,6 +284,180 @@ function listIsolatedProducts(repoRoot, products) { return isolated } +// --- Nested JS workspaces --- + +// A product that vendors its own pnpm workspace (products/desktop is the one +// today) has an apps/ + packages/ layout instead of the backend/ + frontend/ +// split the product rules assume. Its manifests, configs, and assets are none +// of .py, backend/, or .tsx, so they land in the "could be either" bucket and +// widen to every backend lane, which is how a package.json under packages/ +// serializes a TypeScript-only PR against all of Python. +// +// The workspace file is the product's own declaration of which subtrees are JS +// packages, so it is a safer signal than an extension allowlist: a path only +// narrows when the product itself says a JS package lives there. Anything +// outside those subtrees (the product's root manifests, scripts/, backend/) +// keeps the old widening behavior. +const WORKSPACE_DECLARATION = 'pnpm-workspace.yaml' + +// pnpm's own two files at the product root. A pnpm-workspace.yaml makes that +// directory a workspace root rather than a member of the repo-root one, so the +// lockfile beside it resolves that workspace's packages and nothing else. The +// repo-root lockfile is a separate file and stays a tripwire in its own right. +// Neither of these is importable from Python, and neither is a contract +// declaration, so without this rule they fall through the layout checks below +// and claim every backend lane: a desktop dependency bump lands in the same +// lane as all of Python. +// +// The self-gating hazard that keeps CONTRACT_DECLARATIONS widening does not +// transfer here. turbo.json and package.json declare a Python import surface, +// so a PR that narrows one and edits under it in the same commit would gate +// itself against its own new contract. This pair declares no Python surface, +// and the .py carve-out below applies whatever the globs say. +const WORKSPACE_OWN_FILES = [WORKSPACE_DECLARATION, 'pnpm-lock.yaml'] + +// Minimal reader for the `packages:` block of a pnpm workspace file. Only the +// list-of-globs form is understood; anything else yields no globs, which leaves +// the product on the old behavior rather than guessing. +function parseWorkspacePackageGlobs(text) { + const globs = [] + let inPackages = false + for (const rawLine of text.split('\n')) { + const line = rawLine.replace(/#.*$/, '').trimEnd() + if (!line.trim()) { + continue + } + if (/^packages:\s*$/.test(line)) { + inPackages = true + continue + } + if (!inPackages) { + continue + } + const item = line.match(/^\s+-\s+(.+)$/) + if (!item) { + break + } + globs.push(item[1].trim().replace(/^['"]|['"]$/g, '')) + } + return globs +} + +function compileWorkspaceMatcher(globs) { + const include = [] + const exclude = [] + for (const glob of globs) { + const negated = glob.startsWith('!') + const matcher = globToRegExp(negated ? glob.slice(1) : glob) + ;(negated ? exclude : include).push(matcher) + } + if (include.length === 0) { + return null + } + // 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 + } + } + return false + } +} + +function loadProductWorkspaces(repoRoot, products) { + const workspaces = new Map() + for (const product of products) { + const declaration = path.join(repoRoot, 'products', product, WORKSPACE_DECLARATION) + if (!fs.existsSync(declaration)) { + continue + } + let matcher + try { + matcher = compileWorkspaceMatcher(parseWorkspacePackageGlobs(fs.readFileSync(declaration, 'utf8'))) + } catch (error) { + console.error( + `Could not read products/${product}/${WORKSPACE_DECLARATION} (${error.message}); its files keep widening to every backend lane` + ) + continue + } + if (matcher) { + workspaces.set(product, matcher) + } + } + return workspaces +} + +function isInProductWorkspace(product, file, productWorkspaces) { + const matcher = productWorkspaces.get(product) + if (!matcher) { + return false + } + const relativePath = file.slice(`products/${product}/`.length) + return WORKSPACE_OWN_FILES.includes(relativePath) || matcher(relativePath) +} + +// --- Backend-detached products --- + +// The narrowing above stops at the layout rules: a product with a vendored +// workspace still owns every backend lane the moment one of its files reads as +// backend, because the product rules assume every product is a Django product +// whose Python some other product may import. products/desktop is not one. It +// is a standalone app imported from another repository, with no manifest.tsx, +// no backend/, no entry in frontend/src/products.json, and its own desktop-* +// CI. The Python it does carry is a vendored copy of that repository's own +// tooling under tools/, which this repository's suites never load. +// +// Two enforced declarations say so, and both have to hold: +// +// 1. pytest.ini ignores the subtree, so no backend test collects a single +// file under it. ci-backend.yml carries the same exclusion in its path +// filter, but a filter tuned to over-run is not a safe source for lane +// assignment, while an --ignore is a statement that the suite does not +// cover the path at all. +// 2. The product is absent from tach.toml, the enforced Python module graph, +// so no declared module may import it. +// +// A product satisfying both cannot fail another product's backend suite, so +// its files claim its own lanes instead of all of them. Either condition +// missing keeps the old widening, and so does an unreadable pytest.ini or an +// unavailable tach graph. Both declarations are already tripwires, so a PR +// that detaches a product cannot itself run beside anything. +const PYTEST_CONFIG = 'pytest.ini' + +// 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(/\/+$/, '')) +} + +function loadBackendDetachedProducts(repoRoot, products, tachGraph) { + if (!tachGraph) { + return new Set() + } + let ignored + try { + ignored = new Set(parsePytestIgnores(fs.readFileSync(path.join(repoRoot, PYTEST_CONFIG), 'utf8'))) + } catch (error) { + console.error(`Could not read ${PYTEST_CONFIG} (${error.message}); every product widens to all backend lanes`) + return new Set() + } + const detached = new Set() + for (const product of products) { + // tach spells its modules both ways across the file, so a product + // counts as declared under either spelling. + const declared = tachGraph.graph.has(product) || tachGraph.graph.has(product.replace(/_/g, '-')) + if (ignored.has(`products/${product}`) && !declared) { + detached.add(product) + } + } + return detached +} + // --- Contract surfaces --- const CONTRACT_TASK = 'backend:contract-check' @@ -593,7 +767,15 @@ const feProduct = (product) => `fe:product:${product}` const rustCrate = (crate) => `rust:crate:${crate}` function computeTargets(changedFiles, context) { - const { products, isolatedProducts, rustGraph, tachGraph, contractSurfaces = new Map() } = context + const { + products, + isolatedProducts, + rustGraph, + tachGraph, + contractSurfaces = new Map(), + productWorkspaces = new Map(), + backendDetachedProducts = new Set(), + } = context const targets = new Set() const allPyProducts = () => { @@ -735,16 +917,25 @@ function computeTargets(changedFiles, context) { } const isBackend = segments[2] === 'backend' || file.endsWith('.py') const isFrontend = segments[2] === 'frontend' || /\.tsx?$/.test(file) + // Only reached for a file that is neither, and only inside a + // package the product's own pnpm workspace declares. A .py there + // is still backend: the workspace says the directory holds a JS + // package, not that Python cannot be checked into it. + const isWorkspaceOnly = !isBackend && !isFrontend && isInProductWorkspace(product, file, productWorkspaces) if (isFrontend || (!isBackend && !isFrontend)) { targets.add(feProduct(product)) } - if (isBackend || (!isBackend && !isFrontend)) { + if (isBackend || (!isBackend && !isFrontend && !isWorkspaceOnly)) { if (isolatedProducts.has(product)) { targets.add(pyProduct(product)) if (touchesContractSurface(product, file, contractSurfaces)) { changedIsolatedProducts.add(product) } + } else if (backendDetachedProducts.has(product)) { + // No backend suite covers this product and no declared + // module imports it, so the lane it keeps is its own. + targets.add(pyProduct(product)) } else { allPyProducts() } @@ -849,12 +1040,15 @@ function loadTachGraph(repoRoot) { function buildContext(repoRoot) { const products = listProducts(repoRoot) + const tachGraph = loadTachGraph(repoRoot) return { products, isolatedProducts: listIsolatedProducts(repoRoot, products), contractSurfaces: loadContractSurfaces(repoRoot, products), + productWorkspaces: loadProductWorkspaces(repoRoot, products), + backendDetachedProducts: loadBackendDetachedProducts(repoRoot, products, tachGraph), rustGraph: loadRustGraph(repoRoot), - tachGraph: loadTachGraph(repoRoot), + tachGraph, } } @@ -862,9 +1056,12 @@ module.exports = { computeTargets, buildContext, compileContractMatcher, + compileWorkspaceMatcher, globToRegExp, isProductDirectory, isTripwire, + parsePytestIgnores, + parseWorkspacePackageGlobs, parseCrateDependencies, parseCrateName, reverseClosure, diff --git a/.github/scripts/trunk-impacted-targets.test.js b/.github/scripts/trunk-impacted-targets.test.js index 1e308816389c..d55f976b8f6b 100644 --- a/.github/scripts/trunk-impacted-targets.test.js +++ b/.github/scripts/trunk-impacted-targets.test.js @@ -13,10 +13,13 @@ const assert = require('node:assert/strict') const { computeTargets, compileContractMatcher, + compileWorkspaceMatcher, globToRegExp, isProductDirectory, isTripwire, parseCrateDependencies, + parsePytestIgnores, + parseWorkspacePackageGlobs, reverseClosure, ALL, } = require('./trunk-impacted-targets') @@ -44,6 +47,13 @@ const CONTEXT = { }, } +// gamma vendors its own pnpm workspace; alpha and beta keep the conventional +// backend/ + frontend/ layout, so the cases above stay on the old behavior. +const WORKSPACE_CONTEXT = { + ...CONTEXT, + productWorkspaces: new Map([['gamma', compileWorkspaceMatcher(['apps/*', 'packages/*', 'tooling/*'])]]), +} + test('every tripwire forces ALL', () => { const tripwireFiles = [ 'pnpm-lock.yaml', @@ -395,6 +405,114 @@ test('a product file that is neither backend nor frontend claims both domains', assert.equal(targets.includes('fe:product:beta'), true) }) +// A product vendoring its own pnpm workspace has no backend/ + frontend/ split, +// so its manifests and configs land in the "claims both domains" case above and +// drag every backend lane along. Narrowing them is the whole point of reading +// the workspace declaration. +test('a file inside a declared workspace package claims only the product lane', () => { + for (const file of [ + 'products/gamma/packages/agent/package.json', + 'products/gamma/apps/code/snapshots.yml', + 'products/gamma/tooling/config/biome.json', + 'products/gamma/apps/code/assets/icon.svg', + ]) { + assert.deepEqual(computeTargets([file], WORKSPACE_CONTEXT), ['fe:product:gamma'], file) + } +}) + +// The narrowing direction is the dangerous one: a backend lane that stops being +// claimed lets Trunk run this PR beside a conflicting backend PR. The workspace +// declaration says a directory holds a JS package, not that Python cannot be +// checked into it. +test('python inside a declared workspace package still claims the backend lanes', () => { + const targets = computeTargets(['products/gamma/packages/agent/scripts/codegen.py'], WORKSPACE_CONTEXT) + assert.equal(targets.includes('py:core'), true) +}) + +// Only the declared package subtrees narrow. The product root holds the files +// that decide isolation and contract surface, and anything else under the +// product is unclassified in the same way it was before. +test('files outside the declared workspace packages keep widening', () => { + for (const file of ['products/gamma/package.json', 'products/gamma/scripts/release.mjs']) { + assert.equal(computeTargets([file], WORKSPACE_CONTEXT).includes('py:core'), true, file) + } +}) + +// The workspace declaration and its lockfile sit at the product root, so the +// glob matcher alone leaves them in the "claims both domains" case and a +// dependency bump in the vendored workspace still claims every backend lane. +// The second assertion is the boundary: a product with no declaration keeps +// the old widening, which a basename-only version of this rule would lose. +test('the vendored workspace files claim only the product lane', () => { + for (const file of ['products/gamma/pnpm-workspace.yaml', 'products/gamma/pnpm-lock.yaml']) { + assert.deepEqual(computeTargets([file], WORKSPACE_CONTEXT), ['fe:product:gamma'], file) + } + assert.equal( + computeTargets(['products/alpha/pnpm-lock.yaml'], WORKSPACE_CONTEXT).includes('py:product:alpha'), + true + ) +}) + +// delta stands in for products/desktop: an app imported from another +// repository that pytest.ini ignores and tach.toml never declares. Its +// vendored .py files read as backend to the layout rules, so without the +// detachment check they claim every backend lane for suites that never run on +// them. +const DETACHED_CONTEXT = { + ...WORKSPACE_CONTEXT, + products: [...CONTEXT.products, 'delta'], + backendDetachedProducts: new Set(['delta']), +} + +test('a backend-detached product keeps its own lane instead of every backend lane', () => { + assert.deepEqual(computeTargets(['products/delta/tools/agent/policy.py'], DETACHED_CONTEXT), ['py:product:delta']) + assert.deepEqual(computeTargets(['products/delta/biome.json'], DETACHED_CONTEXT), [ + 'fe:product:delta', + 'py:product:delta', + ]) + // gamma is ignored by neither declaration, so the same shapes still widen. + assert.equal(computeTargets(['products/gamma/tools/agent/policy.py'], DETACHED_CONTEXT).includes('py:core'), true) +}) + +// pytest.ini spells the list inside one long addopts line, so a reader anchored +// to the start of a line finds nothing and silently leaves every product +// widening. +test('pytest ignores are read from anywhere in addopts', () => { + assert.deepEqual( + parsePytestIgnores('addopts = -p no:warnings --ignore=tools/hogli --ignore=products/desktop --reuse-db'), + ['tools/hogli', 'products/desktop'] + ) +}) + +// A real pnpm-workspace.yaml carries a catalog: block right after packages:, +// and reading past the list would turn catalog entries into package globs. +test('workspace globs are read only from the packages block', () => { + assert.deepEqual( + parseWorkspacePackageGlobs( + [ + 'packages:', + " - 'apps/*'", + ' - packages/*', + ' - "!packages/legacy"', + '', + 'catalog:', + ' hono: ^1.0.0', + ].join('\n') + ), + ['apps/*', 'packages/*', '!packages/legacy'] + ) +}) + +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) +}) + +test('a workspace declaration with no packages block yields no matcher', () => { + assert.equal(compileWorkspaceMatcher(parseWorkspacePackageGlobs('catalog:\n hono: ^1.0.0\n')), null) +}) + // tools/ is not one bucket. phrocs is Go with its own CI and nothing imports // it, while hogli-commands is loaded by posthog/conftest.py on every pytest // run, so lumping them together either serializes phrocs needlessly or hands