Skip to content

[jsweep] Clean update_work_item.cjs #66772

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.

Sign up for GitHub

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

Closed
github-actions wants to merge 3 commits into from
Closed

[jsweep] Clean update_work_item.cjs #66772

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f32621f
[jsweep] Clean update_work_item.cjs
github-actions[bot] Oct 8, 2026
0515576
Merge remote-tracking branch 'origin/main' into signed/jsweep/clean-u…
Copilot Oct 8, 2026
6fb14cf
Fix review feedback and JS schema test
Copilot Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fix review feedback and JS schema test 
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
  • Loading branch information
@gh-aw-bot
Copilot and gh-aw-bot authored Oct 8, 2026
commit 6fb14cfd47a54170b9a940b9ab2be3e26be254da
9 changes: 7 additions & 2 deletions actions/setup/js/safe_outputs_mcp_arguments.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ function normalizeSafeOutputToolArguments(toolName, args, logger, inputSchema) {
logger?.debug?.(`Recovered safe-output parameter synonyms for '${normalizedToolName}': ${JSON.stringify(remapped)}`);
}

return normalizeBlankOptionalFields(remappedArgs, inputSchema.properties, inputSchema.required);
const normalized = normalizeBlankOptionalFields(remappedArgs, inputSchema.properties, inputSchema.required);
const requiredOneOf = inputSchema["x-requires-one-of"];
if (Array.isArray(requiredOneOf) && !requiredOneOf.some(field => normalized[field] !== undefined && normalized[field] !== false)) {
throw new Error(`Invalid arguments for ${normalizedToolName}: requires at least one of ${requiredOneOf.join(", ")}`);
}
return normalized;
}

/**
Expand All @@ -86,7 +91,7 @@ function stripInternalSafeOutputSchemaMetadata(schema) {

const cleaned = {};
for (const [key, value] of Object.entries(schema)) {
if (key === "x-synonyms") {
if (key === "x-synonyms" || key === "x-requires-one-of") {
continue;
}
cleaned[key] = stripInternalSafeOutputSchemaMetadata(value);
Expand Down
11 changes: 10 additions & 1 deletion actions/setup/js/safe_outputs_mcp_wrapped_arguments.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,18 @@ describe("safe_outputs_mcp wrapped tool arguments", () => {
process.env.GH_AW_SAFE_OUTPUTS = outputPath;

const { server } = createMCPServer();
const response = await server.handleRequest({
const toolsList = await server.handleRequest({
jsonrpc: "2.0",
id: 1,
method: "tools/list",
params: {},
});
const listedTool = toolsList.result.tools.find(tool => tool.name === "update_issue");
expect(listedTool.inputSchema).not.toHaveProperty("x-requires-one-of");

const response = await server.handleRequest({
jsonrpc: "2.0",
id: 2,
method: "tools/call",
params: { name: "update_issue", arguments: { issue_number: 42, ...update } },
});
Expand Down
4 changes: 2 additions & 2 deletions actions/setup/js/safe_outputs_tools.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,7 @@
},
{
"name": "update_issue",
"description": "Update an existing GitHub issue's title, body, labels, assignees, or milestone WITHOUT closing it. This tool is primarily for editing issue metadata and content. While it supports changing status between 'open' and 'closed', use close_issue instead when you want to close an issue with a closing comment. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. IMPORTANT: The behavior of this tool depends on the workflow's `update-issue: target:` configuration. When `target: triggering` (the default), the tool always updates the issue that triggered the workflow and `issue_number` is ignored. When `target: '*'`, the `issue_number` field controls which issue is updated. The tool will fail (not skip silently) when `target: triggering` and there is no triggering issue (e.g., in scheduled or workflow_dispatch workflows).",
"description": "Update an existing GitHub issue's title, body, labels, assignees, or milestone WITHOUT closing it. Provide at least one of: status, title, body, labels, assignees, or milestone. This tool is primarily for editing issue metadata and content. While it supports changing status between 'open' and 'closed', use close_issue instead when you want to close an issue with a closing comment. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. IMPORTANT: The behavior of this tool depends on the workflow's `update-issue: target:` configuration. When `target: triggering` (the default), the tool always updates the issue that triggered the workflow and `issue_number` is ignored. When `target: '*'`, the `issue_number` field controls which issue is updated. The tool will fail (not skip silently) when `target: triggering` and there is no triggering issue (e.g., in scheduled or workflow_dispatch workflows).",
"inputSchema": {
"type": "object",
"properties": {
Expand Down Expand Up @@ -1368,7 +1368,7 @@
"description": "Trustworthiness level of the message source (e.g., \"low\", \"medium\", \"high\")."
}
},
"anyOf": [{ "required": ["status"] }, { "required": ["title"] }, { "required": ["body"] }, { "required": ["labels"] }, { "required": ["assignees"] }, { "required": ["milestone"] }],
"x-requires-one-of": ["status", "title", "body", "labels", "assignees", "milestone"],
"additionalProperties": false
}
},
Expand Down
8 changes: 5 additions & 3 deletions actions/setup/js/update_work_item.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
const { createAzureDevOpsWorkItemHandler } = require("./azure_devops_work_items.cjs");

/**
* Creates the Azure DevOps update work item safe-output handler.
* @param {Object} [config] - Handler configuration
* @returns {Promise<Function>} Message handler
* @typedef {import("./types/handler-factory").HandlerFactoryFunction} HandlerFactoryFunction
*/

