yuris/life-service
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages 1 Wiki Activity Actions

fix(api): classify custom non-IANA TZID as calendar_unsupported, not dependency_unavailable #81

Merged
yuris merged 2 commits from feat/issue-53-tzid into main 2026-09-23 05:30:41 +00:00
Conversation 2 Commits 2 Files changed 7 +1572 -60
yuris commented 2026-09-23 05:02:18 +00:00
Owner
Copy link

Motivation

Issue #53, raised as "finding 2" during the #14 review. docs/technical/verification.md
C07 requires an unknown/custom VTIMEZONE to produce "exact deployment conformance or
explicit calendar_unsupported/partial; no unsupported window labeled free". The #14
review claimed the classification was already right; the issue asked for a reproduction
before anything changed.

The reproduction says the suspicion was right and the code was wrong — but not where the
issue expected it. src/calendar/report.rs does raise CalendarError::CalendarUnsupported
for an unresolvable TZID, and live_read_failure/coverage_state_for do map that class
correctly. What lost the distinction was the fan-out aggregation: every route that
reads all configured collections collapsed "no collection could be read" into
dependency_unavailable, whatever the failures actually were. So the #14 fixture
(TZID=Custom-Timezone) answered HTTP 503 / dependency_unavailable / CLI exit 5 —
telling a caller to retry a dependency that had answered every request it was sent.

Reproduction (before the change)

TZID=Custom-Timezone with its matching VTIMEZONE, served by a mock Radicale, driven
through the daemon's real router over HTTP (cargo test --test issue53_tzid_tests -- --nocapture,
table printed by every_refusing_path_reports_calendar_unsupported_over_http):

Route HTTP error.code CLI exit
GET /v1/agenda 200 status=partial (coverage unsupported) 6
GET /v1/tasks 503 dependency_unavailable 5
GET /v1/items/{id} 503 dependency_unavailable 5
PATCH /v1/tasks/{id} 503 dependency_unavailable 5
PATCH /v1/events/{id} 503 dependency_unavailable 5
POST /v1/tasks/{id}/complete 503 dependency_unavailable 5
POST /v1/resolve 503 dependency_unavailable 5
POST /v1/next 503 dependency_unavailable 5
POST /v1/plans 503 dependency_unavailable 5
POST /v1/reviews 503 dependency_unavailable 5

Every one of those 503s carried the giveaway message
All configured CalDAV collections are unavailable: Calendar unsupported: Unsupported TZID 'Custom-Timezone' — the interpretation failure was known, then discarded by the
aggregation that decided the request had failed.

Only the agenda read was already right: it is a projection with per-collection coverage, so
an uninterpretable collection degrades coverage explicitly instead of failing (scenarios
C07/C13), and it never claimed an outage.

Changes

  • src/api/agenda.rs — one shared reading of a fan-out's failures: the private
    FailureClass vocabulary (Interpretation / Bound / Outage), classify_failure,
    is_outage, and read_failure(context, failures), which takes (collection_id, error)
    pairs so a refusal can name the collections involved. The module header records the
    precedence and the one deliberate exception.
  • Corrected call sites (each previously returned dependency_unavailable for any
    all-collections-failed fan-out): scan_collections and resolve_live_item
    (src/api/items.rs), run_tasks and live_task_inventory (src/api/tasks.rs),
    live_candidates (src/api/resolve.rs), run_agenda (src/api/agenda.rs). This
    covers every read and mutation path that resolves an item or reads the inventory:
    life agenda, life tasks, life item get, life task update, life event update,
    life task complete/reopen/cancel, life resolve, life next, life plan, life review.
  • src/calendar/report.rs — the decision the issue asked for, recorded in the module
    header: an unresolvable TZID stays CalendarError::CalendarUnsupported (capability),
    not Preservation. docs/technical/calendar.md says an unknown/custom zone must be
    "supported by expansion with verified mapping or reported unsupported" — the
    specification's own term for this case is unsupported; Preservation is reserved for
    the other direction (the bytes were interpretable, but rewriting them would not be
    lossless — src/calendar/preservation.rs). Both land on the same refusal after the #14
    mapping, so the choice names the reason rather than changing the outcome.
  • Unchanged by design: live_read_failure (src/api/items.rs) and coverage_state_for.

Review follow-ups folded into this revision

  1. Outage precedence in an all-failed mixed fan-out (the defect the first revision
    introduced).     read_failure now decides the status by class, not by which failure
    happened to come first: if any collection in the fan-out failed with a genuine outage,
    the request is dependency_unavailable (HTTP 503 / exit 5, retryable), even when
    another collection's failure is a capability gap. Letting the capability gap win would
    hide a broken dependency behind a non-retryable "unsupported" verdict and stop a caller
    from ever retrying it. The capability gap is not dropped: it is named in the message
    and carried as an incomplete_calendar warning, so an operator still learns that a
    retry alone will not be enough. Pinned by
    an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported
    (one collection holding the custom zone + one answering 500) on GET /v1/tasks,
    GET /v1/items/{id}, PATCH /v1/tasks/{id} and POST /v1/next.
  2. The missing exit-code assertion on the next row of the C01 test — added; all five
    routes in an_unreachable_backend_is_still_dependency_unavailable now assert
    exit_code_for(...) == 5, not just the status and code.
  3. Unreachable vs unhealthy. The C01 tests previously only proved "a backend that
    answers 500 → 503/exit 5". a_connection_refused_backend_is_still_dependency_unavailable
    adds the shape an operator actually hits: the configured port is bound, released, and
    nothing listens on it, so the connection is refused. Five routes plus the life agenda
    CLI still answer 503 / dependency_unavailable / exit 5.
  4. Suite-count wording — stated below as test result lines and suites separately.

One deliberate exception, and why

The agenda read keeps degrading to partial when its fan-out failed for reasons that are
all non-outages (an uninterpretable collection, a bound), rather than refusing with 422.
That is not my invention and not a leftover: two pre-existing C07 acceptance tests pin it —
tests/recurrence_tests.rs::c07_an_unsupported_collection_is_partial_and_never_declared_free
(a single collection failing with RANGE=THISANDFUTURE) and
::c07_an_unresolvable_vtimezone_is_reported_as_unsupported (a single collection holding an
unresolvable VTIMEZONE), both asserting HTTP 200 / partial with coverage unsupported,
no occurrences claimed and an incomplete_calendar warning. My first revision made every
fan-out refuse on an all-failed read and broke both; this revision restores the pinned
behaviour rather than rewriting the acceptance tests to match my taxonomy. The agenda read
still refuses an all-outage fan-out (C01), and in the mixed all-failed case it reports the
down collection as coverage unavailable and the uninterpretable one as unsupported, so
the outage is disclosed rather than masked (asserted in the mixed all-failed test).

Flagging it because the taxonomy and the pinned tests disagree: if an all-failed,
all-non-outage agenda read should now be 422/exit 2, that is a change to C07 acceptance
behaviour and belongs in its own change with those two tests, not folded in here.

Per-criterion result

  • Custom non-IANA TZID is calendar_unsupported (HTTP 422 / exit 2) on every read and
    mutation path that can encounter it, never dependency_unavailable — met for every path
    that refuses.     Verified per route over HTTP after the change:

    Route HTTP error.code CLI exit
    GET /v1/agenda 200 status=partial, coverage unsupported, incomplete_calendar 6
    GET /v1/tasks 422 calendar_unsupported 2
    GET /v1/items/{id} 422 calendar_unsupported 2
    PATCH /v1/tasks/{id} 422 calendar_unsupported 2
    PATCH /v1/events/{id} 422 calendar_unsupported 2
    POST /v1/tasks/{id}/complete 422 calendar_unsupported 2
    POST /v1/resolve 422 calendar_unsupported 2
    POST /v1/next 422 calendar_unsupported 2
    POST /v1/plans 422 calendar_unsupported 2
    POST /v1/reviews 422 calendar_unsupported 2

    The agenda read is the exception described above (partial, not 422), so this criterion is
    reported as met for the nine refusing paths, partial for the agenda projection. Exit
    codes for the CLI rows are the real binary's (spawned against the daemon's router), not a
    re-derivation of the envelope.

  • A genuinely unreachable backend still reports dependency_unavailable (503 / exit 5),
    C01 preserved — met, in both the unhealthy and the connection-refused shape.
    an_unreachable_backend_is_still_dependency_unavailable (mock answers 500 to everything)
    and a_connection_refused_backend_is_still_dependency_unavailable (nothing listening on
    the configured port) each assert 503 + dependency_unavailable + exit 5 on
    GET /v1/agenda, GET /v1/tasks, GET /v1/items/{id}, PATCH /v1/tasks/{id} and
    POST /v1/next, with the CLI exiting 5 for life agenda. The pre-existing C01 tests
    (tests/agenda_tasks_tests.rs, tests/resolve_tests.rs, tests/next_command_tests.rs,
    tests/replan_tests.rs) all inject 503/500 from the backend and still pass.

  • The #14 fixture is exercised over HTTP, not only at the patcher level — met. Every
    row above is an HTTP request through router_with_state against a mock Radicale that
    serves the fixture (custom TZID plus its VTIMEZONE block); the patcher route is one of
    them (PATCH /v1/tasks/{id}, PATCH /v1/events/{id}), and
    the_patcher_refuses_the_custom_zone_fixture_over_http additionally asserts zero PUTs,
    zero journal rows and byte-identical backend state.

  • Each affected route has a test pinning both the error code and the exit code — met.
    tests/issue53_tzid_tests.rs (new) asserts code and client::exit_code_for for all
    ten routes, plus the CLI binary's own exit codes for life item get (2), life task update (2), life tasks (2), life agenda (6) and the outage direction (5).

Verification

  • nix develop --command cargo fmt --check — clean.
  • nix develop --command cargo clippy --all-targets -- -D warnings — 0 errors.
  • nix develop --command cargo test --no-fail-fast — 752 passed, 0 failed across 43
    test result lines (41 test binaries, doc tests included)    . Baseline main at 37c8101
    measured the same way: 743 passed, 0 failed across 42 test result lines (40 suites);
    this branch adds the 9 tests in tests/issue53_tzid_tests.rs.

New tests (tests/issue53_tzid_tests.rs, 9):
every_refusing_path_reports_calendar_unsupported_over_http,
no_path_reports_the_custom_zone_as_a_backend_outage,
a_refused_mutation_dispatches_nothing_and_leaves_the_resource_intact,
the_cli_exits_two_when_nothing_is_readable_and_six_when_a_collection_is,
an_unreachable_backend_is_still_dependency_unavailable,
a_connection_refused_backend_is_still_dependency_unavailable,
an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported,
a_mixed_deployment_degrades_coverage_and_still_serves_the_readable_collection,
the_patcher_refuses_the_custom_zone_fixture_over_http.

Adjusted test: tests/update_tests.rs::a_custom_non_iana_tzid_is_refused_before_any_dispatch
previously asserted dependency_unavailable — i.e. it pinned the defect. It now asserts
calendar_unsupported, HTTP 422, exit 2, retryable: false, and (unchanged) zero PUTs,
zero journal rows and untouched backend bytes. The assertion is strictly stronger, not
weakened. No other existing test was changed; the two C07 acceptance tests that my first
revision broke pass unchanged.

Pre-existing flake observed, not caused by this change: tests/proposal_apply_tests.rs:: every_step_applied_marks_the_proposal_applied_and_replays_under_the_same_key fails
intermittently (the replay comes back Error at HTTP 200). I measured it on this branch
(8/10 green) and on main at 37c8101 with the same loop (9/10 green, and 1 failure in a
paired 10-run comparison), so it reproduces without this diff. It is a journal/commit_apply_outcome
path this change does not touch; I did not fix it.

Defect found outside this issue's scope (not fixed)

run_item_get (src/api/items.rs) answers not_found when a collection could not be read
and another collection simply held no match — in a multi-collection deployment, absence is
not established there. resolve_live_item in the same file refuses instead (it always has),
so the two id-resolution paths disagree. That is a pre-existing inconsistency in a different
classification (404 vs a refusal), not the TZID/outage confusion this issue is about, so it
is reported here rather than changed silently.

Documentation

No normative text was rewritten. docs/ is excluded from this repository
(.git/info/exclude), so the recorded decision for the TZID classification lives in the
src/calendar/report.rs module header (the codebase's convention for such choices, cf.
src/domain/analytics/mod.rs). skills/life/references/commands.md and errors.md were
checked and need no change: no command, route or error identifier is added, and
tests/skill_package_tests.rs counts are unaffected.

Closes #53

## Motivation Issue #53, raised as "finding 2" during the #14 review. `docs/technical/verification.md` C07 requires an unknown/custom `VTIMEZONE` to produce "exact deployment conformance or explicit `calendar_unsupported`/partial; no unsupported window labeled free". The #14 review claimed the classification was already right; the issue asked for a reproduction before anything changed. The reproduction says the suspicion was right and the code was wrong — but not where the issue expected it. `src/calendar/report.rs` does raise `CalendarError::CalendarUnsupported` for an unresolvable `TZID`, and `live_read_failure`/`coverage_state_for` do map that class correctly. What lost the distinction was the **fan-out aggregation**: every route that reads all configured collections collapsed "no collection could be read" into `dependency_unavailable`, whatever the failures actually were. So the #14 fixture (`TZID=Custom-Timezone`) answered **HTTP 503 / `dependency_unavailable` / CLI exit 5** — telling a caller to retry a dependency that had answered every request it was sent. ## Reproduction (before the change) `TZID=Custom-Timezone` with its matching `VTIMEZONE`, served by a mock Radicale, driven through the daemon's real router over HTTP (`cargo test --test issue53_tzid_tests -- --nocapture`, table printed by `every_refusing_path_reports_calendar_unsupported_over_http`): | Route | HTTP | `error.code` | CLI exit | |---|---|---|---| | `GET /v1/agenda` | 200 | `status=partial` (coverage `unsupported`) | 6 | | `GET /v1/tasks` | 503 | `dependency_unavailable` | 5 | | `GET /v1/items/{id}` | 503 | `dependency_unavailable` | 5 | | `PATCH /v1/tasks/{id}` | 503 | `dependency_unavailable` | 5 | | `PATCH /v1/events/{id}` | 503 | `dependency_unavailable` | 5 | | `POST /v1/tasks/{id}/complete` | 503 | `dependency_unavailable` | 5 | | `POST /v1/resolve` | 503 | `dependency_unavailable` | 5 | | `POST /v1/next` | 503 | `dependency_unavailable` | 5 | | `POST /v1/plans` | 503 | `dependency_unavailable` | 5 | | `POST /v1/reviews` | 503 | `dependency_unavailable` | 5 | Every one of those 503s carried the giveaway message `All configured CalDAV collections are unavailable: Calendar unsupported: Unsupported TZID 'Custom-Timezone'` — the interpretation failure was known, then discarded by the aggregation that decided the request had failed. Only the agenda read was already right: it is a projection with per-collection coverage, so an uninterpretable collection degrades coverage explicitly instead of failing (scenarios C07/C13), and it never claimed an outage. ## Changes - **`src/api/agenda.rs`** — one shared reading of a fan-out's failures: the private `FailureClass` vocabulary (`Interpretation` / `Bound` / `Outage`), `classify_failure`, `is_outage`, and `read_failure(context, failures)`, which takes `(collection_id, error)` pairs so a refusal can name the collections involved. The module header records the precedence and the one deliberate exception. - **Corrected call sites** (each previously returned `dependency_unavailable` for any all-collections-failed fan-out): `scan_collections` and `resolve_live_item` (`src/api/items.rs`), `run_tasks` and `live_task_inventory` (`src/api/tasks.rs`), `live_candidates` (`src/api/resolve.rs`), `run_agenda` (`src/api/agenda.rs`). This covers every read and mutation path that resolves an item or reads the inventory: `life agenda`, `life tasks`, `life item get`, `life task update`, `life event update`, `life task complete/reopen/cancel`, `life resolve`, `life next`, `life plan`, `life review`. - **`src/calendar/report.rs`** — the decision the issue asked for, recorded in the module header: an unresolvable `TZID` stays `CalendarError::CalendarUnsupported` (capability), **not** `Preservation`. `docs/technical/calendar.md` says an unknown/custom zone must be "supported by expansion with verified mapping or **reported unsupported**" — the specification's own term for this case is *unsupported*; `Preservation` is reserved for the other direction (the bytes were interpretable, but rewriting them would not be lossless — `src/calendar/preservation.rs`). Both land on the same refusal after the #14 mapping, so the choice names the reason rather than changing the outcome. - **Unchanged by design:** `live_read_failure` (`src/api/items.rs`) and `coverage_state_for`. ## Review follow-ups folded into this revision 1. **Outage precedence in an all-failed mixed fan-out (the defect the first revision introduced).** `read_failure` now decides the status by class, not by which failure happened to come first: if *any* collection in the fan-out failed with a genuine outage, the request is `dependency_unavailable` (HTTP 503 / exit 5, `retryable`), even when another collection's failure is a capability gap. Letting the capability gap win would hide a broken dependency behind a non-retryable "unsupported" verdict and stop a caller from ever retrying it. The capability gap is **not dropped**: it is named in the message *and* carried as an `incomplete_calendar` warning, so an operator still learns that a retry alone will not be enough. Pinned by `an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported` (one collection holding the custom zone + one answering 500) on `GET /v1/tasks`, `GET /v1/items/{id}`, `PATCH /v1/tasks/{id}` and `POST /v1/next`. 2. **The missing exit-code assertion on the `next` row of the C01 test** — added; all five routes in `an_unreachable_backend_is_still_dependency_unavailable` now assert `exit_code_for(...) == 5`, not just the status and code. 3. **Unreachable vs unhealthy.** The C01 tests previously only proved "a backend that answers 500 → 503/exit 5". `a_connection_refused_backend_is_still_dependency_unavailable` adds the shape an operator actually hits: the configured port is bound, released, and nothing listens on it, so the connection is refused. Five routes plus the `life agenda` CLI still answer 503 / `dependency_unavailable` / exit 5. 4. **Suite-count wording** — stated below as `test result` lines and suites separately. ## One deliberate exception, and why The agenda read keeps degrading to `partial` when its fan-out failed for reasons that are all non-outages (an uninterpretable collection, a bound), rather than refusing with 422. That is not my invention and not a leftover: two pre-existing C07 acceptance tests pin it — `tests/recurrence_tests.rs::c07_an_unsupported_collection_is_partial_and_never_declared_free` (a single collection failing with `RANGE=THISANDFUTURE`) and `::c07_an_unresolvable_vtimezone_is_reported_as_unsupported` (a single collection holding an unresolvable `VTIMEZONE`), both asserting HTTP 200 / `partial` with coverage `unsupported`, no occurrences claimed and an `incomplete_calendar` warning. My first revision made every fan-out refuse on an all-failed read and broke both; this revision restores the pinned behaviour rather than rewriting the acceptance tests to match my taxonomy. The agenda read still refuses an all-outage fan-out (C01), and in the mixed all-failed case it reports the down collection as coverage `unavailable` and the uninterpretable one as `unsupported`, so the outage is disclosed rather than masked (asserted in the mixed all-failed test). Flagging it because the taxonomy and the pinned tests disagree: if an all-failed, all-non-outage agenda read should now be 422/exit 2, that is a change to C07 acceptance behaviour and belongs in its own change with those two tests, not folded in here. ## Per-criterion result - **Custom non-IANA `TZID` is `calendar_unsupported` (HTTP 422 / exit 2) on every read and mutation path that can encounter it, never `dependency_unavailable` — met for every path that refuses.** Verified per route over HTTP after the change: | Route | HTTP | `error.code` | CLI exit | |---|---|---|---| | `GET /v1/agenda` | 200 | `status=partial`, coverage `unsupported`, `incomplete_calendar` | 6 | | `GET /v1/tasks` | 422 | `calendar_unsupported` | 2 | | `GET /v1/items/{id}` | 422 | `calendar_unsupported` | 2 | | `PATCH /v1/tasks/{id}` | 422 | `calendar_unsupported` | 2 | | `PATCH /v1/events/{id}` | 422 | `calendar_unsupported` | 2 | | `POST /v1/tasks/{id}/complete` | 422 | `calendar_unsupported` | 2 | | `POST /v1/resolve` | 422 | `calendar_unsupported` | 2 | | `POST /v1/next` | 422 | `calendar_unsupported` | 2 | | `POST /v1/plans` | 422 | `calendar_unsupported` | 2 | | `POST /v1/reviews` | 422 | `calendar_unsupported` | 2 | The agenda read is the exception described above (partial, not 422), so this criterion is reported as **met for the nine refusing paths, partial for the agenda projection**. Exit codes for the CLI rows are the real binary's (spawned against the daemon's router), not a re-derivation of the envelope. - **A genuinely unreachable backend still reports `dependency_unavailable` (503 / exit 5), C01 preserved — met, in both the unhealthy and the connection-refused shape.** `an_unreachable_backend_is_still_dependency_unavailable` (mock answers 500 to everything) and `a_connection_refused_backend_is_still_dependency_unavailable` (nothing listening on the configured port) each assert 503 + `dependency_unavailable` + exit 5 on `GET /v1/agenda`, `GET /v1/tasks`, `GET /v1/items/{id}`, `PATCH /v1/tasks/{id}` and `POST /v1/next`, with the CLI exiting 5 for `life agenda`. The pre-existing C01 tests (`tests/agenda_tasks_tests.rs`, `tests/resolve_tests.rs`, `tests/next_command_tests.rs`, `tests/replan_tests.rs`) all inject 503/500 from the backend and still pass. - **The #14 fixture is exercised over HTTP, not only at the patcher level — met.** Every row above is an HTTP request through `router_with_state` against a mock Radicale that serves the fixture (custom `TZID` plus its `VTIMEZONE` block); the patcher route is one of them (`PATCH /v1/tasks/{id}`, `PATCH /v1/events/{id}`), and `the_patcher_refuses_the_custom_zone_fixture_over_http` additionally asserts zero `PUT`s, zero journal rows and byte-identical backend state. - **Each affected route has a test pinning both the error code and the exit code — met.** `tests/issue53_tzid_tests.rs` (new) asserts code **and** `client::exit_code_for` for all ten routes, plus the CLI binary's own exit codes for `life item get` (2), `life task update` (2), `life tasks` (2), `life agenda` (6) and the outage direction (5). ## Verification - `nix develop --command cargo fmt --check` — clean. - `nix develop --command cargo clippy --all-targets -- -D warnings` — 0 errors. - `nix develop --command cargo test --no-fail-fast` — **752 passed, 0 failed across 43 `test result` lines (41 test binaries, doc tests included)**. Baseline `main` at 37c8101 measured the same way: 743 passed, 0 failed across 42 `test result` lines (40 suites); this branch adds the 9 tests in `tests/issue53_tzid_tests.rs`. New tests (`tests/issue53_tzid_tests.rs`, 9): `every_refusing_path_reports_calendar_unsupported_over_http`, `no_path_reports_the_custom_zone_as_a_backend_outage`, `a_refused_mutation_dispatches_nothing_and_leaves_the_resource_intact`, `the_cli_exits_two_when_nothing_is_readable_and_six_when_a_collection_is`, `an_unreachable_backend_is_still_dependency_unavailable`, `a_connection_refused_backend_is_still_dependency_unavailable`, `an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported`, `a_mixed_deployment_degrades_coverage_and_still_serves_the_readable_collection`, `the_patcher_refuses_the_custom_zone_fixture_over_http`. Adjusted test: `tests/update_tests.rs::a_custom_non_iana_tzid_is_refused_before_any_dispatch` previously asserted `dependency_unavailable` — i.e. it pinned the defect. It now asserts `calendar_unsupported`, HTTP 422, exit 2, `retryable: false`, and (unchanged) zero `PUT`s, zero journal rows and untouched backend bytes. The assertion is strictly stronger, not weakened. No other existing test was changed; the two C07 acceptance tests that my first revision broke pass unchanged. Pre-existing flake observed, not caused by this change: `tests/proposal_apply_tests.rs:: every_step_applied_marks_the_proposal_applied_and_replays_under_the_same_key` fails intermittently (the replay comes back `Error` at HTTP 200). I measured it on this branch (8/10 green) and on `main` at 37c8101 with the same loop (9/10 green, and 1 failure in a paired 10-run comparison), so it reproduces without this diff. It is a journal/`commit_apply_outcome` path this change does not touch; I did not fix it. ## Defect found outside this issue's scope (not fixed) `run_item_get` (`src/api/items.rs`) answers `not_found` when a collection could not be read and another collection simply held no match — in a multi-collection deployment, absence is not established there. `resolve_live_item` in the same file refuses instead (it always has), so the two id-resolution paths disagree. That is a pre-existing inconsistency in a different classification (404 vs a refusal), not the TZID/outage confusion this issue is about, so it is reported here rather than changed silently. ## Documentation No normative text was rewritten. `docs/` is excluded from this repository (`.git/info/exclude`), so the recorded decision for the `TZID` classification lives in the `src/calendar/report.rs` module header (the codebase's convention for such choices, cf. `src/domain/analytics/mod.rs`). `skills/life/references/commands.md` and `errors.md` were checked and need no change: no command, route or error identifier is added, and `tests/skill_package_tests.rs` counts are unaffected. Closes #53 
Issue #53 (finding 2 of the #14 review). Reproduced over HTTP first: the #14
fixture's TZID=Custom-Timezone answered HTTP 503 / dependency_unavailable /
exit 5 on nine of ten read and mutation paths, telling a caller to retry a
dependency that had answered every request it was sent (C07 requires an
explicit calendar_unsupported or partial instead).

