Repository navigation
feat(bot): report a member's cached voice channel - #385
Conversation
Add Behavior::Voice with mod-gated !voice /voice that reports a member's voice channel from the gateway cache without REST roster fetches.
|
@pullfrog review |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two correctness issues need fixing: negative lookup results are rendered as in-channel successes, and initialized guilds with nobody in voice are classified as uncached.
Reviewed changes across the full PR, including the command pipeline and the linked member-tool requirement in #159.
- Command integration: Added the moderator-level
voicedefault, shared behavior dispatch, and required slash-command user option. - Cached lookup: Added owned voice-state snapshots, channel lookup, name clipping, and separate absence/unavailable outcomes without voice-location REST requests.
- Documentation and tests: Documented prefix/slash usage and added cache fixtures, permission-default checks, and a local no-HTTP behavior test.
Validation: cargo test -p jmodguard-bot commands::voice_lookup --locked passed all 10 selected tests. cargo fmt --all -- --check failed on the added code, consistent with the failed quick CI check. Full preflight and live Discord behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
|
@pullfrog review |
There was a problem hiding this comment.
Important
The response-rendering issue is fixed, but the earlier empty-voice-map finding remains: a fully initialized guild with nobody in voice is reported as unavailable. The quick CI check also still fails on formatting in the new response test.
Reviewed changes across all six changed files, both commits, and the command/cache integration for the member-tool requirement in #159.
- Command integration: Adds the moderator-level
!voice//voicecommand, required user option, and shared behavior dispatch. - Cache lookup: Adds owned voice snapshots, channel-name clipping, and distinct in-channel, absent, and unavailable responses without voice-location REST calls.
- Response correction: Uses the existing dynamic-response convention so negative results reach the final renderer; the earlier rendering thread is verified and resolved.
- Documentation and coverage: Adds command documentation, cache fixtures, a local no-HTTP behavior test, and final-renderer regression coverage.
Validation: all 10 tests selected by cargo test -p jmodguard-bot commands::voice --locked passed, as did voice_not_cached_and_not_in_channel_replies_reach_operators. cargo fmt --all -- --check failed in crates/bot/src/commands/mod.rs, matching the quick CI job's logs. These tests use local Serenity/cache and HTTP fixtures; full preflight and live Discord behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
Initialized guilds with no voice states now answer not-in-channel instead of not-cached; cover VoiceStateUpdate leave via cache and update command docs.
|
@pullfrog review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes across the complete PR and its command dispatch, response rendering, cache lifecycle, and default-command seeding for the member-tool requirement in #159.
- Command integration: Adds moderator-level
!voiceand/voicecommands with a required user argument and shared behavior dispatch. - Cached lookup: Reports owned channel-name/ID snapshots, known absence, or unavailable cache data without Discord REST lookups, with bounded channel-name output.
- Response and lifecycle corrections: Preserves negative replies through the final renderer and correctly classifies initialized empty voice maps and the last member leaving; the earlier findings are verified fixed and their remaining threads resolved.
- Documentation and coverage: Documents both command forms and adds Serenity cache-event fixtures, default-permission checks, response-rendering coverage, and a local no-HTTP behavior test.
Validation: all 11 tests selected by cargo test -p jmodguard-bot commands::voice --locked passed, as did voice_not_cached_and_not_in_channel_replies_reach_operators and cargo fmt --all -- --check. Current GitHub checks, including CI passed, are successful. Local tests use Serenity cache events and local HTTP/WebSocket fixtures; full local preflight and live Discord behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 4 other issues
Security issues:
- Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bot/src/commands/behaviors.rs" line_range="599" />
<code_context>
+ channel_id,
+ channel_name,
+ } => {
+ vars.insert("channel".into(), channel_name);
+ vars.insert("channel_id".into(), channel_id.to_string());
+ Some(crate::templates::render(
</code_context>
<issue_to_address>
**Hidden voice-channel membership exposed**
When a moderator without visibility permission for a cached voice channel queries a member who is in that channel, the Voice behavior returns the channel name and ID without checking whether the invoking moderator can view that channel, exposing a member’s presence in restricted voice channels.
Before returning the channel details, check the invoking member’s effective permissions for that channel and avoid disclosing its name or ID when they lack view access.
</issue_to_address>
### Comment 2
<location path="crates/bot/src/commands/behaviors.rs" line_range="587" />
<code_context>
+ Behavior::Voice => {
+ let mut args = env.args.clone();
+ let target =
+ resolve_target(env, args.next_arg().ok_or_else(|| anyhow!("missing user"))?)
+ .await?;
+ let target_user = user_lookup(env, target).await?;
</code_context>
<issue_to_address>
**Voice lookup selects the wrong member**
When a prefix argument matches multiple cached members by username, nickname, or global name, `resolve_target` selects the first matching cached member, so `Behavior::Voice` looks up that member and can report the wrong person’s voice channel.
Reject ambiguous matches or require a uniquely identified target before looking up the voice state.
</issue_to_address>
### Comment 3
<location path="src/defaults.rs" line_range="260" />
<code_context>
+ "Report which voice channel a user is in (gateway cache).",
+ "!voice <user>",
+ "mod",
+ Behavior::Voice,
+ "",
+ "",
</code_context>
<issue_to_address>
**Older bots reject seeded commands**
When old and new bot binaries overlap while sharing the database, the seeder writes `Behavior::Voice` to the shared command registry, but older binaries cannot deserialize that value, so command-configuration reads fail and users see command errors.
Delay seeding the new behavior until older binaries are no longer reading the shared registry.
Also at `src/types.rs:327`.
</issue_to_address>
### Comment 4
<location path="crates/bot/src/commands/voice_lookup.rs" line_range="51" />
<code_context>
+ let Some(channel_id) = state.channel_id else {
+ return VoiceLookupSnapshot::NotInChannel;
+ };
+ if !channel_belongs_to_guild(cache, guild_id, guild, channel_id) {
+ return VoiceLookupSnapshot::NotCached;
+ }
</code_context>
<issue_to_address>
**Cached voice states appear missing**
When the guild and voice state are cached but the referenced channel is absent from cached channel maps, `channel_belongs_to_guild` returns false, so `snapshot_voice_state` returns `NotCached` despite the cached voice state; `!voice` reports that the state is not cached instead of providing the known channel ID or an appropriate unavailable-channel result.
Handle a missing cached channel separately from a missing guild or voice state, returning the known channel ID or an appropriate unavailable-channel result.
Also at `docs/COMMANDS.md:25`, `crates/bot/src/commands/voice_lookup.rs:53`.
</issue_to_address>
### Comment 5
<location path="crates/bot/src/commands/voice_lookup.rs" line_range="345" />
<code_context>
"ws://{}",
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and the command grants every user with the existing mod permission a way to retrieve any member’s cached voice channel, including its name and ID; if that access level is too broad, the policy is wrong for every guild as soon as it ships. Reverting removes the command, but it cannot undo channel information already disclosed in replies.
Blocking findings: crates/bot/src/commands/behaviors.rs:599, crates/bot/src/commands/behaviors.rs:587, src/defaults.rs:260, crates/bot/src/commands/voice_lookup.rs:51, crates/bot/src/commands/voice_lookup.rs:345
…rades - Hide channel name/id when the actor cannot VIEW_CHANNEL - Refuse ambiguous display-name matches; resolve mentions/IDs without cache - Load voice from binary overlay merge instead of global seed row - Split guild-not-cached vs channel-unavailable reply wording
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bot/src/commands/behaviors.rs" line_range="584" />
<code_context>
suppress_response: false,
})
}
+ Behavior::Voice => {
+ let mut args = env.args.clone();
+ let token = args.next_arg().ok_or_else(|| anyhow!("missing user"))?;
</code_context>
<issue_to_address>
**Voice replies wait on database**
When the database enrollment query stalls or its connection pool cannot provide a connection, `run` awaits `cases::capture` before matching `Behavior::Voice`; a stalled enrollment query therefore blocks this cache-only lookup from replying until the database call returns.
Skip enrollment capture for read-only behaviors such as voice, or bound the enrollment lookup with a timeout.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and a moderator can request a member’s cached voice channel, and the response may be visible to everyone who can read the command channel. If the visibility check or intended disclosure policy is wrong, channel information is exposed and reverting the command cannot retract it.
Blocking findings: crates/bot/src/commands/behaviors.rs:584
Satisfy clippy items_after_test_module on lib test targets (rust-core CI).
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. The command can post a member’s cached voice-channel name and ID into a Discord channel, relying on the moderator gate and channel-view permission check to prevent inappropriate disclosure. Reverting stops future reports, but any private channel or occupancy information already posted cannot be recalled.
There was a problem hiding this comment.
Caution
Prefix voice lookups can bypass member-specific channel visibility denies and disclose restricted channel names and IDs. The required CI gate also fails on the newly added test module.
Reviewed changes across the complete PR, its prefix/slash dispatch and configuration paths, and the member-tool requirement in #159.
- Command integration: Adds moderator-level
!voiceand/voice, a required user argument, dynamic template responses, and ambiguity-aware cached name resolution. - Cached visibility: Adds owned voice-state snapshots, channel visibility checks, bounded names, and separate missing-guild, absent-member, and unavailable-channel outcomes.
- Binary defaults: Excludes
voicefrom shared global seeding and overlays it into effective configuration and slash-command reads. - Documentation and regression coverage: Documents both command forms and tests cache lifecycle, permissions, response rendering, HTTP avoidance, and database overlay loading.
- Validation: All 13 voice tests, the response-renderer regression, the database-overlay regression, and
cargo fmt --all -- --checkpassed. A temporary executable linked against the production snapshot function reproduced the visibility bypass with real Serenity cache objects and a Gateway-style partial member. Currentrust-coreCI fails withitems after a test module; full local preflight and live Discord behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Caution
The prefix-command path can bypass member-specific voice-channel visibility denies. The binary overlay also still permits incompatible command behaviors to be persisted through ordinary configuration edits.
Reviewed changes across the complete PR, its command/configuration callers, and the member-tool requirement in #159.
- Command integration: Adds moderator-level
!voiceand/voice, a required slash user option, and ambiguity-aware cached name resolution. - Voice snapshots: Adds owned cache lookups, channel visibility checks, bounded channel names, and distinct absence/unavailable replies.
- Configuration loading: Excludes
voicefrom global seeding and overlays it into effective configuration and slash registries. - Documentation and tests: Documents the command and covers cache lifecycle, response rendering, permissions, HTTP-free behavior execution, and overlay loading.
Validation: 13 targeted voice tests, the final-response rendering regression, two core overlay tests, and cargo fmt --all -- --check passed. Standalone probes against the compiled production libraries reproduced both findings: the prefix-shaped member disclosed Secret while the corrected caller identity returned ChannelUnavailable, and a captured global command edit persisted {"kind":"voice"}. These checks used local Serenity/cache fixtures and SQLite, not live Discord or remote database acceptance; full local preflight was not run.
gpt-6.1-sol ｜ 𝕏
Prefer the cached guild member for the author over gateway partial payloads, normalize actor_member.user.id to env.actor, and pass actor id plus roles into voice lookup. Reject persisting binary-overlay voice rows.
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bot/src/commands/mod.rs" line_range="2603-2607" />
<code_context>
+ assert_ne!(from_gateway.user.id, UserId::new(AUTHOR));
+
+ let actor = UserId::new(AUTHOR);
+ let actor_member = cache
+ .guild(GuildId::new(GUILD))
+ .and_then(|g| g.members.get(&actor).cloned())
+ .or(Some(from_gateway))
+ .map(|member| super::actor_member_for_command(actor, member))
+ .expect("prefix dispatch must resolve a member");
+ assert_eq!(actor_member.user.id, actor);
</code_context>
<issue_to_address>
**Partial-member regression goes undetected**
When the guild cache contains the author member, as this test's fixture does, `prefix_voice_partial_member_with_member_deny_returns_channel_unavailable` uses the cached author member: lines 2603–2607 select it before `from_gateway`, so the partial payload is discarded. The test still passes if actor-ID normalization fails, leaving the prefix fallback path untested.
Remove the cached author member from this fixture so the test exercises the gateway partial-member fallback and its actor-ID normalization.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and a moderator can query a member’s cached voice state, and a permission-check mistake could disclose that the member is in a hidden channel or reveal its name and ID. Reverting stops future reports, but information already disclosed cannot be taken back.
Blocking findings: crates/bot/src/commands/mod.rs:2607
|
@pullfrog review |
Drop the cached author member from the fixture and assert the wrong partial user id would leak the channel before actor normalization.
There was a problem hiding this comment.
Important
The prefix visibility correction is addressed, but alternate-name saves still bypass the rolling-upgrade storage fence. The latest commit also fails the required formatting check.
Reviewed changes across the complete PR, including the latest partial-member test correction, command dispatch, configuration writers, and the member-tool requirement in #159.
- Command integration: Adds moderator-level
!voiceand/voice, required user selection, ambiguity-aware name resolution, and dynamic template responses. - Cached visibility: Adds owned voice snapshots, caller-specific channel permissions, bounded names, and distinct absent-member, missing-guild, and unavailable-channel outcomes.
- Binary defaults: Excludes
voicefrom global seeding, overlays it into effective configuration and slash registries, and adds name-based persistence guards. - Documentation and coverage: Documents the command and tests cache lifecycle, response rendering, channel visibility, HTTP avoidance, and overlay loading.
- Validation: All 17 tests selected by
cargo test -p jmodguard-bot voice --lockedand all three core tests selected bycargo test -p jmodguard voice --lockedpassed. A standalone probe using the production database writers reproduced alternate-name persistence of{"kind":"voice"}. Latest-headcargo fmt --all -- --checkfails at the new test assignment, andChecks / quickis failing. These checks use local SQLite and Serenity/cache/HTTP fixtures; full local preflight and live Discord or remote database acceptance were not exercised.
gpt-6.1-sol ｜ 𝕏
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. The new command exposes a member’s cached voice-channel membership, name, and ID to moderators who can view that channel. If the permission check or disclosure boundary is wrong, that information is already revealed and reverting cannot take it back.
There was a problem hiding this comment.
Caution
The new built-in overlay discards saved command access controls, re-enabling disabled moderation commands and removing owner, role, and channel restrictions. Legacy command collisions and three failing CI groups also remain.
Reviewed changes across the complete PR, its command/configuration callers, prior review context, and the member-tool requirement in #159.
- Voice command: Adds moderator-level
!voiceand/voice, required user selection, ambiguity-aware cached name resolution, and dynamic template responses. - Cached lookup: Adds owned voice snapshots, caller-specific channel visibility checks, bounded channel names, distinct absence/unavailable results, and no case-enrollment capture for voice.
- Shared authorization: Normalizes Gateway actor identity and prefers invocation-supplied member roles over cached roles.
- Configuration and coverage: Excludes voice from shared seeding, overlays shipped definitions into effective configuration and slash registries, adds persistence guards, and updates documentation and regression tests.
- Validation: The
voiceselection passed eight core and 18 bot tests;enforcement_passed six bot tests;builtin_passed nine core tests; formatting passed. A standalone probe linked against production libraries reproduced saved access-control loss and legacy voice shadowing in both configuration and slash loaders. The existing captured-configuration regression failed locally, and current core/API/remote-database CI logs confirm rejected fixtures. A separate slash-registration recheck exceeded the 120-second shell limit. Tests and probes use local SQLite, Serenity/cache objects, and loopback fixtures; full preflight and live-provider acceptance were not exercised.
gpt-6.1-sol ｜ 𝕏
Use custom_command_fixture for writable command seeds so reserved built-in labels are not cloned from ban. merge_binary_overlay_commands always inserts shipped voice; add load test that ignores stored guild voice overrides.
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. Applying shipped built-in definitions overrides stored command settings, which can change who is allowed to run moderation commands; an incorrect voice-channel visibility check could also disclose channel details in a response. Reverting restores the prior configuration behavior, but cannot undo commands run or information already disclosed while this change was active.
There was a problem hiding this comment.
Caution
Saved moderation access controls are still discarded when built-ins load, and legacy aliases other than voice can still route to the wrong command. The latest fixture corrections and deterministic voice precedence are present, but these two previously reported regressions remain.
Reviewed changes across the complete PR, its command/configuration callers, prior review context, and the member-tool requirement in #159.
- Voice command: Adds moderator-level
!voiceand/voice, required user selection, ambiguity-aware cached name resolution, and template-rendered outcomes. - Cached visibility: Adds owned voice snapshots, caller-specific channel permission checks, bounded channel names, separate absence/unavailable results, and no case-enrollment capture for voice.
- Shared authorization: Normalizes Gateway actor identity and prefers invocation-supplied roles over stale cached member roles.
- Configuration: Keeps voice out of shared database seeding, overlays shipped built-ins into runtime/slash configuration, rejects reserved-name/alias writes, and gives voice explicit lookup precedence.
- Documentation and fixtures: Documents both command forms and updates configuration regression fixtures to use writable custom commands.
- Validation: The
voiceselection passed 10 core and 18 bot tests; the compiled core harness passed 35 configuration/built-in tests; formatting passed. Production-library probes reproduced both findings using local SQLite and the real command lookup. The separate slash-registration recheck timed out; service-boundary validation stopped atfailed to download aws-lc-rs v1.18.1because--offline was specified. Current API, bot, and remote-database CI checks pass;CI passedremains red because the unchanged updater testordinary_exit_after_idle_keeps_health_during_stalled_controlfailed withleft: 1,right: 2. Full local preflight and live Discord behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
Keep write guards rejecting all built-in edits while apply_shipped_builtin_commands merges only matching behavior parameters and always overwrites overlay voice. Route find_command through builtin_command_for_label; slash tests use custom_command_fixture.
Use writable_command_fixture (name matches key, Say behavior) in enrollment writer tests. Strengthen merge_binary_overlay_commands regression for legacy voice rows.
Shipped command identity (name, description, aliases, behavior) stays authoritative while enabled, permission, roles, channels, and cooldown come from stored guild rows. Strip legacy alias shadow rows and align API effective reads with runtime enforcement.
Guilds cannot persist enabled, permission, role, or channel changes for shipped commands. Load still applies controls already stored from older rows.
|
@pullfrog review |
1 similar comment
|
@pullfrog review |
There was a problem hiding this comment.
Important
The dashboard configuration bundle still presents stored command definitions as effective even when runtime replaces them with shipped built-ins.
Reviewed changes Reviewed the full PR against main, including the voice command and the expanded built-in command policy.
- Voice lookup: Adds prefix and slash lookup using cached voice states, permission-aware channel disclosure, ambiguous-name handling, response templates, and command documentation.
- Caller enforcement: Uses the invocation's member roles and authoritative actor ID rather than stale cached roles or a placeholder user ID.
- Built-in policy: Adds the nonpersistent voice overlay, rejects reserved command writes, preserves existing stored access controls on load, resolves label collisions, and aligns individual command reads with runtime.
- Validation: Passed 18 bot voice tests, 13 core built-in tests, the focused API effective-read test, formatting, and service-boundary checks. A temporary HTTP-test assertion reproduced the bundle discrepancy and was removed afterward; the worktree is clean. Full local preflight and live Discord/provider behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Important
Confirmed the dashboard configuration-bundle inconsistency already reported in this review thread: commands[].effective can describe a different built-in behavior than the bot executes. No additional findings; I have not duplicated the inline comment.
Reviewed changes across the full PR for #159, including command dispatch, configuration readers and writers, tests, and the owner's clarifications on built-in write restrictions.
- Voice lookup: Adds moderator-level
!voiceand/voice, ambiguity-aware member resolution, owned cache snapshots, channel visibility checks, and distinct absent/unavailable replies without voice-location REST calls or case capture. - Shared authorization: Normalizes Gateway actor identity and prefers invocation-supplied member roles over stale cached roles.
- Built-in configuration: Excludes voice from shared seeding, applies shipped definitions while preserving stored access controls, reserves built-in names and aliases, and resolves legacy label collisions deterministically. The deliberate prohibition on new built-in saves is preserved.
- API and coverage: Aligns individual command GET results with runtime, updates writable custom-command fixtures, and adds command, cache, permission, persistence, and effective-read regression coverage plus usage documentation.
- Validation: The voice selection passed 10 core and 18 bot tests; built-in selection passed 13 core tests; enforcement selection passed six bot tests; 25 captured-configuration tests and the slash-registration regression passed. Formatting and the original targeted API test passed. A temporary extension of that API test reproduced the existing finding through the real Axum route: the bundle reported
saywhile individual GET returned shippedban. The probe was removed, the original test passed again, and the worktree is clean. CurrentCI passedis successful; full local preflight, browser behavior, and live-provider acceptance were not exercised. Local tests use SQLite, Serenity/cache objects, and loopback/auth fixtures.
gpt-6.1-sol ｜ 𝕏
Guild config and the command list keep raw scope rows, and set effective through the same shipped-builtin registry the bot loads. Built-in writes stay rejected.
|
@pullfrog review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes across the complete PR, its command/configuration callers, prior review threads and owner clarifications, and the member-tool requirement in #159.
- Voice command: Adds moderator-level
!voiceand/voice, ambiguity-aware cached user resolution, owned voice snapshots, channel visibility checks, and distinct absence/unavailable responses without voice-location REST requests or case capture. - Shared enforcement: Normalizes the actor identity from Gateway payloads and prefers invocation-supplied roles over stale cached member roles.
- Built-in definitions: Applies shipped definitions while preserving existing stored access controls, resolves reserved-label collisions deterministically, and rejects new built-in edits under the owner's stated policy.
- Configuration consistency: Keeps
voiceout of shared seed rows and aligns dashboard bundles, command-list/individual reads, runtime configuration, and slash registries with the effective command merge; the latest bundle correction addresses the earlier finding. - Documentation and coverage: Documents both command forms and adds cache-lifecycle, response-rendering, permission, persistence, overlay, and API regressions, with writable custom fixtures replacing built-in save fixtures.
- Validation: Passed 10 core and 18 bot tests selected by
voice, 13 core tests selected bybuiltin_, the invocation-role regressions, slash-registration/retry regression, API effective-command bundle regression, three individual API configuration-read regressions, formatting, and service-boundary checks. The PR'sCI passedgate is green. Local checks used SQLite, Serenity cache objects, and loopback HTTP/WebSocket fixtures; full local preflight and live Discord/provider acceptance were not run.
gpt-6.1-sol ｜ 𝕏
Migration 0021 deletes guild and global command rows whose name is a built-in primary or reserved alias. Load, the slash registry, and API effective reads use the shipped definition and shipped access.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Two CI/publication blockers remain: the migration's line endings fail the whitespace gate, and the schema bump breaks the publication contract fixtures.
Reviewed changes across the complete PR, its command/configuration callers, resolved review threads, and the member-tool requirement in #159.
- Voice lookup: Adds moderator-level prefix/slash lookup using owned Gateway-cache snapshots, actor-specific channel visibility, ambiguity-aware name resolution, and distinct absence/unavailable responses.
- Shared enforcement: Normalizes invocation actor identity and prefers invocation-supplied roles over cached member roles.
- Built-in authority: Uses shipped definitions and shipped access controls consistently in runtime, slash registration, and API effective reads, reserves built-in labels, and rejects built-in saves.
- Migration and compatibility: Adds core migration 0021 to remove legacy built-in/reserved-label rows while keeping
voiceout of shared seeding and preserving other custom rows. - Validation: All 10 core and 18 bot tests selected by
voice, plus 14 compiled core tests selected bybuiltin_, passed. Rust formatting and service-boundary checks passed. Both reported failures were reproduced locally; currentChecks / quickandCI passedare red. Tests use local SQLite, Serenity cache objects, and loopback fixtures; full local preflight and live-provider behavior were not exercised.
gpt-6.1-sol ｜ 𝕏
Store the built-in access migration as LF so whitespace checks pass, and advertise core schema 21 on publication labels. A !voice name or nickname with an empty guild cache uses the not-cached reply.
|
@pullfrog review |
There was a problem hiding this comment.
Important
The previous whitespace and publication blockers are fixed, but the dashboard still offers built-in command edits that the new API guard always rejects.
Reviewed changes across the complete PR, its prefix/slash dispatch, configuration callers, prior review threads, and the member-tool requirement in #159.
- Voice lookup: Adds moderator-level prefix/slash lookup using owned Gateway-cache snapshots, actor-specific channel visibility checks, ambiguity-aware name resolution, and separate absent/unavailable replies.
- Shared enforcement: Normalizes actor identity from partial members and prefers invocation-supplied roles over stale cached roles.
- Built-in authority: Uses shipped definitions and access controls for runtime, slash registration, and API effective reads; reserves shipped names/aliases and rejects built-in saves.
- Migration and compatibility: Adds core migration 0021 to remove legacy built-in/reserved-label rows, preserves other custom rows, and keeps the new voice behavior out of shared seeding.
- Publication corrections: Updates current schema labels and historical publication fixtures; both previous review threads were verified fixed and resolved.
- Validation: The
voiceselector passed 10 core and 19 bot tests; 14 compiled built-in tests, 28 configuration tests, invocation-role tests, the slash-registration regression, and the no-default-features API effective-read regression also passed. Formatting, whitespace, service-boundary, and publication-contract checks passed, andCI passedis green. A shallow Node VM probe of productionCommandEditorconfirmed the active built-in Save action; this was not browser testing. Local Rust tests used SQLite, Serenity cache objects, and loopback fixtures, not live Discord or remote-provider acceptance; full local preflight was not run.
gpt-6.1-sol ｜ 𝕏
| "command description must be 1-100 characters" | ||
| ))); | ||
| } | ||
| if crate::defaults::rejects_binary_overlay_command_persist(storage_name, cmd) { |
There was a problem hiding this comment.
[P2] Make built-in commands read-only in the dashboard
The dashboard still renders Edit for every built-in and opens a form offering permissions, restrictions, cooldowns, responses, and Save. With this unconditional rejection, the normal Commands > ban > Edit > Save flow can never succeed, even though the page explicitly advertises those settings as editable.
Technical details
# The command editor does not reflect built-in immutability
## Affected sites
- crates/api/src/web/api.rs:639-642 rejects every built-in save, including an unchanged shipped definition.
- frontend/app.js:2614-2616 still promises that every command can be renamed or have its responses, permissions, and cooldowns changed.
- frontend/app.js:2664 offers Edit for every command record without distinguishing shipped built-ins.
- frontend/app.js:2684-2961 builds the editable form from effective and sends Save to PUT /commands/{name}, which now always refuses built-ins.
## Evidence
- builtin_command_cannot_be_replaced_via_dashboard_writer passes and explicitly verifies rejection of enabled/permission/role/channel-only built-in saves.
- A shallow Node VM invocation of the actual production CommandEditor with a shipped ban record exposes active Save, Delete, and Cancel controls. The independent assertion that an immutable built-in must not offer active Save fails at that assertion.
- This probe uses lightweight hook/element stubs, not a browser, and does not claim an end-to-end HTTP interaction.
## Required outcome
- Keep the deliberate backend restriction; do not restore built-in writes.
- Present shipped commands as read-only, or otherwise remove unavailable mutation actions and explain their immutable status before users edit a draft.
- Keep custom command editing available and align the Commands-page wording with the distinction.
- Add focused frontend coverage for a built-in record alongside an editable custom record.
Part of #159 (Small member tools: which voice channel is this member in).
Part of #159 (Small member tools: which voice channel is this member in).
Summary
Built-in commands use the shipped definition and the shipped access controls. Stored
enabled,permission,roles,channels, andcooldown_secsare not applied.Migration
0021_clear_builtin_command_accessruns in the core schema for both local SQLite and remote libSQL. It deletes guild and globalcommandsrows whose name is a built-in primary or a reserved alias. After those access fields are no longer kept, the row holds nothing a guild may store, so the row is removed. TheDELETEmatches nothing on a second run.user_versionadvances in the same transaction, so a later open does not replay it. Seed then restores the shipped global rows. The migration file is LF with no trailing whitespace. Publication labels advertise core schema 21. Historical publication fixtures still expect schema 15 and 16 after the newer migration lines are removed.Custom commands under other names are left in place. Load, the slash registry, and API
effectivereads use the shipped command. Built-in writes stay rejected.!voiceby name or nickname, when the guild is absent from cache, uses the same not-cached reply as an id or mention. A cached guild that simply has no matching member still returnscould not resolve user. The generic name resolver is unchanged.Test plan
git diff --check --merge-base origin/mainbash scripts/publish-image-test.shcargo fmt --check, clippy-D warningsaliceboth replynot cachedwith no Discord HTTPSummary by Sourcery
Add a cache-backed voice-channel lookup command while hardening built-in command resolution, access-control handling, and configuration APIs.
New Features:
voicecommand that reports a member’s cached voice channel through prefix and slash interfaces, with safe handling for uncached, hidden, unavailable, and ambiguous cases.Bug Fixes:
Enhancements:
Documentation:
!voicecommand and its slash equivalent.Tests: