Detect binaries despite git attributes and reject unconfirmed reviews (#1825)

agents/bradford/work/queue-327/BUILD.md

@@ -105,3 +105,58 @@ the green run and this old-candidate reproduction. All runs used timeout -k 10.
 
 Holding claimant queue writes per sage-3999 until Boyd finishes row 309's record.
 Last acknowledged row 327 state is in-progress (bradford-0010, revision 3868).
+
+## Dewey landing review B1 (dewey-0956) — changes
+
+Read the invited review body at /mnt/storage/scratch/tmp/dew327vWXloCT/body.md.
+B1 identifies a brief-level gap: numstat follows candidate/host git attributes,
+so *.bin diff makes a NUL-containing blob disappear from the binary-only set.
+The production script indeed filters numstat before reading any blob, consistent
+with that data flow. Proposed correction: enumerate every changed numstat path,
+skip absent/non-blob entries at SHA, classify binary when numstat says binary OR
+committed blob has NUL among first 8000 bytes. Read with cat-file, never worktree.
+Test committed .gitattributes, info/attributes, and core.attributesFile overrides
+in scratch; reject with no row, allow exact reviewed bytes; add NUL-scan mutant.
+Sage owns the brief amendment and next CLEAR/queue sequencing.
+
+N1 acknowledged: candidate-equality check is defense in depth; invalid receipt
+cannot pass queue replay, so the corrupt-queue case does not isolate that check.
+N2/N3 recorded as described in review, without optional secrets/reader changes.
+N4 planned hardening: refuse unconfirmed durable tail notes from rows() with
+exit 2, with a real scratch witness-prefix case rather than a test-only reader.
+No test run or queue write started for B1/N4 before Sage CLEAR.
+
+Sage sage-4013/4015 authorizes B1 and N4 within the brief; amendment committed on
+refactor at 9042754c4 (read with git show, no branch change). Fix and tests written
+before requesting CLEAR as directed. Tests cover all three attribute sources,
+approved manifest success under each override, exact 7999/8000 offset boundary,
+and a real prefix-witness rollback to yield an unconfirmed approval tail.
+Added only the requested sixth mutant, skip-nul-scan; the original five remain.
+No new production configuration knobs, no secrets-diff change, no queue writes.
+Will demonstrate red against the prior candidate using the new tests, then green
+against revised code, under the approved slot. Full landing gate remains Sage's.
+
+After Dewey's round-2 CHANGES record (comment 31522, rev3876), Sage sage-4016
+released only the claimant return to in-progress. Acknowledged:
+`ok bradford-0017 rev 3877 row 327 in-review→in-progress`.
+No other queue write before the next in-review candidate move.
+Test slot remains pending Sage CLEAR after Gyro's build.
+
+## Amendment 1 verification (sage-4017 CLEAR)
+
+b1-regression-red.txt: new tests against 704910ca3 reproduce attributes-hidden
+asset.bin reaching NO_BINARY/PUSH_READY with exit0 instead of exit1.
+n4-regression-red.txt: revised code with only the N4 check removed lets the real
+scratch unconfirmed approval tail reach BINARY_REVIEWED/PUSH_READY, exit0 rather
+than exit2. These are expected negative controls; neither changed committed code.
+
+Final full new-suite run is green: test-run-5.txt records PASS 38 checks and
+KILLED skip-hash, accept-changes, earlier-round, working-tree, no-row, skip-nul-scan.
+All runs used timeout -k 10 240 with the same scratch TMPDIR; no old tests were
+changed or run. bash -n both scripts and git diff --check also passed. No source
+changes followed this green run. Sage's full landing gate is still separate.
+
+Scope in the final delta: classify all changed present blobs using numstat OR
+first-8000-byte NUL; fail closed on queue unconfirmed-tail notes; regression cases
+and requested sixth mutant; TOOLS entry and this plain-text packet. B1's brief
+amendment is Sage's 9042754c4; this branch does not edit the brief or queue files.

agents/bradford/work/queue-327/b1-regression-red.txt

@@ -0,0 +1,27 @@
+node:internal/modules/run_main:107
+    triggerUncaughtException(
+    ^
+
+AssertionError [ERR_ASSERTION]: LOG_OK
+FF_OK
+NO_BINARY
+SECRETS_CLEAN
+NO_ATTRIBUTION
+PUSH_READY ee394f31d1e52751716e2413049f41b2f3072e65..76acc948149e718091fcd3f3807257e752a6616e
+
+
+
+0 !== 1
+
+    at Object.check (file:///home/jwoltje/.t3/worktrees/mosaic-stack/bradford-row327/[eval1]:44:12)
+    at file:///home/jwoltje/.t3/worktrees/mosaic-stack/bradford-row327/[eval1]:100:7
+    at process.processTicksAndRejections (node:internal/process/task_queues:104:5) {
+  generatedMessage: false,
+  code: 'ERR_ASSERTION',
+  actual: 0,
+  expected: 1,
+  operator: 'strictEqual',
+  diff: 'simple'
+}
+
+Node.js v26.8.1

agents/bradford/work/queue-327/n4-regression-red.txt

@@ -0,0 +1,27 @@
+node:internal/modules/run_main:107
+    triggerUncaughtException(
+    ^
+
+AssertionError [ERR_ASSERTION]: LOG_OK
+FF_OK
+BINARY_REVIEWED 1
+SECRETS_CLEAN
+NO_ATTRIBUTION
+PUSH_READY c1a8a1e9b8c3988874a8f7a35afab7c06aea94a5..325cd3671a7bf70b9a3aee60f4446bc93594a12a
+
+
+
+0 !== 2
+
+    at Object.check (file:///home/jwoltje/.t3/worktrees/mosaic-stack/bradford-row327/[eval1]:44:12)
+    at file:///home/jwoltje/.t3/worktrees/mosaic-stack/bradford-row327/[eval1]:121:7
+    at process.processTicksAndRejections (node:internal/process/task_queues:104:5) {
+  generatedMessage: false,
+  code: 'ERR_ASSERTION',
+  actual: 0,
+  expected: 2,
+  operator: 'strictEqual',
+  diff: 'simple'
+}
+
+Node.js v26.8.1

agents/bradford/work/queue-327/test-run-5.txt

@@ -0,0 +1,7 @@
+PASS push-check 38 checks
+KILLED skip-hash
+KILLED accept-changes
+KILLED earlier-round
+KILLED working-tree
+KILLED no-row
+KILLED skip-nul-scan

docs/TOOLS.md

@@ -1378,12 +1378,15 @@ it. Only `--fetch` fetches origin, before resolving BASE.
 
 Success prints `LOG_OK`, `FF_OK`, `NO_BINARY` or `BINARY_REVIEWED n`,
 `SECRETS_CLEAN`, `NO_ATTRIBUTION`, then `PUSH_READY BASE..SHA` as the last line.
-Each present binary needs its committed blob sha256 and exact path in an approved
+A changed present blob counts as binary when numstat marks it binary or its
+first 8000 committed bytes contain NUL, regardless of git attributes. Each
+present binary needs its committed blob sha256 and exact path in an approved
 latest-round candidate from a supplied row. A `changes` receipt blocks that round.
 Commit candidates authorize only changed paths with matching bytes and mode.
-Deleted binaries need no review. Queue reads retain canonical-checkout checks.
+Deleted binaries need no review. Queue reads retain canonical-checkout checks
+and refuse unconfirmed durable tails with exit 2.
 
 Exit 0 means ready; 1 means refusal (`STOP reason`, always the last line); 2 means
 usage or environment error. A refusal never prints later success tokens.
 `timeout -k 10 240 bash scripts/test-push-check.sh` exercises isolated scratch
-repositories and the five required mutants; set TMPDIR to a scratch filesystem.
+repositories and the six required mutants; set TMPDIR to a scratch filesystem.

scripts/push-check.sh

@@ -52,8 +52,11 @@ try {
 
   // --no-renames keeps -z numstat to exactly one path per record. Never parse
   // quoted path output or consult working-tree bytes.
-  const binaries = pathsText(git('diff', '--no-relative', '--no-color', '--no-ext-diff', '--no-textconv', '--no-renames', '--numstat', '-z', base, sha))
-    .split('\0').filter(Boolean).filter(s => s.startsWith('-\t-\t')).map(s => s.slice(4));
+  const changedPaths = pathsText(git('diff', '--no-relative', '--no-color', '--no-ext-diff', '--no-textconv', '--no-renames', '--numstat', '-z', base, sha))
+    .split('\0').filter(Boolean).map(record => {
+      const first = record.indexOf('\t'), second = record.indexOf('\t', first + 1);
+      return { path: record.slice(second + 1), binary: record.startsWith('-\t-\t') };
+    });
   const tree = commit => {
     const entries = new Map();
     for (const entry of pathsText(git('ls-tree', '--full-tree', '-r', '-z', commit)).split('\0').filter(Boolean)) {
@@ -64,7 +67,13 @@ try {
     return entries;
   };
   const have = tree(sha);
-  const present = binaries.filter(path => have.has(path));
+  // Attributes can force a binary to appear as text in numstat. Inspect raw
+  // committed bytes as well, following Git's first-8000-byte NUL rule.
+  const present = changedPaths.filter(({ path, binary }) => {
+    const entry = have.get(path);
+    if (!entry || entry.type !== 'blob') return false;
+    return binary || git('cat-file', 'blob', entry.oid).subarray(0, 8000).includes(0);
+  }).map(({ path }) => path);
   let approved = [];
   if (present.length && rowIds.size) {
     let rows;
@@ -73,6 +82,7 @@ try {
       const { rows: readRows } = await import(pathToFileURL(join(root, 'packages/queue/src/store.mjs')).href);
       const result = readRows({ cwd: process.cwd(), env: process.env });
       if (result.code !== 0) throw new Error('queue');
+      if (result.err.some(note => /unconfirmed tail|not confirmed durable/.test(note))) stop('queue unconfirmed tail', 2);
       rows = result.rows;
       for (const id of rowIds) if (!rows.some(r => r.id === id)) throw new Error('row');
     } catch { stop('queue unavailable or invalid', 2); }

scripts/test-push-check.sh

@@ -92,6 +92,39 @@ try {
     const ff = spawnSync('bash', [target, f.base, log], { cwd: f.repo.root, env: f.repo.env, encoding: 'utf8' });
     assert.equal(ff.status, 1); assert.equal(ff.stdout, 'LOG_OK\nSTOP ff\n'); count++;
   }
+  // B1: attributes may add binary classifications, never hide NUL blobs.
+  for (const source of ['tree', 'info', 'config']) {
+    const f = fixture();
+    if (source === 'tree') f.commit('.gitattributes', '*.bin diff\n');
+    if (source === 'info') writeFileSync(join(f.repo.gitDir, 'info/attributes'), '*.bin diff\n');
+    if (source === 'config') {
+      const attributes = join(f.repo.base, 'attributes'); writeFileSync(attributes, '*.bin diff\n');
+      f.repo.g('config', 'core.attributesFile', attributes);
+    }
+    const sha = f.commit('asset.bin', binary);
+    f.check(sha, [], 1, /^STOP binary asset.bin /);
+    f.round(f.manifest());
+    assert.match(f.check(sha, ['--row', '9']), /BINARY_REVIEWED 1\n/);
+  }
+  // The content rule includes byte 7999, and excludes byte 8000.
+  for (const offset of [7999, 8000]) {
+    const f = fixture(), bytes = Buffer.alloc(8001, 65); bytes[offset] = 0;
+    f.commit('.gitattributes', '*.bin diff\n');
+    const sha = f.commit('asset.bin', bytes);
+    if (offset === 7999) f.check(sha, [], 1, /^STOP binary asset.bin /);
+    else assert.match(f.check(sha), /NO_BINARY\n/);
+  }
+  // N4: an approval beyond the durable witness is visible, not authority.
+  {
+    const f = fixture(), sha = f.commit('asset.bin', binary);
+    const witness = join(f.repo.gitDir, 'mosaic-queue.head');
+    const durable = readFileSync(witness);
+    f.round(f.manifest());
+    writeFileSync(witness, durable);
+    const visible = f.run(['show', '9']);
+    assert.match(visible.err, /not confirmed durable/);
+    f.check(sha, ['--row', '9'], 2, /^STOP queue unconfirmed tail$/);
+  }
   {
     const f = fixture(), sha = f.commit('asset.bin', binary);
     f.check(sha, [], 1, /^STOP binary asset.bin /);
@@ -208,6 +241,7 @@ try {
       ['earlier-round', 'row.review?.rounds.at(-1)', 'row.review?.rounds.at(0)'],
       ['working-tree', "hash(git('cat-file', 'blob', h.oid))", 'hash(readFileSync(path))'],
       ['no-row', 'if (!matches) stop(', 'if (!matches && rowIds.size) stop('],
+      ['skip-nul-scan', " || git('cat-file', 'blob', entry.oid).subarray(0, 8000).includes(0)", ''],
     ];
     for (const [name, before, after] of mutants) {
       assert.ok(source.includes(before), name);