feat(pymthouse): OIDC device flow, billing APIs, and developer-api UI#338
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds PymtHouse across auth/device-flow, billing token/usage, and leaderboard manifest gating. Splits Daydream redirect flow, updates middleware, routes, and UI. Introduces UMD plugin stylesheet management, developer key expiry handling, utility formatters, rate limiting tweaks, docs, seeds, and config/package updates. ChangesAuth and device-flow integration
Billing provider OAuth: redirect vs server flow
PymtHouse billing token and usage APIs
Orchestrator leaderboard: PymtHouse manifest integration
Discovery plans and orchestrator discovery policy
UMD plugin CSS management and mounting
Developer keys expiry, APIs, and UI
Rate limiting and fee formatting utilities
Config, packages, docs, and seeds
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant DeviceAPI as /api/v1/auth/pymthouse-device-approve
participant App as /oidc/device-approved
Middleware->>Client: Redirect with device-approval cookie
Client->>App: GET page
App->>DeviceAPI: GET (fetch userCode)
DeviceAPI-->>App: { userCode }
App->>DeviceAPI: POST approve (CSRF)
DeviceAPI-->>App: { status: authorized } + clear cookie
App-->>Client: Success UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/XL
|
|
|
🗃️ Database Migration Detected This PR includes changes to Prisma schema files. Please ensure:
Requesting review from the core team: @livepeer/core |
github-actions
Bot
added
scope/shell
|
⏳ Review reminder — This PR has been waiting for review for over 24 hours. @team — this PR needs a review. |
seanhanca
requested changes
May 25, 2026
Contributor
seanhanca
left a comment
seanhanca
left a comment
There was a problem hiding this comment.
Review — feat(pymthouse): OIDC device flow, billing APIs, and developer-api UI
Summary
The PR delivers a substantial body of work — OIDC device flow, billing OAuth / proxy routes, expiry handling on developer keys, and a major DeveloperView rewrite — but the device-flow path ships without the user-consent screen that RFC 8628 §3.3 requires, which (combined with non-constant-time HMAC compare and shadowMode CSRF on key mutation routes) creates a real account-takeover surface. The advertised "PKCE hardening" is also not implemented in code (the new column is always written as null).
Blocking issues (must fix)
1. Device flow auto-approves without user consent / user_code display
apps/web-next/src/app/oidc/device-approved/page.tsx:14-51 immediately POSTs to /api/v1/auth/pymthouse-device-approve. The user_code is never shown, there is no Approve/Deny UI, and the device cookie is set by middleware purely from attacker-controllable query params (iss, target_link_uri) at apps/web-next/src/middleware.ts:164-225.
Attack: attacker initiates device flow on PymtHouse, gets user_code, sends the victim a link like https://naap/login?iss=…&target_link_uri=https%3A%2F%2Fpymthouse%2Foidc%2Fdevice%3Fuser_code%3DATTK-XXXX%26client_id%3Dapp_…. Victim opens it (or is already logged in), middleware silently writes the cookie, OAuth/email login → /oidc/device-approved → POST → attacker's CLI receives a victim-scoped signer-session token.
Fix: render an interactive consent screen that displays user_code next to "Approve" and "Deny" buttons; require an explicit user gesture (POST with CSRF token) before invoking approvePymthouseDeviceCode.
2. Non-constant-time HMAC comparison for device-approval cookie
apps/web-next/src/lib/pymthouse-device-initiate.ts:232-236 does providedSignature !== expectedSignature. JS string !== short-circuits on first mismatching byte. Combined with the auto-approval flow above, a timing oracle on signature length gives an attacker a path to forge a device cookie.
Fix: convert both signatures to Buffer and compare with crypto.timingSafeEqual (length-check first, then equal-length compare).
3. CSRF is in shadowMode on developer-key mutation endpoints
apps/web-next/src/app/api/v1/developer/keys/route.ts:136 (POST) and apps/web-next/src/app/api/v1/developer/keys/[id]/route.ts:97 (DELETE) call validateCSRF(request, { shadowMode: true }), which only console.warns violations (see apps/web-next/src/lib/api/csrf.ts:24-78). These routes return rawApiKey and revoke credentials — they must be hard-enforced.
Fix: drop shadowMode: true (the existing daydream/login routes show the pattern).
4. pkceCodeVerifier column added but never written
packages/database/prisma/schema.prisma:200-201 adds the column with a comment touting PKCE; both branches of apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.ts:119,161 explicitly persist pkceCodeVerifier: null, and no S256 challenge ever ships to the provider authorize URL (redirect/route.ts:45-48). The PR description's "Billing OAuth hardening" / "pkceCodeVerifier" claim doesn't match the code.
Fix: either implement PKCE end-to-end (generate verifier in start, send code_challenge + code_challenge_method=S256 in redirect, exchange in callback, then clear the column), or revert the schema/column. Don't ship dead schema.
5. Upstream PymtHouse / Daydream error text leaked to client
apps/web-next/src/app/api/v1/billing/pymthouse/token/route.ts:94returns`PymtHouse signer session failed: ${msg}`wheremsgcomes straight fromtoPmtHouseError.apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/callback/route.ts:48-58,167similarly returns the upstream response body verbatim (escaped, but still disclosing internal details to whoever opens the popup).
Fix: map upstream errors to a fixed set of user-facing strings (PMTHOUSE_TOKEN_FAILED, etc.) and log the raw message server-side only.
6. Billing key prefix leaks 9 chars of secret material
packages/database/src/dev-api/key-utils.ts:36-49 returns t.slice(0, 14) as the "public prefix" for non-NaaP keys (e.g. pmth_AbCdEfGhIj…), which is stored in keyPrefix and returned in every list response. For a pmth_ opaque token that's 9 chars of unmasked secret material.
Fix: for non-NaaP providers return a stable opaque identifier (e.g. ${slug}_${first 4}…) — don't expose secret bytes.
Important (should fix before merge)
1. No rate limit on /api/v1/auth/pymthouse-device-approve
apps/web-next/src/app/api/v1/auth/pymthouse-device-approve/route.ts:26-73 only checks session + cookie. With the CSRF in Blocker #1, a hostile site can iterate. Call enforceRateLimit keyed by session.user.id (e.g. 3/min) and also key on the cookie's userCode.
2. In-process Map rate limiters are largely no-ops on Vercel
apps/web-next/src/app/api/v1/billing/pymthouse/token/route.ts:25-49, start/route.ts:28-46, and apps/web-next/src/lib/api/rate-limit.ts:19 all hold state in a per-instance Map. Vercel Functions scale out, so effective limits multiply by the instance count. Move state to Vercel KV / Upstash, or document as best-effort.
3. OAuth callback redirects to /oidc/device-approved if cookie present
apps/web-next/src/app/api/v1/auth/callback/[provider]/route.ts:50-55 extends the device-flow CSRF to Google/GitHub login. The fix for Blocker #1 (consent screen) closes this; meanwhile do not redirect to /oidc/device-approved from the OAuth callback until the consent gate exists.
4. start route hands back a 90-day signer token without CSRF
apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.ts:69-141 uses the cookie session but never calls validateCSRF. SameSite=Lax mitigates most POST CSRF, but a route that returns a long-lived signer session deserves explicit CSRF. Also gate at the top with if (!authenticatedUser) return errors.unauthorized() instead of the inner throw → 400 mapping that currently surfaces a misleading "User linking failed; please try again".
5. Unbounded fanout in scope=me usage proxy
apps/web-next/src/app/api/v1/billing/pymthouse/usage/route.ts:121-141 runs Promise.all over every endUserId PymtHouse returns. If upstream returns many rows, this becomes N upstream calls per request. Cap at e.g. 25 IDs and aggregate the rest via a single groupBy:'pipeline_model', externalUserId upstream call.
6. naap_device_login_hint writes attacker-controlled value into login form
apps/web-next/src/middleware.ts:208-221 writes whatever login_hint query param the caller passes (≤2048 chars) to a cookie that the client renders into the email input (login-form.tsx:60-81). HttpOnly limits exfiltration but it's still a UX/phishing surface. Validate as z.string().email().max(254) before persisting.
7. DeveloperView at 2120 lines / 78 hooks in a single FC
plugins/developer-api/frontend/src/pages/DeveloperView.tsx:376. Unmaintainable and re-renders the whole tree on any sub-state change. Split into <ApiKeysTab>, <UsageTab>, <ModelsTab>, <DocsTab>, <CreateKeyModal>. The useState(() => resolveTabFromPath(window.location.pathname)) at line 377 also bypasses SSR safety; gate with typeof window === 'undefined'.
8. Duplicated PymtHouse expiry / JWT logic across packages
apps/web-next/src/app/api/v1/developer/keys/route.ts:21-49 and plugins/developer-api/backend/src/server.ts:88-115 both define PYMTHOUSE_SIGNER_SESSION_TTL_MS, isLikelyOidcJwt, decodeJwtExp, computePymthouseExpiry. Extract into @naap/database (or a new @naap/pymthouse-utils).
9. Modal always attaches the keydown listener even when closeOnEscape={false}
packages/ui/src/Modal.tsx:47-56. Effect should early-return when !closeOnEscape. Also no focus-restore on close.
10. pymthouse-device-initiate.ts:104 USER_CODE_RE = /^[A-Z0-9-]{4,16}$/ is permissive enough that ---- validates. Consider requiring at least one non-- char.
Migration & rollout
pkceCodeVerifier String?is additive/nullable and safe forprisma db push, but because it's never written (Blocker #4) prefer to revert it for now and re-add when PKCE is actually implemented.- New cookies (
naap_pmth_device_approval,naap_device_login_hint) default toHttpOnly,Secure(prod),SameSite=Lax,Path=/, 10-min TTL — reasonable. DocumentPYMTHOUSE_DEVICE_COOKIE_SECRET(falls back toNEXTAUTH_SECRET) as required or encode throws. BILLING_PROVIDER_OAUTH_CALLBACK_ORIGINis now required in production (apps/web-next/src/lib/billing-oauth-origin.ts:16-21). Make sure VercelProductionenv has it set before promotion.
Stack / scope
XL diff bundles ~9 independent pieces. The low-risk subset can ship as separate PRs now to give the security-sensitive bits focused attention:
formatFeeWeiString+ test (pure utility).Modalescape-to-close.visibleToUsersplugin discovery field.- UMD plugin stylesheet lifecycle (
apps/web-next/src/lib/plugins/umd-loader.ts+ integration test). @pymthouse/builder-sdk+rimrafdependency adoption.BILLING_PROVIDERSseed forpymthouse.
OIDC device flow, billing token/usage, key expiry, and DeveloperView are tightly coupled to the PymtHouse integration — but the device-flow consent fix should land in this same PR.
Verdict
REQUEST_CHANGES — Blockers: missing device-flow consent UI, timing-unsafe HMAC compare, CSRF shadowMode on key create/revoke, advertised PKCE not actually implemented, upstream error leakage, and the billing-key prefix exposing 9 chars of secret material. The rest of the PR is solid and can land after these are addressed.
4 tasks
|
This PR has conflicts with the base branch. Please rebase to resolve them: git fetch origin
git rebase origin/feat/discovery-plan-billing-provider
# resolve conflicts, then:
git push --force-with-leaseThe |
eliteprox
force-pushed
the
feat/pymthouse-integration-followups
branch
from
db1989c to
899d1c6
Compare
eliteprox
added a commit
that referenced
this pull request
May 26, 2026
… guard - Add a block comment to provider-restrictions.ts explaining why SUPPORTED_PROVIDER_SLUGS only contains 'daydream' while BILLING_PROVIDER_SLUGS in types.ts includes 'pymthouse'. The split is intentional: the Zod schema accepts both so the plan CRUD layer can surface legacy null-slug rows via the pymthouse OR-null WHERE clause, while normalizeBillingProviderSlug rejects 'pymthouse' with a 400 on routes like /capability-catalog until manifest-gating lands in PR #338. - Add an inline comment to the capability-catalog route's validation block to surface the same reasoning at the call site. - Remove the unreachable blank-string guard in billingProviderWhere. The function signature accepts BillingProviderSlug | null ('daydream' | 'pymthouse' | null), so an empty string can never arrive here; the check was dead code. Co-authored-by: Cursor <cursoragent@cursor.com>
eliteprox
added a commit
that referenced
this pull request
May 26, 2026
BILLING_PROVIDER_SLUGS previously included 'pymthouse', which meant the Zod schema accepted it as a valid input even though this PR is Daydream-only. This exposed an unsupported billing provider through the public API. - types.ts: BILLING_PROVIDER_SLUGS now ['daydream'] only; BillingProviderSlug and BillingProviderSlugSchema derive from the narrowed constant. - plans.ts: remove the pymthouse OR-null billingProviderWhere branch — it is now unreachable since BillingProviderSlug = 'daydream'. - provider-restrictions.ts: simplify the block comment; the BILLING_PROVIDER_SLUGS / SUPPORTED_PROVIDER_SLUGS asymmetry is gone. - capability-catalog/route.ts: remove the now-stale inline comment about pymthouse. PymtHouse support (OR-null backfill query, manifest gating) is re-introduced in PR #338 (feat/pymthouse-integration-followups) on top of this PR. Co-authored-by: Cursor <cursoragent@cursor.com>
eliteprox
force-pushed
the
feat/pymthouse-integration-followups
branch
from
899d1c6 to
9713edd
Compare
eliteprox
force-pushed
the
feat/pymthouse-integration-followups
branch
from
151830a to
da234b1
Compare
Contributor
Author
Review resolution notes (initial + re-review)Replying inline on each re-review thread. Summary of initial review blockers/important items that did not get separate inline comments: Initial blockers — all addressed
Initial important — addressed or acknowledged
Latest follow-up commits (post re-review)
Still open (non-blocking follow-ups)
Ready for re-review @seanhanca |
seanhanca
previously approved these changes
May 29, 2026
Rebased onto main (post #337 "Daydream discovery plans and cache refresh"). Re-applies PymtHouse integration on top of the canonical Daydream discovery work that landed in #337: - PymtHouse OIDC device-flow login + device approval routes (consent screen, CSRF, constant-time cookie verify, per-instance rate limiting) - Billing token/usage APIs and usage helpers via @pymthouse/builder-sdk - Developer API key expiry, billing key prefix masking, safe key serialization - developer-api plugin UI (usage, keys, discovery plan tooltip) + backend proxy - PymtHouse manifest gating restored across discovery: provider-restrictions, capability-catalog, python-gateway, plan refresh, global refresh - billingProviderSlug supports 'pymthouse' alongside 'daydream' (OR-null backfill for legacy rows); defaults to pymthouse - UMD plugin stylesheet lifecycle, Modal escape-to-close, plugin visibleToUsers Conflict resolution preserved #337's Daydream discovery baseline and layered the branch's PymtHouse functionality on top. The plugin frontend (identical to main except 5 files) takes the branch versions of the PymtHouse-aware pages. Co-authored-by: Cursor <cursoragent@cursor.com>
eliteprox
force-pushed
the
feat/pymthouse-integration-followups
branch
from
b454bd1 to
f242c0d
Compare
Regenerate the lockfile with the repo-pinned npm@10.9.4 so it matches the merged package.json (adds @pymthouse/builder-sdk and rimraf for web-next). Fixes the failing lockfile-sync / quality-gates CI checks. Verified with `npm ci`. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.ts (2)
79-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
gateway_nonceandgateway_instance_idat runtime.
as stringonly changes TypeScript's view of the value. A JSON object/array here is still truthy, skips the fallback, and then reaches Prisma as a non-string, turning malformed client input into a 500 instead of a 4xx.Suggested fix
- const body = await request.json().catch(() => ({})); - const gatewayNonce = (body.gateway_nonce as string) || crypto.randomBytes(32).toString('hex'); - const gatewayInstanceId = (body.gateway_instance_id as string) || null; + const body = await request.json().catch(() => ({})); + const gatewayNonceRaw = body.gateway_nonce; + const gatewayInstanceIdRaw = body.gateway_instance_id; + + if ( + gatewayNonceRaw != null && + (typeof gatewayNonceRaw !== 'string' || gatewayNonceRaw.length === 0) + ) { + return errors.badRequest('gateway_nonce must be a non-empty string'); + } + + if ( + gatewayInstanceIdRaw != null && + typeof gatewayInstanceIdRaw !== 'string' + ) { + return errors.badRequest('gateway_instance_id must be a string'); + } + + const gatewayNonce = + gatewayNonceRaw ?? crypto.randomBytes(32).toString('hex'); + const gatewayInstanceId = gatewayInstanceIdRaw ?? null;🤖 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 `@apps/web-next/src/app/api/v1/auth/providers/`[providerSlug]/start/route.ts around lines 79 - 81, The request.json() result is being coerced with "as string" but not validated, so non-string JSON values slip through into Prisma; update the start route to perform runtime type checks after parsing the body: for gateway_nonce (variable gatewayNonce) check typeof body.gateway_nonce === 'string' and use that value only if true, otherwise fall back to crypto.randomBytes(...).toString('hex'); for gateway_instance_id (variable gatewayInstanceId) accept body.gateway_instance_id only if typeof === 'string' (or explicitly allow null) and otherwise set to null; apply these checks where body is assigned in the handler so malformed client input yields a 4xx validation path rather than reaching Prisma with wrong types.
173-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark the redirect-flow start response as
no-store.This payload includes
login_session_id, which is the continuation handle for/redirect. The PymtHouse token branch already disables caching, but the Daydream branch leaves this response cacheable.Suggested fix
- return success({ + const response = success({ login_session_id: loginSessionId, expires_in: Math.floor(LOGIN_SESSION_TTL_MS / 1000), poll_after_ms: 1500, }); + response.headers.set('Cache-Control', 'no-store'); + return response;🤖 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 `@apps/web-next/src/app/api/v1/auth/providers/`[providerSlug]/start/route.ts around lines 173 - 177, The response that returns the login_session_id (the success({...}) call) must be marked with Cache-Control: no-store to prevent caching of the redirect-flow continuation handle; update the Daydream branch of the start route so the response produced by success(...) (the object containing login_session_id, expires_in, poll_after_ms and using LOGIN_SESSION_TTL_MS/loginSessionId) sets headers to "Cache-Control: no-store" (or otherwise returns a no-store response variant) matching the PymtHouse token branch behavior.bin/seed-discovery-plans.ts (1)
81-89:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffClarify/Scope
pymthouse→daydreambackfill in seed-discovery-plans
backfillBillingProviderSlugscurrently:
- Converts all
discoveryPlanrows withbillingProviderSlug: 'pymthouse'todaydreamviaupdateMany.- Then backfills
billingProviderSlug: null→pymthouse(this aligns with the app treatingnullaspymthouseand docs defaulting topymthouse).Later, new default plans created by this script use
billingProviderSlug: 'pymthouse', but only for missingbillingPlanIds—so already-existing seeded defaults would end updaydream, while newly created ones would bepymthouse.Confirm whether the
pymthouse→daydreamstep is an intentional one-time correction for legacy rows. If so, scope it to only the plans seeded by this script (e.g.,naap-default-*) and add a “run once / remove later” comment or gating; if not, make the backfill and creation provider slugs consistent.🤖 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 `@bin/seed-discovery-plans.ts` around lines 81 - 89, The unconditional updateMany that migrates billingProviderSlug 'pymthouse' → 'daydream' should be clarified or scoped: either (A) treat it as a one‑time legacy fix and restrict it to only plans created by this seed (e.g., WHERE name startsWith 'naap-default-' or other unique seed identifiers) and add a clear “run once / remove later” comment next to the update (reference: the updateMany block in seed-discovery-plans.ts), or (B) make the backfill logic consistent by not converting existing 'pymthouse' to 'daydream' so that the later null→'pymthouse' backfill and the newly created defaults use the same provider; pick one approach and implement it so updateMany and the subsequent default creation/backfill behavior are aligned.plugins/orchestrator-leaderboard/docs/for-ai.md (1)
121-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the capability regex in this schema block in sync with the server schema.
This section now updates the provider default, but the adjacent capability comment still says
^[a-zA-Z0-9_-]+$. The actual validator accepts.,:, and/, so agents following this doc will incorrectly reject valid capabilities likevideo/model-aorgemma3:4b.🤖 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 `@plugins/orchestrator-leaderboard/docs/for-ai.md` around lines 121 - 126, Update the inline comment for the capabilities field in the schema (the block containing billingProviderSlug and capabilities) so it matches the server validator: allow dot, colon and slash characters in addition to alphanumerics, underscore and hyphen and keep the existing constraints (1..50 items, ≤128 chars per item); modify the comment adjacent to capabilities to explicitly list the allowed characters (so examples like "video/model-a" and "gemma3:4b" are clearly permitted) and ensure it stays in sync with the server-side validator.plugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx (1)
122-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep bulk toggle alias-aware.
isCapabilitySelected()andtoggleCapability()still recognize legacy short model IDs, but bulk deselect now removes only exact full capability strings. A plan containing legacy IDs can stay selected after “remove all” and re-add duplicates on bulk select.Suggested fix
const bulkToggleCapabilities = (capabilities: string[], select: boolean) => { const current = draft.capabilities ?? plan?.capabilities ?? []; + const modelIds = new Set( + capabilities.map((cap) => cap.slice(cap.lastIndexOf('/') + 1)), + ); let next: string[]; if (select) { - const toAdd = capabilities.filter((cap) => !current.includes(cap)); - next = [...current, ...toAdd]; + const normalizedCurrent = current.filter((cap) => !modelIds.has(cap)); + const toAdd = capabilities.filter((cap) => !normalizedCurrent.includes(cap)); + next = [...normalizedCurrent, ...toAdd]; } else { const removeSet = new Set(capabilities); - next = current.filter((cap) => !removeSet.has(cap)); + next = current.filter( + (cap) => !removeSet.has(cap) && !modelIds.has(cap), + ); } setDraft({ capabilities: next }); };🤖 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 `@plugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx` around lines 122 - 132, bulkToggleCapabilities currently removes only exact strings so legacy/alias IDs remain; update it to be alias-aware by using the same matching logic as isCapabilitySelected/toggleCapability: when deselecting, build removal by testing each existing capability with the same alias-equality check (i.e., treat a current capability as removed if any of the requested capabilities matches it via your alias logic), and when selecting, avoid adding duplicates by checking alias-equality against current entries (use the same helper used by isCapabilitySelected/toggleCapability); operate on draft.capabilities/plan?.capabilities and produce next accordingly.plugins/developer-api/backend/src/server.ts (2)
676-705:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter expired PymtHouse keys out of the current list response too.
maybeCleanupExpiredPymthouseKeys(userId)is fire-and-forget here, but Lines 690-704 still serialize every row that was just read. A just-expired key will remain visible until the cleanup finishes or the next throttle window passes, which diverges from the web-next route and surfaces unusable keys.🤖 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 `@plugins/developer-api/backend/src/server.ts` around lines 676 - 705, The current handler fires maybeCleanupExpiredPymthouseKeys(userId) but still includes just-expired PymtHouse keys in the returned list; after fetching keys (the keys variable) filter them to exclude entries where billingProvider?.slug === PYMTHOUSE_PROVIDER_SLUG and computeSignerSessionExpiry(k.createdAt) is <= new Date(), then map the filtered list into formatted and return that; keep the maybeCleanupExpiredPymthouseKeys call as-is (fire-and-forget) but ensure computeSignerSessionExpiry and PYMTHOUSE_PROVIDER_SLUG are referenced when applying the filter so expired PymtHouse keys are not serialized.
928-968:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject expired PymtHouse JWTs in the fallback create path as well.
The Prisma branch returns 400 when
pymthouseTokenExpiry <= now, but the in-memory branch only computesfallbackPymthouseTokenExpiryand still stores the key asACTIVE. That makes no-DB mode accept credentials that are already unusable.Suggested fix
const fallbackPymthouseTokenExpiry = fallbackProvider?.slug === PYMTHOUSE_PROVIDER_SLUG && isLikelyOidcJwt(rawApiKey) ? decodeJwtExp(rawApiKey) : null; + if ( + fallbackProvider?.slug === PYMTHOUSE_PROVIDER_SLUG && + fallbackPymthouseTokenExpiry && + fallbackPymthouseTokenExpiry.getTime() <= Date.now() + ) { + return res.status(400).json({ error: 'PymtHouse token is already expired. Please create a new key.' }); + } const newKey = {🤖 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 `@plugins/developer-api/backend/src/server.ts` around lines 928 - 968, The in-memory create path computes fallbackPymthouseTokenExpiry but does not reject expired PymtHouse JWTs and still pushes an ACTIVE key into inMemoryApiKeys; update the logic in the fallback/create branch to mirror the Prisma behavior: if fallbackPymthouseTokenExpiry is non-null and <= now, respond with a 400 error instead of creating newKey (refer to fallbackPymthouseTokenExpiry, PYMTHOUSE_PROVIDER_SLUG, newKey, inMemoryApiKeys, computeSignerSessionExpiry and rawApiKey), otherwise proceed to create the key and compute expiresAt as currently implemented.packages/types/src/index.ts (1)
237-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
DeveloperApiKeywith the response shape returned by these routes.The API handlers in this PR intentionally omit
keyHash, and they can returnACTIVE,REVOKED, andEXPIRED. This shared type still requireskeyHashand only allows lowercaseactive | revoked, so consumers cannot model the new contract without casts and will miss the expiry state.🤖 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/types/src/index.ts` around lines 237 - 247, Update the DeveloperApiKey type to match the API response: change ApiKeyStatus from the lowercase union ('active' | 'revoked') to the API values ('ACTIVE' | 'REVOKED' | 'EXPIRED'), and make the keyHash field optional or remove it (i.e., change keyHash: string to keyHash?: string) since handlers omit it; keep other fields (createdAt, expiresAt, lastUsedAt) as-is so consumers can model the new contract without casts.apps/web-next/src/app/api/v1/developer/keys/route.ts (1)
120-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the expiry predicate before paginating and counting.
findMany(...skip/take...)andcount()both include expired PymtHouse rows, then Lines 142-144 drop them in memory. That makestotal/totalPagestoo high and can also create short or empty pages becauseskipwas computed against a different result set.🤖 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 `@apps/web-next/src/app/api/v1/developer/keys/route.ts` around lines 120 - 153, The current logic filters out expired PymtHouse keys in-memory after fetching with prisma.devApiKey.findMany and prisma.devApiKey.count, causing incorrect total/totalPages and misaligned pagination; update both prisma.devApiKey.findMany(...) and prisma.devApiKey.count(...) to include the same "not expired" predicate used by isExpiredPymthouseKey (or its equivalent DB-safe conditions) in their where clauses so the DB only returns and counts non-expired keys (keep mapping toSafeDevApiKey for shaping results but remove the post-query .filter that alters pagination).apps/workflows/developer-web/src/pages/DeveloperView.tsx (1)
17-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault tab inconsistent with tab order.
The
tabsarray now placesapi-keysfirst, butactiveTabstill defaults to'models'(line 25). The plugin version atplugins/developer-api/frontend/src/pages/DeveloperView.tsxdefaults to'api-keys'. Consider aligning for consistency.Suggested fix
export const DeveloperView: React.FC = () => { - const [activeTab, setActiveTab] = useState<TabId>('models'); + const [activeTab, setActiveTab] = useState<TabId>('api-keys');🤖 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 `@apps/workflows/developer-web/src/pages/DeveloperView.tsx` around lines 17 - 25, The default active tab in DeveloperView is inconsistent with the tabs order: the tabs array lists 'api-keys' first but the component's state initializes activeTab to 'models'. Update the DeveloperView component to initialize useState<TabId> (activeTab) to 'api-keys' (or derive the default from tabs[0].id) so the default selection matches the tabs array and aligns with the plugin's behavior; adjust the initialization in the DeveloperView component where activeTab is defined.apps/web-next/src/lib/orchestrator-leaderboard/refresh.ts (1)
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the plan’s provider into the cache-key revision.
providerRestrictionRevision()is now provider-scoped, but this key still calls it withoutplan.billingProviderSlug. That leaves PymtHouse plans with the default'na'revision segment, so manifest changes do not naturally roll the cache key.Suggested fix
export function buildPlanEvaluationCacheKey(plan: DiscoveryPlan): string { const slug = plan.billingProviderSlug ?? 'null'; - const rev = providerRestrictionRevision(); + const rev = providerRestrictionRevision(plan.billingProviderSlug); const capFp = fingerprintCapabilityList(plan.capabilities); return `${plan.id}${PLAN_CACHE_KEY_SEP}${slug}${PLAN_CACHE_KEY_SEP}${rev}${PLAN_CACHE_KEY_SEP}${capFp}`; }🤖 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 `@apps/web-next/src/lib/orchestrator-leaderboard/refresh.ts` around lines 35 - 39, The cache key omits provider scope because providerRestrictionRevision() is called without the plan's provider; update buildPlanEvaluationCacheKey to pass the plan's billing provider slug into providerRestrictionRevision (e.g., const rev = providerRestrictionRevision(plan.billingProviderSlug) or using the existing slug variable) so the revision segment reflects the plan's provider and cache keys roll when a provider-specific manifest changes.apps/web-next/src/app/api/v1/auth/callback/[provider]/route.ts (1)
78-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop forwarding raw OAuth exception text to clients.
Both catch blocks still serialize
err.messageinto the redirect query / JSON body. That re-opens the upstream error leak this PR is supposed to close and can expose provider or internal details to end users.Suggested fix
} catch (err) { console.error('OAuth callback error:', err); const typedErr = err as Error & { code?: string }; if (typedErr.code === 'ACCOUNT_SUSPENDED') { return NextResponse.redirect(new URL('/login?error=account_suspended', request.url)); } - const message = err instanceof Error ? encodeURIComponent(err.message) : 'oauth_failed'; - return NextResponse.redirect(new URL(`/login?error=${message}`, request.url)); + return NextResponse.redirect(new URL('/login?error=oauth_failed', request.url)); } } @@ } catch (err) { console.error('OAuth callback error:', err); const typedErr = err as Error & { code?: string; reason?: string | null }; if (typedErr.code === 'ACCOUNT_SUSPENDED') { return NextResponse.json( @@ { status: 403 } ); } - const message = err instanceof Error ? err.message : 'OAuth authentication failed'; return NextResponse.json( - { success: false, error: { code: 'OAUTH_FAILED', message } }, + { + success: false, + error: { + code: 'OAUTH_FAILED', + message: 'Authentication failed. Please try again.', + }, + }, { status: 400 } ); } }Also applies to: 138-157
🤖 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 `@apps/web-next/src/app/api/v1/auth/callback/`[provider]/route.ts around lines 78 - 85, The catch block currently forwards raw error text to clients (it uses err.message via the message variable and returns NextResponse.redirect), so change the error responses to never include err.message: keep server-side logging of the full error (console.error(err) or process logger) but map client-facing redirects to fixed, safe error codes (e.g., 'account_suspended' already used, otherwise use 'oauth_failed' or provider-specific stable keys) and remove encodeURIComponent(err.message) usage; update the code paths that reference typedErr and NextResponse.redirect (and the second catch at the other block) to return only those fixed query values and not the raw err string, ensuring request is still used for URL construction while logging full details on the server only.
🧹 Nitpick comments (7)
apps/web-next/src/lib/plugins/umd-loader.ts (1)
354-356: 💤 Low valueSimplify redundant assignment.
didCreateRecordis set tofalsethen immediately totrue.♻️ Suggested simplification
- let didCreateRecord = false; managedStylesheets.set(url, record); - didCreateRecord = true;And update the catch block:
- if (didCreateRecord && record.refCount <= 0) { + if (record.refCount <= 0) { managedStylesheets.delete(url); }🤖 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 `@apps/web-next/src/lib/plugins/umd-loader.ts` around lines 354 - 356, Remove the redundant initial assignment of didCreateRecord: instead of setting let didCreateRecord = false; then immediately didCreateRecord = true after managedStylesheets.set(url, record), initialize didCreateRecord to true where appropriate or set it only once after successful creation in the function containing managedStylesheets.set; also update the catch block in the same function to rely on this single flag (didCreateRecord) for cleanup logic so it accurately reflects whether record was created or not—search for didCreateRecord and managedStylesheets.set(url, record) to locate the exact spot.apps/web-next/package.json (1)
7-7: ⚖️ Poor tradeoffClean .next deletion may slow builds unnecessarily.
Deleting
.nextbefore every build prevents Next.js from reusing cached build artifacts, which can significantly increase build times, especially in CI/CD. Next.js 15 has improved incremental builds that rely on this cache.If this was added to work around a specific caching bug, consider:
- Using
next build --no-cacheonly when needed- Cleaning
.nextonly in CI (via a separatebuild:ciscript)- Filing an issue with Next.js if there's a reproducible cache corruption problem
⚡ Alternative approach for selective cache clearing
- "build": "rimraf .next && next build", + "build": "next build", + "build:clean": "rimraf .next && next build",Use
build:cleanonly when cache issues are suspected.🤖 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 `@apps/web-next/package.json` at line 7, The package.json "build" script currently force-deletes .next ("build": "rimraf .next && next build"), which prevents Next.js incremental cache reuse; update scripts so the default "build" runs just "next build" and add explicit alternatives such as "build:ci" that runs rimraf .next && next build or "build:no-cache" that runs next build --no-cache (or a "build:clean" script) so cache-clearing is performed only when intended; modify the script entries named "build" and add the new script keys to package.json accordingly.docs/pymthouse-integration.md (2)
90-90: ⚡ Quick winSimplify complex sentence for clarity.
This 200+ character sentence packs many concepts (manifest sync, excludedCapabilities, fail-open, cache busting) into a single statement. Consider breaking it into multiple sentences or a bulleted list for better readability.
📝 Suggested restructure
### Network Price discovery allowlist (PymtHouse → NaaP) For billing provider **`pymthouse`**, NaaP periodically syncs the manifest from **`GET {PYMTHOUSE_ISSUER_URL without /oidc}/apps/{publicClientId}/manifest`** using M2M Basic credentials. **Allowlist behavior:** - The manifest's **`excludedCapabilities`** array is authoritative for denials - NaaP allows every catalog capability not explicitly excluded - The **`capabilities`** array is informational only (PymtHouse may know fewer capabilities than NaaP) - **`manifestVersion`** enables cache busting when present **Default behavior:** A missing manifest **denies discovery by default**. Set **`PYMTHOUSE_ALLOW_MISSING_MANIFEST_FAIL_OPEN=1`** only in controlled environments to restore legacy fail-open behavior (generates high-severity audit log).🤖 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 `@docs/pymthouse-integration.md` at line 90, The sentence describing PymtHouse manifest behavior is overly long and hard to read; split it into multiple sentences or a short bulleted list and explicitly call out the elements referenced (GET {PYMTHOUSE_ISSUER_URL without /oidc}/apps/{publicClientId}/manifest, excludedCapabilities, capabilities, manifestVersion, and the PYMTHOUSE_ALLOW_MISSING_MANIFEST_FAIL_OPEN flag) so each rule is clear: one sentence for how NaaP fetches the manifest, one for allow/deny rules (excludedCapabilities authoritative; everything else allowed), one noting capabilities is informational only, one that manifestVersion is for cache busting, and a final sentence about default deny and the controlled-use fail-open flag (PYMTHOUSE_ALLOW_MISSING_MANIFEST_FAIL_OPEN=1) and its audit implication.
1-158: ⚡ Quick winConsider adding troubleshooting section.
The documentation is comprehensive but lacks a troubleshooting section for common integration issues. Consider adding common error scenarios and their resolutions (e.g., "M2M authentication fails," "Device approval returns 400," "Usage API returns not configured message").
🤖 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 `@docs/pymthouse-integration.md` around lines 1 - 158, Add a concise "Troubleshooting" section to docs/pymthouse-integration.md that lists common failure scenarios and fixes: include entries for "M2M authentication fails" (check PYMTHOUSE_M2M_CLIENT_ID / PYMTHOUSE_M2M_CLIENT_SECRET, PYMTHOUSE_ISSUER_URL and reference createPmtHouseClientFromEnv), "Device approval returns 400" (verify device/initiate settings, issuer origin and PMTHOUSE_BASE_URL, and RFC 8628 flow), and "Usage API returns not configured message" (ensure env vars present and reference GET /api/v1/billing/pymthouse/usage and PYMTHOUSE_NOT_CONFIGURED_MESSAGE), plus quick remediation steps and links to syncPymthouseManifestSnapshot and relevant routes (/api/v1/apps/{publicClientId}/manifest) for manifest/allowlist issues.plugins/developer-api/frontend/src/pages/DeveloperView.tsx (2)
143-151: 💤 Low valueGuard against invalid
createdAtwhen computing expiry.If
key.createdAtis malformed,computeSignerSessionExpirymay produce an invalidDate, andtoISOString()will throw. Consider wrapping in a try-catch or validating before calling.Proposed defensive guard
function resolveApiKeyExpiresAt(key: ApiKey): string | null { if (key.expiresAt != null && String(key.expiresAt).trim() !== '') { return key.expiresAt; } if (key.billingProvider?.slug === 'pymthouse') { + try { + const exp = computeSignerSessionExpiry(key.createdAt); + if (!Number.isFinite(exp.getTime())) return null; + return exp.toISOString(); + } catch { + return null; + } - return computeSignerSessionExpiry(key.createdAt).toISOString(); } return null; }🤖 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 `@plugins/developer-api/frontend/src/pages/DeveloperView.tsx` around lines 143 - 151, In resolveApiKeyExpiresAt, guard the computeSignerSessionExpiry call against an invalid key.createdAt by validating or try-catching the conversion: verify key.createdAt can produce a valid Date (or call computeSignerSessionExpiry inside a try/catch) and only call toISOString() when the resulting Date is valid; if validation fails or an exception is thrown, return null instead of letting toISOString() propagate. Ensure you reference resolveApiKeyExpiresAt, computeSignerSessionExpiry and key.createdAt when making the change so the fix is applied in the correct function.
1671-1684: 💤 Low valueNon-standard Tailwind opacity values.
border-white/8(8%) andhover:bg-white/3(3%) work with Tailwind JIT but are non-standard. Consider using/10and/5for consistency with the rest of the file.🤖 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 `@plugins/developer-api/frontend/src/pages/DeveloperView.tsx` around lines 1671 - 1684, Replace the non-standard Tailwind opacity utilities used in the table markup: change the header row className that includes "border-white/8" to use a standard value like "border-white/10", and change the mapped row className that includes "hover:bg-white/3" to use a standard value like "hover:bg-white/5" (these are the className strings on the header <tr className="border-b border-white/8"> and the mapped <tr ... className="border-b border-white/5 hover:bg-white/3 ..."> respectively).apps/web-next/src/lib/api/rate-limit.ts (1)
57-69: ⚡ Quick winDrop the query string from the anonymous bucket key.
When
ipis missing, hashing the fullrequest.urlmakes the fallback throttle much weaker on query-driven routes because each query variant gets a fresh bucket. Hashpathname(or justkeyPrefix) instead of the full URL.♻️ Suggested change
const key = ip ? `${options.keyPrefix}:${ip}` : `${options.keyPrefix}:anon:${createHash('sha256') .update( [ request.headers.get('user-agent') ?? '', request.headers.get('accept') ?? '', request.method, - request.url, + new URL(request.url).pathname, ].join('|'), ) .digest('hex') .slice(0, 24)}`;🤖 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 `@apps/web-next/src/lib/api/rate-limit.ts` around lines 57 - 69, The anonymous rate-limit bucket key currently hashes the full request.url which allows query-string variants to create separate buckets; update the key construction (the const key logic that uses createHash and options.keyPrefix) to use only the request pathname (or options.keyPrefix) instead of request.url when ip is missing—extract the pathname from the request (e.g., via new URL(request.url).pathname or request.nextUrl.pathname) and include that in the hashed payload along with the user-agent, accept, and method so query parameters are dropped from the anonymous bucket key.
🤖 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 `@apps/web-next/next.config.js`:
- Line 31: Remove '`@pymthouse/builder-sdk`' from the transpilePackages array in
next.config.js: locate the transpilePackages config (the array that currently
includes '`@pymthouse/builder-sdk`') and delete that entry so Next.js no longer
forces transpilation for this package; if necessary, run a local build to
confirm nothing breaks and leave a brief inline comment noting removal because
the package ships precompiled dist/*.js files and exports point to dist.
In
`@apps/web-next/src/app/api/v1/auth/providers/`[providerSlug]/callback/route.ts:
- Around line 91-105: The HTML callback branches for providerSlug 'pymthouse'
and non-'daydream' (and the missing state/token branch) return sensitive
callback pages without preventing caching; update the responses in route.ts to
include Cache-Control: no-store (or use/extend htmlResponse to accept and set a
no-store header) for every branch that returns HTML in the callback (the calls
to htmlResponse in the pymthouse branch, the unsupported-billing-provider
branch, and the "Missing token or state" branch), ensuring the response headers
include "Cache-Control: no-store" (and optionally "Pragma: no-cache" and
"Expires: 0") so bearer tokens in query strings are never cached.
In `@apps/web-next/src/app/api/v1/auth/pymthouse-device-approve/route.ts`:
- Around line 34-52: The GET handler returns per-session data and must disable
browser caching; update the GET function to include Cache-Control: no-store on
its responses (wrap or augment the NextResponse produced by errors.unauthorized,
errors.badRequest, and success so they set the header) so
tryParseDeviceApprovalCookie / NAAP_PMTH_DEVICE_APPROVAL_COOKIE-backed responses
cannot be cached and replayed.
In `@apps/web-next/src/app/api/v1/billing/pymthouse/usage/route.ts`:
- Around line 46-68: The fallback path in mapUpstreamUsageFailure currently
returns raw upstream text via err.message; update the final return to avoid
leaking upstream error messages by replacing err.message with a generic
client-facing message (e.g., "Upstream service error") and ensure a sensible
default status (e.g., 502) if err.status is missing; modify the final return
expression in mapUpstreamUsageFailure (which uses PmtHouseError/toPmtHouseError
and the error(...) helper) to use error(err.code || 'UPSTREAM_ERROR', 'Upstream
service error', err.status ?? 502) instead of forwarding err.message.
- Around line 24-38: The regex in stripCryptoUnitFields is too permissive and
removes any key containing the substring (e.g., "method" matches "eth"); update
the filter to only skip keys that are exactly a crypto unit or end with one.
Replace the /(wei|eth|gwei)/i check on the variable key with a precise test such
as: lower-case the key and continue only if one of ['wei','eth','gwei'] matches
exactly or is a suffix (e.g., key.toLowerCase() === unit ||
key.toLowerCase().endsWith(unit)); keep using the same function name
stripCryptoUnitFields and the loop over Object.entries to locate where to
change.
In `@apps/web-next/src/app/oidc/device-approved/page.tsx`:
- Around line 17-49: The useEffect's started.current guard causes a deadlock
under React Strict Mode because cleanup sets cancelled = true then remount
returns early; remove the started ref check and reliance on started.current in
the effect (i.e., delete the "if (started.current) return;" and the line that
sets started.current = true) and instead rely on the local cancelled flag or
replace it with an AbortController to cancel the in-flight fetch inside the
async IIFE; keep the existing cancelled handling (or wire the fetch to the
AbortController signal and abort in the cleanup) and ensure setUserCode,
setPhase and setMessage only run when not cancelled/aborted.
In `@apps/web-next/src/lib/billing-oauth-origin.ts`:
- Around line 37-53: The code currently ignores a non-local x-forwarded-host and
falls back to http://localhost:3000; update the fallback logic to honor
forwardedHost even when it's not local: in the block after the host check,
handle forwardedHost regardless of isLocalHost by computing protocol the same
way as for resolvedHost (use forwardedProto || 'http' if
isLocalHost(forwardedHost), otherwise 'https') and return
`${protocol}://${forwardedHost}` so public proxy-hosted origins are used instead
of defaulting to localhost; reference variables: host, forwardedHost,
forwardedProto and helper isLocalHost.
In `@apps/web-next/src/lib/orchestrators-discovery-policy.ts`:
- Around line 19-35: passesFilters currently only enforces maxSwapRatio and
ignores gpuRamGbMin/gpuRamGbMax, priceMax, and maxAvgLatencyMs from
DiscoveryPolicy.filters; update passesFilters (or a pre-validation step) to
evaluate these fields against the DashboardOrchestrator properties (e.g., check
DashboardOrchestrator.gpuRamGb between gpuRamGbMin/gpuRamGbMax, ensure
DashboardOrchestrator.pricePerHour <= priceMax, and ensure
DashboardOrchestrator.avgLatencyMs <= maxAvgLatencyMs) and return false when any
constraint is violated, or alternatively strip/validate unsupported keys on the
DiscoveryPolicy before calling passesFilters so advertised filters are enforced
consistently.
- Around line 45-48: The current switch for sort modes 'latency' and 'price'
wrongly falls back to row.slaScore, silently changing semantics; change the
'latency' and 'price' branches to NOT return slaScore — instead return undefined
(or a sentinel like NaN) so the caller can detect "metric unavailable" and
preserve input order or reject the sort mode upstream; update the consumer or
add an explicit validation of sortBy ('latency'|'price') to throw or no-op when
DashboardOrchestrator rows don't expose these metrics. Ensure you edit the
switch handling those cases (the 'latency'/'price' case and any caller code that
interprets the row value) rather than returning row.slaScore.
In `@apps/web-next/src/lib/plugins/umd-loader.ts`:
- Around line 340-378: When awaiting an existing.shared loadingPromise you must
decrement the incremented refCount if that promise rejects; update the logic
around managedStylesheets.get(url) where existing.refCount is incremented and
existing.loadingPromise is awaited so that you catch rejections, decrement
existing.refCount, and if existing.refCount <= 0 delete the entry from
managedStylesheets. In practice, wrap the await of existing.loadingPromise in a
try/catch (referencing existing.loadingPromise, existing.refCount, and
managedStylesheets) and perform the same cleanup done in the record-creator path
(decrement refCount and remove the map entry when it reaches 0) before
rethrowing the error.
In `@apps/web-next/src/lib/pymthouse-device-initiate.test.ts`:
- Around line 80-87: The test "tryParseDeviceApprovalCookie rejects expired
payload" currently passes plain JSON so tryParseDeviceApprovalCookie exits at
the malformed-cookie guard; instead construct a legitimately encoded/signed
cookie (i.e., include the '.' separator and use the same signing/encoding
routine your app uses) with an exp timestamp in the past, import vi from vitest
and use vi.useFakeTimers/vi.setSystemTime (or equivalent) to advance Date.now()
past exp, then call tryParseDeviceApprovalCookie and assert it resolves to null;
reference tryParseDeviceApprovalCookie and the test case name to locate where to
replace the raw JSON with a real signed cookie and add the vi time manipulation.
In `@apps/web-next/src/lib/pymthouse-discovery-plans.ts`:
- Around line 82-100: The plan- and capability-level discoveryPolicy values are
being cast directly from external JSON (r.discoveryPolicy and
cc.discoveryPolicy) which can propagate malformed policies; implement a
validation/parsing helper (e.g., validateDiscoveryPolicy(policy: unknown):
DiscoveryPolicy | null) that checks required fields (e.g., topN is a positive
integer, slaMinScore is a number in expected range, any filter shapes/types) and
returns a sanitized DiscoveryPolicy or null, then replace the direct casts for
discoveryPolicy and capPolicy with calls to that validator so invalid or
malformed policies are dropped before pushing into capabilities or assigning the
plan-level discoveryPolicy.
In `@apps/web-next/src/lib/pymthouse-manifest.ts`:
- Around line 196-202: The branch in isCapabilityAllowedForProvider that handles
pipeline === '*' only allows capabilities that endWith `/${modelId}` and misses
bare model IDs, so update the condition to also reject a capability equal to the
bare model name; specifically, in the pipeline === '*' branch (and where modelId
is not '*') return true only if modelId === '*' or normalizedCapability ===
modelId or normalizedCapability.endsWith(`/${modelId}`) so a bare
"whisper-large-v3" is treated the same as "*/whisper-large-v3".
In `@apps/web-next/src/middleware.ts`:
- Around line 189-192: The call to encodeDeviceApprovalCookiePayload can throw
when cookie-signing secrets are misconfigured (PYMTHOUSE_DEVICE_COOKIE_SECRET /
NEXTAUTH_SECRET), causing a 500; wrap the await
encodeDeviceApprovalCookiePayload(...) in a try/catch and treat failures the
same as validatePymthouseDeviceInitiateQuery failures by returning the graceful
error redirect/response path instead of letting the exception bubble. Locate the
encode call (encodeDeviceApprovalCookiePayload) and add error handling that logs
the issue and triggers the existing user-facing redirect flow for invalid
device-initiate requests.
In `@packages/utils/src/formatFeeWeiString.ts`:
- Around line 27-37: Normalize maxFractionDigits to a finite integer before
computing cappedDigits: coerce maxFractionDigits to a Number, treat
non-finite/NaN as 0, use Math.floor to drop any fractional part, then clamp that
integer into the 0–18 range to produce cappedDigits used by formatFeeWeiString;
this prevents fractional values from being passed into BigInt exponent math
(affecting expressions using cappedDigits, roundWei, and WEI_PER_ETH).
In `@plugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx`:
- Around line 135-139: The effect is mutating draft.billingProviderSlug via
setDraft when pymthouseConfigured is false, causing existing PymtHouse plans to
become "dirty" and persist a wrong provider; remove this unconditional mutation
and only set a default provider when creating a new plan (not when editing an
existing one). Change the useEffect so it either is removed entirely or its body
first checks a creation flag or draft identity (e.g., draft.id falsy or an
isNewPlan flag) before calling setDraft({ billingProviderSlug: 'daydream' });
alternatively compute the fallback UI/default value from pymthouseConfigured and
effectiveBillingProvider instead of calling setDraft inside useEffect, keeping
references to useEffect, pymthouseConfigured, effectiveBillingProvider,
setDraft, and draft.billingProviderSlug to locate the change.
---
Outside diff comments:
In `@apps/web-next/src/app/api/v1/auth/callback/`[provider]/route.ts:
- Around line 78-85: The catch block currently forwards raw error text to
clients (it uses err.message via the message variable and returns
NextResponse.redirect), so change the error responses to never include
err.message: keep server-side logging of the full error (console.error(err) or
process logger) but map client-facing redirects to fixed, safe error codes
(e.g., 'account_suspended' already used, otherwise use 'oauth_failed' or
provider-specific stable keys) and remove encodeURIComponent(err.message) usage;
update the code paths that reference typedErr and NextResponse.redirect (and the
second catch at the other block) to return only those fixed query values and not
the raw err string, ensuring request is still used for URL construction while
logging full details on the server only.
In `@apps/web-next/src/app/api/v1/auth/providers/`[providerSlug]/start/route.ts:
- Around line 79-81: The request.json() result is being coerced with "as string"
but not validated, so non-string JSON values slip through into Prisma; update
the start route to perform runtime type checks after parsing the body: for
gateway_nonce (variable gatewayNonce) check typeof body.gateway_nonce ===
'string' and use that value only if true, otherwise fall back to
crypto.randomBytes(...).toString('hex'); for gateway_instance_id (variable
gatewayInstanceId) accept body.gateway_instance_id only if typeof === 'string'
(or explicitly allow null) and otherwise set to null; apply these checks where
body is assigned in the handler so malformed client input yields a 4xx
validation path rather than reaching Prisma with wrong types.
- Around line 173-177: The response that returns the login_session_id (the
success({...}) call) must be marked with Cache-Control: no-store to prevent
caching of the redirect-flow continuation handle; update the Daydream branch of
the start route so the response produced by success(...) (the object containing
login_session_id, expires_in, poll_after_ms and using
LOGIN_SESSION_TTL_MS/loginSessionId) sets headers to "Cache-Control: no-store"
(or otherwise returns a no-store response variant) matching the PymtHouse token
branch behavior.
In `@apps/web-next/src/app/api/v1/developer/keys/route.ts`:
- Around line 120-153: The current logic filters out expired PymtHouse keys
in-memory after fetching with prisma.devApiKey.findMany and
prisma.devApiKey.count, causing incorrect total/totalPages and misaligned
pagination; update both prisma.devApiKey.findMany(...) and
prisma.devApiKey.count(...) to include the same "not expired" predicate used by
isExpiredPymthouseKey (or its equivalent DB-safe conditions) in their where
clauses so the DB only returns and counts non-expired keys (keep mapping
toSafeDevApiKey for shaping results but remove the post-query .filter that
alters pagination).
In `@apps/web-next/src/lib/orchestrator-leaderboard/refresh.ts`:
- Around line 35-39: The cache key omits provider scope because
providerRestrictionRevision() is called without the plan's provider; update
buildPlanEvaluationCacheKey to pass the plan's billing provider slug into
providerRestrictionRevision (e.g., const rev =
providerRestrictionRevision(plan.billingProviderSlug) or using the existing slug
variable) so the revision segment reflects the plan's provider and cache keys
roll when a provider-specific manifest changes.
In `@apps/workflows/developer-web/src/pages/DeveloperView.tsx`:
- Around line 17-25: The default active tab in DeveloperView is inconsistent
with the tabs order: the tabs array lists 'api-keys' first but the component's
state initializes activeTab to 'models'. Update the DeveloperView component to
initialize useState<TabId> (activeTab) to 'api-keys' (or derive the default from
tabs[0].id) so the default selection matches the tabs array and aligns with the
plugin's behavior; adjust the initialization in the DeveloperView component
where activeTab is defined.
In `@bin/seed-discovery-plans.ts`:
- Around line 81-89: The unconditional updateMany that migrates
billingProviderSlug 'pymthouse' → 'daydream' should be clarified or scoped:
either (A) treat it as a one‑time legacy fix and restrict it to only plans
created by this seed (e.g., WHERE name startsWith 'naap-default-' or other
unique seed identifiers) and add a clear “run once / remove later” comment next
to the update (reference: the updateMany block in seed-discovery-plans.ts), or
(B) make the backfill logic consistent by not converting existing 'pymthouse' to
'daydream' so that the later null→'pymthouse' backfill and the newly created
defaults use the same provider; pick one approach and implement it so updateMany
and the subsequent default creation/backfill behavior are aligned.
In `@packages/types/src/index.ts`:
- Around line 237-247: Update the DeveloperApiKey type to match the API
response: change ApiKeyStatus from the lowercase union ('active' | 'revoked') to
the API values ('ACTIVE' | 'REVOKED' | 'EXPIRED'), and make the keyHash field
optional or remove it (i.e., change keyHash: string to keyHash?: string) since
handlers omit it; keep other fields (createdAt, expiresAt, lastUsedAt) as-is so
consumers can model the new contract without casts.
In `@plugins/developer-api/backend/src/server.ts`:
- Around line 676-705: The current handler fires
maybeCleanupExpiredPymthouseKeys(userId) but still includes just-expired
PymtHouse keys in the returned list; after fetching keys (the keys variable)
filter them to exclude entries where billingProvider?.slug ===
PYMTHOUSE_PROVIDER_SLUG and computeSignerSessionExpiry(k.createdAt) is <= new
Date(), then map the filtered list into formatted and return that; keep the
maybeCleanupExpiredPymthouseKeys call as-is (fire-and-forget) but ensure
computeSignerSessionExpiry and PYMTHOUSE_PROVIDER_SLUG are referenced when
applying the filter so expired PymtHouse keys are not serialized.
- Around line 928-968: The in-memory create path computes
fallbackPymthouseTokenExpiry but does not reject expired PymtHouse JWTs and
still pushes an ACTIVE key into inMemoryApiKeys; update the logic in the
fallback/create branch to mirror the Prisma behavior: if
fallbackPymthouseTokenExpiry is non-null and <= now, respond with a 400 error
instead of creating newKey (refer to fallbackPymthouseTokenExpiry,
PYMTHOUSE_PROVIDER_SLUG, newKey, inMemoryApiKeys, computeSignerSessionExpiry and
rawApiKey), otherwise proceed to create the key and compute expiresAt as
currently implemented.
In `@plugins/orchestrator-leaderboard/docs/for-ai.md`:
- Around line 121-126: Update the inline comment for the capabilities field in
the schema (the block containing billingProviderSlug and capabilities) so it
matches the server validator: allow dot, colon and slash characters in addition
to alphanumerics, underscore and hyphen and keep the existing constraints (1..50
items, ≤128 chars per item); modify the comment adjacent to capabilities to
explicitly list the allowed characters (so examples like "video/model-a" and
"gemma3:4b" are clearly permitted) and ensure it stays in sync with the
server-side validator.
In `@plugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx`:
- Around line 122-132: bulkToggleCapabilities currently removes only exact
strings so legacy/alias IDs remain; update it to be alias-aware by using the
same matching logic as isCapabilitySelected/toggleCapability: when deselecting,
build removal by testing each existing capability with the same alias-equality
check (i.e., treat a current capability as removed if any of the requested
capabilities matches it via your alias logic), and when selecting, avoid adding
duplicates by checking alias-equality against current entries (use the same
helper used by isCapabilitySelected/toggleCapability); operate on
draft.capabilities/plan?.capabilities and produce next accordingly.
---
Nitpick comments:
In `@apps/web-next/package.json`:
- Line 7: The package.json "build" script currently force-deletes .next
("build": "rimraf .next && next build"), which prevents Next.js incremental
cache reuse; update scripts so the default "build" runs just "next build" and
add explicit alternatives such as "build:ci" that runs rimraf .next && next
build or "build:no-cache" that runs next build --no-cache (or a "build:clean"
script) so cache-clearing is performed only when intended; modify the script
entries named "build" and add the new script keys to package.json accordingly.
In `@apps/web-next/src/lib/api/rate-limit.ts`:
- Around line 57-69: The anonymous rate-limit bucket key currently hashes the
full request.url which allows query-string variants to create separate buckets;
update the key construction (the const key logic that uses createHash and
options.keyPrefix) to use only the request pathname (or options.keyPrefix)
instead of request.url when ip is missing—extract the pathname from the request
(e.g., via new URL(request.url).pathname or request.nextUrl.pathname) and
include that in the hashed payload along with the user-agent, accept, and method
so query parameters are dropped from the anonymous bucket key.
In `@apps/web-next/src/lib/plugins/umd-loader.ts`:
- Around line 354-356: Remove the redundant initial assignment of
didCreateRecord: instead of setting let didCreateRecord = false; then
immediately didCreateRecord = true after managedStylesheets.set(url, record),
initialize didCreateRecord to true where appropriate or set it only once after
successful creation in the function containing managedStylesheets.set; also
update the catch block in the same function to rely on this single flag
(didCreateRecord) for cleanup logic so it accurately reflects whether record was
created or not—search for didCreateRecord and managedStylesheets.set(url,
record) to locate the exact spot.
In `@docs/pymthouse-integration.md`:
- Line 90: The sentence describing PymtHouse manifest behavior is overly long
and hard to read; split it into multiple sentences or a short bulleted list and
explicitly call out the elements referenced (GET {PYMTHOUSE_ISSUER_URL without
/oidc}/apps/{publicClientId}/manifest, excludedCapabilities, capabilities,
manifestVersion, and the PYMTHOUSE_ALLOW_MISSING_MANIFEST_FAIL_OPEN flag) so
each rule is clear: one sentence for how NaaP fetches the manifest, one for
allow/deny rules (excludedCapabilities authoritative; everything else allowed),
one noting capabilities is informational only, one that manifestVersion is for
cache busting, and a final sentence about default deny and the controlled-use
fail-open flag (PYMTHOUSE_ALLOW_MISSING_MANIFEST_FAIL_OPEN=1) and its audit
implication.
- Around line 1-158: Add a concise "Troubleshooting" section to
docs/pymthouse-integration.md that lists common failure scenarios and fixes:
include entries for "M2M authentication fails" (check PYMTHOUSE_M2M_CLIENT_ID /
PYMTHOUSE_M2M_CLIENT_SECRET, PYMTHOUSE_ISSUER_URL and reference
createPmtHouseClientFromEnv), "Device approval returns 400" (verify
device/initiate settings, issuer origin and PMTHOUSE_BASE_URL, and RFC 8628
flow), and "Usage API returns not configured message" (ensure env vars present
and reference GET /api/v1/billing/pymthouse/usage and
PYMTHOUSE_NOT_CONFIGURED_MESSAGE), plus quick remediation steps and links to
syncPymthouseManifestSnapshot and relevant routes
(/api/v1/apps/{publicClientId}/manifest) for manifest/allowlist issues.
In `@plugins/developer-api/frontend/src/pages/DeveloperView.tsx`:
- Around line 143-151: In resolveApiKeyExpiresAt, guard the
computeSignerSessionExpiry call against an invalid key.createdAt by validating
or try-catching the conversion: verify key.createdAt can produce a valid Date
(or call computeSignerSessionExpiry inside a try/catch) and only call
toISOString() when the resulting Date is valid; if validation fails or an
exception is thrown, return null instead of letting toISOString() propagate.
Ensure you reference resolveApiKeyExpiresAt, computeSignerSessionExpiry and
key.createdAt when making the change so the fix is applied in the correct
function.
- Around line 1671-1684: Replace the non-standard Tailwind opacity utilities
used in the table markup: change the header row className that includes
"border-white/8" to use a standard value like "border-white/10", and change the
mapped row className that includes "hover:bg-white/3" to use a standard value
like "hover:bg-white/5" (these are the className strings on the header <tr
className="border-b border-white/8"> and the mapped <tr ... className="border-b
border-white/5 hover:bg-white/3 ..."> respectively).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c01caa3c-5bfd-42df-b6f2-8aafe5c19ebf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (69)
apps/web-next/next.config.jsapps/web-next/package.jsonapps/web-next/src/app/(auth)/login/login-form.tsxapps/web-next/src/app/api/v1/auth/callback/[provider]/route.tsapps/web-next/src/app/api/v1/auth/device-login-hint/route.tsapps/web-next/src/app/api/v1/auth/providers/[providerSlug]/callback/route.tsapps/web-next/src/app/api/v1/auth/providers/[providerSlug]/redirect/route.tsapps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.tsapps/web-next/src/app/api/v1/auth/pymthouse-device-approve/route.tsapps/web-next/src/app/api/v1/billing/pymthouse/token/route.tsapps/web-next/src/app/api/v1/billing/pymthouse/usage/route.test.tsapps/web-next/src/app/api/v1/billing/pymthouse/usage/route.tsapps/web-next/src/app/api/v1/developer/keys/[id]/route.tsapps/web-next/src/app/api/v1/developer/keys/route.tsapps/web-next/src/app/api/v1/orchestrator-leaderboard/capability-catalog/route.tsapps/web-next/src/app/api/v1/orchestrator-leaderboard/dataset/refresh/route.tsapps/web-next/src/app/api/v1/orchestrator-leaderboard/python-gateway/route.tsapps/web-next/src/app/oidc/device-approved/page.tsxapps/web-next/src/components/layout/app-layout.tsxapps/web-next/src/components/layout/top-bar.tsxapps/web-next/src/components/plugin/BackgroundPluginLoader.tsxapps/web-next/src/components/plugin/PluginLoader.tsxapps/web-next/src/contexts/auth-context.tsxapps/web-next/src/lib/api/rate-limit.tsapps/web-next/src/lib/billing-oauth-origin.tsapps/web-next/src/lib/billing-providers.tsapps/web-next/src/lib/formatFeeWeiString.test.tsapps/web-next/src/lib/gateway/__tests__/transform.test.tsapps/web-next/src/lib/orchestrator-leaderboard/__tests__/provider-restrictions.test.tsapps/web-next/src/lib/orchestrator-leaderboard/__tests__/types-validation.test.tsapps/web-next/src/lib/orchestrator-leaderboard/global-refresh.tsapps/web-next/src/lib/orchestrator-leaderboard/plans.tsapps/web-next/src/lib/orchestrator-leaderboard/provider-restrictions.tsapps/web-next/src/lib/orchestrator-leaderboard/refresh.tsapps/web-next/src/lib/orchestrator-leaderboard/types.tsapps/web-next/src/lib/orchestrators-discovery-policy.test.tsapps/web-next/src/lib/orchestrators-discovery-policy.tsapps/web-next/src/lib/plugins/__tests__/integration.test.tsapps/web-next/src/lib/plugins/umd-loader.tsapps/web-next/src/lib/pymthouse-client.tsapps/web-next/src/lib/pymthouse-device-initiate.test.tsapps/web-next/src/lib/pymthouse-device-initiate.tsapps/web-next/src/lib/pymthouse-discovery-plans.test.tsapps/web-next/src/lib/pymthouse-discovery-plans.tsapps/web-next/src/lib/pymthouse-manifest.test.tsapps/web-next/src/lib/pymthouse-manifest.tsapps/web-next/src/middleware.tsapps/workflows/developer-web/src/pages/DeveloperView.tsxbin/seed-discovery-plans.tsdocs/pymthouse-integration.mdpackages/database/src/billing-providers.tspackages/database/src/dev-api/key-utils.tspackages/database/src/index.tspackages/database/src/plugin-discovery.tspackages/types/src/index.tspackages/ui/src/Modal.tsxpackages/utils/src/formatFeeWeiString.tspackages/utils/src/index.tsplugins/developer-api/backend/package.jsonplugins/developer-api/backend/src/server.tsplugins/developer-api/frontend/package.jsonplugins/developer-api/frontend/src/pages/DeveloperView.tsxplugins/orchestrator-leaderboard/docs/for-ai.mdplugins/orchestrator-leaderboard/docs/openapi.yamlplugins/orchestrator-leaderboard/frontend/e2e/leaderboard.spec.tsplugins/orchestrator-leaderboard/frontend/src/hooks/usePlanDetail.tsplugins/orchestrator-leaderboard/frontend/src/pages/LeaderboardPage.tsxplugins/orchestrator-leaderboard/frontend/src/pages/PlanCreatePage.tsxplugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx
- Defer the server-only `@/lib/pymthouse-client` import in pymthouse-manifest until PymtHouse env is configured. Importing it eagerly loaded `@pymthouse/builder-sdk/env`, whose server-only guard throws under jsdom, breaking every test that merely imported the manifest module. - Seed the manifest snapshot directly in the discovery-policy allowlist test instead of routing through a stubbed fetch; the SDK client is a process-wide singleton that binds `fetch` on first construction, so the prior approach was non-deterministic across the suite. Co-authored-by: Cursor <cursoragent@cursor.com>
- Updated middleware to gracefully handle cookie encoding errors, providing user-friendly redirects instead of server errors. - Enhanced API response handling to prevent raw error messages from being exposed to clients. - Implemented no-store cache control for sensitive responses to prevent caching of session data. - Refactored various API routes to ensure consistent error handling and response formatting. - Updated package-lock.json to reflect dependency changes and ensure compatibility. Co-authored-by: Cursor <cursoragent@cursor.com>
…ailable behavior The latency/price sort modes no longer fall back to slaScore (no per-row metric exists); they preserve input order instead. Update the unit test to assert the new contract and add a price case. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web-next/src/lib/plugins/umd-loader.ts (1)
365-365: 💤 Low valueConsider removing redundant
didCreateRecordcheck.
didCreateRecordis alwaystrueon line 365 (unconditional assignment in the creator path). The conditiondidCreateRecord && record.refCount <= 0on line 383 could be simplified to justrecord.refCount <= 0.However, keeping it may improve readability by mirroring the potential future shared-awaiter cleanup structure.
♻️ Optional simplification
- const didCreateRecord = true; + // Creator path: we own this recordAnd in the error handler:
- if (didCreateRecord && record.refCount <= 0) { + if (record.refCount <= 0) {Also applies to: 381-387
🤖 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 `@apps/web-next/src/lib/plugins/umd-loader.ts` at line 365, Remove the redundant didCreateRecord boolean check and simplify the cleanup conditions: where the code currently uses "didCreateRecord && record.refCount <= 0" (and the analogous check in the error handler), drop the didCreateRecord operand and use "record.refCount <= 0" alone; update any early-return logic around the creator path (the variable didCreateRecord which is unconditionally set to true) by removing its declaration/usages to avoid dead state while preserving the existing cleanup behavior in the creator/error paths (referencing didCreateRecord, record.refCount, and the surrounding cleanup block).
🤖 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/types/src/index.ts`:
- Line 237: Some consumers still compare ApiKey.status to lowercase strings;
update all checks to use the new ApiKeyStatus uppercase literals or normalize
the value before comparing. Search for comparisons like oldKey.status ===
'revoked' (and similar in ApiKeyTable, KeyDetailPanel, and rotate route
handlers) and replace with oldKey.status === 'REVOKED' (or call .toUpperCase()
on status first), and update any conditional branches or switches to use
'ACTIVE' | 'REVOKED' | 'EXPIRED' so runtime logic matches the ApiKeyStatus type.
---
Nitpick comments:
In `@apps/web-next/src/lib/plugins/umd-loader.ts`:
- Line 365: Remove the redundant didCreateRecord boolean check and simplify the
cleanup conditions: where the code currently uses "didCreateRecord &&
record.refCount <= 0" (and the analogous check in the error handler), drop the
didCreateRecord operand and use "record.refCount <= 0" alone; update any
early-return logic around the creator path (the variable didCreateRecord which
is unconditionally set to true) by removing its declaration/usages to avoid dead
state while preserving the existing cleanup behavior in the creator/error paths
(referencing didCreateRecord, record.refCount, and the surrounding cleanup
block).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fe0ce23-e335-4491-90d5-e500e9112538
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (28)
apps/web-next/next.config.jsapps/web-next/package.jsonapps/web-next/src/app/api/v1/auth/callback/[provider]/route.tsapps/web-next/src/app/api/v1/auth/providers/[providerSlug]/callback/route.tsapps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.tsapps/web-next/src/app/api/v1/auth/pymthouse-device-approve/route.tsapps/web-next/src/app/api/v1/billing/pymthouse/usage/route.tsapps/web-next/src/app/api/v1/developer/keys/route.tsapps/web-next/src/app/oidc/device-approved/page.tsxapps/web-next/src/lib/api/rate-limit.tsapps/web-next/src/lib/billing-oauth-origin.tsapps/web-next/src/lib/orchestrator-leaderboard/refresh.tsapps/web-next/src/lib/orchestrators-discovery-policy.test.tsapps/web-next/src/lib/orchestrators-discovery-policy.tsapps/web-next/src/lib/plugins/umd-loader.tsapps/web-next/src/lib/pymthouse-device-initiate.test.tsapps/web-next/src/lib/pymthouse-discovery-plans.tsapps/web-next/src/lib/pymthouse-manifest.tsapps/web-next/src/middleware.tsapps/workflows/developer-web/src/pages/DeveloperView.tsxbin/seed-discovery-plans.tsdocs/pymthouse-integration.mdpackages/types/src/index.tspackages/utils/src/formatFeeWeiString.tsplugins/developer-api/backend/src/server.tsplugins/developer-api/frontend/src/pages/DeveloperView.tsxplugins/orchestrator-leaderboard/docs/for-ai.mdplugins/orchestrator-leaderboard/frontend/src/pages/PlanDetailPage.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/pymthouse-integration.md
🚧 Files skipped from review as they are similar to previous changes (19)
- plugins/orchestrator-leaderboard/docs/for-ai.md
- apps/workflows/developer-web/src/pages/DeveloperView.tsx
- apps/web-next/src/lib/pymthouse-device-initiate.test.ts
- apps/web-next/src/lib/api/rate-limit.ts
- apps/web-next/src/lib/billing-oauth-origin.ts
- apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/start/route.ts
- apps/web-next/src/middleware.ts
- packages/utils/src/formatFeeWeiString.ts
- apps/web-next/src/app/api/v1/billing/pymthouse/usage/route.ts
- apps/web-next/src/app/api/v1/developer/keys/route.ts
- apps/web-next/src/lib/pymthouse-discovery-plans.ts
- apps/web-next/src/app/api/v1/auth/providers/[providerSlug]/callback/route.ts
- apps/web-next/src/app/api/v1/auth/pymthouse-device-approve/route.ts
- apps/web-next/src/lib/orchestrators-discovery-policy.ts
- apps/web-next/src/lib/orchestrator-leaderboard/refresh.ts
- plugins/developer-api/frontend/src/pages/DeveloperView.tsx
- apps/web-next/src/lib/pymthouse-manifest.ts
- apps/web-next/src/app/oidc/device-approved/page.tsx
- plugins/developer-api/backend/src/server.ts
Updated the API key status across multiple components and data sources to use uppercase values ('ACTIVE' and 'REVOKED') instead of lowercase ('active' and 'revoked'). This change ensures consistency in status representation throughout the application.
Eliminated the unused 'didCreateRecord' variable from the attachPluginStylesheet function, simplifying the error handling logic for managing stylesheets. This change enhances code clarity and maintains the intended functionality.
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
2 participants
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.
Summary
Stacks the remaining work from #307 on top of #337 (
feat/discovery-plan-billing-provider).PymtHouse auth & billing
user_codeconsent) and device approval routespkceCodeVerifieronBillingProviderOAuthSessionGET /api/v1/billing/pymthouse/tokenand usage API with pipeline-model breakdownDeveloper experience
plugins/developer-apiDeveloperView (usage metrics, keys, discovery plan tooltip)plugins/developer-apibackend token/usage proxy routesdeveloper-webDeveloperView alignmentPlatform
umd-loader, plugin loaders)@pymthouse/builder-sdk+rimrafbuild script inapps/web-nextformatFeeWeiStringutility, Modal escape-to-close, plugin discoveryvisibleToUsersStack
feat/discovery-plan-billing-provider)After both merge, #307 can be closed.
Test plan
user_code→ tokenRelated: #307, #337
Made with Cursor
Summary by CodeRabbit
New Features
Improvements
Documentation