Repository navigation
feat(bot): add /ticket-setup for support category and agent role - #1863
Conversation
📝 WalkthroughWalkthroughAdds ChangesTicket setup configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant TicketSetupCommand
participant GuildSettingsService
participant DiscordInteraction
Admin->>TicketSetupCommand: Run set, clear, or show
TicketSetupCommand->>GuildSettingsService: Persist or fetch guild settings
GuildSettingsService-->>TicketSetupCommand: Return result or current settings
TicketSetupCommand->>DiscordInteraction: Send ephemeral status embed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
e28f9b8 to
7bdccb3
Compare
Kimi review (
|
|
Addressed in 29df7b1. setGuildSettings now logs the original database error with the guild ID before returning false. The focused GuildSettingsService suite passes with 37 tests. The branch is up to date with main. |
There was a problem hiding this comment.
Really solid PR. The null versus undefined split is a genuine bug fix, not just tidying: GuildSettingsPatch narrows exactly the three nullable columns, the comment on djRoleId: null explains why it matters so the next person doesn't undo it, and writes null for nullable columns so clearers can disable features locks the behaviour in. Gating on ManageGuild and adding the guild id to the errorLog context are both right. I checked that toPrismaData already had copy('supportCategoryId') and copy('supportAgentRoleId') rather than assuming it, and it does, so the new columns persist correctly.
There's one thing I need fixed before this can go in.
/ticket-setup set role:@everyone makes every ticket public
The set branch takes any role with no validation:
const role = interaction.options.getRole('role', true)
await guildSettingsService.setGuildSettings(guildId, {
supportCategoryId: category.id,
supportAgentRoleId: role.id,
})Discord's role picker includes @everyone. That id then reaches buildTicketOverwrites (packages/bot/src/functions/general/commands/ticket.ts:156):
return [
{ id: guild.roles.everyone.id, deny: [PermissionFlagsBits.ViewChannel] },
{ id: requestorId, allow },
{ id: agentRoleId, allow }, // allow = ViewChannel, SendMessages, ReadMessageHistory
]A channel carries at most one overwrite per target id, so when agentRoleId is the everyone role the third entry replaces the first. The deny is gone and @everyone gets ViewChannel, SendMessages and ReadMessageHistory on every ticket created from that point on. Tickets are exactly where people paste account details and complaints about other members, so that's the worst channel in the server to accidentally open up.
I don't think this is an exotic mistake either. @everyone sits at the top of the role list and the option description ("Role granted access to every open ticket") reads like it could plausibly be the right answer.
Rejecting it in set is enough:
if (role.id === interaction.guildId) {
// @everyone would make every ticket world-readable
return
}Worth considering whether buildTicketOverwrites should defend itself too, since a stored value can predate this command, but the command-level guard is the part I'd want in this PR.
Two optional extras while you're in there:
role.managedroles can't be assigned to members, so picking one produces a config that grants access to nobody and fails silently. A check with a clear error would save someone an afternoon.setstorescategory.idwithout checking the bot hasManageChannelsthere. Today that surfaces much later at/ticket openas a channel-creation error with nothing pointing back at setup. Probing at configure time turns it into an immediate, actionable message.
On the red checks: not yours. Security was an unpassable repo-wide gate, kimi-review fails on every PR because an API key isn't set (#1877), and the npm ci errors come from a stale lockfile on main. Fixed in #1876; rebase once it lands.
|
Heads-up: the CI blockers I mentioned are fixed on What that clears:
Please rebase onto The review feedback above is separate and still stands. |
a6fb6ac to
f371aa3
Compare
|
Addressed in 94342a0 (squashed, rebased onto main).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/functions/general/commands/ticket-setup.ts`:
- Around line 106-126: Update the permission validation around
interaction.guild.members.me so an unresolved bot member is handled as a setup
failure rather than bypassing the check. Require a resolved me member and verify
it has ManageChannels in categoryIdForPerms before proceeding; otherwise use the
existing interactionReply error path and return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22dea0e2-5df1-43db-b579-7c79837e6c37
📒 Files selected for processing (8)
packages/bot/src/functions/general/commands/ticket-setup.spec.tspackages/bot/src/functions/general/commands/ticket-setup.tspackages/bot/src/functions/general/commands/ticket.tspackages/bot/src/functions/music/commands/djrole.spec.tspackages/bot/src/functions/music/commands/djrole.tspackages/shared/src/services/GuildSettingsService.spec.tspackages/shared/src/services/GuildSettingsService.tspackages/shared/src/services/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shared/src/services/index.ts
- packages/bot/src/functions/music/commands/djrole.ts
- packages/bot/src/functions/music/commands/djrole.spec.ts
- packages/shared/src/services/GuildSettingsService.spec.ts
- packages/shared/src/services/GuildSettingsService.ts
f371aa3 to
94342a0
Compare
LucasSantana-Dev
left a comment
LucasSantana-Dev
left a comment
There was a problem hiding this comment.
Review: approve with nits
The command is correct, well-guarded, and auto-registers cleanly.
P2 — missing test for the new overwrite guard: ticket.ts:166-178 adds the agentRoleId !== guild.roles.everyone.id guard in buildTicketOverwrites, and the PR summary advertises "ensure overwrites never grant @everyone" as a bug fix, but only the setup-side rejection is tested. The defense-in-depth branch for stale configs (set via the generic settings API before this guard existed) is exercised by nothing; a regression here silently re-opens the public-ticket hole. Suggest a ticket.spec.ts case with agentRoleId === guild.roles.everyone.id asserting only 2 overwrites are produced.
P3 (nits): in the set branch, when interaction.guild?.members.me is uncached (me is null), the Manage Channels pre-check is silently skipped and the config is saved anyway — not a failure since /ticket open catches the missing permission at channel-create time (ticket.ts:82-92), but a warning would be friendlier. Also const categoryIdForPerms = category.id is a one-use alias; inline it.
What's good: the djRoleId: undefined → null fix in djrole.ts:73 repairs a real latent bug — toPrismaData strips undefined (GuildSettingsService.ts:158), so /djrole clear previously never cleared the column — and it ships with a dedicated null-write test. The @everyone guard uses the correct Discord identity (role.id === guildId). Registration needs no plumbing: getCommandFiles auto-discovers the new file while excluding *.spec.*.
Adds set/clear/show for support category and agent role, with null (not undefined) clears so tickets can actually be disabled. Logs guild settings write failures. Rejects @everyone and managed roles, checks Manage Channels at configure time, and defends buildTicketOverwrites against a world-readable agent overwrite.
94342a0 to
f986646
Compare
|
Addressed the nits.
Rebased onto main. |
## Summary Two structural failures block every fork PR's required checks (seen on #1863, #1864, #1865, #1866, #1867, #1674 after their CI was approved): - **SonarCloud Scan (required) hard-fails on forks**: fork PRs get no secrets, so `SONAR_TOKEN` is never present and the token-policy step exits 1. Now the sonar job is skipped for fork PRs (a skipped required check counts as passing). Same pattern deploy-staging already uses. - **danger 403s on forks**: `review-tools.yml` ran on `pull_request`, where the fork token is forced read-only and the comment POST fails with 403. Switched to `pull_request_target`; the reusable workflow checks out and executes base-repo code only (documented in the file header, same safety rule as the other target workflows). ## Test plan - [x] actionlint clean on both files - [ ] Next push to an external contributor PR: SonarCloud Scan shows skipped, danger posts its comment After merge I will update the seven open contributor branches to main so they pick this up. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Unblocks fork PRs by fixing CI gates for SonarCloud and `danger`. Fork PRs now pass required checks without secrets and get review comments. - Bug Fixes - Skip SonarCloud Scan on fork PRs to avoid failing when `SONAR_TOKEN` is unavailable (skipped required check counts as passing). - Run review tools on `pull_request_target` so `danger` can comment on forks; workflow executes base-repo code only. <sup>Written for commit 316f567. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1898?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/functions/general/commands/ticket.spec.ts`:
- Around line 132-140: Strengthen the permission assertion in the ticket command
test by checking the “everyone” overwrite’s deny collection contains
PermissionFlagsBits.ViewChannel and its allow collection does not contain that
flag. Replace the current defined-only assertion while preserving the existing
overwrite count and ID checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8596e0fa-f7c9-41ef-b986-b8650080a36f
📒 Files selected for processing (9)
packages/bot/src/functions/general/commands/ticket-setup.spec.tspackages/bot/src/functions/general/commands/ticket-setup.tspackages/bot/src/functions/general/commands/ticket.spec.tspackages/bot/src/functions/general/commands/ticket.tspackages/bot/src/functions/music/commands/djrole.spec.tspackages/bot/src/functions/music/commands/djrole.tspackages/shared/src/services/GuildSettingsService.spec.tspackages/shared/src/services/GuildSettingsService.tspackages/shared/src/services/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/bot/src/functions/music/commands/djrole.ts
- packages/bot/src/functions/music/commands/djrole.spec.ts
- packages/shared/src/services/index.ts
- packages/bot/src/functions/general/commands/ticket.ts
- packages/shared/src/services/GuildSettingsService.spec.ts
- packages/shared/src/services/GuildSettingsService.ts
- packages/bot/src/functions/general/commands/ticket-setup.spec.ts
- packages/bot/src/functions/general/commands/ticket-setup.ts
|
A note from the maintainer side: sorry this PR waited as long as it did for a proper review, and sorry for the rounds of branch updates and re-running checks today. The churn was on our side, not yours. Your PRs exposed real gaps in how this repo handled external contributions: CI runs sat in a silent approval queue, some gates could never pass on fork PRs (SonarCloud, danger), and the team had no notification when external PRs arrived. Those are all fixed as of today:
Your branch is up to date and the full suite is green. Thanks for the patience and for the contribution. External contributors are very welcome here. |
🤖 I have created a release *beep* *boop* --- <details><summary>2.38.0</summary> ## [2.38.0](v2.37.3...v2.38.0) (2026-07-27) ### Features * **bot:** add /ticket-setup for support category and agent role ([#1863](#1863)) ([3f4af39](3f4af39)) * **frontend:** per-action loading and connection gating on music controls ([#1866](#1866)) ([2dda60f](2dda60f)) * **frontend:** show stale progress when music SSE lags ([#1867](#1867)) ([4952e73](4952e73)) * **music:** surface recommendationReason in nowplaying and queue ([#1864](#1864)) ([960fd62](960fd62)) * **ops:** blue/green zero-downtime deploys — Phase 1 web tier ([#1786](#1786)) ([f5f7597](f5f7597)) ### Bug Fixes * **docker:** make compose stack boot from a fresh .env ([#1674](#1674)) ([babe0ef](babe0ef)) * **docker:** treat an empty db password as missing in compose guards ([#1881](#1881)) ([718c0ad](718c0ad)) * **frontend:** make landing page usable at mobile widths ([#1865](#1865)) ([6190350](6190350)) * **frontend:** stop hero grid columns overflowing on narrow viewports ([#1874](#1874)) ([ce5cea0](ce5cea0)) * **invite:** add /invite where cloudflare pages reads it ([#1895](#1895)) ([0528f66](0528f66)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Description
Tickets (
/ticket open) already readsupportCategoryIdandsupportAgentRoleIdfrom GuildSettings, but there was no first-class way to set them. Admins had to use the generic settings API.Fix
/ticket-setupcommand withset,clear, andshowsubcommands (same shape as/djrole).settakes a category channel and an agent role, then persists both viaguildSettingsService.setGuildSettings./ticket openerror now points admins at/ticket-setup set.Verification
/djrolecommand and GuildSettings fields already used by/ticket.Checklist
Destructive / irreversible interaction (Tier A)
Not applicable. No Discord message deletion, bans, or other destructive actions.
Feature-removal sweep
Not applicable. Additive command only.
Fixes #1804
Summary by cubic
Adds a ManageGuild-gated
/ticket-setupcommand to configure the ticket category and agent role with strong privacy guards./ticket opennow directs admins to/ticket-setup set. Fixes #1804.New Features
/ticket-setupwithset,clear, andshowto manage support category and agent role.Bug Fixes
@everyoneand managed roles; require Manage Channels in the chosen category; clear error when the bot member is uncached./ticket opennever grants@everyonein overwrites so stale configs stay private.guildIdwhen settings writes fail.Written for commit 817b856. Summary will update on new commits.
Summary by CodeRabbit
/ticket-setupwithset,clear, andshowto configure support ticket category and agent role.@everyoneand managed/integration roles) and checks required category permissions./ticket-setup set.@everyone./ticket-setup, guild settings null/error handling,/ticket open, and DJ role clearing.