fix(sheets): never write outside the requested range on positional updates - #941
fix(sheets): never write outside the requested range on positional updates#941cathrynlavery wants to merge 2 commits into
Conversation
…dates - single-cell ranges preserve the positional value literally (commas and pipes are no longer treated as row/cell separators), fixing silent overwrite of neighboring live cells - multi-cell positional updates validate parsed shape against the range and error loudly on mismatch instead of fanning out past the range - links set --cells-json now accepts @file and @- like its sibling flags - command-level HTTP recorder tests prove request bodies and that no update call is issued on shape errors
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 30, 2026, 7:53 AM ET / 11:53 UTC. ClawSweeper reviewWhat this changesThe PR makes positional Sheets updates preserve literal single-cell text, rejects positional matrices that exceed finite target-range bounds, resolves named ranges before checking shape, and permits Merge readinessKeep this PR open for maintainer review: it fixes a source-reproducible Sheets overwrite path, has focused regression coverage and credible redacted live proof, but intentionally changes compatibility for oversized positional payloads targeting finite ranges. Priority: P1 Review scores
Verification
How this fits togetherThe Sheets update command parses a user-provided A1 or named range and a positional value matrix before sending it to the Google Sheets Values API. This change adds range-aware validation between command parsing and the API write so a bounded update cannot expand into neighboring cells. flowchart LR
A[CLI Sheets update command] --> B[Parse A1 or named range]
B --> C{Range shape known?}
C -->|Named range| D[Load spreadsheet range catalog]
C -->|A1 range| E[Parse positional values]
D --> E
E --> F[Check finite range bounds]
F --> G[Google Sheets Values API]
G --> H[Requested spreadsheet cells]
Decision needed
Why: The patch prevents destructive writes and preserves partial writes, but it deliberately changes behavior for callers that relied on oversized payloads extending beyond a finite target range. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt bounded positional writes if maintainers accept that finite ranges are a hard safety boundary, with the existing partial-write behavior retained and the changed overflow behavior called out in release-facing review context. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current Is this the best way to solve the issue? Yes, subject to the compatibility decision: range-aware parsing before the Values API is the narrowest way to prevent unintended expansion while the revised patch preserves supported smaller partial writes. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1b261244fc26. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
… range Address review: finite-range validation now rejects only matrices that EXCEED the range's row/column bounds instead of requiring exact shape equality. Partial positional updates (e.g. one value into Sheet1!A1:B2), which current main and the Values API support, pass through unchanged. Single-cell literal preservation and the pre-API oversize rejection are unchanged. Regression tests added for the partial-write case and for row/column overflow.
|
@clawsweeper re-review Both blockers addressed in |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
1 participant
Problem
gog sheets update <id> "G31" "text, with, commas"parses the positional value as comma-separated rows, so a single-cell write fans out down the column and silently overwrites neighboring cells. We hit this in production against a live tracker sheet.Fix
--values-jsonand append behavior unchanged.links set --cells-jsonnow accepts@fileand@-(stdin), matching its sibling flags; docs updated.Real behavior proof (redacted)
Built from this branch (
v0.34.2-0.20260727222104+dirty), run against a disposable test spreadsheet created for this proof; spreadsheet ID redacted:All three review asks demonstrated live: (1) comma-containing single-cell update lands literally in exactly one cell with neighbors untouched, (2) oversize payload rejected before the API call (exit 2), (3) partial positional write within a larger finite range succeeds — the P1 case.
Tests
go test ./internal/sheetsvalues,go test ./internal/cmd -run 'TestSheetsUpdatePositional|TestSheetsLinksSet', andgo test ./...all green (42 packages). New coverage:TestSheetsUpdatePositionalPartialMultiCellRangeWritesOneCell(asserts the recorder receives exactly[["one"]]),TestParseArgsForShapeAllowsSmallerMatrix,TestParseArgsForShapeRejectsExceedingRangeBounds, plus the existing single-cell literal and zero-write-on-error assertions.