Demote non-actionable report_error! calls to log::warn! in terminal model#13911
Demote non-actionable report_error! calls to log::warn! in terminal model#13911warp-dev-github-integration[bot] wants to merge 2 commits into
Conversation
…odel Audits every `report_error!` call in the terminal model, session, history, and writeable_pty files per the logging-and-error-reporting skill, and demotes the ones that fire on external/expected/gracefully-handled conditions (channel teardown sends, temp-file cleanup, user/enterprise regex compile failures, corrupted or unexpected DCS input, invalid shell names, hostname lookup fallback, remote history read failure, bootstrap path-conversion fallback) to `log::warn!` so they no longer create Sentry issues. Genuine invariant/"should never happen" guards (ANSI handler contract stubs, bootstrap stage-machine violations, block-height/gap invariants, prompt-marker routing invariants, docker-sandbox routing-drift guard, the registered ReadHistoryContentsError sink, etc.) are kept as `report_error!`. Co-Authored-By: Warp <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 |
![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 selected terminal-model report_error! sites to log::warn! to reduce non-actionable Sentry issues.
Concerns
- Several new warn-level breadcrumbs include sensitive or untrusted runtime values. Warn breadcrumbs are still uploaded on crash-reporting builds, so demoting from
report_error!should also move those values behindsafe_warn!full-only logging or omit them from release breadcrumbs.
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
| Err(err) => { | ||
| report_error!(anyhow::Error::new(err) | ||
| .context("Failed to construct new RegexDFA with combined secrets")); | ||
| log::warn!("Failed to construct new RegexDFA with combined secrets: {err:#}"); |
There was a problem hiding this comment.
err can include the user or enterprise regex pattern that failed to compile, and warn breadcrumbs are uploaded to Sentry; use safe_warn! for both regex compile failures so the safe arm is static and the full error stays dogfood-local.
There was a problem hiding this comment.
Fixed — switched both regex/DFA compile-failure logs to safe_warn!, so the offending pattern in err only appears in the dogfood-only full: arm and the uploaded breadcrumb (safe:) is a static string.
| anyhow::Error::new(err).context("Failed to delete kitty temporary file"), | ||
| extra: { "path" => %path } | ||
| ); | ||
| log::warn!("Failed to delete kitty temporary file (path = {path}): {err:#}"); |
There was a problem hiding this comment.
safe_warn! or omit the path so user-controlled file paths are not sent to Sentry breadcrumbs.
There was a problem hiding this comment.
Fixed — this now uses safe_warn!; the user-controlled temp-file path is confined to the dogfood-only full: arm and kept out of the Sentry breadcrumb.
| extra: { "shell" => %data.shell } | ||
| None => log::warn!( | ||
| "Received invalid shell name in init_subshell: {}", | ||
| data.shell |
There was a problem hiding this comment.
data.shell comes from the terminal DCS payload and warn breadcrumbs are uploaded; log a safe static message or move the value to safe_warn!'s full arm.
There was a problem hiding this comment.
Fixed — init_subshell now uses safe_warn! with a static safe: message, and the DCS-supplied data.shell only in the full: arm. Applied the same fix to the sibling sourced_rc_file case, which had the identical issue.
Addresses review feedback: warn-level breadcrumbs are uploaded to Sentry on crash-reporting builds, so the demoted warns that include user/enterprise secret regex patterns (secrets.rs), a user-controlled temp-file path (kitty.rs), and a DCS-supplied shell name (terminal_model.rs init_subshell + sourced_rc_file) now use `safe_warn!` — the `safe:` arm is a static, breadcrumb- safe message and the sensitive detail only appears in the dogfood-only `full:` arm. Co-Authored-By: Warp <agent@warp.dev>
![warp-dev-github-integration[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Per the request in the Slack thread: one inline note per demotion hunk explaining the reasoning, citing the logging-and-error-reporting principles and my interpretation of each error case. (The 4 safe_warn! conversions also address the automated review's breadcrumb-PII concern.)
| anyhow::Error::new(err).context("Failed to delete kitty temporary file"), | ||
| extra: { "path" => %path } | ||
| safe_warn!( | ||
| safe: ("Failed to delete kitty temporary file"), |
There was a problem hiding this comment.
Demotion report_error! → safe_warn!. Per logging-and-error-reporting, report_error! is reserved for actionable failures where our code is wrong. fs::remove_file failing to clean up a kitty temp file is an external filesystem error we can't fix, and it's best-effort/handled (the sibling shm_unlink path just below already logs the same class at warn!) — "non-ideal but expected" = warn!. Used safe_warn! because warn breadcrumbs are uploaded to Sentry and the temp path is user-controlled, so the path only appears in the dogfood-only full: arm.
| report_error!(anyhow::Error::new(err) | ||
| .context("Failed to construct new RegexDFA with combined secrets")); | ||
| safe_warn!( | ||
| safe: ("Failed to construct new RegexDFA with combined secrets"), |
There was a problem hiding this comment.
Demotion report_error! → safe_warn!. The DFA fails to build only when the combined user/enterprise secret regexes can't compile (size/complexity limits) — bad external config, not an our-code bug, and gracefully handled (the previous regexes are kept, per the fn doc). That's the skill's warn! case (recoverable/handled). safe_warn! because err can embed the offending regex pattern and warn breadcrumbs ship to Sentry — the pattern stays in the dogfood-only full: arm.
| report_error!(anyhow::Error::new(err) | ||
| .context("Failed to construct new Regex with combined secrets")); | ||
| safe_warn!( | ||
| safe: ("Failed to construct new Regex with combined secrets"), |
There was a problem hiding this comment.
Same reasoning as the RegexDFA case above: a failure to build the combined secret regex is external config, gracefully handled (old regexes retained), so warn! per the skill. safe_warn! keeps the potentially-sensitive pattern in err out of the Sentry-uploaded breadcrumb (dogfood-only full: arm).
| "Failed to send ExecutedExecutorCommandEvent to InBandCommandExecutor" | ||
| )); | ||
| log::warn!( | ||
| "Failed to send ExecutedExecutorCommandEvent to InBandCommandExecutor: {e:#}" |
There was a problem hiding this comment.
Demotion report_error! → log::warn!. try_send failing here means the receiving InBandCommandExecutor channel is gone — a teardown/race, not a bug. Per the skill, channel-teardown/handled conditions are warn!, not Sentry-worthy. The error is a channel SendError with no sensitive payload, so plain log::warn!.
| } | ||
| Err(e) => { | ||
| warp_errors::report_error!(e); | ||
| log::warn!("Failed to get local hostname when determining session type: {e:#}"); |
There was a problem hiding this comment.
Demotion report_error! → log::warn!. get_local_hostname() failing is an environmental/system error (not our bug) and is handled with a fallback (default to Local) — the skill's warn! case. The error is a failure to read the hostname, so it doesn't carry the hostname value; plain log::warn! is safe.
| "Received invalid shell name in init_subshell", | ||
| extra: { "shell" => %data.shell } | ||
| None => safe_warn!( | ||
| safe: ("Received invalid shell name in init_subshell"), |
There was a problem hiding this comment.
Demotion report_error! → safe_warn!. An unparseable shell name comes from the terminal DCS payload (external/untrusted input) and is handled gracefully — the skill's warn! case, not actionable. data.shell is DCS-supplied and warn breadcrumbs ship to Sentry, so it appears only in the dogfood-only full: arm.
| "Received invalid shell name in SourcedRCFileForWarpValue", | ||
| extra: { "shell" => %data.shell } | ||
| safe_warn!( | ||
| safe: ("Received invalid shell name in SourcedRCFileForWarpValue"), |
There was a problem hiding this comment.
Same as the init_subshell case above: an invalid shell name in the sourced-rc-file DCS is external/untrusted input, handled gracefully → warn!. safe_warn! keeps the DCS-supplied data.shell out of the Sentry breadcrumb (dogfood-only full: arm).
| "Error occurred when sending generator command output" | ||
| )); | ||
| log::warn!( | ||
| "Error occurred when sending generator command output: {error}" |
There was a problem hiding this comment.
Demotion report_error! → log::warn!. try_send of generator output failing (right after an is_closed() check) is a receiver-drop race/teardown, not a bug — this matches the sibling warn! a few lines up for the exact same operation. Per the skill, teardown/handled conditions are warn!. No sensitive data.
| // Sending over a sync sender can block the current thread, so we do this async. | ||
| if let Err(e) = sender_clone.send(insert_command_event) { | ||
| report_error!(anyhow::Error::new(e).context("Error sending ModelEvent")); | ||
| log::warn!("Error sending ModelEvent: {e:#}"); |
There was a problem hiding this comment.
Demotion report_error! → log::warn!. Sending the ModelEvent fails only when the receiver has been dropped (shutdown/teardown) — expected, not our bug. Per the skill that's warn!, not Sentry-worthy. The SendError Display carries no sensitive data.
| } else { | ||
| self.write_terminating_bootstrap_bytes(ctx); | ||
| report_error!("Could not convert bootstrap script file path to str"); | ||
| log::warn!("Could not convert bootstrap script file path to str"); |
There was a problem hiding this comment.
Demotion report_error! → log::warn!. This is an already-handled fallback path — we fall back to write_terminating_bootstrap_bytes when the bootstrap-file path can't be converted to a str (an env/OS edge case, not a bug). Per the skill, expected fallback paths belong at warn!, not report_error!. Static message.
1 participant
Description
Audits every
report_error!call in the terminal-model batch of files (blocks/session/terminal_model/history/writeable_pty) against.agents/skills/logging-and-error-reporting/SKILL.mdand demotes the ones that don't represent actionable engineering issues tolog::warn!, to reduce Sentry noise. Onlyreport_error!creates Sentry issues;log::warn!is a breadcrumb only.Demoted to
log::warn!(12) — external / expected / gracefully-handled conditions, none of which are our-code bugs:model/session.rs— in-band output channel send (teardown), local-hostname lookup failure (falls back to Local), remote history-file read failure.model/terminal_model.rs— bootstrap message with no pending session info (treated as corrupted DCS), invalid shell name ininit_subshellandsourced_rc_fileDCS.model/secrets.rs— user/enterprise secret regex + DFA compile failures (keeps existing regexes).model/kitty.rs— temp-file cleanup failure (mirrors the siblingshm_unlinklog::warn!).model/session/command_executor/in_band_command_executor.rs— generator output channel send (mirrors siblinglog::warn!).writeable_pty/command_history.rs—ModelEventchannel send (teardown).writeable_pty/pty_controller.rs— bootstrap-script path conversion fallback.Kept as
report_error!(26) — genuine invariants / "should never happen" states that are real bugs if hit: theansi::Handlerset_title/push_title/pop_title"should be handled by TerminalModel" stubs (block/blocks/alt_screen/header_grid), bootstrap stage-machine violations, block-height/gap consistency invariants, prompt-marker routing invariants, the docker-sandbox routing-drift guard, the missing response-initiator invariant, the shell-type conflict anomaly, and the registeredReadHistoryContentsErrorsink (which already self-filters viais_actionable).Now-unused
use warp_errors::report_error;imports were removed from the files that no longer report.Linked Issue
N/A — requested via the factory-client triage channel.
Testing
Pure logging-level change (no behavior change), so no regression test is applicable.
./script/format— passes.cargo clippy -p warp --features local_fs,local_tty -- -D warnings— passes (0 warnings; covers every edited path, including thelocal_fs-gatedkitty.rs/pty_controller.rschanges).Agent Mode
Conversation: https://staging.warp.dev/conversation/490137f6-cb72-4636-a960-9abdb6b9cf22
Run: https://oz.staging.warp.dev/runs/019f71d5-9632-75d3-8a6f-2d307c38822b
This PR was generated with Oz.