BRIC-18: Add MCP tool docstring warnings and simplify publish_section_version - #38
Conversation
…tion_version - Add IMPORTANT docstring warnings to all 3 MCP tools telling LLMs not to pass hallucinated parameters (limit, offset, etc.) - Simplify publish_section_version: remove section_key, workflow_id, workflow_name, workflow_metadata from public params (now hardcoded internally) to reduce LLM hallucination surface area - Add trailing newline
…nature - Remove section_key, workflow_id, workflow_name, workflow_metadata from test calls to match the new simplified signature - Assert hardcoded defaults are passed to use_case.execute
Code Review — PR #38📊 Score : 8/10✅ Points positifs1. Objectif clair et bien exécuté 2. Docstrings IMPORTANT bien pensées 3. Hardcoding au bon endroit 4. Tests bien mis à jour
|
Kaiohz
left a comment
Kaiohz
left a comment
There was a problem hiding this comment.
See inline comments for specific suggestions.
| workflow_metadata: dict | None = None, | ||
| content: dict | ||
| ) -> dict: | ||
| """Publie la réponse structurée d'une section d'un projet Bricks. |
There was a problem hiding this comment.
📌 Ce hardcoding est la bonne décision pour réduire la surface d'erreur LLM, mais un commentaire expliquant pourquoi serait utile pour les devs futurs :
# Hardcoded: MCP tool only publishes consolidated data sections.
# These params are internal implementation details, not caller-facing.| workflow_id: str = "agent-haiku-files-v1", | ||
| workflow_name: str = "agent-haiku-files-v1", | ||
| workflow_metadata: dict | None = None, | ||
| content: dict |
There was a problem hiding this comment.
💡 L'exemple (e.g., limit, offset, etc.) est générique. Pour ce tool spécifiquement, mentionner les params retirés serait plus précis pour le LLM :
IMPORTANT: This tool ONLY accepts project_unique_id and content.
Do NOT pass section_key, workflow_id, workflow_name, or workflow_metadata — they are handled internally and are not supported as parameters.
| await publish_section_version( | ||
| project_unique_id="proj-abc", | ||
| section_key="summary", | ||
| content="Summary content", |
There was a problem hiding this comment.
Le nom du test test_forward_all_params_to_use_case est devenu trompeur — il ne forward plus "all params" mais vérifie les defaults hardcodés. Suggestion : renommer en test_forward_params_with_hardcoded_defaults.
…e ownership Address PR #38 review (Major #1) — make composable-agents the single owner of the mcp_servers table schema: - Remove McpRegistryStore.ensure_schema() and the _CREATE_TABLE_SQL constant: the table is created and evolved by composable-agents' Alembic migrations (008/009). Keeping a second CREATE TABLE IF NOT EXISTS here was the dual ownership the review flagged as a drift risk. - Remove ensure_schema from the McpServerRegistryStore port and the startup call in main.py; drop the TestEnsureSchema test class. - Rewrite mcp_registry_store.py module docstring: the store is a consumer of the shared table and assumes it exists (deployment prerequisite: composable- agents migrations must run first on a fresh database). - README: correct the MCP Server Registry section to state composable-agents owns the table schema; rewrite the 'Database Migrations' section which falsely claimed Alembic auto-runs at startup with a raganything_alembic_ version table and a 001_add_bm25_support migration — none of that exists. Document the real schema provisioning (composable-agents migrations for mcp_servers, langchain-postgres runtime creation for classical_rag_*, on- demand BM25 index). Remove the fictional alembic/ block from the project structure tree. - Remove the orphan src/alembic.ini (no alembic directory exists). - Clean stale 'alembic/env.py' references in logging.py docstrings.
mcp-raganything now owns the mcp_servers table schema end-to-end (service + table), resolving the ownership split flagged in the composable-agents PR #38 review. Previously the schema was created by composable-agents' Alembic migrations 008/009 while everything else MCP-related lived here — an incoherent split. - Add alembic dependency; bootstrap src/alembic/ (env.py with async engine, version_table='raganything_alembic_version' to avoid collisions with composable-agents' alembic_version on the shared DB; script.py.mako). - Add 001_create_mcp_servers_table: full squashed schema (was 008+009 in composable-agents), CREATE TABLE IF NOT EXISTS so it's safe on DBs where the table was previously created by the now-removed legacy migrations. - main.py: _run_alembic_upgrade() runs migrations to head at startup (asyncio.to_thread), unconditionally, before the registry pool is built. - Update mcp_registry_store.py + port docstrings: table is owned by this service's migrations. - README: Database Schema section now documents the real Alembic setup; restore the alembic/ block in the project tree; MCP Server Registry section states mcp-raganything owns service + schema.
…her (#59) * feat: MCP server registry — CRUD API, OpenAPI MCP factory, Fernet cipher - MCP server registry store (Postgres) with CRUD use cases - OpenAPI MCP factory (fastmcp) for dynamic MCP server generation - Fernet secret cipher for encrypting auth tokens at rest - Swagger 2.0 to OpenAPI 3.0 converter - MCP registry routes (/api/v1/mcp/servers, /api/v1/mcp/servers/{name}/reveal) - Startup rehydration of registered MCP servers - Full test suite: 472 tests passing * Remove continue-on-error from SonarQube step * refactor(review): drop ensure_schema, document composable-agents table ownership Address PR #38 review (Major #1) — make composable-agents the single owner of the mcp_servers table schema: - Remove McpRegistryStore.ensure_schema() and the _CREATE_TABLE_SQL constant: the table is created and evolved by composable-agents' Alembic migrations (008/009). Keeping a second CREATE TABLE IF NOT EXISTS here was the dual ownership the review flagged as a drift risk. - Remove ensure_schema from the McpServerRegistryStore port and the startup call in main.py; drop the TestEnsureSchema test class. - Rewrite mcp_registry_store.py module docstring: the store is a consumer of the shared table and assumes it exists (deployment prerequisite: composable- agents migrations must run first on a fresh database). - README: correct the MCP Server Registry section to state composable-agents owns the table schema; rewrite the 'Database Migrations' section which falsely claimed Alembic auto-runs at startup with a raganything_alembic_ version table and a 001_add_bm25_support migration — none of that exists. Document the real schema provisioning (composable-agents migrations for mcp_servers, langchain-postgres runtime creation for classical_rag_*, on- demand BM25 index). Remove the fictional alembic/ block from the project structure tree. - Remove the orphan src/alembic.ini (no alembic directory exists). - Clean stale 'alembic/env.py' references in logging.py docstrings. * feat(alembic): move mcp_servers table ownership to mcp-raganything mcp-raganything now owns the mcp_servers table schema end-to-end (service + table), resolving the ownership split flagged in the composable-agents PR #38 review. Previously the schema was created by composable-agents' Alembic migrations 008/009 while everything else MCP-related lived here — an incoherent split. - Add alembic dependency; bootstrap src/alembic/ (env.py with async engine, version_table='raganything_alembic_version' to avoid collisions with composable-agents' alembic_version on the shared DB; script.py.mako). - Add 001_create_mcp_servers_table: full squashed schema (was 008+009 in composable-agents), CREATE TABLE IF NOT EXISTS so it's safe on DBs where the table was previously created by the now-removed legacy migrations. - main.py: _run_alembic_upgrade() runs migrations to head at startup (asyncio.to_thread), unconditionally, before the registry pool is built. - Update mcp_registry_store.py + port docstrings: table is owned by this service's migrations. - README: Database Schema section now documents the real Alembic setup; restore the alembic/ block in the project tree; MCP Server Registry section states mcp-raganything owns service + schema. * fix(review): address PR #59 blockers and should-fix items Blockers (🔴): - Preserve created_at on PUT: UpdateMcpServerUseCase merges created_at=existing.created_at, updated_at=now before persisting (external + openapi paths). Regression tests added. - Remove /generated/pending sentinel: RegisteredMcpServer.url is now str | None; dropped validate_url_required and _OPENAPI_PENDING_URL. Mounted URL set by use case before save (DB NOT NULL respected). - E2E security test: exercise real McpApiKeyMiddleware via the runner — tools/list and tools/call rejected (ToolError) without/bad key, allowed with good key, bypassed when master_key empty. - Rehydration: emit ERROR-level summary log when skipped > 0 (+ tests). Should-fix (🟡): - Delete use case: unconditional runner.unmount (no-op for absent names), drop the secret-decrypting store.get round-trip. - updated_at = now() server-side in the upsert ON CONFLICT clause. - Document auth_token null ambiguity in masked responses (/reveal). - OPENAPI_MAX_SPEC_BYTES in AppConfig, injected into the factory. - Shared _McpServerBaseRequest for create/update/validate requests. - Unify test class naming (TestFernetSecretCipherConstructor -> Test). - Strip stale TDD-Red comments from 9 test modules. Tests: 481 passed. Ruff/mypy: no new errors.
1 participant
Jira
BRIC-18
Changes
Related: SoluDevTech/composable-agents#24