fix(channels): suppress Telegram PATCH 404 reaching Sentry (TAURI-R7)#2222
Conversation
Prior fix (TAURI-2Y) added BackendApiError::MessageNotFound in authed_json and catch sites in all four bus.rs call sites, but parse_message_path matched only exact 4-segment paths. A BACKEND_URL with a path prefix (e.g. /api/v1/...) adds extra segments, causing parse_message_path to return None — the 404 then falls through to report_error and Sentry (28 events). Three defense layers: 1. parse_message_path: add sliding-window fallback to match channels/*/messages/* anywhere in the segment list 2. authed_json: defense-in-depth inline check — suppress PATCH/DELETE 404s on any channel-message path that parse_message_path still can't match 3. is_channel_message_not_found_event: outermost before_send Sentry filter for any future call site that bypasses the first two layers 17 new tests covering parse_message_path edge cases (canonical, base-path prefix, double prefix, trailing slash, percent-encoded slug, negative cases) and the observability filter (PATCH/DELETE match, GET/wrong-status/wrong-domain non-match, exception-value path). Closes tinyhumansai#2203
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughExpand parse_message_path to find channel-message subsequences inside prefixed paths, add an authed_json early-404 bail for unparseable PATCH/DELETE channel-message paths, and add a Sentry classifier + before_send integration to drop expected channel-message 404 noise. ChangesChannel message 404 robustness and observability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/rest_tests.rs (1)
425-467: ⚡ Quick winAdd a case that hits the parse-failure fallback branch.
This test currently validates the typed
MessageNotFoundpath, but it doesn’t exercise the newcontains("/channels/") && contains("/messages/")suppression branch whenparse_message_pathreturnsNone.Suggested test extension
#[tokio::test] async fn authed_json_patch_404_with_base_path_prefix_does_not_report() { @@ let app = axum::Router::new().route( "/channels/telegram/messages/9999", axum::routing::any(|| async { (axum::http::StatusCode::NOT_FOUND, "Not Found") }), - ); + ).route( + "/foo/channels/telegram/bar/messages/9999", + axum::routing::any(|| async { (axum::http::StatusCode::NOT_FOUND, "Not Found") }), + ); @@ assert_eq!(provider, "telegram"); assert_eq!(message_id, "9999"); + + // Parse-failure fallback: contains both markers but does not match + // channels/<provider>/messages/<id> in any 4-segment window. + let err = client + .authed_json( + "mock-jwt", + Method::PATCH, + "/foo/channels/telegram/bar/messages/9999", + None, + ) + .await + .unwrap_err(); + assert!( + err.downcast_ref::<BackendApiError>().is_none(), + "fallback branch should not wrap as MessageNotFound" + ); + assert!( + err.to_string().contains("channel message not found (404)"), + "fallback branch should bail with suppression marker" + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/rest_tests.rs` around lines 425 - 467, Add a second assertion path in authed_json_404_with_base_path_prefix test that exercises the parse-failure fallback: create a request path that will cause parse_message_path to return None but still matches the heuristic (contains("/channels/") && contains("/messages/")), call BackendOAuthClient::authed_json with that path (ensuring base path handling like before), await the error, and assert it is an untyped 404 suppression (i.e., not BackendApiError::MessageNotFound and no panic/report). Reference authed_json, BackendOAuthClient::new, and parse_message_path/contains("/channels/") && contains("/messages/") when locating where to add the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/rest.rs`:
- Around line 548-555: The log in the authed_json path is using tracing::warn
for an expected, intentionally suppressed 404; change the call to a lower
verbosity (tracing::debug or tracing::trace) so it no longer emits a warn-level
diagnostic. Locate the tracing::warn! invocation with domain="backend_api" and
operation="authed_json" (the channel-message 404 message) and replace it with
tracing::debug! (or tracing::trace! per project convention) preserving the same
structured fields and message text.
---
Nitpick comments:
In `@src/api/rest_tests.rs`:
- Around line 425-467: Add a second assertion path in
authed_json_404_with_base_path_prefix test that exercises the parse-failure
fallback: create a request path that will cause parse_message_path to return
None but still matches the heuristic (contains("/channels/") &&
contains("/messages/")), call BackendOAuthClient::authed_json with that path
(ensuring base path handling like before), await the error, and assert it is an
untyped 404 suppression (i.e., not BackendApiError::MessageNotFound and no
panic/report). Reference authed_json, BackendOAuthClient::new, and
parse_message_path/contains("/channels/") && contains("/messages/") when
locating where to add the new case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39979cd8-f06f-4868-982d-704ea4124aa7
📒 Files selected for processing (4)
src/api/rest.rssrc/api/rest_tests.rssrc/core/observability.rssrc/main.rs
Expected, intentionally suppressed paths should not emit warn-level diagnostics. This is the inline fallback in authed_json that fires only when parse_message_path cannot match an exotic URL variant. Addresses CodeRabbit review on PR tinyhumansai#2222.
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
Summary
parse_message_pathinsrc/api/rest.rswith a sliding-window fallback sochannels/*/messages/*is matched anywhere in the path, not only when it's exactly 4 segmentsauthed_json: PATCH/DELETE 404s on any channel-message path that the parser still can't match are silently discarded (warn log, no Sentry)is_channel_message_not_found_eventtosrc/core/observability.rsas an outermostbefore_sendSentry filter, consistent with existing filter patternsparse_message_pathedge cases and the observability filterProblem
Sentry issue OPENHUMAN-TAURI-R7 — 28 production events (Windows + Linux).
PATCH /channels/telegram/messages/<id>returns 404 when the backend GC's the relay row while the core still holds themessage_id.A prior fix (TAURI-2Y) added
BackendApiError::MessageNotFoundinauthed_jsonand catch sites in all fourbus.rscall sites (lines 441, 564, 721, 779). Those catch sites are complete — no gaps there.The root cause is
parse_message_pathmatching only exactly 4 path segments (/channels/<p>/messages/<id>). If a user'sBACKEND_URLhas a path prefix (e.g.https://api.example.com/api/v1),Url::joinproduces/api/v1/channels/telegram/messages/1103— 6 segments — causingparse_message_pathto returnNone. The 404 then falls through toreport_error→ Sentry.Solution
Three layered defenses following the existing defense-in-depth pattern in this codebase:
parse_message_pathsliding window (src/api/rest.rs:35–57): keeps the fast-path 4-segment check, then scanssegments.windows(4)for thechannels/*/messages/*subsequence anywhere in the path.Inline suppression in
authed_json(src/api/rest.rs:528–545): afterparse_message_pathreturnsNone, bail withtracing::warn!+anyhow::bail!if the method is PATCH/DELETE and the URL path contains both/channels/and/messages/. No Sentry event fires.before_sendfilter (src/core/observability.rs,src/main.rs):is_channel_message_not_found_eventmatchesdomain=backend_api + failure=non_2xx + status=404 + method=PATCH|DELETE + path contains /channels/ + /messages/. Same pattern asis_budget_event,is_max_iterations_event, etc.Submission Checklist
cargo test -p openhumanCloses #2203Impact
bus.rscatch sites — this just prevents the Sentry noise that was slipping through whenparse_message_pathcouldn't parse the URLRelated
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/channels-telegram-patch-4044d6d15d4737b579968319f47c9b3ff3a22b2e909Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test -p openhuman -- parse_message_path channel_message_not_found authed_json→ 17 passedcargo fmt --check✓ ·cargo clippy -p openhuman✓ (no errors)Validation Blocked
Behavior Changes
authed_json, never reachingreport_errorbus.rscatch sites; this removes the Sentry noise that occurred whenparse_message_pathreturnedNoneParity Contract
bus.rsMessageNotFoundcatch sites unchanged; existingauthed_json_surfaces_message_not_found_on_404test still passesauthed_json_404_outside_messages_path_still_reportsconfirms non-channel-message 404s still reachreport_errorDuplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests