Conversation
Add 'workflows' as a valid permission scope in the permissions-mapping schema. The workflows permission supports only 'write' (or none), matching the GitHub App permissions model where workflows: write allows updating GitHub Actions workflow files. This enables autocomplete and validation for 'permissions: workflows:' in workflow YAML files.
There was a problem hiding this comment.
Pull request overview
Adds the workflows permission scope to the workflow schema so the language service can autocomplete and validate permissions: workflows: write in GitHub Actions workflow YAML.
Changes:
- Extend
permissions-mappingin the workflow schema to includeworkflowswithpermission-level-write-or-no-access. - Add completion tests ensuring
workflowsis suggested at both top-level and job-level permissions. - Add completion test coverage ensuring
workflowsvalues do not includeread.
Show a summary per file
| File | Description |
|---|---|
workflow-parser/src/workflow-v1.0.json |
Adds workflows to the permissions mapping schema using permission-level-write-or-no-access. |
languageservice/src/complete.test.ts |
Adds completion tests verifying workflows appears in permissions key completion and that value completion doesn’t offer read. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| expect(result).not.toBeUndefined(); | ||
| const labels = result.map(x => x.label); | ||
| expect(labels).toContain("write"); | ||
| expect(labels).not.toContain("read"); |
There was a problem hiding this comment.
The test name says it "offers only write and none" for workflows, but the assertions only check for write and absence of read. Add an assertion that none is offered (and consider asserting no other values are present) so the test matches its intent and guards the permission-level-write-or-no-access behavior.
| expect(labels).not.toContain("read"); | |
| expect(labels).toContain("none"); | |
| expect([...labels].sort()).toEqual(["none", "write"]); |
| describe("permissions workflows completion", () => { | ||
| it("includes workflows in top-level permissions", async () => { | ||
| const input = `on: push | ||
| permissions: | ||
| |`; | ||
| const result = await complete(...getPositionFromCursor(input)); | ||
|
|
||
| expect(result).not.toBeUndefined(); | ||
| const labels = result.map(x => x.label); | ||
| expect(labels).toContain("workflows"); | ||
| }); | ||
|
|
||
| it("offers only write and none for workflows", async () => { | ||
| const input = `on: push | ||
| permissions: | ||
| workflows: |`; | ||
| const result = await complete(...getPositionFromCursor(input)); | ||
|
|
||
| expect(result).not.toBeUndefined(); | ||
| const labels = result.map(x => x.label); | ||
| expect(labels).toContain("write"); | ||
| expect(labels).not.toContain("read"); | ||
| }); | ||
|
|
||
| it("includes workflows in job-level permissions", async () => { | ||
| const input = `on: push | ||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| |`; | ||
| const result = await complete(...getPositionFromCursor(input)); | ||
|
|
||
| expect(result).not.toBeUndefined(); | ||
| const labels = result.map(x => x.label); | ||
| expect(labels).toContain("workflows"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This PR states it enables both autocomplete and validation for permissions: workflows: write, but the added tests only cover completion. Consider adding a validation test that workflows: write is accepted and workflows: read is rejected to ensure the schema change is exercised through the validation pipeline too.
Uh oh!
There was an error while loading. Please reload this page.