From 93ee14d530a4263efd57dbc76f245e1b39818535 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 19:39:22 +0000 Subject: [PATCH] refactor: move the seed tool under Server/cmd/seed (RL-10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Server/scripts/seed.go` was a `package main` sitting directly in `Server/scripts/`, which made `Server/scripts` itself one of the module's three main packages — a developer tool in the module's build graph under a directory name that says "loose scripts". It also did filesystem work in `func init()`: `os.MkdirAll("data", 0o750)` ran before `flag.Parse()`, so the directory appeared even when the tool immediately refused to run. The audit row (RL-10) claims that `init()` fires "during test discovery". It does not, and the obvious fix aimed at that claim would be aimed at nothing: `Server/scripts/` contains zero `_test.go` files, so Go never builds a test binary there and `go test ./...` never runs the `init()`. The residual defect is narrower and real — an untagged `package main` in the build graph, plus a side effect on a path (`go run ./cmd/seed -h`) that has nothing to do with tests. Done: - `Server/scripts/seed.go` -> `Server/cmd/seed/main.go`, joining `cmd/genprotocol/` from RL-09. `Server/scripts/` now holds shell and JS tooling only (docker-smoke.sh, k6/, toxiproxy/, voice-test.sh) and no Go entry point at all. - The `os.MkdirAll` moved out of `init()` to immediately before `db.Open` in `main()` — the one call that needs the directory, since `db.Open` -> `OpenWithMaxReaders` -> `openFile` creates no intermediate directories. - The package doc comment's usage lines were wrong in two ways, not one: they named `go run scripts/seed.go`, which no longer exists, and they omitted the mandatory `-confirm-dev`, so neither documented command could ever have run. Both corrected, and `seed.go is a standalone tool` became the conventional `Command seed populates ...`. - `Server/CLAUDE.md`'s Layout list now names `cmd/` and states that no Go entry point lives in `scripts/`. Two files, 20 insertions, 17 deletions. `go list` main packages go from `{server, server/cmd/genprotocol, server/scripts}` to `{server, server/cmd/genprotocol, server/cmd/seed}` — the count is unchanged at three, which is the honest framing: this relocates a main package to a conventional path, it does not remove one from the build graph. Verified: both directions, by building the pre-change file and the post-change file and running each in a fresh empty directory. Before, `seed` with no flags exits 1 *and leaves a `data/` directory behind*; `seed -h` exits 0 and also leaves `data/` behind. After, both exit the same way and create nothing — `data/ exists=NO` in each case. The happy path is unchanged: `seed -confirm-dev` in an empty directory creates `data/` at mode 0750, writes `data/chatserver.db`, and reports 4 users / 5 channels / 31 messages; a second run reports 0 new rows, so idempotence survives. The old documented invocation now fails loudly (`go run scripts/seed.go` -> `stat scripts/seed.go: no such file or directory`) and the new one is what the comment says. All four build-tag variants compile, `go vet ./...` passes, `gofmt -l` is clean outside `db/dbgen`, and `npx prettier --check .` passes. Behaviour delta, called out rather than left silent: the two cases above (`-h`, and a missing `-confirm-dev`) no longer create `./data`. That is a change, not a pure relocation. It is the change RL-10 asks for — the remedy text is "remove import/test-time filesystem side effects" — and the alternative that preserves the old behaviour exactly, making the `MkdirAll` the first statement of `main()` before `flag.Parse()`, would keep precisely the side effect the item exists to remove. Not included: `Server/scripts/genprotocol` was moved to `Server/cmd/` by the RL-09 commit rather than here, so the "executable tooling under conventional command ownership" class is closed across the two commits, not this one alone. `filepath.Dir(*dbPath)` was evaluated for the `MkdirAll` and rejected: it would fix a real gap (`-db /elsewhere/x.db` still creates a useless `./data` and does not create `/elsewhere`) but it means creating an arbitrary directory from CLI input, and that is a behaviour change past "shift it out of `init()`" — worth its own item. No `make seed` target was added, and the dated `docs/audit-*.md` rows naming `Server/scripts/seed.go` keep the old path. The findings ledger has zero references to this file, so no re-render was needed. Refs RL-10, L-10 --- Server/CLAUDE.md | 4 ++- Server/{scripts/seed.go => cmd/seed/main.go} | 33 ++++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) rename Server/{scripts/seed.go => cmd/seed/main.go} (93%) diff --git a/Server/CLAUDE.md b/Server/CLAUDE.md index 38648a70..577b2064 100644 --- a/Server/CLAUDE.md +++ b/Server/CLAUDE.md @@ -10,7 +10,9 @@ prometheus. `permissions/` role checks · `service/` domain logic shared by both entry points - `db/` hand-written query wrappers; `db/dbgen/` is generated (see `db-change`) - `cmd/` executable tooling, one `package main` per subdirectory — - `cmd/genprotocol/` regenerates the protocol constants from `protocol/schema.json` + `cmd/genprotocol/` regenerates the protocol constants from `protocol/schema.json`, + `cmd/seed/` fills a dev database (`go run ./cmd/seed -confirm-dev`). + `scripts/` holds shell/JS tooling only; no Go entry point lives there - `admin/` web admin panel · `updater/` self-update + signature verification · `plugin/` WASM plugin runtime (`-tags wazero`) · `telemetry/` OTel (`-tags otel`) - `syncutil/` lock helpers that gain deadlock detection under `-tags deadlock` diff --git a/Server/scripts/seed.go b/Server/cmd/seed/main.go similarity index 93% rename from Server/scripts/seed.go rename to Server/cmd/seed/main.go index a74651a0..e23dd6be 100644 --- a/Server/scripts/seed.go +++ b/Server/cmd/seed/main.go @@ -1,11 +1,13 @@ -// seed.go is a standalone tool that populates an OwnCord database with -// development data (users, channels, messages, DMs). It is idempotent: -// running it multiple times against the same database is safe. +// Command seed populates an OwnCord database with development data (users, +// channels, messages, DMs). It is idempotent: running it multiple times +// against the same database is safe. // -// Usage: +// Usage (from the Server/ directory): // -// go run scripts/seed.go # uses ./data/chatserver.db -// go run scripts/seed.go -db path/to/owncord.db # custom path +// go run ./cmd/seed -confirm-dev # uses ./data/chatserver.db +// go run ./cmd/seed -confirm-dev -db path/to/owncord.db # custom path +// +// -confirm-dev is mandatory: the seeded accounts use weak passwords. package main import ( @@ -137,6 +139,15 @@ func main() { log.SetFlags(0) // no timestamp prefix — keep output clean + // The default DB path is data/chatserver.db, and db.Open does not create + // intermediate directories. Ensure ./data exists so a fresh checkout works. + // This sits here rather than in an init() so that importing or building the + // package touches no filesystem — the tool creates the directory only when + // it is actually about to open the default database. + if err := os.MkdirAll("data", 0o750); err != nil { + log.Printf("warning: could not create data directory: %v", err) + } + database, err := db.Open(*dbPath) if err != nil { log.Fatalf("failed to open database at %s: %v", *dbPath, err) @@ -359,13 +370,3 @@ func createDMConversation(database *db.DB, userIDs []int64) (int, error) { return created, nil } - -// ─── Ensure data directory exists ─────────────────────────────────────────── - -func init() { - // The default DB path is data/chatserver.db. Ensure the data directory - // exists so db.Open doesn't fail on a fresh checkout. - if err := os.MkdirAll("data", 0o750); err != nil { - log.Printf("warning: could not create data directory: %v", err) - } -}