From ebdf375898a66d7d846ce3056666733375e64ae8 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 15 Oct 2026 12:53:11 +0200 Subject: [PATCH 1/5] =?UTF-8?q?fix(review):=20#1258=20N1=20=E2=80=94=20a?= =?UTF-8?q?=20stale=20copy's=20409=20names=20a=20source=20version=20the=20?= =?UTF-8?q?conflict=20never=20showed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On the copy-editor path, open()'s `stale` refusal named the source's current version even when the source had moved alone past the edit's sourceBaseUpdatedAt. The client keeps that as conflictSourceUpdatedAt, so "keep mine" passed both locks and overwrote a source change the user never saw. The refusal now names the source's current version only when the copy was written with it (the source has not moved past the copy's updatedAt), and otherwise the edit's own sourceBaseUpdatedAt, so the replay gets `stale_source` and the user chooses again. Thread: https://github.com/ConductionNL/keepiq/pull/1258#discussion_r4281694386 Class sweep: no other place names a source version that moved alone. lockVersions() already names sourceBase; `stale_source` names the source move that is its own conflict; `incomplete` and the write context do not feed a conflict replay. Co-Authored-By: Claude Opus 5.5 --- lib/Service/ShareSaveService.php | 8 ++++- tests/Unit/Service/ShareSaveServiceTest.php | 37 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/lib/Service/ShareSaveService.php b/lib/Service/ShareSaveService.php index d4f417173..78b219d1b 100644 --- a/lib/Service/ShareSaveService.php +++ b/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 diff --git a/tests/Unit/Service/ShareSaveServiceTest.php b/tests/Unit/Service/ShareSaveServiceTest.php index 060c693a2..14b42ba6b 100644 --- a/tests/Unit/Service/ShareSaveServiceTest.php +++ b/tests/Unit/Service/ShareSaveServiceTest.php @@ -455,6 +455,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. From 794089b6fff53b8b426c933d76f6a5672fd7f38f Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 15 Oct 2026 13:33:33 +0200 Subject: [PATCH 2/5] =?UTF-8?q?docs(review):=20#1258=20N1=20=E2=80=94=20pr?= =?UTF-8?q?e-push:=20say=20which=20source=20version=20a=20stale=20copy's?= =?UTF-8?q?=20409=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user-sharing spec promised both copy-editor 409s carry "the source's version". A `stale` answer now names the source's current version only when the copy was written with it, and otherwise the edit's own sourceBaseUpdatedAt. The Requirement says so, a new Scenario covers the source that moved alone, and the docblocks of staleRow(), StaleWriteException and ShareSaveController::staleBody that repeated the old promise now match. Thread: https://github.com/ConductionNL/keepiq/pull/1258#discussion_r4281694386 Co-Authored-By: Claude Opus 5.5 --- lib/Controller/ShareSaveController.php | 2 +- lib/Exception/StaleWriteException.php | 7 ++++--- lib/Service/ShareSaveService.php | 7 ++++--- .../sharing-atomic-share-save/specs/user-sharing/spec.md | 7 ++++++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/Controller/ShareSaveController.php b/lib/Controller/ShareSaveController.php index f89e9f853..b76820aa7 100644 --- a/lib/Controller/ShareSaveController.php +++ b/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 * diff --git a/lib/Exception/StaleWriteException.php b/lib/Exception/StaleWriteException.php index 8893a909d..e15405eb2 100644 --- a/lib/Exception/StaleWriteException.php +++ b/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 as the + * copy was written with it, 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 * diff --git a/lib/Service/ShareSaveService.php b/lib/Service/ShareSaveService.php index 78b219d1b..9e9697224 100644 --- a/lib/Service/ShareSaveService.php +++ b/lib/Service/ShareSaveService.php @@ -405,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 the copy was written with it, 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 * diff --git a/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md b/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md index aed273e29..f3f4561fc 100644 --- a/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md +++ b/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 has not moved past the copy's `updatedAt`), 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 From 862ae320adc64608553c2e85fc21357c37750c3a Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 15 Oct 2026 13:33:42 +0200 Subject: [PATCH 3/5] =?UTF-8?q?docs(review):=20#1258=20N1=20=E2=80=94=20pr?= =?UTF-8?q?e-push:=20a=20copy=20written=20after=20its=20source=20can=20cos?= =?UTF-8?q?t=20one=20more=20choice?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Keeping mine after the copy moved" scenario promised the replay saves when neither version moved again. When a copy-only write moved the copy past the source, the stale 409 now names the edit's source base, so "keep mine" can first meet a `stale_source` refusal. That is safe; the scenario now states the extra choice. Thread: https://github.com/ConductionNL/keepiq/pull/1258#discussion_r4281694386 Co-Authored-By: Claude Opus 5.5 --- .../sharing-atomic-share-save/specs/offline-edit-queue/spec.md | 1 + 1 file changed, 1 insertion(+) diff --git a/openspec/changes/sharing-atomic-share-save/specs/offline-edit-queue/spec.md b/openspec/changes/sharing-atomic-share-save/specs/offline-edit-queue/spec.md index 8859ff726..45e9f94ed 100644 --- a/openspec/changes/sharing-atomic-share-save/specs/offline-edit-queue/spec.md +++ b/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 From 2d2e8964e1c54604bc393df677024fbc5aa6e86a Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 15 Oct 2026 13:42:59 +0200 Subject: [PATCH 4/5] =?UTF-8?q?test(review):=20#1258=20N1=20=E2=80=94=20pr?= =?UTF-8?q?e-push:=20name=20the=20stale-copy=20test=20for=20the=20case=20i?= =?UTF-8?q?t=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testAStaleCopyNamesTheSourceVersionItFound now runs through the source-base branch: only the copy moved and the source stayed at the edit's base. It is kept, apart from the source-moved-alone case, under the name testAStaleCopyWhoseSourceStayedNamesTheSourceBase. Thread: https://github.com/ConductionNL/keepiq/pull/1258#discussion_r4281694386 Co-Authored-By: Claude Opus 5.5 --- tests/Unit/Service/ShareSaveServiceTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Unit/Service/ShareSaveServiceTest.php b/tests/Unit/Service/ShareSaveServiceTest.php index 14b42ba6b..fc7475d5b 100644 --- a/tests/Unit/Service/ShareSaveServiceTest.php +++ b/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')); From 78fad4a2c7b509aa09f84a82c2f609a9ac37184e Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 15 Oct 2026 13:54:44 +0200 Subject: [PATCH 5/5] =?UTF-8?q?docs(review):=20#1258=20N1=20=E2=80=94=20pr?= =?UTF-8?q?e-push:=20the=20409=20names=20the=20current=20source=20only=20w?= =?UTF-8?q?hen=20its=20version=20equals=20the=20copy's?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit movedOn() compares versions for equality, so a stale copy's 409 names the source's current version only when the source's updatedAt equals the copy's; a copy written after its source gets the edit's base. The user-sharing Requirement said "has not moved past"; it and the docblocks of staleRow() and StaleWriteException::getSourceUpdatedAt() now say the versions are equal. Thread: https://github.com/ConductionNL/keepiq/pull/1258#discussion_r4281694386 Co-Authored-By: Claude Opus 5.5 --- lib/Exception/StaleWriteException.php | 4 ++-- lib/Service/ShareSaveService.php | 2 +- .../sharing-atomic-share-save/specs/user-sharing/spec.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Exception/StaleWriteException.php b/lib/Exception/StaleWriteException.php index e15405eb2..3e04e7b65 100644 --- a/lib/Exception/StaleWriteException.php +++ b/lib/Exception/StaleWriteException.php @@ -54,8 +54,8 @@ public function getCurrent(): Secret { }//end getCurrent() /** - * The source version a copy-editor's conflict stands on: the source as the - * copy was written with it, or else the edit's own source base, so the + * 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 diff --git a/lib/Service/ShareSaveService.php b/lib/Service/ShareSaveService.php index 9e9697224..be8176c9d 100644 --- a/lib/Service/ShareSaveService.php +++ b/lib/Service/ShareSaveService.php @@ -407,7 +407,7 @@ private function lockVersions(ShareSavePath $path, ShareSaveRequest $request, Da * The `stale` refusal of the caller's row. On the copy-editor path it * 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 the copy was written with it, otherwise the edit's source base. + * 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 diff --git a/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md b/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md index f3f4561fc..9724aa837 100644 --- a/openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md +++ b/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 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 has not moved past the copy's `updatedAt`), 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). +`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