fix(CODEWIKI-005): CU-86akbhhru 6 review findings across 4 files - #52
Conversation
| last_accessed=datetime.fromisoformat(value['last_accessed']) | ||
| ) | ||
| except Exception as e: | ||
| print(f"Error loading cache index: {e}") |
There was a problem hiding this comment.
🦩 🟠 cache_manager.py uses print() instead of the required logging module
Added import logging and module-level logger = logging.getLogger(__name__) at the top of codewiki/src/fe/cache_manager.py, and replaced the print(f"Error loading cache index: {e}") call in load_cache_index (and the analogous print in save_cache_index for consistency) with logger.error(...) calls, per CODEWIKI-005.
🤖 Prompt for AI agents
In codewiki/src/fe/cache_manager.py around line 41, review and complete this code-review fix: cache_manager.py uses print() instead of the required logging module.
What the draft fix changed: Added `import logging` and module-level `logger = logging.getLogger(__name__)` at the top of `codewiki/src/fe/cache_manager.py`, and replaced the `print(f"Error loading cache index: {e}")` call in `load_cache_index` (and the analogous `print` in `save_cache_index` for consistency) with `logger.error(...)` calls, per CODEWIKI-005.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| last_accessed=datetime.fromisoformat(value['last_accessed']) | ||
| ) | ||
| except Exception as e: | ||
| print(f"Error loading cache index: {e}") | ||
| logger.error(f"Error loading cache index: {e}, backing up corrupted file") | ||
| try: | ||
| backup_file = self.cache_dir / f"cache_index.json.corrupted.{int(datetime.now().timestamp())}" | ||
| index_file.rename(backup_file) | ||
| except Exception as backup_error: | ||
| logger.error(f"Error backing up corrupted cache index: {backup_error}") | ||
| self.cache_index = {} | ||
|
|
||
| def save_cache_index(self): | ||
| """Save cache index to disk.""" |
There was a problem hiding this comment.
🦩 🟠 field_manager.load_json exceptions in CacheManager are silently swallowed, corrupting cache index recovery
In load_cache_index, the broad except Exception block now logs the error via logger.error and additionally attempts to rename the corrupted cache_index.json to a timestamped .corrupted.<ts> backup file (with its own nested try/except logged on failure), then explicitly resets self.cache_index = {} so recovery state is deterministic rather than partially populated. This surfaces the failure via logging and preserves the bad file for inspection, but does not implement schema validation, does not notify calling code of the failure (no exception re-raised or return flag), and does not attempt actual repair of the JSON content — a complete fix may require propagating a status/exception to callers of CacheManager.__init__ or adding schema validation, which is out of scope for this minimal change.
🤖 Prompt for AI agents
In codewiki/src/fe/cache_manager.py around line 29, review and complete this code-review fix: field_manager.load_json exceptions in CacheManager are silently swallowed, corrupting cache index recovery.
What the draft fix changed: In `load_cache_index`, the broad `except Exception` block now logs the error via `logger.error` and additionally attempts to rename the corrupted `cache_index.json` to a timestamped `.corrupted.<ts>` backup file (with its own nested try/except logged on failure), then explicitly resets `self.cache_index = {}` so recovery state is deterministic rather than partially populated. This surfaces the failure via logging and preserves the bad file for inspection, but does not implement schema validation, does not notify calling code of the failure (no exception re-raised or return flag), and does not attempt actual repair of the JSON content — a complete fix may require propagating a status/exception to callers of `CacheManager.__init__` or adding schema validation, which is out of scope for this minimal change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| ], capture_output=True, text=True, timeout=WebAppConfig.CLONE_TIMEOUT) | ||
|
|
||
| if result.returncode != 0: | ||
| print(f"Error cloning repository: {result.stderr}") |
There was a problem hiding this comment.
🦩 🟠 github_processor.py uses print() for error output instead of logging
Replaced all print(...) calls with logger.error(...) in GitHubRepoProcessor.clone_repository (both clone-failure and checkout-failure branches, plus the outer except Exception handler), and added a module-level logger = logging.getLogger(__name__) plus import logging at the top of the file.
🤖 Prompt for AI agents
In codewiki/src/fe/github_processor.py around line 69, review and complete this code-review fix: github_processor.py uses print() for error output instead of logging.
What the draft fix changed: Replaced all `print(...)` calls with `logger.error(...)` in `GitHubRepoProcessor.clone_repository` (both clone-failure and checkout-failure branches, plus the outer `except Exception` handler), and added a module-level `logger = logging.getLogger(__name__)` plus `import logging` at the top of the file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| ], capture_output=True, text=True, timeout=WebAppConfig.CLONE_TIMEOUT) | ||
|
|
||
| if result.returncode != 0: | ||
| print(f"Error cloning repository: {result.stderr}") | ||
| logger.error(f"Error cloning repository: {result.stderr}") | ||
| return False | ||
|
|
||
| # Checkout specific commit |
There was a problem hiding this comment.
🦩 🟠 GitHubRepoProcessor.clone_repository leaves target_dir/.git present after full clone even on checkout failure, and swallows all exceptions broadly
In GitHubRepoProcessor.clone_repository, added shutil.rmtree(target_dir, ignore_errors=True) cleanup (guarded by os.path.isdir) after the checkout failure branch (when commit_id is set and git checkout fails) and in the outer except Exception handler, so a fully-cloned-but-failed-checkout directory is removed instead of left on disk; added import shutil. Not fully verified against all callers' expectations of target_dir state on failure, and does not address partial/interrupted clone directories in the non-commit_id shallow-clone failure path, which was left unchanged since that path never completes a full clone before failing.
🤖 Prompt for AI agents
In codewiki/src/fe/github_processor.py around line 73, review and complete this code-review fix: GitHubRepoProcessor.clone_repository leaves target_dir/.git present after full clone even on checkout failure, and swallows all exceptions broadly.
What the draft fix changed: In `GitHubRepoProcessor.clone_repository`, added `shutil.rmtree(target_dir, ignore_errors=True)` cleanup (guarded by `os.path.isdir`) after the checkout failure branch (when `commit_id` is set and `git checkout` fails) and in the outer `except Exception` handler, so a fully-cloned-but-failed-checkout directory is removed instead of left on disk; added `import shutil`. Not fully verified against all callers' expectations of `target_dir` state on failure, and does not address partial/interrupted clone directories in the non-commit_id shallow-clone failure path, which was left unchanged since that path never completes a full clone before failing.
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
| @@ -34,20 +37,20 @@ def load_flamingo_guidelines() -> str: | |||
| guidelines_path = os.environ.get(GUIDELINES_ENV_VAR) | |||
|
|
|||
| if not guidelines_path: | |||
There was a problem hiding this comment.
🦩 🟠 print() used for diagnostic output inside codewiki/src/be module instead of logging
Added import logging and logger = logging.getLogger(__name__) at module top, and replaced every print(...) call across load_flamingo_guidelines, sanitize_problematic_patterns, sanitize_and_escape_format_braces, load_custom_instructions, and load_validation_rules with appropriate logger.info/logger.debug/logger.warning calls (info for normal load status, debug for the brace-count/sanitization trace output, warning for missing-file/exception cases), fully removing print() usage per CODEWIKI-005 while preserving message text and call sites unchanged.
🤖 Prompt for AI agents
In codewiki/src/be/flamingo_guidelines.py around line 36, review and complete this code-review fix: print() used for diagnostic output inside codewiki/src/be module instead of logging.
What the draft fix changed: Added `import logging` and `logger = logging.getLogger(__name__)` at module top, and replaced every `print(...)` call across `load_flamingo_guidelines`, `sanitize_problematic_patterns`, `sanitize_and_escape_format_braces`, `load_custom_instructions`, and `load_validation_rules` with appropriate `logger.info`/`logger.debug`/`logger.warning` calls (info for normal load status, debug for the brace-count/sanitization trace output, warning for missing-file/exception cases), fully removing print() usage per CODEWIKI-005 while preserving message text and call sites unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| """Load the module tree structure from module_tree.json.""" | ||
| tree_file = docs_folder / "module_tree.json" | ||
| if not tree_file.exists(): | ||
| print(f"Warning: module_tree.json not found in {docs_folder}") |
There was a problem hiding this comment.
🦩 🟠 visualise_docs.py uses print() throughout instead of logging
Replaced all print() calls with a module-level logger (created via logging.getLogger(__name__) at the top of the file) throughout load_module_tree() and main(): warnings use logger.warning, errors use logger.error, and informational startup/status messages use logger.info. This addresses the finding directly by routing all diagnostic output through logging instead of print(). Note: no logging.basicConfig() is added since this module is a library/CLI entry point invoked via main(), and adding global logging configuration was judged out of scope (a refactor) for this minimal fix — if the root logger has no handlers configured elsewhere, logger.info/warning calls will still emit via Python's default handler for WARNING+ but INFO messages may not be visible unless the application configures logging elsewhere. This is a reasonable minimal fix but a reviewer may want an explicit logging.basicConfig() call in main() to guarantee visibility of the startup banners.
🤖 Prompt for AI agents
In codewiki/src/fe/visualise_docs.py around line 58, review and complete this code-review fix: visualise_docs.py uses print() throughout instead of logging.
What the draft fix changed: Replaced all `print()` calls with a module-level `logger` (created via `logging.getLogger(__name__)` at the top of the file) throughout `load_module_tree()` and `main()`: warnings use `logger.warning`, errors use `logger.error`, and informational startup/status messages use `logger.info`. This addresses the finding directly by routing all diagnostic output through logging instead of print(). Note: no `logging.basicConfig()` is added since this module is a library/CLI entry point invoked via `main()`, and adding global logging configuration was judged out of scope (a refactor) for this minimal fix — if the root logger has no handlers configured elsewhere, `logger.info`/`warning` calls will still emit via Python's default handler for WARNING+ but `INFO` messages may not be visible unless the application configures logging elsewhere. This is a reasonable minimal fix but a reviewer may want an explicit `logging.basicConfig()` call in `main()` to guarantee visibility of the startup banners.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
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>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts: - deps.py, typescript.py: competing module docstrings added by both this branch and #55; resolved in favour of the wording on main. - config.py: this branch's new module docstring kept, layered on top of #49's widened dataclasses import (fields, asdict). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflict in test_clustering_simple.py: both this branch and #53 replaced the hardcoded test repo path with a TEST_REPO_PATH env lookup, differing only in the fallback. Resolved in favour of main's fallback, which #53 applied consistently across the other clustering test scripts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). flamingo_guidelines.py is fully superseded by #52, which has merged: this branch's changes to that file are dropped and the file is taken from main wholesale. Resolving the conflict hunk-by-hunk instead left a duplicate 'import logging', since main already has one. What remains is the part of this PR #52 did not cover: the analysis_service.py dead-code cleanup and the background_worker.py print -> logging conversion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes 6 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/fe/cache_manager.py:41codewiki/src/fe/cache_manager.py:29codewiki/src/fe/github_processor.py:69codewiki/src/fe/github_processor.py:73codewiki/src/be/flamingo_guidelines.py:36codewiki/src/fe/visualise_docs.py:58What 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:
2cc7a212-e9ac-481a-a76e-5f03d762c00cMerging 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-86akbhhru CodeWiki backend and CLI review findings (12 PRs)