Merge remote-tracking branch 'origin/main' into garrytan/zsh-glob-fix

# Conflicts:
#	.agents/skills/gstack-browse/SKILL.md
#	.agents/skills/gstack-design-consultation/SKILL.md
#	.agents/skills/gstack-design-review/SKILL.md
#	.agents/skills/gstack-document-release/SKILL.md
#	.agents/skills/gstack-investigate/SKILL.md
#	.agents/skills/gstack-office-hours/SKILL.md
#	.agents/skills/gstack-plan-ceo-review/SKILL.md
#	.agents/skills/gstack-plan-design-review/SKILL.md
#	.agents/skills/gstack-plan-eng-review/SKILL.md
#	.agents/skills/gstack-qa-only/SKILL.md
#	.agents/skills/gstack-qa/SKILL.md
#	.agents/skills/gstack-retro/SKILL.md
#	.agents/skills/gstack-review/SKILL.md
#	.agents/skills/gstack-setup-browser-cookies/SKILL.md
#	.agents/skills/gstack-ship/SKILL.md
#	.agents/skills/gstack/SKILL.md

browse/SKILL.md

@@ -31,6 +31,9 @@ _PROACTIVE=$(~/.claude/skills/gstack/bin/gstack-config get proactive 2>/dev/null
 _BRANCH=$(git branch --show-current 2>/dev/null || echo "unknown")
 echo "BRANCH: $_BRANCH"
 echo "PROACTIVE: $_PROACTIVE"
+source <(~/.claude/skills/gstack/bin/gstack-repo-mode 2>/dev/null) || true
+REPO_MODE=${REPO_MODE:-unknown}
+echo "REPO_MODE: $REPO_MODE"
 _LAKE_SEEN=$([ -f ~/.gstack/.completeness-intro-seen ] && echo "yes" || echo "no")
 echo "LAKE_INTRO: $_LAKE_SEEN"
 _TEL=$(~/.claude/skills/gstack/bin/gstack-config get telemetry 2>/dev/null || true)
@@ -132,6 +135,38 @@ AI-assisted coding makes the marginal cost of completeness near-zero. When you p
 - BAD: "Let's defer test coverage to a follow-up PR." (Tests are the cheapest lake to boil.)
 - BAD: Quoting only human-team effort: "This would take 2 weeks." (Say: "2 weeks human / ~1 hour CC.")
 
+## Repo Ownership Mode — See Something, Say Something
+
+`REPO_MODE` from the preamble tells you who owns issues in this repo:
+
+- **`solo`** — One person does 80%+ of the work. They own everything. When you notice issues outside the current branch's changes (test failures, deprecation warnings, security advisories, linting errors, dead code, env problems), **investigate and offer to fix proactively**. The solo dev is the only person who will fix it. Default to action.
+- **`collaborative`** — Multiple active contributors. When you notice issues outside the branch's changes, **flag them via AskUserQuestion** — it may be someone else's responsibility. Default to asking, not fixing.
+- **`unknown`** — Treat as collaborative (safer default — ask before fixing).
+
+**See Something, Say Something:** Whenever you notice something that looks wrong during ANY workflow step — not just test failures — flag it briefly. One sentence: what you noticed and its impact. In solo mode, follow up with "Want me to fix it?" In collaborative mode, just flag it and move on.
+
+Never let a noticed issue silently pass. The whole point is proactive communication.
+
+## Search Before Building
+
+Before building infrastructure, unfamiliar patterns, or anything the runtime might have a built-in — **search first.** Read `~/.claude/skills/gstack/ETHOS.md` for the full philosophy.
+
+**Three layers of knowledge:**
+- **Layer 1** (tried and true — in distribution). Don't reinvent the wheel. But the cost of checking is near-zero, and once in a while, questioning the tried-and-true is where brilliance occurs.
+- **Layer 2** (new and popular — search for these). But scrutinize: humans are subject to mania. Search results are inputs to your thinking, not answers.
+- **Layer 3** (first principles — prize these above all). Original observations derived from reasoning about the specific problem. The most valuable of all.
+
+**Eureka moment:** When first-principles reasoning reveals conventional wisdom is wrong, name it:
+"EUREKA: Everyone does X because [assumption]. But [evidence] shows this is wrong. Y is better because [reasoning]."
+
+Log eureka moments:
+```bash
+jq -n --arg ts "$(date -u +%Y-%m-%dT%H:%M:%SZ)" --arg skill "SKILL_NAME" --arg branch "$(git branch --show-current 2>/dev/null)" --arg insight "ONE_LINE_SUMMARY" '{ts:$ts,skill:$skill,branch:$branch,insight:$insight}' >> ~/.gstack/analytics/eureka.jsonl 2>/dev/null || true
+```
+Replace SKILL_NAME and ONE_LINE_SUMMARY. Runs inline — don't stop the workflow.
+
+**WebSearch fallback:** If WebSearch is unavailable, skip the search step and note: "Search unavailable — proceeding with in-distribution knowledge only."
+
 ## Contributor Mode
 
 If `_CONTRIB` is `true`: you are in **contributor mode**. You're a gstack user who also helps make it better.
@@ -222,6 +257,42 @@ success/error/abort, and `USED_BROWSE` with true/false based on whether `$B` was
 If you cannot determine the outcome, use "unknown". This runs in the background and
 never blocks the user.
 
+## Plan Status Footer
+
+When you are in plan mode and about to call ExitPlanMode:
+
+1. Check if the plan file already has a `## GSTACK REVIEW REPORT` section.
+2. If it DOES — skip (a review skill already wrote a richer report).
+3. If it does NOT — run this command:
+
+\`\`\`bash
+~/.claude/skills/gstack/bin/gstack-review-read
+\`\`\`
+
+Then write a `## GSTACK REVIEW REPORT` section to the end of the plan file:
+
+- If the output contains review entries (JSONL lines before `---CONFIG---`): format the
+  standard report table with runs/status/findings per skill, same format as the review
+  skills use.
+- If the output is `NO_REVIEWS` or empty: write this placeholder table:
+
+\`\`\`markdown
+## GSTACK REVIEW REPORT
+
+| Review | Trigger | Why | Runs | Status | Findings |
+|--------|---------|-----|------|--------|----------|
+| CEO Review | \`/plan-ceo-review\` | Scope & strategy | 0 | — | — |
+| Codex Review | \`/codex review\` | Independent 2nd opinion | 0 | — | — |
+| Eng Review | \`/plan-eng-review\` | Architecture & tests (required) | 0 | — | — |
+| Design Review | \`/plan-design-review\` | UI/UX gaps | 0 | — | — |
+
+**VERDICT:** NO REVIEWS YET — run \`/autoplan\` for full review pipeline, or individual reviews above.
+\`\`\`
+
+**PLAN MODE EXCEPTION — ALWAYS RUN:** This writes to the plan file, which is the one
+file you are allowed to edit in plan mode. The plan file review report is part of the
+plan's living status.
+
 # browse: QA Testing & Dogfooding
 
 Persistent headless Chromium. First call auto-starts (~3s), then ~100ms per command.

browse/src/browser-manager.ts

@@ -122,7 +122,7 @@ export class BrowserManager {
 
     // Validate URL before allocating page to avoid zombie tabs on rejection
     if (url) {
-      validateNavigationUrl(url);
+      await validateNavigationUrl(url);
     }
 
     const page = await this.context.newPage();

browse/src/cli.ts

@@ -206,6 +206,34 @@ async function startServer(): Promise<ServerState> {
   throw new Error(`Server failed to start within ${MAX_START_WAIT / 1000}s`);
 }
 
+/**
+ * Acquire an exclusive lockfile to prevent concurrent ensureServer() races (TOCTOU).
+ * Returns a cleanup function that releases the lock.
+ */
+function acquireServerLock(): (() => void) | null {
+  const lockPath = `${config.stateFile}.lock`;
+  try {
+    // O_CREAT | O_EXCL — fails if file already exists (atomic check-and-create)
+    const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY);
+    fs.writeSync(fd, `${process.pid}\n`);
+    fs.closeSync(fd);
+    return () => { try { fs.unlinkSync(lockPath); } catch {} };
+  } catch {
+    // Lock already held — check if the holder is still alive
+    try {
+      const holderPid = parseInt(fs.readFileSync(lockPath, 'utf8').trim(), 10);
+      if (holderPid && isProcessAlive(holderPid)) {
+        return null; // Another live process holds the lock
+      }
+      // Stale lock — remove and retry
+      fs.unlinkSync(lockPath);
+      return acquireServerLock();
+    } catch {
+      return null;
+    }
+  }
+}
+
 async function ensureServer(): Promise<ServerState> {
   const state = readState();
 
@@ -234,9 +262,36 @@ async function ensureServer(): Promise<ServerState> {
     }
   }
 
-  // Need to (re)start
-  console.error('[browse] Starting server...');
-  return startServer();
+  // Acquire lock to prevent concurrent restart races (TOCTOU)
+  const releaseLock = acquireServerLock();
+  if (!releaseLock) {
+    // Another process is starting the server — wait for it
+    console.error('[browse] Another instance is starting the server, waiting...');
+    const start = Date.now();
+    while (Date.now() - start < MAX_START_WAIT) {
+      const freshState = readState();
+      if (freshState && isProcessAlive(freshState.pid)) return freshState;
+      await Bun.sleep(200);
+    }
+    throw new Error('Timed out waiting for another instance to start the server');
+  }
+
+  try {
+    // Re-read state under lock in case another process just started the server
+    const freshState = readState();
+    if (freshState && isProcessAlive(freshState.pid)) {
+      return freshState;
+    }
+
+    // Kill the old server to avoid orphaned chromium processes
+    if (state && state.pid) {
+      await killServer(state.pid);
+    }
+    console.error('[browse] Starting server...');
+    return await startServer();
+  } finally {
+    releaseLock();
+  }
 }
 
 // ─── Command Dispatch ──────────────────────────────────────────
@@ -289,6 +344,11 @@ async function sendCommand(state: ServerState, command: string, args: string[],
     if (err.code === 'ECONNREFUSED' || err.code === 'ECONNRESET' || err.message?.includes('fetch failed')) {
       if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting');
       console.error('[browse] Server connection lost. Restarting...');
+      // Kill the old server to avoid orphaned chromium processes
+      const oldState = readState();
+      if (oldState && oldState.pid) {
+        await killServer(oldState.pid);
+      }
       const newState = await startServer();
       return sendCommand(newState, command, args, retries + 1);
     }

browse/src/meta-commands.ts

@@ -223,11 +223,11 @@ export async function handleMetaCommand(
       if (!url1 || !url2) throw new Error('Usage: browse diff <url1> <url2>');
 
       const page = bm.getPage();
-      validateNavigationUrl(url1);
+      await validateNavigationUrl(url1);
       await page.goto(url1, { waitUntil: 'domcontentloaded', timeout: 15000 });
       const text1 = await getCleanText(page);
 
-      validateNavigationUrl(url2);
+      await validateNavigationUrl(url2);
       await page.goto(url2, { waitUntil: 'domcontentloaded', timeout: 15000 });
       const text2 = await getCleanText(page);
 

browse/src/read-commands.ts

@@ -290,7 +290,21 @@ export async function handleReadCommand(
         localStorage: { ...localStorage },
         sessionStorage: { ...sessionStorage },
       }));
-      return JSON.stringify(storage, null, 2);
+      // Redact values that look like secrets (tokens, keys, passwords, JWTs)
+      const SENSITIVE_KEY = /(^|[_.-])(token|secret|key|password|credential|auth|jwt|session|csrf)($|[_.-])|api.?key/i;
+      const SENSITIVE_VALUE = /^(eyJ|sk-|sk_live_|sk_test_|pk_live_|pk_test_|rk_live_|sk-ant-|ghp_|gho_|github_pat_|xox[bpsa]-|AKIA[A-Z0-9]{16}|AIza|SG\.|Bearer\s|sbp_)/;
+      const redacted = JSON.parse(JSON.stringify(storage));
+      for (const storeType of ['localStorage', 'sessionStorage'] as const) {
+        const store = redacted[storeType];
+        if (!store) continue;
+        for (const [key, value] of Object.entries(store)) {
+          if (typeof value !== 'string') continue;
+          if (SENSITIVE_KEY.test(key) || SENSITIVE_VALUE.test(value)) {
+            store[key] = `[REDACTED — ${value.length} chars]`;
+          }
+        }
+      }
+      return JSON.stringify(redacted, null, 2);
     }
 
     case 'perf': {

browse/src/url-validation.ts

@@ -7,6 +7,7 @@ const BLOCKED_METADATA_HOSTS = new Set([
   '169.254.169.254',  // AWS/GCP/Azure instance metadata
   'fd00::',           // IPv6 unique local (metadata in some cloud setups)
   'metadata.google.internal', // GCP metadata
+  'metadata.azure.internal',  // Azure IMDS
 ]);
 
 /**
@@ -43,7 +44,23 @@ function isMetadataIp(hostname: string): boolean {
   return false;
 }
 
-export function validateNavigationUrl(url: string): void {
+/**
+ * Resolve a hostname to its IP addresses and check if any resolve to blocked metadata IPs.
+ * Mitigates DNS rebinding: even if the hostname looks safe, the resolved IP might not be.
+ */
+async function resolvesToBlockedIp(hostname: string): Promise<boolean> {
+  try {
+    const dns = await import('node:dns');
+    const { resolve4 } = dns.promises;
+    const addresses = await resolve4(hostname);
+    return addresses.some(addr => BLOCKED_METADATA_HOSTS.has(addr));
+  } catch {
+    // DNS resolution failed — not a rebinding risk
+    return false;
+  }
+}
+
+export async function validateNavigationUrl(url: string): Promise<void> {
   let parsed: URL;
   try {
     parsed = new URL(url);
@@ -64,4 +81,11 @@ export function validateNavigationUrl(url: string): void {
       `Blocked: ${parsed.hostname} is a cloud metadata endpoint. Access is denied for security.`
     );
   }
+
+  // DNS rebinding protection: resolve hostname and check if it points to metadata IPs
+  if (await resolvesToBlockedIp(hostname)) {
+    throw new Error(
+      `Blocked: ${parsed.hostname} resolves to a cloud metadata IP. Possible DNS rebinding attack.`
+    );
+  }
 }

browse/src/write-commands.ts

@@ -23,7 +23,7 @@ export async function handleWriteCommand(
     case 'goto': {
       const url = args[0];
       if (!url) throw new Error('Usage: browse goto <url>');
-      validateNavigationUrl(url);
+      await validateNavigationUrl(url);
       const response = await page.goto(url, { waitUntil: 'domcontentloaded', timeout: 15000 });
       const status = response?.status() || 'unknown';
       return `Navigated to ${url} (${status})`;

browse/test/commands.test.ts

@@ -386,10 +386,42 @@ describe('Cookies and storage', () => {
   });
 
   test('storage set and get works', async () => {
-    await handleReadCommand('storage', ['set', 'testKey', 'testValue'], bm);
+    await handleReadCommand('storage', ['set', 'testData', 'testValue'], bm);
     const result = await handleReadCommand('storage', [], bm);
     const storage = JSON.parse(result);
-    expect(storage.localStorage.testKey).toBe('testValue');
+    expect(storage.localStorage.testData).toBe('testValue');
+  });
+
+  test('storage read redacts sensitive keys', async () => {
+    await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm);
+    await handleReadCommand('storage', ['set', 'auth_token', 'my-secret-token'], bm);
+    await handleReadCommand('storage', ['set', 'api_key', 'key-12345'], bm);
+    await handleReadCommand('storage', ['set', 'displayName', 'normalValue'], bm);
+    const result = await handleReadCommand('storage', [], bm);
+    const storage = JSON.parse(result);
+    expect(storage.localStorage.auth_token).toMatch(/REDACTED/);
+    expect(storage.localStorage.api_key).toMatch(/REDACTED/);
+    expect(storage.localStorage.displayName).toBe('normalValue');
+  });
+
+  test('storage read redacts sensitive values by prefix', async () => {
+    await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm);
+    // JWT value under innocuous key name
+    await handleReadCommand('storage', ['set', 'userData', 'eyJhbGciOiJIUzI1NiJ9.payload.sig'], bm);
+    // GitHub PAT under innocuous key name
+    await handleReadCommand('storage', ['set', 'repoAccess', 'ghp_abc123def456'], bm);
+    const result = await handleReadCommand('storage', [], bm);
+    const storage = JSON.parse(result);
+    expect(storage.localStorage.userData).toMatch(/REDACTED/);
+    expect(storage.localStorage.repoAccess).toMatch(/REDACTED/);
+  });
+
+  test('storage redaction includes value length', async () => {
+    await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm);
+    await handleReadCommand('storage', ['set', 'session_token', 'abc123'], bm);
+    const result = await handleReadCommand('storage', [], bm);
+    const storage = JSON.parse(result);
+    expect(storage.localStorage.session_token).toBe('[REDACTED — 6 chars]');
   });
 });
 

browse/test/url-validation.test.ts

@@ -2,67 +2,71 @@ import { describe, it, expect } from 'bun:test';
 import { validateNavigationUrl } from '../src/url-validation';
 
 describe('validateNavigationUrl', () => {
-  it('allows http URLs', () => {
-    expect(() => validateNavigationUrl('http://example.com')).not.toThrow();
+  it('allows http URLs', async () => {
+    await expect(validateNavigationUrl('http://example.com')).resolves.toBeUndefined();
   });
 
-  it('allows https URLs', () => {
-    expect(() => validateNavigationUrl('https://example.com/path?q=1')).not.toThrow();
+  it('allows https URLs', async () => {
+    await expect(validateNavigationUrl('https://example.com/path?q=1')).resolves.toBeUndefined();
   });
 
-  it('allows localhost', () => {
-    expect(() => validateNavigationUrl('http://localhost:3000')).not.toThrow();
+  it('allows localhost', async () => {
+    await expect(validateNavigationUrl('http://localhost:3000')).resolves.toBeUndefined();
   });
 
-  it('allows 127.0.0.1', () => {
-    expect(() => validateNavigationUrl('http://127.0.0.1:8080')).not.toThrow();
+  it('allows 127.0.0.1', async () => {
+    await expect(validateNavigationUrl('http://127.0.0.1:8080')).resolves.toBeUndefined();
   });
 
-  it('allows private IPs', () => {
-    expect(() => validateNavigationUrl('http://192.168.1.1')).not.toThrow();
+  it('allows private IPs', async () => {
+    await expect(validateNavigationUrl('http://192.168.1.1')).resolves.toBeUndefined();
   });
 
-  it('blocks file:// scheme', () => {
-    expect(() => validateNavigationUrl('file:///etc/passwd')).toThrow(/scheme.*not allowed/i);
+  it('blocks file:// scheme', async () => {
+    await expect(validateNavigationUrl('file:///etc/passwd')).rejects.toThrow(/scheme.*not allowed/i);
   });
 
-  it('blocks javascript: scheme', () => {
-    expect(() => validateNavigationUrl('javascript:alert(1)')).toThrow(/scheme.*not allowed/i);
+  it('blocks javascript: scheme', async () => {
+    await expect(validateNavigationUrl('javascript:alert(1)')).rejects.toThrow(/scheme.*not allowed/i);
   });
 
-  it('blocks data: scheme', () => {
-    expect(() => validateNavigationUrl('data:text/html,<h1>hi</h1>')).toThrow(/scheme.*not allowed/i);
+  it('blocks data: scheme', async () => {
+    await expect(validateNavigationUrl('data:text/html,<h1>hi</h1>')).rejects.toThrow(/scheme.*not allowed/i);
   });
 
-  it('blocks AWS/GCP metadata endpoint', () => {
-    expect(() => validateNavigationUrl('http://169.254.169.254/latest/meta-data/')).toThrow(/cloud metadata/i);
+  it('blocks AWS/GCP metadata endpoint', async () => {
+    await expect(validateNavigationUrl('http://169.254.169.254/latest/meta-data/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks GCP metadata hostname', () => {
-    expect(() => validateNavigationUrl('http://metadata.google.internal/computeMetadata/v1/')).toThrow(/cloud metadata/i);
+  it('blocks GCP metadata hostname', async () => {
+    await expect(validateNavigationUrl('http://metadata.google.internal/computeMetadata/v1/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks metadata hostname with trailing dot', () => {
-    expect(() => validateNavigationUrl('http://metadata.google.internal./computeMetadata/v1/')).toThrow(/cloud metadata/i);
+  it('blocks Azure metadata hostname', async () => {
+    await expect(validateNavigationUrl('http://metadata.azure.internal/metadata/instance')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks metadata IP in hex form', () => {
-    expect(() => validateNavigationUrl('http://0xA9FEA9FE/')).toThrow(/cloud metadata/i);
+  it('blocks metadata hostname with trailing dot', async () => {
+    await expect(validateNavigationUrl('http://metadata.google.internal./computeMetadata/v1/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks metadata IP in decimal form', () => {
-    expect(() => validateNavigationUrl('http://2852039166/')).toThrow(/cloud metadata/i);
+  it('blocks metadata IP in hex form', async () => {
+    await expect(validateNavigationUrl('http://0xA9FEA9FE/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks metadata IP in octal form', () => {
-    expect(() => validateNavigationUrl('http://0251.0376.0251.0376/')).toThrow(/cloud metadata/i);
+  it('blocks metadata IP in decimal form', async () => {
+    await expect(validateNavigationUrl('http://2852039166/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('blocks IPv6 metadata with brackets', () => {
-    expect(() => validateNavigationUrl('http://[fd00::]/')).toThrow(/cloud metadata/i);
+  it('blocks metadata IP in octal form', async () => {
+    await expect(validateNavigationUrl('http://0251.0376.0251.0376/')).rejects.toThrow(/cloud metadata/i);
   });
 
-  it('throws on malformed URLs', () => {
-    expect(() => validateNavigationUrl('not-a-url')).toThrow(/Invalid URL/i);
+  it('blocks IPv6 metadata with brackets', async () => {
+    await expect(validateNavigationUrl('http://[fd00::]/')).rejects.toThrow(/cloud metadata/i);
+  });
+
+  it('throws on malformed URLs', async () => {
+    await expect(validateNavigationUrl('not-a-url')).rejects.toThrow(/Invalid URL/i);
   });
 });