Fix 38 system audit findings (clean)

[rohitg00]

Summary

• Fixes 38 audit findings (1 critical, 12 high, 16 medium, 9 low) across security, race conditions, performance, correctness, and dead code
• Creates src/version.ts (single VERSION constant replacing 11 hardcoded strings) and src/auth.ts (timing-safe HMAC auth + shared CSP)
• Adds keyed mutex (src/state/keyed-mutex.ts) for race condition protection in remember, relations, and observe functions

Changes (22 files)

• Security: Timing-safe HMAC auth in api.ts, mcp/server.ts; snapshot commitHash validation; HMAC comparison eliminates length leak
• Race conditions: Session-scoped keyed locks in observe, remember, relations
• Input validation: Governance bulk-delete filter guard, team itemType whitelist, maxObservationsPerSession enforcement
• Bugs: Schema version union (0.3.0|0.4.0), GraphNode properties Record<string, unknown>, standalone MCP tools/list filter, future-proof import version validation
• Write ordering: Supersede old memory before saving new to prevent dual-latest on partial failure
• Config: Circuit breaker configurable thresholds with positiveFinite validation, health monitor interval.unref()
• Cleanup: Telemetry dead exports removed, 3 new secret patterns (npm_, glpat-, dop_v1_), CSP header constant

Test plan

• npm run build passes
• All 216 tests pass
• No regressions in existing functionality

Summary by CodeRabbit

• New Features

  • Support for additional export/import data versions (0.3.0 and 0.4.0).
  • Configurable circuit breaker parameters for resilience tuning.
  • Expanded privacy pattern detection for npm, GitLab, and Doppler tokens.

• Bug Fixes

  • Fixed graph query errors with non-string property values.
  • Improved authentication security with constant-time comparisons.

• Improvements

  • Enhanced validation for governance and team operations.
  • Stricter commit hash validation for snapshots.
  • Centralized version management throughout the application.

[coderabbitai]

Warning

Rate limit exceeded

@rohitg00 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 29 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4b8a01a0-1249-4e59-87da-0877dd23bfa3

📥 Commits

Reviewing files that changed from the base of the PR and between b1cb9a1 and 99f8249.

📒 Files selected for processing (22)

• src/auth.ts
• src/eval/schemas.ts
• src/functions/export-import.ts
• src/functions/governance.ts
• src/functions/graph.ts
• src/functions/observe.ts
• src/functions/privacy.ts
• src/functions/relations.ts
• src/functions/remember.ts
• src/functions/snapshot.ts
• src/functions/team.ts
• src/health/monitor.ts
• src/index.ts
• src/mcp/server.ts
• src/mcp/standalone.ts
• src/providers/circuit-breaker.ts
• src/state/keyed-mutex.ts
• src/telemetry/setup.ts
• src/triggers/api.ts
• src/types.ts
• src/version.ts
• test/snapshot.test.ts

📝 Walkthrough

Walkthrough

This PR introduces version centralization via a new VERSION constant, implements per-key concurrency control through a keyed-mutex utility, hardens authentication with timing-safe comparisons, adds schema support for version 0.4.0, expands secret redaction patterns, and makes type and validation improvements across multiple functions.

Changes

