diff --git a/packages/cli/src/__tests__/git.test.ts b/packages/cli/src/__tests__/git.test.ts index de92d6d..9e9893a 100644 --- a/packages/cli/src/__tests__/git.test.ts +++ b/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" }), + ); + }); }); diff --git a/packages/cli/src/__tests__/hook-handlers.test.ts b/packages/cli/src/__tests__/hook-handlers.test.ts index e76af23..85f5d75 100644 --- a/packages/cli/src/__tests__/hook-handlers.test.ts +++ b/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); diff --git a/packages/cli/src/__tests__/hook-ops-sdk.test.ts b/packages/cli/src/__tests__/hook-ops-sdk.test.ts index abaa7a5..476f0ee 100644 --- a/packages/cli/src/__tests__/hook-ops-sdk.test.ts +++ b/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 { + 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", () => { diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 4b372f9..950ed7e 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/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 @@ describe("State file management", () => { }); }); +// ─── 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", () => { diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index dcd4d23..8fa7329 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -1,5 +1,5 @@ import { Command } from "commander"; -import { readFileSync, writeFileSync, unlinkSync, existsSync, mkdirSync, chmodSync, statSync, realpathSync } from "node:fs"; +import { readFileSync, writeFileSync, renameSync, unlinkSync, existsSync, mkdirSync, chmodSync, statSync, realpathSync } from "node:fs"; import { homedir } from "node:os"; import { join, resolve, sep } from "node:path"; import { @@ -76,12 +76,34 @@ function writeState(claudeSessionId: string, state: HookState): void { mkdirSync(dir, { recursive: true, mode: 0o700 }); } const path = stateFilePath(claudeSessionId); - writeFileSync(path, JSON.stringify(state), { encoding: "utf-8", mode: 0o600 }); - // chmod in case the file already existed with looser perms. + // Atomic write: temp file in the same directory, then rename (atomic on + // POSIX) — same pattern as Outbox.drain in outbox.ts. The state file is + // shared: handleSubagentStop does a read-modify-write while parallel + // subagents' PreToolUse/PostToolUse hooks read it concurrently. A bare + // writeFileSync can expose a torn file; readState swallows the parse + // error and returns null, which silently skips policy enforcement for + // that tool call. The pid suffix keeps two concurrent hook processes + // from clobbering each other's temp file. + const tmp = `${path}.${process.pid}.tmp`; try { - chmodSync(path, 0o600); - } catch { - // Best-effort; not all filesystems support chmod. + writeFileSync(tmp, JSON.stringify(state), { encoding: "utf-8", mode: 0o600 }); + // chmod in case the temp file already existed with looser perms + // (writeFileSync's mode only applies at creation). The rename carries + // the temp file's mode to the final path. + try { + chmodSync(tmp, 0o600); + } catch { + // Best-effort; not all filesystems support chmod. + } + renameSync(tmp, path); + } catch (err) { + // Don't leave a stray temp file behind on failure. + try { + unlinkSync(tmp); + } catch { + // Ignore — the temp file may never have been created. + } + throw err; } } @@ -148,15 +170,6 @@ function readHookUsage( return { usage, backend }; } -// ─── Stale state detection ─────────────────────────────────────────────────── - -function handleStaleState(sessionId: string, state: HookState): void { - process.stderr.write( - `[agentops] Stale session detected (database was reset). Restart Claude Code to begin tracking.\n`, - ); - cleanupState(sessionId); -} - // ─── Fail-closed enforcement ───────────────────────────────────────────────── // // Default behavior is fail-open: when the SDK can't reach the dashboard or @@ -445,9 +458,15 @@ function opsConfigFromState(state: HookState, dbPath?: string) { async function finalizeSession(input: HookInput, state: HookState, dbPath?: string): Promise { const ops = createOps(opsConfigFromState(state, dbPath), input.session_id); - // Compute wall time + git diff locally — both are on this machine. - const changedFiles = getChangedFiles(); - const diff = getWorkingTreeDiff(); + // Compute wall time + git diff locally — both are on this machine. Run + // git in the session's tracked directory (state.cwd, captured at + // session-start) rather than the hook subprocess's inherited cwd — the + // subprocess inherits wherever Claude Code was launched from, which may + // be a different repo entirely. Same reasoning as the cwd passed to the + // git lookups in handleSessionStart. + const gitCwd = state.cwd ?? input.cwd; + const changedFiles = getChangedFiles(gitCwd); + const diff = getWorkingTreeDiff(gitCwd); const wallTimeMs = Date.now() - new Date(state.startTime).getTime(); // Read final cost/token usage from the local transcript. diff --git a/packages/cli/src/git.ts b/packages/cli/src/git.ts index 9104a90..5f2205d 100644 --- a/packages/cli/src/git.ts +++ b/packages/cli/src/git.ts @@ -51,24 +51,13 @@ export function getCurrentBranch(cwd?: string): string { return git("rev-parse --abbrev-ref HEAD", cwd) || "unknown"; } -export function getDiff(fromRef?: string, toRef?: string): string { - if (fromRef && toRef) { - return git(`diff ${fromRef} ${toRef}`); - } - if (fromRef) { - return git(`diff ${fromRef}`); - } - // Default: working tree diff (staged + unstaged) - return git("diff HEAD"); -} - export interface ChangedFile { status: "added" | "modified" | "deleted" | "renamed" | "unknown"; path: string; } -export function getChangedFiles(): ChangedFile[] { - const output = git("status --porcelain"); +export function getChangedFiles(cwd?: string): ChangedFile[] { + const output = git("status --porcelain", cwd); if (!output) return []; return output.split("\n").filter(Boolean).map((line) => { @@ -98,24 +87,10 @@ export function getChangedFiles(): ChangedFile[] { }); } -export function getCommitLog(since?: string): string { - const sinceArg = since ? ` --since="${since}"` : " -10"; - return git(`log --oneline${sinceArg}`); -} - -/** - * Take a snapshot of the current working tree state for diff comparison. - * Returns the current HEAD commit hash (or empty string if no commits). - */ -export function snapshotRef(): string { - return git("stash create") || git("rev-parse HEAD") || ""; -} - /** - * Get the diff of all changes in the working tree (staged + unstaged + untracked shown as new). + * Get the diff of all changes in the working tree (staged + unstaged). */ -export function getWorkingTreeDiff(): string { +export function getWorkingTreeDiff(cwd?: string): string { // Include both staged and unstaged - const tracked = git("diff HEAD"); - return tracked; + return git("diff HEAD", cwd); } diff --git a/packages/cli/src/hook-ops.ts b/packages/cli/src/hook-ops.ts index 00865da..df382ca 100644 --- a/packages/cli/src/hook-ops.ts +++ b/packages/cli/src/hook-ops.ts @@ -544,6 +544,29 @@ class DirectOps implements HookOps { // stays in "running" status until an admin intervenes, which is surfaced // to the operator via a loud stderr warning. +// Every SDK HTTP call is bounded by this timeout. Without one, a dashboard +// that HANGS (rather than refuses the connection) stalls checkPolicy on +// every PreToolUse until Claude Code's own hook timeout (60s default), +// freezing the user's session once per tool call — the fail-open path only +// helps when the request errors. Default 5s; override with the +// AGENTOPS_SDK_TIMEOUT_MS env var (positive integer, milliseconds). +// +// A timeout rejects the fetch, which post() catches like any other network +// error (status 0) — so it flows through the existing transient semantics: +// SdkError(status 0) → outboxed for reportAction/reportArtifact/ +// reportMetrics, fail-open (or AGENTOPS_FAIL_CLOSED block) for +// checkPolicy/checkBudget. +const DEFAULT_SDK_TIMEOUT_MS = 5000; + +export function sdkTimeoutMs(): number { + const raw = process.env["AGENTOPS_SDK_TIMEOUT_MS"]?.trim(); + if (raw) { + const n = Number(raw); + if (Number.isFinite(n) && n > 0) return n; + } + return DEFAULT_SDK_TIMEOUT_MS; +} + class SdkOps implements HookOps { private readonly base: string; private readonly token: string; @@ -589,11 +612,15 @@ class SdkOps implements HookOps { Authorization: `Bearer ${this.token}`, }, body: JSON.stringify(body), + // Bound every call so a hanging dashboard can't stall the hook + // (and with it, the user's Claude Code session). See sdkTimeoutMs. + signal: AbortSignal.timeout(sdkTimeoutMs()), }); const data = (await res.json().catch(() => ({}))) as T; return { status: res.status, data }; } catch (err) { - // Network failure surfaces as status 0 so callers treat as transient. + // Network failure (including timeout aborts) surfaces as status 0 + // so callers treat it as transient. const message = err instanceof Error ? err.message : String(err); return { status: 0, data: { error: message } as unknown as T }; }