Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-multiline-comment-skip.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@workflow/builders': patch
---

Fix node-module-error plugin pointing at multi-line comments instead of code usage
56 changes: 56 additions & 0 deletions packages/builders/src/node-module-esbuild-plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,62 @@ describe('workflow-node-module-error plugin', () => {
}
});

it('should not point to JSDoc comments when finding usage', async () => {
const testCode = `
import { Writable } from "stream";
/**
* Convert a Web WritableStream<string> into a Node.js Writable stream
*/
export function workflow() {
return new Writable();
}
`;

const { failure } = await buildWorkflowWithViolation(testCode);

expect(failure.errors).toHaveLength(1);
const violation = failure.errors[0];
expect(violation.text).toContain('"stream" which is a Node.js module');
// Should point to the actual code usage, not the JSDoc comment
expect(violation.location?.lineText).toContain('new Writable()');
expect(violation.location?.lineText).not.toContain('*');
});

it('should not point to single-line block comments when finding usage', async () => {
const testCode = `
import { Writable } from "stream";
/* Writable is used below */
export function workflow() {
return new Writable();
}
`;

const { failure } = await buildWorkflowWithViolation(testCode);

expect(failure.errors).toHaveLength(1);
const violation = failure.errors[0];
// Should point to the actual code usage, not the comment
expect(violation.location?.lineText).toContain('new Writable()');
});

it('should not treat comment delimiters inside strings as comments', async () => {
const testCode = `
import { Writable } from "stream";
const pattern = "/* Writable */";
export function workflow() {
return new Writable();
}
`;

const { failure } = await buildWorkflowWithViolation(testCode);

expect(failure.errors).toHaveLength(1);
const violation = failure.errors[0];
expect(violation.text).toContain('"stream" which is a Node.js module');
// Should point to actual code, not the string containing "Writable"
expect(violation.location?.lineText).toContain('new Writable()');
});

it('should error on Bun module imports', async () => {
const testCode = `
import { serve } from "bun";
Expand Down
59 changes: 45 additions & 14 deletions packages/builders/src/node-module-esbuild-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,28 +159,59 @@ function findIdentifierUsage(
identifier: string
) {
const usageRegex = new RegExp(`\\b${escapeRegExp(identifier)}\\b`);
let inBlockComment = false;

for (let i = startIndex; i < lines.length; i += 1) {
const line = lines[i];

// Skip comments (both // and /* */ style)
const withoutComments = line
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/\/\/.*$/, '');
// Strip string literals first so that comment delimiters inside strings
// (e.g. `const s = "/*";`) don't confuse the comment scanner.
const stringsStripped = lines[i]
.replace(/'(?:[^'\\]|\\.)*'/g, (s) => ' '.repeat(s.length))
.replace(/"(?:[^"\\]|\\.)*"/g, (s) => ' '.repeat(s.length))
.replace(/`(?:[^`\\]|\\.)*`/g, (s) => ' '.repeat(s.length));

let line = stringsStripped;

// Handle multi-line block comments (including JSDoc)
if (inBlockComment) {
const endIdx = line.indexOf('*/');
if (endIdx === -1) {
continue;
}
line = ' '.repeat(endIdx + 2) + line.slice(endIdx + 2);
inBlockComment = false;
}

// Remove (replace with spaces) string literals to avoid matching inside paths
// Use escaped quote handling to properly match strings with escaped quotes
const withoutStrings = withoutComments
.replace(/'(?:[^'\\]|\\.)*'/g, (segment) => ' '.repeat(segment.length))
.replace(/"(?:[^"\\]|\\.)*"/g, (segment) => ' '.repeat(segment.length))
.replace(/`(?:[^`\\]|\\.)*`/g, (segment) => ' '.repeat(segment.length));
// Scan for comments, replacing them with spaces
let processed = '';
let j = 0;
while (j < line.length) {
if (line[j] === '/' && line[j + 1] === '/') {
processed += ' '.repeat(line.length - j);
break;
}
if (line[j] === '/' && line[j + 1] === '*') {
const endIdx = line.indexOf('*/', j + 2);
if (endIdx !== -1) {
const len = endIdx + 2 - j;
processed += ' '.repeat(len);
j = endIdx + 2;
} else {
processed += ' '.repeat(line.length - j);
inBlockComment = true;
break;
}
} else {
processed += line[j];
j += 1;
}
}
Comment on lines +174 to +207
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findIdentifierUsage's comment scanning runs before string-literal stripping and doesn't track string state. If a line contains /* inside a string literal without a matching */ (e.g. const s = "/*";), the loop will incorrectly enter inBlockComment and then skip all subsequent lines until a */ is found (potentially never), causing real identifier usages later in the file to be missed and the violation to go unreported. Consider stripping/replacing strings with spaces before detecting ////* tokens, or extending the scanner to ignore comment delimiters while inside '"/` strings; adding a regression test for this case would help.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems real

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — moved string literal stripping before the comment scanner so /* inside strings no longer triggers block comment mode. Added a regression test for const pattern = "/* Writable */";.


const match = withoutStrings.match(usageRegex);
const match = processed.match(usageRegex);
if (match && match.index !== undefined) {
return {
line: i,
column: match.index,
lineText: line,
lineText: lines[i],
};
}
}
Expand Down
Loading