Introduce optional URL argument to all tools #74
Conversation
alistair3149
force-pushed
the
feat-59
branch
from
14ac700 to
6f32455
Compare
alistair3149
force-pushed
the
feat-59
branch
3 times, most recently
from
8190b30 to
5f4f555
Compare
alistair3149
force-pushed
the
feat-59
branch
from
5f4f555 to
bd9f6f9
Compare
Also include the ADR for this decision.
alistair3149
force-pushed
the
feat-59
branch
from
bd9f6f9 to
6febdc4
Compare
| @@ -0,0 +1,54 @@ | |||
| # 1. Use optional URL argument in MCP tool | |||
|
|
|||
| Date: 2025-06-21 | |||
There was a problem hiding this comment.
Is this from exactly 3 months ago or did you typo?
|
I did a quick read-through and did not spot anything to-me-obvious. No deeper review done yet. Several things bottlenecked on me, so ideally @malberts can do this instead |
malberts
left a comment
malberts
left a comment
There was a problem hiding this comment.
The code runs fine. I have some thoughts on the ADR, but they might be nitpicking/hallucinations.
| getPage( 'Main Page' ) | ||
| ``` | ||
|
|
||
| This is cumbersome for users and LLMs, especially in conversations involving multiple wikis where the LLM needs to track the currently active wiki, which can be prone to error. |
There was a problem hiding this comment.
This is cumbersome for users
Why? The LLM can already handle this without the user specifying it repeatedly:
Granted, it does make the UI a bit more cluttered, but I don't think the LLM prompt is any more or less cumbersome.
There was a problem hiding this comment.
Yes the LLM did handle it perfectly in my tests as well, in fact it had never lose track of the state.
Personally I don't think it matters whether it's cumbersome for the user, because these tools are meant to be used by the LLM. The minor thing might be the LLM invoking the set tools which clutters the UI, but it is a minor issue.
There was a problem hiding this comment.
So how did we end up with this sentence in the ADR? Is it AI-generated?
| getPage( 'Main Page' ) | ||
| ``` | ||
|
|
||
| This is cumbersome for users and LLMs, especially in conversations involving multiple wikis where the LLM needs to track the currently active wiki, which can be prone to error. |
There was a problem hiding this comment.
which can be prone to error.
Do we have evidence of multiple tool calls being more error prone? If we're worried about the LLM forgetting to call setWiki(), or calling it with the wrong URL, should we not be equally worried about it calling getPage() without a URL or with a wrong URL?
Or are we worried about our internal implementation somehow losing track of what is going on? (i.e. a bug)
I raise this not to object to the change itself, but to clarify the ADR. My feeling is the original issue really only solves the "problem" of making tools more flexible by not needing pre-configured wikis.
The current ADR wording makes it sound like we have specific problems (UX and error proneness - neither of which I think are necessarily present, or fully "fixed" here), and the solution is to implement someting that is optional and thus allows the original problems to still exist.
Perhaps I was expecting ADRs more along the lines of:
- Decision: tools should be usable without config
- Decision: tools should be stateless
Where this PR is a step in implementing those. And a future step might be to remove the current state completely.
There was a problem hiding this comment.
Perhaps I am confused about the issue itself, it'll be more beneficial if we re-align on this ADR.
Do we have evidence of multiple tool calls being more error prone?
No. I even tried to prompt the LLM to do the multi-wiki operations, and it still defers to the set wiki tool.
Are we worried about our internal implementation somehow losing track of what is going on?
No, that is not the case, the MCP server can maintain the state properly.
Decision: tools should be usable without config
The tools were already usable without the config, since we have a default config, and set wiki tools are always run before any other tools by the LLM.
Decision: tools should be stateless
That is another matter that needs more clarification. The tools are only used by the LLM, in order to be stateless, we would have to rely on the LLM to pass the state every time. I'm not sure if that is a better approach.
Closes: #59