fix: unify on.permissions schema with shared github_actions_permissions ref#40064
Conversation
…ns ref - Replace inline on.permissions schema (which lacked vulnerability-alerts and other scopes) with $ref to shared #/$defs/github_actions_permissions - Add /on/permissions to knownFieldScopes, knownFieldValidValues, and knownFieldDocs in schema_errors.go for better error hints - Add tests for vulnerability-alerts in on.permissions (schema + extraction) - Fix pre-existing testifylint warning in importinpututil/spec_test.go Fixes #40063 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40064 does not have the 'implementation' label (has_implementation_label=false) and has only 76 new lines of code in business logic directories (≤100 threshold). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Unifies the on.permissions JSON schema with the shared github_actions_permissions definition to accept additional valid GitHub Actions scopes (notably vulnerability-alerts) and aligns schema-error hinting for on.permissions with existing top-level permissions behavior.
Changes:
- Replaced the inline
on.permissionsschema with a shared$defs/github_actions_permissionsreference and added avulnerability-alerts: readexample. - Extended schema error hint metadata to include
/on/permissions(valid values, scopes, docs link). - Added regression tests for
vulnerability-alertsinon.permissionsand extraction coverage inextractOnPermissions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/on_steps_test.go | Adds extractOnPermissions test coverage for vulnerability-alerts. |
| pkg/parser/schemas/main_workflow_schema.json | Switches on.permissions to use shared permissions definition and adds examples. |
| pkg/parser/schema_test.go | Adds schema regression tests to accept vulnerability-alerts and reject unknown scopes in on.permissions. |
| pkg/parser/schema_errors.go | Enables “Did you mean?” suggestions and docs links for /on/permissions unknown-scope errors. |
| pkg/importinpututil/spec_test.go | Minor assertion style update using assert.Empty. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| "permissions": { | ||
| "description": "Additional permissions for the pre-activation job. Use to declare extra scopes required by on.steps (e.g., issues: read for GitHub API calls in steps).", | ||
| "oneOf": [ | ||
| { | ||
| "type": "object", | ||
| "description": "Map of permission scope to level", | ||
| "properties": { | ||
| "actions": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "checks": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "contents": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "deployments": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "discussions": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "issues": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "packages": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "pages": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "pull-requests": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "repository-projects": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "security-events": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| }, | ||
| "statuses": { | ||
| "type": "string", | ||
| "enum": ["read", "write", "none"] | ||
| } | ||
| }, | ||
| "additionalProperties": false | ||
| } | ||
| ], | ||
| "$ref": "#/$defs/github_actions_permissions", | ||
| "examples": [ |
| // Both /permissions and /on/permissions mirror #/$defs/github_actions_permissions in | ||
| // main_workflow_schema.json. Update this list when the schema changes. | ||
| var knownFieldValidValues = map[string]string{ | ||
| // This list mirrors permissions.oneOf[1].properties in main_workflow_schema.json. | ||
| // Both entries mirror $defs/github_actions_permissions in main_workflow_schema.json. | ||
| // Update both when the schema changes. | ||
| "/permissions": "Valid permission scopes: actions, all, attestations, checks, copilot-requests, contents, deployments, discussions, id-token, issues, metadata, models, organization-projects, packages, pages, pull-requests, repository-projects, security-events, statuses, vulnerability-alerts", | ||
| "/permissions": "Valid permission scopes: actions, all, attestations, checks, copilot-requests, contents, deployments, discussions, id-token, issues, metadata, models, organization-projects, packages, pages, pull-requests, repository-projects, security-events, statuses, vulnerability-alerts", | ||
| "/on/permissions": "Valid permission scopes: actions, all, attestations, checks, copilot-requests, contents, deployments, discussions, id-token, issues, metadata, models, organization-projects, packages, pages, pull-requests, repository-projects, security-events, statuses, vulnerability-alerts", | ||
| } |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (3 tests analyzed)
Go: 3 ( Verdict
References:
|
![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 /improve-codebase-architecture — approving with non-blocking improvement suggestions.
📋 Key Themes & Highlights
Key Themes
- Rejection test is under-asserted (
schema_test.go): confirmserr != nilbut never exercises theknownFieldScopes["/on/permissions"]hint path added inschema_errors.go. A broken path mapping would be silent. - Duplicate scope lists (
schema_errors.go):/permissionsand/on/permissionsnow share the same$refin JSON Schema but still carry identical 20-element slices in Go. Should share a singlegithubActionsPermissionScopesvariable. - Acceptance coverage is scope-narrow: only
vulnerability-alertsis tested; the other 6 previously-blocked scopes (attestations,copilot-requests,id-token,metadata,models,organization-projects) are not regression-pinned.
Positive Highlights
- ✅ Core fix is surgical and correct — 57 lines of stale inline schema replaced by one
$ref - ✅ Both acceptance and rejection regression tests are present
- ✅ Independent coverage at the workflow-parse layer via
TestExtractOnPermissionstable case - ✅
schema_errors.gohint parity was correctly identified and added alongside the schema fix - ✅ PR description is clear, names the issue (
#40063), and lists all changed components
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| "engine": "copilot", | ||
| } | ||
| err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/on-permissions-unknown-scope-test.md") | ||
| if err == nil { |
There was a problem hiding this comment.
[/tdd] The rejection test confirms that err != nil, but does not verify that the error message actually contains the "Did you mean?" hints or docs URL added in schema_errors.go for /on/permissions.
If path detection is broken — e.g., the JSON schema path surfaced for an on.permissions violation is not exactly /on/permissions — the new knownFieldScopes and knownFieldDocs entries would be dead code, yet this test would still pass.
💡 Suggested assertion
Consider asserting the error message text, for example:
if err == nil {
t.Error("unknown scope in on.permissions should be rejected")
}
if !strings.Contains(err.Error(), "vulnerability-alerts") {
t.Errorf("expected \"Did you mean?\" hint with valid scopes in error, got: %v", err)
}Or at minimum use assert.ErrorContains(t, err, "Valid permission scopes") to confirm the hint path fires.
| "organization-projects", "packages", "pages", "pull-requests", | ||
| "repository-projects", "security-events", "statuses", "vulnerability-alerts", | ||
| }, | ||
| "/on/permissions": { |
There was a problem hiding this comment.
[/improve-codebase-architecture] The scope slice for /on/permissions is byte-for-byte identical to the /permissions slice, and both comments now say they mirror the same $ref. When a new scope is added to github_actions_permissions, there are four co-ordinated edits needed: two entries in knownFieldValidValues and two in knownFieldScopes.
Since the schema consolidation is the whole point of this PR, this is a good moment to consolidate the Go side too.
💡 Suggested refactor
// githubActionsPermissionScopes is the canonical scope list mirroring
// $defs/github_actions_permissions in main_workflow_schema.json.
// Update here when the schema changes.
var githubActionsPermissionScopes = []string{
"actions", "all", "attestations", "checks", "copilot-requests", "contents", "deployments",
"discussions", "id-token", "issues", "metadata", "models",
"organization-projects", "packages", "pages", "pull-requests",
"repository-projects", "security-events", "statuses", "vulnerability-alerts",
}
var knownFieldScopes = map[string][]string{
"/permissions": githubActionsPermissionScopes,
"/on/permissions": githubActionsPermissionScopes,
}Similarly, the long string in knownFieldValidValues could be built from this slice with strings.Join.
| "schedule": []any{map[string]any{"cron": "0 0 * * *"}}, | ||
| "workflow_dispatch": nil, | ||
| "permissions": map[string]any{ | ||
| "vulnerability-alerts": "read", |
There was a problem hiding this comment.
[/tdd] The acceptance test pins vulnerability-alerts (the scope from the bug report), which is the right regression anchor. However, attestations, copilot-requests, id-token, metadata, models, and organization-projects were also blocked by the old inline schema and are now valid through the shared $ref — none of them have acceptance coverage.
Adding one or two to the acceptance test (e.g., attestations: write) would turn this into a broader schema-drift guard rather than a single-scope pin.
💡 Suggested addition
// covers a second scope that was previously blocked by the inline schema
"attestations": "write",Or use a short table-driven loop over []string{"vulnerability-alerts", "attestations", "copilot-requests"} inside the test to keep it compact.
on.permissionsused a hardcoded inline schema with 12 scopes andadditionalProperties: false, omittingvulnerability-alerts(andattestations,copilot-requests,id-token,metadata,models,organization-projects). This causedgh aw compileto reject valid Dependabot pre-activation gates like:The top-level
permissions:block already used$ref: #/$defs/github_actions_permissions, making the two schemas inconsistent.Changes
main_workflow_schema.json— Replaceon.permissions.oneOf[0]inline object with$ref: #/$defs/github_actions_permissions, unifying both permission blocks on the same shared definition. Adds avulnerability-alerts: readexample.schema_errors.go— Add/on/permissionstoknownFieldValidValues,knownFieldScopes, andknownFieldDocsso typos inon.permissionskeys get "Did you mean?" suggestions and docs links, matching the behaviour already present for top-level/permissions.Tests — Schema acceptance/rejection regression tests for
vulnerability-alertsinon.permissions; newextractOnPermissionstable case for the same scope.