Add ConfigManager for unified configuration management#239
Add ConfigManager for unified configuration management#239AnkushMalaker wants to merge 1 commit intodevfrom
Conversation
- Introduced a new `config_manager.py` module to handle reading and writing configurations from `config.yml` and `.env` files, ensuring backward compatibility. - Refactored `ChronicleSetup` in `backends/advanced/init.py` to utilize `ConfigManager` for loading and updating configurations, simplifying the setup process. - Removed redundant methods for loading and saving `config.yml` directly in `ChronicleSetup`, as these are now managed by `ConfigManager`. - Enhanced user feedback during configuration updates, including success messages for changes made to configuration files.
📝 WalkthroughWalkthroughThe pull request introduces a centralized ConfigManager module to replace local configuration handling in ChronicleSetup. ConfigManager provides unified management of config.yml and .env files, including validation, backup creation, and environment variable synchronization. ChronicleSetup is refactored to delegate all configuration operations to ConfigManager, removing direct file-handling methods. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChronicleSetup
participant ConfigManager
participant Files as config.yml/.env Files
User->>ChronicleSetup: setup_memory()
activate ChronicleSetup
ChronicleSetup->>ConfigManager: set_memory_provider(provider)
activate ConfigManager
rect rgb(220, 240, 255)
Note over ConfigManager: Validate & Update
ConfigManager->>Files: Create backup of config.yml
ConfigManager->>Files: Write provider to config.yml
ConfigManager->>Files: Create backup of .env
ConfigManager->>Files: Update MEMORY_PROVIDER in .env
end
ConfigManager->>ConfigManager: Set env var in current process
ConfigManager-->>ChronicleSetup: Return status (provider, config_path, env_path, restart_flag)
deactivate ConfigManager
ChronicleSetup->>ConfigManager: get_full_config()
ConfigManager->>Files: Load config.yml
ConfigManager-->>ChronicleSetup: Return full config
ChronicleSetup->>ChronicleSetup: Reload self.config_yml_data
deactivate ChronicleSetup
ChronicleSetup-->>User: Setup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
config_manager.py (3)
133-179: Consider using python-dotenv for .env file manipulation.The manual parsing and updating of .env files (lines 144-170) could be replaced with python-dotenv's
set_key()function, which handles edge cases more robustly (quoted values, comments, whitespace variations, etc.). Since the project already uses python-dotenv (imported in backends/advanced/init.py), using it here would reduce code and improve reliability.🔎 Proposed refactor using python-dotenv
Add import at top of file:
import yaml +from dotenv import set_keyReplace the _update_env_file method:
def _update_env_file(self, key: str, value: str): """Update a single key in .env file.""" if self.env_path is None: logger.debug("No service path set, skipping .env update") return if not self.env_path.exists(): logger.warning(f".env file not found at {self.env_path}") return try: - # Read current .env - with open(self.env_path, 'r') as f: - lines = f.readlines() - - # Update or add line - key_found = False - updated_lines = [] - - for line in lines: - if line.strip().startswith(f"{key}="): - updated_lines.append(f"{key}={value}\n") - key_found = True - else: - updated_lines.append(line) - - # If key wasn't found, add it - if not key_found: - updated_lines.append(f"\n# Auto-updated by ConfigManager\n{key}={value}\n") - # Create backup backup_path = f"{self.env_path}.bak" shutil.copy2(self.env_path, backup_path) logger.debug(f"Backed up .env to {backup_path}") - # Write updated file - with open(self.env_path, 'w') as f: - f.writelines(updated_lines) + # Update using python-dotenv + set_key(str(self.env_path), key, value, quote_mode="never") # Update environment variable for current process os.environ[key] = value logger.info(f"Updated {key}={value} in .env file") except Exception as e: logger.exception(f"Failed to update .env file: {e}") raise
218-222: Consider extracting error message for reusability.The error message construction could be extracted to improve maintainability, though the current implementation is clear and functional.
🔎 Optional refactor
+ VALID_PROVIDERS = ["chronicle", "openmemory_mcp", "mycelia"] + # Validate provider provider = provider.lower().strip() - valid_providers = ["chronicle", "openmemory_mcp", "mycelia"] - if provider not in valid_providers: + if provider not in VALID_PROVIDERS: raise ValueError( - f"Invalid provider '{provider}'. " - f"Valid providers: {', '.join(valid_providers)}" + f"Invalid provider '{provider}'. " + f"Valid providers: {', '.join(VALID_PROVIDERS)}" )
332-347: Singleton reinitialization behavior may be unexpected.The
get_config_manager()function reinitializes the global singleton whenever aservice_pathis explicitly provided (line 345). This means calling it with different service paths will create new instances, replacing the previous singleton. This could lead to confusion if different parts of the codebase expect to share the same instance.Consider documenting this behavior more explicitly in the docstring, or implementing a multi-instance pattern if different service paths need separate managers.
🔎 Improved documentation
def get_config_manager(service_path: Optional[str] = None) -> ConfigManager: """ Get global ConfigManager singleton instance. Args: service_path: Optional service path for .env updates. - If None, uses cached instance or creates new one. + If None, uses cached instance or creates new one with auto-detection. + If provided, replaces the global singleton with a new instance + configured for that service path. + + Note: + Providing service_path will reinitialize the singleton, affecting all + subsequent callers. Use with caution in multi-service contexts. Returns: ConfigManager instance """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backends/advanced/init.pyconfig_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Black formatter with 100-character line length for Python code
Use isort for Python import organization
ALL imports must be at the top of the file after the docstring - never import modules in the middle of functions or files
Group imports in Python files: standard library, third-party, then local imports
Use lazy imports sparingly and only when absolutely necessary for circular import issues in Python
Always raise errors in Python, never silently ignore - use explicit error handling with proper exceptions rather than silent failures
Avoid defensivehasattr()checks in Python - research and understand input/response or class structure instead
Files:
config_manager.pybackends/advanced/init.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T23:52:34.959Z
Learnt from: AnkushMalaker
Repo: chronicler-ai/chronicle PR: 178
File: backends/advanced/src/advanced_omi_backend/services/memory/providers/mycelia.py:218-223
Timestamp: 2025-12-08T23:52:34.959Z
Learning: In Python code (chronicle project), prefer logging.exception() inside except blocks to automatically log the full stack trace. When re-raising exceptions, always chain with 'raise ... from e' to preserve the original context; use 'raise ... from None' only if you explicitly want to suppress the context. This improves debuggability across Python files.
Applied to files:
config_manager.pybackends/advanced/init.py
🧬 Code graph analysis (1)
backends/advanced/init.py (1)
config_manager.py (3)
get_full_config(309-316)update_config_defaults(291-307)update_memory_config(260-279)
🪛 Ruff (0.14.10)
config_manager.py
109-109: Do not catch blind exception: Exception
(BLE001)
110-110: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
130-130: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
219-222: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
backends/advanced/init.py (2)
275-277: Configuration reload pattern is appropriate.The pattern of calling
config_manager.update_config_defaults()orconfig_manager.update_memory_config()followed by reloadingconfig_yml_dataensures the in-memory state stays synchronized with the persisted configuration. While this involves multiple disk reads, it's appropriate for a setup script prioritizing correctness over performance.Also applies to: 286-287, 296-297, 318-320, 331-341, 351-359
532-533: Good documentation of ConfigManager behavior.The comments clearly explain that ConfigManager handles persistence automatically, improving maintainability.
config_manager.py (1)
66-78: Fallback behavior when config.yml is not found.The method falls back to using the current directory as repo root if config.yml is not found (lines 76-78). This allows ConfigManager to work in scenarios where config.yml needs to be created, but could also mask configuration issues. The warning log is helpful, but ensure callers handle the case where configuration doesn't exist as expected.
Verify that the setup script and other callers properly handle scenarios where config.yml doesn't exist initially. The current implementation in backends/advanced/init.py (lines 49-51) provides a fallback default structure, which is good.
| # Initialize ConfigManager | ||
| repo_root = Path.cwd().parent.parent # backends/advanced -> repo root | ||
| if str(repo_root) not in sys.path: | ||
| sys.path.insert(0, str(repo_root)) | ||
|
|
||
| from config_manager import ConfigManager | ||
|
|
||
| self.config_manager = ConfigManager(service_path="backends/advanced") | ||
| self.console.print(f"[blue][INFO][/blue] Using config.yml at: {self.config_manager.config_yml_path}") | ||
|
|
||
| # Load existing config or create default structure | ||
| self.config_yml_data = self.config_manager.get_full_config() | ||
| if not self.config_yml_data: | ||
| self.console.print("[yellow][WARNING][/yellow] config.yml not found, will create default structure") | ||
| self.config_yml_data = self._get_default_config_structure() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for ConfigManager import.
The dynamic sys.path manipulation and import could fail if the directory structure is unexpected or if config_manager.py is missing. Consider wrapping the import in a try-except block to provide a clear error message.
🔎 Proposed fix with error handling
# Initialize ConfigManager
repo_root = Path.cwd().parent.parent # backends/advanced -> repo root
if str(repo_root) not in sys.path:
sys.path.insert(0, str(repo_root))
- from config_manager import ConfigManager
+ try:
+ from config_manager import ConfigManager
+ except ImportError as e:
+ self.console.print("[red][ERROR][/red] Failed to import ConfigManager. "
+ "Ensure config_manager.py exists at repository root.")
+ raise RuntimeError("ConfigManager import failed") from e
self.config_manager = ConfigManager(service_path="backends/advanced")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Initialize ConfigManager | |
| repo_root = Path.cwd().parent.parent # backends/advanced -> repo root | |
| if str(repo_root) not in sys.path: | |
| sys.path.insert(0, str(repo_root)) | |
| from config_manager import ConfigManager | |
| self.config_manager = ConfigManager(service_path="backends/advanced") | |
| self.console.print(f"[blue][INFO][/blue] Using config.yml at: {self.config_manager.config_yml_path}") | |
| # Load existing config or create default structure | |
| self.config_yml_data = self.config_manager.get_full_config() | |
| if not self.config_yml_data: | |
| self.console.print("[yellow][WARNING][/yellow] config.yml not found, will create default structure") | |
| self.config_yml_data = self._get_default_config_structure() | |
| # Initialize ConfigManager | |
| repo_root = Path.cwd().parent.parent # backends/advanced -> repo root | |
| if str(repo_root) not in sys.path: | |
| sys.path.insert(0, str(repo_root)) | |
| try: | |
| from config_manager import ConfigManager | |
| except ImportError as e: | |
| self.console.print("[red][ERROR][/red] Failed to import ConfigManager. " | |
| "Ensure config_manager.py exists at repository root.") | |
| raise RuntimeError("ConfigManager import failed") from e | |
| self.config_manager = ConfigManager(service_path="backends/advanced") | |
| self.console.print(f"[blue][INFO][/blue] Using config.yml at: {self.config_manager.config_yml_path}") | |
| # Load existing config or create default structure | |
| self.config_yml_data = self.config_manager.get_full_config() | |
| if not self.config_yml_data: | |
| self.console.print("[yellow][WARNING][/yellow] config.yml not found, will create default structure") | |
| self.config_yml_data = self._get_default_config_structure() |
🤖 Prompt for AI Agents
In backends/advanced/init.py around lines 37 to 51, the dynamic sys.path
manipulation and "from config_manager import ConfigManager" can raise
ImportError or other exceptions if the file or path is missing; wrap the import
in a try-except block that catches ImportError/Exception, print a clear error to
self.console (including the exception message and the attempted repo_root path),
and then fail fast (raise a RuntimeError or call sys.exit(1)) so the failure is
explicit and actionable; keep the sys.path insertion before the import and
ensure any caught exception includes the original traceback or message in the
output.
| try: | ||
| with open(self.config_yml_path, 'r') as f: | ||
| return yaml.safe_load(f) or {} | ||
| except Exception as e: | ||
| logger.error(f"Failed to load config.yml: {e}") | ||
| return {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve exception handling per coding guidelines.
The error handling should use logging.exception() to include the full stack trace for debugging. Based on learnings, this improves debuggability across Python files.
🔎 Proposed fix
try:
with open(self.config_yml_path, 'r') as f:
return yaml.safe_load(f) or {}
except Exception as e:
- logger.error(f"Failed to load config.yml: {e}")
+ logger.exception("Failed to load config.yml")
return {}As per coding guidelines and learnings, prefer logging.exception() inside except blocks to automatically log the full stack trace.
🧰 Tools
🪛 Ruff (0.14.10)
109-109: Do not catch blind exception: Exception
(BLE001)
110-110: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In config_manager.py around lines 106 to 111, the except block currently calls
logger.error with the exception message; change it to logger.exception(...) so
the full stack trace is logged automatically (you can pass a descriptive message
like "Failed to load config.yml" to logger.exception) and keep returning {}
afterwards; ensure the logger is the standard logging.Logger instance so
logger.exception is available.
| except Exception as e: | ||
| logger.error(f"Failed to save config.yml: {e}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Chain exception and use logging.exception().
The exception should be re-raised with proper chaining to preserve context, and logging.exception() should be used for automatic stack trace logging.
🔎 Proposed fix
except Exception as e:
- logger.error(f"Failed to save config.yml: {e}")
- raise
+ logger.exception("Failed to save config.yml")
+ raise RuntimeError("Failed to save config.yml") from eAs per coding guidelines and learnings, always chain exceptions with 'raise ... from e' to preserve the original context and use logging.exception() for better debuggability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error(f"Failed to save config.yml: {e}") | |
| raise | |
| except Exception as e: | |
| logger.exception("Failed to save config.yml") | |
| raise RuntimeError("Failed to save config.yml") from e |
🧰 Tools
🪛 Ruff (0.14.10)
130-130: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In config_manager.py around lines 129 to 131, the except block currently logs
the error with logger.error and re-raises without chaining; change it to use
logger.exception(...) to include the stack trace and re-raise the new exception
with proper chaining using "raise <exception> from e" (or simply "raise" from a
wrapped exception) so the original context is preserved and full traceback is
logged for debugging.
There was a problem hiding this comment.
@claude
Chain exception and use logging.exception().
can you do this codebase wide and make a PR?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| except Exception as e: | ||
| logger.error(f"Failed to update .env file: {e}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply consistent exception handling.
Similar to other methods, this should use logging.exception() and chain the exception properly.
🔎 Proposed fix
except Exception as e:
- logger.error(f"Failed to update .env file: {e}")
- raise
+ logger.exception("Failed to update .env file")
+ raise RuntimeError("Failed to update .env file") from eAs per coding guidelines and learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error(f"Failed to update .env file: {e}") | |
| raise | |
| except Exception as e: | |
| logger.exception("Failed to update .env file") | |
| raise RuntimeError("Failed to update .env file") from e |
🧰 Tools
🪛 Ruff (0.14.10)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In config_manager.py around lines 177 to 179, the except block currently calls
logger.error(...) and then raises, which is inconsistent with other methods;
replace the logger.error call with logger.exception("Failed to update .env
file") to log the stack trace, and re-raise the original exception (use plain
"raise" so the original traceback is preserved) or if you must raise a different
exception, use "raise NewException(...) from e" to chain properly.
| # Deep merge updates | ||
| config["memory"].update(updates) |
There was a problem hiding this comment.
Comment doesn't match implementation - shallow update, not deep merge.
The comment states "Deep merge updates" but dict.update() performs a shallow merge. This means nested dictionaries in updates will completely replace nested dictionaries in the config, rather than merging their contents recursively.
For example, if config has {"memory": {"extraction": {"enabled": True, "prompt": "..."}}} and you call update_memory_config({"extraction": {"enabled": False}}), the "prompt" field will be lost.
🔎 Proposed fix options
Option 1: Fix the comment (if shallow merge is intended)
- # Deep merge updates
+ # Shallow merge updates
config["memory"].update(updates)Option 2: Implement deep merge (if deep merge is needed)
- # Deep merge updates
- config["memory"].update(updates)
+ # Deep merge updates
+ def deep_merge(base: dict, updates: dict) -> None:
+ for key, value in updates.items():
+ if key in base and isinstance(base[key], dict) and isinstance(value, dict):
+ deep_merge(base[key], value)
+ else:
+ base[key] = value
+
+ deep_merge(config["memory"], updates)Verify which behavior is intended by checking how this method is called in backends/advanced/init.py (lines 318, 331, 351).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Deep merge updates | |
| config["memory"].update(updates) | |
| # Shallow merge updates | |
| config["memory"].update(updates) |
1 participant
config_manager.pymodule to handle reading and writing configurations fromconfig.ymland.envfiles, ensuring backward compatibility.ChronicleSetupinbackends/advanced/init.pyto utilizeConfigManagerfor loading and updating configurations, simplifying the setup process.config.ymldirectly inChronicleSetup, as these are now managed byConfigManager.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.