Skip to content

fix(campaign): preserve unknown cost provenance - #530

Merged
drewstone merged 7 commits into
mainfrom
fix/campaign-unknown-cost-provenance
Aug 3, 2026
Merged

fix(campaign): preserve unknown cost provenance#530
drewstone merged 7 commits into
mainfrom
fix/campaign-unknown-cost-provenance

Conversation

@drewstone

@drewstone drewstone commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • carry CostLedger cost state through campaign cells and canonical run records
  • retain reasoning and cache-write tokens from paid-call receipts
  • inspect every cached cell and its saved billing receipts before concurrent work begins
  • expose blocked cells in planCampaignRun and require an explicit caller choice before rerunning them
  • make profile summaries return null plus explicit provenance when total spend is unknown

This removes the insufficient CampaignCellResult.costEstimated boolean. 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: true to rerun only blocked cells while retaining valid caches. Setting resumable: false remains 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.totalCostUsd changes from number to number | null. Callers must handle null as unknown because a known subtotal is not a known total.

CampaignRunPlan adds cellsBlocked. CampaignRunPlanCell.status adds blocked. RunCampaignOptions and PlanCampaignRunOptions add rerunInvalidCachedCells; the planning input can also receive the exact cost ledger and tags used by execution.

Campaign measurement digests intentionally change because costEstimated is replaced by the complete costProvenance value. 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

  • clean GitHub CI: 4,721 passed, 3 skipped across 350 files
  • official TypeScript optimizer integrations: 2 passed
  • lint: 664 source files passed
  • strict TypeScript and examples: passed
  • build and OpenAPI emission: passed
  • packed package export verification: passed
  • Python client, official GEPA, published GEPA, and DSPy compatibility: passed
  • Python wheel and source distribution verification: passed
  • focused cache, cost, and optimizer-resume checks: 182 passed across 7 files
  • mixed-cache regression: a runnable cell precedes a stale cell, yet zero new dispatches occur before refusal; explicit opt-in reruns only invalid cells and reuses the valid cell
  • missing-receipt regression: planning reports two blocked cells, zero new dispatches occur before refusal, and explicit opt-in reruns exactly those cells
  • failed Router-call regression retains 236,736 fresh input, 9,497,984 cached input, 12,345 cache-write, 155,470 output, and 109,802 reasoning tokens while recording USD as uncaptured
  • unknown-zero regression rejects both missing and explicitly empty receipt IDs without rejecting a canonical free deterministic cache
  • six focused integrity cases cover estimated fallback plus every malformed cost state

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 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?: boolean and adds a required costProvenance: CostProvenance (observed | estimated | uncaptured) sourced directly from CostLedgerSummary.costProvenance (src/cost-ledger.ts:141,576). It propagates reasoning and cacheWrite tokens into CampaignTokenUsage (alr
  • Goals it achieves: Stop encoding unknown spend as zero. Today a failed/expired provider call sets costUnknown: true on its receipt; the old costEstimated heuristic only flagged receipts missing actualCostUsd, so an uncaptured call looked like a $0 observed call and silently polluted campaign aggregates, the measurement digest (provenance.ts), and ProfileSummary.totalCostUsd. After this change, uncaptured s
  • Assessment: Good change on its merits. It aligns the campaign cell with the canonical RunRecord shape (src/run-record.ts:178-180 already had costUsd: number | null + costProvenance) and reuses the substrate CostProvenance type 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 ?? null in
  • 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 CostProvenance discriminated union already lives in cost-ledger.ts:8-11 and is already computed by CostLedgerSummary; the canonical RunRecord already used costUsd: 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.

value-audit · 20260803T064007Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 75c25ce4

Review health 100/100 · Reviewer score 77/100 · Confidence 75/100 · 11 findings (11 low)

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 .totalCostUsd as 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) to costProvenance: 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 fail verifyLoopProvenanceRecord re-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 explicit assertCampaignCellCostProvenance(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 check rec.costProvenance, rec.costUsd, or the new outcome.raw keys (cost_observed, cost_estimated, cost_uncaptured, cost_known_subtotal_usd) introduced in src/campaign/run-record.ts:75-78. All four fixtures also use the same kind (observed), so the adapter's uncaptured→null-costUsd branch and the defaultCostUsd fallback (run-record.ts:64-67) are not hit from this file. Impact: low — sibling tests/campaign/run-profile-matrix.test.ts in 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, and campaignCellToRunRecord (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 emits cost_known_subtotal_usd and 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
tangletools previously approved these changes Aug 3, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 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

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 75c25ce4

Review health 100/100 · Reviewer score 51/100 · Confidence 75/100 · 16 findings (2 medium, 14 low)

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 only costEstimated), and the catch at run-campaign.ts:1045 converts it to miss 'corrupt'. Every pre-existing cache entry is therefore re-dispatched after upgrade — real LLM spend on a resumable run — and planCampaignRun reports these as reason:'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 for costEstimated presence) so operato

