[mashkovd]

Summary

• The $chatgpt-app-submission readiness check blocked generation because the MCP tool annotation hints didn't match inspected behavior.
• Root cause: mcp-go (v0.54.0) defaults readOnlyHint=false, destructiveHint=true, openWorldHint=true (not nil), so any tool that only set readOnlyHint (or nothing) silently shipped destructive=true, openWorld=true — wrong for local-only, non-destructive operations.
• Set the three hints explicitly on the 12 affected tools; added TestToolAnnotations to lock the values against future default drift.

Annotation values (readOnly / destructive / openWorld)

Tool	Value
list_dialogs, get_unread_messages, get_messages, prepare_pin_message, list_telegram_identities, get_user_audit_log	false / false / false
get_my_audit_log	true / false / false
disconnect_telegram_account, delete_telegram_account, revoke_telegram_session, set_telegram_access, set_account_send	false / true / false
send_message, pin_message	false / true / true (unchanged — already aligned)

This is metadata-only; no runtime behavior change.

Note — send_message left as-is (flagged)

toolSendMessage has a comment claiming destructiveHint was omitted on purpose because "ChatGPT blocks destructive-annotated tools before the server call." Because mcp-go defaults destructive=true, the tool ships destructive=true anyway — the comment's intent was never realized. The submission check treats it as aligned at destructive=true, and the Apps SDK docs don't confirm the "blocks the call" claim. Left unchanged here; recommend verifying empirically in ChatGPT Developer Mode whether a real send is invokable, and fixing separately if it isn't.

Test plan

• go build ./...
• go test ./internal/mcp/... (incl. new TestToolAnnotations, 14 subtests)
• After merge + deploy, re-run $chatgpt-app-submission and confirm it no longer blocks; verify tools/list annotations on tg.mctl.ai

[claude]

Claude finished @mashkovd's task in 4m 2s —— View job

---

Code Review

• Gather context
• Read diff and changed files
• Analyze correctness, security, tests
• Post inline comments
• Submit review verdict

---

Verdict: Changes Requested — 1 P2, 2 P3

P2 — readOnlyHint regression on read-only query tools (tools.go lines 157, 351, 694, 824 / annotations_test.go line 28)

list_dialogs, get_messages, list_telegram_identities, and get_user_audit_log were correctly annotated readOnly=true before this PR. The change to readOnly=false is a semantic regression — these tools do not modify state. The root issue was only that destructive and openWorld were silently defaulting to true; fixing those two without touching readOnly is the right approach. Evidence: get_my_audit_log (the user-scoped counterpart of get_user_audit_log) correctly keeps readOnly=true with the new destructive=false, openWorld=false hints added alongside.

Suggested fix for each of the four tools:

mcplib.WithReadOnlyHintAnnotation(true),   // keep
mcplib.WithDestructiveHintAnnotation(false),
mcplib.WithOpenWorldHintAnnotation(false),

Test table rows for those four should then change to readOnly: true.

---

P3 — send_message and pin_message have no explicit readOnlyHint (annotations_test.go line 35)

Both tools rely on mcp-go's default (readOnly=false). Explicit annotation would be self-documenting and resilient to library default changes.

P3 — stale / misleading comment on send_message (tools.go lines 264–266)

The comment claims destructiveHint was omitted so ChatGPT can't invoke the tool, but mcp-go defaults destructive=true regardless — the intent was never realized. This is acknowledged in the PR description; clean it up in a follow-up.

---

What looks good:

• assertHint correctly uses t.Fatalf for nil before dereferencing, preventing a panic in the subtest.
• The first helper is clean and idiomatic.
• The test structure with 14 named subtests gives clear failure messages.
• Destructive / mutation tools (disconnect, delete, revoke, set_access, set_account_send) correctly get destructive=true, openWorld=false.
• prepare_pin_message at readOnly=false is reasonable (it creates a short-lived confirmation token).

[mashkovd]

@claude review

[claude]

Claude finished @mashkovd's task in 3m 45s —— View job

---

Review complete

• Gather context and read changed files
• Review internal/mcp/tools.go changes
• Review internal/mcp/annotations_test.go new file
• Post inline findings
• Submit verdict

---

Approved — 0 P1, 0 P2, 3 P3.

Findings

P3 — openWorldHint=false is semantically off for tools making Telegram API calls (line 159)
list_dialogs, get_unread_messages, and get_messages all make outbound MTProto calls to Telegram's servers, which matches the MCP spec definition of "open world." send_message and pin_message are correctly set to openWorldHint=true. Setting false here is inconsistent — if it's intentional to satisfy the submission check, a brief comment explaining the deviation would prevent future confusion.

P3 — prepare_pin_message annotation contradicts its own description (line 410)
readOnlyHint=false is set, but the tool description says "The prepare step is read-only." The tool only writes a short-lived confirmation token to server-side state, which could reasonably be called a side effect — but the docs claim otherwise. One of them should be corrected.

P3 — pre-existing stale comment on toolSendMessage (lines 264–266, unchanged by this diff)
The comment claims destructiveHint was omitted so ChatGPT won't block the call. After this PR, every other tool has explicit three-hint coverage; send_message remains the only tool that relies on mcp-go defaults for all three hints. Since mcp-go defaults destructive=true, the comment's intent ("omit to avoid destructive=true") was never realized. The PR description already flags this for follow-up — suggest resolving it before the next App Store submission cycle.

