feat(telegram/acp): Topic Binding, Pin Binding Message, Fix Spawn Param Parsing#36683
Conversation
openclaw-barnacle
bot
added
docs
huntharo
force-pushed
the
acp-telegram-value-reapply-main
branch
from
36c06b4 to
b2727d2
Compare
Greptile SummaryThis PR adds Telegram topic-binding support for ACP/subagent flows, Unicode em/en-dash normalization for Key highlights:
Confidence Score: 3/5
Last reviewed commit: 7fde11b |
![greptile-apps[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (currentConversationId && boundConversationId === currentConversationId) { | ||
| parts.push(`Bound this thread to ${sessionKey}.`); |
There was a problem hiding this comment.
The bound-topic confirmation message hardcodes "thread" for all channels. For Telegram contexts, "conversation" or "topic" would match the Telegram terminology used elsewhere in this PR.
| if (currentConversationId && boundConversationId === currentConversationId) { | |
| parts.push(`Bound this thread to ${sessionKey}.`); | |
| parts.push( | |
| binding.conversation.channel === "telegram" | |
| ? `Bound this conversation to ${sessionKey}.` | |
| : `Bound this thread to ${sessionKey}.`, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-acp/lifecycle.ts
Line: 345-346
Comment:
The bound-topic confirmation message hardcodes "thread" for all channels. For Telegram contexts, "conversation" or "topic" would match the Telegram terminology used elsewhere in this PR.
```suggestion
parts.push(
binding.conversation.channel === "telegram"
? `Bound this conversation to ${sessionKey}.`
: `Bound this thread to ${sessionKey}.`,
);
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in c8ba3c2. Telegram bind confirmations now use conversation wording on Telegram surfaces and keep thread wording for Discord.
![chatgpt-codex-connector[bot]](proxy.php?url=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: 36c06b4006
ℹ️ 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".
| route = { | ||
| ...route, | ||
| sessionKey: boundSessionKey, | ||
| agentId: resolveAgentIdFromSessionKey(boundSessionKey), | ||
| }; |
There was a problem hiding this comment.
When a Telegram conversation binding is found, buildTelegramMessageContext only updates route.sessionKey/agentId and leaves route.matchedBy unchanged. On named Telegram accounts, the later fail-closed guard (candidate.accountId !== DEFAULT_ACCOUNT_ID && candidate.matchedBy === "default") will still drop the message even though an explicit conversation binding exists, so bound-topic/session routing silently fails for those accounts. Set matchedBy to a binding value (as done in Discord preflight) when applying a Telegram binding override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c8ba3c2. Bound Telegram conversation overrides now mark matchedBy=binding.channel before the non-default account guard, and there is regression coverage in src/telegram/bot-message-context.thread-binding.test.ts.
huntharo
force-pushed
the
acp-telegram-value-reapply-main
branch
from
300cc7d to
11f9c3d
Compare
huntharo
force-pushed
the
acp-telegram-value-reapply-main
branch
from
11f9c3d to
7fde11b
Compare
|
@greptile-apps can you recheck the code. |
src/telegram/thread-bindings.ts
Outdated
| boundAt: now, | ||
| lastActivityAt: now, |
There was a problem hiding this comment.
boundAt reset incorrectly extends the max-age window on every /session max-age call
setTelegramThreadBindingMaxAgeBySessionKey resets boundAt: now when updating the max-age setting. The entire point of maxAgeMs is to cap the total lifetime of a binding from when it was first created. The sweeper (shouldExpireByMaxAge) and resolveEffectiveBindingExpiresAt both compute expiry as record.boundAt + maxAgeMs. By writing boundAt: now, calling /session max-age 6h restarts the clock from the time of the command, so a binding can be kept alive indefinitely with repeated calls — defeating the purpose of the hard cap.
Contrast this with setTelegramThreadBindingIdleTimeoutBySessionKey (line 680), which correctly leaves boundAt untouched and only updates lastActivityAt: now.
| boundAt: now, | |
| lastActivityAt: now, | |
| const next: TelegramThreadBindingRecord = { | |
| ...entry, | |
| maxAgeMs, | |
| lastActivityAt: now, | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/telegram/thread-bindings.ts
Line: 714-715
Comment:
**`boundAt` reset incorrectly extends the max-age window on every `/session max-age` call**
`setTelegramThreadBindingMaxAgeBySessionKey` resets `boundAt: now` when updating the max-age setting. The entire point of `maxAgeMs` is to cap the total lifetime of a binding from when it was *first created*. The sweeper (`shouldExpireByMaxAge`) and `resolveEffectiveBindingExpiresAt` both compute expiry as `record.boundAt + maxAgeMs`. By writing `boundAt: now`, calling `/session max-age 6h` restarts the clock from the time of the command, so a binding can be kept alive indefinitely with repeated calls — defeating the purpose of the hard cap.
Contrast this with `setTelegramThreadBindingIdleTimeoutBySessionKey` (line 680), which correctly leaves `boundAt` untouched and only updates `lastActivityAt: now`.
```suggestion
const next: TelegramThreadBindingRecord = {
...entry,
maxAgeMs,
lastActivityAt: now,
};
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in c8ba3c2. setTelegramThreadBindingMaxAgeBySessionKey now preserves the original boundAt and only updates maxAgeMs plus lastActivityAt. The regression test in src/telegram/thread-bindings.test.ts asserts that boundAt stays unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fde11b02b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af8eb60dda
ℹ️ 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".
| void persistBindingsToDisk({ | ||
| accountId: manager.accountId, | ||
| persist: true, | ||
| }); |
There was a problem hiding this comment.
setTelegramThreadBindingIdleTimeoutBySessionKey always persists with persist: true, so /session idle will write binding state even when the manager was intentionally created with persist: false. In in-memory/test deployments this leaks ephemeral bindings to disk and can reload stale conversation focus after restart, changing routing behavior unexpectedly; this update path should use the manager’s configured persistence mode (same pattern as Discord lifecycle updates).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2808e9b.
setTelegramThreadBindingIdleTimeoutBySessionKey() now follows the manager's configured persistence mode instead of forcing persist: true, and src/telegram/thread-bindings.test.ts now covers the persist: false case to ensure lifecycle updates do not write state to disk.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8ba3c24e1
ℹ️ 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".
| defaultMaxAgeMs: threadBindings.getMaxAgeMs(), | ||
| }); | ||
| const now = Date.now(); | ||
| const nextExpiry = durationMs > 0 ? now + durationMs : undefined; |
There was a problem hiding this comment.
The confirmation message for /session max-age always uses Date.now() + duration, but max-age expiry is actually anchored to each binding’s original boundAt timestamp. For bindings that were created earlier, this reports a deadline that is later than the real auto-unfocus time, so operators can be told a session is safe for hours even though it may expire almost immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2808e9b.
/session max-age confirmation now derives the displayed expiry from the updated binding state, so Telegram max-age uses boundAt + maxAgeMs instead of Date.now() + duration. Added a regression test in src/auto-reply/reply/commands-session-lifecycle.test.ts for an older binding.
src/telegram/thread-bindings.ts
Outdated
| if (updated.length > 0) { | ||
| void persistBindingsToDisk({ | ||
| accountId: manager.accountId, | ||
| persist: true, |
There was a problem hiding this comment.
setTelegramThreadBindingMaxAgeBySessionKey unconditionally writes with persist: true, so /session max-age will still flush bindings to disk even when the manager was intentionally created with persist: false (for ephemeral/test deployments). That leaks temporary focus state into persisted files and can reapply stale bindings after restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2808e9b.
setTelegramThreadBindingMaxAgeBySessionKey() now respects the manager's persistence mode instead of always persisting, and the new persist: false regression test exercises both idle and max-age lifecycle updates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d5cfe881
ℹ️ 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".
| const currentThreadId = bindingContext.threadId ?? ""; | ||
|
|
||
| if (threadMode === "here" && !currentThreadId) { | ||
| const currentConversationId = bindingContext.conversationId?.trim() || ""; |
There was a problem hiding this comment.
Telegram DM /acp spawn ... --thread here can never bind because this flow relies on bindingContext.conversationId, but that value is empty in normal DMs (resolveAcpCommandConversationId falls back to resolveConversationIdFromTargets, which does not parse telegram:<chatId> targets). As a result, users in active Telegram DMs hit the --thread here requires... / Could not resolve... errors even though DM conversation bindings are otherwise supported, so ACP spawn auto-binding is broken for DMs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5badacd.
ACP command context now reuses the shared Telegram conversation resolver, so Telegram DMs produce the canonical DM conversation id instead of falling through to the generic target parser. Added regressions in src/auto-reply/reply/commands-acp/context.test.ts and src/auto-reply/reply/commands-acp.test.ts covering DM context resolution and /acp spawn ... --thread here in a Telegram DM.
|
Addressed the latest Telegram ACP review in What changed:
Coverage added:
Validated with:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5badacd639
ℹ️ 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".
| }; | ||
| configuredBinding = null; | ||
| configuredBindingSessionKey = ""; | ||
| getSessionBindingService().touch(threadBinding.bindingId); |
There was a problem hiding this comment.
buildTelegramMessageContext updates the Telegram binding activity timestamp before any allowlist/DM policy checks run, so messages that are later rejected (for example, unauthorized users in a restricted group/topic) still call touch and extend idle timeout. In a busy restricted chat this can keep a binding alive indefinitely even though no authorized traffic is occurring, which defeats /session idle auto-unfocus behavior for that conversation.
Useful? React with 👍 / 👎.
osolmaz
force-pushed
the
acp-telegram-value-reapply-main
branch
from
5badacd to
78e428d
Compare
* main: fix(tui): prevent stale model indicator after /model fix(gateway): preserve streamed prefixes across tool boundaries feat(telegram/acp): Topic Binding, Pin Binding Message, Fix Spawn Param Parsing (openclaw#36683) fix(auth): harden openai-codex oauth login path fix(llm-task): load runEmbeddedPiAgent from dist/extensionAPI in installs fix(gateway): honor insecure ws override for remote hostnames fix(routing): avoid full binding rescans in resolveAgentRoute (openclaw#36915) Diffs: restore system prompt guidance (openclaw#36904) fix(browser): close tracked tabs on session cleanup (openclaw#36666) Harden Telegram poll gating and schema consistency (openclaw#36547) fix(memory): avoid destructive qmd collection rebinds plugins: enforce prompt hook policy with runtime validation (openclaw#36567) fix: decouple Discord inbound worker timeout from listener timeout (openclaw#36602) (thanks @dutifulbob) (openclaw#36602) README: add algal to contributors list (openclaw#2046) fix(slack): preserve dedupe while recovering dropped app_mention (openclaw#34937) fix(slack): propagate mediaLocalRoots through Slack send path fix(config): prevent RangeError in merged schema cache key generation fix(auth): grant senderIsOwner for internal channels with operator.admin scope (openclaw#35704) feat: append UTC time alongside local time in shared Current time lines (openclaw#32423)
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟠 Telegram topic conversation binding routes any participant’s messages to bound ACP/session without verifying binder
Description
Vulnerable routing logic: const threadBinding = getSessionBindingService().resolveByConversation({ ... });
...
route = {
...route,
sessionKey: boundSessionKey,
agentId: resolveAgentIdFromSessionKey(boundSessionKey),
matchedBy: "binding.channel",
};While RecommendationEnforce an authorization check before routing via a conversation binding. At minimum, restrict routing to the binding creator (or an explicitly configured allowlist/admin set), especially for group topics: const boundBy = typeof threadBinding.metadata?.boundBy === "string"
? threadBinding.metadata.boundBy.trim()
: "";
const isGroupTopic = isGroup && replyThreadId != null;
if (isGroupTopic && boundBy && boundBy !== "system" && senderId && senderId !== boundBy) {
// Do not route to bound session; fall back to normal routing or ignore.
} else {
// existing route override
}More flexible options:
Analyzed PR: #36683 at commit Last updated on: 2026-03-06T02:55:39Z |
…am Parsing (openclaw#36683) * fix(acp): normalize unicode flags and Telegram topic binding * feat(telegram/acp): restore topic-bound ACP and session bindings * fix(acpx): clarify permission-denied guidance * feat(telegram/acp): pin spawn bind notice in topics * docs(telegram): document ACP topic thread binding behavior * refactor(reply): share Telegram conversation-id resolver * fix(telegram/acp): preserve bound session routing semantics * fix(telegram): respect binding persistence and expiry reporting * refactor(telegram): simplify binding lifecycle persistence * fix(telegram): bind acp spawns in direct messages * fix: document telegram ACP topic binding changelog (openclaw#36683) (thanks @huntharo) --------- Co-authored-by: Onur <[email protected]>
…am Parsing (openclaw#36683) * fix(acp): normalize unicode flags and Telegram topic binding * feat(telegram/acp): restore topic-bound ACP and session bindings * fix(acpx): clarify permission-denied guidance * feat(telegram/acp): pin spawn bind notice in topics * docs(telegram): document ACP topic thread binding behavior * refactor(reply): share Telegram conversation-id resolver * fix(telegram/acp): preserve bound session routing semantics * fix(telegram): respect binding persistence and expiry reporting * refactor(telegram): simplify binding lifecycle persistence * fix(telegram): bind acp spawns in direct messages * fix: document telegram ACP topic binding changelog (openclaw#36683) (thanks @huntharo) --------- Co-authored-by: Onur <[email protected]>
…am Parsing (openclaw#36683) * fix(acp): normalize unicode flags and Telegram topic binding * feat(telegram/acp): restore topic-bound ACP and session bindings * fix(acpx): clarify permission-denied guidance * feat(telegram/acp): pin spawn bind notice in topics * docs(telegram): document ACP topic thread binding behavior * refactor(reply): share Telegram conversation-id resolver * fix(telegram/acp): preserve bound session routing semantics * fix(telegram): respect binding persistence and expiry reporting * refactor(telegram): simplify binding lifecycle persistence * fix(telegram): bind acp spawns in direct messages * fix: document telegram ACP topic binding changelog (openclaw#36683) (thanks @huntharo) --------- Co-authored-by: Onur <[email protected]>
3 participants
Summary
Describe the problem and fix in 2–5 bullets:
--args from Telegram Mac could fail parsing,--thread herecould report unavailable on Telegram, approvals could fail due callback/approval-id mismatches, and bound topics did not consistently route follow-ups to ACP sessions./acp steerworkarounds and breaking multi-topic workspace isolation./acp spawnparsing, Telegram exec-approval buttons + approval-id prefix resolution, ACPX permission-denied guidance, and topic bind confirmation pinning for ACP spawns.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None./acp spawnnow accepts Unicode em/en-dash option prefixes used by Telegram Mac input (auto-converts--when typed)./acp spawn ... --thread here|autowhen Telegram threadBindings ACP spawn is enabled./acp steer.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
/acp spawn codex —mode persistent —thread here —cwd <repoA> —label <labelA>using Telegram Mac typed args (Unicode dash input)./acp steer, such asWho are you?./acp spawn codex —mode persistent —thread here —cwd <repoB> —label <labelB>./acp cwd <path>in-topic, then send another follow-up.Expected
Actual
/acp steer.Evidence
Attach at least one:
Baseline - Almost Nothing Works / Binding Not Supported on Telegram
PR - Typing Indicator Works in Topics / Pins Binding Message to Topic / Routes to Codex when Bound
PR - Topic:
ACP Bound- CWD isjeerreview- Describespackage.jsonfrom JeerReviewPR - Topic:
ACP - Bound 2- CWD isopenclaw- Describespackage.jsonfrom OpenClawHuman Verification (required)
What you personally verified (not just CI), and how:
/acp spawn ... --thread heresucceeded in Telegram topics./acp steer./acp cwd) and subsequent routing correctness.Compatibility / Migration
Yes/No) YesYes/No) No (existing flags used; no new required fields)Yes/No) NoFailure Recovery (if this breaks)
channels.telegram.threadBindings.spawnAcpSessions=falseto disable thread-bound ACP spawn in Telegram./acp spawn ... --thread offas immediate operational fallback.backup-acp-telegram-topic-checkpoint-2026-03-05.src/telegram/*andsrc/auto-reply/reply/commands-acp/*./acp spawn ... --thread herereturning unavailable for Telegram topics.Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.