Prepare safe signal-to-trade demo for Stanford - #81
Conversation
…ier from blocking valid work
Four defects found reviewing this branch, each verified against the gateway
source or reproduced directly.
markets-validation: the Gamma blocklist rejected `search`, `sort`, `end_after`,
and `end_before` on polymarket/markets{,/keyset}. All four are spec-backed
Predexon filters (blockrun/src/lib/predexon.ts POLYMARKET_MARKET_PARAMS, where
P_SEARCH is documented as "this param is `search` here, NOT `q`"), so the
validator refused the exact query the demo needs — open BTC markets ending
after a date, sorted by liquidity — before payment. Narrowed to the params
that really are Gamma-only: active, closed, order, ascending.
markets-validation: markets/listings was hard-blocked as "no longer exposed",
but the gateway still registers it (predexon.ts:155), maps its params (:348),
routes it (:527), and advertises it as a priced Tier-1 endpoint in the x402
manifest. Unblocked and restored in the tool description and skill tables.
errors: `/5[0-9]{2}/` matched any bare 5xx-shaped number, so "max_tokens 512 is
above the limit" was reported as a temporary outage with "try again in a few
minutes" — hiding a validation bug behind retry advice. A 5xx now has to be
labelled as a status (adjacency required) or carry an HTTP reason phrase.
errors: dropping "api error after payment" from isServerError sent post-payment
upstream failures with no parseable status into the payment branch, telling
users to fund a wallet that was never the problem. Restored for the non-4xx case.
payment-serialization: the queue is process-global and each waiter awaited its
predecessor unconditionally, so one x402 call that never settled would wedge
every paid tool until restart. Waiters now give up after a bounded wait
(BLOCKRUN_PAYMENT_QUEUE_MAX_WAIT_MS, default 120s) — losing serialization on one
call beats a permanently stuck server.
Tests updated where they asserted the old behavior; 274 pass, typecheck and
build clean.
VickyXAI
left a comment
VickyXAI
left a comment
There was a problem hiding this comment.
Reviewed on the branch: npm run typecheck, npm test (270 → 274 pass), and npm run build are all green. The annotations split (blockrun_polymarket_read) and the market-buy depth / min-size work in orders.ts are genuinely good and I'd take them as-is.
I found four defects and pushed fixes for them to this branch (5fe6237). One larger item below is a product decision, not a bug, and I've left it alone.
1. Product decision that shouldn't ride along in a demo PR
src/utils/polymarket/constants.ts flips the default CLOB host from BlockRun's Finland egress to https://clob.polymarket.com. That isn't demo scaffolding — it removes the shipped 0.30.3 default for every existing user, and US/UK users lose order placement entirely.
Meanwhile the README this PR edits still leads with:
Read the odds and place the bet, from one self-custody wallet. … the ability to place real, USDC-settled bets on Polymarket
After this merge that's false by default for a large share of users. The Regions section was updated honestly, but the headline claims weren't reconciled with it.
Also: deploy/finland-egress/ is still in the tree. Every doc reference to it was stripped, so the script that deploys the egress now sits in the repo undocumented, contradicting the policy this PR states.
This may well be the right call for a Stanford stage — but it's a decision about the product's flagship capability, currently landing as line 40 of "Prepare safe signal-to-trade demo." Suggest splitting it out and deciding it on its own, then reconciling the copy and the deploy/ directory either way.
2. Fixed: the validator rejected valid Predexon params
GAMMA_STYLE_MARKET_PARAMS blocked search, sort, end_after, end_before on polymarket/markets{,/keyset}. All four are spec-backed Predexon filters — see blockrun/src/lib/predexon.ts POLYMARKET_MARKET_PARAMS, where P_SEARCH carries the comment "this param is search here, NOT q".
Net effect: the exact query this demo wants — open BTC markets ending after a date, sorted by liquidity — was refused before payment with a message claiming Predexon doesn't accept it. test/markets-validation.test.ts asserted that behavior, locking it in.
Narrowed the blocklist to what is genuinely Gamma-only (active, closed, order, ascending) and replaced the test with one that pins the opposite: Predexon's own filters must pass through.
3. Fixed: markets/listings is still a live paid endpoint
The validator returned "Predexon no longer exposes 'markets/listings'", but the gateway still registers it (predexon.ts:155), maps its params (:348), routes it (:527), and advertises it as a priced Tier-1 endpoint in the x402 .well-known manifest (route.ts:580).
One of the two repos is wrong and they can't both ship. Since the gateway is what users are paying against, I unblocked it and restored it in the tool description and skill tables. If it really is retired upstream, the gateway needs a matching PR first.
4. Fixed: formatError sold validation bugs as outages
Broadening hasStatus("500") to /5[0-9]{2}/ matched any bare 5xx-shaped number. Reproduced:
IN : Model rejected: max_tokens 512 is above the limit
OUT: … This is a temporary API issue. … Try again in a few minutes.
512 is everywhere in LLM errors (token limits, embedding dims). A 5xx now has to be labelled as a status — with adjacency required, so context length 512 exceeded doesn't qualify — or carry a standard HTTP reason phrase.
5. Fixed: post-payment outages told users to fund their wallet
Dropping "api error after payment" from isServerError meant a post-payment failure with no parseable status fell through to the payment branch:
IN : API error after payment: upstream provider unavailable
OUT: … your wallet needs funding. Run blockrun_wallet action:"setup" …
That's precisely the confusion this file exists to prevent. Restored for the non-4xx case; the 4xx tightening you added is kept and still tested.
6. Fixed: one hung call could wedge every paid tool
serializePaidRequest is a module-global promise chain and each waiter awaited its predecessor unconditionally, with no timeout. A single x402 call that never settles blocks every subsequent markets / surf / rpc / defi / price call for the life of the process — restart is the only recovery.
Waiters now give up after a bounded wait (BLOCKRUN_PAYMENT_QUEUE_MAX_WAIT_MS, default 120s). Losing serialization on one call beats a permanently stuck server.
Left for you — not changed
- Serialization covers 5 of the paid tools.
chat,search,exa,image,video,music,speech,modal,phoneall still fire concurrently from the same wallet, so the x402 race this guards against is still reachable. I did not extend it, because serializing the 60–180s async media tools would be actively worse. Worth deciding deliberately: either scope the guard to the fast paid-data tools and say so, or find a wallet-level fix. destructiveHint: trueon read-only paid data tools (markets,search,exa,surf). In the MCP spec that hint means irreversible mutation; clients honoring it will prompt for approval on every data query — which cuts against this demo. "Costs money" and "destructive" aren't the same axis. ConsiderreadOnlyHint: trueplus documenting cost, unless per-call approval is the intent.- Smart-money cohort thresholds (≥100 trades / ≥$1000 PnL / ≥0.15 ROI) are invented client-side from one observed 400, with no override. The gateway has no param spec for that path, so this is guesswork that will reject legitimate narrower cohorts.
- Candlesticks
intervalis now mandatory. Validating the value when present is safe; requiring it may break a working server-side default. Worth confirming against a live call. docs/added to npmfilesships the internal Stanford runbook to everynpx @blockrun/mcpconsumer.- Tool count 19 → 20 desyncs from
blockrun/brand-numbers.jsonandawesome-blockrun/brand-numbers.json, both still at 19. Needs companion PRs at release time.
…hange into its own PR The default CLOB host flip (Finland relay -> direct clob.polymarket.com) removes order placement for every US/UK user by default — the repo's headline capability — and it was landing as a side effect of demo prep. Reverted here so this PR ships only the annotations, the read-only tool, pre-payment validation, and the demo material. The egress question gets decided on its own merits. Restored to main: CLOB_HOST default, setup's blocked-region guidance, the 403 mapClobError text, the tool description, the README Regions paragraph and env table row, the setup guide's §3 and troubleshooting rows, and the skill's regions section (plus the test that asserted the new 403 wording). Kept from this branch, since they are independent of the default: the blockrun_polymarket_read flow through the docs and skill, markets/search-based discovery, and client.ts's comment cleanup. The Stanford runbook now sets POLYMARKET_CLOB_HOST=https://clob.polymarket.com explicitly on the presentation machine — with the Finland default restored, setup would otherwise report a permitted region from the venue and the demo's region-honesty claim would not hold. Also fixed the Claude Code install snippet, which passed POLYMARKET_MAX_SESSION_USD without its own -e flag. package.json: drop `docs` from files, so the internal Stanford runbook stops shipping to every npx consumer (verified: 0 docs entries in npm pack). 274 tests pass, typecheck and build clean.
|
Pushed Reverted to main (the parts tied to the default host): Kept, since none of it depends on the default: the The egress question now lives in #83 with the full list of surfaces that have to move together, including Two things worth a look: The Stanford runbook now sets The Claude Code install snippet passed Also dropped 274 tests pass, typecheck and build clean. Still open from the review, all yours: serialization covering only 5 of ~12 paid tools, |
…nverified API contracts Closes the four items left open by the review of this branch. Annotations described price, not effect. `destructiveHint` means "irreversible update" and is only meaningful when `readOnlyHint` is false — it says nothing about money, and MCP has no cost hint. Marking blockrun_markets/search/exa/surf destructive because they settle $0.0095 makes every annotation-honoring client demand approval for a plain lookup, which cuts hardest against the research and demo flows this branch adds. Spend control already lives in the budget ledger (BLOCKRUN_BUDGET_LIMIT, reserveBudget, per-agent delegation). Reassigned by what a tool actually does: - paid reads (markets, search, exa, surf, defi, price) -> readOnly + openWorld - generative (chat, image, video, music, speech, realface) -> write, not destructive; the shape MCP defines for a benign write - rpc -> destructive. It was `paidPrivate`; eth_sendRawTransaction can broadcast a signed transaction, so this is an upgrade, not a relaxation - modal -> destructive. Executes arbitrary code in a sandbox Verified over stdio tools/list, not just at the registration call: 20 tools, 4 destructive (modal, phone, polymarket, rpc). Tests now pin the full matrix and fail if a new tool ships unannotated — which under the spec would silently default to destructive. Serialization scope made deliberate rather than accidental. Added the two fast paid data tools that were missed (search, exa) and documented why the long-running ones are excluded: queueing a 5ms price lookup behind a 3-minute video render trades a rare settlement race for a guaranteed stall. The race is still reachable across a media call and a data call; that needs a wallet/nonce fix, not a queue. Two validator rules were enforcing guesses, and a wrong pre-payment rule blocks an endpoint users are paying for: - candlesticks: "1h fails" is observed, "interval is mandatory" is not. Now validates the value only when one is supplied. - smart-money: the observed failure was the UNFILTERED call. Now requires any recognized cohort filter instead of invented magnitudes (min_trades >= 100, PnL >= $1000, ROI >= 0.15), which would have rejected a 7d window or a 20-trade cohort with no override. 278 tests pass, typecheck and build clean.
|
Pushed Annotations now describe effect, not price
Reassigned by what each tool actually does. Two are now stricter than before:
Verified over a real stdio Serialization scope is now a decision, not an accidentAdded the two fast paid data tools that were missed ( Two validator rules were enforcing guessesA wrong pre-payment rule blocks an endpoint users are paying for, so both now match what was actually observed:
brand-numbers 19 → 20 — deliberately NOT changed hereNot an oversight.
Everything from the review is now either fixed here or tracked in #83. |
BlockRunAI#86 added the br:mcp.tools marker to CONTRIBUTING.md with the then-current 19. This branch ships a 20th tool, so the merge was textually clean but left the marker stale, and CI's 'Brand numbers' step failed on it. That step runs scripts/sync-brand-numbers.mjs --check, which npm test does not cover — local green was not enough to catch this.
|
Status check — every change requested in my review is now resolved, all of it on this branch:
CI green on Two things need you, not me:
Also still open by design: #83 (egress decision). |
All requested changes are resolved on this branch (see the status comment). Dismissing my own review because I authored the fixes, so it is no longer an independent signal — this PR needs a reviewer who did not write it.
…, and pre-payment validation (#87) Releases #81. Adds blockrun_polymarket_read (20th tool), annotates every tool by effect rather than price, validates Predexon request contracts before the x402 payment is created, and serializes paid data calls from one wallet. Gates: 279 tests, typecheck, build, brand-numbers --check, Polymarket read-only e2e, and a stdio smoke confirming 20 tools / 9 in the trading profile / 4 destructive.
Two of the three rules were wrong, and both charged users for a guaranteed failure. markets/listings settles a payment and THEN returns 410 Gone. I unblocked it in 0.33.0 because blockrun's registry still lists, prices, and advertises it — but the gateway only proxies, and Predexon retired the route. Registry presence is not evidence that a route serves. #81 was right; 0.33.0 was the regression. window is not a smart-money cohort filter. No params 400s; { window: '7d' } alone ALSO 400s; { min_trades: '100' } alone succeeds with window defaulting to all_time. 0.33.0 counted window as a filter and passed a certain paid 400 through. Now requires a smart-wallet criterion, and names the reason when only window is present. The candlestick interval whitelist is removed. On one market: no interval succeeds, 1440 succeeds, 1h 422s, 60 returns a paid 400. Which integers a market serves is data-dependent, so a numeric whitelist blocks valid calls AND lets paid failures through. Only shape is checkable client-side. Tool description and prediction-markets skill corrected. 279 tests, typecheck, build, brand-numbers --check green.
…#88) * 0.33.1 — live-probe the pre-payment rules 0.33.0 shipped on inference Two of the three rules were wrong, and both charged users for a guaranteed failure. markets/listings settles a payment and THEN returns 410 Gone. I unblocked it in 0.33.0 because blockrun's registry still lists, prices, and advertises it — but the gateway only proxies, and Predexon retired the route. Registry presence is not evidence that a route serves. #81 was right; 0.33.0 was the regression. window is not a smart-money cohort filter. No params 400s; { window: '7d' } alone ALSO 400s; { min_trades: '100' } alone succeeds with window defaulting to all_time. 0.33.0 counted window as a filter and passed a certain paid 400 through. Now requires a smart-wallet criterion, and names the reason when only window is present. The candlestick interval whitelist is removed. On one market: no interval succeeds, 1440 succeeds, 1h 422s, 60 returns a paid 400. Which integers a market serves is data-dependent, so a numeric whitelist blocks valid calls AND lets paid failures through. Only shape is checkable client-side. Tool description and prediction-markets skill corrected. 279 tests, typecheck, build, brand-numbers --check green. * perf: remove the process-global paid-call queue 0.33.0 added serializePaidRequest on the reasoning that concurrent x402 authorizations from one wallet can race at the settlement layer. Measured, it protects nothing. Base mints a fresh random 32-byte nonce per payment (x402.ts:235), so two concurrent authorizations produce different EIP-712 signatures and cannot collide. The real Solana collision — deterministic signatures from a shared blockhash — is handled inside @blockrun/llm 3.8.4, and its check-and-add (findDistinctTx then issuedFor().add()) is synchronous with no await between, so it is concurrency-safe without any caller queue. The one place concurrency genuinely broke accounting is chat's settled-cost delta, fixed with a fresh non-cached client (wallet.ts:214) — and chat was never in the queue. Cost of keeping it, measured on 4 concurrent markets/search calls: concurrent 2392ms ok=4/4 serialized 6417ms ok=4/4 +4025ms (2.68x) The unserialized arm returned 4/4 clean, which is itself evidence there was no race to serialize against. Removes the module, its tests, and the tool-description and skill copy that advertised the behavior. Closes #89. 276 tests, typecheck, build green. * fix: close the validator bypass, and require the SDK version the queue removal relies on Review of this PR (Codex + three independent agents) found the headline fix didn't hold and the queue removal's justification wasn't enforced. Both confirmed by direct probe before fixing. Every rule was bypassable by decorating the path. validateMarketRequest stripped only outer slashes and matched case-sensitively, so markets/listings?venue=polymarket, Markets/Listings, markets//listings, a trailing tab, and a #fragment all sailed past and settled a payment for the exact failure the rule exists to prevent. The repo already had the answer 13 lines away: normalizeClassifyPath in path-safety.ts drops query/fragment, strips slashes, and lowercases, and its own doc comment describes this hazard for the price tables. Reused here, plus control-character strip and interior slash-run collapse. (The price-classification path still shares the un-collapsed helper; noted in-code, out of scope here.) The dependency floor did not require the fix the CHANGELOG credits. ^3.6.1 admitted seven published versions with no payment-distinctness, and the lockfile pinned the oldest. In 3.6.1 createSolanaPaymentPayload has a fixed compute-unit price and no nonce, so two concurrent same-price Solana calls in one blockhash window build byte-identical transactions — precisely what the deleted queue guarded, on the version CI actually installs. Raised to ^3.8.4; lockfile now resolves 3.9.0, verified to retain findDistinctTx/issuedFor. The benchmark that justified the removal was necessarily Base-only, since this branch's node_modules was 3.6.1. It supports "Base cannot collide"; it never supported the Solana half. Said so in the CHANGELOG rather than leaving the number to imply more than it measured. Smart-money accepted an unusable criterion: `{ min_trades: "" }` passed a presence-only `in` test and 400s upstream exactly like no filter. Now requires a non-blank value, matching the orderbooks rule ten lines up. The error message no longer asserts all seven criteria work — only min_trades was probed alone; the rest are named as accepted-by-symmetry. Docs were teaching the broken values. Five code blocks across two skills still handed out interval "60" — the value this release documents as a paid 400 — while only the prose was updated; models copy the block. The parallel-calls guidance I added was Base-only truth stated universally (the Solana payload has no nonce field), and it contradicted signal-to-trade-demo, which still said "do not launch in parallel". stanford-trading-demo still listed "serialized x402 calls" as a live safety property. All corrected and made chain-aware. CHANGELOG claimed 279 tests; the branch had 276 (the count predated deleting payment-serialization.test.ts in the same diff). Also softened the unconditional "check-and-add is synchronous" claim — the exhausted-nonce branch does await, though it is unreachable below 65 identical payments per blockhash. Tests: added coverage for every confirmed bypass, all seven criteria (dropping four previously kept the suite green), unusable values, and the Binance route that must keep "1h". Mutation-checked — reverting the normalization, dropping criteria, or restoring the presence-only check each fails the suite. 279 tests, typecheck, build, brand-numbers --check green. * chore(release): retarget this branch at 0.36.0 on current main Opened as 0.33.1 off 0.33.0; main is now 0.35.0, so the release number moves to 0.36.0. Minor rather than patch: the paid-call queue removal changes runtime behavior for every paid data tool, not just its validation rules. Lockfile regenerated from package.json rather than taking either side of the rebase conflict; @blockrun/llm resolves to 3.9.0, satisfying the ^3.8.4 floor this branch requires for Solana payment distinctness. Re-ran the whole gate on this baseline: 282 tests, typecheck, build, brand-numbers --check, CLI-loads-without-sharp, and verify:prices (0 routes under-reserving; the 19 over-reserving by $0.001 are the known gateway tx-fee drop, unrelated to this branch).
2 participants
Summary
Validation
Safety / demo scope
No new live order was submitted. Polymarket's official US geoblock prevents a confirmed trade from the Stanford venue, so the live presentation is designed to show current signals and an executable order dry-run, with explicit confirmation still required for any future write.
Draft for review.