[draft] DO NOT MERGE: rayapp test prototype

[elliot-barn]

Adding anyscale CLI wrapper and test workflow to test templates in anyscale

Add a pre-flight check to verify anyscale is installed before running commands, returning a clear error if not. Include comprehensive tests using mock scripts to test success, failure, and not-installed scenarios.

[gemini-code-assist]

Summary of Changes

Hello @elliot-barn, I'm Gemini Code Assist¹! 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 integrates the Anyscale CLI into the rayapp tool by introducing a new subcommand. This enhancement allows rayapp to directly invoke Anyscale CLI commands, complete with pre-execution checks for the Anyscale CLI's presence and robust error handling. The changes aim to streamline workflows that involve both rayapp and the Anyscale platform, ensuring a smoother user experience with clear feedback.

Highlights

• Anyscale CLI Wrapper: Introduced a new anyscale-cli command within rayapp to execute Anyscale CLI commands directly.
• Pre-flight Installation Check: Implemented a check to verify if the anyscale CLI is installed on the system before attempting to run any commands, providing a clear error if it's missing.
• Robust Error Handling: Enhanced error reporting for Anyscale CLI execution, capturing both command-specific errors and standard error output.
• Comprehensive Testing: Added extensive unit tests using mock scripts to cover various scenarios, including successful command execution, command failures with stderr, and cases where the Anyscale CLI is not installed.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a wrapper for the anyscale CLI, including a pre-flight check for its installation and comprehensive tests. The implementation is solid, but I have a few suggestions to improve code quality and robustness. My review includes recommendations for better error handling in anyscale_cli.go, improved path construction and test logic in anyscale_cli_test.go, and fixing missing newlines at the end of files.

Additionally, there are two important points that are outside the changed lines but are highly relevant:

1. The application will panic if run without any command-line arguments. You should add a check for len(args) in rayapp/main.go before accessing args[0].
2. The new anyscale-cli command is missing from the usage message printed by the help command. Please update it to reflect the new functionality.

[gemini-code-assist]

The error formatting could be improved for better diagnostics and to follow modern Go practices. Using %w to wrap the error allows for inspection of the error chain with errors.Is or errors.As. Also, including a newline \n in the error message can make logs harder to parse. It's generally better to have single-line error messages.

Suggested change

	return "", fmt.Errorf("anyscale error: %v\nstderr: %s", err, stderr.String())
	return "", fmt.Errorf("anyscale error: %w, stderr: %s", err, stderr.String())

[gemini-code-assist]

The file should end with a newline character. This is a common convention and is enforced by many editors and tools to prevent issues with file concatenation and processing.

[gemini-code-assist]

For constructing file paths, it's better to use filepath.Join instead of manual string concatenation with /. This makes the code more portable and works correctly on different operating systems, like Windows. You'll need to add import "path/filepath" to the file.

Suggested change

	mockScript := tmp + "/anyscale"
	mockScript := filepath.Join(tmp, "anyscale")

[gemini-code-assist]

The error checking logic is a bit complex and can be simplified for better readability and maintainability. Specifically, errors.Is(tt.wantErr, errAnyscaleNotInstalled) is an unconventional way to check if tt.wantErr is of a certain type. A direct comparison tt.wantErr == errAnyscaleNotInstalled is more idiomatic here.

Suggested change

	if errors.Is(tt.wantErr, errAnyscaleNotInstalled) {
	if !errors.Is(err, errAnyscaleNotInstalled) {
	t.Errorf("expected errAnyscaleNotInstalled, got: %v", err)
	}
	} else if !strings.Contains(err.Error(), tt.wantErr.Error()) {
	t.Errorf("error %q should contain %q", err.Error(), tt.wantErr.Error())
	}
	if tt.wantErr == errAnyscaleNotInstalled {
	if !errors.Is(err, errAnyscaleNotInstalled) {
	t.Errorf("expected errAnyscaleNotInstalled, got: %v", err)
	}
	} else if !strings.Contains(err.Error(), tt.wantErr.Error()) {
	t.Errorf("error %q should contain %q", err.Error(), tt.wantErr.Error())
	}

