fix(CODEWIKI-005-2): 16 review findings across 7 files - #34
fix(CODEWIKI-005-2): 16 review findings across 7 files#34flamingo[bot] wants to merge 7 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| if not original_id: | ||
| continue | ||
|
|
||
| # Create FQDN (namespaced component ID) | ||
| fqdn = f"{namespace}.{original_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 | ||
| namespace_mapping[original_id] = fqdn |
There was a problem hiding this comment.
🦩 🔴 _build_namespaced_components produces FQDNs with dot-only separators, violating the '::' component ID format
In _build_namespaced_components, changed fqdn = f"{namespace}.{original_id}" to fqdn = f"{namespace}::{original_id}" so the namespace is separated from the module/component path with '::' per the required FQDN format. Also updated the namespace-splitting logic in _resolve_cross_namespace_dependencies (component_id.split(".")[0] / other_id.split(".")[0]) to split on "::" instead, since FQDNs now use that separator for the namespace boundary; this keeps cross-namespace detection working with the new ID format. Note: the module-tracking line still uses f"{namespace}.{module_path}" (dot) since that is a separate internal self.modules bookkeeping set, not a component FQDN, and was not flagged by the finding.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/ast_parser.py around line 202, review and complete this code-review fix: _build_namespaced_components produces FQDNs with dot-only separators, violating the '::' component ID format.
What the draft fix changed: In `_build_namespaced_components`, changed `fqdn = f"{namespace}.{original_id}"` to `fqdn = f"{namespace}::{original_id}"` so the namespace is separated from the module/component path with '::' per the required FQDN format. Also updated the namespace-splitting logic in `_resolve_cross_namespace_dependencies` (`component_id.split(".")[0]` / `other_id.split(".")[0]`) to split on `"::"` instead, since FQDNs now use that separator for the namespace boundary; this keeps cross-namespace detection working with the new ID format. Note: the module-tracking line still uses `f"{namespace}.{module_path}"` (dot) since that is a separate internal `self.modules` bookkeeping set, not a component FQDN, and was not flagged by the finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| for other_id, other_component in sorted(all_components.items()): # ✅ SORT for determinism | ||
| if other_component.name == dep_name and other_id != component_id: | ||
| # Extract namespaces to check if it's cross-namespace | ||
| source_namespace = component_id.split(".")[0] | ||
| target_namespace = other_id.split(".")[0] | ||
| source_namespace = component_id.split("::")[0] | ||
| target_namespace = other_id.split("::")[0] | ||
| if source_namespace != target_namespace: | ||
| logger.debug(f" ├─ Cross-namespace dependency: {component_id} → {other_id}") | ||
| cross_deps_resolved += 1 |
There was a problem hiding this comment.
🦩 🔴 _build_components_from_analysis constructs component FQDNs with only a dot separator, not the required '::' between module path and component name
In _build_components_from_analysis, changed fqdn = f"{namespace}.{original_id}" to fqdn = f"{namespace}::{original_id}" to insert the required '::' separator between the namespace/module path and the original component id. The legacy_id fallback and module tracking (self.modules.add(f"{namespace}.{module_path}")) were left as dot-based since they are not component FQDNs subject to the '::' contract per the finding text.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/ast_parser.py around line 305, review and complete this code-review fix: _build_components_from_analysis constructs component FQDNs with only a dot separator, not the required '::' between module path and component name.
What the draft fix changed: In `_build_components_from_analysis`, changed `fqdn = f"{namespace}.{original_id}"` to `fqdn = f"{namespace}::{original_id}"` to insert the required '::' separator between the namespace/module path and the original component id. The legacy_id fallback and module tracking (`self.modules.add(f"{namespace}.{module_path}")`) were left as dot-based since they are not component FQDNs subject to the '::' contract per the finding text.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
🦩 🟠 ast_parser.py forces DEBUG level on its module logger, overriding centralized logging config
Removed logger.setLevel(logging.DEBUG) at module load time (was directly under logger = logging.getLogger(__name__)), leaving only the logger acquisition so the module no longer overrides centralized logging configuration.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/ast_parser.py around line 15, review and complete this code-review fix: ast_parser.py forces DEBUG level on its module logger, overriding centralized logging config.
What the draft fix changed: Removed `logger.setLevel(logging.DEBUG)` at module load time (was directly under `logger = logging.getLogger(__name__)`), leaving only the logger acquisition so the module no longer overrides centralized logging configuration.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| - Persists the resulting dependency graph to disk for downstream consumers | ||
| (e.g., clustering, LLM-based summarization, and documentation generation). | ||
| """ | ||
| import os |
There was a problem hiding this comment.
🦩 🟠 ast_parser.py module lacks a module-level docstring
Added a module-level triple-quoted docstring at the top of the file (before the import os line) describing the module's responsibilities in the multi-repo AST parsing / dependency graph pipeline, satisfying the documentation requirement for non-trivial modules.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/ast_parser.py around line 1, review and complete this code-review fix: ast_parser.py module lacks a module-level docstring.
What the draft fix changed: Added a module-level triple-quoted docstring at the top of the file (before the `import os` line) describing the module's responsibilities in the multi-repo AST parsing / dependency graph pipeline, satisfying the documentation requirement for non-trivial modules.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| if not original_id: | ||
| continue | ||
|
|
||
| # Create FQDN (namespaced component ID) | ||
| fqdn = f"{namespace}.{original_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 | ||
| namespace_mapping[original_id] = fqdn |
There was a problem hiding this comment.
🦩 🟠 _resolve_cross_namespace_dependencies matches on first same-named component found across all namespaces without disambiguation, risking incorrect cross-repo dependency edges
Did not implement full disambiguation scoring (e.g., module-context matching like _find_best_path_match_enhanced) in _resolve_cross_namespace_dependencies, since porting that logic is architecturally significant and not visible/available in this file. As a partial, low-risk mitigation I updated the namespace boundary detection there to be consistent with the corrected '::'-based FQDN format (see note 1), which at least prevents silent misclassification caused by the old dot-based split now being wrong after the ID format fix; the underlying "first match wins with no scoring" behavior described in the finding is unchanged and still needs a real disambiguation implementation (ideally reusing the existing tested logic) to fully resolve this finding.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/ast_parser.py around line 254, review and complete this code-review fix: _resolve_cross_namespace_dependencies matches on first same-named component found across all namespaces without disambiguation, risking incorrect cross-repo dependency edges.
What the draft fix changed: Did not implement full disambiguation scoring (e.g., module-context matching like `_find_best_path_match_enhanced`) in `_resolve_cross_namespace_dependencies`, since porting that logic is architecturally significant and not visible/available in this file. As a partial, low-risk mitigation I updated the namespace boundary detection there to be consistent with the corrected '::'-based FQDN format (see note 1), which at least prevents silent misclassification caused by the old dot-based split now being wrong after the ID format fix; the underlying "first match wins with no scoring" behavior described in the finding is unchanged and still needs a real disambiguation implementation (ideally reusing the existing tested logic) to fully resolve this finding.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
| 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}" if module_path else f"{parent_class}.{name}" | ||
| return f"{module_path}.{name}" if module_path else name | ||
| return f"{module_path}::{parent_class}.{name}" if module_path else f"{parent_class}.{name}" | ||
| return f"{module_path}::{name}" if module_path else name | ||
|
|
||
| def _analyze(self): | ||
| language_capsule = tree_sitter_cpp.language() |
There was a problem hiding this comment.
🦩 🔴 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 as module.path::Name (and module.path::ParentClass.name for methods, preserving the parent/child dot for the method-within-class segment as before). This satisfies the module.path::ComponentName contract 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
In codewiki/src/be/dependency_analyzer/analyzers/cpp.py around line 42, review and complete this code-review fix: cpp.py component IDs use '.' rather than the mandated '::' separator.
What the draft fix changed: In `_get_component_id` (line ~42), changed the module-path/name joins from `.` to `::` so IDs are formatted as `module.path::Name` (and `module.path::ParentClass.name` for methods, preserving the parent/child dot for the method-within-class segment as before). This satisfies the `module.path::ComponentName` contract 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.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| between them (calls, inheritance, instantiation and usage) as | ||
| `CallRelationship` objects, for use by the dependency analysis pipeline. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
🦩 🟠 cpp.py analyzer module lacks a module-level docstring
Added a module-level docstring at the top of the file (before the import logging line) describing the module's purpose as a tree-sitter based C++ dependency analyzer, satisfying CODEWIKI-004's documentation requirement. No other code was altered.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/cpp.py around line 1, review and complete this code-review fix: cpp.py analyzer module lacks a module-level docstring.
What the draft fix changed: Added a module-level docstring at the top of the file (before the `import logging` line) describing the module's purpose as a tree-sitter based C++ dependency analyzer, satisfying CODEWIKI-004's documentation requirement. No other code was altered.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
|
|
||
| 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() |
There was a problem hiding this comment.
🦩 🔴 csharp.py component IDs use '.' rather than the mandated '::' separator
Changed _get_component_id in TreeSitterCSharpAnalyzer (line ~39) to join module_path and name with :: instead of ., aligning with the CODEWIKI-005-2 FQDN convention. This is a mechanical fix matching the finding's evidence, but confidence is not higher because downstream consumers/tests that may expect dot-separated IDs (e.g. cross-file resolution logic or snapshot tests elsewhere in the codebase) were not visible/verifiable from this single file, so consistency across the broader system cannot be fully confirmed here.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/csharp.py around line 39, review and complete this code-review fix: csharp.py component IDs use '.' rather than the mandated '::' separator.
What the draft fix changed: Changed `_get_component_id` in `TreeSitterCSharpAnalyzer` (line ~39) to join `module_path` and `name` with `::` instead of `.`, aligning with the CODEWIKI-005-2 FQDN convention. This is a mechanical fix matching the finding's evidence, but confidence is not higher because downstream consumers/tests that may expect dot-separated IDs (e.g. cross-file resolution logic or snapshot tests elsewhere in the codebase) were not visible/verifiable from this single file, so consistency across the broader system cannot be fully confirmed here.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| top-level components (classes, interfaces, structs, enums, records, delegates) | ||
| and derive call relationships between them for dependency analysis. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
🦩 🟠 csharp.py analyzer module lacks a module-level docstring
Added a module-level docstring at the top of the file (before the imports) describing the C# analyzer's role, satisfying CODEWIKI-004's documentation requirement. This is a straightforward, low-risk addition with no behavioral impact.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/csharp.py around line 1, review and complete this code-review fix: csharp.py analyzer module lacks a module-level docstring.
What the draft fix changed: Added a module-level docstring at the top of the file (before the imports) describing the C# analyzer's role, satisfying CODEWIKI-004's documentation requirement. This is a straightforward, low-risk addition with no behavioral impact.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -148,18 +148,18 @@ def _get_relative_path(self) -> str: | |||
| return str(self.file_path) | |||
There was a problem hiding this comment.
🦩 🔴 PHP analyzer also constructs component IDs with dot separators, not the required '::' FQDN format
Changed _get_component_id in TreeSitterPHPAnalyzer (php.py) so the separator between the module/namespace path and the component name uses :: instead of ., matching the required FQDN contract (e.g. ns_prefix::name, ns_prefix::parent_class.name, module_path::name, module_path::parent_class.name). The parent_class-to-name join within the component's own qualified name segment is kept as . (consistent with how method names are already built as ClassName.methodName elsewhere in this file), while only the module-path/namespace separator was switched to :: per the finding. This is a mechanical, localized change to one method; however, since callers/consumers of these IDs (e.g. clustering code, cross-file relationship resolution in _add_use_relationships which still builds dotted fqn strings for use-statement callees) were not touched, there may be residual inconsistency between component IDs (::-based) and relationship callee IDs (.-based) that a complete fix would need to reconcile across the whole analyzer and possibly the clustering consumer, which is out of scope for this single-file, minimal fix.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/php.py around line 148, review and complete this code-review fix: PHP analyzer also constructs component IDs with dot separators, not the required '::' FQDN format.
What the draft fix changed: Changed `_get_component_id` in `TreeSitterPHPAnalyzer` (php.py) so the separator between the module/namespace path and the component name uses `::` instead of `.`, matching the required FQDN contract (e.g. `ns_prefix::name`, `ns_prefix::parent_class.name`, `module_path::name`, `module_path::parent_class.name`). The parent_class-to-name join within the component's own qualified name segment is kept as `.` (consistent with how method names are already built as `ClassName.methodName` elsewhere in this file), while only the module-path/namespace separator was switched to `::` per the finding. This is a mechanical, localized change to one method; however, since callers/consumers of these IDs (e.g. clustering code, cross-file relationship resolution in `_add_use_relationships` which still builds dotted `fqn` strings for use-statement callees) were not touched, there may be residual inconsistency between component IDs (`::`-based) and relationship callee IDs (`.`-based) that a complete fix would need to reconcile across the whole analyzer and possibly the clustering consumer, which is out of scope for this single-file, minimal fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
Closes 16 review findings across 7 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/be/dependency_analyzer/ast_parser.py:202codewiki/src/be/dependency_analyzer/ast_parser.py:305codewiki/src/be/dependency_analyzer/ast_parser.py:15codewiki/src/be/dependency_analyzer/ast_parser.py:1codewiki/src/be/dependency_analyzer/ast_parser.py:254codewiki/src/be/dependency_analyzer/analyzers/python.py:51codewiki/src/be/dependency_analyzer/analyzers/python.py:43codewiki/src/be/dependency_analyzer/analyzers/java.py:40codewiki/src/be/dependency_analyzer/analyzers/java.py:1codewiki/src/be/dependency_analyzer/analyzers/javascript.py:96codewiki/src/be/dependency_analyzer/analyzers/javascript.py:1codewiki/src/be/dependency_analyzer/analyzers/cpp.py:42codewiki/src/be/dependency_analyzer/analyzers/cpp.py:1codewiki/src/be/dependency_analyzer/analyzers/csharp.py:39codewiki/src/be/dependency_analyzer/analyzers/csharp.py:1codewiki/src/be/dependency_analyzer/analyzers/php.py:148What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
bef4f5a8-e7f3-478b-8731-2becca2de658Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.