fix(astra-review): harden report writing and make tests portable

Findings from three /astra-review rounds on the previous commit:

- writeReport: write to an exclusively created 0600 temp file in the
  destination directory and rename it into place. Removes the lstat/write
  race, replaces (never follows) a pre-existing symlink, and tightens
  permissions on an existing world-readable report. Full-write semantics via
  writeFileSync on the descriptor; temp file removed on any failure.
- scope: normalize the git toplevel and cwd with path.resolve before
  realpath (git reports forward slashes on Windows) and display file paths
  git-style with forward slashes.
- tests: build fixture roots with path.resolve/path.join so containment
  checks hold on Windows; skip POSIX permission and symlink assertions where
  the platform cannot support them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

scripts/astra-review.js

@@ -26,7 +26,9 @@
 
 'use strict';
 
+const crypto = require('crypto');
 const fs = require('fs');
+const path = require('path');
 const { collectChanges, describeScope, parseScope } = require('./lib/astra-review/scope');
 const { buildPrompt, effectiveVerdict, formatReport } = require('./lib/astra-review/prompt');
 const codex = require('./lib/astra-review/codex');
@@ -37,20 +39,31 @@ const EXIT_FAIL = 1;
 const EXIT_ERROR = 2;
 
 /**
- * Write the JSON report without following a pre-existing symlink (a
- * predictable path could otherwise be pointed at another file) and readable
- * only by the current user.
+ * Write the JSON report atomically: the content goes into a fresh, exclusively
+ * created 0600 temp file in the same directory, which is then renamed over the
+ * destination. A pre-existing symlink or world-readable file at the destination
+ * is replaced, never followed or reused, so there is no check-then-write race.
  */
 function writeReport(file, content, io = fs) {
-  let existing = null;
+  const temp = path.join(
+    path.dirname(file),
+    `.${path.basename(file)}.${process.pid}.${crypto.randomBytes(6).toString('hex')}.tmp`
+  );
+  const fd = io.openSync(temp, 'wx', REPORT_FILE_MODE);
+  let published = false;
   try {
-    existing = io.lstatSync(file);
-  } catch (error) {
-    if (!error || error.code !== 'ENOENT') throw error;
+    // writeFileSync on a descriptor loops until every byte is written; a bare
+    // writeSync can return short and would publish a truncated report.
+    io.writeFileSync(fd, content, 'utf8');
+    io.closeSync(fd);
+    io.renameSync(temp, file);
+    published = true;
+  } finally {
+    if (!published) {
+      try { io.closeSync(fd); } catch { /* already closed */ }
+      try { io.unlinkSync(temp); } catch { /* nothing left to clean */ }
+    }
   }
-  if (existing && existing.isSymbolicLink()) throw new Error(`refusing to write report through symlink: ${file}`);
-  if (existing && !existing.isFile()) throw new Error(`report path is not a regular file: ${file}`);
-  io.writeFileSync(file, content, { encoding: 'utf8', mode: REPORT_FILE_MODE });
 }
 
 function usage() {

scripts/lib/astra-review/scope.js

@@ -193,10 +193,15 @@ function readForReview(absolute, root, io) {
   return io.readFile(resolved);
 }
 
+/** Git-style (forward-slash) display path, so reports look the same on every OS. */
+function toGitPath(filePath) {
+  return filePath.split(path.sep).join('/');
+}
+
 function collectFiles(io, files, cwd, root) {
   const entries = files.map((file) => {
     const absolute = path.resolve(cwd, file);
-    const display = path.relative(root, absolute) || file;
+    const display = toGitPath(path.relative(root, absolute)) || file;
     return { display, content: readForReview(absolute, root, io) };
   });
   return {
@@ -290,12 +295,13 @@ function safeRealpath(target) {
 
 function collectChanges(scope, deps = {}) {
   const realpath = deps.realpathSync || safeRealpath;
-  // Canonical paths keep the containment check honest when /tmp is a symlink.
-  const cwd = realpath(deps.cwd || process.cwd());
+  // Canonical, platform-native paths keep the containment check honest when
+  // /tmp is a symlink or when git reports a forward-slash path on Windows.
+  const cwd = realpath(path.resolve(deps.cwd || process.cwd()));
   const runGitIn = deps.runGit || defaultRunGit;
   // Always work from the repository root so paths are root-relative and
   // untracked files outside the current subdirectory are not missed.
-  const root = realpath(runGitIn(['rev-parse', '--show-toplevel'], cwd).trim() || cwd);
+  const root = realpath(path.resolve(runGitIn(['rev-parse', '--show-toplevel'], cwd).trim() || cwd));
   const runGit = (args, options) => runGitIn(args, root, options);
   const io = {
     readFile: deps.readFile || ((file) => fs.readFileSync(file, 'utf8')),

tests/lib/astra-review-scope.test.js

@@ -25,6 +25,11 @@ function test(name, fn) {
   }
 }
 
+// Git reports slash-separated paths; the filesystem side must be native so
+// these fixtures hold on Windows as well as POSIX.
+const ROOT = path.resolve('/repo');
+const R = (...parts) => path.join(ROOT, ...parts);
+const OUTSIDE = path.resolve('/outside');
 const REGULAR_FILE = () => ({ isSymbolicLink: () => false, isFile: () => true });
 const enoent = () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); };
 const lstatOnly = (present) => (file) => (present.includes(file) ? REGULAR_FILE() : enoent());
@@ -95,7 +100,7 @@ test('collectChanges (uncommitted) merges staged, unstaged, and untracked change
   assert.ok(result.diff.includes('+staged change'));
   assert.ok(result.diff.includes('+unstaged change'));
   assert.ok(result.diff.includes('+brand new'));
-  assert.strictEqual(result.root, '/repo');
+  assert.strictEqual(result.root, ROOT);
   assert.strictEqual(result.truncated, false);
 });
 
@@ -122,10 +127,10 @@ test('collectChanges runs git from the repository root, not the current subdirec
     return '';
   };
 
-  scope.collectChanges({ kind: 'uncommitted', value: null }, { runGit, cwd: '/repo/scripts' });
+  scope.collectChanges({ kind: 'uncommitted', value: null }, { runGit, cwd: R('scripts') });
 
-  assert.strictEqual(cwds[0], '/repo/scripts');
-  assert.ok(cwds.slice(1).every((cwd) => cwd === '/repo'), `expected /repo, got ${cwds.slice(1)}`);
+  assert.strictEqual(cwds[0], R('scripts'));
+  assert.ok(cwds.slice(1).every((cwd) => cwd === ROOT), `expected ${ROOT}, got ${cwds.slice(1)}`);
 });
 
 test('collectChanges against a base branch diffs merge-base to the working tree, plus untracked files', () => {
@@ -177,7 +182,7 @@ test('collectChanges for explicit files reads file contents', () => {
 
   const result = scope.collectChanges(
     { kind: 'files', value: ['one.js', 'two.js'] },
-    { runGit: () => '', readFile, cwd: '/repo', lstatSync: REGULAR_FILE }
+    { runGit: () => '', readFile, cwd: ROOT, lstatSync: REGULAR_FILE }
   );
 
   assert.deepStrictEqual(result.files, ['one.js', 'two.js']);
@@ -192,10 +197,10 @@ test('collectChanges resolves explicit relative paths against the invocation dir
 
   const result = scope.collectChanges(
     { kind: 'files', value: ['AGENTS.md'] },
-    { runGit, readFile, cwd: '/repo/docs/zh-CN', lstatSync: REGULAR_FILE }
+    { runGit, readFile, cwd: R('docs', 'zh-CN'), lstatSync: REGULAR_FILE }
   );
 
-  assert.deepStrictEqual(read, ['/repo/docs/zh-CN/AGENTS.md']);
+  assert.deepStrictEqual(read, [R('docs', 'zh-CN', 'AGENTS.md')]);
   assert.deepStrictEqual(result.files, ['docs/zh-CN/AGENTS.md']);
   assert.ok(result.diff.includes('=== docs/zh-CN/AGENTS.md ==='));
 });
@@ -262,7 +267,7 @@ test('collectChanges returns a recovery hint for index and working tree on uncom
 test('collectChanges returns a recovery hint that reads files directly for explicit files', () => {
   const result = scope.collectChanges(
     { kind: 'files', value: ['a.js'] },
-    { runGit: () => '', readFile: () => 'x', cwd: '/repo', lstatSync: REGULAR_FILE }
+    { runGit: () => '', readFile: () => 'x', cwd: ROOT, lstatSync: REGULAR_FILE }
   );
 
   assert.ok(/read/i.test(result.recovery));
@@ -283,7 +288,7 @@ test('files-from-commit reviews current contents of touched files and keeps stil
   ]);
   const readFile = (file) => `now: ${path.basename(file)}`;
 
-  const result = scope.collectChanges({ kind: 'files-from-commit', value: 'HEAD' }, { runGit: git.runGit, readFile, cwd: '/repo/sub', lstatSync: lstatOnly(['/repo/kept.js']) });
+  const result = scope.collectChanges({ kind: 'files-from-commit', value: 'HEAD' }, { runGit: git.runGit, readFile, cwd: R('sub'), lstatSync: lstatOnly([R('kept.js')]) });
 
   assert.deepStrictEqual(result.files, ['kept.js', 'gone-later.js']);
   assert.ok(result.diff.includes('now: kept.js'));
@@ -299,7 +304,7 @@ test('files-from-commit handles a root commit', () => {
     [/^diff-tree --no-renames --no-commit-id --root -r --name-only -z a00b000$/, 'init.js\0'],
   ]);
 
-  const result = scope.collectChanges({ kind: 'files-from-commit', value: 'a00b000' }, { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: REGULAR_FILE });
+  const result = scope.collectChanges({ kind: 'files-from-commit', value: 'a00b000' }, { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: REGULAR_FILE });
 
   assert.deepStrictEqual(result.files, ['init.js']);
 });
@@ -314,7 +319,7 @@ test('files-from-commit includes a file the commit deleted once it has been rest
 
   const result = scope.collectChanges(
     { kind: 'files-from-commit', value: 'HEAD' },
-    { runGit: git.runGit, readFile: () => 'back', cwd: '/repo', lstatSync: lstatOnly(['/repo/restored.js']) }
+    { runGit: git.runGit, readFile: () => 'back', cwd: ROOT, lstatSync: lstatOnly([R('restored.js')]) }
   );
 
   assert.deepStrictEqual(result.files, ['restored.js']);
@@ -326,7 +331,7 @@ test('collectFiles serializes symlinks instead of following them', () => {
     { kind: 'files', value: ['link.js'] },
     {
       runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''),
-      cwd: '/repo',
+      cwd: ROOT,
       lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }),
       readlinkSync: () => '/etc/passwd',
       readFile: () => { readCalled = true; return 'secret'; },
@@ -344,7 +349,7 @@ test('collectFiles refuses paths that resolve outside the repository root', () =
       { kind: 'files', value: ['../outside.js'] },
       {
         runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''),
-        cwd: '/repo',
+        cwd: ROOT,
         lstatSync: () => ({ isSymbolicLink: () => false, isFile: () => true }),
         readFile: () => 'x',
       }
@@ -359,7 +364,7 @@ test('collectFiles refuses non-regular files such as directories', () => {
       { kind: 'files', value: ['src'] },
       {
         runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''),
-        cwd: '/repo',
+        cwd: ROOT,
         lstatSync: () => ({ isSymbolicLink: () => false, isFile: () => false }),
         readFile: () => 'x',
       }
@@ -374,8 +379,8 @@ test('collectFiles refuses files under a symlinked parent directory that resolve
       { kind: 'files', value: ['external/secret.txt'] },
       {
         runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''),
-        cwd: '/repo',
-        realpathSync: (target) => (target === '/repo/external' ? '/outside' : target),
+        cwd: ROOT,
+        realpathSync: (target) => (target === R('external') ? OUTSIDE : target),
         lstatSync: REGULAR_FILE,
         readFile: () => 'secret',
       }
@@ -395,7 +400,7 @@ test('files-from-commit keeps unresolved deletions in scope with their deletion
 
   const result = scope.collectChanges(
     { kind: 'files-from-commit', value: 'HEAD' },
-    { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: lstatOnly([]) }
+    { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: lstatOnly([]) }
   );
 
   assert.deepStrictEqual(result.files, ['removed.js']);
@@ -414,7 +419,7 @@ test('files-from-commit labels a file the commit added and the repair removed as
 
   const result = scope.collectChanges(
     { kind: 'files-from-commit', value: 'HEAD' },
-    { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: lstatOnly([]) }
+    { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: lstatOnly([]) }
   );
 
   assert.deepStrictEqual(result.files, ['added-then-removed.js']);
@@ -430,13 +435,13 @@ test('files-from-commit treats ENOTDIR (parent directory replaced by a file) as
     [/^diff --no-ext-diff --no-textconv parent1 -- config\/settings\.js$/, '-old setting\n'],
   ]);
   const lstatSync = (file) => {
-    if (file === '/repo/config') return REGULAR_FILE();
+    if (file === R('config')) return REGULAR_FILE();
     throw Object.assign(new Error('ENOTDIR'), { code: 'ENOTDIR' });
   };
 
   const result = scope.collectChanges(
     { kind: 'files-from-commit', value: 'HEAD' },
-    { runGit: git.runGit, readFile: () => 'now a file', cwd: '/repo', lstatSync }
+    { runGit: git.runGit, readFile: () => 'now a file', cwd: ROOT, lstatSync }
   );
 
   assert.deepStrictEqual(result.files, ['config', 'config/settings.js']);
@@ -457,7 +462,7 @@ test('files-from-commit treats a dangling symlink as present and serializes the
     { kind: 'files-from-commit', value: 'HEAD' },
     {
       runGit: git.runGit,
-      cwd: '/repo',
+      cwd: ROOT,
       lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }),
       readlinkSync: () => 'missing-target.js',
       readFile: () => { readCalled = true; return 'x'; },
@@ -482,7 +487,7 @@ test('files-from-commit describes a submodule (gitlink) directory instead of fai
 
   const result = scope.collectChanges(
     { kind: 'files-from-commit', value: 'HEAD' },
-    { runGit: git.runGit, cwd: '/repo', lstatSync, readFile: () => { throw new Error('must not read a directory'); } }
+    { runGit: git.runGit, cwd: ROOT, lstatSync, readFile: () => { throw new Error('must not read a directory'); } }
   );
 
   assert.deepStrictEqual(result.files, ['vendor/lib']);
@@ -510,18 +515,18 @@ test('defaultRunGit distinguishes a no-index access failure from a genuine diffe
   const accessError = () => ({ status: 1, stdout: '', stderr: 'error: Could not access nested-repo/' });
 
   assert.throws(
-    () => scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'nested-repo/'], '/repo', {}, accessError),
+    () => scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'nested-repo/'], ROOT, {}, accessError),
     /Could not access/
   );
   const realDiff = () => ({ status: 1, stdout: '+x\n', stderr: '' });
-  assert.strictEqual(scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'x'], '/repo', {}, realDiff), '+x\n');
+  assert.strictEqual(scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'x'], ROOT, {}, realDiff), '+x\n');
 });
 
 test('defaultRunGit fails loudly when a file inventory overflows the buffer', () => {
   const overflow = () => ({ error: Object.assign(new Error('x'), { code: 'ENOBUFS' }), stdout: 'partial.js\0', status: null });
 
   assert.throws(
-    () => scope.defaultRunGit(['ls-files', '--others', '-z'], '/repo', {}, overflow),
+    () => scope.defaultRunGit(['ls-files', '--others', '-z'], ROOT, {}, overflow),
     /exceeded .* inventory would be incomplete/
   );
 });
