Skip to content

Test static credential updates when the owning Gateway is unavailable - #4175

Merged
freeqaz-openai merged 1 commit into
mainfrom
test/static-source-catalog-outage
Oct 30, 2026
Merged

freeqaz-openai merged 1 commit into
mainfrom
test/static-source-catalog-outage

Conversation

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

An operator can retry a static credential update after a Gateway outage; a missing or ambiguous provider requires a configuration fix. The API distinguishes those cases, but the existing test would still pass if an outage were reported as a provider conflict.

Extend the existing HTTP update test for a Gateway-owned source. With a healthy catalog, removing or duplicating the admitted provider still returns 409. With that provider present but its Gateway catalog unavailable, the update returns 503. Both refusals leave the stored source unchanged and cause no additional Secret reads or provider requests. A successful update still uses the admitted provider after providers are reordered.

This changes one test and no production code.

@freeqaz-openai freeqaz-openai added the agent-created PR opened by an AI agent; does not identify the reviewer or approver. label Oct 30, 2026
@clawsweeper

clawsweeper Bot commented Oct 30, 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 30, 2026 07:29
@freeqaz-openai
freeqaz-openai requested a review from a team as a code owner October 30, 2026 07:29
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro 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:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 30, 2026
@clawsweeper

clawsweeper Bot commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR extends the static credential update integration test to distinguish an owning Gateway outage from a provider conflict and verify that refusals preserve the complete stored source.

Example: PATCH an unbound OpenAI credential source while its admitted provider remains registered but the owning Gateway catalog throws an error.

  • Before: The existing test did not assert that this outage returns 503 instead of 409.
  • After: The test requires 503 DEPENDENCY_UNAVAILABLE, unchanged stored source data, and no additional Secret reads or provider requests.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused, source-supported coverage with successful head CI; evidence is integration-harness based, and detailed execution logs were inaccessible.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored test-only PR has no changed production behavior requiring separate live proof; its integration scenario uses the production HTTP API with fixture storage and an injected catalog failure.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Operators need a retryable Gateway outage to remain distinguishable from a provider configuration conflict.
Reason: The maintainer-authored test extension protects a useful existing API distinction without expanding product scope.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it adds focused, useful coverage for an existing API distinction, with no introduced defect found; repository policy and collaborator authorship reserve disposition for maintainers.

Priority: P3
Reviewed head: 5bcd2c60b850a16f8d63d14ebe188d365e962de3

Before merge

None.

Findings

None.

Agent review details

How this fits together

Authenticated credential-source PATCH requests enter the production Fastify API, pass through OCC authorization and source ownership checks, and consult the owning Gateway catalog before credential material access or persistence changes.

flowchart LR
  A[Authenticated PATCH request] --> B[Fastify API]
  B --> C[OCC update handler]
  C --> D[Owning Gateway catalog]
  D --> E[503 on catalog outage]
  C --> F[Secret access and source update]
Loading

Technical review

Best possible solution:

Retain the focused extension to the existing integration test without changing production behavior or introducing another fixture.

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

This PR closes a coverage gap rather than fixing a reproduced production failure; source inspection confirms the expected outage path, and no target code was executed.

Is this the best way to solve the issue?

Extending the existing authenticated HTTP scenario is proportionate: the test exercises the real error mapping and state preservation while injecting only the dependency failure.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Provider-pinning update test and persisted-source assertions keeps the original intent (9bc30f1: Keep the admitted provider across catalog reordering and refuse missing or ambiguous registrations before exposing credential material.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit 9bc30f1 adds tests/integration/credential-proxy-provider-pin.test.ts:414 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 9bc30f1157c8; files: tests/integration/credential-proxy-provider-pin.test.ts)

Review metrics

Metric Value Why it matters
Change scope Production +0/-0; tests +24/-7 The growth adds a distinct failure scenario and stronger preservation assertions inside the existing test.

Labels

Label changes:

  • add P3: This is a focused test improvement for behavior already implemented on main.
  • 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 focused test improvement for behavior already implemented on main.
  • 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 30, 2026, 3:43 AM ET / 07:43 UTC.

@freeqaz-openai
freeqaz-openai merged commit ea5fa56 into main Oct 30, 2026
63 checks passed
@freeqaz-openai
freeqaz-openai deleted the test/static-source-catalog-outage branch October 30, 2026 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-created PR opened by an AI agent; does not identify the reviewer or approver. 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