Skip to content

fix(api): name the OpenShell Proxy, Gateway and service-account refusals - #3739

Merged
freeqaz merged 8 commits into
mainfrom
errmap-2536d
Oct 26, 2026
Merged

freeqaz merged 8 commits into
mainfrom
errmap-2536d

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Finding 2536, slice 4 (the last planned slice). Some OpenShell Credential Proxy, Credential Gateway and service-account refusals still answered with a generic status that hides the fix:

  • Deploy with github-installation access and no observedHookPreviewWorkspace on the Proxy returned a retryable 503, but retrying never helps.
  • A Proxy whose gateway lacks the protected egress middleware, or a preview on a gateway other than 0.1.5, returned the generic 503 text.
  • The Credential Gateway's registration-time copies of the endpoint rules (tenant gateway token_url) answered a generic 404. A stored source whose host / base_url / token_url no longer meets a gateway rule also answered 404 on update and attach.
  • A ChatGPT service-account Backend mismatch answered 409 "already exists".

Change

  • CredentialProxyAdmissionError("installationPreview") (409): names the Installation setting and who changes it. Provisioning status passes it through (same class as the other Proxy admissions).
  • RetryLaterError("proxyRegistration" | "previewGatewayVersion"): still 503 (operator-fixable live gateway state), now with fixed, ID-free text.
  • Credential Gateway (Driver-internal, no contract change):
    • registerSource runs the type's validation with the tenant placement before any gateway write. A failure is a new CredentialSourceRegistrationConfigError (a ScopeViolationError that OCC already passes through), which maps to 400 with /config/<field>, like validateSourceConfig.
    • Stored config that breaks a rule is StoredCredentialSourceConfigError(field) (a ResourceStateConflictError): 409 naming the field and the fix (replace the source).
  • ServiceAccountBackendBindingError("noBinding" | "mismatch") (a ResourceStateConflictError) replaces the two plain conflicts in validateServiceAccountBackendBinding. Provisioning status passes it through.
  • Tests: gitAlone deploy refusal, the installation-preview / gateway-version refusals, and the provisioning-status pass-through of a Proxy admission refusal in the PostgreSQL provisioning journey (shared repositoryProvisioningFixture / workerFailure). Identical answers for missing and unreadable resources use assertSameAnswer. All texts live in tests/helpers/refusal-text.mjs.
  • Docs: breaking-changes entry, credential source refresh flow, protected OpenShell Git reference. The later 2026-10-24 notices move to a new page to keep the main page under the word cap.

Not changed, recorded in the finding's site list:

  • requireRepositoryGitTransport (installed-driver.ts) cannot be reached from a request: a repository Driver exists only when repository is configured, a Proxy requires protected delivery, and installation config checks the same thing at startup.
  • dedicatedSandbox stays: it cannot be reached with today's Drivers, but not in every composition provably.
  • A registration that the gateway refuses still leaves the record deleting; changing that is a contract change (RFC).

Verification

  • tsc, eslint and prettier on the touched files; docs word count.
  • Unit and integration files: 201/201; PostgreSQL provisioning 58/58 and backend ownership 3/3, each file on a fresh database.
  • Mutants, one per retype (each reverts to the old class or drops the check): 13/13 killed. Proxy preview 409, both 503 texts, the registration validation and its 400 mapping, the three stored-config 409s, gitAlone, both service-account reasons, and both provisioning-status pass-throughs. The service-account pass-through first survived, so a job-level test now moves the ChatGPT Backend's workspace after admission.
  • Touched test files pinned 20/20 at 2 CPUs (PostgreSQL files on a fresh database per run): repository-credentials-admission, credential-source-api, egress-enrollment, static-egress, sandbox-driver-startup, http-errors, repository-runtime-material, and PostgreSQL agent-provisioning, backend-ownership and standing-git-transfer.

Risks

  • Status changes for clients that branch on them (503→409, 404→400, 404→409); covered by the breaking-changes entry. Missing or unreadable resources still answer 403/404; every new text is fixed, ID-free and at most 256 characters.
  • Registration now validates before the gateway write; the rules are the same ones validateSourceConfig applies, plus the tenant placement.

…als (wip)

Finding 2536 slice 4: installation-wide access without the preview opt-in is a 409; gateway
registration and version states are 503s that name them; a stored source that breaks a later
OpenShell Gateway rule is a 409 naming the replacement, and a tenant-gateway token_url at
registration a 400 by field; a service account credential from another Backend is a 409.
# Conflicts:
#	tests/helpers/refusal-text.mjs
…status text

Conformance rows for each new answer; deploy cases for the preview opt-in, the gateway version
and a protected source beside installation-wide Git; exact service-account binding texts; a
provisioning job refused for installation-wide access names the refusal in its status. Docs:
breaking-change notice and the two pages that describe the changed answers.
The newest entry put the breaking-changes page over its word cap, and the
earlier 2026-10-24 page is full, so the four later 2026-10-24 entries move
to a new page linked from every notices page.
…oke after admission

The job re-checks the service account's Backend binding; a mutant that drops
ServiceAccountBackendBindingError from the provisioning status pass-through
survived because only admission was tested. Also shortens the earlier
2026-10-24 notices intro to stay under the word cap with the new link.
@freeqaz
freeqaz requested a review from a team as a code owner October 26, 2026 09:29
@clawsweeper

clawsweeper Bot commented Oct 26, 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.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 26, 2026
…config refusals

static-egress still expected the generic DependencyUnavailableError for a gateway
without the protected egress middleware, and sandbox-driver-startup expected
registerSource to throw CredentialSourceConfigError, which it now wraps.
# Conflicts:
#	tests/helpers/refusal-text.mjs
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 26, 2026
@clawsweeper

