-
Notifications
You must be signed in to change notification settings - Fork 424
Fix /help routing fallthrough, error handling, reaction, and mention sanitization
#40476
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.
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
Changes from all commits
856500e
53b57a0
b5b9da4
9b298bd
27653ef
9c640b1
68e923c
e0058d8
105d71a
6914a01
90067b0
835a6f1
29617c1
591172c
95d735f
db7acbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,6 +298,164 @@ async function dispatchWorkflow(workflowId, ref, inputs) { | |
| } | ||
| } | ||
|
|
||
| function isBuiltinHelpEnabled() { | ||
| const raw = (process.env.GH_AW_HELP_COMMAND_ENABLED || "").trim().toLowerCase(); | ||
| if (!raw || raw === "true") { | ||
| return true; | ||
| } | ||
| if (raw === "false") { | ||
| return false; | ||
| } | ||
| core.warning(`Invalid value for GH_AW_HELP_COMMAND_ENABLED (expected 'true' or 'false', got '${raw}'). Using default: enabled.`); | ||
| return true; | ||
| } | ||
|
|
||
| function parseHelpCommandsMetadata() { | ||
| const raw = process.env.GH_AW_HELP_COMMANDS || "[]"; | ||
| try { | ||
| const parsed = JSON.parse(raw); | ||
| if (!Array.isArray(parsed)) { | ||
| return []; | ||
| } | ||
| return parsed | ||
| .flatMap(item => { | ||
| const command = typeof item?.command === "string" ? item.command.trim() : ""; | ||
| if (!command) { | ||
| return []; | ||
| } | ||
| const description = typeof item?.description === "string" ? item.description.trim() : ""; | ||
| return [ | ||
| { | ||
| command, | ||
| description, | ||
| centralized: Boolean(item?.centralized), | ||
| decentralized: Boolean(item?.decentralized), | ||
| label: Boolean(item?.label), | ||
| }, | ||
| ]; | ||
| }) | ||
| .sort((left, right) => left.command.localeCompare(right.command)); | ||
| } catch (error) { | ||
| core.warning(`Failed to parse GH_AW_HELP_COMMANDS metadata: ${String(error)}`); | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Regex matching bare GitHub @mentions outside inline code spans. | ||
| * Captures the preceding non-word character (p1) and the username (p2). | ||
| */ | ||
| const GITHUB_MENTION_RE = /(^|[^\w`])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?)/g; | ||
|
|
||
| /** | ||
| * Neutralizes bare @mentions in a description string so they do not trigger | ||
| * GitHub notifications. Wraps matched mentions in backticks. | ||
| * @param {string} description | ||
| * @returns {string} | ||
| */ | ||
| function neutralizeDescriptionMentions(description) { | ||
| return description.replace(GITHUB_MENTION_RE, (_, p1, p2) => `${p1}\`@${p2}\``); | ||
| } | ||
|
|
||
| function buildCommandBulletLine(entry) { | ||
| const desc = entry.description ? neutralizeDescriptionMentions(entry.description) : ""; | ||
| const suffix = desc ? ` — ${desc}` : ""; | ||
| return `- \`/${entry.command}\`${suffix}`; | ||
| } | ||
|
|
||
| function buildLabelBulletLine(entry) { | ||
| const desc = entry.description ? neutralizeDescriptionMentions(entry.description) : ""; | ||
| const suffix = desc ? ` — ${desc}` : ""; | ||
| return `- \`${entry.command}\`${suffix}`; | ||
| } | ||
|
|
||
| function buildHelpCommentBody(helpCommands) { | ||
| // Commands that are centralized should appear only in the centralized section even if | ||
| // they are also registered as decentralized (e.g. two workflows for the same command). | ||
| const centralized = helpCommands.filter(entry => entry.centralized); | ||
| const centralizedNames = new Set(centralized.map(entry => entry.command)); | ||
| const decentralized = helpCommands.filter(entry => entry.decentralized && !centralizedNames.has(entry.command)); | ||
| const labels = helpCommands.filter(entry => entry.label); | ||
|
|
||
| const lines = ["## Supported Commands", "", "**Centralized slash commands**"]; | ||
| if (centralized.length === 0) { | ||
| lines.push("- _None_"); | ||
| } else { | ||
| for (const entry of centralized) { | ||
| lines.push(buildCommandBulletLine(entry)); | ||
| } | ||
| } | ||
|
|
||
| lines.push("", "**Non-centralized slash commands**"); | ||
| if (decentralized.length === 0) { | ||
| lines.push("- _None_"); | ||
| } else { | ||
| for (const entry of decentralized) { | ||
| lines.push(buildCommandBulletLine(entry)); | ||
| } | ||
| } | ||
|
|
||
| lines.push("", "**Label commands**"); | ||
| if (labels.length === 0) { | ||
| lines.push("- _None_"); | ||
| } else { | ||
| for (const entry of labels) { | ||
| lines.push(buildLabelBulletLine(entry)); | ||
| } | ||
| } | ||
|
|
||
| const docsUrl = (process.env.GH_AW_SLASH_COMMAND_DOCS_URL || "").trim(); | ||
| if (docsUrl) { | ||
| lines.push("", `Learn more: [Slash command documentation](${docsUrl})`); | ||
| } | ||
| return lines.join("\n"); | ||
| } | ||
|
|
||
| async function postBuiltinHelpComment(commentBody) { | ||
| const owner = context.repo.owner; | ||
| const repo = context.repo.repo; | ||
|
|
||
| try { | ||
| const issueNumber = context.payload?.issue?.number ?? context.payload?.pull_request?.number; | ||
| if (issueNumber) { | ||
| await github.rest.issues.createComment({ | ||
| owner, | ||
| repo, | ||
| issue_number: issueNumber, | ||
| body: commentBody, | ||
| headers: { | ||
| "X-GitHub-Api-Version": GITHUB_API_VERSION, | ||
| }, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| if (context.eventName === "discussion" || context.eventName === "discussion_comment") { | ||
| const discussionID = context.payload?.discussion?.node_id; | ||
| if (!discussionID) { | ||
| core.warning("Unable to post builtin /help response: discussion node_id missing."); | ||
| return false; | ||
| } | ||
| await github.graphql( | ||
| ` | ||
| mutation($discussionId: ID!, $body: String!) { | ||
| addDiscussionComment(input: { discussionId: $discussionId, body: $body }) { | ||
| comment { id } | ||
| } | ||
| }`, | ||
| { discussionId: discussionID, body: commentBody } | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| core.warning(`Unable to post builtin /help response for event '${context.eventName}'.`); | ||
| return false; | ||
| } catch (error) { | ||
| core.warning(`Failed to post builtin /help comment: ${String(error)}`); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function toWorkflowDispatchID(route) { | ||
| if (!route?.workflow || typeof route.workflow !== "string" || !route.workflow.trim()) { | ||
| return ""; | ||
|
|
@@ -423,6 +581,19 @@ async function main() { | |
| } | ||
|
|
||
| const commandName = selectedCommand; | ||
| if (commandName === "help") { | ||
| if (isBuiltinHelpEnabled()) { | ||
| await addImmediateReaction("eyes"); | ||
| const posted = await postBuiltinHelpComment(buildHelpCommentBody(parseHelpCommandsMetadata())); | ||
| if (posted) { | ||
| core.info("Posted builtin /help command response."); | ||
| } | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fixWhen the builtin is disabled, fall through to the normal routing path instead of returning early: if (commandName === "help") {
if (isBuiltinHelpEnabled()) {
const posted = await postBuiltinHelpComment(buildHelpCommentBody(parseHelpCommandsMetadata()));
if (posted) {
core.info("Posted builtin /help command response.");
}
return;
}
// Builtin disabled — fall through to normal route dispatch below.
core.info("Builtin /help command is disabled (help_command=false); routing normally.");
}Also note: even when the builtin is enabled, the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. The |
||
| } | ||
| // Builtin /help is disabled — fall through so custom /help workflows still dispatch. | ||
| core.info("Builtin /help command is disabled by aw.json (help_command=false); routing normally."); | ||
| } | ||
|
|
||
| core.info(`Resolved command '/${commandName}' for event identifier '${identifier}'.`); | ||
| const configuredRoutes = resolveMatchingSlashRoutes(slashRouteMap, commandName); | ||
| core.info(`Configured routes for '/${commandName}': ${configuredRoutes.length}.`); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] A command with both
centralized: trueanddecentralized: true(whichbuildHelpCommandEntriescan produce) will appear in both sections of the help comment, confusing users.💡 Example and fix
From
TestBuildHelpCommandEntries,triagegetsCentralized: true, Decentralized: true.buildHelpCommentBodyfilters independently, so it shows/triagetwice.Consider either:
Centralized commands(centralized takes precedence), or(also non-centralized).This case should also be covered by a JS test asserting a command with both flags appears only once (or exactly twice with a clear label).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
buildHelpCommentBodynow tracks the set of centralized command names and excludes them from the decentralized filter:decentralized = helpCommands.filter(entry => entry.decentralized && !centralizedNames.has(entry.command)). A test asserts a command with both flags appears only in the centralized section.