Add ability to archive tool server instead of deleting #728
Conversation
WalkthroughAdds an is_archived boolean across datamodels, API request/response models, web UI forms/pages, and tests; replaces delete flows with archive/unarchive UI actions; and introduces a before-model validator to backfill missing is_archived in ExternalToolServer properties. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Tool Server Page
participant API as Studio Server API
participant Model as ExternalToolServer
User->>UI: Click "Archive" / "Unarchive"
UI->>API: PATCH /tool_servers/{id} { "is_archived": true|false }
API->>Model: Persist/update properties
Note over Model: @model_validator(mode="before") ensures properties.has(is_archived) (default false)
Model-->>API: Updated server object
API-->>UI: 200 OK
UI->>API: GET /tool_servers/{id} && GET /available_tools
API->>API: Build available tools list
alt Exclude archived
API->>API: Filter out servers where is_archived == true
end
API-->>UI: Refreshed data
UI-->>User: Show updated archive state
sequenceDiagram
autonumber
participant Client as Web UI Form
participant API as Studio Server API
Client->>Client: fill form (name, desc, ..., is_archived)
Client->>API: POST/PUT with is_archived in payload
API->>API: Validate/serialize payload (is_archived -> properties)
API-->>Client: 201/200 created/updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEAD
Summary
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/core/kiln_ai/datamodel/external_tool_server.py (1)
28-34: Markis_archivedasNotRequired[bool]to match runtime behavior.The TypedDicts declare
is_archived: boolwithoutNotRequired, which means it's type-checked as a required field (due tototal=True). However, theupgrade_old_propertiesvalidator backfills this field when missing, making it effectively optional at runtime. This inconsistency can cause type-checking errors when creating these properties withoutis_archivedand may confuse developers about whether the field is required.Apply this diff to align the type declarations with the runtime behavior:
class LocalServerProperties(TypedDict, total=True): command: str args: NotRequired[list[str]] env_vars: NotRequired[dict[str, str]] secret_env_var_keys: NotRequired[list[str]] - is_archived: bool + is_archived: NotRequired[bool] class RemoteServerProperties(TypedDict, total=True): server_url: str headers: NotRequired[dict[str, str]] secret_header_keys: NotRequired[list[str]] - is_archived: bool + is_archived: NotRequired[bool]Also applies to: 36-41
app/desktop/studio_server/tool_api.py (1)
60-85: Makeis_archivedoptional with default False to avoid breaking clientsRequiring
is_archivedin all creation/edit payloads is a breaking change. Default it to False in all three request models.class ExternalToolServerCreationRequest(BaseModel): name: str description: str | None = None server_url: str - headers: Dict[str, str] = Field(default_factory=dict) - secret_header_keys: List[str] = Field(default_factory=list) - is_archived: bool + headers: Dict[str, str] = Field(default_factory=dict) + secret_header_keys: List[str] = Field(default_factory=list) + is_archived: bool = Field(default=False) class LocalToolServerCreationRequest(BaseModel): name: str description: str | None = None command: str args: List[str] env_vars: Dict[str, str] = Field(default_factory=dict) - secret_env_var_keys: List[str] = Field(default_factory=list) - is_archived: bool + secret_env_var_keys: List[str] = Field(default_factory=list) + is_archived: bool = Field(default=False) class KilnTaskToolServerCreationRequest(BaseModel): name: str description: str task_id: str run_config_id: str - is_archived: bool + is_archived: bool = Field(default=False)As per coding guidelines (Pydantic v2, strong typing).
🧹 Nitpick comments (6)
app/web_ui/src/lib/api_schema.d.ts (1)
4331-4332: LGTM: archived state added to RemoteServerPropertiesMatches Local/KilnTask properties. Optional: add a description on the backend model so OpenAPI surfaces helpful docs for this field here too.
app/desktop/studio_server/test_tool_api.py (1)
3566-3660: Add coverage for archived MCP servers in /available_toolsYou test exclusion for RAG and Kiln Task. Add a case where an MCP server has
is_archived: Trueand assert it’s excluded from MCP tool sets.libs/core/kiln_ai/datamodel/test_external_tool_server.py (1)
1185-1189: Minor naming/docstring mismatchDocstring says “upgrade_properties” but the validator is “upgrade_old_properties”. Consider aligning naming for clarity.
app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte (3)
253-341: Prevent double-submit and await refresh after archive/unarchiveAdd a simple guard and await data refresh to avoid racey UI states and accidental double clicks.
@@ - async function archive() { - update_archive(true) - } + async function archive() { + await update_archive(true) + } @@ - async function unarchive() { - update_archive(false) - } + async function unarchive() { + await update_archive(false) + } @@ - async function update_archive(is_archived: boolean) { - if (!tool_server) { - return - } + let archiving = false + async function update_archive(is_archived: boolean) { + if (!tool_server || archiving) return + archiving = true @@ - } catch (e) { + } catch (e) { if (is_archived) { archive_error = createKilnError(e) } else { unarchive_error = createKilnError(e) } - } finally { - fetch_tool_server() - if (project_id) { - load_available_tools(project_id, true) - } - } + } finally { + await fetch_tool_server() + if (project_id) { + await Promise.resolve(load_available_tools(project_id, true)) + } + archiving = false + }
365-367: Optional: disable button while archivingIf AppPage supports it, pass a disabled/isBusy flag tied to archiving to prevent repeated clicks.
422-455: Simplify type guards for readability (optional)Instead of
tool_server.type === "remote_mcp" && isToolType(tool_server, tool_server.type), considerisToolType(tool_server, "remote_mcp")(and similarly for local_mcp).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/desktop/studio_server/test_tool_api.py(70 hunks)app/desktop/studio_server/tool_api.py(6 hunks)app/web_ui/src/lib/api_schema.d.ts(4 hunks)app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/local_mcp/edit_local_tool.svelte(1 hunks)app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/remote_mcp/edit_remote_tool.svelte(1 hunks)app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte(5 hunks)libs/core/kiln_ai/datamodel/external_tool_server.py(2 hunks)libs/core/kiln_ai/datamodel/test_external_tool_server.py(24 hunks)libs/core/kiln_ai/tools/test_mcp_server_tool.py(14 hunks)libs/core/kiln_ai/tools/test_mcp_session_manager.py(15 hunks)libs/core/kiln_ai/tools/test_tool_registry.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
app/web_ui/**/*.{ts,svelte}
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.{ts,svelte}: Write frontend web code in TypeScript; do not add plain .js sources
From the web UI, make backend calls only to our FastAPI servers; do not call external services directly
Use TypeScript types and interfaces to maintain strong typing in the frontend
Files:
app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/local_mcp/edit_local_tool.svelteapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/remote_mcp/edit_remote_tool.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte
app/web_ui/**/*.svelte
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.svelte: Author components for Svelte v4 compatibility (do not use Svelte v5-only APIs)
Prefer Tailwind CSS and DaisyUI utility/classes for styling in Svelte components
Files:
app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/local_mcp/edit_local_tool.svelteapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/remote_mcp/edit_remote_tool.svelteapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte
app/web_ui/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for frontend code (including <script lang="ts"> in .svelte files)
Files:
app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/local_mcp/edit_local_tool.svelteapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/add_tools/remote_mcp/edit_remote_tool.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte
libs/{core,server}/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Python 3.10+ for library and server code under libs/core and libs/server
Files:
libs/core/kiln_ai/tools/test_mcp_session_manager.pylibs/core/kiln_ai/datamodel/test_external_tool_server.pylibs/core/kiln_ai/tools/test_mcp_server_tool.pylibs/core/kiln_ai/tools/test_tool_registry.pylibs/core/kiln_ai/datamodel/external_tool_server.py
{libs,app/desktop}/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
{libs,app/desktop}/**/*.py: Use Pydantic v2 (not v1) in Python code that models/validates data
Use explicit type hints for functions, classes, and public APIs in Python code
Files:
libs/core/kiln_ai/tools/test_mcp_session_manager.pylibs/core/kiln_ai/datamodel/test_external_tool_server.pyapp/desktop/studio_server/tool_api.pylibs/core/kiln_ai/tools/test_mcp_server_tool.pyapp/desktop/studio_server/test_tool_api.pylibs/core/kiln_ai/tools/test_tool_registry.pylibs/core/kiln_ai/datamodel/external_tool_server.py
libs/core/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code in libs/core must be compatible with Python 3.10+
Files:
libs/core/kiln_ai/tools/test_mcp_session_manager.pylibs/core/kiln_ai/datamodel/test_external_tool_server.pylibs/core/kiln_ai/tools/test_mcp_server_tool.pylibs/core/kiln_ai/tools/test_tool_registry.pylibs/core/kiln_ai/datamodel/external_tool_server.py
{libs/core,libs/server,app/desktop}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{libs/core,libs/server,app/desktop}/**/*.py: Use Pydantic v2 APIs (e.g., BaseModel v2, model_validate/model_dump) and avoid v1-only patterns
Maintain strong typing in Python code (add type hints and run type checking)
Files:
libs/core/kiln_ai/tools/test_mcp_session_manager.pylibs/core/kiln_ai/datamodel/test_external_tool_server.pyapp/desktop/studio_server/tool_api.pylibs/core/kiln_ai/tools/test_mcp_server_tool.pyapp/desktop/studio_server/test_tool_api.pylibs/core/kiln_ai/tools/test_tool_registry.pylibs/core/kiln_ai/datamodel/external_tool_server.py
app/desktop/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Python 3.13 for all desktop app Python code, including studio_server
Python code in app/desktop must target Python 3.13
Files:
app/desktop/studio_server/tool_api.pyapp/desktop/studio_server/test_tool_api.py
{libs/server,app/desktop/studio_server}/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
{libs/server,app/desktop/studio_server}/**/*.py: Implement REST APIs using FastAPI (no alternative web frameworks) in server code
Use asyncio-based patterns for async operations in backend servers
Files:
app/desktop/studio_server/tool_api.pyapp/desktop/studio_server/test_tool_api.py
app/desktop/studio_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Extend the core server with FastAPI endpoints in app/desktop/studio_server
Files:
app/desktop/studio_server/tool_api.pyapp/desktop/studio_server/test_tool_api.py
🧬 Code graph analysis (3)
libs/core/kiln_ai/datamodel/test_external_tool_server.py (1)
libs/core/kiln_ai/datamodel/external_tool_server.py (2)
ToolServerType(18-25)ExternalToolServer(51-464)
app/desktop/studio_server/tool_api.py (2)
app/web_ui/src/lib/types.ts (1)
ToolApiDescription(79-79)libs/core/kiln_ai/datamodel/tool_id.py (1)
build_kiln_task_tool_id(127-129)
libs/core/kiln_ai/datamodel/external_tool_server.py (1)
app/web_ui/src/lib/types.ts (1)
RemoteServerProperties(84-85)
⏰ 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). (1)
- GitHub Check: Generate Coverage Report
🔇 Additional comments (11)
libs/core/kiln_ai/datamodel/external_tool_server.py (1)
217-227: Good backwards compatibility approach.The validator correctly backfills
is_archivedwhen missing, enabling smooth migration for existing data. Themode="before"ensures it runs before other validators that might expect this field, and theNonecheck prevents errors when properties is absent.Note: This validator makes
is_archivedeffectively optional, so consider marking it asNotRequired[bool]in the TypedDict definitions (see separate comment) for type-checking consistency.app/web_ui/src/lib/api_schema.d.ts (3)
3763-3764: LGTM: archived state included in LocalServerPropertiesConsistent with KilnTaskServerProperties. Ensure the tool-server detail view reads archived state from properties.is_archived since ExternalToolServerApiDescription has no top-level field.
3230-3232: UI request bodies include is_archived — no change needed
All connect/edit flows setis_archived(defaultfalseon create, passed through on update).
3782-3783: is_archived included in both connect_local_mcp and edit_local_mcp calls
All POST and PATCH payloads in edit_local_tool.svelte and +page.svelte include an is_archived property.app/desktop/studio_server/tool_api.py (2)
248-271: Filtering archived servers and always including active Kiln tasks looks good
- Skipping archived servers in available_tools is correct.
- Always appending KilnTask tools after the archive check is consistent.
495-501: Correct propagation of archive state to server propertiesIncluding
is_archivedin properties for remote/local MCP and Kiln task servers is correct and consistent with listing/description usage.Also applies to: 555-561, 593-601
libs/core/kiln_ai/tools/test_mcp_server_tool.py (1)
31-35: Test fixtures updated consistently withis_archivedLooks good and consistent across test cases.
Also applies to: 64-68, 94-98, 120-124, 174-178, 200-204, 236-239, 370-373, 420-423
libs/core/kiln_ai/tools/test_tool_registry.py (1)
92-98: Test fixtures aligned with archive flagConsistent additions; no further changes needed.
Also applies to: 124-130, 165-169, 616-620, 741-743
libs/core/kiln_ai/tools/test_mcp_session_manager.py (1)
35-36: Consistent inclusion of is_archived across fixtures — LGTMThe additions keep tests aligned with the new model shape and backfill validator. No issues.
Would we want MCPSessionManager to actively prevent creating sessions for archived servers (defense-in-depth)? If yes, I can add a small test asserting a ValueError when properties.is_archived is True.
Also applies to: 56-57, 562-563, 621-622, 667-668, 711-713, 770-771, 855-856, 949-951, 989-990, 1004-1005, 1150-1151, 1221-1222, 1524-1525, 1550-1551
libs/core/kiln_ai/datamodel/test_external_tool_server.py (1)
32-35: is_archived defaults/backfill in test data — LGTMFixtures and assertions uniformly include is_archived. Backfill coverage looks good.
Also applies to: 57-58, 66-68, 88-90, 98-99, 123-124, 167-168, 187-188, 219-220, 247-248, 278-279, 295-296, 360-361, 388-389, 546-547, 571-572, 617-618, 670-671, 765-766, 792-793, 1066-1067, 1013-1014, 1029-1030
app/web_ui/src/routes/(app)/settings/manage_tools/[project_id]/tool_servers/[tool_server_id]/+page.svelte (1)
261-292: Confirm backend accepts is_archived in edit_remote_mcp/edit_local_mcp PATCH bodiesConfirmed — both endpoints accept request models with is_archived and apply tool_data.is_archived to saved properties; API schema and tests also include is_archived.
...c/routes/(app)/settings/manage_tools/[project_id]/add_tools/local_mcp/edit_local_tool.svelte
Outdated
Show resolved
Hide resolved
...routes/(app)/settings/manage_tools/[project_id]/add_tools/remote_mcp/edit_remote_tool.svelte
Outdated
Show resolved
Hide resolved
What does this PR do?
Previously users can only delete a tool server, which is problematic when a tool server is used in many of the run-configs. This PR adds the ability to archive a tool server so user could easily re-enable one.
Checklists
Summary by CodeRabbit
New Features
Refactor
Tests