Skip to content

fix(discord): check requester access before the bot answers - #9474

Merged
iscekic merged 2 commits into
mainfrom
drain/discord-bot-reply-authorization
Nov 1, 2026
Merged

iscekic merged 2 commits into
mainfrom
drain/discord-bot-reply-authorization

Conversation

@iscekic

@iscekic iscekic commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — local dev stack at head 8f70ad2 (real Next.js route, Postgres, Redis) with the Discord REST API answered by a loopback fixture that records every request. A forwarded gateway mention is delivered to /discord/webhook, and the bot's posted reply is read back from the fixture ledger.

  • A mention from an author the authorization helper reports a reason for: the bot posts DISCORD_SESSION_DENIAL_MESSAGES[reason] for that reason. Reaction lifecycle is unchanged: hourglass added, removed, check mark added.
  • For an unavailable account, that reply is one general message with the connected-accounts page and Kilo support as the next step. Observed at 8f70ad2.
  • The verification on 82c0493 (the branch's first commit) covered one requester per denial reason plus the installation owner, whose mention reached the fixture chat-completion proxy with the Cloud Agent tool present (toolCount 1).
  • Jest: NODE_ENV=test pnpm exec jest src/lib/discord-bot in apps/web — 7 suites, 71 tests pass.
  • Typecheck pnpm exec tsgo --noEmit --checkers 1 in apps/web, oxlint and oxfmt on the changed files: clean.

No visual surface changed (server-only route behavior), so no screenshots are attached. Design gate does not apply: no apps/mobile UI change.

Change

apps/web/src/lib/discord-bot.ts: processDiscordBotMessage resolves the installation authorization first and returns DISCORD_SESSION_DENIAL_MESSAGES[reason] as the reply when authorizeDiscordSessionRequester reports a reason.

apps/web/src/lib/discord-bot/discord-session-authorization.ts: the reply for an unavailable account is one general message that keeps the connected-accounts page and Kilo support as the next step; the precise reason stays in the server log. The other denial reasons keep their specific guidance.

DISCORD_SESSION_DENIAL_PROMPT_NOTICES and its four notice constants are removed, because the guidance is the reply itself.

apps/web/src/lib/discord-bot.test.ts covers each reason authorizeDiscordSessionRequester can return, a message with no Discord author context, and the account states behind the general message, asserting the exact guidance reply and no model call. The prompt-context test links the installation owner so it still exercises prompt assembly.

Production prerequisites

None.

Proxy answers

  1. Question: Whether the access decision covers every reason the helper reports.
    Answer: Every reason.
    Confidence: 0.72
  2. Question: Where the access decision sits in the reply preparation.
    Answer: First.
    Confidence: 0.75
  3. Question: Whether the unused notice constants are deleted.
    Answer: Deleted.
    Confidence: 0.85
  4. Question: How the reply for an unavailable account should read, given that the reply is posted in the channel.
    Answer: One general message that keeps the support path.
    Confidence: 0.70

The Discord bot resolves the author's access to the installation before it
prepares an answer for a mention. A requester with access continues to the
model with the Cloud Agent tool and the repository context. Every other
requester receives the guidance reply for their case.

The denial-only system-prompt notices are removed because the guidance is
now the reply itself.
@iscekic iscekic added drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening manual Work item from a manually filed finding labels Nov 1, 2026
Comment thread apps/web/src/lib/discord-bot.ts Resolved
@kilo-code-bot

kilo-code-bot Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of HEAD 8f70ad2 found no new defects: the only changes since 82c0493 replace the account_unavailable denial text with a generic message and add a test asserting it does not disclose the account state (blocked/disabled), which correctly preserves the channel-visible privacy boundary.

Assumptions: [INFERENCE] The new ACCOUNT_UNAVAILABLE_MESSAGE points blocked or disabled accounts to /connected-accounts or Kilo support; the wording is generic enough that it does not reveal whether the account is blocked, suspended, or self-disabled, and the precise reason stays in server logs. insertTestUser accepts blocked_at and personal_account_disabled (verified in packages/web-shared/src/tests/helpers/user.helper.ts), so the two new parametrized cases exercise real denial paths. The earlier SUGGESTION at apps/web/src/lib/discord-bot.ts:318 (denial replaces the model reply for every mention) still exists in code but was declined by the author as intentional scope, so it is not re-raised. Tests and typecheck were not executed in this read-only review.

Files Reviewed (3 files)
  • apps/web/src/lib/discord-bot.test.ts - new it.each coverage for blocked and personal-account-disabled requesters; asserts runBot is not called and the reply contains no block|disabl|suspend|bann term
  • apps/web/src/lib/discord-bot/discord-session-authorization.ts - ACCOUNT_UNAVAILABLE_MESSAGE introduced and wired into DISCORD_SESSION_DENIAL_MESSAGES; APP_URL already imported; other denial messages unchanged
  • apps/web/src/lib/discord-bot.ts - unchanged since previous review; authorization early return and tool executor still consistent
Previous Review Summaries (2 snapshots, latest commit 82c0493)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 82c0493)

Status: No New Issues Found | Recommendation: Merge

Executive Summary

Re-reviewed HEAD 82c0493 across the three changed files: the authorization check is correctly hoisted ahead of model selection, prompt assembly, and the spawn_cloud_agent executor, with matching refusal tests; no new defects found.

Assumptions: [INFERENCE] The earlier SUGGESTION on apps/web/src/lib/discord-bot.ts:318 (a denial now replaces the model reply for every mention, including question-only traffic) is still present in code but was explicitly declined by the author as intentional scope; it is not re-raised here, so whether the bot should keep answering session-less questions is a product decision for the human reviewer. The claim that denial paths keep the hourglass/remove/complete reaction lifecycle unchanged relies on apps/web/src/app/discord/webhook/route.ts:149, outside this diff. Tests and typecheck were not executed in this read-only review.

Files Reviewed (3 files)
  • apps/web/src/lib/discord-bot.ts - authorization resolved before model config, prompt assembly, and tool execution; denial returns DISCORD_SESSION_DENIAL_MESSAGES[reason]; narrowing of authorization.kiloUserId inside the tool executor is valid for the const union
  • apps/web/src/lib/discord-bot/discord-session-authorization.ts - prompt-notice constants removed; DISCORD_SESSION_DENIAL_MESSAGES unchanged; no remaining references to the deleted export
  • apps/web/src/lib/discord-bot.test.ts - refusal coverage per denial reason plus missing author context; owner paths updated to link an account

Previous review

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
apps/web/src/lib/discord-bot.ts 318 Early denial replaces the model reply for every mention, so unlinked or non-owner guild members can no longer get answers to questions that need no Cloud Agent session (previously the model answered with tools: []).
Files Reviewed (3 files)
  • apps/web/src/lib/discord-bot.ts - 1 issue (authorization check moved ahead of model configuration and tool calls; denial path returns fixed guidance)
  • apps/web/src/lib/discord-bot/discord-session-authorization.ts - removed unused prompt-notice constants; denial messages unchanged
  • apps/web/src/lib/discord-bot.test.ts - refusal coverage added per denial reason and for missing author context

Fix these issues in Kilo Cloud


Reviewed by deepseek-flash-latest · Input: 34.6K · Output: 3.3K · Cached: 275.6K

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human Substantive functional risk requires human review. labels Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Re: the kilobot Code Review Summary (1 Issue Found, SUGGESTION on apps/web/src/lib/discord-bot.ts:318).

That row is the same finding as the inline thread on that line, and it is answered there: the access decision runs ahead of reply preparation by intent of this change, and answering session-less questions separately is a separate product decision that needs its own scope. The thread is resolved and no code change follows from the summary row.

The wording observation from that remark is noted for such a follow-up; each reason already has its own guidance text, so it reads as the answer for its case.

@iscekic iscekic removed the merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head label Nov 1, 2026

@eshurakov eshurakov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot - Minor (non-blocking): the refusal reply is posted publicly in the channel, and account_unavailable states that the linked Kilo account "cannot start Cloud Agent sessions" for blocked_at, blocked_reason, or personal_account_disabled. Any guild member can now learn the state of another member's Kilo account by having them trigger a mention. Consider posting the account_unavailable denial as a DM (or a generic public message) while keeping the detailed guidance for the other reasons.

A requester whose Kilo account cannot start sessions gets one general guidance
message that points to the connected-accounts page and Kilo support. The
precise denial reason stays in the server log, and the other denial reasons
keep their specific guidance.
@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:blocked:insufficient-e2e Verification does not cover the change labels Nov 1, 2026
@iscekic

iscekic commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Addressed the approving review's minor remark. The reply for an unavailable account is one general message that keeps the connected-accounts page and Kilo support as the next step; the precise reason stays in the server log, and the other denial reasons keep their specific guidance.

Verified on the local dev stack at 8f70ad2 by delivering a forwarded gateway mention: the bot posts that general message and the run log records the reason. Jest covers the account states behind that message.

@iscekic
iscekic added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 56e8f5c Nov 1, 2026
88 checks passed
@iscekic
iscekic deleted the drain/discord-bot-reply-authorization branch November 1, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening manual Work item from a manually filed finding merge:human Substantive functional risk requires human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants