Skip to content

[18.0] [MIG] ai_oca_mcp#76

Open
angelmoya wants to merge 7 commits into
OCA:18.0from
angelmoya:18.0-mig-ai_oca_mcp
Open

[18.0] [MIG] ai_oca_mcp#76
angelmoya wants to merge 7 commits into
OCA:18.0from
angelmoya:18.0-mig-ai_oca_mcp

Conversation

@angelmoya

Copy link
Copy Markdown
Member

Migration to 18.0

No major changes.

Depends on #69

@angelmoya angelmoya force-pushed the 18.0-mig-ai_oca_mcp branch from e54dedd to 5ac6fe0 Compare June 5, 2026 12:13
@oliverg09

Copy link
Copy Markdown

Hello,

I've been reviewing this PR and noticed some behavior that might be worth looking into when an MCP key expires. In that case, the exception AttributeError: ‘function’ object has no attribute ‘clear_cache’ is raised.

It seems to be related to line 31 of the file models/mcp_server_key.py.

Since I’m not yet a member of the organization, I can’t contribute directly. To avoid holding things up, I’d really appreciate it if you could take a look at it when you have a moment. Of course, if you’d rather wait, that’s no problem: as soon as I have permissions, I’ll be happy to push the change and help out.

Thank you very much for your time.

@angelmoya angelmoya mentioned this pull request Jun 23, 2026

@petrus-v petrus-v left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authentication mechanisms sees pretty coupled to the mcp endpoint that I've the feeling it will be hard to overload it to implement an other one

def _hash_key(self, key):
return sha256(key.encode()).hexdigest()

@tools.ormcache("key", "security_key")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we invalidate cache when expiration_date change?

.sudo()
._get_mcp_server_by_key(key, security_key)
)
if expiration_date and expiration_date < fields.Datetime.now():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the interest of this condition because _get_mcp_server_by_key already filter expired server key. Do I miss a case? or is it because the cache is not invalidated when expiration_date changed?


class McpController(http.Controller):
@http.route(
"/mcp/<string:key>", type="http", auth="none", methods=["POST"], csrf=False

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm would suggested to create a dedicated auth="ai_mcp" and de-coupled authentication and business logic even it generate an extra request.

._get_mcp_server_by_key(key, security_key)
)
if expiration_date and expiration_date < fields.Datetime.now():
request.env["mcp.server.key"].sudo().browse(server_id).expire_key()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think will never been called because the filter, I would separate the way how the expire state is changed, using ir.cron ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably called if the key come from cache

"error": {"code": -32000, "message": "Connection failed"},
}
)
server = request.env["mcp.server.key"].sudo().browse(server_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename this variable to something like mcp_server_key as you have mcp.server it's a bit confusing

Suggested change
server = request.env["mcp.server.key"].sudo().browse(server_id)
mcp_server_key = request.env["mcp.server.key"].sudo().browse(server_id)

@petrus-v petrus-v Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in this commit: 0ef5ad3 (in migratoin PR to v17)

"id": payload.get("id"),
"result": {
"protocolVersion": "2025-03-26",
"capabilities": {"tools": {"listChanged": True}},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen any call to notified clients, do I miss something or mis understand th protocal, As far I understand this listChanded capabilities indicate the client the it will be notified when tools list result would changed

Suggested change
"capabilities": {"tools": {"listChanged": True}},
"capabilities": {"tools": {}},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done by d621868 (in migratoin PR to v17)

name = fields.Char()
description = fields.Text()
active = fields.Boolean(default=True)
key = fields.Char(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non locking point: Idea of improvement, add api.constraints to ensure it's a valid url path

Comment on lines +90 to +93
"result": {
"structuredContent": result,
"content": [{"type": "text", "text": json.dumps(result)}],
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems resultType is required in result

Suggested change
"result": {
"structuredContent": result,
"content": [{"type": "text", "text": json.dumps(result)}],
},
"result": {
"resultType": "complete",
"structuredContent": result,
"content": [{"type": "text", "text": json.dumps(result)}],
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can ignore this message, I've linked the draft version of the protocol

@petrus-v

Copy link
Copy Markdown

just realize this is a migration PR so I guess those comment are valid in version 16.0 too.

I'm on the way to migrate this module from v16.0 to v17.0 I'll probably create the PR on top of it to make easier back/forward ports

hashed_key = fields.Char(copy=False)
state = fields.Selection(
[("active", "Active"), ("expired", "Expired")], default="active"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using field selection for those 2 states I'm wondering why we are not rely on Odoo active bool feature ? that we could use those state later for an other purpose different than active or archvied

Comment on lines +16 to +21
key = fields.Char(
required=True,
copy=False,
default=lambda self: uuid4(),
groups="base.group_system",
)

@petrus-v petrus-v Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of this field is a bit confusing, mainly because of mcp.server.key model... I would suggested to named it slug / slug path / server path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants