Drive write lease Phase 3 (part 1): native-document backups - #1669
Merged
Conversation
Phase 3 (in progress) of ADR-0080 (issue #1664): a Google Sheet/Doc/ Slide has no bytes for the Phase 2 byte-backup path to hash and copy locally, so it now backs up as a lossless Drive-side files.copy into a new per-account lease_backup_folder_id setting instead. - FilesApi::copy (files.copy) with its own CopyForBackup scope hint (always the unrestricted `drive` scope -- unlike edit_content there is no app-created-it case, since leasing exists precisely to back up files omni-dev did not create). - LeaseRecord's backup field is now the LeaseBackup enum (Bytes { path, sha256, size } / DriveCopy { file_id }) instead of the three flat, always-required fields Phase 2 shipped -- the two forms are mutually exclusive by construction, which a handful of independently-optional fields would only have documented. - acquire() branches on target.is_google_native(): refuses outright, before authenticating at all, when native and no backup folder is configured (the pre-existing refusal path, now conditional); when a folder is configured, backs up via files.copy after authentication succeeds, same ordering as the byte-backup path. - DriveAccountSettings.lease_backup_folder_id (per-account, same reason write_permissions is: a folder id only means something inside the one Drive it was minted in), resolved by a new active_account_lease_backup_folder_id() helper mirroring active_account_rules(). Caught during implementation: AcquireCommand::execute now calls the new settings-reading helper, which made two existing wiremock-only CLI tests implicitly depend on this machine's real settings.json instead of being fully isolated. Fixed by giving both the same EnvGuard::clear_credentials() isolation drive::test_support's other settings-reading tests already use. Sheets/Docs write verbs still don't require a lease -- only `drive edit` does, unchanged from Phase 2. Wiring --lease into SheetsWrite/SheetsStructure/SheetsDelete/SheetsProtection/DocsWrite is the remainder of Phase 3.
CoverageTotal: 97.21% ⚪ 0.01 pp vs Comparing
Patch coveragePatch: 99.03% (611/617 new lines covered)
Uncovered new lines (6)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 6 lines gained coverage on unchanged code. Indirect changes
|
newhoggy
added a commit
that referenced
this pull request
Sep 11, 2026
Cover the paths the PR #1669 coverage report listed as uncovered new lines, all failure branches or print arms added by the previous two commits: - `active_account_lease_backup_folder_id`: unconfigured account, a configured account with no folder set, and one with a folder set. - `drive lease acquire`: a `files.copy` failure for a native document, a failed post-backup metadata re-fetch, and a post-backup fetch that omits `version` — each must report `Failed` and write no ledger row. The pre-auth `files.get` is mounted once (priority 1) so the second, post-backup call falls through to the failing responder, the same shape the existing TOCTOU regression test uses. - `drive edit`: a failed pre-lease metadata re-fetch reports `Failed` with no media PATCH issued. - `print_result`'s `DriveCopy` arm, `print_report`'s "not required" lease line, and `append_write_scope_hint`'s `CopyForBackup` hint. The remaining lines the report names in `acquire.rs` (682, 746, 787, 813-814) are `panic!` arms inside the tests themselves and are not reachable by design.
…e match Code review of #1664 found the staleness check comparing against a version snapshot taken before the permission gate's own files.get calls, and drive lease acquire recording a version/modifiedTime taken before the (up to 120s) Touch ID prompt and the backup itself — both re-fetch fresh metadata at the right point now. decided_rule_requires_lease also matched rules by folder/depth alone, letting an unrelated rule for a different operation override an explicit require_lease opt-out; it now filters by the operation being checked. drive permissions check now surfaces the requires_lease it already computed instead of dropping it. Also sanitizes the Drive-controlled file name reaching the Touch ID prompt and the printed backup path, moves the blocking authenticate() call onto block_in_place so it can't stall a shared multi-thread runtime, and reuses the existing hex-encoding helper instead of a second implementation. Corrects ADR-0080 to stop describing a CLI-flag/env/settings resolution and global lease settings that haven't landed yet.
Cover the paths the PR #1669 coverage report listed as uncovered new lines, all failure branches or print arms added by the previous two commits: - `active_account_lease_backup_folder_id`: unconfigured account, a configured account with no folder set, and one with a folder set. - `drive lease acquire`: a `files.copy` failure for a native document, a failed post-backup metadata re-fetch, and a post-backup fetch that omits `version` — each must report `Failed` and write no ledger row. The pre-auth `files.get` is mounted once (priority 1) so the second, post-backup call falls through to the failing responder, the same shape the existing TOCTOU regression test uses. - `drive edit`: a failed pre-lease metadata re-fetch reports `Failed` with no media PATCH issued. - `print_result`'s `DriveCopy` arm, `print_report`'s "not required" lease line, and `append_write_scope_hint`'s `CopyForBackup` hint. The remaining lines the report names in `acquire.rs` (682, 746, 787, 813-814) are `panic!` arms inside the tests themselves and are not reachable by design.
newhoggy
force-pushed
the
issue-1664-drive-write-lease
branch
from
September 11, 2026 11:12
0b363a7 to
f467545
Compare
6 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
First slice of Phase 3 of issue #1664 (ADR-0080), landing after Phase 2 (#1667):
drive lease acquirenow backs up native documents (Sheets/Docs/Slides) too, not just binary files.files.copyinto a new per-accountlease_backup_folder_idsetting — restorable by a human in the Drive UI even without this tool.LeaseRecord's backup field is now theLeaseBackupenum (Bytes { path, sha256, size }/DriveCopy { file_id }) instead of the three flat, always-required fields Phase 2 shipped — the two forms are mutually exclusive by construction, which independently-optional fields would only have documented.FilesApi::copy(files.copy) with its ownCopyForBackupscope hint (always the unrestricteddrivescope, since leasing exists precisely to back up files this tool did not create).Sheets/Docs write verbs still don't require a lease — only
drive editdoes, unchanged from Phase 2. Wiring--leaseintoSheetsWrite/SheetsStructure/SheetsDelete/SheetsProtection/DocsWriteis the remainder of Phase 3, in a follow-up PR (5 separate insertion points, one per engine, already scoped).A real test-isolation gap caught along the way
AcquireCommand::executenow calls the new settings-reading helper (active_account_lease_backup_folder_id), which meant two existing wiremock-only CLI tests started implicitly depending on this machine's realsettings.jsoninstead of being fully isolated. Fixed by giving both the sameEnvGuard::clear_credentials()isolationdrive::test_support's other settings-reading tests already use.Test plan
cargo test --lib -- drive::— 1253 passed (18 new)cargo clippy --all-targets --features mcp -- -D warnings— cleancargo fmt --check— cleancargo doc --no-deps --document-private-items— exits 0 (verified locally after the previous PR's cross-platform doc-link failures)cargo test --lib— 9263 passedcargo insta test— no snapshot drift (no CLI flags changed)docs/drive.md,CHANGELOG.md, ADR-0080's landed-so-far note updated