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

feat(api): implement life event cancel / POST /v1/events/{id}/cancel #80

Merged
yuris merged 2 commits from feat/issue-75-event-cancel into main 2026-09-23 05:20:08 +00:00
Conversation 2 Commits 2 Files changed 9 +2074 -65
yuris commented 2026-09-23 04:55:43 +00:00
Owner
Copy link

Motivation

docs/technical/cli-api.md's command-to-route table advertises
life event cancel ID --etag ETAG --input - → POST /v1/events/{id}/cancel (write), but neither
half was reachable: the client had no dispatch arm for event cancel and the router registered no
route, so the command answered dependency_unavailable (exit 5). #32 moved the row to the skill's
not-implemented table so the package stopped over-claiming; this change makes the contract real.

Cancellation is the one event transition with a documented non-obvious semantic — it sets
STATUS:CANCELLED, never DELETE — and it is the same distinction #12 draws for tasks. Without it,
an agent told "cancel that block" has no correct command and would reasonably reach for a free-form
status patch, which is exactly the edit the domain model avoids.

Changes

  • src/domain/event_lifecycle.rs (new): the VEVENT lifecycle vocabulary — TENTATIVE /
    CONFIRMED / CANCELLED, with no COMPLETED — and plan_cancel, which decides the transition
    against the state the live component actually holds. A missing STATUS is RFC 5545's CONFIRMED
    default (never read as cancelled); an already-CANCELLED event plans a no-op; a STATUS outside
    the three values is refused as calendar_unsupported rather than coerced. No un-cancel is planned:
    the domain model names none.
  • src/api/lifecycle.rs: POST /v1/events/{id}/cancel (run_event_cancel) reuses the task
    transitions' two-phase discipline verbatim — mandatory quoted strong If-Match preflight, prepared
    intent durable before dispatch, conditional PUT, and one transaction that moves the operation to
    applied, appends the event_cancelled fact with before_json/after_json and upserts the
    projection. The confirmed-fact commit path was generalised over a Transition descriptor
    (component kind, fact kind, route family, verb) so events and tasks share it instead of duplicating
    it; the task paths' behaviour is otherwise unchanged.
  • src/api/mod.rs: the route registration and its handler. The scope-table entry for
    POST /v1/events/{id}/cancel (write) already existed in src/api/auth.rs and is unchanged.
  • src/cli.rs: EventSubcommands::Cancel now dispatches instead of falling through to
    not_implemented. run_lifecycle_command takes the route family and the verb, so tasks and events
    share one body; the three task verbs pass LifecycleAction::*.name(), so the domain vocabulary
    still names them rather than string literals at the call site.
  • skills/life/references/commands.md: the life event cancel row moves from the not-implemented
    table to the implemented writes table, and tests/skill_package_tests.rs's not-implemented count
    goes 2 → 1 in the same commit.

Request surface

POST /v1/events/{id}/cancel, If-Match: "<quoted strong etag>", Idempotency-Key required, body
deny_unknown_fields with optional reason, optional occurred_at (recorded as the fact's
occurred_at, defaulting to the instant of the action) and optional scope (series |
occurrence).

Specification note

docs/technical/cli-api.md documents the route, the scope and the STATUS:CANCELLED-not-DELETE
semantic, but is silent on the cancellation body members; reason, occurred_at and scope are the
surface chosen here. docs/ is excluded from version control in this clone (.git/info/exclude lists
docs/), so no specification page can travel with this PR; the choice is disclosed here rather than
appended to an untracked page.

Verification

Run in the worktree, on the final commit:

  • cargo test — whole suite: 766 passed, 0 failed, across 40 test binaries plus the doc-test
    target (41 test result lines). Baseline main is 743; the delta is 23 new tests (20 integration
    tests in tests/issue75_event_cancel.rs, 3 unit tests in src/domain/event_lifecycle.rs).
  • cargo clippy --all-targets -- -D warnings — clean, 0 errors.
  • cargo fmt --check — clean.

tests/issue75_event_cancel.rs drives real HTTP through router_with_state against a mock Radicale
that captures every request (method, path, If-Match, exact bytes) and reads the journal back from
SQLite on a separate connection, so each criterion below is asserted on observable behaviour rather
than on the code that produces it.

Per acceptance criterion:

  • life event cancel ID --etag '"v1"' --input - --json cancels the event; STATUS:CANCELLED in the
    resource, resource still present, exit 0     — met.
    cancelling_an_event_sets_status_cancelled_and_never_deletes_it (route: one PUT, 0 DELETE, the
    PUT body carries STATUS:CANCELLED plus the external client's X-CUSTOM-CLIENT-FLAG and
    VALARM, the resource is still stored, data.item.status == "CANCELLED", changed: true, and the
    operation was already prepared at dispatch time). the_cli_cancels_an_event_over_the_transition_route
    (CLI: POST /v1/events/{id}/cancel, exact If-Match, generated op_ key, exit 0).
  • Missing / weak / unquoted / * / multi-tag ETag → 400 invalid_input / exit 2, zero backend
    requests, no journal row     — met. every_unusable_etag_is_refused_before_any_dispatch covers all
    six forms (absent, empty, W/"v1", v1, *, "v1", "v2") and asserts 0 captured requests, 0
    operations rows and 0 life_events rows; the_cli_refuses_unusable_version_evidence_without_transmitting
    asserts exit 2 with nothing transmitted.
  • Stale ETag → 412 etag_conflict / exit 4, operations.state='failed', no refreshed-ETag retry
    — met. a_stale_etag_is_a_conflict_recorded_failed_and_never_retried asserts exactly one PUT, the
    failed state, no fact, an unchanged resource, and that replaying the same key returns the recorded
    failure rather than retrying.
  • Cancelling an already-cancelled event → recorded no-op (applied, changed:false, 0 PUTs, no
    duplicate fact)     — met. cancelling_an_already_cancelled_event_is_a_recorded_no_op asserts one
    PUT total across both calls, changed:false, applied, exactly one event_cancelled fact, and
    that the same key replays the no-op.
  • A cancelled event still appears in the agenda and consumes no capacity, asserted not assumed —
    met. a_cancelled_event_still_appears_in_the_agenda_and_consumes_no_capacity re-reads
    GET /v1/agenda after the cancellation and finds the item with status: "CANCELLED"; the capacity
    claim goes through the existing predicate ApiItem::event_consumes_capacity with the status and
    transparency the agenda reported (plus the opaque-confirmed control case), rather than restating the
    rule.
  • Recurring target without an explicit scope → needs_clarification / exit 3 with the
    series/occurrence choice     — met, reusing #15's scoping rule and payload shape unchanged.
    a_recurring_event_without_a_scope_is_answered_with_the_scope_question asserts status: needs_clarification, questions[0].id == "scope", options ["series","occurrence"], 0 PUTs and no
    operation row; a_series_scoped_cancellation_cancels_the_master covers the answered case.
  • before_json/after_json recorded in one transaction with the operation result — met as far as
    it is observable from outside. the_journal_records_the_transition_snapshots_with_the_operation_result
    asserts both snapshots on the single event_cancelled row (before CONFIRMED, after CANCELLED,
    same item id) together with operations.state = 'applied' and the recorded reason after one
    confirmed call; an_unconfirmed_cancellation_commits_no_fact shows the other side of the same
    boundary — when the remote write is unconfirmed the fact is absent and the operation is
    needs_reconciliation, so the fact is never committed apart from the applied result.
  • The skill's command reference lists it as implemented and tests/skill_package_tests.rs passes
    — met. The row moved to the writes table, and the not-implemented section's parsed command set is
    pinned to exactly {"sync"} (see Tests adjusted); the file passes 7/7.

Additional coverage in the same file: deny_unknown_fields refusal with nothing dispatched; a VTODO
id addressed through the event route (422 invalid_input); an event carrying STATUS:COMPLETED
refused as calendar_unsupported; a missing Idempotency-Key; no journal worker configured (503
journal_unavailable, 0 PUTs); same key with a different payload (idempotency_conflict); and the
CLI conflict path exiting 4.

Tests adjusted

  • tests/cli_protocol_tests.rs: life event cancel was listed in the
    every_body_taking_command_reads_stdin_and_refuses_a_malformed_body command array (15 → 16 entries,
    with a valid cancellation body for the file variant) and the tail block asserting that the command
    exits 5 with dependency_unavailable and dispatches nothing was deleted — that behaviour is exactly
    what this change removes, and the block would otherwise pass only if the feature were absent. No
    assertion was weakened: the command is now covered by the same stdin/file/malformed-body contract as
    every other body-taking command.
  • tests/skill_package_tests.rs: the not-implemented assertion no longer tolerates a count. It
    previously asserted checked >= 2; the section now holds one command, and rather than lowering the
    floor to >= 1 — which accepts both the old and the new count and so establishes nothing — the test
    now asserts that the set of command paths parsed out of the section equals exactly {"sync"}, which
    states the fact instead of a number. The per-row loop is unchanged: every row the section names is
    still parsed with the real clap tree and dispatched, and must exit 5.
    Falsified by hand, in both directions. Putting the life event cancel row back into the
    not-implemented section fails the test at that row's own assert_eq!(exit, DEPENDENCY_UNAVAILABLE)
    with left: 2, right: 5 — the command now really runs and exits 2 with configuration_invalid
    when no server is configured. Emptying the section, so the per-row loop checks nothing, fails at the
    set equality with left: {}, right: {"sync"} — which is the set assertion carrying the claim on
    its own. Both temporary edits were removed again; the pushed commit is the clean state.

