Workspace copy writes every file as a revision, so revision IDs disagree across workspaces #29

Open
opened 2026-10-03 05:04:30 +00:00 by thklein · 1 comment
Owner

Problem

Client.Create(ctx, provider, fromWorkspaces...) copies a workspace with cp (pkg/client/client.go ~line 324). cpFile writes each file with dest.WriteFile(ctx, entry, sourceFile, WriteOptions{}). CreateRevision is nil there, which means create a revision. After that, Create copies the source's revision store over the destination's with a second cp on RevisionClient().

The result depends on what the source had:

  • If the source file has revision info, the second cp overwrites the info the first one wrote. That works, but only by accident of ordering.
  • If the source file has no revision info (it was written with CreateRevision=false, as every knowledge connector does), the destination keeps the info that cpFile's write created. It ends up with revision state the source never had (CurrentID=0 plus a revision file), so the two workspaces disagree on the file's revision ID.

Why it matters

Consumers compare revision IDs across workspaces. Teamchat's "unmodified" check compares the copy's CurrentID with the source's to decide whether a file was changed after it was copied. With the mismatch above, an untouched copy can read as modified (or the reverse), which drives the wrong merge/overwrite decision.

It also writes extra revision objects, which is real storage on S3, for every file of every copied workspace.

Fix

  • In cpFile, write with WriteOptions{CreateRevision: &false}. The copy should reproduce the source, not create history.
  • When copying the revision store, skip revision files numbered above the source's CurrentID. Those are pending or abandoned revisions; see the PendingID marker from #28.
  • Add a test: copy a workspace holding one file with revision info and one without, then assert that StatFile(..., WithLatestRevisionID) matches source and destination for both.

Where it was found

Review of #27 / #28. Related to basepeak-ai/bpai#1220.

## Problem `Client.Create(ctx, provider, fromWorkspaces...)` copies a workspace with `cp` (`pkg/client/client.go` ~line 324). `cpFile` writes each file with `dest.WriteFile(ctx, entry, sourceFile, WriteOptions{})`. `CreateRevision` is nil there, which means **create a revision**. After that, `Create` copies the source's revision store over the destination's with a second `cp` on `RevisionClient()`. The result depends on what the source had: - If the source file has revision info, the second `cp` overwrites the info the first one wrote. That works, but only by accident of ordering. - If the source file has **no** revision info (it was written with `CreateRevision=false`, as every knowledge connector does), the destination keeps the info that `cpFile`'s write created. It ends up with revision state the source never had (`CurrentID=0` plus a revision file), so the two workspaces disagree on the file's revision ID. ## Why it matters Consumers compare revision IDs **across** workspaces. Teamchat's "unmodified" check compares the copy's `CurrentID` with the source's to decide whether a file was changed after it was copied. With the mismatch above, an untouched copy can read as modified (or the reverse), which drives the wrong merge/overwrite decision. It also writes extra revision objects, which is real storage on S3, for every file of every copied workspace. ## Fix - In `cpFile`, write with `WriteOptions{CreateRevision: &false}`. The copy should reproduce the source, not create history. - When copying the revision store, skip revision files numbered above the source's `CurrentID`. Those are pending or abandoned revisions; see the `PendingID` marker from #28. - Add a test: copy a workspace holding one file with revision info and one without, then assert that `StatFile(..., WithLatestRevisionID)` matches source and destination for both. ## Where it was found Review of #27 / #28. Related to basepeak-ai/bpai#1220.
Member

Confirmed in pkg/client/client.go: cpFile (line 348-356) calls dest.WriteFile with WriteOptions{} whose CreateRevision is nil, which beginWrite (revisionwrite.go:125) treats as 'create a revision' rather than CreateRevision=false; and Create (line 114-144) unconditionally runs cp(source.RevisionClient(), dest.RevisionClient()) after the file copy with no bound on which revision files are copied. This matches the issue exactly: files copied from a workspace that never used revisions pick up a spurious CurrentID=0 unless the source happened to have its own revision info to overwrite it, and the revision-store copy has no filter to skip revision files above the source's CurrentID (data for that already exists via revisionInfo.PendingID/CurrentID in revisions.go and promotePendingRevision in revisionwrite.go, so the fix can reuse that bookkeeping). The three fix steps given (set CreateRevision: &false in cpFile, bound the revision-store cp by CurrentID, add a StatFile-based test) are concrete and map directly onto existing structures (revisionInfo, WriteOptions, StatOptions.WithLatestRevisionID), so this is well-scoped and immediately actionable without further design decisions.

Confirmed in pkg/client/client.go: cpFile (line 348-356) calls dest.WriteFile with WriteOptions{} whose CreateRevision is nil, which beginWrite (revisionwrite.go:125) treats as 'create a revision' rather than CreateRevision=false; and Create (line 114-144) unconditionally runs cp(source.RevisionClient(), dest.RevisionClient()) after the file copy with no bound on which revision files are copied. This matches the issue exactly: files copied from a workspace that never used revisions pick up a spurious CurrentID=0 unless the source happened to have its own revision info to overwrite it, and the revision-store copy has no filter to skip revision files above the source's CurrentID (data for that already exists via revisionInfo.PendingID/CurrentID in revisions.go and promotePendingRevision in revisionwrite.go, so the fix can reuse that bookkeeping). The three fix steps given (set CreateRevision: &false in cpFile, bound the revision-store cp by CurrentID, add a StatFile-based test) are concrete and map directly onto existing structures (revisionInfo, WriteOptions, StatOptions.WithLatestRevisionID), so this is well-scoped and immediately actionable without further design decisions.
Sign in to join this conversation.
No description provided.