[prx-my]

What changed and why

cve-lite overrides --fix deleted valid nested override pins. There were two causes, both described in #1634:

1. Every OA005 remove patch pointed at the whole outer entry. npm's object form holds several pins per entry. A finding on one inner key therefore removed /overrides/foo and took its valid siblings with it.
2. The npm "." self-pin was checked as an undeclared dependency. npm uses "." to pin the outer package itself. Treating it as a dependency raised a high OA005.c finding whose auto-fix wiped the override.

Changes:

• oa005-nested-ineffective.ts
  • For the object form, the remove patch now targets [...entryPath, innerKey]. The flat pnpm parent>child form keeps the entry path, since that entry is a single pin.
  • "." returns early once the outer parent is confirmed in the tree. .a and .b still apply to it, because an ineffective or orphaned nested object is dead including its self-pin. .c, .d and .e have no inner dependency to reason about.
• fixer.ts: a remove now also drops a nested override object that became empty, stopping at the container (overrides, resolutions, pnpm.overrides). Without this, scoping the patch would leave "foo": {} behind where the old whole-entry removal left nothing. The existing top-level container pruning is unchanged.
• Docs: one sentence each in docs/rules/OA005.md and website/docs/override-hygiene/oa005.md describing the narrower patch.

Notes for review

• Findings' location.jsonPath still points at the outer entry. Only the fix patch path changed, so terminal, HTML and SARIF output are unchanged. Composite dedup keys on package name, not on path.
• tests/overrides/fixer.test.ts: the findingOf helper's parameter is now typed NonNullable<OverrideFinding["fix"]>["patch"]. This clears a type error that already existed in that file, which the changed-tests gate would otherwise flag.

Testing

• New detector tests cover the "." self-pin, an inner-key-scoped patch next to a valid sibling, and the unchanged pnpm flat-form path. New fixer tests cover sibling preservation and empty nested-object pruning. All three detector tests failed before the change.
• npm run lint:tests, npm run build, and node scripts/typecheck-changed-tests.mjs upstream/main (2 changed test files clean, no new errors).
• npm test: 232 suites, 3578 passed, 1 skipped.
• End-to-end with the built CLI, using the reproduction from fix(overrides): OA005 --fix deletes valid nested override pins #1634 plus one undeclared pin (ghost): only ghost is flagged and removed. "." and bar are kept.

Closes #1634

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[coderabbitai]

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

• Bug Fixes
  • Fixing a flagged pin in an npm object-form override preserves sibling pins and removes the outer entry only when it becomes empty. Empty override containers are also removed when no entries remain.
  • The "." self-pin is not treated as an undeclared dependency. pnpm fixes continue to remove the entire flagged entry.
  • Findings are cleared only when a matching fix is applied, so unresolved findings remain visible.
• Documentation
  • Updated OA005 guidance to describe npm object-form fix behavior and clarify how "." self-pins are handled.

OA005 fixes now remove only the flagged npm nested pin, skip the "." self-pin, and remove empty parent objects after removal. Flat pnpm fixes continue to remove the full entry. The CLI clears findings only when an applied fix matches both the rule and patch.

Changes: OA005 nested override fixes

Layer / File(s)	Summary
Targeted pin detection and removal src/overrides/detectors/oa005-nested-ineffective.ts, tests/overrides/detectors/oa005.test.ts, docs/rules/OA005.md, website/docs/override-hygiene/oa005.md	The detector skips "." and targets only the flagged npm inner pin. Flat pnpm fixes still target the whole entry. Tests and documentation cover these behaviors.
Empty parent cleanup src/overrides/fixer.ts, tests/overrides/fixer.test.ts	The fixer removes empty parent objects after a pin removal. Tests verify that sibling pins remain and empty override containers are removed.
Applied-fix filtering src/cli/overrides.ts, tests/cli/overrides-command.test.ts	The CLI matches findings to applied fixes by rule ID and patch. The end-to-end test checks the remaining finding and exit status after fixing and on a subsequent run.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Minimal · up to 5b9f2

OA005 auto-fix now removes only the flagged nested pin and leaves valid sibling pins in place. The change is limited to override hygiene fixes and has regression tests at the detector, fixer, and CLI levels. No merge-blocking risk is evident.

Pre-merge checks: 7

