Repository navigation
fix: send-by-default connect + ChatGPT prepare/send flow - #171
Conversation
Two send failures surfaced in production: 1. Connected accounts could not send because send_enabled defaulted to read-only and the connect UI buried the opt-in. Flip the connect UI to default send-enabled (wizard radio + non-wizard checkbox pre-selected), keeping read-only as an explicit opt-out and the conservative server-side fallback (absent field -> read-only). Add set_account_send admin tool so operators can toggle send_enabled without a manual SQL UPDATE. 2. ChatGPT stalled after prepare_send_message, never chaining send_message, and the 5m token expired before a manual retry. Add a next_step field to the prepare response instructing the model to call send_message immediately, extend the confirmation TTL to 10m, and reword the tool descriptions so single-step send is the documented default.
|
@claude review |
|
Claude finished @mashkovd's task in 4m 3s —— View job Review complete
Result: changes requested — 2 P2, 1 P3 (no P1) The core logic is sound: the send-default UI flip is clean, the P2 — must address before merge1. 2. No unit test for P3 — nice to have3. 4. Stale comment in |
|
Claude finished @mashkovd's task in 2m 34s —— View job Review Complete
Verdict: Changes requested — 0 P1, 1 P2, 2 P3. The UI default-to-send logic, P2 — P3 — P3 — |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| return toolErr("set_account_send: %v", err), nil | ||
| } | ||
| err = s.Store.SetSendEnabled(ctx, targetUID, enabled) |
There was a problem hiding this comment.
P2 — Silent false-positive when no session row exists.
SetSendEnabled is documented as "A no-op (nil error) when the user has no active session." So if an operator calls set_account_send for a user whose session was revoked (or who has never provisioned a Telegram session at all), the UPDATE matches zero rows, returns nil, and the tool responds {"ok": true, "send_enabled": true} — a false positive. The operator has no way to distinguish "flag updated" from "row not found."
SetAccessTier avoids this by checking RowsAffected == 0 and returning an error. The same guard is needed here, or SetSendEnabled should return a rows-affected count.
Suggested fix — check after the call:
err = s.Store.SetSendEnabled(ctx, targetUID, enabled)
s.audit(ctx, id, "set_account_send", "", err, startedAt)
if err != nil {
return toolErr("set_account_send: %v", err), nil
}
// SetSendEnabled silently no-ops when no active session row exists.
// Surface this so the caller can tell the difference from a real update.
active, sessionErr := s.Store.HasActiveSession(ctx, targetUID)
if sessionErr == nil && !active {
return toolErr("no active Telegram session for telegram_id %d — connect the account first", tgID), nil
}Alternatively, extend SetSendEnabled/Store to return the number of rows affected. Fix this →
| const ConfirmationTTL = 5 * time.Minute | ||
| // send/pin. 10 minutes accommodates multi-turn LLM reasoning delays and | ||
| // connectors (e.g. ChatGPT) that pause for a user turn between the two calls. | ||
| const ConfirmationTTL = 10 * time.Minute |
There was a problem hiding this comment.
P3 — The Confirmation struct comment two lines below (line 19 in the file) still reads "≤5m" after the TTL bump to 10 minutes. Should be "≤10m".
| enabled — bool, required. true enables real sends; false forces dry-run previews. | ||
|
|
||
| The user must have an active session. New accounts are send-enabled by default; use enabled=false to revoke sending for a specific account without revoking its scope or session.`), | ||
| mcplib.WithNumber("telegram_id", |
There was a problem hiding this comment.
P3 — Neither telegram_id nor enabled carries a mcplib.Required() annotation, so the JSON Schema exposed to the LLM does not declare them required. Consistent with the existing toolSetAccess pattern, but enabled silently defaults to false when omitted — an operator who forgets it will silently disable sends for the target account rather than getting a schema-level rejection.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2 (set_account_send silently returns ok=true when no active session row exists — false positive for admin operators), 2 P3 (stale ≤5m comment in confirm.go, missing Required() annotations on set_account_send params). The rest of the change — send-enabled UI defaults, next_step field, TTL bump, new test — is clean.
| "expires_at": c.ExpiresAt.UTC(), | ||
| "payload_hash": hash, | ||
| "next_step": fmt.Sprintf( | ||
| "Now call send_message with the same peer and text, mode=\"send\", and confirmation_id=%q. Do this immediately without asking the user to confirm — the token expires in 10 minutes.", |
There was a problem hiding this comment.
P3 — duplicated TTL literal in prose: "10 minutes" is hard-coded here, same class of bug as the 5m strings this PR is fixing. If ConfirmationTTL is bumped again, this prose will silently go stale. Use the actual expiry instead:
| "Now call send_message with the same peer and text, mode=\"send\", and confirmation_id=%q. Do this immediately without asking the user to confirm — the token expires in 10 minutes.", | |
| "Now call send_message with the same peer and text, mode=\"send\", and confirmation_id=%q. Do this immediately without asking the user to confirm — the token expires at %s.", | |
| c.ID, c.ExpiresAt.UTC().Format(time.RFC3339), |
| return jsonResult(map[string]any{"telegram_id": tgID, "send_enabled": enabled, "ok": true}) | ||
| } | ||
| return tool, handler | ||
| } |
There was a problem hiding this comment.
P2 — no unit test for toolSetAccountSend: This is a new admin tool that modifies per-account send state. The following paths are untested:
- Scope check: an identity without
admin:usersmust be rejected - Happy path:
SetSendEnabledis called with correct(userID, enabled)and the response contains{telegram_id, send_enabled, ok} ErrUserNotFoundbranch: unknowntelegram_idreturns a tool error, not a Go errorenableddefaults tofalsewhen the argument is omitted — silently disabling send could be a footgun; at minimum the default should be verified by a test
The existing toolSetAccess and toolGetUserAuditLog have similar patterns; add at least happy-path + scope + not-found tests here. Fix this →
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2 (missing tests for new admin tool toolSetAccountSend — scope check, happy path, ErrUserNotFound untested), 1 P2 (prepare_pin_message and pin_message tool descriptions still say '5m' while ConfirmationTTL is now 10m — shared constant, both needed updating), 1 P3 (TTL hard-coded as prose '10 minutes' in next_step field). Core logic for send-default UX and ChatGPT two-step fix is correct.
![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: 39c00ae9fd
ℹ️ 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".
| } | ||
| return toolErr("set_account_send: %v", err), nil | ||
| } | ||
| err = s.Store.SetSendEnabled(ctx, targetUID, enabled) |
There was a problem hiding this comment.
set_account_send always returns success after calling Store.SetSendEnabled, but that store method is a no-op when the user has no active Telegram session. In that case this tool still reports "ok": true/"send_enabled": ... even though nothing was changed, which violates the tool’s own contract (“The user must have an active session”) and can mislead operators into believing send access was updated when it was not.
Useful? React with 👍 / 👎.
| if tgID <= 0 { | ||
| return mcplib.NewToolResultError("telegram_id is required and must be a positive integer"), nil | ||
| } | ||
| enabled := boolArg(args, "enabled", false) |
There was a problem hiding this comment.
The handler treats enabled as optional by parsing it with boolArg(..., false) and never checking key presence, so omitting enabled silently disables sending. Because the tool contract marks enabled as required, a malformed or partially generated admin call can revoke send access unintentionally instead of returning a validation error.
Useful? React with 👍 / 👎.
mctl-claude-remote
Bot
added
steward:owned
- set_account_send: surface a clear error when the target user has no active session row instead of a silent ok=true. SetSendEnabled now returns rows-affected so the tool can distinguish a real update from a no-op; the enable_access caller ignores the count. - Add Required() annotations to set_account_send params. - Add unit tests for set_account_send: admin-scope gate, happy path, unknown telegram_id, and no-active-session. - Update prepare_pin_message / pin_message descriptions from 5m to 10m to match the shared ConfirmationTTL. - next_step now embeds the actual expiry timestamp instead of hard-coded prose, and the Confirmation struct comment reflects the 10m TTL.
|
@claude review Addressed all findings from the prior review:
|
|
Claude finished @mashkovd's task in 1m 55s —— View job Review complete
Verdict: Approved — 0 P1, 0 P2, 0 P3. Every finding from the prior two review rounds is cleanly resolved:
The |
|
Claude finished @mashkovd's task in 1m 33s —— View job Review complete
Result: approved — no P1/P2/P3 findings. All prior findings are cleanly addressed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc70e0e7c
ℹ️ 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".
| return toolErr("set_account_send: %v", err), nil | ||
| } | ||
| rows, err := s.Store.SetSendEnabled(ctx, targetUID, enabled) | ||
| s.audit(ctx, id, "set_account_send", "", err, startedAt) |
There was a problem hiding this comment.
When SetSendEnabled matches zero rows (no active session), this handler returns a tool error, but the audit/metrics are already emitted as success because s.audit(...) is called immediately after SetSendEnabled with err == nil. This produces incorrect ok audit rows and success counters for a failed admin operation, which can hide real access-toggle failures during incident/debug workflows.
Useful? React with 👍 / 👎.
Summary
Fixes two production send failures (observed from ChatGPT and Claude mobile connectors).
1. Connected accounts couldn't send (
send_enabled=false)send_enableddefaulted to read-only and the connect UI presented sending as a buried opt-in, so users who connected ended up unable to send (e.g. a read-only connection silently producedsend_message:draft).send_optincheckbox.set_account_send(telegram_id, enabled)(scopeadmin:users) wraps the existingStore.SetSendEnabled, so operators can toggle a per-account gate without a manual SQLUPDATE.2. ChatGPT stalled in the two-step send flow
ChatGPT called
prepare_send_message, then waited for a user turn instead of chainingsend_message; by the time it retried, the 5m token had expired.prepare_send_messageresponse now carries anext_stepfield instructing the model to callsend_messageimmediately with the same args +confirmation_id.Test plan
go build ./...,go vet ./...,gofmtclean on touched filesgo test ./...greenTestEnableUI_DefaultsToSendEnabledlocks in the send-default UITestEnableAccess_*(read-only + opt-in paths) still passNote: production account
user_id=384was already unblocked out-of-band via SQL while this lands.