No product defect outside this scope was found.

Related Issues

Refs #32 (where the gap was found), #11 (event update preflight), #12 (task lifecycle discipline),
#15 (recurring scope).

Closes #75

## Motivation `docs/technical/cli-api.md`'s command-to-route table advertises `life event cancel ID --etag ETAG --input -` → `POST /v1/events/{id}/cancel` (write), but neither half was reachable: the client had no dispatch arm for `event cancel` and the router registered no route, so the command answered `dependency_unavailable` (exit 5). #32 moved the row to the skill's not-implemented table so the package stopped over-claiming; this change makes the contract real. Cancellation is the one event transition with a documented non-obvious semantic — it sets `STATUS:CANCELLED`, never `DELETE` — and it is the same distinction #12 draws for tasks. Without it, an agent told "cancel that block" has no correct command and would reasonably reach for a free-form status patch, which is exactly the edit the domain model avoids. ## Changes - `src/domain/event_lifecycle.rs` (new): the VEVENT lifecycle vocabulary — `TENTATIVE` / `CONFIRMED` / `CANCELLED`, with no `COMPLETED` — and `plan_cancel`, which decides the transition against the state the live component actually holds. A missing `STATUS` is RFC 5545's `CONFIRMED` default (never read as cancelled); an already-`CANCELLED` event plans a no-op; a `STATUS` outside the three values is refused as `calendar_unsupported` rather than coerced. No un-cancel is planned: the domain model names none. - `src/api/lifecycle.rs`: `POST /v1/events/{id}/cancel` (`run_event_cancel`) reuses the task transitions' two-phase discipline verbatim — mandatory quoted strong `If-Match` preflight, prepared intent durable before dispatch, conditional `PUT`, and one transaction that moves the operation to `applied`, appends the `event_cancelled` fact with `before_json`/`after_json` and upserts the projection. The confirmed-fact commit path was generalised over a `Transition` descriptor (component kind, fact kind, route family, verb) so events and tasks share it instead of duplicating it; the task paths' behaviour is otherwise unchanged. - `src/api/mod.rs`: the route registration and its handler. The scope-table entry for `POST /v1/events/{id}/cancel` (write) already existed in `src/api/auth.rs` and is unchanged. - `src/cli.rs`: `EventSubcommands::Cancel` now dispatches instead of falling through to `not_implemented`. `run_lifecycle_command` takes the route family and the verb, so tasks and events share one body; the three task verbs pass `LifecycleAction::*.name()`, so the domain vocabulary still names them rather than string literals at the call site. - `skills/life/references/commands.md`: the `life event cancel` row moves from the not-implemented table to the implemented writes table, and `tests/skill_package_tests.rs`'s not-implemented count goes 2 → 1 in the same commit. ### Request surface `POST /v1/events/{id}/cancel`, `If-Match: "<quoted strong etag>"`, `Idempotency-Key` required, body `deny_unknown_fields` with optional `reason`, optional `occurred_at` (recorded as the fact's `occurred_at`, defaulting to the instant of the action) and optional `scope` (`series` | `occurrence`). ### Specification note `docs/technical/cli-api.md` documents the route, the scope and the `STATUS:CANCELLED`-not-`DELETE` semantic, but is silent on the cancellation body members; `reason`, `occurred_at` and `scope` are the surface chosen here. `docs/` is excluded from version control in this clone (`.git/info/exclude` lists `docs/`), so no specification page can travel with this PR; the choice is disclosed here rather than appended to an untracked page. ## Verification Run in the worktree, on the final commit: - `cargo test` — whole suite: **766 passed, 0 failed**, across 40 test binaries plus the doc-test target (41 `test result` lines). Baseline `main` is 743; the delta is 23 new tests (20 integration tests in `tests/issue75_event_cancel.rs`, 3 unit tests in `src/domain/event_lifecycle.rs`). - `cargo clippy --all-targets -- -D warnings` — clean, 0 errors. - `cargo fmt --check` — clean. `tests/issue75_event_cancel.rs` drives real HTTP through `router_with_state` against a mock Radicale that captures every request (method, path, `If-Match`, exact bytes) and reads the journal back from SQLite on a separate connection, so each criterion below is asserted on observable behaviour rather than on the code that produces it. Per acceptance criterion: - **`life event cancel ID --etag '"v1"' --input - --json` cancels the event; `STATUS:CANCELLED` in the resource, resource still present, exit 0** — met. `cancelling_an_event_sets_status_cancelled_and_never_deletes_it` (route: one `PUT`, 0 `DELETE`, the `PUT` body carries `STATUS:CANCELLED` plus the external client's `X-CUSTOM-CLIENT-FLAG` and `VALARM`, the resource is still stored, `data.item.status == "CANCELLED"`, `changed: true`, and the operation was already `prepared` at dispatch time). `the_cli_cancels_an_event_over_the_transition_route` (CLI: `POST /v1/events/{id}/cancel`, exact `If-Match`, generated `op_` key, exit 0). - **Missing / weak / unquoted / `*` / multi-tag ETag → 400 `invalid_input` / exit 2, zero backend requests, no journal row** — met. `every_unusable_etag_is_refused_before_any_dispatch` covers all six forms (absent, empty, `W/"v1"`, `v1`, `*`, `"v1", "v2"`) and asserts 0 captured requests, 0 `operations` rows and 0 `life_events` rows; `the_cli_refuses_unusable_version_evidence_without_transmitting` asserts exit 2 with nothing transmitted. - **Stale ETag → 412 `etag_conflict` / exit 4, `operations.state='failed'`, no refreshed-ETag retry** — met. `a_stale_etag_is_a_conflict_recorded_failed_and_never_retried` asserts exactly one `PUT`, the `failed` state, no fact, an unchanged resource, and that replaying the same key returns the recorded failure rather than retrying. - **Cancelling an already-cancelled event → recorded no-op (`applied`, `changed:false`, 0 PUTs, no duplicate fact)** — met. `cancelling_an_already_cancelled_event_is_a_recorded_no_op` asserts one `PUT` total across both calls, `changed:false`, `applied`, exactly one `event_cancelled` fact, and that the same key replays the no-op. - **A cancelled event still appears in the agenda and consumes no capacity, asserted not assumed** — met. `a_cancelled_event_still_appears_in_the_agenda_and_consumes_no_capacity` re-reads `GET /v1/agenda` after the cancellation and finds the item with `status: "CANCELLED"`; the capacity claim goes through the existing predicate `ApiItem::event_consumes_capacity` with the status and transparency the agenda reported (plus the opaque-confirmed control case), rather than restating the rule. - **Recurring target without an explicit scope → `needs_clarification` / exit 3 with the series/occurrence choice** — met, reusing #15's scoping rule and payload shape unchanged. `a_recurring_event_without_a_scope_is_answered_with_the_scope_question` asserts `status: needs_clarification`, `questions[0].id == "scope"`, options `["series","occurrence"]`, 0 PUTs and no operation row; `a_series_scoped_cancellation_cancels_the_master` covers the answered case. - **`before_json`/`after_json` recorded in one transaction with the operation result** — met as far as it is observable from outside. `the_journal_records_the_transition_snapshots_with_the_operation_result` asserts both snapshots on the single `event_cancelled` row (before `CONFIRMED`, after `CANCELLED`, same item id) together with `operations.state = 'applied'` and the recorded `reason` after one confirmed call; `an_unconfirmed_cancellation_commits_no_fact` shows the other side of the same boundary — when the remote write is unconfirmed the fact is absent and the operation is `needs_reconciliation`, so the fact is never committed apart from the applied result. - **The skill's command reference lists it as implemented and `tests/skill_package_tests.rs` passes** — met. The row moved to the writes table, and the not-implemented section's parsed command set is pinned to exactly `{"sync"}` (see *Tests adjusted*); the file passes 7/7. Additional coverage in the same file: `deny_unknown_fields` refusal with nothing dispatched; a VTODO id addressed through the event route (422 `invalid_input`); an event carrying `STATUS:COMPLETED` refused as `calendar_unsupported`; a missing `Idempotency-Key`; no journal worker configured (503 `journal_unavailable`, 0 PUTs); same key with a different payload (`idempotency_conflict`); and the CLI conflict path exiting 4. ### Tests adjusted - `tests/cli_protocol_tests.rs`: `life event cancel` was listed in the `every_body_taking_command_reads_stdin_and_refuses_a_malformed_body` command array (15 → 16 entries, with a valid cancellation body for the file variant) and the tail block asserting that the command exits 5 with `dependency_unavailable` and dispatches nothing was deleted — that behaviour is exactly what this change removes, and the block would otherwise pass only if the feature were absent. No assertion was weakened: the command is now covered by the same stdin/file/malformed-body contract as every other body-taking command. - `tests/skill_package_tests.rs`: the not-implemented assertion no longer tolerates a count. It previously asserted `checked >= 2`; the section now holds one command, and rather than lowering the floor to `>= 1` — which accepts both the old and the new count and so establishes nothing — the test now asserts that the set of command paths parsed out of the section equals exactly `{"sync"}`, which states the fact instead of a number. The per-row loop is unchanged: every row the section names is still parsed with the real clap tree and dispatched, and must exit 5. Falsified by hand, in both directions. Putting the `life event cancel` row back into the not-implemented section fails the test at that row's own `assert_eq!(exit, DEPENDENCY_UNAVAILABLE)` with `left: 2, right: 5` — the command now really runs and exits 2 with `configuration_invalid` when no server is configured. Emptying the section, so the per-row loop checks nothing, fails at the set equality with `left: {}`, `right: {"sync"}` — which is the set assertion carrying the claim on its own. Both temporary edits were removed again; the pushed commit is the clean state. No product defect outside this scope was found. ## Related Issues Refs #32 (where the gap was found), #11 (event update preflight), #12 (task lifecycle discipline), #15 (recurring scope). Closes #75 
`docs/technical/cli-api.md` documents `life event cancel ID --etag ETAG
--input -` -> `POST /v1/events/{id}/cancel` (write), but the client had no
dispatch arm for it and the router registered no route. Cancellation is the
one event transition with a documented non-obvious semantic: it sets
`STATUS:CANCELLED` and never deletes the resource.

- `src/domain/event_lifecycle.rs`: the VEVENT lifecycle vocabulary
  (TENTATIVE/CONFIRMED/CANCELLED, no COMPLETED) and `plan_cancel`. A missing
  STATUS is RFC 5545's CONFIRMED default; an already-cancelled event plans a
  no-op; an unknown STATUS is refused as `calendar_unsupported`. No un-cancel
  is planned: the domain model names none.
- `src/api/lifecycle.rs`: `run_event_cancel` reuses the task transitions'
  two-phase discipline verbatim (quoted strong If-Match preflight, prepared
  intent durable before dispatch, conditional PUT, one transaction committing
  the `event_cancelled` fact with before_json/after_json and the projection).
  The confirmed-fact commit path is generalised over a `Transition`
  descriptor so both kinds share it rather than duplicating it.
- `src/api/mod.rs`: the route and its handler.
- `src/cli.rs`: `event cancel` dispatches; `run_lifecycle_command` takes the
  route family and verb so tasks and events share one body.
- `skills/life/references/commands.md`: the row moves to the implemented
  writes table; `tests/skill_package_tests.rs`'s not-implemented count goes
  2 -> 1 in the same commit.
- `tests/issue75_event_cancel.rs`: 20 integration tests covering the
  acceptance criteria against a mock Radicale and the real journal.
yuris commented 2026-09-23 05:06:32 +00:00
Author
Owner
Copy link

REQUEST CHANGES

Acceptance criteria

Criterion Result Evidence
Cancellation sets STATUS:CANCELLED, never deletes, preserves the resource met tests/issue75_event_cancel.rs:456 asserts one PUT, zero DELETEs, exact If-Match, preserved X-CUSTOM-CLIENT-FLAG and VALARM, and a still-stored resource. tests/issue75_event_cancel.rs:1110 separately proves an RRULE survives series cancellation. The implementation applies only a STATUS property edit through the preservation path (src/domain/event_lifecycle.rs:130, src/api/lifecycle.rs:697).
All six unusable ETag forms refuse before dispatch and journaling met every_unusable_etag_is_refused_before_any_dispatch (tests/issue75_event_cancel.rs:665) covers absent, empty, weak, unquoted, wildcard, and multi-tag values and asserts zero backend requests, zero operations rows, and zero life_events rows for every case. The CLI companion asserts exit 2 and zero transmission.
Stale ETag is one failed PUT, no fact, no retry, replayed failure met a_stale_etag_is_a_conflict_recorded_failed_and_never_retried (tests/issue75_event_cancel.rs:813) asserts 412/etag_conflict, exit 4, exactly one PUT, operations.state='failed', zero facts, unchanged resource bytes, and no second PUT on same-key replay.
Already-cancelled event is a recorded no-op met cancelling_an_already_cancelled_event_is_a_recorded_no_op (tests/issue75_event_cancel.rs:566) checks applied, changed:false, one PUT total across the real cancellation plus no-op, exactly one event_cancelled fact, and unchanged counters after replay.
Cancelled event remains in agenda and consumes no capacity met a_cancelled_event_still_appears_in_the_agenda_and_consumes_no_capacity (tests/issue75_event_cancel.rs:521) re-reads /v1/agenda, finds the cancelled event, asserts the returned consumes_capacity:false, calls the existing ApiItem::event_consumes_capacity predicate, and includes an opaque-confirmed positive control.
Route is write-scoped and a read-only credential cannot mutate met src/api/auth.rs:120 maps exactly POST /v1/events/{id}/cancel to Scope::Write; src/api/auth.rs:356 refuses credentials lacking that scope before the handler; the route-table unit case at src/api/auth.rs:581 pins this exact path. d03_route_scopes_are_enforced_per_credential proves the same middleware refuses a read credential on write routes.
Recurring target asks for the existing series/occurrence clarification; series cancellation preserves recurrence met a_recurring_event_without_a_scope_is_answered_with_the_scope_question (tests/issue75_event_cancel.rs:1089) asserts the existing scope question/options, exit 3, zero PUTs, and zero operation rows. a_series_scoped_cancellation_cancels_the_master asserts the master becomes cancelled and RRULE:FREQ=WEEKLY;COUNT=3 remains. Both paths use the shared resolve_scoped_target/clarification_response, not a second envelope shape.
Fact snapshots and operation result share the journal boundary met the_journal_records_the_transition_snapshots_with_the_operation_result (tests/issue75_event_cancel.rs:878) checks before/after snapshots and applied; an_unconfirmed_cancellation_commits_no_fact checks zero facts with needs_reconciliation. The implementation sends operation state, fact, and projection through the single commit_operation_result transaction (src/history/journal.rs:575). This is the strongest available observable boundary; the PR's atomicity caveat is accurate.
Skill row is implemented and the not-implemented count is pinned to the real value partial The row is correctly moved to the implemented writes table (skills/life/references/commands.md:68), and only life sync remains in the not-implemented table. However, the changed assertion is checked >= 1 (tests/skill_package_tests.rs:696), so it accepts both the intended count of one and the old count of two rather than pinning the claimed count.
Documented CLI verb/flags work, including operation ID and stdin validation met EventSubcommands::Cancel exposes positional ID plus --etag, --operation-id, --input, and --scope (src/cli.rs:365); run_event_command dispatches to /v1/events/{id}/cancel through the shared conditional POST path (src/cli.rs:2889). The integration test checks generated op_ keys and exact ETag/body transmission; every_body_taking_command_reads_stdin_and_refuses_a_malformed_body includes event cancel and proves malformed stdin dispatches nothing. The shared helper uses supplied --operation-id verbatim.

Findings

  • major — tests/skill_package_tests.rs:696 — Pin the not-implemented command count instead of using checked >= 1. The PR says only life sync remains and specifically changes the count from 2 to 1, but this predicate is true for both values, so it does not establish that the life event cancel row actually left that section. Use an exact assertion for the one expected row (or assert the parsed command set equals {sync}); this makes the skill-package regression check prove the count claimed by the PR.

Verification I ran

  • nix develop --command cargo test — 766 passed, 0 failed, across 41 Cargo suite result lines (including zero-test binary/doc-test suites).
  • nix develop --command cargo clippy --all-targets -- -D warnings — clean, 0 warnings/errors.
  • nix develop --command cargo fmt --check — clean.
  • git diff --check origin/main...HEAD — clean.

PR-body claims

I reproduced the claimed 766/0 test result and clean clippy/fmt runs. The following claim is stronger than the code supports:

“The assertion still fails if the section stops matching what the CLI really refuses.”

The loop checks behavior of rows it happens to find, but the final lower bound accepts either one or two checked rows. It therefore does not prove the PR's stated not-implemented count of one.

The journal caveat is honest: a black-box test cannot observe an intermediate state inside one SQLite transaction. The combination of the single transaction in commit_operation_result, the confirmed-boundary test, and the unconfirmed zero-fact test is the strongest evidence available without transaction-level fault injection.

Anything I could not check

  • I did not run against a live Radicale deployment; backend behavior was exercised through the repository's capturing HTTP fixture.
  • I could not externally interrupt SQLite between statements inside commit_operation_result; atomicity was checked by reading the transaction implementation and by the observable confirmed/unconfirmed boundary tests.
REQUEST CHANGES ## Acceptance criteria | Criterion | Result | Evidence | |---|---|---| | Cancellation sets `STATUS:CANCELLED`, never deletes, preserves the resource | met | `tests/issue75_event_cancel.rs:456` asserts one PUT, zero DELETEs, exact `If-Match`, preserved `X-CUSTOM-CLIENT-FLAG` and `VALARM`, and a still-stored resource. `tests/issue75_event_cancel.rs:1110` separately proves an `RRULE` survives series cancellation. The implementation applies only a `STATUS` property edit through the preservation path (`src/domain/event_lifecycle.rs:130`, `src/api/lifecycle.rs:697`). | | All six unusable ETag forms refuse before dispatch and journaling | met | `every_unusable_etag_is_refused_before_any_dispatch` (`tests/issue75_event_cancel.rs:665`) covers absent, empty, weak, unquoted, wildcard, and multi-tag values and asserts zero backend requests, zero `operations` rows, and zero `life_events` rows for every case. The CLI companion asserts exit 2 and zero transmission. | | Stale ETag is one failed PUT, no fact, no retry, replayed failure | met | `a_stale_etag_is_a_conflict_recorded_failed_and_never_retried` (`tests/issue75_event_cancel.rs:813`) asserts 412/`etag_conflict`, exit 4, exactly one PUT, `operations.state='failed'`, zero facts, unchanged resource bytes, and no second PUT on same-key replay. | | Already-cancelled event is a recorded no-op | met | `cancelling_an_already_cancelled_event_is_a_recorded_no_op` (`tests/issue75_event_cancel.rs:566`) checks `applied`, `changed:false`, one PUT total across the real cancellation plus no-op, exactly one `event_cancelled` fact, and unchanged counters after replay. | | Cancelled event remains in agenda and consumes no capacity | met | `a_cancelled_event_still_appears_in_the_agenda_and_consumes_no_capacity` (`tests/issue75_event_cancel.rs:521`) re-reads `/v1/agenda`, finds the cancelled event, asserts the returned `consumes_capacity:false`, calls the existing `ApiItem::event_consumes_capacity` predicate, and includes an opaque-confirmed positive control. | | Route is write-scoped and a read-only credential cannot mutate | met | `src/api/auth.rs:120` maps exactly `POST /v1/events/{id}/cancel` to `Scope::Write`; `src/api/auth.rs:356` refuses credentials lacking that scope before the handler; the route-table unit case at `src/api/auth.rs:581` pins this exact path. `d03_route_scopes_are_enforced_per_credential` proves the same middleware refuses a read credential on write routes. | | Recurring target asks for the existing series/occurrence clarification; series cancellation preserves recurrence | met | `a_recurring_event_without_a_scope_is_answered_with_the_scope_question` (`tests/issue75_event_cancel.rs:1089`) asserts the existing `scope` question/options, exit 3, zero PUTs, and zero operation rows. `a_series_scoped_cancellation_cancels_the_master` asserts the master becomes cancelled and `RRULE:FREQ=WEEKLY;COUNT=3` remains. Both paths use the shared `resolve_scoped_target`/`clarification_response`, not a second envelope shape. | | Fact snapshots and operation result share the journal boundary | met | `the_journal_records_the_transition_snapshots_with_the_operation_result` (`tests/issue75_event_cancel.rs:878`) checks before/after snapshots and `applied`; `an_unconfirmed_cancellation_commits_no_fact` checks zero facts with `needs_reconciliation`. The implementation sends operation state, fact, and projection through the single `commit_operation_result` transaction (`src/history/journal.rs:575`). This is the strongest available observable boundary; the PR's atomicity caveat is accurate. | | Skill row is implemented and the not-implemented count is pinned to the real value | partial | The row is correctly moved to the implemented writes table (`skills/life/references/commands.md:68`), and only `life sync` remains in the not-implemented table. However, the changed assertion is `checked >= 1` (`tests/skill_package_tests.rs:696`), so it accepts both the intended count of one and the old count of two rather than pinning the claimed count. | | Documented CLI verb/flags work, including operation ID and stdin validation | met | `EventSubcommands::Cancel` exposes positional ID plus `--etag`, `--operation-id`, `--input`, and `--scope` (`src/cli.rs:365`); `run_event_command` dispatches to `/v1/events/{id}/cancel` through the shared conditional POST path (`src/cli.rs:2889`). The integration test checks generated `op_` keys and exact ETag/body transmission; `every_body_taking_command_reads_stdin_and_refuses_a_malformed_body` includes event cancel and proves malformed stdin dispatches nothing. The shared helper uses supplied `--operation-id` verbatim. | ## Findings - major — `tests/skill_package_tests.rs:696` — Pin the not-implemented command count instead of using `checked >= 1`. The PR says only `life sync` remains and specifically changes the count from 2 to 1, but this predicate is true for both values, so it does not establish that the `life event cancel` row actually left that section. Use an exact assertion for the one expected row (or assert the parsed command set equals `{sync}`); this makes the skill-package regression check prove the count claimed by the PR. ## Verification I ran - `nix develop --command cargo test` — **766 passed, 0 failed**, across **41 Cargo suite result lines** (including zero-test binary/doc-test suites). - `nix develop --command cargo clippy --all-targets -- -D warnings` — clean, 0 warnings/errors. - `nix develop --command cargo fmt --check` — clean. - `git diff --check origin/main...HEAD` — clean. ## PR-body claims I reproduced the claimed 766/0 test result and clean clippy/fmt runs. The following claim is stronger than the code supports: > “The assertion still fails if the section stops matching what the CLI really refuses.” The loop checks behavior of rows it happens to find, but the final lower bound accepts either one or two checked rows. It therefore does not prove the PR's stated not-implemented count of one. The journal caveat is honest: a black-box test cannot observe an intermediate state inside one SQLite transaction. The combination of the single transaction in `commit_operation_result`, the confirmed-boundary test, and the unconfirmed zero-fact test is the strongest evidence available without transaction-level fault injection. ## Anything I could not check - I did not run against a live Radicale deployment; backend behavior was exercised through the repository's capturing HTTP fixture. - I could not externally interrupt SQLite between statements inside `commit_operation_result`; atomicity was checked by reading the transaction implementation and by the observable confirmed/unconfirmed boundary tests.
The section's assertion tolerated a count (`checked >= 2`, lowered to
`checked >= 1`), which accepts both the old and the new count and so does not
establish that `life event cancel` left the section. Assert the parsed command
set equals `{"sync"}` instead: the fact the reference is making, stated as a
fact. The per-row loop is unchanged.

