Confine demo fixtures and isolate helper Git environments

Author: mchwang

docs/implementation/read-only-review.md

@@ -38,3 +38,7 @@ Reproduced and fixed no-change approval with item-owned ambiguous segments; the
 ## Review round 2
 
 Extended the attribution guard to mixed items with both owned and ambiguous segments. The regression reproduced ordinary approval succeeding with unresolved attribution. Approval now rejects any item-specific ambiguity, previously approved affected items become stale, and the UI routes every such item to attribution resolution. Added browser coverage for this mixed case. No findings declined.
+
+A toolbar-focus browser regression reproduced review shortcuts being disabled while a button had focus. Only text-entry/select controls now suppress the single-letter shortcuts; native Enter/Space button behavior is unchanged. The plan-row click case already worked because rendering replaces the focused row.
+
+Review round 3 identified demo fixture config escapes and inherited Git environment redirection. Regression checks first reproduced the config, symlink, and environment failures. Existing demos now require exact fixture paths and ordinary config/database/repository objects (including `.git` and SQLite sidecars). Demo and plant Git commands share a case-insensitive environment scrub and disable inherited global/system Git configuration. Four helper tests cover these boundaries, including mixed-case variable names for Windows.

scripts/demo.ts

@@ -1,17 +1,30 @@
-import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync } from 'node:fs';
+import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync, lstatSync } from 'node:fs';
 import { resolve, join } from 'node:path';
 import { execFileSync } from 'node:child_process';
 import { randomUUID } from 'node:crypto';
 import { Store } from '../runner/store.ts';
 import type { ReviewConfig } from '../runner/review.ts';
 import type { Plan } from '../core/plan.ts';
+import { isolatedGitEnvironment } from './git-environment.ts';
 /** Disposable fixture only. Never runs against the user's working repository. */
 export function createDemo(directory: string): ReviewConfig {
   const root = resolve(directory), configPath = join(root, 'review.json');
-  if (existsSync(configPath)) return JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig;
+  if (existsSync(root) && lstatSync(root).isSymbolicLink()) throw new Error('Demo fixture root must not be a symlink.');
+  if (existsSync(configPath)) {
+    const check = (path: string, directory: boolean) => {
+      const stat = lstatSync(path);
+      if (stat.isSymbolicLink() || (directory ? !stat.isDirectory() : !stat.isFile())) throw new Error('Demo fixture paths must be ordinary local files and directories.');
+    };
+    check(configPath, false);
+    const config = JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig;
+    if (config.demo !== true || config.repository !== join(root, 'retry-service') || config.database !== join(root, 'review.sqlite')) throw new Error('Demo configuration must refer to its own fixture.');
+    check(config.repository, true); check(join(config.repository, '.git'), true); check(config.database, false);
+    for (const suffix of ['-wal', '-shm']) if (existsSync(config.database + suffix)) check(config.database + suffix, false);
+    return config;
+  }
   if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.');
   mkdirSync(root, { recursive: true }); const repository = join(root, 'retry-service'); mkdirSync(repository);
-  const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim();
+  const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, env: isolatedGitEnvironment(), encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim();
   git('init','-b','main'); git('config','user.name','Codeboost Demo'); git('config','user.email','demo@example.invalid'); git('config','commit.gpgsign','false');
   const write = (path: string, text: string | Buffer) => writeFileSync(join(repository,path),text);
   const commit = (message: string) => { git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD'); };

scripts/git-environment.ts

@@ -0,0 +1,9 @@
+/** Git environment names are case-insensitive on Windows. */
+export function isolatedGitEnvironment(): NodeJS.ProcessEnv {
+  return {
+    ...Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key))),
+    GIT_CONFIG_NOSYSTEM: '1',
+    GIT_CONFIG_GLOBAL: '/dev/null',
+    GIT_TERMINAL_PROMPT: '0',
+  };
+}

scripts/plant.ts

@@ -1,3 +1,4 @@
+import { isolatedGitEnvironment } from './git-environment.ts';
 import { execFileSync } from 'node:child_process';
 import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync } from 'node:fs';
 import { resolve, join, dirname } from 'node:path';
@@ -29,7 +30,7 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp
   const declared=candidates[randomInt(candidates.length)]!;
   const owned=history.commits.map((commit,index)=>({index,owner:owners.get(commit.sha)})).filter(entry=>entry.owner&&plan!.items.some(item=>item.id===entry.owner));
   const outside=owned[randomInt(owned.length)]!;
-  const env={...Object.fromEntries(Object.entries(process.env).filter(([key])=>!key.startsWith('GIT_'))),GIT_CONFIG_NOSYSTEM:'1',GIT_CONFIG_GLOBAL:'/dev/null'};
+  const env=isolatedGitEnvironment();
   const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',['-c','core.hooksPath=/dev/null','-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024});
   const git=(cwd:string,...args:string[])=>gitRaw(cwd,...args).trim();
   // Refuse existing paths, symlink targets, and unsupported declared-file transitions before creating output.

test/browser/review.spec.ts

@@ -74,3 +74,6 @@ test('routes mixed owned and ambiguous items to attribution resolution',async({p
  const head=execFileSync('git',['rev-parse','HEAD'],{cwd:repository,encoding:'utf8'}).trim(),snapshot=app.service.store.getSnapshot(identity);app.service.store.recordHistory(identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]);
  await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await page.getByRole('button',{name:'Resolve ambiguous changes',exact:true}).click();await expect(page.getByRole('heading',{name:'Ambiguous',exact:true})).toBeVisible();
 });
+test('keeps review shortcuts active while a toolbar button has focus',async({page})=>{
+ await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();await page.getByRole('button',{name:'Refresh',exact:true}).focus();await page.keyboard.press('n');await expect(page.getByRole('heading',{name:'Document retry behavior'})).toBeVisible();
+});

test/demo.test.ts

@@ -0,0 +1,46 @@
+import { afterEach, expect, it, vi } from 'vitest';
+import { mkdtempSync, renameSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { isolatedGitEnvironment } from '../scripts/git-environment.ts';
+import { createDemo } from '../scripts/demo.ts';
+const roots: string[] = [];
+afterEach(() => { vi.unstubAllEnvs(); roots.splice(0).forEach(root => rmSync(root, { recursive: true, force: true })); });
+function root() { const path = mkdtempSync(join(tmpdir(), 'codeboost-demo-')); roots.push(path); return path; }
+it('rejects existing demo configs redirected outside their fixture', () => {
+  const base = root(), directory = join(base, 'demo'), config = createDemo(directory);
+  expect(createDemo(directory)).toEqual(config);
+  for (const key of ['repository', 'database'] as const) {
+    writeFileSync(join(directory, 'review.json'), JSON.stringify({ ...config, [key]: join(base, 'outside') }));
+    expect(() => createDemo(directory)).toThrow(/fixture/i);
+  }
+});
+it('rejects symlinked demo roots, configs, repositories and databases', () => {
+  const base = root(), directory = join(base, 'demo'); createDemo(directory);
+  symlinkSync(directory, join(base, 'alias'));
+  expect(() => createDemo(join(base, 'alias'))).toThrow(/fixture/i);
+  for (const name of ['review.json', 'retry-service', 'review.sqlite']) {
+    const path = join(directory, name), backup = join(base, name);
+    // Move each object out of the fixture and replace it with a symlink.
+    renameSync(path, backup); symlinkSync(backup, path);
+    expect(() => createDemo(directory)).toThrow(/fixture/i);
+    rmSync(path); renameSync(backup, path);
+  }
+});
+it('scrubs inherited Git repository and config environment in demo commands', () => {
+  const base = root();
+  vi.stubEnv('GIT_DIR', join(base, 'outside.git'));
+  vi.stubEnv('GIT_WORK_TREE', join(base, 'outside'));
+  vi.stubEnv('GIT_CONFIG_COUNT', '1');
+  vi.stubEnv('GIT_CONFIG_KEY_0', 'init.defaultBranch');
+  vi.stubEnv('GIT_CONFIG_VALUE_0', 'inherited');
+  const config = createDemo(join(base, 'demo'));
+  expect(readFileSync(join(config.repository, 'retry.ts'), 'utf8')).toContain('Math.min');
+});
+
+it('scrubs Git environment names case-insensitively for Windows', () => {
+  vi.stubEnv('git_dir', '/outside'); vi.stubEnv('Git_Work_Tree', '/outside');
+  const env = isolatedGitEnvironment();
+  expect(env).not.toHaveProperty('git_dir'); expect(env).not.toHaveProperty('Git_Work_Tree');
+  expect(env.GIT_CONFIG_NOSYSTEM).toBe('1');
+});

web/public/app.js

@@ -397,7 +397,7 @@ document.addEventListener("keydown", (event) => {
     event.metaKey ||
     event.altKey ||
     $("dialog").open ||
-    ["TEXTAREA", "INPUT", "SELECT", "BUTTON"].includes(
+    ["TEXTAREA", "INPUT", "SELECT"].includes(
       document.activeElement?.tagName,
     ) ||
     !data