The interpretation itself was right -- src/calendar/report.rs raises
CalendarUnsupported for an unresolvable TZID, and live_read_failure /
coverage_state_for map that class correctly. What lost the distinction was the
fan-out aggregation: every route that reads all configured collections
collapsed "no collection could be read" into dependency_unavailable whatever
the failures were.

- src/api/agenda.rs: one shared classification (FailureClass,
  classify_failure, is_outage, read_failure). An outage is reported only when
  every failed collection failed with a genuine availability failure;
  otherwise the first failure retrying cannot clear decides --
  calendar_unsupported (422 / exit 2) for CalendarUnsupported/Preservation,
  limit_exceeded for LimitExceeded.
- Corrected the call sites in src/api/items.rs (scan_collections,
  resolve_live_item), src/api/tasks.rs (run_tasks, live_task_inventory),
  src/api/resolve.rs (live_candidates) and src/api/agenda.rs (run_agenda),
  covering agenda, tasks, item get, task/event update, lifecycle transitions,
  resolve, next, plan and review.
- src/calendar/report.rs: recorded the classification decision in the module
  header -- an unresolvable TZID stays CalendarUnsupported (capability), not
  Preservation, per docs/technical/calendar.md's "reported unsupported".
- C01 is preserved: an all-outage fan-out still reports
  dependency_unavailable (503 / exit 5).

Tests: tests/issue53_tzid_tests.rs pins code and exit code per route over HTTP
with the #14 fixture (plus the CLI binary's own exit codes), and proves the
outage direction; tests/update_tests.rs's custom-TZID test asserted the defect
and now asserts the corrected refusal.
yuris commented 2026-09-23 05:14:46 +00:00
Author
Owner
Copy link

REQUEST CHANGES

Acceptance criteria

Criterion Result Evidence
Custom non-IANA TZID is calendar_unsupported (422 / exit 2) on the refusing read/mutation paths, not dependency_unavailable Partial The single-unsupported-collection HTTP matrix covers the nine listed refusing routes in tests/issue53_tzid_tests.rs:597-631; each asserts code, 422, and client::exit_code_for == 2. Agenda is correctly the exception: 200 partial, complete=false, coverage[].state=unsupported, incomplete_calendar, exit 6 (:644-658, :748-788). However, mixed all-failed fan-outs can still misclassify a real outage as 422; see the major finding.
A genuinely unavailable backend remains dependency_unavailable (503 / exit 5) Partial an_unreachable_backend_is_still_dependency_unavailable asserts code and 503 on agenda, tasks, item-get, task-update, and next (:794-866), and exit 5 for four direct envelopes plus the agenda CLI. Its down() mock returns HTTP 500 (:109-115), so it verifies an unhealthy response rather than a connection-refused backend; existing C01 tests also use the 500 fixture. The /v1/next direct assertion does not pin exit 5 (minor finding). Mixed outage + unsupported is not covered and is currently wrong (major finding).
The #14-style fixture is exercised over HTTP Met custom_tzid_ics() includes TZID=Custom-Timezone plus a matching VTIMEZONE (:167-203); the mock is served over TCP and requests use router_with_state/real HTTP (:250-333). The matrix and patcher test reach the fixture through the daemon router.
Every affected listed route pins error code and exit code Met for the nine routes named by the issue every_refusing_path_reports_calendar_unsupported_over_http iterates all nine non-agenda rows and asserts calendar_unsupported, 422, and exit 2 (:615-631).
Agenda follows C07/C13 partial semantics without labeling the unsupported window free Met The test asserts 200 partial, complete=false, coverage[0].state == "unsupported", and incomplete_calendar (:644-658). This matches docs/technical/verification.md C07 and docs/technical/calendar.md's requirement that unsupported semantics prevent a free-time claim.
The TZID decision is recorded and uses the domain term Met src/calendar/report.rs:1-31 records CalendarUnsupported rather than Preservation, with the rationale that the deployment cannot interpret the wall-clock time; this matches the specification's “reported unsupported” terminology. The parser maps an unresolvable TZID at :404-406.

