fix(sdl): preserve all args elements when importing an SDL into the builder - #3536
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesDeployment command inputs now document one-token-per-line values. SDL generation and import preserve complete command and argument arrays, command display handles args-only and whitespace-only values, and tests cover round trips plus a multi-argument fixture. SDL command and argument handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3536 +/- ##
==========================================
- Coverage 74.33% 73.40% -0.94%
==========================================
Files 1150 1060 -90
Lines 29882 27534 -2348
Branches 7471 7017 -454
==========================================
- Hits 22214 20212 -2002
+ Misses 6774 6458 -316
+ Partials 894 864 -30
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the args-array fix is correct and well-tested; the inline nit is cosmetic only and doesn't block this.
Extended reasoning...
Overview
This PR fixes a real reported bug: the SDL builder truncated multi-element args arrays to a single scalar on both import (sdlImport.ts) and generation (sdlGenerator.ts), and previously only emitted args when a command was also set. The fix makes args round-trip through the same newline-joined-token mechanism already used for command (parseSvcCommand/buildCommand), and decouples args emission from command emission. Placeholder text in two textareas was updated to document the one-token-per-line format. Changes are isolated to apps/deploy-web's SDL import/generate utilities and two form components.
Security risks
None. This is client-side form/YAML transformation logic with no auth, crypto, or trust-boundary changes. Worst case of a regression here is a malformed SDL being generated, which is already the class of bug being fixed.
Level of scrutiny
Moderate-low. The change affects what actually gets deployed on-chain (via the regenerated SDL), so correctness matters, but the diff is small, self-contained, and the fix mirrors an existing, already-proven pattern (how command is handled) rather than inventing new logic. Test coverage is strong: new unit tests in sdlGenerator.spec.ts cover multi-token args, args-without-command, and blank-arg cases; sdlImport.spec.ts adds round-trip tests including a semantic-equality fixture (tini-multi-args-sdl.yml) mirroring the originally reported SDL. I traced the core logic (buildCommand, the trimmedCommand/trimmedArg split in generateSdl, and parseSvcCommand in sdlImport) and it matches the stated intent.
Other factors
One inline nit was found: the legacy builder's CommandList.tsx preview still gates its command/args summary solely on command.command being non-empty, so an args-only configuration will show "None" in the preview even though it now deploys correctly per this PR's fix. I verified this against CommandList.tsx:43-50 and confirmed the gate doesn't check command.arg. It's purely cosmetic (deployment behavior is correct) and doesn't warrant blocking this fix. No outstanding reviewer comments need addressing, and the PR description accurately documents a known pre-existing limitation (literal-newline tokens in - | block scalars) as a follow-up, not a regression.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/components/sdl/CommandList.spec.tsx (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover whitespace-only values in the regression tests.
The implementation now treats whitespace-only command and argument values as absent, but this suite only tests empty strings. Add a case using spaces/newlines and assert that
"None"is rendered.🤖 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 `@apps/deploy-web/src/components/sdl/CommandList.spec.tsx` around lines 25 - 29, Extend the regression coverage in the CommandList rendering tests to pass whitespace-only command and argument values, including spaces or newlines, through setup. Assert that the rendered output contains “None,” while preserving the existing empty-string test.
🤖 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.
Nitpick comments:
In `@apps/deploy-web/src/components/sdl/CommandList.spec.tsx`:
- Around line 25-29: Extend the regression coverage in the CommandList rendering
tests to pass whitespace-only command and argument values, including spaces or
newlines, through setup. Assert that the rendered output contains “None,” while
preserving the existing empty-string test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10de1017-2584-440d-b4cb-f8bd2b0b603d
📒 Files selected for processing (2)
apps/deploy-web/src/components/sdl/CommandList.spec.tsxapps/deploy-web/src/components/sdl/CommandList.tsx
There was a problem hiding this comment.
LGTM — approving despite the inline nit, which is a narrow pre-existing edge case (unquoted falsy YAML scalars like 0/false in args) rather than a defect introduced by this PR's core logic.
Extended reasoning...
Overview
This PR fixes a real reported production bug: importing an SDL with multi-element args into the deploy-web builder silently truncated to just the first element, and generateSdl could only ever emit a single-element args array gated behind a non-empty command. The fix routes args through the existing parseSvcCommand/buildCommand newline-tokenization helpers (mirroring how command already round-trips), and decouples args emission from command emission in the generator. Changes are scoped to the SDL import/generate utilities and two UI placeholders/textarea copy, with substantial new round-trip test coverage including a semantic-equality fixture based on the actual reported SDL.
Security risks
None. This is client-side form/YAML transformation logic with no auth, injection, or data-exposure surface — it only affects how text the user already controls is round-tripped through the builder UI.
Level of scrutiny
Medium-low is appropriate: this is a well-tested, narrowly-scoped bug fix in non-critical (but user-visible/deploy-impacting) frontend logic, not a security- or auth-sensitive path, and not CODEOWNER-restricted. The PR includes strong regression coverage (multi-element round-trips, args-without-command, blank-arg omission, and a full semantic round-trip fixture mirroring the originally reported SDL).
Other factors
I previously flagged that CommandList.tsx's preview gate didn't account for args-only configs becoming deployable under this PR's new independent-emission logic; the author addressed this in 762cc7e with a matching trim()-based gate and added CommandList.spec.tsx coverage, so that's resolved. The one remaining inline finding (a nit) is a narrow, pre-existing edge case unrelated to this PR's main risk surface — unquoted YAML falsy scalars in an array being silently dropped by `filter(Boolean)" — and doesn't undermine the correctness of the actual fix being shipped.
There was a problem hiding this comment.
Beyond the two previously reported issues (both now fixed), I re-checked this round's changes: buildCommand's filter(Boolean) only ever runs on trimmed strings (never raw YAML scalars), so it can't reintroduce the falsy-token-drop bug fixed in 3794c74, and the CommandSchema fields are unconstrained strings with no length/pattern validation that would reject multi-line args. No new issues found.
Extended reasoning...
Re-verified the current HEAD (3794c74) against the diff: parseSvcCommand now stringifies tokens before filtering (fixing the 0/false drop bug), and CommandList's preview gate checks both command and arg (fixing the args-only 'None' display bug). Both fixes match what was requested in my prior inline comments and are covered by new regression tests. I additionally checked that buildCommand (used on the regenerate path) can't reintroduce either bug since it only operates on already-stringified, trimmed form values, and confirmed CommandSchema has no validation that would reject multi-line arg/command text. No new findings this round.
2 participants
Why
A user reported (via Slack) that importing an SDL with
command: [/tini, -s, --]andargs: [bash, -c, <setup script>]into the Console GUI silently deployedargs: [bash]only, so the container ran an idle bash and none of the setup script executed.Root cause: the builder form models args as a single scalar.
importSimpleSdlkept onlyargs[0]andgenerateSdlcould only emit a one-elementargslist, and only when a command was set. The configure flow always deploys the SDL regenerated from form values, so the truncation landed on-chain with no warning.What
sdlImport: import the fullargsarray through the existingparseSvcCommand(newline-joined tokens), matching howcommandalready round-trips.sdlGenerator: tokenize the arg field with the existingbuildCommandand emitargsindependently ofcommand, so an SDL with args but no command also round-trips.tini-multi-args-sdl.yml) mirroring the reported SDL including its folded>-script block.Known limitation (pre-existing, applies to
commandidentically): tokens containing literal newlines (- |block scalars) are split per line. Follow-up candidates: a losslessstring[]args model and a post-import round-trip diff warning for anything the builder cannot represent.Summary by CodeRabbit
command/argpresence: services omit missing fields and handlearg-only cases without losing additional tokens.command/argcontains meaningful text.