fix: extend security sanitization + add 10 tests for merged community PRs

- Extend json_safe() to ERROR_CLASS and FAILED_STEP fields
- Improve ERROR_MESSAGE escaping to handle backslashes and newlines
- Replace python3 with bun for JSON validation in gstack-review-log
- Add 7 telemetry injection prevention tests
- Add 2 review-log JSON validation tests
- Add 1 discover-skills hidden directory filtering test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

bin/gstack-review-log

@@ -9,7 +9,7 @@ mkdir -p "$GSTACK_HOME/projects/$SLUG"
 
 # Validate: input must be parseable JSON (reject malformed or injection attempts)
 INPUT="$1"
-if ! printf '%s' "$INPUT" | python3 -c "import json,sys; json.load(sys.stdin)" 2>/dev/null; then
+if ! printf '%s' "$INPUT" | bun -e "JSON.parse(await Bun.stdin.text())" 2>/dev/null; then
   # Not valid JSON — refuse to append
   echo "gstack-review-log: invalid JSON, skipping" >&2
   exit 1

bin/gstack-telemetry-log

@@ -159,15 +159,15 @@ SESSION_ID="$(json_safe "$SESSION_ID")"
 SOURCE="$(json_safe "$SOURCE")"
 EVENT_TYPE="$(json_safe "$EVENT_TYPE")"
 
-# Escape null fields
+# Escape null fields — sanitize ERROR_CLASS and FAILED_STEP via json_safe()
 ERR_FIELD="null"
-[ -n "$ERROR_CLASS" ] && ERR_FIELD="\"$ERROR_CLASS\""
+[ -n "$ERROR_CLASS" ] && ERR_FIELD="\"$(json_safe "$ERROR_CLASS")\""
 
 ERR_MSG_FIELD="null"
-[ -n "$ERROR_MESSAGE" ] && ERR_MSG_FIELD="\"$(echo "$ERROR_MESSAGE" | head -c 200 | sed 's/"/\\"/g')\""
+[ -n "$ERROR_MESSAGE" ] && ERR_MSG_FIELD="\"$(printf '%s' "$ERROR_MESSAGE" | head -c 200 | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' -e 's/	/\\t/g' | tr '\n\r' '  ')\""
 
 STEP_FIELD="null"
-[ -n "$FAILED_STEP" ] && STEP_FIELD="\"$(echo "$FAILED_STEP" | head -c 30)\""
+[ -n "$FAILED_STEP" ] && STEP_FIELD="\"$(json_safe "$FAILED_STEP")\""
 
 # Cap unreasonable durations
 if [ -n "$DURATION" ] && [ "$DURATION" -gt 86400 ] 2>/dev/null; then

test/gen-skill-docs.test.ts

@@ -3,6 +3,7 @@ import { COMMAND_DESCRIPTIONS } from '../browse/src/commands';
 import { SNAPSHOT_FLAGS } from '../browse/src/snapshot';
 import * as fs from 'fs';
 import * as path from 'path';
+import * as os from 'os';
 
 const ROOT = path.resolve(import.meta.dir, '..');
 const MAX_SKILL_DESCRIPTION_LENGTH = 1024;
@@ -1599,6 +1600,29 @@ describe('setup script validation', () => {
   });
 });
 