@@ -529,7 +534,7 @@ test('defaultRunGit fails loudly when a file inventory overflows the buffer', ()
 test('defaultRunGit returns partial output only for diff payloads marked partialOk', () => {
   const overflow = () => ({ error: Object.assign(new Error('x'), { code: 'ENOBUFS' }), stdout: '+partial', status: null });
 
-  const out = scope.defaultRunGit(['diff', 'HEAD', '--'], '/repo', { partialOk: true }, overflow);
+  const out = scope.defaultRunGit(['diff', 'HEAD', '--'], ROOT, { partialOk: true }, overflow);
 
   assert.strictEqual(out, '+partial');
 });
@@ -537,8 +542,8 @@ test('defaultRunGit returns partial output only for diff payloads marked partial
 test('defaultRunGit returns empty output for a failed probe when allowFailure is set', () => {
   const failed = () => ({ status: 128, stdout: '', stderr: 'fatal: bad revision' });
 
-  assert.strictEqual(scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], '/repo', { allowFailure: true }, failed), '');
-  assert.throws(() => scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], '/repo', {}, failed), /bad revision/);
+  assert.strictEqual(scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], ROOT, { allowFailure: true }, failed), '');
+  assert.throws(() => scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], ROOT, {}, failed), /bad revision/);
 });
 
 console.log(`\n${passed} passed, ${failed} failed`);

