-
Notifications
You must be signed in to change notification settings - Fork 52
Setup CodeActions and add quickfix for missing inputs #254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c66abfd to
b6bd0ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces CodeAction capabilities to the GitHub Actions language server, specifically implementing a quickfix feature for automatically adding missing required action inputs. The implementation includes:
- A new code action infrastructure with a provider-based architecture
- A quickfix provider that generates edits to add missing required inputs
- Test infrastructure using golden files to validate code action behavior
- Enhanced diagnostic data to support code actions
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
languageservice/src/code-actions/index.ts |
Main entry point for code actions, implementing provider aggregation and filtering logic |
languageservice/src/code-actions/types.ts |
Type definitions for code action context and provider interface |
languageservice/src/code-actions/quickfix/index.ts |
Exports quickfix providers array |
languageservice/src/code-actions/quickfix/add-missing-inputs.ts |
Implementation of the quickfix provider for adding missing required inputs |
languageservice/src/code-actions/tests/runner.ts |
Test infrastructure for running golden file tests |
languageservice/src/code-actions/tests/runner.test.ts |
Test suite using the golden file runner |
languageservice/src/code-actions/tests/testdata/quickfix/*.yml |
Test input files with markers for expected diagnostics and fixes |
languageservice/src/code-actions/tests/testdata/quickfix/*.golden.yml |
Expected output files after applying code actions |
languageservice/src/validate-action.ts |
Enhanced validation to include diagnostic data needed for code actions |
languageservice/src/validate.actions.test.ts |
Updated tests to verify new diagnostic data fields |
languageservice/src/index.ts |
Exports the new getCodeActions function |
languageserver/src/connection.ts |
Registers code action handler and declares CodeActionKind.QuickFix capability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
1b0264b to
f35d847
Compare
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if (data.hasWithKey && data.withIndent !== undefined) { | ||
| // `with:` exists - use its indentation + 2 for inputs | ||
| const inputIndent = " ".repeat(data.withIndent + 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of assuming 2, have you considered matching the customer's indent? You could detect this based on the indent amount of the first job. For example:
function detectIndentSize(rootToken: MappingToken): number {
const jobsValue = rootToken.find("jobs");
if (jobsValue && isMapping(jobsValue) && jobsValue.count > 0) {
const firstJob = jobsValue.get(0);
if (firstJob?.key.range && jobsValue.range) {
return firstJob.key.range.start.column - jobsValue.range.start.column;
}
}
return 2; // fallback
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you ❤️ I wonder if indent size is something we wanna carry around with WorkflowTemplate itself, I suspect it'll keep coming in use 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the WorkflowContext object might be more appropriate 🤷 (still kind of doesnt fit)
Co-authored-by: Salman Chishti <salmanmkc@GitHub.com>
- Add indentSize to MissingInputsDiagnosticData interface - Pass indentSize parameter from validate.ts to validateActionReference - Detect indentSize from workflow structure (jobs key to first child) - Fall back to detecting from with: block children when available
a1bd7e8 to
fa9be15
Compare
| } | ||
| } | ||
| }, | ||
| "actions/setup-node@v3": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the latest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just static actions that are part of test data (also added in this PR):
| const metadata: Record<string, ActionMetadata> = { | |
| "actions/cache@v1": { | |
| name: "Cache", | |
| description: "Cache dependencies", | |
| inputs: { | |
| path: { | |
| description: "A list of files to cache", | |
| required: true | |
| }, | |
| key: { | |
| description: "Cache key", | |
| required: true | |
| }, | |
| "restore-keys": { | |
| description: "Restore keys", | |
| required: false | |
| } | |
| } | |
| }, | |
| "actions/setup-node@v3": { | |
| name: "Setup Node", | |
| description: "Setup Node.js", | |
| inputs: { | |
| "node-version": { | |
| description: "Node version", | |
| required: true, | |
| default: "16" | |
| } | |
| } | |
| } |
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/cache@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also use latest versions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just static actions that are part of test data (also added in this PR):
| const metadata: Record<string, ActionMetadata> = { | |
| "actions/cache@v1": { | |
| name: "Cache", | |
| description: "Cache dependencies", | |
| inputs: { | |
| path: { | |
| description: "A list of files to cache", | |
| required: true | |
| }, | |
| key: { | |
| description: "Cache key", | |
| required: true | |
| }, | |
| "restore-keys": { | |
| description: "Restore keys", | |
| required: false | |
| } | |
| } | |
| }, | |
| "actions/setup-node@v3": { | |
| name: "Setup Node", | |
| description: "Setup Node.js", | |
| inputs: { | |
| "node-version": { | |
| description: "Node version", | |
| required: true, | |
| default: "16" | |
| } | |
| } | |
| } |
| @@ -0,0 +1,53 @@ | |||
| import {FeatureFlags} from "@actions/expressions"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subdirectory index.ts files break repo pattern
The existing repo pattern imports directly from specific files in the main index.ts:
export {complete} from "./complete.js";
export {hover} from "./hover.js";
export {validate, ValidationConfig, ActionsMetadataProvider} from "./validate.js";
export {ValueProviderConfig, ValueProviderKind} from "./value-providers/config.js"; // Direct file!No subdirectory has its own index.ts - the main package-level index.ts imports directly from the implementation files (e.g., value-providers/config.js, not value-providers/index.js).
This PR introduces:
code-actions/index.ts(new subdirectory index with implementation code)code-actions/quickfix/index.ts(another subdirectory index)- Main
index.tsimports from"./code-actions/index.js"instead of a specific file
Questions:
- Should subdirectory
index.tsfiles contain implementation, or be exports-only like the mainindex.ts?
Recommendation: Follow existing pattern - use meaningful file names instead of index.ts:
code-actions/index.ts→code-actions/code-actions.tscode-actions/quickfix/index.ts→code-actions/quickfix/quickfix-providers.ts
Then update the main index.ts:
export {getCodeActions, CodeActionParams} from "./code-actions/code-actions.js";This avoids having many files named index.ts with implementation code, making the codebase easier to navigate.
This is a minor consistency issue, not a blocker.
| missingRequiredInputs.length === 1 | ||
| ? `Missing required input \`${missingRequiredInputs[0][0]}\`` | ||
| : `Missing required inputs: ${missingRequiredInputs.map(input => `\`${input[0]}\``).join(", ")}`; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Position calculation should live in the quickfix, not validation
The pattern issue: Insert position and indentation are calculated in validate-action-reference.ts but consumed in add-missing-inputs.ts. This differs from how complete.ts works - it calls guessIndentation() at completion time rather than pre-computing during an earlier phase.
Why it matters: With the current approach, every new quickfix requires adding position logic to validation. "Fix deprecated action version"? Add position logic to validation. "Add missing runs-on"? More position logic. Validation accumulates code unrelated to detecting problems.
Specific issues in the current implementation:
| Issue | Code | Problem |
|---|---|---|
| Dangerous fallback | insertPosition = {line: 0, character: 0} |
Would insert at top of file on malformed YAML |
| Non-obvious offset | withToken.range.end.line - 1 |
Comment says "after" but code inserts "before" |
| Scattered state | stepIndent, withIndent, indentSize, insertPosition |
Four related values computed in validation, consumed in quickfix |
Recommendation: Diagnostic carries only: missing inputs, action reference, step token range. Quickfix computes insertion point when generating the fix. Shared position utilities can emerge in the code-actions layer.
- Rename index.ts files to follow repo patterns: - code-actions/index.ts → code-actions/code-actions.ts - code-actions/quickfix/index.ts → quickfix/quickfix-providers.ts - Move position calculation from validation to quickfix: - MissingInputsDiagnosticData now passes raw token ranges - Quickfix computes insertion position and indentation at code action time - detectIndentSize moved from validate.ts to validate-action-reference.ts
cf63c84 to
1116f79
Compare
This PR adds CodeAction capabilities to the language server and sets up the inner loop for them, using workflow files where to define expected diagnostics and golden files to specify expected results. This also adds a
CodeActionKind.QuickFixfor when an action is used but it's missing required inputs: