feat(relayfile): symbol-level merge for concurrent Go edits (Phase 3)#373
Conversation
….MergeFile) Store-layer half of Phase 3 (docs/multi-agent-collaboration-assessment.md). HTTP handler, OpenAPI spec, SDK, and mountsync integration land in a follow-up commit. Two agents editing DIFFERENT functions in the SAME Go file today still hit a whole-file conflict (correct, per Phase 0 — no silent data loss — but it forces a full discard-and-redo even though the actual changes don't overlap). This adds an opt-in merge strategy that auto-applies when the changes are provably disjoint at function granularity, and falls back to a plain conflict — writing nothing — the instant anything is ambiguous. Design, reconciled from two independent proposals (mine and a second pass from Codex) before any code was written — Codex's version was adopted almost entirely; see the design docs for the full reconciliation: - internal/relayfile/merge_go.go: goExtractUnits parses a Go file with go/parser and splits it into a "prologue" (package/imports/types/vars before the first function) plus one unit per top-level function, matched across versions by a stable key (name, or receiver-type-qualified for methods) rather than byte position. Deliberately restrictive for v1: any non-func top-level declaration AFTER the first function makes the file ineligible (falls back to plain conflict) rather than guessing at a more complex layout. goThreeWayMerge takes base/mine/theirs extractions and, per unit, auto-resolves when only one side changed it (or neither), and conflicts when both did — including same-function edits to different lines, which still conflict (function is the unit of collaboration, not line ranges). func init() is never auto-merged regardless of what changed, since its ordering is semantically significant. Two independently-added functions with the same name conflict unless byte-identical. - internal/relayfile/merge.go: Store.MergeFile wires this into the store. Verifies the caller-supplied base content against a new in-memory, bounded revision->content-hash provenance ledger (recordRevisionProvenanceLocked, hooked into recordWriteWithContentIdentityLocked so every write path — WriteFile, BulkWrite, fork commits — feeds it automatically) before trusting it for the merge; a caller can't fabricate a base to trick the algorithm. Snapshots live content under a read lock, computes the (parser-heavy) merge with no lock held at all, then re-verifies nothing moved under a write lock before committing — never holds the store's single global mutex across parsing/diffing. If the live revision moved between snapshot and commit, recomputes against fresh state and retries, bounded by maxMergeCommitAttempts. Non-negotiable safety invariant, enforced at multiple layers: a merge request either writes one fully validated merged file, or it writes nothing — provenance miss, hash mismatch, parse failure, unsupported file layout, or any unresolved conflict all fail closed with zero mutation. The merge result is re-parsed as a final self-check immediately before commit, on top of the same invariant already proven by TestMergeResultsAreAlwaysValidGo in isolation. 23 new tests across three files: the pure extraction/merge algorithm (merge_go_test.go — distinct functions, same-function-different-lines, new-function-plus-unrelated-change, delete-vs-modify, delete-vs-unchanged, both-delete-same-unchanged, init() always-conflicts, file-scope both-vs-one-sided, both-add-same-vs-different, and a dedicated always-valid-Go safety test that caught a real concatenation bug during development), and the full Store.MergeFile integration including a concurrent-commit-retry race test run with -race.
HTTP + SDK half of Phase 3 (docs/multi-agent-collaboration-assessment.md),
built on the store-layer Store.MergeFile primitive.
- POST /v1/workspaces/{workspaceId}/fs/merge, handleMergeFile with full
error mapping: merge_conflict -> 409 with per-unit conflict details,
merge_base_unavailable -> 409, merge_ineligible -> 422 with reason,
invalid input -> 400. Path-scoped fs:write authorization enforced the
same way handleWriteFile enforces it, with ACL treating the target as
always-existing (MergeFile only ever operates on a live file).
- openapi/relayfile-v1.openapi.yaml: documents the endpoint and
MergeRequest/MergeResponse/MergeConflictDetail schemas.
- TypeScript SDK: mergeFile() method, matching types, MergeConflictError
class (carrying conflicts + currentRevision) wired into throwForError
for merge_conflict, plus merge_base_unavailable/merge_ineligible
surfaced as typed RelayFileApiError with the right code.
- HTTP tests: full auto-merge round trip, per-unit conflict detail
verification (exact base/mine/theirs text), no-mutation-on-conflict
and no-mutation-on-unavailable-base guarantees verified by reading
the file back, ineligible-strategy and invalid-input mapping, and a
dedicated path-scoped authorization suite (missing scope, wrong-path
grant, correct-path grant).
- SDK tests: mergeFile() request shape, MergeConflictError branch, and
the two fallback codes staying typed RelayFileApiError.
Co-authored-by: Codex <noreply@openai.com>
|
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: 32 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 (5)
📝 WalkthroughWalkthroughAdds a three-way merge API for eligible Go files, including function-level conflict detection, revision verification, concurrent commit retries, path-scoped authorization, OpenAPI definitions, and TypeScript SDK support. ChangesFilesystem merge feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPAPI
participant RelayfileStore
participant GoMergeEngine
Client->>HTTPAPI: POST /fs/merge with base and proposed content
HTTPAPI->>HTTPAPI: authorize target path with fs:write
HTTPAPI->>RelayfileStore: call MergeFile
RelayfileStore->>GoMergeEngine: perform three-way function merge
GoMergeEngine-->>RelayfileStore: merged content or conflicts
RelayfileStore-->>HTTPAPI: target revision or typed error
HTTPAPI-->>Client: HTTP response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3de3386ea9
ℹ️ 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".
| start = docStart | ||
| } | ||
| } | ||
| end := offset(fn.End()) |
There was a problem hiding this comment.
When a valid Go file contains a standalone comment between functions or after the final function, fn.End() excludes that comment and it is not attached as fn.Doc; reconstruction concatenates only these captured function texts. Any otherwise-successful merge therefore silently deletes such comments even when neither side changed them, and the parse validation cannot detect the loss because the result remains valid Go.
Useful? React with 👍 / 👎.
| if eventType != "file.deleted" { | ||
| contentHash = storedContentHashForFile(ws.Files[path]) | ||
| } | ||
| s.recordRevisionProvenanceLocked(workspaceID, path, revision, contentHash) |
There was a problem hiding this comment.
The sole provenance-recording call added here is reached only through recordWriteWithContentIdentityLocked, while provider ingest mutates files in applyProviderUpsertLocked and emits events directly through appendWorkspaceEventLocked. If an agent reads a provider-synced Go revision, another writer changes it, and the agent then attempts a merge from that base, the ledger cannot find the provider revision and always returns merge_base_unavailable, making this feature unusable for provider-origin files.
Useful? React with 👍 / 👎.
| func (s *Store) commitMergeResultLocked(workspaceID, path string, req MergeRequest, mergedContent, expectedRevision string) (MergeResponse, bool, error) { | ||
| s.mu.Lock() | ||
| ws := s.ensureWorkspaceLocked(workspaceID) | ||
| if ws.Revision != expectedRevision { |
There was a problem hiding this comment.
This compares the workspace-wide revision rather than the target file revision, so any unrelated write in the same workspace invalidates the merge snapshot. In a busy workspace, five writes to other paths while the Go file is being parsed can exhaust maxMergeCommitAttempts and turn a conflict-free merge into a 500 even though the target file never moved; the commit guard should track the snapshotted target file instead.
Useful? React with 👍 / 👎.
| return MergeResponse{ | ||
| TargetRevision: op.Revision, | ||
| Strategy: MergeStrategyGoTopLevelFunctions, | ||
| }, true |
There was a problem hiding this comment.
When a response is dropped and the caller retries with the same contentIdentity, this branch returns only targetRevision and strategy, leaving baseRevision and mergedAgainstRevision empty even though the documented MergeResponse requires them and defines their provenance semantics. Preserve enough merge metadata with the operation, or reconstruct it from the retried request, so retries return the same complete response as the original call.
AGENTS.md reference: AGENTS.md:L171-L180
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
internal/relayfile/merge_go_test.go (1)
56-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a comment that is not attached as a func doc.
TestGoExtractUnitsDocCommentTravelsWithFunccovers the attached case, but a banner comment sitting between two functions (or after the last one) is captured by neither the prologue nor any unit — see the related finding ininternal/relayfile/merge_go.go. A round-trip assertion (extract → goThreeWayMerge(base, base, base)preserves every comment) would lock the invariant down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/relayfile/merge_go_test.go` around lines 56 - 81, Add a test covering a standalone banner comment between functions or after the final function, verifying extraction does not drop it from the prologue or units. Use a round-trip assertion through goThreeWayMerge with identical base inputs to confirm every comment is preserved, alongside the existing TestGoExtractUnitsDocCommentTravelsWithFunc coverage.openapi/relayfile-v1.openapi.yaml (1)
259-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplitting
merge_conflictandmerge_base_unavailableunder one 409oneOfis fine, but neither branch discriminates.Both variants pin
codeto a single-value enum, so validators can resolve them; consider addingdiscriminator: {propertyName: code}so generated clients narrow the union automatically instead of falling back tounknown/manual checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/relayfile-v1.openapi.yaml` around lines 259 - 274, Add a discriminator to the 409 response schema’s oneOf definition, using the code property to distinguish MergeConflictErrorResponse from MergeBaseUnavailableErrorResponse. Keep both existing schema references and the current 409 response behavior unchanged.internal/relayfile/merge_go.go (1)
159-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
recvTypeStringmisses*ast.IndexListExpr, andfuncKeylacks the nil-NameguardfuncDisplayNamehas.A receiver with two or more type parameters (
func (f *Foo[T, U]) Bar()) parses toIndexListExpr, so it degrades to the"?"fallback and produces the keymethod:?.Bar— correctness is preserved by the duplicate-key refusal, but such files become needlessly ineligible. Also, Line 161 dereferencesfn.Name.Nameunguarded while Line 149 treats a nilNameas possible.♻️ Proposed change
func funcKey(fn *ast.FuncDecl) string { + name := "<anonymous>" + if fn.Name != nil { + name = fn.Name.Name + } if fn.Recv == nil || len(fn.Recv.List) == 0 { - return "func:" + fn.Name.Name + return "func:" + name } - return "method:" + recvTypeString(fn.Recv.List[0].Type) + "." + fn.Name.Name + return "method:" + recvTypeString(fn.Recv.List[0].Type) + "." + name } func recvTypeString(expr ast.Expr) string { switch t := expr.(type) { case *ast.StarExpr: return "*" + recvTypeString(t.X) case *ast.Ident: return t.Name case *ast.IndexExpr: return recvTypeString(t.X) + case *ast.IndexListExpr: + return recvTypeString(t.X) default: return "?" } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/relayfile/merge_go.go` around lines 159 - 180, Update recvTypeString to handle *ast.IndexListExpr by recursively resolving its base receiver expression, matching the existing *ast.IndexExpr behavior so multi-parameter generic receivers produce stable method keys. Update funcKey to guard fn.Name before dereferencing it, reusing the nil-name handling established by funcDisplayName and preserving the existing function/method key formats for valid declarations.internal/relayfile/merge_store_test.go (1)
101-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEviction of a real-but-aged base revision isn't covered.
Both subtests exercise "never recorded" / "hash mismatch". The ring-buffer eviction path (a base revision that was recorded but has since aged past
maxRevisionProvenancePerPath) is a distinct branch, and the comment at Lines 210-213 of the race test claims it is covered here. A subtest that writes 21+ revisions and then merges against the seed would close the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/relayfile/merge_store_test.go` around lines 101 - 136, The TestMergeFileRejectsUnverifiableBase test covers unknown and hash-mismatched revisions but not an evicted recorded revision. Add a subtest that writes at least 21 revisions to the same workspace and path so the seed revision ages beyond maxRevisionProvenancePerPath, then merge using the seed’s BaseRevision and BaseContent and assert ErrMergeBaseUnavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/relayfile/merge_go.go`:
- Around line 111-143: Preserve unattached comments in the function region
during extraction by extending each following unit’s start backward to include
the gap after the previous declaration, including any trailing comments after
the final function. Update the extraction logic around goExtractedFile and
goFuncUnit so comments are not dropped while keeping function boundaries and
duplicate-key validation unchanged.
In `@internal/relayfile/merge_store_test.go`:
- Around line 220-249: Make the concurrent churn test deterministic by reducing
its write count well below maxMergeCommitAttempts, while still ensuring the
merge encounters retries. Replace the churn goroutine’s swallowed WriteFile
error return with an error channel, and have the test receive and fail on that
error after waiting for the goroutine. Preserve the existing MergeFile success
assertion and cleanup behavior.
In `@internal/relayfile/merge.go`:
- Around line 239-245: Rename commitMergeResultLocked to commitMergeResult and
update all callers and references accordingly. Preserve its existing behavior of
acquiring the write lock internally, and retain or update the doc comment to
explicitly state that commitMergeResult takes the write lock.
- Around line 141-151: Update merge CAS to use the target file revision rather
than the workspace-wide revision: capture the live file’s revision alongside
liveContent in the merge flow, pass it through the merge result, and make
commitMergeResultLocked compare the current ws.Files[path].Revision against that
captured value. Preserve MergedAgainstRevision as ws.Revision for reporting.
- Around line 229-232: Update the deduplicated retry path that returns
MergeResponse to populate all required fields, setting BaseRevision from the
request and MergedAgainstRevision from the revision recorded on the operation.
Ensure the original merge flow stores the merged-against revision on op so
replayed responses match the initial response.
In `@internal/relayfile/store.go`:
- Around line 548-556: Bound revisionProvenance across paths, not only within
each path. Update the revisionProvenance storage and its write/delete handling
so stale path entries are removed when a file is deleted or a workspace is torn
down, or enforce a global cap/LRU across keys while preserving the existing
per-path bound and fail-closed provenance behavior.
In `@packages/sdk/typescript/src/client.ts`:
- Around line 1606-1626: Update mergeFile to default the optional
MergeFileInput.contentType to "text/x-go" when constructing the request body,
while preserving any explicitly supplied contentType. Ensure the default is
applied before the merge request is sent and the existing cache eviction and
response handling remain unchanged.
---
Nitpick comments:
In `@internal/relayfile/merge_go_test.go`:
- Around line 56-81: Add a test covering a standalone banner comment between
functions or after the final function, verifying extraction does not drop it
from the prologue or units. Use a round-trip assertion through goThreeWayMerge
with identical base inputs to confirm every comment is preserved, alongside the
existing TestGoExtractUnitsDocCommentTravelsWithFunc coverage.
In `@internal/relayfile/merge_go.go`:
- Around line 159-180: Update recvTypeString to handle *ast.IndexListExpr by
recursively resolving its base receiver expression, matching the existing
*ast.IndexExpr behavior so multi-parameter generic receivers produce stable
method keys. Update funcKey to guard fn.Name before dereferencing it, reusing
the nil-name handling established by funcDisplayName and preserving the existing
function/method key formats for valid declarations.
In `@internal/relayfile/merge_store_test.go`:
- Around line 101-136: The TestMergeFileRejectsUnverifiableBase test covers
unknown and hash-mismatched revisions but not an evicted recorded revision. Add
a subtest that writes at least 21 revisions to the same workspace and path so
the seed revision ages beyond maxRevisionProvenancePerPath, then merge using the
seed’s BaseRevision and BaseContent and assert ErrMergeBaseUnavailable.
In `@openapi/relayfile-v1.openapi.yaml`:
- Around line 259-274: Add a discriminator to the 409 response schema’s oneOf
definition, using the code property to distinguish MergeConflictErrorResponse
from MergeBaseUnavailableErrorResponse. Keep both existing schema references and
the current 409 response behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e87c8f18-0e34-4a1b-b089-b631e6060c9e
📒 Files selected for processing (16)
.trajectories/compacted/compact_e4w7ic7jakya_2026-07-25.json.trajectories/compacted/compact_e4w7ic7jakya_2026-07-25.mdinternal/httpapi/server.gointernal/httpapi/server_test.gointernal/relayfile/merge.gointernal/relayfile/merge_go.gointernal/relayfile/merge_go_test.gointernal/relayfile/merge_store_test.gointernal/relayfile/store.goopenapi/relayfile-v1.openapi.yamlpackages/sdk/parity.jsonpackages/sdk/typescript/src/client.test.tspackages/sdk/typescript/src/client.tspackages/sdk/typescript/src/errors.tspackages/sdk/typescript/src/index.tspackages/sdk/typescript/src/types.ts
| ws := s.workspaces[workspaceID] | ||
| var liveContent string | ||
| var liveExists bool | ||
| var currentRevision string | ||
| if ws != nil { | ||
| if f, exists := ws.Files[path]; exists { | ||
| liveContent = f.Content | ||
| liveExists = true | ||
| } | ||
| currentRevision = ws.Revision | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Merge CAS uses the workspace revision, so unrelated writes force retries and can fail an otherwise-clean merge.
currentRevision is ws.Revision (workspace-wide), and commitMergeResultLocked rejects the commit whenever it moved (Line 242). Any concurrent write to any other path in the workspace invalidates the attempt, so on a busy workspace a merge that has no contention on its own file can burn all 5 attempts and return the "did not converge" error. Consider CASing on the target file's revision instead, keeping MergedAgainstRevision as the workspace revision for reporting.
♻️ Sketch
var liveContent string
var liveExists bool
var currentRevision string
+ var liveFileRevision string
if ws != nil {
if f, exists := ws.Files[path]; exists {
liveContent = f.Content
liveExists = true
+ liveFileRevision = f.Revision
}
currentRevision = ws.Revision
}and compare ws.Files[path].Revision against liveFileRevision inside commitMergeResultLocked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/relayfile/merge.go` around lines 141 - 151, Update merge CAS to use
the target file revision rather than the workspace-wide revision: capture the
live file’s revision alongside liveContent in the merge flow, pass it through
the merge result, and make commitMergeResultLocked compare the current
ws.Files[path].Revision against that captured value. Preserve
MergedAgainstRevision as ws.Revision for reporting.
| // revisionProvenance is an in-memory-only, bounded ledger of | ||
| // (workspaceID, path, revision) -> content hash, used by MergeFile to | ||
| // verify a caller-supplied merge base actually matches what was really | ||
| // at that revision before trusting it for a three-way merge. Keyed by | ||
| // workspaceID+"\x00"+path; not persisted (a restart just means an | ||
| // in-flight merge citing a pre-restart revision safely falls back to | ||
| // merge_base_unavailable, the same fail-closed behavior as any other | ||
| // provenance miss). | ||
| revisionProvenance map[string][]revisionProvenanceEntry |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
revisionProvenance is bounded per path but unbounded in the number of paths.
Each (workspaceID, path) key retains up to maxRevisionProvenancePerPath entries forever — nothing prunes on file delete or workspace teardown, and deletes only append a Deleted entry (Line 3814). A bulk import that writes tens of thousands of files leaves that ledger resident for the process lifetime, for paths that will never be merge bases. Consider a global cap/LRU across keys, or dropping the key when the path is deleted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/relayfile/store.go` around lines 548 - 556, Bound revisionProvenance
across paths, not only within each path. Update the revisionProvenance storage
and its write/delete handling so stale path entries are removed when a file is
deleted or a workspace is torn down, or enforce a global cap/LRU across keys
while preserving the existing per-path bound and fail-closed provenance
behavior.
codex review of PR #373 found several real issues in the store-layer merge implementation. Fixed: - [P2, most important] Merge commit CAS compared the whole-workspace revision counter instead of the target file's own revision, so any unrelated write to a different path in the workspace could force a retry or exhaust maxMergeCommitAttempts on a merge that had no real contention on its own file. This directly re-introduced the exact mistake Phase 1 fixed for forks (workspace-wide instead of per-path CAS) -- fixed the same way, comparing ws.Files[path].Revision. - [P1] Comments not attached to any function as a doc comment -- a standalone banner comment between two functions, or anything after the last function -- were captured by neither the prologue nor any unit, so a successful (non-conflicting) merge silently deleted them, and the merge's own always-valid-Go self-check couldn't catch the loss since the result still parsed fine without them. Fixed by making extraction gapless: each function's span now starts exactly where the previous one ended (capturing any comment in between), and the last function's span extends to EOF. This introduced a side effect -- the same function's captured Text can now vary by leading/trailing whitespace alone depending on whether it happens to be first/last in a given version -- fixed by comparing with textsDiffer (trims whitespace, not comment content) instead of raw Text equality for all change-detection, while reconstruction still uses the raw untrimmed Text. - [P2] A deduplicated retry (same ContentIdentity) returned a MergeResponse with empty BaseRevision/MergedAgainstRevision even though the OpenAPI contract declares both required. Now stashes merge metadata on the operation's ProviderResult map at commit time and reconstructs the complete response on a dedup hit. - commitMergeResultLocked renamed to commitMergeResult -- it acquires the store's write lock itself, inverting the *Locked naming convention every other helper in this file follows (which require the caller to already hold the lock); the old name could have led a future caller to deadlock on the non-reentrant RWMutex. - recvTypeString/funcKey: handle *ast.IndexListExpr (multi-parameter generic receivers, e.g. Foo[T, U]) the same way *ast.IndexExpr already was, and guard against a nil fn.Name the same way funcDisplayName already did. New/updated tests: TestMergeUnattachedCommentsSurviveASuccessfulMerge, a dedicated ring-buffer-eviction subtest in TestMergeFileRejectsUnverifiableBase, and a fix to TestMergeFileConcurrentCommitRetries's flakiness risk (reduced churn count below maxMergeCommitAttempts, surfaced its previously-swallowed error). All existing tests re-verified passing, 3x with -race. Known, not fixed in this pass (documented as follow-ups, both flagged by review but lower severity and larger scope than the above): provenance is only recorded for the WriteFile/BulkWrite/fork-commit paths (via recordWriteWithContentIdentityLocked), not for provider ingest or rename/move, which bypass it via direct appendWorkspaceEventLocked calls -- a merge citing a base from one of those paths safely falls back to merge_base_unavailable rather than misbehaving, it just can't succeed; and the provenance ledger is bounded per-path but not across the total number of distinct paths ever written, so a very large number of distinct files could grow it unboundedly over a long-running process's lifetime.
Merge is restricted to .go files server-side; explicitly default here rather than relying on an omitted contentType falling through to the server's generic text/plain fallback for an unrelated code path. Addresses CodeRabbit review feedback on PR #373.
Phase 2 (RebaseFork, PR #372) and Phase 3 (Store.MergeFile symbol-level merge, PR #373) both landed and merged since this doc was last updated. Renumbers the remaining gaps, updates the verdict/recommendation sections, and notes what's explicitly still out of scope (mount rollout, non-Go languages) per the user's Phase 3 scope decision.
…rough symbol merge (Phase 4) (#374) * feat(mountsync): mount rollout — route same-path .go conflicts through symbol merge Phase 4 of goal-B collaborative-engine work. Before today, a mounted workspace push that conflicted on a same path always fell straight to materializeConflict: park the loser's bytes in .relay/conflicts/*.local, pull the winner, overwrite local. Correct, but a whole-file discard even when two agents touched different functions in the same Go file — exactly the case Phase 3's Store.MergeFile (PR #373) already knows how to reconcile. This wires mount rollout: on ErrConflict for a merge-eligible (.go) path, attempt a symbol-level merge before falling back to today's behavior unchanged. The blocking design gap: Store.MergeFile requires actual BaseContent bytes (hash-verified against the server's provenance ledger), not just a revision string — and mountsync tracked only revision/hash per file, never content as-of-a-revision. Added an opportunistic shadow cache (.relay/.mount-shadow/) that snapshots .go content at the one point in the sync loop where local content is proven to exactly match server state (preparePendingBulkWrite's "confirmed clean" branch). A cache miss is always a safe, expected outcome — attemptMountRolloutMerge falls through to materializeConflict exactly as before, never treating a missed opportunity as an error. - RemoteClient gains MergeFile; HTTPClient.MergeFile POSTs /fs/merge. - doJSON's blanket 409-to-ConflictError collapse is narrowed to only the revision_conflict shape, so merge_conflict/merge_base_unavailable survive as typed HTTPError with Code intact (existing callers only ever produced revision_conflict, so this is additive). - attemptMountRolloutMerge only reports success once the merged content is confirmed via readback and written locally; any failure after the server accepts the merge still falls back safely (the fallback path re-reads current server state on its own terms). - Mock RemoteClient implementations (bootstrap_test.go, syncer_test.go, mountfuse's fakeRemoteClient/layoutRemoteClient) default MergeFile to merge_ineligible so no pre-existing test starts auto-merging. Test coverage for the new mechanism (shadow cache, merge success/fallback paths, updated same-file-conflict assertions) follows in a subsequent commit. * test(mountsync): cover mount rollout — shadow cache, merge attempt, real conflict race Unit coverage for the new mechanism (shadow-cache round-trip/prune/miss, attemptMountRolloutMerge's success/merge_conflict/merge_base_unavailable/ readback-failure paths, no-network-on-cache-miss) plus two integration-level tests exercising the real syncer entrypoints (SyncOnce/HandleLocalChange) rather than calling the new methods directly. TestAssessSameFileNearSimultaneousWriteGoMergeAutoMerges is the Go-file sibling of the existing same-path race test: two syncers against a real httptest server + real Store.MergeFile, racing goroutines editing disjoint functions in the same file, asserting the server and both local mirrors converge to a single file with both edits and neither side has a .relay/conflicts/*.local artifact — the mount-rollout counterpart to the existing test's coverage of the classic same-path conflict-artifact path. Written by Codex (finn-mac-mini) against the mechanism from the prior commit; reviewed and independently re-run (build/vet/full suite, and the new tests specifically at -race -count=3) before merging into this branch. * fix(mountsync): populate mount-rollout shadow cache from the default poll-mode scan Found live, driving relayfile-mount through a real bootstrap + reconcile cycle across two physical machines (not just unit tests) — the shadow cache never populated for a file that came in clean and was never edited again, because pushLocal's own confirmed-clean short-circuit (scanLocalFiles's hash==tracked.Hash && !Dirty check) returns before ever calling preparePendingBulkWrite, where the cache-population hook lived. That short-circuit is the only place the DEFAULT, recommended poll-mode scan proves a .go file's on-disk content exactly matches its tracked revision — the watcher-driven path (HandleLocalChange) reaches preparePendingBulkWrite's clean branch directly and was unaffected, which is exactly why the existing unit tests (built against HandleLocalChange) didn't catch this: they never exercised the pushLocal/scanLocalFiles scan path this bug lived in. Net effect before this fix: mount rollout was silently inert under the default poll mode, and only ever worked if the watcher path happened to fire a redundant clean-file event — never in practice. Also fixes TestMountRolloutConflictPathWithoutShadowFallsBack, whose "fresh tracked file has no shadow entry after one SyncOnce" assumption is no longer true now that bootstrap-then-immediately-clean populates the cache within the same cycle (a real, intended improvement) — the test now explicitly evicts the shadow directory to still exercise the cold-cache fallback path it's meant to cover. * fix(mountsync): shadow cache was caching empty content from pushLocal's poll path Second bug found continuing the live cross-machine proof (first was the previous commit — the shadow cache not populating at all under poll mode; this is worse — it populates, with the wrong content). pushLocal's confirmed-clean short-circuit (the site the prior commit wired up) runs on the snapshot scanLocalFiles produced via readLocalSnapshot(path, false) — a deliberate cheap hash-only read that never populates RawContent. Every other caller of recordShadowContent passes a full-content snapshot; this was the one exception, and it was silently caching an empty byte slice as the merge base for every .go file observed clean through periodic polling. An empty cached "base" still round-trips through readShadowContent as a cache hit — recordShadowContent's revision-emptiness guard doesn't catch empty *content* — so attemptMountRolloutMerge would confidently send baseContent="" to the server on every real conflict. The server's provenance ledger verifies baseContent's hash before trusting it (this is the exact mechanism Store.MergeFile uses to reject a fabricated base), so every one of these merge attempts would fail as merge_base_unavailable and silently fall back to the ordinary conflict-artifact path. Net effect: mount rollout would never actually succeed under the default poll mode, even after the previous commit's fix — degraded safely (no corruption, same fallback as always), but the feature was still fully inert. Fix: refreshShadowContentFromDisk does one real, explicit content read (bounded to .go paths, and skipped once the exact revision is already cached, so steady-state cost is unchanged) instead of reusing the cheap scan's contentless snapshot. Added TestMountRolloutShadowCachePopulatesFromPollModeAlone as a regression test: it drives ONLY SyncOnce (no HandleLocalChange), which is what isolates this bug — every existing test happened to also call HandleLocalChange at some point, which always reads full content and silently overwrote pushLocal's empty write before any assertion observed it. This is exactly why the live cross-machine proof — not unit tests alone — is the standing bar for calling a phase done. * fix(mountsync): address PR #374 review — shadow write cost, field reset, merge idempotency - recordShadowContent now short-circuits with a stat/size check before the atomic write + glob + prune, since it's called every scan/push cycle for every clean .go file but the cached base for a given revision never changes once written. Fixes real per-cycle disk churn on a large Go tree. (CodeRabbit, Major/perf) - attemptMountRolloutMerge's success path now mutates the existing tracked entry instead of rebuilding it, preserving DeletePending/Denied/ WriteDenied/DeniedHash instead of silently resetting them just because this particular write went through the merge path instead of an ordinary push. (CodeRabbit, Minor) - HTTPClient.MergeFile now sends a contentIdentity derived from the merge's logical inputs (workspace/path/strategy/baseRevision/content hash, no timestamp), so doJSON's automatic retry on a lost response or 5xx/429 resends an identical key and the server's existing merge-operation dedup (findExistingMergeOpLocked, from Phase 3) returns the original result instead of committing the same merge twice. Without this, a retried merge could mint an extra revision, filesystem event, and provider writeback for content that already landed. (Codex, P1) Full suite green (go build/vet/test ./...), mount-rollout tests re-run at -race -count=3.
1 participant
Summary
Phase 3 of the goal-B fix series. Builds on #370/#371/#372 (all merged). This is the piece that directly answers the original ask: "if two agents are working on backend they should be able to see [each other's changes] in real time across sandboxes" — specifically, two agents editing different functions in the same file should not have to serialize on a whole-file conflict.
Design was reconciled from two independent proposals (mine and Codex's) before any code was written — see
docs/multi-agent-collaboration-assessment.md(Phase 3 section) for the reconciliation. Codex's version was adopted almost entirely: a client-supplied merge base verified against a server-side provenance ledger (not full content history), Go-only viago/parserfor v1 (no cross-language heuristic),init()never auto-merged, same-function-different-lines still conflicts (function is the collaboration unit, not line ranges).Explicitly out of scope for this PR (per an earlier decision): automatic
mountsyncrouting of eligible files through this endpoint ("mount rollout") and non-Go languages are deferred follow-ups. This PR makes the primitive callable; it doesn't wire it into the mount's default write path yet.What changed
Store layer (
internal/relayfile/merge.go,merge_go.go):Store.MergeFile: three-way merge (base/mine/theirs) scoped to top-level Go function declarations, matched by name (not byte position) across versions.maxMergeCommitAttemptsif the live revision moves mid-computation.TestMergeFileConflictLeavesLiveFileUnchangedreads the file back after a conflict and checks it's byte-identical to before).HTTP + SDK (built by Codex on top of the store layer):
POST /v1/workspaces/{workspaceId}/fs/merge?path=...with full error mapping (merge_conflict→ 409 with per-unit details,merge_base_unavailable→ 409,merge_ineligible→ 422, invalid input → 400).mergeFile()method,MergeConflictErrorclass.Multi-agent workflow
Same split as phases 1-2: I (Claude) implemented and tested the store layer directly (23 tests, including a
-raceconcurrency test and a dedicated "merged output must always parse as valid Go" safety test that caught a real bug during development — a missing separator when concatenating adjacent function texts). Codex (onsf-mac-mini, viacodex exec) built the HTTP/SDK layer on top from a detailed spec. Codex's sandbox again couldn't push (no git write access, DNS blocked) or run tests needing loopback binding or a full npm install — picked those up directly, reviewed every diff, ran full independent validation outside that sandbox.Test plan
Store.MergeFileintegration) — all passing, including 3x repeated-racerunsgo build ./...,go vet ./..., fullgo test ./...— all green, run directly onsf-mac-minioutside Codex's sandboxscripts/check-contract-surface.sh— passingnpm test(233/233 passing),npx tsc --noEmit— cleansf-mac-mini; from this laptop, editedCreateUserbased on a shared seed; fromfinn-mac-mini, independently editedDeleteUserin the same live file; merge call from the laptop auto-applied both changes with zero conflict. Then the inverse: both machines edited the same function (CreateUser) — correctly rejected with 409, exact per-unit base/mine/theirs text in the response, and the live file verified byte-identical tofinn-mac-mini's version (no partial merge, no corruption) — all over the real Tailscale network across three distinct physical machines.🤖 Generated with multi-agent assistance (Claude implementing the store layer + reviewing/verifying, Codex implementing the HTTP/SDK layer) as part of the goal-B initiative.