Pyth template completion, MCP token-bloat fix, honest 404s for /v1 - #1
Draft
92Infinitus92 wants to merge 5 commits into
Draft
Pyth template completion, MCP token-bloat fix, honest 404s for /v1#192Infinitus92 wants to merge 5 commits into
92Infinitus92 wants to merge 5 commits into
Conversation
The pyth-price-feed-v2 template exposed only feed_id and price, so scenarios could not express staleness or confidence-band cases that lending/PMM logic actually reads. Adds the remaining PriceFeedMessage fields (conf, exponent, publish_time, prev_publish_time, ema_price, ema_conf) and posted_slot as template properties, and extends llm_context with field semantics, a staleness recipe, and the Surfnet clock rules (timeTravel takes milliseconds, the simulated clock only moves forward). Covered by a forging unit test against a real 134-byte PriceUpdateV2 account: all eight writable fields change to values that differ from the fixture's, bytes before price are asserted untouched, trailing padding is preserved.
absoluteTimestamp is consumed in milliseconds while the Clock sysvar exposes seconds, and jumps are forward-only; the doc comment said neither, which cost a debugging session to rediscover empirically.
get_override_templates inlined every constant option (hundreds of price feeds,
duplicated across templates), producing a ~1.86 MB payload that overflowed LLM
context windows and TPM limits. Constants are now summarized as
{label, description, optionsCount} — 85x smaller measured — and the new
search_constant_options tool resolves concrete values via case-insensitive
search with stable sorted paging (MAX_SEARCH_RESULTS) and self-correcting
errors for unknown template ids or constants. The str:///override_templates
resource shares the same serialization through compact_template_json, so both
MCP surfaces stay compact.
Adds the first tests of the mcp crate: tool error paths, case-insensitive
matching, truncation reporting, and a guarantee that no template leaks inlined
options.
…llback
The studio SPA catch-all answered any unknown GET — including API misses like
GET /v1/scenarios/{id}, which has never existed — with index.html and HTTP 200.
Clients then failed deep in JSON parsing ("Unexpected token '<'") instead of
seeing a clean 404, which is how a tag-wiping frontend bug shipped unnoticed.
Unknown /v1/* paths now get {"error":"not found"} with 404; page routes and
static assets keep the SPA fallback.
Route registration moved into configure_api, shared between the server and the
new HTTP test, so the load-bearing registration order (real endpoints before
the /v1 catch-all scope) is asserted on the exact code production runs.
Collaborator
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+290
to
+319
| #[actix_web::test] | ||
| async fn unknown_v1_paths_return_json_404_instead_of_spa_fallback() { | ||
| let loaded_scenarios = Data::new(RwLock::new(LoadedScenarios::new())); | ||
| let app = test::init_service( | ||
| App::new() | ||
| .app_data(loaded_scenarios) | ||
| .configure(configure_api) | ||
| .service(surfpool_studio_ui::serve_studio_static_files), | ||
| ) | ||
| .await; | ||
|
|
||
| for path in ["/v1/scenarios/some-id", "/v1/nonexistent"] { | ||
| let request = test::TestRequest::get().uri(path).to_request(); | ||
| let response = test::call_service(&app, request).await; | ||
| assert_eq!(response.status(), 404, "expected 404 for {path}"); | ||
| assert_eq!( | ||
| response.headers().get("content-type").unwrap(), | ||
| "application/json", | ||
| "expected JSON body for {path}" | ||
| ); | ||
| } | ||
|
|
||
| let request = test::TestRequest::get().uri("/v1/scenarios").to_request(); | ||
| let response = test::call_service(&app, request).await; | ||
| assert_eq!( | ||
| response.status(), | ||
| 200, | ||
| "registered endpoints must keep working" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The catch-all uses web::route() (no method filter), so all HTTP methods on unknown /v1/* paths correctly return a JSON 404. The test only exercises GET requests, which means the case where a client sends POST or DELETE to a mistyped URL is not covered. Adding at least one non-GET assertion would solidify the guarantee that the scope's default_service is truly method-agnostic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli/src/http/mod.rs
Line: 290-319
Comment:
**Test only covers GET; POST/DELETE method mismatches on unknown paths are untested**
The catch-all uses `web::route()` (no method filter), so all HTTP methods on unknown `/v1/*` paths correctly return a JSON 404. The test only exercises GET requests, which means the case where a client sends POST or DELETE to a mistyped URL is not covered. Adding at least one non-GET assertion would solidify the guarantee that the scope's `default_service` is truly method-agnostic.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Collaborator
Author
There was a problem hiding this comment.
Valid — added a POST assertion in c2c4e65.
Review feedback: the guard's default_service is method-agnostic, but the test only exercised GET. A POST to an unknown /v1 path now asserts the JSON 404 too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four changes, one commit each:
and posted_slot are now overridable, so scenarios can express staleness and
confidence cases. Unit-tested against a real PriceUpdateV2 account.
inlines hundreds of constant options (1.86 MB → 22 KB; it overflowed LLM
context and TPM limits). The new search_constant_options tool resolves
values on demand. First tests in the mcp crate. The /v1/scenarios/templates
HTTP endpoint (studio dropdowns) intentionally keeps full data.
HTTP 200, which masked client/API mismatches as JSON parse errors. Route
registration is shared between server and test via configure_api.
forward-only — neither was documented.
Testing: cli 19/19, mcp 5/5, core pyth 3/3; fmt and clippy clean; live
e2e against a running surfnet.
Open questions
tool parameter?
UIs must not depend on it.
older MCP tools, dead Arc app_data in cli/http, HEAD 404s on GET routes)?
Greptile Summary
This PR bundles four focused improvements: the Pyth
PriceUpdateV2override template grows from 2 to 9 overridable fields (with comprehensive unit tests); the MCPget_override_templatestool stops inlining all constant option lists and instead summarises them by count, while a newsearch_constant_optionstool resolves values on demand (dropping payload size from ~1.86 MB to ~22 KB); unknown/v1/*paths now return a JSON 404 instead of silently falling through to the SPA'sindex.html; and thesurfnet_timeTraveldocs are corrected to document millisecond units and forward-only movement.compact_template_jsonreplaces full constant lists with{label, description, optionsCount}summaries; the newsearch_constant_optionstool provides paginated, case-insensitive search withtruncated/totalMatchessignalling, and both the tool andstr:///override_templatesresource now share the compact serialiser.configure_apicentralises route registration and appends a/v1scopedefault_servicethat returns a structured JSON error, backed by an actix-web integration test that confirms known routes still return 200.conf,exponent,publish_time,prev_publish_time,ema_price,ema_conf,posted_slot) are exercised by a byte-level fixture test that reads each override back through the Borsh-decoded account.Confidence Score: 4/5
Safe to merge; all four changes are well-scoped, tested, and do not touch critical paths like auth or data persistence.
The MCP token-reduction work is the most complex piece: the compact serialiser and search tool are correct and well-tested. The one open question is whether the verbatim metadata field in search results could partially offset the token savings for option-rich templates, but this is a tuning concern rather than a functional defect. The /v1 JSON-404 fix uses actix-web routing correctly and the accompanying test confirms both the catch-all and the known-route case.
Files Needing Attention: crates/mcp/src/surfpool/mod.rs — the metadata field in search results is worth a second look if options carry large blobs.
Important Files Changed
Sequence Diagram
sequenceDiagram participant LLM as LLM Agent participant MCP as MCP Server participant Reg as TemplateRegistry LLM->>MCP: get_override_templates() MCP->>Reg: registry.all() Reg-->>MCP: "Vec<OverrideTemplate>" MCP->>MCP: "compact_template_json(t)<br/>(constants → {label, description, optionsCount})" MCP-->>LLM: "compact JSON (~22 KB)<br/>constants have no inlined options" LLM->>MCP: search_constant_options(templateId, constant?, query) MCP->>Reg: find template by id Reg-->>MCP: OverrideTemplate MCP->>MCP: "filter options (case-insensitive)<br/>collect up to MAX_SEARCH_RESULTS=20" MCP-->>LLM: "{results, totalMatches, truncated}" LLM->>MCP: "create_scenario(templateId, values{feed_id: value})" MCP-->>LLM: scenario URLComments Outside Diff (1)
crates/mcp/src/surfpool/mod.rs, line 584-591 (link)metadataincluded verbatim may undercut the token savingsEach search result includes
"metadata": opt.metadatawithout any summarisation. For Pyth price-feed options this field likely contains extra chain/market data; atMAX_SEARCH_RESULTS = 20results the aggregate metadata blob could be non-trivial. Since thevaluefield is the only partcreate_scenarioactually needs, it's worth deciding whether metadata should be omitted or summarised (e.g. its key names only), similar to how constant options are now summarised incompact_template_json.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(cli): return JSON 404 for unknown /v..." | Re-trigger Greptile