Demote non-actionable AI blocklist error reports#13928
Demote non-actionable AI blocklist error reports#13928warp-dev-github-integration[bot] wants to merge 1 commit into
Conversation
|
@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 |
Co-Authored-By: Oz <oz-agent@warp.dev>
86944be to
dda90d8
Compare
![warp-dev-github-integration[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
| report_error!( | ||
| e.context("Failed to persist 'Show code suggestion speedbump' setting") | ||
| ); | ||
| log::warn!("Failed to persist 'Show code suggestion speedbump' setting: {e:#}"); |
There was a problem hiding this comment.
The logging skill reserves report_error! for actionable invariants or bugs. This best-effort speedbump preference write is recoverable and the suggestion flow continues, so an expected persistence failure belongs at log::warn!.
| report_error!( | ||
| e.context("Failed to persist 'Show code suggestion speedbump' setting") | ||
| ); | ||
| log::warn!("Failed to persist 'Show code suggestion speedbump' setting: {e:#}"); |
There was a problem hiding this comment.
The logging skill says recoverable persistence failures should be warnings when functionality proceeds. Failure to save this speedbump preference does not prevent the unit-test suggestion UI from being created, so it is non-actionable here.
| ctx); | ||
| } | ||
| Err(e) => report_error!(e), | ||
| Err(e) => log::warn!( |
There was a problem hiding this comment.
Changing Agent Mode read-only command permissions is a best-effort settings write; the action handler continues after an external persistence failure. Per the logging skill, this degraded-but-handled path should emit a warning rather than a Sentry issue.
| ); | ||
| } | ||
| Err(e) => report_error!(e), | ||
| Err(e) => log::warn!( |
There was a problem hiding this comment.
This file-access permission update is recoverable and does not indicate an invariant violation. The logging skill classifies handled persistence failures with otherwise intact behavior as log::warn!, so this is intentionally demoted.
| ); | ||
| } | ||
| Err(e) => report_error!(e), | ||
| Err(e) => log::warn!( |
There was a problem hiding this comment.
Codebase-search permission persistence is another best-effort user-setting write; the UI has already applied the selection and continues. Following the logging skill, an external/degraded persistence failure is warning-level, not actionable Sentry reporting.
| ctx, | ||
| ) { | ||
| report_error!(e); | ||
| log::warn!("Failed to persist Agent Mode write-to-PTY permission: {e:#}"); |
There was a problem hiding this comment.
The write-to-PTY permission preference is persisted opportunistically while the CLI view continues. The logging skill directs expected or recoverable settings failures to log::warn! instead of report_error!.
| ctx, | ||
| ) { | ||
| report_error!(e); | ||
| log::warn!( |
There was a problem hiding this comment.
A file-access permission persistence failure is handled without aborting the CLI interaction. This is the logging skill's non-actionable/degraded case, so a warning is appropriate and avoids a noisy Sentry issue.
| ctx, | ||
| ) { | ||
| report_error!(e); | ||
| log::warn!("Failed to persist Agent Mode write-to-PTY permission: {e:#}"); |
There was a problem hiding this comment.
This CLI action updates a user permission setting through a best-effort store and continues on failure. Per the logging skill, recoverable external persistence errors belong at warning level rather than report_error!.
| ctx, | ||
| ) { | ||
| report_error!(e); | ||
| log::warn!("Failed to persist Agent Mode file access permission: {e:#}"); |
There was a problem hiding this comment.
The file-access permission write is non-fatal to the CLI action and is already handled at this sink. The logging skill therefore calls for log::warn! for this expected/degraded persistence failure.
| "Missing collapsible state for orchestration message", | ||
| extra: { "message_id" => ?message_id } | ||
| ); | ||
| log::warn!("Missing collapsible state for orchestration message: {message_id:?}"); |
There was a problem hiding this comment.
The renderer explicitly handles missing collapsible state by returning None, and callers gracefully skip the body. This is the logging skill's handled fallback case, so it should be a warning rather than an actionable Sentry report.
![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 non-actionable AI blocklist persistence/fallback error reports from report_error! to warning breadcrumbs while leaving existing invariant and unexpected-result reports in place.
Concerns
- No blocking correctness, security, or spec-drift concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
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 implementation is limited to the requested ten report_error! demotions, preserves the existing invariant/malformed-state reports, and all GitHub checks are successful. The local presubmit completed formatting and clippy but the test-compilation stage was SIGKILLed by sandbox memory pressure, so CI is the authoritative test result here.
Concerns
scripts/factory-pr-meta verify --pr 13928 --repo warpdotdev/warp finds zero valid blocks, so factory-state cannot resolve APP-4877 to this PR and the task↔PR mapping is not durable. Add exactly one metadata block for APP-4877 (including the repo and PR URL) to the PR description before acceptance.
Verification
gh pr checks 13928 --repo warpdotdev/warp: 20 successful, 6 skipped, 0 failing, 0 pending. git diff --check passed. No regression test is required for this logging-only, behavior-preserving change.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
|
Addressed the review blocker from review 4727401391: the PR body now contains exactly one valid factory-agent metadata block for APP-4877, including the Linear issue, repository, PR URL, and Oz run. |
There was a problem hiding this comment.
Overview
Re-review after the prior rejection for a missing factory-agent metadata block. scripts/factory-pr-meta verify --pr 13928 --repo warpdotdev/warp now returns {"valid":true,"count":1}, so the APP-4877 ↔ PR #13928 mapping is durable and that blocker is resolved. The code is unchanged from the previously-reviewed head (dda90d8e): ten report_error! → log::warn! demotions across block.rs, block/cli.rs, and block/view_impl/orchestration.rs, with invariant/malformed-state reports retained. Every demoted site is a best-effort settings/permission persistence write or a handled let … else fallback where the app continues with functionality intact, which the logging-and-error-reporting skill classifies as log::warn! rather than an actionable Sentry report. Each new log::warn! uses inline format args with {e:#} for the error chain, matching the workspace clippy config and the skill's preferred form, and the now-unused report_error import was removed from cli.rs.
Concerns
- 💡 [SUGGESTION] (non-blocking) The PR body links the Linear issue but omits an "Originating thread" line for the Slack conversation the request came from. The factory PR convention prefers the originating thread link when one is available; adding
Originating thread: https://warpdotdev.slack.com/archives/C0BDQDW8V5E/p1784336824289679near the top of the body would fully satisfy it. Not a merge blocker.
Verification
gh pr checks 13928 --repo warpdotdev/warp: 20 successful, 6 skipped (conditional merge-queue/approval gates), 0 failing, 0 pending — Linux/MacOS/Windows tests and Formatting + Clippy on all platforms pass. Not a user-facing change (logging-level only), so no visual proof is required. No regression test is required for this logging-only, behavior-preserving change; the PR body states the rationale.
Verdict
Found: 0 critical, 0 important, 1 suggestion (non-blocking)
Approve
1 participant
Description
Demote non-actionable
report_error!calls in the AI blocklist audit scope tolog::warn!while retaining Sentry reports for invariants, malformed state, and unexpected action-result types. The changes cover ten persistence/fallback hunks acrossblock.rs,block/cli.rs, andblock/view_impl/orchestration.rs.Linked Issue
Testing
./script/formatpassed.CARGO_BUILD_JOBS=2 cargo clippy --workspace --exclude warp_completer --all-targets --tests -- -D warningspassed.CARGO_BUILD_JOBS=2 cargo clippy -p warp --all-targets --tests -- -D warningspassed.CARGO_BUILD_JOBS=2 cargo clippy -p warp_completer --all-targets --tests -- -D warningspassed../script/presubmitreached test compilation, but rustc was SIGKILLed by sandbox memory pressure; no compiler diagnostic or test failure was reported.Agent Mode
CHANGELOG-NONE
Conversation: https://staging.warp.dev/conversation/b549f2aa-839e-4fa2-9915-055d961b4a2b
Run: https://oz.staging.warp.dev/runs/019f72ca-0c32-7832-a372-e6241f1247be
This PR was generated with Oz.