[freeqaz-openai]

Updating an existing static source selected a provider by type a second time. If the catalog order changed, provider B could receive the source’s Secret and be saved in place of the approved provider A. OCC now resolves the saved provider reference on update. First enrollment keeps its existing selection order.

During a request, OCC retains the provider and methods it used for classification. An ambiguous eligible registration stops the request before Secret access. If the registration or a captured method changes while authorization or Secret access is pending, OCC refuses the handoff. An explicit binding to an unambiguous provider still works.

The tests change the catalog during source updates and requests, count Secret reads and verifier calls, and inspect the saved source after refusal. These checks cover OCC’s handling of the material. A Secret Driver’s accepted network read and a supervisor’s eventual write remain separate operations.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge.

What this changes

This PR preserves an approved credential provider during source updates and pins provider methods throughout request handling.

Example: An openai source approved with source-verifier-a is updated after source-verifier-b moves first in the catalog.

• Before: The update validates the Secret with source-verifier-b and saves that provider.
• After: The update retains source-verifier-a; a missing or ambiguous registration refuses the update.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The implementation is focused and has meaningful integration coverage, but test descriptions alone do not establish the required final-effect authority behavior.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Updating a protected source can silently replace its approved provider and expose its Secret to another same-type provider.
Reason: The maintainer-authored repair preserves the existing ownership contract, retains first-enrollment selection order, and explicitly documents checkpoint detection limits.

Merge readiness

⛔ Blocked before merge - 3 items remain

Keep this PR open: it addresses a source-backed provider ownership bug that remains on main, and its maintainer-approved direction is useful; one recovery-message defect remains.

Priority: P2
Reviewed head: 7514893cced39e8307d71c59e2fd1b209a67314f

Before merge

• Add real behavior proof - Authority-chain proof required: The supplied evidence consists of integration tests and their description, without a recorded after-fix trace of approved-provider success and substituted-provider rejection before credential callbacks, upstream dispatch, or protected response release; provide redacted logs or terminal output for those changed boundaries, excluding credentials and non-public endpoints. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.
• Give callers a recovery action for provider conflicts (P3) - When duplicate registrations exist, a source PATCH now returns HTTP 409 with only “The admitted credential provider is ambiguous.” The drift branch below likewise reports a changed provider without a next action. These messages reach callers unchanged through the HTTP error mapper, leaving them unable to distinguish an administrator repair from a retry. Name an Installation administrator as responsible for resolving duplicate registrations, and give a retry action for a catalog change during admission.
• Resolve merge risk - Before merge, the PR owner must establish final-effect rejection for substituted provider registrations and methods, including changes while credential reads or response observations are pending.

Findings

• [P3] Give callers a recovery action for provider conflicts — packages/occ/src/index.ts:6104-6105

Tests

• Missing end-to-end proof: Supply a recorded allowed-versus-substituted-provider trace through the real authority and transport boundaries, including pending-read and response-release cases; a base-fail/head-pass comparison is not supplied.

Agent review details

How this fits together

OCC mediates between authenticated execution requests, IAM, credential providers, Secret storage, and the enforcing transport, returning credential headers, send permits, or protected responses.

flowchart TD
  A[Authenticated execution] --> B[Credential Proxy]
  B --> C[Captured provider methods]
  B --> D[OCC and IAM checks]
  D --> E[Secret or prepared material]
  E --> B
  B --> F[Enforcing transport]
  F --> G[Upstream provider]

Loading

Technical review

Best possible solution:

Retain provider ownership through the existing OCC lifecycle, add actionable conflict recovery messages, and establish the changed handoffs with final-effect evidence.

Do we have a high-confidence way to reproduce the issue?

Main visibly reselects a provider by type during updates, and the added integration scenario supplies a concrete two-provider reorder; no target code was run during this read-only review.

Is this the best way to solve the issue?

Pinning the saved provider reference and captured methods in OCC addresses the ownership error at its existing owner without changing Driver contracts or adding configuration.

Full review comments:

• [P3] Give callers a recovery action for provider conflicts — packages/occ/src/index.ts:6104-6105
  When duplicate registrations exist, a source PATCH now returns HTTP 409 with only “The admitted credential provider is ambiguous.” The drift branch below likewise reports a changed provider without a next action. These messages reach callers unchanged through the HTTP error mapper, leaving them unable to distinguish an administrator repair from a retry. Name an Installation administrator as responsible for resolving duplicate registrations, and give a retry action for a catalog change during admission.
  Confidence: 0.98

Overall correctness: patch is correct
Overall confidence: 0.89

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against f9f304cfa9c3.

Merge-risk options

Maintainer options:

1. Establish provider handoff behavior (recommended)
   The PR owner supplies a redacted production-path trace showing the approved provider succeeds and a substituted provider receives no credential or protected response material.
2. Hold the change
   Keep this PR open until the changed credential and response handoffs can be observed.

Provenance checked

