Drive write lease Phase 3 (part 2): require --lease on every Sheets/Docs write verb - #1670
Merged
Conversation
Wires ADR-0080 §9's Drive write lease into every content-mutating Sheets/Docs verb, not just `drive edit`: sheets write/append/clear, add-sheet/rename-sheet/insert-rows/insert-columns/duplicate-sheet/ reorder-sheet/hide-sheet/show-sheet, delete-sheet/delete-rows/ delete-columns/delete-range, format-cells/merge-cells/unmerge-cells/ update-borders/update-dimension-properties/auto-resize-columns, set-data-validation/clear-data-validation, protect-range/ update-protection/unprotect-range, and docs replace/append. The fail-closed lease-check sequence (acquire the ledger lock, verify the token is present/live/bound to the right file/not stale) is factored out of drive edit's original implementation into src/drive/lease/check.rs, shared by all seven engines rather than copied per engine. The staleness check is a fresh files.get immediately before each mutating call, never a reuse of metadata fetched before the ancestor-chain walk that decides the folder-permission gate. Docs keeps its own, separate revisionId staleness check (ADR-0076) alongside the new one; neither replaces the other. Also fixes target_gate::TargetGateOutcome::Gated to thread through the requires_lease flag it was previously computing but discarding, which every one of these engines now depends on.
CoverageTotal: 97.24% ⚪ 0.03 pp vs Comparing
Patch coveragePatch: 100% (2208/2208 new lines covered)
|
Adds the lease-flow tests (NoLease/Expired/WrongFile/Stale/Failed, pre-write staleness-refetch failure, and the require_lease:false skip path) that sheets write/structure/format/validation/protection and docs write were each missing at least one of, plus the lease-ledger-unreadable and native-refresh-refetch-failure paths in the newly extracted drive/lease/check.rs, the describe()/log_status() arms for the lease refusal variants, and resolve_ledger_path's non-dry-run branch across the six CLI entry points. check.rs's ledger-unreadable warn! now binds ledger_path.display() to a local before the macro call (the daemon/services/worktrees.rs load_pr_cache precedent) so the branch is coverage-visible whether or not a tracing subscriber is installed, rather than relying on a per-test subscriber to force the lazy field evaluation.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continues ADR-0080 Phase 3 (issue #1664): wires the
--leaserequirement into every remaining content-mutating Sheets/Docs write verb, closing the gap left after PR #1669 (native-document backup support) where onlydrive editactually required a lease.Verbs gated in this PR:
drive sheets write/append/clear(SheetsWrite)add-sheet/rename-sheet/insert-rows/insert-columns/duplicate-sheet/reorder-sheet/hide-sheet/show-sheet(SheetsStructure) anddelete-sheet/delete-rows/delete-columns/delete-range(SheetsDelete)format-cells/merge-cells/unmerge-cells/update-borders/update-dimension-properties/auto-resize-columnsandset-data-validation/clear-data-validation(bothSheetsStructure)protect-range/update-protection/unprotect-range(SheetsProtection)drive docs replace/append(DocsWrite)Design notes
src/drive/lease/check.rsfactors the token-verification sequence (present, unexpired, bound to the right file, not stale) out ofdrive edit's original implementation — the first PR to add a lease check anywhere. A security-critical refusal path copied seven times is exactly the kind of drift a shared function exists to prevent.files.getimmediately before each mutating call, on every surface — never a reuse of metadata fetched before the ancestor-chain walk that decides the folder-permission gate, which can itself take long enough to let the lease go stale first. This mirrors the TOCTOU fix already landed fordrive editin PR Drive write lease Phase 2: drive lease acquire, --lease on drive edit #1667.requiredRevisionIdcheck (guards thebatchUpdatecall itself against a concurrent edit) and this ADR's lease-version check (guards the lease ledger's recorded Driveversion) both run; neither replaces the other.values/batchUpdateAPIs nor the DocsbatchUpdateAPI return Drive metadata in their response, refreshing a lease's recorded version after a successful write costs one extrafiles.geton these surfaces — the same trade-off ADR-0076 already accepted for the Docs revision lease.sheets::target_gate::TargetGateOutcome::Gatedto actually thread through therequires_leaseflag it was computing but discarding — every engine wired in this PR depends on it.Test plan
cargo build --lib --tests --bin omni-devcargo test --lib(9298 passed)cargo clippy --all-targets --features mcp -- -D warnings(clean)cargo fmt --check(clean)cargo insta test— reviewed and accepted (13 new--lease <TOKEN>flags in the CLI help snapshot, matching exactly the verbs listed above)require_lease: falseopt-out)