From a9d356fd268656e910b03c61ad56886e638af496 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:13:16 +0200 Subject: [PATCH] fix(builder): close boundary-test gaps, drop dangling spend-limits copy, guard tool descriptions end-to-end 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 --- .../builder/scripts/regen-tool-schemas.mjs | 16 ++++++-- packages/builder/src/vector/agent-network.ts | 6 +-- .../builder/src/vector/self-improvement.ts | 4 +- packages/builder/src/vector/vector.ts | 10 ++--- packages/builder/test/boundary.test.ts | 37 ++++++++++++++++++- .../integration/rate-limit-gating.test.ts | 4 +- .../builder/test/smoke/tool-inventory.test.ts | 15 ++++++-- .../test/smoke/tool-schemas.snapshot.json | 10 ++--- 8 files changed, 78 insertions(+), 24 deletions(-) diff --git a/packages/builder/scripts/regen-tool-schemas.mjs b/packages/builder/scripts/regen-tool-schemas.mjs index 09582cb..5820fea 100644 --- a/packages/builder/scripts/regen-tool-schemas.mjs +++ b/packages/builder/scripts/regen-tool-schemas.mjs @@ -1,6 +1,9 @@ // packages/builder/scripts/regen-tool-schemas.mjs // Boots the BUILT server, lists tools over SSE, rewrites the smoke snapshot -// sorted by name. Run from the repo root: npm run build && node packages/builder/scripts/regen-tool-schemas.mjs +// sorted by name. Run: npm run build && node packages/builder/scripts/regen-tool-schemas.mjs +// (npm run build must still run from the repo root - or via `npm run build +// --workspace=...` - since it's an npm script; but this script itself now +// runs from any cwd, since its paths are resolved from its own location). // // Deviation from the original brief version: the brief used a flat 2000ms // sleep before connecting. On this machine that raced the server's actual @@ -18,11 +21,18 @@ // packages/builder/test/smoke/tool-inventory.test.ts for the full rationale. import { spawn } from 'node:child_process'; import { writeFileSync } from 'node:fs'; +import { resolve } from 'node:path'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; +// Resolved from this file's own location, not from process.cwd(), so the +// script behaves identically regardless of where it's invoked from (repo +// root, packages/builder, or anywhere else). +const SERVER_ENTRY = resolve(import.meta.dirname, '../build/index.js'); +const SNAPSHOT_PATH = resolve(import.meta.dirname, '../test/smoke/tool-schemas.snapshot.json'); + const PORT = 3939; -const server = spawn('node', ['packages/builder/build/index.js'], { +const server = spawn('node', [SERVER_ENTRY], { env: { ...process.env, PORT: String(PORT) }, stdio: ['ignore', 'ignore', 'pipe'], }); @@ -54,7 +64,7 @@ try { .sort(([a], [b]) => a.localeCompare(b)), ); writeFileSync( - 'packages/builder/test/smoke/tool-schemas.snapshot.json', + SNAPSHOT_PATH, JSON.stringify(snapshot, null, 2) + '\n', ); console.log(`wrote ${tools.length} tool schemas`); diff --git a/packages/builder/src/vector/agent-network.ts b/packages/builder/src/vector/agent-network.ts index 4157e59..18f8713 100644 --- a/packages/builder/src/vector/agent-network.ts +++ b/packages/builder/src/vector/agent-network.ts @@ -357,7 +357,7 @@ Save your Agent DID - you'll need it to update, deregister, or let other agents **Troubleshooting Tips:** 1. Ensure wallet has at least 12 AP3X (10 deposit + fees) -2. Check spend limits with vector_get_spend_limits +2. Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY 3. Each wallet can register multiple agents (different seed UTxOs) 4. Verify Ogmios endpoint is reachable at ${VECTOR_OGMIOS_URL}`, }], @@ -451,7 +451,7 @@ The agent's profile has been updated on-chain. The identity NFT and deposit are 1. Verify the agent DID is correct 2. Your wallet must be the agent's current owner 3. Ensure wallet has enough AP3X for transaction fees -4. Check spend limits with vector_get_spend_limits`, +4. Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY`, }], }; } @@ -672,7 +672,7 @@ The message is recorded on-chain in TX metadata label 674.`, 1. Verify the agent DID is correct: did:vector:agent:{policyId}:{nftAssetName} 2. Ensure wallet has at least 3 AP3X (2 AP3X delivery + fees) 3. Use vector_get_agent_profile to verify the agent exists first -4. Check spend limits with vector_get_spend_limits`, +4. Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY`, }], }; } diff --git a/packages/builder/src/vector/self-improvement.ts b/packages/builder/src/vector/self-improvement.ts index 9bb9b9a..91afc19 100644 --- a/packages/builder/src/vector/self-improvement.ts +++ b/packages/builder/src/vector/self-improvement.ts @@ -465,7 +465,7 @@ Each batch UTxO holds ~30 AP3X for adoption rewards.`, const stakeLovelace = stakeApex * 1_000_000; const safetyCheck = safetyLayer.checkTransaction(stakeLovelace + 4_000_000); if (!safetyCheck.allowed) { - return { content: [{ type: "text", text: `Safety limit exceeded: ${safetyCheck.reason}. Check limits with vector_get_spend_limits.` }] }; + return { content: [{ type: "text", text: `Safety limit exceeded: ${safetyCheck.reason}. Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY.` }] }; } try { @@ -695,7 +695,7 @@ activity tracking token (\`pact_\`) minted. Visible on the Foundation dashboard. 1. Ensure wallet has at least ${stakeApex + 5} AP3X (stake + fees) 2. For Emergency proposals, your agent needs Established reputation (100+ AP3X staked in Game 3) 3. proposalHash must be 64 hex characters (blake2b_256 of your proposal document) -4. Check spend limits with vector_get_spend_limits`, +4. Server spend limits for this tool are set via VECTOR_SPEND_LIMIT_PER_TX / VECTOR_SPEND_LIMIT_DAILY`, }], }; } diff --git a/packages/builder/src/vector/vector.ts b/packages/builder/src/vector/vector.ts index 86a8adf..12ba578 100644 --- a/packages/builder/src/vector/vector.ts +++ b/packages/builder/src/vector/vector.ts @@ -175,7 +175,7 @@ ${utxoList}`, // vector_build_send_apex - build an UNSIGNED AP3X payment (keyless) server.tool( "vector_build_send_apex", - "Build an UNSIGNED transaction sending AP3X to a recipient. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "Build an UNSIGNED transaction sending AP3X to a recipient. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", { changeAddress: z.string().describe("Your wallet address (source of funds; receives change). Get it from your local signer's vector_signer_get_address."), recipientAddress: z.string().describe("Recipient Vector address (addr1...)"), @@ -228,7 +228,7 @@ Non-custodial flow: 1) sign locally with vector_signer_sign { txCbor } — your // vector_build_send_tokens - build an UNSIGNED native-token transfer (keyless) server.tool( "vector_build_send_tokens", - "Build an UNSIGNED transaction sending Vector native tokens. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "Build an UNSIGNED transaction sending Vector native tokens. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", { changeAddress: z.string().describe("Your wallet address (source of funds; receives change). Get it from your local signer's vector_signer_get_address."), recipientAddress: z.string().describe("Recipient Vector address (addr1...)"), @@ -285,7 +285,7 @@ Non-custodial flow: 1) sign locally with vector_signer_sign { txCbor }; 2) broad // vector_build_transaction - build an UNSIGNED multi-output transaction (keyless) server.tool( "vector_build_transaction", - "Build an UNSIGNED multi-output transaction with optional metadata. Takes no key material and never submits - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "Build an UNSIGNED multi-output transaction with optional metadata. Takes no key material and never submits - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", { changeAddress: z.string().describe("Your wallet address (source of funds; receives change). Get it from your local signer's vector_signer_get_address."), outputs: z.array(z.object({ @@ -602,7 +602,7 @@ ${txList} // vector_build_deploy_contract - build an UNSIGNED contract deployment (keyless) server.tool( "vector_build_deploy_contract", - "Build an UNSIGNED transaction deploying a Plutus/Aiken smart contract (locks funds at its script address). Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction.", + "Build an UNSIGNED transaction deploying a Plutus/Aiken smart contract (locks funds at its script address). Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", { changeAddress: z.string().describe("Your wallet address (source of funds; receives change). Get it from your local signer's vector_signer_get_address."), scriptCbor: z.string().describe("Compiled Plutus/Aiken script in CBOR hex format"), @@ -657,7 +657,7 @@ Sign locally with vector_signer_sign, broadcast with vector_submit_transaction, // vector_build_interact_contract - build an UNSIGNED contract interaction (keyless) server.tool( "vector_build_interact_contract", - "Build an UNSIGNED transaction interacting with a deployed Plutus/Aiken contract - lock AP3X at it or spend from it. Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction.", + "Build an UNSIGNED transaction interacting with a deployed Plutus/Aiken contract - lock AP3X at it or spend from it. Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", { changeAddress: z.string().describe("Your wallet address (source of funds, change, and the required signer for spends). Get it from your local signer's vector_signer_get_address."), scriptCbor: z.string().describe("Compiled Plutus/Aiken script in CBOR hex"), diff --git a/packages/builder/test/boundary.test.ts b/packages/builder/test/boundary.test.ts index 4cd1a4a..d8d30de 100644 --- a/packages/builder/test/boundary.test.ts +++ b/packages/builder/test/boundary.test.ts @@ -16,7 +16,14 @@ const CUSTODIAL_UNTIL_LATER_PR = new Set([ 'vector/agent-network.ts', // family 2 — goes keyless in spec PR 7 'vector/self-improvement.ts', // family 3 — goes keyless in spec PR 8 ]); -const FORBIDDEN = /\b(mnemonic|seed|bip39|fromSeed|walletFromSeed|selectWallet\.fromSeed)\b/i; +// No \b boundaries: this is deliberately a substring match, not a whole-word +// one. Word-bounding let `seedPhrase`, `WALLET_SEED` or `bip39x` slip past +// undetected — those are real key-material vocabulary wearing a different +// word shape, not false positives. `fromSeed`/`walletFromSeed` are already +// covered as substrings of `seed`, but stay in the alternation anyway for +// self-documentation (they cost nothing and name the exact API shape this +// guard exists to catch). +const FORBIDDEN = /(mnemonic|seed|bip39|fromSeed|walletFromSeed|selectWallet\.fromSeed)/i; function sourceFiles(dir: string): string[] { const out: string[] = []; @@ -51,4 +58,32 @@ describe('builder custody boundary', () => { ); } }); + + // The allowlist may only shrink (PR 7 drops agent-network.ts to reach 1, PR 8 + // drops self-improvement.ts to reach 0 - see the "goes keyless in spec PR N" + // comments above). Pinning the size means any widening is a deliberate, + // reviewable number change instead of a silent `.add(...)` slipping through. + test('the allowlist may only shrink', () => { + assert.equal(CUSTODIAL_UNTIL_LATER_PR.size, 2, 'the custodial allowlist may only shrink - PR 7 takes it to 1 (drop agent-network), PR 8 to 0'); + }); + + // Pins every FORBIDDEN alternative individually against a fixture corpus, so + // deleting one alternative from the regex (e.g. "we don't need bip39 + // anymore") fails a test by name instead of silently narrowing what this + // guard catches. Also pins representative keyless vocabulary that must NOT + // trip the guard, so widening FORBIDDEN too aggressively is equally visible. + test('every FORBIDDEN alternative is individually pinned against a fixture corpus', () => { + const mustMatch = [ + 'mnemonic', 'MNEMONIC', 'seedPhrase', 'WALLET_SEED', 'bip39', + 'fromSeed', 'walletFromSeed', 'selectWallet.fromSeed', 'seed', + ]; + for (const s of mustMatch) { + assert.match(s, FORBIDDEN, `FORBIDDEN should match "${s}" - deleting the alternative that catches this would silently narrow the guard`); + } + + const mustNotMatch = ['address-only wallet', 'unsigned transaction', 'changeAddress']; + for (const s of mustNotMatch) { + assert.doesNotMatch(s, FORBIDDEN, `FORBIDDEN should NOT match "${s}" - this is legitimate keyless vocabulary`); + } + }); }); diff --git a/packages/builder/test/integration/rate-limit-gating.test.ts b/packages/builder/test/integration/rate-limit-gating.test.ts index c0d71a7..35e0d74 100644 --- a/packages/builder/test/integration/rate-limit-gating.test.ts +++ b/packages/builder/test/integration/rate-limit-gating.test.ts @@ -19,8 +19,8 @@ import type { RawServerHandle } from '../setup.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 vector_get_spend_limits tool it replaces -// (which vector.ts no longer has - Task 5 deleted it, since SafetyLayer state +// 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 diff --git a/packages/builder/test/smoke/tool-inventory.test.ts b/packages/builder/test/smoke/tool-inventory.test.ts index ac4ebf7..78e5099 100644 --- a/packages/builder/test/smoke/tool-inventory.test.ts +++ b/packages/builder/test/smoke/tool-inventory.test.ts @@ -7,9 +7,10 @@ import { startServer, stopServer, ServerContext } from '../setup.ts'; /** * Snapshot of the current tool surface (23 tools) for the non-custodial split * series (see docs/architecture/non-custodial-split.md). PR 6 (Task 5) is the - * PR that took this list from 25 to 23: it deleted `vector_get_address` and - * `vector_get_spend_limits` outright (the safety-layer/wallet-info tools have - * no keyless equivalent — spend limits are gone because the builder no longer + * PR that took this list from 25 to 23: it deleted `vector_get_address` + * outright, along with the tool that reported per-transaction/daily + * SafetyLayer spend status (the safety-layer/wallet-info tools have no + * keyless equivalent — spend status is gone because the builder no longer * holds keys to limit) as part of making the remaining query tools * (`vector_get_utxos`, `vector_get_transaction_history`, `vector_dry_run`) * keyless. PRs 2, 6, 7 and 8 each deliberately restructure and rename tool @@ -141,6 +142,14 @@ describe('tool schema snapshot', () => { live, `Tool "${toolName}" is in the snapshot but was not returned by the live server's listTools().` ); + // Named separately from the deepStrictEqual below: a regen that + // silently drops descriptions (e.g. JSON.stringify dropping an + // `undefined` description key) should fail by naming exactly that, + // not just show up as an opaque diff. + assert.ok( + 'description' in (schemaSnapshot[toolName] as any), + `Snapshot entry for "${toolName}" has no "description" key in test/smoke/tool-schemas.snapshot.json.` + ); assert.deepStrictEqual( { description: live.description, inputSchema: live.inputSchema }, schemaSnapshot[toolName], diff --git a/packages/builder/test/smoke/tool-schemas.snapshot.json b/packages/builder/test/smoke/tool-schemas.snapshot.json index d73f855..fa1871c 100644 --- a/packages/builder/test/smoke/tool-schemas.snapshot.json +++ b/packages/builder/test/smoke/tool-schemas.snapshot.json @@ -22,7 +22,7 @@ } }, "vector_build_deploy_contract": { - "description": "Build an UNSIGNED transaction deploying a Plutus/Aiken smart contract (locks funds at its script address). Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction.", + "description": "Build an UNSIGNED transaction deploying a Plutus/Aiken smart contract (locks funds at its script address). Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", "inputSchema": { "type": "object", "properties": { @@ -61,7 +61,7 @@ } }, "vector_build_interact_contract": { - "description": "Build an UNSIGNED transaction interacting with a deployed Plutus/Aiken contract - lock AP3X at it or spend from it. Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction.", + "description": "Build an UNSIGNED transaction interacting with a deployed Plutus/Aiken contract - lock AP3X at it or spend from it. Takes no key material - sign with your local signer, then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", "inputSchema": { "type": "object", "properties": { @@ -139,7 +139,7 @@ } }, "vector_build_send_apex": { - "description": "Build an UNSIGNED transaction sending AP3X to a recipient. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "description": "Build an UNSIGNED transaction sending AP3X to a recipient. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", "inputSchema": { "type": "object", "properties": { @@ -170,7 +170,7 @@ } }, "vector_build_send_tokens": { - "description": "Build an UNSIGNED transaction sending Vector native tokens. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "description": "Build an UNSIGNED transaction sending Vector native tokens. Takes no key material - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", "inputSchema": { "type": "object", "properties": { @@ -210,7 +210,7 @@ } }, "vector_build_transaction": { - "description": "Build an UNSIGNED multi-output transaction with optional metadata. Takes no key material and never submits - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction.", + "description": "Build an UNSIGNED multi-output transaction with optional metadata. Takes no key material and never submits - sign the returned CBOR with your local signer (vector_signer_sign), then broadcast with vector_submit_transaction and confirm with vector_await_transaction.", "inputSchema": { "type": "object", "properties": {