Skip to content

fix(workflow-budget): reserve physical sends atomically - #809

Closed
luvs01 wants to merge 3 commits into
devfrom
codex/fix-configurable-send-ceiling-vulnerability
Closed

luvs01 wants to merge 3 commits into
devfrom
codex/fix-configurable-send-ceiling-vulnerability

Conversation

@luvs01

@luvs01 luvs01 commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner

Transferred upstream

The completed fix and regression coverage continue in lidge-jun/opencodex#8149, targeting dev. This fork-local PR is closed as transferred; its branch remains the source of the open upstream PR.

The source branch now points to 0b3dc28265d74205b39aa8c6c54dcd9fb6886a23, whose tree 2a0ee9945f81eb9c9e6751ca3d1046cf184706c4 matches the locally validated tree. The upstream PR is ready for review and contains the original commit cf80887e939d031154d2a672690cad2f958830d4.

Completed work

  • Enforce the shared workflow send ceiling at physical admission, including observed/enforced reporters, concurrent requests and retries.
  • Admit new WebSocket workflow roots and retain their leases until the turn ends.
  • Preserve trusted local refusal headers, accurate messages and Combo stop behavior.
  • Require durable spend permits throughout the existing bounded eight-attempt same-account recovery, without expanding ordinary request allowances.
  • Address the original reporter-path review finding and the six upstream inline review findings.

Verification

  • 352 distinct focused behavioral tests passed, including a final native Messages compatibility rerun of 9 tests / 111 assertions.
  • 63 layout, architecture and file-size guard tests passed / 755 assertions.
  • Typecheck passed for 2,312 files; privacy, structure and diff checks passed.
  • Documentation build passed for 641 pages and 93,145 internal links.
  • Current source-branch head 0b3dc28265d74205b39aa8c6c54dcd9fb6886a23 passed upstream Cross-platform CI 41764102474, Catalog 41764102515 and CodeRabbit. All six upstream inline findings are resolved. The upstream PR records the exact local environment limitations and non-blocking follow-up suggestions.

The original draft's failed checks and incomplete reporter path describe historical state; they are superseded by the completed implementation and verification recorded in the upstream PR. The security finding remains open until the fix is accepted and applied upstream.

Original Codex task

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
Walkthrough

The workflow budget now supports reserving and refunding physical sends. Request spend tracking uses those operations around spend-ledger admission. Workflow-send charging was removed from request send-budget reporting call sites.

Changes: Workflow send budget

Layer / File(s) Summary
Reserve and refund workflow sends
src/lib/workflow-budget.ts:674-713, tests/lib/workflow-budget.test.ts:68-79
Adds reserveWorkflowSend and refundWorkflowSend. The test verifies that a reservation enforces the configured limit, updates the send count, and can be refunded.
Integrate reservations with request spend
src/server/responses/request-spend.ts:353-427, src/server/responses/request-send-budget.ts
charge reserves workflow capacity before spend-ledger admission. Already-sent charges use a policy with maxPhysicalSends set to Number.MAX_SAFE_INTEGER. Failed enforced admission refunds the reservation, and refund releases a tracked reservation. The request send-budget reporting call sites no longer charge workflow sends.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant RequestSpendTracker
  participant WorkflowBudget
  participant SpendLedger
  RequestSpendTracker->>WorkflowBudget: reserveWorkflowSend
  RequestSpendTracker->>SpendLedger: admit spend seed
  SpendLedger-->>RequestSpendTracker: admission result
  RequestSpendTracker->>WorkflowBudget: refundWorkflowSend when admission fails
Loading

Suggested reviewers: lidge-jun

Merge Risk: Moderate · previous review cf808

The new atomic send reservation does not cover every physical send path. Additional sends started through the enforced reporter can exceed the configured per-task send ceiling. Route that admission through the workflow reservation and add a test before merging.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the primary change: atomic reservation of physical sends in the workflow-budget logic. The raw summary confirms new reserveWorkflowSend and refundWorkflowSend functions, reservation us…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the primary change: atomic reservation of physical sends in the workflow-budget logic. The raw summary confirms new reserveWorkflowSend and refundWorkflowSend functions, reservation use in request-spend.ts, and a regression test. The title is specific and concise, uses the conventional fix(scope) form, and is 55 characters and 5 words, so it meets the title criteria.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 63de499 and cf80887.

Files selected for processing: 4
  • src/lib/workflow-budget.ts
  • src/server/responses/request-send-budget.ts
  • src/server/responses/request-spend.ts
  • tests/lib/workflow-budget.test.ts
Files with no reviewable changes: 1
  • src/server/responses/request-send-budget.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 31, 2026
@luvs01

luvs01 commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner Author

No description provided.

coderabbitai[bot]

This comment was marked as resolved.

Complete the fork fix on current dev with atomic physical admission, owned retry reporting, credential recovery, and native-wire regressions.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

Devin Review

Comment on lines +365 to +368
if (!options?.deferDispatch && !options?.workflowAlreadyCounted) {
if (alreadySent) chargeWorkflowSends(rootId, 1);
else if (!claimWorkflowSend()) return false;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Failed token admission consumes a workflow send slot

A direct charge with an exhausted token budget claims a workflow slot before the token reservation fails. No send occurs, but later turns can reach the workflow ceiling early.

Learn more

The spend tracker books a root workflow send before it tries to reserve tokens for a direct, undispatched charge. The token reservation can refuse in ensureSeed or reserve, but neither refusal undoes the earlier workflow claim. This affects direct tracker charges without deferDispatch; the execution-budget reservation path uses deferDispatch and does not take this branch. The claimed slot persists in the window despite no physical send.

Example: A tracked root has one remaining send slot, while its provider pool has no token allowance. Calling tracker.charge() returns false for the pool limit, but the root's send count rises by one; a later request to another provider is refused for reaching the send ceiling.

Recommended fix: Reserve the workflow slot only after durable admission succeeds, or add a precise refund to every failed direct-charge exit. Keep the root check and claim in one synchronous admission sequence, and test that a refused token reservation leaves the workflow count unchanged.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 455 to 459
claimPhysicalSend(): number | undefined {
budget.startRequest?.();
if (observer?.enforced !== true || counter.physicalLimit === undefined) return undefined;
if (counter.physicalStarted >= counter.physicalLimit) return undefined;
if (counter.physicalStarted >= counter.physicalLimit || !budget.claimWorkflowSend!()) return undefined;
return ++counter.physicalStarted;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Verification record does not match the reviewed commit

The description names an older commit and says enforced reporters bypass workflow admission. This head adds that admission path, but its description supplies no matching test results. Update the verification record for this commit.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Keep the workflow registration away from the direct-tool-choice registration so both focused fixes merge without a layout conflict.
@luvs01 luvs01 closed this Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant