From 36e1d9680b005b9e1f5625e62cfd82929f5c7df8 Mon Sep 17 00:00:00 2001 From: Sycun <165354365+Sycun@users.noreply.github.com> Date: Thu, 8 Oct 2026 22:46:29 +0800 Subject: [PATCH] fix: a version write keeps the document it lands in and the mode it had Inserting `version` above a leading `---` put it in a document of its own, so every setting after the marker parsed as a second document and was silently ignored: the installation came up on defaults with a config that looked intact. Both writers - the Go one and upgrade.sh's - now insert after the document start, and the parse-back check still refuses anything a line rewrite cannot do safely. upgrade.sh rewrote config.yaml through a temp file created with the process umask, so a 0600 config came back 0644 on every package upgrade - the file holding the API keys became group/world readable. It now creates the temp file privately and restores the original mode before the atomic replace. A package upgrade also deleted `.update-build-pending`, because rsync --delete only keeps what the exclude list names: the installation forgot a compile it still owed and the next click reported "already up to date". Every one-click update artifact is on the keep list now, asserted against .gitignore so the next one cannot be forgotten the same way. --- cmd/server/build_scripts_test.go | 143 +++++++++++++++++++++++++++++++ internal/config/config.go | 27 +++++- internal/config/config_test.go | 43 ++++++++++ upgrade.sh | 44 ++++++++-- 4 files changed, 245 insertions(+), 12 deletions(-) diff --git a/cmd/server/build_scripts_test.go b/cmd/server/build_scripts_test.go index a2c45721..41e52d10 100644 --- a/cmd/server/build_scripts_test.go +++ b/cmd/server/build_scripts_test.go @@ -2,9 +2,13 @@ package main import ( "os" + "os/exec" "path/filepath" + "runtime" "strings" "testing" + + "gopkg.in/yaml.v3" ) // Every `go build` in this repository's scripts has to target the cmd/server package. @@ -45,3 +49,142 @@ func TestScriptsBuildThePackageNotASingleFile(t *testing.T) { t.Fatal("no `go build` command found in the scripts") } } + +func TestUpgradeScriptUpdateConfigVersionPreservesDocumentAndMode(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("upgrade.sh requires a POSIX shell") + } + bash, err := exec.LookPath("bash") + if err != nil { + t.Skip("bash is not available") + } + if _, err := exec.LookPath("python3"); err != nil { + t.Skip("python3 is not available") + } + + root := filepath.Join("..", "..") + scriptPath := filepath.Join(root, "upgrade.sh") + script, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatalf("read %s: %v", scriptPath, err) + } + function := extractShellFunction(t, string(script), "update_config_version") + command := "umask 022\n" + function + "\nCONFIG_FILE=$1\nupdate_config_version \"$2\"\n" + + for _, tc := range []struct { + name string + original string + wantDocumentStart bool + }{ + {name: "document start", original: "---\nserver:\n port: 8088\n", wantDocumentStart: true}, + {name: "private mode", original: "version: \"v1.0.0\"\nserver:\n port: 8088\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(configPath, []byte(tc.original), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(configPath, 0o600); err != nil { + t.Fatal(err) + } + + cmd := exec.Command(bash, "-c", command, "upgrade-version-test", configPath, "v1.2.3") + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("update_config_version failed: %v\n%s", err, output) + } + got, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if tc.wantDocumentStart && !strings.HasPrefix(string(got), "---\n") { + t.Errorf("YAML document start must remain first: %q", got) + } + var parsed struct { + Version string `yaml:"version"` + Server struct { + Port int `yaml:"port"` + } `yaml:"server"` + } + if err := yaml.Unmarshal(got, &parsed); err != nil { + t.Fatalf("updated config is not valid YAML: %v\n%s", err, got) + } + if parsed.Version != "v1.2.3" { + t.Errorf("version = %q, want v1.2.3", parsed.Version) + } + if parsed.Server.Port != 8088 { + t.Errorf("server.port = %d, want 8088", parsed.Server.Port) + } + info, err := os.Stat(configPath) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o600 { + t.Errorf("mode = %v, want 0600 preserved", info.Mode().Perm()) + } + }) + } +} + +// The tarball path syncs with `rsync --delete`, so every artifact a one-click update writes +// at the install root has to be on its keep list. .gitignore is exactly that list of +// machine-local artifacts, so the assertion is derived from it: a new marker cannot be +// forgotten here and silently deleted by the next package upgrade. +func TestUpgradeScriptKeepsEveryOneClickUpdateArtifact(t *testing.T) { + root := filepath.Join("..", "..") + ignore, err := os.ReadFile(filepath.Join(root, ".gitignore")) + if err != nil { + t.Fatal(err) + } + script, err := os.ReadFile(filepath.Join(root, "upgrade.sh")) + if err != nil { + t.Fatal(err) + } + + var artifacts []string + inSection := false + for _, line := range strings.Split(string(ignore), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "# One-click update artifacts") { + inSection = true + continue + } + if !inSection { + continue + } + if strings.HasPrefix(line, "#") { + break + } + if line != "" { + artifacts = append(artifacts, line) + } + } + if len(artifacts) == 0 { + t.Fatal("no update artifacts found in .gitignore: the section this test reads has moved") + } + + // The script names the binary through a variable; compare against the literal name. + keeps := strings.ReplaceAll(string(script), "${BINARY_NAME}", "cyberstrike-ai") + for _, artifact := range artifacts { + if !strings.Contains(keeps, "--exclude="+artifact) { + t.Errorf("upgrade.sh syncs with rsync --delete but does not keep %s", artifact) + } + } +} + +func extractShellFunction(t *testing.T, script, name string) string { + t.Helper() + startMarker := name + "() {" + start := strings.Index(script, startMarker) + if start < 0 { + t.Fatalf("function %s not found", name) + } + lines := strings.Split(script[start:], "\n") + for i := 1; i < len(lines); i++ { + if lines[i] == "}" { + return strings.Join(lines[:i+1], "\n") + } + } + t.Fatalf("function %s has no closing brace", name) + return "" +} diff --git a/internal/config/config.go b/internal/config/config.go index cd70c5ee..3a0509c5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1796,9 +1796,21 @@ func topVersionKey(line string) bool { return false } +func yamlDocumentStart(line string) bool { + line = strings.TrimPrefix(strings.TrimSuffix(line, "\r"), "\uFEFF") + if !strings.HasPrefix(line, "---") { + return false + } + rest := line[len("---"):] + if strings.Trim(rest, " \t") == "" { + return true + } + return len(rest) > 0 && (rest[0] == ' ' || rest[0] == '\t') && strings.HasPrefix(strings.TrimLeft(rest, " \t"), "#") +} + // WriteVersion 就地替换配置文件的顶层 version 字段，保留其它每一行的原样（tarball 升级 -// 路径一直这么做）；没有这个字段就插到文件开头。返回是否真的改了。写盘走临时文件 + 原子 -// rename——这是运维者的活配置，不能留半截。替换后先把新文本解析回验一遍：行级替换够不着的 +// 路径一直这么做）；没有这个字段就插到文件开头（若文件以 YAML 文档标记开头，则插在标记后）。 +// 返回是否真的改了。写盘走临时文件 + 原子 rename——这是运维者的活配置，不能留半截。替换后先把新文本解析回验一遍：行级替换够不着的 // 写法（JSON 单行、多行标量……）解析失败或读不回目标值就拒绝写、原文件不动。版本号停在旧值 // 只是页头显示问题，写坏配置是起不来。 func WriteVersion(path, version string) (bool, error) { @@ -1827,7 +1839,12 @@ func WriteVersion(path, version string) (bool, error) { } } if !replaced { - lines = append([]string{`version: "` + version + `"`}, lines...) + versionLine := `version: "` + version + `"` + if len(lines) > 0 && yamlDocumentStart(lines[0]) { + lines = append([]string{lines[0], versionLine}, lines[1:]...) + } else { + lines = append([]string{versionLine}, lines...) + } } out := strings.Join(lines, "\n") if out == string(data) { @@ -1840,6 +1857,10 @@ func WriteVersion(path, version string) (bool, error) { if err := os.WriteFile(tmp, []byte(out), info.Mode().Perm()); err != nil { return false, err } + if err := os.Chmod(tmp, info.Mode().Perm()); err != nil { + _ = os.Remove(tmp) + return false, err + } if err := os.Rename(tmp, path); err != nil { return false, err } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2ec3d311..30fc6544 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -385,6 +385,9 @@ func TestFileVersionAndWriteVersion(t *testing.T) { if err := os.WriteFile(path, []byte(original), 0o640); err != nil { t.Fatal(err) } + if err := os.Chmod(path, 0o640); err != nil { + t.Fatal(err) + } if v, err := FileVersion(path); err != nil || v != "v1.0.0" { t.Fatalf("version = %q err = %v", v, err) } @@ -425,6 +428,46 @@ func TestFileVersionAndWriteVersion(t *testing.T) { } } +func TestWriteVersionPreservesYAMLDocumentStart(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + original := "---\nserver:\n port: 8088\n" + const originalMode = os.FileMode(0o600) + if err := os.WriteFile(path, []byte(original), originalMode); err != nil { + t.Fatal(err) + } + + changed, err := WriteVersion(path, "v1.2.3") + if err != nil || !changed { + t.Fatalf("WriteVersion: changed=%v err=%v", changed, err) + } + got := readTestFile(t, path) + want := "---\nversion: \"v1.2.3\"\nserver:\n port: 8088\n" + if got != want { + t.Fatalf("file after write:\n%q\nwant:\n%q", got, want) + } + if !strings.HasPrefix(got, "---\n") { + t.Fatalf("YAML document start must remain first: %q", got) + } + if version, err := FileVersion(path); err != nil || version != "v1.2.3" { + t.Fatalf("FileVersion = %q, %v; want v1.2.3", version, err) + } + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load after WriteVersion: %v", err) + } + if cfg.Server.Port != 8088 { + t.Fatalf("server.port = %d, want 8088", cfg.Server.Port) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != originalMode { + t.Fatalf("mode = %v, want %v preserved", info.Mode().Perm(), originalMode) + } +} + // FileVersion 用 YAML 解析（引号键认得出），WriteVersion 若只认顶格裸键，合法配置就会被追加 // 出第二个 version 键、整个文件随后 Load 失败。这条测试钉死两者口径一致，以及"改不了就拒绝写"。 func TestWriteVersionRewritesQuotedKeysAndRefusesWhatItCannot(t *testing.T) { diff --git a/upgrade.sh b/upgrade.sh index 48380749..6ba91df8 100644 --- a/upgrade.sh +++ b/upgrade.sh @@ -28,7 +28,7 @@ set -euo pipefail # - tools/ (user extensions; never overwritten by upgrade) # - roles/, skills/, agents/ # - the rollback points of an earlier one-click update: -# .update-backup/, .update-state.json, cyberstrike-ai.prev +# .update-backup/, .update-state.json, .update-build-pending, cyberstrike-ai.prev ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" cd "$ROOT_DIR" @@ -285,7 +285,7 @@ confirm_or_exit() { fi info " - Preserve tools/: yes (always)" info " - Preserve roles/skills/agents: yes (always)" - info " - Preserve rollback points (.update-backup/, .update-state.json, ${BINARY_NAME}.prev): yes" + info " - Preserve rollback points (.update-backup/, .update-state.json, .update-build-pending, ${BINARY_NAME}.prev): yes" info " - Stop service: ${STOP_SERVICE}" fi @@ -511,7 +511,7 @@ update_config_version() { # config that no longer loads is an installation that no longer starts. local new_tag="$1" python3 - "$CONFIG_FILE" "$new_tag" <<'PY' -import os, re, sys +import os, re, stat, sys, tempfile path = sys.argv[1] tag = sys.argv[2] @@ -544,11 +544,33 @@ for i, line in enumerate(lines): if not refused: if not replaced: - out.insert(0, 'version: "%s"\n' % tag) - tmp = path + ".tmp" - with open(tmp, "w", encoding="utf-8") as f: - f.writelines(out) - os.replace(tmp, path) + insert_at = 0 + if lines: + first = lines[0].rstrip("\r\n") + if first.startswith("\ufeff"): + first = first[1:] + rest = first[3:] if first.startswith("---") else None + if rest is not None and (not rest.strip(" \t") or rest[:1] in (" ", "\t") and rest.lstrip(" \t").startswith("#")): + insert_at = 1 + if not lines[0].endswith(("\n", "\r")): + lines[0] += "\n" + out[0] = lines[0] + out.insert(insert_at, 'version: "%s"\n' % tag) + + original_mode = stat.S_IMODE(os.stat(path).st_mode) + tmp_fd, tmp = tempfile.mkstemp(prefix=os.path.basename(path) + ".", suffix=".tmp", dir=os.path.dirname(os.path.abspath(path))) + try: + with os.fdopen(tmp_fd, "w", encoding="utf-8") as f: + f.writelines(out) + f.flush() + os.fchmod(f.fileno(), original_mode) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except FileNotFoundError: + pass + raise PY } @@ -588,10 +610,14 @@ sync_code() { rsync_excludes+=( "--exclude=agents/" ) # Rollback points written by a one-click update: an rsync --delete that ate them would - # leave the installation with no way back. + # leave the installation with no way back - and eating .update-build-pending would make it + # forget a compile it still owes, so the next click reports "already up to date". rsync_excludes+=( "--exclude=.update-backup/" ) rsync_excludes+=( "--exclude=.update-staging/" ) rsync_excludes+=( "--exclude=.update-state.json" ) + rsync_excludes+=( "--exclude=.update-state.json.tmp" ) + rsync_excludes+=( "--exclude=.update-build-pending" ) + rsync_excludes+=( "--exclude=.update-build-pending.tmp" ) rsync_excludes+=( "--exclude=${BINARY_NAME}.prev" ) # Ensure this upgrade script itself is not deleted.