-
-
Notifications
You must be signed in to change notification settings - Fork 5
feature: velox v2 (v2025.2) #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the builder to a plugin-centric model: Builder now accepts plugins via options, Build returns a binary path, templates and template compilation are reworked to RRModuleVersion, GitHub/GitLab repo-aggregation and modules parsing removed, config moved to Plugins/TargetPlatform, CLI uses cached GitHub downloads and OS-aware executable naming. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (build cmd)
participant Cache as RRCache
participant GH as GitHub HTTP Client
participant Builder as Builder
participant Templates as Template generator
participant Go as Go toolchain
participant FS as Filesystem
CLI->>Cache: init cache
CLI->>GH: NewHTTPClient(token, cache)
CLI->>GH: Download template (uuid)
GH-->>CLI: template path
CLI->>Builder: NewBuilder(tmp, WithPlugins(...), WithGOOS/GOARCH..., WithOutputDir...)
CLI->>Builder: Build(rrRef)
Builder->>Templates: Generate plugins.go & go.mod (RRModuleVersion)
Builder->>Go: go mod download / go mod tidy
Builder->>Go: go build (OS-aware name)
Go-->>Builder: built binary
Builder->>FS: move binary -> outputDir
Builder-->>CLI: return binary path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces Velox v2, a complete redesign and rework of the Velox build system for RoadRunner. The changes simplify configuration, improve performance, and reduce network usage by leveraging Go modules instead of downloading plugins directly.
- Simplified configuration using only module names and tags according to Go conventions
- Replaced GitHub/GitLab-specific plugin downloads with Go module-based approach
- Added support for cross-platform builds with target platform configuration
Reviewed Changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| velox.toml | Updated configuration format from host-specific to module-based plugin definitions |
| config.go | Simplified config structure removing GitLab support and complex plugin configurations |
| builder/builder.go | Refactored builder to use plugin objects instead of module info structures |
| github/github.go | Replaced complex GitHub API integration with simple HTTP client for template downloads |
| internal/cli/build/build.go | Updated build command to use new plugin and builder architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🔭 Outside diff range comments (5)
github/github.go (3)
73-80: Cache mutation bug: Get() result is reset, likely corrupting cache
defer rrbuf.Reset()assumesGetreturns a copy. The interface doesn’t guarantee that and most caches return the stored pointer. Resetting it will zero the cached data, breaking subsequent reads. Avoid mutating the returned buffer; copy it before use.Apply this diff:
- if rrbuf := r.cache.Get(rrVersion); rrbuf != nil { - // here we know that we have a cached buffer - // just save it to the new location (downloadDir + hash) - // rrbuf is a copy of the buffer, so we can freely clear it - defer rrbuf.Reset() - return r.saveRR(rrbuf, rrVersion, filepath.Join(downloadDir, hash)) - } + if rrbuf := r.cache.Get(rrVersion); rrbuf != nil { + // Make an isolated copy to avoid mutating the cached buffer. + local := bytes.NewBuffer(append([]byte(nil), rrbuf.Bytes()...)) + return r.saveRR(local, rrVersion, filepath.Join(downloadDir, hash)) + }
103-145: HTTP resource leaks and brittle redirect/status handling
- The first response body isn’t closed, leaking a connection.
- Strictly checking for only 302 (Found) is brittle; accept any 3xx redirect.
- No status check on the final GET before reading the body.
Apply this diff:
func (r *GitHubClient) downloadRR(buf *bytes.Buffer, rrurl *url.URL) error { r.log.Info("sending download request", zap.String("url", rrurl.String())) req, err := http.NewRequest(http.MethodGet, rrurl.String(), nil) if err != nil { return err } // first request with the original link should return a redirect - resp, err := r.internalClient.Do(req) + resp, err := r.internalClient.Do(req) if err != nil { return err } - - // check the redirect - if resp.StatusCode != http.StatusFound { - return fmt.Errorf("wrong response status, got: %d", resp.StatusCode) - } + // check the redirect (accept any 3xx) + if resp.StatusCode < 300 || resp.StatusCode >= 400 { + _ = resp.Body.Close() + return fmt.Errorf("unexpected status on initial request, got: %d", resp.StatusCode) + } + // ensure the first response body is closed + _ = resp.Body.Close() // we need to follow the redirect, it should be only 1 redirect, so we don't use a recursive approach locurl, err := resp.Location() if err != nil { return fmt.Errorf("failed to get location from response: %w", err) } @@ - // perform the final request to the redirected URL - resp, err = r.internalClient.Get(locurl.String()) + // perform the final request to the redirected URL + resp, err = r.internalClient.Get(locurl.String()) if err != nil { return fmt.Errorf("failed to download repository: %w", err) } + if resp.StatusCode != http.StatusOK { + _ = resp.Body.Close() + return fmt.Errorf("unexpected status on download request, got: %d", resp.StatusCode) + } _, err = io.Copy(buf, resp.Body) _ = resp.Body.Close() if err != nil { return fmt.Errorf("failed to copy response body: %w", err) }
160-178: Zip file not closed before opening it for readingYou write to the zip file and immediately open it via
zip.OpenReaderwhile the writer is still open. This risks reading incomplete data and relying on implicit flushes.Apply this diff to close the file before opening the zip reader:
r.log.Debug("saving repository in temporary folder", zap.String("path", rrSaveDest+zipExt)) f, err := os.Create(rrSaveDest + zipExt) if err != nil { return "", fmt.Errorf("failed to create zip file %s: %w", rrSaveDest+zipExt, err) } - - defer func() { - _ = f.Close() - }() n, err := f.Write(buf.Bytes()) if err != nil { return "", err } r.log.Debug("repository saved", zap.Int("bytes written", n)) + // close writer to flush data before reading + if err := f.Close(); err != nil { + return "", err + } + rc, err := zip.OpenReader(rrSaveDest + zipExt) if err != nil { return "", err }internal/cli/server/server.go (1)
79-87: Fix TOCTOU race on currentlyProcessing (duplicate builds can slip through).Contains + Add are not atomic; two concurrent requests for the same key can both proceed, corrupting filesystem state under the same temp path.
Minimal fix using a per-server mutex to serialize the check-and-add:
- if b.currentlyProcessing.Contains(hash) { + b.procMu.Lock() + if b.currentlyProcessing.Contains(hash) { + b.procMu.Unlock() b.log.Debug("currently processing", zap.String("key", hash)) return nil, connect.NewError(connect.CodeAlreadyExists, fmt.Errorf("build is already in progress")) } - - // save currently processing key - // needed for concurrent requests for the same request_id - b.currentlyProcessing.Add(hash, struct{}{}) - defer b.currentlyProcessing.Remove(hash) + // save currently processing key + // needed for concurrent requests for the same request_id + b.currentlyProcessing.Add(hash, struct{}{}) + b.procMu.Unlock() + defer func() { + b.procMu.Lock() + b.currentlyProcessing.Remove(hash) + b.procMu.Unlock() + }()And add this field to BuildServer:
// add to BuildServer procMu sync.MutexIf you prefer deduplication over rejection, consider golang.org/x/sync/singleflight to coalesce identical builds.
builder/builder.go (1)
236-253: Combine ldflags into a single -ldflags invocationCurrently, -ldflags is appended twice; only the last one takes effect, so -s is lost. Merge flags into one string.
switch b.debug { case true: // debug flags // turn off optimizations buildCmdArgs = append(buildCmdArgs, "-gcflags", "-N") // turn off inlining buildCmdArgs = append(buildCmdArgs, "-gcflags", "-l") // build with debug tags buildCmdArgs = append(buildCmdArgs, "-tags", "debug") case false: - buildCmdArgs = append(buildCmdArgs, "-ldflags", "-s") + // strip debug info via linker flag, will be merged with meta flags below } // LDFLAGS for version and build time, always appended - buildCmdArgs = append(buildCmdArgs, "-ldflags") - buildCmdArgs = append(buildCmdArgs, fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) + ldFlagsParts := make([]string, 0, 3) + if !b.debug { + ldFlagsParts = append(ldFlagsParts, "-s") + } + ldFlagsParts = append(ldFlagsParts, fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) + buildCmdArgs = append(buildCmdArgs, "-ldflags", strings.Join(ldFlagsParts, " "))
🧹 Nitpick comments (20)
github/github.go (2)
191-200: Redundant directory removal/creation and potential simplificationYou remove
dest, thenMkdironrrSaveDest. GivendestisAbs(rrSaveDest), this sequence duplicates earlierRemoveAll/MkdirAllwork and can be simplified to a single, consistent create path.Consider replacing the pair with:
- Single
os.RemoveAll(dest)(once) before extraction.- Single
os.MkdirAll(dest, os.ModePerm).This reduces churn and makes the intent clearer.
206-214: Stray Abs and “..” checks on zip root entry are ineffective
filepath.Abs(rc.File[0].Name)doesn’t validate againstdestand can yield a path unrelated to the extraction root. You already have a robust Zip Slip check insideextract. The additional Abs-based “..” checks add noise and potential confusion.Remove the Abs-based checks and rely on
extract’s safe-join guard. If you still want a quick sanity check, checkrc.File[0].Namedirectly for..without Abs.github/github_test.go (2)
19-21: Use t.TempDir() for isolation and automatic cleanupUsing
os.TempDir()can leave artifacts behind on successful runs and risks collisions. Prefert.TempDir().Apply this diff:
- // Use os.TmpDir() as requested - tmpDir := os.TempDir() + // Isolated temp dir managed by the test framework + tmpDir := t.TempDir()
11-34: Avoid live network in unit tests; replace with httptest or mark as integrationThis test calls out to GitHub and accepts errors, so it doesn’t assert behavior and is flaky/offline-hostile. Consider stubbing with
httptest.Serverto simulate a 302 + 200 flow and assert:
- correct handling of 3xx redirect,
- error on non-3xx initial status,
- error on non-200 download status,
- cache hit path avoids network.
Alternatively, mark as integration and
t.Skip()by default, enabling via an env flag in CI.I can provide a self-contained
httptest-based rewrite exercising both cold and warm-cache paths. Want me to draft it?builder/templates/templateV2024.go (3)
16-16: Drop test-only dependency from go.mod template.The template pins github.com/stretchr/testify, which is test-only. Keeping it in the generated go.mod increases network usage and cache churn for no runtime benefit. It also conflicts with PR goals of reducing downloads.
Apply this diff to remove it:
- github.com/stretchr/testify v1.9.0
22-24: Validate the shape of .Requires entries to be go.mod-compliant.The range renders raw strings inside require(). Ensure each entry strictly matches "module version" (no "latest", prefixes, or extra tokens). Otherwise,
go mod tidy/go buildwill fail.I can add a small unit test that feeds a few plugin sets into NewTemplate and asserts the compiled go.mod parses under
modfile.Parse. Want me to draft it?
40-41: Imports and plugin code slices rely on preformatted strings; add coverage.Since .Imports and .Code are injected verbatim, a malformed entry (missing quotes, alias, or ampersand) will compile into invalid Go. Recommend adding a test similar to the v2025 test that compiles the v2024 plugins template with a few sample plugins and asserts presence of imports and &Plugin{} entries.
Minor wording nit in the comment:
- // format should use prefix as it used in the .Plugins in the Mod template + // format should use the same prefixing as used by the GoMod templateAlso applies to: 45-45, 53-55
internal/cli/server/server.go (1)
126-135: Validate GOOS/GOARCH early and avoid duplicate RR version configuration.
- The values come from the request without strong validation. Recommend normalizing/validating against the supported set (e.g., dist list) to fail fast with a clear error.
- RR version is provided both via WithRRVersion and Build(rrVersion). Keeping both is redundant and risks drift.
Apply this diff to remove duplication:
opts = append(opts, builder.WithPlugins(bplugins...), builder.WithOutputDir(outputPath), - builder.WithRRVersion(req.Msg.GetRrVersion()), builder.WithLogs(sb), builder.WithLogger(b.log.Named("Builder")), builder.WithGOOS(req.Msg.GetTargetPlatform().GetOs()), builder.WithGOARCH(req.Msg.GetTargetPlatform().GetArch()), )I can also add a small validator to check GOOS/GOARCH against a curated list and map common aliases (e.g., aarch64 -> arm64). Want me to draft it?
builder/options.go (1)
20-24: Guard against nil plugins and document override behavior.Setting b.plugins directly will panic later if any element is nil. Also, document that WithPlugins replaces any previously set plugins.
Apply this diff:
-func WithPlugins(plugins ...*plugin.Plugin) Option { - return func(b *Builder) { - b.plugins = plugins - } -} +// WithPlugins sets the plugins to include in the build. This replaces any previously set plugins. +func WithPlugins(plugins ...*plugin.Plugin) Option { + // filter out nils to avoid panics later + filtered := make([]*plugin.Plugin, 0, len(plugins)) + for _, p := range plugins { + if p != nil { + filtered = append(filtered, p) + } + } + return func(b *Builder) { + b.plugins = filtered + } +}internal/cli/root.go (1)
39-63: Unmarshal directly into config to simplify and avoid double indirection.The current flow unmarshals into a pointer-to-pointer and then copies. Simplify by unmarshalling directly into config and validating it.
- var cfg *velox.Config + // use the pre-initialized config // the user doesn't provide a path to the config if pathToConfig == "" { return errors.New("path to the config should be provided") } @@ - err = v.Unmarshal(&cfg) + err = v.Unmarshal(config) if err != nil { return err } - err = cfg.Validate() + err = config.Validate() if err != nil { return err } - *config = *cfg *cfgPath = outputFileconfig_test.go (2)
57-59: Avoid calling Validate() twice; assert on the captured errorMinor test hygiene: don’t invoke c.Validate() twice. It may have side-effects or be expensive.
- require.Error(t, c.Validate()) - assert.Contains(t, c.Validate().Error(), "plugins configuration is required") + err := c.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "plugins configuration is required")
73-75: Same here: capture error once- require.Error(t, c.Validate()) - assert.Contains(t, c.Validate().Error(), "tag is required") + err := c.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "tag is required")internal/cli/build/build.go (4)
28-35: Use os.Getwd instead of syscall.GetwdPrefer the standard library wrapper. No need to depend on syscall here.
- if *out == "." { - wd, err := syscall.Getwd() + if *out == "." { + wd, err := os.Getwd() if err != nil { return err } *out = wd }Also remove the unused syscall import.
36-43: Deterministic plugin order for reproducible buildsIterating over a map yields random order. Sort keys first to stabilize build artifacts.
- bplugins := make([]*plugin.Plugin, 0, len(cfg.Plugins)) - for _, p := range cfg.Plugins { - if p == nil { - zlog.Warn("plugin info is nil") - continue - } - bplugins = append(bplugins, plugin.NewPlugin(p.ModuleName, p.Tag)) - } + // deterministic plugin order + bplugins := make([]*plugin.Plugin, 0, len(cfg.Plugins)) + keys := make([]string, 0, len(cfg.Plugins)) + for k := range cfg.Plugins { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + p := cfg.Plugins[k] + if p == nil { + zlog.Warn("plugin info is nil", zap.String("plugin", k)) + continue + } + bplugins = append(bplugins, plugin.NewPlugin(p.ModuleName, p.Tag)) + }You’ll need to import sort.
45-51: Use config token when provided; avoid os.Exit in RunEPrefer cfg.GitHub.Token when present (it may contain expanded envs). Fall back to GITHUB_TOKEN env if config is empty. Also, return errors from RunE instead of os.Exit to let Cobra handle them.
- rrcache := cacheimpl.NewRRCache() - rp := github.NewHTTPClient(os.Getenv("GITHUB_TOKEN"), rrcache, zlog.Named("GitHub")) - path, err := rp.DownloadTemplate(os.TempDir(), uuid.NewString(), cfg.Roadrunner[ref]) - if err != nil { - zlog.Error("downloading template", zap.Error(err)) - os.Exit(1) - } + rrcache := cacheimpl.NewRRCache() + token := "" + if cfg.GitHub != nil && cfg.GitHub.Token != nil { + token = cfg.GitHub.Token.Token + } + if token == "" { + token = os.Getenv("GITHUB_TOKEN") + } + rp := github.NewHTTPClient(token, rrcache, zlog.Named("GitHub")) + path, err := rp.DownloadTemplate(os.TempDir(), uuid.NewString(), cfg.Roadrunner[ref]) + if err != nil { + return fmt.Errorf("downloading template: %w", err) + }You’ll need to import fmt.
75-81: Return the build error instead of exitingReturning the error respects Cobra’s RunE contract and is easier to test.
- binaryPath, err := builder.NewBuilder(path, opts...).Build(cfg.Roadrunner[ref]) - if err != nil { - zlog.Error("fatal", zap.Error(err)) - os.Exit(1) - } + binaryPath, err := builder.NewBuilder(path, opts...).Build(cfg.Roadrunner[ref]) + if err != nil { + return fmt.Errorf("build failed: %w", err) + }config.go (1)
75-82: Optional: clarify validation error messagesCurrent messages include the counterpart field value (moduleName/tag). Consider a more uniform phrasing to aid users scanning errors.
Example:
- "plugin.tag is required (plugin: )"
- "plugin.module_name is required (tag: )"
No functional change needed; just a suggestion for UX.
builder/builder.go (3)
172-181: Fix output directory existence checkos.IsExist(err) is not meaningful after Stat with nil error. Clean up when Stat succeeds; error out only on unexpected errors.
_, err = os.Stat(b.outputDir) - if err != nil && !os.IsNotExist(err) { - return "", fmt.Errorf("stat failed for output directory %s: %w", b.outputDir, err) - } - - if os.IsExist(err) { - b.log.Info("output path already exists, cleaning up", zap.String("dir", b.outputDir)) - _ = os.RemoveAll(b.outputDir) - } + if err == nil { + b.log.Info("output path already exists, cleaning up", zap.String("dir", b.outputDir)) + _ = os.RemoveAll(b.outputDir) + } else if !os.IsNotExist(err) { + return "", fmt.Errorf("stat failed for output directory %s: %w", b.outputDir, err) + }
311-338: Avoid loading the entire binary into memory when moving; try rename then stream copyThe current moveFile reads the entire binary into memory and leaves the source file in place. Prefer os.Rename with an io.Copy fallback for cross-device moves.
Also add import "io".
-func moveFile(from, to string) error { - ffInfo, err := os.Stat(from) - if err != nil { - return err - } - - fFile, err := os.ReadFile(from) - if err != nil { - return err - } - - toFile, err := os.Create(to) - if err != nil { - return err - } - - err = toFile.Chmod(ffInfo.Mode()) - if err != nil { - return err - } - - _, err = toFile.Write(fFile) - if err != nil { - return err - } - - return toFile.Close() -} +func moveFile(from, to string) error { + // Try a simple rename first (fast path) + if err := os.Rename(from, to); err == nil { + return nil + } + + // Fallback to copy if rename is not possible (e.g., cross-device) + src, err := os.Open(from) + if err != nil { + return err + } + defer src.Close() + + dst, err := os.Create(to) + if err != nil { + return err + } + defer func() { _ = dst.Close() }() + + if fi, err := src.Stat(); err == nil { + _ = dst.Chmod(fi.Mode()) + } + + _, err = io.Copy(dst, src) + return err +}
3-18: Import io for moveFile changeAdd io to imports:
import ( "bytes" "context" + "io" "fmt"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
builder/builder.go(5 hunks)builder/builder_test.go(0 hunks)builder/options.go(2 hunks)builder/template_test.go(0 hunks)builder/templates/compile.go(1 hunks)builder/templates/entry.go(0 hunks)builder/templates/templateV2.go(0 hunks)builder/templates/templateV2023.go(0 hunks)builder/templates/templateV2024.go(2 hunks)builder/templates/templateV2025.go(4 hunks)builder/templates/template_test.go(1 hunks)config.go(3 hunks)config_test.go(2 hunks)github/github.go(1 hunks)github/github_test.go(1 hunks)github/parse_test.go(0 hunks)github/pool.go(0 hunks)github/repo.go(0 hunks)gitlab/repo.go(0 hunks)go.mod(1 hunks)internal/cli/build/build.go(2 hunks)internal/cli/root.go(1 hunks)internal/cli/server/server.go(1 hunks)modulesInfo.go(0 hunks)v2/builder/builder.go(0 hunks)v2/builder/options.go(0 hunks)v2/builder/templates/templateV2024.go(0 hunks)v2/builder/templates/templateV2025.go(0 hunks)v2/config/builder.go(0 hunks)velox.toml(1 hunks)
💤 Files with no reviewable changes (15)
- builder/templates/templateV2.go
- builder/template_test.go
- builder/builder_test.go
- v2/config/builder.go
- v2/builder/templates/templateV2024.go
- v2/builder/templates/templateV2025.go
- builder/templates/entry.go
- github/repo.go
- github/pool.go
- v2/builder/options.go
- github/parse_test.go
- gitlab/repo.go
- modulesInfo.go
- v2/builder/builder.go
- builder/templates/templateV2023.go
🧰 Additional context used
🧬 Code Graph Analysis (12)
builder/templates/template_test.go (3)
v2/builder/templates/template_test.go (1)
TestCompileV2025(12-48)builder/builder_test.go (1)
Test_Builder_getDepsReplace_multipleAbsolute_V2024(148-155)builder/template_test.go (1)
TestCompileV2024(84-140)
builder/templates/compile.go (1)
v2/builder/templates/template_test.go (1)
TestCompileV2025(12-48)
github/github_test.go (2)
v2/github/github.go (2)
internalClient(35-40)Get(27-30)v2/github/github_test.go (1)
TestGitHubClient_DownloadTemplate(11-34)
builder/options.go (5)
config.go (1)
Plugin(50-53)plugin/plugin.go (1)
Plugin(13-17)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)builder/builder.go (1)
Builder(32-44)v2/builder/builder.go (1)
b(61-203)
internal/cli/server/server.go (2)
v2/builder/templates/compile.go (1)
RRModuleVersion(11-20)v2/builder/templates/template_test.go (1)
TestCompileV2025(12-48)
internal/cli/root.go (2)
config.go (1)
Config(18-31)cmd/vx/main.go (1)
main(11-19)
builder/templates/templateV2024.go (2)
v2/builder/templates/compile.go (3)
RRModuleVersion(11-20)CompileGoModTemplate2025(39-46)CompileGoModTemplate2024(62-74)builder/templates/entry.go (2)
Module(9-20)CompileGoModTemplate2024(112-124)
builder/templates/templateV2025.go (2)
v2/builder/templates/compile.go (2)
CompileGoModTemplate2025(39-46)RRModuleVersion(11-20)builder/template_test.go (1)
TestCompileV2025(142-198)
builder/builder.go (7)
config.go (3)
Plugin(50-53)V2025(14-14)V2024(15-15)plugin/plugin.go (1)
Plugin(13-17)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)builder/options.go (1)
Option(11-11)builder/templates/compile.go (4)
NewTemplate(22-37)CompileTemplateV2025(48-60)CompileTemplateV2024(76-88)CompileGoModTemplate2025(39-46)v2/builder/builder.go (1)
b(61-203)builder/builder_test.go (3)
Test_Builder_getDepsReplace_multipleAbsolute_V2024(148-155)Test_Builder_getDepsReplace_multipleAbsolute_V2025(193-200)Test_Builder_getDepsReplace_multipleRelative_V2025(202-209)
config_test.go (1)
config.go (6)
Config(18-31)Debug(33-35)GitHub(42-44)Token(46-48)TargetPlatform(37-40)Plugin(50-53)
internal/cli/build/build.go (9)
config.go (2)
Plugin(50-53)TargetPlatform(37-40)plugin/plugin.go (2)
Plugin(13-17)NewPlugin(23-29)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)cache/cache.go (1)
NewRRCache(14-19)github/github.go (1)
NewHTTPClient(43-68)builder/options.go (7)
Option(11-11)WithPlugins(20-24)WithOutputDir(55-59)WithRRVersion(62-66)WithLogger(48-52)WithGOOS(27-31)WithGOARCH(34-38)builder/builder.go (1)
NewBuilder(47-58)gitlab/repo.go (1)
r(41-134)v2/builder/builder.go (2)
b(61-203)rrTempPath(32-44)
config.go (2)
plugin/plugin.go (1)
Plugin(13-17)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)
🪛 GitHub Actions: Linux
config_test.go
[error] 39-39: TestExpandEnvs failed: Not equal: expected 'foobarbaz', actual '${TOKEN}'.
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
github/github.go (2)
1-1: Package doc comment addition looks goodClear, concise package doc that explains the purpose. No action needed.
233-250: Ambiguous handling of “latest” refs
parseRRreftreats anything not prefixed withvand not 40 chars as a branch, which makeslatestresolve to/refs/heads/latest.zip. If callers pass “latest” intending “latest tag,” this will be incorrect.Please confirm caller guarantees: do we ever pass “latest” for RR? If yes, consider:
- mapping “latest” to the default branch (“master”/“main”), or
- resolving the latest tag via the GitHub API when an access token is present, or
- rejecting ambiguous refs with a clear error.
I can draft a small helper to resolve the latest tag if desired.
builder/templates/compile.go (1)
8-8: Import path update to v2025/plugin looks consistentMatches the repo’s new layout and the plugin-centric refactor. No issues spotted.
go.mod (1)
12-12: No stale references to removed dependencies foundI ran broad searches across the codebase for imports or references to
github.com/google/go-github,gitlab.com/gitlab-org/api, andgolang.org/x/modand found no occurrences. The dependency surface is clean.builder/templates/templateV2025.go (3)
4-8: Go directive/toolchain updates look fineUsing go 1.25 and toolchain pin is consistent with the repo. No issues here.
40-42: Import section generation looks correctIterating
.Importsis simple and flexible. Assumingp.Imports()returns fully-formed import lines ("module/path"), this is good.
53-55: Plugins slice generation aligns with p.Code() contract
&{{$v}},impliesp.Code()returnspkg.Plugin{}(no ampersand). Ensure this invariant is documented/enforced inplugin.Plugin. Otherwise, this will double-ampersand or miss one.Confirm
plugin.Plugin.Code()returns something likeinformer.Plugin{}(without&). If not, we should adjust either the template or the method to avoid ambiguity.builder/templates/templateV2024.go (1)
4-6: Confirm module path correctness for RRModuleVersion (major vs full semver).If RRModuleVersion carries a full semver (e.g., v2024.1.0), appending it to the module path will produce an invalid module path. For Go modules, only the major version (e.g., /v2024) belongs in the path. Please ensure RRModuleVersion provided to this template is major-only, or adjust the data/provider accordingly.
builder/templates/template_test.go (1)
7-8: Import path updates look correct and consistent with the v2025 layout.internal/cli/server/server.go (1)
21-27: Good migration to v2025 packages; aliasing cache avoids interface conflicts.internal/cli/root.go (1)
26-26: Initializing config avoids nil deref; good defensive fix.config_test.go (1)
77-92: LGTM: sensible TargetPlatform defaultsDefaulting to runtime.GOOS/GOARCH when not specified is correct and is exercised by this test.
velox.toml (2)
11-18: LGTM: token via env; platform set explicitlyUsing ${GITHUB_TOKEN} and specifying a target platform is consistent with the new config model and builder flow.
19-150: LGTM: unified plugin schemaPer-plugin tag and module_name mapping aligns with the Plugin type and the plugin-centric builder. No issues spotted.
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
update_plugins.py (5)
1-3: Add timeouts and token-aware GitHub API calls to avoid hangs and rate-limit failuresRequests have no timeout and don’t use Accept/Authorization headers. This can hang indefinitely and hit GH rate limits. Add optional token support via GITHUB_TOKEN/GH_TOKEN and set sane timeouts.
Apply these diffs:
- Imports
-import requests -import toml +import os +import requests +import toml +from typing import Dict
- API headers and timeout
api_url_template = "https://api.github.com/repos/{owner}/{repo}/releases/latest" + +# Optional: use a GitHub token to increase rate limits +GITHUB_TOKEN = os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN") +HEADERS = { + "Accept": "application/vnd.github+json", + **({"Authorization": f"Bearer {GITHUB_TOKEN}"} if GITHUB_TOKEN else {}), +} +TIMEOUT = 10 # seconds
- Robust request with timeout
def get_latest_release(owner: str, repo: str) -> str: url = api_url_template.format(owner=owner, repo=repo) - response = requests.get(url) + response = requests.get(url, headers=HEADERS, timeout=TIMEOUT) if response.status_code == 200: return response.json()["tag_name"] - else: - raise Exception(f"Failed to fetch the latest release for {owner}/{repo}.") + elif response.status_code in (403, 429): + raise Exception(f"Rate limited when fetching {owner}/{repo}. Provide GITHUB_TOKEN to increase limits.") + elif response.status_code == 404: + raise Exception(f"No releases found for {owner}/{repo}.") + else: + raise Exception(f"Failed to fetch the latest release for {owner}/{repo}. HTTP {response.status_code}.")Also applies to: 42-45, 47-53
57-63: Don’t abort the whole update on a single fetch failure; degrade gracefullyExiting on the first failure stops updating other plugins. Log and continue instead.
Apply this diff:
-for plugin, info in plugins.items(): - try: - latest_versions[plugin] = get_latest_release(info["owner"], info["repo"]) - except Exception as e: - exit(f"Error: {e}") +for plugin, info in plugins.items(): + try: + latest_versions[plugin] = get_latest_release(info["owner"], info["repo"]) + except Exception as e: + print(f"Warning: failed to fetch latest release for {plugin} ({info['owner']}/{info['repo']}): {e}") + continue
57-57: Use typing compatible with Python 3.8dict[str, str] requires Python 3.9+. If this script runs on older systems, prefer typing.Dict.
Apply this diff:
-latest_versions = dict[str, str]() +latest_versions: Dict[str, str] = {}
46-53: Fallback to tags for repos without GitHub ReleasesSome repos publish tags but no Releases. GET /releases/latest returns 404 in that case. Add a fallback to GET /tags?per_page=1 or similar.
Example replacement for get_latest_release (keeps your interface, uses HEADERS/TIMEOUT):
def get_latest_release(owner: str, repo: str) -> str: # Try GitHub latest release url = f"https://api.github.com/repos/{owner}/{repo}/releases/latest" r = requests.get(url, headers=HEADERS, timeout=TIMEOUT) if r.status_code == 200: tag = r.json().get("tag_name") if tag: return tag elif r.status_code in (403, 429): raise Exception(f"Rate limited for {owner}/{repo}") # Fallback to latest tag if no releases tags_url = f"https://api.github.com/repos/{owner}/{repo}/tags?per_page=1" t = requests.get(tags_url, headers=HEADERS, timeout=TIMEOUT) if t.status_code == 200 and t.json(): return t.json()[0]["name"] raise Exception(f"Unable to determine latest version for {owner}/{repo}.")
6-40: Static map is in sync—but consider an optional refactor for flexibilityI ran the verification script and confirmed there are no mismatches between velox.toml’s
[plugins.*]entries and the hardcoded map in update_plugins.py:
- Plugins in velox.toml but missing in mapping: []
- Plugins in mapping but not in velox.toml: []
Everything lines up today, but the current approach still locks you into GitHub and risks future drift if a plugin’s module_name or host ever changes. As an optional improvement:
• Read the plugin list directly from velox.toml
• Parse each plugin’smodule_name(e.g.github.com/owner/repo,gitlab.com/...) to infer host and repo
• Fetch tags via the appropriate API (GitHub, GitLab, Bitbucket)Sketch of a helper:
import urllib.parse def parse_host_repo(module_name: str): parsed = urllib.parse.urlparse("https://" + module_name) host = parsed.netloc.lower() owner, repo = parsed.path.strip("/").split("/")[:2] repo = repo[:-4] if repo.endswith(".git") else repo if host.endswith("github.com"): return "github", owner, repo if host.endswith("gitlab.com"): return "gitlab", owner, repo if host.endswith("bitbucket.org"): return "bitbucket", owner, repo return "unknown", owner, repoThis makes the script plugin-centric and host-agnostic, avoiding future KeyErrors or manual map updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
update_plugins.py(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). (1)
- GitHub Check: Analyze (go)
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/cli/root.go (1)
53-56: Fix: Viper Unmarshal into pointer-to-pointer will likely fail and can cause nil derefYou declare
var cfg *velox.Configand then callv.Unmarshal(&cfg). That’s a pointer-to-pointer target; Viper/mapstructure expects a pointer to a concrete struct. As written,cfgmay remain nil, leading to a panic at Line 63 (*cfg) or a silently empty config.Minimal, safe fix:
- Allocate
cfgbefore unmarshal.- Unmarshal into
cfg(not&cfg).Example:
// before line 53 cfg = new(velox.Config) // line 53 err = v.Unmarshal(cfg) if err != nil { return err } // line 63 remains valid *config = *cfgAlternatively, make
cfga value:var cfg velox.Config, thenv.Unmarshal(&cfg)and assign*config = cfg.
🧹 Nitpick comments (5)
internal/cli/root.go (5)
86-86: Pass the base logger, not a pre-named child, to allow PreRunE reconfiguration to take effectYou build a named logger at command construction time:
lg.Named("builder"). Later inPersistentPreRunE, you rebuild the logger and try to update it via*lg = *zlog. However, the previously createdNamedchild holds its own copies (e.g., core), so it won’t reflect the updated logger. Net effect: the build command logs with the initial dev logger, ignoring config.Pass the base
lginstead; the build command derives the name at runtime (zlog.Named("Builder")) and will use the reconfigured logger.Apply this diff:
- build.BindCommand(config, cfgPath, lg.Named("builder")), + build.BindCommand(config, cfgPath, lg),Side note: This also avoids double-naming ("builder" + "Builder") in logs.
24-24: Avoid the generic pointer helper for simple cases; use the actual variable address
cfgPath = p("")allocates an extra string just to hold a pointer. Since you already haveoutputFile, point to it directly. This simplifies ownership and makes intent obvious.Apply this diff:
- cfgPath = p("") + cfgPath = &outputFileWith this,
*cfgPath = outputFileat Line 64 becomes redundant and can be removed for clarity.
92-94: Remove the generic pointer helperp[T any](unused after refactor)Once
cfgPathpoints to&outputFile,p[T any]is no longer needed here. Removing it reduces surface area and avoids hiding escape analysis behind helpers.Apply this diff:
-func p[T any](val T) *T { - return &val -}
40-44: Minor: the empty config-path check is redundant with the current defaultYou default
--configtovelox.toml(Line 81), sopathToConfig == ""will rarely trigger. Consider either:
- Keep it as a defensive check (ok), or
- Drop it to reduce noise.
20-20: Minor: consider handling the logger construction errorCurrently ignoring the error from
zap.NewDevelopment(). If it fails, you’d pass a nil/invalid logger around until overwritten. You can fall back to a no-op logger or exit early.Example:
lg, err := zap.NewDevelopment() if err != nil { return nil // or panic/exit/log fallback }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/cli/build/build.go(2 hunks)internal/cli/root.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/build/build.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cli/root.go (3)
internal/cli/build/build.go (1)
BindCommand(22-76)internal/cli/server/command.go (1)
BindCommand(16-45)cmd/vx/main.go (1)
main(11-19)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/cli/root.go (2)
1-1: Package comment: good additionPackage-level documentation is clear and helpful.
27-27: Initializingconfigto a non-nil value is goodStarting with
config = &velox.Config{}avoids nil handling throughout command binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config.go (1)
72-75: Env expansion for GitHub token: LGTM (as previously requested)The os.ExpandEnv call addresses the earlier review request and aligns with velox.toml/tests.
🧹 Nitpick comments (5)
config.go (5)
1-1: Package documentation nitConsider replacing the placeholder comment with a short, meaningful package doc so go doc tooling shows a helpful summary.
38-41: Default OS/Arch even when TargetPlatform is provided but fields are emptyIf users supply an empty TargetPlatform block, OS/Arch remain empty and may break downstream logic. Fill defaults when fields are empty as well.
Apply this diff in the Validate() method to add resilient defaults:
if c.TargetPlatform == nil { c.TargetPlatform = &TargetPlatform{ OS: runtime.GOOS, Arch: runtime.GOARCH, } +} else { + if c.TargetPlatform.OS == "" { + c.TargetPlatform.OS = runtime.GOOS + } + if c.TargetPlatform.Arch == "" { + c.TargetPlatform.Arch = runtime.GOARCH + } }
51-54: Plugin schema looks right; optional validation ideaThe fields and tags match the v2 design. Optional: validate ModuleName roughly matches a Go module path format to fail early on obvious typos.
65-69: Harden TargetPlatform defaultingSee suggested diff to handle empty fields when TargetPlatform is present. Prevents ending up with empty OS/Arch values.
if c.TargetPlatform == nil { c.TargetPlatform = &TargetPlatform{ OS: runtime.GOOS, Arch: runtime.GOARCH, } -} +} else { + if c.TargetPlatform.OS == "" { + c.TargetPlatform.OS = runtime.GOOS + } + if c.TargetPlatform.Arch == "" { + c.TargetPlatform.Arch = runtime.GOARCH + } +}
77-80: Clarify “plugins required” semanticsCurrently only nil is rejected; an empty map passes validation. If at least one plugin is required, also check len==0.
-if c.Plugins == nil { +if c.Plugins == nil || len(c.Plugins) == 0 { return errors.New("plugins configuration is required") }If empty is intentionally allowed, feel free to ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
config.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
config.go (3)
plugin/plugin.go (1)
Plugin(13-17)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)config_test.go (1)
TestExpandEnvs(12-53)
🪛 GitHub Actions: CodeQL
config.go
[error] 2-2: CodeQL Go extractor error: package requires newer Go version go1.25 (application built with go1.24).
🔇 Additional comments (5)
config.go (5)
12-17: CodeQL pipeline failure: Go version mismatchCI reports CodeQL extractor requires Go 1.25 while the job uses 1.24. Update your CI to use Go 1.25.x (both setup-go and CodeQL job). No code changes required here.
26-31: Config v2 fields addition looks goodNew fields (TargetPlatform, GitHub, Plugins) align with the simplified v2 schema and are idiomatic with mapstructure tags.
43-45: GitHub token container is fineMinimal GitHub block with a Token wrapper keeps room for future fields without breaking schema.
47-49: Token struct: LGTMSimple wrapper suited for env expansion and future metadata.
7-7: runtime import: LGTMUsed for TargetPlatform defaults; no issues.
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config.go (1)
81-90: Fix validation messages and include plugin map key for contextThe messages currently reference the opposite field and omit the map key, which is confusing. Also iterate with the key to improve diagnostics.
-for _, plugin := range c.Plugins { - if plugin == nil { - return fmt.Errorf("plugin is required") - } - if plugin.Tag == "" { - return fmt.Errorf("plugin %s tag is required", plugin.ModuleName) - } - if plugin.ModuleName == "" { - return fmt.Errorf("plugin %s module name is required", plugin.Tag) - } -} +for name, plugin := range c.Plugins { + if plugin == nil { + return fmt.Errorf("plugin %q configuration is required", name) + } + if plugin.ModuleName == "" { + return fmt.Errorf("plugin %q: module name is required", name) + } + if plugin.Tag == "" { + return fmt.Errorf("plugin %q: tag is required", name) + } +}
🧹 Nitpick comments (5)
config.go (5)
1-1: Polish package doc comment to a complete sentenceGo doc comments should be full sentences. This helps pkg.go.dev and IDEs render better docs.
-// Package velox ... +// Package velox provides configuration structures and validation for Velox v2.
4-10: Consider using the stdlib errors package instead of github.com/pkg/errorsYou're only calling New; stdlib errors.New avoids the external dependency with no loss.
import ( + "errors" "fmt" "os" "runtime" - - "github.com/pkg/errors" )
38-41: Optionally validate OS/Arch valuesTo fail fast on invalid target_platform values provided by users, add a basic non-empty check (or a whitelist if you prefer).
c.TargetPlatform = &TargetPlatform{ OS: runtime.GOOS, Arch: runtime.GOARCH, } } + + // Basic sanity check in case user overrides with empty strings + if c.TargetPlatform.OS == "" || c.TargetPlatform.Arch == "" { + return errors.New("target_platform.os and target_platform.arch are required") + }
72-76: Env expansion for GitHub token is correct; consider handling unknown varsos.ExpandEnv leaves unknown vars intact. If a user passes "${GITHUB_TOKEN}" but the env is unset, you’ll get the literal string. If you want to treat that as empty/invalid, add an explicit check post-expansion.
if c.GitHub != nil && c.GitHub.Token != nil { - c.GitHub.Token.Token = os.ExpandEnv(c.GitHub.Token.Token) + c.GitHub.Token.Token = os.ExpandEnv(c.GitHub.Token.Token) + // Optionally, treat unresolved placeholders as empty/invalid: + if c.GitHub.Token.Token == "" { + // token remains optional overall; if you later enforce it, return an error here + } }
43-49: Optional: Flatten GitHub token mapping for simpler TOML
Currently,velox.tomluses the nested form ([github.token]), so if you decide to flatten the struct you’ll need to update docs/tests accordingly.• config.go (lines 43–49):
- type GitHub struct { - Token *Token `mapstructure:"token"` - } - - type Token struct { - Token string `mapstructure:"token"` - } + type GitHub struct { + Token string `mapstructure:"token"` + }• velox.toml (lines 16–17):
– [github.token] – token = "${GITHUB_TOKEN}" + [github] + token = "${GITHUB_TOKEN}"If you’d rather keep the nested
Tokentype, no action is needed—just confirm all examples and tests continue to use[github.token].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
config.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
config.go (2)
plugin/plugin.go (1)
Plugin(13-17)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
config.go (4)
26-31: Config surface LGTMTargetPlatform, GitHub, and Plugins fields are clear and align with the new v2 config direction.
51-54: Plugin schema LGTMmodule_name and tag match the pb schema and the new plugin-centric flow.
65-69: Sane defaults for TargetPlatformDefaulting to runtime.GOOS/GOARCH is a good UX.
77-79: Good guard for missing pluginsEarly error on nil plugins map improves UX.
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/linux.yml (2)
74-79: Map to secrets.GITHUB_TOKEN (or add a fallback), rather than repurposing RT_TOKEN.Renaming the env var to GITHUB_TOKEN is good, but sourcing it from secrets.RT_TOKEN can be confusing and brittle. Prefer the built-in GitHub token, or fall back to it if RT_TOKEN is absent. This also simplifies secret management.
Apply this minimal change:
- GITHUB_TOKEN: ${{ secrets.RT_TOKEN }} + GITHUB_TOKEN: ${{ secrets.RT_TOKEN || secrets.GITHUB_TOKEN }}Optional hardening (outside the selected lines): restrict token scope at the job level to reduce blast radius.
permissions: contents: read packages: readPlease confirm RT_TOKEN is still needed after v2 (tokens are “not required” per the PR); if not, consider removing it from repo secrets.
81-83: Pin vx version; avoid @latest to keep CI reproducible.Installing cmd/vx with @latest can break builds when a new release lands. Pin to a known-good tag or commit.
- go install github.com/roadrunner-server/velox/v2025/cmd/vx@latest + # Pin to a specific tag or commit SHA to avoid unexpected changes: + go install github.com/roadrunner-server/velox/v2025/cmd/[email protected]If you prefer a commit, replace the tag with the SHA. Keep this in sync with velox v2 (v2025.2) used by the repo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/linux.yml(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). (1)
- GitHub Check: Analyze (go)
Reason for This PR
Description of Changes
GITHUB_TOKEN) might be used to download RR, but it is not needed.License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Breaking Changes
Chores