feat(analyst): recursive analyst earns its architecture — typed environment engine, recovery stack, rubric-aligned methodology - #514
Conversation
Five diagnosed losses from the codetracebench-rlm-glm52-20260731 run, each recovered with a named diagnostic instead of a voided case: - an oversized or over-count failure block drops alone, recorded in blockDiagnostics.droppedBlocks, instead of zeroing the whole case - a finding citing a step outside its own block survives when at least one citation falls inside; trimmed citations are recorded, and a finding with no in-block citation is still rejected - the bridge salvages findings_json from a bracket-damaged marker or a trailing findings-shaped JSON array, then spends exactly one repair turn re-emitting the array from the model's own answer, flagged as format_repair_used in the runtime record; an empty answer stays empty - rawFindings rows carry block metadata parsed from the subject grammar so a failed case retains its block coordinates - the model proxy retries provider 429s up to three times with jittered 2s/8s/30s backoff, counting each retry against the request budget and surfacing modelRateLimitRetries; other failures still fail immediately - an engine run that completes with zero findings gets one direct structured call, marked runnerMetadata.abstentionFallback='direct', its cost merged under the same case and repetition tags; engine errors never trigger the fallback
Two repetitions of the recursive analyst agree on almost nothing per case (median predicted-block-set Jaccard 0.085 over 16 labeled cases), so a single run is one noisy draw from the answer distribution. --rlm-samples k runs the engine k times per case, sequentially, each with a fresh subprocess and proxy, and scores only the step-level majority: - each sample's blocks pass the existing subject-grammar validation and expansion; its accepted, evidence-resolved steps are its votes - steps present in >= ceil(k/2) samples survive and reassemble into contiguous consensus blocks; each block borrows metadata from the max-step-overlap contributor (ties: higher confidence, then earlier sample) and carries the mean contributor confidence - consensus blocks re-enter expandCodeTraceFailureBlocks, so width, count, and consequence-evidence rules are enforced with the same drop-with-diagnostic behavior as a single run - the abstention fallback applies AFTER the vote, once, only when no step reached the threshold - never per sample - all k samples' provider calls settle under the case tags; the case usage receipt merges them, and runner metadata records per-sample blocks, answers, cost, and the full voting record - k lands in the run identity digest, provenance metadata, and result.json execution inputs; resumes cannot mix sample counts - k=1 leaves the current path untouched; above 1 requires the codetracebench dataset and the dspy-rlm analyst, refused otherwise
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — b2d809d1
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-01T03:08:05Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 187.2s (2 bridge agents) |
| Total | 187.2s |
💰 Value — sound-with-nits
A coherent, well-factored rewrite of the recursive analyst: typed DSPy output replaces model-emitted JSON markers, a recovery stack sits at the correct Python boundary, and the shared TS expansion/scoring pipeline is reused — only minor proportionality and a typed↔string round-trip worth a note.
- What it does: Three deltas. (1) For CodeTraceBench the Python RLM bridge now builds a typed task signature: the whole trajectory loads into the REPL as variables and the model SUBMITs a pydantic-validated list[IncorrectBlock] (dspy_rlm_bridge.py:486-541); the bridge then builds subjects/URIs/excerpts in code (dspy_rlm_bridge.py:778-819). This replaces the old 'findings_json' marker-string transport for this tas
- Goals it achieves: Eliminate a structural failure class (5 zero-scored marker-format runs) by removing model-emitted JSON transport; recover paid findings lost to format/infra noise so a completed investigation keeps its usable signal; align the prompt with the benchmark's own rubric; and keep the instruments (consensus, recovery diagnostics) that make future accuracy claims falsifiable. The system gets a recursive
- Assessment: Sound and in the codebase's grain. The typed path is the right call: it uses DSPy RLM's native typed-output mechanism instead of asking the model to hand-format field markers, which is exactly where the prior failure class lived. Recovery sits at the correct boundary — the Python bridge is where raw DSPy prediction fields first materialize; TS never sees the marker text, so salvage cannot live the
- Better / existing approach: Searched parse-tolerant.ts, judge-retry.ts/withJudgeRetry, adapters/http.ts retry, the direct runner (benchmark-public-model.ts), and the shared adapter pipeline (benchmark-public-adapters.ts). No existing equivalent is reinvented: recovery must be Python-side, the expansion pipeline is shared not forked, and the abstention floor reuses the direct runner. The only available simplification is low-l
- 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-with-nits
A coherent, fully-wired upgrade to the recursive analyst — typed block output eliminates a real zero-score failure class, the consensus flag ships a measured-null lever dormant by default, and the recovery stack hardens real provider paths; everything routes through the existing benchmark command an
- Integration: Fully reachable on the next codetrace run. The typed signature is auto-selected inside dspy_rlm_bridge.py:345 whenever the instructions carry the
incorrect-steps-token that publicBenchmarkRlmInstructions('codetracebench') always emits (benchmark-public-prompt.ts:106) — so--analyst dspy-rlm(the default, benchmark-command.ts:493) activates it with no caller change.--rlm-samples kflows CLI - Fit with existing patterns: Built in the grain of the codebase. createPublicBenchmarkRlmRunner mirrors the existing createPublicBenchmarkDirectRunner factory pattern. The typed signature is a dataset specialization (codetracebench only; agentrx retains the marker path), not a competing implementation. The recovery stack augments the bridge in place rather than restructuring it. One design choice worth noting: signature selec
- Real-world viability: Holds up past the happy path. The typed path handles oversized traces (view-trace budget exceeded → span-probe fallback at _probe_codetrace_spans), missing verification spans (treated as data, not errors), and pydantic-validated block shapes with a model_validator enforcing first≤last≤+12. The 429 retry is bounded (3 delays, jittered, each retry consumes a budgeted request slot so accounting stays
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Typed-signature activation couples to a prompt substring, not a wire field [integration] ``
_uses_codetrace_contract (dspy_rlm_bridge.py:456) turns the typed path on/off by testing whether the instructions contain 'incorrect-steps-'. That token currently appears only in CODE_TRACE_RLM_CONTRACT (benchmark-public-prompt.ts:106), so the coupling holds today, but a future prompt edit that rephrases the subject grammar would silently revert codetracebench to the old marker path — the exact failure class this PR exists to kill — with no error. The comment at line 68-71 documents the tradeoff
💰 Value Audit
🟡 Consensus voting ships dormant for a measured null [proportion] ``
benchmark-public-consensus.ts (184 lines) plus the multi-sample orchestration in benchmark-public-rlm.ts:91-211 (~120 lines) and the --rlm-samples flag all default to k=1 and are explicitly documented as a measured-null accuracy lever (oracle selection ≤ single-run at 3× cost). For a claims-honesty eval substrate, keeping the instrument that reproduces the null is defensible — it blocks re-proposing consensus without the prior measurement — but it is ~300 lines that never fire in normal operatio
🟡 Typed blocks round-trip through the subject grammar string [better-architecture] ``
_build_block_findings (dspy_rlm_bridge.py:804-812) constructs the subject 'incorrect-steps----consequence-' from typed IncorrectBlock fields; adaptCodeTraceFindings (benchmark-public-adapters.ts:246-292) then parses that same string back via CODE_TRACE_BLOCK_SUBJECT to recover the typed block. A structured block field on the wire finding would remove the serialize/parse pair and the regex coupling. Low-leverage because the subject grammar is the shared contract both runners emit an
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.
|
W2 A/B complete (both splits, 2 reps, glm-5.2, pre-registered):
Deleting the numeric width prior (b2d809d) did not remove the narrow-gold cost — the cascade-extension doctrine itself trades narrow-precision for wide-recall — but grew the wide-gold gain. Pooled over every labeled case measured, the branch tip beats both the pre-W stack and the shipped baseline; the regime structure is disclosed above. Width-adaptive extension is recorded as the next open problem in |
❌ Needs Work —
|
| glm | deepseek | deepseek-flash | aggregate | |
|---|---|---|---|---|
| Readiness | 25 | 67 | 0 | 0 |
| Confidence | 85 | 85 | 85 | 85 |
| Correctness | 25 | 67 | 0 | 0 |
| Security | 25 | 67 | 0 | 0 |
| Testing | 25 | 67 | 0 | 0 |
| Architecture | 25 | 67 | 0 | 0 |
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 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision.
Blocking
🔴 HIGH max-output-tokens default regresses main's #508 fix and PR does not merge cleanly — src/analyst/benchmark-command.ts
Base f85e8f6 (main) pins DEFAULT_MODEL_OUTPUT_TOKENS=16384 with a comment documenting that 4096 made glm-5.2 fail 2/2 smoke cases ('provider reported 8192 completion tokens, exceeding requested limit 4096' -> 502) — that fix is #508, merged into main after this branch split off at d312b80. This PR restores 4096 in both dspy-rlm-engine.ts:16 and the CLI default here (also help text at :454), deleting the explanatory comment. Since parseCommandConfig always passes maxOutputTokens explicitly, a 4096 default overrides even the engine's 16384. git merge-tree f85e8f6 b2d809d reports a content conflict on exactly these lines and GitHub reports mergeable=false/dirty. If the merge resolution keeps the PR's 4096, the documented glm-5.2 502 rejection returns; the PR neither acknowledges nor addre
🔴 HIGH 429 retries inflate requestAttempts() and break GEPA's post-run request-count reconciliation, failing the run it was meant to rescue — src/campaign/external-optimizer-model-proxy.ts
tryConsumeRateLimitRetry increments requestCount (line 134), and requestAttempts() returns requestCount (line 161). Pre-PR, requestAttempts() == number of requests the optimizer process sent to the proxy. external-optimizer-accounting.ts:79 (NOT modified in this PR) then enforces
if (upstream.requestAttempts !== undefined && upstream.requestAttempts !== requestAttempts) throw. The GEPA bridge always reports requestAttempts (gepa_model_proxy.py observe_request counts each HTTP request the GEPA client s
Other
🟠 MEDIUM Codetrace transport selected by substring-sniffing caller-controlled instructions — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_analyzechooses a different signature, a pre-flight dataset/span fetch, typed blocks output, and disables salvage/repair purely on"incorrect-steps-" in instructions. The instructions are caller-controlled wire input. False positive: any generic analysis whose instructions contain that literal silently switches transport — it then requires dataset total_traces==1 (RuntimeError, full analysis failure) and, when the callback lacks getDatasetOverview/viewTrace, fails before any model call. False negative: a codetrace task run with instructions that omit the token (e.g., the JSON-transport prompt, which never contains it) would emit generic-grammar findings that CODE_TRACE_BLOCK_SUBJECT rejects downstream — a silent zero. Current callers are correct (agentrx prompt lacks the token; codetr
🟠 MEDIUM Generic path spends one extra paid completion on every empty-findings analysis — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The salvage/repair stack runs whenever final findings are empty, regardless of whether the model genuinely reported none. Empirically confirmed: a model emitting a well-formed findings_json='[]' plus a normal prose answer yields findings=[], salvage returns None on both sources, so
format_repair_used=Trueand _repair_findings_turn fires one full completion (up to maxOutputTokens, default 4096). Because the tolerant adapter (the default controlAdapter) recovers a missing findings_json to '[]', the empty outcome is the common case on clean generic analyses (agentrx benchmark, general trace-analysis). This is a billing/latency regression: pre-PR an empty result cost nothing extra. It is bounded (exactly one call, within proxy maxRequests = maxIterations+maxLlmCalls+1) and flagged in runtime
🟠 MEDIUM Lint regression: 5 new ruff errors in previously-clean source file — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
Base commit had 0 ruff errors in this file; HEAD has 5. UP037 at line 159:
def _enforce_block_shape(self) -> "IncorrectBlock":uses a quoted forward reference, butfrom __future__ import annotationsis active (line 3), making all annotations lazy already — the quotes are redundant and auto-fixable. E501 at lines 93, 95 (prompt strings 172 an
🟠 MEDIUM Span-probe fallback path has zero test coverage despite complex pagination/termination logic — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The typed analysis has two fetch modes: 'view-trace' (tested by test_codetrace_task_materializes_trajectory_and_emits_code_built_findings) and 'span-probe' (completely untested). _probe_codetrace_spans (line 602) pages through step IDs in batches of 100 up to _MAX_PROBED_STEPS=400 with a break condition
len(missing) == len(page_ids). _view_spans_paged (line 641) re-requests omitted spans viapending = omittedwith a guardlen(omitted) >= len(pending). _search_verification_spans ([line
🟠 MEDIUM Typed path aborts the entire paid investigation on prose-only blocks output — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
When the tolerant adapter recovers a prose-only completion, the blocks field falls through to default "" and _coerce_typed_blocks raises RuntimeError('not a JSON array after one repair'), discarding the completed paid investigation (answer + trajectory + any blocks the model did produce). The generic path instead recovers with exactly one bounded repair turn (_repair_findings_turn). This asymmetry is deliberate (comment: 'fails loudly') and test-locked (test_codetrace_unparseable_blocks_fail_loud_not_silent_empty), but it is the only paid-loss path in the shot: a modest formatting miss (prose SUBMIT without markers/JSON-object wrapper — under the default tolerant adapter the fenced-array-only and prose forms both recover blocks to "") costs the whole case, which on a benchmark scores 0. Re
🟠 MEDIUM _search_verification_spans catches RuntimeError/ValueError too broadly — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The except clause catches RuntimeError and ValueError alongside httpx.HTTPError. The docstring says 'absence is data, not an error', but RuntimeError from _call_trace_tool (e.g. missing callback context at line 990) or ValueError from JSON parsing (line 1000) indicate bridge infrastructure bugs, not absent data. If searchTrace fails due to a missing callback during the span-probe fallback path, the error is silently converted to 'no verification spans' with nothing in the runtime record. Na
🟠 MEDIUM max-output-tokens default reverted 16384->4096; base documented 4096 caused hard 502 provider failures — src/analyst/benchmark-command.ts
Base f85e8f6 set the CLI default and engine DEFAULT_MODEL_OUTPUT_TOKENS to 16_384 with an explicit comment: '4096 is below what current coding models emit... glm-5.2 through an OpenAI-compatible gateway returns 8192 and the request is rejected outright (502 - provider reported 8192 completion tokens, exceeding requested limit 4096), so the default failed 2/2 smoke cases'. This PR reverts BOTH defaults to 4_096 (benchmark-command.ts:519 and dspy-rlm-engine.ts:16) and deletes that comment, with no commit message or code comment explaining why 4096 is now safe. The 502 was provider-side (gateway rejected a >cap completion), not controllable by the cost reservation, so it will recur if the model still emits >4096-token turns. No test guards the default (every test passes explicit maxOutputToke
🟠 MEDIUM Consensus segment wider than MAX_INCORRECT_BLOCK_STEPS is dropped with no fallback — src/analyst/benchmark-public-consensus.ts
consensusCodeTraceBlocks reassembles contiguous kept steps into blocks whose width is the union across samples; with k=2 the threshold is 1, so two overlapping ≤12-step blocks (e.g. [1..12] and [4..15]) yield a kept segment [1..15], width 15 > MAX_INCORRECT_BLOCK_STEPS=12. The final expandCodeTraceFailureBlocks (benchmark-public-adapters.ts:316-317, acceptCodeTraceBlockShape) then drops the whole consensus block. Because consensus.blocks.length was 1, the abstention fallback does NOT fire, so a case where samples agreed on a wide failure silently scores as empty/clean with only a droppedBlocks diagnostic. Guard: trigger the fallback on post-expansion empty when pre-expansion consensus had blocks.
🟠 MEDIUM Abstention floor fires on explicit clean verdicts, contradicting its own contract — src/analyst/benchmark-public-rlm.ts
The single-sample branch triggers the direct fallback on completed.findings.length === 0 and the multi-sample branch on consensus.blocks.length === 0 (line 183). The comment at :263-265 claims 'An explicit clean verdict arrives as a finding and never reaches this branch', but the PR's own prompt (benchmark-public-prompt.ts:55 'return an empty findings array' and :111 'Return no finding for a clean trajectory') directs the model to emit ZERO findings for a clean trajectory — so a clean verdict reaches the branch in every case. Impact: every clean case (single) and every unanimous-clean panel (multi) spends an extra paid direct call, and the retired one-shot
🟠 MEDIUM requestAttempts contract semantic silently changed without updating its doc or the strict reconciliation check — src/campaign/external-optimizer-contracts.ts
The doc still reads 'Provider requests admitted during this proxy process, including failures', but the new retry path makes requestAttempts() count upstream re-fetches (external-optimizer-model-proxy.ts:134), so it is no longer the count of requests the optimizer sent. The downstream strict-equality check in external-optimizer-accounting.ts:79 assumes the old meaning and was left untouched, turning a recoverable 429 into a hard run failure for the GEPA path (see high finding). Either keep requestAttempts() as admitted-request count (recommended) or update the doc, the budget comment for maxRequests, and relax/reconcile external-optimizer-accounting.ts:79 to accept requestAttempts() >= upstream.requestAttempts.
🟠 MEDIUM Retry-inflated requestAttempts() can trip upstream accounting assertion — src/campaign/external-optimizer-model-proxy.ts
tryConsumeRateLimitRetry increments requestCount (line 134), which is returned by requestAttempts() (line 161). Both gepa-optimization-method.ts:441 and skillopt-optimization-method.ts:342 pass modelProxy.requestAttempts() into assertExternalOptimizerCompletionCount (external-optimizer-accounting.ts:79), which throws when upstream.tokenUsage.requestAttempts !== requestAttempts. Before this PR, requestAttempts equaled the optimizer's own call count to the proxy. After this PR, it includes proxy-internal
🟡 LOW 9 new ruff violations in the repo's own lint config — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The base versions pass
ruff checkunder the repo config (select E,F,W,I,B,UP, line-length 100); this PR adds E501 at dspy_rlm_bridge.py:93,95,1179,1180, UP037 at :159, and E501 at test_dspy_rlm_bridge_codetrace.py:119,179,186,216. The test_dspy_rlm_bridge.py:406 E501 is pre-existing. CI does not run ruff on the Python client, so non-blocking, but the new files violate the package's declared lint rules.
🟡 LOW Block contiguity not verified: intermediate steps between first and last are unchecked — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_block_environment_defect validates only first_step, last_step, and consequence_step for existence and kind=='LLM'. For a block first_step=2, last_step=12, steps 3-11 are never checked. If intermediate steps don't exist or are TOOL/other kinds, the block is accepted and the subject 'incorrect-steps-2-12-unescaped-...' implies a contiguous range that doesn't exist. The prompt instructs the model to extend through 'every such step', but the bridge doesn't enforce this invariant. Impact: downstream scoring against the subject grammar may mismatch if it expects all intermediate steps to be LLM spans. Not a crash, but an eval-validity risk for cascading-failure blocks.
🟡 LOW Codetrace contract detection is a bare substring match with no fallback diagnostic — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_CODETRACE_TASK_TOKEN = 'incorrect-steps-' is a simple substring presence check. If the TypeScript benchmark generator changes the instruction token without updating the Python bridge, codetrace tasks silently fall back to the generic findings_json path with no runtime diagnostic. Both paths produce valid output, so this isn't a correctness bug, but the silent fallback makes regression detection hard. Consider adding a runtime record flag or an explicit wire-protocol field to select the codetrace contract instead of relying on instruction-text coupling.
🟡 LOW Contract detection uses fragile substring match that could misdetect non-codetrace prompts — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_uses_codetrace_contractreturns_CODETRACE_TASK_TOKEN in instructionswhere the token is 'incorrect-steps-'. Any analyst prompt mentioning this substring — including one that says 'do not use incorrect-steps- format' — activates the entire typed path (different signature, environment fetch, typed blocks validation, different output schema). The test test_codetrace_contract_detection_matches_only_the_subject_grammar_token confirms common cases but the heuristic is inherently brittle. The comment documents the assumption ('the only analyst prompt that names the subject grammar token') but there is no guard against future prompts reusing the substring.
🟡 LOW Excerpt floor of 8 chars cites a downstream rule that does not exist — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The comment claims 'the downstream evidence gates ... reject any excerpt under 8'. assertExactActionExcerpt (benchmark-evidence-validation.ts:358) actually uses requiredLength = min(12, content.trim().length) with no 8-char floor. A block citing a step whose action content is 1-7 chars is dropped here ('carries no quotable action content') but would pass downstream (excerpt.length == content length >= min(12, len)). Net effect is a narrow recall loss on very short actions plus an inaccurate justification in the source. Low impact in practice; either align the constant to the real gate or correct the comment.
🟡 LOW Rationale field emitted with unstripped whitespace, inconsistent with claim handling — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
IncorrectBlock.claim has
strip_whitespace=Truein its StringConstraints, but IncorrectBlock.rationale (line 151) does not. In _build_block_findings line 814, the code checksblock.rationale.strip()for truthiness but writesblock.rationale(unstripped) into the finding dict. Downstream _validate_finding accepts it via _require_optional_bounded_string which allows leading/trailing whitespace. If the model emits rationale=' foo ', the finding carries ' foo ' rather than 'foo'. Fix: a
🟡 LOW _last_findings_array O(n²) scan on pathological input — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The function scans backward through every '[' in the text with raw_decode, executing O(n) decode attempts. On model output with many non-finding brackets (e.g. a long answer referencing array indices [0]...[999]), this is O(n×k) decode operations. In practice answer text is bounded by output limits, but the scan runs unconditionally on the salvage path. Verified it terminates (tested on 5000-bracket pathological input) but with unnecessary work. Consider early bail-out when search_end reaches a reasonable floor (e.g. last 64KB of text).
🟡 LOW typedBlocks.dropped index values mix two numbering bases — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
Cap-loop and pydantic-parse drops use the model-reported index (position in raw blocks); excerpt-defect drops inside _build_block_findings use the kept-list index (enumerate over
kept). When both kinds of drop occur, runtime.typedBlocks.dropped[].index entries refer to different arrays, which can mislead anyone replaying the diagnostics against the model output. No scoring impact.
🟡 LOW Donor consequenceStep can precede the narrower consensus segment and void a majority-kept block — src/analyst/benchmark-public-consensus.ts
Consensus blocks copy consequenceStep from the donor's original block but set firstStep/lastStep to the (possibly narrower) segment bounds. When the donor block extends before the segment (e.g. donor [1..5] consequence 1, kept segment [3..5]), the consensus block [3..5] consequence 1 fails codeTraceBlockShapeViolation (benchmark-public-adapters.ts:414, consequenceStep < firstStep) and is dropped in the final expansion, losing steps that each carried ≥threshold votes. Recorded in droppedBlocks, but the scored output zeroes a legitimately majority-voted step.
🟡 LOW viewTrace mock ignores trace_id parameter in tests — src/analyst/benchmark-public-rlm.test.ts
The mock's viewTrace function takes no arguments and always returns the same static spans array regardless of requested trace_id. This means the tests never validate that the runner requests the correct trace. Not a bug (the runner uses traceStore from the input case), but it weakens the assertion that the correct trace data was loaded. The same pattern exists in benchmark-command.test.ts tests.
🟡 LOW Falling fallback call errors are swallowed and scored as an empty/clean case — src/analyst/benchmark-public-rlm.ts
When abstentionFallbackRunner.analyze returns an error result, both paths return findings: fallback && !fallback.error ? fallback.findings : adapted.findings — the error is recorded only in metadata (abstentionFallbackError) and the case is scored as an empty/clean verdict instead of a failed run. This is asymmetric with engine failures, which surface as output.error and increment failedRuns. A paid provider failure during the fallback is thus silently mislabeled as 'no findings' in the scored observation.
🟡 LOW Redundant cost-ledger query inside multi-sample recordUsage callback — src/analyst/benchmark-public-rlm.ts
The recordUsage callback in the samples > 1 path calls usageReceiptFromCostLedger(costLedger, caseUsageFilter) on every sample completion (line 117), then again after the loop (line 186). Each inner call is overwritten by the next sample's callback or the post-loop call. The post-loop call alone would suffice. Cost is negligible (~a ledger summary scan with k samples), but the double-read on the last sample is avoidable noise.
🟡 LOW lint gate is red: 4 biome formatting/import-sort violations in shot files — src/analyst/benchmark-public-rlm.ts
pnpm lint(biome check src) exits 1. Four violations, all in this shot: (1) benchmark-public-rlm.ts:13 import name sort (codeTraceBlockMetadataFromSubject before type CodeTraceStepAssignment); (2) benchmark-public-rlm.ts:203 conditional-spread line wrapping; (3) benchmark-public-consensus.test.ts:2 multi-line import + :148 expect wrapping; (4) benchmark-command.test.ts:404 vi.fn callback wrapping. All are auto-fixable withbiome check --write src. These fail the repo's husky/lint-staged gate as-is.
🟡 LOW per-case spend scales linearly with samples; budget name reads per-case — src/analyst/benchmark-public-rlm.ts
Each of k samples calls engine.analyze(), and createDspyRlmTraceEngine builds a fresh modelProxy per analyze() with its own budget.maxCostUsd = maxCostUsdPerAnalysis (dspy-rlm-engine.ts:110-123). So a consensus case's ceiling is effectively k * maxCostUsdPerAnalysis plus one abstention-fallback call, not maxCostUsdPerAnalysis. The run-wide maxCostUsd still bounds total spend, so this is not unbounded, but the field name maxCostUsdPerAnalysis implies a per-case cap. Document the k multiplier on the rlm-samples flag/help text, or enforce a single shared per-case ceiling across all samples.
🟡 LOW Default output tokens reduced from 16384 to 4096 may truncate real completions — src/analyst/dspy-rlm-engine.ts
The constant was changed from 16_384 back to 4_096, removing the comment that documented a real provider failure: 'glm-5.2 through an OpenAI-compatible gateway returns 8192 and the request is rejected outright (502 — provider reported 8192 completion tokens, exceeding requested limit 4096)'. The CLI default in benchmark-command.ts:448 also changed from 16_384 to 4_096. If that gateway/provider limitation still exists, operators running with defaults will get truncation or 502 errors. Mitigated by the --max-output-tokens CLI flag and the tolerant control adapter which can recover partial output.
🟡 LOW No test exercises abort during backoff, or the retry path through an optimizer reconciliation consumer — src/campaign/external-optimizer-model-proxy.ts
Tests cover retry recovery, exhaustion, and non-429 passthrough (external-optimizer-process.test.ts:1229-1374), all with an injected sleepImpl. There is no test that controller.abort() (request timeout or proxy close) lands in the backoff sleep and correctly produces 504/503 with the reservation settled, and no GEPA/SkillOpt-level test proves requestAttempts reconciliation still holds when retries occur — the regression in the high finding shipped untested. Recommend adding an abort-during-backoff test and a reconciliation test with retries enabled.
🟡 LOW No test for budget-exhaustion-during-retry path — src/campaign/external-optimizer-model-proxy.ts
tryConsumeRateLimitRetry() can return false when totalRequestCount >= maxRequests mid-retry. This path (429 received, retry rejected due to budget, 429 forwarded immediately with no sleep) is not covered by any test. The existing tests cover full retry exhaustion (maxRequests=10, all retries consumed) and recovery, but not the budget-boundary case where maxRequests is exactly hit during retries. The condition short-circuits correctly per code inspection (!args.tryConsumeRateLimitRetry() → return forwarded), but lacks test verification.
🟡 LOW Provider Retry-After header ignored; conflicts with llm-client.ts convention — src/campaign/external-optimizer-model-proxy.ts
The retry loop uses a fixed schedule [2s, 8s, 30s] with 25% jitter and ignores the provider's Retry-After response header. The codebase already has parseRetryAfter (llm-client.ts:372) handling both numeric-seconds and HTTP-date formats, and llm-client.ts:687 honors it. A provider returning Retry-After: 60 alongside a 429 will see the proxy retry at 2s (during cooldown), consume a budgeted request slot, and get another 429 — wasting maxRequests budget. Fix: parse forwarded receipt or response headers for Retry-After and use max(providerHint, scheduledDelay).
🟡 LOW rateLimitRetries() is not carried across resume via initialUsage — src/campaign/external-optimizer-model-proxy.ts
initialUsage carries only requests and costUsd, so a resumed proxy reports rateLimitRetries()=0 and a fresh requestAttempts() baseline even though prior 429 retries occurred (they are not distinguishable in the ledger). Reporting-only gap: no budget or reconciliation impact on resumed runs, but the DSPy RLM modelRateLimitRetries runtime field (dspy-rlm-engine.ts:193) will undercount on resumes. Low severity; consider persisting the counter alongside initialUsage if the metric is meant to be cumulative.
🟡 LOW Abort-during-backoff path is untested — tests/campaign/external-optimizer-process.test.ts
The real sleep implementation abortableDelay (external-optimizer-model-proxy.ts:753) rejects when the request signal aborts mid-backoff, which surfaces as a 504 through isAbortError (line 306). None of the new tests close the proxy or abort while a retry sleep is in flight to verify the request fails cleanly with 504 and no ledger/accounting corruption. Since the tests inject sleepImpl, adding a case where the injected sleep aborts (or aborts via proxy.close() during the injected sleep) would pin this. Minor: the abortable path is new behavior added alongside the retry loop.
🟡 LOW Budget exhaustion preventing retries is untested — tests/campaign/external-optimizer-process.test.ts
The 429 retry loop (external-optimizer-model-proxy.ts:278) exits if tryConsumeRateLimitRetry() returns false (budget exhausted). None of the three new tests exercise this path — they all use generous maxRequests budgets (5 or 10). A test with maxRequests=1 could catch a scenario where the initial call + one retry would exhaust the budget, preventing the 2nd retry. Low severity because the primary recovery and exhaustion paths are covered.
🟡 LOW Budget-exhaustion branch of the retry loop is never exercised — tests/campaign/external-optimizer-process.test.ts
The retry loop exits in three ways: non-429 status (tested), retries>=delay-schedule length (tested, sleeps[2]=30s in the 'exhausting' test), and tryConsumeRateLimitRetry() returning false when maxRequests headroom runs out mid-retry (external-optimizer-model-proxy.ts:278). No test sets a budget tight enough to trigger the third path, so the '429 forwarded immediately without sleeping' behavior when the budget is exhausted is unpinned. Test 1 uses maxRequests=5 for 3 fetches and test 2 uses maxRequests=10 for 4 fetches, leaving headroom in both. Fix: add a case with maxRequests=2 or 3 and always-429 upstream, asserting providerCalls=2/3, sleeps.length=headroom-1, and the final 429 forwarded.
🟡 LOW Exhaustion test bounds-checks only sleeps[2], not sleeps[0]/sleeps[1] — tests/campaign/external-optimizer-process.test.ts
Test asserts sleeps.length===3 and bounds on sleeps[2] only; the recovery test (line 1276-1279) checks both earlier slots. Inconsistent — if the schedule array were reordered the first two delays would not be caught here. Cheap to add expect(sleeps[0]/sleeps[1]) bounds for completeness.
🟡 LOW Exhaustion test does not assert the failure-path ledger receipt — tests/campaign/external-optimizer-process.test.ts
The test instantiates
ledgerbut never asserts on it. After the final 429 the proxy records one receipt with usage undefined -> {inputTokens:0, outputTokens:0, costUnknown:true, usageUnknown:true} charged at maximumCostUsd (external-optimizer-model-proxy.ts:298-303,421-434). Asserting ledger.list() equals [objectContaining({costUnknown:true, usageUnknown:true})] would lock in the failure-accounting contract that the recovery test (line 1280) does for the success path. The third test (non-429) likewise creates but ignoresledger.
🟡 LOW No test for retry termination by request budget (only schedule ceiling exercised) — tests/campaign/external-optimizer-process.test.ts
The exhaustion test uses maxRequests:10 so the [2s,8s,30s] schedule is what stops retries (retries>=RATE_LIMIT_RETRY_DELAYS_MS.length). The implementation's tryConsumeRateLimitRetry() at external-optimizer-model-proxy.ts:132-138 has a second termination branch —
totalRequestCount >= args.budget.maxRequestsreturns false and the 429 is forwarded early. No test covers that branch (e.g. persistent 429 with maxRequests:2 should yield 1 attempt + 0 retries forwarded as 429). This leaves the budget-bounded retry path unverified. Add a case with maxRequests:2 and a permanent 429 to assert providerCalls===2, rateLimitRetries()===1, response.status===429.
tangletools · 2026-08-01T03:40:59Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
❌ 2 Blocking Findings — b2d809d1
Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 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-01T03:40:59Z · immutable trace
Premise check withheld merge —
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — f63f2722
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-01T03:48:07Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
⚪ Value Audit — audit-incomplete
| Verdict | audit-incomplete |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 90.0s (2 bridge agents) |
| Total | 90.0s |
💰 Value — error
value agent produced no parseable value-audit JSON.
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":"queue_timeout","admission":{"active":20,"queued":5,"maxActive":20,"maxQueue":48}}}; opencode/zai-coding-plan/glm-5.2: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":
🎯 Usefulness — error
usefulness agent produced no parseable value-audit JSON.
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":"queue_timeout","admission":{"active":20,"queued":6,"maxActive":20,"maxQueue":48}}}; opencode/kimi-for-coding/k2p7: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":
No PR concerns were produced because the value/usefulness agent pass did not complete. Treat this audit as incomplete, not as approval.
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 —
|
| glm | deepseek | aggregate | |
|---|---|---|---|
| Readiness | 31 | 73 | 31 |
| Confidence | 85 | 85 | 85 |
| Correctness | 31 | 73 | 31 |
| Security | 31 | 73 | 31 |
| Testing | 31 | 73 | 31 |
| Architecture | 31 | 73 | 31 |
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 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Ruff lint regressions: 10 new violations where base passed clean — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
Verified:
ruff checkon the base commit (f85e8f6) of dspy_rlm_bridge.py reports 'All checks passed!'; the head version reports 5 errors (E501 on lines 93, 95, 1179, 1180 for unwrapped prompt strings, and UP037 on line 159 for-> "IncorrectBlock"redundant quotes underfrom __future__ import annotations). The two new test files add 5 more E501s. pyproject.toml sets line-length=100 and selects E/F/W/I/B/UP, so this is a configured gate the new code violates. CI does not currently run ruff
🟠 MEDIUM Span-probe fallback path has zero test coverage — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_probe_codetrace_spans (lines 602-617), _search_verification_spans (620-638), and the omitted-span re-request loop inside _view_spans_paged (lines 665-675) are ~70 lines of non-trivial pagination logic selected when viewTrace exceeds the store budget (fetch_mode='span-probe'). None of the 8 codetrace tests exercise this branch — every test's callback fake returns a valid viewTrace dict, so spans always come from the view-trace path and fetch_mode is always 'view-trace'. Concretely
🟠 MEDIUM Consensus threshold ceil(k/2) is not a majority for even sample counts — a single sample can carry any step — src/analyst/benchmark-public-consensus.ts
threshold = Math.ceil(samples / 2). For k=2 -> 1, k=4 -> 2, k=6 -> 3: in every even case the threshold equals exactly half the panel, so a step present in only ONE sample survives (verified: node confirms k=2..6). The docstring promises a 'step-level majority vote' and 'majority threshold', but for even k this is a union/plurality rule, not majority. Concrete impact on this eval-scoring path: at k=2 the scored output is the union of both samples (zero noise reduction), and consensus.blocks.length===0 (the abstention-floor trigger at benchmark-public-rlm.ts:183) requires BOTH samples to be totally empty, so the fallback almost never fires. The test 'Borrows metadata...' bakes in consensus_threshold:1 for k=2, so this is deliberate, but a reviewer should sign off because it weakens the featu
🟠 MEDIUM Per-case provider spend scales with --rlm-samples (k engine budgets + 1 fallback) with no case-level ceiling or CLI warning — src/analyst/benchmark-public-rlm.ts
Each sample calls runTraceAnalyst -> engine.analyze, which builds a fresh ExternalOptimizerModelProxy with budget.maxCostUsd = config.maxCostUsdPerAnalysis ?? 1 (dspy-rlm-engine.ts:120-121). There is no shared/cumulative case-level ceiling: with --rlm-samples 5 --max-cost-usd-per-analysis 1, one case can spend ~$5 across samples plus ~$1 for the abstention fallback (benchmark-public-rlm.ts:184), vs a user's likely ~$1/case mental model. For a parallel benchmark over many cases this can blow a budget silently. The abstention fallback runner shares the ledger (good for accounting) but adds another full paid call. Recommend either documenting that per-case spend scales with samples in the --rlm-samples help, or enforcing a single case-level ceiling across all samples + fallback.
🟠 MEDIUM 429 retry logic has zero direct test coverage — src/campaign/external-optimizer-model-proxy.ts
The retry loop (lines 259-287) that handles provider 429 with jittered backoff has no unit or integration test exercising it. The sleepImpl injection seam (line 66, 73) was added explicitly for testability but is never exercised by any test file (confirmed: no test references sleepImpl or abortableDelay). The only affected test (dspy-rlm-engine.test.ts:132) expects modelRateLimitRetries: 0, meaning the retry code path is never triggered in the test suite. The following scenarios are untested: (a) 42
🟡 LOW Repair-failure branch (LM exception) is untested — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
The except-Exception branch at lines 1196-1198 (LM raises during the single repair turn -> returns ([], f'{type}: {summary}'), recorded as runtime.format_repair_error) has no test. The three new generic-path tests cover salvage-success, repair-success, and no-repair-needed, but not repair-FAILURE. This is the branch that proves a repair failure does not void the investigation — a load-bearing correctness/fail-loud property. Fix: add a test where the FakeLm.call raises, assert findings==[] and runtime.format_repair_error is populated and format_repair_used is True.
🟡 LOW dropped-diagnostics index field mixes two index spaces — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
_typed_prediction_to_findings (line 687) enumerates the full coerced
blockslist for env-check drops, so dropped[].index is the ORIGINAL reported index. It then calls _build_block_findings(kept, ...) which enumerates thekeptSUBLIST (line 787:for index, block in enumerate(blocks)where blocks==kept), so its dropped[].index is the KEPT-list position. Verified by simulation: blocks=[b0_ok, b1_phantom, b2_no_excerpt] produces dropped=[{index:1,reason:'phantom'},{index:0,reason:'no excer
🟡 LOW setdefault silently tolerates duplicate step IDs in codetrace environment — clients/python/src/agent_eval_rpc/dspy_rlm_bridge.py
At line 577:
step_spans.setdefault(int(match.group(1)), span). Sincestep_spansis a freshly-created dict and each step- span_id is expected unique in a well-formed trace,__setitem__(step_spans[n] = span) would match intent and the codebase's 'fail loud' convention. A duplicate step- is a data-integrity signal the bridge should surface (it would affect downstream block validation results). Changesetdefaultto__setitem__or add a diagnostic log when a duplicate is encountered.
🟡 LOW 2-sample consensus effectively void (threshold=1) — src/analyst/benchmark-public-consensus.ts
ceil(2/2) = 1, so every step appearing in any sample passes. The consensus is a no-op at k=2 — any step with a single vote survives. Not a bug (the CLI validation rejects samples<2 as invalid anyway, and the runner requires samples>1), but callers that pass samples=2 will see no disagreement filtering. Consider documenting that the effective minimum for meaningful consensus is 3.
🟡 LOW Abstention fallback error not surfaced as output error in consensus path — src/analyst/benchmark-public-rlm.ts
When consensus produces no blocks AND the subsequent direct fallback also errors, output.error is undefined (only metadata.abstentionFallbackError is set). This means a case where both the panel and the fallback failed is counted as a successful empty run rather than a failure. The error is preserved in metadata for diagnosis but may lead to the case being treated as a trusted negative in scoring. For the single-sample path, the same pattern applies — the fallback error goes to metadata only.
🟡 LOW Default (samples=1) abstention floor only fires on zero raw findings, not on all-findings-rejected — weaker than the consensus path — src/analyst/benchmark-public-rlm.ts
Single-sample fallback condition is
if (completed.findings.length === 0)— it inspects the RAW engine finding count. If the engine emits findings but the adapter rejects ALL of them (malformed subjects / unresolvable evidence), completed.findings.length > 0 so the fallback never fires, and the case returns adapted.findings=[] with no second opinion. The consensus branch (line 183) uses the post-adaptation, post-voteconsensus.blocks.length === 0and so DOES recover this case. Since samples=1 is the default, the most common path has the weaker floor. The comment scopes this deliberately ('submitted no finding at all'), but a model that hallucinates garba
🟡 LOW Each sampleRun embeds a full engine trajectory; high sample counts inflate artifact size toward maxArtifactBytes — src/analyst/benchmark-public-rlm.ts
sampleRuns entries store
trajectory: completed.trajectoryplus per-sample diagnostics for every sample (lines 156-167). The single-sample path stores one trajectory; the consensus path stores k copies plus the consensus decision record. A DSPy RLM trajectory (iteration history) can be sizable, so at high --rlm-samples this multiplies artifact metadata k-fold and can approach maxArtifactBytes with no explicit per-sample truncation. Not a correctness bug (the artifact writer enforces the byte ceiling), but worth a size budget per sampleRun or dropping raw trajectory from sampleRuns.
🟡 LOW sample>1 branch duplicates single-sample adapter & metadata logic — src/analyst/benchmark-public-rlm.ts
The multi-sample consensus branch (lines 91-211) and single-sample branch (lines 212-298) independently construct rawFindings with makeFinding and call adaptPublicBenchmarkFindings with identical shape. The finding-construction and adapter-call logic is duplicated. Not a correctness bug — the two branches produce identical output shapes for identical inputs — but the duplication risks drift if the shared logic changes.
🟡 LOW Provider Retry-After header on 429 responses is ignored — src/campaign/external-optimizer-model-proxy.ts
The retry loop uses fixed delays (RATE_LIMIT_RETRY_DELAYS_MS) and never inspects the 429 response's
Retry-Afterheader. If the provider says 'retry in 60s', the proxy retries in 2s and gets another 429, wasting a budgeted request slot. Honoring Retry-After (capped to the max delay) would reduce wasted upstream calls and budget consumption.
🟡 LOW requestTimeoutMs now covers entire retry loop including backoff sleeps — src/campaign/external-optimizer-model-proxy.ts
The single
setTimeout(() => controller.abort(), requestTimeoutMs ?? 300_000)at line 238 fires across all retries plus their cumulative backoff (up to ~40s + jitter + fetch time). With the default 300s this is safe, but the budget contract documents requestTimeoutMs as 'Per-provider-request deadline' (external-optimizer-contracts.ts:51), now it is per-admitted-request-including-retries. A user configuring a low custom timeout (e.g. 30s) would see the abort fire mid-backoff on the second retry, silently degrading retry effectiveness. Consider either documenting the new semantics or resetting the timeout per attempt.
🟡 LOW Budget-exhausted-mid-retry path not covered — tests/campaign/external-optimizer-process.test.ts
The implementation's tryConsumeRateLimitRetry (external-optimizer-model-proxy.ts:132-138) can return false when totalRequestCount >= maxRequests mid-retry, causing the 429 to be forwarded WITHOUT sleeping — a distinct code path from the exhaustion-via-schedule case tested at line 1293. No test sets maxRequests low enough to trigger this (test 1 uses maxRequests:5 needing 3; test 2 uses maxRequests:10 needing 4). This is a coverage observation, not a defect in the existing tests. Consider a fourth case with maxRequests:2 and a persistent 429 to assert the budget-bounded early-exit path.
🟡 LOW Budget-exhaustion-during-retry path not explicitly tested — tests/campaign/external-optimizer-process.test.ts
The retry-exhaustion test uses maxRequests=10, so tryConsumeRateLimitRetry never returns false (4 total requests << 10). The path where maxRequests is exhausted mid-retry (tryConsumeRateLimitRetry returns false, 429 forwarded immediately) is only covered by code inspection. Add a test with maxRequests=2 and persistent 429s to verify the 429 is forwarded after exactly 2 total attempts.
tangletools · 2026-08-01T04:08:29Z · 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 — 17 non-blocking findings — f63f2722
Full multi-shot audit completed 5/5 planned shots over 25 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 25 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-01T04:08:29Z · immutable trace
Premise check withheld merge —
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 3db08184
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-01T04:28:01Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 117.2s (2 bridge agents) |
| Total | 117.2s |
💰 Value — sound-with-nits
Replaces a fragile model-emitted string-grammar transport with a typed pydantic signature, plus a measured recovery/retry/consensus stack — all in the codebase's grain; one minor dispatch smell.
- What it does: For CodeTraceBench, the DSPy RLM bridge now uses a typed dspy.Signature whose output is list[IncorrectBlock] (a pydantic model with validated step ranges), instead of asking the model to emit a findings_json string whose block coordinates are encoded inside a subject grammar token. The bridge pre-loads the full trajectory into the REPL as Python variables, validates each typed block against the re
- Goals it achieves: (1) Eliminate the marker-format failure class — the PR body and the committed benchmark README (codetracebench-rlm-glm52-20260731/README.md:33) name 5 zero-scored runs from model format failures and 3 cases lost to provider 429s; the typed signature and the retry fix exactly those. (2) Make the engine reach its best measured configuration by aligning the prompt with the benchmark's own rubric and
- Assessment: Sound and in-grain. The typed signature is the right fix at the right boundary: it moves fragile transport encoding from model responsibility to deterministic code while preserving the downstream subject grammar the TypeScript adapter already parses (benchmark-public-adapters.ts:162-163, 246-292), so no downstream contract churn. Both paths legitimately coexist because AgentRx (single root-cause s
- Better / existing approach: Searched for existing consensus/vote/salvage/retry/backoff primitives across src/, clients/python/, and tests/. The only adjacent code is agentRxConsensus (benchmark-dataset-agentrx.ts:334 — single-step/category voting, not block reassembly) and withJudgeRetry (src/judge-retry.ts — LLM-judge-client layer, not the optimizer subprocess proxy). Neither covers this surface. The typed-signature approac
- 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
This PR hardens the recursive DSPy-RLM analyst — the default scored analyst — with a typed pydantic block signature, a layered recovery stack, and a dormant-by-default consensus knob, all wired through the existing benchmark command and shared expansion/evidence pipeline; nothing is dead or competin
- Integration: Fully reachable.
createPublicBenchmarkRlmRunneris the default candidate runner selected insrc/analyst/benchmark-command.ts:207(--analyst dspy-rlmis the default at line 493). The typed signature is selected transparently inside the Python bridge via theincorrect-steps-token that is always present in the CodeTraceBench instructions (dspy_rlm_bridge.py:345,456-457), so no caller chang - Fit with existing patterns: Fits the codebase grain precisely. Typed blocks reuse the exact
CodeTraceFailureBlockinterface and feed the sameexpandCodeTraceFailureBlocksexpansion +resolveAssistantStepEvidenceevidence pipeline the direct runner uses (benchmark-public-adapters.ts:301), so scoring is byte-identical downstream of the adapter regardless of which runner produced the blocks. The Python-built finding sub - Real-world viability: Robust well past the happy path. The bridge handles oversized traces via a deterministic
step-<n>span-probe fallback (dspy_rlm_bridge.py:602-617) plus content-addressed verification-span search; environment materialization and excerpt reads go through the same authenticated tool callback so they count against the Node-side tool budget (dspy_rlm_bridge.py:378-380). Typed blocks get pydantic - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Signature dispatch keys off a substring token in the instructions rather than an explicit wire field [better-architecture] ``
_uses_codetrace_contract (dspy_rlm_bridge.py:456-457) selects the typed signature by testing whether the literal 'incorrect-steps-' appears anywhere in the instructions string. The comment (lines 71-73) says this is deliberate to avoid a wire-protocol change, and the token is currently stable. But it is implicit: a prompt edit that rephrases the subject grammar would silently flip the transport, and a reader cannot tell from the analyze-input schema which signature will run. An explicit field (e
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.
Premise check withheld merge —
|
2 participants
What this is
Eight commits that take the recursive DSPy-RLM trace analyst from a shipped-but-tied configuration (0.3644 scored micro F1 on CodeTraceBench, equal to the retired one-shot at 5.5× cost) to the best measured configuration of this engine, plus the instrument that keeps future claims honest.
Commits, grouped
Correctness (score-independent, each with recovery + fail-loud tests):
64e724d— no paid finding is silently lost: final-turn salvage + one flagged repair turn, out-of-block citation trimmed not fatal, oversized block dropped not case-voiding, bounded 429 retry with jitter, abstention floor (empty RLM → one direct call, marked).eca36d6— typed task-shaped RLM signature: the whole trajectory loads into the REPL as variables (dspy.RLM's native input mechanism); output is a typedlist[IncorrectBlock]via the SUBMIT path — the marker-format failure class (5 zero-scored runs in the shipped benchmark) is structurally gone; subjects/URIs/excerpts are built by code from data the bridge holds.4611f8b—--rlm-samples kconsensus voting (default 1 = current behavior). Measured at k=3: REJECTED as an accuracy lever (oracle sample-selection ≤ single-run value at 3× cost) — the flag ships for reproducibility of that null result, dormant by default.Methodology (prompt, paper-grounded):
0730893/682ad81— the CodeTrace task prompt now follows the benchmark's own annotation protocol (arXiv 2604.11641: backward chain tracing from failure evidence; consequence and botched-repair steps sit INSIDE blocks — the old prompt contradicted the rubric on exactly the boundary the analyst kept missing). De-suppression measured essential: caution-style priors cost −14.6pp.8a570a1/b2d809d— blocks extend through full cascades on per-step evidence; numeric shape priors deleted after measurement (width prior: −10.2pp on narrow-gold, +5.3pp on wide-gold — priors on shape statistics overfit splits; evidence-conditioned rules transfer).Numbers (glm-5.2, pinned 32-case split + 32-case disjoint holdout, unchanged official scorer, 2 reps)
Honest read: the dev micro gain (+8.9pp) did not transfer to the first holdout (−4.0pp micro, +5.8pp macro; CI [−0.285, +0.172]) — the holdout's wide-cascade gold exposed under-extension, which
8a570a1/b2d809dthen targeted (the exposing case: 0 → 0.762). The W2 A/B on both splits is running; result will be appended here before merge. Macro (per-case coverage) improved on BOTH splits — the analyst scores nonzero on far more cases. A second sealed holdout (cascade-heavy, 188 gold steps) exists for the next certification; no accuracy claim in this PR exceeds what the tables show.Verification
Full suite 4614 pass / 3 skip; python bridge tests 34/34 (both files); typecheck, build, digest checker green on every commit; live smokes recorded per arm. Evidence ledger:
.evolve/experiments.jsonlrounds 2026-07-31 → 2026-08-01.