karma/hyper 139 choosing handle on sign up#12
karma/hyper 139 choosing handle on sign up#12Kzoeps wants to merge 5 commits intohypercerts-org:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a new handle selection flow for new users during authentication. Adds a choose-handle route module with validation, integrates it into auth-service, updates the authentication completion flow to route new users to handle selection, and extends PDS with internal handle-checking and inactivity-pinging endpoints. Updates callback signing to support handle parameters. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant AS as Auth Service
participant PDS as PDS Service
participant DB as Database
User->>AS: GET /auth/choose-handle
AS->>AS: Validate auth_flow cookie
AS->>DB: Load flow row
AS->>AS: Verify better-auth session
AS->>AS: Check PDS DID exists
alt DID exists (existing user)
AS-->>User: Redirect /auth/complete
else No DID (new user)
AS->>PDS: Ping inactivity timer
AS-->>User: Render handle picker UI
end
User->>User: Enter handle
User->>AS: POST /auth/choose-handle
AS->>AS: Validate handle format (HANDLE_REGEX)
AS->>AS: Check against RESERVED_HANDLES
alt Invalid format or reserved
AS-->>User: Render picker with error
else Valid
AS->>PDS: GET /_internal/check-handle
PDS-->>AS: exists: boolean
alt Handle taken
AS-->>User: Render picker with "taken" error
else Available
AS->>AS: Sign callback payload (with handle)
AS->>DB: Delete auth_flow row
AS->>AS: Clear auth_flow cookie
AS-->>User: Redirect to PDS epds-callback (signed)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Kzoeps
force-pushed
the
karma/hyper-139-choosing-handle-on-sign-up
branch
from
9c739f3 to
aaff3c4
Compare
|
@Kzoeps is attempting to deploy a commit to the Hypercerts Foundation Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 295-297: The auth flow and cookie are being deleted prematurely in
choose-handle (ctx.db.deleteAuthFlow(flowId) and
res.clearCookie(AUTH_FLOW_COOKIE)), which prevents pds-core's createAccount
collision recovery that redirects back with ?error=handle_taken; stop deleting
the auth_flow row and clearing the AUTH_FLOW_COOKIE here and instead only
delete/clear them after the external createAccount callback definitively
succeeds (the success branch that confirms account creation), leaving TTL to
expire stale flows otherwise so the handle_taken redirect can recover correctly.
- Around line 279-306: The new-user login isn't recorded so subsequent logins
look like first-time; update getFlowAndSession(...) to return/passthrough
clientId into this route and, before redirecting (i.e., before
ctx.db.deleteAuthFlow(flowId)/res.clearCookie and after you compute
callbackParams/signCallback), call ctx.db.recordClientLogin(email, clientId)
using the clientId you plumbed through; ensure the code checks clientId exists
and use the same email variable so hasClientLogin(email, clientId) later will
see the recorded entry.
- Around line 62-64: Replace the silent fallbacks in choose-handle.ts: stop
using ctx.config.pdsPublicUrl and empty string defaults; instead read pdsUrl
from process.env.PDS_INTERNAL_URL and internalSecret from
process.env.EPDS_INTERNAL_SECRET, and if either is missing throw a clear startup
error (e.g., throw new Error(...)) so the service fails fast; keep handleDomain
= ctx.config.pdsHostname as-is but ensure any subsequent internal requests use
the x-internal-secret header with internalSecret when calling pds-core (locate
the pdsUrl and internalSecret variables in this file to implement the checks).
- Around line 156-169: The current fetch to
`${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`
only catches network errors but ignores non-OK HTTP responses; change the try
block to capture the response (e.g., const pingRes = await fetch(...)), then if
(!pingRes.ok) treat it as an expired flow by logging (use logger.debug with
err/context) and short-circuit to the same session-expired error handling/render
used elsewhere in choose-handle.ts instead of continuing to render the picker;
keep the same headers (internalSecret) and AbortSignal.timeout(3000) behavior.
- Around line 224-235: The local variable handleAvailable in choose-handle.ts is
initialized to false but always overwritten before use; remove the dead
initializer to satisfy no-useless-assignment. Change the declaration "let
handleAvailable = false" to an uninitialized declaration (e.g., "let
handleAvailable" or "let handleAvailable: boolean") so subsequent assignments
from the fetch/response logic remain the source of truth; keep references to
handleAvailable in the same function unchanged.
In `@packages/auth-service/src/routes/complete.ts`:
- Around line 91-92: The code currently falls back to ctx.config.pdsPublicUrl
and an empty string for internal credentials; change the lookup in
packages/auth-service/src/routes/complete.ts to require the internal wiring like
the choose-handle route by reading process.env.PDS_INTERNAL_URL and
process.env.EPDS_INTERNAL_SECRET directly, throw or assert (startup error) if
either is missing instead of falling back, and ensure the code that calls
pds-core uses the internal URL variable (pdsUrl) and sends the x-internal-secret
header with internalSecret when making the service-to-service request.
In `@packages/pds-core/src/index.ts`:
- Around line 466-470: The current catch block swallows errors from
pds.ctx.accountManager.getAccount(handle) and returns { exists: false }, which
incorrectly treats lookup errors as available handles; change the catch to
fail-closed by logging the caught error (include the error object) and returning
an error response (e.g. res.status(500).json({ error: 'account lookup failed'
})) or rethrow/pass the error to the next error handler instead of res.json({
exists: false }); update the catch surrounding
pds.ctx.accountManager.getAccount(handle) and res.json({ exists: !!account })
accordingly.
- Around line 198-208: The current catch around createAccount() treats every
failure as a handle collision; change it to detect actual uniqueness/conflict
errors (e.g., a specific error class, error.code === 'CONFLICT' or 409,
UniqueConstraint/Sequelize/Mongo index error, or a dedicated
isHandleCollisionError(createErr) helper) and only perform the logger.warn +
res.redirect to /auth/choose-handle when that check returns true for
chosenHandle; for all other createErr cases, log as logger.error with the full
error and return a 5xx response (e.g., res.status(500).send or next(createErr))
so invite-code, datastore, and provider errors are surfaced instead of being
masked as handle_taken. Use the createAccount, chosenHandle,
logger.warn/logger.error, authHostname, and res.redirect/res.status paths to
locate and implement this branching.
In `@packages/shared/src/__tests__/crypto.test.ts`:
- Line 11: The import VerifyCallbackResult is unused in the test file; remove it
from the import list so ESLint no longer flags it. Locate the import statement
that includes VerifyCallbackResult (alongside other imports) in
packages/shared/src/__tests__/crypto.test.ts and delete only the
VerifyCallbackResult identifier, leaving the other imports intact; rely on the
inferred return type from verifyCallback() instead of the explicit type.
In `@packages/shared/src/crypto.ts`:
- Around line 125-129: The length-only check before decoding signatures is
insufficient; update the verification flow (e.g., in the function that currently
does `if (sig.length !== expected.length)`) to validate that `sig` is a valid
hex string (use a hex-regex like `/^[0-9a-fA-F]+$/`) and that its string length
matches `expected.length` before calling `Buffer.from(sig, 'hex')`; additionally
ensure the decoded buffers have equal byte lengths before calling
`crypto.timingSafeEqual` (and return `{ valid: false }` on any validation
failure) so `Buffer.from`/`timingSafeEqual` cannot throw on malformed input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af5bda66-f147-4984-a7a2-1d748e7a2dc7
📒 Files selected for processing (10)
.beads/issues.jsonlpackages/auth-service/src/__tests__/choose-handle.test.tspackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/index.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/pds-core/src/index.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/crypto.tspackages/shared/src/index.ts
| const pdsUrl = process.env.PDS_INTERNAL_URL || ctx.config.pdsPublicUrl | ||
| const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? '' | ||
| const handleDomain = ctx.config.pdsHostname |
There was a problem hiding this comment.
Fail fast when the internal PDS wiring is missing.
Falling back to ctx.config.pdsPublicUrl and '' turns a startup misconfiguration into public-network calls or guaranteed 401/403s from every internal endpoint. Require the internal URL and secret here instead of silently booting in a broken state.
🛠️ Proposed fix
- const pdsUrl = process.env.PDS_INTERNAL_URL || ctx.config.pdsPublicUrl
- const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? ''
+ const pdsUrl = process.env.PDS_INTERNAL_URL
+ const internalSecret = process.env.EPDS_INTERNAL_SECRET
+ if (!pdsUrl || !internalSecret) {
+ throw new Error('PDS_INTERNAL_URL and EPDS_INTERNAL_SECRET must be set')
+ }As per coding guidelines Auth service must use PDS_INTERNAL_URL to reach pds-core over the internal network and Internal service-to-service calls must use x-internal-secret header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/choose-handle.ts` around lines 62 - 64,
Replace the silent fallbacks in choose-handle.ts: stop using
ctx.config.pdsPublicUrl and empty string defaults; instead read pdsUrl from
process.env.PDS_INTERNAL_URL and internalSecret from
process.env.EPDS_INTERNAL_SECRET, and if either is missing throw a clear startup
error (e.g., throw new Error(...)) so the service fails fast; keep handleDomain
= ctx.config.pdsHostname as-is but ensure any subsequent internal requests use
the x-internal-secret header with internalSecret when calling pds-core (locate
the pdsUrl and internalSecret variables in this file to implement the checks).
| try { | ||
| await fetch( | ||
| `${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`, | ||
| { | ||
| headers: { 'x-internal-secret': internalSecret }, | ||
| signal: AbortSignal.timeout(3000), | ||
| }, | ||
| ) | ||
| } catch (err) { | ||
| logger.debug( | ||
| { err }, | ||
| 'Failed to ping request_uri on choose-handle — ignoring', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Treat a non-OK ping as an expired flow.
fetch() only throws on network failures. If packages/pds-core/src/index.ts Lines 490-504 returns 404 { error: 'request_expired' } or 503, this handler still renders the picker and the user only discovers the failure after submitting. Check pingRes.ok and short-circuit with the same session-expired error page.
🛠️ Proposed fix
try {
- await fetch(
+ const pingRes = await fetch(
`${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`,
{
headers: { 'x-internal-secret': internalSecret },
signal: AbortSignal.timeout(3000),
},
)
+ if (!pingRes.ok) {
+ logger.warn(
+ { status: pingRes.status, requestUri: result.flow.requestUri },
+ 'Failed to extend request_uri on choose-handle',
+ )
+ res
+ .status(400)
+ .type('html')
+ .send(renderError('Session expired, please start over'))
+ return
+ }
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await fetch( | |
| `${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`, | |
| { | |
| headers: { 'x-internal-secret': internalSecret }, | |
| signal: AbortSignal.timeout(3000), | |
| }, | |
| ) | |
| } catch (err) { | |
| logger.debug( | |
| { err }, | |
| 'Failed to ping request_uri on choose-handle — ignoring', | |
| ) | |
| } | |
| try { | |
| const pingRes = await fetch( | |
| `${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`, | |
| { | |
| headers: { 'x-internal-secret': internalSecret }, | |
| signal: AbortSignal.timeout(3000), | |
| }, | |
| ) | |
| if (!pingRes.ok) { | |
| logger.warn( | |
| { status: pingRes.status, requestUri: result.flow.requestUri }, | |
| 'Failed to extend request_uri on choose-handle', | |
| ) | |
| res | |
| .status(400) | |
| .type('html') | |
| .send(renderError('Session expired, please start over')) | |
| return | |
| } | |
| } catch (err) { | |
| logger.debug( | |
| { err }, | |
| 'Failed to ping request_uri on choose-handle — ignoring', | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/choose-handle.ts` around lines 156 - 169,
The current fetch to
`${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`
only catches network errors but ignores non-OK HTTP responses; change the try
block to capture the response (e.g., const pingRes = await fetch(...)), then if
(!pingRes.ok) treat it as an expired flow by logging (use logger.debug with
err/context) and short-circuit to the same session-expired error handling/render
used elsewhere in choose-handle.ts instead of continuing to render the picker;
keep the same headers (internalSecret) and AbortSignal.timeout(3000) behavior.
| // Step 5: Sign callback with handle local part included in HMAC payload. | ||
| // Only the local part (e.g. 'alice') is sent — pds-core appends its own | ||
| // trusted handleDomain, eliminating any possibility of domain mismatch. | ||
| const callbackParams = { | ||
| request_uri: flow.requestUri, | ||
| email, | ||
| approved: '1', | ||
| new_account: '1', | ||
| handle: rawHandle, | ||
| } | ||
| const { sig, ts } = signCallback( | ||
| callbackParams, | ||
| ctx.config.epdsCallbackSecret, | ||
| ) | ||
| const params = new URLSearchParams({ ...callbackParams, ts, sig }) | ||
|
|
||
| // Step 6: Cleanup — delete auth_flow row and clear cookie | ||
| ctx.db.deleteAuthFlow(flowId) | ||
| res.clearCookie(AUTH_FLOW_COOKIE) | ||
|
|
||
| logger.info( | ||
| { email, flowId, fullHandle }, | ||
| 'Handle chosen: redirecting to epds-callback', | ||
| ) | ||
| res.redirect( | ||
| 303, | ||
| `${ctx.config.pdsPublicUrl}/oauth/epds-callback?${params.toString()}`, | ||
| ) |
There was a problem hiding this comment.
Record the initial client login on the new-user path.
packages/auth-service/src/routes/complete.ts Lines 97-99 decide repeat consent using ctx.db.hasClientLogin(email, clientId), but this route never seeds that table for a newly created account. The next auth for the same client will still look “first time” and send the user through consent once more. Please plumb clientId through getFlowAndSession() and call ctx.db.recordClientLogin(...) before redirecting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/choose-handle.ts` around lines 279 - 306,
The new-user login isn't recorded so subsequent logins look like first-time;
update getFlowAndSession(...) to return/passthrough clientId into this route
and, before redirecting (i.e., before
ctx.db.deleteAuthFlow(flowId)/res.clearCookie and after you compute
callbackParams/signCallback), call ctx.db.recordClientLogin(email, clientId)
using the clientId you plumbed through; ensure the code checks clientId exists
and use the same email variable so hasClientLogin(email, clientId) later will
see the recorded entry.
There was a problem hiding this comment.
Skipping this because currently its a bug here that
- new users dont get shown the consent page (which they should be shown)
- The record client login will need to be done on the consent page instead of the choose handle or otp-verification page.
So the pr #11 would already duplicate this and would just be more work
There was a problem hiding this comment.
@Kzoeps, that makes sense — recording the client login before the consent page is shown would be semantically incorrect (the user hasn't actually consented yet). The right place is after the user grants consent on the consent page, so tying this to PR #11 is the correct approach. I'll defer this.
Would you like me to open a follow-up GitHub issue to track:
- New users not being shown the consent page, and
- Ensuring
recordClientLoginis called on the consent page (post-consent)?
✏️ Learnings added
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 12
File: packages/auth-service/src/routes/choose-handle.ts:279-306
Timestamp: 2026-03-12T07:21:23.431Z
Learning: In `packages/auth-service/src/routes/choose-handle.ts` and `packages/auth-service/src/routes/otp-verification.ts` (hypercerts-org/ePDS): Do NOT call `ctx.db.recordClientLogin(email, clientId)` here. `recordClientLogin` must be called only on the consent page (after the user grants consent), not on the choose-handle or OTP-verification step. There is also a known bug where new users are not being shown the consent page — this is tracked in PR `#11`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-26T19:06:17.937Z
Learning: Applies to **/*.{ts,tsx} : All epds-callback redirects must be HMAC-SHA256 signed using `signCallback()` / `verifyCallback()` from `certified-app/shared`
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-26T19:06:17.937Z
Learning: Applies to **/src/*router*.ts : Add file-level JSDoc block at the top of route files explaining the route's purpose, flow steps, and non-obvious behaviour
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-26T19:06:17.937Z
Learning: Applies to **/auth-service/**/*(main|index|app|start).ts : Call `runBetterAuthMigrations()` explicitly on startup — better-auth does not auto-migrate
| // Step 6: Cleanup — delete auth_flow row and clear cookie | ||
| ctx.db.deleteAuthFlow(flowId) | ||
| res.clearCookie(AUTH_FLOW_COOKIE) |
There was a problem hiding this comment.
Keep the auth flow until pds-core accepts the chosen handle.
packages/pds-core/src/index.ts Lines 198-208 redirects back to /auth/choose-handle?error=handle_taken when the final createAccount() collides. Deleting the auth_flow row and cookie here turns that recovery path into an immediate “Session expired”. Let TTL cleanup handle it, or clear the flow only after the callback definitively succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/choose-handle.ts` around lines 295 - 297,
The auth flow and cookie are being deleted prematurely in choose-handle
(ctx.db.deleteAuthFlow(flowId) and res.clearCookie(AUTH_FLOW_COOKIE)), which
prevents pds-core's createAccount collision recovery that redirects back with
?error=handle_taken; stop deleting the auth_flow row and clearing the
AUTH_FLOW_COOKIE here and instead only delete/clear them after the external
createAccount callback definitively succeeds (the success branch that confirms
account creation), leaving TTL to expire stale flows otherwise so the
handle_taken redirect can recover correctly.
| const pdsUrl = process.env.PDS_INTERNAL_URL || ctx.config.pdsPublicUrl | ||
| const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? '' |
There was a problem hiding this comment.
Use the internal PDS wiring on /auth/complete too.
This is still a service-to-service lookup, so falling back to ctx.config.pdsPublicUrl and '' makes a missing internal configuration look like a runtime auth failure instead of a startup error. Require the internal URL and secret here the same way as the new choose-handle route should.
As per coding guidelines Auth service must use PDS_INTERNAL_URL to reach pds-core over the internal network and Internal service-to-service calls must use x-internal-secret header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/complete.ts` around lines 91 - 92, The code
currently falls back to ctx.config.pdsPublicUrl and an empty string for internal
credentials; change the lookup in packages/auth-service/src/routes/complete.ts
to require the internal wiring like the choose-handle route by reading
process.env.PDS_INTERNAL_URL and process.env.EPDS_INTERNAL_SECRET directly,
throw or assert (startup error) if either is missing instead of falling back,
and ensure the code that calls pds-core uses the internal URL variable (pdsUrl)
and sends the x-internal-secret header with internalSecret when making the
service-to-service request.
| } catch (createErr: unknown) { | ||
| // Handle collision — redirect back to choose-handle page | ||
| logger.warn( | ||
| { err: createErr, handle: chosenHandle }, | ||
| 'chosen handle collision at createAccount', | ||
| ) | ||
| res.redirect( | ||
| 303, | ||
| `https://${authHostname}/auth/choose-handle?error=handle_taken`, | ||
| ) | ||
| return |
There was a problem hiding this comment.
Don’t collapse every createAccount() failure into handle_taken.
This catch also hides invite-code misconfiguration, datastore failures, and provider errors as a collision. Redirect back to /auth/choose-handle only for the actual uniqueness/conflict case; everything else should be logged as an error and surfaced as a 5xx so the user doesn’t get a false “handle taken” loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pds-core/src/index.ts` around lines 198 - 208, The current catch
around createAccount() treats every failure as a handle collision; change it to
detect actual uniqueness/conflict errors (e.g., a specific error class,
error.code === 'CONFLICT' or 409, UniqueConstraint/Sequelize/Mongo index error,
or a dedicated isHandleCollisionError(createErr) helper) and only perform the
logger.warn + res.redirect to /auth/choose-handle when that check returns true
for chosenHandle; for all other createErr cases, log as logger.error with the
full error and return a 5xx response (e.g., res.status(500).send or
next(createErr)) so invite-code, datastore, and provider errors are surfaced
instead of being masked as handle_taken. Use the createAccount, chosenHandle,
logger.warn/logger.error, authHostname, and res.redirect/res.status paths to
locate and implement this branching.
| try { | ||
| const account = await pds.ctx.accountManager.getAccount(handle) | ||
| res.json({ exists: !!account }) | ||
| } catch { | ||
| res.json({ exists: false }) |
There was a problem hiding this comment.
Fail closed when the handle lookup errors.
Returning { exists: false } from the catch makes transient account-manager failures look like an available handle. The auth-service will then approve a handle that cannot actually be created.
🛠️ Proposed fix
- } catch {
- res.json({ exists: false })
+ } catch (err) {
+ logger.error({ err, handle }, 'Failed to check handle availability')
+ res.status(503).json({ error: 'handle_check_failed' })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const account = await pds.ctx.accountManager.getAccount(handle) | |
| res.json({ exists: !!account }) | |
| } catch { | |
| res.json({ exists: false }) | |
| try { | |
| const account = await pds.ctx.accountManager.getAccount(handle) | |
| res.json({ exists: !!account }) | |
| } catch (err) { | |
| logger.error({ err, handle }, 'Failed to check handle availability') | |
| res.status(503).json({ error: 'handle_check_failed' }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pds-core/src/index.ts` around lines 466 - 470, The current catch
block swallows errors from pds.ctx.accountManager.getAccount(handle) and returns
{ exists: false }, which incorrectly treats lookup errors as available handles;
change the catch to fail-closed by logging the caught error (include the error
object) and returning an error response (e.g. res.status(500).json({ error:
'account lookup failed' })) or rethrow/pass the error to the next error handler
instead of res.json({ exists: false }); update the catch surrounding
pds.ctx.accountManager.getAccount(handle) and res.json({ exists: !!account })
accordingly.
packages/shared/src/crypto.ts
Outdated
| if (sig.length !== expected.length) return { valid: false } | ||
| const isValid = crypto.timingSafeEqual( | ||
| Buffer.from(expected, 'hex'), | ||
| Buffer.from(sig, 'hex'), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what happens when Buffer.from(str, 'hex')receives non-hex characters or odd-length input, and doescrypto.timingSafeEqual() throw when the buffer lengths differ?
💡 Result:
-
Buffer.from(str, 'hex')with non-hex or odd-length input: it does not throw; it truncates.- Decoding stops (and the result is truncated) at the first non-hex character (e.g.
'1ag123'→<Buffer 1a>). [1] - If the string has an odd number of hex digits, the final single nibble is dropped (e.g.
'1a7'→<Buffer 1a>). [1]
- Decoding stops (and the result is truncated) at the first non-hex character (e.g.
-
crypto.timingSafeEqual(a, b)with different lengths: yes, it throws an error ifaandbhave different byte lengths. [2]
Sources: [1] Node.js Buffer docs, 'hex' truncation examples; [2] Node.js Crypto docs, crypto.timingSafeEqual() requirement/throw on differing lengths.
Validate hex format before decoding signatures.
The length check sig.length === expected.length only compares string lengths, not hex validity. A 64-character string containing non-hex characters (e.g., '1ag123...') will pass this check but truncate when decoded with Buffer.from(sig, 'hex'), causing crypto.timingSafeEqual() to throw when the resulting buffer length mismatches. This creates an unhandled exception path on the public callback route.
🛠️ Proposed fix
- if (sig.length !== expected.length) return { valid: false }
+ const normalizedSig = sig.toLowerCase()
+ if (!/^[0-9a-f]{64}$/.test(normalizedSig)) return { valid: false }
const isValid = crypto.timingSafeEqual(
Buffer.from(expected, 'hex'),
- Buffer.from(sig, 'hex'),
+ Buffer.from(normalizedSig, 'hex'),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (sig.length !== expected.length) return { valid: false } | |
| const isValid = crypto.timingSafeEqual( | |
| Buffer.from(expected, 'hex'), | |
| Buffer.from(sig, 'hex'), | |
| ) | |
| const normalizedSig = sig.toLowerCase() | |
| if (!/^[0-9a-f]{64}$/.test(normalizedSig)) return { valid: false } | |
| const isValid = crypto.timingSafeEqual( | |
| Buffer.from(expected, 'hex'), | |
| Buffer.from(normalizedSig, 'hex'), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/crypto.ts` around lines 125 - 129, The length-only check
before decoding signatures is insufficient; update the verification flow (e.g.,
in the function that currently does `if (sig.length !== expected.length)`) to
validate that `sig` is a valid hex string (use a hex-regex like
`/^[0-9a-fA-F]+$/`) and that its string length matches `expected.length` before
calling `Buffer.from(sig, 'hex')`; additionally ensure the decoded buffers have
equal byte lengths before calling `crypto.timingSafeEqual` (and return `{ valid:
false }` on any validation failure) so `Buffer.from`/`timingSafeEqual` cannot
throw on malformed input.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/auth-service/src/routes/choose-handle.ts (4)
73-77:⚠️ Potential issue | 🟠 MajorRecord the initial client login on the new-user path.
/auth/completesuppresses repeat consent viahasClientLogin(email, clientId), but this route never seeds that table after a successful handle choice. The next authorization for the same client will still look first-time. PassclientIdthroughgetFlowAndSession()and callctx.db.recordClientLogin(email, clientId)before redirecting.Also applies to: 279-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 73 - 77, The choose-handle flow never records the initial client login, so update getFlowAndSession(...) to accept and propagate clientId (add clientId to its return shape) and in the choose-handle handler (where you currently call getFlowAndSession and build the redirect) invoke ctx.db.recordClientLogin(email, clientId) after a successful handle choice and before redirecting; ensure you pass the same clientId through both the original caller and the returned flow object so hasClientLogin sees the prior login on subsequent /auth/complete calls.
62-63:⚠️ Potential issue | 🟠 MajorRequire internal PDS config instead of public fallbacks.
Falling back to
ctx.config.pdsPublicUrland''turns a startup misconfiguration into public-network calls or guaranteed 401/403s from the internal endpoints. Fail fast if either env var is missing.🛠️ Suggested fix
- const pdsUrl = process.env.PDS_INTERNAL_URL || ctx.config.pdsPublicUrl - const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? '' + const pdsUrl = process.env.PDS_INTERNAL_URL + const internalSecret = process.env.EPDS_INTERNAL_SECRET + if (!pdsUrl || !internalSecret) { + throw new Error('PDS_INTERNAL_URL and EPDS_INTERNAL_SECRET must be set') + }As per coding guidelines
Auth service must use PDS_INTERNAL_URL to reach pds-core over the internal networkandInternal service-to-service calls must use x-internal-secret header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 62 - 63, Replace the permissive fallbacks for internal PDS access with strict required configuration: stop using ctx.config.pdsPublicUrl and the empty-string fallback and instead require process.env.PDS_INTERNAL_URL and process.env.EPDS_INTERNAL_SECRET to be present (the variables referenced as pdsUrl and internalSecret in choose-handle.ts). If either is missing, fail fast by throwing an error or exiting during initialization so the service does not attempt public-network calls or calls without the x-internal-secret header; ensure subsequent calls use pdsUrl and include internalSecret in the x-internal-secret header for internal service-to-service requests.
156-163:⚠️ Potential issue | 🟠 MajorTreat non-OK ping responses as expired flows.
fetch()only throws on transport failures. If/_internal/ping-requestreturns404 request_expiredor503, this still renders the picker and the user only finds out after submitting. Short-circuit to the same session-expired page when!pingRes.ok.🛠️ Suggested fix
- await fetch( + const pingRes = await fetch( `${pdsUrl}/_internal/ping-request?request_uri=${encodeURIComponent(result.flow.requestUri)}`, { headers: { 'x-internal-secret': internalSecret }, signal: AbortSignal.timeout(3000), }, ) + if (!pingRes.ok) { + logger.debug( + { status: pingRes.status, requestUri: result.flow.requestUri }, + 'Failed to extend request_uri on choose-handle', + ) + res + .status(400) + .type('html') + .send(renderError('Session expired, please start over')) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 156 - 163, The fetch call that pings the PDS currently ignores HTTP errors because fetch only throws on network failures; capture the response (e.g., const pingRes = await fetch(...)) and check pingRes.ok, and when it's false treat the flow as expired (same path you use for expired sessions — e.g., return/render the session-expired handling used elsewhere or throw a FlowExpired error) instead of letting the code continue; update the fetch invocation that uses pdsUrl, result.flow.requestUri, internalSecret and AbortSignal.timeout(3000) to perform this ok-check and short-circuit.
295-297:⚠️ Potential issue | 🟠 MajorKeep the auth flow alive until pds-core accepts the chosen handle.
pds-corecan still redirect back with?error=handle_takenwhen finalcreateAccount()collides. Deleting the row and cookie here turns that recovery path into an immediate “Session expired”. Let TTL cleanup handle stale flows, or clear the flow only after the callback definitively succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 295 - 297, Currently the code calls ctx.db.deleteAuthFlow(flowId) and res.clearCookie(AUTH_FLOW_COOKIE) immediately after choosing a handle, which prevents pds-core from returning with ?error=handle_taken; remove or comment out those two calls in the choose-handle flow so the auth_flow row and AUTH_FLOW_COOKIE remain valid for the final callback, and instead delete the flow and clear the cookie only after the downstream createAccount() callback definitively succeeds (or let TTL expire stale auth_flow rows).packages/auth-service/src/routes/complete.ts (1)
88-94:⚠️ Potential issue | 🟠 MajorRequire internal PDS config in
/auth/completetoo.This lookup decides whether the user goes down the new-account branch, so public fallback URLs or an empty secret make that decision depend on broken runtime calls instead of valid internal wiring. Fail fast when
PDS_INTERNAL_URLorEPDS_INTERNAL_SECRETis missing.As per coding guidelines
Auth service must use PDS_INTERNAL_URL to reach pds-core over the internal networkandInternal service-to-service calls must use x-internal-secret header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/complete.ts` around lines 88 - 94, The route handler that calls getDidByEmail currently falls back to ctx.config.pdsPublicUrl and empty secret, which can cause wrong branching; update the /auth/complete logic to require and use the internal PDS settings: read PDS_INTERNAL_URL and EPDS_INTERNAL_SECRET and throw a clear error if either is missing, set pdsUrl = PDS_INTERNAL_URL (no public fallback) and internalSecret = EPDS_INTERNAL_SECRET (no empty default), and ensure the call to getDidByEmail (and any other internal calls in this handler) is made with the internal secret sent via the x-internal-secret header.packages/pds-core/src/index.ts (2)
466-470:⚠️ Potential issue | 🟠 MajorFail closed when handle lookup errors.
Returning
{ exists: false }from the catch makes transient account-manager failures look like availability, so the auth service can approve a handle that still cannot be created. Return a 5xx/503 and log the error instead.🛠️ Suggested fix
- } catch { - res.json({ exists: false }) + } catch (err) { + logger.error({ err, handle }, 'Failed to check handle availability') + res.status(503).json({ error: 'handle_check_failed' }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` around lines 466 - 470, The current try/catch around pds.ctx.accountManager.getAccount swallows errors and returns { exists: false }, which masks transient failures; change the catch to capture the error (catch (err)), log it via the service logger (e.g., pds.ctx.logger.error or similar) with context, and respond with a 5xx (preferably 503 Service Unavailable) instead of res.json({ exists: false }) so callers can treat the lookup failure as a service error; update the catch block that surrounds pds.ctx.accountManager.getAccount and replace the silent false-path that uses res.json with a logged error and res.status(503).json(...) response.
198-208:⚠️ Potential issue | 🟠 MajorOnly redirect on an actual handle collision.
This catch still maps invite-code failures, datastore errors, and provider failures to
handle_taken. That hides real 5xxs and sends the user through a recovery path that cannot fix the underlying problem. Redirect back to/auth/choose-handleonly for a verified uniqueness/conflict error; log unexpectedcreateErrand fail the callback instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` around lines 198 - 208, The current catch in createAccount unconditionally treats any createErr as a handle collision and redirects via the logger.warn + res.redirect block; instead only perform that redirect when createErr is a verified uniqueness/conflict error (e.g., error.name/code/message or status === 409, or an instance of the specific Unique/Conflict error your datastore/provider throws). For all other errors, log them as errors (use logger.error with the createErr) and fail the request flow (do not redirect) by passing the error to the error handler or returning a 5xx response so the real failure is surfaced. Ensure you update the code around the logger.warn and res.redirect calls in createAccount to branch on the specific conflict check and avoid swallowing unexpected errors.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/choose-handle.ts (1)
385-388: Use class toggles instead of mutatingstyle.display.The error banner’s visibility is split between inline display state in the template and
errorMsg.style.displayin the script. Moving that to a.hiddenclass keeps the state in CSS and matches the repo’s visibility guideline.As per coding guidelines
Use CSS classes to control visibility (\hidden`, `active`) — avoid inline `display` style except for dynamic values set at render time`.Also applies to: 539-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 385 - 388, The template currently inlines display state for the error banner using errorHtml and later manipulates errorMsg.style.display; instead, render the banner always and use a CSS class (e.g., "hidden") to control visibility: change the errorHtml generation to always include the error div (no inline style) and add or omit the "hidden" class based on error presence, then update the script to toggle errorMsg.classList.add/remove("hidden") (or classList.toggle) instead of setting style.display; apply the same change for the other instance referenced around lines 539-543.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 444-445: The submit button is rendered disabled by default which
prevents form submission if the client-side availability script fails; change
the markup so the <button id="submit-btn"> is enabled by default (remove the
disabled attribute) and update the inline availability script to opt into
disabling or changing the button state once it loads; apply the same change for
the second instance around lines 470-472. Locate the button by id "submit-btn"
and the associated availability script in choose-handle.ts, and keep server-side
POST re-validation as-is.
---
Duplicate comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 73-77: The choose-handle flow never records the initial client
login, so update getFlowAndSession(...) to accept and propagate clientId (add
clientId to its return shape) and in the choose-handle handler (where you
currently call getFlowAndSession and build the redirect) invoke
ctx.db.recordClientLogin(email, clientId) after a successful handle choice and
before redirecting; ensure you pass the same clientId through both the original
caller and the returned flow object so hasClientLogin sees the prior login on
subsequent /auth/complete calls.
- Around line 62-63: Replace the permissive fallbacks for internal PDS access
with strict required configuration: stop using ctx.config.pdsPublicUrl and the
empty-string fallback and instead require process.env.PDS_INTERNAL_URL and
process.env.EPDS_INTERNAL_SECRET to be present (the variables referenced as
pdsUrl and internalSecret in choose-handle.ts). If either is missing, fail fast
by throwing an error or exiting during initialization so the service does not
attempt public-network calls or calls without the x-internal-secret header;
ensure subsequent calls use pdsUrl and include internalSecret in the
x-internal-secret header for internal service-to-service requests.
- Around line 156-163: The fetch call that pings the PDS currently ignores HTTP
errors because fetch only throws on network failures; capture the response
(e.g., const pingRes = await fetch(...)) and check pingRes.ok, and when it's
false treat the flow as expired (same path you use for expired sessions — e.g.,
return/render the session-expired handling used elsewhere or throw a FlowExpired
error) instead of letting the code continue; update the fetch invocation that
uses pdsUrl, result.flow.requestUri, internalSecret and
AbortSignal.timeout(3000) to perform this ok-check and short-circuit.
- Around line 295-297: Currently the code calls ctx.db.deleteAuthFlow(flowId)
and res.clearCookie(AUTH_FLOW_COOKIE) immediately after choosing a handle, which
prevents pds-core from returning with ?error=handle_taken; remove or comment out
those two calls in the choose-handle flow so the auth_flow row and
AUTH_FLOW_COOKIE remain valid for the final callback, and instead delete the
flow and clear the cookie only after the downstream createAccount() callback
definitively succeeds (or let TTL expire stale auth_flow rows).
In `@packages/auth-service/src/routes/complete.ts`:
- Around line 88-94: The route handler that calls getDidByEmail currently falls
back to ctx.config.pdsPublicUrl and empty secret, which can cause wrong
branching; update the /auth/complete logic to require and use the internal PDS
settings: read PDS_INTERNAL_URL and EPDS_INTERNAL_SECRET and throw a clear error
if either is missing, set pdsUrl = PDS_INTERNAL_URL (no public fallback) and
internalSecret = EPDS_INTERNAL_SECRET (no empty default), and ensure the call to
getDidByEmail (and any other internal calls in this handler) is made with the
internal secret sent via the x-internal-secret header.
In `@packages/pds-core/src/index.ts`:
- Around line 466-470: The current try/catch around
pds.ctx.accountManager.getAccount swallows errors and returns { exists: false },
which masks transient failures; change the catch to capture the error (catch
(err)), log it via the service logger (e.g., pds.ctx.logger.error or similar)
with context, and respond with a 5xx (preferably 503 Service Unavailable)
instead of res.json({ exists: false }) so callers can treat the lookup failure
as a service error; update the catch block that surrounds
pds.ctx.accountManager.getAccount and replace the silent false-path that uses
res.json with a logged error and res.status(503).json(...) response.
- Around line 198-208: The current catch in createAccount unconditionally treats
any createErr as a handle collision and redirects via the logger.warn +
res.redirect block; instead only perform that redirect when createErr is a
verified uniqueness/conflict error (e.g., error.name/code/message or status ===
409, or an instance of the specific Unique/Conflict error your
datastore/provider throws). For all other errors, log them as errors (use
logger.error with the createErr) and fail the request flow (do not redirect) by
passing the error to the error handler or returning a 5xx response so the real
failure is surfaced. Ensure you update the code around the logger.warn and
res.redirect calls in createAccount to branch on the specific conflict check and
avoid swallowing unexpected errors.
---
Nitpick comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 385-388: The template currently inlines display state for the
error banner using errorHtml and later manipulates errorMsg.style.display;
instead, render the banner always and use a CSS class (e.g., "hidden") to
control visibility: change the errorHtml generation to always include the error
div (no inline style) and add or omit the "hidden" class based on error
presence, then update the script to toggle
errorMsg.classList.add/remove("hidden") (or classList.toggle) instead of setting
style.display; apply the same change for the other instance referenced around
lines 539-543.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9bc06e-e7f8-4a34-a408-cc9b10180ed6
📒 Files selected for processing (4)
packages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/pds-core/src/index.tspackages/shared/src/__tests__/crypto.test.ts
| <button type="submit" id="submit-btn" class="btn-primary" disabled>Continue</button> | ||
| </form> |
There was a problem hiding this comment.
Don’t make submit depend on client JS to become usable.
The button starts disabled, and only the inline availability script can enable it. If that script is blocked or throws, signup becomes impossible even though the POST handler already re-validates the handle server-side. Render the button enabled by default and let JS opt into disabling once it has loaded.
🛠️ Suggested fix
- <button type="submit" id="submit-btn" class="btn-primary" disabled>Continue</button>
+ <button type="submit" id="submit-btn" class="btn-primary">Continue</button> function updateSubmit() {
submitBtn.disabled = !isAvailable;
}
+
+ updateSubmit();Also applies to: 470-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/routes/choose-handle.ts` around lines 444 - 445,
The submit button is rendered disabled by default which prevents form submission
if the client-side availability script fails; change the markup so the <button
id="submit-btn"> is enabled by default (remove the disabled attribute) and
update the inline availability script to opt into disabling or changing the
button state once it loads; apply the same change for the second instance around
lines 470-472. Locate the button by id "submit-btn" and the associated
availability script in choose-handle.ts, and keep server-side POST re-validation
as-is.
aaff3c4 to
1778d20
Compare
Kzoeps
force-pushed
the
karma/hyper-139-choosing-handle-on-sign-up
branch
from
1778d20 to
3aa74cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/pds-core/src/index.ts (1)
121-132: Consider sharing the handle validation regex.The regex
/^[a-z0-9][a-z0-9-]{1,18}[a-z0-9]$/is likely duplicated between pds-core and auth-service'schoose-handle.ts(which exportsHANDLE_REGEX). If one is updated without the other, validation could become inconsistent.Consider exporting the regex from
@certified-app/sharedalongside the existingRESERVED_HANDLESpattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` around lines 121 - 132, This validation regex is duplicated; instead of hardcoding `/^[a-z0-9][a-z0-9-]{1,18}[a-z0-9]$/` in packages/pds-core/src/index.ts, import and use a shared constant (e.g., HANDLE_REGEX) exported from `@certified-app/shared` (alongside RESERVED_HANDLES) so both pds-core and auth-service use the same source of truth; update the shared package to export HANDLE_REGEX, replace the local regex usage where chosenHandleLocal is validated, and run tests to ensure no behavioral changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/__tests__/crypto.test.ts`:
- Around line 171-181: The tests use full-handle fixtures like
'alice.pds.example.com' but production signs only the local-part (rawHandle);
update the fixtures in the failing tests to use the local-part (e.g., 'alice')
so the payload shape matches signCallback/verifyCallback behavior (look for
tests around the signCallback and verifyCallback calls in this file and replace
handle values in those CallbackParams objects accordingly, including the other
ranges noted).
---
Nitpick comments:
In `@packages/pds-core/src/index.ts`:
- Around line 121-132: This validation regex is duplicated; instead of
hardcoding `/^[a-z0-9][a-z0-9-]{1,18}[a-z0-9]$/` in
packages/pds-core/src/index.ts, import and use a shared constant (e.g.,
HANDLE_REGEX) exported from `@certified-app/shared` (alongside RESERVED_HANDLES)
so both pds-core and auth-service use the same source of truth; update the
shared package to export HANDLE_REGEX, replace the local regex usage where
chosenHandleLocal is validated, and run tests to ensure no behavioral changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf7b0dae-1e20-4490-92a5-5ea307757cf2
📒 Files selected for processing (6)
packages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/pds-core/src/index.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/crypto.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/crypto.ts
- packages/auth-service/src/routes/choose-handle.ts
- packages/auth-service/src/tests/consent.test.ts
1 participant
Summary by CodeRabbit
New Features
Tests