Uh oh!
There was an error while loading. Please reload this page.
feat(errors): add shared user cancellation error - #1986
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## refactor #1986 +/- ##
============================================
- Coverage 97.14% 97.13% -0.01%
============================================
Files 382 382 Lines 22884 22856 -28 ============================================
- Hits 22231 22202 -29 - Misses 653 654 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Hweinstock
left a comment
There was a problem hiding this comment.
i like the approach! few comments, but I think some could be follow-ups I could help pick up.
| export class RuntimeInvokeResponseError extends AgentCoreCLIError { | ||
| readonly reported = true; | ||
| export class RuntimeInvokeResponseError extends SilentCLIError { |
There was a problem hiding this comment.
why are runtime invoke responses silent? I thought this was the error we get when the stream parsing fails.
There was a problem hiding this comment.
Yeah, this is the error we get when response streaming fails. By the time it reaches the root, writeStreamingResponse has already written the sanitized incomplete response summary to stderr. Making this error silent just prevents a second generic Error: response stream failed line. It still goes through structured logging and telemetry.
| if ((error as Error)?.name === "AbortError") return ExitCode.INTERRUPTED; | ||
| if (caught instanceof AgentCoreCLIError) return caught.exitCode; | ||
| return ExitCode.FAILURE; | ||
| const error = AgentCoreCLIError.fromError(caught); |
There was a problem hiding this comment.
nice, really like how simple this is now!
| error_name: error.name, | ||
| error_source: error.source, | ||
| }); | ||
| if (error.exitCode !== 0) { |
There was a problem hiding this comment.
i wonder if it makes sense to expand the exit_reason attribute to accept a cancelled value. That way we still get telemetry for these cancellations.
could be a follow-up since we'll need to adjust the backend schema to accommodate.
There was a problem hiding this comment.
Yeah, I think cancelled would be clearer. Right now cancellation still emits telemetry as failure with error_source: user. I kept the new exit reason out of this PR since it also requires a backend schema change, so I think that should be a follow-up.
There was a problem hiding this comment.
agreed. Once we start getting data we can decide if separating into a separate reason makes sense.
There was a problem hiding this comment.
do we need the same controller.signal.throwIfAborted(); check here?
Also wondering if it makes sense to build an abstraction for this since it seems there's already a few consumers?
Something like:
async function withCancellation<T>(fn: (signal: AbortSignal) => Promise<T>): Promise<T> {
const controller = new AbortController();
const interrupt = () => controller.abort(new UserCancellationError());
process.once("SIGINT", interrupt);
try {
return await fn(controller.signal);
} catch (error) {
controller.signal.throwIfAborted();
throw error;
} finally {
controller.abort();
process.off("SIGINT", interrupt);
}
}
There was a problem hiding this comment.
Agreed. After rebasing, Gateway invoke and dataset update introduced two more consumers, so the abstraction makes sense now. I added withUserCancellation under src/runnable and moved Runtime invoke, Gateway invoke, dataset get, and dataset update onto it. It owns the SIGINT listener, cleanup, and throwIfAborted() normalization, while TUI-local cancellation stays separate. I'll do the same with dev when its merged.
5c7c41f to
2e3832fCompare
- move renderJsonTemplate out of shared src/io into core/eval/invokeDataset - invokeRuntime: raw TypeError/Error -> InputValidationError/RuntimeInvokeResponseError - wire error causes in template + dataset JSON parse - invokeDataset: enrich per-example invoke failure instead of log+rethrow - simulate: bubble Ctrl-C cancellation (telemetry) instead of quiet return; clarify --description help; TODO(#1986) shared abort helper - drop type-guaranteed 'no leak' test; keep AbortSignal wiring in composition test - trim stale/redundant comments (runtime.tsx, invokeRuntime DTO note, TurnResult)
2e3832f to
f17de43CompareUh oh!
There was an error while loading. Please reload this page.
- move renderJsonTemplate out of shared src/io into core/eval/invokeDataset - invokeRuntime: raw TypeError/Error -> InputValidationError/RuntimeInvokeResponseError - wire error causes in template + dataset JSON parse - invokeDataset: enrich per-example invoke failure instead of log+rethrow - simulate: bubble Ctrl-C cancellation (telemetry) instead of quiet return; clarify --description help; TODO(#1986) shared abort helper - drop type-guaranteed 'no leak' test; keep AbortSignal wiring in composition test - trim stale/redundant comments (runtime.tsx, invokeRuntime DTO note, TurnResult)
…#2032) * feat(eval): batch-evaluation simulate — invokeDataset + self-running example classes * refactor(eval): address Harrison's review feedback - move renderJsonTemplate out of shared src/io into core/eval/invokeDataset - invokeRuntime: raw TypeError/Error -> InputValidationError/RuntimeInvokeResponseError - wire error causes in template + dataset JSON parse - invokeDataset: enrich per-example invoke failure instead of log+rethrow - simulate: bubble Ctrl-C cancellation (telemetry) instead of quiet return; clarify --description help; TODO(#1986) shared abort helper - drop type-guaranteed 'no leak' test; keep AbortSignal wiring in composition test - trim stale/redundant comments (runtime.tsx, invokeRuntime DTO note, TurnResult) * fix(eval): validate assertions/expected_trajectory + require {input} in payload-template Bug bash (real exploratory-account run) surfaced two defects in the shared invokeDataset path: 1. PredefinedExample blind-cast assertions/expected_trajectory as string[] with no load-time validation (unlike turns). A non-array value passed load, burned a live paid invoke, then threw a raw '.map is not a function' mislabeled 'failed to invoke'. Now validated in the constructor -> clean InputValidationError before any invoke. 2. A --payload-template with no {input} placeholder was silently accepted, wasting a full ~3-min replay on a constant payload. Now rejected up front in invokeDataset. --------- Co-authored-by: jariy17 <tjariy+jariy17@users.noreply.github.com>
Problem
Commander exits and user cancellations bypass the shared CLI error model. Runtime and Gateway invoke define resource-specific interruption errors, while Project dev defines a separate command interruption type for the same user cancellation outcome.
Solution
CommanderErrorinAgentCoreCLIError.fromError, preserving help as exit0and mapping parse failures to usage exit2SilentCLIErrorcategory so only intentionally silent errors skip generic root stderr outputUserCancellationErrorwith exit code130withUserCancellationfor Runtime invoke, Gateway invoke, dataset get, and dataset updateAbortSignal.reason, including Project devShutting down…message, repeated-signal behavior, and listener cleanupAbortErrorhandling unchangedVerification
146 pass, 0 fail)1544 pass, 0 fail)bun run typecheckbun run lint:checkbun run buildgit diff --check1544 pass, 0 fail1544 pass, 0 fail1544 pass, 0 fail0, help on stdout, no stderr or error log2, one Commander error line, classified as a user error