fix(google-vault): validate against live API docs, add matter/hold/saved-query CRUD coverage#5482
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview Docs ( Icons ( Note: The PR title/description mention Google Vault tool fixes; those changes are not present in the diff shown here. Reviewed by Cursor Bugbot for commit 0a96355. Configure here. |
Greptile SummaryThis PR substantially expands the Google Vault integration from 6 to 22 operations, covering the full matter lifecycle (update, close, reopen, delete, undelete), hold management (update, delete, add/remove held accounts), matter collaborator management, export deletion, and saved query CRUD. It also fixes a critical pre-existing bug where
Confidence Score: 4/5Safe to merge after addressing the stale-ID issue in the block params mapper; all new tool files and the export bug fix are correct. The block params mapper uses conditional spread to map list-specific optional fields to their tool param names. Because the spread is skipped when the list field is empty, a stale exportId/holdId/savedQueryId previously set by one of the new required-ID operations flows through rest and causes the list tool to silently execute a single-resource GET instead of listing everything. apps/sim/blocks/blocks/google_vault.ts — the params mapper list-field overrides need to unconditionally clear stale IDs from rest for all four list operations Important Files Changed
Reviews (10): Last reviewed commit: "docs: regenerate integration docs" | Re-trigger Greptile |
|
@greptile review |
|
@cursor review |
|
Ran an independent 4-way parallel audit against the live Vault API docs (matter lifecycle, exports/file-handling, holds, saved-queries + block wiring + backwards-compat). One additional real finding surfaced and fixed in f0f8986: |
|
@greptile review |
|
@cursor review |
|
@greptile review |
|
@cursor review |
|
@greptile review |
|
@cursor review |
|
@greptile review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0665688. Configure here.
|
@greptile review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1d0b646. Configure here.
…tter/hold/saved-query CRUD coverage - Fix critical bug: create_matters_export sent the deprecated Query.searchMethod field (deprecated 2019, support ended 2020) instead of method, silently breaking account/org-unit scoped exports - Fix duplicate matterId subBlock id (two definitions collided across operations) - Add matter lifecycle: update, close, reopen, delete, undelete - Add matter collaborator management: add/remove permissions - Add export delete - Add hold update, delete, add/remove held accounts - Add saved query create/list/delete - Bump tool versions to 1.0.0 to match repo convention - Fix docsLink to point at docs.sim.ai instead of the vendor site - All new endpoints are covered by the existing ediscovery + devstorage.read_only OAuth scopes (no new scopes requested)
…dden Greptile review: hidden is reserved for framework-injected tokens; pageToken in list_saved_queries.ts should be user-or-llm so an agent/user can pass it, matching the pattern used elsewhere for tool-supplied pagination cursors.
…e on update
Cursor Bugbot: PUT holds/{id} replaces the full resource — a name/query-only
update with no accountEmails/orgUnitId would silently drop the hold's
custodian coverage. Now throws a clear error directing callers to resend the
scope or use add_held_accounts/remove_held_accounts for incremental changes.
… corpus Independent audit (parallel doc-verification pass): create_matters_export and create_saved_query resolved Query.method to undefined when corpus wasn't MAIL and no accountEmails/orgUnitId was given, silently sending an invalid request (method is a required Query field) instead of a clear error. Now throws with an actionable message before the request is sent.
…ry filters Cursor Bugbot: update_matters_holds only sets query.mailQuery/groupsQuery/ driveQuery when terms/date/shared-drive fields are provided, but the PUT replaces the whole hold — omitting a previously-set filter clears it, not leaves it unchanged. Filters are legitimately optional (a hold may have none), so this can't be hard-required like scope; instead the tool and field descriptions now explicitly state the full-replace behavior and direct callers to resupply current values via Vault List Holds first.
Cursor Bugbot: consolidating shared subblocks across operations left two cross-contamination risks since a stale value from one operation stays in block state until overwritten: - accountEmails/orgUnitId are checked emails-first with silent either/or priority; sharing them across update_matters_holds and create_saved_query meant a leftover value from a different operation could silently override the intended scope. Gave both operations dedicated fields (updateHoldAccountEmails/updateHoldOrgUnitId, savedQueryAccountEmails/ savedQueryOrgUnitId), remapped in tools.config.params. - matterId presence alone switches google_vault_list_matters between list-all and single-get. Sharing it with every other operation meant a leftover matterId could silently turn "List Matters" into a single-matter fetch. Gave list_matters its own optional listMatterId field. create_matters_holds/create_matters_export keep sharing accountEmails/ orgUnitId as before this PR (pre-existing behavior, not introduced here).
…where counterparts Cursor Bugbot: exportId/holdId/savedQueryId were shared between their respective list operation (optional filter) and update/delete/held-account operations (required). A stale ID left over from a delete/update on the same block instance would silently narrow the corresponding list operation to a single-resource get instead of listing the collection. Gave each list operation its own dedicated optional field (listExportId, listHoldId, listSavedQueryId), remapped in tools.config.params — same pattern already used for listMatterId.
Greptile: savedQueryAccountEmails/savedQueryOrgUnitId spread after their updateHold* counterparts, so if both were ever truthy at once the wrong one would silently win. In practice they're mutually exclusive (each only populated while its own single 'operation' value is selected, so at most one pair is ever truthy), but reordering costs nothing and removes any doubt about precedence.
1d0b646 to
afbd071
Compare
Reruns scripts/generate-docs.ts against the current block/tool/trigger registry. Picks up google_vault's new operations plus everything else that had landed on staging without a docs regen (bigquery, google_calendar, google_maps, onedrive, microsoft_teams, gitlab, github, discord, dropbox, and others), plus icon and integrations.json updates.
|
@greptile review |
|
@cursor review |
|
Re: the ENTIRE_ORG/DRIVE concern flagged in the latest review summary (now also mentioned for create_saved_query.ts, mirroring the same finding already addressed on create_matters_export.ts): I verified this against the live Google Vault API docs (https://developers.google.com/workspace/vault/reference/rest/v1/Query). The ENTIRE_ORG SearchMethod's documented description is explicit: "Search the data of all accounts in the organization. Supported only for Gmail. When specified, you don't need to specify AccountInfo or OrgUnitInfo." DRIVE is not supported for ENTIRE_ORG per the live docs — extending it to DRIVE (as suggested) would introduce a real bug: unscoped DRIVE saved-query/export requests would be sent to an API that rejects them for that method, trading a clear client-side validation error for a confusing server-side one. create_saved_query.ts intentionally mirrors create_matters_export.ts's MAIL-only restriction because that's what the API actually supports — this is correct, not a bug. Keeping both as-is. |
|
@greptile review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0a96355. Configure here.
|
@greptile review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0a96355. Configure here.
Summary
create_matters_exportsent the deprecatedQuery.searchMethodfield (deprecated 2019, support ended June 2020) instead ofmethod— account/org-unit scoped exports were silently falling back to unscoped/broken behavior against the live APImatterIdsubBlock id that collided across operations1.0.0(repo convention) and fixeddocsLinkto point at docs.sim.ai instead of the vendor siteediscovery+devstorage.read_onlyOAuth scopes — no new scopes requestedType of Change
Testing
Tested manually: verified operation dropdown / tool.access / switch-case alignment (22/22/22), no duplicate subBlock ids,
bun run lintclean,bun run type-checkclean (only pre-existing unrelated@aws-sdk/client-textracterrors present).Checklist