fix(CODEWIKI-005-2): CU-86akbhh7w 16 review findings across 7 files - #34
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
flamingo
Bot
changed the title
|
Blocking: this produces double- The PR changes
Every component collapses to the same hint — the repository name. That is a silent documentation-quality regression, not a crash, so it will not show up as a failed pipeline run. Second effect, if "." in original_id:
module_path = ".".join(original_id.split(".")[:-1])
self.modules.add(f"{namespace}.{module_path}")
One level of module granularity disappears. Note this line still uses Third: this changes the key format of the persisted dependency graph, so every cached graph is invalidated. If |
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts were competing module docstrings in cpp.py, csharp.py and javascript.py, added by both this branch and #55. Resolved in favour of the wording already on main; this branch's _get_component_id changes are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…or change - generate_sub_module_documentations.py: the branch's normalize_component_ids_by_lookup(specs, components, id_to_fqdn) call was a three-argument call against a two-argument function, unpacked as a 3-tuple from a dict return. #47 has since landed the correct dedupe via normalize_component_id_list, so this resolves in favour of main. - analyzers/c.py: reverted the '.' -> '::' component-id change. Changing the separator for C alone is inconsistent with the other seven analyzers, and the FQDN format question belongs with #34/#45 where it can be made coherent end-to-end. The module docstring is kept. What remains is worth having: - deps.py: dict[str, any] -> dict[str, Any]. 'any' is the builtin function, not a type, so the old annotation was meaningless. - agent_orchestrator.__init__: drops a local logger that shadowed the module-level one (same logging.getLogger(__name__) object, so no behaviour change). - typescript.py _get_parent_context: returns 'unknown' instead of falling off the end as None, honouring its '-> str' annotation. Its one caller stores the value and nothing reads it, so this cannot regress. - The tool description now asks for integer IDs, matching what format_potential_core_components actually puts in the prompt. - Module docstrings throughout, plus from_web_job added to the config.py summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Switching the analyzers from '.' to '::' is right - it gives the module path and
the component name a real boundary - but the branch changed only half the
pipeline. ast_parser also joined the namespace with '::', so every component came
out with TWO separators:
openframe-oss-tenant::src.services.auth::AuthService
Everything downstream that reads an FQDN splits on the first '::' and so saw the
repository name and nothing else. Measured against the real hint functions, which
build the component descriptions in the clustering prompt, every component in the
repository collapsed to the same module hint.
Fixed by making one separator authoritative - '<namespace>.<module.path>::<Name>',
exactly one '::':
- ast_parser: namespace joins revert to '.', at both construction sites.
- ast_parser: namespace extraction in _resolve_cross_namespace_dependencies
reverts to split('.')[0]; splitting on '::' returned the whole module path,
so cross-namespace edges compared paths instead of repositories.
- ast_parser: module registration takes original_id.split('::')[0] rather than
dropping the last dot-segment. The old block silently lost one level of module
depth ('...src.services' where main gives '...src.services.auth').
- analyzers/c.py and analyzers/typescript.py were the two the branch missed;
all eight now agree.
Verified: FQDNs carry exactly one '::', the namespace is still recoverable,
cross-namespace comparison still distinguishes repositories, module granularity
matches main, and the prompt hints improve from '(openframe-oss-tenant, service)'
- identical for every component - to '(openframe-oss-tenant.src.services.auth,
service)'.
Note for operators: this changes the key format of the persisted dependency
graph, so any graph cached from a previous run is stale and should be discarded.
CI runs build it fresh.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#34 landed the coherent '::' format: '<namespace>.<module.path>::<Name>', with the namespace still joined by '.' so each FQDN carries exactly one separator. This branch's verify_fqdn_implementation.py asserted the opposite - that ast_parser builds f"{namespace}::{original_id}" - which is precisely the double-separator bug #34 fixed, so the script failed against its own branch. Restored to expect the '.' join; the script now passes. python.py's separator change is already on main via #34, so the conflicting hunk (competing wording on one debug log line) resolves in favour of main. What remains is this branch's real contribution: extract_module_hint and extract_package_hint now parse only the module portion before '::' and warn on an FQDN that lacks one, instead of scanning the whole string. On well-formed FQDNs the output is identical to main's; a malformed id degrades to unknown/core with a warning rather than silently yielding a wrong hint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
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.
ClickUp task: CU-86akbhh7w Code review fixes: CodeWiki backend and CLI review findings (2 PRs)