Conversation
|
@sonukapoor ready for review - OA011 detector for #1193, following OA009's precedent for the major-comparison approach as suggested. Full CONTRIBUTING.md verification sequence passes (151 suites / 2037 tests). |
sonukapoor
left a comment
There was a problem hiding this comment.
Verified rather than read, and the rule is right.
I built a fixture with the exact shape from CopilotKit#7167, an unbounded >=1.19.13 against a parent declaring ^1.13.5, and it fires. Changed the override to >=1.19.13 <2 and it correctly goes quiet. 13 tests green, build clean.
The range arithmetic is where I went looking for problems and didn't find any. <2 giving a max major of 1, <5.2.0 giving 5 because 5.1.99 is still reachable, and one unparseable parent silencing the whole finding rather than guessing are all the right calls. The conservative stance throughout is what makes this safe to ship.
Two notes, neither blocking.
OA011 needs node_modules present, because parentDeclarations comes from walkInstalledTree reading installed manifests. That is OA009's constraint rather than anything you introduced, and it is ours to fix, not yours. Flagging it because it cost me twenty minutes: I ran this against the real CopilotKit repo, got zero findings, and assumed the rule was broken. It wasn't, that checkout just has no node_modules. I am filing an issue to source parent declarations from the lockfile instead, which would make this work on a lockfile-only scan.
The other is a genuine question. A bare >=X reaches Infinity, so any unbounded override with parseable parents fires. On a block like CopilotKit's 69 entries, most of which are bare >=X, that could be dozens of medium findings amounting to "you used >= without a ceiling". I can see the argument that this is precisely the defect, and the argument that an unbounded floor with no known conflict is a weaker signal than a ceiling that genuinely exceeds a parent's. No change requested, I would just like your read before it meets a large real override block.
…ent declarations Overrides win dependency resolution outright, so a floor with no ceiling (or a ceiling above every parent's own) lets the resolver pick a major version none of the packages depending on it ever declared support for. OA011 flags that gap - the mirror image of OA009, which flags a floor already redundant rather than one that reaches too far. Scoped to the same range shapes OA009/OA010 already handle (^X, >=X, >=X <Y) and conservative the same way: silent when there's no parent declaration to compare against, or when a parent's range can't be reduced to a major. No auto-fix - lowering the override or raising the declaring package(s) is a judgement call only a maintainer can make.
9369e02 to
06fd400
Compare
Closes #1193
Overrides win dependency resolution outright, so a floor with no ceiling (or a ceiling set above every parent's own major) lets the resolver pick a major version that nothing depending on the package ever declared support for. That's exactly what happened upstream in CopilotKit/CopilotKit#7167: an unbounded
>=1.19.13override on@hono/node-serverresolved 2.0.0 while all ten manifests declaring the package stayed on 1.x.OA009 already asks the opposite question - is a floor already redundant, met by every parent? OA011 is its mirror: can the floor resolve above what every parent permits at all.
Approach
Scoped to the same floor shapes OA009 and OA010 already handle:
^X, bare>=X, and compound>=X <Y. For each shape I derive the highest major version the range can reach (Infinityfor a bare>=Xwith no ceiling), do the same for every parent declaration, and fire only when the override's ceiling exceeds the highest major any parent allows. Concrete pins, tildes, dist-tags, and workspace:/file:/link: protocols are out of scope, same boundary OA009 draws.Conservative by design: no parent declarations at all skips (nothing to compare against, OA001's territory), and a single parent declaration that can't be reduced to a major (an unrecognized shape) silences the whole finding rather than risk a false positive.
No auto-fix - the issue calls this out explicitly, since lowering the override vs. raising the declaring package(s) is a judgement call only a maintainer can make.
fixis leftundefined.Checklist (per CONTRIBUTING.md)
docs/rules/OA011.mdadded, detector references itsrc/overrides/detectors/index.ts(not inVERIFY_DETECTORS- no auto-fix, nothing to re-verify post-fix)tests/overrides/detectors/oa011.test.ts- firing case (unbounded floor / bare caret above every parent), non-firing case (bounded floor matching parent's major - the hono contrast from the issue), plus the conservative/skip pathswebsite/docs/override-hygiene/oa011.md), sidebar entry, rule count and tables inwebsite/docs/override-hygiene/index.mdandREADME.md(12 → 13 rules)npm ci && npm run lint:tests && npm run build && node dist/index.js advisories sync && npm test- 151 suites / 2037 tests greenKnown limitation (documented, not solved here)
Per the issue:
parentDeclarationsdoesn't yet read workspace member manifests, so on an uninstalled monorepo a member's own declaration can be invisible to this rule - the same gap OA009 already documents, and it improves as workspace-aware resolution (#1114) lands. Called out in both doc pages.