Skip to content

feat(release): require a version bump for a manifest or export change - #642

Closed
drewstone wants to merge 3 commits into
mainfrom
feat/export-surface-gate
Closed

feat(release): require a version bump for a manifest or export change#642
drewstone wants to merge 3 commits into
mainfrom
feat/export-surface-gate

Conversation

@drewstone

@drewstone drewstone commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

The gap

This repository had no version-bump check at all. Two consumer-visible changes could ship under a version npm already holds, and nothing would go red:

  1. a manifest change — a peer range, a dependency pin;
  2. a change to the exported symbols, which is invisible even to a manifest check, because adding an export leaves every published manifest field byte-identical.

Shape 2 is what cost the fleet two days. agent-runtime #893 added and exported PursuitProjection, PursuitRunProjection and PursuitNodeProjection and merged at 0.140.0, a version npm already held. A downstream session then spent hours proving those types resolved in no published version before concluding, wrongly, that Runtime had to write them. They existed; the release did not. agent-eval publishes exactly the same way.

The change

Ported from agent-runtime rather than reinvented — scripts/check-version-bump.mjs, scripts/check-api-surface.mjs and scripts/lib/api-surface.mjs are byte-identical to the files in agent-runtime and agent-knowledge, verified by sha256:

0f3823271d78c6525b8821d4dc442f6104ef0c11b742c8d314e20a2b66a6cce4  scripts/check-version-bump.mjs
246a12a2a81a31a1eafa2c3fa31a91700ba7f4599a3b74cd4195ce4332822a76  scripts/check-api-surface.mjs
6a3807fe76bee4c4a9a700f612520418b801fc3feed41d005e932f19026ab0fd  scripts/lib/api-surface.mjs

They read everything repo-specific out of the manifest under inspection and name no repository, so a diff between the three repos is proof of drift, not expected variation. tests/version-bump-check.test.ts is identical too — 27 tests, all against a throwaway fixture repo.

A shared dependency was not available. agent-eval may not depend on agent-runtime — zero upward dependencies, devDeps included — and a new published package was out of scope. agent-eval could host the implementation for the others, but only by shipping repo tooling in its own tarball and then gating itself against a published copy one release stale.

Why oxc-parser and not the TypeScript compiler API: this repository is on TypeScript 7, which ships no JavaScript compiler API — ts.createProgram is undefined. A checker-based extractor would work in agent-runtime (TypeScript 6) and need rewriting here, which is precisely how one rule grows three different bugs.

How it works

api-surface.json records the symbols each entry point exports, with each name's kind, generated from the built declarations.

  • check:version-bump compares that record against the merge base and requires the level the change implies. Needs nothing but git.
  • check:api-surface regenerates it from dist and fails when the two disagree, so a stale record cannot hide an export.

Both run inside verify:package, which CI already runs after the build. No workflow was added.

The level is read off the consumer's compatibility boundary. At 0.147.0 a >=0.147.0 <0.148.0 window already reaches a patch, so an added export costs a patch and a removal costs a minor. At 1.0+ those become minor and major.

./openapi.json has no declarations and is recorded as an asset entry, not silently dropped.

Negative control

A — export added, version left at 0.147.0

$ PACKAGE_VERSION_BUMP_BASE=feat/export-surface-gate node scripts/check-version-bump.mjs
A consumer-visible change must ship under a higher version.

package.json (@tangle-network/agent-eval) still declares 0.147.0, but against
feat/export-surface-gate (05d1bfa6b19f) it makes an additive change needing a patch bump:
  export added: . EvalNegativeControlProbe
EXIT=1

B — same export, 0.147.0 -> 0.147.1

Consumer-visible package surfaces carry their version bumps (against feat/export-surface-gate (05d1bfa6b19f)).
  package.json: 0 manifest and 1 export change(s) needing a patch bump, paid for by 0.147.0 -> 0.147.1 (patch)
EXIT=0

The probe commits were removed; the branch carries none of them.

The one CI line

actions/checkout defaulted to depth 1, which does not create origin/main, so a merge-base check cannot resolve a base and fails closed on every PR. The checkout in the ci job gains fetch-depth: 0. No job, workflow, or check was added.

Publishing is unaffected: publish.yml runs on push and workflow_dispatch, both baseless events, so the check reports "does not apply" and exits 0. Confirmed against a fixture with no origin/main.

Verification

  • pnpm typecheck clean
  • vitest run tests/version-bump-check.test.ts — 27 passed
  • check:api-surface current: 3423 exports across 27 entry points

Companion PRs: tangle-network/agent-runtime#900, and the agent-knowledge port.

tangletools
tangletools previously approved these changes Aug 18, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 05d1bfa6

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-18T20:06:14Z

@drewstone

Copy link
Copy Markdown
Contributor Author

@tangletools review now

1 similar comment
@drewstone

Copy link
Copy Markdown
Contributor Author

@tangletools review now

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Value Audit did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: agent returned no usable verdict; usefulness: agent returned no usable verdict
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent produced no parseable value-audit JSON.

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 5
  • Bridge error: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: opencode: opencode error; opencode/deepseek/deepseek-v4-pro: opencode: opencode error

🎯 Usefulness — error

usefulness agent produced no parseable value-audit JSON.

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 5
  • Bridge error: opencode/zai-coding-plan/glm-5.2: opencode: opencode error; opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/deepseek/deepseek-v4-pro: opencode: opencode error

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260818T203545Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.1s
Interrogation 421.0s (2 bridge agents)
Total 421.1s

💰 Value — sound-with-nits