Findings

  • major — src/api/agenda.rs:307-315 — Preserve outage classification in mixed all-failed fan-outs. read_failure records interpretation and bound errors but ignores FailureClass::Outage while selecting the response. If one configured collection returns a real transport/HTTP outage and another returns the custom TZID as CalendarUnsupported, every collection has failed, yet first_interpretation wins and the route returns calendar_unsupported (422 / exit 2). This tells the caller not to retry even though one dependency is genuinely down, violating C01 and the stated outage/capability distinction. In this all-failed mixed case the outage must take precedence and remain dependency_unavailable (503 / exit 5); a successful-plus-failed fan-out may still be a disclosed partial result. There is no regression test combining one down collection with one uninterpretable collection—the added mixed test is unsupported plus readable (:903-925)—so this defect is currently unpinned across the shared callers (tasks, item resolution/mutations, resolve, next, plan and review).

  • minor — tests/issue53_tzid_tests.rs:856-866 — Pin the next-route outage exit code. The C01 test asserts HTTP 503 and dependency_unavailable for POST /v1/next, but unlike the other direct route assertions it never calls exit_code_for. The PR body claims the test asserts “503 + dependency_unavailable + exit 5” on all five routes, which is not true for this row; add the explicit exit-5 assertion so a route-specific exit-code drift cannot pass unnoticed.

Verification I ran

  • nix develop --command cargo test — 750 passed, 0 failed. The run contained 40 Running test binaries plus the doc-test section (41 suite-result blocks), all successful. The new tests/issue53_tzid_tests.rs binary ran all 7 tests successfully, including the HTTP fixture, C01 direction, and readable+unsupported mixed case.
  • nix develop --command cargo clippy --all-targets -- -D warnings — clean.
  • nix develop --command cargo fmt --check — clean.

PR-body claims

The claim “750 passed; 0 failed (43 test binaries)” is only partly reproducible: the pass/fail total is correct, but this run reports 40 test binaries plus doc-tests (41 suite-result blocks), not 43. The claim that C01 asserts exit 5 on all five listed routes is overstated because /v1/next lacks that assertion. The claim that the mock is “genuinely down” should be read as unhealthy: the mock returns HTTP 500 rather than refusing the connection; this is still a valid dependency-unavailable response test, but it is not a connection-unreachable reproduction.

Anything not checked

I did not run against a deployed Radicale instance or verify a particular Radicale version's native VTIMEZONE expansion; the repository contract explicitly treats the fixture and exact-deployment conformance as separate concerns.

