GeezerVison

@ben564885 I reproduced this against ancestors_of directly rather than through the CLI, and your diagnosis is exactly right — including the 33-vs-34 boundary you called out. A/B on the same script:

On main:

chain  33: ancestors= 32/32 OK         reject_cycle -> cycle rejected
chain  34: ancestors= 32/33 INCOMPLETE reject_cycle -> NOT CAUGHT
chain  60: ancestors= 32/59 INCOMPLETE reject_cycle -> NOT CAUGHT
chain 200: ancestors= 32/199 INCOMPLETE reject_cycle -> NOT CAUGHT

On this branch:

chain  33: ancestors= 32/32 OK         reject_cycle -> cycle rejected
chain  34: ancestors= 33/33 OK         reject_cycle -> cycle rejected
chain  60: ancestors= 59/59 OK         reject_cycle -> cycle rejected
chain 200: ancestors=199/199 OK        reject_cycle -> cycle rejected

The removal is safe, and here is the proof a reviewer will want. The obvious worry with deleting a loop bound is non-termination. It cannot happen here: if parent in ancestors: continue plus ancestors.add(parent) means every slug is expanded at most once, so the frontier is bounded by the number of definitions. Confirmed against a graph that already contains a cycle:

pre-existing cycle: ancestors_of(a) = ['a', 'b', 'c'] -> terminated

That terminates identically on main and on this branch — so the visited set was always what guaranteed termination, and depth < 32 was never load-bearing for it. It only ever truncated correct answers.

Why this survived, which is worth flagging for the maintainers: tests/test_architecture.py asserted

assert "depth < 32" in graph_text

so the test suite was actively pinning the defect in place. Anyone who noticed the cap and tried to remove it would have gone red and probably assumed the cap was intentional. Changing that assertion to "if parent in ancestors:" is the honest fix, and I mention it only because an architecture-test change can look like weakening a guard when here it is the opposite.

I suspect the 32 came from CLAUDE.md's "depth cap of 32 on any recursive CTE" non-negotiable. That rule is about the SQL recursive CTEs in the read model, where there is no visited set and a bound is genuinely needed. ancestors_of is an in-Python walk that already has one, so the cap was a misapplication of that rule rather than a second layer of it — the "belt-and-suspenders" here is the visited set. If anyone still wants an explicit bound as defence in depth, len(definitions) would be the correct one; 32 is a magic number unrelated to anything in the graph.

Two small notes, neither blocking: frontier.pop() makes this a DFS, which is fine since you need the full ancestor set rather than shortest paths. And the docs touch-ups in almanac/ are right to be in the same PR, since the old wording described the capped behaviour.

I'd merge this as-is. Verified on Windows 11 / Python 3.13; uv run ruff check . clean and the topics tests pass. I have some unrelated PRs open (#64/#65/#66/#68/#69) and none of them touch services/topics/.