refactor(calendar): moving validation logic outside of service - implements #313

[galuszkak]

First stab at this problem - did test it locally at mostly got good results. This should try solve #313

[gemini-code-assist]

Code Review

This pull request refactors the Google Calendar service to use the patch method for event updates, enabling partial updates and preserving unspecified fields. It introduces a centralized validation module, CalendarValidation.ts, which includes a new requirement that all-day workingLocation events must span exactly one day. Additionally, the update logic now supports clearing attachments by passing an empty array. Documentation and tests have been updated to reflect these changes. I have no feedback to provide.

[allenhutchison]

Thanks so much for tackling this, @galuszkak — #313 has been a gnarly one and the events.update → events.patch switch is exactly the right fix. I also really appreciate that you extracted CalendarValidation.ts as its own module; the service file has been due for that split for a while, and the new unit tests make future changes to the validation rules much easier to reason about.

A few specific things worth calling out as genuinely well done:

• The per-field if (x !== undefined) guards when building requestBody in updateEvent are the correct shape for patch semantics. I was looking for somewhere a null or [] might sneak through and I couldn't find one.
• The applyMeetAndAttachments(..., { allowEmptyAttachments }) flag cleanly encodes the asymmetric "create vs update" semantics without leaking into callers. Nice touch.
• The leap-day test (2024-02-29 → 2024-03-01) in CalendarValidation.test.ts is a nice catch, and because addDays does all its arithmetic in UTC, the month/year/DST boundaries are automatically correct.
• The SKILL.md updates mean users will actually discover the new behaviors.
• createValidationErrorResponse joining individual Zod issue.messages instead of the opaque ZodError.message JSON blob is a real UX improvement — thank you.

I left inline comments below with a handful of suggestions. Nothing is blocking and the core fix is solid; the requests are mostly about locking down the #313 fix so it can't silently regress, and about one diverging error message between the MCP schema and the service-layer validator that I noticed while reading. Happy to pair on any of this if it helps — and thanks again for the contribution!

[allenhutchison]

Small consistency thing I noticed: this refinement message says "${fieldName} must include exactly one of 'dateTime' or 'date'", but the matching validator in CalendarValidation.ts says "${fieldName} must have exactly one of 'dateTime' (for timed events) or 'date' (for all-day events)".

Because the MCP layer parses first, users going through the tool interface only ever see the shorter message from this file — which means the more helpful (for timed events)/(for all-day events) hint in the service-layer validator is effectively dead code for the tool path, and the test at CalendarValidation.test.ts:132-139 pins a string real users will never see via MCP.

Could we pick one canonical wording and use it in both places? I'd lean toward bringing the longer, more helpful wording up here so tool users get the better hint. No strong preference though — whichever you prefer, as long as they match.

[galuszkak]

Good point - changed!

[allenhutchison]

Now that this path calls events.patch instead of events.update, some new failure modes become reachable that weren't before (patching an all-day event over a timed one, failed attachment clears, etc.). The existing catch block just below returns error.message, which for a googleapis GaxiosError is usually a terse top-level string like "Invalid Value" — the genuinely useful field-level reasons live in error.response?.data?.error.errors[].

Would you mind folding the structured error into the response when we have one? Something roughly like:

const details = (error as { response?: { data?: { error?: { message?: string; code?: number; errors?: Array<{ location?: string; reason?: string; message?: string }> } } } })?.response?.data?.error;
const errorMessage = details
  ? `${details.message} (code ${details.code})` +
    (details.errors?.length
      ? `: ${details.errors.map(e => `${e.location ?? ''} ${e.reason}: ${e.message}`).join('; ')}`
      : '')
  : error instanceof Error ? error.message : String(error);

This isn't strictly a regression from this PR — the old update path had the same catch — but patch widens the set of errors users will actually hit, and the current message gives them nothing to act on when they do. Happy to pick whichever shape you prefer; the main ask is "surface Google's per-field error detail when it's there."

[galuszkak]

Addressed - I did help myself with Google documentation - and include also locationType.

[allenhutchison]

Nit for a follow-up (feel free to defer): joining Zod issues with '; ' here drops the issue.path array, so a user with two different field errors can't tell which message belongs to which field. Swapping to something like error.issues.map(i => i.path.length ? \${i.path.join('.')}: ${i.message}` : i.message).join('; ')` would give them the field context for free. Totally fine as a separate PR.

[allenhutchison]

Edge case worth handling: a user who accidentally swaps start and end on a workingLocation event — say start.date: '2026-04-06', end.date: '2026-04-05' — will fall into this branch and see "all-day workingLocation events must span exactly one day", which is technically true but pretty confusing when the real issue is that end is before start.