clawsweeper Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

This PR gives OpenShell Proxy, Credential Gateway, and service-account refusals specific HTTP statuses and actionable messages.

Example: Deploy an Agent using github-installation access without observedHookPreviewWorkspace enabled.

  • Before: 503 DEPENDENCY_UNAVAILABLE with a generic dependency message.
  • After: 409 RESOURCE_CONFLICT explaining that installation-wide access requires the observed-hook preview.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful, focused repair with meaningful API and PostgreSQL coverage; evidence remains harness-based and one recovery instruction needs correction.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored diagnostics PR is outside the ordinary contributor proof gate; its supplied evidence consists of reported integration and mutation results, with no material authority-chain change.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Authorized callers receive generic or misleading failures that hide the configuration or stored-source repair they need.
Reason: The maintainer-authored direction improves existing operations without adding settings, changing Driver interfaces, or weakening refusal rules.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep open: this maintainer-authored PR addresses concrete diagnostic failures still present on main, with one minor recovery-message defect to correct.

Priority: P2
Reviewed head: 57d236d00893fdd896936f8ba23b71cd0d70d81d

Before merge

  • Remove the preview-disable recovery instruction (P3) - For github-installation access with the preview enabled on a gateway reporting 0.1.6, this message recommends turning the preview off and retrying. That retry necessarily fails: assertRepositoryGitProfileAvailable rejects github_installation whenever observedHookPreviewWorkspace is absent, before reading the gateway. Remove that alternative and retain the instruction to run the qualified 0.1.5 gateway, updating the expected refusal text.

Findings

  • [P3] Remove the preview-disable recovery instruction — packages/occ/src/errors.ts:1456

Tests

  • Missing end-to-end proof: Supplied coverage exercises the real API/controller with simulated gateway transport; no shipped OpenShell deployment demonstrating the changed responses is supplied.
Agent review details

How this fits together

OCC authorizes credential-source and Agent operations, invokes the selected Drivers, and converts their failures into HTTP responses or durable provisioning status.

flowchart TB
  Caller[API caller] --> API[Authenticated HTTP API]
  API --> OCC[OCC authorization and lifecycle]
  Worker[Provisioning worker] --> OCC
  OCC --> Drivers[OpenShell and Backend validation]
  Drivers --> Errors[Typed refusal errors]
  Errors --> API
  Errors --> Status[Provisioning status]
Loading

Technical review

Best possible solution:

Retain the existing typed-error approach and give the gateway-version refusal a recovery instruction that preserves its required preview setting.

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

Main's Driver errors and HTTP mapper establish the reported generic responses; source inspection also establishes the introduced recovery-message defect without executing target code.

Is this the best way to solve the issue?

Specific error subclasses reuse existing HTTP and worker owners while preserving validation predicates, authorization checks, and registration cleanup behavior.

Full review comments:

  • [P3] Remove the preview-disable recovery instruction — packages/occ/src/errors.ts:1456
    For github-installation access with the preview enabled on a gateway reporting 0.1.6, this message recommends turning the preview off and retrying. That retry necessarily fails: assertRepositoryGitProfileAvailable rejects github_installation whenever observedHookPreviewWorkspace is absent, before reading the gateway. Remove that alternative and retain the instruction to run the qualified 0.1.5 gateway, updating the expected refusal text.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • OpenShell Proxy availability and installation-access refusal keeps the original intent (3115679: Credential Proxy implementation fails closed unless the selected counterpart and sender can enforce the requested protected access.)
  • Stored credential host and token endpoint checks keeps the original intent (909b0b9: Previously stored endpoints must be checked again before profile writes; the gateway-namespace restrictions also derive from 8642bef.)
  • Service-account Backend binding keeps the original intent (118de64: Managed credentials must match the Agent's configured Provider, Driver, workspace, and issuance state; the later Backend rename preserves that ownership rule.)
  • HTTP and provisioning refusal propagation keeps the original intent (c6e9704: Expose fixed, resource-free repair messages after authorization while keeping missing and unreadable resources generic.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

  • node --test tests/conformance/http-errors.test.mjs tests/integration/repository-credentials-admission.test.mjs
  • pnpm lint
  • pnpm format:check
  • pnpm check:workspace

What I checked:

Likely related people:

  • kevinlin-openai: Raw commit 118de64 adds packages/occ/src/providers.ts:203 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 118de64066c3; files: packages/occ/src/providers.ts)
  • Mrunal Patel: Raw commit 6bbdc49 adds apps/controller/src/drivers/credential-gateway/openshell.ts:210 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 6bbdc492f1bf; files: apps/controller/src/drivers/credential-gateway/openshell.ts)

Labels

Label changes:

  • add P2: Specific API and provisioning diagnostics improve recovery for limited OpenShell and service-account configurations.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: Specific API and provisioning diagnostics improve recovery for limited OpenShell and service-account configurations.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

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 26, 2026, 6:05 AM ET / 10:05 UTC.

@freeqaz
freeqaz merged commit 9cf6b8f into main Oct 26, 2026
56 checks passed
@freeqaz
freeqaz deleted the errmap-2536d branch October 26, 2026 10:11
freeqaz added a commit that referenced this pull request Oct 26, 2026
…m test helper (#3742)

Review follow-ups to #3739: the refresh flow page no longer claims an HTTP 409 for a worker-only attach refusal, the breaking-changes notice adds the provisioning case, two unused error statics are dropped, and one shared PostgreSQL work-claim test helper replaces two copies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant