fix topic cycle detection past 32 nodes

[ben564885]

fixes #59.

ancestors_of counted popped nodes instead of levels, so depth < 32 capped total iterations rather than depth. the walk stopped after 32 nodes and returned an incomplete ancestor set, so reject_cycle reported clean and wrote the cycle to topics.yaml. nothing downstream re-checks it, so the corrupt graph was permanent.

dropping the counter is enough: the existing if parent in ancestors: continue guard already bounds the walk to the node set, so it terminates on graphs that are already cyclic. a real depth cap was the alternative, but it would still miss cycles past the bound, and completeness matters more than parity with the descendants query.

tests/test_architecture.py asserted the literal "depth < 32", which pinned the bug in place. that assertion now checks the termination guard instead.

two regression tests: a 34-topic chain (the smallest that outran the old cap) and a wide two-level graph, which used to pass or fail depending on PYTHONHASHSEED because the frontier is seeded from a set. both fail on main and pass here.

pytest 566 passed, ruff check . clean, git diff --check clean, cli smoke ok.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd19d43a99

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Update the Almanac depth-cap contract

With the traversal cap removed here, the shipped Almanac pages now describe the opposite contract: almanac/architecture/wiki/topics-dag.md:59, almanac/reference/topics-yaml.md:71, and almanac/guides/maintain-topics.md:95 still say ancestor/cycle traversal has a defensive depth cap of 32. That leaves the repo-local wiki misleading future agents about the mutation invariant and why the walk is safe, so please update those pages to say the walk is bounded by visited topics rather than by depth.

AGENTS.md reference: AGENTS.md:L8-L15

Useful? React with 👍 / 👎.

[Hotragn]

@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/.