Fix /help routing fallthrough, error handling, reaction, and mention sanitization#40476
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
/help slash command with aw.json opt-out
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
@copilot also generate documentation for supported label commands. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (225 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Pull request overview
This PR adds a centralized builtin /help slash command to the centralized router so users can discover supported slash commands in-context. It introduces a top-level help_command flag in aw.json to opt out, emits help metadata into the generated centralized router workflow, and updates the router script/tests and docs accordingly.
Changes:
- Added
help_command(default-enabled) toaw.jsonparsing + schema, with typed access viaRepoConfig.IsHelpCommandEnabled(). - Extended centralized router workflow generation to emit help command inventory/enablement/docs URL via env vars.
- Implemented builtin
/helphandling inroute_slash_command.cjs, with tests and documentation updates.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/repo_config.go | Adds help_command parsing and enablement helper. |
| pkg/workflow/repo_config_test.go | Adds tests for default/true/false help_command behavior. |
| pkg/workflow/central_slash_command_workflow.go | Emits help metadata/env vars during centralized workflow generation. |
| pkg/workflow/central_slash_command_workflow_test.go | Verifies help env vars and metadata aggregation; tests config disablement. |
| pkg/parser/schemas/repo_config_schema.json | Documents/validates help_command in the JSON schema. |
| pkg/cli/compile_post_processing.go | Loads repo config and passes it to centralized workflow generator. |
| pkg/cli/compile_pipeline.go | Wires gitRoot into the centralized workflow wrapper call. |
| docs/src/content/docs/reference/command-triggers.md | Documents builtin centralized /help and help_command opt-out. |
| actions/setup/js/route_slash_command.test.cjs | Adds tests for builtin /help behavior and disablement. |
| actions/setup/js/route_slash_command.cjs | Implements builtin /help handling + comment posting. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 3
| const commandName = selectedCommand; | ||
| if (commandName === "help") { | ||
| if (!isBuiltinHelpEnabled()) { | ||
| core.info("Builtin /help command is disabled by aw.json (help_command=false)."); | ||
| return; | ||
| } | ||
| const posted = await postBuiltinHelpComment(buildHelpCommentBody(parseHelpCommandsMetadata())); | ||
| if (posted) { | ||
| core.info("Posted builtin /help command response."); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fixed. When help_command: false (builtin disabled), the code now falls through to normal route dispatch instead of returning early — so any centralized /help workflow still fires. When the builtin is enabled, it intentionally preempts custom /help workflows (this is by design: the builtin handler is the primary purpose). A compile-time warning is now emitted via buildHelpCommandEntries whenever a workflow registers help as a command name, surfacing the conflict at gh aw compile time rather than silently at runtime.
| 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; | ||
| } |
There was a problem hiding this comment.
Fixed. postBuiltinHelpComment is now wrapped in a single try-catch that returns false on any API error and emits a core.warning(...) with the error detail. @mentions in descriptions are neutralized via neutralizeDescriptionMentions() (using GITHUB_MENTION_RE) before interpolation into bullet lines, so they cannot trigger GitHub notifications. A test covers the API-error path.
| 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( |
There was a problem hiding this comment.
Fixed. The entire postBuiltinHelpComment body — both the REST createComment path and the GraphQL addDiscussionComment path — is now inside a single try-catch. Failures emit core.warning(...) and return false instead of propagating. A dedicated test covers the discussion-context GraphQL path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /improve-codebase-architecture — requesting changes on a correctness gap and test coverage holes, with a couple of design suggestions.
📋 Key Themes & Highlights
Blocking issues
- Silent dispatch suppression: if a repo defines
command: [help]for a centralized workflow, the new/helpintercept inmain()will consume it with no warning — the user's dispatch silently never fires. A compile-time warning is needed. - Uncaught API errors in
postBuiltinHelpComment: REST and GraphQL calls are not wrapped in try-catch; failures propagate as unhandled rejections with no diagnostic logging.
Test coverage gaps (non-blocking but important)
- Discussion context (
discussion/discussion_comment) — the GraphQL path has zero test coverage - Unsupported event type warning path (
core.warning(...)at line 416) isBuiltinHelpEnabledinvalid-value warning (e.g."banana")parseHelpCommandsMetadatamalformed-JSON and non-array pathsTestBuildHelpCommandEntriesmissing conflict-logging scenario
Design observations
- Command appearing in both sections: a command with both
centralized: trueanddecentralized: truerenders in both the "Centralized" and "Non-centralized" lists — potentially confusing getHelpDocsUrl()is dead code:GH_AW_SLASH_COMMAND_DOCS_URLis always baked into generated YAML, so the JS env fallback can never be reached- Redundant nil guard before
IsHelpCommandEnabledcall (the method already handles nil receivers)
Positive highlights
- ✅ Excellent use of
*bool+ nil-receiver accessor pattern for the opt-out — clean and idiomatic Go - ✅ Deterministic description conflict resolution is well-thought-out and documented
- ✅ The three-env-var boundary (compile → runtime) is a solid and auditable design
- ✅ Good test breadth for the happy path and the disabled path in both JS and Go layers
- ✅
DescriptionBytracking in the aggregate struct is a nice touch for diagnostic logging
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 122.6 AIC · ⌖ 8.51 AIC · ⊞ 6.9K
| setupActionRef := ResolveSetupActionReference(ctx, actionMode, GetVersion(), "", nil) | ||
|
|
||
| content, err := buildCentralSlashCommandWorkflowYAML(slashRoutesByCommand, labelRoutesByCommand, mergedEvents, resolveCentralSlashRunsOn(workflowDataList), setupActionRef) | ||
| helpCommands := buildHelpCommandEntries(workflowDataList) |
There was a problem hiding this comment.
[/grill-with-docs] help is a reserved command name with no compile-time warning — if a user registers command: [help] for a centralized workflow, the intercept in main() will silently consume it and their dispatch will never fire.
💡 Suggested fix
In buildHelpCommandEntries or GenerateCentralSlashCommandWorkflow, check for and warn about the reserved name:
for _, command := range commands {
if command == "help" {
centralSlashCommandWorkflowLog.Printf(
"Warning: 'help' is reserved for the builtin /help handler; " +
"centralized workflows using this command name will not be dispatched")
}
}This surfaces the conflict at compile time instead of silently swallowing dispatches at runtime.
There was a problem hiding this comment.
Fixed. buildHelpCommandEntries now logs a warning via centralSlashCommandWorkflowLog.Printf when any workflow registers "help" as a command name, explaining that it is reserved for the builtin handler and won't dispatch unless help_command is disabled. A test (TestBuildHelpCommandEntries_ReservedHelpCommandName) verifies the entry is still included in the metadata.
| } | ||
|
|
||
| function buildHelpCommentBody(helpCommands) { | ||
| const centralized = helpCommands.filter(entry => entry.centralized); |
There was a problem hiding this comment.
[/tdd] A command with both centralized: true and decentralized: true (which buildHelpCommandEntries can produce) will appear in both sections of the help comment, confusing users.
💡 Example and fix
From TestBuildHelpCommandEntries, triage gets Centralized: true, Decentralized: true. buildHelpCommentBody filters independently, so it shows /triage twice.
Consider either:
- Listing such commands only once under
Centralized commands(centralized takes precedence), or - Adding a third section "Both" or a note like
(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.
Fixed. buildHelpCommentBody now 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.
|
|
||
| const issueNumber = context.payload?.issue?.number ?? context.payload?.pull_request?.number; | ||
| if (issueNumber) { | ||
| await github.rest.issues.createComment({ |
There was a problem hiding this comment.
[/tdd] createComment and graphql can throw on API errors, but neither call is wrapped in try-catch — failures will propagate as unhandled rejections through main(), losing all diagnostic context. Other paths in this file (e.g. dispatchWorkflow) wrap their API calls.
💡 Suggested fix
async function postBuiltinHelpComment(commentBody) {
try {
// ... existing REST / GraphQL code ...
} catch (error) {
core.warning(`Failed to post builtin /help comment: ${String(error)}`);
return false;
}
}This also makes the posted check in main() meaningful for the error case.
There was a problem hiding this comment.
Fixed. postBuiltinHelpComment is now wrapped in try-catch (both REST and GraphQL branches). Errors emit core.warning("Failed to post builtin /help comment: ...") and return false. A test ("warns when postBuiltinHelpComment fails due to API error") confirms graceful degradation.
| expect(issueCommentCalls[0].body).toContain("**Non-centralized commands**"); | ||
| expect(issueCommentCalls[0].body).toContain("- `/local-summary` — Run summary workflow"); | ||
| expect(issueCommentCalls[0].body).toContain("https://github.github.com/gh-aw/reference/command-triggers/"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] No test covers /help in a discussion context. postBuiltinHelpComment has a distinct GraphQL branch (lines 398–413) for discussion / discussion_comment events, but it's untested — a mock mistake or API change here would go undetected.
💡 Suggested test outline
it("handles builtin /help on discussion_comment events via GraphQL", async () => {
globals.context.eventName = "discussion_comment";
globals.context.payload = { discussion: { node_id: "D_test123" }, comment: { body: "/help" } };
await main();
expect(dispatchCalls).toHaveLength(0);
// Assert graphql was called with the addDiscussionComment mutation
const graphqlCalls = globals.github.graphql.mock.calls;
expect(graphqlCalls.length).toBeGreaterThan(0);
// last graphql call should be the help comment, not a reaction
});There was a problem hiding this comment.
Added. New test "handles builtin /help on discussion_comment events via GraphQL" sets eventName = "discussion_comment", provides a discussion.node_id, invokes main(), and asserts that github.graphql was called with the addDiscussionComment mutation and the expected discussionId and help body.
| expect(dispatchCalls).toHaveLength(0); | ||
| expect(issueCommentCalls).toHaveLength(0); | ||
| expect(globals.core.info).toHaveBeenCalledWith(expect.stringContaining("Builtin /help command is disabled")); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Three untested warning paths in the new helper functions:
isBuiltinHelpEnabledwith an invalid value (e.g."banana") — line 310 emits a warning but falls through to enabledparseHelpCommandsMetadatawith malformed JSON — thecatchat line 339 is uncoveredparseHelpCommandsMetadatawith valid JSON that isn't an array — the!Array.isArrayguard at line 318 is uncovered
💡 Suggested additions
it("isBuiltinHelpEnabled warns on invalid value and defaults to enabled", async () => {
process.env.GH_AW_HELP_COMMAND_ENABLED = "banana";
globals.context.payload.comment.body = "/help";
await main();
expect(globals.core.warning).toHaveBeenCalledWith(expect.stringContaining("Invalid value"));
expect(issueCommentCalls).toHaveLength(1); // still posts help
});
it("parseHelpCommandsMetadata handles malformed JSON gracefully", () => {
// Can test the exported function directly, or drive through main() with bad env.
});There was a problem hiding this comment.
Added all three paths: (1) "warns on invalid GH_AW_HELP_COMMAND_ENABLED value and still posts help" — sets env to "banana", asserts warning emitted and help still posted; (2) "handles malformed JSON in GH_AW_HELP_COMMANDS gracefully" — asserts warning + empty command list; (3) "handles non-array JSON in GH_AW_HELP_COMMANDS gracefully" — asserts - _None_ sections rendered.
| } | ||
| } | ||
|
|
||
| function getHelpDocsUrl() { |
There was a problem hiding this comment.
[/grill-with-docs] getHelpDocsUrl() and DEFAULT_SLASH_COMMAND_DOCS_URL are dead code in practice: GH_AW_SLASH_COMMAND_DOCS_URL is always baked into the generated YAML with a fixed value (line 375 of central_slash_command_workflow.go), so the env-var fallback and the JS constant can never be reached at runtime.
💡 Options
Option A (simplest) — Remove getHelpDocsUrl(), DEFAULT_SLASH_COMMAND_DOCS_URL, and the env var override in JS; hardcode the URL directly in buildHelpCommentBody.
Option B (forward-looking) — Expose a docs_url field in aw.json alongside help_command, thread it through the Go generator, and then the JS env override becomes meaningful.
Option A is correct today; Option B adds future flexibility if the URL must be customizable per-repo.
There was a problem hiding this comment.
Removed getHelpDocsUrl() and DEFAULT_SLASH_COMMAND_DOCS_URL. buildHelpCommentBody now reads process.env.GH_AW_SLASH_COMMAND_DOCS_URL directly and only appends the "Learn more" line when the URL is non-empty — eliminating the dead fallback constant entirely.
| content, err := buildCentralSlashCommandWorkflowYAML(slashRoutesByCommand, labelRoutesByCommand, mergedEvents, resolveCentralSlashRunsOn(workflowDataList), setupActionRef) | ||
| helpCommands := buildHelpCommandEntries(workflowDataList) | ||
| helpCommandEnabled := true | ||
| if repoConfig != nil { |
There was a problem hiding this comment.
[/improve-codebase-architecture] The if repoConfig != nil guard is redundant — IsHelpCommandEnabled() already handles a nil receiver and returns true. The guard adds visual noise and could mislead future readers into thinking the method is not nil-safe.
💡 Suggested simplification
helpCommandEnabled := repoConfig.IsHelpCommandEnabled()This removes the local helpCommandEnabled := true initialization and the branching block, relying on the nil-receiver contract that's already documented on IsHelpCommandEnabled.
There was a problem hiding this comment.
Fixed. Removed the if repoConfig != nil guard; helpCommandEnabled is now set directly via repoConfig.IsHelpCommandEnabled(), relying on its documented nil-receiver contract.
|
|
||
| func typeSetKeys(typeSet map[string]bool) []string { | ||
| out := make([]string, 0, len(typeSet)) | ||
| for key := range typeSet { |
There was a problem hiding this comment.
[/tdd] TestBuildHelpCommandEntries doesn't cover the conflict-logging path: the test uses triage-inline with an empty description, so the conflict branch (existing.Description != "" && description != "") is never hit. A test with two non-empty conflicting descriptions would confirm first-wins determinism and that the warning is logged.
💡 Suggested addition
func TestBuildHelpCommandEntries_ConflictingDescriptions(t *testing.T) {
var buf strings.Builder
// Temporarily redirect the logger if it supports a writer, or capture via log.SetOutput
data := []*WorkflowData{
{WorkflowID: "wf-a", Command: []string{"deploy"}, CommandCentralized: true, Description: "Deploy A"},
{WorkflowID: "wf-b", Command: []string{"deploy"}, CommandCentralized: true, Description: "Deploy B"},
}
entries := buildHelpCommandEntries(data)
require.Equal(t, 1, len(entries))
assert.Equal(t, "Deploy A", entries[0].Description, "first description should win")
// Assert log output contains the conflict warning
_ = buf
}There was a problem hiding this comment.
Added TestBuildHelpCommandEntries_ConflictingDescriptions: two workflows register the same "deploy" command with different non-empty descriptions. The test asserts len(entries) == 1 and entries[0].Description == "Deploy service A" (first-wins). The conflict-log path executes during this test even though log output is not directly captured (the logger writes to stderr only when DEBUG is set).
| return true; | ||
| } | ||
|
|
||
| core.warning(`Unable to post builtin /help response for event '${context.eventName}'.`); |
There was a problem hiding this comment.
[/tdd] The core.warning(...) path for unrecognised events (line 416) has no test. It's reached when issueNumber is falsy AND the event is neither discussion nor discussion_comment — e.g. a workflow_dispatch or push trigger.
💡 Suggested test
it("warns and returns false for /help on unsupported event type", async () => {
globals.context.eventName = "push";
globals.context.payload = { comment: { body: "/help" } }; // no issue or PR number
await main();
expect(issueCommentCalls).toHaveLength(0);
expect(globals.core.warning).toHaveBeenCalledWith(
expect.stringContaining("Unable to post builtin /help response for event 'push'")
);
});There was a problem hiding this comment.
Added test "warns and returns false for /help on unsupported event type": sets eventName = "push" with no issue/PR number, invokes main(), and asserts core.warning is called with a message containing "Unable to post builtin /help response for event 'push'" and no comment is posted.
| // Use an intermediate struct with json.RawMessage to defer maintenance parsing. | ||
| var raw struct { | ||
| GHES bool `json:"ghes,omitempty"` | ||
| HelpCommand *bool `json:"help_command,omitempty"` |
There was a problem hiding this comment.
[/grill-with-docs] The existing GHES bool in the intermediate raw struct uses omitempty, but HelpCommand *bool also uses it — while harmless here (pointer zero-value is nil), it's worth confirming the schema default semantics align: omitempty on a *bool means a JSON null is treated the same as a missing key, which is the intended "inherit default" behaviour. Consider a brief comment to make this contract explicit for future maintainers, similar to the comment above on GHES.
💡 Optional one-liner
HelpCommand *bool `json:"help_command,omitempty"` // nil = use default (enabled)Low priority — the existing IsHelpCommandEnabled godoc covers this, but inline signal at the struct level helps readers who land here first.
There was a problem hiding this comment.
Added the inline comment // nil = use default (enabled) directly on the HelpCommand *bool field in the intermediate raw struct, consistent with the suggestion. The IsHelpCommandEnabled godoc already covers the nil-receiver contract; this adds a quick signal at the struct-field level for readers landing there first.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 89/100 — Excellent
📊 Metrics & Test Classification (8 tests analyzed)
Inflation ratios (test lines added / production lines added):
Go: 6 (
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
REQUEST_CHANGES — three correctness/resiliency issues must be fixed before merge.
### Blocking issues
1. /help intercept is terminal in both states (route_slash_command.cjs:548)
The if (commandName === "help") block always returns — both when the builtin fires and when it's disabled. Any centralized workflow registered with command: help is permanently unreachable regardless of help_command setting. When the builtin is disabled the fix is to fall through to normal resolveMatchingSlashRoutes dispatch; when it's enabled, the preemption should be explicitly documented as a reservation of the help command name.
2. postBuiltinHelpComment propagates API errors uncaught (route_slash_command.cjs:396)
Unlike dispatchWorkflow, this function has no try/catch. A rate-limit, network failure, or permission error on issues.createComment / graphql throws straight through main() and hard-fails the action step. The call site already handles posted === false gracefully — wrap the body in try/catch and return false on error.
3. repoConfig = nil on load failure silently re-enables the builtin (compile_post_processing.go:130)
If aw.json is transiently unreadable in non-strict mode, IsHelpCommandEnabled() returns true for the nil receiver — a repo that explicitly set help_command: false gets the builtin re-enabled without any warning indicating that a behavior-controlling key was lost.
### Non-blocking observations
- Missing reaction for
/help(route_slash_command.cjs:550): Every other command path callsaddImmediateReaction()before dispatching./helpskips this, leaving the user with no acknowledgment until the comment appears.
🔎 Code quality review by PR Code Quality Reviewer · 134.6 AIC · ⌖ 7.8 AIC · ⊞ 5.1K
| if (commandName === "help") { | ||
| if (!isBuiltinHelpEnabled()) { | ||
| core.info("Builtin /help command is disabled by aw.json (help_command=false)."); | ||
| return; |
There was a problem hiding this comment.
/help is an unconditional dead end — custom /help workflows are permanently unreachable. Even with help_command: false, this return exits main() before the resolveMatchingSlashRoutes call, so any centralized workflow registered with command: help silently never dispatches.
💡 Suggested fix
When 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 return at line 554 silently shadows any centralized /help workflow. If that preemption is intentional, document it explicitly in the help_command schema description and in IsHelpCommandEnabled's godoc — repo admins who had a /help workflow before this PR will see it silently stop firing with no warning.
There was a problem hiding this comment.
Fixed. The /help block is now inverted: when the builtin is enabled, it intercepts, adds a reaction, posts the help comment, and returns. When the builtin is disabled, it logs an info message and falls through to resolveMatchingSlashRoutes — so custom /help workflows dispatch normally. Added a test ("dispatches custom /help workflow when builtin is disabled") that confirms dispatch when help_command=false.
| }, | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
postBuiltinHelpComment lets API errors propagate uncaught, causing the action step to hard-fail instead of warning and continuing. dispatchWorkflow (line ~281) wraps its REST call in try/catch and returns false on failure. This function follows no such pattern — a permissions error, rate-limit 429, or transient network failure throws straight through main(), which has no surrounding try/catch either.
💡 Suggested fix
Wrap both REST and GraphQL calls the same way dispatchWorkflow does:
async function postBuiltinHelpComment(commentBody) {
try {
const issueNumber = context.payload?.issue?.number ?? context.payload?.pull_request?.number;
if (issueNumber) {
await github.rest.issues.createComment({ ... });
return true;
}
if (context.eventName === "discussion" || context.eventName === "discussion_comment") {
...
await github.graphql(...);
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;
}
}This is especially important because the caller at line 550 already handles posted === false gracefully — the error-swallowing pattern is already expected by the call site.
There was a problem hiding this comment.
Fixed. postBuiltinHelpComment now wraps the entire body in try-catch, mirroring the pattern used by dispatchWorkflow. API errors emit core.warning(...) and return false. The posted check in the caller handles false gracefully — no unhandled rejections.
| core.info("Builtin /help command is disabled by aw.json (help_command=false)."); | ||
| return; | ||
| } | ||
| const posted = await postBuiltinHelpComment(buildHelpCommentBody(parseHelpCommandsMetadata())); |
There was a problem hiding this comment.
/help posts no immediate reaction, breaking the UX contract that every slash command acknowledges receipt with an emoji. Every other command path (label routes at line 508, slash routes at line 566) calls addImmediateReaction() before dispatching. /help silently skips this — the user who typed /help gets no :eyes: acknowledgment and has no way to know the command was received until the comment eventually appears.
💡 Suggested fix
Add a reaction before posting the help comment, consistent with the rest of the router:
if (commandName === "help") {
if (!isBuiltinHelpEnabled()) { ... }
// Acknowledge receipt the same way other commands do.
await addImmediateReaction("eyes");
const posted = await postBuiltinHelpComment(buildHelpCommentBody(parseHelpCommandsMetadata()));
...
}Since /help has no ai_reaction config, hardcoding "eyes" (or a dedicated constant) is fine — the important thing is that the reaction fires before the async comment write.
There was a problem hiding this comment.
Fixed. await addImmediateReaction("eyes") is now called before postBuiltinHelpComment(...) in the builtin-enabled path, consistent with the reaction-before-dispatch pattern used for label routes and slash routes. Added test "adds immediate reaction before posting builtin /help comment" to assert reactionCalls.length === 1.
| return fmt.Errorf("failed to load repo config: %w", err) | ||
| } | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to load repo config: %v", err))) | ||
| repoConfig = nil |
There was a problem hiding this comment.
When LoadRepoConfig fails in non-strict mode, setting repoConfig = nil silently re-enables the builtin /help even if help_command: false was set. IsHelpCommandEnabled() returns true for a nil receiver, so a repo that explicitly opted out gets the builtin re-enabled whenever aw.json is transiently unreadable or malformed. The warning message says nothing about help_command being overridden.
💡 Suggested fix
Either surface a more specific warning when config loss affects behavior-controlling keys, or default helpCommandEnabled to false when the config can't be loaded (fail-closed rather than fail-open):
if err != nil {
if strict {
return fmt.Errorf("failed to load repo config: %w", err)
}
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
fmt.Sprintf("Failed to load repo config; defaulting all repo-config flags (help_command etc.) to their defaults: %v", err),
))
repoConfig = nil
}Alternatively, make the callee accept an explicit helpCommandEnabled bool rather than a nullable *RepoConfig so nil can't accidentally re-enable things.
There was a problem hiding this comment.
Improved the warning message in generateCentralSlashCommandWorkflowWrapper to explicitly mention help_command and other repo-config flags: "Failed to load repo config; repo-config flags (e.g. help_command) will use defaults: ...". This makes it clear to operators that behavior-controlling keys like help_command: false revert to their defaults when aw.json is unreadable.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Run PR-Finisher skill. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Label commands are now included in the
Committed in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…action, sanitization, deduplication, dead code, compile warning Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…s empty Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…mand-help-handler # Conflicts: # pkg/workflow/repo_config.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
/help slash command with aw.json opt-out/help routing fallthrough, error handling, reaction, and mention sanitization
…tion assertions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Two test bugs were causing the failures:
|
The original
/helpbuiltin had several correctness gaps: it hard-returned even when disabled (blocking custom/helpworkflows), let API errors propagate uncaught, posted raw descriptions without sanitizing@mentions, skipped the immediate reaction that every other command path adds, and duplicated commands appearing in both centralized and decentralized sections.Bug fixes
help_command: false, the/helpblock now falls through to normalresolveMatchingSlashRoutesdispatch instead of returning early; custom/helpworkflows fire correctlypostBuiltinHelpCommentis now wrapped in try-catch (both REST and GraphQL paths); failures emitcore.warning(...)and returnfalse, matching the pattern used bydispatchWorkflowaddImmediateReaction("eyes")is called before posting the help comment, consistent with every other command path@mentionsanitization — descriptions are passed throughneutralizeDescriptionMentions()(using namedGITHUB_MENTION_REregex) before interpolation to prevent notification pingscentralizedanddecentralizedflags now appear only in the centralized sectionCleanup
getHelpDocsUrl()andDEFAULT_SLASH_COMMAND_DOCS_URL;buildHelpCommentBodyreads the env var directly and omits the "Learn more" line when the URL is emptyif repoConfig != nilguard —IsHelpCommandEnabled()already handles nil receivers// nil = use default (enabled)inline comment onHelpCommand *boolin the raw parse structCompile-time safety
buildHelpCommandEntriesnow logs a warning when any workflow registers"help"as a command name, surfacing the reservation at compile timeWarning message
generateCentralSlashCommandWorkflowWrappernow explicitly nameshelp_commandin the config-load-failure warning so operators know which flags revert to defaultsTests added
JS: discussion-context
/helpvia GraphQL, invalidGH_AW_HELP_COMMAND_ENABLEDvalue, malformed/non-arrayGH_AW_HELP_COMMANDS, unsupported event warning path, immediate reaction assertion,@mentionneutralization, centralized deduplication, API error graceful degradation, custom/helpdispatch when builtin disabledGo:
TestBuildHelpCommandEntries_ConflictingDescriptions(first-wins),TestBuildHelpCommandEntries_ReservedHelpCommandName