Implementing a CWL exporter for tool-spec - #2
Conversation
WalkthroughAdds a new CWL exporter and supporting types, registers it in the export registry, and extends the export UI/page to configure and invoke CWL exports (version, outputs, base command, container). Implements CWL v1.2/v1.1 YAML generation, validation, and download behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as +page.svelte
participant Exporter as CwlExporter
participant YAML as "CWL YAML"
User->>UI: select "CWL" and configure (version, outputs, base, container)
User->>UI: trigger export/preview
UI->>Exporter: export(metadata, cwlConfig)
Exporter->>Exporter: buildInputs(metadata)
Exporter->>Exporter: buildOutputs(cwlConfig.outputs)
Exporter->>Exporter: generateCommandSequence(metadata, cwlConfig, container)
Exporter->>Exporter: assemble CWL doc (id,label,doc,inputs,outputs,baseCommand,requirements)
Exporter->>YAML: dump YAML string
YAML-->>UI: return YAML
UI->>User: show preview or download file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/routes/+page.svelte (1)
611-757: CWL configuration UI is clear; consider minimal validation feedbackThe CWL block (version selector, outputs, base command, container) matches the config shape and mirrors the Galaxy UX nicely. Since the exporter enforces required
name/type/globfields, you might optionally add lightweight inline validation indicators (e.g., mark missing required fields before export) to avoid users discovering issues only via runtime errors, but this is non-blocking.src/lib/exporters/cwl-exporter.ts (2)
383-421: Validation logic is reasonable; consider surfacing warnings in UIThe
validatemethod correctly enforces “at least one input or output” and checks required fields on outputs, while treating missing outputs as a warning viaconsole.warn. The core checks are appropriate.If you later wire validation into the UI, consider returning structured warnings alongside errors instead of relying solely on
console.warn, so the Svelte layer can display them explicitly. Not required for this PR.
229-252:generateInputBindingis currently unused
generateInputBindingisn’t referenced anywhere, and you explicitly avoidinputBindingfor tool-spec tools. Unless you plan to use it in the near term, you could remove it or add a short comment indicating it’s reserved for future non–tool-spec exporters to keep the class lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/exporters/cwl-exporter.ts(1 hunks)src/lib/exporters/export-registry.ts(2 hunks)src/lib/unified-metadata.ts(1 hunks)src/routes/+page.svelte(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/exporters/export-registry.ts (1)
src/lib/exporters/cwl-exporter.ts (1)
CwlExporter(8-439)
src/lib/exporters/cwl-exporter.ts (1)
src/lib/unified-metadata.ts (3)
UnifiedSoftwareMetadata(106-139)CwlExportConfig(99-104)ToolSpecParameter(39-48)
🔇 Additional comments (8)
src/lib/exporters/export-registry.ts (1)
5-30: CWL exporter registration looks consistent and non-disruptiveThe new
CwlExporterimport andcwlentry integrate cleanly with the existing registry: uniqueid, descriptivename/description, and use of a dedicated exporter instance, without impacting the helper functions that consumeexportFormats.src/lib/unified-metadata.ts (1)
91-105: CWL config types align with exporter and UI usage
CwlOutputandCwlExportConfigmatch how the CWL exporter and Svelte page consume them (outputs array, version union, optional baseCommand/container). Allowingtypeas a union withstringleaves room for advanced CWL types while covering common cases.src/routes/+page.svelte (4)
6-7: CWL state and initialization mirror Galaxy behavior appropriatelyThe added CWL-specific state (
cwlOutputs,cwlVersion,cwlBaseCommand,cwlContainer) and initialization frommetadata.galaxyConfig/repository.fullNamereuse the same patterns as Galaxy, giving sensible defaults while allowing override. This keeps the mental model consistent across formats.Also applies to: 40-45, 68-77
138-167: Export handler correctly passes CWL config through exporterThe
selectedExportFormat === 'cwl'branch builds aCwlExportConfigwith version, outputs, and optional baseCommand/container, then forwards it todownload. Container fallback tometadata.galaxyConfig?.containermirrors the defaulting in the exporter and Galaxy UI, so behavior should be predictable.
175-204: Preview path mirrors CWL export configuration
getCurrentExportDataconstructs the sameCwlExportConfigashandleExportfor thecwlbranch. Keeping these in sync avoids “preview vs. downloaded file” discrepancies, which is especially important while iterating on CWL settings.
229-247: CWL output helpers are straightforward and type-safe
addCwlOutput,removeCwlOutput, andupdateCwlOutputfollow the same immutable update pattern used for Galaxy outputs, andkeyof CwlOutputkeeps field names aligned with the shared type. The default added output (File,*.out) provides a useful starting point.src/lib/exporters/cwl-exporter.ts (2)
426-438: Download override follows existing pattern and looks correctThe
downloadmethod simply threads the CWL config throughexport, wraps the result in a Blob, and triggers an anchor click. This aligns with typical browser download patterns and with how other exporters in the codebase are expected to behave.
15-48: CWL v1.2 support for$namespacesand schema.org metadata is confirmedThe CWL v1.2 specification explicitly allows implementation-extension metadata fields using namespace prefixes declared in
$namespaces. Schema.org namespace usage is recommended in the spec and shown in examples. cwltool and other ecosystem tools (CWL Viewer, community documentation) accept and recommend s:author, s:license, and s:version metadata. cwltool treats these as non-execution extensions and will run the document successfully, though it may historically emit benign warnings about unrecognized extension fields.Your CWL document construction—including the
$namespacesands:metadata fields—aligns with spec expectations and common runner behavior. The code is ready to proceed.
| private buildInputs(data: UnifiedSoftwareMetadata): Record<string, any> { | ||
| const inputs: Record<string, any> = {}; | ||
|
|
||
| if (!data.toolSpec.parameters) { | ||
| return inputs; | ||
| } | ||
|
|
||
| // Tool-spec tools read inputs from inputs.json, not command-line arguments | ||
| // So we don't add inputBinding - inputs will be staged as files | ||
| // and a preprocessing step (user-provided or custom) will generate inputs.json | ||
| for (const [paramName, paramDef] of Object.entries(data.toolSpec.parameters)) { | ||
| const input: any = { | ||
| type: this.mapToolSpecTypeToCwl(paramDef), | ||
| label: paramName.replace(/_/g, ' ').replace(/\b\w/g, l => l.toUpperCase()) | ||
| }; | ||
|
|
||
| if (paramDef.description) { | ||
| input.doc = paramDef.description; | ||
| } | ||
|
|
||
| // Note: No inputBinding - tool-spec doesn't use command-line args | ||
| // Inputs will be staged and used to generate inputs.json | ||
|
|
||
| // Add default value if available | ||
| if (paramDef.default !== undefined && paramDef.default !== null) { | ||
| input.default = paramDef.default; | ||
| } | ||
|
|
||
| inputs[paramName] = input; | ||
| } | ||
|
|
||
| return inputs; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Type mapping is reasonable; optional/array semantics for enums could be improved
The mapping from ToolSpecParameter to CWL types (string → string, integer → int, asset → File, etc.) is sensible, and handling of arrays via { type: 'array', items: ... } is fine for most cases.
Two potential refinements (non-blocking):
-
Optional enums: for
type: 'enum'you currently ignoreparamDef.optional; CWL usually models nullable enums as a union like['null', { type: 'enum', symbols: [...] }]. Right now, optional enums will appear required in the CWL layer. -
Optional arrays:
paramDef.array && paramDef.optionalyieldsitems: ['null', 'string'](or similar), which allowsnullelements rather than an optional parameter. The more canonical encoding would be something like['null', { type: 'array', items: 'string' }].
These are spec-alignment details and don’t block initial adoption but could improve correctness with strict CWL validation.
Please confirm the preferred encoding for optional enum and array types with your CWL target environment before changing this, as implementations may differ slightly.
Also applies to: 182-224
🌐 Web query:
What is the recommended CWL v1.1/v1.2 representation for optional enum and optional array parameters in CommandLineTool inputs?
💡 Result:
Recommended approach: make the input type a union with "null" plus the enum/array schema (or use the shorthand ? where allowed). Examples:
-
Optional enum (explicit schema):
type:- "null"
- name: format_choices
type: enum
symbols: [auto, fasta, fastq]
(equivalently you can use the nullable shorthand where appropriate: format_choices? ). [1][2]
-
Optional array (explicit schema):
type:- "null"
- type: array
items: string
(or use string[]? shorthand when the implementation accepts it). [3][1]
References:
- CWL v1.2 CommandLineTool / CommandInputEnumSchema & CommandInputArraySchema. [1]
- CWL user guide example showing nullable enum in inputs. [2]
- CWL user guide section on array inputs (array and [] forms). [3]
:::
🏁 Script executed:
#!/bin/bash
# Find and display the mapToolSpecTypeToCwl function in cwl-exporter.ts
rg -n "mapToolSpecTypeToCwl" src/lib/exporters/cwl-exporter.ts -A 50 -B 2Length of output: 2050
🏁 Script executed:
#!/bin/bash
# Also search for enum and array handling to understand current implementation
rg -n "type.*enum|type.*array" src/lib/exporters/cwl-exporter.ts -B 3 -A 3Length of output: 280
🏁 Script executed:
#!/bin/bash
# Get lines 182-224 mentioned in review
sed -n '182,224p' src/lib/exporters/cwl-exporter.tsLength of output: 807
Fix enum and array handling to match CWL spec for optional parameters
The current implementation has two bugs:
-
Optional enums (line 198-202): Early return prevents optional check. Optional enums should be
type: ['null', { type: 'enum', symbols: [...] }]but currently output{ type: 'enum', symbols: [...] }(missing null union). -
Optional arrays (lines 211-220): Order of operations is wrong. When both
optionalandarrayare true, the code produces{ type: 'array', items: ['null', 'string'] }(null at item level), but spec requirestype: ['null', { type: 'array', items: 'string' }](null at outer type level).
Fix approach:
- For enums: Remove early return and let enum fall through to optional/array handling
- For arrays: Apply array wrapping first, then wrap final result with null union if optional
🤖 Prompt for AI Agents
In src/lib/exporters/cwl-exporter.ts around lines 145 to 177, the enum and array
handling for optional parameters is incorrect: remove the early return for enum
types so enums fall through to the optional/array logic (so optional enums
become type: ['null', { type: 'enum', symbols: [...] }]), and change the
wrapping order for arrays so you first construct the array type ({ type:
'array', items: <innerType> }) and only then, if optional is true, wrap the
whole array type in a null-union (['null', <arrayType>]) instead of placing
'null' inside items. Ensure the same pattern applies when the inner type is an
enum or primitive.
| private generateCommandSequence(data: UnifiedSoftwareMetadata, config: CwlExportConfig, container?: string): string | undefined { | ||
| if (!container) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const commands: string[] = []; | ||
|
|
||
| // Step 1: Generate inputs.json from CWL inputs | ||
| // We'll use a Python script that reads CWL input values | ||
| if (data.toolSpec.parameters && Object.keys(data.toolSpec.parameters).length > 0) { | ||
| commands.push(this.generateInputsJsonCommand(data)); | ||
| } else { | ||
| commands.push('mkdir -p inputs && echo "{}" > inputs/inputs.json'); | ||
| } | ||
|
|
||
| // Step 2: Run docker with proper mountpoints and RUN_TOOL | ||
| commands.push(this.generateDockerCommand(data, config, container)); | ||
|
|
||
| return commands.join(' && '); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Docker command paths should rely on runtime.outdir rather than escaped $(pwd)
The docker run command is built as:
let dockerCmd = `docker run --rm -v "\$(pwd)/inputs:/data/in:ro" -v "\$(runtime.outdir)/outputs:/data/out:rw" -e RUN_TOOL=${toolName} ${container}`;Issues:
-
\$(pwd)will be passed literally tosh -c(the backslash escapes the$in the shell), sopwdis never executed and the host path becomes"$(pwd)/inputs", which is almost certainly invalid. - The same backslash before
$(runtime.outdir)is unnecessary and may interfere with CWL expression interpolation, depending on the runner.
Given CWL already provides runtime.outdir, you can avoid $(pwd) entirely and lean on it for both mounts, e.g.:
- let dockerCmd = `docker run --rm -v "\$(pwd)/inputs:/data/in:ro" -v "\$(runtime.outdir)/outputs:/data/out:rw" -e RUN_TOOL=${toolName} ${container}`;
+ let dockerCmd =
+ 'docker run --rm' +
+ ' -v "$(runtime.outdir)/inputs:/data/in:ro"' +
+ ' -v "$(runtime.outdir)/outputs:/data/out:rw"' +
+ ` -e RUN_TOOL=${toolName} ${container}`;This relies on CWL to interpolate $(runtime.outdir) to the working directory path before the shell runs, and avoids shell-level $(...) substitution conflicts.
Please verify with your target CWL runner that $(runtime.outdir) is expanded as expected inside argument strings and that the resulting docker -v paths are valid.
Also applies to: 362-381
🌐 Web query:
Does CWL v1.1/v1.2 guarantee that `$(runtime.outdir)` used inside an `arguments` string is expanded before the command is executed, and is it standard practice to mount `$(runtime.outdir)/...` into nested docker containers?
💡 Result:
Short answers
-
Does CWL guarantee
$(runtime.outdir) in arguments is expanded before the command runs? — Yes: CWL expressions (e.g. $ (runtime.outdir)) are specified to be evaluated by the runtime and may be used in arguments; when running in Docker the platform is expected to rewrite runtime.outdir to a valid path inside the container. However the spec also permits implementations to do deferred/opaque resolution of runtime fields (i.e. provide an opaque string) so behaviour can vary by runner. [1][2] -
Is it standard practice to mount $(runtime.outdir)/... into nested Docker containers? — No: CWL itself does not require or standardize mounting into nested containers. Making host-mounted directories visible to a container started from inside another container requires Docker-in-Docker or careful bind-propagation (rshared/rslave) and is an implementation/platform concern rather than a CWL-level guarantee. If you need nested-container access, arrange host-volume mounts or use DinD/appropriate bind-propagation on the runner. [1][3]
Sources
- CWL CommandLineTool / expressions / runtime fields (v1.2). [1]
- CWL user guide example showing arguments: ["-d", $(runtime.outdir)]. [2]
- Docker bind-mount and bind-propagation notes (rshared/rslave) for making mounts visible to nested containers. [3]
Now let me inspect the actual code to verify the implementation details:
Based on the comprehensive web search results, I can now verify the technical concerns in the review comment. Let me now inspect the actual code to confirm the exact issue:
[/run_scripts]
I need to check if there are results from the script. Let me try a more specific approach:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Locate and display the cwl-exporter.ts file content around the mentioned lines
fd "cwl-exporter.ts" -type f --exec cat {} \; | head -400 | tail -120Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for generateDockerCommand or similar methods that construct docker run commands
rg -n "docker run" --type ts -A 3 -B 3 | head -80Length of output: 1015
Now let me inspect the actual code around both line ranges mentioned:
[/run_scripts]
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# View lines around 285-304 to see the first method
sed -n '280,310p' src/lib/exporters/cwl-exporter.tsLength of output: 1028
🏁 Script executed:
#!/bin/bash
# View lines around 362-381 to see the second method and full context
sed -n '355,390p' src/lib/exporters/cwl-exporter.tsLength of output: 1309
Let me verify if there are other instances of this problematic pattern:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for all instances of docker run with backslash escaping patterns
rg -n '\\\$\(' src/lib/exporters/cwl-exporter.tsLength of output: 154
Fix Docker command escaping in generateDockerCommand method (line 370)
Verification confirms the backslash escaping breaks both shell and CWL expression evaluation:
- CWL expressions are evaluated by the runtime and may be used in arguments; implementations must rewrite runtime.outdir to a valid path inside the container
- Backslash-escaping
\$(pwd)prevents shell substitution; the literal string$(pwd)is passed, not the current working directory - Backslash-escaping
\$(runtime.outdir)interferes with CWL expression interpolation - CWL user guide examples show bare
$(runtime.outdir)without escaping in arguments
Change line 370 from:
let dockerCmd = `docker run --rm -v "\$(pwd)/inputs:/data/in:ro" -v "\$(runtime.outdir)/outputs:/data/out:rw" -e RUN_TOOL=${toolName} ${container}`;To:
let dockerCmd =
'docker run --rm' +
' -v "$(runtime.outdir)/inputs:/data/in:ro"' +
' -v "$(runtime.outdir)/outputs:/data/out:rw"' +
` -e RUN_TOOL=${toolName} ${container}`;This avoids shell-level substitution conflicts and allows CWL to properly interpolate runtime.outdir before command execution.
🤖 Prompt for AI Agents
In src/lib/exporters/cwl-exporter.ts around lines 285-304, the Docker command
built in generateDockerCommand incorrectly backslash-escapes $(pwd) and
$(runtime.outdir), preventing shell substitution and CWL expression
interpolation; update the command construction to stop escaping those
expressions and use CWL-friendly quoted expressions (e.g.,
"$(runtime.outdir)/...") for -v mounts, and build the string via concatenation
or template literals so the RUN_TOOL env var is injected normally (no
backslashes) — replace the existing escaped string at the dockerCmd assignment
(near line 370) with a concatenated/quoted form that uses
"$(runtime.outdir)/inputs:/data/in:ro" and
"$(runtime.outdir)/outputs:/data/out:rw" and `-e RUN_TOOL=${toolName}
${container}` so CWL can interpolate runtime.outdir at execution time.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/lib/exporters/cwl-exporter.ts (3)
182-224: [Duplicate] Enum and array type handling bugs remain unfixed from previous reviewThe two bugs flagged in the previous review are still present:
Optional enums (lines 198-202): The early return prevents optional enums from being wrapped with null. Optional enums should produce
['null', { type: 'enum', symbols: [...] }]but currently produce{ type: 'enum', symbols: [...] }.Optional arrays (lines 210-221): When both
optionalandarrayare true, line 212 createsbaseType = ['null', 'string'], then lines 216-221 return{ type: 'array', items: ['null', 'string'] }. This allows null array elements rather than an optional array parameter. The CWL spec requires['null', { type: 'array', items: 'string' }](null at outer type level).Fix approach:
- Remove the early return for enums (make it set
baseTypeinstead of returning)- Apply array wrapping first, then wrap the final result with null union if optional
Apply this fix:
private mapToolSpecTypeToCwl(paramDef: ToolSpecParameter): any { - let baseType: string | string[]; + let baseType: any; switch (paramDef.type) { case 'string': baseType = 'string'; break; case 'integer': baseType = 'int'; break; case 'float': baseType = 'float'; break; case 'boolean': baseType = 'boolean'; break; case 'enum': - return { + baseType = { type: 'enum', symbols: paramDef.values || [] }; - case 'asset': + break; + case 'asset': baseType = 'File'; // Default to File, could be Directory break; default: baseType = 'string'; } - // Handle optional (nullable) - if (paramDef.optional) { - baseType = ['null', baseType as string]; - } - // Handle array if (paramDef.array) { - return { + baseType = { type: 'array', items: baseType }; } + // Handle optional (nullable) - must be after array handling + if (paramDef.optional) { + baseType = ['null', baseType]; + } + return baseType; }
312-345: [Duplicate] Shell-based JSON generation remains unsafe for special charactersThe bug flagged in the previous review is still present. Line 323 escapes parameter names, but CWL substitutes
$(inputs.paramName)with actual parameter values before the shell command executes. If a value contains quotes, backslashes, or newlines, the resulting JSON will be invalid.Example with value
hello"world:printf '{\n "param": "hello"world"\n}' # Invalid JSON - breaks at the quoteAs discussed in the previous review thread, the portable solution is to use
InitialWorkDirRequirementto create both the directory and theinputs.jsonfile. CWL's expression language can safely construct the file content without shell escaping issues. Your currentgenerateInputsJsonListingonly creates an empty directory; it should also include a file entry forinputs/inputs.jsonwith the content built using CWL expressions.See the previous review thread for the detailed implementation suggested by the reviewer.
366-385: [Duplicate] Docker command escaping breaks CWL expression evaluationThe bug flagged in the previous review is still present at line 374. The backslash-escaped expressions
\$(pwd)and\$(runtime.outdir)prevent proper evaluation:
\$(pwd)prevents shell substitution - the literal string"$(pwd)/inputs"is passed to docker (not a valid path)\$(runtime.outdir)interferes with CWL expression interpolationCWL expressions should be bare (without backslashes) so the CWL runtime can interpolate them before the shell executes the command. The previous review confirmed this with CWL documentation.
Apply this fix to line 374:
- let dockerCmd = `docker run --rm -v "\$(pwd)/inputs:/data/in:ro" -v "\$(runtime.outdir)/outputs:/data/out:rw" -e RUN_TOOL=${toolName} ${container}`; + let dockerCmd = + 'docker run --rm' + + ' -v "$(runtime.outdir)/inputs:/data/in:ro"' + + ' -v "$(runtime.outdir)/outputs:/data/out:rw"' + + ` -e RUN_TOOL=${toolName} ${container}`;This uses
$(runtime.outdir)for both mounts (CWL provides the working directory) and removes the backslashes so CWL can properly interpolate the expression.
🧹 Nitpick comments (1)
src/lib/exporters/cwl-exporter.ts (1)
229-252: Consider removing unusedgenerateInputBindingmethodThis method is never called in the codebase. The comment at line 165 explicitly states "No inputBinding - tool-spec doesn't use command-line args", confirming that input bindings are not used for tool-spec tools. Since the method serves no purpose and adds maintenance burden, consider removing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/exporters/cwl-exporter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/exporters/cwl-exporter.ts (1)
src/lib/unified-metadata.ts (3)
UnifiedSoftwareMetadata(106-139)CwlExportConfig(99-104)ToolSpecParameter(39-48)
🔇 Additional comments (2)
src/lib/exporters/cwl-exporter.ts (2)
351-360: InitialWorkDirRequirement syntax fixed, but consider expanding to create JSON fileThe syntax issue from the previous review has been fixed - the code now correctly uses CWL spec properties (
class,basename,listing,writable) instead of the invalidentry/entrynamekeys.However, as discussed in the previous review thread, this method could be expanded to create the
inputs.jsonfile directly using CWL expressions, which would eliminate the shell-based JSON generation vulnerability at lines 312-345. The reviewer provided a detailed implementation in the previous thread showing how to add a file entry to the listing with JSON content built from CWL expressions like$(inputs.paramName).Based on learnings from the previous review.
430-442: LGTM - Download implementation is correctThe download method properly exports the CWL content, creates a blob with the correct MIME type, and triggers a browser download with a sanitized filename. The implementation follows standard patterns.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.