fix(channels): pair Telegram self-bot-token on /start and keep channel tool-calling alive#4414
Conversation
…l tool-calling alive Two channel defects that left the messaging bots doing plain inference with no tool calling and, on Telegram's own-bot-token path, no reply at all. Telegram self-bot-token approval (openhuman#4381) - A blank "Allowed Users" list arms first-run pairing, but the one-time bind code was only printed to core stdout — invisible to a desktop operator — so every message stayed stuck on the approval prompt and never reached the agent. Treat the operator's first `/start` (while pairing is pending AND the allowlist is still empty) as their explicit setup signal: allowlist that sender (runtime + persisted) and let their messages through, matching the "first sender after /start" behaviour the issue sanctions. The guard is tight — an explicitly configured allowlist never auto-approves a stranger's /start, and once the operator is bound the window closes. - Replace the misleading "approve the pairing in the web UI" prompt (no such action exists for this path) with accurate /start / `/bind <code>` guidance. - Factor the shared approve+persist+ack path so /start and /bind stay in lock-step. Channel tool-calling delegation (both Discord and Telegram) - resolve_target_agent laundered a transient Composio failure or a 3s timeout into an empty connected-integration set, which dropped delegate_to_integrations_agent from the turn and disabled tool calling for that message. Use the status-returning fetch and fall back to the cached integration snapshot on Unavailable/timeout (the same defence the first-party turn path uses); only an authoritative empty set collapses the delegation surface. Tests: /start + bindable-identity + allowlist-empty guards; connected-integration fallback matrix (authoritative/unavailable/timeout/no-cache).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTelegram onboarding now recognizes ChangesTelegram approval onboarding
Progressive UI channel gating
Composio connected-integrations fallback
Runtime snapshot cache isolation
Builtin loader graph contract
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant TelegramUser
participant TelegramChannel
participant PairingGuard
participant Persistence
TelegramUser->>TelegramChannel: sends /start or /bind <code>
TelegramChannel->>TelegramChannel: is_start_command() / bindable_identity()
TelegramChannel->>TelegramChannel: allowlist_is_empty()
alt first-run /start onboarding
TelegramChannel->>Persistence: approve_and_persist_sender()
TelegramChannel->>PairingGuard: invalidate_code()
TelegramChannel-->>TelegramUser: connected confirmation
else /bind <code>
TelegramChannel->>Persistence: approve_and_persist_sender()
TelegramChannel-->>TelegramUser: connected confirmation or warning
else unauthorized
TelegramChannel-->>TelegramUser: approval prompt
end
sequenceDiagram
participant DispatchRouting
participant Composio
participant Cache
DispatchRouting->>Composio: fetch_connected_integrations_status()
alt authoritative integrations available
Composio-->>DispatchRouting: Authoritative(list)
else unavailable or timeout
DispatchRouting->>Cache: cached_active_integrations_including_expired()
Cache-->>DispatchRouting: cached snapshot
DispatchRouting->>DispatchRouting: connected_with_fallback()
end
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
agent
The channel streaming engine posts ephemeral placeholders during a turn — an evolving "_working…_" draft, rotating "💭 Still working on it…" fillers, and a "thinking" bubble — then edits them in place and deletes them once the final reply lands. This assumes the channel backend supports both message edit and delete. Discord's adapter supports neither: edits 404 (so each tick posts a fresh bubble instead of updating one) and deleteMessage is a hard "Delete not supported" stub (so nothing gets cleaned up). The result is a wall of un-deletable "💭" messages on every Discord turn, especially long ones. Add channel_supports_progressive_ui() (false for Discord) and gate the draft/filler/thinking sends on it. Discord now shows only the typing indicator during the turn and a single clean final reply; Telegram keeps its evolving-bubble UX unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbca4be5be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…e-cache fallback Codex review comments on tinyhumansai#4414: - channel_recv.rs / pairing.rs (C5): the `/start` self-token onboarding path allowlisted the operator but never consumed the one-time pairing code (only `try_pair` did), leaving the `/bind <code>` window open for a later sender who obtains the stdout code. Add `PairingGuard::invalidate_code()` and call it once the operator is bound via `/start`, finishing the one-time flow. Test asserts the old code no longer pairs after invalidation. - routing.rs / connected_integrations.rs (C6): the transient-failure fallback read the cache via `cached_active_integrations`, which returns None past the 60s TTL — so a backend blip landing just after expiry still collapsed the delegation surface to empty and dropped tool-calling. Add `cached_active_integrations_including_expired()` (TTL-tolerant) and use it only for the fallback; `cached_active_integrations` keeps its freshness semantics for other callers. Test seeds an expired entry: the fresh read drops it, the fallback read serves it.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/channels/bus.rs (1)
307-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an allowlist instead of a denylist for progressive-UI support.
channel_supports_progressive_uionly excludes"discord"and defaults every other/future channel totrue. This is the same failure mode being fixed here: a new channel adapter lacking edit+delete support will silently re-introduce the placeholder-spam bug until someone remembers to add it to this denylist. An allowlist of channels known to support edit+delete (or a capability flag sourced from each provider) would fail safe for unknown/new channels.🤖 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/openhuman/channels/bus.rs` around lines 307 - 324, `channel_supports_progressive_ui` is currently a denylist that only rejects `discord`, so unknown or future providers will incorrectly default to enabling progressive UI. Update this logic to use an allowlist of channels that are confirmed to support both edit and delete, or derive the decision from provider capabilities instead of hardcoding a negative check. Keep the provider-prefix handling in `channel_supports_progressive_ui` and ensure any new adapter must explicitly opt in.src/openhuman/composio/connected_integrations.rs (1)
133-174: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded staleness for the fallback read — consider a monitoring/second-tier bound.
cached_active_integrations_including_expiredwill serve a snapshot of arbitrary age (hours, days) if Composio remains unavailable for an extended outage, or if the process/session simply idles with no fresh fetch. The doc comment on Lines 123-126 states the stale window is bounded by the outage, not by this call, which is true, but nothing here caps how long a stale "connected" snapshot keepsdelegate_to_integrations_agentbelieving a toolkit is usable if the user actually disconnected it during the outage.This is a reasonable resiliency trade-off given the described symptom (empty-set collapse), so not blocking. Consider emitting a metric/log warning when
age_msexceeds some secondary ceiling (e.g., several multiples ofCACHE_TTL) so very old fallback reads are observable in production.🤖 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/openhuman/composio/connected_integrations.rs` around lines 133 - 174, The fallback cache read in read_cached_integrations currently allows arbitrarily old snapshots when allow_expired is true, so add an observability guard there by emitting a warning/metric when cached_at age exceeds a secondary ceiling (for example, several times CACHE_TTL). Use the existing tracing::trace/tracing path in read_cached_integrations and the age_ms calculation to locate the logic, and keep cached_active_integrations_including_expired behavior unchanged except for the added alerting.
🤖 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.
Nitpick comments:
In `@src/openhuman/channels/bus.rs`:
- Around line 307-324: `channel_supports_progressive_ui` is currently a denylist
that only rejects `discord`, so unknown or future providers will incorrectly
default to enabling progressive UI. Update this logic to use an allowlist of
channels that are confirmed to support both edit and delete, or derive the
decision from provider capabilities instead of hardcoding a negative check. Keep
the provider-prefix handling in `channel_supports_progressive_ui` and ensure any
new adapter must explicitly opt in.
In `@src/openhuman/composio/connected_integrations.rs`:
- Around line 133-174: The fallback cache read in read_cached_integrations
currently allows arbitrarily old snapshots when allow_expired is true, so add an
observability guard there by emitting a warning/metric when cached_at age
exceeds a secondary ceiling (for example, several times CACHE_TTL). Use the
existing tracing::trace/tracing path in read_cached_integrations and the age_ms
calculation to locate the logic, and keep
cached_active_integrations_including_expired behavior unchanged except for the
added alerting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b3afbd2-2256-4da0-b33f-e07b4a97f6d6
📒 Files selected for processing (9)
src/openhuman/channels/bus.rssrc/openhuman/channels/providers/telegram/channel_recv.rssrc/openhuman/channels/runtime/dispatch/routing.rssrc/openhuman/composio/connected_integrations.rssrc/openhuman/composio/mod.rssrc/openhuman/composio/ops/mod.rssrc/openhuman/composio/ops_tests.rssrc/openhuman/security/pairing.rssrc/openhuman/security/pairing_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/channels/runtime/dispatch/routing.rs
- src/openhuman/channels/providers/telegram/channel_recv.rs
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Review — Telegram /start pairing + keep channel tool-calling alive
Traced the Telegram unauthorized/receive path (handle_unauthorized_message), the pairing guard, and the Composio connected-integrations fallback in dispatch routing. The core changes are well-reasoned and well-tested at the unit level. Two security/hardening items around the /start onboarding path are worth addressing before merge; the Composio and progressive-UI changes are individually sound.
Changed-files summary
| File | Change | Assessment |
|---|---|---|
telegram/channel_recv.rs |
/start first-run onboarding, shared approve_and_persist_sender, allowlist_is_empty, new prompt copy |
Correct, but onboarding gate lacks a chat-type guard (see #1) |
telegram/channel_core.rs, text.rs |
is_start_command, /start constant |
Good |
security/pairing.rs (+tests) |
invalidate_code() |
Correct; idempotent; closes the /bind window |
channels/bus.rs |
channel_supports_progressive_ui gate |
Works; denylist shape is fragile (#3) |
composio/connected_integrations.rs, mod.rs, ops/mod.rs (+tests) |
expiry-tolerant cache read | Solid, well-tested |
channels/runtime/dispatch/routing.rs (+tests) |
status-aware fetch + connected_with_fallback |
Correct fix for the "no tool calling" symptom |
Findings (prioritized)
1. 🔴 Major (security) — /start onboarding is not restricted to private chats; a group member can hijack operator ownership
channel_recv.rs:331-361 onboards the first /start sender whenever pairing.is_some() && allowlist_is_empty(), with no is_group_message/chat-type guard. If the operator adds the still-unpaired bot to a group during setup (a common flow), the unauthorized prompt at channel_recv.rs:451-458 literally instructs every group member "If you're the operator, send /start to finish connecting your bot" — so the first member who does so is allowlisted as operator and can drive the bot. Operator setup for a self-bot-token is inherently a DM action.
if self.pairing.is_some()
&& self.allowlist_is_empty()
&& !Self::is_group_message(message) // ← onboard from private chats only
&& text.map(Self::is_start_command).unwrap_or(false)
{…and gate the "send /start" hint (channel_recv.rs:451-458) on !is_group_message too, so the bot doesn't advertise the hijack in a group. is_group_message already exists and is tested.
2. 🟠 Major (security / design) — /start removes the shared-secret requirement for onboarding (first-sender-wins TOFU)
Previously onboarding required /bind <code>, where the code is a secret printed only to core stdout. /start onboarding needs no secret — the first party to message the bot becomes operator. This is sanctioned by #4381, but it is a real trust downgrade: anyone who learns or guesses the bot's @username before the operator's first message can claim ownership, and there is no rate-limit/lockout on /start (unlike /bind, which goes through try_pair's lockout). Suggestions, in order of value:
- Combine with #1 (private-chat only) — removes the group attack surface entirely.
- Bound onboarding to a setup window (e.g. accept
/startonboarding only within N minutes of process start), so a stale, world-reachable bot doesn't stay claimable indefinitely. - At minimum, document the guessable-username / first-sender risk near the onboarding block.
(Confidence: medium — the base behavior is a product decision from #4381; #1 is the concrete, low-cost mitigation.)
3. 🟡 Minor — progressive-UI capability is a denylist, not an allowlist
channel_supports_progressive_ui (bus.rs:318) returns provider != "discord", so any new/unknown adapter defaults to progressive UI and can silently re-introduce the exact placeholder-spam bug this PR fixes. Prefer an allowlist of providers confirmed to support edit+delete (telegram/tg), failing safe for unknown channels:
let provider = channel.split(':').next().unwrap_or(channel);
matches!(provider, "telegram" | "tg")(Matches CodeRabbit's nit — I agree with it.)
4. 🟡 Minor (tests) — the onboarding decision is untested; only the helpers are
New tests cover is_start_command, bindable_identity, allowlist_is_empty, and invalidate_code — but not the integrated branch in handle_unauthorized_message: that a /start in pairing mode allowlists the sender and invalidates the code, that a second /start after onboarding falls through to the prompt (no double-onboard), and (per #1) that a group /start does not onboard. Given the security sensitivity and the 80%-changed-line coverage gate, add a focused test. self.send() hits the network, so a seam may be needed; even asserting the allowlist/pairing state transitions (without asserting the sent text) would cover the decision.
5. 🔵 Nitpick — unbounded staleness in the fallback cache read
cached_active_integrations_including_expired will serve a snapshot of arbitrary age during a long outage. Reasonable trade-off (empty-set collapse is worse), but consider a warn! when age exceeds a secondary ceiling (e.g. 5 * CACHE_TTL) so very old fallbacks are observable. (CodeRabbit noted this too.)
6. 🔵 Nitpick — scope
The Composio-routing and progressive-UI changes are logically independent of the Telegram approval fix (CodeRabbit's out-of-scope check). Each is sound on its own; splitting would give cleaner revertability, or link them to a separate issue in the PR body.
Looks good
connected_with_fallbackcorrectly distinguishesAuthoritative(empty)(taken verbatim — respects a real disconnect) fromUnavailable/timeout (falls back to the last snapshot). This is the right fix for the "just normal inference, no tool calling" symptom, and the four-case test matrix covers it.- Updates are processed sequentially in the poll loop (
channel_ops.rs:404-446), so the first/startcompletes and populates the allowlist before the next update — no concurrent double-onboard within an instance. invalidate_code()is idempotent and correctly consumes the stdout code on the/startpath so it can't be replayed via/bindafterward;allowlist_is_empty()is fail-closed on a poisoned lock;bindable_identityprefers the immutable numeric id.- No secret/token values are logged (only "code invalidated" and the allowlisted identity, consistent with existing logging).
Questions
- Is onboarding via a group chat an intended flow, or should it be private-only (#1)?
- Should
/startonboarding be time-boxed to process startup, or is an indefinitely-claimable un-paired bot acceptable for the desktop use case (#2)?
Read-only review — no changes were applied.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Requesting changes on the two security items below (full walkthrough in my detailed review comment above). The Composio fallback and progressive-UI changes are individually sound.
Must address
1. 🔴 /start onboarding is not restricted to private chats — group members can hijack operator ownership.
channel_recv.rs:331-361 onboards the first /start sender on pairing.is_some() && allowlist_is_empty() with no chat-type guard, and the unauthorized prompt (channel_recv.rs:451-458) tells every group member "send /start to finish connecting your bot." If the un-paired bot is added to a group during setup, the first member to /start becomes operator.
if self.pairing.is_some()
&& self.allowlist_is_empty()
&& !Self::is_group_message(message) // onboard from private chats only
&& text.map(Self::is_start_command).unwrap_or(false)
{Gate the "send /start" hint on !is_group_message too. is_group_message already exists and is tested.
2. 🟠 /start first-sender-wins TOFU has no secret and no rate-limit.
/bind <code> required the stdout secret and went through lockout; /start requires neither. Anyone who guesses the bot @username before the operator's first message can claim it. Combine with #1 (private-only) and consider bounding onboarding to a setup window after process start; at minimum document the risk.
Should address
- Tests: add a test for the onboarding decision in
handle_unauthorized_message(onboard + invalidate, second-/startfall-through, group/startdoes not onboard) — currently only the helpers are covered, against the 80%-changed-line gate. - Denylist → allowlist:
channel_supports_progressive_ui(bus.rs:318) should allowlist edit+delete-capable providers (telegram/tg) so new adapters fail safe.
Details, non-blocking nits (stale-cache observability, scope split), and the looks-good list are in the comment above.
…ng + hardening Security review on tinyhumansai#4414: - channel_recv.rs: gate the /start onboarding AND the "send /start" hint on !is_group_message. Previously, if the un-paired bot was added to a group during setup, the prompt told every member "send /start to finish connecting" and the first to do so was allowlisted as operator. Operator setup is a private/DM action; a group /start now falls through to the normal approval prompt. Documented the residual first-sender-wins TOFU risk (no secret, no rate-limit). - channel_tests.rs (+ #[cfg(test)] api_base seam in channel_core.rs): test the onboarding decision — private /start onboards + consumes the code; group /start does not onboard (send() aimed at a dead port, no network). - bus.rs: channel_supports_progressive_ui is now an allowlist (telegram|tg) so a new/unknown adapter fails safe instead of re-introducing the placeholder spam. - connected_integrations.rs: warn! when the transient-failure fallback serves a snapshot older than 5x CACHE_TTL (observability).
|
Thanks @sanil-23 — all addressed in 37bd9f7: Must
Should Nit On scope (#6): agreed the Composio-routing and progressive-UI changes are independent of the Telegram approval fix — happy to split them into a follow-up PR if you'd prefer; each is self-contained. CI note: the earlier |
coderabbitai
Bot
removed
the
agent
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/channels/providers/telegram/channel_core.rs (1)
151-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate command-parsing logic with
extract_bind_code.Both
is_start_commandandextract_bind_code(lines 141-149) parse the leading whitespace-delimited token and strip an@botnamesuffix identically. Consider extracting a shared helper, e.g.fn base_command(text: &str) -> Option<&str>, to avoid the two implementations drifting.♻️ Proposed refactor
+ fn base_command(text: &str) -> Option<&str> { + let command = text.split_whitespace().next()?; + Some(command.split('@').next().unwrap_or(command)) + } + pub(crate) fn extract_bind_code(text: &str) -> Option<&str> { - let mut parts = text.split_whitespace(); - let command = parts.next()?; - let base_command = command.split('@').next().unwrap_or(command); - if base_command != TELEGRAM_BIND_COMMAND { + let base_command = Self::base_command(text)?; + if base_command != TELEGRAM_BIND_COMMAND { return None; } - parts.next().map(str::trim).filter(|code| !code.is_empty()) + text.split_whitespace() + .nth(1) + .map(str::trim) + .filter(|code| !code.is_empty()) } pub(crate) fn is_start_command(text: &str) -> bool { - let Some(command) = text.split_whitespace().next() else { - return false; - }; - let base_command = command.split('@').next().unwrap_or(command); - base_command == TELEGRAM_START_COMMAND + Self::base_command(text) == Some(TELEGRAM_START_COMMAND) }🤖 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/openhuman/channels/providers/telegram/channel_core.rs` around lines 151 - 165, `is_start_command` duplicates the same leading-token and `@botname` stripping logic already used by `extract_bind_code`, so the two paths can drift. Extract the shared parsing into a small helper (for example, a `base_command`-style function) and have both `is_start_command` and `extract_bind_code` call it to keep command parsing consistent and centralized.
🤖 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.
Nitpick comments:
In `@src/openhuman/channels/providers/telegram/channel_core.rs`:
- Around line 151-165: `is_start_command` duplicates the same leading-token and
`@botname` stripping logic already used by `extract_bind_code`, so the two paths
can drift. Extract the shared parsing into a small helper (for example, a
`base_command`-style function) and have both `is_start_command` and
`extract_bind_code` call it to keep command parsing consistent and centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6825aa4c-763a-4f88-90af-17661d636ae0
📒 Files selected for processing (5)
src/openhuman/channels/bus.rssrc/openhuman/channels/providers/telegram/channel_core.rssrc/openhuman/channels/providers/telegram/channel_recv.rssrc/openhuman/channels/providers/telegram/channel_tests.rssrc/openhuman/composio/connected_integrations.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/channels/bus.rs
- src/openhuman/composio/connected_integrations.rs
- src/openhuman/channels/providers/telegram/channel_recv.rs
|
The failing jobs on the latest run — Both fail to compile with the same errors, in files this PR never touches:
Fix: update the branch with Separately: the earlier |
Resolves a semantic conflict introduced by upstream tinyhumansai#4399 ("finish TinyAgents harness migration"): that change retired `SqlRunLedgerCheckpointer` (delegation.rs already moved to the crate's `SqliteCheckpointer`) but left dangling references, so `main` at c43f796 does not compile on its own: - orchestration/graph/mod.rs — imported + constructed the removed `SqlRunLedgerCheckpointer`; now opens `SqliteCheckpointer` against a dedicated `graph_checkpoints.db` under `workspace_dir`, matching the delegation-graph pattern. - orchestration/ops.rs — test used the removed checkpointer; swapped to `SqliteCheckpointer::open` at the same path/thread. - agent_registry/agents/loader.rs — `graph_fn` became `Option<fn() -> AgentGraph>`; the frontend/reasoning builtins were left as bare fn items (E0308). Wrapped in `Some(...)`. - orchestration/graph/state.rs — fixed the stale intra-doc link. Merge is otherwise conflict-free. Full gate green: fmt, clippy -p openhuman, lib check, checkpointer persistence test, and the channels suite.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Re-review — all requested changes addressed ✅
Reviewed the delta since 07c56655a (commit 37bd9f762). Every item from my request-changes is resolved cleanly:
1. 🔴 Group-hijack — fixed. channel_recv.rs onboarding gate now has && !Self::is_group_message(message), and the /start hint is likewise gated on pairing_code_active() && !Self::is_group_message(message), so a group /start falls through to the generic approval prompt and the bot no longer advertises the hijack in a group. Exactly the fix suggested.
2. 🟠 First-sender-wins TOFU — documented. A SECURITY (first-sender-wins TOFU) block now spells out the no-secret/no-lockout tradeoff, that the private-chat guard removes the group surface, and that a startup time-window is a reasonable future hardening. Acceptable given #4381.
3. 🟡 Denylist → allowlist — fixed. channel_supports_progressive_ui is now matches!(provider, "telegram" | "tg"), and the test asserts slack/whatsapp:123 fail safe. Unknown/new adapters now suppress placeholders by default.
4. 🟡 Onboarding-decision test — added. start_onboarding_is_private_only_and_consumes_the_code uses a clean #[cfg(test)] seam (set_api_base_for_tests → dead local port so send() fast-fails) to exercise the real decision: a private /start onboards and consumes the one-time code, while a group /start onboards no one and leaves the code intact. Correct — the assertions target the runtime allowlist/pairing state, which is set before the (failing) send, so the test is deterministic.
Rust Quality (fmt, clippy) is green, so it builds and lints; the composio fallback and progressive-UI changes remain sound. The residual non-blocking nits (stale-cache observability warn, scope split) are fine to defer.
Approving. Nice, precise turnaround.
The process-global runtime-snapshot cache (`RUNTIME_SNAPSHOT_CACHE`) held a single entry with no config identity, so a snapshot built for one config was served to any caller within the 10s TTL. In the app_state e2e suite this made `app_state_snapshot_degrades_runtime_service_status_failures` fail deterministically: ~15 prior `app_state_snapshot` tests populate the cache, and the target test's 2.1s age-out sleep is far short of the 10s TTL, so it read a stale foreign runtime instead of its injected `OPENHUMAN_SERVICE_MOCK` failure state. Latent on main; surfaced once the build was repaired so Rust E2E runs. Key the cache entry by `config.workspace_dir` and treat a key mismatch as a miss (rebuild against the requesting config). Production behaviour is unchanged — one config per process means a steady cache hit within the TTL — while a second config or an E2E harness with a unique workspace now gets a fresh build. Drops the obsolete 2.1s sleep from the e2e test and adds a unit test for the key-mismatch miss.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/app_state/ops.rs (1)
764-802: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-workspace sharding would avoid cross-workspace contention
If one process can serve multiple workspaces concurrently, the current process-wide rebuild gate and single cache slot will serialize unrelated snapshots and churn the cache between configs. Keying both byworkspace_dirwould isolate those workloads.🤖 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/openhuman/app_state/ops.rs` around lines 764 - 802, The runtime snapshot cache and rebuild gate are process-wide, so unrelated workspaces contend and overwrite each other. Update the logic around fresh_cached_runtime_snapshot and build_runtime_snapshot to shard both the cache entry and the rebuild lock by Config.workspace_dir (or an equivalent workspace key) so each workspace gets its own snapshot lifecycle. Keep the existing TTL and single-flight behavior, but scope the lookup, coalescing, and stored RuntimeSnapshot per workspace instead of globally.
🤖 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.
Nitpick comments:
In `@src/openhuman/app_state/ops.rs`:
- Around line 764-802: The runtime snapshot cache and rebuild gate are
process-wide, so unrelated workspaces contend and overwrite each other. Update
the logic around fresh_cached_runtime_snapshot and build_runtime_snapshot to
shard both the cache entry and the rebuild lock by Config.workspace_dir (or an
equivalent workspace key) so each workspace gets its own snapshot lifecycle.
Keep the existing TTL and single-flight behavior, but scope the lookup,
coalescing, and stored RuntimeSnapshot per workspace instead of globally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcc37af8-6d31-4d08-b35b-b211be35f3af
📒 Files selected for processing (3)
src/openhuman/app_state/ops.rssrc/openhuman/app_state/ops_tests.rstests/config_auth_app_state_connectivity_e2e.rs
The orchestration wake-graph agents frontend_agent and reasoning_agent were registered in BUILTINS but missing from RESOURCE_CATALOG, so the catalog_mirrors_builtins parity test failed and cascaded into the PR CI Gate. Add a PromptResource for each, serving its bundled prompt.md, which restores catalog<->BUILTINS parity (39 == 39) and lets MCP clients read their prompt templates.
# Conflicts: # src/openhuman/orchestration/graph/mod.rs # src/openhuman/orchestration/ops.rs
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed the channel changes end-to-end and resolved the merge conflict with main.
Merge conflict (orchestration/graph/mod.rs, orchestration/ops.rs): both the PR and main independently completed the TinyAgents SqliteCheckpointer migration but diverged on the checkpoint DB filename. Took main's canonical orchestration_graph_checkpoints.db — the PR's graph_checkpoints.db would have collided with the delegation graph's checkpointer file (agent_orchestration/delegation.rs), mixing two checkpointer schemas in one SQLite file. main's dedicated filename is correct.
Review — all prior review threads are resolved and the logic holds up:
connected_with_fallback(routing.rs) correctly takes an authoritative empty set at face value while falling back to the (TTL-tolerant) cached snapshot on transient failure/timeout — full test matrix present./startoperator onboarding (channel_recv.rs) is tightly gated: pairing-armed + empty allowlist + private chat only + consumes the one-time code viainvalidate_code(). The residual first-sender-wins TOFU risk is documented.channel_supports_progressive_ui(bus.rs) is a fail-safe allowlist (telegram|tg), so unknown adapters won't re-introduce the Discord placeholder spam.
LGTM. Approving; will merge once required checks are green.
…er merge The merge with main silently duplicated the frontend_agent and reasoning_agent PromptResource entries in RESOURCE_CATALOG: both this branch and main independently added them with identical uri/name/content but slightly different descriptions, so git kept both insertions instead of collapsing them. That broke two parity tests: - all_catalog_uris_are_unique (duplicate agent URIs) - catalog_mirrors_builtins (41 catalog agent entries vs 39 BUILTINS) Keep main's canonical entries (split-brain wording) and drop the redundant tiny.place-wording copies. Catalog agent entries back to 39 == BUILTINS.len(); URIs unique again.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Re-approving on the merge-resolved head (e64b97c). Resolved the conflict with main (took main's canonical orchestration_graph_checkpoints.db checkpointer path in orchestration/graph/mod.rs + ops.rs) and fixed a silent semantic-merge duplicate in mcp_server/resources.rs (both branches added the frontend/reasoning_agent catalog entries with different descriptions, breaking all_catalog_uris_are_unique and catalog_mirrors_builtins). Catalog agent entries back to 39 == BUILTINS.len(), URIs unique. Underlying channel changes reviewed and LGTM.
3 participants
Summary
/startnow pairs them when first-run pairing is pending, so the "own bot token" path actually replies instead of looping on the approval prompt./start//bind <code>guidance.delegate_to_integrations_agentstays available and channel messages can tool-call (Discord + Telegram).Problem
resolve_target_agentwrapped the Composio fetch in a 3s timeout and mapped both the timeout and anUnavailableresult to an empty integration set. An empty set dropsdelegate_to_integrations_agentfrom the turn, so a single transient blip left channel turns doing plain inference with no tool calling.Solution
handle_unauthorized_message, whenpairing.is_some()and the runtime allowlist is still empty, treat the sender's/startas the operator's explicit setup signal: allowlist them (runtime + persisted) and let their subsequent messages reach the agent — the "first sender after /start" behaviour bug: Telegram bot token — operator approval prompt repeats on every message and multiplies, no agent reply (staging regression) #4381 sanctions. The guard is tight: an explicitly-configured allowlist (pairing = None) never auto-approves a stranger's/start, and the window closes once the first sender is bound. Factored a shared approve/persist/ack helper so/startand/bind <code>stay in lock-step.dispatch/routing.rs, switch tofetch_connected_integrations_statusand, onUnavailable/timeout, fall back tocached_active_integrations(the same defence the first-party turn path uses) instead of an empty vec. Only an authoritative empty set collapses the delegation surface. Decision extracted into a pureconnected_with_fallbackhelper with unit tests.Submission Checklist
is_start_command/bindable_identity/allowlist_is_emptyguards;connected_with_fallbackmatrix (authoritative/unavailable/timeout/no-cache).pnpm test:rustgreen, 1208 channel tests pass). Frontend (Vitest) coverageN/A: noapp/changes.N/A: behaviour-only change## Related—N/A: behaviour-only changeN/A: no release-cut surface changeCloses #NNNImpact
Related
/startlinking, link-token TTL, Discord DM-in-group); always-bill channel inference (pending live verification).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/channels-telegram-discord-ohfbca4be5be3e71ec54689f37b205059f317b7d92Validation Run
pnpm --filter openhuman-app format:check— N/A: noapp/changespnpm typecheck— N/A: no TS changescargo test --lib channels(1208 pass),connected_fallback+ new telegram guardscargo fmt --checkclean;cargo clippy --libclean on changed filesBehavior Changes
/startonboards the operator; channel turns keep tool delegation across transient Composio failures.Summary by CodeRabbit
New Features
/start(including/start@...and whitespace variants) to complete pairing.Bug Fixes
/start.Performance / UX
Tests
/start, onboarding rules, cache fallback/TTL behavior, progressive UI gating, and cache isolation.