Align step names in daily-model-inventory Copilot billing job and frontmatter skill installer - #54406
Conversation
…ntmatter skill installer) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test Quality Sentinel analysis required: PR #54406 contains only test assertion updates to existing tests (updating string patterns to match renamed step names). No new behavioral tests were added or modified. The changes are: +2 -2 lines in activation_skills_step_test.go and +1 -1 lines in compiler_activation_steps_test.go, updating assertions to verify the renamed step format from 'Install frontmatter skill 1' to 'Install frontmatter skill: githubnext/skills'. This is a test maintenance change in response to the production code changes, not a new test quality assessment.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54406 does not have the 'implementation' label and has only 20 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good. Step names now include the actual skill identifier, making workflow logs much easier to read without needing to cross-reference the frontmatter skills: list.
One non-blocking note: frontmatterSkillStepName uses fmt.Sprintf("%q", ...) (Go quoted string) to wrap the name. If a skill identifier ever contained a " character, the resulting \" escape would produce invalid YAML. For realistic org/repo/path identifiers this will never occur, but a plain fmt.Sprintf("\"Install frontmatter skill: %s\"", identifier) would be slightly safer and more explicit.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.3 AIC · ⌖ 8.81 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — changes look solid, approving.
📋 Review Summary
What's Good
- ✅
frontmatterSkillStepNameis a well-named, focused helper — single responsibility, clear fallback logic - ✅
fmt.Sprintf("%q", ...)correctly produces YAML-safe quoted strings for names containing: - ✅ The
at > 0guard onLastIndexcorrectly avoids stripping from a leading@edge case - ✅ Tests updated to match the new naming contract
- ✅ All affected lock files recompiled consistently
Minor Observation
The fallback path "Install frontmatter skill %d" is now unreachable in practice (an empty Skill field would be a compiler validation error earlier), but keeping it as a defensive fallback is fine.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 14.8 AIC · ⌖ 9.87 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Aligns generated workflow step names with GitHub Copilot terminology and clearer skill identifiers.
Changes:
- Renames GitHub Copilot billing steps.
- Adds skill identifiers to installer step names.
- Updates tests and generated workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_activation_steps.go |
Generates descriptive skill-install names. |
pkg/workflow/compiler_activation_steps_test.go |
Updates compiler assertions. |
pkg/workflow/activation_skills_step_test.go |
Updates activation-job assertions. |
.github/workflows/daily-model-inventory.md |
Corrects GitHub Copilot naming. |
.github/workflows/daily-model-inventory.lock.yml |
Recompiles billing step names. |
.github/workflows/ponytail-reviewer.lock.yml |
Adds skill identifier. |
.github/workflows/mattpocock-skills-reviewer.lock.yml |
Adds skill identifiers. |
.github/workflows/archivx-agentic-workflows-analyzer.lock.yml |
Adds skill identifier. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
| if at := strings.LastIndex(identifier, "@"); at > 0 { | ||
| identifier = identifier[:at] | ||
| } |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
Non-blocking rename-only change; I didn't find any correctness, security, or performance regressions in the changed lines.
Themes reviewed
daily-model-inventory.md: step-name wording onlypkg/workflow/compiler_activation_steps.go: display-name helper strips@pinpurely for readability while preserving the original skill ref inGH_AW_FRONTMATTER_SKILLS- Updated tests still cover the generated names that matter
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.02 AIC · ⌖ 7.84 AIC · ⊞ 4.6K
Comment /review to run again
|
@copilot resolve the merge conflicts on this branch. |
…mes-daily-model-inventory # Conflicts: # .github/workflows/daily-model-inventory.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Two compiler-generated step names in
.lock.ymlworkflows drifted from the naming glossary and clarity standard: a Copilot billing job indaily-model-inventory.mdstill used the bare "Copilot" form while sibling steps in the same file already used "GitHub Copilot", and the frontmatter skill installer named steps only by index, giving no indication of which skill was being installed.daily-model-inventory.mdCreate placeholder for Copilot billing models→Create placeholder for GitHub Copilot billing modelsUpload Copilot billing models artifact→Upload GitHub Copilot billing models artifactdaily-model-inventory.lock.ymlFrontmatter skill installer (
pkg/workflow/compiler_activation_steps.go)skillRef.Skillvalue already passed toGH_AW_FRONTMATTER_SKILLS@<sha>pin is stripped for readability; falls back to the numbered form if the identifier is empty.lock.ymlfiles (archivx-agentic-workflows-analyzer,mattpocock-skills-reviewer,ponytail-reviewer)Existing unit tests covering these step names were updated to match.