workflows comfy - #1442
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change expands service-template contracts and loading, adds LTX-2.3 product and multishot bundles with ComfyUI provisioning, and supports validated persistent output buckets for Docker service jobs. ChangesService template contracts and loading
Persistent output storage
LTX video bundles
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceTemplate
participant TemplateLoader
participant LTXBootstrap
participant ComfyUI
participant HuggingFace
ServiceTemplate->>TemplateLoader: Load commandFile and workflow files
TemplateLoader->>LTXBootstrap: Provide resolved command and workflow payload
LTXBootstrap->>HuggingFace: Download workflow-referenced safetensors
LTXBootstrap->>ComfyUI: Install workflows and start on port 8188
ComfyUI->>ComfyUI: Render shots and assemble the reel
Possibly related PRs
Suggested labels: 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 |
…nprotocol/ocean-node into feat/ltx-video-ugc-template
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/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 PR. The addition of UGC video workflows and the associated backend support (outputBucketId bind mounts, workflow templating logic, schema validation updates) are solidly architected. The template loading correctly mitigates directory traversal with absolute path checks and strict regexes on workflow IDs. Tests are comprehensive and clearly cover edge cases like unreadable paths or outside template paths. The schema validations are tight (e.g., mutually exclusive rules). LGTM!
Comments:
• [INFO][security] Great job ensuring directory traversal prevention here by fully resolving the target path and checking startsWith(resolvedDir + sep).
• [INFO][style] Good use of refine to strictly enforce the mutually exclusive file and graph payload for a workflow definition.
• [INFO][other] Good approach here: throwing an error gracefully prevents silent failures where a container starts without a required output mount.
• [INFO][security] Excellent regex guard for COMFY_WORKFLOW_ID to prevent path traversal when PACK directory is constructed further down.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/test/unit/service/serviceHandlers.test.ts (1)
163-168: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd authorization-failure coverage.
assertConsumerAllowedForBucketalways resolves inbuildFakes. The current tests cover forwarding and bucket-validation failure, but not the 403 authorization path. Add a test where the stub rejects, then assert that the response is 403 andcreateServiceJobis not called.Also applies to: 957-979
🤖 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/unit/service/serviceHandlers.test.ts` around lines 163 - 168, Add a unit test using the persistentStorage fake from buildFakes, configure assertConsumerAllowedForBucket to reject, and verify the handler returns HTTP 403 without calling createServiceJob. Keep the existing forwarding and bucket-validation failure tests unchanged.src/test/unit/service/templateLoader.test.ts (1)
107-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a workflow file that contains invalid JSON.
Tests 10 to 13 cover inlining, path escape, and command-file skipping. The remaining new branch in
templateLoader.ts(lines 104-109) is theJSON.parsefailure path. That branch drops the single workflow and keeps the template. No test asserts it.Add one test so a future change cannot silently promote that failure to a whole-template skip.
🧪 Proposed additional test
it('14. a workflow file with invalid JSON is dropped, template survives', async () => { mkdirSync(join(dir, 'workflows'), { recursive: true }) writeFileSync(join(dir, 'workflows', 'bad.json'), '{ not json') writeFileSync( join(dir, 'a.json'), JSON.stringify({ ...valid('tmpl-badwf'), workflows: [ { id: 'bad', name: 'Bad', file: 'workflows/bad.json' }, { id: 'inline', name: 'Inline', graph: { nodes: [] } } ] }) ) const [tmpl] = await loadServiceTemplates(dir) expect(tmpl.id).to.equal('tmpl-badwf') expect(tmpl.workflows.map((w) => w.id)).to.deep.equal(['inline']) })🤖 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/unit/service/templateLoader.test.ts` around lines 107 - 128, Add a unit test alongside the existing workflow-loading cases that creates a workflow file containing invalid JSON, references it from a template with a valid inline workflow, and asserts the template remains loaded while only the invalid workflow is dropped. Use the existing test setup and symbols such as loadServiceTemplates, valid, and dir.src/test/unit/service/serviceSchemas.test.ts (1)
89-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the fixture test: report the Zod error and check referenced files exist.
Two gaps:
- The assertion discards
parsed.error. A broken template reports only "expected false to equal true", with no reason.- The test does not check that
commandFileandworkflows[].filepoint at files that exist.loadServiceTemplatesdrops a workflow whose file is missing and skips a template whose command file is missing, both with a log warning only. A typo in a shipped example therefore passes this test.Also consider globbing the folder instead of naming two files, so every shipped example stays validated.
🧪 Proposed test improvement
- it('ltx-video-ugc-product/multishot: templates match ServiceTemplateSchema', () => { - const dir = join(process.cwd(), 'docs', 'serviceTemplates') - for (const file of ['ltx-video-ugc-product.json', 'ltx-video-ugc-multishot.json']) { - const tmpl = JSON.parse(readFileSync(join(dir, file), 'utf8')) - expect(ServiceTemplateSchema.safeParse(tmpl).success).to.equal(true) - } - }) + it('shipped docs/serviceTemplates entries match ServiceTemplateSchema', () => { + const dir = join(process.cwd(), 'docs', 'serviceTemplates') + const files = readdirSync(dir).filter((f) => f.endsWith('.json')) + expect(files.length).to.be.greaterThan(0) + for (const file of files) { + const raw = JSON.parse(readFileSync(join(dir, file), 'utf8')) + for (const tmpl of Array.isArray(raw) ? raw : [raw]) { + const parsed = ServiceTemplateSchema.safeParse(tmpl) + expect( + parsed.success, + `${file}: ${parsed.success ? '' : JSON.stringify(parsed.error.issues)}` + ).to.equal(true) + // Referenced files are resolved at load time and silently dropped when missing. + for (const rel of [tmpl.commandFile, ...(tmpl.workflows ?? []).map((w) => w.file)]) { + if (rel) expect(existsSync(join(dir, rel)), `${file} -> ${rel}`).to.equal(true) + } + } + } + })Add
readdirSyncandexistsSyncto thefsimport.🤖 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/unit/service/serviceSchemas.test.ts` around lines 89 - 95, Strengthen the test around the service template fixture loop by asserting with the parsed Zod result and including parsed.error on failure. Validate that each template’s commandFile and every workflows[].file resolves to an existing file, and prefer discovering all JSON fixtures in serviceTemplates via directory globbing/readdirSync so newly shipped examples are covered automatically; add the needed fs helpers.src/components/core/service/templateLoader.ts (1)
93-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an mtime-based template cache.
GET /serviceTemplatesreloads and parses every template and workflow file on each request. The multishot response is about 85 KB, including about 80 KB of workflow graphs. Preserve live reload while caching unchanged files and invalidating entries when template, workflow, or directory mtimes change.🤖 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/components/core/service/templateLoader.ts` around lines 93 - 112, Add an mtime-based cache around the template and workflow loading flow in the template loader, reusing parsed content when the corresponding file and containing directory mtimes are unchanged. Invalidate and reload entries when template files, workflow files, or relevant directories change, while preserving live reload and the existing behavior of dropping malformed workflows.
🤖 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 `@docs/serviceTemplates/ltx-video-ugc-bootstrap.sh`:
- Line 43: Update the curl invocation in the download loop to add explicit
connection and total transfer timeouts, and include --retry-connrefused
alongside the existing retry options. Also configure --speed-limit and
--speed-time to abort stalled slow transfers while preserving the existing
resume, output, and HTTP status handling.
- Around line 204-211: Update the TTS-Audio-Suite clone command in the
voice-conversion setup block to fetch a specific known-good commit or release
tag instead of the default-branch HEAD. Preserve the existing shallow-clone
behavior where compatible, and ensure the subsequent python3.13 install.py
invocation runs from that pinned checkout.
- Around line 222-223: Resolve the available Python interpreter once before the
existing Python invocations, assign it to PY_BIN, and fail early with a clear
error if no supported interpreter is found. Replace all five hard-coded
python3.13 calls, including the exec command near the end, with "$PY_BIN" while
preserving the current command arguments and error-handling behavior.
- Around line 37-61: Update the model download flow in get() and its download
loop to emit the README-defined [models] stdout markers: use downloading <name>
when starting, ready: <name> after a successful download, already present:
<name> for cached files, WARNING: could not download <name> on failure, and
bundle complete after all models finish. Replace the existing [ocean] progress
messages while preserving the current download validation and error behavior.
In `@docs/serviceTemplates/ltx-video-ugc-product.json`:
- Around line 81-86: The declared minimum disk resources are below the download
requirements. In docs/serviceTemplates/ltx-video-ugc-product.json:81-86, raise
requiredResources.disk.min above the 43.0 GB includes total and update the
description’s “38 GiB” figure to match. In
docs/serviceTemplates/ltx-video-ugc-multishot.json:92-97, add sizeGb to the
TTS-Audio-Suite include entry and raise requiredResources.disk.min to cover the
node pack and its Hugging Face weights.
In `@docs/serviceTemplates/README.md`:
- Around line 188-200: Update the voice conversion paragraph to remove the
instruction to upload reference speech. State that the reference voice is
sourced from Shot 1’s audio, and clarify that the “Reference voice override
(optional — not wired)” LoadAudio node is only available if the user wires it
manually.
- Around line 215-219: Update the workflow description near COMFY_WORKFLOW to
state that its value is gzip-compressed and then base64-encoded, rather than raw
gzip bytes. Also document that the payload may contain either a single graph or
an id-to-graph map, while preserving the existing installation and deep-link
details.
In `@src/`@types/C2D/ServiceOnDemand.ts:
- Around line 32-39: Update the ServiceTemplateCategory type union to Prettier’s
single-line format, keeping all existing members, single quotes, and no trailing
comma.
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3349-3357: Resolve and cache the result of serviceOutputMounts
before claimLock during initial startup, and before removing the existing
container/network during restart; reuse that cached mount in the corresponding
HostConfig construction. Ensure storage or mount-resolution failures occur
before charging or teardown, preserving the existing failure handling, and add
regression coverage for storage failure after request validation.
---
Nitpick comments:
In `@src/components/core/service/templateLoader.ts`:
- Around line 93-112: Add an mtime-based cache around the template and workflow
loading flow in the template loader, reusing parsed content when the
corresponding file and containing directory mtimes are unchanged. Invalidate and
reload entries when template files, workflow files, or relevant directories
change, while preserving live reload and the existing behavior of dropping
malformed workflows.
In `@src/test/unit/service/serviceHandlers.test.ts`:
- Around line 163-168: Add a unit test using the persistentStorage fake from
buildFakes, configure assertConsumerAllowedForBucket to reject, and verify the
handler returns HTTP 403 without calling createServiceJob. Keep the existing
forwarding and bucket-validation failure tests unchanged.
In `@src/test/unit/service/serviceSchemas.test.ts`:
- Around line 89-95: Strengthen the test around the service template fixture
loop by asserting with the parsed Zod result and including parsed.error on
failure. Validate that each template’s commandFile and every workflows[].file
resolves to an existing file, and prefer discovering all JSON fixtures in
serviceTemplates via directory globbing/readdirSync so newly shipped examples
are covered automatically; add the needed fs helpers.
In `@src/test/unit/service/templateLoader.test.ts`:
- Around line 107-128: Add a unit test alongside the existing workflow-loading
cases that creates a workflow file containing invalid JSON, references it from a
template with a valid inline workflow, and asserts the template remains loaded
while only the invalid workflow is dropped. Use the existing test setup and
symbols such as loadServiceTemplates, valid, and dir.
🪄 Autofix
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: e0ddb437-086c-4f4e-aace-134e5663a54d
📒 Files selected for processing (18)
docs/serviceTemplates/README.mddocs/serviceTemplates/ltx-video-ugc-bootstrap.shdocs/serviceTemplates/ltx-video-ugc-multishot.jsondocs/serviceTemplates/ltx-video-ugc-product.jsondocs/serviceTemplates/workflows/ocean_ugc_assemble.jsondocs/serviceTemplates/workflows/ocean_ugc_multishot.jsondocs/serviceTemplates/workflows/ocean_ugc_product.jsonsrc/@types/C2D/ServiceOnDemand.tssrc/@types/commands.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/service/startService.tssrc/components/core/service/templateLoader.tssrc/components/httpRoutes/compute.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceSchemas.test.tssrc/test/unit/service/templateLoader.test.tssrc/utils/config/schemas.ts
| get() { | ||
| if [ -f "$2" ]; then | ||
| echo "[ocean] cached $(basename "$2")" | ||
| return 0 | ||
| fi | ||
| echo "[ocean] downloading $(basename "$2")" | ||
| http_code=$(curl -L --retry 5 --retry-delay 5 -C - -o "$2.part" -w '%{http_code}' "$1") | ||
| if [ "$http_code" = "416" ] && [ -f "$2.part" ]; then | ||
| echo "[ocean] $(basename "$2").part already complete" | ||
| elif [ "$http_code" -lt 200 ] || [ "$http_code" -ge 300 ]; then | ||
| echo "[ocean] download failed for $(basename "$2") (HTTP $http_code)" >&2 | ||
| return 22 | ||
| fi | ||
| # A proxy or HF error page returns a few hundred bytes with HTTP 200; without this floor that | ||
| # body would be cached as a model. Smallest real file is ~300 MB. | ||
| size=$(wc -c < "$2.part") | ||
| if [ "$size" -lt 10485760 ]; then | ||
| echo "[ocean] $(basename "$2") is only $size bytes — not a model file. First bytes:" >&2 | ||
| head -c 300 "$2.part" >&2 || true | ||
| echo >&2 | ||
| rm -f "$2.part" | ||
| return 22 | ||
| fi | ||
| mv "$2.part" "$2" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Emit the [models] progress markers the README documents.
docs/serviceTemplates/README.md (lines 51-56) states that clients drive a "preparing models — N of M" progress line from [models]-prefixed stdout markers: downloading <name>, ready: <name>, already present: <name>, WARNING: could not download <name>, and bundle complete. This script prints [ocean] downloading …, [ocean] cached …, and [ocean] could not fetch …, and never prints a completion marker.
Both shipped bundles use this script, so no bundle in the repository produces the documented progress output. Either emit the markers here or correct the README so the convention matches what the reference bundles do.
📝 Proposed fix in `get()` and the download loop
get() {
if [ -f "$2" ]; then
- echo "[ocean] cached $(basename "$2")"
+ echo "[ocean] cached $(basename "$2")"
+ echo "[models] already present: $(basename "$2")"
return 0
fi
- echo "[ocean] downloading $(basename "$2")"
+ echo "[ocean] downloading $(basename "$2")"
+ echo "[models] downloading $(basename "$2")" mv "$2.part" "$2"
+ echo "[models] ready: $(basename "$2")"
} get "$url" "$MODELS/$sub/$(basename "$url")" ||
- echo "[ocean] could not fetch $(basename "$url") — ComfyUI will start without it" >&2
+ { echo "[ocean] could not fetch $(basename "$url") — ComfyUI will start without it" >&2
+ echo "[models] WARNING: could not download $(basename "$url")"; }
done
+ echo "[models] bundle complete"Also applies to: 144-157
🤖 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 `@docs/serviceTemplates/ltx-video-ugc-bootstrap.sh` around lines 37 - 61,
Update the model download flow in get() and its download loop to emit the
README-defined [models] stdout markers: use downloading <name> when starting,
ready: <name> after a successful download, already present: <name> for cached
files, WARNING: could not download <name> on failure, and bundle complete after
all models finish. Replace the existing [ocean] progress messages while
preserving the current download validation and error behavior.
| return 0 | ||
| fi | ||
| echo "[ocean] downloading $(basename "$2")" | ||
| http_code=$(curl -L --retry 5 --retry-delay 5 -C - -o "$2.part" -w '%{http_code}' "$1") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add connect and transfer timeouts to curl.
curl runs with --retry 5 --retry-delay 5 and no timeout. A stalled connection blocks the download loop with no upper bound. The service is time-boxed and already paid, so a hang consumes the whole session and produces no output.
Set --connect-timeout and --max-time, and add --retry-connrefused so retries also cover a refused connection.
🛡️ Proposed fix
- http_code=$(curl -L --retry 5 --retry-delay 5 -C - -o "$2.part" -w '%{http_code}' "$1")
+ http_code=$(curl -L --retry 5 --retry-delay 5 --retry-connrefused \
+ --connect-timeout 30 --max-time 7200 --speed-limit 1024 --speed-time 120 \
+ -C - -o "$2.part" -w '%{http_code}' "$1")--speed-limit/--speed-time abort a transfer that drops below 1 KB/s for two minutes, which --max-time alone cannot detect on a slow-but-alive connection.
📝 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.
| http_code=$(curl -L --retry 5 --retry-delay 5 -C - -o "$2.part" -w '%{http_code}' "$1") | |
| http_code=$(curl -L --retry 5 --retry-delay 5 --retry-connrefused \ | |
| --connect-timeout 30 --max-time 7200 --speed-limit 1024 --speed-time 120 \ | |
| -C - -o "$2.part" -w '%{http_code}' "$1") |
🤖 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 `@docs/serviceTemplates/ltx-video-ugc-bootstrap.sh` at line 43, Update the curl
invocation in the download loop to add explicit connection and total transfer
timeouts, and include --retry-connrefused alongside the existing retry options.
Also configure --speed-limit and --speed-time to abort stalled slow transfers
while preserving the existing resume, output, and HTTP status handling.
| elif [ -d "$VC_DIR/.git" ] || git clone --depth 1 \ | ||
| https://github.com/diodiogod/TTS-Audio-Suite.git "$VC_DIR"; then | ||
| echo "[ocean] installing voice-conversion dependencies (first launch is slow)" | ||
| # install.py, not `pip install -r requirements.txt`: that file deliberately omits librosa | ||
| # and descript-audio-codec, which install.py adds with --no-deps so pip cannot downgrade | ||
| # numpy/torch under ComfyUI. Installing the requirements alone leaves ChatterboxVC | ||
| # unimportable ("ChatterboxVC not available - check installation"). | ||
| ( cd "$VC_DIR" && python3.13 install.py ) || |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the TTS-Audio-Suite checkout to a commit.
git clone --depth 1 takes the current default-branch HEAD of a third-party repository. The script then runs python3.13 install.py from that checkout on every launch. The container executes unpinned remote code, and install.py installs packages into the container's own site-packages. A change in that repository, whether intentional or from a compromise, changes what runs inside the consumer's paid session. There is no commit pin and no integrity check.
Pin a known-good commit or release tag.
🔒 Proposed fix
+ # Pinned: the bootstrap runs install.py from this checkout, so an unpinned HEAD
+ # would execute whatever the upstream repository contains at launch time.
+ VC_REF="<known-good-commit-sha>"
VC_DIR="$BASE/custom_nodes/TTS-Audio-Suite"
if ! command -v git >/dev/null 2>&1; then
echo "[ocean] no git in the image — skipping voice-conversion nodes" >&2
- elif [ -d "$VC_DIR/.git" ] || git clone --depth 1 \
- https://github.com/diodiogod/TTS-Audio-Suite.git "$VC_DIR"; then
+ elif [ -d "$VC_DIR/.git" ] || ( git clone --filter=blob:none --no-checkout \
+ https://github.com/diodiogod/TTS-Audio-Suite.git "$VC_DIR" &&
+ git -C "$VC_DIR" checkout --detach "$VC_REF" ); then🤖 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 `@docs/serviceTemplates/ltx-video-ugc-bootstrap.sh` around lines 204 - 211,
Update the TTS-Audio-Suite clone command in the voice-conversion setup block to
fetch a specific known-good commit or release tag instead of the default-branch
HEAD. Preserve the existing shallow-clone behavior where compatible, and ensure
the subsequent python3.13 install.py invocation runs from that pinned checkout.
| exec python3.13 /default-comfyui-bundle/ComfyUI/main.py \ | ||
| --base-directory "$BASE" ${OUTPUT_DIR_ARGS} --listen --port 8188 ${CLI_ARGS:-} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not hard-code python3.13 on the exec line.
The script calls python3.13 five times. Four calls are guarded by || true or a surrounding || handler. This one is not. If the image yanwk/comfyui-boot:cu130-megapak-pt211-20260803 ships a different Python minor version, or a later tag changes it, exec fails and the container exits immediately after escrow is already claimed.
Resolve the interpreter once, fail early with a clear message, and reuse the variable.
🛡️ Proposed fix
+# Resolve the interpreter once. The image pins 3.13 today; fail loudly rather than
+# exec'ing a binary that does not exist.
+PY_BIN=""
+for c in python3.13 python3 python; do
+ if command -v "$c" >/dev/null 2>&1; then PY_BIN="$c"; break; fi
+done
+if [ -z "$PY_BIN" ]; then
+ echo "[ocean] no python interpreter found in the image" >&2
+ exit 1
+fiThen replace each python3.13 call with "$PY_BIN", including line 222:
-exec python3.13 /default-comfyui-bundle/ComfyUI/main.py \
+exec "$PY_BIN" /default-comfyui-bundle/ComfyUI/main.py \
--base-directory "$BASE" ${OUTPUT_DIR_ARGS} --listen --port 8188 ${CLI_ARGS:-}🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 223-223: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 223-223: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@docs/serviceTemplates/ltx-video-ugc-bootstrap.sh` around lines 222 - 223,
Resolve the available Python interpreter once before the existing Python
invocations, assign it to PY_BIN, and fail early with a clear error if no
supported interpreter is found. Replace all five hard-coded python3.13 calls,
including the exec command near the end, with "$PY_BIN" while preserving the
current command arguments and error-handling behavior.
| { | ||
| "id": "disk", | ||
| "min": 40, | ||
| "recommended": 80, | ||
| "unit": "GB" | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both templates set disk.min below their own includes manifest sum. The shared root cause is that requiredResources.disk was not derived from the declared download sizes. requiredResources gates SERVICE_START, so a consumer granted the minimum starts the service, pays, and then runs out of space during the download.
docs/serviceTemplates/ltx-video-ugc-product.json#L81-L86: theincludesentries sum to 43.0 GB whiledisk.minis 40. Raisedisk.minabove the sum, and align the "38 GiB" figure indescription(line 4) with the manifest.docs/serviceTemplates/ltx-video-ugc-multishot.json#L92-L97: the same 43.0 GB of models plus the unsizedTTS-Audio-Suiteentry against adisk.minof 45. SetsizeGbon that entry and raisedisk.minto cover the node pack and its Hugging Face weights.
📍 Affects 2 files
docs/serviceTemplates/ltx-video-ugc-product.json#L81-L86(this comment)docs/serviceTemplates/ltx-video-ugc-multishot.json#L92-L97
🤖 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 `@docs/serviceTemplates/ltx-video-ugc-product.json` around lines 81 - 86, The
declared minimum disk resources are below the download requirements. In
docs/serviceTemplates/ltx-video-ugc-product.json:81-86, raise
requiredResources.disk.min above the 43.0 GB includes total and update the
description’s “38 GiB” figure to match. In
docs/serviceTemplates/ltx-video-ugc-multishot.json:92-97, add sizeGb to the
TTS-Audio-Suite include entry and raise requiredResources.disk.min to cover the
node pack and its Hugging Face weights.
| **Voice.** LTX builds each clip's audio from a fresh `LTXVEmptyLatentAudio` conditioned only | ||
| on the text — there is no speaker embedding, so a spoken line is a different person on every | ||
| cut and no amount of prompting locks it. Frame chaining carries the face; nothing carries the | ||
| voice. | ||
|
|
||
| Assemble concatenates the clips' own audio, which keeps ambience and picture in sync, then | ||
| runs the stitched track through **voice conversion** — the same thing dubbing pipelines do. | ||
| `UnifiedVoiceChangerNode` remaps every line to one reference speaker while leaving the words, | ||
| their timing and the room tone alone. That is why conversion beats dubbing a voiceover over | ||
| the footage: the phonemes never move, so ambience survives and the lips still match. Upload a | ||
| few seconds of clean speech as the reference voice; `refinement_passes` 1 is usually right | ||
| (above ~5 it distorts) and `max_chunk_duration` bounds VRAM, rejoining chunks with | ||
| timing-preserving concatenation. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the reference-voice instruction. The workflow does not need an upload.
This paragraph tells the user to upload a few seconds of clean speech as the reference voice. The shipped graph does the opposite. In docs/serviceTemplates/workflows/ocean_ugc_assemble.json, link 65 wires node 20 ("Shot 1 components") audio output into node 62 UnifiedVoiceChangerNode input narrator_target. Node 60 LoadAudio is titled "Reference voice override (optional — not wired)". The workflow's own note and ltx-video-ugc-multishot.json (line 66) both state that no upload is required.
Update the text to state that the reference voice comes from Shot 1's audio, and that LoadAudio is an optional override the user must wire manually.
📝 Proposed documentation fix
-Assemble concatenates the clips' own audio, which keeps ambience and picture in sync, then
-runs the stitched track through **voice conversion** — the same thing dubbing pipelines do.
-`UnifiedVoiceChangerNode` remaps every line to one reference speaker while leaving the words,
-their timing and the room tone alone. That is why conversion beats dubbing a voiceover over
-the footage: the phonemes never move, so ambience survives and the lips still match. Upload a
-few seconds of clean speech as the reference voice; `refinement_passes` 1 is usually right
-(above ~5 it distorts) and `max_chunk_duration` bounds VRAM, rejoining chunks with
-timing-preserving concatenation.
+Assemble concatenates the clips' own audio, which keeps ambience and picture in sync, then
+runs the stitched track through **voice conversion** — the same thing dubbing pipelines do.
+`UnifiedVoiceChangerNode` remaps every line to one reference speaker while leaving the words,
+their timing and the room tone alone. That is why conversion beats dubbing a voiceover over
+the footage: the phonemes never move, so ambience survives and the lips still match. The
+reference voice is taken from Shot 1's own audio, already wired in the graph — nothing to
+upload. To use a different voice, wire another shot's `audio` output (or the optional
+**Reference voice override** `LoadAudio` node) into `narrator_target`. `refinement_passes` 1
+is usually right (above ~5 it distorts) and `max_chunk_duration` bounds VRAM, rejoining
+chunks with timing-preserving concatenation.📝 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.
| **Voice.** LTX builds each clip's audio from a fresh `LTXVEmptyLatentAudio` conditioned only | |
| on the text — there is no speaker embedding, so a spoken line is a different person on every | |
| cut and no amount of prompting locks it. Frame chaining carries the face; nothing carries the | |
| voice. | |
| Assemble concatenates the clips' own audio, which keeps ambience and picture in sync, then | |
| runs the stitched track through **voice conversion** — the same thing dubbing pipelines do. | |
| `UnifiedVoiceChangerNode` remaps every line to one reference speaker while leaving the words, | |
| their timing and the room tone alone. That is why conversion beats dubbing a voiceover over | |
| the footage: the phonemes never move, so ambience survives and the lips still match. Upload a | |
| few seconds of clean speech as the reference voice; `refinement_passes` 1 is usually right | |
| (above ~5 it distorts) and `max_chunk_duration` bounds VRAM, rejoining chunks with | |
| timing-preserving concatenation. | |
| **Voice.** LTX builds each clip's audio from a fresh `LTXVEmptyLatentAudio` conditioned only | |
| on the text — there is no speaker embedding, so a spoken line is a different person on every | |
| cut and no amount of prompting locks it. Frame chaining carries the face; nothing carries the | |
| voice. | |
| Assemble concatenates the clips' own audio, which keeps ambience and picture in sync, then | |
| runs the stitched track through **voice conversion** — the same thing dubbing pipelines do. | |
| `UnifiedVoiceChangerNode` remaps every line to one reference speaker while leaving the words, | |
| their timing and the room tone alone. That is why conversion beats dubbing a voiceover over | |
| the footage: the phonemes never move, so ambience survives and the lips still match. The | |
| reference voice is taken from Shot 1's own audio, already wired in the graph — nothing to | |
| upload. To use a different voice, wire another shot's `audio` output (or the optional | |
| **Reference voice override** `LoadAudio` node) into `narrator_target`. `refinement_passes` 1 | |
| is usually right (above ~5 it distorts) and `max_chunk_duration` bounds VRAM, rejoining | |
| chunks with timing-preserving concatenation. |
🤖 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 `@docs/serviceTemplates/README.md` around lines 188 - 200, Update the voice
conversion paragraph to remove the instruction to upload reference speech. State
that the reference voice is sourced from Shot 1’s audio, and clarify that the
“Reference voice override (optional — not wired)” LoadAudio node is only
available if the user wires it manually.
| The workflow arrives as userData (`COMFY_WORKFLOW_ID` + gzipped `COMFY_WORKFLOW`) and is | ||
| installed at `custom_nodes/<id>/example_workflows/<id>.json`, which ComfyUI serves at | ||
| `/api/workflow_templates/<id>/<id>.json` — so `?template=<id>&source=<id>` deep-links it. | ||
| `source` must name the module; `source=all` only searches ComfyUI's own templates. It is also | ||
| copied into `user/default/workflows/` so it appears in the Workflows sidebar. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State the full COMFY_WORKFLOW encoding.
The text describes COMFY_WORKFLOW as gzipped. ltx-video-ugc-bootstrap.sh line 74 decodes it with base64 -d | gunzip, so the value must be gzipped and then base64-encoded. A client that sends raw gzip bytes fails. The script also accepts an id -> graph map, not only a single graph. Document both details.
📝 Proposed documentation fix
-The workflow arrives as userData (`COMFY_WORKFLOW_ID` + gzipped `COMFY_WORKFLOW`) and is
-installed at `custom_nodes/<id>/example_workflows/<id>.json`, which ComfyUI serves at
+The workflow arrives as userData (`COMFY_WORKFLOW_ID` + `COMFY_WORKFLOW`, the graph JSON
+gzipped and then base64-encoded; either a single graph or an `{"<id>": <graph>, ...}` map)
+and is installed at `custom_nodes/<id>/example_workflows/<id>.json`, which ComfyUI serves at📝 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.
| The workflow arrives as userData (`COMFY_WORKFLOW_ID` + gzipped `COMFY_WORKFLOW`) and is | |
| installed at `custom_nodes/<id>/example_workflows/<id>.json`, which ComfyUI serves at | |
| `/api/workflow_templates/<id>/<id>.json` — so `?template=<id>&source=<id>` deep-links it. | |
| `source` must name the module; `source=all` only searches ComfyUI's own templates. It is also | |
| copied into `user/default/workflows/` so it appears in the Workflows sidebar. | |
| The workflow arrives as userData (`COMFY_WORKFLOW_ID` + `COMFY_WORKFLOW`, the graph JSON | |
| gzipped and then base64-encoded; either a single graph or an `{"<id>": <graph>, ...}` map) | |
| and is installed at `custom_nodes/<id>/example_workflows/<id>.json`, which ComfyUI serves at | |
| `/api/workflow_templates/<id>/<id>.json` — so `?template=<id>&source=<id>` deep-links it. | |
| `source` must name the module; `source=all` only searches ComfyUI's own templates. It is also | |
| copied into `user/default/workflows/` so it appears in the Workflows sidebar. |
🤖 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 `@docs/serviceTemplates/README.md` around lines 215 - 219, Update the workflow
description near COMFY_WORKFLOW to state that its value is gzip-compressed and
then base64-encoded, rather than raw gzip bytes. Also document that the payload
may contain either a single graph or an id-to-graph map, while preserving the
existing installation and deep-link details.
| export type ServiceTemplateCategory = | ||
| | 'image' | ||
| | 'video' | ||
| | 'llm' | ||
| | 'serving' | ||
| | 'notebook' | ||
| | 'embeddings' | ||
| | 'app' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Collapse the union to satisfy Prettier.
ESLint reports a prettier/prettier error on this union. The members fit inside the 90-character print width on one line. Fix it to keep the lint job green.
As per coding guidelines: "Follow Prettier formatting: no semicolons, single quotes, 90-character print width, no trailing commas, and two-space indentation."
🎨 Proposed formatting fix
-export type ServiceTemplateCategory =
- | 'image'
- | 'video'
- | 'llm'
- | 'serving'
- | 'notebook'
- | 'embeddings'
- | 'app'
+export type ServiceTemplateCategory =
+ 'image' | 'video' | 'llm' | 'serving' | 'notebook' | 'embeddings' | 'app'📝 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.
| export type ServiceTemplateCategory = | |
| | 'image' | |
| | 'video' | |
| | 'llm' | |
| | 'serving' | |
| | 'notebook' | |
| | 'embeddings' | |
| | 'app' | |
| export type ServiceTemplateCategory = | |
| 'image' | 'video' | 'llm' | 'serving' | 'notebook' | 'embeddings' | 'app' |
🧰 Tools
🪛 ESLint
[error] 33-39: Replace |·'image'⏎··|·'video'⏎··|·'llm'⏎··|·'serving'⏎··|·'notebook'⏎··|·'embeddings'⏎· with 'image'·|·'video'·|·'llm'·|·'serving'·|·'notebook'·|·'embeddings'
(prettier/prettier)
🤖 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/`@types/C2D/ServiceOnDemand.ts around lines 32 - 39, Update the
ServiceTemplateCategory type union to Prettier’s single-line format, keeping all
existing members, single quotes, and no trailing comma.
Sources: Coding guidelines, Linters/SAST tools
| // Bind-mount for the job's output bucket, when it has one. Throws instead of returning an | ||
| // empty list: an unmounted container silently re-downloads its models and writes results | ||
| // nowhere, so a mount failure must fail the start/restart. | ||
| private async serviceOutputMounts(job: ServiceJob): Promise<DockerMountObject[]> { | ||
| if (!job.outputBucketId) return [] | ||
| const ps = OceanNode.getInstance().getPersistentStorage() | ||
| if (!ps) throw new Error('Persistent storage is not configured on this node') | ||
| return [await ps.getDockerOutputMountObject(job.outputBucketId, job.owner)] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve output mounts before claim and restart teardown.
serviceOutputMounts can throw when persistent storage is unavailable or mount resolution fails. Initial startup calls it after claimLock completes at Line 3563-3581. The failure path then sees claimTx and skips the refund path, so the consumer can be charged without a running container.
Restart calls the same helper after the old container and network are removed at Line 4218-4239. A mount failure can therefore destroy a healthy service and leave it in Error.
Resolve and cache the mount before claimLock and before restart teardown. Reuse the resolved mount in HostConfig, or define an explicit refund and rollback path for mount failures. Add regression coverage for storage failure after request validation.
Also applies to: 3643-3643, 4343-4343
🤖 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/components/c2d/compute_engine_docker.ts` around lines 3349 - 3357,
Resolve and cache the result of serviceOutputMounts before claimLock during
initial startup, and before removing the existing container/network during
restart; reuse that cached mount in the corresponding HostConfig construction.
Ensure storage or mount-resolution failures occur before charging or teardown,
preserving the existing failure handling, and add regression coverage for
storage failure after request validation.
3 participants
Fixes # .
Changes proposed in this PR:
Summary by CodeRabbit