Adds a merge-base gate that fails any consumer-visible change (manifest field or exported symbol) shipped without the version bump it implies, closing the 'merged but never published' hole that publish.yml's registry-skip creates — ported from the sibling repos and verified working here; ship.

  • What it does: Two checks wired into verify:package (package.json:183-186), which runs in CI after build (.github/workflows/ci.yml:59-62) and again in publish.yml:49. check:version-bump (scripts/check-version-bump.mjs) diffs every publishable manifest's consumer-visible fields — npm-copied fields listed at :95-123, with catalog: pins resolved through pnpm-workspace.yaml (:250-263) so an indirect version move c
  • Goals it achieves: Close a real, verified release-integrity hole: publish.yml skips any version already on the registry (.github/workflows/publish.yml:170-193), so a merged change with no bump silently never reaches consumers — the PR body documents this costing two fleet days when exports merged at an already-held 0.140.0. Once merged, neither a manifest change (peer range, dep pin) nor an added export can land wit
  • Assessment: Good, and in the grain. I verified the claims: 27/27 tests pass (vitest, fixture repos), check:version-bump runs clean against origin/main on this branch, and the record covers all 27 entry points with ./openapi.json correctly filed as an asset. The design choices are each argued and sound: a committed record instead of a two-build comparison (lib/api-surface.mjs:1-30 — one git-show, no second che
  • Better / existing approach: none — this is the right approach for now. Searched: (1) repo-internal duplication — none (verifyVersionLock, publint, attw, check-analyst-benchmark all inspected, no ref-diff gate existed); (2) ecosystem tooling — microsoft/api-extractor's API report does the committed-surface-snapshot job, but it requires the TS compiler API, which TS7/tsgo does not provide, so it cannot serve this repo set; (3)
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 5
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A byte-verified port of agent-runtime's release gate that closes a real, demonstrated defect class (exports shipping under a version npm already holds), wired into both CI and the publish path and proven green on this very PR.

  • Integration: Fully wired and reachable now. package.json:183-186 adds check:version-bump as the first step of verify:package and check:api-surface after it, plus the api:surface regeneration entry. verify:package runs on every PR and push to main (.github/workflows/ci.yml:62) after the build (ci.yml:59), and again on the publish path (.github/workflows/publish.yml:49). ci.yml:14-17 adds fetch-depth: 0 so the m
  • Fit with existing patterns: Fits the grain exactly, on two axes. (1) Repo pattern: this repo already runs a verify:package check family (check:skill, check:model-ids, check:analyst-benchmark at package.json:179-182); this slots into the established pipeline rather than inventing a new one. (2) Fleet pattern: I verified the byte-identical claim — sha256s of all three scripts (0f3823…, 246a12…, 6a3807…) match agent-runtime ori
  • Real-world viability: Built for the failure paths, not the happy path. The checker fails closed at every ambiguity I probed: CI events with no base branch (check-version-bump.mjs:162-172), unresolvable base ref (:178-183), unresolvable catalog pin (:256-261), a publishable package with no api-surface.json record (:396-409), a stale record vs built dist (check-api-surface.mjs:209-222), and wildcard export patterns match
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2

💰 Value Audit

🟡 The three-repo byte-identical invariant has no automated enforcement [maintenance] ``

check-version-bump.mjs:59-62 states the files are kept byte-identical across agent-eval/agent-knowledge/agent-runtime and that 'an edit to one belongs in all three', but nothing checks it — I found agent-runtime worktrees still carrying the older 415-line manifest-only version (sha 7c6852…), so drift is the resting state until each repo re-ports. A cheap improvement: a periodic job (or release-time step) that hashes the three copies and files an alert on divergence, or fold that into the existin

🟡 Single-package repo carries multi-workspace generality it cannot exercise [proportion] ``

agent-eval is one publishable package, yet check-version-bump.mjs carries workspace enumeration, per-package records keyed by name, and union-of-publishables logic (:223-243, :369-375). That generality is what makes the three copies identical rather than divergent, so it is a deliberate trade, not accidental — but it means ~30% of the file is dead weight in this repo and every future reader must reason past it. Acceptable as the price of the identity invariant; revisit only if the shared-package


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260818T205134Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 142.9s (2 bridge agents)
Total 142.9s

💰 Value — sound-with-nits

Adds a CI gate that fails any PR changing a consumer-visible manifest field or an exported symbol without the version bump the change implies, closing a real silent-publish hole (publish.yml skips versions already on npm); a well-built port of proven agent-runtime tooling, wired into the existing ve

  • What it does: Adds three byte-identical-to-sibling-repos scripts (verified: local sha256s match the PR body exactly) plus a committed 3,485-line api-surface.json record. check-version-bump.mjs (scripts/check-version-bump.mjs:95-123) diffs every npm-copied manifest field — with catalog: specifiers resolved through pnpm-workspace.yaml (scripts/check-version-bump.mjs:250-263) — and the export-symbol record against
  • Goals it achieves: The gap is verified real: publish.yml:191-192 skips publishing any version npm already holds, so a peer-range move or an added export merged without a bump silently never reaches consumers, and nothing goes red — an added export is invisible even to a manifest check because exports[...].types still points at the same dist path. After merge, a consumer-visible change cannot land under an already-pu
  • Assessment: Good change, in the grain of the codebase. It extends the existing verify:package gate chain rather than adding a new workflow; it fails closed everywhere (missing surface record = failure at check-version-bump.mjs:396-410, unresolvable catalog = throw, CI event with no base branch = throw at check-version-bump.mjs:162-172); the committed-record design beats a two-build comparison because it runs
  • Better / existing approach: No materially better architecture found. Searched: package.json scripts, .github/workflows/{ci,publish}.yml, scripts/ (all 20 files), git log on scripts/. Closest existing capability is verify-package-exports.mjs, which packs the tarball and checks d.ts/runtime agreement (scripts/verify-package-exports.mjs:981,1082) but never compares against the merge base and cannot demand a bump — different job
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A fully-wired release gate that makes both manifest and exported-symbol changes require a version bump, ported byte-identically from agent-runtime and enforced in CI and publish; verified passing against origin/main in this checkout with 27/27 fixture tests green.

  • Integration: Reachable and enforced on every path that matters. package.json:183 puts check:version-bump first in verify:package and check:api-surface after it; .github/workflows/ci.yml:61-62 runs verify:package on every PR and push, with fetch-depth: 0 (ci.yml:17) added specifically so the merge-base diff works; .github/workflows/publish.yml:49 runs the same gate before publishing. The regeneration path exist
  • Fit with existing patterns: Follows the repo's established checker family (scripts/check-skill.mjs, check-model-id-requests.mjs, check-analyst-benchmark-implementation.mjs all run inside verify:package the same way). The byte-identity claim is real: sha256 of all three script files matches /home/drew/code/agent-runtime and /home/drew/code/agent-knowledge (1 unique hash per file), and env vars were neutralized (PACKAGE_VERSIO
  • Real-world viability: Fails closed at every joint I could probe: CI event with no base branch errors instead of silently comparing local main (check-version-bump.mjs:162-172); an unresolvable catalog pin throws rather than comparing equal (mjs:256-261); a publishable package with no api-surface.json record is a failure, not a pass (mjs:396-409); a record stale against the built declarations fails with a diff (check-api
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Two d.ts export-name extractors now coexist in scripts/ [maintenance] ``

scripts/verify-package-exports.mjs:1082 (collectDtsExportNames, regex-based, names only) and scripts/check-api-surface.mjs:106 (exportsOfFile, oxc-parser-based, names + value/type kinds) both walk built declaration files following export * chains. The oxc extractor is strictly more capable. They serve different checks (packed-tarball type/runtime agreement vs committed surface record), and the new files must stay byte-identical to agent-runtime/agent-knowledge siblings, so the duplication is exp


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260818T205607Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 154.3s (2 bridge agents)
Total 154.3s

💰 Value — sound-with-nits

Adds a CI gate that fails any PR changing a consumer-visible manifest field or exported symbol without the matching version bump, closing a real hole this repo's idempotent publish workflow creates; executed well and verified claims, with one minor in-repo overlap.

  • What it does: Adds three byte-identical-across-repo scripts plus a committed record: (1) scripts/check-version-bump.mjs diffs every publishable manifest's consumer-visible fields (deps/peers/exports/engines/install-scripts, with catalog: specifiers resolved through pnpm-workspace.yaml) AND the committed api-surface.json against the PR merge base, failing unless version rose to the level the change implies (
  • Goals it achieves: Prevent consumer-visible changes from merging under a version npm already holds. That failure mode is real here: publish.yml skips any version already on the registry and treats it as success (.github/workflows/publish.yml:191-199), so an unbumped peer-range move or new export silently never ships and nothing goes red — exactly the agent-runtime #893 PursuitProjection incident the header cites. No
  • Assessment: Good change, well executed. The committed-record design over a live two-build comparison is reasoned in the file (merge-base symbols are one git show away; the record shows in the PR diff; the check runs pre-build — scripts/lib/api-surface.mjs:13-23). Fail-closed defaults are consistent with repo doctrine (missing record = failure at scripts/check-version-bump.mjs:396-409; unresolvable catalog =
  • Better / existing approach: No materially better approach for the core capability — I looked. Searched repo scripts/, workflows, and history for any prior version-bump or surface-compare logic (none); checked publint/attw usage (different job); the nearest off-the-shelf equivalent of the surface-record half is @microsoft/api-extractor's API report, but it needs the TS compiler API, which TS7 does not ship — the exact reason
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Closes a real, twice-burned gap — consumer-visible manifest and export changes shipping under a version npm already holds — with two mutually-reinforcing checks wired into the CI and publish paths on day one, matching the repo's established guard-script pattern and its zero-upward-dependency layerin

  • Integration: Fully reachable immediately: check:version-bump and check:api-surface are prepended/appended into verify:package (package.json:183), which CI runs on every PR after build (ci.yml:62) and publish runs before npm publish (publish.yml:49); fetch-depth: 0 (ci.yml:14-17) supplies the merge base. Verified live in this checkout: check-version-bump exits 0 against origin/main, and 27/27 tests pass against
  • Fit with existing patterns: Follows the repo's existing pattern exactly: guard scripts (check-skill, check:model-ids, check-analyst-benchmark) aggregated under verify:package. Vendored byte-identical copies across agent-eval/agent-runtime/agent-knowledge are the correct move given the CLAUDE.md layering rule forbids depending on agent-runtime (devDeps included) and the scripts are repo-agnostic by construction (verified: no
  • Real-world viability: Fail-closed on every error path probed: missing surface record fails (check-version-bump.mjs:396-410), unresolvable base fails before fallback in CI (check-version-bump.mjs:162-172), unresolvable catalog pin fails (check-version-bump.mjs:255-261), wildcard export matching nothing fails (lib/api-surface.mjs:105-112), external export * from fails with a fix instruction (check-api-surface.mjs:137-1
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Cross-repo byte-identity is convention, not enforcement [problem-fit] ``

The three copies of check-version-bump.mjs / check-api-surface.mjs / lib/api-surface.mjs stay identical only by discipline; nothing automates the comparison (sibling repos are not locally verifiable from this checkout, so the PR's sha256 claims are taken on trust). Drift would be visible in a three-way diff but not flagged anywhere. The PR body already names the consolidation path — agent-eval hosting the implementation for the others once it can gate against a published copy. Acceptable now; re

💰 Value Audit

🟡 Two .d.ts export-name readers now live in the repo [duplication] ``

scripts/verify-package-exports.mjs:1082-1148 already contains collectDtsExportNames, a regex-based reader of exported names from .d.ts (used for d.ts-vs-runtime agreement on the packed tarball), while this PR adds an oxc-parser-based reader (exportsOfFile, scripts/check-api-surface.mjs:106-165) that extracts names AND kinds from dist. The jobs differ (packed-tarball names-only vs dist name-to-kind record) and verify-package-exports.mjs is repo-specific rather than one of the byte-identical files


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260818T210156Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — ecd21506

Review health 100/100 · Reviewer score 31/100 · Confidence 95/100 · 19 findings (1 medium, 18 low)

glm: Correctness 31 · Security 31 · Testing 31 · Architecture 31

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 16 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM d.ts extractor has no direct test coverage — scripts/check-api-surface.mjs

tests/version-bump-check.test.ts (634 lines) executes scripts/check-version-bump.mjs end-to-end in tmp repos but never runs scripts/check-api-surface.mjs; the parser/walker (declarationKind, declaredNames, resolveRelativeDeclaration, exportsOfFile's export*/cycle/memoization, export * as ns, the external-re-export guard) is exercised only by the CI happy path where the committed api-surface.json matches the current build. A regression that stays record-consistent (e.g. dropping a name class or misclassifying a kind) would never fail any test — it silently narrows what the version-bump gate can see, the exact silent-wrong the repo's fail-loud doctrine targets. Fix: add a vitest case that writes a fixture package (package.json + dist d.ts files covering export kind markers, export *, export

🟡 LOW 3485-line machine-generated record adds review-diff noise on every export change — api-surface.json

The record is 129 KB / 3485 lines of sorted JSON at the repo root, so every future export addition shows as a large generated diff block. This is the documented design (lib/api-surface.mjs lines 13-19: the record appears in the PR diff so an added export is reviewer-visible) and the stable sort keeps diffs minimal (one line per symbol), so this is an accepted trade-off, not a defect. No change needed; noted for reviewer expectation-setting.

🟡 LOW Fallback version string must be manually synced with pyproject.toml — clients/python/src/agent_eval_rpc/__init__.py

The except PackageNotFoundError branch hardcodes "0.148.0" and must be edited by hand on every release; a miss reports a stale version when the package runs uninstalled (e.g. from a source checkout). This diff updates it correctly and the pattern predates the PR. Optional hardening: derive the fallback from a single source (e.g. read pyproject.toml) or add a CI assert that pyproject.toml, init.py, and uv.lock versions match. No action required for this PR.

🟡 LOW Corrupt committed record crashes with a raw SyntaxError instead of the guidance message — scripts/check-api-surface.mjs

When the committed record differs from the generated one and is not valid JSON, JSON.parse(committed) throws an unhandled SyntaxError with a stack trace (verified: exit 1, stack pointing at line 210), replacing the intended 'The export surface record is out of date ... pnpm run build && pnpm run api:surface' stderr message. It still fails closed, so severity is low, but the actionable remediation text is lost exactly when a human hand-edited the record. Fix: wrap JSON.parse in try/catch and push a failure naming the record file and the parse error.

🟡 LOW Named re-export kind is taken from syntax markers, not the target file — scripts/check-api-surface.mjs

For 'export { X } from "./chunk.js"' the code records kind solely from node.exportKind/specifier.exportKind. Verified with a fixture: an interface re-exported as 'export { Shared } from "./chunk-ABC.js"' records as "value". This repo's tsdown output marks types inline (dist/index.d.ts:2805 uses 'type Foo' specifiers), so the committed record is accurate here, but tsc-emitted d.ts without verbatimModuleSyntax does not add the marker, and the file header states the tool must behave identically across the TypeScript-6/7 repo set. Impact on this repo: an emitter-settings or TS-version change that flips marker style would register every type-only re-export as value->type — compareSurfaces calls that 'narrowed' = breaking, demanding a major/minor bump for a compiler-settings no-op. Fix: resolve

🟡 LOW resolveRelativeDeclaration never tries .d.cts and prefers .d.ts over .d.mts for .mjs specifiers — scripts/check-api-surface.mjs

The candidate list maps '.mjs' to '.d.ts' before '.d.mts' and has no '.cjs' -> '.d.cts' rule, while scripts/lib/api-surface.mjs accepts all of .d.ts/.d.mts/.d.cts as entry declarations (DECLARATION_SUFFIXES). For this repo's tsdown chunks (.d.ts) it never bites, but a dual-format build emitting both foo.d.ts and foo.d.mts would resolve an .mjs specifier to the wrong declaration. Fix: order candidates by extension match (.mjs -> .d.mts first, .cjs -> .d.cts) and fall back to the generic forms.

🟡 LOW Manifest comparison is JSON key-order sensitive: a pure key reorder inside exports demands a version bump — scripts/check-version-bump.mjs

surfaceChanges skips a leaf only when JSON.stringify(left) === JSON.stringify(right), and non-map fields fall through to whole-value stringify. Reproduced: swapping 'types'/'import' key order inside exports['.'] with no semantic change fails with 'exports..: {"types":...,"import":...} -> {"import":...,"types":...}' demanding a patch bump. npm resolves exports conditions by name, not position, so nothing consumer-visible changed. Same class applies to directories, imports, typesVersions, publishConfig, peerDependenciesMeta and nested maps. Impact: CI blocks a legitimate no-op edit until a maintainer burns a version number. Fix: canonicalize object leaves (recursively sort keys) or use an order-insensitive deep-equal before stringify.

🟡 LOW Marking a package private demands a version bump that can never be published — scripts/check-version-bump.mjs

The union-of-names rule keeps a package that stops being publishable in scope, and 'private' is in CONSUMER_VISIBLE_FIELDS, so setting private:true without a bump fails CI (asserted by the test 'rejects marking a published package private without a version bump'). A private package is never published, so no registry version can hold wrong terms and the demanded bump is unspendable; the failure text instructs 'Bump the version in the same pull request' plus a CHANGELOG entry for a version no consumer will ever install. Deliberate per the comments (the transition must not pass silently), but the remediation is incoherent for this path. Fix: demand an explicit acknowledgment for a publishable->private transition instead of a version bump.

🟡 LOW globToRegExp diverges from pnpm glob semantics: space placeholder collision and missing zero-segment globstar match — scripts/check-version-bump.mjs

The conversion uses a literal space as the '' placeholder (''->' ', ''->'[^/]', ' '->'.'), so a workspace pattern containing a real space is corrupted: 'my dir/' becomes ^my.dir/.$ (verified), which both over-matches unrelated directories and mishandles the intended one. Separately, pnpm/fast-glob treat 'a//package.json' as matching zero segments, so a package directly at the root of a 'packages/**' pattern is a workspace member for pnpm but fails ^packages/.$ here and is never inspected — a silent pass for that package, the exact defect class this script exists to close. Also '?' wildcards are escaped to literals. Latent in agent-eval (no pnpm-workspace.yaml at either ref) but live in the byte-identical agent-runtime/agent-knowledge copies the header mandates. Fix: match wit

🟡 LOW '$' patterns in a filename corrupt the recorded subpath via String.replace — scripts/lib/api-surface.mjs

subpath.replace('*', stem) treats $&, $' etc. in stem as replacement patterns. Verified: a built file pre$&post.d.ts under a "./bench/*" export is recorded as subpath ./bench/pre*post. The record stays self-consistent so the gate still compares it, but the entry is filed under a subpath no consumer can import. Fix: subpath.replace('*', () => stem).

🟡 LOW A package without an exports map gets a silently empty surface record — scripts/lib/api-surface.mjs

if (declared === undefined) return { entries: [], assets: [] } — verified: a manifest with main: './dist/index.js' + types: './dist/index.d.ts' and no exports yields an empty surface. check-api-surface then writes/pass-checks an empty record forever and check-version-bump's missing-record rule never fires because the record exists, so symbol changes under main are invisible — contradicting the module doc 'Every entry point a consumer can import from this package'. This repo always declares exports, so the hole is latent for the byte-identical sibling copies or a future package. Fix: fall back to top-level types/typings (then main) for the '.' entry, or fail loud when a publishable package has neither exports nor types.

🟡 LOW Dirent.parentPath requires Node >= 20.12 but engines declares >=20 — scripts/lib/api-surface.mjs

entry.parentPath (nodejs.org/api/fs.html: added in v20.12.0, v18.20.0, v21.4.0) is undefined on Node 20.0-20.11, which package.json engines ('>=20') admits; relative(directory, undefined) then throws TypeError. Unreachable in this repo today (its exports contain no '*'), but the file's own header says these three files are kept byte-identical in agent-runtime/agent-knowledge, whose ./benchmarks/* patterns (per the expandWildcard doc comment) would hit it. Loud crash, never a silent pass. Fix: read engines to >=20.12 or derive the parent without parentPath.

🟡 LOW No direct tests for the exports-resolution half of the module — scripts/lib/api-surface.mjs

TYPE_CONDITIONS, the wildcard one-star validation, the top-level condition-sugar break, and expansion against a real directory tree (including the npm split-at-star rationale in the comment at lines 81-88) have no test anywhere: tests/version-bump-check.test.ts covers only compareSurfaces/surfaceSeverity/versionBumpLevel/requiredBumpLevel end-to-end, and this repo's manifest has no wildcard or sugar exports, so CI exercises none of these paths. A regression in expandWildcard or declarationTargetOf would not fail any test. Fix: a small fixture test calling resolveExportEntries against a temp dist tree (wildcard, nested, asset, one-sided star, no-match throw) — th

🟡 LOW Prerelease ordering ignores semver numeric identifiers, falsely reporting a higher prerelease as 'lower' — scripts/lib/api-surface.mjs

return head.prerelease > base.prerelease ? 'patch' : 'lower' compares prerelease strings lexicographically. Verified: versionBumpLevel('1.0.0-alpha.9','1.0.0-alpha.10') returns 'lower' and ('0.5.0-2','0.5.0-10') returns 'lower', though semver orders 10 above 9 and 2. In check-version-bump.mjs a legitimate prerelease advance that pays for a surface change is then rejected with 'which is not higher'. Fail-closed availability bug, not a safety hole — this repo's releases use plain triples. Fix: split on '.' and compare all-digit identifiers numerically.

🟡 LOW Awkward phrasing in new doc comment ('defaults no engine', 'a version records') — src/multishot/multishot.ts

The comment says 'The script defaults no engine, so a version records whichever one the caller names'. Per the repo's Simplified Technical English discipline, write 'The script sets no default engine, so a record version captures whichever engine the caller names'. Meaning is recoverable and all claims are factually accurate (scripts/record-multishot-golden.ts:191 uses requireArg), so this is cosmetic only — no action required for merge.

🟡 LOW Historical-incident comments conflict with the repo's no-narrative comment rule — tests/version-bump-check.test.ts

Lines 186 and 487 open with 'This is the shape that shipped 0.119.0 twice' / 'The shape that shipped PursuitProjection into no published version'. CLAUDE.md states history belongs in commit messages and PR descriptions, not the source tree. The comments do explain the defect shape the test guards, and they mirror the script header's style, so this is a style nit: reword to describe the invariant (a specifier that resolves differently while the manifest text stays identical) without naming the incident.

🟡 LOW No timeout on the subprocess spawn — tests/version-bump-check.test.ts

execFileAsync(process.execPath, ['scripts/check-version-bump.mjs']) sets no timeout option. A hang in the script (e.g. a git invocation that blocks on a prompt in some environment) would stall the suite until the CI job timeout. Risk is low because the script runs git non-interactively with hooks disabled, but a 30s timeout would make a hang a fast, diagnosable failure.

🟡 LOW Prerelease version-bump semantics are untested — tests/version-bump-check.test.ts

versionBumpLevel in scripts/lib/api-surface.mjs:256-262 has distinct prerelease rules (1.0.0 -> 1.0.1-beta.1 counts as 'lower'; 1.0.0-beta.1 -> 1.0.0 counts as 'patch'; later prerelease counts as 'patch'). No fixture in this file uses a prerelease version, so those branches — including the 'lower' path that must reject — run untested. Add one fixture pair with a -beta.1 base to pin them.

🟡 LOW Widened exports and GITHUB_BASE_REF mapping lack coverage — tests/version-bump-check.test.ts

compareSurfaces' widened case (type -> value, additive) is never exercised — only added/removed/narrowed are. resolveBase's GITHUB_BASE_REF -> origin/ translation is likewise untested (only the merge_group fail-closed branch at line 250 is). Neither path can regress silently without a green suite lying; both are one small fixture each.


tangletools · 2026-08-18T21:04:43Z · trace

tangletools
tangletools previously approved these changes Aug 18, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 19 non-blocking findings — ecd21506

Full multi-shot audit completed 8/8 planned shots over 16 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-18T21:04:43Z · immutable trace

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — ecd21506

Review health 100/100 · Reviewer score 20/100 · Confidence 95/100 · 18 findings (3 medium, 15 low)

glm: Correctness 20 · Security 20 · Testing 20 · Architecture 20

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 9 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM 42 symbols recorded as value are interfaces with no runtime binding — api-surface.json

api-surface.json:3098 records "Artifact": "value" under ./traces, but the source declares it as export interface Artifact (src/trace/schema.ts:214) with no runtime export; same for "AgentProfileCell": "value" at api-surface.json:2315 (src/agent-profile-cell.ts:49 is an interface, and the file has zero plain export {} statements). The root entry correctly records the same names as "type" because src/index.ts:25-31 uses explicit export type {}. Cause: tsdown's d.ts chunks re-export interfaces via unmarked specifiers (dist/traces.d.ts, dist/profile-cell.d.ts line 2) and check-api-surface.mjs:131 defaults an unmarked specifier to 'value' instead of resolving it to its declara

🟠 MEDIUM versionBumpLevel orders prerelease identifiers by string, not semver precedence — scripts/lib/api-surface.mjs

Line 262: return head.prerelease > base.prerelease ? 'patch' : 'lower'. Verified by execution: versionBumpLevel('1.0.0-10', '1.0.0-2') === 'patch' although 1.0.0-2 precedes 1.0.0-10 under semver rule 11 (numeric identifiers compare numerically). When the required level is 'patch' (0.x additive export, or any manifest-only change per check-version-bump.mjs:434), the rank comparison at check-version-bump.mjs:439 passes — a silent pass onto a version the registry may already hold, the precise defect (0.119.0) this check was built to close. The inverse case versionBumpLevel('0.2.3-alpha.9', '0.2.3-alpha.10') === 'lower' false-fails a legitimate bump. Not reachable wi

🟠 MEDIUM Prerelease version semantics have zero coverage, and the uncovered path mis-orders numeric prerelease identifiers — tests/version-bump-check.test.ts

All 27 cases use plain release triples (1.0.0, 0.140.1, 0.9.0), but scripts/lib/api-surface.mjs:256-262 special-cases prereleases with a lexicographic string comparison. Verified by direct execution: versionBumpLevel('1.0.0-alpha.2', '1.0.0-alpha.10') returns 'lower' (semver says alpha.10 > alpha.2, numeric identifiers compare numerically), so a legitimate prerelease bump would be rejected by CI with the false message 'which is not higher'. The script is shared byte-identical with agent-runtime and agent-knowledge, so the first prerelease any of the three cuts hits this. Impact: fail-closed (loud false block, not a silent pass), but the suite this file adds would neither catch the defect nor pin a fix. Fix: add cases for prerelease-to-release ('patch'), prerelease advance, prerelease added

🟡 LOW Newly-wired check:api-surface can crash with raw EISDIR on directory-shaped declaration chunks — package.json

verify:package now runs node scripts/check-api-surface.mjs, whose resolveRelativeDeclaration (scripts/check-api-surface.mjs:81-93) tests candidates with existsSync — true for DIRECTORIES. Reproduced on this worktree's pre-existing dist: dist/traces.d.ts line 1 was export * from './trace'; and dist/trace/ existed as a chunk directory, so candidate 1 (base.replace(/.m?js$/, '.d.ts') is a no-op on an extensionless specifier) returned the directory and readFileSync threw EISDIR at scripts/check-api-surface.mjs:111, exit 1, no diagnostic. On a fresh self-run pnpm build of HEAD the chunk layout is hashed (./schema-BtVldJ3T.js) and the full verify:package chain passes (exit 0), s

🟡 LOW verify:package mixes implicit-run and explicit-run pnpm forms — package.json

The chain uses pnpm check:analyst-benchmark (implicit run form, pre-existing) next to pnpm run check:version-bump / pnpm run check:skill / pnpm run check:model-ids / pnpm run check:api-surface (explicit). Cosmetic only; behavior identical. Normalize to pnpm run for consistency with the two new entries.

🟡 LOW Repo engines '>=20' is wider than oxc-parser's '^20.19.0 || >=22.12.0' — pnpm-lock.yaml

The lock pins oxc-parser@0.144.0 with engines {node: ^20.19.0 || >=22.12.0} while the repo declares engines.node '>=20' (package.json:235). A contributor on Node 20.0-20.18 gets an install-time engine failure. Pre-existing (0.141.0 carried the identical engines field), devDependency-only, so no consumer impact. Fix if desired: narrow repo engines to '>=20.19' in a separate maintenance change; not a blocker for this PR.

🟡 LOW No unit tests for the declaration walker — scripts/check-api-surface.mjs

tests/version-bump-check.test.ts (27 tests, all passing) covers only the record-comparison side in check-version-bump.mjs and lib/api-surface.mjs; nothing imports or spawns check-api-surface.mjs. The oxc AST dispatch, kind classification, and relative-specifier resolution — the trickiest logic in the file — are exercised only by CI's post-build verify:package run. That guard is real (ci green at head sha ecd2150 proves the walker against the actual tsdown output), but regressions in unemitted forms (type-only named stars, .d.mts/.d.cts resolution, cycles) would surface as production crashes rather than test failures. Add a fixture-directory test mirroring tests/version-bump-check.test.ts's style.

🟡 LOW export type * as ns from recorded as value kind — scripts/check-api-surface.mjs

The named-star branch of ExportAllDeclaration sets names[name] = 'value' unconditionally, ignoring node.exportKind. Harness-confirmed: export type * as ns from './chunk1.js' yields {"tns":"value"}. A consumer cannot import tns as a runtime binding, so the record overstates the value surface; when the labeling is later corrected (or the construct first appears), compareSurfaces reports a kind change — value -> type reads as 'narrowed' = breaking and demands an unneeded minor/major bump on a 0.x/stable package via surfaceSeverity. Latent today: no current entry emits it. Fix: names[name] = node.exportKind === 'type' ? 'type' : 'value' — one line, same edit in the byte-identical siblings.

🟡 LOW resolveRelativeDeclaration can return a directory, crashing with EISDIR — scripts/check-api-surface.mjs

The final candidate base passes existsSync, which returns true for directories. An extensionless specifier naming a directory (e.g. export * from './knowledge' under a tsc-style declaration layout — observed live in this worktree) resolves to the directory and readFileSync at line 111 throws EISDIR: illegal operation on a directory with a raw stack instead of the script's structured error. Latent for tsdown output (specifiers always end in .js), but the file header states it is kept byte-identical across agent-eval, agent-knowledge, and agent-runtime and must stay repo-generic. Fix: check statSync(candidate).isFile() per candidate, or replace the bare

🟡 LOW Consumer-visible field list omits man, libc, devEngines — scripts/check-version-bump.mjs

The header claims 'Every field npm copies into the published manifest', but CONSUMER_VISIBLE_FIELDS omits 'man' (man pages installed to the consumer), 'libc' (npm install constraint, sibling of the compared 'os'/'cpu'), 'devEngines' (npm 10+ install restriction), and pnpm's 'acceptDependencies'. A PR that adds or moves one of these ships it with no version bump and the check reports 'consumer surface unchanged' — fail-open for those fields. None is used in this repo's manifest today, so impact is latent; add them to the array (and sync the byte-identical sibling copies).

🟡 LOW Same-triple prerelease downgrade accepted as a paid bump — scripts/check-version-bump.mjs

Evidence (probed against the imported helper): versionBumpLevel('1.0.0-rc.10','1.0.0-rc.2') returns 'patch' because lib/api-surface.mjs:262 compares prerelease strings lexically ('rc.2' > 'rc.10' as text). The acceptance gate rankOf(paidLevel) >= rankOf(requiredLevel) at check-version-bump.mjs:439 then treats a semver DOWNGRADE as payment, letting a consumer-visible change ship on a lower version the registry may already hold — the exact defect class this gate exists to close. The reverse direction ('rc.2'->'rc.10', a real upgrade) is rejected as 'lower', a false positive. semver orders dot-separated identifiers numerically when numeric; fix the identifier-wise compare in lib/api-surface.mjs (and mirror in the sibling repos). Narrow exposure: requires same-triple prerelease moves, which th

🟡 LOW globToRegExp mishandles '?' and literal spaces in pnpm workspace patterns — scripts/check-version-bump.mjs

Evidence (probed): globToRegExp('packages/?') compiles to /^packages/?$/ which matches 'package'/'packages' but NOT 'packages/a', while pnpm (micromatch) matches packages/a — so manifests under a '?' include pattern silently escape policing (fail-open). Also the '' placeholder trick (.replace(/**/g,' ') then ' '->'.') collides with a literal space in a pattern, turning it into '.' over-matching; a space in an '!' exclude pattern would over-exclude. No current repo uses these shapes (this repo has no pnpm-workspace.yaml), but the header commits this file to staying byte-identical across agent-eval/agent-knowledge/agent-runtime, so a future pattern regresses silently. Fix: escape the placeholder with a char class excluded from escaping (e.g. replace '' with '\u0000' sentinel or tran

🟡 LOW Wildcard guard checks star presence, not count, contradicting its own error text — scripts/lib/api-surface.mjs

Lines 144-150 reject a star on one side only, and the error says both sides 'must both carry exactly one ""', but the condition only tests subpath.includes('*')/target.includes('*'). A target with two stars reaches expandWildcard, where const [prefix, suffix] = target.split('*') (line 80) silently drops the third segment. Verified by execution: './dist/x//*.d.ts' then throws the misleading 'matches no built file' error instead of the pattern-shape error. Outcome is fail-loud today because no filename can match a literal-star suf

🟡 LOW expandWildcard requires Node >=20.12 but engines declares >=20 — scripts/lib/api-surface.mjs

Line 95 reads entry.parentPath, added in Node 20.12.0; on Node 20.0-20.11 it is undefined and relative(directory, undefined) throws a TypeError. package.json declares "engines": { "node": ">=20" }, which admits those versions. Only reachable for manifests with wildcard exports (this repo has none), and CI runs Node 24. Fix: use entry.path (the recursive-readdir full relative path, available since the recursive option landed) or tighten engines to >=20.12.

🟡 LOW node-condition-only exports are recorded as assets, hiding their symbols — scripts/lib/api-surface.mjs

TYPE_CONDITIONS = ['types', 'import', 'module', 'default', 'require'] omits 'node'/'node-addons'. Verified by execution: resolveExportEntries({ exports: { '.': { node: { types: './dist/index.d.ts', default: './dist/index.js' } } } }, dir) returns { entries: [], assets: ['.'] }. A package whose manifest gates types behind a node condition has every symbol invisible to both checks — a record exists, so check-version-bump's missing-record fail-closed never fires, and symbol changes silently pass. No live impact: this repo's manifest uses types/import/default only. Fix: add 'node' to TYPE_CONDITIONS after 'types'.

🟡 LOW Base-resolution fallback and early-exit paths untested — tests/version-bump-check.test.ts

Every check() call passes an explicit PACKAGE_VERSION_BUMP_BASE, so only the explicit-base and fail-closed-CI paths (resolveBase lines 149-185) are exercised. Untested: the GITHUB_BASE_REF -> origin/ resolution whose frozen-sha-vs-branch-tip reasoning carries a substantial comment (lines 152-157), the non-CI ['origin/main','main'] candidate fallback, the mergeBase === head 'No commits ahead' early exit, and the 'shares no history with HEAD' error. These are the paths that run in real CI for this repo, tested only

🟡 LOW Behaviorally duplicate test pair — tests/version-bump-check.test.ts

'does not fire when the exported symbols do not move' (line 597) performs the identical fixture mutation and assertion as 'does not fire on an ordinary source-only change' (line 197): write source.ts value 2, commit, expect 'consumer surface unchanged at 1.0.0'. The second adds no marginal coverage. Fix: vary it — e.g. rewrite api-surface.json with keys inside an entry point reordered, or edit only the surface record's 'package' prose field — so the unchanged-path guard covers a distinct non-change.

🟡 LOW Documented workspace: non-resolution boundary is not pinned by any test — tests/version-bump-check.test.ts

check-version-bump.mjs:29-32 documents a deliberate scope boundary: 'workspace:' specifiers are NOT resolved because the sibling's own bump already pays for the change. No test covers it — every dependency in every fixture is either 'catalog:' or a literal. A future refactor that starts resolving workspace: specifiers would demand a second bump for an already-paid change and pass this entire suite silently. Fix: one case where a dependent uses 'workspace:^' against a sibling whose version bumps, asserting the dependent itself is not flagged.


tangletools · 2026-08-18T21:07:56Z · trace

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — ecd21506

Review health 100/100 · Reviewer score 37/100 · Confidence 95/100 · 17 findings (1 medium, 16 low)

glm: Correctness 37 · Security 37 · Testing 37 · Architecture 37

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 9 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Prerelease comparison is lexicographic, contradicting semver numeric identifiers — scripts/lib/api-surface.mjs

Evidence: return head.prerelease > base.prerelease ? 'patch' : 'lower' compares prerelease strings lexicographically. Executed: versionBumpLevel('1.0.0-alpha.9', '1.0.0-alpha.10') returns 'lower', but semver says alpha.10 > alpha.9 because numeric identifiers compare numerically. check-version-bump.mjs then reports 'moves 1.0.0-alpha.9 -> 1.0.0-alpha.10, which is not higher' and blocks a legitimate advance. Fails closed (false rejection, never a false pass), and no current version in this repo uses multi-digit prerelease identifiers, but the file is byte-identical in three repos and the defect triggers the first time a prerelease identifier crosses 9 -> 10. Fix: split on '.' and compare identifiers per semver rule 11 (numeric-vs-numeric numerically, numeric > alphanumeric, else lexical).

🟡 LOW Exact pin 0.144.0 locks out patch releases and drops wasm32-wasi platform support — pnpm-lock.yaml

Specifier moved from ^0.141.0 to exact 0.144.0, so every upstream oxc-parser release now requires a manual bump. Also, 0.144.0 no longer publishes @oxc-parser/binding-wasm32-wasi (verified via npm view optionalDependencies), so wasm/wasi environments can no longer run the parser. Impact is nil for this repo: Node-only (engines >=20, package.json:235), devDependency, and 0.x caret semantics gave no range beyond 0.141.x anyway; the pin likely keeps the new check:api-surface output deterministic. Fix: none required; if automated updates (renovate) are expected, they must handle the exact-pin style.

🟡 LOW Pre-existing audit advisories (fast-uri 3.1.4, js-yaml 4.3.0) ride along untouched — pnpm-lock.yaml

pnpm audit on the head lockfile reports 2 high (fast-uri >=3.0.0 <3.1.5 host confusion, js-yaml >=4.0.0 <4.3.1 quadratic CPU) plus 4 low/moderate. Verified these packages are not part of this diff (git diff shows zero matches for fast-uri/js-yaml) and are not dependencies of oxc-parser@0.144.0 (its only dep is @oxc-project/types). Not a regression from this PR, but the lockfile bump was an opportunity to fix them. Fix: separate PR bumping fast-uri to ^3.1.5 and js-yaml to ^4.3.1 (or via the existing pnpm overrides block).

🟡 LOW Corrupt committed record crashes with a raw stack instead of the clean failure — scripts/check-api-surface.mjs

On a mismatch, the script does JSON.parse(committed) inside the failure branch; a hand-edited or truncated api-surface.json throws an unhandled SyntaxError with a stack trace rather than the composed 'record is out of date' message with the regenerate instructions. Wrap the parse and fall back to reporting a wholesale mismatch when the record is unreadable.

🟡 LOW No direct tests for the d.ts extraction logic — scripts/check-api-surface.mjs

tests/version-bump-check.test.ts (634 lines) exercises only check-version-bump.mjs against hand-written api-surface.json fixtures; nothing tests exportsOfFile, resolveRelativeDeclaration, cycle handling, or the star-re-export paths in this script. The only coverage is the CI verify:package run against this repo's own build, which passes (verified locally) but cannot catch the latent precedence/resolution deviations above. Add fixture d.ts trees (like the existing tmpdir repo pattern) covering star re-exports, .mjs specifiers, and cycles.

🟡 LOW Star re-export overwrites local exports regardless of ES precedence — scripts/check-api-surface.mjs

The ExportAllDeclaration merge does names[name] = ... in source order, so a file containing export declare const x: number followed by export * from './chunk.js' (chunk also exporting x) records x as the re-exported kind, while ES/TS resolution semantics make the local export always win. Latent today: rg shows 0 export * from statements in dist/*.d.ts (rolldown-dts emits explicit export lists), so current records are correct. Fix: collect star re-exports into a pending map and merge without overwriting names already set by local declarations or named exports.

🟡 LOW .mjs specifiers probe .d.ts before .d.mtsscripts/check-api-surface.mjs

Node/TS resolution maps .mjs -> .d.mts and .js -> .d.ts, but the candidate order tries base.replace(/\.m?js$/, '.d.ts') first for both, so a directory containing both foo.d.ts and foo.d.mts behind an ./foo.mjs specifier records the wrong file's symbols. Latent: the current build emits only .js specifiers in d.ts re-exports. Fix: branch on the specifier suffix instead of one shared regex.

🟡 LOW export * as ns recorded unconditionally as 'value' — scripts/check-api-surface.mjs

When ExportAllDeclaration has node.exported (namespace re-export), the name is recorded as 'value' without inspecting whether the target module carries any value bindings; a type-only namespace is recorded as a value export. Stable across runs because the same bundler produces the input, so no false drift today, but a kind fidelity gap if the build shape changes. Latent: no export * as statements exist in current dist.

🟡 LOW Condition-order change inside one exports entry is reported as a consumer-visible change via raw JSON.stringify — scripts/check-version-bump.mjs

Per-key map compare handles top-level fields, but the value under one exports subpath (e.g. {types, import, default}) is compared by JSON.stringify, so swapping condition key order reports a change and demands a patch bump even when every condition still resolves identically. Node resolves exports conditions in declaration order, so this is conservative-correct rather than wrong; record it as a deliberate sensitivity in the header so a maintainer hitting it does not treat it as a bug.

🟡 LOW Two same-named manifests at one ref: the later ls-tree entry silently wins — scripts/check-version-bump.mjs

byName.set(manifest.name, ...) overwrites without checking for an existing entry, so a duplicate workspace package name compares an arbitrary winner picked by ls-tree path order. pnpm itself fails install on duplicate workspace names, so the wrong comparison could only run in a state that already fails elsewhere. Prefer throwing on a duplicate name in a publishable state: the ambiguity is exactly the kind of silent pick this script exists to prevent.

🟡 LOW globToRegExp uses a literal space as the ** placeholder, corrupting patterns that contain one — scripts/check-version-bump.mjs

escaped.replace(/**/g, ' ') then .replace(/ /g, '.') converts EVERY space, not just the placeholder. Verified: globToRegExp('my packages/') produces ^my.packages/[^/]$ and matches 'myXpackages/foo'. A pnpm-workspace pattern naming a directory with a space would match unrelated directories, inspecting the wrong manifests. Practically unreachable in this fleet (no such paths), but the fix is one line: use a sentinel that cannot survive the escape step (e.g. '\u0000').

🟡 LOW A publishable package with no exports field records an empty surface — scripts/lib/api-surface.mjs

Evidence: if (declared === undefined) return { entries: [], assets: [] }. Executed with { main: './dist/index.js', types: './dist/index.d.ts' }: returns an empty surface, so check-api-surface --write commits a record with zero entries and zero assets. Adding or removing an export through main then changes no compared manifest field (main/types values stay byte-identical) and no recorded symbol — an invisible surface change that passes both gates. Not this repo's shape (every publishable package has exports), but the fail-closed design principle applied to missing records (check-version-bump line 396) argues for the same treatment here: throw, or fall back to

🟡 LOW Wildcard guard checks star presence, not the count its error message asserts — scripts/lib/api-surface.mjs

Evidence: the guard is if (!subpath.includes('*') || !target.includes('*')) throw while the thrown message says 'must both carry exactly one ""'. Executed with exports { './x/*/*': { types: './dist/benchmarks/*.d.ts' } } against a populated fixture dir: it passes validation and produces record keys containing a literal star ('./x/a/'), because subpath.replace('*', stem) at line 102 replaces only the first star. The record then silently misstates the published surface instead of raising the intended clear error. Node itself rejects multi-star patterns, so the manifest is already broken, but a gate whose purpose is to fail loud on ambiguity should enforce the

🟡 LOW entry.parentPath requires Node >=20.12 but engines admits 20.0-20.11 — scripts/lib/api-surface.mjs

Evidence: relative(directory, entry.parentPath) reads Dirent.parentPath, added in Node 20.12.0; package.json declares "engines": { "node": ">=20" }. On Node 20.1-20.11 recursive readdirSync with withFileTypes returns Dirents without parentPath (it was .path there), so parentPath is undefined and path.relative throws a TypeError. The crash is loud and only reachable through a wildcard exports pattern, which this repo's manifest does not use, so impact is limited to developer friction on older Node 20.x in the fleet copies. Fix: use entry.parentPath ?? entry.path, or raise the engines floor for the scripts.

🟡 LOW types nested under a non-TYPE condition silently degrades the entry to an asset — scripts/lib/api-surface.mjs

Evidence: the condition walk only inspects TYPE_CONDITIONS ('types', 'import', 'module', 'default', 'require') at each object level. Executed with exports { '.': { node: { types: './dist/index.d.ts' }, default: './dist/index.js' } }: returns null, so resolveExportEntries records '.' as an asset with zero symbols — an entry point whose exports are entirely ungated, which is exactly the defect this module exists to close. This repo's manifest places types at the top of every subpath, so there is no impact here today, but the file is held byte-identical in agent-knowledge and agent-runtime, where a node/browser-conditioned types shape would silently vanish. Consider recursing one level into non-TYPE condition objects, or throwing when a condition object contains a types key this rea

🟡 LOW Historical-incident narrative in test comments — tests/version-bump-check.test.ts

Comments at lines 197-198 ('This is the shape that shipped 0.119.0 twice') and 424-425 ('The shape that shipped PursuitProjection into no published version') recount past incidents. The repo's comment discipline (CLAUDE.md: 'never ... which audit found a bug ... history belongs in commit messages') forbids this; the defect-class rationale already lives in the script header these tests cite. Impact is cosmetic; fix by rewording to the present-tense rule ('a catalog pin move with byte-identical manifests is invisible to a manifest diff'). Consistent with the script's own header style, hence low.

🟡 LOW Three script paths lack coverage — tests/version-bump-check.test.ts

No test exercises versionBumpLevel's prerelease branches (base.prerelease===undefined -> 'lower', prerelease exit -> 'patch', 1.0.0-beta.1 -> 1.0.0-beta.2 -> 'patch'), the widened kind change (type -> value, which maps to 'additive'), or the documented workspace: specifier non-resolution scope boundary. All three are behaviors of scripts/lib/api-surface.mjs and check-version-bump.mjs for which this file is the designated coverage. Add one test per path; none currently risks a false pass, so severity is low.


tangletools · 2026-08-18T21:14:35Z · trace

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — ecd21506

Review health 100/100 · Reviewer score 14/100 · Confidence 95/100 · 20 findings (3 medium, 17 low)

glm: Correctness 14 · Security 14 · Testing 14 · Architecture 14

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 9 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM No tests exercise the script or its parsing lib — scripts/check-api-surface.mjs

tests/ contains zero references to check-api-surface.mjs or lib/api-surface.mjs (verified by grep), while the sibling gate scripts/check-version-bump.mjs has a 634-line test file (tests/version-bump-check.test.ts). The branches unique to re-export-form emit — ExportAllDeclaration following (line 144-153), re-export specifiers (line 128-133), resolveRelativeDeclaration (line 81-93), expandWildcard

🟠 MEDIUM Prerelease versions compared as strings, violating semver numeric-identifier ordering — scripts/lib/api-surface.mjs

Final line: return head.prerelease > base.prerelease ? 'patch' : 'lower' compares prerelease tags with the string relational operator. Semver compares dot-separated identifiers numerically when both are numeric. Verified: versionBumpLevel('1.2.3-2','1.2.3-10') and ('1.2.3-alpha.9','1.2.3-alpha.10') both return 'lower', so a correct higher prerelease bump is reported as a downgrade and check-version-bump fails a compliant release with 'which is not higher'. Fail-loud, not a silent pass, and this repo publishes no prereleases today — but ordering is this function's entire job and the file is byte-identical in three repos. Fix: split on '.', compare identifier pairs with numeric comparison when both match /^\d++$/, else lexical; more identifiers at equality means higher.

🟠 MEDIUM false exports recorded as assets, so blocking a subpath under-charges as additive — scripts/lib/api-surface.mjs

declarationTargetOf returns null for a boolean, so "./internal": false falls to assets.push(subpath) (verified: resolveExportEntries({exports:{'.':'./dist/index.d.ts','./internal':false}}) -> assets:["./internal"]). compareSurfaces then reports a newly added false export as added ... (entry point, no declarations) -> surfaceSeverity 'additive' -> patch only on 0.x. But adding false BLOCKS a subpath consumers could previously deep-import — a breaking change charged at patch is the silent-pass direction this gate exists to close. Removing a false export symmetrically over-charges as breaking. Not reachable via this repo's current exports map, but the header states the module is generic and byte-identical across agent-eval/agent-knowledge/agent-runtime. Fix: track false exports

🟡 LOW Surface record is name/kind-scoped; interface shape changes are invisible to the gate — api-surface.json

The record captures export names and their kind (value|type) only. Removing a field from an exported interface (e.g. dropping a property from ChatRequest) leaves api-surface.json byte-identical, so check:version-bump passes without a bump. This is a documented, deliberate scope decision in scripts/lib/api-surface.mjs:25-29 ('names and kinds are the granularity that matches the defect — a name that resolves in no published version'), not a bug in this file, but it bounds the protection this snapshot provides. No change required for this shot; the global verifier should know the gate's coverage boundary.

🟡 LOW oxc-parser pinned exact (0.144.0), diverging from the caret-range convention used by every other devDependency — package.json

Every other devDependency uses a caret range (e.g. "publint": "^0.3.22"); oxc-parser is now an exact pin "0.144.0". Evidence: commit history ('chore(analyst): re-pin the dependency lock digest for the oxc-parser devDependency', twice) shows this was tuned against pnpm minimumReleaseAge: 4320 lock resolution, so it is deliberate, and the lockfile is consistent (specifier 0.144.0, resolved 0.144.0, installed 0.144.0 verified). Impact is maintenance-only: version movement now requires a manual edit plus lockfile update, and a future contributor re-adding '^' could silently re-introduce the digest churn the re-pin commits fixed. Fix: none required; optionally a one-line comment or a note in the PR description stating the pin is required by minimumReleaseAge would prevent accidental un-pinning.

🟡 LOW Specifier style changes from caret range to exact pin — pnpm-lock.yaml

Specifier went from '^0.141.0' to exact '0.144.0', mirroring package.json line 211; sibling devDeps (tsdown ^0.22.14, publint ^0.3.22) keep carets. Exact pinning of a native-binding parser is defensible for reproducibility, but the mixed style means future bumps of oxc-parser always require a package.json edit rather than a lockfile refresh. Consistency nit only; the lockfile faithfully reflects the manifest and frozen install passes.

🟡 LOW wasm32-wasi platform binding removed with 0.144.0 bump — pnpm-lock.yaml

0.141.0 carried '@oxc-parser/binding-wasm32-wasi' (with @emnapi/core, @emnapi/runtime, @napi-rs/wasm-runtime deps); 0.144.0 has no wasm32-wasi entry — upstream removed it. Impact: oxc-parser can no longer load under a wasm32-wasi host. This repo runs Node only (engines ^20.19.0 || >=22.12.0, CI on Node), and the emnapi packages remain for @rolldown/binding-wasm32-wasi, so nothing here regresses. No action needed; recording so the platform drop is a known consequence, not an accidental lockfile pruning.

🟡 LOW Declaration resolution misses .cjs and misorders .d.mts — scripts/check-api-surface.mjs

The candidate list tries base.replace(/.m?js$/, '.d.ts') before '.d.mts', but Node16/Nodenext resolution maps .mjs -> .d.mts and .cjs -> .d.cts. For a '.cjs' specifier the regex does not match (the only dot is followed by 'c'), so the last candidate is the unchanged base — if the runtime dist/foo.cjs exists, existsSync passes and the function returns the runtime JavaScript file, which is then parsed with lang:'dts' and dies on a parse error instead of finding dist/foo.d.cts. Latent today (no re-export-from statements in the current emit), and it fails loud rather than silently, but for a fleet-shared rule the mapping should be extension-aware: .mjs -> .d.mts first, .cjs -> .d.cts, .js -> .d.ts.

🟡 LOW Re-export specifier kind guessed as value without consulting target — scripts/check-api-surface.mjs

For 'export { X } from "./y"' the name is recorded with kind 'value' unless an inline 'type' marker is present; the actual kind of X inside y is never resolved. A type-only re-export emitted without the marker is misrecorded as a value. Impact is bounded: names still land in the record, so add/remove detection is unaffected; only kind transitions (value->type narrowing) could be misreported, and the current emit (import + local export list, all types marked inline) never takes this branch. If the fleet-wide form ever relies on re-export-from, resolve the kind by parsing the target the way the star-export branch already does.

🟡 LOW TSExportAssignment silently skipped — scripts/check-api-surface.mjs

The top-level filter is if (!node.type.startsWith('Export')) continue, so a TSExportAssignment ('export =' in a .d.ts, CommonJS-style export) is skipped without error — while every unrecognized node that DOES start with 'Export' throws (line 158). That contradicts the file's own fail-loud posture: an entry point that exports via 'export =' would record an empty surface and look identical to an asset. Not reachable with this repo's tsdown emit; fix by adding an explicit case that throws for TSExportAssignment (or records it as 'default').

🟡 LOW export * from copies default and ignores ambiguity — scripts/check-api-surface.mjs

When following ExportAllDeclaration without an exported name, every entry of the target's name map is copied, including 'default' — but ES module semantics never re-export default through a star. Two star-exports of the same name (ambiguity exclusion) are also not implemented. Both over-report, which pushes the gate stricter (a later chunk change would flag a phantom 'removed' default and demand a bump it may not need) rather than looser. Latent: zero 'export * from' in the current dist emit. Fix: skip 'default' when merging star-exports and drop names star-exported by more than one target.

🟡 LOW Check silently no-ops on the publish workflow's shallow checkout — scripts/check-version-bump.mjs

publish.yml (tag push, actions/checkout@v7 default depth 1) also runs verify:package, so this script runs there with no origin/main in the clone; resolveBase returns null and the script prints 'No base ref to compare against' and exits 0. The gate is PR CI (ci.yml has fetch-depth: 0), so enforcement holds, but a future workflow that relies on the publish-time run for enforcement gets a silent pass. Consider treating GITHUB_EVENT_NAME=push with a tag ref as needing an explicit base, or documenting that only PR CI enforces.

🟡 LOW First-introduction PR gets a one-time free pass on export changes — scripts/check-version-bump.mjs

When the base has no api-surface.json for a package, exportChanges is forced to empty, so the same PR that introduces the record may also add exports with no version payment (tests/version-bump-check.test.ts:608 pins this as intended bootstrap). One-time per package per repo and the head-side record is mandatory (missingRecord fails closed), so exposure is bounded to the transition PR — this PR itself. Acceptable; worth a line in the header acknowledging the window is payable-once.

🟡 LOW Prerelease versions compared lexicographically, so rc.10 < rc.2 — scripts/check-version-bump.mjs

paidLevel = versionBumpLevel(base, head) ends in head.prerelease > base.prerelease, a raw string compare. Executed fixture: base 1.0.0-rc.2 with an added export, head 1.0.0-rc.10 -> output 'moves 1.0.0-rc.2 -> 1.0.0-rc.10, which is not higher' and exit 1, though semver orders rc.10 above rc.2. Impact: a legitimate prerelease advancement with numeric segments >= 10 is blocked (false positive, fail-closed direction only — never a false pass). Fix: split the prerelease on '.' and compare dot-separated identifiers per semver rule 11 (numeric identifiers compare numerically) in lib/api-surface.mjs.

🟡 LOW Dirent.parentPath requires Node >= 20.12 but engines declares >= 20 — scripts/lib/api-surface.mjs

readdirSync(..., {recursive:true, withFileTypes:true}) Dirents expose parentPath only from Node 20.12.0 (v20.11 and earlier name it path). On those versions relative(directory, entry.parentPath) receives undefined and throws a bare TypeError instead of a diagnosable message. Loud crash, only on packages whose exports use a wildcard pattern (this repo's map has none). Fix: read entry.parentPath ?? entry.path, or raise engines to >=20.12.

🟡 LOW Wildcard stem used as a replacement string, so $ sequences corrupt the recorded subpath — scripts/lib/api-surface.mjs

subpath.replace('*', stem) interprets $&, $', $` and $$ in the replacement. Verified: a built file a$&b.d.ts is recorded as subpath ./bench/a*b — the $& replaced by the matched *. The record then names an import specifier that does not exist; it round-trips deterministically so the staleness check cannot catch it. Pathological module names only. Fix: use a replacer function (subpath.replace('*', () => stem)).

🟡 LOW Wildcard validation checks presence, not count, despite its own error message — scripts/lib/api-surface.mjs

The error text says a subpath and target 'must both carry exactly one "*"' but the guard only tests .includes('*'). Verified: a two-star target ./dist/*/*.d.ts falls through to expandWildcard, whose split('*') destructuring silently drops the middle segment, and surfaces as the misleading 'matches no built file' error. npm forbids multiple stars so no valid manifest hits this; align the check with the message (count occurrences) or fix the message.

🟡 LOW Named catalogs (catalog:) and widened exports untested — tests/version-bump-check.test.ts

resolveSpecifier (scripts/check-version-bump.mjs:250-263) branches on catalog:<name> resolving through catalogs.named, and compareSurfaces emits a 'widened' (type -> value) change classified additive; neither path is exercised — all fixtures use the default catalog: and only value->type narrowing. Both branches fail-closed/additive respectively so risk is low, but a fixture with a named catalog and one type->value flip would pin them.

🟡 LOW Prerelease version transitions are untested — tests/version-bump-check.test.ts

versionBumpLevel in scripts/lib/api-surface.mjs:256-262 has distinct branches for same-triple prerelease moves (base 1.0.1-rc.1 -> head 1.0.1 pays 'patch'; adding a prerelease to a released version pays 'lower'; prerelease string comparison). This file is the lib's only test coverage and no fixture ever uses a prerelease version, so those branches run only in production. Add one test per direction (e.g., 1.0.0 -> 1.0.1-rc.1 must be rejected as 'lower'; 1.0.1-rc.1 -> 1.0.1 must pay for an additive export change).

🟡 LOW merge-base failure and GITHUB_BASE_REF mapping untested — tests/version-bump-check.test.ts

Two fail-closed paths in scripts/check-version-bump.mjs have no test: line 347-349 ('shares no history with HEAD' when the base ref resolves but has no common ancestor) and the GITHUB_BASE_REF -> origin/ translation at line 157 (a set, non-empty GITHUB_BASE_REF should be used verbatim as origin/). The CI fail-closed test covers only the empty-GITHUB_BASE_REF case. A second fixture repo with no shared history covers the first; an env-only assertion covers the second.


tangletools · 2026-08-18T21:19:42Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 184aa525

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-19T07:20:01Z

@drewstone

Copy link
Copy Markdown
Contributor Author

Closing unmerged — operator decision. The pattern has legitimate prior art (Angular's committed API goldens, api-extractor's .api.md reports), but that context is 1.0 frameworks with mass external consumers. agent-eval is 0.x by settled policy, its consumers are the fleet moving in lockstep, and surface-deletion breakage already has its fix at the consumer side (explicit version-window ranges; runtime refuses 0.x carets). ~1,700 lines of gate code plus a 3,485-line committed artifact is disproportionate machinery for a solved problem — the growth-autopsy instrument-share finding applies. Recorded in the redesign reject list so it is not rebuilt.

@drewstone drewstone closed this Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants