Skip to content

tmbx: the description refusal skips a foreign block, like the one above it (#399) - #414

Open
hugolytics wants to merge 2 commits into
mainfrom
fix/399-tmbx-follow-ups
Open

tmbx: the description refusal skips a foreign block, like the one above it (#399)#414
hugolytics wants to merge 2 commits into
mainfrom
fix/399-tmbx-follow-ups

Conversation

@hugolytics

Copy link
Copy Markdown
Collaborator

Takes the three LOW items from #399 — the slice that is fixes rather than decisions. The decision items on that ticket (capping a planning turn, the FakeCalendar wire map, withholding material_put, the retraction gap) are deliberately left open; each is a call for Hugo, not an implementation, and #399 stays open for them.

The bug: a foreign block deadlocks the day, by the other door

#398 taught _dead_link_violation to skip a block tmbx does not own. The reason is a deadlock: the refusal names one remedy — clear the link with an explicit null — and that null is a write to a foreign event, which _foreign_touches refuses outright. So checking a foreign block refuses every commit of that day forever, with no way out.

_long_description_violation sits one screen below it and never learned the same thing. A foreign block carrying a live handle and an over-long description refused every commit of that day, and both remedies that message offers — shorten the description, or drop the link — are foreign writes that cannot happen. Nothing tmbx owns is broken; the day simply cannot be planned.

Same one-word fix, foreign_handles, already in scope at the call site.

The docstring now records the pairing rather than just the fix: the two refusals share a deadlock, so a filter added to either belongs on both. That is the part that would have prevented this landing a release late.

A guard that could not fail

_write_event_args claimed the private map was "included only when at least one of those eight is set", and an if private: implemented it. Both stopped being true when _private_properties began writing None as an empty string rather than dropping the key — it returns a fixed eight-entry map that is never falsy.

Nothing changes on the wire. test_an_event_with_no_identity_sends_every_key_empty already pinned the real behaviour; this aligns the code and its prose with the test that was already right.

A union nothing was gating

After gather(..., return_exceptions=True) in timeboxing_host.resolve, narrowing by looping over the pair narrows only the loop variable — so constraints and work stayed T | BaseException, and work.facts below is an attribute access on that union. Runtime was always correct; no type checker gates this path today, which is the only reason it went unnoticed. Narrowed one name at a time, constraints first, which is the order the comment above it promises.

Rubric proof

  • The deadlock test fails without the fixtest_a_foreign_blocks_long_description_does_not_refuse_the_day raises PlanViolation: EVT1: description is 1025 characters, over the 1024-character limit on the unfixed code. It uses a live handle, not a dead one, so the dead-link check passes the block through and this refusal is the only one that can fire.
  • The guard test is not vacuous. test_an_owned_blocks_long_description_still_refuses passed on its first run, so per CLAUDE.md it was broken on purpose: over-narrowing the filter to skip every block fails it plus three pre-existing refusal tests.
  • Suite: 3032 passed, 10 skipped, 1 xfailed across tests/unit tests/integration tests/e2e; tests/unit/tmbx + tests/memory 851 passed. Offline, no model calls.

For the human reviewer

  • The foreign-block exclusion is right for the length refusal specifically. It is not the same argument as the dead-link one: a dead handle has no url to write, whereas a long description is a real constraint that simply cannot be acted on for a block tmbx may not write. Both end in the same deadlock, but confirm the conclusion is wanted for this one.
  • Removing if private: is genuinely inert. It is dead by construction rather than by test, so it is worth a second pair of eyes on _private_properties returning a fixed eight-entry map.

Docs for this round are #410, landing on this branch per the CLAUDE.md rule that a round carries its docs ticket into the same PR.

🤖 Generated with Claude Code

hugocool and others added 2 commits September 9, 2026 11:24
…e above it (#399)

Three LOW follow-ups deferred from #398. The first is a bug; the other two
are a lie in a docstring and a union that outlived its narrowing.

**The foreign-block deadlock, reached by the other door.** #398 taught
`_dead_link_violation` to skip a block tmbx does not own, because refusing
over one deadlocks the day: the remedy the message names is a write to a
foreign event, and `_foreign_touches` refuses exactly that. The refusal
one screen below it, `_long_description_violation`, never learned the same
thing. So a foreign block carrying a live handle and an over-long
description refused every commit of that day forever, and both remedies it
offers -- shorten the description, or drop the link -- are foreign writes
that cannot happen. Same one-word fix, `foreign_handles`, already in scope
at the call site.

The two refusals sit one screen apart and share a deadlock, so the
docstring now says a filter added to either belongs on both.

**A guard that could not fail.** `_write_event_args` said the private map
was "included only when at least one of those eight is set" and had an
`if private:` implementing it. Both stopped being true when
`_private_properties` began writing `None` as an empty string instead of
dropping the key: it returns a fixed eight-entry map that is never falsy.
`test_an_event_with_no_identity_sends_every_key_empty` already pinned the
real behaviour, so this aligns the code and the prose with the test rather
than changing what goes over the wire.

**A union nothing was gating.** After `gather(..., return_exceptions=True)`
in `timeboxing_host.resolve`, narrowing by looping over the pair narrows
only the loop variable, leaving `work.facts` an attribute access on
`T | BaseException`. Runtime was always correct; nothing type-checks this
path today, which is the only reason it went unnoticed. Narrowed one name
at a time, constraints first, which is the order the comment above promises.

Tests: the deadlock test fails without the fix. The guard test was checked
by over-narrowing the filter on purpose -- it fails, along with three
pre-existing refusal tests, so it is not vacuous.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#410)

materials-and-work-links.md documented one of the two commit-time refusals
and, since #398, was wrong about the one it did cover.

Adds _long_description_violation: the description limit exists because a
linked event's displayed description has the material url composed into
it, so the authored text has to live separately in tmbx.desc, and that
property is length-capped by the provider. The check sits in the service,
before the write, because the adapter that owns the limit can only raise
from inside the commit's event loop, past the journal and after some
events are already written. It refuses rather than truncates, since a
silent truncation loses what a person wrote and only surfaces on the next
read.

Fixes the _dead_link_violation paragraph, which said the whole commit is
refused if any block's handle is not in the material store — no longer
true. Both refusals now skip a block tmbx doesn't own, because the remedy
either one names is a write to a foreign event, and _foreign_touches
refuses those outright. Checking a foreign block would deadlock every
commit of that day with no way out.

Records the pairing: the two refusals sit one screen apart in service.py
and share that deadlock. #398 fixed the exclusion on one; #399 fixed it on
the other, a release later, for the identical reason — so a filter added
to either belongs on both, and whoever adds a third commit-time refusal
should ask.

Also notes that _write_event_args no longer guards the private map behind
if private:. The "private map merges" section already documented the real
behaviour (all eight tmbx.* keys sent on every write); the code now
matches that prose instead of contradicting it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hugolytics

Copy link
Copy Markdown
Collaborator Author

Docs for #410 have landed on this branch — 0c933cd, docs/architecture/materials-and-work-links.md only. It documents _long_description_violation (which the doc never mentioned), corrects the _dead_link_violation paragraph that has been wrong since #398, and records the shared foreign-block deadlock so the next commit-time refusal gets the same question asked of it.

Two notes on branch state:

This branch is based on 8dd10d4, not current main (b2ffb21, #408). GitHub reports the PR MERGEABLE/CLEAN, so no rebase is needed to merge. I did rebase locally onto b2ffb21 and re-ran the full offline suite there — 3338 passed, 10 skipped, 1 xfailed across tests/unit tests/integration tests/e2e tests/memory — so it is verified green against main including #408, even though what is pushed here predates it.

I could not push that rebase. git push --force-with-lease and git reset --hard are both blocked by this session's own permission settings. Nothing is wrong with the branch; it simply cannot be force-updated from here. If you want the linear history, the rebase is git rebase origin/main and a force-push — otherwise merging as-is is fine and loses nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants