From 2a3b16e0186587ac321d5c1be086417d36fbca36 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 20:04:59 +0000 Subject: [PATCH 1/4] fix(packs): score expected sites at their rule's or note's severity `pack test` counted every missed site as a breaking false negative unless a *rule* with that id was deprecated: `BehaviorNote.severity` was never read, so missed behavior-note sites that only need a look (ioredis `client-defaults`) were reported as missed breaks. An expected site now takes the severity of the rule or note it is expected under: a note's own `severity`, breaking when it reports findings without one (as `check` reports them), and `behavior` for a note `check` never reports. Behavior-note sites get their own precision and recall, in the output and in verification.json (`behavior`); both tallies record the counts behind their rates, so a zero denominator prints `n/a` in `pack test` and the README table instead of 100%. False negatives print their severity. Older records still read as before. Fixes UPT-52. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011t9c3ZLsM9B3KiYGPFDyyK Signed-off-by: Claude --- .changeset/pack-test-behavior-notes.md | 5 + CONTRIBUTING.md | 4 +- docs/packs.md | 27 ++- packages/cli/src/pack.test.ts | 51 ++++++ packages/cli/src/pack.ts | 15 +- packages/core/src/index.ts | 2 + packages/core/src/packs/contract.ts | 34 +++- .../src/packs/tooling/behavior-score.test.ts | 164 ++++++++++++++++++ packages/core/src/packs/tooling/pack-test.ts | 142 ++++++++++----- packages/core/src/packs/tooling/scaffold.ts | 17 +- scripts/docs-packs.ts | 5 +- 11 files changed, 402 insertions(+), 64 deletions(-) create mode 100644 .changeset/pack-test-behavior-notes.md create mode 100644 packages/cli/src/pack.test.ts create mode 100644 packages/core/src/packs/tooling/behavior-score.test.ts diff --git a/.changeset/pack-test-behavior-notes.md b/.changeset/pack-test-behavior-notes.md new file mode 100644 index 0000000..1bb97c0 --- /dev/null +++ b/.changeset/pack-test-behavior-notes.md @@ -0,0 +1,5 @@ +--- +"uptide": patch +--- + +`uptide pack test` scores each expected site at the severity of the rule or behavior note it belongs to, so a missed note that only needs a look, or a decision, no longer counts as a missed break. Behavior-note sites get their own precision and recall in the output and in `verification.json` (`behavior`, with the counts behind each rate), false negatives show their severity, and a rate with nothing to measure prints `n/a` instead of 100%, in `pack test` and in the README's packs table. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6cbf3ce..ac2ec04 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -139,7 +139,9 @@ open a **Pack request** issue, and say you are writing it. pnpm uptide pack test --write # records the status in verification.json ``` Every false positive among breaking findings fails it. Fix the rule, not the ground truth. - A false negative is a site to cover or a known gap to say in the pull request. + A false negative is a site to cover or a known gap to say in the pull request. A site + expected under a behavior note scores at the note's `severity`, and behavior-note sites + get their own precision and recall: a missed "needs a look" is not a missed break. 7. **Smoke-test locally** without editing `verification.json`: ```sh pnpm uptide check --allow-candidate --cwd /path/to/local-fixture diff --git a/docs/packs.md b/docs/packs.md index 1ad07b7..9bd8b01 100644 --- a/docs/packs.md +++ b/docs/packs.md @@ -255,10 +255,21 @@ findings: - **per rule**: a site counts for a rule when file, line and rule all match; - **overall**: at the line level, every severity; +- **behavior notes**: at the line level, the sites of the pack's behavior notes, whatever + their severity; - **breaking**: at the line level, breaking findings only. -It prints precision and recall for each, and every false positive and false negative as -`repo file:line rule`, and sites found under another rule. It exits non-zero on any false +An expected site has the severity of the rule or note it is expected under: a rule's +`severity`; a note's `severity`, or breaking when the note reports findings without one (as +`check` reports them); `behavior` for a note `check` never reports as a finding (`decision`, +`test-follow-up`); breaking for `generic`. A predicted site has the severity `check` reported. +A missed site counts against breaking recall only when its severity is breaking: a missed +`deprecated` note ("needs a look") or a decision is not a missed break. + +It prints precision and recall for each, `n/a` where there was nothing to measure (nothing +predicted, or nothing expected), every false positive as `repo file:line rule (severity)`, +every false negative the same way with the severity it was expected at, and sites found under +another rule. It exits non-zero on any false positive among breaking findings, any fixture failure, or a `verification.json` that no longer matches what it measured (`--write` records it). @@ -277,7 +288,9 @@ breaking finding predicted. Anything else is a **candidate**: cannot be published. Never edit `verification.json` just to smoke-test a pack. The CLI reads the status from `verification.json`, which `pack test --write` writes and -which records the digest of the ground-truth file it was measured against. CI runs +which records the digest of the ground-truth file it was measured against, with precision, +recall and the counts behind them for breaking findings and for behavior-note sites +(`behavior`, absent from records written before it). The gate reads the breaking ones only. CI runs `pack test` for every pack on every pull request that touches packs and fails when the record no longer matches the evidence, so the label cannot outlive what supports it. @@ -318,14 +331,16 @@ pnpm uptide pack test ai --write # record the measured status in verific pnpm uptide pack test ai --fixtures-only --update-fixtures # write after.ts from the rules ``` -`pack test` prints precision and recall per rule, overall and for breaking findings, and -every false positive and false negative as `repository file:line rule`. Exit codes: **0** +`pack test` prints precision and recall per rule, overall, for behavior-note sites and for +breaking findings, and every false positive and false negative as +`repository file:line rule (severity)`. Exit codes: **0** every pack passed, **1** a false positive among breaking findings, a failing fixture, or a `verification.json` the run does not support, **2** it could not run (not in a checkout, bad arguments). The "Verified packs" table in the README is generated from each pack's `verification.json` -and ground truth by `pnpm docs:packs`; CI fails when it is stale. +and ground truth by `pnpm docs:packs`; CI fails when it is stale. Its precision and recall are +the breaking ones, `n/a` when nothing breaking was predicted or expected. ### Config files and transactional edits diff --git a/packages/cli/src/pack.test.ts b/packages/cli/src/pack.test.ts new file mode 100644 index 0000000..658e3bb --- /dev/null +++ b/packages/cli/src/pack.test.ts @@ -0,0 +1,51 @@ +import { type PackTestReport, registeredPacks, unfinishedPack } from '@uptide/core'; +import { expect, it } from 'vitest'; +import { formatPackTest } from './pack.js'; + +it('prints behavior notes on their own row, n/a where nothing was predicted, and each miss with its severity', () => { + const entry = registeredPacks()[0]; + if (!entry) throw new Error('no registered pack'); + const tally = (tp: number, fp: number, fn: number) => ({ + truePositives: tp, + falsePositives: fp, + falseNegatives: fn, + precision: tp + fp ? tp / (tp + fp) : 1, + recall: tp + fn ? tp / (tp + fn) : 1, + }); + const site = (line: number, rule: string, severity: 'breaking' | 'deprecated') => ({ + file: 'src/client.ts', + line, + rule, + severity, + }); + const report: PackTestReport = { + ...unfinishedPack(entry, ''), + problems: [], + repos: [ + { + label: 'owner/name', + from: '5.0.0', + to: '6.0.0', + predicted: [site(13, 'client-defaults', 'deprecated')], + expected: [ + site(9, 'client-defaults', 'deprecated'), + site(20, 'handshake-auth', 'breaking'), + ], + falsePositives: [site(13, 'client-defaults', 'deprecated')], + falseNegatives: [ + site(9, 'client-defaults', 'deprecated'), + site(20, 'handshake-auth', 'breaking'), + ], + wrongRule: [], + }, + ], + overall: tally(0, 1, 2), + breaking: tally(0, 0, 1), + behavior: tally(0, 1, 2), + }; + const out = formatPackTest(report); + expect(out).toMatch(/behavior notes \(sites\)\s+0%\s+0%\s+0\s+1\s+2/); + expect(out).toMatch(/breaking \(sites\)\s+n\/a\s+0%\s+0\s+0\s+1/); + expect(out).toContain('src/client.ts:9 client-defaults (deprecated)'); + expect(out).toContain('src/client.ts:20 handshake-auth (breaking)'); +}); diff --git a/packages/cli/src/pack.ts b/packages/cli/src/pack.ts index 6977a0b..f201205 100644 --- a/packages/cli/src/pack.ts +++ b/packages/cli/src/pack.ts @@ -1,6 +1,6 @@ import { existsSync, readFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; -import type { PackTestReport, RegisteredPack, Tally } from '@uptide/core'; +import { type PackTestReport, type RegisteredPack, rateLabel, type Tally } from '@uptide/core'; import pc from 'picocolors'; import { CliError } from './errors.js'; @@ -48,10 +48,8 @@ export function packsToTest( return [entry]; } -const pct = (n: number): string => `${Math.round(n * 100)}%`; - function row(name: string, t: Tally): string { - return ` ${name.padEnd(28)} ${pct(t.precision).padStart(9)} ${pct(t.recall).padStart(7)} ${String(t.truePositives).padStart(5)} ${String(t.falsePositives).padStart(4)} ${String(t.falseNegatives).padStart(4)}`; + return ` ${name.padEnd(28)} ${rateLabel(t, 'precision').padStart(9)} ${rateLabel(t, 'recall').padStart(7)} ${String(t.truePositives).padStart(5)} ${String(t.falsePositives).padStart(4)} ${String(t.falseNegatives).padStart(4)}`; } /** What `uptide pack test` prints for one pack. */ @@ -109,6 +107,8 @@ export function formatPackTest(report: PackTestReport, color = false): string { for (const [rule, t] of Object.entries(report.rules).sort(([a], [b]) => a.localeCompare(b))) out.push(row(rule, t)); out.push(row('overall (sites)', report.overall)); + // Absent from a report written before behavior notes were scored on their own. + if (report.behavior) out.push(row('behavior notes (sites)', report.behavior)); out.push(c.bold(row('breaking (sites)', report.breaking))); const listed = ( title: string, @@ -133,7 +133,12 @@ export function formatPackTest(report: PackTestReport, color = false): string { listed( 'false negatives', report.repos.flatMap((r) => - r.falseNegatives.map((s) => ({ label: r.label, file: s.file, line: s.line, text: s.rule })), + r.falseNegatives.map((s) => ({ + label: r.label, + file: s.file, + line: s.line, + text: s.severity ? `${s.rule} (${s.severity})` : s.rule, + })), ), ); listed( diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0060ee0..fdcb298 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -186,8 +186,10 @@ export { type PackStatus, type PackVerification, type RegisteredPack, + rateLabel, replaceAtSite, VERIFIED_MIN_REPOS, + type VerificationTally, } from './packs/contract.js'; export { activePacks, packStatus, registeredPacks } from './packs/index.js'; export { leadName, leadNames, packLeading } from './packs/leads.js'; diff --git a/packages/core/src/packs/contract.ts b/packages/core/src/packs/contract.ts index 774b0e9..968d2cf 100644 --- a/packages/core/src/packs/contract.ts +++ b/packages/core/src/packs/contract.ts @@ -157,7 +157,39 @@ export interface PackVerification { truth: string; /** Public repositories in the ground truth (fixtures excluded). */ repos: number; - breaking: { precision: number; recall: number; falsePositives: number }; + breaking: VerificationTally; + /** Sites of behavior notes, whatever their severity. Absent from records written before it. */ + behavior?: VerificationTally; +} + +/** + * Precision and recall as recorded. A ratio with nothing to measure reads 1; the counts, absent + * from records written before they were kept, tell that apart from a perfect score. + */ +export interface VerificationTally { + precision: number; + recall: number; + falsePositives: number; + truePositives?: number; + falseNegatives?: number; +} + +/** A run's tally as `verification.json` records it. */ +export function verificationTally(t: Required): Required { + return { + precision: Number(t.precision.toFixed(4)), + recall: Number(t.recall.toFixed(4)), + falsePositives: t.falsePositives, + truePositives: t.truePositives, + falseNegatives: t.falseNegatives, + }; +} + +/** A precision or recall for people: `n/a` when nothing was predicted, or nothing expected. */ +export function rateLabel(tally: VerificationTally, rate: 'precision' | 'recall'): string { + const missed = rate === 'precision' ? tally.falsePositives : tally.falseNegatives; + if (tally.truePositives === 0 && missed === 0) return 'n/a'; + return `${Math.round(tally[rate] * 100)}%`; } /** diff --git a/packages/core/src/packs/tooling/behavior-score.test.ts b/packages/core/src/packs/tooling/behavior-score.test.ts new file mode 100644 index 0000000..ae7f147 --- /dev/null +++ b/packages/core/src/packs/tooling/behavior-score.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, it } from 'vitest'; +import { definePack, rateLabel } from '../contract.js'; +import { type ScoredSite, scoreSites, scoreTruth, siteSeverity } from './pack-test.js'; + +// Synthetic, modeled on the ioredis 5 → 6 ground truth (PR 129): every expected site belongs to a +// behavior note. `client-defaults` only needs a look (deprecated); `handshake-auth` is declared +// breaking; `node-minimum` is a decision `check` never reports as a site. +const pack = definePack({ + meta: { + package: 'redis-like', + from: '>=5 <6', + to: '>=6 <7', + sources: [{ title: 'guide', url: 'https://example.com/guide' }], + maintainer: 'uptide-dev', + }, + rules: [ + { + id: 'duplicate-signature', + summary: 'duplicate() became generic', + severity: 'breaking', + kinds: ['signature'], + symbols: /^TS2416$/, + guide: 'make the override generic too', + }, + ], + behavior: [ + { + id: 'client-defaults', + summary: 'a client keeps RESP3 and the new retry defaults', + reported: ['finding', 'decision'], + severity: 'deprecated', + detect: () => [], + }, + { + id: 'handshake-auth', + summary: 'AUTH moves into HELLO', + reported: ['finding'], + severity: 'breaking', + detect: () => [], + }, + { id: 'node-minimum', summary: 'Node 20 or newer', reported: ['decision'] }, + ], + instructions: '', +}); + +const at = (spec: string, rule: string, severity?: ScoredSite['severity']): ScoredSite => { + const [file, line] = spec.split(':') as [string, string]; + return { file, line: Number(line), rule, ...(severity ? { severity } : {}) }; +}; +const repo = (predicted: ScoredSite[], expected: ScoredSite[]) => ({ + predicted, + expected, + ...scoreSites(predicted, expected), +}); +const needsALook = (spec: string) => at(spec, 'client-defaults', 'deprecated'); + +/** The four scored repositories of the ioredis truth, with what the pack predicted there. */ +const repos = [ + // voiceai: five rebuilt constructions; two predicted at the construction, not the run's first + // line; two constructions changed by pure insertion, which are not sites. + repo( + ['src/cache.ts:101', 'src/cache.ts:108', 'src/jobs.ts:92', 'src/jobs.ts:98'] + .concat(['src/pubsub.ts:140', 'src/limiter.ts:57']) + .map(needsALook), + [ + 'src/cache.ts:98', + 'src/cache.ts:108', + 'src/jobs.ts:89', + 'src/jobs.ts:98', + 'src/vectors.ts:134', + ].map((s) => at(s, 'client-defaults')), + ), + // open-foundry: the truth anchors the rewritten acquisition, the pack the call below it. + repo( + [needsALook('packages/api/src/server.ts:138')], + [at('packages/api/src/server.ts:136', 'client-defaults')], + ), + // dokunc: one construction found; the redaction test it misses is a breaking note's. + repo( + [needsALook('apps/collab/src/server.ts:108'), needsALook('apps/web/src/lib/redis.ts:54')], + [ + at('apps/collab/src/server.ts:108', 'client-defaults'), + at('apps/web/src/lib/log.test.ts:20', 'handshake-auth'), + at('apps/web/src/lib/log.test.ts:28', 'handshake-auth'), + ], + ), + // A clean upgrade: the client it reports was left as it was, by choice. + repo([needsALook('src/lib/cache/client.ts:13')], []), +]; + +describe('behavior-note sites', () => { + it('count as breaking misses only when their note is declared breaking', () => { + const score = scoreTruth(pack, repos); + // Before: 6 breaking false negatives, every client-defaults miss among them. + expect(score.breaking).toMatchObject({ + truePositives: 0, + falsePositives: 0, + falseNegatives: 2, + }); + expect(score.overall).toMatchObject({ truePositives: 3, falsePositives: 7, falseNegatives: 6 }); + expect(score.rules['client-defaults']).toMatchObject({ + truePositives: 3, + falsePositives: 7, + falseNegatives: 4, + }); + }); + + it('are scored on their own: precision and recall of every note site', () => { + const { behavior } = scoreTruth(pack, repos); + expect(behavior).toEqual({ + truePositives: 3, + falsePositives: 7, + falseNegatives: 6, + precision: 0.3, + recall: 1 / 3, + }); + }); + + it('take the severity of the rule or note they are expected under', () => { + expect(siteSeverity(pack, 'duplicate-signature')).toBe('breaking'); + expect(siteSeverity(pack, 'client-defaults')).toBe('deprecated'); + expect(siteSeverity(pack, 'handshake-auth')).toBe('breaking'); + // Never reported as a finding: neither breaking nor deprecated. + expect(siteSeverity(pack, 'node-minimum')).toBe('behavior'); + // `check` reports a detected note without a severity as breaking; so does the scorer. + const unmarked = { ...pack, behavior: [{ ...pack.behavior[1], id: 'x', severity: undefined }] }; + expect(siteSeverity(unmarked as typeof pack, 'x')).toBe('breaking'); + expect(siteSeverity(pack, 'generic')).toBe('breaking'); + }); + + it('leave a decision-only note out of the breaking misses', () => { + const score = scoreTruth(pack, [repo([], [at('package.json:4', 'node-minimum')])]); + expect(score.breaking.falseNegatives).toBe(0); + expect(score.behavior.falseNegatives).toBe(1); + }); +}); + +describe('rateLabel', () => { + it('says n/a for a zero denominator, where the ratio reads 100%', () => { + const none = { + precision: 1, + recall: 0, + truePositives: 0, + falsePositives: 0, + falseNegatives: 2, + }; + expect(rateLabel(none, 'precision')).toBe('n/a'); + expect(rateLabel(none, 'recall')).toBe('0%'); + expect(rateLabel({ ...none, falseNegatives: 0, recall: 1 }, 'recall')).toBe('n/a'); + expect( + rateLabel( + { precision: 0.9, recall: 0.5, truePositives: 9, falsePositives: 1, falseNegatives: 9 }, + 'precision', + ), + ).toBe('90%'); + }); + + it('reads a record written before the counts were kept as it did', () => { + expect(rateLabel({ precision: 1, recall: 0.7375, falsePositives: 0 }, 'precision')).toBe( + '100%', + ); + expect(rateLabel({ precision: 1, recall: 0.7375, falsePositives: 0 }, 'recall')).toBe('74%'); + }); +}); diff --git a/packages/core/src/packs/tooling/pack-test.ts b/packages/core/src/packs/tooling/pack-test.ts index e0fe341..d5b682c 100644 --- a/packages/core/src/packs/tooling/pack-test.ts +++ b/packages/core/src/packs/tooling/pack-test.ts @@ -16,16 +16,21 @@ import { recordedStatus, statusOf, truthDigest, + verificationTally, } from '../contract.js'; import { type FixtureResult, fixturesPass, runFixtures } from './fixtures.js'; import { type EnsureOptions, ensureFixture, ensureRepo, label, truthProblems } from './truth.js'; -/** A site as scored: repository-relative file, line, the rule it belongs to, its severity. */ +/** + * A site as scored: repository-relative file, line, the rule it belongs to, its severity. A + * predicted site has the severity `check` reported; an expected one, its rule's or note's + * (`siteSeverity`), where `behavior` is a note `check` never reports as a finding. + */ export interface ScoredSite { file: string; line: number; rule: string; - severity?: 'breaking' | 'deprecated' | 'unverified'; + severity?: 'breaking' | 'deprecated' | 'unverified' | 'behavior'; } export interface Tally { @@ -83,6 +88,8 @@ export interface PackTestReport { overall: Tally; /** Ground truth, at the line level, breaking findings only: the verified gate. */ breaking: Tally; + /** Ground truth, at the line level, the sites of behavior notes, whatever their severity. */ + behavior: Tally; passed: boolean; } @@ -171,13 +178,87 @@ export function scoreSites( }; } +/** + * The severity of an expected site: its rule's, or its note's as `check` reports the note's + * findings (breaking unless the note says otherwise). A note `check` never reports as a finding + * is `behavior`; a site no rule or note covers (`generic`) is breaking. + */ +export function siteSeverity( + pack: Pick, + rule: string, +): 'breaking' | 'deprecated' | 'behavior' { + const own = pack.rules.find((r) => r.id === rule); + if (own) return own.severity; + const note = pack.behavior.find((b) => b.id === rule); + if (!note) return 'breaking'; + if (note.severity) return note.severity; + return note.detect && note.reported.includes('finding') ? 'breaking' : 'behavior'; +} + +/** + * Every scored repository together: per rule, at the line level, for breaking findings, and + * for the sites of behavior notes. + */ +export function scoreTruth( + pack: Pick, + scored: Pick[], +): Pick { + const all = (k: K) => + scored.flatMap((r) => r[k]); + const fp = all('falsePositives'); + const fn = all('falseNegatives'); + const predicted = all('predicted'); + const overall = tally(predicted.length - fp.length, fp.length, fn.length); + const breakingPredicted = predicted.filter((p) => p.severity === 'breaking'); + const breakingFp = fp.filter((p) => p.severity === 'breaking'); + // A miss is a breaking one when the rule or note it was expected under is. + const breaking = tally( + breakingPredicted.length - breakingFp.length, + breakingFp.length, + fn.filter((f) => siteSeverity(pack, f.rule) === 'breaking').length, + ); + const notes = new Set(pack.behavior.map((b) => b.id)); + const ofNote = (s: ScoredSite) => notes.has(s.rule); + const noteFp = fp.filter(ofNote).length; + const behavior = tally( + predicted.filter(ofNote).length - noteFp, + noteFp, + fn.filter(ofNote).length, + ); + const rules: Record = {}; + const ids = new Set([ + ...pack.rules.map((r) => r.id), + ...pack.behavior.map((b) => b.id), + ...predicted.map((p) => p.rule), + ...all('expected').map((e) => e.rule), + ]); + for (const id of ids) { + let tp = 0; + let rfp = 0; + let rfn = 0; + for (const r of scored) { + const want = new Set(r.expected.filter((e) => e.rule === id).map(key)); + const got = new Set(r.predicted.filter((p) => p.rule === id).map(key)); + for (const k of got) (want.has(k) ? () => tp++ : () => rfp++)(); + for (const k of want) if (!got.has(k)) rfn++; + } + if (tp + rfp + rfn > 0) rules[id] = tally(tp, rfp, rfn); + } + return { rules, overall, breaking, behavior }; +} + async function scoreRepo( pack: Pack, entry: GroundTruthRepo, root: string, options: EnsureOptions, ): Promise { - const expected = entry.findings.map((f) => ({ file: f.file, line: f.line, rule: f.rule })); + const expected = entry.findings.map((f) => ({ + file: f.file, + line: f.line, + rule: f.rule, + severity: siteSeverity(pack, f.rule), + })); const base: RepoScore = { label: label(entry), ...(entry.repo ? { repo: entry.repo } : {}), @@ -295,58 +376,25 @@ export async function testPack( if (!options.fixturesOnly && problems.length === 0) for (const repo of truth.repos) repos.push(await scoreRepo(pack, repo, options.root, options)); - const scored = repos.filter((r) => !r.error); - const all = (k: K) => - scored.flatMap((r) => r[k]); - const fp = all('falsePositives'); - const fn = all('falseNegatives'); - const predicted = all('predicted'); - const overall = tally(predicted.length - fp.length, fp.length, fn.length); - const breakingPredicted = predicted.filter((p) => p.severity === 'breaking'); - const breakingFp = fp.filter((p) => p.severity === 'breaking'); - // A miss is a breaking one unless the rule it was expected under is deprecated. - const deprecatedRules = new Set( - pack.rules.filter((r) => r.severity === 'deprecated').map((r) => r.id), + const { rules, overall, breaking, behavior } = scoreTruth( + pack, + repos.filter((r) => !r.error), ); - const breaking = tally( - breakingPredicted.length - breakingFp.length, - breakingFp.length, - fn.filter((f) => !deprecatedRules.has(f.rule)).length, - ); - const rules: Record = {}; - const ids = new Set([ - ...pack.rules.map((r) => r.id), - ...pack.behavior.map((b) => b.id), - ...predicted.map((p) => p.rule), - ...all('expected').map((e) => e.rule), - ]); - for (const id of ids) { - let tp = 0; - let rfp = 0; - let rfn = 0; - for (const r of scored) { - const want = new Set(r.expected.filter((e) => e.rule === id).map(key)); - const got = new Set(r.predicted.filter((p) => p.rule === id).map(key)); - for (const k of got) (want.has(k) ? () => tp++ : () => rfp++)(); - for (const k of want) if (!got.has(k)) rfn++; - } - if (tp + rfp + rfn > 0) rules[id] = tally(tp, rfp, rfn); - } // A pack whose upgrade leaves part of the group behind produces an inconsistent install. const status: PackStatus = repos.some((r) => r.error || r.leftBehind?.length) || options.fixturesOnly ? 'candidate' - : statusOf(truth, { falsePositives: breakingFp.length, predicted: breakingPredicted.length }); + : statusOf(truth, { + falsePositives: breaking.falsePositives, + predicted: breaking.truePositives + breaking.falsePositives, + }); const verification: PackVerification = { status, truth: read ? truthDigest(read.text) : '', repos: publicRepos(truth).length, - breaking: { - precision: Number(breaking.precision.toFixed(4)), - recall: Number(breaking.recall.toFixed(4)), - falsePositives: breakingFp.length, - }, + breaking: verificationTally(breaking), + behavior: verificationTally(behavior), }; const recorded = recordedStatus(entry.verification); // Fixtures alone say nothing about ground truth: only a full run can find the record stale. @@ -360,7 +408,7 @@ export async function testPack( const passed = scoredAll && fixturesPass(fixtures) && - breakingFp.length === 0 && + breaking.falsePositives === 0 && !repos.some((r) => r.leftBehind?.length) && !stale; return { @@ -377,6 +425,7 @@ export async function testPack( rules, overall, breaking, + behavior, passed, }; } @@ -415,6 +464,7 @@ export function unfinishedPack(entry: RegisteredPack, reason: string): PackTestR rules: {}, overall: none, breaking: none, + behavior: none, passed: false, }; } diff --git a/packages/core/src/packs/tooling/scaffold.ts b/packages/core/src/packs/tooling/scaffold.ts index a7545ef..aad2ffa 100644 --- a/packages/core/src/packs/tooling/scaffold.ts +++ b/packages/core/src/packs/tooling/scaffold.ts @@ -3,7 +3,12 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; import { UptideError } from '../../errors.js'; import { minVersion } from '../../fetch/range.js'; -import { type GroundTruth, type PackVerification, truthDigest } from '../contract.js'; +import { + type GroundTruth, + type PackVerification, + truthDigest, + verificationTally, +} from '../contract.js'; import { packsDir } from './pack-test.js'; import { packConstant, packDir, parseRegistry, renderRegistry } from './registry-file.js'; @@ -148,11 +153,19 @@ export function scaffoldPack(options: ScaffoldOptions): ScaffoldResult { }).status === 0; // What `pack test` records for a pack with no ground-truth repository yet, tied to the // ground-truth file as formatted: the first run finds it current. + const nothing = { + precision: 1, + recall: 1, + truePositives: 0, + falsePositives: 0, + falseNegatives: 0, + }; const verification: PackVerification = { status: 'candidate', truth: truthDigest(readFileSync(join(home, 'ground-truth.json'), 'utf8')), repos: 0, - breaking: { precision: 1, recall: 1, falsePositives: 0 }, + breaking: verificationTally(nothing), + behavior: verificationTally(nothing), }; const verificationFile = join(home, 'verification.json'); writeFileSync(verificationFile, `${JSON.stringify(verification, null, 2)}\n`); diff --git a/scripts/docs-packs.ts b/scripts/docs-packs.ts index 4c53133..aab8eb6 100644 --- a/scripts/docs-packs.ts +++ b/scripts/docs-packs.ts @@ -9,6 +9,7 @@ import { readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { rateLabel } from '../packages/core/src/packs/contract.ts'; import { registeredPacks } from '../packages/core/src/packs/index.ts'; const root = fileURLToPath(new URL('..', import.meta.url)); @@ -29,8 +30,6 @@ function rangeLabel(range: string): string { return range; } -const percent = (n: number): string => `${Math.round(n * 100)}%`; - export function packsTable(): string { const rows = registeredPacks().map(({ dir, pack, verification }) => { const truth = JSON.parse( @@ -41,7 +40,7 @@ export function packsTable(): string { pack.meta.from === pack.meta.to ? rangeLabel(pack.meta.from) : `${rangeLabel(pack.meta.from)} → ${rangeLabel(pack.meta.to)}`; - return `| \`${pack.meta.package}\` | ${range} | ${percent(verification.breaking.precision)} | ${percent(verification.breaking.recall)} | ${repos.join(', ')} | ${verification.status} |`; + return `| \`${pack.meta.package}\` | ${range} | ${rateLabel(verification.breaking, 'precision')} | ${rateLabel(verification.breaking, 'recall')} | ${repos.join(', ')} | ${verification.status} |`; }); return [ '| Package | Range | Precision | Recall | Ground-truth repositories | Status |', From 34bc36187a3e3932242bb18b2f4ce012f5ae64a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 22:28:52 +0000 Subject: [PATCH 2/4] chore(packs): record behavior-note tallies and the counts behind each rate `uptide pack test --write` for every registered pack, against the same ground truth: each verification.json gains `behavior` and the true-positive and false-negative counts under `breaking`. Status, truth digest, repositories, and breaking precision, recall and false positives are unchanged for all ten packs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011t9c3ZLsM9B3KiYGPFDyyK Signed-off-by: Claude --- packages/core/src/packs/ai/verification.json | 11 ++++++++++- .../core/src/packs/lucide-react/verification.json | 11 ++++++++++- packages/core/src/packs/next/verification.json | 11 ++++++++++- packages/core/src/packs/react/verification.json | 11 ++++++++++- .../core/src/packs/sentry-browser/verification.json | 11 ++++++++++- .../core/src/packs/sentry-nextjs/verification.json | 11 ++++++++++- .../core/src/packs/sentry-react/verification.json | 11 ++++++++++- packages/core/src/packs/stripe/verification.json | 11 ++++++++++- packages/core/src/packs/vitest/verification.json | 11 ++++++++++- packages/core/src/packs/zod/verification.json | 11 ++++++++++- 10 files changed, 100 insertions(+), 10 deletions(-) diff --git a/packages/core/src/packs/ai/verification.json b/packages/core/src/packs/ai/verification.json index 21cdaa4..32312cd 100644 --- a/packages/core/src/packs/ai/verification.json +++ b/packages/core/src/packs/ai/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.8182, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 9, + "falseNegatives": 2 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/lucide-react/verification.json b/packages/core/src/packs/lucide-react/verification.json index c809202..12a9308 100644 --- a/packages/core/src/packs/lucide-react/verification.json +++ b/packages/core/src/packs/lucide-react/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 1, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 11, + "falseNegatives": 0 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/next/verification.json b/packages/core/src/packs/next/verification.json index 876641d..abffd1c 100644 --- a/packages/core/src/packs/next/verification.json +++ b/packages/core/src/packs/next/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.8462, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 11, + "falseNegatives": 2 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/react/verification.json b/packages/core/src/packs/react/verification.json index 49069a8..0842e0c 100644 --- a/packages/core/src/packs/react/verification.json +++ b/packages/core/src/packs/react/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.6899, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 89, + "falseNegatives": 40 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/sentry-browser/verification.json b/packages/core/src/packs/sentry-browser/verification.json index 80a0d09..c7b547d 100644 --- a/packages/core/src/packs/sentry-browser/verification.json +++ b/packages/core/src/packs/sentry-browser/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 1, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 3, + "falseNegatives": 0 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/sentry-nextjs/verification.json b/packages/core/src/packs/sentry-nextjs/verification.json index 12c3fef..5d3e3c6 100644 --- a/packages/core/src/packs/sentry-nextjs/verification.json +++ b/packages/core/src/packs/sentry-nextjs/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 1, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 8, + "falseNegatives": 0 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/sentry-react/verification.json b/packages/core/src/packs/sentry-react/verification.json index 18d96ef..5994af5 100644 --- a/packages/core/src/packs/sentry-react/verification.json +++ b/packages/core/src/packs/sentry-react/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 1, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 2, + "falseNegatives": 0 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/stripe/verification.json b/packages/core/src/packs/stripe/verification.json index 9036546..6774abc 100644 --- a/packages/core/src/packs/stripe/verification.json +++ b/packages/core/src/packs/stripe/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.9, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 18, + "falseNegatives": 2 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } diff --git a/packages/core/src/packs/vitest/verification.json b/packages/core/src/packs/vitest/verification.json index 515ceeb..71f0b3c 100644 --- a/packages/core/src/packs/vitest/verification.json +++ b/packages/core/src/packs/vitest/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.5, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 16, + "falseNegatives": 16 + }, + "behavior": { + "precision": 1, + "recall": 0, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 13 } } diff --git a/packages/core/src/packs/zod/verification.json b/packages/core/src/packs/zod/verification.json index 0570c61..6eb5ce7 100644 --- a/packages/core/src/packs/zod/verification.json +++ b/packages/core/src/packs/zod/verification.json @@ -5,6 +5,15 @@ "breaking": { "precision": 1, "recall": 0.7375, - "falsePositives": 0 + "falsePositives": 0, + "truePositives": 59, + "falseNegatives": 21 + }, + "behavior": { + "precision": 1, + "recall": 1, + "falsePositives": 0, + "truePositives": 0, + "falseNegatives": 0 } } From 9308233f569baacfbf8eda87561733e52c764d3e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 22:28:52 +0000 Subject: [PATCH 3/4] docs(contributing): run pack test locally What a full `pnpm uptide pack test` run needs on a contributor's machine: corepack enabled for Yarn only (a blanket `corepack enable` shims the global pnpm too), COREPACK_NPM_REGISTRY where repo.yarnpkg.com is blocked, and per-pack cache directories evicted after each pack to bound disk. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011t9c3ZLsM9B3KiYGPFDyyK Signed-off-by: Claude --- CONTRIBUTING.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ac2ec04..8ea3097 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -156,6 +156,31 @@ open a **Pack request** issue, and say you are writing it. `pnpm uptide` runs the CLI from source, so nothing needs building while you work. +## Run pack test locally + +`pnpm uptide pack test` installs each ground-truth repository with the package manager its +lockfile names, scripts off. For a full run: + +- **Corepack for Yarn only.** Repositories that pin Yarn Berry in `packageManager` (tldraw, + `yarn@4.7.0`) fail under Yarn 1. Run `corepack enable yarn`, not `corepack enable`: that + also replaces your global pnpm with a shim, and tests that run pnpm outside this checkout + then try to download a pnpm of corepack's choosing. +- **Where Yarn 4 comes from.** Corepack fetches Yarn 2 and later from repo.yarnpkg.com. Where + that host is blocked (a proxy, an allowlist), fetch it from npm instead: + `export COREPACK_NPM_REGISTRY=https://registry.npmjs.org COREPACK_ENABLE_DOWNLOAD_PROMPT=0`. +- **Disk.** Each repository stays cached with its dependencies installed, 2 to 8 GB per pack. + To score every pack on a bounded disk, give each pack its own caches and delete them after: + ```sh + for p in ai zod; do # each pack you score + export UPTIDE_GROUND_TRUTH_CACHE=/tmp/uptide-truth/$p UPTIDE_CACHE_DIR=/tmp/uptide-cache/$p + pnpm uptide pack test $p + rm -rf "$UPTIDE_GROUND_TRUTH_CACHE" "$UPTIDE_CACHE_DIR" + done + ``` + Comparing two checkouts (a before and after table), score each pack in both before you + delete its caches. In-tree fixtures (`fixtures/repos/storefront`) are installed inside the + checkout itself, so run each checkout once without `--offline`. + ## Rules that do not bend These are what make Uptide safe to run on someone else's code. A pull request that breaks From 28fb60ac702566cd53a2c7a17d06124bf3458011 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 20:15:35 +0000 Subject: [PATCH 4/4] feat(packs): anchor a pure insertion into an unchanged call as a ground-truth site docs/packs.md counted no pure insertion as a site, so an upgrade that only adds an option inside an existing call (open-foundry pinned `protocol: 2` in its Redis constructor) could not be written as ground truth: a pack finding at that call always scored as a false positive. Such an insertion is now a site at the start line of the innermost call (or `new`) whose parentheses hold it, through object and array literals, or of an object literal no call takes. Insertions between statements, into a function body, or into a call the upgrade also changed stay out. The diff reading moves from scripts/packs-truth.ts into packs/tooling/truth-diff.ts, and `pnpm packs:truth` prints the drafted `anchors` for review. Scoring is unchanged: sites are lines. Fixes UPT-53. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011t9c3ZLsM9B3KiYGPFDyyK Signed-off-by: Claude --- .changeset/pack-truth-insertion-anchors.md | 5 + CONTRIBUTING.md | 4 +- docs/packs.md | 15 +- packages/core/src/index.ts | 8 + .../core/src/packs/tooling/truth-diff.test.ts | 159 +++++++++++++++++ packages/core/src/packs/tooling/truth-diff.ts | 161 ++++++++++++++++++ scripts/packs-truth.ts | 49 +++--- 7 files changed, 369 insertions(+), 32 deletions(-) create mode 100644 .changeset/pack-truth-insertion-anchors.md create mode 100644 packages/core/src/packs/tooling/truth-diff.test.ts create mode 100644 packages/core/src/packs/tooling/truth-diff.ts diff --git a/.changeset/pack-truth-insertion-anchors.md b/.changeset/pack-truth-insertion-anchors.md new file mode 100644 index 0000000..1cecc1c --- /dev/null +++ b/.changeset/pack-truth-insertion-anchors.md @@ -0,0 +1,5 @@ +--- +"uptide": patch +--- + +Ground truth can name a site for an upgrade that only adds lines inside an existing call, such as an option added to a constructor: the site is the call's start line, so a pack finding there scores as a true positive instead of a false positive. `pnpm packs:truth` drafts these anchors from insert-only hunks for review, and docs/packs.md and CONTRIBUTING describe the rule. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8ea3097..f123152 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -132,7 +132,9 @@ open a **Pack request** issue, and say you are writing it. Add `--also @` for each package the upgrade commit moved together with it (its lockfile has the versions). Read each site, drop what is not about the package, and give each its rule (`generic` - when no rule of the pack covers it). (15 minutes) + when no rule of the pack covers it). An option the upgrade only added inside an existing + call is drafted under `anchors`, at the call's start line; keep it like any other site. + (15 minutes) 6. **Score it.** ```sh pnpm uptide pack test # fetches the repositories once, scripts off diff --git a/docs/packs.md b/docs/packs.md index 9bd8b01..f19b230 100644 --- a/docs/packs.md +++ b/docs/packs.md @@ -198,8 +198,14 @@ Where the expected findings come from: two sources that are not Uptide. the old side). One site per change: consecutive changed lines of the same change are one site, at the first line of code (a line that is plainly another rule's starts a new one); a run with a diagnostic inside it, or right after a diagnostic of the same rule, is - that diagnostic's site. Comment-only lines, pure insertions (no line at ``), and - changes for another package are not sites. + that diagnostic's site. Comment-only lines and changes for another package are not sites. + A pure insertion (no line at ``) is a site only when it adds to a call or object + the upgrade otherwise left unchanged, such as an option added to a constructor: + `new Redis(url, {` at line 138 that gains `protocol: 2` is a site at 138. The anchor is the + start line of the innermost call (or `new`) whose parentheses hold the inserted lines, + through any object and array literals, or of the object literal when no call takes it. + An insertion between statements or into a function body is not a site, nor is one into a + call the upgrade also changed: that call's changed lines are its site. `pnpm packs:truth` drafts both for one repository: @@ -209,8 +215,9 @@ pnpm packs:truth ai vercel/chatbot --to 7.0.9 \ --also @ai-sdk/react@4.0.10 --also @ai-sdk/provider@4.0.1 ``` -It prints the compiler's new diagnostics with each line's text, and the changed and inserted -lines. You read each one, drop what is not about the package, and give every kept site its +It prints the compiler's new diagnostics with each line's text, the changed and inserted +lines, and `anchors`: each insertion into an unchanged call or object, at the line the rule +above anchors it, with the lines inserted there. You read each one, drop what is not about the package, and give every kept site its rule, from its code and text. A site the pack has no rule for is expected under `generic`. The expected findings never come from Uptide's output: a site `check` reports that neither source has is a false positive, and it stays one. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fdcb298..f6bbbb6 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -215,6 +215,14 @@ export { type PinnedRepo, truthCacheDir, } from './packs/tooling/truth.js'; +export { + type ChangedLine, + diffLines, + enclosingLiteral, + type InsertedLine, + type InsertionAnchor, + insertionAnchors, +} from './packs/tooling/truth-diff.js'; export type { MigrationPack, MigrationRule } from './packs/types.js'; export { zodPack } from './packs/zod/index.js'; export { type PlanOptions, type PlanServices, upgradePlan } from './plan/gather.js'; diff --git a/packages/core/src/packs/tooling/truth-diff.test.ts b/packages/core/src/packs/tooling/truth-diff.test.ts new file mode 100644 index 0000000..3aba5d5 --- /dev/null +++ b/packages/core/src/packs/tooling/truth-diff.test.ts @@ -0,0 +1,159 @@ +import { describe, expect, it } from 'vitest'; +import { definePack } from '../contract.js'; +import { scoreSites, scoreTruth } from './pack-test.js'; +import { diffLines, enclosingLiteral, insertionAnchors } from './truth-diff.js'; + +/** `body` placed so that its first line is line `first` of the file. */ +const at = (first: number, body: string) => + `${Array.from({ length: first - 1 }, (_, i) => `// ${i + 1}`).join('\n')}\n${body}`; + +// Synthetic, modeled on open-foundry's ioredis 5 → 6 upgrade (packages/api/src/server.ts): the +// commit rewrote how the client constructor is obtained (135–137) and inserted `protocol: 2` +// into the unchanged constructor call that starts at 138. +const server = at( + 134, + ` if (url) { + // the client is optional + const mod = await import('redis-like'); + const Client = mod.default as unknown as new (url: string, o: object) => Client; + client = new Client(url, { + // fail fast + maxRetriesPerRequest: 1, + enableOfflineQueue: false, + }); + limiter = new Limiter(client); + } +`, +); +const diff = `diff --git a/src/server.ts b/src/server.ts +--- a/src/server.ts ++++ b/src/server.ts +@@ -135,3 +135,2 @@ async function main(): Promise { +- // the client is optional +- const mod = await import('redis-like'); +- const Client = mod.default as unknown as new (url: string, o: object) => Client; ++ // the client stays optional ++ const { Client } = await import('redis-like'); +@@ -138,0 +138,2 @@ async function main(): Promise { ++ // keep the v5 wire protocol ++ protocol: 2, +`; + +describe('an option added inside an unchanged call', () => { + const lines = diffLines(diff); + const anchors = insertionAnchors(lines, (file) => + file === 'src/server.ts' ? server : undefined, + ); + + it('is drafted as a site at the call, next to the rewritten lines above it', () => { + expect(lines.changed.map((c) => c.line)).toEqual([135, 136, 137]); + expect(lines.inserted.map((i) => [i.after, i.text])).toEqual([ + [138, '// keep the v5 wire protocol'], + [138, 'protocol: 2,'], + ]); + expect(anchors).toEqual([ + { + file: 'src/server.ts', + line: 138, + text: 'client = new Client(url, {', + within: 'call', + inserted: [ + { after: 138, text: '// keep the v5 wire protocol' }, + { after: 138, text: 'protocol: 2,' }, + ], + }, + ]); + }); + + it('scores the prediction at the call as a true positive once the truth anchors it there', () => { + const pack = definePack({ + meta: { + package: 'redis-like', + from: '>=5 <6', + to: '>=6 <7', + sources: [{ title: 'guide', url: 'https://example.com/guide' }], + maintainer: 'uptide-dev', + }, + rules: [], + behavior: [ + { + id: 'client-defaults', + summary: 'a client keeps RESP3', + reported: ['finding'], + severity: 'deprecated', + detect: () => [], + }, + ], + instructions: '', + }); + const predicted = [ + { + file: 'src/server.ts', + line: 138, + rule: 'client-defaults', + severity: 'deprecated' as const, + }, + ]; + const site = (line: number) => ({ file: 'src/server.ts', line, rule: 'client-defaults' }); + // Before: the rewritten run alone (136) is a site, and the call is a false positive. + const before = scoreSites(predicted, [site(136)]); + expect(before.falsePositives.map((s) => s.line)).toEqual([138]); + // After: the insertion's anchor is a site too. + const expected = [site(136), ...anchors.map((a) => site(a.line))]; + const score = scoreTruth(pack, [{ predicted, expected, ...scoreSites(predicted, expected) }]); + expect(score.rules['client-defaults']).toMatchObject({ + truePositives: 1, + falsePositives: 0, + falseNegatives: 1, + }); + }); +}); + +describe('enclosingLiteral', () => { + const text = `const a = 1; +const client = new Client({ + tls: { + ca, + }, +}); +const options = { + retry: 1, +}; +app.use((req) => { + log(req); +}); +function start() { + connect(); +} +configure( + url, +); +`; + + it('anchors at the call through nested object literals, and at a lone object', () => { + expect(enclosingLiteral('a.ts', text, 2)).toEqual({ line: 2, endLine: 6, within: 'call' }); + expect(enclosingLiteral('a.ts', text, 4)).toEqual({ line: 2, endLine: 6, within: 'call' }); + expect(enclosingLiteral('a.ts', text, 7)).toEqual({ line: 7, endLine: 9, within: 'object' }); + // A new argument between the parentheses. + expect(enclosingLiteral('a.ts', text, 17)).toEqual({ line: 16, endLine: 18, within: 'call' }); + }); + + it('anchors nothing between statements or inside a function body, even one passed to a call', () => { + expect(enclosingLiteral('a.ts', text, 1)).toBeUndefined(); + expect(enclosingLiteral('a.ts', text, 6)).toBeUndefined(); + expect(enclosingLiteral('a.ts', text, 10)).toBeUndefined(); + expect(enclosingLiteral('a.ts', text, 13)).toBeUndefined(); + expect(enclosingLiteral('a.ts', text, 19)).toBeUndefined(); + }); + + it('leaves an insertion into a call the upgrade also changed to that call’s changed lines', () => { + const changed = [{ file: 'a.ts', line: 3, text: 'tls: {' }]; + const inserted = [{ file: 'a.ts', after: 4, text: 'rejectUnauthorized: false,' }]; + expect(insertionAnchors({ changed, inserted }, () => text)).toEqual([]); + }); + + it('reads JSX', () => { + const tsx = 'export const App = () => render(\n ,\n el,\n);\n'; + expect(enclosingLiteral('app.tsx', tsx, 2)).toEqual({ line: 1, endLine: 4, within: 'call' }); + }); +}); diff --git a/packages/core/src/packs/tooling/truth-diff.ts b/packages/core/src/packs/tooling/truth-diff.ts new file mode 100644 index 0000000..6d29491 --- /dev/null +++ b/packages/core/src/packs/tooling/truth-diff.ts @@ -0,0 +1,161 @@ +import { ts } from 'ts-morph'; + +/** The source files `packs:truth` reads a diff of. */ +export const SOURCE_FILE = /\.[cm]?[jt]sx?$/; + +/** A line of `` the upgrade changed or removed. */ +export interface ChangedLine { + file: string; + line: number; + text: string; +} + +/** A line the upgrade added with no line of `` beside it: after old line `after`. */ +export interface InsertedLine { + file: string; + after: number; + text: string; +} + +/** + * Where a pure insertion inside an unchanged call or object is a site: the start line of the + * call (`within: 'call'`), or of an object literal no call takes (`within: 'object'`). + */ +export interface InsertionAnchor { + file: string; + line: number; + /** The anchor line's text in ``. */ + text: string; + within: 'call' | 'object'; + inserted: { after: number; text: string }[]; +} + +/** + * The old side of `git diff -U0 `, in source files: every changed or + * removed line, and every pure insertion (a hunk with no old line). Paths are relative to + * `directory` when given; files outside it are left out. + */ +export function diffLines( + diff: string, + directory?: string, +): { changed: ChangedLine[]; inserted: InsertedLine[] } { + const changed: ChangedLine[] = []; + const inserted: InsertedLine[] = []; + let file = ''; + let oldLine = 0; + let newRun = false; + for (const line of diff.split('\n')) { + if (line.startsWith('--- ')) continue; + if (line.startsWith('+++ ')) { + file = line.slice(4).replace(/^b\//, ''); + continue; + } + const hunk = /^@@ -(\d+)(?:,(\d+))? \+\d+(?:,\d+)? @@/.exec(line); + if (hunk) { + oldLine = Number(hunk[1]); + newRun = hunk[2] === '0'; + if (newRun) oldLine += 1; + continue; + } + if (!SOURCE_FILE.test(file) || (directory && !file.startsWith(`${directory}/`))) continue; + const where = directory ? file.slice(directory.length + 1) : file; + if (line.startsWith('-')) + changed.push({ file: where, line: oldLine++, text: line.slice(1).trim() }); + else if (line.startsWith('+') && newRun) + inserted.push({ file: where, after: oldLine - 1, text: line.slice(1).trim() }); + } + return { changed, inserted }; +} + +/** Nodes an insertion can sit in and still be part of the call or object around them. */ +const TRANSPARENT = new Set([ + ts.SyntaxKind.ObjectLiteralExpression, + ts.SyntaxKind.ArrayLiteralExpression, + ts.SyntaxKind.PropertyAssignment, + ts.SyntaxKind.ShorthandPropertyAssignment, + ts.SyntaxKind.SpreadAssignment, + ts.SyntaxKind.SpreadElement, +]); + +function scriptKind(file: string): ts.ScriptKind { + if (file.endsWith('.tsx')) return ts.ScriptKind.TSX; + if (file.endsWith('.jsx')) return ts.ScriptKind.JSX; + return /\.[cm]?js$/.test(file) ? ts.ScriptKind.JS : ts.ScriptKind.TS; +} + +/** + * The call or object an insertion after line `after` (1-based) of `text` adds to, with its + * first and last line: the innermost call or `new` whose argument list holds it, through any + * object and array literals; else the outermost object literal holding it. None when the lines + * go between statements, into a function body, or anywhere else. + */ +export function enclosingLiteral( + file: string, + text: string, + after: number, +): { line: number; endLine: number; within: 'call' | 'object' } | undefined { + const source = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, scriptKind(file)); + const starts = source.getLineStarts(); + const at = after >= starts.length ? text.length : (starts[after] ?? text.length); + const holds = (n: ts.Node) => n.getStart(source) < at && at < n.end; + let node: ts.Node = source; + for (;;) { + const inner: ts.Node | undefined = ts.forEachChild(node, (c) => (holds(c) ? c : undefined)); + if (!inner) break; + node = inner; + } + const lines = (n: ts.Node) => ({ + line: source.getLineAndCharacterOfPosition(n.getStart(source)).line + 1, + endLine: source.getLineAndCharacterOfPosition(n.end).line + 1, + }); + let object: ts.Node | undefined; + for (let n: ts.Node | undefined = node; n; n = n.parent) { + if (ts.isCallExpression(n) || ts.isNewExpression(n)) { + // Between the parentheses (the arguments start after `(`, the call ends at `)`): not in + // the callee or its type arguments. + const open = n.arguments?.pos; + return open !== undefined && open <= at && at < n.end + ? { ...lines(n), within: 'call' } + : undefined; + } + if (!TRANSPARENT.has(n.kind)) break; + if (ts.isObjectLiteralExpression(n)) object = n; + } + return object ? { ...lines(object), within: 'object' } : undefined; +} + +/** + * Pure insertions that add to a call or object the upgrade left otherwise unchanged (an option + * added to a constructor), anchored at the call's start line: the site `docs/packs.md` says + * such a change has. One anchor per call, with every line inserted into it. An insertion into a + * call the upgrade also changed is not anchored: its changed lines are the site. + */ +export function insertionAnchors( + lines: { changed: ChangedLine[]; inserted: InsertedLine[] }, + read: (file: string) => string | undefined, +): InsertionAnchor[] { + const anchors = new Map(); + const texts = new Map(); + for (const insertion of lines.inserted) { + if (!texts.has(insertion.file)) texts.set(insertion.file, read(insertion.file)); + const text = texts.get(insertion.file); + if (text === undefined) continue; + const literal = enclosingLiteral(insertion.file, text, insertion.after); + if (!literal) continue; + const touched = lines.changed.some( + (c) => c.file === insertion.file && c.line >= literal.line && c.line <= literal.endLine, + ); + if (touched) continue; + const key = `${insertion.file}:${literal.line}`; + const anchor = anchors.get(key) ?? { + file: insertion.file, + line: literal.line, + text: (text.split('\n')[literal.line - 1] ?? '').trim(), + within: literal.within, + inserted: [], + }; + anchor.inserted.push({ after: insertion.after, text: insertion.text }); + anchors.set(key, anchor); + } + return [...anchors.values()]; +} diff --git a/scripts/packs-truth.ts b/scripts/packs-truth.ts index a361019..e257f90 100644 --- a/scripts/packs-truth.ts +++ b/scripts/packs-truth.ts @@ -8,6 +8,9 @@ * - changed: every line of `` the upgrade commit changed or removed (`git diff -U0`, * the old side), in source files. Pure insertions have no line in `` and are listed * apart. + * - anchors: the pure insertions that add to a call or object the upgrade left otherwise + * unchanged (an option added to a constructor), at the call's start line: the site such a + * change has (docs/packs.md, "Ground truth"), drafted for review like the rest. * - compiler: the diagnostics the repository's own TypeScript reports with the target version * swapped in and not without it, errors and deprecations (6385, 6387), keyed on file, code * and message. The target is linked where the repository has the package installed, as an @@ -32,7 +35,15 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, join, relative } from 'node:path'; -import { createNpmFetcher, ensureRepo, maxSatisfying, satisfies, validRange } from '@uptide/core'; +import { + createNpmFetcher, + diffLines, + ensureRepo, + insertionAnchors, + maxSatisfying, + satisfies, + validRange, +} from '@uptide/core'; import { compilerDiagnostics, type Diagnostic } from './packs-truth-compiler.ts'; const args = process.argv.slice(2); @@ -65,32 +76,15 @@ const git = (...a: string[]) => maxBuffer: 256 * 1024 * 1024, }); git('fetch', '-q', '--depth', '1', '--no-tags', 'origin', upgrade); -const SOURCE = /\.[cm]?[jt]sx?$/; -const changed: { file: string; line: number; text: string }[] = []; -const inserted: { file: string; after: number; text: string }[] = []; -let file = ''; -let oldLine = 0; -let newRun = false; -for (const line of git('diff', '-U0', '--no-color', '--no-ext-diff', commit, upgrade).split('\n')) { - if (line.startsWith('--- ')) continue; - if (line.startsWith('+++ ')) { - file = line.slice(4).replace(/^b\//, ''); - continue; - } - const hunk = /^@@ -(\d+)(?:,(\d+))? \+\d+(?:,\d+)? @@/.exec(line); - if (hunk) { - oldLine = Number(hunk[1]); - newRun = hunk[2] === '0'; - if (newRun) oldLine += 1; - continue; - } - if (!SOURCE.test(file) || (directory && !file.startsWith(`${directory}/`))) continue; - const where = directory ? relative(directory, file) : file; - if (line.startsWith('-')) - changed.push({ file: where, line: oldLine++, text: line.slice(1).trim() }); - else if (line.startsWith('+') && newRun) - inserted.push({ file: where, after: oldLine - 1, text: line.slice(1).trim() }); -} +const { changed, inserted } = diffLines( + git('diff', '-U0', '--no-color', '--no-ext-diff', commit, upgrade), + directory, +); +// Read at ``, which is what is checked out. +const anchors = insertionAnchors({ changed, inserted }, (file) => { + const path = join(project, file); + return existsSync(path) ? readFileSync(path, 'utf8') : undefined; +}); // Where the repository has the package: the root's node_modules and each workspace's. function installs(name: string, dir: string, depth = 0): string[] { @@ -236,6 +230,7 @@ console.log( })), changed, inserted, + anchors, }, null, 2,