🟠 MEDIUM Breaking public API change without deprecation or release-note signal — src/campaign/types.ts

costEstimated?: boolean is replaced by required costProvenance: CostProvenance, and ProfileSummary.totalCostUsd changes numbernumber|null (run-profile-matrix.ts:168). Both types are exported via src/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 a campaignCellCostProvenance-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.totalCostUsd derives from campaign.aggregates.cost.costProvenance (ALL channels including judge receipts), so a single unknown-cost judge receipt nulls the profile total while byProfile.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: number to totalCostUsd: number | null and 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 the costProvenance object. canonicalDigest JSON-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. verifyLoopProvenanceRecord only self-checks the embedded digest, so no hard failure — but the schema: '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/catch in readCachedCell collapses all failures (JSON parse error, manifest mismatch, cell mismatch, provenance validation) into a single miss '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) vs agentCost.reasoningTokens !== undefined && > 0 and agentCost.cacheWriteTokens !== undefined && > 0. CostLedgerSummary declares cachedTokens as required number and 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 through campaignCellCostProvenance, while readCachedCell validates 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?: boolean and no costProvenance, 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 campaignCellCostProvenance across *.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 first it(...) 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 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
tangletools previously approved these changes Aug 3, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — a899ae8e

Review health 100/100 · Reviewer score 57/100 · Confidence 75/100 · 14 findings (2 medium, 12 low)

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.totalCostUsd changes from number to number | null (line 168) and CampaignCellResult.costEstimated?: boolean is removed in favor of mandatory costProvenance (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(...) or cell.costEstimated will 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 remaining costEstimated refs), and RunRecord.costUsd was

🟠 MEDIUM New mandatory costProvenance silently invalidates all pre-existing resumability caches — src/campaign/run-campaign.ts

readCachedCell now calls campaignCellCostProvenance(cached), which throws on any cell lacking costProvenance (the pre-0.142.2 costEstimated shape). 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 test reruns 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