[gemini-code-assist]

The file should end with a newline character. This is a common convention and is enforced by many editors and tools to prevent issues with file concatenation and processing.

[andrew-anyscale]

Does this need reviewing? 👁️ 👁️

[gitar-bot]

I've addressed the code review findings:

Fixed Issues:

1. rayapp/anyscale_cli.go:89 - Fixed hardcoded compute config to use config.computeConfig instead of hardcoded value
2. rayapp/anyscale_cli.go:58 - Made RunAnyscaleCLI method public (was runAnyscaleCLI) to match test expectations
3. rayapp/anyscale_cli_test.go:82 - Fixed test to check output instead of err.Error() for stderr content
4. rayapp/anyscale_cli_test.go:112 - Added missing newline at end of file
5. rayapp/test.go:93 - Fixed missing comment marker (was terminate workspace, now // terminate workspace)
6. rayapp/test.go:94 - Fixed undefined variable tr to wtc
7. go.mod:3 - Updated Go version from 1.25.2 (invalid) to 1.24 to match dependencies

Verification:

• All anyscale CLI tests pass ✅
• Project builds successfully ✅

[gitar-bot]

⚠️ Edge Case: Workspace leaked on test failure

Details

If copyTemplateToWorkspace or runCmdInWorkspace fail, the function returns immediately without calling terminateWorkspace. This leaves the workspace running, consuming resources and potentially costing money.

Impact: Failed tests will leave orphaned workspaces that must be manually cleaned up.

Suggested fix: Use defer for cleanup:

// After successful workspace creation
defer func() {
    if err := anyscaleCLI.terminateWorkspace(wtc.workspaceName); err != nil {
        fmt.Printf("warning: failed to terminate workspace: %v\n", err)
    }
}()

---

Was this helpful? React with 👍 / 👎

[gitar-bot]

I've addressed the critical bugs found in the code review:

Fixed:

1. go.mod:3 - Updated invalid Go version from 1.25.2 to 1.24
2. anyscale_cli.go:89 - Fixed hardcoded compute config value to use config.computeConfig
3. anyscale_cli.go:58 - Made RunAnyscaleCLI method public (was runAnyscaleCLI) to match test expectations
4. anyscale_cli_test.go:82 - Fixed test to check output instead of err.Error() for stderr content
5. test.go:94 - Fixed undefined variable tr to wtc
6. anyscale_cli.go - Added missing newline at end of file

Verification:

• ✅ All anyscale CLI tests pass
• ✅ Project builds successfully

[elliot-barn]

Does this need reviewing? 👁️ 👁️

no sir this is for prototyping. Will clean it up and then break it down into smaller PRs

[gitar-bot]

Code Review 👍 Approved with suggestions 10 resolved / 13 findings

Prototype PR with 3 pre-existing issues still present: double close of zip.Writer, workspace leaked on test failure, and help text documenting non-existent flags. Author acknowledges this is for prototyping and will be cleaned up.

⚠️ Edge Case: Workspace leaked on test failure

📄 rayapp/test.go:77 📄 rayapp/test.go:82 📄 rayapp/test.go:87

If copyTemplateToWorkspace or runCmdInWorkspace fail, the function returns immediately without calling terminateWorkspace. This leaves the workspace running, consuming resources and potentially costing money.

Impact: Failed tests will leave orphaned workspaces that must be manually cleaned up.

Suggested fix: Use defer for cleanup:

// After successful workspace creation
defer func() {
    if err := anyscaleCLI.terminateWorkspace(wtc.workspaceName); err != nil {
        fmt.Printf("warning: failed to terminate workspace: %v\n", err)
    }
}()

💡 Quality: Help text documents flags that don't exist

📄 rayapp/rayapp/main.go:82 📄 rayapp/rayapp/main.go:29 📄 rayapp/rayapp/main.go:31 📄 rayapp/rayapp/main.go:56

The printUsage() function shows test flags --workspace, --template-dir, and --config that don't exist in the actual flag definitions. The testFlags FlagSet only defines --build. This creates user confusion when they try to use the documented flags.

Impact: Users will get errors when trying to use documented flags.

Suggested fix: Either implement the documented flags or update the help text to match the actual implementation:

fmt.Println("Test flags:")
fmt.Println("  --build string     Build file (default \"BUILD.yaml\")")

💡 Bug: Double close of zip.Writer in zipDirectory

📄 rayapp/util.go:101 📄 rayapp/util.go:124

The zipDirectory function has both defer z.Close() (line 18) and an explicit z.Close() call (line 41). This results in the zip writer being closed twice.

While zip.Writer.Close() is idempotent and won't cause a runtime error, this is an incorrect code pattern that can mask real errors. If the explicit Close() fails, the deferred Close() will run but its error will be ignored.

Suggested fix: Remove the defer z.Close() line since the explicit close with error handling is the correct approach:

z := zip.NewWriter(outFile)
// Remove: defer z.Close()

err = filepath.Walk(...)

Alternatively, remove the explicit z.Close() and let the defer handle it, but then you lose error handling on close.

✅ 10 resolved

✅ Bug: Hardcoded compute config ignores user-provided value

📄 rayapp/anyscale_cli.go:89
In createEmptyWorkspace, when config.computeConfig is not empty, the code appends a hardcoded value "tmpl-test-basic-serverless-aws:1" instead of using the actual config.computeConfig value.

Current code:

if config.computeConfig != "" {
    args = append(args, "--compute-config", "tmpl-test-basic-serverless-aws:1")
}

Expected:

if config.computeConfig != "" {
    args = append(args, "--compute-config", config.computeConfig)
}

This defeats the purpose of having a configurable compute config field.

✅ Bug: Undefined variable `tr` causes compilation failure

📄 rayapp/test.go:98
Line 98 uses tr.workspaceName but tr is not defined anywhere in this function. The variable should be wtc.workspaceName to match the receiver of the method.

This will cause a build failure with:

undefined: tr

Fix:

if err := anyscaleCLI.terminateWorkspace(wtc.workspaceName); err != nil {

✅ Edge Case: Infinite loop if workspace never reaches RUNNING state

📄 rayapp/test.go:67
The workspace status polling loop in test.go has no timeout or maximum retry count. If the workspace gets stuck in STARTING state, fails to start, or enters an unexpected state, this loop will run forever.

Impact: The test command can hang indefinitely waiting for a workspace that will never become ready.

Suggested fix: Add a timeout or maximum retry count:

maxRetries := 60 // 30 minutes with 30s sleep
for i := 0; !strings.Contains(state, StateRunning.String()); i++ {
    if i >= maxRetries {
        return fmt.Errorf("workspace %s did not reach RUNNING state after %d attempts", wtc.workspaceName, maxRetries)
    }
    // ... rest of polling logic
}

✅ Quality: Tests modify global PATH without parallel safety

📄 rayapp/anyscale_cli_test.go:37 📄 rayapp/anyscale_cli_test.go:71
The setupMockAnyscale function modifies the global PATH environment variable. While it uses t.Cleanup to restore the original value, running tests with t.Parallel() would cause race conditions since environment variables are process-global.

Impact: Tests cannot safely run in parallel, which slows down test suite execution.

Suggested fix: This is acceptable for now but should be documented. Consider using a more isolated approach in the future, such as injecting the command executor as a dependency.

✅ Bug: Nil pointer dereference if template not found

📄 rayapp/test.go:47
In test.go, after iterating through templates to find a matching one, there's no check whether wtc.template was actually found before using it. If the template name doesn't match any template in the build file, wtc.template remains nil, and the code will panic when accessing wtc.template.ClusterEnv.BuildID in createEmptyWorkspace().

Impact: Runtime panic if an invalid template name is provided.

Suggested fix:

for _, tmpl := range tmpls {
    if tmpl.Name == wtc.tmplName {
        wtc.template = tmpl
        break
    }
}

if wtc.template == nil {
    return fmt.Errorf("template %q not found in %s", wtc.tmplName, wtc.buildFile)
}

...and 5 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}