Positive notes

• The root-cause analysis (mcp-go defaults are non-nil, not nil) is correct and well-explained.
• TestToolAnnotations is a solid regression guard: it checks all three hints for all 14 tools, uses t.Fatalf on nil to catch a library default-to-nil regression, and will fail loudly if any value drifts.
• The first() helper neatly discards the handler without needing a blank identifier.

[claude]

P2 — readOnlyHint regression for read-only tools

list_dialogs does not modify any state — it only reads dialog metadata from the Telegram client. Before this PR it was correctly annotated readOnly=true. The change to readOnly=false is a semantic regression: MCP clients (and future ChatGPT tooling) that honour readOnlyHint will now incorrectly treat this tool as one that may modify state.

The root cause was that destructive=true, openWorld=true were silently defaulting in. The correct fix is to keep readOnly=true and add the missing explicit hints:

Suggested change

	mcplib.WithReadOnlyHintAnnotation(false),
	mcplib.WithReadOnlyHintAnnotation(true),
	mcplib.WithDestructiveHintAnnotation(false),
	mcplib.WithOpenWorldHintAnnotation(false),

The same issue applies to get_messages (line 351), list_telegram_identities (line 694), and get_user_audit_log (line 824) — all genuine read-only queries changed from true to false. Evidence of the correct pattern: get_my_audit_log (line 640) kept readOnly=true while adding the missing hints.

get_unread_messages and prepare_pin_message are debatable (get_unread_messages could plausibly trigger a read-receipt in some contexts; prepare_pin_message creates a short-lived token) so leaving those as readOnly=false is reasonable.

[claude]

P3 — openWorldHint=false is semantically inconsistent with external Telegram calls

list_dialogs (and likewise get_unread_messages at line 212, get_messages at line 353) makes outbound MTProto calls to Telegram's infrastructure. Per the MCP spec, openWorldHint=true means the tool "may interact with an open world of external entities" — which is exactly what these tools do.

send_message and pin_message are correctly annotated openWorldHint=true. Setting false here creates an inconsistency: a client could use these hints to decide whether to sandbox or log outbound traffic, and false would be misleading.

This may be intentional to pass the ChatGPT App submission check (which appeared to treat openWorldHint=false as correct for these tools). If so, add a brief comment explaining why the annotation differs from the semantic meaning of the hint.

[claude]

P3 — annotation contradicts the tool's own description

readOnlyHint=false is set here, but the tool description immediately below says: "The prepare step is read-only."

If prepare_pin_message truly has no side effects (it only issues a short-lived confirmation token stored server-side), it should have readOnlyHint=true to match get_my_audit_log and the documentation. If there is a side effect that makes it non-read-only, the description should be updated to remove the "read-only" claim.

[claude]

P3 — send_message and pin_message rely on mcp-go defaults for readOnlyHint

The test asserts readOnly=false for both tools, but neither toolSendMessage nor toolPinMessage sets WithReadOnlyHintAnnotation explicitly — the value is inferred from the mcp-go library default. If the library ever changes its default to nil (or true), this assertion would catch it via the nil-guard in assertHint, but the failure message would be confusing.

Also note: toolSendMessage still carries the comment "destructiveHint omitted: ChatGPT blocks destructive-annotated tools…" (tools.go line 264-266), which the PR description correctly identifies as a falsehood — destructive=true ships via default. That stale comment is worth cleaning up in a follow-up.

[claude]

P2 — test table encodes the annotation regression

get_user_audit_log is tested here with readOnly=false. Its counterpart get_my_audit_log (line 29) correctly uses readOnly=true. These are semantically equivalent read operations (one user-scoped, one admin-scoped) — the inconsistency is a sign that the readOnly=false values on the query tools are an oversight rather than intent.

If the P2 on list_dialogs (tools.go line 157) is fixed, the expected values for list_dialogs, get_messages, list_telegram_identities, and get_user_audit_log in this table should all be updated to true.

[claude]

Has P1/P2 findings, changes requested: 1 P2 (readOnly=false regression on genuinely read-only tools), 2 P3 nits. See inline comments for details.

[claude]

No P1/P2 findings (3 P3). Good to merge.

P3 nits (non-blocking):

1. openWorldHint=false on list_dialogs / get_unread_messages / get_messages is semantically inconsistent — these tools make outbound MTProto calls, matching the MCP spec definition of open-world. If the value is intentional for the ChatGPT App submission check, add a comment explaining the deviation (inline at line 159).
2. prepare_pin_message has readOnlyHint=false but its description says 'The prepare step is read-only.' — the annotation and docs contradict each other (inline at line 410).
3. Pre-existing (not introduced by this diff, but worth tracking): the comment on toolSendMessage (lines 264-266) claims destructiveHint was omitted to prevent ChatGPT from blocking the call, but mcp-go defaults destructive=true so the tool ships that value anyway. After this PR, send_message is the only tool without explicit three-hint coverage. The PR description already flags this for follow-up; suggest resolving that TODO before the next App Store submission cycle.

[mctl-claude-remote]

@claude review