fix(cli): atomic hook state writes, SDK fetch timeout, correct git cwd

packages/cli/src/__tests__/git.test.ts

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
-import { getCurrentRepo, getCurrentBranch, getDiff, getChangedFiles, getCommitLog, snapshotRef, getWorkingTreeDiff } from "../git.js";
+import { getCurrentRepo, getCurrentBranch, getChangedFiles, getWorkingTreeDiff } from "../git.js";
 
 // Mock child_process.execSync
 vi.mock("node:child_process", () => ({
@@ -92,43 +92,6 @@ describe("getCurrentBranch", () => {
   });
 });
 
-describe("getDiff", () => {
-  it("gets diff between two refs", () => {
-    mockExecSync.mockReturnValue("+added line\n");
-    const result = getDiff("abc123", "def456");
-    expect(mockExecSync).toHaveBeenCalledWith(
-      "git diff abc123 def456",
-      expect.objectContaining({ encoding: "utf-8" }),
-    );
-    expect(result).toBe("+added line");
-  });
-
-  it("gets diff from a single ref", () => {
-    mockExecSync.mockReturnValue("-removed\n");
-    getDiff("abc123");
-    expect(mockExecSync).toHaveBeenCalledWith(
-      "git diff abc123",
-      expect.objectContaining({ encoding: "utf-8" }),
-    );
-  });
-
-  it("defaults to diff HEAD when no refs given", () => {
-    mockExecSync.mockReturnValue("some diff\n");
-    getDiff();
-    expect(mockExecSync).toHaveBeenCalledWith(
-      "git diff HEAD",
-      expect.objectContaining({ encoding: "utf-8" }),
-    );
-  });
-
-  it("returns empty string when git fails", () => {
-    mockExecSync.mockImplementation(() => {
-      throw new Error("not a git repo");
-    });
-    expect(getDiff()).toBe("");
-  });
-});
-
 describe("getChangedFiles", () => {
   it("parses porcelain status output", () => {
     // Note: git() trims the full output, so leading space on first line is lost.
@@ -167,52 +130,22 @@ describe("getChangedFiles", () => {
     });
     expect(getChangedFiles()).toEqual([]);
   });
-});
-
-describe("getCommitLog", () => {
-  it("returns commit log output", () => {
-    mockExecSync.mockReturnValue("abc1234 Initial commit\ndef5678 Second commit\n");
-    const log = getCommitLog();
-    expect(log).toContain("abc1234 Initial commit");
-  });
-
-  it("passes since argument when provided", () => {
-    mockExecSync.mockReturnValue("abc1234 Recent commit\n");
-    getCommitLog("2025-01-01");
-    expect(mockExecSync).toHaveBeenCalledWith(
-      expect.stringContaining('--since="2025-01-01"'),
-      expect.anything(),
-    );
-  });
-
-  it("defaults to -10 when no since provided", () => {
-    mockExecSync.mockReturnValue("some log\n");
-    getCommitLog();
-    expect(mockExecSync).toHaveBeenCalledWith(
-      expect.stringContaining("-10"),
-      expect.anything(),
-    );
-  });
-});
-
-describe("snapshotRef", () => {
-  it("returns stash create result when available", () => {
-    mockExecSync.mockReturnValueOnce("abc123def\n");
-    expect(snapshotRef()).toBe("abc123def");
-  });
 
-  it("falls back to rev-parse HEAD when stash create returns empty", () => {
-    mockExecSync
-      .mockReturnValueOnce("\n") // stash create returns empty
-      .mockReturnValueOnce("deadbeef\n"); // rev-parse HEAD
-    expect(snapshotRef()).toBe("deadbeef");
+  it("passes cwd through to git when provided", () => {
+    // finalizeSession runs in the hook subprocess, whose cwd is wherever
+    // Claude Code was launched from — the status must run in the tracked
+    // repo (state.cwd) or the run records another repo's changes.
+    mockExecSync.mockReturnValue("M  src/index.ts\n");
+    getChangedFiles("/some/working/dir");
+    const [, opts] = mockExecSync.mock.calls[0]!;
+    expect((opts as { cwd?: string }).cwd).toBe("/some/working/dir");
   });
 
-  it("returns empty string when both fail", () => {
-    mockExecSync.mockImplementation(() => {
-      throw new Error("not a git repo");
-    });
-    expect(snapshotRef()).toBe("");
+  it("does not set cwd when none is provided", () => {
+    mockExecSync.mockReturnValue("\n");
+    getChangedFiles();
+    const [, opts] = mockExecSync.mock.calls[0]!;
+    expect((opts as { cwd?: string }).cwd).toBeUndefined();
   });
 });
 
@@ -227,4 +160,13 @@ describe("getWorkingTreeDiff", () => {
     mockExecSync.mockReturnValue("\n");
     expect(getWorkingTreeDiff()).toBe("");
   });
+
+  it("passes cwd through to git when provided", () => {
+    mockExecSync.mockReturnValue("+new line\n");
+    getWorkingTreeDiff("/some/working/dir");
+    expect(mockExecSync).toHaveBeenCalledWith(
+      "git diff HEAD",
+      expect.objectContaining({ cwd: "/some/working/dir" }),
+    );
+  });
 });

packages/cli/src/__tests__/hook-handlers.test.ts

@@ -9,6 +9,7 @@ import {
 } from "node:fs";
 import { tmpdir } from "node:os";
 import { join, resolve } from "node:path";
+import { execSync } from "node:child_process";
 import {
   _readState,
   _cleanupState,
@@ -794,6 +795,45 @@ describe("handleSessionEnd", () => {
     }
   });
 
+  it("computes the git diff from the session's cwd, not the hook process cwd", async () => {
+    // Build a real throwaway git repo under tmpHome with one unstaged
+    // modification. The vitest process itself runs from the agentops repo,
+    // so if finalizeSession ran git without a cwd it would diff THIS repo
+    // (and never see the marker below).
+    const repoDir = join(tmpHome, "session-repo");
+    mkdirSync(repoDir, { recursive: true });
+    const g = (cmd: string) =>
+      execSync(`git ${cmd}`, { cwd: repoDir, stdio: ["pipe", "pipe", "pipe"] });
+    g("init");
+    writeFileSync(join(repoDir, "tracked.txt"), "original contents\n", "utf-8");
+    g("add tracked.txt");
+    g('-c user.email=t@t.test -c user.name=t commit -m init');
+    writeFileSync(
+      join(repoDir, "tracked.txt"),
+      "MARKER-changed-in-session-repo\n",
+      "utf-8",
+    );
+
+    const sid = freshSessionId();
+    await _handleSessionStart({ session_id: sid, cwd: repoDir }, testDbPath);
+    const state = _readState(sid)!;
+
+    await runHook(() => _handleSessionEnd({ session_id: sid, cwd: repoDir }, testDbPath));
+
+    const db = getDb(testDbPath);
+    const run = getRun(db, createRunId(state.runId))!;
+    const diffs = run.artifacts.flatMap((a) => a.diffs).join("\n");
+    expect(diffs).toContain("MARKER-changed-in-session-repo");
+
+    // changedFilesCount flows into the run.completed event payload.
+    const events = listEvents(db, { limit: 20 });
+    const completed = events.find(
+      (e) => e.type === "run.completed" && e.sourceId === state.runId,
+    )!;
+    expect(completed).toBeDefined();
+    expect((completed.payload as { filesChanged?: number }).filesChanged).toBe(1);
+  });
+
   it("emits run.completed event", async () => {
     const sid = freshSessionId();
     await _handleSessionStart({ session_id: sid, cwd: "/tmp/test-cwd" }, testDbPath);

packages/cli/src/__tests__/hook-ops-sdk.test.ts

@@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { existsSync, mkdirSync, readFileSync, rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { resolve } from "node:path";
-import { createOps, resolveOpsConfig, isSdkMode, SdkError } from "../hook-ops.js";
+import { createOps, resolveOpsConfig, isSdkMode, SdkError, sdkTimeoutMs } from "../hook-ops.js";
 import { outboxPath } from "../outbox.js";
 import type { Action, Metrics } from "@agentops/core";
 import { createActionId } from "@agentops/core";
@@ -136,6 +136,106 @@ describe("resolveOpsConfig + createOps", () => {
   });
 });
 
+// ─── Fetch timeout ─────────────────────────────────────────────────────────
+//
+// A dashboard that HANGS (accepts the connection but never responds) must
+// not stall the hook until Claude Code's 60s hook timeout. Every SDK fetch
+// carries AbortSignal.timeout(sdkTimeoutMs()); the resulting rejection is
+// caught like any network error (status 0), so it follows the existing
+// transient semantics: outboxed for reports, thrown (→ fail-open in the
+// handlers) for synchronous policy decisions.
+
+describe("SDK fetch timeout", () => {
+  let savedTimeout: string | undefined;
+
+  beforeEach(() => {
+    savedTimeout = process.env["AGENTOPS_SDK_TIMEOUT_MS"];
+  });
+
+  afterEach(() => {
+    if (savedTimeout === undefined) delete process.env["AGENTOPS_SDK_TIMEOUT_MS"];
+    else process.env["AGENTOPS_SDK_TIMEOUT_MS"] = savedTimeout;
+  });
+
+  /** Fetch stub that never resolves — only rejects when its signal aborts. */
+  function mockHangingFetch(): ReturnType<typeof vi.fn> {
+    const fn = vi.fn(
+      (_url: string, init: RequestInit) =>
+        new Promise((_resolve, reject) => {
+          const signal = init.signal;
+          if (!signal) return; // no signal → hang forever (test would time out)
+          if (signal.aborted) reject(signal.reason);
+          else signal.addEventListener("abort", () => reject(signal.reason));
+        }),
+    );
+    vi.stubGlobal("fetch", fn);
+    return fn;
+  }
+
+  it("defaults to 5000ms, honors AGENTOPS_SDK_TIMEOUT_MS, ignores garbage", () => {
+    delete process.env["AGENTOPS_SDK_TIMEOUT_MS"];
+    expect(sdkTimeoutMs()).toBe(5000);
+    process.env["AGENTOPS_SDK_TIMEOUT_MS"] = "250";
+    expect(sdkTimeoutMs()).toBe(250);
+    process.env["AGENTOPS_SDK_TIMEOUT_MS"] = "not-a-number";
+    expect(sdkTimeoutMs()).toBe(5000);
+    process.env["AGENTOPS_SDK_TIMEOUT_MS"] = "-1";
+    expect(sdkTimeoutMs()).toBe(5000);
+  });
+
+  it("attaches an AbortSignal to every SDK fetch", async () => {
+    const fetchMock = mockFetch([{ status: 200, body: { ok: true } }]);
+    const ops = createOps(resolveOpsConfig(), "timeout-signal-session");
+
+    await ops.reportAction("run_abc", makeAction());
+
+    const [, init] = fetchMock.mock.calls[0] as [string, RequestInit];
+    expect(init.signal).toBeInstanceOf(AbortSignal);
+  });
+
+  it("checkPolicy against a hanging server rejects with SdkError status 0 (transient)", async () => {
+    process.env["AGENTOPS_SDK_TIMEOUT_MS"] = "25";
+    mockHangingFetch();
+    const ops = createOps(resolveOpsConfig(), "timeout-hang-session");
+
+    let caught: unknown;
+    try {
+      await ops.checkPolicy({
+        runId: "run_abc",
+        toolName: "Bash",
+        toolInput: { command: "ls" },
+        cumulativeCostUsd: 0,
+      });
+      expect.unreachable("checkPolicy should have thrown");
+    } catch (err) {
+      caught = err;
+    }
+    expect(caught).toBeInstanceOf(SdkError);
+    // Status 0 = network-shaped failure → handlers fail-open (or block
+    // under AGENTOPS_FAIL_CLOSED), exactly like a refused connection.
+    expect((caught as SdkError).status).toBe(0);
+  });
+
+  it("reportAction against a hanging server is queued in the outbox, not thrown", async () => {
+    process.env["AGENTOPS_SDK_TIMEOUT_MS"] = "25";
+    mockHangingFetch();
+    const ops = createOps(resolveOpsConfig(), "timeout-outbox-session");
+
+    const errSpy = vi
+      .spyOn(process.stderr, "write")
+      .mockImplementation(() => true);
+    try {
+      await expect(ops.reportAction("run_abc", makeAction())).resolves.toBeUndefined();
+    } finally {
+      errSpy.mockRestore();
+    }
+
+    const outbox = readOutbox("timeout-outbox-session");
+    expect(outbox).toHaveLength(1);
+    expect(outbox[0]!.op).toBe("reportAction");
+  });
+});
+
 // ─── Happy path ────────────────────────────────────────────────────────────
 
 describe("SdkOps happy path", () => {

packages/cli/src/__tests__/hook.test.ts

@@ -1,6 +1,6 @@
 import { describe, it, expect, beforeEach, afterEach } from "vitest";
 import { Command } from "commander";
-import { existsSync, readFileSync, writeFileSync, unlinkSync, mkdirSync } from "node:fs";
+import { existsSync, readFileSync, writeFileSync, unlinkSync, mkdirSync, readdirSync, rmSync } from "node:fs";
 import { tmpdir, homedir } from "node:os";
 import { join, resolve } from "node:path";
 import { registerHookCommand } from "../commands/hook.js";
@@ -139,6 +139,72 @@
   });
 });
 
+// ─── Atomic state write tests ────────────────────────────────────────────────
+//
+// writeState must be temp-file + rename (atomic on POSIX): the state file is
+// shared between SubagentStop's read-modify-write and parallel subagents'
+// PreToolUse/PostToolUse readers. A torn read would make readState return
+// null and silently skip policy enforcement for that tool call.
+
+describe("Atomic state writes", () => {
+  const testSessionId = "test-session-atomic-" + Date.now();
+
+  const state: HookState = {
+    runId: "run_atomic",
+    sessionId: "session_atomic",
+    dbPath: "/tmp/test.db",
+    startTime: "2025-01-01T00:00:00.000Z",
+    agentsSpawned: 0,
+    agentsCompleted: 0,
+    finalized: false,
+  };
+
+  afterEach(() => {
+    _cleanupState(testSessionId);
+  });
+
+  it("leaves no temp file behind after a successful write", () => {
+    _writeState(testSessionId, state);
+    const dir = join(homedir(), ".agentops", "state");
+    const leftovers = readdirSync(dir).filter(
+      (f) => f.startsWith(`${testSessionId}.json.`) && f.endsWith(".tmp"),
+    );
+    expect(leftovers).toEqual([]);
+  });
+
+  it("produces complete, valid JSON on disk", () => {
+    _writeState(testSessionId, state);
+    const raw = readFileSync(stateFilePath(testSessionId), "utf-8");
+    expect(JSON.parse(raw)).toEqual(state);
+  });
+
+  it("overwrite replaces the whole file, never mixes contents", () => {
+    // A longer first payload followed by a shorter one would leave trailing
+    // garbage under a naive truncate-less in-place write scheme.
+    const long: HookState = { ...state, cwd: "/very/long/path/".repeat(50) };
+    _writeState(testSessionId, long);
+    _writeState(testSessionId, state);
+    const raw = readFileSync(stateFilePath(testSessionId), "utf-8");
+    expect(JSON.parse(raw)).toEqual(state);
+  });
+
+  it("cleans up the temp file when the rename fails", () => {
+    // Occupy the final path with a directory so renameSync fails.
+    const finalPath = stateFilePath(testSessionId);
+    mkdirSync(finalPath, { recursive: true });
+    try {
+      expect(() => _writeState(testSessionId, state)).toThrow();
+      const dir = join(homedir(), ".agentops", "state");
+      const leftovers = readdirSync(dir).filter(
+        (f) => f.startsWith(`${testSessionId}.json.`) && f.endsWith(".tmp"),
+      );
+      expect(leftovers).toEqual([]);
+    } finally {
+      rmSync(finalPath, { recursive: true, force: true });
+    }
+  });
+});
+
 // ─── Action mapping tests ───────────────────────────────────────────────────
 
 describe("Tool-to-action mapping", () => {
@@ -956,7 +1022,7 @@
 
   afterEach(() => {
     try {
       const { rmSync } = require("node:fs");
       rmSync(tmpTranscriptDir, { recursive: true, force: true });
     } catch {
       // Ignore cleanup errors