Skip to content

fix(agent-briefing): reconcile charges across summary generations - #8692

Merged
iscekic merged 7 commits into
mainfrom
drain/briefing-ledger-consistency
Oct 24, 2026
Merged

iscekic merged 7 commits into
mainfrom
drain/briefing-ledger-consistency

Conversation

@iscekic

@iscekic iscekic commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — local dev stack on this VM (Next.js dev server, local Postgres), an owned fake account, the real agentBriefing tRPC routes over HTTP with that account's token, and the real GET /api/cron/agent-briefing charge-sweep route. Report rows and usage rows were inserted directly into the local database; no production calls were made.

  • A report whose settlement record names a usage delivery that is still queued stays scheduled and reported pending past the inference deadline, includes the usage rows already committed for that generation in costMicrodollars (300 µ$), and keeps retry unavailable. A retry request at that point answered 409 CONFLICT.
  • Once the queued delivery's usage row is present in the ledger, the next sweep reports the full cost (700 µ$), sets chargeStatus: settled, clears the charge schedule, and makes retry available; the retry request then proceeds to cost-disclosure validation (400 BAD_REQUEST) instead of 409 CONFLICT.
  • Every generation retained on a report is summed: two settled generations (300 µ$ + 500 µ$) report 800 µ$; a usage row for the earlier generation that lands later raises the reported total to 900 µ$ while both generations remain on the report.
  • A delivery recorded 20 days ago, past the queue window, settles from the committed ledger and clears the schedule.

Change

  • A generation's settlement record keeps the gateway's delivery outcome (committed or queued) and the time the receipt was written.
  • Reconciliation treats a committed usage row as authoritative. A queued outcome keeps its generation open for the usage queue's retention window, measured from the receipt time, and settles from the committed ledger as soon as the row exists. After that window, or for a receipt without an outcome, the request-deadline bound applies.
  • Reconciliation sums the baseline plus every generation the report retains, so a usage row that lands for an earlier generation after a retry is still counted.
  • A reconciled report that is already final is skipped only when its charge schedule is already cleared, so a leftover schedule is removed once.
  • Retry stays available only when every tracked generation's usage is final.
  • API shapes and error codes are unchanged.

Verification

  • pnpm --filter web exec jest --runInBand src/lib/agent-briefing src/routers/agent-briefing-router.one-off.test.ts src/routers/agent-briefing-router.test.ts: 8 suites, 122 tests pass, after merging main.
  • pnpm --filter web exec jest --runInBand ../../packages/web-shared/src/lib/agent-briefing-usage.test.ts ../../packages/web-shared/src/lib/ai-gateway/handlers/llm-proxy.test.ts: 2 suites, 156 tests pass.
  • pnpm --filter @kilocode/worker-utils test: 46 files, 1032 tests pass.
  • pnpm --filter web typecheck, pnpm --filter @kilocode/web-shared typecheck.
  • The HTTP route runs described under Evidence.

Production prerequisites

None.

Proxy answers

  1. Question: Settlement boundary for this billing change.
    Answer: Conservative receipt — one request-scoped settlement record per generation, with same-generation history retained.
    Confidence: 0.60
  2. Question: How the repair path should work when a settlement record is missing.
    Answer: Deadline-bounded repair against the generation's own enforced deadline.
    Confidence: 0.70
  3. Question: How long a known queued usage delivery keeps a report scheduled before its charge is final.
    Answer: Queue retention window (four days).
    Confidence: 0.60
  4. Question: Which bound to use when queue retention may be configured above its default.
    Answer: Fourteen days, the platform maximum, so any supported queue setting is covered.
    Confidence: 0.60

Every generation a report has run keeps its settlement record, and
reconciliation sums all of them plus the preserved baseline under the
report row lock. A generated usage identity is recorded by the gateway
after usage accounting completes, and is accepted only for the report's
current generation, owner, organization scope and capability.

Retry stays unavailable until every tracked generation's settlement
record and usage row are present, and the pending summary keeps the
reconciled total. The estimate check compares against the cost the
authorization was granted for, so charges recorded later for an earlier
generation do not invalidate the queued generation.
@iscekic iscekic added drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening labels Oct 22, 2026
Comment thread apps/web/src/lib/agent-briefing/summary.ts Outdated
Comment thread packages/web-shared/src/lib/agent-briefing-usage.ts
@kilo-code-bot

kilo-code-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit is comment-only and corrects the queued-delivery anchor description in reconcileLockedBriefingCharge; I verified the new wording against how the anchor is actually derived and found no defect.

