feat(relayfile): fork API — SDK + server routes + core primitives#58
feat(relayfile): fork API — SDK + server routes + core primitives#58khaliqgant merged 5 commits intomainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1ed3e4650
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (s *Server) handleCommitFork(w http.ResponseWriter, _ *http.Request, workspaceID, forkID, correlationID string) { | ||
| resp, err := s.store.CommitFork(workspaceID, forkID, correlationID) | ||
| if err != nil { |
There was a problem hiding this comment.
handleCommitFork immediately calls s.store.CommitFork(...) without re-validating per-file ACL rules or path-scoped authorization for the overlay entries being promoted. In a shared workspace/proposal flow, a caller with broad fs:write scope on the workspace (but without access to specific files/paths) can still commit another actor’s staged overlay changes to protected files, which is an authorization bypass.
Useful? React with 👍 / 👎.
| writeJSON(w, http.StatusConflict, map[string]any{ | ||
| "error": "parent_moved", | ||
| "currentRevision": parentMoved.CurrentRevision, | ||
| "correlationId": correlationID, |
There was a problem hiding this comment.
The parent_moved branch returns a custom payload with error/currentRevision instead of the normal code/message envelope used elsewhere. The TypeScript SDK error parser keys off code/message, so commitFork conflicts become generic API errors and lose currentRevision, which prevents clients from reliably detecting and handling rebase/retry paths.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| case relayfile.ErrForkExpired: | ||
| writeJSON(w, http.StatusGone, map[string]any{ | ||
| "error": "fork_expired", | ||
| "correlationId": correlationID, | ||
| }) |
There was a problem hiding this comment.
🔴 Fork expired error response uses "error" key instead of standard "code" key, breaking SDK error detection
In handleCommitFork, when the fork has expired, the response uses writeJSON with a non-standard JSON shape {"error": "fork_expired", "correlationId": "..."} instead of the standard writeError format {"code": "...", "message": "...", "correlationId": "..."} used by every other endpoint (see writeError at internal/httpapi/server.go:2365-2371). The SDK's throwForError at packages/sdk/typescript/src/client.ts:945-1015 reads data.code from the ErrorResponse interface (packages/sdk/typescript/src/types.ts:264-269), so the resulting RelayFileApiError will have code: "api_error" and message: "HTTP 410" instead of code: "fork_expired". SDK consumers cannot programmatically identify fork expiry errors.
| case relayfile.ErrForkExpired: | |
| writeJSON(w, http.StatusGone, map[string]any{ | |
| "error": "fork_expired", | |
| "correlationId": correlationID, | |
| }) | |
| case relayfile.ErrForkExpired: | |
| writeError(w, http.StatusGone, "fork_expired", err.Error(), correlationID) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| writeJSON(w, http.StatusBadRequest, map[string]any{ | ||
| "error": "nested_forks_not_supported", | ||
| "correlationId": correlationID, | ||
| }) | ||
| return |
There was a problem hiding this comment.
🟡 Nested forks error response uses "error" key instead of standard "code" key
Same inconsistency as the fork_expired response: handleCreateFork uses writeJSON with {"error": "nested_forks_not_supported"} instead of the standard writeError format with "code" and "message" keys. The SDK will see this as code: "api_error" and message: "HTTP 400" instead of the intended error code.
| writeJSON(w, http.StatusBadRequest, map[string]any{ | |
| "error": "nested_forks_not_supported", | |
| "correlationId": correlationID, | |
| }) | |
| return | |
| writeError(w, http.StatusBadRequest, "nested_forks_not_supported", "nested forks are not supported", correlationID) | |
| return |
Was this helpful? React with 👍 or 👎 to provide feedback.
| _, task := s.recordWriteLocked(ws, path, revision, "file.deleted", existing.Provider, correlationID) | ||
| tasks = append(tasks, task) | ||
| } | ||
| deletedCount++ |
There was a problem hiding this comment.
🟡 Redundant nested if existed blocks in fork commit delete path
In CommitForkWithValidator, the delete case (lines 1640-1644) contains two redundant if existed wrappers around deletedCount++. The first if existed block (lines 1633-1639) already handles the actual file deletion. The second pair of nested if existed blocks is clearly a merge or copy-paste artifact. While the Go compiler accepts this and the runtime behavior is correct (the condition is checked redundantly), this is malformed code that could mislead future editors into thinking there's complex conditional logic here.
Malformed code block
if existed {
if existed {
deletedCount++
}
}Should be simply:
if existed {
deletedCount++
}
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Follow-up on Devin review feedback:
I also fixed the related failing CI wiring in the same push by building |
2 participants
Summary
/v1/workspaces/:workspaceId/forks:POST /forkscreates or returns an idempotent fork for(workspaceId, proposalId).DELETE /forks/:forkIddiscards overlay state idempotently.POST /forks/:forkId/commitpromotes overlay writes/deletes back to the parent when the parent revision still matches.forkIdquery scoping to existing fs routes for file read/write/delete, bulk write, tree, and query.internal/httpapi+internal/relayfile.Store). The Cloudflare Worker/Durable Object tree described in the issue is not present onmain.@relayfile/corefork primitives inpackages/core/src/forks.tsand re-exported them.createFork(input)discardFork(input)commitFork(input)forkIdSDK fields forwriteFile,readFile,deleteFile,bulkWrite,listTree, andqueryFiles.@relayfile/core:0.3.0->0.5.0@relayfile/sdk:0.3.0->0.4.0Storage Model
Fork reads and listings merge parent files with overlay entries: overlay writes win, overlay deletes hide parent files. The current Go store does not have historical point-in-time file reads, so unmodified fork paths read from the current parent view. Commit still compare-and-sets against the stored
parentRevisionand returnsparent_movedif the parent changed.Test Coverage
SDK tests added in
packages/sdk/typescript/src/client.test.ts:createForkposts{ proposalId, ttlSeconds }and returns a fork handle.createForkomitsttlSecondswhen unset.discardForksendsDELETE /forks/:forkIdand resolves on 204.commitForksendsPOST /forks/:forkId/commitand returns commit counts.writeFilewithforkIdappendsforkIdand keeps the JSON body unchanged.readFilewithforkIdappendsforkId.bulkWritewithforkIdappendsforkId.listTreewithforkIdappendsforkId.writeFilewithoutforkIddoes not includeforkId.queryFileswithforkIdappendsforkId.deleteFilewithforkIdappendsforkId.Server tests added in
internal/httpapi/server_test.go:parent_moved.proposalId-> sameforkId.fork_expired.Core tests added in
packages/core/src/forks.test.tscover TTL normalization, expiry calculation, overlay revision generation, write overlay storage, and delete tombstones.Migration Notes
No database migration is required for this repo's current in-process/JSON store.
New persisted JSON state fields:
persistedState.forks: active fork metadata and overlays.workspaceState.revision: parent workspace head revision used for fork commit compare-and-set.In-memory only:
Known Gaps / Out of Scope
POST /forkswithforkIdreturnsnested_forks_not_supported.docs/fork-architecture.md.apps/relayfileis not present onmain; this PR implements the server that exists in this repo.Verification
npm installnpm run build --workspace=packages/core && npm run test --workspace=packages/corenpm run typecheck --workspace=packages/sdk/typescript && npm run test --workspace=packages/sdk/typescript && npm run build --workspace=packages/sdk/typescriptgo test ./...npm run typecheck --if-presentnpm run build --if-presentnpm test --if-present