Distinguish reviewable batches and settle abandoned open submissions
This commit is contained in:
parent
6472a15a17
commit
7670364f67
6 changed files with 117 additions and 5 deletions
16
README.md
16
README.md
|
|
@ -103,6 +103,22 @@ Work comes in batches: `td claim --unit <file>` takes every open function of a f
|
||||||
them together. Byte-identical functions merge automatically; the others are peer-reviewed by three
|
them together. Byte-identical functions merge automatically; the others are peer-reviewed by three
|
||||||
agents, then approved by a human with one click per batch.
|
agents, then approved by a human with one click per batch.
|
||||||
|
|
||||||
|
### Batch visibility
|
||||||
|
|
||||||
|
`#/batches` lists all submissions, including failed and historical batches. The
|
||||||
|
In review tab requires an actual `in_review` or `peer_review` attempt; an `open`
|
||||||
|
batch alone is not proof that anything is reviewable. Awaiting merge separately
|
||||||
|
shows approved or merging attempts. `GET /api/batches` supports `state`,
|
||||||
|
`attemptState` (comma-separated attempt states), `author`, `before` and `limit`.
|
||||||
|
Filters apply before pagination.
|
||||||
|
|
||||||
|
The merge worker settles abandoned open code batches when they contain failed
|
||||||
|
attempts and no attempt remains live. It records the reason and an audit event,
|
||||||
|
preserving attempt feedback, task states, review votes and scores. This does not
|
||||||
|
approve historical code: missing bindings and stale payloads need current-source
|
||||||
|
verification and a new submission. Maintenance and actively merging batches are
|
||||||
|
not settled by this reconciliation.
|
||||||
|
|
||||||
## Building td
|
## Building td
|
||||||
|
|
||||||
`node tools/build-td/build.mjs` (Node 22.21) writes `dist/td-windows-x64.exe`, `dist/td-linux-x64` and
|
`node tools/build-td/build.mjs` (Node 22.21) writes `dist/td-windows-x64.exe`, `dist/td-linux-x64` and
|
||||||
|
|
|
||||||
|
|
@ -554,6 +554,11 @@ export function createApp(ctx) {
|
||||||
where.push(`b.state IN (${states.map(() => '?').join(',')})`);
|
where.push(`b.state IN (${states.map(() => '?').join(',')})`);
|
||||||
args.push(...states);
|
args.push(...states);
|
||||||
}
|
}
|
||||||
|
const attemptStates = (query.get('attemptState') ?? '').split(',').filter(Boolean);
|
||||||
|
if (attemptStates.length) {
|
||||||
|
where.push(`EXISTS (SELECT 1 FROM attempts a WHERE a.batch_id = b.id AND a.state IN (${attemptStates.map(() => '?').join(',')}))`);
|
||||||
|
args.push(...attemptStates);
|
||||||
|
}
|
||||||
if (query.get('author')) {
|
if (query.get('author')) {
|
||||||
where.push('u.name = ? COLLATE NOCASE');
|
where.push('u.name = ? COLLATE NOCASE');
|
||||||
args.push(query.get('author'));
|
args.push(query.get('author'));
|
||||||
|
|
|
||||||
|
|
@ -4,7 +4,7 @@ import { HttpError } from './http.js';
|
||||||
import { now } from './db.js';
|
import { now } from './db.js';
|
||||||
import { hasRole } from './accounts.js';
|
import { hasRole } from './accounts.js';
|
||||||
import { STATUS } from '../shared/compare.js';
|
import { STATUS } from '../shared/compare.js';
|
||||||
import { returnBatch, batchAttempts, settleBatch } from './batches.js';
|
import { returnBatch, batchAttempts, settleBatch, LIVE_STATES } from './batches.js';
|
||||||
import { withoutFunctions, functionRegions } from './split.js';
|
import { withoutFunctions, functionRegions } from './split.js';
|
||||||
import { addUnitsToCompileCommands } from '../shared/project.js';
|
import { addUnitsToCompileCommands } from '../shared/project.js';
|
||||||
import { RANK, bestResults, BIG_BATCH } from './verify.js';
|
import { RANK, bestResults, BIG_BATCH } from './verify.js';
|
||||||
|
|
@ -173,11 +173,31 @@ export class Merger {
|
||||||
this.log.info(`${stuck.length} batch(es) returned: a function dropped out and the rest cannot merge without it`);
|
this.log.info(`${stuck.length} batch(es) returned: a function dropped out and the rest cannot merge without it`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Imports and interrupted state transitions can leave an open batch with no work to merge.
|
||||||
|
// Settle its envelope without changing task states, attempts, reviews or scores.
|
||||||
|
settleInactive() {
|
||||||
|
const ph = LIVE_STATES.map(() => '?').join(',');
|
||||||
|
const batches = this.store.all(
|
||||||
|
`SELECT b.id FROM batches b WHERE b.state = 'open' AND b.kind = 'code'
|
||||||
|
AND EXISTS (SELECT 1 FROM attempts a WHERE a.batch_id = b.id AND a.state IN ('needs_cleanup', 'rejected', 'returned'))
|
||||||
|
AND NOT EXISTS (SELECT 1 FROM attempts a WHERE a.batch_id = b.id AND a.state IN (${ph}))`,
|
||||||
|
...LIVE_STATES,
|
||||||
|
).filter((b) => !this.active.has(b.id));
|
||||||
|
this.store.tx(() => {
|
||||||
|
for (const b of batches) {
|
||||||
|
const reason = 'No attempts remain in verification, review or approval; inspect the individual results and resubmit corrected work.';
|
||||||
|
settleBatch(this.store, b.id, reason);
|
||||||
|
this.store.event('batch_settled', null, { batch: b.id, reason });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
/** Starts merges while a lane is free; resolves when the merges it started are done. */
|
/** Starts merges while a lane is free; resolves when the merges it started are done. */
|
||||||
async pump() {
|
async pump() {
|
||||||
if (!this.stopped) {
|
if (!this.stopped) {
|
||||||
this.finishSkips();
|
this.finishSkips();
|
||||||
this.returnUnmergeable();
|
this.returnUnmergeable();
|
||||||
|
this.settleInactive();
|
||||||
}
|
}
|
||||||
const started = [];
|
const started = [];
|
||||||
while (!this.stopped && this.running < MERGE_SLOTS) {
|
while (!this.stopped && this.running < MERGE_SLOTS) {
|
||||||
|
|
|
||||||
|
|
@ -1131,12 +1131,13 @@ function renderQueue() {
|
||||||
// --- batches
|
// --- batches
|
||||||
|
|
||||||
let batchStates = '';
|
let batchStates = '';
|
||||||
|
let batchAttemptStates = '';
|
||||||
let batchRows = [];
|
let batchRows = [];
|
||||||
|
|
||||||
const BATCH_STATES = {
|
const BATCH_STATES = {
|
||||||
queued: 'Waiting for the worker.',
|
queued: 'Waiting for the worker.',
|
||||||
verifying: 'The worker is checking and compiling it.',
|
verifying: 'The worker is checking and compiling it.',
|
||||||
open: 'Checked; its functions are in peer review or waiting to merge.',
|
open: 'Not yet closed; each function has its own verification or review state.',
|
||||||
merging: 'Being merged onto main.',
|
merging: 'Being merged onto main.',
|
||||||
merged: 'On main.',
|
merged: 'On main.',
|
||||||
returned: 'Sent back to its author: see why below.',
|
returned: 'Sent back to its author: see why below.',
|
||||||
|
|
@ -1146,6 +1147,7 @@ const BATCH_STATES = {
|
||||||
async function loadBatches({ more = false } = {}) {
|
async function loadBatches({ more = false } = {}) {
|
||||||
const q = new URLSearchParams({ limit: '50' });
|
const q = new URLSearchParams({ limit: '50' });
|
||||||
if (batchStates) q.set('state', batchStates);
|
if (batchStates) q.set('state', batchStates);
|
||||||
|
if (batchAttemptStates) q.set('attemptState', batchAttemptStates);
|
||||||
const author = $('batches-author').value.trim();
|
const author = $('batches-author').value.trim();
|
||||||
if (author) q.set('author', author);
|
if (author) q.set('author', author);
|
||||||
if (more && batchRows.length) q.set('before', String(batchRows[batchRows.length - 1].id));
|
if (more && batchRows.length) q.set('before', String(batchRows[batchRows.length - 1].id));
|
||||||
|
|
@ -1180,6 +1182,7 @@ function renderBatches(hasMore) {
|
||||||
for (const t of document.querySelectorAll('#batches-tabs .tab')) {
|
for (const t of document.querySelectorAll('#batches-tabs .tab')) {
|
||||||
t.addEventListener('click', () => {
|
t.addEventListener('click', () => {
|
||||||
batchStates = t.dataset.states;
|
batchStates = t.dataset.states;
|
||||||
|
batchAttemptStates = t.dataset.attemptStates ?? '';
|
||||||
for (const x of document.querySelectorAll('#batches-tabs .tab')) x.classList.toggle('active', x === t);
|
for (const x of document.querySelectorAll('#batches-tabs .tab')) x.classList.toggle('active', x === t);
|
||||||
loadBatches().catch((e) => toast(e.message));
|
loadBatches().catch((e) => toast(e.message));
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,7 @@
|
||||||
<meta name="color-scheme" content="dark light">
|
<meta name="color-scheme" content="dark light">
|
||||||
<title>Clank Together</title>
|
<title>Clank Together</title>
|
||||||
<link rel="icon" href="/favicon.svg" type="image/svg+xml">
|
<link rel="icon" href="/favicon.svg" type="image/svg+xml">
|
||||||
<link rel="stylesheet" href="/style.css?v=batches-20261027">
|
<link rel="stylesheet" href="/style.css?v=batch-states-20261027">
|
||||||
</head>
|
</head>
|
||||||
<body>
|
<body>
|
||||||
<header class="topbar">
|
<header class="topbar">
|
||||||
|
|
@ -302,7 +302,8 @@
|
||||||
<div class="tabs small" role="tablist" id="batches-tabs">
|
<div class="tabs small" role="tablist" id="batches-tabs">
|
||||||
<button type="button" class="tab active" data-states="">All</button>
|
<button type="button" class="tab active" data-states="">All</button>
|
||||||
<button type="button" class="tab" data-states="queued,verifying">Checking</button>
|
<button type="button" class="tab" data-states="queued,verifying">Checking</button>
|
||||||
<button type="button" class="tab" data-states="open,merging">In review</button>
|
<button type="button" class="tab" data-states="open" data-attempt-states="in_review,peer_review">In review</button>
|
||||||
|
<button type="button" class="tab" data-states="open,merging" data-attempt-states="approved,merging">Awaiting merge</button>
|
||||||
<button type="button" class="tab" data-states="returned">Sent back</button>
|
<button type="button" class="tab" data-states="returned">Sent back</button>
|
||||||
<button type="button" class="tab" data-states="merged">Merged</button>
|
<button type="button" class="tab" data-states="merged">Merged</button>
|
||||||
</div>
|
</div>
|
||||||
|
|
@ -412,6 +413,6 @@
|
||||||
<div class="hazard thin" aria-hidden="true"></div>
|
<div class="hazard thin" aria-hidden="true"></div>
|
||||||
<p>Clank Together · source is open: <code id="clone-cmd">git clone https://clank.rodecomp.online/git/clank/game-decomp.git</code></p>
|
<p>Clank Together · source is open: <code id="clone-cmd">git clone https://clank.rodecomp.online/git/clank/game-decomp.git</code></p>
|
||||||
</footer>
|
</footer>
|
||||||
<script src="/app.js?v=batches-20261027"></script>
|
<script src="/app.js?v=batch-states-20261027"></script>
|
||||||
</body>
|
</body>
|
||||||
</html>
|
</html>
|
||||||
|
|
|
||||||
|
|
@ -1907,3 +1907,70 @@ test('the batch list shows every batch, also those sent back before review, filt
|
||||||
assert.deepEqual((await call('GET', '/api/batches?author=blister&limit=1', null, token)).body.map((b) => b.id), [merged]);
|
assert.deepEqual((await call('GET', '/api/batches?author=blister&limit=1', null, token)).body.map((b) => b.id), [merged]);
|
||||||
ctx.store.run(`UPDATE attempts SET state = 'rejected' WHERE batch_id = ?`, sentBack);
|
ctx.store.run(`UPDATE attempts SET state = 'rejected' WHERE batch_id = ?`, sentBack);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('review batch filtering precedes pagination and ignores stale open envelopes', async () => {
|
||||||
|
const token = await activeUser('batchfilter');
|
||||||
|
const user = ctx.accounts.byName('batchfilter');
|
||||||
|
const task = ctx.store.get('SELECT id FROM tasks LIMIT 1').id;
|
||||||
|
const ids = [];
|
||||||
|
const seed = (states) => {
|
||||||
|
const id = Number(ctx.store.run(`INSERT INTO batches (user_id, base_commit, files_json, state, created_at) VALUES (?, ?, '{}', 'open', ?)`, user.id, ctx.repo.head(), now()).lastInsertRowid);
|
||||||
|
ids.push(id);
|
||||||
|
for (const state of states) ctx.store.run(`INSERT INTO attempts (task_id, user_id, base_commit, files_json, state, batch_id, created_at) VALUES (?, ?, ?, '{}', ?, ?, ?)`, task, user.id, ctx.repo.head(), state, id, now());
|
||||||
|
return id;
|
||||||
|
};
|
||||||
|
try {
|
||||||
|
const human = seed(['in_review']);
|
||||||
|
const peer = seed(['peer_review', 'needs_cleanup']);
|
||||||
|
const approved = seed(['approved']);
|
||||||
|
seed(['needs_cleanup', 'withdrawn', 'superseded']);
|
||||||
|
const query = '/api/batches?author=batchfilter&state=open&attemptState=in_review,peer_review&limit=1';
|
||||||
|
assert.deepEqual((await call('GET', query, null, token)).body.map((b) => b.id), [peer]);
|
||||||
|
assert.deepEqual((await call('GET', `${query}&before=${peer}`, null, token)).body.map((b) => b.id), [human]);
|
||||||
|
assert.deepEqual((await call('GET', '/api/batches?author=batchfilter&attemptState=approved,merging', null, token)).body.map((b) => b.id), [approved]);
|
||||||
|
assert.equal((await call('GET', '/api/batches?author=batchfilter', null, token)).body.length, 4);
|
||||||
|
} finally {
|
||||||
|
for (const id of ids) {
|
||||||
|
ctx.store.run('DELETE FROM attempts WHERE batch_id = ?', id);
|
||||||
|
ctx.store.run('DELETE FROM batches WHERE id = ?', id);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('the merger settles abandoned open batches without changing attempts, tasks, or live work', async () => {
|
||||||
|
await activeUser('batchsettle');
|
||||||
|
const user = ctx.accounts.byName('batchsettle');
|
||||||
|
const task = ctx.store.get('SELECT * FROM tasks LIMIT 1');
|
||||||
|
const ids = [];
|
||||||
|
const seed = (states, kind = 'code') => {
|
||||||
|
const id = Number(ctx.store.run(`INSERT INTO batches (user_id, base_commit, files_json, state, kind, created_at) VALUES (?, ?, '{}', 'open', ?, ?)`, user.id, ctx.repo.head(), kind, now()).lastInsertRowid);
|
||||||
|
ids.push(id);
|
||||||
|
for (const state of states) ctx.store.run(`INSERT INTO attempts (task_id, user_id, base_commit, files_json, state, batch_id, score, result_json, created_at) VALUES (?, ?, ?, '{}', ?, ?, 0.5, ?, ?)`, task.id, user.id, ctx.repo.head(), state, id, JSON.stringify({ feedback: { summary: 'Historical binding failure' } }), now());
|
||||||
|
return id;
|
||||||
|
};
|
||||||
|
try {
|
||||||
|
const abandoned = seed(['needs_cleanup', 'withdrawn', 'superseded']);
|
||||||
|
const rejected = seed(['rejected', 'returned']);
|
||||||
|
const live = ['queued', 'verifying', 'in_review', 'peer_review', 'approved', 'merging'].map((state) => seed(['needs_cleanup', state]));
|
||||||
|
const active = seed(['needs_cleanup']);
|
||||||
|
ctx.merger.active.add(active);
|
||||||
|
const maintenance = seed([], 'maintenance');
|
||||||
|
const empty = seed([]);
|
||||||
|
const before = ctx.store.all('SELECT * FROM attempts WHERE batch_id = ?', abandoned);
|
||||||
|
ctx.merger.settleInactive();
|
||||||
|
for (const id of [abandoned, rejected]) assert.equal(ctx.store.get('SELECT state FROM batches WHERE id = ?', id).state, 'returned');
|
||||||
|
for (const id of [...live, active, maintenance, empty]) assert.equal(ctx.store.get('SELECT state FROM batches WHERE id = ?', id).state, 'open');
|
||||||
|
assert.deepEqual(ctx.store.all('SELECT * FROM attempts WHERE batch_id = ?', abandoned), before);
|
||||||
|
assert.deepEqual(ctx.store.get('SELECT * FROM tasks WHERE id = ?', task.id), task);
|
||||||
|
const eventCount = () => ctx.store.get(`SELECT COUNT(*) n FROM events WHERE type = 'batch_settled' AND json_extract(data, '$.batch') = ?`, abandoned)?.n;
|
||||||
|
assert.equal(eventCount(), 1);
|
||||||
|
ctx.merger.settleInactive();
|
||||||
|
assert.equal(eventCount(), 1, 'reconciliation is idempotent');
|
||||||
|
} finally {
|
||||||
|
for (const id of ids) {
|
||||||
|
ctx.merger.active.delete(id);
|
||||||
|
ctx.store.run('DELETE FROM attempts WHERE batch_id = ?', id);
|
||||||
|
ctx.store.run('DELETE FROM batches WHERE id = ?', id);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue