From 63e447077de87cf56c96ac8ad7e7cd5a772d7feb Mon Sep 17 00:00:00 2001 From: ragnarok-clanker Date: Mon, 26 Oct 2026 23:06:58 +0100 Subject: [PATCH] Admins can merge a batch sent back only for lower matching scores --- src/server/app.js | 9 +++++++++ src/server/cli.js | 10 ++++++++++ src/server/db.js | 2 ++ src/server/merge.js | 32 +++++++++++++++++++++++++++++++- src/server/verify.js | 4 ++-- test/server.test.js | 37 +++++++++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/server/app.js b/src/server/app.js index 4212b21..b8a813c 100644 --- a/src/server/app.js +++ b/src/server/app.js @@ -1000,6 +1000,15 @@ export function createApp(ctx) { return r; }); + // An admin merges a batch that came back only for lowering matching scores (merge.js). + router.on('POST', '/api/admin/batches/:id/accept-regressions', async ({ req, params }) => { + const user = auth(req, { role: 'admin', session: true }); + const body = await readJson(req, 16 * 1024); + const r = merger.acceptRegressions(user, intParam(params.id), body.comment ? String(body.comment) : null); + if (runsWorker) setImmediate(() => merger.pump().catch(() => {})); + return r; + }); + // --- maintainers and admins (browser session only) router.on('POST', '/api/admin/tasks/:id', async ({ req, params }) => { diff --git a/src/server/cli.js b/src/server/cli.js index 27bb67a..4e9ef51 100644 --- a/src/server/cli.js +++ b/src/server/cli.js @@ -48,6 +48,8 @@ Usage: td-server [options] (all commands accept --data , defaul ghidra build | refresh the server's Ghidra project (download, pseudocode); refresh also runs after merges rebuild compile all of main and refresh match status tasks generate create tasks for functions that have none + batch accept-regressions [comment] + merge a batch sent back only for lower matching scores toolchain pack [--image I] package the compiler from the compile image for td toolchain install (downloads: approved accounts only) `; @@ -345,6 +347,14 @@ async function main() { return; } + if (cmd === 'batch' && sub === 'accept-regressions') { + // The running worker picks the reopened batch up on its next round. + const ctx = openServer(dataDir); + const [id, admin, ...comment] = rest; + console.log(ctx.merger.acceptRegressions(ctx.accounts.requireUser(admin), Number(id), comment.join(' ') || null)); + return; + } + if (cmd === 'rebuild') { const ctx = openServer(dataDir); const t0 = Date.now(); diff --git a/src/server/db.js b/src/server/db.js index 305ca17..cbb37dc 100644 --- a/src/server/db.js +++ b/src/server/db.js @@ -313,6 +313,8 @@ export class Store { this.addColumn('peer_reviews', 'weight', 'INTEGER NOT NULL DEFAULT 1'); this.addColumn('peer_reviews', 'token_id', 'INTEGER'); this.addColumn('attempts', 'peer_skipped', 'INTEGER NOT NULL DEFAULT 0'); + // A maintainer accepted that this attempt lowers matching scores (cleaner source over closer bytes). + this.addColumn('attempts', 'regressions_accepted', 'INTEGER NOT NULL DEFAULT 0'); this.db.exec('CREATE INDEX IF NOT EXISTS attempts_user_time ON attempts(user_id, created_at)'); // "No decompile" claims: an attempt that says a function needs no source (peer reviewed like code). this.addColumn('attempts', 'kind', "TEXT NOT NULL DEFAULT 'code'"); // code | skip diff --git a/src/server/merge.js b/src/server/merge.js index b4268da..7d6669f 100644 --- a/src/server/merge.js +++ b/src/server/merge.js @@ -102,6 +102,35 @@ export class Merger { return { state: newState }; } + /** + * An admin's decision that a batch sent back only for lowering matching scores merges anyway: + * the project prefers that source (e.g. no calling convention the original code did not need) + * over closer bytes. Every function of the batch is accepted as it is, without peer review, and + * the merge records the lower scores. + */ + acceptRegressions(admin, batchId, comment) { + if (!hasRole(admin, 'admin')) throw new HttpError(403, 'only admins can accept lower matching scores'); + return this.store.tx(() => { + const batch = this.store.get('SELECT * FROM batches WHERE id = ?', batchId); + if (!batch) throw new HttpError(404, 'no such batch'); + if (batch.state !== 'returned') throw new HttpError(409, `batch is ${batch.state}`); + const attempts = batchAttempts(this.store, batchId); + const blocking = attempts.filter((a) => { + const fb = a.result_json ? JSON.parse(a.result_json).feedback ?? {} : {}; + return !['needs_cleanup', 'returned'].includes(a.state) || fb.compileErrors?.length || fb.annotationErrors?.length || fb.summary === 'compilation failed'; + }); + if (!attempts.length || blocking.length) throw new HttpError(409, `only a batch that compiled and came back for its scores can be accepted (${blocking.map((a) => a.symbol_id).join(', ') || 'empty'})`); + for (const a of attempts) { + this.store.run(`UPDATE attempts SET state = 'approved', regressions_accepted = 1, peer_skipped = 1 WHERE id = ?`, a.id); + this.store.run(`UPDATE tasks SET state = 'approved', updated_at = ? WHERE id = ? AND state != 'done'`, now(), a.task_id); + this.store.run('INSERT INTO reviews (attempt_id, reviewer_id, action, comment, created_at) VALUES (?, ?, ?, ?, ?)', a.id, admin.id, 'approve', comment?.slice(0, 4000) ?? null, now()); + } + this.store.run(`UPDATE batches SET state = 'open', result_json = json_remove(COALESCE(result_json, '{}'), '$.returned', '$.mergeFailure') WHERE id = ?`, batchId); + this.store.event('regressions_accepted', admin.id, { batch: batchId, attempts: attempts.map((a) => a.id) }); + return { batchId, accepted: attempts.map((a) => a.id) }; + }); + } + /** * Merges the oldest open batch whose attempts are all approved. Failing that, splits off the * approved part of a batch whose other functions still wait for review, so finished work @@ -333,8 +362,9 @@ export class Merger { const outside = this.verifier.changedOutside(check, merged.files, targets, batch.user_id); const confirmedIds = new Set(); const results = []; + const allowRegressions = attempts.some((a) => a.regressions_accepted); for (const a of attempts) { - const ev = this.verifier.evaluate({ check, rec, targetId: a.symbol_id, files: merged.files, changedHeaders, outside }); + const ev = this.verifier.evaluate({ check, rec, targetId: a.symbol_id, files: merged.files, changedHeaders, outside, allowRegressions }); // Byte-identical, or confirmed by peers and approved by a human; anything worse bounces. const identical = ev.state === 'approved' || ev.state === 'in_review'; const peerConfirmed = ev.state === 'peer_review' && this.peers?.confirmed(a.id); diff --git a/src/server/verify.js b/src/server/verify.js index d49c3bc..96c392d 100644 --- a/src/server/verify.js +++ b/src/server/verify.js @@ -663,7 +663,7 @@ export class Verifier { } /** Routes a check result: rejected, needs_cleanup or in_review. */ - evaluate({ check, rec, targetId, files, changedHeaders, outside = [] }) { + evaluate({ check, rec, targetId, files, changedHeaders, outside = [], allowRegressions = false }) { const fileSet = new Set(Object.keys(files)); const policy = this.project.matchPolicy; const feedback = { summary: '', warnings: [] }; @@ -687,7 +687,7 @@ export class Verifier { for (const w of annWarnings) feedback.warnings.push(`${w.file}:${w.line} ${w.message}`); const failed = check.units.filter((u) => !u.compiled); const target = check.units.flatMap((u) => u.functions.map((f) => ({ ...f, file: u.file }))).find((f) => f.id === targetId); - const regressions = this.regressions(check, files); + const regressions = allowRegressions ? [] : this.regressions(check, files); feedback.target = target ?? null; feedback.annotationErrors = annErrors; diff --git a/test/server.test.js b/test/server.test.js index afe8c9a..fb27c1f 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -344,6 +344,43 @@ test('the last admin cannot be revoked or demoted', async () => { assert.equal((await request('POST', '/api/admin/users/root/role', { role: 'contributor' }, { cookie: admin })).status, 409); }); +test('an admin accepts a batch sent back only for lower scores; it merges past the regression check', async () => { + ctx.store.run(`INSERT INTO func_status (symbol_id, file, status, score, commit_hash, updated_at, confirmed) VALUES ('fn_00002', 'src/a.cpp', 'mismatch', 0.8, 'x', 0, 1)`); + const lower = { annotationErrors: [], units: [{ file: 'src/a.cpp', compiled: true, lintErrors: [], functions: [{ id: 'fn_00002', status: STATUS.MISMATCH, score: 0.7, relocations: [] }], unannotated: [] }] }; + const rec = { conflicts: [], unknown: [], learned: [] }; + const ev = (allowRegressions) => ctx.verifier.evaluate({ check: lower, rec, targetId: 'fn_00002', files: { 'src/a.cpp': '' }, changedHeaders: [], allowRegressions }); + assert.equal(ev(false).state, 'needs_cleanup', 'a lower score of a confirmed function is a regression'); + assert.equal(ev(true).state, 'peer_review', 'accepted: judged like any other mismatch'); + ctx.store.run(`DELETE FROM func_status`); + + await ctx.accounts.createUser('rgmaint', 'maintainer', PASSWORD); + await activeUser('rgauthor'); + const author = ctx.accounts.byName('rgauthor'); + const v = ctx.store.currentVersion(); + const sym = ctx.store.ensureSymbol({ kind: 'function', versionId: v.id, addr: 0x401a00, size: 8, unit: 'src/rg.cpp' }).symbol.id; + ctx.tasks.generate(); + const task = ctx.store.get('SELECT id FROM tasks WHERE symbol_id = ?', sym).id; + ctx.store.run(`UPDATE tasks SET state = 'needs_cleanup' WHERE id = ?`, task); + const batch = Number(ctx.store.run(`INSERT INTO batches (user_id, base_commit, files_json, state, created_at) VALUES (?, ?, '{}', 'returned', ?)`, author.id, ctx.repo.head(), now()).lastInsertRowid); + const feedback = (f) => JSON.stringify({ feedback: f }); + const attempt = Number(ctx.store.run(`INSERT INTO attempts (task_id, user_id, base_commit, files_json, state, batch_id, result_json, created_at) VALUES (?, ?, ?, '{}', 'needs_cleanup', ?, ?, ?)`, task, author.id, ctx.repo.head(), batch, feedback({ summary: 'your change breaks 1 function(s) that matched before', compileErrors: [], annotationErrors: [] }), now()).lastInsertRowid); + + assert.throws(() => ctx.merger.acceptRegressions(ctx.accounts.byName('rgmaint'), batch, null), /only admins/); + ctx.store.run(`UPDATE attempts SET result_json = ? WHERE id = ?`, feedback({ summary: 'compilation failed', compileErrors: [{ file: 'src/rg.cpp' }] }), attempt); + assert.throws(() => ctx.merger.acceptRegressions(ctx.accounts.byName('root'), batch, null), /compiled and came back for its scores/, 'code that does not build is never forced in'); + ctx.store.run(`UPDATE attempts SET result_json = ? WHERE id = ?`, feedback({ summary: 'your change breaks 1 function(s) that matched before', compileErrors: [], annotationErrors: [] }), attempt); + + assert.deepEqual(ctx.merger.acceptRegressions(ctx.accounts.byName('root'), batch, 'prefer no __stdcall').accepted, [attempt]); + const a = ctx.store.get('SELECT * FROM attempts WHERE id = ?', attempt); + assert.deepEqual([a.state, a.regressions_accepted, a.peer_skipped], ['approved', 1, 1]); + assert.ok(ctx.peers.confirmed(attempt), 'the merge re-check accepts it without peer verdicts'); + assert.equal(ctx.store.get('SELECT state FROM batches WHERE id = ?', batch).state, 'open', 'back in the merge queue'); + assert.throws(() => ctx.merger.acceptRegressions(ctx.accounts.byName('root'), batch, null), /batch is open/); + ctx.store.run(`UPDATE batches SET state = 'superseded' WHERE id = ?`, batch); + ctx.store.run(`UPDATE attempts SET state = 'rejected' WHERE id = ?`, attempt); + ctx.store.run(`UPDATE tasks SET state = 'done' WHERE id = ?`, task); +}); + test('changing the password logs out browser sessions', async () => { const s = await login('erin'); const r = await request('POST', '/api/me/password', { current: PASSWORD, password: 'a brand new password' }, { cookie: s });