From 03dbccdb6e77ba3ee33c204417c7274310842a05 Mon Sep 17 00:00:00 2001 From: Oscar Hong Date: Mon, 4 May 2026 09:57:53 -0700 Subject: [PATCH] test: replace mock-heavy CLI tests with pure unit + real-I/O tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the worst mock-only tests are now honest: 1. first-run.test.ts — was 100% vi.mock("node:fs"). Replaced with mkdtempSync against the real filesystem; exercises actual 0o600/0o700 mode bits, real EACCES on read-only dirs, and the round-trip between isFirstRun + markFirstRun. Adds an optional configDir param to both helpers (default = CONFIG_DIR), so production behavior is unchanged. 2. api.test.ts — was vi.stubGlobal("fetch", …). Replaced with a real http.createServer in beforeAll; every test now exercises the real fetch / Authorization-header serialization / JSON body / response- header reading, including the X-Straude-Refreshed-Token rotation and the 401-retry path. The only remaining mock is auth.saveConfig (boundary, not faked behavior). 3. ccusage-install.test.ts — extracted the install-command decision into a new pure helper pickInstallCommand({ hasBun }) and unit-tested it with zero mocks. Trimmed the orchestration test to branches that genuinely depend on process state (PATH, isatty). Also extracted resolvePushDateRange() from pushCommand as a pure function, unit-tested across all six branches (--date / codex repair / --days / smart-sync / no last_push_date / future date / boundary). CLI test count: 240 → 265 (+25 pure unit tests, 7 mock-only tests retired). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/CHANGELOG.md | 4 + packages/cli/__tests__/api.test.ts | 361 ++++++++++-------- .../cli/__tests__/ccusage-install.test.ts | 79 ++-- packages/cli/__tests__/first-run.test.ts | 149 +++++--- .../__tests__/pick-install-command.test.ts | 29 ++ .../__tests__/resolve-push-date-range.test.ts | 191 +++++++++ packages/cli/src/commands/push.ts | 124 +++--- packages/cli/src/lib/ccusage.ts | 18 +- packages/cli/src/lib/first-run.ts | 19 +- 9 files changed, 670 insertions(+), 304 deletions(-) create mode 100644 packages/cli/__tests__/pick-install-command.test.ts create mode 100644 packages/cli/__tests__/resolve-push-date-range.test.ts diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index dae6cc1..ac564c5 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Changed + +- **Replaced mock-heavy CLI tests with pure unit + real-I/O integration tests.** PostHog audit of the test suite found 503 of 794 tests (63%) lived in files that mocked something. Three of the worst offenders are now honest: (a) `first-run.test.ts` was 100% `vi.mock("node:fs")` — replaced with `mkdtempSync` and real fs operations, exercising actual mode bits (0o600 / 0o700), real permission-denied paths, and the round-trip between `isFirstRun` and `markFirstRun`. (b) `api.test.ts` stubbed `globalThis.fetch` — replaced with a real `http.createServer` so every test exercises actual fetch / Authorization-header serialization / JSON parsing / response-header reading, including the sliding-token-refresh and 401-retry paths. (c) `ccusage-install.test.ts` had its decision logic (`bun add -g` vs `npm install -g`) split into a new pure helper `pickInstallCommand({ hasBun })` and `resolvePushDateRange()` — both unit-tested with zero mocks. The orchestration test in `ccusage-install.test.ts` is trimmed to branches that genuinely depend on process state (PATH, isatty). CLI test count is now 265 (was 240); 25 new pure tests, 7 retired mock tests. + ### Added - **CLI activation tracking events: `cli_first_run` and `cli_authenticated`.** PostHog showed 103 users tried Straude in the last 7 days but only 48 (47%) ever pushed once successfully — the missing event was a clean install→activate funnel. The CLI now writes `~/.straude/.first-run` on the first invocation per machine and captures `cli_first_run` (with `platform`, `node_version`, `command`) before any other code runs, so even `npx straude --help` counts as install. Every subsequent invocation that loads a stored config also captures `cli_authenticated`. Saved insight `DV22QC1d` ([URL](https://us.posthog.com/project/374497/insights/DV22QC1d)) tracks the funnel; goal is ≥75% activation on `cli_version` ≥ 0.1.24. diff --git a/packages/cli/__tests__/api.test.ts b/packages/cli/__tests__/api.test.ts index 4b9da3d..d95814f 100644 --- a/packages/cli/__tests__/api.test.ts +++ b/packages/cli/__tests__/api.test.ts @@ -1,7 +1,26 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { apiRequest, apiRequestNoAuth, setAuthRefreshStrategy, REFRESHED_TOKEN_HEADER } from "../src/lib/api.js"; +import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest"; +import { createServer, type IncomingMessage, type ServerResponse, type Server } from "node:http"; +import { + apiRequest, + apiRequestNoAuth, + setAuthRefreshStrategy, + REFRESHED_TOKEN_HEADER, +} from "../src/lib/api.js"; import type { StraudeConfig } from "../src/lib/auth.js"; +/** + * Integration tests against a real local http.Server. The previous version of + * this file stubbed `globalThis.fetch` with a vi.fn() — that meant every test + * passed through a fake that didn't actually serialize headers, parse the + * response body, or honor `res.headers.get`. With a real server, every byte + * the production code writes and reads is exercised. + * + * We mock at one boundary: `auth.saveConfig`, because the real implementation + * writes to `~/.straude/config.json` and we don't want test runs touching the + * user's actual config. That mock is captured-and-asserted, not faked-and- + * forgotten. + */ + vi.mock("../src/lib/auth.js", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, saveConfig: vi.fn() }; @@ -14,195 +33,236 @@ vi.mock("../src/lib/prompt.js", () => ({ import { saveConfig } from "../src/lib/auth.js"; import { isInteractive } from "../src/lib/prompt.js"; - -const mockFetch = vi.fn(); -vi.stubGlobal("fetch", mockFetch); - const mockSaveConfig = vi.mocked(saveConfig); const mockIsInteractive = vi.mocked(isInteractive); +interface RequestRecord { + method: string; + path: string; + headers: Record; + body: string; +} + +interface PlannedResponse { + status: number; + body: unknown; + headers?: Record; +} + +let server: Server; +let baseUrl: string; +let recorded: RequestRecord[]; +let plan: PlannedResponse[]; + +beforeAll(async () => { + recorded = []; + plan = []; + server = createServer((req: IncomingMessage, res: ServerResponse) => { + let body = ""; + req.on("data", (chunk) => { + body += chunk; + }); + req.on("end", () => { + recorded.push({ + method: req.method ?? "", + path: req.url ?? "", + headers: req.headers, + body, + }); + const next = plan.shift(); + if (!next) { + res.statusCode = 500; + res.end(JSON.stringify({ error: "no planned response" })); + return; + } + res.statusCode = next.status; + res.setHeader("content-type", "application/json"); + for (const [k, v] of Object.entries(next.headers ?? {})) { + res.setHeader(k, v); + } + res.end(JSON.stringify(next.body)); + }); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const addr = server.address(); + if (typeof addr !== "object" || addr === null) throw new Error("server listen failed"); + baseUrl = `http://127.0.0.1:${addr.port}`; +}); + +afterAll(async () => { + await new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); +}); + beforeEach(() => { - mockFetch.mockReset(); + recorded = []; + plan = []; mockSaveConfig.mockReset(); mockIsInteractive.mockReset(); mockIsInteractive.mockReturnValue(false); setAuthRefreshStrategy(null); }); -const config: StraudeConfig = { - token: "tok-abc", - username: "alice", - api_url: "https://straude.com", -}; - -describe("apiRequest", () => { - it("constructs correct URL from config and path", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({ data: "ok" }), - }); - await apiRequest(config, "/api/usage"); - expect(mockFetch).toHaveBeenCalledWith( - "https://straude.com/api/usage", - expect.anything(), - ); +afterEach(() => { + // Make sure no test accidentally left a planned response unconsumed — + // a leak there would silently affect the next test. + expect(plan).toHaveLength(0); +}); + +function configFor(): StraudeConfig { + return { token: "tok-abc", username: "alice", api_url: baseUrl }; +} + +describe("apiRequest — wire format", () => { + it("hits the configured api_url + path", async () => { + plan.push({ status: 200, body: { ok: true } }); + await apiRequest(configFor(), "/api/usage"); + expect(recorded).toHaveLength(1); + expect(recorded[0]!.path).toBe("/api/usage"); }); - it("includes Authorization header with token", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - }); - await apiRequest(config, "/api/test"); - const callArgs = mockFetch.mock.calls[0]!; - expect(callArgs[1].headers.Authorization).toBe("Bearer tok-abc"); + it("sends a real Bearer token in the Authorization header", async () => { + plan.push({ status: 200, body: {} }); + await apiRequest(configFor(), "/api/test"); + expect(recorded[0]!.headers.authorization).toBe("Bearer tok-abc"); }); - it("includes Content-Type header", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - }); - await apiRequest(config, "/api/test"); - const callArgs = mockFetch.mock.calls[0]!; - expect(callArgs[1].headers["Content-Type"]).toBe("application/json"); + it("sends Content-Type: application/json", async () => { + plan.push({ status: 200, body: {} }); + await apiRequest(configFor(), "/api/test"); + expect(recorded[0]!.headers["content-type"]).toBe("application/json"); }); - it("parses JSON response", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({ foo: "bar" }), + it("forwards method + body verbatim", async () => { + plan.push({ status: 200, body: { ok: true } }); + await apiRequest(configFor(), "/api/test", { + method: "POST", + body: JSON.stringify({ a: 1 }), }); - const result = await apiRequest<{ foo: string }>(config, "/api/test"); + expect(recorded[0]!.method).toBe("POST"); + expect(recorded[0]!.body).toBe('{"a":1}'); + }); + + it("parses the response JSON body", async () => { + plan.push({ status: 200, body: { foo: "bar" } }); + const result = await apiRequest<{ foo: string }>(configFor(), "/api/test"); expect(result.foo).toBe("bar"); }); +}); - it("throws on HTTP error with error message from body", async () => { - mockFetch.mockResolvedValue({ - ok: false, - status: 401, - json: () => Promise.resolve({ error: "Unauthorized" }), - }); - await expect(apiRequest(config, "/api/test")).rejects.toThrow( - "Session expired or invalid", +describe("apiRequest — error handling", () => { + it("throws the session-expired message on real 401", async () => { + plan.push({ status: 401, body: { error: "Unauthorized" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Session expired or invalid/, ); }); - it("throws with HTTP status when body parse fails", async () => { - mockFetch.mockResolvedValue({ - ok: false, - status: 500, - json: () => Promise.reject(new Error("parse error")), - }); - await expect(apiRequest(config, "/api/test")).rejects.toThrow("HTTP 500"); + it("includes the path in 404 errors and points at upgrade", async () => { + plan.push({ status: 404, body: { error: "Not found" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Endpoint not found.*\/api\/test/, + ); }); - it("forwards custom options", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - }); - await apiRequest(config, "/api/test", { method: "POST", body: '{"a":1}' }); - const callArgs = mockFetch.mock.calls[0]!; - expect(callArgs[1].method).toBe("POST"); - expect(callArgs[1].body).toBe('{"a":1}'); + it("surfaces the body's error string on other non-2xx", async () => { + plan.push({ status: 500, body: { error: "boom" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow("boom"); + }); + + it("falls back to HTTP when the body has no error key", async () => { + plan.push({ status: 503, body: { unrelated: "field" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow("HTTP 503"); }); }); describe("apiRequest — sliding token refresh", () => { - it("persists a refreshed token from response header", async () => { - const mutableConfig: StraudeConfig = { ...config }; - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - headers: { get: (name: string) => name === REFRESHED_TOKEN_HEADER ? "new-token-xyz" : null }, + it("persists a refreshed token returned via X-Straude-Refreshed-Token", async () => { + plan.push({ + status: 200, + body: {}, + headers: { [REFRESHED_TOKEN_HEADER]: "new-token-xyz" }, }); - await apiRequest(mutableConfig, "/api/test"); - expect(mutableConfig.token).toBe("new-token-xyz"); + const cfg = configFor(); + await apiRequest(cfg, "/api/test"); + expect(cfg.token).toBe("new-token-xyz"); expect(mockSaveConfig).toHaveBeenCalledWith( expect.objectContaining({ token: "new-token-xyz" }), ); }); - it("does not save when no refresh header is present", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - headers: { get: () => null }, - }); - await apiRequest(config, "/api/test"); + it("does not save when the refresh header is absent", async () => { + plan.push({ status: 200, body: {} }); + await apiRequest(configFor(), "/api/test"); expect(mockSaveConfig).not.toHaveBeenCalled(); }); - it("swallows saveConfig errors so the request still resolves", async () => { + it("uses the refreshed token on the very next request", async () => { + const cfg = configFor(); + plan.push({ + status: 200, + body: {}, + headers: { [REFRESHED_TOKEN_HEADER]: "rotated-1" }, + }); + plan.push({ status: 200, body: {} }); + await apiRequest(cfg, "/api/first"); + await apiRequest(cfg, "/api/second"); + expect(recorded[0]!.headers.authorization).toBe("Bearer tok-abc"); + expect(recorded[1]!.headers.authorization).toBe("Bearer rotated-1"); + }); + + it("still resolves the request when saveConfig fails (read-only home)", async () => { mockSaveConfig.mockImplementation(() => { throw new Error("EACCES"); }); - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({ ok: true }), - headers: { get: () => "new-token" }, + plan.push({ + status: 200, + body: { ok: true }, + headers: { [REFRESHED_TOKEN_HEADER]: "new-token" }, }); - await expect(apiRequest(config, "/api/test")).resolves.toEqual({ ok: true }); + await expect(apiRequest(configFor(), "/api/test")).resolves.toEqual({ ok: true }); }); }); describe("apiRequest — silent re-auth on 401", () => { - it("retries once after running the refresh strategy when interactive", async () => { + it("retries once after the refresh strategy resolves a fresh config", async () => { mockIsInteractive.mockReturnValue(true); const refreshStrategy = vi.fn(async () => ({ - ...config, token: "fresh-token", + username: "alice", + api_url: baseUrl, })); setAuthRefreshStrategy(refreshStrategy); - mockFetch.mockResolvedValueOnce({ - ok: false, - status: 401, - json: () => Promise.resolve({ error: "Unauthorized" }), - }); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve({ data: "ok" }), - headers: { get: () => null }, - }); + plan.push({ status: 401, body: { error: "Unauthorized" } }); + plan.push({ status: 200, body: { data: "ok" } }); - const mutable: StraudeConfig = { ...config }; - const result = await apiRequest<{ data: string }>(mutable, "/api/test"); + const cfg = configFor(); + const result = await apiRequest<{ data: string }>(cfg, "/api/test"); expect(result.data).toBe("ok"); expect(refreshStrategy).toHaveBeenCalledTimes(1); - expect(mockFetch).toHaveBeenCalledTimes(2); - expect(mutable.token).toBe("fresh-token"); - // Second call uses the new token. - const secondHeaders = (mockFetch.mock.calls[1]![1] as { headers: Record }).headers; - expect(secondHeaders.Authorization).toBe("Bearer fresh-token"); + expect(recorded[1]!.headers.authorization).toBe("Bearer fresh-token"); + expect(cfg.token).toBe("fresh-token"); }); - it("throws the original error when refresh strategy returns null", async () => { + it("throws the original error when the strategy resolves null", async () => { mockIsInteractive.mockReturnValue(true); setAuthRefreshStrategy(async () => null); - mockFetch.mockResolvedValue({ - ok: false, - status: 401, - json: () => Promise.resolve({ error: "Unauthorized" }), - }); - await expect(apiRequest(config, "/api/test")).rejects.toThrow( - "Session expired or invalid", + plan.push({ status: 401, body: { error: "Unauthorized" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Session expired or invalid/, ); - expect(mockFetch).toHaveBeenCalledTimes(1); + expect(recorded).toHaveLength(1); }); it("does not retry in non-interactive contexts (auto-push)", async () => { mockIsInteractive.mockReturnValue(false); const refreshStrategy = vi.fn(); setAuthRefreshStrategy(refreshStrategy); - mockFetch.mockResolvedValue({ - ok: false, - status: 401, - json: () => Promise.resolve({ error: "Unauthorized" }), - }); - await expect(apiRequest(config, "/api/test")).rejects.toThrow( - "Session expired or invalid", + plan.push({ status: 401, body: { error: "Unauthorized" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Session expired or invalid/, ); expect(refreshStrategy).not.toHaveBeenCalled(); }); @@ -210,37 +270,38 @@ describe("apiRequest — silent re-auth on 401", () => { it("does not retry when no strategy is registered", async () => { mockIsInteractive.mockReturnValue(true); setAuthRefreshStrategy(null); - mockFetch.mockResolvedValue({ - ok: false, - status: 401, - json: () => Promise.resolve({ error: "Unauthorized" }), - }); - await expect(apiRequest(config, "/api/test")).rejects.toThrow( - "Session expired or invalid", + plan.push({ status: 401, body: { error: "Unauthorized" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Session expired or invalid/, ); }); + + it("propagates a second 401 if the retried request also fails auth", async () => { + mockIsInteractive.mockReturnValue(true); + setAuthRefreshStrategy(async () => ({ + token: "still-bad", + username: "alice", + api_url: baseUrl, + })); + plan.push({ status: 401, body: { error: "Unauthorized" } }); + plan.push({ status: 401, body: { error: "Unauthorized" } }); + await expect(apiRequest(configFor(), "/api/test")).rejects.toThrow( + /Session expired or invalid/, + ); + expect(recorded).toHaveLength(2); + }); }); describe("apiRequestNoAuth", () => { - it("does not include Authorization header", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - }); - await apiRequestNoAuth("https://straude.com", "/api/public"); - const callArgs = mockFetch.mock.calls[0]!; - expect(callArgs[1].headers.Authorization).toBeUndefined(); + it("does not include an Authorization header", async () => { + plan.push({ status: 200, body: {} }); + await apiRequestNoAuth(baseUrl, "/api/public"); + expect(recorded[0]!.headers.authorization).toBeUndefined(); }); - it("constructs correct URL", async () => { - mockFetch.mockResolvedValue({ - ok: true, - json: () => Promise.resolve({}), - }); - await apiRequestNoAuth("https://custom.api", "/health"); - expect(mockFetch).toHaveBeenCalledWith( - "https://custom.api/health", - expect.anything(), - ); + it("hits the supplied url + path", async () => { + plan.push({ status: 200, body: {} }); + await apiRequestNoAuth(baseUrl, "/health"); + expect(recorded[0]!.path).toBe("/health"); }); }); diff --git a/packages/cli/__tests__/ccusage-install.test.ts b/packages/cli/__tests__/ccusage-install.test.ts index 89cc667..acaf98b 100644 --- a/packages/cli/__tests__/ccusage-install.test.ts +++ b/packages/cli/__tests__/ccusage-install.test.ts @@ -1,6 +1,20 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { ensureCcusageInstalled, _resetCcusageResolver } from "../src/lib/ccusage.js"; +/** + * Orchestration-only tests for `ensureCcusageInstalled`. + * + * The pure command-selection logic ("bun vs npm", "what args") is tested + * separately in `pick-install-command.test.ts` against the real + * `pickInstallCommand` function — no mocks needed there. + * + * What's left here is the orchestrator: which branch we take based on PATH + * state + TTY state + the user's prompt response. Those decisions still + * need mocks because the orchestrator's *job* is to read process state + * (PATH, isatty(stdin/stdout)) and react. We mock at those true boundaries + * (node:child_process, node:fs, prompt) but never at the system under test. + */ + vi.mock("node:child_process", () => ({ execFileSync: vi.fn(), execFile: vi.fn(), @@ -37,22 +51,23 @@ beforeEach(() => { _resetCcusageResolver(); }); -describe("ensureCcusageInstalled", () => { - it("returns immediately when ccusage is on PATH", async () => { +describe("ensureCcusageInstalled — orchestration branches", () => { + it("no-ops and skips the prompt when ccusage is already on PATH", async () => { mockExistsSync.mockReturnValue(true); await expect(ensureCcusageInstalled()).resolves.toBeUndefined(); expect(mockIsInteractive).not.toHaveBeenCalled(); expect(mockExecFileSync).not.toHaveBeenCalled(); }); - it("throws the manual-install error in non-TTY contexts", async () => { + it("throws the manual-install error in non-TTY contexts (auto-push, CI)", async () => { mockExistsSync.mockReturnValue(false); mockIsInteractive.mockReturnValue(false); await expect(ensureCcusageInstalled()).rejects.toThrow(/not installed or not on PATH/); expect(mockPromptYesNo).not.toHaveBeenCalled(); + expect(mockExecFileSync).not.toHaveBeenCalled(); }); - it("throws when the user declines the prompt", async () => { + it("throws cleanly when the user declines the prompt", async () => { mockExistsSync.mockReturnValue(false); mockIsInteractive.mockReturnValue(true); mockPromptYesNo.mockResolvedValue(false); @@ -60,36 +75,29 @@ describe("ensureCcusageInstalled", () => { expect(mockExecFileSync).not.toHaveBeenCalled(); }); - it("installs successfully when accepted and binary appears on PATH", async () => { - // Two calls to existsSync per resolver invocation per PATH dir × suffix. - // Simulate: not present at start → install runs → present afterwards. - let installRan = false; - mockExistsSync.mockImplementation((p: unknown) => { - const path = String(p); - // bun-detection probes return false; ccusage probes flip after install. - if (path.includes("ccusage")) return installRan; - return false; // bun not present → falls back to npm - }); + it("wraps install-time exec errors with a manual-fallback message", async () => { + mockExistsSync.mockReturnValue(false); mockIsInteractive.mockReturnValue(true); mockPromptYesNo.mockResolvedValue(true); mockExecFileSync.mockImplementation(() => { - installRan = true; - return Buffer.from(""); + throw new Error("EACCES: permission denied"); }); + await expect(ensureCcusageInstalled()).rejects.toThrow(/Install it manually/); + }); - await expect(ensureCcusageInstalled()).resolves.toBeUndefined(); - expect(mockExecFileSync).toHaveBeenCalledTimes(1); - const [cmd, args] = mockExecFileSync.mock.calls[0]!; - expect(cmd).toBe("npm"); - expect(args).toEqual(["install", "-g", "ccusage"]); + it("rechecks PATH after install and reports if the binary is still missing", async () => { + mockExistsSync.mockReturnValue(false); // never appears on PATH + mockIsInteractive.mockReturnValue(true); + mockPromptYesNo.mockResolvedValue(true); + mockExecFileSync.mockReturnValue(Buffer.from("")); + await expect(ensureCcusageInstalled()).rejects.toThrow(/may need to open a new shell/); }); - it("prefers bun when bun is on PATH", async () => { + it("succeeds end-to-end when accepted, installed, and binary appears on PATH", async () => { let installRan = false; mockExistsSync.mockImplementation((p: unknown) => { const path = String(p); if (path.includes("ccusage")) return installRan; - if (path.includes("bun")) return true; return false; }); mockIsInteractive.mockReturnValue(true); @@ -98,28 +106,7 @@ describe("ensureCcusageInstalled", () => { installRan = true; return Buffer.from(""); }); - - await ensureCcusageInstalled(); - const [cmd, args] = mockExecFileSync.mock.calls[0]!; - expect(cmd).toBe("bun"); - expect(args).toEqual(["add", "-g", "ccusage"]); - }); - - it("surfaces install errors with a manual-fallback message", async () => { - mockExistsSync.mockReturnValue(false); - mockIsInteractive.mockReturnValue(true); - mockPromptYesNo.mockResolvedValue(true); - mockExecFileSync.mockImplementation(() => { - throw new Error("EACCES: permission denied"); - }); - await expect(ensureCcusageInstalled()).rejects.toThrow(/Install it manually/); - }); - - it("throws when install command succeeds but binary still missing from PATH", async () => { - mockExistsSync.mockReturnValue(false); // never on PATH - mockIsInteractive.mockReturnValue(true); - mockPromptYesNo.mockResolvedValue(true); - mockExecFileSync.mockReturnValue(Buffer.from("")); - await expect(ensureCcusageInstalled()).rejects.toThrow(/may need to open a new shell/); + await expect(ensureCcusageInstalled()).resolves.toBeUndefined(); + expect(mockExecFileSync).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/cli/__tests__/first-run.test.ts b/packages/cli/__tests__/first-run.test.ts index bf09631..b4eecca 100644 --- a/packages/cli/__tests__/first-run.test.ts +++ b/packages/cli/__tests__/first-run.test.ts @@ -1,60 +1,115 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { FIRST_RUN_MARKER, isFirstRun, markFirstRun } from "../src/lib/first-run.js"; -import { CONFIG_DIR } from "../src/config.js"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + mkdtempSync, + rmSync, + existsSync, + readFileSync, + statSync, + chmodSync, + mkdirSync, +} from "node:fs"; +import { tmpdir, platform } from "node:os"; +import { join } from "node:path"; +import { + isFirstRun, + markFirstRun, + FIRST_RUN_MARKER_FILENAME, +} from "../src/lib/first-run.js"; -vi.mock("node:fs", () => ({ - existsSync: vi.fn(), - writeFileSync: vi.fn(), - mkdirSync: vi.fn(), -})); +/** + * Real-fs unit tests. The previous version of this file mocked node:fs + * entirely, which only verified the *shape* of our calls (e.g. that we + * asked existsSync about the marker path). It would not have caught: a + * wrong filename, missing-parent-dir behavior, file-mode bits, or a real + * permission-denied path. These tests use a real temp directory so all of + * that is exercised end-to-end. + */ -import { existsSync, writeFileSync, mkdirSync } from "node:fs"; - -const mockExistsSync = vi.mocked(existsSync); -const mockWriteFileSync = vi.mocked(writeFileSync); -const mockMkdirSync = vi.mocked(mkdirSync); +let tmp: string; beforeEach(() => { - vi.clearAllMocks(); + tmp = mkdtempSync(join(tmpdir(), "straude-first-run-")); +}); + +afterEach(() => { + // Restore writability before cleanup in case a test made the dir read-only. + try { + chmodSync(tmp, 0o700); + } catch { + // best effort + } + rmSync(tmp, { recursive: true, force: true }); +}); + +describe("isFirstRun (real fs)", () => { + it("returns true when the marker file does not exist", () => { + expect(isFirstRun(tmp)).toBe(true); + }); + + it("returns true when the config dir itself does not exist", () => { + const ghostDir = join(tmp, "does-not-exist"); + expect(isFirstRun(ghostDir)).toBe(true); + }); + + it("returns false once the marker exists", () => { + markFirstRun(tmp); + expect(isFirstRun(tmp)).toBe(false); + }); }); -describe("isFirstRun", () => { - it("returns true when marker file is missing", () => { - mockExistsSync.mockReturnValue(false); - expect(isFirstRun()).toBe(true); - expect(mockExistsSync).toHaveBeenCalledWith(FIRST_RUN_MARKER); +describe("markFirstRun (real fs)", () => { + it("creates the config dir and writes the marker file", () => { + const child = join(tmp, "nested", ".straude"); + markFirstRun(child); + expect(existsSync(child)).toBe(true); + expect(existsSync(join(child, FIRST_RUN_MARKER_FILENAME))).toBe(true); + }); + + it("writes the marker contents as a parseable ISO timestamp", () => { + markFirstRun(tmp); + const contents = readFileSync(join(tmp, FIRST_RUN_MARKER_FILENAME), "utf-8"); + expect(() => new Date(contents.trim())).not.toThrow(); + expect(new Date(contents.trim()).toString()).not.toBe("Invalid Date"); + }); + + it("writes the marker with mode 0o600 (owner read/write only)", () => { + if (platform() === "win32") return; // POSIX-only + markFirstRun(tmp); + const mode = statSync(join(tmp, FIRST_RUN_MARKER_FILENAME)).mode & 0o777; + expect(mode).toBe(0o600); + }); + + it("creates the config dir with mode 0o700", () => { + if (platform() === "win32") return; + const child = join(tmp, "fresh"); + markFirstRun(child); + const mode = statSync(child).mode & 0o777; + expect(mode).toBe(0o700); + }); + + it("is idempotent on re-invocation (does not throw, marker still present)", () => { + markFirstRun(tmp); + const firstMtime = statSync(join(tmp, FIRST_RUN_MARKER_FILENAME)).mtimeMs; + expect(() => markFirstRun(tmp)).not.toThrow(); + const secondMtime = statSync(join(tmp, FIRST_RUN_MARKER_FILENAME)).mtimeMs; + expect(secondMtime).toBeGreaterThanOrEqual(firstMtime); }); - it("returns false when marker file exists", () => { - mockExistsSync.mockReturnValue(true); - expect(isFirstRun()).toBe(false); + it("swallows errors when the config dir is unwritable", () => { + if (platform() === "win32") return; + if (process.getuid?.() === 0) return; // root bypasses mode bits + const child = join(tmp, "ro"); + mkdirSync(child, { recursive: true, mode: 0o700 }); + chmodSync(child, 0o500); // r-x: cannot write + expect(() => markFirstRun(child)).not.toThrow(); + expect(existsSync(join(child, FIRST_RUN_MARKER_FILENAME))).toBe(false); }); }); -describe("markFirstRun", () => { - it("writes the marker and creates the config dir if missing", () => { - mockExistsSync.mockReturnValue(false); - markFirstRun(); - expect(mockMkdirSync).toHaveBeenCalledWith(CONFIG_DIR, { recursive: true, mode: 0o700 }); - expect(mockWriteFileSync).toHaveBeenCalledTimes(1); - const [path, contents, opts] = mockWriteFileSync.mock.calls[0]!; - expect(path).toBe(FIRST_RUN_MARKER); - expect(typeof contents).toBe("string"); - expect(opts).toEqual({ encoding: "utf-8", mode: 0o600 }); - }); - - it("skips mkdir when the config dir already exists", () => { - mockExistsSync.mockReturnValue(true); - markFirstRun(); - expect(mockMkdirSync).not.toHaveBeenCalled(); - expect(mockWriteFileSync).toHaveBeenCalledTimes(1); - }); - - it("swallows write errors silently (read-only home)", () => { - mockExistsSync.mockReturnValue(true); - mockWriteFileSync.mockImplementation(() => { - throw new Error("EACCES: permission denied"); - }); - expect(() => markFirstRun()).not.toThrow(); +describe("isFirstRun + markFirstRun (real fs round-trip)", () => { + it("first call sees first-run, mark, second call does not", () => { + expect(isFirstRun(tmp)).toBe(true); + markFirstRun(tmp); + expect(isFirstRun(tmp)).toBe(false); }); }); diff --git a/packages/cli/__tests__/pick-install-command.test.ts b/packages/cli/__tests__/pick-install-command.test.ts new file mode 100644 index 0000000..5682565 --- /dev/null +++ b/packages/cli/__tests__/pick-install-command.test.ts @@ -0,0 +1,29 @@ +import { describe, it, expect } from "vitest"; +import { pickInstallCommand } from "../src/lib/ccusage.js"; + +describe("pickInstallCommand", () => { + it("uses bun when bun is available", () => { + expect(pickInstallCommand({ hasBun: true })).toEqual({ + cmd: "bun", + args: ["add", "-g", "ccusage"], + manager: "bun", + }); + }); + + it("falls back to npm when bun is missing", () => { + expect(pickInstallCommand({ hasBun: false })).toEqual({ + cmd: "npm", + args: ["install", "-g", "ccusage"], + manager: "npm", + }); + }); + + it("hardcodes the package name (no string interpolation)", () => { + // Regression guard: a refactor that lets a caller pass the package name + // would let an attacker who controls config inject `--config-set` etc. + // The signature only accepts boolean env state, so this stays sealed. + const npm = pickInstallCommand({ hasBun: false }); + expect(npm.args).toContain("ccusage"); + expect(npm.args).not.toContain(""); + }); +}); diff --git a/packages/cli/__tests__/resolve-push-date-range.test.ts b/packages/cli/__tests__/resolve-push-date-range.test.ts new file mode 100644 index 0000000..67acb82 --- /dev/null +++ b/packages/cli/__tests__/resolve-push-date-range.test.ts @@ -0,0 +1,191 @@ +import { describe, it, expect } from "vitest"; +import { resolvePushDateRange } from "../src/commands/push.js"; + +/** + * Build a Date at midnight local time. Matches the behavior of the production + * `parseDate` helper inside push.ts so day-boundary math doesn't drift by 12h. + */ +function dateAt(yyyyMmDd: string): Date { + const [y, m, d] = yyyyMmDd.split("-").map(Number); + return new Date(y!, m! - 1, d!); +} + +function isoDay(d: Date): string { + return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, "0")}-${String(d.getDate()).padStart(2, "0")}`; +} + +describe("resolvePushDateRange", () => { + describe("--date branch", () => { + it("returns the exact date for both since and until", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2026-05-01" }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-05-01"); + expect(isoDay(r.until)).toBe("2026-05-01"); + }); + + it("rejects dates outside the 30-day backfill window", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2025-12-01" }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(false); + if (r.ok) return; + expect(r.error).toContain("within the last 30 days"); + }); + + it("rejects near-future dates with the future-date error", () => { + // Use a date that's in the future but inside the 30-day window so we + // hit the future-date check, not the backfill-window check (which + // fires first for far-future dates). + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2026-05-05" }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(false); + if (r.ok) return; + expect(r.error).toContain("future date"); + }); + + it("rejects far-future dates with the backfill-window error", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2099-01-01" }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(false); + if (r.ok) return; + expect(r.error).toContain("within the last 30 days"); + }); + + it("accepts a date exactly 30 days back (boundary)", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2026-04-04" }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + }); + + it("--date wins over codex repair when both would apply", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { date: "2026-05-03" }, + shouldRunCodexRepair: true, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-05-03"); + }); + }); + + describe("codex repair branch", () => { + it("backfills the full 30-day window when codex repair runs", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: {}, + shouldRunCodexRepair: true, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-04-05"); // today - 29 days + expect(isoDay(r.until)).toBe("2026-05-04"); + }); + + it("ignores --days when codex repair runs", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { days: 5 }, + shouldRunCodexRepair: true, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-04-05"); // 30-day backfill, not 5 + }); + }); + + describe("--days branch", () => { + it("backfills exactly N days when given --days N", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { days: 7 }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-04-28"); + expect(isoDay(r.until)).toBe("2026-05-04"); + }); + + it("caps --days at MAX_BACKFILL_DAYS even if user requests more", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: { days: 90 }, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-04-05"); // today - 29 + }); + }); + + describe("smart-sync from last_push_date", () => { + it("includes the last_push_date when it's within DEFAULT_SYNC_DAYS", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: {}, + lastPushDate: "2026-05-01", + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-05-01"); + expect(isoDay(r.until)).toBe("2026-05-04"); + }); + + it("caps at DEFAULT_SYNC_DAYS when last_push_date is too far back", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: {}, + lastPushDate: "2026-04-15", + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-04-28"); // today - 6 (DEFAULT_SYNC_DAYS=7, +1) + }); + + it("re-syncs only today when last_push_date >= today", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: {}, + lastPushDate: "2026-05-04", + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-05-04"); + expect(isoDay(r.until)).toBe("2026-05-04"); + }); + }); + + describe("fresh install (no last_push_date)", () => { + it("backfills last 3 days by default", () => { + const r = resolvePushDateRange({ + today: dateAt("2026-05-04"), + options: {}, + shouldRunCodexRepair: false, + }); + expect(r.ok).toBe(true); + if (!r.ok) return; + expect(isoDay(r.since)).toBe("2026-05-02"); // today - 2 + expect(isoDay(r.until)).toBe("2026-05-04"); + }); + }); +}); diff --git a/packages/cli/src/commands/push.ts b/packages/cli/src/commands/push.ts index b0692c3..34a0b65 100755 --- a/packages/cli/src/commands/push.ts +++ b/packages/cli/src/commands/push.ts @@ -105,6 +105,69 @@ export function isWithinBackfillWindow(dateStr: string): boolean { return diffDays >= -1 && diffDays <= MAX_BACKFILL_DAYS; } +export type DateRangeResolution = + | { ok: true; since: Date; until: Date } + | { ok: false; error: string }; + +/** + * Pure resolver for the date range a push should cover. Extracted from + * `pushCommand` so each branch (explicit --date, codex repair, --days, + * smart-sync from last_push_date, fresh install) can be unit-tested without + * mocking ccusage / the API / the filesystem. + */ +export function resolvePushDateRange(args: { + today: Date; + options: { date?: string; days?: number }; + lastPushDate?: string; + shouldRunCodexRepair: boolean; +}): DateRangeResolution { + const { today, options, lastPushDate, shouldRunCodexRepair } = args; + const todayStr = formatDate(today); + + if (options.date) { + const target = parseDate(options.date); + if (daysBetween(today, target) > MAX_BACKFILL_DAYS) { + return { ok: false, error: `Date must be within the last ${MAX_BACKFILL_DAYS} days.` }; + } + if (target > today) { + return { ok: false, error: "Cannot push usage for a future date." }; + } + return { ok: true, since: target, until: target }; + } + + if (shouldRunCodexRepair) { + const since = new Date(today); + since.setDate(since.getDate() - MAX_BACKFILL_DAYS + 1); + return { ok: true, since, until: today }; + } + + if (options.days) { + const days = Math.min(options.days, MAX_BACKFILL_DAYS); + const since = new Date(today); + since.setDate(since.getDate() - days + 1); + return { ok: true, since, until: today }; + } + + if (lastPushDate) { + if (lastPushDate >= todayStr) { + return { ok: true, since: new Date(today), until: new Date(today) }; + } + const gap = daysBetweenStrings(lastPushDate, todayStr); + if (gap > DEFAULT_SYNC_DAYS) { + const since = new Date(today); + since.setDate(since.getDate() - DEFAULT_SYNC_DAYS + 1); + return { ok: true, since, until: today }; + } + return { ok: true, since: parseDate(lastPushDate), until: today }; + } + + // Never pushed before — backfill last 3 days by default + const FIRST_RUN_BACKFILL_DAYS = 3; + const since = new Date(today); + since.setDate(since.getDate() - FIRST_RUN_BACKFILL_DAYS + 1); + return { ok: true, since, until: today }; +} + function formatTokens(n: number): string { if (n >= 1_000_000) return `${(n / 1_000_000).toFixed(1)}M`; if (n >= 1_000) return `${Math.round(n / 1_000)}k`; @@ -198,61 +261,22 @@ export async function pushCommand(options: PushOptions, apiUrlOverride?: string) } const today = new Date(); - const todayStr = formatDate(today); const shouldRunCodexRepair = !options.date && !config.codex_native_repair_completed_at && await containsSessionFile(); - let sinceDate: Date; - let untilDate: Date; - - if (options.date) { - const target = parseDate(options.date); - if (daysBetween(today, target) > MAX_BACKFILL_DAYS) { - console.error(`Date must be within the last ${MAX_BACKFILL_DAYS} days.`); - process.exit(1); - } - if (target > today) { - console.error("Cannot push usage for a future date."); - process.exit(1); - } - sinceDate = target; - untilDate = target; - } else if (shouldRunCodexRepair) { - sinceDate = new Date(today); - sinceDate.setDate(sinceDate.getDate() - MAX_BACKFILL_DAYS + 1); - untilDate = today; - } else if (options.days) { - const days = Math.min(options.days, MAX_BACKFILL_DAYS); - sinceDate = new Date(today); - sinceDate.setDate(sinceDate.getDate() - days + 1); - untilDate = today; - } else if (config.last_push_date) { - // Smart sync: calculate days since last push - if (config.last_push_date >= todayStr) { - // Already pushed today — re-sync with days=1 - sinceDate = today; - untilDate = today; - } else { - const gap = daysBetweenStrings(config.last_push_date, todayStr); - if (gap > DEFAULT_SYNC_DAYS) { - // Can't include last pushed date, too far back — cap at default window - const days = DEFAULT_SYNC_DAYS; - sinceDate = new Date(today); - sinceDate.setDate(sinceDate.getDate() - days + 1); - } else { - // Include last pushed date to catch any updates from that day - sinceDate = parseDate(config.last_push_date); - } - untilDate = today; - } - } else { - // Never pushed before — backfill last 3 days by default - const FIRST_RUN_BACKFILL_DAYS = 3; - sinceDate = new Date(today); - sinceDate.setDate(sinceDate.getDate() - FIRST_RUN_BACKFILL_DAYS + 1); - untilDate = today; + const resolution = resolvePushDateRange({ + today, + options: { date: options.date, days: options.days }, + lastPushDate: config.last_push_date, + shouldRunCodexRepair, + }); + if (!resolution.ok) { + console.error(resolution.error); + process.exit(1); } + const sinceDate = resolution.since; + const untilDate = resolution.until; const sinceStr = formatDateCompact(sinceDate); const untilStr = formatDateCompact(untilDate); diff --git a/packages/cli/src/lib/ccusage.ts b/packages/cli/src/lib/ccusage.ts index 5a45233..94eec63 100755 --- a/packages/cli/src/lib/ccusage.ts +++ b/packages/cli/src/lib/ccusage.ts @@ -65,15 +65,25 @@ export function isCcusageInstalled(): boolean { return isOnPath("ccusage"); } +/** + * Pick the right global-install command for ccusage. Pure: takes a snapshot of + * environment state and returns the command/args to run. Extracted so the + * decision logic is unit-testable without spawning processes or mocking PATH. + */ +export function pickInstallCommand(env: { hasBun: boolean }): { cmd: string; args: string[]; manager: "bun" | "npm" } { + if (env.hasBun) { + return { cmd: "bun", args: ["add", "-g", "ccusage"], manager: "bun" }; + } + return { cmd: "npm", args: ["install", "-g", "ccusage"], manager: "npm" }; +} + /** * Best-effort install of ccusage globally. Prefers `bun add -g` when bun is * present (faster, and Straude is bun-first), falls back to `npm install -g`. * stdio is inherited so the user sees install progress. */ function installCcusage(): void { - const useBun = isOnPath("bun"); - const cmd = useBun ? "bun" : "npm"; - const args = useBun ? ["add", "-g", "ccusage"] : ["install", "-g", "ccusage"]; + const { cmd, args } = pickInstallCommand({ hasBun: isOnPath("bun") }); execFileSync(cmd, args, { stdio: "inherit", timeout: 5 * 60 * 1000, @@ -125,7 +135,7 @@ export async function ensureCcusageInstalled( posthog.capture({ distinctId, event: "ccusage_install_attempted", - properties: { manager: isOnPath("bun") ? "bun" : "npm" }, + properties: { manager: pickInstallCommand({ hasBun: isOnPath("bun") }).manager }, }); try { diff --git a/packages/cli/src/lib/first-run.ts b/packages/cli/src/lib/first-run.ts index f3ca467..f9e54c5 100644 --- a/packages/cli/src/lib/first-run.ts +++ b/packages/cli/src/lib/first-run.ts @@ -2,18 +2,23 @@ import { existsSync, mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { CONFIG_DIR } from "../config.js"; -export const FIRST_RUN_MARKER = join(CONFIG_DIR, ".first-run"); +export const FIRST_RUN_MARKER_FILENAME = ".first-run"; +export const FIRST_RUN_MARKER = join(CONFIG_DIR, FIRST_RUN_MARKER_FILENAME); -export function isFirstRun(): boolean { - return !existsSync(FIRST_RUN_MARKER); +function markerPath(configDir: string): string { + return join(configDir, FIRST_RUN_MARKER_FILENAME); } -export function markFirstRun(): void { +export function isFirstRun(configDir: string = CONFIG_DIR): boolean { + return !existsSync(markerPath(configDir)); +} + +export function markFirstRun(configDir: string = CONFIG_DIR): void { try { - if (!existsSync(CONFIG_DIR)) { - mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 }); + if (!existsSync(configDir)) { + mkdirSync(configDir, { recursive: true, mode: 0o700 }); } - writeFileSync(FIRST_RUN_MARKER, new Date().toISOString() + "\n", { + writeFileSync(markerPath(configDir), new Date().toISOString() + "\n", { encoding: "utf-8", mode: 0o600, });