feat(mcp): surface scheduled mcp_tool tasks before they break (#390) - #557
feat(mcp): surface scheduled mcp_tool tasks before they break (#390)#557padak wants to merge 1 commit into
Conversation
Phase 2 of the MCP removal, part 1 of 2: find the tasks, do not rewrite them. `agent --type mcp_tool` is removed in v0.85.0. It is the one deprecated surface whose users are absent when it breaks: an interactive `tool call` warns on every invocation right up to removal, but a scheduled task was warned once -- when it was created -- and then runs unattended. At removal it simply starts failing on its next cron tick. - `kbagent doctor` gains an `mcp_tool_tasks` check listing each affected task, the tool it calls and the native command that replaces it. A tool the parity map does not know reports `native_command: null` rather than inventing one. Filesystem only -- no API call, no MCP spawn -- and skips when there is no agents.json. - `kbagent agent list` notes the affected tasks under the table and points at doctor. The marker is deliberately NOT in the Type cell: Rich truncated it to "DEPRECA…" there, which is worse than not flagging it. `--json` gains an additive per-task `deprecation` key, present only on affected tasks so every existing consumer sees a byte-identical payload. Deliberately NOT included: automatic rewriting. ParityEntry carries only the command name, no argument mapping, while mcp_tool params are the MCP tool's own `input` dict -- different key naming and, for several tools, different structure. Guessing the argv of a scheduled WRITE task that then runs unattended is the worst place to be wrong; a bad rewrite beats a clear failure in no scenario. Part 2 will emit a reviewable migration plan with explicit TODOs instead.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| try: | ||
| tasks = AgentStore(config_dir=self._config_store.config_dir).load_tasks() | ||
| except Exception as exc: | ||
| return { | ||
| "check": "mcp_tool_tasks", | ||
| "name": "Deprecated mcp_tool agent tasks", | ||
| "status": "warn", | ||
| "message": f"Could not read {AGENTS_FILENAME}: {exc}", | ||
| } | ||
|
|
||
| affected = [t for t in tasks if getattr(t.action, "type", None) == "mcp_tool"] | ||
| if not affected: | ||
| return { | ||
| "check": "mcp_tool_tasks", | ||
| "name": "Deprecated mcp_tool agent tasks", | ||
| "status": "pass", | ||
| "message": f"No tasks use the deprecated 'mcp_tool' action ({len(tasks)} checked).", | ||
| } |
There was a problem hiding this comment.
🟡 Health check reports a clean result when the scheduled-task file is damaged
A damaged list of scheduled tasks is reported as healthy (the pass branch at src/keboola_agent_cli/services/doctor_service.py:216-222) instead of warning, because the reader silently returns an empty list for an unparsable file, so users are told nothing needs migrating when the check could not actually see their tasks.
Impact: Someone with soon-to-break scheduled tasks and a corrupted task file gets a false all-clear from the one command they run to check for problems.
Why the warn branch is unreachable and what the user sees
AgentStore.load_tasks() (src/keboola_agent_cli/server/agents_store.py:120-136) catches OSError / json.JSONDecodeError itself, logs a warning and returns []. It also skips individual entries that fail validation. Therefore the try/except Exception around the call in _check_mcp_tool_tasks never fires for the realistic failure modes, and the check falls through to the pass result with the message "No tasks use the deprecated 'mcp_tool' action (0 checked)". The PR's own test (tests/test_mcp_tool_task_detection.py:79-85) documents this by accepting either pass or warn. A more accurate behaviour would be to parse agents.json directly (or have the check detect that the file exists but yielded zero tasks) and report warn with the parse error.
Prompt for agents
In DoctorService._check_mcp_tool_tasks (src/keboola_agent_cli/services/doctor_service.py) the try/except around AgentStore.load_tasks() is effectively dead: AgentStore.load_tasks (src/keboola_agent_cli/server/agents_store.py) already swallows OSError and json.JSONDecodeError, logs, and returns an empty list, and also skips entries that fail pydantic validation. As a result a corrupt or unreadable agents.json produces a 'pass' result claiming no tasks use the deprecated mcp_tool action, which is a false all-clear in the exact command users run when they suspect something is wrong. Consider reading/parsing agents.json in the check itself (or detecting the 'file exists but zero tasks parsed' case) so a damaged file yields the intended 'warn' status with the parse error, while a genuinely empty task list still passes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) | ||
| raise typer.Exit(code=5) from None | ||
| payload = {"tasks": [t.model_dump(mode="json") for t in tasks]} | ||
| payload = {"tasks": [_annotate_deprecation(t.model_dump(mode="json")) for t in tasks]} |
There was a problem hiding this comment.
🔍 Serve REST /agents list does not carry the new deprecation key
The additive deprecation key is applied only in the CLI command layer, so kbagent serve's /agents listing and the Web UI keep the old payload and give no hint about tasks that will break at v0.85.0. That matches the PR description (CLI-only, phase 2 part 1), but it means UI users of scheduled tasks — arguably the population least likely to run kbagent doctor — still get no signal. Worth confirming it is planned for part 2.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Phase 2 of the MCP removal, part 1 of 2: find the tasks, do not rewrite them.
agent --type mcp_toolis removed in v0.85.0. It is the one deprecated surfacewhose users are absent when it breaks: an interactive
tool callwarns onevery invocation right up to removal, but a scheduled task was warned once --
when it was created -- and then runs unattended. At removal it simply starts
failing on its next cron tick.
kbagent doctorgains anmcp_tool_taskscheck listing each affected task,the tool it calls and the native command that replaces it. A tool the parity
map does not know reports
native_command: nullrather than inventing one.Filesystem only -- no API call, no MCP spawn -- and skips when there is no
agents.json.
kbagent agent listnotes the affected tasks under the table and points atdoctor. The marker is deliberately NOT in the Type cell: Rich truncated it to
"DEPRECA…" there, which is worse than not flagging it.
--jsongains anadditive per-task
deprecationkey, present only on affected tasks so everyexisting consumer sees a byte-identical payload.
Deliberately NOT included: automatic rewriting. ParityEntry carries only the
command name, no argument mapping, while mcp_tool params are the MCP tool's own
inputdict -- different key naming and, for several tools, differentstructure. Guessing the argv of a scheduled WRITE task that then runs
unattended is the worst place to be wrong; a bad rewrite beats a clear failure
in no scenario. Part 2 will emit a reviewable migration plan with explicit
TODOs instead.