Add support for services#163
Conversation
📝 WalkthroughWalkthroughThe CLI now supports Service-on-Demand workflows: template discovery, escrow verification, service startup, status and log retrieval, extension, restart, and stop operations. Documentation, dependency updates, CI trigger changes, and end-to-end coverage are included. ChangesService-on-Demand workflow
CI pull request trigger
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant Commands
participant serviceHelpers
participant Escrow
participant ProviderInstance
participant Node
CLI->>Commands: startService(options)
Commands->>serviceHelpers: resolve resources and estimate cost
Commands->>Escrow: verify service escrow
Commands->>ProviderInstance: serviceStart(params)
ProviderInstance->>Node: create service
Node-->>ProviderInstance: return service job
ProviderInstance-->>Commands: return service job
Commands->>serviceHelpers: pollServiceStatus()
serviceHelpers->>Node: fetch service status
Node-->>serviceHelpers: return running status
serviceHelpers-->>Commands: return running service job
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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:
This is an excellent PR introducing the Service-on-Demand lifecycle commands. The architectural split between CLI parsing, core actions, and helpers is clean. Input validation, cost estimations, interactive prompts, and robust polling (especially the old container ID check during restarts) are very well implemented. A few minor informational suggestions are provided for edge cases.
Comments:
• [INFO][style] options.env is not defined as an option for the startService command (only as a positional <computeEnvId>). This fallback is redundant but harmless. You could simplify this to const envId = computeEnvId;.
• [INFO][logic] Because template values are copied before explicit flags are applied, if a template has a tag and the user explicitly passes --checksum, both tag and checksum will be truthy. This causes specCount > 1 and triggers the validation error below. Since cli.ts already prevents mixing --template and --image, this is perfectly fine for now, but worth noting if you ever want to allow users to override a template's image specifications in the future.
• [INFO][style] The userData file and inline parsing logic here works perfectly, though it duplicates some of the validation logic found in parseUserData inside src/serviceHelpers.ts. Since template validation isn't strictly needed for a restart, this is acceptable, but consider reusing parseUserData if you wish to completely centralize the parsing logic.
• [INFO][logic] The use of notContainerId to avoid race conditions when checking for Running status immediately after a restart is a fantastic piece of defensive programming. Excellent handling of this edge case!
![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
🤖 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/commands.ts`:
- Around line 1873-1879: Update the expiry logging in the service extension
success path near the extendPayments output to guard both oldExpiry and
newJob.expiresAt before calling toISOString(). Reuse the same safe
expiry-formatting behavior established by printServiceJob, ensuring undefined or
zero values do not throw after a successful serviceExtend.
In `@test/serviceFlow.test.ts`:
- Around line 40-49: Call homedir() when constructing the default address-file
path instead of interpolating the function reference; update both
test/serviceFlow.test.ts sites at lines 40-49 and 62-72, including getAddresses
and the before hook default.
🪄 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: 7723449d-06bd-4c66-9e53-be4ad933bf80
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/cli.tssrc/commands.tssrc/serviceHelpers.tstest/serviceFlow.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli.ts (2)
659-670: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError paths return without
process.exit(1).Across the new service commands (
startServiceLines 659-670,serviceLogsLines 783-786,extendServiceLines 807-814,restartServiceLines 845-848,stopServiceLines 901-904), validation failures log an error andreturnwithout exiting with a non-zero code. Scripts/CI invoking these commands can't detect failure via exit status.As per coding guidelines,
{src/cli.ts,src/commands.ts}: "Useprocess.exit(code)for CLI exit codes with code 0 for success and 1 for errors."if (!envId || !duration || !token) { console.error(chalk.red("Missing required arguments: <computeEnvId> <duration> <paymentToken>")); + process.exit(1); return; }Also applies to: 783-786, 807-814, 845-848, 901-904
🤖 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/cli.ts` around lines 659 - 670, Update validation failure paths in startService, serviceLogs, extendService, restartService, and stopService to call process.exit(1) after logging errors instead of returning normally. Preserve the existing error messages and successful execution flow so CLI consumers receive a non-zero status only for validation failures.Source: Coding guidelines
623-707: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
restartServicedockerfile flags path-based or rename them explicitly.
startServicetreats--dockerfile/--additional-docker-filesas file paths, butrestartServicetreats the same flag names as inline contents:--dockerfileis passed directly and--additional-docker-filesis parsed as JSON without reading a file. Reusing thestartServiceconvention withrestartServicecan fail during JSON parsing or send path text as Dockerfile content. Align both commands to the same convention or rename one side’s flags, e.g.--dockerfile-filevs--dockerfile.🤖 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/cli.ts` around lines 623 - 707, Align restartService with startService’s path-based convention for --dockerfile and --additional-docker-files: read the Dockerfile path and parse the additional-files path contents before passing them to the command implementation. Update the corresponding restartService option handling and argument mapping, or explicitly rename its inline-content flags to avoid reusing these path-based names.
🤖 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/cli.ts`:
- Around line 851-863: Update restartService to explicitly reject supplying both
options.userData and options.userDataFile before the existing parsing branches,
using a clear mutual-exclusion error; preserve the current behavior when only
one option is provided.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 659-670: Update validation failure paths in startService,
serviceLogs, extendService, restartService, and stopService to call
process.exit(1) after logging errors instead of returning normally. Preserve the
existing error messages and successful execution flow so CLI consumers receive a
non-zero status only for validation failures.
- Around line 623-707: Align restartService with startService’s path-based
convention for --dockerfile and --additional-docker-files: read the Dockerfile
path and parse the additional-files path contents before passing them to the
command implementation. Update the corresponding restartService option handling
and argument mapping, or explicitly rename its inline-content flags to avoid
reusing these path-based names.
🪄 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: f5595119-43b4-4f7e-ae25-1f8669130a5c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
README.mdpackage.jsonsrc/cli.tssrc/commands.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- README.md
- src/commands.ts
| if (options.userData) { | ||
| const parsed = JSON.parse(options.userData); | ||
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | ||
| throw new Error("--user-data must be a JSON object"); | ||
| } | ||
| params.userData = parsed; | ||
| } else if (options.userDataFile) { | ||
| const parsed = JSON.parse(fs.readFileSync(options.userDataFile, "utf8")); | ||
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | ||
| throw new Error("--user-data-file must contain a JSON object"); | ||
| } | ||
| params.userData = parsed; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent priority between --user-data and --user-data-file in restartService.
If both flags are supplied, --user-data silently wins with no error (Lines 851-863), unlike the explicit --template/--image conflict check in startService (Lines 667-670). Add an explicit mutual-exclusion error for consistency and to avoid silently ignoring a user-supplied file.
Proposed fix
try {
+ if (options.userData && options.userDataFile) {
+ throw new Error("Provide either --user-data or --user-data-file, not both.");
+ }
if (options.userData) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.userData) { | |
| const parsed = JSON.parse(options.userData); | |
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | |
| throw new Error("--user-data must be a JSON object"); | |
| } | |
| params.userData = parsed; | |
| } else if (options.userDataFile) { | |
| const parsed = JSON.parse(fs.readFileSync(options.userDataFile, "utf8")); | |
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | |
| throw new Error("--user-data-file must contain a JSON object"); | |
| } | |
| params.userData = parsed; | |
| } | |
| if (options.userData && options.userDataFile) { | |
| throw new Error("Provide either --user-data or --user-data-file, not both."); | |
| } | |
| if (options.userData) { | |
| const parsed = JSON.parse(options.userData); | |
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | |
| throw new Error("--user-data must be a JSON object"); | |
| } | |
| params.userData = parsed; | |
| } else if (options.userDataFile) { | |
| const parsed = JSON.parse(fs.readFileSync(options.userDataFile, "utf8")); | |
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | |
| throw new Error("--user-data-file must contain a JSON object"); | |
| } | |
| params.userData = parsed; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 857-857: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(options.userDataFile, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli.ts` around lines 851 - 863, Update restartService to explicitly
reject supplying both options.userData and options.userDataFile before the
existing parsing branches, using a clear mutual-exclusion error; preserve the
current behavior when only one option is provided.
1 participant
Fixes #160
Add Service-on-Demand support to ocean-cli
Adds full CLI support for Service-on-Demand — launching long-running Docker
containers (JupyterLab, inference servers, nginx, …) on an Ocean Node compute
environment, paid up-front via on-chain escrow for a requested duration, and
reachable through forwarded ports. Unlike a compute job (runs an algorithm to
completion and exits), a service stays up until it expires, is stopped, or
is extended.
New commands (8)
getServiceTemplatesserviceTemplatesstartService--templateor--image)getServiceStatusmyServicesgetServiceslistServices--status/--include-all/--fromfiltersserviceLogscomputeServiceLogs--since)extendServicerestartService--cmd/--entrypointoverridestopServiceAll commands support both positional args and named options, work over HTTP and
P2P (transport auto-selected by ocean.js), and are picked up automatically by the
interactive REPL (no manual command-list maintenance).
Changes
src/serviceHelpers.ts(new) — pure, testable helpers: status labels &coloring, environment↔template resource matching (
availableFor,envSatisfiesTemplate,findServiceEnvironments,templateMismatchReason),default-resource resolution, client-side cost estimation,
userDataparsing/validation (keys-only echo — values are never logged), escrow
pre-verification with actionable remediation output, status polling, and a
human-first job pretty-printer.
src/commands.ts— eight newCommandsmethods plus a shared payment-prompthelper.
startServiceorchestrates the full flow: resolve env → resolvecontainer spec (template and/or explicit flags, with the
template.command → dockerCmd/template.entrypoint → dockerEntrypointrename) → resolveresources → estimate cost → pre-verify escrow → confirm → start → poll to
Runningand print the endpoint.src/cli.ts— command registration + option parsing (ports, JSONcmd/entrypoint, status filter). Read-only
getServiceTemplates/getServicesaccept an optional
--nodetarget, mirroringgetComputeEnvironments.test/serviceFlow.test.ts(new) — end-to-end system test covering the fulllifecycle, with a
skipLifecycleguard so it skips cleanly on a node withoutservices support.
README.md— feature bullet, a "Service-on-Demand" examples section, and aper-command option reference for all eight commands.
Notable correctness details
serviceRestartarg order (#2114):dockerCmd/dockerEntrypointsit betweenuserDataandsignal— the call passes all three before theAbortSignal.getServicesis node-wide, not owner-scoped (#2115): returnsServiceJobListed(docker image-spec fields stripped); results may include other owners' services.
serviceGetStreamableLogs(#2113): returns an async-iterable ornull—the null case is handled, and consumption reuses the exact buffer-then-print
pattern from
computeStreamableLogs(no forced timeout — logs are long-lived).surface only as an async
Error/*Failedstatus), and the CLI prints the exactdepositEscrow/authorizeEscrowremediation commands on failure.userDatavalues are never logged — only keys are echoed.Commandsmethods neverprocess.exitonrecoverable errors; the payment prompt opens its own readline (the REPL pauses to
yield stdin), mirroring
startCompute.Testing
Verified end-to-end against a running barge (ocean-node v4) — all eight commands,
both the happy path and failure/skip paths.
test/serviceFlow.test.tspasses10/10 in ~40 s using a lightweight custom image
(
nginxinc/nginx-unprivileged:alpineon port 8080):The client-side cost estimate matched the node-computed cost exactly, and
getServicesconfirmed thatdockerCmd/dockerEntrypoint/dockerfilearestripped from
ServiceJobListed.Summary by CodeRabbit