feat-2893: maintain custom and original description for tools#2928
Conversation
jonpspri
left a comment
There was a problem hiding this comment.
Code Review: feat-2893 — maintain custom and original description for tools
Overview
This PR adds an original_description column to the tools table so that when a user customizes a tool's description, the original description from the upstream MCP server is preserved. It includes an Alembic migration, ORM model change, schema change, service layer updates, and test updates.
Issues
1. Critical: original_description is overwritten on every update (defeats the purpose)
In tool_service.py:1557, during bulk import with conflict_strategy="update":
existing_tool.original_description = tool.descriptionThis overwrites the original description with the new incoming description on every update. The same happens in the gateway re-sync path at gateway_service.py:4140 — only description is updated, but original_description is never set on re-sync, meaning it won't track upstream changes either.
The core semantic question this PR needs to answer: Should original_description capture the description as first registered, or should it track the upstream MCP server's current description? The current implementation does neither reliably:
- On initial registration:
original_description = tool.description✓ - On bulk update:
original_description = tool.description(overwrites with new value — wrong for both semantics) - On
update_tool()(line 3912):original_descriptionis never updated (correct if "original" means "first registered", wrong if it means "upstream current") - On gateway re-sync (line 4140):
original_descriptionis not updated (same ambiguity)
Recommendation: For bulk update and gateway re-sync, do NOT overwrite original_description — it should remain as the value from initial registration. Only update it if it's None (backfill scenario).
2. Critical: Migration down_revision uses a suspicious revision ID
The migration's down_revision is c1c2c3c4c5c6 — this looks like a placeholder/test value, not a real Alembic revision ID. While a file with that name does exist in the versions directory, this sequential hex pattern is unusual and should be verified. If this doesn't match the actual current head on main, the migration chain will break.
alembic heads reports 8a16a77260f0 as the single head, so the chain appears intact on this branch, but verify it merges cleanly with main.
3. Medium: ToolUpdate schema doesn't include original_description
Only ToolRead got the new field. ToolUpdate (the Pydantic schema for updating tools) doesn't expose original_description. This is probably correct behavior (users shouldn't be able to overwrite the original), but it should be an explicit design decision. Consider whether an admin should be able to correct it.
4. Medium: Import/export round-trip inconsistency
export_service.py exports original_description, but there's no corresponding handling in the import path to consume it. If someone exports tools and re-imports them, the original_description will either be lost or set to the current description value.
5. Low: Test values don't match the feature semantics
Several tests set original_description to different values than description (e.g., description="A test tool", original_description="A test tool original"), which is good for verifying they're stored separately. However, test_update_tool at line 1574 asserts original_description == "A test tool original" after an update — this is testing that original_description is preserved, which is the correct behavior. But the production code at line 1557 would actually overwrite it on bulk update, contradicting this test's assumptions.
6. Low: No UI or API documentation
No router changes expose original_description for display or management in the admin UI. The field will be returned in API responses via ToolRead, but there's no way for users to see/use the original vs. custom description distinction in the UI.
Code Quality
- The migration follows the idempotent pattern correctly (checks for table/column existence)
- The data backfill in the migration (
UPDATE tools SET original_description = description) is good - Column placement in
db.pyis logical (right afterurl, beforedescription) - Test coverage is thorough for the new field
Summary
The main concern is the semantic inconsistency in how original_description is handled across create vs. update paths. The bulk update path at line 1557 should not overwrite original_description — it should preserve the value from initial registration. Similarly, the gateway re-sync path should decide whether to track upstream changes in original_description or leave it as the initial value.
Here is my response:
|
When tools are registered, preserve the original description from the source MCP server in a new original_description field. This allows users to customize the tool description while retaining the original for reference. - Add original_description column to Tool model and ToolRead schema - Populate original_description at tool registration time - Include original_description in export service output - Add Alembic migration with data backfill from existing descriptions Closes IBM#2893 Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
7e56832 to
2cb3b01
Compare
crivetimihai
left a comment
There was a problem hiding this comment.
Rebased onto main, squashed to a single commit, and fixed the following issues:
-
Alembic migration
down_revisionpointed to nonexistentc1c2c3c4c5c6— updated toba202ac1665f(current head). Without this fix,alembic headswould show multiple heads and break migrations. -
Missing
original_descriptionin_create_db_tool()(gateway_service.py) — tools created during gateway health-check refresh would haveoriginal_description = NULL. Addedoriginal_description=tool.descriptionto the factory method. -
Bulk import overwrote
original_description(tool_service.py) — whenconflict_strategy="update", the code unconditionally resetoriginal_descriptionto the import's description, defeating the write-once semantics. Changed to only set when NULL:if getattr(existing_tool, "original_description", None) is None.
All unit tests pass. Feature design is consistent with the existing original_name/custom_name pattern.
…riginal_description - Protect user-customized descriptions during gateway sync/refresh by only overwriting description when it matches original_description - Add default value (None) to ToolRead.original_description for cache compatibility across deployments - Add original_description to tool cache payload for consistency - Restore original_description during export/import cycle - Update tests to cover description preservation behavior Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
…al_name - Use Tool.original_name (not computed Tool.name) for restore lookup - Generate import_batch_id to scope restore to newly created tools only - Skip restore when no tools were created (respects skip/fail semantics) - Add tests for restore-by-batch and skip-no-restore behaviors - Fix missing original_description on gateway service test mock Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
db.flush() without a subsequent commit loses the restore changes since register_tools_bulk already committed its own transaction. Verified with E2E testing against running docker-compose cluster. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
* feat: add original_description column to tools table (#2893) When tools are registered, preserve the original description from the source MCP server in a new original_description field. This allows users to customize the tool description while retaining the original for reference. - Add original_description column to Tool model and ToolRead schema - Populate original_description at tool registration time - Include original_description in export service output - Add Alembic migration with data backfill from existing descriptions Closes #2893 Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: preserve custom descriptions during gateway refresh and harden original_description - Protect user-customized descriptions during gateway sync/refresh by only overwriting description when it matches original_description - Add default value (None) to ToolRead.original_description for cache compatibility across deployments - Add original_description to tool cache payload for consistency - Restore original_description during export/import cycle - Update tests to cover description preservation behavior Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: scope import original_description restore by batch ID and original_name - Use Tool.original_name (not computed Tool.name) for restore lookup - Generate import_batch_id to scope restore to newly created tools only - Skip restore when no tools were created (respects skip/fail semantics) - Add tests for restore-by-batch and skip-no-restore behaviors - Fix missing original_description on gateway service test mock Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: commit (not flush) import original_description restore db.flush() without a subsequent commit loses the restore changes since register_tools_bulk already committed its own transaction. Verified with E2E testing against running docker-compose cluster. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
* feat: add original_description column to tools table (#2893) When tools are registered, preserve the original description from the source MCP server in a new original_description field. This allows users to customize the tool description while retaining the original for reference. - Add original_description column to Tool model and ToolRead schema - Populate original_description at tool registration time - Include original_description in export service output - Add Alembic migration with data backfill from existing descriptions Closes #2893 Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: preserve custom descriptions during gateway refresh and harden original_description - Protect user-customized descriptions during gateway sync/refresh by only overwriting description when it matches original_description - Add default value (None) to ToolRead.original_description for cache compatibility across deployments - Add original_description to tool cache payload for consistency - Restore original_description during export/import cycle - Update tests to cover description preservation behavior Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: scope import original_description restore by batch ID and original_name - Use Tool.original_name (not computed Tool.name) for restore lookup - Generate import_batch_id to scope restore to newly created tools only - Skip restore when no tools were created (respects skip/fail semantics) - Add tests for restore-by-batch and skip-no-restore behaviors - Fix missing original_description on gateway service test mock Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: commit (not flush) import original_description restore db.flush() without a subsequent commit loses the restore changes since register_tools_bulk already committed its own transaction. Verified with E2E testing against running docker-compose cluster. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
* feat: add original_description column to tools table (IBM#2893) When tools are registered, preserve the original description from the source MCP server in a new original_description field. This allows users to customize the tool description while retaining the original for reference. - Add original_description column to Tool model and ToolRead schema - Populate original_description at tool registration time - Include original_description in export service output - Add Alembic migration with data backfill from existing descriptions Closes IBM#2893 Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: preserve custom descriptions during gateway refresh and harden original_description - Protect user-customized descriptions during gateway sync/refresh by only overwriting description when it matches original_description - Add default value (None) to ToolRead.original_description for cache compatibility across deployments - Add original_description to tool cache payload for consistency - Restore original_description during export/import cycle - Update tests to cover description preservation behavior Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: scope import original_description restore by batch ID and original_name - Use Tool.original_name (not computed Tool.name) for restore lookup - Generate import_batch_id to scope restore to newly created tools only - Skip restore when no tools were created (respects skip/fail semantics) - Add tests for restore-by-batch and skip-no-restore behaviors - Fix missing original_description on gateway service test mock Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: commit (not flush) import original_description restore db.flush() without a subsequent commit loses the restore changes since register_tools_bulk already committed its own transaction. Verified with E2E testing against running docker-compose cluster. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
resolves new feature request #2893
closes #2893