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 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)