Skip to content

fix(exports): restore root pairArms consumed by published dists; enforce d.ts/js agreement - #583

Merged
drewstone merged 1 commit into
mainfrom
hotfix/0145-1
Aug 12, 2026
Merged

fix(exports): restore root pairArms consumed by published dists; enforce d.ts/js agreement#583
drewstone merged 1 commit into
mainfrom
hotfix/0145-1

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

The 0.145.0 fleet bump crashed consumers that install published agent-knowledge dists: 7.0.8 and 7.0.10 import pairArms from the @tangle-network/agent-eval package ROOT at ESM link time. Commit 94c3ced (#581) dropped pairArms from the root barrel; the removal shipped in 0.144.13 and carried into 0.145.0. A static ESM named import of a missing export fails before any code runs.

Audit basis (published dists, not sources)

Extracted every @tangle-network/agent-eval import specifier from the dist JS of the published npm tarballs of agent-runtime 0.128.0 / 0.132.4 and agent-knowledge 7.0.8 / 7.0.10 / 7.2.2 — 90 distinct symbols across root, /analyst, /campaign, /contract, /pipelines, /experiment — then probed each against 0.145.0's packed dist/index.js at runtime.

Result: pairArms (root) was the only missing symbol. agent-runtime imports none of the missing set; agent-knowledge 7.2.2 already uses /experiment.

Change

  • Restore pairArms to the root barrel, with PairArmsOptions, PairArmsResult, MatchedPair, MatchedRunRecordPair, PairRunRecordsResult type exports (parity with 0.144.12).
  • verify:package gains verifyDistTypeRuntimeAgreement: for every typed entry point, a name the d.ts declares as a value must be a runtime export of the js, and the reverse. TypeScript itself classifies each candidate (namespace-import probe), so the check does not depend on the declaration bundler's format. Kill test: removing pairArms from dist/index.js alone fails the script with .: d.ts declares value pairArms but the js does not export it.
  • Release 0.145.1: version lock across package.json / pyproject.toml / __init__.py / uv.lock; analyst dependency-lock digest repinned; CHANGELOG entry.

Note: the published 0.145.0 tarball's d.ts and js were measured to AGREE on all 26 entry points (the reported d.ts/js mismatch does not reproduce against the npm artifact; it is explained by version skew in the reporting consumer's install). The new check makes the class unshippable regardless.

Proof

  • pnpm typecheck clean; pnpm test: 5190 passed, 3 skipped; pnpm build clean; pnpm verify:package exit 0.
  • Packed 0.145.1 tarball: 90/90 audited consumer symbols resolve at runtime; independent d.ts-vs-js diff: 26/26 entry points agree.
  • tsc probe: import { pairArms, type PairArmsOptions, ... } from root compiles against the packed tarball.

Published agent-knowledge dists (7.0.x) import pairArms from the
package root at ESM link time. Commit 94c3ced dropped it from the
root barrel, so those packages crash at import with 0.144.13+.
Restore pairArms and its types (PairArmsOptions, PairArmsResult,
MatchedPair, MatchedRunRecordPair, PairRunRecordsResult) at the root.

Audit basis: every import specifier extracted from the dist JS of
agent-runtime 0.128.0 / 0.132.4 and agent-knowledge 7.0.8 / 7.0.10 /
7.2.2, probed against the packed tarball at runtime. pairArms was the
only missing symbol; 90 of 90 now resolve.

verify:package now proves every typed entry point's d.ts and js agree
exactly, in both directions. TypeScript classifies each candidate name
off a namespace-import probe, so the check is independent of the
declaration bundler's output format.

Release 0.145.1: version lock moved across package.json, pyproject,
__init__.py, and uv.lock; analyst dependency-lock digest repinned.

@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 — d7a9f763

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.

tangletools · auto-approval · reason: drewstone_author · 2026-08-12T02:48:51Z

@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
Concerns 2 (1 medium-concern, 1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 214.6s (2 bridge agents)
Total 214.6s

💰 Value — sound-with-nits

Correctly restores the dropped root pairArms export that crashed published agent-knowledge dists, and adds a well-built d.ts/js agreement check; but leaves pairArms off the existing consumer-contract pin list that was built specifically to prevent this regression class.

  • What it does: Restores pairArms (and PairArmsOptions, PairArmsResult, MatchedPair, MatchedRunRecordPair, PairRunRecordsResult) to the root barrel at src/index.ts:507-509, reversing the drop made by #581 (commit 94c3ced), then ships 0.145.1 with version-lock and analyst dependency-lock digest repinned across package.json/pyproject.toml/init.py/uv.lock. Adds verifyDistTypeRuntimeAgreement (~175 lines) to scri
  • Goals it achieves: Unblock consumers whose PUBLISHED dists (not just sources) import pairArms from the package root at ESM link time — agent-knowledge 7.0.8/7.0.10 crash on import with 0.144.13+. Secondary: prevent future d.ts/js drift on any entry point by catching it in CI/publish before it ships.
  • Assessment: The fix is correct, minimal, and necessary: 94c3ced dropped pairArms from a barrel that published dists still import; the PR body's audit (probing 90 specifiers from 5 published tarballs) found pairArms was the only missing symbol, and the PR restores exactly that surface. The digest repin is forced (package.json/pyproject/uv.lock changed). The new agreement check is well-designed for ITS goal — i
  • Better / existing approach: Searched: tests/consumer-contract.test.ts (the designated pin list), tests/*.test.ts for pairArms, src/experiment/index.ts (subpath), the full verify-package-exports.mjs, and attw's scope. The pairArms restoration itself is the right approach. The new agreement check is a net positive but orthogonal to the regression. The existing, in-repo guard for 'don't drop a root symbol a consumer imports' is
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A targeted export restoration plus a durable d.ts/js agreement check wired into the existing CI/publish verification pipeline — both halves will be used.

  • Integration: pairArms is restored to the root barrel at src/index.ts:509 and is consumed by published agent-knowledge dists (7.0.x) at ESM link time; the new verifyDistTypeRuntimeAgreement is called at scripts/verify-package-exports.mjs:146 inside verify:package, which runs in ci.yml:58 and publish.yml:49. Both changes are reachable and exercised on every push and every release. pairArms also flows throu
  • Fit with existing patterns: The new check extends the established verify:package family in the same script (verifyPackedDependencyCohort, verifyVersionLock, etc.) and reuses the existing run helper at line 1273 and the existing tsc binary under node_modules/.bin. It complements rather than competes with attw --pack --profile esm-only (also in verify:package): attw statically resolves types but does not execute the runt
  • Real-world viability: The d.ts parser (collectDtsExportNames, line 1074) is fail-closed: it throws on any unrecognized export form so a d.ts generator format change cannot silently skip names. The runtime probe loads real ESM via --input-type=module, and the TypeScript namespace-import probe correctly distinguishes type-only names (expected TS errors at probe lines >= 2) from genuine value/runtime mismatches. The f
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: commented out code src/index.ts

+// import it from the package root at ESM link time.

💰 Value Audit

🟠 pairArms not added to consumer-contract.test.ts pin list — the guard built to prevent exactly this regression [maintenance] ``

tests/consumer-contract.test.ts:55-106 (ROOT_RUNTIME_SYMBOLS) was created by #581 with the explicit purpose (per that commit's message) of pinning 'each restored symbol with the importing repo named, so the next tiering pass cannot drop them silently.' It pins pairRunRecords (line 89, from the same module) but not pairArms. The next tiering pass that treats this file as the source of truth — which is its documented role — will reason pairArms has no consumer and drop it again, recreating this ex


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 · 20260812T025430Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — d7a9f763

Review health 100/100 · Reviewer score 77/100 · Confidence 85/100 · 13 findings (13 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 83 86 77 77
Confidence 85 85 85 85
Correctness 83 86 77 77
Security 83 86 77 77
Testing 83 86 77 77
Architecture 83 86 77 77

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

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

🟡 LOW Changelog cites wrong removal version for pairArms — CHANGELOG.md

Line 15: 'so its removal in 0.144.13 crashed those consumers at import'. pairArms was removed in 0.145.0 (the deep clean, PR #581), not 0.144.13 (GEPA callback timeout alignment). Correct to 'removal in 0.145.0'.

🟡 LOW Date separator format inconsistent with the rest of the file — CHANGELOG.md

Line 11 uses ## [0.145.1] — 2026-08-11 (em-dash) while the majority of the file's entries use a hyphen, e.g. ## [0.144.13] - 2026-08-11 (line 55). The immediate neighbor 0.145.0 (line 20) also uses the em-dash, so this is consistent with the newest sections but inconsistent with the historical body. Cosmetic only; no functional impact. Fix: normalize the em-dash separators in the 0.145.x sections to match the file's dominant hyphen style if desired.

🟡 LOW Duplicated version source of truth invites drift — clients/python/src/agent_eval_rpc/__init__.py

__version__ hardcodes '0.145.1' as a manual fallback for PackageNotFoundError, duplicating version = "0.145.1" in pyproject.toml:7. This PR updates both consistently (and uv.lock:37 matches), so current state is correct. Risk is future drift: a one-file bump (e.g. release tooling editing only pyproject.toml) would leave the source-tree fallback stale. Mitigation: derive the fallback from a single constant, or add a test asserting version('agent-eval-rpc') == __version__ when installed. Pre-existing pattern, not introduced by this PR.

🟡 LOW Default-export drift is never checked — scripts/verify-package-exports.mjs

candidates filters out name !== 'default', but collectDtsExportNames form 1 (line 1084) parses export { foo as default } and pushes the name default. Any d.ts that declares a default via the brace-alias form is consumed and then silently dropped, so a default present on one side and absent on the other never surfaces as a mismatch — contradicting the function's stated 'agree exactly, in both directions' goal. Verified empirically: a fixture with d.ts export { foo as default } + js with no default passes the check. The plain export default form does throw (unrecognized export), so this is a narrow, currently-theoretical gap (no default exports

🟡 LOW Indented exports bypass d.ts parser guardrail — scripts/verify-package-exports.mjs

All four export-pattern regexes (lines 1084, 1096-1097, 1104, 1111) and the unrecognized-export guard (line 1132) use ^ with /gm, meaning they only match lines that START with export. Indented export lines (e.g. inside declare module '...' { export ... }) would be silently skipped by both the collectors AND the guardrail. tsup-generated barrel files never indent, so this is not a current bug, but if the generator format changes to produce indented exports within namespaces, the guardrail designed to catch format chan

🟡 LOW Line-anchored export parsing leaves a silent-collection window — scripts/verify-package-exports.mjs

The unrecognized-export guard and all four forms anchor ^export at line start, so two export statements emitted on a single physical line (e.g. export const a = ...; export const b = ...;) would silently skip the second name. Because the candidate set is the union of collected d.ts names and runtime keys, this is only a false negative when a name is missed by the collector AND absent from runtimeKeys — the generator (tsdown/rollup dts) does not emit such forms today, and the two-sided check bounds the impact. Worth a defensive comment or handling, not blocking.

🟡 LOW No unit tests for the new parser and probe mapping — scripts/verify-package-exports.mjs

The ~180 new lines of regex parsing and tsc-output mapping (candidate index = tsc line - 2, error-format regex ^(.+?)\(\d+,\d+\): error TS\d+) are exercised only through the full verify:package release gate, which requires a build + npm pack + tsc install. tsconfig.script.json does not include this .mjs (no typecheck coverage either). A regression in the parser or a tsc output-format change would fail only at release time. Recommend a fast unit test running collectDtsExportNames over fixture d.ts files (single-line block, multi-line, aliases, export* chains) and a drift fixture asserting the pairArms-class error fires.

🟡 LOW Unrecognized-export guard doesn't distinguish known-skipped forms — scripts/verify-package-exports.mjs

export default and export async function would trigger the unrecognized-export error at line 1136. While the fail-loud behavior is correct for unexpected generator changes, export default is a valid TS syntax that is intentionally skipped (line 1006 filters name !== 'default'). If tsup ever adds export default to barrel d.ts files, the error message would be confusing. Acknowledge export default explicitly in the guard (skip it and add a comment) so a future tsup change produces a clear signal rather than a cryptic

🟡 LOW collectDtsExportNames does not handle export default or export = — scripts/verify-package-exports.mjs

The parser recognizes four export forms (named, declaration, namespace re-export, wildcard re-export) but not export default ... or export = .... Either form would trigger the unrecognized-export throw at line 1132-1138. This is partially by design (fail-loud on generator format changes), but the current package happens to avoid default exports — verified via grep of all non-test source. If a future entry point or d.ts generator emits export default, the verification will hard-fail with a confusing 'unrecognized export statement' message rather than a clear 'add default-export handling' message. Consider either explicitly documenting this as a k

🟡 LOW packageDir recomputed instead of passed as parameter — scripts/verify-package-exports.mjs

The function recomputes const packageDir = join(appDir, 'node_modules', '@tangle-network', 'agent-eval') instead of receiving it as a parameter. This duplicates the path knowledge from the call site (line 145 where the symlink is created). If the symlink target or package name ever changes, both locations need updating in sync. Minor DRY violation; not a correctness bug today.

🟡 LOW tsc diagnostic parsing depends on stable plain-format output — scripts/verify-package-exports.mjs

The diagnostic regex /^(.+?)\((\d+),\d+\): error TS\d+/ assumes tsc's non-pretty output format (file(line,col): error TSxxxx). If a future tsc version changes this format, the parser would silently miss type-only classifications, causing every type-only name to appear as a value-declared mismatch (false positive — loud failure, not silent). Mitigated by using the pinned tsc from repoRoot/node_modules/.bin/tsc, but there is no assertion that the parsed diagnostic count is plausible (e.g., if tscOutput contains error-like text but zero TS\d+ matches, the script proceeds with empty typeOnlyByEntry sets). A defensive check that tsc.status is either 0 or 1 (not a signal-based kill or unknown exit code) would add robustness.

🟡 LOW Comment says 'stays' but the export was removed and is being restored — src/index.ts

The comment 'pairArms stays a root export' implies the export was continuous. Git history (commit 94c3ced, PR #581) shows pairArms was deleted from root exports in 0.145.0 and this PR restores it. The CHANGELOG correctly uses 'Restored'. Suggest aligning the in-source comment to 'pairArms must remain a root export' to avoid the historical-tense wording and to match the repo's no-narrative comment rule. Cosmetic only; no functional impact.

🟡 LOW No explicit root-export regression guard for pairArms — src/index.ts

The restore is correct (surface matches pre-refactor 94c3ced~1 exactly), but nothing asserts pairArms is present at the package root. The new verifyDistTypeRuntimeAgreement in scripts/verify-package-exports.mjs only compares the packed d.ts against the packed js, so a future refactor that drops the export from BOTH files (as #581 did) would pass verify:package. The existing runtime probe at scripts/verify-package-exports.mjs:686 asserts pairedSignTest but not pairArms. Fix: add if (!('pairArms' in root)) throw new Error('missing root export pairArms') next to the pairedSignTest probe.


tangletools · 2026-08-12T03:02:38Z · 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.

✅ Approved — 13 non-blocking findings — d7a9f763

Full multi-shot audit completed 5/5 planned shots over 8 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 8 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 8 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-12T03:02:38Z · immutable trace

@drewstone
drewstone merged commit bb3faee into main Aug 12, 2026
2 checks passed
@drewstone
drewstone deleted the hotfix/0145-1 branch August 12, 2026 08:19
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