From 9718f859bc064a9ded919005041b1e6fa0111192 Mon Sep 17 00:00:00 2001 From: ben564885 Date: Fri, 31 Jul 2026 00:25:54 -0700 Subject: [PATCH 1/2] fix(topics): detect cycles beyond 32 topic nodes --- src/codealmanac/services/topics/graph.py | 4 +- tests/test_architecture.py | 2 +- tests/test_topics_mutation.py | 61 ++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/codealmanac/services/topics/graph.py b/src/codealmanac/services/topics/graph.py index 274c1d19..d1e0d4b0 100644 --- a/src/codealmanac/services/topics/graph.py +++ b/src/codealmanac/services/topics/graph.py @@ -41,9 +41,7 @@ def ancestors_of(definitions: tuple[TopicDefinition, ...], slug: str) -> set[str } ancestors: set[str] = set() frontier = list(parents_by_child.get(slug, set())) - depth = 0 - while frontier and depth < 32: - depth += 1 + while frontier: parent = frontier.pop() if parent in ancestors: continue diff --git a/tests/test_architecture.py b/tests/test_architecture.py index f1bf4574..210001ae 100644 --- a/tests/test_architecture.py +++ b/tests/test_architecture.py @@ -391,7 +391,7 @@ def test_topics_service_keeps_graph_and_repository_boundaries(): ] == [] assert "def reject_cycle(" in graph_text assert "def ancestors_of(" in graph_text - assert "depth < 32" in graph_text + assert "if parent in ancestors:" in graph_text assert "codealmanac.services.index" not in graph_text assert "codealmanac.services.repositories" not in graph_text assert "def existing_topic_slugs(" in read_model_text diff --git a/tests/test_topics_mutation.py b/tests/test_topics_mutation.py index 521f79ca..956a33e2 100644 --- a/tests/test_topics_mutation.py +++ b/tests/test_topics_mutation.py @@ -373,6 +373,67 @@ def test_rename_malformed_page_frontmatter_fails_before_topics_yaml_write( assert topics_path.read_text(encoding="utf-8") == before +def test_link_rejects_cycle_in_chain_longer_than_traversal_bound( + tmp_path: Path, + isolated_home: Path, +): + # A 34-topic chain is the smallest that outran the old 32-iteration cap in + # ancestors_of, so the guard reported clean and wrote the cycle to disk. + repo = make_repo(tmp_path) + topics_path = repo / "almanac/topics.yaml" + entries = "".join( + f" - slug: n{index}\n" + f" title: N{index}\n" + f" parents: [n{index + 1}]\n" + for index in range(33) + ) + topics_path.write_text( + f"topics:\n{entries} - slug: n33\n title: N33\n parents: []\n", + encoding="utf-8", + ) + before = topics_path.read_text(encoding="utf-8") + app = create_app( + AppConfig(database_path=isolated_home / ".codealmanac/codealmanac.db") + ) + + with pytest.raises(ConflictError): + app.topics.link(LinkTopicRequest(cwd=repo, child="n33", parent="n0")) + + assert topics_path.read_text(encoding="utf-8") == before + + +def test_link_rejects_cycle_in_wide_shallow_graph( + tmp_path: Path, + isolated_home: Path, +): + # Only two levels deep, but wide enough that the old cap stopped early. + # ancestors_of seeds its frontier from a set, so this used to pass or fail + # depending on PYTHONHASHSEED. + repo = make_repo(tmp_path) + topics_path = repo / "almanac/topics.yaml" + parents = ", ".join(f"b{index}" for index in range(40)) + entries = "".join( + f" - slug: b{index}\n" + f" title: B{index}\n" + f" parents: [{'deep' if index == 0 else ''}]\n" + for index in range(40) + ) + topics_path.write_text( + f"topics:\n - slug: a\n title: A\n parents: [{parents}]\n" + f"{entries} - slug: deep\n title: Deep\n parents: []\n", + encoding="utf-8", + ) + before = topics_path.read_text(encoding="utf-8") + app = create_app( + AppConfig(database_path=isolated_home / ".codealmanac/codealmanac.db") + ) + + with pytest.raises(ConflictError): + app.topics.link(LinkTopicRequest(cwd=repo, child="deep", parent="a")) + + assert topics_path.read_text(encoding="utf-8") == before + + def make_repo(tmp_path: Path) -> Path: repo = tmp_path / "repo" (repo / "almanac").mkdir(parents=True) From 49f48d54bdaa814f113ffa2c732720b26506fe8d Mon Sep 17 00:00:00 2001 From: Ben Nisevich <262652256+bennisev-dev@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:43:03 -0700 Subject: [PATCH 2/2] almanac: drop the removed ancestor depth cap from the topics pages 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. --- almanac/architecture/wiki/topics-dag.md | 4 +++- almanac/guides/maintain-topics.md | 2 +- almanac/reference/topics-yaml.md | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/almanac/architecture/wiki/topics-dag.md b/almanac/architecture/wiki/topics-dag.md index ca9994cc..78a6f4ae 100644 --- a/almanac/architecture/wiki/topics-dag.md +++ b/almanac/architecture/wiki/topics-dag.md @@ -56,7 +56,9 @@ The tests lock down that behavior. Creating a topic preserves existing comments, The graph layer prevents self-parent links and missing-parent links before mutation writes [@topic-graph]. When an edge is added, `reject_cycle` checks whether the child is already an ancestor of the proposed parent. If it is, the operation raises a conflict instead of writing the edge [@topic-graph]. -Ancestor traversal has a depth cap of 32 [@topic-graph]. The cap is defensive: valid topic graphs should be much shallower, and the mutation path should not risk unbounded traversal if the file is already strange. +Ancestor traversal is bounded by the visited topic set, not by depth [@topic-graph]. `ancestors_of` skips any parent already in the result, so the walk visits each topic at most once and terminates even when `topics.yaml` already contains a cycle. That termination guard is the invariant to preserve: it keeps the walk safe without truncating it, so `reject_cycle` sees the complete ancestor set at any depth. + +There was previously a `depth < 32` counter here. It counted popped nodes rather than levels, so graphs wider or longer than 32 topics returned a partial ancestor set and `reject_cycle` wrote the cycle it was meant to block. Do not reintroduce a traversal cap; the visited set already bounds the walk, and any cap reopens that hole. ## Architectural Boundary diff --git a/almanac/guides/maintain-topics.md b/almanac/guides/maintain-topics.md index 32b920e2..2e15ee3e 100644 --- a/almanac/guides/maintain-topics.md +++ b/almanac/guides/maintain-topics.md @@ -92,7 +92,7 @@ Dispatch turns those CLI calls into typed topic requests before calling the serv ## Keep The DAG Valid -Before adding parent edges, the graph layer validates that parents exist, rejects self-parent links, and rejects cycles [@topic-graph]. Cycle detection walks ancestors with a depth cap of 32, which protects the command path if the graph is already unusual [@topic-graph]. +Before adding parent edges, the graph layer validates that parents exist, rejects self-parent links, and rejects cycles [@topic-graph]. Cycle detection walks the full ancestor set with no depth cap; the walk is bounded by the topics it has already visited, so it terminates on an unusual graph without missing ancestors [@topic-graph]. If a command refuses a parent or reports that a link would create a cycle, fix the topic design rather than editing around the command. The command is protecting the browse graph. diff --git a/almanac/reference/topics-yaml.md b/almanac/reference/topics-yaml.md index 5a1d1f5b..107c57ce 100644 --- a/almanac/reference/topics-yaml.md +++ b/almanac/reference/topics-yaml.md @@ -68,7 +68,7 @@ If the file is missing or empty, the loader treats it as an empty mapping and cr Slugs are kebab-case. The shared slug helper lowercases text, replaces runs of non-alphanumeric characters with hyphens, and strips leading or trailing hyphens [@slug]. A topic named `Auth Flow` therefore becomes `auth-flow`. -Parents form a directed acyclic graph. A topic cannot be its own parent, requested parents must already exist for create/link operations, and adding an edge that would create a cycle raises a conflict [@topic-graph]. Ancestor traversal has a defensive depth cap of 32 [@topic-graph]. +Parents form a directed acyclic graph. A topic cannot be its own parent, requested parents must already exist for create/link operations, and adding an edge that would create a cycle raises a conflict [@topic-graph]. Ancestor traversal has no depth cap: it is bounded by the set of topics already visited, so it covers the whole ancestor set at any depth and still terminates on an already-cyclic file [@topic-graph]. ## Mutation Behavior