Skip to content

Commit 4719592

Browse files
committed
feat(ptc): add line and column numbers to static analysis errors
- Extract lineNumber directly from QuickJS error object instead of regex parsing - Add bounds checking to only report lines within agent code - Display column numbers for type errors (line N, col M) - Simplify typeValidator line number logic with clearer comments - Add tests for line numbers on first/last lines and column display
1 parent b6aa5d0 commit 4719592

File tree

5 files changed

+86
-86
lines changed

5 files changed

+86
-86
lines changed

src/node/services/ptc/staticAnalysis.ts

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -134,23 +134,28 @@ async function validateSyntax(code: string): Promise<AnalysisError | null> {
134134
});
135135

136136
if (result.error) {
137-
const errorObj: unknown = ctx.dump(result.error) as unknown;
137+
const errorObj = ctx.dump(result.error) as Record<string, unknown>;
138138
result.error.dispose();
139139

140-
// Parse QuickJS error format to extract line/column
141-
// ctx.dump returns an object like { name: "SyntaxError", message: "...", stack: "..." }
142-
const errorStr =
143-
typeof errorObj === "object" && errorObj !== null && "message" in errorObj
144-
? String((errorObj as { message: unknown }).message)
145-
: String(errorObj);
146-
const lineMatch = /line (\d+)/i.exec(errorStr);
147-
const columnMatch = /column (\d+)/i.exec(errorStr);
140+
// QuickJS error object has: { name, message, stack, fileName, lineNumber }
141+
const message =
142+
typeof errorObj.message === "string" ? errorObj.message : JSON.stringify(errorObj);
143+
const rawLine = typeof errorObj.lineNumber === "number" ? errorObj.lineNumber : undefined;
144+
145+
// Only report line if it's within agent code bounds.
146+
// The wrapper is `(function() { ${code} })` - all on one line with code inlined.
147+
// So QuickJS line N = agent line N for lines within the code.
148+
// Errors detected at the closing wrapper (missing braces, incomplete expressions)
149+
// will have line numbers beyond the agent's code - don't report those.
150+
const codeLines = code.split("\n").length;
151+
const line =
152+
rawLine !== undefined && rawLine >= 1 && rawLine <= codeLines ? rawLine : undefined;
148153

149154
return {
150155
type: "syntax",
151-
message: errorStr,
152-
line: lineMatch ? parseInt(lineMatch[1], 10) : undefined,
153-
column: columnMatch ? parseInt(columnMatch[1], 10) : undefined,
156+
message,
157+
line,
158+
column: undefined, // QuickJS doesn't provide column for syntax errors
154159
};
155160
}
156161

src/node/services/ptc/typeValidator.test.ts

Lines changed: 38 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ describe("validateTypes", () => {
7171
);
7272
expect(result.valid).toBe(false);
7373
// Error should mention 'path' doesn't exist or 'filePath' is missing
74-
expect(result.errors.some((e) => e.message.includes("path") || e.message.includes("filePath"))).toBe(
75-
true
76-
);
74+
expect(
75+
result.errors.some((e) => e.message.includes("path") || e.message.includes("filePath"))
76+
).toBe(true);
7777
});
7878

