Add block scalar newline warning (#295)

In YAML, block scalars (`|` and `>`) silently add a trailing newline by default
("clip" chomping). This can cause subtle bugs when the newline is unintentional.

This PR adds a warning when clip chomping is used in fields where trailing
newlines commonly cause issues:

- Environment variables (workflow, job, step, container, service levels)
- Action inputs (`with:`)
- Reusable workflow inputs and secrets
- Job outputs
- Matrix values (including `include` and `exclude`)
- Concurrency groups

The warning suggests using `|-` (strip) or `|+` (keep) to be explicit.

Intentionally does NOT warn for:
- `run:` scripts (trailing newlines are normal)
- Fields trimmed server-side (`if:`, `name:`, `runs-on:`, etc.)

The feature is gated behind the `blockScalarChompingWarning` feature flag.
This commit is contained in:
eric sciple
2026-01-12 09:36:43 -06:00
committed by GitHub
parent 54404aa9ff
commit 2816233a40
14 changed files with 1479 additions and 25 deletions
+351
View File
@@ -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("|-");
});
});
});
@@ -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
};
@@ -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
);
}
@@ -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 {
@@ -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);
});
});
@@ -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:
@@ -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;
}
}
@@ -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`);