feat(sharing): save a shared secret and every copy in one request by SudoThijn · Pull Request #1258 · ConductionNL/keepiq

SudoThijn · 2026-10-13T17:36:39Z

Summary

Closes #1254. Fixes #1253: all four of its bugs, which are all in the web app's sync after an edit.

A value change of a shared secret used to take separate requests: the editor's row, /sync for local copies and one PUT /federated-shares/{id} per partner share. They weren't one transaction, so copies could lag behind without anyone knowing, and in the web app the fan-out didn't work at all (#1253). This adds one request that saves everything together, and moves the web app onto it.

Backend

PUT /api/v1/secrets/{id}/share-save (ShareSaveController, ShareSaveService, lib/Service/ShareSave/). It takes the sparse patch with baseUpdatedAt, copies, federated, skipped and suspend, and writes the row, every copy and every partner share in one transaction, or nothing.
- Owner path and copy-editor path. A write-grade member saves through their own copy. Partner shares stay owner-only; the response says when the editor's save couldn't reach them.
- Version check. A compare-and-set on updated_at under a row lock, with the source locked first. A stale base answers 409 stale / stale_source.
- Completeness check. It runs before the transaction and again under the lock: 409 incomplete with the required lists. A no_vault_key skip is refused when the holder has a vault key.
- Staging, mirroring the certificate lookup's 100. At most 100 entries per list per call. Larger saves go in chunks into keepiq_share_save_stages (migration, 30-minute expiry job, removed on account deletion, not backed up), and the final call commits them in the same transaction.
- Partner notify after commit. It is queued on the row inside the transaction, with a 60-second lease. Conditional writes never replace a pending SHARE_UNSHARED; this also fixes that race on the old routes.
- restoredFrom. A version restore is one share-save, audited in the same transaction. POST /versions/{id}/restore gains an optional baseUpdatedAt.
The old routes keep working for the browser extension.

Web app

src/services/shareSave.js:
- sends with empty lists first, then answers incomplete by looking certificates up in batches of 100 and encrypting sparse patches;
- stages above 100 entries;
- waits out the partner lookup rate limit (at most 5 minutes in total);
- offers "Save without them" only for an unreachable partner.
The edit dialog, offline replay and version restore all go through it. The old fan-out (share.js, federatedShare.js, offline.js) is deleted.
Each Editing a shared secret does not update recipients' copies #1253 bug is fixed:
- certificates are looked up by user;
- the right field names are sent;
- unchanged fields are never cleared;
- a failed save is shown, and the dialog can't be closed while saving.
Offline failures keep their reason through a reload and offer "Try again" where it makes sense.

Docs

OpenSpec change openspec/changes/sharing-atomic-share-save/: proposal, design, tasks, and deltas for user-sharing, federated-sharing, folder-permission-grades, offline-edit-queue, secret-version-history and secrets-write-ui.
CHANGELOG [Unreleased] entries.

Follow-ups

[FEATURE] Share creation sends the source version it encrypted from #1255: share creation should send the source version it encrypted from (a share created during a save can keep the old value).
[FEATURE] Batch partner certificate lookup for the owner's own partner shares #1256: batch partner certificate lookup (one save handles about 360 partner shares now).
Web app partner certificate check: trust in the own server, and verifier strictness #1257: the partner lookup trusts the own server for the cloud id and root (this predates the change).
The browser extension (ext-share-sync on feature/browser-extension-2) moves to share-save separately; after that, /sync and the federated share update route can go.

Test plan

PHPUnit, full suite in the container: 2251 tests, 3 skipped, including DB-backed lock and interleaving tests (ShareSaveVersionLockTest).
npm test: 2055 tests. npm run lint, test:l10n, check:l10n-js and npm run build pass.
phpcs, phpmd and phpstan pass. psalm crashes with the same environment error as on development.
The hydra gates show only the 8 failures that already exist on development; none names a changed file.
Live e2e on the dev instance, tests/e2e/workflows/share-save.spec.ts:
- owner edit;
- copy editor;
- stale second tab;
- rollback on an owner-row refusal;
- version restore;
- a recipient added mid-save;
- offline replay.
Staging above 100 copies (KEEPIQ_E2E_STAGING=1) saved correctly, but its full rerun wasn't completed. Partner shares weren't tested live, because there is no second instance.
Reviewed in three correctness/security rounds and a final rereview, with mutation checks on the guards.

Known limits

A browser Back, or the vault lock redirect, still closes the edit dialog while it is saving.
The new strings were translated by hand in all locales; nobody has reviewed them except en and nl.

Add PUT /api/v1/secrets/{id}/share-save. It takes the editor's sparse patch with baseUpdatedAt, the ciphertext for every local copy and every active partner share, and the copies left out on purpose, and writes them in one transaction or not at all. A stale base or an incomplete list is refused with 409. Saves above 100 entries per list are staged in chunks and committed by the final call. Partner instances are notified after the commit, with the notification queued on the row and never stored over a pending unshare. The web app now saves value edits, offline replays and version restores of shared secrets through it, and shows a failed save as an error. That removes the old fan-out and with it the sync bugs of #1253. The old /sync and federated share update routes stay for the browser extension. Closes #1254 Fixes #1253

github-actions · 2026-10-13T17:45:26Z

Quality Report — ConductionNL/keepiq @ `cfcd1e3`

Check	PHP	Vue	Security	License	Tests
lint	❌
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		❌
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-13 17:45 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-10-13T18:01:39Z

Quality Report — ConductionNL/keepiq @ `78bd266`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2248/2248 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-13 18:01 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-10-13T18:36:53Z

Quality Report — ConductionNL/keepiq @ `12af394`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2248/2248 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-13 18:36 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan

Verdict: REQUEST_CHANGES (Strict)

The share-save core holds: IDOR on every id, stage binding, rollback on every exit, and per-recipient encryption all check out, and CI is fully green. Strict blocks on the eight 🟡 below. Most of them are places where the new guarantees don't reach the paths around share-save: the old routes, the decline/accept receivers, suspend, and owner reassignment.

🔧 Fix PRs: #1259, #1260, #1261 — 10 of 16 findings, one commit each

#1259 backend core (A1, A3, A4) · #1260 federation (B1, B2, B5) · #1261 web client (C1, C3, C4, C5). Merge them into feature/1254-share-save, then re-request the review. The other findings stay on their threads.

🟡 Concerns (8)

Version-restore base check is check-then-write, not compare-and-set — lib/Service/SecretVersionService.php:163. findById, then a PHP compare, then a plain update() with no lock, so a share-save that commits in between is silently overwritten. (fix proposed in #1259)
Old write routes skip the version lock, splitting share-saves — lib/Service/ShareSyncService.php:124. /sync compares expectedUpdatedAt in PHP and never calls advanceVersion (grep advanceVersion lib/Service/ShareSyncService.php is empty), so it can write copies in between the steps of a share-save.
Decline/accept receivers still overwrite a pending unshare — lib/Service/FederatedDeclineReceiver.php:96. Plain shareMapper->update() at lines 97 and 134 after a read that the signatory fetch can stretch out; a partner can race a revoke into DECLINED and then resume it to ACTIVE. This contradicts the PR body's "never replace a pending SHARE_UNSHARED". (fix proposed in #1260)
Suspend keeps queued SHARE_UPDATED; retry pings and flips status — lib/Service/FederatedShareService.php:399. suspendRow leaves pending_notification set and findDueNotifications has no status filter, so the retry job keeps notifying the suspended partner and after 6 attempts writes failed over suspended. (fix proposed in #1260)
Partner shares of a previous owner silently excluded — lib/Service/ShareSave/ShareSaveScope.php:252. After reassignOwner (account deletion with a delegate, or a team transfer), the old owner's partner shares are neither written nor reported, stay pullable with a stale value, and ownedShare lets nobody revoke them.
"Pause"/"resume" promised, but suspension is terminal — openspec/changes/sharing-atomic-share-save/specs/federated-sharing/spec.md:53. The only way back to ACTIVE is from DECLINED (FederatedDeclineReceiver.php:129); nothing resumes a suspended share, yet the spec and the UI string say the owner resumes it later.
stale_source base is read at save time, not edit time — src/services/shareSave.js:359. sourceBaseUpdatedAt comes from the write-context fetched when the save starts, so a source moved by an old route while the dialog was open (or while an offline entry sat in the queue) is overwritten without a 409. (fix proposed in #1261)
"Save without them" pauses partners the user never saw — src/services/shareSave.js:462. With withoutUnreachable, every share that is unreachable on the retry's fresh lookup is suspended, not only the ones listed in the prompt the user agreed to.

🟢 Minor (8)

Write grade is checked only before the transaction — lib/Service/ShareSave/ShareSaveScope.php:107. resolveSyncWriter runs in open(), before atomic(), so a downgrade during the request doesn't stop it. (fix proposed in #1259)
Restore log line is written before the commit — lib/Service/ShareSaveService.php:305. recordRestored logs before recordTeamWrite, stages->drop and the commit, so a rolled-back restore still leaves a log line. (fix proposed in #1259)
Restore audit records an unverified client claim — lib/Service/ShareSaveService.php:151. versionToRestore checks row and ownership only; nothing compares the patch with the version.
Delegate's write to the source is not attributed — lib/Service/ShareSave/ShareSaveHistory.php:107. resolveSyncWriter returns false for a delegate, and recordTeamWrite requires teamWriter.
tasks.md cites a non-existent FederatedShareMapper test — openspec/changes/sharing-atomic-share-save/tasks.md:7. git grep -l 'class FederatedShareMapperTest' -- tests is empty; the cases live in ShareSaveVersionLockTest. (fix proposed in #1260)
Batched cert test uses one key for all users — tests/store/shareSave.spec.js:174. All 230 users get BOB_CERT, so a mix-up of certificates between users can't fail this test. (fix proposed in #1261)
Edit silently waits behind the write-back's partner wait — src/store/modules/secret.js:793. settledBase awaits a write-back that has no onWait and can sit up to 5 minutes in the partner lookup's rate-limit wait. (fix proposed in #1261)
Offline failed list shows a raw error code — src/store/modules/offline.js:124. e.response.data.error is shown verbatim. (fix proposed in #1261)

✅ Verified clean

IDOR: {id} goes through loadOwned (owner = caller). Copy ids are limited to required ∪ ignored and only required is written. Share and suspend ids are limited to the caller's active shares of the source. A copy editor sending any federated/suspend entry, chunks included, gets federated_owner_only.
Staging: a stage is bound to user, secret, base and source base. User B cannot load, extend, commit or drop user A's stage (stage_mismatch is checked before the stale-drop). The final call re-authorises.
"One transaction or nothing" on the share-save route itself: every exit inside write() (stale, stale_source, incomplete, policy refusal, partner revoked) rolls back and has a case in ShareSaveServiceTest. ShareSaveVersionLockTest ran on the pgsql PHPUnit leg of this SHA.
Client crypto: certificates are looked up by userId, never by position, and batching by 100 is correct. Sparse patches keep absent and null apart on edit, write-back and offline replay. The new offline failure is sealed (sealedFailure). The partner-certificate cache holds public certificates in memory only and is cleared on vault lock.
Bounded loops: incomplete stops after 3 rounds; the partner rate-limit wait is capped at 5 min in total and checks vault lock.
Notify path on share-save: writeCiphertext, queue(UPDATED), the attempt outcome, deleteIfPending and suspendRow are conditional writes. notify() runs after atomic(), and a failed send stays queued.
Migrations: the +1 line in the shipped Version001000 only declares the new table in the consolidated schema for fresh installs, and Version001013 is guarded with hasTable and uses explicit keepiq_sss_* index names. No install ends up inconsistent.
Lifecycle: stages are removed on account deletion (deleteByUser), left out of backups (NOT_BACKED_UP, asserted in ArchiveTest), and expired by a job registered in info.xml.
Removed fan-out: syncUpdate, syncAsTeamWriter and federatedShare.syncUpdate have no callers left in src/ or tests/.
Spec anchors: all 19 new @spec tags resolve to a file and heading.

Gates: quality / Hydra Gates green on b89594b8 (CI); no local run. All other checks green, including both required ones.
Persistence audit: run on the PR head: 15 of 26 checks covered, 9 partial, 1 missing. Its partial and missing items are folded into the findings above (write-grade timing, suspend vs. queued notify, owner reassignment, audit gaps). A revoked editor, a cross-user stage and a pending unshare all hold.
ADRs verified: ADR-003, keepiq's RSA/AES encryption architecture, cited in SecretVersionService. The restore re-encrypts in the browser for every recipient, which is consistent with it, but see the 🟢 on the unverified restore claim.
Scope decision: Strict, XL (149 files, 11,737 lines), analysed in three slices: backend core, federation and lifecycle, web client. Residual: the bodies of tests/Unit/Service/ShareServiceTest.php, tests/dialogs/SecretEditDialog.shareSave.spec.js, tests/components/OfflineEdits.spec.js and the e2e helpers were read by outline only. The 74 l10n files were checked for key parity in en, nl, de and fr.

Connected issues

Issue	State	This PR
#1254 Save a shared secret and every copy in one request	open	Implements it (`Closes #1254`); closes on merge.
#1253 Editing a shared secret does not update recipients' copies	open	Fixes its four web-app bugs (`Fixes #1253`); closes on merge.
#1255, #1256, #1257	open	Follow-ups named in the PR body; unaffected by these findings.

🤖 Reviewed with Claude Code

…e, not compare-and-set #1258 (comment) SecretVersionService::restore() now runs the snapshot, the head update and the audit in one transaction (TTransactional::atomic). With a base, the head is first moved off it by SecretMapper::advanceVersion(id, base, savedAt), as ShareSaveService::lockVersions does; a 0 return throws StaleWriteException with the reloaded row and rolls back, so a share-save committing between the read and the write is refused, not overwritten. The head is written with that same savedAt (max(now, base + 1)). A null base keeps today's behaviour: no check, updatedAt = now. Tests: SecretVersionServiceTest gains testARestoreOverAHeadThatMovesAfterTheCheckIsRefusedUntouched (advanceVersion returns 0 -> StaleWriteException, no insert, no update, begin/rollback); the go-ahead case now asserts the compare-and-set and the commit. Class sweep: the other OfflineEditGuard callers (SecretService::update via checkedUpdate, SecretService::findOwned) are pre-existing PUT/expiry paths outside this PR and are left as they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ction #1258 (comment) ShareSaveScope gains assertStillWriter(path, userId), which re-runs ShareSyncService::resolveSyncWriter() on the source for the copy-editor path. ShareSaveService::write() calls it right after lockVersions(), so a grade lowered between resolve() and the lock refuses the save with the same 403 `not_allowed` as the pre-check and rolls it back. The refusal mapping moved into a private writerOn() shared by resolve() and the re-check, so both refuse identically. Test: ShareSaveServiceTest::testAGradeLoweredBeforeTheLockRefusesTheSaveAndRollsBack (resolveGrade write -> read: not_allowed/403, log ['begin','lock src','lock c-bob','rollback'], no team-write audit). Class sweep: resolveSyncWriter has no other caller in the share-save path; the owner path has no grade to re-check. The old syncUpdate keeps its own shape (outside this finding). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…gs and flips status #1258 (comment) suspendRow() now clears pending_notification, notify_attempts and next_notify_at alongside the status, so a share suspended by the owner, by "save without them" or by suspendForPartner() keeps no queued SHARE_UPDATED. updateIfActive() writes the cleared columns (the entity marks them changed; the DB test reads them back). Class sweep: findDueNotifications() now also filters on status. It returns a row only while it is active or while the ending notification (SHARE_UNSHARED, passed in by retryDue so the Db layer does not import the service) is pending on it. A pending unshare lives on a revoked row (and on a failed one after giving up, which has no next_notify_at), so revoked rows still get their unshare delivered; suspended, declined and failed rows that still carry an old SHARE_UPDATED are no longer contacted nor flipped to failed. Tests: - FederatedShareOwnerChangesTest: testASuspendDropsAQueuedUpdate, testRemovingThePartnerDropsAQueuedUpdate (writeCiphertext -> suspend -> retryDue past lease and backoff: nothing sent, still suspended). - ShareSaveVersionLockTest (DB): testASuspendClearsTheQueuedUpdate, testTheRetryJobSkipsAnInactiveShareButDeliversARevokesUnshare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#1258 (comment) SecretVersionService::recordRestored() now only dispatches the audit event (which commits or rolls back with the transaction); the nextcloud.log line moved to a new logRestored(). ShareSaveService::save() calls it through ShareSaveHistory::logRestore() after atomic() returns, so a share-save that fails after recordRestore (team-write audit, stage drop, COMMIT) leaves no "restored" log line. Class sweep: SecretVersionService::restore() had the same ordering; it now logs after its own transaction (added in A1) commits. Tests: ShareSaveServiceTest::testARestoreThatFailsAfterItsAuditIsNotLogged (team-write audit throws after recordRestored -> rollback, logRestored never called); the restore happy path asserts the log comes after 'commit'. SecretVersionServiceTest::testARestoreThatRollsBackIsNotLogged, the happy path asserts begin/commit/log order, and testARecordedRestoreIsLoggedOnlyByLogRestored replaces testARecordedRestoreIsLogged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…dit time #1258 (comment) saveShared() took sourceBaseUpdatedAt from the write-context it fetched when the save started, so stale_source only caught a source that moved in the milliseconds between that GET and the PUT. It now takes the source version the edit was made from (options.sourceBaseUpdatedAt) and falls back to the save-time context.sourceUpdatedAt only when none was recorded. The fallback is read once, from the first write-context, so the owner re-lookup after a refused skip no longer moves it either. What each saveShared caller now sends: - Edit dialog: reads the write-context beside the secret on open and records the source version when the secret is a copy (null for an own secret, and offline, where nothing can be read); the save waits for it and passes it through updateSecret -> saveShared. - updateSecret: passes options.sourceBaseUpdatedAt to saveShared, and to the offline queue entry (new plain field sourceBaseUpdatedAt next to baseUpdatedAt; sealEntry/PLAIN_FIELDS/coalesce keep it, coalesce keeps the earliest one recorded). - Merge write-back (persistMergedPending): records none, so it takes the save-time version; it is made from the row fetched a moment before. An edit made from the version the write-back started on now moves its source base forward to the write-back's own sourceUpdatedAt (never back), as it already did for baseUpdatedAt; without that, a copy edit during a write-back would be refused as stale_source against the write-back itself. settledBase() keeps its signature; the new settledBases() returns both. - Offline replay: sends the entry's recorded source version; an entry from before this change (null) takes the replay-time version. The legacy `sync` re-save keeps the replay-time version, as its base is the row fetched at replay too. "Keep mine" on a conflict clears the recorded source version, so a stale_source conflict the user overrides does not come back on every replay. - Version restore: unchanged, the save-time version. A restore replaces the whole value with a chosen version; nothing in the history panel reads the source. Spec deltas (secrets-write-ui, offline-edit-queue) say the same. Tests: shareSave.spec copy-editor case asserts the edit-time source version; new cases for stale_source on a source that moved since the edit, the save-time fallback, and the write-back moving the source base on; offlineQueue.spec records and replays it and clears it on "keep mine"; offline-queue.spec coalesce keeps the earliest; the dialog spec records the version it opened on. The fixture's 200 answer now carries sourceUpdatedAt, as the server's does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#1258 (comment) The 100-per-call certificate test gave all 230 users Bob's key, so a certificate paired with the wrong user (by position, or shifted across a batch boundary) still passed. Now user-0 and user-150, on either side of the 100 boundary, hold the primary and secondary fixture keys and every other user a third key generated with generateKeyPair(); the lookup answers each batch in reverse order. The test decrypts copy-user-0, copy-user-150 and copy-user-99 each with its own holder's private key, so an index-based userId -> certificate mapping fails it. Class sweep: the other one-key batches in this spec (the staged-round describe and the restoredFrom chunk test) check staging, not who a copy is for; the decrypting tests at the copy-editor path already use two keys. Left as they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nding unshare #1258 (comment) FederatedDeclineReceiver::handle() and resume() no longer write with the plain shareMapper->update(). Both go through the new FederatedShareMapper::updateIfStatus(share, status, ending), in the style of updateIfActive: the UPDATE matches only while the row still has the status it was read with and SHARE_UNSHARED is not pending on it. When 0 rows match (the owner revoked while the partner's signature was being checked), the revoke wins: nothing is written or audited, handle() answers [] as it does for an already-revoked row, and resume() throws ShareNotFound as it does for a share that ended. Class sweep: the "not pending SHARE_UNSHARED" predicate also covers a revocation whose retries gave up (status failed, SHARE_UNSHARED still pending). Without it a decline turned that row into declined and a later accept made it active again. No other plain update() of keepiq_federated_shares remains outside revoke() itself and the unshare queue, which are meant to win. Tests: - FederatedCopyDeclineTest: testADeclineNeverUndoesARevokeThatLandedMeanwhile, testADeclineKeepsTheUnshareOfARevocationThatGaveUp. - FederatedCopyRestoreTest: testARevokeThatLandedWhileTheAcceptWasCheckedStaysEnded. (Both alicesProvider() mappers now hand out copies, so a read can go stale, and model updateIfStatus.) - ShareSaveVersionLockTest (DB): testAnAcceptReadBeforeARevokeLeavesTheShareRevoked, testADeclineReadBeforeARevokeLeavesTheShareRevoked (the revoke runs through FederatedShareService::revoke with a failing messenger inside getIncomingSignedRequest), testARecipientsChangeIsWrittenOnlyWhileNoUnshareIsPending. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…artner wait #1258 (comment) A dialog save that settledBase() held behind a merge write-back waited for the whole write-back, up to PARTNER_LIMIT_WAIT_CAP_MS when that one waited on the partner lookup limit, without the "waiting for the lookup limit" note: the user's onWait only fired for the user's own save. The write-back now passes its own onWait, which records the wait on its in-flight entry and tells every save held behind it. settledBases() takes the caller's onWait (updateSecret passes options.onWait), calls it at once when the write-back is already waiting, and registers it until the write-back settles. The dialog therefore shows secret-edit-waiting for an inherited wait exactly as for its own. Class sweep: the only other caller held behind a write-back is the version restore (secretVersion.restore -> settledBase); its panel has no waiting note, so it passes none and is unchanged. Test: SecretEditDialog.shareSave.spec — a write-back held on a 429 shows secret-edit-waiting while the user's save waits, and the save then goes out on the write-back's version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…Mapper test #1258 (comment) Task 1.3 now names where its verification lives: the partner-share cases in ShareSaveVersionLockTest (the database tests of updateIfActive, updateIfUnchanged, deleteIfPending, findDueNotifications and the new updateIfStatus) and the queued-update cases in FederatedShareOwnerChangesTest (including the suspend cases added for B2). Class sweep: git grep for "FederatedShareMapper database" / "FederatedShareMapperTest" finds no other copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#1258 (comment) failureText() showed a refused offline change as "The server refused this change ({reason})" with the server's machine code (invalid_key, too_many_entries, source_base_required, ...) in the user's failed list. It now reuses shareSaveErrorText() on the stored code, so a code that function knows (version_refused) gets its sentence, and any other code gets the existing translated "Failed to save secret". The code itself stays stored with the entry and is no longer shown. No new string: "Failed to save secret" is already translated in every locale, and a new key in en/nl only would fail the l10n parity check for the other 35 locales. The key "The server refused this change ({reason})." is now unused; it is left in the 74 locale files rather than churning them in this fix. Class sweep: the other failure kinds (partner_unreachable, partner_lookup_limit, not_allowed, server) already render sentences or the server's translated message; no other place renders failure.reason. Tests: OfflineEdits.spec asserts the invalid_key entry reads "Failed to save secret" without "invalid_key"; offlineQueue.spec covers the version_refused mapping and the fallback for unknown and empty codes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…he user-facing text #1258 (comment) "a share save the server refuses as malformed fails the entry with its reason" pinned the raw code in the shown text, which C5 removed. It now asserts the code stays stored with the entry ({code: 'refused', reason: 'invalid_key'}), the shown reason is the translated "Failed to save secret", and that text does not contain 'invalid_key'. Sweep: git grep for toContain('invalid_key'), reason).toContain and .reason) under tests finds no other test pinning the offline refusal code (OfflineEdits.spec was updated in C5; the rest are unrelated reasons of import, org policy, save policy and federation certificates). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ad under the lock #1258 (comment) assertStillWriter() re-checked the grade on $path->source, the row read before the transaction. Moving a secret to another folder (SecretMapper::updateFolderForSecrets) changes folder_id without bumping updated_at, so the source compare-and-set does not catch it, and the re-check judged the grade on the old folder. It now reads the source again with SecretMapper::findById() after the locks and checks the grade on that row; a source deleted meanwhile is NotFoundException, as in resolve(). Test: ShareSaveServiceTest::testASourceMovedOutOfItsTeamFolderBeforeTheLockRefusesTheSave (source leaves the write-grade folder after resolve(): not_allowed/403, ['begin','lock src','lock c-bob','rollback']). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… changed under the lock #1258 (comment) assertStillWriter() discarded writerOn()'s result, so ShareSavePath::teamWriter could be stale: a delegation revoked while a write grade remains (or the reverse) let the save through and audited it as the wrong kind of write. It now refuses with the same 403 `not_allowed` when the fresh value differs from $path->teamWriter. Refuse rather than re-path: the path is immutable and built before the transaction, the case needs a permission change racing an in-flight save, and refusing keeps the attribution honest with one comparison. The client reloads and saves again on the current path. Test: ShareSaveServiceTest::testADelegationRevokedBeforeTheLockRefusesTheSaveEvenWithAWriteGrade (delegate at resolve(), write-grade member under the lock: not_allowed/403, rollback, no team-write audit). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… not found, not a 500 #1258 (comment) When advanceVersion() returns 0 because the head was deleted after the base check, restoreHead() reloaded it with findById(), whose DoesNotExistException escaped atomic() and reached the controller, which catches only StaleWriteException and InvalidArgumentException (500). A private reload() now maps it to InvalidArgumentException('Secret not found'), mirroring ShareSaveService::reload(); the restore route answers it with 400, as it answers a missing version. The transaction still rolls back. Test: SecretVersionServiceTest::testARestoreOverAHeadDeletedAfterTheCheckIsRefusedAsNotFound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…s the head past its version #1258 (comment) On the null-base path restoreHead() set updatedAt = new DateTime(), which can equal (same second) or precede (clock skew) the stored updated_at; the restored head then kept its pre-restore version and a later edit based on that version passed advanceVersion/the stale check and overwrote the restore. savedAt is now always max(time(), stored + 1), like ShareSaveService::savedAt(). With a base the value is unchanged (the base check guarantees base == stored). Test: SecretVersionServiceTest::testARestoreWithoutABaseMovesTheHeadPastItsStoredVersion (stored version one hour ahead, no base -> updatedAt = stored + 1, no compare-and-set). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ot found, not a 500 #1258 (comment) SecretVersionService::restore() read the head for the base check with a bare SecretMapper::findById(); a secret deleted between getVersion() and that read threw DoesNotExistException past the controller (500). It now uses the private reload() added in 15d48a3, so the route answers InvalidArgumentException('Secret not found') with 400, like a missing version. Sweep (git grep 'secretMapper->findById\|secrets->findById' over SecretVersionService, lib/Service/ShareSave, ShareSaveService): no other bare read left. SecretVersionService::reload(), ShareSaveService::reload() and ShareSaveScope (resolve(), assertStillWriter()) already map DoesNotExistException. Test: SecretVersionServiceTest::testARestoreOfAHeadDeletedBeforeItsReadIsRefusedAsNotFound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…elated change #1258 (comment) The conditional write from the previous B1 commit matched only the status read before the signature check. A decline whose row changed for any other reason meanwhile (active -> suspended by suspendRow, routine through "save without them" and suspendForPartner, or active -> failed by a retry give-up) was answered [] and dropped: no DECLINED status, no audit, and the partner never resends. handle() now goes through decline(row, again): when the conditional write matches 0 rows, the row is read once more and decided again. Revoked or declined, or SHARE_UNSHARED pending: the revoke wins and nothing is written. Otherwise the decline is applied against the newly read status. After that one retry it gives up as []. resume() goes through accept(row, again) with the same one re-read. The choice for the accept: from declined the only other changes are a revoke (and the unshare deleting the row) or a concurrent accept. A row re-read as active (another accept won) is answered [] as for a live share; one re-read as declined is tried once more (sentAgain checked again); revoked, suspended, failed or gone throws ShareNotFound, the answer for a share that ended. Tests (red on 2cb80e7, green now): - FederatedCopyDeclineTest::testADeclineStillCountsWhenTheShareChangedMeanwhile (owner suspends / retry job gives an update up during the check) - FederatedCopyRestoreTest::testAnAcceptThatLosesToAnotherAcceptFindsTheShareLive - ShareSaveVersionLockTest (DB): testADeclineReadBeforeASuspendStillDeclinesTheShare (receiverRacedByARevoke is now built on a general receiverRacedBy) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…tion #1258 (comment) A revocation whose retries gave up (status failed, SHARE_UNSHARED still pending, next_notify_at NULL) is never retried. The previous B1 commits answered a decline for it with a no-op, which left a zombie row. But the partner's SHARE_DECLINED proves its copy is gone, and removing the copy was the whole point of the unshare. decline() now finishes the revocation whenever SHARE_UNSHARED is pending on the row, whether the status is revoked (retries still running) or failed (given up), on the first read or on the re-read after a lost conditional write. It does this the way FederatedNotificationDelivery::attempt() handles a delivered unshare: deleteIfPending(share, SHARE_UNSHARED). That path records no audit of its own (the revoke was audited when it was made), so neither does this one. This corrects the wording of 5803f5a. There, "the revoke wins" meant nothing was written. Now the revoke wins and is completed: the row goes, the decline never marks it declined, and a later SHARE_ACCEPTED for it gets ShareNotFound, as for a removed share. The class docblock and the decline() docblock say so. Tests (red on 78e04e9, green now): - FederatedCopyDeclineTest: testARevocationOnItsWayStaysARevocation -> testADeclineFinishesARevocationOnItsWay; testADeclineKeepsTheUnshareOfARevocationThatGaveUp (which pinned the zombie) -> testADeclineFinishesARevocationThatGaveUp; testADeclineNeverUndoesARevokeThatLandedMeanwhile now expects the row gone. The mapper mock gained deleteIfPending, and $outbound is null once deleted. - ShareSaveVersionLockTest (DB): testADeclineReadBeforeARevokeLeavesTheShareRevoked -> testADeclineReadBeforeARevokeFinishesTheRevocation; new testADeclineFinishesARevocationThatGaveUp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…e versions it sent and kept #1258 (comment) Regression in e3bf188: after a merge write-back through a copy had FINISHED, a copy editor's save was refused as stale_source against the write-back itself. The write-back record was deleted when it settled, the store had already moved the open copy (and so the dialog's `original`) to the write-back's version, and the dialog still sent the source version it read before the write-back. The forward move for a write-back in flight also moved any older source version forward, masking an owner change that landed between the dialog's read and the write-back's. - saveShared() returns `sentSourceBaseUpdatedAt`, the source version it sent (null on the owner path). - The write-back record keeps base, the sent source version, and the versions it wrote (updatedAt, sourceUpdatedAt), and stays after it settles, until the next write-back of the secret or the next save of it (updateSecret, and the version restore, drop a settled one through the new dropSettledWriteBack()). A settled record no longer replays its partner-limit wait to a later save. - settledBases() moves an edit to (updatedAt, sourceUpdatedAt) only when its copy version is the write-back's base or result AND its source version is the one the write-back sent or wrote (or it recorded none). The write-back is the only change between those pairs, so this covers an edit read before it and one whose copy the store already moved, and any other version (an owner change) leaves the edit's own versions, so the server answers stale / stale_source. The old "never back" maximum is gone. Edits with no recorded source version (restore, old queue entries) behave as before. - Dialog: reads the write-context after fetchSecret resolves, never before the copy it shows; submit awaits the source read before it reads `original.updatedAt`, so both are read after the wait. Tests (red before this commit): - shareSave.spec: a finished write-back, edit from the moved copy version and the opened source version -> sent as (SAVED_AT, SAVED_AT); an in-flight write-back that sent another source version than the edit -> the edit keeps (BASE, EDIT_SOURCE) and gets stale_source. - SecretEditDialog.shareSave.spec: fetchSecret returns the store's currentSecret and starts the write-back, which finishes before submit; the save sends the write-back's versions and the dialog closes saved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ource version #1258 (comment) e3bf188 said coalesce "keeps the earliest one recorded" but used `existing ?? incoming`, which took a later edit's source version when the first change recorded none. A coalesced entry now keeps the first change's source version as it keeps its baseUpdatedAt, null included: null means the replay takes the source version of its own moment, the documented fallback, rather than a version a later edit read. The queue.js docblock says so; offline-queue.spec covers a chain whose first change recorded one (kept) and one whose first recorded none (null). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…match #1258 (comment) 12f55ec kept a settled merge write-back until the next successful save of the secret. A save the server refused as changed meanwhile (409) left it in place, so a later edit or version restore made from the version before the write-back was still moved past it: in the suite, "shows a stale restore of the own copy like any stale save, and changes nothing" sent the write-back's version instead of BASE, after the preceding stale_source test left its record behind. That turns an honest stale into a silent overwrite. The test's expectation is right and is unchanged. A 409 from a save now forgets the settled write-back (forgetWriteBackOnRefusal(), called from updateSecret's failure path and from secretVersion.restore's): the refused edit is redone from a fresh open, so nothing made from before the write-back is matched to it any more. Other failures (partner unreachable, network) keep it, so "Save without them" or a retry of the same edit still matches. Callers of settledBase/settledBases checked: updateSecret (settledBases) and secretVersion.restore (settledBase) are the only two; both now drop the record on success (12f55ec) and on a 409 (here). A restore made while a write-back is in flight still moves past it, as the author's "bases a restore made during the write-back" test requires; once the write-back has finished, baseOf() already returns the version the store moved the open copy to. Test: shareSave.spec "forgets the write-back once an edit was refused, so a restore from before it is not moved past it" (red on 4aa8357). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hat conflict's versions A `stale` 409's row version lived only in the in-memory conflict, so after a reload "keep mine" replayed on the base the 409 refused and the user had to choose again. The source version also differed: null before a reload, which let the replay take the source of its own moment, and the edit's original source base after one, which a source that moved is refused on. The entry now stores the 409's row version as `conflictUpdatedAt`, in plain next to `conflictSourceUpdatedAt`. After a `stale` 409 of a copy the source version is read right after it, so keeping the change overrides the source the user decided against and is refused again if it moves later. resolveConflict() reads both from the entry, before and after a reload alike, and clears them.

… its 409 names A share-save of a copy now answers `stale` with the source's version as `sourceUpdatedAt`, as it does `stale_source`, so the replay keeps that version with the conflict instead of reading /write-context after the 409. The read cost a request and could see a source that moved after the 409, which keeping the change would then have saved over. A `stale` without the key needs no read: the server leaves it out only on the owner path, an own secret has no source, and the web app ships with the server that names it. A copy holder without a write grade replays a plain update, whose 409 names no source version and which sends none, so nothing is kept for it and keeping the change replays on the row version alone.

PUT /secrets/{id}/sync audited only a team writer. Anyone but the owner is now recorded on the source, a delegate as `delegate sync`, the same shape a share-save through a copy uses, and the entry is written inside the sync's transaction.

…sion A 409 `stale` on the copy-editor path now carries `sourceUpdatedAt` next to `current`, as `stale_source` does, so the web app's offline queue can keep it with the conflict without reading /write-context. It is the source's version as the save found it: the read before the transaction, or the base the source keeps when the copy refuses under the lock. The owner path leaves it out.

…'s secret

github-actions · 2026-10-14T15:54:01Z

Quality Report — ConductionNL/keepiq @ `de342d2`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2314/2314 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-14 15:53 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan

Verdict: Request changes (Strict, round 3)

1 new blocker: a stale copy's 409 names the source's current version, so "keep mine" can overwrite a source-only change the user never saw. All 6 round-2 threads are resolved. Before a merge: that blocker, and its thread, since development requires every review thread resolved and 1 approval.

🔧 Fix PR: #1272 — 1 of 1 findings, one fix commit plus four pre-push follow-ups

Merge it into feature/1254-share-save, then re-request the review.

Round	Found	Status
1 (`b89594b8`)	🟡 8 · 🟢 8 (A1–A6, B1–B5, C1–C5)	resolved (fix PRs #1259–#1261 and the author's round-2 and round-3 commits)
2 (`c4c5b5b3`)	🔴 1 (offline source version) · 🟢 3 (stale replay after reload, `/sync` delegate audit, app-owned request fill)	resolved in `e3161fc`, `fb78ff2`/`fb9d878`, `dd4dd75`, `ac072fb`
3 (`218e7b06`) — this review	🟡 1 (stale copy names an unseen source version, fix-induced)	open

Finding	Was	Now	Where
🔴 The offline cache drops sourceUpdatedAt, so an offline copy edit still records no source version	🔴 round 2	resolved in `e3161fc`	thread
🟡 stale_source base is read at save time, not edit time	🟡 round 1	resolved in `e3161fc` (offline half)	thread
🟢 After a reload, "keep mine" on a stale copy conflict replays on the refused base	🟢 round 2	resolved in `fb78ff2`, `fb9d878`	thread
🟢 A delegate's write through /sync is not attributed on the source	🟢 round 2	resolved in `dd4dd75`	thread
🟢 Delegate's write to the source is not attributed	🟢 round 1	resolved in `dd4dd75` (with `dbc0d9d`)	thread
🟢 A request fill on an application-owned secret skips the version lock	🟢 round 2	resolved in `ac072fb` (changelog narrowed to user-owned secrets)	thread
🟡 A stale copy's 409 names a source version the conflict never showed	new	new — 🟡, fix-induced (`99320a6`), blocks under the round-3 rule (fix proposed in #1272)	comment

Connected issues — checked, 0 changed

Issue	State	What it is	Note
#1254	open	the share-save feature this PR adds	finished by this PR. The base is `development`, not `main`, so "Closes" won't close it on merge
#1253	open	the web app's sync after an edit doesn't reach recipients' copies	finished by this PR, same as above
#1255, #1256, #1257	open	follow-ups named in the PR body	unaffected

Nothing was closed: both finished issues stay open until the PR is merged.

Details of this round

Scope: the 8 commits since round 2: 6 fixes and 2 local merges (a4123b9 backend, 218e7b0 web). git show --cc shows no conflict resolution in either. The diff is 15 files, +616/-60. One agent read it in Strict mode, with the surface enumeration scoped to the delta.
Round-3 rule: a new 🟡 blocks only when this PR or its fix commits introduced it. N1 comes from 99320a6, which the round-3 fixes added, so it blocks.
Verified clean: sourceUpdatedAt survives the whole path: manifest → encryptSnapshot → IndexedDB → decryptSnapshot → offline.vault → dialog → queue entry → replay. conflictUpdatedAt/conflictSourceUpdatedAt are sealed as plain fields, re-sealed on suite rotation and cleared by resolveConflict(), and entries queued before this change fall back to their own bases. The /sync audit runs inside the transaction, so an audit failure rolls the sync back. The stale 409's sourceUpdatedAt is shown only to callers who pass writerOn(), who can already read it from /write-context, so it leaks nothing. The narrowed changelog has no unqualified copy left in the specs, design.md or the PR body.
Local run at 218e7b0: npx vitest run (node 24): 251 files, 2088 tests passed. PHPUnit unit (ShareSaveServiceTest|ShareSaveControllerTest|ShareServiceTest, in the dev stack's Nextcloud container): 128 tests passed. The N1 repro case failed as expected and was then removed.
CI: green on 218e7b06 (40 succeeded, 9 skipped by their conditions).
Gates: CI quality / Hydra Gates ✅ on 218e7b06.

Before the next round: this round's one finding was fix-induced (fix rules on: fix-induced 1 + missed-sibling 0 ≥ new 0), so the next fixes go through the class sweep and the pre-push check first:

vendor/bin/phpunit -c phpunit-unit.xml --filter ShareSaveServiceTest: a case with c-bob stale and src moved alone must expect BASE as the named source version
npx vitest run tests/store/offlineQueue.spec.js tests/vitest/offline-snapshot.spec.js

… conflict never showed 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: #1258 (comment) 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 <noreply@anthropic.com>

…opy's 409 names 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: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…n cost one more choice 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: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ase it checks 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: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nly when its version equals the copy's 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: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

rjzondervan

Verdict: Approve (Quick, round 4)

Nothing left to address: every blocking finding from rounds 1–3 is resolved, and 0 review threads are open. What still stands between the PR and a merge: CI on 03898f96 is partly still running, and quality / Security (composer) is red (not a required check; details below).

🔧 Fix PR: #1272 — merged, 1 of 1 verified

Round	Found	Status
1 (`b89594b8`)	🟡 8 · 🟢 8 (A1–A6, B1–B5, C1–C5)	resolved (fix PRs #1259–#1261 and the author's round-2 and round-3 commits)
2 (`c4c5b5b3`)	🔴 1 · 🟢 3	resolved in round 3's author commits
3 (`218e7b06`)	🟡 1 (stale copy names an unseen source version)	fixed in #1272
4 (`03898f96`) — this review	none	—

Finding	Was	Now	Where
🟡 A stale copy's 409 names a source version the conflict never showed	🟡 round 3	resolved in #1272 (`ebdf3758`)	thread

Details of this round

Scope: the only change since round 3 is the merge of #1272 (03898f96, parents 218e7b06 and 78fad4a2). git show --cc shows no conflict resolution, and git diff 78fad4a2 03898f96 is empty, so the head's tree is the fix branch's tree. No analysis agent ran, because there are no other commits.
N1 on the head: open() names the source's current version on a stale 409 only when it equals the copy's version. testAStaleCopyDoesNotNameASourceVersionThatMovedAlone and testAStaleCopyNamesTheSourceVersionItWasWrittenWith pin both branches: each of three mutations (flip the condition, always the base, always the current source) turns one red. ShareSaveServiceTest|ShareSaveControllerTest|ShareServiceTest: 130 tests OK on that tree.
CI on 03898f96 when this was posted: 32 succeeded, 9 skipped by their conditions, 7 still running (PHPUnit stable35 pgsql, vitest, Hydra Gates, Newman, standalone, two-instances). quality / Security (composer) failed. It was green on 218e7b06 and on development at 03:21 today, and #1272 changes no composer file, so it most likely reports an advisory published since then against a dependency. Its log wasn't readable yet while the workflow run was still in progress. It is not one of development's required checks (Conflict markers and PHP syntax, Frontend Check (format)), but it is worth reading before the merge.
Gates: CI quality / Hydra Gates still running on 03898f96 at this verdict; gate findings are not part of this round.

github-actions · 2026-10-15T12:34:17Z

Quality Report — ConductionNL/keepiq @ `ace0776`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2316/2316 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-15 12:34 UTC

Download the full PDF report from the workflow artifacts.

…hare-save # Conflicts: # src/store/modules/federatedShare.js # tests/store/federatedShare.sync.spec.js

github-actions · 2026-10-15T12:54:15Z

Quality Report — ConductionNL/keepiq @ `7c104e6`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		❌
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2321/2321 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-15 12:54 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-10-15T13:08:14Z

Quality Report — ConductionNL/keepiq @ `74fdadc`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2321/2321 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-15 13:08 UTC

Download the full PDF report from the workflow artifacts.

SudoThijn added 2 commits October 13, 2026 19:36

docs(openspec): say share-save fixes all four bugs of #1253

5db856e

fix(ci): PHP 8.3 syntax in a share-save test, prettier, missing @SPEC

15d7dc8

test(federation): follow the owner's update through share-save

b89594b

rjzondervan reviewed Oct 14, 2026

View reviewed changes

rjzondervan requested changes Oct 14, 2026

View reviewed changes

rjzondervan and others added 21 commits October 14, 2026 10:42

SudoThijn added 7 commits October 14, 2026 17:13

docs(review): #1258 — changelog: the request-fill retry covers a user…

ac072fb

…'s secret

Merge #1258 round-3 review fixes (backend)

a4123b9

Merge #1258 round-3 review fixes (web)

218e7b0

SudoThijn mentioned this pull request Oct 14, 2026

An application's write to its secret can overwrite a concurrent write #1269

Open

1 task

rjzondervan reviewed Oct 15, 2026

View reviewed changes

Comment thread lib/Service/ShareSaveService.php

rjzondervan requested changes Oct 15, 2026

View reviewed changes

rjzondervan and others added 5 commits October 15, 2026 12:53

rjzondervan mentioned this pull request Oct 15, 2026

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

Merged

Merge pull request #1272 from ConductionNL/review/pr-1258-fixes-r3

03898f9

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

rjzondervan previously approved these changes Oct 15, 2026

View reviewed changes

fix(deps): bump symfony/yaml to v7.4.21 (CVE-2026-109413)

e635519

Merge remote-tracking branch 'origin/development' into feature/1254-s…

b38273d

…hare-save # Conflicts: # src/store/modules/federatedShare.js # tests/store/federatedShare.sync.spec.js

SudoThijn added 2 commits October 15, 2026 14:56

style(test): format the verified-key spec

84bb627

fix(test): drop the duplicate pemOf in the federated certificate spec

01a5fa8

SudoThijn mentioned this pull request Oct 15, 2026

fix(test): drop the duplicate pemOf in the federated certificate spec #1275

Closed

This was referenced Oct 15, 2026

[FEATURE] Save a shared secret and every copy of it in one request #1254

Closed

Editing a shared secret does not update recipients' copies #1253

Closed

feat: Edit items and folders, share edits, themes and layout in the browser extension #1249

Open

Conversation

SudoThijn commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Backend

Web app

Docs

Follow-ups

Test plan

Known limits

Uh oh!

github-actions Bot commented Oct 13, 2026

Quality Report — ConductionNL/keepiq @ cfcd1e3

Uh oh!

github-actions Bot commented Oct 13, 2026

Quality Report — ConductionNL/keepiq @ 78bd266

Uh oh!

github-actions Bot commented Oct 13, 2026

Quality Report — ConductionNL/keepiq @ 12af394

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjzondervan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Verdict: REQUEST_CHANGES (Strict)

🔧 Fix PRs: #1259, #1260, #1261 — 10 of 16 findings, one commit each

🟡 Concerns (8)

🟢 Minor (8)

✅ Verified clean

Connected issues

Uh oh!

github-actions Bot commented Oct 14, 2026

Quality Report — ConductionNL/keepiq @ de342d2

Uh oh!

Uh oh!

rjzondervan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Verdict: Request changes (Strict, round 3)

🔧 Fix PR: #1272 — 1 of 1 findings, one fix commit plus four pre-push follow-ups

Connected issues — checked, 0 changed

Uh oh!

rjzondervan left a comment

Choose a reason for hiding this comment

Verdict: Approve (Quick, round 4)

🔧 Fix PR: #1272 — merged, 1 of 1 verified

Uh oh!

github-actions Bot commented Oct 15, 2026

Quality Report — ConductionNL/keepiq @ ace0776

Uh oh!

github-actions Bot commented Oct 15, 2026

Quality Report — ConductionNL/keepiq @ 7c104e6

Uh oh!

github-actions Bot commented Oct 15, 2026

Quality Report — ConductionNL/keepiq @ 74fdadc

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SudoThijn commented Oct 13, 2026 •

edited

Loading

Quality Report — ConductionNL/keepiq @ `cfcd1e3`

Quality Report — ConductionNL/keepiq @ `78bd266`

Quality Report — ConductionNL/keepiq @ `12af394`

rjzondervan left a comment •

edited

Loading

Quality Report — ConductionNL/keepiq @ `de342d2`

rjzondervan left a comment •

edited

Loading

Quality Report — ConductionNL/keepiq @ `ace0776`

Quality Report — ConductionNL/keepiq @ `7c104e6`

Quality Report — ConductionNL/keepiq @ `74fdadc`