test: fix timeouts (#3)

* test: fix timeouts

* fix: add git credentials to test

* docs: capture learnings

* ci: update workflow name to comply with org setup

* refactor: format fix

Author: rhuanbarreto

.archgate/adrs/ARCH-005-testing-standards.md

@@ -40,6 +40,8 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes
 - Use `tests/fixtures/` for sample data files
 - Use temp directories (`mkdtemp`) for tests that write to the filesystem
 - Clean up temp directories in `afterEach` or `afterAll`
+- **Close external SDK instances** (servers, clients, transports) in `afterEach` or `afterAll` by calling their cleanup method (e.g., `await server.close()`). Manage their lifecycle in `beforeEach`/`afterEach` rather than inside individual test bodies so cleanup is guaranteed.
+- **When a test creates a temp git repo and needs to call `git commit`, configure local user identity first** — CI runners have no global git config, so commits fail without explicit local identity. Set it with `await Bun.$\`git config user.email "test@test.com"\`.cwd(tempDir).quiet()`and`await Bun.$\`git config user.name "Test"\`.cwd(tempDir).quiet()`immediately after`git init`.
 - Test public module interfaces, not private implementation details
 - Use descriptive test names that explain the expected behavior
 
@@ -48,6 +50,8 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes
 - Don't test private implementation details — test the public API of each module
 - Don't depend on network access in unit tests
 - Don't leave temp files after test runs
+- **Don't leave external SDK instances open after tests** — instances from libraries such as `@modelcontextprotocol/sdk` hold internal references (e.g., `AjvJsonSchemaValidator` backed by `ajv`) that keep Bun's event loop alive on Linux, causing `bun test` to hang indefinitely after all tests complete even though every test passes. Always call the cleanup method in `afterEach`.
+- **Don't rely on globally-configured git identity in temp git repos** — always set `user.email` and `user.name` locally in any repo that makes commits. Omitting this works locally (where developers have global git config) but fails silently in CI, producing a cryptic `ShellPromise` error with no indication that git identity is the cause.
 - Don't skip tests without a tracking issue
 - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module
 
@@ -83,6 +87,53 @@ describe("runChecks", () => {
 });
 ```
 
+### Good Example — Temp Git Repo with Commits
+
+```typescript
+// tests/engine/git-files.test.ts
+it("returns both staged and unstaged changes", async () => {
+  await Bun.$`git init`.cwd(tempDir).quiet();
+  // GOOD: set local identity before any commit — CI has no global git config
+  await Bun.$`git config user.email "test@test.com"`.cwd(tempDir).quiet();
+  await Bun.$`git config user.name "Test"`.cwd(tempDir).quiet();
+  writeFileSync(join(tempDir, "a.ts"), "export const a = 1;");
+  await Bun.$`git add a.ts`.cwd(tempDir).quiet();
+  await Bun.$`git commit -m "init"`.cwd(tempDir).quiet();
+  // ... rest of test
+});
+```
+
+### Good Example — External SDK Cleanup
+
+```typescript
+// tests/mcp/resources.test.ts
+import { describe, expect, test, beforeEach, afterEach } from "bun:test";
+import { mkdtempSync, rmSync, mkdirSync } from "node:fs";
+import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
+import { registerResources } from "../../src/mcp/resources";
+
+describe("registerResources", () => {
+  let tempDir: string;
+  let server: McpServer;
+
+  // GOOD: lifecycle managed in beforeEach/afterEach, not inside test bodies
+  beforeEach(() => {
+    tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-res-test-"));
+    mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true });
+    server = new McpServer({ name: "test", version: "0.0.0" });
+  });
+
+  afterEach(async () => {
+    await server.close(); // GOOD: releases internal validator references
+    rmSync(tempDir, { recursive: true, force: true });
+  });
+
+  test("does not throw when registering resources", () => {
+    expect(() => registerResources(server, tempDir)).not.toThrow();
+  });
+});
+```
+
 ### Bad Example
 
 ```typescript
@@ -102,6 +153,22 @@ it("writes output", async () => {
   await Bun.write("/tmp/test-output.json", data);
   // No cleanup — file persists after test
 });
