Skip to content

Commit 3dde9b2

Browse files
committed
fix(agent): bill the context popover for the tool list that was actually sent
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.
1 parent b82f726 commit 3dde9b2

2 files changed

Lines changed: 35 additions & 3 deletions

File tree

extensions/levelcode-ai/agent.js

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ const NEEDS_ROOT = new Set([
8686
const PORTABLE_TOOLS = TOOLS.filter((t) => !NEEDS_ROOT.has(t.name));
8787

8888
const TOOLS_TOKENS_EST = Math.round(JSON.stringify(TOOLS).length / 4);
89+
// The same estimate for the rootless list, and it has to exist separately rather than be derived at
90+
// call time: the plain path deliberately never re-stringifies (see toolsTokensEst below), so without a
91+
// second constant a rootless run reports the FULL schema cost for a list it never sent — about 1000
92+
// tokens, two thirds of the tool budget, charged against a window that never spent it.
93+
const PORTABLE_TOOLS_TOKENS_EST = Math.round(JSON.stringify(PORTABLE_TOOLS).length / 4);
8994

9095
// Cross-session memory recall (docs/levelcode-sessions-memory.md). Added to a run's tools ONLY when the host
9196
// wires ctx.recallSessions (memory + the recall setting on), so it costs nothing otherwise. Read-only and
@@ -756,8 +761,11 @@ async function runAgent(ctx) {
756761
if (ctx.recallSessions) { tools = tools.concat([RECALL_TOOL]); } // cross-session recall (host-gated by memory settings)
757762
const baseTools = ctx.recallSessions ? builtins.concat([RECALL_TOOL]) : builtins; // built-ins + recall; MCP is the rest
758763
// Recomputed only when MCP or recall actually contributed tools, so the plain path keeps the module
759-
// constant and pays nothing for a feature it isn't using.
760-
const toolsTokensEst = (mcp.tools.length || ctx.recallSessions) ? Math.round(JSON.stringify(tools).length / 4) : TOOLS_TOKENS_EST;
764+
// constant and pays nothing for a feature it isn't using — but there are now TWO plain paths, and the
765+
// constant has to match the list that was actually sent. Reporting the full cost for a rootless run
766+
// was the same mistake as leaving baseTools on TOOLS, one line further down.
767+
const builtinsTokensEst = root ? TOOLS_TOKENS_EST : PORTABLE_TOOLS_TOKENS_EST;
768+
const toolsTokensEst = (mcp.tools.length || ctx.recallSessions) ? Math.round(JSON.stringify(tools).length / 4) : builtinsTokensEst;
761769
// The MCP SHARE of that, reported separately so the context popover can show what these servers cost
762770
// (docs/MCP.md S5). Every tool schema rides EVERY turn, so a chatty server is a standing tax on the
763771
// window rather than a one-off — and until it has its own segment, that cost is invisible.

extensions/levelcode-ai/test/agentNoWorkspace.test.js

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,12 @@ test('the tools that need a root are withheld, and only those', () => {
5555
const gated = needsRoot();
5656
// Everything that resolves a path or a cwd. Miss one and it is offered rootless, then fails on the
5757
// model's first call — which is worse than not offering it, because the model retries.
58-
for (const name of ['list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file', 'run_command']) {
58+
// read_command_output is in this list because run_command is: it reads the output of a background
59+
// command, so rootless it can only ever refer to a run that could not have started. Review caught it
60+
// missing — the un-gating it guards against is a plausible edit, since the tool takes no path and
61+
// reads as portable at a glance.
62+
for (const name of ['list_files', 'read_file', 'search', 'edit_file', 'write_file', 'delete_file',
63+
'run_command', 'read_command_output']) {
5964
assert.ok(gated.includes(name), name + ' resolves a workspace path but is not in NEEDS_ROOT');
6065
}
6166
// …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', () => {
8287
'baseTools still counts the full TOOLS — the context popover would report tools that were not sent');
8388
});
8489

90+
test('the context popover is billed for the list that was actually sent', () => {
91+
// Review found this one line below the baseTools fix, which is the same bug: the token estimate fell
92+
// back to a module constant built from the FULL tool list, so a rootless run with no MCP reported the
93+
// cost of eight schemas it never sent — ~1000 tokens, about two thirds of the tool budget, charged
94+
// against a window that never spent it. Worse than a missing feature: it is a meter reading high.
95+
assert.match(agent, /const PORTABLE_TOOLS_TOKENS_EST = Math\.round\(JSON\.stringify\(PORTABLE_TOOLS\)\.length \/ 4\);/,
96+
'no rootless token estimate — the popover reports the full tool cost for a list that was not sent');
97+
assert.match(agent, /const builtinsTokensEst = root \? TOOLS_TOKENS_EST : PORTABLE_TOOLS_TOKENS_EST;/,
98+
'the estimate no longer switches on the root');
99+
assert.match(agent, /const toolsTokensEst = \(mcp\.tools\.length \|\| ctx\.recallSessions\)[\s\S]{0,120}: builtinsTokensEst;/,
100+
'the plain path still falls back to the full-TOOLS constant');
101+
102+
// The two constants must actually differ, or the guard above passes on a list that gates nothing —
103+
// the vacuous-pass this whole change would otherwise be measured by.
104+
assert.ok(/PORTABLE_TOOLS = TOOLS\.filter/.test(agent), 'PORTABLE_TOOLS is no longer a strict subset');
105+
const gated = needsRoot();
106+
assert.ok(gated.length > 0, 'NEEDS_ROOT is empty — the two estimates would be identical and this test vacuous');
107+
});
108+
85109
test('MCP is unaffected by a missing root — that is half the point', () => {
86110
// The GitHub server, the filesystem server pointed somewhere else, anything stdio: none of them need
87111
// the editor to have a folder open. But they are spawned with a cwd, so a null one has to resolve to

0 commit comments

Comments
 (0)