From 7a3e85a38c99d47a220359e006d04e479bdcb51a Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Mon, 12 Oct 2026 14:53:35 -0500 Subject: [PATCH] dewey: row 63 (#1556) round 1 review packet, changes (test-only) Co-Authored-By: Claude Opus 5.5 --- .../dewey/work/queue-63-review/r1/mutants.py | 33 +++++++++++ .../dewey/work/queue-63-review/r1/mutants.txt | 17 ++++++ .../work/queue-63-review/r1/proof-tests.mjs | 59 +++++++++++++++++++ .../dewey/work/queue-63-review/r1/verdict.md | 40 +++++++++++++ 4 files changed, 149 insertions(+) create mode 100644 agents/dewey/work/queue-63-review/r1/mutants.py create mode 100644 agents/dewey/work/queue-63-review/r1/mutants.txt create mode 100644 agents/dewey/work/queue-63-review/r1/proof-tests.mjs create mode 100644 agents/dewey/work/queue-63-review/r1/verdict.md diff --git a/agents/dewey/work/queue-63-review/r1/mutants.py b/agents/dewey/work/queue-63-review/r1/mutants.py new file mode 100644 index 000000000..4f51e5c88 --- /dev/null +++ b/agents/dewey/work/queue-63-review/r1/mutants.py @@ -0,0 +1,33 @@ +# Row 63 review mutants (Dewey): each swaps one exact string in the scratch copy, runs the tasks +# verbs and tree tests, restores the file, and reports killed (tests fail) or SURVIVED. +import subprocess, sys, os +M = os.path.expanduser('~/dewey-scratch/r63mut') +env = dict(os.environ, TMPDIR=os.path.expanduser('~/dewey-scratch/tmp')) +V = 'packages/tasks/src/verbs.mjs' +T = ['packages/tasks/tests/verbs.test.mjs', 'packages/tasks/tests/tree.test.mjs'] +ALL = ['packages/tasks/tests/*.test.mjs'] +muts = [ + ('located: first relation only', V, " for (const r of list) {", " for (const r of list.slice(0, 1)) {", ALL), + ('located: a missing task passes', V, "const x = await ctx.sync('GET', `/tasks/${r.other}`);\n if (outcome(x) !== 'ok') fail(refusal(x));", "const x = await ctx.sync('GET', `/tasks/${r.other}`);\n if (outcome(x) !== 'ok') continue;", ALL), + ('relation: keeps the root project', V, "return { kind: x.kind, other: other.task, project: other.project };", "return { kind: x.kind, other: other.task, project: ctx.project };", ALL), + ('step: re-read on the root board', V, " after = await read(at);\n } catch {\n // the re-read failed too", " after = await read({ ...at, project: ctx.project, view: ctx.view });\n } catch {\n // the re-read failed too", ALL), + ('handle: final read on the root board', V, " after = await read(at);\n } catch {\n // handled below: a failed", " after = await read({ ...at, project: ctx.project, view: ctx.view });\n } catch {\n // handled below: a failed", ALL), + ('create: final read on the root board', V, " after = await read(at);\n } catch {\n // handled below, as in handle()", " after = await read({ ...at, project: ctx.project, view: ctx.view });\n } catch {\n // handled below, as in handle()", ALL), + ('create: fallback todo of the root', V, "taskFields(r.json, at.view.ids.todo)", "taskFields(r.json, ctx.view.ids.todo)", ALL), + ('create: authorize before located', V, " await located(plan.others);\n ctx.broker.authorize(cap, 'task.create', { decision: plan.decision, target: null });", " ctx.broker.authorize(cap, 'task.create', { decision: plan.decision, target: null });\n await located(plan.others);", ALL), + ('installed: outside is not-installed', V, " if (!ctx.tree.has(project)) fail('task-project');\n", "", ALL), + ('create: ref under the root', V, "const taskRef = ref(at.project, id);", "const taskRef = ref(ctx.project, id);", ALL), +] +only = sys.argv[1:] +for name, f, old, new, tests in muts: + if only and not any(o in name for o in only): continue + p = os.path.join(M, f); src = open(p).read() + n = src.count(old) + if n != 1: print(f'BAD-ANCHOR ({n}) {name}', flush=True); continue + open(p, 'w').write(src.replace(old, new)) + try: + r = subprocess.run(['node', '--test', *tests], cwd=M, env=env, capture_output=True, text=True, timeout=600) + finally: + open(p, 'w').write(src) + fails = [l for l in r.stdout.splitlines() if l.startswith('not ok')] + print(('killed ' if r.returncode else 'SURVIVED') + f' {name}' + (f' ({len(fails)} failing)' if fails else ''), flush=True) diff --git a/agents/dewey/work/queue-63-review/r1/mutants.txt b/agents/dewey/work/queue-63-review/r1/mutants.txt new file mode 100644 index 000000000..82cee51ed --- /dev/null +++ b/agents/dewey/work/queue-63-review/r1/mutants.txt @@ -0,0 +1,17 @@ +SURVIVED located: first relation only +killed located: a missing task passes +killed relation: keeps the root project +SURVIVED step: re-read on the root board +BAD-ANCHOR (2) handle: final read on the root board +SURVIVED create: fallback todo of the root +killed create: authorize before located +killed installed: outside is not-installed +killed create: ref under the root +SURVIVED handle: final read on the root board +SURVIVED create: final read on the root board +-- with Dewey's four proof tests appended to verbs.test.mjs in the scratch copy -- +killed located: first relation only +killed step: re-read on the root board +killed handle: final read on the root board +killed create: final read on the root board +killed create: fallback todo of the root diff --git a/agents/dewey/work/queue-63-review/r1/proof-tests.mjs b/agents/dewey/work/queue-63-review/r1/proof-tests.mjs new file mode 100644 index 000000000..f18a4e9ff --- /dev/null +++ b/agents/dewey/work/queue-63-review/r1/proof-tests.mjs @@ -0,0 +1,59 @@ +// Dewey's four proof tests for row 63 round 1 (#1556). Appended to the end of +// packages/tasks/tests/verbs.test.mjs in a scratch copy of candidate ec28d736, they pass +// there and kill the five mutants that survived the packet's tests (mutants.txt). They use +// that file's own tree(), task(), refusedWith, TREE, world, assert and FakeVikunja. Take, +// rename or rewrite them as you like. +const treeWith = async (t, extra) => { + const w = await world(t, { ...TREE, ...extra }); + const child = w.fake.project('Child', w.owner, { parent: w.project }); + const board = w.fake.install(child); + await w.adapter.start('demo'); + assert.equal(w.adapter.status()[0].state, 'ready', w.adapter.status()[0].refused); + return { w, child, board }; +}; +test('DEWEY a child verb and a child create record their final read', async (t) => { + const { w, child } = await tree(t); + const pm = w.agent('pm'); + const made = await task(w, { project: child }); + assert.notEqual(w.snapshots(made.task_ref).at(-1).etag, null); + await w.call(pm, 'task.assign', { task_ref: made.task_ref, role: 'coder' }); + assert.notEqual(w.snapshots(made.task_ref).at(-1).etag, null); +}); +test('DEWEY a child create whose final read fails records the child todo bucket', async (t) => { + const fake = new FakeVikunja(); + let armed = false; + const fetch = async (url, init = {}) => { + const res = await fake.fetch(url, init); + if (armed && (init.method ?? 'GET') === 'POST' && /\/projects\/\d+\/tasks$/.test(new URL(url).pathname)) { + armed = false; + fake.fault('GET', /^\/tasks\/\d+$/, 0); + } + return res; + }; + const { w, child, board } = await treeWith(t, { fake, fetch }); + w.agent('pm'); + armed = true; + const r = await task(w, { project: child }); + assert.equal(w.snapshots(r.task_ref).at(-1).fields.bucket, board.buckets.todo); +}); +test('DEWEY a lost answer in the child settles on the child board', async (t) => { + const { w, child } = await tree(t); + const pm = w.agent('pm'); + const coder = w.agent('coder'); + const { task_ref } = await task(w, { project: child }); + await w.call(pm, 'task.assign', { task_ref, role: 'coder' }); + w.fake.fault('PUT', /\/buckets\/\d+\/tasks$/, 500, { apply: true }); + await w.call(coder, 'task.update.assigned', { task_ref, state: 'in-progress' }); + assert.equal(w.events('task.state').length, 1); +}); +test('DEWEY every related task is read, not only the first', async (t) => { + const { w, child } = await tree(t); + const pm = w.agent('pm'); + const root = await task(w); + const other = await task(w); + const id = Number(other.task_ref.split('/')[1]); + await assert.rejects( + w.call(pm, 'task.schedule', { task_ref: root.task_ref, relations: { add: [{ kind: 'related', task_ref: other.task_ref }, { kind: 'blocking', task_ref: `vikunja:${child}/${id}` }] } }), + refusedWith('task-moved'), + ); +}); diff --git a/agents/dewey/work/queue-63-review/r1/verdict.md b/agents/dewey/work/queue-63-review/r1/verdict.md new file mode 100644 index 000000000..304f028af --- /dev/null +++ b/agents/dewey/work/queue-63-review/r1/verdict.md @@ -0,0 +1,40 @@ +**Dewey: changes, round 1, candidate manifest `ec28d73646c62a78c5f5af102193c867f4709f654520510e0f0db5527f4cc2bb`** + +The code is right as far as I can find. One finding, test-only: five of the sites this row moved from the root to the task's own project have no test that fails if they go back to the root. + +**What I checked** + +- `build.patch` applied with `git apply --index` to a detached worktree of `08941d8b` checks 6/6. None of the six paths, and not `fake.mjs`, changed between `08941d8b` and `refactor` HEAD. +- On the candidate, sequentially, 19:47:19-19:47:41Z, all rc=0: tasks 80, harness 59, business 60, bus 95. Same counts as your gate. +- `ctx.tree` is always set before a verb runs. `startup` sets it, and `start()` runs reconcile (with `discover` when descendants is on) before `ready`. Verbs, `tick` and `reconcile` share the business's `serial()` queue, so the tree can't change in the middle of a verb. +- No `ctx.view` is left in `read`, `step`, `handle` or `create`'s writes. The `ctx.project` and `ctx.tree` uses left are `installed()` and create's default project. +- `project-not-installed` needs no registry entry. No other package lists task refusal codes. +- The harness `project` property and its test: integer, minimum 1, optional, on `task_create` only. + +**Finding (blocking): the child path's re-reads and fallback are untested** + +My mutants (`mutants.py`, `mutants.txt`) ran in a scratch copy against every `packages/tasks` test. 5 of 10 survived. The `BAD-ANCHOR` line in `mutants.txt` is my first anchor for `handle`'s final read, which also matched create's. The two lines after it are the rerun with separate anchors. + +| Mutant | What a child task would get | +|---|---| +| `handle`'s final read uses the root's project and view | The verb succeeds, but the snapshot carries the pre-write read's `updated` and a null etag. | +| `create`'s final read uses the root's project and view | The create falls back to the POST answer, with a null etag. | +| `create`'s fallback uses `ctx.view.ids.todo` | A child create whose final read fails records the root's todo bucket id as the child task's bucket. | +| `step`'s re-read uses the root's project and view | A child write that landed but lost its answer refuses `write-uncertain` instead of settling. | +| `located` reads only the first relation | With two or more relations, a later ref that names the wrong project gets written. This is the hole choice 2 closes. | + +The first three fail silently, because a failed final read is tolerated by design. A regression there would leave wrong evidence and no failing test. The packet's tests always go through the root versions of these paths (lines 210-230 and 315-375). + +Asked: tests in `verbs.test.mjs` that kill these five. `proof-tests.mjs` in this directory has four that do. Appended to `verbs.test.mjs` in the scratch copy, the file is 28/28, and all five mutants are killed (the last block of `mutants.txt`). The other five mutants were killed by your tests: a missing related task passing, `relation()` keeping the root project, create authorizing before `located`, an outside ref giving `project-not-installed`, and create's ref under the root. No code change is asked. + +**Your five choices: I agree with all five.** + +1. One code, `project-not-installed`, for create's watch-only, unknown and outside projects. A create's `project` names where to write, and a ref outside the tree keeps `task-project`. The distinction holds, and the README table says it. +2. Relations read before authorize. This closes a real hole. The read tells a caller whether a task id exists in a project the sync token can see (`task-moved` or `task-not-found`). `read()` already did that for `task_ref` before this row, so nothing new is disclosed. The cost is at most 50 reads on create, or 100 on schedule (50 add plus 50 remove). +3. `tree.test.mjs`. It is the one assertion whose behaviour the brief changes, and the comment says where the installed cases live. +4. Create's decision binds no target. Agreed for this row. A create decision binds nothing about the task today, the title included, so the project is no worse off. A follow-up row would be the place if Sage wants decisions to name it. +5. `fake.mjs` untouched. Nothing more was needed. + +**Non-blocking** + +- `packages/tasks/README.md` line 194 is 118 characters. The text added before "`task.created` is recorded" wasn't rewrapped. Rewrap it in the next round if you like.