kaykayyali/zalbot
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(skill-check): FR-43 single player-locked Roll button (ship-and-break) — Story 6.1 pt1

Browse Source 
Retire the Advantage/Disadvantage/Custom-Modifier buttons and the modifier
modal on the skill-check surface. The player now gets a single Roll button;
advantage/disadvantage and the modifier are decided UPSTREAM (LLM emit args +
Foundry stats) and stored on pendingSkillCheck — shown as embed fields, not
chosen by the player. Per the architecture (internal/shared-use stakes), this
is a ship-and-break cutover in one release (no flag).

- PendingSkillCheck += discordId (the targeted player; locks the Roll button)
  + durationSeconds (Feature A timed checks — used in pt2).
- embeds/skillCheck: buildRollButtons() returns one sc_roll button; removed
  buildRollButtons' modifier variants and buildModifierRollButtons.
- rollHandler: routes only sc_roll; canRoll(pendingDiscordId, clickerId) —
  fail-CLOSED when the targeted discordId is known and the clicker differs
  (in-world 'This roll is not yours to make.'), fail-OPEN when discordId is
  absent (name-match fragility — a legit player is never soft-locked out of
  their own roll). Roll mode + modifier come from pendingSkillCheck. Removed
  the sc_adv/sc_dis/sc_mod/sc_*_m:* paths and the modifier modal.
- skillCheckEmit: stores discordId on pendingSkillCheck; migrated the
  pendingSkillCheck set from the non-atomic update() to atomicMutate (closes
  the TOCTOU window where an await on thread.send separated resolve from
  persist — a 5.2 migration miss).
- bot/index.ts: interactionCreate guard is button-only now (modal retired).

Tests: skillCheckEmbed (single-button contract), rollHandler (isSkillCheckInteraction
single-Roll + canRoll player-lock: allow target / reject other / fail-open when
unknown), toolDispatcher (atomicMutate mutator-capture for skill_check_emit).
423 unit pass; tsc clean.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Kaysser Kayyali
2026-06-21 00:26:56 +00:00
parent 86d4354b51
commit 8f8335802e
8 changed files with 97 additions and 227 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
src/bot/embeds/skillCheck.ts
View File