• Static source admission and update selection changes intended behavior with a stated reason (Credential Proxy Implementation #2487: The original implementation established protected-source admission and provider ownership; catalog-order selection also served updates even though admitted sources already recorded a provider.)
• Request classification, injection, and response handling keeps the original intent (Credential Proxy Implementation #2487: Provider adapters own classification, injection, and response policy, while admitted work retains its original owner through settlement.)
• Final send authorization and uncertain receipts keeps the original intent (Order native credential send authorization with grant withdrawal #3871: Native IAM authorization and permit persistence share transaction ordering, and lost acknowledgments preserve uncertain receipts without issuing another permit.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

• Current-main source confirms the update bug: Main's admitStaticEgressSource selects the first provider matching the source type, including when updating an already-admitted source. (packages/occ/src/index.ts:6071, f9f304cfa9c3)
• Pinned introduced change: The introduced diff retains saved provider references on update and captures provider functions with current-registration checks around credential and response handoffs; the verified test merge contains the same seven changed paths. (packages/occ/src/credential-proxy.ts:293, 7514893cced3)
• Meaningful integration scenarios: The added tests exercise real OCC/API and authority code with in-memory persistence and wire fixtures, covering catalog reorder, duplicate registrations, pending Secret reads, permit persistence, stream renewal, and response release. (tests/integration/credential-proxy-provider-pin.test.ts:413, 7514893cced3)
• Existing ownership intent: The merged implementation states that admitted work stays with its original owner through settlement; this PR preserves that intent. Credential Proxy Implementation #2487. (packages/occ/src/index.ts:5759, 31156790238d)
• Prior send-ordering proof has a different scope: The predecessor records PostgreSQL and HTTPS observations for grant withdrawal and lost commit acknowledgments; those observations do not exercise this PR's provider substitution scenarios. Order native credential send authorization with grant withdrawal #3871. (packages/occ/src/index.ts:5574, c98be2044ce4)
• Proof discussion inspected: The complete PR body describes tests and explicitly separates OCC handling from Secret Driver network reads and supervisor writes; both comments are bot status messages and provide no additional behavior trace. (7514893cced3)

Likely related people:

• Free Wortley: Raw commit c98be20 adds packages/occ/src/index.ts:5730 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: c98be2044ce4; files: packages/occ/src/index.ts)

Review metrics

Metric	Value	Why it matters
Production and test changes	Production +262/-71; tests +639/-0	The test growth covers distinct scheduling and custody boundaries using existing fixtures rather than adding a parallel production subsystem.

Labels

Label changes:

• add P2: The bug affects credential-source updates and requests when trusted provider registrations reorder, conflict, or change during an operation.
• add merge-risk: 🚨 security-boundary: The change controls which provider code receives credential and response material, and final-effect authority evidence remains incomplete.
• add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

• P2: The bug affects credential-source updates and requests when trusted provider registrations reorder, conflict, or change during an operation.
• merge-risk: 🚨 security-boundary: The change controls which provider code receives credential and response material, and final-effect authority evidence remains incomplete.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add final-effect proof that the approved provider succeeds while substituted registrations or methods receive no credential or protected response material.

Rating scale

6/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.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 27, 2026, 12:55 PM ET / 16:55 UTC.

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf). Comment-only review of 7514893cce.

Summary: correct as far as I can tell. I found no Driver-contract or IAM change and nothing that blocks landing.

Checked

• Update keeps the approved provider: staticEgressProvider(type, previous) resolves the saved EgressRef instead of picking by type in catalog order. When the saved provider is no longer registered, admitted is undefined. The default parameter of admitStaticEgressSource then falls back to catalog order. That path is reachable only with explicit live adoption and execution scope, the same as main's alreadyAdmitted gate. assertCurrent() runs around the Secret read and inject. The saved provider is the copied ref.
• Request path: every candidate is captured before any classify runs, so one classifier cannot swap a later route's provider. checkEgressUseIn uses the exchange's captured provider. That still matches the source's admitted provider, because admitted.key must deep-equal use.route.binding.source, and the route's source.provider is the key the provider was resolved from. isCurrent() is checked at the Secret read port (before readCredentialInput), after selection, after injection, in authorizeSend inside the transaction, after the send receipt commits, at stream-lease renewal and before response release.
• Fail closed: a classifier or response-policy callback that throws now becomes a refusal or response_blocked instead of an exception. Two registrations with the same id+generation refuse as stale_configuration.
• Contracts: CredentialProviderDefinition, CredentialSourceControl and the Driver interfaces are unchanged. EgressAuthorityPorts/ExchangeProvider are internal to OCC, and authorization calls are the same as before. The duplicate-ref refusal checks provider identity, which the contract already assumes, so I don't think it needs an RFC.
• Identity stability: the OpenShell Driver builds sourceControl.providers once in its constructor, so ports.provider(route) === receiver holds for the Driver's lifetime and changes only on reselection.
• Tests: credential-proxy-provider-pin.test.ts and egress-prepared-github-metadata.test.mjs passed 20/20 at 2 CPUs (11 tests per run, 0 skipped). The new file is in checks-baseline-1 with all 9 names in expectedTests. CI Required and First Agent Smoke are green on this head. The PR merges cleanly with main and with #3873, #3876 and #3880.

Non-blocking

• index.ts update path: the fallback relies on JS default-parameter semantics (admitted is undefined, so selection = this.staticEgressProvider(type)). An explicit admitted ?? this.staticEgressProvider(source.type) at the call site, or a one-line comment, would make the re-adoption fallback easier to see.
• The provider-pin tests change the fixture Driver's providers array in place. In production the list is fixed per Driver instance and changes only through reselection or a restart. A one-line comment saying the in-place edit stands in for that would help future readers judge the scenario.
• If a saved provider is registered but now has a different sourceType.type, the update fails with the generic "cannot protect <type> sources" message. That is fail-closed, but the message doesn't say why.