chore(deps): agent-eval 0.145.0 with the caller-owned model call - #85
Conversation
Bump @tangle-network/agent-eval from 0.143.0 to 0.145.0. Add a pnpm override so agent-runtime 0.126.0 cannot ghost a second eval version into the tree; pnpm resolves one version. agent-eval 0.144.0 removed apiKey and baseUrl from the DSPy RLM engine: the caller now owns each model call and returns a typed result, a cost receipt, and execution evidence. The CLI implements that call with agent-eval's own callLlm and receipt converters, and logs every execution observation to the analyst stderr log.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 00489095
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-12T00:43:08Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 229.9s (2 bridge agents) |
| Total | 229.9s |
💰 Value — sound
Forced migration across agent-eval 0.144.0's caller-owned-execution break, done in-grain using the library's own callLlm + cost-receipt primitives; no equivalent to reuse exists.
- What it does: Bumps @tangle-network/agent-eval 0.143.0 -> 0.145.0, adds a pnpm.overrides pin so agent-runtime 0.126.0 (peer <0.144.0) cannot drag a second eval version into the tree, and rewrites buildAnalysisEngine in src/cli.ts:504-567 to the new contract: createDspyRlmTraceEngine no longer takes apiKey/baseUrl, so the CLI now owns each model call via a
callcallback that wraps agent-eval's own callLlm, der - Goals it achieves: (1) Stay current with agent-eval so the published census law and downstream protocol stay aligned. (2) Comply with 0.144.0's 'Agent Eval never receives provider credentials' boundary: the CLI owns the one OpenAI-compatible HTTP hop and returns a typed result + cost receipt per call. (3) Keep one version of agent-eval in the install tree despite the not-yet-republished agent-runtime peer range. Net
- Assessment: Good change, executed in the grain of the codebase. The migration is forced by an upstream breaking change (DspyRlmTraceEngineOptions at node_modules/@tangle-network/agent-eval/dist/index-BWDrSVfw.d.ts:160-206 no longer accepts apiKey/baseUrl), and the implementation responds by reusing agent-eval's own primitives (callLlm, costReceiptFromLlm, costReceiptFromLlmError) rather than hand-rolling an H
- Better / existing approach: none — this is the right approach. I searched for an existing OpenAI-compatible caller to extend: src/replay-fix.ts:42 (zaiChatCaller) POSTs to ${baseUrl}/chat/completions with a bearer token, but it returns {content, usage:{promptTokens, completionTokens}} — a strictly weaker shape that strips the canonical ChatResponse and reduces usage to two token counts. The new ExternalOptimizerModelCallResu
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A forced dependency migration (agent-eval 0.144.0's caller-owned breaking change) implemented correctly against the real 0.145.0 contract, on the live --llm path, plus a dedup pin that holds the tree to one version.
- Integration: Fully wired and reachable. buildAnalysisEngine (src/cli.ts:486) is the only engine constructor; it is called from investigate (src/cli.ts:1216) and cmdImprove (src/cli.ts:1241), both gated on the shipped --llm flag (src/cli.ts:244,1697). Every new symbol imported (callLlm, costReceiptFromLlm, costReceiptFromLlmError, LlmCallRequest) exists in the installed agent-eval 0.145.0 dist (types-Cx3YUh2r.d
- Fit with existing patterns: Fits the codebase's single-engine architecture with no competing pattern. DspyRlmTraceEngineOptions (index-BWDrSVfw.d.ts:160-208) dropped apiKey/baseUrl and now requires call/callRef/recordExecution; the CLI implements precisely that seam using agent-eval's own callLlm — i.e. the package's published primitive, not a hand-rolled fetch. TraceAnalysisEngine flows unchanged into analyze.ts:35 and impr
- Real-world viability: Handles the non-happy paths. callLlm is wrapped in try/catch; failures return a typed failure result with costReceiptFromLlmError(err) ?? an explicit unknown-receipt (costUnknown:true, usageUnknown:true) rather than a silent zero-cost receipt, so the fail-closed ledger treats unknowns honestly. The per-call AbortSignal is forwarded into callLlm. recordExecution logs the finite execution observatio
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
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.
❌ Needs Work —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 56 | 60 | 25 | 25 |
| Confidence | 75 | 75 | 75 | 75 |
| Correctness | 56 | 60 | 25 | 25 |
| Security | 56 | 60 | 25 | 25 |
| Testing | 56 | 60 | 25 | 25 |
| Architecture | 56 | 60 | 25 | 25 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.
Blocking
🔴 HIGH Override forces agent-eval@0.145.0 + agent-interface@0.46.1 into agent-runtime@0.126.0 despite peer ranges that exclude them; no published agent-runtime version supports agent-eval — pnpm-lock.yaml
Snapshot resolves agent-eval@0.145.0 and agent-interface@0.46.1, but this same file's metadata (L679-685) declares agent-runtime peers agent-eval '0.145.0' / agent-interface '>=0.43.0 <0.44.0' — 0.46.1 violates the declared range internally, and the published agent-runtime@0.126.0 tarball (identical integrity sha512-JTsF13tdb0vv+Tb2Rzm9M2vm6izLno7bJ0Yk/VC0zDE63xllIia0tNKBj2ZGDAkbNFARSIz3wqGFZ/A00x52Yg==) declares agent-eval peer '>=0.143.0 <0.144.0'. I regenerated with the pinned pnpm 10.33.4: install succeeds but pnpm warns '✕ unmet peer @tangle-network/agent-interface@">=0.43.0 <0.44.0": found 0.46.1' for agent-runtime and agent-profile-materialize@0.10.2 (peer '>=0.38.0 <0.44.0', L1728). Registry audit of every agent-runtime 0.123.1..0.132.4 shows all agent-eval peers cap at '<0.145.0',
Other
🟠 MEDIUM pnpm override force-pins agent-eval 0.145.0 onto agent-runtime outside its declared peer range — package.json
The new override forces @tangle-network/agent-eval to 0.145.0 tree-wide, but agent-runtime@0.126.0 (an unchanged dependency) declares peer '@tangle-network/agent-eval': '>=0.143.0 <0.144.0' in the base lockfile. The head lockfile rewrites that declared range to '0.145.0' (pnpm-lock.yaml:683-684), masking the violation. The same consumer's peer '@tangle-network/agent-interface' resolves 0.46.1, outside its declared '>=0.43.0 <0.44.0' (was 0.43.0 in base). agent-eval 0.144.0 contained a breaking change (removed provider-credential path, per the src/cli.ts migration comment), so 0.145.0 is a breaking-change minor forced onto a consumer that was explicitly bounded below 0.144.0. Fix: confirm agent-runtime@0.126.0 is API-compatible with agent-eval@0.145.0 + agent-interface@0.46.1, or bump agent
🟠 MEDIUM pnpm.overrides forces agent-eval 0.145.0 onto agent-runtime@0.126.0 outside its declared peer range — package.json
agent-runtime@0.126.0 (still pinned at line 63) declares peerDependencies.@tangle-network/agent-eval as ">=0.143.0 <0.144.0" (verified via
npm view @tangle-network/agent-runtime@0.126.0 peerDependencies). The override at lines 67-70 forces 0.145.0, which is outside that range and is a documented breaking release: agent-eval CHANGELOG for 0.144.0 and 0.145.0 lists breaking changes including the caller-owned ChatClient migration, RawAnalystFinding.evidence shape change, defineTraceAnalyst return-type change, and removal of provider SDK types. agent-runtime@0.126.0's compiled
🟠 MEDIUM Unmet peer dependencies: agent-runtime 0.126.0 and agent-profile-materialize 0.10.2 receive agent-interface 0.46.1 outside their declared <0.44.0 peer range — pnpm-lock.yaml
agent-eval@0.145.0 (forced by the new override) depends on agent-interface@0.46.1 and agent-core@0.5.4. The runtime snapshot at line ~1715 records dependencies on agent-eval 0.145.0 AND agent-interface 0.46.1, but the runtime package's own peer declaration still reads
@tangle-network/agent-interface: '>=0.43.0 <0.44.0'(line ~684). pnpm install (frozen and fresh) emits:unmet peer @tangle-network/agent-interface@">=0.43.0 <0.44.0": found 0.46.1for both @tangle-network/agent-runtime 0.126.0 and @tangle-network/agent-profile-materialize 0.10.2 (peer>=0.38.0 <0.44.0). 0.43.0 -> 0.46.1 is three minor bumps in 0.x semver where minor = potentially breaking. The lockfile is installable but the transitive graph now ships a peer combination the runtime author did not bless. Impact: possible
🟠 MEDIUM agent-interface version split across the dependency tree — pnpm-lock.yaml
agent-knowledge@7.0.8 depends on agent-interface@0.43.0 (line 1724), but its surrounding packages — agent-eval@0.145.0 (line 1702), agent-runtime@0.126.0 (line 1735), agent-core@0.5.4 (line 1694) — all use agent-interface@0.46.1. agent-runtime receives agent-interface@0.46.1 as a peer dep but passes objects into agent-knowledge, which validates against 0
🟠 MEDIUM package.json pnpm.overrides shadows and silently disables the pnpm-workspace.yaml overrides, downgrading esbuild 0.28.1 -> 0.27.7 — pnpm-lock.yaml
The committed lockfile's
overrides:block now lists only@tangle-network/agent-eval: 0.145.0. The three overrides previously pinned in pnpm-workspace.yaml (esbuild ^0.28.1, vite ^8.1.2, ws ^8.21.0, added deliberately by Drew in commit da6c901) no longer take effect. Verified by regen: with both files present, pnpm 10.33.4 resolves esbuild to 0.27.7 for tsup/vite/vitest (see snapshots at bundle-require@5.1.0(esbuild@0.27.7), vite@8.1.5(...esbuild@0.27.7...), vitest@4.1.10(...esbuild@0.27.7...)) and 0.28.1 lingers only as an optional platform-package leftover. Proved causally: addingesbuild: 0.28.1into package.json's pnpm.overrides forces esbuild@0.28.1 in the regen, while the same directive in pnpm-workspace.yaml does nothing. Impact: any reason the maintainer originally pinned esbu
🟠 MEDIUM Echoed-model identity now hard-fails alias/renaming gateways — src/cli.ts
In agent-eval 0.145.0,
snapshotModelReceiptandsnapshotChatResponse(external-optimizer-model-proxy.ts:631-633, 937-941) require receipt.model and response.model to equal the engine's configured model. The CLI's receipt usescostReceiptFromLlm(response)whosemodelisservedModel ?? req.model(llm-client.ts), so any OpenAI-compatible gateway that echoes a canonical/dated id different from the requested--model(common with LiteLLM/version-resolving routers — the exact OPENAI_BASE_URL use-case this CLI advertises) fails EVERY analyst call with an opaqueoptimizer model callback response used 'X' instead of 'Y'OwnerModelContractError → 502, then bridge retries exhaust. 0.143.0's proxy forwarded the raw provider body with no echoed-model check, so this is a behavior regression
🟠 MEDIUM request.callId is dropped instead of becoming the provider idempotency key — src/cli.ts
callLlm(req, { apiKey, baseUrl, signal })omitsidempotencyKey. The proxy passes a stablecallIdon every admitted call (ExternalOptimizerModelCallRequest.callId, 'Stable identity allocated by the cost ledger for this paid call'), and agent-eval's own reference owner passes it through:client.call(structuredClone(request.request), { signal: request.signal, idempotencyKey: request.callId })(model-execution.test-support.ts:34-36). callLlm retries transient failures up to 3 attempts; if attempt 1 is processed and billed by the provider but the response is lost (timeout/ECONNRESET), the retry re-sends the same logical call without an Idempotency-Key header, so the provider can double-bill and the run is charged once. Fix: `callLlm(req, { apiKey, baseUrl, signal, idempotencyKey: reques
🟠 MEDIUM structuredClone outside try/catch can leak rejection — src/cli.ts
Line 510:
const req = structuredClone(request) as LlmCallRequestexecutes before the try block at line 511. If structuredClone throws (e.g. a non-cloneable getter on an unfamiliar engine version's request object), the error propagates as a promise rejection from the callback. The engine contracts (line 82-83 of external-optimizer-contracts-lixrOZdX.d.ts) state 'Rejecting loses the execution record and therefore fails the optimizer attempt' — a thrown clone error would bypa
🟡 LOW Override duplicates an already-exact direct dependency version — package.json
The direct dependency already pins agent-eval exactly to '0.145.0' (package.json:62); the overrides block restates the identical version. The override is what dedupes the tree, so it is not dead config, but the two must never drift apart — a future bump touching only one entry would silently split resolution (two copies) or violate peer ranges. Suggest a one-line comment noting the override must track the direct pin, or rely on the lockfile check to catch divergence.
🟡 LOW Stale lockfile overrides cleaned up — esbuild pin lost but benign — package.json
The previous lockfile had overrides for esbuild (^0.28.1), vite (^8.1.2), and ws (^8.21.0) that were not in package.json's pnpm.overrides. Regenerating the lockfile with the new pnpm.overrides block drops them. Verified: esbuild resolves to 0.27.7 which includes the 0.25.0 content-injection fix; ws is no longer a transitive dep; vite ^8.1.5 in devDependencies already exceeds ^8.1.2. No vulnerability reintroduced. If esbuild 0.28.1 was pinned for a non-security reason (e.g. a tsup/vitest edge case), consider re-adding the esbuild override.
🟡 LOW Orphaned lockfile entry '@tangle-network/agent-core@0.4.33' after removing agent-eval@0.143.0 — pnpm-lock.yaml
The packages (L652) and snapshots (L1687) sections still define agent-core@0.4.33, but its only dependent was agent-eval@0.143.0, which this PR removed; no snapshot references it anymore (grep over the snapshots section returns only its own definition). Harmless to installs (frozen install passed) but stale metadata that a clean 'pnpm install --lockfile-only' would prune. Fix: regenerate the lockfile.
🟡 LOW agent-knowledge@7.0.8 not bumped alongside agent-interface upgrade — pnpm-lock.yaml
agent-knowledge@7.0.8 still resolves agent-interface@0.43.0 while the rest of the tree uses 0.46.1. This is a transitive dep of agent-runtime@0.126.0. It may need a version bump to stay compatible with the surrounding ecosystem.
🟡 LOW esbuild downgraded from 0.28.1 to 0.27.7 — pnpm-lock.yaml
Removal of the esbuild:^0.28.1 override (package.json line 62-64 removed) causes pnpm to resolve esbuild@0.27.7 instead of 0.28.1. tsup@8.5.1→bundle-require@5.1.0 drives the resolution. While vite 8.1.5 and vitest 4.1.10 declare ^0.27.0 || ^0.28.0 compatibility, this is a full minor version step back that may miss fixes in 0.28.x. The downgrade was not mentioned in the commit message. Confirm esbuild 0.27.7 works with the build and test commands.
🟡 LOW esbuild split into two copies (0.27.7 for vite/tsup, 0.28.1 for tsx) after removing the esbuild override — pnpm-lock.yaml
Removing the esbuild:^0.28.1 override drops vite@8.1.5/tsup@8.5.1/bundle-require@5.1.0 to esbuild@0.27.7 while tsx@4.23.1 keeps 0.28.1 (L2317), yielding two native esbuild copies instead of the previously unified 0.28.1. No security impact: both are above the 0.25.0 fix for the esbuild dev-server arbitrary-read CVE (CVE-2025-61104), and both satisfy vite's esbuild peer '^0.27.0 || ^0.28.0'. This is a deduplication/size nit only; the base lockfile's esbuild/vite/ws overrides were never declared in base package.json, so their removal is consistent with the manifest.
🟡 LOW pnpm-workspace.yaml now contains misleading dead config (overrides block has no effect) — pnpm-lock.yaml
Side effect of the shadowing in finding #1. pnpm-workspace.yaml is unchanged by this PR and still declares
overrides: { esbuild: ^0.28.1, vite: ^8.1.2, ws: ^8.21.0 }.pnpm config liststill surfaces these, but the lockfile (the source of truth for installed versions) shows they are not applied. Future readers/maintainers will assume esbuild/vite/ws are pinned when they are floating. This is a maintenance trap, not a runtime defect. Fix: remove the overrides block from pnpm-workspace.yaml, or migrate the agent-eval override there and drop package.json's pnpm.overrides.
🟡 LOW Abort/timeout flattened to a retryable 502 instead of a cancel signal — src/cli.ts
When the caller signal aborts (analysis deadline, request-timeout controller), callLlm throws a DOMException AbortError; the catch here converts it into
{succeeded:false, error: err.message}. The proxy's abort branch (isAbortError(paid.error)→ 504) never fires because paid.error is now a message string;handleModelProxyRequestsends 502 viamodelCallFailed, which the Python bridge's client treats as transient and retries against a dead intent, delaying cancellation and wasting retries. The contract requires the callback to always resolve (re-throwing wraps into MissingModelExecutionError and fails the attempt), so the fix is limited: mark the error text so the proxy/bridge can distinguish 'aborted' (e.g. prefix withAbortError:), or accept the 502-on-cancel as designed. Low impa
🟡 LOW No test coverage for buildAnalysisEngine contract migration — src/cli.ts
grep across tests/ found zero references to buildAnalysisEngine, callLlm, or costReceiptFromLlm. This PR migrates the analyst execution seam from pass-through credentials to a caller-owned ExternalOptimizerModelCall, which is precisely the shape a unit test with a stub call callback and an injected rejection should pin. The gap predates the PR but the migration is the natural moment to add one. Low because tsc verifies the type contract and the change is mechanical against the new agent-eval exports.
🟡 LOW No test covers the call callback success or error paths — src/cli.ts
The only --llm test (tests/cli.test.ts:861) uses a fakeBridge that exits(1) at startup before the engine makes any model call. The call callback — the most complex new code in this PR — is never invoked in any test. Neither the successful callLlm path (costReceiptFromLlm, execution fields) nor the error path (costReceiptFromLlmError recovery, costUnknown fallback) nor the recordExecution logging is tested. Verified by reading the full test (line 863-940): fakeBridge only prints stderr and exits, so the engine fails at 'every agentic analyst died at startup', not at the call callback level.
🟡 LOW Provider error detail lost on non-2xx — src/cli.ts
error: err.messagesurfaces onlyLLM call failed with HTTP <status>—LlmCallError.body(the provider's actual reason: invalid key, context length, model not found) is discarded from both the execution evidence and the[analyst] FAILlog line, and the 502 body sent back to the bridge contains only that generic message. 0.143.0 forwarded the raw provider response body, so operators debugging a 400/401 lose the cause. Fix: include the truncatedbodyandstatusin theexecutionobject (and optionally in the error text), being careful to keep it out of log lines if the gateway echoes credentials.
🟡 LOW callId from Engine callback discarded — src/cli.ts
ExternalOptimizerModelCallRequest carries a readonly callId (line 44 of external-optimizer-contracts-lixrOZdX.d.ts) allocated by the cost ledger for each paid call. The destructuring at line 509 drops it:
async ({ request, signal }) =>. Neither the callLlm call nor the recordExecution callback uses callId, so per-call correlation between the callback's log output and the engine's internal ledger entries is lost. Not a bug — the engine retains callId internally — but recoverable via a one-line addition to execution/recordExecution.
🟡 LOW structuredClone of an already-immutable request is defensive overhead — src/cli.ts
ExternalOptimizerChatRequest is DeepReadonly by contract (external-optimizer-contracts-lixrOZdX.d.ts:35-39) and callLlm does not mutate its input, so the structuredClone before the cast to LlmCallRequest adds a deep copy of the messages array per call for no behavioral gain. The cast alone (or a shallow spread) would suffice. Keep if the goal is belt-and-suspenders protection against future callLlm mutations; otherwise drop. Negligible cost relative to the network call that follows.
tangletools · 2026-08-12T01:01:50Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
❌ 1 Blocking Finding — 00489095
Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 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-12T01:01:50Z · immutable trace
…pnpm override The published agent-runtime 0.132.13 peers on agent-eval '>=0.145.2 <0.146.0'. The override existed only to force past the old peer cap and is no longer needed. pnpm resolves one agent-eval version (0.145.3) across the tree.
agent-runtime 0.133.0 peers on sandbox '>=0.21.1 <0.22.0'. The repo held sandbox 0.17.2, which does not satisfy that range. sandbox 0.21.1 is published, so the peer contract closes with one version of each Tangle package in the tree.
The loopback proxy allocates callId as the cost ledger's stable identity for one paid call. callLlm retries transient failures, so without an idempotency key a response that is billed but lost is charged again on the retry. Also move structuredClone inside the try block. The engine contract requires the callback to resolve; a throw from clone rejected the promise and lost the execution record. The catch now reads the model from the immutable request.
Blocking finding addressed — the override is gone and the peers close on published versionsThe HIGH finding on Now on
Verified on the pushed head:
This also resolves the override side effects flagged in the review. With Two contract findings fixed in
|
Not merging: the merge block is a stale review, not a real redEverything this PR controls is green on State
What is actually green on
Why I am not merging anyway The blocking HIGH finding was the To land this, either: re-run the reviewer against |
…l record Move the owned model-call path out of the CLI into its own module so it can be tested directly. cli.ts runs main() at import, so the callback had no reachable test seam. Three defects in the failure path: - An abort resolved as a plain failure string. The proxy only sends 504 when it catches a thrown AbortError, so a cancelled call reached the bridge as a 502 that it retries against a dead intent. The failure text now names the class. - LlmCallError.body was dropped, leaving only the HTTP status. The provider's own reason now stays in the execution evidence, bounded, and out of the log line because a gateway can echo request headers into an error body. - callId was absent from the execution record, so a callback observation could not be correlated with the ledger entry for the same paid call. Cover the success, provider-error, and abort paths against a local OpenAI-compatible server, and raise agent-runtime to 0.133.2.
Remaining findings from
|
| requested | served | verdict | result |
|---|---|---|---|
gpt-4o |
gpt-4o-2024-08-06 |
alias | PASS |
glm-5.2 |
zai/glm-5.2 |
alias | PASS |
glm-5.2 |
glm-5.2:latest |
alias | PASS |
gpt-4o |
gpt-4o@20240806 |
alias | PASS |
gpt-4o |
gpt-4o |
exact | PASS |
gpt-4o |
claude-opus-5 |
substituted-cross-family | FAIL |
Only a genuine cross-family substitution fails, which is the intended integrity behaviour, not a regression. The unreported verdict is also unreachable from this path: a body that omits model leaves servedModel null, and both the receipt and the response carry servedModel ?? req.model, so the check sees exact.
LOW — structuredClone is defensive overhead. Keeping it. The line casts a DeepReadonly contract type to a mutable LlmCallRequest; the clone is what makes that cast honest rather than a lie about ownership. One deep copy of a messages array is negligible next to the HTTP call.
Already moot on this head
agent-interface split, agent-knowledge not bumped, esbuild downgrade / two copies, pnpm-workspace.yaml dead config, override duplicates the direct pin, orphaned agent-core@0.4.33 — all were consequences of the deleted override. On d258224 the tree resolves one version each: agent-eval 0.145.3, agent-interface 0.47.0, agent-runtime 0.133.2, agent-core 0.6.1, agent-knowledge 7.2.4, sandbox 0.21.1, esbuild 0.28.1. The pnpm-workspace.yaml overrides apply again (present in the lockfile header) and agent-core@0.4.33 is gone (grep -c = 0).
Checks on d258224
- Clean install (
rm -rf node_modules && pnpm install): exit 0, zero peer warnings. pnpm typecheckclean.pnpm test— 51 files, 657/657 passed (654 + 3 new). No flake this run.pnpm buildclean;pnpm check:packageok.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — d2582243
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-13T01:50:49Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 231.9s (2 bridge agents) |
| Total | 231.9s |
💰 Value — sound
Forced migration across the agent-eval 0.144.0 caller-owned-execution breaking change, implemented in the codebase's grain by reusing agent-eval's own primitives on the seam it defines — no better approach or existing equivalent found.
- What it does: Bumps @tangle-network/agent-eval 0.143.0→0.145.3 (plus the peer-cohort agent-runtime 0.126.0→0.133.2 and sandbox 0.17.2→0.21.1, with no pnpm.overrides) and migrates buildAnalysisEngine in src/cli.ts:486 across the 0.144.0 breaking change: createDspyRlmTraceEngine no longer accepts apiKey/baseUrl, so the CLI now supplies a caller-owned
callcallback (pluscallRefandrecordExecution). The cal - Goals it achieves: (1) Move off a version the rest of the cohort has passed, resolving the peer-cap violation by raising consumers rather than masking it with a tree-wide pnpm.overrides. (2) Own the model-execution path so the CLI controls retries, idempotency, and cost receipts instead of delegating provider credentials into the library. (3) Give that path a reachable test seam — cli.ts calls main() at import (src/
- Assessment: Sound and in-grain. The 0.144.0 change is documented in node_modules/@tangle-network/agent-eval/CHANGELOG.md lines 173-184 as a deliberate move of execution ownership to the caller ('Agent Eval no longer accepts provider credentials or performs provider retries for this path; the package that owns execution ... makes each admitted call'). The new module implements exactly that seam using the libra
- Better / existing approach: none — this is the right approach. Searched for a reusable implementation before concluding: (a) agent-eval itself ships no reference callback — the contract comment in node_modules/@tangle-network/agent-eval/dist/external-optimizer-contracts-lixrOZdX.d.ts states 'The package that owns execution implements this with its exact execution path,' so omission is by design; (b) @tangle-network/agent-run
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
Migrates the analyst model call onto agent-eval 0.145's caller-owned seam exactly as the published contract requires; it is reachable from live CLI paths, type-checks clean, and its tests prove the idempotency-key and failure-record behavior end-to-end.
- Integration: createAnalystModelCall is constructed once per buildAnalysisEngine call (cli.ts:505) and passed as the required
callseam to createDspyRlmTraceEngine. buildAnalysisEngine is itself called from theanalyze/improvecommands at cli.ts:1178 and cli.ts:1203, gated by--llm. The path is live in this PR, not waiting on a caller. The derived type `ExternalOptimizerModelCall = NonNullable<Parameter - Fit with existing patterns: This is the only caller-owned model execution path in the repo, and it is shaped exactly to the contract the engine dictates: returns the success/failure discriminated union with a
CostReceiptInputreceipt and an opaqueexecutionrecord on every admitted call.recordExecutionroutes the engine's own observation into the existinganalystLogchannel (cli.ts:507-512). A separate raw-fetch cal - Real-world viability: Failure paths are handled with explicit reasoning, not happy-path-only: abort is detected via
signal.aborted || err.name === 'AbortError'and named in the error text so the bridge's 504 branch can never fire on a cancel (analyst-model-call.ts:67-72); provider error bodies are captured viaLlmCallError.status/.bodyand bounded to PROVIDER_ERROR_BODY_LIMIT=2000 because the body is attacker-inf - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
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 | 68 | 74 | 58 | 58 |
| Confidence | 85 | 85 | 85 | 85 |
| Correctness | 68 | 74 | 58 | 58 |
| Security | 68 | 74 | 58 | 58 |
| Testing | 68 | 74 | 58 | 58 |
| Architecture | 68 | 74 | 58 | 58 |
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 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Provider timeout is recorded as a caller abort — src/analyst-model-call.ts
const aborted = signal.aborted || err.name === 'AbortError'conflates two distinct failures. Verified against agent-eval 0.145.3's callLlm (dist/llm-client): the per-attempt timeout doessetTimeout(() => controller.abort(), timeoutMs)and after retries exhaust, callLlm rethrows the fetch's DOMException with name 'AbortError' while the caller'ssignal.abortedstays false (empirically reproduced: error name 'AbortError', signal.aborted false). This path therefore returnsexecution.aborted: trueand error text prefixedAbortError:for a plain provider timeout — exactly the marker the bridge uses to distinguish a cancelled call from a transient one it should retry (per the comment at [lines 67-70](https://github.com/tangle-network/traces/blob/d258224318994162be93ee848accf4dd551c4036/
🟡 LOW Large minor-version jumps; API drift must be confirmed by source shots — package.json
agent-runtime jumps 0.126.0→0.133.2 (+7 minors) and sandbox 0.17.2→0.21.1 (+4 minors). package.json itself is correct and resolves, but whether src/cli.ts and src/analyst-model-call.ts still typecheck and behave against the bumped APIs is outside this shot's files. Global verifier should require a green typecheck + the new analyst-model-call tests against these exact versions before merge.
🟡 LOW engines.node range undershoots new dependency's engine floor — package.json
package.json declares "engines": { "node": ">=22" }, but the bumped @tangle-network/agent-runtime@0.133.2 now declares engines { node: '>=22.13.0' } (visible in pnpm-lock.yaml). A user on node 22.0–22.12 passes this package's engine gate but will hit an engine warning/failure when the runtime dependency loads. Impact: misleading support window; no runtime corruption. Fix: tighten to "node": ">=22.13.0" to match the effective floor of the new dependency set.
🟡 LOW sandbox 0.21.1 is ahead of the npm 'latest' dist-tag (0.21.0) — package.json
npm dist-tags show @tangle-network/sandbox latest=0.21.0 while this pin uses 0.21.1 (develop points at a 0.21.1-develop prerelease). 0.21.1 is published, resolvable, and not deprecated, so not blocking — just flag that the pin targets a patch-ahead release rather than the current 'latest'. Confirm intentional.
🟡 LOW agent-runtime engine floor above manifest engines — pnpm-lock.yaml
agent-runtime@0.133.2 declares engines node '>=22.13.0' while package.json engines is '>=22' (allows 22.0.0-22.12.x). Pre-existing (0.126.0 had the same floor, unchanged context line), so not introduced by this PR, and pnpm only warns without .npmrc engine-strict. Recommend aligning package.json engines to '>=22.13.0' for accuracy, but not a merge blocker.
🟡 LOW agent-runtime node engine floor raised to 22.13.0, package.json still declares >=22 — pnpm-lock.yaml
agent-runtime@0.133.2 records engines.node '>=22.13.0' (lockfile line 525), while this repo's package.json engines declares '>=22' (unchanged in this PR). CI uses node-version 22 which resolves to the latest 22.x (well above 22.13.0), so CI and most consumers are fine. But a consumer or downstream tool pinned to Node 22.0–22.12 would fail to install/run agent-runtime, while the package advertises >=22 support. This is a minor version-floor mismatch inherited from the upstream bump, not a lockfile bug. Fix if desired: bump package.json engines to '>=22.13.0' (outside this shot's scope) or accept the drift. Note the old agent-runtime@0.126.0 optional peer 'playwright ^1.40.0' was rem
🟡 LOW No coverage for the LlmResponseError receipt-recovery branch — src/analyst-model-call.ts
costReceiptFromLlmError(err) only returns a non-undefined receipt when err is an LlmResponseError (HTTP completed but contract violated). The test suite covers LlmCallError (400) and AbortError, but never the LlmResponseError path — so the branch that recovers a real receipt from a structured-response failure is exercised only by upstream package tests, not by this CLI's own suite. Add a test that returns a 200 with a body callLlm treats as a contract violation (or stub costReceiptFromLlmError) to assert the recovered receipt flows through instead of the {costUnknown:true, usageUnknown:true} fallback. Coverage gap only; production path is correct by inspection.
🟡 LOW No test asserts non-Error throws are normalized — src/analyst-model-call.ts
Line 66 coerces non-Error throws with
new Error(String(error)), but no test exercises a transport or upstream that throws a non-Error value (string, plain object). If a future callLlm revision throws a non-Error, the err.name check and the LlmCallError instanceof check both silently fall through to the generic failure path — which is the intended behavior, but unverified. Minor; add a one-line stub test if callLlm ever changes its throw contract.
🟡 LOW Stale agent-eval version in doc comment — src/analyst-model-call.ts
Comment says 'agent-eval 0.144.0 stopped accepting provider credentials' but package.json pins @tangle-network/agent-eval 0.145.3 and the upgrade commit 0048909 is titled 'agent-eval 0.145.0 with the caller-owned model call'. The version that actually dropped credentials is 0.145.0, not 0.144.0. Fix the comment to read 0.145.0 (or drop the version entirely — the contract description stands on its own). No behavioral impact, only documentation drift.
🟡 LOW costReceiptFromLlmError branch is unreachable via callLlm — src/analyst-model-call.ts
The comment claims costReceiptFromLlmError 'recovers the provider receipt when the response completed but violated the contract'. That recovery path only fires for LlmResponseError, but free-form callLlm never throws LlmResponseError (only LlmCallError on non-2xx and the raw JSON.parse SyntaxError on a non-JSON 200); LlmResponseError is thrown by callLlmJson, which this code does not use. So in practice costReceiptFromLlmError(err) always returns undefined here and the explicit costUnknown/usageUnknown fallback is what actually runs. Correct behavior, misleading comment — tighten the comment to say the fallback is the operative path given callLlm's error set.
🟡 LOW costReceiptFromLlmError is dead code in this path — src/analyst-model-call.ts
The comment claims costReceiptFromLlmError 'recovers the provider receipt when the response completed but violated the contract', but it only returns a receipt for
instanceof LlmResponseError(dist/llm-client), which is thrown exclusively by callLlmJson, never by callLlm — the only function used here. The?? {costUnknown:true, usageUnknown:true}fallback therefore always wins. The fallback is correct and explicit, so this is a redundancy/nit: the recovery branch and its comment describe behavior this file can never exercise.
🟡 LOW endpointFormat contract field is silently dropped — src/analyst-model-call.ts
ExternalOptimizerModelCallRequest carries endpointFormat?: 'chat-completions' | 'responses', but the callback never reads it and always dispatches callLlm, which is chat-completions-only. Not broken today: agent-eval's loopback proxy (external-optimizer-subprocess) normalizes both /v1/chat/completions and /v1/responses requests into the same canonical {model,messages,maxTokens,temperature,thinking} shape before invoking this callback, and encodeCanonicalModelResponse re-encodes the returned ChatResponse back into whichever path the child requested. The residual risk is a provider/model exposed only through the Responses API, which would then receive a chat-completions call and fail. Informational; no fix required unless the CLI starts routing Responses-only models, but worth a one-line com
🟡 LOW endpointFormat contract field is silently ignored — src/analyst-model-call.ts
The admitted-call request carries
endpointFormat?: 'chat-completions' | 'responses'(ExternalOptimizerModelCallRequest), and agent-eval's subprocess bridge accepts both /v1/chat/completions and /v1/responses (dist/external-optimizer-subprocess MODEL_PROXY_PATHS). This callback destructures only {request, callId, signal} and always routes through callLlm -> POST {baseUrl}/chat/completions. Today the dspy-rlm controller drives chat-completions, so this is inert; but if the engine ever admits a responses-format request, the call silently hits the wrong endpoint and returns a chat-completions envelope, without any error. Either honor endpointFormat or explicitly reject 'responses'.
🟡 LOW Per-call stderr log adds noise on every successful model call — src/cli.ts
recordExecution logs a line per model call to stderr including successes ('[analyst] model call 1 ok gpt-5-mini'). For analyze/improve --llm runs with many controller turns this is verbose, though it is intentional observability and matches the pre-existing '[analyst] FAIL' registry logging pattern. Not a correctness issue; consider gating success lines behind a verbose flag if stderr noise becomes a problem.
🟡 LOW buildAnalysisEngine wiring has no direct test coverage — src/cli.ts
The new engine options (call: createAnalystModelCall({apiKey,baseUrl}), callRef:
traces-cli:${baseUrl}#${model}, recordExecution log callback) are only verified transitively: analyst-model-call.test.ts tests the module in isolation (3/3 pass), but nothing asserts the wiring itself — callRef string format, that recordExecution emits the '[analyst] model call N ok/FAIL' line, or that budget/maxOutputTokens/runner still forward. A future agent-eval contract change or a typo in callRef/observation field access would only fail in real --llm runs. Suggest a unit test on buildAnalysisEngine (option forwarding + recordExecution log format) since the seam is now CLI-owned.
🟡 LOW callRef delimiter collides if OPENAI_BASE_URL contains '#' — src/cli.ts
callRef is built as
traces-cli:${baseUrl}#${model}. baseUrl comes from process.env.OPENAI_BASE_URL verbatim. If that URL already carries a fragment ('#...'), the model suffix is appended after a second '#' and the identity becomes ambiguous. callRef is only used as a stable log/provenance string (never parsed back), so impact is cosmetic. Fix: use a delimiter unlikely to appear in a URL, e.g.\u001f(unit separator) or::, or encodeURIComponent the model segment.
🟡 LOW recordExecution stderr line drops the provider reason and error class — src/cli.ts
analystLog is called with
{ error: observation.error }only, so the FAIL line prints[analyst] model call N FAIL <model> — <message>but omitserror_classand the LlmCallError status/body that analyst-model-call.ts otherwise preserves in the execution record. The comment on analystLog (cli.ts:1124-1126) explicitly wants the structured reason surfaced, and the observation.execution (which carries status/body) is not forwarded here, only the flattened string. Impact: an operator watching stderr sees a status-less failure and must dig into the persisted execution record for the 400/429/500 cause. Fix (optional): passerror_class(e.g. 'LlmCallError') or the execution's status into the fields object so analystLog's existing error_class branch renders it. Not a correctness bug — evidenc
🟡 LOW Abort path does not assert unknown cost/usage receipt — tests/analyst-model-call.test.ts
Test 2 (400) asserts result.receipt.costUnknown===true and usageUnknown===true, but test 3 (abort) does not. The source produces the same fallback receipt for an AbortError because costReceiptFromLlmError returns undefined for non-LlmResponseError (verified at llm-client-d0-2TT1g.js:297-299). An aborted call is the highest-risk case for a stale/incorrect charge being committed, so asserting these flags here would lock the contract symmetrically. Coverage gap, not a bug — source behavior is correct.
🟡 LOW Error-body truncation boundary not exercised — tests/analyst-model-call.test.ts
The mock 400 body is ~70 bytes, well under PROVIDER_ERROR_BODY_LIMIT (2000). The assertion String(execution.body).length <= PROVIDER_ERROR_BODY_LIMIT passes trivially without testing the slice at src/analyst-model-call.ts:79 (err.body.slice(0, PROVIDER_ERROR_BODY_LIMIT)). A body over the limit would prove the truncation actually fires rather than just that the constant is exported. Source comment notes the body is attacker-influenced, so the bound is the load-bearing property — worth stressing.
🟡 LOW No assertion that Authorization header reaches the provider — tests/analyst-model-call.test.ts
The test captures seenIdempotencyKey but not the Authorization header. createAnalystModelCall passes apiKey through to callLlm, which populates 'Authorization: Bearer test-key' (llm-client-d0-2TT1g.js header build). The loopback gateway already receives req.headers, so a one-line assertion on req.headers['authorization'] would close the auth-reachability gap at the seam. Minor — the credential plumbing is owned by callLlm, but this test is the natural place to witness it.
🟡 LOW Provider error body truncation is asserted but never actually exercised — tests/analyst-model-call.test.ts
The test asserts
expect(String(execution.body).length).toBeLessThanOrEqual(PROVIDER_ERROR_BODY_LIMIT)(line 88) against an 82-character error body, far under the 2000-char limit. Because the mock body is already tiny, this assertion passes trivially and would still pass even iferr.body.slice(0, PROVIDER_ERROR_BODY_LIMIT)were removed from src/analyst-model-call.ts:79. The stated purpose of the limit (bounding attacker-influenced, unbounded bodies held in memory by the proxy) is therefore never verified. Fix: have the non-2xx handler return an error body exceeding 2000 chars and assert the retained body is exactly 2000 (or <= limit AND shorter than the sent bod
🟡 LOW Provider-error-body truncation boundary is never exercised — tests/analyst-model-call.test.ts
The 400 handler sends a ~26-char body; the assertion
expect(String(execution.body).length).toBeLessThanOrEqual(PROVIDER_ERROR_BODY_LIMIT)(2000) is trivially satisfied, so the source truncationerr.body.slice(0, PROVIDER_ERROR_BODY_LIMIT)(src/analyst-model-call.ts:79) could be deleted and this test still passes. Send a body > 2000 chars (e.g. a 4000-char error message) and assert the retained body is exactly the 2000-char prefix. This path is security-relevant: the comment says the body is attacker-influenced and held in memory.
🟡 LOW Request endpoint path is never asserted — tests/analyst-model-call.test.ts
All three handlers ignore
req.url, so a regression in the endpoint construction (${baseUrl}/chat/completionsat llm-client:531-532) or a baseUrl/build mismatch would pass undetected. Add e.g.expect(req.url).toBe('/v1/chat/completions')inside a handler to pin the path the proxy actually receives.
🟡 LOW callArgs cast bypasses compile-time shape coupling — tests/analyst-model-call.test.ts
as unknown as Parameters<ReturnType<typeof createAnalystModelCall>>[0]suppresses type errors, so a source rename (e.g.request->req) or added required field would still compile and only surface at runtime. Derived-type construction is the right idea; the double cast defeats it. Prefer building the object and letting the derived type check it, or add asatisfies-style assertion. Tests still catch drift at runtime, hence low.
🟡 LOW callArgs double-cast silently defeats contract type-checking — tests/analyst-model-call.test.ts
callArgsreturns{ request, callId, signal } as unknown as Parameters<ReturnType<typeof createAnalystModelCall>>[0](line 38). Theas unknown aschain bypasses the compiler, so if the call contract field names or shape change in agent-eval, the test still compiles and either fails opaquely at runtime or stops testing the real shape. The seam deliberately does not export its contract type by name (see src/analyst-model-call.ts:10-15), which is the underlying cause, but the test can tighten this by deriving the type once and constructing a fully-typed object instead of casting. Minor robustness nit, not a current bug.
🟡 LOW costReceiptFromLlmError recovery branch is untested — tests/analyst-model-call.test.ts
Test 2 covers only the fallback arm (
costUnknown/usageUnknown: true). The source's other arm —costReceiptFromLlmError(err)recovering a completed provider receipt on an LlmResponseError (structured-response contract violation) — has no test. A malformed-usage response is the likely trigger; a case asserting the recovered receipt would pin that accounting path. Low because LlmResponseError is not reachable through plain callLlm without jsonSchemaTransport.
tangletools · 2026-08-13T02:21:56Z · trace
Superseded by re-review — no blocking findings on latest commit.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 26 non-blocking findings — d2582243
Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 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-13T02:21:56Z · immutable trace
callLlm aborts an internal controller to enforce its own per-attempt timeout, so a plain provider timeout rethrows a DOMException named AbortError while the caller signal stays clear. Reading the error name marked that timeout as a cancellation, which is the one state the bridge does not retry — the inverse of what the marker is for. Reproduced against agent-eval 0.145.3: a gateway that never answers gives err.name 'AbortError' with signal.aborted false. Also raise engines.node to 22.13.0, the floor agent-runtime 0.133.2 declares.
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — da25c8fa
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-13T02:25:17Z
|
| State | Detail |
|---|---|
| Interrupted | webhook restarted |
No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.
tangletools · #85 · model: kimi-for-coding · updated 2026-08-13T03:21:37Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — da25c8fa
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-13T03:27:13Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 240.8s (2 bridge agents) |
| Total | 240.8s |
💰 Value — sound
Forced agent-eval 0.145 migration that moves the analyst model call into the CLI via agent-eval's own callLlm, with three real failure-path correctness fixes (idempotency key, abort classification, provider-cause retention) and tests; ships as-is.
- What it does: Bumps @tangle-network/agent-eval 0.143.0->0.145.3 (+ agent-runtime 0.126.0->0.133.2, sandbox 0.17.2->0.21.1) and migrates across the 0.144.0 breaking change where createDspyRlmTraceEngine stopped accepting apiKey/baseUrl. The CLI now owns each model call: src/analyst-model-call.ts wraps agent-eval's own callLlm and returns the engine's {succeeded, response, receipt, execution} contract, derives th
- Goals it achieves: (1) Stay current with agent-eval and its peers without a tree-wide override that hides peer violations. (2) Own the model-call execution path the dependency now requires the caller to own. (3) Make the owned path correct under retry, cancellation, and provider error cases the previous engine-internal path handled opaquely. (4) Give the callback a reachable test seam — src/cli.ts runs main() at imp
- Assessment: Coherent and in the grain of the codebase. The migration is forced (the dependency removed a capability), and the response reuses agent-eval's own primitives (callLlm, costReceiptFromLlm, costReceiptFromLlmError, LlmCallError) rather than reinventing transport — the wrapper exists only to adapt callLlm's throw-based API into the engine's always-resolves callback contract. The type is derived from
- Better / existing approach: none — this is the right approach. I searched src/ for existing OpenAI-compatible callers that could be reused or extended. Two exist: zaiChatCaller (src/replay-fix.ts:42) and the callBridge/httpJsonTransport path (src/analyst-engine-prime.ts:511). Neither fits this seam: both return a simple {content, usage} outcome for a one-shot prompt, with no cost receipt, no ledger idempotency key, and no ex
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
Forced migration to agent-eval 0.145.x's caller-owned model-call contract, implemented with the library's own primitives and wired into the single --llm engine path; reaches live CLI commands and is exercised by tests that cover the tricky abort/timeout and idempotency cases.
- Integration: Reachable and singular. createAnalystModelCall is invoked once at src/cli.ts:505 inside buildAnalysisEngine, which is called from investigate() (cli.ts:1178) and cmdImprove() (cli.ts:1203) — the two CLI paths that carry --llm. The integration is forced: agent-eval 0.145.3's DspyRlmTraceEngineOptions (node_modules/.../index-BWDrSVfw.d.ts:160-206) requires
call/callRef/recordExecutionand no l - Fit with existing patterns: Follows the codebase's grain. It uses agent-eval's own callLlm/costReceiptFromLlm/costReceiptFromLlmError/LlmCallError/LlmCallRequest rather than reinventing an HTTP path, and derives the contract type from the factory (analyst-model-call.ts:15) specifically because the library does not export the contract by name — the right move against silent drift. The callRef/recordExecution wiring in cli.ts:
- Real-world viability: Holds past the happy path. Tests (tests/analyst-model-call.test.ts, all 4 passing under vitest threads pool) cover: success with the Idempotency-Key header asserted end-to-end through a real HTTP server; non-2xx with provider reason retained and byte-bounded; callLlm internal timeout NOT mislabeled as cancellation; and caller abort correctly marked. The timeout/abort distinction is the load-bearin
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
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 | 71 | 55 | 55 |
| Confidence | 85 | 85 | 85 | 85 |
| Correctness | 83 | 71 | 55 | 55 |
| Security | 83 | 71 | 55 | 55 |
| Testing | 83 | 71 | 55 | 55 |
| Architecture | 83 | 71 | 55 | 55 |
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 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Body-truncation guard is never exercised; the length assertion is tautological — tests/analyst-model-call.test.ts
The 400 handler serves a 63-char body ({error:{message:'model not found: test-model',type:'invalid_request_error'}}), so src/analyst-model-call.ts:85
err.body.slice(0, PROVIDER_ERROR_BODY_LIMIT)is a no-op andString(execution.body).length <= 2000can never fail — the test passes even if the slice is deleted. The guard exists precisely because the body is attacker-influenced and held in memory in the execution record, so the regression it protects is the untested one. Fix: serve a body > PROVIDER_ERROR_BODY_LIMIT and assert the retained body is truncated to exactly the limit (and that no suffix leaks).
🟡 LOW engines.node tightening is a consumer-facing floor bump — package.json
engines.node changed '>=22' -> '>=22.13.0'. This is justified and consistent — agent-runtime 0.133.2 (bumped in the same diff) declares engines {node: '>=22.13.0'}, so keeping the host package in sync avoids a broken install on Node 22.0-22.12 where the dependency would refuse/err. Impact is limited to users on those 13 patch releases of Node 22; a minor-version install/update now errors for them. Not a bug, but confirm the README/install.sh don't still advertise plain Node >=22 (documentation consistency is in a different shot's files).
🟡 LOW peer surface change without a version bump — package.json
agent-runtime 0.126.0 -> 0.133.2 drops the optional playwright peerDependency (visible in the pnpm-lock.yaml diff, old peerDependenciesMeta listed playwright optional; new does not) and the package version stays 0.11.5. A consumer relying on traces to transitively provide playwright through agent-runtime could see a resolution change on upgrade. The main/sdk export surface is unchanged, so this is likely benign, but a minor bump or a changelog note would make the peer-surface shift explicit. No evidence of an actual break.
🟡 LOW Node engine floor silently raised to 22.13.0 — pnpm-lock.yaml
agent-runtime 0.133.2 declares engines node >=22.13.0 (previously >=22 in package.json / agent-runtime 0.126.0 had no >=22.13.0 requirement surfaced in the old graph). package.json engines is bumped to >=22.13.0 in lockstep, so this is consistent. Impact: users on Node 22.0.0–22.12.x will fail install/runtime. This is intentional and aligned across package.json + lockfile, so it is informational, not blocking.
🟡 LOW Cancel-vs-timeout discriminator is a bare text prefix; timeout message contains 'aborted' — src/analyst-model-call.ts
The bridge distinguishes a cancelled call from a transient one by the 'AbortError: ' prefix on message. The per-attempt-timeout path (signal.aborted === false) yields the raw DOMException message 'This operation was aborted' (verified in callLlm's retry loop: controller.abort() on the attempt timeout, rethrown at llm-client-d0-2TT1g.js:745). That message contains the substring 'aborted'. The proxy's own abort classifier is exactly a loose substring test (isAbortError: message.toLowerCase().includes('abort'), external-optimizer-subprocess.js:1076-1077). If the consuming DSPy/litellm stack matches similarly loosely rather than on the exact prefix, a provider timeout is misclassified as a cancellation and the retry the comment promises ([line 72-76](https://github.com/tangle-network/traces/bl
🟡 LOW Failure-path execution evidence omits durationMs and uses a different model source than success — src/analyst-model-call.ts
Success execution records durationMs: response.durationMs and requestedModel: req.model (lines 59-66); failure execution records neither durationMs nor a servedModel, and reads requestedModel: request.model (line 103). The two request objects hold the same value today (req is structuredClone(request)), so this is cosmetic, but the asymmetric provenance record makes a retried/lost-call audit slightly harder and the two model reads could drift if the clone ever changes.
🟡 LOW Idempotency-Key reused across callLlm's temperature-1 retry with a mutated request body — src/analyst-model-call.ts
idempotencyKey: callId is attached to every callLlm attempt. callLlm builds the header set once (llm-client-d0-2TT1g.js:537) but, on a provider's temperature-1 rejection, mutates effectiveRequest.temperature and retries (llm-client-d0-2TT1g.js:608-615) — two different request bodies under one Idempotency-Key. Gateways that bind key to payload hash (OpenAI documents this) reject the fallback retry, turning a previously recoverable temperature-1 rejection into a hard, non-retryable failure. The traces callback cannot prevent the in-flight mutation; either callLlm should regenerate the key when the body changes, or the collision should be documented as a known edge.
🟡 LOW Provider error body may echo request headers into persisted evidence — src/analyst-model-call.ts
The LlmCallError body (attacker/gateway-influenced) is retained verbatim up to 2000 chars in execution provenance. The code deliberately keeps it out of the log line, but if a gateway reflects request headers into an error body, the Bearer credential would be persisted inside the first 2000 chars — the slice bound does not redact. The author acknowledges this risk in the comment at line 82, so it is an accepted residual, but it is not fully mitigated: the bound limits size, not the presence of a secret. Consider a header/credential redaction pass on the body before persisting, or dropping the body when it contains an Authorization-looking substring.
🟡 LOW Success path uses req.model, failure path uses request.model — cosmetic inconsistency — src/analyst-model-call.ts
On success,
requestedModel: req.model(the structuredClone). On failure,requestedModel: request.model(the original readonly request). Both always hold the same value since clone is a deep copy and model is a string. The failure path correctly cannot referencereq(declared inside try). Purely cosmetic — no behavioral impact.
🟡 LOW baseUrl persisted in opaque execution evidence — src/analyst-model-call.ts
The execution record includes
baseUrlon both success (line 57) and failure (line ~100) paths. Per the ExternalOptimizerModelExecutionObservation contract, the opaqueexecutionobject is persisted as provenance by the engine's recordExecution hook. baseUrl is not a secret (apiKey is correctly excluded), and for a CLI tool where the operator configures the endpoint this is acceptable. But if this provenance is ever exported or shared, the internal gateway URL is exposed. No fix required for this use case; flagging for awareness.
🟡 LOW baseUrl persisted verbatim into every execution record — src/analyst-model-call.ts
baseUrl is stored in both success and failure execution evidence (lines 58, 102). In cli.ts the value comes from OPENAI_BASE_URL (cli.ts:501), which a user can set with embedded userinfo or a query token (https://user:token@host/v1). Those credentials would then persist into the run's model-execution evidence, which the proxy retains (recordExecutionReceipt) and which flows into reports/upload. The apiKey is correctly never included; consider redacting any credential-shaped URL components from the stored baseUrl or documenting the expectation that baseUrl is credential-free.
🟡 LOW costReceiptFromLlmError branch is dead in this path — src/analyst-model-call.ts
costReceiptFromLlmError only returns a receipt for LlmResponseError (see agent-eval llm-client source:
return error instanceof LlmResponseError ? costReceiptFromLlm(error.result) : void 0). This path calls callLlm, which throws LlmCallError / DOMException(AbortError) / JSON parse errors — never LlmResponseError (that is thrown by callLlmStructured). So the?? fallbackat line 93 always fires and the recovered-receipt branch never does. Not a bug — the fallback is correct — but the comment at lines 90-92 ('recovers the provider receipt when the re
🟡 LOW Model-call execution evidence is not surfaced after a failed --llm run — src/cli.ts
recordExecution reduces every admitted model call to one stderr line ('[analyst] model call ok|FAIL '); the full observation (callId, servedModel, finishReason, durationMs, and for failures the LlmCallError status/body) is captured by createAnalystModelCall but never logged or written anywhere the operator can inspect. The engine retains these transiently in result.runtime.modelExecutions (verified in installed dspy-rlm-engine dist, line ~306), but the traces report path renders none of it, so a mid-run model failure after the fact leaves only the one-liner. Not a regression vs. the pre-change engine-internal logging, but the new wrapper deliberately builds richer evidence that this sink throws away. Suggest writing the observation (or at least the FAIL branch with status/body
🟡 LOW No repo test exercises the full engine->bridge->proxy->call handshake — src/cli.ts
tests/analyst-model-call.test.ts covers the call callback in isolation against a stub gateway, and tests/cli.test.ts:861-941 uses a bridge that dies at startup (DSPY-BRIDGE-FAILURE) so it never reaches a model call. Nothing in the repo drives the complete path createDspyRlmTraceEngine -> Python bridge -> loopback model proxy -> createAnalystModelCall -> provider and back, so a mismatch in the proxy<->callback contract (e.g. budget fields, request shape) would pass CI. The seam is agent-eval's own tested code and the callback is type-locked to the engine's parameter type (NonNullable<Parameters[0]['call']>), so this is a coverage gap rather than a defect.
🟡 LOW recordExecution drops the execution provenance the seam retained — src/cli.ts
The contract for recordExecution (agent-eval external-optimizer-contracts.d.ts) is 'Persist the finite execution record returned for every admitted call', and ExternalOptimizerModelExecutionObservation carries an
executionfield plus callId/path/responseStatus. src/analyst-model-call.ts:82-87 deliberately routes the provider's error status/body intoexecutionand NOT into the log line, because 'a gateway can echo request headers into an error body'. But buildAnalysisEngine's recordExecution (lines 507-512) only writes a one-line message (sequence/succeeded/model/error) and discards observation.execution entirely — so the provider error detail (status code + body snippet) has n
🟡 LOW Body-truncation assertion never exercises the PROVIDER_ERROR_BODY_LIMIT boundary — tests/analyst-model-call.test.ts
The test asserts
String(execution.body).length <= PROVIDER_ERROR_BODY_LIMITbut the error body is ~80 bytes, so the slice(0, 2000) in src/analyst-model-call.ts:85 never actually truncates. The invariant holds trivially. To prove the bound fires, feed a body > 2000 bytes and assert the retained length equals PROVIDER_ERROR_BODY_LIMIT exactly. Impact: a regression that removes the slice (e.g., passing the full body) would not be caught. Low severity because the assertion still guards against a future unbounded body.
🟡 LOW Error-body truncation bound is asserted but never exercised — tests/analyst-model-call.test.ts
The assertion
expect(String(execution.body).length).toBeLessThanOrEqual(PROVIDER_ERROR_BODY_LIMIT)uses a mock error body of ~86 chars (JSON.stringify of {error:{message:'model not found: test-model',...}}) against a 2000-char limit. The actual truncation (err.body.slice(0, PROVIDER_ERROR_BODY_LIMIT)in src/analyst-model-call.ts:85) is never hit, so a regression that drops the slice or raises the limit would still pass. This is a security-bounding behavior (the body is attacker-influenced and held in memory by the proxy), so it deserves a >2000-byte fixture asserting the body is exactly sliced to the limit.
🟡 LOW Idempotency-key reuse across retries is not covered — tests/analyst-model-call.test.ts
The mock gateway responds 200 on the first attempt, so callLlm never retries and the test only proves the header is sent once. The comment's stated purpose ('a retried-but-already-billed call is not charged twice') is never actually observed: no test makes the first attempt fail transiently and asserts the second attempt carries the same idempotency-key header. A regression that regenerated the key per attempt would pass the suite.
🟡 LOW Module-global server shared across tests with overwrite-on-start — tests/analyst-model-call.test.ts
startGatewayoverwrites the module-levelserverbefore the previous one is closed in afterEach, andrunning.close()waits on open sockets (no closeAllConnections/closeIdleConnections). Tests run sequentially today so it works (verified 4/4), but a test that failed before afterEach, or a handler that leaves a keep-alive socket open, silently leaks/strands the prior server and can stall close() up to keepAliveTimeout. Scoping the server per-test and calling closeAllConnections() in cleanup would make the harness robust to future additions.
🟡 LOW Timeout test asserts only the absence of a marker, not the surfaced error — tests/analyst-model-call.test.ts
expect(result.error.startsWith('AbortError:')).toBe(false)is a weak negative assertion. The meaningful claim (aborted=false) is covered, but nothing verifies the call actually timed out versus failing some other way — the test would also pass if callLlm surfaced, say, a network error with a non-AbortError message. The timeout itself is only proven implicitly (the test hangs at vitest's 5s limit if the internal timeout never fires). Asserting elapsed time (>= ~timeoutMs) or the specific AbortError-flavored message would make the distinction airtight.
🟡 LOW Timeout test duration (~2.25s) couples to callLlm retry defaults and TANGLE_LLM_MAXIMUM_ATTEMPTS env — tests/analyst-model-call.test.ts
callLlm classifies the internal-timeout AbortError as transient (/AbortError/i in TRANSIENT_ERROR_PATTERNS) and retries up to DEFAULT_MAXIMUM_ATTEMPTS (3). The test therefore runs 3 × 250ms timeouts + 500ms + 1s backoff ≈ 2250ms (confirmed: 2256ms measured). The seam createAnalystModelCall does not forward maximumAttempts, so the test cannot pin it. If CI exports TANGLE_LLM_MAXIMUM_ATTEMPTS > ~10, the test could approach the 30s suite timeout. The assertions are correct regardless of retry count (aborted stays false because only the caller signal is read), so this is efficiency/robustness, not correctness.
🟡 LOW Timeout-test runtime scales with TANGLE_LLM_MAXIMUM_ATTEMPTS env — tests/analyst-model-call.test.ts
The 250ms timeout test depends on callLlm's retry loop: with the default maximumAttempts=3 it runs ~2.25s (timeouts + 500/1000ms backoff), but agent-eval reads TANGLE_LLM_MAXIMUM_ATTEMPTS from the environment (default 3). If a developer or CI sets it high, the test slows linearly. It also couples to agent-eval internals (AbortError-as-timeout classification), which the source comment deliberately documents — acceptable, but worth pinning
maximumAttemptsvia the call opts or noting the env dependency.
🟡 LOW costReceiptFromLlmError recovery branch is untested — tests/analyst-model-call.test.ts
The 400 test only exercises the
?? {costUnknown:true,usageUnknown:true}fallback. The other receipt branch in the source — a response that completed but violated the schema contract (LlmResponseError, where costReceiptFromLlmError returns a real receipt) — has no test, so a completed-and-billed call that must NOT read as costUnknown/usageUnknown is not guarded. Also untested: success path with missing/partial usage (usageUnknown=true on a succeeded call).
tangletools · 2026-08-13T03:51:56Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 23 non-blocking findings — da25c8fa
Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 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-13T03:51:56Z · immutable trace
2 participants
What
@tangle-network/agent-eval0.143.0 → 0.145.3.@tangle-network/agent-runtime0.126.0 → 0.133.2 (peersagent-eval >=0.145.2 <0.146.0,agent-interface >=0.47.0 <0.48.0,sandbox >=0.21.1 <0.22.0) and@tangle-network/sandbox0.17.2 → 0.21.1.pnpm.overrides. An earlier revision of this PR pinned agent-eval tree-wide to get pastagent-runtime@0.126.0's<0.144.0peer cap. That masked the violation rather than resolving it, so the override is deleted and the consumers were raised instead.createDspyRlmTraceEngineno longer acceptsapiKey/baseUrl, so the CLI now owns each model call (call+callRef+recordExecution) using agent-eval's owncallLlm,costReceiptFromLlm, andcostReceiptFromLlmError. Failure receipts keep usage and cost explicitly unknown instead of zero.src/analyst-model-call.tswith tests.src/cli.tscallsmain()at import, so the callback previously had no reachable test seam.Why a code change was needed
The 0.145.x census law preserved every root-barrel symbol this repo imports, and it held: no import broke. The migration comes from the intermediate 0.144.0 release (caller-owned optimizer execution), which this repo skips over when jumping from 0.143.0.
Failure-path fixes in the owned model call
request.callIdis now the provider idempotency key.callLlmretries transient failures, so without it a response that was billed but lost is charged twice on the retry.AbortErrorand can never take its 504 branch — it always became a 502 the bridge retries against a dead intent. The failure text now names the class andexecution.abortedrecords it.LlmCallError.statusand a boundedbodystay in the execution evidence, and out of the log line because a gateway can echo request headers into an error body.structuredClonemoved inside thetry; a throw from it would have rejected the callback, which the contract says loses the execution record and fails the optimizer attempt.Checks
rm -rf node_modules && pnpm install): exit 0, zero peer warnings.agent-eval0.145.3,agent-interface0.47.0,agent-runtime0.133.2,agent-core0.6.1,agent-knowledge7.2.4,sandbox0.21.1,esbuild0.28.1.pnpm typecheckclean.pnpm test: 51 files, 657/657 passed.pnpm buildclean;pnpm check:packageok.