/**
* @type {HandlerFactoryFunction}
*/
async function main(config = {}) {
return createAzureDevOpsWorkItemHandler("ado_update_work_item", config);
Expand Down
38 changes: 11 additions & 27 deletions actions/setup/js/update_work_item.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { main } from "./update_work_item.cjs";

global.core = { debug: vi.fn(), info: vi.fn(), warning: vi.fn() };
Expand All @@ -9,41 +9,25 @@ describe("update_work_item", () => {
global.fetch = vi.fn();

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

global.fetch is installed in beforeEach but never restored, so this file can leak a mocked network layer into later suites and make unrelated tests pass or fail for the wrong reason.

💡 Restore the global after each test

The existing shared-handler suite already deletes global.fetch in afterEach, and this wrapper test needs the same isolation. Without that cleanup, any later test file that forgets to stub fetch will silently run against this leftover mock instead of the real environment.

afterEach(() => {
  delete global.fetch;
});

That keeps this file self-contained and avoids cross-suite state pollution in CI.

Copilot AI Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added afterEach cleanup to delete the mocked global.fetch after every test. Included in 6fb14cf.

});

it("returns a handler function", async () => {
expect(typeof (await main())).toBe("function");
afterEach(() => {
delete global.fetch;
});

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

L15: delete: duplicate default-config handler type assertion. The preceding test already proves main() returns a handler; nothing replaces it.

Copilot AI Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removed the redundant default-config test; the remaining smoke test covers handler creation, and a focused assertion checks update-specific wiring. Included in 6fb14cf.

it("works with default config", async () => {
const handler = await main(undefined);
expect(typeof handler).toBe("function");
it("returns a handler function", async () => {
expect(typeof (await main())).toBe("function");
});

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] Good coverage of the wrapper's thin-pass-through contract, but these 6 tests duplicate behavior already exercised in azure_devops_work_items.test.cjs (field rejection, area-path rejection use near-identical fixtures) without adding tests for behavior unique to this wrapper, e.g. that main() correctly forwards the "ado_update_work_item" type string vs. another type.

💡 Suggestion

Consider either: (a) trimming this file to just the handler-creation/default-config smoke tests (2 tests) since the validation logic is already fully covered in azure_devops_work_items.test.cjs, or (b) adding one assertion that proves type-specific wiring, e.g. asserting the error message explicitly references ado_update_work_item (which test 3 already does) and is not reused verbatim from another handler's config. As-is, 4 of the 6 tests re-verify handleUpdateWorkItem internals rather than the main() wrapper's own responsibility (delegating to the correct handler type), so a bug in the wrapper's type string wouldn't be caught by tests that only check validation errors.

@copilot please address this.

Copilot AI Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trimmed duplicated shared-handler cases and focused wrapper coverage on its configured handler type and input immutability. Included in 6fb14cf.


it("rejects fields disabled by configuration", async () => {
it("uses the update work-item handler", async () => {
const handler = await main({ target: "*", title: false });
const result = await handler({ id: 42, title: "New title" }, {});
expect(result).toEqual({ success: false, error: "title updates are not enabled by ado_update_work_item" });
expect(global.fetch).not.toHaveBeenCalled();
});

it("rejects area paths outside configured prefixes", async () => {
const handler = await main({ staged: true, area_path: true, allowed_area_prefixes: ["proj\\Platform"] });
const result = await handler({ id: 42, area_path: "proj\\Other" }, {});
expect(result.success).toBe(false);
expect(result.error).toContain("area_path");
expect(global.fetch).not.toHaveBeenCalled();
});

it("does not call the API in staged mode", async () => {
const handler = await main({ staged: true, target: "*", title: true });
const result = await handler({ id: 42, title: "New title" }, {});
expect(result.success).toBe(true);
expect(global.fetch).not.toHaveBeenCalled();
});

it("does not mutate the input message", async () => {
const handler = await main({ staged: true, target: "*", title: true });
const message = { id: 42, title: "T" };
await handler(message, {});
expect(message).toEqual({ id: 42, title: "T" });
it("does not mutate the input body when applying a configured footer", async () => {
const handler = await main({ staged: true, target: "*", body: true, body_footer: "Configured footer" });
const message = { id: 42, body: "Original body" };
expect(await handler(message, {})).toMatchObject({ success: true, staged: true });
expect(message).toEqual({ id: 42, body: "Original body" });
});
});
4 changes: 2 additions & 2 deletions pkg/workflow/js/safe_outputs_tools.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,7 @@
},
{
"name": "update_issue",
"description": "Update an existing GitHub issue's title, body, labels, assignees, or milestone WITHOUT closing it. This tool is primarily for editing issue metadata and content. While it supports changing status between 'open' and 'closed', use close_issue instead when you want to close an issue with a closing comment. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. IMPORTANT: The behavior of this tool depends on the workflow's `update-issue: target:` configuration. When `target: triggering` (the default), the tool always updates the issue that triggered the workflow and `issue_number` is ignored. When `target: '*'`, the `issue_number` field controls which issue is updated. The tool will fail (not skip silently) when `target: triggering` and there is no triggering issue (e.g., in scheduled or workflow_dispatch workflows).",
"description": "Update an existing GitHub issue's title, body, labels, assignees, or milestone WITHOUT closing it. Provide at least one of: status, title, body, labels, assignees, or milestone. This tool is primarily for editing issue metadata and content. While it supports changing status between 'open' and 'closed', use close_issue instead when you want to close an issue with a closing comment. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. IMPORTANT: The behavior of this tool depends on the workflow's `update-issue: target:` configuration. When `target: triggering` (the default), the tool always updates the issue that triggered the workflow and `issue_number` is ignored. When `target: '*'`, the `issue_number` field controls which issue is updated. The tool will fail (not skip silently) when `target: triggering` and there is no triggering issue (e.g., in scheduled or workflow_dispatch workflows).",
"inputSchema": {
"type": "object",
"properties": {
Expand Down Expand Up @@ -1368,7 +1368,7 @@
"description": "Trustworthiness level of the message source (e.g., \"low\", \"medium\", \"high\")."
}
},
"anyOf": [{ "required": ["status"] }, { "required": ["title"] }, { "required": ["body"] }, { "required": ["labels"] }, { "required": ["assignees"] }, { "required": ["milestone"] }],
"x-requires-one-of": ["status", "title", "body", "labels", "assignees", "milestone"],
"additionalProperties": false
}
},
Expand Down
9 changes: 6 additions & 3 deletions pkg/workflow/safe_output_blank_fields_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ for (const [type, { fields }] of Object.entries(config)) {
assert.equal(collector.isValid, true, type + "." + field);
continue;
}
if (tool.inputSchema.anyOf) continue;
if (tool.inputSchema.anyOf || tool.inputSchema["x-requires-one-of"]) continue;
const args = normalizeSafeOutputToolArguments(type, { [field]: blank }, undefined, tool.inputSchema);
const mcpAccepted = !validateArgumentsAgainstSchema(args, tool.inputSchema) &&
validateStringMinLengths(args, tool.inputSchema).length === 0;
Expand Down Expand Up @@ -82,8 +82,11 @@ assert.equal(blankUpdate.isValid, false, "blank fields must not satisfy requires
const updateTool = tools.find(tool => tool.name === "update_issue");
for (const field of ["status", "labels"]) {
const raw = { type: "update_issue", issue_number: 42, [field]: "" };
const normalized = normalizeSafeOutputToolArguments("update_issue", raw, undefined, updateTool.inputSchema);
assert.ok(validateArgumentsAgainstSchema(normalized, updateTool.inputSchema), "normalized update_issue must fail MCP cross-field validation: " + field);
assert.throws(
() => normalizeSafeOutputToolArguments("update_issue", raw, undefined, updateTool.inputSchema),
/requires at least one of/,
"MCP normalization must reject update_issue with only a blank field: " + field
);
assert.equal(validateItem(raw, raw.type, 1).isValid, false, "update_issue collector must reject a sole blank field: " + field);
}
const clearedIssueType = validateItem({ type: "set_issue_type", issue_type: "" }, "set_issue_type", 1);
Expand Down
Loading