diff --git a/expressions/src/features.test.ts b/expressions/src/features.test.ts index eb69099..6694141 100644 --- a/expressions/src/features.test.ts +++ b/expressions/src/features.test.ts @@ -51,7 +51,7 @@ describe("FeatureFlags", () => { it("returns all features when all is enabled", () => { const flags = new FeatureFlags({all: true}); - expect(flags.getEnabledFeatures()).toEqual(["missingInputsQuickfix"]); + expect(flags.getEnabledFeatures()).toEqual(["missingInputsQuickfix", "blockScalarChompingWarning"]); }); }); }); diff --git a/expressions/src/features.ts b/expressions/src/features.ts index ab1b7fd..b0a0770 100644 --- a/expressions/src/features.ts +++ b/expressions/src/features.ts @@ -21,6 +21,13 @@ export interface ExperimentalFeatures { * @default false */ missingInputsQuickfix?: boolean; + + /** + * Warn when block scalars (| or >) use implicit clip chomping, + * which adds a trailing newline that may be unintentional. + * @default false + */ + blockScalarChompingWarning?: boolean; } /** @@ -32,7 +39,7 @@ export type ExperimentalFeatureKey = Exclude; * All known experimental feature keys. * This list must be kept in sync with the ExperimentalFeatures interface. */ -const allFeatureKeys: ExperimentalFeatureKey[] = ["missingInputsQuickfix"]; +const allFeatureKeys: ExperimentalFeatureKey[] = ["missingInputsQuickfix", "blockScalarChompingWarning"]; export class FeatureFlags { private readonly features: ExperimentalFeatures; diff --git a/languageserver/README.md b/languageserver/README.md index e8633e3..c31614b 100644 --- a/languageserver/README.md +++ b/languageserver/README.md @@ -126,6 +126,7 @@ initializationOptions: { | Feature | Description | |---------|-------------| | `missingInputsQuickfix` | Code action to add missing required inputs for actions | +| `blockScalarChompingWarning` | Warn when block scalars (`\|` or `>`) use implicit clip chomping, which adds a trailing newline that may be unintentional | Individual feature flags take precedence over `all`. For example, `{ all: true, missingInputsQuickfix: false }` enables all experimental features except `missingInputsQuickfix`. diff --git a/languageserver/src/connection.ts b/languageserver/src/connection.ts index d0a7ede..32e6ce8 100644 --- a/languageserver/src/connection.ts +++ b/languageserver/src/connection.ts @@ -123,7 +123,8 @@ export function initConnection(connection: Connection) { actionsMetadataProvider: getActionsMetadataProvider(client, cache), fileProvider: getFileProvider(client, cache, repoContext?.workspaceUri, async path => { return await connection.sendRequest(Requests.ReadFile, {path} satisfies ReadFileRequest); - }) + }), + featureFlags }; const result = await validate(textDocument, config); diff --git a/languageservice/src/validate-format-string.ts b/languageservice/src/validate-format-string.ts new file mode 100644 index 0000000..eaeaa23 --- /dev/null +++ b/languageservice/src/validate-format-string.ts @@ -0,0 +1,199 @@ +/** + * Format string validation for format() function calls. + * Port of Go's format_validator.go from actions-workflow-parser. + */ + +import {Expr, FunctionCall, Literal, Binary, Unary, Logical, Grouping, IndexAccess} from "@actions/expressions/ast"; +import {Kind} from "@actions/expressions/data/expressiondata"; + +/** + * Error types for format string validation + */ +export type FormatStringError = + | {type: "invalid-syntax"; message: string} + | {type: "arg-count-mismatch"; expected: number; provided: number}; + +/** + * Validates a format string and returns the maximum placeholder index. + * Port of Go's validateFormatString from format_validator.go. + * + * @param formatString The format string to validate + * @returns { valid: boolean, maxArgIndex: number } where maxArgIndex is -1 if no placeholders + */ +export function validateFormatString(formatString: string): {valid: boolean; maxArgIndex: number} { + let maxIndex = -1; + let i = 0; + + while (i < formatString.length) { + // Find next left brace + let lbrace = -1; + for (let j = i; j < formatString.length; j++) { + if (formatString[j] === "{") { + lbrace = j; + break; + } + } + + // Find next right brace + let rbrace = -1; + for (let j = i; j < formatString.length; j++) { + if (formatString[j] === "}") { + rbrace = j; + break; + } + } + + // No more braces + if (lbrace < 0 && rbrace < 0) { + break; + } + + // Left brace comes first (or only left brace exists) + if (lbrace >= 0 && (rbrace < 0 || lbrace < rbrace)) { + // Check if it's escaped + if (lbrace + 1 < formatString.length && formatString[lbrace + 1] === "{") { + // Escaped left brace + i = lbrace + 2; + continue; + } + + // This is a placeholder opening - find the closing brace + rbrace = -1; + for (let j = lbrace + 1; j < formatString.length; j++) { + if (formatString[j] === "}") { + rbrace = j; + break; + } + } + + if (rbrace < 0) { + // Missing closing brace + return {valid: false, maxArgIndex: -1}; + } + + // Validate placeholder content (must be digits only) + if (rbrace === lbrace + 1) { + // Empty placeholder {} + return {valid: false, maxArgIndex: -1}; + } + + // Parse the index and validate it's all digits + let index = 0; + for (let j = lbrace + 1; j < rbrace; j++) { + const c = formatString[j]; + if (c < "0" || c > "9") { + // Non-numeric character + return {valid: false, maxArgIndex: -1}; + } + index = index * 10 + (c.charCodeAt(0) - "0".charCodeAt(0)); + } + + if (index > maxIndex) { + maxIndex = index; + } + + i = rbrace + 1; + continue; + } + + // Right brace comes first (or only right brace exists) + // Check if it's escaped + if (rbrace + 1 < formatString.length && formatString[rbrace + 1] === "}") { + // Escaped right brace + i = rbrace + 2; + continue; + } + + // Unescaped right brace outside of placeholder + return {valid: false, maxArgIndex: -1}; + } + + return {valid: true, maxArgIndex: maxIndex}; +} + +/** + * Walks an expression AST to find and validate all format() function calls. + * + * @param expr The expression AST to validate + * @returns Array of validation errors found + */ +export function validateFormatCalls(expr: Expr): FormatStringError[] { + const errors: FormatStringError[] = []; + const stack: Expr[] = [expr]; + + while (stack.length > 0) { + const node = stack.pop(); + if (!node) { + continue; + } + + if (node instanceof FunctionCall) { + if (node.functionName.lexeme.toLowerCase() === "format") { + const error = validateSingleFormatCall(node); + if (error) { + errors.push(error); + } + } + // Push args for further processing (to find nested format calls) + for (const arg of node.args) { + stack.push(arg); + } + } else if (node instanceof Binary) { + stack.push(node.left, node.right); + } else if (node instanceof Unary) { + stack.push(node.expr); + } else if (node instanceof Logical) { + for (const arg of node.args) { + stack.push(arg); + } + } else if (node instanceof Grouping) { + stack.push(node.group); + } else if (node instanceof IndexAccess) { + stack.push(node.expr, node.index); + } + // Literal, ContextAccess - no children to process + } + + return errors; +} + +/** + * Validates a single format() function call. + * + * @param fc The FunctionCall AST node + * @returns Validation error if found, undefined if valid + */ +function validateSingleFormatCall(fc: FunctionCall): FormatStringError | undefined { + // Must have at least one argument (the format string) + if (fc.args.length < 1) { + return undefined; + } + + // First argument must be a string literal + const firstArg = fc.args[0]; + if (!(firstArg instanceof Literal) || firstArg.literal.kind !== Kind.String) { + return undefined; // Can't validate dynamic format strings + } + + const formatString = firstArg.literal.coerceString(); + const numArgs = fc.args.length - 1; // Subtract 1 for format string itself + + const {valid, maxArgIndex} = validateFormatString(formatString); + + if (!valid) { + return { + type: "invalid-syntax", + message: "Format string has invalid syntax (missing closing brace, unescaped braces, or invalid placeholder)" + }; + } + + if (maxArgIndex >= numArgs) { + return { + type: "arg-count-mismatch", + expected: maxArgIndex + 1, // Convert 0-based index to count + provided: numArgs + }; + } + + return undefined; +} diff --git a/languageservice/src/validate.block-scalar-warning.test.ts b/languageservice/src/validate.block-scalar-warning.test.ts new file mode 100644 index 0000000..8bde8e9 --- /dev/null +++ b/languageservice/src/validate.block-scalar-warning.test.ts @@ -0,0 +1,835 @@ +import {FeatureFlags} from "@actions/expressions"; +import {DiagnosticSeverity} from "vscode-languageserver-types"; +import {registerLogger} from "./log.js"; +import {createDocument} from "./test-utils/document.js"; +import {TestLogger} from "./test-utils/logger.js"; +import {clearCache} from "./utils/workflow-cache.js"; +import {validate, ValidationConfig} from "./validate.js"; + +registerLogger(new TestLogger()); + +const configWithFlag: ValidationConfig = { + featureFlags: new FeatureFlags({blockScalarChompingWarning: true}) +}; + +beforeEach(() => { + clearCache(); +}); + +describe("block scalar chomping - warning cases", () => { + describe("step-level env values", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: | + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("does not warn with keep chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: |+ + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn with strip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: |- + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("uses > indicator in warning message for folded scalars", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: > + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '>' implicitly adds a trailing newline that may be unintentional. Use '>-' to remove it, or '>+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for plain string env value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: | + hello world +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("job-level env values", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + env: + MY_VAR: | + some value + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("workflow-level env values", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +env: + GLOBAL_VAR: | + some value +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("container env values", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + container: + image: node:18 + env: + CONTAINER_VAR: | + some value + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("service container env values", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + services: + redis: + image: redis + env: + REDIS_PASSWORD: | + secret123 + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("action input (with)", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + script: | + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("does not warn with keep chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + script: |+ + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn with strip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + script: |- + \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + }); + + describe("reusable workflow inputs (with)", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + call-workflow: + uses: ./.github/workflows/reusable.yml + with: + my-input: | + some value +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("reusable workflow secrets", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + call-workflow: + uses: ./.github/workflows/reusable.yml + secrets: + my-secret: | + \${{ secrets.TOKEN }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("job outputs", () => { + it("warns with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + outputs: + my_output: | + \${{ steps.test.outputs.value }} + steps: + - id: test + run: echo "value=test" >> $GITHUB_OUTPUT +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("does not warn with strip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + outputs: + my_output: |- + \${{ steps.test.outputs.value }} + steps: + - id: test + run: echo "value=test" >> $GITHUB_OUTPUT +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + }); + + describe("matrix values", () => { + it("warns for matrix vector value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + config: + - | + value1 + - value2 + steps: + - run: echo \${{ matrix.config }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("does not warn with strip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + config: + - |- + value1 + - value2 + steps: + - run: echo \${{ matrix.config }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("warns for matrix include value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest] + include: + - os: | + windows-latest + special: true + steps: + - run: echo \${{ matrix.os }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for matrix exclude value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + node: [16, 18] + exclude: + - os: | + windows-latest + node: 16 + steps: + - run: echo \${{ matrix.os }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for deeply nested matrix value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + config: + - foo: + bar: | + baz + steps: + - run: echo \${{ matrix.config }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for deeply nested matrix include value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest] + include: + - os: ubuntu-latest + config: + nested: | + value + steps: + - run: echo \${{ matrix.config }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for deeply nested matrix exclude value with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu-latest, windows-latest] + exclude: + - os: windows-latest + config: + nested: | + value + steps: + - run: echo \${{ matrix.os }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); + + describe("concurrency", () => { + it("warns for concurrency string with clip chomping", async () => { + const input = ` +on: push +concurrency: | + my-group-\${{ github.ref }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("does not warn for concurrency with strip chomping", async () => { + const input = ` +on: push +concurrency: |- + my-group-\${{ github.ref }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("warns for concurrency.group with clip chomping", async () => { + const input = ` +on: push +concurrency: + group: | + my-group-\${{ github.ref }} + cancel-in-progress: true +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + + it("warns for job-level concurrency with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + concurrency: | + job-group-\${{ github.ref }} + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result).toContainEqual( + expect.objectContaining({ + message: + "Block scalar '|' implicitly adds a trailing newline that may be unintentional. Use '|-' to remove it, or '|+' to explicitly keep it.", + code: "block-scalar-chomping", + severity: DiagnosticSeverity.Warning + }) + ); + }); + }); +}); + +describe("block scalar chomping - no warning cases", () => { + describe("fields trimmed server-side", () => { + it("does not warn for job-if with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + if: | + github.ref == 'refs/heads/main' + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for step-if with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo done + if: | + github.ref == 'refs/heads/main' +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for runs-on with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: | + ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for job name with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + name: | + My Job + runs-on: ubuntu-latest + steps: + - run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for step name with clip chomping", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - name: | + My Step + run: echo done +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + }); + + describe("run field (intentionally allowed)", () => { + it("does not warn for step run field", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: | + echo hello + echo world +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for run field with expression", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: | + echo \${{ github.ref }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + }); + + describe("non-block scalars", () => { + it("does not warn for quoted strings", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: "hello world" +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for flow scalars", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: hello world +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + + it("does not warn for inline expressions", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo $VAR + env: + VAR: \${{ matrix.value }} +`; + const result = await validate(createDocument("wf.yaml", input), configWithFlag); + + expect(result.filter(d => d.code === "block-scalar-chomping")).toEqual([]); + }); + }); +}); diff --git a/languageservice/src/validate.format-string.test.ts b/languageservice/src/validate.format-string.test.ts new file mode 100644 index 0000000..ce5a6bd --- /dev/null +++ b/languageservice/src/validate.format-string.test.ts @@ -0,0 +1,273 @@ +import {DiagnosticSeverity} from "vscode-languageserver-types"; +import {createDocument} from "./test-utils/document.js"; +import {validate} from "./validate.js"; +import {clearCache} from "./utils/workflow-cache.js"; +import {validateFormatString} from "./validate-format-string.js"; + +beforeEach(() => { + clearCache(); +}); + +describe("format string validation", () => { + describe("validateFormatString unit tests", () => { + it("returns valid for simple placeholder", () => { + const result = validateFormatString("{0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for multiple placeholders", () => { + const result = validateFormatString("{0} {1} {2}"); + expect(result).toEqual({valid: true, maxArgIndex: 2}); + }); + + it("returns valid for text with placeholder", () => { + const result = validateFormatString("hello {0} world"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for escaped left braces", () => { + const result = validateFormatString("{{0}} {0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for escaped right braces", () => { + const result = validateFormatString("{0}}}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for no placeholders", () => { + const result = validateFormatString("hello world"); + expect(result).toEqual({valid: true, maxArgIndex: -1}); + }); + + it("returns invalid for missing closing brace", () => { + const result = validateFormatString("{0"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for empty placeholder", () => { + const result = validateFormatString("{}"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for non-numeric placeholder", () => { + const result = validateFormatString("{abc}"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for unescaped closing brace", () => { + const result = validateFormatString("text } more"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("handles out-of-order placeholders", () => { + const result = validateFormatString("{2} {0} {1}"); + expect(result).toEqual({valid: true, maxArgIndex: 2}); + }); + + it("handles repeated placeholders", () => { + const result = validateFormatString("{0} {0} {0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + }); + + describe("InvalidFormatString workflow validation", () => { + it("errors on missing closing brace", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toContainEqual( + expect.objectContaining({ + code: "invalid-format-string", + severity: DiagnosticSeverity.Error + }) + ); + }); + + it("errors on empty braces", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toContainEqual( + expect.objectContaining({ + code: "invalid-format-string" + }) + ); + }); + + it("errors on non-numeric placeholder", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{abc}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toContainEqual( + expect.objectContaining({ + code: "invalid-format-string" + }) + ); + }); + + it("allows valid format strings", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0} {1}', github.event_name, github.ref) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "invalid-format-string" + }) + ); + }); + + it("allows escaped braces", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{{0}} {0}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "invalid-format-string" + }) + ); + }); + }); + + describe("FormatArgCountMismatch workflow validation", () => { + it("errors when placeholder exceeds arg count", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{2}', 'arg0', 'arg1') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch", + severity: DiagnosticSeverity.Error + }) + ); + }); + + it("errors when referencing arg 0 with no args", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0}') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch" + }) + ); + }); + + it("allows when arg count matches", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0} {1} {2}', 'a', 'b', 'c') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch" + }) + ); + }); + + it("handles no placeholders correctly", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('hello world') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch" + }) + ); + }); + + it("skips validation for dynamic format strings", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format(env.FORMAT_STRING, 'arg') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + // Should not have format errors since we can't validate dynamic strings + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "invalid-format-string" + }) + ); + expect(result).not.toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch" + }) + ); + }); + + it("validates nested format calls", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0}', format('{2}', 'a')) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + // The inner format call has an error + expect(result).toContainEqual( + expect.objectContaining({ + code: "format-arg-count-mismatch" + }) + ); + }); + }); +}); diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index e2ddc76..bd5b937 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -1,4 +1,4 @@ -import {Lexer, Parser, data} from "@actions/expressions"; +import {FeatureFlags, Lexer, Parser, data} from "@actions/expressions"; import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast"; import {TemplateParseResult, WorkflowTemplate, isBasicExpression, isMapping, isString} from "@actions/workflow-parser"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; @@ -27,6 +27,7 @@ import {mapRange} from "./utils/range.js"; import {getOrConvertWorkflowTemplate, getOrParseWorkflow} from "./utils/workflow-cache.js"; import {validateActionReference} from "./validate-action-reference.js"; import {validateAction} from "./validate-action.js"; +import {validateFormatCalls} from "./validate-format-string.js"; import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config.js"; import {defaultValueProviders} from "./value-providers/default.js"; @@ -38,6 +39,7 @@ export type ValidationConfig = { contextProviderConfig?: ContextProviderConfig; actionsMetadataProvider?: ActionsMetadataProvider; fileProvider?: FileProvider; + featureFlags?: FeatureFlags; }; export type ActionsMetadataProvider = { @@ -84,7 +86,7 @@ async function validateWorkflow(textDocument: TextDocument, config?: ValidationC }); // Validate expressions and value providers - await additionalValidations(diagnostics, textDocument.uri, template, result.value, config); + await additionalValidations(diagnostics, textDocument.uri, template, result.value, config, config?.featureFlags); } // For now map parser errors directly to diagnostics @@ -108,9 +110,10 @@ async function additionalValidations( documentUri: URI, template: WorkflowTemplate, root: TemplateToken, - config?: ValidationConfig + config?: ValidationConfig, + featureFlags?: FeatureFlags ) { - for (const [parent, token, key] of TemplateToken.traverse(root)) { + for (const [parent, token, key, ancestors] of TemplateToken.traverse(root)) { // If the token is a value in a pair, use the key definition for validation // If the token has a parent (map, sequence, etc), use this definition for validation const validationToken = key || parent || token; @@ -128,7 +131,12 @@ async function additionalValidations( ); } - // If this is a job-if, step-if, or snapshot-if field (which are strings that should be treated as expressions), validate it + // Validate block scalar chomping for expressions and strings + if (featureFlags?.isEnabled("blockScalarChompingWarning")) { + validateBlockScalarChomping(diagnostics, token, parent, key, ancestors); + } + + // `if` conditions allow omitting ${{ }}, so validate strings in these fields as expressions const definitionKey = token.definition?.key; if ( isString(token) && @@ -148,7 +156,9 @@ async function additionalValidations( finalCondition, token.definitionInfo, undefined, - token.source + token.source, + undefined, + token.blockScalarHeader ); await validateExpression( @@ -735,6 +745,28 @@ async function validateExpression( continue; } + // Validate format() function calls + const formatErrors = validateFormatCalls(expr); + for (const formatError of formatErrors) { + if (formatError.type === "invalid-syntax") { + diagnostics.push({ + message: `Invalid format string: ${formatError.message}`, + range: mapRange(expression.range), + severity: DiagnosticSeverity.Error, + code: "invalid-format-string" + }); + } else if (formatError.type === "arg-count-mismatch") { + diagnostics.push({ + message: `Format string references argument {${formatError.expected - 1}} but only ${ + formatError.provided + } argument(s) provided`, + range: mapRange(expression.range), + severity: DiagnosticSeverity.Error, + code: "format-arg-count-mismatch" + }); + } + } + const context = await getWorkflowExpressionContext( namedContexts, contextProviderConfig, @@ -822,3 +854,90 @@ function getStaticConcurrencyGroup(token: TemplateToken | undefined): StringToke return undefined; } + +/** + * Validates YAML block scalar chomping. + * + * Block scalars (| and >) implicitly add a trailing newline by default ("clip" chomping). + * This is often unintended by the workflow author and can cause unexpected behavior. + * This function warns on certain fields when clip chomping is used (implicit trailing newline) + * and suggests they explicitly use strip (|-) or keep (|+) to clarify intent. + * + * Only specific fields are validated - those where trailing newlines may cause + * issues but aren't automatically trimmed server-side. For example env, inputs, outputs, etc. + * + * Skipped fields: + * - run: Multi-line scripts commonly have trailing newlines + * - Fields trimmed server-side: name, uses, shell, if, etc. + */ +function validateBlockScalarChomping( + diagnostics: Diagnostic[], + token: TemplateToken, + parent: TemplateToken | undefined, + key: TemplateToken | undefined, + ancestors: TemplateToken[] +): void { + // Not an expression or string? + if (!isBasicExpression(token) && !isString(token)) { + return; + } + + // Not a block scalar? + const header = token.blockScalarHeader; + if (!header) { + return; + } + + // Not "clip" chomp style? + if (header.includes("+") || header.includes("-")) { + return; + } + + // Check if we should warn + let shouldWarn = false; + const parentDefinitionName = parent?.definition?.key; + const tokenDefinitionName = token.definition?.key; + const keyName = key && isString(key) ? key.value : undefined; + if ( + parentDefinitionName && + [ + "workflow-env", + "job-env", + "step-env", + "container-env", + "step-with", + "job-outputs", + "workflow-job-with", + "workflow-job-secrets" + ].includes(parentDefinitionName) + ) { + // env, with, outputs, or secrets fields + shouldWarn = true; + } else if ( + ancestors.some(ancestor => { + const ancestorKey = ancestor.definition?.key; + return ancestorKey === "matrix" || ancestorKey === "matrix-filter" || ancestorKey === "matrix-filter-item"; + }) + ) { + // Matrix values (vectors, include, exclude) + shouldWarn = true; + } else if (tokenDefinitionName && ["workflow-concurrency", "job-concurrency"].includes(tokenDefinitionName)) { + // Concurrency shorthand + shouldWarn = true; + } else if (keyName === "group" && parentDefinitionName === "concurrency-mapping") { + // Concurrency group field + shouldWarn = true; + } + + if (!shouldWarn) { + return; + } + + const blockIndicator = header.startsWith("|") ? "|" : ">"; + diagnostics.push({ + message: `Block scalar '${blockIndicator}' implicitly adds a trailing newline that may be unintentional. Use '${blockIndicator}-' to remove it, or '${blockIndicator}+' to explicitly keep it.`, + range: mapRange(token.range), + severity: DiagnosticSeverity.Warning, + code: "block-scalar-chomping" + }); +} diff --git a/workflow-parser/src/expressions.test.ts b/workflow-parser/src/expressions.test.ts index 1feca02..3077719 100644 --- a/workflow-parser/src/expressions.test.ts +++ b/workflow-parser/src/expressions.test.ts @@ -201,4 +201,355 @@ jobs: throw new Error("expected if to be a string (will be converted to expression later)"); } }); + + describe("Block scalar chomp style preservation", () => { + it("preserves clip chomping (|) for literal block scalar", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: | + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe("|"); + }); + + it("preserves strip chomping (|-) for literal block scalar", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: |- + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe("|-"); + }); + + it("preserves keep chomping (|+) for literal block scalar", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: |+ + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe("|+"); + }); + + it("preserves folded clip (>) chomping", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: > + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe(">"); + }); + + it("preserves folded strip (>-) chomping", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: >- + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe(">-"); + }); + + it("preserves with explicit indent (|2)", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: |2 + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe("|2"); + }); + + it("preserves with explicit indent and strip (|-2)", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: |-2 + \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBe("|-2"); + }); + + it("handles flow scalars (no chomp info for inline)", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: \${{ github.event_name }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + expect(testToken.blockScalarHeader).toBeUndefined(); + }); + + it("preserves block scalar info for format expressions with multiple sub-expressions", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + env: + TEST: | + Hello \${{ github.event_name }} World \${{ github.ref }} + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const env = build.get(1).value.assertMapping("env"); + const testToken = env.get(0).value; + + if (!isBasicExpression(testToken)) { + throw new Error("expected TEST to be a basic expression"); + } + + // The format expression should preserve the block scalar info + expect(testToken.blockScalarHeader).toBe("|"); + }); + + it("preserves block scalar info on StringToken for isExpression fields", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + if: | + github.event_name == 'push' + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const ifToken = build.get(1).value; + + // For isExpression fields without ${{ }}, the token is a StringToken + if (!isString(ifToken)) { + throw new Error("expected if to be a string"); + } + + expect(ifToken.blockScalarHeader).toBe("|"); + }); + + it("preserves block scalar info on StringToken for isExpression fields with strip", () => { + const result = parseWorkflow( + { + name: "test.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + if: |- + github.event_name == 'push' + steps: + - run: echo hi` + }, + nullTrace + ); + + expect(result.context.errors.getErrors()).toHaveLength(0); + + const workflowRoot = result.value!.assertMapping("root")!; + const jobs = workflowRoot.get(1).value.assertMapping("jobs"); + const build = jobs.get(0).value.assertMapping("job"); + const ifToken = build.get(1).value; + + if (!isString(ifToken)) { + throw new Error("expected if to be a string"); + } + + expect(ifToken.blockScalarHeader).toBe("|-"); + }); + }); }); diff --git a/workflow-parser/src/templates/template-reader.ts b/workflow-parser/src/templates/template-reader.ts index ff28fed..24a0676 100644 --- a/workflow-parser/src/templates/template-reader.ts +++ b/workflow-parser/src/templates/template-reader.ts @@ -613,7 +613,9 @@ class TemplateReader { `format('${format.join("")}'${args.join("")})`, definitionInfo, expressionTokens, - raw + raw, + undefined, + token.blockScalarHeader ); } @@ -695,7 +697,8 @@ class TemplateReader { definitionInfo, undefined, token.source, - expressionRange + expressionRange, + token.blockScalarHeader ), error: undefined }; diff --git a/workflow-parser/src/templates/tokens/basic-expression-token.ts b/workflow-parser/src/templates/tokens/basic-expression-token.ts index e264c93..0928d2d 100644 --- a/workflow-parser/src/templates/tokens/basic-expression-token.ts +++ b/workflow-parser/src/templates/tokens/basic-expression-token.ts @@ -24,7 +24,19 @@ export class BasicExpressionToken extends ExpressionToken { public readonly expressionRange: TokenRange | undefined; /** - * @param originalExpressions If the basic expression was transformed from individual expressions, these will be the original ones + * The block scalar header (e.g., "|", "|-", "|+", ">", ">-", ">+") if parsed from a YAML block scalar. + */ + public readonly blockScalarHeader: string | undefined; + + /** + * @param file The file ID where this token originated + * @param range The range of the entire expression including `${{` and `}}` + * @param expression The expression string without `${{` and `}}` markers + * @param definitionInfo Schema definition info for this token + * @param originalExpressions If transformed from individual expressions (e.g., format()), these are the originals + * @param source The original source string from the YAML + * @param expressionRange The range of just the expression, excluding `${{` and `}}` + * @param blockScalarHeader The block scalar header (e.g., "|", "|-") if parsed from a YAML block scalar */ public constructor( file: number | undefined, @@ -33,13 +45,15 @@ export class BasicExpressionToken extends ExpressionToken { definitionInfo: DefinitionInfo | undefined, originalExpressions: BasicExpressionToken[] | undefined, source: string | undefined, - expressionRange?: TokenRange | undefined + expressionRange?: TokenRange | undefined, + blockScalarHeader?: string | undefined ) { super(TokenType.BasicExpression, file, range, undefined, definitionInfo); this.expr = expression; this.source = source; this.originalExpressions = originalExpressions; this.expressionRange = expressionRange; + this.blockScalarHeader = blockScalarHeader; } public get expression(): string { @@ -55,7 +69,8 @@ export class BasicExpressionToken extends ExpressionToken { this.definitionInfo, this.originalExpressions, this.source, - this.expressionRange + this.expressionRange, + this.blockScalarHeader ) : new BasicExpressionToken( this.file, @@ -64,7 +79,8 @@ export class BasicExpressionToken extends ExpressionToken { this.definitionInfo, this.originalExpressions, this.source, - this.expressionRange + this.expressionRange, + this.blockScalarHeader ); } diff --git a/workflow-parser/src/templates/tokens/string-token.ts b/workflow-parser/src/templates/tokens/string-token.ts index 998ddab..5affac0 100644 --- a/workflow-parser/src/templates/tokens/string-token.ts +++ b/workflow-parser/src/templates/tokens/string-token.ts @@ -6,23 +6,26 @@ import {TokenType} from "./types.js"; export class StringToken extends LiteralToken { public readonly value: string; public readonly source: string | undefined; + public readonly blockScalarHeader: string | undefined; public constructor( file: number | undefined, range: TokenRange | undefined, value: string, definitionInfo: DefinitionInfo | undefined, - source?: string + source?: string, + blockScalarHeader?: string ) { super(TokenType.String, file, range, definitionInfo); this.value = value; this.source = source; + this.blockScalarHeader = blockScalarHeader; } public override clone(omitSource?: boolean): TemplateToken { return omitSource - ? new StringToken(undefined, undefined, this.value, this.definitionInfo, this.source) - : new StringToken(this.file, this.range, this.value, this.definitionInfo, this.source); + ? new StringToken(undefined, undefined, this.value, this.definitionInfo, this.source, this.blockScalarHeader) + : new StringToken(this.file, this.range, this.value, this.definitionInfo, this.source, this.blockScalarHeader); } public override toString(): string { diff --git a/workflow-parser/src/templates/tokens/template-token.test.ts b/workflow-parser/src/templates/tokens/template-token.test.ts index 79e3680..bba86c1 100644 --- a/workflow-parser/src/templates/tokens/template-token.test.ts +++ b/workflow-parser/src/templates/tokens/template-token.test.ts @@ -1,11 +1,13 @@ /* eslint-disable @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion */ import {nullTrace} from "../../test-utils/null-trace.js"; import {parseWorkflow} from "../../workflows/workflow-parser.js"; +import {MappingToken} from "./mapping-token.js"; +import {SequenceToken} from "./sequence-token.js"; import {StringToken} from "./string-token.js"; import {TemplateToken} from "./template-token.js"; describe("traverse", () => { - it("returns parent token and key", () => { + it("returns parent token, key, and ancestors", () => { const workflow = parseWorkflow( { name: "wf.yaml", @@ -18,19 +20,118 @@ describe("traverse", () => { const traverser = TemplateToken.traverse(root); // Root - expect(traverser.next()!.value).toEqual([undefined, root, undefined]); + const rootResult = traverser.next()!.value!; + expect(rootResult[0]).toBeUndefined(); + expect(rootResult[1]).toBe(root); + expect(rootResult[2]).toBeUndefined(); + expect(rootResult[3]).toEqual([]); // On const onResult = traverser.next().value!; expect(onResult[0]).toBe(root); expect(getValue(onResult[1])).toEqual("on"); expect(onResult[2]).toBeUndefined(); + expect(onResult[3]).toEqual([root]); // Push const pushResult = traverser.next().value!; expect(pushResult[0]).toBe(root); expect(getValue(pushResult[1])).toEqual("push"); expect(getValue(pushResult[2])).toEqual("on"); + expect(pushResult[3]).toEqual([root]); + }); + + it("returns ancestors for nested mappings", () => { + const workflow = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest` + }, + nullTrace + ); + + const root = workflow.value!; + const results = Array.from(TemplateToken.traverse(root)); + + // Find the "ubuntu-latest" token + const ubuntuResult = results.find(r => getValue(r[1]) === "ubuntu-latest")!; + expect(ubuntuResult).toBeDefined(); + + // Ancestors should be: root -> jobs mapping -> build mapping + const ancestors = ubuntuResult[3]; + expect(ancestors.length).toBe(3); + expect(ancestors[0]).toBe(root); + expect(ancestors[1]).toBeInstanceOf(MappingToken); // jobs mapping + expect(ancestors[2]).toBeInstanceOf(MappingToken); // build mapping + }); + + it("returns ancestors for sequences", () => { + const workflow = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hello` + }, + nullTrace + ); + + const root = workflow.value!; + const results = Array.from(TemplateToken.traverse(root)); + + // Find the "echo hello" token + const echoResult = results.find(r => getValue(r[1]) === "echo hello")!; + expect(echoResult).toBeDefined(); + + // Ancestors should be: root -> jobs mapping -> build mapping -> steps sequence -> step mapping + const ancestors = echoResult[3]; + expect(ancestors.length).toBe(5); + expect(ancestors[0]).toBe(root); + expect(ancestors[1]).toBeInstanceOf(MappingToken); // jobs mapping + expect(ancestors[2]).toBeInstanceOf(MappingToken); // build mapping + expect(ancestors[3]).toBeInstanceOf(SequenceToken); // steps sequence + expect(ancestors[4]).toBeInstanceOf(MappingToken); // step mapping + }); + + it("returns correct ancestors for matrix values", () => { + const workflow = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + node: [a, b] + steps: + - run: echo hi` + }, + nullTrace + ); + + const root = workflow.value!; + const results = Array.from(TemplateToken.traverse(root)); + + // Find the "a" token (first matrix value) + const nodeValueResult = results.find(r => { + const token = r[1]; + return token instanceof StringToken && token.value === "a"; + })!; + expect(nodeValueResult).toBeDefined(); + + // Ancestors: root -> jobs mapping -> build mapping -> strategy mapping -> matrix mapping -> node sequence + const ancestors = nodeValueResult[3]; + expect(ancestors.length).toBeGreaterThanOrEqual(5); + expect(ancestors[0]).toBe(root); + // Last ancestor should be the sequence containing [a, b] + expect(ancestors[ancestors.length - 1]).toBeInstanceOf(SequenceToken); }); }); diff --git a/workflow-parser/src/templates/tokens/template-token.ts b/workflow-parser/src/templates/tokens/template-token.ts index 36f067e..7588f5b 100644 --- a/workflow-parser/src/templates/tokens/template-token.ts +++ b/workflow-parser/src/templates/tokens/template-token.ts @@ -185,14 +185,23 @@ export abstract class TemplateToken { /** * Returns all tokens (depth first) - * @param value The object to travese + * @param value The object to traverse * @param omitKeys Whether to omit mapping keys + * @yields A tuple of [parent, token, keyToken, ancestors] for each token in the tree */ public static *traverse( value: TemplateToken, omitKeys?: boolean - ): Generator<[parent: TemplateToken | undefined, token: TemplateToken, keyToken: TemplateToken | undefined], void> { - yield [undefined, value, undefined]; + ): Generator< + [ + parent: TemplateToken | undefined, + token: TemplateToken, + keyToken: TemplateToken | undefined, + ancestors: TemplateToken[] + ], + void + > { + yield [undefined, value, undefined, []]; switch (value.templateTokenType) { case TokenType.Sequence: @@ -202,7 +211,7 @@ export abstract class TemplateToken { while (state.parent) { if (state.moveNext(omitKeys ?? false)) { value = state.current as TemplateToken; - yield [state.parent?.current, value, state.currentKey]; + yield [state.parent?.current, value, state.currentKey, state.getAncestors()]; switch (value.type) { case TokenType.Sequence: diff --git a/workflow-parser/src/templates/tokens/traversal-state.ts b/workflow-parser/src/templates/tokens/traversal-state.ts index 523e980..311ef5b 100644 --- a/workflow-parser/src/templates/tokens/traversal-state.ts +++ b/workflow-parser/src/templates/tokens/traversal-state.ts @@ -66,4 +66,19 @@ export class TraversalState { throw new Error(`Unexpected token type '${this._token.templateTokenType}' when traversing state`); } } + + /** + * Returns the ancestor tokens from root to the current token's parent container. + */ + public getAncestors(): TemplateToken[] { + const ancestors: TemplateToken[] = []; + let state: TraversalState | undefined = this.parent; + while (state) { + if (state.current) { + ancestors.unshift(state.current); + } + state = state.parent; + } + return ancestors; + } } diff --git a/workflow-parser/src/workflows/yaml-object-reader.ts b/workflow-parser/src/workflows/yaml-object-reader.ts index eb0d76e..6b5054f 100644 --- a/workflow-parser/src/workflows/yaml-object-reader.ts +++ b/workflow-parser/src/workflows/yaml-object-reader.ts @@ -152,11 +152,27 @@ export class YamlObjectReader implements ObjectReader { return new BooleanToken(fileId, range, value, undefined); case "string": { let source: string | undefined; + let blockScalarHeader: string | undefined; + if (token.srcToken && "source" in token.srcToken) { source = token.srcToken.source; + + // Extract block scalar header (e.g., |-, |+, >-) + // + // CST node interfaces are supported and documented per yaml library maintainer: + // https://eemeli.org/yaml/#parser -> "For a complete description of CST node + // interfaces, please consult the cst.ts source." + // See also: https://github.com/eemeli/yaml/issues/643 + if (token.srcToken.type === "block-scalar" && "props" in token.srcToken) { + const props = token.srcToken.props as Array<{type: string; source?: string}>; + const headerProp = props.find(p => p.type === "block-scalar-header"); + if (headerProp?.source) { + blockScalarHeader = headerProp.source; + } + } } - return new StringToken(fileId, range, value, undefined, source); + return new StringToken(fileId, range, value, undefined, source, blockScalarHeader); } default: throw new Error(`Unexpected value type '${typeof value}' when reading object`);