feat(run): add support for custom runner defined in gox.mod#2517
feat(run): add support for custom runner defined in gox.mod#2517joeykchen wants to merge 2 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello @joeykchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a flexible mechanism for projects to define and execute custom runtime environments. By allowing a 'run' directive in 'gop.mod', 'xgo run' can now delegate execution to a specified external command, enabling specialized workflows like those required by SPX and other classfile projects. This enhances the extensibility of the 'xgo' tool without altering its core 'run' logic for projects without a custom runner. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to support custom runners defined in gop.mod. While the implementation is a good start for local projects, I've identified several issues that need addressing. My review highlights a critical flaw in handling remote packages (PkgPathProj) which renders the feature incomplete for that use case. Additionally, there's a bug related to loading configuration from the wrong directory, fragile logic for installing and locating the custom runner binary, and a concern about build reproducibility due to fetching the @latest version of the runner. I've provided specific comments and code suggestions to resolve these issues.
cmd/internal/run/run.go
Outdated
| if absDir, err := filepath.Abs(projDir); err == nil { | ||
| projDir = absDir | ||
| } |
There was a problem hiding this comment.
The error from filepath.Abs is silently ignored. If filepath.Abs fails (for example, if os.Getwd() fails), projDir will remain a relative path. This could lead to unexpected behavior later, as subsequent logic might rely on an absolute path. It's safer to handle this potential error, at least by logging it.
if absDir, err := filepath.Abs(projDir); err == nil {
projDir = absDir
} else {
log.Warnln("failed to get absolute path for", projDir, ":", err)
}
cmd/internal/run/run.go
Outdated
| if _, err := os.Stat(cmdPath); os.IsNotExist(err) { | ||
| // Try to install the command | ||
| fmt.Printf("Installing custom runner: %s\n", cmdPkgPath) | ||
| installCmd := exec.Command("go", "install", cmdPkgPath+"@latest") |
There was a problem hiding this comment.
The custom runner is installed using go install <pkg>@latest. This can lead to non-reproducible builds, as running the same command at different times might fetch different versions of the runner, potentially with breaking changes. This also introduces a potential security risk if a new malicious version is published. It is recommended to remove @latest and let go install resolve the version based on the project's dependencies, which provides more stable and secure behavior.
installCmd := exec.Command("go", "install", cmdPkgPath)![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
cmd/internal/run/run.go
Outdated
| func runCustomRunner(cmdPkgPath, projDir string, args []string) error { | ||
| // Extract the command name from package path | ||
| // e.g., "github.com/goplus/spx/v2/cmd/spxrun" -> "spxrun" | ||
| cmdName := filepath.Base(cmdPkgPath) |
There was a problem hiding this comment.
Security: Command injection vulnerability
cmdPkgPath comes from user-controlled gop.mod file without validation. Malicious values like "../../../evil/cmd" or "cmd;malicious" could lead to executing unintended binaries.
Recommendation: Validate package path format:
func validatePackagePath(pkgPath string) error {
// Valid Go package path pattern
validPattern := `^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*(/[a-zA-Z0-9._-]+)+$`
matched, _ := regexp.MatchString(validPattern, pkgPath)
if !matched {
return fmt.Errorf("invalid package path format: %s", pkgPath)
}
// Reject dangerous characters
if strings.ContainsAny(pkgPath, ";|&`$\n\r") {
return fmt.Errorf("package path contains forbidden characters")
}
return nil
}Call before using cmdPkgPath.
cmd/internal/run/run.go
Outdated
| // Try GOPATH/bin | ||
| gopath := os.Getenv("GOPATH") | ||
| if gopath == "" { | ||
| gopath = filepath.Join(os.Getenv("HOME"), "go") |
There was a problem hiding this comment.
Cross-platform issue: Windows compatibility
Using $HOME environment variable doesn't work on Windows. Use os.UserHomeDir() instead:
userHome, err := os.UserHomeDir()
if err != nil {
return fmt.Errorf("failed to determine user home directory: %w", err)
}
gopath = filepath.Join(userHome, "go")Also consider that Windows binaries have .exe extension.
cmd/internal/run/run.go
Outdated
| mod, err := xgomod.Load(projDir) | ||
| if err != nil { | ||
| return "" |
There was a problem hiding this comment.
Error handling: Silent failure obscures issues
Returning empty string on any error makes debugging difficult. Users won't know if their gop.mod is malformed vs. simply missing.
Suggestion: Log errors in debug mode:
mod, err := xgomod.Load(projDir)
if err != nil {
if *flagDebug {
log.Printf("Failed to load gop.mod from %s: %v", projDir, err)
}
return ""
}
cmd/internal/run/run.go
Outdated
| // Build the command arguments | ||
| cmdArgs := []string{"--path", projDir} | ||
| cmdArgs = append(cmdArgs, args...) |
There was a problem hiding this comment.
Documentation: Undocumented CLI contract
The --path flag convention is hardcoded but not documented. Custom runners MUST implement this interface, which should be clearly specified.
Suggestion: Update the runCustomRunner function documentation to specify:
- Custom runners must accept
--path <dir>flag - Additional args are passed through
- Working directory is set to
projDir - stdin/stdout/stderr are inherited
cmd/internal/run/run.go
Outdated
| // getCustomRunner checks if the target project directory has a custom runner | ||
| // defined in its gop.mod file. Returns the runner command package path or empty string. |
There was a problem hiding this comment.
Documentation: Missing gop.mod format specification
The documentation doesn't explain the expected gop.mod structure or provide an example.
Suggestion: Enhance documentation:
// getCustomRunner checks if the target project directory has a custom runner
// defined in its gop.mod file. It looks for the opt.run.cmd field containing
// a Go package import path.
//
// Example gop.mod:
// [opt.run]
// cmd = "github.com/goplus/spx/v2/cmd/spxrun"
//
// Returns the runner command package path (Go import path) or empty string if
// no custom runner is configured or if the gop.mod file cannot be loaded.
Code Review SummaryThis PR adds a useful feature for custom runners, but has critical issues that must be fixed before merging: Critical Issues (Must Fix)
Important Issues (Should Fix)
The implementation shows good code structure, but the security concerns and critical bug need immediate attention. Please address the inline comments before merging. |
joeykchen
force-pushed
the
feature/add-run-directive
branch
3 times, most recently
from
5753165 to
d9b7c24
Compare
Fix:builds a Go command package if not found feat(run): add support for custom runner defined in gop.mod Add support for 'run' directive in gop.mod files that allows projects to specify a custom runner command. When 'xgo run' is executed and the project has a 'run' directive, it will invoke the specified runner instead of the standard Go run. Example gop.mod: run github.com/goplus/spx/v2/cmd/spxrun Changes: - Add getCustomRunner() function that loads the target project's gop.mod using xgomod.Load() to check for run directive (minimal impact on existing xgo logic) - Add runCustomRunner() function that: - Extracts command name from package path - Looks up the command in PATH and GOPATH/bin - Auto-installs the command if not found - Passes project directory and arguments to the runner This enables SPX and other classfile projects to define their own specialized run workflows.
joeykchen
force-pushed
the
feature/add-run-directive
branch
from
d9b7c24 to
32e31db
Compare
xushiwei
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2517 +/- ##
=======================================
Coverage 93.98% 93.98%
=======================================
Files 34 34
Lines 9613 9613
=======================================
Hits 9035 9035
Misses 413 413
Partials 165 165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Add support for 'run' directive in gop.mod files that allows projects to specify a custom runner command. When 'xgo run' is executed and the project has a 'run' directive, it will invoke the specified runner instead of the standard Go run.
Example gop.mod:
run github.com/goplus/spx/v2/cmd/spxrun
Changes:
This enables SPX and other classfile projects to define their own specialized run workflows.