From a47b35c15967368feaa88e2e1e5c2d07afff5bd9 Mon Sep 17 00:00:00 2001 From: Saty <5752427+SatyPardus@users.noreply.github.com> Date: Sun, 18 Oct 2026 11:17:54 +0200 Subject: [PATCH] Five "same" verdicts accept a function; objections are kept for later review A function with peerAutoAccept (5) "same" verdicts is accepted without human review even if a peer said "different". Every function that reaches main with at least one "different" verdict (auto-accepted or approved by a human) is recorded in later_review; the review page lists them with the attempt and a "Reviewed" button. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RNwtXPYqFu3oKaaXwYwNWW --- src/server/app.js | 17 +++++++++++++++++ src/server/db.js | 13 +++++++++++++ src/server/merge.js | 9 ++++++++- src/server/peer.js | 19 +++++++++---------- src/server/web/app.js | 26 ++++++++++++++++++++++++-- test/server.test.js | 10 +++++++--- 6 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/server/app.js b/src/server/app.js index 91067a4..3d76ed7 100644 --- a/src/server/app.js +++ b/src/server/app.js @@ -571,6 +571,23 @@ export function createApp(ctx) { ); }); + // Functions merged although a peer said "different", for a human to look at later. + router.on('GET', '/api/review/later', ({ req }) => { + auth(req, { role: 'reviewer', session: true }); + return store.all( + `SELECT l.id, l.symbol_id, s.name, l.attempt_id, l.objections, l.merged_commit, l.created_at, u.name AS author + FROM later_review l JOIN symbols s ON s.id = l.symbol_id JOIN attempts a ON a.id = l.attempt_id JOIN users u ON u.id = a.user_id + WHERE l.cleared_at IS NULL ORDER BY l.id`, + ); + }); + + router.on('POST', '/api/review/later/:id/clear', ({ req, params }) => { + const user = auth(req, { role: 'reviewer', session: true }); + const r = store.run('UPDATE later_review SET cleared_by = ?, cleared_at = ? WHERE id = ? AND cleared_at IS NULL', user.id, Date.now(), intParam(params.id)); + if (!r.changes) throw new HttpError(404, 'nothing to clear'); + return { ok: true }; + }); + router.on('POST', '/api/attempts/:id/review', async ({ req, params }) => { const user = auth(req, { role: 'reviewer', session: true }); const body = await readJson(req, 16 * 1024); diff --git a/src/server/db.js b/src/server/db.js index fa1d700..fabc01a 100644 --- a/src/server/db.js +++ b/src/server/db.js @@ -207,6 +207,19 @@ CREATE TABLE IF NOT EXISTS batch_checks ( check_json TEXT NOT NULL ); +-- Functions that reached main although a peer reviewer said "different": kept for a human to look +-- at later (e.g. once everything is decompiled). +CREATE TABLE IF NOT EXISTS later_review ( + id INTEGER PRIMARY KEY, + symbol_id TEXT NOT NULL REFERENCES symbols(id), + attempt_id INTEGER NOT NULL REFERENCES attempts(id), + merged_commit TEXT, + objections INTEGER NOT NULL, + created_at INTEGER NOT NULL, + cleared_by INTEGER REFERENCES users(id), + cleared_at INTEGER +); + -- Remote checks, queued by the web process and run by the worker process. CREATE TABLE IF NOT EXISTS check_jobs ( id INTEGER PRIMARY KEY, diff --git a/src/server/merge.js b/src/server/merge.js index b3a92e3..ba54bf7 100644 --- a/src/server/merge.js +++ b/src/server/merge.js @@ -282,7 +282,7 @@ export class Merger { (peers.length ? `\nPeer-reviewed-by: ${peers.join(', ')}` : '') + (reviewers.length ? `\nReviewed-by: ${reviewers.join(', ')}` : '') + (!reviewers.length && !confirmedIds.size ? '\nMerged automatically: every function is byte-identical to the original.' : '') + - (!reviewers.length && confirmedIds.size ? `\nAccepted without human review: at least ${this.peers?.autoAccept ?? 5} peers confirmed the logic and none objected.` : '') + + (!reviewers.length && confirmedIds.size ? `\nAccepted without human review: at least ${this.peers?.autoAccept ?? 5} peers confirmed the logic.` : '') + '\n'; const commit = this.repo.commitFiles(merged.files, message, `${author} <${author.replace(/[^A-Za-z0-9_.-]/g, '_')}@users.clank-together.invalid>`); @@ -294,6 +294,13 @@ export class Merger { } this.store.run(`UPDATE batches SET state = 'merged', merged_commit = ? WHERE id = ?`, commit, batch.id); this.store.run('DELETE FROM batch_checks WHERE batch_id = ?', batch.id); + // Merged although a peer said "different": kept for a human to look at later. + for (const a of attempts) { + const objections = this.store.get(`SELECT COUNT(*) AS n FROM peer_reviews WHERE attempt_id = ? AND verdict = 'different'`, a.id).n; + if (objections) { + this.store.run('INSERT INTO later_review (symbol_id, attempt_id, merged_commit, objections, created_at) VALUES (?, ?, ?, ?, ?)', a.symbol_id, a.id, commit, objections, now()); + } + } this.store.event('merged', batch.user_id, { batch: batch.id, functions: attempts.length, commit }); if (split) { this.rebaseRemainder(split, merged.files, commit); diff --git a/src/server/peer.js b/src/server/peer.js index 4c68e4d..3803c97 100644 --- a/src/server/peer.js +++ b/src/server/peer.js @@ -7,8 +7,8 @@ // many "same" verdicts send it on to human review, that many "different" ones send it back to its // author with their reasons. With mixed verdicts, further reviewers are let in until one side // gets there. A function stays open for more reviewers while it waits for a human: with -// `peerAutoAccept` (default 5) "same" verdicts and no "different" at all it is accepted without -// human review. +// `peerAutoAccept` (default 5) "same" verdicts it is accepted without human review. If anyone said +// "different", it is merged all the same and kept in later_review for a human (merge.js). import { HttpError } from './http.js'; import { now } from './db.js'; @@ -65,10 +65,9 @@ export class PeerReviews { .slice(0, limit); } - // A function waiting for a human takes further reviewers toward auto-acceptance, unless someone - // already found a difference (then the human decides). + // A function waiting for a human takes further reviewers toward auto-acceptance. openForAutoAccept(r) { - return this.autoAccept > this.required && r.objections === 0 && r.confirmations + r.in_progress < this.autoAccept; + return this.autoAccept > this.required && r.confirmations + r.in_progress < this.autoAccept; } /** @@ -182,12 +181,12 @@ export class PeerReviews { const same = counts.same + (verdict === 'same' ? 1 : 0); const different = counts.different + (verdict === 'different' ? 1 : 0); if (a.state === 'in_review') { - // Already waiting for a human: enough agreeing peers and no objection accept it. - if (same >= this.autoAccept && different === 0) { + // Already waiting for a human: enough agreeing peers accept it (objections are kept for later). + if (same >= this.autoAccept) { this.store.run('DELETE FROM peer_leases WHERE attempt_id = ?', attemptId); this.store.run(`UPDATE attempts SET state = 'approved' WHERE id = ?`, attemptId); this.store.run(`UPDATE tasks SET state = 'approved', updated_at = ? WHERE id = ?`, now(), a.task_id); - this.store.event('auto_accepted', null, { attempt: attemptId, same }); + this.store.event('auto_accepted', null, { attempt: attemptId, same, different }); return { state: 'approved', same, different, required: this.required }; } return { state: 'in_review', same, different, required: this.required }; @@ -205,8 +204,8 @@ export class PeerReviews { return { state: 'needs_cleanup', same, different, required: this.required }; } if (same >= this.required) { - // Leases toward auto-acceptance stay; the others end here. - if (!(this.autoAccept > this.required && different === 0)) this.store.run('DELETE FROM peer_leases WHERE attempt_id = ?', attemptId); + // Leases toward auto-acceptance stay. + if (!(this.autoAccept > this.required)) this.store.run('DELETE FROM peer_leases WHERE attempt_id = ?', attemptId); this.store.run(`UPDATE attempts SET state = 'in_review' WHERE id = ?`, attemptId); this.store.run(`UPDATE tasks SET state = 'in_review', updated_at = ? WHERE id = ?`, now(), a.task_id); return { state: 'in_review', same, different, required: this.required }; diff --git a/src/server/web/app.js b/src/server/web/app.js index 9c6319d..2be2bb9 100644 --- a/src/server/web/app.js +++ b/src/server/web/app.js @@ -369,9 +369,31 @@ async function loadCrew() { // --- review async function loadQueue() { - const q = await api('/api/review/queue'); + const [q, later] = await Promise.all([api('/api/review/queue'), api('/api/review/later').catch(() => [])]); const body = $('queue-body'); body.replaceChildren(); + if (later.length) { + // Merged although a peer objected: for a closer look once there is time. + const d = el('details', null, 'fold'); + d.append( + el('summary', `Flagged for later review (${later.length}): merged although a peer said "different"`), + table( + ['Function', 'Name', 'Author', 'Objections', 'Merged', ''], + later.map((l) => [l.symbol_id, l.name || '', l.author, l.objections, (l.merged_commit || '').slice(0, 10), [ + button('Open', () => openAttempt(l.attempt_id)), + button('Reviewed', async () => { + try { + await post(`/api/review/later/${l.id}/clear`); + await loadQueue(); + } catch (e) { + alert(e.message); + } + }), + ]]), + ), + ); + body.append(d); + } if (!q.length) return body.append(el('p', 'Nothing waiting for review. Byte-identical work merges on its own; this queue holds functions in peer review and the ones peers confirmed.', 'muted')); const canSkipPeers = atLeast('maintainer'); const ready = q.filter((a) => a.state === 'in_review'); @@ -431,7 +453,7 @@ function queueBatches(body, items, skipsPeers) { const working = a.peer_working ? ` · ${a.peer_working} reviewing` : ''; if (a.state === 'peer_review') return `${a.peer_same} same · ${a.peer_different} different${working}`; // Waiting for a human: more peers can still accept it (5 agreeing, no objection). - return a.peer_different ? `${a.peer_same} same · ${a.peer_different} different: contested, you decide` : `${a.peer_same} same · accepts itself at 5${working}`; + return `${a.peer_same} same · accepts itself at 5${a.peer_different ? ` · ${a.peer_different} different: flagged for later` : ''}${working}`; }; body.append( table( diff --git a/test/server.test.js b/test/server.test.js index 6fb58db..4a77afe 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -412,7 +412,7 @@ test('a header batch is verified on its own units and merged with every includer } }); -test('five agreeing peers and no objection accept a function without human review', async () => { +test('five agreeing peers accept a function without human review, also over an objection', async () => { await activeUser('aaauthor'); const author = ctx.accounts.byName('aaauthor'); const peers = []; @@ -441,8 +441,8 @@ test('five agreeing peers and no objection accept a function without human revie await review(contested.attempt, peers[0], 'different'); for (let i = 1; i <= 3; i++) await review(contested.attempt, peers[i], 'same'); assert.equal(ctx.store.get('SELECT state FROM attempts WHERE id = ?', contested.attempt).state, 'in_review'); - const more = await call('POST', '/api/peer-reviews/claim', { attemptId: contested.attempt }, peers[4]); - assert.equal(more.status, 409, 'an objection leaves the decision to a human'); + assert.equal((await review(contested.attempt, peers[4], 'same')).state, 'in_review'); + assert.equal((await review(contested.attempt, peers[5], 'same')).state, 'approved', 'five "same" accept it over one objection (flagged for later at merge)'); for (const x of [clean, contested]) { ctx.store.run(`UPDATE attempts SET state = 'rejected' WHERE id = ?`, x.attempt); @@ -668,11 +668,14 @@ test('batches: one compile, auto-approval, peer review, one function sent back, // Round 2: the author fixes B (new fingerprint) and resubmits it; three peers confirm. bHash = 'hash-b-2'; + const objector = await activeUser('bpeer4'); const taskOfB = claim.body.tasks.find((t) => t.symbol === b).task; assert.equal((await call('POST', '/api/tasks/claim', { taskId: taskOfB }, author)).status, 200); const s2 = await call('POST', '/api/batches', { taskIds: [taskOfB], baseCommit: ctx.repo.head(), files: { 'src/batch.cpp': source } }, author); const r2 = Object.fromEntries((await settle(s2.body.batchId)).functions.map((f) => [f.symbol, f])); assert.equal(r2[b].state, 'peer_review', 'changed code gets reviewed again'); + await call('POST', '/api/peer-reviews/claim', { attemptId: r2[b].attemptId }, objector); + await call('POST', `/api/peer-reviews/${r2[b].attemptId}/verdict`, { verdict: 'different', reason: 'the loop bound looks off by one to me' }, objector); for (const t of peers) { await call('POST', '/api/peer-reviews/claim', { attemptId: r2[b].attemptId }, t); await call('POST', `/api/peer-reviews/${r2[b].attemptId}/verdict`, { verdict: 'same', reason: 'now checks the flag first like the original' }, t); @@ -693,6 +696,7 @@ test('batches: one compile, auto-approval, peer review, one function sent back, assert.equal(merged.state, 'merged', JSON.stringify(merged)); assert.match(ctx.repo.git(['log', '-1', '--format=%B']), new RegExp(`Decompile ${b}[\\s\\S]*Peer-reviewed-by`)); assert.equal(ctx.store.get('SELECT confirmed FROM func_status WHERE symbol_id = ?', b).confirmed, 1); + assert.equal(ctx.store.get('SELECT objections FROM later_review WHERE attempt_id = ?', r2[b].attemptId)?.objections, 1, 'merged over an objection: flagged for later review'); assert.equal(ctx.store.get('SELECT code_hash FROM func_status WHERE symbol_id = ?', a).code_hash, 'hash-a'); // Carry-over: an unchanged, peer-confirmed function keeps its verdicts in a resubmission.