Assumptions: [INFERENCE] none. The changed comment matches the code: generation.deadlineAt is set at claim time to startedAt + maxDurationSeconds (apps/web/src/lib/agent-briefing/summary.ts:658-666), so falling back to it when deliveryAt is absent does use a deadline from the same request lifetime, and it can be later than queue acceptance - the point the previous wording got wrong.

Scope

Reviewed the single commit after the previously reviewed SHA d45e3a5e3: ac8311580 (docs(agent-briefing): correct the queued-delivery anchor comment), 1 file, 2 changed lines (comment text only).

Verification of the changed code

  • apps/web/src/lib/agent-briefing/summary.ts:552-553 - the new comment says a record without deliveryAt falls back to the request deadline "inside the same request lifetime". Line 555 gives exactly that fallback (deadlineMs), and line 658-666 shows the deadline is derived from the same generation's startedAt and maxDurationSeconds, so the description is accurate and the earlier "never later than the queue acceptance it bounds" claim is correctly retracted.
  • No behavioural change: the diff touches only the two comment lines; anchorMs, the retention-window comparison at line 557, and the deadline/grace path at line 561 are unchanged.
  • No stale finding to carry forward: the previous review recorded zero inline comments and no open defects, and the earlier settled-row schedule fix at line 577 is untouched.
Files Reviewed (1 file)
  • apps/web/src/lib/agent-briefing/summary.ts
Previous Review Summaries (4 snapshots, latest commit d45e3a5)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit d45e3a5)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit adds a single guard so a settled report with a leftover charge schedule is rewritten once to clear it, and I found no defect in the changed lines or the new test.

Assumptions: [INFERENCE] row.next_charge_check_at is only populated by the reconciliation and generation-claim paths read here, so a settled row that still carries a schedule is always a leftover rather than an intentionally armed follow-up; the update path sets next_charge_check_at: null whenever awaitingRows is false, which holds for every settled outcome, so the write cannot repeat on the next sweep.

Scope

Reviewed the one commit after the previously reviewed SHA 30132a387: d45e3a5e3 (fix(agent-briefing): clear a leftover charge schedule when a report is final), 2 files, 29 changed lines.

Verification of the changed code

  • apps/web/src/lib/agent-briefing/summary.ts:577 - the short-circuit that skips the write when a settled row already matches now also requires row.next_charge_check_at === null, so a final report whose schedule was left behind is rewritten once. The update at lines 582-585 sets the schedule to null because awaitingRows is false whenever settled is true, so the condition cannot loop or re-arm the sweep.
  • apps/web/src/routers/agent-briefing-router.one-off.test.ts:1128-1152 - the new case settles a committed receipt, artificially re-arms next_charge_check_at in the past, re-reconciles the stale row, and asserts the schedule is cleared while the total, settled status, and canRetry are preserved and visible through get. Without the line 577 guard the early return would leave the schedule set, so the test is meaningful.
  • No regeneration or locking change: reconcileBriefingCharge still re-reads the row under for update, and the added condition only affects whether a write happens.

Residual notes (not findings): a manual DLQ redrive after the queue window can still commit usage for an already-settled generation; that would make the stored total stale until the next reconcileBriefingCharge run, which the leftover-schedule fix now does clear for rows whose sweep is still armed.

Files Reviewed (2 files)
  • apps/web/src/lib/agent-briefing/summary.ts
  • apps/web/src/routers/agent-briefing-router.one-off.test.ts

Previous review (commit 30132a3)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit makes a queued usage receipt keep its generation open for the queue's retention window instead of closing it at the inference deadline, and I found no defect in the changed lines.

Assumptions: [INFERENCE] delivery: 'queued' is only reachable where USAGE_QUEUE_ONLY_ENABLED is set, which packages/web-shared/src/lib/config.server.ts:68-71 gates to non-production staging, so the 14-day pending window has no production exposure. [INFERENCE] The queued row commits under the receipt's own usageId (the consumer forwards core.id to the usage writer), so the usageId match at apps/web/src/lib/agent-briefing/summary.ts:544 settles it without waiting for the window; this was read from code, not exercised against a redrive.

Scope

Reviewed the one commit after the previously reviewed SHA 578e28ad: 30132a387 (fix(agent-briefing): keep queued usage deliveries scheduled), 5 files, 140 changed lines.

