fix topic cycle detection past 32 nodes - #60
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| depth = 0 | ||
| while frontier and depth < 32: | ||
| depth += 1 | ||
| while frontier: |
There was a problem hiding this comment.
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 👍 / 👎.
The parent commit removed the `depth < 32` counter from `ancestors_of`, but topics-dag.md, topics-yaml.md, and maintain-topics.md still described a defensive depth cap of 32 as the reason the ancestor walk is safe. That left the wiki stating the opposite of the shipped mutation invariant. All three now say the walk is bounded by the visited topic set rather than by depth, which is what makes it terminate on an already-cyclic topics.yaml without truncating the ancestor set. topics-dag.md also records why the old counter was wrong (it counted popped nodes, not levels) so the cap does not get reintroduced.
26412d4 to
49f48d5
Compare
2 participants
fixes #59.
ancestors_ofcounted popped nodes instead of levels, sodepth < 32capped total iterations rather than depth. the walk stopped after 32 nodes and returned an incomplete ancestor set, soreject_cyclereported clean and wrote the cycle totopics.yaml. nothing downstream re-checks it, so the corrupt graph was permanent.dropping the counter is enough: the existing
if parent in ancestors: continueguard 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.pyasserted 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
PYTHONHASHSEEDbecause the frontier is seeded from a set. both fail onmainand pass here.pytest566 passed,ruff check .clean,git diff --checkclean, cli smoke ok.