7979
test("catches missing required property", () => {
@@ -96,9 +96,9 @@ describe("validateTypes", () => {
9696
muxTypes
9797
);
9898
expect(result.valid).toBe(false);
99-
expect(result.errors.some((e) => e.message.includes("number") || e.message.includes("string"))).toBe(
100-
true
101-
);
99+
expect(
100+
result.errors.some((e) => e.message.includes("number") || e.message.includes("string"))
101+
).toBe(true);
102102
});
103103

104104
test("catches calling non-existent tool", () => {
@@ -126,82 +126,57 @@ mux.file_read({ path: "test.txt" });`,
126126
expect(errorWithLine!.line).toBe(3);
127127
});
128128

129-
test("allows dynamic property access (no strict checking on unknown keys)", () => {
130-
const result = validateTypes(
131-
`
132-
const result = mux.file_read({ filePath: "test.txt" });
133-
const key = "content";
134-
console.log(result[key]);
135-
`,
136-
muxTypes
137-
);
138-
// This should pass - we don't enforce strict property checking on results
139-
expect(result.valid).toBe(true);
129+
test("returns line 1 for error on first line", () => {
130+
const result = validateTypes(`mux.file_read({ path: "test.txt" });`, muxTypes);
131+
expect(result.valid).toBe(false);
132+
const errorWithLine = result.errors.find((e) => e.line !== undefined);
133+
expect(errorWithLine).toBeDefined();
134+
expect(errorWithLine!.line).toBe(1);
140135
});
141136

142-
test("allows console.log/warn/error", () => {
137+
test("returns correct line for error on last line of multi-line code", () => {
143138
const result = validateTypes(
144-
`
145-
console.log("hello");
146-
console.warn("warning");
147-
console.error("error");
148-
`,
139+
`const a = 1;
140+
const b = 2;
141+
const c = 3;
142+
const d = 4;
143+
mux.file_read({ path: "wrong" });`,
149144
muxTypes
150145
);
151-
expect(result.valid).toBe(true);
146+
expect(result.valid).toBe(false);
147+
const errorWithLine = result.errors.find((e) => e.line !== undefined);
148+
expect(errorWithLine).toBeDefined();
149+
expect(errorWithLine!.line).toBe(5);
152150
});
153151

154-
test("catches extra unexpected properties with object literal", () => {
155-
// TypeScript's excess property checking on object literals
156-
// Note: mux.* functions return results directly (no await) due to Asyncify
157-
const result = validateTypes(
158-
`
159-
mux.file_read({ filePath: "test.txt", unknownProp: true });
160-
`,
161-
muxTypes
162-
);
163-
// With strict: false, TS typically allows extra props on object literals
164-
expect(typeof result.valid).toBe("boolean");
152+
test("returns column number for type errors", () => {
153+
// Column should point to the problematic property
154+
const result = validateTypes(`mux.file_read({ path: "test.txt" });`, muxTypes);
155+
expect(result.valid).toBe(false);
156+
const errorWithLine = result.errors.find((e) => e.column !== undefined);
157+
expect(errorWithLine).toBeDefined();
158+
expect(errorWithLine!.column).toBeGreaterThan(0);
165159
});
166160

167-
test("handles multiline code correctly", () => {
161+
test("allows dynamic property access (no strict checking on unknown keys)", () => {
168162
const result = validateTypes(
169163
`
170-
const path = "test.txt";
171-
const offset = 10;
172-
const limit = 50;
173-
const result = mux.file_read({
174-
filePath: path,
175-
offset: offset,
176-
limit: limit
177-
});
178-
console.log(result);
164+
const result = mux.file_read({ filePath: "test.txt" });
165+
const key = "content";
166+
console.log(result[key]);
179167
`,
180168
muxTypes
181169
);
170+
// This should pass - we don't enforce strict property checking on results
182171
expect(result.valid).toBe(true);
183172
});
184173

185-
test("catches type error in later statement", () => {
186-
const result = validateTypes(
187-
`
188-
mux.file_read({ filePath: "test.txt" });
189-
mux.file_read({ filePath: 123 });
190-
`,
191-
muxTypes
192-
);
193-
expect(result.valid).toBe(false);
194-
});
195-
196-
test("allows valid bash tool call with all required params", () => {
174+
test("allows console.log/warn/error", () => {
197175
const result = validateTypes(
198176
`
199-
mux.bash({
200-
script: "echo hello",
201-
timeout_secs: 10,
202-
run_in_background: false,
203-
display_name: "Echo"
204-
});
177+
console.log("hello");
178+
console.warn("warning");
179+
console.error("error");
205180
`,
206181
muxTypes
207182
);

src/node/services/ptc/typeValidator.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,16 @@ ${muxTypes}
7777
.filter((d) => !ts.flattenDiagnosticMessageText(d.messageText, "").includes("console"))
7878
.map((d) => {
7979
const message = ts.flattenDiagnosticMessageText(d.messageText, " ");
80-
// Extract line number if available, adjusting for wrapper prefix
80+
// Extract line number if available
8181
if (d.file && d.start !== undefined) {
8282
const { line, character } = d.file.getLineAndCharacterOfPosition(d.start);
83-
// Line is 0-indexed; wrapper adds 1 line ("function __agent__() {\n")
84-
// Only report line if it's in the agent's code (not in muxTypes after)
83+
// TS line is 0-indexed. Wrapper adds 1 line before agent code, so:
84+
// TS line 0 = wrapper, TS line 1 = agent line 1, TS line 2 = agent line 2, etc.
85+
// This means TS 0-indexed line number equals agent 1-indexed line number.
86+
// Only report if within agent code bounds (filter out wrapper and muxTypes)
8587
const agentCodeLines = code.split("\n").length;
86-
const adjustedLine = line; // line 1 in file = line 0 (0-indexed) = agent line 1
87-
if (adjustedLine >= 1 && adjustedLine <= agentCodeLines) {
88-
return { message, line: adjustedLine, column: character + 1 };
88+
if (line >= 1 && line <= agentCodeLines) {
89+
return { message, line, column: character + 1 };
8990
}
9091
}
9192
return { message };

src/node/services/tools/code_execution.test.ts

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,20 @@ describe("createCodeExecutionTool", () => {
127127
expect(result.error).toContain("Code analysis failed");
128128
});
129129

130+
it("includes line numbers for syntax errors with invalid tokens", async () => {
131+
const tool = await createCodeExecutionTool(runtimeFactory, new ToolBridge({}));
132+
133+
// Invalid token @ on line 2 - parser detects it on the exact line
134+
const result = (await tool.execute!(
135+
{ code: "const x = 1;\nconst y = @;\nconst z = 3;" },
136+
mockToolCallOptions
137+
)) as PTCExecutionResult;
138+
139+
expect(result.success).toBe(false);
140+
expect(result.error).toContain("Code analysis failed");
141+
expect(result.error).toContain("(line 2)");
142+
});
143+
130144
it("rejects code using unavailable globals", async () => {
131145
const tool = await createCodeExecutionTool(runtimeFactory, new ToolBridge({}));
132146

@@ -165,7 +179,7 @@ describe("createCodeExecutionTool", () => {
165179
expect(result.error).toContain("require");
166180
});
167181

168-
it("includes line numbers for type errors", async () => {
182+
it("includes line and column numbers for type errors", async () => {
169183
const mockTools: Record<string, Tool> = {
170184
bash: createMockTool("bash", z.object({ script: z.string() })),
171185
};
@@ -179,10 +193,10 @@ describe("createCodeExecutionTool", () => {
179193
expect(result.success).toBe(false);
180194
expect(result.error).toContain("Code analysis failed");
181195
expect(result.error).toContain("scriptz");
182-
expect(result.error).toContain("(line 2)");
196+
expect(result.error).toContain("(line 2, col");
183197
});
184198

185-
it("includes line numbers for calling non-existent tools", async () => {
199+
it("includes line and column for calling non-existent tools", async () => {
186200
const mockTools: Record<string, Tool> = {
187201
bash: createMockTool("bash", z.object({ script: z.string() })),
188202
};
@@ -195,7 +209,7 @@ describe("createCodeExecutionTool", () => {
195209

196210
expect(result.success).toBe(false);
197211
expect(result.error).toContain("Code analysis failed");
198-
expect(result.error).toContain("(line 3)");
212+
expect(result.error).toContain("(line 3, col");
199213
});
200214
});
201215

src/node/services/tools/code_execution.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,12 @@ ${muxTypes}
9494
const analysis = await analyzeCode(code, muxTypes);
9595
if (!analysis.valid) {
9696
const errorMessages = analysis.errors.map((e) => {
97-
const location = e.line ? ` (line ${e.line})` : "";
97+
const location =
98+
e.line && e.column
99+
? ` (line ${e.line}, col ${e.column})`
100+
: e.line
101+
? ` (line ${e.line})`
102+
: "";
98103
return `- ${e.message}${location}`;
99104
});
100105
return {

0 commit comments

Comments
 (0)