fix(vendor-graph): track-minor was track-latest — follow the pinned x.y line - #44
Conversation
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 track-minor freshness semantics so it follows the pinned x.y.* minor line (not “latest”), and ensures revendor proposals target the version allowed by the policy.
Changes:
- Add
policyTargetVersionto staleness verdicts and use it forproposeRevendor. - Rework
track-minorlogic to compute staleness based on the pinned minor line and prerelease rules. - Add targeted tests covering minor-line behavior, prerelease handling, non-release-shaped pins, and proposal targeting.
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 | Implements minor-line tracking, adds policyTargetVersion, and updates proposal targeting logic. |
| ts/src/vendor-graph.test.ts | Adds regression tests validating the corrected track-minor semantics and proposal behavior. |
Comments suppressed due to low confidence (1)
ts/src/vendor-graph.ts:1
StalenessVerdictis exported, and adding a new required field is a breaking change for downstream TypeScript consumers that implement/construct this interface or rely on structural typing. If this package is intended to publish a patch-level fix, consider makingpolicyTargetVersionoptional with a well-defined fallback, or ensure the release/versioning strategy reflects this breaking API change.
/**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const newestOnLine = onLine[onLine.length - 1]?.version | ||
| // The proposal target is the newest IN-LANE release, never `latestVersion`. `0.5.0` being | ||
| // newer is not a reason to propose it to a pin that follows `0.4.*`. | ||
| if (newestOnLine !== undefined) policyTargetVersion = newestOnLine |
There was a problem hiding this comment.
Valid, and fixed in d00aa6f — this was the sharpest comment of the round.
Reproduced against the real ingest rather than reasoned about: with the 0.4 line declared out of precedence order ['0.4.5', '0.4.46', '0.4.9'], the old onLine[onLine.length - 1] returned policyTargetVersion = 0.4.9 and a rationale naming 0.4.9 the "newest". onLine preserves supersession-chain (declared) order, which the filter three lines up already treats as possibly out of order — so trusting its tail reintroduced the exact chain-vs-precedence defect this stack exists to remove, one level down, and proposeRevendor would have sealed a receipt for the wrong version.
Now onLine.reduce((m, a) => compareVersions(a.version, m.version) > 0 ? a : m).version, the same ranking newestReleasedVersion already uses. Added a regression test that declares the line out of order and asserts the target and rationale name 0.4.46 — RED against the old code ('0.4.9' !== '0.4.46'), green now. Full suite 427/427, typecheck clean, ts/dist rebuilt.
The sibling latestVersion = intervening[intervening.length - 1] makes the same declared-order assumption for track-latest; it is pre-existing and outside this diff, flagged as a follow-up in the PR description.
| function minorLineOf(v: string): string | undefined { | ||
| const p = parseVersion(v) | ||
| if (!p) return undefined | ||
| return `${p.release[0] ?? '0'}.${p.release[1] ?? '0'}` | ||
| } |
There was a problem hiding this comment.
Valid as a normalization gap, not actioned here — and it is not reachable from the register. minorLineOf builds the lane key from raw digit components, so 01.02.0 and 1.2.0 would land on different lanes (01.02 vs 1.2) even though compareVersions ranks them equal. But leading zeros in a numeric identifier are invalid under SemVer §2, so the vendor register does not emit them (the live register is canonical semver throughout), which makes this a latent inconsistency rather than a live defect. If lane keys are ever normalized through compareVersions' numeric view it should be closed there; keeping it out of a fix scoped to the target-selection bug.
| const followsPre = isPrerelease(pinnedVersion) | ||
| const onLine = intervening.filter((a) => | ||
| minorLineOf(a.version) === line && | ||
| compareVersions(a.version, pinnedVersion) > 0 && | ||
| (followsPre || !isPrerelease(a.version))) | ||
| const offLine = intervening.filter((a) => minorLineOf(a.version) !== line) |
There was a problem hiding this comment.
Not actioned — this is not a hot path. minorLineOf/isPrerelease re-parse per intervening artifact, but intervening is one pin's release chain (a handful of entries), parseVersion is a linear non-backtracking scan, and stalenessOf runs once per pin in an offline analysis that seals a receipt — not per-request. Caching parsed views would trade an unmeasurable saving for indirection on the exact code path a reader needs to follow to trust the verdict. Leaving as-is.
Review disposition — including the suppressed channelCopilot ran in degraded mode (banner) and reviewed 2 of 6 changed files, so the diff was reviewed independently. Dispositions: Fixed
Answered on-thread
Suppressed channel —
|
4a6c02f to
8043934
Compare
….y line
`track-minor` was implemented with `majorOf`:
const sameMajorNewer = intervening.some((a) => majorOf(a.version) === majorOf(pinnedVersion))
const majorMoved = releaseDistance > 0 && majorOf(latestVersion) !== majorOf(pinnedVersion)
freshnessState = sameMajorNewer || majorMoved ? 'stale' : 'current'
A policy whose NAME states one contract and whose CODE honours another. And it is
worse than "track-minor is really track-major": the two arms are EXHAUSTIVE — if
nothing newer shares the pinned major, the newest release cannot either, so
`majorMoved` fires. `releaseDistance > 0` was therefore always stale, which is
`track-latest`. Three declared policies, two distinct behaviours, and the collapsed
one is the DEFAULT that every pin in the live vendor register declares.
Consequence, measured before the change: a pin at 0.4.46 was reported stale against
0.5.0, and `proposeRevendor` sealed an EffectRequest to take it. A manifest author
who wrote `track-minor` to stay on a minor line was silently opted in to leaving it.
Established semantics — a pin on `x.y.*` follows its own minor line:
· 0.4.46 → 0.4.47 in-lane ⇒ stale, target 0.4.47 (accepted)
· 0.4.46 → 0.5.0 off-lane ⇒ current (refused)
· 0.4.46 → 1.0.0 off-lane ⇒ current (refused)
`current` here never means "nothing happened upstream": the rationale names every
off-line release and `releaseDistance`/`latestVersion` still report the real gap.
Also fixes the same defect one layer down. `proposeRevendor` took
`verdict.latestVersion`, so a correctly-stale track-minor pin at 0.4.46 with 0.4.47
AND 0.5.0 upstream would have proposed → 0.5.0: the exact bump the policy refuses,
under a rationale saying it was withheld. `StalenessVerdict` now carries
`policyTargetVersion` — the newest release the POLICY will take — and the proposal
uses it. track-latest is unchanged and still proposes the newest release.
Two boundary rules, stated so they are not rediscovered as bugs:
· a pin that is not release-shaped (a sha, `unknown`) names no line, so its state
is `unknown` — the same answer, for the same reason, that an unobserved pin gets
· a prerelease does not make a stable pin stale (semver ranges treat prereleases as
opt-in); a pin that is itself a prerelease does follow them
The line is derived through this branch's `parseVersion`, not `split('.')`, so
`v0.4.46`, `0.4.46+build.7` and `0.4` all resolve to the 0.4 line — which is why this
stacks on #42 rather than duplicating a version parser.
Proof: the 9 new tests fail against the `majorOf` implementation and pass against
this one. 426/426 green, typecheck clean, ts/dist rebuilt.
…ENCE, not the chain tail
`stalenessOf`'s track-minor arm set `policyTargetVersion` to `onLine[onLine.length - 1]` —
the last on-line release in supersession-chain order. That chain is the register's DECLARED
order, which the filter directly above treats as possibly out of order (it ranks membership
with `compareVersions` for exactly that reason). Taking the chain tail as "newest" reintroduced
the chain-vs-precedence defect this stack exists to remove, one level down: an out-of-order
0.4 line `[0.4.46, 0.4.9]` proposed `-> 0.4.9`, sealed a re-vendor receipt for it, and printed
a rationale calling 0.4.9 the "newest".
Reduce by `compareVersions` instead — the same ranking `newestReleasedVersion` already uses.
The regression test drives the real ingest with releases declared out of precedence order; it
is RED against the old `onLine[last]` (`'0.4.9' !== '0.4.46'`) and green now. Full suite
427/427, typecheck clean, ts/dist rebuilt.
Addresses the Copilot review comment on pulls/44 ("newestOnLine is derived from the last
element of onLine ... compute the max by compareVersions instead of relying on array order").
`policyTargetVersion` kept required — it is a derived output field, always produced — with the
rationale documented on the interface.
Follow-up, not fixed here: `latestVersion = intervening[intervening.length - 1]` makes the same
declared-order assumption for track-latest; it is pre-existing and outside this diff.
d00aa6f to
a96b307
Compare
Stacked on #42 (
fix/vendor-graph-semver-ordering) — merge #42 first. It stacks because this fix needs #42'sparseVersion, not because the two touch the same lines.The defect
track-minorwas implemented withmajorOf:A policy whose name states one contract and whose code honours another.
It is worse than "track-minor is really track-major". The two arms are exhaustive: if nothing newer shares the pinned major, then the newest release cannot either, so
majorMovedfires.releaseDistance > 0was therefore always stale — which istrack-latest. Three declared policies, two distinct behaviours, and the collapsed one is the default (defaultFreshnessPolicy ?? 'track-minor') that every pin in the live vendor register declares.Measured before the change — identical graph, both policies:
track-minortrack-latest0.4.46 → 0.4.47patch, inside minor0.4.46 → 0.5.0minor, leaves minor0.4.46 → 1.0.0major, leaves major0.4.46, nothing newer0 of 5 probe cases distinguished the two policies.
majorOfonly ever changed the rationale string, never a verdict.The semantics established
A pin on
x.y.*follows its own minor line. It accepts patch bumps within that line; it does not accept bumps that leave it.0.4.46 → 0.4.47in-lane0.4.470.4.46 → 0.5.0off-lane0.4.46 → 1.0.0off-lanecurrenthere never means "nothing happened upstream".releaseDistanceandlatestVersionstill report the real gap, and the rationale names every off-line release:Leaving the line stays a deliberate act — an owner editing the pin, or declaring
track-latest— not something the freshness plane proposes on the owner's behalf.The same defect one layer down
proposeRevendortookverdict.latestVersion. So a correctly staletrack-minorpin at0.4.46with both0.4.47and0.5.0upstream would still have proposed→ 0.5.0: the exact bump the policy refuses, sealed into anEffectRequest, under a rationale saying it was withheld. FixingstalenessOfalone would have moved the bug rather than closed it.StalenessVerdictnow carriespolicyTargetVersion— the newest release the policy will take (track-latest⇒ newest;pin-exact⇒ the pin;track-minor⇒ newest in-lane) — and the proposal uses it.track-latestis unchanged and still proposes the newest release; the split is per-policy, not global.Boundary rules, stated so they are not rediscovered as bugs
unknown) names nox.yline, so its state isunknown— the same answer, for the same reason, that an unobserved pin already gets. Derivingcurrentwould be exactly the guess the unobserved branch refuses to make. A register declaringcurrentover it becomes adispositionViolation, which is correct: you cannot follow a minor line from a sha.0.4.47-rc.1leaves0.4.46current), matching how semver ranges treat prereleases — opt-in, never inherited. A pin that is itself a prerelease does follow them.parseVersion, notsplit('.'), sov0.4.46,0.4.46+build.7and0.4all resolve to the0.4line, and1/1.0/1.0.0are one lane.Proof
The 9 new tests were run against both implementations:
Including
the live register is unaffected: 0.4.40 → 0.4.45 is all one 0.4 line— the fix must not quietly un-stale the real finding the plane was built to report. (It fails pre-fix only on the newpolicyTargetVersionassertion; the state was and remainsstale.)Full suite 426/426,
npm run typecheckclean,ts/distrebuilt and committed.Lane note
ts/src/vendor-graph.tsis also touched by #40 and #42.git merge-treeagainst both:The three PRs touch disjoint regions of the source: #42 the ingest + comparator, #40
analyzeVendorFreshness, this onestalenessOf+proposeRevendor. The only real overlap is the rebuiltts/dist, which git cannot auto-merge — whoever merges second runsnpm run build && git add ts/dist.Follow-up from merge-gate review (fix pushed in d00aa6f)
Fixed — the in-lane proposal target. The Copilot review flagged that
newestOnLinetookonLine[onLine.length - 1](the supersession-chain tail), while the filter just above ranks membership withcompareVersionsprecisely because "a register may declare its releases out of order." Reproduced against the real ingest —['0.4.5', '0.4.46', '0.4.9']on the 0.4 line made the old code propose→ 0.4.9and print a rationale calling 0.4.9 the "newest" — and fixed to reduce bycompareVersions(the rankingnewestReleasedVersionalready uses). The regression test is RED againstonLine[last]('0.4.9' !== '0.4.46') and green now; full suite 427/427,ts/distrebuilt.Follow-up, deliberately NOT in this diff:
latestVersion = intervening[intervening.length - 1]instalenessOfmakes the same declared-order assumption for thetrack-latestarm — the chain tail is trusted as "latest". It is pre-existing on that line (not introduced here) and outside thetrack-minorregion this change touches, so it is left for a separate change rather than widening the diff. Same remedy: rank the chain bycompareVersions, or guarantee precedence order at ingest.