REQUEST CHANGES ## Acceptance criteria | Criterion | Result | Evidence | |---|---|---| | Custom non-IANA `TZID` is `calendar_unsupported` (422 / exit 2) on the refusing read/mutation paths, not `dependency_unavailable` | **Partial** | The single-unsupported-collection HTTP matrix covers the nine listed refusing routes in `tests/issue53_tzid_tests.rs:597-631`; each asserts code, 422, and `client::exit_code_for == 2`. Agenda is correctly the exception: `200 partial`, `complete=false`, `coverage[].state=unsupported`, `incomplete_calendar`, exit 6 (`:644-658`, `:748-788`). However, mixed all-failed fan-outs can still misclassify a real outage as 422; see the major finding. | | A genuinely unavailable backend remains `dependency_unavailable` (503 / exit 5) | **Partial** | `an_unreachable_backend_is_still_dependency_unavailable` asserts code and 503 on agenda, tasks, item-get, task-update, and next (`:794-866`), and exit 5 for four direct envelopes plus the agenda CLI. Its `down()` mock returns HTTP 500 (`:109-115`), so it verifies an unhealthy response rather than a connection-refused backend; existing C01 tests also use the 500 fixture. The `/v1/next` direct assertion does not pin exit 5 (minor finding). Mixed outage + unsupported is not covered and is currently wrong (major finding). | | The #14-style fixture is exercised over HTTP | **Met** | `custom_tzid_ics()` includes `TZID=Custom-Timezone` plus a matching `VTIMEZONE` (`:167-203`); the mock is served over TCP and requests use `router_with_state`/real HTTP (`:250-333`). The matrix and patcher test reach the fixture through the daemon router. | | Every affected listed route pins error code and exit code | **Met for the nine routes named by the issue** | `every_refusing_path_reports_calendar_unsupported_over_http` iterates all nine non-agenda rows and asserts `calendar_unsupported`, 422, and exit 2 (`:615-631`). | | Agenda follows C07/C13 partial semantics without labeling the unsupported window free | **Met** | The test asserts `200 partial`, `complete=false`, `coverage[0].state == "unsupported"`, and `incomplete_calendar` (`:644-658`). This matches `docs/technical/verification.md` C07 and `docs/technical/calendar.md`'s requirement that unsupported semantics prevent a free-time claim. | | The `TZID` decision is recorded and uses the domain term | **Met** | `src/calendar/report.rs:1-31` records `CalendarUnsupported` rather than `Preservation`, with the rationale that the deployment cannot interpret the wall-clock time; this matches the specification's “reported unsupported” terminology. The parser maps an unresolvable `TZID` at `:404-406`. | ## Findings - **major — `src/api/agenda.rs:307-315` — Preserve outage classification in mixed all-failed fan-outs.** `read_failure` records interpretation and bound errors but ignores `FailureClass::Outage` while selecting the response. If one configured collection returns a real transport/HTTP outage and another returns the custom `TZID` as `CalendarUnsupported`, every collection has failed, yet `first_interpretation` wins and the route returns `calendar_unsupported` (422 / exit 2). This tells the caller not to retry even though one dependency is genuinely down, violating C01 and the stated outage/capability distinction. In this all-failed mixed case the outage must take precedence and remain `dependency_unavailable` (503 / exit 5); a successful-plus-failed fan-out may still be a disclosed partial result. There is no regression test combining one down collection with one uninterpretable collection—the added mixed test is unsupported plus readable (`:903-925`)—so this defect is currently unpinned across the shared callers (tasks, item resolution/mutations, resolve, next, plan and review). - **minor — `tests/issue53_tzid_tests.rs:856-866` — Pin the next-route outage exit code.** The C01 test asserts HTTP 503 and `dependency_unavailable` for `POST /v1/next`, but unlike the other direct route assertions it never calls `exit_code_for`. The PR body claims the test asserts “503 + `dependency_unavailable` + exit 5” on all five routes, which is not true for this row; add the explicit exit-5 assertion so a route-specific exit-code drift cannot pass unnoticed. ## Verification I ran - `nix develop --command cargo test` — **750 passed, 0 failed**. The run contained **40 `Running` test binaries plus the doc-test section** (41 suite-result blocks), all successful. The new `tests/issue53_tzid_tests.rs` binary ran all 7 tests successfully, including the HTTP fixture, C01 direction, and readable+unsupported mixed case. - `nix develop --command cargo clippy --all-targets -- -D warnings` — clean. - `nix develop --command cargo fmt --check` — clean. ## PR-body claims The claim “**750 passed; 0 failed (43 test binaries)**” is only partly reproducible: the pass/fail total is correct, but this run reports 40 test binaries plus doc-tests (41 suite-result blocks), not 43. The claim that C01 asserts exit 5 on all five listed routes is overstated because `/v1/next` lacks that assertion. The claim that the mock is “genuinely down” should be read as unhealthy: the mock returns HTTP 500 rather than refusing the connection; this is still a valid dependency-unavailable response test, but it is not a connection-unreachable reproduction. ## Anything not checked I did not run against a deployed Radicale instance or verify a particular Radicale version's native `VTIMEZONE` expansion; the repository contract explicitly treats the fixture and exact-deployment conformance as separate concerns.
Review follow-ups on the first revision, which got the precedence backwards in
the opposite direction: a fan-out where one collection held the uninterpretable
zone and another was genuinely down answered calendar_unsupported (422 / exit 2),
masking a real outage behind a non-retryable verdict.

- read_failure now decides by class, not by which failure came first: any
  genuine outage in the fan-out keeps the request dependency_unavailable
  (HTTP 503 / exit 5, retryable), even when another collection's failure is a
  capability gap. The gap is not dropped by that — it is named in the message
  and carried as an incomplete_calendar warning, so an operator still learns
  that a retry alone will not be enough. It takes (collection_id, error) pairs
  so the refusal names the collections involved.
- The agenda read keeps its pinned degradation: two pre-existing C07 acceptance
  tests (tests/recurrence_tests.rs::c07_an_unsupported_collection_is_partial_
  and_never_declared_free, ::c07_an_unresolvable_vtimezone_is_reported_as_
  unsupported) require an all-failed, all-non-outage agenda read to answer
  partial with coverage `unsupported`. The first revision refused it and broke
  both; this restores the pinned behaviour instead of rewriting the tests. An
  all-outage agenda fan-out still refuses (C01), and in the mixed all-failed
  case the down collection is reported `unavailable` and the uninterpretable
  one `unsupported`, so the outage is disclosed rather than masked.

Tests: a mixed all-failed fan-out (one down + one unsupported) asserting
503/dependency_unavailable/exit 5 with the unsupported collection still named;
a connection-refused backend (port bound, released, nothing listening) proving
the unreachable case, not just an unhealthy 500; and the missing exit-code
assertion on the `next` row of the C01 test.
yuris commented 2026-09-23 05:30:19 +00:00
Author
Owner
Copy link

APPROVE

Acceptance criteria

