add services - #26
Conversation
📝 WalkthroughWalkthroughAdds Service-on-Demand support across MCP schemas, cost estimation, escrow preflight, node lifecycle wrappers, tool registration, documentation, prompts, and unit/integration tests. ChangesService-on-Demand
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPTools
participant NodeClient
participant OceanNode
participant EVMRegistry
Client->>MCPTools: estimateServiceCost
MCPTools->>OceanNode: inspect environments and fee schedules
MCPTools->>EVMRegistry: read token decimals
MCPTools-->>Client: cost and escrow payment data
Client->>MCPTools: serviceStart
MCPTools->>NodeClient: run escrow gate and start request
NodeClient->>OceanNode: start service
OceanNode-->>Client: Starting service record
Client->>MCPTools: serviceStatus polling
MCPTools->>NodeClient: query service status
NodeClient->>OceanNode: fetch lifecycle state
OceanNode-->>Client: Running service and endpoints
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
Excellent implementation of Service-on-Demand functionality. The code demonstrates a high level of care for edge cases (e.g., float64 arithmetic in token conversion, chunked stream limiting, and accurate MCP schema descriptions). Minor drift found in a code comment, but the logic itself is flawless. LGTM!
Comments:
• [INFO][style] The comment mentions 45 Restarting as 'settled enough to report', but the settled function directly below it explicitly does NOT include 45 (it only checks for 40 and terminal statuses). The code logic is correct since 45 Restarting is an in-flight status and should continue polling (as defined in your SERVICE_POLLING_GUIDE), so just the comment needs a tiny tweak to remove the mention of 45 being settled.
- // 40 Running and 45 Restarting are both "settled enough to report"; anything
+ // 40 Running is "settled enough to report"; anything
// terminal ends the wait too. 50 Stopping is in flight, so it keeps waiting.• [INFO][other] Excellent handling of JavaScript float64 to exact decimal string conversion here. Manually expanding the scientific notation output of toPrecision ensures that no binary-representation noise leaks into the raw base units for escrow preflight. Great job avoiding the typical parseUnits pitfalls.
• [INFO][performance] This collectStream implementation cleanly and safely handles chunk concatenation with truncation. Bounding the memory growth when streaming large service logs (which can be days long) is an excellent defensive practice.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/integration/services.test.ts (2)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
last.status !== 40is dead.isServiceTerminal(40)is false (asserted inserviceStatusLabels.test.ts), so the extra clause never affects the branch and reads as if Running were terminal.♻️ Simplify
- if (isServiceTerminal(last.status) && last.status !== 40) { + if (isServiceTerminal(last.status)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/integration/services.test.ts` around lines 78 - 84, Remove the redundant last.status !== 40 condition from the terminal-failure check in the service-waiting logic, leaving isServiceTerminal(last.status) as the sole predicate while preserving the existing error message and fail-fast behavior.
93-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo teardown: a failure mid-suite leaves a paid service running. If
reaches Running/logs/extendthrows, the final stop test never runs and the escrow-funded service stays up until expiry. There is also no counterpart teardown forProviderInstance.setupP2P, which can keep mocha alive.♻️ Suggested best-effort cleanup hook
it('lists templates without treating an empty catalogue as a failure', async () => {Add after the
beforehook:after(async () => { if (!serviceId) return try { await nodeClient.serviceStop(node, AUTH_TOKEN!, serviceId, TIMEOUT_MS) } catch { // best-effort: the service may already be stopped or expired } })Also applies to: 235-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/integration/services.test.ts` around lines 93 - 116, Add an after teardown hook alongside the existing before setup in the integration suite: when serviceId exists, best-effort call nodeClient.serviceStop with node, AUTH_TOKEN, serviceId, and TIMEOUT_MS, swallowing cleanup errors so already-stopped or expired services do not fail teardown. Also clean up the P2P resources initialized by ProviderInstance.setupP2P using the existing provider teardown API.src/tools/serviceTools.ts (1)
871-889: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
waitForRunningis silently ignored whenserviceIdis omitted. The response still reportswaitedSeconds: 0, waitTimedOut: false, which reads as "settled" rather than "never waited". Consider surfacing a note (or rejecting the combination) so an agent doesn't conclude the service is up.♻️ Suggested handling
- if (args.waitForRunning && args.serviceId) { + if (args.waitForRunning && !args.serviceId) { + return errorPayload( + 'waitForRunning requires a serviceId — there is no single job to wait on otherwise.' + ) + } + if (args.waitForRunning && args.serviceId) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/serviceTools.ts` around lines 871 - 889, Handle the case where waitForRunning is true but serviceId is absent instead of silently skipping the wait logic. Update the response state around waitedSeconds and waitTimedOut to surface that no service was selected for waiting, or reject the invalid combination, while preserving the existing polling behavior when serviceId is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/serviceCost.ts`:
- Around line 292-335: Harden parseUserData against ReDoS from untrusted
spec.validation patterns before invoking re.test(value). Add a bounded, safe
validation approach that rejects overly complex or unsafe regex patterns, or
evaluates them outside the main thread with a timeout; preserve invalid-pattern
warnings and existing mismatch errors for accepted patterns.
In `@src/tools/serviceTools.ts`:
- Line 696: Add a required-image validation guard in serviceStart beside the
existing image-mode/port checks, before serviceEscrowGate is called. Reject
requests when the start image is omitted or undefined, rather than allowing the
optional serviceContainerSpecSchema.image value to be cast and forwarded to the
node.
---
Nitpick comments:
In `@src/test/integration/services.test.ts`:
- Around line 78-84: Remove the redundant last.status !== 40 condition from the
terminal-failure check in the service-waiting logic, leaving
isServiceTerminal(last.status) as the sole predicate while preserving the
existing error message and fail-fast behavior.
- Around line 93-116: Add an after teardown hook alongside the existing before
setup in the integration suite: when serviceId exists, best-effort call
nodeClient.serviceStop with node, AUTH_TOKEN, serviceId, and TIMEOUT_MS,
swallowing cleanup errors so already-stopped or expired services do not fail
teardown. Also clean up the P2P resources initialized by
ProviderInstance.setupP2P using the existing provider teardown API.
In `@src/tools/serviceTools.ts`:
- Around line 871-889: Handle the case where waitForRunning is true but
serviceId is absent instead of silently skipping the wait logic. Update the
response state around waitedSeconds and waitTimedOut to surface that no service
was selected for waiting, or reject the invalid combination, while preserving
the existing polling behavior when serviceId is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02f6723d-d4df-479b-baef-9c1f560b6fb7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackage.jsonsrc/clients/nodeClient.tssrc/prompts/registerPrompts.tssrc/resources/resourceCatalog.tssrc/test/integration/docsMerge.test.tssrc/test/integration/services.test.tssrc/test/unit/tools/registerTools.test.tssrc/test/unit/tools/serviceCost.test.tssrc/test/unit/tools/serviceStatusLabels.test.tssrc/test/unit/tools/serviceTemplates.test.tssrc/test/unit/tools/serviceTools.test.tssrc/tools/escrowPreflight.tssrc/tools/p2pSchemas.tssrc/tools/registerTools.tssrc/tools/serviceCost.tssrc/tools/serviceSchemas.tssrc/tools/serviceTools.tssrc/utils/serviceOnDemand.ts
| export function parseUserData( | ||
| data: Record<string, unknown> | undefined, | ||
| userConfigurableEnvVars?: UserConfigurableEnvVar[] | ||
| ): { data?: Record<string, unknown>; warnings: string[] } { | ||
| if (!data) return { warnings: [] } | ||
| if (typeof data !== 'object' || Array.isArray(data)) { | ||
| throw new Error('userData must be a JSON object (not an array or primitive)') | ||
| } | ||
|
|
||
| const warnings: string[] = [] | ||
| const byKey = new Map((userConfigurableEnvVars ?? []).map((v) => [v.key, v])) | ||
|
|
||
| for (const key of Object.keys(data)) { | ||
| const spec = byKey.get(key) | ||
| if (!spec) { | ||
| if (userConfigurableEnvVars?.length) { | ||
| warnings.push( | ||
| `userData key "${key}" is not listed in the template's userConfigurableEnvVars — ` + | ||
| `sent anyway (templates are suggestions, not allow-lists).` | ||
| ) | ||
| } | ||
| continue | ||
| } | ||
| if (!spec.validation) continue | ||
| let re: RegExp | undefined | ||
| try { | ||
| re = new RegExp(spec.validation) | ||
| } catch { | ||
| warnings.push( | ||
| `Template validation pattern for "${key}" is not a valid regex — skipped.` | ||
| ) | ||
| continue | ||
| } | ||
| const value = data[key] | ||
| if (typeof value === 'string' && !re.test(value)) { | ||
| throw new Error( | ||
| `userData value for "${key}" does not match the template's validation pattern ` + | ||
| `(${spec.validation}). The value is not shown.` | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return { data, warnings } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
ReDoS risk: template-supplied regex run against consumer input.
spec.validation originates from a remote node operator's getServiceTemplates() response (an arbitrary, untrusted peer reachable via findServiceNodes), and new RegExp(spec.validation) is then executed synchronously via re.test(value) against consumer-controlled userData. A malicious or compromised operator can publish a catastrophic-backtracking pattern in a template and hang the whole (single-threaded) MCP server process for any consumer that supplies a matching value — this is exactly the CWE-1333 ReDoS shape both linters flagged.
Consider bounding pattern complexity (e.g. reject overly long/nested patterns, or vet with a safe-regex checker) or running .test() with an execution timeout (e.g. via a worker) before trusting operator-supplied patterns.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 317-317: Do not use variable for regular expressions
Context: new RegExp(spec.validation)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
[warning] 317-317: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(spec.validation)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 GitHub Check: Lint
[warning] 318-318:
Found non-literal argument to RegExp Constructor
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/serviceCost.ts` around lines 292 - 335, Harden parseUserData
against ReDoS from untrusted spec.validation patterns before invoking
re.test(value). Add a bounded, safe validation approach that rejects overly
complex or unsafe regex patterns, or evaluates them outside the main thread with
a timeout; preserve invalid-pattern warnings and existing mismatch errors for
accepted patterns.
Source: Linters/SAST tools
| environment: z | ||
| .string() | ||
| .describe('Env id — must be service-capable (findServiceEnvironments).'), | ||
| ...serviceContainerSpecSchema, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'serviceSchemas.ts' --exec cat -nRepository: oceanprotocol/on-mcp
Length of output: 16978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'serviceTools.ts' --exec sh -c 'wc -l "$1"; sed -n "680,810p" "$1"' sh {}Repository: oceanprotocol/on-mcp
Length of output: 5514
Add a required-image guard to serviceStart. It shares serviceContainerSpecSchema.image, which is optional, so an omitted start image is cast to string as undefined and sent to the node. Add the check beside the image-mode/port validation before calling serviceEscrowGate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/serviceTools.ts` at line 696, Add a required-image validation guard
in serviceStart beside the existing image-mode/port checks, before
serviceEscrowGate is called. Reject requests when the start image is omitted or
undefined, rather than allowing the optional serviceContainerSpecSchema.image
value to be cast and forwarded to the node.
1 participant
Fixes #24
Service-on-Demand: rent long-running containers on an Ocean node
Summary
Adds the Service-on-Demand surface to on-mcp: 11 tools covering the full lifecycle of a long-running, pre-paid container on an Ocean node (discover → estimate → provision escrow → start → poll → operate → stop), plus a
run_serviceprompt, anocean://docs/service-on-demandresource, and 8 newNodeClientwrappers over theSERVICE_*protocol commands. Requires@oceanprotocol/lib9.0.0-next.8 (up from^8.2.0), which is where the service commands land.A service is not a compute job: it does not run to completion, it stays up for a paid
durationand exposes network endpoints the consumer connects to. That difference drives most of the design below.Why
Ocean nodes gained Service-on-Demand (ocean-node #1408) but nothing exposed it to agents. The lifecycle has several traps that an agent gets wrong by default, and each one costs the user real money, so the tools are shaped to make the traps unreachable rather than merely documented:
serviceStartreturns aserviceIdatStarting (10); escrow lock → image pull/build + vulnerability scan → claim → port allocation → container start all happen in a background loop. An agent that reports "started" is wrong.initializeCompute; services have no equivalent command, so the cost has to be estimated client-side against the node's own arithmetic.serviceStoptears the container down but keeps cpu/ram/gpu and host ports reserved untilexpiresAt. Stopping early neither refunds nor frees capacity — an agent offering it as a cost saving is actively harmful.Stopping (50)is not an end state andError (99)still holds the paid reservation and is restartable.What changed
New: 11 service tools —
src/tools/serviceTools.tsfindServiceEnvironmentsfindServiceNodeslist_discovered_peers; unreachable peers are skipped, not fatal; reportsprobedCountand what it truncatedgetServiceTemplatesserviceStartArgsprojectionestimateServiceCostpaymentobjectserviceStartserviceStatuswaitForRunninggetServicesserviceExtendserviceRestartserviceStopexpiresAtserviceLogsDesign decisions worth reviewing:
templateIdonserviceStart. The node has no such parameter — templates are a suggestion, not an allow-list, and the caller always passes the container spec explicitly.getServiceTemplatestherefore emits aserviceStartArgsprojection per entry, pre-renamed (template.command→dockerCmd,template.entrypoint→dockerEntrypoint), because those fields copied verbatim are silently dropped and the container runs its image default.userDataKeys(yours, fillable viauserData) vsoperatorEnvVarKeys(the operator's — keys exposed, values never).CapDrop: ['ALL'], so it cannot bind them — it would start, be charged for, and never serve); mutually exclusivetag/checksum/dockerfile;features.services === falseenvs; and a RESPEC guard that catches "one newdockerCmd" before the node's 400.serviceLogsis bounded by default —since: '5m', 256 KiB cap,truncated: truerather than a huge blob. Unbounded, the node streams the container's entire history.userDatavalues never appear in a response. Tools echo keys only; the node strips values too, so a rotation is a full RESPEC restart (documented with a recipe).New: cost + decoration helpers —
src/tools/serviceCost.ts,src/tools/serviceSchemas.tsPure, network-free, unit-tested. Ported from ocean-cli's
serviceHelpers.tswith the divergences from ocean-node's own arithmetic corrected:estimateServiceCostimplements the node's formula —price(id) × amount × ceil(effectiveDuration / 60)witheffectiveDuration = max(duration, env.minJobDuration)— matchesfeeTokencase-insensitively but echoes it back verbatim as advertised, and surfacesunpricedResourceIdsbecause the node prices an unknown resource id at 0 silently (a typo'd resource looks free).toRawAmountemits a decimal string: an 18-decimal amount exceedsNumber.MAX_SAFE_INTEGER.escrowRequired: falseand nopayment, instead of an invalid payment object.decorateServiceJobadds what the node does not send:statusLabel(covering45 Restarting),expiresAtIso,isTerminal,paymentClaimed(a precondition for restart), and warnings for the two money-relevant statuses —Stopping (50)is in flight and still holds resources;Error (99)is terminal but restartable with the reservation intact.New:
serviceEscrowGateon the two paying toolsserviceStartandserviceExtendreuse the existingescrow_preflightcore: estimate → build payment →runEscrowPreflight→ refuse withescrow_preflight_failedwhen escrow cannot back the service. This matters more than for compute — a doomed service start still returns aserviceIdand then dies asynchronously atLocking, which is strictly worse than a synchronous refusal. Best-effort throughout: noevmRegistry, an unresolvable payer, a free service or any internal failure falls through and lets the node decide.skipEscrowPreflightbypasses.src/tools/escrowPreflight.tsgainsserviceMinLockSeconds(d)— a padded lower bound, notd + 3600. The node's rule isd + claimDurationTimeout, andclaimDurationTimeoutis per-node config no protocol command exposes, so an exact-3600 figure would green-light a service whosecreateLockthen fails with nothing to explain why.New: 8
NodeClientwrappers —src/clients/nodeClient.tsThin wrappers like the rest of the class, with two deliberate asymmetries flagged in comments so nobody "fixes" them:
serviceStartandserviceRestartroute viaProviderInstance(BaseProvider) rather thangetP2p(), because only BaseProvider carries thenotifyIncentiveBackendService*hooks.getImpl()dispatches aNodeP2Ptarget to the sameP2pProvider, so wire behaviour is identical; the hooks are no-ops unlessINCENTIVE_BACKEND_URLis set (we do not set it), but the call sites are correct if a deployment ever does.Also refactors the compute-result/compute-log collectors onto a shared
collectStream(raw, maxBytes)so service logs can cap collection.Fixed: auth-token signing under lib v9 —
src/tools/p2pSchemas.tsThe signed message gained a fourth component:
consumerAddress + nonce + protocolCommand + issuerPeerId.issuerPeerIdis empty for every command exceptCREATE_AUTH_TOKEN, where it is the target node's peerId — so only token minting was affected, and the previously documented string produced a signature the node rejects.P2P_AUTH_SIGNING_GUIDEnow documents this, lists the 8 service commands with theirPROTOCOL_COMMANDSvalues, andcreateAuthTokenaccepts the new optionalvalidUntil.P2P_RECOMMENDED_NODES_GUIDEalso now says explicitly not to use the recommended-nodes list orfind_providerfor services: capability is per-environment (features.services) with no DHT advertise string, so discovery is a fan-out.New: docs resource + prompt
ocean://docs/service-on-demand(src/utils/serviceOnDemand.ts) — the services mental model, status table, money semantics, safety surface, and recommended flow. Registered in the resource catalog and listed in the README.run_serviceprompt — goal + optional node peerID + duration → discover, estimate, provision escrow, start, poll to Running without pestering the user between polls, hand over endpoints with the "endpoints are not node-authenticated" caveat.Removed:
debug_nodeand theDIAGNOSTICStableon-mcp is a remote server talking to remote p2p ocean-nodes. Shipping
docker logs ocean-node,ss -tlnp,nvidia-smiandcat /etc/docker/daemon.jsonchecklists to a client that cannot run them was wrong, so the 9-entryDIAGNOSTICStable and the prompt it fed are gone (−183 lines). Node-operator guidance stays where it already lived and does not assume shell access:get_workflow("troubleshoot_node"),check_node_eligibility,search_docs.Notes for reviewers
features.servicesis advisory and stays that way. It defaults totruenode-side and the start path never re-checks thatserviceOnDemandwas configured, so neither the flag nor an empty template list proves a node is provisioned. Env eligibility is a strong filter;SERVICE_STARTis authoritative. No upstream change requested.estimateServiceCostis an estimate, and the tool descriptions say so repeatedly. Beyond the unpriced-id trap:serviceOnDemand.maxDurationSeconds(default 86400) is not advertised, so an over-long duration fails at start with a 400.calculateResourcesCostin ocean-node'scompute_engine_base.tsis one shared method called byinitializeCompute,startCompute,startServiceandextendService, so nothing had to change on the compute side. One known client-side divergence remains, services-only and false-negative-safe: ouravailableForsubtractsinUsefor every resource kind, while the node applies the per-env ceiling to fungible only and never blocks allocation on shareable discrete resources. That can makefindServiceEnvironmentsdrop an env the node would accept. Not fixed here.getServicesbeing node-wide is a footgun — the description leads with it, since presenting another consumer's services as the user's own would be a privacy problem.Summary by CodeRabbit
New Features
run_serviceguided workflow for launching services.Documentation