Skip to content

Commit 397df3f

Browse files
committed
fix(web/shell): #107 review — an agent run counts as applied only the changes that were, not a folder the review kept back
The bug. After levelcode-ai's review the runtime counted what it had applied as `total - rest - failed`: every change of the set that the review neither handed back nor said it failed. shellCommit's `failed` counts FILE changes, and when a delete fails it keeps back the removal of the folder the file was in: that rmdir is in neither list, so it was counted as applied. `rm -r d` with two files, one delete failing: 3 - 0 - 1 = 2 for one change made. The count reaches the visitor and the model in the notes of the part that follows the review (the folders it leaves to the shell, which start from that count): with a folder to make whose creation fails, the stderr said "PARTLY applied — 1 of 2 file changes" from the review and then "applied 2 of 4 file changes; stopped at /scratch/m"; a Stop while that folder was being made said "2 of 4 were applied". One was. The fix (runtime.js applyChanges). What the review applied is the set's files less the ones it could not apply and less whatever else it handed back (unchanged: an entry that is not a folder of the set is still to be done, so it was not done by the review). Folders are never the review's to apply; one it keeps out of `rest` is applied by nobody and is no longer counted. The answer's shape is not changed and levelcode-ai is not touched: shellCommit's `failed` is the file changes it could not apply (host.js's SpawnOptions.commit says "how many changes it could NOT apply", and the review applies no others); README §5 now says that, and how the shell counts what was applied. Tests. shell-runtime-commit.test.js runs levelcode-ai's real commitShellChanges as the review, answering as agent.js hands it on, over a set of mkdir m, two deletes in d and rmdir d, with the second delete refused by the editor: the folder it leaves cannot be made ("applied 1 of 4 file changes; stopped at /scratch/m: quota."), and a Stop while it is being made ("1 of 4 were applied"). Both fail on abe8a25 (they say 2 of 4) and pass here. The other runtime suites pass unchanged.
1 parent ba7b046 commit 397df3f

3 files changed

Lines changed: 54 additions & 5 deletions

File tree

‎web/shell/README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,10 @@ script that ran to its end, whatever its status; otherwise nothing is applied):
225225
the same Keep/Undo and per-turn checkpoint as `edit_file`. It answers `{ rest, notes, failed?, refused? }`; `rest`
226226
may only contain `mkdir`/`rmdir` entries **of this change set** (anything else is counted failed and named), which
227227
the shell applies with `workspace.fs`; an answer that is not an object with a `rest` list is "gave no answer";
228-
`failed` must be a finite non-negative number. `notes` are appended to stderr.
228+
`failed` must be a finite non-negative number: the file changes (writes and deletes) it could not apply. What the
229+
result counts as applied is the set's files less `failed` and less anything else handed back, then the folders of
230+
`rest` the shell made or removed; a folder the review keeps out of `rest` (shellCommit keeps the removal of one that a
231+
file could not be deleted from) is applied by nobody and is not counted. `notes` are appended to stderr.
229232
2. **The visitor's terminal** (no `commit`): `src/commit.js` validates (inside the folders, size caps, entry cap),
230233
checks that nothing changed on disk since the command read it (read-only, bounded), and writes bytes with
231234
`workspace.fs` — creates, then deletes, deepest paths first, **never recursive and never to the trash** — and reports

‎web/shell/src/runtime.js‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,9 +1044,9 @@ function createRuntime(deps) {
10441044
* workspace directly otherwise. Validated as a whole first in both cases.
10451045
*
10461046
* What the review hands back is trusted for what the contract says it is - `rest`, a list of the folders the
1047-
* command made and removed, which this shell then makes and removes; `notes`; `failed`, a count - and no
1048-
* more: an entry that is not one of those folders is not applied, an answer that is not that shape is "no
1049-
* answer", and a count that is not a count is not believed.
1047+
* command made and removed, which this shell then makes and removes; `notes`; `failed`, the count of file changes
1048+
* it could not apply - and no more: an entry that is not one of those folders is not applied, an answer that is not
1049+
* that shape is "no answer", and a count that is not a count is not believed.
10501050
*
10511051
* Time is bounded the same way in both routes (the gate): a call to the file system is waited for `fsMs` at most
10521052
* and the part that writes has a budget in all - for the agent `commitMs` for the review and then the folders it
@@ -1103,11 +1103,16 @@ function createRuntime(deps) {
11031103
else { failed = total; notes.push(note('the review gave a count of changes it could not apply that is not a number, so none of them are counted as applied')); }
11041104
}
11051105
const handedBack = refused ? [] : r.rest;
1106-
let applied = refused ? 0 : Math.max(0, total - handedBack.length - failed);
11071106
// What the review leaves to the shell is the folders the command made and removed, and only those.
11081107
const folders = new Set(handed.filter((c) => c.kind === 'mkdir' || c.kind === 'rmdir').map((c) => c.kind + ' ' + c.path));
11091108
const rest = handedBack.filter((/** @type {any} */ c) => c && (c.kind === 'mkdir' || c.kind === 'rmdir') && folders.has(c.kind + ' ' + c.path));
11101109
const ignored = handedBack.length - rest.length;
1110+
// What the review applied is the files it was given, less the ones it could not apply and whatever else it handed
1111+
// back (what is handed back is still to be done). Folders are never its to apply: it hands back the ones this shell
1112+
// is to make and remove, and one it keeps back is applied by nobody (shellCommit keeps back the removal of a folder
1113+
// when a file in it could not be deleted), so it is not counted as applied.
1114+
const files = handed.filter((c) => c.kind === 'write' || c.kind === 'delete').length;
1115+
let applied = refused ? 0 : Math.max(0, files - failed - ignored);
11111116
if (ignored > 0) {
11121117
failed += ignored;
11131118
notes.push(note(ignored + ' entr' + (ignored === 1 ? 'y' : 'ies') + ' the review handed back to the shell ' + (ignored === 1 ? 'is' : 'are') + ' not ' + (ignored === 1 ? 'a folder' : 'folders') + ' the command made or removed, and ' + (ignored === 1 ? 'was' : 'were') + ' NOT applied'));

‎web/test/unit/shell-runtime-commit.test.js‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,47 @@ async function agent(rig, command, opts) {
351351
r.rt.dispose();
352352
});
353353

354+
await test('a review that fails one delete keeps the folder it was under: the shell counts what was applied (1 of 4), not the set less what failed', async () => {
355+
// levelcode-ai's own review, its answer handed on as agent.js hands it on
356+
const { commitShellChanges } = require(path.join(__dirname, '..', '..', '..', 'extensions', 'levelcode-ai', 'shellCommit.js'));
357+
for (const how of ['the folder it leaves cannot be made', 'Stop while the folder it leaves is being made']) {
358+
const r = createRig();
359+
r.ws.put('/scratch/d/a.txt', 'a\n');
360+
r.ws.put('/scratch/d/b.txt', 'b\n');
361+
ending(r, { exitCode: 0, changes: [mkdir('/scratch/m'), del('/scratch/d/a.txt', 'a\n'), del('/scratch/d/b.txt', 'b\n'), rmdir('/scratch/d')] });
362+
const ctx = {
363+
applyEdit: async () => false,
364+
applyDelete: async (/** @type {{ path: string }} */ o) => {
365+
if (o.path === 'd/b.txt') { return false; }
366+
await r.ws.fs.delete(r.ws.vscode.Uri.parse('levelcode-scratch:///scratch/' + o.path), { recursive: false });
367+
return true;
368+
},
369+
};
370+
const commit = async (/** @type {any[]} */ changes, /** @type {any} */ control) => {
371+
const x = await commitShellChanges(changes, ctx, { folders: () => [{ name: 'scratch', root: '/scratch' }], readNow: async (abs) => r.ws.bytes(abs), cancelled: () => control.cancelled() });
372+
return { rest: x.rest, notes: x.notes, failed: x.failed.length, refused: x.refused };
373+
};
374+
// the review applied d/a.txt, could not delete d/b.txt, and so did not hand back the removal of d: one change was made
375+
if (how === 'the folder it leaves cannot be made') {
376+
r.ws.fail('createDirectory', (k) => k.endsWith('/scratch/m'), new Error('quota'));
377+
const res = await agent(r, 'x', { commit });
378+
assert.strictEqual(res.code, 1);
379+
assert.ok(/PARTLY applied — 1 of 2 file changes/.test(res.stderr), res.stderr);
380+
assert.ok(/applied 1 of 4 file changes; stopped at \/scratch\/m: quota\./.test(res.stderr), how + ': ' + res.stderr);
381+
} else {
382+
r.ws.hang('createDirectory', (k) => k.endsWith('/scratch/m'));
383+
const child = r.rt.spawnAgent('x', { cwd: '/scratch', commit });
384+
const o = observe(child, 3000);
385+
await until(() => r.ws.calls.includes('createDirectory levelcode-scratch:/scratch/m'));
386+
child.kill('SIGTERM');
387+
await o.closed;
388+
assert.ok(/stopped while its file changes were being applied: 1 of 4 were applied\./.test(o.stderr), how + ': ' + o.stderr);
389+
}
390+
assert.deepStrictEqual([r.ws.list('/scratch'), r.ws.list('/scratch/d')], [['d'], ['b.txt']], how);
391+
r.rt.dispose();
392+
}
393+
});
394+
354395
await test('a review that THROWS: the status is 1, one honest note, and the shell applies nothing of its own', async () => {
355396
const r = createRig();
356397
ending(r, { exitCode: 0, changes: [wr('/scratch/a', 'a', null), mkdir('/scratch/d')] });

0 commit comments

Comments
 (0)