Conversation
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>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 20, 2026, 4:10 PM ET / 20:10 UTC. ClawSweeper reviewWhat this changesThe 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 Review scores
Verification
How this fits togetherCalendar 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]
Before merge
Findings
Agent review detailsSecurityNone. Review metricsNone. Technical reviewBest 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 469b823aead2. LabelsLabel changes:
Label 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
|
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 callscancelZoomMeeting, but it discards that error with_ =, so the leftover meeting is silent behind the Calendar 500.--remove-zoomstill 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/main469b823, Kong only reported the Calendar failure:After this patch, the same 500 plus failed Zoom delete is joined:
The error text includes
Zoom meeting rollback failedandzoom 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 (runKongonCalendarCreateCmd/CalendarUpdateCmd).Exact steps or command run after this patch:
Ran
calendar create --with-zoomagainst a Calendar POST that returns 500, with Zoom delete returningzoom delete denied. Repeated forcalendar update --with-zoomwhere GET succeeds and PATCH returns 500.Evidence after fix: terminal output from the patched command path:
Both errors contain
Zoom meeting rollback failedandzoom delete denied.Observed result after fix: Create and update wrap
errors.Join(calendarErr, cancelErr)instead of returning only the Calendar 500.--remove-zoomstill warns and continues.What was not tested: A live Zoom API plus live Google Calendar write. The production
cancelZoomMeetingcall is the same helper used by regenerate/delete; this change only stops discarding its error on the failed-write rollback path.