fix(sheets): correct Connected Sheets sheet identity and SYNC_ALL extract reads - #1001
Merged
steipete merged 4 commits intoAug 23, 2026
Merged
Conversation
…ce id Live spreadsheets.get responses omit Spreadsheet.dataSources[].sheetId, so `sheets datasource list` reported sheetId 0 for every data source, and the sheet lookup could latch onto an unrelated tab whose sheet id happens to be 0 and report that tab's title and execution status instead. Match the linked sheet on its data source id first, fall back to the sheet id only when the API actually supplied one, and treat the resolved sheet's own id as authoritative, mirroring what the extract listing already does. The fixture hardcoded dataSources[].sheetId, which no live response carries; drop it and add a decoy tab with sheet id 0 so the regression stays covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A ranged spreadsheets.get only returns the sheets its ranges intersect, so the anchor lookup never carries the separate DATA_SOURCE sheet that holds a SYNC_ALL table's column list. Every extract configured to sync all columns failed with "cannot determine columns for data-source table". Fall back to an unranged, properties-only fetch when the ranged response cannot supply the column count. The fixture server returned the whole spreadsheet regardless of ranges, and its only extract used SELECTED columns, so neither half of this path was exercised. Make the fixture server range-aware and add a SYNC_ALL extract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 22, 2026, 9:30 AM ET / 13:30 UTC. ClawSweeper reviewWhat this changesThe PR fixes Connected Sheets data-source identity resolution and adds a narrow metadata fetch so SYNC_ALL extracts can be read, with regression fixtures and user guidance. Regression provenancePossible regression — probable (reproduction; reviewed change; known regression link). No predecessor PR is attributed. Merge readiness✅ Ready for maintainer review This PR remains necessary: current main still mis-resolves an omitted data-source sheet ID and cannot read SYNC_ALL extracts whose columns live on a separate DATA_SOURCE sheet. The focused patch has no actionable correctness or security finding and is ready for maintainer review. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherConnected Sheets commands fetch spreadsheet metadata and extract anchors from the Sheets API, then derive a bounded values range for CLI output. The linked data-source sheet supplies identity, status, and sometimes the column count needed for that read. flowchart LR
A[Google spreadsheet] --> B[Connected Sheets metadata]
B --> C[Linked data-source sheet]
B --> D[Extract anchor]
C --> E[Column count]
D --> E
E --> F[Bounded values read]
F --> G[CLI JSON or table output]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the targeted compatibility repair with its regression coverage and the documented pacing guidance for bulk SYNC_ALL reads. Do we have a high-confidence way to reproduce the issue? Yes—source inspection gives a high-confidence path: a range-scoped lookup omits the DATA_SOURCE sheet needed for SYNC_ALL column counting, and current main returns an error. The PR fixture directly models that path. Is this the best way to solve the issue? Yes—the branch keeps the existing CLI contract, uses the API's data-source ID as the primary identity, and requests only missing column metadata when needed. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ab7e0ed706f9. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
The unranged fallback added for SYNC_ALL extracts reused the full snapshot field mask, which also pulls dataSources and dataSourceSchedules even though only the data-source sheet column definitions are needed. Request just those columns so the extra call stays small, and describe the call and its pacing implications in the Connected Sheets guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add a direct table-driven test for findSheetsDataSourceSheet: the fixture no longer supplies dataSources[].sheetId, so neither the data-source-id-first ordering nor the sheet-id fallback was exercised through the command surface. Assert how often the unranged column lookup fires — once for a SYNC_ALL extract, never for a SELECTED one. Running it unconditionally would not just cost a request; it would also overwrite a SELECTED table's own column count and widen the read range. Also stop calling t.Fatalf from the fixture server's handler goroutine, where runtime.Goexit turns an assertion into a confusing handler abort, and drop a redundant PathUnescape: net/http already decodes r.URL.Path, so re-decoding would reject sheet names containing '%'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Refs #938
Live validation of the read-only Connected Sheets surface from #989, against a real set of
BigQuery-backed spreadsheets, surfaced two defects. Both are invisible to the current tests
because the fixture does not match what the API actually returns.
Summary
authoritative, so
sheets datasource liststops reportingsheetId: 0spreadsheets.getcannot supplyan extract's column count, which unblocks every
SYNC_ALLextractdataSources[].sheetIdfrom the fixture, so bothpaths are exercised the way the live API behaves
Details
datasource listalways reportedsheetId: 0. Livespreadsheets.getresponses do notpopulate
Spreadsheet.dataSources[].sheetId; it comes back null.sheetsDataSourceToItememittedsource.SheetIddirectly. The correct id is already available on the matched sheet, anddatasource table listuses it, so the two code paths now agree.That same zero value also made
findSheetsDataSourceSheetunsafe: it comparedproperties.SheetId == source.SheetIdbefore checking the data source id, so in a spreadsheetcontaining a tab with sheet id 0, every data source would match that unrelated tab and report its
title and
dataExecutionStatus. I could not reproduce this in practice — none of the 59spreadsheets I scanned has a tab with sheet id 0 — so I am flagging it as latent rather than
observed. The ordering is wrong either way.
datasource table readfailed for everySYNC_ALLextract. ASYNC_ALLtable carries noinline column list, so the column count has to come from
dataSourceSheetPropertieson theassociated
DATA_SOURCEsheet. The anchor lookup passesranges, and a rangedspreadsheets.getreturns only the sheets those ranges intersect, which excludes the
DATA_SOURCEsheet:So
dataSourceColumnCountfound nothing and the command exited withcannot determine columns for data-source table.Proof
make cigo test ./internal/cmd -run 'TestSheetsDataSource' -count=1 -vtest reports the decoy tab, and the SYNC_ALL read test reproduces the exact
cannot determine columnserrorLive validation was read-only throughout (
--readonlyon every invocation; no spreadsheet wasmodified):
datasource table readCorpus: 59 spreadsheets scanned, 21 with Connected Sheets, 54 anchored extracts. Every extract in
it uses
SYNC_ALL; there was not a singleSELECTEDextract, which is why this path failing wastotal rather than partial.
Two notes:
SYNC_ALLfallback costs a secondspreadsheets.geton that path. Reading all 54 extractsback to back tripped Sheets rate limits a few times; pacing the loop cleared it. Worth knowing
before scripting bulk extract reads.
docs/sheets-connected.mdpresents--extra-scopes .../bigquery.readonly --force-consentas arequired step. It is not required when the stored token already carries a superset: all of the
above ran on a token holding
cloud-platformandspreadsheets, with no re-consent. That maybe a cheaper route to a live Connected Sheets environment than provisioning the exact scopes.