tests/scripts/astra-review.test.js

@@ -7,6 +7,8 @@
 'use strict';
 
 const assert = require('assert');
+const fs = require('fs');
+const os = require('os');
 const path = require('path');
 const { spawnSync } = require('child_process');
 const cli = require('../../scripts/astra-review');
@@ -156,26 +158,88 @@ test('runCli passes model, prompt, and timeout through to the reviewer', () => {
   assert.ok(seen.prompt.includes('git show sha:<path>'));
 });
 
-test('writeReport refuses to write through an existing symlink', () => {
-  const io = {
-    lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }),
-    writeFileSync: () => { throw new Error('must not write'); },
+function fakeReportIo({ renameFails = false, writeFails = false } = {}) {
+  const events = [];
+  return {
+    events,
+    openSync: (file, flags, mode) => { events.push(['open', file, flags, mode]); return 7; },
+    writeFileSync: (fd, content) => {
+      events.push(['write', fd, content]);
+      if (writeFails) throw Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' });
+    },
+    writeSync: () => { throw new Error('writeSync can return short; use writeFileSync on the descriptor'); },
+    closeSync: (fd) => { events.push(['close', fd]); },
+    renameSync: (from, to) => {
+      events.push(['rename', from, to]);
+      if (renameFails) throw new Error('EXDEV');
+    },
+    unlinkSync: (file) => { events.push(['unlink', file]); },
   };
+}
 
-  assert.throws(() => cli.writeReport('/tmp/x.json', '{}', io), /symlink/);
+const OUT_DIR = path.resolve('/out');
+const REPORT = path.join(OUT_DIR, 'report.json');
+
+test('writeReport creates a private temp file exclusively and renames it over the destination', () => {
+  const io = fakeReportIo();
+
+  cli.writeReport(REPORT, '{}', io);
+
+  const open = io.events.find((event) => event[0] === 'open');
+  assert.ok(open[1].startsWith(path.join(OUT_DIR, '.report.json.')) && open[1].endsWith('.tmp'), `temp file in same dir: ${open[1]}`);
+  assert.strictEqual(open[2], 'wx');
+  assert.strictEqual(open[3], 0o600);
+  const rename = io.events.find((event) => event[0] === 'rename');
+  assert.strictEqual(rename[1], open[1]);
+  assert.strictEqual(rename[2], REPORT);
+  assert.ok(io.events.findIndex((event) => event[0] === 'close') < io.events.indexOf(rename), 'closed before rename');
 });
 
-test('writeReport creates a user-only file when the path is new', () => {
-  let written = null;
-  const io = {
-    lstatSync: () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); },
-    writeFileSync: (file, content, options) => { written = { file, content, options }; },
-  };
+test('writeReport removes the temp file when the rename fails', () => {
+  const io = fakeReportIo({ renameFails: true });
 
-  cli.writeReport('/tmp/new.json', '{}', io);
+  assert.throws(() => cli.writeReport(REPORT, '{}', io), /EXDEV/);
+  assert.ok(io.events.some((event) => event[0] === 'unlink' && event[1].endsWith('.tmp')));
+});
 
-  assert.strictEqual(written.file, '/tmp/new.json');
-  assert.strictEqual(written.options.mode, 0o600);
+test('writeReport writes the whole content via the descriptor and cleans up when the write fails', () => {
+  const ok = fakeReportIo();
+  cli.writeReport(REPORT, '{"full":true}', ok);
+  assert.deepStrictEqual(ok.events.find((event) => event[0] === 'write').slice(1), [7, '{"full":true}']);
+
+  const io = fakeReportIo({ writeFails: true });
+  assert.throws(() => cli.writeReport(REPORT, '{}', io), /ENOSPC/);
+  assert.ok(io.events.some((event) => event[0] === 'close'), 'descriptor closed after failure');
+  assert.ok(io.events.some((event) => event[0] === 'unlink' && event[1].endsWith('.tmp')), 'temp file removed');
+  assert.ok(!io.events.some((event) => event[0] === 'rename'), 'nothing published');
+});
+
+test('writeReport replaces a symlink and a world-readable file with a private regular file (real fs)', () => {
+  const posix = process.platform !== 'win32';
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'astra-report-'));
+  const victim = path.join(dir, 'victim.txt');
+  fs.writeFileSync(victim, 'keep me');
+  const linked = path.join(dir, 'linked.json');
+  let canSymlink = true;
+  try {
+    fs.symlinkSync(victim, linked);
+  } catch {
+    canSymlink = false; // Windows without symlink privilege: replacement is still checked below
+  }
+  const shared = path.join(dir, 'shared.json');
+  fs.writeFileSync(shared, 'old', { mode: 0o644 });
+
+  if (canSymlink) cli.writeReport(linked, '{"a":1}');
+  cli.writeReport(shared, '{"b":2}');
+
+  if (canSymlink) {
+    assert.strictEqual(fs.readFileSync(victim, 'utf8'), 'keep me', 'symlink target untouched');
+    assert.strictEqual(fs.lstatSync(linked).isSymbolicLink(), false, 'symlink replaced by a regular file');
+    assert.strictEqual(fs.readFileSync(linked, 'utf8'), '{"a":1}');
+  }
+  assert.strictEqual(fs.readFileSync(shared, 'utf8'), '{"b":2}');
+  if (posix) assert.strictEqual((fs.statSync(shared).mode & 0o777), 0o600, 'existing 0644 file becomes 0600');
+  assert.deepStrictEqual(fs.readdirSync(dir).filter((name) => name.endsWith('.tmp')), [], 'no temp files left');
 });
 
 test('CLI exits 2 with usage on a bad argument', () => {