fix: close release review findings

.changeset/release-review-fixes.md

@@ -0,0 +1,5 @@
+---
+"@googleworkspace/cli": patch
+---
+
+Harden credential fallback and preserve document tab context when resolving comment anchors.

crates/google-workspace-cli/src/auth.rs

@@ -352,6 +352,23 @@ async fn load_credentials_with_loader(
     enc_path: &std::path::Path,
     default_path: &std::path::Path,
     load_encrypted: impl FnOnce(&std::path::Path) -> anyhow::Result<String>,
+) -> anyhow::Result<Credential> {
+    load_credentials_with_loaders(
+        env_file,
+        enc_path,
+        default_path,
+        |path| std::fs::symlink_metadata(path),
+        load_encrypted,
+    )
+    .await
+}
+
+async fn load_credentials_with_loaders(
+    env_file: Option<&str>,
+    enc_path: &std::path::Path,
+    default_path: &std::path::Path,
+    metadata: impl FnOnce(&std::path::Path) -> std::io::Result<std::fs::Metadata>,
+    load_encrypted: impl FnOnce(&std::path::Path) -> anyhow::Result<String>,
 ) -> anyhow::Result<Credential> {
     // 1. Explicit env var — plaintext file (User or Service Account)
     if let Some(path) = env_file {
@@ -367,8 +384,19 @@ async fn load_credentials_with_loader(
         );
     }
 
-    // 2. Encrypted credentials
-    if enc_path.exists() {
+    // 2. Encrypted credentials. Inspect symlink metadata so dangling symlinks
+    // and inaccessible paths are treated as credential failures, not absence.
+    let encrypted_present = match metadata(enc_path) {
+        Ok(_) => true,
+        Err(error) if error.kind() == std::io::ErrorKind::NotFound => false,
+        Err(_) => {
+            anyhow::bail!(
+                "Failed to inspect saved credentials at {}. Credentials and token caches have been preserved; no fallback credentials were used.",
+                crate::output::sanitize_for_terminal(&enc_path.display().to_string())
+            )
+        }
+    };
+    if encrypted_present {
         // A read, decryption, or keyring failure does not mean the files are
         // disposable. Stop here so a retry cannot silently select another account.
         // Do not render backend error details, which may contain sensitive data.
@@ -910,6 +938,45 @@ mod tests {
         }
     }
 
+    #[cfg(unix)]
+    #[tokio::test]
+    #[serial_test::serial]
+    async fn test_load_credentials_dangling_encrypted_symlink_blocks_fallback() {
+        use std::os::unix::fs::symlink;
+
+        let dir = tempfile::tempdir().unwrap();
+        let enc_path = dir.path().join("credentials.enc");
+        let fallback_path = dir.path().join("credentials.json");
+        let fallback_json = r#"{"client_id":"fallback","client_secret":"secret","refresh_token":"refresh","type":"authorized_user"}"#;
+        std::fs::write(&fallback_path, fallback_json).unwrap();
+        symlink(dir.path().join("missing-encrypted"), &enc_path).unwrap();
+
+        let error = load_credentials_inner(None, &enc_path, &fallback_path)
+            .await
+            .expect_err("dangling encrypted credential symlink must block fallback");
+        assert!(error.to_string().contains("saved credentials"));
+    }
+
+    #[tokio::test]
+    async fn test_load_credentials_metadata_failure_blocks_fallback() {
+        let dir = tempfile::tempdir().unwrap();
+        let enc_path = dir.path().join("credentials.enc");
+        let fallback_path = dir.path().join("credentials.json");
+        let error = load_credentials_with_loaders(
+            None,
+            &enc_path,
+            &fallback_path,
+            |_| Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)),
+            |_| Ok(String::new()),
+        )
+        .await
+        .expect_err("credential metadata failures must block fallback");
+        assert!(error
+            .to_string()
+            .contains("Failed to inspect saved credentials"));
+        assert!(!error.to_string().contains("No credentials found"));
+    }
+
     #[tokio::test]
     #[serial_test::serial]
     async fn test_load_credentials_keyring_failure_preserves_files_and_blocks_adc() {

crates/google-workspace-cli/src/helpers/docs/read.rs

@@ -306,33 +306,48 @@ pub(super) fn normalize_with_comments(document: &Value) -> Result<Value, GwsErro
 
 fn collect_comment_anchors(document: &Value) -> std::collections::HashMap<String, Vec<Value>> {
     let mut anchors = std::collections::HashMap::new();
-    collect_comment_anchors_recursive(document, &mut anchors);
+    collect_comment_anchors_recursive(document, None, &mut anchors);
     anchors
 }
 
 fn collect_comment_anchors_recursive(
     value: &Value,
+    current_tab_id: Option<String>,
     anchors: &mut std::collections::HashMap<String, Vec<Value>>,
 ) {
     match value {
         Value::Object(object) => {
+            let tab_id = object
+                .get("tabProperties")
+                .and_then(|properties| properties.get("tabId"))
+                .or_else(|| object.get("tabId"))
+                .and_then(Value::as_str)
+                .map(String::from)
+                .or(current_tab_id);
             if let Some(comment_anchors) = object.get("commentAnchors").and_then(Value::as_object) {
                 for (id, anchor) in comment_anchors {
-                    let ranges = anchor
+                    let mut ranges = anchor
                         .get("ranges")
                         .and_then(Value::as_array)
                         .cloned()
                         .unwrap_or_default();
+                    if let Some(tab_id) = tab_id.as_deref() {
+                        for range in &mut ranges {
+                            if range.get("tabId").is_none() {
+                                range["tabId"] = Value::String(tab_id.to_string());
+                            }
+                        }
+                    }
                     anchors.insert(id.clone(), ranges);
                 }
             }
             for child in object.values() {
-                collect_comment_anchors_recursive(child, anchors);
+                collect_comment_anchors_recursive(child, tab_id.clone(), anchors);
             }
         }
         Value::Array(array) => {
             for child in array {
-                collect_comment_anchors_recursive(child, anchors);
+                collect_comment_anchors_recursive(child, current_tab_id.clone(), anchors);
             }
         }
         _ => {}

crates/google-workspace-cli/src/helpers/docs/read_tests.rs

@@ -215,6 +215,28 @@ fn comment_anchor_resolution_respects_document_segments() {
     assert_eq!(output["comments"][1]["referencedText"], json!(["header"]));
 }
 
+#[test]
+fn comment_anchor_without_tab_id_uses_enclosing_tab() {
+    let mut input = legacy();
+    input["comments"] = json!([{"commentId": "c2", "anchorId": "a2"}]);
+    input["tabs"] = json!([
+        {"tabProperties": {"tabId": "tab-1"}, "documentTab": {"body": {"content": []}}},
+        {"tabProperties": {"tabId": "tab-2"}, "documentTab": {
+            "body": {"content": [{
+                "startIndex": 1, "endIndex": 7,
+                "paragraph": {"elements": [{
+                    "startIndex": 1, "endIndex": 7,
+                    "textRun": {"content": "second"}
+                }]}
+            }]},
+            "commentAnchors": {"a2": {"ranges": [{"startIndex": 1, "endIndex": 7}]}}
+        }}
+    ]);
+
+    let output = read::normalize_with_comments(&input).unwrap();
+    assert_eq!(output["comments"][0]["referencedText"], json!(["second"]));
+}
+
 #[test]
 fn request_rejects_partial_masks_lossy_views_and_parameter_bypasses() {
     for params in [