Add update_tab_from_markdown tool and fix addDocumentTab response key - #727
Merged
Conversation
Add two tests that exercise tab_id through every supported markdown construct - heading, paragraph, bold span, list, and fenced code - and verify the tabId field appears on every location and range when tab_id is provided, and is absent when it is omitted. All existing helpers (including the inline createParagraphBullets dict in the list branch and the inline updateParagraphStyle dict in the blockquote branch) were already threading tab_id correctly; no writer changes were needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verifies against HONOUR-HEALTH-01 A01 doc that: - OAuth against a user-provided GCP client - Docs API addDocumentTab creates a tab - Markdown writer emits tab-targeted batchUpdate requests - batchUpdate applies the writer's output into the tab - includeTabsContent doc.get returns the populated tab body Spike revealed the request field is addDocumentTab not createDocumentTab (the fork's insert_doc_tab reads the reply from the wrong key - fix in a follow-up commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Docs batchUpdate response for an addDocumentTab request comes back under the key "addDocumentTab" matching the request field name, not "createDocumentTab". Two spots in the fork silently extracted tab_id=None because of this mismatch: - gdocs/docs_tools.py insert_doc_tab (the MCP tool) - gdocs/managers/batch_operation_manager.py _extract_created_tabs Both now check for "addDocumentTab" first and fall back to "createDocumentTab" in case the API ever surfaces the legacy name. Discovered via spike testing in commit b374139 against a real Google Doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Google Docs API rejects deleteContentRange ranges that include the trailing newline terminating a segment body. The previous code passed endIndex equal to the last element's endIndex, which is one past the final newline for a fresh tab. The API returns "The range cannot include the newline character at the end of the segment." Fix by using tab_end - 1 as endIndex, and raising the skip threshold from tab_end > 1 to tab_end > 2 so a tab containing only its mandatory terminating newline is correctly treated as empty. Discovered by the integration test added in the next commit.
Two pytest-asyncio tests verify end-to-end behaviour of the update_tab_from_markdown MCP tool: 1. Populate a fresh tab with markdown, assert the result payload and that the tab now contains multiple structural elements and a substantive character count. 2. Populate a tab, then call update_tab_from_markdown again with different markdown and replace_existing=True; assert the content length changed, confirming the replace path clears old content. Both tests create a scratch tab via direct Docs API addDocumentTab (sidesteps insert_doc_tab string parsing), invoke the unwrapped MCP tool, and clean up with deleteTab. Authentication reuses the OAuth token cached by scripts/spike/spike_tab_operations.py at ~/.workspace-mcp/spike_token.json. The tests skip cleanly when GOOGLE_CLIENT_SECRET_PATH, USER_GOOGLE_EMAIL, INTEGRATION_TEST_DOC_ID, or the cached token are not present. Also adds the 'integration' pytest marker so the non-integration suite runs unchanged with `pytest -m "not integration"`.
Google Docs renders adjacent paragraphs tightly stacked when written via the batchUpdate API. Reference "paste from markdown" rendering shows a visible blank line between paragraphs. To match, emit an extra blank paragraph after each top-level block (heading, paragraph, fence, blockquote, list). List items and blockquote-internal paragraphs stay tight - spacers only emit at the top level. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pushes the real blog-article.md (15,891 chars) via update_tab_from_markdown into a scratch tab on the A01 condition doc. User visually compared the rendered output against the paste-from-markdown reference tab and confirmed content identity. The only noted difference (paragraph spacing) has been fixed in commit e25270c by emitting blank spacer paragraphs between top-level markdown blocks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates three Google Docs tab tools into a single action-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Tool as "manage_doc_tab\n(tool wrapper)"
participant Parser as "markdown_to_docs_requests\n(MarkdownIt)"
participant DocsAPI as "Google Docs API\n(get & batchUpdate)"
participant BOM as "BatchOperationManager"
User->>Tool: call manage_doc_tab(action="populate_from_markdown", markdown_text, tab_id?)
Tool->>DocsAPI: get(documentId, includeTabsContent=true)
DocsAPI-->>Tool: document with tabs
Tool->>Tool: _find_tab_end_index(tab_id)
alt markdown provided
Tool->>Parser: markdown_to_docs_requests(markdown_text, tab_id, start_index)
Parser-->>Tool: list of batchUpdate requests
Tool->>DocsAPI: batchUpdate(requests)
DocsAPI-->>BOM: batchUpdate replies
BOM-->>Tool: extract_created_tabs / normalize replies
Tool-->>User: return {success, action, tab_id, requests_applied, link}
else no markdown
Tool-->>User: return {success, action, tab_id, requests_applied:0}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/gdocs/test_docs_markdown_writer.py (2)
236-237: Passencoding="utf-8"toread_text().Relying on the platform default encoding can flake on Windows (cp1252) or locales without
LANGset. Since the fixture is markdown that almost certainly contains non-ASCII, pin the encoding.🧹 Suggested diff
- md = md_path.read_text() + md = md_path.read_text(encoding="utf-8")(Both test functions.)
Also applies to: 251-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gdocs/test_docs_markdown_writer.py` around lines 236 - 237, The test reads the markdown fixtures with md_path.read_text() without specifying encoding, which can cause flaky behavior on non-UTF-8 platforms; update both places where md_path.read_text() is called (the two occurrences in the test file) to pass encoding="utf-8" so the fixture is read deterministically (i.e., replace md_path.read_text() with md_path.read_text(encoding="utf-8")).
107-116: Tighten thefontFamilyassertion — the writer only emitsCourier New.
_build_text_styleingdocs/docs_markdown_writer.py(Line 138, 267) hardcodesfontFamily: "Courier New". Accepting"Roboto Mono"or"Consolas"as alternatives here means a future silent change of the hardcoded font would go undetected. Tighten to the exact value the code emits.🧹 Suggested diff
- assert ts.get("weightedFontFamily", {}).get("fontFamily") in ( - "Courier New", - "Roboto Mono", - "Consolas", - ) + assert ts.get("weightedFontFamily", {}).get("fontFamily") == "Courier New"(Apply in both
test_inline_code_emits_monospace_styleandtest_fenced_code_block_emits_monospace_style.)Also applies to: 167-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gdocs/test_docs_markdown_writer.py` around lines 107 - 116, The tests accept multiple monospace fonts but the writer's _build_text_style function hardcodes fontFamily to "Courier New", so update the assertions in test_inline_code_emits_monospace_style and test_fenced_code_block_emits_monospace_style to expect exactly "Courier New" (use ts.get("weightedFontFamily", {}).get("fontFamily") == "Courier New" or an assertion that the value equals "Courier New" instead of allowing "Roboto Mono" or "Consolas").gdocs/docs_markdown_writer.py (1)
126-147: Fenced code blocks emit one more trailing newline than other blocks.
text = content_with_nl + "\n"(Line 129-130) already bakes in one blank line, then Line 144 appends another spacer "\n". Every other block branch (paragraph, heading, list, blockquote) only emits a single spacer. Net result: fenced code has two trailing blank paragraphs and the existing unit test attests/gdocs/test_docs_markdown_writer.pyline 174 pins this behavior ("def foo():\n return 42\n\n"). If the double gap is intentional, a one-line comment here would help; otherwise drop thetext += "\n"on Line 130 so code blocks match the rest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_markdown_writer.py` around lines 126 - 147, Fenced code blocks currently append an extra blank line because text is set to content + "\n" and then a separate spacer "\n" is inserted later; update the fenced-block branch (where tok.type == "fence", the local variable text and the subsequent _build_insert_text(cursor[0], "\n", tab_id) spacer) to emit only a single trailing blank paragraph — remove the inner text += "\n" so text is just content (or content if it already ends with "\n") and let the later spacer provide the single blank line, or alternatively remove the final spacer instead if you prefer keeping the inner newline; adjust tests if the intent is to change the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gdocs/docs_markdown_writer.py`:
- Around line 1-40: The module docstring claims "CommonMark+GFM" but
markdown_to_docs_requests creates the parser with MarkdownIt("commonmark"),
which does not enable GFM features; either switch the parser to the GFM preset
(replace the MarkdownIt("commonmark") call in markdown_to_docs_requests with the
GFM preset, e.g., MarkdownIt("gfm") or equivalent plugin-enabled factory so
tables/strikethrough/linkify work) or update the top-level module docstring to
state "CommonMark only" so documentation matches behavior.
In `@gdocs/docs_tools.py`:
- Around line 2500-2505: The docstring for the function that accepts tab_id
incorrectly instructs callers to obtain the tab ID from list_doc_tabs (which
doesn't exist); update the help text to point to inspect_doc_structure instead.
Edit the Args section where tab_id is described so it reads something like
"tab_id: Target tab ID (obtain from insert_doc_tab or
inspect_doc_structure)"—make this change in the docstring associated with the
function that mentions markdown_text, replace_existing and tab_id (the
insert_doc_tab-related docstring) so generated help/schema references the
correct helper inspect_doc_structure.
- Around line 2482-2485: The docstring for update_tab_from_markdown incorrectly
references list_doc_tabs; update the documentation to reference
inspect_doc_structure (which is the function that provides tab information) and
adjust any example/description text to mention inspect_doc_structure by name; do
not change the decorator usage on update_tab_from_markdown — leave the bare
`@server.tool`() decorator as-is.
In `@scripts/spike/spike_tab_operations.py`:
- Line 44: Replace the hard-coded TARGET_DOC_ID constant by reading it from an
environment variable (use the same pattern/name as
tests/integration/test_update_tab_from_markdown.py, e.g.
INTEGRATION_TEST_DOC_ID) so the script works for any user; update the
TARGET_DOC_ID reference in spike_tab_operations.py to fetch
os.environ.get("INTEGRATION_TEST_DOC_ID") (optionally with a safe fallback or a
clear error if not set) and ensure any doc-comment is removed or generalized.
In `@tests/integration/test_update_tab_from_markdown.py`:
- Around line 68-81: The docs_service fixture currently only checks
INTEGRATION_TEST_DOC_ID and credentials but not USER_GOOGLE_EMAIL, which later
leads to KeyError when code does os.environ["USER_GOOGLE_EMAIL"]; update the
docs_service fixture to also check os.environ.get("USER_GOOGLE_EMAIL") and call
pytest.skip with a clear message if it's not set so tests that reference
os.environ["USER_GOOGLE_EMAIL"] (e.g., later uses around lines that access that
env) are skipped instead of erroring.
---
Nitpick comments:
In `@gdocs/docs_markdown_writer.py`:
- Around line 126-147: Fenced code blocks currently append an extra blank line
because text is set to content + "\n" and then a separate spacer "\n" is
inserted later; update the fenced-block branch (where tok.type == "fence", the
local variable text and the subsequent _build_insert_text(cursor[0], "\n",
tab_id) spacer) to emit only a single trailing blank paragraph — remove the
inner text += "\n" so text is just content (or content if it already ends with
"\n") and let the later spacer provide the single blank line, or alternatively
remove the final spacer instead if you prefer keeping the inner newline; adjust
tests if the intent is to change the current behavior.
In `@tests/gdocs/test_docs_markdown_writer.py`:
- Around line 236-237: The test reads the markdown fixtures with
md_path.read_text() without specifying encoding, which can cause flaky behavior
on non-UTF-8 platforms; update both places where md_path.read_text() is called
(the two occurrences in the test file) to pass encoding="utf-8" so the fixture
is read deterministically (i.e., replace md_path.read_text() with
md_path.read_text(encoding="utf-8")).
- Around line 107-116: The tests accept multiple monospace fonts but the
writer's _build_text_style function hardcodes fontFamily to "Courier New", so
update the assertions in test_inline_code_emits_monospace_style and
test_fenced_code_block_emits_monospace_style to expect exactly "Courier New"
(use ts.get("weightedFontFamily", {}).get("fontFamily") == "Courier New" or an
assertion that the value equals "Courier New" instead of allowing "Roboto Mono"
or "Consolas").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6471a35e-627e-41ca-8c73-8b4db2cd127e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
README.mdgdocs/docs_markdown_writer.pygdocs/docs_tools.pygdocs/managers/batch_operation_manager.pypyproject.tomlscripts/spike/acceptance_honour_health_a01.pyscripts/spike/spike_tab_operations.pyskills/managing-google-workspace/references/docs.mdtests/gdocs/fixtures/sample_blog_article.mdtests/gdocs/test_docs_markdown_writer.pytests/gdocs/test_insert_doc_tab_response.pytests/integration/__init__.pytests/integration/test_update_tab_from_markdown.py
Comment on lines
+38
to
+44
| TOKEN_CACHE = pathlib.Path.home() / ".workspace-mcp" / "spike_token.json" | ||
| A01_DOC_ID = "1UyL1dL6GBztnVGpLQ5H0EQ8Cfh_k6mjiRQ56MCsJnb0" | ||
| BLOG_ARTICLE_MD = pathlib.Path( | ||
| "/Users/juliandickie/Documents/GitHub/ahpra-writing-research-cc/" | ||
| "clients/HONOUR-HEALTH-01/content/A01-low-back-pain/" | ||
| "honour-health-A01-low-back-pain-blog-article.md" | ||
| ) |
Contributor
There was a problem hiding this comment.
Make this acceptance spike portable and self-bootstrapping.
BLOG_ARTICLE_MD is pinned to one developer machine, and get_credentials() requires ~/.workspace-mcp/spike_token.json to already exist even though the module docs say GOOGLE_CLIENT_SECRET_PATH is enough. As committed, a fresh checkout cannot reproduce the acceptance run. Please switch the markdown source/doc ID/token cache to repo-relative or CLI/env inputs, and either bootstrap credentials when the cache is missing or document that an existing cache is mandatory. The repo already has tests/gdocs/fixtures/sample_blog_article.md you can reuse here.
Also applies to: 47-52
…ewline
- gdocs/docs_markdown_writer.py - module docstring accurately claimed
"CommonMark+GFM" but the parser is created with MarkdownIt("commonmark")
with no GFM preset or plugins enabled. Updated the docstring to say
"CommonMark" and explicitly list GFM features as not enabled.
- gdocs/docs_markdown_writer.py - fenced code blocks emitted one more
trailing newline than other top-level blocks. The fence branch baked
in an extra "\n" on top of the universal spacer paragraph, producing
two blank lines after every code block. Removed the redundant newline;
spacer now provides the single blank line matching every other block.
- gdocs/docs_tools.py - update_tab_from_markdown docstring referenced a
non-existent list_doc_tabs helper. Corrected to inspect_doc_structure
which is the real helper for enumerating tab IDs.
- tests/gdocs/test_docs_markdown_writer.py - fixture read_text() calls
now pass encoding="utf-8" so the test is deterministic on Windows and
locales where the default encoding is not UTF-8.
- tests/gdocs/test_docs_markdown_writer.py - monospace font assertions
tightened from "in (Courier New, Roboto Mono, Consolas)" to an exact
match against "Courier New" since that is what _build_text_style
actually emits. A future silent change of the font would now fail
the test instead of passing silently.
- tests/gdocs/test_docs_markdown_writer.py - updated fenced code block
test expectation to match the single-newline output from the code fix.
- tests/integration/test_update_tab_from_markdown.py - docs_service
fixture now skips when USER_GOOGLE_EMAIL is unset instead of letting
tests raise KeyError deep in the async MCP call. Message directs the
user to export the env var and re-run.
- scripts/spike/spike_tab_operations.py - TARGET_DOC_ID was hardcoded
to a Julian-specific condition doc. Changed to read from
INTEGRATION_TEST_DOC_ID env var (same convention as the integration
test) with a clear error if unset. Any contributor can now run the
spike against their own scratch Doc.
Full fork suite passes - 817 tests, 0 failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
gdocs/docs_tools.py (3)
2519-2548: Consider validating thattab_idexists before issuing the batch.When
replace_existing=Trueand the tab is missing,_find_tab_end_indexreturns0, the delete is silently skipped, and thenmarkdown_to_docs_requestsemitsinsertTextrequests that target a nonexistenttabId. The Docs API will reject the batch (via@handle_http_errors), but the error surface is opaque — callers get a generic API error rather than a clear "tab not found" message. A lightweight check after thedocuments().get(...)call (e.g., "was the tab present indoc.get('tabs')?") would make failure mode clearer and avoid the unnecessary network round-trip.Low-priority — the current behavior is not broken, just not maximally helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2519 - 2548, When replace_existing is true we call service.documents().get(..., includeTabsContent=True) and rely on _find_tab_end_index returning 0 for a missing tab, which lets markdown_to_docs_requests emit insertText requests that reference a nonexistent tab_id and yields an opaque API error; after fetching the document check that the requested tab_id actually exists in the returned doc (e.g. inspect doc.get('tabs') or similar) and if missing raise/return a clear "tab not found" error before building or sending the batch (affecting the logic around service.documents().get, _find_tab_end_index, and markdown_to_docs_requests so that we short-circuit with a descriptive error when the tab is absent).
2510-2510: Prefer top-level import formarkdown_to_docs_requests.The lazy/function-local import avoids a module-load-time dependency, but
gdocs.docs_markdown_writeris already an unconditional runtime dependency of this tool. Moving the import to the top of the file matches the rest ofdocs_tools.pyand surfaces ImportErrors at module load rather than on first invocation.♻️ Proposed refactor
@@ top-level imports from gdocs.docs_markdown import ( convert_doc_to_markdown, format_comments_inline, format_comments_appendix, parse_drive_comments, ) +from gdocs.docs_markdown_writer import markdown_to_docs_requests @@ update_tab_from_markdown body - from gdocs.docs_markdown_writer import markdown_to_docs_requests - logger.info(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` at line 2510, Move the function-local import of markdown_to_docs_requests into the module-level imports: remove "from gdocs.docs_markdown_writer import markdown_to_docs_requests" from inside the function where it's currently imported and add a top-level import alongside the other imports in gdocs/docs_tools.py so markdown_to_docs_requests is imported at module load (use the exact symbol name markdown_to_docs_requests and module gdocs.docs_markdown_writer to locate and update the import).
2485-2561: Change return type from baredicttodict[str, Any]or a typed model.The function currently returns
dictwithout type parameters. FastMCP cannot derive a precise JSON-Schema from an unparameterizeddict; usingdict[str, Any]or a TypedDict/Pydantic model will generate a proper schema that documents the response structure to clients.This aligns with the guideline: "Ensure new/modified FastMCP tools conform to MCP JSON-Schema generated by FastMCP (
@mcp.tool) and validate that tool signatures use only primitive or Pydantic types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2485 - 2561, update_tab_from_markdown currently types its return as plain dict; change the annotation to a parameterized type (e.g., dict[str, Any]) or define and use a TypedDict/Pydantic model for the response to allow FastMCP to generate a proper JSON schema. Update the function signature for update_tab_from_markdown to return dict[str, Any] (or replace with MyResponseTypedDict/MyResponseModel) and add the necessary import(s) (typing.Any or TypedDict/Pydantic BaseModel) and ensure the returned dict shape matches that type. Keep the runtime return values the same but make the static type reflect keys "success", "requests_applied", and "tab_id".tests/integration/test_update_tab_from_markdown.py (1)
160-188: Integration coverage is reasonable; consider stronger content assertions.
requests_applied >= 5,elements >= 4, andchars > 50are good lower-bound smoke checks, but they won't catch silent regressions where the writer emits the wrong content (e.g., bold marker residue, dropped list items, or swapped paragraphs). A cheap upgrade is to fetch the tab text andassert "Integration Test - Round 1" in textor check that each list item string is present. Optional — current assertions are already a strong signal given the opt-in nature of the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_update_tab_from_markdown.py` around lines 160 - 188, The test test_update_tab_from_markdown_populates_empty_tab currently only checks counts; enhance it to assert specific content so regressions that change text shape are caught: after calling fn (update_tab_from_markdown) and before cleanup, fetch the tab's plain text (using the existing _count_tab_content helper or a small helper to read text from service/document_id/tab_id) and assert that key strings from SAMPLE_MARKDOWN_FIRST (e.g., the expected heading "Integration Test - Round 1" and each list item text) appear in the fetched text; keep the existing numeric asserts but add those containment assertions to verify actual rendered content.gdocs/docs_markdown_writer.py (1)
235-324: Inline renderer looks correct; stack tuple shape could be tightened.Bold/italic/code/link handling with the open/close stack correctly supports nesting (e.g.,
**bold *italic***) and the reverse-index scan pops the most recent matching opener, which is the right behavior for well-formed markdown. The mixed 2-tuple/3-tuple stack entries work but are a readability nit; a small dataclass or a consistent(style_name, start_local_pos, payload_or_none)shape would be easier to follow. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_markdown_writer.py` around lines 235 - 324, The _render_inline_with_styles function uses a mixed 2-tuple/3-tuple "stack" which makes matching logic harder to read; change stack to a consistent shape (e.g., tuples of (style_name: str, start_local: int, payload: Optional[str]) or a small dataclass InlineStyleEntry) and update all push/pop sites (places that append for "strong_open"/"em_open", "link_open", and the loop that finds matching openers for "strong_close"/"em_close" and "link_close") to use the new 3-field shape and access fields by name or fixed indexes; keep the reverse-index scan logic and _build_text_style calls unchanged apart from reading start_local and href/payload from the new unified entry.tests/gdocs/test_docs_markdown_writer.py (1)
1-295: Comprehensive unit coverage for the writer.The suite exercises indices, request shapes, preset names, style ranges (excluding trailing spacers),
tab_idthreading in both presence and absence, and fixture-based smoke coverage with monotonic-index assertions — a good mix of structural and semantic checks. The tight assertions on exacttexts == [...]sequences for the spacer/list-item/blockquote cases will catch most regressions in the cursor-advancement logic.One optional addition: a negative-path test for the
link_openbranch whenhrefis missing (see the related suggestion indocs_markdown_writer.py), which would lock in the desired fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gdocs/test_docs_markdown_writer.py` around lines 1 - 295, Add a negative-path unit test for markdown_to_docs_requests that covers the link_open branch when an inline link has no href (e.g. the markdown contains "[text]()", "[text]" without URL, or malformed link), asserting the function falls back to not emitting a link style (i.e., no updateTextStyle with a "link" entry) and that insertText still contains the link text plus proper spacer behavior; locate the test near other inline-style tests (e.g., next to test_link_emits_link_style) and reference markdown_to_docs_requests and the link_open handling in docs_markdown_writer.py so the test validates the desired fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gdocs/docs_markdown_writer.py`:
- Around line 79-130: The code in the list-handling block (using tok, tokens,
the matching-close scan and the inner k loop in gdocs/docs_markdown_writer.py)
fails to account for nested lists of a different type and thus silently drops
them; update the matching-close scan and the item iteration so any nested
list_open/close of either kind is tracked and nested list items are
flattened/processed: in the depth scan, treat tokens[j].type in
("bullet_list_open","ordered_list_open") as an increment and their corresponding
"_close" types as decrements (rather than only comparing to tok.type), and in
the inner loop that iterates items between i and j (the code that finds
"list_item_open" and locates its inline child before calling
_render_inline_with_styles and appending createParagraphBullets), search for
list_item_open tokens at any nesting depth (or search forward to the next
"inline" token inside a list_item_open) so nested ordered/bullet lists are
either flattened into the outer list or correctly captured for bullets created
by createParagraphBullets.
- Around line 300-322: When handling "link_open"/"link_close" tokens in
gdocs/docs_markdown_writer.py, guard against a missing href by checking the
extracted href before creating the link style request: in the branch that pops a
("link_open", ...) from stack and calls _build_text_style, skip (do not append
to style_requests) if href is None (or empty), otherwise call _build_text_style
as before; reference the stack entries, local_pos, base_index, tab_id and the
_build_text_style call so you only suppress invalid {"link": {"url": None}}
requests to the Docs API.
---
Nitpick comments:
In `@gdocs/docs_markdown_writer.py`:
- Around line 235-324: The _render_inline_with_styles function uses a mixed
2-tuple/3-tuple "stack" which makes matching logic harder to read; change stack
to a consistent shape (e.g., tuples of (style_name: str, start_local: int,
payload: Optional[str]) or a small dataclass InlineStyleEntry) and update all
push/pop sites (places that append for "strong_open"/"em_open", "link_open", and
the loop that finds matching openers for "strong_close"/"em_close" and
"link_close") to use the new 3-field shape and access fields by name or fixed
indexes; keep the reverse-index scan logic and _build_text_style calls unchanged
apart from reading start_local and href/payload from the new unified entry.
In `@gdocs/docs_tools.py`:
- Around line 2519-2548: When replace_existing is true we call
service.documents().get(..., includeTabsContent=True) and rely on
_find_tab_end_index returning 0 for a missing tab, which lets
markdown_to_docs_requests emit insertText requests that reference a nonexistent
tab_id and yields an opaque API error; after fetching the document check that
the requested tab_id actually exists in the returned doc (e.g. inspect
doc.get('tabs') or similar) and if missing raise/return a clear "tab not found"
error before building or sending the batch (affecting the logic around
service.documents().get, _find_tab_end_index, and markdown_to_docs_requests so
that we short-circuit with a descriptive error when the tab is absent).
- Line 2510: Move the function-local import of markdown_to_docs_requests into
the module-level imports: remove "from gdocs.docs_markdown_writer import
markdown_to_docs_requests" from inside the function where it's currently
imported and add a top-level import alongside the other imports in
gdocs/docs_tools.py so markdown_to_docs_requests is imported at module load (use
the exact symbol name markdown_to_docs_requests and module
gdocs.docs_markdown_writer to locate and update the import).
- Around line 2485-2561: update_tab_from_markdown currently types its return as
plain dict; change the annotation to a parameterized type (e.g., dict[str, Any])
or define and use a TypedDict/Pydantic model for the response to allow FastMCP
to generate a proper JSON schema. Update the function signature for
update_tab_from_markdown to return dict[str, Any] (or replace with
MyResponseTypedDict/MyResponseModel) and add the necessary import(s) (typing.Any
or TypedDict/Pydantic BaseModel) and ensure the returned dict shape matches that
type. Keep the runtime return values the same but make the static type reflect
keys "success", "requests_applied", and "tab_id".
In `@tests/gdocs/test_docs_markdown_writer.py`:
- Around line 1-295: Add a negative-path unit test for markdown_to_docs_requests
that covers the link_open branch when an inline link has no href (e.g. the
markdown contains "[text]()", "[text]" without URL, or malformed link),
asserting the function falls back to not emitting a link style (i.e., no
updateTextStyle with a "link" entry) and that insertText still contains the link
text plus proper spacer behavior; locate the test near other inline-style tests
(e.g., next to test_link_emits_link_style) and reference
markdown_to_docs_requests and the link_open handling in docs_markdown_writer.py
so the test validates the desired fallback behavior.
In `@tests/integration/test_update_tab_from_markdown.py`:
- Around line 160-188: The test
test_update_tab_from_markdown_populates_empty_tab currently only checks counts;
enhance it to assert specific content so regressions that change text shape are
caught: after calling fn (update_tab_from_markdown) and before cleanup, fetch
the tab's plain text (using the existing _count_tab_content helper or a small
helper to read text from service/document_id/tab_id) and assert that key strings
from SAMPLE_MARKDOWN_FIRST (e.g., the expected heading "Integration Test - Round
1" and each list item text) appear in the fetched text; keep the existing
numeric asserts but add those containment assertions to verify actual rendered
content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef487bd5-fa65-4915-aaf5-0e6b80c5bcd5
📒 Files selected for processing (5)
gdocs/docs_markdown_writer.pygdocs/docs_tools.pyscripts/spike/spike_tab_operations.pytests/gdocs/test_docs_markdown_writer.pytests/integration/test_update_tab_from_markdown.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/spike/spike_tab_operations.py
Comment on lines
+79
to
+130
| if tok.type in ("bullet_list_open", "ordered_list_open"): | ||
| preset = ( | ||
| "BULLET_DISC_CIRCLE_SQUARE" | ||
| if tok.type == "bullet_list_open" | ||
| else "NUMBERED_DECIMAL_ALPHA_ROMAN" | ||
| ) | ||
| list_start = cursor[0] | ||
| # Find the matching closing token | ||
| close_type = tok.type.replace("_open", "_close") | ||
| depth = 1 | ||
| j = i + 1 | ||
| while j < len(tokens) and depth > 0: | ||
| if tokens[j].type == tok.type: | ||
| depth += 1 | ||
| elif tokens[j].type == close_type: | ||
| depth -= 1 | ||
| if depth == 0: | ||
| break | ||
| j += 1 | ||
| # Iterate items between i and j | ||
| k = i + 1 | ||
| while k < j: | ||
| item = tokens[k] | ||
| if item.type == "list_item_open": | ||
| # Inner structure typically - list_item_open, paragraph_open, inline, paragraph_close, list_item_close | ||
| # Find the inline token within this list_item | ||
| if k + 2 < j and tokens[k + 2].type == "inline": | ||
| inline_tok = tokens[k + 2] | ||
| text, inline_styles = _render_inline_with_styles( | ||
| inline_tok.children or [], cursor[0], tab_id | ||
| ) | ||
| text += "\n" | ||
| requests.append(_build_insert_text(cursor[0], text, tab_id)) | ||
| cursor[0] += len(text) | ||
| requests.extend(inline_styles) | ||
| k += 1 | ||
| list_end = cursor[0] | ||
| # One createParagraphBullets covering the full list range | ||
| rng = {"startIndex": list_start, "endIndex": list_end} | ||
| if tab_id: | ||
| rng["tabId"] = tab_id | ||
| requests.append({ | ||
| "createParagraphBullets": { | ||
| "range": rng, | ||
| "bulletPreset": preset, | ||
| } | ||
| }) | ||
| # Blank spacer paragraph between top-level blocks for visual spacing | ||
| requests.append(_build_insert_text(cursor[0], "\n", tab_id)) | ||
| cursor[0] += 1 | ||
| i = j + 1 | ||
| continue |
Contributor
There was a problem hiding this comment.
Nested lists of different types are silently dropped.
The matching-close scan at lines 90–97 only tracks tokens whose type equals tok.type (e.g., bullet_list_open); a nested ordered_list inside a bullet list increments nothing but still consumes tokens that the inner k loop (lines 99–114) skips because it only processes list_item_open at the current nesting level. As a result, nested lists of a different kind are not rendered into the output at all.
This is consistent with the "unsupported features" note at the top of the module, but it's a silent data loss rather than a syntax error for callers. Consider either (a) documenting this more prominently (e.g., in the tool docstring for update_tab_from_markdown) or (b) falling back to flattening nested-list items into the outer list at a single bullet level. Not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gdocs/docs_markdown_writer.py` around lines 79 - 130, The code in the
list-handling block (using tok, tokens, the matching-close scan and the inner k
loop in gdocs/docs_markdown_writer.py) fails to account for nested lists of a
different type and thus silently drops them; update the matching-close scan and
the item iteration so any nested list_open/close of either kind is tracked and
nested list items are flattened/processed: in the depth scan, treat
tokens[j].type in ("bullet_list_open","ordered_list_open") as an increment and
their corresponding "_close" types as decrements (rather than only comparing to
tok.type), and in the inner loop that iterates items between i and j (the code
that finds "list_item_open" and locates its inline child before calling
_render_inline_with_styles and appending createParagraphBullets), search for
list_item_open tokens at any nesting depth (or search forward to the next
"inline" token inside a list_item_open) so nested ordered/bullet lists are
either flattened into the outer list or correctly captured for bullets created
by createParagraphBullets.
Owner
|
Noticed this and several other tab functions were missing from tool tiers so I did a refactor and consolidation as well as cleaning this up. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
skills/managing-google-workspace/references/docs.md (1)
217-217: Usebooleanfor type-consistency in the parameter table.Every other
booleanrow in this file uses the spellingboolean(e.g.include_comments,match_case,bold_headers,detailed); only this row readsbool. Aligns the doc table style.📝 Suggested change
-| replace_existing | bool | no | `True` | Clear tab body before inserting markdown | +| replace_existing | boolean | no | `True` | Clear tab body before inserting markdown |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/managing-google-workspace/references/docs.md` at line 217, The table row for the parameter replace_existing uses the type `bool` but should use `boolean` to match the rest of the file; update the type cell for the parameter named replace_existing to `boolean` so it is consistent with other boolean parameters like include_comments, match_case, bold_headers, and detailed.gdocs/docs_tools.py (1)
2458-2493: Optional: surface a cleaner error whentab_iddoesn't exist on populate.
_find_tab_end_indexreturns0when the target tab isn't found. Withreplace_existing=True, thetab_end > 2check then silently skips the clear, andmarkdown_to_docs_requests(..., tab_id=tab_id)is still emitted against the non-existent tab. The batchUpdate ultimately fails with a generic Google Docs error, which is harder to act on than an upfrontUserInputError.Not a blocker — the operation does fail rather than corrupt data — but a pre-flight check would give the caller a clearer, faster diagnostic.
♻️ Suggested guard
- if replace_existing: - doc = await asyncio.to_thread( - service.documents() - .get(documentId=document_id, includeTabsContent=True) - .execute - ) - tab_end = _find_tab_end_index(doc, tab_id) + if replace_existing: + doc = await asyncio.to_thread( + service.documents() + .get(documentId=document_id, includeTabsContent=True) + .execute + ) + tab_end = _find_tab_end_index(doc, tab_id) + if tab_end == 0: + raise UserInputError( + f"Tab '{tab_id}' was not found in document {document_id}. " + "Use inspect_doc_structure to discover valid tab IDs." + ) # tab_end includes the segment-terminating newline that Google Docs # refuses to delete, so we delete up to tab_end - 1. Empty tabs # (tab_end <= 2) have nothing to clear. - if tab_end and tab_end > 2: + if tab_end > 2:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2458 - 2493, The code currently proceeds to build markdown_to_docs_requests even when _find_tab_end_index(document, tab_id) returns 0 (tab missing), causing a generic downstream Google Docs batchUpdate error; add a pre-flight check after calling _find_tab_end_index to detect a missing tab and raise a UserInputError like "'tab_id' not found in document" when tab_end == 0 (or falsy) before appending any requests or calling markdown_to_docs_requests, keeping the existing replace_existing logic and using the same service.documents().get(...).execute call that retrieves the document.gdocs/docs_markdown_writer.py (1)
1-46: Remove unusedlinkify-it-pydependency frompyproject.toml(line 29).The package is declared as a dependency but never imported or used in the codebase. Since
MarkdownIt("commonmark")does not enable linkify functionality and no other module references this package, it can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_markdown_writer.py` around lines 1 - 46, The project declares the dependency "linkify-it-py" but the code (e.g., markdown_to_docs_requests and its use of MarkdownIt("commonmark")) never imports or uses it, so remove "linkify-it-py" from the dependency list in pyproject.toml and regenerate the lockfile (poetry/pip-tools/pipenv/etc.) or run your package manager's install to update the environment; after that run tests and lint to ensure nothing else relies on that package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gdocs/docs_markdown_writer.py`:
- Around line 1-46: The project declares the dependency "linkify-it-py" but the
code (e.g., markdown_to_docs_requests and its use of MarkdownIt("commonmark"))
never imports or uses it, so remove "linkify-it-py" from the dependency list in
pyproject.toml and regenerate the lockfile (poetry/pip-tools/pipenv/etc.) or run
your package manager's install to update the environment; after that run tests
and lint to ensure nothing else relies on that package.
In `@gdocs/docs_tools.py`:
- Around line 2458-2493: The code currently proceeds to build
markdown_to_docs_requests even when _find_tab_end_index(document, tab_id)
returns 0 (tab missing), causing a generic downstream Google Docs batchUpdate
error; add a pre-flight check after calling _find_tab_end_index to detect a
missing tab and raise a UserInputError like "'tab_id' not found in document"
when tab_end == 0 (or falsy) before appending any requests or calling
markdown_to_docs_requests, keeping the existing replace_existing logic and using
the same service.documents().get(...).execute call that retrieves the document.
In `@skills/managing-google-workspace/references/docs.md`:
- Line 217: The table row for the parameter replace_existing uses the type
`bool` but should use `boolean` to match the rest of the file; update the type
cell for the parameter named replace_existing to `boolean` so it is consistent
with other boolean parameters like include_comments, match_case, bold_headers,
and detailed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81c46d4a-50af-4042-a9ee-dc0332c4cec8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
README.mdcore/tool_tiers.yamlgdocs/docs_markdown_writer.pygdocs/docs_tools.pypyproject.tomlskills/managing-google-workspace/SKILL.mdskills/managing-google-workspace/references/docs.mdtests/gdocs/test_docs_markdown_writer.pytests/gdocs/test_insert_doc_tab_response.pytests/integration/test_update_tab_from_markdown.pytests/test_main_permissions_tier.py
✅ Files skipped from review due to trivial changes (3)
- core/tool_tiers.yaml
- skills/managing-google-workspace/SKILL.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
gdocs/docs_tools.py (4)
2470-2495: Tab existence is only validated whenreplace_existing=True.When
replace_existing=False,_find_tab_end_indexis skipped, so a typo intab_idis only surfaced by the downstream DocsbatchUpdatefailure (raised as a genericHttpError/ToolExecutionError) rather than the friendlyUserInputError("'<tab_id>' not found in document")users get on thereplace_existing=Truepath. Consider validatingtab_idexistence unconditionally forpopulate_from_markdownso behavior is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2470 - 2495, In populate_from_markdown, always validate that tab_id exists by calling _find_tab_end_index(document, tab_id) unconditionally (before branching on replace_existing) and raise the same UserInputError("'<tab_id>' not found in document") if it returns falsy; keep the existing deleteContentRange append (using tab_end > 2 and tab_end - 1) only when replace_existing is True, but perform the existence check and capture tab_end first via service.documents().get(...).execute and store it for later use so the behavior is consistent regardless of replace_existing.
2426-2511: Inconsistent return-shape across actions.The four action branches return dicts with different keys:
createreturns{"message", "tab_id"},delete/renamereturn{"message"}only, andpopulate_from_markdownreturns{"success", "requests_applied", "tab_id"}with nomessageand nolink. Callers (and the FastMCP-generated schema) get a heterogeneous payload that's awkward to consume and to test.Consider standardizing on a common shape like
{"success": bool, "message": str, "tab_id": Optional[str], ...}so each action returns the same minimum keys.♻️ Suggested unified shape
- return {"message": f"{msg} Link: {link}", "tab_id": new_tab_id} + return { + "success": True, + "message": f"{msg} Link: {link}", + "tab_id": new_tab_id, + } @@ - return { - "message": f"Deleted tab '{tab_id}' from document {document_id}. Link: {link}" - } + return { + "success": True, + "message": f"Deleted tab '{tab_id}' from document {document_id}. Link: {link}", + "tab_id": tab_id, + } @@ - return { - "message": f"Renamed tab '{tab_id}' to '{title}' in document {document_id}. Link: {link}" - } + return { + "success": True, + "message": f"Renamed tab '{tab_id}' to '{title}' in document {document_id}. Link: {link}", + "tab_id": tab_id, + } @@ - if not all_requests: - return {"success": True, "requests_applied": 0, "tab_id": tab_id} + if not all_requests: + return { + "success": True, + "message": f"No changes applied to tab '{tab_id}' in document {document_id}. Link: {link}", + "tab_id": tab_id, + "requests_applied": 0, + } @@ - return { - "success": True, - "requests_applied": len(all_requests), - "tab_id": tab_id, - } + return { + "success": True, + "message": f"Populated tab '{tab_id}' from markdown in document {document_id}. Link: {link}", + "tab_id": tab_id, + "requests_applied": len(all_requests), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2426 - 2511, The action branches (when action == "create", "delete", "rename", and "populate_from_markdown") return inconsistent dict shapes; update each branch so they all return a unified response schema (e.g., keys: "success": bool, "message": str, "tab_id": Optional[str], "requests_applied": int, "link": Optional[str]) — ensure the create/delete/rename branches add "success" and "requests_applied" (0 when none), include "tab_id" and "link" where available, and make populate_from_markdown include a human-readable "message" and "link"; modify the return statements in the create/delete/rename blocks and the final populate_from_markdown return to conform, referencing the existing variables action, tab_id, link, all_requests, and markdown_text to populate fields.
2335-2354: Empty/missing-body tab is indistinguishable from a normal tab here.
_find_tab_end_indexreturns1both when the tab exists with no content and whendocumentTabis absent (e.g., a non-document tab). Thepopulate_from_markdowncaller treats1as "valid empty tab" and proceeds to apply markdown requests, which will then fail at the API layer with a less helpful error. Consider distinguishing "tab not a document tab" up front:♻️ Optional guard
def walk(tabs: list) -> int: for tab in tabs: tab_props = tab.get("tabProperties", {}) if tab_props.get("tabId") == target_tab_id: - body = tab.get("documentTab", {}).get("body", {}) + if "documentTab" not in tab: + raise UserInputError( + f"Tab '{target_tab_id}' is not a document tab and cannot be populated." + ) + body = tab["documentTab"].get("body", {}) content = body.get("content", [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2335 - 2354, _find_tab_end_index currently returns 1 when a tab exists but lacks a documentTab (because body defaults to {}), which makes empty/missing-body tabs indistinguishable; update _find_tab_end_index to explicitly detect when tab.get("documentTab") is missing and return 0 (or another falsy sentinel) in that case, while preserving the existing return of 1 for a real document tab with an empty body, and ensure callers like populate_from_markdown treat 0 as "not a document tab / invalid for markdown" and skip applying markdown requests.
2371-2371: Use a Pydantic model or TypedDict for the return type instead of baredict.The function returns different dict shapes based on the
actionparameter:
create,populate_from_markdown: includesuccessand/ortab_iddelete,rename: include onlymessageDefine a union of Pydantic models (or TypedDicts) to accurately represent these variants. This aligns with the coding guidelines: "Use Pydantic v2 models for strict typing of request/response schemas in FastMCP tools" and ensures FastMCP generates a proper JSON-Schema that documents the actual response structure for LLM clients, instead of a permissive
additionalProperties: trueschema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` at line 2371, The function currently returns a bare dict with different shapes depending on the action parameter; replace that loose return type with a precise union of Pydantic v2 models (or TypedDicts) that capture each variant: e.g., CreatePopulateResponse with fields success: bool and optional tab_id: str, and DeleteRenameResponse with field message: str, then update the function signature to return Union[CreatePopulateResponse, DeleteRenameResponse] (or the equivalent TypedDict union) and update the code paths for actions 'create', 'populate_from_markdown', 'delete', and 'rename' to return instances/dicts matching those models so FastMCP produces strict JSON schema for responses.skills/managing-google-workspace/references/docs.md (1)
217-217: Use lowercasetruefor boolean default for consistency.Other rows in this and surrounding tables use lowercase
true/false(e.g.,bold_headersdefaulttrue,match_casedefaultfalse). The Python-literalTruehere is inconsistent with the rest of the doc.-| replace_existing | boolean | no | `True` | Clear tab body before inserting markdown | +| replace_existing | boolean | no | true | Clear tab body before inserting markdown |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/managing-google-workspace/references/docs.md` at line 217, Update the table row for the parameter `replace_existing` so its default value uses lowercase `true` instead of the Python literal `True`; mirror the casing used by other rows such as `bold_headers` and `match_case` to keep boolean defaults consistent across the docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/managing-google-workspace/references/docs.md`:
- Around line 219-225: The docs currently contradict themselves about image
handling between the "Supported markdown" section (which says "images rendered
as linked alt text") and the "Not yet supported" list ("embedded image
insertion"); update the text to be consistent by choosing one approach: either
remove images from the Supported list or explicitly clarify the distinction
(e.g., state under "Supported markdown" that images are supported only as linked
alt text fallback, and under "Not yet supported" keep "embedded image insertion"
to indicate inline embedding is unsupported). Edit the "Supported markdown"
heading content and the "Not yet supported" bullet so the phrasing matches
exactly (refer to the phrases "images rendered as linked alt text" and "embedded
image insertion") and ensure the PR description's claim ("Not supported: images,
tables, footnotes, smart chips, equations") is reflected or reconciled.
---
Nitpick comments:
In `@gdocs/docs_tools.py`:
- Around line 2470-2495: In populate_from_markdown, always validate that tab_id
exists by calling _find_tab_end_index(document, tab_id) unconditionally (before
branching on replace_existing) and raise the same UserInputError("'<tab_id>' not
found in document") if it returns falsy; keep the existing deleteContentRange
append (using tab_end > 2 and tab_end - 1) only when replace_existing is True,
but perform the existence check and capture tab_end first via
service.documents().get(...).execute and store it for later use so the behavior
is consistent regardless of replace_existing.
- Around line 2426-2511: The action branches (when action == "create", "delete",
"rename", and "populate_from_markdown") return inconsistent dict shapes; update
each branch so they all return a unified response schema (e.g., keys: "success":
bool, "message": str, "tab_id": Optional[str], "requests_applied": int, "link":
Optional[str]) — ensure the create/delete/rename branches add "success" and
"requests_applied" (0 when none), include "tab_id" and "link" where available,
and make populate_from_markdown include a human-readable "message" and "link";
modify the return statements in the create/delete/rename blocks and the final
populate_from_markdown return to conform, referencing the existing variables
action, tab_id, link, all_requests, and markdown_text to populate fields.
- Around line 2335-2354: _find_tab_end_index currently returns 1 when a tab
exists but lacks a documentTab (because body defaults to {}), which makes
empty/missing-body tabs indistinguishable; update _find_tab_end_index to
explicitly detect when tab.get("documentTab") is missing and return 0 (or
another falsy sentinel) in that case, while preserving the existing return of 1
for a real document tab with an empty body, and ensure callers like
populate_from_markdown treat 0 as "not a document tab / invalid for markdown"
and skip applying markdown requests.
- Line 2371: The function currently returns a bare dict with different shapes
depending on the action parameter; replace that loose return type with a precise
union of Pydantic v2 models (or TypedDicts) that capture each variant: e.g.,
CreatePopulateResponse with fields success: bool and optional tab_id: str, and
DeleteRenameResponse with field message: str, then update the function signature
to return Union[CreatePopulateResponse, DeleteRenameResponse] (or the equivalent
TypedDict union) and update the code paths for actions 'create',
'populate_from_markdown', 'delete', and 'rename' to return instances/dicts
matching those models so FastMCP produces strict JSON schema for responses.
In `@skills/managing-google-workspace/references/docs.md`:
- Line 217: Update the table row for the parameter `replace_existing` so its
default value uses lowercase `true` instead of the Python literal `True`; mirror
the casing used by other rows such as `bold_headers` and `match_case` to keep
boolean defaults consistent across the docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aea0fe0-14cf-4a39-9da9-14fdbae9fd74
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
gdocs/docs_tools.pypyproject.tomlskills/managing-google-workspace/references/docs.mdtests/gdocs/test_insert_doc_tab_response.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
skills/managing-google-workspace/references/docs.md (1)
205-205: Capitalize "Markdown" as a proper noun.Static analysis flagged this; "Markdown" is conventionally capitalized when referring to the format.
📝 Proposed wording
-Create, rename, delete, or populate tabs from markdown. Uses `action` to select the operation. +Create, rename, delete, or populate tabs from Markdown. Uses `action` to select the operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/managing-google-workspace/references/docs.md` at line 205, The sentence "Create, rename, delete, or populate tabs from markdown. Uses `action` to select the operation." incorrectly lowercases the format name; update the documentation in skills/managing-google-workspace/references/docs.md to capitalize Markdown (change "markdown" to "Markdown") so it reads "Create, rename, delete, or populate tabs from Markdown." and keep the rest of the sentence unchanged.gdocs/docs_tools.py (1)
2413-2413: UseLiteralfor theactionparameter to improve generated schema.The tool currently types
action: strand validates membership at runtime againstvalid_actions. Switching the annotation toLiteral["create", "rename", "delete", "populate_from_markdown"]lets FastMCP surface the allowed values directly in the JSON schema, gives clients/IDEs autocomplete, and removes the need for the runtime tuple check. This also matches the discriminator already encoded inCreateDocTabResponse.action, etc.♻️ Proposed refactor
- action: str, + action: Literal["create", "rename", "delete", "populate_from_markdown"], tab_id: Optional[str] = None, ... ): ... - valid_actions = ("create", "rename", "delete", "populate_from_markdown") - if action not in valid_actions: - raise UserInputError( - f"Invalid action '{action}'. Must be one of: {', '.join(valid_actions)}" - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` at line 2413, The parameter annotation for action should be changed from a generic str to a Literal of allowed values so the generated schema surfaces valid options and removes the need for the runtime membership check; update the function/method signature that currently declares action: str to use typing.Literal["create","rename","delete","populate_from_markdown"], add the Literal import, and then remove the manual tuple/valid_actions membership validation in the same function (and any redundant runtime checks) since CreateDocTabResponse.action already encodes the discriminator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gdocs/docs_tools.py`:
- Around line 2540-2541: The current _find_tab_end_index returns 0 for both "not
found" and "found-but-no-documentTab", so the caller sees the misleading
UserInputError when tab_end is falsy; update _find_tab_end_index to disambiguate
by returning a sentinel (e.g., None or -1) when the tab id is not found and
returning 0 only when the tab exists but has no documentTab, or alternatively
raise a specific exception (e.g., TabHasNoDocumentError or ValueError with a
clear message) when a matching tab lacks documentTab; then update callers that
check tab_end (and the code that raises UserInputError) to handle the new
sentinel/exception and surface a precise error mentioning the tab exists but is
not a documentTab (using the function name _find_tab_end_index, variable
tab_end, and UserInputError in your changes).
- Around line 2536-2560: The current logic calls
markdown_to_docs_requests(markdown_text, tab_id=tab_id) with its default
start_index=1 so when replace_existing is False new content is prepended; update
the behavior so non-replacing calls append instead: detect non-empty tab via
tab_end (e.g., tab_end > 2) and pass start_index=tab_end - 1 to
markdown_to_docs_requests(markdown_text, tab_id=tab_id, start_index=tab_end - 1)
so content is appended after existing text, or alternatively raise
UserInputError when replace_existing is False and tab_end indicates a non-empty
tab if you prefer rejecting mixed content. Ensure you reference tab_end,
replace_existing, tab_id and markdown_to_docs_requests when making the change.
---
Nitpick comments:
In `@gdocs/docs_tools.py`:
- Line 2413: The parameter annotation for action should be changed from a
generic str to a Literal of allowed values so the generated schema surfaces
valid options and removes the need for the runtime membership check; update the
function/method signature that currently declares action: str to use
typing.Literal["create","rename","delete","populate_from_markdown"], add the
Literal import, and then remove the manual tuple/valid_actions membership
validation in the same function (and any redundant runtime checks) since
CreateDocTabResponse.action already encodes the discriminator.
In `@skills/managing-google-workspace/references/docs.md`:
- Line 205: The sentence "Create, rename, delete, or populate tabs from
markdown. Uses `action` to select the operation." incorrectly lowercases the
format name; update the documentation in
skills/managing-google-workspace/references/docs.md to capitalize Markdown
(change "markdown" to "Markdown") so it reads "Create, rename, delete, or
populate tabs from Markdown." and keep the rest of the sentence unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c168f3a0-0e45-4e27-9b7a-15584335398c
📒 Files selected for processing (3)
gdocs/docs_tools.pyskills/managing-google-workspace/references/docs.mdtests/gdocs/test_insert_doc_tab_response.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/gdocs/test_insert_doc_tab_response.py
…into fork-extension
Owner
|
This turned into a huge change but I just took it for a spin and she works beautifully! |
13 tasks
Contributor
Author
Working great so far using the manage doc tab function heaps, thanks for the quick merge |
This was referenced Jun 13, 2026
12 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
2 participants
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.
Adds a new MCP tool update_tab_from_markdown that parses CommonMark+GFM markdown and populates a specific tab in a Google Doc via batchUpdate. Fills a directional gap in the existing gdocs module - the fork already converts Docs to markdown (docs_markdown.py) but had no converter for the write direction.
Also fixes two pre-existing bugs discovered during integration testing against real Google Docs:
insert_doc_tab silently returning tab_id=None - the batchUpdate reply for an addDocumentTab request comes back under the key addDocumentTab, not createDocumentTab as the fork assumed. Same bug existed in batch_operation_manager._extract_created_tabs. Both fixed with dual-key lookup (primary addDocumentTab, defensive fallback createDocumentTab).
deleteContentRange off-by-one - when clearing existing tab content, endIndex = last_element.endIndex includes the segment-terminating newline that Google Docs refuses to delete. Fixed to endIndex = tab_end - 1 with skip threshold raised to tab_end > 2 for empty tabs.
New module gdocs/docs_markdown_writer.py (~290 lines) using markdown-it-py (new dependency, permissive MIT license). Every request threads an optional tab_id through to the range/location objects, making this usable for both whole-doc writes and tab-scoped writes.
Type of Change
This is NOT a breaking change. All modifications are additive or defensively backwards-compatible.
Testing
Tests added and passing locally, and tested manually.
Test summary:
Checklist
Code style - matches the existing @server.tool() @handle_http_errors @require_google_service decorator stack, uses asyncio.to_thread for blocking .execute() calls, follows the existing List[dict] / Optional[str] typing conventions.
Self-review completed.
Comments added in the cursor-arithmetic logic and the tab-id threading in docs_markdown_writer.py.
No new warnings introduced by this change.
"Allow edits from maintainers" is enabled on this PR.
Additional Notes
Supported markdown constructs - H1-H6 headings, paragraphs, inline bold / italic / code / links, ordered and unordered lists, fenced code blocks, blockquotes, horizontal rules.
Not yet supported (documented as such in the tool docstring) - images (would need Drive upload first), tables (falls through to plain text), footnotes, smart chips, equations.
Dependencies added - markdown-it-py>=3.0.0 and linkify-it-py>=2.0.0. Both are MIT-licensed, actively maintained, zero native deps.
Example usage
Create a tab
insert_doc_tab(
document_id="...",
title="Blog Article",
index=0,
)
Populate it from a markdown file
update_tab_from_markdown(
document_id="...",
tab_id="t.abc123",
markdown_text=open("post.md").read(),
replace_existing=True,
)
Blast radius - All changes are additive (new module, new tool, new tests) except the two bug fixes in insert_doc_tab and batch_operation_manager._extract_created_tabs. Those fixes use dual-key lookup so existing callers that happened to receive createDocumentTab-keyed responses (theoretical, never observed in testing) continue to work. Public API surface unchanged.
Motivation - Built to support batch population of client documents with tabs from markdown source files. Full design doc available in the contributor's linked repo if helpful context for review.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores