Skip to content

Conversation

@rustatian
Copy link
Member

@rustatian rustatian commented Aug 17, 2025

Reason for This PR

  • Redesign and rework Velox.

Description of Changes

  1. New configuration (incompatible). Simpler, supports all kinds of modules and code hostings like GitHub, GitLab, BitBucket, custom urls, etc.
  2. Much faster builds.
  3. Much less networking usage. Plugins are not downloading like in the previous version. Only by Go modules.
  4. Downloaded plugins are cached in the GOCACHE (like any Go mod dep).
  5. Support for building for the different target platforms.
  6. Simpler configuration: only the module name (according to the Go convention) and tag are needed.
  7. No need for the tokens (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]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • New Features

    • Plugin-centric build: declare plugins by tag+module & target platform; build now produces and returns an OS-aware executable path.
    • Cached GitHub template downloads for faster, repeatable retrieval.
  • Breaking Changes

    • Config schema migrated to unified [plugins.*] and top-level token; legacy GitHub/GitLab plugin blocks removed.
    • Older template-generation/runtime builder paths removed.
  • Chores

    • Dependency and import cleanup; CI env key renamed to GITHUB_TOKEN.

Signed-off-by: Valery Piashchynski <[email protected]>
@rustatian rustatian self-assigned this Aug 17, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 17, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Refactors 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

