From 67965b04f17fa26722ae5e611fe43c9d972bfe2b Mon Sep 17 00:00:00 2001 From: Saty <5752427+SatyPardus@users.noreply.github.com> Date: Tue, 20 Oct 2026 04:56:07 +0200 Subject: [PATCH] Refuse removing annotations of functions not held; new source files join the build list - A submission may not drop the annotation of a function of the current build it does not hold (51 annotations were lost that way); removing a duplicate is fine. - A unit missing from compile.json compiles with the command shape of its folder, and the merge adds it to compile.json in the same commit (22 contributor files were outside the build). Co-Authored-By: Claude Opus 5.5 --- src/server/merge.js | 8 +++++++- src/server/verify.js | 39 +++++++++++++++++++++++++++++++++++++++ src/shared/project.js | 40 ++++++++++++++++++++++++++++++++++++++-- test/units.test.js | 18 ++++++++++++++++++ 4 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 test/units.test.js diff --git a/src/server/merge.js b/src/server/merge.js index f0f9138..9c8c04d 100644 --- a/src/server/merge.js +++ b/src/server/merge.js @@ -6,6 +6,7 @@ import { hasRole } from './accounts.js'; import { STATUS } from '../shared/compare.js'; import { returnBatch, batchAttempts, settleBatch } from './batches.js'; import { withoutFunctions, functionRegions } from './split.js'; +import { addUnitsToCompileCommands } from '../shared/project.js'; import { RANK, bestResults, BIG_BATCH } from './verify.js'; // Merges run in two lanes: one big merge (a header batch that must recompile many units) at a @@ -353,7 +354,12 @@ export class Merger { (!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.` : '') + '\n'; - const commit = this.repo.commitFiles(merged.files, message, `${author} <${author.replace(/[^A-Za-z0-9_.-]/g, '_')}@users.clank-together.invalid>`); + // New source files join the build list in the same commit (with the flags of their folder). + const listFile = this.project.compileCommands ?? null; + const grown = listFile ? addUnitsToCompileCommands(this.project, this.repo.show(head, listFile), Object.keys(merged.files)) : null; + const filesToCommit = grown ? { ...merged.files, [listFile]: grown.text } : merged.files; + const commit = this.repo.commitFiles(filesToCommit, grown ? `${message}Adds to the build: ${grown.added.join(', ')}\n` : message, `${author} <${author.replace(/[^A-Za-z0-9_.-]/g, '_')}@users.clank-together.invalid>`); + if (grown) this.project.compile.units = grown.units; this.store.tx(() => { this.applyCheckToDatabase(check, rec, commit, batch.user_id, { confirmedIds }); diff --git a/src/server/verify.js b/src/server/verify.js index 6347391..61e179e 100644 --- a/src/server/verify.js +++ b/src/server/verify.js @@ -221,6 +221,38 @@ export class Verifier { return errors; } + /** + * Annotations of functions of the current version that the submission removes although it does + * not hold them (not in the batch, not leased, not in a claimed region). Removing a duplicate is + * fine: the function stays annotated elsewhere on main. + */ + droppedAnnotations(files, baseCommit, attempts, userId) { + const ANN = /\/\/\s*(?:FUNCTION|STUB)\s*:\s*(fn_\d+)/g; + const ids = (t) => new Set([...String(t ?? '').matchAll(ANN)].map((m) => m[1])); + const before = new Set(); + const after = new Set(); + for (const [f, text] of Object.entries(files)) { + for (const id of ids(this.repo.show(baseCommit, f))) before.add(id); + for (const id of ids(text)) after.add(id); + } + const held = new Set(attempts.map((a) => a.symbol_id)); + const v = this.store.currentVersion(); + const out = []; + for (const id of before) { + if (after.has(id) || held.has(id)) continue; + if (!this.store.get('SELECT 1 FROM symbol_addresses WHERE symbol_id = ? AND version_id = ?', id, v.id)) continue; // not in this build + const t = this.store.get('SELECT * FROM tasks WHERE symbol_id = ?', id); + if (t && ((t.state === 'leased' && t.lease_user === userId) || this.regions?.covers({ id: userId }, t))) continue; + try { + if (this.repo.git(['grep', '-l', '-w', '-e', `FUNCTION: ${id}`, 'HEAD', '--', '.', ...Object.keys(files).map((f) => `:(exclude)${f}`)]).trim()) continue; + } catch { + // not annotated anywhere else + } + out.push(id); + } + return out; + } + /** Functions the submission declares that nothing defines (undefined.js). */ undefinedGate(files, baseCommit) { const submitted = Object.keys(files); @@ -412,6 +444,13 @@ export class Verifier { if (!this.repo.isAncestor(batch.base_commit)) return finishAll('rejected', { summary: `base commit ${batch.base_commit} is not part of main; sync and resubmit` }); const textErrors = this.freeTextGate(submitted, batch.base_commit); if (textErrors.length) return finishAll('rejected', { summary: 'rejected: the submission adds comments or strings that are not allowed (see lintErrors)', lintErrors: textErrors }); + const dropped = this.droppedAnnotations(submitted, batch.base_commit, attempts, batch.user_id); + if (dropped.length) { + return finishAll('needs_cleanup', { + summary: `removes the annotation of ${dropped.join(', ')}, which this submission does not hold: functions of the original lose their match that way. Keep the annotation (and the code), or claim the function too`, + droppedAnnotations: dropped, + }); + } const undefinedNames = this.undefinedGate(submitted, batch.base_commit); if (undefinedNames.length) { return finishAll('needs_cleanup', { diff --git a/src/shared/project.js b/src/shared/project.js index d83d026..d80ea22 100644 --- a/src/shared/project.js +++ b/src/shared/project.js @@ -35,6 +35,41 @@ export function loadProject(repoDir) { return project; } +/** + * The command shape a unit that is not in the build list yet gets: the one most units of its + * folder use (or of the nearest parent folder that has units), else the most common one. + */ +export function inferShape(project, file) { + const units = project.compile?.units; + if (!units) return null; + if (units[file] !== undefined) return units[file]; + const tally = (prefix) => { + const n = new Map(); + for (const [f, s] of Object.entries(units)) if (f.startsWith(prefix)) n.set(s, (n.get(s) ?? 0) + 1); + return [...n].sort((a, b) => b[1] - a[1] || a[0] - b[0])[0]?.[0]; + }; + const parts = file.split('/'); + for (let i = parts.length - 1; i > 0; i--) { + const s = tally(parts.slice(0, i).join('/') + '/'); + if (s !== undefined) return s; + } + return tally('') ?? 0; +} + +/** + * The build list with new units added (each with its inferred shape), as the text the repository + * stores it in, or null when nothing is new. `text` is the list as it is on main. + */ +export function addUnitsToCompileCommands(project, text, files) { + if (!project.compile?.units || text === null) return null; + const doc = JSON.parse(text); + const added = files.filter((f) => isUnit(f) && doc.units[f] === undefined && !/(^|\/)Win32\//.test(f)); + if (!added.length) return null; + for (const f of added) doc.units[f] = inferShape({ compile: doc }, f); + doc.units = Object.fromEntries(Object.entries(doc.units).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0))); + return { text: JSON.stringify(doc, null, 1), added, units: doc.units }; +} + export function loadToolchainLock(repoDir) { const file = path.join(repoDir, 'toolchain.lock.json'); return fs.existsSync(file) ? JSON.parse(fs.readFileSync(file, 'utf8')) : {}; @@ -42,8 +77,9 @@ export function loadToolchainLock(repoDir) { /** Compiler flags and toolchain for one translation unit. */ export function unitSettings(project, file) { - const shape = project.compile?.units?.[file]; - if (shape !== undefined) return { cflags: project.compile.shapes[shape], toolchain: project.toolchain, env: project.compile.env ?? {} }; + // A unit the build list does not have yet compiles the way it will once it is added (merge.js). + const shape = project.compile?.units ? inferShape(project, file) : undefined; + if (shape !== undefined && shape !== null) return { cflags: project.compile.shapes[shape], toolchain: project.toolchain, env: project.compile.env ?? {} }; let cflags = project.cflags; let toolchain = project.toolchain; for (const o of project.fileOverrides ?? []) { diff --git a/test/units.test.js b/test/units.test.js new file mode 100644 index 0000000..ce2a41c --- /dev/null +++ b/test/units.test.js @@ -0,0 +1,18 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { inferShape, addUnitsToCompileCommands } from '../src/shared/project.js'; + +test('new source files get the flags of their folder and join the build list', () => { + const compile = { shapes: [['/a'], ['/b']], units: { 'Engine/Gx/A.cpp': 1, 'Engine/Gx/B.cpp': 1, 'Engine/Base/C.cpp': 0, 'WoW/D.cpp': 0 } }; + const project = { compile }; + assert.equal(inferShape(project, 'Engine/Gx/New.cpp'), 1, 'its folder'); + assert.equal(inferShape(project, 'Engine/Gx/Deep/New.cpp'), 1, 'the nearest folder with units'); + assert.equal(inferShape(project, 'Tools/X.cpp'), 0, 'the most common one'); + const text = JSON.stringify(compile, null, 1); + const grown = addUnitsToCompileCommands(project, text, ['Engine/Gx/New.cpp', 'Engine/Gx/A.cpp', 'Engine/Gx/x.h', 'WoW/Win32/Old.cpp']); + assert.deepEqual(grown.added, ['Engine/Gx/New.cpp'], 'headers, listed units and Win32 copies are left alone'); + const doc = JSON.parse(grown.text); + assert.deepEqual(Object.keys(doc.units), ['Engine/Base/C.cpp', 'Engine/Gx/A.cpp', 'Engine/Gx/B.cpp', 'Engine/Gx/New.cpp', 'WoW/D.cpp']); + assert.equal(doc.units['Engine/Gx/New.cpp'], 1); + assert.equal(addUnitsToCompileCommands(project, text, ['Engine/Gx/A.cpp']), null); +});