Skip to content

fix(cli): keep repo .beads config when dolt_data_dir escapes#2614

Open
garitar wants to merge 3 commits intosteveyegge:mainfrom
Algorune:codex/fix-2590-real-root-cause
Open

fix(cli): keep repo .beads config when dolt_data_dir escapes#2614
garitar wants to merge 3 commits intosteveyegge:mainfrom
Algorune:codex/fix-2590-real-root-cause

Conversation

@garitar
Copy link
Contributor

@garitar garitar commented Mar 15, 2026

Summary

While auditing #2590 and PR #2591, I found a remaining failing case in the normal CLI path: when metadata.json sets dolt_data_dir outside .beads, bd list derives the wrong config directory from filepath.Dir(dbPath), loses the repo-local port/database config, and can fall back to the wrong server.

This patch fixes that by resolving the owning .beads directory independently from the discovered Dolt data path before loading config and opening the store.

Fixes #2590.

Why this is different from #2591

PR #2591 improves applyConfigDefaults(), but the failing bd list path in cmd/bd/main.go was already calling doltserver.DefaultConfig(beadsDir). The real bug was that beadsDir itself was wrong whenever dolt_data_dir escaped .beads.

So this PR addresses a narrower but load-bearing CLI failure mode:

  • dbPath points at the external Dolt data dir
  • filepath.Dir(dbPath) is no longer the repo's .beads
  • configfile.Load(beadsDir) misses metadata.json, dolt-server.port, and the configured database name
  • the CLI falls back to default/unresolved server settings and may auto-start a shadow server

Proof Repro

Added TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads in cmd/bd/main_test.go.

It creates a repo with:

  • .beads/metadata.json setting dolt_data_dir = ../external-dolt
  • a seeded .beads/dolt-server.port
  • a real issue created in the configured database

Then it runs a real bd list --json subprocess.

Behavior:

  • On unpatched origin/main, the repro fails because bd list loses the repo config, hits the wrong port under fallback behavior, auto-starts a shadow server, and cannot find the configured database.
  • On this branch, the same repro passes and lists the seeded issue.

Test Plan

  • go test ./cmd/bd -run '^TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads$' -count=1 -v
  • go test ./cmd/bd -run '^(TestBlockedEnvVars|TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads)$' -count=1
  • go test ./cmd/bd -run '^(TestImportOpenToClosedTransition|TestImportClosedToOpenTransition|TestBlockedEnvVars|TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads)$' -count=1

Notes

Thanks to #2591 for narrowing the search area. I left a cross-link there as well so maintainers can compare the two failure modes directly.

@garitar
Copy link
Contributor Author

garitar commented Mar 15, 2026

Quick CI note:

  • I pushed one tiny follow-up commit to satisfy the current make fmt-check baseline. It only runs gofmt on two pre-existing files that are already red on current main: cmd/bd/import.go and internal/storage/dolt/store.go.
  • The remaining Test (Embedded Dolt) failure looks unrelated to this PR. Current main is already failing the same job, and the failure mode matches the existing embedded multi-process crash in internal/storage/embeddeddolt/concurrency_test.go (TestConcurrencyMultiProcess / TestHelperProcess, nil-pointer panic inside Dolt embedded open).

So after the formatting-only follow-up, the remaining red appears to be upstream/background rather than caused by the #2590 CLI fix itself.

@garitar
Copy link
Contributor Author

garitar commented Mar 16, 2026

Canary follow-up: I just pushed bf13f6c4 onto this branch.

The functional fix still held, but the regression harness had one portability problem: it was hardcoded to build bd from a local folder which only works on one checkout shape. The test now builds from the active package directory via os.Getwd() instead.

Focused validation:

  • go test ./cmd/bd -run '^TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads$' -count=1

@codecov-commenter
Copy link

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4805 1 4804 549
View the top 1 failed test(s) by shortest run time
github.com/steveyegge/beads/cmd/bd::TestGetVersionsSinceOrder
Stack Traces | 0s run time
=== RUN   TestGetVersionsSinceOrder
    version_tracking_test.go:80: Versions not in chronological order: 0.61.0 (2026-03-15) should come before 0.57.0 (2026-03-01)
--- FAIL: TestGetVersionsSinceOrder (0.00s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link
Contributor

@DreadPirateRobertz DreadPirateRobertz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the filepath.Dir(dbPath) assumption breaks when dolt_data_dir escapes .beads. The fix to resolve the owning .beads directory independently from the Dolt data path is the right approach.

The test case (TestListUsesRepoBeadsDirWhenDoltDataDirEscapesDotBeads) is well-constructed — it exercises the exact failure path with an external data dir, seeded port file, and verifiable issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bd ignores configured port, connects to default 3307

3 participants