From 82595ec557056bd1e573512eb241c042e0fcbf0d Mon Sep 17 00:00:00 2001 From: rUv Date: Mon, 14 Sep 2026 22:20:33 +0000 Subject: [PATCH] dream(security): #3151 sliding-window reset for maxToolCallsPerTurn (evaluated, ACCEPT) (#3152) * dream(security): #3151 sliding-window reset for maxToolCallsPerTurn (evaluated, ACCEPT) Follow-up to PR #3139 (2026-08-31, unmerged): the opt-in MCP policy enforcer's maxToolCallsPerTurn was a permanent session-lifetime cumulative counter that never reset, explicitly disclosed as unsafe to enable by default. Tonight's research (5 parallel roles) converged on a wall-clock sliding window as the correct fix, corroborated by the MCP spec's own silence on rate-limit mechanism, SEP-2567's removal of the session concept, Cloudflare's production sliding-window-counter data, and how FastMCP/PolicyLayer solve the same problem in the MCP ecosystem. - checkAndRecordCall/evaluateToolCall now take an injectable `now` (defaults to Date.now(), zero production behavior change at the one real call site in mcp-server.ts). - New McpPolicy.turnWindowMs field (default 60000ms); shipped in .harness/mcp-policy.json alongside the existing maxToolCallsPerTurn. - 30/30 tests passing (24 pre-existing unmodified + 6 new). Broader MCP-suite sweep: 52/56 (4 failures are pre-existing unbuilt-sibling- package environmental gaps, unrelated to this diff). - Independent adversarial critique: CONFIRMED-WITH-CAVEATS. Two minor findings (untested exact window boundary; audit-log timestamp cosmetic desync from the injected clock) fixed before this commit. Full report: docs/dream-cycle/dream-gist-2026-09-01.md * dream(security): #3151 update LEDGER.md for 2026-09-01 Ledger row for tonight's dream-cycle: sliding-window reset for maxToolCallsPerTurn (PR #3152, stacked on #3139). * dream(security): #3151 address ruvnet's PR #3152 review (round 1) Retargeted PR #3152's base to main: dream/2026-08-31-security is not main/develop, so every required CI workflow (ci.yml, integration-tests.yml, cve-audit.yml, v3-ci.yml, verification-pipeline.yml) is configured to trigger only on PRs targeting main/develop and structurally never ran on either #3139 or this stacked PR's head, regardless of what was pushed. This is a repo-wide trigger-configuration fact, not fixable by a commit. Also addresses the review's other concrete asks: - Built the two sibling packages disclosed as unbuilt (@claude-flow/cli-core, @claude-flow/neural) via tsc. Broader MCP-suite sweep improves from 52/56 to 201/204 -- remaining 3 failures confirmed (independently, by testing a raw ::1 bind outside any test) to be EAFNOSUPPORT: IPv6 loopback binding unsupported at the container/OS level in this sandbox, unrelated to this diff and not fixable by any code change here. - Added an integration test exercising the real stdio dispatcher (MCPServerManager.handleMCPMessage) through a full deny -> expiry -> allow cycle with fake timers, proving the fix holds without bypassing audit logging or the opt-in flag -- the review's specific ask, previously only covered at the direct evaluateToolCall() call level. - Strengthened documentation (McpPolicy.maxToolCallsPerTurn JSDoc and .harness/mcp-policy.json's own field comment): explicit that despite the field name this is wall-clock rate limiting, not a literal per-conversational-turn counter -- MCP has no protocol-level turn concept. 31/31 policy-enforcer tests passing (was 30/30; +1 dispatcher-level test). --------- Co-authored-by: Claude --- .harness/mcp-policy.json | 4 +- docs/dream-cycle/LEDGER.md | 1 + docs/dream-cycle/dream-gist-2026-09-01.md | 74 +++++++++++ .../cli/__tests__/mcp-policy-enforcer.test.ts | 117 +++++++++++++++++- .../cli/src/mcp-tools/policy-enforcer.ts | 85 ++++++++++--- 5 files changed, 261 insertions(+), 20 deletions(-) create mode 100644 docs/dream-cycle/dream-gist-2026-09-01.md diff --git a/.harness/mcp-policy.json b/.harness/mcp-policy.json index 8800170722..8a5ce77df7 100644 --- a/.harness/mcp-policy.json +++ b/.harness/mcp-policy.json @@ -2,7 +2,7 @@ "$comment": "ADR-150 iter 30 — MCP governance policy for ruflo's claude-flow MCP server. Closes the 'no-policy' HIGH finding from `harness mcp-scan`. Configured to reflect ruflo's ACTUAL MCP-layer posture: the claude-flow MCP server exposes 314 tools that operate via higher-level abstractions (memory_*, hooks_*, agentdb_*) — NOT raw shell, network, or filesystem. Those primitives are reached via native Claude Code tools (Bash / Write / Edit / WebFetch) which are governed by Claude Code's own permission system, not this MCP policy. Hence: this policy denies all three at the MCP layer.", "schema": 1, - "policyVersion": 1, + "policyVersion": 2, "harnessId": "claude-flow", "rationale": "ADR-022 default-deny posture. The claude-flow MCP server's tools are pure-data operations (read/write via AgentDB, route via embeddings, dispatch via hooks). Shell / network / file-write are NOT in this server's tool surface — they happen at the native-Claude-Code layer, outside this policy's jurisdiction.", @@ -17,6 +17,8 @@ "toolTimeoutMs": 600000, "maxToolCallsPerTurn": 200, + "$comment_maxToolCallsPerTurn": "Despite the field name, this is a wall-clock rate limit (at most this many calls per rolling turnWindowMs), not a literal per-conversational-turn counter -- MCP has no protocol-level 'turn' concept to count against. Treat and document this as rate limiting to policy consumers.", + "turnWindowMs": 60000, "dangerousPatterns": [ "Bash\\(rm\\b", diff --git a/docs/dream-cycle/LEDGER.md b/docs/dream-cycle/LEDGER.md index eb1abfacd1..cece16a793 100644 --- a/docs/dream-cycle/LEDGER.md +++ b/docs/dream-cycle/LEDGER.md @@ -186,3 +186,4 @@ as a first-class, high-priority candidate, not a background note. **Note (2026-09-09):** 2026-09-08 (memory, #3231/#3232 — wired embedding near-dup detection into `MemoryConsolidator.dedup()`, ACCEPT, directly acting on 09-07's own recommendation #3) exists as a real branch+issue+draft-PR (verified via `search_pull_requests`, not inferred) but is not transcribed as a row here — same "ledger rows only reach `main` via PR merge" structural gap noted repeatedly since 2026-08-19/09-03/09-05. Verified GitHub state (via MCP tools, not `gh` CLI) for the trailing nights before tonight's own row: #3139 (08-31) OPEN/draft 9d old; #3152 (09-01) OPEN/draft 8d old; #3160 (09-02) OPEN/draft 7d old; #3169 (09-03) **MERGED** 09-05; #3184 (09-05) **MERGED** 09-05; #3221 (09-07) **MERGED** 09-07 (same day); #3232 (09-08) OPEN/draft 1d old. Swarm-surface backlog specifically: #3086 (08-24) now **16 days old, 0 follow-up comments → STALE** per the >14-day rule; #3123 (08-29) 11d old, has reviewer comments, not yet stale. Trailing-9-night merge rate has improved to 3/9 (09-03, 09-05, 09-07) vs. the "0 of last 14" note in earlier gists. | 2026-09-09 | swarm | AgentPool.performHealthChecks() stamped lastHeartbeat=now on every "still healthy" tick, and updateAgentHeartbeat() (the only other legitimate way to advance it) has zero callers anywhere in v3/ — so the check's own tick was the sole thing keeping every pooled agent's heartbeat fresh, meaning timeSinceLastActivity could never exceed one interval and replaceUnhealthyAgent() could never fire for any agent, in any pool, ever; removed the self-stamp | #3242 | #3243 | yes | ACCEPT-scoped | 223/223 tests (+3 new), baseline-fails-2-of-3/candidate-passes-3-of-3 via stash isolation; adversarial critic confirmed independently but disclosed @claude-flow/swarm has no reachable path through the shipped @claude-flow/cli today (blast radius is the swarm package's own UnifiedSwarmCoordinator, not an active CLI hot path) | 008c96a0... | 08-24..09-02 backlog unchanged (see note above); #3169/#3184/#3221 merged since 09-05 | +| 2026-09-01 | security | `maxToolCallsPerTurn` was session-lifetime cumulative, never reset (disclosed bug in #3139); wall-clock sliding window fix, new `turnWindowMs` policy field (follow-up to #3139, stacked PR) | #3151 | #3152 | yes | ACCEPT | 30/30 new-suite tests, 52/56 broader MCP sweep (4 pre-existing unbuilt-sibling-package env failures, unrelated); independent critic CONFIRMED-WITH-CAVEATS, 2 minor findings fixed pre-commit | 7d93a482... | rebased onto main (post #3086/#3094/#3103/#3110/#3119/#3123/#3130/#3139 merges), awaiting fresh CI as of 2026-09-14 | diff --git a/docs/dream-cycle/dream-gist-2026-09-01.md b/docs/dream-cycle/dream-gist-2026-09-01.md new file mode 100644 index 0000000000..d9571adab1 --- /dev/null +++ b/docs/dream-cycle/dream-gist-2026-09-01.md @@ -0,0 +1,74 @@ +# MCP Tool-Call Rate-Limit Reset Semantics SOTA Report — 2026-09-01 + +TL;DR: Ruflo's opt-in MCP governance policy enforcer (shipped 2026-08-31, PR #3139, still unmerged) tracked `maxToolCallsPerTurn` as a **session-lifetime cumulative counter that never resets** — a long legitimate session could get permanently locked out until the server process restarted. Tonight (2026-09-01) research across 5 parallel roles found convergent 2026 evidence that the right fix is a **wall-clock sliding window**, not a turn-count reset (which is gameable) and not a token bucket (wrong shape for a hard per-window ceiling). Implemented, tested (24 tests, 9 new), and pushed as a follow-up on top of #3139's branch. + +## What's New in 2026 + +| Finding | Source | Confidence | +|---|---|---| +| MCP spec mandates "rate limit tool invocations" with zero mechanism guidance | modelcontextprotocol.io/specification/2025-06-18/server/tools (direct read) | A | +| MCP's 2026-07-28 revision (SEP-2567) removes the protocol-level session concept entirely | SEP-2567 spec text (direct read) | A | +| Cloudflare's production sliding-window-counter rate limiter: 0.003% wrong-decision rate across 400M requests | Cloudflare rate-limiting engineering writeups | A | +| Google ADK's `RunConfig.max_llm_calls` is a session-lifetime cumulative counter with no documented reset — the same latent bug class | google.github.io/adk-docs; adk-python issue #3828 | B | +| FastMCP middleware and the PolicyLayer MCP firewall both anchor resets to wall-clock time (token-bucket / fixed-calendar-window), never to turn count | gofastmcp.com/python-sdk docs; policylayer.com/blog (direct quotes) | A | +| "Resilient Consensus in Agentic AI" (arXiv:2606.15024): unfiltered LLM-agent consensus fails classical resilient-consensus guarantees; explicit filter/weighting layers recover it | arXiv:2606.15024, controlled experiment | B | + +## Ruflo Current Capability + +`v3/@claude-flow/cli/src/mcp-tools/policy-enforcer.ts` (branch-only, unmerged as of tonight) enforces `auditLog` and `maxToolCallsPerTurn` for the stdio `tools/call` dispatch, fully opt-in via `RUFLO_MCP_ENFORCE_POLICY=1`, fail-closed on missing policy or failed audit writes (PR #3139 review round 1). Prior to tonight, `checkAndRecordCall` kept a `Map` that only incremented — once a session hit the shipped default of 200 calls, it stayed denied for the life of the stdio process. The module's own doc comment already disclosed this as unsafe to enable without a real reset. `.harness/mcp-policy.json` declared the field but no window/reset concept existed anywhere in the schema. + +## Competitor Comparison + +| Competitor | Rate-limits tool calls? | Reset semantics | Confidence | +|---|---|---|---| +| LangGraph | No built-in mechanism | N/A — `recursion_limit` halts the graph; rate limiting pushed to developer/proxy layer | B | +| OpenAI Agents SDK | Yes, hard cap | `max_turns` per `Runner.run()`; no in-run reset, no time component | A | +| Google ADK | Yes | `max_llm_calls`, session-lifetime cumulative, **no documented reset** — same bug class Ruflo had | B | +| CrewAI | Yes, pacing not budget | `max_rpm` — rolling-minute throttle, closer to leaky-bucket pacing | B | +| Anthropic MCP reference SDK / protocol | No | Deliberately deferred to transport/gateway layer by design | A | +| FastMCP (community MCP middleware) | Yes | Token bucket (default) or sliding window, continuously live, keyed per-client | A | +| PolicyLayer (MCP firewall) | Yes | Fixed calendar-aligned windows (UTC hour/day boundaries), persists across restarts | A | + +Synthesis: MCP itself has no rate-limiting concept by design — the convergence is happening *around* the protocol (gateway products), not in it. Among agent frameworks, none implement a real per-turn sliding window for tool calls; what ships is either a hard circuit-breaker, a cumulative counter that never resets (ADK — Ruflo wasn't an outlier, it matched a real shipped default elsewhere), or wall-clock pacing. The two purpose-built MCP rate limiters that got this right both anchor to wall-clock time specifically to avoid a turn-count-reset being gameable by a chatty loop re-arming its own budget. + +## Hypothesis + +> Given a stdio MCP session under Ruflo's opt-in `RUFLO_MCP_ENFORCE_POLICY` governance, when `maxToolCallsPerTurn` enforcement changes from a permanent session-lifetime cumulative counter to a wall-clock sliding window (new `turnWindowMs` policy field, default 60000ms), then a session that pauses beyond the window has its call budget restored, relative to today's baseline (permanent lockout once exhausted), subject to: (1) default/flag-unset behavior is byte-for-byte unchanged; (2) all existing policy-enforcer and MCP-related tests remain green; (3) fully deterministic coverage via an injectable clock, zero LLM cost; (4) no change required at the `mcp-server.ts` call site. + +Frozen before implementation; not modified after seeing results. + +## Benchmarks + +No benchmark corpus applies — this is a deterministic unit-level correctness fix (a counter either resets on schedule or it doesn't), not a quality/latency tradeoff evaluated against a task corpus. + +## Evaluation + +**evaluated: accepted.** Real evaluator: `vitest run`, deterministic, $0, zero LLM calls. + +- `__tests__/mcp-policy-enforcer.test.ts`: **30/30 passing** (24 pre-existing tests, unmodified, all pass identically since none crossed a window boundary; 6 new — 5 direct `checkAndRecordCall` sliding-window cases [limit+1 denial then restoration, partial roll-off proving a genuine sliding window rather than a periodic full clear, default-window fallback, invalid-window fallback, and an exact-boundary pin (`now === cutoff`) added after independent adversarial review flagged it as untested] + 1 `evaluateToolCall` integration case using `vi.useFakeTimers()`, the existing repo pattern from `__tests__/services/workspace-lease.test.ts`, proving the fail-closed pipeline respects the reset end-to-end). +- Broader regression sweep across all 11 MCP-related test files in the package: **52/56 passing**. The 4 failures trace entirely to two unbuilt sibling packages in this fresh checkout — `@claude-flow/cli-core/dist` and `@claude-flow/neural/dist` (confirmed missing via direct `test -d` before any candidate code was touched) — the same disclosed environmental-gap class as 2026-08-17's and 2026-08-31's nights. `git diff --stat` confirms tonight's diff touches exactly 3 source-adjacent files (`policy-enforcer.ts`, its test, `.harness/mcp-policy.json`) plus the pnpm lockfile from installing dependencies — none of the failing tests import any of them. +- One-time environment fix (not a candidate change, disclosed per the same convention as prior nights): `pnpm install` had never been run for the v3 workspace in this fresh checkout, and `@claude-flow/mcp` (imported only by the unrelated, pre-existing `startHttpServer()` method in the same `mcp-server.ts` file) was unbuilt, breaking Vite's static transform for every test importing that module — built cleanly via plain `tsc` before running tests. + +## Darwin Results + +Not run. This is a binary correctness fix (the counter resets correctly, or it doesn't) with one tunable numeric parameter (`turnWindowMs`) that is plausibly Darwin-eligible in principle (evolve against a synthetic call-pattern corpus to balance throttling effectiveness vs. false-lockout rate), but building a representative benchmark corpus for that tonight would expand scope well beyond "small, reviewable" — explicitly the bias STEP 1.1 calls for given zero of the last 14 dream-cycle PRs have merged. Flagged as a legitimate future-night candidate rather than attempted here. + +## SOTA Proof & Witness + +Independent adversarial critique (separate subagent, no authoring context): **CONFIRMED-WITH-CAVEATS**. Both real findings were minor and fixed before this report was finalized: (1) the exact window-boundary case (`now === cutoff`) was untested — added as a pinned test; (2) the audit-log entry's `timestamp` field used wall-clock `Date.now()` instead of the injected `now`, cosmetic-only in production (both default to `Date.now()`) but could desync from the enforcement decision under a synthetic clock in tests — fixed to derive from `now`. No blocking correctness or security issue found. Reward-hack checklist (manual, no generic CLI tool reachable — consistent with every prior night's finding that `@metaharness/weight-eft` is a LoRA-distillation tool, not a diff/benchmark scanner): no test weakened, no benchmark/gold data exists to weaken, no cherry-picked cases, no seed manipulation (fully deterministic), $0 cost confirmed. + +Flywheel: no `.claude-flow/flywheel/` state exists in this repo. Evidence retained as the committed test file + issue + this gist, same as every prior algorithmic/correctness dream-cycle candidate. + +| Field | Value | +|---|---| +| Session commit | `29f048fc3b556f857cf2b126d2a84c19d2daa0d0` | +| Report SHA-256 (pre-witness content) | `75fa8021ab6b9890ab5202217c7bf343c97ad18a9c4e313c85d09eda3385f990` | +| Witness stamp | `7d93a48202ac2ee6c8568fd6f038b043f96deb624da06d59ae38a77310de91f8` | + +Verifier procedure: take this gist's content with the Witness table's two value cells blanked back out, SHA-256 it, concatenate with the session commit above, SHA-256 again — result must equal the witness stamp. + +## Recommended Next Steps + +1. **Merge #3139 and this follow-up together** (or squash) — tonight's fix is meaningless without last night's enforcer; both are small, reviewable, and address the human reviewer's own round-1 feedback. +2. **A future night should prototype Darwin-tuning `turnWindowMs`** against a synthetic legitimate-burst-vs-abusive-loop corpus now that a real reset mechanism exists to tune. +3. **Thread `QueenCoordinator.weightedConsensus()` trust weights into the actual vote tally** on the next swarm-surface night — now triple-corroborated (2026-08-19 original finding, DySCo on 2026-08-31, and tonight's independent "Resilient Consensus in Agentic AI" + "Predictive Maps of Multi-Agent Reasoning" findings). diff --git a/v3/@claude-flow/cli/__tests__/mcp-policy-enforcer.test.ts b/v3/@claude-flow/cli/__tests__/mcp-policy-enforcer.test.ts index 885483d7cf..cb031b4774 100644 --- a/v3/@claude-flow/cli/__tests__/mcp-policy-enforcer.test.ts +++ b/v3/@claude-flow/cli/__tests__/mcp-policy-enforcer.test.ts @@ -1,5 +1,6 @@ /** - * Opt-in MCP governance policy enforcement (dream-cycle 2026-08-31, security). + * Opt-in MCP governance policy enforcement (dream-cycle 2026-08-31, security; + * sliding turn-window reset added 2026-09-01, follow-up to review round 1). * * `.harness/mcp-policy.json` declared `auditLog` / `maxToolCallsPerTurn` but * nothing in the running MCP server (`mcp-server.ts`) ever read it before @@ -88,6 +89,65 @@ describe('checkAndRecordCall — per-session budget', () => { }); }); +describe('checkAndRecordCall — sliding turn-window reset (dream-cycle 2026-09-01)', () => { + beforeEach(() => resetPolicyEnforcerState()); + + it('denies the (limit+1)th call within the window, then allows again once turnWindowMs has elapsed', () => { + const policy: McpPolicy = { maxToolCallsPerTurn: 2, turnWindowMs: 1000 }; + expect(checkAndRecordCall(policy, 's1', 0).allowed).toBe(true); + expect(checkAndRecordCall(policy, 's1', 100).allowed).toBe(true); + const denied = checkAndRecordCall(policy, 's1', 200); + expect(denied.allowed).toBe(false); + expect(denied.reason).toMatch(/maxToolCallsPerTurn.*within the last 1000ms/); + + // Still within the window relative to the oldest call (t=0): still denied. + expect(checkAndRecordCall(policy, 's1', 999).allowed).toBe(false); + + // Past the window relative to the oldest call: budget is restored, no + // process restart required (this is the bug this candidate fixes — the + // 2026-08-31 implementation would stay denied forever here). + expect(checkAndRecordCall(policy, 's1', 1001).allowed).toBe(true); + }); + + it('pins the exact boundary: a call at now === oldest-call-time + windowMs is already outside the window (strict `>` cutoff, not `>=`)', () => { + // Flagged by adversarial review (2026-09-01): the boundary itself + // (now == cutoff) was previously untested, only now-1 (denied) and + // now+1 (allowed). Reopening exactly on the boundary — rather than one + // ms later — favors the caller, not an attacker, but pin it explicitly + // so a future refactor can't silently flip `>` to `>=` unnoticed. + const policy: McpPolicy = { maxToolCallsPerTurn: 1, turnWindowMs: 1000 }; + expect(checkAndRecordCall(policy, 's1', 0).allowed).toBe(true); + expect(checkAndRecordCall(policy, 's1', 1000).allowed).toBe(true); + }); + + it('prunes only expired timestamps, not the whole window (a genuine sliding window, not a periodic full clear)', () => { + const policy: McpPolicy = { maxToolCallsPerTurn: 2, turnWindowMs: 1000 }; + expect(checkAndRecordCall(policy, 's1', 0).allowed).toBe(true); // recorded at t=0 + expect(checkAndRecordCall(policy, 's1', 600).allowed).toBe(true); // recorded at t=600 + expect(checkAndRecordCall(policy, 's1', 600).allowed).toBe(false); // budget full: [0, 600] + + // t=1100: cutoff is 100, so only the t=0 call has expired; the t=600 + // call is still live. If this were a full periodic clear instead of a + // real sliding window, both calls made at t=0..600 would either both + // still count or both be wiped — this proves only the stale one drops. + expect(checkAndRecordCall(policy, 's1', 1100).allowed).toBe(true); // [600, 1100] + expect(checkAndRecordCall(policy, 's1', 1100).allowed).toBe(false); // full again + }); + + it('defaults turnWindowMs to 60000 when the policy omits it', () => { + const policy: McpPolicy = { maxToolCallsPerTurn: 1 }; + expect(checkAndRecordCall(policy, 's1', 0).allowed).toBe(true); + expect(checkAndRecordCall(policy, 's1', 59_999).allowed).toBe(false); + expect(checkAndRecordCall(policy, 's1', 60_001).allowed).toBe(true); + }); + + it('ignores a non-positive/invalid turnWindowMs and falls back to the 60000ms default', () => { + const policy: McpPolicy = { maxToolCallsPerTurn: 1, turnWindowMs: -5 }; + expect(checkAndRecordCall(policy, 's1', 0).allowed).toBe(true); + expect(checkAndRecordCall(policy, 's1', 60_001).allowed).toBe(true); + }); +}); + describe('appendAuditLog', () => { let logFile: string; beforeEach(() => { @@ -176,6 +236,25 @@ describe('evaluateToolCall — single enforcement entry point (fail-closed)', () expect(result.allowed).toBe(true); }); + it('restores budget after the turn window elapses, through the full evaluateToolCall pipeline (fake timers, matching this package\'s existing pattern)', () => { + vi.useFakeTimers(); + try { + vi.setSystemTime(0); + const policy: McpPolicy = { auditLog: true, maxToolCallsPerTurn: 1, turnWindowMs: 1000 }; + expect(evaluateToolCall(policy, 's1', 'memory_store').allowed).toBe(true); + expect(evaluateToolCall(policy, 's1', 'memory_store').allowed).toBe(false); + + vi.setSystemTime(1001); + const afterWindow = evaluateToolCall(policy, 's1', 'memory_store'); + expect(afterWindow.allowed).toBe(true); + + const lines = fs.readFileSync(logFile, 'utf-8').trim().split('\n'); + expect(lines).toHaveLength(3); + } finally { + vi.useRealTimers(); + } + }); + it('handles concurrent calls for the same session without double-allowing past the budget', async () => { const policy: McpPolicy = { maxToolCallsPerTurn: 3 }; // evaluateToolCall is synchronous end-to-end (no await inside), so @@ -299,4 +378,40 @@ describe('MCPServerManager tools/call — policy wiring', () => { expect(second.error.message).toMatch(/Policy denied/); spy.mockRestore(); }); + + it('deny -> expiry -> allow through the real stdio dispatcher (MCPServerManager.handleMCPMessage), not just direct evaluateToolCall calls (requested in PR #3152 review)', async () => { + process.env.RUFLO_MCP_ENFORCE_POLICY = '1'; + const policy = { auditLog: true, maxToolCallsPerTurn: 1, turnWindowMs: 1000 }; + fs.writeFileSync(policyPath, JSON.stringify(policy), 'utf-8'); + const mod = await import('../src/mcp-tools/policy-enforcer.js'); + const spy = vi.spyOn(mod, 'loadMcpPolicy').mockReturnValue(policy); + const { MCPServerManager } = await import('../src/mcp-server.js'); + const server = new (MCPServerManager as any)(); + + vi.useFakeTimers(); + try { + vi.setSystemTime(0); + const first = await dispatch(server, 'demo_tool', 'sess-window-e2e'); + expect(first.error).toBeUndefined(); // allow + + const second = await dispatch(server, 'demo_tool', 'sess-window-e2e'); + expect(second.error).toBeDefined(); // deny — budget exhausted + expect(second.error.message).toMatch(/Policy denied/); + + vi.setSystemTime(1001); // past turnWindowMs + const third = await dispatch(server, 'demo_tool', 'sess-window-e2e'); + expect(third.error).toBeUndefined(); // allow — expiry restored the budget + + // The opt-in flag and fail-closed audit logging were not bypassed to + // get there: every one of the three calls above, including the denial, + // is present in the audit log (this module's own real appendAuditLog, + // not a stub) written through the real dispatcher. + const lines = fs.readFileSync(getAuditLogPath(), 'utf-8').trim().split('\n'); + expect(lines).toHaveLength(3); + expect(JSON.parse(lines[1]).allowed).toBe(false); + } finally { + vi.useRealTimers(); + spy.mockRestore(); + } + }); }); diff --git a/v3/@claude-flow/cli/src/mcp-tools/policy-enforcer.ts b/v3/@claude-flow/cli/src/mcp-tools/policy-enforcer.ts index e95b11124f..95b3261118 100644 --- a/v3/@claude-flow/cli/src/mcp-tools/policy-enforcer.ts +++ b/v3/@claude-flow/cli/src/mcp-tools/policy-enforcer.ts @@ -30,16 +30,31 @@ * misconfiguration defeats the feature. See `evaluateToolCall()`. * * Known scope limits (disclosed, not fixed here): - * - Despite the policy field's name, this enforces a *session-lifetime - * cumulative* cap, not a per-conversational-turn cap that resets — a - * long-lived stdio session can exhaust `maxToolCallsPerTurn` (200 in - * the shipped `.harness/mcp-policy.json`) under entirely legitimate - * use and stay locked out until the MCP server process restarts. - * Do not raise this above default-off without a real per-turn reset. * - Only wired into the stdio `tools/call` dispatch * (`MCPServerManager.handleMCPMessage`). The separate HTTP/websocket * path (`startHttpServer()`, via `@claude-flow/mcp`) does not call * this module and is unaffected even when this flag is set. + * + * `maxToolCallsPerTurn` reset semantics (dream-cycle 2026-09-01, follow-up + * to 2026-08-31 review round 1): despite the field's name, the original + * implementation enforced a *session-lifetime cumulative* cap that never + * reset — a long-lived stdio session could exhaust the budget under + * entirely legitimate use and stay locked out until the MCP server process + * restarted. Research that night (see the dream-cycle gist) found: (1) the + * MCP spec only mandates "rate limit tool invocations" with zero mechanism + * guidance, and its 2026-07-28 revision (SEP-2567) is actively removing the + * session concept from the protocol entirely; (2) every framework/product + * that gets this right (FastMCP's rate-limiting middleware, the PolicyLayer + * MCP firewall, Cloudflare's public rate limiter) anchors the reset to + * wall-clock time, not to a turn or session counter that never decays — + * a turn-count reset is gameable by a chatty loop re-arming its own budget, + * which wall-clock time is not. This module now enforces a *sliding + * wall-clock window*: `maxToolCallsPerTurn` calls are allowed per rolling + * `turnWindowMs` (default 60000) per session, keyed by call timestamp so + * calls fall out of the window as time passes rather than accumulating + * forever. `now` is an injectable parameter (defaults to `Date.now`) so + * production callers need no change and tests stay fully deterministic via + * `vi.useFakeTimers()`. */ import * as fs from 'fs'; @@ -54,7 +69,17 @@ export interface McpPolicy { auditLog?: boolean; requireApprovalForDangerous?: boolean; toolTimeoutMs?: number; + /** + * Despite the name, this is a WALL-CLOCK rate limit, not a literal + * conversational-turn counter — MCP has no protocol-level concept of a + * "turn" to count against (confirmed: the spec is silent on it, and its + * 2026-07-28 revision removes the session concept entirely). Enforced as + * "at most this many calls in any rolling `turnWindowMs` window" per + * session. Treat it, and document it to callers, as rate limiting. + */ maxToolCallsPerTurn?: number; + /** Rolling window (ms) over which `maxToolCallsPerTurn` is counted. Default 60000. */ + turnWindowMs?: number; dangerousPatterns?: string[]; approvedServers?: string[]; [key: string]: unknown; @@ -78,13 +103,16 @@ export function loadMcpPolicy( } } +const DEFAULT_TURN_WINDOW_MS = 60_000; + interface SessionState { - callCount: number; + /** Timestamps (ms) of calls still believed to be within the current sliding window. */ + callTimes: number[]; } const sessionState = new Map(); -/** Test-only: clear per-session call counters between test cases. */ +/** Test-only: clear per-session call state between test cases. */ export function resetPolicyEnforcerState(): void { sessionState.clear(); } @@ -95,21 +123,41 @@ export interface PolicyCheckResult { } /** - * Increments and checks the calling session's tool-call count against - * `policy.maxToolCallsPerTurn`. Denies (without incrementing further) once - * the session is already at or over the limit. + * Checks (and, if allowed, records) a tool call against + * `policy.maxToolCallsPerTurn`, counted over a sliding window of + * `policy.turnWindowMs` (default 60000ms) rather than the session's whole + * lifetime. Calls older than the window are pruned before comparing count + * to limit, so a session that pauses gets its budget back rather than + * staying denied until the process restarts. `now` defaults to `Date.now` + * for production callers; tests inject a controlled clock instead. */ -export function checkAndRecordCall(policy: McpPolicy, sessionId: string): PolicyCheckResult { +export function checkAndRecordCall( + policy: McpPolicy, + sessionId: string, + now: number = Date.now(), +): PolicyCheckResult { const limit = policy.maxToolCallsPerTurn; if (typeof limit !== 'number' || !Number.isFinite(limit) || limit <= 0) { return { allowed: true }; } - const state = sessionState.get(sessionId) ?? { callCount: 0 }; - if (state.callCount >= limit) { + const configuredWindow = policy.turnWindowMs; + const windowMs = + typeof configuredWindow === 'number' && Number.isFinite(configuredWindow) && configuredWindow > 0 + ? configuredWindow + : DEFAULT_TURN_WINDOW_MS; + + const state = sessionState.get(sessionId) ?? { callTimes: [] }; + const cutoff = now - windowMs; + state.callTimes = state.callTimes.filter((t) => t > cutoff); + + if (state.callTimes.length >= limit) { sessionState.set(sessionId, state); - return { allowed: false, reason: `maxToolCallsPerTurn (${limit}) exceeded for this session` }; + return { + allowed: false, + reason: `maxToolCallsPerTurn (${limit}) exceeded within the last ${windowMs}ms for this session`, + }; } - state.callCount += 1; + state.callTimes.push(now); sessionState.set(sessionId, state); return { allowed: true }; } @@ -165,6 +213,7 @@ export function evaluateToolCall( policy: McpPolicy | null, sessionId: string, toolName: string, + now: number = Date.now(), ): PolicyCheckResult { if (policy === null) { return { @@ -173,9 +222,9 @@ export function evaluateToolCall( }; } - const budget = checkAndRecordCall(policy, sessionId); + const budget = checkAndRecordCall(policy, sessionId, now); const auditOk = appendAuditLog(policy, { - timestamp: new Date().toISOString(), + timestamp: new Date(now).toISOString(), sessionId, toolName, allowed: budget.allowed,