feat(bluebubbles): add reply-context API fallback for cache misses#71820
Conversation
|
@greptile |
Greptile SummaryAdds an opt-in best-effort fallback ( Confidence Score: 5/5Safe to merge — no P0 or P1 issues found; behaviour is strictly additive behind an opt-in flag. No correctness or security bugs identified. The previous dead-code finding (redundant No files require special attention. Reviews (2): Last reviewed commit: "fix(bluebubbles): address PR #71820 revi..." | Re-trigger Greptile |
| if (body || sender) { | ||
| rememberBlueBubblesReplyCache({ | ||
| accountId: params.accountId, | ||
| messageId: replyToId, | ||
| chatGuid: params.chatGuid, | ||
| chatIdentifier: params.chatIdentifier, | ||
| chatId: params.chatId, | ||
| senderLabel: sender, | ||
| body, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The if (body || sender) condition on line 115 can never be false at that point: the early-return two lines above (if (!body && !sender) { return null; }) already guarantees that at least one of them is truthy. The wrapper if is dead code and can be removed to make the intent clearer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/bluebubbles/src/monitor-reply-fetch.ts
Line: 115-126
Comment:
**Redundant guard is unreachable dead code**
The `if (body || sender)` condition on line 115 can never be false at that point: the early-return two lines above (`if (!body && !sender) { return null; }`) already guarantees that at least one of them is truthy. The wrapper `if` is dead code and can be removed to make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 75c4ac1dd5612a35247e198894d46ce0347401b7. Removed the if (body || sender) wrapper — the if (!body && !sender) return null two lines above already guarantees one is truthy by the time we reach the cache write.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3c29d598
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ssrfPolicy = | ||
| params.accountConfig && isPrivateNetworkOptInEnabled(params.accountConfig) | ||
| ? ({ allowPrivateNetwork: true as const } as const) | ||
| : undefined; |
There was a problem hiding this comment.
Always apply SSRF guard on reply-context fetches
When fallback is enabled but network.dangerouslyAllowPrivateNetwork is not explicitly true, this sets ssrfPolicy to undefined and passes it to blueBubblesFetchWithTimeout; that helper only uses fetchWithSsrFGuard when a policy is present, so this new path skips SSRF hostname/pinning protections entirely. In practice, a compromised or redirecting BlueBubbles endpoint can pivot this request to unintended internal targets, unlike the rest of the BlueBubbles client paths that stay guarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 75c4ac1dd5612a35247e198894d46ce0347401b7. Routed through resolveBlueBubblesClientSsrfPolicy — the same three-mode helper every other BB client request uses. The resolver never returns undefined, so blueBubblesFetchWithTimeout always engages fetchWithSsrFGuard:
- mode 1: explicit private-network opt-in →
{ allowPrivateNetwork: true } - mode 2: parseable trusted hostname →
{ allowedHostnames: [hostname] } - mode 3: explicit opt-out on a private host or unparseable URL →
{}(default-deny)
Same bug class as #68234. Added an explicit "never passes undefined to the underlying fetch (regression for #71820 codex review)" test plus mode-by-mode coverage in extensions/bluebubbles/src/monitor-reply-fetch.test.ts. Thanks for the catch.
| * cache eviction), opt in to fetching the original message from the | ||
| * BlueBubbles HTTP API as a best-effort fallback. Off by default. | ||
| */ | ||
| replyContextApiFallback: z.boolean().optional().default(false), |
There was a problem hiding this comment.
Preserve channel-level fallback setting for account configs
Defaulting replyContextApiFallback to false inside each account schema causes account entries that omit the field to still materialize false, which then overrides any channel-level channels.bluebubbles.replyContextApiFallback: true during account merge. In multi-account deployments, operators enabling the global toggle will silently get no fallback unless they duplicate the flag under every accounts.<id> block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 75c4ac1dd5612a35247e198894d46ce0347401b7. Dropped .default(false) from the per-account schema field — accounts that omit the flag now inherit the channel-level value through mergeAccountConfig instead of clobbering it.
Added two end-to-end tests in setup-surface.test.ts that exercise the runtime merge path through resolveBlueBubblesAccount:
- channel-level
replyContextApiFallback: truesurvives when an account omits the flag - account-level
falsestill overrides channel-leveltrue
Plus a schema-level test confirming the per-account default no longer materializes for missing fields.
|
Codex review: needs changes before merge. What this changes: This PR adds a default-off BlueBubbles Required change before merge: There is one narrow mechanical repair suitable for automation: make the reply-context fallback cache lookup/write-back handle part-index-prefixed IDs consistently and add a focused regression test. Security review: Security review cleared: The diff adds an opt-in HTTP fetch path, but it routes through the existing typed BlueBubbles client and SSRF policy, encodes the validated message id, remains best-effort, and does not add new dependencies, workflow changes, or secret-handling expansion. Review findings:
Review detailsBest possible solution: Land the BlueBubbles fallback after preserving cache hits for part-index-prefixed reply ids, keeping the feature default-off, extension-owned, typed-client routed, and documented, with any future default-on decision left to a separate product review. Do we have a high-confidence way to reproduce the issue? Yes. Main can be reproduced by processing an inbound BlueBubbles reply with a Is this the best way to solve the issue? Yes, with one small repair. The PR's default-off, BlueBubbles-owned typed-client fallback is the narrow maintainable direction; the safer final shape is to canonicalize or alias prefixed reply ids consistently so the promised cache amortization works. Full review comments:
Overall correctness: patch is correct Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against eabab1f64f9e. |
|
Addressed all three review findings in 75c4ac1dd5612a35247e198894d46ce0347401b7:
Verified locally: |
75c4ac1 to
f1247c6
Compare
|
Rebased onto latest Branch is now 0 commits behind |
|
Pushed 205b89fe6c — added |
|
Resolved CHANGELOG.md merge conflict in 706de15b54. Upstream cut release |
a2ba44d to
1f690de
Compare
|
Force-pushed a clean rebase onto current upstream/main (1f690dec35). Two-commit history, 7 changed files (all BlueBubbles + CHANGELOG.md), +739/-0 — strictly scoped to the feature. Addressed all three codex review items:
|
|
Two follow-ups pushed (27b6105990, 9d081574a3) that fold in @zqchris's #73241 architectural improvements while preserving this PR's production wins. Refactor: route through typed BlueBubblesClient (27b6105990)The reply-context fetch now uses Also picked up @zqchris's reply-id shape validation as defense-in-depth: Test seam swapped from raw fetch ( Pickup: attachment-error visibility + log redaction (9d081574a3)Both behavioral changes originated in @zqchris's #73241 and are preserved with attribution in the CHANGELOG:
Production wins this PR has and #73241 doesn't
Net diff7 files, +815/-7 (was +739/-0). Strictly BB-scoped. Tests: 78 BB-targeted (incl. setup-surface end-to-end), 0 changes outside the BlueBubbles extension and CHANGELOG. CHANGELOG entry moved to the bottom of CC @zqchris — appreciated the architectural pointer and the observability fix; happy to defer to your PR if maintainers prefer a fresh review surface, or close mine and land yours. |
9d08157 to
a7f8f1d
Compare
|
Rebased onto
First Current branch tip is Open to maintainer review whenever cycles exist — happy to flip the default, slim the config nesting ( |
c388b55 to
7506d2b
Compare
7506d2b to
b41b207
Compare
…nt merge, dead code) - Route reply-context fetch through resolveBlueBubblesClientSsrfPolicy so the SSRF guard is always engaged. The three-mode resolver never returns undefined; previously we passed undefined when private-network opt-in was off, which silently skipped fetchWithSsrFGuard. Same bug class as openclaw#68234. - Drop .default(false) from per-account replyContextApiFallback so accounts that omit the flag inherit the channel-level value through mergeAccountConfig instead of clobbering it with false. - Remove redundant if (body || sender) guard left after the early-return on the previous line. - Add three-mode SSRF coverage in monitor-reply-fetch.test.ts (mode 1 opt-in, mode 2 hostname allowlist, mode 3 default-deny) plus an explicit regression test, and end-to-end inheritance tests in setup-surface.test.ts through resolveBlueBubblesAccount.
…eBubblesClient Replaces the manual SSRF-policy-resolve + raw-fetch path with createBlueBubblesClientFromParts, so the request goes through the same typed client surface every other BlueBubbles call uses. Picks up the folding-into-typed-client suggestion the codex meta-review left as a follow-up on the original PR review, and matches the architectural pattern in openclaw#73241. - Drop direct imports of buildBlueBubblesApiUrl, blueBubblesFetchWithTimeout, and resolveBlueBubblesClientSsrfPolicy. The typed client owns SSRF policy resolution internally and cannot produce an undefined policy. - Add sanitizeReplyToId: strip part-index prefix (`p:0/<guid>` → `<guid>`) and validate the bare GUID against the known charset + 128-char cap before it reaches the API path. Defense-in-depth alongside the typed client's URL construction (CWE-20). - Swap the test seam from fetchImpl (raw fetch) to clientFactory (typed client constructor). New tests assert factory-call params for SSRF modes 1/2/3 plus the regression invariant from the original review. - Keep in-flight dedupe and reply-cache write-back (production wins for high-volume group chats — concurrent webhooks for the same replyToId coalesce into one fetch, and successful responses populate the cache so subsequent webhooks resolve from RAM). Tests: 23 in monitor-reply-fetch.test.ts (+1 new pathological-id case, +1 part-index-strip case), 78 BB-targeted total green.
…r and redact secrets in logs Carries forward two BlueBubbles observability/security fixes from openclaw#73241 so this PR is a strict superset of the competing implementation. - Promote attachment download failures from logVerbose to runtime.error alongside a verbose copy. Previously, BB attachment download failures (pinned-dispatcher compat bugs, BB server 500s, transient network errors) were invisible at default log level — agents only saw the `<media:image> (1 image)` placeholder text with nothing in the log pointing at the actual failure. - Extend sanitizeForLog to redact `?password=…` / `?token=…` / `?api_key=…` / `?secret=…` query params and `Authorization: Bearer …` headers before they reach the log sink. BB uses query-string auth by default, so error chains carrying captured request URLs were vulnerable to leaking the API password into log aggregators (CWE-532). - Move the changelog entry to the bottom of `## Unreleased` → `### Changes`. Insertions at the top of that section are the most common cause of CHANGELOG conflicts on long-lived PRs; bottom position keeps the diff context stable. Both behavioral changes originated in @zqchris's PR openclaw#73241; attribution preserved in the CHANGELOG entry.
b41b207 to
04f6a87
Compare
|
Merged via squash.
Thanks @coletebou! |
…penclaw#71820) Merged via squash. Prepared head SHA: 04f6a87 Co-authored-by: coletebou <12384893+coletebou@users.noreply.github.com> Co-authored-by: omarshahine <10343873+omarshahine@users.noreply.github.com> Reviewed-by: @omarshahine
…penclaw#71820) Merged via squash. Prepared head SHA: 04f6a87 Co-authored-by: coletebou <12384893+coletebou@users.noreply.github.com> Co-authored-by: omarshahine <10343873+omarshahine@users.noreply.github.com> Reviewed-by: @omarshahine
Summary
Adds an opt-in best-effort fallback: when an inbound BlueBubbles reply arrives without
replyToBody/replyToSenderand the in-memory reply-context cache misses, fetch the original message from the BlueBubbles HTTP API (GET /api/v1/message/{id}) and populate the cache so subsequent replies amortize. Off by default (channels.bluebubbles.replyContextApiFallback: false).When the cache hits — the common case — there is zero additional cost. The fallback only fires when the existing in-RAM lookup comes up empty.
Motivation
The current in-memory reply cache is fast but lives in one process's RAM. Several real deployment patterns drop reply context today even when the original message exists in BlueBubbles:
In all four cases the agent currently loses thread context and the user has to re-explain. With this opt-in, BlueBubbles becomes the source-of-truth fallback. (Related to the Private-API-status-cache class of degradation discussed in #43764 — different cache, complementary fix path.)
Design choices
false— strictly backwards-compatible. Path:channels.bluebubbles.replyContextApiFallback(also accepted per-account underaccounts.<id>.). Schema inextensions/bluebubbles/src/config-schema.ts, type inextensions/bluebubbles/src/types.ts.fetchBlueBubblesReplyContextinextensions/bluebubbles/src/monitor-reply-fetch.ts— single responsibility, fully testable in isolation, no deep dependencies.${accountId}:${replyToId}— concurrent webhooks for replies to the same original message coalesce into one HTTP fetch. Released on settle.null; the caller proceeds with empty reply context, matching pre-fallback behavior.buildBlueBubblesApiUrl,blueBubblesFetchWithTimeout,isPrivateNetworkOptInEnabled,normalizeOptionalString,normalizeBlueBubblesHandle,rememberBlueBubblesReplyCache. No duplication.{data: {...}}-wrapped and flat shapes. Body extracted fromtext??body??subject. Sender fromhandle.address??handle.id??senderId??sender.isPrivateNetworkOptInEnabledseam.Why not always-on?
Three reasons to keep the default off in this PR:
Happy to flip the default once there's a comfortable baseline. The implementation is identical either way.
Why not push notifications from BB Server?
Considered. BlueBubbles Server's webhook payload doesn't include the full original message body for
replyevents — onlyselectedMessageGuid(an ID). The HTTP API is the only authoritative source upstream of the cache. Asking BB Server to denormalize would be a separate cross-project ask.Test plan
pnpm test extensions/bluebubbles/src/monitor-reply-fetch.test.ts— 18 cases covering:replyToId/ missing creds → no fetch made)text→body→subject)handle.address→handle.id→senderId→sender)normalizeBlueBubblesHandlenull, no throwaccountId+replyToId→ one fetch)accountId→ independent fetches)Plus the full
pnpm check:changedlane (528 BB extension tests + typecheck core/extensions, lint, import-cycles) andpnpm build. All green.Backward compatibility
With
replyContextApiFallbackdefaulting tofalse, every existing deployment behaves identically to currentmain. To opt in:{ "channels": { "bluebubbles": { "replyContextApiFallback": true } } }(or under
channels.bluebubbles.accounts.<accountId>.replyContextApiFallbackfor per-account control.)Open questions
replyContextApiFallbackmatches theenrichGroupParticipantsFromContactsprecedent. If you'd prefer a richer surface for future tuning knobs (timeout override, max-age cap), happy to nest underreplyContext: { apiFallback: true, ... }.Marking as draft for maintainer review per the "larger behavior" guidance in
CLAUDE.md. Happy to slim or restructure based on feedback before flipping out of draft.