Check name	Status	Explanation
Title check		I checked the title against the changeset. It uses the required lowercase type prefix "fix:" and names the scope and behavior: OA005 --fix now removes only the flagged nested pin. Since the title foll…
Description check		The description follows the template. It has a "What changed and why" section that explains the two root causes, the per-file changes, and the reasoning behind them. It includes a "Closes #1634" line,…
Linked Issues check		I checked the whole-PR source diff for the two changed source files against the merge base. Issue #1634 objectives are met. (1) Scoped remove for npm object form: In src/overrides/detectors/oa005-nes…
Out of Scope Changes check		Each changed file maps to issue #1634 or to the fix behavior it requires. - src/overrides/detectors/oa005-nested-ineffective.ts: the scoped fix path and the "." guard. These form the core fix. - src/…
Linked Issue		I checked the pull request body against the Linked issue criterion. The body contains "Closes #1634" in its final line. That matches the required keyword form (Closes, Fixes, Resolves or Refs followed…
Closes Keyword Matches Scope		The PR body uses "Closes #1634". I read the linked issue and checked each requested item against the diff. Issue items and evidence: 1. "." is never checked against the parent's dependencies. Met. `cl…
No New Runtime Dependency		The check fails only if package.json gains a new dependencies or optionalDependencies entry, or moves a package from devDependencies into dependencies. The reviewed range (`refs/coderabbit/pre…

Full details: Title check

Explanation

I checked the title against the changeset. It uses the required lowercase type prefix "fix:" and names the scope and behavior: OA005 --fix now removes only the flagged nested pin. Since the title follows the required form and matches the main change, it passes. It is 63 characters long, which is acceptable. The title also meets the lowercase-after-colon rule and has no trailing period.

Full details: Description check

Explanation

The description follows the template. It has a "What changed and why" section that explains the two root causes, the per-file changes, and the reasoning behind them. It includes a "Closes #1634" line, which satisfies the template's "Closes #" requirement. It also adds review notes and testing details, which go beyond the template but add useful context. The description is complete and specific.

Full details: Linked Issues check

Explanation

I checked the whole-PR source diff for the two changed source files against the merge base. Issue #1634 objectives are met.

(1) Scoped remove for npm object form: In src/overrides/detectors/oa005-nested-ineffective.ts, classify() sets fixPath to [...entryPath, innerKey] when form is "object", and to entryPath otherwise. The remove patch uses fixPath. A fix for one inner key removes only /overrides/foo/<innerKey>. The pnpm flat parent>child form keeps the entry path, as the issue requires. [#1634]

(2) "." self-pin: The guard if (innerKey === ".") return null; sits after the .a and .b checks and before the .c check. The "." key does not reach the OA005.c undeclared-dependency check. The issue asks for this. The PR keeps .a and .b for ".", which the issue does not forbid. [#1634]

(3) Empty nested object pruning: The fixer.ts remove branch walks up from the removed path. It deletes each emptied ancestor and stops at the container depth. Top-level container pruning is unchanged. [#1634]

The reviewed head also includes a src/cli/overrides.ts change. It matches applied fixes by ruleId and patch content instead of package name. Without it, scoped fixes would hide sibling findings for the same package as resolved. This supports the sibling-preservation objective.

Uncertainty: I did not run the test suite. Test results rest on the author report. I did not confirm that the appliedPatches field name used in src/cli/overrides.ts matches its type definition. The author reports a clean build.

Full details: Out of Scope Changes check

Explanation

Each changed file maps to issue #1634 or to the fix behavior it requires.

• src/overrides/detectors/oa005-nested-ineffective.ts: the scoped fix path and the "." guard. These form the core fix.
• src/overrides/fixer.ts: empty nested-object pruning. The issue expects this cleanup.
• src/cli/overrides.ts: post-fix remaining-finding filter now matches on ruleId and patch. This keeps unfixed sibling findings visible after --fix. The PR description does not list this change by name. The commit message and linked objectives do describe it, so I treat it as in scope.
• docs/rules/OA005.md and website/docs/override-hygiene/oa005.md: one sentence each describing the narrower patch.
• Tests in tests/overrides/detectors/oa005.test.ts, tests/overrides/fixer.test.ts, and tests/cli/overrides-command.test.ts cover the new behavior. The fixer.test.ts helper type change is a typing fix in a file this change already touches.

I found no demonstrated unrelated change.

Full details: Linked Issue

Explanation

I checked the pull request body against the Linked issue criterion. The body contains "Closes #1634" in its final line. That matches the required keyword form (Closes, Fixes, Resolves or Refs followed by an issue number), so the check passes. The linked issue is #1634, "fix(overrides): OA005 --fix deletes valid nested override pins". The provider-reported primary PR is OWASP/cve-lite-cli #1636. This check only looks at the body text, so I did not need repository inspection to decide it. For users, the body means a link to the tracked issue is in place. Once this merges, #1634 closes automatically.

Full details: Closes Keyword Matches Scope

Explanation

The PR body uses "Closes #1634". I read the linked issue and checked each requested item against the diff. Issue items and evidence: 1. "." is never checked against the parent's dependencies. Met. classify in src/overrides/detectors/oa005-nested-ineffective.ts returns null for innerKey "." before the .c check. The test 'does not treat the "." self-pin as an undeclared inner dependency' covers it. 2. A fix for one inner key removes only that key. Met. For form "object", fixPath is [...entryPath, innerKey]. The test 'removes only the flagged inner key' expects /overrides/parent/missing, and the valid sibling is kept. 3. Empty-container pruning after removal. Met. applyOp in src/overrides/fixer.ts removes a nested object left empty. It stops at overrides, resolutions, or pnpm.overrides. The new fixer tests cover this. 4. Flat pnpm parent>child keeps whole-entry removal. Met. The test 'keeps the whole-entry fix path for the pnpm flat parent>child form' covers this. 5. Tests for a "." key and for an undeclared inner key with a valid sibling. Met in tests/overrides/detectors/oa005.test.ts. Deviation, not a failure: the issue's suggested fix skips "." in the object-form loop. The PR keeps .a and .b for "." and skips only .c, .d, and .e. The issue's expected behavior requires only that "." is never checked against the parent's dependencies, and the PR meets that. The third commit (matching fixes by applied patch) goes beyond the issue. This check does not fail on extra scope. Every listed item is delivered, so Closes #1634 is accurate.

Full details: No New Runtime Dependency

Explanation

The check fails only if package.json gains a new dependencies or optionalDependencies entry, or moves a package from devDependencies into dependencies. The reviewed range (refs/coderabbit/pre-merge-check/base to head) changes eight files: docs/rules/OA005.md, src/cli/overrides.ts, src/overrides/detectors/oa005-nested-ineffective.ts, src/overrides/fixer.ts, tests/cli/overrides-command.test.ts, tests/overrides/detectors/oa005.test.ts, tests/overrides/fixer.test.ts, and website/docs/override-hygiene/oa005.md. Neither package.json nor package-lock.json appears in that list. A diff of both files against the base is empty. No runtime dependency is added or moved, so the check does not apply a failure condition. The PR's source changes use existing modules and the test changes use the existing test setup.

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Automatic approval policy

Commit: 5b9f2e0a1307.

Setting	Effective value	Source
Automatic approvals	Enabled	Global configuration
Merge risk	Moderate	Global configuration
Security risk	Not set	Global configuration

No overrides matched. The global limits apply.

Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.

Run configuration

• Source mode: Auto
• Repository: OWASP/cve-lite-cli
• Branch: main
• File: .coderabbit.yaml
• Commit: 3d7af2c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e6d3b237-a797-4feb-b64b-ad7018f0ba52

Commits

Reviewing files that changed from the base of the PR and between caa6e1a and 5b9f2e0.

Files selected for processing: 3

• src/cli/overrides.ts
• tests/cli/overrides-command.test.ts
• tests/overrides/detectors/oa005.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[sonukapoor]

Thanks @prx-my, this is the right fix. I ran your repro plus a few variants against a real installed tree. On main every one of them wiped the whole overrides block; with your branch only the flagged pins go, "." and the valid siblings stay, and an entry that ends up empty is cleaned up. Tests and build are green on top of current main.

One thing needs to change before it goes in, and it's a side effect of the narrower patch rather than anything wrong in your code. After --fix, the overrides command works out which findings are still open by rule plus package name (src/cli/overrides.ts, the appliedKeys filter). That was fine when the whole entry got deleted, but now it hides real findings. Take "foo": { "ghost": "1.0.0", "baz": "2.0.0" } with baz 1.0.0 installed: overrides . --fix --fail-on medium removes ghost, prints "Applied 1 fix" and exits 0. Run overrides . --fail-on medium right after and the OA005 leak on baz is still there and it exits 1. Could you change that filter so it only drops the findings whose patch was actually applied (matching on the patch path works), and add a test for that case?

[prx-my]

Thanks @prx-my, this is the right fix. I ran your repro plus a few variants against a real installed tree. On main every one of them wiped the whole overrides block; with your branch only the flagged pins go, "." and the valid siblings stay, and an entry that ends up empty is cleaned up. Tests and build are green on top of current main.

One thing needs to change before it goes in, and it's a side effect of the narrower patch rather than anything wrong in your code. After --fix, the overrides command works out which findings are still open by rule plus package name (src/cli/overrides.ts, the appliedKeys filter). That was fine when the whole entry got deleted, but now it hides real findings. Take "foo": { "ghost": "1.0.0", "baz": "2.0.0" } with baz 1.0.0 installed: overrides . --fix --fail-on medium removes ghost, prints "Applied 1 fix" and exits 0. Run overrides . --fail-on medium right after and the OA005 leak on baz is still there and it exits 1. Could you change that filter so it only drops the findings whose patch was actually applied (matching on the patch path works), and add a test for that case?

Thanks @sonukapoor, both done in 5b9f2e0

• The appliedKeys filter in src/cli/overrides.ts now matches each finding’s own patch against the applied patches, instead of rule plus package name.
• That way a finding is dropped only when its patch was applied. I matched on the whole patch rather than the path, since relocate ops have no path.
• Added an end-to-end test using your foo: { ghost, baz } case. After --fix, the OA005 leak on baz is still reported and the run exits 1. It fails on the previous filter.
• Added the two "." cases to oa005.test.ts: a dead outer parent (.b) and a non-npm project (.a). Both still produce a remove patch.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.