Repository navigation
feat: create calendars without inviting members - #35
Conversation
Picks up the lockfile refresh from ehs208#36 so this PR no longer carries it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the missing Korean README entries, use a distinct emoji from Create Events, list calendar creation in the project guide, and drop the Security changelog entry now that ehs208#36 ships the lockfile refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ehs208
left a comment
There was a problem hiding this comment.
Thanks for this. The direction fits the project well: creating a calendar with no invitations, a strict input schema, and avoiding responses that leak upstream payloads.
I pushed two commits to your branch (thanks for allowing maintainer edits), so please pull before you push again:
- Merged
main. #36 shipped the same lockfile refresh, sopackage-lock.jsonno longer appears in this diff. - Synced the docs: added the Korean README entries, changed the emoji to 🗓️ so it differs from Create Events, listed calendar creation in
CLAUDE.md, and removed the### Securitychangelog entry because #36 covers it.
A few code changes are needed before merge:
1. Use the rate limiter (src/client/api.ts)
createCalendar is the only write that skips rateLimiter.executeWithRetry, so it also skips request throttling. The retry concern doesn't apply here: executeWithRetry retries only on 429, which means the server rejected the request without processing it, so a retry cannot create a duplicate. Timeouts and 5xx errors are thrown immediately. Please wrap the POST like createEvent does, and map 403 to TimeTreeAPIError the same way. Your existing "called once on failure" test still passes after this change.
2. Make errors distinguishable (src/tools/calendar-tools.ts)
Keeping upstream bodies and submitted names out of responses is the right goal. However, the bare catch {} currently gives a name over 20 characters, an invalid purpose, and an auth failure the same message ("Check existing calendars before retrying"), so the client can't tell what to fix. Suggested handling:
ZodError: report which field is invalid and why, without echoing the value.- 403 or auth failure: return "Authentication failed".
- Anything else: keep your current message, which fits timeouts well, and add
logger.error('Tool: create_calendar failed', { error }). The shared logger masks sensitive fields.
Please don't reuse commonWriteError from event-crud-tools.ts as is. Its 404 branch says "Event not found", and its fallback forwards error.message.
3. Make purpose required
With a default of lover, a request like "make me a calendar" silently creates a couple calendar for a work or personal use case. Requiring purpose (drop the default and add it to required) makes the client choose one explicitly.
Nits (optional):
- A one-line comment on
this.authManager.getCsrfToken();saying it is a precondition check would help. - A handler-level test would be nice: valid input returns only
{id, name, alias_code}, and invalid input returnsisError.
|
I merged Since this PR started,
The earlier review items (rate limiter, distinguishable errors, required |
# Conflicts: # CHANGELOG.md # README.ja.md # README.ko.md # README.md # package-lock.json # src/client/api.ts
|
Addressed the three review items and optional test/comment suggestions in
Added handler tests for strict validation, response shape, and error privacy, plus a CSRF precondition comment. Merged current main, preserved its revised READMEs, and added the tool to the desktop extension manifest. Refreshed the locked MCP SDK/proxy-addr versions because current audits otherwise reported high/critical advisories. Validation: a clean detached checkout passes Ready for another review. |
|
Follow-up to my previous review update: pushed
Added regression tests for 500/502/503/504 with both authentication error codes, successful authentication recovery, invalid success responses, and persistent authentication rejection. The only shared-client behavior change is the guard against authentication retries on server errors; the remaining changes cover calendar creation, tests, and matching documentation. Validation:
@codex review |
ehs208
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I reviewed 4eb58de: skipping re-authentication on 5xx, separating input validation from the write so a bad success response reports an uncertain outcome, and normalizing persistent auth rejections all look right. npm run verify (64 tests) and npm run build:mcpb pass locally. Approving.
Description
Add
create_calendarto create a calendar with an explicit name and purpose, without inviting members. Calendar creation uses the existing throttled request path and authentication recovery. Rejected requests may retry; timeouts and server errors are not retried.Invalid input returns safe field-level reasons, authentication failures return a distinct message, and uncertain failures tell the client to check existing calendars before retrying. Successful responses contain only calendar ID, name, and alias code.
Changes
nameandpurpose; no default calendar purpose.Validation
npm ci && npm run verifypasses on Node 22 and 24.974c7bf(run 37591429752).