Gateway: Telegram approval channel — approve from the phone - #409
Merged
Merged
Conversation
- Pin scrub's catch-all directly: ureq::Error::BadUri/RequireHttpsOnly/ ConnectProxyFailed embed the request URI verbatim in their own Display, and the mock-server tests never exercised that arm. - Test strip_token with a description that actually contains the token (the existing "Bad Request" fixture didn't). - Floor ureq's own log target to info (gateway/mod.rs log_filter_from) so an operator's RUST_LOG=trace can never make ureq print the bot-token- bearing request path via the log-crate bridge. - Bound MockServer's port-wait loop with a 5s deadline, matching the daemon_client.rs precedent it mirrors, instead of spinning forever if the bind never lands. - Add a debug_assert! pinning get_updates' timeout_secs to the documented long-poll ceiling instead of only a prose contract.
Thread channel identity through approval resolution so the gateway audit log records which channel (http/native/telegram) answered a pending approval, instead of hardcoding channel: None. - approval.rs: new ResolvedVia enum (Http/Native/Telegram); resolve() and the oneshot payload/WaitOutcome now carry it alongside Decision. - approval_routes.rs: /approvals HTTP resolve tags ResolvedVia::Http. - approval_native.rs: the native macOS dialog channel tags ResolvedVia::Native via a pulled-out resolve_as_native helper, so the channel tag itself is directly test-callable without driving a real osascript dialog. - server.rs: policy_gate/await_approval propagate an Option<ResolvedVia> alongside Decision; record_audit's channel: None hardcode is replaced with a real per-call-site value across every #[tool] handler. New brain_get test pins the channel at a second (non-brain_capture) record_audit call site. Gateway PR 5, Task 3. Review fix wave: policy_gate/await_approval doc comments now describe the post-refactor arities and where `channel` originates; Resolution's doc no longer implies it's absent from the public surface it actually appears in.
…me edits Adds TelegramChannel (telegram.rs): fire() sends a pending approval as a Telegram message with Approve/Deny inline buttons and remembers its message_id + text; note_outcome() edits that message to show the outcome (approved/denied/expired) with the original summary, removing the keyboard, and is at-most-once per approval. Both are fire-and-forget (spawn_blocking, dropped JoinHandle) so they never block await_approval's hot path. GatewayState gains telegram: Option<Arc<TelegramChannel>>, built once at startup iff telegram::is_available. await_approval fires it alongside the native dialog and calls note_outcome from all three WaitOutcome arms (approve/deny/timeout), regardless of which channel actually resolved the approval, so a Telegram prompt never outlives its own approval. Also fixes an unverified claim from Task 1: editMessageText does NOT clear an existing inline keyboard merely by omitting reply_markup on the real Bot API (the docs ship a separate editMessageReplyMarkup method for exactly this reason). edit_message_text now sends an explicit empty reply_markup on every edit, closing the same stale-control bug class PR 4 already fixed for the native dialog.
…on, framing
F1 (blocking): TelegramChannel.sent had no expiry sweep, so a cancelled
tool-call future (client disconnect during the up-to-300s wait) leaked an
orphan entry — and its message's live Approve/Deny buttons — for the rest
of the process, since note_outcome is reachable only through
await_approval's three WaitOutcome arms, none of which a cancelled future
ever runs. fire now carries pending.expires into the stored entry and
retains only unexpired ones on every successful insert, mirroring
Approvals::register's identical precedent; this also closes the
send/resolve race (an orphan from that race carries the same expiry and is
swept the same way) at no extra cost. Corrected the doc comment that
previously claimed a one-to-one guarantee the code never actually held.
Also, from the same review round:
- F3: debug_assert! that callback_data payloads stay within Telegram's
64-byte limit (mirrors the LONG_POLL_CEILING_SECS precedent).
- F4: pin the two previously-untested outcome strings ("Denied via {via}",
the timeout wording) with new server.rs integration tests, alongside a
shared fixture for the three Telegram outcome-wording tests.
- F5: correct a doc overclaim — state.telegram is resolved once at
construction, not re-evaluated live like approval_native::is_available()
— and explain why that's fine (gateway.yml isn't re-read after startup).
- F6: stop logging full approval ids in the two new tracing::warn!s; log
an 8-character prefix instead, matching this crate's existing
cleartext-logging convention for mint_secret_32 values.
- F7: fix a test doc comment that described resolution as going through
the operator /approvals HTTP surface when the test actually resolves
out-of-band via state.approvals.resolve, the established pattern every
other approval test in this file already uses.
- F8 (product call): the Telegram prompt now carries the same Client/Tool
framing the native dialog already renders above its summary, so a
Telegram approver — often away from the machine the native dialog would
pop on — can see which connected client is asking.
- Nit: fire's own test now asserts exactly one request was sent, not just
at least one.
…inline buttons Gateway PR 5, Task 5: TelegramChannel::ensure_polling gives ResolvedVia::Telegram its first production caller. A single named "tg-approval-poll" thread, spawned on demand via a compare_exchange-guarded AtomicBool (Drop-guard reset so a panic can't wedge it), long-polls getUpdates, validates each callback (from_id must match the configured chat_id; callback_data must parse as "a:<id>"/"d:<id>"), resolves the matching pending approval, and answers the button. The getUpdates offset persists to ~/.onebrain/gateway/telegram.offset (0700/0600, home resolved via crate::home only) so a restart doesn't replay old updates. The thread exits once a poll cycle observes no pending approvals left. server::await_approval nudges the poller right after firing the Telegram prompt. Fixes two pre-existing hazards found while wiring this in: - A MutexGuard temporary in a match scrutinee was held across an .await in the test mock handler (non-Send future); relocated to its own `let` binding. - Two independent crate::test_env::set_var/set_vars calls on the same thread self-deadlock on that module's non-reentrant lock; every affected fixture and test now makes exactly one combined call. Also: the three pre-existing Telegram outcome-editing tests in server.rs now resolve their asserted request by method name instead of a fixed positional index (background getUpdates traffic can interleave), tighten their asserted outcome strings to the byte-exact "✅ "/"⛔ " glyphs, and redirect $HOME so the now-live poller they trigger never touches the real ~/.onebrain.
Formatting-only follow-up to 8eb0c4d.
…r, expiry filter, token-keyed offset Nine review findings on the demand-driven telegram poller (8eb0c4d), all fixed in this one commit on top of cade428: - F1: closed a lost-wakeup race between poll_loop's exit decision and a concurrent ensure_polling call. The loop now relinquishes `polling` BEFORE its final recheck, and re-acquires it itself if new work showed up in that gap; if a concurrent ensure_polling call already reclaimed it, this thread disarms its own Drop guard and hands off rather than clobbering the replacement thread's ownership of the flag. - F2: added a 1s floor (MIN_CYCLE_INTERVAL) on successful poll cycles, so an instant-answering getUpdates peer can't turn the loop into a hot spin (this module's own test mock reproduced exactly that hazard before the fix). Batched offset persistence to once per getUpdates call instead of once per update, closing an unbounded-disk-write path from an unauthenticated DM burst; documented why either crash-window ordering is safe given Approvals is in-memory per process. Removed the now-redundant 50ms mock delays in both test harnesses. - F3: the exit check now filters by `expires > now`, not bare Approvals::list().is_empty() — an abandoned, unpruned entry can no longer keep the poller alive forever. - F4: is_available now rejects chat_id <= 0 (group/channel ids are negative in Telegram's own convention; this channel only works for a private chat) instead of a silently dead channel. handle_update debug_asserts the callback's own chat_id against the configured one, tolerant of Telegram's legitimate None case. - F5: parse_callback_data_table — direct, mock-free coverage of the callback_data parser. - F6: fixed POLL_TIMEOUT_SECS's doc (25, not the 35s ceiling) and named 25s explicitly in the module doc's linger sentence, per the brief. - F7: server.rs's three Telegram outcome tests now wait out their real poller via a new pub(crate) TelegramChannel::is_polling() test accessor, instead of leaving a detached thread running past the test's own return. - F8: short_id now truncates by char, not byte index, so it can't fall back to logging an entire attacker-controlled string on a non-ASCII boundary — it now also covers remote callback_data, not just locally-minted ids. - F9: the offset file is keyed by a short SHA-256 digest of the bot token (reusing auth/core.rs's existing base64url_nopad, zero new deps), so a token rotation gets a fresh offset file instead of silently inheriting a stale high-water mark that makes getUpdates return nothing. Also: fixture_router_with_mutating_policy_and_telegram now takes `home: &Path` and asserts it against crate::home::home_dir(), so a future caller can't forget to redirect HOME before triggering a live poller. Widened test wait bounds throughout telegram.rs and server.rs to real elapsed-time deadlines, since MIN_CYCLE_INTERVAL means a wait spanning a second poll cycle can now take a couple of real seconds even when nothing is wrong.
…t_id warn, injectable floor Ten review-round-2 findings (2 Important) on top of 87e754f, all fixed in this one commit: - I1: F1's exit/reclaim fix relied on an undocumented invariant borrowed from Approvals::pending's Mutex (a store-buffering/Dekker shape that Release/Acquire alone does not make sound). Promoted every access to `polling` (ensure_polling's initial CAS, poll_loop's exit-path store and reclaim CAS, the Drop guard's store, is_polling's load) to SeqCst, removing the dependency on the mutex's ordering behavior entirely, and documented the reasoning in poll_loop's own doc comment. - I1b: added a probabilistic soak test — hammers register->resolve-> ensure_polling a few hundred times from the test thread against a live poller, then proves the poller is still genuinely alive by routing one final approval through a real scripted callback, wrapped in a bounded timeout so a regression fails loudly instead of hanging. - I2: GatewayState::new now warns when bot_token is set but chat_id is not a positive private-chat id, via a new pure predicate (chat_id_is_misconfigured) tested directly for the two shapes (token set + invalid chat_id -> warn; nothing configured -> no warn) rather than capturing tracing output, which this crate has no existing harness for. - I3a/I3b: ensure_polling_never_double_spawns now waits on 3 observed requests instead of a fixed 3.5s sleep; the cycle floor is now a test-overridable AtomicU64 (set_cycle_floor_ms_for_test) instead of a const, defaulting to production's 1s but dialed down in tests that don't need to pay it. Gateway module suite: 15s -> ~3.4s. Minors, same commit: - M1: ensure_polling's doc no longer claims the offset is persisted per-update before handling (it's batched after, per F2). - M2: corrected the "polling==false is always the very last thing" claim in both wait helpers' docs — false on the reclaim-win path; documented why the tests tolerate the narrow transient-false window anyway. - M3: documented why the cycle floor paces update-carrying cycles too, not just empty ones (an attacker feeding one update per cycle would defeat an empty-only floor). - M4: offset advance now uses the batch's MAX update_id (not the last entry, which trusted sort order) via saturating_add(1), clamped against the offset already on record, so a malformed entry parsing to update_id=0 can no longer drive the persisted offset backward. - M5: replaced the debug_assert! on TgCallback.chat_id with a warn+refuse (same shape as the from_id mismatch) — a debug_assert on remote input let a legitimate operator panic the poller thread under a benign chat-context edge case. - M6: fixed two stale pre-F9 filename references and documented that an orphaned old telegram.offset is simply never read again, harmlessly. - M7: softened the F3 test's cycle-count claim to what's actually guaranteed (at least one live observation before expiry), not a specific count. - M8: documented the thread-spawn-failure residual (an approval stays unpolled until an unrelated nudge) in ensure_polling's own doc comment.
…ring-continuation bug
Two string literals authored via a Python triple-quoted heredoc used Rust's
backslash-newline continuation syntax inside the Python string. Python's own
triple-quoted string parser treats a trailing backslash-newline as ITS OWN
line-continuation escape (removing the backslash+newline but, unlike Rust,
NOT stripping the next line's leading whitespace), so the heredoc's own
indentation ended up baked into the resulting Rust string literal as a run
of ~18 literal spaces:
- server.rs: GatewayState::new's telegram misconfiguration warning
("...is not a positive private-chat id...")
- telegram.rs: the soak test's timeout .expect(...) message
("...it is wedged, which is exactly...")
Fixed both to a single space. Swept crates/onebrain-cli/src/commands/gateway/
for any other run of 10+ spaces inside a string literal (grep -nE) — none
found; the one other backslash-continued string this task added
(fixture_router_with_mutating_policy_and_telegram's assertion message) was
authored directly via the Edit tool, not the Python heredoc, and is unaffected.
Adds `onebrain gateway telegram setup`: paste a BotFather token, press START on the bot in Telegram, and the wizard captures the resulting chat_id and writes gateway.yml's telegram: block (read-modify-write via a raw serde_yaml::Value so unknown keys and other top-level fields survive, and so the write path isn't defeated by TelegramConfig's own skip_serializing on bot_token), then sends a confirmation message. - cli.rs: GatewayVerb::Telegram(TelegramCmd) -> TelegramVerb::Setup - dispatch.rs: wires the new arm to commands::gateway::telegram_setup - commands/gateway/telegram_setup.rs: testable core `run_setup` (takes stdin/stdout, api_base, and the gateway dir as parameters) plus the production `telegram_setup` wrapper; gateway.yml written 0600, its parent dir 0700, mirroring the existing pairing.json / offset-file helpers elsewhere in this module - A bad token surfaces only "token was not accepted by Telegram", never the token itself or the underlying TgError text - The /start wait is budgeted by call count (60s / 5s = 12 getUpdates calls) rather than wall-clock Instant, so the timeout test runs in milliseconds against a mock instead of stalling 60 real seconds - A captured chat_id <= 0 (group chat) is rejected with a clear message telling the user to DM the bot instead
…mbiguous batches, tolerate group messages, retry transport errors H1 (HIGH, review finding): the capture loop took the FIRST message-bearing update in the unconfirmed backlog, not the operator's own /start. A bot's @username resolves publicly the instant BotFather assigns it, so a stranger who messages the bot before the operator presses START could have their chat_id written to gateway.yml instead — and since telegram::handle_update authorizes purely on from_id == chat_id, that stranger would own Approve/Deny for every future gateway tool call. Fixed with three cooperating pieces: - drain_backlog() fetches and confirms any pre-existing backlog BEFORE the "press START" prompt is even printed, so anything the operator goes on to send lands strictly after this point. - The wait loop only captures when a batch names exactly one distinct private (chat_id > 0) chat; a batch naming more than one aborts with an error naming the count rather than silently picking one. A group message (chat_id <= 0) is skipped and the wait continues rather than terminating the wizard; a timeout having seen only group messages reports a group-specific hint instead of the generic timeout message. - Every batch consumed (including, best-effort, the one that resolved the capture) is confirmed server-side via a follow-up getUpdates call carrying the advanced offset, so nothing can resurface on a later run. M1: added a direct test that write_telegram_config preserves unrelated existing gateway.yml keys (port/vaults/policy) across the read-modify- write. L1: get_updates_with_retry wraps every getUpdates call with up to 3 attempts (2s backoff, mirroring poll_loop's own POLL_ERROR_BACKOFF) on a transport failure, so one network blip during the 60s wait doesn't force re-entering the token. Still propagates the real error once retries are exhausted rather than reinterpreting it as a timeout. L2: dedicated test for the group-chat-only timeout path, asserting the private-chat wording in the error message. L3: a post-write send_message failure now restates the saved chat_id in its error message. 7 new tests (10 total in this file). Whole gateway suite: 345 passed (was 338), 3.38s. Full crate suite: 1819 passed (was 1812).
…lti-page drain, transport-scoped retry
C1 (Critical): drain_backlog fetched exactly one page, then made a second
"confirming" call and discarded its response. With Telegram's default
getUpdates page size (100), a 101+ message backlog left everything from
update 101 onward both undrained AND unconfirmed, reviving H1 gated only
on backlog size. Fixed by looping getUpdates(offset, 0) until a page
comes back empty (bounded, DRAIN_MAX_PAGES), which both pages through
and confirms in the same operation — no more fetch/confirm split.
RULING: capture is now identity-proven, not order-trusted. run_setup
mints a per-run setup code (mint_pairing_code, the same shape `gateway
pair` already uses — never persisted, never logged) and only accepts a
private message whose text contains it; a group message carrying the
code is skipped (wait continues) rather than treated as a terminal
error, with its own timeout hint. This closes the residual multi-batch
race the previous round's fix only narrowed to one batch, and fixes I2:
an operator who already pressed START (or messaged the bot) before
running the wizard is no longer told to press a button Telegram won't
show them — the drain reports how many earlier messages it cleared
("Cleared N earlier message(s).") so they can see their own message was
accounted for, and the instruction is "send this code" either way.
I1: TgError gained is_transport(), computed in scrub from which
ureq::Error variant produced it (every real variant is transport; the
Other variant this module manufactures itself for an API-level
description or a malformed response is not). get_updates_with_retry
retries only a transport failure, so a revoked token or a 409 Conflict
surfaces immediately instead of burning 3 attempts and 4+ seconds of
sleep first. Added a pure classification unit test in telegram_api.rs.
Also: the from_id == chat_id invariant (the same one
telegram::handle_update will later authorize on) is enforced at
capture, not just trusted; a post-write sendMessage failure restates
the saved chat_id; TgUpdate gained message_text so the wizard can read
what a message actually says.
9 tests added/rewritten this round (12 total in telegram_setup.rs, 1
new pure classification test in telegram_api.rs). Gateway suite: 348
passed (was 345), 3.5s.
…de ordering, fix stale retry text, guard degenerate code
Finding 1: operator-facing text contradicted what the code can actually
capture. Pressing START alone sends only a bare /start with no code, so
message_carries_code can never accept it — the old wording ("Pressing
START also works...") read as sufficient on its own and would stall an
operator who took it literally for the full 60s. Reworded to say press
START first for a brand-new bot, then send the code. Both timeout error
strings named THIS run's {code} as the thing to send after re-running,
but a fresh run mints a different code — following that instruction
guarantees a second failure. Dropped {code} from both, pointing instead
at "the new code it prints"; the group-chat variant's ordering also
changed so re-running comes before sending, not after.
Finding 2: round 2 had inverted the operational order from round 1 —
the code was minted and printed BEFORE drain_backlog ran, contradicting
the module's own H1 doc section ("drain_backlog still runs first").
This reopened a self-inflicted timeout window: an operator who sent the
code promptly, while a still-in-progress drain paged through a chatty
backlog, could have their own live message silently swallowed by a
drain call that never inspects content — then see "Cleared 1 earlier
message(s)" counting their own message, followed by a stall into the
now-self-defeating retry text. Moved the drain block (and its two
writeln!s) back above the code-minting/print block, restoring round 1's
correct order and making the doc true again. Updated the numbered Flow
list in run_setup's own doc comment to match (drain is now step 3,
code-mint is step 4).
Also added the reviewer's hardening nit: message_carries_code's whole
design rests on `contains(&normalize_code(code))` never being
vacuously true for an empty-normalizing code. Unreachable today
(mint_pairing_code always returns 8 alphanumeric characters) but now
guarded by a debug_assert! at the point the code enters the system,
rather than trusted silently.
No test changes needed or made — all 12 telegram_setup.rs tests (which
discover the code dynamically via wait_for_printed_code regardless of
print position) still pass unmodified; re-run 5x clean. Explicitly out
of scope per the coordinator: a DRAIN_MAX_PAGES cap-hit test, a mock
enforcing Telegram's 100-per-page limit, and a get_updates_with_retry
non-transport no-sleep test.
Two capstone tests drive a real spawned `onebrain gateway run` process over real HTTP: OAuth to get a token, brain_capture under ask_once blocks, TelegramChannel::fire sends a real (mock, loopback-only) Bot API prompt with Approve/Deny buttons carrying no raw note body, this test taps one by scripting the poller's next getUpdates response with the matching a:/d: callback, and the resulting decision — approve or deny — is verified end to end: note written or absent, audit line names channel:"telegram", and the mock server saw the outcome edit clearing the inline keyboard. The mock Bot API server is a real axum server bound to 127.0.0.1, mirroring telegram.rs's and telegram_api.rs's own #[cfg(test)] mock fixtures — duplicated per this crate's established per-file mock-server convention, never shared. ONEBRAIN_TELEGRAM_API_BASE points the spawned binary at it, so no packet reaches the real Telegram API from either test.
…it-channel claims docs/gateway.md gains a Telegram approval channel section (setup command, dedicated-bot requirement, config keys, no-pairing-code auth model, message flow, the poller's <=25s linger + token-keyed offset file), updates the approval-channels table and the two "not yet shipped"/"always false" callouts left over from before this epic shipped, and corrects the audit-log table + sample line: Task 3 (this branch) made the `channel` field real for http/native and this work extended it to telegram, so the old "always null today" claim and the impossible `"decision":"approved","channel":null` sample were both wrong. docs/reference/mcp.md's approvals bullet now names the Telegram channel too. CHANGELOG.md gets the Gateway PR 5 entry the unreleased 3.5.0 section was still missing, and fixes the PR 4 entry's now-stale "telegram is not implemented yet ... always reports false" line. Every added/changed #anchor was checked against GitHub's own heading-slug algorithm (ad-hoc script, not checked in) plus scripts/check-links.py.
…issing schema row, stale help text, overstated audit/replay claims
M1: docs/gateway.md's chat_id row claimed a non-positive id "posts
prompts but can never be answered" — is_available() actually returns
false for chat_id <= 0, so GatewayState::new never builds the channel
at all and no prompt is ever sent (contradicted the very next line,
which had this right). Rewritten to describe the real behavior.
M2: added the missing `telegram` row to the gateway.yml schema table,
matching the `policy` row's "see below" pattern.
M3: cli.rs's `gateway telegram setup` help text still described the
pre-nonce design ("press START on the bot") the wizard no longer uses
— a user following it would time out. Updated to name the one-time
code.
L1/L2: CHANGELOG.md and docs/reference/mcp.md overstated the audit
`channel` field ("every line now names which channel resolved a
call") and the offset cursor ("a restart never replays or loses
updates") — both softened to match what the code (and gateway.md's
own correct table row / poll_loop's own doc comment) actually says:
channel is still null for auto/policy-denied/timedout, and a crash
between a persisted batch and the next cycle can replay a few
already-seen (harmlessly no-op) updates. Carried the same replay-claim
fix into gateway.md's own poller paragraph, which had the identical
overstatement.
L3: added gateway_telegram_e2e.rs to the two places
(tests/support/mod.rs, tests/gateway_http.rs) that enumerate the
gateway e2e harnesses sharing the redacted-capture-tail convention.
L4: gateway.md's auth-model paragraph named only the from_id==chat_id
check; handle_update also refuses (same "not authorized" toast) when
the tapped message's own reported chat_id disagrees with the
configured one. Documented both paths.
Also: fixed the same "Client/Tool framing" overstatement (only Tool:
and capture: title= are actually asserted) in
gateway_telegram_e2e.rs's own doc comments, and replaced its one
unresolvable intra-doc link (`[super::telegram::TelegramChannel::fire]`
— this is an integration-test binary with no lib target for that to
ever resolve against) with plain text.
Re-verified after editing: cargo fmt --all --check, both +1.98.0
clippy configs (-D warnings), the gateway module suite (348) and this
file's 2 e2e tests directly, then the FULL workspace suite (1822
passed, unchanged) and the lex-only suite (1816 passed, unchanged) —
confirming this wave is genuinely doc/comment-only, not assumed. Doc
anchors re-checked against GitHub's slug rules and
scripts/check-links.py — both clean. No stray processes after any run.
…stone, wizard restart notice, dead-code allow, yml comment preservation Important 1: a resolution that beat the in-flight sendMessage left the prompt's Approve/Deny buttons live forever. `fire` inserts into `sent` from inside its spawn_blocking closure; `note_outcome` removes synchronously, so an approval resolving first found nothing to edit and the message Telegram delivered a moment later was never cleared. Under the documented-legal `policy.approval_wait_seconds: 0` that was EVERY gated call, not a corner case. `note_outcome` now leaves a SentSlot::Resolved tombstone carrying the outcome text; `fire`'s success path claims it (before the sweep, so age can't lose it), edits immediately, and inserts nothing. The existing expiry sweep covers tombstones too, so they cannot accumulate. Important 2: `gateway run` freezes its Telegram config at startup, but the docs teach a "run this from a second terminal" pattern for `gateway pair`. An operator applying that habit to `telegram setup` got two success messages and a silently dead channel. The wizard now says to restart a running gateway; the doc section says so too. No hot reload — out of scope. Minor 3: removed telegram_api's stale blanket #![allow(dead_code)] (every item has had a production caller since Task 6) so a future deletion of strip_token's call site or a scrub arm can't pass CI in silence, and rewrote the module doc that still described the allow's old coverage. Plus the deferred regression test pinning that an explicit RUST_LOG=ureq=trace is still floored to info — the direct guard for the token-in-URL leak. Minor 4: the wizard no longer silently strips comments from a hand-edited gateway.yml. A file with no telegram: block gets the block APPENDED textually, leaving existing bytes untouched; a file that already has one still round-trips through serde_yaml (in-place key replacement would mean string surgery on YAML) but now reports it, and run_setup prints a note.
… starvation gate The wizard's wait loop is budgeted by CALL COUNT (MAX_WAIT_SECS / WAIT_POLL_SECS = 12), not wall-clock — a deliberate, adjudicated choice that is correct against a real long-polling Telegram. But the test mock answers instantly, so those 12 iterations elapse in well under a millisecond, while wait_for_printed_code only notices the printed code on a 2ms poll. The test thread was therefore racing the wizard for the right to script the code-carrying response, and losing ~5% of the time: the wizard burned its whole budget against default-empty replies and returned "no message carrying the code arrived" before anything was queued. The defect is in the TEST's timing assumption, not the wizard, so the fix is in the harness. MockState gains a starvation gate: while armed, a getUpdates call that finds NOTHING scripted — no queued response and no standing set_response — blocks in the handler instead of being served an empty default. start_run_setup arms it before the wizard thread exists; RunningSetup::join releases it, so release is necessarily ordered after the test has finished queueing and no test can forget to do it. Drain calls are untouched: every start_run_setup test queues its full drain sequence, so they are never starved. The two tests that call run_setup directly (drain-paging, clean-timeout) are unaffected — the gate defaults to off. This removes the race rather than widening its window. The group-skip test's `>= 4` call-count assertion, which existed only to tolerate the race, is now an exact `== 4` — so a regression in the gate fails loudly instead of returning to flaking. Verified on an idle machine with both binaries under identical load: gate disabled 18/20, gate enabled 20/20. Unloaded, the final code is 50/50 (the unfixed code also passes 50/50 unloaded, which is why the load experiment is the discriminating one).
…art notice Two doc-comment blocks landed on the wrong item when a new test helper was inserted between an existing doc block and the item it documented: telegram.rs's is_polling doc merged onto the new sent_len, and telegram_setup.rs's has_comment_line doc merged onto appended_reads_back. Restored each block to its own item. Also: the "restart a running gateway" notice was only printed after a successful confirmation send, so the "config saved, confirmation send failed" path (setup_send_confirmation_failure_restates_the_saved_chat_id) never told the operator to restart. The notice now prints on both paths; extended that test to assert it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase ⑤ of the v3.5 Gateway epic. Approve from the phone.
The gateway could already stop a risky tool call and wait for a human — but only through the operator HTTP surface or a macOS dialog, both of which need you standing at the Mac. That defeats the point of the epic, which is commanding OneBrain from a phone while a Mac runs at home. This adds the third channel: Approve/Deny buttons in Telegram.
Spec:
01-projects/onebrain/shared/2026-08-28-remote-mcp-gateway-design.md§5.3.Design + plan:
01-projects/onebrain/cli/2026-08-29-gateway-telegram-{design,plan}.md.Follows #408 (policy engine, audit log, approvals,
brain_capture).What's in it
A dedicated bot, by necessity. Telegram allows exactly one
getUpdatesconsumer per token, and OneBrain's Claude Code plugin already holds that slot for the existing bot. Sharing it would mean the two long-pollers fight over updates and button presses land at the wrong process. So the gateway gets its own bot — which also gives it a clean identity: this bot does approvals, nothing else.One-command setup.
onebrain gateway telegram setuptakes the BotFather token, validates it, prints a one-time code, waits for you to send that code to the bot, writesgateway.yml, and sends a confirmation. The code is what makes setup safe (below).Demand-driven polling. The poller thread exists only while approvals are pending and exits when the last one resolves. An idle gateway makes no network requests at all. That is also the honest scope: the window where a button press matters is exactly the window where something is waiting for it.
Outcome edits close the loop. Whichever channel answers — Telegram, dialog, HTTP, or a timeout — the original message is edited to "✅ Approved via telegram" / "⛔ Denied via …" / "⏰ Expired" and its buttons removed. A stale control that silently does nothing is the bug class PR 4 spent a fix wave closing for dialogs; it does not get to reappear here.
The audit log finally names the channel.
AuditEntry.channelhad existed since PR 4 with nothing populating it. This threads channel identity through resolution for all three channels, not just the new one — labeling the newcomer while leaving the incumbents anonymous would have been the same consent-visibility gap the epic keeps closing. It staysnullwhere no human answered: policy auto, policy deny, capacity refusal, timeout.Zero new dependencies: the Bot API client is built on the
ureqalready in the workspace.Security
The setup nonce is load-bearing. The first implementation captured the first message-bearing update in the bot's backlog. A bot's username resolves publicly (
t.me/<name>) the moment BotFather assigns it, so any stranger who messaged the bot first would be written into config as the approving chat — and since button presses authorize on exactly that id, they would hold Approve/Deny over every future tool call. Review caught it by writing a throwaway test that proved it rather than by reading.The first fix — drain the backlog before waiting — was itself incomplete: the client never sends a page limit, so Telegram's default of 100 applies, and a 101-message backlog reproduced the hijack exactly. The existing tests could not have caught that, because the mock ignores page limits.
So the model changed rather than the patch: setup now mints a one-time code and accepts only a message carrying it. Arrival order is not proof of identity, and no amount of drain correctness closes the window where a stranger messages during the wait. ~42 bits from a rejection-sampled generator, full-length match, homoglyphs stripped so it fails closed.
Token hygiene. The Bot API URL embeds the token, so every error path routes through a single scrubbing constructor —
TgErrorhas no other way to be built. The token is redacted fromDebug, skipped inSerialize, andureq's log target is floored toinfobecause atRUST_LOG=traceit would print the full request path. The config struct's guarantee is structural, not "nobody happens to format it".Callback authorization is
from_id == chat_id, checked before anything touches the registry — no existence oracle, no timing or offset side channel. Group chat ids are rejected at config load with a startup warning naming the cause, because a negative id would otherwise post prompts nobody could ever answer.Review
Seven tasks, each reviewed on landing; three needed multiple fix rounds. Findings that mattered most:
gateway runinstalled no tracing subscriber at all in PR 4, so every operator diagnostic this PR relies on went nowhere. Fixed there; several PR-4 decisions had been adjudicated "acceptable because the operator sees a warning".Verification
cargo fmt --all --check·cargo +1.98.0 clippy --workspace --all-targets -- -D warningsand the lex-only--no-default-featuresconfig · gateway module 355 · full workspace green ·scripts/coverage.sh --ci-gateat 94.57% against the 94 ratchet.One test flake was found and closed rather than tolerated: the wizard's wait loop is budgeted by call count, so against an instantly-answering mock it could exhaust its budget before the test queued a response — roughly 5% even on an idle machine, and CI runners are never idle. The mock now blocks until the test has queued, with the release tied to the only path that returns the wizard's result, so no future test can forget to arm it.
Clippy is pinned to 1.98.0 deliberately — CI's stable is 1.98.0, and a local 1.97 clean run turned an earlier PR in this series red.
Deferred
Task-completion notifications (needs durable tasks) · multiple chats and group approvals · webhook mode (the daemon is loopback-only; long-poll is the design) · audit-log rotation (a retention question, not a missing size check — the size check is here).
Two follow-ups worth their own issues rather than ledger lines: a mock that generically enforces Telegram's 100-per-page cap — that harness blind spot is what hid the incomplete first fix of the hijack, so it has a proven track record — and the mock Bot API server now duplicated verbatim across four files.