@@ -46,29 +46,12 @@ export function buildSkillCheckEmbed(
return embed;
}
export function buildRollButtons(modifier?: number): ActionRowBuilder<ButtonBuilder> {
if (modifier !== undefined) {
const sign = modifier >= 0 ? `+${modifier}` : String(modifier);
return new ActionRowBuilder<ButtonBuilder>().addComponents(
new ButtonBuilder().setCustomId(`sc_roll_m:${modifier}`).setLabel(`Roll (${sign})`).setStyle(ButtonStyle.Primary),
new ButtonBuilder().setCustomId(`sc_adv_m:${modifier}`).setLabel(`Adv (${sign})`).setStyle(ButtonStyle.Success),
new ButtonBuilder().setCustomId(`sc_dis_m:${modifier}`).setLabel(`Dis (${sign})`).setStyle(ButtonStyle.Danger),
new ButtonBuilder().setCustomId('sc_mod').setLabel('Custom Modifier').setStyle(ButtonStyle.Secondary),
);
}
// FR-43: a single player-locked Roll button. Advantage/disadvantage and the
// modifier are decided upstream (LLM emit + Foundry stats) and shown as embed
// fields — the player no longer chooses them. The button is locked to the
// targeted player via PendingSkillCheck.discordId (validated in rollHandler).
export function buildRollButtons(): ActionRowBuilder<ButtonBuilder> {
return new ActionRowBuilder<ButtonBuilder>().addComponents(
new ButtonBuilder().setCustomId('sc_roll').setLabel('Roll').setStyle(ButtonStyle.Primary),
new ButtonBuilder().setCustomId('sc_adv').setLabel('Advantage').setStyle(ButtonStyle.Success),
new ButtonBuilder().setCustomId('sc_dis').setLabel('Disadvantage').setStyle(ButtonStyle.Danger),
new ButtonBuilder().setCustomId('sc_mod').setLabel('Roll with Modifier').setStyle(ButtonStyle.Secondary),
);
}
export function buildModifierRollButtons(modifier: number): ActionRowBuilder<ButtonBuilder> {
const sign = modifier >= 0 ? `+${modifier}` : String(modifier);
return new ActionRowBuilder<ButtonBuilder>().addComponents(
new ButtonBuilder().setCustomId(`sc_roll_m:${modifier}`).setLabel(`Roll (${sign})`).setStyle(ButtonStyle.Primary),
new ButtonBuilder().setCustomId(`sc_adv_m:${modifier}`).setLabel(`Advantage (${sign})`).setStyle(ButtonStyle.Success),
new ButtonBuilder().setCustomId(`sc_dis_m:${modifier}`).setLabel(`Disadvantage (${sign})`).setStyle(ButtonStyle.Danger),
);
}

131
src/bot/handlers/rollHandler.ts
View File

@@ -4,13 +4,9 @@ import {
type Client,
type ThreadChannel,
type TextChannel,
ModalBuilder,
TextInputBuilder,
TextInputStyle,
ActionRowBuilder,
} from 'discord.js';
import { sessionManager } from '../../session/sessionManager.js';
import { buildSkillCheckEmbed, buildModifierRollButtons, EMBED_COLOR } from '../embeds/skillCheck.js';
import { buildSkillCheckEmbed, EMBED_COLOR } from '../embeds/skillCheck.js';
import { scheduleEncounterLLMTurn } from './messageRouter.js';
import type { ChatMessage } from '../../types/index.js';
@@ -37,22 +33,37 @@ function rollDisadvantage(): { value: number; desc: string } {
return { value, desc: `rolled with disadvantage (${a}, ${b}) → **${value}**` };
}
async function submitResult(
interaction: ButtonInteraction,
roll: { value: number; desc: string },
modifier: number,
client: Client,
): Promise<void> {
// FR-43: is this clicker allowed to roll this check? The Roll button is locked
// to the targeted player. Fail-OPEN when the targeted discordId is unknown
// (e.g. the LLM fuzzed the character name so no roster match was found) — a
// legit player must not be soft-locked out of their own roll. Fail-CLOSED when
// the targeted discordId is known and the clicker is someone else.
export function canRoll(pendingDiscordId: string | undefined, clickerId: string): boolean {
return !pendingDiscordId || pendingDiscordId === clickerId;
}
async function submitResult(interaction: ButtonInteraction, client: Client): Promise<void> {
const channel = interaction.channel as RollChannel | null;
if (!channel?.isThread()) return;
const session = await sessionManager.get(channel.id);
if (!session?.pendingSkillCheck) {
const pending = session?.pendingSkillCheck;
if (!pending) {
await interaction.reply({ content: 'This skill check has already been resolved.', flags: 64 });
return;
}
const { dc, player, prompt } = session.pendingSkillCheck;
// FR-43 player-lock.
if (!canRoll(pending.discordId, interaction.user.id)) {
await interaction.reply({ content: 'This roll is not yours to make.', flags: 64 });
return;
}
// Advantage/disadvantage and the modifier are decided upstream (LLM emit +
// Foundry stats) and stored on pendingSkillCheck — the player only clicks Roll.
const { dc, player, prompt, advantage, disadvantage } = pending;
const modifier = pending.modifier ?? 0;
const roll = disadvantage ? rollDisadvantage() : advantage ? rollAdvantage() : rollSingle();
const total = roll.value + modifier;
const success = total >= dc;
@@ -86,96 +97,22 @@ async function submitResult(
scheduleEncounterLLMTurn(session.threadId, channel, client, true);
}
// FR-43: only the single player-locked `sc_roll` button is routed. The retired
// Advantage/Disadvantage/Custom-Modifier buttons and the modifier modal are
// gone — adv/dis/modifier are decided upstream, not by the player.
export function isSkillCheckInteraction(
interaction: ButtonInteraction | ModalSubmitInteraction,
): boolean {
if (interaction.isButton()) return interaction.customId.startsWith('sc_');
if (interaction.isModalSubmit()) return interaction.customId === 'sc_mod_modal';
return false;
return interaction.isButton() && interaction.customId.startsWith('sc_');
}
export async function handleRollInteraction(
interaction: ButtonInteraction | ModalSubmitInteraction,
client: Client,
): Promise<void> {
if (interaction.isButton()) {
const id = interaction.customId;
if (id === 'sc_roll') return submitResult(interaction, rollSingle(), 0, client);
if (id === 'sc_adv') return submitResult(interaction, rollAdvantage(), 0, client);
if (id === 'sc_dis') return submitResult(interaction, rollDisadvantage(), 0, client);
if (id === 'sc_mod') {
const modal = new ModalBuilder()
.setCustomId('sc_mod_modal')
.setTitle('Enter your modifier')
.addComponents(
new ActionRowBuilder<TextInputBuilder>().addComponents(
new TextInputBuilder()
.setCustomId('modifier_value')
.setLabel('Modifier (e.g. +3, -1, 5)')
.setStyle(TextInputStyle.Short)
.setRequired(true)
.setMaxLength(4),
),
);
await interaction.showModal(modal);
return;
}
// sc_roll_m:3, sc_adv_m:-2, sc_dis_m:1
const modMatch = /^sc_(roll|adv|dis)_m:(-?\d+)$/.exec(id);
if (modMatch) {
const type = modMatch[1];
const modifier = parseInt(modMatch[2], 10);
const roll =
type === 'adv' ? rollAdvantage() :
type === 'dis' ? rollDisadvantage() :
rollSingle();
return submitResult(interaction, roll, modifier, client);
}
return;
}
// Modal submit for modifier
if (interaction.isModalSubmit() && interaction.customId === 'sc_mod_modal') {
const channel = interaction.channel as RollChannel | null;
if (!channel?.isThread()) return;
const session = await sessionManager.get(channel.id);
if (!session?.pendingSkillCheck) {
await interaction.reply({ content: 'This skill check has already been resolved.', flags: 64 });
return;
}
const rawMod = interaction.fields.getTextInputValue('modifier_value').trim();
const modifier = parseInt(rawMod.replace(/^\+/, ''), 10);
if (isNaN(modifier)) {
await interaction.reply({
content: 'Invalid modifier — enter a number like `+3`, `-1`, or `5`.',
flags: 64,
});
return;
}
// Remove buttons from the original skill check embed now that modifier flow is active
const { messageId, player, prompt, dc, modifier: charModifier, skill } = session.pendingSkillCheck;
if (messageId) {
const original = await (channel as ThreadChannel).messages.fetch(messageId).catch(() => null);
if (original) {
const bare = buildSkillCheckEmbed(player, prompt, dc, undefined, undefined, charModifier, skill);
await original.edit({ embeds: [bare], components: [] }).catch(() => null);
}
}
const sign = modifier >= 0 ? `+${modifier}` : String(modifier);
const modEmbed = buildSkillCheckEmbed(player, prompt, dc)
.setFooter({ text: `Modifier: ${sign}` });
await interaction.reply({
embeds: [modEmbed],
components: [buildModifierRollButtons(modifier)],
});
}
}
if (!interaction.isButton()) return;
if (interaction.customId === 'sc_roll') return submitResult(interaction, client);
// Any other sc_* id is a retired button (sc_adv/sc_dis/sc_mod/sc_*_m:*) that
// should no longer be rendered; ignore it (a stale embed from before the
// cutover) rather than crash.
}

11
src/bot/index.ts
View File

@@ -53,13 +53,10 @@ client.once('ready', () => {
});
client.on('interactionCreate', async (interaction) => {
// ── Skill-check roll buttons and modifier modal
if (
(interaction.isButton() || interaction.isModalSubmit()) &&
isSkillCheckInteraction(interaction)
) {
const kind = interaction.isButton() ? 'button' : 'modal';
const id = interaction.isButton() ? interaction.customId : interaction.customId;
// ── Skill-check roll button (single, player-locked — FR-43)
if (interaction.isButton() && isSkillCheckInteraction(interaction)) {
const kind = 'button';
const id = interaction.customId;
log.info('interaction', `${kind} ${id}`, { user: interaction.user.username });
const start = Date.now();
try {

7
src/harness/tools/skillCheckEmit.ts
View File

@@ -109,19 +109,20 @@ const skillCheckEmit: ToolPlugin = {
}
const sent = await ctx.thread.send({ embeds: [buildSuspenseEmbed(player, prompt)] });
await sessionManager.update(ctx.session.threadId, {
await sessionManager.atomicMutate(ctx.session.threadId, () => ({
pendingSkillCheck: {
player, prompt, dc, messageId: sent.id, modifier, skill: skill || undefined,
advantage: advantage || undefined,
disadvantage: disadvantage || undefined,
discordId: discordId || undefined,
},
pendingSkillCheckAttempts: 0,
});
}));
setTimeout(() => {
sent
.edit({
embeds: [buildSkillCheckEmbed(player, prompt, dc, undefined, undefined, modifier, skill || undefined, advantage || undefined, disadvantage || undefined)],
components: [buildRollButtons(modifier)],
components: [buildRollButtons()],
})
.catch(() => null);
}, 1_500);

12
src/types/index.ts
View File

@@ -39,11 +39,13 @@ export interface PendingSkillCheck {
player: string;
prompt: string;
dc: number;
messageId?: string; // Discord message ID of the embed with roll buttons
modifier?: number; // Pre-fetched Foundry skill/ability modifier, if available
skill?: string; // Skill name as provided by the LLM (e.g. "Perception")
advantage?: boolean; // LLM determined the player has advantage on this roll
disadvantage?: boolean; // LLM determined the player has disadvantage on this roll
messageId?: string; // Discord message ID of the embed with roll buttons
modifier?: number; // Pre-fetched Foundry skill/ability modifier, if available
skill?: string; // Skill name as provided by the LLM (e.g. "Perception")
advantage?: boolean; // LLM/Foundry granted advantage on this roll (decided upstream)
disadvantage?: boolean; // LLM/Foundry granted disadvantage on this roll (decided upstream)
discordId?: string; // Targeted player's Discord ID — the Roll button is locked to them (FR-43)
durationSeconds?: number; // If set, the check is timed; expiry finalizes as FAILURE (Feature A)
}
export interface SessionState {

39
tests/unit/rollHandler.test.ts
View File

@@ -1,5 +1,5 @@
import { describe, it, expect } from 'vitest';
import { isSkillCheckInteraction } from '../../src/bot/handlers/rollHandler.js';
import { isSkillCheckInteraction, canRoll } from '../../src/bot/handlers/rollHandler.js';
import type { ButtonInteraction, ModalSubmitInteraction } from 'discord.js';
function fakeButton(customId: string): ButtonInteraction {
@@ -10,18 +10,9 @@ function fakeModal(customId: string): ModalSubmitInteraction {
return { isButton: () => false, isModalSubmit: () => true, customId } as unknown as ModalSubmitInteraction;
}
describe('isSkillCheckInteraction', () => {
it('recognises all base roll button IDs', () => {
describe('isSkillCheckInteraction (FR-43 single player-locked Roll)', () => {
it('recognises the sc_roll button', () => {
expect(isSkillCheckInteraction(fakeButton('sc_roll'))).toBe(true);
expect(isSkillCheckInteraction(fakeButton('sc_adv'))).toBe(true);
expect(isSkillCheckInteraction(fakeButton('sc_dis'))).toBe(true);
expect(isSkillCheckInteraction(fakeButton('sc_mod'))).toBe(true);
});
it('recognises modifier sub-button IDs', () => {
expect(isSkillCheckInteraction(fakeButton('sc_roll_m:3'))).toBe(true);
expect(isSkillCheckInteraction(fakeButton('sc_adv_m:-2'))).toBe(true);
expect(isSkillCheckInteraction(fakeButton('sc_dis_m:0'))).toBe(true);
});
it('rejects unrelated button IDs', () => {
@@ -30,11 +21,25 @@ describe('isSkillCheckInteraction', () => {
expect(isSkillCheckInteraction(fakeButton(''))).toBe(false);
});
it('recognises the modifier modal ID', () => {
expect(isSkillCheckInteraction(fakeModal('sc_mod_modal'))).toBe(true);
});
it('rejects other modal IDs', () => {
it('no longer routes a modal (the modifier modal is retired)', () => {
expect(isSkillCheckInteraction(fakeModal('sc_mod_modal'))).toBe(false);
expect(isSkillCheckInteraction(fakeModal('some_other_modal'))).toBe(false);
});
});
describe('canRoll — player-locked Roll button (FR-43)', () => {
it('allows the targeted player when their discordId is known', () => {
expect(canRoll('user-a', 'user-a')).toBe(true);
});
it('rejects a different clicker when the targeted discordId is known (fail-closed)', () => {
expect(canRoll('user-a', 'user-b')).toBe(false);
});
it('fail-opens when the targeted discordId is unknown (name-match fragility)', () => {
// A legit player must not be soft-locked out of their own roll when the LLM
// fuzzed the character name and no roster match was found.
expect(canRoll(undefined, 'anyone')).toBe(true);
expect(canRoll('', 'anyone')).toBe(true);
});
});

77
tests/unit/skillCheckEmbed.test.ts
View File

@@ -3,7 +3,6 @@ import {
buildSkillCheckEmbed,
buildSuspenseEmbed,
buildRollButtons,
buildModifierRollButtons,
EMBED_COLOR,
} from '../../src/bot/embeds/skillCheck.js';
@@ -86,79 +85,15 @@ describe('buildSkillCheckEmbed', () => {
});
});
describe('buildRollButtons', () => {
it('returns exactly 4 buttons with no modifier', () => {
describe('buildRollButtons (FR-43 single player-locked Roll)', () => {
it('returns exactly one button — the player-locked Roll', () => {
const data = buildRollButtons().toJSON();
expect(data.components).toHaveLength(4);
expect(data.components).toHaveLength(1);
});
it('has the plain custom IDs in order when no modifier', () => {
it('uses the sc_roll custom id with label "Roll"', () => {
const data = buildRollButtons().toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll', 'sc_adv', 'sc_dis', 'sc_mod']);
});
it('returns 4 buttons with pre-applied modifier when modifier is provided', () => {
const data = buildRollButtons(5).toJSON();
expect(data.components).toHaveLength(4);
});
it('uses modifier-encoded IDs when modifier is provided', () => {
const data = buildRollButtons(5).toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll_m:5', 'sc_adv_m:5', 'sc_dis_m:5', 'sc_mod']);
});
it('handles negative modifier in IDs', () => {
const data = buildRollButtons(-3).toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll_m:-3', 'sc_adv_m:-3', 'sc_dis_m:-3', 'sc_mod']);
});
it('shows modifier in labels when modifier is provided', () => {
const data = buildRollButtons(4).toJSON();
const labels = data.components.map((c: { label: string }) => c.label);
expect(labels[0]).toContain('+4');
expect(labels[1]).toContain('+4');
expect(labels[2]).toContain('+4');
});
});
describe('buildModifierRollButtons', () => {
it('returns exactly 3 buttons', () => {
const data = buildModifierRollButtons(3).toJSON();
expect(data.components).toHaveLength(3);
});
it('encodes a positive modifier in all custom IDs', () => {
const data = buildModifierRollButtons(3).toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll_m:3', 'sc_adv_m:3', 'sc_dis_m:3']);
});
it('encodes a negative modifier in all custom IDs', () => {
const data = buildModifierRollButtons(-2).toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll_m:-2', 'sc_adv_m:-2', 'sc_dis_m:-2']);
});
it('shows the +sign in labels for positive modifiers', () => {
const data = buildModifierRollButtons(5).toJSON();
const labels = data.components.map((c: { label: string }) => c.label);
expect(labels[0]).toContain('+5');
expect(labels[1]).toContain('+5');
expect(labels[2]).toContain('+5');
});
it('shows the minus sign in labels for negative modifiers', () => {
const data = buildModifierRollButtons(-1).toJSON();
const labels = data.components.map((c: { label: string }) => c.label);
expect(labels[0]).toContain('-1');
});
it('handles zero modifier', () => {
const data = buildModifierRollButtons(0).toJSON();
const ids = data.components.map((c: { custom_id: string }) => c.custom_id);
expect(ids).toEqual(['sc_roll_m:0', 'sc_adv_m:0', 'sc_dis_m:0']);
expect(data.components[0].custom_id).toBe('sc_roll');
expect(data.components[0].label).toBe('Roll');
});
});

20
tests/unit/toolDispatcher.test.ts
View File

@@ -2,12 +2,14 @@ import { vi, describe, it, expect, beforeEach } from 'vitest';
const {
mockSessionUpdate,
mockAtomicMutate,
mockLogEncounter,
mockWriteSummary,
mockBuildSkillCheckEmbed,
mockBuildResolutionEmbed,
} = vi.hoisted(() => ({
mockSessionUpdate: vi.fn(),
mockAtomicMutate: vi.fn(),
mockLogEncounter: vi.fn().mockResolvedValue({ enc_id: 'e1' }),
mockWriteSummary: vi.fn().mockReturnValue('/data/summaries/test.txt'),
mockBuildSkillCheckEmbed: vi.fn().mockReturnValue({ data: { title: 'Skill Check' } }),
@@ -15,7 +17,7 @@ const {
}));
vi.mock('../../src/session/sessionManager.js', () => ({
sessionManager: { update: mockSessionUpdate },
sessionManager: { update: mockSessionUpdate, atomicMutate: mockAtomicMutate },
}));
vi.mock('../../src/graphmcp/client.js', () => ({
logEncounter: mockLogEncounter,
@@ -43,6 +45,13 @@ function makeThread() {
beforeEach(() => {
vi.clearAllMocks();
// Faithful atomicMutate: run the mutator against the mock session and return
// the merged state, so mutator logic (e.g. skill_check_emit's pendingSkillCheck
// set) runs and its patch is inspectable.
mockAtomicMutate.mockImplementation(async (_tid: string, mutator: (s: any) => any) => {
const patch = await mutator(mockSession);
return { ...mockSession, ...patch };
});
});
describe('dispatchTool — skill_check_emit', () => {
@@ -61,10 +70,11 @@ describe('dispatchTool — skill_check_emit', () => {
{ tool: 'skill_check_emit', args: { player: 'Aelindra', prompt: 'Chase DC', dc: 13 } },
{ session: mockSession, thread: thread as any },
);
expect(mockSessionUpdate).toHaveBeenCalledWith(
mockSession.threadId,
expect.objectContaining({ pendingSkillCheck: expect.objectContaining({ dc: 13 }) }),
);
// skill_check_emit persists via atomicMutate(tid, mutator) — capture the patch.
expect(mockAtomicMutate).toHaveBeenCalledWith(mockSession.threadId, expect.any(Function));
const mutator = mockAtomicMutate.mock.calls[0][1] as (s: any) => any;
const patch = mutator(mockSession) as { pendingSkillCheck: { dc: number } };
expect(patch.pendingSkillCheck.dc).toBe(13);
});
it('returns a systemMessage confirming the embed was posted', async () => {