[RagedPanda]

Fixes #741

vfox upgrade stages the download in <install dir>/vfox_upgrade and renames the new binary over the old one, so it needs write access to the directory that holds vfox. On Windows, RequestPermission() handles this by relaunching elevated; on every other OS it was a stub, so an install in /usr/local/bin (the default install.sh target) failed with a raw mkdir ... permission denied.

RequestPermission() in upgrade_unix.go now creates and removes a temp file in the executable's directory. A real write probe also catches read-only mounts and ACLs that mode bits or ownership would miss. If the probe fails, the command exits with a message naming the directory and suggesting sudo <path to vfox> upgrade. The check stays where it was, after the "already up to date" check.

The Upgrade section of the usage docs (English and Chinese) now mentions that the command needs write access to the install directory.

Tests: TestCheckUpgradeDirWritable in cmd/commands/upgrade_unix_test.go covers a writable directory (no error, probe file removed) and a read-only one (error names the directory and the sudo command; skipped when running as root).

[codecov]

Codecov Report

❌ Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.45%. Comparing base (10c9f96) to head (a8708f8).

Files with missing lines	Patch %	Lines
cmd/commands/upgrade_unix.go	76.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
+ Coverage   51.39%   51.45%   +0.05%     
==========================================
  Files         111      111              
  Lines        8240     8256      +16     
==========================================
+ Hits         4235     4248      +13     
- Misses       3583     3586       +3     
  Partials      422      422              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[linghengqian]

Thanks for the PR. I tried it in a chmod 555 directory: it prints the new message, exits with 1 and leaves the directory as it was. A read-only binary in a writable directory still upgrades.

The one thing I want changed before this goes in is the tests. Neither subtest creates the binary, and the binary is the part that trips up a naive check. install.sh moves it into /usr/local/bin with sudo mv, so it stays owned by whoever unpacked it. That's the setup in #741: /usr/local/bin is root:root 755 and vfox belongs to the user. The binary is writable, the directory isn't, so a check on the binary says everything is fine and the upgrade then dies at mkdir. That is why the probe has to look at the directory, but nothing in the tests would notice if someone swapped it for something cheaper later.

I tried a few such swaps against the current tests:

• Checking the binary instead, with unix.Access(exePath, unix.W_OK) or its mode bits. Both are caught, but only because the binary isn't there. With a real one in place they'd let it through.
• Checking the directory's mode bits. This passes both subtests, and it's wrong for /usr/local/bin: the owner write bit is set, but you aren't the owner. t.TempDir() always belongs to the test user, so the current tests can't show that.

Could you add these three subtests to TestCheckUpgradeDirWritable? Each swap above fails at least one of them, and all three pass with your probe. They don't depend on how the check is done, only on it looking at the directory (unix.Access(dir, unix.W_OK) passes too).

	// install.sh moves the binary into /usr/local/bin with sudo, and it stays
	// owned by the user who unpacked it. The binary is writable, the directory
	// is not, so a check on the binary would pass here.
	t.Run("writable binary in a read-only directory", func(t *testing.T) {
		if os.Geteuid() == 0 {
			t.Skip("root can write to a read-only directory")
		}
		dir := t.TempDir()
		exePath := filepath.Join(dir, "vfox")
		if err := os.WriteFile(exePath, []byte("vfox"), 0755); err != nil {
			t.Fatal(err)
		}
		if err := os.Chmod(dir, 0555); err != nil {
			t.Fatal(err)
		}
		t.Cleanup(func() { os.Chmod(dir, 0755) })

		f, err := os.OpenFile(exePath, os.O_WRONLY, 0)
		if err != nil {
			t.Fatalf("the binary itself should be writable: %v", err)
		}
		f.Close()

		if err := checkUpgradeDirWritable(dir, exePath); err == nil {
			t.Fatal("expected an error, the directory is read-only")
		}
	})

	// Replacing the binary only needs the directory, so its own mode does not matter.
	t.Run("read-only binary in a writable directory", func(t *testing.T) {
		dir := t.TempDir()
		exePath := filepath.Join(dir, "vfox")
		if err := os.WriteFile(exePath, []byte("vfox"), 0555); err != nil {
			t.Fatal(err)
		}
		if err := checkUpgradeDirWritable(dir, exePath); err != nil {
			t.Fatalf("expected no error, got %v", err)
		}
	})

	// The root directory is root:root 755, like /usr/local/bin: the owner write
	// bit is set, but this user is not the owner. t.TempDir() is always ours,
	// so it can't show this.
	t.Run("directory owned by someone else", func(t *testing.T) {
		if os.Geteuid() == 0 {
			t.Skip("root can write anywhere")
		}
		if unix.Access("/", unix.W_OK) == nil {
			t.Skip("/ is writable by this user")
		}
		if err := checkUpgradeDirWritable("/", "/vfox"); err == nil {
			t.Fatal("expected an error for a directory this user does not own")
		}
	})

The last one uses / as a directory that belongs to someone else. It needs golang.org/x/sys/unix in the imports, which is already in go.mod.

A few smaller things, none of them blocking.

The error message has two small problems. It always suggests sudo, but that only helps when the problem is permissions: on a read-only mount even root can't write, so there the hint points the wrong way. And it repeats the probe's file name, for example open /usr/local/bin/.vfox_upgrade_check_449351111: permission denied, when the *fs.PathError carries the reason on its own. Both can be fixed in the same few lines (this needs errors and io/fs in the imports):

	probe, err := os.CreateTemp(dir, ".vfox_upgrade_check_*")
	if err != nil {
		reason := err
		var pathErr *fs.PathError
		if errors.As(err, &pathErr) {
			reason = pathErr.Err
		}
		msg := fmt.Sprintf("Cannot upgrade vfox: %s is not writable (%v).", dir, reason)
		// sudo only helps with permissions, not with a read-only filesystem
		if errors.Is(err, fs.ErrPermission) {
			msg += fmt.Sprintf("\nRe-run the upgrade with write access to it, for example: sudo %s upgrade", exePath)
		}
		return cli.Exit(msg, 1)
	}

A directory without write permission then gives ... is not writable (permission denied). plus the sudo line, and a read-only mount gives ... is not writable (read-only file system). without it. Your existing assertions still pass.

The docs say to use sudo, but that only applies to Linux and macOS and the page covers every platform: on Windows vfox upgrade relaunches itself elevated. Opening the paragraph with "On Linux and macOS" would sort that out, in the English page and the zh-hans one.

Otherwise this looks good to me.

[RagedPanda]

Thanks for the thorough review, and for testing the swaps. Everything you asked for is in a8708f8:

• Tests: added the three subtests to TestCheckUpgradeDirWritable as written (a writable binary in a read-only directory, a read-only binary in a writable directory, and / as a directory owned by someone else). All five subtests pass locally on macOS as a non-root user.
• Error message: it now shows only the reason from the *fs.PathError (permission denied, read-only file system) instead of repeating the probe file's name. It adds the sudo line only when the error is fs.ErrPermission, so a read-only mount no longer suggests sudo.
• Docs: the paragraph now starts with "On Linux and macOS", in both the English and the zh-hans pages.

go test ./... passes, apart from TestSelect_Show in internal/shared/printer, which waits for terminal input and hangs outside a TTY. It is unrelated to this change.

[linghengqian]

LGTM