Skip to content

Bump SDK to pick up PKCE auto-negotiation from discovery#49

Merged
jeremy merged 5 commits into
mainfrom
bump/sdk-pkce-negotiation
Mar 6, 2026
Merged

Bump SDK to pick up PKCE auto-negotiation from discovery#49
jeremy merged 5 commits into
mainfrom
bump/sdk-pkce-negotiation

Conversation

@jeremy

@jeremy jeremy commented Mar 5, 2026

Copy link
Copy Markdown
Member

Summary

Problem

Launchpad's OAuth discovery document contains no code_challenge_methods_supported:

{
  "issuer": "https://launchpad.37signals.com",
  "authorization_endpoint": "https://launchpad.37signals.com/authorization/new",
  "token_endpoint": "https://launchpad.37signals.com/authorization/token",
  "response_types_supported": ["code"],
  "grant_types_supported": ["authorization_code", "refresh_token"]
}

The SDK's performInteractiveLogin unconditionally generated PKCE (code_challenge + code_verifier). Launchpad ignores these parameters, breaking the interactive OAuth flow.

SDK fix (b0939b8)

The SDK now reads code_challenge_methods_supported from discovery and only generates PKCE when the server advertises S256:

// Before — always generates PKCE
const pkce = await generatePKCE();

// After — conditional on server support
const serverSupportsPKCE = config.codeChallengeMethodsSupported?.includes("S256") ?? false;
const pkce = serverSupportsPKCE ? await generatePKCE() : undefined;

Wire-level evidence (before/after auth URLs and token exchange bodies) is in the SDK repo's own test suite.

Test plan

  • npm run typecheck — clean
  • npm test — 1130 passed, 10 skipped
  • npm run lint — pre-existing warnings only, none from this change
  • openclaw channels add end-to-end smoke test (interactive, requires browser)

The SDK's performInteractiveLogin unconditionally generated PKCE
(code_challenge + code_verifier), but Launchpad's discovery document
advertises no code_challenge_methods_supported. The SDK now checks
discovery metadata and only generates PKCE when the server advertises
S256 support (basecamp/basecamp-sdk#148).

Also adapts to the SDK's renamed error code ("auth" → "auth_required").
Copilot AI review requested due to automatic review settings March 5, 2026 22:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the interactive OAuth flow by bumping @37signals/basecamp from 63388edb0939b8 (v0.2.0 → v0.2.2). Launchpad's OAuth discovery document does not advertise code_challenge_methods_supported, so the SDK was previously sending PKCE parameters unconditionally, breaking the authorization code flow. The new SDK version auto-negotiates PKCE based on the discovery document. The companion code change adapts classifyDispatchError to the SDK's renamed error code ("auth""auth_required").

Changes:

  • Bumps @37signals/basecamp to commit b0939b8 (v0.2.2) with PKCE auto-negotiation and "auth_required" error code rename.
  • Adapts classifyDispatchError in src/dispatch.ts to handle the renamed SDK error code.
  • Updates package-lock.json to reflect the new SDK version, integrity hash, and incidental lockfile cleanups (removal of hono as an orphaned entry, protocol normalizations to git+ssh://).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/dispatch.ts Renames the matched BasecampError code from "auth" to "auth_required" to align with the updated SDK.
package.json Pins @37signals/basecamp to the new commit hash.
package-lock.json Updates resolved version/integrity for the SDK bump; removes orphaned hono entry; normalizes git URLs to git+ssh://.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/dispatch.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: addab80654

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread package-lock.json Outdated
npm install rewrote resolved URLs from git+https:// to git+ssh://
due to local SSH config, which would break installs without GitHub
SSH access.
Copilot AI review requested due to automatic review settings March 5, 2026 23:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Regenerate package-lock.json with npm 10.9.4 (Node 22.22.0) to match
  CI, fixing the hono sync error. Fix git+ssh:// resolved URLs back to
  git+https://.
- Add tests for classifyDispatchError with BasecampError instances
  (auth_required, forbidden, rate_limit) to cover the structured code
  path alongside the existing heuristic fallback tests.
@jeremy jeremy force-pushed the bump/sdk-pkce-negotiation branch from 2396027 to f15e16e Compare March 5, 2026 23:45
Copilot AI review requested due to automatic review settings March 5, 2026 23:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jeremy jeremy merged commit e487539 into main Mar 6, 2026
5 checks passed
@jeremy jeremy deleted the bump/sdk-pkce-negotiation branch March 6, 2026 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants