-
Notifications
You must be signed in to change notification settings - Fork 36
feat(core): optional intent label on interaction tools (#894) #915
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
Changes from all commits
6d61ad0
7c23126
9605a89
e9756e5
13b083c
b8677ff
e8b81d6
a7ca90d
fab715e
b5b1e72
00646c1
0275395
34dec2d
3d51a5f
fa0851f
2ec45a1
0fb76fa
fba09a6
4dbd67b
75fa907
6e406e0
61fa8b6
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 |
|---|---|---|
|
|
@@ -329,25 +329,6 @@ export interface MCPServerOptions { | |
| initialToolTier?: ToolTier; | ||
| } | ||
|
|
||
|
|
||
| export function summarizeMcpResultForJournal(result: MCPResult): string | undefined { | ||
| const content = result.content; | ||
| if (!Array.isArray(content)) return undefined; | ||
| const injectedHint = typeof (result as Record<string, unknown>)._hint === 'string' | ||
| ? String((result as Record<string, unknown>)._hint).trim() | ||
| : undefined; | ||
| const text = content | ||
| .map((part) => (part && part.type === 'text' ? part.text : '')) | ||
| .filter((textPart) => { | ||
| if (!textPart) return false; | ||
| return injectedHint === undefined || textPart.trim() !== injectedHint; | ||
| }) | ||
| .join(' ') | ||
| .replace(/\s+/g, ' ') | ||
| .trim(); | ||
| return text ? text.slice(0, 500) : undefined; | ||
| } | ||
|
|
||
| export class MCPServer { | ||
| private tools: Map<string, ToolRegistry> = new Map(); | ||
| private resources: Map<string, MCPResourceDefinition> = new Map(); | ||
|
|
@@ -2034,24 +2015,6 @@ export class MCPServer { | |
| // when --secrets was not passed. | ||
| const finalResult = redactSecrets(result); | ||
| this.recordToolOutputObservability(toolName, finalResult); | ||
|
|
||
| // Record to task journal after response redaction so arbitrary literal | ||
| // secret values cannot be persisted in journal result summaries. | ||
| try { | ||
| const journal = getTaskJournal(); | ||
| const entry = journal.createEntry( | ||
| toolName, | ||
| sessionId, | ||
| toolArgs, | ||
| Date.now() - toolStartTime, | ||
| !(finalResult as MCPResult).isError, | ||
| summarizeMcpResultForJournal(finalResult as MCPResult), | ||
| ); | ||
| journal.record(entry); | ||
| } catch { | ||
| // Best-effort journal recording | ||
| } | ||
|
|
||
| return finalResult; | ||
|
Comment on lines
2016
to
2018
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.
Returning immediately here removes the only journal path that derived Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
| const message = formatError(error); | ||
|
|
@@ -2095,14 +2058,7 @@ export class MCPServer { | |
| // Record to task journal | ||
| try { | ||
| const journal = getTaskJournal(); | ||
| const entry = journal.createEntry( | ||
| toolName, | ||
| sessionId, | ||
| telemetryToolArgs, | ||
| Date.now() - toolStartTime, | ||
| false, | ||
| redactedMessage, | ||
| ); | ||
| const entry = journal.createEntry(toolName, sessionId, telemetryToolArgs, Date.now() - toolStartTime, false); | ||
| journal.record(entry); | ||
| } catch { | ||
| // Best-effort journal recording | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ interface Position { | |
|
|
||
| const definition: MCPToolDefinition = { | ||
| name: 'drag_drop', | ||
| description: 'Drag and drop by selector or coordinates.', | ||
| description: 'Drag and drop by selector or coordinates. Pass intent="..." (≤120 chars) to label this action in audit logs.', | ||
| inputSchema: { | ||
| type: 'object', | ||
| properties: { | ||
|
|
@@ -53,6 +53,11 @@ const definition: MCPToolDefinition = { | |
| type: 'number', | ||
| description: 'Delay in ms between steps. Default: 10', | ||
| }, | ||
| intent: { | ||
| type: 'string', | ||
| description: 'Human-readable label for this action in audit logs (≤120 chars)', | ||
| maxLength: 120, | ||
| }, | ||
| }, | ||
| required: ['tabId'], | ||
| }, | ||
|
|
@@ -72,6 +77,23 @@ const handler: ToolHandler = async ( | |
| const targetY = args.targetY as number | undefined; | ||
| const steps = (args.steps as number | undefined) ?? 10; | ||
| const delay = (args.delay as number | undefined) ?? 10; | ||
| const intent = args.intent as string | undefined; | ||
|
|
||
| // Validate intent when provided — use typeof guard for null-safety | ||
| if (typeof intent === 'string') { | ||
| if (intent === '') { | ||
| return { | ||
| content: [{ type: 'text', text: 'INVALID_INTENT: intent must not be an empty string' }], | ||
| isError: true, | ||
| }; | ||
|
Comment on lines
+84
to
+88
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.
The new Useful? React with 👍 / 👎. |
||
| } | ||
| if (intent.length > 120) { | ||
| return { | ||
| content: [{ type: 'text', text: `INVALID_INTENT: intent exceeds 120 characters (got ${intent.length})` }], | ||
|
Comment on lines
+90
to
+92
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.
Useful? React with 👍 / 👎. |
||
| isError: true, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const sessionManager = getSessionManager(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,7 +59,7 @@ export interface UploadPathValidationResult { | |
|
|
||
| const definition: MCPToolDefinition = { | ||
| name: 'file_upload', | ||
| description: 'Upload files to a file input element on the page.', | ||
| description: 'Upload files to a file input element on the page. Pass intent="..." (≤120 chars) to label this action in audit logs.', | ||
| inputSchema: { | ||
| type: 'object', | ||
| properties: { | ||
|
|
@@ -76,6 +76,11 @@ const definition: MCPToolDefinition = { | |
| items: { type: 'string' }, | ||
| description: 'File paths to upload. Paths must resolve under configured file_upload roots.', | ||
| }, | ||
| intent: { | ||
| type: 'string', | ||
| description: 'Human-readable label for this action in audit logs (≤120 chars)', | ||
| maxLength: 120, | ||
| }, | ||
| }, | ||
| required: ['tabId', 'selector', 'filePaths'], | ||
| }, | ||
|
|
@@ -244,6 +249,23 @@ const handler: ToolHandler = async ( | |
| const tabId = args.tabId as string; | ||
| const selector = args.selector as string; | ||
| const filePaths = args.filePaths as string[]; | ||
| const intent = args.intent as string | undefined; | ||
|
|
||
| // Validate intent when provided — use typeof guard for null-safety | ||
| if (typeof intent === 'string') { | ||
| if (intent === '') { | ||
| return { | ||
| content: [{ type: 'text', text: 'INVALID_INTENT: intent must not be an empty string' }], | ||
| isError: true, | ||
| }; | ||
|
Comment on lines
+257
to
+260
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.
Returning Useful? React with 👍 / 👎. |
||
| } | ||
| if (intent.length > 120) { | ||
|
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.
Here Useful? React with 👍 / 👎. |
||
| return { | ||
| content: [{ type: 'text', text: `INVALID_INTENT: intent exceeds 120 characters (got ${intent.length})` }], | ||
| isError: true, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const sessionManager = getSessionManager(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,15 +19,10 @@ import { humanType, humanMouseMove } from '../stealth/human-behavior'; | |
| import { detectLoginOutcome, LoginDetectResult } from './login-detector'; | ||
| import { wrapMutatingHandler } from '../utils/snapshot-cache-helper'; | ||
| import { coerceVerifyMode, runVerify, VERIFY_FIELD_SCHEMA } from '../core/perception/verify'; | ||
| import { | ||
| appendReturnAfterState, | ||
| parseReturnAfterState, | ||
| RETURN_AFTER_STATE_SCHEMA, | ||
| } from './_shared/return-after-state'; | ||
|
|
||
| const definition: MCPToolDefinition = { | ||
| name: 'fill_form', | ||
| description: 'Fill form fields and optionally submit.', | ||
| description: 'Fill form fields and optionally submit. Pass intent="..." (≤120 chars) to label this action in audit logs.', | ||
| inputSchema: { | ||
| type: 'object', | ||
| properties: { | ||
|
|
@@ -72,7 +67,11 @@ const definition: MCPToolDefinition = { | |
| additionalProperties: { type: 'string' }, | ||
| }, | ||
| verify: VERIFY_FIELD_SCHEMA, | ||
| returnAfterState: RETURN_AFTER_STATE_SCHEMA, | ||
| intent: { | ||
| type: 'string', | ||
| description: 'Human-readable label for this action in audit logs (≤120 chars)', | ||
| maxLength: 120, | ||
| }, | ||
| }, | ||
| required: ['tabId'], | ||
| }, | ||
|
|
@@ -94,7 +93,23 @@ const handler: ToolHandler = async ( | |
| const pollInterval = Math.min(Math.max((args.pollInterval as number) || 300, 50), 2000); | ||
| const loginCheck: 'auto' | 'off' = (args.loginCheck === 'off') ? 'off' : 'auto'; | ||
| const verifyMode = coerceVerifyMode(args.verify); | ||
| const returnAfterState = parseReturnAfterState(args.returnAfterState); | ||
| const intent = args.intent as string | undefined; | ||
|
|
||
| // Validate intent when provided — use typeof guard for null-safety | ||
| if (typeof intent === 'string') { | ||
| if (intent === '') { | ||
| return { | ||
| content: [{ type: 'text', text: 'INVALID_INTENT: intent must not be an empty string' }], | ||
| isError: true, | ||
| }; | ||
|
Comment on lines
+101
to
+104
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.
This validation branch returns a normal MCP result with Useful? React with 👍 / 👎. |
||
| } | ||
| if (intent.length > 120) { | ||
|
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.
The new validation path treats any defined Useful? React with 👍 / 👎. |
||
| return { | ||
| content: [{ type: 'text', text: `INVALID_INTENT: intent exceeds 120 characters (got ${intent.length})` }], | ||
| isError: true, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const sessionManager = getSessionManager(); | ||
|
|
||
|
|
@@ -627,7 +642,7 @@ const handler: ToolHandler = async ( | |
| if (detectorFailedLogin) errorReason = 'login_failed'; | ||
| else if (submitFailed) errorReason = 'submit_failed'; | ||
|
|
||
| const fillFormResult: MCPResult = { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
|
Comment on lines
+645
to
648
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.
Useful? React with 👍 / 👎. |
||
|
|
@@ -643,14 +658,6 @@ const handler: ToolHandler = async ( | |
| : {}), | ||
| ...(fillVerifyReport ? { verify: fillVerifyReport } : {}), | ||
| } as MCPResult; | ||
| // Snapshot capture happens after the post-action wait inside withDomDelta | ||
| // (and the optional login-detector poll), so the snapshot reflects the | ||
| // post-action DOM. Only attach on non-error results — when fill_form | ||
| // failed there is no point paying for an extra snapshot. | ||
| if (!isError) { | ||
| await appendReturnAfterState(fillFormResult, page, sessionId, tabId, returnAfterState, context); | ||
| } | ||
| return fillFormResult; | ||
| } catch (error) { | ||
| return { | ||
| content: [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,11 +7,10 @@ import { MCPToolDefinition, MCPResult, ToolHandler, ToolContext, hasBudget } fro | |
| import { getSessionManager } from '../session-manager'; | ||
| import { getRefIdManager, formatStaleRefError, makeStaleRefError } from '../utils/ref-id-manager'; | ||
| import { withDomDelta } from '../utils/dom-delta'; | ||
| import { wrapMutatingHandler } from '../utils/snapshot-cache-helper'; | ||
|
|
||
| const definition: MCPToolDefinition = { | ||
| name: 'form_input', | ||
| description: 'Set one form element value by ref.\n\nWhen to use: Filling a single known input, textarea, select, or checkbox by ref.\nWhen NOT to use: Use fill_form({fields:{...}}) for multiple fields or optional submit.', | ||
| description: 'Set one form element value by ref. Pass intent="..." (≤120 chars) to label this action in audit logs.\n\nWhen to use: Filling a single known input, textarea, select, or checkbox by ref.\nWhen NOT to use: Use fill_form({fields:{...}}) for multiple fields or optional submit.', | ||
| inputSchema: { | ||
| type: 'object', | ||
| properties: { | ||
|
|
@@ -27,6 +26,11 @@ const definition: MCPToolDefinition = { | |
| type: 'string', | ||
| description: 'Value to set. Checkboxes: "true"/"false"', | ||
| }, | ||
| intent: { | ||
| type: 'string', | ||
| description: 'Human-readable label for this action in audit logs (≤120 chars)', | ||
| maxLength: 120, | ||
| }, | ||
| }, | ||
| required: ['ref', 'value', 'tabId'], | ||
| }, | ||
|
|
@@ -40,6 +44,23 @@ const handler: ToolHandler = async ( | |
| const tabId = args.tabId as string; | ||
| const ref = args.ref as string; | ||
| const value = args.value; | ||
| const intent = args.intent as string | undefined; | ||
|
|
||
| // Validate intent when provided — use typeof guard for null-safety | ||
| if (typeof intent === 'string') { | ||
| if (intent === '') { | ||
| return { | ||
| content: [{ type: 'text', text: 'INVALID_INTENT: intent must not be an empty string' }], | ||
| isError: true, | ||
| }; | ||
|
Comment on lines
+52
to
+55
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.
Returning Useful? React with 👍 / 👎. |
||
| } | ||
| if (intent.length > 120) { | ||
|
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.
This branch assumes Useful? React with 👍 / 👎. |
||
| return { | ||
| content: [{ type: 'text', text: `INVALID_INTENT: intent exceeds 120 characters (got ${intent.length})` }], | ||
| isError: true, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const sessionManager = getSessionManager(); | ||
| const refIdManager = getRefIdManager(); | ||
|
|
@@ -433,10 +454,5 @@ const handler: ToolHandler = async ( | |
| }; | ||
|
|
||
| export function registerFormInputTool(server: MCPServer): void { | ||
| // Snapshot-cache (#879): bump docEpoch after every successful set. | ||
| const sm = getSessionManager(); | ||
| const wrapped = wrapMutatingHandler(handler, (sid, tid) => | ||
| tid ? sm.getPage(sid, tid) : Promise.resolve(null), | ||
| ); | ||
| server.registerTool('form_input', wrapped, definition); | ||
| server.registerTool('form_input', handler, definition); | ||
| } | ||
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.
This commit removes
summarizeMcpResultForJournalfromsrc/mcp-server.ts, buttests/tools/journal.test.tsstill imports and executes it (import { MCPServer, summarizeMcpResultForJournal } ...at line 42 and usage at line 68). That leaves the tree in a broken state where type-check/test compilation fails with a missing export, so either the helper must remain exported here or the dependent test/callers must be updated in the same change.Useful? React with 👍 / 👎.