Add SkyFi MCP Satellite Imagery Connection and Tool Integration#748
Conversation
…ration Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds SkyFi OAuth account connection, encrypted token persistence, MCP-based imagery tools, SkyFi-specific agent behavior, settings controls, and streamed result rendering. ChangesSkyFi MCP integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Settings as ToolSelectionForm
participant OAuth as SkyFi OAuth
participant Callback as callback route
participant DB as skyfi_oauth_tokens
User->>Settings: Connect SkyFi account
Settings->>OAuth: Start PKCE authorization
OAuth->>Callback: Return code and state
Callback->>DB: Validate state and save tokens
Callback-->>Settings: Redirect to settings
sequenceDiagram
participant Researcher as researcher
participant Tool as skyfiQueryTool
participant MCP as SkyFi MCP server
participant Results as SkyfiSection
Researcher->>Tool: Submit SkyfiQuery
Tool->>MCP: Execute geocode or imagery action
MCP-->>Tool: Return result text
Tool->>Results: Stream structured output
Results-->>User: Render Markdown results
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Manus seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking findings
-
Billable orders are not confirmation-gated.
lib/agents/tools/skyfi.tsx:212-217,252-254exposesplace_orderto the general model tool and immediately callsskyfi_place_archive_order. The requirement to ask for permission exists only in the tool description; there is no server-side confirmation token, priorvalidate_ordercheck, or other authorization gate. A model-generated call can therefore place a charge without explicit user confirmation. Move ordering behind a separate confirmed action or enforce a server-side confirmation/nonce before calling the billable MCP tool. -
OAuth refresh tokens are stored but never used.
lib/skyfi/provider.ts:70-83returns the persisted access token without checkingtokenExpiry, whilelib/agents/tools/skyfi.tsx:142-158creates the MCP transport with that token directly. The callback persistsrefresh_tokenand expiry, but no refresh-token grant or token rotation is implemented. Once an access token expires, the account remains linked but every MCP call uses stale credentials until the user reconnects. Implement refresh-on-expiry (including persistence of rotated tokens) or explicitly reject expired tokens and require reauthorization. -
Line drawings are sent as AOIs even though the schema requires polygons.
lib/agents/tools/skyfi.tsx:175-183falls back to the first drawn feature when no polygon exists, andgeoJsonToWktat:21-35emitsLINESTRING(...). That value is then passed asaoito search, validation, and ordering at:198-217; the schema documentsaoias a WKT polygon. A user who has drawn only a line will send an invalid AOI to SkyFi instead of receiving a clear validation error. Require a polygon or convert/buffer supported line geometry before invoking those tools.
Validation: reviewed the full 18-file diff and existing PR checks at this head. The existing Vercel – qcx check is failing; Vercel Preview Comments passed, Supabase Preview was skipped, license/cla is pending, and CodeRabbit is rate-limited. No CI-style commands were rerun.
…te imagery Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.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: 15
🤖 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 `@app/api/skyfi/callback/route.ts`:
- Around line 23-24: Replace the inline baseUrl/redirectUri derivation in the
callback handler, including the catch block, with the existing getRedirectUri()
helper from lib/actions/skyfi.ts; import and reuse that helper so both success
and error paths use the consolidated redirect-URI logic.
- Around line 41-53: Update the token-exchange fetch in the callback route to
enforce an abort timeout, using the same timeout pattern and duration as the
existing SkyFi Server Actions. Ensure the timeout signal is passed to the fetch
request and remains scoped to the OAuth token request.
In `@components/settings/components/tool-selection-form.tsx`:
- Around line 187-210: Update the disconnect handler in the SkyFi tool-selection
component to use a dedicated isDisconnecting state, mirroring the existing
isConnecting state, instead of loadingStatus. Set it before the disconnectSkyfi
request and clear it in finally, then use isDisconnecting for the disconnect
button’s loading/disabled UI while preserving loadingStatus for
connection-status checks.
In `@components/skyfi-section.tsx`:
- Line 16: Update the component’s render logic around useStreamableValue and the
existing SearchSkeleton/error UI to handle the returned error state explicitly.
When the stream error is present, render the error message instead of the
loading skeleton; preserve the pending skeleton and successful data rendering
for their existing states.
- Around line 67-70: Confirm that `@/lib/utils` exports the shared cn helper, then
remove the local cn implementation and import the shared utility in the
component that uses it. Preserve all existing class composition behavior while
relying on the app-wide Tailwind conflict-merging implementation.
In `@lib/actions/skyfi.ts`:
- Around line 19-22: Export getRedirectUri from the skyfi action module and
update the callback route to import and reuse it wherever it currently
reconstructs NEXT_PUBLIC_APP_URL, baseUrl, or redirectUri. Remove both inline
constructions while preserving the existing redirect behavior.
- Around line 27-57: Bound the external SkyFi requests in ensureClientRegistered
and the skyfi_whoami check within getSkyfiConnectionStatus with an
AbortController timeout, passing the signal to fetch and handling aborts as
request failures. Use the same timeout behavior for both calls so a hanging
SkyFi server cannot block the action indefinitely.
In `@lib/actions/users.ts`:
- Around line 268-310: The timeout handling in testSkyfiConnection must cancel
the underlying MCP operations instead of abandoning them via Promise.race. Use
AbortController/AbortSignal support in StreamableHTTPClientTransport if
available, apply it to both client.connect and client.callTool timeout paths,
and ensure cleanup does not close the client concurrently with an unfinished
operation.
- Around line 219-263: Remove the unused getSkyfiConfig and saveSkyfiConfig
actions, along with testSkyfiConnection if present in the same module. Delete
their users.metadata.skyfi read/write logic and related imports or helpers,
while preserving the active OAuth actions that use skyfiOAuthTokens.accessToken.
In `@lib/agents/tools/index.tsx`:
- Around line 12-20: Update getTools to conditionally register skyfiQueryTool
only when selectedModel identifies a SkyFi model; leave it out of the returned
tools map for all other models. Ensure the selectedModel check controls the
actual tool surface, not just prompt text, while preserving the existing
registration of unrelated tools.
In `@lib/agents/tools/skyfi.tsx`:
- Around line 189-227: Update the mcpArgs construction in the search,
validate_order, and place_order branches of the queryType switch so extraParams
is spread before the resolved aoi assignment. Ensure resolvedAoiWkt ||
extraParams?.aoi is set last, preventing extraParams.aoi from overriding a map-
or geocoding-derived AOI while preserving the existing fallback behavior.
- Line 126: Update skyfiTool.execute to extract maxResults from params and
include it when building mcpArgs for the search flow, using the exact
skyfi_search_archive_with_map argument name; preserve existing handling for
queryType, location, aoi, and extraParams.
- Around line 250-291: Replace the Promise.race timeout around
mcpClient.callTool in the SkyFi execution flow with the MCP SDK’s request
timeout/cancellation options so the underlying HTTP/SSE request is aborted. For
billable place_order calls, report the timeout through the API before closing
the client, and include a stable idempotency key in the request to prevent
duplicate orders on retries.
In `@lib/db/schema.ts`:
- Around line 133-145: Protect the sensitive fields in
skyfiOAuthTokens—accessToken, refreshToken, and clientSecret—using the
application’s established encryption-at-rest mechanism before persistence and
decrypt them only within SkyfiOAuthProvider when needed. Update the
corresponding schema/persistence and read paths while preserving existing OAuth
behavior; do not leave these credentials as plaintext text values.
In `@lib/skyfi/provider.ts`:
- Around line 29-165: Replace the select-then-insert/update logic in
saveClientInformation, saveTokens, saveCodeVerifier, and saveState with atomic
upserts keyed by skyfiOAuthTokens.userId, preserving existing values not
supplied by each operation. Update saveClientInformation and clientInformation
to persist and return registration_client_uri and registration_access_token from
the DCR response instead of fabricating empty strings.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: bbb53489-cbad-4f01-bebf-2378acdb3f76
📒 Files selected for processing (18)
.env.local.exampleapp/actions.tsxapp/api/skyfi/callback/route.tscomponents/settings/components/settings.tsxcomponents/settings/components/tool-selection-form.tsxcomponents/skyfi-section.tsxcomponents/tool-badge.tsxdrizzle/migrations/0009_add_skyfi_oauth_tokens.sqldrizzle/migrations/meta/0006_snapshot.jsondrizzle/migrations/meta/_journal.jsonlib/actions/skyfi.tslib/actions/users.tslib/agents/researcher.tsxlib/agents/tools/index.tsxlib/agents/tools/skyfi.tsxlib/db/schema.tslib/schema/skyfi.tsxlib/skyfi/provider.ts
📜 Review details
🧰 Additional context used
🪛 SQLFluff (4.2.2)
drizzle/migrations/0009_add_skyfi_oauth_tokens.sql
[error] 16-16: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
🔇 Additional comments (17)
lib/schema/skyfi.tsx (1)
1-40: LGTM!lib/agents/researcher.tsx (1)
16-21: LGTM!Also applies to: 37-37, 55-66, 80-92, 123-128, 163-163
lib/agents/tools/skyfi.tsx (1)
49-56: 🔒 Security & PrivacyVerify
X-Skyfi-Api-Keyheader usage alongsideAuthorization: Bearer.The transport sends the same OAuth access token twice — once as
Authorization: Bearerand once asX-Skyfi-Api-Key. Please confirm with SkyFi's MCP documentation whether the API-key header is actually expected/accepted for OAuth-authenticated sessions, or if this is a leftover from an earlier API-key-based auth approach that should be removed now that OAuth is in place.app/actions.tsx (1)
24-24: LGTM!Also applies to: 857-862
components/tool-badge.tsx (1)
2-2: LGTM!Also applies to: 17-18
.env.local.example (1)
39-43: LGTM!drizzle/migrations/0009_add_skyfi_oauth_tokens.sql (1)
1-16: LGTM!drizzle/migrations/meta/0006_snapshot.json (1)
704-807: LGTM!drizzle/migrations/meta/_journal.json (1)
46-73: LGTM!lib/db/schema.ts (1)
147-165: LGTM!components/settings/components/settings.tsx (1)
25-25: LGTM!Also applies to: 43-43, 63-63, 99-116, 125-153
lib/actions/skyfi.ts (2)
101-145: LGTM!
63-96: LGTM!Also applies to: 150-166
app/api/skyfi/callback/route.ts (1)
8-22: LGTM!Also applies to: 27-40, 54-82
components/settings/components/tool-selection-form.tsx (1)
53-76: LGTM!Also applies to: 156-186, 211-261
lib/skyfi/provider.ts (2)
176-178: 🩺 Stability & AvailabilityNo change needed for
redirectToAuthorization.Current usage only returns an authorize URL manually via
startSkyfiConnection; the agent tool only reads existing tokens, so the no-op redirect method does not hide a supported SDK auth flow.
1-1: 🩺 Stability & AvailabilityNo change needed.
@modelcontextprotocol/sdkv1.x exposes the dist output through a subpath export pattern, and a directdist/esmimport is not currently inconsistent with the pinned SDK version’s resolution behavior.
| Always ask the user for permission before placing a billable order! Ensure you call 'validate_order' first to display the cost.`, | ||
| parameters: skyfiQuerySchema, | ||
| execute: async (params: z.infer<typeof skyfiQuerySchema>) => { | ||
| const { queryType, location, aoi, params: extraParams } = params; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline lib/agents/tools/skyfi.tsx --view expanded || true
echo "== relevant sections =="
wc -l lib/agents/tools/skyfi.tsx
sed -n '1,220p' lib/agents/tools/skyfi.tsx
echo "== search for maxResults/schema occurrences =="
rg -n "maxResults|skyfi_search_archive_with_map|search_archive|tool_schema|params\\.|mcpArgs" lib/agents/tools/skyfi.tsx . -g '!node_modules' -g '!dist' -g '!build' || trueRepository: QueueLab/QCX
Length of output: 13394
🌐 Web query:
skyfi MCP tool skyfi_search_archive_with_map maxResults schema
💡 Result:
Based on the available documentation for SkyFi Model Context Protocol (MCP) servers, there is no tool explicitly named skyfi_search_archive_with_map [1][2]. The standard tool for searching historical imagery in these implementations is typically named skyfi_search_archives [1][3][2]. Regarding your inquiry about a maxResults parameter within the schema for archive searching: 1. Standard MCP Schema: In common open-source implementations of the SkyFi MCP server, the skyfi_search_archives tool input schema typically includes fields such as aoi (Area of Interest), fromDate, toDate, openData, productTypes, and resolution [1]. These implementations often do not explicitly list a maxResults parameter within the base tool's JSON schema [1]. 2. Pagination: Many of these MCP servers manage result sets via pagination rather than a single maxResults limit [4][5]. For example, CLI tools interacting with similar underlying APIs often use parameters like page and pageSize (with defaults such as 25) to control the volume of returned data [5]. 3. Implementation Variance: Because there are multiple community-driven MCP server implementations for SkyFi (e.g., those by NoaheCampbell, jpwilson, and others), the specific schema can vary by repository [1][2][6]. If you are using a specific version, you should verify the schema by calling the MCP tools/list endpoint, which returns the precise tool definitions and input schemas supported by your connected server. If you are encountering a tool name or parameter in an environment that suggests skyfi_search_archive_with_map exists, it may be a custom extension or a specific variation unique to your installation. You can confirm the exact signature by querying the MCP server's tools/list method or checking the schema definition provided by your server's documentation [7][2].
Citations:
- 1: https://github.com/NoaheCampbell/SkyFi-MCP/blob/master/mcp-skyfi-specification.md
- 2: https://github.com/jpwilson/skyfi-mcp
- 3: https://github.com/NoaheCampbell/SkyFi-MCP
- 4: https://github.com/pyrex41/skyfi_mcp
- 5: https://github.com/ianzepp/skyfi-cli
- 6: https://github.com/TylerPohn/skyfi-mcp
- 7: https://glama.ai/mcp/servers/mfuechec/SkyFiMCP
Forward maxResults only for the supported MCP argument name
skyfiQuerySchema.parameters exposes maxResults to the LLM, but skyfiTool.execute currently drops it before building mcpArgs, so the advertised limit is ignored for search. Add it to the handled cases and use the exact skyfi_search_archive_with_map field name for this limit.
🤖 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 `@lib/agents/tools/skyfi.tsx` at line 126, Update skyfiTool.execute to extract
maxResults from params and include it when building mcpArgs for the search flow,
using the exact skyfi_search_archive_with_map argument name; preserve existing
handling for queryType, location, aoi, and extraParams.
| export const skyfiOAuthTokens = pgTable('skyfi_oauth_tokens', { | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }).unique(), | ||
| accessToken: text('access_token'), | ||
| refreshToken: text('refresh_token'), | ||
| tokenExpiry: timestamp('token_expiry', { withTimezone: true }), | ||
| clientId: text('client_id'), | ||
| clientSecret: text('client_secret'), | ||
| codeVerifier: text('code_verifier'), | ||
| state: text('state'), | ||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
OAuth credentials stored in plaintext.
accessToken, refreshToken, and clientSecret are stored as plain text with no application-level encryption. A DB read (backup leak, replica access, SQL injection elsewhere, etc.) directly exposes live SkyFi credentials for every connected user. Consider encrypting these at rest (e.g., envelope encryption with a KMS-managed key, or a pgcrypto/application-layer AEAD wrapper) before persisting, decrypting only when read by SkyfiOAuthProvider.
🤖 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 `@lib/db/schema.ts` around lines 133 - 145, Protect the sensitive fields in
skyfiOAuthTokens—accessToken, refreshToken, and clientSecret—using the
application’s established encryption-at-rest mechanism before persistence and
decrypt them only within SkyfiOAuthProvider when needed. Update the
corresponding schema/persistence and read paths while preserving existing OAuth
behavior; do not leave these credentials as plaintext text values.
…ry tools with token rotation and confirmation gates Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
…ining Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/settings/components/tool-selection-form.tsx (1)
61-77: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStale-response race in connection-status effect.
No cancellation guard means an older
loadStatus()call resolving after a newer one can overwrite state with stale data ifselectedModeltoggles to "SkyFi" and back quickly.🐛 Proposed fix
useEffect(() => { + let cancelled = false; async function loadStatus() { if (selectedModel === "SkyFi") { setLoadingStatus(true); try { const status = await getSkyfiConnectionStatus(); - setSkyfiConnected(status.connected); - setSkyfiBudget(status.budget || null); + if (!cancelled) { + setSkyfiConnected(status.connected); + setSkyfiBudget(status.budget || null); + } } catch (err) { console.error("Failed to load SkyFi connection status:", err); } finally { - setLoadingStatus(false); + if (!cancelled) setLoadingStatus(false); } } } loadStatus(); + return () => { cancelled = true; }; }, [selectedModel]);🤖 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 `@components/settings/components/tool-selection-form.tsx` around lines 61 - 77, Update the useEffect loadStatus flow to ignore results from obsolete requests when selectedModel changes or the effect is cleaned up. Add an active/cancellation guard checked before setSkyfiConnected, setSkyfiBudget, and setLoadingStatus, and deactivate it in the effect cleanup while preserving the existing error handling and SkyFi-only behavior.lib/skyfi/provider.ts (1)
105-164: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRefresh-token fetch has no timeout and is racy under concurrent calls.
Two issues in this block:
- The
fetch('https://mcp.skyfi.com/oauth/token', ...)refresh call (lines 129-139) has noAbortController/timeout, unlike the equivalent calls that were hardened elsewhere in this PR (ensureClientRegistered,getSkyfiConnectionStatus, the callback route's token exchange). A hanging SkyFi server will block whichever caller invokedtokens().- Concurrent calls to
tokens()for the same user near expiry read the same stored refresh token and both attempt to refresh it. If the token endpoint rotates/invalidates refresh tokens on use, the losing call's exchange fails and the function falls through to returning the already-expiring/expireddecryptedAccessToken, silently handing back a stale token to that caller.🔒️ Proposed fix (timeout portion)
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); const response = await fetch('https://mcp.skyfi.com/oauth/token', { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, body: new URLSearchParams({ grant_type: 'refresh_token', client_id: row.clientId, refresh_token: decryptedRefreshToken, }), + signal: controller.signal, }); + clearTimeout(timeoutId);The concurrency race requires serializing refreshes per user (e.g., an in-flight-promise cache keyed by
userId, or a DB-level advisory lock/compare-and-swap on the stored refresh token) to fix properly.🤖 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 `@lib/skyfi/provider.ts` around lines 105 - 164, Update tokens() so the refresh fetch uses an AbortController-based timeout consistent with the other hardened OAuth calls. Serialize refresh attempts per userId using an in-flight promise cache or equivalent lock, and have concurrent callers reuse the completed refresh result or reread stored tokens after waiting rather than returning the stale decryptedAccessToken. Preserve the existing saveTokens and fallback behavior for non-concurrent refresh failures.lib/agents/tools/skyfi.tsx (1)
35-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConnection-timeout race still leaves a dangling
connect()call with no way to ever abort it.
getConnectedSkyfiMcpClient'sPromise.race([client.connect(transport), timeout(15s)])does not cancelclient.connect(transport)when the timeout wins — it just stops waiting. Inexecute(), whengetConnectedSkyfiMcpClientreturnsnullafter this race, the outer 30-secondtimeoutId(whosecontroller.abort()is the only thing that could eventually cancel the abandonedconnect()/underlying fetch) is immediately cleared viaclearTimeout(timeoutId)before it can ever fire. The result: the leaked connection attempt keeps running with nothing left to cancel it.Also applies to: 153-168
🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 35 - 81, Replace the uncancellable Promise.race timeout in getConnectedSkyfiMcpClient with cancellation-aware handling: use an AbortController or the provided signal to abort the transport request when the 15-second connection timeout expires, and ensure the pending client.connect(transport) settles before returning null. Update execute() so its outer timeout signal is not cleared until any in-flight connection attempt has been cancelled and cleaned up.
🤖 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 `@lib/agents/tools/skyfi.tsx`:
- Line 121: The execute flow in the SkyFi tool drops the schema’s top-level
orderId before constructing MCP requests. Destructure orderId from params and
include it in the appropriate mcpArgs for every switch branch that supports
order tracking or placement, while preserving existing extraParams handling and
ensuring the value reaches the SkyFi MCP server.
- Around line 203-206: Update the place_order argument construction and
stableIdempotencyKey derivation: ensure the server-generated key is applied
after spreading extraParams so caller-provided idempotencyKey cannot override
it, and include the order-specific extraParams/orderId data in the hash input so
distinct orders produce distinct keys. Preserve the existing AOI override
behavior and use a deterministic serialization for structured parameters.
- Line 122: Remove or sanitize the full parameter object logged by the Skyfi
tool execution handler near the Execute logging statement. Preserve only
non-sensitive execution context and ensure user-provided location or aoi values
are not written to server logs.
- Around line 188-196: Update the invalid-AOI early-return branch in the SkyFi
query flow to call closeClient(mcpClient) before returning, matching the cleanup
performed by the success and catch-all failure paths. Preserve the existing
timeout clearing, feedback updates, and error response.
- Around line 43-51: Update the MCP tool invocation flow to pass the timeout
AbortSignal through the third options argument of callTool, using callTool(...,
undefined, { signal }) for both skyfi_geocode_aoi and the intended MCP tool;
keep the existing timeout controller and tool arguments unchanged.
In `@lib/utils/encryption.ts`:
- Around line 3-32: Update the encrypt and decrypt functions to use
authenticated AES-256-GCM instead of AES-256-CBC, preserving the existing hashed
key derivation and null handling. Store the IV, ciphertext, and GCM
authentication tag in the encrypted value, and have decrypt parse all
components, set the auth tag, and return null when authentication or decryption
fails.
- Line 4: Update the ENCRYPTION_KEY initialization in encryption.ts to remove
the hardcoded fallback and fail immediately when process.env.ENCRYPTION_KEY is
unset. Preserve the existing 32-byte key requirement and ensure configuration
errors are surfaced during initialization rather than allowing encryption with a
source-visible key.
---
Outside diff comments:
In `@components/settings/components/tool-selection-form.tsx`:
- Around line 61-77: Update the useEffect loadStatus flow to ignore results from
obsolete requests when selectedModel changes or the effect is cleaned up. Add an
active/cancellation guard checked before setSkyfiConnected, setSkyfiBudget, and
setLoadingStatus, and deactivate it in the effect cleanup while preserving the
existing error handling and SkyFi-only behavior.
In `@lib/agents/tools/skyfi.tsx`:
- Around line 35-81: Replace the uncancellable Promise.race timeout in
getConnectedSkyfiMcpClient with cancellation-aware handling: use an
AbortController or the provided signal to abort the transport request when the
15-second connection timeout expires, and ensure the pending
client.connect(transport) settles before returning null. Update execute() so its
outer timeout signal is not cleared until any in-flight connection attempt has
been cancelled and cleaned up.
In `@lib/skyfi/provider.ts`:
- Around line 105-164: Update tokens() so the refresh fetch uses an
AbortController-based timeout consistent with the other hardened OAuth calls.
Serialize refresh attempts per userId using an in-flight promise cache or
equivalent lock, and have concurrent callers reuse the completed refresh result
or reread stored tokens after waiting rather than returning the stale
decryptedAccessToken. Preserve the existing saveTokens and fallback behavior for
non-concurrent refresh failures.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6835cf40-6e3f-4513-97bf-b16091029437
📒 Files selected for processing (12)
app/api/skyfi/callback/route.tscomponents/settings/components/tool-selection-form.tsxcomponents/skyfi-section.tsxdrizzle/migrations/0009_add_skyfi_oauth_tokens.sqllib/actions/skyfi.tslib/agents/tools/index.tsxlib/agents/tools/skyfi.tsxlib/db/schema.tslib/schema/skyfi.tsxlib/skyfi/provider.tslib/utils/encryption.tspublic/sw.js
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
public/sw.js
[warning] Avoid using the initial state variable in setState
Context: setTimeout(t,e)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (10)
drizzle/migrations/0009_add_skyfi_oauth_tokens.sql (1)
1-18: LGTM!lib/db/schema.ts (1)
133-168: LGTM!lib/actions/skyfi.ts (1)
1-193: LGTM!app/api/skyfi/callback/route.ts (1)
1-101: LGTM!components/settings/components/tool-selection-form.tsx (1)
157-270: LGTM!lib/schema/skyfi.tsx (1)
1-43: LGTM!lib/agents/tools/index.tsx (1)
7-20: LGTM! Correctly gatesskyfiQueryToolregistration behindselectedModel === 'SkyFi', resolving the prior concern that the tool was always exposed.Also applies to: 36-42
lib/agents/tools/skyfi.tsx (1)
183-187: 🎯 Functional Correctness | ⚡ Quick win
.find()picks the first Polygon, not the "latest" one.
drawnFeatures.find(f => f.type === 'Polygon')returns the first matching element in array order, but the variable is namedlatestPolygon. IfdrawnFeaturesis appended in chronological order (oldest first), this silently uses a stale AOI instead of the user's most recent drawing. Please confirm the ordering convention ofdrawnFeaturesupstream; if it's not guaranteed most-recent-first, use.slice().reverse().find(...)or track the last-drawn feature explicitly.components/skyfi-section.tsx (1)
1-82: LGTM! Both prior concerns (unsurfaced stream error, divergent localcn) are resolved — explicit error branch added and sharedcnfrom@/lib/utilsnow imported.public/sw.js (1)
1-2: LGTM! This is a regenerated build artifact (Serwist precache manifest); no hand-authored logic changed. The static-analysis "setstate-same-var" hint onsetTimeout(t,e)is a false positive against minified vendor bundle code, not React state.
…SkyFi tool robustness - Migrate encryption/decryption from AES-256-CBC to AES-256-GCM with integrity tag verification and strict ENCRYPTION_KEY environment variable initialization checks. - Add deterministic serialize helper for extraParams and orderId when deriving stableIdempotencyKey in skyfiTool. - Correctly apply idempotencyKey after spreading extraParams to prevent user override. - Destructure orderId and include in mcpArgs for list_orders, validate_order, and place_order. - Support AbortSignal passing in both skyfi MCP client tool calls. - Sanitize tool logging by omitting location and aoi values. - Call closeClient on invalid-AOI early-return path. - Redirect unauthenticated users to /sign-in in startSkyfiConnection. - Add unit tests under tests-unit/encryption.test.ts. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Update startSkyfiConnection() and skyfiTool to dynamically resolve redirectUri from the incoming request headers using next/headers, making it highly robust against mismatches in localhost, preview, or production domains. - Ensure type safety and compilability are maintained. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Enable decrypt() to seamlessly detect and decrypt legacy 2-part AES-256-CBC payloads, resolving issues where pre-existing tokens could not be decrypted. - Add unit tests for legacy CBC backwards-compatibility. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Support forceRegister in ensureClientRegistered(). - Force client re-registration in startSkyfiConnection() to guarantee that client ID redirect URIs always match the current domain dynamically, preventing SkyFi authorization redirect_uri mismatch errors. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
…1de8 Implement AES-256-GCM encryption, secure initialization, and improve SkyFi tool robustness
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/actions/skyfi.ts (1)
59-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the timeout active until the response body is consumed.
fetch()can resolve after headers arrive. Clearing the timer at Line 59 leavesres.text()/res.json()unbounded, so a stalled SkyFi response body can still hang this Server Action. Clear the timer in afinallyblock after parsing completes.Proposed fix
- clearTimeout(timeoutId); - if (!res.ok) { throw new Error(`Dynamic Client Registration failed: ${await res.text()}`); } @@ - } catch (error: any) { + } finally { clearTimeout(timeoutId); - throw error; }🤖 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 `@lib/actions/skyfi.ts` around lines 59 - 75, Move the clearTimeout call in the SkyFi request flow so the timeout remains active through res.text(), res.json(), and client information persistence. Add a finally block around the response handling in the enclosing function and clear timeoutId there, avoiding duplicate cleanup while preserving existing error propagation.lib/agents/tools/skyfi.tsx (2)
241-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent nested parameters from overriding the search limit.
maxResultsis assigned before...extraParams, soparams.maxResultscan silently replace the top-level value and bypass its default. Spread first, then assign the validated top-level limit.Suggested fix
mcpArgs = { - maxResults: maxResults || 5, - ...extraParams, + ...extraParams, + maxResults: maxResults ?? 5, aoi: resolvedAoiWkt || extraParams?.aoi };🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 241 - 245, Update the mcpArgs construction so extraParams is spread before maxResults, allowing the validated top-level maxResults value (with its default) to take precedence over extraParams.maxResults; preserve the existing aoi assignment behavior.
304-315: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat MCP
isErrorresults as failures.MCP tool execution errors resolve with
CallToolResult.isError === true, but this path only readscontent.textand then returnssuccess: truewitherror: null. CheckmcpResult.isErrorbefore building success UI; route isError results through the failure response so failed validation or order operations are not presented as successful.🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 304 - 315, Check mcpResult.isError immediately after callTool returns and before constructing the success response or parsing content blocks. When true, route the result through the existing failure response path, preserving the extracted error details where available; only build the success UI when isError is false.
🤖 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 `@lib/agents/tools/skyfi.tsx`:
- Around line 223-228: Move the stable idempotency-key derivation from its
current position to after the location-geocoding/AOI-resolution flow, using the
finalized resolvedAoiWkt. Update the hash input in the stableIdempotencyKey
calculation to include all order-specific arguments, including the resolved AOI
and location-derived values, so distinct place_order locations cannot share a
key.
- Around line 20-24: Update the host resolution in the callback URL logic to
derive the origin from the configured allowlist rather than trusting
headersList.get('host') directly. Permit HTTP only for exact localhost and
127.0.0.1 values, reject local-like suffixes and arbitrary unapproved hosts, and
preserve the /api/skyfi/callback path for allowed origins.
- Around line 43-49: Update deterministicSerialize to produce a canonical,
type-preserving representation for all supported values: distinguish primitive
types, null, and undefined, and serialize arrays with their order and structure
preserved. Keep object keys sorted for deterministic output, and ensure values
such as numeric versus string fields and null versus undefined cannot generate
the same idempotency key.
In `@tests-unit/encryption.test.ts`:
- Around line 2-6: Ensure ENCRYPTION_KEY is configured before the encryption
module loads: move the fallback assignment ahead of the import in
tests-unit/encryption.test.ts, or set it in the test command/setup. Keep the
existing encrypt and decrypt imports and test key value unchanged.
---
Outside diff comments:
In `@lib/actions/skyfi.ts`:
- Around line 59-75: Move the clearTimeout call in the SkyFi request flow so the
timeout remains active through res.text(), res.json(), and client information
persistence. Add a finally block around the response handling in the enclosing
function and clear timeoutId there, avoiding duplicate cleanup while preserving
existing error propagation.
In `@lib/agents/tools/skyfi.tsx`:
- Around line 241-245: Update the mcpArgs construction so extraParams is spread
before maxResults, allowing the validated top-level maxResults value (with its
default) to take precedence over extraParams.maxResults; preserve the existing
aoi assignment behavior.
- Around line 304-315: Check mcpResult.isError immediately after callTool
returns and before constructing the success response or parsing content blocks.
When true, route the result through the existing failure response path,
preserving the extracted error details where available; only build the success
UI when isError is false.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 090765b0-1606-4337-a038-62de3408619c
📒 Files selected for processing (5)
lib/actions/skyfi.tslib/agents/tools/skyfi.tsxlib/utils/encryption.tspublic/sw.jstests-unit/encryption.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
public/sw.js
[warning] Avoid using the initial state variable in setState
Context: setTimeout(t,e)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (3)
lib/utils/encryption.ts (1)
3-56: LGTM!lib/agents/tools/skyfi.tsx (1)
140-141: LGTM!Also applies to: 212-212, 251-251, 265-268, 274-274, 289-289, 302-303
public/sw.js (1)
2-2: LGTM!
| const headersList = await headers(); | ||
| const host = headersList.get('host'); | ||
| if (host) { | ||
| const protocol = host.startsWith('localhost') || host.startsWith('127.0.0.1') ? 'http' : 'https'; | ||
| return `${protocol}://${host}/api/skyfi/callback`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)skyfi|agents|callback|skyfi' || true
echo "== target file =="
wc -l lib/agents/tools/skyfi.tsx 2>/dev/null || true
cat -n lib/agents/tools/skyfi.tsx 2>/dev/null | sed -n '1,120p'
echo "== relevant usages =="
rg -n "getRedirectUri|redirectUri|skyfi/callback|skyfi" -S . \
-g '!node_modules' -g '!dist' -g '!build' \
| sed -n '1,220p'Repository: QueueLab/QCX
Length of output: 32313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from urllib.parse import urlsplit
def derived(host):
protocol = 'http' if (host.startswith('localhost') or host.startswith('127.0.0.1')) else 'https'
return f'{protocol}://{host}/api/skyfi/callback'
hosts = [
'example.com',
'localhost',
'localhost.attacker.example',
'127.0.0.1',
'127.0.0.1.attacker.example',
'127.0.0.1.example.attacker.example',
'[::1]',
'localhost.example',
'ATTACKER.EG'
]
for h in hosts:
u = derived(h)
print(h, '=>', u, 'scheme=', urlsplit(u).scheme, 'netloc=', urlsplit(u).netloc)
PYRepository: QueueLab/QCX
Length of output: 1077
Validate the host before using it as the SkyFi OAuth redirect origin.
The request Host is trusted directly to build the authorization/callback URL and passed to SkyFi’s OAuth flow. startsWith('localhost') also accepts forged local-like hosts like localhost.attacker.example, while arbitrary hosts can produce untrusted HTTPS redirect URIs. Resolve the origin from a configured allowlist and permit HTTP only for exact local hostnames/IPs.
🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 20 - 24, Update the host resolution
in the callback URL logic to derive the origin from the configured allowlist
rather than trusting headersList.get('host') directly. Permit HTTP only for
exact localhost and 127.0.0.1 values, reject local-like suffixes and arbitrary
unapproved hosts, and preserve the /api/skyfi/callback path for allowed origins.
| function deterministicSerialize(val: any): string { | ||
| if (val === null || val === undefined) return ''; | ||
| if (typeof val !== 'object') return String(val); | ||
| const keys = Object.keys(val).sort(); | ||
| const parts = keys.map(k => `${k}:${deterministicSerialize(val[k])}`); | ||
| return `{${parts.join(',')}}`; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make idempotency serialization type-preserving.
String(val) and the empty fallback collapse distinct values such as {x: 1}/{x: "1"} and {x: null}/{x: undefined}. Because this output contributes to billable place_order idempotency keys, distinct requests can collide and be treated as the same order.
Use a canonical serializer that preserves primitive types, arrays, null, and undefined.
Suggested fix
function deterministicSerialize(val: any): string {
- if (val === null || val === undefined) return '';
- if (typeof val !== 'object') return String(val);
+ if (val === null) return 'null';
+ if (val === undefined) return 'undefined';
+ if (typeof val !== 'object') return `${typeof val}:${String(val)}`;
+ if (Array.isArray(val)) return `[${val.map(deterministicSerialize).join(',')}]`;
const keys = Object.keys(val).sort();
- const parts = keys.map(k => `${k}:${deterministicSerialize(val[k])}`);
+ const parts = keys.map(k => `${JSON.stringify(k)}:${deterministicSerialize(val[k])}`);
return `{${parts.join(',')}}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function deterministicSerialize(val: any): string { | |
| if (val === null || val === undefined) return ''; | |
| if (typeof val !== 'object') return String(val); | |
| const keys = Object.keys(val).sort(); | |
| const parts = keys.map(k => `${k}:${deterministicSerialize(val[k])}`); | |
| return `{${parts.join(',')}}`; | |
| } | |
| function deterministicSerialize(val: any): string { | |
| if (val === null) return 'null'; | |
| if (val === undefined) return 'undefined'; | |
| if (typeof val !== 'object') return `${typeof val}:${String(val)}`; | |
| if (Array.isArray(val)) return `[${val.map(deterministicSerialize).join(',')}]`; | |
| const keys = Object.keys(val).sort(); | |
| const parts = keys.map(k => `${JSON.stringify(k)}:${deterministicSerialize(val[k])}`); | |
| return `{${parts.join(',')}}`; | |
| } |
🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 43 - 49, Update
deterministicSerialize to produce a canonical, type-preserving representation
for all supported values: distinguish primitive types, null, and undefined, and
serialize arrays with their order and structure preserved. Keep object keys
sorted for deterministic output, and ensure values such as numeric versus string
fields and null versus undefined cannot generate the same idempotency key.
| // Stable idempotency key derivation to prevent duplicate order placements | ||
| const serializedExtraParams = deterministicSerialize(extraParams); | ||
| const hashInput = `${userId}_${resolvedAoiWkt || ''}_${queryType}_${orderId || ''}_${serializedExtraParams}`; | ||
| stableIdempotencyKey = crypto.createHash('sha256') | ||
| .update(hashInput) | ||
| .digest('hex'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive the order key after AOI resolution.
The hash is computed before the location-geocoding block. For a place_order request that supplies only location, resolvedAoiWkt is empty and location is omitted, so different locations with the same remaining fields receive the same idempotency key. A legitimate billable order can therefore be mistaken for a retry of another order.
Move key derivation after geocoding and hash the final AOI plus all order-specific arguments.
🤖 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 `@lib/agents/tools/skyfi.tsx` around lines 223 - 228, Move the stable
idempotency-key derivation from its current position to after the
location-geocoding/AOI-resolution flow, using the finalized resolvedAoiWkt.
Update the hash input in the stableIdempotencyKey calculation to include all
order-specific arguments, including the resolved AOI and location-derived
values, so distinct place_order locations cannot share a key.
| import { encrypt, decrypt } from '../lib/utils/encryption'; | ||
|
|
||
| console.log("Starting encryption tests..."); | ||
|
|
||
| const ENCRYPTION_KEY = process.env.ENCRYPTION_KEY || 'my-super-secret-key-32-chars-long-or-more'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git grep -n -C2 -e 'ENCRYPTION_KEY' -e 'tests-unit/encryption.test' -- \
':!lib/utils/encryption.ts'Repository: QueueLab/QCX
Length of output: 952
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'encryption.test.ts|encryption.ts|package.json|.*ci.*|.*workflow.*|.*config.*' . | sed 's#^\./##' | head -200
echo
echo "== tests-unit/encryption.test.ts outline/length =="
wc -l tests-unit/encryption.test.ts
sed -n '1,80p' tests-unit/encryption.test.ts
echo
echo "== lib/utils/encryption.ts outline/length =="
wc -l lib/utils/encryption.ts
sed -n '1,220p' lib/utils/encryption.ts
echo
echo "== package/test scripts =="
if [ -f package.json ]; then
node -e 'const p=require("./package.json"); console.log(JSON.stringify({scripts:p.scripts,devDependencies:p.devDependencies,dependencies:p.dependencies}, null, 2))'
fi
echo
echo "== files mentioning tests script or encryption key =="
git grep -n -C2 -i 'encrypt|encryption\.test|ENCRYPTION_KEY|run tests|test:suite|vitest|jest|tsx|jest-environment' package.json . -- ':!package-lock.json' ':!pnpm-lock.yaml' ':!yarn.lock' ':!node_modules' | head -250Repository: QueueLab/QCX
Length of output: 9875
Set the test key before importing the encryption module.
tests-unit/encryption.test.ts imports ../lib/utils/encryption before applying its local fallback, and lib/utils/encryption.ts throws when process.env.ENCRYPTION_KEY is absent. Configure ENCRYPTION_KEY in the test command/setup before the module loads, or inline the key before importing the module.
🤖 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 `@tests-unit/encryption.test.ts` around lines 2 - 6, Ensure ENCRYPTION_KEY is
configured before the encryption module loads: move the fallback assignment
ahead of the import in tests-unit/encryption.test.ts, or set it in the test
command/setup. Keep the existing encrypt and decrypt imports and test key value
unchanged.
…tion
When an unauthenticated user attempts to connect SkyFi, the server action
previously called redirect('/sign-in') which fails from a server action
context. Now startSkyfiConnection returns an authRequired flag, and the
client-side handler redirects to /sign-in with the current page as
redirect_url, leveraging the existing sign-in redirect pattern.
This ensures users who aren't logged in are properly sent through the
Clerk sign-in flow and returned to settings after authentication, so
they can complete the SkyFi OAuth connection.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/actions/skyfi.ts (3)
161-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the abort timeout active through response-body parsing.
clearTimeout(timeoutId)runs beforeawait res.json(). If SkyFi sends headers but stalls the body,res.json()can still hang indefinitely. Clear the timer only after parsing, preferably infinally.🤖 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 `@lib/actions/skyfi.ts` around lines 161 - 164, Update the request handling around res.json() so timeoutId remains active while parsing the response body; move clearTimeout(timeoutId) into a finally block that runs after response parsing and preserves cleanup for both successful and failed parsing.
163-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not report failed authentication as connected.
Any non-OK response or exception—including revoked/expired credentials, JSON-RPC errors returned with HTTP 200, or timeouts—falls through to
{ connected: true }. The settings UI can therefore show “SkyFi Account Linked” while every tool call fails. Treat authentication failures as disconnected and represent transient upstream failures separately.🤖 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 `@lib/actions/skyfi.ts` around lines 163 - 173, Update the getSkyfiConnectionStatus flow so only a successful, valid whoami response returns connected: true. Handle non-OK responses, JSON-RPC errors, invalid authentication content, exceptions, and timeouts as disconnected, while representing transient upstream failures separately rather than falling through to the unconditional connected result. Preserve the existing successful budget extraction path.
13-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not use an unvalidated
Hostheader as the OAuth redirect origin.
getRedirectUri()reads requestHost, uses it directly asredirect_uri, and treats hosts likelocalhost.attacker.exampleas local by string prefix. Add a strict allowlist for exact local hosts or use a configured canonical origin for the callback.🤖 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 `@lib/actions/skyfi.ts` around lines 13 - 25, Update getRedirectUri to avoid using an unvalidated Host header for the OAuth redirect origin. Accept only exact approved local hosts such as localhost and 127.0.0.1, or otherwise use the configured canonical origin from NEXT_PUBLIC_APP_URL; do not classify hosts by prefix or return arbitrary Host values.
🤖 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.
Outside diff comments:
In `@lib/actions/skyfi.ts`:
- Around line 161-164: Update the request handling around res.json() so
timeoutId remains active while parsing the response body; move
clearTimeout(timeoutId) into a finally block that runs after response parsing
and preserves cleanup for both successful and failed parsing.
- Around line 163-173: Update the getSkyfiConnectionStatus flow so only a
successful, valid whoami response returns connected: true. Handle non-OK
responses, JSON-RPC errors, invalid authentication content, exceptions, and
timeouts as disconnected, while representing transient upstream failures
separately rather than falling through to the unconditional connected result.
Preserve the existing successful budget extraction path.
- Around line 13-25: Update getRedirectUri to avoid using an unvalidated Host
header for the OAuth redirect origin. Accept only exact approved local hosts
such as localhost and 127.0.0.1, or otherwise use the configured canonical
origin from NEXT_PUBLIC_APP_URL; do not classify hosts by prefix or return
arbitrary Host values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1f9d945-5528-4558-b944-04ad9383a59d
📒 Files selected for processing (2)
components/settings/components/tool-selection-form.tsxlib/actions/skyfi.ts
📜 Review details
🔇 Additional comments (2)
lib/actions/skyfi.ts (1)
84-117: LGTM!Also applies to: 183-207
components/settings/components/tool-selection-form.tsx (1)
3-3: LGTM!Also applies to: 23-27, 52-77, 158-274
Remove the entire Account tab from the settings page, including the Discord linking card, connect/disconnect buttons, and the tab trigger. Clean up the now-unused useUser import and clerkUser variable.
…nd functionality The Discord account linking functionality (connect/disconnect via Clerk OAuth) must remain available in the backend and in the tab content for programmatic or background invocation. Only the visible tab trigger button is hidden (className="hidden", disabled) with a clear comment warning future agents not to re-add a visible Discord button to the Settings UI.
2 participants
This PR implements a comprehensive, robust per-user SkyFi MCP tool integration. Users can link their SkyFi accounts securely in the "Tools" Settings panel using an OAuth 2.1 + PKCE flow with dynamic client registration (DCR). The chat agent can then consume active map drawings or geocoded places to build EPSG:4326 WKT Areas of Interest (AOIs) and query/search archive satellite imagery directly, displaying inline result sections and badges.
PR created automatically by Jules for task 5330974344690781074 started by @ngoiyaeric
Summary by CodeRabbit