fix(subagent-runner): drop spurious 10s STUCK warning on session.prompt
The phaseWatchdog at 10s fired "STUCK phase=session.prompt" on every healthy LLM call longer than 10 seconds. Verified via strace on the running dogfood sf: bytes were actively flowing on the TLS socket (fd 29) to the LLM provider while STUCK was being logged — the session.prompt was never actually stuck, the watchdog was just diagnostic-only and oblivious to stream activity. The noOutputTimeoutMs watchdog (set to 60s for triage in commitd80060fec) is the actual kill mechanism. It is already event-aware: every meaningful subagent event resets the timer via armNoOutputTimer + isMeaningfulSubagentOutputEvent. The 10s STUCK warning was added in commit67e5ac9dbas investigation infrastructure for the sf-mp8e02m1-zpk903 family of bugs, but now it is just noise that makes legitimate 30-200s LLM responses look broken. Keeps the 10s STUCK watchdog for the three setup phases (resourceLoader.reload, createAgentSession, bindExtensions) where 10s of silence is a real hang signal — those phases normally run in sub-second. Also includes: - biome.json: bump $schema URL from 2.4.14 to 2.4.15 to match the current biome CLI (clears the deserialize warning) - scripts/check-test-imports.{,test.}mjs: format + drop a useless regex escape that biome flagged in landed code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Mikael Hugo
parent
c09cad1cf0
commit
f55d490e1d
4 changed files with 265 additions and 76 deletions
|
|
@ -1,5 +1,5 @@
|
|||
{
|
||||
"$schema": "https://biomejs.dev/schemas/2.4.14/schema.json",
|
||||
"$schema": "https://biomejs.dev/schemas/2.4.15/schema.json",
|
||||
"vcs": {
|
||||
"enabled": true,
|
||||
"clientKind": "git",
|
||||
|
|
|
|||
|
|
@ -327,7 +327,14 @@ export async function runSubagent(
|
|||
`[subagent:${name}] phase=session.prompt-entered taskLen=${promptTask.length} timeoutMs=${timeoutMs} noOutputMs=${noOutputTimeoutMs}\n`,
|
||||
);
|
||||
const promptEnteredAt = phaseStartedAt();
|
||||
const promptWatchdog = phaseWatchdog("session.prompt", promptEnteredAt);
|
||||
// Note: no phaseWatchdog for session.prompt. The 10s STUCK warning fired
|
||||
// spuriously on healthy 30–60s LLM responses (verified by strace: bytes
|
||||
// were flowing on the TLS socket the whole time). The noOutputTimeoutMs
|
||||
// watchdog below is the event-aware replacement — it resets on every
|
||||
// meaningful subagent event (see armNoOutputTimer / isMeaningfulSubagent-
|
||||
// OutputEvent) and aborts only after a true silence window. Keep the
|
||||
// STUCK watchdog for setup phases (resourceLoader.reload, createAgent-
|
||||
// Session, bindExtensions) where 10s of silence really is a hang signal.
|
||||
|
||||
try {
|
||||
if (timeoutMs === 0 && noOutputTimeoutMs === 0 && !options?.signal) {
|
||||
|
|
@ -336,7 +343,6 @@ export async function runSubagent(
|
|||
runExtensionHooks: false,
|
||||
});
|
||||
await promptPromise;
|
||||
clearTimeout(promptWatchdog);
|
||||
logPhase("session.prompt-returned", promptEnteredAt);
|
||||
cleanup();
|
||||
return { ok: true, output: extractFinalOutput(), exitCode: 0 };
|
||||
|
|
@ -416,7 +422,6 @@ export async function runSubagent(
|
|||
});
|
||||
competitors.unshift(
|
||||
promptPromise.then(() => {
|
||||
clearTimeout(promptWatchdog);
|
||||
logPhase("session.prompt-returned", promptEnteredAt);
|
||||
return {} as RaceResult;
|
||||
}),
|
||||
|
|
@ -425,7 +430,6 @@ export async function runSubagent(
|
|||
const result = await Promise.race(competitors);
|
||||
|
||||
cleanup();
|
||||
clearTimeout(promptWatchdog);
|
||||
|
||||
if (result.timedOut) {
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -30,54 +30,207 @@ const root = resolve(__dirname, "..");
|
|||
// ── Helpers ───────────────────────────────────────────────────────────────────
|
||||
|
||||
const NODE_BUILTINS = new Set([
|
||||
"process", "require", "module", "exports", "console", "global", "Buffer",
|
||||
"setTimeout", "setInterval", "clearTimeout", "clearInterval", "queueMicrotask",
|
||||
"Array", "Object", "String", "Number", "Boolean", "Symbol", "BigInt",
|
||||
"Map", "Set", "WeakMap", "WeakSet", "Promise", "Math", "JSON", "Date",
|
||||
"RegExp", "Error", "EvalError", "RangeError", "ReferenceError", "SyntaxError",
|
||||
"TypeError", "URIError", "AggregateError", "AbortController", "AbortSignal",
|
||||
"URL", "URLSearchParams", "TextEncoder", "TextDecoder", "TransformStream",
|
||||
"ReadableStream", "WritableStream", "Blob", "File", "FormData", "Headers",
|
||||
"Request", "Response", "fetch", "WebSocket", "EventSource",
|
||||
"setImmediate", "clearImmediate", "structuredClone", "atob", "btoa",
|
||||
"navigator", "window", "document", "location", "history", "gc",
|
||||
"finalizationRegistry", "Intl", "Reflect", "Proxy",
|
||||
"process",
|
||||
"require",
|
||||
"module",
|
||||
"exports",
|
||||
"console",
|
||||
"global",
|
||||
"Buffer",
|
||||
"setTimeout",
|
||||
"setInterval",
|
||||
"clearTimeout",
|
||||
"clearInterval",
|
||||
"queueMicrotask",
|
||||
"Array",
|
||||
"Object",
|
||||
"String",
|
||||
"Number",
|
||||
"Boolean",
|
||||
"Symbol",
|
||||
"BigInt",
|
||||
"Map",
|
||||
"Set",
|
||||
"WeakMap",
|
||||
"WeakSet",
|
||||
"Promise",
|
||||
"Math",
|
||||
"JSON",
|
||||
"Date",
|
||||
"RegExp",
|
||||
"Error",
|
||||
"EvalError",
|
||||
"RangeError",
|
||||
"ReferenceError",
|
||||
"SyntaxError",
|
||||
"TypeError",
|
||||
"URIError",
|
||||
"AggregateError",
|
||||
"AbortController",
|
||||
"AbortSignal",
|
||||
"URL",
|
||||
"URLSearchParams",
|
||||
"TextEncoder",
|
||||
"TextDecoder",
|
||||
"TransformStream",
|
||||
"ReadableStream",
|
||||
"WritableStream",
|
||||
"Blob",
|
||||
"File",
|
||||
"FormData",
|
||||
"Headers",
|
||||
"Request",
|
||||
"Response",
|
||||
"fetch",
|
||||
"WebSocket",
|
||||
"EventSource",
|
||||
"setImmediate",
|
||||
"clearImmediate",
|
||||
"structuredClone",
|
||||
"atob",
|
||||
"btoa",
|
||||
"navigator",
|
||||
"window",
|
||||
"document",
|
||||
"location",
|
||||
"history",
|
||||
"gc",
|
||||
"finalizationRegistry",
|
||||
"Intl",
|
||||
"Reflect",
|
||||
"Proxy",
|
||||
// Node.js globals (not ESM builtins but always available)
|
||||
"__dirname", "__filename", "import", "imports",
|
||||
"__dirname",
|
||||
"__filename",
|
||||
"import",
|
||||
"imports",
|
||||
]);
|
||||
|
||||
// Node.js module exports commonly used in tests without explicit import
|
||||
const NODE_MODULE_GLOBALS = new Set([
|
||||
"assert", "assertEq", "deepEqual", "strictEqual",
|
||||
"path", "fs", "os", "url", "querystring", "crypto", "buffer",
|
||||
"stream", "util", "events", "querystring", "stringDecoder",
|
||||
"readline", "tty", "domain", "constants", " timers", "async_hooks",
|
||||
"assert",
|
||||
"assertEq",
|
||||
"deepEqual",
|
||||
"strictEqual",
|
||||
"path",
|
||||
"fs",
|
||||
"os",
|
||||
"url",
|
||||
"querystring",
|
||||
"crypto",
|
||||
"buffer",
|
||||
"stream",
|
||||
"util",
|
||||
"events",
|
||||
"querystring",
|
||||
"stringDecoder",
|
||||
"readline",
|
||||
"tty",
|
||||
"domain",
|
||||
"constants",
|
||||
" timers",
|
||||
"async_hooks",
|
||||
]);
|
||||
|
||||
// JavaScript reserved words and keywords that should never be flagged as undeclared
|
||||
const JS_KEYWORDS = new Set([
|
||||
// Reserved words
|
||||
"break", "case", "catch", "continue", "debugger", "default", "delete",
|
||||
"do", "else", "finally", "for", "function", "if", "in", "instanceof",
|
||||
"new", "return", "switch", "this", "throw", "try", "typeof", "var",
|
||||
"void", "while", "with", "class", "const", "enum", "export", "extends",
|
||||
"import", "super", "implements", "interface", "let", "package", "private",
|
||||
"protected", "public", "static", "yield", "async", "await", "of",
|
||||
"get", "set", "target", "from", "as",
|
||||
"break",
|
||||
"case",
|
||||
"catch",
|
||||
"continue",
|
||||
"debugger",
|
||||
"default",
|
||||
"delete",
|
||||
"do",
|
||||
"else",
|
||||
"finally",
|
||||
"for",
|
||||
"function",
|
||||
"if",
|
||||
"in",
|
||||
"instanceof",
|
||||
"new",
|
||||
"return",
|
||||
"switch",
|
||||
"this",
|
||||
"throw",
|
||||
"try",
|
||||
"typeof",
|
||||
"var",
|
||||
"void",
|
||||
"while",
|
||||
"with",
|
||||
"class",
|
||||
"const",
|
||||
"enum",
|
||||
"export",
|
||||
"extends",
|
||||
"import",
|
||||
"super",
|
||||
"implements",
|
||||
"interface",
|
||||
"let",
|
||||
"package",
|
||||
"private",
|
||||
"protected",
|
||||
"public",
|
||||
"static",
|
||||
"yield",
|
||||
"async",
|
||||
"await",
|
||||
"of",
|
||||
"get",
|
||||
"set",
|
||||
"target",
|
||||
"from",
|
||||
"as",
|
||||
]);
|
||||
|
||||
// Common test framework globals that are always available in vitest/jest
|
||||
const TEST_GLOBALS = new Set([
|
||||
"describe", "it", "test", "expect", "beforeEach", "afterEach",
|
||||
"beforeAll", "afterAll", "suite", "spec", "context", "given",
|
||||
"when", "then", "and", "describe.skip", "it.skip", "test.skip",
|
||||
"describe.only", "it.only", "test.only", "describe.each", "it.each",
|
||||
"test.each", "describe.concurrent", "it.concurrent", "test.concurrent",
|
||||
"vi", "vitest", "spyOn", "jest",
|
||||
"describe",
|
||||
"it",
|
||||
"test",
|
||||
"expect",
|
||||
"beforeEach",
|
||||
"afterEach",
|
||||
"beforeAll",
|
||||
"afterAll",
|
||||
"suite",
|
||||
"spec",
|
||||
"context",
|
||||
"given",
|
||||
"when",
|
||||
"then",
|
||||
"and",
|
||||
"describe.skip",
|
||||
"it.skip",
|
||||
"test.skip",
|
||||
"describe.only",
|
||||
"it.only",
|
||||
"test.only",
|
||||
"describe.each",
|
||||
"it.each",
|
||||
"test.each",
|
||||
"describe.concurrent",
|
||||
"it.concurrent",
|
||||
"test.concurrent",
|
||||
"vi",
|
||||
"vitest",
|
||||
"spyOn",
|
||||
"jest",
|
||||
]);
|
||||
|
||||
// Boolean and literal values that always exist
|
||||
const JS_LITERALS = new Set(["true", "false", "null", "undefined", "NaN", "Infinity"]);
|
||||
const JS_LITERALS = new Set([
|
||||
"true",
|
||||
"false",
|
||||
"null",
|
||||
"undefined",
|
||||
"NaN",
|
||||
"Infinity",
|
||||
]);
|
||||
|
||||
/**
|
||||
* Parse the statically-imported identifiers from the top of a file.
|
||||
|
|
@ -92,7 +245,7 @@ function parseImports(source) {
|
|||
const stripped = source
|
||||
.replace(/'[^']*'/g, "'_'")
|
||||
.replace(/"[^"]*"/g, '"_"')
|
||||
.replace(/`[^`]*`/g, '``_`')
|
||||
.replace(/`[^`]*`/g, "``_`")
|
||||
.replace(/\/\/[^\n]*/g, "")
|
||||
.replace(/\/\*[\s\S]*?\*\//g, "");
|
||||
|
||||
|
|
@ -107,16 +260,19 @@ function parseImports(source) {
|
|||
const namedRe = /import\s+\{([^}]+)\}\s+from\s+["'][^"']+["']/g;
|
||||
for (const m of stripped.matchAll(namedRe)) {
|
||||
const content = m[1];
|
||||
const items = content.split(",").map((s) => {
|
||||
s = s.trim();
|
||||
if (s.includes(" as ")) {
|
||||
s = s.split(" as ").pop().trim();
|
||||
}
|
||||
if (s.startsWith("type ")) {
|
||||
s = s.replace("type ", "").trim();
|
||||
}
|
||||
return s.replace(/\s+/g, "");
|
||||
}).filter(Boolean);
|
||||
const items = content
|
||||
.split(",")
|
||||
.map((s) => {
|
||||
s = s.trim();
|
||||
if (s.includes(" as ")) {
|
||||
s = s.split(" as ").pop().trim();
|
||||
}
|
||||
if (s.startsWith("type ")) {
|
||||
s = s.replace("type ", "").trim();
|
||||
}
|
||||
return s.replace(/\s+/g, "");
|
||||
})
|
||||
.filter(Boolean);
|
||||
items.forEach((id) => named.add(id));
|
||||
}
|
||||
|
||||
|
|
@ -143,7 +299,7 @@ function collectReferences(source) {
|
|||
const stripped = source
|
||||
.replace(/'[^']*'/g, "'_'")
|
||||
.replace(/"[^"]*"/g, '"_"')
|
||||
.replace(/`[^`]*`/g, '``_`')
|
||||
.replace(/`[^`]*`/g, "``_`")
|
||||
.replace(/\/\/[^\n]*/g, "")
|
||||
.replace(/\/\*[\s\S]*?\*\//g, "");
|
||||
|
||||
|
|
@ -154,7 +310,7 @@ function collectReferences(source) {
|
|||
|
||||
// Replace member expressions and calls with placeholders to isolate identifiers
|
||||
const cleaned = stripped
|
||||
.replace(/\b[a-zA-Z_$][\w$]*\.[a-zA-Z_$][\w$]*(?=\s*[\[.\(])/g, "_")
|
||||
.replace(/\b[a-zA-Z_$][\w$]*\.[a-zA-Z_$][\w$]*(?=\s*[[.(])/g, "_")
|
||||
.replace(/\[[^\]]+\]/g, "[_]");
|
||||
|
||||
// Now collect bare identifiers (word boundaries)
|
||||
|
|
@ -208,13 +364,22 @@ function checkFile(filePath, source) {
|
|||
// - Variable names assigned in try/catch or before import
|
||||
// - Playwright/MCP test helpers
|
||||
const COMMON_TEST_LOCALS = new Set([
|
||||
"canLaunchChromium", "isAvailable", "hasSupport", "isSupported",
|
||||
"describeOrSkip", "itOrSkip", "skipIf", "runIf",
|
||||
"jiti", "interopDefault", "debug",
|
||||
"canLaunchChromium",
|
||||
"isAvailable",
|
||||
"hasSupport",
|
||||
"isSupported",
|
||||
"describeOrSkip",
|
||||
"itOrSkip",
|
||||
"skipIf",
|
||||
"runIf",
|
||||
"jiti",
|
||||
"interopDefault",
|
||||
"debug",
|
||||
]);
|
||||
|
||||
const filteredUndeclared = [...undeclared].filter(
|
||||
(id) => !COMMON_TEST_LOCALS.has(id) &&
|
||||
(id) =>
|
||||
!COMMON_TEST_LOCALS.has(id) &&
|
||||
!id.match(/^(can|has|is|should|will)[A-Z]/) &&
|
||||
!id.match(/^(testBrowser|chromiumInstance|pageInstance)$/),
|
||||
);
|
||||
|
|
@ -251,9 +416,12 @@ function findTestFiles(dir) {
|
|||
}
|
||||
results.push(...findTestFiles(full));
|
||||
} else if (
|
||||
(entry.name.endsWith(".test.js") || entry.name.endsWith(".test.mjs") ||
|
||||
entry.name.endsWith(".test.ts") || entry.name.endsWith(".spec.js") ||
|
||||
entry.name.endsWith(".spec.mjs") || entry.name.endsWith(".spec.ts")) &&
|
||||
(entry.name.endsWith(".test.js") ||
|
||||
entry.name.endsWith(".test.mjs") ||
|
||||
entry.name.endsWith(".test.ts") ||
|
||||
entry.name.endsWith(".spec.js") ||
|
||||
entry.name.endsWith(".spec.mjs") ||
|
||||
entry.name.endsWith(".spec.ts")) &&
|
||||
!entry.name.includes("node_modules")
|
||||
) {
|
||||
results.push(full);
|
||||
|
|
@ -271,9 +439,7 @@ const verbose = args.includes("--verbose");
|
|||
|
||||
// If specific paths are provided, use those instead of scanning
|
||||
// Filter out only flag args, keep everything else as a file path
|
||||
const explicitPaths = args.filter(
|
||||
(a) => !a.startsWith("--"),
|
||||
);
|
||||
const explicitPaths = args.filter((a) => !a.startsWith("--"));
|
||||
|
||||
let allFiles = [];
|
||||
if (explicitPaths.length > 0) {
|
||||
|
|
@ -293,7 +459,9 @@ if (explicitPaths.length > 0) {
|
|||
process.exit(1);
|
||||
}
|
||||
if (verbose) {
|
||||
console.error(`[check-test-imports] explicit mode: ${allFiles.length} file(s)`);
|
||||
console.error(
|
||||
`[check-test-imports] explicit mode: ${allFiles.length} file(s)`,
|
||||
);
|
||||
}
|
||||
} else {
|
||||
const scanDirs = [
|
||||
|
|
@ -341,4 +509,4 @@ if (jsonOut) {
|
|||
}
|
||||
}
|
||||
|
||||
process.exit(issues.length > 0 ? 1 : 0);
|
||||
process.exit(issues.length > 0 ? 1 : 0);
|
||||
|
|
|
|||
|
|
@ -42,14 +42,22 @@ function run(args) {
|
|||
|
||||
/** Create a temp test file, return absolute path. */
|
||||
function makeTempFile(id, content) {
|
||||
try { mkdirSync(tmpDir, { recursive: true }); } catch { /* */ }
|
||||
try {
|
||||
mkdirSync(tmpDir, { recursive: true });
|
||||
} catch {
|
||||
/* */
|
||||
}
|
||||
const file = resolve(tmpDir, `regression-${id}.test.mjs`);
|
||||
writeFileSync(file, content);
|
||||
return file;
|
||||
}
|
||||
|
||||
function cleanupTempDir() {
|
||||
try { rmdirSync(tmpDir); } catch { /* ignore */ }
|
||||
try {
|
||||
rmdirSync(tmpDir);
|
||||
} catch {
|
||||
/* ignore */
|
||||
}
|
||||
}
|
||||
|
||||
// ── Tests ─────────────────────────────────────────────────────────────────────
|
||||
|
|
@ -89,7 +97,9 @@ if (!existsSync(realFile)) {
|
|||
|
||||
// ── Test 2: Detects undeclared identifier in anti-pattern file ───────────────
|
||||
console.log("\n[2] Detects undeclared identifier in anti-pattern file");
|
||||
const file2 = makeTempFile("regression-02", `
|
||||
const file2 = makeTempFile(
|
||||
"regression-02",
|
||||
`
|
||||
// Regression test — generated by check-test-imports.test.mjs
|
||||
// DO NOT COMMIT
|
||||
import { describe, it, expect } from "vitest";
|
||||
|
|
@ -100,7 +110,8 @@ describe("test", () => {
|
|||
undeclaredFn();
|
||||
});
|
||||
});
|
||||
`);
|
||||
`,
|
||||
);
|
||||
const result2 = run([file2, "--json"]);
|
||||
try {
|
||||
const parsed = JSON.parse(result2.stdout);
|
||||
|
|
@ -118,7 +129,9 @@ try {
|
|||
|
||||
// ── Test 3: Clean file with namespace import passes ─────────────────────────
|
||||
console.log("\n[3] Clean file with namespace import + local vars passes");
|
||||
const file3 = makeTempFile("regression-03", `
|
||||
const file3 = makeTempFile(
|
||||
"regression-03",
|
||||
`
|
||||
// Clean: namespace import + only local variables
|
||||
// DO NOT COMMIT
|
||||
import { describe, it, expect } from "vitest";
|
||||
|
|
@ -131,21 +144,21 @@ describe("test", () => {
|
|||
expect(myLocalVar).toBeDefined();
|
||||
});
|
||||
});
|
||||
`);
|
||||
`,
|
||||
);
|
||||
const result3 = run([file3, "--json"]);
|
||||
try {
|
||||
const parsed = JSON.parse(result3.stdout);
|
||||
assert(
|
||||
parsed.count === 0,
|
||||
`Clean file: count ${parsed.count}`,
|
||||
);
|
||||
assert(parsed.count === 0, `Clean file: count ${parsed.count}`);
|
||||
} catch {
|
||||
assert(false, `Script should not throw on clean file`);
|
||||
}
|
||||
|
||||
// ── Test 4: Local const/let declarations not flagged ───────────────────────
|
||||
console.log("\n[4] Local const/let declarations not flagged");
|
||||
const file4 = makeTempFile("regression-04", `
|
||||
const file4 = makeTempFile(
|
||||
"regression-04",
|
||||
`
|
||||
// Local variable declarations should not be flagged
|
||||
// DO NOT COMMIT
|
||||
import { describe, it, expect } from "vitest";
|
||||
|
|
@ -159,7 +172,8 @@ describe("test", () => {
|
|||
expect(myLocalVar).toBeDefined();
|
||||
});
|
||||
});
|
||||
`);
|
||||
`,
|
||||
);
|
||||
const result4 = run([file4, "--json"]);
|
||||
try {
|
||||
const parsed = JSON.parse(result4.stdout);
|
||||
|
|
@ -173,7 +187,9 @@ try {
|
|||
|
||||
// ── Test 5: Short lowercase variables not flagged ───────────────────────────
|
||||
console.log("\n[5] Short lowercase variables not flagged");
|
||||
const file5 = makeTempFile("regression-05", `
|
||||
const file5 = makeTempFile(
|
||||
"regression-05",
|
||||
`
|
||||
// Short lowercase vars like i, fn are common test locals
|
||||
// DO NOT COMMIT
|
||||
import { describe, it, expect } from "vitest";
|
||||
|
|
@ -186,7 +202,8 @@ describe("test", () => {
|
|||
expect(j).toBeGreaterThan(0);
|
||||
});
|
||||
});
|
||||
`);
|
||||
`,
|
||||
);
|
||||
const result5 = run([file5, "--json"]);
|
||||
try {
|
||||
const parsed = JSON.parse(result5.stdout);
|
||||
|
|
@ -203,4 +220,4 @@ try {
|
|||
console.log(`\n${"-".repeat(50)}`);
|
||||
console.log(`Results: ${passed} passed, ${failed} failed`);
|
||||
cleanupTempDir();
|
||||
process.exit(failed > 0 ? 1 : 0);
|
||||
process.exit(failed > 0 ? 1 : 0);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue