feat: add suggestion-aware doc text tools with section chunking - #629
feat: add suggestion-aware doc text tools with section chunking#629pgonzale60 wants to merge 6 commits into
Conversation
Added comprehensive Google Contacts integration with 6 core tools: Tools implemented: - list_contacts: List all contacts with pagination support - search_contacts: Search contacts by name, email, or phone - get_contact: Get detailed contact information - create_contact: Create new contacts with name, email, phone, organization - update_contact: Update existing contact information - delete_contact: Permanently delete contacts Technical changes: - Created gcontacts module following existing service patterns - Added People API scopes (contacts.readonly and contacts) - Integrated with auth system and service decorator - Added contacts to tool tiers configuration * Core tier: list, search, get (read-only) * Extended tier: create, update (write operations) * Complete tier: delete (destructive operation) - Updated main.py to register contacts module - Added contacts to CLI argument choices Usage: uv run main.py --tools contacts uv run main.py --tool-tier core --tools contacts Requires enabling People API in Google Cloud Console: https://console.cloud.google.com/flows/enableapi?apiid=people.googleapis.com
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…areness - New gdocs/docs_text.py helper: extract_sections (heading-based chunking) and render_elements (original vs accepted suggestion modes) - list_doc_sections returns TOC or chunk info for headingless docs - get_doc_text supports mode=original|accepted, section_index, chunk_index - 15 unit tests covering all extraction logic Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Google Contacts (People API) MCP tools, suggestion-aware Google Docs text extraction and sectioning utilities, and configurable OAuth callback port support via Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/test_docs_text.py (2)
102-105: Remove redundant import.
render_elementsis already imported at line 6. This local re-import is unnecessary.🧹 Proposed fix
def test_depth_guard(self): # Should return empty string at depth > 5 - from gdocs.docs_text import render_elements assert render_elements([make_paragraph([make_text_run("x\n")])], "original", depth=6) == ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_docs_text.py` around lines 102 - 105, The test function test_depth_guard contains a redundant local import of render_elements; remove the line "from gdocs.docs_text import render_elements" inside test_depth_guard so the function uses the render_elements already imported at the top of the file, leaving the rest of the test (the call to render_elements and the assertion) unchanged.
2-4: Prefer proper package configuration over sys.path manipulation.Direct
sys.pathmanipulation is fragile and can cause issues in different environments. Consider using apyproject.tomlorsetup.pywith the package installed in editable mode (pip install -e .), which allows imports to work naturally.If this pattern is required for CI compatibility, consider using
pytest'spythonpathconfiguration inpyproject.tomlorpytest.iniinstead.♻️ Alternative using pytest configuration
In
pyproject.toml:[tool.pytest.ini_options] pythonpath = ["."]Then the test file can simply be:
"""Unit tests for gdocs/docs_text.py""" -import sys -import os -sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) - from gdocs.docs_text import render_elements, extract_sections🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_docs_text.py` around lines 2 - 4, The test file tests/test_docs_text.py contains brittle sys.path manipulation via sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))); remove that line and rely on proper package installation or pytest configuration instead—either install the package in editable mode (pip install -e .) so imports work naturally, or add pythonpath = ["."] under [tool.pytest.ini_options] in pyproject.toml (or equivalent pytest.ini) so tests can import without modifying sys.path.gdocs/docs_text.py (1)
35-48: Depth guard uses strict inequality - verify boundary behavior.The guard
if depth > 5returns empty at depth 6, meaning depth 5 is the last level that renders content. For deeply nested tables (tables within table cells), this allows 6 levels of recursion (0-5). This seems reasonable but should be documented.Consider adding a brief comment explaining the chosen limit (e.g., "Google Docs rarely has tables nested more than 5 levels deep").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_text.py` around lines 35 - 48, The depth guard in render_elements currently uses "if depth > 5" which permits recursion for depths 0–5 and stops at 6; add a concise explanatory comment next to the guard inside the render_elements function (or above it) stating the intentional limit and rationale (e.g., "limit recursion to 5 levels; Google Docs rarely nests tables deeper than 5"), so readers understand the boundary behavior and that depth 5 is the last rendered level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/comments.py`:
- Around line 137-153: The current loop in core/comments.py which uses
service.comments().list to fetch all pages (variables: comments, page_token,
kwargs) must be changed to enforce a hard cap (e.g., max_items) and support
cursor-based continuation: add a max_items parameter to the enclosing function
or tool contract, stop fetching once len(comments) >= max_items, truncate the
returned comments array to max_items, and return the current page_token (or a
nextCursor) so callers can continue pagination later; ensure you do not
accumulate all pages into memory and do not perform long-running reads beyond
the request budget by breaking the loop early and signaling continuation.
In `@docs/superpowers/specs/2026-03-28-doc-text-with-suggestions-design.md`:
- Around line 96-104: The fenced code blocks showing the response format
examples are missing language specifiers; update both examples that contain the
blocks with captions "[Section 2/8: \"Introduction\" | mode: original]" and
"[Chunk 1/5 | mode: accepted | chunk_size: 10000]" to use a plain-text specifier
(e.g., change the opening triple backticks to ```text) so markdownlint is
satisfied and the examples render consistently.
In `@gcontacts/contacts_tools.py`:
- Around line 125-126: Multiple functions in gcontacts.contacts_tools.py are
importing json inside their bodies (e.g., the locations around lines 125-126,
213-214, 304-305, 398-399, 507-508, 551-552) which is inefficient; move a single
import json to the top of the module and delete the local "import json"
statements from each function (those that call json.dumps(response, indent=2)
and similar). Ensure the module-level import is placed with other imports and
that all functions continue using json.* without local imports.
- Line 44: Existing logger calls leak PII by including user_google_email (e.g.,
the logger.info in list_contacts); update all logger statements that interpolate
user_google_email to remove the email and instead log non-PII context such as
the tool name, operation (e.g., "list_contacts invoked"), or a session/trace id;
search for logger.* calls in contacts_tools.py that mention user_google_email
(including the instances flagged) and replace the message to omit the email
while preserving useful context and any existing log level or structured fields.
- Around line 479-482: The early return uses json.dumps when update_mask is
empty, but json is imported later in the function which causes a NameError; fix
by importing json at module level (preferred) or at least before the
early-return check inside the function so that the call to json.dumps in the
update flow (referencing update_mask) can execute without error—ensure the
import is placed before any use of json in contacts_tools.py where functions
like the update routine reference update_mask.
---
Nitpick comments:
In `@gdocs/docs_text.py`:
- Around line 35-48: The depth guard in render_elements currently uses "if depth
> 5" which permits recursion for depths 0–5 and stops at 6; add a concise
explanatory comment next to the guard inside the render_elements function (or
above it) stating the intentional limit and rationale (e.g., "limit recursion to
5 levels; Google Docs rarely nests tables deeper than 5"), so readers understand
the boundary behavior and that depth 5 is the last rendered level.
In `@tests/test_docs_text.py`:
- Around line 102-105: The test function test_depth_guard contains a redundant
local import of render_elements; remove the line "from gdocs.docs_text import
render_elements" inside test_depth_guard so the function uses the
render_elements already imported at the top of the file, leaving the rest of the
test (the call to render_elements and the assertion) unchanged.
- Around line 2-4: The test file tests/test_docs_text.py contains brittle
sys.path manipulation via sys.path.insert(0,
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))); remove that line
and rely on proper package installation or pytest configuration instead—either
install the package in editable mode (pip install -e .) so imports work
naturally, or add pythonpath = ["."] under [tool.pytest.ini_options] in
pyproject.toml (or equivalent pytest.ini) so tests can import without modifying
sys.path.
🪄 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: c971b265-e966-4e8d-89d7-4246d59f97d5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
auth/oauth_config.pyauth/scopes.pyauth/service_decorator.pycore/comments.pycore/tool_tiers.yamldocs/superpowers/specs/2026-03-28-doc-text-with-suggestions-design.mdfastmcp_server.pygcontacts/__init__.pygcontacts/contacts_tools.pygdocs/docs_text.pygdocs/docs_tools.pymain.pytests/test_docs_text.py
| comments = [] | ||
| page_token = None | ||
| while True: | ||
| kwargs = dict( | ||
| fileId=file_id, | ||
| fields="comments(id,content,author,createdTime,modifiedTime,resolved,replies(content,author,id,createdTime,modifiedTime))" | ||
| ).execute | ||
| ) | ||
|
|
||
| comments = response.get('comments', []) | ||
| pageSize=100, | ||
| fields="nextPageToken,comments(id,content,author,createdTime,modifiedTime,resolved,replies(content,author,id,createdTime,modifiedTime))" | ||
| ) | ||
| if page_token: | ||
| kwargs["pageToken"] = page_token | ||
| response = await asyncio.to_thread( | ||
| service.comments().list(**kwargs).execute | ||
| ) | ||
| comments.extend(response.get('comments', [])) | ||
| page_token = response.get('nextPageToken') | ||
| if not page_token: | ||
| break |
There was a problem hiding this comment.
Bound paginated reads to avoid oversized/slow tool responses.
Line 137-Line 153 now fetches and accumulates every page into one response. On large files, this can exceed practical MCP payload size and request-time budgets. Please add a hard cap (max_items) plus cursor-based continuation in the tool contract (or at minimum truncate and signal continuation token).
Suggested direction
async def _read_comments_impl(service, app_name: str, file_id: str) -> str:
@@
- comments = []
- page_token = None
+ comments = []
+ page_token = None
+ max_items = 250
+ truncated = False
@@
- comments.extend(response.get('comments', []))
+ page_comments = response.get('comments', [])
+ comments.extend(page_comments)
+ if len(comments) >= max_items:
+ comments = comments[:max_items]
+ truncated = True
+ page_token = response.get('nextPageToken')
+ break
page_token = response.get('nextPageToken')
if not page_token:
break
@@
- output = [f"Found {len(comments)} comments in {app_name} {file_id}:\\n"]
+ output = [f"Found {len(comments)} comments in {app_name} {file_id}:\\n"]
+ if truncated or page_token:
+ output.append("Results truncated; add cursor/max_items parameters to continue pagination.")
+ output.append("")As per coding guidelines, "Use pagination helpers (list_page_size, nextCursor) for large result sets to prevent oversized LLM responses" and "Avoid long-running operations (>30s) inside request context; instead stream partial results or schedule background tasks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/comments.py` around lines 137 - 153, The current loop in
core/comments.py which uses service.comments().list to fetch all pages
(variables: comments, page_token, kwargs) must be changed to enforce a hard cap
(e.g., max_items) and support cursor-based continuation: add a max_items
parameter to the enclosing function or tool contract, stop fetching once
len(comments) >= max_items, truncate the returned comments array to max_items,
and return the current page_token (or a nextCursor) so callers can continue
pagination later; ensure you do not accumulate all pages into memory and do not
perform long-running reads beyond the request budget by breaking the loop early
and signaling continuation.
| ``` | ||
| [Section 2/8: "Introduction" | mode: original] | ||
| <text> | ||
| ``` | ||
| or for chunks: | ||
| ``` | ||
| [Chunk 1/5 | mode: accepted | chunk_size: 10000] | ||
| <text> | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
The response format examples are missing language specifiers, which was flagged by markdownlint. Adding text or plaintext as the language will satisfy the linter and improve documentation consistency.
📝 Proposed fix
**Response format:** Text content prefixed with metadata header:
-```
+```text
[Section 2/8: "Introduction" | mode: original]
<text>or for chunks:
- +text
[Chunk 1/5 | mode: accepted | chunk_size: 10000]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| [Section 2/8: "Introduction" | mode: original] | |
| <text> | |
| ``` | |
| or for chunks: | |
| ``` | |
| [Chunk 1/5 | mode: accepted | chunk_size: 10000] | |
| <text> | |
| ``` | |
| **Response format:** Text content prefixed with metadata header: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-03-28-doc-text-with-suggestions-design.md` around
lines 96 - 104, The fenced code blocks showing the response format examples are
missing language specifiers; update both examples that contain the blocks with
captions "[Section 2/8: \"Introduction\" | mode: original]" and "[Chunk 1/5 |
mode: accepted | chunk_size: 10000]" to use a plain-text specifier (e.g., change
the opening triple backticks to ```text) so markdownlint is satisfied and the
examples render consistently.
| Returns: | ||
| str: JSON string containing list of contacts with names, email addresses, phone numbers, and pagination info. | ||
| """ | ||
| logger.info(f"[list_contacts] Invoked. Email: '{user_google_email}'") |
There was a problem hiding this comment.
Avoid logging user email addresses (PII).
Multiple log statements include user_google_email, which is personally identifiable information. As per coding guidelines: "Never log or leak secrets, refresh tokens, or PII in exceptions or event streams."
Consider logging only non-PII context such as the tool name or a session identifier.
🛡️ Proposed fix example
- logger.info(f"[list_contacts] Invoked. Email: '{user_google_email}'")
+ logger.info("[list_contacts] Invoked")Apply similar changes to all affected log statements.
Also applies to: 156-156, 242-243, 343-343, 443-443, 539-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gcontacts/contacts_tools.py` at line 44, Existing logger calls leak PII by
including user_google_email (e.g., the logger.info in list_contacts); update all
logger statements that interpolate user_google_email to remove the email and
instead log non-PII context such as the tool name, operation (e.g.,
"list_contacts invoked"), or a session/trace id; search for logger.* calls in
contacts_tools.py that mention user_google_email (including the instances
flagged) and replace the message to omit the email while preserving useful
context and any existing log level or structured fields.
| import json | ||
| return json.dumps(response, indent=2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move json import to module level.
The json module is imported inside each function. This is inefficient and unconventional. Moving it to the module level improves readability and follows standard Python practices.
♻️ Proposed fix
Add at module level:
import asyncio
+import json
import logging
from typing import Any, Dict, List, OptionalThen remove all the import json statements inside functions.
Also applies to: 213-214, 304-305, 398-399, 507-508, 551-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gcontacts/contacts_tools.py` around lines 125 - 126, Multiple functions in
gcontacts.contacts_tools.py are importing json inside their bodies (e.g., the
locations around lines 125-126, 213-214, 304-305, 398-399, 507-508, 551-552)
which is inefficient; move a single import json to the top of the module and
delete the local "import json" statements from each function (those that call
json.dumps(response, indent=2) and similar). Ensure the module-level import is
placed with other imports and that all functions continue using json.* without
local imports.
| if not update_mask: | ||
| return json.dumps({ | ||
| "error": "No fields to update. Please provide at least one field to update." | ||
| }, indent=2) |
There was a problem hiding this comment.
json module used before import — will raise NameError.
At line 480, json.dumps is called, but json is not imported until line 507 in this function. This will cause a NameError at runtime when no fields are provided to update.
🐛 Proposed fix — move import to top of function or module level
Either add at module level (preferred):
import asyncio
import logging
+import json
from typing import Any, Dict, List, OptionalOr add before the early return:
if not update_mask:
+ import json
return json.dumps({
"error": "No fields to update. Please provide at least one field to update."
}, indent=2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gcontacts/contacts_tools.py` around lines 479 - 482, The early return uses
json.dumps when update_mask is empty, but json is imported later in the function
which causes a NameError; fix by importing json at module level (preferred) or
at least before the early-return check inside the function so that the call to
json.dumps in the update flow (referencing update_mask) can execute without
error—ensure the import is placed before any use of json in contacts_tools.py
where functions like the update routine reference update_mask.
…ering The Docs API suggestionsViewMode parameter handles suggestion filtering server-side, making client-side parsing of suggestedInsertionIds / suggestedDeletionIds unnecessary. - render_elements simplified to plain text extraction (no mode param) - SUGGESTIONS_VIEW_MODE map added to docs_text.py - Both tools pass suggestionsViewMode to documents().get() - Tests updated (14 passing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
gdocs/docs_tools.py (1)
1329-1345: Reject simultaneoussection_indexandchunk_indexto avoid ambiguous requests.Current behavior silently prioritizes
section_indexwhen both are set. Returning a clear validation error makes the API contract explicit.Proposed fix
body_elements = doc_data.get("body", {}).get("content", []) doc_name = file_metadata["name"] + if section_index is not None and chunk_index is not None: + return "Provide either section_index or chunk_index, not both." + # --- Section-based access --- if section_index is not None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 1329 - 1345, Add input validation to reject requests where both section_index and chunk_index are provided: at the start of the function handling these options (the block that currently checks "if section_index is not None:" and later "if chunk_index is not None:"), detect if both section_index and chunk_index are not None and return a clear validation error string (or raise a ValueError/Http error consistent with existing API patterns) explaining that both cannot be set simultaneously; update any calling comments and preserve existing behavior of extract_sections, render_elements and subsequent section/chunk handling when only one is provided.
🤖 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 1195-1201: The new tools list_doc_sections and get_doc_text are
defined but not registered in the docs tool tier registry, so add entries for
both to core/tool_tiers.yaml under the appropriate docs/tools tier; update the
registry to include the tool names (list_doc_sections, get_doc_text), their
service_type ("docs"), and any required metadata (scopes/visibility/tier)
consistent with existing tool entries so they become discoverable at runtime.
- Line 1278: Validate the chunk_size parameter before any chunk math: in the
function that takes chunk_size (the parameter named chunk_size) add a guard that
ensures isinstance(chunk_size, int) and chunk_size > 0 (or coerce to int and
then check) and raise a clear ValueError if invalid; this prevents division/ceil
operations later (the block that divides/uses chunk_size around the chunking
logic) from raising runtime errors or producing invalid behavior.
- Around line 1246-1252: The returned guidance assumes num_chunks > 0 and yields
an invalid range when char_len == 0; update the block that computes num_chunks
(using num_chunks = -(-char_len // chunk_size)) to handle the zero-length case:
if num_chunks == 0, return a message stating the document
(file_metadata["name"]) is empty and no chunks are available instead of
"chunk_index=0..-1"; otherwise keep the existing message that shows Total
characters, Recommended chunk_size and the valid range
"chunk_index=0..{num_chunks - 1}". Ensure you reference the same variables
(num_chunks, char_len, chunk_size, file_metadata) and the get_doc_text
chunk_index guidance in the two branches.
---
Nitpick comments:
In `@gdocs/docs_tools.py`:
- Around line 1329-1345: Add input validation to reject requests where both
section_index and chunk_index are provided: at the start of the function
handling these options (the block that currently checks "if section_index is not
None:" and later "if chunk_index is not None:"), detect if both section_index
and chunk_index are not None and return a clear validation error string (or
raise a ValueError/Http error consistent with existing API patterns) explaining
that both cannot be set simultaneously; update any calling comments and preserve
existing behavior of extract_sections, render_elements and subsequent
section/chunk handling when only one is provided.
🪄 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: cffbef6b-cc30-40e3-9730-e459c48d4a04
📒 Files selected for processing (3)
gdocs/docs_text.pygdocs/docs_tools.pytests/test_docs_text.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_docs_text.py
| @server.tool() | ||
| @handle_http_errors("list_doc_sections", is_read_only=True, service_type="docs") | ||
| @require_multiple_services([ | ||
| {"service_type": "drive", "scopes": "drive_read", "param_name": "drive_service"}, | ||
| {"service_type": "docs", "scopes": "docs_read", "param_name": "docs_service"} | ||
| ]) | ||
| async def list_doc_sections( |
There was a problem hiding this comment.
Register the new tools in core/tool_tiers.yaml or they remain undiscoverable.
list_doc_sections and get_doc_text are introduced here, but they are not present in the docs tool tier registry from core/tool_tiers.yaml (context snippet). This will prevent tier-based exposure at runtime.
Also applies to: 1264-1269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gdocs/docs_tools.py` around lines 1195 - 1201, The new tools
list_doc_sections and get_doc_text are defined but not registered in the docs
tool tier registry, so add entries for both to core/tool_tiers.yaml under the
appropriate docs/tools tier; update the registry to include the tool names
(list_doc_sections, get_doc_text), their service_type ("docs"), and any required
metadata (scopes/visibility/tier) consistent with existing tool entries so they
become discoverable at runtime.
| num_chunks = -(-char_len // chunk_size) # ceiling division | ||
| return ( | ||
| f'Document "{file_metadata["name"]}" has no headings.\n' | ||
| f"Total characters: {char_len}\n" | ||
| f"Recommended chunk_size: {chunk_size} ({num_chunks} chunks)\n" | ||
| f"Use get_doc_text with chunk_index=0..{num_chunks - 1}" | ||
| ) |
There was a problem hiding this comment.
Handle empty headingless documents explicitly in chunk guidance.
Line 1246 computes num_chunks = 0 for empty docs, but Line 1251 then returns chunk_index=0..-1, which is an invalid range.
Proposed fix
char_len = len(full_text)
chunk_size = 10000
num_chunks = -(-char_len // chunk_size) # ceiling division
+ if num_chunks == 0:
+ return (
+ f'Document "{file_metadata["name"]}" has no headings and no text content.\n'
+ "Use get_doc_text to confirm mode-specific output if needed."
+ )
return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gdocs/docs_tools.py` around lines 1246 - 1252, The returned guidance assumes
num_chunks > 0 and yields an invalid range when char_len == 0; update the block
that computes num_chunks (using num_chunks = -(-char_len // chunk_size)) to
handle the zero-length case: if num_chunks == 0, return a message stating the
document (file_metadata["name"]) is empty and no chunks are available instead of
"chunk_index=0..-1"; otherwise keep the existing message that shows Total
characters, Recommended chunk_size and the valid range
"chunk_index=0..{num_chunks - 1}". Ensure you reference the same variables
(num_chunks, char_len, chunk_size, file_metadata) and the get_doc_text
chunk_index guidance in the two branches.
| mode: str, | ||
| section_index: int = None, | ||
| chunk_index: int = None, | ||
| chunk_size: int = 10000, |
There was a problem hiding this comment.
Validate chunk_size as a positive integer before chunk math.
Line 1347 divides by chunk_size with no guard; chunk_size <= 0 can fail with runtime errors or invalid chunk behavior.
Proposed fix
if mode not in ("original", "accepted"):
return 'Invalid mode. Use "original" or "accepted".'
+ if chunk_size <= 0:
+ return "Invalid chunk_size. Use a positive integer."Also applies to: 1347-1358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gdocs/docs_tools.py` at line 1278, Validate the chunk_size parameter before
any chunk math: in the function that takes chunk_size (the parameter named
chunk_size) add a guard that ensures isinstance(chunk_size, int) and chunk_size
> 0 (or coerce to int and then check) and raise a clear ValueError if invalid;
this prevents division/ceil operations later (the block that divides/uses
chunk_size around the chunking logic) from raising runtime errors or producing
invalid behavior.
|
Something funky is going on with this branch you've got, it's way off the main. Will switch to draft until you're ready! |
|
Closing in favour of a clean rebase onto current upstream main. |
|
Thank you for reviewing this. Sorry for the spam! All the best, |
2 participants
Summary
list_doc_sectionstool — returns a table of contents based on heading structure, or chunk info for headingless documentsget_doc_texttool — retrieves document text inoriginalmode (excluding pending insertions, preserving pending deletions) oracceptedmode (insertions included, deletions excluded)section_indexand chunk-based access viachunk_index/chunk_sizefor long documentsgdocs/docs_text.pymodule withextract_sectionsandrender_elementshelpersTest plan
uv run pytest tests/test_docs_text.py -v— all 15 tests passlist_doc_sectionson a doc with headings — verify numbered TOC returnedlist_doc_sectionson a headingless doc — verify char count and chunk info returnedget_doc_textwithmode="original"— verify suggested insertions absentget_doc_textwithmode="accepted"— verify suggested deletions absentget_doc_textwith out-of-rangesection_index— verify clear error messageget_doc_texton a.docxfile — verify error directing toget_doc_content🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests