Skip to content

Conversation

@intval
Copy link

@intval intval commented Jun 15, 2025

This addresses issuen #58

@taylorwilsdon
Copy link
Owner

Thanks for this! Will build and take a look today

Copy link
Owner

@taylorwilsdon taylorwilsdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much appreciate the PR, code looks clean and high quality in general. Have a few todos as I test now, but will get this merged today.

.vscode/mcp.json Outdated
"google_workspace": {
"type": "stdio",
"command": "cmd",
"args": ["/c", "cd", "google_workspace_mcp", "&&", "uv", "run", "main.py", "--single-user", "--tools", "docs"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to your windows installation so we'll need to rewrite - thinking maybe we can just use uvx workspace-mcp for general plug & play

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with a small caveat,
cmd /c is not the cd c:/, it's actually an argument to cmd to start a command, a bit like bash -c 'echo hi'
cmd is windows specific, but i've comitted it intentionally, because it took me a while to figure out how to actually run it on windows. I'd like to keep this info somewhere, though we can also move it readme and just keep here everything from run onwards

uv.lock Outdated
{ url = "https://files.pythonhosted.org/packages/44/96/392abd49b094d30b91d9fbda6a69519e95802250b777841cf3bda8fe136c/charset_normalizer-3.4.2-cp313-cp313-win32.whl", hash = "sha256:aaeeb6a479c7667fbe1099af9617c83aaca22182d6cf8c53966491a0f1b7ffb7", size = 98064, upload_time = "2025-05-02T08:33:17.06Z" },
{ url = "https://files.pythonhosted.org/packages/e9/b0/0200da600134e001d91851ddc797809e2fe0ea72de90e09bec5a2fbdaccb/charset_normalizer-3.4.2-cp313-cp313-win_amd64.whl", hash = "sha256:aa6af9e7d59f9c12b33ae4e9450619cf2488e2bbe9b44030905877f0b2324980", size = 105641, upload_time = "2025-05-02T08:33:18.753Z" },
{ url = "https://files.pythonhosted.org/packages/20/94/c5790835a017658cbfabd07f3bfb549140c3ac458cfc196323996b10095a/charset_normalizer-3.4.2-py3-none-any.whl", hash = "sha256:7f56930ab0abd1c45cd15be65cc741c28b1c9a34876ce8c17a2fa107810c0af0", size = 52626, upload_time = "2025-05-02T08:34:40.053Z" },
sdist = { url = "https://files.pythonhosted.org/packages/e4/33/89c2ced2b67d1c2a61c19c6751aa8902d46ce3dacb23600a283619f5a12d/charset_normalizer-3.4.2.tar.gz", hash = "sha256:5baececa9ecba31eff645232d59845c07aa030f0c81ee70184a90d35099a0e63", size = 126367, upload-time = "2025-05-02T08:34:42.01Z" }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version of uv are you using? I've never seen upload-time before, only upload_time (introduced in v0.6.15) or nothing - curious what else is floating around out there

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uv --version
uv 0.7.4 (6fbcd09b5 2025-05-15)

I'm not coming from the python world, so this all seemed just legit ;)

@taylorwilsdon
Copy link
Owner

taylorwilsdon commented Jun 15, 2025

Hey @intval - I tested it out, looks pretty good to me! Do you imagine folks use the copy functionality without the templated aspect and to leave that standalone, or should we instruct the LLM to trigger the replace text mechanism in step?
image
image
image

FWIW - the current state can already accomplish the copy without the new function:
image
So I'd say we either link the copy & replace bits together for a chained action or drop the copy_google_doc logic since we can accomplish that without it. Let me know what you think - t

@taylorwilsdon taylorwilsdon requested a review from Copilot June 16, 2025 19:32

This comment was marked as outdated.

@taylorwilsdon taylorwilsdon requested a review from Copilot June 16, 2025 19:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces two new Google Docs tools: one for copying a document from a template and another for performing bulk text replacements within a document, along with added support for service account credentials in authentication.

  • Added copy_google_doc and replace_text_in_google_doc functions in docs_tools.py
  • Enhanced Google authentication to detect and use service account credentials based on file naming conventions
  • Updated VS Code configuration files for local development and debugging

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

File Description
gdocs/docs_tools.py New tools for copying and updating Google Docs with improved logging and error handling.
auth/google_auth.py Support for service account credentials added; OAuth2 branch logging improved.
.vscode/mcp.json Server configuration for Google Workspace MCP added.
.vscode/launch.json Launch configuration for debug mode added.
Comments suppressed due to low confidence (1)

gdocs/docs_tools.py:304

  • Consider using a more specific exception type rather than a generic Exception to provide clearer error handling and API design.
raise Exception("Template document or parent folder not found. Check the IDs. HTTP Status: 404")

Comment on lines +64 to +75
if "iam.gserviceaccount.com" in filename:
logger.info(f"[single-user] Found service account file: {filepath}")
try:
credentials = service_account.Credentials.from_service_account_file(filepath,scopes=SCOPES)
logger.info(f"[single-user] Successfully loaded service account credentials from {filepath}")

# Force refresh to get a token, since by default it's not set and the library considers then credentials to be invalid
credentials.refresh(Request())
return credentials
except Exception as e:
logger.warning(f"[single-user] Error loading service account credentials from {filepath}: {e}")
continue
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying solely on the filename to identify service account credentials might be fragile; consider validating the file contents for expected service account keys.

Suggested change
if "iam.gserviceaccount.com" in filename:
logger.info(f"[single-user] Found service account file: {filepath}")
try:
credentials = service_account.Credentials.from_service_account_file(filepath,scopes=SCOPES)
logger.info(f"[single-user] Successfully loaded service account credentials from {filepath}")
# Force refresh to get a token, since by default it's not set and the library considers then credentials to be invalid
credentials.refresh(Request())
return credentials
except Exception as e:
logger.warning(f"[single-user] Error loading service account credentials from {filepath}: {e}")
continue
try:
with open(filepath, 'r') as f:
creds_data = json.load(f)
# Validate required keys for service account credentials
if all(key in creds_data for key in ["client_email", "private_key", "project_id"]):
logger.info(f"[single-user] Found valid service account file: {filepath}")
try:
credentials = service_account.Credentials.from_service_account_file(filepath, scopes=SCOPES)
logger.info(f"[single-user] Successfully loaded service account credentials from {filepath}")
# Force refresh to get a token, since by default it's not set and the library considers the credentials to be invalid
credentials.refresh(Request())
return credentials
except Exception as e:
logger.warning(f"[single-user] Error loading service account credentials from {filepath}: {e}")
continue
else:
logger.warning(f"[single-user] File {filepath} does not contain valid service account keys.")
continue
except (IOError, json.JSONDecodeError) as e:
logger.warning(f"[single-user] Error reading or parsing file {filepath}: {e}")
continue

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually correct, i pondered about it for a moment and decided that it's a tradeoff between this and reading the file from disk twice, and decided that for single user configuration it will do.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @intval - hm, when I create a service account key it does stub out an email with iam.gserviceaccount.com in it, but the actual generated keyfile name is open-webui-444821-b145f5cb467e.json which would not match this check. A user would have to manually rename the key for this logic to work. Thoughts on splitting the service account PR out from the copy & template bits so we can get that merged and figure out the best approach for service accounts separately?

@taylorwilsdon
Copy link
Owner

Hey @intval - just wanted to ping one more time to see if ya had any thoughts on the above before moving forward here!

@intval
Copy link
Author

intval commented Jun 18, 2025

Ad merging copy functionality with the templated:

I think it's better to keep them separate.
Copying a document seems like a valid operation in it's own stead
I ended up using it on it's own as well.

I also think that templating can be an op on it's own, since it may be a multi step approach,
e.g. first, replace this bunch, show the user, then replace another bunch of variables.

I suppose in order to support both agents and humans, it would make sense to add the third method, that combines the two and keep the existing two as separate instances. This will allow more granular agentic flow, like the one i use:
a. read document content to identify placeholders
b. copy the document
c. maybe append to it more text as append_text
b. obtain via rag values to insert for placeholders read by (a) or freshly added by (c)
d. implement placeholder replacement

Comment on lines +64 to +75
if "iam.gserviceaccount.com" in filename:
logger.info(f"[single-user] Found service account file: {filepath}")
try:
credentials = service_account.Credentials.from_service_account_file(filepath,scopes=SCOPES)
logger.info(f"[single-user] Successfully loaded service account credentials from {filepath}")

# Force refresh to get a token, since by default it's not set and the library considers then credentials to be invalid
credentials.refresh(Request())
return credentials
except Exception as e:
logger.warning(f"[single-user] Error loading service account credentials from {filepath}: {e}")
continue
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @intval - hm, when I create a service account key it does stub out an email with iam.gserviceaccount.com in it, but the actual generated keyfile name is open-webui-444821-b145f5cb467e.json which would not match this check. A user would have to manually rename the key for this logic to work. Thoughts on splitting the service account PR out from the copy & template bits so we can get that merged and figure out the best approach for service accounts separately?

@taylorwilsdon taylorwilsdon added the enhancement New feature or request label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants