Remove expression support from skills frontmatter; add skills to gh-aw-manifest#42676
Remove expression support from skills frontmatter; add skills to gh-aw-manifest#42676Copilot wants to merge 4 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…r; add skills to gh-aw-manifest Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42676 does not have the 'implementation' label and has only 59 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR makes workflow skills: references statically resolvable at compile time by removing GitHub Actions expression support in skill specs, and records the resulting (static) skill list in the # gh-aw-manifest: header for deterministic auditing/pinning.
Changes:
- Tightened
skills:frontmatter validation to only allowowner/repo@<40-hex-sha>andowner/repo/skill/path@<40-hex-sha>(no${{ ... }}expressions). - Extended
gh-aw-manifestto include a sorted/deduplicatedskillslist and wired it into YAML header generation. - Updated/removed tests that previously asserted expression-based skill refs were accepted or propagated.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/skills_frontmatter.go | Removes expression acceptance and enforces static skill spec format. |
| pkg/workflow/skills_frontmatter_test.go | Updates validation expectations to reject expression-based skill refs. |
| pkg/workflow/safe_update_manifest.go | Adds Skills to GHAWManifest and sorts/deduplicates before serialization. |
| pkg/workflow/safe_update_manifest_test.go | Adds coverage for skills sorting/dedup and updates constructor call sites. |
| pkg/workflow/compiler_yaml.go | Passes workflow skills into NewGHAWManifest for lock-header emission. |
| pkg/workflow/compiler_orchestrator_frontmatter_test.go | Expects compile-time error when skills use expressions. |
| pkg/workflow/activation_skills_step_test.go | Removes expression-skill activation job test (no longer supported). |
| actions/setup/js/add_reaction_and_edit_comment.cjs | Pure formatting change (ternary condensed to one line). |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Regenerated lock header content after compiler changes. |
| .github/workflows/test-quality-sentinel.lock.yml | Regenerated lock header content after compiler changes. |
| .github/workflows/pr-sous-chef.lock.yml | Regenerated lock header content after compiler changes. |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Regenerated lock header content after compiler changes. |
| .github/workflows/daily-geo-optimizer.lock.yml | Regenerated lock header content after compiler changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Low
| Version int `json:"version"` | ||
| Secrets []string `json:"secrets"` | ||
| Actions []GHAWManifestAction `json:"actions"` | ||
| Skills []string `json:"skills,omitempty"` // frontmatter skill specs (owner/repo@sha), sorted |
| // Embed the gh-aw-manifest immediately after gh-aw-metadata for easy machine parsing. | ||
| // The manifest records all secrets, external actions, and container images detected at | ||
| // compile time so that subsequent compilations can perform safe update enforcement. | ||
| manifest := NewGHAWManifest(secrets, actions, data.ActionResolutionFailures, data.DockerImagePins, data.Redirect) | ||
| manifest := NewGHAWManifest(secrets, actions, data.ActionResolutionFailures, data.DockerImagePins, data.Redirect, data.Skills) |
|
@copilot fix impeccable workflow https://github.com/github/gh-aw/actions/runs/28499128376/job/84472065029#step:11:1 |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (4 tests)
|
| Component | Calculation | Points |
|---|---|---|
| Design ratio (4/4) | 1.0 × 40 | 40.0 |
| Edge coverage (3/4) | 0.75 × 30 | 22.5 |
| Duplicate penalty (0 clusters) | 20 − 0 | 20.0 |
| Inflation bonus | Inflation detected (minor) | 0 |
| Total | 82.5 → 83 |
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). No guideline violations.
This PR correctly removes expression support from skills frontmatter and adds skills tracking to the manifest. All changed tests enforce behavioral contracts: two tests flip from accepting to rejecting GitHub Actions expressions (verifying the new stricter validation policy), and two new table rows verify manifest skills sorting/deduplication and nil/omitempty semantics. The deletion of
TestBuildActivationJob_AddsExpressionSkillInstallStepsis appropriate cleanup for the removed feature.
References: §28499128388
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
awmgmcpg
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
🧪 Test quality analysis by Test Quality Sentinel · 61.9 AIC · ⌖ 10.5 AIC · ⊞ 6.8K · ◷
Comment /review to run again
PR Review: Remove expression support from skills frontmatter 🧠Applied 📋 Issues found (4)
@copilot please address the review comments above.
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 96.8 AIC · ⌖ 7.45 AIC · ⊞ 6.6K
Comment /matt to run again
| safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d", | ||
| currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers)) | ||
| safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d, skills=%d", | ||
| currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers), len(skillSpecs)) |
There was a problem hiding this comment.
[/codebase-design] The "Manifest built" log records len(skillSpecs) (the raw input count) before deduplication, so if duplicates are present the logged count will be higher than what ends up in the manifest.
💡 Suggested fix
Move the log after the dedup/sort block, using len(sortedSkills) for consistency with how secrets and actions counts are reported:
sort.Strings(sortedSkills)
if len(sortedSkills) == 0 {
sortedSkills = nil
}
safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d, skills=%d",
currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers), len(sortedSkills))@copilot please address this.
| wantVersion: 1, | ||
| wantSecrets: []string{}, | ||
| wantSkills: []string{ | ||
| "githubnext/skills/review/security@1f181b37d3fe5862ab590648f25a292e345b5de6", |
There was a problem hiding this comment.
[/tdd] The sorted-and-deduplicated test case (line 140–151) only exercises the case where the "lexicographically later" value survives. Consider adding a test case that verifies the first element alphabetically wins — i.e. "githubnext/skills@abc" < "githubnext/skills@xyz" is correctly ordered — and a case with an empty-string skill spec to confirm the blank-filtering path is exercised.
💡 Missing edge cases
{
name: "empty-string skill specs are filtered out",
skillSpecs: []string{
"",
"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6",
"",
},
wantVersion: 1,
wantSecrets: []string{},
wantSkills: []string{
"githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de6",
},
},The validation layer should already prevent empty strings from reaching here, but testing the filtering in NewGHAWManifest directly makes the contract explicit and prevents silent data corruption if something bypasses validation.
@copilot please address this.
| require.Error(t, err, "expected error: GitHub Actions expressions are not allowed in skills refs") | ||
| assert.Nil(t, result) | ||
| assert.True(t, | ||
| strings.Contains(err.Error(), "40-char-sha") || strings.Contains(err.Error(), "does not match pattern"), |
There was a problem hiding this comment.
[/tdd] The assertion on line 319 accepts two different error message substrings ("40-char-sha" OR "does not match pattern"). The second branch ("does not match pattern") does not exist in the current error message produced by validateSkillSpecValue, so this disjunction is dead code. Narrow the assertion to the string that is actually produced.
💡 Suggested simplification
Since validateSkillSpecValue always returns:
skills[N] must use owner/repo@<40-char-sha> or owner/repo/skill/path@<40-char-sha> (got %q). ...
The test can simply assert:
require.ErrorContains(t, err, "40-char-sha")The "does not match pattern" branch was likely defensive scaffolding from a previous iteration. Keeping it dilutes signal — if the error message ever changes, this test would silently still pass via the wrong branch.
@copilot please address this.
| @@ -124,7 +124,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD | |||
| // Embed the gh-aw-manifest immediately after gh-aw-metadata for easy machine parsing. | |||
| // The manifest records all secrets, external actions, and container images detected at | |||
There was a problem hiding this comment.
[/codebase-design] The inline comment on line 124–126 still says "secrets, external actions, and container images" — it does not mention skills. Since skills are now a first-class manifest field alongside secrets and actions, the comment should be updated to stay accurate.
💡 Suggested update
// Embed the gh-aw-manifest immediately after gh-aw-metadata for easy machine parsing.
// The manifest records all secrets, external actions, container images, and skill specs
// detected at compile time so that subsequent compilations can perform safe update enforcement.@copilot please address this.
| Version int `json:"version"` | ||
| Secrets []string `json:"secrets"` | ||
| Actions []GHAWManifestAction `json:"actions"` | ||
| Skills []string `json:"skills,omitempty"` // frontmatter skill specs (owner/repo@sha), sorted |
There was a problem hiding this comment.
[/codebase-design] The Skills field is placed between Actions and ResolutionFailures in the struct but the JSON field ordering in the lock file header will follow struct field order. Other manifest fields (Secrets, Actions) are always present (not omitempty), while Skills uses omitempty. This is fine, but consider whether downstream consumers that parse the manifest JSON should be documented as needing to handle a missing skills key — especially tools that detect newly introduced skills as a "safe update" concern.
💡 Context
If safe-update enforcement will eventually compare current manifest skills against a previously compiled manifest to detect newly introduced skills (as it already does for secrets), the omission of the field for zero-skill workflows is correct but worth noting in the struct godoc. Something like:
// Skills records the sorted, deduplicated skill specs from workflow frontmatter.
// Absent (nil) when the workflow has no skills declaration.
Skills []string `json:"skills,omitempty"`This makes the nil-vs-empty distinction explicit for future maintainers.
@copilot please address this.
Impeccable Skills Review: Remove expression support from skills frontmatter; add skills to gh-aw-manifestThis is a clean, well-scoped change. Removing expression support from skill specs is the right call — dynamic expressions are not statically auditable and could allow bypasses of the pinning and supply-chain verification the manifest exists to enable. What works well:
One nit (see inline comment): Verified clean:
Overall: LGTM pending the log nit above.
|
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 100.4 AIC · ⌖ 6.72 AIC · ⊞ 4.9K
| safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d", | ||
| currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers)) | ||
| safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d, skills=%d", | ||
| currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers), len(skillSpecs)) |
There was a problem hiding this comment.
The "Manifest built" log fires before the dedup loop runs, so len(skillSpecs) here is the raw input count, not the final deduplicated count. If a caller passes duplicate skill specs the log will report a higher number than what actually ends up in the manifest, which could mislead debugging sessions.
Consider moving this log to after sortedSkills is computed (or renaming the placeholder to raw_skills):
safeUpdateManifestLog.Printf("Manifest built: version=%d, secrets=%d, actions=%d, containers=%d, skills=%d",
currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers), len(sortedSkills))@copilot please address this.
…r; fix manifest comments Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The Fixed in the latest commit by removing the Also tightened the |
🤖 PR Triage
Score breakdown: impact 38 + urgency 18 + quality 14 Security/audit-relevant change: removes dynamic expression support from skills frontmatter, improving determinism and auditability. Touches core compilation logic and multiple lock files. Active review in progress (9 comments). Ready for human sign-off.
|
Skills refs must be statically resolvable at compile time —
${{ inputs.skill_ref }}andowner/repo@${{ github.sha }}expressions cannot be audited, pinned, or recorded deterministically. This removes expression support entirely and records the resulting static skill list in the manifest.Validation changes (
skills_frontmatter.go)skillSpecExpressionRefRegexpandgithubActionsExpressionRegexpvalidateSkillSpecValuenow only acceptsowner/repo@<40-hex-char-sha>orowner/repo/skill/path@<40-hex-char-sha>${{ inputs.skill_ref }}→ valid; After: → compile errorManifest (
safe_update_manifest.go,compiler_yaml.go)Skills []string(sorted, deduplicated,omitempty) toGHAWManifestNewGHAWManifestgains askillSpecs []stringparameter; compiler passesdata.Skills# gh-aw-manifest:lock-file header alongside secrets, actions, and containersTests
skills_frontmatter_test.go: flip "accepts github actions expressions" → "rejects github actions expressions"; remove expression cases fromTestIsRepositorySkillSpecactivation_skills_step_test.go: removeTestBuildActivationJob_AddsExpressionSkillInstallStepscompiler_orchestrator_frontmatter_test.go:TestParseFrontmatterSection_ExpressionSkillsRefnow asserts a compile-time errorsafe_update_manifest_test.go: add skills sorting/dedup coverage; update allNewGHAWManifestcall sites