Repository navigation
Conversation
… operations Requests still authenticating draw on a global budget of 256. Once the peer is verified and its Sandbox matches, the slot moves to a share keyed on the verified sender and Sandbox (256, a quarter of the 1024 cap), so one Sandbox's open handles no longer refuse every other Agent.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
…ation Review follow-ups: the refusal test now checks after its loop that the share and the authentication budget still hold exactly, adds the missing-installation path, and the budget test counts every verification call. The docs name the installation lookup in the release rule and the cost of a Driver whose verification does I/O.
clawsweeper
Bot
added
clawsweeper:intake:bug
|
@clawsweeper re-review |
|
Codex review: needs changes before merge. What this changesThis PR adds a shared authentication budget and verified per-Sandbox shares to Credential Proxy operation accounting. Example: Sandbox A attempts to retain 1024 operation handles while Sandbox B starts a request.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Needs changes before merge - 1 item remains Keep open: this PR repairs operation exhaustion still present on main. No introduced runtime defect was found; one required flow-documentation update remains. Priority: P2 Before merge
Findings
Tests
Agent review detailsHow this fits togetherOCC Credential Proxy receives transport-owned Sandbox requests, verifies identity, checks installation and policy, and selects credential material before separate send authorization. flowchart TB
A[Sandbox request] --> B[Global authentication budget]
B --> C[Peer verification]
C --> D[Verified Sandbox operation share]
D --> E[Installation and replay checks]
E --> F[Policy and material selection]
F --> G[Handle and receipt lifecycle]
Technical reviewBest possible solution: Retain the existing registry and final-hold release mechanism, add verified Sandbox shares, and document the accounting handoff in the owning flow. Do we have a high-confidence way to reproduce the issue? Current-main source permits one verified Sandbox to retain all 1024 operation slots; the supplied regression targets that exact failure, but tests were not run during this read-only review. Is this the best way to solve the issue? The repair uses the existing operation owner, authenticates before assigning a share, preserves atomic admission, and keeps pending work charged until settlement. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against f2a5b4b745ab. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 23, 2026, 4:35 AM ET / 08:35 UTC. |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review |
|
ClawSweeper could not start a re-review for this item. |
Problem
CredentialProxy.resolve()charges its in-progress operation cap (1024 per API process) before it authenticates the peer, and a ready handle keeps its slot until it closes or its deadline passes (at most 30 s). Every Sandbox shares that one cap. A single Sandbox that keeps 1024 handles or pending policy and Secret lookups open makesresolverefuse every other Agent in the installation withunavailable/resolver, for up to 30 s at a time.#3281 fixed the same problem for the replay table only. The operation cap was left alone there because it is charged before authentication, when the only Sandbox identity is the unauthenticated
sandboxIdparameter. A share keyed on that value could be spent by a peer claiming another Sandbox's ID.Change
packages/occ/src/credential-proxy.ts. I followed #3281's pattern.resolvestill charges one slot before any await. That slot counts toward the 1024 cap and toward a new global authentication budget of 256 (OPERATION_AUTHENTICATION_BUDGET). This keeps unauthenticated floods bounded. The bundled OpenShell verification runs in memory, so in practice the budget only limits same-tick bursts.verifyPeersucceeds,peer.sandboxIdmatches, and the peer has not expired, the slot moves in the same synchronous turn out of the authentication budget and into a share keyed on the verified sender and Sandbox (OPERATION_SANDBOX_SHARE, 256: a quarter of the cap). The replay table already keys its share the same way.unavailable/resolverrefusal.OperationChargerelease point returns the slot exactly once. That is the last hold on every path: refusal, throw, abort, handle close, and deadline discard. The release returns the slot to the budget if verification never settled, or to the share if it did.docs/reference/drivers/credential-proxy.mdnow lists the three operation bounds. The request-time trace indocs/flows/credential-source-egress.mdnow describes the slot handoff. That doc stays under its word cap (2498 of 2500).Sizing: a quarter of the cap, as in #3281. No existing test needed more once the tests that filled the cap from one Sandbox were rewritten to use four. Replay protection, the replay table and the OpenShell exchange caps are unchanged.
Provisional decision. The share identity (sender, Sandbox), the quarter share and the 256 authentication budget are provisional, so a maintainer can overrule them. One alternative is to authenticate at the transport, before
resolve, and pass an authenticated identity in. That would remove the global pre-authentication budget, but it changes the driver/transport boundary, so it is out of scope here.Verification
Tests in
tests/integration/static-egress.test.mjsuse the sharedsyntheticDispatchFixtureand itssandboxes()helper:New: "one Sandbox's open handles leave operation capacity to other Sandboxes". It fails on main ("another Sandbox's first request" is refused once one Sandbox holds 1024 open handles). It also covers release on handle close and on deadline discard.
New: "requests still authenticating share a small budget charged before verification". It covers the global budget, refusal before verification, and the move to the share after verification.
New: "every refusal returns its operation slot". It runs 257 refusals on each of ten paths:
After the loop it checks that the share and the authentication budget still hold exactly. A slot released twice would show up as extra capacity.
Updated: "in-flight Secret selection and outstanding handles consume the same bounded capacity" now fills the 1024 cap with four Sandboxes and checks that a fifth is refused before its request is copied.
Updated: "cancelled and denied exchanges retain capacity until receipt work settles" now holds the target Sandbox's share instead of the global cap.
16 mutants, all killed. They cover each guard (the cap, the budget and the share, each dropped and off by one; the share refusal ignored; the share keyed on the sender only) and each release path (the budget, the share and the global count each left unreleased; the authentication slot not taken; the move keeping the authentication slot; a release on every hold; the budget or the share released twice).
The capacity and replay tests (12 cases) pass 20/20, each run pinned to 2 CPUs.
Internal security review: approved, with no must-fix. I applied its should-fixes: a vacuous "never verified" assertion, double-release coverage, and docs wording.
static-egress,credential-proxy-peer-pinandcredential-proxy-withdrawal: 230 pass and 1 skipped (pre-existing) locally on Node 24.tsc --build packages/occ, eslint and prettier are clean on the touched files.Risks
unavailable/resolver.