Skip to content

feat(analyst): trace analysts run official DSPy RLMs; retire the Ax stack - #495

Merged
drewstone merged 7 commits into
mainfrom
feat/analyst-accuracy-evolution
Jul 31, 2026
Merged

feat(analyst): trace analysts run official DSPy RLMs; retire the Ax stack#495
drewstone merged 7 commits into
mainfrom
feat/analyst-accuracy-evolution

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

What

Trace analysts are now recursive research programs: the official dspy.RLM runs in a sandboxed Deno/Pyodide child process with seven allowlisted trace tools, strict JSON I/O, and metered model calls through the external-optimizer proxy. callLlmJson survives only as the explicit one-shot direct benchmark baseline. The Ax stack (ax-service, ax-cost-service, trace-analyst/loop, AxGepaSteeringOptimizer, the @ax-llm/ax dependency) is deleted — audited zero external callers.

New engine boundary: src/analyst/engine.ts, dspy-rlm-engine.ts, trace-tool-callback.ts, benchmark-public-rlm.ts, and clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py (official dspy.RLM in a child process; 14 dedicated tests plus a live-Deno inspect path).

Why

The published CodeTraceBench numbers (13.47% all-row / 32.73% scored F1 with test artifacts; 15.02% / 30.99% trajectory-only) came from the one-shot direct runner. The recursive-RLM engine is the path to beating that; the semantics were pinned as "a trace analyst IS an RLM program", and the code now matches.

Audit hardening (two independent adversarial passes)

