Skip to content

ci: pin pnpm via packageManager for the release workflow - #3

Merged
nicknisi merged 10 commits into
mainfrom
dx-review-and-release-please
Jul 23, 2026
Merged

nicknisi merged 10 commits into
mainfrom
dx-review-and-release-please

Conversation

@nicknisi

Copy link
Copy Markdown
Member

release-please's publish job (`release.yml`) uses `pnpm/action-setup@v6` with no `version` input, which resolves pnpm from package.json's `packageManager` field — a field this repo didn't have. CI only worked because `ci.yml` passed an explicit `version: 10`, so the gap would have surfaced at the worst moment: the first automated npm publish, which would fail at pnpm setup.

This adopts the authkit-nextjs pattern wholesale (the repo `release.yml` was copied from):

  • `package.json`: add `"packageManager": "pnpm@10.28.0"` — the single version source of truth
  • `ci.yml`: drop the explicit `version: 10` and move both jobs to the same `action-setup@v6` pin `release.yml` uses

Verified: pnpm@10.28.0 honors `pnpm-workspace.yaml`'s `allowBuilds` (electron/esbuild postinstalls run), local pnpm auto-switches via the field, and lint/typecheck/tests are green.

This commit was pushed to the previous branch moments after PR #1 was squash-merged, so it needs its own PR.

claude and others added 10 commits July 23, 2026 10:24
Mirror the authkit-nextjs release setup: a release-please workflow that
opens/maintains a release PR and, on merge, calls a reusable release
workflow to publish the scoped public package to npm with provenance.
Also add conventional-commit PR-title linting.

- .github/workflows/release-please.yml: release-please-action + GitHub App
  token, triggers publish on release_created
- .github/workflows/release.yml: pnpm publish --access=public --provenance
- .github/workflows/lint-pr-title.yml: semantic PR title check
- release-please-config.json: release-type "node", "." package
- .release-please-manifest.json: pinned at 0.0.0 (first release computes
  the version from conventional commits)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
A denied/cancelled sign-in or a failed code exchange previously only
logged to console; the renderer never learned of it and stayed silently
signed out. Add an auth-error broadcast path so consumers can react.

- new SDK-owned IPC channel `authkit:auth-error` (main -> renderer)
- AuthErrorPayload type carrying only a safe { code, message } — no
  tokens; refresh-token confinement is unchanged
- broadcastAuthError() mirrors broadcastAuthChange(); createAuthKit emits
  it on a callback `error` param and on a failed completeCallback
- preload bridge exposes onAuthError(); AuthKitProvider tracks it and
  surfaces `error` on useAuth(), cleared on the next successful sign-in
- exported AuthErrorPayload from the root, /react and /preload entries;
  broadcastAuthError from /internals

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
Housekeeping ahead of the first release; no runtime behavior change.

- remove leftover "Phase 1/2/3/4" roadmap wording from shipped source and
  spec comments (neutral wording instead)
- .oxfmtrc.json: drop the stale `docs/` ignore (no docs/ dir exists)
- package.json: bump engines.node to >=22.11.0 to match the authkit
  house style and what CI actually runs (Node 22)
- README: document the new useAuth().error field and the auth-error
  failure-surfacing behavior

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
Add bump-minor-pre-major so breaking changes bump the minor version
while < 1.0.0 instead of going to 1.0.0, matching the authkit-session
and authkit-tanstack-start SDK configs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
The e2e helper hard-coded the __authkit_electron bridge global as a
dotted property access inside a Playwright evaluate callback, which
tripped oxlint's no-underscore-dangle rule. Import AUTHKIT_BRIDGE_KEY
from src/shared/ipc-channels.js and pass it into evaluate as an
argument with bracket access, matching the single-shared-constant
convention established in 574f7e3. No runtime behavior change.
oxlint exits 0 on warnings by default, so CI's `pnpm run lint` let
the no-underscore-dangle warning fixed in 81d881e pass unnoticed.
Add --deny-warnings so any warning fails the build instead of
silently accumulating.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
When the sign-in ceremony fails to open (e.g. win.loadURL() rejects
because the auth server is unreachable), the failure only rejected the
renderer's signIn() promise — it never reached the auth-error broadcast,
so useAuth().error stayed empty while every other failure mode (consent
denied, window closed, failed code exchange) populated it.

