fix(campaign): preserve unknown cost provenance - #530
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 75c25ce4
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-03T06:28:18Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 224.0s (2 bridge agents) |
| Total | 224.0s |
💰 Value — sound
Replaces the coarse, re-derived costEstimated boolean on campaign cells with the ledger's authoritative CostProvenance, carrying full token usage and failing closed on stale caches — coherent and squarely in the codebase's grain.
- What it does: Makes a campaign cell carry the same cost representation the rest of the package already uses: it drops the ambiguous
costEstimated?: booleanand adds a requiredcostProvenance: CostProvenance(observed | estimated | uncaptured) sourced directly fromCostLedgerSummary.costProvenance(src/cost-ledger.ts:141,576). It propagatesreasoningandcacheWritetokens intoCampaignTokenUsage(alr - Goals it achieves: Stop encoding unknown spend as zero. Today a failed/expired provider call sets
costUnknown: trueon its receipt; the oldcostEstimatedheuristic only flagged receipts missingactualCostUsd, so an uncaptured call looked like a $0 observed call and silently polluted campaign aggregates, the measurement digest (provenance.ts), andProfileSummary.totalCostUsd. After this change, uncaptured s - Assessment: Good change on its merits. It aligns the campaign cell with the canonical
RunRecordshape (src/run-record.ts:178-180 already hadcostUsd: number | null+costProvenance) and reuses the substrateCostProvenancetype rather than inventing a new one — exactly the substrate-first layering this repo's CLAUDE.md mandates. It removes a real fallback (costEstimated: cell.costEstimated ?? nullin - Better / existing approach: none — this is the right approach. Searched src/cost-ledger.ts, src/run-record.ts, src/campaign/{types,run-campaign,run-record,provenance}.ts and the presets layer. The authoritative
CostProvenancediscriminated union already lives in cost-ledger.ts:8-11 and is already computed byCostLedgerSummary; the canonicalRunRecordalready usedcostUsd: number | null+costProvenance. The change - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
Replaces the campaign layer's insufficient costEstimated boolean with the CostProvenance discriminated union already used everywhere else, fixing a real silent-zero class and surfacing ledger data that was already computed but dropped at the cell boundary.
- Integration: Fully reachable and correctly wired. The new costProvenance field on CampaignCellResult is populated in executeCell (run-campaign.ts:654) from agentCost.costProvenance, which already existed on CostLedgerSummary (cost-ledger.ts:141). It flows through campaignCellToRunRecord (run-record.ts:63-68) into the canonical RunRecord, through campaignMeasurementDigest (provenance.ts:577), and up into Profil
- Fit with existing patterns: Excellent — it eliminates the last costEstimated boolean in the repo (grep confirms zero references remain) and adopts the CostProvenance union that run-record.ts, contract/, analyst/, multishot/, and trace-analyst/ already use. The raw metric flags (cost_observed/cost_estimated/cost_uncaptured at run-record.ts:76-78) exactly mirror code-agent-session.ts:318-320. The campaignCellCostProvenance val
- Real-world viability: Holds up on error paths. The PR's headline scenario — a failed router call with costUnknown:true — is tested concretely (run-profile-matrix.test.ts: 'preserves known usage while leaving failed-call cost uncaptured'): token usage (input/cached/cacheWrite/output/reasoning) is retained while costUsd is null and provenance is uncaptured, and the profile total correctly becomes null rather than a fake
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Stale-schema cache miss is reported as 'corrupt' [robustness] ``
readCachedCell (run-campaign.ts:1043-1046) wraps campaignCellCostProvenance in a bare catch that returns reason:'corrupt' for any throw, including a legitimate stale-schema cell missing costProvenance. Both paths correctly rerun the cell, so there is no correctness impact, but a cache-debugging log would conflate 'JSON malformed' with 'predates this field'. Consider validating costProvenance before the try, or returning a distinct reason like 'stale-schema'. Does not gate shipping.
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-flash | aggregate | |
|---|---|---|---|
| Readiness | 80 | 77 | 77 |
| Confidence | 75 | 75 | 75 |
| Correctness | 80 | 77 | 77 |
| Security | 80 | 77 | 77 |
| Testing | 80 | 77 | 77 |
| Architecture | 80 | 77 | 77 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟡 LOW Public API break: ProfileSummary.totalCostUsd changed from number to number | null — src/campaign/presets/run-profile-matrix.ts
totalCostUsd is now
number | null(null when costProvenance.kind==='uncaptured'), a deliberate correctness fix so a partial subtotal is never presented as a total. But ProfileSummary is an exported substrate type on the 0.142.x line, and every consumer that read.totalCostUsdas a number is now compile-broken and must null-guard. Worth calling out as a breaking change in the release/PR description (and confirming no consumer package reads it unguarded) rather than landing it as a silent patch-line change.
🟡 LOW campaignMeasurementDigest schema string not version-bumped despite field substitution — src/campaign/provenance.ts
The digest input changed from
costEstimated: cell.costEstimated ?? null(boolean|null) tocostProvenance: cell.costProvenance({kind,usd}|{kind,usd:null}). This changes the digest value for identical campaigns. The schema marker remains'tangle.campaign-measurement'with no version suffix. Any persisted loop-provenance record carrying a campaignDigest computed by the old algorithm would failverifyLoopProvenanceRecordre-verification under new code. Acceptable as a clean break (the old field no longer exists on the type, so mixed-version interop is already impossible), but the schema string could carry a version (e.g.tangle.campaign-measurement.v2) to make the break self-documenting.
🟡 LOW Pre-PR resumability caches are silently invalidated, causing a full silent re-dispatch (re-spend) — src/campaign/run-campaign.ts
campaignCellCostProvenance(cached) throws for any cached-result.json written before this change (they have no costProvenance field), and the catch at line 1045 downgrades it to {status:'miss',reason:'corrupt'}, so executeCell re-dispatches the cell and pays for it again. This is intended and covered by 'reruns a cached cell that predates explicit cost provenance' (tests/campaign/run-campaign.test.ts:262), and it is correct (a pre-provenance $0 cell is exactly the fabricated-zero bug being fixed). Nit: an operator resuming a large campaign gets zero warning that every prior cell will re-run and re-bill; a one-line notice (console.warn when the miss reason is 'corr
🟡 LOW Validation call discards return value — side-effect-only invocation — src/campaign/run-campaign.ts
campaignCellCostProvenance(cached)is called purely for its throw-on-invalid side effect; its return value is discarded. This is a defensible early-fail (turns an old/malformed cache entry into a 'miss' rather than crashing later inside campaignCellToRunRecord), but a reader must infer intent from the function name alone. Consider an explicitassertCampaignCellCostProvenance(cached)void-returning alias, or an inline comment naming the invariant being checked, to signal that the call exists to reject stale cache shapes.
🟡 LOW Injected defaultCostUsd produces two contradictory cost figures in the same raw record — src/campaign/run-record.ts
When cellCostProvenance.kind==='uncaptured' and options.defaultCostUsd !== undefined, the record becomes estimated with costUsd=default (lines 64-68, 74) while line 75 STILL emits cost_known_subtotal_usd: cell.costUsd, and lines 76-78 report cost_uncaptured:0 / cost_estimated:1. outcome.raw therefore carries cost_usd=default AND cost_known_subtotal_usd= (possibly very different), and the 'uncapture
🟡 LOW Cache regression test is white-box coupled to internal storage layout — tests/campaign/run-campaign.test.ts
The test hardcodes '/cost-provenance-cache/a_0/cached-result.json', duplicating run-campaign.ts's cell-dir sanitization (cellId 'a:0' → 'a_0', run-campaign.ts:378) and the 'cached-result.json' filename (run-campaign.ts:390). If either is renamed, storage.read(cachePath) returns undefined and the test fails with a confusing JSON.parse TypeError ('Cannot convert undefined or null to object') instead of a meaningful assertion. The in-memory storage makes the path mutation harmless, so this is a maintainability nit, not a correctness bug — but the test would be more robust reading the storage directory listing (e.g., storage.list('/cost-provenance-cache/a_0')) rather than the literal path.
🟡 LOW Legacy campaign caches are silently invalidated on upgrade, triggering a full paid re-run — tests/campaign/run-campaign.test.ts
The new test pins that a cached cell lacking costProvenance is re-dispatched (dispatchCount 1→2). The source change that makes this true is readCachedCell calling campaignCellCostProvenance(cached) inside a try/catch (run-campaign.ts:1043-1046), which maps every legacy cache row (written by versions with only costEstimated) to {status:'miss',reason:'corrupt'}. Because cached-result.json has no schema version and the manifest hash does not include the cost-schema version, upgrading agent-eval invalidates every pre-existing campaign cache in one go — each cell re-executes against the real LLM provider. That is a deliberate fail-loud choice (better than serving unprovenanced cost), and the test documents it, but nothing surfaces the invalidation to the operator. Recommend logging the count/re
🟡 LOW campaignCellCostProvenance validator error branches lack direct unit tests — tests/campaign/run-campaign.test.ts
The new campaignCellCostProvenance validator (src/campaign/run-record.ts:143-170) has five throw branches: invalid costUsd, missing provenance, invalid uncaptured.usd, invalid observed/estimated kind or usd, and costUsd≠provenance.usd inconsistency. The shot's cache-migration test (run-campaign.test.ts:262) only exercises the 'missing provenance' branch indirectly (via readCachedCell's swallowed catch). The remaining four — including the consistency check that prevents a cell advertising costUsd=5 with provenance.usd=3 from silently passing — have no test in any of the four files. Import campaignCellCostProvenance directly and assert each throw by message. Impact: a future edit weakening the consistency check would not be caught.
🟡 LOW defaultCostUsd upgrade branch (uncaptured→estimated) is untested — tests/campaign/run-profile-matrix.test.ts
campaignCellToRunRecord (src/campaign/run-record.ts:64-67) upgrades an uncaptured cell to {kind:'estimated', usd: options.defaultCostUsd} when defaultCostUsd is provided. The new uncaptured test in run-profile-matrix.test.ts:342 deliberately omits defaultCostUsd, so this upgrade path — which changes both record.costUsd (null→number) and record.costProvenance (uncaptured→estimated) and gates the cost_known_subtotal_usd raw field — has zero coverage across all four shot files. Add a sibling test that passes defaultCostUsd to runProfileMatrix (or calls campaignCellToRunRecord directly) and asserts the upgrade produces a valid estimated record. Impact: a regression in the fallback would silently re-derive wrong cost labels. Low because validateRunRecord still catches structurally invalid outpu
🟡 LOW Fixture update adds no assertion that costProvenance or new cost_* raw flags flow through to RunRecord — tests/rl-adapters.test.ts
The diff adds
costProvenance: { kind: 'observed', usd: <n> }to all 4 cells but the existing assertions (lines 88-146) never checkrec.costProvenance,rec.costUsd, or the newoutcome.rawkeys (cost_observed,cost_estimated,cost_uncaptured,cost_known_subtotal_usd) introduced insrc/campaign/run-record.ts:75-78. All four fixtures also use the same kind (observed), so the adapter'suncaptured→null-costUsd branch and thedefaultCostUsdfallback (run-record.ts:64-67) are not hit from this file. Impact: low — siblingtests/campaign/run-profile-matrix.test.tsin this PR covers all three branches including the uncaptured path with full raw-fla
🟡 LOW Fixtures updated but no assertions on cost-provenance mapping — tests/rl-adapters.test.ts
The fixture now carries
costProvenance({kind:'observed'}) on all 4 cells, andcampaignCellToRunRecord(src/campaign/run-record.ts:60-97) now maps that into RunRecord.costProvenance plus raw flags cost_observed/cost_estimated/cost_uncaptured, yet this file asserts none of them. The 'uncaptured' branch (which emitscost_known_subtotal_usdand nulls costUsd — the headline behavior of commit 'fix(campaign): preserve unknown cost provenance') is entirely unexercised here. Mitigated: dedicated assertions live in other PR files (tests/campaign/run-profile-matrix.test.ts:148,221,264,274; run-campaign.test.ts:278-289), and all 7 tests here pass. Fix: add an expect on rec[0].costProvenance (e.g. toEqual({kind:'observed',usd:0.01})) and a fixture cell with {kind:'uncaptured',usd:null} to lock
tangletools · 2026-08-03T06:43:49Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 11 non-blocking findings — 75c25ce4
Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 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-03T06:43:49Z · immutable trace
✅ No Blockers —
|
| glm | deepseek | deepseek-flash | aggregate | |
|---|---|---|---|---|
| Readiness | 80 | 92 | 51 | 51 |
| Confidence | 75 | 75 | 75 | 75 |
| Correctness | 80 | 92 | 51 | 51 |
| Security | 80 | 92 | 51 | 51 |
| Testing | 80 | 92 | 51 | 51 |
| Architecture | 80 | 92 | 51 | 51 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM All pre-PR cached cells invalidate as 'corrupt' and silently re-run (spend on resume) — src/campaign/run-campaign.ts
campaignCellCostProvenance(cached)(run-record.ts:150) throws 'has no costProvenance' for any cached-result.json written before this release (old cells carry onlycostEstimated), and the catch at run-campaign.ts:1045 converts it tomiss 'corrupt'. Every pre-existing cache entry is therefore re-dispatched after upgrade — real LLM spend on a resumable run — and planCampaignRun reports these asreason:'corrupt', masking the actual cause (schema change, not corruption). The new test 'reruns a cached cell that predates explicit cost provenance' proves this is intentional, but there is no one-time migration or reason label distinguishing 'old schema' from 'genuinely corrupt'. Recommendation: distinguish an explicit 'legacy-schema' miss (e.g. check forcostEstimatedpresence) so operato
🟠 MEDIUM Breaking public API change without deprecation or release-note signal — src/campaign/types.ts
costEstimated?: booleanis replaced by requiredcostProvenance: CostProvenance, andProfileSummary.totalCostUsdchangesnumber→number|null(run-profile-matrix.ts:168). Both types are exported viasrc/campaign/index.ts. Any out-of-repo producer of CampaignCellResult (or consumer reading ProfileSummary) now fails typecheck/breaks at runtime with no migration path. Within-repo producers are all updated (verified by green typecheck). Acceptable for 0.x, but flag in the release notes and consider acampaignCellCostProvenance-style helper exported for external producers.
🟡 LOW Profile-level cost surfaces can disagree when only judge cost is uncaptured — src/campaign/presets/run-profile-matrix.ts
byProfile.totalCostUsdderives fromcampaign.aggregates.cost.costProvenance(ALL channels including judge receipts), so a single unknown-cost judge receipt nulls the profile total whilebyProfile.integrity.totalCostUsd(sum of record agent costs) remains nonzero. This is the documented intent ('null when any call's cost was not captured'), but a consumer comparing the two surfaces on the same profile sees a divergence with no code path explaining the channel split. Worth a comment or a per-profile breakdown noting the scope (agents vs agents+judges).
🟡 LOW ProfileSummary.totalCostUsd changed from number to number | null (breaking public-API type) — src/campaign/presets/run-profile-matrix.ts
ProfileSummary is exported from src/campaign/index.ts:231. The field went from
totalCostUsd: numbertototalCostUsd: number | nulland the value now returns null when campaign.aggregates.cost.costProvenance.kind === 'uncaptured' (previously it returned the known subtotal). The change is semantically correct — reporting a partial receipt sum as a 'total' was misleading — but any external consumer that calls .toFixed() or does arithmetic on this field will break at compile time (TS) or runtime (JS). No internal consumer breaks (verified: the only internal reader of ProfileSummary.totalCostUsd is the matrix return itself; multishot/matrix.ts:338,347 read a different MatrixProfileSummary type). Low severity because it is intentional and the package is pre-1.0 (0.142.2), but worth flagging
🟡 LOW campaignMeasurementDigest field swap is not schema-versioned — src/campaign/provenance.ts
The digest field
costEstimated(with?? null) is replaced by thecostProvenanceobject.canonicalDigestJSON-round-trips, so the same underlying run content now hashes differently across the upgrade boundary; durable loop-provenance records (provenance.ts:365/450/455) embedding this digest are not comparable pre/post this PR.verifyLoopProvenanceRecordonly self-checks the embedded digest, so no hard failure — but theschema: 'tangle.campaign-measurement'tag is unchanged while the payload shape changed, which defeats schema-versioned content addressing. Bump the schema tag.
🟡 LOW 'corrupt' reason swallows provenance validation errors in cache planning — src/campaign/run-campaign.ts
The
try/catchin readCachedCell collapses all failures (JSON parse error, manifest mismatch, cell mismatch, provenance validation) into a singlemiss 'corrupt'. With the new validation this reason now also fires for stale-schema cells, so planCampaignRun/runCampaign cannot distinguish a truncated file from a version-skewed one. Consider re-throwing or tagging the specific validation error for observability.
🟡 LOW Asymmetric undefined guards in CampaignTokenUsage assembly — src/campaign/run-campaign.ts
The three token spreads use inconsistent predicates:
agentCost.cachedTokens > 0(no undefined guard) vsagentCost.reasoningTokens !== undefined && > 0andagentCost.cacheWriteTokens !== undefined && > 0. CostLedgerSummary declares cachedTokens as requirednumberand the other two as optional, but summary() at cost-ledger.ts:522-524,572-574 initializes and returns all three unconditionally, so the guards are effectively equivalent today. Harmless, but the asymmetry is a future-confusion trap if another summary producer ever omits cachedTokens. Nit only.
🟡 LOW Fresh cells are not validated before cache write — asymmetric with read path — src/campaign/run-campaign.ts
storage.write(cachePath, JSON.stringify(cell))writes the cell without first passing it throughcampaignCellCostProvenance, whilereadCachedCellvalidates on every read. An internally-inconsistent cell (e.g. provenance.kind='observed' with usd≠costUsd) is persisted and only detected on the NEXT run, where it silently invalidates as 'corrupt' and re-runs. Validate the cell (or its cost fields) before persisting so a defect surfaces at production time, not at resume time.
🟡 LOW Stale cache entries (pre-PR schema with costEstimated, no costProvenance) silently become 'corrupt' misses — src/campaign/run-campaign.ts
readCachedCell now calls campaignCellCostProvenance(cached) inside its try/catch. A cache file written by the previous version carries
costEstimated?: booleanand nocostProvenance, so the validator throws 'has no costProvenance' and the catch returns {status:'miss', reason:'corrupt'}. This is a safe migration (the cell re-runs rather than loading inconsistent state — good), but for users with large on-disk caches every cached cell silently invalidates on first run after upgrade, with no log distinguishing 'schema-old' from actual corruption. Optional: detect the missing-field case and emit a distinct miss reason like 'schema-stale' so the re-run cost is attributable.
🟡 LOW cache validation error swallowed into generic 'corrupt' miss reason — src/campaign/run-campaign.ts
When campaignCellCostProvenance(cached) throws (line 1043) — because a cached cell lacks costProvenance, has inconsistent values, or has invalid fields — the catch block returns { status: 'miss', reason: 'corrupt' }. The specific reason (e.g. 'has no costProvenance', 'costUsd inconsistent with costProvenance') is lost. This was a pre-existing pattern before this PR (the catch-all already existed), but the PR adds campaignCellCostProvenance as a new throw source inside the try block without adding granularity. Consider returning distinct miss reasons from the validation failures so operators diagnosing cache invalidation can distinguish corrupt JSON from schema-mi
🟡 LOW campaignCellCostProvenance validator has no direct unit tests for its error paths — src/campaign/run-record.ts
The new exported validator encodes at least 5 distinct throw branches: (1) non-finite/negative costUsd, (2) missing/non-object costProvenance, (3) uncaptured with usd!==null, (4) kind not in {observed,estimated} or usd non-finite/negative, (5) provenance.usd !== cell.costUsd mismatch. A repo-wide grep for
campaignCellCostProvenanceacross *.test.ts returns zero direct test hits — it is only exercised indirectly when campaignCellToRunRecord / readCachedCell happen to feed it valid data. Because this function is now the cost-integrity gate at the cache and record-projection boundaries (the exact place a silent-zero or stale-cache bug would slip through), its throw paths deserve direct coverage: each of the 5 invalid inputs should produce the documented error. Fix: add a run-record.test.ts
🟡 LOW Empty-ledger cell is asserted as 'observed' $0, coupling the test to a vacuous summary — tests/campaign/run-campaign.test.ts
The dispatch never calls ctx.cost.runPaidCall, so the ledger has zero receipts and summary() returns {kind:'observed', usd:0} via vacuous receipts.every(...) (cost-ledger.ts:579-580). The test enshrines 'no cost calls == observed $0' while the PR's own types.ts:581 comment says 'Unknown cost is never encoded as zero'. Only a receipt explicitly marked costUnknown yields 'uncaptured'; a cell that failed before any paid call is indistinguishable from a real $0 run. Test uses expectUsage:'off', so nothing else flags it. Defensible, but either document the choice in the test or add an assertion that no-receipt failed cells surface as uncaptured.
🟡 LOW cost_known_subtotal_usd only exercised at $0 — its distinguishing value untested — tests/campaign/run-profile-matrix.test.ts
The field's purpose (run-record.ts:75) is to preserve the KNOWN subtotal of an uncaptured cell, i.e. mixed receipts: one known $X + one costUnknown. The new test's only receipt is costUnknown (costUsd forced to 0 by buildReceipt, cost-ledger.ts:1032-1038), so cost_known_subtotal_usd is always asserted as 0. A mixed known/unknown cell would assert subtotal = known sum while cost_usd stays absent and byProfile.totalCostUsd is null — the exact scenario the field exists for. Add one mixed-receipt case.
🟡 LOW defaultCostUsd override branch (uncaptured -> estimated) has zero test coverage — tests/campaign/run-profile-matrix.test.ts
run-record.ts:64-67 adds a new branch: when a cell's provenance is 'uncaptured' and options.defaultCostUsd is set, the record becomes {kind:'estimated', usd: defaultCostUsd} while cost_known_subtotal_usd still carries cell.costUsd. Grep shows defaultCostUsd is referenced only in src (run-record-adapters.ts, run-record.ts) and in NO test — this PR's changed tests (including the new uncaptured test) never exercise the override or its interaction with cost_uncaptured:1 vs cost_estimated:1 flags. This is the only new provenance transformation left untested by the PR.
🟡 LOW No assertion that costProvenance propagates through campaignToRunRecords — tests/rl-adapters.test.ts
The test adds costProvenance to every fixture cell but never asserts the field reaches the produced RunRecord. campaignToRunRecords is a thin map over campaignCellToRunRecord; a single
expect(recs[0]!.costProvenance).toEqual({ kind: 'observed', usd: 0.01 })in the firstit(...)block (around line 101, next to the existing tokenUsage assertion) would pin the adapter contract at this seam. Low impact because src/campaign/run-record.ts emits costProvenance unconditionally and run-profile-matrix.test.ts already asserts the end-to-end propagation — this is defense-in-depth, not a gap.
🟡 LOW costProvenance added to fixtures but never asserted in adapter output — tests/rl-adapters.test.ts
The diff adds costProvenance to all 4 campaign cells (lines 31, 47, 63, 78) but no expectation in either test verifies that campaignToRunRecords propagates it. campaignCellToRunRecord (src/campaign/run-record.ts:63-103) now derives record-level costUsd, costProvenance, and raw flags cost_usd / cost_observed / cost_estimated / cost_uncaptured / cost_known_subtotal_usd from these fixtures, yet the assertions stop at tokenUsage/terminalOutcome. E.g. first cell costUsd 0.01 observed should yield rec.costUsd===0.01, rec.costProvenance==={kind:'observed',usd:0.01}, raw.cost_observed===1, raw.cost_usd===0.01 — none are checked. A regression in cost propagation through this a
tangletools · 2026-08-03T06:51:07Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 16 non-blocking findings — 75c25ce4
Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 12 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-03T06:51:07Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — a899ae8e
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-03T07:08:21Z
✅ No Blockers —
|
| glm | deepseek | deepseek-flash | aggregate | |
|---|---|---|---|---|
| Readiness | 83 | 89 | 57 | 57 |
| Confidence | 75 | 75 | 75 | 75 |
| Correctness | 83 | 89 | 57 | 57 |
| Security | 83 | 89 | 57 | 57 |
| Testing | 83 | 89 | 57 | 57 |
| Architecture | 83 | 89 | 57 | 57 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 13 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Breaking published type changes: totalCostUsd nullability and removal of costEstimated — src/campaign/presets/run-profile-matrix.ts
ProfileSummary.totalCostUsdchanges fromnumbertonumber | null(line 168) andCampaignCellResult.costEstimated?: booleanis removed in favor of mandatorycostProvenance(src/campaign/types.ts:581). Both types are exported from src/campaign/index.ts and shipped in@tangle-network/agent-eval. Downstream consumers (agent-runtime, agent-knowledge, hosted/reporting code, example scripts) that read.totalCostUsd.toFixed(...)orcell.costEstimatedwill fail to compile in TS or throw at runtime in JS (null.toFixed). No in-repo consumer of the old shape was missed (grep confirms zero remainingcostEstimatedrefs), and RunRecord.costUsd was
🟠 MEDIUM New mandatory costProvenance silently invalidates all pre-existing resumability caches — src/campaign/run-campaign.ts
readCachedCellnow callscampaignCellCostProvenance(cached), which throws on any cell lackingcostProvenance(the pre-0.142.2costEstimatedshape). The throw is swallowed by the surrounding try/catch and downgraded to{status:'miss', reason:'corrupt'}, so every cached cell written by a previous version re-dispatches. Confirmed by the new testreruns a cached cell that predates explicit cost provenance(dispatchCount 1→2). For resumable loops/improvement campaigns that reuse a runDir across runs, upgrading silently re-executes all cached cells and re-incurrs LLM spend with no warning (reason 'corrupt' is indistinguishable from a genuine corruption). Deliberate and tested, but this is a soft cache-schema migration: recommend release-note visibility and/or an explicit log of the m
🟡 LOW Whole-profile total nulls when any single receipt was unknown-cost — src/campaign/presets/run-profile-matrix.ts
totalCostUsdis derived fromcampaign.aggregates.cost.costProvenance, which is the run-wide ledger summary over{tags:{runDir}}— i.e. ALL cells plus judge-channel receipts. A single unknown-cost receipt anywhere in the profile flipsbyProfile[].totalCostUsdto null even when 49/50 cells were fully observed, and the numeric subtotal remains visible oncampaign.aggregates.cost.totalCostUsd(inconsistent surfaces). Conservative and intentional per the new tests, but consider documenting the whole-or-nothing semantics onProfileSummaryand having consumers prefercostProvenance+aggregates.cost.totalCostUsdfor the partial floor.
🟡 LOW campaignMeasurementDigest input shape changed — old digests not comparable to new — src/campaign/provenance.ts
The digest now hashes
costProvenance(object) instead ofcostEstimated(boolean|null). Two campaigns with identical underlying data but different code versions will produce different digests. This is expected for a schema change and affects only cross-version digest comparisons. No action needed beyond awareness — the digest is a content hash, not a compatibility boundary.
🟡 LOW Cached cells from pre-PR versions are silently invalidated as 'corrupt' — src/campaign/run-campaign.ts
When
campaignCellCostProvenance(cached)is called on a cache file written by a pre-PR version (hascostEstimated, nocostProvenance), it throws → caught by the try/catch → returns {status:'miss', reason:'corrupt'}. This is the correct safe behavior (cells re-run), but ALL existing campaign caches are silently invalidated with no logging or migration path. For long-running campaigns with resume, this means a full re-run after upgrading. Not a bug — the schema changed and re-running is the conservative choice — but worth documenting in release notes so operators aren't surprised by cache misses after upgrade.
🟡 LOW Mutually-exclusive cost flags erase the partially-observed signal at cell level — src/campaign/run-record.ts
cost_observed/cost_estimated/cost_uncapturedare emitted as one-hot flags (0/1 each). A cell with mixed receipts (one provider-billed, one unknown-cost) reportscost_uncaptured: 1andcost_observed: 0even though part of the spend WAS observed; the partial-observation detail lives only incost_known_subtotal_usd. Not a bug — the docstring says exactly this — but downstream reporters that read the one-hot flags will conclude the cell is entirely unobserved. Consider a note on the raw-key contract or acost_known_subtotal_usdpresence check for such consumers.
🟡 LOW defaultCostUsd passes unvalidated NaN/Infinity into costProvenance.usd before downstream catch — src/campaign/run-record.ts
When
cellCostProvenance.kind === 'uncaptured'andoptions.defaultCostUsdis set, the value is used directly ascostProvenance.usd. IfdefaultCostUsdis NaN or Infinity,validateRunRecord(called at line 115) catches it viaexpectNonNegativeNumber→expectFiniteNumber, so no silent corruption. The error message does not namedefaultCostUsdas the source however, making diagnosis harder. Adding aNumber.isFinite(defaultCostUsd)guard before the override would fail earlier with a clearer message.
🟡 LOW raw cost_estimated field semantics changed for uncaptured cells — src/campaign/run-record.ts
Old code:
raw.cost_estimated = cell.costEstimated ? 1 : 0(any receipt estimated?). New code:raw.cost_estimated = costProvenance.kind === 'estimated' ? 1 : 0(overall provenance estimated?). For uncaptured cells, old could produce 1, new always produces 0 (thecost_uncapturedflag takes over). Downstream readingraw.cost_estimateddirectly (bypassingRunRecord.costProvenance) would see shifted values. The newcost_observed/cost_uncapturedflags provide strictly more information;costProvenanceon RunRecord is authoritative.
🟡 LOW tokens_per_dollar/cost_per_quality now use the default estimate instead of the measured subtotal for uncaptured cells — src/campaign/run-record.ts
For an uncaptured cell with
options.defaultCostUsdset (thecampaignToRunRecords/rl-adapters path),costUsdbecomesdefaultCostUsdandtokens_per_dollar(line 100) andcost_per_quality(line 103) are computed against that default rather than the previously-used known subtotal (cell.costUsd). When the default is a rough per-call guess, these derived ratios silently change value and provenance (estimated total ÷ real tokens). The new test pinscostUsd=5, cost_known_subtotal_usd=2, confirming the total swap is deliberate, but the
🟡 LOW cost_known_subtotal_usd: 0 can read as 'free' without the cost_uncaptured flag — tests/campaign/run-profile-matrix.test.ts
The test asserts raw.cost_known_subtotal_usd === 0 for a failed call whose only receipt is unknown-cost. This is correct: the ledger books unknown-cost receipts at costUsd 0 (cost-ledger.ts:1032-1033), so the 'known subtotal' is 0 even though the run consumed ~9.7M tokens. The field name 'known_subtotal' plus the 0 value can be misread downstream as 'measured zero' without the sibling cost_uncaptured: 1 flag. Consider a comment or a non-zero-subtotal variant (e.g., one costed receipt + one unknown-cost receipt) to lock in that cost_known_subtotal_usd excludes unknown-cost calls; current single-case coverage leaves the semantics implicit.
🟡 LOW Rejection cases bypass campaignCellToRunRecord, leaving the validator-call coupling unguarded — tests/campaign/run-record-cost-provenance.test.ts
The it.each negative cases (lines 55-88) call campaignCellCostProvenance directly. The positive test (lines 11-40) proves campaignCellToRunRecord invokes the validator today (run-record.ts:63), but no test asserts campaignCellToRunRecord ITSELF rejects an invalid cell. A future refactor that drops the validator call from the projection would pass every test here yet silently accept missing/inconsistent provenance on records. Fix: assert each invalid cell also rejects through campaignCellToRu
🟡 LOW Validator rejection paths not exercised through campaignCellToRunRecord — tests/campaign/run-record-cost-provenance.test.ts
The it.each negative cases call campaignCellCostProvenance directly. campaignCellToRunRecord delegates to it (run-record.ts:63), so the throw propagation through the record builder is inferred, not proven by a test. Low impact: the validator is the contract boundary and is tested; the delegation is a single literal call. A one-line test asserting campaignCellToRunRecord throws on the same inputs would close the gap.
🟡 LOW Campaign-path cost-provenance classification is unasserted in this file — tests/rl-adapters.test.ts
campaignCellToRunRecord now emits raw.cost_observed / cost_estimated / cost_uncaptured and routes defaultCostUsd differently per kind, but no assertion in this file checks those fields or the uncaptured-subtotal path. All four fixtures use kind:'observed', so the estimated and uncaptured branches of the campaign adapter are exercised only indirectly. Impact: a regression that flips the classification bit would not be caught here. Fix (optional): add one estimated fixture and one uncaptured fixture, and assert first.outcome.raw.cost_observed === 1 plus the corresponding bit on the new rows. Non-blocking — the validator contract itself is tested in tests/campaign/run-record-cost-provenance.test.ts.
🟡 LOW No assertions on cost provenance output of the adapter — tests/rl-adapters.test.ts
The fixtures now carry costProvenance, but neither describe block asserts the mapped output (record.costUsd, record.costProvenance, or outcome.raw.cost_observed/cost_estimated/cost_uncaptured/cost_known_subtotal_usd). The field crosses the adapter boundary in campaignCellToRunRecord (src/campaign/run-record.ts:63-79), so this integration test would not catch a regression that mis-maps provenance (e.g. dropping costProvenance or flipping observed/estimated flags). Add one assertion, e.g. expect(first.costProvenance).toEqual({ kind: 'observed', usd: 0.01 }) and expect(first.outcome.raw.cost_observed).toBe(1). The unit-level invariants are covered in tests/campaign/run-record-cost-provenance.test.ts; this is a gap in the rl-adapters layer only.
tangletools · 2026-08-03T07:24:50Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 14 non-blocking findings — a899ae8e
Full multi-shot audit completed 3/3 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 13 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-03T07:24:50Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 0269be22
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-03T07:28:25Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 6800a54e
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-03T07:47:13Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 6800a54e
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-03T07:48:22Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — b9a25c8f
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-03T08:07:12Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — b9a25c8f
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-03T08:08:22Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — b9a25c8f
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-03T08:27:12Z
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 5 non-blocking findings — b9a25c8f
Full multi-shot audit completed 3/3 planned shots over 16 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-03T08:29:44Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 1769.6s (2 bridge agents) |
| Total | 1769.6s |
💰 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: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content; opencode/deepseek/deepseek-v4-pro: Bridge returned 503: {"error":{"message":"cli-bridge admission timed out after 30000ms","type":"admission_rejected","reason":"queue_timeout","admission":{"active":20,"queued":1,"maxActive":20,"maxQueue":48}}}
🎯 Usefulness — sound
Replaces the flat costEstimated boolean with the full CostProvenance discriminated union the cost-ledger already computes, adding defense-in-depth cache validation that gates re-spend on broken caches behind an explicit caller opt-in — wired end-to-end with no dead ends.
- Assessment: The change is coherent, correctly integrated, fits the codebase's grain, and handles real error paths. No materially better approach or existing equivalent was found.
- Integration: Fully reachable. The new
costProvenancefield flows fromCostLedger.summary()(cost-ledger.ts:576) →agentCost.costProvenance→CampaignCellResult.costProvenance(run-campaign.ts:654) → cached JSON →readCachedCellvalidation viacampaignCellCostProvenance(run-campaign.ts:1191) →campaignCellToRunRecord(run-record.ts:63-67) →RunRecord.costProvenance→ `ProfileSummary.costProvena - Fit with existing patterns: Matches the codebase's existing
CostProvenancediscriminated union (cost-ledger.ts:8-11) perfectly — the ledger already computed observed/estimated/uncaptured; the campaign was just discarding that into a boolean. Follows the repo's 'fail loud, no fallbacks' philosophy: invalid caches are never silently reused or rerun. The blocked-cell + explicit-opt-in pattern mirrors the existing `resumable: - Real-world viability: Holds up. Defense in depth:
assertScheduleCachesReusablepre-scans all cached cells before any spend begins (run-campaign.ts:1062-1096), andexecuteCellre-reads + re-validates receipts per-cell (lines 406-429).cachedCellReceiptProblemhandles legacy caches withoutcostCallIds, malformed arrays, empty arrays with paid activity, and missing ledger receipts — each path returns a specific di - Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge warning: opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content; 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":0,"maxActive":20,"maxQueue":48}}}
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.
Summary
planCampaignRunand require an explicit caller choice before rerunning themThis removes the insufficient
CampaignCellResult.costEstimatedboolean. Cached data with missing or invalid cost provenance, unreadable or malformed content, mismatched cell identity, or invalid receipt identities is never silently reused or rerun.Callers can set
rerunInvalidCachedCells: trueto rerun only blocked cells while retaining valid caches. Settingresumable: falseremains the explicit full-rerun path. New caches with explicitly empty receipt IDs remain reusable only when every dispatch and deterministic judge was free; any unknown, estimated, or token-bearing activity still requires exact receipt IDs.API and data notes
ProfileSummary.totalCostUsdchanges fromnumbertonumber | null. Callers must handle null as unknown because a known subtotal is not a known total.CampaignRunPlanaddscellsBlocked.CampaignRunPlanCell.statusaddsblocked.RunCampaignOptionsandPlanCampaignRunOptionsaddrerunInvalidCachedCells; the planning input can also receive the exact cost ledger and tags used by execution.Campaign measurement digests intentionally change because
costEstimatedis replaced by the completecostProvenancevalue. Existing loop provenance records still verify from their own stored content, but digest equality is not expected across this schema boundary.This is a breaking pre-1.0 API/data change and will release as
0.143.0, followed by a Runtime peer-cohort update that compiles against the nullable summary type.Proof