Skip to content

feat(sheets): verify formula errors structurally#849

Merged
steipete merged 1 commit into
openclaw:mainfrom
alexknowshtml:feat/sheets-safe-formula
Jun 21, 2026
Merged

feat(sheets): verify formula errors structurally#849
steipete merged 1 commit into
openclaw:mainfrom
alexknowshtml:feat/sheets-safe-formula

Conversation

@alexknowshtml

@alexknowshtml alexknowshtml commented Jun 19, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

Summary

  • use the existing --values-json @file / @- path for shell-safe formula input
  • add --fail-on-formula-error to read back the exact updated range as typed grid data
  • fail on real Sheets formula errors while preserving RAW literal strings such as #REF!
  • return structured formulaErrors entries with cell, type, and message
  • document the workflow and extend disposable live coverage

This replaces the proposed --safe-formula input path with the command's canonical file/stdin input and replaces displayed-text matching with effectiveValue.errorValue inspection.

Verification

  • make ci
  • autoreview: clean against main
  • focused tests for file input, typed errors, RAW literals, and readback failures
  • disposable live Google Sheets run covering a valid file formula, RAW literal error text, and a failing divide-by-zero formula

@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 9:37 AM ET / 13:37 UTC.

Summary
This PR adds --safe-formula and --verify to gog sheets update, reading single formulas from files and optionally reading the updated range back for spreadsheet error tokens.

Reproducibility: not applicable. as a feature PR. Source inspection shows current main already supports file/stdin JSON input through --values-json, but no live Google Sheets behavior was demonstrated for the new flags.

Review metrics: 2 noteworthy metrics.

  • User-facing flags: 2 added. Both flags change the CLI surface and need docs, tests, and live Google proof before merge.
  • Changed surface: 1 code file changed, 0 tests/docs changed. The branch adds behavior without regression coverage or regenerated command documentation.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted live Google Sheets proof for --safe-formula and --verify, preferably terminal output or a terminal screenshot showing command and readback; updating the PR body should trigger a fresh review, or a maintainer can comment @clawsweeper re-review.
  • Resolve whether --safe-formula should be removed in favor of --values-json @file or explicitly approved as a separate API.
  • [P1] Add focused httptest coverage and regenerated command docs for any flags that remain.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body has examples but no terminal output, logs, screenshot, recording, or linked artifact from a real Google Sheets run; contributor proof with private data redacted is required. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging --safe-formula as-is creates a second file-input path for Sheets values with different path expansion, stdin, and validation behavior than --values-json @file.
  • [P1] The PR has no live Google Sheets output, log, screenshot, recording, or linked artifact proving formula write and verify behavior against the API.
  • [P1] Generated command docs and focused regression coverage are absent, so the new flags would be easy to miss and harder to stabilize.

Maintainer options:

  1. Use the existing values file path (recommended)
    Remove or defer --safe-formula, document --values-json @file/@- for formula writes, and keep any verification work on the existing values parser.
  2. Approve a separate formula API
    If maintainers want the convenience flag, require explicit API direction plus tests, generated docs, and live proof showing why the existing JSON file input is not enough.
  3. Pause until proof is available
    Hold the PR until the contributor adds redacted live command output or a terminal screenshot demonstrating both flags against a real sheet.

Next step before merge

  • [P1] Needs contributor real behavior proof and maintainer direction on the overlapping Sheets update flags before any merge path.

Security
Cleared: The diff adds opt-in local file reads and an opt-in Sheets readback, with no concrete secret-handling, dependency, workflow, or supply-chain regression found.

Review findings

  • [P1] Avoid adding a second formula file input path — internal/cmd/sheets.go:180
  • [P3] Regenerate docs for the new flags — internal/cmd/sheets.go:180-181
Review details

Best possible solution:

Keep one clear Sheets update file-input story, then add any verification behavior with generated docs, focused tests, and redacted live Google Sheets proof.

Do we have a high-confidence way to reproduce the issue?

Not applicable as a feature PR. Source inspection shows current main already supports file/stdin JSON input through --values-json, but no live Google Sheets behavior was demonstrated for the new flags.

Is this the best way to solve the issue?

No. --verify may be useful, but --safe-formula duplicates the existing --values-json @file/stdin path unless maintainers explicitly choose a separate formula-file API.

Full review comments:

  • [P1] Avoid adding a second formula file input path — internal/cmd/sheets.go:180
    sheets update already reads values from a file or stdin through --values-json @file/@- via resolveInlineOrFileBytes, and the command already defaults to USER_ENTERED. Adding --safe-formula creates a parallel single-cell parser with different path expansion, stdin, and validation semantics unless the existing JSON file path is shown to be insufficient.
    Confidence: 0.86
  • [P3] Regenerate docs for the new flags — internal/cmd/sheets.go:180-181
    The PR adds user-visible sheets update flags but does not update the generated command docs, so docs/commands/gog-sheets-update.md will omit them after merge. Include the generated docs update for whichever flags remain.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against d2e0bd74809d.

Label changes

Label changes:

  • add P3: This is a low-risk Sheets ergonomics feature request, not an urgent regression or data-loss/security issue.
  • add merge-risk: 🚨 other: The branch adds a parallel Sheets formula file-input path whose API overlap and divergent semantics are not settled by green CI.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body has examples but no terminal output, logs, screenshot, recording, or linked artifact from a real Google Sheets run; contributor proof with private data redacted is required. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P3: This is a low-risk Sheets ergonomics feature request, not an urgent regression or data-loss/security issue.
  • merge-risk: 🚨 other: The branch adds a parallel Sheets formula file-input path whose API overlap and divergent semantics are not settled by green CI.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body has examples but no terminal output, logs, screenshot, recording, or linked artifact from a real Google Sheets run; contributor proof with private data redacted is required. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • PR diff adds two user-facing flags: The branch adds SafeFormula and Verify fields plus readback logic in SheetsUpdateCmd. (internal/cmd/sheets.go:180, 5cdc419ed6a6)
  • Current main already has file/stdin value input: resolveInlineOrFileBytes supports literal input, -, @file, and @- for flags that otherwise require shell-escaped JSON strings. (internal/cmd/input_spec.go:12, d2e0bd74809d)
  • Sheets update uses the existing file input helper: Current main wires --values-json through resolveInlineOrFileBytes and defaults sheets update to USER_ENTERED. (internal/cmd/sheets.go:176, d2e0bd74809d)
  • Generated command docs are not updated: The current generated gog sheets update command page lists --values-json and --input, but the PR changes no docs files for the new flags. (docs/commands/gog-sheets-update.md:17, d2e0bd74809d)
  • No real behavior proof in discussion: The PR body contains examples, and gh pr view showed no contributor comments, reviews, logs, screenshots, recordings, or linked artifacts demonstrating a real Google Sheets run.
  • History for routing: Recent Sheets update and input-helper history is concentrated in commits touching internal/cmd/sheets.go, internal/cmd/input_spec.go, and internal/sheetsvalues/values.go. (internal/cmd/sheets.go:172)

Likely related people:

  • steipete: Recent commits refactored Sheets value parsing, structured input readers, and the current sheets update implementation that this PR changes. (role: recent area contributor; confidence: high; commits: 47cb3c6b3058, f51bb37a4d06, 508a4d3cd368; files: internal/cmd/sheets.go, internal/cmd/input_spec.go, internal/sheetsvalues/values.go)
  • Tsopic: Authored the merged Sheets batch value update feature, which shares the same values JSON and Sheets write surface. (role: adjacent feature contributor; confidence: medium; commits: f911d6b21835; files: internal/cmd/sheets.go, docs/commands/gog-sheets-batch-update.md)
  • chrischall: Authored recent merged Sheets cell-operation work including formula-oriented copy-paste behavior near this command area. (role: adjacent Sheets contributor; confidence: medium; commits: 1ad331ba0725, 6b79be045844; files: internal/cmd/sheets.go, internal/cmd/sheets_copy_paste.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 19, 2026
@steipete @alexknowshtml @claude
feat(sheets): verify formula errors structurally
9c0a8a1 
Co-authored-by: Alex Hillman <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>
@steipete
steipete force-pushed the feat/sheets-safe-formula branch from 5cdc419 to 9c0a8a1 Compare June 21, 2026 23:40
@steipete steipete changed the title feat(sheets): add --safe-formula and --verify flags to update command feat(sheets): verify formula errors structurally Jun 21, 2026
@steipete
steipete merged commit 4d3862a into openclaw:main Jun 21, 2026
5 checks passed
@steipete

steipete commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

Landed in 4d3862a.

I kept the core goal and rewrote the implementation around the existing --values-json @file / @- input path. Formula verification now reads typed effectiveValue.errorValue data for the exact updated range, returns cell/type/message details, fails on readback errors, and does not misclassify RAW literal error-looking strings. Docs, focused regressions, disposable live Sheets proof, local CI, autoreview, and all hosted jobs are green.

Thanks @alexknowshtml for the contribution and for identifying the shell-safe formula and verification gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexknowshtml @steipete