Add run-again footer hints for slash and label command triggers#41920
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41920 does not have the 'implementation' label and has only 13 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR improves the default safe-output footer “run again” guidance by adding explicit label-trigger hints alongside the existing slash-command hints, and wires label-command metadata through compiler-generated environment variables so footer rendering can resolve label names at runtime.
Changes:
- Extend default footer rendering to include a label-based rerun hint (and render both label + slash hints when both are configured).
- Plumb
GH_AW_LABEL_COMMANDSthrough safe-output environment generation (Go compiler + generated YAML env blocks). - Add/extend JS and Go tests to cover label-only and combined hint behavior, plus env-var generation.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml.go | Emits GH_AW_LABEL_COMMANDS in generated env blocks alongside existing command metadata. |
| pkg/workflow/compiler_safe_outputs_job.go | Adds GH_AW_LABEL_COMMANDS to job-level safe-output env var generation. |
| pkg/workflow/compiler_safe_outputs_job_test.go | Extends env-var generation coverage to validate GH_AW_LABEL_COMMANDS. |
| actions/setup/js/messages_footer.cjs | Adds label rerun hint rendering and factors “first command” selection into a shared helper. |
| actions/setup/js/messages.test.cjs | Adds footer tests for label-only and slash+label combined rerun hint output (and env-driven label hint rendering). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics & Test Classification (5 tests analyzed)
Go: 1 ( Verdict
|
![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 and /grill-with-docs — no blocking correctness issues, but a few test-coverage gaps and a design-consistency point worth addressing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps (3 items):
getFirstCommandHinthas no isolated unit tests;generateOutputCollectionStepincompiler_yaml.gois untested; label-command-only scenario missing from Go tests. - Design asymmetry: label command hint text is hardcoded while slash commands support
slashCommandPlaceholder— either document the intentional limit or extend the API to match. - Comment quality: extracting
getFirstCommandHintdropped the "compiler-generated JSON" context that justified the silent error ignore.
Positive Highlights
- ✅ Clean extraction of
getFirstCommandHint— good refactor that eliminates the duplicated JSON-parse block. - ✅ The combined slash+label test uses
toBe(exact match) — prevents silent ordering regressions. - ✅
GH_AW_LABEL_COMMANDSis correctly cleaned up in the globalafterEach, making individual test cleanups safe even if assertions throw. - ✅ The two-site env var emission (
compiler_safe_outputs_job.go+compiler_yaml.go) is consistent with the existing slash-command pattern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 79.6 AIC · ⌖ 9.91 AIC · ⊞ 6.6K
| } catch { | ||
| // Silently ignore malformed JSON; hints are non-critical enhancements. | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
[/tdd] getFirstCommandHint is tested only indirectly through generateFooterWithMessages. Direct unit tests for its edge cases would make the contract explicit and catch regressions faster.
💡 Suggested test cases to add
describe("getFirstCommandHint", () => {
it("returns undefined for undefined input", () =>
expect(getFirstCommandHint(undefined)).toBeUndefined());
it("returns undefined for empty array JSON", () =>
expect(getFirstCommandHint(JSON.stringify([]))).toBeUndefined());
it("returns undefined when first element is not a string", () =>
expect(getFirstCommandHint(JSON.stringify([42]))).toBeUndefined());
it("returns undefined for malformed JSON", () =>
expect(getFirstCommandHint("not-json")).toBeUndefined());
it("returns first command from valid array", () =>
expect(getFirstCommandHint(JSON.stringify(["deploy", "release"]))).toBe("deploy"));
});Without isolated tests, a future change to getFirstCommandHint that breaks the empty-array or non-string guards would only be caught if an existing integration-level test exercises that path.
@copilot please address this.
| } | ||
| if len(data.LabelCommand) > 0 { | ||
| if labelCommandsJSON, err := json.Marshal(data.LabelCommand); err == nil { | ||
| fmt.Fprintf(yaml, " GH_AW_LABEL_COMMANDS: %q\n", string(labelCommandsJSON)) |
There was a problem hiding this comment.
[/tdd] The GH_AW_LABEL_COMMANDS emission in generateOutputCollectionStep has no test coverage. compiler_yaml_test.go has 21 test functions but none exercise this path — a label-command workflow fixture would add meaningful protection here.
💡 Suggested test sketch
Add a test case to TestGenerateOutputCollectionStep (or the nearest existing snapshot/fixture test) that sets data.LabelCommand = []string{"deploy"} and asserts the emitted YAML contains GH_AW_LABEL_COMMANDS: "[\"deploy\"]".
This mirrors the existing test coverage gap for GH_AW_COMMANDS in compiler_yaml_test.go — worth closing for both while the context is fresh.
@copilot please address this.
| workflowData: &WorkflowData{ | ||
| Name: "Test Workflow", | ||
| Command: []string{"review-bot"}, | ||
| LabelCommand: []string{"deploy"}, |
There was a problem hiding this comment.
[/tdd] LabelCommand is added to the combined slash-command-plus-placeholder test case only. A dedicated label-command-only test case (no Command set) would verify isolation and serve as a clearer specification.
💡 Suggested additional test case
{
name: "with label command only",
workflowData: &WorkflowData{
Name: "Test Workflow",
LabelCommand: []string{"deploy"},
SafeOutputs: &SafeOutputsConfig{},
},
workflowID: "test-workflow",
expectedVars: map[string]string{
"GH_AW_LABEL_COMMANDS": `"[\"deploy\"]"`,
},
notExpectedVars: []string{"GH_AW_COMMANDS"},
checkContains: true,
},This ensures the env var is emitted correctly even when there is no accompanying slash command, and confirms GH_AW_COMMANDS is not mistakenly injected.
@copilot please address this.
| } | ||
| // Append label command hint when applicable (workflow has a label command trigger) | ||
| if (ctx.labelCommand) { | ||
| defaultFooter += `\n> <sub>Add label <em>{label_command}</em> to run again</sub>`; |
There was a problem hiding this comment.
[/grill-with-docs] The label command hint hard-codes "to run again" while the analogous slash command hint supports slashCommandPlaceholder for custom text (e.g. "to review this PR"). This asymmetry should be intentional — if so, please document it in the JSDoc; if not, a labelCommandPlaceholder field would make the two hints consistent.
💡 Documenting the intentional asymmetry
If customisation is intentionally out of scope for now, add a note to the FooterContext JSDoc:
* `@property` {string} [labelCommand] - Label command name for the run-again hint.
* Note: unlike slash commands, the hint text is fixed to "to run again".
* Add labelCommandPlaceholder when custom text is needed.This prevents future contributors from adding the placeholder in two inconsistent places.
@copilot please address this.
| return commands[0]; | ||
| } | ||
| } catch { | ||
| // Silently ignore malformed JSON; hints are non-critical enhancements. |
There was a problem hiding this comment.
[/grill-with-docs] The catch comment lost a useful piece of context from the original inline slash-command block: "The value is compiler-generated JSON, so this path should not occur in practice." Preserving that reasoning helps future readers understand why the silent ignore is safe.
💡 Suggested updated comment
} catch {
// Silently ignore malformed JSON; hints are non-critical enhancements.
// In practice this path should not occur — the value is compiler-generated JSON.
}@copilot please address this.
There was a problem hiding this comment.
Functionally correct — one non-blocking gap
The template substitution is sound (verified via toSnakeCase({ ...ctx }) converting labelCommand → label_command), getFirstCommandHint is a clean DRY refactor of the existing inline JSON parsing, the Go error-handling pattern is consistent with GH_AW_COMMANDS in the same files, and test teardown is safe (vi.resetModules() + beforeEach cleanup cover it).
One medium gap: the label hint hardcodes "to run again" with no labelCommandPlaceholder equivalent, unlike the slash command hint. Commented inline — non-blocking but worth addressing before the next feature that needs custom text.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 102.5 AIC · ⌖ 7.03 AIC · ⊞ 5.2K
| } | ||
| // Append label command hint when applicable (workflow has a label command trigger) | ||
| if (ctx.labelCommand) { | ||
| defaultFooter += `\n> <sub>Add label <em>{label_command}</em> to run again</sub>`; |
There was a problem hiding this comment.
Label hint text is hardcoded with no customization path, unlike slashCommandPlaceholder which allows workflows to say "to review this PR" etc.
💡 Suggested improvement
slashCommand supports custom hint text via slashCommandPlaceholder (read from GH_AW_COMMAND_PLACEHOLDER). The label hint hardcodes "to run again" with no equivalent escape hatch:
// Current
defaultFooter += `\n> <sub>Add label <em>{label_command}</em> to run again</sub>`;
// With parity
const labelHintText = ctx.labelCommandPlaceholder || "to run again";
defaultFooter += `\n> <sub>Add label <em>{label_command}</em> ${labelHintText}</sub>`;This would also require:
labelCommandPlaceholderin theFooterContexttypedefGH_AW_LABEL_COMMAND_PLACEHOLDERenv var ingenerateFooterWithMessages- Corresponding compiler emission in
compiler_safe_outputs_job.goandcompiler_yaml.go
A deploy workflow will never be able to say "to trigger deployment" instead of the generic "to run again" without this. Since slashCommandPlaceholder already exists and the two hint types are rendered in the same block, the omission will be noticed.
Generated safe-output footers already hinted slash-command re-entry, but did not guide users for label-command flows. This change makes footer hints explicit for both trigger types so users can immediately re-run workflows with the correct action.
Footer behavior
Runtime metadata plumbing
GH_AW_LABEL_COMMANDSto safe-outputs environment generation so footer rendering can resolve label-command names at runtime.Compiler output updates
GH_AW_LABEL_COMMANDSin generated safe-output ingestion env blocks where command metadata is already passed.Coverage updates
GH_AW_LABEL_COMMANDS.