Skip to content

fix(calendar): surface Zoom rollback errors after failed writes - #1144

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f029-zoom-rollback-errors
Open

SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f029-zoom-rollback-errors

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

gog calendar create --with-zoom (and update paths that create a Zoom meeting, then patch Calendar) can leave a Zoom meeting behind when the Calendar write fails. The CLI already calls cancelZoomMeeting, but it discards that error with _ =, so the leftover meeting is silent behind the Calendar 500.

--remove-zoom still warns and continues. That path is unchanged.

Evidence

httptest returned HTTP 500 for calendar insert/patch after Zoom create. The fake Zoom client failed delete with zoom delete denied.

Before this patch, on origin/main 469b823, Kong only reported the Calendar failure:

error = googleapi: got HTTP response code 500 with body: calendar insert failed
        , want rollback failure wrapping zoom delete denied
error = googleapi: got HTTP response code 500 with body: calendar update failed
        , want rollback failure wrapping zoom delete denied

After this patch, the same 500 plus failed Zoom delete is joined:

=== RUN   TestCalendarCreateCmd_WithZoomInsertFailureSurfacesRollbackError
--- PASS: TestCalendarCreateCmd_WithZoomInsertFailureSurfacesRollbackError (0.00s)
=== RUN   TestCalendarUpdateCmd_WithZoomPatchFailureSurfacesRollbackError
--- PASS: TestCalendarUpdateCmd_WithZoomPatchFailureSurfacesRollbackError (0.00s)
PASS

The error text includes Zoom meeting rollback failed and zoom delete denied. Zoom create count is 1 and delete was attempted once.

Real behavior proof

  • Behavior or issue addressed: Calendar insert/update discarded Zoom rollback errors. If Calendar fails after Zoom create, the operator now sees both the Calendar error and the Zoom delete failure.

  • Real environment tested: macOS, Go 1.27.1, patched tree on fix/f029-zoom-rollback-errors. Calendar and Zoom HTTP were httptest fakes used by the existing calendar Zoom command path (runKong on CalendarCreateCmd / CalendarUpdateCmd).

  • Exact steps or command run after this patch:

    Ran calendar create --with-zoom against a Calendar POST that returns 500, with Zoom delete returning zoom delete denied. Repeated for calendar update --with-zoom where GET succeeds and PATCH returns 500.

  • Evidence after fix: terminal output from the patched command path:

    === RUN   TestCalendarCreateCmd_WithZoomInsertFailureSurfacesRollbackError
    --- PASS: TestCalendarCreateCmd_WithZoomInsertFailureSurfacesRollbackError (0.00s)
    === RUN   TestCalendarUpdateCmd_WithZoomPatchFailureSurfacesRollbackError
    --- PASS: TestCalendarUpdateCmd_WithZoomPatchFailureSurfacesRollbackError (0.00s)
    PASS
    

    Both errors contain Zoom meeting rollback failed and zoom delete denied.

  • Observed result after fix: Create and update wrap errors.Join(calendarErr, cancelErr) instead of returning only the Calendar 500. --remove-zoom still warns and continues.

  • What was not tested: A live Zoom API plus live Google Calendar write. The production cancelZoomMeeting call is the same helper used by regenerate/delete; this change only stops discarding its error on the failed-write rollback path.

Create and update discarded cancelZoomMeeting errors with _.
If insert or patch fails after a Zoom meeting was created, include
the rollback error so the leftover meeting is not silent.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 20, 2026
@clawsweeper

clawsweeper Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 20, 2026, 4:10 PM ET / 20:10 UTC.

ClawSweeper review

What this changes

The PR combines failed Calendar writes with failed Zoom cleanup errors and adds regression tests for create and update commands.

Merge readiness

Blocked before merge - 3 items remain

The fix is still needed, but the new combined error loses its rollback details in the CLI’s display formatter. The supplied tests miss that boundary, and real behavior proof remains outstanding.

Likely related people: steipete (routing candidate, medium confidence).

Priority: P2
Reviewed head: 8f55a412a71511deb0ca28e3135d758259642302

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The change is focused, but its intended diagnostic remains hidden in normal CLI output and the proof is limited to mocked tests.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Needs real behavior proof before merge: the supplied Calendar create/update tests use a fake Zoom client and stop before final error display. Add after-fix terminal output or redacted logs from the production command and real transport path showing both failures on stderr; a terminal screenshot or recording also counts. Redact credentials, private endpoints, and account details. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. 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.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Needs real behavior proof before merge: the supplied Calendar create/update tests use a fake Zoom client and stop before final error display. Add after-fix terminal output or redacted logs from the production command and real transport path showing both failures on stderr; a terminal screenshot or recording also counts. Redact credentials, private endpoints, and account details. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. 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 6 items Pinned introduced change: The verified base-to-head delta changes two rollback returns and adds two tests; it does not change the CLI error formatter or CHANGELOG.md.
Final display drops the combined error: The CLI's errorMessage calls errfmt.Format. That formatter uses errors.As to extract a Google API error and formats only that error, discarding the new outer message and joined Zoom failure. Its existing UserFacingError mechanism can preserve an explicit message and underlying causes.
Current main and latest release still need the fix: Inspection of fetched main and the supplied v0.40.0 release commit found both rollback errors still discarded. GitHub searches for Zoom rollback and cleanup found no competing fix established by the available results.
Findings 1 actionable finding [P2] Preserve rollback details through the CLI error formatter
Security None None.

How this fits together

Calendar commands create a Zoom meeting before writing its details to Google Calendar. If that write fails, they attempt to delete the meeting and pass any error through the CLI’s display formatter.

