feat: user editable oauth proxy server config#2122
Merged
simplesagar merged 27 commits intomainfrom Apr 9, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f6be371 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Collaborator
🚀 Preview Environment (PR #2122)Preview URL: https://pr-2122.dev.getgram.ai
Gram Preview Bot |
simplesagar
force-pushed
the
sagar/age-1737-feat-editable-oauth-proxy-server-config
branch
from
c4d0fd8 to
b3d4260
Compare
simplesagar
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
simplesagar
force-pushed
the
sagar/age-1737-feat-editable-oauth-proxy-server-config
branch
from
a135708 to
f118134
Compare
walker-tx
requested changes
Apr 8, 2026
Member
walker-tx
left a comment
walker-tx
left a comment
There was a problem hiding this comment.
Overall looks fine. Please review Devin's comments, and also resolve merge conflicts, and then I'll approve.
Member
Author
|
@walker-tx thanks for the review — addressing your feedback: Merge conflicts ✅ resolved. Rebased onto Your inline comment on the useMemo ✅ addressed in Devin's remaining open comments — all either resolved or intentionally deferred:
Also included from my self-review (commit
Ready for re-review when you have a moment. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review feedback: document why provider_type and slug are excluded from OAuthProxyServerUpdateForm, and align oauth_proxy_server attribute description with AddOAuthProxyServerForm conventions.
…ion failures (AGE-1737) Review feedback: make audit log assertions symmetric across all error case tests. InvalidAuthMethodRejected, EmptyEnvironmentSlugRejected, and EnvironmentSlugNotFound now match the beforeCount/afterCount pattern already used by GramProviderRejected and EmptyFormIsNoOp.
Introduces isOAuthEditModalOpen state and a second ConnectOAuthModal instance bound to it. Adds an Edit button to OAuthDetailsModal's custom-OAuth branch alongside the existing Unlink button. The ConnectOAuthModal editMode prop wiring is added in the next commit (Task 9); until then the dashboard typecheck will fail with 'Property editMode does not exist'.
…ng (AGE-1737) Adds editMode prop to ConnectOAuthModal and OAuthTabModal, pre-fills the proxy form state via useEffect, dispatches useUpdateOAuthProxyServerMutation when in edit mode, and swaps the modal title and submit button labels. Slug and provider_type remain immutable in edit mode.
…ields unconditionally (AGE-1737) Review feedback on Task 9: - handleProxySubmit's environment slug guard was unconditional, blocking edit-mode submission when the prefilled slug was empty even though the button's disabled condition correctly exempted edit mode. Add the same !editMode gate to the runtime guard. - The useEffect prefill skipped provider-level fields when oauthProxyProviders[0] was absent, leaving stale state. Remove the if (provider) wrapper and use optional chaining so all form fields are reset on every editMode change.
…teOAuthProxyServer (AGE-1737)
Adds the three test cases listed in the spec but missing from Task 5: - ClearScopes: verifies that an explicit empty slice clears scopes (PATCH semantics distinguishing nil vs empty) - SoftDeletedProxyRejected: verifies updates against a toolset whose proxy association was removed return 404 (no proxy attached) - WrongProjectIsolation: verifies cross-project lookups are rejected with CodeNotFound All 11/11 tests pass.
After rebasing onto origin/main, regenerated openapi spec, SDK, and speakeasy workflow lock to reconcile with main's recent changes. The SDK package.json version was corrected from 0.32.28 (artifact of multiple pre-commit regen runs during the original feature work) to 0.32.21 (main's 0.32.20 + one canonical Speakeasy bump).
- cli/internal/api/toolsets.go: pass the new UpdateOAuthProxyServer endpoint to toolsets.NewClient (the generated NewClient now requires 11 endpoints after Goa added the update method) - updateoauthproxyserver_test.go: replace require.True(errors.As(...)) with require.ErrorAs(...) to satisfy the testifylint linter, and drop the now-unused errors import
Two real bugs flagged by Devin Review: 1. Unstable editMode object reference (MCPDetails.tsx) The editMode prop was created as an inline object literal in JSX, producing a new reference on every parent re-render. This caused OAuthTabModal's useEffect([editMode]) to fire on every parent re-render, wiping any user-typed input from the form. Memoize the editMode object via useMemo so its reference is only invalidated when toolset.oauthProxyServer actually changes. 2. Audience field cannot be cleared in edit mode (MCPDetails.tsx) The submit handler used 'audience: proxyAudience || undefined' which coerced empty strings to undefined, causing the field to be omitted from the request body. The server then skipped the update and the user could never clear a previously-set audience. Pass proxyAudience as-is so an empty input is sent and applied. Plus: correct an SDK version downgrade (also flagged by Devin) Main's client/sdk is at 0.32.28 across gen.yaml/jsr.json/package.json/ config.ts/gen.lock, but the post-rebase regen produced 0.32.21 due to the rebase taking the pre-rebase gen.yaml (at 0.32.20) via -X theirs and Speakeasy bumping from there. Manually set the version to 0.32.29 (= main's 0.32.28 + 1 for the new updateOAuthProxyServer endpoint surface).
…E-1737) Devin Review flagged that the previous fix introduced a worse bug: removing '|| undefined' from audience meant that opening the edit modal on a proxy with audience=NULL would silently submit 'audience: ""' on save (because the form prefills empty-string for null DB values), mutating NULL → "" server-side. Empty audience and absent audience are NOT equivalent in OAuth authorization URL construction (?audience= vs no audience= param). Fix: snapshot the prefilled audience in a useRef and only send the audience field in the update request when the user actually changed it. This preserves the user's explicit clear (still works) without silently mutating untouched audience values.
…737) Two improvements from self-review: 1. Drop '(admin only)' from updateOAuthProxyServer description The endpoint's Security() block is just session + project_slug (or apikey + producer scope) — there's no actual admin role enforcement. The description was misleading callers reading the generated OpenAPI spec. Remove the misleading suffix; proper admin enforcement across both add and update endpoints is tracked as a follow-up ticket since AddOAuthProxyServer has the same gap. 2. Capture before/after toolset snapshots in LogToolsetUpdateOAuthProxy Previously the audit log captured only oauth_proxy_server_id, slug, and toolset_version_after in metadata. For an audit log this is too thin — a 'who changed our prod audience' investigation cannot reconstruct the configuration from the audit row alone. Mirror the LogToolsetUpdate pattern: clone the pre-mutation *types.Toolset, strip the Tools field to avoid audit bloat, and serialize both snapshots via marshalAuditPayload alongside the existing metadata. The AudienceOnly test gets new assertions that decode the snapshot blobs and verify the before/after audience values are captured (previously the test only asserted the snapshot fields were nil).
Second rebase onto main brought in listToolsetsForOrg (#2128 area) which required regenerating Goa service and SDK outputs. The -X theirs strategy took my pre-rebase versions of several generated files, so a fresh mise gen:goa-server and mise gen:sdk run was needed to reconcile with main. Also: corrected the SDK version to 0.32.38 (= main's 0.32.37 + 1 for the new updateOAuthProxyServer endpoint). Speakeasy's fresh regen produced 0.32.31 because the rebase took gen.yaml from my pre-rebase branch via -X theirs; manually bumping to 0.32.38 across gen.yaml, gen.lock, jsr.json, package.json, and config.ts. This is the same pattern as the previous rebase — worth documenting in CLAUDE.md as a rebase-time gotcha for branches that touch the SDK.
…ad of the parent (AGE-1737) Review feedback from Walker: the useMemo I added to stabilize the editMode reference was papering over the root cause. The underlying issue is that the useEffect's dep array was keyed on editMode (a wrapper object recreated inline in JSX on every parent render) instead of the stable inner reference editMode.proxyServer. Fix at the source: - Remove the useMemo from MCPSettingsTab and restore the inline editMode literal in JSX - In OAuthTabModal, extract editProxyServer = editMode?.proxyServer and use that as the useEffect dependency Why this works: toolset.oauthProxyServer has a stable reference across parent re-renders thanks to react-query's default structuralSharing (if the underlying data hasn't changed, the nested ref is preserved). So the effect only re-fires when the actual proxy server data changes — not when the parent re-renders for unrelated reasons. User-typed form state is preserved across window-focus refetches and other parent render triggers. Tradeoff vs useMemo approach: this eliminates a layer of indirection and moves the 'why' into the effect's dep array comment, which is where a future reader would naturally look. One less thing to maintain in MCPSettingsTab.
…GE-1737) Third rebase onto main (now at 1b702be..b328938) pulled in more Speakeasy auto-bumps. Main's SDK version files are intentionally divergent: - gen.yaml/gen.lock/jsr.json/config.ts: 0.32.41 (Speakeasy-managed) - package.json: 0.32.38 (Changesets-managed, last released) Speakeasy's mise gen:sdk propagates one version across all 5 files during regen, but Changesets releases reset package.json to its own tracking. This commit matches main's split: - 4 Speakeasy files → 0.32.42 (= main's 0.32.41 + 1 for the endpoint) - package.json → 0.32.38 (matches main; Changesets will handle the future bump via the existing editable-oauth-proxy-server.md entry) Also refreshed server/gen/http/openapi3.json and .speakeasy/workflow.lock from the goa + sdk regen.
simplesagar
force-pushed
the
sagar/age-1737-feat-editable-oauth-proxy-server-config
branch
from
fbf843f to
346494d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…r (AGE-1737)
Devin review finding: AddOAuthProxyServer validates that
authorization_endpoint and token_endpoint are non-empty for custom
providers (impl.go:1008-1012), but UpdateOAuthProxyServer had no
equivalent guard. An API caller bypassing the UI could send
authorization_endpoint: "" or token_endpoint: "", which flows through
conv.PtrToPGText as pgtype.Text{Valid: true, String: ""}, and the
COALESCE in UpdateOAuthProxyProviderFields would overwrite the
existing endpoint with empty string — breaking the OAuth proxy for
all users of the toolset.
Add empty-string guards pre-transaction alongside the existing
token_endpoint_auth_methods_supported allowlist check. The UI already
prevents this case, so impact was limited to direct API callers, but
API boundaries should defend themselves.
Plus two new tests mirroring EmptyEnvironmentSlugRejected:
- EmptyAuthorizationEndpointRejected
- EmptyTokenEndpointRejected
Both assert no audit row is written on validation failure.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
simplesagar
changed the title
…xyServer (AGE-1737) Devin review finding: AddOAuthProxyServer requires non-empty scopes_supported and token_endpoint_auth_methods_supported for custom providers (impl.go:1013-1018), but UpdateOAuthProxyServer allowed sending [] to clear them entirely. An OAuth proxy with zero scopes or zero auth methods is invalid and can't function. Add pre-transaction guards rejecting empty (non-nil) arrays for both fields. PATCH semantics are preserved: nil = no change (skip), but empty array is now a validation error rather than a clear. This changes the previously-documented behavior from the original spec (Q3: 'empty array = clear'). Business validation (non-empty scopes required for custom providers) overrides the generic PATCH semantic — you cannot update a proxy into an invalid state. Replaced the ClearScopes happy-path test with EmptyScopesRejected and added EmptyAuthMethodsRejected. Test suite now 14 cases.
walker-tx
approved these changes
Apr 9, 2026
simplesagar
deleted the
sagar/age-1737-feat-editable-oauth-proxy-server-config
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes AGE-1737. Adds an editable path for OAuth proxy server configuration so admins can change the audience, endpoints, scopes, auth methods, and environment slug on an existing OAuth proxy without unlinking and recreating it (which is the current pain point).
Backend
updateOAuthProxyServer(POST /rpc/toolsets.updateOAuthProxyServer) — admin-only, mirrors the shape ofaddOAuthProxyServerserver/internal/oauth/repo/queries.sql:GetOAuthProxyProviderByServer,UpdateOAuthProxyServerAudience,UpdateOAuthProxyProviderFields. PATCH semantics viaCOALESCE(sqlc.narg(...), col)—nilparameter = no change, empty slice = clear(*Service).UpdateOAuthProxyServerin its own fileserver/internal/toolsets/updateoauthproxyserver.go. Validates auth methods, rejects gram-managed proxies (custom-only), enforces project isolation, performs a transactional update of the server + provider rows, and does a JSON read-modify-write of thesecretsJSONB blob whenenvironment_slugchangestoolset:update_oauth_proxymirroring the existing attach/detach event shapesFrontend
OAuthDetailsModal's custom-OAuth branch (Gram-managed proxies stay view-only)editModeprop onConnectOAuthModalandOAuthTabModalthat pre-fills the proxy form viauseEffect, disables the slug input, and dispatchesuseUpdateOAuthProxyServerMutationinstead of the create mutationTest Plan
mise build:server(orarch -arm64 go build ./server/...on Apple Silicon dev machines) — backend compilescd server && go test ./internal/toolsets/...— all 11 newUpdateOAuthProxyServertests pass alongside existing toolsets testscd server && go test ./...— full backend test suite (1000+ tests across 51 packages) passescd client/dashboard && pnpm tsc --noEmit— dashboard typecheckstoolset:update_oauth_proxyevent🤖 Generated with Claude Code