fix(cloud): materialize relation mutations on chunk-ingest (WriteChunk)#600
fix(cloud): materialize relation mutations on chunk-ingest (WriteChunk)#600forNerzul wants to merge 1 commit into
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChunk ingestion now materializes relation mutations from ChangesChunk relation materialization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant WriteChunk
participant materializedChunkMutations
participant cloud_mutations
participant ListMutationsSince
WriteChunk->>materializedChunkMutations: materialize chunk.Mutations
materializedChunkMutations->>cloud_mutations: persist relation MutationEntry
ListMutationsSince->>cloud_mutations: read relation mutation
cloud_mutations-->>ListMutationsSince: return persisted relation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/cloud/cloudstore/cloudstore_test.go`:
- Around line 475-479: Update the relation-upsert test fixtures in the relevant
cloudstore tests so every happy-path relation payload includes judgment_status,
marked_by_actor, and marked_by_kind. Add an explicit error case omitting a
required relation payload field and assert the server-side validation error,
while preserving the existing missing entity_key coverage.
In `@internal/cloud/cloudstore/cloudstore.go`:
- Around line 414-418: Validate relation upsert mutations before appending them
to entries. In the mutation-building flow around ValidateSyncMutationPayload,
preserve the default op behavior but reject missing or incomplete relation
payloads—including required relation identifiers, judgment metadata, and project
fields—with an explicit error; do not persist `{}` or silently discard invalid
mutations.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e8a88dc-ba91-40db-9063-99144d4e4385
📒 Files selected for processing (2)
internal/cloud/cloudstore/cloudstore.gointernal/cloud/cloudstore/cloudstore_test.go
| { | ||
| name: "relation missing entity key", | ||
| chunk: engramsync.ChunkData{Mutations: []store.SyncMutation{{Entity: store.SyncEntityRelation}}}, | ||
| want: "mutations[0].entity_key is required", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use valid relation-upsert fixtures and cover missing payload fields.
The direct helper fixture omits required judgment_status, marked_by_actor, and marked_by_kind fields. It currently tests acceptance of an invalid upsert; make both happy-path payloads complete and add an error case for a missing required relation payload field.
As per path instructions, “Verify coverage of happy path, error paths, and edge cases,” and “Admin-enforceable sync policies must be validated server-side and fail loudly.”
Also applies to: 491-594
🤖 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 `@internal/cloud/cloudstore/cloudstore_test.go` around lines 475 - 479, Update
the relation-upsert test fixtures in the relevant cloudstore tests so every
happy-path relation payload includes judgment_status, marked_by_actor, and
marked_by_kind. Add an explicit error case omitting a required relation payload
field and assert the server-side validation error, while preserving the existing
missing entity_key coverage.
Source: Path instructions
| payload := json.RawMessage(strings.TrimSpace(mutation.Payload)) | ||
| if len(payload) == 0 { | ||
| payload = json.RawMessage("{}") | ||
| } | ||
| entries = append(entries, MutationEntry{Project: project, Entity: store.SyncEntityRelation, EntityKey: entityKey, Op: op, Payload: payload}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate relation upserts before inserting them.
A missing op becomes upsert, while an empty payload becomes {} and incomplete payloads are persisted. ValidateSyncMutationPayload requires relation identifiers, judgment metadata, and project fields for an upsert; otherwise mutation-pull consumers receive an unappliable row.
Proposed fix
payload := json.RawMessage(strings.TrimSpace(mutation.Payload))
if len(payload) == 0 {
payload = json.RawMessage("{}")
}
+ validation := store.ValidateSyncMutationPayload(
+ store.SyncEntityRelation, op, string(payload), entityKey,
+ )
+ if validation.Message != "" {
+ return nil, fmt.Errorf(
+ "cloudstore: materialize chunk: mutations[%d] relation: %s",
+ i, validation.Message,
+ )
+ }
entries = append(entries, MutationEntry{Project: project, Entity: store.SyncEntityRelation, EntityKey: entityKey, Op: op, Payload: payload})As per path instructions, “Admin-enforceable sync policies must be validated server-side and fail loudly, never drop data silently.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| payload := json.RawMessage(strings.TrimSpace(mutation.Payload)) | |
| if len(payload) == 0 { | |
| payload = json.RawMessage("{}") | |
| } | |
| entries = append(entries, MutationEntry{Project: project, Entity: store.SyncEntityRelation, EntityKey: entityKey, Op: op, Payload: payload}) | |
| payload := json.RawMessage(strings.TrimSpace(mutation.Payload)) | |
| if len(payload) == 0 { | |
| payload = json.RawMessage("{}") | |
| } | |
| validation := store.ValidateSyncMutationPayload( | |
| store.SyncEntityRelation, op, string(payload), entityKey, | |
| ) | |
| if validation.Message != "" { | |
| return nil, fmt.Errorf( | |
| "cloudstore: materialize chunk: mutations[%d] relation: %s", | |
| i, validation.Message, | |
| ) | |
| } | |
| entries = append(entries, MutationEntry{Project: project, Entity: store.SyncEntityRelation, EntityKey: entityKey, Op: op, Payload: payload}) |
🤖 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 `@internal/cloud/cloudstore/cloudstore.go` around lines 414 - 418, Validate
relation upsert mutations before appending them to entries. In the
mutation-building flow around ValidateSyncMutationPayload, preserve the default
op behavior but reject missing or incomplete relation payloads—including
required relation identifiers, judgment metadata, and project fields—with an
explicit error; do not persist `{}` or silently discard invalid mutations.
Source: Path instructions
The gap is real. The mutation-push path validates relation required-fields and atomically rejects the batch ( Why not this exact snippet, though — three verified reasons:
Plan: keeping this PR scoped to #502 (restore relation presence in chunk-ingest, matching the sibling session/observation/prompt branches that also don't field-validate here). I'm filing a separate issue for the validation asymmetry, which needs a real design choice — the push validator (7 fields, no |
|
Tip For best results, initiate chat on the files or code changes.
Keeping ✏️ Learnings added
|
materializedChunkMutations built cloud_mutations rows only from chunk.Sessions/Observations/Prompts and never iterated chunk.Mutations, where relations ride. A relation uploaded via the chunk path landed in cloud_chunks.payload but never reached cloud_mutations, so mutation-pull consumers never saw it — the chunk-ingest sibling of the push-path gap that Gentleman-Programming#379 fixed. Mirror Gentleman-Programming#379 in materializedChunkMutations: emit relation MutationEntry rows from chunk.Mutations, skipping typed entities already materialized above to avoid duplicate cloud_mutations rows. Adds unit coverage plus a Postgres-gated WriteChunk end-to-end + replay-idempotency test. Refs Gentleman-Programming#502
forNerzul
force-pushed
the
fix/cloud-chunk-ingest-relations
branch
from
cfabcab to
06f86d9
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@Alan-TheGentleman rebased on latest The relation-payload validation asymmetry CodeRabbit flagged is intentionally deferred to #601 (rationale in the thread above); this PR stays scoped to restoring relation presence in chunk-ingest per #502. |
1 participant
🔗 Linked Issue
Closes #502
🏷️ PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance, dependencies, toolingtype:breaking-change— Breaking change📝 Summary
WriteChunk) materializedcloud_mutationsrows only fromchunk.Sessions/Observations/Promptsand never iteratedchunk.Mutations, where relations ride — so a relation uploaded via the chunk path landed incloud_chunks.payloadbut never reachedcloud_mutations, and mutation-pull consumers never saw it.materializedChunkMutations: emit relationMutationEntryrows fromchunk.Mutations, skipping the typed entities already materialized above so no duplicatecloud_mutationsrows are produced.📂 Changes
internal/cloud/cloudstore/cloudstore.gomaterializedChunkMutationsnow iterateschunk.Mutationsand emits a relationMutationEntryfor eachrelation-entity mutation (defaultupsert, empty payload →{},entity_keyrequired). Session/observation/prompt mutations are skipped because they are already materialized from the typed collections, avoiding duplicate rows. Capacity hint updated.internal/cloud/cloudstore/cloudstore_test.goTestMaterializedChunkMutationsCarriesRelationFromChunkMutations(unit: relation materialized, typed rows not duplicated), arelation missing entity keyvalidation case, andTestWriteChunkMaterializesRelationMutationIntoCloudMutations(Postgres-gated end-to-end + replay idempotency).🧪 Test Plan
go test ./...(22 packages ok, 0 failures)go test -tags e2e ./internal/server/...(ok)TDD: the new unit test failed before the fix (
got 2entries — the relation was dropped) and passes after.go vet ./...andgo build ./...are clean. The end-to-endWriteChunk → cloud_mutationstest is gated onCLOUDSTORE_TEST_DSN(requires Postgres) and is skipped locally without it; the unit-level coverage runs unconditionally.✅ Contributor Checklist
Closes #502)type:*label to this PR — checkedtype:bugabove; I have READ access only, so a maintainer may need to apply the label.go test ./...go test -tags e2e ./internal/server/...docs/codebase/sync-and-cloud.md:31already documents that chunks carry the relations graph, and this fix makes the code honor that contract.Co-Authored-Bytrailers in commits💬 Notes for Reviewers
chunk.Sessions/Observations/Prompts, so the new loop filters tostore.SyncEntityRelationto avoid duplicatecloud_mutationsrows (the push materializer emits an observation both as a typed row and as achunk.Mutationsentry, so the dedup matters — covered by a test).ApplyPulledChunkaborting a chunk on a relation FK miss instead of deferring. This PR does not touch it.Summary by CodeRabbit