Skip to content

fix(api): name repository, standing approval and credential source field refusals - #3725

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

freeqaz merged 5 commits into
mainfrom
errmap-2536

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

requestFailure in apps/controller/src/http/errors.ts replaces the message of a plain
ScopeViolationError (404 "The requested platform resource was not found."), a plain
ResourceConflictError (409 "The requested platform resource already exists.") and a
non-whitelisted DependencyUnavailableError (503 "A required platform dependency is
unavailable."). Several OCC refusals that a caller can fix were thrown with those classes, so
the specific text never reached the caller (finding 2536, first slice):

  • A credential source with an unknown or missing config/secrets field answered 404.
  • Agent create, update, provisioning and deploy with repository selections the Namespace does
    not approve, or with both repositoryAccess and repositoryBindings, answered 404.
  • Standing repository approval refusals answered 409 "already exists" or 404 (a policy
    refusal at approve and at deploy).
  • A harnessAuth.source naming a ServiceAccount in another Namespace answered 404.
  • Deploy and provisioning with repository bindings on a Compute Driver without repository
    credentials (Docker, SSH) answered a retryable 503.

The generic 404 for missing or unreadable resources is deliberate (no existence oracle), so
this is not a global pass-through: each site was checked for what the caller has already
proved before the throw.

Change

13 throw sites, each retyped to a class requestFailure already returns verbatim:

Site Was Now
credential source catalog fields (unknown, required) 404 400 CredentialSourceConfigError, detail /config/<field> or /secrets/<field>, e.g. Credential source type openai has no field secrets.extra: remove it. Its secrets fields: api_key.
repository selections not approved; both selection forms 404 400 RepositoryBindingSelectionError with a fixed text per rule
standing approval: binding locked, one binding (save, deploy), policy, scope changed 409 generic / 404 409 ResourceStateConflictError naming the step
ServiceAccount Harness source in another Namespace (save, provisioning) 404 400 SecretBindingValidationError
Compute Driver without repository credentials 503 409 ResourceStateConflictError naming the admin fix

Disclosure rules kept:

  • Every site runs after the caller's grant on the Agent, Namespace or source, except the
    ServiceAccount Namespace check, which reads only the request and runs before any account
    lookup.
  • No text names an ID, name or field of a resource the caller cannot read. The repository
    text is the same for unknown, foreign-Namespace, wrong-profile and duplicate refs. Standing
    approval texts say an approval blocks, but give no approval ID (approvals are read with Agent
    administer), as SecretReferencedError counts referents the caller cannot read.
  • CredentialSourceConfigError gains an optional group (default config), and HTTP now caps
    its message and escapes its detail path. This class lives in OCC; the Driver contract is
    unchanged.

Docs: credential source and Agent error rows, the standing approval guide, and a breaking-change
notice for the status changes (the two oldest 2026-10-24 notices move to the earlier page to
keep the main page under the length limit).

The other reachable sites from the audit (protected Credential Proxy deploy admission,
prepared-source update, the two user-worded 503s, Driver-level ScopeViolationErrors and
ResourceConflictErrors) are listed for a follow-up.

Verification

  • tests/conformance/credential-source-occ.test.mjs: exact status, text and detail path for
    the unknown and required field refusals on create and update; a caller without the source
    grant gets the same answer for an existing and a missing source (update) and Namespace
    (create), with each request's own ID hidden.
  • tests/integration/repository-credentials-admission.test.mjs: exact 400 text at create,
    PATCH, provisioning and deploy after a policy change; 409 text for SSH Compute; a caller
    without grants gets the same answer for an existing and a missing Agent on PATCH and deploy,
    while the administrator gets the named refusal for the same request.
  • tests/integration/occ-api.test.mjs: the ServiceAccount Namespace rule at Agent create
    (existing and missing account answer alike) and at provisioning (a missing account in the
    route Namespace stays 404).
  • tests/integration/postgres-standing-approval-api.test.ts: exact text for each standing
    approval refusal, and a deploy after a policy change answers 409 without the approval ID.
  • tests/integration/postgres-protected-credential-free.test.mjs: a credential-free source
    without execution authority now answers 400 naming config.authority (the catalog requires
    it) instead of 404. tests/integration/egress-prepared-github-metadata.test.mjs: the same for
    a github-app-metadata source without consent.
  • tests/browser/console-agent-repositories.test.ts: the catalog-shrink recovery now sees 400 at
    Agent create; the console already treats 400 as a known rejection and offers Reload
    repository choices
    as before.
  • Mutants on a remote test VM: reverting the repository retype fails the repository tests;
    running the field check ahead of the source grant and naming the stored source fails the
    identical-answer test (400 vs 403); making the deploy policy text echo the approval ID fails
    the standing approval test.
  • Touched files pinned 20/20 at 2 CPUs (the two PostgreSQL files each on a fresh database per
    run); postgres-platform lane, tsc --build with tests and
    eslint on the touched files pass.

Risks

  • Status changes (404 to 400/409, 503 to 409) for the listed requests; clients matching the old
    status or generic text need the breaking-change notice. The console shows these as before
    (its 400/409 messages are its own).
  • A queued provisioning job whose Compute Driver lost repository support now fails permanently
    instead of retrying; every retry would have failed the same way.
  • GET .../repository-approval-scopes and a provisioning status read whose stored repository
    selection the policy no longer approves now answer 400 with the named rule instead of 404.
  • Standing approval texts tell a caller with Agent update or deploy, but without Agent
    administer, that an approval exists (no ID). That caller was already refused; only the
    reason is new.

…eld refusals

Several OCC refusals that depend only on the request, or on state the caller
already proved access to, were thrown as plain ScopeViolationError,
ResourceConflictError or DependencyUnavailableError, so the API answered the
generic 404 "not found", 409 "already exists" or a retryable 503.
Retype them to verbatim classes with text that says what to change:
credential source catalog fields (400 with the field path), repository
selections (400), standing repository approval conflicts (409), a
ServiceAccount Harness source in another Namespace (400), and a Compute
Driver without repository credentials (409). Finding 2536.
…hange notice

Move the two oldest 2026-10-24 notices to the earlier page to keep the
main page under the length limit.
@freeqaz
freeqaz requested a review from a team as a code owner October 26, 2026 05:25
@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
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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:needs-repro Preliminary submission classification; full source review pending. clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment 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 maintainer review before merge.

What this changes

This PR replaces generic API refusals with specific field, repository-selection, standing-approval, and Compute-capability explanations.

Example: Create an openai credential source without secrets.api_key.

  • Before: 404 NOT_FOUND: The requested platform resource was not found.
  • After: 400 INVALID_REQUEST names secrets.api_key, explains how to supply it, and points to /secrets/api_key.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported repair with meaningful integration coverage and green checks; supplied behavior evidence remains harness-based.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored PR is outside the ordinary contributor proof gate; its registered API, IAM, and PostgreSQL tests support the changed refusal behavior, while live-server evidence is not supplied.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Callers receive misleading not-found, already-exists, or retryable-dependency responses for requests they can correct.
Reason: The maintainer-authored direction improves existing API diagnostics while retaining resource-concealment rules and explicitly documenting compatibility consequences.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it makes existing refusals actionable without changing admission rules, and no introduced correctness or security defect was found.

Priority: P2
Reviewed head: 82e759279e4969dbbeab996841f0e2fc3e7844ca

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No separate shipped-server HTTP trace is supplied; reviewed coverage uses controller calls and registered-app harnesses, with regression mutation results reported in the body.
Agent review details

How this fits together

The controller authorizes API requests, OCC validates resource operations, and the HTTP error mapper converts typed refusals into public status codes, messages, and field details.

flowchart LR
  Request[API request] --> IAM[Exact operation authorization]
  IAM --> OCC[OCC admission checks]
  OCC --> Mapper[Typed error mapping]
  Mapper --> Response[HTTP refusal and recovery guidance]
Loading

Technical review

Best possible solution:

Land the focused typed-refusal changes with the documented status changes and existing disclosure boundaries preserved.

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

Current-main source shows the catalog and repository refusals reaching generic error mappings; the changed tests exercise these paths, but no target code was run during this read-only review.

Is this the best way to solve the issue?

Using existing typed errors is a focused solution that retains authorization order, avoids a global message pass-through, and escapes field-detail paths.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Credential-source catalog refusals changes intended behavior with a stated reason (unknown: The base validates catalog fields before gateway effects; older-content git log -S and blame encountered unavailable blobs, so the precise introduction remains unknown.)
  • Repository-selection scope errors changes intended behavior with a stated reason (2fd3d79: Repository lifecycle admission rejects selections outside Namespace policy; the PR preserves rejection and replaces concealment with fixed text containing no repository identity.)
  • Standing-approval refusals changes intended behavior with a stated reason (3115679: Credential Proxy Implementation #2487 established exact, separately approved standing access; all approval predicates and authority checks remain intact.)
  • Typed credential-source field errors keeps the original intent (669cf7a: Support OpenAI-compatible Codex model endpoints #913 introduced field-specific configuration errors; the optional group preserves existing config callers while supporting secrets details.)
  • Cross-Namespace request validation keeps the original intent (8f9d976: fix(occ): reject cross-namespace credential source and discovery secrets as invalid requests #1064 established named request errors for foreign references checked before lookup; the ServiceAccount change follows that boundary.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Pinned production diff: The introduced production changes retype existing refusals and improve field-message formatting; the admission predicates remain unchanged. (packages/occ/src/index.ts:14015, 82e759279e49)
  • Current main still needs the change: The relevant production files on fetched main retain the base behavior: catalog failures and repository-policy refusals still use generic scope errors. (packages/occ/src/index.ts:13981, 5632162a3197)
  • Disclosure boundary coverage: Added coverage compares missing and unreadable Agent and credential-source identities, while authorized callers receive the named refusals; ServiceAccount Namespace checks use only submitted references. (tests/integration/repository-credentials-admission.test.mjs:1004, 82e759279e49)
  • Accepted compatibility tradeoff: The collaborator-authored body explicitly accepts changed HTTP statuses, permanent rejection of queued provisioning after repository support disappears, and fixed approval-existence explanations without approval IDs; the breaking-change notice documents client handling. (docs/guides/deploy/breaking-changes.md:24, 82e759279e49)
  • Existing worker-message boundary: The existing PostgreSQL provisioning test deliberately expects a generic terminal worker message and a detailed refusal from Retry; the new HTTP wording does not require passing arbitrary worker errors through. (tests/integration/postgres-agent-provisioning.test.mjs:1189, 82e759279e49)
  • Validation and supplied proof: CI Required, PostgreSQL Platform, Console Browser, and Repository Credentials checks succeeded for the reviewed head. The full body reports focused repetitions and mutation checks; discussion contains only bot status comments, with no separate live HTTP artifact. (82e759279e49)

Likely related people:

  • Free Wortley: Raw commit 2fd3d79 adds packages/occ/src/index.ts:2453 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 2fd3d7913258; files: packages/occ/src/index.ts)
  • Sally O'Malley: Raw commit 669cf7a adds packages/occ/src/errors.ts:893 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 669cf7a81118; files: packages/occ/src/errors.ts)

Labels

Label changes:

  • add P2: Specific API diagnostics improve recovery for rejected operations without changing successful admission.
  • add merge-risk: 🚨 compatibility: Clients matching the previous statuses or generic text must adapt; this tradeoff is explicitly accepted in the maintainer-authored PR and documented.
  • 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:

  • P2: Specific API diagnostics improve recovery for rejected operations without changing successful admission.
  • merge-risk: 🚨 compatibility: Clients matching the previous statuses or generic text must adapt; this tradeoff is explicitly accepted in the maintainer-authored PR and documented.
  • 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 26, 2026, 2:00 AM ET / 06:00 UTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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