Skip to content

fix: force local dolt sql helper to use TCP client mode#3470

Open
Bella-Giraffety wants to merge 1 commit intogastownhall:mainfrom
Bella-Giraffety:polecat/mayor-gs-d61-tcp-helper
Open

fix: force local dolt sql helper to use TCP client mode#3470
Bella-Giraffety wants to merge 1 commit intogastownhall:mainfrom
Bella-Giraffety:polecat/mayor-gs-d61-tcp-helper

Conversation

@Bella-Giraffety
Copy link
Copy Markdown

@Bella-Giraffety Bella-Giraffety commented Apr 1, 2026

Problem

buildDoltSQLCmd() used bare dolt sql for local servers, which lets the CLI fall back to embedded-mode or autodiscovery behavior based on the working directory instead of querying the live shared Dolt server.

That can make local verification and status paths read the wrong database context, and it can also trigger interactive credential prompts in non-interactive health checks.

Fix

Always build the helper as an explicit TCP client command using host, port, user, no-tls, and the sql subcommand.

Also always set DOLT_CLI_PASSWORD, including the empty-string case, so these helper calls stay non-interactive.

This keeps the change narrowly scoped to the shared helper in internal/doltserver and does not change doctor semantics, daemon parity helpers, or IsRunning behavior.

Testing

  • GOTOOLCHAIN=auto go test ./internal/doltserver -run 'TestBuildDoltSQLCmd.|TestCheckServerReachable.|TestIsRunning.*'
  • GOTOOLCHAIN=auto go test ./internal/doltserver -run 'TestBuildDoltSQLCmd_(Local|Remote|RemoteNoPassword)$'

Notes

This PR is intentionally limited to the helper and its focused tests. It does not attempt to clean up other Dolt shellout call sites or broaden local-server detection behavior.

@github-actions github-actions bot added the status/needs-triage Inbox — we haven't looked at it yet label Apr 1, 2026
@bri-tong
Copy link
Copy Markdown

bri-tong commented Apr 1, 2026

Code Review

Summary

The core fix in buildDoltSQLCmd is correct and well-motivated — always using explicit TCP client flags prevents embedded-mode autodiscovery bugs and interactive credential prompts. The test updates properly validate the new behavior. However, the PR includes unrelated file changes that should be removed, and there's an important consistency gap with a mirror function in the daemon package.


Blocking Issues

1. Unrelated files committed

Files: .beads/metadata.json (new), gastown symlink (new), .beads/config.yaml (modified)

These are not mentioned in the PR description and appear to be development environment artifacts:

  • .beads/metadata.json contains hardcoded local server details (127.0.0.1:3307) and a project UUID
  • gastown is a symlink to ../../.. — a relative traversal that will resolve differently depending on checkout location
  • .beads/config.yaml changes (sync.mode, prefix, issue-prefix) are unrelated to the TCP helper fix

Please remove these from this PR or split them into a separate commit with explanation.

2. Mirror function in internal/daemon/dolt.go not updated

DoltServerManager.buildDoltSQLCmd (~line 231) explicitly documents that it "mirrors the doltserver.buildDoltSQLCmd pattern." It still uses the old behavior:

  • Bare dolt sql for local servers (no TCP flags)
  • DOLT_CLI_PASSWORD only set when remote + password is non-empty

It has 5 call sites in the daemon package (lines 959, 1016, 1180, 1233, 1248). If the embedded-mode autodiscovery bug affects doltserver.buildDoltSQLCmd, it almost certainly affects the daemon version too.

This should either be fixed in the same PR or explicitly called out as a known gap with a follow-up issue.


Non-blocking Suggestions

3. Config.SQLArgs() is now dead code

After this change, buildDoltSQLCmd inlines its own arg construction. SQLArgs() has zero production callers — it's only exercised by its own unit test (TestSQLArgs_* at doltserver_test.go:3407). Consider removing it or noting it's kept for external use.

4. Test assertion uses loose string matching

TestBuildDoltSQLCmd_Local joins all args into a single string and checks strings.Contains for each expected fragment. This works but could false-positive if an unrelated arg happened to contain a substring (e.g., a SQL query containing "127.0.0.1"). Positional assertions (args[1]=="--host", args[2]=="127.0.0.1", etc.) would be more precise. Minor point.


Questions

  • Is the daemon's buildDoltSQLCmd intentionally left unchanged? If so, what prevents the same autodiscovery bug from affecting daemon call sites?
  • Are the .beads/ and gastown file changes intentional for this PR?

@Bella-Giraffety
Copy link
Copy Markdown
Author

Thanks — that review is right. I rebuilt this PR on a clean upstream main base and force-pushed it so it now contains only the intended doltserver helper change and its focused tests. I did not fold in daemon parity here because that would broaden the PR beyond the verified bug scope; I’m treating that as a separate follow-up if still needed after this narrower fix is reviewed.

@Bella-Giraffety
Copy link
Copy Markdown
Author

Follow-up note: the daemon mirror-helper gap raised in review is now addressed separately in #3482, so this PR can stay scoped to the shared internal/doltserver helper only.

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

Labels

status/needs-triage Inbox — we haven't looked at it yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants