fix(protocol): unify plan/message write paths through contributeOperation (#228)

src/cli/commands/inbox.test.ts

@@ -0,0 +1,243 @@
+/**
+ * Tests for `grove inbox` command — specifically the send path's
+ * contract enforcement (Codex adversarial review finding #3).
+ *
+ * Before the fix, `grove inbox send` fed sendMessageAsDiscussion a
+ * bare OperationDeps with no contract and no handoff store, so
+ * GROVE.md role-kind rules did not apply to CLI message sends —
+ * agents could bypass allowed_kinds=['work'] via the CLI even when
+ * the same operation was blocked via MCP.
+ *
+ * These tests run the actual handleInbox function (not an internal
+ * helper) so they exercise the full command bootstrap path: locate
+ * .grove, open SQLite stores, read GROVE.md, wrap with
+ * EnforcingContributionStore, build OperationDeps, call
+ * sendMessageAsDiscussion.
+ */
+
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import { mkdir, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { initSqliteDb, SqliteContributionStore } from "../../local/sqlite-store.js";
+import { handleInbox } from "./inbox.js";
+import type { InitOptions } from "./init.js";
+import { executeInit } from "./init.js";
+
+async function createTempDir(): Promise<string> {
+  const dir = join(
+    tmpdir(),
+    `grove-inbox-test-${Date.now()}-${Math.random().toString(36).slice(2)}`,
+  );
+  await mkdir(dir, { recursive: true });
+  return dir;
+}
+
+function makeInitOptions(cwd: string): InitOptions {
+  return {
+    name: "test-grove",
+    mode: "evaluation",
+    seed: [],
+    metric: [],
+    force: false,
+    agentOverrides: { agentId: "test-agent" },
+    cwd,
+  };
+}
+
+/**
+ * Capture console.error / console.log / process.exitCode for the
+ * duration of a handleInbox call so we can observe the command's
+ * side effects without mocking.
+ */
+async function runInbox(
+  args: readonly string[],
+  groveOverride: string,
+): Promise<{ stdout: string[]; stderr: string[]; exitCode: number | undefined }> {
+  const stdout: string[] = [];
+  const stderr: string[] = [];
+  const origLog = console.log;
+  const origError = console.error;
+  const origExitCode = process.exitCode;
+  process.exitCode = undefined;
+  console.log = (...args) => {
+    stdout.push(args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" "));
+  };
+  console.error = (...args) => {
+    stderr.push(args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" "));
+  };
+  try {
+    await handleInbox(args, groveOverride);
+  } finally {
+    console.log = origLog;
+    console.error = origError;
+  }
+  const exitCode = process.exitCode;
+  process.exitCode = origExitCode;
+  return { stdout, stderr, exitCode };
+}
+
+describe("grove inbox send — contract enforcement (Codex finding #3)", () => {
+  let dir: string;
+
+  beforeEach(async () => {
+    dir = await createTempDir();
+  });
+
+  afterEach(async () => {
+    await rm(dir, { recursive: true, force: true });
+  });
+
+  test("succeeds when GROVE.md does not exist (no contract, no enforcement)", async () => {
+    await executeInit(makeInitOptions(dir));
+    // Overwrite the default GROVE.md with nothing by deleting it.
+    await rm(join(dir, "GROVE.md"), { force: true });
+
+    const { exitCode, stdout } = await runInbox(
+      ["send", "plain body", "--to", "@reviewer", "--agent-id", "alice", "--json"],
+      join(dir, ".grove"),
+    );
+    expect(exitCode).not.toBe(1);
+    expect(stdout.join("\n")).toMatch(/"cid"\s*:/);
+  });
+
+  test("succeeds when contract allows discussion kind", async () => {
+    await executeInit(makeInitOptions(dir));
+    // Contract with no agent_constraints — everything allowed.
+    await writeFile(
+      join(dir, "GROVE.md"),
+      `---
+contract_version: 3
+name: test-grove
+mode: exploration
+---
+`,
+      "utf-8",
+    );
+
+    const { exitCode } = await runInbox(
+      ["send", "allowed body", "--to", "@reviewer", "--agent-id", "alice", "--json"],
+      join(dir, ".grove"),
+    );
+    expect(exitCode).not.toBe(1);
+
+    // Verify the contribution actually landed.
+    const db = initSqliteDb(join(dir, ".grove", "grove.db"));
+    const store = new SqliteContributionStore(db);
+    try {
+      const all = await store.list({ kind: "discussion" });
+      expect(all.length).toBeGreaterThanOrEqual(1);
+      expect(all.some((c) => c.context?.message_body === "allowed body")).toBe(true);
+    } finally {
+      store.close();
+    }
+  });
+
+  // -------------------------------------------------------------------------
+  // Codex round 3 finding #1: malformed GROVE.md must fail closed.
+  //
+  // An earlier version of this patch wrapped readFile + parseGroveContract
+  // in one broad try/catch. A YAML syntax error was indistinguishable from
+  // "file does not exist", so a broken contract silently fell through to
+  // the unenforced path — reopening the exact bypass the enforcement fix
+  // was supposed to close.
+  //
+  // Fix: only swallow ENOENT; let parse errors propagate. This test writes
+  // a GROVE.md with invalid YAML and asserts handleInbox rejects the send
+  // with an error (not a silent success).
+  // -------------------------------------------------------------------------
+  test("fails closed when GROVE.md is malformed (YAML parse error)", async () => {
+    await executeInit(makeInitOptions(dir));
+    // Malformed YAML frontmatter — unclosed bracket, invalid structure.
+    await writeFile(
+      join(dir, "GROVE.md"),
+      `---
+contract_version: 3
+name: test-grove
+mode: evaluation
+agent_constraints:
+  allowed_kinds: [work
+---
+`,
+      "utf-8",
+    );
+
+    // handleInbox re-throws parse errors through to the CLI dispatcher;
+    // the test wrapper captures uncaught rejections so we assert on that.
+    let caughtError: unknown;
+    try {
+      await runInbox(
+        ["send", "should-fail", "--to", "@reviewer", "--agent-id", "alice"],
+        join(dir, ".grove"),
+      );
+    } catch (err) {
+      caughtError = err;
+    }
+
+    // Either the command threw synchronously OR exited non-zero with an
+    // error visible. In both cases: no discussion contribution landed.
+    const db = initSqliteDb(join(dir, ".grove", "grove.db"));
+    const store = new SqliteContributionStore(db);
+    try {
+      const discussions = await store.list({ kind: "discussion" });
+      expect(discussions).toHaveLength(0);
+    } finally {
+      store.close();
+    }
+
+    // And we surfaced *some* signal of failure (thrown or exit=1).
+    // The important property is: we did NOT silently succeed with no
+    // enforcement — which is what the previous code did.
+    expect(caughtError).toBeDefined();
+  });
+
+  // -------------------------------------------------------------------------
+  // Codex round 2 finding #3: contract enforcement must apply to
+  // the CLI inbox send path, not only the MCP path.
+  // -------------------------------------------------------------------------
+  test("rejects when contract restricts allowed_kinds to ['work']", async () => {
+    await executeInit(makeInitOptions(dir));
+    // Contract that blocks discussion contributions.
+    await writeFile(
+      join(dir, "GROVE.md"),
+      `---
+contract_version: 3
+name: test-grove
+mode: evaluation
+agent_constraints:
+  allowed_kinds: [work]
+---
+`,
+      "utf-8",
+    );
+
+    const { exitCode, stderr } = await runInbox(
+      [
+        "send",
+        "should-be-blocked",
+        "--to",
+        "@reviewer",
+        "--agent-id",
+        "alice",
+        "--agent-name",
+        "Alice",
+      ],
+      join(dir, ".grove"),
+    );
+
+    // Must fail with a non-zero exit.
+    expect(exitCode).toBe(1);
+    // Must surface the role_kind violation to stderr.
+    expect(stderr.join("\n")).toMatch(/not allowed to submit kind 'discussion'/);
+
+    // And crucially: NO discussion contribution should land in the DAG.
+    const db = initSqliteDb(join(dir, ".grove", "grove.db"));
+    const store = new SqliteContributionStore(db);
+    try {
+      const discussions = await store.list({ kind: "discussion" });
+      expect(discussions).toHaveLength(0);
+    } finally {
+      store.close();
+    }
+  });
+});

