fix(vendor-graph): rank releases by version, not by ASCII - #42
Conversation
`newestReleasedVersion` picked the newest release with a bare `.sort()`, which is lexicographic. ASCII orders 0.4.10, 0.4.46, 0.4.5, 0.4.9 and so answers "0.4.9". The engine is at 0.4.46 today, so this is not latent: every repository past a two-digit patch resolved to the wrong newest version, and that value is handed to `blastRadiusOf` as `proposedVersion` — every sealed blast-radius report named a release nobody would cut. Worse, and reachable from the register as written: a pin may name a version the release history does not list, and the ingest builds that artifact with no supersession chain — so it is a HEAD and joins the candidate set. Under ASCII a commit sha, or the literal `unknown`, outranks every real release because letters sort above digits. The report then proposed cutting `unknown`. Replaced with `compareVersions`, exported so the rule is assertable, and with the tie-break DOCUMENTED rather than implicit — an undocumented tie-break is how this class of bug returns. Numeric release components compared as digit strings (exact, no float); non-release strings sort below every release; a prerelease precedes its release; prerelease identifiers follow semver §11, so `rc.2` precedes `rc.10` instead of reintroducing the same defect one level down; build metadata is ignored for precedence; and the order is TOTAL, returning 0 only for identical strings, because an unresolved tie makes a sort depend on input order and this sort is sealed into a receipt. Hand-scanned, not regex-matched: `slug()` in this file already earned a CodeQL polynomial-redos finding on a register-supplied value. Three smaller defects found in the same review: - `supersessionChain`'s docstring claimed "longest path" while the code took the lowest next node id and the comment below said so. The code is right — lowest-id is replayable and a sealed receipt must be re-derivable — so the docstring is what changed. - The pinned-artifact branch built `producedBy` to `vfpId.repository(... ?? '')`, i.e. a dangling edge to `vfp:repo/` whenever the matched source declared no repo. Guarded on non-empty and routed through `ensureRepo` so the target node is guaranteed to exist. - `stats.releases++` was the one counter not gated by a `seen*` set: it counted manifest LINES while every other counter counted NODES CREATED, so a register declaring one release twice made them disagree. Tests: the ordering test is red against the old `.sort()` (asserts 0.4.46, gets 0.4.9) and uses the four-version case, so a single-digit-patch fixture cannot pass in its place. The forked-history test pins the documented lowest-id behaviour — its branches are built so longest-path gives a different `releaseDistance`, so a future "improvement" breaks a test instead of silently rewriting receipts.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes vendor freshness analysis to rank versions by semantic precedence (not lexicographic ASCII), and addresses related ingest correctness issues that affected sealed blast-radius receipts.
Changes:
- Replace lexicographic version sorting with an exported, documented
compareVersions()semver-like comparator. - Fix ingest consistency bugs: gate
stats.releaseslike other node counters and prevent danglingproducedByedges to an empty repository id. - Add targeted tests covering version precedence, forked supersession determinism, ingest stats correctness, and regression cases (
unknown/commit SHA).
Reviewed changes
Copilot reviewed 2 out of 6 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ts/src/vendor-graph.ts | Adds compareVersions() + parser, uses it for newest release selection, fixes ingest stats gating and producedBy edge creation. |
| ts/src/vendor-graph.test.ts | Adds regression tests for version precedence, forked supersession behavior, stats gating, and empty-repo edge prevention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function parseVersion(raw: string): ParsedVersion | undefined { | ||
| let s = raw | ||
| if (s.charCodeAt(0) === 118 /* v */) s = s.slice(1) | ||
| const plus = s.indexOf('+') | ||
| if (plus !== -1) s = s.slice(0, plus) // build metadata is not part of precedence | ||
| const dash = s.indexOf('-') | ||
| const core = dash === -1 ? s : s.slice(0, dash) | ||
| const pre = dash === -1 ? undefined : s.slice(dash + 1) | ||
| if (core.length === 0) return undefined |
There was a problem hiding this comment.
Rejecting — the current behavior is the semver-correct one, and the suggested change would reject valid versions. Under SemVer §10 build metadata is + followed by dot-separated identifiers that MAY contain hyphens, so 1.0.0+build-rc.1 is release 1.0.0 with build metadata build-rc.1 — the -rc.1 is part of the build tag, not a prerelease. Stripping at the first + and reading 1.0.0 is therefore correct, not a precedence boost. Verified: compareVersions('1.0.0+build-rc.1', '1.0.0-rc.1') > 0 (outranks the prerelease, as it must) and < '2.0.0' (not boosted above a real higher release). Enforcing - before + would make the parser reject a legal build-metadata string.
| export function compareVersions(a: string, b: string): number { | ||
| const ascii = (x: string, y: string): number => (x < y ? -1 : x > y ? 1 : 0) |
There was a problem hiding this comment.
Fair terminology point, keeping the wording. The ascii helper is x < y ? -1 : x > y ? 1 : 0, i.e. JS code-unit order, and you are right that "ASCII" is loose for the general case. It is exact for the inputs it runs on, though: every value reaching this comparator is a version string over [0-9A-Za-z.+-], all in the ASCII range, where code-unit order and ASCII order coincide. The name documents the intended domain rather than the engine primitive; renaming to codeUnit would be defensible but changes no behavior.
| test('compareVersions is linear — the version parser is not a ReDoS', () => { | ||
| const pathological = '9'.repeat(200_000) + 'x' | ||
| const t0 = process.hrtime.bigint() | ||
| for (let i = 0; i < 50; i++) compareVersions(pathological, '0.4.46') | ||
| const ms = Number(process.hrtime.bigint() - t0) / 1e6 | ||
|
|
||
| assert.ok(ms < 5_000, `version parsing went superlinear: ${ms.toFixed(0)} ms`) | ||
| // ...and it still gets the answer right: a 200k-digit non-release is not newer than 0.4.46. | ||
| assert.ok(compareVersions(pathological, '0.4.46') < 0) | ||
| }) |
There was a problem hiding this comment.
Fair in principle; leaving it as an absolute bound because the margin is enormous. Measured, the 50 iterations over a 200k-char pathological input complete in ~13 ms against the 5000 ms ceiling — ~380x headroom — so contention or a debug host would have to slow it by two-plus orders of magnitude to flake. The test's real job is to catch a super-LINEAR regression (a nested-quantifier rewrite of the hand scanner), which a 5 s wall on a fixed workload catches with room to spare. A relative-growth assertion is the more rigorous shape and I'd take it in a rewrite; not worth the added complexity for a guard with this much slack.
Retrospective Copilot review on #38, raised in both rounds. Four defects in
ts/src/vendor-graph.ts; one of them is live onmaintoday.The live one —
newestReleasedVersion.sort()on version strings is lexicographic:Not latent. The engine is at 0.4.46/0.4.47, so every repository past a two-digit patch resolved to the wrong "newest", and that value is handed to
blastRadiusOfasproposedVersion— so every sealed blast-radius report named a release nobody would cut.While writing the test I found the same defect reaching further than the ticket described. A pin may name a version the release history does not list; the ingest builds that artifact with no supersession chain, so it is a head and joins the candidate set. Under ASCII, letters outrank digits — so a
vendored_commitsha, or the literalunknown, beat every real release. The report proposed cuttingunknown. Covered by its own test.The comparator, and its documented tie-break
Replaced with
compareVersions, exported so the rule is assertable rather than implicit — an undocumented tie-break is how this class of bug comes back. Full rules are on the docstring; in order:v?digits(.digits)*-pre?+build?. Anything else (commit shas, dates,unknown,"") is not a release and sorts below every release; non-releases compare by ASCII between themselves.0. Compared as digit strings, not viaNumber, so precision never collapses two versions into a tie.1.0.0-rc.1<1.0.0.rc.10belowrc.2and reintroduce the same defect one level down.0only for identical strings. Same-release-different-text (0.4vs0.4.0,1.0.0+avs1.0.0+b) still orders, by raw text. An unresolved tie makes a sort depend on input order, and this sort's result is sealed into a receipt.Hand-scanned rather than regex-matched, deliberately:
slug()in this same file already earned a CodeQLjs/polynomial-redosfinding on a register-supplied value, and a nested-quantifier version pattern is the textbook way to earn a second. There is a linearity test.Three smaller defects, same review
supersessionChaindocstring (~L517) — claimed "longest path"; the code takes the lowest next node id and the comment two lines below already said so. The code is correct — lowest-id is replayable, and a sealed receipt must be re-derivable from the graph alone — so the docstring changed, not the code.producedByedge (~L477) —vfpId.repository(field(src,'repo') ?? '')built an edge tovfp:repo:, a dangling edge to a node the source loop never creates (it skips sources with norepo). Guarded on non-empty and routed throughensureRepo, so the target is guaranteed to exist rather than assumed to.stats.releases++(~L418) — the one counter not gated by aseen*set. It counted manifest lines whileartifacts/contracts/repositories/consumerAppscounted nodes created, so a register declaring one release twice made them disagree with no way for a caller to tell which question it had asked.Tests — red before green
vendor-graph.test.tshad no coverage for either semver ordering or forkedsupersededByhistories. Both added. Against the unmodified source, the new tests fail:The ordering test uses the four-version case, so a single-digit-patch fixture cannot pass in its place — and it asserts through
analyzeVendorFreshness().blastRadius, the real surface that was wrong, not just the private helper.The fork test pins the documented lowest-id behaviour: its branches are built so the two rules give different answers (low-id branch is a dead end at distance 1, high-id branch runs to distance 2), so a future "improvement" to longest-path changes
releaseDistance/latestVersionand breaks a test instead of silently rewriting receipts.After the fix: 417 pass / 0 fail (was 410 — seven new tests).
typecheckclean,buildclean,check:distexit 0 and reproducible.ts/distcommitted.Coordination with #40
#40 (
consume contractViolations) touches this same file. Checked before starting and verified empirically rather than by inspection —git merge-treeof this branch against #40's head:ts/src/vendor-graph.tsandts/src/vendor-graph.test.ts— auto-merge clean, zero conflict markers. The lanes are disjoint: this diff contains 0 lines mentioningcontractViolations.ts/dist/*conflicts — unavoidably. Git treats those files as binary (a NUL byte at ~offset 5985), so they cannot be auto-merged. Both branches rebuild them.Whoever merges second must rebuild
dist, not hand-resolve it:npm run build && git add ts/dist. It is a pure derivative of source, so this is mechanical, but it will not resolve itself.Do not merge — for review.