feat(sync): rfc-003 phase a.2.3 — digest endpoint + payload_hash + metadata_digest_version bump - #56
Merged
Conversation
…tadata_digest_version bump
Phase A.2.3 completes the server-side Phase A by wiring the
§metadata_digest_version invariant + the GET /api/v1/sync/digest
endpoint that consumes it.
## What changes
### `db::digest` (new submodule)
Four monotone-counter helpers:
- `bump_profile(conn, profile_id, entity)` — for profile-scoped
entities. `INSERT ... ON CONFLICT DO UPDATE SET version = + 1`
so two concurrent writes on the same key serialise atomically
through the unique-row lock.
- `bump_user(conn, user_id, entity)` — same shape but targets
`user_metadata_digest_version` (A.2.2.0).
- `read_profile` / `read_user` — `None` on no row = `version 0`.
### Apply pipeline — every write computes + binds `payload_hash`
`apply::canon` module collects the helpers (`opt_string` /
`string` / `opt_i64` / `i64` / `bool` / `strings`) every handler
uses to build the canonical-fields `Map<String, Value>`. The hash
is then BLAKE3 over the canonical wire form via the A.2.2.1
`compute_payload_hash` helper, bound at the same INSERT/UPDATE
site as the HLC tuple.
- `library` / `playlist` — INSERT binds hash; SET_FIELD is now a
2-round-trip (SELECT current state → recompute hash over the
full row state → UPDATE everything). 2 round-trips preserve the
§metadata_digest_version invariant ("bump iff payload_hash
actually changes"). Single-field updates over a NULL row
(parent not materialised yet) return Skipped — same handling
as the playlist track-list ops use.
- `track` — INSERT binds hash over the full audio-metadata
payload (title, file_hash, duration_ms, audio specs, album,
artists). `TrackInput` gains a `payload_hash: &[u8]` field.
- `liked` / `rating` — UPSERTs always overwrite hash + HLC; the
refresh-on-conflict semantics from A.2.2 preserve.
- `profile_resolve` — auto-provision binds hash over `{name,
color_id}` and bumps the `profile` digest.
Every successful write calls the matching `bump_*` helper in the
same transaction. Delete handlers also bump (the row leaving the
set changes the set_hash).
### `GET /api/v1/sync/digest`
Returns the RFC-003 §4 snapshot:
```json
{
"set_hash": "<blake3-hex>",
"version": <i64>,
"max_hlc": { "wall": N, "logical": M, "origin_device_id": "uuid-or-null" },
"members": [{ "canonical_id": "...", "payload_hash": "<hex>" }, ...]
}
```
- `entity` is required. Profile-scoped entities require
`profile_canonical_id`; user-scoped ones reject it (400 on a
mismatched pair).
- Profile resolution returns 404 on a canonical id not visible to
this user.
- Members are sorted by canonical_id before being fed to
`set_hash`. Track members use `<library_canonical_id>\u{1f}<file_path>`
as the composite key since tracks have no canonical_id of their
own.
- `set_hash` is BLAKE3-256 over (canonical_id_len_le_u32,
canonical_id_bytes, payload_hash_bytes) per member.
### Type relocation
`DigestMember` / `DigestResponse` / `MaxHlc` move from
`api/sync.rs` to `src/sync.rs` so `db::digest_read` can build them
without depending on a private `api::sync` submodule.
### New deps
`hex` — for the `payload_hash` byte ↔ hex round-trip in the
digest response (the database stores BYTEA, the wire emits hex).
## Tests
5 new cases in `tests/apply_digest.rs`:
- `library_insert_binds_payload_hash_and_bumps_digest` — 32-byte
BLAKE3 hash on the row, both `library` and `profile` digest
counters bump to 1.
- `library_set_field_recomputes_hash_and_bumps_digest` — SET_FIELD
changes the hash and the counter advances.
- `digest_endpoint_returns_stable_set_hash_for_library` — two
inserts, members sorted, second call returns identical
set_hash + version.
- `digest_endpoint_rejects_profile_id_on_user_scoped_entity` —
400 when `profile_canonical_id` is sent alongside `liked_track`.
- `digest_endpoint_user_scoped_liked_round_trip` — user-scoped
path round-trips through the user_metadata_digest_version table.
Full suite green (250 tests).
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplémentation de la phase A.2.3 du RFC-003 : calcul et persistance d'un ChangesRFC-003 Phase A.2.3 — payload_hash et endpoint digest
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as get_digest handler
participant Reader as db::digest_read
participant PG as PostgreSQL
Client->>Handler: GET /api/v1/sync/digest?entity=library&profile_canonical_id=X
Handler->>Handler: Valider entity + présence/absence profile_canonical_id
Handler->>Reader: resolve_profile_id(pool, user_id, "X")
Reader->>PG: SELECT id FROM profile WHERE canonical_id="X"
PG-->>Reader: profile_id ou null
alt profile_id trouvé
Handler->>Reader: build_profile_digest(pool, profile_id, "library")
Reader->>PG: BEGIN REPEATABLE READ
Reader->>PG: SELECT version FROM metadata_digest_version WHERE profile_id=? AND entity="library"
PG-->>Reader: i64 version
Reader->>PG: SELECT canonical_id, payload_hash, hlc_wall, hlc_logical, origin_device_id WHERE payload_hash IS NOT NULL
PG-->>Reader: ensemble membres filtrés
Reader->>Reader: Trier par canonical_id + hacher déterministe
Reader->>Reader: Calculer max(hlc_wall, hlc_logical, origin_device_id)
Reader->>PG: COMMIT
Reader-->>Handler: DigestResponse {set_hash, version, max_hlc, members}
Handler-->>Client: 200 JSON DigestResponse
else profile_id non trouvé
Handler-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
scope: server
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-actions
Bot
added
type: feat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/apply.rs`:
- Around line 306-326: The digest is being bumped unconditionally via
bump_profile without confirming that the database operation actually produced an
effective mutation. When an INSERT...ON CONFLICT...DO UPDATE statement
encounters a conflict and the UPDATE branch either affects zero rows or produces
no material change, or when other mutation attempts fail to modify data, bumping
the profile digest will cause version drift that breaks the version↔state
invariant on the sync side. Modify the code to check whether the operation
actually resulted in a change before calling bump_profile. For the INSERT...ON
CONFLICT at lines 306-326 in src/apply.rs (file anchor), add logic to determine
if the operation was an INSERT or an UPDATE and whether the UPDATE actually
modified any rows before calling bump_profile. Apply the same fix pattern at the
sibling sites in src/apply.rs at lines 836-857 and 1045-1066, ensuring that
bump_profile is only called when a material mutation has been confirmed to
occur.
In `@src/db.rs`:
- Around line 1162-1179: The snapshot digest reads are not atomic: the version
is read in one query, then members are read separately in another query based on
the entity type (in the match statement with profile_self_members,
members_library, members_playlist, track_members branches). A concurrent write
between these reads can cause the returned version to mismatch the returned
members and set_hash. Wrap all the reads (both the read_profile call and the
subsequent member-fetching calls) within a single database transaction to ensure
atomicity of the snapshot digest. This pattern needs to be applied at both
affected locations (around lines 1162-1179 and 1189-1204).
In `@tests/apply_digest.rs`:
- Around line 181-185: The assertion at line 181-185 uses a permissive
greater-than-or-equal comparison (>= 2) when checking the library_version
counter after set_field, which masks possible regressions where the digest
counter might be incremented more times than expected. Change the comparison
operator from >= to == to enforce that the version is exactly 2, making the test
more protective and detecting any spurious or unexpected bumps to the digest
counter.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 2b14f3ce-0ab7-4bbe-af9f-365f56fa92b3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (6)
Cargo.tomlsrc/api/sync.rssrc/apply.rssrc/db.rssrc/sync.rstests/apply_digest.rs
…reads Three review fixes on #56: 1. profile_resolve::find_or_provision — race-window UPSERT (concurrent first-op for the same canonical_id) hits DO UPDATE with a self- assignment of canonical_id, no material change. The previous unconditional bump_profile call overcounted the digest version in that race. Now uses RETURNING (xmax = 0) AS inserted to discriminate fresh INSERT vs race-window UPDATE; only bumps on the INSERT path. 2. library::set_field + playlist::set_field — the SELECT-first step now fetches the existing payload_hash alongside the canonical fields. When the recomputed hash equals the existing one (an idempotent re-emit), the handler skips the UPDATE + bump entirely. Preserves the §metadata_digest_version invariant "bump iff payload_hash actually changes" against idempotent re-emit traffic. 3. db::digest_read::build_{profile,user}_digest — the version read + members read now run inside a SET TRANSACTION ISOLATION LEVEL REPEATABLE READ tx so a concurrent apply tx committing the row change + the digest bump between them can't return a mismatched (version, members) pair. Without the isolation upgrade, default READ COMMITTED could land us with a stale version next to fresh members and the client would cache the new state under the old version, missing the next refresh. 4. tests/apply_digest.rs — assert_eq!(version, 2) instead of >= 2 on the library digest after insert + set_field. The permissive >= masked a possible regression where the library handler accidentally bumped the profile counter or set_field bumped twice. The exact count is the meaningful contract. Also factored the 5-tuple Option<(String, Option<String>, String, String, Option<Vec<u8>>)> behind a `type CurrentRow = ...` alias to satisfy clippy::type_complexity. Full apply + apply_digest suites green. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/apply.rs (1)
862-882:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNe bump pas le digest si l’
UPDATEn’a touché aucune ligne.Après le pré-read, un
DELETEconcurrent peut faire tomber l’UPDATEà0ligne. Dans ce cas,bump_profiledérive encoremetadata_digest_versionsans mutation réelle, ce qui recasse l’invariantversion ↔ étatdansplaylist::set_fieldetlibrary::set_field.Correctif minimal proposé
- sqlx::query( + let res = sqlx::query( "UPDATE playlist SET name = $1, description = $2, color_id = $3, icon_id = $4, \ updated_at = $5, hlc_wall = $6, hlc_logical = $7, \ origin_device_id = $8, payload_hash = $9 \ WHERE profile_id = $10 AND canonical_id = $11", ) @@ - .execute(&mut *conn) - .await?; - - db::digest::bump_profile(conn, profile_id, ENTITY).await?; + .execute(&mut *conn) + .await?; + if res.rows_affected() == 0 { + return Ok(ApplyOutcome::Skipped); + } + db::digest::bump_profile(conn, profile_id, ENTITY).await?; Ok(ApplyOutcome::Applied)- sqlx::query( + let res = sqlx::query( "UPDATE library SET name = $1, description = $2, color_id = $3, icon_id = $4, \ updated_at = $5, hlc_wall = $6, hlc_logical = $7, \ origin_device_id = $8, payload_hash = $9 \ WHERE profile_id = $10 AND canonical_id = $11", ) @@ - .execute(&mut *conn) - .await?; - - db::digest::bump_profile(conn, profile_id, ENTITY).await?; + .execute(&mut *conn) + .await?; + if res.rows_affected() == 0 { + return Ok(ApplyOutcome::Skipped); + } + db::digest::bump_profile(conn, profile_id, ENTITY).await?; Ok(ApplyOutcome::Applied)Also applies to: 1081-1101
🤖 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 `@src/apply.rs` around lines 862 - 882, The code unconditionally calls db::digest::bump_profile() after the UPDATE statement without checking if any rows were actually affected. If a concurrent DELETE happens between the pre-read and UPDATE, the UPDATE may affect 0 rows but bump_profile is still invoked, breaking the version-state invariant in playlist::set_field and library::set_field. Fix this by capturing the result of .execute() (which returns the number of affected rows), checking if the row count is greater than 0, and only then calling bump_profile. Apply this same fix to the other location mentioned in the comment at lines 1081-1101.
🤖 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.
Duplicate comments:
In `@src/apply.rs`:
- Around line 862-882: The code unconditionally calls db::digest::bump_profile()
after the UPDATE statement without checking if any rows were actually affected.
If a concurrent DELETE happens between the pre-read and UPDATE, the UPDATE may
affect 0 rows but bump_profile is still invoked, breaking the version-state
invariant in playlist::set_field and library::set_field. Fix this by capturing
the result of .execute() (which returns the number of affected rows), checking
if the row count is greater than 0, and only then calling bump_profile. Apply
this same fix to the other location mentioned in the comment at lines 1081-1101.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ffe4870-2bab-46f8-ae1c-36faa4a7834d
📒 Files selected for processing (3)
src/apply.rssrc/db.rstests/apply_digest.rs
CodeRabbit follow-up on #56: between the SELECT-first preread and the UPDATE in playlist::set_field / library::set_field, a concurrent DELETE can fire. The UPDATE then matches 0 rows but the previous code unconditionally called bump_profile, breaking the §metadata_digest_version invariant for the second time on the same PR. Now captures res.rows_affected() and returns ApplyOutcome::Skipped without bumping when the UPDATE found nothing — same shape as the idempotent-skip branch already added in this PR. Apply + apply_digest suites green. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
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)
src/apply.rs (1)
213-245:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLe dispatch laisse toujours
profiletomber dansUnknown.
GET /api/v1/sync/digestexposeentity=profile, et cette PR annonce unpayload_hash/digest pour cette entité. Or cematchne route queplaylist,library,track,liked_trackettrack_rating: une opprofilene sera jamais matérialisée ni ne bumprametadata_digest_version. Le serveur reste donc bloqué sur le profil auto-provisionné ("Synced profile","violet"), ce qui casse la convergence dès qu’un client pousse un rename ou un changement de couleur. Il faut soit ajouter un handlerprofile::applyici, soit retirerprofiledu contrat digest tant que ce flux n’existe pas.🤖 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 `@src/apply.rs` around lines 213 - 245, The entity matching in the apply function does not handle the "profile" case, causing profile operations to fall through to the Unknown default case without being materialized or bumping the digest version. Add a "profile" case to the outer match statement that extracts profile_canonical_id and calls profile_resolve::find_or_provision (similar to how "playlist", "library", and "track" are handled in the current code), then routes to a profile::apply handler in the inner match statement. If the profile::apply handler does not yet exist, either implement it following the same pattern as the other entity handlers, or alternatively remove "profile" from the digest contract if this flow is not yet ready for production.
🤖 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 `@src/apply.rs`:
- Around line 213-245: The entity matching in the apply function does not handle
the "profile" case, causing profile operations to fall through to the Unknown
default case without being materialized or bumping the digest version. Add a
"profile" case to the outer match statement that extracts profile_canonical_id
and calls profile_resolve::find_or_provision (similar to how "playlist",
"library", and "track" are handled in the current code), then routes to a
profile::apply handler in the inner match statement. If the profile::apply
handler does not yet exist, either implement it following the same pattern as
the other entity handlers, or alternatively remove "profile" from the digest
contract if this flow is not yet ready for production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53ffd3ca-ef71-4d69-993e-bfdbc282ab22
📒 Files selected for processing (1)
src/apply.rs
CodeRabbit follow-up on #56: the digest endpoint already exposes `entity=profile` but the apply dispatcher fell through to Unknown for inbound profile ops, so a desktop rename / recolour would never materialise server-side and the digests would diverge between clients on the next sync. Adds the missing handler: - Dispatcher now routes "profile" alongside playlist/library/track after profile_resolve resolves the canonical id. - Module `apply::profile` handles `set` ops on field `name` / `color_id`. Mirrors `library::set_field`: - SELECT-first pre-read for current state + payload_hash - Canonical-fields hash over (name, color_id) — matches the auto-provisioning shape so a rename can collapse to a no-op when values are equal - Race-window guard (UPDATE rows_affected == 0 → Skipped + no bump) - Idempotent-skip when hash unchanged - No `updated_at` write — the profile schema has no such column; `last_used_at` is playback-recency, not sync-write. HLC carries the §2 ordering authority. INSERT is implicit via `profile_resolve` auto-provision; DELETE has no wire shape (profiles cascade on user delete). New test `profile_set_name_updates_row_and_bumps_digest` covers the round-trip: auto-provision bumps to 1, rename via set_field bumps to exactly 2, name + payload_hash both update. Apply + apply_digest suites green (6 cases). Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
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)
src/apply.rs (1)
214-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValide entièrement les ops
profileavantfind_or_provision.Le dispatcher provisionne un profil sur la seule base de
profile_canonical_id, puis laisseprofile::applyrejeter les shapes non supportées. Du coup, unprofileinvalide (insert,delete, champ inconnu, etc.) peut créer la ligne et bump le digest avant de finir enUnknown. En plus, rien ne vérifie queentity_id == profile_canonical_id, donc le log durable peut pointer vers un profil différent de celui réellement modifié.💡 Correctif minimal
"profile" => { let Some(profile_canonical) = op.profile_canonical_id.as_deref() else { tracing::debug!( entity = entity, "apply: missing profile_canonical_id, skipping" ); return Ok(ApplyOutcome::Skipped); }; + match (op.op.as_str(), op.field.as_deref()) { + ("set", Some("name" | "color_id")) if op.entity_id == profile_canonical => {} + ("set", Some("name" | "color_id")) => { + return Err(ApplyError::InvalidPayload { + entity: "profile", + op: "set", + reason: "entity_id must match profile_canonical_id for profile ops" + .to_owned(), + }); + } + _ => return Ok(ApplyOutcome::Unknown), + } let profile_id = profile_resolve::find_or_provision( conn, user_id, profile_canonical, created_at, stamp, ) .await?; - match entity { - "playlist" => playlist::apply(conn, profile_id, op, created_at, stamp).await, - "library" => library::apply(conn, profile_id, op, created_at, stamp).await, - "track" => track::apply(conn, profile_id, op, created_at, stamp).await, - "profile" => profile::apply(conn, profile_id, op, stamp).await, - _ => unreachable!(), - } + profile::apply(conn, profile_id, op, stamp).await }🤖 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 `@src/apply.rs` around lines 214 - 234, The dispatcher is provisioning a profile by calling find_or_provision before validating if the operation is actually valid for the profile entity type. This can cause invalid profile operations (inserts, deletes, unknown fields) to create or modify database rows and bump the digest before the operation is rejected as Unknown. Additionally, there is no validation that entity_id matches profile_canonical_id, so the durable log could reference a different profile than was actually modified. For the "profile" case in the match statement, validate the operation shape and verify that entity_id equals profile_canonical_id before calling find_or_provision, then only proceed with the actual profile::apply call if validation succeeds.
🤖 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 `@src/apply.rs`:
- Around line 214-234: The dispatcher is provisioning a profile by calling
find_or_provision before validating if the operation is actually valid for the
profile entity type. This can cause invalid profile operations (inserts,
deletes, unknown fields) to create or modify database rows and bump the digest
before the operation is rejected as Unknown. Additionally, there is no
validation that entity_id matches profile_canonical_id, so the durable log could
reference a different profile than was actually modified. For the "profile" case
in the match statement, validate the operation shape and verify that entity_id
equals profile_canonical_id before calling find_or_provision, then only proceed
with the actual profile::apply call if validation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b93713ce-4b12-413c-8634-2a9646dd17af
📒 Files selected for processing (2)
src/apply.rstests/apply_digest.rs
CodeRabbit follow-up on #56: the dispatcher's combined "playlist | library | track | profile" arm called find_or_provision before letting profile::apply reject unsupported shapes, so a malformed profile op (insert/delete/unknown field) would auto-create the row + bump the digest, only to then be rejected as Unknown. Worse, nothing checked that `entity_id` matched `profile_canonical_id` — for the profile entity those two values MUST refer to the same row, and a mismatch would silently mutate a different profile than the op claimed. Profile is unique among the profile-scoped entities: entity_id IS the profile's canonical_id by design. The other three entities (playlist / library / track) carry their own entity_id distinct from the parent profile, so the confusion doesn't exist there. Split "profile" into its own dispatcher arm with shape + entity_id validation BEFORE find_or_provision: - Required op shape: ("set", Some("name" | "color_id")). - entity_id MUST equal profile_canonical_id; mismatch surfaces as ApplyError::InvalidPayload (caller rolls back the durable log). - Unsupported op shapes return Unknown without provisioning. New test `profile_mismatched_entity_id_rejects_and_skips_provisioning` covers both halves: the 500 response on mismatch AND the absence of a profile row after the failed push (zero side effects). Apply + apply_digest suites green (7 cases). Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
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 (3)
tests/apply_digest.rs (1)
287-296:⚠️ Potential issue | 🟡 MinorValidation asymétrique : le pipeline apply ignore
profile_canonical_idpour les entités user-scoped, tandis que l'endpoint digest le rejette.L'opération
liked_track(ligne 295) inclutSome(PROFILE_CID), mais le handlerliked::apply()n'utilise jamais ce champ — il le ignore silencieusement. En contraste, l'endpoint digest (src/api/sync.rs:591-594) valide et rejette explicitementprofile_canonical_idpourliked_trackavec un400 Bad Request.Cette asymétrie crée une incohérence : le chemin d'écriture accepte le paramètre pour les entités user-scoped (tolère), le chemin de lecture le refuse (rejette). Vérifier si c'est intentionnel pour la compatibilité backward-compatible, sinon aligner les deux chemins sur une même politique de validation.
🤖 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 `@tests/apply_digest.rs` around lines 287 - 296, There is a validation asymmetry between the apply and digest paths for user-scoped entities like liked_track: the op() call for liked_track at this location includes Some(PROFILE_CID), but the liked::apply() handler silently ignores this field, while the digest endpoint in src/api/sync.rs (lines 591-594) explicitly rejects profile_canonical_id with a 400 Bad Request. Determine if this inconsistency is intentional for backward compatibility or if both paths should use the same validation policy. If they should be aligned, either remove Some(PROFILE_CID) from the liked_track operation in this test case (to match the digest endpoint's rejection behavior) or ensure both paths consistently accept or ignore the field. Update the test and implementation to reflect the chosen consistent policy.src/apply.rs (2)
1552-1560:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRejette les métriques audio négatives avant l’upsert.
file_size,duration_mset les entiers audio optionnels sont seulement typés, jamais bornés. Un client bogué peut donc faire persister-1ou-42, qui seront ensuite hachés et exposés comme des métadonnées valides.Diff minimal proposé
let file_size = payload_i64_required(op, "file_size")?; let duration_ms = payload_i64_required(op, "duration_ms")?; let track_number = payload_i64_optional(op, "track_number")?; let disc_number = payload_i64_optional(op, "disc_number")?; let year = payload_i64_optional(op, "year")?; let bitrate = payload_i64_optional(op, "bitrate")?; let sample_rate = payload_i64_optional(op, "sample_rate")?; let channels = payload_i64_optional(op, "channels")?; let bit_depth = payload_i64_optional(op, "bit_depth")?; + for (key, value) in [ + ("file_size", Some(file_size)), + ("duration_ms", Some(duration_ms)), + ("track_number", track_number), + ("disc_number", disc_number), + ("year", year), + ("bitrate", bitrate), + ("sample_rate", sample_rate), + ("channels", channels), + ("bit_depth", bit_depth), + ] { + if let Some(value) = value { + if value < 0 { + return Err(ApplyError::InvalidPayload { + entity: ENTITY, + op: "insert", + reason: format!("payload.{key} must be >= 0"), + }); + } + } + }Also applies to: 1796-1833
🤖 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 `@src/apply.rs` around lines 1552 - 1560, The code extracts numeric payload values (file_size, duration_ms, track_number, disc_number, year, bitrate, sample_rate, channels, bit_depth) without validating they are non-negative. Add validation checks after each payload extraction using the payload_i64_required and payload_i64_optional functions to ensure these audio metrics are not negative values, rejecting any negative integers before they are used in the upsert operation. Apply the same validation pattern at all locations where these audio metric fields are extracted.
919-926:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValide aussi les champs optionnels des snapshots quand ils sont présents.
Ici, un
artistnon-string est silencieusement effacé et unduration_msmal typé devient0. Ça contredit le contrat documenté desnapshots_from_payloadet persiste un snapshot corrompu au lieu de rejeter le batch.Diff minimal proposé
- let artist = inner - .get("artist") - .and_then(Value::as_str) - .map(str::to_owned); - let duration_ms = inner - .get("duration_ms") - .and_then(Value::as_i64) - .unwrap_or(0); + let artist = match inner.get("artist") { + None | Some(Value::Null) => None, + Some(Value::String(s)) => Some(s.clone()), + Some(_) => { + return Err(ApplyError::InvalidPayload { + entity: ENTITY, + op: "tracks", + reason: format!( + "payload.snapshots[{key}].artist must be a string or null" + ), + }); + } + }; + let duration_ms = match inner.get("duration_ms") { + None | Some(Value::Null) => 0, + Some(Value::Number(_)) => inner["duration_ms"].as_i64().ok_or_else(|| { + ApplyError::InvalidPayload { + entity: ENTITY, + op: "tracks", + reason: format!( + "payload.snapshots[{key}].duration_ms must fit in i64" + ), + } + })?, + Some(_) => { + return Err(ApplyError::InvalidPayload { + entity: ENTITY, + op: "tracks", + reason: format!( + "payload.snapshots[{key}].duration_ms must be an integer or null" + ), + }); + } + };🤖 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 `@src/apply.rs` around lines 919 - 926, The code in the snapshot parsing logic for the artist and duration_ms fields silently ignores or defaults malformed values instead of rejecting them, which violates the documented contract of snapshots_from_payload. When extracting the artist field, instead of using and_then to silently drop non-string values, validate that if the field exists it must be a string and return an error if not. Similarly, for the duration_ms field, instead of using unwrap_or(0) to default malformed values, validate that if the field exists it must be an i64 and return an error if it has an incorrect type. This ensures the batch is rejected when optional fields are present but corrupted, rather than persisting a snapshot with invalid data.
🤖 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 `@src/apply.rs`:
- Around line 1552-1560: The code extracts numeric payload values (file_size,
duration_ms, track_number, disc_number, year, bitrate, sample_rate, channels,
bit_depth) without validating they are non-negative. Add validation checks after
each payload extraction using the payload_i64_required and payload_i64_optional
functions to ensure these audio metrics are not negative values, rejecting any
negative integers before they are used in the upsert operation. Apply the same
validation pattern at all locations where these audio metric fields are
extracted.
- Around line 919-926: The code in the snapshot parsing logic for the artist and
duration_ms fields silently ignores or defaults malformed values instead of
rejecting them, which violates the documented contract of
snapshots_from_payload. When extracting the artist field, instead of using
and_then to silently drop non-string values, validate that if the field exists
it must be a string and return an error if not. Similarly, for the duration_ms
field, instead of using unwrap_or(0) to default malformed values, validate that
if the field exists it must be an i64 and return an error if it has an incorrect
type. This ensures the batch is rejected when optional fields are present but
corrupted, rather than persisting a snapshot with invalid data.
In `@tests/apply_digest.rs`:
- Around line 287-296: There is a validation asymmetry between the apply and
digest paths for user-scoped entities like liked_track: the op() call for
liked_track at this location includes Some(PROFILE_CID), but the liked::apply()
handler silently ignores this field, while the digest endpoint in
src/api/sync.rs (lines 591-594) explicitly rejects profile_canonical_id with a
400 Bad Request. Determine if this inconsistency is intentional for backward
compatibility or if both paths should use the same validation policy. If they
should be aligned, either remove Some(PROFILE_CID) from the liked_track
operation in this test case (to match the digest endpoint's rejection behavior)
or ensure both paths consistently accept or ignore the field. Update the test
and implementation to reflect the chosen consistent policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c38bfd0-2b54-4f3d-8b2a-b4c0dac55504
📒 Files selected for processing (2)
src/apply.rstests/apply_digest.rs
CodeRabbit pre-push review on #56 — two valid findings, third skipped with rationale. ## 1. track::insert — reject negative numeric audio metrics The `payload_i64_required` / `payload_i64_optional` helpers validate the WIRE TYPE (must be i64) but not the VALUE DOMAIN. A wire-shape-valid `file_size: -1` or `duration_ms: -5000` would silently land in the upsert with no schema CHECK on those columns to catch it. Two new helpers in `apply::track`: - `require_nonneg(key, value)` for required fields (file_size, duration_ms). - `require_nonneg_opt(key, value)` for optional fields (track_number, disc_number, year, bitrate, sample_rate, channels, bit_depth). Applied immediately after the type-shape extraction at the apply boundary so a structurally-broken payload surfaces as InvalidPayload (rollback durable log) rather than persisting. ## 2. snapshots_from_payload — reject malformed optional fields The module docstring promises "we prefer to reject a corrupt batch up front rather than store a mix of populated + NULL rows that would be hard to audit later". The actual code violated that: - `artist`: `.and_then(Value::as_str).map(...)` silently dropped a non-string value into `None`. - `duration_ms`: `.and_then(Value::as_i64).unwrap_or(0)` silently defaulted a non-integer to `0`. Now both use explicit match-on-Value::variant patterns: - Absent / Null → keep the default (None / 0). - Present + correct type → use it. - Present + wrong type → return InvalidPayload. ## 3. Test asymmetry — skipped The reviewer flagged that `apply::liked::apply` silently ignores `profile_canonical_id` while the digest endpoint rejects it. This asymmetry is intentional and documented: - Apply receives `profile_canonical_id` as a top-level wire-shape field on EVERY op (per `SyncOpIn` definition). The desktop emits it unconditionally; making apply reject it for user-scoped entities would force the desktop to special-case its emit pipeline. - The digest endpoint receives `profile_canonical_id` as a query STRING with strict routing semantics — rejecting it for user-scoped entities catches misuse of a routing parameter. Same name, two contexts, two different contracts. No code change. ## Tests Two new cases in tests/apply.rs: - `track_insert_rejects_negative_numeric_field` — injects `bitrate: -1` into an otherwise-valid track insert; push fails, no row leaks. - `playlist_insert_tracks_rejects_malformed_snapshot_artist` — injects `snapshots["42"].artist: 7`; push fails. Full apply + apply_digest suites green (37 + 7 cases). Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
There was a problem hiding this comment.
Actionable comments posted: 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.
Inline comments:
In `@tests/apply.rs`:
- Around line 1668-1717: The test
playlist_insert_tracks_rejects_malformed_snapshot_artist validates that the push
operation fails due to a malformed snapshot artist field, but it does not verify
that no playlist_track rows were inadvertently created in the database despite
the failure. Following the pattern used in the
track_insert_rejects_negative_numeric_field test which includes a COUNT(*) = 0
check, add a similar database query after the assertion at line 1716 to verify
that no playlist_track rows were inserted, ensuring the operation failed
atomically with no data leakage.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: fc989a9e-1b85-4533-b811-8bac4e6de0e4
📒 Files selected for processing (3)
src/apply.rstests/apply.rstests/apply_digest.rs
Trivial follow-up on #56 CodeRabbit review. Mirrors the pattern from `track_insert_rejects_negative_numeric_field`: asserting on the response status alone proves the push was rejected but doesn't prove the rejection was atomic. The COUNT(*) on playlist_track filtered to the playlist's canonical_id closes that gap — if a future regression starts a transaction, inserts rows, then errors on snapshot parsing, this assertion catches it. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
5 tasks
This was referenced Aug 9, 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.
What
Phase A.2.3 closes the server-side Phase A by wiring the §metadata_digest_version invariant + the
GET /api/v1/sync/digestendpoint that consumes it.Follows #50–#55. This is the last server PR before Phase A.3 (desktop schema additions).
API
Returns:
{ "set_hash": "<blake3-hex>", "version": <i64>, "max_hlc": { "wall": N, "logical": M, "origin_device_id": "<uuid-or-null>" }, "members": [ { "canonical_id": "...", "payload_hash": "<blake3-hex>" } ] }entityis required. Profile-scoped entities (library/playlist/profile/track) requireprofile_canonical_id; user-scoped (liked_track/track_rating) reject it (400 on mismatched pair).set_hashso two replicas compute identical bytes.<library_canonical_id>\u{1f}<file_path>since tracks have no single canonical_id.set_hash= BLAKE3-256 over(canonical_id_len_le_u32, canonical_id_bytes, payload_hash_bytes)per member.Code change
New
db::digestsubmoduleFour monotone-counter helpers —
bump_profile/bump_user/read_profile/read_user. INSERT...ON CONFLICT DO UPDATE serialises concurrent writes atomically.Apply pipeline
apply::canoncollects payload-shape helpers. Every INSERT/UPSERT computes the canonical fields map →compute_payload_hash(A.2.2.1) → binds to the row → callsbump_*. SET_FIELD becomes a 2-round-trip (SELECT current → recompute over full state → UPDATE) to preserve the §metadata_digest_version invariant ("bump iff payload_hash actually changes"). Delete handlers also bump.track::insertTrackInputgainspayload_hash: &[u8].liked/ratingUPSERT-on-conflict refreshes hash + HLC + payload (same shape as A.2.2's HLC refresh).Type relocation
DigestMember/DigestResponse/MaxHlcmove fromapi/sync.rs→src/sync.rssodb::digest_readcan build them without depending on a privateapi::syncsubmodule.New deps
hex0.4 —payload_hashbyte ↔ hex round-trip (BYTEA storage, hex wire).What this does NOT do
sync_op.idordering; Phase C activates trueWHERE existing.hlc < incoming.hlc.trackdigest covers the per-tenant track set without playlist membership.members(the digest can't say "in sync" about a row it can't hash). Phase B handles backfill.Test plan
cargo fmt --all— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all-features --test apply_digest— 5 new cases pass:library_insert_binds_payload_hash_and_bumps_digest— 32-byte hash, bothlibrary+profilecounters bump to 1library_set_field_recomputes_hash_and_bumps_digest— SET_FIELD changes hash + advances counterdigest_endpoint_returns_stable_set_hash_for_library— two inserts, members sorted, idempotent re-readdigest_endpoint_rejects_profile_id_on_user_scoped_entity— 400 on mismatched scopedigest_endpoint_user_scoped_liked_round_trip— user-scoped path round-trips viauser_metadata_digest_versionRefs
Summary by CodeRabbit
Notes de version
Nouvelles fonctionnalités
GET /api/v1/sync/digestpour récupérer un instantané structuré (hash global, versions, membres etmax_hlc).Améliorations / Correctifs
payload_hashdéterministe pour appliquer des changements de façon idempotente.profile_canonical_id) et de la structure dessnapshots(types stricts, valeurs négatives rejetées).Tests