Skip to content

Validate Discord app distribution config #611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rissrice2105-agent wants to merge 1 commit into from
Closed

Validate Discord app distribution config #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/targets/chat-discord/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,40 @@ describe('Discord chat target', () => {
})).rejects.toThrow('DISCORD_APP_TOKEN not in vault');
});

it('rejects non-numeric application IDs before writing manifests', async () => {
const outDir = await mkdtemp(join(tmpdir(), 'sh1pt-discord-'));
tempDirs.push(outDir);

await expect(adapter.build(fakeBuildContext({
outDir,
version: '1.2.3',
}) as any, {
applicationId: 'abc-123',
distribution: 'public',
})).rejects.toThrow('numeric Discord snowflake');
});

it('rejects unsupported distributions in dry-run shipping', async () => {
await expect(adapter.ship(fakeShipContext({
version: '1.2.3',
dryRun: true,
}) as any, {
applicationId: '123456',
distribution: 'server-listing',
} as any)).rejects.toThrow('distribution must be one of');
});
Comment on lines +108 to +116

@greptile-apps greptile-apps Bot Jun 5, 2026

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.

P2 Missing build-path coverage for invalid distribution

The new distribution test only exercises adapter.ship. Both build and ship reach manifestFor → requireDistribution, so the validation fires on both paths. Adding a parallel test for adapter.build with an invalid distribution would close the gap and guard against a future refactor that separates the two paths.


it('rejects unsupported OAuth scopes in dry-run shipping', async () => {
await expect(adapter.ship(fakeShipContext({
version: '1.2.3',
dryRun: true,
}) as any, {
applicationId: '123456',
distribution: 'public',
scopes: ['identify'],
} as any)).rejects.toThrow('scope must be one of');
});

it('patches the application and overwrites global slash commands', async () => {
const fetchMock = vi.fn()
.mockResolvedValueOnce({ ok: true, status: 200, text: async () => '{"id":"123456"}' })
Expand Down
22 changes: 19 additions & 3 deletions packages/targets/chat-discord/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,24 @@ interface DiscordCommand {
options?: unknown[];
}

const DISTRIBUTIONS = ['private', 'public', 'directory'] as const;
const SCOPES = ['bot', 'applications.commands'] as const;

function requireApplicationId(config: Config): string {
const applicationId = config.applicationId?.trim();
if (!applicationId) throw new Error('chat-discord requires applicationId');
if (!/^\d+$/.test(applicationId)) throw new Error('chat-discord applicationId must be a numeric Discord snowflake');
return applicationId;
}

function requireDistribution(config: Config): Config['distribution'] {
const distribution = String(config.distribution ?? '').trim();
if (!DISTRIBUTIONS.includes(distribution as Config['distribution'])) {
throw new Error(`chat-discord distribution must be one of: ${DISTRIBUTIONS.join(', ')}`);
}
Comment on lines +42 to +44

@greptile-apps greptile-apps Bot Jun 5, 2026

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.

P2 Same pattern: the error message for an invalid distribution doesn't include the rejected value. When config is assembled dynamically (e.g. from a deserialized config file) the error gives no hint of what value was actually received.

Suggested change
if (!DISTRIBUTIONS.includes(distribution as Config['distribution'])) {
throw new Error(`chat-discord distribution must be one of: ${DISTRIBUTIONS.join(', ')}`);
}
if (!DISTRIBUTIONS.includes(distribution as Config['distribution'])) {
throw new Error(`chat-discord distribution "${distribution}" is not supported; must be one of: ${DISTRIBUTIONS.join(', ')}`);
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return distribution as Config['distribution'];
}

function normalizeCommands(commands: Config['slashCommands']): DiscordCommand[] {
return (commands ?? []).map((command) => {
const name = command.name.replace(/^\//, '').trim().toLowerCase();
Expand All @@ -49,7 +61,10 @@ function normalizeCommands(commands: Config['slashCommands']): DiscordCommand[]
}

function scopesFor(config: Config): string[] {
return config.scopes?.length ? config.scopes : ['bot', 'applications.commands'];
const scopes = config.scopes?.length ? config.scopes : ['bot', 'applications.commands'];
const invalid = scopes.find((scope) => !SCOPES.includes(scope as typeof SCOPES[number]));
if (invalid) throw new Error(`chat-discord scope must be one of: ${SCOPES.join(', ')}`);
Comment on lines +65 to +66

@greptile-apps greptile-apps Bot Jun 5, 2026

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.

P2 The error omits the invalid value, making it harder to debug when a bad scope is passed programmatically. Including the rejected value in the message gives the caller something actionable to act on immediately.

Suggested change
const invalid = scopes.find((scope) => !SCOPES.includes(scope as typeof SCOPES[number]));
if (invalid) throw new Error(`chat-discord scope must be one of: ${SCOPES.join(', ')}`);
const invalid = scopes.find((scope) => !SCOPES.includes(scope as typeof SCOPES[number]));
if (invalid) throw new Error(`chat-discord scope "${invalid}" is not supported; must be one of: ${SCOPES.join(', ')}`);

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return scopes;
}

function inviteUrl(config: Config): string {
Expand All @@ -63,17 +78,18 @@ function inviteUrl(config: Config): string {

function manifestFor(config: Config, version: string) {
const commands = normalizeCommands(config.slashCommands);
const distribution = requireDistribution(config);
return {
provider: 'discord',
applicationId: requireApplicationId(config),
version,
distribution: config.distribution,
distribution,
interactionsEndpointUrl: config.interactionsEndpointUrl,
scopes: scopesFor(config),
permissions: config.permissions ?? 0,
inviteUrl: inviteUrl(config),
commands,
directoryReviewRequired: config.distribution === 'directory',
directoryReviewRequired: distribution === 'directory',
};
}

Expand Down
Loading