Repository navigation
fix: a move preserves authorship instead of re-stamping it (#3263) - #3273
Conversation
Move is copy-to-target + delete-the-source, and the copy leg re-created every node as if it had just been written by whoever ran the move. Two mechanisms: 1. RetargetNode stamped LastModified = UtcNow unconditionally. A relocation writes no content, so this was the wrong field to touch. Reproduces on any backend. 2. The copy leg read the nodes it re-created from the QUERY, and a query result is a projection. PostgreSqlSqlGenerator's SELECT list carries no created_by, created_date or last_modified_by — they are real columns the point read and the INSERT both use, but no query projects them. So on the production backend every node arrived with the stamps blank and the create path filled them with "now, by the caller". Copy the projection, delete the source, and the authorship of the whole subtree is gone, with no version history behind it. Re-creating a node is a lifecycle operation, so it now reads the node from storage — the same rule the move's delete leg already follows for its path enumeration (#839). The query still decides WHICH nodes (it is the read RLS filters); storage decides WHAT they are. Move preserves all four stamps; copy deliberately does not — a copy is a new node and is stamped for the copier, because AccessContextScope impersonates MeshNode.CreatedBy and an inherited creator would run owner-scoped work as someone who never touched the node. CopyNodeRequest.PreserveAuthorship carries the distinction; no public member is removed or re-signed. Found and filed separately: #3272 — a move DELETES every satellite under the moved node and reports success (same handler, same family). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW
There was a problem hiding this comment.
🟡 Changes recommended
There are security-critical identity-scope issues in the copy handler (Observable.Using + SwitchAccessContext) and PreserveAuthorship semantics that should be enforced server-side to prevent misuse.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a data-integrity defect where MoveNodeRequest (implemented as copy+delete) was rewriting node authorship/audit stamps across the moved subtree. It introduces an explicit “preserve authorship” behavior for the move’s copy leg, adds regression tests, and documents the CQRS/projection pitfall that caused the production backend divergence.
Changes:
- Ensure move’s copy leg preserves
CreatedDate/CreatedBy/LastModified/LastModifiedByby reading authoritative nodes from storage and by not re-stampingLastModifiedduring retarget. - Add
CopyNodeRequest.PreserveAuthorship(defaultfalse) and set it totrueonly for the move orchestration path. - Add a monolith-hosted regression test plus a What’s New entry and architecture documentation update.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs | Adds regression coverage for move preserving stamps and copy stamping the copier. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Changes move/copy handlers to preserve stamps for move via storage reads and retargeting changes. |
| src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs | Adds the PreserveAuthorship flag to CopyNodeRequest. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-04-moving-something-keeps-its-author.md | User-facing release note for the move-stamp fix. |
| src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md | Documents “query results are projections” and the move/copy stamp rules. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ? meshService.CreateNode(node) | ||
| : Observable.Using( | ||
| () => accessService.SwitchAccessContext(callerAccessContext), | ||
| _ => meshService.CreateNode(node)); |
There was a problem hiding this comment.
This one is on pre-existing code that this diff does not touch: CreateUnderCaller's Observable.Using(() => accessService.SwitchAccessContext(...), ...) was already there, and it is the site already recorded in test/ImpersonationScopeSites.allow (src/MeshWeaver.Mesh.Contract/MeshExtensions.cs 1) — so the ratchet knows about it and the guard suite is green on it (250/250 locally).
Changing that impersonation seam to ImpersonationScopeExtensions.RunAs(...) is a real change to how identity is carried across the copy's scheduler hop, with its own blast radius, and it is not what this PR is about — a data-loss fix should not also quietly re-plumb the identity latch. It belongs in its own change against the allow-file entry, where it can be measured on its own.
Pull request was converted to draft
…reserving Two findings from the review of the first commit, both correct. 1. PreserveAuthorship was honoured unconditionally. CopyNodeRequest is a wire message, so the flag is caller-settable — and a node whose CreatedBy names somebody else is not inert: AccessContextScope.FromNode impersonates exactly that identity, so any caller who can read a node could mint one, in a place they control, that owner-scoped work then runs AS its author. Measured with the gate removed: an Editor (Read|Create|Update, no Delete) successfully copied an admin's node with its authorship intact. RLS does not close this — reading the source is precisely what an Editor may do. The flag now requires Delete on the source's namespace: what MoveNodePermissionAttribute already requires of a mover, so it confers nothing the platform did not already grant that caller. An UNDETERMINED fold refuses too, reported as unavailability rather than a denial (#974/#2742). 2. Authoritative() fell back to the query projection when the point read came back empty — which, for a preserving copy, is the exact projection that destroyed the authorship, so the fix would have quietly reopened the bug on the one store that cannot answer. It now refuses; since the delete leg only runs after the copy succeeds, a refused read leaves the source and its stamps where they are. A plain copy keeps the fallback: its stamps are cleared anyway, so nothing is lost. PreserveAuthorshipIsGatedTest pins both halves — the Editor is refused with Unauthorized, and the same Editor's plain copy still succeeds and is stamped for them. Without the second the first would be satisfied by a subject who could not copy at all. The third review comment (Observable.Using + SwitchAccessContext) is on pre-existing code this diff does not touch — the allow-listed site already recorded in test/ImpersonationScopeSites.allow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW
Attribution of the shard-4 red on 59c7cee (run 33843827448)One failing test: It is not reachable from this diff. The change touches Measured rather than assumed:
So it is neither always-red on main nor reproducible here — a race whose odds moved, not a One concrete observation for whoever owns the test (added today in 32714b2, #3110). Its own xUnit does not order What I did instead of re-running: merged |
Both PRs rewrite HandleCopyNodeRequest / HandleMoveNodeRequest. They are orthogonal defects in one operation, so the reconciliation keeps BOTH and lets them compose rather than picking a side: - #3263 (main) decides WHAT a re-created node is: every node is read from IStorageAdapter via Authoritative(...) rather than the query projection, and RetargetNode's preserveAuthorship keeps the four stamps for a move and clears them for a copy. PreserveAuthorship stays permission-gated on Delete over the source namespace — untouched, including the UNDETERMINED-is-a-refusal arm. - #3272 (here) decides WHICH nodes there are: storage names the satellite CONTAINERS, the RLS-filtered query says what may be carried out of them, and RequireComplete refuses before creating anything when the copy cannot cover what storage holds. The satellites the sweep finds now go through Authoritative(...) too — they arrived through the same query surface, so a preserving copy must not re-create them from a projection either. Also folded: one "[CopyNode] start" LogDebug naming both flags, and CqrsAndContentAccess.md's forward reference to #3272 as still-open now points at Doc/Architecture/MovingNodes. Verified after the merge: MoveCarriesSatellitesTest + MoveNodePreservesAuthorshipTest 4/4, MeshWeaver.Hosting.Test 337/337, MeshWeaver.Documentation.Test 250/250 (the edited doc confirmed embedded in the built assembly), Mesh.Contract and both test projects -c Release -warnaserror with 0 Error(s). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #3263.
A move relocates a node. It re-stamped it instead —
createdDate,createdBy,lastModifiedandlastModifiedByall rewritten to "now, by whoever ran the move", on the node named in the requestand on every descendant under it. With no version history behind it (#3264) the originals are gone.
Measured on memex.systemorph.com 2026-09-03: one subtree move rewrote ~80 nodes, a commercial
proposal among them.
The defect — two mechanisms, one root cause
MoveisCopy(source → target)+ delete-the-source, and the copy leg re-creates every nodethrough
CreateNodeRequest. Two things then decide the stamps, and both got it wrong:1.
RetargetNodestampedLastModified = DateTimeOffset.UtcNowunconditionally.src/MeshWeaver.Mesh.Contract/MeshExtensions.cs. A relocation writes no content, so this was simplythe wrong field to touch. This is the half that reproduces on any backend.
2. The copy leg read the nodes it re-created from the QUERY, and a query result is a projection.
The subtree read is
meshService.Query<MeshNode>("path:{source} scope:subtree"), and what a providerreturns for that is not the stored node.
PostgreSqlSqlGenerator.GenerateSelectQuery(MeshWeaver.Plugins)projects
and nothing else.
created_by,created_dateandlast_modified_byare real columns — the storageadapter reads them on a point read (
AuthorCols) and the INSERT writes them — but no queryprojects them. So on the production backend every node the copy handler saw arrived with
CreatedBy = nullandCreatedDate = default, and the create path filled the blanks the way itfills any blank:
CreatedDate == default ? now : …,CreatedBy← the caller. Copy the projection,delete the source, and the authorship of the whole subtree is gone.
That is why the fix is not "write the old values back afterwards": the values were never read. A
lifecycle operation that re-creates a node now reads it from storage — exactly as the move's
delete leg already enumerates its paths from storage rather than the catalog (#839, whose comment
sits four lines above the call that did the opposite). The split is:
storage instead would let a caller copy nodes they cannot read into a location they control;
IStorageAdapter.Read(path)per selected path.The decision you asked for, stated explicitly
Move preserves all four stamps, including
lastModified/lastModifiedBy. The issue's reasoningholds and the code did not contradict it:
MeshNodedocumentscreatedDateas "set once at creationtime; never updated thereafter" and
createdByas "never changed", andlastModified/lastModifiedByanswer who last wrote this content — a relocation writes none. A move worth recording belongs in the
activity log or its own history entry, not on top of who wrote the thing.
Copy is deliberately the opposite: a copy is a NEW node and is stamped for the copier. This is a
real decision, not a leftover — and it is why the flag exists rather than an unconditional
"preserve":
AccessContextScopederives the identity it impersonates fromMeshNode.CreatedBy. A copy thatinherited the original author would run owner-scoped work as someone who never touched it.
store's query happened to project: preserved on the in-memory adapter, freshly stamped on Postgres.
Now both stamp the copier, deterministically.
pathandnamespacechange, and nothing else does.namespacekeeps its partition prefix — theretarget derives it from the new path and never strips a segment.
Public surface
One addition:
CopyNodeRequest.PreserveAuthorship(init-only, defaultsfalse). Nothing publicis removed or re-signed —
RetargetNode,HandleCopyNodeRequestandHandleMoveNodeRequestare allprivate, and
IMeshService.CopyNodeis untouched on purpose (changing its signature would have beena public-member removal for a flag only one caller needs). The move handler therefore posts the copy
request itself, through the same issuing-hub/target/AccessContext seam
MeshService.CopyNodeuses,so the per-node creates still authorise as the mover.
Test evidence
test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs, realMonolithMeshTestBase, nomocks, no sleeps, reads through the base's authoritative owner-hub
ReadNode(not a query row — thewhole point is which stamps the node CARRIES).
Move_KeepsTheStamps_OnTheNodeAndOnItsDescendants— authorauthor-acreatesproposal/andproposal/Pricing,mover-bmoves the subtree, both nodes are read back at their new paths.Without the src change (measured on the unmodified handler):
With it:
Passed! - Failed: 0, Passed: 2.Copy_StampsTheCopier_BecauseACopyIsANewNodeis the control arm and also fails onmain(a copythere inherits
author-aon the in-memory adapter), so "both operations preserve" cannot bemistaken for a simplification.
🚨 What the tests do NOT prove, said plainly. Mechanism 2 — the projection — is not falsifiable
by a core test: the in-memory query provider returns the stored node verbatim, so
createdBy,createdDateandlastModifiedByalready survived a move there. OnlylastModifiedfails in-memory,and that is the failure quoted above. The evidence for the other half is the production provider's
SELECT list quoted at the top, which lives in MeshWeaver.Plugins. Restoring the field there would fix
that backend; reading from storage fixes every backend and is what a re-create should have been doing
regardless, so it is done here rather than deferred across repos.
Found on the way, filed not folded in
#3272 — a move DELETES every satellite under the moved node (
_Comment,_Thread,_Access, …)and reports success. Same handler, same family: the copy leg's content query excludes satellite paths
by design while the delete leg's
ListDescendantPathsenumerates every table of the partition.Measured on the core in-memory mesh; not fixed here because the enumeration is what has to change
and the query is the read RLS filters, which needs its own design rather than a rushed one inside this
fix.
Systemorph/MeshWeaver.Plugins#1310 — the PG query SELECT omits the three authorship columns.
The backend half quoted at the top, filed as its own contract gap: it is not move-specific, so any
consumer reading authorship off a query-served node gets a silent null on Postgres and the right
answer in-memory. This PR makes the move immune to it; the gap itself is fixed there.
Also recorded in
Doc/Architecture/CqrsAndContentAccess— "A query result is a PROJECTION — neverre-create a node from one", with the operation/stamp table and both findings.
Verification
dotnet build -c Release -warnaserror, one project per invocation:MeshWeaver.Mesh.Contract,MeshWeaver.Documentation,MeshWeaver.Hosting,MeshWeaver.Graph,MeshWeaver.Mesh.Operations,MeshWeaver.Hosting.Test,MeshWeaver.Graph.Test— all0 Warning(s) 0 Error(s).MoveNodePreservesAuthorshipTest(2/2),CodeNodeSegmentNameValidator*(15/15 — it pins that themove's copy leg runs the Create validators and leaves the source intact when they refuse),
DocumentationLinkIntegrityTest+WhatsNewEntryIntegrityTest.🤖 Generated with Claude Code
https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW
Review round (59c7cee)
Two findings from the automatic review, both correct and both fixed.
PreserveAuthorshipwas honoured unconditionally.CopyNodeRequestis a wire message, so theflag is caller-settable — and I measured that this was not theoretical: with the gate removed, an
Editor (
Read | Create | Update | …, noDelete) successfully copied an admin's node with itsauthorship intact (
refusal: success=True reason= error=). Row-level security does not close it,because reading the source is exactly what an Editor may do. The handler now requires
Permission.Deleteon the source's namespace — the same checkMoveNodePermissionAttributemakesof a mover — so the flag confers nothing the platform did not already grant that caller; an
UNDETERMINED fold refuses too, reported as unavailability rather than as a denial (#974/#2742).
PreserveAuthorshipIsGatedTestpins both halves: the Editor is refused withUnauthorized, and thesame Editor's plain copy still succeeds and is stamped for them. Without that second fact the
first is satisfied by a subject who could not copy at all, and the gate would be indistinguishable
from RLS refusing the whole operation. The suite builds its mesh from
ConfigureMeshBase— thedefault one grants Public → Admin, under which every identity holds Delete and no refusal is
observable.
Authoritative()'s fallback could reopen the bug behind the fix. A preserving copy now REFUSESwhen the point read comes back empty, instead of falling back to the projection that destroyed the
authorship in the first place. Because the delete leg only runs after the copy succeeds, a refused
read leaves the source and its stamps where they are — the failure mode is "the move did not
happen", not "the move happened and lost the author". A plain copy keeps the fallback: its stamps
are cleared anyway, so nothing that matters is lost.
The third comment (
Observable.Using+SwitchAccessContext) is on pre-existing code this diff doesnot touch — the site already recorded in
test/ImpersonationScopeSites.allow. Re-plumbing theidentity latch is a real change with its own blast radius and does not belong inside a data-loss fix.