From f9796eef8e4b2053ed210633e72e4dc545baf9fa Mon Sep 17 00:00:00 2001
From: garrytan <garrytan@gmail.com>
Date: Thu, 8 Oct 2026 06:20:39 +0000
Subject: [PATCH] fix(timeline): reject a missing or malformed --limit with
 exit 2 (#1723)

gstack-timeline-read parsed --limit with parseInt and fell back to 20, so
'--limit 1abc' became 1, 'nope', 0 and -5 silently became the default, and a
bare --limit crashed on an unbound $2. Validate the argument as a positive
integer before the no-timeline early exit and exit 2 with an example. No flag
still means 20.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
---
 bin/gstack-timeline-read | 18 ++++++++++-----
 test/timeline.test.ts    | 48 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/bin/gstack-timeline-read b/bin/gstack-timeline-read
index e57d039cb..e1edb9179 100755
--- a/bin/gstack-timeline-read
+++ b/bin/gstack-timeline-read
@@ -5,7 +5,8 @@
 # Session timeline: local-only, never sent anywhere.
 # Reads ~/.gstack/projects/$SLUG/timeline.jsonl, filters, formats.
 # Exit 0 silently if no timeline file exists or nothing matches. Missing bun
-# exits 127 and a failed read exits non-zero with its stderr (#2790).
+# exits 127 and a failed read exits non-zero with its stderr (#2790). A missing
+# or non-positive-integer --limit exits 2 before any read (#1723).
 set -euo pipefail
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
 eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null)"
@@ -20,7 +21,16 @@ BRANCH=""
 while [[ $# -gt 0 ]]; do
   case "$1" in
     --since) SINCE="$2"; shift 2 ;;
-    --limit) LIMIT="$2"; shift 2 ;;
+    --limit)
+      if [[ $# -lt 2 ]]; then
+        echo "gstack-timeline-read: --limit must be a positive integer (got no value); example: --limit 20" >&2
+        exit 2
+      fi
+      if [[ ! "$2" =~ ^[1-9][0-9]*$ ]]; then
+        echo "gstack-timeline-read: --limit must be a positive integer (got '$2'); example: --limit 20" >&2
+        exit 2
+      fi
+      LIMIT="$2"; shift 2 ;;
     --branch) BRANCH="$2"; shift 2 ;;
     *) shift ;;
   esac
@@ -41,9 +51,7 @@ cat "$TIMELINE_FILE" | GSTACK_TIMELINE_SINCE="$SINCE" GSTACK_TIMELINE_BRANCH="$B
 const lines = (await Bun.stdin.text()).trim().split('\n').filter(Boolean);
 const since = process.env.GSTACK_TIMELINE_SINCE || '';
 const branch = process.env.GSTACK_TIMELINE_BRANCH || '';
-const limitRaw = process.env.GSTACK_TIMELINE_LIMIT || '20';
-const parsedLimit = Number.parseInt(limitRaw, 10);
-const limit = Number.isSafeInteger(parsedLimit) && parsedLimit > 0 ? parsedLimit : 20;
+const limit = Number(process.env.GSTACK_TIMELINE_LIMIT);
 
 let sinceMs = 0;
 if (since) {
diff --git a/test/timeline.test.ts b/test/timeline.test.ts
index a7cfa3f13..eb4303cc4 100644
--- a/test/timeline.test.ts
+++ b/test/timeline.test.ts
@@ -1,5 +1,5 @@
 import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
-import { execFileSync, execSync, ExecSyncOptionsWithStringEncoding } from 'child_process';
+import { execFileSync, execSync, spawnSync, ExecSyncOptionsWithStringEncoding } from 'child_process';
 import * as fs from 'fs';
 import * as path from 'path';
 import * as os from 'os';
@@ -56,6 +56,16 @@ function runReadArgs(args: string[] = []): string {
   }
 }
 
+function runReadStatus(args: string[]): { status: number | null; stdout: string; stderr: string } {
+  const result = spawnSync(path.join(BIN, 'gstack-timeline-read'), args, {
+    cwd: ROOT,
+    env: { ...process.env, GSTACK_HOME: tmpDir },
+    encoding: 'utf-8',
+    timeout: 15000,
+  });
+  return { status: result.status, stdout: result.stdout, stderr: result.stderr };
+}
+
 beforeEach(() => {
   tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-timeline-'));
   slugDir = path.join(tmpDir, 'projects');
@@ -176,4 +186,40 @@ describe('gstack-timeline-read', () => {
     expect(unlimitedEvents).toBe(5);
     expect(limitedEvents).toBe(2);
   });
+
+  test('no --limit still shows the 20 most recent events', () => {
+    for (let i = 0; i < 25; i++) {
+      runLog(JSON.stringify({ skill: 'review', event: 'completed', branch: 'main', ts: `2026-03-01T10:${String(i).padStart(2, '0')}:00Z` }));
+    }
+    const result = runReadStatus([]);
+    expect(result.status).toBe(0);
+    const events = result.stdout.split('\n').filter(l => l.startsWith('- '));
+    expect(events.length).toBe(20);
+    expect(events[0]).toContain('10:05:00Z');
+  });
+
+  // #1723: a malformed --limit used to be parsed leniently (1abc -> 1) or
+  // silently replaced by the default; a bare --limit crashed on unbound $2.
+  const badLimits: Array<[string, string[], string]> = [
+    ['missing value', ['--limit'], 'got no value'],
+    ['zero', ['--limit', '0'], "got '0'"],
+    ['negative', ['--limit', '-5'], "got '-5'"],
+    ['non-numeric', ['--limit', 'abc'], "got 'abc'"],
+    ['trailing garbage', ['--limit', '1abc'], "got '1abc'"],
+  ];
+  for (const history of ['empty', 'populated'] as const) {
+    for (const [label, args, got] of badLimits) {
+      test(`--limit ${label} exits 2 with an example on ${history} history`, () => {
+        if (history === 'populated') {
+          runLog(JSON.stringify({ skill: 'review', event: 'completed', branch: 'main', ts: '2026-03-28T10:00:00Z' }));
+        }
+        const result = runReadStatus(args);
+        expect(result.status).toBe(2);
+        expect(result.stdout).toBe('');
+        expect(result.stderr).toContain('--limit must be a positive integer');
+        expect(result.stderr).toContain(got);
+        expect(result.stderr).toContain('example: --limit 20');
+      });
+    }
+  }
 });