Skip to content

fix(channels/bluesky-sidecar): recover reply context via XRPC on cache miss (closes #5452)#5471

Merged
houko merged 1 commit into
mainfrom
fix/bluesky-sidecar-thread-cache-restart-recovery
May 21, 2026
Merged

fix(channels/bluesky-sidecar): recover reply context via XRPC on cache miss (closes #5452)#5471
houko merged 1 commit into
mainfrom
fix/bluesky-sidecar-thread-cache-restart-recovery

Conversation

@houko

@houko houko commented May 21, 2026

Copy link
Copy Markdown
Contributor

Closes #5452.

Symptom

After ANY bluesky sidecar 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_notification stashed the AT Protocol {root: {uri, cid}, parent: {uri, cid}} reply struct in an in-process _thread_cache LRU (capacity 200) keyed by notification URI. _post_status looked it up on outbound. Cache empty after restart → lookup returned None → reply posted with no reply field → AT Protocol treats it as top-level.

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 is structurally insufficient.

Fix (option-1 from the issue — narrowest blast radius)

On _thread_cache miss in _post_status, issue one app.bsky.feed.getPosts?uris=<uri> XRPC call to re-fetch the post's cid AND its existing record.reply chain. The new _recover_reply_ref helper:

  1. Sanity-guards the input — must start with at://, otherwise return None without any HTTP call. A misrouted librefang_user from another channel (dingtalk sessionWebhook URL, telegram @handle) never reaches bsky's XRPC.
  2. Re-fetches via _get_json with the bearer token already in scope.
  3. Reconstructs {root, parent} using the same root-resolution rules _compute_reply_ref uses on inbound:
    • Post that IS itself a reply → keep its record.reply.root as the thread root.
    • Top-level post → root == parent.
  4. Re-populates _thread_cache so subsequent chunks in the same _post_status call (or any other reply to the same URI soon after) skip the re-fetch.
  5. Returns None on 4xx/5xx/malformed — caller logs a WARN naming the URI and degrades to a top-level skeet (same as pre-fix, but with operator visibility instead of silent mis-threading).

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 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 — this PR is option-1 only.

Test plan

  • cd sdk/python && pytest tests/test_bluesky_adapter.py49 passed (was 44; +5 regression guards)
  • cd sdk/python && pytest tests/ — full sidecar suite 1855 passed

New regression guards

Test Asserts
test_post_status_cache_miss_recovers_reply_ref_via_xrpc Cache empty → getPosts re-fetch → reply ref attached to createRecord → cache re-populated. URI-encoded in query param.
test_post_status_cache_miss_recovery_preserves_existing_thread_root Fetched post that's itself a reply: root = post's record.reply.root (NOT the immediate parent). Prevents deep-thread fork on every cache miss.
test_post_status_cache_miss_recovery_caches_for_subsequent_chunks 3-chunk reply with cache miss = 1 session + 1 getPosts + 3 createRecord (5 total), not 1 + 3×(getPosts + createRecord) (7 total).
test_post_status_cold_cache_recovery_failure_falls_back_to_unthreaded getPosts 404 → degrades to unthreaded post + WARN. Pre-existing test renamed + updated.
test_recover_reply_ref_rejects_non_at_uri Cross-channel garbage (dingtalk URL, @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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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.
@houko
houko force-pushed the fix/bluesky-sidecar-thread-cache-restart-recovery branch from 9c7067c to 3255cf1 Compare May 21, 2026 08:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@houko
houko merged commit 8a83908 into main May 21, 2026
15 checks passed
@houko
houko deleted the fix/bluesky-sidecar-thread-cache-restart-recovery branch May 21, 2026 08:36
@github-actions github-actions Bot added area/docs Documentation and guides area/sdk JavaScript and Python SDKs labels May 21, 2026
f-liva pushed a commit to f-liva/librefang that referenced this pull request May 21, 2026
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`.
@github-actions github-actions Bot added the size/L 250-999 lines changed label May 21, 2026
f-liva pushed a commit to f-liva/librefang that referenced this pull request May 25, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation and guides area/sdk JavaScript and Python SDKs size/L 250-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

channels/bluesky-sidecar: _thread_cache lost on sidecar restart — replies degrade to top-level skeets

1 participant