fix(deps): remove bundler .bundle output fallback#9585
fix(deps): remove bundler .bundle output fallback#9585nateberkopec wants to merge 1 commit intojdx:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Greptile SummaryThis PR simplifies
Confidence Score: 3/5Not safe to merge as-is — the fix regresses non-vendored Bundler projects to always re-running A single P1 finding is present: hardcoding src/deps/providers/bundler.rs — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[mise runs deps check] --> B[BundlerDepsProvider outputs]
B --> C[Always returns vendor/bundle]
C --> D{vendor/bundle exists?}
D -->|Yes - vendored setup| E[check_freshness: compare hashes]
E --> F{Gemfile.lock changed?}
F -->|No| G[Fresh: skip bundle install]
F -->|Yes| H[Stale: run bundle install]
D -->|No - system gems| I[OutputsMissing]
I --> J[Always stale: run bundle install every time]
Reviews (1): Last reviewed commit: "fix(deps): remove bundler .bundle output..." | Re-trigger Greptile |
| fn outputs(&self) -> Vec<PathBuf> { | ||
| let root = self.base.config_root(); | ||
| // Check for vendor/bundle if using --path vendor/bundle | ||
| let vendor = root.join("vendor/bundle"); | ||
| if vendor.exists() { | ||
| vec![vendor] | ||
| } else { | ||
| // Use .bundle directory as fallback indicator | ||
| vec![root.join(".bundle")] | ||
| } | ||
| vec![self.base.config_root().join("vendor/bundle")] | ||
| } |
There was a problem hiding this comment.
vendor/bundle absence causes perpetual reinstalls for non-vendored projects
Most Bundler users install gems to the system/user gem path (i.e., without --path vendor/bundle), so vendor/bundle will never exist for them. check_freshness in engine.rs returns OutputsMissing when any output path is absent (line 674), which means bundle install will be triggered on every mise invocation for these projects.
The old .bundle fallback was flawed (.bundle/config is written even before gems install), but the fix over-corrects to a path that only exists for vendored setups. Projects that don't vendor will regress from "occasionally re-running" to "always re-running".
There was a problem hiding this comment.
@greptileai I may need to take a closer look tomorrow but at first glance I do think this is an issue since I don't see anywhere vendor/bundle gets set (unless bundle install is doing that). But it's been a bit since I looked at this code so I can't remember if that's necessary
There was a problem hiding this comment.
I've traced the full flow. Here's what I found:
The concern is confirmed. In engine.rs, check_freshness short-circuits on output existence before hash checking:
// Check if any output is missing
for output in &outputs {
if !output.exists() {
return Ok(FreshnessResult::OutputsMissing);
}
}OutputsMissing → is_fresh() returns false → always re-runs. The hash-based DepsState mechanism is never reached for non-vendored projects.
Also worth noting: pip has the same issue — it hardcodes .venv, so pip install without a virtualenv always re-runs too.
The underlying problem is that NoOutputs (empty outputs()) is also considered stale, so there's no way to say "skip output check, rely on hashes only".
The cleanest fix for both bundler and pip would be one of:
- Engine change: make
FreshnessResult::NoOutputsfall through to the hash check instead of being immediately stale — then non-vendored bundler could returnvec![]and rely purely onDepsStatehashing - Dynamic detection: return
vendor/bundleif it exists, elsevec![], paired with (1) - Stamp file: write a mise-managed marker after a successful run (e.g.,
.mise/deps/bundler.stamp) and use that as the output — works without any engine changes
Option 3 is the most self-contained fix for this PR. Options 1+2 would be a better long-term architecture fix that would also fix pip's equivalent issue.
|
closing in favor of #9622 |
…9622) ## Summary - Fixes #9585's underlying issue across all built-in providers, not just bundler. - `check_freshness` in `src/deps/engine.rs` now skips the output existence / session-stale checks when a provider returns no outputs and lets the source-hash check decide freshness on its own. First run is still stale (no stored hashes); subsequent runs with unchanged sources are fresh. - `bundler`, `pip`, `go`, `poetry`, and `uv` providers now return their conventional in-project output (`vendor/bundle`, `.venv`, `vendor/`) only when it actually exists; otherwise they return `vec![]` and rely on hashing. This stops `bundle install` / `pip install` / `go mod download` / `poetry install` / `uv sync` from re-running on every invocation for projects that don't keep deps in the project tree. - `FreshnessResult::NoOutputs` is removed since it's no longer reachable. ## Background The Greptile bot flagged this on #9585 — verified by tracing the flow: `engine.rs::check_freshness` checked output existence before the hash check, returning `OutputsMissing` (not fresh) whenever any declared output didn't exist. `bundle install` to the system gem path, `pip install` without a venv, `go mod download` without `vendor/`, etc. all left nothing in the project tree, so the providers were perpetually stale and the source-hash mechanism was never reached. ## Test plan - [x] `mise run lint` clean - [x] `mise run test:unit` — 827 passed - [x] `mise run test:e2e cli/test_deps cli/test_deps_depends cli/test_deps_tool_install` — all green - [x] New e2e regression in `e2e/cli/test_deps` covers the empty-outputs path: first run stale → install runs, second run fresh, source change → stale again 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes deps freshness evaluation and persisted state, which can alter when dependency install steps run or are skipped across multiple built-in providers. > > **Overview** > Fixes deps providers that don’t write outputs inside the project tree from being **perpetually stale** by letting `check_freshness` fall back to **source hashing** when required outputs are empty. > > Introduces `DepsProvider::optional_outputs()` and persists *seen optional outputs* in `DepsState`, so built-in providers can detect deletion of canonical directories (e.g. `.venv`, `vendor/`, `vendor/bundle`) **only after they’ve been observed**, without forcing reruns for projects that never create them. Updates `bundler`, `pip`, `go`, `poetry`, and `uv` to use optional outputs, extends `--list`/`--explain` to display them, and adds e2e coverage for empty-output providers and the “always run when no sources/outputs” case. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 14f1876. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
.bundleis not written bybundle install, so this causesbundle installto run every time because it's always stale.This change makes it look a bit more like
pip.