Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions app/src/ai/agent_sdk/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,7 @@ impl AgentDriver {
.await
.context("Failed to update agent task state to InProgress")
{
report_error!(e);
log::error!("Failed to update agent task state to InProgress: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}
}
// Primary: WARP_SANDBOX_DEADLINE client-side timer.
Expand Down Expand Up @@ -1032,7 +1032,7 @@ impl AgentDriver {
Self::run_snapshot_upload(&foreground).await;

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");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}

Self::cleanup(foreground).await;
Expand Down Expand Up @@ -3010,7 +3010,7 @@ impl AgentDriver {
.await
.context("Failed to clean up harness runtime state")
{
report_error!(err);
log::warn!("Failed to clean up harness runtime state: {err:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}

// A runtime failure detected mid-run takes precedence over the
Expand Down Expand Up @@ -3940,9 +3940,7 @@ pub(super) async fn report_driver_error(
.update_agent_task(task_id, Some(state), None, None, Some(status_update))
.await
{
report_error!(
anyhow!(e).context(format!("Failed to report driver error for task {task_id}"))
);
log::error!("Failed to report driver error for task {task_id}: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill says log::error! is for genuine failures caused by an external system when there is no code fix to investigate. This is a best-effort server status update after an already-classified driver failure; a reporting-network failure should not generate a second Sentry issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}
}

Expand Down
3 changes: 1 addition & 2 deletions app/src/ai/agent_sdk/driver/harness/claude_code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ use parent_bridge::{MessageBridge, MessageBridgeCleanupDisposition};
use shell_words::quote as shell_quote;
#[cfg(test)]
use wake_driver::{ClaudeWakeRemoteContext, CLAUDE_WAKE_PROMPT_FILE_NAME};
use warp_errors::report_error;

#[cfg(test)]
use super::super::OZ_MESSAGE_LISTENER_STATE_ROOT_ENV;
Expand Down Expand Up @@ -461,7 +460,7 @@ impl HarnessRunner for ClaudeHarnessRunner {
.create_external_conversation(CLAUDE_CODE_FORMAT)
.await
.map_err(|e| {
report_error!(&e);
log::error!("Failed to create external conversation: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill distinguishes external failures from actionable bugs: use log::error! when the external system prevented the operation but there is no code defect to fix. Creating the external conversation is a server/network dependency and the typed setup error is returned to the caller, so this local breadcrumb avoids Sentry noise for ordinary service failures.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

AgentDriverError::ConfigBuildFailed(e)
})
})
Expand Down
3 changes: 1 addition & 2 deletions app/src/ai/agent_sdk/driver/harness/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use tempfile::NamedTempFile;
use uuid::Uuid;
use warp_cli::agent::Harness;
use warp_core::features::FeatureFlag;
use warp_errors::report_error;
use warp_managed_secrets::ManagedSecretValue;
use warpui::{ModelHandle, ModelSpawner, SingletonEntity};

Expand Down Expand Up @@ -326,7 +325,7 @@ impl HarnessRunner for CodexHarnessRunner {
.create_external_conversation(CODEX_CLI_FORMAT)
.await
.map_err(|e| {
report_error!(&e);
log::error!("Failed to create external conversation: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill says external-system failures that prevent an operation but do not indicate a Warp bug belong at log::error!. Codex conversation creation depends on the server/network and the setup error is propagated, so this should not create a separate Sentry issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

AgentDriverError::ConfigBuildFailed(e)
})
})
Expand Down
3 changes: 1 addition & 2 deletions app/src/ai/agent_sdk/driver/harness/gemini.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use serde::{Deserialize, Serialize};
use serde_json::{Map, Value};
use tempfile::NamedTempFile;
use warp_cli::agent::Harness;
use warp_errors::report_error;
use warp_managed_secrets::ManagedSecretValue;
use warpui::{ModelHandle, ModelSpawner};

Expand Down Expand Up @@ -167,7 +166,7 @@ impl HarnessRunner for GeminiHarnessRunner {
.create_external_conversation(GEMINI_CLI_FORMAT)
.await
.map_err(|e| {
report_error!(&e);
log::error!("Failed to create external conversation: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging skill reserves report_error! for failures worth engineering attention; ordinary external failures belong at log::error!. Gemini conversation creation is a server/network dependency and its setup error is returned, making a breadcrumb sufficient here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

AgentDriverError::ConfigBuildFailed(e)
})
})
Expand Down
28 changes: 4 additions & 24 deletions app/src/ai/agent_sdk/driver/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,7 @@ pub(super) async fn run_declarations_script(
};
let script_path = PathBuf::from(script_path);
if !script_path.exists() {
report_error!(
"Snapshot declarations script not found; skipping",
extra: { "script_path" => %script_path.display(), "task_id" => %task_id }
);
log::warn!("Snapshot declarations script not found; skipping");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill says expected or handled fallback paths should use log::warn!. A missing optional declarations script is explicitly skipped and the upload pipeline continues, so this missing resource is not an actionable Sentry event.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

return;
}

Expand All @@ -173,36 +170,19 @@ pub(super) async fn run_declarations_script(
let output = match command.output().with_timeout(script_timeout).await {
Ok(Ok(output)) => output,
Ok(Err(e)) => {
report_error!(
anyhow::Error::new(e).context("Failed to spawn snapshot declarations script"),
extra: { "script_path" => %script_path.display(), "task_id" => %task_id }
);
log::error!("Failed to spawn snapshot declarations script: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill uses log::error! for a genuine external/environment failure that prevented the intended operation but is not itself a code bug. Failing to spawn the configured script is an environment/process failure and this helper returns while the caller continues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

return;
}
Err(_) => {
report_error!(
"Snapshot declarations script timed out",
extra: {
"script_path" => %script_path.display(),
"timeout" => ?script_timeout,
"task_id" => %task_id
}
);
log::warn!("Snapshot declarations script timed out");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill places recoverable timeouts and fallback behavior at log::warn!. The declarations script timeout is absorbed and the pipeline uses whatever declarations already exist, so it should not create a Sentry issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

return;
}
};

if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
log::error!("Snapshot declarations script stderr: {stderr}");
report_error!(
"Snapshot declarations script exited with non-zero status",
extra: {
"script_path" => %script_path.display(),
"status" => %output.status,
"task_id" => %task_id
}
);
log::error!("Snapshot declarations script exited with non-zero status");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skill says log::error! is appropriate for a real external failure when there is no Warp defect to fix. A configured script's non-zero exit is an external command failure; this helper logs it and continues rather than treating it as a Sentry-actionable invariant violation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}
}

Expand Down
8 changes: 3 additions & 5 deletions app/src/ai/agent_sdk/federate.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use std::process;

use anyhow::{anyhow, Result};
use anyhow::Result;
use serde_json::json;
use warp_cli::agent::OutputFormat;
use warp_cli::federate::{FederateCommand, IssueGcpTokenArgs, IssueTokenArgs};
use warp_cli::GlobalOptions;
use warp_core::features::FeatureFlag;
use warp_errors::report_error;
use warp_managed_secrets::ManagedSecretManager;
use warpui::platform::TerminationMode;
use warpui::{AppContext, SingletonEntity as _};
Expand Down Expand Up @@ -103,11 +102,10 @@ fn issue_gcp_token(ctx: &mut AppContext, args: IssueGcpTokenArgs) -> Result<()>
let output =
serde_json::to_string(&token).expect("gcp token output should serialize");

// If we can't cache the token, report an error but don't fail the command.
// If we can't cache the token, log an error but don't fail the command.
if let Some(output_path) = output_file {
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:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

}
}

Expand Down
15 changes: 4 additions & 11 deletions app/src/ai/agent_sdk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use warp_cli::share::ShareRequest;
use warp_cli::task::{MessageCommand, TaskCommand};
use warp_cli::{CliCommand, GlobalOptions, OZ_HARNESS_ENV};
use warp_core::features::FeatureFlag;
use warp_errors::report_error;
use warp_graphql::object_permissions::OwnerType;
use warp_isolation_platform::IsolationPlatformError;
#[cfg(not(target_family = "wasm"))]
Expand Down Expand Up @@ -1148,7 +1147,7 @@ impl AgentDriverRunner {
Some(id)
}
Err(e) => {
report_error!(e);
log::error!("Failed to create task: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

// Continue without a task_id rather than failing entirely
None
}
Expand Down Expand Up @@ -1193,7 +1192,7 @@ impl AgentDriverRunner {
let parsed_task_id = match task_id_str.parse().context("Failed to parse task ID") {
Ok(id) => Some(id),
Err(e) => {
report_error!(e);
log::warn!("Failed to parse task ID: {e:#}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

None
}
};
Expand Down Expand Up @@ -1435,21 +1434,15 @@ impl AgentDriverRunner {
let environment = foreground
.spawn(move |_, ctx| -> Result<_, AgentDriverError> {
let server_id = ServerId::try_from(environment_id.as_str()).map_err(|_| {
report_error!(
"Invalid environment ID",
extra: { "environment_id" => %environment_id }
);
log::warn!("Invalid environment ID");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

AgentDriver::log_valid_environments(ctx);
AgentDriverError::EnvironmentNotFound(environment_id.clone())
})?;
let sync_id = SyncId::ServerId(server_id);

CloudAmbientAgentEnvironment::get_by_id(&sync_id, ctx)
.ok_or_else(|| {
report_error!(
"Environment not found with ID",
extra: { "environment_id" => %environment_id }
);
log::warn!("Environment not found with ID");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This accepted demotion remains unchanged in rework commit d3b5e34; no additional code change was requested for this path.

AgentDriver::log_valid_environments(ctx);
AgentDriverError::EnvironmentNotFound(environment_id)
})
Expand Down
Loading