fix: update stale on.command: reference to on.slash-command: in check_command_position - #48916
Conversation
…_position Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
on.command: reference to on.slash-command: in check_command_position
|
@copilot update all occurrences |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Attempts to update slash-command terminology, but changes several valid slash_command references to unsupported slash-command.
Changes:
- Updates the command-position denial hint.
- Revises codemod metadata, logs, and tests.
- Changes related implementation and test comments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/check_command_position.cjs |
Updates denial guidance. |
pkg/cli/codemod_slash_command.go |
Revises migration terminology. |
pkg/cli/codemod_slash_command_test.go |
Updates metadata expectation. |
pkg/cli/fix_command_test.go |
Changes fixture commentary. |
pkg/workflow/frontmatter_extraction_yaml.go |
Changes legacy-format commentary. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
pkg/cli/codemod_slash_command.go:16
- These user-visible codemod details claim the migration produces
on.slash-command, whileApplyactually writesslash_commandand that is the schema-supported key. Runninggh aw fixwill therefore describe its own output incorrectly.
Name: "Migrate on.command to on.slash-command",
Description: "Replaces deprecated 'on.command' field with 'on.slash-command'",
pkg/cli/codemod_slash_command.go:65
- This log reports that the line was replaced with
on.slash-command, but line 61 actually writesslash_command. The debug trace becomes misleading when diagnosing a migration.
slashCommandCodemodLog.Printf("Replaced on.command with on.slash-command on line %d", i+1)
pkg/cli/codemod_slash_command.go:76
- This completion log names
on.slash-commandeven though the applied migration emitson.slash_command, making successful fix logs inaccurate.
slashCommandCodemodLog.Print("Applied on.command to on.slash-command migration")
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48916 does not have the 'implementation' label and has only 8 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![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 /diagnosing-bugs — requesting changes on two comments that were incorrectly updated.
📋 Key Themes & Highlights
Issues Found
- Inverted deprecation comment (
frontmatter_extraction_yaml.go): The comment was changed to labelon.slash-commandas the legacy/deprecated format — but it is the current format. The legacy format ison.command. This misleads maintainers. - Inaccurate test fixture comment (
fix_command_test.go): The comment was changed to say the fixture useson.slash-command, but the fixture deliberately uses the deprecatedon.commandfield so the codemod can migrate it. The original comment was correct.
Positive Highlights
- ✅ The user-facing hint in
check_command_position.cjsis correctly updated - ✅ Codemod
Name,Description, and log messages are consistently updated to useon.slash-command - ✅ Test assertion in
codemod_slash_command_test.gocorrectly tracks the name change
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.3 AIC · ⌖ 5.01 AIC · ⊞ 7K
Comment /matt to run again
| frontmatterLog.Print("Extracting command configuration from frontmatter") | ||
| // Check new format: on.slash_command or on.slash_command.name (preferred) | ||
| // Also check legacy format: on.command or on.command.name (deprecated) | ||
| // Also check legacy format: on.slash-command or on.slash-command.name (deprecated) |
There was a problem hiding this comment.
[/diagnosing-bugs] Incorrect comment: on.slash-command is the current preferred syntax, not the legacy/deprecated one — the legacy format is on.command. This edit inverts the comment's meaning and will mislead maintainers.
💡 Suggested fix
Revert this line to the original:
// Also check legacy format: on.command or on.command.name (deprecated)The code itself correctly falls back to command (not slash-command) when slash_command is absent, so the original comment accurately described the behaviour.
@copilot please address this.
| workflowFile := filepath.Join(tmpDir, "test-workflow.md") | ||
|
|
||
| // Create a workflow with deprecated on.command field | ||
| // Create a workflow with deprecated on.slash-command field |
There was a problem hiding this comment.
[/diagnosing-bugs] Inaccurate test comment: the test creates a workflow with the deprecated on.command field (that's the whole point — the codemod migrates away from it). Changing the comment to say on.slash-command describes the output, not the input fixture being set up.
💡 Suggested fix
Revert to the original comment:
// Create a workflow with deprecated on.command fieldThis comment correctly describes the test fixture. The test validates that the codemod transforms on.command → on.slash-command; describing the fixture as already using on.slash-command is misleading.
@copilot please address this.
There was a problem hiding this comment.
Review Summary
The core intent is correct: replacing stale on.command: references with on.slash-command:. However, two code comments were changed incorrectly, inverting what is deprecated vs. what is new.
Blocking issues (2):
pkg/workflow/frontmatter_extraction_yaml.goline 225 — The updated comment now sayson.slash-commandis the legacy/deprecated format, buton.slash-commandis the new preferred format. The deprecated trigger ison.command; revert this comment.pkg/cli/fix_command_test.goline 560 — The comment now says the test uses a deprecatedon.slash-commandfield, but the test fixture actually uses thecommand:YAML key. The original comment was accurate; revert it.
All other changes (user-facing error message, codemod metadata, log messages, unit test assertion) look correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.4 AIC · ⌖ 4.9 AIC · ⊞ 5.3K
| frontmatterLog.Print("Extracting command configuration from frontmatter") | ||
| // Check new format: on.slash_command or on.slash_command.name (preferred) | ||
| // Also check legacy format: on.command or on.command.name (deprecated) | ||
| // Also check legacy format: on.slash-command or on.slash-command.name (deprecated) |
There was a problem hiding this comment.
The updated comment is incorrect. The legacy/deprecated trigger is on.command (not on.slash-command). on.slash-command is actually the new preferred format. The comment should remain as it was:
// Also check legacy format: on.command or on.command.name (deprecated)
@copilot please address this.
| workflowFile := filepath.Join(tmpDir, "test-workflow.md") | ||
|
|
||
| // Create a workflow with deprecated on.command field | ||
| // Create a workflow with deprecated on.slash-command field |
There was a problem hiding this comment.
The comment was incorrectly changed. The test creates a workflow with the deprecated on.command field (the YAML key command: is on line 563), not on.slash-command. Revert this comment to:
// Create a workflow with deprecated on.command field
@copilot please address this.
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Maintenance
📊 Analysis Summary
Changed test files:
Verdict
|
There was a problem hiding this comment.
Verdict: Request Changes — the fix replaces one wrong string with another wrong string
This PR's stated goal is to correct a stale on.command: reference, but it consistently substitutes on.slash-command: (hyphen) where the actual, schema-defined frontmatter key is on.slash_command: (underscore). This error is repeated across all 5 changed files, including in a user-facing denial message, a codemod's name/description/log strings (which now contradict the codemod's own findAndReplaceInLine call using the underscore), and two code comments that now describe the legacy field incorrectly.
💡 Details and evidence
pkg/parser/schemas/main_workflow_schema.jsonline 531 defines"slash_command"(underscore) as the schema key — confirmed noslash-command(hyphen) key exists anywhere in the schema or extraction code.pkg/workflow/frontmatter_extraction_yaml.gocallsextractOnTriggerValue(frontmatter, "slash_command")andextractOnTriggerValue(frontmatter, "command")— neverslash-command.pkg/cli/codemod_slash_command.go'sfindAndReplaceInLine(line, "command", "slash_command")still replaces with the underscored field, but the surrounding Name/Description/log text now claims it producesslash-command.
All four issues are inline. Recommend reverting every slash-command (hyphen) substitution to slash_command (underscore) and re-verifying only the specific check_command_position.cjs denial message (which genuinely was stale) against the schema before resubmitting.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 39.1 AIC · ⌖ 4.88 AIC · ⊞ 7.8K
Comment /review to run again
| await writeDenialSummary( | ||
| `The trigger comment did not start with a required command. Expected one of: ${expectedCommands}. Found: \`${firstWord}\`.`, | ||
| "Make sure the trigger comment starts with the required command defined in `on.command:` in the workflow frontmatter." | ||
| "Make sure the trigger comment starts with the required command defined in `on.slash-command:` in the workflow frontmatter." |
There was a problem hiding this comment.
This introduces a new incorrect reference: the actual frontmatter key is on.slash_command: (underscore), not on.slash-command: (hyphen).
💡 Why this is wrong
The PR aims to fix a stale reference but swaps one wrong string for another. Per pkg/parser/schemas/main_workflow_schema.json (line 531) and pkg/workflow/frontmatter_extraction_yaml.go, the field is defined as slash_command with an underscore, e.g.:
on:
slash_command: my-botThere is no hyphenated slash-command key anywhere in the schema or extraction code. This user-facing message will now direct users to a syntax that will not parse/match, defeating the purpose of the fix.
Fix:
"Make sure the trigger comment starts with the required command defined in `on.slash_command:` in the workflow frontmatter."| var slashCommandCodemodLog = logger.New("cli:codemod_slash_command") | ||
|
|
||
| // getCommandToSlashCommandCodemod creates a codemod for migrating on.command to on.slash_command | ||
| // getCommandToSlashCommandCodemod creates a codemod for migrating on.command to on.slash-command |
There was a problem hiding this comment.
Same underscore-vs-hyphen error repeated in comments/log strings — the target field is slash_command, not slash-command.
💡 Why this is wrong
The codemod itself calls findAndReplaceInLine(line, "command", "slash_command") (line 61) — i.e. the actual code correctly produces slash_command with an underscore. But this PR changes the surrounding comments, Name, Description, and log messages to say slash-command (hyphen), which now contradicts the code's own behavior. Anyone reading the codemod name/description or debug logs will be told the tool replaces with a hyphenated key when it actually writes an underscored one.
Fix: revert these five string changes to slash_command (underscore) to match what findAndReplaceInLine actually produces, and instead fix only the truly stale hint in check_command_position.cjs.
| frontmatterLog.Print("Extracting command configuration from frontmatter") | ||
| // Check new format: on.slash_command or on.slash_command.name (preferred) | ||
| // Also check legacy format: on.command or on.command.name (deprecated) | ||
| // Also check legacy format: on.slash-command or on.slash-command.name (deprecated) |
There was a problem hiding this comment.
This comment now states the legacy field is on.slash-command, but the code two lines below calls extractOnTriggerValue(frontmatter, "command") — the legacy key is command, not slash-command.
💡 Why this is wrong
Before this change the comment read "Also check legacy format: on.command or on.command.name (deprecated)" which correctly matched the code (extractOnTriggerValue(frontmatter, "command") at line 227). The edit replaces the accurate legacy field name (command) with a nonexistent one (slash-command), making the comment actively misleading and disconnected from the code it documents. This is a case of "fixing" a comment by making it wrong in a new way.
Fix: revert to // Also check legacy format: on.command or on.command.name (deprecated).
|
|
||
| func TestFixCommand_CommandToSlashCommandMigration(t *testing.T) { | ||
| // Create a temporary directory for test files | ||
| tmpDir := t.TempDir() |
There was a problem hiding this comment.
This comment change is also incorrect: the test literally uses command: (line below), not slash-command: — the comment now contradicts the test fixture directly above it.
💡 Why this is wrong
The test content is:
on:
command: my-botThe comment was changed from "Create a workflow with deprecated on.command field" to "Create a workflow with deprecated on.slash-command field", but the fixture never contains slash-command — it contains command. This mislabels the very fixture it precedes and will confuse future maintainers reading the test.
Fix: revert to // Create a workflow with deprecated on.command field.
|
@copilot run pr-finisher skill |
…oss all files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in commit
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/30468492325
|
All review threads are resolved and the branch is up to date with CI must be re-triggered by a maintainer (close/reopen the PR or |
|
@copilot make sure all "on.command" are changed to "on.slash_command" to match proper syntax |
The user-facing reference in The remaining
No changes are needed beyond what's already been done. |
|
🎉 This pull request is included in a new release. Release: |
The denial message shown when a trigger comment fails slash-command validation still pointed users to the deprecated
on.command:frontmatter key instead of the currenton.slash-command:syntax.Change
actions/setup/js/check_command_position.cjs: updated the user-facing hint inwriteDenialSummaryfrom`on.command:`→`on.slash-command:`Migration-context references in
codemod_slash_command.goand related tests (e.g."Migrate on.command to on.slash_command") were intentionally left unchanged — they name the deprecated field accurately as the source of the migration.Run: https://github.com/github/gh-aw/actions/runs/30468492325