diff --git a/docs/packages-github.md b/docs/packages-github.md index 9749fd67..289d0117 100644 --- a/docs/packages-github.md +++ b/docs/packages-github.md @@ -72,6 +72,10 @@ token in `config.json`. | `GOCLAW_PACKAGES_GITHUB_BIN_DIR` | `{runtimeDir}/bin` | Where extracted binaries land | | `GOCLAW_PACKAGES_GITHUB_MANIFEST` | `{bin_dir}/../github-packages.json` | Manifest path | +`packages.scratch_dir` in `config.json` is optional. If it is empty or cannot +be created, updates use `{runtimeDir}/tmp` so bare-metal services do not depend +on root-owned release directories such as `/opt/goclaw/tmp`. + Token scopes: - public-only repos: no scopes required - private repos: `repo` @@ -224,7 +228,8 @@ Check response header `X-RateLimit-Reset` (Unix epoch). Wait or set #### Scratch dir leftover after crash -Path: `{BinDir}/../tmp/{name}-{tag}-{nanos}/` +Path: `{runtimeDir}/tmp/{name}-{tag}-{nanos}/` unless +`packages.scratch_dir` points to another writable directory. Safe to remove any `{name}-*-*` directory under tmp after ensuring no active update is in flight. Phase 2 will add startup GC. diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 75f99fe8..bf1a7cde 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,18 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-29 +### GitHub Releases update scratch dir fallback (issue #94) + +- Changed GitHub Releases package updates to prefer `{runtimeDir}/tmp` for + scratch extraction and staging, instead of deriving tmp from a release or + binary directory. +- If `packages.scratch_dir` is configured but cannot be created, the update + executor now logs a warning and falls back to runtime tmp before failing. +- Added regression tests for default scratch-dir selection and fallback from an + unusable configured scratch path. + +--- + ### CLI Credentials git preset null-env crash (issue #93) **Fixes** diff --git a/internal/config/config.go b/internal/config/config.go index 86d6f0d3..3f405911 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -72,9 +72,8 @@ type Config struct { // empty string → default 1h. // // ScratchDir is the tmp workspace used by the update executor for download -// + extract + staging before atomic swap. Defaults to "{BinDir}/../tmp" when -// empty; operators MAY set explicitly to avoid symlink-resolution issues -// (red-team H6). +// + extract + staging before atomic swap. Empty or unusable values fall back to +// "{runtimeDir}/tmp"; operators MAY set an explicit writable path. type PackagesConfig struct { GitHubToken string `json:"github_token,omitempty"` // Phase 2 stub UpdatesCheckTTL string `json:"updates_check_ttl,omitempty"` // e.g. "1h" diff --git a/internal/skills/github_update_executor.go b/internal/skills/github_update_executor.go index 9814cba1..49360eca 100644 --- a/internal/skills/github_update_executor.go +++ b/internal/skills/github_update_executor.go @@ -26,15 +26,14 @@ var ( // - C3: re-verifies asset via meta SHA256 when present; refuses staged // URL whose host is not in allowedDownloadHosts. // - C4: saveManifest retries up to 3× before declaring desync. -// - H6: explicit ScratchDir (no "../tmp" symlink hazard). +// - scratch workspace resolves to writable runtime storage before staging. // - L4: file written with 0755 during extraction, not chmod post-rename. type GitHubUpdateExecutor struct { Installer *GitHubInstaller - ScratchDir string // explicit; defaults to filepath.Join(BinDir, "..", "tmp") if empty + ScratchDir string // optional override; falls back to {runtimeDir}/tmp if unusable } -// NewGitHubUpdateExecutor wires the executor. Call SetScratchDir to override -// the default tmp path. +// NewGitHubUpdateExecutor wires the executor. func NewGitHubUpdateExecutor(installer *GitHubInstaller) *GitHubUpdateExecutor { return &GitHubUpdateExecutor{Installer: installer} } @@ -42,12 +41,51 @@ func NewGitHubUpdateExecutor(installer *GitHubInstaller) *GitHubUpdateExecutor { // Source returns "github". func (e *GitHubUpdateExecutor) Source() string { return "github" } -// scratchDir returns the resolved scratch directory. -func (e *GitHubUpdateExecutor) scratchDir() string { - if e.ScratchDir != "" { - return e.ScratchDir +// scratchDirCandidates returns scratch parents in preference order. +func (e *GitHubUpdateExecutor) scratchDirCandidates() []string { + candidates := make([]string, 0, 3) + add := func(path string) { + path = strings.TrimSpace(path) + if path == "" { + return + } + cleaned := filepath.Clean(path) + for _, existing := range candidates { + if existing == cleaned { + return + } + } + candidates = append(candidates, cleaned) } - return filepath.Join(filepath.Dir(e.Installer.Config.BinDir), "tmp") + + add(e.ScratchDir) + add(filepath.Join(packageRuntimeDir(), "tmp")) + if e.Installer != nil && e.Installer.Config != nil && e.Installer.Config.BinDir != "" { + add(filepath.Join(filepath.Dir(e.Installer.Config.BinDir), "tmp")) + } + return candidates +} + +func (e *GitHubUpdateExecutor) createScratchDir(name, target string) (string, error) { + suffix := fmt.Sprintf("%s-%s-%d", name, sanitizeTag(target), time.Now().UnixNano()) + var errs []error + for idx, base := range e.scratchDirCandidates() { + scratch := filepath.Join(base, suffix) + if err := os.MkdirAll(scratch, 0o755); err != nil { + errs = append(errs, fmt.Errorf("%s: %w", scratch, err)) + continue + } + if idx > 0 { + slog.Warn("github.update: scratch dir fallback selected", + "scratch", scratch, + "previous_error", errs[0]) + } + return scratch, nil + } + if len(errs) == 0 { + return "", errors.New("create scratch dir: no scratch directory candidates") + } + return "", fmt.Errorf("create scratch dir: %w", errors.Join(errs...)) } // Update applies the target version. The caller holds PackageLocker for @@ -130,10 +168,9 @@ func (e *GitHubUpdateExecutor) Update(ctx context.Context, name, toVersion strin } // Prepare scratch dir — isolated per-update. - scratch := filepath.Join(e.scratchDir(), - fmt.Sprintf("%s-%s-%d", name, sanitizeTag(target), time.Now().UnixNano())) - if err := os.MkdirAll(scratch, 0o755); err != nil { - return fmt.Errorf("create scratch dir: %w", err) + scratch, err := e.createScratchDir(name, target) + if err != nil { + return err } defer os.RemoveAll(scratch) diff --git a/internal/skills/github_update_executor_test.go b/internal/skills/github_update_executor_test.go index c3a38a6d..d4cf08d0 100644 --- a/internal/skills/github_update_executor_test.go +++ b/internal/skills/github_update_executor_test.go @@ -246,6 +246,61 @@ func TestGitHubUpdateExecutor_NotInstalled(t *testing.T) { } } +func TestGitHubUpdateExecutor_DefaultScratchDirUsesRuntimeDir(t *testing.T) { + runtimeDir := t.TempDir() + t.Setenv("RUNTIME_DIR", runtimeDir) + + cfg := &GitHubPackagesConfig{ + BinDir: filepath.Join(string(filepath.Separator), "opt", "goclaw", "bin"), + ManifestPath: filepath.Join(t.TempDir(), "manifest.json"), + } + inst := NewGitHubInstaller(NewGitHubClient(""), cfg) + + exec := NewGitHubUpdateExecutor(inst) + candidates := exec.scratchDirCandidates() + if len(candidates) == 0 { + t.Fatal("scratchDirCandidates returned no candidates") + } + + want := filepath.Join(runtimeDir, "tmp") + if candidates[0] != want { + t.Fatalf("primary scratch dir = %q, want runtime tmp %q", candidates[0], want) + } +} + +func TestGitHubUpdateExecutor_CreateScratchDirFallsBackFromBadConfiguredDir(t *testing.T) { + runtimeDir := t.TempDir() + t.Setenv("RUNTIME_DIR", runtimeDir) + + cfg := &GitHubPackagesConfig{ + BinDir: filepath.Join(runtimeDir, "bin"), + ManifestPath: filepath.Join(runtimeDir, "github-packages.json"), + } + inst := NewGitHubInstaller(NewGitHubClient(""), cfg) + + blockedPath := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(blockedPath, []byte("blocked"), 0o644); err != nil { + t.Fatal(err) + } + + exec := NewGitHubUpdateExecutor(inst) + exec.ScratchDir = blockedPath + + scratch, err := exec.createScratchDir("goclaw", "v0.8.0-beta.2") + if err != nil { + t.Fatalf("createScratchDir should fall back from bad configured dir: %v", err) + } + defer os.RemoveAll(scratch) + + wantPrefix := filepath.Join(runtimeDir, "tmp") + string(filepath.Separator) + if !strings.HasPrefix(scratch, wantPrefix) { + t.Fatalf("scratch dir = %q, want prefix %q", scratch, wantPrefix) + } + if fi, statErr := os.Stat(scratch); statErr != nil || !fi.IsDir() { + t.Fatalf("scratch dir not created: info=%v err=%v", fi, statErr) + } +} + func TestGitHubUpdateExecutor_MetaAssertions_NilSafe(t *testing.T) { // Red-team C6: nil-safe map assertions must never panic. cases := []map[string]any{