fix(sheets): never write outside the requested range on positional updates - #941
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 August 2, 2026, 3:57 AM ET / 07:57 UTC. ClawSweeper reviewWhat this changesThe branch makes positional Google Sheets updates range-aware, preserving comma-bearing single-cell text, rejecting finite-range overflow before the Values API update, resolving named ranges for live validation, and accepting Merge readinessThis PR is still necessary: current Priority: P1 Review scores
Verification
How this fits togetherThe Sheets CLI converts positional or JSON input into a matrix sent to the Google Sheets Values API. This patch changes the positional-input path between range parsing and the update request so the payload cannot silently extend outside a finite target range. flowchart LR
Input[CLI range and positional values] --> Parse[Parse positional matrix]
Parse --> Range[A1 or named-range resolution]
Range --> Bounds[Validate finite range bounds]
Bounds --> Preview[Offline dry-run or live request]
Preview --> API[Google Sheets Values API]
API --> Result[Updated spreadsheet cells]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the focused range-aware positional parsing change and retain the explicit named-range dry-run refusal, while documenting that scripts needing an offline named-range preview should use A1 notation or Do we have a high-confidence way to reproduce the issue? Yes. Current main's positional parser splits commas and pipes without inspecting the requested range, and the supplied live transcript demonstrates the resulting single-cell overwrite scenario plus the fixed behavior on the branch. Is this the best way to solve the issue? Yes. Range-aware validation at the positional parsing boundary is the narrowest maintainable fix: it preserves the existing delimiter syntax for multi-cell updates, allows smaller valid matrices, and stops only writes that exceed finite bounds. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3c9466af5171. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (21 earlier review cycles; latest 8 shown)
|
… 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
|
Maintainer follow-up: I fixed the remaining named-range dry-run mismatch on the contributor branch in 2c88862. Positional updates to named or otherwise unresolved ranges now refuse an offline I also cleared the branch's five lint failures in the touched code/tests. Proof:
This clears the concrete review blocker. My recommendation is LAND: the branch fixes a live-proven neighboring-cell overwrite, preserves partial-write compatibility, and now has accurate dry-run behavior. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
rating: 🦞 diamond lobster
2 participants
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.