Upgrade wrapped atproto PDS to latest upstream (Node 22 + ESM) - #233
Open
aspiers wants to merge 12 commits into
Open
Upgrade wrapped atproto PDS to latest upstream (Node 22 + ESM)#233aspiers wants to merge 12 commits into
aspiers wants to merge 12 commits into
Conversation
Platform prerequisite for the upstream @atproto upgrade (bead atproto-pd5.1). Latest @atproto/pds and @atproto/oauth-provider drop Node 18/20 and convert to pure ESM, so pds-core must flip first. - pds-core: add "type":"module" and engines.node>=22; bump @types/node ^22 - root engines.node >=20.0.0 -> >=22; add .nvmrc (22) - tsconfig: module/moduleResolution Node16 -> NodeNext - index.ts: replace bare require.resolve with createRequire(import.meta.url) - preview-shared.ts: createRequire(__filename) -> createRequire(import.meta.url); refresh stale CJS/Node-20 comments - preview-test-helpers: annotate mockLogger return (TS2742 under NodeNext) - Dockerfiles + CI workflows: node 20 -> 22 Dependency versions unchanged (bead atproto-pd5.2). Build clean, 299 pds-core tests green, dist/index.js boots as ESM on Node 22. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bead atproto-pd5.2. Raise the three atproto pins through the breaking minor channels to the current latest: - @atproto/oauth-provider ^0.15.9 -> ^0.21.1 - @atproto/oauth-provider-ui ^0.4.3 -> ^0.8.9 - @atproto/pds ^0.4.209 -> ^0.5.23 Upstream advanced past the upgrade brief (checked 2026-07-27): oauth-provider crossed an additional 0.21.0 minor beyond the report's 0.20.x target — an extra breaking channel to clear in bead .3/.4. Install is clean on Node 22 with a single resolved copy of each package and no old transitive versions. The build is intentionally RED after this commit: the barrel-import and type breakage is fixed in beads .3 (imports) and .5 (Account typing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bead atproto-pd5.3. oauth-provider 0.20.0 removed the bare barrel entrypoint (confirmed absent in 0.21.1: exports are ./errors ./constants ./store ./hooks ./provider ./verifier ./middleware ./utils). Repoint each pds-core import: - HandleUnavailableError -> @atproto/oauth-provider/errors - DEVICE_ID_*/SESSION_ID_* consts -> @atproto/oauth-provider/constants - DeviceAccount, DeviceId (types) -> @atproto/oauth-provider/store - OAuthProvider (type) -> @atproto/oauth-provider/provider No BrandingInput/CustomizationInput references exist in pds-core, so no rename needed. All 4 module-not-found (TS2307) errors are resolved. The remaining build errors are strong did/handle typing + the changed Account shape, fixed in bead atproto-pd5.5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bead atproto-pd5.5. oauth-provider-api 0.7 retyped Account: the OIDC-style
`sub`/`preferred_username` string fields became branded `did: DidString` and
`handle?: HandleString`, and pds/oauth-provider getAccount/createAccount now
require branded DidString/HandleString/AtIdentifierString rather than plain
strings.
- add @atproto/syntax ^0.7.2 (direct dep) for the isValid* type guards
- import Account type; drop the `let account: any` escape hatch — type it
Account | undefined and narrow before use
- account.sub -> account.did at every read site
- brand handles via isValidHandle before createAccount / getAccount:
- constructed chosen handle: invalid => 500 (would be a construction bug)
- generated random handle: invalid => throw (retryable)
- brand lookup identifiers via isValidHandle / isValidAtIdentifier, preserving
behaviour and improving one case:
- /_internal/account-by-handle: invalid id => { email: null } (= not found)
- /_internal/check-handle: invalid handle => { exists: true } so signup
blocks it up front instead of failing later at account creation
- hosted-domain resolution: invalid handle => 404 (= not found)
- auth-ui-guard login_hint match: account.sub/preferred_username ->
account.did/handle; update the binding test fixture to the new shape
pds-core build is clean and all 299 tests pass on Node 22.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bead atproto-pd5.6. oauth-provider-ui 0.4->0.8 restructured the account chooser and changed its copy (strings are now runtime-resolved i18n message ids, so the served DOM text changed): - "Another account" rebind: the aria-label changed from "Login to account that is not listed" to "Sign in to an account that is not listed", and the visible text from "Another account" to "Select another account". Match either variant (aria-label list + text list) so the rebind survives this change and degrades rather than breaks on the next copy tweak. Verified both new strings are present in the installed 0.8.9 bundle. - "Sign up" hide: 0.8 dropped the signup affordance from the account- selector page entirely (no "Sign up"/"Create account" string in the account-page bundle), so hideSignup() is now a no-op. Kept as a cheap guard and documented the change. client-css-injection.ts needs no change: it keys off `</head>` and the CSP style-src header, both markup-version-independent. The email-beside-handle enrichment matches on handle/DID text content, not aria-labels, so it is unaffected. pds-core build clean, all 299 tests pass on Node 22 (chooser test updated to assert the new + legacy selector variants). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bead atproto-pd5.8. Document the upstream bump (pds 0.5.23, oauth-provider 0.21.1, oauth-provider-ui 0.8.9) and the operator-facing Node 22 requirement for the release changelog. Full validation on Node 22: recursive build clean, typecheck clean, 1084 unit tests pass, coverage thresholds hold (stmts 58.4 / branch 57.6 / funcs 71.9 / lines 57.2), pds-core boots as ESM. e2e deferred to CI (needs a live stack). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: aafe175 The changes in this PR will be included in the next version bump. 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe project upgrades its runtime baseline to Node.js 22.19.0, adopts ESM-compatible resolution, updates AT Protocol dependencies, and strengthens typed account and handle validation. OAuth account-switching logic and tests support updated upstream fields and labels. ChangesRuntime and account upgrade
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthProvider
participant PdsCore
participant AccountStore
OAuthProvider->>PdsCore: provide typed account identity
PdsCore->>PdsCore: validate identifier and handle syntax
PdsCore->>AccountStore: resolve or create account
AccountStore-->>PdsCore: return account.did
PdsCore-->>OAuthProvider: bind resolved account
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-233 environment in ePDS
|
Coverage Report for CI Build 30900021364Coverage decreased (-0.3%) to 57.352%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the wrapped upstream AT Protocol stack used by packages/pds-core (notably @atproto/pds and @atproto/oauth-provider*) and completes the accompanying platform migration to Node.js 22 with pure ESM semantics across pds-core, plus related CI/Docker updates.
Changes:
- Bump upstream AT Protocol dependencies (
@atproto/pds→0.5.23,@atproto/oauth-provider→0.21.1,@atproto/oauth-provider-ui→0.8.9) and adapt imports/types to new subpaths + brandeddid/handle. - Migrate
pds-coreto pure ESM (NodeNext TS config,type: module,createRequire(import.meta.url)replacements). - Update infra to Node 22 (Docker images, CI workflows,
.nvmrc) and add a changeset documenting the operator requirement.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks upgraded atproto dependency graph and new transitive deps for the Node 22 stack. |
| packages/pds-core/tsconfig.json | Switches TS module settings to NodeNext for ESM correctness. |
| packages/pds-core/src/lib/preview-shared.ts | Updates createRequire usage for ESM + keeps asset loading lazy. |
| packages/pds-core/src/lib/device-accounts.ts | Repoints oauth-provider imports to new 0.21 subpaths. |
| packages/pds-core/src/index.ts | Adapts startup/package resolution for ESM; updates account typing + handle validation guards; updates internal endpoints. |
| packages/pds-core/src/chooser-enrichment.ts | Updates chooser DOM matching to oauth-provider-ui 0.8 copy/markup changes. |
| packages/pds-core/src/auth-ui-guard.ts | Updates oauth-provider imports and account field names (sub→did, preferred_username→handle). |
| packages/pds-core/src/tests/preview-test-helpers.ts | Adds explicit return type to avoid TS2742 under NodeNext resolution. |
| packages/pds-core/src/tests/chooser-enrichment.test.ts | Updates assertions to match the new chooser enrichment matching strategy. |
| packages/pds-core/src/tests/auth-ui-guard.test.ts | Updates fixtures/predicates for the new strongly-typed account fields. |
| packages/pds-core/package.json | Marks package as type: module, bumps deps, sets Node engine to 22. |
| package.json | Raises repo-wide Node engine requirement to 22. |
| Dockerfile.pds | Bumps base/runtime image to node:22-alpine. |
| Dockerfile.demo | Bumps base/runtime image to node:22-alpine. |
| Dockerfile.auth | Bumps base/runtime image to node:22-alpine. |
| .nvmrc | Pins local dev Node major to 22. |
| .github/workflows/release.yml | Updates release workflow Node version to 22. |
| .github/workflows/e2e-tests.yml | Updates e2e workflow Node version to 22. |
| .github/workflows/ci.yml | Updates CI workflows Node version to 22. |
| .changeset/atproto-upstream-upgrade.md | Adds release note + operator guidance for the upstream upgrade and Node 22 requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The ESM migration missed a `__dirname` at the favicon/public-dir setup (index.ts). `__dirname` is a CommonJS global and is undefined in pure ESM, so the pds-core container crashed at startup with `ReferenceError: __dirname is not defined` — after config parsing, which is why the local docker boot-test (which aborted earlier on missing blobstore config) and the vitest suite (vitest provides __dirname) both missed it. It surfaced only on the Railway deploy, where full env let startup reach the crash and the healthcheck then failed. Derive the module directory from `fileURLToPath(import.meta.url)` instead. Verified in-container on Node 22: the image now boots to "ePDS running" (listening on :3000) with valid env; no __dirname reference remains in the compiled dist. Build + 299 pds-core tests still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
package.json:9
engines.nodeis set to>=22, but the lockfile now includesundici@8.10.0withengines: {node: '>=22.19.0'}. This means installs/runs on Node 22.0–22.18 can fail (especially with engine-strict) even though the repo claims they’re supported. Consider bumping the minimum Node version to match the strictest transitive runtime requirement.
"engines": {
"node": ">=22"
},
packages/pds-core/package.json:9
engines.nodeis set to>=22, but the lockfile pulls inundici@8.10.0(via the upgraded atproto stack) which declaresengines: {node: '>=22.19.0'}. To avoid runtime/installation failures on early Node 22 minors, align this package’s minimum Node version with the actual dependency requirements.
"engines": {
"node": ">=22"
},
packages/pds-core/src/index.ts:1245
- When
domainisn’t a syntactically valid handle, the route returns404 NotFoundwith a “not found” message. This is inconsistent with the earlierInvalidRequestresponses for bad inputs, and it makes an invalid query param look like a valid-but-missing resource. Returning400 InvalidRequesthere would better reflect the input validation failure.
if (!isValidHandle(domain)) {
return res.status(404).json({
error: 'NotFound',
message: 'handle not found for this domain',
})
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/pds-core/src/index.ts (1)
28-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove all static imports before module initialization.
@atproto/pdsappears betweennode:imports, and the OAuth and syntax imports appear afteratprotoPdsPkgis initialized. Group all Node built-ins first, then external packages, and initializeatprotoPdsPkgafter the complete import block.Proposed import layout
-import { fileURLToPath } from 'node:url' -import { PDS, envToCfg, envToSecrets, readEnv } from '`@atproto/pds`' import { readFileSync } from 'node:fs' import { createRequire } from 'node:module' +import { fileURLToPath } from 'node:url' +import { PDS, envToCfg, envToSecrets, readEnv } from '`@atproto/pds`' +import { HandleUnavailableError } from '`@atproto/oauth-provider/errors`' +import type { Account } from '`@atproto/oauth-provider/store`' +import { + isValidHandle, + isValidAtIdentifier, + type DidString, +} from '`@atproto/syntax`' const atprotoPdsPkg = ... - -import { HandleUnavailableError } from '`@atproto/oauth-provider/errors`' -import type { Account } from '`@atproto/oauth-provider/store`' -import { ... } from '`@atproto/syntax`'As per coding guidelines: “Order imports as: Node built-ins (with node: prefix), external packages, internal workspace packages (
@certified-app/*), then local relative imports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pds-core/src/index.ts` around lines 28 - 45, Reorder the imports in the module so every node:-prefixed built-in import comes first, followed by all external package imports including `@atproto/pds`, OAuth, and syntax symbols; move the atprotoPdsPkg initialization block below the complete import section without changing its behavior.Source: Coding guidelines
packages/pds-core/src/__tests__/auth-ui-guard.test.ts (1)
781-785: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winModel optional
account.handlein the test fixture.The production predicate accepts an optional
account.handle, butbindingrequires a string and casts the field as required. This prevents tests from constructing a valid DID-only account. Change the parameter and returned type tohandle?: string, then add a DID-only case.Proposed fixture fix
- function binding(did: string, handle: string) { + function binding(did: string, handle?: string) { ... - } as unknown as { account: { did: string; handle: string } } + } as unknown as { account: { did: string; handle?: string } }Confirm that the installed
@atproto/oauth-provider0.21.1 type preserves this optional field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pds-core/src/__tests__/auth-ui-guard.test.ts` around lines 781 - 785, Update the test fixture function binding to accept an optional handle and return an account type with handle?: string, avoiding a cast that marks it required. Add a DID-only test case exercising the production predicate, and verify the installed `@atproto/oauth-provider` 0.21.1 types preserve the optional account.handle field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/tsconfig.json`:
- Around line 4-5: Update the module and moduleResolution settings in
packages/pds-core/tsconfig.json from NodeNext to Node16 to comply with the
repository TypeScript configuration rule; only change the repository rule
instead if NodeNext is intentionally required.
---
Nitpick comments:
In `@packages/pds-core/src/__tests__/auth-ui-guard.test.ts`:
- Around line 781-785: Update the test fixture function binding to accept an
optional handle and return an account type with handle?: string, avoiding a cast
that marks it required. Add a DID-only test case exercising the production
predicate, and verify the installed `@atproto/oauth-provider` 0.21.1 types
preserve the optional account.handle field.
In `@packages/pds-core/src/index.ts`:
- Around line 28-45: Reorder the imports in the module so every node:-prefixed
built-in import comes first, followed by all external package imports including
`@atproto/pds`, OAuth, and syntax symbols; move the atprotoPdsPkg initialization
block below the complete import section without changing its behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 507d0b68-61ff-46ff-a894-767542b9c91d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/atproto-upstream-upgrade.md.github/workflows/ci.yml.github/workflows/e2e-tests.yml.github/workflows/release.yml.nvmrcDockerfile.authDockerfile.demoDockerfile.pdspackage.jsonpackages/pds-core/package.jsonpackages/pds-core/src/__tests__/auth-ui-guard.test.tspackages/pds-core/src/__tests__/chooser-enrichment.test.tspackages/pds-core/src/__tests__/preview-test-helpers.tspackages/pds-core/src/auth-ui-guard.tspackages/pds-core/src/chooser-enrichment.tspackages/pds-core/src/index.tspackages/pds-core/src/lib/device-accounts.tspackages/pds-core/src/lib/preview-shared.tspackages/pds-core/tsconfig.json
Recover the coverage the strong-typing change (bead .5) dropped: the branded isValid* guards on the /_internal handle-lookup endpoints lived inline in the untested main() and pulled overall coverage down. Extract them into src/lib/identifier-guards.ts as type predicates (canLookUpAccountByHandle / canCheckHandle / canResolveHandle) — following the upstream-favicon.ts extract-and-test pattern — so the per-endpoint decision is documented and unit-tested, while the `x is HandleString`/`AtIdentifierString` predicate form preserves the narrowing getAccount() needs. Covers the non-obvious case (an invalid handle on the availability check reads as taken, not free) and the DID-vs-handle distinction between endpoints. 12 new tests; pds-core suite now 311 passing. Build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/pds-core/src/tests/identifier-guards.test.ts:39
- This test intends to verify that
canCheckHandle()rejects DIDs even when they are syntactically valid, butdid:plc:abc123is probably not a valid PLC DID. Using a valid DID here makes the assertion meaningful and avoids the test passing/failing for the wrong reason.
it('rejects a DID (this endpoint only accepts handles)', () => {
// A DID is not a HandleString, so availability-check treats it as invalid
// — which the endpoint reports as "taken", never "free".
expect(canCheckHandle('did:plc:abc123')).toBe(false)
})
Extend the identifier-guard extraction to the createAccount handle-branding sites (chosen + random handle) via isValidConstructedHandle, moving the last inline isValidHandle calls out of the untested main() and into the tested lib/identifier-guards module. index.ts no longer imports isValidHandle directly. +4 tests (16 total in identifier-guards). Build clean, full suite 1100 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (3)
package.json:9
engines.nodeis set to>=22, but the lockfile includes runtime dependencies that declareengines: { node: '>=22.19.0' }(e.g.undici@8.10.0). To avoid advertising support for Node 22.0–22.18 (which may fail at install/runtime), bump the documented engine floor to match the strictest dependency.
"engines": {
"node": ">=22"
},
packages/pds-core/package.json:9
- This package declares
engines.node: >=22, but the upgraded atproto dependency tree pulls in packages that require Node>=22.19.0(e.g.undici@8.10.0via@atproto-labs/fetch-node). To prevent users running earlier Node 22 minors, align this engine constraint with the strictest transitive requirement.
"engines": {
"node": ">=22"
},
packages/pds-core/src/auth-ui-guard.ts:387
- The JSDoc for
filterCandidateBindingsstill refers to legacysub/preferred_usernamefields, but the implementation now matchesaccount.did/account.handle. This makes the comment misleading for future maintenance—update it to the new field names to match upstream oauth-provider-api 0.7 typing.
// Account is now strongly typed (oauth-provider-api 0.7): sub -> did,
// preferred_username -> handle (handle is optional).
({ account }) => account.did === loginHint || account.handle === loginHint,
)
Address PR review (Copilot, CodeRabbit): - engines.node >=22 -> >=22.19.0 (root + pds-core) and .nvmrc 22 -> 22.19.0: the lockfile pulls undici@8.10.0 (via @atproto) which declares engines.node >=22.19.0, so >=22 would allow installs on 22.0-22.18 that break at runtime. Changeset updated to state the 22.19 floor and why. - pds-core tsconfig module/moduleResolution NodeNext -> Node16, matching the repo convention (root, auth-service, shared all use Node16). Node16 fully supports the pure-ESM build; verified tsc --build clean. Build + typecheck clean, 315 pds-core tests pass on Node 22. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address CodeRabbit review: the `atprotoPdsPkg` version-read sat between the `node:` import group and the external `@atproto/*` imports. Move the two `node:` imports (fs, module) up with the other built-ins, keep all static imports together, and relocate the `atprotoPdsPkg` initialization below the import block. `dotenv.config()` / `applyPdsPortFallback()` stay at the top by design (they must run before readEnv, per the existing comments). No behaviour change; build + typecheck clean, all pds-core tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
(reply generated by Claude Opus via Claude Code) Addressing the CodeRabbit review-summary nitpick on Fixed in 5e4070d. Moved the The other summary nitpick (the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/pds-core/src/lib/identifier-guards.ts:32
canLookUpAccountByHandleaccepts both handles and DIDs (it delegates toisValidAtIdentifier), so the name is misleading and makes call sites harder to understand. Consider renaming to something identifier-focused (e.g.canLookUpAccountByIdentifier/canLookUpAccountByAtIdentifier) and updating the import/uses accordingly.
export function canLookUpAccountByHandle(
identifier: string,
): identifier is AtIdentifierString {
return isValidAtIdentifier(identifier)
}
packages/pds-core/package.json:20
@atproto/cryptois listed as a direct dependency, but there are no references to it anywhere inpackages/pds-core/src(and the lockfile already pulls in newer@atproto/cryptotransitively via@atproto/pds). Keeping this unused direct dependency increases install surface and risks multiple@atproto/cryptoversions in the tree. Remove it if it's not intentionally pinned/used.
"dependencies": {
"@atproto/crypto": "^0.4.5",
"@atproto/oauth-provider": "^0.21.1",
"@atproto/oauth-provider-ui": "^0.8.9",
"@atproto/pds": "^0.5.23",
"@atproto/syntax": "^0.7.2",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/pds-core/src/index.ts:481
res.status(500).send('Account resolution failed')returns plain text, but other error paths in this callback userenderError(...)HTML. For consistency (and to ensure the browser gets an HTML page), return the same rendered error here too.
res.status(500).send('Account resolution failed')
packages/pds-core/src/lib/identifier-guards.ts:2
- The header comment says these guards are for the
/_internalhandle-lookup endpoints, butcanResolveHandle()is also used by the public/tls-checkroute (Caddy on-demand TLS). Update the comment so it accurately reflects where these guards are used.
* Branded-identifier guards for the `/_internal` handle-lookup endpoints.
packages/pds-core/src/index.ts:358
res.status(500).send('Invalid handle')returns a plain-text 500 while other failure paths in this handler return the HTML error page viarenderError(...). Using the same HTML response here keeps content-type and user experience consistent.
This issue also appears on line 481 of the same file.
res.status(500).send('Invalid handle')
packages/pds-core/package.json:16
@atproto/cryptois listed as a direct dependency of@certified-app/pds-core, but it doesn't appear to be imported/used anywhere in this package. If it's no longer needed after the upstream upgrade, removing it would avoid pinning an extra unused version in the lockfile.
"@atproto/crypto": "^0.4.5",
aspiers
commented
Aug 4, 2026
aspiers
commented
Aug 4, 2026
…ail claim Address review (aspiers): - identifier-guards.test.ts: use a proper `did:plc:` value (did:plc:7iza6de2dwap2sbkpav7c6c6) instead of the abc123 placeholder at both fixture sites. Still a valid DID per @atproto/syntax, so the assertions are unchanged — just realistic. - changeset: email verification is already provided by ePDS's auth-service, so it is not an upstream gain. Reframe it as unchanged (alongside OTP sign-in) rather than listing it as new; keep handle change + deactivate/delete, which the upstream account-manager UI genuinely adds. Build + 315 pds-core tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
.github/workflows/ci.yml:35
- Same as the format job: since
engines.nodeis>=22.19.0, pin this workflow job to 22.19.0 (or include it in a version matrix) so the minimum supported runtime is tested in CI.
- uses: actions/setup-node@v4
with:
node-version: 22
cache: pnpm
.github/workflows/ci.yml:47
- Same as other CI jobs: consider pinning
node-versionto 22.19.0 (or adding a matrix including it) to ensure the declared minimum supported Node version stays valid.
- uses: actions/setup-node@v4
with:
node-version: 22
cache: pnpm
.github/workflows/ci.yml:59
- Same as other CI jobs: to validate the stated support floor (
engines.node >=22.19.0), pin this job’s Node version to 22.19.0 (or include it in a version matrix) rather than tracking the latest 22.x.
- uses: actions/setup-node@v4
with:
node-version: 22
cache: pnpm
packages/pds-core/package.json:17
@atproto/cryptois listed as a direct dependency of@certified-app/pds-core, but there are no imports of it anywhere in this package. Keeping it here pins an older@atproto/crypto@0.4.xalongside the upgraded upstream stack (which pulls@atproto/crypto@0.5.xtransitively), increasing duplication and making future upgrades harder to reason about. Remove the unused direct dependency (or bump it to match upstream if it’s intentionally required).
"dependencies": {
"@atproto/crypto": "^0.4.5",
"@atproto/oauth-provider": "^0.21.1",
.github/workflows/ci.yml:23
- The repo now declares
engines.node >=22.19.0, but CI usesnode-version: 22(latest 22.x). That won’t exercise the minimum supported Node version and could allow accidental use of APIs introduced after 22.19.0. Consider pinning CI to22.19.0(or adding a matrix that includes the minimum) so compatibility with the stated floor is continuously verified.
This issue also appears in the following locations of the same file:
- line 32
- line 44
- line 56
- uses: actions/setup-node@v4
with:
node-version: 22
cache: pnpm
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
…as errors Three review points from PR #234. Add an optional trailing argument to the backfill that scopes the run to addresses containing it, case-insensitively -- "@gmail.com" for a domain, a full address for one account. Operators can then work through a deployment in batches or repair a single user instead of being forced to process the whole table in one go. No argument still means every account: "no filter" must never be read as "match nothing", or a mistyped invocation would silently do nothing and look like a clean run. The report names the filter, because otherwise "0 account(s)" cannot be distinguished between nothing-left-to-do and your-filter-matched-nothing. Raise the failed-confirmation log from warn to error. Swallowing the exception keeps the user signed in, but the account is left claiming an unverified address to every relying party until some later sign-in happens to succeed, and the operator has no other signal it occurred. Self-healing is not the same as harmless, so it belongs at error-level alerting. Add End users to the changeset's audience list. The claim is user-visible: once #233 lands, the PDS /account route stops telling people to verify an address they have already confirmed. Verified against a real account.sqlite built by upstream's migrator: the domain filter matched mixed-case addresses, left non-matching accounts untouched, a later unfiltered run picked up the remainder, and no email_token rows were left behind.
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
…as errors Three review points from PR #234. Add an optional trailing argument to the backfill that scopes the run to addresses containing it, case-insensitively -- "@gmail.com" for a domain, a full address for one account. Operators can then work through a deployment in batches or repair a single user instead of being forced to process the whole table in one go. No argument still means every account: "no filter" must never be read as "match nothing", or a mistyped invocation would silently do nothing and look like a clean run. The report names the filter, because otherwise "0 account(s)" cannot be distinguished between nothing-left-to-do and your-filter-matched-nothing. Raise the failed-confirmation log from warn to error. Swallowing the exception keeps the user signed in, but the account is left claiming an unverified address to every relying party until some later sign-in happens to succeed, and the operator has no other signal it occurred. Self-healing is not the same as harmless, so it belongs at error-level alerting. Add End users to the changeset's audience list. The claim is user-visible: once #233 lands, the PDS /account route stops telling people to verify an address they have already confirmed. Verified against a real account.sqlite built by upstream's migrator: the domain filter matched mixed-case addresses, left non-matching accounts untouched, a later unfiltered run picked up the remainder, and no email_token rows were left behind.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Upgrades the wrapped atproto stack in
pds-coreto the latest upstream, gated behind a Node 22 + pure-ESM platform migration. Delivers beads epicatproto-pd5(8 beads).What changed
@atproto/pds^0.4.209^0.5.23@atproto/oauth-provider^0.15.9^0.21.1@atproto/oauth-provider-ui^0.4.3^0.8.9engines.node>=20>=22Commits (one per bead, in dependency order):
build(pds-core): move to Node 22 + pure ESM—type: module,engines.node>=22, NodeNext,require.resolve→createRequire(import.meta.url), Docker/CI node 20→22,.nvmrc.build(pds-core): bump @atproto deps to latest upstream.refactor(pds-core): repoint oauth-provider imports to 0.21 subpaths— 0.20 removed the barrel entrypoint; repointed to/errors,/constants,/store,/provider.fix(pds-core): adopt strongly-typed Account did/handle—Accountmoved to@atproto/oauth-provider-apiwithsub/preferred_username→ brandeddid/handle; added@atproto/syntaxforisValid*guards.fix(pds-core): match chooser enrichment to oauth-provider-ui 0.8 markup— aria-label/text copy changed; the "Another account" → auth-service rebind now matches new + legacy variants.docs(changeset): atproto upstream upgrade + Node 22 requirement.Two beads were no-code validations:
as any/as unknown asreach-ins (requestManager,clientManager,deviceManager.store,accountManager.*,ctx.accountManager.db.db) confirmed intact against the new upstream.d.tsat signature + field-shape level.onSignInFailedreconcile — found not applicable:onSignInFailedfires on oauth-provider's password sign-in, which ePDS bypasses (passwordless). ePDS OTP logging is on the separate better-authemailOTPpath. No duplication; nothing to reconcile. (@atproto/pdsalso hardcodes its hooks object, so the hook is unreachable regardless.)Validation (Node 22)
pnpm --recursive build— cleanpnpm typecheck— cleanpnpm test— 1084 passed / 73 filespds-core/dist/index.jsboots as ESMOperator note
Requires Node.js 22 (was 20). Docker images already use
node:22-alpine. A nativebetter-sqlite3rebuild is needed on an in-place Node switch (pnpm rebuild better-sqlite3); a clean install on Node 22 handles it.Out of scope: the
constantTimelog warning (unchanged upstream; silence separately).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Compatibility