Skip to content

Explain how to recover from credential-provider conflicts - #3883

Merged
freeqaz-openai merged 2 commits into
mainfrom
fix/explain-credential-provider-conflicts
Oct 27, 2026
Merged

freeqaz-openai merged 2 commits into
mainfrom
fix/explain-credential-provider-conflicts

Conversation

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

A static-source update with duplicate registrations for its approved provider now tells the caller to ask an Installation administrator to resolve them. If the provider changes during admission, both create and update return HTTP 409 and tell the caller to check the current provider configuration before retrying the refused credential-source request. This guidance does not apply to a provider operation.

The HTTP tests check the error messages, verify a refused update preserves its original source, and verify a refused create leaves no new source. This addresses the caller-guidance item from the review of #3879.

Observed provider behavior from #3879

With same-type provider B ordered before the admitted provider A, a signed-in source update returned 200. It called A once and B zero times, kept A as the admitted provider, and advanced the source from generation 1 to 2. When A was duplicated or missing, the update returned 409, made no Secret reads or verifier calls, and left the source at generation 2.

The production OCC and OpenShell exchange receiver ran over authenticated loopback gRPC/TLS with PostgreSQL and a counting HTTPS origin. The authenticated test peer made the upstream HTTP request. Counts are per request. Where a row lists two replacements, each was exercised separately and produced the same counts.

Change during the request Send permits Authenticated origin requests Protected frames / bytes received by the peer
None 1 1 1 / 49
Replace the registration or injector during the Secret GET 0 0 0 / 0
Replace the registration or protector after the origin returned 200 1 1 0 / 0
Replace the registration or protector after the original protector ran, while receipt observation waited on PostgreSQL 1 1 0 / 0

During the Secret GET cases, neither injector ran and the peer received no credential headers. In both response-stage cases, the origin request had already completed. The receiver withheld the protected frame from the peer. These observations cover the production receiver with a controlled peer. Installed-supervisor behavior was outside the scope of the run.

@clawsweeper

clawsweeper Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
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.

@freeqaz-openai
freeqaz-openai marked this pull request as ready for review October 27, 2026 17:41
@freeqaz-openai
freeqaz-openai requested a review from a team as a code owner October 27, 2026 17:41
@clawsweeper clawsweeper Bot added clawsweeper:intake:docs Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:docs Preliminary submission classification; full source review pending. labels Oct 27, 2026
@clawsweeper

clawsweeper Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR adds administrator and retry guidance to credential-provider conflict errors and extends existing HTTP integration coverage.

Example: A source approved with source-verifier-a is updated while two registrations claim that provider.

  • Before: HTTP 409 says only that the admitted credential provider is ambiguous.
  • After: HTTP 409 identifies duplicate registrations and asks the caller to contact an Installation administrator.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, correct diagnostic repair with meaningful HTTP harness coverage and passing required CI; supplied runtime observations do not exercise the revised wording.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored message-only repair is outside the ordinary contributor proof gate; its HTTP tests cover the changed responses, while the body's historical runtime observations concern the earlier provider-pinning behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Credential-source callers receive conflict errors without knowing who must resolve duplicate registrations or what to check before retrying.
Reason: The maintainer-authored change addresses a concrete prior finding with two message replacements and focused coverage.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it provides a focused, useful recovery-message improvement, with no introduced defects found.

Priority: P3
Reviewed head: f77ee2810de517d029d8776a3989a82001f33ca8

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No shipped-entry-point trace demonstrates the revised messages, and no executed base-fail/head-pass comparison was supplied.
Agent review details

How this fits together

OCC admits credential-source create and update requests against registered providers; its conflict errors reach authenticated HTTP callers through the controller API.

flowchart TD
  A[Authenticated caller] --> B[Controller HTTP API]
  B --> C[OCC source admission]
  C --> D[Provider registration checks]
  D --> E[Conflict with recovery guidance]
  E --> B
Loading

Technical review

Best possible solution:

Keep the recovery guidance in the existing OCC conflict branches and retain the focused HTTP assertions.

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

Current-main source shows both unhelpful conflict messages, and the existing HTTP scenarios supply concrete duplicate-registration and admission-drift triggers; no target code was run during this read-only review.

Is this the best way to solve the issue?

The patch improves the messages at their existing owner without changing admission semantics, adding configuration, or duplicating recovery machinery.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Credential-provider conflict errors and HTTP scenarios keeps the original intent (9bc30f1: The provider-pinning repair refuses ambiguous registrations and provider changes before credential handoff; this PR preserves those refusals and explains recovery.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Pinned introduced diff: The production change consists of two error-message replacements; provider selection, authorization, Secret access, and persistence behavior are unchanged. (packages/occ/src/index.ts:6105, f77ee2810de5)
  • Current main retains the old wording: Current main still reports ambiguity and admission drift without recovery guidance. (packages/occ/src/index.ts:6105, 7fbf06ff478c)
  • HTTP coverage preserves admission invariants: The changed scenarios assert conflict response envelopes, preserve the existing source after refusal, and verify refused creation leaves no source; they extend the existing real Fastify/OCC fixture rather than introduce another implementation. (tests/integration/credential-proxy-provider-pin.test.ts:553, f77ee2810de5)
  • Linked review establishes the narrow purpose: The review at Keep the selected credential provider through source updates and requests #3879 (comment) requested administrator guidance for duplicate registrations and a recovery action for admission drift; this PR addresses that finding.
  • Required CI passes at the reviewed head: CI Required succeeds for the reviewed head; newer successful admission and dispatch checks supersede the earlier cancelled dispatch attempt. (f77ee2810de5)
  • Policy and history inspection: Read the complete root policy; no nested policies or maintainer-notes directory were present for the changed paths. Scoped feature history identifies the provider implementation and subsequent pinning repair. Older-blob searches and the synthetic-merge diff encountered unavailable local objects; the pinned introduced diff and current-main source were readable. (AGENTS.md, f77ee2810de5)

Labels

Label changes:

  • add P3: This is a narrow diagnostic improvement for an administrator-managed provider-conflict workflow.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is a narrow diagnostic improvement for an administrator-managed provider-conflict workflow.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

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, 1:55 PM ET / 17:55 UTC.

@freeqaz-openai
freeqaz-openai merged commit 475cb89 into main Oct 27, 2026
63 of 66 checks passed
@freeqaz-openai
freeqaz-openai deleted the fix/explain-credential-provider-conflicts branch October 27, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant