Conversation
There was a problem hiding this comment.
Lots of cruft was gathering in the other PR, thanks for fixing and getting this up on its feet!
I realize this is still in Draft mode, not yet ready for review, so leaving this as a comment.
Just one structural thing - kindof a nit but makes it a lot easier to grok the repo and see when changes are just visual, can you please move all the gui things into one top-level gui directory?
So, /app becomes /gui (or a subdirectory) and /packages/gui also moves into /gui somewhere?
We'll still need some other top-level changes in this PR, so we can hook it all together, but that'll make it much easier to review and understand going forward
I'm also not a big fan of their default naming, src-tauri is confusing, I'm biased towards the way I named things in https://github.com/Lay3rLabs/WAVS/tree/c097a189082e04b12be942e5f2dc67253c7e325a/packages/gui with:
- backend
- frontend
- media
- shared
but it's not a showstopper... and as long as all this stuff is self-contained in /gui, we can use the tauri defaults and vibecode it and treat it as "if it looks right it is right"
JakeHartnell
force-pushed
the
wavs-app
branch
from
685c2a1 to
4f8a0a0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dakom
left a comment
dakom
left a comment
There was a problem hiding this comment.
This was introduced in #1126.
@copilot Not requesting code changes — looking for an architectural evaluation of packages/wavs-mcp
Please comment with your review, do not make any changes:
- How much of the MCP server's functionality duplicates what a generic OpenAPI/Swagger-to-MCP bridge would provide, given that the WAVS HTTP API is already documented via OpenAPI?
- Whether embedding private key handling (WAVS_CHAIN_WRITE_CREDENTIAL, WAVS_SIGNING_MNEMONIC) in an MCP server — where an AI can autonomously send on-chain transactions — is a good security posture.
- The scaffold tool generates WASM component boilerplate, but components are generic WASM concepts — it's services (which tie triggers to workflows and components) that are WAVS-specific. Does the scaffolding actually help with the WAVS-specific part, or is it just generating generic WASM component code that doesn't reflect how services are configured and deployed?
- Whether this package would be better maintained separately from the core WAVS repo, to avoid coupling MCP tooling to the core release cycle.
|
On 1 — OpenAPI bridge overlap A generic OpenAPI-to-MCP bridge exposes raw HTTP semantics: JSON request bodies, path parameters, auth headers. That works for exploration, but it means the AI is constructing AddServiceRequest JSON blobs and managing auth headers manually — not a great experience. The MCP tools provide a curated layer above that: each tool has a typed schema, a description that communicates intent and preconditions, and unified credential management across all operations (the distinction between --token for node auth vs --chain-write-credential for on-chain ops isn't obvious from an OpenAPI spec). wavs_deploy_dev_service is a concrete example: it orchestrates a two-step save-then-register flow that the raw API exposes as separate calls with no obvious ordering guidance. More importantly, the chain-write and local tooling (7 of 22 tools) can't be bridged from OpenAPI at all — they're EVM contract interactions and local build operations. Users need this package anyway. Splitting "use the generic bridge for these 15, use the MCP package for these 7" is a worse DX than a single coherent toolset. On 2 — Private key handling This is opt-in. The MCP server operates fully without credentials for read-only use. Chain-write capabilities are only active if you explicitly provide --chain-write-credential. Same posture as any dev tool that can optionally sign transactions. On 3 — Scaffolding scope Fair point. On 4 — Repo coupling The MCP package imports wavs-types and utils from the workspace and embeds WIT files from the project tree. In the same repo, staying in sync is free — any API change that breaks the MCP tools fails immediately. Moving it to a separate repo turns that into a version matrix: publish wavs-types, cut a new MCP release, update the pin. That overhead only makes sense once there's a concrete reason (different release cadence, active external contributors who need isolation). Right now there isn't one IMO. |
I don't understand this point. There is no other way to communicate to the server except with these "raw HTTP semantics"... it's not a great experience for a robot, but that's okay. For a human using an agent - yeah, the API alone doesn't document intent - that's what OpenAPI/Swagger is for. Agents can consume that perfectly fine. If MCP is helpful here, my point isn't "don't add MCP", it's "don't re-invent the wheel, OpenAPI->MCP solutions already exist" But tbh it's not clear to me that MCP is actually more helpful than a simple markdown file telling the agent to consume the OpenAPI docs. (again - not saying "don't expose an MCP server", it's just that the http part of it isn't a huge win by itself) Is there something you wanted your agent to do, in terms of talking to the wavs http server, that it failed to do properly? Odds are the fix there is just better docs and publishing the OpenAPI docs cleanly.
What are these tools? WAVS doesn't know about any specific contract shapes. We deliberately decoupled that and it sits in different middleware repos, wavs-tools, etc. This was intentional to make sure we didn't accidentally bake in any assumptions in WAVS itself. In general our approach is not to talk to middleware contracts directly, but rather use the docker commands - and those docker images exist outside this repo. Similar but different to above, my point isn't "don't add MCP". This is a use-case where the agent legitimately won't know what the middleware images are capable of in a standard way (there's no OpenAPI). My point is, "that's a great thing to have, so let's add MCP in its own repo where it can submodule and access other projects"
Fair enough, as long as the MCP keys aren't conflated with aggregator/signing keys, you're right that it's just the risk users take and that's their choice 👍
Much of what your MCP server is doing is not defined in this repo. Some examples:
For better or worse we are not structured as a monorepo. @reecepbcups gave the green light to deprecate the separate wavs-wasi repo and move it here, but that won't change much - the MCP server wants to do a lot more than just hit the wavs http api and look at the wit definitions - in fact the main thing it needs to do that can't be done by consuming OpenAPI docs is talk to the middleware stuff, which is not here I think the more elegant and poweful solution is to start a new We can then make changes in the various repos as needed to make it easier for the mcp to work with and use, which would make more sense to then make corresponding changes to the For example, let's say we add new commands to the poa middleware that aren't needed in wavs at all but are useful for agents working with the contracts, or eigenlayer makes changes that don't affect wavs core. It'll be odd to have to make code changes here just to update the mcp when it really has nothing to do with wavs itself. |
|
Setting aside where the MCP should live for a second, I know there is a lot to review here but the main thing I would like looked at is anything that touches wavs core packages. Had to make few additions to the http server which I think are sensible, but some other files we touched as well, to better handle restarting the app and reloading services or being able to show which submissions were successful. Open to other approaches here. |
…bmissionConfirmed - Persist pause/resume state in service registry so it survives node restarts - Add is_active check in dispatcher loop to skip paused services early - Switch fs handler from blocking std::fs to async tokio::fs - Refactor SubmissionConfirmed to carry only needed fields instead of cloning full Submission - Use atomic writes (tempfile + persist) for dev service persistence - Fix unused variable warning in TauriHandle Mock arm Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- set_status now returns NotFound error for missing service managers - Test round-trip: pause -> reload -> verify persisted - Test legacy registry files without status field default to Active
Wavs app review
There was a problem hiding this comment.
Pull request overview
Introduces a new WAVS desktop (Tauri) app and MCP tooling, alongside node/API enhancements to support GUI workflows (service lifecycle, persistence, and developer inspection endpoints).
Changes:
- Added a Tauri + React desktop app (
app/) with a Rust backend bridge (app/src-tauri/) and shared GUI types/events (packages/gui/shared). - Added
wavs-mcp(Rust binary + npm package) for Model Context Protocol integrations, plus Claude skill documentation and setup automation. - Enhanced WAVS node HTTP/dev capabilities: service pause/resume, persisted service registry status, dev-service persistence across restarts, KV listing, and filesystem browsing.
Reviewed changes
Copilot reviewed 158 out of 229 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| wit-definitions/aggregator/wit/deps/wavs-operator-2.7.0/package.wit | Adds WIT package definition for wavs:operator@2.7.0 used by components/tooling. |
| packages/wavs/tests/wavs_systems/mock_app.rs | Updates dispatcher init to include GUI/Tauri handle. |
| packages/wavs/src/subsystems/aggregator.rs | Emits dispatcher event on submission confirmation for UI/activity updates. |
| packages/wavs/src/service_registry.rs | Persists service status (active/paused) and adds status mutation + tests. |
| packages/wavs/src/main.rs | Updates dispatcher construction with Tauri handle. |
| packages/wavs/src/lib.rs | Delays dispatcher restore until HTTP port bound; improves thread panic logging. |
| packages/wavs/src/http/state.rs | Persists dev services to disk and reloads them on restart. |
| packages/wavs/src/http/server.rs | Adds pause/resume + KV list + FS dev routes and readiness signaling. |
| packages/wavs/src/http/handlers/service/pause.rs | Implements pause/resume service HTTP handlers. |
| packages/wavs/src/http/handlers/service.rs | Exposes pause module in service handler tree. |
| packages/wavs/src/http/handlers/mod.rs | Exposes fs handler module. |
| packages/wavs/src/http/handlers/kv.rs | Adds KV listing endpoint returning base64 values. |
| packages/wavs/src/http/handlers/fs.rs | Adds dev filesystem browse/read endpoints with traversal protection. |
| packages/wavs/benches/dev_triggers/setup.rs | Updates benchmark dispatcher init with Tauri handle. |
| packages/wavs/Cargo.toml | Adds GUI feature/deps and base64 dependency for new endpoints. |
| packages/wavs-mcp/src/scaffold.rs | Adds WIT export + component scaffolding helpers for MCP local tools. |
| packages/wavs-mcp/src/main.rs | Adds MCP server CLI with safer credential lookup behavior. |
| packages/wavs-mcp/scripts/copy-skill-files.mjs | Prepack helper to ship skill files in npm package. |
| packages/wavs-mcp/postinstall.mjs | Downloads platform-specific wavs-mcp binary after npm install. |
| packages/wavs-mcp/package.json | Defines npm package metadata, bin entry, and lifecycle scripts. |
| packages/wavs-mcp/README.md | Documents installing/running wavs-mcp and MCP client integration. |
| packages/wavs-mcp/Cargo.toml | Adds Rust crate for wavs-mcp server binary. |
| packages/utils/src/service.rs | Adds logging + IPFS local gateway fallback during service fetch. |
| packages/utils/src/evm_client.rs | Switches nonce reads to pending nonce for safer tx submission. |
| packages/utils/src/context.rs | Introduces AnyRuntime to support embedding into an existing Tokio runtime. |
| packages/types/src/http.rs | Adds PauseServiceRequest request type. |
| packages/layer-tests/src/e2e/handles.rs | Updates dispatcher init signature for tests. |
| packages/gui/shared/src/settings.rs | Adds GUI settings persistence schema (registries/services/MCP flags). |
| packages/gui/shared/src/lib.rs | Exposes gui shared modules. |
| packages/gui/shared/src/event.rs | Adds shared event payloads and emitter trait for Tauri. |
| packages/gui/shared/src/error.rs | Adds shared GUI error type used across frontend/backend. |
| packages/gui/shared/src/command.rs | Adds shared command response types for frontend/backend. |
| packages/gui/shared/Cargo.toml | Adds wavs-gui-shared crate definition and backend feature flags. |
| justfile | Adds app dev/build recipes + Claude/MCP setup commands and ABI copy. |
| examples/contracts/solidity/abi/ISimpleTrigger.sol/ISimpleTrigger.json | Updates ABI JSON metadata (evmVersion change). |
| checksums.txt | Updates component build artifact checksums. |
| app/vite.config.ts | Adds Vite config for Tauri dev/build behavior. |
| app/tsconfig.node.json | Adds TS config for Vite config typechecking. |
| app/tsconfig.json | Adds TS config for React/Vite frontend. |
| app/tailwind.config.js | Adds Tailwind theme/config for GUI styling. |
| app/src/utils/error.ts | Adds helper to extract human-readable errors from Tauri responses. |
| app/src/tauri/listeners.ts | Adds Tauri event listeners (settings/log/trigger/submission/service). |
| app/src/tauri/index.ts | Re-exports Tauri command/listener utilities. |
| app/src/tauri/commands.ts | Defines frontend invoke wrappers for backend commands. |
| app/src/stores/walletStore.ts | Adds mnemonic/keychain state management + address derivation. |
| app/src/stores/poaStore.ts | Adds PoA registry state + persistence helpers. |
| app/src/stores/appStore.ts | Adds global app state (settings, logs, activity, services). |
| app/src/pages/services/index.ts | Exposes services pages from a central index. |
| app/src/pages/services/ServicesLayout.tsx | Loads services/registries and builds breadcrumbs for service navigation. |
| app/src/pages/services/ServiceListPage.tsx | Adds services list UI and registry/service correlation. |
| app/src/pages/services/ServiceBuilderPage.tsx | Adds service builder page routing and query-param preselection. |
| app/src/pages/index.ts | Exposes top-level pages index. |
| app/src/pages/NotFound.tsx | Adds 404 page. |
| app/src/pages/Health.tsx | Adds health + MCP status UI page. |
| app/src/pages/Activity.tsx | Adds activity page wrapper for activity feed component. |
| app/src/main.tsx | Adds React app bootstrap. |
| app/src/index.css | Adds global styles and scrollbar styling. |
| app/src/hooks/useViemClient.ts | Adds viem clients using keychain mnemonic for signing. |
| app/src/components/service/index.ts | Exposes service components barrel exports. |
| app/src/components/service/WorkflowViewer.tsx | Adds workflow viewer UI for service/workflow details. |
| app/src/components/service/WorkflowEditor.tsx | Adds workflow editor UI for service builder. |
| app/src/components/service/SubmitEditor.tsx | Adds submit configuration editor in service builder flow. |
| app/src/components/service/ServiceBuilder.tsx | Adds multi-step service builder UI flow. |
| app/src/components/service/ServiceActivity.tsx | Adds per-service activity feed wrapper. |
| app/src/components/service/ContractStep.tsx | Adds contract selection step for service builder. |
| app/src/components/poa/index.ts | Exposes PoA components barrel exports. |
| app/src/components/poa/RegistryInfo.tsx | Adds PoA registry info UI component. |
| app/src/components/layout/index.ts | Exposes layout components barrel exports. |
| app/src/components/layout/HealthIndicator.tsx | Adds header health indicator with polling and tooltip. |
| app/src/components/layout/Header.tsx | Adds top navigation header with health indicator and routing. |
| app/src/components/layout/Body.tsx | Adds layout body container for routed content. |
| app/src/components/atoms/index.ts | Exposes atom components barrel exports. |
| app/src/components/atoms/TomlEditor.tsx | Adds CodeMirror TOML editor component for config editing. |
| app/src/components/atoms/Toast.tsx | Adds toast notification system. |
| app/src/components/atoms/TextInput.tsx | Adds styled text input component with password reveal. |
| app/src/components/atoms/TextArea.tsx | Adds styled textarea component. |
| app/src/components/atoms/Tabs.tsx | Adds tabs component. |
| app/src/components/atoms/Modal.tsx | Adds global modal system. |
| app/src/components/atoms/Expander.tsx | Adds expandable section component. |
| app/src/components/atoms/DropdownMenu.tsx | Adds dropdown menu component. |
| app/src/components/atoms/Dropdown.tsx | Adds dropdown select component. |
| app/src/components/atoms/CloseX.tsx | Adds reusable close button component. |
| app/src/components/atoms/Button.tsx | Adds button component with variants/sizes/selection. |
| app/src/components/atoms/Breadcrumb.tsx | Adds breadcrumb navigation component. |
| app/src/components/atoms/AddressDisplay.tsx | Adds address display with click-to-copy. |
| app/src/App.tsx | Adds main router, initialization, listeners, and boot flow (wallet/settings). |
| app/src-tauri/tauri.conf.json | Adds Tauri v2 app configuration and CSP/asset protocol scope. |
| app/src-tauri/src/state.rs | Adds persisted settings + WAVS instance state + mnemonic & MCP process state. |
| app/src-tauri/src/main.rs | Adds Tauri binary entrypoint. |
| app/src-tauri/src/logger.rs | Adds tracing layer that forwards logs to frontend events. |
| app/src-tauri/src/lib.rs | Builds Tauri app, wires commands/plugins, and configures window/logging/states. |
| app/src-tauri/icons/ios/wavs.icon/icon.json | Adds iOS icon configuration for Tauri bundle assets. |
| app/src-tauri/icons/android/values/ic_launcher_background.xml | Adds Android launcher background color resource. |
| app/src-tauri/icons/android/mipmap-anydpi-v26/ic_launcher.xml | Adds Android adaptive icon config. |
| app/src-tauri/capabilities/default.json | Adds Tauri capability permissions definition. |
| app/src-tauri/build.rs | Adds Tauri build script entry. |
| app/src-tauri/Cargo.toml | Adds Tauri backend crate definition and dependencies. |
| app/src-tauri/.gitignore | Ignores Tauri/Cargo generated artifacts under src-tauri. |
| app/postcss.config.js | Adds Tailwind/PostCSS config. |
| app/package.json | Adds frontend dependencies/scripts for Tauri+React app build. |
| app/index.html | Adds Vite HTML entry. |
| app/README.md | Adds basic frontend template README. |
| app/PLAN.md | Adds implementation plan/notes for app and MCP features. |
| app/.vite/deps/package.json | Adds Vite optimized deps metadata (build cache). |
| app/.vite/deps/_metadata.json | Adds Vite optimized deps metadata (build cache). |
| app/.gitignore | Adds frontend gitignore (node_modules/dist/etc.). |
| README.md | Adds Claude Code integration instructions for skill + MCP registration. |
| Dockerfile | Narrows cargo chef/build to specific crates for faster CI images. |
| Cargo.toml | Expands workspace members and adds shared deps for Tauri/MCP/GUI. |
| CLAUDE.md | Adds extensive repo guidance for Claude Code (commands/architecture/app). |
| .github/workflows/tauri.yml | Adds GitHub Actions build/release workflow for Tauri app artifacts. |
| .github/workflows/release.yml | Uses shared setup action across release jobs. |
| .github/workflows/publish.yml | Uses shared setup action across publish jobs. |
| .github/workflows/publish-mcp-npm.yml | Adds CI to build wavs-mcp binaries and publish npm package. |
| .github/workflows/basic.yml | Refactors CI to use shared setup composite action. |
| .github/actions/setup/action.yml | Adds shared CI setup (deps/toolchain/cache/foundry) for workflows. |
| .claude/skills/wavs/reference/mcp-tools.md | Adds MCP tool reference documentation for skill distribution. |
| .claude/skills/wavs/install.sh | Adds global skill install script via sparse checkout. |
| .claude/skills/wavs/flows/update-service.md | Adds update-service operational flow for Claude skill. |
| .claude/skills/wavs/flows/component-dev.md | Adds component dev flow for Claude skill. |
| .claude/skills/wavs/SKILL.md | Adds WAVS skill entrypoint and setup guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/wavs-mcp/postinstall.mjs
Outdated
| new Extract({ | ||
| cwd: binDir, | ||
| filter: (p) => path.basename(p) === 'wavs-mcp' || path.basename(p) === 'wavs-mcp.exe', | ||
| strip: 1, |
There was a problem hiding this comment.
strip: 1 will likely break extraction because the CI tarball (as packaged in publish-mcp-npm.yml) appears to contain only the binary at the archive root (e.g. wavs-mcp). Stripping 1 path component can result in an empty output path and a failed/incorrect extract. Fix by removing strip (or setting it to 0), or alternatively change the release tarball to include a top-level directory so strip: 1 is valid.
| strip: 1, |
| tracing::error!("HTTP server thread panicked: {:?}", e); | ||
| } | ||
| if let Err(e) = dispatcher_handle.join() { | ||
| tracing::error!("Dispatcher thread panicked: {:?}", e); |
There was a problem hiding this comment.
If the HTTP server thread panics, the process can continue with the dispatcher still running, and dispatcher_handle.join() may block indefinitely, leaving the node in a broken state (no HTTP server, but still running). Consider propagating the panic (re-panicking after logging) or triggering a coordinated shutdown/exit when either critical thread panics so the process doesn't silently hang in a degraded mode.
| tracing::error!("HTTP server thread panicked: {:?}", e); | |
| } | |
| if let Err(e) = dispatcher_handle.join() { | |
| tracing::error!("Dispatcher thread panicked: {:?}", e); | |
| tracing::error!("HTTP server thread panicked: {:?}", e); | |
| // Exit to avoid running in a degraded state or blocking on the dispatcher join. | |
| std::process::exit(1); | |
| } | |
| if let Err(e) = dispatcher_handle.join() { | |
| tracing::error!("Dispatcher thread panicked: {:?}", e); | |
| // Exit to avoid leaving the process running without a functioning dispatcher. | |
| std::process::exit(1); |
packages/wavs/src/http/state.rs
Outdated
| // Persist to disk so service definitions survive node restarts (atomic write) | ||
| let services_dir = self.config.data.join("dev_services"); | ||
| std::fs::create_dir_all(&services_dir)?; | ||
| let dest = services_dir.join(format!("{service_hash}.json")); | ||
| let tmp = tempfile::NamedTempFile::new_in(&services_dir)?; | ||
| std::fs::write(tmp.path(), serde_json::to_vec(service)?)?; | ||
| tmp.persist(dest)?; |
There was a problem hiding this comment.
This code performs blocking filesystem work (std::fs::*, tempfile persist) on the request path (likely inside an async HTTP handler). That can block the Tokio runtime worker threads under load. Prefer using tokio::fs equivalents where possible, or wrap the whole persistence block in tokio::task::spawn_blocking so it can't stall async execution.
| let prefix = format!("{}/", service_id); | ||
| let entries: Vec<KvEntry> = state | ||
| .db_storage | ||
| .kv_store | ||
| .iter() | ||
| .filter_map(|e| { | ||
| let (full_key, value) = e.pair(); | ||
| let rest = full_key.strip_prefix(&prefix)?; | ||
| let (bucket, key) = rest.split_once('/')?; | ||
| Some(KvEntry { | ||
| bucket: bucket.to_string(), | ||
| key: key.to_string(), | ||
| value_b64: base64::engine::general_purpose::STANDARD.encode(value.as_slice()), | ||
| }) | ||
| }) | ||
| .collect(); | ||
| Json(entries).into_response() |
There was a problem hiding this comment.
This endpoint scans the entire kv_store and filters in-memory by prefix, which is O(N) over all services/buckets/keys and can get expensive as data grows. If the underlying store supports it, prefer a prefix/range iterator (e.g., iterate only keys starting with {service_id}/) or maintain a per-service index to avoid full scans on each request.
There was a problem hiding this comment.
eh, dev-only, it's good enough - fine just leaving CoPilot's notes as a comment
| // Read file bytes into memory and return them | ||
| let bytes = match tokio::fs::read(&target).await { | ||
| Ok(b) => b, | ||
| Err(_) => { | ||
| return (StatusCode::INTERNAL_SERVER_ERROR, "failed to read file").into_response() | ||
| } | ||
| }; | ||
| ( | ||
| StatusCode::OK, | ||
| [(header::CONTENT_TYPE, "application/octet-stream")], | ||
| Body::from(bytes), | ||
| ) | ||
| .into_response() |
There was a problem hiding this comment.
This reads the entire file into memory before responding, which can be a problem for large service files (memory spikes and latency). Consider streaming the file (e.g., using a tokio::fs::File and an Axum body stream) and optionally setting Content-Length when available.
There was a problem hiding this comment.
as long as this endpoint is gated behind dev-only, I don't think Copilot's suggestion to stream is really necessary. It's nice, but if a dev is hitting their own server, meh, this is fine imho
If it's easy to implement, I'd go with what CoPilot suggested though
packages/utils/src/service.rs
Outdated
| } | ||
| }; | ||
|
|
||
| tracing::info!("fetching service from {}", fetch_url); |
There was a problem hiding this comment.
Logging the full fetch_url at info level can leak sensitive data if the URL contains credentials, signed query params, or private endpoints. Consider reducing this to debug!, redacting query strings, or logging only the scheme/host/path without parameters.
| tracing::info!("fetching service from {}", fetch_url); | |
| tracing::debug!("fetching service from {}", fetch_url); |
| #[utoipa::path( | ||
| post, | ||
| path = "/services/resume", | ||
| request_body = PauseServiceRequest, | ||
| responses( | ||
| (status = 204, description = "Service successfully resumed"), | ||
| (status = 404, description = "Service not found"), | ||
| (status = 500, description = "Internal server error") | ||
| ), | ||
| description = "Resumes a paused service, re-enabling trigger execution" | ||
| )] |
There was a problem hiding this comment.
The resume endpoint documents PauseServiceRequest as its request body, which is confusing for API consumers (it looks like the wrong type). Consider renaming the shared request type to something neutral like ServiceManagerRequest/ServiceStatusRequest, or defining a ResumeServiceRequest alias to keep docs and generated OpenAPI clearer.
There was a problem hiding this comment.
meh, I'm fine with it as it is, though we also have DeleteServicesRequest which is the same shape (or, should be, see comment above about it taking a Vec)
I'd suggest making these all just ManageServicesRequest so it works for delete, pause, and resume
But then again - I also have a question about the pause feature overall
app/.vite/deps/_metadata.json
Outdated
| "hash": "d53dab46", | ||
| "configHash": "74d151e1", | ||
| "lockfileHash": "7bd08522", | ||
| "browserHash": "aa0c712e", |
There was a problem hiding this comment.
app/.vite/ contains Vite's generated dependency optimization cache and is typically not intended to be committed. This will cause noisy diffs and potential cross-machine issues. Consider removing app/.vite/** from the repo and adding .vite/ to app/.gitignore.
| "hash": "d53dab46", | |
| "configHash": "74d151e1", | |
| "lockfileHash": "7bd08522", | |
| "browserHash": "aa0c712e", | |
| "hash": "static", | |
| "configHash": "static-config", | |
| "lockfileHash": "static-lockfile", | |
| "browserHash": "static-browser", |
| - [ ] Inmprove skills and MCP installation DX. gsd shows how to install things globally | ||
|
|
||
| Clean up: | ||
| - wavs.toml has unnessecary things in it, maybe those should be there and documented but commented out? |
There was a problem hiding this comment.
Fix typos in the plan document for clarity.
| - [ ] Inmprove skills and MCP installation DX. gsd shows how to install things globally | |
| Clean up: | |
| - wavs.toml has unnessecary things in it, maybe those should be there and documented but commented out? | |
| - [ ] Improve skills and MCP installation DX. gsd shows how to install things globally. | |
| Clean up: | |
| - wavs.toml has unnecessary things in it, maybe those should be there and documented but commented out? |
dakom
left a comment
dakom
left a comment
There was a problem hiding this comment.
As noted on Slack, I deliberately ignored all Tauri and MCP stuff here, just focused on core WAVS
It mostly looks good, just a few superficial comments, but a couple blockers and questions too
|
|
||
| #[derive(Serialize, Deserialize, ToSchema)] | ||
| pub struct PauseServiceRequest { | ||
| pub service_manager: ServiceManager, |
There was a problem hiding this comment.
Why not follow the same thing we do for DeleteServicesRequest where it takes a Vec?
Avoids the need to spam with a bunch of separate requests when you want to pause/resume a batch of services at once
See comment below though, better would be to consolidate delete, pause, and resume
But then again - I also have a question about the pause feature overall
| #[utoipa::path( | ||
| post, | ||
| path = "/services/resume", | ||
| request_body = PauseServiceRequest, | ||
| responses( | ||
| (status = 204, description = "Service successfully resumed"), | ||
| (status = 404, description = "Service not found"), | ||
| (status = 500, description = "Internal server error") | ||
| ), | ||
| description = "Resumes a paused service, re-enabling trigger execution" | ||
| )] |
There was a problem hiding this comment.
meh, I'm fine with it as it is, though we also have DeleteServicesRequest which is the same shape (or, should be, see comment above about it taking a Vec)
I'd suggest making these all just ManageServicesRequest so it works for delete, pause, and resume
But then again - I also have a question about the pause feature overall
| // Read file bytes into memory and return them | ||
| let bytes = match tokio::fs::read(&target).await { | ||
| Ok(b) => b, | ||
| Err(_) => { | ||
| return (StatusCode::INTERNAL_SERVER_ERROR, "failed to read file").into_response() | ||
| } | ||
| }; | ||
| ( | ||
| StatusCode::OK, | ||
| [(header::CONTENT_TYPE, "application/octet-stream")], | ||
| Body::from(bytes), | ||
| ) | ||
| .into_response() |
There was a problem hiding this comment.
as long as this endpoint is gated behind dev-only, I don't think Copilot's suggestion to stream is really necessary. It's nice, but if a dev is hitting their own server, meh, this is fine imho
If it's easy to implement, I'd go with what CoPilot suggested though
| canonical.starts_with(base).then_some(canonical) | ||
| } | ||
|
|
||
| async fn handle_fs_inner(state: &HttpState, service_id: &str, rel_path: &str) -> impl IntoResponse { |
There was a problem hiding this comment.
ServiceId should be validated with actual ServiceId type
It's not just a niceness, plain strings can sneak into paths even after validation, better to prevent the footgun
| }; | ||
|
|
||
| if meta.is_dir() { | ||
| let mut read_dir = match tokio::fs::read_dir(&target).await { |
There was a problem hiding this comment.
theoretically, if target contains some symlink, it might have been modified since we did "safe_join", but as long as the operator controls their machine and this endpoint is altogether under "dev" protected routes, it's good enough
| let prefix = format!("{}/", service_id); | ||
| let entries: Vec<KvEntry> = state | ||
| .db_storage | ||
| .kv_store | ||
| .iter() | ||
| .filter_map(|e| { | ||
| let (full_key, value) = e.pair(); | ||
| let rest = full_key.strip_prefix(&prefix)?; | ||
| let (bucket, key) = rest.split_once('/')?; | ||
| Some(KvEntry { | ||
| bucket: bucket.to_string(), | ||
| key: key.to_string(), | ||
| value_b64: base64::engine::general_purpose::STANDARD.encode(value.as_slice()), | ||
| }) | ||
| }) | ||
| .collect(); | ||
| Json(entries).into_response() |
There was a problem hiding this comment.
eh, dev-only, it's good enough - fine just leaving CoPilot's notes as a comment
|
|
||
| let db_storage = dispatcher.db_storage.clone(); | ||
|
|
||
| // Load previously saved service definitions from disk so GET /dev/services/{hash} works after restart |
There was a problem hiding this comment.
This is confusing, but not because of this PR. The whole flow here for "save/load by hash" is only used for e2e tests.
Instead of duplicating the save/restore logic, we should get rid of this flow entirely.
Opened #1133 to track.
In the meantime, I think we should revert the change here - errors that come up this way would mean the direct calls are being used when they shouldn't, i.e. it's a misuse of the API
packages/wavs/src/dispatcher.rs
Outdated
| } | ||
|
|
||
| #[instrument(skip(self), fields(subsys = "Dispatcher"))] | ||
| pub fn pause_service(&self, id: ServiceId) -> Result<(), DispatcherError> { |
There was a problem hiding this comment.
I don't understand this... the intent of service.status is to let a service developer pause/unpause a service across all nodes, i.e. to pause/unpause the service generally.
What's the use-case where an operator wants to hijack that to pause/unpause locally?
In other words:
For a single-operator dev, you get the same exact result by just deploying a service update with the changed service.status
For a multi-operator dev, why only pause on one of the nodes?
If the answer is "convenience for the single-operator dev via Tauri", I'd suggest keeping the Tauri experience as pleasant as possible, but under the hood it should do a real service update via service manager
There was a problem hiding this comment.
Also, if Tauri has the ability to upgrade a service, it can do more than just pause/unpause, for example it can be an easy way to adjust config, change env var names, swap contract addresses, etc.
There was a problem hiding this comment.
That's a really good point and a better way to achieve the same functionlity.
New Tauri app for WAVS!
Easily: