feat(mcp): add delete_occurrence, refusing when it would send calendar mail #19
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat-delete-occurrence"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs #16, step 3. The delete half of "I need to be able to delete or potentially change individual elements of a series."
Hold this merge
Do not merge until the
EXDATEbehaviour is measured against a real Stalwart. Every test below runs against a mock, so they prove this repository writes the iCalendar it intends to write. None of them proves the server acts on it. That is exactly the claim #16 calls the whole hazard — aPUTthe server accepts while the occurrence keeps rendering — and it is unmeasured untilcaldav-test@kampong.socialexists.Merging this green would be reporting a check as passed that was never run against the thing it is about.
What it does
Adds an
EXDATEto the master and deletes the override for that occurrence, in one conditionalPUT.The value type and
TZIDcome from the master'sDTSTART, never from the caller. RFC 5545 requires the exclusion to match the occurrence as theRRULEgenerates it, and a mismatch is accepted and excludes nothing. Taking both fromDTSTARTmakes the wrong shape unrepresentable rather than merely documented — the caller supplies only the value.The override goes in the same write. Left behind it is an orphan: clients render an occurrence the series says does not exist.
Three refusals, each because the server would accept and then not act
recurrence_idwhose value type disagrees withDTSTARTDTSTARTas implicitly includedRANGE=THISANDFUTURE, which applies to that occurrence and every later one and is a different operationThat third one is the explicit rejection #16 asked for on the write side. The read side landed in
3dfc511.The scheduling refusal
Refuses when the object carries any
ATTENDEE, naming every one, unless the call passessend_scheduling_messages: true.There is no way to make this write quiet, and that is a property of the server rather than a gap here: Stalwart parses
Schedule-Reply: Fbut consults it only incrates/dav/src/calendar/delete.rs;update.rsnever reads it.SCHEDULE-AGENT=CLIENTdoes work on aPUTand is persistent data that would also silence every later legitimate update. So the only honest options are sending the mail and not touching the event, and those are the only two offered. Per call rather than per session, because the cost is paid per operation.It names every attendee, including those on overrides, and says the server decides which are actually mailed —
send_update_messages()requires!email.is_local, and we cannot tell which domains Stalwart hosts. Naming too many is recoverable; naming too few puts mail in a stranger's inbox.The addresses stay out of telemetry:
audit::error_classrecords a static class string, never the message.What it does not do
recurrence_idis trusted as given. There is noRRULEexpander here, so an unmatched value writes anEXDATEthat excludes nothing. The tool description says that in those words rather than implying a validation that does not happen — an honest limitation beats a complete one later, and a caller looking at an occurrence in a client already has the value.Verified by mutation
Five, each with the pattern asserted to apply and the harness checked for a
test result:line, after both halves of that went wrong earlier today.EXDATEhead losesTZIDandVALUE=DATEPUTbody assertionexcluding_an_occurrence_writes_an_exdate_shaped_like_dtstart, and the wiremock testexcluding_twice_is_idempotentanddelete_occurrence_does_not_put_when_already_excludedan_attended_series_refuses_and_names_every_attendee, aloneRANGEcheck removeda_this_and_future_override_is_rejected_rather_than_excluded, aloneThe third row is the one worth reading: an already-excluded occurrence must produce no
PUTat all, not a no-opPUT. A no-op write bumpsSEQUENCEand, on an attended series, sends mail for a change that did not happen.delete_occurrence_does_not_put_when_already_excludedis a wiremock test with noPUTmock mounted, so a stray write fails verification rather than passing unnoticed.Gates
cargo fmt --check,cargo clippy -D warnings,cargo test --all-features(120 + 9),cargo audit,cargo deny check bans licenses sources— green onrustc 1.98.0, through the shared build slot.The refusal message and its doc comment said Stalwart sends no iMIP to an attendee hosted on itself, and that we cannot tell which domains those are. Both halves are wrong, and verified so at v0.16.14 rather than relayed: // crates/groupware/src/scheduling/mod.rs let is_local = local_addresses.contains(&email); `local_addresses` comes from `build_account_info`, which fills it with the authenticated account's own addresses and its groups', each expanded across the domain's names. It is an identity set, not the set of hosted domains — `is_local_domain()` exists separately and is not what this uses. So the only address `send_update_messages()` suppresses is the caller's own, and a second mailbox on the same server is not local to the acting account and does receive an iMIP. The refusal now says to assume every named attendee is written to. That is the same conservative behaviour as before, reached by a correct mechanism instead of one that understated the risk. The bad belief also had a live consequence: an experiment designed on it would have expected no mail for a same-server attendee, so a correct run would have looked like a bug. AGENTS.md says that in those words. Refs #16Still held. This is maintenance, not a step toward merging.
Brought up to date with
mainatb96aa45, green on both contexts,mergeable=trueagain. The hold is unchanged: every test here runs against a mock, none proves Stalwart acts on the write, and "the server accepts thePUTand the occurrence still renders" is the whole hazard. The measurement on #16 is still unrun and still waiting on one token.Why now rather than at merge time. Nine
AGENTS.mdentries have landed onmainsince this branch was cut, and it had gone tomergeable=falseon a content conflict. Resolving that at the moment the hold lifts would mean doing it under time pressure, next to a decision about whether a measurement cleared. Doing it now separates the two.A merge rather than a rebase, deliberately. A rebase would need a force push, which costs a human approval, and habitual force-pushing removes the signal from the ones that are real. Nothing here needs history rewritten: the conflict is purely additive, both sides append to Known Pitfalls, so both sets are kept.
What changed in the branch: nothing of its own.
delete_occurrenceand its five mutation-verified guards are as they were at1e88356. The merge brings in the v0.2.0 and v0.2.1 work frommain— the limiter observability,get_event_raw, the master-component fix — and 128 tests pass together.Note for whoever merges this eventually:
mainis now protected. Direct pushes are refused,CI / cargo*is required, and approvals are zero, so merging this remains a one-call operation once the measurement clears.d3f6bd2b25c67fab7c6eThe blocker here is false, and I am the one who should have been asked six days ago.
The premise. A
PROPFIND /dav/cal/returning 401 as the fixture account was read as the directory is OIDC and cannot hold passwords. A 401 is returned by both that and this principal has no password set, so it could not have distinguished them.The discriminating measurement is a different population, not a better request: whether any other principal carries one.
x:Account/getagainststalwart-mail-0, counts of entries, values never read:So the directory holds passwords. The precedent is the same shape as what this needs:
scratch-spf-probeis a probe principal with a password, in this directory, created for this reason. The fixtures were never missing accounts. They were missing one field.Both are outside the mail path, checked against the live outbound routing table rather than against the word "test" in their names: neither appears in it, and both have
aliases=0andgroups=0. A credential on them cannot affect anyone's mail.Dispatched to
platform: Stalwart-local credential on those two principals, no OIDC exception, stored in 1Password, verified by a realPROPFINDround trip. Hazard named in the brief rather than discovered in a report —x:Account/setreplaces thecredentialsobject rather than merging into it, andjulianhas six entries.The
waiting-on-julianlabel is now wrong and should come off. Nothing here needs Julian: no directory exception is required, and he is not the one who lifts this. Leaving it puts a false entry on his queue infleet-ledger.sh waiting, which is worse than no label.blockedis still right, and the blocker is nowplatformsetting the credential.If the
PROPFINDstill returns 401 with one set, that is a real directory finding and I want it in that form. Do not build a second acceptance path to get a green.Unblocked:
PROPFIND /dav/cal/returns 207 for both fixtures. And my mechanism in the comment above is wrong — correcting it here rather than editing it.What I got wrong
I wrote "so the directory holds passwords". It does not.
x:Account/setoncredentialsis refused for these accounts:x:AppPassword/setis what works. So the original six-day belief was half right: the directory is external and cannot hold a primary password, and it holds an app password fine.My count was a correct measurement supporting a wrong conclusion. "6 of 26 carry credentials" is true, and
credentialsconflates two slot types. The fact that actually predicts the outcome is the breakdown: four of those six hold only AppPassword slots, and onlyjulianandreportshave a Password slot, which predates the directory. I read a total where the composition was the answer.platformfound this by attempting the set on a throwaway principal rather than on a target, which is the only reason a forbidden response cost nothing.A trap for whoever picks this up
The 1Password items that already existed for both fixtures are not Stalwart credentials. Both had passwords, 28 and 32 characters, while Stalwart reported
credEntries=0for both principals. Tested rather than assumed:The old value is indistinguishable from a wrong one. The attendee item's URL is
login.kampong.social, so at least that one is a Logto credential. Both old items were left untouched; two new ones were created, and their notes say which is which.The credentials
Paths, since three of four guesses were wrong
Labels
Both
blockedandwaiting-on-julianshould come off. Nothing here needed Julian at any point, and it is no longer blocked. Leavingwaiting-on-julianputs a false entry on his queue infleet-ledger.sh waiting.The measurement ran and the hold is lifted. Two of my own refusals came out, and the review found two defects I put in.
Measurement: #16,
issuecomment-17026. Executed against the deployed Stalwart in a collection created and deleted for the purpose, containment as committed, no fixture carryingATTENDEEorORGANIZER.Stalwart matches an
EXDATEby instant, not by literal form. So two refusals in this change, both pre-registered to be removed if the run went against them, are gone at6317289:EXDATE:20260903T000000Zremoves an occurrence of an all-day seriesEXDATEforDTSTART's own occurrence removed it, leaving five of sixThe
RANGE=THISANDFUTURErefusal stays. It is not a claim about the server: that parameter names a different operation, so excluding the occurrence is not what the caller asked for whatever the server would do.The exclusion is now shaped to the caller's value rather than to
DTSTART's parameters, because that combination became reachable when the value-type refusal went, and a date under aTZID=head is malformed however forgiving the matching is.The cross-engine review, run on the diff and not on the code it repairs
Asked as can this new code do the wrong thing in the case it was written for. It found two, at
3139ebd, and both are the repair carrying a defect of the family it repaired.The idempotency check compared values and ignored the zone. An existing
EXDATE;TZID=UTC:20260908T090000counted as already excluding the 09:00 Singapore occurrence. It does not, and the consequence was worse than a redundant write: the exclusion was skipped, the override was still removed, and the occurrence reappeared while the call reported success. That defect exists because the measurement changed the model — learning that matching is by instant retired the two refusals I had on trial and invalidated a third assumption I had not questioned.remove_overridestopped at the first match. Duplicate overrides for one occurrence left the rest orphaned by the newEXDATE, so a later read reports the occurrence the deletion claimed to remove. Duplicates are invalid iCalendar, and accepting them while preserving the contradictory half is the wrong direction for a deletion tool.What is unchanged
The hazard is confirmed, only its cause was misdescribed. A wrong instant is still accepted with 201 and still excludes nothing, and a value the
RRULEnever generates is indistinguishable from a wrongly-zoned one. That is why an expander is the only possible pre-flight check, and it moves ahead oftruncate_seriesas pre-registered.recurrence_idis still trusted as given, and the tool description still says so.Mutations
Every fix has a test that dies when it is reverted, and nothing else dies:
VALUE=DATERANGEcheck removeda_this_and_future_override_is_rejected_rather_than_excludedan_exdate_in_a_different_zone_is_not_already_excludedremove_overridestops at the first matchevery_override_for_the_occurrence_is_removed_not_just_the_firstGates
fmt --check,clippy -D warnings,test --all-features(141 + 9),audit,deny— green throughbuild-slot.sh.3139ebd10f43313a995d