From e74664fe9e6fbb4dbfcdfb726e6d51c5b328a9cf Mon Sep 17 00:00:00 2001 From: Joncallim Date: Thu, 9 Jul 2026 14:50:49 +0000 Subject: [PATCH 1/9] Harden work-package execution repair --- web/__tests__/work-package-executor.test.ts | 164 +++++++++++++++++++- web/worker/work-package-executor.ts | 76 +++++++-- 2 files changed, 217 insertions(+), 23 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index e5ab649..5b35963 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -188,6 +188,34 @@ describe('parseWorkPackageExecutionPlan', () => { expect(parsed.summary).toBe('Built tracker') expect(parsed.files).toHaveLength(1) }) + + it('parses a one-line fenced execution JSON block', () => { + const payload = JSON.stringify({ + schemaVersion: 1, + summary: 'Built tracker', + files: [{ path: 'package.json', content: '{"scripts":{}}' }], + commands: [], + }) + + const parsed = parseWorkPackageExecutionPlan(`\`\`\`work_package_execution_json ${payload}\`\`\``) + + expect(parsed.summary).toBe('Built tracker') + expect(parsed.files).toHaveLength(1) + }) + + it('parses a JSON-encoded execution response string', () => { + const payload = JSON.stringify({ + schemaVersion: 1, + summary: 'Built tracker', + files: [{ path: 'package.json', content: '{"scripts":{}}' }], + commands: [], + }) + + const parsed = parseWorkPackageExecutionPlan(JSON.stringify(payload)) + + expect(parsed.summary).toBe('Built tracker') + expect(parsed.files).toHaveLength(1) + }) }) describe('hasLocalConflictCopyPathSegment', () => { @@ -319,7 +347,7 @@ describe('executeWorkPackage', () => { }, { path: 'index.test.js', - content: 'import test from "node:test"; import assert from "node:assert/strict"; test("ok", () => assert.equal(1, 1));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("ok", () => assert.equal(1, 1));\n', }, ], commands: [['npm', 'test'], ['npm', 'run', 'build']], @@ -1131,7 +1159,7 @@ describe('executeWorkPackage', () => { }, { path: 'index.test.js', - content: 'import test from "node:test"; import assert from "node:assert/strict"; test("ok", () => assert.equal(1, 1));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("ok", () => assert.equal(1, 1));\n', }, ], commands: [['npm', 'test']], @@ -1261,7 +1289,7 @@ describe('executeWorkPackage', () => { validationStatus: 'failed', }) await expect(fs.stat(sandbox)).resolves.toMatchObject({}) - expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText).toHaveBeenCalledTimes(3) } }) @@ -1523,7 +1551,7 @@ describe('executeWorkPackage', () => { prompt: 'Build a tiny task tracker web app. Add focused tests and make sure the app builds.', }, }))).rejects.toThrow(/placeholder tests/i) - expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText).toHaveBeenCalledTimes(3) }) it('rejects invalid node:test shell scripts before command execution', async () => { @@ -1543,7 +1571,7 @@ describe('executeWorkPackage', () => { }, { path: 'tracker.test.js', - content: 'import test from "node:test"; import assert from "node:assert/strict"; test("adds item", () => assert.equal(1, 1));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("adds item", () => assert.equal(1, 1));\n', }, { path: 'build-check.js', content: 'console.log("build ok");\n' }, ], @@ -1596,7 +1624,7 @@ describe('executeWorkPackage', () => { }, { path: 'tracker.test.js', - content: 'import test from "node:test"; import assert from "node:assert/strict"; test("adds item", () => assert.equal(["a"].length, 1));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("adds item", () => assert.equal(["a"].length, 1));\n', }, { path: 'build-check.js', @@ -1618,4 +1646,128 @@ describe('executeWorkPackage', () => { expect(result.summary).toBe('Built and tested the tracker.') expect(result.commandResults.map((item) => item.exitCode)).toEqual([0, 0]) }) + + it('reprompts through invalid JSON and bad generated tests for a tiny task tracker', async () => { + mocks.generateText + .mockResolvedValueOnce({ + text: '```work_package_execution_json\n{"schemaVersion":1,"summary":"Cut off"', + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated weak tracker tests.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ + scripts: { + build: 'node build-check.js', + test: 'node test.js', + }, + }), + }, + { + path: 'tracker.test.js', + content: 'const test = require("node:test"); test("adds item", () => console.log("ok"));\n', + }, + { + path: 'build-check.js', + content: 'console.log("build validated");\n', + }, + ], + commands: [['npm', 'test'], ['npm', 'run', 'build']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Built a dependency-free tiny task tracker.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ + scripts: { + build: 'node build-check.js', + test: 'node --test', + }, + }), + }, + { + path: 'taskStore.js', + content: [ + 'function createState() { return { tasks: [] }; }', + 'function addTask(state, title) {', + ' const task = { id: String(state.tasks.length + 1), title, completed: false };', + ' return { tasks: [...state.tasks, task] };', + '}', + 'function toggleTask(state, id) {', + ' return { tasks: state.tasks.map((task) => task.id === id ? { ...task, completed: !task.completed } : task) };', + '}', + 'function deleteTask(state, id) { return { tasks: state.tasks.filter((task) => task.id !== id) }; }', + 'function filterTasks(state, filter) {', + ' if (filter === "active") return state.tasks.filter((task) => !task.completed);', + ' if (filter === "completed") return state.tasks.filter((task) => task.completed);', + ' return state.tasks;', + '}', + 'function hydrate(raw) {', + ' try {', + ' const parsed = JSON.parse(raw);', + ' return Array.isArray(parsed.tasks) ? parsed : createState();', + ' } catch {', + ' return createState();', + ' }', + '}', + 'module.exports = { createState, addTask, toggleTask, deleteTask, filterTasks, hydrate };', + '', + ].join('\n'), + }, + { + path: 'tracker.test.js', + content: [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + 'const { createState, addTask, toggleTask, deleteTask, filterTasks, hydrate } = require("./taskStore.js");', + '', + 'test("add, complete, filter, delete, and malformed storage fallback", () => {', + ' let state = createState();', + ' state = addTask(state, "Ship the tracker");', + ' assert.equal(state.tasks.length, 1);', + ' state = toggleTask(state, state.tasks[0].id);', + ' assert.deepEqual(filterTasks(state, "completed").map((task) => task.title), ["Ship the tracker"]);', + ' assert.equal(filterTasks(state, "active").length, 0);', + ' state = deleteTask(state, state.tasks[0].id);', + ' assert.equal(filterTasks(state, "all").length, 0);', + ' assert.deepEqual(hydrate("{bad json"), createState());', + '});', + '', + ].join('\n'), + }, + { + path: 'build-check.js', + content: [ + 'const fs = require("node:fs");', + 'for (const file of ["taskStore.js", "tracker.test.js"]) {', + ' if (!fs.existsSync(file)) throw new Error(`${file} is missing`);', + '}', + '', + ].join('\n'), + }, + ], + commands: [['npm', 'test'], ['npm', 'run', 'build']], + }), + }) + + const result = await executeWorkPackage(context({ + task: { + ...context().task, + prompt: 'Build a tiny task tracker web app. Add focused tests and make sure the app builds.', + }, + })) + + expect(mocks.generateText).toHaveBeenCalledTimes(3) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('Execution response was not valid JSON.') + expect(mocks.generateText.mock.calls[2][0].prompt).toContain('Generated test file is not a focused node:test assertion: tracker.test.js') + expect(result.summary).toBe('Built a dependency-free tiny task tracker.') + expect(result.commandResults.map((item) => item.exitCode)).toEqual([0, 0]) + }) }) diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index ea5fb09..100087d 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -36,7 +36,7 @@ const MAX_FILE_BYTES = 512 * 1024 const MAX_COMMANDS = 5 const MAX_COMMAND_OUTPUT_BYTES = 16 * 1024 const COMMAND_TIMEOUT_MS = 120_000 -const MAX_GENERATION_ATTEMPTS = 2 +const MAX_GENERATION_ATTEMPTS = 3 const DEFAULT_GENERATION_TIMEOUT_MS = 120_000 const DEFAULT_GENERATION_MAX_OUTPUT_TOKENS = 8000 export const MAX_WORK_PACKAGE_EXECUTION_ATTEMPTS = 3 @@ -289,9 +289,26 @@ function assertAllowedCommand(command: string[]): void { // small number of candidate start positions is more than enough. const MAX_JSON_SCAN_ATTEMPTS = 64 -function extractJson(rawText: string): string { - const fenced = /```(?:work_package_execution_json|json)?\s*\n([\s\S]*?)\n?```/i.exec(rawText) - if (fenced) return fenced[1] +function uniqueNonEmpty(values: string[]): string[] { + const seen = new Set() + const result: string[] = [] + for (const value of values) { + const trimmed = value.trim() + if (trimmed === '' || seen.has(trimmed)) continue + seen.add(trimmed) + result.push(trimmed) + } + return result +} + +function extractJsonCandidates(rawText: string): string[] { + const candidates: string[] = [] + const fencedPattern = /```([a-zA-Z0-9_-]+)?[^\S\r\n]*(?:\r?\n)?([\s\S]*?)```/g + for (const match of rawText.matchAll(fencedPattern)) { + const language = match[1]?.toLowerCase() ?? '' + if (language !== '' && language !== 'json' && language !== 'work_package_execution_json') continue + candidates.push(match[2]) + } let attempts = 0 for (let start = rawText.indexOf('{'); start >= 0; start = rawText.indexOf('{', start + 1)) { @@ -320,13 +337,23 @@ function extractJson(rawText: string): string { } else if (char === '}') { depth -= 1 if (depth === 0) { - return rawText.slice(start, index + 1) + candidates.push(rawText.slice(start, index + 1)) + break } } } } - return rawText + candidates.push(rawText) + return uniqueNonEmpty(candidates) +} + +function parseJsonCandidate(candidate: string): unknown { + let parsed = JSON.parse(candidate) as unknown + if (typeof parsed === 'string' && parsed.trim().startsWith('{')) { + parsed = JSON.parse(parsed) as unknown + } + return parsed } function normalizeExecutionPlan(parsed: unknown): WorkPackageExecutionPlan { @@ -448,10 +475,8 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin throw new Error('The user requested tests, but the execution plan did not run npm test.') } - const hasTestFile = plan.files.some((file) => - /(^|\/)(__tests__\/|.*(\.test|\.spec)\.[cm]?[jt]sx?$|test\.[cm]?js$)/i.test(file.path), - ) - if (!hasTestFile) { + const testFiles = plan.files.filter((file) => isJavaScriptFile(file.path) && isTestFile(file.path)) + if (testFiles.length === 0) { throw new Error('The user requested focused tests, but the execution plan did not include a test file.') } @@ -465,6 +490,11 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin if (isUnsafePackageScript(testScript)) { throw new Error('The generated test script includes unsafe shell behavior.') } + for (const file of testFiles) { + if (!/\bnode:test\b/.test(file.content) || !/\bassert\b/.test(file.content) || isPlaceholderContent(file.content)) { + throw new Error(`Generated test file is not a focused node:test assertion: ${file.path}`) + } + } } if (promptRequestsBuild(prompt)) { @@ -533,6 +563,9 @@ async function generateValidatedExecutionPlan(input: { '', 'Return a corrected full `work_package_execution_json` response.', 'Do not reuse placeholder tests or echo-only build scripts.', + 'For dependency-free JavaScript tests, set package.json scripts.test to `node --test`.', + 'Name test files `*.test.js` and import or require `node:test` plus `node:assert/strict`; assert real requested behavior.', + 'Do not return `node:test` as a shell command, console-only tests, placeholder tests, or prose outside the JSON fence.', ].join('\n') } } @@ -541,14 +574,23 @@ async function generateValidatedExecutionPlan(input: { } export function parseWorkPackageExecutionPlan(rawText: string): WorkPackageExecutionPlan { - const jsonText = extractJson(rawText) - let parsed: unknown - try { - parsed = JSON.parse(jsonText) - } catch { - throw new Error('Execution response was not valid JSON.') + let parseError: Error | null = null + let shapeError: Error | null = null + for (const jsonText of extractJsonCandidates(rawText)) { + let parsed: unknown + try { + parsed = parseJsonCandidate(jsonText) + } catch { + parseError ??= new Error('Execution response was not valid JSON.') + continue + } + try { + return normalizeExecutionPlan(parsed) + } catch (err) { + shapeError ??= err instanceof Error ? err : new Error(String(err)) + } } - return normalizeExecutionPlan(parsed) + throw shapeError ?? parseError ?? new Error('Execution response was not valid JSON.') } export function hasLocalConflictCopyPathSegment(filePath: string): boolean { From fc587c88fa1df35e39d2c122e9c64f9ea813d17b Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:19:37 +0800 Subject: [PATCH 2/9] Prioritize execution plan parse feedback --- web/__tests__/work-package-executor.test.ts | 10 ++++++++++ web/worker/work-package-executor.ts | 14 ++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 5b35963..e9eed58 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -189,6 +189,16 @@ describe('parseWorkPackageExecutionPlan', () => { expect(parsed.files).toHaveLength(1) }) + it('reports malformed execution JSON instead of an incidental object shape error', () => { + const raw = [ + 'Diagnostic metadata: {"provider":"local"}', + '```work_package_execution_json', + '{"schemaVersion":1,"summary":"Cut off"', + ].join('\n') + + expect(() => parseWorkPackageExecutionPlan(raw)).toThrow(/not valid JSON/i) + }) + it('parses a one-line fenced execution JSON block', () => { const payload = JSON.stringify({ schemaVersion: 1, diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index 100087d..7e551a2 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -310,6 +310,10 @@ function extractJsonCandidates(rawText: string): string[] { candidates.push(match[2]) } + // The full response is a stronger error signal than brace-scan fallbacks: + // those scans may find unrelated JSON in prose surrounding a malformed plan. + candidates.push(rawText) + let attempts = 0 for (let start = rawText.indexOf('{'); start >= 0; start = rawText.indexOf('{', start + 1)) { if (++attempts > MAX_JSON_SCAN_ATTEMPTS) break @@ -344,7 +348,6 @@ function extractJsonCandidates(rawText: string): string[] { } } - candidates.push(rawText) return uniqueNonEmpty(candidates) } @@ -574,23 +577,22 @@ async function generateValidatedExecutionPlan(input: { } export function parseWorkPackageExecutionPlan(rawText: string): WorkPackageExecutionPlan { - let parseError: Error | null = null - let shapeError: Error | null = null + let firstError: Error | null = null for (const jsonText of extractJsonCandidates(rawText)) { let parsed: unknown try { parsed = parseJsonCandidate(jsonText) } catch { - parseError ??= new Error('Execution response was not valid JSON.') + firstError ??= new Error('Execution response was not valid JSON.') continue } try { return normalizeExecutionPlan(parsed) } catch (err) { - shapeError ??= err instanceof Error ? err : new Error(String(err)) + firstError ??= err instanceof Error ? err : new Error(String(err)) } } - throw shapeError ?? parseError ?? new Error('Execution response was not valid JSON.') + throw firstError ?? new Error('Execution response was not valid JSON.') } export function hasLocalConflictCopyPathSegment(filePath: string): boolean { From d24f64aec223db18a511d9694fa17b33e6a676a5 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:26:16 +0800 Subject: [PATCH 3/9] Close execution repair review gaps --- web/__tests__/work-package-executor.test.ts | 96 +++++++++++++++++++++ web/worker/work-package-executor.ts | 61 ++++++++++--- 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index e9eed58..0b12023 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -155,6 +155,25 @@ describe('parseWorkPackageExecutionPlan', () => { expect(parsed.commands).toEqual([['npm', 'test']]) }) + it('normalizes package-script validation command aliases', () => { + const parsed = parseWorkPackageExecutionPlan(JSON.stringify({ + schemaVersion: 1, + summary: 'Built tracker', + files: [{ + path: 'package.json', + content: JSON.stringify({ + scripts: { + build: 'node build-check.js', + test: 'node tracker.test.js', + }, + }), + }], + commands: [['node', 'tracker.test.js'], ['node', 'build-check.js']], + })) + + expect(parsed.commands).toEqual([['npm', 'test'], ['npm', 'run', 'build']]) + }) + it('rejects unsupported commands', () => { expect(() => parseWorkPackageExecutionPlan(JSON.stringify({ schemaVersion: 1, @@ -1597,6 +1616,83 @@ describe('executeWorkPackage', () => { }))).rejects.toThrow(/test script is invalid/i) }) + it('accepts focused tests that use a localStorage stub and implementation placeholder text', async () => { + mocks.generateText.mockResolvedValue({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Built and tested the tracker.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ + scripts: { + build: 'node build-check.js', + test: 'node --test', + }, + }), + }, + { + path: 'tracker.js', + content: 'export function configure(input) { input.placeholder = "Add task"; }\n', + }, + { + path: 'tracker.test.js', + content: [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + 'test("persists with a localStorage stub", () => {', + ' const localStorageStub = new Map([["tasks", "[]"]]);', + ' assert.equal(localStorageStub.get("tasks"), "[]");', + '});', + '', + ].join('\n'), + }, + { + path: 'build-check.js', + content: 'console.log("build validated");\n', + }, + ], + commands: [['node', '--test'], ['node', 'build-check.js']], + }), + }) + + const result = await executeWorkPackage(context({ + task: { + ...context().task, + prompt: 'Build a tiny task tracker web app. Add focused tests and make sure the app builds.', + }, + })) + + expect(result.summary).toBe('Built and tested the tracker.') + expect(result.commandResults.map((item) => item.command)).toEqual([ + ['npm', 'test'], + ['npm', 'run', 'build'], + ]) + }) + + it('repairs a response truncated at the output limit', async () => { + mocks.generateText + .mockResolvedValueOnce({ + finishReason: 'length', + text: '{"schemaVersion":1,"summary":"Cut off"', + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Returned a concise complete plan.', + files: [{ path: 'package.json', content: '{}' }], + commands: [], + }), + }) + + const result = await executeWorkPackage(context()) + + expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('configured output limit') + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('Keep the response concise') + expect(result.summary).toBe('Returned a concise complete plan.') + }) + it('reprompts once when validation rejects the first generated plan', async () => { mocks.generateText .mockResolvedValueOnce({ diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index 7e551a2..01a7598 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -283,6 +283,43 @@ function assertAllowedCommand(command: string[]): void { } } +function normalizeCommandParts(command: unknown): string[] { + if (!Array.isArray(command)) throw new Error('Each command must be a string array.') + const normalized = command.map((part) => { + if (typeof part !== 'string') throw new Error('Each command part must be a string.') + return part.trim() + }).filter(Boolean) + if (normalized.length === 0) throw new Error('Execution command must be a non-empty string array.') + return normalized +} + +function scriptCommandMatches( + packageJson: Record | null, + scriptName: string, + normalizedCommand: string, +): boolean { + const script = packageScript(packageJson, scriptName) + return script !== '' && normalizeCommand(script.split(/\s+/)) === normalizedCommand +} + +function normalizeValidationCommand(command: unknown, packageJson: Record | null): string[] { + const normalized = normalizeCommandParts(command) + const normalizedCommand = normalizeCommand(normalized) + if (ALLOWED_COMMANDS.has(normalizedCommand)) return normalized + + if (normalizedCommand === 'npm run test') return ['npm', 'test'] + if (normalizedCommand === 'npm build') return ['npm', 'run', 'build'] + + const scriptMatches = [ + { canonical: ['npm', 'test'], name: 'test' }, + { canonical: ['npm', 'run', 'build'], name: 'build' }, + { canonical: ['npm', 'run', 'lint'], name: 'lint' }, + ].filter(({ name }) => scriptCommandMatches(packageJson, name, normalizedCommand)) + + if (scriptMatches.length === 1) return scriptMatches[0].canonical + throw new Error(`Command is not allowed: ${normalizedCommand}`) +} + // Bounds the brace-scan fallback below. Restarting a full inner scan from every // `{` is O(n²) on adversarial model output (e.g. thousands of unclosed braces), // which can stall the shared worker. Real responses put the object first, so a @@ -390,16 +427,9 @@ function normalizeExecutionPlan(parsed: unknown): WorkPackageExecutionPlan { return { path: filePath, content } }) + const packageJson = readGeneratedPackageJson(files) const commands = Array.isArray(parsed.commands) - ? parsed.commands.map((command) => { - if (!Array.isArray(command)) throw new Error('Each command must be a string array.') - const normalized = command.map((part) => { - if (typeof part !== 'string') throw new Error('Each command part must be a string.') - return part.trim() - }).filter(Boolean) - assertAllowedCommand(normalized) - return normalized - }) + ? parsed.commands.map((command) => normalizeValidationCommand(command, packageJson)) : [] if (commands.length > MAX_COMMANDS) { throw new Error(`Execution response included too many commands; maximum is ${MAX_COMMANDS}.`) @@ -426,7 +456,7 @@ function hasCommand(commands: string[][], expected: string): boolean { } function isPlaceholderContent(content: string): boolean { - return /\b(no tests? needed|not needed for this example|placeholder|todo only|stub)\b/i.test(content) + return /\b(no tests? needed|not needed for this example|placeholder|todo only)\b/i.test(content) } function readGeneratedPackageJson(files: WorkPackageExecutionFile[]): Record | null { @@ -484,7 +514,11 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin } const testScript = packageScript(packageJson, 'test') - if (isNoOpScript(testScript) || plan.files.some((file) => isPlaceholderContent(file.content))) { + if ( + isNoOpScript(testScript) || + isPlaceholderContent(testScript) || + testFiles.some((file) => isPlaceholderContent(file.content)) + ) { throw new Error('The generated test plan appears to contain placeholder tests.') } if (isInvalidNodeTestScript(testScript)) { @@ -528,6 +562,7 @@ async function generateValidatedExecutionPlan(input: { const controller = new AbortController() const timeout = setTimeout(() => controller.abort(), generationTimeoutMs()) let text: string + let responseError: Error | null = null try { const generated = await generateText({ abortSignal: controller.signal, @@ -538,7 +573,7 @@ async function generateValidatedExecutionPlan(input: { temperature: 0.1, }) if (generated.finishReason === 'length') { - throw new Error( + responseError = new Error( `Model generation stopped at the configured output limit (${generationMaxOutputTokens()} tokens) before producing a complete execution plan.`, ) } @@ -553,6 +588,7 @@ async function generateValidatedExecutionPlan(input: { } try { + if (responseError) throw responseError const plan = parseWorkPackageExecutionPlan(text) validatePlanAgainstPrompt(plan, input.taskPrompt) return plan @@ -565,6 +601,7 @@ async function generateValidatedExecutionPlan(input: { `Validation error: ${lastError.message}`, '', 'Return a corrected full `work_package_execution_json` response.', + 'Keep the response concise enough to finish within the configured output limit.', 'Do not reuse placeholder tests or echo-only build scripts.', 'For dependency-free JavaScript tests, set package.json scripts.test to `node --test`.', 'Name test files `*.test.js` and import or require `node:test` plus `node:assert/strict`; assert real requested behavior.', From 5a3f678c2e308cb4a59ec41343cdfcbd42b7900b Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:36:39 +0800 Subject: [PATCH 4/9] Complete work-package repair hardening --- .env.example | 4 +- docs/acp-zed-connector.md | 9 +- ...0006-executable-workforce-beta-boundary.md | 7 +- docs/developer-guide.md | 10 +- docs/operator-guide.md | 12 +- .../work-package-execution-context.test.ts | 17 ++- web/__tests__/work-package-executor.test.ts | 116 ++++++++++++++++-- web/worker/work-package-executor.ts | 62 ++++++---- 8 files changed, 180 insertions(+), 57 deletions(-) diff --git a/.env.example b/.env.example index 1d30853..ff09af1 100644 --- a/.env.example +++ b/.env.example @@ -58,8 +58,8 @@ FORGE_WORKER_CLAIM_TIMEOUT_SECONDS=5 # generated files. # FORGE_WORK_PACKAGE_EXECUTION=1 # FORGE_HOST_REPOSITORY_WRITES=1 -# ACP package execution remains separately disabled by default because ACP -# adapters are local processes and are not OS-confined by Forge. +# ACP package execution is enabled after task approval. Set this to 0 when local +# adapter process access is not acceptable; ACP is not OS-confined by Forge. # FORGE_ACP_WORK_PACKAGE_EXECUTION=0 # Workspace root — where Forge stores user-owned operational files: projects, diff --git a/docs/acp-zed-connector.md b/docs/acp-zed-connector.md index 5aae6d2..960610e 100644 --- a/docs/acp-zed-connector.md +++ b/docs/acp-zed-connector.md @@ -71,10 +71,11 @@ When a task uses an ACP provider, Forge: Forge uses one adapter process per call. It does not keep a long-lived pool of ACP agents. -Executable Workforce packages do not use ACP providers by default. ACP adapters -are local processes and Forge does not OS-confine them to the package attempt -sandbox. Set `FORGE_ACP_WORK_PACKAGE_EXECUTION=1` only after accepting that -local process risk for the repositories where Forge runs. +Executable Workforce packages may use a configured ACP provider after task +approval. ACP adapters are local processes and Forge does not OS-confine them +to the package attempt sandbox. Set `FORGE_ACP_WORK_PACKAGE_EXECUTION=0` to +disable ACP package execution on hosts where that local process risk is not +acceptable. ## Current Limits diff --git a/docs/adr/0006-executable-workforce-beta-boundary.md b/docs/adr/0006-executable-workforce-beta-boundary.md index 8aa0f46..1f29f26 100644 --- a/docs/adr/0006-executable-workforce-beta-boundary.md +++ b/docs/adr/0006-executable-workforce-beta-boundary.md @@ -28,9 +28,10 @@ edits and a reviewable sandbox copy of every generated file. disables model execution and keeps the handoff-artifact-only path. - `FORGE_HOST_REPOSITORY_WRITES=0`, `false`, `off`, `no`, or `disabled` lets package models run but keeps generated files sandbox-only. -- ACP-backed work-package execution is separately disabled by default. Set - `FORGE_ACP_WORK_PACKAGE_EXECUTION=1` only when the operator accepts that ACP - adapters are local processes and are not OS-confined by Forge. +- ACP-backed work-package execution is enabled after an operator configures an + ACP provider and approves the task. ACP adapters are local processes and are + not OS-confined by Forge. Set `FORGE_ACP_WORK_PACKAGE_EXECUTION=0`, `false`, + `off`, `no`, or `disabled` to prevent ACP package execution. - After Architect plan approval, Forge may execute one eligible specialist work package at a time. Parallel specialist execution remains out of scope. - Forge may collect bounded read-only host-repository context before a package diff --git a/docs/developer-guide.md b/docs/developer-guide.md index f997c8d..8e85f86 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -35,10 +35,10 @@ back through the same provider interface used by the worker. The currently wired Agent Client Protocol adapters wrap local tools such as Codex CLI and Claude Code; the underlying CLI must already be installed, authenticated, and runnable on the worker host. Architect ACP calls run in an isolated runtime directory. -Executable work-package ACP calls are disabled by default because ACP adapters -are local processes, not OS-confined sandboxes. Operators can opt in with -`FORGE_ACP_WORK_PACKAGE_EXECUTION=1` after accepting that risk. See [ACP and the -Zed connector](acp-zed-connector.md). +Executable work-package ACP calls are enabled after task approval. ACP adapters +are local processes, not OS-confined sandboxes. Operators can opt out with +`FORGE_ACP_WORK_PACKAGE_EXECUTION=0` when that risk is not acceptable. See [ACP +and the Zed connector](acp-zed-connector.md). ## Local Development @@ -155,7 +155,7 @@ Feature flag defaults: | `FORGE_WORK_PACKAGE_HANDOFF` | enabled | Set `0` or `false` to stop package handoff claims. | | `FORGE_WORK_PACKAGE_EXECUTION` | enabled | Set `0`, `false`, `off`, `no`, or `disabled` to stop specialist package execution and create handoff artifacts only. | | `FORGE_HOST_REPOSITORY_WRITES` | enabled | Set `0`, `false`, `off`, `no`, or `disabled` to keep generated files sandbox-only and skip local project edits. | -| `FORGE_ACP_WORK_PACKAGE_EXECUTION` | disabled | Set `1`, `true`, `on`, `yes`, or `enabled` only when local ACP package execution is an accepted operator risk. | +| `FORGE_ACP_WORK_PACKAGE_EXECUTION` | enabled | Set `0`, `false`, `off`, `no`, or `disabled` to block ACP package execution when local adapter process access is not acceptable. | | `FORGE_RUNNING_WORK_PACKAGE_STALE_SECONDS` | `900` | Recovery window before a retry marks an interrupted running work package blocked and starts the next eligible attempt. | ### Executable Workforce Beta diff --git a/docs/operator-guide.md b/docs/operator-guide.md index 012e5a5..4f2426b 100644 --- a/docs/operator-guide.md +++ b/docs/operator-guide.md @@ -153,11 +153,11 @@ For the currently wired ACP adapters: - The adapter wraps the local `codex` or `claude` CLI. - The local CLI must already be installed and logged in. - The Forge project must have a local folder so Forge can validate and bound - repository context. Architect planning uses an isolated runtime directory; - executable work-package ACP sessions are disabled by default because local ACP - adapters are not OS-confined by Forge. Set - `FORGE_ACP_WORK_PACKAGE_EXECUTION=1` only for repositories where that local - process access is acceptable. + repository context. Architect planning uses an isolated runtime directory. + Executable work-package ACP sessions are enabled after task approval, but the + local adapter is not OS-confined by Forge. Set + `FORGE_ACP_WORK_PACKAGE_EXECUTION=0` where that local process access is not + acceptable. - Installing the Zed editor is not required; Forge uses Agent Client Protocol adapter packages, not the editor itself. @@ -317,7 +317,7 @@ Worker and workspace options: | `FORGE_WORK_PACKAGE_HANDOFF` | Set `0` or `false` to disable default work-package handoff claims | | `FORGE_WORK_PACKAGE_EXECUTION` | Set `0`, `false`, `off`, `no`, or `disabled` to disable default package execution and create handoff artifacts only | | `FORGE_HOST_REPOSITORY_WRITES` | Set `0`, `false`, `off`, `no`, or `disabled` to keep generated files sandbox-only and skip local project edits | -| `FORGE_ACP_WORK_PACKAGE_EXECUTION` | Set `1`, `true`, `on`, `yes`, or `enabled` only when local ACP package execution is an accepted operator risk | +| `FORGE_ACP_WORK_PACKAGE_EXECUTION` | Set `0`, `false`, `off`, `no`, or `disabled` to block ACP package execution when local adapter process access is not acceptable | | `FORGE_RUNNING_WORK_PACKAGE_STALE_SECONDS` | Defaults to `900`; retry handoff treats older running package rows as interrupted and recovers them before continuing | | `FORGE_WORKSPACE_ROOT` | Fixed workspace root override | | `FORGE_MCPS_ROOT` | Fixed shared MCP root override | diff --git a/web/__tests__/work-package-execution-context.test.ts b/web/__tests__/work-package-execution-context.test.ts index a50fcfe..fdab062 100644 --- a/web/__tests__/work-package-execution-context.test.ts +++ b/web/__tests__/work-package-execution-context.test.ts @@ -104,8 +104,10 @@ describe('loadWorkPackageExecutionContext', () => { expect(context.validatedProjectRoot).toBe('/workspace/real-project') }) - it('blocks ACP-backed executable work packages unless explicitly enabled', async () => { + it('blocks ACP-backed executable work packages when explicitly disabled', async () => { vi.clearAllMocks() + const previous = process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION + process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = '0' const project = { id: 'project-1', localPath: '/workspace/project' } const task = { id: 'task-1', projectId: 'project-1', pmProviderConfigId: 'provider-task' } const workPackage = { id: 'pkg-1', assignedRole: 'backend' } @@ -116,17 +118,22 @@ describe('loadWorkPackageExecutionContext', () => { config: { providerType: 'acp', modelId: 'codex-cli::gpt-5.3-codex-spark' }, }) - await expect(loadWorkPackageExecutionContext('task-1', 'pkg-1')) - .rejects.toThrow(/ACP work-package execution is disabled/i) + try { + await expect(loadWorkPackageExecutionContext('task-1', 'pkg-1')) + .rejects.toThrow(/ACP work-package execution is disabled/i) + } finally { + if (previous === undefined) delete process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION + else process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = previous + } expect(mocks.assertProjectLocalPathForExecution).not.toHaveBeenCalled() expect(mocks.getModel).not.toHaveBeenCalled() }) - it('allows ACP-backed executable work packages when ACP execution is explicitly enabled', async () => { + it('allows ACP-backed executable work packages by default', async () => { vi.clearAllMocks() const previous = process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION - process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = '1' + delete process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION const project = { id: 'project-1', localPath: '/workspace/project' } const task = { id: 'task-1', projectId: 'project-1', pmProviderConfigId: 'provider-task' } const workPackage = { id: 'pkg-1', assignedRole: 'backend' } diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 0b12023..40e1f9e 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -245,6 +245,24 @@ describe('parseWorkPackageExecutionPlan', () => { expect(parsed.summary).toBe('Built tracker') expect(parsed.files).toHaveLength(1) }) + + it('parses a JSON-encoded fenced execution response string', () => { + const payload = [ + '```work_package_execution_json', + JSON.stringify({ + schemaVersion: 1, + summary: 'Built tracker', + files: [{ path: 'package.json', content: '{"scripts":{}}' }], + commands: [], + }), + '```', + ].join('\n') + + const parsed = parseWorkPackageExecutionPlan(JSON.stringify(payload)) + + expect(parsed.summary).toBe('Built tracker') + expect(parsed.files).toHaveLength(1) + }) }) describe('hasLocalConflictCopyPathSegment', () => { @@ -1253,7 +1271,7 @@ describe('executeWorkPackage', () => { await expect(executeWorkPackage(context())).rejects.toThrow(/at least one checkable JavaScript source file/i) }) - it('throws validation failures with durable sandbox output metadata', async () => { + it('wraps command-selected generation validation failures with durable empty sandbox metadata', async () => { mocks.generateText.mockResolvedValue({ text: JSON.stringify({ schemaVersion: 1, @@ -1279,16 +1297,15 @@ describe('executeWorkPackage', () => { expect(err).toBeInstanceOf(WorkPackageExecutionError) const failure = (err as WorkPackageExecutionError).failureDetails expect(failure.sandboxPath).toBe(path.join(tempRoot, '.forge', 'task-runs', 'task-1', 'pkg-1', 'attempt-1')) - expect(failure.fileCount).toBe(1) + expect(failure.fileCount).toBe(0) expect(failure.artifactMetadata).toMatchObject({ - files: ['package.json'], + files: [], generatedBy: 'work-package-executor', repositoryWrites: false, - sandboxWrites: true, + sandboxWrites: false, validationStatus: 'failed', }) - expect(failure.commandResults).toHaveLength(1) - expect(failure.commandResults[0].exitCode).not.toBe(0) + expect(failure.commandResults).toHaveLength(0) } }) @@ -1642,7 +1659,9 @@ describe('executeWorkPackage', () => { 'const assert = require("node:assert/strict");', 'test("persists with a localStorage stub", () => {', ' const localStorageStub = new Map([["tasks", "[]"]]);', + ' const input = { placeholder: "Add task" };', ' assert.equal(localStorageStub.get("tasks"), "[]");', + ' assert.equal(input.placeholder, "Add task");', '});', '', ].join('\n'), @@ -1693,6 +1712,89 @@ describe('executeWorkPackage', () => { expect(result.summary).toBe('Returned a concise complete plan.') }) + it('repairs selected test validation when the task prompt does not mention tests', async () => { + mocks.generateText + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated weak tests.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { test: 'node --test' } }), + }, + { + path: 'tracker.test.js', + content: 'const test = require("node:test"); test("adds", () => console.log("ok"));\n', + }, + ], + commands: [['npm', 'test']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated focused tests.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { test: 'node --test' } }), + }, + { + path: 'tracker.test.js', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("adds", () => assert.equal(1, 1));\n', + }, + ], + commands: [['npm', 'test']], + }), + }) + + const result = await executeWorkPackage(context()) + + expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('not a focused node:test assertion') + expect(result.summary).toBe('Generated focused tests.') + }) + + it('repairs a selected placeholder build when the task prompt does not mention builds', async () => { + mocks.generateText + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated a placeholder build.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { build: 'echo "build passed"' } }), + }, + { path: 'app.js', content: 'export const ready = true;\n' }, + ], + commands: [['npm', 'run', 'build']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated a meaningful build check.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { build: 'node build-check.js --strict true' } }), + }, + { path: 'app.js', content: 'export const ready = true;\n' }, + { path: 'build-check.js', content: 'console.log("build validated");\n' }, + ], + commands: [['npm', 'run', 'build']], + }), + }) + + const result = await executeWorkPackage(context()) + + expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('build script appears to be a placeholder') + expect(result.summary).toBe('Generated a meaningful build check.') + }) + it('reprompts once when validation rejects the first generated plan', async () => { mocks.generateText .mockResolvedValueOnce({ @@ -1774,7 +1876,7 @@ describe('executeWorkPackage', () => { }, { path: 'tracker.test.js', - content: 'const test = require("node:test"); test("adds item", () => console.log("ok"));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("adds item", () => console.log("ok"));\n', }, { path: 'build-check.js', diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index 01a7598..e912c8d 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -27,6 +27,7 @@ import { sanitizeWorkerMessage } from './redaction' import { publishTaskEvent } from './events' import { recordTaskLogBestEffort } from './task-logs' import { shouldApplyHostRepositoryWrites } from './repository-edit-policy' +import { defaultOnFeatureFlagEnabled } from './feature-flags' const execFile = promisify(execFileCallback) @@ -47,8 +48,6 @@ const ALLOWED_COMMANDS = new Set([ 'npm run lint', ]) -const ACP_EXECUTION_ENABLED_VALUES = new Set(['1', 'true', 'on', 'yes', 'enabled']) - type TaskRow = typeof tasks.$inferSelect type ProjectRow = typeof projects.$inferSelect type WorkPackageRow = typeof workPackages.$inferSelect @@ -256,8 +255,7 @@ function isAcpModel(model: LanguageModel): boolean { } function isAcpWorkPackageExecutionEnabled(env: Record = process.env): boolean { - const raw = env.FORGE_ACP_WORK_PACKAGE_EXECUTION?.trim().toLowerCase() - return raw !== undefined && ACP_EXECUTION_ENABLED_VALUES.has(raw) + return defaultOnFeatureFlagEnabled(env.FORGE_ACP_WORK_PACKAGE_EXECUTION) } function generationTimeoutMs(): number { @@ -325,6 +323,7 @@ function normalizeValidationCommand(command: unknown, packageJson: Record() @@ -389,11 +388,7 @@ function extractJsonCandidates(rawText: string): string[] { } function parseJsonCandidate(candidate: string): unknown { - let parsed = JSON.parse(candidate) as unknown - if (typeof parsed === 'string' && parsed.trim().startsWith('{')) { - parsed = JSON.parse(parsed) as unknown - } - return parsed + return JSON.parse(candidate) as unknown } function normalizeExecutionPlan(parsed: unknown): WorkPackageExecutionPlan { @@ -456,7 +451,7 @@ function hasCommand(commands: string[][], expected: string): boolean { } function isPlaceholderContent(content: string): boolean { - return /\b(no tests? needed|not needed for this example|placeholder|todo only)\b/i.test(content) + return /\b(no tests? needed|not needed for this example|todo only)\b/i.test(content) } function readGeneratedPackageJson(files: WorkPackageExecutionFile[]): Record | null { @@ -481,11 +476,17 @@ function packageScript(packageJson: Record | null, name: string function isNoOpScript(script: string): boolean { return ( script === '' || - /(^|\s)(true|exit 0)(\s|$)/i.test(script) || - /^\s*echo\s+['"]?(build script not needed|no tests? needed|not needed for this example|ok|done)/i.test(script) + /^\s*(?:true|exit\s+0)\s*$/i.test(script) || + /^\s*echo(?:\s|$)/i.test(script) ) } +function isFocusedNodeTestAssertion(content: string): boolean { + const hasTestCall = /\b(?:test|it)(?:\.(?:only|skip|todo))?\s*\(/.test(content) + const hasAssertionCall = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/.test(content) + return /\bnode:test\b/.test(content) && /\bnode:assert\/strict\b/.test(content) && hasTestCall && hasAssertionCall +} + function isInvalidNodeTestScript(script: string): boolean { return /^\s*node:test(\s|$)/i.test(script) } @@ -502,15 +503,17 @@ function isUnsafePackageScript(script: string): boolean { function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: string): void { const packageJson = readGeneratedPackageJson(plan.files) + const testsRequested = promptRequestsTests(prompt) + const testCommandSelected = hasCommand(plan.commands, 'npm test') - if (promptRequestsTests(prompt)) { - if (!hasCommand(plan.commands, 'npm test')) { - throw new Error('The user requested tests, but the execution plan did not run npm test.') - } + if (testsRequested && !testCommandSelected) { + throw new Error('The user requested tests, but the execution plan did not run npm test.') + } + if (testsRequested || testCommandSelected) { const testFiles = plan.files.filter((file) => isJavaScriptFile(file.path) && isTestFile(file.path)) if (testFiles.length === 0) { - throw new Error('The user requested focused tests, but the execution plan did not include a test file.') + throw new Error('The execution plan selected focused tests but did not include a test file.') } const testScript = packageScript(packageJson, 'test') @@ -528,17 +531,19 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin throw new Error('The generated test script includes unsafe shell behavior.') } for (const file of testFiles) { - if (!/\bnode:test\b/.test(file.content) || !/\bassert\b/.test(file.content) || isPlaceholderContent(file.content)) { + if (!isFocusedNodeTestAssertion(file.content) || isPlaceholderContent(file.content)) { throw new Error(`Generated test file is not a focused node:test assertion: ${file.path}`) } } } - if (promptRequestsBuild(prompt)) { - if (!hasCommand(plan.commands, 'npm run build')) { - throw new Error('The user requested a build check, but the execution plan did not run npm run build.') - } + const buildRequested = promptRequestsBuild(prompt) + const buildCommandSelected = hasCommand(plan.commands, 'npm run build') + if (buildRequested && !buildCommandSelected) { + throw new Error('The user requested a build check, but the execution plan did not run npm run build.') + } + if (buildRequested || buildCommandSelected) { const buildScript = packageScript(packageJson, 'build') if (isNoOpScript(buildScript)) { throw new Error('The generated build script appears to be a placeholder.') @@ -615,7 +620,9 @@ async function generateValidatedExecutionPlan(input: { export function parseWorkPackageExecutionPlan(rawText: string): WorkPackageExecutionPlan { let firstError: Error | null = null - for (const jsonText of extractJsonCandidates(rawText)) { + const candidates = extractJsonCandidates(rawText).map((text) => ({ depth: 0, text })) + for (let index = 0; index < candidates.length; index += 1) { + const { depth, text: jsonText } = candidates[index] let parsed: unknown try { parsed = parseJsonCandidate(jsonText) @@ -623,6 +630,11 @@ export function parseWorkPackageExecutionPlan(rawText: string): WorkPackageExecu firstError ??= new Error('Execution response was not valid JSON.') continue } + if (typeof parsed === 'string' && depth < MAX_JSON_STRING_DECODE_DEPTH) { + const decoded = extractJsonCandidates(parsed).map((text) => ({ depth: depth + 1, text })) + candidates.splice(index + 1, 0, ...decoded) + continue + } try { return normalizeExecutionPlan(parsed) } catch (err) { @@ -801,7 +813,7 @@ async function validateGeneratedCommand(projectRoot: string, command: string[]): for (const file of testFiles) { const absolute = path.join(projectRoot, file) const content = await fs.readFile(absolute, 'utf8') - if (!/\bnode:test\b/.test(content) || !/\bassert\b/.test(content) || isPlaceholderContent(content)) { + if (!isFocusedNodeTestAssertion(content) || isPlaceholderContent(content)) { throw new Error(`Generated test file is not a focused node:test assertion: ${file}`) } await safeSyntaxCheck(absolute) @@ -1511,7 +1523,7 @@ export async function loadWorkPackageExecutionContext( if (!provider) throw new Error(`Provider config ${providerConfigId} is missing or inactive.`) if (provider.config.providerType === 'acp' && !isAcpWorkPackageExecutionEnabled()) { throw new Error( - 'ACP work-package execution is disabled. Set FORGE_ACP_WORK_PACKAGE_EXECUTION=1 only after accepting that ACP adapters are local processes and are not OS-confined by Forge.', + 'ACP work-package execution is disabled by FORGE_ACP_WORK_PACKAGE_EXECUTION. Remove the setting or set it to 1 after accepting that ACP adapters are local processes and are not OS-confined by Forge.', ) } From 3818b08ec070c541b7dfb6b013e532c00fe8a100 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:40:36 +0800 Subject: [PATCH 5/9] Repair selected lint validation --- web/__tests__/work-package-executor.test.ts | 59 ++++++++++++++++++++- web/worker/work-package-executor.ts | 13 +++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 40e1f9e..f6c9c06 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -164,14 +164,15 @@ describe('parseWorkPackageExecutionPlan', () => { content: JSON.stringify({ scripts: { build: 'node build-check.js', + lint: 'node lint-check.js', test: 'node tracker.test.js', }, }), }], - commands: [['node', 'tracker.test.js'], ['node', 'build-check.js']], + commands: [['node', 'tracker.test.js'], ['node', 'build-check.js'], ['node', 'lint-check.js']], })) - expect(parsed.commands).toEqual([['npm', 'test'], ['npm', 'run', 'build']]) + expect(parsed.commands).toEqual([['npm', 'test'], ['npm', 'run', 'build'], ['npm', 'run', 'lint']]) }) it('rejects unsupported commands', () => { @@ -1795,6 +1796,60 @@ describe('executeWorkPackage', () => { expect(result.summary).toBe('Generated a meaningful build check.') }) + it('repairs selected lint validation before writing sandbox files', async () => { + mocks.generateText + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated a placeholder lint command.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { lint: 'echo "lint passed"' } }), + }, + { path: 'app.js', content: 'module.exports = { ready: true };\n' }, + ], + commands: [['npm', 'run', 'lint']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated unchecked TypeScript lint input.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { lint: 'node lint-check.js' } }), + }, + { path: 'app.tsx', content: 'export const App = () =>
;\n' }, + ], + commands: [['npm', 'run', 'lint']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated checkable lint input.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { lint: 'node lint-check.js' } }), + }, + { path: 'app.js', content: 'module.exports = { ready: true };\n' }, + { path: 'lint-check.js', content: 'console.log("lint validated");\n' }, + ], + commands: [['npm', 'run', 'lint']], + }), + }) + + const result = await executeWorkPackage(context()) + + expect(mocks.generateText).toHaveBeenCalledTimes(3) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('lint script appears to be a placeholder') + expect(mocks.generateText.mock.calls[2][0].prompt).toContain('at least one checkable JavaScript source file') + expect(result.summary).toBe('Generated checkable lint input.') + }) + it('reprompts once when validation rejects the first generated plan', async () => { mocks.generateText .mockResolvedValueOnce({ diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index e912c8d..6f42f57 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -552,6 +552,19 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin throw new Error('The generated build script includes unsafe shell behavior.') } } + + if (hasCommand(plan.commands, 'npm run lint')) { + const lintScript = packageScript(packageJson, 'lint') + if (isNoOpScript(lintScript)) { + throw new Error('The generated lint script appears to be a placeholder.') + } + if (isUnsafePackageScript(lintScript)) { + throw new Error('The generated lint script includes unsafe shell behavior.') + } + if (!plan.files.some((file) => isJavaScriptFile(file.path))) { + throw new Error('Static lint validation requires at least one checkable JavaScript source file.') + } + } } async function generateValidatedExecutionPlan(input: { From 504ee9dc496693b99a86e29c1801cbbfbc11e0fd Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:47:46 +0800 Subject: [PATCH 6/9] Close final execution repair review gaps --- .../work-package-execution-context.test.ts | 51 +++++---- web/__tests__/work-package-executor.test.ts | 106 ++++++++++++++++-- web/worker/work-package-executor.ts | 106 +++++++++++++++++- 3 files changed, 224 insertions(+), 39 deletions(-) diff --git a/web/__tests__/work-package-execution-context.test.ts b/web/__tests__/work-package-execution-context.test.ts index fdab062..19fae5e 100644 --- a/web/__tests__/work-package-execution-context.test.ts +++ b/web/__tests__/work-package-execution-context.test.ts @@ -104,31 +104,34 @@ describe('loadWorkPackageExecutionContext', () => { expect(context.validatedProjectRoot).toBe('/workspace/real-project') }) - it('blocks ACP-backed executable work packages when explicitly disabled', async () => { - vi.clearAllMocks() - const previous = process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION - process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = '0' - const project = { id: 'project-1', localPath: '/workspace/project' } - const task = { id: 'task-1', projectId: 'project-1', pmProviderConfigId: 'provider-task' } - const workPackage = { id: 'pkg-1', assignedRole: 'backend' } - mocks.dbSelect - .mockReturnValueOnce(chain([{ task, project, workPackage }])) - .mockReturnValueOnce(chain([{ id: 'agent-backend', providerConfigId: null }])) - mocks.getProvider.mockResolvedValue({ - config: { providerType: 'acp', modelId: 'codex-cli::gpt-5.3-codex-spark' }, - }) - - try { - await expect(loadWorkPackageExecutionContext('task-1', 'pkg-1')) - .rejects.toThrow(/ACP work-package execution is disabled/i) - } finally { - if (previous === undefined) delete process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION - else process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = previous - } + it.each(['0', 'flase'])( + 'blocks ACP-backed executable work packages when the setting is %s', + async (setting) => { + vi.clearAllMocks() + const previous = process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION + process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = setting + const project = { id: 'project-1', localPath: '/workspace/project' } + const task = { id: 'task-1', projectId: 'project-1', pmProviderConfigId: 'provider-task' } + const workPackage = { id: 'pkg-1', assignedRole: 'backend' } + mocks.dbSelect + .mockReturnValueOnce(chain([{ task, project, workPackage }])) + .mockReturnValueOnce(chain([{ id: 'agent-backend', providerConfigId: null }])) + mocks.getProvider.mockResolvedValue({ + config: { providerType: 'acp', modelId: 'codex-cli::gpt-5.3-codex-spark' }, + }) + + try { + await expect(loadWorkPackageExecutionContext('task-1', 'pkg-1')) + .rejects.toThrow(/ACP work-package execution is disabled/i) + } finally { + if (previous === undefined) delete process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION + else process.env.FORGE_ACP_WORK_PACKAGE_EXECUTION = previous + } - expect(mocks.assertProjectLocalPathForExecution).not.toHaveBeenCalled() - expect(mocks.getModel).not.toHaveBeenCalled() - }) + expect(mocks.assertProjectLocalPathForExecution).not.toHaveBeenCalled() + expect(mocks.getModel).not.toHaveBeenCalled() + }, + ) it('allows ACP-backed executable work packages by default', async () => { vi.clearAllMocks() diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index f6c9c06..429b8e1 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -1223,31 +1223,117 @@ describe('executeWorkPackage', () => { await expect(fs.stat(outsideFile)).rejects.toMatchObject({ code: 'ENOENT' }) }) - it('fails build validation when no JavaScript source files can be checked', async () => { + it('repairs build validation when no JavaScript source files can be checked', async () => { + mocks.generateText + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated unchecked TypeScript.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { build: 'tsc --noEmit' } }), + }, + { + path: 'src/app.tsx', + content: 'export const App = () =>
\n', + }, + ], + commands: [['npm', 'run', 'build']], + }), + }) + .mockResolvedValueOnce({ + text: JSON.stringify({ + schemaVersion: 1, + summary: 'Generated checkable JavaScript.', + files: [ + { + path: 'package.json', + content: JSON.stringify({ scripts: { build: 'node build-check.js' } }), + }, + { path: 'app.js', content: 'module.exports = { ready: true };\n' }, + { path: 'build-check.js', content: 'console.log("build validated");\n' }, + ], + commands: [['npm', 'run', 'build']], + }), + }) + + const result = await executeWorkPackage(context({ + task: { + ...context().task, + prompt: 'Build a tiny task tracker web app. Make sure it builds.', + }, + })) + + expect(mocks.generateText).toHaveBeenCalledTimes(2) + expect(mocks.generateText.mock.calls[1][0].prompt).toContain('at least one checkable JavaScript source file') + expect(result.summary).toBe('Generated checkable JavaScript.') + }) + + it.each([ + [ + 'comment-only calls', + [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + '// test("fake", () => assert.equal(1, 1));', + '', + ].join('\n'), + ], + [ + 'skipped and TODO tests', + [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + 'test.skip("skipped", () => assert.equal(1, 1));', + 'test.todo("todo");', + '', + ].join('\n'), + ], + [ + 'string-only calls', + [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + 'const example = "test(\\"fake\\", () => assert.equal(1, 1))";', + 'void example;', + '', + ].join('\n'), + ], + [ + 'string-only module references', + [ + 'const modules = `require("node:test") require("node:assert/strict")`;', + 'const test = (_name, callback) => callback();', + 'const assert = { equal: () => undefined };', + 'test("fake", () => assert.equal(1, 1));', + 'void modules;', + '', + ].join('\n'), + ], + ])('rejects %s as focused test coverage', async (_label, content) => { mocks.generateText.mockResolvedValue({ text: JSON.stringify({ schemaVersion: 1, - summary: 'Generated unchecked TypeScript.', + summary: 'Generated non-running tests.', files: [ { path: 'package.json', - content: JSON.stringify({ scripts: { build: 'tsc --noEmit' } }), - }, - { - path: 'src/app.tsx', - content: 'export const App = () =>
\n', + content: JSON.stringify({ scripts: { test: 'node --test' } }), }, + { path: 'tracker.test.js', content }, ], - commands: [['npm', 'run', 'build']], + commands: [['npm', 'test']], }), }) await expect(executeWorkPackage(context({ task: { ...context().task, - prompt: 'Build a tiny task tracker web app. Make sure it builds.', + prompt: 'Build a tiny task tracker web app with focused tests.', }, - }))).rejects.toThrow(/at least one checkable JavaScript source file/i) + }))).rejects.toThrow(/not a focused node:test assertion/i) + expect(mocks.generateText).toHaveBeenCalledTimes(3) }) it('fails lint validation when no JavaScript source files can be checked', async () => { diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index 6f42f57..a75735a 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -27,7 +27,7 @@ import { sanitizeWorkerMessage } from './redaction' import { publishTaskEvent } from './events' import { recordTaskLogBestEffort } from './task-logs' import { shouldApplyHostRepositoryWrites } from './repository-edit-policy' -import { defaultOnFeatureFlagEnabled } from './feature-flags' +import { defaultOnFeatureFlagState } from './feature-flags' const execFile = promisify(execFileCallback) @@ -255,7 +255,8 @@ function isAcpModel(model: LanguageModel): boolean { } function isAcpWorkPackageExecutionEnabled(env: Record = process.env): boolean { - return defaultOnFeatureFlagEnabled(env.FORGE_ACP_WORK_PACKAGE_EXECUTION) + const state = defaultOnFeatureFlagState(env.FORGE_ACP_WORK_PACKAGE_EXECUTION) + return state.recognized && state.enabled } function generationTimeoutMs(): number { @@ -481,10 +482,102 @@ function isNoOpScript(script: string): boolean { ) } +function scannedJavaScriptText(content: string, stripStrings: boolean): string { + const output = content.split('') + let state: 'code' | 'single' | 'double' | 'template' | 'line-comment' | 'block-comment' = 'code' + let escaped = false + + const blank = (index: number) => { + if (output[index] !== '\n' && output[index] !== '\r') output[index] = ' ' + } + + for (let index = 0; index < content.length; index += 1) { + const char = content[index] + const next = content[index + 1] + + if (state === 'code') { + if (char === '/' && next === '/') { + blank(index) + blank(index + 1) + index += 1 + state = 'line-comment' + } else if (char === '/' && next === '*') { + blank(index) + blank(index + 1) + index += 1 + state = 'block-comment' + } else if (char === "'") { + if (stripStrings) blank(index) + state = 'single' + } else if (char === '"') { + if (stripStrings) blank(index) + state = 'double' + } else if (char === '`') { + if (stripStrings) blank(index) + state = 'template' + } + continue + } + + if (state === 'line-comment') { + if (char === '\n' || char === '\r') state = 'code' + else blank(index) + continue + } + + if (state === 'block-comment') { + if (char === '*' && next === '/') { + blank(index) + blank(index + 1) + index += 1 + state = 'code' + } else { + blank(index) + } + continue + } + + if (stripStrings) blank(index) + if (escaped) { + escaped = false + } else if (char === '\\') { + escaped = true + } else if ( + (state === 'single' && char === "'") || + (state === 'double' && char === '"') || + (state === 'template' && char === '`') + ) { + state = 'code' + } + } + + return output.join('') +} + +function hasExecutableModuleReference(commentFree: string, executable: string, pattern: RegExp): boolean { + return [...commentFree.matchAll(pattern)].some((match) => { + const start = match.index ?? 0 + const executableMatch = executable.slice(start, start + match[0].length) + return /\b(?:require|from|import)\b/.test(executableMatch) + }) +} + function isFocusedNodeTestAssertion(content: string): boolean { - const hasTestCall = /\b(?:test|it)(?:\.(?:only|skip|todo))?\s*\(/.test(content) - const hasAssertionCall = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/.test(content) - return /\bnode:test\b/.test(content) && /\bnode:assert\/strict\b/.test(content) && hasTestCall && hasAssertionCall + const commentFree = scannedJavaScriptText(content, false) + const executable = scannedJavaScriptText(content, true) + const hasNodeTestImport = hasExecutableModuleReference( + commentFree, + executable, + /\brequire\s*\(\s*['"]node:test['"]\s*\)|\bfrom\s*['"]node:test['"]|\bimport\s*['"]node:test['"]/g, + ) + const hasNodeAssertImport = hasExecutableModuleReference( + commentFree, + executable, + /\brequire\s*\(\s*['"]node:assert\/strict['"]\s*\)|\bfrom\s*['"]node:assert\/strict['"]|\bimport\s*['"]node:assert\/strict['"]/g, + ) + const hasTestCall = /\b(?:test|it)(?:\.only)?\s*\(/.test(executable) + const hasAssertionCall = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/.test(executable) + return hasNodeTestImport && hasNodeAssertImport && hasTestCall && hasAssertionCall } function isInvalidNodeTestScript(script: string): boolean { @@ -551,6 +644,9 @@ function validatePlanAgainstPrompt(plan: WorkPackageExecutionPlan, prompt: strin if (isUnsafePackageScript(buildScript)) { throw new Error('The generated build script includes unsafe shell behavior.') } + if (!plan.files.some((file) => isJavaScriptFile(file.path))) { + throw new Error('Static build validation requires at least one checkable JavaScript source file.') + } } if (hasCommand(plan.commands, 'npm run lint')) { From cd1fa8af74ac1e7d6a2c80135fd156c2aeebe413 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:58:16 +0800 Subject: [PATCH 7/9] Handle regex literals in focused test validation --- docs/acp-zed-connector.md | 7 +-- web/__tests__/work-package-executor.test.ts | 2 + web/worker/work-package-executor.ts | 49 ++++++++++++++++++++- 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/docs/acp-zed-connector.md b/docs/acp-zed-connector.md index 960610e..5985238 100644 --- a/docs/acp-zed-connector.md +++ b/docs/acp-zed-connector.md @@ -83,9 +83,10 @@ acceptable. - Forge does not receive detailed token usage from ACP. - Tool calls from the underlying coding agent are not exposed as Forge runtime MCP grants. -- ACP package execution is separately opt-in. When enabled, it relies on the - package attempt working directory and Forge's execution JSON path guards; it is - not an OS sandbox or a general live tool grant. +- ACP package execution is enabled after task approval unless the operator sets + `FORGE_ACP_WORK_PACKAGE_EXECUTION=0`. When active, it relies on the package + attempt working directory and Forge's execution JSON path guards; it is not an + OS sandbox or a general live tool grant. - If a runtime does not expose model selection through ACP, Forge stores the selected model on the provider record but cannot force the local runtime to use it. diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 429b8e1..350cf7e 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -1744,9 +1744,11 @@ describe('executeWorkPackage', () => { content: [ 'const test = require("node:test");', 'const assert = require("node:assert/strict");', + "const quotePattern = /['\"]/;", 'test("persists with a localStorage stub", () => {', ' const localStorageStub = new Map([["tasks", "[]"]]);', ' const input = { placeholder: "Add task" };', + ' assert.equal(quotePattern.test("\\\""), true);', ' assert.equal(localStorageStub.get("tasks"), "[]");', ' assert.equal(input.placeholder, "Add task");', '});', diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index a75735a..8252b9b 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -484,8 +484,10 @@ function isNoOpScript(script: string): boolean { function scannedJavaScriptText(content: string, stripStrings: boolean): string { const output = content.split('') - let state: 'code' | 'single' | 'double' | 'template' | 'line-comment' | 'block-comment' = 'code' + let state: 'code' | 'single' | 'double' | 'template' | 'regex' | 'line-comment' | 'block-comment' = 'code' let escaped = false + let regexAllowed = true + let regexCharacterClass = false const blank = (index: number) => { if (output[index] !== '\n' && output[index] !== '\r') output[index] = ' ' @@ -508,13 +510,37 @@ function scannedJavaScriptText(content: string, stripStrings: boolean): string { state = 'block-comment' } else if (char === "'") { if (stripStrings) blank(index) + escaped = false state = 'single' } else if (char === '"') { if (stripStrings) blank(index) + escaped = false state = 'double' } else if (char === '`') { if (stripStrings) blank(index) + escaped = false state = 'template' + } else if (char === '/' && regexAllowed) { + blank(index) + escaped = false + regexCharacterClass = false + state = 'regex' + } else if (/\s/.test(char)) { + continue + } else if (/[A-Za-z_$]/.test(char)) { + let end = index + 1 + while (end < content.length && /[\w$]/.test(content[end])) end += 1 + const word = content.slice(index, end) + regexAllowed = /^(?:await|case|delete|else|in|instanceof|of|return|throw|typeof|void|yield)$/.test(word) + index = end - 1 + } else if (/\d/.test(char)) { + regexAllowed = false + } else if (char === ')' || char === ']' || char === '}') { + regexAllowed = false + } else if (char === '.' || (char === '+' && next === '+') || (char === '-' && next === '-')) { + regexAllowed = false + } else { + regexAllowed = true } continue } @@ -537,6 +563,26 @@ function scannedJavaScriptText(content: string, stripStrings: boolean): string { continue } + if (state === 'regex') { + blank(index) + if (char === '\n' || char === '\r') { + state = 'code' + regexAllowed = true + } else if (escaped) { + escaped = false + } else if (char === '\\') { + escaped = true + } else if (char === '[') { + regexCharacterClass = true + } else if (char === ']') { + regexCharacterClass = false + } else if (char === '/' && !regexCharacterClass) { + state = 'code' + regexAllowed = false + } + continue + } + if (stripStrings) blank(index) if (escaped) { escaped = false @@ -548,6 +594,7 @@ function scannedJavaScriptText(content: string, stripStrings: boolean): string { (state === 'template' && char === '`') ) { state = 'code' + regexAllowed = false } } From 4fec15604286fb0a53c186539080c17cc9751da6 Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:08:33 +0800 Subject: [PATCH 8/9] Require assertions inside registered tests --- web/__tests__/work-package-executor.test.ts | 2 +- web/worker/work-package-executor.ts | 63 ++++++++++++++++++++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 350cf7e..769d81b 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -2019,7 +2019,7 @@ describe('executeWorkPackage', () => { }, { path: 'tracker.test.js', - content: 'const test = require("node:test"); const assert = require("node:assert/strict"); test("adds item", () => console.log("ok"));\n', + content: 'const test = require("node:test"); const assert = require("node:assert/strict"); assert.equal(1, 1); test("adds item", () => {});\n', }, { path: 'build-check.js', diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index 8252b9b..a88d15d 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -609,6 +609,65 @@ function hasExecutableModuleReference(commentFree: string, executable: string, p }) } +function findClosingParenthesis(content: string, openingIndex: number): number { + let depth = 0 + for (let index = openingIndex; index < content.length; index += 1) { + if (content[index] === '(') depth += 1 + if (content[index] !== ')') continue + depth -= 1 + if (depth === 0) return index + } + return -1 +} + +function splitTopLevelArguments(content: string): string[] { + const argumentsList: string[] = [] + let parentheses = 0 + let brackets = 0 + let braces = 0 + let start = 0 + + for (let index = 0; index < content.length; index += 1) { + const char = content[index] + if (char === '(') parentheses += 1 + else if (char === ')') parentheses -= 1 + else if (char === '[') brackets += 1 + else if (char === ']') brackets -= 1 + else if (char === '{') braces += 1 + else if (char === '}') braces -= 1 + else if (char === ',' && parentheses === 0 && brackets === 0 && braces === 0) { + argumentsList.push(content.slice(start, index)) + start = index + 1 + } + } + argumentsList.push(content.slice(start)) + return argumentsList +} + +function hasAssertionInRegisteredTest(executable: string): boolean { + const assertionPattern = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/ + for (const match of executable.matchAll(/\b(?:test|it)(?:\.only)?\s*\(/g)) { + const openingIndex = (match.index ?? 0) + match[0].lastIndexOf('(') + const closingIndex = findClosingParenthesis(executable, openingIndex) + if (closingIndex < 0) continue + + const callArguments = splitTopLevelArguments(executable.slice(openingIndex + 1, closingIndex)) + .filter((argument) => argument.trim() !== '') + const callback = callArguments.at(-1) ?? '' + const arrowIndex = callback.indexOf('=>') + const functionIndex = callback.search(/\bfunction\b/) + const callbackBodyIndex = [ + arrowIndex >= 0 ? arrowIndex + 2 : -1, + functionIndex >= 0 ? functionIndex + 'function'.length : -1, + ].filter((index) => index >= 0).sort((left, right) => left - right)[0] + + if (callbackBodyIndex !== undefined && assertionPattern.test(callback.slice(callbackBodyIndex))) { + return true + } + } + return false +} + function isFocusedNodeTestAssertion(content: string): boolean { const commentFree = scannedJavaScriptText(content, false) const executable = scannedJavaScriptText(content, true) @@ -622,9 +681,7 @@ function isFocusedNodeTestAssertion(content: string): boolean { executable, /\brequire\s*\(\s*['"]node:assert\/strict['"]\s*\)|\bfrom\s*['"]node:assert\/strict['"]|\bimport\s*['"]node:assert\/strict['"]/g, ) - const hasTestCall = /\b(?:test|it)(?:\.only)?\s*\(/.test(executable) - const hasAssertionCall = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/.test(executable) - return hasNodeTestImport && hasNodeAssertImport && hasTestCall && hasAssertionCall + return hasNodeTestImport && hasNodeAssertImport && hasAssertionInRegisteredTest(executable) } function isInvalidNodeTestScript(script: string): boolean { From bfa2c407148522b00ba8e3a58231e109569aa1dd Mon Sep 17 00:00:00 2001 From: Joncallim <64296013+Joncallim@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:16:00 +0800 Subject: [PATCH 9/9] Reject custom runner test calls --- web/__tests__/work-package-executor.test.ts | 10 +++++++++- web/worker/work-package-executor.ts | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/web/__tests__/work-package-executor.test.ts b/web/__tests__/work-package-executor.test.ts index 769d81b..71940b5 100644 --- a/web/__tests__/work-package-executor.test.ts +++ b/web/__tests__/work-package-executor.test.ts @@ -2019,7 +2019,15 @@ describe('executeWorkPackage', () => { }, { path: 'tracker.test.js', - content: 'const test = require("node:test"); const assert = require("node:assert/strict"); assert.equal(1, 1); test("adds item", () => {});\n', + content: [ + 'const test = require("node:test");', + 'const assert = require("node:assert/strict");', + 'const runner = { test: (callback) => callback() };', + 'assert.equal(1, 1);', + 'runner.test(() => assert.equal(1, 1));', + 'test("adds item", () => {});', + '', + ].join('\n'), }, { path: 'build-check.js', diff --git a/web/worker/work-package-executor.ts b/web/worker/work-package-executor.ts index a88d15d..f08b3d8 100644 --- a/web/worker/work-package-executor.ts +++ b/web/worker/work-package-executor.ts @@ -646,7 +646,7 @@ function splitTopLevelArguments(content: string): string[] { function hasAssertionInRegisteredTest(executable: string): boolean { const assertionPattern = /\b(?:assert(?:\.[A-Za-z_$][\w$]*)?|ok|equal|strictEqual|deepEqual|deepStrictEqual|throws|rejects)\s*\(/ - for (const match of executable.matchAll(/\b(?:test|it)(?:\.only)?\s*\(/g)) { + for (const match of executable.matchAll(/(?:^|[^\w$.])(?:test|it)(?:\.only)?\s*\(/g)) { const openingIndex = (match.index ?? 0) + match[0].lastIndexOf('(') const closingIndex = findClosingParenthesis(executable, openingIndex) if (closingIndex < 0) continue