fix: exempt dependency manifests from sharedGlobals global invalidation#76
fix: exempt dependency manifests from sharedGlobals global invalidation#76ocombe wants to merge 5 commits into
Conversation
A lockfile or package.json listed in an nx.json `sharedGlobals` namedInput short-circuited `find_affected` to "all projects" before the lockfile analyzer ran (core.rs Step 1b returns early on any global trigger). That made `--lockfile-strategy` dead code for exactly the files it exists for: the source/asset partition already excludes the lockfile as "handled separately", but the global-invalidation gate was never taught the same. Exempt the detected package manager's lockfile and the workspace-root package.json (they change together on a dependency update) from global invalidation, so the lockfile + semantic pipeline computes the real affected set instead. Any other global trigger (.nvmrc, nx.json, etc.) still invalidates every project.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesDependency manifest exemption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Core as find_affected_internal
participant Lockfile as lockfile::is_dependency_manifest
participant Analysis as lockfile analysis
Core->>Core: detect package manager and lockfile status
Core->>Lockfile: classify global trigger
Lockfile-->>Core: dependency-manifest result
Core->>Core: filter global triggers
Core->>Analysis: reuse lockfile_changed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core.rs (1)
162-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate/conflicting step numbering.
The new comment block repeats "Step 5: Partition changed files..." immediately after the existing "Step 5: Track affected packages..." (line 162), and is then immediately followed by the pre-existing "Step 6: Partition changed files into source and non-source" (line 170) describing the same operation. This creates confusing, redundant step labels.
♻️ Suggested cleanup
// Step 5: Track affected packages and their causes let mut affected_packages = FxHashSet::default(); let mut project_causes: FxHashMap<String, Vec<AffectCause>> = FxHashMap::default(); - // Step 5: Partition changed files into source and non-source (excluding - // lockfiles). `detected_pm` / `lockfile_filename` are computed above so the - // dependency-manifest exemption and this partition share one detection. - - // Step 6: Partition changed files into source and non-source + // Step 6: Partition changed files into source and non-source (excluding + // lockfiles). `detected_pm` / `lockfile_filename` are computed above so the + // dependency-manifest exemption and this partition share one detection. let (source_files, asset_files): (Vec<&ChangedFile>, Vec<&ChangedFile>) = changed_files🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core.rs` around lines 162 - 171, The step labels around the changed-files partitioning are duplicated and conflicting: the new explanatory comment repeats the same partitioning step immediately after the existing affected-packages step, while the following comment already documents the partitioning in the code path using changed_files. Clean this up by keeping a single clear step label and merging the explanatory text near the changed_files partition logic so the numbering and description are consistent.tests/integration_test.rs (1)
3791-3915: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression coverage; consider adding the "manifest-only, no lockfile change" edge case.
Both tests correctly cover the happy path (lockfile bump alone doesn't globally invalidate) and the guard-rail (non-manifest sharedGlobals still invalidates globally). Consider adding a third case where only
package.jsonchanges (dependency edited) without a correspondingpackage-lock.jsonupdate, to lock in behavior for the gap flagged insrc/lockfile.rs'sis_dependency_manifest(package.json is exempted from global invalidation even when the lockfile pipeline has nothing to analyze).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_test.rs` around lines 3791 - 3915, Add a regression test for the manifest-only case: in tests/integration_test.rs alongside test_dependency_manifest_in_shared_globals_does_not_globally_invalidate, create a scenario where package.json changes in sharedGlobals without any package-lock.json update, then assert the change does not globally invalidate unrelated projects while still affecting the project that imports the edited dependency. Use the existing helpers setup_lockfile_test_repo, TrueAffectedConfig, lockfile_projects, and find_affected to mirror the current lockfile tests and cover the gap in src/lockfile.rs around is_dependency_manifest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lockfile.rs`:
- Around line 99-113: The exemption logic in is_dependency_manifest is too broad
because workspace-root package.json is always treated as dependency-manifest
even when no package manager/lockfile path is available. Update the check so
package.json is only exempt from sharedGlobals when the matching lockfile can be
identified and has changed, aligning it with the Step 5c gating in core.rs;
otherwise keep package.json as a global trigger. Use the existing
is_dependency_manifest and lockfile_name helpers to constrain the condition
without changing other manifest handling.
---
Nitpick comments:
In `@src/core.rs`:
- Around line 162-171: The step labels around the changed-files partitioning are
duplicated and conflicting: the new explanatory comment repeats the same
partitioning step immediately after the existing affected-packages step, while
the following comment already documents the partitioning in the code path using
changed_files. Clean this up by keeping a single clear step label and merging
the explanatory text near the changed_files partition logic so the numbering and
description are consistent.
In `@tests/integration_test.rs`:
- Around line 3791-3915: Add a regression test for the manifest-only case: in
tests/integration_test.rs alongside
test_dependency_manifest_in_shared_globals_does_not_globally_invalidate, create
a scenario where package.json changes in sharedGlobals without any
package-lock.json update, then assert the change does not globally invalidate
unrelated projects while still affecting the project that imports the edited
dependency. Use the existing helpers setup_lockfile_test_repo,
TrueAffectedConfig, lockfile_projects, and find_affected to mirror the current
lockfile tests and cover the gap in src/lockfile.rs around
is_dependency_manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8873478-bfda-4648-a9d0-27651349c9df
📒 Files selected for processing (3)
src/core.rssrc/lockfile.rstests/integration_test.rs
Address review feedback: `is_dependency_manifest` exempted the workspace-root package.json from global invalidation unconditionally, but core only runs lockfile analysis when the lockfile actually changed. A package.json-only dependency edit (uninstalled range bump, or a repo with no lockfile) could then skip both the global fallback and the lockfile pipeline, under-including affected projects. Gate the package.json exemption on `lockfile_changed`: the lockfile stays always-exempt, but package.json only when the lockfile changed in the same diff (a real dependency update). A package.json-only change stays a global trigger. Also collapse a duplicate "Step 5"/"Step 6" partition comment and add a regression test for the manifest-only conservative fallback.
|
Thanks @coderabbitai — addressed all three in c8a8e47: 1. 🟠 (Major) 2. 🔵 Duplicate step comment — fixed; collapsed into a single "Step 6: Partition…" comment. 3. 🔵 Manifest-only test — added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git log --oneline -5
git show c8a8e47 --stat 2>/dev/null | head -50Length of output: 1775 🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git show c8a8e47 -- src/lockfile.rs | head -150
echo "-----CORE-----"
git show c8a8e47 -- src/core.rs
echo "-----TEST-----"
git show c8a8e47 -- tests/integration_test.rs | head -100Length of output: 13160 This looks solid — the fix correctly addresses the under-inclusion risk. Nice catch on the edge case — this closes the gap well. Approving. [approve] ᕦ(ò_óˇ)ᕤ |
📦 Preview Release AvailableA preview release has been published for commit 4893366. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-76-4893366/front-ops-domino-1.4.1.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-76-4893366/front-ops-domino-1.4.1.tgz affectedDetails |
shaharkazaz
left a comment
shaharkazaz
left a comment
There was a problem hiding this comment.
LGTM and test on our platform
✅ Validated on a real monorepo (preview release
|
| Binary / strategy | Affected (of 352) | |
|---|---|---|
1.4.0 (registry) |
352 | full-repo fan-out (the bug) |
preview direct (default) |
78 | ✅ narrowed to direct importers |
preview full |
222 | ✅ + transitive re-export chain |
preview none |
0 | ✅ lockfile ignored per docs |
Conservative guardrails (preview binary, single-file working-tree changes):
| Scenario | Affected | Expected |
|---|---|---|
package.json-only edit (no lockfile change) |
352 | ✅ stays global — no under-inclusion (the fix from the review round) |
.nvmrc change |
352 | ✅ still global — exemption stays narrow |
| single source-file edit | 1 | ✅ normal semantic path intact |
So: real dep bumps drop 352 → 78/222 (lockfile analysis now runs instead of the sharedGlobals short-circuit), while package.json-only and non-manifest globals correctly stay conservative. Numbers match the integration tests' intent end-to-end.
| let Some(name) = file_path.to_str() else { | ||
| return false; | ||
| }; | ||
| if pm.is_some_and(|pm| name == lockfile_name(pm)) { |
There was a problem hiding this comment.
the package.json-only guard stays conservative (global), but this exempts the lockfile unconditionally — not gated on strategy. under --lockfile-strategy none, Step 5c never runs, so a lockfile in sharedGlobals now goes all→zero instead of all. that's silently under-including, the exact thing you're guarding against for package.json-only. deliberate? if so worth a none-strategy integration test — all 3 new ones use Direct, and none is the path that changes most.
Address review: the lockfile was exempted from sharedGlobals global invalidation unconditionally, but under `--lockfile-strategy none` the lockfile analysis (Step 5c) never runs. A lockfile listed in sharedGlobals would then be neither globally invalidated nor analyzed, silently dropping all -> 0 — the same under-inclusion the package.json guard prevents. Gate the whole exemption on `lockfile_analysis_enabled` (strategy != none). With analysis off, dependency manifests stay global triggers (conservative fallback). Add a none-strategy integration test covering this path.
|
@shaharkazaz @EladBezalel pushed Could one of you approve the pending Preview Release workflow run for this commit? It's a fork PR so the run is stuck on |
EladBezalel
left a comment
EladBezalel
left a comment
There was a problem hiding this comment.
mechanics look right — single PM detection reused, is_dependency_manifest only matches workspace-root paths, and the analysis-enabled gate avoids the all→0 trap. nicely tested.
one thing to flag clearly: this narrows default (direct) behavior — a manifest in sharedGlobals goes from "all projects" to a subset. i'm fine shipping it because lockfiles not in sharedGlobals already narrow today, so this just makes the two paths consistent rather than adding new under-inclusion risk. but it's a real behavior change — please call it out in the release notes / treat as a minor bump, not a silent patch. don't think the opt-in flag is necessary given the consistency argument, but that's your call.
| // exempted from global invalidation below. | ||
| let detected_pm = lockfile::detect_package_manager(&config.cwd); | ||
| let lockfile_filename = detected_pm.as_ref().map(|pm| lockfile::lockfile_name(pm)); | ||
| let lockfile_changed = detected_pm |
There was a problem hiding this comment.
this lockfile_changed is computed on the unfiltered changed_files, but Step 5c recomputes has_lockfile_changed on the filtered set (after filter_negated_files). if a lockfile were ever negated out of a named input, the exemption here would drop it from global triggers while Step 5c wouldn't analyze it → all→0 for that trigger. very unlikely (lockfile both in sharedGlobals and negated), but it's the exact under-inclusion the gate is meant to prevent. intentional / worth a guard?
There was a problem hiding this comment.
Fixed in 4893366 — Step 5c now reuses the single lockfile_changed computed for the exemption gate instead of recomputing has_lockfile_changed on the filtered set, so the gate and the analysis agree by construction (no divergence possible even if a lockfile were somehow negated). Added no new test since it's a pure single-source-of-truth refactor with identical observable behavior, but the existing lockfile/none-strategy tests still pass.
…-not-global-invalidation
Address review (Elad): the exemption gate computed `lockfile_changed` on the unfiltered `changed_files`, while Step 5c recomputed `has_lockfile_changed` on the post-negation filtered set. If a lockfile were ever negated out of a named input the two could disagree — dropping the lockfile from global triggers while skipping its analysis (all -> 0), the exact under-inclusion the gate prevents. Reuse the single `lockfile_changed` value in Step 5c so the gate and the analysis agree by construction.
|
Pushed
On versioning: agreed this is a behavior change, not a silent patch. There's no release-please here (versions bump via manual
|
3 participants
Problem
When a package lockfile or
package.jsonis listed (transitively, viadefault) in annx.jsonsharedGlobalsnamedInput,find_affectedshort-circuits to all projects before the lockfile analyzer runs — making--lockfile-strategy/lockfileStrategydead code for exactly the files it exists for.The gate lives in
core.rsStep 1b:The source/asset partition already treats the lockfile as "handled separately" (it's excluded at Step 6 because lockfile analysis owns it), but the global-invalidation gate was never taught the same thing. So the two subsystems disagree, and the coarse one wins.
Observed impact (verified on a ~350-project Nx workspace,
@front-ops/domino@1.4.0)--lockfile-strategy directnonepackage.jsonscript-only edit.nvmrc(in sharedGlobals)--lockfile-strategy nonechanging nothing proves the fan-out is thesharedGlobalsshort-circuit, not lockfile logic. With the short-circuit removed, the same dep bump narrows to ~78 (direct) / ~219 (full) — the analyzer was already correct, just unreachable.Fix
Exempt dependency-manifest files from global invalidation so they flow through the existing lockfile + semantic pipeline instead of short-circuiting — but only when that pipeline will actually process them:
--lockfile-strategy!=none).package.jsonis exempt only when analysis is enabled and the lockfile also changed in the same diff — a real dependency update touches both, so lockfile analysis can resolve the affected importers.package.json-only change (uninstalled version-range edit, or a repo with no lockfile), anything under--lockfile-strategy none, and any other global trigger (.nvmrc,nx.json, …).Gating on analysis-enabled avoids a symmetric under-inclusion: without it, a lockfile in
sharedGlobalsundernonewould be neither globally invalidated nor analyzed, silently dropping all → 0.core.rsdetects the package manager once, up front, computeslockfile_changed(has_lockfile_changed) andlockfile_analysis_enabled, and filtersglobal_triggersthroughlockfile::is_dependency_manifest(...). Because the Step 1b short-circuit, the final union, and the report all key offglobal_triggers, filtering at the trigger source fixes all three at once.Semantics note for reviewers
With analysis on (the default
direct), a lockfile — or apackage.jsonaccompanied by a lockfile change — insharedGlobalsis now analyzed instead of fanning out to all projects. This is the intended purpose of the lockfile analyzer; there was never a documented option meaning "invalidate all on lockfile change" — that was incidental tosharedGlobalsmembership. Under--lockfile-strategy none, behavior is unchanged from1.4.0: with no analysis to narrow the set, dependency manifests insharedGlobalsstill globally invalidate. Happy to gate the whole thing behind an explicit opt-in flag if you'd prefer.Tests
lockfile::is_dependency_manifestunit test — lockfile matches the detected PM (not other PMs');package.jsononly withlockfile_changed; never.nvmrcor nestedpackage.json.test_dependency_manifest_in_shared_globals_does_not_globally_invalidate(Direct) — lockfile insharedGlobals+ dep bump → only the importing project, not all.test_manifest_only_change_without_lockfile_still_globally_invalidates(Direct) —package.jsonedited with no lockfile update → globally invalidates (nothing to analyze).test_lockfile_in_shared_globals_with_none_strategy_still_globally_invalidates(None) — lockfile insharedGlobalsundernone→ globally invalidates (all), not 0.test_non_manifest_shared_global_still_globally_invalidates—.nvmrcinsharedGlobalsstill fans out to all projects.cargo fmt,cargo clippy --all-targets,cargo test --lib(208 passed), and thelockfile+named_inputsintegration groups all pass. (The pre-existingtest_asset_chain_with_intermediate_constantshared-fixture test fails identically on cleanmainin a fresh clone — unrelated to this change.)Summary by CodeRabbit