fix(review): #1258 review findings, round 3

lib/Controller/ShareSaveController.php

@@ -163,7 +163,7 @@ private function forbiddenBody(ForbiddenException $refusal): array {
 
 	/**
 	 * The body of a `stale` 409: the caller's row as it is now and, on the
-	 * copy-editor path, the source's version.
+	 * copy-editor path, the source version the conflict stands on.
 	 *
 	 * @param StaleWriteException $refusal The refusal
 	 *

lib/Exception/StaleWriteException.php

@@ -36,7 +36,7 @@ class StaleWriteException extends RuntimeException {
 	 * Constructor.
 	 *
 	 * @param Secret $current The secret as it is now
-	 * @param DateTime|null $sourceUpdatedAt The source's version, when the secret is a copy saved through a share-save
+	 * @param DateTime|null $sourceUpdatedAt The source version the conflict stands on, when the secret is a copy saved through a share-save
 	 *
 	 * @return void
 	 */
@@ -54,8 +54,9 @@ public function getCurrent(): Secret {
 	}//end getCurrent()
 
 	/**
-	 * The source's version a copy-editor's share-save found, so the client can
-	 * keep it with the conflict; null on every other write.
+	 * The source version a copy-editor's conflict stands on: the source's current
+	 * version when it equals the copy's, or else the edit's own source base, so the
+	 * client can keep it with the conflict; null on every other write.
 	 *
 	 * @return DateTime|null
 	 *

lib/Service/ShareSaveService.php

@@ -244,7 +244,13 @@ private function open(string $id, ShareSaveRequest $request, string $userId, arr
 			request: $request,
 			step: function () use ($path, $request): void {
 				if ($this->movedOn(row: $path->row, base: $request->baseUpdatedAt) === true) {
-					throw $this->staleRow(path: $path, current: $path->row, sourceVersion: $path->source->getUpdatedAt());
+					// Name the source only as this copy was written with it: a source that moved alone was not in the conflict.
+					$sourceVersion = $request->sourceBaseUpdatedAt;
+					if ($this->movedOn(row: $path->source, base: $path->row->getUpdatedAt()) === false) {
+						$sourceVersion = $path->source->getUpdatedAt();
+					}
+
+					throw $this->staleRow(path: $path, current: $path->row, sourceVersion: $sourceVersion);
 				}
 
 				if ($path->isCopyEditor() === true
@@ -399,12 +405,13 @@ private function lockVersions(ShareSavePath $path, ShareSaveRequest $request, Da
 
 	/**
 	 * The `stale` refusal of the caller's row. On the copy-editor path it
-	 * names the source's version too, so the client can keep it with the
-	 * conflict without reading it again.
+	 * names a source version too, so the client can keep it with the
+	 * conflict without reading it again: the source's current version only
+	 * when it equals the copy's version, otherwise the edit's source base.
 	 *
 	 * @param ShareSavePath $path The path
 	 * @param Secret $current The caller's row as it is now
-	 * @param DateTime|null $sourceVersion The source's version as this save found it
+	 * @param DateTime|null $sourceVersion The source version the conflict stands on
 	 *
 	 * @return StaleWriteException
 	 *

openspec/changes/sharing-atomic-share-save/specs/offline-edit-queue/spec.md

@@ -94,6 +94,7 @@ A queued update that changes a value, login or additional fields MUST be replaye
 - **GIVEN** a queued edit of a copy whose replay was refused with 409 `stale`, because the owner saved the secret meanwhile
 - **WHEN** the user keeps their change, before or after a reload
 - **THEN** the replay MUST send the copy version and the source version that 409 named, and save when neither has moved again
+- **AND** when the copy was written after its source, such as by a plain update of the copy, the 409 names the source version the edit was made from, so keeping the change MAY first be refused with 409 `stale_source` and cost the user one more choice before it saves
 
 #### Scenario: A pending sync from before the change is finished
 

openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md

@@ -22,7 +22,7 @@ When the owner, or a recipient allowed to write the source, updates the value, l
 ## ADDED Requirements
 
 ### Requirement: A shared secret and every copy save in one transaction
-`PUT /api/v1/secrets/{id}/share-save` MUST take the caller's sparse patch with a required `baseUpdatedAt`, the ciphertext for every local copy (`copies`), the whole value for every active partner share (`federated`), and the copies and partner shares left out on purpose (`skipped`, `suspend`). `{id}` MUST be the caller's own row: the source on the owner path, or the caller's copy on the copy-editor path, where `sourceBaseUpdatedAt` is also required. The server MUST write the caller's row, every copy and every partner share in one database transaction, or write nothing. It MUST answer 409 `stale` with the current row as `current` when `{id}` changed since `baseUpdatedAt`, and 409 `stale_source` when the source changed on the copy-editor path. On the copy-editor path both answers MUST carry the source's version as `sourceUpdatedAt`. A refusal of the caller's own row, such as a vault policy, MUST roll back the copies too. A save on the copy-editor path MUST also be recorded in the audit trail on the source, with the caller as actor and the user ids of the holders whose copies were skipped (the owner's when the source itself was skipped).
+`PUT /api/v1/secrets/{id}/share-save` MUST take the caller's sparse patch with a required `baseUpdatedAt`, the ciphertext for every local copy (`copies`), the whole value for every active partner share (`federated`), and the copies and partner shares left out on purpose (`skipped`, `suspend`). `{id}` MUST be the caller's own row: the source on the owner path, or the caller's copy on the copy-editor path, where `sourceBaseUpdatedAt` is also required. The server MUST write the caller's row, every copy and every partner share in one database transaction, or write nothing. It MUST answer 409 `stale` with the current row as `current` when `{id}` changed since `baseUpdatedAt`, and 409 `stale_source` when the source changed on the copy-editor path. On the copy-editor path both answers MUST carry a source version as `sourceUpdatedAt`: `stale_source` the source's current version; `stale` the source's current version only when the copy was written with it (the source's `updatedAt` equals the copy's), and otherwise the edit's own `sourceBaseUpdatedAt`, so a `stale` answer never names a source change the conflict does not show. A refusal of the caller's own row, such as a vault policy, MUST roll back the copies too. A save on the copy-editor path MUST also be recorded in the audit trail on the source, with the caller as actor and the user ids of the holders whose copies were skipped (the owner's when the source itself was skipped).
 
 #### Scenario: A copy editor's save is audited on the source
 - GIVEN a secret owned by Alice and shared with Bob, who holds a write grade, and with Carol, who has no vault key
@@ -39,6 +39,11 @@ When the owner, or a recipient allowed to write the source, updates the value, l
 - WHEN this editor sends a share-save with the older `baseUpdatedAt`
 - THEN the server MUST answer 409 `stale` with the current row and change nothing
 
+#### Scenario: A stale copy does not name a source that moved alone
+- GIVEN a copy editor's edit of a copy, and a copy and a source that both moved since, the source later than the copy
+- WHEN the copy editor sends a share-save with the older `baseUpdatedAt` and `sourceBaseUpdatedAt`
+- THEN the server MUST answer 409 `stale` with the copy as `current` and the edit's own `sourceBaseUpdatedAt` as `sourceUpdatedAt`, and change nothing
+
 #### Scenario: A copy holder without a write grade is refused
 - GIVEN a recipient whose copy is shared with a read grade
 - WHEN that recipient sends a share-save for their copy

tests/Unit/Service/ShareSaveServiceTest.php

@@ -441,9 +441,12 @@ public function testARowThatMovesOnBeforeTheLockRollsBack(): void {
 	}
 
 	/**
+	 * Only the copy moved; the source is still at the edit's base, which the
+	 * refusal names.
+	 *
 	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-shared-secret-and-every-copy-save-in-one-transaction
 	 */
-	public function testAStaleCopyNamesTheSourceVersionItFound(): void {
+	public function testAStaleCopyWhoseSourceStayedNamesTheSourceBase(): void {
 		$this->bobWritesOnAGrade();
 		$this->rows['c-bob']->setUpdatedAt(new DateTime('2026-10-01T10:00:07+00:00'));
 
@@ -455,6 +458,43 @@ public function testAStaleCopyNamesTheSourceVersionItFound(): void {
 		$this->assertSame([], $this->log);
 	}
 
+	/**
+	 * The source moved alone past the edit's base, so the conflict never
+	 * showed that change: the refusal names the edit's own source base.
+	 *
+	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-shared-secret-and-every-copy-save-in-one-transaction
+	 */
+	public function testAStaleCopyDoesNotNameASourceVersionThatMovedAlone(): void {
+		$this->bobWritesOnAGrade();
+		$this->rows['c-bob']->setUpdatedAt(new DateTime('2026-10-01T10:00:07+00:00'));
+		$this->rows['src']->setUpdatedAt(new DateTime('2026-10-01T10:00:09+00:00'));
+
+		$refusal = $this->refusal(fn () => $this->service()->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob'));
+
+		$this->assertInstanceOf(StaleWriteException::class, $refusal);
+		$this->assertSame('c-bob', $refusal->getCurrent()->getId());
+		$this->assertSame(self::BASE, $refusal->getSourceUpdatedAt()?->format(DATE_ATOM));
+		$this->assertSame([], $this->log);
+	}
+
+	/**
+	 * A save stamps the source and the copy with one version, so the copy
+	 * shown in the conflict carries the source's current version.
+	 *
+	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-shared-secret-and-every-copy-save-in-one-transaction
+	 */
+	public function testAStaleCopyNamesTheSourceVersionItWasWrittenWith(): void {
+		$this->bobWritesOnAGrade();
+		$this->rows['c-bob']->setUpdatedAt(new DateTime('2026-10-01T10:00:07+00:00'));
+		$this->rows['src']->setUpdatedAt(new DateTime('2026-10-01T10:00:07+00:00'));
+
+		$refusal = $this->refusal(fn () => $this->service()->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob'));
+
+		$this->assertInstanceOf(StaleWriteException::class, $refusal);
+		$this->assertSame('2026-10-01T10:00:07+00:00', $refusal->getSourceUpdatedAt()?->format(DATE_ATOM));
+		$this->assertSame([], $this->log);
+	}
+
 	/**
 	 * The source was locked and moved before the copy refused, but that move
 	 * rolls back, so the version named is the one the source keeps.