Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Co-authored-by: Cursor <cursoragent@cursor.com>
amikofalvy
force-pushed
the
feat/revive-single-phase-generation
branch
from
b4d9df9 to
a7eb78c
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
3 Key Findings | Risk: Medium
This PR correctly identifies and fixes a real bug where AI SDK prototype getters are lost when spreading response objects. The solution approach is sound, but there are several areas that warrant attention before merging.
🟠⚠️ Major (2) 🟠⚠️
🟠 1) Agent.ts:141-143 Passthrough case bypasses validation entirely
Issue: The early return if (!response.steps) passes through the response unmodified with an unsafe type assertion (as unknown as ResolvedGenerationResponse). This creates two problems:
-
Type safety violation: Returns
Record<string, unknown>cast toResolvedGenerationResponsewithout validating required fields (text,finishReason,steps). Downstream code at lines 2888-2894 expects these fields to exist. -
Inconsistent behavior: If the AI SDK ever returns a response where
stepsis falsy but has prototype getters ontextorfinishReason, those getters will still be lost when spreading downstream.
Why: Downstream code accessing response.text, response.steps, or response.finishReason will receive undefined instead of the expected values, potentially causing the same empty response bug this PR aims to fix. Example from line 2890: response.steps[response.steps.length - 1].text would crash if steps is undefined.
Fix: Add validation for the passthrough case or throw an error for invalid responses:
if (!response.steps) {
// Passthrough: validate this looks like a pre-resolved response
if (!response.text && !response.finishReason) {
throw new Error(
'Invalid response shape: missing both steps and required fields'
);
}
return response as unknown as ResolvedGenerationResponse;
}Or, more defensively, always try to resolve the getters even when steps is falsy.
Refs: Agent.ts:141-143 · Usage at line 2890
🟠 2) Agent.ts:145-152 Promise.all rejection has no contextual error handling
Issue: The Promise.all at lines 145-152 resolves four getters concurrently. If any getter returns a rejected Promise (e.g., due to stream errors, AI SDK internal failures, or unexpected response shapes), the entire Promise.all rejects with an unhelpful error that propagates to the generic catch block at line 2930.
Why: When debugging production issues, engineers won't know whether the AI SDK call failed or whether property extraction failed. The error will be logged as "Generation error in Agent" without indicating this was a response resolution issue. This makes diagnosing issues with new AI SDK versions or malformed responses extremely difficult.
Fix: Wrap in try-catch with contextual error message:
try {
const [steps, text, finishReason, output] = await Promise.all([...]);
return { ...response, steps, text, finishReason, output } as ResolvedGenerationResponse;
} catch (error) {
logger.error(
{
error: error instanceof Error ? error.message : String(error),
responseKeys: Object.keys(response),
},
'Failed to resolve generation response properties - AI SDK response may be malformed'
);
throw new Error(
`Failed to resolve generation response: ${error instanceof Error ? error.message : String(error)}`
);
}Refs: Agent.ts:145-152 · Catch block at line 2930
🟡 Minor (1) 🟡
🟡 1) Test coverage Missing Promise rejection test scenarios
Issue: The test suite comprehensively covers happy-path scenarios (direct values, Promise-based values, empty strings, structured output) but does not test what happens when one of the Promise-based getters rejects.
Why: Since this function is called on every generation response, adding rejection tests would improve confidence in error propagation behavior and prevent regressions.
Fix: Add a test case:
it('should propagate rejection when Promise-based getter rejects', async () => {
class MockRejectedPromise {
get steps() { return Promise.reject(new Error('Stream closed')); }
get text() { return Promise.resolve(''); }
get finishReason() { return Promise.resolve('stop'); }
get output() { return Promise.resolve(undefined); }
}
const result = new MockRejectedPromise();
await expect(resolveGenerationResponse(result as any))
.rejects.toThrow('Stream closed');
});Refs: resolveGenerationResponse.test.ts
💭 Consider (3) 💭
💭 1) Agent.ts:121 output?: any undermines type safety
Issue + Why: Per AGENTS.md, avoid any where possible. The output field contains structured data (dataComponents) that is accessed with specific shape expectations (e.g., line 2865). Consider a more precise type like output?: { dataComponents?: Array<any> }.
💭 2) generateTaskHandler.ts:507-512 Missing type guard for Part mapping
Issue + Why: The map transforms parts with any type. No validation ensures part conforms to the Part discriminated union. If parts contain { kind: 'data' } without a data field, it violates the DataPart interface contract. Consider adding validation: if (!part.data) continue; or a type guard.
💭 3) Agent.ts:3594 Removed parser.finalize() call
Issue + Why: The diff shows await parser.finalize(); was removed from processStreamEvents. Verify this is intentional and that finalization happens elsewhere (appears to be in handleStreamGeneration). If so, this is fine.
📌 Inline Comments (1)
- 🟡
Agent.ts:2868Logging finishReason before resolution may log[object Promise]
💡 APPROVE WITH SUGGESTIONS
Summary: This PR solves a real and important bug with the AI SDK's prototype getters being lost when spreading. The core approach is correct. The suggestions above are about defensive programming—adding validation for edge cases and improving debuggability. The passthrough validation (Major #1) is the most important to address, as it could silently pass invalid responses through. The error handling improvement (Major #2) and test coverage (Minor #1) would make this more robust for production.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
Agent.ts:154-160 |
Type assertion spreads response without verifying all fields | The explicit field assignments (steps, text, finishReason, output) after the spread ensure the required fields are set. The type assertion is safe in this specific context. |
Agent.ts:3600 |
Unsafe (response as any).formattedContent cast |
This is outside the scope of this PR's changes - it's pre-existing code. The PR does not modify this pattern. |
Agent.ts:3632-3636 |
Stream error message could be undefined | Pre-existing code not modified by this PR. Out of scope. |
| Tests | Missing tests for other finishReason values | Nice to have but the core bug fix is well-tested. Other finishReason values would follow the same code path. |
| Tests | No concurrent/race condition tests | Promise.all handles timing correctly by design; explicit timing tests add limited value. |
Reviewer Stats
| Reviewer | Returned | Inline Comments | Main Findings | Pending Recs | Discarded |
|---|---|---|---|---|---|
pr-review-standards |
3 | 0 | 2 | 0 | 1 |
pr-review-tests |
6 | 0 | 1 | 0 | 5 |
pr-review-types |
5 | 0 | 0 | 0 | 5 |
pr-review-errors |
4 | 0 | 1 | 0 | 3 |
pr-review-sre |
4 | 1 | 0 | 0 | 3 |
| Total | 22 | 1 | 4 | 0 | 17 |
| dataComponentsCount: (rawResponse.output as any)?.dataComponents?.length || 0, | ||
| finishReason: rawResponse.finishReason, | ||
| }, | ||
| '✅ Generation completed' |
There was a problem hiding this comment.
MINOR Logging finishReason before resolution may log [object Promise]
For streaming responses, rawResponse.finishReason is a Promise-based getter. Logging it directly before resolveGenerationResponse completes will log [object Promise] rather than the actual finish reason value.
Consider moving this logging after resolveGenerationResponse(rawResponse) completes, using response.finishReason instead:
| '✅ Generation completed' | |
| }, | |
| '✅ Generation completed - awaiting property resolution' | |
| ); | |
| response = await resolveGenerationResponse(rawResponse); | |
| logger.info( | |
| { | |
| agentId: this.config.id, | |
| hasOutput: !!response.output, | |
| dataComponentsCount: (response.output as any)?.dataComponents?.length || 0, | |
| finishReason: response.finishReason, | |
| }, | |
| '✅ Response properties resolved' |
Note: This is a partial suggestion showing the approach - you'd need to adjust line ranges accordingly.
Wrap Promise.all in try-catch with descriptive error message so failures during response property resolution are distinguishable from generic generation errors. Also capture steps getter value once to avoid double-invocation of prototype getters (prevents unhandled rejections when a getter returns a rejected Promise). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
PR Review Summary
0 New Key Findings | Risk: Medium (unresolved prior findings)
Good progress since the last review! The new commit addressed two of the three key findings:
- ✅ Major #2 (Promise.all error handling) — Now wrapped in try-catch with contextual logging
- ✅ Minor #1 (Promise rejection test) — Test case added at lines 283-306
However, Major #1 (passthrough validation) remains unaddressed and is the most important item to fix before merging.
📌 Inline Comments (1)
- 🟠
generateTaskHandler.ts:507-512Part mapping missing validation for undefined text/data fields
🕐 Pending Recommendations (2)
From previous reviews:
- 🟠
Agent.ts:143-144Passthrough case bypasses validation entirely — The early returnif (!response.steps)performs an unsafe type assertion without validating required fields (text,finishReason). Downstream code expects these to exist and will crash if they're undefined. This is the most important item to address.
From review threads:
- 🟡
Agent.ts:2883Logging finishReason before resolution — For streaming responses,rawResponse.finishReasonis a Promise-based getter. Logging it beforeresolveGenerationResponsecompletes will output[object Promise]instead of the actual value.
💭 Consider (2) 💭
💭 1) Agent.ts:121 output?: any undermines type safety
Issue + Why: The output field uses any type, but downstream code at lines 2896-2898 accesses response.output?.dataComponents assuming a specific structure. Consider output?: { dataComponents?: Array<unknown> } for better type safety.
💭 2) Agent.ts:2891 Object mutation pattern after resolution
Issue + Why: The code mutates response.object = response.output after resolveGenerationResponse returns. This creates an implicit invariant not expressed in the type system. Consider either setting object inside resolveGenerationResponse or documenting the expected relationship between these fields.
💡 APPROVE WITH SUGGESTIONS
Summary: The core fix is correct and well-tested — resolving AI SDK prototype getters before spreading prevents silent data loss. The error handling improvements in this commit address prior feedback. The one remaining blocker is the passthrough validation (Major #1 from the previous review) — adding validation for responses without steps would prevent invalid data from flowing downstream. The inline comment about Part mapping validation is a minor safety improvement.
Discarded (12)
| Location | Issue | Reason Discarded |
|---|---|---|
Agent.ts:3647-3653 |
Stream error handling extracts message unsafely | Pre-existing code not modified by this PR. Out of scope. |
Agent.ts:3599-3607 |
Stream processing lacks error wrapping | Pre-existing code pattern. The generic catch at line 2945 handles these. |
| Test file | Missing object field preservation test |
The object field is assigned via mutation at line 2891, not resolved from getters. Current tests adequately cover the resolution logic. |
| Test file | Missing toolCalls array preservation test | toolCalls is a property of steps array elements, which are already tested for preservation. |
| Test file | Missing mixed Promise/direct value test | Promise.resolve() handles both cases by design — explicit test adds limited value. |
| Test file | Missing other finishReason value tests | All finishReason values follow identical code path. |
Agent.ts:157-163 |
Type assertion after spread | Explicit field assignments ensure required fields are set. Safe in context. |
Agent.ts:3656 |
Removed parser.finalize() call |
Finalization now happens in handleStreamGeneration (line 3609). Intentional refactor. |
Agent.ts:3523 |
formatFinalResponse return type |
Good improvement — ResolvedGenerationResponse return type is more precise. |
Agent.ts:3556 |
handleGenerationError return type never |
Good improvement — explicit never return type documents that function always throws. |
| Types | Generic type parameter for output | Would add complexity without clear benefit for current use case. |
| Types | Discriminated union for output type | Over-engineering for current needs. |
Reviewer Stats
| Reviewer | Returned | Inline Comments | Main Findings | Pending Recs | Discarded |
|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 0 | 0 |
pr-review-tests |
5 | 0 | 0 | 0 | 5 |
pr-review-types |
4 | 0 | 0 | 1 | 3 |
pr-review-errors |
4 | 0 | 0 | 1 | 3 |
pr-review-sre |
0 | 0 | 0 | 0 | 0 |
| Total | 14 | 1 | 0 | 2 | 11 |
| const parts: Part[] = (response.formattedContent?.parts || []).map((part: any): Part => { | ||
| if (part.kind === 'data') { | ||
| return { kind: 'data' as const, data: part.data }; | ||
| } | ||
| return { kind: 'text' as const, text: part.text }; | ||
| }); |
There was a problem hiding this comment.
MAJOR Missing validation for undefined text/data fields
When part.kind === 'data' but part.data is undefined, this creates an invalid DataPart that violates the type contract (DataPart requires data: { [key: string]: any }). Similarly, if part.kind === 'text' but part.text is undefined, the TextPart contract is violated.
This could cause downstream A2A consumers to crash or silently lose data.
| const parts: Part[] = (response.formattedContent?.parts || []).map((part: any): Part => { | |
| if (part.kind === 'data') { | |
| return { kind: 'data' as const, data: part.data }; | |
| } | |
| return { kind: 'text' as const, text: part.text }; | |
| }); | |
| const parts: Part[] = (response.formattedContent?.parts || []).reduce<Part[]>((acc, part: any) => { | |
| if (part.kind === 'data') { | |
| if (part.data !== undefined) { | |
| acc.push({ kind: 'data' as const, data: part.data }); | |
| } | |
| } else { | |
| acc.push({ kind: 'text' as const, text: part.text ?? '' }); | |
| } | |
| return acc; | |
| }, []); |
1 participant
No description provided.