chore(ci): ignore tool caches when listing products for lane names - #76610
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: "chore(ci): ignore tool caches when listi..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Hardens the merge-queue lane target script so local runs don’t invent extra “product” lanes from untracked tool cache directories that can exist alongside real products under products/.
Changes:
- Add
isProductDirectory()and use it inlistProducts()to ignore directories starting with.or__, plusnode_modules. - Add a unit test asserting common cache directory names are rejected while real product names are accepted.
- Minor formatting change in
compileContractMatcher(no functional change intended).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/scripts/trunk-impacted-targets.js | Filters out cache-like directories when enumerating products; exports helper for test coverage. |
| .github/scripts/trunk-impacted-targets.test.js | Adds regression test to ensure cache directories are not treated as products. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ruff and pytest leave .ruff_cache and __pycache__ next to the products, so a local run of the target script invented a lane for each. CI is unaffected: a fresh checkout holds only tracked directories and the compute job runs no Python. This keeps a local run answering the same as CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f215ebb to
82588e2
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
This pull request was merged into |
…76610) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Problem
listProductstreats every directory underproducts/as a product, so a local run of the target script picks up the caches that sit beside them and invents a lane for each:Nothing downstream rejects a nonsense target name, so
py:product:.ruff_cacheandpy:product:__pycache__end up in the target set and every widened change reports two lanes more than it should.Note
CI is not affected, so this is hardening rather than a bug fix. A fresh checkout contains only tracked directories, both of these are untracked (
git ls-files products/.ruff_cacheis empty), and thecomputejob runs no Python — it checks out, fetches, sets up Node, and runs the script. I checked that before writing the fix, because the fix would otherwise have been justified on a false premise.What it does cost is that a local run disagrees with CI, which matters because the script is the thing you run by hand to reason about lane behavior. Numbers I gathered locally while working on #76481 and #76483 were inflated by these two entries. The comparisons in those PRs are unaffected — both sides of every diff used the same context — but the absolute lane counts quoted there run about two high.
Changes
Skip directories whose names start with
.or__, plusnode_modules.A denylist rather than a marker-file allowlist on purpose. Missing a new kind of junk leaves a harmless extra lane that overlaps consistently; excluding a real product would send all of that product's PRs to
ALL. Given the two failure modes, the cheap one is the right one to risk.The filter is also safe in the direction it can be wrong: an unrecognized product name falls through to
ALL, so a product that this wrongly dropped would widen rather than narrow.How did you test this code?
node --test .github/scripts/trunk-impacted-targets.test.js— 37 pass, 1 new.The new case asserts the four cache names are rejected and three real products accepted. It earns its place because the symptom is silent: no consumer validates target names, so the only way this surfaces is someone puzzling over a local run that disagrees with CI.
Also re-ran
buildContextagainst the working tree before and after, which is where the 87 → 85 numbers above come from.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
Not applicable.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Spotted by Claude (Claude Code) while dry-running the telemetry payload in #76593 and flagged there as a follow-up; this is that follow-up.
Branched off master rather than stacked. It touches
listProducts, while #76481 and #76483 touch the workspace and product-classification code further down the same file, so the hunks don't overlap.Worth a reviewer knowing: four more directories under
products/have no tracked files on master either (deployments,desktop_recordings,discord_app,query_performance_ai). Those are indistinguishable from real products by name, they don't exist in a CI checkout, and they look like leftovers from other branches in a local tree — so this filter deliberately does not try to catch them./writing-testsinvoked before adding the test case.