Addresses opus reviewer findings on the Task 5 commit (33c4b52). Important: - agent-network.ts (3 sites) and self-improvement.ts (2 sites): response copy no longer tells a caller to run vector_get_spend_limits (deleted in Task 5) after a safety-limit error. Replaced with "Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY" (matching each site's existing phrasing). Nothing else in either file touched - both stay custodial until PRs 7/8. Also reworded the two comments (of my own, from Task 5) that named the deleted tool - tool-inventory.test.ts's header docblock and rate-limit-gating.test.ts's probe-choice comment - so a final `grep -rn vector_get_spend_limits packages/` is clean except run.test.ts (Task 6's, left untouched as instructed). - boundary.test.ts: added a third test pinning CUSTODIAL_UNTIL_LATER_PR.size === 2, so PR 7/8 shrinking it is a visible, reviewable number change rather than a silent list edit. - boundary.test.ts: FORBIDDEN is no longer word-bounded (`/(mnemonic|seed|bip39|fromSeed|walletFromSeed|selectWallet\.fromSeed)/i`, \b dropped) so it catches `seedPhrase`, `WALLET_SEED`, `bip39x`-shaped vocabulary, not just whole-word matches. Verified empirically (not just by inspection) that this introduces zero new hits outside the two allowlisted files - grepped `seed`/`bip39`/`mnemonic` case-insensitively across all of packages/builder/src first, confirmed every hit was already confined to agent-network.ts/self-improvement.ts, then reran the full boundary suite to prove it: no benign collisions, nothing to report back before committing. Added a fourth test pinning every FORBIDDEN alternative against a fixture corpus (mnemonic/MNEMONIC/seedPhrase/WALLET_SEED/bip39/ fromSeed/walletFromSeed/selectWallet.fromSeed/seed all match; "address-only wallet"/"unsigned transaction"/"changeAddress" don't) so deleting an alternative from the regex fails a named test instead of silently narrowing the guard. Cheap folds: - vector.ts: the five build_* tool DESCRIPTIONS (not response copy - that already taught the confirm step as of Task 5) now end with "...then broadcast with vector_submit_transaction and confirm with vector_await_transaction." Regenerated the snapshot; verified structurally that the delta is description-field-only on exactly those five entries (inputSchema untouched, all 23 keys unchanged) - this is also what makes the Task 5 addition-A docblock's "descriptions teach the full flow" claim actually true end-to-end. - tool-inventory.test.ts: each per-tool test now also asserts 'description' in schemaSnapshot[toolName] before the deepStrictEqual, so a regen that silently drops a description (e.g. JSON.stringify eating an undefined value) fails by naming the tool, not as an opaque diff. - regen-tool-schemas.mjs: server entry point and snapshot output path are now resolved from import.meta.dirname instead of hardcoded repo-root-relative strings, so the script runs from any cwd. Verified by actually invoking it from packages/builder (not the repo root) and confirming it wrote to the one correct packages/builder/test/smoke/tool-schemas.snapshot.json. Full gate re-run after all of the above: typecheck 0 errors, test:unit 144/144 (140 baseline + 4 boundary tests, up from 2), test:smoke 26/26, test:integration:auth 12/12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
129 lines
6.2 KiB
TypeScript
129 lines
6.2 KiB
TypeScript
import { describe, test, before, after } from 'node:test';
|
|
import assert from 'node:assert/strict';
|
|
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
|
|
import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js';
|
|
import { spawnServer, stopRawServer } from '../setup.ts';
|
|
import type { RawServerHandle } from '../setup.ts';
|
|
|
|
// Covers callTool()-level behavior of per-identity rate limiting, against a
|
|
// real spawned server driving the real registered tools. rate-limiter.test.ts
|
|
// unit-tests RateLimiter/limiterFor in isolation; test:smoke only ever calls
|
|
// listTools(). Neither exercises a single tool handler, so neither would
|
|
// notice checkRateLimit/rateLimiter falling out of scope in agent-network.ts
|
|
// or self-improvement.ts (both @ts-nocheck - tsc can't catch it either). See
|
|
// .superpowers/sdd/pr5-integration-tests-report.md for the mutation-testing
|
|
// evidence.
|
|
//
|
|
// Tool choice: vector_build_send_apex (vector.ts) with a deliberately invalid
|
|
// changeAddress. lucidForAddress (packages/builder/src/vector/build.ts)
|
|
// validates the address with getAddressDetails() and throws BEFORE any
|
|
// provider call - proven by Task 2's booby-trapped-provider test. Handlers
|
|
// return caught errors as normal content, so isError stays false and this
|
|
// probe is just as hermetic as the spend-limit-status tool it replaces
|
|
// (vector.ts no longer has one - Task 5 deleted it, since SafetyLayer state
|
|
// is no longer exposed to keyless callers).
|
|
// vector_get_agent_profile (agent-network.ts) and
|
|
// vector_self_improvement_analyze_metrics (self-improvement.ts) DO reach
|
|
// Ogmios/Koios in their normal bodies, but only after their own
|
|
// checkRateLimit() call, which is the first statement in both handlers and
|
|
// returns before any network code runs. This file only ever calls those two
|
|
// once alice's budget is already exhausted, so that network-touching code is
|
|
// never reached in a passing run - confirmed empirically below (response time,
|
|
// and the exact rate-limit text rather than a network error). That is also
|
|
// exactly why this suite is hermetic and safe for CI: see the report for the
|
|
// full argument.
|
|
|
|
const TOKEN_ALICE = 'tok-alice-rl-3c9f1a';
|
|
const TOKEN_BOB = 'tok-bob-rl-7d2e6b';
|
|
|
|
const RATE_LIMIT_ENV = {
|
|
MCP_AUTH_TOKENS: `alice:${TOKEN_ALICE},bob:${TOKEN_BOB}`,
|
|
// Deliberately tiny: the first call consumes the identity's only slot, the
|
|
// second is refused, for the rest of the real 60s sliding window - which
|
|
// easily outlasts this whole test file.
|
|
VECTOR_RATE_LIMIT_PER_MINUTE: '1',
|
|
};
|
|
|
|
const RATE_LIMIT_PATTERN = /^Rate limit exceeded\. Retry after \d+ms\.$/;
|
|
|
|
async function connect(port: number, token: string): Promise<Client> {
|
|
const transport = new SSEClientTransport(new URL(`http://localhost:${port}/sse`), {
|
|
requestInit: { headers: { Authorization: `Bearer ${token}` } },
|
|
});
|
|
const client = new Client({ name: 'rate-limit-integration-test', version: '1.0.0' });
|
|
await client.connect(transport);
|
|
return client;
|
|
}
|
|
|
|
interface ToolOutcome {
|
|
text: string;
|
|
isError: boolean;
|
|
}
|
|
|
|
async function call(client: Client, name: string, args: Record<string, unknown> = {}): Promise<ToolOutcome> {
|
|
const result = await client.callTool({ name, arguments: args });
|
|
const content = result.content as Array<{ type: string; text?: string }>;
|
|
return { text: content?.[0]?.text ?? '', isError: result.isError === true };
|
|
}
|
|
|
|
describe('per-identity rate limiting across real registered tools', () => {
|
|
let ctx: RawServerHandle;
|
|
let alice: Client;
|
|
let bob: Client;
|
|
|
|
before(async () => {
|
|
ctx = await spawnServer(RATE_LIMIT_ENV);
|
|
alice = await connect(ctx.port, TOKEN_ALICE);
|
|
bob = await connect(ctx.port, TOKEN_BOB);
|
|
});
|
|
|
|
after(async () => {
|
|
try { await alice.close(); } catch {}
|
|
try { await bob.close(); } catch {}
|
|
await stopRawServer(ctx);
|
|
});
|
|
|
|
test('alice exhausts her budget on a vector.ts tool; bob is unaffected (isolation via the real tools, not just limiterFor)', async () => {
|
|
const PROBE_ARGS = { changeAddress: 'not-an-address', recipientAddress: 'not-an-address', amount: 1 };
|
|
|
|
const first = await call(alice, 'vector_build_send_apex', PROBE_ARGS);
|
|
assert.equal(first.isError, false, 'first call must not be a tool error');
|
|
assert.ok(!RATE_LIMIT_PATTERN.test(first.text), 'first call must not be rate-limited yet');
|
|
|
|
const second = await call(alice, 'vector_build_send_apex', PROBE_ARGS);
|
|
assert.equal(second.isError, false, 'a rate-limit refusal is a normal, well-formed result - not a thrown tool error');
|
|
assert.match(second.text, RATE_LIMIT_PATTERN, 'alice must now be rate-limited');
|
|
|
|
// Bob has never called anything: this proves his bucket is independent of
|
|
// alice's, driven through a real connection and a real tool call - not an
|
|
// inference from limiterFor() in isolation (rate-limiter.test.ts already
|
|
// covers that in isolation; this covers the wiring rate-limiter.test.ts
|
|
// cannot see).
|
|
const bobFirst = await call(bob, 'vector_build_send_apex', PROBE_ARGS);
|
|
assert.equal(bobFirst.isError, false);
|
|
assert.ok(!RATE_LIMIT_PATTERN.test(bobFirst.text), "bob must be unaffected by alice's exhausted budget");
|
|
});
|
|
|
|
test('alice, still exhausted, is also rate-limited on an agent-network.ts tool', async () => {
|
|
const r = await call(alice, 'vector_get_agent_profile', { agent_id: 'did:vector:agent:test:test' });
|
|
assert.equal(r.isError, false, 'must be a well-formed rate-limit result, not a thrown error');
|
|
assert.match(
|
|
r.text,
|
|
RATE_LIMIT_PATTERN,
|
|
'a checkRateLimit/rateLimiter scoping regression in agent-network.ts would surface here as ' +
|
|
'isError:true with a ReferenceError message ("rateLimiter is not defined") instead of this text'
|
|
);
|
|
});
|
|
|
|
test('alice, still exhausted, is also rate-limited on a self-improvement.ts tool', async () => {
|
|
const r = await call(alice, 'vector_self_improvement_analyze_metrics', {});
|
|
assert.equal(r.isError, false, 'must be a well-formed rate-limit result, not a thrown error');
|
|
assert.match(
|
|
r.text,
|
|
RATE_LIMIT_PATTERN,
|
|
'a checkRateLimit/rateLimiter scoping regression in self-improvement.ts would surface here as ' +
|
|
'isError:true with a ReferenceError message ("rateLimiter is not defined") instead of this text'
|
|
);
|
|
});
|
|
});
|