diff --git a/CLAUDE.md b/CLAUDE.md index ae4a7f0657..2978e242aa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -535,7 +535,7 @@ Rule (unchanged): any new social-presence primitive (typing-indicator, read-rece | runtime | `commonly_update_task` does | title? | |---|---|---| - | MCP | *"Append an update note to a task **without changing status**"* → `POST /api/v1/tasks/:podId/:taskId/updates`, params `{podId, taskId, text}` | no — and **no MCP tool wraps a PATCH route at all** | + | MCP | *"Append an update note to a task **without changing status**"* → `POST /api/v1/tasks/:podId/:taskId/updates`, params `{podId, taskId, text, priority?, prUrl?, continueAnyway?}`; optional `prUrl` first PATCHes the task URL | no — the PATCH wrapper exposes `prUrl` only, not `title` | | openclaw extension | *"Patch task fields: assignee, status, dep, prUrl, notes, **title**… For progress notes use `commonly_add_task_update` instead"* → `PATCH /api/v1/tasks/:podId/:taskId` | yes | The backend is not the constraint. `PATCH /:podId/:taskId` lists `title` in its `allowed` array and is gated by `auth` + `requirePodMember(podId, userId, { write: true })` — **the same pair guarding the note-append route beside it**, which every seat hits on every lease renewal. So the authority is identical and only the tool surface differs. diff --git a/commonly-mcp/__tests__/tools.test.mjs b/commonly-mcp/__tests__/tools.test.mjs index 03bb94a12b..4caf2cc176 100644 --- a/commonly-mcp/__tests__/tools.test.mjs +++ b/commonly-mcp/__tests__/tools.test.mjs @@ -357,6 +357,66 @@ describe('commonly_get_tasks / create / claim / complete / update', () => { expect(url).toBe('https://x.example/api/v1/tasks/POD/TASK-001/updates'); expect(JSON.parse(init.body)).toEqual({ text: 'still working' }); }); + + it('update_task saves a normalized PR URL before appending the note', async () => { + const fetchSpy = installFetch(async () => okResponse({ ok: true })); + await byName.commonly_update_task.call({ + podId: 'POD', taskId: 'TASK-001', text: 'PR opened', + prUrl: 'https://github.com/Team-Commonly/commonly/pull/2399?tab=files#diff', + }); + expect(fetchSpy).toHaveBeenCalledTimes(2); + const [[patchUrl, patchInit], [noteUrl, noteInit]] = fetchSpy.mock.calls; + expect(patchUrl).toBe('https://x.example/api/v1/tasks/POD/TASK-001'); + expect(patchInit.method).toBe('PATCH'); + expect(JSON.parse(patchInit.body)).toEqual({ + prUrl: 'https://github.com/Team-Commonly/commonly/pull/2399', + }); + expect(noteUrl).toBe('https://x.example/api/v1/tasks/POD/TASK-001/updates'); + expect(noteInit.method).toBe('POST'); + expect(JSON.parse(noteInit.body)).toEqual({ text: 'PR opened' }); + }); + + it.each([ + 'https://example.com/Team-Commonly/commonly/pull/2399', + 'https://github.com/Team-Commonly/commonly/issues/2399', + 'https://github.com/Team-Commonly/commonly/pull/2399/extra', + 'https://github.com/../commonly/pull/2399', + 'https://github.com/Team-Commonly/commonly/pull/0', + 'https://github.com/Team-Commonly/commonly/pull/2399/', + 123, + ])('update_task refuses a non-PR URL before any request: %s', async (prUrl) => { + const fetchSpy = installFetch(async () => okResponse({ ok: true })); + const result = await byName.commonly_update_task.call({ + podId: 'POD', taskId: 'TASK-001', text: 'PR opened', prUrl, + }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('prUrl must be https://github.com/'); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('preserves priority and the freshness override when saving a PR URL and a note', async () => { + const fetchSpy = installFetch(async () => okResponse({ ok: true })); + await byName.commonly_update_task.call({ + podId: 'POD', taskId: 'TASK-001', text: 'PR opened', priority: 'P1', continueAnyway: true, + prUrl: 'https://github.com/Team-Commonly/commonly/pull/2399', + }); + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(JSON.parse(fetchSpy.mock.calls[1][1].body)).toEqual({ text: 'PR opened', priority: 'P1', continueAnyway: true }); + }); + + it('does not append a note when the PR URL PATCH is refused', async () => { + const fetchSpy = installFetch(async () => ({ + ok: false, status: 409, text: async () => JSON.stringify({ error: 'Task is leased by another agent' }), + })); + const result = await byName.commonly_update_task.call({ + podId: 'POD', taskId: 'TASK-001', text: 'PR opened', + prUrl: 'https://github.com/Team-Commonly/commonly/pull/2399', + }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('409'); + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(fetchSpy.mock.calls[0][1].method).toBe('PATCH'); + }); }); describe('commonly_create_pod', () => { diff --git a/commonly-mcp/src/tools.js b/commonly-mcp/src/tools.js index bf390f9a44..764c25c713 100644 --- a/commonly-mcp/src/tools.js +++ b/commonly-mcp/src/tools.js @@ -58,6 +58,14 @@ const reqWith = (props, requiredKeys) => ({ ...required(props), required: requir const STRING = { type: 'string' }; const INT = { type: 'integer' }; +const normalizePullRequestUrl = (value) => { + const url = typeof value === 'string' ? value.split(/[?#]/, 1)[0] : ''; + const match = /^https:\/\/github\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/pull\/[1-9]\d*$/.exec(url); + if (!match || match[0] !== url || new URL(url).href !== url) { + throw new Error('prUrl must be https://github.com///pull/'); + } + return url; +}; const CLAIM_OUTCOME = { type: 'string', enum: ['declined', 'completed'] }; const DECISION_CLASS = { type: 'string', enum: ['strategy', 'implementation', 'prioritization'] }; const DECISION_OPTION = { @@ -409,19 +417,27 @@ export const buildTools = (config) => { }, { name: 'commonly_update_task', - description: 'Append an update note and optionally set priority (P0 highest through P3), without changing status — visible in the task drawer history. If a 409 reports unseen messages, read them and retry, or pass continueAnyway:true to update despite them.', + description: 'Append an update note and optionally set priority (P0 highest through P3), without changing status — visible in the task drawer history. Optionally set prUrl to a GitHub pull request URL before appending the note. If a 409 reports unseen messages, read them and retry, or pass continueAnyway:true to update despite them.', inputSchema: reqWith({ podId: STRING, taskId: STRING, text: STRING, priority: { type: 'string', enum: ['P0', 'P1', 'P2', 'P3'] }, + prUrl: STRING, continueAnyway: { type: 'boolean' }, }, ['podId', 'taskId', 'text']), - call: wrap(async ({ podId, taskId, text, priority, continueAnyway }) => request(config, { - method: 'POST', - path: `/api/v1/tasks/${encodeURIComponent(podId)}/${encodeURIComponent(taskId)}/updates`, - body: { text, priority, continueAnyway }, - })), + call: wrap(async ({ podId, taskId, text, priority, prUrl, continueAnyway }) => { + const taskPath = `/api/v1/tasks/${encodeURIComponent(podId)}/${encodeURIComponent(taskId)}`; + if (prUrl !== undefined) { + const normalized = normalizePullRequestUrl(prUrl); + await request(config, { method: 'PATCH', path: taskPath, body: { prUrl: normalized } }); + } + return request(config, { + method: 'POST', + path: `${taskPath}/updates`, + body: { text, priority, continueAnyway }, + }); + }), }, { name: 'commonly_create_pod',