+
+// BAD: SDK instance created inside test body — no guaranteed cleanup path
+it("registers resources", () => {
+  const server = new McpServer({ name: "test", version: "0.0.0" });
+  // server.close() never called — event loop held open on Linux
+  expect(() => registerResources(server, tempDir)).not.toThrow();
+});
+
+// BAD: git commit without local identity — works locally, fails in CI
+it("reads changes", async () => {
+  await Bun.$`git init`.cwd(tempDir).quiet();
+  writeFileSync(join(tempDir, "a.ts"), "x");
+  await Bun.$`git add a.ts`.cwd(tempDir).quiet();
+  await Bun.$`git commit -m "init"`.cwd(tempDir).quiet();
+  // Fails in CI: "*** Please tell me who you are."
+});
 ```
 
 ## Consequences
@@ -124,23 +191,28 @@ it("writes output", async () => {
   - **Mitigation:** The project pins a specific Bun version via `.prototools` (currently 1.3.8). Test runner API changes are caught during controlled Bun upgrades with full test suite validation.
 - **Coverage reporting gaps** — `bun test --coverage` may not report accurate coverage for all code paths, especially for dynamically imported modules.
   - **Mitigation:** Coverage is a guideline (80% target), not a hard gate. Critical modules (engine, formats) are tested thoroughly regardless of coverage numbers.
+- **Third-party SDK event loop retention** — External SDK instances that hold internal resource references (e.g., `AjvJsonSchemaValidator` inside `@modelcontextprotocol/sdk`) keep Bun's event loop alive on Linux after all tests complete, causing `bun test` to hang indefinitely. This does not surface on macOS (event loop drains normally there), making it a Linux-CI-only failure that is hard to reproduce locally.
+  - **Mitigation:** Always manage SDK lifecycle in `beforeEach`/`afterEach` and call the cleanup method (`close()`, `destroy()`, `disconnect()`) in `afterEach`. Add `timeout-minutes` to CI jobs as a safety net — the `code-pull-request.yml` job is set to 10 minutes to cap any future regressions.
 
 ## Compliance and Enforcement
 
 ### Automated Enforcement
 
 - **Archgate rule** `ARCH-005/test-mirrors-src`: Scans all source files in `src/` and verifies a corresponding `.test.ts` file exists in `tests/`. Severity: `error`.
-- **CI pipeline**: `bun test` runs on every pull request. Test failures block merge.
+- **CI pipeline**: `bun test --timeout 60000` runs on every pull request. Test failures and per-test timeouts block merge. All workflow jobs have `timeout-minutes` set to prevent indefinite hangs.
 
 ### Manual Enforcement
 
 Code reviewers MUST verify:
 
 1. New source files have corresponding test files
 2. Tests use temp directories for filesystem operations (no hardcoded paths)
-3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup)
+3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup) — including both temp directories and external SDK instances
+4. Tests that instantiate SDK objects (servers, clients, connections) manage their lifecycle in `beforeEach`/`afterEach`, not inside individual test bodies
+5. Tests that call `git commit` on a temp repo configure `user.email` and `user.name` locally before committing
 
 ## References
 
 - [Bun test runner documentation](https://bun.sh/docs/cli/test)
 - [ARCH-001 — Command Structure](./ARCH-001-command-structure.md) — In-process execution enables testing commands directly without process spawning
+- [ARCH-006 — Dependency Policy](./ARCH-006-dependency-policy.md) — Third-party dependencies introduce runtime behaviors (like event loop retention) that must be accounted for in test teardown

.claude/agent-memory/archgate-developer/MEMORY.md

@@ -26,6 +26,8 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv
 
 ## Patterns & Fixes
 
+- **`McpServer` event loop retention on Linux** — `new McpServer()` from `@modelcontextprotocol/sdk` creates an `AjvJsonSchemaValidator` (backed by `ajv` + `ajv-formats`) that keeps Bun's event loop alive on Linux after tests complete, causing `bun test` to hang for 30+ minutes. macOS drains the event loop fine — this is a Linux-CI-only failure. Fix: manage server lifecycle in `beforeEach`/`afterEach` and call `await server.close()` in `afterEach`. Also captured in ARCH-005 Don'ts.
+- **`git commit` in temp repos requires local identity** — CI runners have no global `user.email`/`user.name` configured. Any test that runs `git commit` on a temp repo MUST call `git config user.email` and `git config user.name` locally after `git init`. Fails with a cryptic `ShellPromise` error in CI; passes locally. Also captured in ARCH-005 Do's.
 - **Bun import cache-busting**: Bun caches `import()` per-process. For long-running processes (MCP server), append `?t=${Date.now()}` to the import path to force re-reading from disk. Applied in `src/engine/loader.ts`.
 - **Never use `bunx prettier` directly** — Always use `bun run format` (to fix) or `bun run format:check` (to verify). Using `bunx prettier` can fail or use a different version than the project's devDependency. The same applies to all dev tools: prefer `bun run <script>` over `bunx <tool>` when a package.json script exists.
 - **`Bun.Glob.match()` triggers oxlint `prefer-regexp-test`** — `Bun.Glob.match()` returns a boolean (not a RegExp), but oxlint can't tell. Suppress with `// oxlint-disable-next-line prefer-regexp-test -- Bun.Glob.match() returns boolean, not RegExp`.

.github/workflows/code-pull-request.yml

@@ -12,8 +12,9 @@ concurrency:
 
 jobs:
   validate:
-    name: Validate
+    name: Validate Code
     runs-on: ubuntu-latest
+    timeout-minutes: 10
     if: github.event.pull_request.draft == false
     steps:
       - name: Checkout code

.github/workflows/release-binaries.yml

@@ -15,6 +15,7 @@ permissions:
 jobs:
   build:
     name: Build ${{ matrix.artifact }}
+    timeout-minutes: 15
     strategy:
       fail-fast: false
       matrix:
@@ -70,6 +71,7 @@ jobs:
   update-homebrew:
     name: Update Homebrew tap
     needs: build
