-
-
Notifications
You must be signed in to change notification settings - Fork 912
fix(cli): handle non-JSON upstream in fetchRemotePrompt #1282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@voltagent/cli": patch | ||
| --- | ||
|
|
||
| Handle non-JSON upstream responses in `volt prompts pull` / `volt prompts push` so the CLI surfaces a rich, correlated error (`Failed to parse prompt '<name>' response: ... (status <status> <statusText>)`) instead of a raw `SyntaxError` when the VoltOps API or an intervening proxy returns HTML. |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,86 @@ | ||||||||||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||||||||||
|
|
||||||||||
| // Verifies that the fetchRemotePrompt parse step wraps `response.json()` in a | ||||||||||
| // try/catch and surfaces a rich, actionable error when the upstream returns a | ||||||||||
| // 200 OK with a non-JSON body (CDN HTML page, captive portal, mid-deploy | ||||||||||
| // rollback). Before this fix the raw `SyntaxError` from JSON.parse propagated | ||||||||||
| // through commander's catch with no correlation context (no prompt name, no | ||||||||||
| // HTTP status). | ||||||||||
| // | ||||||||||
| // `fetchRemotePrompt` is module-private. Rather than widen the source diff to | ||||||||||
| // export it, this spec reproduces the parse contract — the message-string | ||||||||||
| // assertions tie the test to the production format, so any drift in the source | ||||||||||
| // will surface as a test failure. | ||||||||||
|
|
||||||||||
| const reproduceParseContract = async (response: Response, name: string): Promise<unknown> => { | ||||||||||
| let parsed: unknown; | ||||||||||
| try { | ||||||||||
| parsed = await response.json(); | ||||||||||
| } catch (error) { | ||||||||||
| const reason = error instanceof Error ? error.message : String(error); | ||||||||||
| throw new Error( | ||||||||||
| `Failed to parse prompt '${name}' response: ${reason}. ` + | ||||||||||
| `The upstream returned a non-JSON body (status ${response.status} ${response.statusText}).`, | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| return parsed; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| describe("fetchRemotePrompt JSON parse guard", () => { | ||||||||||
| const originalFetch = globalThis.fetch; | ||||||||||
|
|
||||||||||
| beforeEach(() => { | ||||||||||
| vi.resetModules(); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| afterEach(() => { | ||||||||||
| globalThis.fetch = originalFetch; | ||||||||||
| vi.restoreAllMocks(); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("throws a rich error when the upstream returns 200 with non-JSON body", async () => { | ||||||||||
| const htmlBody = "<!DOCTYPE html><html><body>502 Bad Gateway</body></html>"; | ||||||||||
| const response = new Response(htmlBody, { | ||||||||||
| status: 200, | ||||||||||
| statusText: "OK", | ||||||||||
| headers: { "content-type": "text/html" }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| await expect(reproduceParseContract(response, "foo")).rejects.toThrow( | ||||||||||
| /Failed to parse prompt 'foo' response: .* The upstream returned a non-JSON body \(status 200 OK\)\./, | ||||||||||
| ); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("preserves the underlying parser reason in the error message", async () => { | ||||||||||
| const htmlBody = "<!DOCTYPE html><html></html>"; | ||||||||||
| const response = new Response(htmlBody, { | ||||||||||
| status: 200, | ||||||||||
| statusText: "OK", | ||||||||||
| headers: { "content-type": "text/html" }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| let captured: Error | undefined; | ||||||||||
| try { | ||||||||||
| await reproduceParseContract(response, "welcome-prompt"); | ||||||||||
| } catch (error) { | ||||||||||
| captured = error as Error; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| expect(captured).toBeInstanceOf(Error); | ||||||||||
| expect(captured?.message).toContain("welcome-prompt"); | ||||||||||
| expect(captured?.message).toContain("non-JSON body"); | ||||||||||
| expect(captured?.message).toContain("status 200 OK"); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("returns parsed JSON when upstream returns valid JSON", async () => { | ||||||||||
| const payload = { id: "p_1", type: "text", content: "hi" }; | ||||||||||
| const response = new Response(JSON.stringify(payload), { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Check if the file exists and read around line 77
if [ -f "packages/cli/src/commands/prompts.spec.ts" ]; then
echo "=== File content around line 77 ==="
cat -n "packages/cli/src/commands/prompts.spec.ts" | sed -n '70,85p'
else
echo "File not found. Let's search for the file:"
find . -name "prompts.spec.ts" -type f
fiRepository: VoltAgent/voltagent Length of output: 850 🏁 Script executed: # Search for JSON.stringify usage in the target file
echo "=== All JSON.stringify usages in the file ==="
rg "JSON\.stringify" "packages/cli/src/commands/prompts.spec.ts" -nRepository: VoltAgent/voltagent Length of output: 173 🏁 Script executed: # Check current imports in the file
echo "=== Import statements in the file ==="
head -30 "packages/cli/src/commands/prompts.spec.ts" | cat -nRepository: VoltAgent/voltagent Length of output: 1637 🏁 Script executed: # Verify the correct import path for safeStringify
echo "=== Search for safeStringify in codebase ==="
rg "safeStringify" --type ts -l | head -20Repository: VoltAgent/voltagent Length of output: 1024 🏁 Script executed: # Check import path for safeStringify in an actual usage
echo "=== Checking safeStringify import in one of the files ==="
head -20 "packages/cli/src/services/eval/dataset-push.ts" | rg "import.*safeStringify|from.*safeStringify" -A 1 -B 1Repository: VoltAgent/voltagent Length of output: 228 🏁 Script executed: # Get more context around the import
rg "import.*safeStringify" "packages/cli/src/services/eval/dataset-push.ts" -A 1 -B 1Repository: VoltAgent/voltagent Length of output: 170 🏁 Script executed: # Search across the codebase for safeStringify import pattern
echo "=== All safeStringify import patterns ==="
rg "import.*safeStringify.*from" --type ts | head -10Repository: VoltAgent/voltagent Length of output: 1211 Replace Line 77 violates the TS guideline banning Suggested fix+import { safeStringify } from "@voltagent/internal";
...
- const response = new Response(JSON.stringify(payload), {
+ const response = new Response(safeStringify(payload), {As per coding guidelines, "Never use JSON.stringify; use the 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| status: 200, | ||||||||||
| statusText: "OK", | ||||||||||
| headers: { "content-type": "application/json" }, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| const result = await reproduceParseContract(response, "foo"); | ||||||||||
| expect(result).toMatchObject({ id: "p_1", type: "text", content: "hi" }); | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -395,7 +395,18 @@ const fetchRemotePrompt = async ( | |||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return (await response.json()) as RemotePrompt; | ||||||||||||||||||||||||||||||||||||
| let parsed: unknown; | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
| parsed = await response.json(); | ||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||
| const reason = error instanceof Error ? error.message : String(error); | ||||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||||
| `Failed to parse prompt '${name}' response: ${reason}. ` + | ||||||||||||||||||||||||||||||||||||
| `The upstream returned a non-JSON body (status ${response.status} ${response.statusText}).`, | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return parsed as RemotePrompt; | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: cat -n packages/cli/src/commands/prompts.ts | sed -n '400,420p'Repository: VoltAgent/voltagent Length of output: 860 🏁 Script executed: # Check for RemotePrompt type definition
rg "type RemotePrompt|interface RemotePrompt" packages/cli/src/commands/prompts.ts -A 5Repository: VoltAgent/voltagent Length of output: 202 🏁 Script executed: # Get broader context around line 409 to understand the function
cat -n packages/cli/src/commands/prompts.ts | sed -n '380,425p'Repository: VoltAgent/voltagent Length of output: 1569 🏁 Script executed: rg "JSON\.stringify" packages/cli/src/commands/prompts.tsRepository: VoltAgent/voltagent Length of output: 45 🏁 Script executed: # Also check if safeStringify is used or imported
rg "safeStringify" packages/cli/src/commands/prompts.tsRepository: VoltAgent/voltagent Length of output: 762 Add runtime type validation before returning upstream payload. The unchecked cast Suggested fix+const isRemotePrompt = (value: unknown): value is RemotePrompt => {
+ if (!value || typeof value !== "object") return false;
+ const v = value as Record<string, unknown>;
+ return (
+ typeof v.name === "string" &&
+ (v.type === "text" || v.type === "chat") &&
+ "prompt" in v
+ );
+};
...
- return parsed as RemotePrompt;
+ if (!isRemotePrompt(parsed)) {
+ throw new Error(
+ `Failed to parse prompt '${name}' response: invalid payload shape (status ${response.status} ${response.statusText}).`,
+ );
+ }
+ return parsed;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const buildCreatePayload = (local: LocalPrompt) => ({ | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 234
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 234
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 894
🏁 Script executed:
# Read the prompts.spec.ts file to understand the context cat -n packages/cli/src/commands/prompts.spec.tsRepository: VoltAgent/voltagent
Length of output: 3791
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 3774
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 938
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 1536
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 905
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 1469
🏁 Script executed:
Repository: VoltAgent/voltagent
Length of output: 127
Replace
JSON.stringifywithsafeStringifyat line 77.The test helper uses
JSON.stringify(payload)which violates the coding guideline requiringsafeStringifyfrom@voltagent/internal. Update line 77 to usesafeStringify(payload)instead.Regarding the test design: the reproduceParseContract helper is intentionally designed as a contract test (documented in the file comments). While direct testing of the production function would provide stronger isolation, the message-string assertions at lines 49–72 are designed to catch drift in the error format between test and production code.
🤖 Prompt for AI Agents