Skip to content

feat(scan): report token budget stop in JSON summary.budget_exceeded - #791

Open
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:feat/issue-771-scan-budget-exceeded
Open

feat(scan): report token budget stop in JSON summary.budget_exceeded#791
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:feat/issue-771-scan-budget-exceeded

Conversation

@chethanuk

@chethanuk chethanuk commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Description

ocr scan --format json --max-tokens-budget N already detects the aggregate budget stop — it records a token_budget_reached warning and stops dispatching — but summary.budget_exceeded stayed false, because scan.Agent.BudgetExceeded() was hardcoded to return false. Automation consuming the JSON had to parse the warning list to tell a complete scan from one truncated by the budget.

The signal already existed inside scan; it just never reached the result provider.

dispatchBatch (agent.go:586)
  recordWarning("token_budget_reached", …)     :636
  budgetHit = true            ← per-batch local, dies with the call
                              ← now also: a.budgetExceeded = true    :641
  break
      │
      ├─ normal return  :696 ─┐
      ├─ ctx-cancel     :651 ─┤→ dispatchSubtasks
      └─ (err path)           ┘   if err != nil { return }   :538  ← returns first
                                  if budgetHit { break }     :557

emitRunResult → ag.BudgetExceeded() → summary.budget_exceeded

The flag is set where budgetHit is set, not at the break. dispatchBatch has three exits that carry budgetHit, and the ctx-cancel exit at :651 reaches dispatchSubtasks' err != nil return at :538 — which fires before the if budgetHit check at :557. Writing the flag at the break would silently lose it on cancel-after-budget-hit. One write at :641 covers all three exits.

No mutex or atomic: budgetHit = true runs in dispatchBatch's own loop body, before sem <- struct{}{} and outside the worker closure, and dispatchBatch has one caller in a sequential batch loop — one writer on one goroutine, read only after Run returns. internal/agent/agent.go stores the same flag as a plain bool on the diff-review path. make test runs with -race, so this is enforced rather than argued.

budget_exceeded is diagnostic only and does not touch scan's status semantics: scan publishes no run manifest, so the status stays whatever the warnings imply — completed_with_warnings when the gate trips (the token_budget_reached warning is what produces that, not this flag), success otherwise. Both subtests assert the status explicitly so this cannot start driving it. omitempty keeps budget_exceeded out of the JSON entirely when the gate does not trip.

Limitation

The new CLI-level test drives the real *scan.Agent and the real emitRunResult, not a spawned ocr binary — no test in this repo spawns it, and doing so would need live provider credentials. The flag-parsing layer between parseScanFlags and scan.Args.MaxTokensBudget is covered separately by cmd/opencodereview/scan_cmd_test.go.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

make test (with -race) and make check both pass on the current main.

  • cmd/opencodereview/scan_budget_json_test.go (new) — drives a real *scan.Agent over eight fixture files through the real budget gate and emitRunResult, asserting summary.budget_exceeded == true plus the token_budget_reached warning at MaxTokensBudget=120_000, and that the raw JSON contains no budget_exceeded key at all when no budget is set.
  • internal/scan/budget_exceeded_test.go (new) — table-driven, both directions, through dispatchSubtasks.
  • Mutation check: deleting only the a.budgetExceeded = true line fails both.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

budget_exceeded is currently undocumented under pages/ for the review path too, so documenting it belongs in a separate docs PR rather than this one.

Related Issues

closes #771

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

scan already detects the aggregate token-budget stop (it prints the
"[ocr] token budget reached" line and records a token_budget_reached
warning) but BudgetExceeded() was hard-coded false, so
summary.budget_exceeded never appeared in `ocr scan --format json`.

The write goes next to `budgetHit = true` in dispatchBatch's per-file
gate. That is the only site that sets budgetHit, and it covers all three
exits that carry the stop out of dispatchBatch: normal return, ctx-cancel
return, and the caller's `if budgetHit { break }`. Setting it at the
dispatchSubtasks break instead would lose it on the ctx-cancel path.

Plain bool, no mutex: dispatchBatch's loop is the only writer, it runs on
the caller's goroutine, and the value is read by emitRunResult after Run
returns. The spawned subtask goroutines never touch it. Matches the
existing internal/agent.Agent.budgetExceeded field.

Status and exit code are untouched — reaching the budget is a controlled
truncation, so out.Status stays the warning-derived value.
@chethanuk
chethanuk force-pushed the feat/issue-771-scan-budget-exceeded branch from 3f1bea5 to 611847e Compare August 14, 2026 21:06
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.

scan: expose token budget stop state in JSON output

1 participant