feat(dolt): git-origin collision guard in bd dolt remote add + bd doctor (be-7eu1d)#4153
feat(dolt): git-origin collision guard in bd dolt remote add + bd doctor (be-7eu1d)#4153quad341 wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CI fix — addressed the failing Check doc flags freshness: The new The previously-failing check now passes; the rest of the suite is re-running. |
quad341
force-pushed
the
feat/be-7eu1d-git-origin-guard
branch
from
1df140a to
07dcb9a
Compare
quad341
force-pushed
the
feat/be-7eu1d-git-origin-guard
branch
from
0780e00 to
ef9622d
Compare
maphew
left a comment
maphew
left a comment
There was a problem hiding this comment.
Thanks for this. The guard is useful and belongs in core, but I do not think this branch should merge as-is yet.
Findings:
-
The collision guard misses supported Dolt Git remote forms.
cmd/bd/dolt_remote_guard.goonly strips.gitand trailing slashes, andcmd/bd/dolt.gouses that helper before adding the remote. That meansgit+https://github.com/org/repo.git,git+ssh://..., and SCP-style equivalents can bypass a git origin of the same repository even though those are first-class Dolt remote URL forms. -
bd doctorhas the same normalization gap.cmd/bd/doctor/remotes.gocompares SQL remotes to git origin through its own simple normalization helper, so it can miss the same Dolt-normalized equivalents. The command path and doctor path should share one Dolt-aware comparator. -
The new embedded guard test file is tagged
embeddeddolt, but it calls helpers from cgo-tagged test files.go test -tags 'embeddeddolt gms_pure_go' ./cmd/bd ...fails to compile with undefined helper symbols, so this coverage is not reliable under its declared tags.
Recommended path: fix-merge after split/restack. The git-origin guard itself is worth preserving, but this PR is stacked on still-open #4151. I would land or fix #4151 separately, then restack this to just the guard, doctor, and docs changes. Keep #4323 as the merge vehicle for the push-only no-push behavior.
I checked the red CI enough to separate signal from noise: generated docs are current, the lint failure is in cmd/bd/bootstrap.go and already present on current upstream/main, and the embedded routed-store failures appear unrelated to the changed files here.
Checks run locally:
bd-main/scripts/pr-preflight.sh 4153 --repo gastownhall/beadsgit merge-tree upstream/main refs/remotes/upstream/pr-4153git diff --check upstream/main...HEAD./scripts/generate-cli-docs.sh --check- targeted
CGO_ENABLED=0 go test -tags gms_pure_go ./cmd/bd ... go test ./cmd/bd/doctor ...
No code was pushed from this review.
codex-gpt-5.5-xhigh on behalf of maphew
…v2ou) Failing tests for be-3v2ou (dolt.local-only enforcement, FR-01 to FR-05). All tests fail or refuse to compile until the builder adds isDoltLocalOnly(). cmd/bd/dolt_local_only_test.go (unit, no build tag): - TestIsDoltLocalOnly_FalseByDefault - TestIsDoltLocalOnly_TrueWhenConfigSet - TestIsDoltLocalOnly_FalseAfterUnset - TestMaybeAutoPush_SkipsWhenLocalOnly cmd/bd/dolt_local_only_embedded_test.go (build:embeddeddolt): - TestDoltLocalOnly_Push_ExitsZeroWithMessage - TestDoltLocalOnly_Push_LocalStoreCopy - TestDoltLocalOnly_Push_JSON_DisabledStatus - TestDoltLocalOnly_Pull_ExitsZeroWithMessage - TestDoltLocalOnly_Pull_JSON_DisabledStatus - TestDoltLocalOnly_RemoteAdd_RefusesExitOne - TestDoltLocalOnly_RemoteAdd_ErrorOnStderr - TestDoltLocalOnly_ConfigRoundTrip - TestDoltLocalOnly_DoltStatus_ShowsDisabled Message copy from be-0jyq7 (UX spec). Exact strings asserted so the builder cannot ship copy drift. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… FR-05)
- Add isDoltLocalOnly() in dolt_local_only.go (reads dolt.local-only config key)
- bd dolt push: exit-0 no-op with 'Remote sync is disabled...' message when set;
--json emits {status:disabled,reason:dolt.local-only=true} via outputJSONRaw
- bd dolt pull: same exit-0 no-op + 'Nothing to pull.' message
- bd dolt remote add: exit-1 error to stderr ('cannot add Dolt remote: ... disabled')
- bd dolt status: appends 'Remote sync: disabled (dolt.local-only=true)' indicator
in both embedded and server-mode text paths
- maybeAutoPush: guard fires before getStore() (before any store interaction)
- Fix test build tag: cgo (not embeddeddolt) to match CI embedded test convention
- Fix TestDoltLocalOnly_ConfigRoundTrip: bd config get returns "(not set ...)" not ""
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tor (be-7eu1d, FR-06 to FR-09) - dolt_remote_guard.go: guardNormalizeURL() + doltRemoteMatchesGitOrigin() helpers - dolt.go: --allow-git-origin flag; refuse (or warn) when Dolt remote URL matches git origin - doctor/types.go: CategoryDolt = "Dolt Storage" + add to CategoryOrder - doctor/remotes.go: CheckDoltRemoteGitOrigin() warning check - doctor.go: wire Check 7f1 into the doctor run Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The PR added the --allow-git-origin flag to 'bd dolt remote add' but did not regenerate docs, failing the 'doc flags freshness' check. Output of ./scripts/generate-cli-docs.sh ./bd — adds the flag to docs/CLI_REFERENCE.md and the dolt.md command pages (latest + versioned snapshots). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The function was extracted to dolt_local_only.go in the stacked PR but the original declaration in init.go was not removed, causing a redeclared-in-block build error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hecks Full regeneration for the complete PR: --allow-git-origin flag, --force flag on dolt remote remove, and the bd doctor CheckDoltRemoteGitOrigin check. Also regenerates federation.md for completeness. Output of ./scripts/generate-cli-docs.sh ./bd and ./scripts/generate-llms-full.sh. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The be-7eu1d git-origin guard block left double blank lines around the new "Check 7f1" stanza, failing `make fmt-check` (CI "Check formatting" and the gofmt stage of "PR Lint"). gofmt collapses them to single blanks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous regen (07dcb9a) was built with a CGO-enabled bd, which includes the CGO-only `bd federation` subcommands. CI builds the docs binary with CGO_ENABLED=0 (scripts/ci/pr-policy.sh build_docs_binary), so check-doc-flags rejected the extra federation subcommand pages. Regenerated docs/CLI_REFERENCE.md, the federation cli-reference pages (live + versioned 1.0.0/1.0.4/1.0.5), and website/static/llms-full.txt with a CGO_ENABLED=0 binary. check-doc-flags + check-doc-freshness now pass locally under LC_ALL=C. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes the 'Check formatting' CI failure on this PR — the only PR-introduced gofmt miss among the changed Go files.
quad341
force-pushed
the
feat/be-7eu1d-git-origin-guard
branch
from
2416bcf to
d2a85da
Compare
…uard and doctor Address @maphew's review on PR gastownhall#4153 (be-e7k8): 1. Add doltremote.CanonicalForComparison() — normalizes both sides of a URL comparison to Dolt's git+ prefix form, then strips trailing slashes and .git. This makes https://github.com/org/repo.git compare equal to git+https://github.com/org/repo.git, and git@github.com:org/repo.git equal to git+ssh://git@github.com/org/repo.git. 2. Replace the simple guardNormalizeURL helper in dolt_remote_guard.go and normRemoteURL in doctor/remotes.go with doltremote.CanonicalForComparison so both paths share one implementation and stay in sync. 3. Fix build tag on dolt_remote_origin_guard_embedded_test.go from embeddeddolt to embeddeddolt && cgo: the file delegates to bdDoltSeparate which lives in a cgo-only file, so the embeddeddolt-only tag caused a compile failure under CGO_ENABLED=0 + embeddeddolt. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@maphew — all three findings addressed, pushed as 1. Collision guard URL normalization — Added 2. Doctor remotes — 3. Build tag mismatch — On the stacking question: the |
3 participants
Summary
bd dolt remote add) — exits 1 with actionable error message and hints--allow-git-originflag bypasses the guard with a stderr warning (for monorepo layouts)bd doctoremits aCategoryDolt / "Dolt Remote vs Git Origin"warning check when an existing remote collidesdoctor.CategoryDolt = "Dolt Storage"constant +CategoryOrderpositionStacking dependency
feat/be-3v2ou-dolt-local-only). The guard fires after theisDoltLocalOnly()check — correct ordering is enforced byTestDoltRemoteAdd_LocalOnly_FiresBeforeGuard. Please merge #4151 first; this branch is stacked on it.Implementation notes
dolt_remote_guard.go:guardNormalizeURL()(strips.git/trailing slash) +doltRemoteMatchesGitOrigin()— reusesgitOriginGetURL()fromsync_git.gonormalizeRemoteURL()insync_remote.godoes Dolt-format URL conversion (https://→git+https://); the guard uses a separate simpler helper to avoid conflating those semanticsdoctor/remotes.go:CheckDoltRemoteGitOrigin()queries SQL remotes and normalizes for comparisondoctor.go: Check 7f1 wired into the doctor run (warning only, does not setOverallOK = false)Test plan
CGO_ENABLED=0 go build -tags gms_pure_go ./...— cleanTestGitOriginGetURL_*,TestGuardNormalizeURL_*,TestDoltRemoteMatchesGitOrigin_*,TestAllowGitOriginFlag_*,TestCategoryDolt_*— all passBEADS_TEST_EMBEDDED_DOLT=1) — require live Dolt server; deferred to CIcmd/bdfailures (TestAutoExportGitAddFailureExitsNonZero,TestOutputContextFunction, etc.) confirmed on base branch, not introduced here🤖 Generated with Claude Code