Skip to content

fix(contracts): older clients still load PRs from newer servers - #19899

Open
tris203 wants to merge 1 commit into
pingdotgg:mainfrom
tris203:fix/pr-actions-forward-compatible
Open

tris203 wants to merge 1 commit into
pingdotgg:mainfrom
tris203:fix/pr-actions-forward-compatible

Conversation

@tris203

@tris203 tris203 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A mobile build from before #19257 can't open any GitHub pull request from a server that includes it. #19257 added delete-branch to PullRequestAction, and the old client's strict literal decode of capabilities.actions fails the whole response:

Couldn't load this pull request
Expected "merge" | "ready" | … | "approve-workflows" at ["value"]["capabilities"]["actions"][5]

Any future action, verdict, or merge/update method would break older clients the same way, and mobile builds routinely lag servers.

Change

The PR contracts now use the existing forward-compatibility helper, ForwardCompatibleArray (from #15951, already used for auth scopes and provider auth methods), for the lists of growing literal sets that the server sends:

  • PullRequestCapabilities: actions, mergeMethods, updateMethods, review.verdicts
  • PullRequestViewerPermissions: actions, verdicts, updateMethods
  • PullRequestProviderSummary: actions

Unknown members are dropped, so an older client simply doesn't offer the new control. Decoded types are unchanged. Client→server inputs, such as the action request's action, stay strict.

This only protects clients built from this change onward. Builds that have already shipped still need updating.

Scope and approval

This is a very small, focused fix for an obvious bug, so it doesn't need a prior issue:

  • Obvious defect: an older client crashes on every GitHub PR as soon as the server gains an action. It can't load the page at all, and nothing about intended behavior is in question.
  • Small: it changes no product behavior. Known values decode exactly as before, and unknown ones are dropped instead of failing the whole response.
  • Existing precedent: it applies the forward-compatibility helper the contracts already use for other growing server→client lists (feat(contracts): clients tolerate union members they don't know yet #15951).
  • One problem: every changed line is one of the lists of growing literal sets in the same PR payloads that fail in the same way.

Verification

The report. An Android client from before #19257, connected to a server that includes it, fails to open #14883:

Couldn't load this pull request: Expected merge | … | approve-workflows at value.capabilities.actions[5]

Reproduction and fix, on the wire. I decoded PullRequestCapabilities through its JSON wire codec (Schema.toCodecJson, as clients read RPC responses) using the contracts from three revisions:

Each decoded two payloads. The first is GitHub's capabilities exactly as upstream main sends them, which includes delete-branch. The second is a later server that adds an action no build knows about yet (archive).

[client before #19257 (511d20c6f2)] server with #19257: FAILED Expected "merge" | "ready" | "draft" | "close" | "reopen" | "update-branch" | "enable-auto-merge" | "disable-auto-merge" | "revert" | "approve-workflows" at ["actions"][5]
[client before #19257 (511d20c6f2)] future server: FAILED Expected "merge" | "ready" | "draft" | "close" | "reopen" | "update-branch" | "enable-auto-merge" | "disable-auto-merge" | "revert" | "approve-workflows" at ["actions"][5]
[upstream main, no fix (42da166d13)] server with #19257: OK actions=["merge","ready","draft","close","reopen","delete-branch","update-branch","enable-auto-merge","disable-auto-merge","revert","approve-workflows"]
[upstream main, no fix (42da166d13)] future server: FAILED Expected "merge" | "ready" | "draft" | "close" | "reopen" | "delete-branch" | "update-branch" | "enable-auto-merge" | "disable-auto-merge" | "revert" | "approve-workflows" at ["actions"][6]
[this PR (b79fe71d95)] server with #19257: OK actions=["merge","ready","draft","close","reopen","delete-branch","update-branch","enable-auto-merge","disable-auto-merge","revert","approve-workflows"]
[this PR (b79fe71d95)] future server: OK actions=["merge","ready","draft","close","reopen","delete-branch","update-branch"]
  • The first line reproduces the reported error exactly, at index 5.
  • The fourth line shows that upstream main has the same weakness for the next new action.
  • With this PR, the unknown action is dropped and every known action is kept.

Tests and typechecks

  • The new test in packages/contracts/src/pullRequest.test.ts covers an unknown action and an unknown verdict in capabilities: vp test run packages/contracts/src/pullRequest.test.ts, 32 passed.
  • packages/contracts, apps/server, apps/mobile and apps/web typecheck.

Not checked: a rebuilt mobile client against a newer server on a device. The other changed fields (viewer permissions, provider summary, merge and update methods) use the same ForwardCompatibleArray helper, which is covered by baseSchemas.test.ts.

Written with Claude Opus 5.5 in Claude Code (T3 Code); reviewed by GPT-6.1-Sol (Codex).

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 15, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at b79fe71

Macroscope's review found this PR approvable — This is a narrowly scoped contract compatibility fix that drops only unknown server-advertised capability values while preserving known behavior and strict client inputs. The production change is self-contained and covered by existing helper tests plus a targeted regression test.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 948c2cd0-7440-414e-9ea5-c9b6ca183468
📥 Commits

Reviewing files that changed from the base of the PR and between b79fe71 and f6d8e00.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ff30ac0a-55c2-4eb8-9347-9b00c8f3ddef
📥 Commits

Reviewing files that changed from the base of the PR and between 42da166 and b79fe71.

📒 Files selected for processing: 2
  • packages/contracts/src/pullRequest.test.ts
  • packages/contracts/src/pullRequest.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Pull request capability, viewer-permission, and provider-summary arrays now use ForwardCompatibleArray. A test checks that decoding drops unsupported actions and verdicts while retaining supported values.

Changes: Pull request decoding

Layer / File(s) Summary
Forward-compatible array decoding
packages/contracts/src/pullRequest.ts, packages/contracts/src/pullRequest.test.ts
Capability, viewer-permission, and provider-summary arrays use ForwardCompatibleArray. The test verifies that unsupported actions and verdicts are dropped while supported values remain.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b79fe

This change is intended to let updated clients keep loading pull requests when servers provide unfamiliar capability values. No merge-blocking risk is established by the supplied evidence.

Security architecture risk: ⚪ Minimal · up to b79fe

The change improves compatibility with newer servers without granting additional permissions. Action requests remain strict, and server-side permission checks still govern execution.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed exposure is response interpretation across client/server version skew. Unsupported advertisements are removed; the PR does not add action literals, provider credentials, or a new execution entrypoint.

Trust Boundaries and Controls

  • observed — Client-visible permission advertisements are not the execution authority. Strict RPC input validation and server-side capability and fresh permission checks remain in place before provider operations, countering an authorization-bypass interpretation of the relaxed response decoding.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main fix: allowing older clients to load pull requests from newer servers.
Description check Passed The description covers the problem, change, scope and approval rationale, verification steps, observed results, and limitations. It explains the forward-compatibility behavior and reports focused test…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

A server that offers a PR action an older client does not know, such as
delete-branch, failed the client's whole pull request decode. Lists of
actions, verdicts, merge methods, and update methods the server sends now
decode with ForwardCompatibleArray, so unknown members are dropped.
@tris203
tris203 force-pushed the fix/pr-actions-forward-compatible branch from b79fe71 to f6d8e00 Compare October 15, 2026 22:01

This branch has not been deployed

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

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant