feat: sync private provider updates (batch 2) - #223
Closed
hyrious wants to merge 7 commits into
Closed
Conversation
Summary by CodeRabbit
WalkthroughAdds Keepa, Lingxing, Photoroom, and SellerSprite provider integrations with schemas, authentication, executors, runtimes, validation, normalization, and error handling. Extends Shopify Admin with fulfillment, inventory, product, mutation, and bulk-operation actions, including transit-file downloads and expanded response normalization. Sequence Diagram(s)sequenceDiagram
participant LingxingAction
participant LingxingMCPClient
participant LingxingServer
LingxingAction->>LingxingMCPClient: discover or invoke tool
LingxingMCPClient->>LingxingServer: connect with credential
LingxingServer-->>LingxingMCPClient: return tools or result
LingxingMCPClient-->>LingxingAction: return normalized response
sequenceDiagram
participant ShopifyAction
participant ShopifyGraphQL
participant TransitFiles
ShopifyAction->>ShopifyGraphQL: submit bulk query
ShopifyGraphQL-->>ShopifyAction: return operation status
ShopifyAction->>ShopifyGraphQL: retrieve result URL
ShopifyAction->>TransitFiles: store bulk result
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
Collaborator
Author
|
按维护者安排，改为本地合并整理，关闭此拆分 PR。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/providers/sellersprite/actions.ts (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMarketplace code list duplicated across
actions.tsandruntime.ts. The set of nine supported SellerSprite marketplace codes is hardcoded independently in two places; if one list is updated without the other, schema-valid marketplaces would be rejected by the runtime (or vice versa).
src/providers/sellersprite/actions.ts#L8-L18: keep this list as the single source of truth, e.g. export the raw code array used to buildmarketplaceSchema.src/providers/sellersprite/runtime.ts#L18: import the exported code list fromactions.tsand buildsellerSpriteMarketplacesfrom it instead of a separately hardcoded array.🤖 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/providers/sellersprite/actions.ts` around lines 8 - 18, Export the raw marketplace code array used by marketplaceSchema in src/providers/sellersprite/actions.ts as the single source of truth. In src/providers/sellersprite/runtime.ts, import that exported array and derive sellerSpriteMarketplaces from it, removing the duplicated hardcoded list while preserving the existing schema and runtime behavior.src/providers/photoroom/runtime.ts (2)
102-104: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant full-image copy.
readBoundedResponseBytesalready returns a freshly allocated exact-sizeUint8Array, soUint8Array.from(bytes)doubles peak memory for every edit with no benefit.♻️ Proposed simplification
- const stored = await context.transitFiles.create( - new File([Uint8Array.from(bytes)], fileName, { type: contentType }), - ); + const stored = await context.transitFiles.create(new File([bytes], fileName, { type: contentType }));🤖 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/providers/photoroom/runtime.ts` around lines 102 - 104, Remove the redundant Uint8Array.from conversion in the transitFiles.create call within the runtime image-upload flow, passing the freshly allocated bytes directly to File while preserving the existing fileName and contentType metadata.
227-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win402 mapped to 502 invites pointless retries.
Payment-required/credit-exhausted is a durable client-side condition; surfacing it as a gateway error makes retry/backoff layers hammer the API. Consider mapping to 402 (or 400) with the payload preserved.
🤖 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/providers/photoroom/runtime.ts` around lines 227 - 232, Update the 402 handling in the provider response error mapping to return a non-retryable client error status, preferably 402 instead of 502, while preserving the existing payload and response status details in ProviderRequestError.src/providers/shopify_admin/runtime.ts (2)
1102-1107: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the redundant byte copy.
readBoundedResponseBytesalready returns a freshUint8Array;Uint8Array.from(bytes)copies the whole (up tomaxBytes) buffer again beforeFilecopies it once more.♻️ Proposed tweak
- const stored = await context.transitFiles.create(new File([Uint8Array.from(bytes)], name, { type: mimeType })); + const stored = await context.transitFiles.create(new File([bytes], name, { type: mimeType }));🤖 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/providers/shopify_admin/runtime.ts` around lines 1102 - 1107, Remove the redundant Uint8Array.from conversion in the bulk result handling near readBoundedResponseBytes, passing the returned bytes directly to the File constructor while preserving the existing name and MIME type.
660-670: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHardcoded
lineItemsFirst: 250multiplies query cost.With
firstdefaulting to 50 fulfillment orders, each list call requests up to 12,500 line-item nodes, which is a likely throttle/cost-limit failure on large orders. Either expose the page size or pick a smaller default here and let callers page line items viaget_fulfillment_order.♻️ Lower the nested page size
- lineItemsFirst: 250, + lineItemsFirst: optionalInteger(input.lineItemsFirst) ?? 50,🤖 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/providers/shopify_admin/runtime.ts` around lines 660 - 670, Update list_order_fulfillment_orders so lineItemsFirst is no longer hardcoded to 250; use a smaller nested page-size default or an exposed input value, while preserving pagination through get_fulfillment_order for callers needing additional line items.src/providers/shopify_admin/actions.ts (1)
206-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMetafield and product-option inputs accept empty objects.
Every property is listed as optional, so
{}passes validation for a metafield or a product option. Shopify requires at leastkey/value(plustypefor new definitions) andname/valuesfor options, so these end up as remote user errors instead of local input errors.♻️ Tighten the required sets
const metafieldInput = s.object( "A Shopify metafield to create or update on the product.", { id: gid, namespace: s.nonEmptyString("The metafield namespace."), key: s.nonEmptyString("The metafield key."), type: s.nonEmptyString("The Shopify metafield type."), value: s.string("The metafield value encoded as a string."), }, - { optional: ["id", "namespace", "key", "type", "value"] }, + { required: ["key", "value"] }, );const productOptionInput = s.object( "A product option and its possible values.", { name: s.nonEmptyString("The product option name."), position: s.integer("The product option position."), linkedMetafield: linkedMetafieldCreateInput, values: s.array("Values associated with this product option.", productOptionValueInput, { minItems: 1 }), }, - { optional: ["name", "position", "linkedMetafield", "values"] }, + { required: ["name"] }, );🤖 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/providers/shopify_admin/actions.ts` around lines 206 - 258, Require meaningful fields in metafieldInput so an empty object cannot validate: keep id optional, require key and value, and enforce type for new metafield definitions according to the existing create/update contract. In productOptionInput, make name and values required while retaining only genuinely optional fields such as position and linkedMetafield; preserve linkedMetafieldCreateInput’s intended optional values behavior.
🤖 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 `@src/providers/keepa/runtime.ts`:
- Around line 390-412: Update the Keepa response handling around readJsonPayload
and createKeepaError so JSON parse failures do not hardcode status 502. Preserve
response.status for empty or non-JSON bodies, and route those failures through
the existing status/error mapping so 429 and 503 responses retain their
retry-related reasons while valid JSON behavior remains unchanged.
- Around line 398-401: Sanitize the error message in the ProviderRequestError
construction within the Keepa request flow before interpolating it, removing or
replacing the API key from any embedded request URL. Preserve the existing
fallback for non-Error values and ensure the redacted message is used for both
the error payload and logs.
In `@src/providers/lingxing/runtime.ts`:
- Around line 159-164: Update the credential hashing used by the profile
identity in the Lingxing runtime so it incorporates both the configured endpoint
and mcpKey, rather than endpoint alone. Apply this consistently to the accountId
and displayName generation around hashLingxingCredential, including the
corresponding logic near the alternate occurrence, while preserving the
non-reversible hash behavior.
In `@src/providers/photoroom/runtime.ts`:
- Around line 123-147: Restrict both user-supplied image URLs to public HTTP(S)
URLs. In src/providers/photoroom/runtime.ts lines 123-147, pass imageUrl and
background.imageUrl through the shared assertPublicHttpUrl before setting query
parameters; in src/providers/photoroom/actions.ts lines 121-125, define both
corresponding schema fields with s.url(...) to match the output schema. Ensure
no private, local, credential-bearing, or non-HTTP(S) URLs are accepted.
- Around line 149-156: Update the lookup guard in the shadowStyle handling to
accept only own mappings from photoroomShadowModeByStyle, rejecting inherited
Object.prototype keys such as "constructor" before setting shadow.mode. Preserve
the existing unsupported-style ProviderRequestError for invalid or inherited
values.
In `@src/providers/sellersprite/runtime.ts`:
- Around line 289-298: Align runtime validation with the schemas by trimming
surrounding whitespace before strict month and ASIN checks. Update
normalizeOptionalMonth and the related isValidMonth, isAsin, and requireAsin
flows so padded values accepted by monthSchema/asinSchema validate successfully
while preserving the existing YYYYMM and 10-character constraints.
In `@src/providers/shopify_admin/actions.ts`:
- Around line 826-844: Update the get_inventory_quantities action declaration to
include the provider permissions required by InventoryLevel: read_inventory and
the appropriate location read permission. Keep the existing input and output
schemas unchanged, and ensure the declared scopes are exposed alongside the
action’s other metadata.
In `@src/providers/shopify_admin/runtime.ts`:
- Around line 1045-1055: Update readMutationResult so Shopify mutation
userErrors create a caller-error response with outer status 400 while preserving
providerStatus: 422 for Shopify’s status. Keep the existing error message,
userErrors payload, and successful result path unchanged.
- Around line 1074-1092: In downloadShopifyAdminBulkResult, validate the parsed
input.url with assertPublicHttpUrl before issuing the request, then replace
context.fetcher with providerFetch while preserving the existing headers,
timeout signal, and response handling.
---
Nitpick comments:
In `@src/providers/photoroom/runtime.ts`:
- Around line 102-104: Remove the redundant Uint8Array.from conversion in the
transitFiles.create call within the runtime image-upload flow, passing the
freshly allocated bytes directly to File while preserving the existing fileName
and contentType metadata.
- Around line 227-232: Update the 402 handling in the provider response error
mapping to return a non-retryable client error status, preferably 402 instead of
502, while preserving the existing payload and response status details in
ProviderRequestError.
In `@src/providers/sellersprite/actions.ts`:
- Around line 8-18: Export the raw marketplace code array used by
marketplaceSchema in src/providers/sellersprite/actions.ts as the single source
of truth. In src/providers/sellersprite/runtime.ts, import that exported array
and derive sellerSpriteMarketplaces from it, removing the duplicated hardcoded
list while preserving the existing schema and runtime behavior.
In `@src/providers/shopify_admin/actions.ts`:
- Around line 206-258: Require meaningful fields in metafieldInput so an empty
object cannot validate: keep id optional, require key and value, and enforce
type for new metafield definitions according to the existing create/update
contract. In productOptionInput, make name and values required while retaining
only genuinely optional fields such as position and linkedMetafield; preserve
linkedMetafieldCreateInput’s intended optional values behavior.
In `@src/providers/shopify_admin/runtime.ts`:
- Around line 1102-1107: Remove the redundant Uint8Array.from conversion in the
bulk result handling near readBoundedResponseBytes, passing the returned bytes
directly to the File constructor while preserving the existing name and MIME
type.
- Around line 660-670: Update list_order_fulfillment_orders so lineItemsFirst is
no longer hardcoded to 250; use a smaller nested page-size default or an exposed
input value, while preserving pagination through get_fulfillment_order for
callers needing additional line items.
🪄 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: ae1df366-1c17-42b5-b177-417ae4e29a73
📒 Files selected for processing (19)
src/providers/keepa/actions.tssrc/providers/keepa/definition.tssrc/providers/keepa/executors.tssrc/providers/keepa/runtime.tssrc/providers/lingxing/actions.tssrc/providers/lingxing/definition.tssrc/providers/lingxing/executors.tssrc/providers/lingxing/runtime.tssrc/providers/photoroom/actions.tssrc/providers/photoroom/definition.tssrc/providers/photoroom/executors.tssrc/providers/photoroom/runtime.tssrc/providers/sellersprite/actions.tssrc/providers/sellersprite/definition.tssrc/providers/sellersprite/executors.tssrc/providers/sellersprite/runtime.tssrc/providers/shopify_admin/actions.tssrc/providers/shopify_admin/executors.tssrc/providers/shopify_admin/runtime.ts
Comment on lines
+390
to
+412
| payload = await readJsonPayload(response); | ||
| } catch (error) { | ||
| if (error instanceof ProviderRequestError) { | ||
| throw error; | ||
| } | ||
| if (timeoutHandle.didTimeout() || isAbortLikeError(error)) { | ||
| throw new ProviderRequestError(504, "Keepa request timed out"); | ||
| } | ||
| throw new ProviderRequestError( | ||
| 502, | ||
| error instanceof Error ? `Keepa request failed: ${error.message}` : "Keepa request failed", | ||
| ); | ||
| } finally { | ||
| timeoutHandle.cleanup(); | ||
| } | ||
|
|
||
| const envelope = optionalRecord(payload); | ||
| if (!envelope) { | ||
| throw new ProviderRequestError(502, "Keepa returned an invalid response envelope", payload); | ||
| } | ||
| if (!response.ok || envelope.error) { | ||
| throw createKeepaError(response.status, envelope, input.phase); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-JSON error bodies erase the upstream status, defeating 429/503 mapping.
JSON parsing happens before the response.ok check, and readJsonPayload hardcodes 502. A rate-limit response with an empty body, or a gateway 503 HTML page, becomes 502 "Keepa returned invalid JSON" and never reaches createKeepaError, so token_budget_exhausted / service_unavailable reasons are lost for retry logic.
🐛 Proposed fix: preserve the HTTP status on unparseable bodies
- payload = await readJsonPayload(response);
+ payload = await readJsonPayload(response);
} catch (error) {-async function readJsonPayload(response: Response): Promise<unknown> {
- const text = await response.text();
- if (text.trim() === "") {
- throw new ProviderRequestError(502, "Keepa returned an empty response");
- }
- try {
- return JSON.parse(text) as unknown;
- } catch {
- throw new ProviderRequestError(502, "Keepa returned invalid JSON");
- }
-}
+async function readJsonPayload(response: Response): Promise<unknown> {
+ const text = await response.text();
+ const failureStatus = response.ok ? 502 : response.status >= 500 ? response.status : response.status;
+ if (text.trim() === "") {
+ if (!response.ok) {
+ throw new ProviderRequestError(failureStatus, `Keepa request failed with HTTP ${response.status}`);
+ }
+ throw new ProviderRequestError(502, "Keepa returned an empty response");
+ }
+ try {
+ return JSON.parse(text) as unknown;
+ } catch {
+ if (!response.ok) {
+ throw new ProviderRequestError(failureStatus, `Keepa request failed with HTTP ${response.status}`);
+ }
+ throw new ProviderRequestError(502, "Keepa returned invalid JSON");
+ }
+}Also applies to: 416-426
🤖 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/providers/keepa/runtime.ts` around lines 390 - 412, Update the Keepa
response handling around readJsonPayload and createKeepaError so JSON parse
failures do not hardcode status 502. Preserve response.status for empty or
non-JSON bodies, and route those failures through the existing status/error
mapping so 429 and 503 responses retain their retry-related reasons while valid
JSON behavior remains unchanged.
Comment on lines
+398
to
+401
| throw new ProviderRequestError( | ||
| 502, | ||
| error instanceof Error ? `Keepa request failed: ${error.message}` : "Keepa request failed", | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Redact the API key before interpolating fetch error messages.
The key travels in the query string (Line 369), and some network/redirect errors embed the request URL in error.message, which would then be persisted in the error payload and logs.
- throw new ProviderRequestError(
- 502,
- error instanceof Error ? `Keepa request failed: ${error.message}` : "Keepa request failed",
- );
+ const reason = error instanceof Error
+ ? error.message.replaceAll(input.context.apiKey, "[redacted]")
+ : undefined;
+ throw new ProviderRequestError(502, reason ? `Keepa request failed: ${reason}` : "Keepa request failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| throw new ProviderRequestError( | |
| 502, | |
| error instanceof Error ? `Keepa request failed: ${error.message}` : "Keepa request failed", | |
| ); | |
| const reason = error instanceof Error | |
| ? error.message.replaceAll(input.context.apiKey, "[redacted]") | |
| : undefined; | |
| throw new ProviderRequestError( | |
| 502, | |
| reason ? `Keepa request failed: ${reason}` : "Keepa request failed", | |
| ); |
🤖 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/providers/keepa/runtime.ts` around lines 398 - 401, Sanitize the error
message in the ProviderRequestError construction within the Keepa request flow
before interpolating it, removing or replacing the API key from any embedded
request URL. Preserve the existing fallback for non-Error values and ensure the
redacted message is used for both the error payload and logs.
Comment on lines
+159
to
+164
| const credentialHash = hashLingxingCredential(context); | ||
| return { | ||
| profile: { | ||
| accountId: `lingxing:mcp:${credentialHash}`, | ||
| displayName: `Lingxing MCP · ${credentialHash.slice(-6)}`, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive the credential profile ID from the credential as well as the endpoint.
Line 383 hashes only endpoint; accounts using the documented shared MCP URL therefore receive the same accountId and display name. Include mcpKey in the non-reversible hash so configured Lingxing accounts remain distinct.
Proposed fix
-function hashLingxingCredential(credential: Pick<LingxingCredential, "endpoint">): string {
- return createHash("sha256").update(credential.endpoint.toString()).digest("hex").slice(0, 16);
+function hashLingxingCredential(credential: LingxingCredential): string {
+ return createHash("sha256")
+ .update(credential.endpoint.toString())
+ .update("\0")
+ .update(credential.mcpKey)
+ .digest("hex")
+ .slice(0, 16);
}Also applies to: 383-385
🤖 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/providers/lingxing/runtime.ts` around lines 159 - 164, Update the
credential hashing used by the profile identity in the Lingxing runtime so it
incorporates both the configured endpoint and mcpKey, rather than endpoint
alone. Apply this consistently to the accountId and displayName generation
around hashLingxingCredential, including the corresponding logic near the
alternate occurrence, while preserving the non-reversible hash behavior.
Comment on lines
+123
to
+147
| export function buildPhotoroomEditUrl(input: Record<string, unknown>): URL { | ||
| const url = new URL(photoroomEditUrl); | ||
| url.searchParams.set("imageUrl", requireTrimmedInputString(input.imageUrl, "imageUrl")); | ||
|
|
||
| const background = requireInputObject(input.background, "background"); | ||
| const backgroundType = requireInputString(background.type, "background.type"); | ||
| if (backgroundType === "keep") { | ||
| url.searchParams.set("removeBackground", "false"); | ||
| setOptionalString(url, "background.blur.mode", background.blurMode); | ||
| setOptionalNumber(url, "background.blur.radius", background.blurRadius); | ||
| } else if (backgroundType === "color") { | ||
| url.searchParams.set("removeBackground", "true"); | ||
| url.searchParams.set("background.color", requireTrimmedInputString(background.color, "background.color")); | ||
| } else if (backgroundType === "image") { | ||
| url.searchParams.set("removeBackground", "true"); | ||
| url.searchParams.set("background.imageUrl", requireTrimmedInputString(background.imageUrl, "background.imageUrl")); | ||
| setOptionalString(url, "background.scaling", background.scaling); | ||
| } else if (backgroundType === "ai") { | ||
| url.searchParams.set("removeBackground", "true"); | ||
| url.searchParams.set("background.prompt", requireTrimmedInputString(background.prompt, "background.prompt")); | ||
| setOptionalNumber(url, "background.seed", background.seed); | ||
| setOptionalString(url, "background.expandPrompt.mode", background.expandPromptMode); | ||
| } else { | ||
| throw new ProviderRequestError(400, `unsupported photoroom background type: ${backgroundType}`); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
User-supplied image URLs are never constrained to public HTTP(S). Neither the schema (format: "uri") nor the runtime restricts scheme or host, so file://, loopback/metadata, or credential-bearing URLs are accepted and forwarded verbatim to the third party.
src/providers/photoroom/runtime.ts#L123-L147: runimageUrl(Line 125) andbackground.imageUrl(Line 138) through the shared public-onlyassertPublicHttpUrlbeforeurl.searchParams.set.src/providers/photoroom/actions.ts#L121-L125: uses.url(...)forimageUrland forbackground.imageUrlat Line 40, matching the output schema at Line 207.
As per coding guidelines: "User-supplied content or download URLs must always use public-only assertPublicHttpUrl and public-only providerFetch, never a private-aware fetcher."
📍 Affects 2 files
src/providers/photoroom/runtime.ts#L123-L147(this comment)src/providers/photoroom/actions.ts#L121-L125
🤖 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/providers/photoroom/runtime.ts` around lines 123 - 147, Restrict both
user-supplied image URLs to public HTTP(S) URLs. In
src/providers/photoroom/runtime.ts lines 123-147, pass imageUrl and
background.imageUrl through the shared assertPublicHttpUrl before setting query
parameters; in src/providers/photoroom/actions.ts lines 121-125, define both
corresponding schema fields with s.url(...) to match the output schema. Ensure
no private, local, credential-bearing, or non-HTTP(S) URLs are accepted.
Source: Coding guidelines
Comment on lines
+149
to
+156
| const shadowStyle = optionalRawString(input.shadowStyle); | ||
| if (shadowStyle !== undefined) { | ||
| const shadowMode = photoroomShadowModeByStyle[shadowStyle]; | ||
| if (!shadowMode) { | ||
| throw new ProviderRequestError(400, `unsupported photoroom shadow style: ${shadowStyle}`); | ||
| } | ||
| url.searchParams.set("shadow.mode", shadowMode); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Object-literal lookup reaches Object.prototype.
photoroomShadowModeByStyle["constructor"] is truthy, so the guard passes and a function is stringified into shadow.mode. Schema validation covers this today, but this function is exported and validates its own inputs elsewhere.
🛡️ Proposed fix
- const shadowMode = photoroomShadowModeByStyle[shadowStyle];
- if (!shadowMode) {
+ const shadowMode = Object.hasOwn(photoroomShadowModeByStyle, shadowStyle)
+ ? photoroomShadowModeByStyle[shadowStyle]
+ : undefined;
+ if (shadowMode === undefined) {📝 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.
Suggested change
| const shadowStyle = optionalRawString(input.shadowStyle); | |
| if (shadowStyle !== undefined) { | |
| const shadowMode = photoroomShadowModeByStyle[shadowStyle]; | |
| if (!shadowMode) { | |
| throw new ProviderRequestError(400, `unsupported photoroom shadow style: ${shadowStyle}`); | |
| } | |
| url.searchParams.set("shadow.mode", shadowMode); | |
| } | |
| const shadowStyle = optionalRawString(input.shadowStyle); | |
| if (shadowStyle !== undefined) { | |
| const shadowMode = Object.hasOwn(photoroomShadowModeByStyle, shadowStyle) | |
| ? photoroomShadowModeByStyle[shadowStyle] | |
| : undefined; | |
| if (shadowMode === undefined) { | |
| throw new ProviderRequestError(400, `unsupported photoroom shadow style: ${shadowStyle}`); | |
| } | |
| url.searchParams.set("shadow.mode", shadowMode); | |
| } |
🤖 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/providers/photoroom/runtime.ts` around lines 149 - 156, Update the lookup
guard in the shadowStyle handling to accept only own mappings from
photoroomShadowModeByStyle, rejecting inherited Object.prototype keys such as
"constructor" before setting shadow.mode. Preserve the existing
unsupported-style ProviderRequestError for invalid or inherited values.
Comment on lines
+289
to
+298
| function normalizeOptionalMonth(value: unknown): string | undefined { | ||
| if (value === undefined) { | ||
| return undefined; | ||
| } | ||
| const month = requireInputString(value, "month"); | ||
| if (!isValidMonth(month)) { | ||
| throw new ProviderRequestError(400, "month must use YYYYMM format"); | ||
| } | ||
| return month; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Month/ASIN validators reject whitespace the schema explicitly allows.
monthSchema/asinSchema in actions.ts use patterns like ^\s*[0-9]{4}...\s*$ and ^\s*[A-Za-z0-9]{10}\s*$, which tolerate surrounding whitespace. But isValidMonth and isAsin/requireAsin check exact length with no trimming, so a value that passes the input-schema validation (e.g. " 202301 " or " ABC1234567 " with padding) will still be rejected here with a "must use YYYYMM format"/"must contain 10 ASCII letters or digits" error. Either trim before the strict checks, or tighten the schema patterns to match the runtime's actual expectations.
🩹 Proposed fix
function requireAsin(value: unknown, fieldName: string): string {
- const asin = requireInputString(value, fieldName).toUpperCase();
+ const asin = requireInputString(value, fieldName).trim().toUpperCase();
if (!isAsin(asin)) {
throw new ProviderRequestError(400, `${fieldName} must contain 10 ASCII letters or digits`);
}
return asin;
} function normalizeOptionalMonth(value: unknown): string | undefined {
if (value === undefined) {
return undefined;
}
- const month = requireInputString(value, "month");
+ const month = requireInputString(value, "month").trim();
if (!isValidMonth(month)) {
throw new ProviderRequestError(400, "month must use YYYYMM format");
}
return month;
}Also applies to: 326-332, 474-486, 488-501
🤖 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/providers/sellersprite/runtime.ts` around lines 289 - 298, Align runtime
validation with the schemas by trimming surrounding whitespace before strict
month and ASIN checks. Update normalizeOptionalMonth and the related
isValidMonth, isAsin, and requireAsin flows so padded values accepted by
monthSchema/asinSchema validate successfully while preserving the existing
YYYYMM and 10-character constraints.
Comment on lines
+826
to
+844
| action( | ||
| "get_inventory_quantities", | ||
| "Retrieve selected inventory quantity states for one Shopify inventory item at one location.", | ||
| s.actionInput( | ||
| { | ||
| inventoryItemId: gid, | ||
| locationId: gid, | ||
| names: s.stringArray("Inventory quantity state names to retrieve.", { | ||
| minItems: 1, | ||
| itemDescription: "An inventory quantity state name.", | ||
| default: ["available", "on_hand"], | ||
| }), | ||
| includeInactive: s.boolean("Whether Shopify should return the inventory level when it is inactive."), | ||
| }, | ||
| ["inventoryItemId", "locationId"], | ||
| "The Shopify inventory level lookup input.", | ||
| ), | ||
| s.actionOutput({ inventoryLevel: s.nullable(inventoryLevel) }, "The normalized Shopify inventory level response."), | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
get_inventory_quantities declares no provider permissions.
InventoryLevel requires read_inventory (and location data requires location read access), yet this action ships with an empty permission set while every other new action in this batch declares its permissions. Catalog/scope surfaces will under-report what the action needs.
🔧 Declare the read permissions
s.actionOutput({ inventoryLevel: s.nullable(inventoryLevel) }, "The normalized Shopify inventory level response."),
+ { providerPermissions: ["read_inventory", "read_locations"] },
),📝 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.
Suggested change
| action( | |
| "get_inventory_quantities", | |
| "Retrieve selected inventory quantity states for one Shopify inventory item at one location.", | |
| s.actionInput( | |
| { | |
| inventoryItemId: gid, | |
| locationId: gid, | |
| names: s.stringArray("Inventory quantity state names to retrieve.", { | |
| minItems: 1, | |
| itemDescription: "An inventory quantity state name.", | |
| default: ["available", "on_hand"], | |
| }), | |
| includeInactive: s.boolean("Whether Shopify should return the inventory level when it is inactive."), | |
| }, | |
| ["inventoryItemId", "locationId"], | |
| "The Shopify inventory level lookup input.", | |
| ), | |
| s.actionOutput({ inventoryLevel: s.nullable(inventoryLevel) }, "The normalized Shopify inventory level response."), | |
| ), | |
| action( | |
| "get_inventory_quantities", | |
| "Retrieve selected inventory quantity states for one Shopify inventory item at one location.", | |
| s.actionInput( | |
| { | |
| inventoryItemId: gid, | |
| locationId: gid, | |
| names: s.stringArray("Inventory quantity state names to retrieve.", { | |
| minItems: 1, | |
| itemDescription: "An inventory quantity state name.", | |
| default: ["available", "on_hand"], | |
| }), | |
| includeInactive: s.boolean("Whether Shopify should return the inventory level when it is inactive."), | |
| }, | |
| ["inventoryItemId", "locationId"], | |
| "The Shopify inventory level lookup input.", | |
| ), | |
| s.actionOutput({ inventoryLevel: s.nullable(inventoryLevel) }, "The normalized Shopify inventory level response."), | |
| { providerPermissions: ["read_inventory", "read_locations"] }, | |
| ), |
🤖 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/providers/shopify_admin/actions.ts` around lines 826 - 844, Update the
get_inventory_quantities action declaration to include the provider permissions
required by InventoryLevel: read_inventory and the appropriate location read
permission. Keep the existing input and output schemas unchanged, and ensure the
declared scopes are exposed alongside the action’s other metadata.
Comment on lines
+1045
to
+1055
| function readMutationResult(payload: ShopifyAdminGraphQLResponse, fieldName: string): Record<string, unknown> { | ||
| const result = readObject(readObject(payload.data, "data")[fieldName], fieldName); | ||
| const errors = readMutationUserErrors(result.userErrors); | ||
| if (errors.length > 0) { | ||
| throw new ProviderRequestError(502, `shopify_admin ${fieldName} user error: ${errors.join("; ")}`, { | ||
| providerStatus: 422, | ||
| userErrors: result.userErrors, | ||
| }); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How do other providers classify provider-side user/validation errors?
rg -nP -C3 'new ProviderRequestError\(\s*(422|400)' src/providers | head -60
rg -nP -C3 'providerStatus' src/providers src/core | head -60Repository: oomol-lab/open-connector
Length of output: 8231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Shopify admin runtime around the mutation error path and related error handling.
sed -n '1010,1075p' src/providers/shopify_admin/runtime.ts
printf '\n---\n'
# Find how ProviderRequestError is interpreted elsewhere in the repo.
rg -n -C2 'ProviderRequestError\(' src | head -200
printf '\n---\n'
# Look for Shopify admin-specific error mapping or conventions.
rg -n -C2 'shopify_admin.*providerStatus|providerStatus.*shopify_admin|userErrors|mutation user error' src/providers/shopify_admin src | head -200Repository: oomol-lab/open-connector
Length of output: 23713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ProviderRequestError shape and how its status/providerStatus are used.
rg -n -C3 'class ProviderRequestError|interface ProviderRequestError|type ProviderRequestError|providerStatus' src/core src | head -240
printf '\n---\n'
# Check whether 502 is used as a generic wrapper for upstream failures rather than caller errors.
rg -n -C2 'new ProviderRequestError\(502' src/providers src/core | head -200Repository: oomol-lab/open-connector
Length of output: 18075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ProviderRequestError definition and how its status is intended to be used.
sed -n '130,210p' src/providers/provider-runtime.ts
printf '\n---\n'
# Find any logic that maps ProviderRequestError.status to execution/runtime outcomes.
rg -n -C3 'error\.status|instanceof ProviderRequestError|ProviderRequestError' src/core src/providers | head -240Repository: oomol-lab/open-connector
Length of output: 18788
Map Shopify mutation userErrors to 400/422, not 502. These are input/validation failures, so the outer status should be a caller error; keep providerStatus: 422 only if you need Shopify’s code. src/providers/shopify_admin/runtime.ts:1047-1051
🤖 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/providers/shopify_admin/runtime.ts` around lines 1045 - 1055, Update
readMutationResult so Shopify mutation userErrors create a caller-error response
with outer status 400 while preserving providerStatus: 422 for Shopify’s status.
Keep the existing error message, userErrors payload, and successful result path
unchanged.
Comment on lines
+1074
to
+1092
| async function downloadShopifyAdminBulkResult( | ||
| input: Record<string, unknown>, | ||
| context: ShopifyAdminActionContext, | ||
| ): Promise<Record<string, unknown>> { | ||
| if (!context.transitFiles) { | ||
| throw new ProviderRequestError(500, "shopify_admin download_bulk_result requires transit file storage"); | ||
| } | ||
|
|
||
| const url = requiredString(input.url, "url", providerInputError); | ||
| const timeout = createProviderTimeout(context.signal, bulkResultDownloadTimeoutMs); | ||
| let response: Response | undefined; | ||
| try { | ||
| response = await context.fetcher(url, { | ||
| headers: { | ||
| accept: "application/jsonl, application/x-ndjson, application/json, text/plain", | ||
| "user-agent": providerUserAgent, | ||
| }, | ||
| signal: timeout.signal, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Signature of the shared public-URL guard and how other providers download user-supplied URLs.
ast-grep run --pattern 'export async function assertPublicHttpUrl($$$) { $$$ }' --lang typescript src
ast-grep run --pattern 'export function assertPublicHttpUrl($$$) { $$$ }' --lang typescript src
rg -nP -C4 '\bassertPublicHttpUrl\s*\(' src/providers
# Does shopify_admin build its fetcher with private network access allowed?
rg -nP -C6 'allowPrivateNetwork|isPrivateNetworkAccessAllowed|createProviderFetch|skipDnsValidation' src/providers/shopify_adminRepository: oomol-lab/open-connector
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/providers/shopify_admin/runtime.ts
echo "=== outline ==="
ast-grep outline "$file" --view expanded | sed -n '1,220p'
echo
echo "=== relevant lines around downloadShopifyAdminBulkResult ==="
nl -ba "$file" | sed -n '1040,1125p'
echo
echo "=== search for validation/fetcher helpers in file ==="
rg -n -C 3 'assertPublicHttpUrl|providerFetch|createProviderFetch|allowPrivateNetwork|isPrivateNetworkAccessAllowed|input\.url|bulkResult' "$file"Repository: oomol-lab/open-connector
Length of output: 7574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/providers/shopify_admin/runtime.ts
echo "=== download helper section ==="
sed -n '1074,1125p' "$file" | cat -n
echo
echo "=== call sites for downloadShopifyAdminBulkResult ==="
rg -n -C 4 'downloadShopifyAdminBulkResult\(' "$file"
echo
echo "=== URL/result extraction around bulk operation handling ==="
sed -n '988,1045p' "$file" | cat -n
echo
echo "=== any URL validation helpers in this file ==="
rg -n -C 3 'assertPublicHttpUrl|new URL\(|host is required|myshopify|bulk' "$file"Repository: oomol-lab/open-connector
Length of output: 11479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== bulk operation shape / URL fields ==="
rg -n -C 4 'normalizeBulkOperation|downloadUrl|url:' src/providers/shopify_admin/runtime.ts
echo
echo "=== action input schema for download_bulk_result ==="
rg -n -C 5 'download_bulk_result|download bulk result|fileName|url' src/providers/shopify_admin
echo
echo "=== fetcher construction / private-network handling in Shopify Admin provider ==="
rg -n -C 4 'createProviderFetch|providerFetch|allowPrivateNetwork|isPrivateNetworkAccessAllowed|context\.fetcher' src/providers/shopify_admin src/providersRepository: oomol-lab/open-connector
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== bulk operation normalization and download URL fields ==="
sed -n '1248,1270p' src/providers/shopify_admin/runtime.ts | cat -n
echo
sed -n '880,910p' src/providers/shopify_admin/runtime.ts | cat -n
echo
echo "=== provider fetcher construction in shopify_admin ==="
rg -n -C 6 'createProviderFetch|providerFetch|context\.fetcher|allowPrivateNetwork|isPrivateNetworkAccessAllowed' src/providers/shopify_admin src/providers | sed -n '1,220p'Repository: oomol-lab/open-connector
Length of output: 17167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== download_bulk_result schema / definition ==="
rg -n -C 4 'download_bulk_result|download bulk result|bulk result' src/providers/shopify_admin
echo
echo "=== Shopify Admin provider files ==="
git ls-files src/providers/shopify_admin
echo
echo "=== fetcher construction / private-network config in Shopify Admin files ==="
rg -n -C 4 'createProviderFetch|providerFetch|allowPrivateNetwork|isPrivateNetworkAccessAllowed|fetcher:' src/providers/shopify_adminRepository: oomol-lab/open-connector
Length of output: 9705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== s.url definition ==="
rg -n -C 4 'function url\(|url\(' src | sed -n '1,220p'
echo
echo "=== bulk result input type / schema helpers in shopify_admin ==="
sed -n '1008,1019p' src/providers/shopify_admin/actions.ts | cat -n
echo
echo "=== any URL validation in action schema for shopify_admin download_bulk_result ==="
rg -n -C 4 'actionInput\(|s\.url\(|partialDataUrl|download_bulk_result' src/providers/shopify_admin/actions.ts src/providers/shopify_admin/definition.tsRepository: oomol-lab/open-connector
Length of output: 39751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== search for s.url helper definition ==="
rg -n -C 4 's\.url\(|url:\s*s\.url|function url\(' src packages
echo
echo "=== shopify_admin download_bulk_result action definition ==="
sed -n '1008,1019p' src/providers/shopify_admin/actions.ts | cat -nRepository: oomol-lab/open-connector
Length of output: 50381
Validate the bulk-result URL before fetching it. s.url(...) only checks URI syntax, so input.url can still target arbitrary hosts. Add assertPublicHttpUrl and switch this request to providerFetch instead of context.fetcher to keep the download path off private or metadata endpoints.
🤖 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/providers/shopify_admin/runtime.ts` around lines 1074 - 1092, In
downloadShopifyAdminBulkResult, validate the parsed input.url with
assertPublicHttpUrl before issuing the request, then replace context.fetcher
with providerFetch while preserving the existing headers, timeout signal, and
response handling.
Source: Coding guidelines
This was referenced Jul 30, 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.
概要
同步第二批 5 个 provider：
边界
transitFiles验证
npm run generate:catalognpm run fix-checknpm run buildgit diff --check origin/main...HEAD在包含两批提交的集成分支上执行
node ../connect-port.ts --force，两边 missing provider/action 均为(none)。未运行全量测试。