+describe('discover-skills hidden directory filtering', () => {
+  test('discoverTemplates skips dot-prefixed directories', () => {
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-discover-'));
+    try {
+      // Create a hidden dir with a template (should be excluded)
+      fs.mkdirSync(path.join(tmpDir, '.hidden'), { recursive: true });
+      fs.writeFileSync(path.join(tmpDir, '.hidden', 'SKILL.md.tmpl'), '---\nname: evil\n---\ntest');
+      // Create a visible dir with a template (should be included)
+      fs.mkdirSync(path.join(tmpDir, 'visible'), { recursive: true });
+      fs.writeFileSync(path.join(tmpDir, 'visible', 'SKILL.md.tmpl'), '---\nname: good\n---\ntest');
+
+      const { discoverTemplates } = require('../scripts/discover-skills');
+      const results = discoverTemplates(tmpDir);
+      const dirs = results.map((r: { tmpl: string }) => r.tmpl);
+
+      expect(dirs).toContain('visible/SKILL.md.tmpl');
+      expect(dirs).not.toContain('.hidden/SKILL.md.tmpl');
+    } finally {
+      fs.rmSync(tmpDir, { recursive: true, force: true });
+    }
+  });
+});
+
 describe('telemetry', () => {
   test('generated SKILL.md contains telemetry start block', () => {
     const content = fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8');

test/review-log.test.ts

@@ -0,0 +1,77 @@
+import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
+import { execSync, ExecSyncOptionsWithStringEncoding } from 'child_process';
+import * as fs from 'fs';
+import * as path from 'path';
+import * as os from 'os';
+
+const ROOT = path.resolve(import.meta.dir, '..');
+const BIN = path.join(ROOT, 'bin');
+
+let tmpDir: string;
+let slugDir: string;
+
+function run(input: string, opts: { expectFail?: boolean } = {}): { stdout: string; exitCode: number } {
+  const execOpts: ExecSyncOptionsWithStringEncoding = {
+    cwd: ROOT,
+    env: { ...process.env, GSTACK_HOME: tmpDir },
+    encoding: 'utf-8',
+    timeout: 10000,
+  };
+  try {
+    const stdout = execSync(`${BIN}/gstack-review-log '${input.replace(/'/g, "'\\''")}'`, execOpts).trim();
+    return { stdout, exitCode: 0 };
+  } catch (e: any) {
+    if (opts.expectFail) {
+      return { stdout: e.stderr?.toString() || '', exitCode: e.status || 1 };
+    }
+    throw e;
+  }
+}
+
+beforeEach(() => {
+  tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-revlog-'));
+  // gstack-review-log uses gstack-slug which needs a git repo — create the projects dir
+  // with a predictable slug by pre-creating the directory structure
+  slugDir = path.join(tmpDir, 'projects');
+  fs.mkdirSync(slugDir, { recursive: true });
+});
+
+afterEach(() => {
+  fs.rmSync(tmpDir, { recursive: true, force: true });
+});
+
+describe('gstack-review-log', () => {
+  test('appends valid JSON to review JSONL file', () => {
+    const input = '{"skill":"plan-eng-review","status":"clean"}';
+    const result = run(input);
+    expect(result.exitCode).toBe(0);
+
+    // Find the JSONL file that was written
+    const projectDirs = fs.readdirSync(slugDir);
+    expect(projectDirs.length).toBeGreaterThan(0);
+    const projectDir = path.join(slugDir, projectDirs[0]);
+    const jsonlFiles = fs.readdirSync(projectDir).filter(f => f.endsWith('.jsonl'));
+    expect(jsonlFiles.length).toBeGreaterThan(0);
+
+    const content = fs.readFileSync(path.join(projectDir, jsonlFiles[0]), 'utf-8').trim();
+    const parsed = JSON.parse(content);
+    expect(parsed.skill).toBe('plan-eng-review');
+    expect(parsed.status).toBe('clean');
+  });
+
+  test('rejects non-JSON input with non-zero exit code', () => {
+    const result = run('not json at all', { expectFail: true });
+    expect(result.exitCode).not.toBe(0);
+
+    // Verify nothing was written
+    const projectDirs = fs.readdirSync(slugDir);
+    if (projectDirs.length > 0) {
+      const projectDir = path.join(slugDir, projectDirs[0]);
+      const jsonlFiles = fs.readdirSync(projectDir).filter(f => f.endsWith('.jsonl'));
+      if (jsonlFiles.length > 0) {
+        const content = fs.readFileSync(path.join(projectDir, jsonlFiles[0]), 'utf-8').trim();
+        expect(content).toBe('');
+      }
+    }
+  });
+});

test/telemetry.test.ts

@@ -125,6 +125,82 @@ describe('gstack-telemetry-log', () => {
     expect(events[0]).toHaveProperty('_branch');
   });
 
+  // ─── json_safe() injection prevention tests ────────────────
+  test('sanitizes skill name with quote injection attempt', () => {
+    setConfig('telemetry', 'anonymous');
+    run(`${BIN}/gstack-telemetry-log --skill 'review","injected":"true' --duration 10 --outcome success --session-id inj-1`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    // Must be valid JSON (no injection — quotes stripped, so no field injection possible)
+    const event = JSON.parse(lines[0]);
+    // The key check: no injected top-level property was created
+    expect(event).not.toHaveProperty('injected');
+    // Skill field should have quotes stripped but content preserved
+    expect(event.skill).not.toContain('"');
+  });
+
+  test('truncates skill name exceeding 200 chars', () => {
+    setConfig('telemetry', 'anonymous');
+    const longSkill = 'a'.repeat(250);
+    run(`${BIN}/gstack-telemetry-log --skill '${longSkill}' --duration 10 --outcome success --session-id trunc-1`);
+
+    const events = parseJsonl();
+    expect(events[0].skill.length).toBeLessThanOrEqual(200);
+  });
+
+  test('sanitizes outcome with newline injection attempt', () => {
+    setConfig('telemetry', 'anonymous');
+    // Use printf to pass actual newline in the argument
+    run(`bash -c 'OUTCOME=$(printf "success\\nfake\\":\\"true"); ${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome "$OUTCOME" --session-id inj-2'`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event).not.toHaveProperty('fake');
+  });
+
+  test('sanitizes session_id with backslash-quote injection', () => {
+    setConfig('telemetry', 'anonymous');
+    run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome success --session-id 'id\\\\"","x":"y'`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event).not.toHaveProperty('x');
+  });
+
+  test('sanitizes error_class with quote injection', () => {
+    setConfig('telemetry', 'anonymous');
+    run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-class 'timeout","extra":"val' --session-id inj-3`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event).not.toHaveProperty('extra');
+  });
+
+  test('sanitizes failed_step with quote injection', () => {
+    setConfig('telemetry', 'anonymous');
+    run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --failed-step 'step1","hacked":"yes' --session-id inj-4`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event).not.toHaveProperty('hacked');
+  });
+
+  test('escapes error_message quotes and preserves content', () => {
+    setConfig('telemetry', 'anonymous');
+    run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'Error: file "test.txt" not found' --session-id inj-5`);
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event.error_message).toContain('file');
+    expect(event.error_message).toContain('not found');
+  });
+
   test('creates analytics directory if missing', () => {
     // Remove analytics dir
     const analyticsDir = path.join(tmpDir, 'analytics');