ronantakizawa/codereview-bench huggingface dataset:
Repository: google/gvisor
File: runsc/cmd/pidfile.go (Go)

Code before:
package cmd

import (
	"fmt"
	"os"
	"path/filepath"
	"strconv"
)

// WritePidFile writes pid file atomically if possible.
func WritePidFile(path string, pid int) error {
	pidStr := []byte(strconv.Itoa(pid))

	_, err := os.Stat(path)
	if err != nil && !os.IsNotExist(err) {
		// If path exists, write in place, because file could be pipe or something.
		if err := os.WriteFile(path, pidStr, 0644); err != nil {
			return fmt.Errorf("failed to write pid file %s: %w", path, err)
		}
		return nil
	} else if err != nil {
		return fmt.Errorf("stat file %s failed: %w", path, err)
	}

	// Otherwise write using temp file to make write atomic.
	dir := filepath.Dir(path)
	tempFile, err := os.CreateTemp(dir, "pid-tmp-*")
	if err != nil {
		return fmt.Errorf("failed to create temp pid file in dir %s: %w", dir, err)
	}

	tempFileRenamed := false
	defer func(tempFile *os.File) {
		_ = tempFile.Close()
		if !tempFileRenamed {
			_ = os.Remove(tempFile.Name())
		}
	}(tempFile)

	if err := os.Chmod(tempFile.Name(), 0644); err != nil {
		return fmt.Errorf("failed to chmod pid file %s: %w", tempFile.Name(), err)
	}

	if _, err := tempFile.Write(pidStr); err != nil {
		return fmt.Errorf("failed to write pid file %s: %w", tempFile.Name(), err)
	}

Review comment (suggestion):
The block ends with `return` now, so replace `} else if err != nil {` with

```
}
if err != nil {
```

Code after:
// Copyright 2025 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package cmd

import (
	"fmt"
	"os"
	"path/filepath"
	"strconv"
)

// WritePidFile writes pid file atomically if possible.
func WritePidFile(path string, pid int) error {
	pidStr := []byte(strconv.Itoa(pid))

	st, err := os.Stat(path)
	if err == nil && !st.Mode().IsRegular() {
		// If not regular file, write in place.
		if err := os.WriteFile(path, pidStr, 0644); err != nil {
			return fmt.Errorf("failed to write pid file %s: %w", path, err)
		}
		return nil
	}
	if err != nil && !os.IsNotExist(err) {
		return fmt.Errorf("stat file %s failed: %w", path, err)
	}

	// Otherwise write using temp file to make write atomic.
	dir := filepath.Dir(path)
	tempFile, err := os.CreateTemp(dir, "pid-tmp-*")
	if err != nil {
		return fmt.Errorf("failed to create temp pid file in dir %s: %w", dir, err)
	}
Repository: google/gvisor
File: runsc/cmd/pidfile.go (Go)

Code before:
package cmd

import (
	"fmt"
	"os"
	"path/filepath"
	"strconv"
)

// WritePidFile writes pid file atomically if possible.
func WritePidFile(path string, pid int) error {
	pidStr := []byte(strconv.Itoa(pid))

	_, err := os.Stat(path)
	if err != nil && !os.IsNotExist(err) {
		// If path exists, write in place, because file could be pipe or something.
		if err := os.WriteFile(path, pidStr, 0644); err != nil {
			return fmt.Errorf("failed to write pid file %s: %w", path, err)
		}
		return nil
	}
	if err != nil {
		return fmt.Errorf("stat file %s failed: %w", path, err)
	}

	// Otherwise write using temp file to make write atomic.
	dir := filepath.Dir(path)
	tempFile, err := os.CreateTemp(dir, "pid-tmp-*")
	if err != nil {
		return fmt.Errorf("failed to create temp pid file in dir %s: %w", dir, err)
	}

	tempFileRenamed := false
	defer func(tempFile *os.File) {
		_ = tempFile.Close()
		if !tempFileRenamed {
			_ = os.Remove(tempFile.Name())
		}
	}(tempFile)

	if err := os.Chmod(tempFile.Name(), 0644); err != nil {

Review comment (suggestion):
@nixprime makes the good point that you can determine whether a file is regular using the stat data itself, using `st.Mode().IsRegular()`. This would be a more robust way of determining whether you should do a rename vs just writing. So:
- If `stat` successful && regular file: rename
- If `stat` unsuccessful && `ENOENT`: also rename
- If `stat` successful && not regular file: write in place
- If `stat` unsuccessful && not `ENOENT`: return the error from stat

Code after:
// Copyright 2025 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package cmd

import (
	"fmt"
	"os"
	"path/filepath"
	"strconv"
)

// WritePidFile writes pid file atomically if possible.
func WritePidFile(path string, pid int) error {
	pidStr := []byte(strconv.Itoa(pid))

	st, err := os.Stat(path)
	if err == nil && !st.Mode().IsRegular() {
		// If not regular file, write in place.
		if err := os.WriteFile(path, pidStr, 0644); err != nil {
			return fmt.Errorf("failed to write pid file %s: %w", path, err)
		}
		return nil
	}
	if err != nil && !os.IsNotExist(err) {
		return fmt.Errorf("stat file %s failed: %w", path, err)
	}

	// Otherwise write using temp file to make write atomic.
	dir := filepath.Dir(path)
Repository: google/gvisor
File: pkg/sentry/control/fs.go (Go)

Code before:
	})
	if mntns == nil {
		return nil, fmt.Errorf("mount namespace is nil for %s", containerID)
	}
	return mntns, nil
}

// TarRootfsUpperLayer is a RPC stub which serializes the rootfs upper layer to
// a tar file. When the rootfs is not an overlayfs, it returns an error.
func (f *Fs) TarRootfsUpperLayer(o *TarRootfsUpperLayerOpts, _ *struct{}) error {
	if len(o.FilePayload.Files) != 1 {
		return ErrInvalidFiles
	}
	outFD := o.FilePayload.Files[0]
	defer outFD.Close()

	ctx := f.Kernel.SupervisorContext()
	mntns, err := f.mountNamespaceForContainer(o.ContainerID)
	if err != nil {
		return err
	}
	defer mntns.DecRef(ctx)

	root := mntns.Root(ctx)
	defer root.DecRef(ctx)
	ts, ok := root.Mount().Filesystem().Impl().(vfs.TarSerializer)
	if !ok {
		return fmt.Errorf("rootfs is not an overlayfs")
	}
	if err := ts.TarUpperLayer(ctx, outFD); err != nil {
		return fmt.Errorf("failed to serialize rootfs upper layer to tar: %v", err)
	}
	return nil
}

// CatOpts contains options for the Cat RPC call.
type CatOpts struct {
	// Files are the filesystem paths for the files to cat.
	Files []string `json:"files"`

	// FilePayload contains the destination for output.
	urpc.FilePayload
}

// Cat is a RPC stub which prints out and returns the content of the files.
func (f *Fs) Cat(o *CatOpts, _ *struct{}) error {
	// Create an output stream.
	if len(o.FilePayload.Files) != 1 {
		return ErrInvalidFiles
	}


Review comment (bug):
Ah thanks for fixing this bug! I think we were leaking `mntns.Root(ctx)` ref earlier.

Code after:
		mntns = leader.GetMountNamespace()
	})
	if mntns == nil {
		return nil, fmt.Errorf("could not find any tasks for %s", containerID)
	}
	return mntns, nil
}

// TarRootfsUpperLayer is a RPC stub which serializes the rootfs upper layer to
// a tar file. When the rootfs is not an overlayfs, it returns an error.
func (f *Fs) TarRootfsUpperLayer(o *TarRootfsUpperLayerOpts, _ *struct{}) error {
	if len(o.FilePayload.Files) != 1 {
		return ErrInvalidFiles
	}
	outFD := o.FilePayload.Files[0]
	defer outFD.Close()

	ctx := f.Kernel.SupervisorContext()
	mntns, err := f.mountNamespaceForContainer(o.ContainerID)
	if err != nil {
		return err
	}
	defer mntns.DecRef(ctx)

	root := mntns.Root(ctx)
	defer root.DecRef(ctx)
	ts, ok := root.Mount().Filesystem().Impl().(vfs.TarSerializer)
	if !ok {
		return fmt.Errorf("rootfs is not an overlayfs")
	}
	if err := ts.TarUpperLayer(ctx, outFD); err != nil {
		return fmt.Errorf("failed to serialize rootfs upper layer to tar: %v", err)
	}
	return nil
}

// CatOpts contains options for the Cat RPC call.
type CatOpts struct {
	// Files are the filesystem paths for the files to cat.
	Files []string `json:"files"`

	// FilePayload contains the destination for output.
	urpc.FilePayload
}

// Cat is a RPC stub which prints out and returns the content of the files.
func (f *Fs) Cat(o *CatOpts, _ *struct{}) error {
	// Create an output stream.
	if len(o.FilePayload.Files) != 1 {
		return ErrInvalidFiles
	}
Repository: google/gvisor
File: runsc/cmd/tar.go (Go)

Code before:
// Execute implements subcommands.Command.
func (r *RootfsUpper) Execute(ctx context.Context, f *flag.FlagSet, args ...any) subcommands.ExitStatus {
	if f.NArg() != 1 {
		util.Fatalf("container id must be provided")
	}

	id := f.Arg(0)
	conf := args[0].(*config.Config)

	c, err := container.Load(conf.RootDir, container.FullID{ContainerID: id}, container.LoadOpts{})
	if err != nil {
		util.Fatalf("error loading container: %v", err)
	}

	util.Infof("Serializing rootfs upper layer into a tar archive for container: %s, sandbox: %s", id, c.Sandbox.ID)

	out := os.Stdout
	if r.file != "" {
		out, err = os.OpenFile(r.file, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
		if err != nil {
			util.Fatalf("failed to open output file: %v", err)
		}
		defer out.Close()
	}

	if err := c.Sandbox.TarRootfsUpperLayer(id, out); err != nil {
		util.Fatalf("TarRootfsUpperLayer failed: %v", err)
	}
	return subcommands.ExitSuccess
}


Review comment (suggestion):
I think we should also define `TarRootfsUpperLayer()` method on `Container`, which would then call `c.Sandbox.TarRootfsUpperLayer(c.ID, out)`. That would be cleaner. Then in the tests you can simply do `containers[0].TarRootfsUpperLayer(rootTar)` instead of `containers[0].Sandbox.TarRootfsUpperLayer(containers[0].ID, rootTar)`

Code after:
// Execute implements subcommands.Command.
func (r *RootfsUpper) Execute(ctx context.Context, f *flag.FlagSet, args ...any) subcommands.ExitStatus {
	if f.NArg() != 1 {
		util.Fatalf("container id must be provided")
	}

	id := f.Arg(0)
	conf := args[0].(*config.Config)

	c, err := container.Load(conf.RootDir, container.FullID{ContainerID: id}, container.LoadOpts{})
	if err != nil {
		util.Fatalf("error loading container: %v", err)
	}

	util.Infof("Serializing rootfs upper layer into a tar archive for container: %s, sandbox: %s", id, c.Sandbox.ID)

	out := os.Stdout
	if r.file != "" {
		out, err = os.OpenFile(r.file, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
		if err != nil {
			util.Fatalf("failed to open output file: %v", err)
		}
		defer out.Close()
	}

	if err := c.TarRootfsUpperLayer(out); err != nil {
		util.Fatalf("TarRootfsUpperLayer failed: %v", err)
	}
	return subcommands.ExitSuccess
}
Repository: google/gvisor
File: runsc/specutils/specutils.go (Go)

Code before:
				}
				if parts[0] == name {
					name = parts[1]
					break
				}
			}
		}
	}
	return name
}

func containerNameNoRemap(spec *specs.Spec) string {
	return spec.Annotations[annotationContainerName]
}

// RootfsTarUpperPath returns the path to the rootfs upper tar file.
// Prioritizes container-specific upper rootfs snapshot annotation.
// Returns empty string if no annotation is found.
func RootfsTarUpperPath(spec *specs.Spec) string {
	if spec == nil || spec.Annotations == nil {
		return ""
	}
	if name := ContainerName(spec); name != "" {
		if path := spec.Annotations[annotationRootfsUpperTarContainer+name]; path != "" {
			return path
		}
	}
	return spec.Annotations[annotationRootfsUpperTar]
}

// AnnotationToBool parses the annotation value as a bool. On failure, it logs a warning and
// returns false.
func AnnotationToBool(spec *specs.Spec, annotation string) bool {
	val, ok := spec.Annotations[annotation]
	if !ok {
		return false
	}
	ret, err := strconv.ParseBool(val)
	if err != nil {
		log.Warningf("Failed to parse annotation %q=%q as a bool: %v", annotation, val, err)
		return false
	}
	return ret
}


Review comment (suggestion):
Hmm in the multi-container case, there should be no fallback IMO. As you point out in this PR, the `dev.gvisor.tar.rootfs.upper` annotation by itself is meaningless in multi-container use-case. I don't think we want to load the same TAR file into multiple containers. This must have slipped through our code review when this feature was added. We had mainly added it for single container use.

Code after:
					break
				}
			}
		}
	}
	return name
}

func containerNameNoRemap(spec *specs.Spec) string {
	return spec.Annotations[ContainerdContainerNameAnnotation]
}

// RootfsTarUpperPath returns the path to the rootfs upper tar file, or empty
// string if not set. In multi-container mode, only container-specific
// annotations are used.
func RootfsTarUpperPath(spec *specs.Spec) string {
	if spec == nil || spec.Annotations == nil {
		return ""
	}
	if name := ContainerName(spec); name != "" {
		return spec.Annotations[AnnotationRootfsUpperTar+"."+name]
	}
	// Multi-container sandbox must use container-specific annotations.
	cType := SpecContainerType(spec)
	if cType == ContainerTypeContainer || cType == ContainerTypeSandbox {
		return ""
	}
	return spec.Annotations[AnnotationRootfsUpperTar]
}

// AnnotationToBool parses the annotation value as a bool. On failure, it logs a warning and
// returns false.
func AnnotationToBool(spec *specs.Spec, annotation string) bool {
	val, ok := spec.Annotations[annotation]
	if !ok {
		return false
	}
	ret, err := strconv.ParseBool(val)
	if err != nil {
		log.Warningf("Failed to parse annotation %q=%q as a bool: %v", annotation, val, err)
		return false
	}
	return ret
}
Repository: googleapis/genai-toolbox
File: internal/sources/mysql/mysql_test.go (Go)

Code before:
		{
			desc: "empty query params",
			in: `
			sources:
				my-mysql-instance:
					kind: mysql
					host: 0.0.0.0
					port: my-port
					database: my_db
					user: my_user
					password: my_pass
					queryParams: {}
			`,
			want: server.SourceConfigs{
				"my-mysql-instance": mysql.Config{
					Name:        "my-mysql-instance",
					Kind:        mysql.SourceKind,
					Host:        "0.0.0.0",
					Port:        "my-port",
					Database:    "my_db",
					User:        "my_user",
					Password:    "my_pass",
					QueryParams: map[string]string{},
				},
			},
		},
	}
	for _, tc := range tcs {
		tc := tc
		t.Run(tc.desc, func(t *testing.T) {
			t.Parallel()
			got := struct {
				Sources server.SourceConfigs `yaml:"sources"`
			}{}
			// Parse contents
			err := yaml.Unmarshal(testutils.FormatYaml(tc.in), &got)
			if err != nil {
				t.Fatalf("unable to unmarshal: %s", err)
			}
			if diff := cmp.Diff(tc.want, got.Sources, cmpopts.EquateEmpty()); diff != "" {
				t.Fatalf("mismatch (-want +got):\n%s", diff)
			}
		})
	}

}

func TestFailParseFromYaml(t *testing.T) {
	tcs := []struct {
		desc     string
		in       string

Review comment (suggestion):
```suggestion
```

This is not necessary since it will be tested in the other test cases that does not have `queryParams` defined. :) 

Code after:
	}
	for _, tc := range tcs {
		t.Run(tc.desc, func(t *testing.T) {
			t.Parallel()
			got := struct {
				Sources server.SourceConfigs `yaml:"sources"`
			}{}
			// Parse contents
			err := yaml.Unmarshal(testutils.FormatYaml(tc.in), &got)
			if err != nil {
				t.Fatalf("unable to unmarshal: %s", err)
			}
			if diff := cmp.Diff(tc.want, got.Sources, cmpopts.EquateEmpty()); diff != "" {
				t.Fatalf("mismatch (-want +got):\n%s", diff)
			}
		})
	}

}

func TestFailParseFromYaml(t *testing.T) {
	tcs := []struct {
		desc string
		in   string
		err  string
	}{
		{
			desc: "extra field",
			in: `
			sources:
				my-mysql-instance:
					kind: mysql
					host: 0.0.0.0
					port: my-port
					database: my_db
					user: my_user
					password: my_pass
					foo: bar
			`,
			err: "unknown field \"foo\"",
		},
		{
			desc: "missing required field",
			in: `
			sources:
				my-mysql-instance:
					kind: mysql
					port: my-port
					database: my_db
					user: my_user
					password: my_pass
Repository: googleapis/genai-toolbox
File: internal/sources/mysql/mysql_test.go (Go)

Code before:
			in: `
			sources:
				my-mysql-instance:
					kind: mysql
					port: my-port
					database: my_db
					user: my_user
					password: my_pass
			`,
			err:      "unable to parse source \"my-mysql-instance\" as \"mysql\": Key: 'Config.Host' Error:Field validation for 'Host' failed on the 'required' tag",
			contains: false,
		},
		{
			desc: "invalid query params type",
			in: `
			sources:
				my-mysql-instance:
					kind: mysql
					host: 0.0.0.0
					port: 3306
					database: my_db
					user: my_user
					password: my_pass
					queryParams: not-a-map
			`,
			err:      "cannot decode",
			contains: true,
		},
	}
	for _, tc := range tcs {
		tc := tc
		t.Run(tc.desc, func(t *testing.T) {
			t.Parallel()
			got := struct {
				Sources server.SourceConfigs `yaml:"sources"`
			}{}
			// Parse contents
			err := yaml.Unmarshal(testutils.FormatYaml(tc.in), &got)
			if err == nil {
				t.Fatalf("expect parsing to fail")
			}
			errStr := err.Error()
			if tc.contains {
				if !strings.Contains(errStr, tc.err) {
					t.Fatalf("unexpected error: got %q, want substring %q", errStr, tc.err)
				}
			} else {
				if errStr != tc.err {
					t.Fatalf("unexpected error: got %q, want %q", errStr, tc.err)
				}
			}

Review comment (suggestion):
This test case should trigger the unmarshal error in golang, we can copy the whole error string here too. Thank you for adding this :) 

Code after:
			t.Parallel()
			got := struct {
				Sources server.SourceConfigs `yaml:"sources"`
			}{}
			// Parse contents
			err := yaml.Unmarshal(testutils.FormatYaml(tc.in), &got)
			if err == nil {
				t.Fatalf("expect parsing to fail")
			}
			errStr := err.Error()
			if !strings.Contains(errStr, tc.err) {
				t.Fatalf("unexpected error: got %q, want substring %q", errStr, tc.err)
			}
		})
	}
}

// TestFailInitialization test error during initialization without attempting a DB connection.
func TestFailInitialization(t *testing.T) {
	t.Parallel()

	cfg := mysql.Config{
		Name:         "instance",
		Kind:         "mysql",
		Host:         "localhost",
		Port:         "3306",
		Database:     "db",
		User:         "user",
		Password:     "pass",
		QueryTimeout: "abc", // invalid duration
	}
	_, err := cfg.Initialize(context.Background(), noop.NewTracerProvider().Tracer("test"))
	if err == nil {
		t.Fatalf("expected error for invalid queryTimeout, got nil")
	}
	if !strings.Contains(err.Error(), "invalid queryTimeout") {
		t.Fatalf("unexpected error: %v", err)
	}
}
Repository: googleapis/genai-toolbox
File: tests/dataform/dataform_integration_test.go (Go)

Code before:

	return tmpDir, cleanup
}

func TestDataformCompileTool(t *testing.T) {
	if _, err := exec.LookPath("dataform"); err != nil {
		t.Skip("dataform CLI not found in $PATH, skipping integration test")
	}

	projectDir, cleanupProject := setupTestProject(t)
	defer cleanupProject()

	toolConfig := map[string]any{
		"tools": map[string]any{
			"my-dataform-compiler": map[string]any{
				"kind":        "dataform-compile",
				"description": "Tool to compile dataform projects",
			},
		},
		"sources": map[string]any{}, // No source needed for this tool
	}

	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
	defer cancel()

	cmd, cleanupServer, err := tests.StartCmd(ctx, toolConfig)
	if err != nil {
		t.Fatalf("command initialization returned an error: %s", err)
	}
	defer cleanupServer()

	waitCtx, cancelWait := context.WithTimeout(ctx, 30*time.Second)
	defer cancelWait()
	out, err := testutils.WaitForString(waitCtx, regexp.MustCompile(`Server ready to serve`), cmd.Out)
	if err != nil {
		t.Logf("toolbox command logs: \n%s", out)
		t.Fatalf("toolbox didn't start successfully: %s", err)
	}

	nonExistentDir := filepath.Join(os.TempDir(), "non-existent-dir-for-test-12345")
	os.Remove(nonExistentDir)

	testCases := []struct {
		name       string
		reqBody    string
		wantStatus int
		wantBody   string // Substring to check for in the response
	}{
		{
			name:       "success case",
			reqBody:    fmt.Sprintf(`{"project_dir":"%s"}`, projectDir),

Review comment (suggestion):
```suggestion
	cmd, cleanupServer, err := tests.StartCmd(ctx, toolsFile)
```

Code after:
	return tmpDir, cleanup
}

func TestDataformCompileTool(t *testing.T) {
	if _, err := exec.LookPath("dataform"); err != nil {
		t.Skip("dataform CLI not found in $PATH, skipping integration test")
	}

	projectDir, cleanupProject := setupTestProject(t)
	defer cleanupProject()

	toolsFile := map[string]any{
		"tools": map[string]any{
			"my-dataform-compiler": map[string]any{
				"kind":        "dataform-compile-local",
				"description": "Tool to compile dataform projects",
			},
		},
	}

	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
	defer cancel()

	cmd, cleanupServer, err := tests.StartCmd(ctx, toolsFile)
	if err != nil {
		t.Fatalf("command initialization returned an error: %s", err)
	}
	defer cleanupServer()

	waitCtx, cancelWait := context.WithTimeout(ctx, 30*time.Second)
	defer cancelWait()
	out, err := testutils.WaitForString(waitCtx, regexp.MustCompile(`Server ready to serve`), cmd.Out)
	if err != nil {
		t.Logf("toolbox command logs: \n%s", out)
		t.Fatalf("toolbox didn't start successfully: %s", err)
	}

	nonExistentDir := filepath.Join(os.TempDir(), "non-existent-dir")

	testCases := []struct {
		name       string
		reqBody    string
		wantStatus int
		wantBody   string // Substring to check for in the response
	}{
		{
			name:       "success case",
			reqBody:    fmt.Sprintf(`{"project_dir":"%s"}`, projectDir),
			wantStatus: http.StatusOK,
			wantBody:   "test_col",
		},