Could we add an explicit ordering check before the duration check, with its own message? Something like:

if (input.end.date < input.start.date) {
  throw createIssue(
    ['end', 'date'],
    'end.date must be on or after start.date',
  );
}

Minor but I think it'll save someone a confused bug report later.

[allenhutchison]

The leap-day test just below is great — could we add a handful more to pin the UTC-based addDays math against future refactors that might accidentally switch to local-time arithmetic? Suggested cases:

• Year boundary accept: start: 2024-12-31, end: 2025-01-01
• Month boundary accept: start: 2024-01-31, end: 2024-02-01
• Non-leap Feb accept: start: 2023-02-28, end: 2023-03-01
• Same-day reject: start: 2024-01-15, end: 2024-01-15
• Reversed-dates reject (pairs with the ordering-check comment on CalendarValidation.ts)

All of these pass today with the correct UTC math; the tests are purely to freeze the behavior so it can't silently regress.

[galuszkak]

Added all 5.

[allenhutchison]

Three small asks on this new test, grouped since they're all right here:

1. Test name. "should patch only changed fields for status events" is slightly misleading — the test body doesn't set eventType, so it's really another "only send provided fields" case. Could we either rename (e.g. drop the for status events part) or actually construct a focusTime/workingLocation update? I noticed because I was grepping for #313 coverage and the name threw me off.

2. Parametrized #313 regression test. The existing toHaveBeenCalledWith exact-match assertions give implicit coverage, but I think it's worth one explicit parametrized test that pins the guarantee field-by-field. A future refactor that accidentally defaults attendees: [] or description: '' into the patch body would be a direct #313 regression, and a test named after the issue will make the intent obvious:

it.each([
  ['summary',     { summary: 'X' }],
  ['description', { description: 'X' }],
  ['start',       { start: { dateTime: '2024-01-15T10:00:00Z' } }],
  ['end',         { end:   { dateTime: '2024-01-15T11:00:00Z' } }],
  ['attendees',   { attendees: ['a@b.com'] }],
])(
  '#313: patch body for %s update contains only that field',
  async (field, patch) => {
    mockCalendarAPI.events.patch.mockResolvedValue({ data: { id: 'e' } });
    await calendarService.updateEvent({ eventId: 'e', ...patch });
    const body = mockCalendarAPI.events.patch.mock.calls[0][0].requestBody;
    expect(Object.keys(body)).toEqual([field]);
  },
);

3. Guard that events.update is never called again. One-liner complement:

afterEach(() => {
  expect(mockCalendarAPI.events.update).not.toHaveBeenCalled();
});

If any future branch in updateEvent ever reintroduces events.update (e.g. as an error-recovery fallback), this catches it at PR time. Since this whole PR is about moving off events.update, it feels right to pin that invariant.

[galuszkak]

Nice trick with afterEach! Added.

[allenhutchison]

Optional follow-up: since this PR introduces explicit attachments: [] clearing semantics, it'd be nice to pin the parallel behaviors for the other fields too:

• description: '' — current code sends description: '' (clears it), which seems intentional but is untested.
• attendees: [] — current code sends attendees: [] (which Google interprets as "clear all attendees"), also untested and undocumented.

Just one test each to lock in whichever semantics you intend. Happy for this to be a separate PR.

[galuszkak]

added - actually this was really good point.

[allenhutchison]

The rejection path for multi-day workingLocation is tested here, and the single-day happy path is covered at the unit level in CalendarValidation.test.ts — but I don't see an integration-style test on CalendarService that proves a valid single-day all-day workingLocation event actually survives validation and reaches events.insert with the right body. Something like:

it('creates a single-day all-day workingLocation event', async () => {
  mockCalendarAPI.events.insert.mockResolvedValue({ data: { id: 'wl1' } });
  await calendarService.createEvent({
    start: { date: '2024-01-15' },
    end: { date: '2024-01-16' },
    eventType: 'workingLocation',
    workingLocationProperties: { type: 'homeOffice' },
  });
  const body = mockCalendarAPI.events.insert.mock.calls[0][0].requestBody;
  expect(body.start).toEqual({ date: '2024-01-15' });
  expect(body.end).toEqual({ date: '2024-01-16' });
  expect(body.eventType).toBe('workingLocation');
});

Without this, a future tightening of the validator that accidentally rejects single-day input would only fail unit tests, not the integration surface.

[galuszkak]

I think this is ready for re-review. I did updated CONTRIBUTING.md - because when working with agent he always forget to run prettier - which wasn't mentioned in CONTRIBUTING.md but it is in Github Action check...