Add backend API compatibility contract#4
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. Warning Review limit reached
More reviews will be available in 28 minutes and 40 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: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
WalkthroughThis pull request implements a compatibility protocol between independently deployed desktop and backend services. The backend exports a new Sequence Diagram(s)sequenceDiagram
participant ElectronClient as Electron Client
participant CompatibilityAPI as /api/compatibility
participant Proxy as proxyAndBill
participant BackendUpstream as Upstream API
ElectronClient->>CompatibilityAPI: GET /api/compatibility with nevermindDesktopHeaders
CompatibilityAPI-->>ElectronClient: CompatibilityManifest + x-request-id + x-nevermind-backend-version
ElectronClient->>Proxy: API request with nevermindDesktopHeaders
Proxy->>Proxy: Strip x-request-id and x-nevermind-* before forward
Proxy->>BackendUpstream: Forwarded request
BackendUpstream-->>Proxy: Upstream response
Proxy-->>ElectronClient: Response + x-request-id + x-nevermind-backend-version
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/lib/compatibility.test.ts (1)
11-76: ⚡ Quick winReplace anonymous test callbacks with named functions for guideline compliance.
Line 11 and the
test(...)blocks use anonymous arrow callbacks; this conflicts with the TS/JS naming guideline.Proposed refactor
-afterEach(() => { +afterEach(function resetCompatibilityEnv() { delete process.env.NEVERMIND_MIN_DESKTOP_VERSION; delete process.env.NEVERMIND_LATEST_DESKTOP_VERSION; delete process.env.NEVERMIND_DESKTOP_UPDATE_URL; }); -test('parses desktop compatibility headers', () => { +test('parses desktop compatibility headers', function parsesDesktopCompatibilityHeaders() { // ... });As per coding guidelines: "
**/*.{ts,tsx,js}: Prefer named functions over anonymous arrow functions."🤖 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 `@backend/src/lib/compatibility.test.ts` around lines 11 - 76, Replace anonymous arrow callbacks in the test file with named functions: convert the afterEach callback and each test(...) arrow callback (e.g., the callbacks for "parses desktop compatibility headers", "keeps older clients without headers compatible by default", "detects unsupported desktop versions and API versions", "compares semver-like desktop versions", and "returns a stable unsupported-client error shape") into separately declared named functions (e.g., function afterEachCleanup(), function parsesDesktopCompatibilityHeaders(), function keepsOlderClientsCompatible(), function detectsUnsupportedDesktopAndAPI(), function comparesSemverLikeDesktopVersions(), function returnsStableUnsupportedClientErrorShape()) and pass those function identifiers to afterEach and test instead of inline arrows; ensure async tests remain async function declarations so existing awaits work and keep the current assertions and environment setup/teardown logic unchanged.backend/src/lib/compatibility.ts (1)
109-113: 💤 Low valueConsider removing the type cast for better type safety.
Line 110 requires
as 1becauseSUPPORTED_API_VERSIONSis typed asreadonly [1](literal type), but this creates a type safety gap. Consider one of these alternatives:♻️ Alternative approaches
Option 1: Use
.some()for clearer intent and better type inference:- if (client.apiVersion !== null && !SUPPORTED_API_VERSIONS.includes(client.apiVersion as 1)) return 'unsupported_api_version'; + if (client.apiVersion !== null && !SUPPORTED_API_VERSIONS.some(v => v === client.apiVersion)) return 'unsupported_api_version';Option 2: Type the constant as
readonly number[]instead of usingas const:-export const SUPPORTED_API_VERSIONS = [1] as const; +export const SUPPORTED_API_VERSIONS: readonly number[] = [1];Then remove the cast:
- if (client.apiVersion !== null && !SUPPORTED_API_VERSIONS.includes(client.apiVersion as 1)) return 'unsupported_api_version'; + if (client.apiVersion !== null && !SUPPORTED_API_VERSIONS.includes(client.apiVersion)) return 'unsupported_api_version';🤖 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 `@backend/src/lib/compatibility.ts` around lines 109 - 113, In unsupportedClientReason, remove the unsafe cast "as 1" and update the API-version membership check to avoid literal typing on SUPPORTED_API_VERSIONS; for example, change the check to use SUPPORTED_API_VERSIONS.some(v => v === client.apiVersion) (or alternatively change SUPPORTED_API_VERSIONS from a readonly tuple to readonly number[] and keep includes) so the comparison is type-safe and no cast is required, referencing the function unsupportedClientReason and the SUPPORTED_API_VERSIONS constant.
🤖 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 `@package.json`:
- Line 14: The root "test" script in package.json runs backend tests via "pnpm
--dir backend test" but CI only installs deps at repo root (mise exec -- pnpm
install --frozen-lockfile) and there is no pnpm workspace wiring, so backend's
devDependency tsx won't be installed; fix by either adding a pnpm-workspace.yaml
(and ensure backend is included) so root install populates backend/node_modules,
or modify the root test workflow/script to run "pnpm --dir backend install
--frozen-lockfile" before "pnpm --dir backend test" so backend devDependencies
(e.g., tsx from backend/package.json) are installed prior to testing.
In `@src/electron/ai.ts`:
- Around line 410-417: The code currently sets dashboardUrl in the
'unsupported_client' response by calling updateUrlFromErrorText(text), which
trusts arbitrary error text and can produce a malicious download link; change
this to never trust extracted URLs — either remove updateUrlFromErrorText(text)
and always use NEVERMIND_UPDATE_URL, or validate the extracted URL against a
strict allowlist/expected host(s) before using it; update the code paths around
the 'unsupported_client' return (the object with kind:'unsupported_client') and
the updateUrlFromErrorText usage to perform that validation or fall back to
NEVERMIND_UPDATE_URL.
In `@src/electron/nevermind-compatibility.ts`:
- Around line 33-40: The compatibility fetch in checkNevermindCompatibility
currently has no timeout and can hang; modify checkNevermindCompatibility to use
an AbortController (create controller and pass controller.signal to fetch),
start a timer (e.g., 5s or configurable) that calls controller.abort() when
elapsed, and clear the timer after fetch completes; ensure the catch handles
abort errors appropriately (log a timeout-specific warning) and keep existing
behaviour of returning null on failure — reference the function name
checkNevermindCompatibility, the local variables trimmed and res, and the fetch
call to `${trimmed}/api/compatibility`.
---
Nitpick comments:
In `@backend/src/lib/compatibility.test.ts`:
- Around line 11-76: Replace anonymous arrow callbacks in the test file with
named functions: convert the afterEach callback and each test(...) arrow
callback (e.g., the callbacks for "parses desktop compatibility headers", "keeps
older clients without headers compatible by default", "detects unsupported
desktop versions and API versions", "compares semver-like desktop versions", and
"returns a stable unsupported-client error shape") into separately declared
named functions (e.g., function afterEachCleanup(), function
parsesDesktopCompatibilityHeaders(), function keepsOlderClientsCompatible(),
function detectsUnsupportedDesktopAndAPI(), function
comparesSemverLikeDesktopVersions(), function
returnsStableUnsupportedClientErrorShape()) and pass those function identifiers
to afterEach and test instead of inline arrows; ensure async tests remain async
function declarations so existing awaits work and keep the current assertions
and environment setup/teardown logic unchanged.
In `@backend/src/lib/compatibility.ts`:
- Around line 109-113: In unsupportedClientReason, remove the unsafe cast "as 1"
and update the API-version membership check to avoid literal typing on
SUPPORTED_API_VERSIONS; for example, change the check to use
SUPPORTED_API_VERSIONS.some(v => v === client.apiVersion) (or alternatively
change SUPPORTED_API_VERSIONS from a readonly tuple to readonly number[] and
keep includes) so the comparison is type-safe and no cast is required,
referencing the function unsupportedClientReason and the SUPPORTED_API_VERSIONS
constant.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 220c3099-848f-4317-98cd-14f381165ad4
📒 Files selected for processing (16)
.agents/skills/backend-api-compatibility/SKILL.mdAGENTS.mdbackend/.env.examplebackend/package.jsonbackend/src/lib/compatibility.test.tsbackend/src/lib/compatibility.tsbackend/src/lib/proxy.tsbackend/src/pages/api/compatibility.tsbackend/src/pages/api/health.tsbackend/src/pages/api/v1/active-model.tspackage.jsonsrc/docs/backend-api-compatibility.mdsrc/electron/ai.tssrc/electron/nevermind-api.tssrc/electron/nevermind-auth.tssrc/electron/nevermind-compatibility.ts
pablopunk
force-pushed
the
backend-api-compatibility
branch
from
d7e395a to
6c82c32
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/use-ai-chat.ts (1)
7-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
data.actionwhen parsing limit events.
AiLimitStatenow carriesaction, butlimitStateFromEventonly validatestitle/message. Add a minimal runtime guard so malformed action payloads don’t propagate.Proposed fix
function limitStateFromEvent(event: AiChatEvent): AiLimitState | null { const data = event.data as AiLimitState | undefined if (!data || typeof data !== 'object' || !data.title || !data.message) return null - return data + if (data.action && (typeof data.action !== 'object' || !data.action.type || !data.action.title)) { + return { ...data, action: undefined } + } + return data }As per coding guidelines, "Keep every action/search/view payload that crosses Electron IPC
structuredClone-safe; strip handlers/functions after registering them and add clone-safety checks for new payload shapes."🤖 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 `@src/use-ai-chat.ts` around lines 7 - 10, The limitStateFromEvent helper currently only checks title/message and can let malformed actions through; update limitStateFromEvent to validate data.action too by confirming event.data (AiLimitState) has an action property that is clone-safe (e.g. typeof data.action === 'string' and not a function) and meets any expected shape (non-empty string or specific allowed values if available); if the action check fails, return null so malformed payloads don't propagate. Ensure you modify the limitStateFromEvent function and its local variable `data` checks to include this guard.
♻️ Duplicate comments (1)
src/electron/nevermind-compatibility.ts (1)
90-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to compatibility manifest fetch.
Line 92 performs an unbounded network call in a preflight path; a hung backend can block sign-in/AI flows indefinitely.
Proposed fix
async function fetchCompatibilityManifest(baseUrl: string) { const trimmed = normalizeBaseUrl(baseUrl) - const res = await fetch(`${trimmed}/api/compatibility`, { headers: nevermindDesktopHeaders() }) + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 5_000) + let res: Response + try { + res = await fetch(`${trimmed}/api/compatibility`, { + headers: nevermindDesktopHeaders(), + signal: controller.signal, + }) + } finally { + clearTimeout(timeout) + } if (res.status === 404) 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 `@src/electron/nevermind-compatibility.ts` around lines 90 - 93, The fetchCompatibilityManifest function makes an unbounded fetch to `${trimmed}/api/compatibility`; wrap that fetch in an AbortController-based timeout (e.g., 3-5s) so the request is aborted if it hangs: create an AbortController, pass controller.signal to fetch in fetchCompatibilityManifest, set a timer to call controller.abort() after the timeout, and clear the timer after fetch completes; catch the abort/timeout error and return null (same as handling 404) so sign-in/AI preflight won't block indefinitely. Ensure you reference normalizeBaseUrl and nevermindDesktopHeaders unchanged and clean up the timer in all code paths.
🧹 Nitpick comments (2)
backend/src/pages/api/compatibility.ts (1)
4-9: ⚡ Quick winUse a named handler for the route export.
Convert the anonymous arrow at Line 4 to a named function expression to align with the repository convention.
Proposed refactor
-export const GET: APIRoute = ({ request }) => { +export const GET: APIRoute = function GET({ request }) { const requestId = requestIdFromHeaders(request.headers); return Response.json(compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }), { headers: compatibilityHeaders(requestId), }); };As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Prefer named functions over anonymous arrow functions.🤖 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 `@backend/src/pages/api/compatibility.ts` around lines 4 - 9, Replace the anonymous arrow exported as GET with a named function expression (e.g., export const GET: APIRoute = function getCompatibility({ request }) { ... }) so the handler has an explicit name; keep the same implementation using requestIdFromHeaders(request.headers), compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }), and compatibilityHeaders(requestId), and preserve the APIRoute type and returned Response.json call.backend/src/pages/api/contract-routes.test.ts (1)
48-50: 💤 Low valueConsider typing the route context more precisely.
The
as anyassertion bypasses type checking and could hide signature changes. Consider defining a minimal typed context that matches the actual route handler requirements.♻️ Optional: Add a typed context interface
+type MinimalAPIContext = Pick<import('astro').APIContext, 'request' | 'url'>; + function routeContext(request: Request, url = new URL(request.url)) { - return { request, url } as any; + return { request, url } as MinimalAPIContext; }🤖 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 `@backend/src/pages/api/contract-routes.test.ts` around lines 48 - 50, The test helper routeContext currently returns an untyped any which hides signature mismatches; define a small typed interface (e.g., RouteContext { request: Request; url: URL; /* add other fields the handlers expect */ }) and change the function routeContext(request: Request, url = new URL(request.url)) to return that type instead of using `as any` (replace `as any` with the concrete interface or explicit return type) so the route handler tests get proper compile-time checks.
🤖 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 `@backend/src/lib/compatibility.ts`:
- Line 85: The API-version parsing currently uses parsePositiveInteger which
maps non-positive values (e.g. 0) to null and therefore bypasses the
compatibility gate; update the parsing so that non-positive values are preserved
as numeric (not converted to null) so they are treated as explicitly invalid by
the enforcement logic: replace the parsePositiveInteger(...) call used to set
apiVersion (the expression using request.headers.get('x-nevermind-api-version'))
with a parseInteger(...) or equivalent parse that returns the numeric value
(including 0 or negatives) and ensure you do not coerce non-positive numbers
back to null; apply the same change to the other occurrences referenced around
parsePositiveInteger usage (the blocks around the apiVersion assignment and the
checks at lines noted in the comment) so the compatibility checks see an
explicit invalid version instead of a missing one.
In `@package.json`:
- Line 45: The dependency "module-details-from-path" is present in package.json
but not imported anywhere; search the codebase for any require/import of
module-details-from-path (or dynamic uses) and if there are none remove
"module-details-from-path" from package.json and update lockfile (pnpm)
accordingly; if it is required at runtime (e.g., used by an entrypoint script or
dynamically loaded), add a comment in package.json or add the explicit
import/require in the appropriate bootstrap/entrypoint (refer to the entrypoint
file or startup function that needs module-details-from-path) and document why
it’s required so the dependency is justified.
In `@src/electron/main.ts`:
- Around line 1238-1240: The call to currentNevermindCompatibilityManifest()
must be scoped to the active backend by passing its baseUrl so we don't pick
manifests from other cached backends; change the call to
currentNevermindCompatibilityManifest(activeBaseUrl) (or
currentNevermindCompatibilityManifest(getActiveBackendBaseUrl()) /
currentNevermindCompatibilityManifest(activeBackend.baseUrl) depending on which
symbol exists) and ensure the same change is applied at the other site that
calls currentNevermindCompatibilityManifest so the “Update Nevermind” prompt
uses the active backend's manifest.
- Around line 1228-1235: The updateActionForCompatibilityPrompt function
currently returns an openUrl action using backend-provided updateUrl without
validating the scheme; validate updateUrl before returning an {type: 'openUrl',
...} action by parsing it (new URL(updateUrl)) and allowing only http: or https:
schemes (reject other protocols or malformed URLs), and return undefined if
validation fails; update references: updateActionForCompatibilityPrompt,
updateUrl, and the openUrl action so only safe URLs reach shell.openExternal.
In `@src/electron/nevermind-api.ts`:
- Around line 5-13: nevermindDesktopHeaders currently spreads caller-supplied
headers last, allowing callers to override contract fields; update
nevermindDesktopHeaders to prevent overriding the reserved keys
('X-Nevermind-Client','X-Nevermind-Client-Version','X-Nevermind-API-Version','X-Nevermind-Platform','X-Nevermind-Arch')
by either spreading the incoming headers first and then the fixed keys, or by
filtering those reserved keys out of the input headers before merging, so the
function always returns the canonical contract values for those symbols.
---
Outside diff comments:
In `@src/use-ai-chat.ts`:
- Around line 7-10: The limitStateFromEvent helper currently only checks
title/message and can let malformed actions through; update limitStateFromEvent
to validate data.action too by confirming event.data (AiLimitState) has an
action property that is clone-safe (e.g. typeof data.action === 'string' and not
a function) and meets any expected shape (non-empty string or specific allowed
values if available); if the action check fails, return null so malformed
payloads don't propagate. Ensure you modify the limitStateFromEvent function and
its local variable `data` checks to include this guard.
---
Duplicate comments:
In `@src/electron/nevermind-compatibility.ts`:
- Around line 90-93: The fetchCompatibilityManifest function makes an unbounded
fetch to `${trimmed}/api/compatibility`; wrap that fetch in an
AbortController-based timeout (e.g., 3-5s) so the request is aborted if it
hangs: create an AbortController, pass controller.signal to fetch in
fetchCompatibilityManifest, set a timer to call controller.abort() after the
timeout, and clear the timer after fetch completes; catch the abort/timeout
error and return null (same as handling 404) so sign-in/AI preflight won't block
indefinitely. Ensure you reference normalizeBaseUrl and nevermindDesktopHeaders
unchanged and clean up the timer in all code paths.
---
Nitpick comments:
In `@backend/src/pages/api/compatibility.ts`:
- Around line 4-9: Replace the anonymous arrow exported as GET with a named
function expression (e.g., export const GET: APIRoute = function
getCompatibility({ request }) { ... }) so the handler has an explicit name; keep
the same implementation using requestIdFromHeaders(request.headers),
compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }),
and compatibilityHeaders(requestId), and preserve the APIRoute type and returned
Response.json call.
In `@backend/src/pages/api/contract-routes.test.ts`:
- Around line 48-50: The test helper routeContext currently returns an untyped
any which hides signature mismatches; define a small typed interface (e.g.,
RouteContext { request: Request; url: URL; /* add other fields the handlers
expect */ }) and change the function routeContext(request: Request, url = new
URL(request.url)) to return that type instead of using `as any` (replace `as
any` with the concrete interface or explicit return type) so the route handler
tests get proper compile-time checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37cdd9bd-b47e-4604-8588-4ac1b2d9bdc7
📒 Files selected for processing (33)
.agents/skills/backend-api-compatibility/SKILL.md.github/pull_request_template.md.github/workflows/ci.ymlAGENTS.mdbackend/.env.examplebackend/package.jsonbackend/src/db/client.tsbackend/src/fixtures/contracts/desktop-v1/compatibility-manifest.jsonbackend/src/fixtures/contracts/desktop-v1/unsupported-client-error.jsonbackend/src/lib/compatibility.test.tsbackend/src/lib/compatibility.tsbackend/src/lib/cost.tsbackend/src/lib/env.tsbackend/src/lib/proxy.tsbackend/src/lib/ratelimit.tsbackend/src/lib/upstream.tsbackend/src/lib/workos.tsbackend/src/pages/api/auth/signin.tsbackend/src/pages/api/compatibility.tsbackend/src/pages/api/contract-routes.test.tsbackend/src/pages/api/health.tsbackend/src/pages/api/v1/active-model.tspackage.jsonscripts/check-backend-contract-fixtures.cjsscripts/release.shsrc/docs/backend-api-compatibility.mdsrc/electron/ai.tssrc/electron/main.tssrc/electron/nevermind-api.tssrc/electron/nevermind-auth.tssrc/electron/nevermind-compatibility.tssrc/extension-view.tsxsrc/use-ai-chat.ts
✅ Files skipped from review due to trivial changes (9)
- backend/src/fixtures/contracts/desktop-v1/unsupported-client-error.json
- backend/src/lib/env.ts
- backend/src/pages/api/auth/signin.ts
- .github/pull_request_template.md
- scripts/release.sh
- AGENTS.md
- backend/src/fixtures/contracts/desktop-v1/compatibility-manifest.json
- .agents/skills/backend-api-compatibility/SKILL.md
- backend/src/lib/compatibility.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/src/pages/api/v1/active-model.ts
- backend/package.json
- backend/src/pages/api/health.ts
- backend/src/lib/proxy.ts
- src/docs/backend-api-compatibility.md
- src/electron/nevermind-auth.ts
- src/electron/ai.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/use-ai-chat.ts (1)
7-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
data.actionwhen parsing limit events.
AiLimitStatenow carriesaction, butlimitStateFromEventonly validatestitle/message. Add a minimal runtime guard so malformed action payloads don’t propagate.Proposed fix
function limitStateFromEvent(event: AiChatEvent): AiLimitState | null { const data = event.data as AiLimitState | undefined if (!data || typeof data !== 'object' || !data.title || !data.message) return null - return data + if (data.action && (typeof data.action !== 'object' || !data.action.type || !data.action.title)) { + return { ...data, action: undefined } + } + return data }As per coding guidelines, "Keep every action/search/view payload that crosses Electron IPC
structuredClone-safe; strip handlers/functions after registering them and add clone-safety checks for new payload shapes."🤖 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 `@src/use-ai-chat.ts` around lines 7 - 10, The limitStateFromEvent helper currently only checks title/message and can let malformed actions through; update limitStateFromEvent to validate data.action too by confirming event.data (AiLimitState) has an action property that is clone-safe (e.g. typeof data.action === 'string' and not a function) and meets any expected shape (non-empty string or specific allowed values if available); if the action check fails, return null so malformed payloads don't propagate. Ensure you modify the limitStateFromEvent function and its local variable `data` checks to include this guard.
♻️ Duplicate comments (1)
src/electron/nevermind-compatibility.ts (1)
90-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to compatibility manifest fetch.
Line 92 performs an unbounded network call in a preflight path; a hung backend can block sign-in/AI flows indefinitely.
Proposed fix
async function fetchCompatibilityManifest(baseUrl: string) { const trimmed = normalizeBaseUrl(baseUrl) - const res = await fetch(`${trimmed}/api/compatibility`, { headers: nevermindDesktopHeaders() }) + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 5_000) + let res: Response + try { + res = await fetch(`${trimmed}/api/compatibility`, { + headers: nevermindDesktopHeaders(), + signal: controller.signal, + }) + } finally { + clearTimeout(timeout) + } if (res.status === 404) 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 `@src/electron/nevermind-compatibility.ts` around lines 90 - 93, The fetchCompatibilityManifest function makes an unbounded fetch to `${trimmed}/api/compatibility`; wrap that fetch in an AbortController-based timeout (e.g., 3-5s) so the request is aborted if it hangs: create an AbortController, pass controller.signal to fetch in fetchCompatibilityManifest, set a timer to call controller.abort() after the timeout, and clear the timer after fetch completes; catch the abort/timeout error and return null (same as handling 404) so sign-in/AI preflight won't block indefinitely. Ensure you reference normalizeBaseUrl and nevermindDesktopHeaders unchanged and clean up the timer in all code paths.
🧹 Nitpick comments (2)
backend/src/pages/api/compatibility.ts (1)
4-9: ⚡ Quick winUse a named handler for the route export.
Convert the anonymous arrow at Line 4 to a named function expression to align with the repository convention.
Proposed refactor
-export const GET: APIRoute = ({ request }) => { +export const GET: APIRoute = function GET({ request }) { const requestId = requestIdFromHeaders(request.headers); return Response.json(compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }), { headers: compatibilityHeaders(requestId), }); };As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Prefer named functions over anonymous arrow functions.🤖 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 `@backend/src/pages/api/compatibility.ts` around lines 4 - 9, Replace the anonymous arrow exported as GET with a named function expression (e.g., export const GET: APIRoute = function getCompatibility({ request }) { ... }) so the handler has an explicit name; keep the same implementation using requestIdFromHeaders(request.headers), compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }), and compatibilityHeaders(requestId), and preserve the APIRoute type and returned Response.json call.backend/src/pages/api/contract-routes.test.ts (1)
48-50: 💤 Low valueConsider typing the route context more precisely.
The
as anyassertion bypasses type checking and could hide signature changes. Consider defining a minimal typed context that matches the actual route handler requirements.♻️ Optional: Add a typed context interface
+type MinimalAPIContext = Pick<import('astro').APIContext, 'request' | 'url'>; + function routeContext(request: Request, url = new URL(request.url)) { - return { request, url } as any; + return { request, url } as MinimalAPIContext; }🤖 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 `@backend/src/pages/api/contract-routes.test.ts` around lines 48 - 50, The test helper routeContext currently returns an untyped any which hides signature mismatches; define a small typed interface (e.g., RouteContext { request: Request; url: URL; /* add other fields the handlers expect */ }) and change the function routeContext(request: Request, url = new URL(request.url)) to return that type instead of using `as any` (replace `as any` with the concrete interface or explicit return type) so the route handler tests get proper compile-time checks.
🤖 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 `@backend/src/lib/compatibility.ts`:
- Line 85: The API-version parsing currently uses parsePositiveInteger which
maps non-positive values (e.g. 0) to null and therefore bypasses the
compatibility gate; update the parsing so that non-positive values are preserved
as numeric (not converted to null) so they are treated as explicitly invalid by
the enforcement logic: replace the parsePositiveInteger(...) call used to set
apiVersion (the expression using request.headers.get('x-nevermind-api-version'))
with a parseInteger(...) or equivalent parse that returns the numeric value
(including 0 or negatives) and ensure you do not coerce non-positive numbers
back to null; apply the same change to the other occurrences referenced around
parsePositiveInteger usage (the blocks around the apiVersion assignment and the
checks at lines noted in the comment) so the compatibility checks see an
explicit invalid version instead of a missing one.
In `@package.json`:
- Line 45: The dependency "module-details-from-path" is present in package.json
but not imported anywhere; search the codebase for any require/import of
module-details-from-path (or dynamic uses) and if there are none remove
"module-details-from-path" from package.json and update lockfile (pnpm)
accordingly; if it is required at runtime (e.g., used by an entrypoint script or
dynamically loaded), add a comment in package.json or add the explicit
import/require in the appropriate bootstrap/entrypoint (refer to the entrypoint
file or startup function that needs module-details-from-path) and document why
it’s required so the dependency is justified.
In `@src/electron/main.ts`:
- Around line 1238-1240: The call to currentNevermindCompatibilityManifest()
must be scoped to the active backend by passing its baseUrl so we don't pick
manifests from other cached backends; change the call to
currentNevermindCompatibilityManifest(activeBaseUrl) (or
currentNevermindCompatibilityManifest(getActiveBackendBaseUrl()) /
currentNevermindCompatibilityManifest(activeBackend.baseUrl) depending on which
symbol exists) and ensure the same change is applied at the other site that
calls currentNevermindCompatibilityManifest so the “Update Nevermind” prompt
uses the active backend's manifest.
- Around line 1228-1235: The updateActionForCompatibilityPrompt function
currently returns an openUrl action using backend-provided updateUrl without
validating the scheme; validate updateUrl before returning an {type: 'openUrl',
...} action by parsing it (new URL(updateUrl)) and allowing only http: or https:
schemes (reject other protocols or malformed URLs), and return undefined if
validation fails; update references: updateActionForCompatibilityPrompt,
updateUrl, and the openUrl action so only safe URLs reach shell.openExternal.
In `@src/electron/nevermind-api.ts`:
- Around line 5-13: nevermindDesktopHeaders currently spreads caller-supplied
headers last, allowing callers to override contract fields; update
nevermindDesktopHeaders to prevent overriding the reserved keys
('X-Nevermind-Client','X-Nevermind-Client-Version','X-Nevermind-API-Version','X-Nevermind-Platform','X-Nevermind-Arch')
by either spreading the incoming headers first and then the fixed keys, or by
filtering those reserved keys out of the input headers before merging, so the
function always returns the canonical contract values for those symbols.
---
Outside diff comments:
In `@src/use-ai-chat.ts`:
- Around line 7-10: The limitStateFromEvent helper currently only checks
title/message and can let malformed actions through; update limitStateFromEvent
to validate data.action too by confirming event.data (AiLimitState) has an
action property that is clone-safe (e.g. typeof data.action === 'string' and not
a function) and meets any expected shape (non-empty string or specific allowed
values if available); if the action check fails, return null so malformed
payloads don't propagate. Ensure you modify the limitStateFromEvent function and
its local variable `data` checks to include this guard.
---
Duplicate comments:
In `@src/electron/nevermind-compatibility.ts`:
- Around line 90-93: The fetchCompatibilityManifest function makes an unbounded
fetch to `${trimmed}/api/compatibility`; wrap that fetch in an
AbortController-based timeout (e.g., 3-5s) so the request is aborted if it
hangs: create an AbortController, pass controller.signal to fetch in
fetchCompatibilityManifest, set a timer to call controller.abort() after the
timeout, and clear the timer after fetch completes; catch the abort/timeout
error and return null (same as handling 404) so sign-in/AI preflight won't block
indefinitely. Ensure you reference normalizeBaseUrl and nevermindDesktopHeaders
unchanged and clean up the timer in all code paths.
---
Nitpick comments:
In `@backend/src/pages/api/compatibility.ts`:
- Around line 4-9: Replace the anonymous arrow exported as GET with a named
function expression (e.g., export const GET: APIRoute = function
getCompatibility({ request }) { ... }) so the handler has an explicit name; keep
the same implementation using requestIdFromHeaders(request.headers),
compatibilityManifestForRequest(request, { requestId, route: 'compatibility' }),
and compatibilityHeaders(requestId), and preserve the APIRoute type and returned
Response.json call.
In `@backend/src/pages/api/contract-routes.test.ts`:
- Around line 48-50: The test helper routeContext currently returns an untyped
any which hides signature mismatches; define a small typed interface (e.g.,
RouteContext { request: Request; url: URL; /* add other fields the handlers
expect */ }) and change the function routeContext(request: Request, url = new
URL(request.url)) to return that type instead of using `as any` (replace `as
any` with the concrete interface or explicit return type) so the route handler
tests get proper compile-time checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37cdd9bd-b47e-4604-8588-4ac1b2d9bdc7
📒 Files selected for processing (33)
.agents/skills/backend-api-compatibility/SKILL.md.github/pull_request_template.md.github/workflows/ci.ymlAGENTS.mdbackend/.env.examplebackend/package.jsonbackend/src/db/client.tsbackend/src/fixtures/contracts/desktop-v1/compatibility-manifest.jsonbackend/src/fixtures/contracts/desktop-v1/unsupported-client-error.jsonbackend/src/lib/compatibility.test.tsbackend/src/lib/compatibility.tsbackend/src/lib/cost.tsbackend/src/lib/env.tsbackend/src/lib/proxy.tsbackend/src/lib/ratelimit.tsbackend/src/lib/upstream.tsbackend/src/lib/workos.tsbackend/src/pages/api/auth/signin.tsbackend/src/pages/api/compatibility.tsbackend/src/pages/api/contract-routes.test.tsbackend/src/pages/api/health.tsbackend/src/pages/api/v1/active-model.tspackage.jsonscripts/check-backend-contract-fixtures.cjsscripts/release.shsrc/docs/backend-api-compatibility.mdsrc/electron/ai.tssrc/electron/main.tssrc/electron/nevermind-api.tssrc/electron/nevermind-auth.tssrc/electron/nevermind-compatibility.tssrc/extension-view.tsxsrc/use-ai-chat.ts
✅ Files skipped from review due to trivial changes (9)
- backend/src/fixtures/contracts/desktop-v1/unsupported-client-error.json
- backend/src/lib/env.ts
- backend/src/pages/api/auth/signin.ts
- .github/pull_request_template.md
- scripts/release.sh
- AGENTS.md
- backend/src/fixtures/contracts/desktop-v1/compatibility-manifest.json
- .agents/skills/backend-api-compatibility/SKILL.md
- backend/src/lib/compatibility.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/src/pages/api/v1/active-model.ts
- backend/package.json
- backend/src/pages/api/health.ts
- backend/src/lib/proxy.ts
- src/docs/backend-api-compatibility.md
- src/electron/nevermind-auth.ts
- src/electron/ai.ts
🛑 Comments failed to post (5)
backend/src/lib/compatibility.ts (1)
85-85:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid API version values can bypass the compatibility gate.
At Line 227, non-positive versions (for example
0) are converted tonull, so Line 134 skips API-version enforcement. An explicitly invalid API version should be treated as unsupported, not as “missing”.Proposed fix
- apiVersion: parsePositiveInteger(request.headers.get('x-nevermind-api-version')), + apiVersion: parseInteger(request.headers.get('x-nevermind-api-version')), ... -function parsePositiveInteger(value: string | null) { +function parseInteger(value: string | null) { const parsed = Number.parseInt(value || '', 10); - return Number.isFinite(parsed) && parsed > 0 ? parsed : null; + return Number.isFinite(parsed) ? parsed : null; }Also applies to: 133-136, 225-227
🤖 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 `@backend/src/lib/compatibility.ts` at line 85, The API-version parsing currently uses parsePositiveInteger which maps non-positive values (e.g. 0) to null and therefore bypasses the compatibility gate; update the parsing so that non-positive values are preserved as numeric (not converted to null) so they are treated as explicitly invalid by the enforcement logic: replace the parsePositiveInteger(...) call used to set apiVersion (the expression using request.headers.get('x-nevermind-api-version')) with a parseInteger(...) or equivalent parse that returns the numeric value (including 0 or negatives) and ensure you do not coerce non-positive numbers back to null; apply the same change to the other occurrences referenced around parsePositiveInteger usage (the blocks around the apiVersion assignment and the checks at lines noted in the comment) so the compatibility checks see an explicit invalid version instead of a missing one.package.json (1)
45-45:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Search for imports or requires of module-details-from-path rg -n "module-details-from-path" --type ts --type js -C2Repository: pablopunk/nvm
Length of output: 39
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Confirm presence in package.json around the cited line echo "== package.json context ==" python3 - <<'PY' import itertools, json path="package.json" with open(path,'r',encoding='utf-8') as f: for i,line in enumerate(f, start=1): if i>=35 and i<=60: print(f"{i}:{line.rstrip()}") PY # 2) Search for direct usage across more file types echo "== rg usage across repo (all file types) ==" rg -n "module-details-from-path" -C2 || true # 3) Search for related strings that might indicate indirect usage echo "== rg 'details-from-path' ==" rg -n "details-from-path" -C2 || true # 4) Check lockfiles for transitive inclusion echo "== lockfile occurrences ==" for f in pnpm-lock.yaml package-lock.json yarn.lock; do if [ -f "$f" ]; then echo "-- $f --" rg -n "module-details-from-path" "$f" -C1 || true fi doneRepository: pablopunk/nvm
Length of output: 7169
🌐 Web query:
module-details-from-path 1.0.4 native bindings pure javascript files💡 Result:
module-details-from-path 1.0.4 does not use native bindings; it’s implemented in pure JavaScript. Evidence: the package on npm describes its purpose and usage (parsing an absolute file path to return
{ name, basedir, path }) and lists only JavaScript source files (e.g.,index.js) in the package contents, with no mention of native.nodebinaries or build steps [1]. The npm listing shows “0 Dependencies,” consistent with a small, pure-JS utility [1].Citations:
Confirm
module-details-from-pathis actually needed (Electronapp.asarshould be fine)
package.jsonincludesmodule-details-from-pathat1.0.4, but there are no code imports/requires of it anywhere in the repo (onlypackage.json/pnpm-lock.yamlreferences).- The package is pure JavaScript (no native bindings/build artifacts), so it shouldn’t have
app.asarcompatibility issues.- If it isn’t used at runtime, remove it from
dependenciesor add a clear justification/entrypoint that requires it.🤖 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 `@package.json` at line 45, The dependency "module-details-from-path" is present in package.json but not imported anywhere; search the codebase for any require/import of module-details-from-path (or dynamic uses) and if there are none remove "module-details-from-path" from package.json and update lockfile (pnpm) accordingly; if it is required at runtime (e.g., used by an entrypoint script or dynamically loaded), add a comment in package.json or add the explicit import/require in the appropriate bootstrap/entrypoint (refer to the entrypoint file or startup function that needs module-details-from-path) and document why it’s required so the dependency is justified.src/electron/main.ts (2)
1228-1235:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
updateUrlbefore creating anopenUrlaction.Line 1234 trusts backend-provided URL as-is. This allows non-HTTP(S) schemes to flow into
shell.openExternal, which expands attack surface.Proposed fix
+function safeExternalUpdateUrl(raw?: string) { + if (!raw) return null + try { + const parsed = new URL(raw) + return parsed.protocol === 'https:' || parsed.protocol === 'http:' ? parsed.toString() : null + } catch { + return null + } +} + function updateActionForCompatibilityPrompt(updateUrl?: string) { + const safeUrl = safeExternalUpdateUrl(updateUrl) const downloadedInfo = isNewerVersion(updateManager.state.downloadedInfo?.version) ? updateManager.state.downloadedInfo : null const availableInfo = isNewerVersion(updateManager.state.availableInfo?.version) ? updateManager.state.availableInfo : null if (downloadedInfo) return { type: 'installUpdate', title: `Install Nevermind ${downloadedInfo.version || ''}`.trim() } if (availableInfo) return { type: 'downloadUpdate', title: `Download Nevermind ${availableInfo.version || ''}`.trim() } if (updateManager.canUseAutoUpdates()) return { type: 'checkForUpdates', title: 'Check for Update' } - return updateUrl ? { type: 'openUrl', title: 'Download Update', url: updateUrl } : undefined + return safeUrl ? { type: 'openUrl', title: 'Download Update', url: safeUrl } : undefined }🤖 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 `@src/electron/main.ts` around lines 1228 - 1235, The updateActionForCompatibilityPrompt function currently returns an openUrl action using backend-provided updateUrl without validating the scheme; validate updateUrl before returning an {type: 'openUrl', ...} action by parsing it (new URL(updateUrl)) and allowing only http: or https: schemes (reject other protocols or malformed URLs), and return undefined if validation fails; update references: updateActionForCompatibilityPrompt, updateUrl, and the openUrl action so only safe URLs reach shell.openExternal.
1238-1240:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope compatibility prompt to the active backend base URL.
At Line 1238, calling
currentNevermindCompatibilityManifest()withoutbaseUrlselects the latest manifest across all cached backends, which can show the wrong “Update Nevermind” prompt.Also applies to: 3161-3161
🤖 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 `@src/electron/main.ts` around lines 1238 - 1240, The call to currentNevermindCompatibilityManifest() must be scoped to the active backend by passing its baseUrl so we don't pick manifests from other cached backends; change the call to currentNevermindCompatibilityManifest(activeBaseUrl) (or currentNevermindCompatibilityManifest(getActiveBackendBaseUrl()) / currentNevermindCompatibilityManifest(activeBackend.baseUrl) depending on which symbol exists) and ensure the same change is applied at the other site that calls currentNevermindCompatibilityManifest so the “Update Nevermind” prompt uses the active backend's manifest.src/electron/nevermind-api.ts (1)
5-13: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Lock compatibility headers so callers can’t override contract fields.
At Line 12,
...headersis merged last, so any caller can accidentally overrideX-Nevermind-API-Version/ client metadata and break compatibility signaling.Proposed fix
export function nevermindDesktopHeaders(headers: Record<string, string> = {}) { return { + ...headers, 'X-Nevermind-Client': 'desktop', 'X-Nevermind-Client-Version': app.getVersion(), 'X-Nevermind-API-Version': NEVERMIND_DESKTOP_API_VERSION, 'X-Nevermind-Platform': process.platform, 'X-Nevermind-Arch': process.arch, - ...headers, } }Based on learnings: Keep desktop/backend API changes backward-compatible for supported released clients; see
src/docs/backend-api-compatibility.md.🤖 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 `@src/electron/nevermind-api.ts` around lines 5 - 13, nevermindDesktopHeaders currently spreads caller-supplied headers last, allowing callers to override contract fields; update nevermindDesktopHeaders to prevent overriding the reserved keys ('X-Nevermind-Client','X-Nevermind-Client-Version','X-Nevermind-API-Version','X-Nevermind-Platform','X-Nevermind-Arch') by either spreading the incoming headers first and then the fixed keys, or by filtering those reserved keys out of the input headers before merging, so the function always returns the canonical contract values for those symbols.
This was referenced Jun 5, 2026
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
1 participant
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
Adds the first production-safe slice of the desktop/backend compatibility lifecycle so the continuously deployed backend can remain safe for tagged Electron desktop releases.
Plan checklist
Phase 1 — Instrumentation and manifest foundation
GET /api/compatibilitymanifest endpoint.Phase 2 — Desktop compatibility UX
Phase 3 — Contract tests and release gates
pnpm test.Phase 4 — Feature flags and progressive rollout
Phase 5 — API major-version migration path
/api/v1/*as the stable desktop API in docs./api/v2only when a real breaking change exists.Docs and agent workflow
src/docs/backend-api-compatibility.md..agents/skills/backend-api-compatibility/SKILL.md.AGENTS.mdrule for future desktop/backend API changes.Verification
mise exec pnpm -- pnpm testmise exec pnpm -- pnpm --dir backend astro check— passes with existing deprecation hints onlymise exec pnpm -- pnpm --dir backend testFollow-ups / open decisions
NEVERMIND_MIN_DESKTOP_VERSION,NEVERMIND_LATEST_DESKTOP_VERSION, andNEVERMIND_DESKTOP_UPDATE_URL.Summary by CodeRabbit
New Features
Documentation
Tests / CI