feat(drive): add conditional file replacement - #929
Conversation
|
Hi! I opened this as a draft to get early feedback on the API approach. The main design question is using Drive v2 only for conditional replacement because Drive v3 does not expose the file ETag needed for an atomic Would this limited use of Drive v2 be acceptable upstream, or would you prefer a different direction? If the approach looks reasonable, I’ll mark the PR ready for full review. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 1:40 PM ET / 17:40 UTC. ClawSweeper reviewWhat this changesThis PR adds Merge readinessKeep this PR open. It adds a bounded opt-in Drive write-safety feature with credible real-Drive proof and no definite patch defect found, but landing it still requires an explicit maintainer decision to support the isolated Drive v2 dependency used for atomic ETag preconditions. Priority: P2 Review scores
Verification
How this fits togetherThe Drive upload command converts a local file and CLI flags into Google Drive API writes. Normal replacement continues through Drive v3, while the new conditional path obtains version and ETag metadata through Drive v2 before issuing one guarded media update. flowchart LR
A[Local file and upload flags] --> B[Drive upload command]
B --> C{Expected version supplied?}
C -->|No| D[Drive v3 replacement]
C -->|Yes| E[Drive v2 metadata read]
E --> F{Version and ETag valid?}
F -->|Yes| G[Single If-Match media update]
F -->|No| H[Conflict or fail-closed error]
G --> I[Upload result or HTTP 412 conflict]
Decision needed
Why: The patch is technically bounded and proven, but source tests cannot choose the repository's long-term support policy for an older Google Drive API surface or the intended user experience when that API becomes unavailable. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the opt-in v3-compatible default and land the guarded v2 path only if maintainers explicitly accept Drive v2 as the narrowly scoped atomic-precondition mechanism, with the documented fail-closed behavior and regression coverage preserved. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR proposes a new opt-in Drive write mode, not a failure of an established current-main behavior. The contributor did provide a high-confidence live validation path for the proposed success and stale-version outcomes. Is this the best way to solve the issue? Unclear pending maintainer policy direction. The ETag AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fd3d054499f3. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
Updated the conditional replacement path in The conditional update now:
The new tests verify that:
|
Regarding the requested behavior evidence, the automated tests now distinguish the two conflict cases:
No credential-backed Drive run has been performed for this draft yet. A redacted live run can demonstrate the matching-version success path and the already-stale-version path, but reliably inducing the post-GET HTTP 412 race requires coordinating a concurrent write and is better covered deterministically. Would the maintainers prefer live redacted success/stale-version evidence before deciding whether the isolated Drive v2 conditional path is acceptable, or should the v2 design direction be resolved first? |
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
Credential-backed Drive evidenceValidated PR head The account and Drive file ID are redacted below. Nonessential JSON fields are omitted. Matching-version replacementThe initial upload was downloaded and verified against the local baseline: The conditional dry run represented the expected precondition: {
"dry_run": true,
"op": "drive.upload",
"request": {
"replace_file_id": "<FILE_ID>",
"if_version": 3,
"size": 79,
"mime_type": "text/plain"
}
}The matching conditional replacement succeeded: The downloaded replacement matched the local payload: Drive version values are monotonic but not necessarily consecutive. Google documents that the counter reflects every server-side change, including changes not visible to the user. The conditional implementation relies only on exact equality and does not assume that a successful replacement increments the version by one. Already-stale versionAfter Drive had advanced to version Post-conflict verification: The stale payload was not applied. Atomic-race and retry validationThe focused local suite was rerun using These cases verify the exact Cleanup and credential handlingThe disposable file was moved to Drive Trash: {
"deleted": false,
"id": "<FILE_ID>",
"trashed": true
}The repository remained clean. gog's existing authentication boundary was used; no OAuth token, client secret, authorization code, or credential material was printed or captured. |
clawsweeper
Bot
added
proof: sufficient
Co-authored-by: Karsten Evers <50057690+karstenevers@users.noreply.github.com>
779c18a to
bdd8abd
Compare
|
Maintainer decision accepted: gog supports the narrowly scoped Drive v2 ETag path for opt-in conditional replacement. Existing and unconditional uploads remain on Drive v3; the conditional write fails closed if precondition material is unavailable and never retries or falls back to an unconditional update. Landed candidate: Proof on that exact candidate:
The commit preserves contributor credit with |
2 participants
Summary
Add opt-in atomic conflict protection to
gog drive upload --replace.Callers that previously read a Drive file can pass its version:
Existing replacement remains available and unchanged:
Motivation
Replacement previously performed a metadata read followed by an unconditional media update. If another collaborator changed the file between those requests, the newer content could be overwritten.
A separate version comparison is insufficient because the file can still change after the comparison and before the update. The final media update must carry an atomic server-side precondition.
Implementation
Conditional replacement:
versionand ETag.--if-version.If-Matchon one mediaFiles.Updaterequest.Drive v3 does not expose the file ETag needed for this precondition. Therefore, only the opt-in conditional path uses Drive v2, which exposes both
versionandetag. Normal uploads and unconditional replacement remain on Drive v3.The Drive v2 service is created through gog’s existing authenticated Google API service factory. OAuth tokens are not exported or exposed, and no second authentication implementation is introduced.
The Drive v2 media update uses HTTP
PUTrather than the v3PATCH, but theIf-Matchprecondition is attached to the final media write and closes the check/write race atomically.Design tradeoff
Drive v3 does not expose the file ETag needed for an atomic
If-Matchprecondition. This implementation therefore uses Drive v2 only for the opt-in conditional replacement path.Google currently documents Drive v2 without an announced sunset date, but recommends v3 for general use. If v2 is deprecated later, this path will need an alternative atomic precondition mechanism.
The v2 dependency is isolated from normal uploads and unconditional v3 replacement. Conditional replacement also fails closed rather than falling back to an unsafe update.
Conflict behavior
--replacebehavior remains unconditional when--if-versionis absent.Backward compatibility
The new behavior is opt-in. Existing callers can continue using:
without supplying a version and without any change to current output or replacement behavior.
Tests
Added
httptestcoverage for:If-Matchpropagation;Validation completed:
make fmt;make test;make ci.make cicompleted successfully with:0 issues;Credential-backed Drive evidence
PR head
779c18a6a14e681b25c9ae7081c9be22214121fbwas validated against Google Drive on July 19, 2026 using the local fork and one disposable text file.3to6, and the downloaded replacement matched the local payload byte-for-byte.3reported a conflict against current version6.httptestsuite also passed for exactIf-Matchpropagation, HTTP 412 conflict handling, single-attempt media updates, missing-precondition fail-closed behavior, and prevention of unconditional retry or fallback.Drive version values are monotonic but not necessarily consecutive. Google documents that the counter reflects every server-side change, including changes not visible to the user. The conditional implementation relies only on exact equality and does not assume that a successful replacement increments the version by one.
The account and Drive file ID are redacted from the posted evidence. gog’s existing authentication boundary was used; no OAuth token, client secret, authorization code, or credential material was printed or recorded.
References
files.update