Four items from the final whole-branch review, all docs-only:
1. Architecture diagram: the ASCII diagram routed every tool call through
Rate Limiter -> Safety Layer -> Lucid/Ogmios, which is false for the
keyless build_* path (safety.ts is never consulted there). Annotated the
Safety Layer box with a footnote marker instead of redrawing the diagram -
box alignment verified programmatically before and after (same column
positions for every vertical bar).
2. Configuration table: added one sentence stating VECTOR_SPEND_LIMIT_PER_TX/
DAILY govern only the still-custodial families, matching .env.example's
existing scope note.
3. Testing section: "builds every build_* tool's unsigned transaction"
overclaimed - keyless-build.test.ts covers 4 of 5 (verified directly
against the test source: vector_build_send_tokens is absent from it,
covered instead by unit tests and the legacy integration suite). Reworded
to name the gap explicitly.
4. packages/builder/test/integration/keyless-e2e.test.ts:9 (comment only):
"which lives on PR #7's branch" was stale now that #7 is merged and
packages/signer is in-tree. Reworded to name the in-tree package and be
precise about what the stand-in does and doesn't exercise (same CML
primitives, without decode/policy/audit).
#7 (signer) and #8 (builder hardening) merged into main this morning; this
branch is rebased onto the result and packages/signer is now in-tree. Three
sites still said pairing the signer with the hosted builder "does not yet
remove the custody exposure" (true when #7 was written, before this PR's
keyless build_* tools existed - now false for the wallet/tx/contract family):
- README.md's "Local signer" section: now states the 4-call non-custodial
flow (signer_get_address -> build_* -> signer_sign -> submit -> await) is
live today for that family, names the signer's four tools, and keeps the
agent-registry/self-improvement families explicitly scoped out.
- packages/signer/README.md's "Known limitations" bullet and "Running it"
closing line: same correction, with the specific now-keyless build_* tool
names and an accurate (not overclaiming) description of what the E2E test
exercises - it signs with the same CML primitives sign.ts uses, as a
stand-in for calling vector_signer_sign directly, not a call into the
signer package's own code.
Grepped case-insensitively across the repo for "does not yet remove" /
"custody exposure" / "signer half is ready" / "builder half is not yet"
before and after: exactly these 3 sites found, all corrected, zero stale
instances remain.
Opus review round 2 on Task 6:
- agent-network.ts: seedUtxo -> oneShotUtxo (5 occurrences). This is
one-shot-minting-policy vocabulary that survives PR 7's keyless rewrite,
but the boundary test's FORBIDDEN regex is an unanchored case-insensitive
substring match on "seed" - left as-is it would have false-red PR 7's
guard the moment agent-network.ts drops off the custodial allowlist. Only
selectWallet.fromSeed (genuine key-material vocabulary, deleted by PR 7
anyway) still matches `seed` in this file. File is @ts-nocheck, so this
was verified mechanically, not just visually: line-match counts before
(seedUtxo=5) and after (seedUtxo=0, oneShotUtxo=5) line up exactly, and a
fresh `import()` of the module still loads error-free.
- keyless-build.test.ts: the first tier-1 test summed every output
(`paid >= 3_000_000n`), but a self-send's change output lands at the same
address as the payment, so the sum floor could never actually detect a
dropped or wrong payment amount - change alone clears it. Added an
exact-count check (outputs at exactly 3_000_000n lovelace, >=1) alongside
the existing sum/fee/unsigned assertions, matching the pattern already
used by the build_transaction test in the same file. Also added the
missing assertBuildNotSubmitted check on the dry-run test's own build
response.
- run.test.ts: deleted walletBalanceAda - write-only since the UTxO
Consolidation block (its only reader) was removed earlier in this task.
Re-verified full gate plus one live re-run of the tightened tier-1 suite
(build-only, no funds spent, no E2E resubmission).
"different seed UTxOs" is one-shot minting-policy vocabulary, not key
material, but the boundary test's FORBIDDEN regex matches "seed" as a
substring anywhere in scanned source. agent-network.ts is allowlisted as
custodial today so this line is invisible to the guard, but PR 7 takes it
keyless and drops the allowlist entry - at that point this comment would
false-red the guard for no reason. Reworded to "one-shot UTxOs", meaning
unchanged.
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>
Finishes the builder-side wallet/tx family's non-custodial split (spec PR 6).
Task 4 registered the keyless build_* tools; this task deletes the last
custodial surface in vector.ts and makes every remaining query tool keyless:
- Deleted initLucid, getWalletInfo, the vector_get_address and
vector_get_spend_limits tool registrations, and the now-unused safetyLayer
and Lucid imports. SafetyLayer/safety.ts itself stays (still used by
agent-network.ts and self-improvement.ts until PRs 7-8).
- vector_get_utxos, vector_get_transaction_history: address is now required,
mnemonic param and branch deleted, provider construction routed through
makeProvider().
- vector_dry_run: mnemonic param removed; the outputs-branch now builds via
lucidForAddress + buildMultiOutput (keyless, UNSIGNED) instead of signing
with a mnemonic-derived wallet.
- packages/shared/src/types.ts: removed the now-dead VectorToken,
VectorWalletInfo, VectorAdaTransactionResult, VectorTokenTransactionResult,
VectorBuildTransactionResult, VectorDeployContractResult and
VectorInteractContractResult interfaces (and their re-exports from
vector/index.ts). SpendLimits/SpendStatus/AuditEntry stay - safety.ts is
still live for the two custodial families.
- vector_build_transaction / vector_build_deploy_contract /
vector_build_interact_contract response copy now teaches the full
build -> sign -> submit -> confirm flow (they previously stopped at
submit), matching vector_build_send_apex/send_tokens.
- rate-limit-gating.test.ts: repointed its hermetic probe from the
now-deleted vector_get_spend_limits to vector_build_send_apex with an
invalid changeAddress - lucidForAddress rejects it in-process before any
provider call, so the suite stays hermetic.
- Added packages/builder/test/boundary.test.ts: a mechanical grep-based
custody boundary (design-spec success criterion 1). Verified failing before
the rewrite (vector.ts's mnemonic vocabulary) and passing after; also
verified the guard actually fails by injecting `// mnemonic` into build.ts
and watching it name the exact file:line before reverting.
- Tool surface is now 23 (25 - 2 deletions). tool-inventory.test.ts updated
accordingly.
Snapshot shape widened (this once, deliberately): tool-schemas.snapshot.json
entries are now { description, inputSchema } instead of a bare inputSchema,
so the regression guard also covers the security-teaching copy in tool
descriptions (the sign/submit/confirm flow callers learn from them), not just
JSON Schema shape. tool-inventory.test.ts's per-tool test now deep-compares
the whole entry and is relabeled "schema+description unchanged". This is why
the snapshot diff touches every entry, not just the 10 that actually changed.
Committed the regen script at packages/builder/scripts/regen-tool-schemas.mjs
(previously scratchpad-only) with this shape change applied.
Verified against the feat/builder-auth-and-submit baseline: the snapshot key
delta is exactly 4 renames (send_apex/send_tokens/deploy_contract/
interact_contract -> build_*), 4 changed-in-place (build_transaction,
dry_run, get_utxos, get_transaction_history), and 2 removals (get_address,
get_spend_limits) - confirmed with a structural JSON diff, since the uniform
shape-widening makes raw line-based git diff noisy (e.g. it shows
vector_message_agent's key line as touched even though its inputSchema is
byte-identical old vs new - a Myers-diff line-matching artifact, not real
agent-network.ts drift).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Move fixture-provider.test.ts up to test/ (test:unit's glob is
packages/*/test/*.test.ts, deliberately shallow, so a nested
test/fixtures/ file was silently never run). The fixture module itself
(fixture-provider.ts, the capture script, the JSON fixture) stays in
test/fixtures/; only the *.test.ts file moved, importing it via
./fixtures/fixture-provider.ts.
- Harden capture-protocol-params.mjs: refuse to write a fixture whose
costModels/coinsPerUtxoByte/keyDeposit could have come from
OgmiosProvider's own `??` fallback defaults instead of a genuine
response. The brief's sanity numbers (minFeeA 45, minFeeB 156253, ...)
are literally those same fallback literals, so matching them proves
nothing; costModels has no populated fallback, so a well-formed,
appropriately-sized cost model per Plutus version is the one signal a
schema-mismatched response cannot fake.
- packages/builder/tsconfig.json: include test/**/* so `npm run
typecheck` actually checks builder's test files (it previously covered
src/**/* only, unlike packages/shared). Needed
allowImportingTsExtensions for the now-in-scope relative .ts-suffixed
imports across every existing test file. Surfaced one real pre-existing
gap: auth.test.ts calls loadAuthConfig with deliberately minimal env
objects, but the exported signature demanded the full
env.d.ts-augmented NodeJS.ProcessEnv (7 required custom fields)
even though the function only ever reads MCP_AUTH_TOKENS. Narrowed the
parameter to Record<string, string | undefined> - the function's real
contract - which is a type-only change with a single production call
site (index.ts's parameterless loadAuthConfig()) that already relies on
the default.
Verified: test:unit 117/117 (was 113 - the 4 fixture tests were never
counted before), typecheck 0 errors, test:smoke 28/28, build unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The "unusual identity strings" fixture embedded a literal 0x00 byte in a
string literal instead of the \x00 escape sequence. Consequence: the file
had no valid text encoding as far as git's binary-diff heuristic is
concerned, so the whole 5278-byte file rendered as "Binary files differ"
with zero textual diff - invisible to review, and would have been
invisible to any GitHub PR reviewer too. It also made the raw NUL
character indistinguishable from the adjacent ASCII-space case when
viewed directly.
Same runtime string (a genuine one-character NUL string, confirmed via
'\x00'.charCodeAt(0) === 0), no behavior change - the file is just
diffable text again.
vector_await_transaction's confirmation check only ever looked for an
unspent output at index 0 - a heuristic that can read a landed transaction
as unconfirmed, since these tools broadcast CBOR signed elsewhere with no
guarantee of that output existing or staying unspent. The check also wrapped
the lookup in .catch(() => []), collapsing genuine infrastructure failures
(Ogmios down, malformed response) into the same "not confirmed yet" result,
defeating pollUntilConfirmed's documented contract that a check rejection
should propagate.
OgmiosProvider.getKoiosTxStatus + koiosIndicatesConfirmed (shared/provider.ts)
now provide a direct, non-swallowing Koios tx_status check; poll.ts's new
buildConfirmationCheck picks it when VECTOR_KOIOS_URL is configured and
falls back to the outRef heuristic (also no longer swallowing errors)
otherwise - the same preference order OgmiosProvider.awaitTx already used,
without reusing awaitTx itself (still not caller-configurable, still
swallows every failure). awaitTx stays in place only because the Provider
interface requires it; nothing else calls it.
buildConfirmationCheck and koiosIndicatesConfirmed are unit tested via
dependency injection, the same pattern poll.test.ts already uses for
now/sleep, with no network access required.
The snapshot guard flagged both additions, which is what it is for. Delta
verified as exactly two ADDED entries with no existing schema changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the keyless half of the non-custodial flow: the signer returns
signed CBOR, and these broadcast it and wait for confirmation.
submitTx previously returned the first 64 hex characters of the submitted
CBOR when the response failed to parse - a string indistinguishable from a
real transaction hash. It now refuses via a new exported
parseSubmitResponse, since these tools are the first callers to actually
surface that value to an agent. Extracted to module scope and tested
directly (18 cases) rather than through submitTx itself, since provider.ts
imports fetch from cross-fetch rather than reading the global - stubbing it
would leave the real implementation live and hit a genuine DNS lookup.
vector_await_transaction's polling loop is extracted into vector/poll.ts
(pollUntilConfirmed) rather than left inline in the tool. Two reasons:
- The original loop slept a fixed 3s every iteration regardless of budget
remaining, so a short timeoutSeconds could overshoot its deadline by
close to 3x (e.g. timeoutSeconds=1 could block ~3s). The sleep is now
clamped to the time left before the deadline.
- Extracting it makes the loop unit-testable (11 cases) with an injected
fake clock, instead of only being reachable through a live network call.
Does not reuse the provider's own awaitTx: it hard-codes a 60-attempt
budget with no caller control and returns a bare boolean, collapsing
"still pending" and "gave up" into one signal - the tool needs a
configurable timeout and distinct wording for the two, since a pending tx
may still land and must not be reported as failed.
Judgement call on the brief's fixture: its final test asserted
parseSubmitResponse(CBOR.slice(0, 64)) throws. With the simple 64-hex-char
isTxHash check specified in the brief, 'ab'.repeat(32) legitimately
qualifies as a hash - it IS 64 valid hex characters - so that assertion was
changed to assert.equal instead of contorting the implementation to add
stricter validation the brief didn't ask for. The invariant that matters
still holds: parseSubmitResponse only ever receives the HTTP response body,
never the submitted tx, so this value can no longer be invented by our own
code from data it already held - only echoed back by a server, in which
case treating it like any other bare-text hash is correct.
Beyond the brief: added coverage for JSON responses in shapes the parser
doesn't recognize (array/number/null/boolean/nested object/unhandled field
name), the "id" field alias, a zero-budget/never-yielding poll, and that a
rejection from the confirmation check propagates instead of being read as
"not yet confirmed".
test:unit 96/96, typecheck clean. test:smoke fails as expected (23 -> 25,
naming vector_await_transaction and vector_submit_transaction as added) -
regenerating that snapshot is Task 5's job. test:integration:auth 12/12
unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 2 and 3 verified their work with throwaway scripts against a real
spawned server that were never committed, so npm test protected none of it -
index.ts's auth gate had zero automated coverage, and the @ts-nocheck
agent-network.ts/self-improvement.ts rate-limit wiring had none beyond
listTools().
Adds two hermetic integration suites (real spawned server, localhost only,
no wallet/chain calls):
- auth-gating.test.ts: both HTTP routes' 401/403 gating, the auth-before-
session-lookup ordering, cross-identity rejection, token-leak scanning,
and that the server survives all of it (the reviewer's severe finding -
a missing early `return` after a 401 falls through into a second
writeHead() and crashes the whole process via an unhandled rejection).
- rate-limit-gating.test.ts: per-identity isolation and that exhaustion on
a vector.ts tool also rate-limits agent-network.ts and self-improvement.ts
tools for that identity, through the real registered tools rather than
limiterFor() in isolation.
Extends test/setup.ts additively (spawnServer/stopRawServer/RawServerHandle)
so these tests can set MCP_AUTH_TOKENS and drive per-request Authorization
headers by hand; startServer()/stopServer()/callTool() are unchanged and the
existing run.test.ts still fails for exactly its pre-existing reason.
Wires a new test:integration:auth script into CI's build-and-test job.
All five mutations from the review were applied to a working-tree copy,
built, and reverted per-mutation to prove the new tests actually fail:
- removing either route's early `return` after a 401: caught (cascading
ECONNREFUSED once the process crashes; a dedicated liveness test targets
this directly)
- the cross-identity check's undefined-lookup branch: not independently
catchable - transports/sessionIdentities are always populated and
cleared together, synchronously, so that state is unreachable via HTTP;
documented in the report, verified a same-region operator-flip mutation
IS caught cleanly
- module-scope `identity` reference in self-improvement.ts: crashes the
server at startup (ReferenceError at import time), caught via the
before() hook
- checkRateLimit restored to module scope in agent-network.ts: caught
cleanly, isError:true / "rateLimiter is not defined" instead of the
well-formed rate-limit text
No file under any src/ changed. test:unit 67/67, test:smoke 26/26 (23
tools, 0 drift), typecheck clean, new suite 12/12.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The limiter was a module-level singleton, so one busy caller throttled every
other caller on the box and a shared daily budget was trivially exhausted.
Buckets are now keyed by authenticated identity, and keyed centrally so
reconnecting does not hand back a clean bucket.
POST /messages is gated too, and cross-checked against the session's own
identity - otherwise a valid token for one caller could drive another
caller's session by guessing its id.
All four loadAuthConfig throw sites could be made to interpolate the secret
that triggered them with the suite staying fully green - the one constraint
this module states twice in its own header had no regression protection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hosted deployments set MCP_AUTH_TOKENS; localhost self-hosters are unaffected.
The resolved identity doubles as the rate-limit bucket key. Rejection reasons
never echo the supplied token, and a bare token's log label is a hash prefix.
Hardened beyond the brief after an adversarial pass (20 additional tests):
- A malformed MCP_AUTH_TOKENS that parses to zero usable identities (e.g.
",,," from a broken template) now throws instead of silently resolving to
`enabled: false` - the exact "operator believes auth is on, server is
actually open" failure shape this module exists to prevent. An explicitly
empty/unset value still disables cleanly, unchanged.
- A token containing embedded whitespace is now rejected at config-load
time: resolveIdentity's \S+ match can never span it, so without this check
the token would be configured yet permanently unpresentable.
- Documented and pinned two non-fixable format limitations: only the first
colon in an entry separates label from token (a label can't itself contain
one), and a literal comma inside a label/token silently fragments the
entry since the delimiter has no escape mechanism.
- Pinned correct existing behaviour: exact case-sensitive token match, no
prefix/superstring matching, Map-backed storage is immune to
__proto__/constructor prototype pollution, one label may legitimately
cover multiple tokens (rotation), empty-string header treated as missing.
- Map.get token lookup is not constant-time; judged acceptable and
documented inline rather than swapped for crypto.timingSafeEqual - see
the comment above the lookup in auth.ts for the reasoning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review verified six obfuscations that bypass boundary.test.ts's ambient-
global and forbidden-import checks: globalThis['fetch'](...), an alias
(const f = fetch; f(...)), an aliased bracket reference, the comma-operator
trick ((0, globalThis.fetch)(...)), and two dynamic import()s with a
concatenated specifier (import('node:' + 'dns'), import('child_' +
'process')). README.md called the check "the package's central security
guarantee" and boundary.test.ts's header claimed the signer "CANNOT reach
the network -- not that it chooses not to." Neither line was true of what a
text scan can actually promise.
Hardened cheaply, within reach of a text scan:
- FORBIDDEN_IMPORTS gains node:dns, node:dns/promises, node:http2,
node:worker_threads, node:vm, each with a one-line rationale.
- AMBIENT_NETWORK_GLOBALS gains globalThisQualifiedAccess(name), matching a
global reached via globalThis in either dot or bracket ("computed member
access") notation, plus navigator['sendBeacon'] bracket notation.
Verified empirically against the real hardened check (temporary probe
files under src/, one per evasion, deleted immediately after -- see the
task report for the full per-file breakdown): globalThis['fetch'](...),
the aliased-bracket-reference variant, and the comma-operator
globalThis.fetch trick are now all caught. The plain-identifier alias and
both concatenated dynamic imports still pass -- those need real data-flow
tracking or evaluating an arbitrary runtime expression, neither of which a
text scan can do, and no attempt was made to chase them.
Reworded both the README section and boundary.test.ts's header comment to
say what the check actually verifies: today's shipped src/ has no
accidental or careless network capability. Deliberate obfuscation is named
as a code-review responsibility, not something this file promises to catch.
Final whole-branch review, Fix 4 of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decode.ts's trailing-bytes consumption check re-serialises a parsed
transaction and compares it against the input, refusing on any mismatch.
That check relies on CML's parse-then-reserialise round trip preserving the
agent registry's indefinite-length CBOR constructor encoding (D8 79 9F ...
FF, required by the Conway-era Aiken validators; Lucid's Data.to emits the
definite-length form instead, which those validators reject) rather than
silently canonicalising it -- if it did not, the consumption check would
misread that harmless re-encoding as unrecognised trailing data and refuse
every transaction using this ecosystem's own real encoding.
sign.test.ts already pins this exact claim for sign.ts's own, independent
from_cbor_hex call, and its own header comment flagged decode.ts as having
"the exact same untested gap" -- out of that task's file scope, left for a
later task. This is that closure: the same fixture (built with the same
cborBytes/cborUint helpers, embedded both as a body datum and a witness-set
datum) ported to exercise decodeTransaction directly.
Verified by mutation (swapping the comparison to tx.to_canonical_cbor_hex()):
probed directly first and confirmed canonical re-serialisation really does
rewrite this fixture's bytes, then confirmed exactly the 4 tests that call
decodeTransaction() on it fail, while the sanity test and the byte-survival
test (neither of which call decodeTransaction) are unaffected -- see the
task report.
Final whole-branch review, Fix 3b of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decode.ts's readAssets() builds each asset's `unit` as
`policy.to_hex() + name.to_hex()`, deliberately not
`name.to_cbor_hex().slice(2)` -- the CBOR byte-string header is 1 byte for
names up to 23 bytes but grows to 2 bytes at 24 (Cardano asset names run up
to 32 bytes), so a fixed slice(2) leaves the extra header byte glued onto
the front of the unit for every 24-32 byte name. That fix shipped with this
module's first commit but was only ever verified by a throwaway probe --
nothing in the suite constructed a real CML multi-asset output, across 8
tasks and 22 commits.
Builds a real TransactionOutput carrying a MultiAsset value with three
deterministic asset names (23 bytes, exactly 24, and the 32-byte max) under
one policy, re-serialised through Transaction.new(...) so the fixture
exercises decodeTransaction's full parse path, not just readAssets() in
isolation. Asserts the decoded unit is exactly policyHex + assetNameHex.
Verified by mutation (reverting to name.to_cbor_hex().slice(2)): the 24-byte
and 32-byte tests fail with the corrupted unit carrying a stray leading
header byte (18.../20... prefix); the 23-byte test is correctly unaffected,
since both forms agree below the boundary -- see the task report.
Final whole-branch review, Fix 3a of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Number(env.VECTOR_SIGNER_ACCOUNT_INDEX ?? '0') was never validated. Verified
against the real walletFromSeed: a NaN-producing value, a negative integer,
and a fractional value each derived a DIFFERENT, syntactically valid
addr1... wallet with no error anywhere in the chain, and describe() would
echo the nonsense value back as if nothing were wrong ("mnemonic (account
index NaN)"). Spend limits three lines away already fail loud on the same
shape of mistake (BigInt(...) throws on garbage) -- this applies that same
instinct here: parseAccountIndex() rejects anything that is not a
non-negative integer, naming the variable and the offending value.
Only called from the two MnemonicKeySource-constructing branches, not
unconditionally -- PrivateKeyKeySource never reads an account index, so a
leftover garbage VECTOR_SIGNER_ACCOUNT_INDEX must not block a
private-key-configured signer that never uses it. Pinned by its own test.
config.test.ts covers: NaN-producing input, a negative integer, a fractional
value, a valid value (cross-checked against a directly-constructed
MnemonicKeySource for the same wallet), the unset default, and the
PrivateKeyKeySource non-interference case. Verified by mutation (temporarily
disabling the guard) that exactly the three invalid-input tests fail and the
other three are unaffected -- see the task report.
Final whole-branch review, Fix 2 of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vector-signer-audit-log.json resolves inside the repo checkout by default,
and the README self-disclosed that it was NOT in .gitignore (unlike the
builder's vector-audit-log.json). A documented footgun is worse than no
footgun when the fix is one line. Adds the entry and updates the README
paragraph that described the gap to describe the fix instead.
Final whole-branch review, Fix 1 of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Includes an opt-in testnet round-trip test (keyless build -> decode -> policy
-> sign, never submits), excluded from CI because it needs a funded wallet.
Run against the real funded testnet wallet: pass 3, fail 0, nothing submitted.
Verified every README claim against the code as implemented, not just the
brief text, and corrected several:
- VECTOR_SIGNER_AUDIT_LOG_PATH's default resolves against the package's own
install location (repo root), not the process's cwd as originally stated --
and that default filename is not yet covered by .gitignore.
- An audit-log write failure blocks ALL signing, not just accounting:
audit.append() throws by design and vector_signer_sign withholds an
already-produced signature rather than release one with no durable record.
- Documented the #private-field defense against accidental key leakage via
JSON.stringify/console.log, and the read-only/sign tool split that keeps
the signing key out of scope for get_address/decode/get_spend_limits.
- Documented that pairing this signer with the CURRENT hosted builder does
not yet remove the custody exposure it's designed to fix: no builder tool
is keyless yet, and there is no submit_transaction tool to hand a produced
signature to. That's the next stage of the migration, not shipped here.
Env-var cross-check (brief's Step 6, both directions) passes clean.
A decode_transaction that signs only when policy-allowed, worded to avoid
"sign", passed all 21 tests - a preview tool returning real signed bytes with
no audit entry and no budget consumed. And the stdout-purity constraint was
only enforced incidentally by an unrelated stderr sanity check, while the MCP
client silently swallows unparseable stdout lines.
Gap 1 (decode-can-sign) is closed structurally, not just by test: tools.ts
splits into registerReadOnlyTools() (get_address, decode_transaction,
get_spend_limits - receives only signerAddress/limits/auditLogPath/audit,
never config or keySource) and registerSignTool() (the only function in the
module whose closure ever holds a loaded SigningIdentity). Reproducing the
review's exact mutation against the new code is now a compile error
(TS2304: Cannot find name 'identity') rather than a passing test suite.
Defense-in-depth on top of that: a new content-based test independently
computes what signing the previewed transaction would actually produce and
asserts decode's response never contains those bytes, under any wording -
this is what catches a more determined mutation that deliberately re-widens
the read-only context to reach signing capability again.
Gap 2 (stdout purity) is closed with a purpose-built minimal Transport
(RawCapturingStdioTransport) that captures every raw stdout byte independently
of the SDK's own lenient line parsing, while still driving the real Client
class for a correct initialize handshake. Asserts every line written to
stdout, across a healthy startup and a normal tool call, parses as JSON.
Both fixes reproduced-then-verified: applying the review's mutations first
confirmed each gap was real (all tests green under the vulnerable code),
then confirmed each new check catches it, then confirmed restoring the fix
returns to 23/23 with no collateral. Also re-ran the original hard-requirement
mutations (signed- and refused-path unguarded audit.append, the exact
catch{warn();return signature} anti-pattern, and a missing return after the
refused branch that lets a refused transaction fall through to real signing)
against the restructured tools.ts to confirm the refactor weakened nothing -
none did; the missing-return mutation alone is caught by 4 tests, including
3 of the brief's own original 9.
typecheck 0, test:unit 180/180, builder test:smoke 26/23/0, boundary 4/4,
signer test:smoke:signer 23/23 (9 brief + 12 original edge-case pass + 2 new).
Hard requirement for this task: audit.ts's append() throws by design on a
write failure (see its own CONTRACT note). The trap is catching that throw
and still returning the signature already produced - that reproduces exactly
the failure the throw exists to prevent, one call site later: a real
signature reaches the caller with no durable record of it, so a restarted
process's committedTodayLovelace() silently under-counts and the daily limit
stops binding.
vector_signer_sign now wraps both audit.append() calls explicitly:
- Signed path: if the append throws, the signature is withheld. The
function returns before ever reading signed.signedCborHex - the only
`return` that reaches it is the one after a successful append. The
response tells the caller a valid signature WAS produced but could not be
durably recorded, so it is being withheld rather than released.
- Refused path (lower stakes - no funds move either way): if the append
throws, the refusal itself is still delivered with its concrete policy
reason (never silently replaced by a generic disk-error message), plus a
note that this specific refusal could not be recorded, so the caller does
not mistake an incomplete audit trail for a normal one.
Mutation-tested against the brief's own unguarded version (see the task
report for the full transcript): reverting to unwrapped `audit.append()`
calls does NOT leak signed CBOR either way (the throw prevents the function
from ever reaching its return statement) - but it does replace "REFUSED.
<reason>" with the SDK's generic isError conversion of audit.ts's own
message on BOTH paths, so a refusal's own policy reason becomes invisible
to the caller exactly when the audit log is unhealthy. Exactly 2 of 21
tests failed under that mutation, precisely the two targeting this gap, no
collateral.
Edge-case pass added to the smoke test (12 new tests, each on its own
dedicated server instance):
- loadConfig() / the built server's behaviour with no key material
configured at all (clear throw; process exits non-zero and promptly,
never hangs, never writes anything to stdout).
- No mnemonic word or the derived private key appears in the startup
banner or in any tool response across a sweep that includes a real
successful sign.
- decode_transaction never writes an audit entry, even for an in-policy
transaction.
- An undecodable transaction passed to sign() never reaches evaluate(), so
there is nothing to audit yet - by design, pinned rather than assumed.
- get_spend_limits surfaces the concrete refusal reason, not just the
word REFUSED.
- The hard requirement itself: a control test proving the in-policy
fixture genuinely signs under a healthy audit log, then the same
transaction against an unwritable audit log withholding the signature,
then a pin of the (intentional, conservative) consequence that the
withheld signature still counts toward this process's own committed-
today total even though nothing was ever released.
- The refused-path decision under the same unwritable-audit-log condition.
Also reworded decode_transaction's response text ("(read-only)" /
"would be ALLOWED" instead of "(not signed)" / "would be signed") so it
never contains the substring "signed" even when a decoded transaction
happens to be in-policy - matches this same smoke test's own
`!/signed/i.test(text)` check on the brief's decode test, which otherwise
fails against the brief's own unmodified wording on every call.
typecheck 0, test:unit 180/180, builder test:smoke 26/23/0, boundary 4/4,
signer test:smoke:signer 21/21 (9 brief + 12 edge-case pass).
stdio transport only - no HTTP listener, no Provider, no outbound network.
Wires together decode.ts/policy.ts/sign.ts/audit.ts/keysource.ts (Tasks 2-6)
behind four tools: vector_signer_get_address, vector_signer_decode_transaction,
vector_signer_sign, vector_signer_get_spend_limits.
Two corrections needed beyond the brief's given code to reach its own stated
"pass 9, fail 0" outcome, both verified necessary by reproducing the failure
first:
- packages/signer/tsup.config.ts: target bumped es2020 -> es2022. index.ts's
top-level `await server.connect(...)` is ES2022 syntax; esbuild hard-errors
("Top-level await is not available in the configured target environment")
under the package's original es2020 target rather than attempting an unsafe
downlevel transform. Scoped to the signer package only - the builder never
uses top-level await and its own tsup.config.ts is untouched.
- tools.ts's decode_transaction response text: "(not signed)" and "would be
signed" both contain the substring "signed", which collides with this same
smoke test's own `!/signed/i.test(text)` check on every call, including the
refused case. Reworded to "(read-only)" / "would be ALLOWED" - same meaning,
no collision. Fixed the response text rather than loosening the test, which
stays exactly as given.
Brief's own smoke test (9 tests) passes verbatim. Workspace-wide: typecheck 0,
test:unit 180/180 (no regressions), builder test:smoke still 26/23/0.
The audit-append-failure handling required by the task (a produced signature
must never be released if its audit record fails to write) is not yet in this
commit - see the follow-up commit.
is_valid lives on Transaction rather than TransactionBody, so hardcoding it
was invisible to every hash-equality test - flipping it passed all 21. For a
deliberately-invalid script transaction that silently rewrites the claim a
node re-checks in phase 2.
Also pins the indefinite-length CBOR round trip that the trailing-bytes guard
depends on, which until now was asserted only in a comment.
from_cbor_hex only requires a valid prefix, so appending bytes to a real
transaction let sign.ts silently sign a shorter transaction than the caller
supplied and return a confident result — a signature over different bytes
than what was asked for, with nothing to reveal the discrepancy. sign.ts
calls from_cbor_hex independently of decode.ts and does not inherit that
module's consumption check (commit 5da4447) automatically; this closes the
same gap here, with the identical case-insensitive comparison and reasoning.
Also closes an untested claim: decode.ts's own suite documents but never
pins that the case-insensitive comparison tolerates legitimate uppercase-hex
input without a false positive. Pinned here for sign.ts.
Edge-case pass beyond the brief's 7 tests, all reproduced against the
unmodified implementation before any fix and mutation-verified after:
- trailing bytes silently accepted (BUG, fixed above)
- existing vkey witnesses are preserved and appended to, not replaced
(already correct; mutation-tested by dropping the copy loop)
- two different keys produce two independently-verifying witnesses,
chained or independent (already correct)
- determinism strengthened to compare raw signature bytes across a
freshly re-derived key instance, not just the memoized one (already
correct)
- auxiliary data / metadata survives signing byte-for-byte, body hash
unchanged (already correct)
- no thrown error, on any path, ever echoes the private key value
(already correct; mutation-tested by deliberately leaking it)
Lucid's constructor requires a Provider, which would put a network object in
the signer. The CML path was verified to produce an identical tx hash and
byte-identical signed output. Preserves any pre-existing witnesses.
TypeScript `private` is compile-time only, so JSON.stringify or console.log of
a KeySource printed the mnemonic or signing key in full - contradicting this
module's own "never via a log line" invariant, and one natural mistake away
in a package that already logs diagnostics to stderr.
Native #private fields plus toJSON/inspect overrides. Also pins the default
account index, which no test previously required to be 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
walletFromSeed is pure derivation and needs no Provider, keeping this
module network-free. MnemonicKeySource is the familiar default;
PrivateKeyKeySource is strictly safer since it controls one account
rather than the whole HD tree. describe() is asserted never to leak key
material.
Verified rather than assumed: walletFromSeed's private key prefix is
ed25519e_sk1 and its address is addr1... (Base, Mainnet default) - the
brief's regex already matched. The brief's test mnemonic passes BIP39
checksum validation.
Hardened beyond the brief after an edge-case pass:
- Mnemonic case: bip39's wordlist lookup is case-sensitive, so an
uppercase or mixed-case mnemonic threw outright. Casing carries no
BIP39 meaning, so lowercasing is safe - now applied, and verified to
derive the identical address rather than merely "some" address.
- PrivateKeyKeySource validated the trimmed key/address but stored the
original untrimmed value - the same knownAddresses() bug shape as
policy.ts (validate trimmed, store padded, silently fail every later
exact-string comparison). Now stores what was validated.
- Empty mnemonic input reported "got 1" word (''.split(' ') === ['']),
not "got 0" - message accuracy only, both branches already rejected.
Documented, not fixed: PrivateKeyKeySource cannot verify that `address`
is actually controlled by `privateKeyBech32`. That needs CML, and this
module deliberately imports only @lucid-evolution/wallet, whose one
raw-private-key function requires a live Provider. The failure is
bounded if misused - signing needs no address, so a mismatch surfaces as
an on-chain rejection, and policy.ts's own-address matching only ever
over-refuses on a wrong address, never under-refuses.
29 tests beyond the brief's 10. 5 of the fixes above were mutation-tested
(each reverted and confirmed to fail red before restoring the fix).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A malformed amount followed by valid entries silently dropped every later
entry when the loop aborted instead of skipping - 30/30 tests passed against
that mutation because every bad-amount case put the bad entry last.
Persistence now writes to a temp file and renames, so a kill mid-write costs
at most the in-flight entry rather than the whole day's history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records intent at signing time; the signer has no network so it cannot
observe submission. Refusals are recorded but never consume daily budget.
Hardened beyond the brief after an edge-case pass found two genuine
under-report vectors in committedTodayLovelace() (the function the daily
limit's alreadySpentTodayLovelace comes from): a negative netOutflowLovelace
string parses as a valid negative bigint and would silently subtract from the
total, and a null element in the on-disk array crashes the summation outright
rather than being skipped. Both are neutralized. Also: load() now reports a
non-array log file on stderr instead of resetting silently, recent(0) no
longer returns the whole log (Array.prototype.slice(-0) is slice(0), not "no
entries" - a plain JS gotcha), and append() now throws on a write failure
instead of swallowing it, since a swallowed failure is a normal-looking
return while the entry silently exists only in memory until the next
restart's reload finds it missing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
knownAddresses validated the trimmed value but stored the original, so an
own address configured with an incidental leading or trailing space passed
the fail-closed gate yet could never match its own outputs - every change
output was then counted as outflow and normal transactions were refused.
Also pins prefix-match, trim-before-check, and case-sensitivity, none of
which any test previously required.
Fix 1 (Important, live bypass): the blank-address fix from the prior commit
used .trim().length > 0, which only strips the ECMAScript WhiteSpace set
(NBSP U+00A0, BOM U+FEFF, etc). U+200B ZERO WIDTH SPACE and U+2060 WORD
JOINER are Unicode category Cf (format), not Zs, so trim() leaves them
untouched - ownAddresses: ['\u200B'] passed the filter, bypassed the
fail-closed guard, and reproduced allowed:true against the shipped code
(verified directly, not just against a mutant). Replaced the blank-only
filter with a positive check: an entry only counts as a known own address if
it starts with 'addr' after trimming. A denylist of invisible codepoints is
unwinnable; this rules out anything built from them without enumerating any.
Applies at both call sites (evaluate's guard and computeOutflow's Set) via
the shared knownAddresses() helper, as it already did.
Fix 2 (Important, surviving mutant): added a regression test that exercises
computeOutflow's own Set-construction filtering directly, independent of
evaluate's separate guard - a blank/invisible output address paired with a
matching blank/invisible ownAddresses entry must still count as outflow, not
be matched as change. Nothing previously exercised this combination: every
prior blank-entry test either went through evaluate (whose guard would
refuse first) or paired a blank ownAddresses entry with a real output
address. Confirmed this test now fails if computeOutflow's Set reverts to
unfiltered while evaluate's guard stays filtered (re-verified in this
session; see the report addendum for output).
Fix 3: added a doc comment on evaluate() stating plainly that it bounds
lovelace only - assetMovements is reported for visibility and never gates
allowed. No interface change; this is documentation of the existing,
deliberate v1 scope boundary already flagged in the task report, not new
behavior.
Re-ran the full mutation set after all three fixes: inverted classification,
removed fail-closed guard entirely, guard reverted to raw
ownAddresses.length === 0, and computeOutflow's Set left unfiltered are all
caught. Full before/after evidence in the report addendum.
Adversarial pass on the spend policy found that ownAddresses.length === 0
does not catch a non-empty array containing only blank entries - a realistic
outcome of ''.split(',') on an empty config/env value, which yields [''] not
[]. A blank entry never matches a real output address, so it degrades to
"every output looks foreign" rather than permitting anything, but it bypassed
the explicit fail-closed path and its message, and if the resulting
over-counted outflow still happened to land under the configured limits the
transaction would be signed without any real own address ever having been
known - exactly the condition that check exists to refuse.
knownAddresses() filters blank entries before both the Set used for
classification and the fail-closed emptiness check, so an all-blank list now
refuses the same way a literally empty one does. A blank entry alongside a
real address is unaffected (regression-tested).
Also adds the rest of the adversarial pass: exact-limit boundaries one
lovelace either side, many-small-outputs summation, aggregation correctness
(duplicate own addresses, repeated foreign addresses, multi-asset and
multi-output asset movements), check-ordering pins, zero-limit and
already-over-budget edge cases, and a pinned note that SpendLimits governs
lovelace only - native asset movements are tracked but not limited.
Every core comparison was verified to actually fail on injection (inverted
own/foreign check, dropped fee, dropped daily history, off-by-one on both
limit comparisons) before being reverted; see the task report.
Net outflow is every output that does not return to us, plus the fee - more
accurate than summing declared amounts, and computable without chain access.
Refuses when no own address is known, since change cannot then be identified.
The mechanism explanation is worth keeping; the snapshot of which fixtures
were tested on which day is commit-message content that goes stale silently.
from_cbor_hex only requires a valid prefix, so extra bytes were silently
dropped and decode returned a confident, fully-populated result - the exact
fail-open the module exists to prevent, and policy trusts this output.
The signer's tsconfig includes test files, and they import source with an
explicit .ts extension (which tsx resolves at runtime). Without this flag
tsc exits 2, which fails the blocking typecheck gate in CI. Safe because
noEmit is set; the alternative - excluding tests - would leave them
unchecked.