+    timeout-minutes: 5
     runs-on: ubuntu-latest
 
     steps:

.github/workflows/release.yml

@@ -19,6 +19,7 @@ permissions:
 jobs:
   check:
     runs-on: ubuntu-latest
+    timeout-minutes: 10
     name: Context check
     outputs:
       continue: ${{ steps.check.outputs.continue }}
@@ -56,6 +57,7 @@ jobs:
           branch: release
   pull-request:
     runs-on: ubuntu-latest
+    timeout-minutes: 10
     name: Pull request
     needs: check
     if: needs.check.outputs.workflow == 'pull-request'
@@ -91,6 +93,7 @@ jobs:
           branch: release
   release:
     runs-on: ubuntu-latest
+    timeout-minutes: 10
     name: Release
     needs: check
     if: needs.check.outputs.workflow == 'release'

package.json

@@ -40,8 +40,8 @@
     "typecheck": "tsc --build",
     "format": "prettier --write .",
     "format:check": "prettier --check .",
-    "test": "bun test",
-    "test:watch": "bun test --watch",
+    "test": "bun test --timeout 60000",
+    "test:watch": "bun test --watch --timeout 60000",
     "validate": "bun run lint && bun run typecheck && bun run format:check && bun test && bun run check",
     "commit": "cz",
     "build": "bun run scripts/build.ts",

tests/engine/git-files.test.ts

@@ -59,6 +59,8 @@ describe("git-files", () => {
 
     test("returns both staged and unstaged changes", async () => {
       await Bun.$`git init`.cwd(tempDir).quiet();
+      await Bun.$`git config user.email "test@test.com"`.cwd(tempDir).quiet();
+      await Bun.$`git config user.name "Test"`.cwd(tempDir).quiet();
       writeFileSync(join(tempDir, "a.ts"), "export const a = 1;");
       await Bun.$`git add a.ts`.cwd(tempDir).quiet();
       await Bun.$`git commit -m "init"`.cwd(tempDir).quiet();

tests/mcp/resources.test.ts

@@ -7,23 +7,24 @@ import { registerResources } from "../../src/mcp/resources";
 
 describe("registerResources", () => {
   let tempDir: string;
+  let server: McpServer;
 
   beforeEach(() => {
     tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-res-test-"));
     mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true });
+    server = new McpServer({ name: "test", version: "0.0.0" });
   });
 
-  afterEach(() => {
+  afterEach(async () => {
+    await server.close();
     rmSync(tempDir, { recursive: true, force: true });
   });
 
   test("does not throw when registering resources", () => {
-    const server = new McpServer({ name: "test", version: "0.0.0" });
     expect(() => registerResources(server, tempDir)).not.toThrow();
   });
 
   test("registers the adr resource template", () => {
-    const server = new McpServer({ name: "test", version: "0.0.0" });
     registerResources(server, tempDir);
     // If registration succeeded without throwing, the resource is registered
     expect(true).toBe(true);

tests/mcp/server.test.ts

@@ -2,28 +2,30 @@ import { describe, expect, test, beforeEach, afterEach } from "bun:test";
 import { mkdtempSync, rmSync, mkdirSync } from "node:fs";
 import { join } from "node:path";
 import { tmpdir } from "node:os";
+import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
 import { createMcpServer } from "../../src/mcp/server";
 
 describe("createMcpServer", () => {
   let tempDir: string;
+  let server: McpServer;
 
   beforeEach(() => {
     tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-server-test-"));
     mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true });
+    server = createMcpServer(tempDir);
   });
 
-  afterEach(() => {
+  afterEach(async () => {
+    await server.close();
     rmSync(tempDir, { recursive: true, force: true });
   });
 
   test("returns an McpServer instance", () => {
-    const server = createMcpServer(tempDir);
     expect(server).toBeDefined();
     expect(typeof server.connect).toBe("function");
   });
 
   test("server has tool and resource registration methods", () => {
-    const server = createMcpServer(tempDir);
     expect(typeof server.registerTool).toBe("function");
     expect(typeof server.registerResource).toBe("function");
   });

tests/mcp/tools.test.ts

@@ -7,23 +7,24 @@ import { registerTools } from "../../src/mcp/tools/index";
 
 describe("registerTools", () => {
   let tempDir: string;
+  let server: McpServer;
 
   beforeEach(() => {
     tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-tools-test-"));
     mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true });
+    server = new McpServer({ name: "test", version: "0.0.0" });
   });
 
-  afterEach(() => {
+  afterEach(async () => {
+    await server.close();
     rmSync(tempDir, { recursive: true, force: true });
   });
 
   test("does not throw when registering tools", () => {
-    const server = new McpServer({ name: "test", version: "0.0.0" });
     expect(() => registerTools(server, tempDir)).not.toThrow();
   });
 
   test("registers all expected tools", () => {
-    const server = new McpServer({ name: "test", version: "0.0.0" });
     const registerSpy = spyOn(server, "registerTool");
     registerTools(server, tempDir);
     // The tools module registers 4 tools: check, list_adrs, review_context, session_context