Skip to content

Commit b655f95

Browse files
committed
fix(levelcode-ai): address PR review — running members, escaping, schema, leaks
Copilot review of the calm-transcript PR surfaced several real issues: - **Running members were collapsed on insertion**, hiding the live command's output behind a chevron — the opposite of the intended fixed-footprint header (finished members fold, the running one stays readable). groupAppend now folds only non-running members; groupStepDone folds a member when it completes successfully. This is the behavior the original screenshots asked for. - **A group could claim to be finished while a background command still ran.** finalizeGroup now detects a still-running member, keeps the live header and the Stop control, and lets that member's exit re-enter and settle the group — so a running command is never misrepresented as done nor loses its only Stop button. - **Grouped edit cards rendered with the full diff open** (groupAppend ran before the card's innerHTML existed, so collapseMember couldn't close it). New grouped edit cards now default the diff closed — a one-line row. - **The tooltip attribute wasn't quote-safe.** search chips carry `search "foo"`; esc() doesn't escape quotes, so the raw " closed the title="…" attribute. Uses escAttr now. - **`explanation` is now schema-required** on run_command / read_file / search, matching the "MUST include" contract (was prose-only; schema allowed omission). - **Step-map leaks**: editSteps was never cleared (now released in resolveEditCard and pruned on checkpoint restore alongside its card); termSteps leaked on termExitFinish's early return (now settled + deleted there too). - **Aggregate accuracy**: listing files was folded into "searched the workspace"; it's now its own "listed files" clause. - **groupActivity off mid-run** now closes any open group so the timeline returns fully flat immediately. Kept intentionally (product owner's explicit request): the `/rme` command and its base64 payload — see the reply on that thread. groupReducer +3 tests (running-stays-expanded, still-running-close, quote-escape), narrativeUi +1 (list ≠ search). Every change verified in a rendered webview with an actually-running command. Gate green (22 suites).
1 parent 7aa37cd commit b655f95

4 files changed

Lines changed: 77 additions & 15 deletions

File tree