totalCostUsd is derived from campaign.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 flips byProfile[].totalCostUsd to null even when 49/50 cells were fully observed, and the numeric subtotal remains visible on campaign.aggregates.cost.totalCostUsd (inconsistent surfaces). Conservative and intentional per the new tests, but consider documenting the whole-or-nothing semantics on ProfileSummary and having consumers prefer costProvenance + aggregates.cost.totalCostUsd for 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 of costEstimated (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 (has costEstimated, no costProvenance), 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_uncaptured are emitted as one-hot flags (0/1 each). A cell with mixed receipts (one provider-billed, one unknown-cost) reports cost_uncaptured: 1 and cost_observed: 0 even though part of the spend WAS observed; the partial-observation detail lives only in cost_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 a cost_known_subtotal_usd presence 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' and options.defaultCostUsd is set, the value is used directly as costProvenance.usd. If defaultCostUsd is NaN or Infinity, validateRunRecord (called at line 115) catches it via expectNonNegativeNumberexpectFiniteNumber, so no silent corruption. The error message does not name defaultCostUsd as the source however, making diagnosis harder. Adding a Number.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 (the cost_uncaptured flag takes over). Downstream reading raw.cost_estimated directly (bypassing RunRecord.costProvenance) would see shifted values. The new cost_observed/cost_uncaptured flags provide strictly more information; costProvenance on 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.defaultCostUsd set (the campaignToRunRecords/rl-adapters path), costUsd becomes defaultCostUsd and tokens_per_dollar (line 100) and cost_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 pins costUsd=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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 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
tangletools previously approved these changes Aug 3, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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
tangletools previously approved these changes Aug 3, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — b9a25c8f

Review health 100/100 · Reviewer score 73/100 · Confidence 75/100 · 5 findings (1 medium, 4 low)

deepseek-flash: Correctness 73 · Security 73 · Testing 73 · Architecture 73

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 16 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Legacy caches without costProvenance now hard-block every resumable run — src/campaign/run-campaign.ts

readCachedCell now calls campaignCellCostProvenance(cached) and classifies any cache lacking the new costProvenance field as 'missing-cost-provenance' (run-record.ts:151-153 throws 'has no costProvenance'). cacheIssueRequiresExplicitRerun() returns true for that reason, so assertScheduleCachesReusable (line 1092) blocks the cell and runCampaign refuses to start with the default resumable:true + rerunInvalidCachedCells:false. At base commit f379f2a, the old executeCell reused a cache when costCallIds===undefined AND no usage AND no judgeScores — i.e. free deterministic legacy caches were safely reused without receipts. Those same caches now require a paid re-dispa

🟡 LOW Preflight doubles cache I/O and does O(cells x ledger) receipt scans — src/campaign/run-campaign.ts

assertScheduleCachesReusable re-reads every cached-result.json for the full schedule, and cachedCellReceiptProblem calls costLedger.list({tags}) per cell (line 1128) — a full O(n) ledger scan per cell, so the preflight is O(cells x receipts). executeCell then re-reads each cache a second time (acknowledged TOCTOU guard, line 236). For large schedules (hundreds of cells) over a large shared ledger this is a measurable double-pass before any dispatch. Not a correctness bug; could be collapsed to a single ledger pass building a tag->receipt

🟡 LOW Breaking public type surface shipped without a version gate — src/campaign/types.ts

CampaignCellResult.costEstimated?: boolean is removed and costProvenance: CostProvenance becomes REQUIRED (types.ts:577-580); ProfileSummary.totalCostUsd changes number->number|null (run-profile-matrix.ts:168); campaignCellToRunRecord/campaignCellCostProvenance now THROW on any cell without costProvenance (run-record.ts:63,151) instead of deriving from costUsd/costEstimated as before. Internal callers are all green (typecheck + tests pass) and the change is deliberate and documented in the field comment, but consumers of the published package (agent-runtime/agent-knowledge, and any external code calling campaignCellToRunRecord with their own cells or summing byProfile.totalCostUsd across profiles) get compile-time breaks or, for JS consumers, runtime throws on legacy-shaped cells. Recommen

🟡 LOW 'uncaptured zero-cost cache' test name overclaims; fixture is not zero-cost — tests/campaign/run-campaign.test.ts

The cache under test was produced by DISPATCH, which reports actualCostUsd 0.01, so cached.costUsd stays 0.01 after costProvenance is overwritten to { kind:'uncaptured', usd:null } — it is a paid-but-uncaptured cache, not zero-cost. The genuinely-free uncaptured cache (kind 'uncaptured', costUsd 0, costCallIds []) also blocks, but via the kind !== 'observed' disjunct in cachedCellReceiptProblem (run-campaign.ts:1104-1106), which this test never isolates. Both block for the intended fail-closed reason, so the assertions are correct, but the 'zero-cost' framing and the untested free path could mislead a future reader about which condition triggers the block.

🟡 LOW Corrupt-cache matrix omits the JSON.parse-throw path — tests/campaign/run-campaign.test.ts

The corrupt-classification tests cover valid-JSON-non-object (null/[]/42/string) and unreadable-but-existing files, but not a file whose raw content fails JSON.parse. readCachedCell (src/campaign/run-campaign.ts:1177-1181) has a distinct try/catch branch returning reason 'corrupt' for unparseable JSON that no test pins; a regression there (e.g. throwing instead of classifying) would not be caught. Suggest adding a case writing e.g. '{not json' and asserting status 'blocked' + reason 'corrupt'.


tangletools · 2026-08-03T08:29:44Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 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

@drewstone
drewstone merged commit c85bbba into main Aug 3, 2026
2 checks passed

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 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 costProvenance field flows from CostLedger.summary() (cost-ledger.ts:576) → agentCost.costProvenanceCampaignCellResult.costProvenance (run-campaign.ts:654) → cached JSON → readCachedCell validation via campaignCellCostProvenance (run-campaign.ts:1191) → campaignCellToRunRecord (run-record.ts:63-67) → RunRecord.costProvenance → `ProfileSummary.costProvena
  • Fit with existing patterns: Matches the codebase's existing CostProvenance discriminated 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: assertScheduleCachesReusable pre-scans all cached cells before any spend begins (run-campaign.ts:1062-1096), and executeCell re-reads + re-validates receipts per-cell (lines 406-429). cachedCellReceiptProblem handles legacy caches without costCallIds, 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.

value-audit · 20260803T083907Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants