Guard factory SDK cloud provider deletes#244
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deleteFile method to the MountClient interface and implements it in RelayfileCloudMountClient and FakeMountClient. It includes safety invariants to restrict provider deletes (e.g., Linear or Slack) to failed, unlinked orphan drafts, backed by a new callsite invariant test and comprehensive unit tests. The review feedback suggests preserving the original write/create operation ID in a separate map (#lastWriteOpByPath) to prevent retried delete operations from overwriting it, ensuring that safety checks can still be verified correctly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| readonly #tokenProvider: TokenProvider | ||
| readonly #baseUrl?: string | ||
| readonly #eventClient?: RelayfileEventClient | ||
| readonly #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise<boolean> |
There was a problem hiding this comment.
To prevent retried delete operations from overwriting the tracked write/create operation ID (which is needed to verify the safety invariants of the original creation), we should introduce a separate map #lastWriteOpByPath to preserve the original write operation ID.
| readonly #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise<boolean> | |
| readonly #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise<boolean> | |
| readonly #lastWriteOpByPath = new Map<string, string>() |
| this.#lastOpByPath.set(path, (await this.#client.deleteFile({ | ||
| workspaceId: this.workspaceId, | ||
| path, | ||
| baseRevision: current.revision, | ||
| })).opId) |
There was a problem hiding this comment.
Before overwriting #lastOpByPath with the new deleteOpId, preserve the original write/create operation ID in #lastWriteOpByPath if it hasn't been saved already.
const writeOpId = this.#lastOpByPath.get(path)
if (writeOpId && !this.#lastWriteOpByPath.has(path)) {
this.#lastWriteOpByPath.set(path, writeOpId)
}
this.#lastOpByPath.set(path, (await this.#client.deleteFile({
workspaceId: this.workspaceId,
path,
baseRevision: current.revision,
})).opId)| const opId = this.#lastOpByPath.get(path) | ||
| if (!opId || !this.#client.getOp) { | ||
| throw new Error(`Refusing provider delete for ${path}: create operation is unknown`) | ||
| } |
There was a problem hiding this comment.
Use the preserved write/create operation ID from #lastWriteOpByPath if available, falling back to #lastOpByPath.
| const opId = this.#lastOpByPath.get(path) | |
| if (!opId || !this.#client.getOp) { | |
| throw new Error(`Refusing provider delete for ${path}: create operation is unknown`) | |
| } | |
| const opId = this.#lastWriteOpByPath.get(path) ?? this.#lastOpByPath.get(path) | |
| if (!opId || !this.#client.getOp) { | |
| throw new Error(`Refusing provider delete for ${path}: create operation is unknown`) | |
| } |
|
Implemented the PR-scoped fix for the verified review finding: provider delete safety now keeps the original write/create op id separate from the latest op id, so a delete retry does not validate against a prior delete op. Changes:
Addressed comments
Advisory NotesNone. Local verification run:
I did not run the macOS-only packaged CI job or verify GitHub mergeability from here. |
|
No open PR-scoped findings remain. I did not need to make new edits; the current checkout already contains the Gemini-requested preserved write-op fix and regression coverage. Addressed comments
Advisory NotesNone. Verification completed:
GitHub Actions for head |
kjgbot
force-pushed
the
factory-sdk/d7-deletefile-guard
branch
from
4120aa6 to
a24c484
Compare
1 participant
Summary
MountClient.deleteFilesurface and implement it inRelayfileCloudMountClient./linear/**and/slack/**.getOp(opId)is terminal failed/dead-lettered/canceled,providerResult.externalIdis absent,isAllowedDelete(path, currentContent)also approves.mount.deleteFilecallsites cannot appear outside guarded writeback modules.Tests
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonNotes
origin/main; no changes to Harden factory SDK cloud writeback payloads #243.src/mainedits and no new dependencies.