extensions/levelcode-ai/agent.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,13 @@ const SYSTEM_BASE = [
4242

4343
const TOOLS = [
4444
{ name: 'list_files', description: 'List workspace files (optional glob like "**/*.js"). Excludes node_modules/.git/build dirs.', input_schema: { type: 'object', properties: { glob: { type: 'string' } } } },
45-
{ name: 'read_file', description: 'Read a workspace file (path relative to the workspace root; in a multi-root workspace use the folder-name prefix exactly as list_files shows it).', input_schema: { type: 'object', properties: { path: { type: 'string' }, explanation: { type: 'string', description: 'ALWAYS provide: 3-8 words, active voice, imperative — WHY you are reading this ("Read the runAgent call site", "Read agent.js tool definitions"). Shown to the user as this action\'s label.' } }, required: ['path'] } },
46-
{ name: 'search', description: 'Search file contents for a literal string. Returns file:line snippets.', input_schema: { type: 'object', properties: { query: { type: 'string' }, explanation: { type: 'string', description: 'ALWAYS provide: 3-8 words, active voice, imperative — what you are looking for ("Find every postMessage call site"). Shown to the user as this action\'s label.' } }, required: ['query'] } },
45+
{ name: 'read_file', description: 'Read a workspace file (path relative to the workspace root; in a multi-root workspace use the folder-name prefix exactly as list_files shows it).', input_schema: { type: 'object', properties: { path: { type: 'string' }, explanation: { type: 'string', description: 'REQUIRED: 3-8 words, active voice, imperative — WHY you are reading this ("Read the runAgent call site", "Read agent.js tool definitions"). Shown to the user as this action\'s label.' } }, required: ['path', 'explanation'] } },
46+
{ name: 'search', description: 'Search file contents for a literal string. Returns file:line snippets.', input_schema: { type: 'object', properties: { query: { type: 'string' }, explanation: { type: 'string', description: 'REQUIRED: 3-8 words, active voice, imperative — what you are looking for ("Find every postMessage call site"). Shown to the user as this action\'s label.' } }, required: ['query', 'explanation'] } },
4747
{ name: 'update_plan', description: 'Declare or update your task checklist for a multi-step goal. Pass the FULL list each time, each item with a status. Call it once up front (all pending), then again to mark an item in_progress when you start it and done when finished. Skip for trivial single-step goals.', input_schema: { type: 'object', properties: { todos: { type: 'array', items: { type: 'object', properties: { title: { type: 'string' }, status: { type: 'string', enum: ['pending', 'in_progress', 'done'] } }, required: ['title', 'status'] } } }, required: ['todos'] } },
4848
{ name: 'edit_file', description: 'Make a targeted edit to an EXISTING file: replace an exact, unique snippet (old_str) with new_str. Applied immediately; the user reviews it with Keep/Undo. old_str must appear exactly once — include enough surrounding context to be unique.', input_schema: { type: 'object', properties: { path: { type: 'string' }, old_str: { type: 'string' }, new_str: { type: 'string' } }, required: ['path', 'old_str', 'new_str'] } },
4949
{ name: 'write_file', description: 'Create a new file (or fully overwrite a short one) with the COMPLETE content. For edits to existing files, prefer edit_file. Applied immediately; the user reviews it with Keep/Undo.', input_schema: { type: 'object', properties: { path: { type: 'string' }, content: { type: 'string' } }, required: ['path', 'content'] } },
5050
{ name: 'delete_file', description: 'Delete an EXISTING workspace file (e.g. removing a file during a refactor). Applied immediately; the user reviews it with Keep/Undo, and the per-turn checkpoint can restore it. To RENAME or move a file: write_file the new path, then delete_file the old one.', input_schema: { type: 'object', properties: { path: { type: 'string' } }, required: ['path'] } },
51-
{ name: 'run_command', description: 'Run a shell command in the workspace root (or a named workspace folder via "folder" in multi-root workspaces). Requires approval. Pass background:true for commands that do not exit on their own (servers, watchers) so the agent is not blocked — it returns immediately and you read progress later with read_command_output.', input_schema: { type: 'object', properties: { command: { type: 'string' }, explanation: { type: 'string', description: 'ALWAYS provide: 5-10 words, active voice, imperative — what this command does ("Run the extension unit tests", "Find the insertion point in section 10"). Shown to the user as this action\'s label.' }, folder: { type: 'string', description: 'multi-root workspaces only: the workspace folder NAME to run in; defaults to the first folder' }, background: { type: 'boolean', description: 'true = start it and keep working without waiting (dev servers, watchers, tail -f). Returns immediately with an id; poll read_command_output for its output/status.' } }, required: ['command'] } },
51+
{ name: 'run_command', description: 'Run a shell command in the workspace root (or a named workspace folder via "folder" in multi-root workspaces). Requires approval. Pass background:true for commands that do not exit on their own (servers, watchers) so the agent is not blocked — it returns immediately and you read progress later with read_command_output.', input_schema: { type: 'object', properties: { command: { type: 'string' }, explanation: { type: 'string', description: 'REQUIRED: 5-10 words, active voice, imperative — what this command does ("Run the extension unit tests", "Find the insertion point in section 10"). Shown to the user as this action\'s label.' }, folder: { type: 'string', description: 'multi-root workspaces only: the workspace folder NAME to run in; defaults to the first folder' }, background: { type: 'boolean', description: 'true = start it and keep working without waiting (dev servers, watchers, tail -f). Returns immediately with an id; poll read_command_output for its output/status.' } }, required: ['command', 'explanation'] } },
5252
{ name: 'read_command_output', description: 'Read recent output + status of a command started with run_command background:true. Returns a status header ([running on :3000] / [exited 0] / [stopped]) followed by the latest output lines. Poll this to wait for a server to become ready before testing against it.', input_schema: { type: 'object', properties: { id: { type: 'string', description: 'the id returned by a background run_command' }, lines: { type: 'number', description: 'max recent output lines to return (default 80, max 400)' } }, required: ['id'] } },
5353
{ name: 'ask_user', description: 'Ask the user one or more multiple-choice questions when the goal genuinely depends on a decision only they can make (tech stack, scope, where to put files, must-have features). The user picks by CLICKING — do NOT write questions as prose. Ask ONCE up front with all your questions, then proceed with the answers and never re-ask. Prefer sensible defaults over asking; only ask when a wrong guess would waste real work.', input_schema: { type: 'object', properties: { questions: { type: 'array', items: { type: 'object', properties: { header: { type: 'string', description: 'a 1-3 word tag for the question' }, question: { type: 'string' }, multiSelect: { type: 'boolean', description: 'true if several options can be picked at once' }, options: { type: 'array', items: { type: 'object', properties: { label: { type: 'string' }, description: { type: 'string' } }, required: ['label'] } } }, required: ['question', 'options'] } } }, required: ['questions'] } },
5454
{ name: 'use_skill', description: 'Load an expert playbook (SKILL.md) for a task type, chosen from the "Available skills" list in your system prompt. Returns the skill\'s step-by-step instructions as the tool result — then follow them. Read-only and instant (no approval). Call it once, early, when the goal matches a skill\'s description.', input_schema: { type: 'object', properties: { name: { type: 'string', description: 'the exact skill name from the Available skills list' } }, required: ['name'] } }

extensions/levelcode-ai/media/chat.html

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1542,7 +1542,8 @@
15421542
const files = (list) => list.length > 2 ? (list.length + ' files') : list.map(short).join(', ');
15431543
const of = (kind) => uniq(steps.filter((s) => s.kind === kind && s.path).map((s) => s.path));
15441544
const cmds = steps.filter((s) => s.kind === 'cmd').length;
1545-
const searches = steps.filter((s) => s.kind === 'search' || s.kind === 'list').length;
1545+
const searches = steps.filter((s) => s.kind === 'search').length;
1546+
const lists = steps.filter((s) => s.kind === 'list').length; // listing files ≠ searching content
15461547
const reads = of('read'), edits = of('edit'), creates = of('create'), dels = of('delete');
15471548
// File work leads, commands trail ("Read and edited extension.js, ran a command") — the reference
15481549
// ordering: what changed matters more than how many shells it took.
@@ -1558,6 +1559,7 @@
15581559
if (creates.length){ parts.push('created ' + files(creates)); }
15591560
if (dels.length){ parts.push('deleted ' + files(dels)); }
15601561
if (searches){ parts.push('searched the workspace'); }
1562+
if (lists){ parts.push('listed files'); }
15611563
if (cmds){ parts.push('ran ' + (cmds === 1 ? 'a command' : cmds + ' commands')); }
15621564
if (steps.some((s) => s.kind === 'verify')){ parts.push('verified the edits'); }
15631565
if (!parts.length){ return steps.length === 1 ? '1 step' : steps.length + ' steps'; }
@@ -1634,7 +1636,10 @@
16341636
function groupAppend(el, step){
16351637
if (!groupsOn){ log.appendChild(el); return; }
16361638
const g = curGroup || openGroup();
1637-
collapseMember(el);
1639+
// A RUNNING member stays expanded so its live output is readable — that is the whole point of
1640+
// the fixed-footprint header. Finished/instantaneous members (chips, an already-exited command)
1641+
// fold to a one-line row now; a running one folds later, in groupStepDone, once it completes.
1642+
if (!step || step.status !== 'running'){ collapseMember(el); }
16381643
g.body.appendChild(el);
16391644
if (step){ step.group = g; g.steps.push(step); refreshGroupHead(); }
16401645
}
@@ -1665,6 +1670,14 @@
16651670
g.el.remove(); return;
16661671
}
16671672
g.closed = true;
1673+
// A member may still be running when narration closes the group (a background command). Don't
1674+
// claim the group is finished or hide the Stop control — keep the live chrome and let that
1675+
// member's own groupStepDone re-enter here to complete the group once it actually exits.
1676+
if (g.steps.some((s) => s.status === 'running')){
1677+
g.el.classList.remove('collapsed'); // never hide a command that is still going
1678+
refreshGroupHead();
1679+
return;
1680+
}
16681681
g.el.classList.remove('running');
16691682
g.el.classList.add(g.failed ? 'gfailed' : 'gok'); // recolours the rail node — see .tl-group.gok
16701683
g.label.textContent = groupAggregate(g.steps);
@@ -1696,6 +1709,9 @@
16961709
// if the user had collapsed it — a hidden failure is the one thing this UI must never do.
16971710
if (step.card && step.card.classList){ step.card.classList.remove('collapsed'); }
16981711
if (g.el.isConnected){ g.el.classList.remove('collapsed'); }
1712+
} else {
1713+
// Success: the step is done, so it folds to a one-line row (it stayed expanded while running).
1714+
collapseMember(step.card);
16991715
}
17001716
if (g.closed){ finalizeGroup(g); } else if (g === curGroup){ refreshGroupHead(); }
17011717
}
@@ -1715,7 +1731,7 @@
17151731
// tool text stays as the tooltip so the underlying path/query is never lost.
17161732
const shown = (groupsOn && st.base) ? st.base : String(text || '');
17171733
d.innerHTML = '<div class="tl-rail"><span class="tl-node">' + (IC[icon] ? codicon(icon) : esc(icon || '•')) + '</span></div>'
1718-
+ '<div class="tl-body"><span class="toolt" title="' + esc(text || '') + '">' + esc(shown) + '</span></div>';
1734+
+ '<div class="tl-body"><span class="toolt" title="' + escAttr(text || '') + '">' + esc(shown) + '</span></div>';
17191735
groupAppend(d, st);
17201736
scrollIfStuck();
17211737
}
@@ -1901,8 +1917,15 @@
19011917
scrollIfStuck();
19021918
}
19031919
function termExitFinish(m){
1904-
const card = termCards[m.id]; if (!card) { return; }
1905-
const st = card.querySelector('.termstatus'); if (!st) { return; }
1920+
// Even if the card is gone (checkpoint restore / prune), still settle the group step and drop
1921+
// the step map entry — otherwise termSteps leaks and holds a stale group reference all session.
1922+
const card = termCards[m.id];
1923+
if (!card || !card.querySelector('.termstatus')){
1924+
if (termSteps[m.id]){ groupStepDone(termSteps[m.id], m.how === 'timeout' || (m.how !== 'stopped' && m.code !== 0)); delete termSteps[m.id]; }
1925+
delete termCards[m.id];
1926+
return;
1927+
}
1928+
const st = card.querySelector('.termstatus');
19061929
const dur = (typeof m.ms === 'number') ? (m.ms >= 1000 ? (m.ms / 1000).toFixed(1) + 's' : m.ms + 'ms') : '';
19071930
let icon, text, cls;
19081931
if (m.how === 'stopped'){ icon = 'circle-slash'; text = 'stopped'; cls = 'bad'; }
@@ -2113,7 +2136,11 @@
21132136
function addEditCard(m){
21142137
clearEmpty(); clearStatus(); agentBubble = null;
21152138
let card = editCards[m.id];
2116-
let wasOpen = true;
2139+
// A NEW edit card inside a group starts as a one-line row (diff closed); ungrouped it keeps
2140+
// today's open-diff default. (collapseMember can't close the diff at groupAppend time because the
2141+
// card's innerHTML isn't built yet, so the default has to be right here.) A re-render preserves
2142+
// whatever open/closed state the user last left it in.
2143+
let wasOpen = !groupsOn;
21172144
if (!card){
21182145
card = document.createElement('div'); card.className = 'editcard';
21192146
editCards[m.id] = card;
@@ -2174,6 +2201,7 @@
21742201
const det = card.querySelector('details.diffwrap'); if (det) { det.open = false; } // collapse so resolved cards don't stack tall
21752202
card.classList.add('resolved');
21762203
delete editCards[m.id];
2204+
delete editSteps[m.id]; // release the group step ref alongside the card (no unbounded growth)
21772205
}
21782206
// ---- context-window usage: always-visible footer stat + a warning bar when it fills ----
21792207
const ctxBar = document.getElementById('ctxBar');
@@ -2826,7 +2854,9 @@
28262854
if (typeof m.contextLimit === 'number'){ renderContext({ limit: m.contextLimit }); }
28272855
lastProvider = m.provider || '';
28282856
lastModelId = m.modelId || m.model || ''; // gateway sends modelId (the real id); BYOK model IS the id
2829-
if (typeof m.groupActivity === 'boolean'){ groupsOn = m.groupActivity; } // calm-transcript toggle
2857+
// calm-transcript toggle. Turning it OFF mid-run closes any open group first, so the timeline
2858+
// returns to fully flat immediately instead of leaving one stray group behind.
2859+
if (typeof m.groupActivity === 'boolean'){ if (!m.groupActivity){ closeGroup(); } groupsOn = m.groupActivity; }
28302860
renderRouting();
28312861
}
28322862
else if (m.type === 'assistantStart'){ shown = ''; pending = ''; doneSignaled = false; flushAll = false; current = makeStream(add('assistant', '')); setStreaming(true); }
@@ -2885,7 +2915,9 @@
28852915
else if (m.type === 'reset'){
28862916
log.innerHTML = '<div id="empty"><div class="big">New chat</div>Ask about your code, or describe what to build.</div>';
28872917
selLabel = null; ctxFiles = []; renderChips();
2888-
for (const k in editCards) { delete editCards[k]; } for (const k in termCards) { delete termCards[k]; } for (const k in verifyCards) { delete verifyCards[k]; } for (const k in bgTasks) { delete bgTasks[k]; } renderBgTasks(); updateReviewBar(0); renderPlan([]); renderContext({ input: 0 });
2918+
for (const k in editCards) { delete editCards[k]; } for (const k in termCards) { delete termCards[k]; } for (const k in verifyCards) { delete verifyCards[k]; } for (const k in bgTasks) { delete bgTasks[k]; }
2919+
for (const k in editSteps) { delete editSteps[k]; } for (const k in termSteps) { delete termSteps[k]; } for (const k in verifySteps) { delete verifySteps[k]; } // step maps ride with their cards
2920+
renderBgTasks(); updateReviewBar(0); renderPlan([]); renderContext({ input: 0 });
28892921
pending = ''; shown = ''; doneSignaled = false; flushAll = false; current = null; setStreaming(false);
28902922
forceStick();
28912923
}

0 commit comments

Comments
 (0)