diff --git a/docs/architecture/materials-and-work-links.md b/docs/architecture/materials-and-work-links.md index 0970c9a..d4ad138 100644 --- a/docs/architecture/materials-and-work-links.md +++ b/docs/architecture/materials-and-work-links.md @@ -87,13 +87,43 @@ per linked block: itself. `_dead_link_violation` (`src/tmbx/service.py`) refuses the whole commit if -any block's handle is not in the material store, naming the block and the -handle. This is stricter than the check at `apply` time, which lets a block -the patch never touched keep a handle whose row is already gone — refusing -there would hide the rest of the day from whoever has to fix it. At commit -there is nothing left to guess from: a handle with no row means no URL to -write, so the commit is refused until that block's link is cleared with an -explicit null. +a linked block's handle is not in the material store, naming the block and +the handle — unless the block is one tmbx does not own, which this check +skips (see below). This is stricter than the check at `apply` time, which +lets a block the patch never touched keep a handle whose row is already +gone — refusing there would hide the rest of the day from whoever has to +fix it. At commit there is nothing left to guess from: a handle with no row +means no URL to write, so the commit is refused until that block's link is +cleared with an explicit null. + +`_long_description_violation` (`src/tmbx/service.py`) refuses the whole +commit if a linked block's description is over `MAX_DESCRIPTION_CHARS` +(`src/tmbx/calendar/port.py`). The limit falls out of the composition +above: a linked event's *displayed* description has the material's URL +appended to it, so the text a person actually wrote has to survive +somewhere else — the `tmbx.desc` private property covered below — and that +property is length-capped by the provider. Over the limit there is nowhere +left to put the authored text, so the commit refuses rather than +truncating it: silently shortening a description loses what somebody +wrote, and the loss would only surface the next time the day was read +back. The check sits here, in the service, before anything is written, +rather than in the adapter that owns the limit (`_private_properties` in +`src/tmbx/calendar/gcal.py`) — the adapter can only raise from inside the +commit's event loop, past the journal and with some events already +written, which would leave a half-committed day with no journal entry to +explain it. The adapter keeps its own raise as a backstop for any caller +that reaches it without going through the service. + +**Neither refusal checks a block tmbx does not own.** The remedy each one +names — clear the link, or shorten the description — is a write to the +foreign event, and `_foreign_touches` refuses exactly that: tmbx must +never write an event it doesn't own. Checking a foreign block here would +refuse every commit of that day forever, with no way to comply. #398 +added the skip to `_dead_link_violation`; #399, a release later, added +the identical skip to `_long_description_violation`, for the same reason. +The two refusals sit one screen apart in `service.py` and share this +deadlock, so a filter added to either belongs on both — worth checking +for, whoever adds a third commit-time refusal next. ## The private map merges, so a cleared key is sent empty, not omitted @@ -106,6 +136,11 @@ back as absence. This applies to every key that predates links as much as to `tmbx.link` itself: a required-kind slug removed from a block, or an authored description that had changed, previously stayed on the event under the old value and could silently win over what the user actually wrote. +`_write_event_args` (`src/tmbx/calendar/gcal.py`) used to guard this map +behind `if private:`, which read as though the map were sometimes left +out; it never was, since `_private_properties` returns a fixed eight-entry +dict that is never empty, so the guard could not fail. The guard is gone +now and the code matches what this section has always described. ## Undo restores the link, not just the block diff --git a/src/fateforger/slack_bot/timeboxing_host.py b/src/fateforger/slack_bot/timeboxing_host.py index 15a8192..9257445 100644 --- a/src/fateforger/slack_bot/timeboxing_host.py +++ b/src/fateforger/slack_bot/timeboxing_host.py @@ -292,9 +292,18 @@ async def resolve( self._work_refs(snapshot, day), return_exceptions=True, ) - for settled in (constraints, work): - if isinstance(settled, BaseException): - raise settled + # Narrowed one name at a time, rather than by looping over the pair: + # a loop narrows only its own variable, so `constraints` and `work` + # stayed `T | BaseException` for the reader and for a type checker, + # and `work.facts` below is an attribute access on that union. Runtime + # was always correct -- the loop does raise -- but nothing gates this + # path today, so the union was load-bearing on no one noticing. + # Constraints first keeps the order a caller saw when these ran in + # sequence, which is what the comment above promises. + if isinstance(constraints, BaseException): + raise constraints + if isinstance(work, BaseException): + raise work return PlanningContext( facts=[ *planning_facts( diff --git a/src/tmbx/calendar/gcal.py b/src/tmbx/calendar/gcal.py index 987e1b4..60379dd 100644 --- a/src/tmbx/calendar/gcal.py +++ b/src/tmbx/calendar/gcal.py @@ -494,27 +494,34 @@ def _write_event_args(event: CalendarEvent, *, tz: str) -> dict[str, Any]: not a nested ``{dateTime, timeZone}`` object; that nested shape shows up only in this repo's throwaway dev seed scripts, not the tool's actual schema. ``extendedProperties.private`` carries identity plus - ``block_type``/``timing_mode``/``anchor_source``/``link``/``desc`` — - see the module docstring — and is included only when at least one of - those eight is set, since a foreign event is never written here at all - (the service never calls create/update for one). + ``block_type``/``timing_mode``/``anchor_source``/``link``/``desc`` — see + the module docstring — and is sent **always, with all eight keys**, even + when every one of them is empty. + + That last part is the correction, not a detail. This said the map was + "included only when at least one of those eight is set", and a guard here + implemented it. Both stopped being true when ``_private_properties`` began + writing ``None`` as an empty string rather than dropping the key: it now + returns a fixed eight-entry map that is never falsy, so the guard could + not fail and the sentence described a behaviour nothing performed. An + event whose identity was cleared is precisely the one that must say so on + the wire — the server *merges* this map, so a key left out keeps its last + value — and ``test_an_event_with_no_identity_sends_every_key_empty`` is + what holds that. The description sent is not ``event.description`` verbatim: a linked event's url is appended to it (``_description_for``), and the authored text goes into ``tmbx.desc`` so the composition can be reversed on the way back (``_private_properties``). """ - private = _private_properties(event) - args: dict[str, Any] = { + return { "summary": event.summary, "description": _description_for(event), "start": event.start.isoformat(timespec="seconds"), "end": event.end.isoformat(timespec="seconds"), "timeZone": tz, + "extendedProperties": {"private": _private_properties(event)}, } - if private: - args["extendedProperties"] = {"private": private} - return args def _normalize_events(payload: Any) -> list[dict[str, Any]]: diff --git a/src/tmbx/service.py b/src/tmbx/service.py index 76a9ac5..b8376bc 100644 --- a/src/tmbx/service.py +++ b/src/tmbx/service.py @@ -678,7 +678,9 @@ def _unknown_link_violation( ) -def _long_description_violation(plan: Plan) -> Violation | None: +def _long_description_violation( + plan: Plan, foreign_handles: Collection[str] +) -> Violation | None: """The refusal for a linked block whose description cannot be round-tripped. ``None`` when every linked block fits. A provider keeps a linked block's @@ -695,6 +697,17 @@ def _long_description_violation(plan: Plan) -> Violation | None: committing. So this is a cost of attaching a ticket, and the message says which block to shorten. + **A foreign block is not checked**, for the reason ``_dead_link_violation`` + skips one directly above: ``_write`` never touches a foreign event, so + there is no description to round-trip and nothing is half-answered — and + both remedies this message offers, shortening the description and dropping + the link, are updates to a foreign handle, which ``_foreign_touches`` + refuses. Checking one would refuse every commit of that day forever with + no way out. The filter arrived here a release after the one below it, + which is the whole reason this paragraph is here: the two refusals sit one + screen apart and share a deadlock, so a filter added to either belongs on + both. + It lives here, before the write, rather than in the adapter that owns the limit, because the adapter can only raise from inside the commit's event loop — past the journal, with some events already written and the delete @@ -706,7 +719,9 @@ def _long_description_violation(plan: Plan) -> Violation | None: too_long = [ block for block in plan.blocks - if block.link is not None and len(block.d) > MAX_DESCRIPTION_CHARS + if block.link is not None + and len(block.d) > MAX_DESCRIPTION_CHARS + and block.h not in foreign_handles ] if not too_long: return None @@ -1082,7 +1097,7 @@ async def _commit_once( dead = _dead_link_violation(patched, materials, foreign_handles) if dead is not None: raise PlanViolation(dead) - too_long = _long_description_violation(patched) + too_long = _long_description_violation(patched, foreign_handles) if too_long is not None: raise PlanViolation(too_long) except PlanViolation as exc: diff --git a/tests/unit/tmbx/test_link_reaches_the_calendar.py b/tests/unit/tmbx/test_link_reaches_the_calendar.py index 18b31d7..d37b246 100644 --- a/tests/unit/tmbx/test_link_reaches_the_calendar.py +++ b/tests/unit/tmbx/test_link_reaches_the_calendar.py @@ -467,7 +467,12 @@ def test_max_description_chars_is_exported_by_the_port(): def _foreign_event( - eid: str, start_h: int, end_h: int, *, link_id: str | None = None + eid: str, + start_h: int, + end_h: int, + *, + link_id: str | None = None, + description: str = "", ) -> CalendarEvent: """A calendar event tmbx did not write: no ``uid``, so no ownership. @@ -479,6 +484,7 @@ def _foreign_event( return CalendarEvent( event_id=eid, summary="Standup", + description=description, start=datetime(2026, 8, 17, start_h, 0), end=datetime(2026, 8, 17, end_h, 0), etag="v1", @@ -567,6 +573,104 @@ async def test_an_owned_blocks_dead_handle_still_refuses(tmp_path, materials): assert DEAD_HANDLE in str(excinfo.value) +async def test_a_foreign_blocks_long_description_does_not_refuse_the_day( + tmp_path, materials, known +): + """The same deadlock, one screen below the one that was fixed. + + ``_dead_link_violation`` learned to skip a foreign block; the refusal + directly after it did not. So a foreign block carrying a *live* handle and + an over-long description refused every commit of the day, and the two + remedies the message offers -- shorten the description, or drop the link -- + are both writes to a foreign event, which ``_foreign_touches`` refuses. + Nothing tmbx owns is broken and the day cannot be planned: the deadlock + ``test_a_foreign_blocks_dead_handle_does_not_refuse_the_day`` describes, + reached by the other door. + + Live handle, not ``DEAD_HANDLE``, so the dead-link check passes it through + and this refusal is the only one left that can fire. + """ + calendar = RecordingCalendar( + { + "primary": [ + _foreign_event( + "f1", 9, 10, link_id=known, description="x" * (MAX_DESCRIPTION_CHARS + 1) + ), + _event("e2", "DW1", 10, 12, description="focus block"), + ] + } + ) + service = await _service(tmp_path, materials, calendar) + _plan, snapshot = await service.read("primary", DAY) + + result = await service.commit( + snapshot, Patch(ops=[UpdateBlock(h="DW1", d="deep focus")]) + ) + + assert result.committed is True + + +async def test_neither_remedy_for_a_foreign_long_description_is_available( + tmp_path, materials, known +): + """Why the filter, and not a message telling the caller to fix it. + + The half that makes it a deadlock rather than an inconvenience: both + remedies the refusal names are updates to a foreign handle, and both are + refused before they reach the calendar. + """ + calendar = RecordingCalendar( + { + "primary": [ + _foreign_event( + "f1", 9, 10, link_id=known, description="x" * (MAX_DESCRIPTION_CHARS + 1) + ), + _event("e2", "DW1", 10, 12), + ] + } + ) + service = await _service(tmp_path, materials, calendar) + plan, snapshot = await service.read("primary", DAY) + foreign = next(block.h for block in plan.blocks if block.link == known) + + for remedy in ({"d": "short"}, {"link": None}): + with pytest.raises(ForeignBlockError): + await service.commit( + snapshot, + Patch.model_validate( + {"ops": [{"op": "update", "h": foreign, **remedy}]} + ), + ) + + +async def test_an_owned_blocks_long_description_still_refuses( + tmp_path, materials, known +): + """The filter narrows the check to what tmbx owns; it does not remove it.""" + calendar = RecordingCalendar( + { + "primary": [ + _foreign_event("f1", 9, 10), + _event( + "e2", + "DW1", + 10, + 12, + link_id=known, + description="x" * (MAX_DESCRIPTION_CHARS + 1), + ), + ] + } + ) + service = await _service(tmp_path, materials, calendar) + _plan, snapshot = await service.read("primary", DAY) + + with pytest.raises(PlanViolation) as excinfo: + await service.commit(snapshot, Patch(ops=[UpdateBlock(h="DW1", n="Focus")])) + + assert excinfo.value.violation.kind is ViolationKind.DESCRIPTION_TOO_LONG + + # --------------------------------------------------------------------------- # undo puts the url back, and never refuses over a link # ---------------------------------------------------------------------------