Validate component IDs in save_module_tree to catch orphaned components - #83
Validate component IDs in save_module_tree to catch orphaned components#83LiberiFatali wants to merge 2 commits into
Conversation
surface unmatched (orphaned) tree ids and unassigned (leftover) index components in the save_module_tree response and a workspace validation file, so silent documentation gaps are caught before docs are generated.
|
@anhnh2002 could you take a look? |
There was a problem hiding this comment.
Thanks for tackling #82
What's good
- Validation never blocks the save — tree files, session cache, and processing order behave exactly as before;
validationis a pure addition to the response, so existing consumers are unaffected. - Correct, minimal core logic: deterministic
sorted()output,or []guarding missing/Nonecomponents, nestedchildrenhandled. - Solid test coverage (happy path, orphan, leftover, nested, combined, missing session) and docs kept in sync (
server.pytool description,IDE_DRIVEN_GUIDE.mdtable,.gitignoreconvention).
Requesting changes on two points (see inline comments):
leftover_component_idsis computed against the full component index, but clustering intentionally excludes non-essential components (the cluster prompt says so explicitly, and the original pipeline clusters onlyleaf_nodes). As written, the warning fires on every normal run and will likely push agents to over-assign components to silence it. Compare againstsession.leaf_nodesand downgrade leftovers fromwarningto informational. Theunmatched_idscheck is correct as-is.- The unbounded ID lists are embedded in the response twice (plus the log). Cap what goes over stdio and keep the full lists in
module_tree_validation.json.
Minor: module_tree_validation.json could be added to the workspace-layout listings in the workspace.py docstring and IDE_DRIVEN_GUIDE.md; the and children in _collect_component_ids is redundant since walking an empty dict is a no-op.
|
|
||
| module_tree = arguments["module_tree"] | ||
| output_dir = session.output_dir | ||
| known_ids = set(session.components.keys()) |
There was a problem hiding this comment.
known_ids = set(session.components.keys()) is the full component index, but clustering by design operates on a subset and intentionally excludes components. The cluster prompt (prompt_template.py:132-137) explicitly says "It's normal that some components are not essential to the repository" and "DO NOT include components that are not essential." The original pipeline also clusters only leaf_nodes (format_potential_core_components(leaf_nodes, ...) in cluster_modules.py).
So on any real repo, every non-leaf and every deliberately-excluded component lands in leftover_component_ids, and the "will receive no documentation" warning fires on every normal save. Since this response is consumed by an LLM agent, a persistent warning is likely to push agents to cram all components into modules just to silence it — degrading clustering quality, the opposite of this PR's intent.
Suggestion:
- Keep the
unmatched_idscheck against the full index as-is — that's the real bug-catcher and is correct. - Compute leftovers against
set(session.leaf_nodes)(the actual clustering candidate set) instead, and present it as informational (e.g. anotefield) rather than folding it intowarning.
Note the tests don't catch this because _make_session sets leaf_nodes identical to components — worth adding a case where components ⊃ leaf_nodes once the baseline is fixed.
| if unmatched_ids or leftover_ids: | ||
| warnings: List[str] = [] | ||
| if unmatched_ids: | ||
| warnings.append( | ||
| f"{len(unmatched_ids)} component id(s) in the module tree do not " | ||
| f"exist in the analysis index and will be omitted from docs: " | ||
| f"{unmatched_ids}" | ||
| ) | ||
| if leftover_ids: | ||
| warnings.append( | ||
| f"{len(leftover_ids)} indexed component(s) are assigned to no " | ||
| f"module and will receive no documentation: {leftover_ids}" | ||
| ) | ||
| warning = " ".join(warnings) | ||
| logger.warning( | ||
| "save_module_tree for session %s: %s", | ||
| session_id, | ||
| warning, | ||
| ) | ||
| else: | ||
| warning = "" |
There was a problem hiding this comment.
The full ID lists are transmitted three times per call: in the validation field, again inside the warning string, and again in the log line. Combined with the leftover-baseline issue above, a repo with a few thousand components could push 100KB+ through the MCP stdio channel on every save_module_tree — exactly the bloat the workspace design exists to avoid (see the workspace.py docstring: "Instead of transmitting bulky data through the MCP stdio channel...").
Suggestion: keep the counts in the response, truncate the embedded lists (e.g. first 20 + "see module_tree_validation.json for the full list"), and let the workspace JSON file carry the complete lists.
…ponse - Compute leftover_component_ids against the clustering candidate set (session.leaf_nodes) instead of the full component index, since the cluster prompt deliberately excludes non-essential components; surface leftovers as an informational 'note' rather than a 'warning'. - Keep the unmatched_ids check against the full index as the real warning. - Cap embedded ID lists in the response at 20 with *_truncated flags and keep full lists in module_tree_validation.json to avoid stdio bloat. - Simplify _collect_component_ids (empty-dict walk is a no-op). - Add module_tree_validation.json to workspace layout listings. - Tests: separate leaf_nodes from components, add non-leaf not-flagged, leftover-vs-candidates, list truncation, and no-warning-on-note cases.
|
Thanks for the careful review — both points are valid and fixed in 1. Leftover baseline now uses
2. Response ID lists are capped
Minors: Tests: 9/9 passing in |
2 participants
Address issue #82
Surface unmatched (orphaned) tree ids and unassigned (leftover) index components in the
save_module_treeresponse and a workspace validation file, so silent documentation gaps are caught before docs are generated.