feat(telegram): approval forwarding + /sessions + /chat commands#33
feat(telegram): approval forwarding + /sessions + /chat commands#33
Conversation
- Tool approvals forwarded to Telegram with Allow/Deny inline buttons - Reply to approval message with text to deny with instructions - /sessions lists all active Ghost sessions with ID, project, mode - /chat <session_id> <message> sends a message to a specific session - Server ApprovalNotifier interface for external approval forwarding - Callback query handlers for approve:/deny: button presses - Short session IDs supported (prefix match)
📝 WalkthroughWalkthroughThis PR integrates Telegram bot approval notifications into Ghost. It introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant User as Telegram User
participant Bot as Telegram Bot
participant Server as Ghost Server
participant DB as Database/State
User->>Server: Trigger action requiring approval
Server->>Bot: Call NotifyApproval(sessionID, projectName, toolName, input)
Bot->>Bot: Store approval state (sessionID, toolName)
Bot->>User: Send approval message with Approve/Deny buttons
User->>Bot: Press Approve or Deny button
Bot->>Bot: Parse callback (approve: or deny: prefix)
Bot->>Server: POST /api/v1/sessions/{sessionID}/approve
Server->>DB: Process approval/denial
Server-->>Bot: Response (success/error)
Bot->>User: Edit message to show approval result
Bot->>Bot: Clear approval state
alt User replies to approval message
User->>Bot: Reply with instruction text
Bot->>Bot: Check for pending approval
Bot->>Server: POST /api/v1/sessions/{sessionID}/approve with instructions
Server->>DB: Process denial with instructions
Bot->>User: Confirm denial with instructions
Bot->>Bot: Clear approval state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ghost/main.go`:
- Around line 357-361: The bot is being given a bind address
(cfg.Server.ListenAddr) that may be non-dialable (e.g. ":8080", "0.0.0.0");
before calling tgBot.SetServerAddr, normalize cfg.Server.ListenAddr into a
dialable base URL used by the Telegram code (internal/telegram/approval.go and
internal/telegram/sessions.go). Implement logic near where tgBot.SetServerAddr
is called to parse cfg.Server.ListenAddr, ensure a host is present (replace
empty or wildcard hosts like ""/":"/ "0.0.0.0"/"::" with "127.0.0.1" or
"localhost"), ensure a port is present, prepend the http(s) scheme as
appropriate, and pass that normalized URL to tgBot.SetServerAddr (leaving
srv.SetApprovalNotifier(tgBot) unchanged).
In `@internal/telegram/approval.go`:
- Around line 17-22: The current single-slot approvalState (type approvalState
with fields mu, sessionID, toolName) allows a new approval to overwrite
tb.approval.sessionID and misroute instruction replies across sessions; change
the design to track approvals by the unique approval message identity (chat ID +
message ID) instead of a single global sessionID, store a map keyed by that pair
to approval metadata (sessionID, toolName), set the map entry when sending an
approval message, and update handleInstructionReply to look up the sessionID by
inspecting msg.ReplyToMessage (use ReplyToMessage.Chat.ID +
ReplyToMessage.MessageID as the key) and route replies to the correct session;
ensure proper locking around the map when creating, reading, and deleting
entries to avoid races.
- Around line 179-181: The callApproveAPI function currently uses http.Post
without a timeout and doesn't set an Authorization header, which can hang or
produce 401s; replace the http.Post calls in callApproveAPI with an
http.NewRequestWithContext (or create an http.Client with Timeout) using a
context with a deadline (e.g., 5–10s) and add the required Authorization header
(use the agent's stored credential field such as tb.apiKey/tb.authToken or a
provided config value) before sending via http.DefaultClient.Do (or the custom
client), and apply the same pattern to the other similar http calls flagged (the
nearby http.Post usages in callApproveAPI and the analogous patterns in
sessions.go).
In `@internal/telegram/sessions.go`:
- Around line 53-55: Create a small safe helper (e.g., shortID(id string)
string) that returns id if len(id) < 8 else id[:8], then replace raw slices
s.ID[:8] and fullID[:8] with shortID(s.ID) and shortID(fullID) respectively;
update the two locations referencing s.ID[:8] (the fmt.Fprintf call using
mdv2.Esc(s.ID[:8])) and the uses at the later block (lines ~105-106) so they
call mdv2.Esc(shortID(...)) to avoid panics on short IDs.
- Around line 98-105: The code calls tb.sendChatMessage(fullID, message) and
treats a nil error as success without inspecting the SSE stream payload; change
sendChatMessage (and its call sites around the replies at the shown block and
the similar block at 138-145) so it parses SSE events from the response body and
returns a non-nil error or special status when an SSE event with type "error" or
"approval_required" is received; update the calling site (where tb.reply
currently sends "📤 Sent to ...") to check that returned status/error and, on
SSE "error", reply with the error details (using mdv2.Esc(err.Error())) and
return, and on "approval_required" reply with a clear waiting/approval message
and return instead of reporting success.
- Around line 84-90: The current loop picks the first session whose s.ID has the
provided sessionID prefix (fullID variable), which can mis-route when multiple
IDs share that prefix; instead collect all matches from sessions where
strings.HasPrefix(s.ID, sessionID), then if zero matches return a not-found
error, if exactly one set fullID to that match, and if multiple return an
explicit ambiguity error (or prompt) listing the matching IDs so the caller can
be more specific; update the logic around the fullID variable and any callers
that expect a single resolved ID to handle the new ambiguous-case error path.
- Around line 109-113: fetchSessions (and the sibling POST call that creates
sessions) currently use http.Get/http.Post without timeouts or auth; replace
these with context-aware HTTP requests using a reusable http.Client with
Timeout: 10*time.Second, build requests via http.NewRequestWithContext(ctx,
...), and set the Authorization header to "Bearer "+tb.bearerToken (or
tb.config.BearerToken if you store it on the Bot struct). Concretely, in
Bot.fetchSessions use context.WithTimeout to create ctx, create req :=
http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://%s/api/v1/sessions",
tb.serverAddr), nil), set req.Header.Set("Authorization", "Bearer
"+tb.bearerToken), use client := &http.Client{Timeout: 10 * time.Second} and
client.Do(req) (similarly replace http.Post usage in the session-creation
function), and handle context/timeouts and non-2xx status codes accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c3ed4df-30c5-49ec-8fac-b0bfc8939d5d
📒 Files selected for processing (6)
cmd/ghost/main.gointernal/server/chat.gointernal/server/server.gointernal/telegram/approval.gointernal/telegram/bot.gointernal/telegram/sessions.go
| // Wire Telegram bot as approval notifier + give it the server address. | ||
| if tgBot != nil { | ||
| srv.SetApprovalNotifier(tgBot) | ||
| tgBot.SetServerAddr(cfg.Server.ListenAddr) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the current state of cmd/ghost/main.go around lines 357-361
wc -l cmd/ghost/main.goRepository: wcatz/ghost
Length of output: 76
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '350,370p' cmd/ghost/main.goRepository: wcatz/ghost
Length of output: 590
🏁 Script executed:
# Search for SetServerAddr to understand how it's used
rg "SetServerAddr" --type go -A 3 -B 3Repository: wcatz/ghost
Length of output: 452
🏁 Script executed:
# Look for the ListenAddr configuration structure
rg "ListenAddr" --type go -A 2 -B 2Repository: wcatz/ghost
Length of output: 2272
🏁 Script executed:
# Check for any URL building that uses the server address
rg "http://" --type go -A 2 -B 2 | head -50Repository: wcatz/ghost
Length of output: 2537
🏁 Script executed:
head -30 cmd/ghost/main.goRepository: wcatz/ghost
Length of output: 812
🏁 Script executed:
# Check if net is already imported
rg "^import" -A 20 cmd/ghost/main.go | head -30Repository: wcatz/ghost
Length of output: 492
Normalize the server address before passing it to the Telegram bot.
cfg.Server.ListenAddr is a bind address and may be non-dialable (e.g., :8080, ::, 0.0.0.0). The bot uses this address to construct HTTP URLs in internal/telegram/approval.go and internal/telegram/sessions.go (e.g., http://<addr>/api/v1/sessions), which will fail if the address cannot be dialed from a client.
Suggested fix
+import "net"
@@
if tgBot != nil {
srv.SetApprovalNotifier(tgBot)
- tgBot.SetServerAddr(cfg.Server.ListenAddr)
+ host, port, err := net.SplitHostPort(cfg.Server.ListenAddr)
+ if err == nil {
+ if host == "" || host == "0.0.0.0" || host == "::" {
+ host = "127.0.0.1"
+ }
+ tgBot.SetServerAddr(net.JoinHostPort(host, port))
+ } else {
+ tgBot.SetServerAddr(cfg.Server.ListenAddr)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Wire Telegram bot as approval notifier + give it the server address. | |
| if tgBot != nil { | |
| srv.SetApprovalNotifier(tgBot) | |
| tgBot.SetServerAddr(cfg.Server.ListenAddr) | |
| } | |
| // Wire Telegram bot as approval notifier + give it the server address. | |
| if tgBot != nil { | |
| srv.SetApprovalNotifier(tgBot) | |
| host, port, err := net.SplitHostPort(cfg.Server.ListenAddr) | |
| if err == nil { | |
| if host == "" || host == "0.0.0.0" || host == "::" { | |
| host = "127.0.0.1" | |
| } | |
| tgBot.SetServerAddr(net.JoinHostPort(host, port)) | |
| } else { | |
| tgBot.SetServerAddr(cfg.Server.ListenAddr) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ghost/main.go` around lines 357 - 361, The bot is being given a bind
address (cfg.Server.ListenAddr) that may be non-dialable (e.g. ":8080",
"0.0.0.0"); before calling tgBot.SetServerAddr, normalize cfg.Server.ListenAddr
into a dialable base URL used by the Telegram code
(internal/telegram/approval.go and internal/telegram/sessions.go). Implement
logic near where tgBot.SetServerAddr is called to parse cfg.Server.ListenAddr,
ensure a host is present (replace empty or wildcard hosts like ""/":"/
"0.0.0.0"/"::" with "127.0.0.1" or "localhost"), ensure a port is present,
prepend the http(s) scheme as appropriate, and pass that normalized URL to
tgBot.SetServerAddr (leaving srv.SetApprovalNotifier(tgBot) unchanged).
| // approvalState tracks which session has a pending approval for instruction replies. | ||
| type approvalState struct { | ||
| mu sync.Mutex | ||
| sessionID string // session with pending approval | ||
| toolName string | ||
| } |
There was a problem hiding this comment.
Global single-slot approval state can misroute replies across sessions.
A new approval overwrites tb.approval.sessionID, and instruction replies are accepted for any reply-to-bot message. With concurrent approvals, instructions can be sent to the wrong session.
Safer direction
type approvalState struct {
mu sync.Mutex
- sessionID string // session with pending approval
- toolName string
+ pending map[string]string // key: "<chatID>:<messageID>" -> sessionID
}Track per approval message (chat/message pair), and in handleInstructionReply resolve sessionID using ReplyToMessage identity instead of a single global slot.
Also applies to: 56-59, 138-155, 169-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/approval.go` around lines 17 - 22, The current single-slot
approvalState (type approvalState with fields mu, sessionID, toolName) allows a
new approval to overwrite tb.approval.sessionID and misroute instruction replies
across sessions; change the design to track approvals by the unique approval
message identity (chat ID + message ID) instead of a single global sessionID,
store a map keyed by that pair to approval metadata (sessionID, toolName), set
the map entry when sending an approval message, and update
handleInstructionReply to look up the sessionID by inspecting msg.ReplyToMessage
(use ReplyToMessage.Chat.ID + ReplyToMessage.MessageID as the key) and route
replies to the correct session; ensure proper locking around the map when
creating, reading, and deleting entries to avoid races.
| if tb.serverAddr == "" { | ||
| return fmt.Errorf("server address not configured") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/telegram/approval.go | head -220Repository: wcatz/ghost
Length of output: 7644
🏁 Script executed:
rg -n "http.Client|Transport|Authorization" internal/telegram/ -A 2 -B 2Repository: wcatz/ghost
Length of output: 37
🏁 Script executed:
rg -n "type Bot struct" internal/telegram/ -A 20Repository: wcatz/ghost
Length of output: 1350
🏁 Script executed:
rg -n "http\." internal/telegram/ -B 2 -A 2Repository: wcatz/ghost
Length of output: 2223
callApproveAPI needs timeout and Authorization header handling.
The http.Post call on line 196 lacks a timeout, risking indefinite hangs if the server is unresponsive. Additionally, no Authorization header is set, causing 401 failures if the API requires authentication.
Use http.DefaultClient with a context timeout or create a custom client with http.Client{Timeout: ...}. Add an Authorization header to the request if the API requires it.
Also applies to: lines 195–197, 202–204, and similar patterns in sessions.go (lines 111, 132).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/approval.go` around lines 179 - 181, The callApproveAPI
function currently uses http.Post without a timeout and doesn't set an
Authorization header, which can hang or produce 401s; replace the http.Post
calls in callApproveAPI with an http.NewRequestWithContext (or create an
http.Client with Timeout) using a context with a deadline (e.g., 5–10s) and add
the required Authorization header (use the agent's stored credential field such
as tb.apiKey/tb.authToken or a provided config value) before sending via
http.DefaultClient.Do (or the custom client), and apply the same pattern to the
other similar http calls flagged (the nearby http.Post usages in callApproveAPI
and the analogous patterns in sessions.go).
| fmt.Fprintf(&sb, "%s `%s`\n %s \\| %s \\| %d msgs\n\n", | ||
| status, mdv2.Esc(s.ID[:8]), mdv2.Esc(s.ProjectName), mdv2.Esc(s.Mode), s.Messages) | ||
| } |
There was a problem hiding this comment.
Guard short-ID slicing to avoid panics.
Both s.ID[:8] and fullID[:8] can panic when IDs are shorter than 8 characters.
Safe short-ID helper
- fmt.Fprintf(&sb, "%s `%s`\n %s \\| %s \\| %d msgs\n\n",
- status, mdv2.Esc(s.ID[:8]), mdv2.Esc(s.ProjectName), mdv2.Esc(s.Mode), s.Messages)
+ fmt.Fprintf(&sb, "%s `%s`\n %s \\| %s \\| %d msgs\n\n",
+ status, mdv2.Esc(shortID(s.ID)), mdv2.Esc(s.ProjectName), mdv2.Esc(s.Mode), s.Messages)
@@
- tb.reply(ctx, b, update, fmt.Sprintf("📤 Sent to `%s`:\n_%s_",
- mdv2.Esc(fullID[:8]), mdv2.Esc(message)))
+ tb.reply(ctx, b, update, fmt.Sprintf("📤 Sent to `%s`:\n_%s_",
+ mdv2.Esc(shortID(fullID)), mdv2.Esc(message)))
+
+func shortID(id string) string {
+ if len(id) <= 8 {
+ return id
+ }
+ return id[:8]
+}Also applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/sessions.go` around lines 53 - 55, Create a small safe
helper (e.g., shortID(id string) string) that returns id if len(id) < 8 else
id[:8], then replace raw slices s.ID[:8] and fullID[:8] with shortID(s.ID) and
shortID(fullID) respectively; update the two locations referencing s.ID[:8] (the
fmt.Fprintf call using mdv2.Esc(s.ID[:8])) and the uses at the later block
(lines ~105-106) so they call mdv2.Esc(shortID(...)) to avoid panics on short
IDs.
| fullID := "" | ||
| for _, s := range sessions { | ||
| if strings.HasPrefix(s.ID, sessionID) { | ||
| fullID = s.ID | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle ambiguous short ID prefixes explicitly.
Current logic takes the first prefix match, so /chat can target the wrong session when multiple IDs share the prefix.
Suggested resolution logic
- fullID := ""
- for _, s := range sessions {
- if strings.HasPrefix(s.ID, sessionID) {
- fullID = s.ID
- break
- }
- }
- if fullID == "" {
+ var matches []string
+ for _, s := range sessions {
+ if s.ID == sessionID {
+ matches = []string{s.ID}
+ break
+ }
+ if strings.HasPrefix(s.ID, sessionID) {
+ matches = append(matches, s.ID)
+ }
+ }
+ if len(matches) == 0 {
tb.reply(ctx, b, update, "Session not found\\. Use /sessions to list\\.")
return
}
+ if len(matches) > 1 {
+ tb.reply(ctx, b, update, "Session ID is ambiguous\\. Please use a longer prefix\\.")
+ return
+ }
+ fullID := matches[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fullID := "" | |
| for _, s := range sessions { | |
| if strings.HasPrefix(s.ID, sessionID) { | |
| fullID = s.ID | |
| break | |
| } | |
| } | |
| var matches []string | |
| for _, s := range sessions { | |
| if s.ID == sessionID { | |
| matches = []string{s.ID} | |
| break | |
| } | |
| if strings.HasPrefix(s.ID, sessionID) { | |
| matches = append(matches, s.ID) | |
| } | |
| } | |
| if len(matches) == 0 { | |
| tb.reply(ctx, b, update, "Session not found\\. Use /sessions to list\\.") | |
| return | |
| } | |
| if len(matches) > 1 { | |
| tb.reply(ctx, b, update, "Session ID is ambiguous\\. Please use a longer prefix\\.") | |
| return | |
| } | |
| fullID := matches[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/sessions.go` around lines 84 - 90, The current loop picks
the first session whose s.ID has the provided sessionID prefix (fullID
variable), which can mis-route when multiple IDs share that prefix; instead
collect all matches from sessions where strings.HasPrefix(s.ID, sessionID), then
if zero matches return a not-found error, if exactly one set fullID to that
match, and if multiple return an explicit ambiguity error (or prompt) listing
the matching IDs so the caller can be more specific; update the logic around the
fullID variable and any callers that expect a single resolved ID to handle the
new ambiguous-case error path.
| // Send message via API. This returns an SSE stream — we just report success. | ||
| err = tb.sendChatMessage(fullID, message) | ||
| if err != nil { | ||
| tb.reply(ctx, b, update, "Error: "+mdv2.Esc(err.Error())) | ||
| return | ||
| } | ||
|
|
||
| tb.reply(ctx, b, update, fmt.Sprintf("📤 Sent to `%s`:\n_%s_", |
There was a problem hiding this comment.
Don’t return success without interpreting SSE outcome events.
sendChatMessage drains the stream and returns nil on HTTP 200, but /api/v1/sessions/{id}/send reports runtime failures and approval waits inside SSE events (error, approval_required). This can show “sent” even when execution failed or is blocked.
Also applies to: 138-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/sessions.go` around lines 98 - 105, The code calls
tb.sendChatMessage(fullID, message) and treats a nil error as success without
inspecting the SSE stream payload; change sendChatMessage (and its call sites
around the replies at the shown block and the similar block at 138-145) so it
parses SSE events from the response body and returns a non-nil error or special
status when an SSE event with type "error" or "approval_required" is received;
update the calling site (where tb.reply currently sends "📤 Sent to ...") to
check that returned status/error and, on SSE "error", reply with the error
details (using mdv2.Esc(err.Error())) and return, and on "approval_required"
reply with a clear waiting/approval message and return instead of reporting
success.
| func (tb *Bot) fetchSessions() ([]apiSession, error) { | ||
| url := fmt.Sprintf("http://%s/api/v1/sessions", tb.serverAddr) | ||
| resp, err := http.Get(url) //nolint:gosec | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/telegram/sessions.go | head -150Repository: wcatz/ghost
Length of output: 4842
🏁 Script executed:
rg -A 20 "type Bot struct" internal/telegram/Repository: wcatz/ghost
Length of output: 1287
🏁 Script executed:
rg -i "bearer|authorization|auth" internal/telegram/sessions.goRepository: wcatz/ghost
Length of output: 37
🏁 Script executed:
cd internal/telegram && go vet ./...Repository: wcatz/ghost
Length of output: 2419
🏁 Script executed:
cd . && go vet ./internal/telegram/...Repository: wcatz/ghost
Length of output: 37
🏁 Script executed:
rg -i "bearer|x-api-key|authorization" internal/Repository: wcatz/ghost
Length of output: 1839
🏁 Script executed:
cat internal/config/config.example.yaml | grep -A 5 -B 5 "auth_token"Repository: wcatz/ghost
Length of output: 402
🏁 Script executed:
rg -A 10 "authMiddleware" internal/server/server.goRepository: wcatz/ghost
Length of output: 932
🏁 Script executed:
rg -B 20 "api/v1/sessions" internal/server/server.go | head -50Repository: wcatz/ghost
Length of output: 715
🏁 Script executed:
rg -A 15 'r.Route\("/api/v1/sessions"' internal/server/server.goRepository: wcatz/ghost
Length of output: 702
Add timeout and bearer token support to HTTP API calls.
http.Get (line 111) and http.Post (line 132) have no deadline and send no authorization header. These requests will fail with 401 when server auth is enabled, and without a timeout they can hang bot handlers indefinitely.
Create a context-aware http.Client with a reasonable timeout (e.g., 10 seconds) and add bearer token support from the bot's configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/sessions.go` around lines 109 - 113, fetchSessions (and the
sibling POST call that creates sessions) currently use http.Get/http.Post
without timeouts or auth; replace these with context-aware HTTP requests using a
reusable http.Client with Timeout: 10*time.Second, build requests via
http.NewRequestWithContext(ctx, ...), and set the Authorization header to
"Bearer "+tb.bearerToken (or tb.config.BearerToken if you store it on the Bot
struct). Concretely, in Bot.fetchSessions use context.WithTimeout to create ctx,
create req := http.NewRequestWithContext(ctx, "GET",
fmt.Sprintf("http://%s/api/v1/sessions", tb.serverAddr), nil), set
req.Header.Set("Authorization", "Bearer "+tb.bearerToken), use client :=
&http.Client{Timeout: 10 * time.Second} and client.Do(req) (similarly replace
http.Post usage in the session-creation function), and handle context/timeouts
and non-2xx status codes accordingly.
Summary
ApprovalNotifierinterface decouples approval forwarding from TelegramArchitecture
Test plan
go vet ./...cleango build ./cmd/ghost/succeedsSummary by CodeRabbit
Release Notes