Criterion Result Evidence
Custom non-IANA TZID is calendar_unsupported (422 / exit 2) on refusing read and mutation paths, not dependency_unavailable Met for the nine refusing routes; agenda is the documented partial projection tests/issue53_tzid_tests.rs::every_refusing_path_reports_calendar_unsupported_over_http asserts code, HTTP 422, and exit 2 for tasks, item get, both patch routes, complete, resolve, next, plan, and review. The agenda assertions remain 200/partial, complete=false, coverage=unsupported, incomplete_calendar, exit 6.
Genuine backend unavailability remains dependency_unavailable (503 / exit 5), with retryability Met The revised C01 tests assert HTTP 503, code, and exit 5 on all five routes. a_connection_refused_backend_is_still_dependency_unavailable configures a port that is bound and then released before the daemon reads it, so this is a real refused connection rather than the mock's 500 response. The mixed all-failed regression asserts dependency_unavailable, retryable=true, exit 5, and retains the capability gap in both the message and an incomplete_calendar warning.
The #14-style fixture is exercised through real HTTP Met The fixture contains TZID=Custom-Timezone and a matching VTIMEZONE; the tests serve it through the mock CalDAV TCP server and send requests through the daemon router. The patcher test additionally asserts zero PUTs, zero journal rows, and unchanged bytes.
Each affected route pins error code and exit code Met The single-zone matrix asserts both for all nine refusing routes, and the C01/mixed tests pin both values for the outage paths.
Agenda behavior does not label an unsupported window free Met The two unchanged C07 acceptance tests explicitly require HTTP 200/partial, unsupported coverage, no occurrences, and incomplete_calendar. The revised mixed test additionally shows an outage as unavailable and the custom zone as unsupported, with complete=false and no items. This is consistent with verification.md C07's explicit calendar_unsupported/partial allowance and calendar.md's prohibition on claiming free time under unsupported semantics; changing these tests to 422 would be a separate normative behavior change, not a correction required by this issue.
TZID classification decision is recorded and uses domain terminology Met src/calendar/report.rs's module header records the choice of CalendarUnsupported (capability) rather than Preservation (lossless rewrite failure), matching the specification's “reported unsupported” term.

Findings

None. The previous mixed-fan-out classification defect and missing /v1/next exit assertion are fixed in this head.

Verification I ran

  • nix develop --command cargo test — 752 passed, 0 failed; the revised issue53_tzid_tests binary ran 9/9, including the mixed all-failed, connection-refused, retryability, and warning-preservation cases.
  • nix develop --command cargo test --test issue53_tzid_tests an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported -- --nocapture — 1 passed.
  • nix develop --command cargo test --test issue53_tzid_tests a_connection_refused_backend_is_still_dependency_unavailable -- --nocapture — 1 passed.
  • nix develop --command cargo clippy --all-targets -- -D warnings — clean.
  • nix develop --command cargo fmt --check — clean.

The reported intermittent proposal_apply_tests failure was not observed in my full run. I did not run a deployed Radicale compatibility check; this review covers the repository fixture and daemon HTTP behavior.

APPROVE ## Acceptance criteria | Criterion | Result | Evidence | |---|---|---| | Custom non-IANA `TZID` is `calendar_unsupported` (422 / exit 2) on refusing read and mutation paths, not `dependency_unavailable` | **Met for the nine refusing routes; agenda is the documented partial projection** | `tests/issue53_tzid_tests.rs::every_refusing_path_reports_calendar_unsupported_over_http` asserts code, HTTP 422, and exit 2 for tasks, item get, both patch routes, complete, resolve, next, plan, and review. The agenda assertions remain 200/partial, `complete=false`, `coverage=unsupported`, `incomplete_calendar`, exit 6. | | Genuine backend unavailability remains `dependency_unavailable` (503 / exit 5), with retryability | **Met** | The revised C01 tests assert HTTP 503, code, and exit 5 on all five routes. `a_connection_refused_backend_is_still_dependency_unavailable` configures a port that is bound and then released before the daemon reads it, so this is a real refused connection rather than the mock's 500 response. The mixed all-failed regression asserts `dependency_unavailable`, `retryable=true`, exit 5, and retains the capability gap in both the message and an `incomplete_calendar` warning. | | The #14-style fixture is exercised through real HTTP | **Met** | The fixture contains `TZID=Custom-Timezone` and a matching `VTIMEZONE`; the tests serve it through the mock CalDAV TCP server and send requests through the daemon router. The patcher test additionally asserts zero PUTs, zero journal rows, and unchanged bytes. | | Each affected route pins error code and exit code | **Met** | The single-zone matrix asserts both for all nine refusing routes, and the C01/mixed tests pin both values for the outage paths. | | Agenda behavior does not label an unsupported window free | **Met** | The two unchanged C07 acceptance tests explicitly require HTTP 200/partial, unsupported coverage, no occurrences, and `incomplete_calendar`. The revised mixed test additionally shows an outage as `unavailable` and the custom zone as `unsupported`, with `complete=false` and no items. This is consistent with `verification.md` C07's explicit `calendar_unsupported`/partial allowance and `calendar.md`'s prohibition on claiming free time under unsupported semantics; changing these tests to 422 would be a separate normative behavior change, not a correction required by this issue. | | TZID classification decision is recorded and uses domain terminology | **Met** | `src/calendar/report.rs`'s module header records the choice of `CalendarUnsupported` (capability) rather than `Preservation` (lossless rewrite failure), matching the specification's “reported unsupported” term. | ## Findings None. The previous mixed-fan-out classification defect and missing `/v1/next` exit assertion are fixed in this head. ## Verification I ran - `nix develop --command cargo test` — **752 passed, 0 failed**; the revised `issue53_tzid_tests` binary ran 9/9, including the mixed all-failed, connection-refused, retryability, and warning-preservation cases. - `nix develop --command cargo test --test issue53_tzid_tests an_outage_in_a_mixed_all_failed_fan_out_decides_the_status_and_the_gap_is_still_reported -- --nocapture` — 1 passed. - `nix develop --command cargo test --test issue53_tzid_tests a_connection_refused_backend_is_still_dependency_unavailable -- --nocapture` — 1 passed. - `nix develop --command cargo clippy --all-targets -- -D warnings` — clean. - `nix develop --command cargo fmt --check` — clean. The reported intermittent `proposal_apply_tests` failure was not observed in my full run. I did not run a deployed Radicale compatibility check; this review covers the repository fixture and daemon HTTP behavior.
yuris merged commit e5a7b3648f into main 2026-09-23 05:30:41 +00:00
yuris changed title from fix(api): classify custom non-IANA TZID as calendar_unsupported, not dependency_unavailable to fix(api): classify custom non-IANA TZID as calendar_unsupported, not dependency_unavailable 2026-09-23 06:25:43 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/life-service!81
No description provided.