Enhance stack detection and add deploy --analyze #485
Conversation
Add comprehensive stack detection system with the following capabilities: Languages: - Ruby (Bundler, Rails) - Python (pip, pipenv, poetry, uv) - Node.js (npm, yarn) - Bun - Go - Rust (new) Python frameworks: - Django - Flask - FastAPI (with fastapi run command) - Gunicorn - Uvicorn Key changes: - Add Rust stack with Cargo.toml TOML parsing - Add Go version detection from go.mod - Add uv package manager support with uv.lock TOML parsing - Add DetectionEvent system to track what was detected during analysis - Add WebCommand() interface for inferring Procfile web commands - Add Init() lifecycle hook for post-detection initialization - Pass BuildOptions through to stack detection and initialization - Truncate Go image versions to major.minor only
Add new RPC endpoint to analyze apps without building, useful for dry-run deployments and debugging stack detection. New API types: - ServiceInfo: service name, command, and source (procfile/app_config/image) - DetectionEvent: kind, name, message for tracking what was detected - AnalysisResult: stack, services, working_dir, entrypoint, env_vars, events The AnalyzeApp endpoint: - Detects stack (go, node, python, ruby, rust, bun, dockerfile) - Parses Procfile and app.toml for service definitions - Reports services that use image default (Dockerfile CMD) - Collects detection events for debugging - Returns env var keys (not values) for security
Stackbuild can now infer a reasonable value for the web service
Used while testing, speeding it up made debugging faster
📝 WalkthroughWalkthroughAdds an AnalyzeApp RPC and CLI flow with new AnalysisResult, ServiceInfo, and DetectionEvent types; wires streaming tardata through RPC; introduces stack Init/WebCommand/Events lifecycle and image-config fetching; adds StartDirectory config, updates tar/progress APIs to use io.ReadCloser, CLI output, and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (miren deploy --analyze)
participant Tar as Tar Generator (pkg/tarx)
participant Client as Builder RPC Client
participant Server as Build Server (Builder.AnalyzeApp)
participant Stack as Stack Detector (pkg/stackbuild)
participant Config as Config Builder (buildVersionConfig)
CLI->>Tar: MakeTar(dir) -> io.ReadCloser (tardata)
Tar-->>Client: stream tardata
Client->>Server: AnalyzeApp(tardata)
Server->>Stack: DetectStack(codeDir, buildOpts)
Stack->>Stack: Init(buildOpts) and emit Events
Stack-->>Server: stack info, Entrypoint, WebCommand, Events
Server->>Config: parse Procfile/AppConfig, buildVersionConfig(...)
Config-->>Server: core_v1alpha.Config, services, env
Server-->>Client: AnalysisResult (stack, services, events, config)
Client-->>CLI: AnalysisResult
CLI->>CLI: Format & display analysis output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Files/areas needing extra attention:
Comment |
- Add deploy --analyze flag documentation to CLI reference - Add tip about using --analyze for preview in getting-started - Update supported languages list to include Rust and Bun - Add uv package manager support for Python - Add Python framework detection table (FastAPI, Django, Flask) - Add complete Rust language section with Cargo.toml examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
controllers/deployment/launcher.go (1)
257-271: HardenStartDirectoryhandling (clean + require absolute) to avoid surprising CWD.
Right now any string is accepted; a relative/unclean value will propagate into the sandbox container. Consider normalizing and forcing absolute.@@ - // Determine start directory, defaulting to /app - startDir := ver.Config.StartDirectory - if startDir == "" { - startDir = "/app" - } + // Determine start directory, defaulting to /app + startDir := strings.TrimSpace(ver.Config.StartDirectory) + if startDir == "" { + startDir = "/app" + } else { + // use path (not filepath) since this is a container/Linux path + startDir = path.Clean(startDir) + if !strings.HasPrefix(startDir, "/") { + startDir = "/" + startDir + } + }(Requires adding
pathimport.)pkg/stackbuild/stackbuild.go (3)
42-68: Public API expansion (Init/WebCommand/Events) is coherent; consider returning a copy fromEvents().
Events() []DetectionEventcurrently exposes the internal slice for mutation by callers.func (s *MetaStack) Events() []DetectionEvent { - return s.events + return slices.Clone(s.events) }(Needs
slicesimport.)
943-989: Node stack: WebCommand heuristics look reasonable; consider deduplicatinggetPackageScriptswith Bun.
NodeStack.getPackageScriptsandBunStack.getPackageScriptsare identical; could live onMetaStack(or a small helper) to keep behavior aligned.Also applies to: 1016-1041, 1051-1093
1387-1508: Rust stack: straightforward initial support; consider a clearer failure mode when binary name doesn’t exist.
cp target/release/<binaryName> /bin/appwill hard-fail if<binaryName>isn’t produced (workspace, lib-only crates, multiple bins). Not necessarily wrong, but it may be worth emitting a more actionable error (or probingCargo.tomlbins later).docs/docs/languages.md (2)
250-259: Add language identifier for consistency with other code blocks.The Procfile example code block lacks a language identifier, which triggers a markdown lint warning. Based on learnings, Procfile is not YAML and should not use the
yamlidentifier. Consider either omitting the language (which is fine) or using a generic identifier liketextorbashfor syntax highlighting if the linter requires one.-``` +```text # Run the compiled binary web: /bin/app
227-273: Comprehensive Rust documentation following established patterns.The new Rust section is well-documented with:
- Clear detection criteria (Cargo.toml)
- Build process explanation
- Binary name detection logic
- Example Procfile and Cargo.toml
Minor observation: Lines 254-255 show duplicate
web: /bin/appentries with different comments. The second one ("With environment-based port") doesn't demonstrate anything different. Consider either removing the duplicate or showing a command that actually uses the environment variable.cli/commands/deploy.go (1)
802-815: Consider extracting shared pattern validation logic.The pattern loading and validation logic (lines 802-815) is duplicated from the main
Deployfunction (lines 274-290). Consider extracting this to a helper function to reduce duplication and ensure consistent behavior.func loadIncludePatterns(dir string) ([]string, error) { ac, err := appconfig.LoadAppConfigUnder(dir) if err != nil { return nil, fmt.Errorf("failed to load app config: %w", err) } if ac == nil || ac.Include == nil { return nil, nil } for _, pattern := range ac.Include { if err := tarx.ValidatePattern(pattern); err != nil { return nil, fmt.Errorf("invalid include pattern %q: %w", pattern, err) } } return ac.Include, nil }servers/build/buildkit.go (1)
558-578: Silent failure on JSON unmarshal error shadows the actual error variable.At line 576, the
errfromjson.Unmarshalshadows the outererrvariable used for the final return. While the warning is logged, this could cause confusion. Consider using a different variable name for clarity.- if err := json.Unmarshal([]byte(configJSON), &imgConfig); err == nil { + if unmarshalErr := json.Unmarshal([]byte(configJSON), &imgConfig); unmarshalErr == nil { res.WorkingDir = imgConfig.Config.WorkingDir if res.Entrypoint == "" { res.Entrypoint = buildImageCommand(imgConfig.Config.Entrypoint, nil) } if res.Command == "" { res.Command = buildImageCommand(imgConfig.Config.Cmd, nil) } } else { - b.Log.Warn("failed to parse image config", "error", err) + b.Log.Warn("failed to parse image config", "error", unmarshalErr) }servers/build/buildkit_test.go (2)
57-72: Cleanup functions capture a potentially cancelled context.The cleanup functions (lines 58-62, 122-134, 214-226) capture the
ctxfromsetupTestInfra, which may be cancelled by the time cleanup runs. This could cause cleanup failures to be silently ignored. Consider usingcontext.Background()for cleanup operations.func setupTestInfra(t *testing.T, ctx context.Context) *testInfra { t.Helper() + + // Use background context for cleanup to ensure it runs even if test context is cancelled + cleanupCtx := context.Background() cl, err := client.NewClientWithOpts(client.FromEnv) require.NoError(t, err) // ... // Add network cleanup infra.cleanups = append(infra.cleanups, func() { - if err := cl.NetworkRemove(ctx, networkResp.ID); err != nil { + if err := cl.NetworkRemove(cleanupCtx, networkResp.ID); err != nil { t.Logf("failed to remove network: %v", err) } })Apply similar changes to
setupBuildkitContainerandsetupLocalRegistry.
136-150: Goroutine for container logs may leak.The goroutine capturing container logs (lines 138-150) has no mechanism to stop when the test completes. While it will eventually terminate when the container is killed, it could cause issues if the container removal fails. Consider adding a done channel or using
t.Cleanupwith cancellation.servers/build/build.go (1)
863-894: Shell quoting logic may produce incorrect results for certain inputs.The
buildImageCommandfunction uses%qformatting for parts containing special characters, which adds Go-style quotes (with backslash escapes). This may not produce shell-compatible output in all cases. For example, a part containing a single quote would be quoted as"it\'s"which is valid in Go but may cause issues in some shells.Consider using
shellescapeor a more robust quoting mechanism if this command string is passed directly to a shell. If it's only used for display purposes, the current implementation is acceptable.api/build/build_v1alpha/rpc.gen.go (1)
963-969: Potential nil pointer dereference in Result() accessor.The
Result()method dereferences*v.data.Resultwithout checking if the dereferenced value is nil. Ifv.data.Resultpoints to a nil*AnalysisResult, this will return nil safely, but the double-pointer pattern is confusing.This follows the existing pattern for
AccessInfo()(line 929-931), so it's consistent with the codebase. No change needed, but worth noting for future generator improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
api/build/build_v1alpha/rpc.gen.go(6 hunks)api/build/rpc.yml(2 hunks)api/core/core_v1alpha/schema.gen.go(6 hunks)api/core/schema.yml(1 hunks)cli/commands/deploy.go(3 hunks)controllers/deployment/launcher.go(1 hunks)docker/Dockerfile(1 hunks)docs/docs/cli-reference.md(1 hunks)docs/docs/getting-started.md(1 hunks)docs/docs/languages.md(3 hunks)pkg/buildkit/buildkit.go(1 hunks)pkg/deploygating/gate.go(1 hunks)pkg/imagerefs/imagerefs.go(3 hunks)pkg/stackbuild/stackbuild.go(22 hunks)pkg/stackbuild/stackbuild_test.go(1 hunks)servers/build/build.go(5 hunks)servers/build/build_test.go(2 hunks)servers/build/buildkit.go(5 hunks)servers/build/buildkit_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
controllers/deployment/launcher.gopkg/imagerefs/imagerefs.gopkg/buildkit/buildkit.gopkg/stackbuild/stackbuild_test.goservers/build/build.gocli/commands/deploy.goservers/build/build_test.goservers/build/buildkit.goservers/build/buildkit_test.goapi/build/build_v1alpha/rpc.gen.gopkg/deploygating/gate.goapi/core/core_v1alpha/schema.gen.gopkg/stackbuild/stackbuild.go
**/*.gen.go
📄 CodeRabbit inference engine (CLAUDE.md)
Code generation outputs for entity schemas and RPC interfaces should have
.gen.gosuffix
Files:
api/build/build_v1alpha/rpc.gen.goapi/core/core_v1alpha/schema.gen.go
🧠 Learnings (5)
📚 Learning: 2025-12-08T22:25:56.550Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 460
File: docs/docs/languages.md:46-46
Timestamp: 2025-12-08T22:25:56.550Z
Learning: Procfile is not YAML and uses a distinct format; in docs, when including Procfile code blocks, do not label them with the language identifier 'yaml'. Use the correct identifier (e.g., 'Procfile' if supported) or omit language labeling to avoid implying YAML syntax. Apply this guidance to all markdown documentation that contains Procfile examples.
Applied to files:
docs/docs/languages.mddocs/docs/getting-started.mddocs/docs/cli-reference.md
📚 Learning: 2025-08-12T20:23:01.642Z
Learnt from: phinze
Repo: mirendev/runtime PR: 173
File: cli/commands/download_release.go:30-31
Timestamp: 2025-08-12T20:23:01.642Z
Learning: In the mirendev/runtime repository, there are two types of artifacts published: miren-linux-<arch>.zip (containing just the Go binary) and miren-base-linux-<arch>.tar.gz (containing additional utilities and referred to as a "release"). The download_release.go command specifically targets the latter tarball format for full release downloads.
Applied to files:
docker/Dockerfile
📚 Learning: 2025-07-09T22:37:37.646Z
Learnt from: phinze
Repo: mirendev/runtime PR: 124
File: controllers/app/deployment.go:59-61
Timestamp: 2025-07-09T22:37:37.646Z
Learning: In the core_v1alpha package, the Decode methods on generated structs like AppVersion and Concurrency have the signature `func (o *StructName) Decode(e entity.AttrGetter)` and do not return errors. Error handling should not be added for these Decode method calls.
Applied to files:
api/core/core_v1alpha/schema.gen.go
📚 Learning: 2025-10-21T03:19:32.427Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 232
File: pkg/entity/entity.go:102-105
Timestamp: 2025-10-21T03:19:32.427Z
Learning: In pkg/entity/entity.go, the shallow cloning behavior in Entity.Attrs() and EntityComponent.Attrs() (using slices.Clone) is acceptable and intentional, even though it means mutable values like KindBytes, KindArray, and KindComponent are aliased between the original and returned attributes.
Applied to files:
api/core/core_v1alpha/schema.gen.go
📚 Learning: 2025-12-05T18:29:59.865Z
Learnt from: CR
Repo: mirendev/runtime PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T18:29:59.865Z
Learning: Entity schemas should be defined in `api/*/schema.yml` files and have corresponding generated Go structs
Applied to files:
api/core/core_v1alpha/schema.gen.go
🧬 Code graph analysis (7)
controllers/deployment/launcher.go (2)
api/core/core_v1alpha/schema.gen.go (2)
Config(205-212)Env(571-576)api/compute/compute_v1alpha/schema.gen.go (1)
SandboxSpecContainer(160-171)
servers/build/build.go (3)
pkg/buildkit/buildkit.go (1)
BuildResult(290-292)appconfig/appconfig.go (1)
AppConfig(55-63)api/core/core_v1alpha/schema.gen.go (5)
Config(205-212)Variable(705-710)Services(358-367)Env(571-576)Commands(308-311)
cli/commands/deploy.go (4)
api/build/build_v1alpha/rpc.gen.go (2)
NewBuilderClient(901-903)BuilderClient(897-899)appconfig/appconfig.go (1)
LoadAppConfigUnder(100-122)pkg/tarx/tarx.go (1)
ValidatePattern(17-24)pkg/rpc/stream/stream.go (1)
ServeReader(133-135)
servers/build/build_test.go (1)
servers/build/build.go (1)
ConfigInputs(237-249)
servers/build/buildkit.go (1)
pkg/stackbuild/stackbuild.go (2)
BuildOptions(23-40)DetectStack(71-91)
servers/build/buildkit_test.go (2)
pkg/imagerefs/imagerefs.go (1)
BuildKit(16-16)servers/build/buildkit.go (2)
Buildkit(24-33)BuildStack(269-277)
api/build/build_v1alpha/rpc.gen.go (4)
pkg/rpc/oid.go (2)
Capability(7-16)OID(5-5)pkg/rpc/call.go (1)
Call(13-18)pkg/rpc/stream/stream.gen.go (3)
RecvStreamClient(297-299)RecvStream(266-268)AdaptRecvStream(282-295)pkg/rpc/client.go (2)
Client(29-35)InlineCapability(587-590)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/languages.md
250-250: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test-e2e
- GitHub Check: test
🔇 Additional comments (34)
docker/Dockerfile (1)
14-27: No action required—BuildKit is already core project infrastructure.The caching strategy is sound and follows best practices (early COPY of dependency files, explicit download step with cache mount, and build caching). However, the concern about BuildKit support is unfounded. The project already mandates BuildKit as part of its development and build infrastructure:
hack/common-setup.shprovidesstart_buildkitd()function- Test scripts explicitly start buildkitd daemon
CLAUDE.mddocuments that the dev environment uses buildkit internally- ISO environment includes buildkit as a standard service
- The project uses standalone buildkitd daemon, not just optional BuildKit support
The Dockerfile changes are consistent with existing project patterns and require no additional BuildKit configuration or fallback mechanisms. Builds will work as expected in the project's standard CI/CD and development environments.
Likely an incorrect or invalid review comment.
pkg/imagerefs/imagerefs.go (3)
5-5: LGTM!The
stringsimport is correctly added to support the version truncation logic inGetGolangImage.
50-59: LGTM! Version truncation logic is correct.The truncation logic correctly handles various version formats:
- "1.21.5" → "1.21" ✓
- "1.21" → "1.21" ✓
- "1" → "1" (no truncation) ✓
The documentation clearly explains the behavior, which aligns with Go's official Docker image tagging convention (major.minor).
71-74: LGTM! Follows established pattern.The
GetRustImagefunction correctly follows the same pattern as other language image functions (Python, Ruby, Node, Bun). The absence of version truncation is appropriate, as only Go requires major.minor formatting for its Docker image tags.pkg/deploygating/gate.go (1)
34-36: Deploy gating relaxation looks good.
This keeps the only hard failure as “invalid directory”, and avoids blocking deploys based on missing web service definition.api/core/schema.yml (1)
157-163: Schema addition forstart_directoryis consistent and well-placed.
Field name and docs align withcore_v1alpha.Config.StartDirectoryand deployment usage.api/core/core_v1alpha/schema.gen.go (1)
196-212: Generated schema changes look consistent; please ensure all codegen outputs are regenerated together.
Given the embedded encoded schema payload changed too, I’d just double-check the repo’s codegen step was run cleanly and no other.gen.goartifacts are now stale.Also applies to: 235-237, 260-262, 282-284, 298-301, 1341-1341
pkg/stackbuild/stackbuild.go (4)
70-91:DetectStack(..., opts)callingInit(opts)is good; verify no callers depend on old signature.
This is a breaking API change—worth ensuring all internal call sites compile and any downstream modules are updated.
93-120: Event collection onMetaStacklooks fine; consider documenting/guarding “Detect vs Init” usage.
Right now stacks are free to callEvent()in bothDetect()andInit()—that’s good, just ensure tooling/UI expects duplicates if both stages report the same thing.
1108-1144: Bun stack changes look consistent with Node flow.
Detection + WebCommand fallback to entry point is a nice UX improvement.Also applies to: 1193-1225
1241-1275: Go version selection: please verify patch-level tags are valid forimagerefs.GetGolangImage.
parseGoModVersion()can return1.23.4, andGenerateLLBwill use it whenopts.Versionis empty. Ifimagerefs.GetGolangImageonly supportsmajor.minor, this will fail at runtime.Also applies to: 1319-1385
pkg/stackbuild/stackbuild_test.go (1)
541-549: Updated Go version detection test expectations make sense.
This aligns the test with returning the fullgodirective value (including patch).docs/docs/cli-reference.md (1)
13-24: Docs update fordeploy --analyzeis clear and scannable.
Flag table is a good addition.docs/docs/getting-started.md (1)
76-94: Getting-started “preview before deploying” tip is helpful and matches the new flow.
Language detection list updates (uv/bun/rust) read well.servers/build/build_test.go (3)
925-994: Well-structured test coverage forbuildImageCommand.The table-driven tests cover important edge cases including nil/empty inputs, shell form commands, and arguments with spaces that need quoting. Good coverage of the quoting logic for
-carguments.
996-1246: Comprehensive tests forbuildVersionConfigwith good precedence coverage.The tests thoroughly verify the config building logic including:
- Image entrypoint creating web service when no services configured
- Procfile web taking precedence over image entrypoint
- App config command taking precedence over image entrypoint
- Preservation of manual env vars from existing config
- Combined entrypoint and command scenarios
This provides confidence in the config merging logic.
1248-1329: Test is appropriately scoped as a JSON format specification test—no extraction function exists to call.The test correctly validates the JSON structure format expected by
BuildImage, where working directory extraction is simply field access (stack.Image().Config.WorkingDirat line 998 of build.go). Since no dedicatedextractWorkingDirFromImageConfigfunction exists in the codebase, the current inline approach serves as a useful specification test documenting the expected format.pkg/buildkit/buildkit.go (1)
336-352: Clean refactoring to use consolidatedBuildOptionsstruct.The change properly consolidates
OnBuild,Version, andAlpineImageinto a singleBuildOptionsstruct, which is passed to bothDetectStackandGenerateLLB. This improves API consistency and makes it easier to add new options in the future.docs/docs/languages.md (1)
61-86: Good documentation of Python framework detection and uv support.The Python section now clearly documents:
- Detection of
uv.lockfor the uv package manager- Priority order of dependency management systems
- Framework-specific start command detection
This aligns well with the PR's goal of enhancing stack detection.
cli/commands/deploy.go (4)
34-34: Good addition of--analyzeflag for dry-run analysis.The flag is well-positioned with other build-related flags and has a clear description.
93-102: Clean early return for analyze mode.The analyze path correctly establishes an RPC client connection before calling
analyzeApp. This avoids running unnecessary deployment preparation logic when only analysis is requested.
736-768: Style definitions are appropriately scoped as module-level vars.The lipgloss styles for analyze output are cleanly defined with descriptive names and consistent color choices. Using module-level vars avoids recreating these on each call.
863-886: Clear service output with graceful handling of empty commands.The services display logic properly:
- Shows source information when available
- Handles empty commands by displaying "image default" with appropriate styling
- Uses consistent formatting
This provides good UX for understanding detected services.
api/build/rpc.yml (4)
44-58: Well-designedServiceInfotype with clear documentation.The type captures essential service information with appropriate documentation. The
sourcefield providing provenance (procfile, app_config, image) is valuable for debugging and user understanding.
60-74:DetectionEventtype supports the detection event system well.The
kindfield enumeration (file, package, framework, config, script, dir) covers the detection categories comprehensively. The design allows for flexible event reporting.
76-110:AnalysisResultis comprehensive with good security consideration.The type captures all relevant analysis output. Notable design decision:
env_varscontains only keys (not values) for security, which is explicitly documented. This prevents accidental exposure of secrets during analysis.
140-147: Clean API addition foranalyzeAppmethod.The method follows the established pattern of accepting tar data as a stream and returns a structured result. The documentation clearly states the purpose: analyzing without building.
servers/build/buildkit.go (1)
371-395: LGTM on BuildOptions integration.The refactoring to pass
BuildOptionstoDetectStackandGenerateLLBaligns well with the new stack detection API. The population ofres.Commandfromstack.WebCommand()andres.WorkingDirfrom stack image config is correct.servers/build/buildkit_test.go (2)
244-296: Good integration test coverage for WorkingDir extraction.The test properly validates that
WorkingDiris extracted from Dockerfile builds using the local registry and BuildKit container setup. The test infrastructure with network isolation is well-designed.
397-448: Good coverage of ENTRYPOINT and CMD extraction.This test validates both
EntrypointandCommandfields are correctly extracted from a Dockerfile with both directives. The assertions are appropriate.servers/build/build.go (2)
236-338: Well-structured refactoring with ConfigInputs and buildVersionConfig.The extraction of version config building into a pure function
buildVersionConfigis a good refactoring that improves testability and code reuse betweenBuildFromTarandAnalyzeApp. The logic for synthetic web service creation (lines 275-287) correctly handles the case when no explicit web command exists.
915-934: AnalyzeApp follows established patterns.The method correctly handles tar extraction and cleanup with
defer os.RemoveAll(path). The error handling is consistent withBuildFromTar.api/build/build_v1alpha/rpc.gen.go (2)
193-475: Generated types follow established patterns.The new
ServiceInfo,DetectionEvent, andAnalysisResulttypes follow the same pointer-based optional field pattern as existing types in this file. The CBOR/JSON marshaling implementations are consistent.
884-891: This concern is based on a misunderstanding of the RPC system design. TheIndexfield on methods is metadata that is not used for method dispatch or routing. Methods are dispatched by theirNamefield through a string-keyed map in theInterfacetype, not byIndex.In the RPC schema definition (YAML), method definitions do not specify indices—only fields do. The code generator assigns
Index: 0to all methods as a default placeholder value, which is the expected and correct behavior. This is consistent across all generated RPC files and causes no issues.Likely an incorrect or invalid review comment.
teresamychu
left a comment
teresamychu
left a comment
There was a problem hiding this comment.
LGTM! Just some minor suggestions.
| } | ||
|
|
||
| // Services | ||
| if analysisResult.HasServices() && analysisResult.Services() != nil { |
There was a problem hiding this comment.
💭 Maybe add a note when there are no services detected? So the user knows that the app right now wont actually run anything?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/commands/deploy.go (1)
819-829: LGTM!The tar reader is now properly closed with
defer r.Close()at line 824, fixing the potential resource leak identified in the previous review.servers/build/buildkit.go (1)
291-351: HTTP client improvements address previous concerns.The implementation now includes:
- Context propagation via
http.NewRequestWithContext- 10-second timeout on the HTTP client
- HTTPS-first with explicit HTTP fallback only when
insecureis trueThis addresses the previous review concerns about indefinite hangs and security.
servers/build/build.go (1)
981-982: File handle properly closed.The file handle returned by
tr.Open("Dockerfile.miren")is now correctly closed withf.Close(), fixing the resource leak identified in the previous review.
🧹 Nitpick comments (4)
cli/commands/deploy.go (1)
868-890: Consider adding a message when no services are detected.Based on a past review comment, it may be helpful to inform the user when the analysis detects no services, so they understand the app won't run anything yet.
Apply this diff to add a note when services are empty:
// Services if analysisResult.HasServices() && analysisResult.Services() != nil { services := *analysisResult.Services() if len(services) > 0 { ctx.Printf("\n%s\n", analyzeTitleStyle.Render("Services")) for _, svc := range services { sourceInfo := "" if svc.Source() != "" { sourceInfo = lipgloss.NewStyle().Faint(true).Render(fmt.Sprintf(" (%s)", svc.Source())) } command := svc.Command() if command == "" { // Service uses Dockerfile CMD (image default) command = lipgloss.NewStyle().Faint(true).Italic(true).Render("image default") } ctx.Printf(" %s: %s%s\n", analyzeValueStyle.Render(svc.Name()), command, sourceInfo) } + } else { + ctx.Printf("\n%s\n", analyzeTitleStyle.Render("Services")) + ctx.Printf(" %s\n", lipgloss.NewStyle().Faint(true).Render("No services detected. Add a Procfile or app.toml to define services.")) } }pkg/stackbuild/stackbuild_test.go (1)
848-855: Test passes by coincidence, not by testing the intended behavior.This test case is named "npm with main entry point" and includes
"main": "index.js"in package.json, but the actual detection relies on theindex.jsfile existing (line 852), not on parsing themainfield from package.json. TheNodeStack.Init()method iterates over hardcoded filenames rather than reading themainfield.If the intent is to support arbitrary entry points via the
mainfield, consider parsing it from package.json. Otherwise, rename this test to clarify it tests file-based detection.pkg/stackbuild/stackbuild.go (2)
867-876: Redundant fallback in gunicorn detection.Lines 872-875 check if
app.pyexists and return a gunicorn command, but line 875 returns the same command regardless. Theif s.hasFile("app.py")check has no effect.if s.hasGunicorn && !s.hasFastAPI { if s.wsgiModule != "" { return "gunicorn " + s.wsgiModule + " -b 0.0.0.0:$PORT" } - // Fallback: check common entry points - if s.hasFile("app.py") { - return "gunicorn app:app -b 0.0.0.0:$PORT" - } - return "gunicorn app:app -b 0.0.0.0:$PORT" + // Fallback to common entry point + return "gunicorn app:app -b 0.0.0.0:$PORT" }
1053-1066: Consider extracting duplicategetPackageScripts()toMetaStack.
NodeStack.getPackageScripts()andBunStack.getPackageScripts()are identical implementations. Consider moving this toMetaStackto reduce duplication.Also applies to: 1195-1208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
pkg/stackbuild/testdata/python-uv/uv.lockis excluded by!**/*.lockpkg/stackbuild/testdata/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
cli/commands/deploy.go(4 hunks)pkg/deploygating/gate.go(2 hunks)pkg/deploygating/gate_test.go(3 hunks)pkg/progress/upload/upload.go(3 hunks)pkg/stackbuild/stackbuild.go(23 hunks)pkg/stackbuild/stackbuild_test.go(3 hunks)pkg/stackbuild/testdata/python-uv/pyproject.toml(1 hunks)pkg/stackbuild/testdata/rust/Cargo.toml(1 hunks)pkg/stackbuild/testdata/rust/main.rs(1 hunks)pkg/tarx/tarx.go(1 hunks)servers/build/build.go(5 hunks)servers/build/build_test.go(2 hunks)servers/build/buildkit.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/stackbuild/testdata/rust/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- servers/build/build_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
pkg/deploygating/gate.gopkg/deploygating/gate_test.gopkg/stackbuild/stackbuild_test.gopkg/progress/upload/upload.gocli/commands/deploy.goservers/build/buildkit.goservers/build/build.gopkg/tarx/tarx.gopkg/stackbuild/stackbuild.go
🧠 Learnings (7)
📚 Learning: 2025-11-15T00:16:45.268Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 373
File: components/activator/activator.go:1513-1513
Timestamp: 2025-11-15T00:16:45.268Z
Learning: In components/activator/activator.go and other files in the mirendev/runtime repository, when checking for not-found errors from Patch operations, use `errors.Is(err, cond.ErrNotFound{})`. The cond package passes NotFound errors through as value types, consistent with how `cond.ErrConflict{}` is handled for optimistic concurrency control operations.
Applied to files:
pkg/deploygating/gate.go
📚 Learning: 2025-06-18T22:34:20.129Z
Learnt from: phinze
Repo: mirendev/runtime PR: 101
File: cli/commands/dev.go:0-0
Timestamp: 2025-06-18T22:34:20.129Z
Learning: Path traversal safety concerns don't apply when working with HOME directory-based paths in the dev.go command, as the paths are safely constructed from trusted HOME environment variables.
Applied to files:
pkg/deploygating/gate_test.go
📚 Learning: 2025-02-06T20:28:03.771Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 25
File: pkg/progress/progresswriter/printer.go:43-58
Timestamp: 2025-02-06T20:28:03.771Z
Learning: The package in pkg/progress/progresswriter is vendored from an external source and should not be modified directly.
Applied to files:
pkg/progress/upload/upload.go
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/deploy.go
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
servers/build/buildkit.go
📚 Learning: 2025-06-03T00:10:14.949Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 84
File: controllers/sandbox/sandbox.go:229-236
Timestamp: 2025-06-03T00:10:14.949Z
Learning: In controllers/sandbox/sandbox.go, socket file cleanup before creating new runsc monitor sockets is implemented as a "best case check" where errors from os.Remove() can be ignored, as this is a defensive cleanup operation that shouldn't block initialization if it fails.
Applied to files:
servers/build/build.go
📚 Learning: 2025-05-29T16:33:23.497Z
Learnt from: phinze
Repo: mirendev/runtime PR: 70
File: Makefile:15-16
Timestamp: 2025-05-29T16:33:23.497Z
Learning: The dagger module in the mirendev/runtime repository has custom test functionality that supports additional flags like `--tags` and `--tests` for filtering tests, as implemented in dagger/main.go.
Applied to files:
pkg/stackbuild/stackbuild.go
🧬 Code graph analysis (3)
pkg/deploygating/gate.go (2)
appconfig/appconfig.go (2)
LoadAppConfigUnder(100-122)AppConfigPath(65-65)pkg/tasks/procfile.go (1)
ParseFile(24-71)
servers/build/buildkit.go (1)
pkg/stackbuild/stackbuild.go (2)
BuildOptions(23-40)DetectStack(71-91)
pkg/stackbuild/stackbuild.go (1)
pkg/imagerefs/imagerefs.go (1)
GetRustImage(72-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test-e2e
🔇 Additional comments (28)
pkg/tarx/tarx.go (1)
26-26: LGTM!The return type change from
io.Readertoio.ReadCloseris correct and aligns with the underlyingos.Pipe()which returns*os.File(anio.ReadCloser). This enables callers to properly close the pipe reader when done.pkg/deploygating/gate.go (1)
38-56: LGTM!The updated validation logic correctly:
- Validates app config syntax and provides actionable remedy messages
- Checks Procfile syntax while treating missing files as acceptable
- Removes the hard requirement for a web service, allowing stackbuild to infer defaults
The error handling using
errors.Is(err, os.ErrNotExist)is idiomatic Go.pkg/deploygating/gate_test.go (1)
111-113: LGTM!The test updates correctly validate the new error messages and remedy guidance from
gate.go, including:
- "invalid app configuration" with "Fix the configuration error" remedy
- "failed to parse Procfile" with "Fix the syntax error in Procfile" remedy
Also applies to: 126-129, 197-197
cli/commands/deploy.go (1)
300-301: Resource management improvement.The addition of
defer r.Close()properly closes the tar reader, addressing the resource leak concern from the previous review.servers/build/buildkit.go (1)
398-422: BuildOptions construction looks correct.The
BuildOptionsstruct is properly populated with fields fromBuildStackand passed toDetectStackandGenerateLLB. The extractedEntrypoint,Command, andWorkingDirfrom the stack are correctly assigned to theBuildResult.pkg/progress/upload/upload.go (1)
12-12: LGTM!The changes correctly:
- Update the field and constructor to use
io.ReadCloser- Add a
Close()method that properly delegates to the underlying reader- Align with the
tarx.MakeTarreturn type changeAlso applies to: 28-28, 37-39
servers/build/build.go (4)
236-342: Well-structured config building logic.The introduction of
ConfigInputsand the pure functionbuildVersionConfigis excellent:
- Consolidates config computation in a testable function
- Clear separation of concerns between inputs and logic
- Proper handling of synthetic web service injection
- Correct env var merging with source tracking
867-898: Command building handles various entrypoint/cmd formats.The
buildImageCommandfunction appropriately:
- Combines entrypoint and cmd arrays
- Returns single parts directly
- Quotes parts with special characters for shell safety
1066-1066: Source tracking consistency verified.The condition at line 1066 checks
source == "stack", anddetermineServiceSourcecorrectly returns"stack"at line 1116 when the command matchesbuildResult.Commandfor the web service. This addresses the previous review concern about the mismatch between "infer" and "stack".Also applies to: 1094-1121
919-1092: AnalyzeApp implementation is comprehensive.The new
AnalyzeAppmethod properly:
- Extracts and analyzes the tarball without building
- Detects stack and collects detection events
- Reuses
buildVersionConfigfor consistency with BuildFromTar- Tracks service sources (app_config, procfile, stack, image)
- Returns structured analysis results
pkg/stackbuild/testdata/rust/main.rs (1)
1-3: LGTM!Standard Rust test fixture for the new Rust stack detection tests.
pkg/stackbuild/testdata/python-uv/pyproject.toml (1)
1-8: LGTM!Valid PEP 621 pyproject.toml test fixture for uv package manager detection.
pkg/stackbuild/stackbuild_test.go (7)
574-610: LGTM!Well-structured test for the new Rust stack. Correctly sets up src directory, copies test files, and verifies the built binary exists at
bin/app.
612-642: LGTM!Good test coverage for the new uv package manager detection. Correctly verifies
Detect()returns true before generating LLB.
644-729: LGTM!Comprehensive table-driven tests covering Rails, Puma (with/without config), Unicorn, Rack apps, and the "no web server" edge case.
731-807: LGTM!Good coverage of Python web command detection including FastAPI, Gunicorn with Django WSGI, Uvicorn, and Flask scenarios.
809-894: LGTM!Thorough Node.js web command tests covering npm/yarn package managers, various script names (start, serve, server), main entry points, and TypeScript handling.
896-956: LGTM!Good Bun web command test coverage including scripts and direct entry point execution.
958-986: LGTM!Simple but effective tests confirming Go and Rust stacks return
/bin/appas their web command.pkg/stackbuild/stackbuild.go (9)
42-68: LGTM!Clean interface extension with
DetectionEventtype and newStackinterface methods (Init,WebCommand,Events). The design follows good Go patterns.
75-88: Detection order is correct for Node vs Bun.Good that
BunStackappears afterNodeStackin the list. SinceNodeStack.Detect()requiresyarn.lock,package-lock.json, or a Procfile referencing node/npm/yarn (not justpackage.json), projects withbun.lockwon't incorrectly match as Node.
461-477: LGTM!
WebCommand()correctly prioritizes Rails over other servers and handles Puma config detection. The switch-case fallthrough logic is clear.
587-625: Heuristic detection via substring matching is acceptable for this use case.The substring matching approach may produce occasional false positives (e.g., package names in comments), but the TOML-parsed
uv.lockcheck provides accurate detection when available. For heuristic stack detection, this tradeoff is reasonable.
1068-1095: LGTM!
WebCommand()correctly handles the difference between yarn (no "run" needed) and npm (requires "run" for scripts). The fallback to TypeScript vianpx tsxis a good touch.
1302-1319: LGTM!
parseGoModVersion()now correctly returns the full version string including patch level, which is more precise for determining the Go version to use.
1454-1506: Good Rust stack implementation overall.The structure follows the established patterns from other stacks. Good use of cache mounts for cargo registry and target directory, and properly excludes the target directory from the build context.
715-737: LGTM!The uv package manager integration correctly handles pyproject.toml and uv.lock, installs uv, and runs
uv sync --no-devwith appropriate cache mounts.
829-863: Good approach to uv.lock parsing with package name normalization.The
parseUvLock()function correctly handles Python package naming conventions by storing both hyphenated and underscored variants, ensuring packages likemy-packageandmy_packageare both matched.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go.mod(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-e2e
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: build-binaries (linux, amd64, depot-ubuntu-latest)
- GitHub Check: lint
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/stackbuild/stackbuild.go (1)
513-748: Python stack implementation looks good, minor inconsistency in cache mount setup.The package manager detection and WebCommand inference are well-designed. The cache directory setup is mostly correct, but there's a minor inconsistency:
- pipenv (line 683) and poetry (line 705) use explicit
Mkdir("/home/app/.cache", ...)before mounting- uv (lines 731-733) creates the cache inline within the mount
While both approaches work, using explicit mkdir consistently would be clearer.
Consider standardizing cache directory creation:
case pythonPkgUv: // Copy pyproject.toml and uv.lock first uvState := state.File(llb.Copy(localCtx, "/", "/app", &llb.CopyInfo{ IncludePatterns: []string{"pyproject.toml", "uv.lock", "README.md"}, }), llb.WithCustomName("copy pyproject.toml and uv.lock")) // Install uv state = uvState.Run( llb.Shlex("pip install uv"), llb.AddMount("/root/.cache/pip", pipCache, llb.AsPersistentCacheDir("pip", llb.CacheMountShared)), llb.WithCustomName("[phase] Installing uv"), ).Root() + state = state.File(llb.Mkdir("/home/app/.cache", 0777, llb.WithParents(true))) + uvCache := llb.Scratch().File( + llb.Mkdir("/uv", 0777, llb.WithParents(true)), + ) + // Install dependencies with uv sync state = s.chownApp(state).Dir("/app").Run( llb.Shlex("uv sync --no-dev"), - llb.AddMount("/home/app/.cache", llb.Scratch().File( - llb.Mkdir("/uv", 0777, llb.WithParents(true)), - ), llb.AsPersistentCacheDir("user-uv", llb.CacheMountShared)), + llb.AddMount("/home/app/.cache", uvCache, llb.AsPersistentCacheDir("user-uv", llb.CacheMountShared)), llb.User("app"), llb.WithCustomName("[phase] Installing Python dependencies with uv"), ).Root()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/stackbuild/stackbuild.go(23 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
pkg/stackbuild/stackbuild.go
🧠 Learnings (1)
📚 Learning: 2025-05-29T16:33:23.497Z
Learnt from: phinze
Repo: mirendev/runtime PR: 70
File: Makefile:15-16
Timestamp: 2025-05-29T16:33:23.497Z
Learning: The dagger module in the mirendev/runtime repository has custom test functionality that supports additional flags like `--tags` and `--tests` for filtering tests, as implemented in dagger/main.go.
Applied to files:
pkg/stackbuild/stackbuild.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-e2e
- GitHub Check: test
- GitHub Check: build-binaries (darwin, arm64, macos-latest)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: lint
🔇 Additional comments (9)
pkg/stackbuild/stackbuild.go (9)
42-68: LGTM: Clean API design for detection events and stack lifecycle.The DetectionEvent type and updated Stack interface provide a solid foundation for the analysis flow. The Init/WebCommand/Events lifecycle is well-structured.
71-91: LGTM: DetectStack now properly initializes stacks.The updated signature ensures Init is called after detection, addressing the previous concern about RubyStack and other stacks depending on Init being called before GenerateLLB.
93-119: LGTM: Event tracking mechanism is well-implemented.The MetaStack event support provides a clean foundation for stacks to report detection findings.
192-477: LGTM: RubyStack initialization flow is correct.The Init method properly sets detection state used by GenerateLLB and WebCommand. The previous concern about GenerateLLB depending on Init being called first is now resolved by the updated DetectStack flow.
923-1095: LGTM: NodeStack implementation is solid.The package manager detection, script parsing, and WebCommand inference are well-designed. The TypeScript fallback using
npx tsxis a nice touch.
1097-1226: LGTM: BunStack follows NodeStack pattern consistently.The Bun implementation mirrors the Node.js approach, which promotes consistency across the codebase.
1302-1319: Verify: parseGoModVersion returns full version, but PR objectives mention major.minor.The PR objectives state "Go version detection from go.mod (major.minor)", but
parseGoModVersioncurrently returns the full version string (e.g., "1.23.1" instead of "1.23"). This might be intentional if you want to use the exact version specified in go.mod, but please verify this aligns with your requirements.If you intended to extract only major.minor, apply this diff:
func (s *GoStack) parseGoModVersion() string { content, err := s.readFile("go.mod") if err != nil { return "" } lines := strings.SplitSeq(string(content), "\n") for line := range lines { line = strings.TrimSpace(line) if strings.HasPrefix(line, "go ") { parts := strings.Fields(line) if len(parts) >= 2 { - return parts[1] + version := parts[1] + // Extract major.minor only + versionParts := strings.Split(version, ".") + if len(versionParts) >= 2 { + return versionParts[0] + "." + versionParts[1] + } + return version } } } return "" }
1376-1387: LGTM: APP environment variable pattern is consistent.Setting the
APPenvironment variable at line 1376 allows referencing the binary location. This pattern is also used in RustStack (line 1511), promoting consistency.
1389-1521: LGTM: RustStack is well-implemented and addresses previous binary naming concern.The new Rust stack implementation is solid. The binary naming logic (lines 1491-1501) correctly handles Cargo's hyphen-to-underscore conversion by trying the normalized name first, then falling back to the original name. This addresses the previous review comment about
my-appproducing a binary namedmy_app.
Summary
deploy --analyzecommand for pre-build app analysisLanguages & Frameworks Supported
Languages:
Python frameworks: Django, Flask, FastAPI, Gunicorn, Uvicorn
Key Changes
Stack Detection (
pkg/stackbuild/)AnalyzeApp RPC (
servers/build/)CLI (
cli/commands/deploy.go)--analyzeflag to analyze without buildingDocker Build
Test plan
make testto verify all tests passm deploy --analyzeon sample apps (Go, Python, Rust, Node)Summary by CodeRabbit
New Features
New Languages / Detection
Behavior Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.