[rjzondervan]

Fixes for the round-1 review of #1294 (#1294 (review)). Merge into fix/1285-group-member-share-save, then re-request the review; nothing here is merged by the reviewer.

Findings → commits

Finding	Commit	What changed
f1 🔴 A group member's save leaves the owner no version to restore	b25708a2	ShareSyncService takes an appended, nullable SecretVersionService; writeCopyBlobs() snapshots the source's pre-update state, inside the transaction, before anyone but its owner overwrites it (share-save and /sync, so team-folder writers and delegates too). Only when a value changes, as SecretService::shouldSnapshot(); the owner's own save takes no extra snapshot. Only the source gets a version: secret-version-history keeps the canonical history on the owner's copy.
f2 🟡 /sync opened to group members without share-save's guards	aceeb9d0, b6c14936, 2d90c220	For a group-write caller: an empty expectedUpdatedAt is refused; under the lock the source is loaded again and the caller re-checked (read-only, use-only, still a writer); the source and every live copy must be named. A copy may go without the new value only when its holder has no vault key (share-save's refusedSkips rule), and then its row is not written. The two tests that synced with '' now pass a base.
f4 🟡 user-sharing spec still says either party's update propagates	d0ee2e51	The suggestion at L194, and L12 and L440 reworded to name the owner, delegates and write-grade recipients; git grep "either party" -- openspec/specs is empty.
f6 🟢 Two body claims without a test	47a16bed	A team-folder copy plus group membership gets write; membership lost, or the group share revoked, between resolve() and the lock refuses the save.

Left to the author (threads stay open)

• 🔴 f3 Group membership changes grant write-for-everyone without the owner: a product decision for the PO, not a code change this PR can make.
• 🟢 f5 write-context 404 locks some holders out of their own row: not changed here.
• From the pre-push check, older than fix(sharing): let group members save a shared secret for everyone #1294: /sync for team-folder writers and delegates keeps the old behaviour (empty base, a subset of copies, no re-check under the lock), which existing tests rely on; a source with updatedAt null takes no lock; /sync accepts an empty key next to login/additionalFields, which share-save refuses (assertKeyNotEmpty).

Checks run on this branch

• Full unit suite in the dev container (PHP 8.4, phpunit -c phpunit-unit.xml) → 2365 tests, OK, 46 skipped (database-backed).
• Mutations, one at a time: removing the source snapshot turns GroupMemberWritesForEveryoneTest::testAGroupMembersSyncKeepsTheSourcesPreviousValueAsAVersion, ShareSaveServiceTest::testAWriteGradeMembersSaveKeepsTheSourcesPreviousValueAsAVersion and ::testAGroupMemberWritesTheSourceAndTheOtherCopiesAndIsAuditedAsAGroupWrite red; allowing the empty base turns testAGroupMembersSyncWithoutABaseIsRefused red; dropping the guards under the lock turns four tests red; dropping the read-only and the use-only re-check each turns its own test red; accepting a valueless copy of a holder with a vault key, or writing a keyless holder's copy, each turns its own test red.
• composer phpcs (lib/) → 0 errors.
• Pre-push adversarial check (separate agent), two rounds → round 1 findings fixed in b6c14936 and 2d90c220, round 2 clean.
• Not run: the database-backed tests (ShareSaveVersionLockTest), phpstan, psalm and the e2e spec; CI runs them.

🤖 Generated with Claude Code