fixes #343 and #344

[Shashankss1205]

Reference Issues/PRs

Fixes #343 and #344

What does this implement/fix? Explain your changes.

Refactors runtime configuration handling by centralizing environment-variable-based settings into a dedicated configuration module and simplifying runtime initialization logic.

Key Changes

Added

• "src/sktime_mcp/config.py"
  • Introduces a centralized "Settings" configuration class.
  • Adds typed configuration properties for:
    • "SKTIME_MCP_LOG_LEVEL"
    • "SKTIME_MCP_LOG_PATH"
    • "SKTIME_MCP_AUTO_FORMAT"
    • "SKTIME_MCP_JOB_MAX_AGE_HOURS"
    • "SKTIME_MCP_JOB_CLEANUP_INTERVAL"
  • Provides documented defaults and environment-variable-driven overrides.
  • Exposes a shared "settings" instance for consistent runtime access.

Updated

• "src/sktime_mcp/runtime/executor.py"

  • Removes direct environment variable parsing from runtime initialization.
  • Replaces inline "os.environ.get(...)" usage with centralized "settings.auto_format".
  • Simplifies executor setup and improves configuration consistency.

• "src/sktime_mcp/server.py"

  • Replaces scattered configuration logic with shared settings-based access.
  • Removes duplicated environment variable parsing for:
    • logging
    • cleanup intervals
    • job retention
  • Improves logger initialization flow using centralized config values.
  • Simplifies periodic cleanup scheduling logic.
  • Refactors stdio handling comments and startup flow for clarity.

• "README.md"

  • Adds a configuration table documenting supported environment variables and defaults.

• Validation / Error Handling

  • Improves explicit error messaging for missing required dataframe columns.

Does your contribution introduce a new dependency?

No new dependencies introduced.

What should a reviewer concentrate their feedback on?

1. Centralized configuration approach

   • Whether consolidating all runtime env-variable access into "config.py" improves maintainability and clarity versus localized access.

2. Settings API design

   • Use of computed "@Property" methods instead of loading values eagerly during initialization.

3. Runtime behavior compatibility

   • Ensuring previous environment-variable behavior remains fully backward compatible.

4. Logging initialization changes

   • Verifying logger setup still behaves correctly across:
     • stderr-only mode
     • file logging mode
     • custom log levels

Testing / Validation

Verified locally:

• ✅ Existing MCP server startup works correctly
• ✅ Environment variable overrides apply successfully
• ✅ Logger initialization behaves as expected
• ✅ Periodic cleanup task initializes correctly
• ✅ Auto-format configuration is correctly propagated into runtime execution

Any other comments?

This refactor mainly aims to:

• reduce duplicated configuration logic,
• improve readability,
• simplify future config additions,
• and make runtime behavior easier to test.

PR checklist

For all contributions

• I've added unit tests and made sure they pass locally ("make check").
• I've updated relevant documentation where applicable.
• I've verified backward compatibility for existing environment variables.

[felipeangelimvieira]

Is this really desired? Could you explain in more details the problem that motivated this change? Although there is a comment, I'm not sure if I understand the practical problem that was happening

[Shashankss1205]

You're right, the current comment was probably too vague.

The intent behind this change is to protect the MCP JSON-RPC transport stream. Since MCP communicates over stdout/stdin, any accidental "print()" or third-party library output written to stdout can corrupt protocol messages and cause the client to fail parsing responses.

For example, if stdout contains:

loading model...
{"jsonrpc":"2.0","id":1,"result":...}

the client may fail because it expects stdout to contain only JSON-RPC payloads.

By redirecting:
sys.stdout = sys.stderr

any accidental prints/debug output are pushed to stderr instead, keeping stdout reserved for MCP responses.

That said, the current implementation also included an unused "original_stdout" variable, which I’ll remove since stdout is not restored later.