Stop hashing compiled output into pack:npm - #494
Conversation
Turbo folds a dependency's task hash into its dependents, so the compile:ts and compile:skills edges already key pack:npm to the sources that produce what it packs. Listing the output directory on top of that also keyed it to whatever a prior run left on disk, so one source state hashed one way in a warm tree and another on a fresh checkout, costing cache hits and buying no correctness. Keying off the dependencies' declared outputs instead, via mode: "dependencyOutputs", defers the hash until those dependencies finish. Turbo then reports an empty hash under --dry=json, which is what turbo-run reads to decide whether CI can skip pnpm install. Dropping the glob leaves that hash knowable. Resolves: #409 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used🪛 LanguageTool.changeset/pack-npm-input-stability.md[style] ~9-~9: ‘on top of that’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT) 🪛 markdownlint-cli2 (0.23.2).changeset/pack-npm-input-stability.md[warning] 5-5: First line in a file should be a top-level heading (MD041, first-line-heading, first-line-h1) 🪛 SkillSpector (2.11.0)packages/cli/skills/gtb-build-pipeline/SKILL.md[error] 141: [AE1] null: Referenced artifact was not completely inspected Remediation: Make the referenced artifact locally available and fully analyzable, or remove the reference. (analysis-evasion (AE1)) 🔇 Additional comments (6)
📝 WalkthroughWalkthrough
Changesnpm input stability
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The packaging cache-input change is consistently reflected across configuration, validation, and documentation, with no actionable current-head risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/cli/src/lib/turbo-config.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/cli/test/task-graph.test.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). packages/cli/test/turbo-json-dist-source.test.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
pack:npmlisteddist/source/**, minus the four files it writes itself, among its inputs. Turbo folds a dependency's task hash into its dependents, so thecompile:tsandcompile:skillsedges already keypack:npmto the sources that produce what it packs. The glob was redundant for correctness and harmful for caching: it also keyed the task to whatever a prior run left on disk, so one source state hashed one way in a warm tree and another on a fresh checkout.Dropping the glob and its negations leaves the dependency edges to do the keying.
Testing
pnpm check(44/44) andgtb verifyboth pass, but neither can show cache behavior. Measured by hand against turbo 2.10.12:packages/eslint-plugin-yamllint/dist/source, previously movedpack:npm's hash. It now holds atf880a249f99f4a5b, and the hash is still non-empty, soturbo-run's dry-run install-skip keeps working.Notes for reviewers
The premise of #409 was that the plain glob let
pack:npmhit and restore a stale tarball. It doesn't, and neither failure mode it predicts reproduces, because the dependency hash covers both. The real cost was spurious misses, which is the opposite problem.That matters for the fix chosen. #409 proposed
mode: "dependencyOutputs"and was split out specifically to decide the CI trade-off that mode forces: it defers the hash until dependencies finish, turbo then reports an empty hash under--dry=json, andturbo-runwould install on everybuild:ci,build, andtest:e2ejob. I confirmed that cost is real. Dropping the glob avoids it, so there is no trade-off left to accept.The reasoning is recorded in the
gtb-build-pipelineownership reference so the glob doesn't return as an improvement.Resolves: #409