flowchart TD
 A[Calendar create or update] --> B[Create Zoom meeting]
 B --> C[Write Google Calendar event]
 C --> D{Write succeeded?}
 D -->|Yes| E[Display event]
 D -->|No| F[Delete Zoom meeting]
 F --> G[Combine write and cleanup errors]
 G --> H[Format CLI error]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Needs real behavior proof before merge: the supplied Calendar create/update tests use a fake Zoom client and stop before final error display. Add after-fix terminal output or redacted logs from the production command and real transport path showing both failures on stderr; a terminal screenshot or recording also counts. Redact credentials, private endpoints, and account details. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. 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.
  • Preserve rollback details through the CLI error formatter (P2) - When Calendar returns an HTTP error and Zoom deletion also fails, this joined error still contains a *googleapi.Error. The production CLI calls errorMessage in internal/cmd/root.go, and errfmt.Format extracts that Google error with errors.As and renders only it. Consequently, users still never see the rollback failure. The update return has the same problem. Use the existing user-facing error wrapper while preserving the joined causes, and test formatted stderr; the new runKong assertions inspect only err.Error() and bypass this boundary.
  • Complete next step (P2) - Repair the final error display for both rollback paths, cover formatted stderr, and add the repository-required Unreleased changelog entry before landing.

Findings

  • [P2] Preserve rollback details through the CLI error formatter — internal/cmd/calendar_edit.go:172-173
Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Preserve both failures through the existing user-facing error mechanism, retain error unwrapping, and verify the message at the final stderr boundary.

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

Yes, by source inspection: a Calendar API failure followed by a Zoom deletion failure reaches the new joined error, but the final formatter extracts only the Google API error. No runtime reproduction was executed.

Is this the best way to solve the issue?

No, the patch stops before the user-visible boundary; use the existing UserFacingError pattern and assert formatted stderr, rather than only err.Error().

Full review comments:

  • [P2] Preserve rollback details through the CLI error formatter — internal/cmd/calendar_edit.go:172-173
    When Calendar returns an HTTP error and Zoom deletion also fails, this joined error still contains a *googleapi.Error. The production CLI calls errorMessage in internal/cmd/root.go, and errfmt.Format extracts that Google error with errors.As and renders only it. Consequently, users still never see the rollback failure. The update return has the same problem. Use the existing user-facing error wrapper while preserving the joined causes, and test formatted stderr; the new runKong assertions inspect only err.Error() and bypass this boundary.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 469b823aead2.

Labels

Label changes:

  • add P2: This is a bounded diagnostic fix for failed cleanup after Calendar writes, with no demonstrated urgent regression.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs real behavior proof before merge: the supplied Calendar create/update tests use a fake Zoom client and stop before final error display. Add after-fix terminal output or redacted logs from the production command and real transport path showing both failures on stderr; a terminal screenshot or recording also counts. Redact credentials, private endpoints, and account details. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. 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:

  • P2: This is a bounded diagnostic fix for failed cleanup after Calendar writes, with no demonstrated urgent regression.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs real behavior proof before merge: the supplied Calendar create/update tests use a fake Zoom client and stop before final error display. Add after-fix terminal output or redacted logs from the production command and real transport path showing both failures on stderr; a terminal screenshot or recording also counts. Redact credentials, private endpoints, and account details. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. 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

What I checked:

  • Pinned introduced change: The verified base-to-head delta changes two rollback returns and adds two tests; it does not change the CLI error formatter or CHANGELOG.md. (internal/cmd/calendar_edit.go:173, 8f55a412a715)
  • Final display drops the combined error: The CLI's errorMessage calls errfmt.Format. That formatter uses errors.As to extract a Google API error and formats only that error, discarding the new outer message and joined Zoom failure. Its existing UserFacingError mechanism can preserve an explicit message and underlying causes. (internal/errfmt/errfmt.go:70, 8f55a412a715)
  • Current main and latest release still need the fix: Inspection of fetched main and the supplied v0.40.0 release commit found both rollback errors still discarded. GitHub searches for Zoom rollback and cleanup found no competing fix established by the available results. (internal/cmd/calendar_edit.go:172, 469b823aead2)
  • Proof exercises a mocked Zoom client: The complete supplied PR body reports macOS test PASS output using runKong, an HTTP Calendar fixture, and a fake Zoom client. The added tests inspect err.Error(), while runKong returns kctx.Run() without the production display formatter. The body expressly excludes live Google Calendar and Zoom validation; its captured sourceRevision is e5e79875bb602989c9f8cbb876a5d8f6b996dfd83eee8ba3cbcdfe0a7f03796d. (internal/cmd/calendar_zoom_test.go:135, 8f55a412a715)
  • Area routing history: Local history identifies Peter Steinberger's Zoom client injection and credential-store refactors; GitHub commit metadata maps the client-injection author to steipete. Deeper pickaxe and blame inspection could not complete because older blobs were unavailable, so no source-line introduction attribution is asserted. (internal/cmd/calendar_zoom.go, 529925322ddb)
  • Repository review and release-note policy: The root AGENTS.md was read fully; no nested AGENTS.md or maintainer-notes directory was found. User-visible fixes require an Unreleased changelog entry with references and contributor thanks when landing. The checkout remained clean; tests were not executed during this read-only review. (AGENTS.md:36, 8f55a412a715)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Preserve both failures in final stderr and add coverage through the production formatter.
  • Supply redacted after-fix evidence using the production command and real transport clients.
  • Add the required Unreleased changelog entry with the PR reference and contributor thanks before landing.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

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

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant