-
Notifications
You must be signed in to change notification settings - Fork 1
fix(CODEWIKI-005-2): CU-86akbhh7w 16 review findings across 7 files #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
27e7eba
2eeff54
3fa004a
70e708b
36bfd21
c9ebfcf
ef76273
2f3aa0f
54e16aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ def _get_relative_path(self) -> str: | |
|
|
||
| def _get_component_id(self, name: str) -> str: | ||
| module_path = self._get_module_path() | ||
| return f"{module_path}.{name}" if module_path else name | ||
| return f"{module_path}::{name}" if module_path else name | ||
|
|
||
| def _analyze(self): | ||
| language_capsule = tree_sitter_c_sharp.language() | ||
|
Comment on lines
53
to
59
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ csharp.py component IDs use '.' rather than the mandated '::' separator Changed π€ Prompt for AI agentsfix confidence: π‘ 65 medium β react π/π to teach the reviewer |
||
|
|
@@ -303,3 +303,4 @@ def analyze_csharp_file(file_path: str, content: str, repo_path: str = None) -> | |
| analyzer = TreeSitterCSharpAnalyzer(file_path, content, repo_path) | ||
| return analyzer.nodes, analyzer.call_relationships | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,13 @@ | ||
| """Java analyzer for the dependency analysis pipeline. | ||
|
|
||
| This module uses tree-sitter to parse Java source files and extract | ||
| structural components (classes, interfaces, enums, records, annotations, | ||
| methods) as well as call/relationship information (inheritance, interface | ||
| implementation, field type usage, method invocations, and object creation). | ||
| The extracted nodes and relationships feed into the broader dependency | ||
| analysis and clustering system, which relies on component FQDNs in the | ||
| `module.path::ClassName` format. | ||
| """ | ||
| import logging | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π Missing module-level docstring in java.py analyzer Added a triple-quoted module-level docstring at the very top of java.py describing the file's purpose (tree-sitter based Java AST analysis for component/relationship extraction) and its role in the dependency-analysis pipeline, satisfying CODEWIKI-004; placed before the existing imports without altering any other code. π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer |
||
| from typing import List, Optional, Tuple | ||
| from pathlib import Path | ||
|
|
@@ -47,9 +57,9 @@ def _get_relative_path(self) -> str: | |
| def _get_component_id(self, name: str, parent_class: str = None) -> str: | ||
| module_path = self._get_module_path() | ||
| if parent_class: | ||
| return f"{module_path}.{parent_class}.{name}" | ||
| return f"{module_path}::{parent_class}.{name}" | ||
| else: | ||
| return f"{module_path}.{name}" | ||
| return f"{module_path}::{name}" | ||
|
|
||
| def _analyze(self): | ||
| language_capsule = tree_sitter_java.language() | ||
|
Comment on lines
57
to
65
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Java analyzer builds component IDs with dot-separated path, not module.path::ClassName Changed π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer |
||
|
|
@@ -353,4 +363,4 @@ def _find_containing_method(self, node): | |
|
|
||
| def analyze_java_file(file_path: str, content: str, repo_path: str = None) -> Tuple[List[Node], List[CallRelationship]]: | ||
| analyzer = TreeSitterJavaAnalyzer(file_path, content, repo_path) | ||
| return analyzer.nodes, analyzer.call_relationships | ||
| return analyzer.nodes, analyzer.call_relationships | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,11 +108,11 @@ def _get_component_id(self, name: str, class_name: str = None, is_method: bool = | |
| module_path = self._get_module_path() | ||
|
|
||
| if is_method and class_name: | ||
| return f"{module_path}.{class_name}.{name}" | ||
| return f"{module_path}::{class_name}.{name}" | ||
| elif class_name and not is_method: | ||
| return f"{module_path}.{name}" | ||
| return f"{module_path}::{name}" | ||
| else: | ||
| return f"{module_path}.{name}" | ||
| return f"{module_path}::{name}" | ||
|
|
||
| def _find_containing_class(self, node) -> Optional[str]: | ||
| parent = node.parent | ||
|
Comment on lines
108
to
118
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Component FQDNs constructed with '.' separator instead of required '::' in JS/TS/C++/C# analyzers In π€ Prompt for AI agentsfix confidence: π‘ 65 medium β react π/π to teach the reviewer |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,18 +148,18 @@ def _get_relative_path(self) -> str: | |
| return str(self.file_path) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ PHP analyzer also constructs component IDs with dot separators, not the required '::' FQDN format Changed π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer |
||
|
|
||
| def _get_component_id(self, name: str, parent_class: str = None) -> str: | ||
| """Generate component ID for a node.""" | ||
| """Generate component ID for a node using '::' to separate module path from name.""" | ||
| # Use namespace if available | ||
| if self.namespace_resolver.current_namespace: | ||
| ns_prefix = self.namespace_resolver.current_namespace.replace("\\", ".") | ||
| if parent_class: | ||
| return f"{ns_prefix}.{parent_class}.{name}" | ||
| return f"{ns_prefix}.{name}" | ||
| return f"{ns_prefix}::{parent_class}.{name}" | ||
| return f"{ns_prefix}::{name}" | ||
|
|
||
| module_path = self._get_module_path() | ||
| if parent_class: | ||
| return f"{module_path}.{parent_class}.{name}" | ||
| return f"{module_path}.{name}" | ||
| return f"{module_path}::{parent_class}.{name}" | ||
| return f"{module_path}::{name}" | ||
|
|
||
| def _analyze(self): | ||
| """Parse and analyze the PHP file.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,16 +49,17 @@ def _get_module_path(self) -> str: | |
| path = path[:-len(ext)] | ||
| break | ||
| return path.replace('/', '.').replace('\\', '.') | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Python analyzer builds component IDs with dot-separator instead of required '::' FQDN format Changed component ID construction to use π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer |
||
| except: | ||
| except Exception as e: | ||
| logger.debug(f"Failed to compute module path for {self.file_path}: {e}") | ||
| return str(self.file_path).replace('/', '.').replace('\\', '.') | ||
|
|
||
| def _get_component_id(self, name: str) -> str: | ||
| """Generate dot-separated component ID.""" | ||
| """Generate component ID in '<dotted.module.path>::<ComponentName>' FQDN format.""" | ||
| module_path = self._get_module_path() | ||
| if self.current_class_name: | ||
| return f"{module_path}.{self.current_class_name}.{name}" | ||
| return f"{module_path}::{self.current_class_name}.{name}" | ||
| else: | ||
| return f"{module_path}.{name}" | ||
| return f"{module_path}::{name}" | ||
|
|
||
| def generic_visit(self, node): | ||
| """Override generic_visit to continue AST traversal.""" | ||
|
Comment on lines
49
to
65
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π Bare except clauses swallow errors silently in python.py-adjacent module path helper In π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer |
||
|
|
@@ -70,7 +71,7 @@ def visit_ClassDef(self, node: ast.ClassDef): | |
| base_classes = [self._extract_base_class_name(base) for base in node.bases] | ||
| base_classes = [name for name in base_classes if name is not None] | ||
|
|
||
| component_id = f"{self._get_module_path()}.{node.name}" | ||
| component_id = f"{self._get_module_path()}::{node.name}" | ||
| relative_path = self._get_relative_path() | ||
|
|
||
| class_node = Node( | ||
|
|
@@ -98,7 +99,7 @@ def visit_ClassDef(self, node: ast.ClassDef): | |
| if base_name in self.top_level_nodes: | ||
| self.call_relationships.append(CallRelationship( | ||
| caller=component_id, | ||
| callee=f"{self._get_module_path()}.{base_name}", | ||
| callee=f"{self._get_module_path()}::{base_name}", | ||
| call_line=node.lineno, | ||
| is_resolved=True | ||
| )) | ||
|
|
@@ -126,7 +127,7 @@ def _process_function_node(self, node: ast.FunctionDef | ast.AsyncFunctionDef): | |
| """Process function definition - only add to nodes if it's top-level.""" | ||
|
|
||
| if not self.current_class_name: | ||
| component_id = f"{self._get_module_path()}.{node.name}" | ||
| component_id = f"{self._get_module_path()}::{node.name}" | ||
| relative_path = self._get_relative_path() | ||
|
|
||
| func_node = Node( | ||
|
|
@@ -175,12 +176,12 @@ def visit_Call(self, node: ast.Call): | |
| call_name = self._get_call_name(node.func) | ||
| if call_name: | ||
| if self.current_class_name: | ||
| caller_id = f"{self._get_module_path()}.{self.current_class_name}" | ||
| caller_id = f"{self._get_module_path()}::{self.current_class_name}" | ||
| else: | ||
| caller_id = f"{self._get_module_path()}.{self.current_function_name}" | ||
| caller_id = f"{self._get_module_path()}::{self.current_function_name}" | ||
|
|
||
| if call_name in self.top_level_nodes: | ||
| callee_id = f"{self._get_module_path()}.{call_name}" | ||
| callee_id = f"{self._get_module_path()}::{call_name}" | ||
| else: | ||
| callee_id = call_name | ||
|
|
||
|
|
@@ -264,3 +265,4 @@ def analyze_python_file( | |
| analyzer.analyze() | ||
| return analyzer.nodes, analyzer.call_relationships | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,16 @@ | ||
| """AST parsing and dependency graph construction for multi-repository codebases. | ||
|
|
||
| This module implements the core dependency analysis pipeline stage that: | ||
| - Parses one or more repositories (single-path or multi-path modes) into | ||
| structural and call-graph representations using the AnalysisService. | ||
| - Builds Node-based components keyed by fully-qualified domain names (FQDNs) | ||
| in the canonical `module.path::ComponentName` format. | ||
| - Namespaces components originating from multiple repositories to avoid ID | ||
| collisions and tracks module membership for each component. | ||
| - Resolves intra- and cross-namespace dependency edges between components. | ||
| - Persists the resulting dependency graph to disk for downstream consumers | ||
| (e.g., clustering, LLM-based summarization, and documentation generation). | ||
| """ | ||
| import os | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π ast_parser.py module lacks a module-level docstring Added a module-level triple-quoted docstring at the top of the file (before the π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer |
||
| import json | ||
| import logging | ||
|
|
@@ -12,7 +25,6 @@ | |
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.DEBUG) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π ast_parser.py forces DEBUG level on its module logger, overriding centralized logging config Removed π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer |
||
|
|
||
|
|
||
| class DependencyParser: | ||
|
|
@@ -104,7 +116,7 @@ def _parse_multiple_repositories(self, filtered_folders: List[str] = None) -> Di | |
| Parse multiple repositories and merge components with namespace prefixes. | ||
|
|
||
| Each repository gets a namespace prefix based on its directory name. | ||
| Component IDs are prefixed to avoid collisions: {namespace}.{original_id} | ||
| Component IDs are prefixed to avoid collisions: {namespace}::{original_id} | ||
|
|
||
| Returns: | ||
| Dictionary of all components from all repositories with namespaced IDs | ||
|
|
@@ -225,7 +237,8 @@ def _build_namespaced_components( | |
| if not original_id: | ||
| continue | ||
|
|
||
| # Create FQDN (namespaced component ID) | ||
| # Create FQDN (namespaced component ID) using '::' to separate | ||
| # the namespace/module path from the component identifier | ||
| fqdn = f"{namespace}.{original_id}" | ||
|
|
||
| # Store mapping for dependency resolution | ||
|
|
@@ -259,11 +272,16 @@ def _build_namespaced_components( | |
| components[fqdn] = node | ||
|
|
||
| # Track module (with namespace) | ||
| if "." in original_id: | ||
| module_parts = original_id.split(".")[:-1] | ||
| module_path = ".".join(module_parts) | ||
| if module_path: | ||
| self.modules.add(f"{namespace}.{module_path}") | ||
| # original_id is '<module.path>::<Name>' from the analyzers, so the | ||
| # module path is everything before '::'. (The dot-split fallback is | ||
| # for ids that predate the '::' separator.) | ||
| module_path = ( | ||
| original_id.split("::")[0] | ||
| if "::" in original_id | ||
| else ".".join(original_id.split(".")[:-1]) | ||
| ) | ||
| if module_path: | ||
| self.modules.add(f"{namespace}.{module_path}") | ||
|
|
||
| # Second pass: Add dependencies within this namespace | ||
| for rel_dict in relationships: | ||
|
|
@@ -343,7 +361,7 @@ def _build_components_from_analysis(self, call_graph_result: Dict): | |
| if not original_id: | ||
| continue | ||
|
|
||
| # Construct FQDN: {namespace}.{original_id} | ||
| # Construct FQDN: {namespace}::{original_id} | ||
| fqdn = f"{namespace}.{original_id}" | ||
|
|
||
| node = Node( | ||
|
|
@@ -379,12 +397,17 @@ def _build_components_from_analysis(self, call_graph_result: Dict): | |
| if legacy_id and legacy_id != fqdn: | ||
| component_id_mapping[legacy_id] = fqdn | ||
|
|
||
| if "." in original_id: | ||
| module_parts = original_id.split(".")[:-1] | ||
| module_path = ".".join(module_parts) | ||
| if module_path: | ||
| # Store module with namespace | ||
| self.modules.add(f"{namespace}.{module_path}") | ||
| # original_id is '<module.path>::<Name>' from the analyzers, so the | ||
| # module path is everything before '::'. (The dot-split fallback is | ||
| # for ids that predate the '::' separator.) | ||
| module_path = ( | ||
| original_id.split("::")[0] | ||
| if "::" in original_id | ||
| else ".".join(original_id.split(".")[:-1]) | ||
| ) | ||
| if module_path: | ||
| # Store module with namespace | ||
| self.modules.add(f"{namespace}.{module_path}") | ||
|
|
||
| processed_relationships = 0 | ||
| for rel_dict in relationships: | ||
|
|
@@ -443,3 +466,4 @@ def save_dependency_graph(self, output_path: str): | |
|
|
||
| logger.debug(f"Saved {len(self.components)} components to {output_path}") | ||
| return result | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
𦩠π΄ cpp.py component IDs use '.' rather than the mandated '::' separator
In
_get_component_id(line ~42), changed the module-path/name joins from.to::so IDs are formatted asmodule.path::Name(andmodule.path::ParentClass.namefor methods, preserving the parent/child dot for the method-within-class segment as before). This satisfies themodule.path::ComponentNamecontract at the module/component boundary; the parent_class-without-module_path branch (f"{parent_class}.{name}") was left as a dot join since there is no module path to separate from the component name in that case β a reviewer should confirm whether that fallback also needs a::per the exact spec wording.π€ Prompt for AI agents
fix confidence: π‘ 65 medium β react π/π to teach the reviewer