Fix false positive for literal text in if conditions (#285)

* Fix false positive for literal text in `if` conditions

Use token.value (parsed string without YAML quotes) instead of token.source
(raw YAML text) for expression parsing in single-line strings. This fixes a
false positive where `if: "${{ expr }}"` incorrectly triggered the
"literal text in condition" error because the outer quotes were treated as
literal text.

Follow-up to PR #216
Related issue: https://github.com/github/vscode-github-actions/issues/542

* Move issue reference to comment

PR_REVIEW.md

@@ -1,59 +0,0 @@
-# PR #283 Review: Use property descriptions for completion items
-
-## Summary
-
-This PR fixes a bug where completion items for action.yml were missing descriptions. The root cause was that `mappingValues()` only looked at the type definition's description, ignoring property-level descriptions in the schema.
-
-## Changes Analysis
-
-### Core Fix ([definition.ts](languageservice/src/value-providers/definition.ts))
-
-**Before:**
-```typescript
-let description: string | undefined;
-if (value.type) {
-  const typeDef = definitions[value.type];
-  description = typeDef?.description;
-}
-```
-
-**After:**
-```typescript
-let description: string | undefined = value.description;
-if (value.type) {
-  const typeDef = definitions[value.type];
-  if (!description) {
-    description = typeDef?.description;
-  }
-}
-```
-
-✅ **Correct approach** - prioritizes property description, falls back to type description.
-
-### Test Coverage
-
-1. **complete-action.test.ts**: Two new tests verify `author` and `branding` completions include documentation
-2. **hover-action.test.ts**: New test for `author` hover + updated `branding` test to verify "Documentation" link
-
-## Potential Issues
-
-### 1. One-of expansion doesn't use property description
-
-Looking at line 140-142:
-```typescript
-const expanded = expandOneOfToCompletions(oneOfDef, definitions, key, description, indentation, mode);
-```
-
-This passes `description` to `expandOneOfToCompletions`, but at this point `description` may have been populated from the property. **This is correct** - the property description is passed through.
-
-### 2. Consistency check
-
-The PR description mentions this is consistent with hover. Verified: [template-reader.ts#L225](workflow-parser/src/templates/template-reader.ts#L225) shows hover uses `nextPropertyDef.description` when available.
-
-## Verdict
-
-✅ **LGTM** - Clean, minimal fix that aligns completion behavior with hover. Good test coverage for the specific cases mentioned.
-
-## Minor Suggestions (non-blocking)
-
-1. Could add a test for a property that has NO description but whose type DOES have one, to verify fallback works (e.g., `inputs` which references `inputs-strict` type that has a description)

languageservice/src/validate.expressions-literal-text.test.ts

@@ -211,4 +211,104 @@ jobs:
       );
     });
   });
+
+  // https://github.com/github/vscode-github-actions/issues/542
+  describe("YAML-quoted expressions", () => {
+    it("allows double-quoted expression in job-if", async () => {
+      // Quotes are needed when the expression contains a colon
+      const input = `
+on: push
+jobs:
+  publish:
+    if: "\${{ startsWith(github.event.head_commit.message, 'chore: release') }}"
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`;
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).not.toContainEqual(
+        expect.objectContaining({
+          code: "expression-literal-text-in-condition"
+        })
+      );
+    });
+
+    it("allows single-quoted expression in job-if", async () => {
+      const input = `
+on: push
+jobs:
+  publish:
+    if: '\${{ startsWith(github.event.head_commit.message, "chore: release") }}'
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`;
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).not.toContainEqual(
+        expect.objectContaining({
+          code: "expression-literal-text-in-condition"
+        })
+      );
+    });
+
+    it("allows double-quoted expression in step-if", async () => {
+      const input = `
+on: push
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - if: "\${{ contains(github.event.head_commit.message, 'skip: ci') }}"
+        run: echo hi
+`;
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).not.toContainEqual(
+        expect.objectContaining({
+          code: "expression-literal-text-in-condition"
+        })
+      );
+    });
+
+    it("still errors when there is actual literal text outside expression", async () => {
+      // Even with quotes, if there's literal text outside ${{ }}, it should error
+      const input = `
+on: push
+jobs:
+  build:
+    if: "push == \${{ github.event_name }}"
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`;
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toContainEqual(
+        expect.objectContaining({
+          code: "expression-literal-text-in-condition"
+        })
+      );
+    });
+
+    it("errors on multiple expressions with literal text between them", async () => {
+      const input = `
+on: push
+jobs:
+  build:
+    if: "\${{ true }} and \${{ false }}"
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`;
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toContainEqual(
+        expect.objectContaining({
+          code: "expression-literal-text-in-condition"
+        })
+      );
+    });
+  });
 });

workflow-parser/src/templates/template-reader.ts

@@ -451,7 +451,13 @@ class TemplateReader {
     }
 
     const allowedContext = definitionInfo.allowedContext;
-    const raw = token.source || token.value;
+    const isSingleLine = token.range === undefined || token.range.start.line === token.range.end.line;
+
+    // For single-line strings, use token.value (without YAML quotes) for expression detection,
+    // because token.source includes quote characters that would be incorrectly detected as literal text.
+    // For multi-line block scalars, use token.source directly because it makes position calculation easier
+    // (no quote characters to handle, and token.source preserves the original line/column structure in YAML).
+    const raw = isSingleLine ? token.value : token.source ?? token.value;
 
     let startExpression: number = raw.indexOf(OPEN_EXPRESSION);
     if (startExpression < 0) {
@@ -496,14 +502,17 @@ class TemplateReader {
         );
 
         let tr = token.range!;
-        if (tr.start.line === tr.end.line) {
-          // If it's a single line expression, adjust the range to only cover the sub-expression
+        if (isSingleLine) {
+          // Single-line: Adjust the range to only cover the sub-expression.
+          // Calculate offset to account for YAML quote characters.
+          // For example, `"${{ expr }}"` has source with quotes, value without.
+          const offset = (token.source ?? raw).indexOf(OPEN_EXPRESSION) - raw.indexOf(OPEN_EXPRESSION);
           tr = {
-            start: {line: tr.start.line, column: tr.start.column + startExpression},
-            end: {line: tr.end.line, column: tr.start.column + endExpression + 1}
+            start: {line: tr.start.line, column: tr.start.column + startExpression + offset},
+            end: {line: tr.end.line, column: tr.start.column + endExpression + 1 + offset}
           };
         } else {
-          // Adjust the range to only cover the expression for multi-line strings
+          // Multi-line: Adjust the range to only cover the expression
           const startRaw = raw.substring(0, startExpression);
           const adjustedStartLine = startRaw.split("\n").length;
           const beginningOfLine = startRaw.lastIndexOf("\n");