feat(relayfile): language-agnostic three-way line merge (Phase 5)#375
Conversation
…nes-v1) Phase 5 of goal-B: the user pushed back on Phase 3/4's Go-only symbol-level merge — "why does this need to be language specific?" — and they're right. AST-based merge was one way to get symbol-level auto-merge, not the only way, and it doesn't generalize (every language needs its own parser; there is no mature pure-Go TypeScript parser, and every option investigated — vendoring esbuild's internal Go parser, a Node sidecar around tsc, tree- sitter+CGo — was a real new dependency or architecture change for a single language, with no path to covering Python/JSON/Markdown/etc. either). MergeStrategyThreeWayLines (merge_lines.go) is the standard diff3-style three-way merge — the same algorithm git and every other VCS use — over raw lines, not any language's AST. It works identically for TypeScript, Python, JSON, Markdown, or anything else, with zero new dependencies: a bounded (maxMergeDiffLines=3000/side) full LCS DP table, memory-safe by construction rather than by the more involved Hirschberg linear-space technique, since the bound keeps the naive table under ~36MB even at its limit. Resolution walks "sync points" (base lines unchanged in both mine and theirs) and between them takes whichever side actually changed something relative to base, or — if both changed it identically — either; only a genuine divergence (both changed the same span to different content) is a conflict. Same all-or-nothing safety invariant as the Go strategy: any conflict means nothing is written. Store.MergeFile now dispatches on req.Strategy: existing Go-AST path unchanged for go-top-level-functions-v1, new line-based path for three-way-lines-v1 (no separate extraction step — it works directly on the raw base/mine/theirs strings). Also fixes two latent bugs the second strategy exposed: commitMergeResult and findExistingMergeOpLocked both unconditionally reported MergeStrategyGoTopLevelFunctions in their MergeResponse regardless of which strategy was actually used — harmless while only one strategy existed, wrong the moment a second one did. Both now report req.Strategy / the strategy stashed in the op's ProviderResult metadata. New regression test (TestMergeFileDedupedRetryReportsCorrectStrategy) covers the ContentIdentity dedup path directly, which had no prior test coverage at all even for the original strategy. Tests written alongside the algorithm (not after): 15 pure-function cases covering disjoint/same-line/identical-change/insertion/deletion/same- position-insertion-conflict/no-trailing-newline/oversized-input, plus 6 store-integration tests (end-to-end success, conflict-leaves-file- unchanged, unverifiable-base, oversized-content, no-file-extension- restriction, dedup) — the latter deliberately exercised through a .ts file throughout to prove the "any language" claim concretely, not just in prose. Mountsync's mount-rollout wiring (broadening eligibility beyond .go, switching the strategy it requests, updating tests) is scoped as a follow-up commit, split to Codex per the established pattern for this initiative.
…e strategy Mount rollout now requests three-way-lines-v1 instead of go-top-level-functions-v1, and mountRolloutMergeEligible is broadened from ".go files only" to any text-like file (isTextLikeContentType( detectContentType(remotePath))) — TypeScript, Python, JSON, Markdown, extensionless files like README, etc. — while still correctly excluding binary content (images, archives). Found and fixed a real, independent bug along the way: Go's stdlib MIME registry maps ".ts" to "video/mp2t" (MPEG transport stream), not TypeScript — which would have made every .ts file classified as binary and therefore ineligible for mount rollout, exactly backwards from the goal of this phase. detectContentType now special-cases ".ts" to "text/typescript", the same way it already special-cased ".md"/".markdown". Also updates the two content-type fallbacks that hardcoded "text/x-go" (inside attemptMountRolloutMerge and HTTPClient.MergeFile) to the neutral "text/plain", the OpenAPI spec's strategy enum (now lists both merge strategies), and the TypeScript SDK's MergeFileInput/MergeFileResponse strategy union type + its own contentType default. New test coverage: TestMountRolloutMergeEligible (table test proving .ts/ .py/.json/extensionless-README are now eligible, .png/.zip are correctly still not), and TestMountRolloutConflictPathAutoMergesTypeScriptFile (the TypeScript sibling of the existing Go integration test, through the real syncer entrypoints, not a direct unit call). Existing tests' mock mergeFileFunc strategy assertions updated from MergeStrategyGoTopLevelFunctions to MergeStrategyThreeWayLines, since that's what mount rollout now sends. Built by Codex (finn-mac-mini) against the store-layer work from the prior commit; reviewed and independently re-validated end to end before merging into this branch: go build/vet/test ./... (including -race on the new and existing mount-rollout tests), TS SDK build/typecheck/233 tests, and scripts/check-contract-surface.sh, all green.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa5dbaeaf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case mineText == theirsText: | ||
| // Includes the common "neither side changed this span" case, | ||
| // and "both sides made the identical change" — neither is a | ||
| // real conflict. | ||
| output = append(output, mineRegion...) |
There was a problem hiding this comment.
When repeated lines make the LCS alignment ambiguous, treating equal regions as the same edit can silently discard one side's insertion. For example, with base x\na\ny\n, mine x\na\na\ny\n, and theirs x\nb\na\na\ny\n, the independent insertions should produce x\nb\na\na\na\ny\n (as git merge-file does), but this branch emits only two a lines because both trailing regions contain a\n and are collapsed here. Detect ambiguous alignments or merge explicit edit hunks rather than deduplicating solely by regional text equality.
Useful? React with 👍 / 👎.
| // A neutral default matches the server and works for either merge | ||
| // strategy when the caller did not provide a more specific type. | ||
| contentType: input.contentType ?? "text/plain", |
There was a problem hiding this comment.
For existing go-top-level-functions-v1 callers that omit the optional contentType, this changes the request from text/x-go to text/plain. A successful merge overwrites the stored file's content type with this value and propagates it to provider writeback, so upgrading the SDK silently changes metadata for Go files. Select the default based on the strategy (retaining text/x-go for the existing Go strategy) rather than applying the neutral default to both.
Useful? React with 👍 / 👎.
…contentType regression - linesThreeWayMerge could silently DROP one side's independent insertion when repeated/duplicated lines near an edit made the LCS alignment genuinely ambiguous (P1, Codex review). Concrete example: base "x\na\ny\n", mine independently inserts a second "a", theirs independently inserts "b" AND a second "a" — a single greedy-leftmost alignment can attribute both sides' inserted "a" to the same logical gap purely because they're textually identical, collapsing two real insertions into one and losing content. `git merge-file` resolves this correctly by picking a different, non-colliding alignment; rather than replicate that exact heuristic, this now computes the merge TWICE — once preferring the earliest valid occurrence when a repeated line admits a choice (lcsMatchPreferFirst, the existing algorithm), once preferring the latest (new lcsMatchPreferLast, backtracking a prefix-LCS table from the end) — and only trusts the result when both agree on content and conflict-freeness. Disagreement means the alignment was ambiguous, and the safe response is a conflict, never a guess — same fail-closed philosophy as every other ambiguity case in this file. Roughly doubles the algorithm's cost (measured 138ms at the 3000-line bound, worst case) for a rare, already-bounded operation. New regression test (TestLinesMergeAmbiguousDuplicateInsertionsConflict) reproduces the reviewer's exact scenario and confirms it now conflicts instead of silently losing content. - The TypeScript SDK's mergeFile default contentType regressed existing go-top-level-functions-v1 callers (P2, Codex review): changing the blanket default from "text/x-go" to "text/plain" meant an existing Go caller that omits contentType would silently get its stored file's content type (and provider-writeback metadata) rewritten on every merge. The default is now chosen per-strategy: "text/x-go" for go-top-level-functions-v1 (restores prior behavior), "text/plain" for three-way-lines-v1 (the correct neutral default for a language-agnostic strategy). New test (mergeFile defaults contentType per strategy when omitted) locks this in. Full suite green: go build/vet/test ./... (including -race on the merge package), TS SDK build/typecheck/234 tests.
Phase 5 (language-agnostic three-way line merge, PR #375) landed and merged since this doc was last updated after Phase 4. Adds gap #6 (the Go-only merge restriction, fixed), renumbers the CRDT/OT gap, and updates the verdict/recommendation to reflect all six gaps from the assessment being closed — no remaining scope gap from the original assessment.
1 participant
Summary
Phase 5 of the goal-B real-time multi-agent collaboration work (follows #370-#374). The user pushed back on Phase 3/4's design directly: "why does this need to be language specific?" — correctly. AST-based symbol-level merge (
go-top-level-functions-v1) only works for Go because it needs a Go parser, and there's no equivalent mature parser for TypeScript/Python/etc. in Go — every option investigated (vendoring esbuild's internal Go parser, a Node sidecar around the real TypeScript compiler, tree-sitter+CGo) was a real new dependency or architecture change, and only solved one language.This adds
MergeStrategyThreeWayLines(three-way-lines-v1) — a diff3-style three-way line merge, the same class of algorithm git and every VCS use. It operates on raw lines, not any language's AST, so it works identically for TypeScript, Python, JSON, Markdown, or anything else, with zero new dependencies.The algorithm
internal/relayfile/merge_lines.go: a bounded (maxMergeDiffLines = 3000lines/side) full LCS DP table (memory-safe by construction — ~36MB worst case — rather than needing the more involved Hirschberg linear-space technique). Resolution walks "sync points" (base lines unchanged in both mine and theirs) and between them takes whichever side actually changed something relative to base; if both changed it identically, that's not a conflict either — only a genuine divergence (both sides changed the same span to different content) conflicts. Same all-or-nothing safety invariant as the Go strategy: any conflict means nothing is written, ever.Store.MergeFilenow dispatches onreq.Strategy: the Go-AST path is completely unchanged forgo-top-level-functions-v1; the new strategy has no file-extension restriction, bounded only by content size/line count.Also fixes two latent bugs the second strategy exposed:
commitMergeResultandfindExistingMergeOpLockedboth unconditionally reportedMergeStrategyGoTopLevelFunctionsin theirMergeResponseregardless of which strategy was actually used — harmless while only one strategy existed, wrong the moment a second one did.Mount rollout
mountRolloutMergeEligibleis broadened from ".go files only" to any text-like file (isTextLikeContentType(detectContentType(remotePath))) — TypeScript, Python, JSON, Markdown, extensionless files, etc. — while still excluding binary content.Found along the way: Go's stdlib MIME registry maps
.tstovideo/mp2t(MPEG transport stream), not TypeScript — which would have made every.tsfile classified as binary and therefore ineligible, exactly backwards from this phase's goal.detectContentTypenow special-cases.ts→text/typescript, the same way it already special-cases.md/.markdown.Tests
.tsfile throughout to prove the "any language" claim concretely, not just in prose.TestMountRolloutMergeEligible: table test proving.ts/.py/.json/extensionless-READMEare now eligible,.png/.zipcorrectly remain ineligible.TestMountRolloutConflictPathAutoMergesTypeScriptFile: the TypeScript sibling of the existing Go integration test, through the real syncer entrypoints.Full suite green:
go build/vet/test ./...(including-raceon new and existing merge/mount-rollout tests), TS SDK build/typecheck/233 tests,scripts/check-contract-surface.sh.Split
Core algorithm + store dispatch: written directly (safety-critical, novel). Mountsync eligibility broadening + HTTP/OpenAPI/SDK touch-ups + test coverage: built by Codex against the store-layer work, reviewed and independently re-validated end to end before merging into this branch.
Test plan
go build ./...,go vet ./...,go test ./...-race.tsfile through the real mount daemon on two physical machines (to follow before merge, per this initiative's standing verification requirement)