Demote non-actionable AI agent SDK error reports#13931
Demote non-actionable AI agent SDK error reports#13931warp-dev-github-integration[bot] wants to merge 2 commits into
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-dev-github-integration[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
| .context("Failed to update agent task state to InProgress") | ||
| { | ||
| report_error!(e); | ||
| log::error!("Failed to update agent task state to InProgress: {e:#}"); |
There was a problem hiding this comment.
The logging skill says log::error! is appropriate for a genuine failure caused by an external system when there is no code defect to fix. Updating the task state is an external API operation; the driver continues after this handled failure, so it should not create a Sentry issue.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
|
|
||
| if tx.send(result).is_err() { | ||
| report_error!("Caller did not wait for agent driver to finish"); | ||
| log::warn!("Caller did not wait for agent driver to finish"); |
There was a problem hiding this comment.
The skill assigns recoverable/handled conditions and fallbacks to log::warn!. A dropped receiver means the caller exited before consuming the result, while teardown still proceeds; this is expected lifecycle degradation rather than an engineering issue.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| Ok(()) => true, | ||
| Err(err) => { | ||
| report_error!(err); | ||
| log::error!("Failed to save harness conversation (final): {err:#}"); |
There was a problem hiding this comment.
Per the skill, external failures that are real but not our code's fault belong at log::error!, not report_error!. Final conversation persistence can fail because of the external storage/network path and the harness already degrades to a non-resumable cleanup disposition, so this should remain breadcrumb-only.
There was a problem hiding this comment.
This path is restored to report_error! in rework commit d3b5e34: a final save failure selects DropResumptionState, so the resulting loss of resumability is actionable rather than a breadcrumb-only external failure.
| .context("Failed to clean up harness runtime state") | ||
| { | ||
| report_error!(err); | ||
| log::warn!("Failed to clean up harness runtime state: {err:#}"); |
There was a problem hiding this comment.
The skill places best-effort cleanup and recoverable handled paths at log::warn!. Harness runtime-state cleanup is attempted after the run and failure does not block the completed result, so this is not Sentry-actionable.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| for provider in providers { | ||
| if let Err(err) = provider.cleanup().await { | ||
| report_error!(anyhow!(err).context("Unable to clean up cloud provider")); | ||
| log::warn!("Unable to clean up cloud provider: {err:#}"); |
There was a problem hiding this comment.
The skill reserves report_error! for failures worth engineering attention and uses log::warn! for handled fallback/cleanup paths. Cloud-provider cleanup only removes temporary credentials after the run; a cleanup failure is an external best-effort teardown issue and does not affect the run result.
There was a problem hiding this comment.
This path is restored to report_error! in rework commit d3b5e34: AWS/GCP provider cleanup removes credential material, so failures need structured visibility even though teardown continues.
| } | ||
| Err(e) => { | ||
| report_error!(e); | ||
| log::error!("Failed to create task: {e:#}"); |
There was a problem hiding this comment.
The skill says external-system failures belong at log::error! when the app can handle the failure without a code fix. Task creation is a server/API dependency and this path deliberately continues without a task ID, so it should not create a Sentry issue.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| Ok(id) => Some(id), | ||
| Err(e) => { | ||
| report_error!(e); | ||
| log::warn!("Failed to parse task ID: {e:#}"); |
There was a problem hiding this comment.
The skill calls expected handled input/resource failures log::warn!. A malformed task ID is invalid external/user-provided input and this path falls back to an unscoped run, so it is not an engineering issue for Sentry.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| "Invalid environment ID", | ||
| extra: { "environment_id" => %environment_id } | ||
| ); | ||
| log::warn!("Invalid environment ID"); |
There was a problem hiding this comment.
The skill recommends plain log::* for expected validation misses and user-provided invalid values. An invalid environment ID is handled by listing valid environments and returning a user-facing not-found error, so log::warn! is the appropriate non-Sentry level.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| "Environment not found with ID", | ||
| extra: { "environment_id" => %environment_id } | ||
| ); | ||
| log::warn!("Environment not found with ID"); |
There was a problem hiding this comment.
The skill says gracefully handled missing resources should not use report_error!; use log::warn! for the degraded/user-correctable path. An environment absent from the local collection is expected invalid configuration, and valid IDs are listed before returning the typed error.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
| if let Err(err) = std::fs::write(&output_path, &output) { | ||
| report_error!(anyhow!(err) | ||
| .context(format!("Error writing GCP token to {output_path}"))); | ||
| log::error!("Error writing GCP token to {output_path}: {err:#}"); |
There was a problem hiding this comment.
The skill says external filesystem failures that do not indicate a Warp bug belong at log::error!, while handled fallback paths should not page Sentry. The token is printed and the command terminates successfully even if optional caching fails, so this remains a local breadcrumb.
There was a problem hiding this comment.
Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR demotes several report_error! calls in the AI agent SDK driver and third-party harness paths to breadcrumb-only log::* calls. Most changes are consistent with reducing non-actionable Sentry noise, and no approved spec context was available for this PR.
Concerns
- Final harness conversation save failures can still complete the run successfully while dropping resumption state, so they should remain structured reports.
- Cloud provider cleanup failures can leave temporary credential material on disk; demoting them to warning breadcrumbs hides a security-relevant cleanup failure.
- Snapshot upload timeouts break the end-of-run handoff artifact path and should keep structured reporting with diagnostic context.
Security
- Cloud provider cleanup errors cover AWS/GCP credential file cleanup, so suppressing their structured report can hide leaked credential artifacts.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
The demotions are generally consistent with the logging policy for recoverable external failures, and GitHub CI is green. Three demotions still hide failures that materially affect resumption, credential cleanup, or cloud handoff reliability.
Concerns
- A final conversation-save failure causes the code to select
DropResumptionState; this can silently make a completed run non-resumable, so it remains an actionable incident. - Cloud-provider cleanup owns AWS OIDC and GCP credential files; a failed cleanup can leave credential material on disk and requires structured reporting rather than a warning breadcrumb.
- Snapshot upload is the end-of-run handoff artifact path; a timeout can make follow-up agents miss workspace context and should retain structured reporting with timeout/task context.
Verification
GitHub CI is green (21 successful checks). Local ./script/presubmit completed formatting and clippy but could not finish the build because the review environment exhausted disk space while linking WarpUI examples.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Keep final harness persistence, cloud credential cleanup, and snapshot handoff timeout failures in structured error reporting so reliability and security incidents remain visible. Co-Authored-By: Oz <oz-agent@warp.dev>
|
Rework cycle 1 addressed all three requested findings in commit d3b5e34: final harness-save failures again use structured |
|
Validation update: the exact presubmit Warp clippy gate |
There was a problem hiding this comment.
Overview
Re-reviewed PR #13931 at d3b5e34 after rework cycle 1. The demotions are limited to handled external failures, expected fallbacks, and user/configuration errors, while the three actionable paths identified previously retain structured reporting: final harness conversation-save failures, AWS/GCP credential cleanup failures, and snapshot handoff upload timeouts.
Verification
All required GitHub checks are successful (20 successful, 6 skipped). The PR records passing formatting, clippy, and focused AgentDriver tests; the local full presubmit reached the test build but was interrupted by the review environment, so CI is the authoritative full-gate result.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
1 participant
Description
Demote
report_error!calls in the AI agent SDK driver that represent handled external failures, best-effort cleanup, invalid user/configuration input, or expected fallback paths. These conditions now remain visible as locallog::error!/log::warn!breadcrumbs without creating Sentry issues; invariant violations and critical handoff failures remain structured reports.Originating thread: https://warp-dev.slack.com/archives/C0BDQDW8V5E/p1784336780055059
Linked Issue
Linear: https://linear.app/warpdotdev/issue/APP-4874/demote-non-actionable-report-error-calls-in-ai-agent-sdk-driver-to
Testing
./script/formatpassed.cargo clippypassed with no warnings.CARGO_BUILD_JOBS=1 cargo test -p warp --lib ai::agent_sdk::driver::tests -- --nocapturepassed: 45 tests.CARGO_BUILD_JOBS=1 NEXTEST_TEST_THREADS=1 ./script/presubmitcompleted formatting, clippy, clang-format, WGSL checks, and most tests, but environment-dependent integration tests failed (SSH/password-prompt, XDG/DBus, and missing branch metadata); no failures implicated these logging changes.Agent Mode
Conversation: https://staging.warp.dev/conversation/e3b04a67-a444-4f36-bcbf-745cec9e9508
Run: https://oz.staging.warp.dev/runs/019f72c9-6232-7222-9129-ce27acc069cf
This PR was generated with Oz.