Add writeback draft content identity to mount bulk writes#253
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 21 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds persistent content identity tracking through the writeback pipeline. A new ChangesContentIdentity Writeback Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Additional daemon ground truth from review: |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
khaliqgant
force-pushed
the
fix/writeback-content-identity
branch
from
5a0f89d to
52cb9ad
Compare
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Reviewed PR #253 and fixed a real propagation gap: Added regression coverage in Local verification passed: |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #253 and fixed a real propagation gap: Added regression coverage in Local verification passed: |
|
Reviewed PR #253 and made one focused fix: Validation run locally:
|
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #253 and made one focused fix: Validation run locally:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/relayfile/store.go (1)
1417-1468:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject invalid
contentIdentityvalues before recording the writeback.
BulkWritenow persistsinput.ContentIdentityas-is intows.Opsand queued writebacks. That means emptykind/keyor a negativettlSecondscan leak back out through operation and pending-writeback responses even though the new contract requires non-empty identifiers and a non-negative TTL.Proposed fix
for _, input := range files { path := normalizePath(input.Path) if strings.TrimSpace(input.Path) == "" { errorsOut = append(errorsOut, BulkWriteError{ Path: input.Path, @@ if err := validateEncodedContent(input.Content, encoding); err != nil { errorsOut = append(errorsOut, BulkWriteError{ Path: path, Code: "invalid_content", Message: err.Error(), }) continue } + if input.ContentIdentity != nil { + identity := input.ContentIdentity + if strings.TrimSpace(identity.Kind) == "" || strings.TrimSpace(identity.Key) == "" || identity.TTLSeconds < 0 { + errorsOut = append(errorsOut, BulkWriteError{ + Path: path, + Code: "invalid_content_identity", + Message: "contentIdentity.kind, contentIdentity.key, and contentIdentity.ttlSeconds must be valid", + }) + continue + } + } contentType := strings.TrimSpace(input.ContentType) if contentType == "" { contentType = "text/markdown" }🤖 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/relayfile/store.go` around lines 1417 - 1468, Reject invalid input.ContentIdentity before calling recordWriteWithContentIdentityLocked by validating its fields in the BulkWrite loop: ensure ContentIdentity.Kind and ContentIdentity.Key are non-empty and ContentIdentity.TtlSeconds is >= 0; if validation fails append a BulkWriteError (e.g., Code "invalid_content_identity" and a clear Message) for that path and continue. Update the validation logic near the loop that constructs file and before the call to s.recordWriteWithContentIdentityLocked(ws, path, revision, eventType, file.Provider, "", input.ContentIdentity) so invalid identities are never persisted into ws.Ops or queued writebacks.
🤖 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 @.trajectories/completed/2026-06/traj_y5jru5dh9ku6/summary.md:
- Line 1: The trajectory title currently reads "Trajectory: Review and fix PR
`#243`" while the summary body references "PR `#253`"; update the inconsistent PR
reference so both places use the same PR number (choose the correct canonical ID
and replace the other), specifically edit the title string "Trajectory: Review
and fix PR `#243`" or the summary body occurrence of "PR `#253`" to match, and
verify no other occurrences in this file (summary.md) contain the mismatched PR
ID so completion metadata is consistent.
In @.trajectories/completed/2026-06/traj_y5jru5dh9ku6/trajectory.json:
- Line 5: The structured trajectory metadata has a PR number mismatch: the JSON
field "title" (and the related task.title) currently references PR `#243` while
the retrospective text refers to PR `#253`; update the "title"/task.title value to
match the retrospective (PR `#253`) or vice‑versa so both fields are identical
(e.g., change task.title/"title" to "Review and fix PR `#253`") to ensure
consistent historical indexing.
In `@internal/mountsync/syncer_test.go`:
- Around line 1842-1844: The test currently asserts identity.Kind against the
internal constant mountWritebackCreateDraftContentIdentityKind which can mask
contract drift; update the assertions in internal/mountsync/syncer_test.go to
compare identity.Kind to the literal string "mount-writeback-create-draft"
instead of the constant (replace uses of
mountWritebackCreateDraftContentIdentityKind with the wire literal) in both
places around the existing identity.Kind checks so the test verifies the actual
on-the-wire value.
In `@internal/relayfile/store.go`:
- Around line 3095-3106: The map currently assigns the internal *ContentIdentity
pointer directly to "contentIdentity" (using item.ContentIdentity and
op.ContentIdentity), which can let callers mutate store state; instead clone the
ContentIdentity instance before inserting into itemOut. Locate the block that
builds itemOut in store.go (the itemOut map and the conditional that checks
item.ContentIdentity and op.ContentIdentity) and replace direct assignments with
a shallow/deep copy (use the same cloning approach used in the writeback
path—e.g., call the existing ContentIdentity.Clone or construct a new
ContentIdentity and copy fields) so the map contains an independent copy rather
than the original pointer.
In `@packages/sdk/typescript/src/types.ts`:
- Around line 59-63: The response interfaces are missing the new contentIdentity
field: update the TypeScript types by importing/using ContentIdentity and adding
contentIdentity?: ContentIdentity to both OperationStatusResponse and
WritebackItem so SDK consumers can access the server/OpenAPI-provided
contentIdentity object.
---
Outside diff comments:
In `@internal/relayfile/store.go`:
- Around line 1417-1468: Reject invalid input.ContentIdentity before calling
recordWriteWithContentIdentityLocked by validating its fields in the BulkWrite
loop: ensure ContentIdentity.Kind and ContentIdentity.Key are non-empty and
ContentIdentity.TtlSeconds is >= 0; if validation fails append a BulkWriteError
(e.g., Code "invalid_content_identity" and a clear Message) for that path and
continue. Update the validation logic near the loop that constructs file and
before the call to s.recordWriteWithContentIdentityLocked(ws, path, revision,
eventType, file.Provider, "", input.ContentIdentity) so invalid identities are
never persisted into ws.Ops or queued writebacks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a699d1e4-4d40-4487-96d9-5f64274b6125
📒 Files selected for processing (12)
.trajectories/active/traj_y5jru5dh9ku6/trajectory.json.trajectories/completed/2026-06/traj_y5jru5dh9ku6/summary.md.trajectories/completed/2026-06/traj_y5jru5dh9ku6/trajectory.json.trajectories/index.jsoninternal/mountsync/syncer.gointernal/mountsync/syncer_test.gointernal/mountsync/types.gointernal/relayfile/store.gointernal/relayfile/store_test.goopenapi/relayfile-v1.openapi.yamlpackages/sdk/typescript/src/client.test.tspackages/sdk/typescript/src/types.ts
💤 Files with no reviewable changes (2)
- .trajectories/active/traj_y5jru5dh9ku6/trajectory.json
- .trajectories/index.json
| "id": "traj_y5jru5dh9ku6", | ||
| "version": 1, | ||
| "task": { | ||
| "title": "Review and fix PR #243" |
There was a problem hiding this comment.
Fix PR number mismatch in structured trajectory metadata.
task.title points to PR #243, but retrospective text documents PR #253. Keep both fields aligned to avoid incorrect historical indexing.
Suggested fix
- "title": "Review and fix PR `#243`"
+ "title": "Review and fix PR `#253`"Also applies to: 53-53
🤖 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 @.trajectories/completed/2026-06/traj_y5jru5dh9ku6/trajectory.json at line 5,
The structured trajectory metadata has a PR number mismatch: the JSON field
"title" (and the related task.title) currently references PR `#243` while the
retrospective text refers to PR `#253`; update the "title"/task.title value to
match the retrospective (PR `#253`) or vice‑versa so both fields are identical
(e.g., change task.title/"title" to "Review and fix PR `#253`") to ensure
consistent historical indexing.
| if identity.Kind != mountWritebackCreateDraftContentIdentityKind { | ||
| t.Fatalf("content identity kind = %q, want %q", identity.Kind, mountWritebackCreateDraftContentIdentityKind) | ||
| } |
There was a problem hiding this comment.
Pin the ContentIdentity.Kind wire literal in these contract tests
Both assertions currently compare against an internal constant, so a constant drift could pass tests while still breaking the locked wire contract. Assert the literal "mount-writeback-create-draft" in the test path.
🔧 Suggested patch
@@
- if identity.Kind != mountWritebackCreateDraftContentIdentityKind {
- t.Fatalf("content identity kind = %q, want %q", identity.Kind, mountWritebackCreateDraftContentIdentityKind)
+ if identity.Kind != "mount-writeback-create-draft" {
+ t.Fatalf("content identity kind = %q, want %q", identity.Kind, "mount-writeback-create-draft")
}
@@
- if identity.Kind != mountWritebackCreateDraftContentIdentityKind {
- t.Fatalf("flushed identity kind = %q, want %q", identity.Kind, mountWritebackCreateDraftContentIdentityKind)
+ if identity.Kind != "mount-writeback-create-draft" {
+ t.Fatalf("flushed identity kind = %q, want %q", identity.Kind, "mount-writeback-create-draft")
}Also applies to: 1947-1949
🤖 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/mountsync/syncer_test.go` around lines 1842 - 1844, The test
currently asserts identity.Kind against the internal constant
mountWritebackCreateDraftContentIdentityKind which can mask contract drift;
update the assertions in internal/mountsync/syncer_test.go to compare
identity.Kind to the literal string "mount-writeback-create-draft" instead of
the constant (replace uses of mountWritebackCreateDraftContentIdentityKind with
the wire literal) in both places around the existing identity.Kind checks so the
test verifies the actual on-the-wire value.
| itemOut := map[string]any{ | ||
| "id": item.OpID, | ||
| "workspaceId": item.WorkspaceID, | ||
| "path": item.Path, | ||
| "revision": item.Revision, | ||
| "correlationId": item.CorrelationID, | ||
| }) | ||
| } | ||
| if item.ContentIdentity != nil { | ||
| itemOut["contentIdentity"] = item.ContentIdentity | ||
| } else if op.ContentIdentity != nil { | ||
| itemOut["contentIdentity"] = op.ContentIdentity | ||
| } |
There was a problem hiding this comment.
Clone contentIdentity before returning pending items.
The response currently exposes the queue/op's internal *ContentIdentity pointer directly. In-process callers can mutate store state through the returned map. Return a clone here, the same way the writeback path already does.
Proposed fix
itemOut := map[string]any{
"id": item.OpID,
"workspaceId": item.WorkspaceID,
"path": item.Path,
"revision": item.Revision,
"correlationId": item.CorrelationID,
}
if item.ContentIdentity != nil {
- itemOut["contentIdentity"] = item.ContentIdentity
+ itemOut["contentIdentity"] = cloneContentIdentity(item.ContentIdentity)
} else if op.ContentIdentity != nil {
- itemOut["contentIdentity"] = op.ContentIdentity
+ itemOut["contentIdentity"] = cloneContentIdentity(op.ContentIdentity)
}
result = append(result, itemOut)
}📝 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.
| itemOut := map[string]any{ | |
| "id": item.OpID, | |
| "workspaceId": item.WorkspaceID, | |
| "path": item.Path, | |
| "revision": item.Revision, | |
| "correlationId": item.CorrelationID, | |
| }) | |
| } | |
| if item.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = item.ContentIdentity | |
| } else if op.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = op.ContentIdentity | |
| } | |
| itemOut := map[string]any{ | |
| "id": item.OpID, | |
| "workspaceId": item.WorkspaceID, | |
| "path": item.Path, | |
| "revision": item.Revision, | |
| "correlationId": item.CorrelationID, | |
| } | |
| if item.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = cloneContentIdentity(item.ContentIdentity) | |
| } else if op.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = cloneContentIdentity(op.ContentIdentity) | |
| } |
🤖 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/relayfile/store.go` around lines 3095 - 3106, The map currently
assigns the internal *ContentIdentity pointer directly to "contentIdentity"
(using item.ContentIdentity and op.ContentIdentity), which can let callers
mutate store state; instead clone the ContentIdentity instance before inserting
into itemOut. Locate the block that builds itemOut in store.go (the itemOut map
and the conditional that checks item.ContentIdentity and op.ContentIdentity) and
replace direct assignments with a shallow/deep copy (use the same cloning
approach used in the writeback path—e.g., call the existing
ContentIdentity.Clone or construct a new ContentIdentity and copy fields) so the
map contains an independent copy rather than the original pointer.
| export interface ContentIdentity { | ||
| kind: string; | ||
| key: string; | ||
| ttlSeconds?: number; | ||
| } |
There was a problem hiding this comment.
Expose contentIdentity on the response types as well.
The server and OpenAPI now return this object from operation-status and pending-writeback payloads, but OperationStatusResponse and WritebackItem still omit it in this file. SDK consumers won't be able to read the new field even though the API sends it.
Proposed fix
export interface OperationStatusResponse {
opId: string;
path?: string;
revision?: string;
action?: WritebackActionType;
provider?: string;
status: OperationStatus;
attemptCount: number;
nextAttemptAt?: string | null;
lastError?: string | null;
providerResult?: Record<string, unknown>;
+ contentIdentity?: ContentIdentity;
correlationId?: string;
createdAt?: string;
updatedAt?: string;
completedAt?: string | null;
}
@@
export interface WritebackItem {
id: string;
workspaceId: string;
path: string;
revision: string;
correlationId: string;
+ contentIdentity?: ContentIdentity;
state?: WritebackListState;
provider?: string;
action?: WritebackActionType;
ts?: string;
code?: WritebackDeadLetterErrorCode | string;🤖 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 `@packages/sdk/typescript/src/types.ts` around lines 59 - 63, The response
interfaces are missing the new contentIdentity field: update the TypeScript
types by importing/using ContentIdentity and adding contentIdentity?:
ContentIdentity to both OperationStatusResponse and WritebackItem so SDK
consumers can access the server/OpenAPI-provided contentIdentity object.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/relayfile/store.go">
<violation number="1" location="internal/relayfile/store.go:3103">
P2: Return a cloned `ContentIdentity` here instead of the internal pointer to prevent callers from mutating store state through `GetPendingWritebacks` results.</violation>
</file>
<file name="openapi/relayfile-v1.openapi.yaml">
<violation number="1" location="openapi/relayfile-v1.openapi.yaml:2463">
P2: Update the TypeScript SDK response interfaces to include `contentIdentity`; the API contract now exposes this field, and leaving SDK response types unchanged will hide it from SDK consumers.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| itemOut["contentIdentity"] = item.ContentIdentity | ||
| } else if op.ContentIdentity != nil { | ||
| itemOut["contentIdentity"] = op.ContentIdentity |
There was a problem hiding this comment.
P2: Return a cloned ContentIdentity here instead of the internal pointer to prevent callers from mutating store state through GetPendingWritebacks results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/relayfile/store.go, line 3103:
<comment>Return a cloned `ContentIdentity` here instead of the internal pointer to prevent callers from mutating store state through `GetPendingWritebacks` results.</comment>
<file context>
@@ -3077,13 +3092,19 @@ func (s *Store) GetPendingWritebacks(workspaceID string) []map[string]any {
- })
+ }
+ if item.ContentIdentity != nil {
+ itemOut["contentIdentity"] = item.ContentIdentity
+ } else if op.ContentIdentity != nil {
+ itemOut["contentIdentity"] = op.ContentIdentity
</file context>
| itemOut["contentIdentity"] = item.ContentIdentity | |
| } else if op.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = op.ContentIdentity | |
| itemOut["contentIdentity"] = cloneContentIdentity(item.ContentIdentity) | |
| } else if op.ContentIdentity != nil { | |
| itemOut["contentIdentity"] = cloneContentIdentity(op.ContentIdentity) |
| @@ -2460,6 +2460,8 @@ components: | |||
| providerResult: | |||
There was a problem hiding this comment.
P2: Update the TypeScript SDK response interfaces to include contentIdentity; the API contract now exposes this field, and leaving SDK response types unchanged will hide it from SDK consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openapi/relayfile-v1.openapi.yaml, line 2463:
<comment>Update the TypeScript SDK response interfaces to include `contentIdentity`; the API contract now exposes this field, and leaving SDK response types unchanged will hide it from SDK consumers.</comment>
<file context>
@@ -2460,6 +2460,8 @@ components:
providerResult:
type: object
additionalProperties: true
+ contentIdentity:
+ $ref: '#/components/schemas/ContentIdentity'
correlationId:
</file context>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
1 participant
Summary
contentIdentityto the relayfile bulk write wire type and OpenAPI schemamount-writeback-create-draftidentities for mountsync bulk writeback draft uploadssnapshot.Hashlowercase SHA-256 hex semantics, and explicitttlSeconds: 2592000in Go testsLocked #1977 contract
mount-writeback-create-draftworkspaceId:normalizedRemotePath:contentHashws_test:/slack/channels/C123/messages/messages 5ab77d67.json:751f9591557700f69b5ceefcdec7ead8563a10f0a712c501a5028699be021511snapshot.Hashdirectly2592000(30 days), fixed-from-first-write coverage for ordinary replay windows; Cloud Phase 3 owns dirty STARTED sandboxes of any ageSequencing / deploy path
Cloud Phase 1 receiver must merge and deploy first; this daemon field is inert until Cloud bulk writes honor
contentIdentity. After this PR merges: publishrelayfile-mount, bumpRELAYFILE_MOUNT_VERSIONin the Cloud snapshot config, runrebuild-snapshot.yml, promote the snapshot pin, then deploy Cloud so the new snapshot reaches prod. hn-monitor verification must be cross-run/persisted-draft replay, not a same-window false pass.Verification
go test ./internal/mountsync -run 'TestBulkWrite_ContentIdentity|TestBulkWrite_FlushSendsContentIdentity|TestBulkWrite_ChunksBySerializedRequestSize'\n-go test ./internal/relayfile ./internal/httpapi ./internal/mountsync\n-go test ./cmd/... ./internal/mountfuse\n-scripts/check-contract-surface.sh\n-go test ./...\n-go build ./...\n\nReview gate: rf-review + claude-1 before merge. Do not merge before Cloud Phase 1 is live.