The sign-in IPC handler now broadcasts the same safe { code, message }
payload on failure AND still returns { ok: false }, so signIn() keeps
throwing (backwards-compatible) while useAuth().error is populated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT8sJBpwfAuRYKP7Lk53Hh
release.yml's pnpm/action-setup@v6 step passes no version input, so
it reads the packageManager field — which the repo did not have. The
publish workflow would have failed at pnpm setup on the first real
release; CI only worked because ci.yml passed an explicit version.

Add packageManager: pnpm@10.28.0 as the single version source of
truth and move ci.yml to the same v6 pin release.yml already uses,
matching the authkit-nextjs convention.

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@nicknisi
nicknisi merged commit ff42271 into main Jul 23, 2026
5 of 6 checks passed
@greptile-apps

greptile-apps Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Greptile Summary

This PR has two distinct concerns bundled together: the stated CI fix (pinning pnpm via packageManager so release.yml's pnpm/action-setup@v6 can resolve the version without a hard-coded version: input), and a substantive new feature (auth-error broadcasting) that surfaces sign-in failures — denied/cancelled ceremonies, failed code exchanges, and unreachable auth servers — to the renderer via useAuth().error.

  • CI fix: package.json gains "packageManager": "pnpm@10.28.0" as the single version source of truth; both ci.yml jobs are upgraded from action-setup v4 to v6; three new workflows are added (release-please.yml, release.yml, lint-pr-title.yml).
  • Auth-error feature: A new authkit:auth-error IPC channel, AuthErrorPayload type, broadcastAuthError helper, and onAuthError preload bridge method allow the main process to push safe { code, message } payloads to every renderer window on any sign-in failure; AuthKitProvider subscribes and exposes error through useAuth(), cleared on the next successful sign-in.
  • Window ceremony fix: captured = true is set before closeWindow() in the loadURL catch block, preventing a double error broadcast when the auth server is unreachable.

Confidence Score: 3/5

The release.yml publish step is missing npm authentication, meaning the first automated release it enables will still fail.

The pnpm wiring and the auth-error feature are well-implemented with thorough test coverage. However, release.yml's publish step has no NODE_AUTH_TOKEN (or equivalent) set: setup-node with registry-url writes a .npmrc that references this env var, but the var is never populated, so pnpm publish will fail with an auth error on the first automated release — the exact failure mode the PR is trying to prevent. The engines.node narrowing (20→22.11) is also undocumented and could affect consumers.

.github/workflows/release.yml needs an npm authentication token on the publish step; package.json's engines.node bump warrants a note in the changelog or PR description.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Upgrades both jobs from pnpm/action-setup v4 (explicit version: 10) to v6 (reads from packageManager field); straightforward and correct.
.github/workflows/release.yml New reusable publish workflow; pnpm setup correctly reads packageManager, but the publish step is missing NODE_AUTH_TOKEN for npm authentication — the first automated release will fail.
.github/workflows/release-please.yml New release orchestration workflow; permissions, token generation, and workflow_call wiring look correct.
package.json Adds packageManager field (the core fix), promotes lint to --deny-warnings, and silently narrows the engines.node floor from >=20 to >=22.11.0.
src/main/ipc-handlers.ts Adds broadcastAuthError and a shared sendToAllWindows helper; signIn handler now broadcasts an auth-error on failure before returning ok:false.
src/main/create-auth-kit.ts Wires broadcastError into both the deep-link callback and the IPC handler path; adds idempotent registerProtocol guard.
src/react/auth-kit-provider.tsx Subscribes to onAuthError, surfaces failures via error state, and clears it on the next successful sign-in; cleanup correctly unsubscribes both listeners.
src/main/ceremony/window.ts Fixes a double-broadcast bug: sets captured=true before closeWindow() in the loadURL catch block so the closed handler doesn't also emit a synthetic window_closed URL.

Sequence Diagram

sequenceDiagram
    participant R as Renderer (useAuth)
    participant P as Preload Bridge
    participant IPC as ipcMain
    participant SM as SessionManager
    participant C as Ceremony (open)

    R->>P: signIn()
    P->>IPC: invoke(authkit:sign-in)
    IPC->>SM: beginSignIn()
    SM->>C: ceremony.open(authUrl)

    alt Ceremony succeeds → callback arrives
        C-->>IPC: deep-link callback URL
        IPC->>SM: authenticateWithCode()
        alt Exchange succeeds
            SM-->>IPC: AuthResult
            IPC->>P: broadcast(authkit:auth-changed, payload)
            P-->>R: "onAuthChange → user set, error = null"
        else Exchange fails
            IPC->>P: "broadcast(authkit:auth-error, {code, message})"
            P-->>R: onAuthError → error set
        end
    else Ceremony denied / cancelled
        C-->>IPC: "?error=access_denied / ?error=window_closed"
        IPC->>P: "broadcast(authkit:auth-error, {code, message})"
        P-->>R: onAuthError → error set
        IPC-->>P: "invoke result {ok:true, data:null}"
    else Ceremony open() throws (unreachable auth server)
        C-->>IPC: throws SignInOpenError
        IPC->>P: "broadcast(authkit:auth-error, {code, message})"
        P-->>R: onAuthError → error set
        IPC-->>P: "invoke result {ok:false, error}"
        P-->>R: signIn() rejects
    end
Loading

Comments Outside Diff (1)

  1. package.json, line 242-244 (link)

    P2 Undocumented narrowing of the supported Node.js range

    The minimum Node.js version jumps from >=20.0.0 to >=22.11.0. Node 20 is an active LTS release, so any consumer pinned to it would break silently — their install succeeds, but the engines check at runtime would warn or fail depending on the package manager. This change is not mentioned in the PR description, the commit title, or the CHANGELOG.md delta, so it is easy to overlook in a semver bump.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: package.json
    Line: 242-244
    
    Comment:
    **Undocumented narrowing of the supported Node.js range**
    
    The minimum Node.js version jumps from `>=20.0.0` to `>=22.11.0`. Node 20 is an active LTS release, so any consumer pinned to it would break silently — their install succeeds, but the engines check at runtime would warn or fail depending on the package manager. This change is not mentioned in the PR description, the commit title, or the `CHANGELOG.md` delta, so it is easy to overlook in a semver bump.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
.github/workflows/release.yml:28-30
**Missing npm authentication token on publish step**

The `pnpm publish` step has no `NODE_AUTH_TOKEN` (or equivalent) set. `actions/setup-node` with `registry-url` configures `.npmrc` to use `NODE_AUTH_TOKEN`, but if that env var is absent at publish time, the command will fail with an authentication error. OIDC (`id-token: write`) provides the provenance attestation, not the npm auth; a separate npm token is still required unless this package has been enrolled in npm's Trusted Publishers program. The very failure mode the PR is trying to prevent (first automated publish silently breaking) would still occur.

### Issue 2 of 2
package.json:242-244
**Undocumented narrowing of the supported Node.js range**

The minimum Node.js version jumps from `>=20.0.0` to `>=22.11.0`. Node 20 is an active LTS release, so any consumer pinned to it would break silently — their install succeeds, but the engines check at runtime would warn or fail depending on the package manager. This change is not mentioned in the PR description, the commit title, or the `CHANGELOG.md` delta, so it is easy to overlook in a semver bump.

Reviews (1): Last reviewed commit: "ci: pin pnpm via packageManager for the ..." | Re-trigger Greptile

Comment on lines +28 to +30
run: pnpm install

- name: Build project

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Missing npm authentication token on publish step

The pnpm publish step has no NODE_AUTH_TOKEN (or equivalent) set. actions/setup-node with registry-url configures .npmrc to use NODE_AUTH_TOKEN, but if that env var is absent at publish time, the command will fail with an authentication error. OIDC (id-token: write) provides the provenance attestation, not the npm auth; a separate npm token is still required unless this package has been enrolled in npm's Trusted Publishers program. The very failure mode the PR is trying to prevent (first automated publish silently breaking) would still occur.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/release.yml
Line: 28-30

Comment:
**Missing npm authentication token on publish step**

The `pnpm publish` step has no `NODE_AUTH_TOKEN` (or equivalent) set. `actions/setup-node` with `registry-url` configures `.npmrc` to use `NODE_AUTH_TOKEN`, but if that env var is absent at publish time, the command will fail with an authentication error. OIDC (`id-token: write`) provides the provenance attestation, not the npm auth; a separate npm token is still required unless this package has been enrolled in npm's Trusted Publishers program. The very failure mode the PR is trying to prevent (first automated publish silently breaking) would still occur.

How can I resolve this? If you propose a fix, please make it concise.

nicknisi added a commit that referenced this pull request Jul 23, 2026
PR #3 placed "packageManager" after "description" (matching
authkit-nextjs), but oxfmt sorts package.json keys canonically and wants
it last — so `format:check` fails on main and on every PR's merge ref.
PR #3's own CI caught this but was merged before the check was read.
nicknisi added a commit that referenced this pull request Jul 23, 2026
PR #3 placed "packageManager" after "description" (matching
authkit-nextjs), but oxfmt sorts package.json keys canonically and wants
it last — so `format:check` fails on main and on every PR's merge ref.
PR #3's own CI caught this but was merged before the check was read.
nicknisi added a commit that referenced this pull request Jul 24, 2026
PR #3 placed "packageManager" after "description" (matching
authkit-nextjs), but oxfmt sorts package.json keys canonically and wants
it last — so `format:check` fails on main and on every PR's merge ref.
PR #3's own CI caught this but was merged before the check was read.
nicknisi added a commit that referenced this pull request Jul 24, 2026
…can) (#4)

* chore: add public-repo readiness files

Prerequisites for making the repo public per the WorkOS runbook
for shipping public-facing repositories, mirroring authkit-nextjs:

- CODEOWNERS assigning the repo to @workos/dx
- MIT LICENSE plus README License and Contributing sections
- Daily Socket tier-1 vulnerability scan workflow
- Bug report issue template adapted for Electron
  (OS, Electron version, sign-in ceremony)

* ci: harden the Socket scan workflow

Security review of the workflow (originally a verbatim copy from
authkit-nextjs) flagged three issues:

- Script injection: distinct_id was interpolated directly into a
  run: block; pass it through the DISTINCT_ID env var instead so
  workflow_dispatch input can't inject shell commands.
- Supply chain: npm install -g socket floated the latest release;
  pin to socket@1.1.145 since the CLI runs adjacent to
  SOCKET_SECURITY_API_TOKEN.
- Excessive permissions: no permissions block meant default token
  grants; add top-level contents: read, as checkout is the only
  GitHub API use.

* style: move packageManager to oxfmt's canonical position

PR #3 placed "packageManager" after "description" (matching
authkit-nextjs), but oxfmt sorts package.json keys canonically and wants
it last — so `format:check` fails on main and on every PR's merge ref.
PR #3's own CI caught this but was merged before the check was read.
nicknisi added a commit that referenced this pull request Jul 24, 2026
* test: poll for the deep-link terminal event, not a fixed flush

The four deep-link callback tests gated their assertions on
flushAsync(), a fixed 5-microtask + 5 ms window. The callback
pipeline unseals real crypto state (via authkit-session) before the
code exchange, so under parallel test-file load the fixed window lost
the race roughly 1 in 5 full-suite runs ("authenticateWithCode called
0 times").

Each test now awaits vi.waitFor() on its terminal observable event —
the renderer broadcast (sendSpy called once) — with a 5 s timeout
before running its remaining assertions. Negative assertions are safe
because they run after the pipeline has provably completed. The
flushAsync helper (and its orphaned doc comment) is removed as dead
code.

Verified: 12 consecutive full-suite runs green (153/153); lint and
typecheck pass.

* style: move packageManager to oxfmt's canonical position

PR #3 placed "packageManager" after "description" (matching
authkit-nextjs), but oxfmt sorts package.json keys canonically and wants
it last — so `format:check` fails on main and on every PR's merge ref.
PR #3's own CI caught this but was merged before the check was read.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants