fix(exports): restore root pairArms consumed by published dists; enforce d.ts/js agreement - #583
Conversation
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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟡 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:
pairArmsis 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 newverifyDistTypeRuntimeAgreementis called at scripts/verify-package-exports.mjs:146 insideverify: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:packagefamily in the same script (verifyPackedDependencyCohort, verifyVersionLock, etc.) and reuses the existingrunhelper at line 1273 and the existing tsc binary under node_modules/.bin. It complements rather than competes withattw --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
exportform 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.
✅ No Blockers —
|
| 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, duplicatingversion = "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 assertingversion('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
candidatesfilters outname !== 'default', butcollectDtsExportNamesform 1 (line 1084) parsesexport { foo as default }and pushes the namedefault. 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.tsexport { foo as default }+ js with no default passes the check. The plainexport defaultform 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 withexport. Indented export lines (e.g. insidedeclare 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
^exportat line start, so twoexportstatements 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.jsondoes 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 defaultandexport async functionwould trigger the unrecognized-export error at line 1136. While the fail-loud behavior is correct for unexpected generator changes,export defaultis a valid TS syntax that is intentionally skipped (line 1006 filters name !== 'default'). If tsup ever addsexport defaultto barrel d.ts files, the error message would be confusing. Acknowledgeexport defaultexplicitly 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 ...orexport = .... 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 emitsexport 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 thattsc.statusis 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
pairArmsis 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 assertspairedSignTestbut notpairArms. Fix: addif (!('pairArms' in root)) throw new Error('missing root export pairArms')next to the pairedSignTest probe.
tangletools · 2026-08-12T03:02:38Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
Problem
The 0.145.0 fleet bump crashed consumers that install published
agent-knowledgedists: 7.0.8 and 7.0.10 importpairArmsfrom the@tangle-network/agent-evalpackage ROOT at ESM link time. Commit 94c3ced (#581) droppedpairArmsfrom 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-evalimport 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 packeddist/index.jsat 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
pairArmsto the root barrel, withPairArmsOptions,PairArmsResult,MatchedPair,MatchedRunRecordPair,PairRunRecordsResulttype exports (parity with 0.144.12).verify:packagegainsverifyDistTypeRuntimeAgreement: 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: removingpairArmsfromdist/index.jsalone fails the script with.: d.ts declares value pairArms but the js does not export it.__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 typecheckclean;pnpm test: 5190 passed, 3 skipped;pnpm buildclean;pnpm verify:packageexit 0.import { pairArms, type PairArmsOptions, ... }from root compiles against the packed tarball.