Cohort / File(s) Summary
Builder core
builder/builder.go, builder/options.go
Rewrote Builder to use plugins []*plugin.Plugin and Option pattern. NewBuilder(rrTmpPath string, opts ...Option) and Build(rrRef string) (string, error) signatures changed. Added WithPlugins, GOOS/GOARCH options, OS-aware executable naming, outputDir cleanup; removed module-centric helpers.
Builder tests removed
builder/builder_test.go, builder/template_test.go
Deleted unit tests validating getDepsReplace and template generation.
Templates (builder)
builder/templates/*, builder/templates/templateV2024.go, builder/templates/templateV2025.go, builder/templates/compile.go
Removed many runtime template compile helpers and entry types; templates now reference RRModuleVersion and new data model (Requires/Imports/Code); import paths updated.
Templates API removed
builder/templates/entry.go, builder/templates/templateV2.go, builder/templates/templateV2023.go, builder/templates/template_test.go
Deleted Entry/Template types, older V2/V2023 template constants and related compile functions/tests.
Config & toml
config.go, config_test.go, velox.toml
Migrated config to Plugins map[string]*Plugin, added TargetPlatform, GitHub/Token types; removed GitLab/CodeHosting; tests updated; velox.toml converted to [plugins.*] with tag and module_name.
CLI build flow
internal/cli/build/build.go, internal/cli/server/server.go, internal/cli/root.go
CLI now constructs []*plugin.Plugin via plugin.NewPlugin, creates cached GitHub HTTP client (uuid-based template id), uses WithPlugins, WithGOOS/GOARCH, and logs returned binary path; root uses generic pointer helper.
Hosting & fetch removal
github/repo.go, github/pool.go, gitlab/repo.go, github/parse_test.go
Removed GitHub/GitLab repo fetchers, worker pool, module-info aggregation, and parse tests—eliminates repo download/zip/extract and plugin go.mod retrieval logic.
Modules info removed
modulesInfo.go
Removed ModulesInfo, ParseModuleInfo, IsDigit and associated regex/logic.
v2 legacy removal
v2/builder/*, v2/builder/templates/*, v2/config/builder.go
Deleted v2 builder, options, templates and builder config structs/constants.
Dependency & misc edits
go.mod, github/github.go, github/github_test.go, update_plugins.py, .github/workflows/linux.yml
Updated dependencies (added uuid, pruned GH/GitLab/x/mod deps), minor import/path fixes, update_plugins.py now writes plugins.*.tag, workflow env renamed to GITHUB_TOKEN.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

C-enhancement

Suggested reviewers

  • wolfy-j

Poem

I thump my paws on fresher ground,
Plugins gather, tidy and round.
Tags hop in and UUIDs hum,
Templates stitch — the build's begun.
Out pops rr, ready and sound. 🐇✨

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
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/v2

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rustatian rustatian added the B-Breaking changes Breaking changes label Aug 17, 2025
@rustatian rustatian requested a review from Copilot August 17, 2025 19:40
@rustatian rustatian moved this to 🏗 In progress in Jira 😄 Aug 17, 2025
Copy link
Contributor

Copilot AI left a 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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() assumes Get returns 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 reading

You write to the zip file and immediately open it via zip.OpenReader while 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.Mutex

If 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 invocation

Currently, -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 simplification

You remove dest, then Mkdir on rrSaveDest. Given dest is Abs(rrSaveDest), this sequence duplicates earlier RemoveAll/MkdirAll work 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 against dest and can yield a path unrelated to the extraction root. You already have a robust Zip Slip check inside extract. 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, check rc.File[0].Name directly for .. without Abs.

github/github_test.go (2)

19-21: Use t.TempDir() for isolation and automatic cleanup

Using os.TempDir() can leave artifacts behind on successful runs and risks collisions. Prefer t.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 integration

This test calls out to GitHub and accepts errors, so it doesn’t assert behavior and is flaky/offline-hostile. Consider stubbing with httptest.Server to 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 build will 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 template

Also 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 = outputFile
config_test.go (2)

57-59: Avoid calling Validate() twice; assert on the captured error

Minor 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.Getwd

Prefer 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 builds

Iterating 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 RunE

Prefer 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 exiting

Returning 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 messages

Current 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 check

os.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 copy

The 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 change

Add 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5b3990b and 0efa236.

⛔ Files ignored due to path filters (1)
  • go.sum is 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 good

Clear, concise package doc that explains the purpose. No action needed.


233-250: Ambiguous handling of “latest” refs

parseRRref treats anything not prefixed with v and not 40 chars as a branch, which makes latest resolve 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 consistent

Matches the repo’s new layout and the plugin-centric refactor. No issues spotted.

go.mod (1)

12-12: No stale references to removed dependencies found

I ran broad searches across the codebase for imports or references to github.com/google/go-github, gitlab.com/gitlab-org/api, and golang.org/x/mod and found no occurrences. The dependency surface is clean.

builder/templates/templateV2025.go (3)

4-8: Go directive/toolchain updates look fine

Using go 1.25 and toolchain pin is consistent with the repo. No issues here.


40-42: Import section generation looks correct

Iterating .Imports is simple and flexible. Assuming p.Imports() returns fully-formed import lines ("module/path"), this is good.


53-55: Plugins slice generation aligns with p.Code() contract

&{{$v}}, implies p.Code() returns pkg.Plugin{} (no ampersand). Ensure this invariant is documented/enforced in plugin.Plugin. Otherwise, this will double-ampersand or miss one.

Confirm plugin.Plugin.Code() returns something like informer.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 defaults

Defaulting 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 explicitly

Using ${GITHUB_TOKEN} and specifying a target platform is consistent with the new config model and builder flow.


19-150: LGTM: unified plugin schema

Per-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]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 failures

Requests 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:

  1. Imports
-import requests
-import toml
+import os
+import requests
+import toml
+from typing import Dict
  1. 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
  1. 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 gracefully

Exiting 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.8

dict[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 Releases

Some 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 flexibility

I 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’s module_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, repo

This 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0efa236 and 20fc5f9.

📒 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]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 deref

You declare var cfg *velox.Config and then call v.Unmarshal(&cfg). That’s a pointer-to-pointer target; Viper/mapstructure expects a pointer to a concrete struct. As written, cfg may remain nil, leading to a panic at Line 63 (*cfg) or a silently empty config.

Minimal, safe fix:

  • Allocate cfg before 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 = *cfg

Alternatively, make cfg a value: var cfg velox.Config, then v.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 effect

You build a named logger at command construction time: lg.Named("builder"). Later in PersistentPreRunE, you rebuild the logger and try to update it via *lg = *zlog. However, the previously created Named child 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 lg instead; 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 have outputFile, point to it directly. This simplifies ownership and makes intent obvious.

Apply this diff:

-        cfgPath      = p("")
+        cfgPath      = &outputFile

With this, *cfgPath = outputFile at Line 64 becomes redundant and can be removed for clarity.


92-94: Remove the generic pointer helper p[T any] (unused after refactor)

Once cfgPath points 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 default

You default --config to velox.toml (Line 81), so pathToConfig == "" 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 error

Currently 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 20fc5f9 and 4739a60.

📒 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 addition

Package-level documentation is clear and helpful.


27-27: Initializing config to a non-nil value is good

Starting with config = &velox.Config{} avoids nil handling throughout command binding.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 nit

Consider 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 empty

If 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 idea

The 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 defaulting

See 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” semantics

Currently 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4739a60 and 0a764cc.

📒 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 mismatch

CI 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 good

New fields (TargetPlatform, GitHub, Plugins) align with the simplified v2 schema and are idiomatic with mapstructure tags.


43-45: GitHub token container is fine

Minimal GitHub block with a Token wrapper keeps room for future fields without breaking schema.


47-49: Token struct: LGTM

Simple wrapper suited for env expansion and future metadata.


7-7: runtime import: LGTM

Used for TargetPlatform defaults; no issues.

@rustatian rustatian changed the title feature: velox v2 feature: velox v2 (v2025.2) Aug 18, 2025
Signed-off-by: Valery Piashchynski <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 context

The 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 sentence

Go 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/errors

You'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 values

To 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 vars

os.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.toml uses 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 Token type, 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0a764cc and 545112f.

📒 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 LGTM

TargetPlatform, GitHub, and Plugins fields are clear and align with the new v2 config direction.


51-54: Plugin schema LGTM

module_name and tag match the pb schema and the new plugin-centric flow.


65-69: Sane defaults for TargetPlatform

Defaulting to runtime.GOOS/GOARCH is a good UX.


77-79: Good guard for missing plugins

Early error on nil plugins map improves UX.

Signed-off-by: Valery Piashchynski <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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: read

Please 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 545112f and a5f8562.

📒 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)

@rustatian rustatian merged commit eb72d37 into master Aug 18, 2025
7 checks passed
@rustatian rustatian deleted the feature/v2 branch August 18, 2025 16:08
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jira 😄 Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-Breaking changes Breaking changes

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants