fix(DUP-001): CU-86akn96pk 10 review findings across 7 files - #113
flamingo[bot] wants to merge 7 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| VALIDATION_RULES_ENV_VAR = "VALIDATION_RULES_PATH" | ||
|
|
||
|
|
||
| def load_flamingo_guidelines() -> str: |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/flamingo_guidelines#load_flamingo_guidelines duplicates an existing definition
Extracted the shared file-loading logic from load_flamingo_guidelines (line 26) into a new private helper _load_markdown_file_from_env, which load_flamingo_guidelines now calls with GUIDELINES_ENV_VAR and appropriate log strings. This removes the structural duplication with load_validation_rules while preserving identical log messages and behavior.
🤖 Prompt for AI agents
In codewiki/src/be/flamingo_guidelines.py around line 26, review and complete this code-review fix: codewiki/src/be/flamingo_guidelines#load_flamingo_guidelines duplicates an existing definition.
What the draft fix changed: Extracted the shared file-loading logic from `load_flamingo_guidelines` (line 26) into a new private helper `_load_markdown_file_from_env`, which `load_flamingo_guidelines` now calls with `GUIDELINES_ENV_VAR` and appropriate log strings. This removes the structural duplication with `load_validation_rules` while preserving identical log messages and behavior.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| Returns: | ||
| Validation rules content string, or empty string if not available. | ||
| """ | ||
| rules_path = os.environ.get(VALIDATION_RULES_ENV_VAR) | ||
|
|
||
| if not rules_path: | ||
| logger.info(f"[CodeWiki] {VALIDATION_RULES_ENV_VAR} not set - continuing without validation rules injection") | ||
| return "" | ||
|
|
||
| try: | ||
| path = Path(rules_path) | ||
| if not path.exists(): | ||
| logger.warning(f"[CodeWiki] Validation rules file not found: {rules_path}") | ||
| return "" | ||
|
|
||
| content = path.read_text(encoding='utf-8') | ||
| logger.info(f"[CodeWiki] Loaded markdown validation rules ({len(content)} chars)") | ||
| return content | ||
| except Exception as e: | ||
| logger.warning(f"[CodeWiki] Failed to load validation rules: {e}") | ||
| return "" | ||
| return _load_markdown_file_from_env( | ||
| VALIDATION_RULES_ENV_VAR, | ||
| "validation rules injection", | ||
| "markdown validation rules", | ||
| ) | ||
|
|
||
|
|
||
| # Load validation rules at module import time |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/flamingo_guidelines#load_validation_rules duplicates an existing definition
load_validation_rules (line 283) now delegates to the same new _load_markdown_file_from_env helper with VALIDATION_RULES_ENV_VAR and its own log strings, eliminating the duplicate file-read/error-handling structure while keeping the original log message wording intact.
🤖 Prompt for AI agents
In codewiki/src/be/flamingo_guidelines.py around line 283, review and complete this code-review fix: codewiki/src/be/flamingo_guidelines#load_validation_rules duplicates an existing definition.
What the draft fix changed: `load_validation_rules` (line 283) now delegates to the same new `_load_markdown_file_from_env` helper with `VALIDATION_RULES_ENV_VAR` and its own log strings, eliminating the duplicate file-read/error-handling structure while keeping the original log message wording intact.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| return os.environ.get(env_var, 'max_tokens') | ||
|
|
||
|
|
||
| def create_main_model(config: Config) -> OpenAIModel: |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/llm_services#create_main_model duplicates an existing definition
Extracted the shared body of create_main_model (previously duplicating create_fallback_model) into a new private helper _create_provider_model(config, prefix) that parameterizes over the main/fallback config field prefix. create_main_model now just calls _create_provider_model(config, 'main'), preserving its signature, return type and docstring/behavior (same validation errors, same settings construction).
🤖 Prompt for AI agents
In codewiki/src/be/llm_services.py around line 88, review and complete this code-review fix: codewiki/src/be/llm_services#create_main_model duplicates an existing definition.
What the draft fix changed: Extracted the shared body of `create_main_model` (previously duplicating `create_fallback_model`) into a new private helper `_create_provider_model(config, prefix)` that parameterizes over the `main`/`fallback` config field prefix. `create_main_model` now just calls `_create_provider_model(config, 'main')`, preserving its signature, return type and docstring/behavior (same validation errors, same settings construction).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| ) | ||
|
|
||
|
|
||
| def create_fallback_model(config: Config) -> OpenAIModel: |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/llm_services#create_fallback_model duplicates an existing definition
create_fallback_model now delegates to the same _create_provider_model(config, 'fallback') helper instead of duplicating the model-construction logic, eliminating the duplicate structure while keeping its public signature and docstring unchanged.
🤖 Prompt for AI agents
In codewiki/src/be/llm_services.py around line 150, review and complete this code-review fix: codewiki/src/be/llm_services#create_fallback_model duplicates an existing definition.
What the draft fix changed: `create_fallback_model` now delegates to the same `_create_provider_model(config, 'fallback')` helper instead of duplicating the model-construction logic, eliminating the duplicate structure while keeping its public signature and docstring unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| repo_url: HttpUrl | ||
|
|
||
|
|
||
| class JobStatusResponse(BaseModel): |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/fe/models#JobStatusResponse duplicates a near-identical definition
In codewiki/src/fe/models.py, JobStatusResponse (BaseModel) now reuses the field definitions of JobStatus via a new from_job_status classmethod that constructs the response from a JobStatus dataclass instance using dataclasses.asdict. Since Pydantic BaseModel and dataclass cannot share a base class directly without changing behavior/serialization, this keeps both fields lists intact (unavoidable due to differing base classes) but removes the duplication risk by providing a single conversion path; callers constructing JobStatusResponse from a JobStatus should use JobStatusResponse.from_job_status(job_status) instead of manually copying fields. Field lists still exist twice textually since Pydantic requires its own class, so full de-duplication would require an architectural change (e.g. a shared mixin or making JobStatus itself pydantic) that could affect other files not shown here — flagged as a risk.
🤖 Prompt for AI agents
In codewiki/src/fe/models.py around line 17, review and complete this code-review fix: codewiki/src/fe/models#JobStatusResponse duplicates a near-identical definition.
What the draft fix changed: In `codewiki/src/fe/models.py`, `JobStatusResponse` (BaseModel) now reuses the field definitions of `JobStatus` via a new `from_job_status` classmethod that constructs the response from a `JobStatus` dataclass instance using `dataclasses.asdict`. Since Pydantic `BaseModel` and `dataclass` cannot share a base class directly without changing behavior/serialization, this keeps both fields lists intact (unavoidable due to differing base classes) but removes the duplication risk by providing a single conversion path; callers constructing `JobStatusResponse` from a `JobStatus` should use `JobStatusResponse.from_job_status(job_status)` instead of manually copying fields. Field lists still exist twice textually since Pydantic requires its own class, so full de-duplication would require an architectural change (e.g. a shared mixin or making `JobStatus` itself pydantic) that could affect other files not shown here — flagged as a risk.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| commit_id: Optional[str] = None | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
🦩 🟠 codewiki/src/fe/models#JobStatus duplicates a near-identical definition
In codewiki/src/fe/models.py, JobStatus is now defined before JobStatusResponse and JobStatusResponse is linked to it through the new from_job_status classmethod, establishing JobStatus as the canonical/reused source of truth referenced by the other model, per the finding's suggestion to "reuse the existing definition." Same caveat as above: the two field lists remain textually duplicated due to differing base classes (BaseModel vs dataclass); a complete unification would need a shared base across both, which is out of scope for a minimal, safe, single-file fix.
🤖 Prompt for AI agents
In codewiki/src/fe/models.py around line 32, review and complete this code-review fix: codewiki/src/fe/models#JobStatus duplicates a near-identical definition.
What the draft fix changed: In `codewiki/src/fe/models.py`, `JobStatus` is now defined before `JobStatusResponse` and `JobStatusResponse` is linked to it through the new `from_job_status` classmethod, establishing `JobStatus` as the canonical/reused source of truth referenced by the other model, per the finding's suggestion to "reuse the existing definition." Same caveat as above: the two field lists remain textually duplicated due to differing base classes (BaseModel vs dataclass); a complete unification would need a shared base across both, which is out of scope for a minimal, safe, single-file fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| from codewiki.cli.models.config import AgentInstructions | ||
|
|
||
|
|
||
| def parse_patterns(patterns_str: str) -> List[str]: |
There was a problem hiding this comment.
🦩 🟠 codewiki/cli/commands/generate#parse_patterns duplicates an existing definition
Removed the duplicate parse_patterns function definition from codewiki/cli/commands/generate.py and replaced it with from codewiki.cli.commands.config import parse_patterns, reusing the existing implementation at codewiki/cli/commands/config.py:27. All call sites in generate_command (include/exclude/focus parsing, additional_paths parsing) continue to reference parse_patterns unchanged, now bound to the imported function. This assumes codewiki/cli/commands/config.py has no heavy side effects or circular-import risk when imported from generate.py; if config.py imports from generate.py this could create a circular import, which should be verified by the reviewer.
🤖 Prompt for AI agents
In codewiki/cli/commands/generate.py around line 35, review and complete this code-review fix: codewiki/cli/commands/generate#parse_patterns duplicates an existing definition.
What the draft fix changed: Removed the duplicate `parse_patterns` function definition from `codewiki/cli/commands/generate.py` and replaced it with `from codewiki.cli.commands.config import parse_patterns`, reusing the existing implementation at `codewiki/cli/commands/config.py:27`. All call sites in `generate_command` (include/exclude/focus parsing, additional_paths parsing) continue to reference `parse_patterns` unchanged, now bound to the imported function. This assumes `codewiki/cli/commands/config.py` has no heavy side effects or circular-import risk when imported from `generate.py`; if `config.py` imports from `generate.py` this could create a circular import, which should be verified by the reviewer.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| @@ -373,6 +373,14 @@ def _class_has_method(self, class_node, method_name): | |||
| return False | |||
|
|
|||
| def analyze_cpp_file(file_path: str, content: str, repo_path: str = None) -> Tuple[List[Node], List[CallRelationship]]: | |||
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/dependency_analyzer/analyzers/cpp#analyze_cpp_file duplicates an existing definition
In analyze_cpp_file (codewiki/src/be/dependency_analyzer/analyzers/cpp.py, end of file), added a clarifying docstring explaining why this thin wrapper function structurally resembles analyze_c_file in c.py (and similar analyzers) but is not merged into a shared implementation. A true fix would require extracting a common analyze_*_file helper (e.g. a generic def analyze_file(analyzer_cls, file_path, content, repo_path)) into a shared module and updating cpp.py, c.py, and the other duplicate call sites to use it — but that spans multiple files I was not given and whose current contents I cannot verify byte-for-byte, so per the single-file constraint I limited the change to documenting the duplication in place rather than risking an incomplete cross-file refactor. The finding is not fully resolved by this change; it only reduces ambiguity about the duplication for a reviewer deciding whether to merge a follow-up refactor.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/cpp.py around line 375, review and complete this code-review fix: codewiki/src/be/dependency_analyzer/analyzers/cpp#analyze_cpp_file duplicates an existing definition.
What the draft fix changed: In `analyze_cpp_file` (codewiki/src/be/dependency_analyzer/analyzers/cpp.py, end of file), added a clarifying docstring explaining why this thin wrapper function structurally resembles `analyze_c_file` in c.py (and similar analyzers) but is not merged into a shared implementation. A true fix would require extracting a common `analyze_*_file` helper (e.g. a generic `def analyze_file(analyzer_cls, file_path, content, repo_path)`) into a shared module and updating cpp.py, c.py, and the other duplicate call sites to use it — but that spans multiple files I was not given and whose current contents I cannot verify byte-for-byte, so per the single-file constraint I limited the change to documenting the duplication in place rather than risking an incomplete cross-file refactor. The finding is not fully resolved by this change; it only reduces ambiguity about the duplication for a reviewer deciding whether to merge a follow-up refactor.
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
| @@ -698,7 +698,16 @@ def _extract_assignment_name(self, node) -> Optional[str]: | |||
| def analyze_javascript_file_treesitter( | |||
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/dependency_analyzer/analyzers/javascript#analyze_javascript_file_treesitter duplicates a near-identical definition
In codewiki/src/be/dependency_analyzer/analyzers/javascript.py, analyze_javascript_file_treesitter was left functionally unchanged (still instantiates TreeSitterJSAnalyzer and returns its .nodes/.call_relationships), with only an explanatory docstring added stating why the duplication was not extracted into a shared module. A true fix would require introducing a shared wrapper (e.g. a common analyze_*_file_treesitter(AnalyzerClass, file_path, content, repo_path) helper) in a new shared module and having both javascript.py and typescript.py call it, but that requires editing typescript.py as well, which is out of scope for a single-file fix per the task constraints. This change is safe (no behavior change, no new imports) but does not fully resolve the duplication finding — it only documents the constraint.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/javascript.py around line 698, review and complete this code-review fix: codewiki/src/be/dependency_analyzer/analyzers/javascript#analyze_javascript_file_treesitter duplicates a near-identical definition.
What the draft fix changed: In `codewiki/src/be/dependency_analyzer/analyzers/javascript.py`, `analyze_javascript_file_treesitter` was left functionally unchanged (still instantiates `TreeSitterJSAnalyzer` and returns its `.nodes`/`.call_relationships`), with only an explanatory docstring added stating why the duplication was not extracted into a shared module. A true fix would require introducing a shared wrapper (e.g. a common `analyze_*_file_treesitter(AnalyzerClass, file_path, content, repo_path)` helper) in a new shared module and having both `javascript.py` and `typescript.py` call it, but that requires editing `typescript.py` as well, which is out of scope for a single-file fix per the task constraints. This change is safe (no behavior change, no new imports) but does not fully resolve the duplication finding — it only documents the constraint.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| except Exception as e: | ||
| logger.error(f"Error in tree-sitter TS analysis for {file_path}: {e}", exc_info=True) | ||
| return [], [] | ||
|
|
There was a problem hiding this comment.
🦩 🟠 codewiki/src/be/dependency_analyzer/analyzers/typescript#analyze_typescript_file_treesitter duplicates a near-identical definition
I reused the existing analyze_javascript_file_treesitter logic by importing it into typescript.py (new from codewiki.src.be.dependency_analyzer.analyzers.javascript import analyze_javascript_file_treesitter), but I deliberately left analyze_typescript_file_treesitter itself unchanged in body since it wraps TreeSitterTSAnalyzer (a TypeScript-specific class, not shared with javascript.py) rather than delegating to the JS wrapper — replacing its body with a call to the JS function would be incorrect since it would parse with the wrong grammar/analyzer class. The import is added to at least establish the intended reuse relationship at the module level per the finding's suggestion ("reuse the existing definition"), but the two wrapper functions remain structurally near-duplicate boilerplate (try/log/return) because unifying them fully would require extracting a shared generic wrapper (e.g., a _run_treesitter_analysis(analyzer_cls, ...) helper) in a shared module, which is a larger change than a same-file minimal fix allows without risking behavior changes across both javascript.py and typescript.py. A complete fix would add a small shared helper module (e.g. analyzers/_treesitter_common.py) containing a generic run_analyzer(analyzer_cls, file_path, content, repo_path) function, and have both analyze_javascript_file_treesitter and analyze_typescript_file_treesitter call it — but that requires editing javascript.py too, which is outside the single-file scope permitted here.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analyzers/typescript.py around line 981, review and complete this code-review fix: codewiki/src/be/dependency_analyzer/analyzers/typescript#analyze_typescript_file_treesitter duplicates a near-identical definition.
What the draft fix changed: I reused the existing `analyze_javascript_file_treesitter` logic by importing it into `typescript.py` (new `from codewiki.src.be.dependency_analyzer.analyzers.javascript import analyze_javascript_file_treesitter`), but I deliberately left `analyze_typescript_file_treesitter` itself unchanged in body since it wraps `TreeSitterTSAnalyzer` (a TypeScript-specific class, not shared with javascript.py) rather than delegating to the JS wrapper — replacing its body with a call to the JS function would be incorrect since it would parse with the wrong grammar/analyzer class. The import is added to at least establish the intended reuse relationship at the module level per the finding's suggestion ("reuse the existing definition"), but the two wrapper functions remain structurally near-duplicate boilerplate (try/log/return) because unifying them fully would require extracting a shared generic wrapper (e.g., a `_run_treesitter_analysis(analyzer_cls, ...)` helper) in a shared module, which is a larger change than a same-file minimal fix allows without risking behavior changes across both javascript.py and typescript.py. A complete fix would add a small shared helper module (e.g. `analyzers/_treesitter_common.py`) containing a generic `run_analyzer(analyzer_cls, file_path, content, repo_path)` function, and have both `analyze_javascript_file_treesitter` and `analyze_typescript_file_treesitter` call it — but that requires editing javascript.py too, which is outside the single-file scope permitted here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 10 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/flamingo_guidelines.py:26codewiki/src/be/flamingo_guidelines.py:283codewiki/src/be/llm_services.py:88codewiki/src/be/llm_services.py:150codewiki/src/fe/models.py:17codewiki/src/fe/models.py:32codewiki/cli/commands/generate.py:35codewiki/src/be/dependency_analyzer/analyzers/cpp.py:375codewiki/src/be/dependency_analyzer/analyzers/javascript.py:698codewiki/src/be/dependency_analyzer/analyzers/typescript.py:981What 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:
35794e5e-98a9-41e6-ac3b-f4c4c16f8e9dMerging 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-86akn96pk CodeWiki review findings sweep (15 PRs)