Security/correctness and API/DX audits ran against the full range. Fixed in this PR:

  • Fabricated-evidence hole (HIGH): excerpt verification recursed over every string field of a span, so an excerpt matching a span_id, status, name, or a timestamp digit verified. Excerpts now match span attributes/status_message (or a cited finding's content fields) only, with an 8-char minimum; six attack cases pinned as tests.
  • Unpinned sandbox runtime (HIGH): DSPy's runner.js imports npm:pyodide unversioned and --no-config means no lockfile — npm latest flipped 0.29.3314.0.3 during development, silently switching interpreters. An import map now pins pyodide@314.0.3 and the pin is recorded in runtime identity.
  • Hardcoded attestation (HIGH): _runtime_identity ignored the deno command and reported constants. The sandbox record is now derived from the command that ran (config, node-modules-dir, permission flags with scoped-path counts, never raw paths).
  • All-or-nothing finding parse (MED): one malformed row from the bridge discarded the whole paid investigation; rows now validate per row with logged rejections counted in runtime.rejectedFindings.
  • Bridge/schema contradiction (MED): the bridge's canonical fixture certified subject step:2, which the TypeScript finding-subject grammar rejects; fixture now emits incorrect-step-2.
  • Whitespace abort (MED): a trailing newline on answer/findings_json raised after all money was spent; model output is stripped before validation.
  • Red release gate (BLOCKER): verify-package-exports demanded the deleted buildTraceAnalystTools; it now negative-pins all removed exports. CHANGELOG documents the full breaking surface. The benchmark README states its numbers describe the retired runner and forbids citing them for the new engine without a fresh certified run.

What the security audit could NOT break: live sandbox escape attempts (host FS, network, env, subprocess — all blocked), tool-allowlist bypass (enforced on both sides, zero schema drift across all 7 tools), secret leakage into the child (allowlisted env, ephemeral proxy token only), ReDoS (RE2JS), subprocess leaks (process-group kill with escalation), and the store-backed citation check (base64/foreign ids rejected; net-new versus the pre-image).

Known deferred (audit findings not in this PR)

  • No real-seam integration test in CI: Python tests mock dspy, TS tests mock the bridge. The cheap token-free version (real tool specs through the real bridge validator + a real interpreter boot) is the highest-value follow-up.
  • Reasoning models can report output_tokens above the completion cap; the proxy's over-output rejection then kills the run (availability, not cost, is the exposure).
  • Tool-budget exhaustion surfaces as a bare HTTP 429 to the RLM (maxToolCalls not forwarded; response body dropped by raise_for_status), burning iterations.
  • --max-cost-usd feeds both the per-analysis and run-wide caps; per-analysis needs its own flag.
  • The implementation digest discovers TypeScript via the import graph but hardcodes the two Python files.
  • traced-analyst step spans are emitted post-hoc with ~0 duration (onTurn removal).
  • Missing-Python DX is a raw ModuleNotFoundError; the bridge's inspect preflight exists but is unwired from TypeScript.
  • Legacy "kind" vocabulary remains on constants (DEFAULT_TRACE_ANALYST_KINDS, *_KIND_SPEC, kind-factory.ts).

Verification

  • TypeScript: pnpm typecheck clean; pnpm test 4,525 passed / 0 failed / 3 skipped (339 files); pnpm build + node scripts/verify-package-exports.mjs green.
  • Python: 17/17 bridge tests; live inspect through real Deno booting pinned pyodide 314.0.3 with the derived attestation. (test_official_optimizer_compatibility/skillopt failures in a local venv reproduce identically at the merge base — environment extras, not this change.)
  • Benchmark digests: implementation 44633a52… (78 files), dependency lock b00ffeed… (4 files), checker green.

Not in this PR

A fresh certified benchmark run with the DSPy engine (paid) and the labeled comparison against the retired runner's numbers. Until that lands, no headline accuracy number may be cited for the new engine — the reference-result test and benchmark README both enforce the provenance split.

…tack

- clients/python dspy_rlm_bridge: official dspy.RLM in a child process,
  seven allowlisted trace tools, strict JSON I/O (14/14 tests)
- src/analyst engine contract (engine.ts, dspy-rlm-engine.ts,
  trace-tool-callback.ts); callLlmJson demoted to an explicit
  non-recursive baseline
- store-backed citation verification: cited trace/span ids must resolve
  in the trace store, encoded ids are rejected
- external-optimizer model proxy fails on zero-usage 200 responses and
  on over-reserved output after recording the actual charge
- delete ax-service, ax-cost-service, trace-analyst/loop,
  structure-findings, AxGepaSteeringOptimizer (no external callers)
- repin analyst benchmark implementation and dependency-lock digests
  for the changed fingerprint set
…produced it

The CodeTraceBench artifact records the implementation that ran it; binding
those fields to the live ANALYST_BENCHMARK_* constants asserted that the
current implementation produced a run it did not. The published digests are
now literal historical facts, and a fresh certified run is required before
any headline claim about the current engine.
…s removed surface

- verify-package-exports no longer requires the deleted buildTraceAnalystTools
  and pins every removed analyst, steering, and traces export with the
  script's negative-assertion convention
- CHANGELOG documents the breaking analyst-engine surface: required engine,
  defineTraceAnalyst returning TraceAnalystDefinition without a cost field,
  createTraceAnalyst rename, instructions rename, pairwise-only steering,
  the Ax stack removal, and the Python dspy extra
- the published CodeTraceBench README states its numbers came from the
  retired one-shot runner at implementation 4dba263b and forbids citing
  them for the current engine without a fresh certified run
… attested sandbox

Closes the adversarial-audit findings on the DSPy engine boundary:

- excerpt citations verify against span attributes and status_message (or a
  cited finding's content fields) only — identifiers, names, statuses, and
  timestamps are no longer quotable evidence — and excerpts under 8 chars
  are rejected as unverifiable
- one malformed finding row from the bridge no longer discards the whole
  paid investigation: rows are validated per row, rejections are logged and
  counted in runtime.rejectedFindings
- the bridge's canonical fixture emits a grammar-valid subject
  (incorrect-step-2); step:2 was certified Python-side but rejected by the
  TypeScript finding-subject schema
- the Deno/Pyodide sandbox pins pyodide via an import map (npm latest
  flipped 0.29.3 to 314.0.3 during development, silently switching
  interpreters on a cold cache) and the pin is recorded in runtime identity
- the sandbox attestation is derived from the deno command that actually
  ran (config, node-modules-dir, permission flags with scoped-path counts)
  instead of hardcoded constants
- model output answer and findings_json are stripped before validation so
  a trailing newline cannot abort a fully-paid run
- the analyst benchmark implementation digest is repinned for these changes

TS: typecheck clean, 4525 passed / 3 skipped. Python: 17/17 bridge tests
plus a live inspect through real Deno booting pinned pyodide 314.0.3.
tangletools
tangletools previously approved these changes Jul 30, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — ef36ad97

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-07-30T22:48:04Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 71bca06f

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-07-30T23:07:11Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 71bca06f

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-07-30T23:08:03Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 130.8s (2 bridge agents)
Total 130.8s

💰 Value — sound

Replaces the bespoke Ax agent loop with the official DSPy RLM in a sandboxed child process behind a clean pluggable engine seam; reuses existing cost-metered subprocess infra, deletes a fully-dead stack with zero remaining callers.

  • What it does: Trace analysts are now run by the official dspy.RLM (Recursive Language Model) executing in a sandboxed Deno/Pyodide child process. A new TraceAnalysisEngine interface (src/analyst/engine.ts:60) decouples the analyst definition (research question + evidence rules, no model/credentials) from the execution engine. createDspyRlmTraceEngine (src/analyst/dspy-rlm-engine.ts:39) composes the pre-existing
  • Goals it achieves: (1) Make 'a trace analyst IS an RLM program' literally true in code — the recursive engine is the official upstream library, not a hand-rolled Ax agent loop. (2) Beat the published one-shot direct baseline (32.73% scored F1) with a recursive approach that retrieves evidence on demand. (3) Introduce a stable engine boundary so the analyst definition and finding contract don't change when swapping D
  • Assessment: Sound and well-grained. The TraceAnalysisEngine seam is clean: defineTraceAnalyst now returns a pure definition (src/analyst/define.ts) with no model, credentials, or execution state; you bind it to any engine via runTraceAnalyst (src/analyst/kind-factory.ts:48). The DSPy engine reuses existing infrastructure rather than reinventing — startExternalOptimizerModelProxy and runExternalOptimizerProces
  • Better / existing approach: none — this is the right approach. I searched for an existing recursive-loop primitive in agent-eval that this could reuse: TraceAnalysisEngine is referenced only in the new files, and there is no other RLM/PythonInterpreter/recursive-agent-loop in the substrate (grep across src/*.ts). The only prior recursive loop was the deleted Ax-based trace-analyst/loop.ts, which this replaces. agent-eval is
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Replaces a zero-caller custom Ax stack with the official DSPy RLM behind a clean, minimal engine interface that reuses the codebase's existing subprocess/cost-ledger/trace-store machinery and is wired as the default benchmark runner.

  • Integration: Fully reachable. createDspyRlmTraceEngine is exported from both barrels (src/index.ts:70, src/analyst/index.ts:191), is the default analyst runner in the benchmark command (benchmark-command.ts:186 via createPublicBenchmarkRlmRunner), and plugs into analyzeTraces (trace-analyst/analyst.ts:28 options.engine) and buildDefaultAnalystRegistry (default-registry.ts:26). The deleted Ax stack leaves
  • Fit with existing patterns: Fits the codebase grain precisely. The new TraceAnalysisEngine interface (engine.ts:60) is a 5-field abstraction that decouples the research engine from the analyst definition — defineTraceAnalyst (define.ts:10) returns a pure value with no model/credentials, bound to any engine at run time. This is consolidation, not divergence: it retires the parallel Ax approach and routes through the same
  • Real-world viability: Holds up off the happy path. The engine starts an authenticated loopback trace-tool callback (trace-tool-callback.ts) and a cost-metered model proxy, spawns the Python bridge with a sandbox startup probe (print(1+1) must return '2' before any paid call), reconciles bridge-reported model-call counts against proxy-recorded completions (dspy-rlm-engine.ts:138), and tolerates per-row finding noise w
  • 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.

value-audit · 20260730T230938Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 235.9s (2 bridge agents)
Total 235.9s

💰 Value — sound-with-nits

Replaces the hand-rolled Ax agent loop with a clean TraceAnalysisEngine abstraction backed by the official DSPy RLM in a sandboxed child process — a coherent migration along the codebase's established bridge grain; only minor helper duplication remains.

  • What it does: Introduces a TraceAnalysisEngine interface (src/analyst/engine.ts) whose production implementation (createDspyRlmTraceEngine, src/analyst/dspy-rlm-engine.ts) runs the official dspy.RLM in a Deno/Pyodide child process via a Python bridge (clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py), with seven allowlisted trace tools exposed over an authenticated loopback socket (src/analyst/trace-tool-ca
  • Goals it achieves: (1) Pin the semantics so 'a trace analyst IS an RLM program' uses the upstream DSPy RLM rather than a hand-maintained Ax agent loop, on the path to beating the published 13.47%/32.73% CodeTraceBench direct-baseline numbers. (2) Remove the @ax-llm/ax dependency, which carried its own agent runtime and fought agent-eval's zero-upward-dependency substrate doctrine. (3) Establish a clean engine bounda
  • Assessment: Sound migration executed in the codebase's grain. The new TraceAnalysisEngine interface is the right decoupling layer — analyst kinds and finding contracts are now engine-agnostic (kind-factory.ts:85 calls args.engine.analyze with no engine-specific assumptions). The DSPy RLM bridge reuses the exact established subprocess contract (runExternalOptimizerProcess + module string, identical to gepa_bri
  • Better / existing approach: none — this is the right approach. I searched for duplication and a materially better architecture: (1) The Python bridge reuses optimizer_bridge_common.atomic_write_json and the same --input/--output contract as gepa_bridge.py/skillopt_bridge.py — no reinvention. (2) There is no pre-existing recursive-engine abstraction this duplicates; the old loop.ts was Ax-specific and is correctly superseded
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

The DSPy RLM engine is the default on the main CLI benchmark path and the documented programmatic entry, cleanly replacing the heavier Ax stack with the official dspy.RLM behind a thin swappable interface.

  • Integration: Fully reachable on three live paths: (1) the CLI analyst-benchmark command defaults to createPublicBenchmarkRlmRunner which builds createDspyRlmTraceEngine (benchmark-command.ts:186, benchmark-public-rlm.ts:73); (2) analyzeTraces takes the engine directly (trace-analyst/analyst.ts:52,81) and is the documented quickstart (docs/trace-analysis.md:37-55); (3) buildDefaultAnalystRegistry + createTraceA
  • Fit with existing patterns: Fits the codebase grain precisely. The TraceAnalysisEngine interface (engine.ts:60) separates the research definition (question/instructions/tools/limits) from the runner, explicitly allowing DSPy RLM, HALO, or caller-owned implementations — a substrate primitive per the repo's layering doctrine. It consolidates a competing pattern (the custom AxGepaSteeringOptimizer + @ax-llm/ax dependency) into
  • Real-world viability: Built beyond the happy path. Cost is bounded (maxCostUsd default $1, dspy-rlm-engine.ts:49), model calls are metered through a proxy that cross-checks reported vs recorded completions and throws on mismatch (dspy-rlm-engine.ts:138), tool calls are capped by maxToolCalls via an authenticated loopback-only callback (trace-tool-callback.ts:17), AbortSignal propagates through every layer, and cleanup
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Small benchmark helpers duplicated between the two runners [duplication] ``

trajectoryIdFromCaseId (benchmark-public-rlm.ts:221 and benchmark-public-model.ts:641) and isPaidCallControlError (benchmark-public-rlm.ts:229 and benchmark-public-model.ts:358) are byte-identical across the RLM and direct runners; pricingForModel is also near-identical. These could be extracted to a shared benchmark-public-adapters or benchmark-public-utils module. This is a pre-existing pattern (the direct runner always carried its own copies), newly multiplied by adding the RLM runner — not b


What this audit checks

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

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

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

value-audit · 20260730T231212Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 71bca06f

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 23 findings (6 medium, 17 low)

glm: Correctness 0 · Security 0 · Testing 0 · Architecture 0

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

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

🟠 MEDIUM Bearer token and model API key may transit plaintext HTTP — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_require_http_url accepts scheme in {http, https} for both modelProxy.baseUrl (line 479) and toolCallback.url (line 486). The callback Bearer token (line 437) and the model apiKey (passed to dspy.LM) are then sent over the wire. If the Node orchestrator ever points either URL at a non-loopback host over plain http, the secret is exposed on th

🟠 MEDIUM Pyodide pin is entry-module only; transitive npm deps stay unpinned under --no-config — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_PYODIDE_IMPORT_MAP pins only npm:pyodide/pyodide.js to 314.0.3. The Deno command runs with --no-config (line 351) and --node-modules-dir=none, so there is no lockfile; every transitive npm range resolves to whatever the registry serves at cache-populate time. The code comment (lines 322-326) acknowledges this residual surface honestly. Impact: eval reproducibility — two cold caches on different days can run different pyodide dependency closures while reporting the same pinned PYODIDE

🟠 MEDIUM --max-cost-usd reused as per-analysis budget; one case can exhaust the run — src/analyst/benchmark-command.ts

Line 441 computes maxCostUsd = positiveFiniteFlag(flags, 'max-cost-usd', 5). Line 457 assigns maxCostUsdPerAnalysis: maxCostUsd while line 464 sets the run-wide maxCostUsd to the same value. The cost ledger is shared across all cases (line 162-166, `costCeiling

🟠 MEDIUM Evidence gate silently rejects all non-trace-span, non-finding URIs — src/analyst/kind-factory.ts

evidenceIsResolvable (lines 215-286) only admits trace:///span/ URIs (checked via store.hasSpans) and finding:// URIs (checked against the prior/upstream findings map). Every other URI scheme — including the previously supported span://, event://, metric:// — falls through to rejectEvidence(context, citation.uri, 'citation is not a supplied finding or trace span') and returns false, dropping the ENTIRE finding (acceptFindings line 209 continues). evidenceKindFromUri in finding-signature.ts ([line 106-110](https://github

🟠 MEDIUM New billing-integrity and spawn-safety logic has no test coverage — src/campaign/external-optimizer-model-proxy.ts

grep for startExternalOptimizerModelProxy|forwardModelProxyRequest|runExternalOptimizerProcess|resolveRunnerCommand across all *.test.ts returns zero matches. The four new behaviors added in this PR — zero-usage detection (lines 338-345), output-token cap enforcement (line 348), the channel option (line 215), and resolveRunnerCommand in external-optimizer

🟠 MEDIUM Post-hoc trajectory spans distort timing and lose step detail — src/traced-analyst.ts

Step spans are emitted in a serial await-loop AFTER await analyzeTraces(input, options) returns (line 34), iterating result.trajectory. Two consequences: (a) every step span's startedAt/endedAt clusters after the real work finishes, so the trace timeline no longer shows steps interleaved with execution — the parent span's measured duration is inflated by N×span-creation I/O for a trajectory that can reach maxIterations(12)×maxToolCalls(48) entries; (b) each step span records only analyst.step (index) and analyst.step_bytes, discarding the step content entirely. The old per-turn spans recorded stage/turn/is_error/code_length/output_length. Given trajectory is typed `

🟡 LOW README 'keeps provider credentials in Node' is imprecise vs bridge behavior — clients/python/README.md

The new README section states the TS API 'keeps provider credentials in Node'. The bridge in fact receives and uses modelProxy.apiKey (bridge line 470) and toolCallback.token (bridge line 437) in-process for the run's duration, reading them from the JSON input file. The intended meaning (Node is the credential authority and issues scoped credentials) is reasonable, but the literal claim could mislead a reader auditing the Python process. Suggest rewording to 'Node issues scoped, run-limited credentials to the bridge' to match the code.

🟡 LOW deno==2.7.14 extra forces ~50MB platform wheels even when Deno is on PATH — clients/python/pyproject.toml

Adding deno==2.7.14 as a hard member of the [dspy] extra pulls ~50MB-per-platform binary wheels for every installer of the extra. _find_deno_executable (bridge line 359) already prefers the interpreter-adjacent binary and PATH before falling back to the packaged deno package, so the install is only a last-resort resolver. For users who already operate Deno, the pin is pure install bloat. Consider documenting the PATH option or making deno a recommended (not required) extra member while keeping the pin for reproducibility.

🟡 LOW Empty toolSpecs accepted, allowing an evidence-less RLM run — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

toolSpecs is only checked to be a list (line 499); an empty list passes, _build_dspy_tools returns [], and the RLM runs with no tools. The prompt requires evidence URIs from 'enabled tools', so the model cannot produce citable findings and must return [] — self-limiting, but the run still bills model calls for an analytically useless invocation. Not a correctness bug; consider rejecting empty toolSpecs at validation time to fail fast.

🟡 LOW Validator has ~20 rejection branches but most are untested directly — clients/python/tests/test_dspy_rlm_bridge.py

_validate_analyze_input (bridge line 454) enforces exact-key sets for 4 objects, http-url + no-credentials, positive-integer limits, unique tool names, known tool names, and exact properties/required sets per tool — roughly 20 distinct failure modes. The test file covers the all-canonical-schemas happy path (test_analyze_accepts_all_canonical_node_tool_schemas) and two indirect findings-malformation cases, but has NO direct negative tests for: non-'analyze' operation, missing/extra top-level keys, whitespace apiKey/model, zero/negative limits, duplicate tool name, unknown tool name, properties/required set mismatch, or url-with-credentials. A regre

🟡 LOW Breaking public API renames without deprecation aliases — src/analyst/index.ts

The index.ts barrel removes createPublicBenchmarkModelRunner (renamed to createPublicBenchmarkDirectRunner), createAnalystAi / CreateAnalystAiConfig, createTraceAnalystKind / CreateTraceAnalystKindOpts / TraceAnalystKindSpec, DefineTraceAnalystOptions / TraceAnalystAnalyze, structureFindings / StructureFindingsOptions / StructureFindingsResult, and AnalystBenchmarkCommandDependencies.createModelRunner (renamed to createAnalystRunner). These are breaking changes with no deprecation aliases. External consumers (agent-runtime, agent-knowledge) will fail at compile time. This is consistent with the PR's intent (retiring Ax), but consumers must be updated atomically. Verify that all dependent packages are updated in the same PR or coordinated release.

🟡 LOW Excerpt verification bypassed when the model omits the excerpt field — src/analyst/kind-factory.ts

Lines 245-258: the excerpt content check (containsExactText) only runs if (citation.excerpt !== undefined). If the model omits excerpt, only span existence is verified (store.hasSpans). A model can cite a trace span it never actually read — as long as the span_id is correct, the finding passes. For localization benchmarks this is acceptable (citing the correct span IS the task), but the 'verified excerpts' guarantee from commit ef36ad9 is partial: it verifies content only when the model volunteers an excerpt. The schema makes excerpt optional (finding-signature.ts [line 18](https://github.com/tangle-network/agent-eval/blob/71bca06f5c7d07862d43712be169142f

🟡 LOW Strict exact-substring excerpt matching may reject valid near-quotes — src/analyst/kind-factory.ts

Line 254 uses containsExactText([span.attributes, span.status_message], citation.excerpt) which calls value.includes(expected) (line 306). Models frequently paraphrase, truncate with ellipsis, or normalize whitespace when quoting. Any deviation from the exact stored string causes the finding to be rejected with 'excerpt is not present in the cited span content'. This systematically biases the benchmark against weaker models that quote imprecisely, even when their localization is correct. The MINIMUM_EXCERPT_LENGTH of 8 ([line 300](https

🟡 LOW Improvement-kind 'maximum budget' assertion is now a tie, not a maximum — src/analyst/kinds/kinds.test.ts

The test asserts IMPROVEMENT_KIND_SPEC.limits?.maxLlmCalls equals the max across all default kinds. After the refactor improvement=8, but failure-mode=8 and knowledge-poisoning=8 as well (knowledge-gap=5). So improvement is tied with two other kinds, not uniquely maximal. The test still passes but no longer proves the intended invariant that improvement has the strict top budget for competing fixes. Either raise improvement's maxLlmCalls above the others or tighten the assertion (e.g. strict greater-than the non-improvement kinds).

🟡 LOW New trace-tool-callback test skips request-size cap and close-during-active paths — src/analyst/trace-tool-callback.test.ts

The new test file covers auth (401) and the call-limit (429-before-execute) well, but does not exercise MAX_REQUEST_BYTES (>1MB -> reject), MAX_RESPONSE_BYTES (>4MB -> 413), unknown-tool (404), the AbortSignal-driven close, or close-while-handler-running. The implementation has non-trivial logic in readJson size accounting (trace-tool-callback.ts:151-157) and closeCallback active-handler draining (lines 124-138) that is untested. Add at least one oversized-body and one abort-during-close case.

🟡 LOW Output-token cap check may false-positive on reasoning models — src/campaign/external-optimizer-model-proxy.ts

usage.outputTokens is the raw provider-reported output_tokens/completion_tokens, which for some providers already includes reasoning tokens. parseProviderUsage (line 487) returns outputTokens as the raw value without subtracting reasoningTokens. If an optimizer uses a reasoning model where the provider reports combined completion+reasoning in output_tokens, a response at or near the requested cap could trip usage.outputTokens > args.maxOutputTokens and return a spurious 502 to the optimizer, failing the run even though the provider behaved correctly per its own accounting. Consider comparing against the non-reasoning portion, or documenting tha

🟡 LOW resolveRunnerCommand silently trusts process.cwd() for relative runner paths — src/campaign/external-optimizer-subprocess.ts

A relative runner command (e.g. './python', 'venv/bin/python') now resolves via path.resolve() against process.cwd() rather than the tempdir cwd passed to spawn(). This is the correct fix for the original bug (spawn resolved relative to tempdir, which only ever contained input.json), but the resolution target is implicit. If the host process's cwd changes between configuration and execution, the resolved binary changes silently. The runner command is user-controlled config (not a security boundary), so this is a robustness nit, not a vuln. An absolute path in config avoids the ambiguity entirely.

🟡 LOW SteeringOptimizationSelector retains fields only the removed backend populated — src/steering-optimizer.ts

The interface still exposes optional signature?, labels?, rationale? and SteeringOptimizationResult still exposes selector?, but the only remaining backend ('pairwise') never sets selector (PairwiseSteeringOptimizer.optimize returns no selector field). With SteeringOptimizerBackend narrowed to 'pairwise', no current producer can populate these fields. This is intentional forward-compat per the class docstring ('Model-backed candidate generation belongs to the official GEPA campaign integration'), so non-blocking — but if that integration lands via a different module, these fields risk becoming permanently dead surface. Consider dropping selector? from SteeringOptimizationResult (and the now-unused SteeringOptimizationSelector interface) now and re-adding when a real producer arrives, to ke

🟡 LOW analyzeTraces wrapper has no direct unit test — src/trace-analyst/analyst.ts

The public entry point's own logic is untested at this layer: the non-empty-question TypeError (line 56-58), the id/description/area fallback chain (lines 59,72-74), and the ensureIndexed-before-engine contract for OtlpFileTraceStore (line 64-66) have no test. The underlying runTraceAnalyst is well-tested in src/analyst/analyst.test.ts, and the wrapper is thin, so risk is low — but a regression that silently

🟡 LOW Prompt loses detailed byte-budget, result-shape, and truncation-recovery guidance — src/trace-analyst/prompts.ts

The v6 prompt prescribed a concrete DISCOVERY→NARROW→DEEP-READ protocol with: the 150_000-byte small/large threshold, the result-shape contract ('iterate result.hits NOT result.matches', 'retry only omitted_span_ids'), oversized-response handling ('if viewTrace returns oversized, DO NOT retry — switch to searchTrace'), and the 4KB→16KB truncation ladder. The new v1 prompt keeps ordering and anti-fabrication rules but drops all of this. Tool descriptions (tools.ts:78-132) partially compensate (oversized→searchTrace, has_more→refine, retry omitted ids) but the result-field-name contract and the explicit byte threshold are gone. Risk: degraded analysis quality on large/oversized traces where the model misparses result shapes or over-fetches. Version was bumped v6→research-v1 so run records st

🟡 LOW tools.test.ts drops getDatasetOverview filter-schema assertion and dual-surface parity — src/trace-analyst/tools.test.ts

The old suite asserted the full getDatasetOverview filters JSON schema (additionalProperties:false, has_errors, service_names, model_names, tool_names, time bounds, regex_pattern minLength:1) and that the Ax adapter and neutral descriptor rejected identically. Both are removed. Runtime parseTraceInput still enforces unknown-filter rejection (covered by the 'rejects an invalid filter' case at line 138), but published-schema drift — e.g. accidentally dropping additionalProperties:false or a filter field — would no longer fail a test. The Ax-adapter parity tests are correctly removed (the adapter no longer exists). Re-add one assertion on byName.get('getDatasetOve

🟡 LOW JSON.stringify(step) allocated only to measure byte length — src/traced-analyst.ts

const encoded = JSON.stringify(step) (line 36) is computed solely to feed Buffer.byteLength(encoded) (line 43); encoded is never otherwise used. For large opaque step objects in a long trajectory this allocates a full JSON string per step purely for a size metric. Defensible if exact UTF-8 byte count is the desired signal (vs .length), but if step_bytes is only a rough size hint, a cheaper proxy avoids the per-step allocation in an already-serial post-hoc loop.

🟡 LOW No test coverage for the traced wrapper — src/traced-analyst.ts

git ls-files shows no traced-analyst.test.ts. The wrapper owns real behavior — parent span creation, per-step span emission, success-attribute aggregation (modelCalls/toolCalls/step_count/finding_count/answer_length), and the error path (parentSpan.fail + rethrow). This PR is a full API migration (onTurn removed, trajectory/modelCalls/toolCalls added); a regression test using a stub emitter + stub engine asserting span names, parentage, and the catch-path fail() would lock the contract. Repo uses vitest; a single test file would cover it.


tangletools · 2026-07-30T23:31:37Z · trace

tangletools
tangletools previously approved these changes Jul 30, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 23 non-blocking findings — 71bca06f

Full multi-shot audit completed 8/8 planned shots over 67 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-07-30T23:31:37Z · immutable trace

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 71bca06f

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

glm: Correctness 11 · Security 11 · Testing 11 · Architecture 11

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

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

🟠 MEDIUM Bridge fails the entire paid analysis on any malformed finding, contradicting the TS engine's tolerance design — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_parse_findings_json raises ValueError on the first invalid finding row, which propagates through _analyze → main(), killing the subprocess with no output file written. All model calls are already paid for at this point (LM history is populated, program() has returned). The TS engine that consumes this output (src/analyst/dspy-rlm-engine.ts:189-191) explicitly states: 'Findings are model output: one malformed row is model noise, not a bridge fault, and the rest of the paid investigation must survive it' — it uses safeParse per row, increments rejectedFindings, and continues. The Python bridge defeats this design: a single model hiccup (extra field, wrong type) destroys the entire investigation. This is costly for an eval system where each analysis consumes multiple LLM calls. Fix: collect

🟠 MEDIUM Deno binary version attestation can silently diverge from the pinned wheel — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_find_deno_executable resolves in order: (1) beside Python interpreter, (2) shutil.which('deno') — PATH, (3) packaged find_deno_bin(). The deno PyPI wheel does NOT install a bin/deno script (binary lives inside the package data dir), so step 1 typically fails in a venv. Step 2 then picks up any system deno on PATH before reaching the pinned wheel at step 3. Meanwhile _runtime_identity line 615 records 'deno': _package_version('deno') which always returns the wheel metadata version ('2.7.14') regardless of which binary actually ran. The code's own comment ([line 606-609](https://github.com/tangle-network/agent-eval/blob/71bca06f5c7d07862d437

🟠 MEDIUM Post-success span emission can discard a valid analysis result — src/traced-analyst.ts

The trajectory-emission loop (lines 35-48) runs inside the same try block as analyzeTraces (line 34). If JSON.stringify(step) throws on a non-serializable step (trajectory is typed readonly unknown[] in engine.ts:42 — BigInt or circular references are unguarded and produce a TypeError) or a trace-store appendSpan/updateSpan write fails (disk full, locked store), control jumps to the catch at 61-63, which calls parentSpan.fail() and re-throws. The caller therefore never receives the already-computed, successful result from a potentially long inves

🟡 LOW Bridge applies stricter string validation than the TS schema, creating asymmetric false rejections — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_require_non_empty_string rejects any string where value != value.strip() (leading/trailing whitespace). This is applied to model-generated fields via _require_bounded_string: finding.claim (line 560), finding.subject (line 562), evidence.uri (line 591). The TS schema (src/analyst/finding-signature.ts:17) uses z.string().trim().min(1).max(200

🟡 LOW Evidence URI scheme not validated — hallucinated URIs pass both layers — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

The prompt (line 62-64) requires evidence URIs to use trace:///span/ or reference IDs returned by enabled tools. _validate_finding only checks the URI is a bounded non-empty trimmed string (line 591-595). The TS schema (finding-signature.ts:17) is equally lenient — z.string().trim().min(1).max(2000). Neither layer enforces the trace:// or finding:// scheme. A model can hallucinate 'https://example.com/fake' or 'span://nonexistent' and the finding pa

🟡 LOW No test coverage for tool-callback error paths or Deno-executable fallback chain — clients/python/tests/test_dspy_rlm_bridge.py

The test suite covers happy paths well but has gaps: (1) _call_trace_tool (line 432) has no tests for HTTP error responses (4xx/5xx via raise_for_status), invalid JSON bodies, malformed envelopes (extra keys, missing 'result'), or timeout scenarios — all are untested failure modes for a network-dependent function. (2) _find_deno_executable (line 366) fallback chain is untested beyond step 1 (beside_python): shutil.which PATH preference and the packaged find_deno_bin() fallback have no coverage, which is esp

🟡 LOW Hardcoded runnerIds in benchmark persistence rejects custom-injected runners on resume — src/analyst/benchmark-command-persistence.ts

readProgress validates observation.runnerId against a hardcoded check: observation.runnerId !== 'empty' && observation.runnerId !== 'dspy-rlm'. The benchmark command supports dependency injection via AnalystBenchmarkCommandDependencies.createAnalystRunner, but any custom runner returning a different id would have its observations rejected on --resume. The runnerIds list in createRunIdentity (line 159) is also hardcoded to ['empty', 'dspy-rlm']. Only affects test-time injection; the default flow always uses dspy-rlm.

🟡 LOW redactSensitiveText omitted-char count is imprecise — src/analyst/benchmark-public-errors.test.ts

In benchmark-public-errors.ts:102, omitted = redacted.length - 460 does not account for the marker string's own length in the final concatenation. For an 800-char input, the marker claims '340 chars omitted' but the actual gap is ~326 chars. The output is still capped at exactly 500 chars (verified: head_len + marker_len + tail_len = 500) and the test only checks .toContain('chars omitted') without asserting the exact count. Cosmetic inaccuracy only.

🟡 LOW Published reference result is stale relative to current implementation — src/analyst/benchmark-reference-result.test.ts

The published result.json still has runnerId 'model' (66 occurrences) and implementationSha256 4dba263b..., while the live constants are now 0c4928e0... with runnerIds ['empty','dspy-rlm']. The test correctly decouples these by hardcoding the historical digests (verified: values match result.json exactly). Impact: new runs produce dspy-rlm artifacts that cannot be directly compared against the reference. This is intentional per the test comment but no PR note indicates when a fresh certified run will replace it. Not a blocker — flag for follow-up.

🟡 LOW maxRequests budget formula is generous relative to expected LLM call count — src/analyst/dspy-rlm-engine.ts

The model proxy request budget is maxIterations + maxLlmCalls + 1. For failure-mode (maxIterations=24, maxLlmCalls=8), this allows 33 provider requests though only 8 LLM calls are expected. Each request is individually cost-gated by the reservation check, so no overspend occurs — but a runaway bridge could make 33 provider calls (each up to maxCostUsd/33 marginal cost) before hitting the request limit instead of being capped at maxLlmCalls. The integrity check at line 138 (modelCalls === successfulCompletions) would still catch a discrepancy post-hoc. Consider using min(maxIterations, maxLlmCalls) + 1 as a tighter ceiling, or document why the sum is intentional.

🟡 LOW trace-tool-callback test lacks concurrent call-limit coverage — src/analyst/trace-tool-callback.test.ts

The test verifies sequential enforcement of maxCalls (call 1 returns 200, call 2 returns 429) but does not test concurrent in-flight requests. The implementation checks calls >= maxCalls then awaits readJson before incrementing, so two simultaneous requests could both pass the limit check before either increments (TOCTOU). The test passes because it awaits each call serially. This is a coverage gap, not a test bug — the implementation file is outside this shot.

🟡 LOW HTTP server handle leaks if listenLocal fails in trace-tool-callback and model-proxy — src/analyst/trace-tool-callback.ts

createServer(server) is called before listenLocal(server). If listenLocal throws (e.g., EMFILE on socket), the server object is never closed — the handle leaks and can keep the event loop alive. The same pattern exists in startExternalOptimizerModelProxy (external-optimizer-model-proxy.ts:121). In practice, binding to 127.0.0.1:0 almost never fails, and the pattern is shared with pre-existing campaign code, so impact is negligible. Fix: wrap the listen in try/catch and call closeServer(server) on failure.

🟡 LOW New optional channel parameter is untested beyond the default — src/campaign/external-optimizer-model-proxy.ts

The channel?: CostChannel passthrough (args.args.channel ?? 'optimizer') was added at lines 45-46 and 169, but the test suite only exercises the default 'optimizer' channel (external-optimizer-process.test.ts:436 asserts channel:'optimizer' on the receipt). No test passes a custom channel to confirm the override propagates into the cost ledger receipt. Low impact — it is a one-line passthrough into runPaidCall — but coverage gap on new public surface.

🟡 LOW Zero-usage error path reports the message via two redundant branches — src/campaign/external-optimizer-model-proxy.ts

When zeroUsage is true, forwardModelProxyRequest sets both usageComplete=false AND usageError. In handleModelProxyRequest the !usageComplete branch (264-269) already sends 502 with paid.value.usageError ?? default, so the standalone if (paid.value.usageError) block at 270-273 only fires for the output-exceeds-limit case (where usageComplete is true). Functionally correct, but the two branches overlap for the zero-usage case and the second guard's true target (output-limit) is obscured. A comment clarifying the output-limit-only intent at line 270 would aid review.

🟡 LOW Backslash check in resolveRunnerCommand is dead code on supported platforms — src/campaign/external-optimizer-subprocess.ts

runExternalOptimizerProcess throws on win32 at line 41-45, so the !command.includes('\\') branch can never execute on a supported platform. On POSIX a backslash is a legal filename character so resolve() handles it, but the explicit Windows-style check adds confusion. Consider dropping the includes('\\') clause or adding a comment that it is defensive only.

🟡 LOW Breaking public-API removals with no deprecation alias — src/index.ts

This diff removes public exports from the root barrel: createAnalystAi/CreateAnalystAiConfig (line 42 in the old file), AxGepaSteeringOptimizer + AxSteeringOptimizerConfig (old lines ~803/809), and renames createTraceAnalystKind -> createTraceAnalyst, TraceAnalystKindSpec -> TraceAnalystDefinition, CreateTraceAnalystKindOpts -> CreateTraceAnalystOptions. These are breaking changes for any external consumer importing from the package root. Evidence is concrete: the symbols are gone from their source modules and the barrel. No deprecation aliases are retained. For a 0.x substrate (v0.137.0) under active API evolution this is acceptable per SemVer and consistent with the PR's +4320/-

🟡 LOW Vestigial selector/skipped fields after backend narrowing — src/steering-optimizer.ts

SteeringOptimizationResult retains selector?: SteeringOptimizationSelector (line 26) and skipped?: boolean (line 27), and SteeringOptimizationSelector retains signature?/labels?/rationale? (lines 14-19). With SteeringOptimizerBackend narrowed to 'pairwise', PairwiseSteeringOptimizer.optimize() ([lines 44-53](https://github.com/tangle-network/agent-eval/blob/71bca06f5c7d07862d43712be169142f05de63a7/src/steering-

🟡 LOW Prompt simplification drops specific failure-prevention guidance for general entry point — src/trace-analyst/prompts.ts

The prompt shrank from ~50 lines of detailed DISCOVERY→NARROW→DEEP-READ protocol to 15 lines of generic policy. The old prompt had concrete guards against specific failure modes: the 150KB threshold for choosing viewTrace vs searchTrace (old line on raw_jsonl_bytes), the result-shape contract ('iterate result.hits NOT result.matches'), explicit oversized-response handling ('DO NOT retry the same call'), per-turn console.log observability, and the output contract citing trace/span ids. The new general prompt has none of these. Kind-specific prompts (failure-mode.ts, knowledge-gap.ts, etc.) carry domain guidance, but the standalone analyzeTraces entry point — which defaults instructions to this prompt (analyst.ts:77) — may produce lower-quality or more error-prone investigations for open-end

🟡 LOW Test dropped JSON schema detail assertions (additionalProperties, filter schema) — validation still enforced at runtime — src/trace-analyst/tools.test.ts

The old test asserted additionalProperties:false on viewSpans and the full getDatasetOverview filter schema shape (has_errors, service_names, agent_names, model_names, tool_names, start_time_after/before, regex_pattern). The new test only checks queryTraces required:limit, viewSpans span_ids/min/max, and searchTrace max_matches bounds. The zod schemas in store-schemas.ts still use .strict() on every input schema and traceFiltersSchema, so unknown-field rejection is still enforced at runtime (confirmed: the 'rejects unknown fields' and 'rejects invalid filter' tests still pass). The gap is assertion coverage, not validation strictness — but a future change that accidentally loosens .strict() would not be caught by these tests.

🟡 LOW New success-path step-span loop is untested — src/traced-analyst.ts

tests/analyst-tracing.test.ts was updated to use a stub TraceAnalysisEngine, but both remaining tests (lines 27, 55) point source at a nonexistent file so analyzeTraces throws before producing a trajectory. The new post-completion loop at lines 35-48 — the core behavioral change in this file — is never exercised, and neither the per-step spans (analyst:step-N, analyst.step, analyst.step_bytes) nor the parent summary attributes (analyst.model_calls, analyst.tool_calls, analyst.step_count, analyst.finding_count, analyst.answer_length) are asserted. The

🟡 LOW Serialized step payload computed then discarded; step spans carry minimal signal — src/traced-analyst.ts

Line 36 runs JSON.stringify(step) for every trajectory entry, but only Buffer.byteLength(encoded) is recorded on the span (line 43); the serialized string is never stored. For large trajectories this is wasted CPU for observability that adds little — each step span carries only an index and a byte count, and the parent already records analyst.step_count (line 54). Additionally, because steps are emitted post-completion and each sp


tangletools · 2026-07-30T23:36:41Z · trace

…-evolution

# Conflicts:
#	src/analyst/default-registry.ts
#	src/analyst/define.ts
#	src/analyst/index.ts
#	src/analyst/kind-factory.ts
#	src/analyst/kinds/kinds.test.ts
…-evolution

# Conflicts:
#	src/analyst/benchmark-implementation.ts
#	src/analyst/benchmark-reference-result.test.ts

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 1bed96cb

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-07-31T00:07:59Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 104.0s (2 bridge agents)
Total 104.0s

💰 Value — sound-with-nits

Replaces a hand-rolled Ax agent loop with the official DSPy RLM running in a sandboxed child process behind a clean engine seam — directionally correct and in-grain, with one duplicated loopback-server skeleton worth extracting later.

  • What it does: Trace analysts become recursive research programs: a new TraceAnalysisEngine interface (src/analyst/engine.ts:60) decouples the analyst definition from the recursive engine, and createDspyRlmTraceEngine (src/analyst/dspy-rlm-engine.ts:41) runs the official dspy.RLM inside a sandboxed Deno/Pyodide child process (clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py) with seven allowlisted trace tool
  • Goals it achieves: (1) Make a trace analyst BE the official DSPy RLM rather than a bespoke reimplementation of recursion, so the published CodeTraceBench numbers (13.47%/32.73%) can be beaten by the real recursive path instead of the one-shot baseline. (2) Delete ~5000 lines of custom agent-loop machinery and a third-party dependency (Ax) in favor of the upstream canonical implementation. (3) Introduce a clean engin
  • Assessment: Sound and in the grain of the codebase. The engine seam is well-placed: the definition, finding schema, cost ledger, and exact-run identity are the stable substrate, and the engine is the variable — createDspyRlmTraceEngine reuses the existing external-optimizer subprocess spawner, model proxy, and cost-metering infrastructure rather than inventing new process management. analyzeTraces (src/trace-
  • Better / existing approach: Searched for an existing recursive/agent-loop implementation to reuse: the old trace-analyst/loop.ts WAS the bespoke loop and it is correctly being deleted, not extended — so no existing capability is being reinvented. The one materially better design is at the HTTP boundary: trace-tool-callback.ts (src/analyst/trace-tool-callback.ts:37-58) is a near-verbatim copy of the authenticated-loopback-ser
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

The DSPy RLM trace-analyst engine replaces the deleted Ax stack and is fully wired into the CLI, public API, default registry, and benchmark command, reusing the existing external-optimizer subprocess/proxy grain.

  • Integration: Fully reachable from four surfaces: the agent-eval analyst-benchmark CLI command (cli.ts:16 → runAnalystBenchmarkCommand, with createPublicBenchmarkRlmRunner as the default candidate runner at benchmark-command.ts:186), the public createDspyRlmTraceEngine export (src/index.ts:70), the standalone analyzeTraces wrapper (trace-analyst/analyst.ts:52, engine required), and buildDefaultAnalystRegistry
  • Fit with existing patterns: Follows the established grain precisely: the new engine reuses the same runExternalOptimizerProcess / startExternalOptimizerModelProxy / runWithCleanup trio that GEPA (gepa-optimization-method.ts:347) and SkillOpt (skillopt-optimization-method.ts:248) already use, and the Python bridge is a sibling of gepa_bridge.py and skillopt_bridge.py. TraceAnalysisEngine (engine.ts:60) is engine-agnostic by d
  • Real-world viability: Handles realistic conditions: per-row finding parsing tolerates malformed model output without discarding the rest of a paid investigation (dspy-rlm-engine.ts:206-222); model-call counts are cross-checked between the bridge's self-report and the provider proxy's recorded completions and throw on mismatch (dspy-rlm-engine.ts:155); cost accounting is enforced per observation (benchmark-command.ts:34
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Third copy of the authenticated loopback-server skeleton [duplication] ``

trace-tool-callback.ts reproduces the server lifecycle, abort handling, and active-handler tracking already in external-optimizer-callback.ts and external-optimizer-model-proxy.ts (same accepting flag, activeControllers/activeHandlers Sets, abortRequest that destroys request+response, closePromise ??= idempotent close, waitForActiveHandlers loop — compare src/analyst/trace-tool-callback.ts:37-58 and :124-138 with src/campaign/external-optimizer-callback.ts:26-66). A shared createAuthenticatedLoo


What this audit checks

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

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

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

value-audit · 20260731T001000Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 1bed96cb

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 25 findings (4 medium, 21 low)

glm: Correctness 0 · Security 0 · Testing 0 · Architecture 0

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

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

🟠 MEDIUM Whitespace in model-generated finding strings causes hard validation failure, discarding valid analysis — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_require_non_empty_string rejects any string where value != value.strip(). _validate_finding applies this to claim (line 560), subject (562), and evidence.uri (591-594) — all of which are LLM-generated JSON values not guaranteed to be pre-trimmed. If the model emits "claim": " Step 2 failed" or "uri": " trace://t/span/s", the entire analysis is discarded with no output written. This is inconsistent on two axes: (1) excerpt, rationale, and recommended_action use _require_optional_bounded_string which ALLOWS surrounding whitespace; (2) the top-level answer and findings_json are explicitly stripped in _prediction_string ([lines 702-703](https:

🟠 MEDIUM Per-analysis cost budget defaults to the run-wide budget — src/analyst/benchmark-command.ts

maxCostUsdPerAnalysis: maxCostUsd reuses the run-wide --max-cost-usd flag (default $5) as the per-case spend limit. With N benchmark cases, each case independently gets a $5 model-proxy budget. The shared CostLedger has a $5 run-wide cap, so case 1 can exhaust the entire run budget, causing all subsequent cases to fail with cost-limit errors — a confusing UX for operators who expect $5 total. The createPublicBenchmarkRlmRunner default (maxCostUsdPerAnalysis ?? 1) would give $1/case, which is more sensible. Consider a separate --max-cost-per-analysis flag or defaulting per-analysis to maxCostUsd / plannedCaseCount.

🟠 MEDIUM TOCTOU race on tool-call limit in trace-tool-callback — src/analyst/trace-tool-callback.ts

The calls >= args.maxCalls guard (line ~88) and calls += 1 increment (line ~96) are separated by await readJson(request) and several async-yielding checks. Node.js single-threaded execution means a second concurrent HTTP request can pass the limit check before the first increments the counter, allowing maxCalls+N tool executions. For the current DSPy RLM consumer (sequential Python subprocess) this is unlikely to trigger, but the TraceAnalysisEngine interface is generic and a concurrent engine would bypass the declared tool-call budget. Fix: move calls += 1 to immediately after the guard, before any async work, or use an atomic reservation pattern. The test 'rejects calls before executing a tool beyond the declared limit' passes only because requests are sequential.

🟠 MEDIUM Trajectory step content is stringified for byte length then discarded — step spans carry no step data — src/traced-analyst.ts

Lines 36-46 compute encoded = JSON.stringify(step) only to set analyst.step_bytes = Buffer.byteLength(encoded); encoded is never attached to the span. The resulting analyst:step-N spans contain only an index and a byte size — not the step payload. The prior implementation recorded per-turn stage/is_error/code_length/output_length, so this is an observability regression: the trace tree no longer lets a reviewer see what each engine step actually did. Fix: attach the step (full or truncated) as a span attribute or a child event payload before ending the span. Low blast radius since callers can still read result.trajectory directly, hence medium not high.

🟡 LOW Tool callback result payload has no size bound — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

The result from the callback is checked for finite-JSON serializability (_require_finite_json, line 450) but has no byte/character cap. A large trace projection flows into the sandbox interpreter and potentially into the LLM context window unbounded by this bridge (DSPy max_output_chars bounds agent output, not tool-result input). The input file itself is capped at _MAX_INPUT_BYTES=4MB (line 709) but tool results are not. The Node-side tools are documented as 'bounded summary/projection/page

🟡 LOW model_calls depends on undocumented DSPy internal lm.history attribute — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_lm_history_length reads lm.history and requires it to be a list. This is an internal DSPy attribute, not a documented public API. A DSPy minor-version change that renames or reshapes history would cause RuntimeError('DSPy LM does not expose model-call history') at line 686, breaking all analyses. Mitigated by the pin dspy==3.2.1 in pyproject.toml, but worth a comment noting the coupling so a future DSPy bump updates this proactively.

🟡 LOW toolCallback.url not restricted to loopback; bearer token exfiltratable to arbitrary endpoint — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py

_require_http_url (line 762) accepts any http/https URL with a netloc. The callback bearer token (callback['token']) is sent as an Authorization header to this URL (line 438). The input file is written by the trusted Node parent, so this is not directly exploitable, but if the input file is tampered with or the Node process is misconfigured, the token redirects to an attacker endpoint with no SSRF guard. modelProxy.baseUrl legitimately needs to allow remote LLM endpoints, so a blanket loopba

🟡 LOW Test gaps: httpx error paths, input size limit, comma-path rejection, and deno cache probing untested — clients/python/tests/test_dspy_rlm_bridge.py

The 17 tests cover happy paths and tool-callback payloads well, but leave several branches unexercised: (1) httpx error responses from _call_trace_tool (response.raise_for_status on 4xx/5xx, connection errors, timeouts); (2) the _MAX_INPUT_BYTES=4MB rejection in _read_json; (3) the comma-in-path guard in _build_deno_command (line 353) that prevents allow-read scope ambiguity; (4) _deno_cache_dir's subprocess deno-info probing and DENO_DIR env handling; (5) the whitespace-in-findings edge case described in the medium finding above. None are blocking, but (1) and (3) guard security-adjacent behavior.

🟡 LOW Runner id 'dspy-rlm' duplicated as a bare literal across 6+ coupled sites — src/analyst/benchmark-command-artifact.ts

The literal 'dspy-rlm' is hardcoded independently in benchmark-command-artifact.ts:81 (runnerIds type), benchmark-command-result.ts:150 (candidateRunnerId), benchmark-command.ts:272/344/687 (comparison, accounting guard, exit code), plus every test. There is no shared constant, so any rename requires synchronized edits across the manifest type, the comparison, the cost-accounting gate (which silently skips enforcement for any non-matching id at benchmark-command.ts:344), and the exit-code selector. A wrong id in one site would either skip cost-accounting enforcement or mis-select the exit code with no compile error. Extract a RUNNER_ID constant. Pre-existing pattern (was 'model') but this migration touched most sites, making it the right moment to fix.

🟡 LOW benchmark-implementation.test.ts relies on esbuild's bundler metafile for manifest discovery — src/analyst/benchmark-implementation.test.ts

The implementation-file completeness check shells out to scripts/check-analyst-benchmark-implementation.mjs, which spins up a full esbuild bundle (metafile:true) purely to enumerate the file set. In this sandbox the esbuild native binary panicked with a Go-runtime stack trace under the vitest worker, blocking 3 of the test's 4 cases (the checker subprocess crashed before asserting). I verified the underlying invariant is sound by reproducing the exact esbuild options standalone (succeeds, 80/80 manifest match) and recomputing both digests by hand — so this is an environment/tooling fragility, not a PR defect. Worth flagging because it makes this test brittle to esbuild version/platform changes; the manifest is already an explicit sorted list, so the bundle-driven discovery is the only heav

🟡 LOW Truncation marker reports inaccurate omitted-character count — src/analyst/benchmark-public-errors.ts

The formula omitted = redacted.length - 460 assumes a fixed marker overhead of 40 chars, but the actual marker ...[${omitted} chars omitted]... varies with digit count (~25-35 chars). The actual omitted count is redacted.length - (500 - marker.length), so the reported number overstates by 5-15 chars. The output length is correct (tail slice compensates), only the displayed count is wrong. The test 'keeps the beginning and final exception' only checks for presence of 'chars omitted', not the exact count.

🟡 LOW define.test.ts dropped coverage for defineCustomAnalyst's missing-cost guard — src/analyst/define.test.ts

The old suite tested that defineTraceAnalyst rejected a missing cost declaration from JS callers; the migration moved the guard to defineCustomAnalyst (define.ts:58-60, 'cost must be declared') but the new tests only cover executionConfig validation. The guard still exists but is no longer directly exercised. TS enforces cost for typed callers, so only untyped/JS callers are exposed. Add a one-line case asserting defineCustomAnalyst throws /cost must be declared/ when cost is omitted.

🟡 LOW Bridge output validation rejects zero model calls — src/analyst/dspy-rlm-engine.ts

The check (value.modelCalls as number) <= 0 throws when the bridge reports modelCalls: 0. If the DSPy RLM ever completes an analysis using only prepared context without a provider call (unlikely but not impossible for trivial cases), this would reject a valid result. The subsequent cross-check parsed.modelCalls !== successfulCompletions would also fail since the proxy recorded 0 completions. Consider allowing modelCalls: 0 when the findings array is empty and the answer is substantive.

🟡 LOW Evidence verification makes 2 store calls per citation with no batching — src/analyst/kind-factory.ts

For each trace:// evidence citation, hasSpans is called to verify existence, then viewSpans is called again to verify the excerpt. With N citations across M accepted findings, this is up to 2NM store round-trips. For the default kinds (minimumEvidenceCitations=1-2, typically <10 findings), this is ~20-40 store calls — acceptable. But a knowledge-poisoning kind with minimumEvidenceCitations=2 and many findings could amplify latency. Not a correctness issue, but worth noting for performance-sensitive deployments. Could batch by trace_id to reduce round-trips.

🟡 LOW Breaking public-API rename actorDescription -> instructions is documented but untested for external compatibility — src/campaign/analyst-surface.ts

The analyze callback input field renamed from actorDescription to instructions. This is an exported public type (re-exported via src/campaign/index.ts:25). External consumers implementing this interface break at compile time. The rename is semantically justified (the field carries analysis instructions, not an actor description) and is documented in CHANGELOG.md:50 as a breaking change. All in-repo implementors are updated and the test (analyst-surface.test.ts:58) confirms the new key. No action required beyond awareness; flagged only for completeness of the public-API surface audit.

🟡 LOW Over-limit output-token check may reject legitimate responses from providers that over-count tokens — src/campaign/external-optimizer-model-proxy.ts

The new usageError fires when usage.outputTokens > parsed.maxOutputTokens on a 2xx response, returning 502. usage.outputTokens is the provider-reported total (parseProviderUsage returns the raw completion_tokens / output_tokens, with reasoning_tokens as a subset field). Providers that count reasoning/effective tokens generously relative to the requested max_tokens cap could trip this check even when the visible completion is within limits. This is a deliberate strict-contract enforcement (fail loud, tested at external-optimizer-process.test.ts:1110), and the error message is diagnostic. No fix needed; noting as a behavioral change that could surface as false 502s on unusual providers.

🟡 LOW Breaking public-API renames with no deprecation alias — src/index.ts

createTraceAnalystKind→createTraceAnalyst, TraceAnalystKindSpec→TraceAnalystDefinition, CreateTraceAnalystKindOpts→CreateTraceAnalystOptions, plus removal of createAnalystAi/CreateAnalystAiConfig and AxGepaSteeringOptimizer/AxSteeringOptimizerConfig are hard breaks with no @deprecated shim. This is fine for an intentional major refactor (the verify-package-exports.mjs @ts-expect-error guards prove intent), but downstream consumers on the old names will fail to compile at upgrade. Confirm the PR's CHANGELOG/version bump (already in this PR's CHANGELOG per the diffstat) carries these as breaking, and consider whether a 0.x vs 1.x bump is warranted. No code change required for this shot.

🟡 LOW Backend type narrowed to single literal union — src/steering-optimizer.ts

export type SteeringOptimizerBackend = 'pairwise' is now a single-string literal union; consumers reading result.backend cannot see any other value. Harmless but slightly noisy. Consider replacing usages with the literal 'pairwise' (or inlining) once the dead selector/skipped fields are removed, or keep as-is if a future campaign backend is planned — the class docstring (lines 34-38) implies one is.

🟡 LOW Dead result fields and orphaned selector interface after GEPA removal — src/steering-optimizer.ts

PairwiseSteeringOptimizer.optimize (lines 47-52) never sets selector?, skipped?, and selectVariant was deleted. SteeringOptimizationSelector (lines 14-19) and the selector?: SteeringOptimizationSelector field (line 26) plus skipped?: boolean (line 27) are now unreachable from the sole produc

🟡 LOW budgetUsd silently ignored when costLedger is also supplied — src/trace-analyst/analyst.ts

AnalyzeTracesOptions declares both budgetUsd and costLedger as independent optional inputs, and analyzeTraces forwards both into context (lines 86-87). But runTraceAnalyst (src/analyst/kind-factory.ts:62) does const costLedger = context.costLedger ?? new CostLedger(context.budgetUsd) — when a shared costLedger is supplied, budgetUsd is never applied to it. A caller passing { budgetUsd: 5, costLedger: sharedLedger } expecting a per-call cap gets no enforcement of the 5 USD bound. The shared-ledger-wins behavior is defensible, but the AnalyzeTracesOptions JSDoc does not state the precedence. Fix: add a one-line note on budgetUsd ('ignored when costLedger is sup

🟡 LOW runId defaults to a constant when neither runId nor id is supplied — src/trace-analyst/analyst.ts

Line 59 id = input.id?.trim() || 'trace-analysis' and line 84 runId: options.runId ?? id mean a caller invoking analyzeTraces({ question }, { source, engine }) with no id/runId gets runId='trace-analysis', definition.id='trace-analysis', and costTags.analystId='trace-analysis' for every such call. correlationId (randomUUID) keeps per-call logs distinguishable, and runTraceAnalyst does not key any storage off runId, so there is no data-integrity risk. But cost telemetry and log attribution for callers that omit both fields will conflate

🟡 LOW Inconsistent assertion style between two validation-rejection test blocks — src/trace-analyst/tools.test.ts

The 'rejects invalid %s arguments' block (line 67) asserts .rejects.toBeInstanceOf(TraceAnalysisValidationError) while the 'rejects an invalid filter' block (line 145) asserts .rejects.toMatchObject({ code: 'validation' }). Both paths route through parseTraceInput which always throws TraceAnalysisValidationError (which carries code:'validation'), so both pass today, but the mixed style is a latent maintenance trap: if a future error type also sets code:'validation', the toMatchObject assertion would still pass while instanceof wo

🟡 LOW Default limits hard-coded (12 / 8) duplicate DEFAULT_TRACE_ANALYST_LIMITS and can silently drift — src/traced-analyst.ts

Lines 27-28 use options.limits?.maxIterations ?? 12 and options.limits?.maxLlmCalls ?? 8. The real defaults live in DEFAULT_TRACE_ANALYST_LIMITS (src/analyst/engine.ts:16-22) as maxIterations=12, maxLlmCalls=8. If those constants change, these span attributes will report stale limits while the engine enforces the new ones. Import and reference DEFAULT_TRACE_ANALYST_LIMITS.maxIterations / .maxLlmCalls instead. Pure maintainability nit; no runtime impact today.

🟡 LOW Step spans are emitted post-hoc with no real duration or error status — src/traced-analyst.ts

The for-loop over result.trajectory runs AFTER analyzeTraces resolves (line 34), so each step span is created and immediately .end()-ed (line 47) — startedAt ≈ endedAt, and a step that errored mid-engine-run is indistinguishable from a successful one. The old wrapper marked turn.isError via turnSpan.fail(). Acceptable for a pure audit breadcrumb, but the span tree no longer represents step timing or per-step failure. If timing/failure granularity matters, the engine itself would need to emit spans via a callback; document the limitation or drop the per

🟡 LOW Success path of tracedAnalyzeTraces has no test coverage — src/traced-analyst.ts

tests/analyst-tracing.test.ts only exercises the error path (nonexistent source file), so the trajectory-span loop (lines 35-48) and the success parentSpan.end attributes (analyst.model_calls/tool_calls/step_count/finding_count/answer_length) are never asserted. A fake engine + in-memory store (both already available in-repo) could stub analyzeTraces to return a result with a 2-3 entry trajectory and assert the step-N spans and summary attributes. Not blocking since the path is simple and typechecks, but it is the most behaviorally meaningful part of the wrapper.


tangletools · 2026-07-31T00:32:21Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 25 non-blocking findings — 1bed96cb

Full multi-shot audit completed 8/8 planned shots over 67 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-07-31T00:32:21Z · immutable trace

@drewstone
drewstone merged commit 4878b6b into main Jul 31, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants