From 680e16a7b738b9e9ff5fb4e8a16c8b34903e23b3 Mon Sep 17 00:00:00 2001 From: zuhairhalawa <71436209+zuhairhalawa@users.noreply.github.com> Date: Fri, 6 Nov 2026 00:51:09 +0400 Subject: [PATCH] feat: explain known-range notes removed while the range keeps an end Removing last_known_affected_version_range while the ranges still have an end was reported as field-removed, which suggested the value may have been dropped by accident. In real edits the end had usually moved too. known-range-removed still needs review, and says whether the end also changed. With no end left it stays field-removed, because that really can lose data. --allow field-removed no longer covers this case. The version is not bumped. --- CHANGELOG.md | 4 ++ README.md | 2 +- src/core/diff/affected.ts | 40 +++++++++++++++++- test/real-edits.test.ts | 86 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 126 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab2b0cf..24d1e5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ This changes finding codes and allow-list coverage. The cases come from running was reported as an added event and a removed field, and the second suggested the value may have been dropped by accident. The finding says whether the two name the same end, comparing strings only. A `--allow` list naming `range-event-added,field-removed` no longer covers this case. +- New `known-range-removed` finding: `last_known_affected_version_range` removed while the ranges still have an + end. This was `field-removed`, which suggested the value may have been dropped by accident, although the end + had usually moved too. It still needs review, and it says whether the end also changed. With no end left it + stays `field-removed`. `--allow field-removed` no longer covers this case. - New `version-spelling-changed` finding: a range value that changes only in leading zeros, such as `2025.08.14` to `2025.8.14`. `--allow range-event-changed` no longer covers this case. - New `version-leading-zero` warning from `validate`, for range events in `npm` packages and `SEMVER` ranges. The diff --git a/README.md b/README.md index 88a68b4..06f41ce 100644 --- a/README.md +++ b/README.md @@ -209,7 +209,7 @@ the indexes in paths: | --- | --- | | `severity[]` (also `affected[].severity`) | Matched by type, so CVSS v3 and v4 (and several entries) stay distinct. Removed = review, changed = review (with metric delta and v3 base score), added = info. | | `affected[]` | Matched by ecosystem+name; a lone unmatched package in the same ecosystem is treated as a **rename** so its ranges/severity are still compared. Removed/added packages = review. | -| ranges / events | `introduced`, `fixed`, `last_affected`, `limit` added/removed/changed = review, with an explanation (e.g. removing the only `fixed` leaves the range open-ended). Replacing `last_affected` with `fixed` (or the reverse) in a range, with the same number of values on both sides, = one review finding (`range-bound-converted`), because `last_affected` is an inclusive end and `fixed` an exclusive one, so affected status may change and needs checking against the package's release history (the repository's history for a GIT range). Several values of one kind in a range are listed without being paired. Replacing `database_specific.last_known_affected_version_range` with a range's one new end = one review finding (`known-range-converted`), which says whether the two name the same end. A value that changes only in leading zeros = review (`version-spelling-changed`). Version ranges changing for one package but not for another that had the same version data = review (`sibling-package-unchanged`). Explicit `versions[]` additions/removals = review. | +| ranges / events | `introduced`, `fixed`, `last_affected`, `limit` added/removed/changed = review, with an explanation (e.g. removing the only `fixed` leaves the range open-ended). Replacing `last_affected` with `fixed` (or the reverse) in a range, with the same number of values on both sides, = one review finding (`range-bound-converted`), because `last_affected` is an inclusive end and `fixed` an exclusive one, so affected status may change and needs checking against the package's release history (the repository's history for a GIT range). Several values of one kind in a range are listed without being paired. Replacing `database_specific.last_known_affected_version_range` with a range's one new end = one review finding (`known-range-converted`), which says whether the two name the same end. Removing that note while the ranges still have an end = review (`known-range-removed`), which says whether the end also changed. With no end left it is an ordinary removed field. A value that changes only in leading zeros = review (`version-spelling-changed`). Version ranges changing for one package but not for another that had the same version data = review (`sibling-package-unchanged`). Explicit `versions[]` additions/removals = review. | | references, aliases, related, upstream | Removal = review (lost provenance / cross-references); addition = info. URLs compared ignoring trailing slash and host case. | | CWE (`database_specific.cwe_ids`), GitHub severity label | Removal/change = review; addition = info. | | `id`, `published`, `withdrawn` | Changes = review (withdrawing is flagged). Two exceptions = info: setting a missing `published`, and changing the date of an existing `withdrawn`. | diff --git a/src/core/diff/affected.ts b/src/core/diff/affected.ts index 6e902ec..009c7ba 100644 --- a/src/core/diff/affected.ts +++ b/src/core/diff/affected.ts @@ -216,6 +216,33 @@ function knownRangeConverted(known: string, end: AddedEnd, where: string, path: }; } +/** Every end in an entry's ranges, sorted and without repeats so that array order cannot decide anything. */ +function endsOf(entry: JsonObject): string[] { + const ends = new Set(); + for (const range of asObjectArray(getOwn(entry, "ranges"))) { + for (const event of asObjectArray(getOwn(range, "events"))) { + for (const kind of ["fixed", "last_affected"] as const) { + const value = getOwn(event, kind); + if (isString(value)) ends.add(`"${kind}" ${value}`); + } + } + } + return [...ends].sort(); +} + +function knownRangeNoteRemoved(known: string, ends: string[], endMoved: boolean, label: string, path: string): Finding { + return { + code: "known-range-removed", + level: "review", + path: `${path}.database_specific`, + message: `${KNOWN_RANGE} "${shorten(known, 80)}" was removed from ${label}, whose ranges still end at ${previewList(ends)}.`, + why: endMoved + ? "The end also changed in this edit, so a note about the old end may no longer apply. Confirm the new end is right, and that the note recorded nothing else." + : "The ranges keep the same end, so tools that read only the range events are not affected. Confirm the note recorded nothing the end does not say.", + before: known, + }; +} + function spellingChanged(kind: EventKind, from: string, to: string, where: string, path: string, strict: boolean): Finding { const fromZero = hasLeadingZero(from); const toZero = hasLeadingZero(to); @@ -546,10 +573,19 @@ function diffPair(b: JsonObject, a: JsonObject, ai: number, out: Finding[], ctx: } diffSeverity(getOwn(b, "severity"), getOwn(a, "severity"), `${path}.severity`, ` (affected ${label})`, out); - const replaced = diffRanges(b, a, label, path, pa?.ecosystem ?? pb?.ecosystem, knownRangeRemoved(b, a), out, ctx); + const known = knownRangeRemoved(b, a); + const replaced = diffRanges(b, a, label, path, pa?.ecosystem ?? pb?.ecosystem, known, out, ctx); diffVersions(b, a, label, path, out); + let reported = replaced; + const endsAfter = endsOf(a); + // With no end left, the note may be all that said where the range stops, so that stays an ordinary removed field. + if (known !== undefined && !replaced && endsAfter.length > 0) { + const endMoved = JSON.stringify(endsOf(b)) !== JSON.stringify(endsAfter); + out.push(knownRangeNoteRemoved(known, endsAfter, endMoved, label, path)); + reported = true; + } // Every other key of database_specific is still compared, so a value dropped alongside it is still reported. - const rest = (entry: JsonObject): JsonValue => (replaced ? withoutKnownRange(remainder(entry)) : remainder(entry)); + const rest = (entry: JsonObject): JsonValue => (reported ? withoutKnownRange(remainder(entry)) : remainder(entry)); diffGeneric(rest(b), rest(a), path, out); } diff --git a/test/real-edits.test.ts b/test/real-edits.test.ts index 5e44716..7c4cc72 100644 --- a/test/real-edits.test.ts +++ b/test/real-edits.test.ts @@ -138,7 +138,7 @@ describe("a known range replaced by an end event", () => { it("does not apply when an end was also removed", () => { const r = diff(record(entry("a:lib", lastAt("2.0.0"), { known: "<= 2.0.0" })), record(entry("a:lib", fixedAt("2.1.0")))); - expect(reviewCodes(r).sort()).toEqual(["field-removed", "range-bound-converted"]); + expect(reviewCodes(r).sort()).toEqual(["known-range-removed", "range-bound-converted"]); }); it("does not apply when the known range is not a string", () => { @@ -155,11 +155,91 @@ describe("a known range replaced by an end event", () => { record({ package: { ecosystem: "Maven", name: "a:lib" }, ranges, ...(known ? { database_specific: { [KNOWN]: known } } : {}) } as JsonObject); const r1 = diff(mk([rangeA, rangeB], "< 6.0.0"), mk([endA, endB])); const r2 = diff(mk([rangeB, rangeA], "< 6.0.0"), mk([endB, endA])); - expect(reviewCodes(r1).sort()).toEqual(["field-removed", "range-event-added", "range-event-added"]); + expect(reviewCodes(r1).sort()).toEqual(["known-range-removed", "range-event-added", "range-event-added"]); expect(reviewCodes(r2).sort()).toEqual(reviewCodes(r1).sort()); }); }); +describe("a known range removed while the range keeps an end", () => { + const before = record(entry("a:lib", fixedAt("8.6.1"), { known: "<= 8.6.0" })); + const after = record(entry("a:lib", fixedAt("8.6.1"))); + + it("is its own review finding, without the dropped by accident wording", () => { + const r = diff(before, after); + expect(reviewCodes(r)).toEqual(["known-range-removed"]); + expect(codes(r)).not.toContain("field-removed"); + const f = r.findings[0]; + expect(f?.before).toBe("<= 8.6.0"); + expect(f?.message).toContain("<= 8.6.0"); + expect(f?.message).toContain("fixed"); + expect(f?.message).toContain("8.6.1"); + expect(f?.why).not.toContain("accident"); + expect(f?.why).toContain("same end"); + }); + + it("does not loosen the exit code", () => { + expect(diff(before, after).status).toBe("review"); + expect(cliDiff(before, after).code).toBe(1); + }); + + it("says so when the end also moved", () => { + const r = diff(record(entry("a:lib", fixedAt("8.0.32"), { known: "<= 8.0.31" })), record(entry("a:lib", fixedAt("8.0.33")))); + expect(reviewCodes(r).sort()).toEqual(["known-range-removed", "range-event-changed"]); + const f = r.findings.find((x) => x.code === "known-range-removed"); + expect(f?.why).toContain("also changed"); + expect(f?.why).not.toContain("same end"); + }); + + it("does not call a reordered range a moved end", () => { + const events: JsonObject[] = [{ introduced: "0" }, { fixed: "1" }, { introduced: "2" }, { last_affected: "3" }]; + const r = diff(record(entry("a:lib", events, { known: "<= 3" })), record(entry("a:lib", [...events].reverse()))); + expect(r.findings.find((x) => x.code === "known-range-removed")?.why).toContain("same end"); + }); + + it("works with last_affected as the end", () => { + const r = diff(record(entry("a:lib", lastAt("2"), { known: "< 3" })), record(entry("a:lib", lastAt("2")))); + expect(reviewCodes(r)).toEqual(["known-range-removed"]); + expect(r.findings[0]?.message).toContain("last_affected"); + }); + + it("stays a dropped field when no end remains, because that really can lose data", () => { + const r = diff(record(entry("a:lib", OPEN, { known: "< 2" })), record(entry("a:lib", OPEN))); + expect(reviewCodes(r)).toEqual(["field-removed"]); + expect(r.findings[0]?.why).toContain("accident"); + }); + + it("does not replace the conversion finding when the note became the end", () => { + const r = diff(record(entry("a:lib", OPEN, { known: "< 2" })), record(entry("a:lib", fixedAt("2")))); + expect(reviewCodes(r)).toEqual(["known-range-converted"]); + }); + + it("stays quiet when the note stays", () => { + const r = diff(before, record(entry("a:lib", fixedAt("8.6.1"), { known: "<= 8.6.0" }))); + expect(codes(r)).toEqual([]); + }); + + it("treats a note that is not a string as an ordinary dropped field", () => { + const r = diff(record(entry("a:lib", fixedAt("2"), { known: 5 })), record(entry("a:lib", fixedAt("2")))); + expect(reviewCodes(r)).toEqual(["field-removed"]); + }); + + it("needs its own code in an allow list, which does not hide other removed fields", () => { + expect(diff(before, after, ["known-range-removed"]).status).toBe("clean"); + expect(diff(before, after, ["field-removed"]).status).toBe("review"); + expect(cliDiff(before, after, ["--allow", "known-range-removed"]).code).toBe(0); + const b = record(entry("a:lib", fixedAt("2"), { extra: { database_specific: { [KNOWN]: "< 2", note: "kept" } } })); + expect(diff(b, record(entry("a:lib", fixedAt("2"))), ["known-range-removed"]).status).toBe("review"); + }); + + it("still reports another key dropped from database_specific", () => { + const b = record(entry("a:lib", fixedAt("2"), { extra: { database_specific: { [KNOWN]: "<= 1", note: "kept" } } })); + const r = diff(b, record(entry("a:lib", fixedAt("2")))); + expect(reviewCodes(r).sort()).toEqual(["field-removed", "known-range-removed"]); + const removed = r.findings.find((f) => f.code === "field-removed"); + expect(removed?.before).toEqual({ note: "kept" }); + }); +}); + describe("packages left out of a version change", () => { it("names the package that kept the old range", () => { const r = diff( @@ -228,7 +308,7 @@ describe("packages left out of a version change", () => { record(entry("a:one", fixedAt("2")), entry("a:two", fixedAt("2"), { known: "<= 1" })), ); expect(codes(r)).not.toContain("sibling-package-unchanged"); - expect(reviewCodes(r)).toEqual(["field-removed"]); + expect(reviewCodes(r)).toEqual(["known-range-removed"]); }); it("counts a package whose note changed but whose ranges did not as left out", () => {