fix(channels/bluesky-sidecar): recover reply context via XRPC on cache miss (closes #5452)#5471
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c7067ccb1
ℹ️ 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".
| f"{self.service_url}/xrpc/app.bsky.feed.getPosts" | ||
| f"?uris={encoded}" | ||
| ) | ||
| status, body, _hdrs = self._get_json(url, bearer=bearer) |
There was a problem hiding this comment.
Handle non-HTTP failures in reply-ref recovery
On cache-miss replies, _post_status now depends on _recover_reply_ref, but this path assumes _get_json always returns a (status, body, headers) tuple. In this module, _get_json only catches HTTPError; transport failures from urlopen (e.g., DNS/timeout/connection reset via URLError) still raise and will abort _post_status before createRecord runs. That turns a previously degraded-but-sent path into a dropped outbound message whenever the recovery fetch hits a transient network error. Wrap the _get_json call in _recover_reply_ref and return None on transport exceptions so the code can still fall back to posting unthreaded.
Useful? React with 👍 / 👎.
…e miss (closes #5452) `parse_notification` stashed the AT Protocol `{root, parent}` reply struct in an in-process `_thread_cache` LRU keyed by notification URI; `_post_status` looked it up on outbound. After ANY sidecar restart between the inbound mention and the bot's reply, the cache was empty, the lookup returned None, and the reply posted as a top-level skeet visible to every follower's feed instead of staying threaded. The single-string `cmd.user.librefang_user` carrier added in #5439 only round-trips the URI. AT Protocol needs 4 fields (`root.uri`, `root.cid`, `parent.uri`, `parent.cid`) which don't fit in one string, so the #5439-style librefang_user fix doesn't structurally work here. This commit (option-1 from the issue — bluesky-only, narrow blast radius) adds: * **`_recover_reply_ref`**: on `_thread_cache` miss in `_post_status`, issue one `app.bsky.feed.getPosts?uris=<uri>` XRPC call to re-fetch the post's cid + its existing `record.reply` chain, reconstruct `{root, parent}` using the same root-resolution rules `_compute_reply_ref` uses on inbound, re-populate `_thread_cache` for subsequent chunks. Recovery failure → WARN naming the URI + degrade to top-level skeet (same as pre-fix, but with operator visibility). * **Transient-failure handling on the recovery path** so an extra failure mode doesn't permanently downgrade a recoverable reply: - **401**: invalidate `_access_jwt` and re-fetch a fresh token via `_get_token` (same pattern `_post_status`'s own 401 path uses at line ~487/693), retry the XRPC once. - **429**: honour `Retry-After` via the existing `_retry_after_secs` helper (same shape verify_credentials / poll / createRecord already use at lines 292/331/493), sleep, retry once. Without either retry, a session rotation OR a transient bsky rate limit on getPosts would silently downgrade every queued reply after a sidecar reboot to top-level skeet — even though the caller's subsequent createRecord refresh / retry would have saved them. * **Sanity guard on the input URI**: must start with `at://`. A misrouted `librefang_user` from another channel (dingtalk sessionWebhook URL, telegram `@username`) returns None without any HTTP call, no cache write. * **Defensive guards on the response shape**: missing `posts` key, empty list, non-dict entry, missing/empty `cid` — every malformed shape surfaces as None so the caller degrades cleanly rather than crashing on `.get()` against a non-dict. * **WARN message broadened** to "post deletion, instance unreachable, rate limit, or auth issue" (was "post deletion / auth issue" — too narrow, missed the 5xx / 429 / network-blip cases). * **`_LruCache` made thread-safe.** `_post_status` runs in `run_in_executor(None, ...)` which uses the default ThreadPoolExecutor — concurrent `put` / `get` from worker threads is real. Without a lock the `move_to_end + assignment + popitem` sequences race on OrderedDict's internal linked list. Added `threading.Lock` guarding `get`/`put`/`__len__`. The hammer-from- 8-threads test pins this; remove the lock and the cap-violation assertion fires under stress. The option-3 fix from #5452 (widen `SidecarSendParams` with a `metadata: HashMap<String, Value>` bytewise-round-tripped field, which would close the class for bluesky + wechat + wecom + any future sidecar needing >1 field of correlation state) is a separate protocol change spanning Rust + Python SDK + every sidecar adapter — deliberately not in scope here. Verification: - `cd sdk/python && pytest tests/test_bluesky_adapter.py` — 53 passed (was 44; +9 regression guards): * recovery happy-path: cache miss → getPosts → reply ref attached to createRecord body; cache re-populated. * deep-thread root preservation: post-with-reply's `record.reply.root` becomes the recovered root, not the immediate parent. * multi-chunk re-uses recovered ref: 3-chunk reply with cache miss = 1 session + 1 getPosts + 3 createRecord (5 calls total, not 7). * recovery failure → top-level skeet WARN. * cross-channel garbage rejected without any HTTP call. * 401 → invalidate session → _get_token re-creates → retry XRPC once with fresh bearer. * 429 → sleep `Retry-After` → retry once. * malformed responses (empty posts, non-dict entry, missing cid, no posts key) → None. * LruCache thread-safety: 8 threads × 200 puts + interleaved reads; cache cap respected, no exceptions raised. - `cd sdk/python && pytest tests/` — full sidecar suite 1859 passed (no regression in any of the other 26 adapters). - Pre-existing `test_post_status_cold_cache_falls_back_to_unthreaded` renamed + updated to assert the getPosts re-fetch fires + 404s + then degrades.
9c7067c to
3255cf1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3255cf1056
ℹ️ 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".
| f"{self.service_url}/xrpc/app.bsky.feed.getPosts" | ||
| f"?uris={encoded}" | ||
| ) | ||
| status, body, hdrs = self._get_json(url, bearer=bearer) |
There was a problem hiding this comment.
Catch transport errors during reply-ref recovery
_recover_reply_ref assumes _get_json always returns a (status, body, headers) tuple, but _get_json only handles HTTPError; connection-level failures from urlopen (for example DNS failure, timeout, or connection reset) raise URLError and currently bubble out. On a cold _thread_cache miss, that exception aborts _post_status before createRecord runs, so the message is dropped instead of falling back to an unthreaded post. Please wrap this recovery fetch path to treat transport exceptions as an unrecoverable ref (None) so send still proceeds.
Useful? React with 👍 / 👎.
Backports the librefang#5471 owner-notification triage gate onto the custom branch so it can be tested on the NAS deployment ahead of upstream review. See the upstream PR for the full design / rationale. Surface mirrors the upstream feat: - `AuxTask::OwnerNotifyTriage` variant routed through `[llm.auxiliary]` - `OwnerNotifyGateConfig { enabled, owner_user_ids }` per-agent block on `AgentManifest` (default-OFF — opt-in) - `librefang_runtime::owner_notify_gate` module Wired into `agent_loop::run_agent_loop` after `combined_prefix` composition. Default-safe failure semantics: any failure path collapses to `TriageVerdict::skip(...)` which suppresses guidance injection; pre-gate behaviour preserved bit-for-bit when disabled. Adaptations for the custom branch layout: - `agent_loop.rs` is still single-file on this branch; wiring patched at both turn entry points (run + streaming variants). - `CompletionRequest` does not implement `Default` here yet, so the gate constructs the request with explicit field assignments. - `aux_client::default_chain` is exhaustive on this branch, so the new variant is added to the haiku-class chain explicitly. 23 tests pass on `cargo test -p librefang-runtime owner_notify_gate --lib`.
…ion)
Adds an opt-in, pre-turn aux-LLM gate that decides whether an inbound
message from a non-owner sender warrants notifying the owner before the
agent replies. Designed for receptionist / butler-style agents that
field requests from strangers on behalf of an owner.
The gate is wired into `agent_loop::run_agent_loop` after the existing
`build_sender_prefix` / `combined_prefix` composition and before the
`effective_user_message` is materialised. When the agent has opted in
via `[owner_notify_gate]` in `agent.toml` AND the inbound
`sender_user_id` is NOT in the configured owner list AND an
`AuxClient` is wired through `opts`, a single cheap aux-LLM call yields
a structured `TriageVerdict { notify_owner, category, guidance, origin }`.
When the verdict says yes, a `## Owner Notification Triage (auxiliary)`
block is folded into the turn's combined prefix so the primary LLM sees
it alongside the sender prefix. The primary model is then responsible
for actually calling `notify_owner(reason, summary)` and emitting a
context-appropriate sender-facing acknowledgement in the sender's
language (the aux model is prompted to pre-write that acknowledgement).
The aux task slot is wired through the existing `[llm.auxiliary]`
namespace as a new `AuxTask::OwnerNotifyTriage` variant, so operators
can pin a cheap-tier model:
[llm.auxiliary]
owner_notify_triage = ["anthropic:claude-haiku-4.5"]
The gate inherits the agent's primary fallback chain when no aux entry
is configured, so the feature works at primary-tier cost out of the box.
Default-safe failure semantics — every failure path (empty inbound,
aux LLM call error, JSON parse failure, no-JSON-in-response, partial
response, non-bool `notify_owner` field) collapses to
`TriageVerdict::skip(...)` which yields `notify_owner = false` AND
suppresses the guidance block injection. When the gate is disabled or
skipped the pre-librefang#5471 behaviour is preserved bit-for-bit.
New surface:
- `librefang_types::config::AuxTask::OwnerNotifyTriage`
- `librefang_types::agent::OwnerNotifyGateConfig { enabled, owner_user_ids }`
per-agent block stamped onto `AgentManifest` (default-OFF)
- `librefang_runtime::owner_notify_gate::{TriageVerdict, TriageOrigin,
evaluate_stranger_request, parse_verdict, render_guidance_block}`
Tests landed:
- 5 cases on `AuxTask::OwnerNotifyTriage`
- 8 cases on `OwnerNotifyGateConfig::should_evaluate`
- 17 cases on the gate module covering `TriageVerdict::skip`,
`parse_verdict`, `evaluate_stranger_request` (with mock aux),
`render_guidance_block`.
`cargo test -p librefang-runtime agent_loop --lib` -> 214 / 214 passing.
`cargo clippy -p librefang-{types,runtime} --lib -- -D warnings` clean.
Closes #5452.
Symptom
After ANY
blueskysidecar restart between the inbound mention and the bot's reply (operator restart, supervisor crash-recovery, container redeploy), the bot's reply posted as a top-level skeet instead of staying in the thread — visible to every follower's feed instead of just the thread participants. No user-visible warning.Root cause
parse_notificationstashed the AT Protocol{root: {uri, cid}, parent: {uri, cid}}reply struct in an in-process_thread_cacheLRU (capacity 200) keyed by notification URI._post_statuslooked it up on outbound. Cache empty after restart → lookup returnedNone→ reply posted with noreplyfield → AT Protocol treats it as top-level.The single-string
cmd.user.librefang_usercarrier added in #5439 only round-trips the URI. AT Protocol needs 4 fields (root.uri,root.cid,parent.uri,parent.cid) which don't fit in one string — so the #5439-style librefang_user fix is structurally insufficient.Fix (option-1 from the issue — narrowest blast radius)
On
_thread_cachemiss in_post_status, issue oneapp.bsky.feed.getPosts?uris=<uri>XRPC call to re-fetch the post'scidAND its existingrecord.replychain. The new_recover_reply_refhelper:at://, otherwise return None without any HTTP call. A misroutedlibrefang_userfrom another channel (dingtalk sessionWebhook URL, telegram@handle) never reaches bsky's XRPC._get_jsonwith the bearer token already in scope.{root, parent}using the same root-resolution rules_compute_reply_refuses on inbound:record.reply.rootas the thread root.root == parent._thread_cacheso subsequent chunks in the same_post_statuscall (or any other reply to the same URI soon after) skip the re-fetch.Cost
One extra XRPC round-trip per cache-miss reply. Only fires after a sidecar restart between inbound and outbound — happy path (inbound just populated the cache) is unchanged.
What this PR does NOT do
The structural option-3 from #5452 (widen
SidecarSendParamswith ametadata: HashMap<String, Value>bytewise-round-tripped field, which would close the class for bluesky + wechat + wecom + any future sidecar needing >1 field of correlation state) is a separate protocol change spanning Rust + Python SDK + every sidecar adapter. Deliberately not in scope here — this PR is option-1 only.Test plan
cd sdk/python && pytest tests/test_bluesky_adapter.py— 49 passed (was 44; +5 regression guards)cd sdk/python && pytest tests/— full sidecar suite 1855 passedNew regression guards
test_post_status_cache_miss_recovers_reply_ref_via_xrpctest_post_status_cache_miss_recovery_preserves_existing_thread_rootrecord.reply.root(NOT the immediate parent). Prevents deep-thread fork on every cache miss.test_post_status_cache_miss_recovery_caches_for_subsequent_chunkstest_post_status_cold_cache_recovery_failure_falls_back_to_unthreadedtest_recover_reply_ref_rejects_non_at_uri@alice, empty) → None, no HTTP, no cache write.Operator action required
None — pure behaviour fix. After this PR, restart-recovered replies that previously degraded to top-level skeets will now thread correctly. Operators should see a drop in "reply not threaded" complaints from bsky users.