Falsified by hand: re-adding the `life event cancel` row fails the row's own
`assert_eq!(exit, DEPENDENCY_UNAVAILABLE)` (left: 2, right: 5), and emptying
the section fails the set equality (left: {}, right: {"sync"}).
yuris commented 2026-09-23 05:19:49 +00:00
Author
Owner
Copy link

APPROVE

I re-reviewed the follow-up commit c2d3d2841b653a3d70769669f0831a13b7724d12. The new BTreeSet<String> assertion is a real cutover pin: it requires the parsed not-implemented command paths to equal exactly {sync}, so it rejects both an omitted row ({}) and a stale extra row ({sync, event cancel}), unlike the former lower-bound count. validate_strict(&root, &tokens) makes each row meaningful by validating the whole documented command line against the actual clap tree: command path, options and their values, and positional arguments before the row is dispatched. The per-row dispatch assertion still requires every named row to return exit 5, so the set assertion cannot be satisfied by merely naming arbitrary parseable commands.

The reported falsification is consistent with the diff and current behavior: restoring life event cancel to that section would make its row be dispatched and fail the row-level exit-5 assertion (the implemented command returns exit 2 without a configured endpoint), while emptying the section leaves {} and fails the exact-set assertion. The temporary edits are not present in the pushed diff.

I reran nix develop --command cargo test --test skill_package_tests (7 passed), nix develop --command cargo clippy --all-targets -- -D warnings (clean), and nix develop --command cargo fmt --check (clean). The prior full-suite run on the implementation commit was 766 passed / 0 failed across 41 result lines; this follow-up only strengthens the skill-package regression assertion. No unpublished findings remain.

APPROVE I re-reviewed the follow-up commit `c2d3d2841b653a3d70769669f0831a13b7724d12`. The new `BTreeSet<String>` assertion is a real cutover pin: it requires the parsed not-implemented command paths to equal exactly `{sync}`, so it rejects both an omitted row (`{}`) and a stale extra row (`{sync, event cancel}`), unlike the former lower-bound count. `validate_strict(&root, &tokens)` makes each row meaningful by validating the whole documented command line against the actual clap tree: command path, options and their values, and positional arguments before the row is dispatched. The per-row dispatch assertion still requires every named row to return exit 5, so the set assertion cannot be satisfied by merely naming arbitrary parseable commands. The reported falsification is consistent with the diff and current behavior: restoring `life event cancel` to that section would make its row be dispatched and fail the row-level exit-5 assertion (the implemented command returns exit 2 without a configured endpoint), while emptying the section leaves `{}` and fails the exact-set assertion. The temporary edits are not present in the pushed diff. I reran `nix develop --command cargo test --test skill_package_tests` (7 passed), `nix develop --command cargo clippy --all-targets -- -D warnings` (clean), and `nix develop --command cargo fmt --check` (clean). The prior full-suite run on the implementation commit was 766 passed / 0 failed across 41 result lines; this follow-up only strengthens the skill-package regression assertion. No unpublished findings remain.
yuris merged commit 1dfffa8745 into main 2026-09-23 05:20:08 +00:00
yuris changed title from feat(api): implement life event cancel / POST /v1/events/{id}/cancel to feat(api): implement life event cancel / POST /v1/events/{id}/cancel 2026-09-23 06:25:44 +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!80
No description provided.