From 4efc56fa8dff0c972b11c6689d19622f9b1df1f1 Mon Sep 17 00:00:00 2001 From: ragnarok-clanker Date: Mon, 26 Oct 2026 18:06:07 +0100 Subject: [PATCH] Let agents revise merged functions through region claims A region whose functions are all on main can be claimed, and its done tasks submitted as revisions: competing candidates verified and peer reviewed like new work, while the task stays done whatever the outcome. Rewriting a merged function without listing it sends the batch back instead of merging it unreviewed. Admins can force a done task back to open. --- src/server/app.js | 15 ++++-- src/server/regions.js | 17 ++++--- src/server/tasks.js | 7 ++- src/server/verify.js | 17 +++++-- test/server.test.js | 106 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 16 deletions(-) diff --git a/src/server/app.js b/src/server/app.js index 8afea5d..ce6be00 100644 --- a/src/server/app.js +++ b/src/server/app.js @@ -672,9 +672,11 @@ export function createApp(ctx) { return guided({ ...r, baseCommit: repo.head(), - note: r.alsoClaimedBy.length - ? `Also claimed by ${r.alsoClaimedBy.join(', ')}: you may submit any function of it; the first accepted candidate of each function merges. Prefer an unclaimed region (td_regions free) unless you mean to help here.` - : 'Port every function of the region, compile and check locally, then submit them together (td submit --all).', + note: !r.leased.length && r.functions.every((f) => f.state === 'done' || f.state === 'skipped') + ? 'Every function of this region is on main. To improve merged code, edit it, check locally, and submit the rewritten functions by task id (td submit --files ...): each one is a revision, reviewed like new work; main and the task stay as they are if it is sent back.' + : r.alsoClaimedBy.length + ? `Also claimed by ${r.alsoClaimedBy.join(', ')}: you may submit any function of it; the first accepted candidate of each function merges. Prefer an unclaimed region (td_regions free) unless you mean to help here.` + : 'Port every function of the region, compile and check locally, then submit them together (td submit --all).', }); }); @@ -946,7 +948,12 @@ export function createApp(ctx) { const body = await readJson(req, 4096); const t = tasks.get(intParam(params.id)); if (body.priority !== undefined) store.run('UPDATE tasks SET priority = ? WHERE id = ?', Number(body.priority) | 0, t.id); - if (body.state === 'open' && t.state !== 'done') tasks.reopen(t.id); + // A done function is reopened only with force: merged code that is wrong and must be redone. + // Improving it does not need this; a region claim lets agents submit it as a revision. The + // tasks_done_stays_done trigger lets a done task move to 'open' only. + if (body.state === 'open' && t.state === 'done' && body.force === true) { + store.run(`UPDATE tasks SET state = 'open', lease_user = NULL, lease_expires = NULL, updated_at = ? WHERE id = ?`, now(), t.id); + } else if (body.state === 'open' && t.state !== 'done') tasks.reopen(t.id); return tasks.get(t.id); }); diff --git a/src/server/regions.js b/src/server/regions.js index 2aaba91..cf5ce68 100644 --- a/src/server/regions.js +++ b/src/server/regions.js @@ -3,9 +3,11 @@ // An agent lists the regions (what is open, done, in flight, and who already works there), looks // at the candidates in its own checkout, and registers the one it takes. That leases every open // function of the region to it in one step. A region may be claimed by several agents; each may -// then submit any function of it that is not on main yet. Competing candidates of a function are -// verified and reviewed independently, and the first accepted one merges (merge.js supersedes the -// others), so two agents' work combines per function. +// then submit any function of it. Competing candidates of a function are verified and reviewed +// independently, and the first accepted one merges (merge.js supersedes the others), so two +// agents' work combines per function. A function already on main is submitted the same way, as a +// revision: reviewed like any candidate, and the task stays done whatever its outcome. A region +// whose functions are all on main can still be claimed for that. import { HttpError } from './http.js'; import { now } from './db.js'; @@ -118,8 +120,6 @@ export class Regions { return this.store.tx(() => { const members = this.members(kind, key); if (!members.length) throw new HttpError(404, `no functions in ${kind} ${key}`); - const left = members.filter((m) => !FINISHED.includes(m.state)); - if (!left.length) throw new HttpError(409, `${key} is finished: every function is on main`); const expires = now() + this.leaseMs; this.store.run( `INSERT INTO region_claims (user_id, token_id, kind, key, created_at, expires_at) VALUES (?, ?, ?, ?, ?, ?) @@ -174,12 +174,15 @@ export class Regions { }); } - /** Unfinished functions of `user`'s regions that someone else holds or has on its way. */ + /** + * Functions of `user`'s regions they may submit without holding them: unfinished ones someone + * else holds or has on its way, and ones already on main (revisions). + */ sharedTasks(user, own = new Set()) { const out = []; for (const r of this.mine(user)) { for (const m of this.members(r.kind, r.key)) { - if (own.has(m.id) || FINISHED.includes(m.state) || m.state === 'merging' || CLAIMABLE.includes(m.state)) continue; + if (own.has(m.id) || m.state === 'skipped' || m.state === 'merging' || CLAIMABLE.includes(m.state)) continue; out.push({ taskId: m.id, symbol: m.symbol_id, state: m.state }); } } diff --git a/src/server/tasks.js b/src/server/tasks.js index 018279d..636ed6a 100644 --- a/src/server/tasks.js +++ b/src/server/tasks.js @@ -283,13 +283,16 @@ export class Tasks { /** * A task `user` may submit: one they hold, or (a competing candidate) one in a region they - * claimed that is not on main yet. Returns the task and whether it is theirs. + * claimed. A function already on main is submitted as a revision: a competing candidate that + * goes through verification and peer review like any other, while the task stays done (a + * revision sent back leaves main and the task as they were). Returns the task and whether it is theirs. */ requireSubmittable(user, taskId) { const t = this.get(taskId); if (t.state === 'leased' && t.lease_user === user.id) return { task: t, own: true }; if (this.regions?.covers(user, t)) { - if (['done', 'skipped', 'merging'].includes(t.state)) throw new HttpError(409, `task ${taskId} (${t.symbol_id}) is ${t.state === 'merging' ? 'being merged' : 'already on main'}; leave it out`); + if (t.state === 'merging') throw new HttpError(409, `task ${taskId} (${t.symbol_id}) is being merged; leave it out`); + if (t.state === 'skipped') throw new HttpError(409, `task ${taskId} (${t.symbol_id}) is skipped; leave it out`); return { task: t, own: CLAIMABLE.includes(t.state) }; } throw new HttpError(409, `you do not hold task ${taskId} (${t.symbol_id}) and no region you claimed covers it`); diff --git a/src/server/verify.js b/src/server/verify.js index fa2dd78..bc6c790 100644 --- a/src/server/verify.js +++ b/src/server/verify.js @@ -414,8 +414,10 @@ export class Verifier { * Unknown on main (no fingerprint yet, e.g. before the first rebuild) is not counted. */ // With `userId`, only functions someone else is working on count (held by another account, or in - // another account's submission on its way to main). Other neighbours may change: their status is - // measured again, and the regression check still protects whatever already matches. + // another account's submission on its way to main), and functions on main whose source the + // submission rewrote without listing them: those are revisions and need their own review. + // Other neighbours may change: their status is measured again, and the regression check still + // protects whatever already matches. changedOutside(check, files, targets, userId = null) { const fileSet = new Set(Object.keys(files)); const seen = new Map(); @@ -427,7 +429,16 @@ export class Verifier { else if (main.code_hash && main.code_hash !== f.codeHash) out.push({ id, file: f.file, why: 'compiled code changed' }); } if (userId == null) return out; - return out.filter((o) => this.workedOnByOthers(o.id, userId)); + return out.filter((o) => this.workedOnByOthers(o.id, userId) || this.unlistedRevision(o, files)); + } + + /** A function on main whose source this submission rewrote, though it is not one of its tasks. */ + unlistedRevision(o, files) { + const t = this.store.get('SELECT state FROM tasks WHERE symbol_id = ?', o.id); + if (!t || !['done', 'skipped'].includes(t.state)) return false; + if (this.unchangedFromBase(o.id, files, 'HEAD')) return false; + o.why = 'already on main and rewritten: list it in taskIds to submit it as a revision'; + return true; } workedOnByOthers(symbolId, userId) { diff --git a/test/server.test.js b/test/server.test.js index aaa9450..b3c0afa 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1233,6 +1233,112 @@ test('regions: two agents claim one file, the first accepted candidate merges, t ctx.verifier.check = realCheck; }); +test('revisions: merged functions of a claimed region are reviewed again; the task stays done whatever the outcome', async () => { + const author = await activeUser('rvauthor'); + delete ctx.merger.pump; + const v = ctx.store.currentVersion(); + const unit = 'src/revision.cpp'; + const a = ctx.store.ensureSymbol({ kind: 'function', versionId: v.id, addr: 0x402800, size: 8, unit }).symbol.id; + const b = ctx.store.ensureSymbol({ kind: 'function', versionId: v.id, addr: 0x402840, size: 8, unit }).symbol.id; + ctx.tasks.generate(); + const taskA = ctx.store.get('SELECT id FROM tasks WHERE symbol_id = ?', a).id; + const taskB = ctx.store.get('SELECT id FROM tasks WHERE symbol_id = ?', b).id; + const onMain = `// FUNCTION: ${a}\nint ra() { return 1; }\n\n// FUNCTION: ${b}\nint rb() { return 2; }\n`; + ctx.repo.commitFiles({ [unit]: onMain }, 'seed revision.cpp', 'x '); + ctx.store.run( + `INSERT INTO func_status (symbol_id, file, status, score, commit_hash, updated_at, confirmed, code_hash) VALUES (?, ?, 'mismatch', 0.7, 'x', ?, 1, 'a-main'), (?, ?, 'mismatch', 0.6, 'x', ?, 1, 'b-main')`, + a, unit, now(), b, unit, now(), + ); + ctx.store.run(`UPDATE tasks SET state = 'done' WHERE id IN (?, ?)`, taskA, taskB); + + // Stubbed compiler: a body's score and code follow its text. + const realCheck = ctx.verifier.check; + const realMergeCheck = ctx.verifier.mergeCheck; + const bodyOf = (text, name) => new RegExp(`int ${name}\\(\\) \\{ ([^}]*) \\}`).exec(text)?.[1] ?? ''; + ctx.verifier.check = async (files) => { + const text = files[unit] ?? ''; + const ra = bodyOf(text, 'ra'); + const rb = bodyOf(text, 'rb'); + const functions = [ + { id: a, status: STATUS.MISMATCH, score: ra.includes('worse') ? 0.4 : ra.includes('better') ? 0.8 : 0.7, file: unit, codeHash: ra === 'return 1;' ? 'a-main' : `a-${ra}`, relocations: [], size: { ours: 8, original: 8 } }, + { id: b, status: STATUS.MISMATCH, score: 0.6, file: unit, codeHash: rb === 'return 2;' ? 'b-main' : `b-${rb}`, relocations: [], size: { ours: 8, original: 8 } }, + ]; + return { check: { annotationErrors: [], units: [{ file: unit, compiled: true, lintErrors: [], unannotated: [], bindings: {}, inferred: [], includes: [], errors: [], functions }] }, rec: { conflicts: [], unknown: [], learned: [], learnedAddresses: [] }, changedHeaders: [], units: [unit] }; + }; + ctx.verifier.mergeCheck = async (batch, files) => ({ ...(await ctx.verifier.check(files)), reused: 0, compiled: 1 }); + const batchState = async (id) => (await call('GET', `/api/batches/${id}`, null, author)).body; + const settle = (id) => waitFor(async () => { const st = await batchState(id); return !['queued', 'verifying'].includes(st.state) && st; }); + const submit = (taskIds, text) => call('POST', '/api/batches', { taskIds, baseCommit: ctx.repo.head(), files: { [unit]: text } }, author); + const verdicts = async (attemptId, verdict, names) => { + for (const n of names) { + await activeUser(n); + ctx.store.run(`INSERT INTO peer_reviews (attempt_id, reviewer_id, token_id, verdict, weight, reason, created_at) VALUES (?, ?, NULL, ?, 1, 'read', ?)`, attemptId, ctx.accounts.byName(n).id, verdict, now()); + } + return ctx.peers.decide(ctx.store.get('SELECT * FROM attempts WHERE id = ?', attemptId)); + }; + + try { + // Without a claim the merged function cannot be submitted. + assert.equal((await submit([taskA], onMain.replace('return 1;', 'int better = 1; return better;'))).status, 409); + + // A region whose functions are all on main can still be claimed, for revisions. + const claim = await call('POST', '/api/regions/claim', { kind: 'unit', key: unit }, author); + assert.equal(claim.status, 200, JSON.stringify(claim.body)); + assert.deepEqual(claim.body.leased, []); + assert.match(claim.body.note, /revision/); + const me = (await call('GET', '/api/me', null, author)).body; + assert.ok(me.regionTasks.some((t) => t.taskId === taskA), 'plain td submit finds rewritten merged functions'); + + // A revision that moves a confirmed function away from the original goes back. + const worse = await submit([taskA], onMain.replace('return 1;', 'int worse = 1; return worse;')); + assert.equal(worse.status, 200, JSON.stringify(worse.body)); + assert.equal(worse.body.attempts[0].competing, true); + assert.match((await settle(worse.body.batchId)).functions[0].summary, /breaks 1 function/); + assert.equal(ctx.tasks.get(taskA).state, 'done'); + + // Rewriting a merged function the submission does not list is refused: it would merge unreviewed. + const better = onMain.replace('return 1;', 'int better = 1; return better;'); + const riding = await submit([taskA], better.replace('return 2;', 'int two = 2; return two;')); + const ridingState = await settle(riding.body.batchId); + assert.match(ridingState.functions[0].summary, new RegExp(`does not hold: ${b}`)); + assert.equal(ctx.tasks.get(taskB).state, 'done'); + + // Peers find a difference: the revision goes back, main and the task stay as they were. + const rejected = await submit([taskA], better); + const rejectedState = await settle(rejected.body.batchId); + assert.equal(rejectedState.functions[0].state, 'peer_review', JSON.stringify(rejectedState.functions[0])); + assert.equal((await verdicts(rejectedState.functions[0].attemptId, 'different', ['rvp1', 'rvp2', 'rvp3'])).state, 'needs_cleanup'); + assert.equal(ctx.tasks.get(taskA).state, 'done'); + assert.equal(ctx.repo.show('HEAD', unit), onMain); + + // Peers confirm it: the revision merges, the task stays done, and main records its new code. + const accepted = await submit([taskA], better); + const acceptedState = await settle(accepted.body.batchId); + assert.equal((await verdicts(acceptedState.functions[0].attemptId, 'same', ['rvp4', 'rvp5', 'rvp6'])).state, 'approved'); + await ctx.merger.pump(); + await waitFor(async () => (await batchState(accepted.body.batchId)).state === 'merged'); + assert.equal(ctx.tasks.get(taskA).state, 'done'); + assert.equal(ctx.repo.show('HEAD', unit), better); + const status = ctx.store.get('SELECT score, confirmed, code_hash FROM func_status WHERE symbol_id = ?', a); + assert.deepEqual([status.score, status.confirmed, status.code_hash], [0.8, 1, 'a-int better = 1; return better;']); + } finally { + ctx.verifier.check = realCheck; + ctx.verifier.mergeCheck = realMergeCheck; + ctx.store.run('DELETE FROM region_claims'); + } +}); + +test('admin: a done task is reopened only with force, and then as open', async () => { + await ctx.accounts.createUser('rvadmin', 'admin', PASSWORD); + const cookie = await login('rvadmin'); + const done = ctx.store.get(`SELECT id FROM tasks WHERE state = 'done' LIMIT 1`); + const plain = await request('POST', `/api/admin/tasks/${done.id}`, { state: 'open' }, { cookie }); + assert.equal(plain.body.state, 'done'); + const forced = await request('POST', `/api/admin/tasks/${done.id}`, { state: 'open', force: true }, { cookie }); + assert.equal(forced.body.state, 'open'); + ctx.store.run(`UPDATE tasks SET state = 'done' WHERE id = ?`, done.id); +}); + test('new work is claimed by region; a single function only to fix one that came back', async () => { const token = await activeUser('rtuser'); const open = ctx.store.get(`SELECT id FROM tasks WHERE state = 'open' LIMIT 1`);