Merge origin/main (v1.64.0.0) into garrytan/time-attack-fork-review

Both waves fixed several of the same bugs; resolutions keep whichever
shape this branch's tests pin (#2018 jq bind, #1798 set-- pattern,
stop-ack, lock errors, polyfill windowsHide) and take main's richer
codex Step 2A (it absorbed the same mktemp fix). True unions: memory-
ingest keeps main's capability-probed --include-gitignored inside our
GIT_CEILING defense; setup wraps main's Playwright platform override in
our stale-healing install lock; package.json takes main's diff@^9 and
the combined test glob (design/test + ios-qa/daemon/test, 30s timeout).
Generated SKILL.md files regenerated from resolved templates, never
hand-picked. Ship goldens refreshed; parity/carve budgets re-measured
for the summed preamble growth of both waves (itemized per entry).

browse/src/browser-manager.ts

@@ -167,7 +167,11 @@ export function probePoisonedChromiumBundle(chromiumExecutablePath: string): voi
  * restarts on backoff.
  */
 export async function resolveDisconnectCause(browser: Browser | null): Promise<'clean' | 'crash'> {
-  const proc = browser?.process();
+  // `.process()` only exists on browsers we launched ourselves. A browser
+  // obtained via connectOverCDP() (or a stub in tests) has no such method —
+  // calling it blind throws inside the disconnect handler, which killed the
+  // whole daemon with "browser?.process is not a function".
+  const proc = typeof browser?.process === 'function' ? browser.process() : null;
   if (proc && proc.exitCode === null && proc.signalCode === null) {
     await new Promise<void>((resolve) => {
       const timer = setTimeout(resolve, 1000);
@@ -869,19 +873,31 @@ export class BrowserManager {
     const page = this.pages.get(tabId);
     if (!page) throw new Error(`Tab ${tabId} not found`);
 
+    // Capture BEFORE close(): the page 'close' event handler wired in
+    // wirePageEvents() can fire while page.close() is awaited. It removes
+    // the tab from the maps and reassigns activeTabId (to 0 when no tabs
+    // remain), so a post-close `tabId === this.activeTabId` check is
+    // order-dependent — whether the event dispatches before or after
+    // close() resolves varies across Playwright/Chromium versions and
+    // machines, and losing the race means the last-tab auto-create below
+    // never runs, leaving the manager with zero tabs.
+    const wasActive = tabId === this.activeTabId;
+
     await page.close();
     this.pages.delete(tabId);
     this.tabSessions.delete(tabId);
     this.tabOwnership.delete(tabId);
 
     // Switch to another tab if we closed the active one
-    if (tabId === this.activeTabId) {
+    if (wasActive) {
       const remaining = [...this.pages.keys()];
-      if (remaining.length > 0) {
-        this.activeTabId = remaining[remaining.length - 1];
-      } else {
+      if (remaining.length === 0) {
         // No tabs left — create a new blank one
         await this.newTab();
+      } else if (!this.pages.has(this.activeTabId)) {
+        // The 'close' handler may have already switched to a valid tab;
+        // only reassign when activeTabId no longer points at a live tab.
+        this.activeTabId = remaining[remaining.length - 1];
       }
     }
   }

browse/src/config.ts

@@ -34,7 +34,12 @@ export function getGitRoot(): string | null {
     const proc = Bun.spawnSync(['git', 'rev-parse', '--show-toplevel'], {
       stdout: 'pipe',
       stderr: 'pipe',
-      timeout: 2_000, // Don't hang if .git is broken
+      // Raised from 2s: under heavy machine load `git rev-parse` routinely
+      // takes >2s (measured 6.3s spikes). Timing out here returns null →
+      // resolveConfig falls back to process.cwd() → state files scatter across
+      // cwds (split-brain daemons; `goto` and `url` hit different servers). 8s
+      // still bounds a genuinely broken .git from hanging the CLI forever.
+      timeout: 8_000,
     });
     if (proc.exitCode !== 0) return null;
     return proc.stdout.toString().trim() || null;
@@ -78,6 +83,20 @@ export function resolveConfig(
   };
 }
 
+function isIgnoredByGit(projectDir: string, relPath: string): boolean {
+  try {
+    const proc = Bun.spawnSync(['git', 'check-ignore', '-q', '--', relPath], {
+      cwd: projectDir, stdout: 'pipe', stderr: 'pipe',
+      timeout: 2_000,
+    });
+    return proc.exitCode === 0;
+  } catch {
+    // git not found, timed out, or not a repo (exit 128). Fall through to
+    // the text-check path — appending is the safe default when unsure.
+    return false;
+  }
+}
+
 /**
  * Create the .gstack/ state directory if it doesn't exist.
  * Throws with a clear message on permission errors.
@@ -96,6 +115,9 @@ export function ensureStateDir(config: BrowseConfig): void {
   }
 
   // Ensure .gstack/ is in the project's .gitignore
+  // First, check if git already ignores .gstack/ (via global excludes, .git/info/exclude, or parent .gitignore)
+  if (isIgnoredByGit(config.projectDir, '.gstack/')) return;
+
   const gitignorePath = path.join(config.projectDir, '.gitignore');
   try {
     const content = fs.readFileSync(gitignorePath, 'utf-8');

browse/src/file-permissions.ts

@@ -42,6 +42,52 @@ import * as os from 'os';
 
 let warnedOnce = false;
 
+let cachedSid: string | null | undefined;
+
+/**
+ * Resolve the current user's SID, cached for the process lifetime.
+ *
+ * Returns null if `whoami` is unavailable or its output cannot be parsed,
+ * in which case callers fall back to a domain-qualified account name.
+ */
+function currentUserSid(): string | null {
+  if (cachedSid !== undefined) return cachedSid;
+  try {
+    // Pin to the System32 binary. A bare `whoami` resolves to the MSYS/Git
+    // Bash build under a bash-flavoured PATH, which rejects `/user` — the
+    // lookup would then silently fail on one of the most common Windows
+    // setups for this tool.
+    const systemRoot = process.env.SystemRoot || process.env.windir || 'C:\\Windows';
+    const out = execFileSync(`${systemRoot}\\System32\\whoami.exe`, ['/user', '/fo', 'csv', '/nh'], {
+      encoding: 'utf8',
+    });
+    const match = out.match(/S-1-[\d-]+/);
+    cachedSid = match ? match[0] : null;
+  } catch {
+    cachedSid = null;
+  }
+  return cachedSid;
+}
+
+/**
+ * The principal to hand icacls for "the current user".
+ *
+ * An unqualified username is ambiguous: on a machine whose hostname equals
+ * the username, it fails to resolve to the user account and icacls silently
+ * writes an ACE for the machine SID instead. Combined with `/inheritance:r`
+ * that leaves a directory whose only ACE matches nobody — locking out the
+ * process that just created it.
+ *
+ * `*<SID>` is icacls' literal-SID form and is immune to that ambiguity.
+ * The domain-qualified name is the fallback.
+ */
+function currentUserPrincipal(): string {
+  const sid = currentUserSid();
+  if (sid) return `*${sid}`;
+  const domain = process.env.USERDOMAIN || os.hostname();
+  return `${domain}\\${os.userInfo().username}`;
+}
+
 function warnIcaclsFailure(fsPath: string, err: unknown): void {
   if (warnedOnce) return;
   warnedOnce = true;
@@ -67,7 +113,7 @@ function warnIcaclsFailure(fsPath: string, err: unknown): void {
 export function restrictFilePermissions(filePath: string): void {
   if (process.platform === 'win32') {
     try {
-      const user = os.userInfo().username;
+      const user = currentUserPrincipal();
       execFileSync(
         'icacls',
         [filePath, '/inheritance:r', '/grant:r', `${user}:(F)`],
@@ -97,7 +143,7 @@ export function restrictFilePermissions(filePath: string): void {
 export function restrictDirectoryPermissions(dirPath: string): void {
   if (process.platform === 'win32') {
     try {
-      const user = os.userInfo().username;
+      const user = currentUserPrincipal();
       execFileSync(
         'icacls',
         [dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`],

browse/src/server.ts

@@ -1592,8 +1592,18 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
     process.env.GSTACK_AGENT_WATCHDOG_TICK_MS || '60000',
     10,
   );
-  const RESPAWN_GUARD_WINDOW_MS = 60_000;
   const RESPAWN_GUARD_MAX = 3;
+  // The guard window MUST span enough ticks for RESPAWN_GUARD_MAX respawns to
+  // land inside it. This was a fixed 60_000 against a 60_000 tick, so at most
+  // ONE respawn could ever be in the window and `respawnHistory.length >= 3`
+  // was unreachable — the guard could not fire at the default tick rate, and a
+  // steady one-per-tick leak ran unbounded instead of stopping after 3. Scale
+  // with the tick so the intent ("3 crashes in quick succession → stop") holds
+  // at any tick value: 3 respawns within 5 ticks trips it.
+  const RESPAWN_GUARD_WINDOW_MS = Math.max(
+    60_000,
+    AGENT_WATCHDOG_TICK_MS * (RESPAWN_GUARD_MAX + 2),
+  );
   let agentRespawnGuardTripped = false;
 
   if (ownsTerminalAgent) {
@@ -1626,6 +1636,7 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
         const pid = spawnTerminalAgent({
           stateFile: cfg.config.stateFile,
           serverPort: cfg.browsePort,
+          ownerPid: process.pid,
           cwd: cfg.config.projectDir,
         });
         if (pid) {

browse/src/terminal-agent-control.ts

@@ -48,12 +48,13 @@ export function resolveTerminalAgentScript(searchHints: { metaDir?: string; exec
  *
  * Used by both the CLI cold-start path (cli.ts) and the v1.44 watchdog in
  * server.ts. Centralizing here removes a copy-paste between them and means
- * future spawn-env additions (e.g. BROWSE_OWNER_PID for the generation
- * counter rollout) land in one place.
+ * spawn-env additions (BROWSE_OWNER_PID being the first) land in one place.
  */
 export function spawnTerminalAgent(opts: {
   stateFile: string;
   serverPort: number;
+  /** PID of the browse server that owns this agent. */
+  ownerPid: number;
   cwd?: string;
   /** Optional extra env vars to add to the agent's process env. */
   extraEnv?: Record<string, string>;
@@ -74,6 +75,7 @@ export function spawnTerminalAgent(opts: {
       ...process.env,
       BROWSE_STATE_FILE: opts.stateFile,
       BROWSE_SERVER_PORT: String(opts.serverPort),
+      BROWSE_OWNER_PID: String(opts.ownerPid),
       ...(opts.extraEnv || {}),
     },
     stdio: ['ignore', 'ignore', 'ignore'],

browse/src/terminal-agent.ts

@@ -30,6 +30,11 @@ import { writeAgentRecord, clearAgentRecord } from './terminal-agent-control';
 const STATE_FILE = process.env.BROWSE_STATE_FILE || path.join(process.env.HOME || '/tmp', '.gstack', 'browse.json');
 const PORT_FILE = path.join(path.dirname(STATE_FILE), 'terminal-port');
 const BROWSE_SERVER_PORT = parseInt(process.env.BROWSE_SERVER_PORT || '0', 10);
+const BROWSE_OWNER_PID = parseInt(process.env.BROWSE_OWNER_PID || '0', 10);
+const OWNER_WATCHDOG_MS = parseInt(
+  process.env.GSTACK_TERMINAL_OWNER_WATCHDOG_MS || '15000',
+  10,
+);
 const EXTENSION_ID = process.env.BROWSE_EXTENSION_ID || ''; // optional: tighten Origin check
 const INTERNAL_TOKEN = crypto.randomBytes(32).toString('base64url'); // shared with parent server via env at spawn
 /**
@@ -598,12 +603,10 @@ function buildServer() {
         // first that matches a known token.
         const protoHeader = req.headers.get('sec-websocket-protocol') || '';
         let token: string | null = null;
-        let acceptedProtocol: string | null = null;
         for (const raw of protoHeader.split(',').map(s => s.trim()).filter(Boolean)) {
           const candidate = raw.startsWith('gstack-pty.') ? raw.slice('gstack-pty.'.length) : raw;
           if (validTokens.has(candidate)) {
             token = candidate;
-            acceptedProtocol = raw;
             break;
           }
         }
@@ -632,13 +635,13 @@ function buildServer() {
         // sessionsById so /internal/restart and (Commit 3) re-attach
         // lookups can find it.
         const sessionId = validTokens.get(token) ?? null;
+        // No explicit Sec-WebSocket-Protocol echo: Bun >= 1.3 auto-echoes the
+        // first offered protocol in the 101 response, so setting the header
+        // here produced a DUPLICATE header — strict clients (Chromium, python
+        // websockets) reject the handshake per RFC 6455 and the sidebar
+        // terminal could never connect. Verified on Bun 1.3.6.
         const upgraded = server.upgrade(req, {
           data: { cookie: token, sessionId },
-          // Echo the protocol back so the browser accepts the upgrade.
-          // Required when the client sends Sec-WebSocket-Protocol — the
-          // server MUST select one of the offered protocols, otherwise
-          // the browser closes the connection immediately.
-          ...(acceptedProtocol ? { headers: { 'Sec-WebSocket-Protocol': acceptedProtocol } } : {}),
         });
         return upgraded ? undefined : new Response('upgrade failed', { status: 500 });
       }
@@ -987,13 +990,33 @@ function main() {
   console.log(`[terminal-agent] listening on 127.0.0.1:${port} pid=${process.pid} gen=${CURRENT_GEN}`);
 
   // Cleanup port file + agent record on exit.
+  let cleaningUp = false;
   const cleanup = () => {
+    if (cleaningUp) return;
+    cleaningUp = true;
     safeUnlink(PORT_FILE);
+    safeUnlink(INTERNAL_TOKEN_FILE);
     clearAgentRecord(dir);
     process.exit(0);
   };
   process.on('SIGTERM', cleanup);
   process.on('SIGINT', cleanup);
+
+  // The terminal agent is intentionally detached so it survives the short-lived
+  // CLI launcher, but its real owner is the persistent browse server. If that
+  // server crashes or is killed before running normal shutdown, the agent would
+  // otherwise be adopted by PID 1 and live forever. Poll the server PID and use
+  // the same cleanup path as an intentional shutdown when it disappears.
+  if (BROWSE_OWNER_PID > 0) {
+    const ownerWatchdog = setInterval(() => {
+      try {
+        process.kill(BROWSE_OWNER_PID, 0);
+      } catch {
+        cleanup();
+      }
+    }, OWNER_WATCHDOG_MS);
+    (ownerWatchdog as any)?.unref?.();
+  }
 }
 
 // Export the internal token so cli.ts can pass the SAME value to the parent

browse/src/url-validation.ts

@@ -269,9 +269,24 @@ export async function validateNavigationUrl(url: string): Promise<string> {
     return pathToFileURL(fsPath).href + parsed.search + parsed.hash;
   }
 
+  // about:blank ONLY — the canonical empty page, and the one the daemon opens its own
+  // first tab on. Blocking it meant `browse newtab about:blank` failed, which is what
+  // `make-pdf setup` runs as its Chromium smoke test: make-pdf reported "Chromium failed
+  // to launch" against a perfectly healthy Chromium, and any browse session whose daemon
+  // restarted could never recreate the blank tab it starts from.
+  //
+  // Deliberately not the whole `about:` scheme. about:blank has no origin, loads nothing
+  // and runs nothing; about:config, about:net-internals and friends are real surfaces.
+  // Exact href match, not a prefix test, so `about:blankfoo` stays blocked.
+  // Compared lower-cased: the URL parser normalises the PROTOCOL but not the opaque part,
+  // so `ABOUT:BLANK` parses to href `about:BLANK` and an exact === would reject it.
+  if (parsed.protocol === 'about:' && parsed.href.toLowerCase() === 'about:blank') {
+    return 'about:blank';
+  }
+
   if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
     throw new Error(
-      `Blocked: scheme "${parsed.protocol}" is not allowed. Only http:, https:, and file: URLs are permitted.`
+      `Blocked: scheme "${parsed.protocol}" is not allowed. Only http:, https:, file:, and about:blank URLs are permitted.`
     );
   }
 

browse/src/write-commands.ts

@@ -249,11 +249,11 @@ export async function handleWriteCommand(
       if (!filePath) throw new Error('Usage: browse load-html <file> [--wait-until load|domcontentloaded|networkidle] [--tab-id <N>]  |  load-html --from-file <payload.json> [--tab-id <N>]');
 
       // Extension allowlist
-      const ALLOWED_EXT = ['.html', '.htm', '.xhtml', '.svg'];
+      const ALLOWED_EXT = ['.html', '.htm', '.xhtml'];
       const ext = path.extname(filePath).toLowerCase();
       if (!ALLOWED_EXT.includes(ext)) {
         throw new Error(
-          `load-html: file does not appear to be HTML. Expected .html/.htm/.xhtml/.svg, got ${ext || '(no extension)'}. Rename the file if it's really HTML.`
+          `load-html: file does not appear to be HTML. Expected .html/.htm/.xhtml, got ${ext || '(no extension)'}. Rename the file if it's really HTML.`
         );
       }
 
@@ -377,11 +377,14 @@ export async function handleWriteCommand(
       const value = valueParts.join(' ');
       if (!selector || !value) throw new Error('Usage: browse fill <selector> <value>');
       const resolved = await session.resolveRef(selector);
-      if ('locator' in resolved) {
-        await resolved.locator.fill(value, { timeout: 5000 });
-      } else {
-        await target.locator(resolved.selector).fill(value, { timeout: 5000 });
-      }
+      const locator = 'locator' in resolved ? resolved.locator : target.locator(resolved.selector);
+      await locator.fill(value, { timeout: 5000 });
+      // Playwright's fill() only dispatches an `input` event. Frameworks that
+      // validate on `change` (AngularJS ng-change, debounced strength/match
+      // checks — e.g. cPanel's Jupiter theme) never see the update, so a value
+      // that's correct in the DOM can still fail the framework's own
+      // validation. Dispatch `change` too so those listeners fire.
+      await locator.dispatchEvent('change');
       // Wait for network to settle (form validation XHRs)
       await page.waitForLoadState('networkidle', { timeout: 2000 }).catch(() => {});
       return `Filled ${selector}`;