feat(fix): auto-remove unused pnpm catalog entries (closes #335)#363
Merged
Conversation
…e.yaml Adds `fallow fix` support for `unused-catalog-entries` findings shipped in v2.70.0 (#329). The previous JSON `actions[]` array declared `remove-catalog-entry` with `auto_fixable: false`; this PR closes the round-trip with a line-aware YAML deleter. Implementation choices: - Line-aware deletion (no YAML round-trip writer in the workspace). Object-form entries are detected by consuming subsequent lines whose indent is strictly greater than the entry's own indent. - Per-instance `auto_fixable`: catalog entries with a non-empty `hardcoded_consumers` list keep `auto_fixable: false`; entries safe to remove are marked `true`. Agents that filter on the bool no longer apply destructive ops to entries that need consumer-side migration first. - Skip records use the existing `remove_dependency` shape (`{type, applied: false, skipped: true, skip_reason, description, consumers}`) instead of a new top-level type, keeping the `fixes[]` array grep-able by issue kind. - Reparse-and-validate the post-edit content with `serde_yaml_ng` before persisting; reject multi-document YAML files up front. - Top-level `"skipped": <n>` count in JSON output alongside `total_fixed`. Human stderr surfaces a one-line skip summary plus a `pnpm install` reminder when any catalog entry was actually removed. - LSP code action mirrors the same hardcoded-consumer guard; emits a single TextEdit covering the full line range so undo collapses to one operation. Fixes #335.
…fix output
Folds in review feedback before merge:
- LSP code action's "is the reported line still the right catalog entry?"
sanity check was a loose `line.contains(entry_name)` substring scan.
pnpm catalogs commonly hold sibling entries with shared prefixes
(`react` and `react-native`, `lodash` and `lodash-es`, `core-js` and
`core-js-bundle`); if the file had been edited since `fallow check`
ran, the action could delete the wrong line. Replaced with anchored
key matching that accepts unquoted, double-quoted, and single-quoted
YAML key forms.
- LSP handler now accepts `file_lines` from the caller so the deletion
range is computed against in-memory editor content (mirrors
`build_remove_export_actions`). Previously read the file from disk,
ignoring unsaved edits.
- LSP action's reconstructed diagnostic now uses the named-catalog
message wording (`'react' in catalog 'react17' is not referenced...`)
to match what the diagnostic emitter publishes, so "Fix all in file"
source actions correlate cleanly.
- `consumers` field on skip records is now serialized with the same
`to_string_lossy().replace('\\', "/")` normalization that
`serde_path::serialize_vec` applies to `hardcoded_consumers` on the
check side, so agents correlating check + fix output see identical
path strings on Windows.
- Top-level `"skipped": 0` is now emitted on the zero-issues early-exit
JSON envelope so consumers can rely on the field being present.
- Human stderr reorders to lead with the actionable next step
(`Run \`pnpm install\`...`) and follows with residual work
(`Skipped N catalog entry/entries...`). Fixed the awkward
`entry(ies)` pluralization.
…rate rules - CHANGELOG.md [Unreleased]: full user-facing entry under Added describing the line-aware deletion, hardcoded-consumer guard, per-instance auto_fixable flip, JSON shape additions (top-level skipped, skip records with skip_reason / consumers / description), the pnpm install reminder, and the LSP code action. - .claude/rules/detection.md: extends the existing 'Unused pnpm catalog entries' bullet with the auto-fix details (line-aware strategy, hardcoded-consumer skip, multi-doc YAML guard, reparse-validate, LSP anchored-key matching). - .claude/rules/cli-crate.md: updates the fix/ module description to list catalog.rs alongside the existing fixers. - crates/cli/src/fix/mod.rs: includes the emit_human_summary helper extraction that satisfies the workspace too_many_lines lint (run_fix is back under the 150-line ratchet); was inadvertently left uncommitted in 5a1c780.
…oesn't break
A real install-breaking edge case was caught post-implementation: when
the last entry of a named catalog (`catalogs.react17:`) or the default
catalog (`catalog:`) was removed, the header was left bare (`react17:`)
which parses as a null value in YAML. pnpm 10.33.4 then errors with
`Cannot convert undefined or null to object` at install time.
Reproduced on `tests/fixtures/issue-329-pnpm-catalog`: after
`fallow fix --yes` the file's `react17:` was emptied, and a synthetic
pnpm smoke against the resulting layout failed.
This commit:
- Adds `find_parent_header_line` + `rewrite_empty_catalog_parents` in
`crates/cli/src/fix/catalog.rs`. After draining the deletion ranges,
for each unique parent header whose entries were deleted, check
whether any non-comment-non-blank children survive at greater indent.
If none survive, append \` {}\` to the header line so the catalog
serializes as an empty mapping (verified accepted by pnpm 10.33.4).
- Mirrors the same logic in
`crates/lsp/src/code_actions/quick_fix.rs::build_parent_rewrite_edit`.
The LSP code action now emits a second \`TextEdit\` in the same
\`WorkspaceEdit\` whenever the entry being removed is the last child
of its parent, so the quick-fix and `fallow fix` produce equivalent
results.
- Updates the existing \`leaves_empty_catalog_header_in_place\` unit
test (which asserted the buggy state) and adds three regression
tests covering default-catalog rewrite, named-catalog rewrite, and
the negative case (siblings remain so no rewrite fires).
- Adds three LSP regression tests covering the new TextEdit-pair
shape (one deletion + one parent rewrite) and the no-rewrite path
when siblings stay.
- Updates the CHANGELOG `[Unreleased]` entry and the detection.md rule
bullet to describe the rewrite behavior.
Companion-doc updates land in fallow-docs (de563ec) and fallow-skills
(f1c2d8e) in the same drop.
Verified end-to-end against `tests/fixtures/issue-329-pnpm-catalog`:
after fix, `pnpm-workspace.yaml`'s `catalogs.react17` is `{}` (empty
mapping), not null. Python yaml.safe_load confirms the parsed value
type is dict, not None.
…9 fixture A parallel reviewer caught a real install-breaking bug in #335's initial implementation because no integration test exercised \`fallow fix --yes\` end-to-end against the canonical \`tests/fixtures/issue-329-pnpm-catalog\` fixture and inspected the resulting YAML's parsed value types. Synthetic helper unit tests in \`fix/catalog.rs\` covered the rewrite logic in isolation, but the binary-vs-fixture flow had no coverage. This commit adds \`fix_catalog_issue_335_empties_parent_to_empty_map_not_null\`, which: - Copies the issue-329 fixture into a tempdir (avoids mutating the canonical fixture). - Runs \`fallow fix --yes --format json --quiet\` against it through the actual binary via the existing \`run_fallow_in_root\` harness. - Parses the post-fix \`pnpm-workspace.yaml\` with serde_yaml_ng and asserts \`catalogs.react17\` is an EMPTY MAPPING (\`{}\`), not null. String-level assertion alone is insufficient because the bug reproduced (bare \`react17:\`) parses as null but visually looks like an empty catalog group. - Sanity-checks that sibling \`catalogs.legacy\` and the default \`catalog:\` map are untouched where consumers still reference them. - Asserts the JSON envelope's top-level \`skipped: 1\` count (the fixture's \`hardcoded-pkg\` entry has a hardcoded consumer in \`packages/app\`). Adds \`serde_yaml_ng\` to \`fallow-cli\` dev-dependencies so the integration test can parse the result properly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #335.
fallow fixnow auto-removes unused entries frompnpm-workspace.yaml. Theunused-catalog-entriesdetector shipped in v2.70.0 (#329), but until now the only available action was the suppression comment; users had to hand-edit the YAML.react:\n specifier: ^18.2.0are removed as one block.catalog: {}(or<name>: {}) so the file stays installable. Barereact17:parses as null in YAML, which pnpm rejects withCannot convert undefined or null to objectat install time (verified against pnpm 10.33.4).hardcoded_consumersis non-empty are skipped: removing the catalog entry while a workspace package still pins a hardcoded version would break the nextpnpm install. The skip is surfaced in stderr and in the JSON output.auto_fixableon the check-command JSON action flips tofalsefor entries with hardcoded consumers, so agents that filter on the bool no longer apply destructive ops to entries needing consumer-side migration first."skipped"count alongside"total_fixed"on the fix-command JSON envelope for partial-fix gating in CI.Run pnpm install to refresh pnpm-lock.yamlreminder.Remove unused catalog entryquick-fix code action mirrors the same guards plus an anchored key-prefix sanity check so sibling entries with shared prefixes (reactvsreact-native,lodashvslodash-es) cannot be deleted by mistake.Follow-up issues filed:
catalogs.X:headers #359 — empty catalog group cleanup detectorauto_fixablesemantics in JSON schema + explain.rs #361 — document per-instanceauto_fixablesemanticsCompanion-repo docs updated in fallow-docs (
8273cde,de563ec) and fallow-skills (4c21c52,f1c2d8e).Test plan
cargo test -p fallow-cli -p fallow-lsppasses (19 catalog unit tests, 9 LSP catalog code-action tests, snapshot tests for json output)cargo clippy --workspace --all-targets -- -D warningscleancargo fmt --all -- --checkcleantests/fixtures/issue-329-pnpm-catalog: realpnpm install --no-frozen-lockfilesucceeds against the post-fix filereact17:(pre-fix shape) reproducesCannot convert undefined or null to objectagainst the same pnpm 10.33.4skippedfield, NEW has 5 catalog records (4 would-remove + 1 hardcoded-consumer skip) +skipped: 1