From 1a968f2eb555f59278c7d659d53ca0b201ed556b Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sat, 15 Aug 2026 20:21:46 -0400 Subject: [PATCH 1/3] fix(chat): every reveal of the chat view handles its own rejection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #76, which fixed the two instances in the code it introduced and left the older ones alone to keep that review diff readable. `executeCommand` returns a Thenable, so a bare call in a void context turns any rejection into an unhandled promise rejection in the extension host — attributed to nothing, which is the part that makes it useless. SIX SITES, NOT SEVEN. The brief for this change listed the `levelcode.ai.focus` registration as needing a `return`. It does not: `() => vscode.commands.executeCommand(…)` is a concise arrow body, so it already returns the thenable and VS Code already reports its failures. Left alone. The other six are all BACKGROUND reveals — addSelection, addContext, resumeSession, the editor panel's onDidDispose, the sessions view's newSession, and handleLaunch. In each the accompanying work has already succeeded by the time the reveal runs, so failing that work because the panel would not come forward would be worse than the panel not coming forward. They route through one `focusChatView(why)` helper that logs and never rejects. Logged, not swallowed: the complaint was "attributed to nothing", so `.catch(() => {})` would answer the letter of it and none of the substance. `why` names the caller in the log. ONE GUARD FOR THE WHOLE CLASS. Rather than six assertions naming six functions, the test scans every occurrence of the call and accepts only returned / awaited / concise-arrow / helper-wrapped / inline-handled forms. That covers the seventh site and the eighth nobody has written yet — which matters, because this pattern reached six copies precisely because nothing was watching for it. Verified non-vacuous against develop: the same scan reports exactly the six bare sites there (lines 423, 468, 1055, 2298, 2435, 2531) and zero here. Bypasses, each reverted and confirmed to fail: - a single site returned to a bare call; the switch-case site returned to a bare call - the focus command wrapped in a block body so it stops returning (proves the guard protects the site that was already correct) - the helper swallowing silently; the log dropping its caller attribution One assertion deliberately relaxed after bypassing it: pinning `.then(undefined, …)` over `.catch(…)` failed a refactor with no behavioural difference. It now accepts either and still fails when the handler stops logging. 23 tests in chatSurface, 32 suites green. --- extensions/levelcode-ai/extension.js | 34 +++++++++++--- .../levelcode-ai/test/chatSurface.test.js | 46 ++++++++++++++++++- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 9ddfabf..cecb2f3 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -416,11 +416,33 @@ function captureSelection() { }; } +/** + * Reveal the chat view for its side effect only, and never reject. + * + * `executeCommand` returns a Thenable, so a bare call in a void context turns any rejection into an + * unhandled promise rejection in the extension host — noisy, and attributed to nothing in particular, + * which is the part that makes it useless. + * + * Every caller here is a BACKGROUND reveal: the work it accompanies (a selection added, a session + * resumed, a login launched) has already succeeded by the time this runs. Failing that work because + * the panel would not come forward would be worse than the panel not coming forward. + * + * Logged, never swallowed. `.catch(() => {})` would hide the one failure that is genuinely hard to + * diagnose — a chat surface that silently never appears — so `why` names the caller in the log. + * + * NOT for a command handler whose whole job IS the reveal: `levelcode.ai.focus` returns the thenable + * instead, so VS Code reports the failure to the user who asked for it. See moveChatToSidebar. + */ +function focusChatView(why) { + return Promise.resolve(vscode.commands.executeCommand('levelcodeAi.chat.focus')) + .then(undefined, (e) => dbg('chat.focus.failed', { why, msg: String((e && e.message) || e) })); +} + function addSelection() { const sel = captureSelection(); if (!sel) { vscode.window.showInformationMessage('LevelCode AI: select some code first.'); return; } pendingContext = sel.block; - vscode.commands.executeCommand('levelcodeAi.chat.focus'); + focusChatView('addSelection'); post({ type: 'context', label: sel.label }); } @@ -465,7 +487,7 @@ async function addContext() { } } } - vscode.commands.executeCommand('levelcodeAi.chat.focus'); + focusChatView('addContext'); postContextFiles(); } @@ -1052,7 +1074,7 @@ async function resumeSession(id) { post({ type: 'sessionResumed', id, title: (r.entry && r.entry.title) || 'Session', note: r.note || '', tier: r.plan && r.plan.tier, turns }); postContextFiles(); refreshSessions(); // the resumed session bumps to the top — keep both surfaces current - vscode.commands.executeCommand('levelcodeAi.chat.focus'); + focusChatView('resumeSession'); dbg('sessions.resumed', { id, tier: r.plan && r.plan.tier, restored: agentMessages.length, shown: turns.length }); } @@ -2295,7 +2317,7 @@ async function openChatInEditor(opts) { // resolveWebviewView then makes it live, and without this the chat would have no surface at all. activeWebview = undefined; pendingTranscriptReplay = 'Back in the sidebar'; - vscode.commands.executeCommand('levelcodeAi.chat.focus'); + focusChatView('editorClosed'); } dbg('chat.closedEditor', {}); }); @@ -2432,7 +2454,7 @@ class SessionsViewProvider { // The real session index for this workspace (empty on a fresh install — the view shows its // own empty state). Posted to THIS view's webview, not the chat's. case 'listSessions': view.webview.postMessage({ type: 'sessions', entries: sessionList() }); break; - case 'newSession': newChat(); vscode.commands.executeCommand('levelcodeAi.chat.focus'); break; + case 'newSession': newChat(); focusChatView('sessions.newSession'); break; case 'sessionAction': await handleSessionAction(msg.action, msg.id); break; // Memory tab (§M3): list the recorded outcomes + facts; act on them; open the file. case 'listMemory': { const mm = sessionsManager(); view.webview.postMessage({ type: 'memoryList', items: mm ? mm.memoryItems() : [], facts: mm ? mm.factsList() : [] }); break; } @@ -2528,7 +2550,7 @@ async function postAccount(open) { // second login and no interceptable code ever travels through the custom scheme. async function handleLaunch() { dbg('account.launch', {}); - vscode.commands.executeCommand('levelcodeAi.chat.focus'); + focusChatView('account.launch'); const token = ctx ? await ctx.secrets.get(ACCOUNT_TOKEN_KEY) : null; if (!token) { await accountSignIn(); } } diff --git a/extensions/levelcode-ai/test/chatSurface.test.js b/extensions/levelcode-ai/test/chatSurface.test.js index 0485fd0..90e9af6 100644 --- a/extensions/levelcode-ai/test/chatSurface.test.js +++ b/extensions/levelcode-ai/test/chatSurface.test.js @@ -144,7 +144,9 @@ test('RESTORE: a sidebar that was never resolved is revealed rather than assumed // If the container has not been opened this session, sidebarChatView is undefined — restoring by // writing to it would throw, and doing nothing would leave the chat with no surface at all. const open = fnBody(ext, 'openChatInEditor'); - assert.match(open, /if \(sidebarChatView\) \{[\s\S]*\} else \{[\s\S]*levelcodeAi\.chat\.focus/, + // The reveal now goes through focusChatView() so its rejection cannot go unhandled; what this test + // cares about is unchanged — the else-branch must still reveal the view rather than assume it. + assert.match(open, /if \(sidebarChatView\) \{[\s\S]*\} else \{[\s\S]*focusChatView\(/, 'the never-resolved sidebar case is unhandled'); }); @@ -310,4 +312,46 @@ test('MOVE BACK: there is a button on the tab, and it reuses the dispose hand-ov assert.match(body, /levelcodeAi\.chat\.focus/, 'with no panel open the command must still reveal the chat, not do nothing'); }); +test('FOCUS: no reveal of the chat view is left to reject unhandled', () => { + // One guard for the whole class, rather than six assertions that each name a function. `executeCommand` + // returns a Thenable, so a bare call in a void context makes any rejection an unhandled promise + // rejection in the extension host — attributed to nothing, which is what makes it useless. + // + // Scanning every call site means the NEXT one is covered too. That matters here: this pattern was + // copied into six places over time precisely because nothing was watching for it. + const CALL = "vscode.commands.executeCommand('levelcodeAi.chat.focus')"; + const bare = []; + for (let i = ext.indexOf(CALL); i >= 0; i = ext.indexOf(CALL, i + 1)) { + const before = ext.slice(Math.max(0, i - 40), i); + const after = ext.slice(i + CALL.length, i + CALL.length + 40); + const handled = /\breturn\s+$/.test(before) // returned — a command handler VS Code awaits + || /\bawait\s+$/.test(before) // awaited by a caller that catches + || /=>\s*$/.test(before) // concise arrow body: also a return + || /Promise\.resolve\($/.test(before) // wrapped by focusChatView + || /^\s*\)?\s*\.(then|catch)\(/.test(after); // handled inline + if (!handled) { bare.push('line ' + ext.slice(0, i).split('\n').length); } + } + assert.deepStrictEqual(bare, [], + 'these reveals are fire-and-forget — a rejection becomes an unhandled promise rejection.\n' + + 'Use focusChatView(why) for a background reveal, or `return` it when the command IS the reveal:\n ' + + bare.join('\n ')); +}); + +test('FOCUS: the shared helper logs the failure and names who caused it', () => { + // The whole complaint was "attributed to nothing", so swallowing it silently would answer the letter + // of the review and none of it. A chat surface that never appears, with no trace, is the failure + // that costs an afternoon. + const body = fnBody(ext, 'focusChatView'); + assert.match(body, /dbg\('chat\.focus\.failed'/, 'the failure is not logged — .catch(() => {}) is not a fix'); + assert.match(body, /\bwhy\b/, 'the log must name the caller, or it is as unattributed as the rejection was'); + assert.ok(!/\bthrow\b/.test(body), 'the helper must not rethrow — every caller uses it in a void context'); + // Either `.then(undefined, …)` or `.catch(…)`. They are equivalent here and pinning one would fail a + // refactor that changes nothing; what must not disappear is the rejection handler itself. + assert.match(body, /\.then\(undefined,|\.catch\(/, 'no rejection handler — the helper can still reject'); + + // And it must be the thing the background callers actually use. + const callers = (ext.match(/focusChatView\('/g) || []).length; + assert.ok(callers >= 6, 'expected the background reveals to route through the helper, found ' + callers); +}); + console.log('\nchatSurface: ' + n + ' tests passed.'); From b82f72689ba85882e2486fd74e4a2afa58c0b8f8 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sat, 15 Aug 2026 20:29:33 -0400 Subject: [PATCH 2/3] feat(agent): a missing workspace gates the file tools, not the whole run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runAgent opened with a blanket refusal — "Open a folder first — the agent works on your workspace." — written for the file tools but placed where it failed the ENTIRE run. So with no folder open you could not ask what an error meant, could not reach a single MCP server (the GitHub server does not care whether you have a folder open), and could not ask about the file open in the editor in front of you. The reported case was exactly that: "Explain what the current file does", refused, for a request that never needed a workspace. The root still gates the tools that resolve a path or a cwd against it. It no longer gates the agent. withheld rootless : list_files, read_file, search, edit_file, write_file, delete_file, run_command, read_command_output still available : update_plan, ask_user, use_skill + every MCP tool Withheld rather than offered-and-failing: a tool that is present but errors on every call is worse than one that is absent, because the model retries it. THREE THINGS THAT WOULD HAVE MADE THIS A WORSE EXPERIENCE THAN THE REFUSAL: - PORTABLE_TOOLS is DERIVED from NEEDS_ROOT, not a second hand-written list, so the two cannot disagree about a tool. - baseTools switches too. Left on the full TOOLS it would bill the context popover for schemas that were never sent. - The model is TOLD why the tools are missing. Without that it sees a list with no read_file and improvises — answering about files it cannot see, or apologising at length for a limit it cannot name. The note also points at what still works and, if the request genuinely needs files, at File > Open Folder. MCP servers are still spawned with a cwd, so a null root now falls back to os.homedir() rather than being passed through. Guards, each bypass-verified by reverting the fix: - the blanket refusal restored (the reported bug) - run_command un-gated; ask_user gated (which would rebuild the refusal a tool at a time) - NEEDS_ROOT naming a tool that no longer exists, i.e. rename drift - the tool list no longer switching on the root; baseTools billing for unsent tools - MCP spawned with a null cwd - the note built but never concatenated into the prompt — the classic version of this bug, which looks right in review and does nothing 5 tests in agentNoWorkspace, 33 suites green. --- extensions/levelcode-ai/agent.js | 43 ++++++- .../test/agentNoWorkspace.test.js | 114 ++++++++++++++++++ 2 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 extensions/levelcode-ai/test/agentNoWorkspace.test.js diff --git a/extensions/levelcode-ai/agent.js b/extensions/levelcode-ai/agent.js index 1a248a0..9ec3698 100644 --- a/extensions/levelcode-ai/agent.js +++ b/extensions/levelcode-ai/agent.js @@ -13,6 +13,7 @@ const vscode = require('vscode'); const fs = require('fs'); const path = require('path'); const cp = require('child_process'); +const os = require('os'); // MCP servers need a cwd even when no folder is open const providers = require('./providers/index'); const { formatVerifyFeedback, verifyOutcome, looksUnrunnable, sniffPort, sniffPreviewUrl, looksReady } = require('./verify'); const { classifyCommand, dangerLabel } = require('./commandSafety'); @@ -68,6 +69,22 @@ function buildSystem(menu) { const list = menu.map((s) => '- ' + s.name + ': ' + s.description).join('\n'); return SYSTEM_BASE + '\n\nAvailable skills (call use_skill with the name):\n' + list; } +// The tools that resolve a PATH or a CWD against the workspace root. With no folder open they have +// nothing to resolve against, so they are withheld from the model rather than offered and left to fail +// one call at a time — a tool that is present but always errors is worse than one that is absent. +// +// Everything NOT in here works fine rootless: update_plan and ask_user are pure conversation, use_skill +// reads from the extension, and every MCP tool talks to its own server (the GitHub server does not care +// whether you have a folder open). That is the whole reason the old blanket refusal was wrong. +// +// read_command_output is included because it reads the output of a background run_command, and without +// a root there is no way to have started one. +const NEEDS_ROOT = new Set([ + 'list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file', + 'run_command', 'read_command_output' +]); +const PORTABLE_TOOLS = TOOLS.filter((t) => !NEEDS_ROOT.has(t.name)); + const TOOLS_TOKENS_EST = Math.round(JSON.stringify(TOOLS).length / 4); // Cross-session memory recall (docs/levelcode-sessions-memory.md). Added to a run's tools ONLY when the host @@ -636,7 +653,7 @@ async function setupMcp(ctx, wsFolders, dbg) { // lazy option. Only the FIRST run of a session pays it — mcpClient keeps handles in a module // registry, and connectAll reuses a live one. ctx.post({ type: 'agentStatus', text: 'starting MCP servers…' }); - const { handles, problems: connectProblems } = await connectAll(trusted, { cwd: ctx.root }); + const { handles, problems: connectProblems } = await connectAll(trusted, { cwd: ctx.root || os.homedir() }); for (const p of connectProblems) { dbg('mcp.connect', p); ctx.post({ type: 'agentTool', icon: 'warning', text: '🔌 mcp · "' + p.server + '" failed to start — ' + p.message }); @@ -675,8 +692,11 @@ async function setupMcp(ctx, wsFolders, dbg) { } async function runAgent(ctx) { + // No workspace is no longer a refusal. It used to fail the whole run here, which meant a question + // that never needed a folder — "what does this error mean?", anything through an MCP server, a + // follow-up about the conversation itself — died on a guard written for the file tools. The root + // still gates those tools (see NEEDS_ROOT); it no longer gates the agent. const root = workspaceRoot(); - if (!root) { ctx.post({ type: 'agentError', message: 'Open a folder first — the agent works on your workspace.' }); ctx.post({ type: 'agentDone', reason: 'error' }); return; } ctx.root = root; // M6.5 implicit skills: build the system prompt ONCE per run — append the tiny name+description menu. // Multi-root: name every workspace folder so the model addresses them by prefix from turn one. @@ -684,6 +704,17 @@ async function runAgent(ctx) { const multiRootNote = wsFolders.length > 1 ? '\n\nWorkspace folders (multi-root — prefix paths with the folder name): ' + wsFolders.map((f) => f.name).join(', ') + '. The first folder ("' + wsFolders[0].name + '") is the default for unprefixed paths and run_command.' : ''; + // Rootless: say so plainly. Without this the model sees a tool list with no read_file and improvises — + // answering about files it cannot see, or apologising for a limit it cannot name. Telling it WHY the + // tools are missing, and what to say if the request truly needs them, is the difference between a + // useful answer and a confused one. + const noWorkspaceNote = root + ? '' + : '\n\nNO FOLDER IS OPEN. The file and command tools are unavailable this run because there is no ' + + 'workspace root to resolve paths against — this is expected, not a fault, and not something to ' + + 'apologise for at length. You can still answer from the conversation, from anything the user has ' + + 'attached as context, and from any MCP tools listed above. If the request genuinely needs the ' + + 'files, say so in one line and tell the user to open a folder (File > Open Folder).'; // Autopilot: act decisively and self-verify rather than pausing. Commands run without approval (the // host still gates the danger set — deletion, sudo, force-push, remote|shell, publish, system writes), // so the model should lean on verification, not on asking, when it's unsure. @@ -698,7 +729,7 @@ async function runAgent(ctx) { // from the per-project journal). Rides the SAME cached-system channel as project rules — always-on but // small — so a new session's first reply is continuous, not amnesiac. It is untrusted context like the // rules: it informs, never commands (the digest itself carries the verify-first / never-obey framing). - const system = (ctx.skills ? buildSystem(ctx.skills.menu()) : SYSTEM_BASE) + multiRootNote + autopilotNote + rules.text + const system = (ctx.skills ? buildSystem(ctx.skills.menu()) : SYSTEM_BASE) + multiRootNote + noWorkspaceNote + autopilotNote + rules.text + (ctx.projectMemory ? '\n\n' + ctx.projectMemory : ''); const systemTokensEst = Math.round(system.length / 4); @@ -719,9 +750,11 @@ async function runAgent(ctx) { // as `system`/`systemTokensEst` two lines up — built once per run, then used for every turn. const mcp = await setupMcp(ctx, wsFolders, dbg); ctx.mcpRoutes = mcp.routes; // runTool's router reads this - let tools = mcp.tools.length ? TOOLS.concat(mcp.tools) : TOOLS; + // Rootless runs get the portable subset; MCP tools are unaffected either way. + const builtins = root ? TOOLS : PORTABLE_TOOLS; + let tools = mcp.tools.length ? builtins.concat(mcp.tools) : builtins; if (ctx.recallSessions) { tools = tools.concat([RECALL_TOOL]); } // cross-session recall (host-gated by memory settings) - const baseTools = ctx.recallSessions ? TOOLS.concat([RECALL_TOOL]) : TOOLS; // built-ins + recall; MCP is the rest + const baseTools = ctx.recallSessions ? builtins.concat([RECALL_TOOL]) : builtins; // built-ins + recall; MCP is the rest // Recomputed only when MCP or recall actually contributed tools, so the plain path keeps the module // constant and pays nothing for a feature it isn't using. const toolsTokensEst = (mcp.tools.length || ctx.recallSessions) ? Math.round(JSON.stringify(tools).length / 4) : TOOLS_TOKENS_EST; diff --git a/extensions/levelcode-ai/test/agentNoWorkspace.test.js b/extensions/levelcode-ai/test/agentNoWorkspace.test.js new file mode 100644 index 0000000..263dbc6 --- /dev/null +++ b/extensions/levelcode-ai/test/agentNoWorkspace.test.js @@ -0,0 +1,114 @@ +/*--------------------------------------------------------------------------------------------- + * The agent runs without a folder open — run: node test/agentNoWorkspace.test.js + * + * What this replaces: runAgent used to open with a blanket refusal — + * + * if (!root) { post({ type: 'agentError', message: 'Open a folder first — …' }); return; } + * + * written for the file tools, but placed where it failed the ENTIRE run. So with no folder open you + * could not ask what an error meant, could not reach a single MCP server (the GitHub server does not + * care whether you have a folder open), and could not ask about the file sitting in the editor in + * front of you. The reported case was exactly that: "Explain what the current file does" — refused, + * for a request that never needed a workspace. + * + * The root still gates the tools that resolve a path or a cwd against it. It no longer gates the agent. + * + * Asserted from SOURCE: standing up a real runAgent needs a live VS Code host and a provider, which + * this pure-unit suite deliberately does not stand up — the same approach agentMaxSteps.test.js takes. + *--------------------------------------------------------------------------------------------*/ +// @ts-check +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); + +const agent = fs.readFileSync(path.join(__dirname, '..', 'agent.js'), 'utf8'); + +let n = 0; +function test(name, fn) { fn(); n++; console.log(' ok - ' + name); } + +/** The `NEEDS_ROOT` set as shipped, read out of the source rather than restated here. */ +function needsRoot() { + const m = /const NEEDS_ROOT = new Set\(\[([\s\S]*?)\]\);/.exec(agent); + assert.ok(m, 'agent.js no longer declares NEEDS_ROOT'); + return (m[1].match(/'([a-z_]+)'/g) || []).map((s) => s.replace(/'/g, '')); +} + +/** Every built-in tool name, from the TOOLS array. */ +function allToolNames() { + const start = agent.indexOf('const TOOLS = ['); + assert.ok(start > 0, 'agent.js no longer declares TOOLS'); + const block = agent.slice(start, agent.indexOf('\n];', start)); + return (block.match(/\{ name: '([a-z_]+)'/g) || []).map((s) => s.replace(/.*'([a-z_]+)'.*/, '$1')); +} + +test('the blanket refusal is gone', () => { + assert.ok(!/Open a folder first/.test(agent), + 'runAgent still refuses outright when no folder is open — that guard belongs on the tools, not the run'); + // And the root is still READ — the tools need it, and losing it would silently disable them everywhere. + assert.match(agent, /const root = workspaceRoot\(\);\s*\n\s*ctx\.root = root;/, + 'runAgent must still resolve and carry the root, even when it is null'); +}); + +test('the tools that need a root are withheld, and only those', () => { + const gated = needsRoot(); + // Everything that resolves a path or a cwd. Miss one and it is offered rootless, then fails on the + // model's first call — which is worse than not offering it, because the model retries. + for (const name of ['list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file', 'run_command']) { + assert.ok(gated.includes(name), name + ' resolves a workspace path but is not in NEEDS_ROOT'); + } + // …and nothing that works fine without one. Gating these would rebuild the old refusal a tool at a + // time: they are the entire reason a rootless run is still useful. + for (const name of ['update_plan', 'ask_user', 'use_skill']) { + assert.ok(!gated.includes(name), name + ' needs no workspace — gating it removes the point of the change'); + } + // The set must name real tools, or a rename silently un-gates one. + const known = allToolNames(); + const unknown = gated.filter((g) => !known.includes(g)); + assert.deepStrictEqual(unknown, [], 'NEEDS_ROOT names tools that no longer exist: ' + unknown.join(', ')); +}); + +test('the portable subset is what a rootless run actually offers', () => { + assert.match(agent, /const PORTABLE_TOOLS = TOOLS\.filter\(\(t\) => !NEEDS_ROOT\.has\(t\.name\)\)/, + 'PORTABLE_TOOLS must be derived from NEEDS_ROOT, not maintained as a second hand-written list'); + assert.match(agent, /const builtins = root \? TOOLS : PORTABLE_TOOLS;/, + 'the run no longer switches its tool list on the root'); + // Both assemblies must use it. baseTools feeds the context-usage split; if it kept the full TOOLS the + // popover would bill the user for tools that were never sent. + assert.match(agent, /let tools = mcp\.tools\.length \? builtins\.concat\(mcp\.tools\) : builtins;/, + 'the model is still handed the unfiltered TOOLS'); + assert.match(agent, /const baseTools = ctx\.recallSessions \? builtins\.concat\(\[RECALL_TOOL\]\) : builtins;/, + 'baseTools still counts the full TOOLS — the context popover would report tools that were not sent'); +}); + +test('MCP is unaffected by a missing root — that is half the point', () => { + // The GitHub server, the filesystem server pointed somewhere else, anything stdio: none of them need + // the editor to have a folder open. But they are spawned with a cwd, so a null one has to resolve to + // something real rather than being passed through. + assert.match(agent, /connectAll\(trusted, \{ cwd: ctx\.root \|\| os\.homedir\(\) \}\)/, + 'MCP servers are spawned with a null cwd when no folder is open'); + assert.match(agent, /^const os = require\('os'\);/m, "agent.js does not require 'os'"); + assert.ok(!/NEEDS_ROOT[\s\S]{0,400}mcp/i.test(agent.slice(agent.indexOf('const NEEDS_ROOT'), agent.indexOf('const PORTABLE_TOOLS'))), + 'MCP tools must not be filtered by NEEDS_ROOT — they are not workspace tools'); +}); + +test('the model is told WHY the tools are missing', () => { + // Without this it sees a tool list with no read_file and improvises: answering about files it cannot + // see, or apologising at length for a limit it cannot name. + const m = /const noWorkspaceNote = root[\s\S]*?;\n/.exec(agent); + assert.ok(m, 'no rootless system-prompt note — the model gets a truncated tool list and no explanation'); + const note = m[0]; + assert.match(note, /NO FOLDER IS OPEN/, 'the note must state the condition plainly'); + assert.match(note, /MCP/, 'the note must point at what DOES still work, not only at what does not'); + assert.match(note, /Open Folder/, 'the note must tell the user the way out when the request really needs files'); + assert.match(note, /^\s*const noWorkspaceNote = root\s*\n?\s*\? ''/m, + 'the note must be empty when a folder IS open, or every normal run pays for it'); + + // And it has to reach the prompt. A note that is built and never concatenated is the classic version + // of this bug — it looks right in review and does nothing. + assert.match(agent, /\+ multiRootNote \+ noWorkspaceNote \+ autopilotNote/, + 'noWorkspaceNote is never added to the system prompt'); +}); + +console.log('\nagentNoWorkspace: ' + n + ' tests passed.'); From 3dde9b23c33c729faf7e471feeeb108d84808b8e Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sat, 15 Aug 2026 20:42:06 -0400 Subject: [PATCH 3/3] fix(agent): bill the context popover for the tool list that was actually sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both review points on #78 were right. 1. THE TOKEN ESTIMATE STILL COUNTED THE FULL TOOL LIST `toolsTokensEst` falls back to a module constant on the plain path (no MCP, no recall) so that path never re-stringifies. But that constant was built from TOOLS, and after this PR there are TWO plain paths — a rootless run sends PORTABLE_TOOLS and was billed for TOOLS. Measured: ~1000 tokens, about two thirds of the tool budget, reported against a window that never spent it. That is worse than a missing feature; it is a meter reading high, and the context popover exists precisely so that number can be trusted. This is the same mistake as leaving baseTools on TOOLS, one line further down — which I fixed, described in the PR body, and then missed here. PORTABLE_TOOLS_TOKENS_EST is a second constant rather than a call-time derivation, because keeping the plain path free of JSON.stringify is the whole reason the constant exists. 2. THE TEST DID NOT PIN read_command_output It asserted seven of the eight gated tools. read_command_output was the omission, and it is the plausible one to lose: it takes no path and reads as portable at a glance, so nothing would have objected to un-gating it. Rootless it can only ever refer to a background run_command that could not have started. Guards, each bypass-verified by reverting the fix: - read_command_output un-gated (the second review point) - the estimate reverting to the full-TOOLS constant (the first) - the estimate no longer switching on the root; the rootless estimate deleted - PORTABLE_TOOLS no longer a filtered subset, which would make the estimate guard vacuous 6 tests in agentNoWorkspace, 33 suites green. --- extensions/levelcode-ai/agent.js | 12 +++++++-- .../test/agentNoWorkspace.test.js | 26 ++++++++++++++++++- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/extensions/levelcode-ai/agent.js b/extensions/levelcode-ai/agent.js index 9ec3698..fb2a462 100644 --- a/extensions/levelcode-ai/agent.js +++ b/extensions/levelcode-ai/agent.js @@ -86,6 +86,11 @@ const NEEDS_ROOT = new Set([ const PORTABLE_TOOLS = TOOLS.filter((t) => !NEEDS_ROOT.has(t.name)); const TOOLS_TOKENS_EST = Math.round(JSON.stringify(TOOLS).length / 4); +// The same estimate for the rootless list, and it has to exist separately rather than be derived at +// call time: the plain path deliberately never re-stringifies (see toolsTokensEst below), so without a +// second constant a rootless run reports the FULL schema cost for a list it never sent — about 1000 +// tokens, two thirds of the tool budget, charged against a window that never spent it. +const PORTABLE_TOOLS_TOKENS_EST = Math.round(JSON.stringify(PORTABLE_TOOLS).length / 4); // Cross-session memory recall (docs/levelcode-sessions-memory.md). Added to a run's tools ONLY when the host // wires ctx.recallSessions (memory + the recall setting on), so it costs nothing otherwise. Read-only and @@ -756,8 +761,11 @@ async function runAgent(ctx) { if (ctx.recallSessions) { tools = tools.concat([RECALL_TOOL]); } // cross-session recall (host-gated by memory settings) const baseTools = ctx.recallSessions ? builtins.concat([RECALL_TOOL]) : builtins; // built-ins + recall; MCP is the rest // Recomputed only when MCP or recall actually contributed tools, so the plain path keeps the module - // constant and pays nothing for a feature it isn't using. - const toolsTokensEst = (mcp.tools.length || ctx.recallSessions) ? Math.round(JSON.stringify(tools).length / 4) : TOOLS_TOKENS_EST; + // constant and pays nothing for a feature it isn't using — but there are now TWO plain paths, and the + // constant has to match the list that was actually sent. Reporting the full cost for a rootless run + // was the same mistake as leaving baseTools on TOOLS, one line further down. + const builtinsTokensEst = root ? TOOLS_TOKENS_EST : PORTABLE_TOOLS_TOKENS_EST; + const toolsTokensEst = (mcp.tools.length || ctx.recallSessions) ? Math.round(JSON.stringify(tools).length / 4) : builtinsTokensEst; // The MCP SHARE of that, reported separately so the context popover can show what these servers cost // (docs/MCP.md S5). Every tool schema rides EVERY turn, so a chatty server is a standing tax on the // window rather than a one-off — and until it has its own segment, that cost is invisible. diff --git a/extensions/levelcode-ai/test/agentNoWorkspace.test.js b/extensions/levelcode-ai/test/agentNoWorkspace.test.js index 263dbc6..09874b5 100644 --- a/extensions/levelcode-ai/test/agentNoWorkspace.test.js +++ b/extensions/levelcode-ai/test/agentNoWorkspace.test.js @@ -55,7 +55,12 @@ test('the tools that need a root are withheld, and only those', () => { const gated = needsRoot(); // Everything that resolves a path or a cwd. Miss one and it is offered rootless, then fails on the // model's first call — which is worse than not offering it, because the model retries. - for (const name of ['list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file', 'run_command']) { + // read_command_output is in this list because run_command is: it reads the output of a background + // command, so rootless it can only ever refer to a run that could not have started. Review caught it + // missing — the un-gating it guards against is a plausible edit, since the tool takes no path and + // reads as portable at a glance. + for (const name of ['list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file', + 'run_command', 'read_command_output']) { assert.ok(gated.includes(name), name + ' resolves a workspace path but is not in NEEDS_ROOT'); } // …and nothing that works fine without one. Gating these would rebuild the old refusal a tool at a @@ -82,6 +87,25 @@ test('the portable subset is what a rootless run actually offers', () => { 'baseTools still counts the full TOOLS — the context popover would report tools that were not sent'); }); +test('the context popover is billed for the list that was actually sent', () => { + // Review found this one line below the baseTools fix, which is the same bug: the token estimate fell + // back to a module constant built from the FULL tool list, so a rootless run with no MCP reported the + // cost of eight schemas it never sent — ~1000 tokens, about two thirds of the tool budget, charged + // against a window that never spent it. Worse than a missing feature: it is a meter reading high. + assert.match(agent, /const PORTABLE_TOOLS_TOKENS_EST = Math\.round\(JSON\.stringify\(PORTABLE_TOOLS\)\.length \/ 4\);/, + 'no rootless token estimate — the popover reports the full tool cost for a list that was not sent'); + assert.match(agent, /const builtinsTokensEst = root \? TOOLS_TOKENS_EST : PORTABLE_TOOLS_TOKENS_EST;/, + 'the estimate no longer switches on the root'); + assert.match(agent, /const toolsTokensEst = \(mcp\.tools\.length \|\| ctx\.recallSessions\)[\s\S]{0,120}: builtinsTokensEst;/, + 'the plain path still falls back to the full-TOOLS constant'); + + // The two constants must actually differ, or the guard above passes on a list that gates nothing — + // the vacuous-pass this whole change would otherwise be measured by. + assert.ok(/PORTABLE_TOOLS = TOOLS\.filter/.test(agent), 'PORTABLE_TOOLS is no longer a strict subset'); + const gated = needsRoot(); + assert.ok(gated.length > 0, 'NEEDS_ROOT is empty — the two estimates would be identical and this test vacuous'); +}); + test('MCP is unaffected by a missing root — that is half the point', () => { // The GitHub server, the filesystem server pointed somewhere else, anything stdio: none of them need // the editor to have a folder open. But they are spawned with a cwd, so a null one has to resolve to