Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions codewiki/src/be/flamingo_guidelines.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
# In prompts:
prompt = f"{get_custom_instructions_section()}{get_guidelines_section()}Your actual prompt here..."
"""
import logging
import os
from pathlib import Path

logger = logging.getLogger(__name__)

GUIDELINES_ENV_VAR = "FLAMINGO_MARKDOWN_GUIDELINES_PATH"
CUSTOM_INSTRUCTIONS_ENV_VAR = "CUSTOM_REPO_INSTRUCTIONS"
VALIDATION_RULES_ENV_VAR = "VALIDATION_RULES_PATH"
Expand All @@ -34,20 +37,20 @@ def load_flamingo_guidelines() -> str:
guidelines_path = os.environ.get(GUIDELINES_ENV_VAR)

if not guidelines_path:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

print(f"[CodeWiki] {GUIDELINES_ENV_VAR} not set - continuing without Flamingo guidelines")
logger.info(f"[CodeWiki] {GUIDELINES_ENV_VAR} not set - continuing without Flamingo guidelines")
return ""

try:
path = Path(guidelines_path)
if not path.exists():
print(f"[CodeWiki] Guidelines file not found: {guidelines_path}")
logger.warning(f"[CodeWiki] Guidelines file not found: {guidelines_path}")
return ""

content = path.read_text(encoding='utf-8')
print(f"[CodeWiki] Loaded Flamingo markdown guidelines ({len(content)} chars)")
logger.info(f"[CodeWiki] Loaded Flamingo markdown guidelines ({len(content)} chars)")
return content
except Exception as e:
print(f"[CodeWiki] Failed to load guidelines: {e}")
logger.warning(f"[CodeWiki] Failed to load guidelines: {e}")
return ""


Expand Down Expand Up @@ -75,12 +78,12 @@ def sanitize_problematic_patterns(text: str) -> str:
"""
import re

print(f"[DEBUG] sanitize_problematic_patterns called - input length: {len(text)}")
logger.debug(f"[DEBUG] sanitize_problematic_patterns called - input length: {len(text)}")

# Count braces before sanitization
open_count_before = text.count('{')
close_count_before = text.count('}')
print(f"[DEBUG] BEFORE: {{ count={open_count_before}, }} count={close_count_before}")
logger.debug(f"[DEBUG] BEFORE: {{ count={open_count_before}, }} count={close_count_before}")

# 1. GitHub Actions syntax: ${{...}} β†’ ${...}
# Use iterative approach for robustness with nested braces
Expand All @@ -104,8 +107,8 @@ def sanitize_problematic_patterns(text: str) -> str:
# Count braces after sanitization
open_count_after = text.count('{')
close_count_after = text.count('}')
print(f"[DEBUG] AFTER: {{ count={open_count_after}, }} count={close_count_after}")
print(f"[DEBUG] Sample (first 200 chars): {text[:200]}")
logger.debug(f"[DEBUG] AFTER: {{ count={open_count_after}, }} count={close_count_after}")
logger.debug(f"[DEBUG] Sample (first 200 chars): {text[:200]}")

return text

Expand Down Expand Up @@ -146,7 +149,7 @@ def sanitize_and_escape_format_braces(text: str) -> str:
"""
import re

print(f"[DEBUG] sanitize_and_escape_format_braces called - input length: {len(text)}")
logger.debug(f"[DEBUG] sanitize_and_escape_format_braces called - input length: {len(text)}")

# STEP 1: SANITIZATION (if not already done)
# This ensures problematic patterns are normalized before we escape braces
Expand All @@ -164,7 +167,7 @@ def preserve_numeric(match):

# Replace all {digit} patterns with markers
text = re.sub(r'\{(\d+)\}', preserve_numeric, text)
print(f"[DEBUG] Preserved {len(numeric_placeholders)} numeric placeholders: {list(numeric_placeholders.values())}")
logger.debug(f"[DEBUG] Preserved {len(numeric_placeholders)} numeric placeholders: {list(numeric_placeholders.values())}")

# STEP 3: ESCAPE ALL REMAINING BRACES
# Now escape ALL braces (non-numeric content like {Decision}, {Component})
Expand All @@ -180,8 +183,8 @@ def preserve_numeric(match):
# Count braces after escaping
open_count_after = result.count('{')
close_count_after = result.count('}')
print(f"[DEBUG] AFTER ESCAPING: {{ count={open_count_after}, }} count={close_count_after}")
print(f"[DEBUG] Sample (first 200 chars): {result[:200]}")
logger.debug(f"[DEBUG] AFTER ESCAPING: {{ count={open_count_after}, }} count={close_count_after}")
logger.debug(f"[DEBUG] Sample (first 200 chars): {result[:200]}")

return result

Expand Down Expand Up @@ -259,16 +262,16 @@ def load_custom_instructions() -> str:
custom_instructions = os.environ.get(CUSTOM_INSTRUCTIONS_ENV_VAR, "")

if not custom_instructions:
print(f"[CodeWiki] {CUSTOM_INSTRUCTIONS_ENV_VAR} not set - continuing without custom instructions")
logger.info(f"[CodeWiki] {CUSTOM_INSTRUCTIONS_ENV_VAR} not set - continuing without custom instructions")
return ""

print(f"[CodeWiki] Loaded custom repo instructions ({len(custom_instructions)} chars)")
logger.info(f"[CodeWiki] Loaded custom repo instructions ({len(custom_instructions)} chars)")

# CRITICAL: Sanitize on input - this is the ONLY place text sanitization should happen
# Shell scripts pass raw text, and we handle all sanitization here in Python
# This prevents double-sanitization and ensures consistent behavior
sanitized = sanitize_problematic_patterns(custom_instructions)
print(f"[CodeWiki] Sanitized custom instructions ({len(sanitized)} chars after sanitization)")
logger.info(f"[CodeWiki] Sanitized custom instructions ({len(sanitized)} chars after sanitization)")

return sanitized

Expand All @@ -291,20 +294,20 @@ def load_validation_rules() -> str:
rules_path = os.environ.get(VALIDATION_RULES_ENV_VAR)

if not rules_path:
print(f"[CodeWiki] {VALIDATION_RULES_ENV_VAR} not set - continuing without validation rules injection")
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():
print(f"[CodeWiki] Validation rules file not found: {rules_path}")
logger.warning(f"[CodeWiki] Validation rules file not found: {rules_path}")
return ""

content = path.read_text(encoding='utf-8')
print(f"[CodeWiki] Loaded markdown validation rules ({len(content)} chars)")
logger.info(f"[CodeWiki] Loaded markdown validation rules ({len(content)} chars)")
return content
except Exception as e:
print(f"[CodeWiki] Failed to load validation rules: {e}")
logger.warning(f"[CodeWiki] Failed to load validation rules: {e}")
return ""


Expand Down Expand Up @@ -375,3 +378,4 @@ def get_custom_instructions_section() -> str:
escaped_instructions +
"\n\n---\n"
)

15 changes: 12 additions & 3 deletions codewiki/src/fe/cache_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"""

import hashlib
import logging
from datetime import datetime, timedelta
from pathlib import Path
from typing import Optional, Dict
Expand All @@ -12,6 +13,8 @@
from .config import WebAppConfig
from codewiki.src.utils import file_manager

logger = logging.getLogger(__name__)


class CacheManager:
"""Manages documentation cache."""
Expand All @@ -38,7 +41,13 @@ def load_cache_index(self):
last_accessed=datetime.fromisoformat(value['last_accessed'])
)
except Exception as e:
print(f"Error loading cache index: {e}")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

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."""
Comment on lines 41 to 53

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -56,7 +65,7 @@ def save_cache_index(self):

file_manager.save_json(data, index_file)
except Exception as e:
print(f"Error saving cache index: {e}")
logger.error(f"Error saving cache index: {e}")

def get_repo_hash(self, repo_url: str) -> str:
"""Generate hash for repository URL."""
Expand Down Expand Up @@ -116,4 +125,4 @@ def cleanup_expired_cache(self):
del self.cache_index[repo_hash]

if expired_entries:
self.save_cache_index()
self.save_cache_index()
18 changes: 13 additions & 5 deletions codewiki/src/fe/github_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@
GitHub repository processing utilities.
"""

import logging
import os
import shutil
import subprocess
from typing import Dict
from urllib.parse import urlparse

from .config import WebAppConfig

logger = logging.getLogger(__name__)


class GitHubRepoProcessor:
"""Handles GitHub repository processing."""
Expand Down Expand Up @@ -66,7 +70,7 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->
], capture_output=True, text=True, timeout=WebAppConfig.CLONE_TIMEOUT)

if result.returncode != 0:
print(f"Error cloning repository: {result.stderr}")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

logger.error(f"Error cloning repository: {result.stderr}")
return False

# Checkout specific commit
Comment on lines 70 to 76

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -75,7 +79,9 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->
], cwd=target_dir, capture_output=True, text=True, timeout=30)

if result.returncode != 0:
print(f"Error checking out commit {commit_id}: {result.stderr}")
logger.error(f"Error checking out commit {commit_id}: {result.stderr}")
if os.path.isdir(target_dir):
shutil.rmtree(target_dir, ignore_errors=True)
return False
else:
# Clone repository with shallow depth (default behavior)
Expand All @@ -84,10 +90,12 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->
], 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

return True
except Exception as e:
print(f"Error cloning repository: {e}")
return False
logger.error(f"Error cloning repository: {e}")
if os.path.isdir(target_dir):
shutil.rmtree(target_dir, ignore_errors=True)
return False
29 changes: 16 additions & 13 deletions codewiki/src/fe/visualise_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"""

import argparse
import logging
import sys
from pathlib import Path
from typing import Dict, Optional
Expand All @@ -25,6 +26,8 @@
from .templates import DOCS_VIEW_TEMPLATE
from codewiki.src.utils import file_manager

logger = logging.getLogger(__name__)

app = FastAPI(title="Documentation Server", description="Simple documentation server for hosting markdown documentation folders")

# Global variables to store configuration
Expand Down Expand Up @@ -55,13 +58,13 @@ def load_module_tree(docs_folder: Path) -> Optional[Dict]:
"""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}")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

logger.warning(f"module_tree.json not found in {docs_folder}")
return None

try:
return file_manager.load_json(tree_file)
except Exception as e:
print(f"Error loading module_tree.json: {e}")
logger.error(f"Error loading module_tree.json: {e}")
return None


Expand Down Expand Up @@ -219,17 +222,17 @@ def main():
# Validate docs folder
docs_folder = Path(args.docs_folder)
if not docs_folder.exists():
print(f"Error: Documentation folder '{docs_folder}' does not exist")
logger.error(f"Documentation folder '{docs_folder}' does not exist")
sys.exit(1)

if not docs_folder.is_dir():
print(f"Error: '{docs_folder}' is not a directory")
logger.error(f"'{docs_folder}' is not a directory")
sys.exit(1)

# Check for overview.md
overview_file = docs_folder / "overview.md"
if not overview_file.exists():
print(f"Warning: overview.md not found in '{docs_folder}'")
logger.warning(f"overview.md not found in '{docs_folder}'")

# Set global variables and environment variable for uvicorn reload
global DOCS_FOLDER, MODULE_TREE
Expand All @@ -240,16 +243,16 @@ def main():
import os
os.environ['DOCS_FOLDER'] = DOCS_FOLDER

print(f"πŸ“š Starting documentation server...")
print(f"πŸ“ Documentation folder: {DOCS_FOLDER}")
print(f"🌐 Server running at: http://{args.host}:{args.port}")
print(f"πŸ“– Main page: overview.md")
logger.info(f"πŸ“š Starting documentation server...")
logger.info(f"πŸ“ Documentation folder: {DOCS_FOLDER}")
logger.info(f"🌐 Server running at: http://{args.host}:{args.port}")
logger.info(f"πŸ“– Main page: overview.md")

if MODULE_TREE:
modules_count = len(MODULE_TREE)
print(f"πŸ—‚οΈ Found {modules_count} main modules in module_tree.json")
logger.info(f"πŸ—‚οΈ Found {modules_count} main modules in module_tree.json")

print("\nPress Ctrl+C to stop the server")
logger.info("Press Ctrl+C to stop the server")

try:
import uvicorn
Expand All @@ -261,8 +264,8 @@ def main():
log_level="debug" if args.debug else "info"
)
except KeyboardInterrupt:
print("\nπŸ‘‹ Server stopped")
logger.info("πŸ‘‹ Server stopped")


if __name__ == "__main__":
main()
main()