Cohort / File(s)	Summary
Authentication & Security src/auth.ts, src/privacy.ts, src/mcp/server.ts, src/triggers/api.ts	Introduces timing-safe string comparison utility, replaces direct equality checks with constant-time comparisons for auth validation, centralizes VIEWER_CSP constant, expands SECRET_PATTERN_SOURCES with npm, GitLab, and Doppler token patterns.
Version Management src/version.ts, src/index.ts, src/eval/schemas.ts, src/functions/export-import.ts, src/functions/snapshot.ts, src/mcp/standalone.ts, src/telemetry/setup.ts, src/triggers/api.ts	Introduces VERSION constant (supporting 0.3.0 and 0.4.0), centralizes version references across modules, updates schema to accept both versions, and removes hardcoded version strings.
Concurrency Control src/state/keyed-mutex.ts, src/functions/observe.ts, src/functions/relations.ts, src/functions/remember.ts	Adds new withKeyedLock utility for per-key serialization, applies locks to observation storage, memory relation creation, and memory recording to prevent concurrent state mutations; adds maxObservationsPerSession limit.
Schema & Type Updates src/types.ts, src/eval/schemas.ts	Broadens GraphNode.properties from Record<string, string> to Record<string, unknown>, allowing non-string property values; updates export schema to accept version 0.4.0.
Function Improvements src/functions/graph.ts, src/functions/governance.ts, src/functions/team.ts	Adds type guard for string properties in graph queries to prevent runtime errors, adds validation guard for governance bulk operations requiring filters, adds itemType validation for team shares.
Provider & Utility Updates src/providers/circuit-breaker.ts, src/health/monitor.ts, src/functions/snapshot.ts	Refactors circuit breaker with configurable options interface, allows Node process exit via interval.unref(), adds commit hash format validation for snapshot restoration.
Telemetry src/telemetry/setup.ts	Removes public getCounters/getHistograms exports, centralizes version via imported VERSION constant.
Tests test/snapshot.test.ts	Updates mock return values and test assertions to reflect new 7-character commit hash format (abc1234) and JSON serialization changes.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Function as observe/relations/<br/>remember Function
    participant KeyedLock as withKeyedLock
    participant StateKV as State/KV
    participant Lock as Per-Key<br/>Promise Lock

    Client->>Function: Request
    Function->>KeyedLock: withKeyedLock(key, operation)
    KeyedLock->>Lock: Enqueue operation<br/>for this key
    Lock->>Lock: Await prior<br/>key-specific promise
    KeyedLock->>StateKV: Validate & Update<br/>(inside lock)
    StateKV-->>KeyedLock: Result
    KeyedLock->>Lock: Cleanup & Dequeue
    KeyedLock-->>Function: Return result
    Function-->>Client: Response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Health monitor interval not unref'd — keeps process alive #45 — Both PRs add interval.unref() to the health monitor to prevent the timer from keeping the Node process alive.
• ExportImportInputSchema rejects current v0.4.0 exports #17 — Both PRs address the schema and export-import version handling to support version 0.4.0 alongside 0.3.0.

Possibly related PRs

• feat: agentmemory v0.4.0 — the memory layer for all AI coding agents #9 — Implements overlapping v0.4.0 features including graph, snapshots, team sharing, governance, export/import versioning, keyed-mutex, MCP updates, and telemetry changes.
• fix: comprehensive security and reliability audit #6 — Modifies src/triggers/api.ts to harden viewer and auth handling with stricter CSP and timing-safe comparison logic.
• feat: agentmemory v0.4.0 — MCP resources, prompts, enrichment, confidence #7 — Modifies src/functions/relations.ts to add keyed-lock concurrency control, which interacts with the memory relation creation flow updated in this PR.

Poem

🐰 A rabbit hops through locks so keen,
Timing-safe and ever serene—
Versions centralized with care,
Keys and secrets guarded fair,
v0.4.0 blooms everywhere! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix 38 system audit findings (clean)' accurately summarizes the main objective of the changeset, which is resolving 38 audit findings across multiple categories.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/system-audit-clean

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/functions/export-import.ts (1)

105-106: Keep import compatibility explicit, not implicitly coupled to current runtime version.

Including VERSION in the allowlist can auto-admit future formats before migration logic is defined. Prefer an explicit compatibility list.

♻️ Suggested change

