Add step-summary command visibility to agentic_commands dispatcher#36346
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves observability of the centralized agentic_commands slash-command dispatcher by appending routing details (configured commands + selected command from the payload) to the GitHub Actions step summary, and extends the unit tests to verify that summary output for slash and non-slash payloads.
Changes:
- Added
appendRoutingSummary(existingCommands, selectedCommand)to write routing context intoGITHUB_STEP_SUMMARY. - Moved payload token parsing earlier in
main()and logs the selected command regardless of routing match outcome. - Extended
route_slash_commandtests to assert step-summary output for both slash and non-slash payloads.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/route_slash_command.cjs | Adds step-summary instrumentation and parses the selected slash command earlier for consistent logging. |
| actions/setup/js/route_slash_command.test.cjs | Adds assertions verifying the step-summary output for slash and non-slash payloads. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| if (!summary || typeof summary.addHeading !== "function" || typeof summary.addRaw !== "function" || typeof summary.write !== "function") { | ||
| return; | ||
| } |
| const existingCommandsText = normalizedCommands.length ? normalizedCommands.join(", ") : "<none>"; | ||
| const selectedCommandText = selectedCommand ? `/${selectedCommand}` : "<none>"; |
| expect(summaryMock.addRaw).toHaveBeenCalledWith("- Existing commands: /archie", true); | ||
| expect(summaryMock.addRaw).toHaveBeenCalledWith("- Selected command: <none>", true); | ||
| expect(summaryMock.write).toHaveBeenCalledWith({ overwrite: false }); |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36346 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well under the 100-line threshold). No custom design-gate config present. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs and one security-adjacent ordering issue must be fixed before merge.
### Blocking issues
-
<none>renders invisible in GitHub Markdown (line 27) —<none>is parsed as an HTML tag and disappears in rendered output. Use`<none>`or<none>. -
addEOLmissing from capability guard (line 19) — the guard checksaddHeading/addRaw/writebut notaddEOL. A missingaddEOLthrows inside the chained call and falls into catch rather than cleanly no-oping. -
Command inventory written before auth checks (line 255, new comment) —
appendRoutingSummaryfires for every event, exposing the full slash-command list and the actor's attempted command in the step summary before any membership/identity guard runs.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.2M
| const labelRouteMap = JSON.parse(process.env.GH_AW_LABEL_ROUTING || "{}"); | ||
| core.info(`Configured centralized slash commands: ${Object.keys(slashRouteMap).length}.`); | ||
| core.info(`Configured decentralized label commands: ${Object.keys(labelRouteMap).length}.`); | ||
| const text = resolveBodyText(); |
There was a problem hiding this comment.
Command inventory exposed before auth checks: appendRoutingSummary writes all configured slash commands and the actor's attempted command to the step summary before any identity or membership validation runs. Any user who can post a comment will get a step summary showing the full command list, even if they're subsequently rejected.
💡 Suggested fix
Move the appendRoutingSummary call to after all auth/membership guards pass — i.e., just before the command dispatch logic where commandName is already validated:
// After auth checks pass...
const commandName = selectedCommand;
core.info(`Resolved command '/${commandName}'...`);
await appendRoutingSummary(Object.keys(slashRouteMap), commandName);Alternatively, if visibility of configured commands on unauthorized events is intentional for diagnostics, document that decision explicitly — but the current code gives no indication this is deliberate.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — overall the change is clean and well-motivated, with a few test-coverage gaps worth addressing.
📋 Key Themes & Highlights
Key Themes
- Untested branches: The
core.summaryguard path and thesummary.writerejection path both have no test coverage — see inline comments. - Unconditional summary write: The routing summary fires before
isPRClosedAtStart()and label-event guards, which means it also runs for events where slash-command routing is irrelevant. Documenting the intent (or moving the call) would prevent confusion.
Positive Highlights
- ✅ Non-overwrite write strategy (
{ overwrite: false }) is the right choice to avoid clobbering prior summary content. - ✅
normalizedCommandsis sorted before display — consistent ordering makes step summaries easy to compare across runs. - ✅
selectedCommand = firstWord.startsWith("/") ? firstWord.slice(1) : ""eliminates the duplicated parsing logic that previously existed downstream, and thecommandName = selectedCommandreassignment keeps the change surgical. - ✅ Two new test cases cover the happy path and the non-slash payload path.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
Comments that could not be inline-anchored
actions/setup/js/route_slash_command.cjs:34
[/tdd] The if (!summary || ...) guard branch is never exercised — if core.summary is absent the function silently returns, but this path has no test coverage.
<details>
<summary>💡 Suggested test</summary>
Add a case that omits core.summary and verifies neither an error nor a summary write occurs:
it("handles missing core.summary gracefully", async () => {
globals.core.summary = undefined;
await main();
// dispatch still proceeds
expect(dispatchCalls).toHaveLength(1);…
</details>
<details><summary>actions/setup/js/route_slash_command.cjs:44</summary>
**[/tdd]** The `catch` block (line 44) is reachable when `summary.write()` rejects, but no test covers this path — so the `core.warning` call and the non-throwing contract are untested.
<details>
<summary>💡 Suggested test</summary>
```js
it("warns but does not throw when summary.write rejects", async () => {
summaryMock.write = vi.fn(async () => { throw new Error("disk full"); });
await expect(main()).resolves.not.toThrow();
expect(globals.core.warning).toHaveBeenCalledWith(
expect…
</details>
<details><summary>actions/setup/js/route_slash_command.cjs:258</summary>
**[/zoom-out]** `appendRoutingSummary` runs unconditionally before the `isPRClosedAtStart()` guard and the `labeled` event branch. For a closed-PR or label event, the summary section will still be written — `selectedCommand` will show `<none>` even though slash-command routing is not applicable for those paths.
If this is intentional (always log what arrived), a brief comment here would make the intent clear. If slash-command summary should only appear for slash-command–eligible events, consid…
</details>
The centralized
agentic_commandsdispatcher did not expose which commands were configured or which command was selected for the current event in the workflow step summary. This change adds explicit summary output so routing decisions are visible at-a-glance during triage.Summary instrumentation
appendRoutingSummary(existingCommands, selectedCommand)inroute_slash_command.cjs.GITHUB_STEP_SUMMARYwith:<none>when absent)Dispatcher flow update
main().Test coverage
route_slash_command.test.cjsto assert step-summary output for:/archie ...)Selected command: <none>)