feat(api): guard unpin with ownership check and reference-counted quota decrement#485
Merged
Conversation
Entire-Checkpoint: ae7bbe1126cc
Entire-Checkpoint: 8f94afd7fcd8
Entire-Checkpoint: f3c45f8a62fe
Entire-Checkpoint: d5bddac316f1
Entire-Checkpoint: 9cbc6d7a27e3
Finish the interrupted planner run: add plans 42-04..42-08 (live migration gate, controller wiring + api:generate, BullMQ outbox drain + drift report, non-BYO backfill script, Grafana cross-user-attempt alert). Correct stale cross-plan number references in 42-01/42-03 and finalize the ROADMAP entry. Covers all 13 CONTEXT decisions (D-01..D-13) and both requirement IDs across the 8-plan set; every new plan carries a STRIDE threat_model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 951500ea043f
Entire-Checkpoint: 0a2deed2f95d
…e registration - PendingUnpin entity with unique cid index, no userId per D-05 outbox design - Migration 1749000000000 creates pending_unpins table with idx_pending_unpins_cid - Migration 1749100000000 adds idx_pinned_cids_cid to pinned_cids for refcount queries - PendingUnpin registered in app.module entities array per DATABASE_EVOLUTION_PROTOCOL §4.2
- Assert deleteFile calls unpinFromIpfs, removeUsage, then fetchQuota in order - Assert fetchQuota rejection does not reject deleteFile and logs a warning - Assert both removeUsage and fetchQuota are invoked before resolve
…eleteFile - Fire fetchQuota() as fire-and-forget after removeUsage (D-12) - Rejection is swallowed via .catch(logger.warn) to keep delete path non-blocking - No new imports; logger already imported on line 3
- RED/GREEN TDD gates verified, 3 specs passing - One-line fire-and-forget fetchQuota after removeUsage (D-12)
…vice - unpinCrossUserAttempts counter for D-02 cross-user unpin audit - driftOrphanedPinsTotal counter for D-06 Kubo drift report - pendingUnpinsGauge for D-05 outbox depth monitoring
Entire-Checkpoint: db0759160182
- Applied AddPendingUnpins1749000000000 and AddPinnedCidCidIndex1749100000000 - Both migrations confirmed [X] in migration:show (entries 18 and 19) - pending_unpins table + idx_pending_unpins_cid + idx_pinned_cids_cid verified via to_regclass() - No source files modified; halted at checkpoint:human-verify for DB confirmation
- Provisions CipherBox Security rule group alert on cipherbox_unpin_cross_user_attempts_total - Fires on any non-zero rate(counter[5m]) — gt 0 threshold (D-10, D-02) - Uses GRAFANA_ALERTS_FOLDER_UID and GRAFANA_CLOUD_DATASOURCE_UID placeholder UIDs - noDataState/execErrState OK; for 5m; severity warning, operation unpin-security
…rovider mocks - six guardedUnpin cases: no-row-unknown, no-row-cross-user, refcount>0, refcount===0 success, refcount===0 Kubo-fail, advisory-lock-ordering - DataSource mock with transaction callback invoking manager mock - manager.getRepository returns per-entity mocks for PinnedCid and PendingUnpin - IPFS_PROVIDER mock and MetricsService mock injected into TestingModule
- SUMMARY.md for CipherBox Security alert rule provisioning
…nd refcount outbox - advisory xact lock as first statement via manager.query pg_advisory_xact_lock (D-04) - ownership check with cross-user detection, logger.warn, and metric inc (D-01, D-02) - in-transaction row delete as quota decrement (D-03) - refcount gate with orIgnore outbox insert when count hits zero (D-05) - post-commit best-effort Kubo unpin with outbox row cleanup on success (D-03 ordering) - Kubo failure leaves outbox row for BullMQ retry worker without rejecting the request - vault.module.ts adds PendingUnpin to forFeature and provides IPFS_PROVIDER locally without importing IpfsModule to avoid circular dependency
Entire-Checkpoint: c6c34f9e6764
…sation - Expand unpin describe to pass req with user.id; assert guardedUnpin called and ipfsProvider.unpinFile NOT called - Add Test 4: compensation routes through guardedUnpin on recordPin failure, not raw unpinFile - Add Test 5: compensation is best-effort; guardedUnpin rejection swallowed, original recordPin error rethrown - Add Test 3: upload happy path does not call guardedUnpin or unpinFile
RED phase: selectRowsToDelete D-09 predicate (BYO exclusion, CID presence check) and parseKuboPinLs NDJSON parser. Five behaviors per the plan spec. No implementation yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Seven cases: drain success, drain not-pinned=success, drain failure-leaves-row - Gauge publish after drain pass - Drift orphan detected increments counter without deleting - Drift all-accounted does not increment counter - Dispatch routing to drain/drift/no-op for unknown jobs - RED: fails because PendingUnpinProcessor does not exist yet
… guardedUnpin - Replace ipfsProvider.unpinFile in unpin() with vaultService.guardedUnpin(req.user.id, dto.cid) - Add @request() req to unpin() signature; remove controller-level fileUnpins.inc (now inside guardedUnpin per 42-03) - Replace ipfsProvider.unpinFile in upload compensation with vaultService.guardedUnpin; add D-13 race window comment - Regenerate api-client via pnpm api:generate; openapi.json formatting-only diff confirms D-11 no schema change
github-actions
Bot
added
release:web:feat
Contributor
Release Preview
Cascade Details
|
- Add Entire CLI husky hooks (post-commit, post-rewrite, pre-push, prepare-commit-msg); commit-msg now skips gracefully when the CLI is absent - Ignore .planning/tmp/ (GSD docs-generation scratch) - Remove DEFERRED.md and REFACTORING.md (deprecated, removed by gsd health) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: e9643b3221d4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/api/src/ipfs/pending-unpin/pending-unpin.processor.ts (1)
81-89: 🏗️ Heavy liftDrift report currently materializes full DB CID sets in memory.
Pulling all rows from
pinned_cidsandpending_unpinseach run will degrade as data grows; consider paginated reads or a DB-driven diff strategy to cap memory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/ipfs/pending-unpin/pending-unpin.processor.ts` around lines 81 - 89, The code in pending-unpin.processor.ts currently builds dbCids by loading all rows from pinnedCidRepository.find(...) and pendingUnpinRepository.find(...), which will OOM as tables grow; change the implementation to avoid materializing full sets by (a) using paginated/streamed reads from pinnedCidRepository and pendingUnpinRepository (e.g., repository.createQueryBuilder().select('cid').orderBy('cid').limit(pageSize).offset/where cursor) and merge/process each page incrementally, or (b) better, push the diff into the DB with a set-based query (e.g., SELECT cid FROM pending_unpins WHERE cid NOT IN (SELECT cid FROM pinned_cids) or use LEFT JOIN WHERE pinned_cids.cid IS NULL) so you never load all cids into memory; update the code paths that reference dbCids to consume the incremental/DB-driven result instead of the in-memory Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/ipfs/ipfs.controller.ts`:
- Around line 122-127: The internal rollback call to VaultService.guardedUnpin
in the upload error path should not trigger cross-user audit metrics; update the
caller (the await this.vaultService.guardedUnpin(...) in the upload error
handling) to pass an options object like { suppressCrossUserAudit: true }, and
then modify VaultService.guardedUnpin to gate the branch that increments/records
cipherbox_unpin_cross_user_attempts_total and any cross-user log messages behind
a check for !options?.suppressCrossUserAudit so internal suppression prevents
emitting that metric/log.
In `@apps/api/src/ipfs/pending-unpin/pending-unpin.module.ts`:
- Around line 59-63: The catch block that currently logs a warning ("Failed to
register PendingUnpin schedulers (non-fatal): ...") swallows scheduler
registration failures in all environments; update it so only development
continues silently: keep the current this.logger.warn(...) and return in
development (e.g. if process.env.NODE_ENV === 'development'), but for
non-development environments log at error level (this.logger.error(..., error))
and rethrow the error (or exit startup) so the process fails fast and the
failure isn't silently ignored; change the catch surrounding the PendingUnpin
scheduler registration (the code that builds the `message` and calls
this.logger.warn) to implement this environment-aware behavior.
In `@apps/api/src/ipfs/pending-unpin/pending-unpin.processor.ts`:
- Around line 105-107: The fetch call that assigns to response in
pending-unpin.processor.ts (the call to
`${this.apiUrl}/api/v0/pin/ls?type=recursive`) has no timeout and can hang the
worker; update the call to use an AbortController with a configurable timeout
(e.g., 15–30s) and pass controller.signal into fetch options, start a timer to
call controller.abort() after the timeout, and clear the timer once fetch
resolves/rejects so the request is always aborted on timeout and resources are
cleaned up; ensure you handle the abort error path where the fetch rejects due
to the timeout and surface/log a clear error so the drift job can continue.
In `@apps/api/src/vault/vault.service.ts`:
- Around line 255-258: The advisory lock must be acquired as the first
transactional statement; remove the separate hashtext query and replace both
lines with a single call that computes and acquires the lock in one
statement—e.g. use manager.query('SELECT
pg_advisory_xact_lock(abs(hashtext($1))::bigint)', [cid]) instead of extracting
h first via manager.query and then calling pg_advisory_xact_lock; ensure this
single manager.query call is the first statement in the transaction where the
current code (the two manager.query calls referencing cid and h) appears.
In `@apps/web/src/services/delete.service.ts`:
- Line 22: The call site in delete.service.ts uses
quotaStore.fetchQuota().catch(...), but fetchQuota (in quota.store.ts) currently
swallows API errors and never rejects, so the catch is never reached; to fix,
change quotaStore.fetchQuota() implementation to propagate errors (rethrow or
return a rejected Promise) when the underlying API call fails so callers like
deleteFile can perform fire-and-forget error handling, and keep the existing
catch(logger.warn(...)) in delete.service.ts to satisfy Phase 42-02 logging
requirements.
In `@scripts/backfill-pinned-cids.ts`:
- Around line 172-193: The loop that deletes batches (using ids, BATCH_SIZE and
incrementing totalDeleted) currently catches batchErr and logs but still leads
to process.exit(0), making failures appear successful; modify the code to record
when any batch fails (e.g., set a boolean like hadBatchFailure = true inside the
catch for batchErr) and after the loop/during shutdown call
dataSource.destroy(), then exit non-zero if hadBatchFailure (use process.exit(1)
instead of process.exit(0)); ensure existing logging for batch errors remains
and totalDeleted logic is unchanged.
---
Nitpick comments:
In `@apps/api/src/ipfs/pending-unpin/pending-unpin.processor.ts`:
- Around line 81-89: The code in pending-unpin.processor.ts currently builds
dbCids by loading all rows from pinnedCidRepository.find(...) and
pendingUnpinRepository.find(...), which will OOM as tables grow; change the
implementation to avoid materializing full sets by (a) using paginated/streamed
reads from pinnedCidRepository and pendingUnpinRepository (e.g.,
repository.createQueryBuilder().select('cid').orderBy('cid').limit(pageSize).offset/where
cursor) and merge/process each page incrementally, or (b) better, push the diff
into the DB with a set-based query (e.g., SELECT cid FROM pending_unpins WHERE
cid NOT IN (SELECT cid FROM pinned_cids) or use LEFT JOIN WHERE pinned_cids.cid
IS NULL) so you never load all cids into memory; update the code paths that
reference dbCids to consume the incremental/DB-driven result instead of the
in-memory Set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06355b19-bcfc-47c0-8bc0-011913e65716
📒 Files selected for processing (50)
.planning/PROJECT.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/42-api-unpin-integrity/42-01-PLAN.md.planning/phases/42-api-unpin-integrity/42-01-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-02-PLAN.md.planning/phases/42-api-unpin-integrity/42-02-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-03-PLAN.md.planning/phases/42-api-unpin-integrity/42-03-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-04-PLAN.md.planning/phases/42-api-unpin-integrity/42-04-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-05-PLAN.md.planning/phases/42-api-unpin-integrity/42-05-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-06-PLAN.md.planning/phases/42-api-unpin-integrity/42-06-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-07-PLAN.md.planning/phases/42-api-unpin-integrity/42-07-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-08-PLAN.md.planning/phases/42-api-unpin-integrity/42-08-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-CONTEXT.md.planning/phases/42-api-unpin-integrity/42-DISCUSSION-LOG.md.planning/phases/42-api-unpin-integrity/42-PATTERNS.md.planning/phases/42-api-unpin-integrity/42-RESEARCH.md.planning/phases/42-api-unpin-integrity/42-REVIEW.md.planning/phases/42-api-unpin-integrity/42-VALIDATION.md.planning/phases/42-api-unpin-integrity/42-VERIFICATION.md.planning/todos/done/2026-06-11-ipfs-unpin-missing-ownership-check.md.planning/todos/done/2026-06-11-server-quota-never-decremented-on-unpin.md.prettierignoreapps/api/src/app.module.tsapps/api/src/ipfs/ipfs.controller.spec.tsapps/api/src/ipfs/ipfs.controller.tsapps/api/src/ipfs/pending-unpin/pending-unpin.module.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.tsapps/api/src/metrics/metrics.service.tsapps/api/src/migrations/1749000000000-AddPendingUnpins.tsapps/api/src/migrations/1749100000000-AddPinnedCidCidIndex.tsapps/api/src/scripts/backfill-helpers.spec.tsapps/api/src/scripts/backfill-helpers.tsapps/api/src/vault/entities/index.tsapps/api/src/vault/entities/pending-unpin.entity.tsapps/api/src/vault/vault.module.tsapps/api/src/vault/vault.service.spec.tsapps/api/src/vault/vault.service.tsapps/web/src/services/delete.service.spec.tsapps/web/src/services/delete.service.tsdocker/grafana/alerts/unpin-cross-user-attempts.jsonrelease-please-config.jsonscripts/backfill-pinned-cids.ts
Fixes prettier/prettier lint errors failing CI on PR #485. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 2ad409cff4c8
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
===========================================
+ Coverage 62.38% 84.62% +22.23%
===========================================
Files 135 101 -34
Lines 10162 6030 -4132
Branches 1085 1108 +23
===========================================
- Hits 6340 5103 -1237
+ Misses 3598 693 -2905
- Partials 224 234 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Address the six unresolved review threads on the unpin-integrity PR: - guardedUnpin takes a suppressCrossUserAudit option; the upload-error rollback passes it so a deduped-CID compensation no longer emits a false cross-user security signal - guardedUnpin acquires the advisory xact lock as the first transactional statement by computing the hashtext key inline - PendingUnpin scheduler registration fails fast in production instead of swallowing the error in every environment - bound the Kubo pin/ls fetch with a 30s AbortSignal timeout so a network hang cannot stall the drift job - quota store fetchQuota resolves a success flag so deleteFile logs the reconcile warning on real failures, keeping its no-throw contract for the fire-and-forget callers - backfill script exits non-zero when any delete batch fails Also rename delete.service.spec.ts to delete.service.test.ts so vitest's *.test.ts include glob actually runs it, and update the affected unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 085d7019ea7e
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/delete.service.test.ts (1)
51-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOrder assertion for
fetchQuotais incomplete.The test title promises "then fetchQuota in order" but line 53 only asserts
mockFetchQuotawas called once. It doesn't verify thatfetchQuotawas actually third in thecallOrderarray. If a future refactor reorders the calls, this test might still pass incorrectly.Proposed fix
expect(callOrder[0]).toBe('unpinFromIpfs'); expect(callOrder[1]).toBe('removeUsage'); - expect(mockFetchQuota).toHaveBeenCalledOnce(); + expect(callOrder[2]).toBe('fetchQuota');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/services/delete.service.test.ts` around lines 51 - 53, The test promises to verify "then fetchQuota in order" but only checks mockFetchQuota was called once; update the assertions to also verify ordering by adding an expectation that callOrder[2] (the third call) equals 'fetchQuota' and optionally replace mockFetchQuota assertion with expect(mockFetchQuota).toHaveBeenCalledTimes(1) for clarity. Locate the assertions around callOrder and mockFetchQuota in delete.service.test.ts and add expect(callOrder[2]).toBe('fetchQuota'); while keeping or changing the existing mockFetchQuota assertion to toHaveBeenCalledTimes(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/src/services/delete.service.test.ts`:
- Around line 51-53: The test promises to verify "then fetchQuota in order" but
only checks mockFetchQuota was called once; update the assertions to also verify
ordering by adding an expectation that callOrder[2] (the third call) equals
'fetchQuota' and optionally replace mockFetchQuota assertion with
expect(mockFetchQuota).toHaveBeenCalledTimes(1) for clarity. Locate the
assertions around callOrder and mockFetchQuota in delete.service.test.ts and add
expect(callOrder[2]).toBe('fetchQuota'); while keeping or changing the existing
mockFetchQuota assertion to toHaveBeenCalledTimes(1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cb5a2f2-e56d-439b-9082-5c09d678926d
📒 Files selected for processing (19)
.gitignore.husky/commit-msg.husky/post-commit.husky/post-rewrite.husky/pre-push.husky/prepare-commit-msg.planning/DEFERRED.md.planning/REFACTORING.mdapps/api/src/ipfs/ipfs.controller.spec.tsapps/api/src/ipfs/ipfs.controller.tsapps/api/src/ipfs/pending-unpin/pending-unpin.module.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.tsapps/api/src/vault/vault.service.spec.tsapps/api/src/vault/vault.service.tsapps/web/src/services/delete.service.test.tsapps/web/src/services/delete.service.tsapps/web/src/stores/quota.store.tsscripts/backfill-pinned-cids.ts
💤 Files with no reviewable changes (2)
- .planning/REFACTORING.md
- .planning/DEFERRED.md
✅ Files skipped from review due to trivial changes (5)
- .husky/prepare-commit-msg
- .husky/post-rewrite
- .husky/pre-push
- .husky/post-commit
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/src/services/delete.service.ts
- apps/api/src/ipfs/pending-unpin/pending-unpin.module.ts
- apps/api/src/vault/vault.service.spec.ts
- apps/api/src/ipfs/ipfs.controller.ts
- apps/api/src/ipfs/pending-unpin/pending-unpin.processor.ts
- scripts/backfill-pinned-cids.ts
- apps/api/src/ipfs/ipfs.controller.spec.ts
- apps/api/src/vault/vault.service.ts
Addresses CodeRabbit outside-diff finding: the 'in order' test pushed
'fetchQuota' to callOrder but only asserted it was called once. Add
expect(callOrder[2]).toBe('fetchQuota') to actually verify ordering.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 738503093ae2
Entire-Checkpoint: a4c7c3940bbb
Owner
Author
|
@coderabbitai please review the changes in this PR |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
apps/web/src/services/delete.service.test.ts (1)
30-74: ⚡ Quick winStrengthen edge-case coverage for the fire-and-forget contract.
Current tests are good, but two high-value cases are still missing: (1)
fetchQuotanever settles (to provedeleteFileis truly non-blocking), and (2)unpinFromIpfsrejects (to prove quota mutation/reconcile are skipped on failure).✅ Suggested test additions
describe('deleteFile', () => { @@ it('invokes both removeUsage and fetchQuota before resolving', async () => { await deleteFile('bafytest789', 512); expect(mockRemoveUsage).toHaveBeenCalledWith(512); expect(mockFetchQuota).toHaveBeenCalledOnce(); }); + + it('does not block deleteFile when fetchQuota never settles', async () => { + mockFetchQuota.mockImplementationOnce(() => new Promise<boolean>(() => {})); + + const result = await Promise.race([ + deleteFile('bafypending', 256).then(() => 'resolved'), + new Promise<'timeout'>((resolve) => setTimeout(() => resolve('timeout'), 25)), + ]); + + expect(result).toBe('resolved'); + }); + + it('rejects when unpin fails and does not touch quota state', async () => { + vi.mocked(unpinFromIpfs).mockRejectedValueOnce(new Error('unpin failed')); + + await expect(deleteFile('bafyfail', 999)).rejects.toThrow('unpin failed'); + expect(mockRemoveUsage).not.toHaveBeenCalled(); + expect(mockFetchQuota).not.toHaveBeenCalled(); + }); });As per coding guidelines for
**/*.test.ts, this improves edge-case coverage and test quality for the intended non-blocking flow. Based on learnings from the Phase 42 acceptance context, non-blocking reconcile behavior should be explicitly proven.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/services/delete.service.test.ts` around lines 30 - 74, Add two tests to strengthen the fire-and-forget contract for deleteFile: (1) "fetchQuota never settles" — mock fetchQuota to return a never-resolving Promise (e.g. new Promise(()=>{})), call await deleteFile('id', size) and assert it resolves quickly (no hang), then after a microtask tick (await Promise.resolve()) assert mockRemoveUsage was calledWith(size) and that the test did not await fetchQuota; (2) "unpinFromIpfs rejects" — mock unpinFromIpfs to reject with an Error, call await expect(deleteFile('id', size)).rejects.toThrow(theError) and assert mockRemoveUsage and mockFetchQuota were not called and logger.warn was not invoked; reference the functions deleteFile, fetchQuota, unpinFromIpfs, removeUsage, and logger.warn when locating spots to add these tests.Source: Coding guidelines
.husky/post-commit (1)
4-4: 💤 Low valueConsider consistent error messaging across hooks.
This hook silently continues when
entireCLI is missing, while.husky/commit-msgprints a helpful error message. If the silent behavior is intentional (post-commit is less visible), that's fine; otherwise, consider consistent user feedback across all hooks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.husky/post-commit at line 4, The post-commit hook currently swallows the missing "entire" CLI (the if/then/else using command -v entire) whereas your commit-msg hook prints an error; update the else branch for the existing check for the "entire" command so it prints a helpful error to stderr (mirroring the commit-msg message) and keep the non-failing behavior (preserve the current || true semantics) so commits don't break; target the conditional that invokes entire hooks git post-commit and replace the no-op else with an stderr echo that instructs the user that the "entire" CLI is not installed and how to install or configure it.apps/api/src/vault/vault.service.spec.ts (1)
933-944: ⚡ Quick winThe test name promises
logger.warnverification but doesn’t assert it.Add an explicit warning assertion (or rename the test) so this remains a meaningful security-audit test instead of partially-checked behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/vault/vault.service.spec.ts` around lines 933 - 944, The test "no-row, cross-user: calls unpinCrossUserAttempts.inc and logger.warn, no delete, no Kubo" promises to assert a warning was logged but doesn't; update the test in vault.service.spec.ts to explicitly assert the logger warning by adding an expectation on the mocked logger.warn (e.g., expect(mockLogger.warn).toHaveBeenCalledTimes(1) or expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('cross-user') / the specific message the guardedUnpin path logs)), while keeping the existing checks for mockMetricsService.unpinCrossUserAttempts.inc, mockManagerPinnedCidRepo.delete, and mockIpfsProvider.unpinFile. Ensure you reference the same mock logger instance used in the test harness so the assertion verifies the guardedUnpin logging path.apps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.ts (1)
149-198: ⚡ Quick winAdd explicit drift failure-path tests (timeout/non-OK/malformed NDJSON).
The suite validates drift happy paths, but it doesn’t directly lock in the new failure guards in
fetchKuboPins. Adding these cases will prevent regressions in timeout handling and parser resilience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.ts` around lines 149 - 198, Add explicit tests in pending-unpin.processor.spec.ts that exercise fetchKuboPins failure paths: (1) a timeout/rejected fetch (mock global.fetch to reject), (2) a non-OK response (mock fetch to resolve { ok: false }), and (3) malformed NDJSON (mock fetch.text to return invalid JSON/NDJSON). For each test call processor.process(makeJob('drift-report')) and assert it completes without throwing, that no DB deletes or IPFS unpins occur (mockPendingUnpinRepository.delete and mockIpfsProvider.unpinFile are not called), and restore the fetch mock after each case; this will lock in the failure guards implemented in fetchKuboPins.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.husky/prepare-commit-msg:
- Line 3: Edit the .husky/prepare-commit-msg hook to stop discarding stderr from
the Entire CLI: remove the "2>/dev/null" redirection on the `entire hooks git
prepare-commit-msg "$1" "$2"` invocation so legitimate errors are printed, while
leaving the existing `|| true` behavior intact to avoid failing the hook.
- Line 3: The prepare-commit-msg hook currently silences the absence of the
entire binary (it uses the no-op ':'), causing inconsistent messaging with the
commit-msg hook; update the .husky/prepare-commit-msg script so that when
command -v entire fails it prints the same helpful install/usage message as
.husky/commit-msg instead of doing nothing, and keep the existing call to entire
hooks git prepare-commit-msg "$1" "$2" 2>/dev/null || true unchanged when entire
is present.
In @.planning/phases/42-api-unpin-integrity/42-07-PLAN.md:
- Line 151: The table row contains unescaped pipe characters in the string
"host|password|user(name)=" which are breaking the Markdown table; update that
cell in the row (the T-42-25 entry) to either escape each pipe (replace '|' with
'\|') or rephrase the text (e.g., "host, password, user(name)=" or
"host\|password\|user(name)=") so the row renders as a single table cell and
passes markdownlint.
In @.planning/phases/42-api-unpin-integrity/42-VALIDATION.md:
- Line 22: The quick-run command's Jest pattern currently escapes the
alternation (`--testPathPattern='(ipfs\|vault)'`) which matches the literal
`ipfs|vault`; change the pattern to use a real alternation by replacing
`(ipfs\|vault)` with `(ipfs|vault)` in the command (`pnpm --filter
`@cipherbox/api` test -- --testPathPattern='(ipfs|vault)'`), updating the doc
string so Jest runs tests for either "ipfs" or "vault".
In `@apps/api/src/ipfs/ipfs.controller.ts`:
- Around line 122-130: The rollback call in ipfs.controller.ts to
vaultService.guardedUnpin(...) is a no-op when no pinned_cids row exists, so
Kubo pins can be left orphaned if recordPin fails; fix by changing the
VaultService.guardedUnpin implementation so it does not return early when the
owned {userId, cid} row is missing but instead still enqueues a PendingUnpin (or
directly calls ipfsProvider.unpinFile) with the passed options (preserving
suppressCrossUserAudit), ensuring the Kubo pin is cleaned up; update any unit
tests that assert the old early-return behavior to expect the
PendingUnpin/enqueue or direct unpin call, and keep the ipfs.controller.ts
rollback call to guardedUnpin unchanged.
In `@apps/api/src/scripts/backfill-helpers.ts`:
- Around line 59-61: The catch block that currently swallows malformed NDJSON
lines must fail closed: instead of silently skipping non-JSON progress lines,
surface the error so the backfill aborts before delete decisions are made.
Replace the empty catch in the NDJSON parsing code with logic that logs the
malformed line/context and rethrows (or returns a failing Result/throws a
specific Error) so the caller of the backfill helper cannot proceed with a
partial CID set; update the parsing location (the try/catch around NDJSON
parsing in backfill-helpers.ts) accordingly and ensure callers either propagate
or handle the thrown error so deletes are not executed on incomplete data.
In `@scripts/backfill-pinned-cids.ts`:
- Around line 85-87: The fetch call to
`${KUBO_API_URL}/api/v0/pin/ls?type=recursive` currently has no timeout; add an
AbortSignal timeout by passing signal:
AbortSignal.timeout(KUBO_PIN_LS_TIMEOUT_MS) in the fetch options so the request
is aborted after the configured KUBO_PIN_LS_TIMEOUT_MS (matching the processor
implementation), e.g. include signal:
AbortSignal.timeout(KUBO_PIN_LS_TIMEOUT_MS) alongside method: 'POST' to ensure
the backfill job fails fast on network stalls.
---
Nitpick comments:
In @.husky/post-commit:
- Line 4: The post-commit hook currently swallows the missing "entire" CLI (the
if/then/else using command -v entire) whereas your commit-msg hook prints an
error; update the else branch for the existing check for the "entire" command so
it prints a helpful error to stderr (mirroring the commit-msg message) and keep
the non-failing behavior (preserve the current || true semantics) so commits
don't break; target the conditional that invokes entire hooks git post-commit
and replace the no-op else with an stderr echo that instructs the user that the
"entire" CLI is not installed and how to install or configure it.
In `@apps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.ts`:
- Around line 149-198: Add explicit tests in pending-unpin.processor.spec.ts
that exercise fetchKuboPins failure paths: (1) a timeout/rejected fetch (mock
global.fetch to reject), (2) a non-OK response (mock fetch to resolve { ok:
false }), and (3) malformed NDJSON (mock fetch.text to return invalid
JSON/NDJSON). For each test call processor.process(makeJob('drift-report')) and
assert it completes without throwing, that no DB deletes or IPFS unpins occur
(mockPendingUnpinRepository.delete and mockIpfsProvider.unpinFile are not
called), and restore the fetch mock after each case; this will lock in the
failure guards implemented in fetchKuboPins.
In `@apps/api/src/vault/vault.service.spec.ts`:
- Around line 933-944: The test "no-row, cross-user: calls
unpinCrossUserAttempts.inc and logger.warn, no delete, no Kubo" promises to
assert a warning was logged but doesn't; update the test in
vault.service.spec.ts to explicitly assert the logger warning by adding an
expectation on the mocked logger.warn (e.g.,
expect(mockLogger.warn).toHaveBeenCalledTimes(1) or
expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('cross-user')
/ the specific message the guardedUnpin path logs)), while keeping the existing
checks for mockMetricsService.unpinCrossUserAttempts.inc,
mockManagerPinnedCidRepo.delete, and mockIpfsProvider.unpinFile. Ensure you
reference the same mock logger instance used in the test harness so the
assertion verifies the guardedUnpin logging path.
In `@apps/web/src/services/delete.service.test.ts`:
- Around line 30-74: Add two tests to strengthen the fire-and-forget contract
for deleteFile: (1) "fetchQuota never settles" — mock fetchQuota to return a
never-resolving Promise (e.g. new Promise(()=>{})), call await deleteFile('id',
size) and assert it resolves quickly (no hang), then after a microtask tick
(await Promise.resolve()) assert mockRemoveUsage was calledWith(size) and that
the test did not await fetchQuota; (2) "unpinFromIpfs rejects" — mock
unpinFromIpfs to reject with an Error, call await expect(deleteFile('id',
size)).rejects.toThrow(theError) and assert mockRemoveUsage and mockFetchQuota
were not called and logger.warn was not invoked; reference the functions
deleteFile, fetchQuota, unpinFromIpfs, removeUsage, and logger.warn when
locating spots to add these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d1d0c96-e290-4ebb-baf9-1f729cfd4899
📒 Files selected for processing (60)
.gitignore.husky/commit-msg.husky/post-commit.husky/post-rewrite.husky/pre-push.husky/prepare-commit-msg.planning/DEFERRED.md.planning/PROJECT.md.planning/REFACTORING.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/42-api-unpin-integrity/42-01-PLAN.md.planning/phases/42-api-unpin-integrity/42-01-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-02-PLAN.md.planning/phases/42-api-unpin-integrity/42-02-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-03-PLAN.md.planning/phases/42-api-unpin-integrity/42-03-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-04-PLAN.md.planning/phases/42-api-unpin-integrity/42-04-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-05-PLAN.md.planning/phases/42-api-unpin-integrity/42-05-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-06-PLAN.md.planning/phases/42-api-unpin-integrity/42-06-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-07-PLAN.md.planning/phases/42-api-unpin-integrity/42-07-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-08-PLAN.md.planning/phases/42-api-unpin-integrity/42-08-SUMMARY.md.planning/phases/42-api-unpin-integrity/42-CONTEXT.md.planning/phases/42-api-unpin-integrity/42-DISCUSSION-LOG.md.planning/phases/42-api-unpin-integrity/42-PATTERNS.md.planning/phases/42-api-unpin-integrity/42-RESEARCH.md.planning/phases/42-api-unpin-integrity/42-REVIEW.md.planning/phases/42-api-unpin-integrity/42-SECURITY.md.planning/phases/42-api-unpin-integrity/42-VALIDATION.md.planning/phases/42-api-unpin-integrity/42-VERIFICATION.md.planning/todos/done/2026-06-11-ipfs-unpin-missing-ownership-check.md.planning/todos/done/2026-06-11-server-quota-never-decremented-on-unpin.md.prettierignoreapps/api/src/app.module.tsapps/api/src/ipfs/ipfs.controller.spec.tsapps/api/src/ipfs/ipfs.controller.tsapps/api/src/ipfs/pending-unpin/pending-unpin.module.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.spec.tsapps/api/src/ipfs/pending-unpin/pending-unpin.processor.tsapps/api/src/metrics/metrics.service.tsapps/api/src/migrations/1749000000000-AddPendingUnpins.tsapps/api/src/migrations/1749100000000-AddPinnedCidCidIndex.tsapps/api/src/scripts/backfill-helpers.spec.tsapps/api/src/scripts/backfill-helpers.tsapps/api/src/vault/entities/index.tsapps/api/src/vault/entities/pending-unpin.entity.tsapps/api/src/vault/vault.module.tsapps/api/src/vault/vault.service.spec.tsapps/api/src/vault/vault.service.tsapps/web/src/services/delete.service.test.tsapps/web/src/services/delete.service.tsapps/web/src/stores/quota.store.tsdocker/grafana/alerts/unpin-cross-user-attempts.jsonrelease-please-config.jsonscripts/backfill-pinned-cids.ts
💤 Files with no reviewable changes (2)
- .planning/DEFERRED.md
- .planning/REFACTORING.md
Addresses CodeRabbit round-2 review on PR 485: - parseKuboPinLs fails closed on malformed NDJSON instead of skipping it; a corrupt/truncated pin/ls response would otherwise undercount the live CID set and misclassify legitimately-pinned rows as phantom deletes - backfill pin/ls fetch is bounded by AbortSignal.timeout, matching the pending-unpin processor, so a network stall can't hang the job - escape the pipe chars in the 42-07 threat-register table so it renders Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: c2da7518be38
Merged
This was referenced Jun 14, 2026
FSM1
added a commit
that referenced
this pull request
Jun 14, 2026
…ec duplicate Addresses CodeRabbit review on PR #488: - file.test.ts: updateFileMetadata zeroizes fileIpnsPrivateKey in-place, so the shared mockPrivateKey was all-zero after the first test. Reinitialize it per test in beforeEach so each case exercises a real non-zero key. - delete.service.spec.ts: stale branch-phase-42 duplicate of the canonical delete.service.test.ts (from #485). It never ran (web vitest includes only *.test.ts) and its fetchQuota-rejection assertion did not match the resolved-falsy implementation contract. Removed; coverage fully retained by delete.service.test.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 52515563f6ea
FSM1
added a commit
that referenced
this pull request
Jun 14, 2026
…S publish (#488) * docs(42): capture phase context Entire-Checkpoint: ae7bbe1126cc * docs(state): record phase 42 context session Entire-Checkpoint: 8f94afd7fcd8 * docs(42): research phase api-unpin-integrity Entire-Checkpoint: f3c45f8a62fe * docs(phase-42): add validation strategy Entire-Checkpoint: d5bddac316f1 * chore: exclude GSD-controlled files from prettier formatting Entire-Checkpoint: 9cbc6d7a27e3 * docs(42): complete phase plan for api unpin integrity Finish the interrupted planner run: add plans 42-04..42-08 (live migration gate, controller wiring + api:generate, BullMQ outbox drain + drift report, non-BYO backfill script, Grafana cross-user-attempt alert). Correct stale cross-plan number references in 42-01/42-03 and finalize the ROADMAP entry. Covers all 13 CONTEXT decisions (D-01..D-13) and both requirement IDs across the 8-plan set; every new plan carries a STRIDE threat_model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 951500ea043f * docs(42): record planning gates and decision coverage Entire-Checkpoint: 0a2deed2f95d * feat: add PendingUnpin entity, unpin outbox migrations, and app.module registration - PendingUnpin entity with unique cid index, no userId per D-05 outbox design - Migration 1749000000000 creates pending_unpins table with idx_pending_unpins_cid - Migration 1749100000000 adds idx_pinned_cids_cid to pinned_cids for refcount queries - PendingUnpin registered in app.module entities array per DATABASE_EVOLUTION_PROTOCOL §4.2 * test(42-02): add failing spec for fetchQuota reconcile after removeUsage - Assert deleteFile calls unpinFromIpfs, removeUsage, then fetchQuota in order - Assert fetchQuota rejection does not reject deleteFile and logs a warning - Assert both removeUsage and fetchQuota are invoked before resolve * feat(42-02): reconcile quota with server after local removeUsage in deleteFile - Fire fetchQuota() as fire-and-forget after removeUsage (D-12) - Rejection is swallowed via .catch(logger.warn) to keep delete path non-blocking - No new imports; logger already imported on line 3 * docs(42-02): complete quota reconcile plan summary - RED/GREEN TDD gates verified, 3 specs passing - One-line fire-and-forget fetchQuota after removeUsage (D-12) * feat: add cross-user, drift, and pending-unpins metrics to MetricsService - unpinCrossUserAttempts counter for D-02 cross-user unpin audit - driftOrphanedPinsTotal counter for D-06 Kubo drift report - pendingUnpinsGauge for D-05 outbox depth monitoring * docs: complete 42-01 schema and metrics foundation plan * docs(phase-42): update tracking after wave 1 Entire-Checkpoint: db0759160182 * docs: complete 42-04 migration apply and verification gate - Applied AddPendingUnpins1749000000000 and AddPinnedCidCidIndex1749100000000 - Both migrations confirmed [X] in migration:show (entries 18 and 19) - pending_unpins table + idx_pending_unpins_cid + idx_pinned_cids_cid verified via to_regclass() - No source files modified; halted at checkpoint:human-verify for DB confirmation * feat: add Grafana alert for unpin cross-user attempt rate - Provisions CipherBox Security rule group alert on cipherbox_unpin_cross_user_attempts_total - Fires on any non-zero rate(counter[5m]) — gt 0 threshold (D-10, D-02) - Uses GRAFANA_ALERTS_FOLDER_UID and GRAFANA_CLOUD_DATASOURCE_UID placeholder UIDs - noDataState/execErrState OK; for 5m; severity warning, operation unpin-security * test(42-03): add failing guardedUnpin spec with DataSource and IPFS provider mocks - six guardedUnpin cases: no-row-unknown, no-row-cross-user, refcount>0, refcount===0 success, refcount===0 Kubo-fail, advisory-lock-ordering - DataSource mock with transaction callback invoking manager mock - manager.getRepository returns per-entity mocks for PinnedCid and PendingUnpin - IPFS_PROVIDER mock and MetricsService mock injected into TestingModule * docs: complete 42-08 plan — unpin cross-user attempt Grafana alert - SUMMARY.md for CipherBox Security alert rule provisioning * feat(42-03): implement VaultService.guardedUnpin with advisory lock and refcount outbox - advisory xact lock as first statement via manager.query pg_advisory_xact_lock (D-04) - ownership check with cross-user detection, logger.warn, and metric inc (D-01, D-02) - in-transaction row delete as quota decrement (D-03) - refcount gate with orIgnore outbox insert when count hits zero (D-05) - post-commit best-effort Kubo unpin with outbox row cleanup on success (D-03 ordering) - Kubo failure leaves outbox row for BullMQ retry worker without rejecting the request - vault.module.ts adds PendingUnpin to forFeature and provides IPFS_PROVIDER locally without importing IpfsModule to avoid circular dependency * docs(42-03): complete guardedUnpin security core plan summary * docs(phase-42): update tracking after wave 2 Entire-Checkpoint: c6c34f9e6764 * test(42-05): add failing tests for guardedUnpin delegation and compensation - Expand unpin describe to pass req with user.id; assert guardedUnpin called and ipfsProvider.unpinFile NOT called - Add Test 4: compensation routes through guardedUnpin on recordPin failure, not raw unpinFile - Add Test 5: compensation is best-effort; guardedUnpin rejection swallowed, original recordPin error rethrown - Add Test 3: upload happy path does not call guardedUnpin or unpinFile * test: add failing specs for backfill-helpers pure functions RED phase: selectRowsToDelete D-09 predicate (BYO exclusion, CID presence check) and parseKuboPinLs NDJSON parser. Five behaviors per the plan spec. No implementation yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(42-06): add failing processor spec for drain and drift behaviors - Seven cases: drain success, drain not-pinned=success, drain failure-leaves-row - Gauge publish after drain pass - Drift orphan detected increments counter without deleting - Drift all-accounted does not increment counter - Dispatch routing to drain/drift/no-op for unknown jobs - RED: fails because PendingUnpinProcessor does not exist yet * feat(42-05): wire unpin to guardedUnpin; reroute compensation through guardedUnpin - Replace ipfsProvider.unpinFile in unpin() with vaultService.guardedUnpin(req.user.id, dto.cid) - Add @request() req to unpin() signature; remove controller-level fileUnpins.inc (now inside guardedUnpin per 42-03) - Replace ipfsProvider.unpinFile in upload compensation with vaultService.guardedUnpin; add D-13 race window comment - Regenerate api-client via pnpm api:generate; openapi.json formatting-only diff confirms D-11 no schema change * feat: implement backfill-helpers pure functions GREEN phase: selectRowsToDelete (D-09 predicate, BYO exclusion at assertion layer) and parseKuboPinLs (NDJSON line-by-line parser per Pitfall 6 research). All unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(42-06): implement PendingUnpinProcessor and PendingUnpinModule - PendingUnpinProcessor extends WorkerHost dispatching drain-pending-unpins and drift-report jobs - drainPendingUnpins retries via ipfsProvider.unpinFile; not-pinned is success via provider (local.provider.ts:94); failure leaves row for next run - pendingUnpinsGauge.set published after each drain pass with outbox count - runDriftReport fetches Kubo pin/ls as NDJSON line-by-line, diffs against pinned_cids union pending_unpins, increments driftOrphanedPinsTotal for orphans, never deletes (D-06 read-only constraint) - PendingUnpinModule registers pending-unpins queue with two schedulers: drain every 5 min, drift report every hour via upsertJobScheduler - IPFS_PROVIDER provided locally to avoid IpfsModule circular import - PendingUnpinModule registered in app.module.ts imports * docs(42-05): complete controller wiring and api:generate plan summary * feat: add backfill-pinned-cids standalone script One-shot quota-repair script for non-BYO users. Bootstraps DataSource like run-migrations.ts. Mandatory empty/unreachable-Kubo guard aborts with exit(1) before any DELETE. BYO users excluded at both query layer (is_byo_user = false JOIN on vaults) and predicate layer. --dry-run flag reports candidates without mutating. Batched deletes (BATCH_SIZE = 10) match migration.processor pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(42-06): complete pending-unpins drain worker and drift report plan Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: complete 42-07 backfill script plan SUMMARY.md for one-shot quota-repair backfill (non-BYO only). BYO exclusion at two layers, mandatory empty-Kubo guard, --dry-run, TDD RED/GREEN gate compliance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(phase-42): update tracking after wave 3 Entire-Checkpoint: 2b9c4b2bf053 * docs(42): add code review report Entire-Checkpoint: 24805aeb2ec0 * docs(phase-42): complete phase execution Entire-Checkpoint: 721dd07aa122 * docs(phase-42): close 2 todos resolved by this phase Entire-Checkpoint: ba2b3c7b4a0c * docs(phase-42): evolve PROJECT.md after phase completion Entire-Checkpoint: 2e0567aec3f3 * docs(43): capture phase context Entire-Checkpoint: 4fab50c0a02c * docs(43): research fuse-write-durability phase Entire-Checkpoint: b3c9f9e38dfa * docs(phase-43): add validation strategy Entire-Checkpoint: 38fb3e8479a1 * docs(43): create phase plan Entire-Checkpoint: f322bc921693 * test(43-01): add failing tests for durable write journal RED gate: tests reference JournalEntry/JournalOp/JournalEntryStatus/WriteQueue types that do not yet exist — compilation fails as expected. Covers tasks 1-3: - Serialization round-trips for UploadFile and MkdirPublish entries - D-05 journal_no_plaintext invariant test - JournalEntryStatus::Failed round-trip - put/load_all_for_vault/remove/update_status disk persistence tests - D-07 vault scoping, D-09 park_on_max_retries, malformed JSON skip - D-08 replay_order_mkdir_before_upload ordering * feat(43-01): implement durable write journal with fsync barrier GREEN gate: all 14 queue tests and 6 state tests pass. - Replace memory-only VecDeque WriteQueue with path-backed journal - JournalOp enum: UploadFile + MkdirPublish (D-03), no ino fields (D-02) - JournalEntryStatus: Pending/InProgress/Failed with last_error (D-09) - JournalEntry: id, vault_root_ipns, op, retries, status (D-07) - put() writes JSON + sync_all() fsync barrier + 0o600 perms (D-04, T-43-02) - load_all_for_vault() filters by vault, skips malformed JSON (D-07, V5) - record_failure() parks entries on Failed instead of dropping (D-09) - ordered_for_replay() returns MkdirPublish before UploadFile (D-08) - SyncStatus::WriteParked { pending, failed } added to state.rs (D-10) - Zero new crate dependencies * docs(43-01): complete durable write journal plan summary RED/GREEN TDD gate confirmed. 14 queue + 6 state tests pass. All D-01 through D-10 design decisions satisfied. * docs(phase-43): update tracking after wave 1 Entire-Checkpoint: 100d8a96be2d * feat: add FsEvent enum, journal field on CipherBoxFS, MkdirConflict drain arm - Define FsEvent enum with UploadComplete and MkdirConflict variants near UploadComplete - Add pub journal: cipherbox_sdk::WriteQueue field to CipherBoxFS struct - Change upload_rx/upload_tx channel type from UploadComplete to FsEvent - Update drain_upload_completions to match on FsEvent; add MkdirConflict arm that inserts parent_ino into mutated_folders and calls queue_publish for D-11a - Wrap all upload_tx.send sites in FsEvent::UploadComplete - Add cipherbox-sdk workspace dep to cipherbox-fuse Cargo.toml - Inject journal field into CipherBoxFS construction in fuse/mod.rs and windows/mod.rs * feat: reorder handle_release for journal-fsync-before-ack - Build JournalEntry with JournalOp::UploadFile using stable IPNS names for D-02 compliance - Call fs.journal.put before acking OS establishing the fsync durability barrier (D-04) - Move handle.cleanup() before reply.ok() so plaintext temp file is deleted immediately after journal fsync (D-05) - Spawn background upload thread AFTER reply.ok(); thread calls journal.remove on success - Failed uploads do not remove journal entry so replay can recover (D-09) - Add Clone derive to WriteQueue to allow cloning into background thread closure * feat: journal MkdirPublish before reply.entry and signal retry on conflict - Build JournalEntry with JournalOp::MkdirPublish using stable IPNS names and hex-wrapped child keys (D-02, D-05) - Call fs.journal.put before reply.entry establishing the fsync durability barrier (D-04, T-43-07) - Replace false "Debounced publish will retry" warning with actual FsEvent::MkdirConflict signal so FS thread re-arms the debounced publisher (D-11a) - Keep journal entry alive until parent publish confirms; remove only on success branch (D-11b) - Remove TODO comment and stale "will retry" comment from parent-publish conflict arm * docs: complete 43-02 fuse write durability callback wiring plan * docs(phase-43): update tracking after wave 2 Entire-Checkpoint: 86e89302da42 * feat: journal handle_cleanup before spawn in Windows WinFsp path - Build JournalEntry with JournalOp::UploadFile using stable IPNS names for D-02 compliance - Call fs.journal.put before spawning upload thread establishing the fsync durability barrier (D-04) - Move handle.cleanup() to before the spawn so plaintext temp file is deleted immediately after journal fsync (D-05) - Introduce UploadSpawnParams struct to cleanly separate prepare+journal phase from spawn phase - Spawned thread calls journal.remove on confirmed upload success; leaves entry on failure for replay (D-09) * feat: journal MkdirPublish before directory reply and signal conflict retry in Windows handle_create - Build JournalEntry with JournalOp::MkdirPublish using hex-wrapped child keys and parent_folder_ipns_name - Call fs.journal.put before the success reply so the fsync barrier precedes the OS ack (D-04) - Clone encrypted_folder_key_hex_for_journal before move into InodeKind::Folder to avoid use-after-move - Replace v2 TODO and false debounced-retry comment with upload_tx.send(FsEvent::MkdirConflict) (D-11a) - Remove journal entry only on confirmed parent publish success; keep entry on conflict for replay (D-11b) * docs: complete 43-03 WinFsp write durability Windows callback wiring plan - SUMMARY documents handle_cleanup journal-fsync-before-spawn reordering (D-04, D-05) - SUMMARY documents handle_create mkdir journal-fsync-before-reply with conflict retry (D-11a/b) - All data-loss bugs now fixed on all three platforms completing D-12 * feat: add replay_for_vault and cb-journal injection on mount - Extract PublishCoordinator construction before CipherBoxFS literal so it can be passed to replay before mount - Add replay_for_vault free fn in crates/fuse/src/lib.rs: loads vault-scoped entries via load_all_for_vault (D-07), orders MkdirPublish before UploadFile via ordered_for_replay (D-08), and fetch-merges-CAS-publishes each entry without re-publishing the stale journaled snapshot (D-06, Pitfall 4) - replay_for_vault skips already-Failed entries and logs-not-fails on error so a partial replay never prevents mount - MkdirPublish replay checks idempotency (Pitfall 5): skips merge if child IPNS already present in fetched parent metadata - UploadFile replay re-uploads ciphertext idempotently, re-publishes file IPNS, then fetch-merges file pointer into current parent metadata * feat: bridge SyncStatus WriteParked to tray notification - Add TrayStatus::WriteParked variant with label Upload Failed and is_connected returning false - Add send_write_parked_notification using tauri_plugin_notification with ZK-safe neutral copy, title CipherBox Upload Failed - Extend SyncStatus match in sync/mod.rs with two D-10 arms: failed > 0 fires park notification and returns TrayStatus::WriteParked; failed == 0 is silent and returns TrayStatus::Syncing so transient retries do not spam the user - No new dependencies added to Cargo.toml * docs: complete 43-04 desktop durability loop plan * docs(phase-43): update tracking after wave 3 Entire-Checkpoint: a24a12ec1d78 * docs(43): add code review report Entire-Checkpoint: 4f3ecdf95e38 * docs(phase-43): record verification with gaps Entire-Checkpoint: 78a027254b82 * docs(43): create gap closure plans Entire-Checkpoint: 75b2f9f716ec * test(43-05): failing tests for parent_ipns_key_hex round-trip and created_at_ms ordering - upload_entry_parent_ipns_key_hex_round_trips: verifies field survives serde (CR-01) - mkdir_entry_parent_ipns_key_hex_round_trips: same for MkdirPublish (CR-01) - replay_order_sorts_by_created_at_within_group: deterministic ordering within groups (WR-01) - journal_no_plaintext_with_parent_ipns_key: no raw key bytes in JSON (D-05 extended) Tests fail to compile (RED gate) * feat(43-05): journal schema parent_ipns_key_hex, ordered replay, atomic perms - Add parent_ipns_key_hex to UploadFile and MkdirPublish (CR-01 fix: user-ECIES-wrapped) - ordered_for_replay sorts each group by created_at_ms ascending (WR-01) - put() sets 0o600 atomically via OpenOptionsExt::mode at create time (WR-03a) - put() and remove() fsync parent journal dir after write (WR-03b) - Remove impl Default for WriteQueue (WR-09) - Remove write_queue field from SyncDaemon (FUSE layer owns drain per 43-01) - 18 queue tests pass including new round-trip and ordering tests * fix(43-05): replay signs and publishes parent IPNS record using journaled key CR-01: fetch_merge_publish_parent now accepts unwrapped parent IPNS key, signs and publishes IPNS record with CAS. Returns Err on Conflict retaining the entry. Old CID unpinned only after confirmed Success. Phantom record_publish removed (IN-06). CR-02: replay_upload_entry ecies-unwraps the file IPNS key before [u8;32] cast. Journaled hex stored as-is in FilePointer without re-wrap. CR-03: replay_mkdir_entry writes user-wrapped child IPNS key as-is (no re-wrap). Write side (write_ops.rs, windows/write_ops.rs) now journals user-wrapped key. read_ops.rs and write_ops.rs: journal parent_ipns_key_hex (user-ECIES-wrapped). cargo check -p cipherbox-fuse passes with zero errors. * fix(43-05): resolve_folder_key BFS descends nested folder tree WR-02: replace single-level root scan with bounded BFS descent (depth cap 32). Resolves parent folders nested two or more levels below root. Enqueues each subfolder child with its unwrapped folder key for further iteration. Returns folder key for the matched target; parent IPNS key for signing still comes from journaled parent_ipns_key_hex (no desktop signature change needed). cargo check -p cipherbox-fuse passes. * fix(43-05): prevent test temp dir collision in parallel test runs Use atomic counter plus thread ID numeric part for unique temp dir names instead of timestamp subsec_nanos plus thread ID string length. Eliminates flaky journal_remove failure when multiple queue tests collide on the same path. * docs(43-05): execution summary for replay correctness gap closure Covers CR-01/02/03 fixes, WR-01/02/03/09, TDD gate compliance, deviations, and threat surface scan. * docs(phase-43): update tracking after wave 4 Entire-Checkpoint: f10c0095290e * fix: correct UploadSpawnParams types for winfsp feature compilation - CR-05: api field changed from Arc<ApiConfig> to Arc<ApiClient> - CR-05: rt field changed from Arc<Runtime> to tokio::runtime::Handle - CR-05: coordinator from crate::publish_coordinator::PublishCoordinator to crate::PublishCoordinator - CR-07: journal_entry replaces journal_entry_id so record_failure can be called in error path - CR-08 mirror: journal removal gated on per-file IPNS publish success - CR-04 mirror: document WinFsp void-return constraint; no success state committed on prepare failure * fix: close CR-04/08/07 in handle_release write path - CR-04: Err arm of prepare_result now calls reply.error(libc::EIO) and returns; no longer falls through to the trailing reply.ok() - CR-08: removed premature journal.remove from background upload thread; replay is now the authoritative cleanup path so the entry survives until the parent folder pointer is confirmed published - CR-07: background upload failure now calls journal.record_failure via a JournalEntry snapshot carried into the spawn closure; retries increment and the entry parks as Failed after max_retries * docs: complete 43-06 plan summary for fuse write-side gap closure * feat: add replay_for_vault call to Windows mount for crash recovery - CR-06: collect initial IPNS sequence numbers during root and subfolder pre-populate - CR-06: construct PublishCoordinator seeded from initial sequences before replay - CR-06: call cipherbox_fuse::replay_for_vault after pre-populate and before CipherBoxFS construction - CR-06: pass seeded coordinator to CipherBoxFS so live session and replay share it - Mirrors apps/desktop/src-tauri/src/fuse/mod.rs:219-242 * docs: add 43-07 summary for Windows durability gap closure * docs(phase-43): update tracking after wave 5 Entire-Checkpoint: 589b9a6ad3dc * feat: inject real WriteQueue into SyncDaemon and emit WriteParked from journal - SyncDaemon::new gains a write_queue parameter; no WriteQueue::default - sync_cycle counts Failed/Pending entries via load_all_for_vault and emits WriteParked when failed > 0, Idle when failed == 0 (CR-07) - CipherBoxSdkClient::start_sync updated to accept and forward WriteQueue * feat: wire cb-journal WriteQueue into desktop sync daemon - create_sync_daemon gains a write_queue parameter forwarded to SyncDaemon - start_sync_daemon constructs WriteQueue from data_local_dir/cipherbox/cb-journal matching the path the FUSE mount uses so daemon and FUSE share one journal (CR-07) * docs: complete 43-08 plan for CR-07 WriteParked pipeline closure * docs(phase-43): update tracking after wave 6 Entire-Checkpoint: 433b5c61d889 * fix: align Windows journal removal with replay-only mechanism Entire-Checkpoint: a0f77592b6f0 * test(43): persist human verification items as UAT Entire-Checkpoint: 8a1b3c742c8c * docs(44): capture phase context Entire-Checkpoint: 38d75e0b4acb * docs(phase-44): research IPNS conflict handling Entire-Checkpoint: 0d59d04c7778 * docs(phase-44): add validation strategy Entire-Checkpoint: b1b0ee68f651 * docs(44): create phase plan Entire-Checkpoint: 7d0efec64cbc * docs(44): add pattern map Entire-Checkpoint: f9c8bcbdc8e5 * test(44-01): add failing tests for ConflictError and mergeChildren - ConflictError: field carriage, instanceof Error, message content (no plaintext) - isConflictExhausted: true/false/null/object type-guard branches - mergeChildren: 10 permutation matrix tests covering D-01 and D-02 * feat(44-01): implement ConflictError, isConflictExhausted, and mergeChildren - ConflictError extends Error carrying ipnsName, attempts, lastRemoteSeq - isConflictExhausted type-guard (instanceof ConflictError) - mergeChildren three-way merge keyed by child.id with D-01/D-02 semantics - Both symbols re-exported from sdk-core barrel * docs(44-01): complete plan summary for ConflictError and mergeChildren * docs(phase-44): update tracking after wave 1 Entire-Checkpoint: b31885c946c8 * feat(44-02): merge-and-republish 4-attempt retry loop in updateFolderMetadataAndPublish - Add optional baseChildren param for three-way merge on 409 (backward compat) - Move encrypt+upload inside the loop for a fresh CID each attempt - Re-resolve seq authoritatively, re-fetch remote metadata, call mergeChildren - Log console.warn when baseChildren omitted (union fallback) - Throw ConflictError after 4 failed attempts - Add BACKOFF_BASE_MS, BACKOFF_CAP_MS constants and retryDelayMs helper * test(44-03): add failing tests for mergeVersions and updateFileMetadata CAS conflict path * test(44-02): add conflict handling tests for updateFolderMetadataAndPublish - merge-on-409: asserts remote-only child survives in merged metadata - union-fallback warning: asserts console.warn fires when baseChildren omitted - ConflictError after 4 attempts: asserts isConflictExhausted + attempts===4 - non-409 propagation: asserts original error not wrapped as ConflictError * docs(44-02): complete plan summary for merge-and-republish retry loop * feat(44-03): implement mergeVersions helper and updateFileMetadata CAS with conflict merge - Add exported mergeVersions helper implementing RESEARCH Pattern 5 - Add maxVersionsPerFile optional param to updateFileMetadata defaulting to MAX_VERSIONS_PER_FILE - updateFileMetadata now publishes internally via createAndPublishIpnsRecord with CAS - On 409: re-resolve, re-fetch remote FileMetadata, latest-wins by modifiedAt - Loser content preserved as VersionEntry in merged versions - versions union-merged, deduped by cid, sorted desc, capped by maxVersionsPerFile - ConflictError thrown after second consecutive 409 - Non-409 errors propagate unchanged - Zeroize fileIpnsPrivateKey in finally block - New return shape: ipnsName, metadataCid, newSequenceNumber, prunedCids * docs(44-03): complete plan summary for file CAS publish and conflict merge * docs(phase-44): update tracking after wave 2 Entire-Checkpoint: abeb98fb76b0 * feat(44-04): add baseChildren snapshots to all updateFolderMetadataAndPublish callers - client.ts: 8 call sites now pass pre-mutation baseChildren snapshot (createFolder parent, createFolder subfolder-init, renameItem, moveItem dest, moveItem source, deleteItem, uploadFile, uploadFiles) - bin/index.ts: 2 call sites pass pre-mutation baseChildren snapshot (addToBin before remove, restoreFromBin before add) - Enables three-way merge instead of union fallback at all SDK call sites - ConflictError propagates unwrapped to callers * feat(44-04): wire shared-write.ts to three-way merge and Plan-03 file return shape - 4 updateFolderMetadataAndPublish calls now pass baseChildren: swCtx.children (uploadToSharedFolder, createSharedSubfolder, renameInSharedFolder, deleteFromSharedFolder) — moves write-share paths off union fallback to three-way merge (D-08 headline fix) - updateFileMetadata call rewired to new Plan-03 return shape; redundant batchPublishIpnsRecords publish removed (file record now published internally via CAS inside updateFileMetadata) - Pre-existing Phase-42 prunedCids drop not regressed (unchanged behavior) - ConflictError propagates to web hook callers via withConflictRetry * feat(44-05): rewire useFileOperations to new updateFileMetadata CAS contract - Switch updateFileMetadata import from web service to @cipherbox/sdk-core (publishes internally) - Destructure new return shape { ipnsName, metadataCid, newSequenceNumber, prunedCids } - Remove redundant replaceFileInFolder call (file IPNS published inside updateFileMetadata) - Pass maxVersionsPerFile from vault settings store to honor user-configured cap (T-44-19) - Pass baseChildren: parentFolder.children to fire-and-forget folder re-publish (T-44-17) - Import isConflictExhausted and log conflict-specific warning in folder re-publish catch (T-44-18) * feat(44-05): add baseChildren to useFileVersions lazy-migration folder re-publishes - Pass baseChildren: parentFolder.children to both updateFolderMetadataAndPublish calls (restoreVersion and deleteVersion lazy-migration paths) to enable three-way merge on 409 - Import isConflictExhausted from @cipherbox/sdk-core for conflict-specific logging in catch - Both calls remain fire-and-forget; D-09 confirmed: no Rust/crates changes * docs(44-04): complete SDK caller baseChildren sweep plan summary * docs(44-05): complete web hook caller adoption for IPNS conflict handling * fix(sdk): update BinEntry test fixture to use itemType instead of type field * docs(phase-44): update tracking after wave 3 Entire-Checkpoint: 140f7eba1ec7 * docs(44): add code review report Entire-Checkpoint: 3d5b6d9f2696 * docs(phase-44): record verification with gaps Entire-Checkpoint: 7f892bbcc0a6 * docs(44): create gap closure plans Entire-Checkpoint: 970eac27480d * feat(44-06): return publishedChildren from updateFolderMetadataAndPublish - Widen return type to include publishedChildren: FolderChild[] - Return currentLocalChildren as publishedChildren at the success path - Add WR-08 folder conflict test asserting merged published set is surfaced * feat(44-06): adopt publishedChildren in sdk callers and shared-write - client.ts: 8 sites adopt publishedChildren for state + folder:updated event - bin/index.ts: addToBin and restoreFromBin adopt publishedChildren - shared-write.ts: 4 folder functions return publishedChildren in result shape - Update sdk test mocks to include publishedChildren in return value * feat(44-06): adopt publishedChildren in web hooks for folder convergence - useSharedWriteOps: 4 handlers adopt result.publishedChildren into folderChildrenRef and setFolderChildren instead of updatedChildren - useFileOperations: file-edit fire-and-forget adopts publishedChildren via updateFolderChildren on success - useFileVersions: both lazy-migration fire-and-forget paths adopt publishedChildren via updateFolderChildren alongside updateFolderSequence * docs(44-06): add plan summary for CR-01 publishedChildren adoption * test(44-07): add failing WR-08 assertion for CR-02 prunedCids reference filter - WR-08 conflict test: verifies prunedCids does not overlap with published mergedMetadata references (CR-02 safety invariant) - Scenario: unsorted existing versions cause v-NEW (high timestamp) to be pruned by positional slice then resurrected by remote merge - Asserts loser cid present in retry payload versions[] - Asserts prunedCids has empty intersection with publishedRefs - Asserts genuinely overflowed non-referenced cid still pruned * fix(44-07): filter prunedCids against mergedMetadata references in file 409 path (CR-02) - Build Set of referenced CIDs from mergedMetadata.cid and mergedMetadata.versions[].cid after the conflict merge - De-dupe the accumulated prunedCids before filtering to prevent duplicate unpin entries from both initial prune and extraPruned - Filter: keep only CIDs not referenced by the published merged record - Prevents destructive unpin of version content still live in IPNS record - Addresses CR-02 / D-07 from 44-VERIFICATION.md * docs(44-07): complete CR-02 filter fix and WR-08 file test plan summary * docs(phase-44): update tracking after gap wave Entire-Checkpoint: 1f315a408d07 * docs(phase-44): record passing verification after gap closure Entire-Checkpoint: b44faa83e9b1 * docs(phase-44): complete phase execution Entire-Checkpoint: e945c05f92db * docs(phase-44): close todo resolved by this phase Entire-Checkpoint: db20a2e88b94 * docs(phase-44): evolve PROJECT.md after milestone completion Entire-Checkpoint: eb95fe8f833d * docs(43): add pattern map and reset auto-chain flag Entire-Checkpoint: d23682f96008 * fix: restore ROADMAP truncated during phase-44 gap planning Entire-Checkpoint: 20cce1cdb2c4 * refactor(sdk-core): dedupe phase-44 conflict helpers, parallelize file resolve+upload /simplify pass on phase-44 net changes: - extract is409() into errors.ts; replace 3 inlined 409 predicates - extract fetchAndDecryptFileMetadata() (file read-half companion to encryptAndUpload) - Promise.all the independent resolve + encrypt-upload in updateFileMetadata sdk-core tests: 190/190. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 7f2e3b3cc480 * docs: capture 3 deferred phase-44 findings as todos Entire-Checkpoint: dd51f176ebc0 * docs(phase-44): add security threat verification Entire-Checkpoint: 48df27663073 * fix(sdk-core): preserve local version history when remote wins file conflict The updateFileMetadata 409 conflict merge passed remoteMeta.versions as the second mergeVersions arg. When the remote write wins latest-wins, the loser is the LOCAL metadata, so this silently dropped the local writer's prior version history. Use loser.versions instead (identical in the local-wins case, correct in both directions). Also re-export is409 from the sdk-core barrel (defined in errors.ts but not exported) and add unit tests for is409 plus a remote-wins history-preservation regression test. Found by CodeRabbit CLI review of phase 44. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d6e33b5f68ce * docs(phase-44): record post-audit CodeRabbit finding and fix in SECURITY.md A CodeRabbit CLI review run after the SECURED verdict found a critical data-loss bug (remoteMeta.versions vs loser.versions in the file 409 merge) that T-44-09/T-44-21 over-trusted. Documents the gap and its fix (9f3c561); threats_open stays 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f3639d42e06b * docs(44): ship phase 44 — PR #488 Entire-Checkpoint: 12fd277b31ab * test(sdk): drop stale batchPublishIpnsRecords assertion in updateSharedFile Plan-03 moved file-record IPNS publishing inside updateFileMetadata (CAS), so updateSharedFile no longer calls batchPublishIpnsRecords directly. The assertion was stale and only surfaced in CI because phase-44 local verification ran sdk-core but not the full packages/sdk suite. The updateFileMetadata assertion already covers the publish path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 887be6f7ff59 * chore(release): set release targets for PR #488 * test: reinit zeroized key per test and remove stale delete.service.spec duplicate Addresses CodeRabbit review on PR #488: - file.test.ts: updateFileMetadata zeroizes fileIpnsPrivateKey in-place, so the shared mockPrivateKey was all-zero after the first test. Reinitialize it per test in beforeEach so each case exercises a real non-zero key. - delete.service.spec.ts: stale branch-phase-42 duplicate of the canonical delete.service.test.ts (from #485). It never ran (web vitest includes only *.test.ts) and its fetchQuota-rejection assertion did not match the resolved-falsy implementation contract. Removed; coverage fully retained by delete.service.test.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 52515563f6ea --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the two unpin-path gaps from the v1.1 UAT audit (gap-closure Phase 42):
pin/rmfor any CID the caller named, with no verification that the caller owned it. A share recipient (or any authenticated user who learned a CID) could unpin another user's content.pinned_cidsrow was never removed and usage never went down.Both fixes land together so authorization, row deletion, and the quota update stay consistent.
What changed
VaultService.guardedUnpin— per-CIDpg_advisory_xact_lock(hashtext(cid))serializes concurrent unpins; in one transaction it verifies the caller owns apinned_cids(userId, cid)row, deletes it (the delete is the quota decrement), reference-counts the CID across all users, and inserts into apending_unpinsoutbox only when the global refcount hits zero. Kubopin/rmis issued post-commit, best-effort.{ success: true }and increments the audit metriccipherbox_unpin_cross_user_attempts_total(Grafana alert included). No 403 that would confirm a CID exists.pending_unpinsoutbox + drain worker — a BullMQ processor drains the outbox (best-effort Kubo unpin) and a read-only drift report surfaces orphaned pins without mutating anything.pinned_cidsrows that predate this change.delete.servicereconciles quota viafetchQuotaafter the localremoveUsage.AddPendingUnpins,AddPinnedCidCidIndex), 3 new metrics, and the regenerated@cipherbox/api-clientfor the opaque unpin response DTO.Verification
gsd-verifier: passed, 8/8 observable truths verified against the code.cipherbox-postgrescontainer:pending_unpinstable +idx_pending_unpins_cid(unique) +idx_pinned_cids_cidall present with the expected column shape.vault.service,ipfs.controller,pending-unpin,backfill-helpers(TDD RED→GREEN for the guardedUnpin / processor / backfill plans)..planning/phases/42-api-unpin-integrity/42-REVIEW.md. None goal-defeating; worth a skim before merge (notably WR-01abs(hashtext)cast ordering and WR-02 upload-compensation no-op).Notes
packages/api-client/are committed alongside the API changes (pre-commitcheck-api-client.shparity).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Data Corrections
Documentation