Repository navigation
Throw typed errors from updateFields - #26
Merged
Merged
Conversation
Every refusal was a plain Error, so a consumer could only tell a caller's mistake from a library failure by matching message prefixes, and any rewording silently turned refusals into internal errors. Refusals now throw UpdateFieldsError with a stable code, the property concerned and, for a refused series move, the suggested RRULE. Messages are unchanged; failures that are not the caller's stay plain Errors. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The guard checks name and code instead of instanceof so it holds when the ESM and the CommonJS build are both loaded; only the packed tarball shows whether the class and guard actually ship in both. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Consumers branch on the codes, so they need the contract: which code a refusal carries and that messages may change while codes do not. Closes #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
UpdateFieldsError, its codes and the guard are new exported API; in 0.x that is a minor bump. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An instant written to a property in a zone was always rewritten as UTC. A Berlin series moved with a UTC DTSTART thus became a UTC series, and after the DST change every occurrence drifted an hour in local time. With absoluteTime "keep-zone" the instant is written as its wall clock in the TZID that applies, which stays; an instant in the second pass of the repeated hour has no such wall clock and is refused. The default "as-given" keeps today's behaviour. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Callers need to know when the zone is kept, where no zone applies, and which codes the option adds or widens. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An ical.js without the step the work limit hooks into is a library failure, not the caller's; reported as CHECK_LIMIT_EXCEEDED, a consumer would tell an LLM to fix input that is fine. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review of #26 found codes that lied in both directions: library failures (a TypeError inside a move, ical.js failing to expand) surfaced as caller codes, while malformed stored values and wrong-type arguments surfaced as raw TypeErrors. A consumer mapping codes to "fix your input" for an LLM needs both to be exact. - Re-wrapping keeps only a refusal's code (now as cause); anything else stays a plain Error. Deliberate inner throws are typed at their source. - A stored value or rule that cannot be read is INVALID_VALUE/INVALID_RULE with its property, whichever check finds it, and only when needed. - An unknown TZID is UNKNOWN_TZID on a move and on a new rule alike. - Argument types are checked up front (INVALID_INPUT, INVALID_VALUE). - remedy (fix-value, same-call, rewrite-object, none) says what the message says, so consumers never parse messages. - Before the first publish: ORPHANS_OVERRIDES becomes ORPHANED_EXCEPTIONS and WRONG_OBJECT_KIND splits off COMPONENT_NOT_FOUND. - The code list is frozen, the guard narrows to its code, toJSON keeps the message. Tests are one table typed against every code, so a code without a case does not compile. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The table is the consumer contract: code, when, and what to do about it, plus which failures are plain Errors and why the guard needs the original. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Reading a stored rule lazily let a SUMMARY write serialise a malformed RRULE (UNTIL=garbage) as ical.js mangled it (UNTIL=garb-ag-e): a write silently changed a property it does not touch. Untouched RRULE/EXRULE lines now pass through as written, by the same mechanism that rewrites single rule parts; one whose line cannot be found is refused if it cannot be read. A missing ical.js hook gets context and its cause. Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The re-verifier found the table and the absoluteTime section narrower than the code: a stored value carried by a move is same-call, a DUE with its own TZID is converted, and a parse failure keeps ical.js' error as cause (set here too). Refs #25 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Review summary: reviewers A (correctness) and B (consumer/API design), then a re-verifier and a final fix round. Internal failures stay plain Errors; caller refusals carry code + remedy (+ suggestion/property); remedy matches the message at all ~60 throw sites; absoluteTime keep-zone: 0 mismatches over 128,760 instants in 11 zones vs Intl, default byte-identical to 0.5.0 over 6,336 cases; dav-mcp unaffected (its schema only passes strings). Untouched RRULE lines now pass through byte for byte. Breaking changes listed in the PR body. CI green on Node 18–26. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every refusal of
updateFields(series move refused, orphaned overrides, RECURRENCE-ID on a master, invalidtype, unparseable values, …) was a plainError. A consumer could only tell a caller mistake from a library failure by matching message prefixes — dav-mcp does exactly that (PhilflowIO/dav-mcp#114) — and any rewording silently turned refusals back into internal errors.Change
UpdateFieldsError extends Errorwith a stablecode, a machine-readableremedy(fix-value/same-call/rewrite-object/none, matching what the message says),property(upper-cased, where there is one),suggestion(forSERIES_MOVE_REFUSED: the RRULE the library already proposes in the message),causewhen re-wrapped, andtoJSON().UPDATE_FIELDS_ERROR_CODES, the union typesUpdateFieldsErrorCodeandUpdateFieldsRemedy,UpdateFieldsErrorDetails, and the guardisUpdateFieldsError(error, code?), which narrows to the given code. The guard checks name + code instead ofinstanceof, so it holds when the ESM and CJS builds are both loaded. Present in both.d.tsand.d.mts.throwreachable fromupdateFields/seriesMaster/parseDateValuewas classified. Caller-input refusals carry a code; message texts are unchanged (the whole existing suite passes untouched). When a caught refusal is re-thrown with a longer message (DTEND: …,DTSTART changed, and … cannot follow it (…)), it keeps its inner code, so an UNTIL in the DST overlap isDST_AMBIGUOUS, notSERIES_MOVE_REFUSED. Genuine internal failures (unexpected jCal, a rule text that cannot be found again) stay plainError.absoluteTime: 'as-given' | 'keep-zone'(default'as-given'= previous behaviour for zoned values). Today an instant (Zor+02:00) written where a TZID applies is rewritten as UTC; a Europe/Berlin series moved with2026-10-06T08:00:00Zbecomes a UTC series and drifts an hour after the DST change. With'keep-zone'the instant is converted (src/zone.ts: the object's VTIMEZONE, else IANA) to the wall clock in the TZID that applies — the property's own TZID, else DTSTART's for DTEND/DUE/EXDATE/RDATE/RECURRENCE-ID — and the TZID stays. An instant in the second pass of the repeated DST hour is refused (DST_AMBIGUOUS); a TZID with unknown rules is refused (UNKNOWN_TZID) instead of silently falling back to UTC. Where no zone applies (UTC/floating DTSTART, UTC-only properties) it writes UTC as before; all-day rules, zoneless values and dates are unaffected. New codeINVALID_ABSOLUTE_TIMEfor a bad option value.chore(release): 0.6.0— new exported API in 0.x → minor bump. Not tagged. (The absoluteTime commits came later and sit after it; no history rewrite.)INVALID_INPUTINVALID_ICALENDARINVALID_TYPEtypenot vevent/vtodo/vjournalINVALID_FLOATING_TIMEfloatingTimenot keep/localINVALID_ABSOLUTE_TIMEabsoluteTimenot as-given/keep-zoneCOMPONENT_NOT_FOUNDWRONG_OBJECT_KINDtypeon a vCard / on a bare component of another typeNO_MASTERINVALID_VALUEVALUE_TYPE_MISMATCHZONE_MISMATCHUNKNOWN_TZIDUNSUPPORTED_VTIMEZONEUNKNOWN_RULE_PARTRRULE:prefixDUPLICATE_RULE_PARTINVALID_RULERECURRENCE_ID_ON_MASTERSERIES_MOVE_REFUSEDsuggestion), existing UNTIL/EXDATE/RDATE cannot follow; override cannot follow / all-day switchORPHANED_EXCEPTIONSDST_AMBIGUOUSkeep-zoneinstant in the repeated hour's second passCHECK_LIMIT_EXCEEDEDSERIES_UNVERIFIABLEAnything else is a plain
Error, never a refusal code: a missing ical.js hook (Cannot check the series: ical.js no longer exposes …,cause= the inner error), ical.js failing inside a move or an expansion, a rule text that cannot be found again (both with the outer message andcause).A write never changes a rule it does not touch: untouched RRULE/EXRULE lines go back byte for byte (also a malformed
UNTIL=garbage, which 0.5.0 refused with a raw ical.js error and an intermediate build silently rewrote toUNTIL=garb-ag-e); where a line cannot be found in the text, ical.js writes it and an unreadable rule is refused (INVALID_RULE).Breaking changes in 0.6.0
null, arrays now throwINVALID_VALUEwithproperty(0.5.0 wrote e.g.PRIORITY:5from5, and threw raw TypeErrors on date properties).UpdateFieldsError:error.nameandString(error)readUpdateFieldsError: …instead ofError: …; stillinstanceof Error, message texts unchanged.INVALID_VALUE/INVALID_RULE, with a message naming the stored value) instead of raw ical.js messages.nullobject, fields or options, several top-level components) throwINVALID_INPUTinstead of raw TypeErrors.Tests
test/errors.test.ts: one table typedsatisfies Record<UpdateFieldsErrorCode, Case[]>(a code without a case does not compile), one test per case asserting code, remedy, property and suggestion — including the skipped-hour twin, the CHECK_LIMIT_EXCEEDED move branch, malformed stored values, wrong-type arguments; plus tests that untouched rules (also a malformed one) come back byte for byte, that a missing ical.js hook, a TypeError during a move and an iterator failure stay plain Errors; each refusal isinstanceof Errorandinstanceof UpdateFieldsError, hasname === 'UpdateFieldsError', and passes the guard;property/suggestionare checked where set; the guard rejects plain Errors and lookalikes.test/absoluteTime.test.ts, run for Europe/Berlin without VTIMEZONE and for a zone only its VTIMEZONE defines: a weekly series moved with aZDTSTART keepsTZIDat the converted wall time, its EXDATE moves along, and occurrences after 25 Oct stay at 10:00 local (vs. the default, where they drift to 08:00Z = 09:00); numeric offsets; DTEND across the DST change, and DTEND without own TZID taking DTSTART's; a zoneless value in the same call unaffected; first pass of the repeated hour accepted, second pass refused withDST_AMBIGUOUS; UTC/floating DTSTART identical to the default; all-day still refused; unknown TZID refused; DTSTAMP stays UTC.TZ=UTC,America/New_York,Asia/Kolkata(379 passed, 2 integration skipped).test/consumer-smoke.mjs(packed tarball in CI) now also checks a typed refusal from both the ESM and the CJS build, and that the guard holds across them. Ran it locally againstnpm pack.origin/main(fix(release): wait out registry lag in the npm check #24, release.yml only) on top; release.yml was not touched here.Closes #25
🤖 Generated with Claude Code