fix(cost): enforce verifiable scan spending limits - #465
Conversation
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a147e46ca9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95b3782aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…/codex/portfolio-pr-465-20260815
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9f37a9b17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const session of this.#sessions.values()) { | ||
| if (session.threadId !== null && included.has(session.threadId)) { | ||
| if (session.threadId !== this.#threadId) { | ||
| if (session.pendingLineBytes > 0) observed.unverified = true; |
There was a problem hiding this comment.
When a tracked root or worker log contains a newline-terminated malformed JSON record, readSessionEvent silently ignores it and pendingLineBytes remains zero, so this verification loop can accept an earlier token count as final. If the corrupt record is the worker's last, higher token_count before a valid completion event, a budgeted scan can finish above --max-cost; record parse failures for included sessions must make budget verification fail closed.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| }, | ||
| ); | ||
|
|
||
| testPosix.each(["parented", "independent"] as const)( |
There was a problem hiding this comment.
each through the skipped-test alias
On Windows or when the tests run as root, testPosix is test.skip, but Bun does not define test.skip.each; evaluating this line throws TypeError: testPosix.each is not a function before the suite runs. I reproduced this with bun test tests-ts/cost.test.ts as root, where it prevents all subsequent cost tests from loading, so parameterize with test.each while applying the platform skip separately.
AGENTS.md reference: sdk/typescript/AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
| this.#snapshot.cost === null || | ||
| (cost !== null && cost.estimatedUsd >= this.#snapshot.cost.estimatedUsd) | ||
| ) { | ||
| this.#snapshot = { usage, cost }; |
There was a problem hiding this comment.
This retains only the highest aggregate snapshot, so token-counter resets can discard spend when another session grows later. For example, after observing root usage 100 plus worker usage 1,000, if the worker counter resets to 100 and final root usage reaches 500, the new aggregate is 600 and this code keeps the old 1,100 snapshot, even though the per-session high-water total is 1,500; that undercount can let the scan exceed --max-cost. Preserve each tracked session's highest usage before summing the final snapshot.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6f45b6ee5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? usage | ||
| : subtractTokenUsage(usage, session.inheritedUsage); | ||
| if (ownUsage !== null) session.usage = ownUsage; | ||
| if (ownUsage !== null) { |
There was a problem hiding this comment.
When a tracked root or worker emits an earlier valid token_count, followed by a valid JSON token_count whose totals fail normalization (for example, missing output tokens, impossible cache totals, or an inherited-usage subtraction that underflows), this branch silently retains the earlier accounting while a subsequent task_complete marks the session final. stop() can then accept the stale lower usage and let the scan exceed --max-cost. Unlike the existing JSON parse-failure guard, the fresh evidence here is that a successfully parsed but invalid accounting event never sets accountingError; treat usage === null or ownUsage === null as unverified accounting.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392a1952c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session.accounting?.cost == null || | ||
| (cost !== null && | ||
| cost.estimatedUsd >= session.accounting.cost.estimatedUsd) | ||
| ) { | ||
| session.accounting = { usage: ownUsage, cost }; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier aggregate high-water case is a reset followed by growth within one session: snapshots 1,000 → 100 → 600 represent 1,600 cumulative tokens—the existing rollout delta reader handles a decrease as a reset at sdk/typescript/_bundled_plugin/scripts/workbench_scan_usage.py:474-476—but this comparison retains only the 1,000-token snapshot. A scan whose counter resets and then continues can therefore exceed --max-cost while reporting the lower high-water cost; accumulate deltas across resets rather than selecting one maximum snapshot.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
1 participant
Summary
Enforce explicitly requested scan spending limits when final root or delegated-worker usage cannot be verified.
Fixes #223. Related prior work: #224.
Changes
Testing
Final local checks on the frozen follow-up head
392a1952:bun --no-env-file test --randomize --seed 12345 --timeout 30000, covering all of./tests-tswith JUnit reporting — 1,376 passed, 11 skipped, 0 failed, and 9,428 assertions across 63 files. Ambient provider credentials and runtime/state overrides were removed; both live-test opt-ins were disabled.pnpm run build,pnpm pack,pnpm run check:package, and a separatepnpm run test:packageagainst the resulting archive — all passed sequentially once, with the repository's original timeout limits. The package checks covered the public import, CLI, 106 bundled plugin files, bundled Codex version, and an actual nested worker without global Codex.392a1952, with no candidates. The exact-head receipt check passed. Source remained clean and unchanged before and after every full-suite and package process.Focused checks on the same head:
bun --no-env-file test --timeout 30000 tests-ts/cost.test.ts --test-name-pattern 'keeps mocked unusable own usage scoped to accounting'— the isolated mock passed; its dedicated child-mode run passed 20 assertions. Coverage includes unavailable normalization/subtraction, retained worker cost, first-error precedence, and optional/unrelated/root-only/replay/info-null controls. No actual session or filesystem failure was induced.bun --no-env-file test --timeout 30000 tests-ts/cost.test.ts --test-name-pattern 'mocked .*sessions|mocked per-session|mocked unverified evidence|resolves mocked worker ancestry|limits mocked worker-directory|keeps mocked replay errors|keeps mocked unusable own usage|prefers mocked completed usage'— 9 isolated mock tests passed. The dedicated child-mode run passed the same 9 tests with 153 assertions.bun --no-env-file test --timeout 30000 tests-ts/cost.test.ts --test-name-pattern 'counts the scan and delegated workers|counts independent Deep workers|retains the larger observed cost|verifies independent Deep worker ownership|keeps final budget enforcement|uses completed-turn usage when the final session refresh fails|requires completed root-session evidence|preserves observed active-worker costs|verifies final delegated-worker completion|ignores replayed parent history|uses each session.s final cumulative usage|preserves higher observed root usage|adds observed worker usage to the completed root after a failed refresh|rejects a budgeted scan when the completed model cannot be priced|allows unavailable completed-turn usage'— 31 passed, 63 assertions, 0 failures.bun --no-env-file test --timeout 30000 tests-ts/api.test.ts --test-name-pattern 'handles a session-tracking failure|stops and records a scan|recovers exhausted deep-scan budget|fails a budgeted scan|rechecks strict final accounting|runs post-scan instructions after'— 26 passed, 198 assertions, 0 failures, including strict-finalization, cancellation, known-overage, and ordinary follow-up controls.pnpm run types— passed.pnpm exec prettier --check src/cost.ts tests-ts/cost.test.ts— passed.git diff HEAD^ HEAD --check— passed.b6f45b6e: fixed-seed full SDK suite passed 1,375 tests with 11 skips, 0 failures, and 9,427 assertions. Standard build, pack, package check, and separate installed-package smoke passed. Three native review passes and independent verification were clean; pushed CI and Security Review were green. The later Code Review identified the pre-existing owned-accounting gap addressed in392a1952. Those results do not establish the successor head's full-suite, package, native-review, or pushed-CI status.Risk and rollout
Scans with an explicit spending limit fail when final cost cannot be verified instead of succeeding with understated or unknown usage. One accounting-only strict recheck permits normal completion after a late flush; a cleanup snapshot alone is never proof of final completeness. An exhausted verification failure cannot start a post-scan follow-up. Known parent ownership prevents older scans from contaminating reused output paths; genuinely unresolved worker ownership still fails closed. Missing-session failures retain freshly readable cost without becoming eligible for root-only recovery. A known overage retains the existing limit-exceeded classification even when other usage is incomplete. Recognized unusable owned accounting stays unverified while previously known cost is retained. Per-session high-water accounting preserves a priced lower bound; it does not claim complete reset-delta accumulation. Scans without explicit limits retain best-effort tracking and whole valid usage even when pricing is unavailable. No price, component-wise usage maximum, model-turn retry, delay, polling loop, input-size limit, or session-count limit is invented.
Public disclosure review
Newly authored material was reviewed for public disclosure. Existing automated review comments contain access-restricted report references; maintainer cleanup remains outstanding.