Honor --data-dir in shared CLI config path (fixes #611) #614

Merged

crueber merged 1 commit from fix/issue-611 into main

2026-09-15 23:12:59 +00:00

crueber commented

2026-09-15 23:09:59 +00:00

Owner

Verification:

New unit tests cmd/walhub/datadir_sync_test.go: TestDataDirFlagSyncMatrix (7 subtests: flag repoint, no-flag unchanged, file store.root wins, file cache.dir wins, env overlay wins, explicit --config, env-file data dir) + TestServeFixupIdempotent; syncDataDirFlag at 100% cover.
go test ./cmd/walhub/... ./internal/config/... -race green; gofmt/vet clean; internal/config cover 96% (gate holds; gate covers internal/... only).
Real binaries: base binary leaks the default-store policy doc under a different AND a nonexistent --data-dir; fixed binary returns {} in both, writes land under the flag dir only; explicit store.root and WALHUB__STORE__ROOT win at the binary level too.
Web untouched (CLI/config only): no web tests/build needed.

Every store-touching subcommand ignored --data-dir: config.Load derives paths from ResolveDataDir(env) only, and only serve repaired that. This change applies the serve-style flag sync in the shared resolveConfig (plus the --env-file twin loadWithEnvFiles), re-pointing ONLY flag-derived paths (DataDir, Store.Root, Cache.Dir) when they equal the env-default-derived paths. Explicit file values and WALHUB__* overlay preserved by construction (no FirstRunDefaults re-derivation). Serve's fixup kept as an idempotent no-op. Verification: - New unit tests cmd/walhub/datadir_sync_test.go: TestDataDirFlagSyncMatrix (7 subtests: flag repoint, no-flag unchanged, file store.root wins, file cache.dir wins, env overlay wins, explicit --config, env-file data dir) + TestServeFixupIdempotent; syncDataDirFlag at 100% cover. - go test ./cmd/walhub/... ./internal/config/... -race green; gofmt/vet clean; internal/config cover 96% (gate holds; gate covers internal/... only). - Real binaries: base binary leaks the default-store policy doc under a different AND a nonexistent --data-dir; fixed binary returns {} in both, writes land under the flag dir only; explicit store.root and WALHUB__STORE__ROOT win at the binary level too. - Web untouched (CLI/config only): no web tests/build needed.

crueber added 1 commit

2026-09-15 23:10:00 +00:00

CLI: honor --data-dir in shared resolveConfig (Forgejo #611 ) 1ed619d783

Thread the peeled --data-dir flag into every subcommand via
syncDataDirFlag in cmd/walhub/config.go: re-point only flag-derived
paths (DataDir, Store.Root, Cache.Dir) when they equal the
ResolveDataDir(env)-derived paths, exactly like the serve fixup.
Explicit file values and WALHUB__* overlay preserved by construction.
Serve's own fixup kept as an idempotent no-op (noted in comment).

Tests: TestDataDirFlagSyncMatrix (flag/env/file/defaults/explicit-config/
env-file matrix) + TestServeFixupIdempotent. Docs: 11_config_cli.md
§3.1.1 + Decisions entry (law 12).

crueber commented

2026-09-15 23:12:44 +00:00

Author

Owner

Independent review: APPROVED (no fix commit needed — no defects found; all checks below verified directly in /tmp/walhub-611).

Method: read the full diff + config.go/serve.go/load.go/firstrun.go; ran gofmt, vet, full cmd/walhub suite, -race on the new tests, coverage profile; replayed the new matrix against origin/main config.go/serve.go (red, as expected); live-binary check (policy set under --data-dir A, get from A/nonexistent-X/default).

Comparison logic — sound. (a) Coinciding explicit file value (file hard-codes exactly /store) would be re-pointed: accepted as negligible — it requires hard-coding an ephemeral default path, is indistinguishable from the default without provenance tracking, and matches the serve semantics the issue prescribes; arguably flag-authoritative is even correct there. (b) Explicit --config + --data-dir: both resolveConfig legs sync; pinned by the "explicit --config plus flag" subtest. (c) Env-file leg: sync receives the SAME merged getenv closure Load used (config.go:204,212) — overlay values can never misclassify as flag-derived. Correct.
loadWithEnvFiles — understood (resolveConfig + --env-file pairs layered under real env). Zero-pair path delegates to resolveConfig (single sync); non-empty path syncs once per leg and never calls resolveConfig. No double-repoint skew.
Serve — byte-identical end state to before: post-sync values no longer equal the env-derived paths (or are equal only when flag==env, where re-pointing is a literal no-op), and both sides use os.Getenv in the serve path. Pinned by TestServeFixupIdempotent. No behavior change.
Field lesson honored — no FirstRunDefaults re-derivation anywhere; only DataDir assignment + two conditional re-points. WALHUB__* overlay preserved (verified: Defaults().Store.Root="" / Cache.Dir="/tmp/walgit" can never equal env-derived paths unless explicitly set to them).
Quality gates — gofmt clean, vet clean, full go test ./cmd/walhub/ green incl. -race, internal/config green. Matrix is RED on origin/main (all flag subtests fail, DataDir unmoved) and green here — true regression coverage. Coverage gate covers internal/... only (Makefile:34), so cmd/ is ungated, but syncDataDirFlag itself measures 100%.
Docs — 11_config_cli.md §3.1.1 sentence and the Decisions entry describe the actual code (verified openStore :501-503 falls back to /store on empty root, as the entry claims).
Common cases unchanged — no-flag and explicit---config-without-flag both hit the c.dataDir == "" early return before any mutation.

Live binary proof (WALHUB_DATA_DIR hermetic): policy set under --data-dir A reads back from A; nonexistent X returns {} (empty, no default leak); default dir returns {} (no cross-contamination); config dump under nonexistent X prints store.root/cache.dir under X.

Verdict: APPROVE / merge-ready. No changes requested, no fix commits made (worktree /tmp/walhub-611 left untouched on 1ed619d).

Independent review: APPROVED (no fix commit needed — no defects found; all checks below verified directly in /tmp/walhub-611). Method: read the full diff + config.go/serve.go/load.go/firstrun.go; ran gofmt, vet, full cmd/walhub suite, -race on the new tests, coverage profile; replayed the new matrix against origin/main config.go/serve.go (red, as expected); live-binary check (policy set under --data-dir A, get from A/nonexistent-X/default). 1. Comparison logic — sound. (a) Coinciding explicit file value (file hard-codes exactly <envDefault>/store) would be re-pointed: accepted as negligible — it requires hard-coding an ephemeral default path, is indistinguishable from the default without provenance tracking, and matches the serve semantics the issue prescribes; arguably flag-authoritative is even correct there. (b) Explicit --config + --data-dir: both resolveConfig legs sync; pinned by the "explicit --config plus flag" subtest. (c) Env-file leg: sync receives the SAME merged getenv closure Load used (config.go:204,212) — overlay values can never misclassify as flag-derived. Correct. 2. loadWithEnvFiles — understood (resolveConfig + --env-file pairs layered under real env). Zero-pair path delegates to resolveConfig (single sync); non-empty path syncs once per leg and never calls resolveConfig. No double-repoint skew. 3. Serve — byte-identical end state to before: post-sync values no longer equal the env-derived paths (or are equal only when flag==env, where re-pointing is a literal no-op), and both sides use os.Getenv in the serve path. Pinned by TestServeFixupIdempotent. No behavior change. 4. Field lesson honored — no FirstRunDefaults re-derivation anywhere; only DataDir assignment + two conditional re-points. WALHUB__* overlay preserved (verified: Defaults().Store.Root="" / Cache.Dir="/tmp/walgit" can never equal env-derived paths unless explicitly set to them). 5. Quality gates — gofmt clean, vet clean, full `go test ./cmd/walhub/` green incl. -race, internal/config green. Matrix is RED on origin/main (all flag subtests fail, DataDir unmoved) and green here — true regression coverage. Coverage gate covers internal/... only (Makefile:34), so cmd/ is ungated, but syncDataDirFlag itself measures 100%. 6. Docs — 11_config_cli.md §3.1.1 sentence and the Decisions entry describe the actual code (verified openStore :501-503 falls back to <dataDir>/store on empty root, as the entry claims). 7. Common cases unchanged — no-flag and explicit---config-without-flag both hit the `c.dataDir == ""` early return before any mutation. Live binary proof (WALHUB_DATA_DIR hermetic): policy set under --data-dir A reads back from A; nonexistent X returns {} (empty, no default leak); default dir returns {} (no cross-contamination); config dump under nonexistent X prints store.root/cache.dir under X. Verdict: APPROVE / merge-ready. No changes requested, no fix commits made (worktree /tmp/walhub-611 left untouched on 1ed619d).

crueber merged commit 9243fd1e92 into main

2026-09-15 23:12:59 +00:00

crueber referenced this pull request

2026-09-15 23:13:00 +00:00

CLI ignores --data-dir for the store: repo policy/wal/compact/import read and write the default store #611

crueber referenced this pull request from a commit

2026-09-15 23:13:01 +00:00

Merge pull request 'Honor --data-dir in shared CLI config path (fixes #611)' (#614) from fix/issue-611 into main