-
Notifications
You must be signed in to change notification settings - Fork 11
Introduce optional URL argument to all tools #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # 1. Use optional URL argument in MCP tool | ||
|
|
||
| Date: 2025-06-21 | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted. | ||
|
|
||
| ## Context | ||
|
|
||
| Currently, switching between wikis requires using the stateful `set-wiki` command. To get pages from two different wikis, one must do: | ||
|
|
||
| ``` | ||
| setWiki( 'foo.example.com' ) | ||
| getPage( 'Main Page' ) | ||
|
|
||
| setWiki( 'bar.example.com' ) | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So how did we end up with this sentence in the ADR? Is it AI-generated?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do we have evidence of multiple tool calls being more error prone? If we're worried about the LLM forgetting to call 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:
Where this PR is a step in implementing those. And a future step might be to remove the current state completely.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seconded
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I am confused about the issue itself, it'll be more beneficial if we re-align on this ADR.
No. I even tried to prompt the LLM to do the multi-wiki operations, and it still defers to the set wiki tool.
No, that is not the case, the MCP server can maintain the state properly.
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.
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. |
||
|
|
||
| The proposed solution is to add an optional URL argument to commands, allowing for stateless calls: | ||
|
|
||
| ``` | ||
| getPage( 'Main Page', 'foo.example.com' ) | ||
| getPage( 'Main Page', 'bar.example.com' ) | ||
| ``` | ||
|
|
||
| This works smoothly for LLMs. If a user's initial prompt is "I have my test wiki at test.example.wiki and production at www.example.wiki", and they later say "copy the pages from the Foo category on my test wiki to production", the LLM can provide the correct arguments for all the calls without invoking extra tool calls, preventing potential errors. | ||
|
|
||
| ## Decision | ||
|
|
||
| An optional `wikiUrl` argument will be added as the last parameter to all MCP tools that interact with a MediaWiki instance. | ||
|
|
||
| * If the `wikiUrl` argument is provided, the tool will target that wiki for the operation. | ||
| * If the argument is omitted, the tool will fall back to the wiki set by `set-wiki`, or a default wiki from the server configuration. | ||
| * The `set-wiki` tool will be retained for now as a convenience for sessions focused on a single wiki. Its long-term utility will be evaluated separately. | ||
|
|
||
| This decision considers the following use cases: | ||
| * **Single, locked-down wiki**: In environments like an internal corporate wiki, the wiki can be fixed in the MCP configuration. In this setup, the optional `wikiUrl` argument should be ignored. | ||
| * **Multiple wikis**: For a general-purpose "wiki helper chatbot" where wikis are not known in advance, this argument is essential. | ||
| * **Ad-hoc single wiki**: While `set-wiki` could serve this case, the optional argument is sufficient and avoids the complexity of maintaining state, especially since a single-wiki conversation can evolve to include multiple wikis. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| * Simplifies interactions involving multiple wikis. | ||
| * Reduces the need for the LLM to track the "current" wiki state. | ||
| * Makes interactions more robust and stateless. | ||
|
|
||
| ### Negative | ||
| * Increases complexity in most tools. Extra code is needed to handle the current wiki. | ||
| * Requires a coordinated change across all relevant tool definitions. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| export class WikiDiscoveryError extends Error { | ||
| public constructor( message: string ) { | ||
| super( message ); | ||
| this.name = 'WikiDiscoveryError'; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| import { makeApiRequest, fetchPageHtml } from './utils.js'; | ||
| import { getAllWikis, updateWikiConfig } from './config.js'; | ||
| import { WikiDiscoveryError } from './errors.js'; | ||
|
|
||
| const COMMON_SCRIPT_PATHS = [ '/w', '' ]; | ||
|
|
||
| // TODO: Move these types to a dedicated file if we end up using Action API types elsewhere | ||
| interface MediaWikiActionApiSiteInfoGeneral { | ||
| sitename: string; | ||
| articlepath: string; | ||
| scriptpath: string; | ||
| server: string; | ||
| servername: string; | ||
| // Omitted other fields for now since we don't use them | ||
| } | ||
|
|
||
| interface MediaWikiActionApiSiteInfoQuery { | ||
| general: MediaWikiActionApiSiteInfoGeneral; | ||
| } | ||
|
|
||
| interface MediaWikiActionApiResponse { | ||
| query?: MediaWikiActionApiSiteInfoQuery; | ||
| } | ||
|
|
||
| export interface WikiInfo { | ||
| sitename: string; | ||
| articlepath: string; | ||
| scriptpath: string; | ||
| server: string; | ||
| servername: string; | ||
| } | ||
|
|
||
| export async function resolveWiki( wikiUrl: string ): Promise<string> { | ||
| const url = new URL( wikiUrl ); | ||
| const allWikis = getAllWikis(); | ||
|
|
||
| if ( allWikis[ url.hostname ] ) { | ||
| return url.hostname; | ||
| } | ||
|
|
||
| const wikiServer = parseWikiUrl( wikiUrl ); | ||
| const wikiInfo = await getWikiInfo( wikiServer, wikiUrl ); | ||
|
|
||
| if ( wikiInfo !== null ) { | ||
| updateWikiConfig( wikiInfo.servername, { | ||
| sitename: wikiInfo.sitename, | ||
| server: wikiInfo.server, | ||
| articlepath: wikiInfo.articlepath, | ||
| scriptpath: wikiInfo.scriptpath | ||
| } ); | ||
| return wikiInfo.servername; | ||
| } else { | ||
| throw new WikiDiscoveryError( 'Failed to determine wiki info. Please ensure the URL is correct and the wiki is accessible.' ); | ||
| } | ||
| } | ||
|
|
||
| export function parseWikiUrl( wikiUrl: string ): string { | ||
| const url = new URL( wikiUrl ); | ||
| return `${ url.protocol }//${ url.host }`; | ||
| } | ||
|
|
||
| export async function getWikiInfo( | ||
| wikiServer: string, originalWikiUrl: string | ||
| ): Promise<WikiInfo | null> { | ||
| return ( await fetchUsingCommonScriptPaths( wikiServer ) ) ?? | ||
| ( await fetchUsingScriptPathsFromHtml( wikiServer, originalWikiUrl ) ); | ||
| } | ||
|
|
||
| async function fetchWikiInfoFromApi( | ||
| wikiServer: string, scriptPath: string | ||
| ): Promise<WikiInfo | null> { | ||
| const baseUrl = `${ wikiServer }${ scriptPath }/api.php`; | ||
| const params = { | ||
| action: 'query', | ||
| meta: 'siteinfo', | ||
| siprop: 'general', | ||
| format: 'json', | ||
| origin: '*' | ||
| }; | ||
|
|
||
| let data: MediaWikiActionApiResponse | null = null; | ||
| try { | ||
| data = await makeApiRequest<MediaWikiActionApiResponse>( baseUrl, params ); | ||
| } catch ( error ) { | ||
| console.error( `Error fetching wiki info from ${ baseUrl }:`, error ); | ||
| return null; | ||
| } | ||
|
|
||
| if ( data === null || data.query?.general === undefined ) { | ||
| return null; | ||
| } | ||
|
|
||
| const general = data.query.general; | ||
|
|
||
| // We don't need to check for every field, the API should be returning the correct values. | ||
| if ( typeof general.scriptpath !== 'string' ) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| sitename: general.sitename, | ||
| scriptpath: general.scriptpath, | ||
| articlepath: general.articlepath.replace( '/$1', '' ), | ||
| server: general.server, | ||
| servername: general.servername | ||
| }; | ||
| } | ||
|
|
||
| async function fetchUsingCommonScriptPaths( | ||
| wikiServer: string | ||
| ): Promise<WikiInfo | null> { | ||
| for ( const candidatePath of COMMON_SCRIPT_PATHS ) { | ||
| const apiResult = await fetchWikiInfoFromApi( wikiServer, candidatePath ); | ||
| if ( apiResult ) { | ||
| return apiResult; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| async function fetchUsingScriptPathsFromHtml( | ||
| wikiServer: string, | ||
| originalWikiUrl: string | ||
| ): Promise<WikiInfo | null> { | ||
| const htmlContent = await fetchPageHtml( originalWikiUrl ); | ||
| const htmlScriptPathCandidates = extractScriptPathsFromHtml( htmlContent, wikiServer ); | ||
| const pathsToTry = htmlScriptPathCandidates.length > 0 ? | ||
| htmlScriptPathCandidates : COMMON_SCRIPT_PATHS; | ||
|
|
||
| for ( const candidatePath of pathsToTry ) { | ||
| const apiResult = await fetchWikiInfoFromApi( wikiServer, candidatePath ); | ||
| if ( apiResult ) { | ||
| return apiResult; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function extractScriptPathsFromHtml( htmlContent: string | null, wikiServer: string ): string[] { | ||
| const candidatesFromHtml: string[] = []; | ||
| if ( htmlContent ) { | ||
| const fromSearchForm = extractScriptPathFromSearchForm( htmlContent, wikiServer ); | ||
| if ( fromSearchForm !== null ) { | ||
| candidatesFromHtml.push( fromSearchForm ); | ||
| } | ||
| } | ||
|
|
||
| const uniqueCandidatesFromHtml = [ ...new Set( candidatesFromHtml ) ]; | ||
| return uniqueCandidatesFromHtml.filter( ( p ) => typeof p === 'string' && ( p === '' || p.trim() !== '' ) ); | ||
| } | ||
|
|
||
| function extractScriptPathFromSearchForm( htmlContent: string, wikiServer: string ): string | null { | ||
| const searchFormMatch = htmlContent.match( /<form[^>]+id=['"]searchform['"][^>]+action=['"]([^'"]*index\.php[^'"]*)['"]/i ); | ||
| if ( searchFormMatch && searchFormMatch[ 1 ] ) { | ||
| const actionAttribute = searchFormMatch[ 1 ]; | ||
| try { | ||
| const fullActionUrl = new URL( actionAttribute, wikiServer ); | ||
| const path = fullActionUrl.pathname; | ||
| const indexPathIndex = path.toLowerCase().lastIndexOf( '/index.php' ); | ||
| if ( indexPathIndex !== -1 ) { | ||
| return path.slice( 0, indexPathIndex ); | ||
| } | ||
| } catch ( error ) { | ||
| console.error( `Error extracting script path from search form: ${ error }` ); | ||
| } | ||
| } | ||
| return null; | ||
| } |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this from exactly 3 months ago or did you typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I used the date on the issue #59