Verification of the requested changes

  • apps/web/src/lib/agent-briefing/summary.ts:551-557 - a delivery: 'queued' generation no longer becomes final at the request deadline; it stays waiting until deliveryAt + 14 days, so the sweep keeps next_charge_check_at armed and a delayed commit is still counted. A committed row that matches the receipt settles immediately at line 544, so the window only applies to deliveries whose row has not landed. Receipts without a delivery marker keep the previous deadline-plus-grace behavior.
  • The ordinary sweep and list paths are covered: the new case in apps/web/src/routers/agent-briefing-router.one-off.test.ts asserts pending past the inference grace, then a commit observed by sweepAgentBriefings() produces settled with the committed total, and a second case asserts the window ends reconciliation.
  • packages/web-shared/src/lib/agent-briefing-usage.ts:78-84 - the receipt still refuses a second write for the same generation and merges usageId, delivery, and deliveryAt into the existing generation object, preserving its other fields.
  • packages/web-shared/src/lib/agent-briefing-usage.test.ts - both receipt statuses assert the persisted delivery outcome.

Residual notes (not findings): after the 14-day window a manual DLQ redrive could commit a row for an already-settled generation; the stored total would then be stale on list until a get or estimate reconciles it. DEVELOPMENT.md:604 says a failed accounting "closes its generation at once", while receipts without a delivery marker still wait for the generation deadline plus the grace margin.

Files Reviewed (5 files)
  • apps/web/src/lib/agent-briefing/summary.ts
  • apps/web/src/routers/agent-briefing-router.one-off.test.ts
  • packages/web-shared/src/lib/agent-briefing-usage.ts
  • packages/web-shared/src/lib/agent-briefing-usage.test.ts
  • DEVELOPMENT.md

Previous review (commit 578e28a)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The follow-up commit adds a deadline-bounded repair path for missing settlement receipts and documents the cross-region row lock, resolving both previously reported findings; no new defects were found in the changed lines.

Assumptions: [INFERENCE] The 10-minute CHARGE_REPAIR_GRACE_MS covers the gateway's own 800s function limit against the 600s maximum briefing duration, so no usage row can still be written when the repair adopts the ledger. This was verified from apps/ai-gateway/src/app/api/v1/[...path]/route.ts:9, packages/app-shared/src/agent-briefing.ts:122, and apps/web/src/lib/agent-briefing/summary.ts:637-705; it was not validated against production timing.

Verification of previous findings

  • apps/web/src/lib/agent-briefing/summary.ts (previously WARNING, lost receipt) - resolved. Reconcile now classifies each generation as settled/repaired/waiting/unreachable, adopts committed usage for a generation once its own recorded deadlineAt plus the grace margin has passed, records deadlineAt on every new generation, and stops re-arming next_charge_check_at once no tracked generation can still change. New tests cover settlement past the deadline without a receipt, pending state inside the deadline, and the legacy case no longer being re-polled.
  • packages/web-shared/src/lib/agent-briefing-usage.ts (previously SUGGESTION, cross-region row lock) - resolved. Lines 32-34 now document why the primary row lock is required for the post-response receipt write.
Files Reviewed (3 files)
  • apps/web/src/lib/agent-briefing/summary.ts
  • apps/web/src/routers/agent-briefing-router.one-off.test.ts
  • packages/web-shared/src/lib/agent-briefing-usage.ts

Previous review (commit c24af34)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/agent-briefing/summary.ts 527 Settlement requires a usageId supplied only by a one-shot, best-effort receipt; a transient write failure or a generation claimed before deploy stays unsettled forever, keeps the sweep re-polling every 60s, and blocks retry for an already-billed failed summary.

SUGGESTION

File Line Issue
packages/web-shared/src/lib/agent-briefing-usage.ts 31 The gateway path opens a primary-region transaction with a row lock from sfo1 without the justification comment required by packages/web-shared/AGENTS.md.
Files Reviewed (11 files)
  • DEVELOPMENT.md - no issues
  • apps/web/src/lib/agent-briefing/summary.ts - 1 warning
  • apps/web/src/routers/agent-briefing-router.one-off.test.ts - no issues
  • apps/web/src/routers/agent-briefing-router.test.ts - no issues
  • packages/web-shared/src/lib/agent-briefing-usage.ts - 1 suggestion
  • packages/web-shared/src/lib/agent-briefing-usage.test.ts - no issues
  • packages/web-shared/src/lib/ai-gateway/handlers/llm-proxy.ts - root cause of the warning
  • packages/web-shared/src/lib/ai-gateway/handlers/llm-proxy.test.ts - no issues
  • packages/web-shared/src/lib/ai-gateway/llm-proxy-helpers.ts - no issues
  • packages/worker-utils/src/redact-headers.ts - no issues
  • packages/worker-utils/src/redact-headers.test.ts - no issues