-      const supportedVersions = new Set(["0.3.0", "0.4.0", VERSION]);
+      const supportedVersions = new Set<ExportData["version"]>([
+        "0.3.0",
+        "0.4.0",
+      ]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/functions/export-import.ts` around lines 105 - 106, Remove the implicit
coupling to the current runtime by not including the global VERSION in the
allowlist; update the supportedVersions Set (the variable supportedVersions) to
contain only explicitly-approved schema strings (e.g., "0.3.0", "0.4.0") and
ensure the import check that uses importData.version continues to reject unknown
versions; when you add migration/compatibility logic in the future, explicitly
add new version strings to supportedVersions rather than re-adding VERSION.

src/types.ts (1)

288-288: Prefer JSON-serializable typing over unconstrained unknown.

Record<string, unknown> is very broad for persisted/exported graph payloads. A JSON value type keeps flexibility while preserving storage/export safety.

♻️ Suggested typing refinement

+export type JsonPrimitive = string | number | boolean | null;
+export type JsonValue = JsonPrimitive | JsonValue[] | { [key: string]: JsonValue };
+
 export interface GraphNode {
   id: string;
@@
-  properties: Record<string, unknown>;
+  properties: Record<string, JsonValue>;
   sourceObservationIds: string[];
   createdAt: string;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` at line 288, The field named properties currently typed as
Record<string, unknown> should be changed to a JSON-serializable type to
guarantee persisted/exported graph payloads are safe; replace Record<string,
unknown> with Record<string, JSONValue> (or JSONValue for non-object cases) and
add/export a JSONValue union/type (e.g., type JSONValue = string | number |
boolean | null | JSONObject | JSONArray; type JSONObject = Record<string,
JSONValue>; type JSONArray = JSONValue[] ) so all values are constrained to
JSON-serializable primitives/objects/arrays and preserve backward compatibility
for storage and export operations.

src/mcp/standalone.ts (1)

9-15: Consider deriving IMPLEMENTED_TOOLS from dispatch handlers to avoid drift.

This set is now a second source of truth versus handleToolCall; a small handler map refactor would keep them auto-synced.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp/standalone.ts` around lines 9 - 15, IMPLEMENTED_TOOLS is a hard-coded
second source of truth; instead derive it from the dispatch/handler registry to
avoid drift by building the set from the keys of the handler map used by
handleToolCall (or whichever dispatch function registers handlers). Change code
to export or read the unified handler map (e.g., the map/object that maps tool
names to handler functions used by handleToolCall) and compute IMPLEMENTED_TOOLS
= new Set(Object.keys(handlerMap)) (or equivalent) so the set is always in sync
with the handlers.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/functions/observe.ts`:
- Around line 90-99: Session-limit rejection can leave dedup state recorded even
though no observation was persisted; ensure dedup hashes are only written after
successful persistence inside the withKeyedLock(`obs:${payload.sessionId}`)
block. Move any dedup recording logic (where the observation hash is stored) out
of pre-check code and into the post-success path after kv.put/kv.putMany (or
whatever persistence method is used), or if you prefer to keep current ordering,
roll back/remove the dedup entry when returning the limit error in the
withKeyedLock branch; reference withKeyedLock, payload.sessionId,
maxObservationsPerSession, kv.list and KV.observations to locate and adjust the
code paths.

In `@src/functions/relations.ts`:
- Around line 50-85: The current single pair lock (lockKey) only serializes
identical pairs and allows concurrent mutations to the same memory (e.g.,
relate(A,B) vs relate(A,C)); fix by acquiring per-memory locks in a
deterministic order: replace the single withKeyedLock(lockKey, ...) call with
nested withKeyedLock calls on the two memory IDs sorted (e.g., await
withKeyedLock(sortedIds[0], async () => await withKeyedLock(sortedIds[1], async
() => { ... }))). Update the block that builds lockKey and the withKeyedLock
call so both memory IDs are locked individually (use data.sourceId and
data.targetId sorted) before reading/updating kv.get(KV.memories, ...), pushing
to relatedIds, and kv.set(...).

In `@src/functions/team.ts`:
- Around line 13-14: The code currently excludes the "observation" item type
causing valid callers to be rejected; update VALID_ITEM_TYPES (and any checks
around it in src/functions/team.ts) to include "observation" so it matches
TeamSharedItem.type and the tool schema, and then adjust the content-read logic
that branches on itemType (used around the item lookup code referenced in lines
~31-33) to route observation IDs to KV.memories (i.e., when itemType ===
"observation" use the KV.memories path) to avoid false "Item not found" errors.

---

Nitpick comments:
In `@src/functions/export-import.ts`:
- Around line 105-106: Remove the implicit coupling to the current runtime by
not including the global VERSION in the allowlist; update the supportedVersions
Set (the variable supportedVersions) to contain only explicitly-approved schema
strings (e.g., "0.3.0", "0.4.0") and ensure the import check that uses
importData.version continues to reject unknown versions; when you add
migration/compatibility logic in the future, explicitly add new version strings
to supportedVersions rather than re-adding VERSION.

In `@src/mcp/standalone.ts`:
- Around line 9-15: IMPLEMENTED_TOOLS is a hard-coded second source of truth;
instead derive it from the dispatch/handler registry to avoid drift by building
the set from the keys of the handler map used by handleToolCall (or whichever
dispatch function registers handlers). Change code to export or read the unified
handler map (e.g., the map/object that maps tool names to handler functions used
by handleToolCall) and compute IMPLEMENTED_TOOLS = new
Set(Object.keys(handlerMap)) (or equivalent) so the set is always in sync with
the handlers.

In `@src/types.ts`:
- Line 288: The field named properties currently typed as Record<string,
unknown> should be changed to a JSON-serializable type to guarantee
persisted/exported graph payloads are safe; replace Record<string, unknown> with
Record<string, JSONValue> (or JSONValue for non-object cases) and add/export a
JSONValue union/type (e.g., type JSONValue = string | number | boolean | null |
JSONObject | JSONArray; type JSONObject = Record<string, JSONValue>; type
JSONArray = JSONValue[] ) so all values are constrained to JSON-serializable
primitives/objects/arrays and preserve backward compatibility for storage and
export operations.

---

ℹ️ Review info

Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 17277155-2d2c-4057-892c-f1320b7a5819

📥 Commits

Reviewing files that changed from the base of the PR and between e2d8e70 and b1cb9a1.

📒 Files selected for processing (22)

• src/auth.ts
• src/eval/schemas.ts
• src/functions/export-import.ts
• src/functions/governance.ts
• src/functions/graph.ts
• src/functions/observe.ts
• src/functions/privacy.ts
• src/functions/relations.ts
• src/functions/remember.ts
• src/functions/snapshot.ts
• src/functions/team.ts
• src/health/monitor.ts
• src/index.ts
• src/mcp/server.ts
• src/mcp/standalone.ts
• src/providers/circuit-breaker.ts
• src/state/keyed-mutex.ts
• src/telemetry/setup.ts
• src/triggers/api.ts
• src/types.ts
• src/version.ts
• test/snapshot.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Allow observation to match the declared/shared contract.

Line 13 and Line 31 currently reject observation, but TeamSharedItem.type and tool schema both include it. This is a behavior break for valid callers.

Suggested fix

-const VALID_ITEM_TYPES = new Set(["memory", "pattern"]);
+const VALID_ITEM_TYPES: ReadonlySet<TeamSharedItem["type"]> = new Set([
+  "observation",
+  "memory",
+  "pattern",
+]);

@@
-    async (data: {
+    async (data: {
       itemId: string;
-      itemType: "memory" | "pattern";
+      itemType: TeamSharedItem["type"];
       project?: string;
     }) => {

Also note: once observation is accepted again, the content read path (currently KV.memories) should be selected by itemType to avoid false “Item not found” for observation IDs.

Also applies to: 31-33

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/functions/team.ts` around lines 13 - 14, The code currently excludes the
"observation" item type causing valid callers to be rejected; update
VALID_ITEM_TYPES (and any checks around it in src/functions/team.ts) to include
"observation" so it matches TeamSharedItem.type and the tool schema, and then
adjust the content-read logic that branches on itemType (used around the item
lookup code referenced in lines ~31-33) to route observation IDs to KV.memories
(i.e., when itemType === "observation" use the KV.memories path) to avoid false
"Item not found" errors.