[fix]: expose error cause in UnderstudyCommandException#1705
Conversation
🦋 Changeset detectedLatest commit: e2047d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile SummaryEnhanced
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Understudy Action Executed] --> B{Action Succeeds?}
B -->|Yes| C[Action Complete]
B -->|No| D[Catch Error]
D --> E{Error is Error instance?}
E -->|Yes| F[Extract e.message and e.stack]
E -->|No| G[Convert to String]
F --> H[Create UnderstudyCommandException with message and cause]
G --> H
H --> I[Log error details via v3Logger]
I --> J[Throw UnderstudyCommandException]
J --> K[Error propagates with original cause preserved]
Last reviewed commit: 8e78a14 |
Additional Comments (2)
|
There was a problem hiding this comment.
cubic analysis
1 issue found across 4 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk; the only flagged item is a medium-severity error-handling consistency concern.
- In
packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts,e.messageis accessed without ane instanceof Errorguard, which could throw if a non-Error is caught and affect logging/throw behavior. - Pay close attention to
packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts- add a type guard before usinge.messagefor consistency and safety.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts">
<violation number="1" location="packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts:172">
P2: Unsafe `e.message` access without type guard. Same issue as `selectOption` — the logging and throw should guard with `e instanceof Error` for consistency with the rest of the file.</violation>
</file>
Linked issue analysis
Linked issue: STG-1310: include cause in understudycommandexception
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | UnderstudyCommandException includes a cause parameter to preserve original error | Added constructor(message, cause?) in sdkErrors.ts |
| ✅ | Callsites updated to pass original error as the cause when throwing UnderstudyCommandException | Updated throws in actHandlerUtils.ts to pass e |
| ✅ | UnderstudyCommandException moved into sdkErrors.ts for public use (no inline class left in handlers) | Removed inline class and now import from sdkErrors.ts |
| ✅ | Unit tests added covering name, message, cause, and stack behavior | Added tests asserting cause and stacks in new test file |
| ✅ | Original error stack is preserved/accessed via the cause for debugging | Test asserts original stack accessible via cause |
Architecture diagram
sequenceDiagram
participant User as SDK Consumer
participant ActHandler as actHandlerUtils
participant Page as Understudy / Page
participant Error as sdkErrors (UnderstudyCommandException)
User->>>ActHandler: performAction(method, args)
rect rgb(20, 20, 30)
Note over ActHandler, Page: Action Execution (e.g., click, fill, hover)
ActHandler->>>Page: execute browser command
alt Success Path
Page-->>ActHandler: return result
ActHandler-->>User: success
else Failure Path
Page-->>ActHandler: throw originalError (e.g. Timeout)
ActHandler->>Error: NEW: instantiate(message, originalError)
Note over Error: CHANGED: Extends StagehandError<br/>Stores originalError in .cause
Error-->>ActHandler: exception object
ActHandler-->>User: CHANGED: throw UnderstudyCommandException (with cause)
end
end
Note over User: User can now access<br/>err.cause for full stack trace
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
1 issue found across 4 files
Confidence score: 4/5
- This PR looks safe to merge overall, with one medium-severity concern around error handling behavior.
- In
packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts, the outercatchinperformUnderstudyMethodcan double-wrapUnderstudyCommandException, which may complicate error inspection or logging. - Severity is moderate (5/10) and localized to exception wrapping, so impact should be limited if callers don’t depend on exact error chains.
- Pay close attention to
packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts- avoid double-wrappingUnderstudyCommandExceptionto keep error causes clean.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts">
<violation number="1" location="packages/core/lib/v3/handlers/handlerUtils/actHandlerUtils.ts:126">
P2: The outer `catch` in `performUnderstudyMethod` re-wraps errors that inner handlers already threw as `UnderstudyCommandException` with a cause, creating a double-wrapped chain (`UnderstudyCommandException → UnderstudyCommandException → original`). Since the goal of this PR is to preserve the original error cause, consider re-throwing `UnderstudyCommandException` instances directly to avoid an unnecessary extra layer:
```ts
if (e instanceof UnderstudyCommandException) throw e;
throw new UnderstudyCommandException(msg, e);
```</violation>
</file>
Linked issue analysis
Linked issue: STG-1310: include cause in understudycommandexception
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | UnderstudyCommandException accepts an optional cause parameter and stores it | Added constructor with cause in sdkErrors.ts |
| ✅ | Understudy handlers pass the original error as the cause when throwing UnderstudyCommandException | Replaced throws with new UnderstudyCommandException(msg, e) |
| ✅ | Original error stack is preserved / accessible via the exception's cause | Constructor forwards cause to StagehandError and tests assert cause.stack |
| ✅ | Non-Error cause values are supported (type-safe handling of unknown causes) | Constructor uses cause?: unknown and tests cover string cause |
| ✅ | Handlers compute message/stack safely for non-Error values and log them | Added msg/stack guards and logger uses those values |
| ✅ | UnderstudyCommandException moved into sdkErrors.ts (public exposure) | Class removed from handlerUtils and added to sdkErrors.ts |
| ✅ | Unit tests added covering name, message, cause, and stack behavior | Added understudy-command-exception.test.ts with multiple assertions |
Architecture diagram
sequenceDiagram
participant Client as User / SDK Client
participant Handler as ActHandler (actHandlerUtils)
participant Locator as Understudy Locator
participant Logger as v3Logger
participant Exception as UnderstudyCommandException
participant SDKError as StagehandError (Base)
Client->>Handler: performAction(ctx, args)
rect rgb(30, 41, 59)
Note over Handler,Locator: Try block
Handler->>Locator: executeCommand(args)
Locator-->>Handler: throw originalError (Error | unknown)
end
rect rgb(70, 20, 20)
Note over Handler: Catch block
Handler->>Handler: Safely extract message & stack
Handler->>Logger: log(message, stack, context)
Handler->>Exception: new UnderstudyCommandException(message, originalError)
Exception->>SDKError: super(message, originalError)
Note over SDKError: Stores originalError as .cause
Handler-->>Client: throw UnderstudyCommandException
end
Note over Client: Client accesses .cause to see<br/>original stack trace / details
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
why
what changed
UnderstudyCommandExceptionintosdkErrors.tscauseparamactHandlerUtils.tsto include causetest plan
understudy-command-exception.test.tsSummary by cubic
Expose the original error as the cause in UnderstudyCommandException to preserve stack traces and context during Understudy actions, with type-safe handling for non-Error values. Addresses Linear STG-1310.
Written for commit e2047d6. Summary will update on new commits. Review in cubic