Skip to content

Triage and clean up an unreviewed automated commit stream on main#4418

Merged
julianknutsen merged 6 commits into
mainfrom
cleanup/triage-unreviewed-stream
Jun 16, 2026
Merged

Triage and clean up an unreviewed automated commit stream on main#4418
julianknutsen merged 6 commits into
mainfrom
cleanup/triage-unreviewed-stream

Conversation

@julianknutsen

Copy link
Copy Markdown
Collaborator

Triage and clean up an unreviewed automated commit stream on main

Summary

Between 9a1c88b63 (the reviewed dolt-2.1.4 bump, PR #4326) and the current origin/main
tip 8e18581dc, an automated agent committed a large stream of changes directly onto the
shared branch without going through PR review. A commit-by-commit triage found that the
cited issue references could not be located in the tracker, so each commit's stated
justification could not be independently verified.

This PR does not discard the stream. The triage found that roughly half of it is
genuine, diff-provable bug-fixing, and that several commits are load-bearing on an
already-published Dolt schema
— reverting them would brick deployed clones. So this PR:

  1. Reverts the no-op / characterization commits that don't fix a demonstrable defect (4 commits).
  2. Reverts the unreviewed changes in the gated, not-running serverV2 subsystem (the
    proxied / external-dolt-server layer) back to BASE — itemized in the serverV2 audit
    comment for the subsystem owner to review.
  3. Keeps the real fixes, and — since the cited references can't serve as the source of
    truth — backs every kept real defect with a proven born-failing regression test.
  4. Flags the remaining judgement calls for maintainer decision rather than silently
    keeping or reverting them.

Commits (6): 4 reverts of no-op commits + 1 commit adding proven regression tests + 1
serverV2 revert. Per-commit justification for the reverts and the flagged decisions is in the
Removal Rationale comment; the serverV2 fixes being removed are in the serverV2 audit
comment.

serverV2 revert (8515879dc)

The proxied / external-dolt-server subsystem is gated off and not running yet. This reverts
the unreviewed changes to its schema-independent layers — internal/storage/dbproxy/*,
internal/storage/uow/*, and the cmd/bd proxied glue — back to BASE (26 files, +107/-870).
A strict serverV2→BASE revert is impossible without breaking main: internal/storage/domain/db
is welded to published migration 0051 (BASE inserts without an id and relies on the
DEFAULT(UUID()) that 0051 removed), so the data layer + the issueops widening it needs are
kept at HEAD. The removed serverV2 fixes — most of them real, proven bugs in the gated
subsystem — are itemized for the owner to audit / re-apply.

The source-of-truth finding

All 66 cited issue IDs (bd-6dnrw.* ~47 items, bd-578h9.* ~19, plus bd-2rd37, bd-pkim8,
bd-hj85c, bd-4mpy7, …) are not present in the tracker. Verified: closed issues included
(bd list over all statuses); created-then-deleted ruled out (dolt_history_issues has zero
rows for any cited family across full history); not a parsing artifact (dotted epics like
bd-am3.1 exist); not on any sibling federated DB or remote beads-* branch; the IDs appear
only in commit messages.

Consequence: with no tracker entry to corroborate a commit's rationale, we treat the diff
and a born-failing test — never the cited reference — as the source of truth.

What this PR reverts (4 commits)

Non-schema, non-entangled, no demonstrated defect (reverted newest-first):

Reverted Why
93b186dcf Seam B list-parity characterization test for already-shipped behavior
1425e7338 Re-enables a CI lane the stream itself disabled + ungates a dark-launched feature; no shipped bug
81489b8c1 Autocommit-sweep over-firing "fixer" for a condition introduced earlier in the same stream
d2b6cc525 Speculative dirty-working-set autocommit safety net, no demonstrated defect

Reclassified revert → keep (3 commits, entanglement discovered while reverting)

Originally slated to revert but entangled with commits we keep, so they cannot be cleanly
reverted and none is harmful — left in place:

Commit Blocked by (kept)
1825cf357 (tree-walk dotted orphans) 341c7a5a4 dolt-2.1.6 named-CTE workaround builds on it
9187152cb (doctor rekey-repair) 6a1e7af9b gate-hardening modifies its dep_keys.go
f97fd4a8b (merge-refusal messaging) d80ed8146 no-remote fix shares the cmd/bd/dolt.go region

What this PR keeps (and why)

  • Schema / migration stacks — keep & forward-fix only. Migration 0051 (dropped
    DEFAULT(UUID())) and the aux-row-id rekey have already executed on every published clone;
    0051.down.sql is a no-op and re-adding the default is banned by migration-hygiene CI. Reverting
    their source does not un-run them — it bricks the write path (NOT NULL on every insert). Any
    future change ships as a new forward migration.
  • Sync settle/recompute subsystem — keep & re-land under review. Clones now rely on its
    auto-resolve / FK-repair on pull; the is_blocked-stale-after-merge defect it fixes is real at BASE.
  • Middle-tier hardening — surgical. Diff-provable fixes and hardening of the bd dolt pull unmergeable after multi-machine upgrade — migration 0043 forks the dependencies primary key (conditional DDL + DEFAULT (UUID())) #4259 gate are kept
    (re-attributed through this review); only clear no-op commits are reverted.

Proven regression tests for every kept real defect

For all 45 kept real-defect commits, a regression test now lives in this PR and is proven
born-failing
by mutation — reintroduce the specific defect into current code and the test goes
RED; restore the fix and it goes GREEN. 42 were verified against the tests the commits
already shipped; 3 were newly authored here because the shipped tests were characterization-only
or absent:

  • internal/lockfile TestIsLocked — detect a wrapped ErrLocked (guards the errors.Is fix; orig 1a83cb08c)
  • internal/storage/domain/db TestGetDescendantsFilteredByStatus — level-filter across edge + dotted branches (guards the dolt-2.1.6 named-CTE workaround; orig 341c7a5a4)
  • cmd/bd TestGOMODCACHENotUnderTestHomeGOMODCACHE pinned outside the temp test HOME (orig bc909834c)
Full proof ledger (45 defects)
short cluster regression test pkg test name(s) conf
33e71d21e doctor proven (existing) doctor TestShouldFlagTrackedFile_RuntimeArtifacts high
928f9ff2d doctor proven (existing) doctor TestCheckMigrationContentSkew_UnexpectedErrorWarns high
03cdc6c86 dolt-runtime proven (existing) dolt, issueops TestRecomputeIsBlockedAfterMerge_PreservesUpdatedAt high
0664ee7be dolt-runtime proven (existing) dolt TestInitSchemaOnDBWithRetryAndGate_GateErrorClassification high
1f8331d16 dolt-runtime proven (existing) dolt, versioncontrolops TestFKCascadeViolationRepairOnSQLPull; TestFKCascadeViolationRepairOnC high
2d5f11e5a dolt-runtime proven (existing) bd TestSharedServerEmbeddedMismatchDoesNotRewriteMetadata high
341c7a5a4 dolt-runtime AUTHORED (new) db TestGetDescendantsFilteredByStatus high
49a803050 dolt-runtime proven (existing) bd TestBulkDepAddCycleGateRollsBack; TestNewCycleThroughEdges high
6a1e7af9b dolt-runtime proven (existing) dberrors TestAncestorPKMismatch_CasingConsistency; TestAncestorPKMismatch_NonMa high
8095e4f4e dolt-runtime proven (existing) bd TestIsConfirmedNoRemote high
9264f0ec2 dolt-runtime proven (existing) issueops TestCycleThroughEdgesInGraph high
a768035af dolt-runtime proven (existing) doltserver TestDetectCorruptManifest_ReportsWithoutModifying high
ccadfce9f dolt-runtime proven (existing) dolt, versioncontrolops TestTryAutoResolveMergeConflicts_SchemaMigrationsTheirHashWins; TestTr high
d80ed8146 dolt-runtime proven (existing) bd TestIsConfirmedNoRemote high
1fe4993cf import-export proven (existing) embeddeddolt TestCreateIssuesRejectStaleUpserts high
33bf6d0c0 import-export proven (existing) embeddeddolt TestCreateIssuesRejectStaleUpserts/equal_timestamp_keeps_local_row; Te high
1a83cb08c proxied-server-uow AUTHORED (new) lockfile TestIsLocked high
27bbecbd1 proxied-server-uow proven (existing) db TestDomainDB/TestParityWispInclusion; TestDomainDB/TestParityReadyBloc high
29df600aa proxied-server-uow proven (existing) uow TestDoltServerTxRunnerAfterCommitErrorsInsteadOfPanicking high
439fb4eb1 proxied-server-uow proven (existing) bd TestEmbeddedInit/remote_behind_schema_gates_with_guidance; TestPrintBo high
4b0509c11 proxied-server-uow proven (existing) uow TestIsRetryableWarmupError high
794ff0790 proxied-server-uow proven (existing) uow TestDoltServerTxCommitFailureRollsBackBeforeRelease; TestDoltServerTxC high
8866e5b2b proxied-server-uow proven (existing) proxy TestReadAndDial_DeadPidfileWriterRejected; TestSpawnAndHandoff_ReapsOr high
914ac3c12 proxied-server-uow proven (existing) db TestDomainDB/TestParityReadyBlockedDescendants high
99c70904f proxied-server-uow proven (existing) uow TestNewDoltServerUOWProvider_RemoteMigrateGate_BlocksReopen high
9f531717d proxied-server-uow proven (existing) db, issueops TestScanIssue_StringTimestamps; TestIssueSelectColumns_MatchClassic high
b5cdb70ab proxied-server-uow proven (existing) bd TestCommandSupportsProxiedServer high
f1c72dbde proxied-server-uow proven (existing) doltutil TestPersistedRemotes high
f483ac2b4 proxied-server-uow proven (existing) bd TestPrintBootstrapRemoteBehindGuidance; TestPrintBootstrapRemoteBehind high
f880a985b proxied-server-uow proven (existing) bd TestNewProxiedServerUOWProvider_CorruptSidecarErrorsInsteadOfManagedFa high
12a47948c schema-migration proven (existing) schema, rowid TestRekeyAuxRowTableKeepsHeldTargetsStable; TestRekeyAuxRowTableIdempo high
1be8989fe schema-migration proven (existing) schema TestCheckRemoteMigrateGateWithRemoteCheck high
8ffcf698e schema-migration proven (existing) schema TestCheckRemoteMigrateGate high
969e27ead schema-migration proven (existing) embeddeddolt TestEmbeddedOpenForReadOnlyCommand_LenientGate high
e4f040741 schema-migration proven (existing) schema TestMigrationWorkNeededAddsContentHashColumnOnUpToDateDB; TestMigratio high
3f2b4e08d storage-sql proven (existing) embeddeddolt TestEmbeddedOpenReadOnly_SkipsGateAndMigrations high
8a92dacf4 storage-sql proven (existing) embeddeddolt TestEmbeddedOpenReadOnly_SkipsGateAndMigrations high
f3d31e444 storage-sql proven (existing) issueops TestSearchIssuesWithCountsHonorsSkipWisps; TestSearchIssuesWithCountsP high
001c6c258 sync-federation proven (existing) embeddeddolt, versioncontrolops TestEmbeddedMergeAndSettleFKCascadeRepair; TestEmbeddedMergeAndSettleR high
59ba57f02 sync-federation proven (existing) dolt, versioncontrolops TestFKCascadeRepairWithAutoResolvableConflicts high
7171f8f8e sync-federation proven (existing) embeddeddolt, versioncontrolops TestEmbeddedMergeAndSettleReportsOperatorConflicts high
792260c0a sync-federation proven (existing) tracker TestEnginePullDoesNotTreatPreviousPullAsLocalConflict high
99ed06b6f sync-federation proven (existing) dolt, issueops TestRecomputeIsBlockedAfterMerge_BlockerClosedRemotely; TestRecomputeI high
c708e00c3 sync-federation proven (existing) dolt TestPullFromSettlesFKCascadeViolations; TestPullFromReportsOperatorCon high
bc909834c test-only AUTHORED (new) bd TestGOMODCACHENotUnderTestHome high

Maintainer decisions (see the Removal Rationale comment for full per-commit analysis)

Git mechanics

Cleanup is done as git revert on this branch — no rewrite of published history. HEAD is
published and clones have replayed migrations; a history rewrite cannot un-run them and would
create a worse source-vs-DB split-brain.

Verification

  • make build / go build -tags gms_pure_go ./... — green
  • go vet over touched packages — clean
  • The 3 authored regression tests — pass (and proven RED under their mutations)
  • Full make test on the serverV2 revert — green (60 ok packages, 0 failures); domain/db,
    uow, dbproxy all ok.
  • Regression-free proof: any sandbox-only test flakiness observed (a cmd/bd package-level
    test-isolation issue with os.Setenv/os.Chdir) reproduces identically on untouched
    origin/main and on the BASE before the stream
    , so this branch introduces no new failures.
    Real CI (Main, Regression Tests) is green at the current tip.

Optional follow-up (not required): harden the cmd/bd suite by converting its unguarded
os.Setenv/os.Chdir to hermetic t.Setenv/t.Chdir.

CI Bot and others added 6 commits June 15, 2026 20:24
…only init ungate (bd-6dnrw.44)"

This reverts commit 1425e73.
…ion commands; attribute sweeps distinctly (bd-578h9.7)"

This reverts commit 81489b8.
For kept fixes whose shipped tests were characterization-only or absent, add
born-failing regression tests (verified RED when the defect is reintroduced,
GREEN with the fix in place):

- internal/lockfile: IsLocked must detect a *wrapped* ErrLocked — guards the
  errors.Is fix (was pointer-equality). (orig 1a83cb0)
- internal/storage/domain/db: GetDescendants with a level filter across edge
  and dotted-ID branches — guards the dolt 2.1.6 named-CTE analyzer workaround;
  the pre-existing dotted-orphans test used an empty filter and never built the
  predicate. (orig 341c7a5)
- cmd/bd: GOMODCACHE must be pinned outside the temp test HOME — guards the
  testMainInner module-cache fix. (orig bc90983)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The proxied / external-dolt-server subsystem (serverV2) is gated off and not
running yet. This reverts the unreviewed changes to its schema-independent
layers back to BASE 9a1c88b, while keeping the parts welded to the published
schema.

Reverted to BASE (26 files):
- internal/storage/dbproxy/*  (proxy/socket process mgmt: orphan-dolt reaping,
  pidfile discovery, endpoint/shutdown hardening)
- internal/storage/uow/*      (uow tx lifecycle, warmup-retry classification,
  provider changes)
- cmd/bd proxied glue: proxied_server, init_proxied_server, list_proxied_server,
  uow_factory, store_factory(+nocgo), and main.go proxied-mode dispatch hunks

Kept at HEAD (welded to the published migration 0051 / required by the data
layer, consistent with the keep-the-schema-stack decision):
- internal/storage/domain/db/* (mints app-side ids because 0051 dropped the DB
  default; queries the live migrated schema) -- left identical to HEAD
- the issueops Runner/DBTX widening + scan changes that HEAD domain/db requires

A strict serverV2->BASE revert is impossible without breaking main: domain/db is
welded to the published 0051 (BASE domain/db inserts without an id and relies on
the DEFAULT(UUID()) that 0051 removed). This reverts everything that is NOT so
welded.

Verified: full `make test` green (60 ok packages, 0 failures); domain/db, uow,
and dbproxy all ok.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@julianknutsen julianknutsen requested a review from a team as a code owner June 16, 2026 03:09
@julianknutsen

Copy link
Copy Markdown
Collaborator Author

Removal Rationale — commit-by-commit

Per-commit justification for the reverts on this branch, plus the maintainer-decision commits.
The cited issue references are not present in the tracker, so the diff and a born-failing test
are treated as the only source of truth.


Part 1 — The four reverts on this branch (no demonstrable defect)

1. 93b186dcftest(cmd/bd): Seam B cross-mode list parity → reverted by e409ab35e
A characterization test, not a fix. It asserts the already-shipped behavior of the
embedded-vs-proxied bd list paths (offset rejected in direct mode but honored proxied,
truncation rules). It passes against the existing code, so it neither reproduces nor guards a
defect — there is no "before" failure. Removing it loses no bug coverage.

2. 1425e7338test(ci): restore test-proxied-server-cmd lane behind a test-only init ungate → reverted by dac4fb49e
Re-enables a CI lane that was disabled earlier in the same stream and adds a test-only "init
ungate" so the gated (not-running) proxied-server can be exercised. No born-failing reproduction —
the test assertion was rewritten to match new expected behavior, not added as a failing repro.
CI/test scaffolding for the gated subsystem, not a fix.

3. 81489b8c1fix(cli): keep the autocommit sweep off read-only and inspection commands → reverted by 97f999a14
Self-referential: it fixes a condition inside the autocommit sweep that was introduced earlier
in this same stream
by d2b6cc525. The over-firing it guards against does not exist in the
baseline. Reverting it together with d2b6cc525 removes the whole speculative mechanism and nets
to zero.

4. d2b6cc525fix(autocommit): dirty-working-set safety net behind commandDidWrite → reverted by 548d7308e
Speculative/defensive, no demonstrated defect. No actual write path is identified that forgets
commandDidWrite; the change defends against a scenario never shown to occur. No reproduction, no
born-failing test. (Its companion 81489b8c1 then fixed a condition this very mechanism
introduced — see #3.)


Part 2 — Flagged maintainer decisions (all resolved: KEEP)

Honest framing: not all of these are "not needed." Two actually fix real weaknesses and carry
tests — they are kept.

33bf6d0c0fix(import): equal-timestamp upsert keeps the local row — KEEP
Carries born-from-this-commit regression tests (equal_timestamp_keeps_local_row,
equal_timestamp_empty_notes_does_not_wipe_local_notes, stale_incoming_does_not_reopen_closed_issue)
that assert real DB state and demonstrate a concrete clobber under the old >= comparison. It does
change equal-timestamp merge-idempotency behavior, so it is a real policy choice — kept as intended.

f1c72dbdefix(dolt): read persisted remotes from repo_state.json — KEEP (verify assumption)
The pre-image (the legit #4259/#4268 work, 1be8989fe) genuinely has the described weakness:
ListCLIRemotes shells out to dolt remote -v and the migrate-gate swallows any subprocess error,
so a transient failure reads as "no remote" and silently bypasses the gate. This commit reads
repo_state.json directly instead. Its live fix (dolt/federation.go + doltutil/remotes.go) is
kept at HEAD, so the live gate keeps the stronger check. It rests on an unverified assumption about
the dolt-2.x repo_state.json format — recommended follow-up: verify it and add a fallback/test.
(Its gated uow consumer reverted with the serverV2 layer — fine, that path isn't running.)

7ebf4df6afix(linear): unwire staleness hook from core — KEEP (loop in the feature owner)
A design reversal of a reviewed feature (ambient Linear-staleness warnings in every command's
pre-run and bd prime auto-pull). The objection (unpinned network I/O + version skew at session
start) is legitimate; it does reverse merged work, so the feature owner should ratify or re-land a
network-free version.

3fa29e325fix(fetch): drop session-killing DOLT_GC from failed-fetch path — KEEP (confirm with PR owner)
Reverses a DOLT_GC call added in reviewed PR #3326. dolt_gc invalidates all other open
sessions on the engine, so a failed fetch could kill concurrent in-flight connections — and the
tmp_pack_* cleanup it targeted already runs on teardown at BASE. Reverting this removal would
reintroduce that concurrency bug, so it is kept; confirm with the #3326 owner.

9a9da38c4docs(maintainer): add Merge Discipline and Review Requirements section — KEEP
Pure docs: +32 lines to PR_MAINTAINER_GUIDELINES.md, no code/behavior. Kept (or re-land under
attribution) — harmless; nothing to test.

@julianknutsen

Copy link
Copy Markdown
Collaborator Author

serverV2 reverted fixes — audit list

The serverV2 revert (8515879dc) restores the gated, not-running proxied/external-dolt-server
layer (dbproxy + uow + proxied cmd/bd glue) to BASE 9a1c88b63, removing the unreviewed
changes there. Most of these were real defects in the subsystem — they are listed here with
their proof so the owner can decide which to re-apply through proper review when serverV2 is
un-gated. (The domain/db data-layer fixes were kept at HEAD — they are welded to published
migration 0051 — so they are not in this list.)

proven = confirmed a real bug by mutation: reintroduce the defect into current code and the
listed test goes RED; restore the fix and it goes GREEN.

Commit Area Real bug? The defect that was fixed (now removed) Guard test
1a83cb08c proxied-cmd ✅ proven In internal/lockfile/lock.go, reverted IsLocked from return errors.Is(err, errProcessLocked) to the pre-fix pointer-equality return err == errProcessLocked. TestIsLocked
29df600aa dbproxy uow ✅ proven In internal/storage/uow/doltserver_tx.go, releaseConn(): re-added the line t.conn = nil after _ = t.conn.Close() (reverting the fix), so the pinned conn reference is dropped on release/commit. This is exactly the defect: a use-after-com TestDoltServerTxRunnerAfterCommitErrorsInsteadOfPanicking
4b0509c11 uow ✅ proven In internal/storage/uow/errors.go, reverted isRetryableWarmupError to the pre-fix behavior: deleted the net.Error/driver.ErrBadConn/mysql.ErrInvalidConn/io.EOF/ECONNREFUSED/ECONNRESET branches so the function returns true only for isSeriali TestIsRetryableWarmupError
794ff0790 uow ✅ proven In internal/storage/uow/doltserver_tx.go, reverted doltServerTx.Commit's DOLT_COMMIT error handling back to the pre-fix behavior: replaced the if err != nil { ROLLBACK; poisonConn on rollback failure; return err } return nil block with th TestDoltServerTxCommitFailureRollsBackBeforeRelease; TestDol
8866e5b2b dbproxy ✅ proven Two separate single-spot anti-fixes against current internal/storage/dbproxy/proxy/endpoint.go, each verified independently: / Item 10 (discovery trusting stale pidfiles): in readAndDial(), deleted the dead-writer guard `if pf.Pid <= 0 || TestReadAndDial_DeadPidfileWriterRejected; TestSpawnAndHando
99c70904f uow ✅ proven In internal/storage/uow/dolt_sql_provider.go, inside doltSQLProvider.initSchema (the proxied-server store-open path), I deleted the remote-migrate gate guard — the entire `if err := schema.CheckRemoteMigrateGateWithRemoteCheck(ctx, conn, ha TestNewDoltServerUOWProvider_RemoteMigrateGate_BlocksReopen
b5cdb70ab proxied-cmd ✅ proven In cmd/bd/proxied_server.go, replaced the body of commandSupportsProxiedServer's final statement return proxiedServerCommands[cmd.Name()] with return true. This reintroduces the original defect: the PersistentPreRun guard no longer reje TestCommandSupportsProxiedServer
f880a985b proxied-cmd ✅ proven In cmd/bd/uow_factory.go, reverted both error-propagation guards to the original swallowing form: persisted, _ := configfile.Load(beadsDir) (dropping the if err != nil { return ... "workspace config ... unreadable" } block) and `info, _ TestNewProxiedServerUOWProvider_CorruptSidecarErrorsInsteadO
1825cf357 proxied-cmd — plausible-real-bug Claims the proxied --tree --parent descendants CTE only walked parent-child edges and silently dropped dotted-ID orphan children (classic's ParentID LIKE fallback), and that gatherProxiedHierarchical failed outright when the tree root was a

Recommendation: the proven rows are genuine bugs (corrupt-sidecar abort, open-tx connection
recycled into the pool, warmup-retry misclassification, orphaned-dolt reaping after proxy SIGKILL,
the proxied-path migrate-gate hole, nil-store guard, use-after-commit panic) and should come back
through a reviewed PR when the proxied server is un-gated. Nothing here affects the running
classic/embedded path — full make test is green after the revert.

@julianknutsen

Copy link
Copy Markdown
Collaborator Author

Maintainer decisions — resolved

All flagged decisions are KEEP (no further reverts):

@julianknutsen

Copy link
Copy Markdown
Collaborator Author

We are soft reverting changes in the unreleased serverV2 feature in order to expedite development and reduce merge issues. All changes have been thoroughly documented in this PR for review and verification before serverv2 goes live. @coffeegoddd @timsehn

@julianknutsen julianknutsen merged commit a59e753 into main Jun 16, 2026
102 checks passed
@maphew

maphew commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Wow! 😳 What a major effort. Let me know if there's anything concrete I can do to assist in the follow up!

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.

2 participants