return ErrInvalidComposeFile when yaml parsing fails#1936
Merged
jordanstephens merged 8 commits intomainfrom Feb 18, 2026
Merged
return ErrInvalidComposeFile when yaml parsing fails#1936jordanstephens merged 8 commits intomainfrom
jordanstephens merged 8 commits intomainfrom
Conversation
since it may be debugging a load error
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCompose-load error handling was reorganized: compose command paths now call a unified handler; session loader invokes an interactive debug flow on compose-load errors; prompt text in the debugger was updated; a minor whitespace tweak in the loader. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI (compose command)
participant Loader as Compose Loader
participant Debugger as Defang Debugger
User->>CLI: run compose command
CLI->>Loader: LoadProject(...)
Loader-->>CLI: return error (compose parse/validation)
alt Interactive mode
CLI->>Loader: CreateProjectForDebug()
Loader-->>CLI: debug project
CLI->>Debugger: NewDebugger(debug project)
Debugger-->>CLI: debugger instance
CLI->>Debugger: DebugComposeLoadError(debug config)
Debugger->>User: interactive prompts and guidance
else Non-interactive
CLI-->>User: return original error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
lionello
reviewed
Feb 18, 2026
lionello
approved these changes
Feb 18, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
src/cmd/cli/command/compose.go (1)
84-90:⚠️ Potential issue | 🔴 CriticalCritical: Missing
returncauses nil pointer dereference.When
LoadProjectfails,handleInvalidComposeFileErris called but its result is not returned. Execution continues to line 90 whereproject.Nameis accessed, butprojectisnilwhenloadErr != nil, causing a panic.🐛 Proposed fix
project, loadErr := session.Loader.LoadProject(ctx) if loadErr != nil { - handleInvalidComposeFileErr(ctx, loadErr) + return handleInvalidComposeFileErr(ctx, loadErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/compose.go` around lines 84 - 90, When LoadProject returns an error, the code calls handleInvalidComposeFileErr but continues and dereferences project causing a panic; update the error branch after calling handleInvalidComposeFileErr to stop further execution (add a return) so you don't call canIUseProvider with a nil project. Specifically modify the block around session.Loader.LoadProject(ctx) / handleInvalidComposeFileErr to return immediately on loadErr, ensuring project.Name and project.Services are only accessed when project is non-nil before invoking canIUseProvider.
🧹 Nitpick comments (1)
src/pkg/cli/compose/loader.go (1)
127-129: Consider adding a comment explaining the string-based detection.The string prefix check is a pragmatic workaround since compose-go doesn't expose a specific error type for YAML parsing failures (as noted in the PR description). A brief comment would help future maintainers understand this is intentional.
💡 Suggested comment
+ // compose-go doesn't expose a specific error type for YAML parsing failures, + // so we detect them by checking for the "yaml: " prefix in the error message. if strings.HasPrefix(err.Error(), "yaml: ") { return nil, ErrInvalidComposeFile{fmt.Errorf("failed to parse compose file: %w", err)} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/loader.go` around lines 127 - 129, Add a brief explanatory comment above the strings.HasPrefix(err.Error(), "yaml: ") check in src/pkg/cli/compose/loader.go clarifying that this string-based detection is an intentional pragmatic workaround because compose-go does not expose a dedicated YAML parse error type; reference the rationale (compose-go missing typed error) and mention that the check distinguishes YAML parse errors from other errors in the function that returns ErrInvalidComposeFile so future maintainers understand it’s deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/session.go`:
- Around line 142-159: The function handleInvalidComposeFileErr is shadowing the
original compose load error by reassigning the parameter err when calling
CreateProjectForDebug(); change the code so you do not overwrite the incoming
err—store the project loader error in a new variable (e.g., projectErr or
loaderErr) or use a short-named project variable for the project while keeping
the original err intact, then pass the preserved original err into
debugger.DebugComposeLoadError; ensure NewDebugger and CreateProjectForDebug
return values are handled with distinct variable names so the original compose
error propagated into DebugComposeLoadError is not lost.
---
Outside diff comments:
In `@src/cmd/cli/command/compose.go`:
- Around line 84-90: When LoadProject returns an error, the code calls
handleInvalidComposeFileErr but continues and dereferences project causing a
panic; update the error branch after calling handleInvalidComposeFileErr to stop
further execution (add a return) so you don't call canIUseProvider with a nil
project. Specifically modify the block around session.Loader.LoadProject(ctx) /
handleInvalidComposeFileErr to return immediately on loadErr, ensuring
project.Name and project.Services are only accessed when project is non-nil
before invoking canIUseProvider.
---
Nitpick comments:
In `@src/pkg/cli/compose/loader.go`:
- Around line 127-129: Add a brief explanatory comment above the
strings.HasPrefix(err.Error(), "yaml: ") check in src/pkg/cli/compose/loader.go
clarifying that this string-based detection is an intentional pragmatic
workaround because compose-go does not expose a dedicated YAML parse error type;
reference the rationale (compose-go missing typed error) and mention that the
check distinguishes YAML parse errors from other errors in the function that
returns ErrInvalidComposeFile so future maintainers understand it’s deliberate.
jordanstephens
force-pushed
the
jordan/avoid-swallowing-invalid-compose-file-error
branch
from
fb02138 to
858e56b
Compare
lionello
reviewed
Feb 18, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cmd/cli/command/session.go (1)
147-149: Consider adding a comment explaining the string-matching rationale.The prefix matching for
"yaml: "and"validating "is fragile and depends on compose-go's error message format. A brief comment would help future maintainers understand why this approach is used and what to watch for if compose-go changes.📝 Suggested comment
+ // compose-go doesn't provide typed errors for YAML parsing failures, + // so we detect them by matching known error prefixes. If compose-go + // changes its error format, this detection may need updating. if !strings.HasPrefix(err.Error(), "yaml: ") && !strings.HasPrefix(err.Error(), "validating ") { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/session.go` around lines 147 - 149, Add a brief inline comment above the conditional that checks err.Error() prefixes ("yaml: " and "validating ") in session.go explaining that this string-matching is intentionally used to detect specific compose-go error messages (parsing/validation) and is fragile because it relies on compose-go's error text format; mention what kind of errors it intends to allow through (yaml parse/validation), that changes in compose-go error wording would require updating these prefixes, and suggest adding a TODO linking to compose-go issue or replacing with typed/errors when compose-go exposes structured error types; place this comment immediately above the if using the exact symbols strings.HasPrefix(err.Error(), "yaml: ") and strings.HasPrefix(err.Error(), "validating ") to make the rationale discoverable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cmd/cli/command/session.go`:
- Around line 142-168: Preserve the original parsing error in
handleInvalidComposeFileErr by avoiding reassignment of the incoming err;
capture the original error into a new variable (e.g., originalErr := err) at the
top, then call compose.NewLoader().CreateProjectForDebug() and
debug.NewDebugger(...) into distinct variables (e.g., project, loadErr and
debugger, dbgErr) instead of reusing err, and pass originalErr into
debugger.DebugComposeLoadError and return originalErr at the end so the original
YAML/validation error is not lost.
---
Nitpick comments:
In `@src/cmd/cli/command/session.go`:
- Around line 147-149: Add a brief inline comment above the conditional that
checks err.Error() prefixes ("yaml: " and "validating ") in session.go
explaining that this string-matching is intentionally used to detect specific
compose-go error messages (parsing/validation) and is fragile because it relies
on compose-go's error text format; mention what kind of errors it intends to
allow through (yaml parse/validation), that changes in compose-go error wording
would require updating these prefixes, and suggest adding a TODO linking to
compose-go issue or replacing with typed/errors when compose-go exposes
structured error types; place this comment immediately above the if using the
exact symbols strings.HasPrefix(err.Error(), "yaml: ") and
strings.HasPrefix(err.Error(), "validating ") to make the rationale
discoverable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Since #1797, we are now loading the project name before loading the stack (in order to fetch remote stacks) this means that our previously implemented handler for compose loader errors is never reached, so we need to invoke it earlier. I am matching the error string for the
yaml:prefix in order to trigger invoking the debugger, rather than always doing so, because there may be other errors that are not well handled by our debug agent. The compose-go(*ProjectOptions) LoadProject(context.Context)function does not return a specific error in this case, so I string matching is the best I can do.Linked Issues
Checklist
Summary by CodeRabbit
Bug Fixes
New Features
Updates