src/cli/commands/inbox.ts

@@ -6,12 +6,13 @@
  *   grove inbox read [--from <agent-id>] [--since <iso>] [--limit <n>] [--json]
  */
 
+import { readFile } from "node:fs/promises";
+import { join } from "node:path";
 import { parseArgs } from "node:util";
-import { createContribution } from "../../core/manifest.js";
-import type { ContributionInput } from "../../core/models.js";
 import type { AgentOverrides } from "../../core/operations/agent.js";
 import { resolveAgent } from "../../core/operations/agent.js";
-import { readInbox, sendMessage } from "../../core/operations/messaging.js";
+import type { OperationDeps } from "../../core/operations/deps.js";
+import { readInbox, sendMessageAsDiscussion } from "../../core/operations/messaging.js";
 import { formatTable, formatTimestamp, outputJson } from "../format.js";
 
 // ---------------------------------------------------------------------------
@@ -64,40 +65,103 @@ async function handleSend(args: readonly string[], groveOverride?: string): Prom
     return;
   }
 
-  const { initCliDeps } = await import("../context.js");
-  const deps = initCliDeps(process.cwd(), groveOverride);
+  // Mirror the `grove discuss` bootstrap so the CLI send path goes through
+  // the same contract-enforcement pipeline as the MCP path. Previously this
+  // command wrapped a bare `CliDeps` which carried no contract, so GROVE.md
+  // role-kind rules and rate limits silently did not apply to inbox sends —
+  // a loophole the MCP fix didn't close.
+  const { resolveGroveDir } = await import("../utils/grove-dir.js");
+  const { createSqliteStores } = await import("../../local/sqlite-store.js");
+  const { FsCas } = await import("../../local/fs-cas.js");
+  const { DefaultFrontierCalculator } = await import("../../core/frontier.js");
+  const { parseGroveContract } = await import("../../core/contract.js");
+  const { EnforcingContributionStore } = await import("../../core/enforcing-store.js");
+
+  const { groveDir, dbPath } = resolveGroveDir(groveOverride);
+  const stores = createSqliteStores(dbPath);
+  const cas = new FsCas(join(groveDir, "cas"));
+  const frontier = new DefaultFrontierCalculator(stores.contributionStore);
+
+  // Load GROVE.md from the grove root (parent of .grove/).
+  //
+  // Separate the readFile catch from the parse call so a MALFORMED contract
+  // fails closed instead of silently falling through to unenforced mode.
+  // The first pass of this patch combined both into one try/catch, which
+  // meant a YAML syntax error would be indistinguishable from "file does
+  // not exist" — any broken contract file reopened the CLI bypass we're
+  // supposed to be closing.
+  //
+  // ENOENT is the only acceptable fallthrough. Everything else (parse
+  // errors, permission denied, schema validation) propagates to the
+  // outer error handler so the operator sees the failure.
+  const groveRoot = join(groveDir, "..");
+  const grovemdPath = join(groveRoot, "GROVE.md");
+  let contract: Awaited<ReturnType<typeof parseGroveContract>> | undefined;
+  let grovemdContent: string | undefined;
+  try {
+    grovemdContent = await readFile(grovemdPath, "utf-8");
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException)?.code;
+    if (code !== "ENOENT") {
+      // Permission error, I/O error, etc. — surface loudly.
+      throw err;
+    }
+    // GROVE.md does not exist — proceed without enforcement, same as
+    // `grove discuss` in a grove without a contract.
+  }
+  if (grovemdContent !== undefined) {
+    // parseGroveContract intentionally runs OUTSIDE the catch: YAML/schema
+    // errors must propagate, not be swallowed as "no contract".
+    contract = parseGroveContract(grovemdContent);
+  }
+
+  // Wrap with EnforcingContributionStore when a contract exists. Without
+  // this, rate-limits / allowed-kinds / clock-skew checks would not fire
+  // for inbox sends even when configured in GROVE.md.
+  const contributionStore = contract
+    ? new EnforcingContributionStore(stores.contributionStore, contract, { cas })
+    : stores.contributionStore;
 
   try {
     const agentOverrides: AgentOverrides = {
       agentId: values["agent-id"] as string | undefined,
       agentName: values["agent-name"] as string | undefined,
     };
-    const agent = resolveAgent(agentOverrides);
 
-    const computeCid = (input: ContributionInput): string => {
-      return createContribution(input).cid;
+    const opDeps: OperationDeps = {
+      contributionStore,
+      claimStore: stores.claimStore,
+      cas,
+      frontier,
+      handoffStore: stores.handoffStore,
+      ...(contract !== undefined ? { contract } : {}),
     };
 
-    const result = await sendMessage(
-      deps.store,
+    const result = await sendMessageAsDiscussion(
       {
-        agent,
+        agent: agentOverrides,
         body,
         recipients,
-        inReplyTo: values["reply-to"] as string | undefined,
+        ...(values["reply-to"] !== undefined ? { inReplyTo: values["reply-to"] as string } : {}),
         tags: (values.tag ?? []) as string[],
       },
-      computeCid,
+      opDeps,
     );
 
+    if (!result.ok) {
+      console.error(`Error: ${result.error.message}`);
+      process.exitCode = 1;
+      return;
+    }
+
     if (values.json) {
-      outputJson({ cid: result.cid, recipients, body });
+      outputJson({ cid: result.value.cid, recipients, body });
     } else {
-      console.log(`Message sent: ${result.cid}`);
+      console.log(`Message sent: ${result.value.cid}`);
       console.log(`  to: ${recipients.join(", ")}`);
     }
   } finally {
-    deps.close();
+    stores.contributionStore.close();
   }
 }