Skip to content

fix(server): pairing tokens work on Node versions that cannot bind booleans - #16730

Merged
juliusmarminge merged 1 commit into
pingdotgg:mainfrom
chisewaguri:fix/pairing-scope-bind
Oct 8, 2026
Merged

juliusmarminge merged 1 commit into
pingdotgg:mainfrom
chisewaguri:fix/pairing-scope-bind

Conversation

@chisewaguri

Copy link
Copy Markdown
Contributor

Problem

One-time pairing fails with HTTP 500 on Node 24.18.1. AuthPairingLinkRepository.consumeAvailable binds requestedScopes === undefined, a JavaScript boolean, and node:sqlite in that version rejects it with Provided value cannot be bound to SQLite parameter 5. The pairing page shows "Primary environment request failed during exchange-bootstrap-credential (HTTP 500)".

package.json allows ^24.13.1. CI resolves to 24.21.0, which accepts booleans, so CI passed when #9785 added the bind.

Change

Bind 1 or 0 instead of the boolean. SQLite has no boolean type, so the OR check behaves the same.

Scope and approval

This is a one-line fix for an obvious regression from #9785. Pairing a browser or phone with a one-time token fails on any Node version that rejects boolean binds.

Verification

  • On Node 24.18.1, vp test run apps/server/src/auth/PairingGrantStore.test.ts fails 6 of 10 tests on main with the bind error and passes 10 of 10 with this change.
  • Direct check of node:sqlite with prepare("select ? as v").get(true): Node 24.18.1 throws Provided value cannot be bound to SQLite parameter 1, and Node 24.21.0 returns { v: 1 }.
  • Before the fix, pairing a web dev server on Windows with Node 24.18.1 returned HTTP 500 from /api/auth/browser-session.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 7, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This one-line fix preserves pairing-scope logic while replacing boolean SQLite bindings with compatible integer values, addressing failures on affected Node versions. Because it changes authentication and pairing-token persistence behavior, the sensitive-authentication review requirement applies despite the narrow scope.

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

@coderabbitai coderabbitai 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.

Pre-merge checks failed. Please resolve the failing checks before merging.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: 56afce92-7b14-4726-ba07-e3e31723b273
📥 Commits

Reviewing files that changed from the base of the PR and between 10f39eb and 0dd644d.

📒 Files selected for processing (1)
  • apps/server/src/persistence/AuthPairingLinks.ts

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


📝 Walkthrough

Walkthrough

The consume query now uses numeric values for its optional requested-scope condition. The overlap check remains unchanged.

Changes

Pairing link consumption

Layer / File(s) Summary
Update scope condition
apps/server/src/persistence/AuthPairingLinks.ts
The consume query uses 1 when scopes are omitted and 0 when scopes are supplied. The overlap check remains unchanged.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 0dd64

Pairing continues to handle omitted and supplied scopes as intended while avoiding boolean SQLite binds. No actionable user-facing risk remains before merge.

Architecture Summary

Architecture risk: 🔵 Low · up to 0dd64

The change affects 1 system.

Changed systems: apps/server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/persistence/AuthPairingLinks.ts: The consume query’s optional requested-scope condition now interpolates 1 when scopes are omitted and 0 when supplied, replacing boolean interpolation; the subsequent overlap check is unchanged.

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore (reviewers only)

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Approvability ❌ Error The PR changes pairing behavior in apps/server/src/persistence/AuthPairingLinks.ts: consumeAvailable now binds 1 or 0 for the requested-scope condition instead of a boolean. This matches the r… A maintainer should review the pairing-related change in apps/server/src/persistence/AuthPairingLinks.ts before approval.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the server fix and the Node versions affected by boolean binding.
Description check ✅ Passed The description covers the problem, change, scope and approval rationale, and focused verification results.
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.
Full details: Approvability

Explanation

The PR changes pairing behavior in apps/server/src/persistence/AuthPairingLinks.ts: consumeAvailable now binds 1 or 0 for the requested-scope condition instead of a boolean. This matches the rule “Changes authentication, pairing, credentials, secrets, or remote connection trust.” The pull request needs a maintainer's review.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@atimmer

atimmer commented Oct 8, 2026

Copy link
Copy Markdown

[Responding] on behalf of Anton (Claude Opus 5.5)

Also hitting this on macOS arm64 with an installed server build (bundled Node 24.13.1), not just a dev server: every pairing link fails with Provided value cannot be bound to SQLite parameter 5. Node 24.19.0 rejects boolean binds too.

It compounds with #9789: sessions paired before preview:operate existed never gain it, so the Browser surface stays disabled for remote environments until the client re-pairs, and re-pairing is exactly what this breaks. Removing the environment to re-pair leaves the client locked out until this lands.

…oleans

The pairing link consume query bound a JavaScript boolean. node:sqlite in
Node 24.18.1 rejects that, so every one-time pairing failed with HTTP 500.
package.json allows ^24.13.1. Bind 1 or 0 instead.
@chisewaguri
chisewaguri force-pushed the fix/pairing-scope-bind branch from 0dd644d to 16c0009 Compare October 8, 2026 15:35
@juliusmarminge
juliusmarminge merged commit 4daec10 into pingdotgg:main Oct 8, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants