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

@@ -672,7 +672,9 @@ export function createApp(ctx) {
     return guided({
       ...r,
       baseCommit: repo.head(),
-      note: r.alsoClaimedBy.length
+      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 <taskId...> --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);
   });
 

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
+// then submit any function of it. Competing candidates of a function are verified and reviewed
-// verified and reviewed independently, and the first accepted one merges (merge.js supersedes the
+// independently, and the first accepted one merges (merge.js supersedes the others), so two
-// others), so two agents' work combines per function.
+// 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 });
       }
     }

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`);

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
+  // another account's submission on its way to main), and functions on main whose source the
-  // measured again, and the regression check still protects whatever already matches.
+  // 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) {

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 <x@y.invalid>');
+  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`);