:sapling: Add more verbose logs to solution server #852
Conversation
Signed-off-by: Fabian von Feilitzsch <[email protected]>
WalkthroughAdds structured logging across the package: configures a central module logger in constants, adds a DB-specific logger with guarded state-change logging in the DAO, and instruments server flows (startup, DB mutations, solution/file lifecycle, hint generation, and tool wrappers). No API signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py (1)
6-14: Consider safer file logging and avoiding global logging config at import time
- Optional: switch to a rotating file handler and specify UTF-8 to prevent unbounded log growth and encoding issues.
- Optional: configuring logging via basicConfig at import time in a module that may be imported elsewhere can surprise embedding apps. If this module is only used as the server entry stack, it’s fine; otherwise, gate it (e.g., only configure if no handlers are present).
Apply this diff to improve the file handler:
logging.basicConfig( level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", handlers=[ - logging.FileHandler("kai_mcp_server.log"), + logging.handlers.RotatingFileHandler( + "kai_mcp_server.log", maxBytes=10_000_000, backupCount=5, encoding="utf-8" + ), logging.StreamHandler(sys.stderr), ], )kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (3)
21-21: Prefer single logging mechanism (avoid mixinglogandlogger)You’ve added robust structured logging; consider phasing out the legacy log helper to prevent duplicate messages to stderr and keep a single path.
809-824: Remove extraneous f-stringAt Line 822, the f-string has no placeholders. Drop the leading f to satisfy Ruff F541 and avoid confusion.
Apply this diff:
- else: - logger.info(f"[RESPONSE] get_best_hint - No hint found") + else: + logger.info("[RESPONSE] get_best_hint - No hint found")
1024-1031: Background task: capture and log exceptions
asyncio.create_task(...)will swallow exceptions unless awaited or inspected. Add a done-callback to log failures; this prevents silent errors in background hint generation.Apply this diff:
- logger.info( + logger.info( f"[DB_STATE] All solutions for client {client_id} are ACCEPTED or MODIFIED - triggering hint generation" ) - asyncio.create_task(generate_hint_v3(kai_ctx, client_id)) # type: ignore[unused-awaitable, unused-ignore] + task = asyncio.create_task(generate_hint_v3(kai_ctx, client_id)) # type: ignore[unused-awaitable, unused-ignore] + def _log_task_result(t: asyncio.Task) -> None: + exc = t.exception() + if exc: + logger.error(f"[HINT_GENERATION] Background task failed: {exc}") + task.add_done_callback(_log_task_result)Optionally, also consider cancelling outstanding tasks on shutdown to avoid stray work on process exit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py(1 hunks)kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py(2 hunks)kai_mcp_solution_server/src/kai_mcp_solution_server/server.py(25 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (3)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py (1)
log(23-32)kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py (3)
get_async_engine(102-113)DBSolution(269-340)update_solution_status(313-328)kai_mcp_solution_server/src/kai_mcp_solution_server/db/python_objects.py (2)
SolutionFile(7-9)SolutionStatus(104-109)
kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py (1)
kai_mcp_solution_server/src/kai_mcp_solution_server/db/python_objects.py (1)
SolutionStatus(104-109)
🪛 Ruff (0.12.2)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py
822-822: f-string without any placeholders
Remove extraneous f prefix
(F541)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py
19-19: Use a context manager for opening files
(SIM115)
🔇 Additional comments (9)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py (1)
16-17: LGTM: central module logger name is consistentLogger name “kai_mcp_solution_server” establishes the parent for child loggers in other modules. Good choice for hierarchical filtering.
kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py (3)
8-9: LGTM: dedicated DB loggerUsing a child logger “kai_mcp_solution_server.db” keeps DB logs scoped and easy to filter.
314-322: LGTM: status change guarded with before/after and logged once
- Capturing previous_status and only logging on change avoids noise.
- Early return on first non-ACCEPTED file matches the “first failing file determines solution status” rule.
325-329: LGTM: acceptance transition loggingTransition to ACCEPTED only when all files are accepted is clear, and logging the transition adds useful auditability.
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (5)
118-148: LGTM: clear startup instrumentationGood separation of [STARTUP]/[STARTUP_ERROR] and explicit model initialization logging. The use of
render_as_string(hide_password=True)later is a nice touch.
259-271: LGTM: tool request/response loggingClear REQUEST/RESPONSE pairing makes tracing calls straightforward.
286-303: LGTM: batched incident creation loggingGood visibility with counts and per-item debug detail gated by INFO/DEBUG.
933-942: LGTM: accept_file visibilityUseful INFO/DEBUG details around affected solutions and targeted file URI.
621-633: No print() calls in server module—suggestion not applicableI located the server implementation at
./kai/rpc_server/server.pyand ran a full search forprint()calls; none were found. There’s no stderr print to replace, so you can ignore the recommendation to remove adjacentprint()statements here.Likely an incorrect or invalid review comment.
| # Keep the original log function for compatibility but enhance it | ||
| log_file = open("stderr.log", "a+") | ||
| log_file.close() | ||
|
|
||
|
|
||
| def log(*args: Any, **kwargs: Any) -> None: | ||
| print(*args, file=log_file if not log_file.closed else sys.stderr, **kwargs) | ||
| """Legacy log function - enhanced with timestamp and logging level""" | ||
| timestamp = datetime.now().isoformat() | ||
| message = " ".join(str(arg) for arg in args) | ||
| logger.info(message) | ||
| print( | ||
| f"[{timestamp}] {message}", | ||
| file=log_file if not log_file.closed else sys.stderr, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
Fix legacy log(): avoid potential TypeError with conflicting 'file' kwarg and remove phantom file handle
- The current implementation always passes a file= argument to print and also forwards kwargs; if the caller passes file in kwargs, print will raise “got multiple values for argument 'file'”.
- The module-level log_file is opened and immediately closed; it will never be used. Ruff SIM115 is triggered here as well.
Apply this diff to simplify and harden the legacy logger:
-# Keep the original log function for compatibility but enhance it
-log_file = open("stderr.log", "a+")
-log_file.close()
+# Keep the original log function for compatibility but route to stdlib logging and stderr.
def log(*args: Any, **kwargs: Any) -> None:
"""Legacy log function - enhanced with timestamp and logging level"""
timestamp = datetime.now().isoformat()
message = " ".join(str(arg) for arg in args)
logger.info(message)
- print(
- f"[{timestamp}] {message}",
- file=log_file if not log_file.closed else sys.stderr,
- **kwargs,
- )
+ # Preserve print-like kwargs but avoid conflicting 'file'
+ kwargs = dict(kwargs)
+ kwargs.pop("file", None)
+ print(f"[{timestamp}] {message}", file=sys.stderr, **kwargs)📝 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.
| # Keep the original log function for compatibility but enhance it | |
| log_file = open("stderr.log", "a+") | |
| log_file.close() | |
| def log(*args: Any, **kwargs: Any) -> None: | |
| print(*args, file=log_file if not log_file.closed else sys.stderr, **kwargs) | |
| """Legacy log function - enhanced with timestamp and logging level""" | |
| timestamp = datetime.now().isoformat() | |
| message = " ".join(str(arg) for arg in args) | |
| logger.info(message) | |
| print( | |
| f"[{timestamp}] {message}", | |
| file=log_file if not log_file.closed else sys.stderr, | |
| **kwargs, | |
| ) | |
| # Keep the original log function for compatibility but route to stdlib logging and stderr. | |
| def log(*args: Any, **kwargs: Any) -> None: | |
| """Legacy log function - enhanced with timestamp and logging level""" | |
| timestamp = datetime.now().isoformat() | |
| message = " ".join(str(arg) for arg in args) | |
| logger.info(message) | |
| # Preserve print-like kwargs but avoid conflicting 'file' | |
| kwargs = dict(kwargs) | |
| kwargs.pop("file", None) | |
| print(f"[{timestamp}] {message}", file=sys.stderr, **kwargs) |
🧰 Tools
🪛 Ruff (0.12.2)
19-19: Use a context manager for opening files
(SIM115)
| logger.info("[STARTUP] KAI MCP Solution Server starting...") | ||
| try: | ||
| settings = SolutionServerSettings() | ||
| log(f"Settings: {settings.model_dump_json(indent=2)}") | ||
| logger.info( | ||
| f"[STARTUP] Database DSN: {settings.db_dsn.render_as_string(hide_password=True)}" | ||
| ) | ||
| logger.info(f"[STARTUP] Drop all tables: {settings.drop_all}") | ||
|
|
There was a problem hiding this comment.
Do not log full settings; risk of secret leakage
log(f"Settings: {settings.model_dump_json(indent=2)}") immediately above these lines will dump all settings, including llm_params, which often contain API keys/tokens. This is a high-risk leak in logs.
- Remove that call or redact sensitive fields before logging any settings snapshot.
- Continue to log DSN via
render_as_string(hide_password=True)as done here.
If you want a sanitized snapshot, consider:
def sanitized_settings(settings: SolutionServerSettings) -> dict[str, Any]:
data = settings.model_dump()
# Redact common sensitive fields
redacted = {"llm_params", "api_key", "token", "authorization", "password"}
for k in list(data):
if k.lower() in redacted:
data[k] = "***REDACTED***"
return data
# Usage:
# logger.info("[STARTUP] Settings: %s", json.dumps(sanitized_settings(settings), indent=2, default=str))🤖 Prompt for AI Agents
In kai_mcp_solution_server/src/kai_mcp_solution_server/server.py around lines
155 to 163, the current call log(f"Settings:
{settings.model_dump_json(indent=2)}") dumps all settings including secrets (API
keys/tokens) and must be removed or replaced; remove that direct dump and
instead either omit logging the full settings or create and log a sanitized
settings snapshot that redacts sensitive keys (e.g., llm_params, api_key, token,
authorization, password) before serialization, while keeping the existing
logger.info call that prints the DSN via render_as_string(hide_password=True).
fabianvf
force-pushed
the
kai-mcp-ss-verbose-logs
branch
from
554ae58 to
b9c9332
Compare
Signed-off-by: Fabian von Feilitzsch <[email protected]>
fabianvf
force-pushed
the
kai-mcp-ss-verbose-logs
branch
from
b9c9332 to
f2c0ac2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (1)
158-163: Remove full settings dump from logs; high risk of secret leakage (repeat).
log(f"Settings: {settings.model_dump_json(indent=2)}")will serializellm_paramsand other sensitive fields (API keys/tokens). This is a critical leak vector and must be removed or replaced with a sanitized snapshot.Apply this diff to stop leaking secrets:
- log(f"Settings: {settings.model_dump_json(indent=2)}") + # Do not log raw settings; they may contain secrets (e.g., API keys) + logger.info("[STARTUP] Settings loaded (redacted)")If you still want a sanitized snapshot, add this helper (outside the shown range) and log it:
def sanitized_settings(settings: SolutionServerSettings) -> dict[str, Any]: data = settings.model_dump() # pydantic-safe types redacted_keys = {"llm_params", "api_key", "token", "authorization", "password", "secret"} for k in list(data): if k.lower() in redacted_keys: data[k] = "***REDACTED***" return data # Usage at startup: # logger.info("[STARTUP] Settings: %s", json.dumps(sanitized_settings(settings), indent=2, default=str))
🧹 Nitpick comments (7)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (7)
174-176: Preferlogger.exceptionto capture traceback; avoid dual logging of the same error.
logger.error(...); log(traceback...)splits stack traces across two loggers and may duplicate outputs. Uselogger.exception(...)once.- logger.error(f"[STARTUP_ERROR] Failed to start server: {str(e)}") - log(f"Error in lifespan: {traceback.format_exc()}") + logger.exception("[STARTUP_ERROR] Failed to start server")
624-627: Don’t mixKeep all emissions via the configured logger to avoid duplicate or out-of-band logs.
- print( - f"No accepted or modified solutions found for client {client_id}. No hint generated.", - file=sys.stderr, - ) + # Maintain a single logging surface + # (message already logged at INFO above)
408-417: Avoidsession.commit()insideasync_sessionmaker.begin()blocks; useflush()and rely on context commit.Calling
commit()inside abegin()context is redundant and can complicate transaction handling. If you only need PKs,await session.flush()is sufficient; the context manager will commit on exit.Example refactor pattern:
# Inside `async with session_maker.begin() as session:` session.add(obj) await session.flush() # PKs available here via obj.id # (No explicit commit here) # Context manager commits on normal exitApplies similarly to
create_incident,create_solution, anddelete_solution.Also applies to: 727-729
286-303: Batch-create incidents in a single transaction to reduce overhead.Looping over
create_incidentopens multiple transactions and commits per item. Prefer a dedicated bulk path that creates all incidents in onebegin()block, then flushes once for IDs.Sketch:
async with kai_ctx.session_maker.begin() as session: # Preload or create violations once per unique (ruleset_name, violation_name) # Create DBIncident objects for all extended_incidents session.add_all(incidents) await session.flush() results = [CreateIncidentResult(incident_id=inc.id, solution_id=0) for inc in incidents] return resultsIf helpful, I can provide a concrete refactor.
21-21: Unify onlogger.*; avoid the legacylog()helper to prevent duplicates and split formatting.This module mixes
log()andlogger.*. Givenlog()already callslogger.infoand also prints to a file/stderr, messages can be duplicated. Preferloggerexclusively throughout.Examples to update:
- Replace
log("...")withlogger.debug("...")orlogger.info("...")as appropriate.- Keep one sink; rely on handlers configured in
constants.py.Also applies to: 965-967, 1040-1042, 1093-1093
440-441: Prefer parameterized logging over f-strings for consistency and lazy formatting.Use
logger.info("... %s ...", value)rather than f-strings so formatting cost is avoided when the level is disabled, and to keep consistency with structured logging.Example:
# Before logger.info(f"[REQUEST] create_solution - client_id: {client_id}, incidents: {incident_ids}") # After logger.info("[REQUEST] create_solution - client_id: %s, incidents: %s", client_id, incident_ids)Applies similarly to other log statements in this file.
Also applies to: 741-743, 809-811, 910-912
263-269: Single global lock serializes all operations; consider per-client locks to improve throughput.
kai_ctx.lockmakes all tool calls globally sequential. Consider a simple lock registry keyed byclient_idto isolate contention between different clients.Sketch:
# In KaiSolutionServerContext self._locks: dict[str, asyncio.Lock] = {} def lock_for(self, client_id: str) -> asyncio.Lock: return self._locks.setdefault(client_id, asyncio.Lock()) # Usage async with kai_ctx.lock_for(client_id): ...This keeps per-client consistency while enabling concurrency across clients.
Also applies to: 293-300, 444-452, 746-752, 814-823, 915-925
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py(3 hunks)kai_mcp_solution_server/src/kai_mcp_solution_server/server.py(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (3)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py (1)
log(23-32)kai_mcp_solution_server/src/kai_mcp_solution_server/db/dao.py (3)
get_async_engine(102-113)DBSolution(269-340)update_solution_status(313-328)kai_mcp_solution_server/src/kai_mcp_solution_server/db/python_objects.py (2)
SolutionFile(7-9)SolutionStatus(104-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
🔇 Additional comments (2)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (2)
610-611: Hint generation logging is solid; good use of lifecycle markers.Also applies to: 621-623, 631-633, 685-686, 699-701, 705-707
715-729: Deletion flow logging is comprehensive and clear.Good job covering not-found, pre-delete state, and post-commit success.
| logger.info("[STARTUP] Initializing database engine...") | ||
| self.engine = await get_async_engine( | ||
| self.settings.db_dsn, self.settings.drop_all | ||
| ) | ||
| self.session_maker = async_sessionmaker( | ||
| bind=self.engine, expire_on_commit=False | ||
| ) | ||
| logger.info("[STARTUP] Database engine initialized successfully") | ||
|
|
There was a problem hiding this comment.
Guard against destructive drop_all=True in non-dev environments.
drop_all=True will irreversibly wipe the database. Add an explicit environment guard so this can’t happen accidentally outside dev/test.
Suggested patch:
async def create(self) -> None:
- logger.info("[STARTUP] Initializing database engine...")
+ logger.info("[STARTUP] Initializing database engine...")
+ # Safety guard: forbid drop_all outside dev/test unless explicitly allowed
+ if self.settings.drop_all:
+ env = os.getenv("KAI_ENV", "").lower()
+ if env not in {"dev", "test", "local"} and os.getenv("KAI_ALLOW_DROP_ALL") != "1":
+ logger.error("[STARTUP_ERROR] Refusing to start with drop_all=True in environment: %s", env or "<unset>")
+ raise RuntimeError("Refusing to run with drop_all=True outside dev/test without KAI_ALLOW_DROP_ALL=1")Also applies to: 160-163
🤖 Prompt for AI Agents
In kai_mcp_solution_server/src/kai_mcp_solution_server/server.py around lines
119-127 (and also apply same guard at 160-163), the code may call
get_async_engine with settings.drop_all=True which will irreversibly wipe the
database; add an explicit environment guard that prevents drop_all from being
true outside safe environments: check the current environment (e.g.,
settings.env or an ENV var like KAI_ENV) and only allow drop_all when it equals
"development" or "test" (or when a deliberate override flag like
ALLOW_DB_DROP=true is set); if not in an allowed env, set drop_all to False (or
raise a clear runtime error) and log a warning explaining the override so
accidental destructive runs are prevented.
|
LGTM |
3 participants
Summary by CodeRabbit
New Features
Chores