fix(desktop): make sidebar endpoint resilient to unmatched profile scopes (#67600) - #67617
fix(desktop): make sidebar endpoint resilient to unmatched profile scopes (#67600)#67617webtecnica wants to merge 1 commit into
Conversation
Related to #67600. The fallback handles invalid profile scopes, while current-main tracing of that report sends |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding diagnostics around an otherwise silent sidebar result.
Problems
- The later reproduction evidence on #67600 shows the affected packaged Desktop build calls
/api/profiles/sessions?profile=default, not/api/profiles/sessions/sidebar. That legacy route is handled byhermes_cli/web_server.py:4294-4329, while this PR only changes the batched route athermes_cli/web_server.py:4421. The captured request already carries the canonicaldefaultscope, so this change cannot affect the reported production path. - The new fallback at
hermes_cli/web_server.py:4484substitutes default-profile recents for any unmatched scope. That can hide the caller defect and show a different profile's sessions instead of retaining the requested-profile semantics. - Current coverage exercises only
recents_profile=allintests/hermes_cli/test_web_server.py:1845-1871; it does not cover either new normalization branch.
Suggested changes
- Re-scope from the captured released-client request/renderer path in #67600, after validating its response body and consumption path.
- Avoid default-profile substitution for arbitrary invalid scopes unless that behavior is explicitly intended, and add regression coverage for the selected contract.
Automated hermes-sweeper review.
| f'falling back to "default"' | ||
| ), | ||
| }) | ||
| recents_scope = "default" |
There was a problem hiding this comment.
An unmatched scope now displays the default profile's recents rather than preserving the requested profile or reporting a distinguishable validation failure. #67600's captured release request already sends profile=default to the separate /api/profiles/sessions route, so this substitution cannot address that repro and can mask a wrong caller scope.
|
Thanks for picking this up so quickly. The hardening here is worth having on its own — the silent zero-row return was the second problem I raised in #67600, and surfacing a warning in One thing I wanted to flag before merge, in case it's useful: I'm not sure this will change the symptom on my setup, and if so it might be worth keeping #67600 open. Timing-wise this PR landed at 17:49, shortly after the scope-divergence hypothesis and a little before I posted a packet capture testing it — so the data below simply wasn't available yet when you wrote the fix. It came out the other way: 1. The affected client never calls Two captures on the backend host — one against the published release, one against a build I compiled from No request to 2. On this setup there doesn't seem to be a scope mismatch to repair. The value sent for the root profile is 3. It reproduces on a source build of I packed the Desktop from Small suggestionThe resilience change is worth landing regardless. Would you consider switching One loose end from my last comment that may be the more promising thread: Happy to run any capture or instrumented build you'd like on the affected machine. |
|
@webtecnica following up on the one small thing from my earlier comment: would you consider changing Since I wrote that comment I've pinned a bisection window on #67600, which I think settles it:
Both boundaries are local builds from a checkout, on two machines against one backend. Within that window, exactly one commit touches None of that argues against landing your change. The silent zero-row return was a real problem and surfacing it in |
SummaryThree PRs address or reference #67600: merged #67245 introduced the batched sidebar fan-out whose primary-profile requests bypassed OAuth-aware authentication; open #67617 hardens unmatched server-side profile scopes but does not touch that authenticated client path; closed #71503 routes those requests through the OAuth-aware helper and matches the reported cause. Related pull requests
Duplicates#71503 and merged #72835 implement essentially the same OAuth-aware authentication fix for the primary sidebar session slices; #71503 is superseded by #72835. #67617 is not a duplicate because it changes server-side scope normalization instead. Suggested consolidationTreat #67600 as implemented on main by merged #72835 at commit Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 41 kB of PR diffs, 15 kB of issue/PR text, 23 kB of discussion (17 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
Refs #67600 — the
defaultprofile sidebar renders empty when the Desktop client sends arecents_profilethat doesn't exactly match"default"case-sensitively.Root Cause
get_profiles_sessions_sidebarat line 4511 uses an exact case-sensitive comparison (name == recents_scope). If the Desktop client sends"Default","", or any other non-exact identifier, the default profile never matches and the recents slice silently returns zero rows with no error.Fix
After computing
recents_scopeand before entering the per-profile loop, validate it against known profile names fromlist_profiles():recents_scopediffers only in case from a known profile name, use the canonical casing."default"— if the scope doesn't match any profile name at all (empty, wrong case with no match, bare path, etc.), use"default"as the scopee.errorslist so the client can see that a fallback occurred. This makes the bug class visible instead of silently returning an empty list.The
errorslist was moved up from line 4474 to before the validation block so warnings can be appended before the per-profile loop runs.Behavior Matrix
recents_profilesent"default""Default"""errors"/Users/…/.hermes"errors"all"