Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions actions/setup/js/update_handler_factory.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ function createUpdateHandlerFactory(handlerConfig) {

const updateData = updateDataResult.data;

// Store the original workflow repo in updateData so that executeUpdate functions
// can build correct attribution URLs. effectiveContext.repo may be overridden to
// the cross-repo target, but the run URL must always reference the current workflow.
updateData._workflowRepo = context.repo;

// Validate that we have something to update
// Note: Fields starting with "_" are internal (e.g., _rawBody, _operation)
// and will be processed by executeUpdate. We should NOT skip if _rawBody exists.
Expand All @@ -217,11 +222,13 @@ function createUpdateHandlerFactory(handlerConfig) {
};
}

core.info(`Updating ${itemTypeName} #${itemNumber} with: ${JSON.stringify(updateFields)}`);
// Include "body" in logged fields when a body update is queued (stored as internal _rawBody)
const logFields = hasRawBody ? [...updateFields, "body"] : updateFields;
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

logFields appends "body" whenever _rawBody is present, but some handlers (e.g., update_pull_request) already include body in updateFields, which can lead to duplicate entries in the log output (e.g., ["body","body"]). Consider de-duping (or only adding "body" when it’s not already present) so diagnostics stay accurate.

Suggested change
const logFields = hasRawBody ? [...updateFields, "body"] : updateFields;
const logFields = hasRawBody
? (updateFields.includes("body") ? updateFields : [...updateFields, "body"])
: updateFields;

Copilot uses AI. Check for mistakes.
core.info(`Updating ${itemTypeName} #${itemNumber} in ${effectiveContext.repo.owner}/${effectiveContext.repo.repo} with: ${JSON.stringify(logFields)}`);

// If in staged mode, preview the update without applying it
if (isStaged) {
logStagedPreviewInfo(`Would update ${itemTypeName} #${itemNumber} with fields: ${JSON.stringify(updateFields)}`);
logStagedPreviewInfo(`Would update ${itemTypeName} #${itemNumber} with fields: ${JSON.stringify(logFields)}`);
return {
success: true,
staged: true,
Expand Down
46 changes: 46 additions & 0 deletions actions/setup/js/update_handler_factory.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -589,5 +589,51 @@ describe("update_handler_factory.cjs", () => {
// executeUpdate should not have been called
expect(mockExecuteUpdate).not.toHaveBeenCalled();
});

it("should inject _workflowRepo into updateData before calling executeUpdate", async () => {
let capturedUpdateData = null;
const mockExecuteUpdate = vi.fn().mockImplementation(async (githubClient, context, num, data) => {
capturedUpdateData = data;
return { html_url: "https://example.com", title: "Updated" };
});

const handlerFactory = factoryModule.createUpdateHandlerFactory({
itemType: "update_test",
itemTypeName: "test item",
supportsPR: false,
resolveItemNumber: vi.fn().mockReturnValue({ success: true, number: 42 }),
buildUpdateData: vi.fn().mockReturnValue({ success: true, data: { title: "Test" } }),
executeUpdate: mockExecuteUpdate,
formatSuccessResult: vi.fn().mockReturnValue({ success: true }),
});

const handler = await handlerFactory({ "target-repo": "other-owner/side-repo", allowed_repos: ["other-owner/side-repo"] });
await handler({ issue_number: 42, repo: "other-owner/side-repo" });

// _workflowRepo must reference the original context.repo (the current workflow)
expect(capturedUpdateData._workflowRepo).toBeDefined();
expect(capturedUpdateData._workflowRepo.owner).toBe(mockContext.repo.owner);
expect(capturedUpdateData._workflowRepo.repo).toBe(mockContext.repo.repo);
});

it("should include body in log fields when _rawBody is present", async () => {
const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com", title: "Updated" });

const handlerFactory = factoryModule.createUpdateHandlerFactory({
itemType: "update_test",
itemTypeName: "test item",
supportsPR: false,
resolveItemNumber: vi.fn().mockReturnValue({ success: true, number: 42 }),
buildUpdateData: vi.fn().mockReturnValue({ success: true, data: { _rawBody: "new body content" } }),
executeUpdate: mockExecuteUpdate,
formatSuccessResult: vi.fn().mockReturnValue({ success: true }),
});

const handler = await handlerFactory({});
await handler({ body: "new body content" });

// The log should mention "body" even though _rawBody starts with underscore
expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining('"body"'));
});
});
});
10 changes: 7 additions & 3 deletions actions/setup/js/update_issue.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const { sanitizeTitle } = require("./sanitize_title.cjs");
const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs");
const { ERR_VALIDATION } = require("./error_codes.cjs");
const { parseBoolTemplatable } = require("./templatable.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");

/**
* Maximum limits for issue update parameters to prevent resource exhaustion.
Expand Down Expand Up @@ -44,7 +45,7 @@ async function executeIssueUpdate(github, context, issueNumber, updateData) {
const titlePrefix = updateData._titlePrefix || "";

// Remove internal fields
const { _operation, _rawBody, _includeFooter, _titlePrefix, ...apiData } = updateData;
const { _operation, _rawBody, _includeFooter, _titlePrefix, _workflowRepo, ...apiData } = updateData;

// Fetch current issue if needed (title prefix validation or body update)
if (titlePrefix || rawBody !== undefined) {
Expand Down Expand Up @@ -74,10 +75,13 @@ async function executeIssueUpdate(github, context, issueNumber, updateData) {

const currentBody = currentIssue.body || "";

// Get workflow run URL for AI attribution
// Get workflow run URL for AI attribution.
// Use the original workflow repo (_workflowRepo) rather than context.repo, because
// context may be effectiveContext with repo overridden to a cross-repo target.
const workflowName = process.env.GH_AW_WORKFLOW_NAME || "GitHub Agentic Workflow";
const workflowId = process.env.GH_AW_WORKFLOW_ID || "";
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
const workflowRepo = _workflowRepo || context.repo;
const runUrl = buildWorkflowRunUrl(context, workflowRepo);

// Use helper to update body (handles all operations including replace)
apiData.body = updateBody({
Expand Down
143 changes: 143 additions & 0 deletions actions/setup/js/update_issue.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -741,3 +741,146 @@ describe("update_issue.cjs - title_prefix configuration", () => {
expect(mockGithub.rest.issues.update).not.toHaveBeenCalled();
});
});

describe("update_issue.cjs - cross-repo and operation integration", () => {
beforeEach(async () => {
vi.clearAllMocks();
vi.resetModules();
process.env.GH_AW_WORKFLOW_NAME = "Test Workflow";
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
});

it("should route API calls to the cross-repo target when repo field is provided", async () => {
let capturedOwner, capturedRepo;

mockGithub.rest.issues.get.mockImplementation(async ({ owner, repo, issue_number }) => ({
data: { number: issue_number, title: "Cross-repo Issue", body: "Original", html_url: `https://github.com/${owner}/${repo}/issues/${issue_number}` },
}));
mockGithub.rest.issues.update.mockImplementation(async ({ owner, repo, issue_number, ...data }) => {
capturedOwner = owner;
capturedRepo = repo;
return { data: { number: issue_number, title: "Cross-repo Issue", body: data.body, html_url: `https://github.com/${owner}/${repo}/issues/${issue_number}` } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({
"target-repo": "other-owner/other-repo",
allowed_repos: ["other-owner/other-repo"],
target: "*",
});

const result = await handler({ issue_number: 42, repo: "other-owner/other-repo", body: "New content" }, {});

expect(result.success).toBe(true);
expect(capturedOwner).toBe("other-owner");
expect(capturedRepo).toBe("other-repo");
});

it("should use current workflow repo in attribution URL for cross-repo updates", async () => {
let capturedBody;

mockGithub.rest.issues.get.mockResolvedValue({
data: { number: 42, title: "Cross-repo Issue", body: "Original", html_url: "https://github.com/other-owner/other-repo/issues/42" },
});
mockGithub.rest.issues.update.mockImplementation(async ({ body }) => {
capturedBody = body;
return { data: { number: 42, title: "Cross-repo Issue", body, html_url: "https://github.com/other-owner/other-repo/issues/42" } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({
"target-repo": "other-owner/other-repo",
allowed_repos: ["other-owner/other-repo"],
target: "*",
});

await handler({ issue_number: 42, repo: "other-owner/other-repo", operation: "replace", body: "New content" }, {});

// Attribution URL must reference the current workflow's repo, not the cross-repo target
expect(capturedBody).toContain("testowner/testrepo/actions/runs/12345");
expect(capturedBody).not.toContain("other-owner/other-repo/actions/runs/12345");
});

it("should apply 'append' operation: new content added after existing body", async () => {
let capturedBody;

mockGithub.rest.issues.get.mockResolvedValue({
data: { number: 100, title: "Test", body: "Existing body", html_url: "https://github.com/testowner/testrepo/issues/100" },
});
mockGithub.rest.issues.update.mockImplementation(async ({ body }) => {
capturedBody = body;
return { data: { number: 100, title: "Test", body, html_url: "https://github.com/testowner/testrepo/issues/100" } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({ target: "*" });
await handler({ issue_number: 100, operation: "append", body: "Appended content" }, {});

expect(capturedBody).toContain("Existing body");
expect(capturedBody).toContain("Appended content");
expect(capturedBody.indexOf("Existing body")).toBeLessThan(capturedBody.indexOf("Appended content"));
});

it("should apply 'replace' operation: old content replaced by new content", async () => {
let capturedBody;

mockGithub.rest.issues.get.mockResolvedValue({
data: { number: 100, title: "Test", body: "Old body to replace", html_url: "https://github.com/testowner/testrepo/issues/100" },
});
mockGithub.rest.issues.update.mockImplementation(async ({ body }) => {
capturedBody = body;
return { data: { number: 100, title: "Test", body, html_url: "https://github.com/testowner/testrepo/issues/100" } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({ target: "*" });
await handler({ issue_number: 100, operation: "replace", body: "Replacement content" }, {});

expect(capturedBody).not.toContain("Old body to replace");
expect(capturedBody).toContain("Replacement content");
});

it("should apply 'prepend' operation: new content placed before existing body", async () => {
let capturedBody;

mockGithub.rest.issues.get.mockResolvedValue({
data: { number: 100, title: "Test", body: "Existing body", html_url: "https://github.com/testowner/testrepo/issues/100" },
});
mockGithub.rest.issues.update.mockImplementation(async ({ body }) => {
capturedBody = body;
return { data: { number: 100, title: "Test", body, html_url: "https://github.com/testowner/testrepo/issues/100" } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({ target: "*" });
await handler({ issue_number: 100, operation: "prepend", body: "Prepended content" }, {});

expect(capturedBody).toContain("Existing body");
expect(capturedBody).toContain("Prepended content");
expect(capturedBody.indexOf("Prepended content")).toBeLessThan(capturedBody.indexOf("Existing body"));
});

it("should apply 'replace-island' operation: only the island section is updated", async () => {
const existingBody = "Header\n<!-- gh-aw-island-start:test-workflow -->\nOld island\n<!-- gh-aw-island-end:test-workflow -->\nFooter";
let capturedBody;

mockGithub.rest.issues.get.mockResolvedValue({
data: { number: 100, title: "Test", body: existingBody, html_url: "https://github.com/testowner/testrepo/issues/100" },
});
mockGithub.rest.issues.update.mockImplementation(async ({ body }) => {
capturedBody = body;
return { data: { number: 100, title: "Test", body, html_url: "https://github.com/testowner/testrepo/issues/100" } };
});

const { main } = await import("./update_issue.cjs");
const handler = await main({ target: "*" });
await handler({ issue_number: 100, operation: "replace-island", body: "New island content" }, {});

expect(capturedBody).toContain("Header");
expect(capturedBody).toContain("Footer");
expect(capturedBody).toContain("New island content");
expect(capturedBody).not.toContain("Old island");
expect(capturedBody).toContain("<!-- gh-aw-island-start:test-workflow -->");
expect(capturedBody).toContain("<!-- gh-aw-island-end:test-workflow -->");
});
});
10 changes: 7 additions & 3 deletions actions/setup/js/update_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { resolveTarget } = require("./safe_output_helpers.cjs");
const { createUpdateHandlerFactory, createStandardResolveNumber, createStandardFormatResult } = require("./update_handler_factory.cjs");
const { sanitizeTitle } = require("./sanitize_title.cjs");
const { parseBoolTemplatable } = require("./templatable.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");

/**
* Execute the pull request update API call
Expand All @@ -29,7 +30,7 @@ async function executePRUpdate(github, context, prNumber, updateData) {
const includeFooter = updateData._includeFooter !== false; // Default to true

// Remove internal fields
const { _operation, _rawBody, _includeFooter, ...apiData } = updateData;
const { _operation, _rawBody, _includeFooter, _workflowRepo, ...apiData } = updateData;

// If we have a body, process it with the appropriate operation
if (rawBody !== undefined) {
Expand All @@ -41,10 +42,13 @@ async function executePRUpdate(github, context, prNumber, updateData) {
});
const currentBody = currentPR.body || "";

// Get workflow run URL for AI attribution
// Get workflow run URL for AI attribution.
// Use the original workflow repo (_workflowRepo) rather than context.repo, because
// context may be effectiveContext with repo overridden to a cross-repo target.
const workflowName = process.env.GH_AW_WORKFLOW_NAME || "GitHub Agentic Workflow";
const workflowId = process.env.GH_AW_WORKFLOW_ID || "";
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
const workflowRepo = _workflowRepo || context.repo;
const runUrl = buildWorkflowRunUrl(context, workflowRepo);

// Use helper to update body (handles all operations including replace)
apiData.body = updateBody({
Expand Down
18 changes: 18 additions & 0 deletions actions/setup/js/workflow_metadata_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,24 @@ function getWorkflowMetadata(owner, repo) {
};
}

/**
* Build a workflow run URL from the provided context and workflow repository.
*
* This is the canonical helper for constructing attribution run URLs in safe-output
* handlers. It must always receive the **original** workflow repo (not an overridden
* cross-repo effectiveContext) so that footer links point back to the actual workflow
* run regardless of which repository the output action targets.
*
* @param {any} ctx - GitHub Actions context (provides serverUrl and runId)
* @param {{ owner: string, repo: string }} workflowRepo - The repository that owns the workflow run
* @returns {string} The full workflow run URL
*/
function buildWorkflowRunUrl(ctx, workflowRepo) {
const server = ctx.serverUrl || process.env.GITHUB_SERVER_URL || "https://github.com";
return `${server}/${workflowRepo.owner}/${workflowRepo.repo}/actions/runs/${ctx.runId}`;
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

buildWorkflowRunUrl will produce a URL containing undefined if ctx.runId is missing (e.g., in tests/local runs or unusual contexts). Consider defaulting runId to 0 (or throwing a clear error) to match getWorkflowMetadata’s behavior and avoid generating broken attribution links.

Suggested change
return `${server}/${workflowRepo.owner}/${workflowRepo.repo}/actions/runs/${ctx.runId}`;
const runId = ctx.runId ?? 0;
return `${server}/${workflowRepo.owner}/${workflowRepo.repo}/actions/runs/${runId}`;

Copilot uses AI. Check for mistakes.
}

module.exports = {
getWorkflowMetadata,
buildWorkflowRunUrl,
};
33 changes: 32 additions & 1 deletion actions/setup/js/workflow_metadata_helpers.test.cjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @ts-check

const { getWorkflowMetadata } = require("./workflow_metadata_helpers.cjs");
const { getWorkflowMetadata, buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");

describe("getWorkflowMetadata", () => {
let originalEnv;
Expand Down Expand Up @@ -95,3 +95,34 @@ describe("getWorkflowMetadata", () => {
expect(metadata.runUrl).toBe("https://github.com/test-owner/test-repo/actions/runs/0");
});
});

describe("buildWorkflowRunUrl", () => {
it("should build run URL from context.serverUrl and explicit workflowRepo", () => {
const ctx = { serverUrl: "https://github.com", runId: 42000 };
const url = buildWorkflowRunUrl(ctx, { owner: "wf-owner", repo: "wf-repo" });
expect(url).toBe("https://github.com/wf-owner/wf-repo/actions/runs/42000");
});

it("should fall back to GITHUB_SERVER_URL when context.serverUrl is absent", () => {
const originalEnv = process.env.GITHUB_SERVER_URL;
process.env.GITHUB_SERVER_URL = "https://ghes.example.com";
const ctx = { runId: 99 };
const url = buildWorkflowRunUrl(ctx, { owner: "ent-owner", repo: "ent-repo" });
expect(url).toBe("https://ghes.example.com/ent-owner/ent-repo/actions/runs/99");
if (originalEnv === undefined) {
delete process.env.GITHUB_SERVER_URL;
} else {
process.env.GITHUB_SERVER_URL = originalEnv;
}
});

it("should use the workflowRepo, not a cross-repo target", () => {
// Simulates the cross-repo case: context.repo is the target but workflowRepo is the workflow owner
const ctx = { serverUrl: "https://github.com", runId: 7777, repo: { owner: "cross-owner", repo: "cross-repo" } };
const workflowRepo = { owner: "wf-owner", repo: "wf-repo" };
const url = buildWorkflowRunUrl(ctx, workflowRepo);
expect(url).toBe("https://github.com/wf-owner/wf-repo/actions/runs/7777");
expect(url).not.toContain("cross-owner");
expect(url).not.toContain("cross-repo");
});
});
Loading