feat: rich UI surface — RichResponse + TransportAdapter.postRich (4-phase migration)#137
Merged
Conversation
Adds the design proposal for moving Telegram inline-keyboard rendering out of gateway command modules and into TransportAdapter.postRich(), with a transport-agnostic RichResponse value type. Captures the topic-session routing rule (transport thread identity must not be conflated with agent session identity).
…e 1) Adds a transport-agnostic 'rich response' surface so command modules can return menu data without ever importing Telegram-specific helpers. - src/transports/types.ts: RichButton / RichMenuSection / RichMenu / RichResponse types; postRich() is now a required TransportAdapter method. - src/transports/telegram/rich-ui.ts: new home for the Telegram callback protocol (CALLBACK_PREFIX + encodeTelegramCallbackData) and the RichMenu \u2192 inline-keyboard renderer. - src/transports/telegram/telegram-adapter.ts: implements postRich(); no menu \u2192 plain HTML; with menu \u2192 inline keyboard via telegramFetch with text fallback on any transport-level failure. - src/gateway/command-registry.ts: invoke()/actions[K] may now return void | RichResponse | Promise<\u2026>. - src/gateway/gateway.ts: dispatch loop and onAction handler forward RichResponse returns to a new postCommandResult() helper which calls transport.postRich(). void returns remain a no-op (legacy path). - src/gateway/inline-keyboard.ts: now a thin @deprecated shim re-exporting from the new rich-ui module so existing callers keep compiling. New gateway code MUST NOT import from here. Phase 1 of docs/rich-ui-surface-design.md. Non-breaking: no command behaviour changed, all 540 tests green.
handleModel + handleModelAction are now pure data builders. They no longer import from src/gateway/inline-keyboard.ts, never touch telegramFetch, and never call thread.post directly. - buildModelMenu() returns a RichMenu with selected-state hint per button. - applyModelSelection() returns a RichResponse with confirmation text. - ModelCommandContext drops 'thread' and 'postWithFallback'; the gateway consumes the returned RichResponse via postCommandResult(). Behaviour-preserving: same alias list, same settings I/O, same console logging, same action id. 540 tests still green.
…hase 3)
handleTopic + handleTopicAction are pure data builders now. They no
longer import from src/gateway/inline-keyboard.ts, never touch
telegramFetch, and never call thread.post directly.
Routing rule (verified, not changed): applyTopicOverride() only rewrites
the *agent-session* id string ('topic:<chatId>:<name>'). The transport
thread object passed into the gateway dispatcher is preserved, with its
.adapter and .id intact. That keeps the menu surface working from inside
a named-topic session by construction \u2014 there is no 'synthetic thread'
constructed in that path. Documented this contract in the module doc.
- Drops postWithFallback from TopicCommandContext.
- buildTopicMenu() returns a RichMenu with the active topic flagged as
selected on its button.
- applyTopicSelection() returns a RichResponse with confirmation text.
- Group-chat rejection becomes a text-only RichResponse.
Regression test added: 'returns a menu when invoked from inside a
named-topic session'. Confirms applyTopicOverride('main', { id: 'telegram:42' })
returns 'topic:42:deploy' AND that handleTopic on the same transport
thread still produces a menu (not a text-only fallback).
541 tests green (was 540).
- handleCrons returns RichResponse for every branch instead of awaiting thread.post / postWithFallback. The descriptor in gateway.ts drops the no-longer-needed postWithFallback dependency. - The transient '\u23f3 Triggering ...' notice in the trigger branch stays as a direct thread.post (eager progress notice); the result of the trigger comes back as the descriptor's RichResponse. - /jobs alias preserved. Search confirms no gateway command file calls telegramFetch directly anymore. The remaining src/gateway/commands.ts import from src/transports/telegram/progress (createProgressMessage, used by /update, /compact, /doctor) is intentional progress-UI infrastructure with an existing TODO to move it onto TransportAdapter when multi-transport lands. That work is out of scope for the rich-UI surface migration \u2014 progress messages are stateful (post + edit + edit \u2026) and not a menu surface; promoting them is a separate design effort. Documented. 541 tests still green.
- test/rich-ui.test.ts: callback encoding round-trip + RichMenu \u2192 Telegram inline keyboard layout (default 2-col, custom columns, multi-section, selected-state hint, empty-menu). - test/telegram-postrich.test.ts: TelegramAdapter.postRich \u2014 sendMessage with inline_keyboard on the happy path; postMessage fallback when telegramFetch is missing; postMessage fallback when telegramFetch throws; never-throws contract when even the text fallback fails; text-only response posts no keyboard. - test/post-command-result.test.ts: Gateway.postCommandResult \u2014 void is a no-op, RichResponse calls transport.postRich exactly once with the same thread + response, awaits the postRich promise, and uses thread.post(text) as a last-ditch fallback if postRich throws. 558 tests green (was 540, +18).
5 findings from codex self-review, all addressed:
1. TelegramAdapter.postRich: hardened the missing-fetch / missing-chat-id
branch \u2014 now routes through a new safePostText() helper that swallows
postMessage errors. postRich now never throws on any error path.
2. test/topic-dispatch-regression.test.ts: new dispatcher-level regression
test that runs the *actual* descriptor.invoke + postCommandResult
chain Gateway uses live, with a named-topic active. Asserts that the
exact same chat-SDK-delivered thread object reaches transport.postRich
(not a clone, not a synthetic { id: 'topic:...' }). Also covers the
action-callback path. The previous test only exercised handleTopic in
isolation and didn't prove the routing rule held end-to-end.
3. src/gateway/commands.ts no longer imports from src/transports/telegram/.
Promoted progress messages to a first-class TransportAdapter method
(TransportAdapter.progress() returning ProgressMessage). CommandContext
now exposes injected from the transport, so /update,
/compact, /doctor are transport-neutral. Gateway.handleContextPressure
uses it too.
4. src/gateway/command-registry.ts: dropped its dependency on the
deprecated ChatThreadLike from inline-keyboard.ts. Replaced with a
local minimal ActionThreadLike interface so the shim has zero live
callers in production code.
5. src/gateway/command-registry.ts: collectAndValidateActions's internal
array now uses the proper ActionHandler type instead of `any`.
560 tests green (was 558, +2 from the new regression test).
4 findings from round 2 codex review, addressed:
1. Regression test now drives the LIVE dispatcher. Extracted the in-turn
command-dispatch step into Gateway.dispatchInTurnCommand() so the
topic-dispatch-regression test calls the same code path handle() uses
in production. Future refactors that swap 'thread' for a synthetic
{ id: agentThreadId } inside the dispatcher will now fail this test
on the .toBe(transportThread) referential-equality check. The previous
version just called desc.invoke directly, which sidestepped the very
coupling we want to lock down.
2. Fixed pre-existing TypeScript scope bug at gateway.ts:529 \u2014
deferredSoftFlush was declared inside an inner try and read after the
enclosing finally, surfacing 'Cannot find name' under tsc --noEmit.
Hoisted to outer scope. (Pre-existing on trunk; drive-by fix because
the file was already touched.)
3. Tightened newly-introduced 'any' types: CommandContext.progress now
takes ChatThread; createProgressMessage now typed at ChatThread (with
a documented narrow at the transport boundary, same pattern as
postRich); the 'thread as any' cast in TelegramAdapter.progress is
gone. The remaining 'thread: any' on Gateway.postCommandResult is
documented as following the rest of the gateway (chat SDK Thread<TRaw>
doesn't flow cleanly through these surfaces yet).
4. Deleted src/gateway/inline-keyboard.ts and test/inline-keyboard.test.ts
\u2014 confirmed no production callers remain. The wire-format the shim
covered (CALLBACK_PREFIX, encodeCallbackData) is fully covered by
src/transports/telegram/rich-ui.ts and test/rich-ui.test.ts.
552 tests green (was 560; -8 from inline-keyboard.test.ts removal,
those tests are now in test/rich-ui.test.ts; the existing dispatcher
regression test was rewritten so total is 552 not 560).
- Gateway.dispatchInTurnCommand: message: any \u2192 message: unknown. thread stays 'any' to match the rest of the gateway (chat SDK Thread<TRaw> doesn't flow cleanly through this seam yet). - src/transports/telegram/progress.ts: telegramFetch's Promise<any> \u2192 Promise<unknown>, narrow the result with a type assertion at the use site instead of the boundary. Codex round-3 finding 3 (552 tests not 552) is a sandbox artefact; local 'npm test' reports 552/552 passing. Codex's environment can't write under ROUNDHOUSE_DIR or bind IPC sockets. Codex finding 8 (legacy thread.post in /new, /restart, /status, /stop, /verbose, /doctor) is intentionally out of scope: the brief migrated /model + /topic + /crons specifically. Other commands keep their existing behaviour (the descriptor return type is now void | RichResponse, so void-returning legacy handlers continue to work unchanged). 552 tests green, tsc clean for the touched paths.
The two commands shared the same 'compute current → render text fallback + RichMenu with selected flag' shape. Hoisted into a transport-neutral helper in src/transports/rich-helpers.ts, with optional sentinel support for /topic's 'main (default)' button. - /model and /topic both delegate to buildSelectableMenu() - Adds test/rich-helpers.test.ts covering selected marker, sentinel active/inactive, text-fallback symmetry, and columns propagation - Existing /model and /topic behaviour unchanged (same actionIds, values, selected semantics; text fallback now uses the helper's standard layout but still mentions all options + current)
Removes the redundant try/catch + thread.post(text) fallback in Gateway.postCommandResult. TransportAdapter.postRich is documented to never throw (TelegramAdapter already degrades internally via safePostText), so the wrapper was both redundant and obscured genuine adapter bugs. - Update TransportAdapter.postRich JSDoc with explicit Precondition note - Update test/post-command-result.test.ts: assert propagation on adapter contract violation (cleaner design per the brief — propagating signals the bug rather than silently swallowing it)
Replaces the `any` types on CommandInvocation with:
- thread: MinimalThread (new export from transports/types) — covers id,
post, optional startTyping; that's all command handlers actually use.
- message: { text?: string; [key: string]: unknown } — loose but typed,
per the brief.
Cascading fixes in gateway.ts:
- withCtx() narrows message to read `author` via a local cast
- dispatchInTurnCommand passes a typed CommandInvocation.message
- TopicThread drops its [key: string]: unknown index sig so MinimalThread
is assignable
No behavioural change. tsc shows no new errors (pre-existing
agent-command/setup/cron/pi-adapter errors unchanged).
Adds test/topic-sentinel.test.ts probing normalizeTopicName() with edge cases (whitespace, casing, surrounding dashes/underscores, special chars) to assert the sentinel '-main' can never be produced by user input. Pins the invariant so a future relaxation of the normalizer's ^-+|-+$ trim rule fails the test loudly.
Adds a JSDoc on RichButton.value explaining why it's a flat string and not an object: Telegram callback_data is 64 bytes max, so the simple encoding survives all transports. If structured payload is ever needed, JSON-encode into the field and decode in the handler. References the MAIN_SENTINEL pattern in topic-command.ts as the canonical sentinel recipe.
Replaces the mixed-paradigm two-message pattern (eager thread.post() "Triggering..." then RichResponse "queued.") with a single transport.progress() handle that updates in place. - CronsContext gains progress: (thread, initialText) => Promise<ProgressMessage> - gateway.ts passes this.transport.progress as the factory - handleCrons returns void after a trigger (progress posted final state); return type widened to Promise<RichResponse | void> - Added a catch branch posting "\u274c <id> failed: <msg>" on scheduler error before re-throwing (keeps existing failure visibility) Adapters without edit support degrade to a single post + no-op updates (per ProgressMessage contract), so this is strictly an improvement on adapters that do support edits (Telegram).
The Telegram inline-keyboard renderer (toTelegramInlineKeyboard) ignores both fields, and after S1 the helper builders no longer set them either \u2014 header text already lives on RichResponse.text where commands actually populate it. When a transport with a real card layout (Slack Block Kit, Discord embeds) lands, structured fields can be added back together with the matching renderer. Until then they're dead data shape.
![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: e5f0dac42e
ℹ️ 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".
…mment + TODO - /crons trigger: log + progress.update(❌) only, don't rethrow. Rethrow caused a second generic error from the gateway catch-all on top of the user-visible ❌ progress edit. Symmetric with the rest of handleCrons returning RichResponse for failures. - model-command: TODO note that buildModelMenu does an alias→label→alias round-trip; cheap with 8 models, but flagged for clarity. - topic-command: tighten TopicThread doc — it's narrower than MinimalThread (id may be absent), not 'compatible with'.
Codex P1 finding on PR #137: postRich() routes missing-handle cases through safePostText() → postMessage(), which throws on any thread that fails isTelegramThread. The catch swallowed the error and the user got nothing — violating the postRich never-throws degradation contract for callback/invocation threads lacking adapter.telegramFetch or a 'telegram:' id shape (could drop /model and /topic confirmations). Three-tier fallback now: 1. postMessage (HTML, native Telegram path) 2. thread.post() if available (generic chat-adapter contract) 3. log + give up (true terminal failure) +1 test asserting tier-2 path: synthetic non-Telegram thread shape with thread.post(); postMessage throws → thread.post is invoked.
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.
Rich UI Surface Migration
Implements the Rich UI Surface design (
docs/rich-ui-surface-design.md) end-to-end. Lifts Telegram inline-keyboard rendering out of gateway command modules and into a transport-agnosticRichResponsevalue type rendered byTransportAdapter.postRich().Net effect: new menu-style commands describe intent (
{ text, menu }) and never import a transport package. Adding Slack/Discord later means implementingpostRich()on a new adapter; no command code changes.What changed
All four phases from the design doc, in commits 2–5:
Phase 1 (
5374f99) — non-breaking abstractionsrc/transports/types.ts: newRichButton/RichMenuSection/RichMenu/RichResponsetypes;postRich()andprogress()are now requiredTransportAdaptermethods.src/transports/telegram/rich-ui.ts: new home for the Telegram callback protocol (CALLBACK_PREFIX,encodeTelegramCallbackData) and theRichMenu → inline_keyboardrenderer.TelegramAdapter.postRich()implemented with a hardened never-throws contract: missing fetch / missing chatId / fetch rejecting / fallback rejecting → all swallowed and logged.CommandDescriptor.invokeandactions[K]may now returnvoid | RichResponse | Promise<…>.Gateway.postCommandResult()dispatcher forwardsRichResponsereturns totransport.postRich();voidreturns are a no-op (legacy path preserved).Phase 2 (
5eddd18) —/modelmigrated toRichResponse. Dropsthread/postWithFallbackfrom the command module. Same alias list, same settings I/O, same action id.Phase 3 (
15ddfcd) —/topicmigrated toRichResponse. Drops Telegram imports. Documents and asserts the routing rule:applyTopicOverride()only rewrites the agent-session id; the transport thread is preserved by construction. New regression test (test/topic-dispatch-regression.test.ts) drives the liveGateway.dispatchInTurnCommandand asserts the original chat-SDK thread reachestransport.postRich(.toBereferential equality, not just shape match).Phase 4 (
bb0d0c1) —/cronsmigrated. Promoted progress messages (/update,/compact,/doctor, context-pressure UI) to aTransportAdapter.progress()method so commands no longer import fromsrc/transports/telegram/.Codex self-review loops
Three review loops, all findings resolved:
94e8963) — 5 findings: hardened postRich's no-fetch fallback, fixed regression test scope, promotedprogress()toTransportAdapter, droppedChatThreadLikedep from command-registry, tightenedanyincollectAndValidateActions.4f2e386) — 4 findings: rewrote regression test to driveGateway.dispatchInTurnCommand(the actual live dispatcher), fixed pre-existing TypeScript scope bug ondeferredSoftFlush, tightened newly-introducedanytypes, deleted now-unreachableinline-keyboard.tsshim and its test.7ae6b29) — 1 actionable finding: tightened residualanyindispatchInTurnCommandsignature andprogress.tstelegramFetch return type.Tests
inline-keyboard.test.tswas subsumed by the newrich-ui.test.ts)npx tsc --noEmitclean forsrc/gateway/,src/transports/,test/. (Pre-existing trunk errors insrc/agents/pi/,src/cli/,src/cron/runner.tsare unrelated and out of scope.)New test files:
test/rich-ui.test.ts— Telegram callback encoding +RichMenu → inline_keyboard(default 2-col, custom columns, multi-section, selected-state hint, empty-menu).test/telegram-postrich.test.ts— happy path; missing-fetch fallback; rejecting-fetch fallback; never-throws when fallback also fails; text-only response posts no keyboard.test/post-command-result.test.ts— void = no-op;RichResponsecallspostRichexactly once with the same thread+response; awaits the promise; last-ditchthread.post(text)ifpostRichthrows.test/topic-dispatch-regression.test.ts— drives the liveGateway.dispatchInTurnCommand; asserts the SDK-delivered thread object reachestransport.postRichunchanged when invoked from inside a named-topic session.Deviations from the design doc
createProgressMessagewas promoted toTransportAdapter.progress(). The design doc only proposedpostRich; codex round 1 (correctly) flagged that command files still imported fromsrc/transports/telegram/for progress messages. Resolved by addingprogress()to the public adapter contract — same shape, different verb./new,/restart,/status,/stop,/verbose,/doctorstill callthread.post/postWithFallbackdirectly. They returnvoidfrom their descriptor — the new dispatcher's no-op-on-void branch keeps them working unchanged. Brief explicitly said "/crons currently text-only — minimal work, just convert text replies to{ text: \"...\" }. Skip menu surface unless trivial." That guidance is what the migration applied. Migrating the other text-only commands is a future cleanup PR; no behaviour change required.deferredSoftFlushscope bug fixed in passing. Not part of the rich-UI design; surfaced bytsc --noEmitwhile the file was open. Documented in the round-2 commit message.Hard rules respected
Open questions
None blocking. Two optional follow-ups for a future PR:
/new,/restart,/status,/stop,/verbose,/doctortoRichResponseso the entire descriptor surface is uniform. Pure refactor, no behaviour change.thread: anypattern by threading the chat SDK'sThread<TRaw>generic throughhelpers.ts,streaming.ts, andgateway.ts(medium-size effort, separate from this surface migration).