Fix these issues in Kilo Cloud


Reviewed by deepseek-flash-latest · Input: 54.1K · Output: 2.4K · Cached: 298.2K

Review guidance: REVIEW.md from base branch main

Each generation records the deadline its request enforces. When a
settlement record is missing, reconciliation adopts the committed usage
rows for that generation once the deadline plus a margin has passed, and
stops rechecking generations whose settlement can no longer change.
Document why the receipt write takes the primary row lock.
@iscekic iscekic added merge:human Substantive functional risk requires human review. intent:ok Janitor change confirmed as a real bug fix, not intended behavior labels Oct 22, 2026
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Changes requested: the deadline repair can stop reconciliation while a known usage receipt is still queued.

At apps/web/src/lib/agent-briefing/summary.ts:543-566, missing committed rows become unreachable after the grace period. The code then clears next_charge_check_at. In the supported staging queue-only mode, queue acceptance precedes persistence, and delayed delivery or redrive can commit usage later. The ordinary sweep then no longer revisits the report, leaving its stored/listed cost stale. No production exposure is claimed; queue-only mode is staging-gated.

Keep known queued usage scheduled until authoritative persistence or a terminal delivery result. Do not infer accounting finality from the inference deadline alone. Cover a delayed commit past the grace period through the ordinary sweep/list path. This is source-derived analysis, not an executed queue-redrive reproduction.

@iscekic iscekic added the do-not-merge Do not merge this PR (yet) label Oct 24, 2026
Each generation's settlement record now carries the gateway's delivery
outcome and when it was recorded. Reconciliation keeps a queued delivery
open for the queue's retention window and settles the total from the
committed ledger, so a delivery that lands late is still counted.
@iscekic iscekic removed the merge:human Substantive functional risk requires human review. label Oct 24, 2026
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

bot: The settlement record now carries the gateway's delivery outcome and the time it was recorded. Reconciliation keeps a queued outcome open for the usage queue's retention window, so a usage row committed after the inference deadline is still counted: the reconciled total follows the ledger whenever the row lands, and retry becomes available only when every tracked generation is final.

Verified end to end through the sweep route on the local stack. With only a queued receipt present, the report stayed scheduled and pending while already-committed usage was included in the total. Once the second row was committed, the next sweep settled the report with the full total and enabled retry.

Commit: 30132a3. CI is re-running on the new head.

…s final

Reconciliation skipped its write when the reported charge already matched
the ledger, which also kept a charge check time that no longer needed to
run. The skip now requires the schedule to be clear, so a final report
stops being rechecked once.
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Sweep proof on d45e3a5, run against the local stack with a reference report row and the real charge-sweep route.

  • While a settlement record's delivery is queued, reconciliation keeps a charge check scheduled even past the inference deadline, reports pending, includes every usage row committed for that generation, and withholds retry until the record is final.
  • A report becomes settled with retry available and no charge check scheduled once the ledger holds the receipt's usage row.
  • A delivery time older than the queue window is final, so the report settles from the committed ledger and the schedule clears.

Checks on this head: 3 suites, 60 tests pass; typecheck for @kilocode/web-shared and web; oxlint and oxfmt clean.

The fallback anchor is the request deadline from the same request
lifetime, not a bound that precedes queue acceptance.
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Re-ran the same three sweep checks on ac83115 (comment-only delta): queued receipt past the inference deadline stays scheduled and pending with retry unavailable; the settled state appears once the ledger holds the receipt's usage row, with the schedule cleared; a delivery time older than the queue window settles from the committed ledger with the schedule cleared.

Suites and typecheck are unchanged from the previous head; CI is running on this one.

@iscekic iscekic added the merge:human Substantive functional risk requires human review. label Oct 24, 2026
…-consistency

# Conflicts:
#	apps/web/src/lib/agent-briefing/summary.ts
@iscekic iscekic added merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. do-not-merge Do not merge this PR (yet) labels Oct 24, 2026
@iscekic
iscekic added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 2bfbd19 Oct 24, 2026
86 of 87 checks passed
@iscekic
iscekic deleted the drain/briefing-ledger-consistency branch October 24, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain intent:ok Janitor change confirmed as a real bug fix, not intended behavior janitor:trust Janitor finding: trust boundary hardening merge:human Substantive functional risk requires human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants