diff --git a/actions/setup/js/add_comment.cjs b/actions/setup/js/add_comment.cjs index 85dc41e4077..71d24f71eeb 100644 --- a/actions/setup/js/add_comment.cjs +++ b/actions/setup/js/add_comment.cjs @@ -610,9 +610,8 @@ async function main(config = {}) { commentIdToReuse = allowedCommentId.commentId; } - // Check if item_number or issue_number was explicitly provided in the message. - // item_number takes precedence over issue_number when both are present. - // pr-number is accepted as an alias for item_number for robustness. + // Track whether the configured target selected an explicit item rather than the + // triggering context. This controls reply and retry behavior later in the handler. /** @type {{ success: boolean, number?: number | null, deferred?: boolean, error?: string }} */ let itemTargetResult = { success: true, number: null }; if (hasExplicitCommentId) { @@ -641,22 +640,10 @@ async function main(config = {}) { }; } } else { - itemTargetResult = resolveSafeOutputIssueTarget({ message, tempIdMap: temporaryIdMap, repoParts, handlerType: HANDLER_TYPE, aliases: ["item_number", "issue_number", "pr-number"] }); - if (!itemTargetResult.success) return itemTargetResult; - } - - if (itemTargetResult.number != null) { - itemNumber = itemTargetResult.number; - core.info(`Using explicitly provided target number (item_number/issue_number/pr-number): #${itemNumber}`); - } else if (!hasExplicitCommentId) { - // Check if this is a discussion context const isDiscussionContext = effectiveContext.eventName === "discussion" || effectiveContext.eventName === "discussion_comment"; - - if (isDiscussionContext) { - // For discussions, always use the discussion context + if (commentTarget === "triggering" && isDiscussionContext) { isDiscussion = true; itemNumber = effectiveContext.payload?.discussion?.number; - if (!itemNumber) { core.warning("Discussion context detected but no discussion number found"); return { @@ -664,13 +651,20 @@ async function main(config = {}) { error: "No discussion number available", }; } - core.info(`Using discussion context: #${itemNumber}`); } else { - // For issues/PRs, use the resolveTarget helper which respects target configuration + let targetItem = message; + if (commentTarget === "*") { + itemTargetResult = resolveSafeOutputIssueTarget({ message, tempIdMap: temporaryIdMap, repoParts, handlerType: HANDLER_TYPE, aliases: ["item_number", "issue_number", "pr-number"] }); + if (!itemTargetResult.success) return itemTargetResult; + if (itemTargetResult.number != null) { + targetItem = { ...message, item_number: itemTargetResult.number }; + } + } + const targetResult = resolveTarget({ targetConfig: commentTarget, - item: message, + item: targetItem, context: effectiveContext, itemType: "add_comment", supportsPR: true, // add_comment supports both issues and PRs @@ -710,6 +704,9 @@ async function main(config = {}) { } itemNumber = targetResult.number; + if (commentTarget !== "triggering") { + itemTargetResult = { success: true, number: itemNumber }; + } core.info(`Resolved target ${targetResult.contextType} #${itemNumber} (target config: ${commentTarget})`); } } diff --git a/actions/setup/js/add_comment.test.cjs b/actions/setup/js/add_comment.test.cjs index 420c16e9af8..228cd1fb0a2 100644 --- a/actions/setup/js/add_comment.test.cjs +++ b/actions/setup/js/add_comment.test.cjs @@ -283,7 +283,7 @@ describe("add_comment", () => { expect(result.error).toBeTruthy(); }); - it("should use explicit item_number even with triggering target", async () => { + it("should ignore explicit item_number with triggering target", async () => { const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); /** @type {any} */ @@ -310,8 +310,8 @@ describe("add_comment", () => { const result = await handler(message, {}); expect(result.success).toBe(true); - expect(capturedIssueNumber).toBe(777); - expect(result.itemNumber).toBe(777); + expect(capturedIssueNumber).toBe(8535); + expect(result.itemNumber).toBe(8535); }); it("should resolve from context when item_number is not provided", async () => { @@ -545,7 +545,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: 'triggering' }); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -1055,7 +1055,7 @@ describe("add_comment", () => { throw err; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); // Explicit item_number targeting a different discussion (via 404 fallback) const message = { @@ -1117,7 +1117,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -1179,7 +1179,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -1302,7 +1302,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -1364,7 +1364,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -1997,7 +1997,7 @@ describe("add_comment", () => { } }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: 'triggering' }); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2198,7 +2198,7 @@ describe("add_comment", () => { } }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: 'triggering' }); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2257,7 +2257,7 @@ describe("add_comment", () => { } }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: 'triggering' }); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2292,7 +2292,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2315,7 +2315,7 @@ describe("add_comment", () => { it("should defer when temporary ID is not yet resolved", async () => { const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2348,7 +2348,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2370,7 +2370,7 @@ describe("add_comment", () => { it("should handle invalid temporary ID format", async () => { const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2439,7 +2439,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2472,7 +2472,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -2495,7 +2495,7 @@ describe("add_comment", () => { it("should defer when issue_number has unresolved temporary ID", async () => { const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -3079,7 +3079,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", @@ -3124,7 +3124,7 @@ describe("add_comment", () => { }; }; - const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`); const message = { type: "add_comment", diff --git a/actions/setup/js/add_reviewer.cjs b/actions/setup/js/add_reviewer.cjs index 39eca8d58b5..e4c109e452c 100644 --- a/actions/setup/js/add_reviewer.cjs +++ b/actions/setup/js/add_reviewer.cjs @@ -17,9 +17,8 @@ const HANDLER_TYPE = "add_reviewer"; const { processItems } = require("./safe_output_processor.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); -const { getPullRequestNumber } = require("./pr_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); -const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { attachExecutionState, extractReviewStateFromData, fetchPullRequestReviewState } = require("./safe_output_execution_metadata.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); @@ -85,6 +84,7 @@ async function main(config = {}) { const allowedReviewers = config.allowed ?? []; const allowedTeamReviewers = config.allowed_team_reviewers ?? []; const maxCount = config.max ?? 10; + const targetConfig = config.target || "triggering"; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); const isStaged = isStagedMode(config); @@ -152,21 +152,20 @@ async function main(config = {}) { processedCount++; - const { prNumber, error } = getPullRequestNumber(message, context); - - if (error) { - core.warning(error); - return { - success: false, - error, - }; - } - if (prNumber === null) { + const targetResult = resolveTarget({ + targetConfig, + item: message, + context, + itemType: HANDLER_TYPE, + }); + if (!targetResult.success) { + core.warning(targetResult.error); return { success: false, - error: "Pull request number is required", + error: targetResult.error, }; } + const prNumber = targetResult.number; const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "pull request reviewer"); if (!repoResult.success) { diff --git a/actions/setup/js/add_reviewer.test.cjs b/actions/setup/js/add_reviewer.test.cjs index b5baaa0b24a..fef46d3020c 100644 --- a/actions/setup/js/add_reviewer.test.cjs +++ b/actions/setup/js/add_reviewer.test.cjs @@ -269,6 +269,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); it("should use explicit PR number from message", async () => { + const { main } = require("./add_reviewer.cjs"); + handler = await main({ max: 10, allowed: ["user1"], target: "*" }); const message = { type: "add_reviewer", reviewers: ["user1"], @@ -310,7 +312,7 @@ describe("add_reviewer (Handler Factory Architecture)", () => { const result = await handler(message, {}); expect(result.success).toBe(false); - expect(result.error).toContain("No pull_request_number provided and not in pull request context"); + expect(result.error).toContain("not running in pull request context"); expect(mockGithub.rest.pulls.requestReviewers).not.toHaveBeenCalled(); }); @@ -510,6 +512,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => { }); it("should return error for invalid pull_request_number", async () => { + const { main } = require("./add_reviewer.cjs"); + handler = await main({ max: 10, allowed: ["user1"], target: "*" }); const invalidValues = ["not-a-number", null, "abc123"]; for (const invalidValue of invalidValues) { @@ -523,7 +527,7 @@ describe("add_reviewer (Handler Factory Architecture)", () => { const result = await handler(message, {}); expect(result.success).toBe(false); - expect(result.error).toContain("Invalid pull_request_number"); + expect(result.error).toMatch(/Invalid pull_request_number|no pull_request_number/); expect(mockGithub.rest.pulls.requestReviewers).not.toHaveBeenCalled(); } }); diff --git a/actions/setup/js/assign_milestone.cjs b/actions/setup/js/assign_milestone.cjs index 83d913d6fd3..bf2e24716e0 100644 --- a/actions/setup/js/assign_milestone.cjs +++ b/actions/setup/js/assign_milestone.cjs @@ -8,7 +8,7 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); -const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { loadTemporaryIdMapFromResolved, resolveRepoIssueTarget } = require("./temporary_id.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); @@ -45,6 +45,7 @@ async function main(config = {}) { // Extract configuration const allowedMilestones = config.allowed || []; const maxCount = config.max || 10; + const targetConfig = config.target || "triggering"; const autoCreate = config.auto_create === true; const githubClient = await createAuthenticatedGitHubClient(config); @@ -151,34 +152,46 @@ async function main(config = {}) { const milestoneOwner = repoResult.repoParts.owner; const milestoneRepo = repoResult.repoParts.repo; - // Resolve issue_number, which may be a temporary ID (e.g. "aw_abc123") or a plain number - const resolvedIssueTarget = resolveRepoIssueTarget(item.issue_number, temporaryIdMap, milestoneOwner, milestoneRepo); - - // If the issue_number is a temporary ID that hasn't been resolved yet, defer processing - if (resolvedIssueTarget.wasTemporaryId && !resolvedIssueTarget.resolved) { - core.info(`Deferring assign_milestone: unresolved temporary ID (${item.issue_number})`); - return { - success: false, - deferred: true, - error: resolvedIssueTarget.errorMessage || `Unresolved temporary ID: ${item.issue_number}`, - }; + let targetItem = item; + let resolvedIssueTarget; + if (targetConfig === "*" && item.issue_number != null) { + resolvedIssueTarget = resolveRepoIssueTarget(item.issue_number, temporaryIdMap, milestoneOwner, milestoneRepo); + if (resolvedIssueTarget.wasTemporaryId && !resolvedIssueTarget.resolved) { + core.info(`Deferring assign_milestone: unresolved temporary ID (${item.issue_number})`); + return { + success: false, + deferred: true, + error: resolvedIssueTarget.errorMessage || `Unresolved temporary ID: ${item.issue_number}`, + }; + } + if (resolvedIssueTarget.errorMessage || !resolvedIssueTarget.resolved) { + core.error(`Invalid issue_number: ${item.issue_number}`); + return { + success: false, + error: `Invalid issue_number: ${item.issue_number}`, + }; + } + targetItem = { ...item, issue_number: resolvedIssueTarget.resolved.number }; } - if (resolvedIssueTarget.errorMessage || !resolvedIssueTarget.resolved) { - core.error(`Invalid issue_number: ${item.issue_number}`); - return { - success: false, - error: `Invalid issue_number: ${item.issue_number}`, - }; + const numberResult = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: HANDLER_TYPE, + supportsIssue: true, + }); + if (!numberResult.success) { + core.warning(numberResult.error); + return { success: false, error: numberResult.error }; } - - const issueNumber = resolvedIssueTarget.resolved.number; + const issueNumber = numberResult.number; const repoParts = { owner: milestoneOwner, repo: milestoneRepo }; const filterResult = await checkRequiredFilter(githubClient, repoParts, issueNumber, requiredLabels, requiredTitlePrefix, "assign_milestone"); if (filterResult) return filterResult; - if (resolvedIssueTarget.wasTemporaryId) { + if (resolvedIssueTarget?.wasTemporaryId) { core.info(`Resolved temporary ID '${item.issue_number}' to issue #${issueNumber}`); } diff --git a/actions/setup/js/assign_milestone.test.cjs b/actions/setup/js/assign_milestone.test.cjs index 05d3b63cb01..7c4e733f2b9 100644 --- a/actions/setup/js/assign_milestone.test.cjs +++ b/actions/setup/js/assign_milestone.test.cjs @@ -68,6 +68,7 @@ describe("assign_milestone (Handler Factory Architecture)", () => { handler = await main({ max: 10, allowed: [], + target: "*", }); }); @@ -261,7 +262,7 @@ describe("assign_milestone (Handler Factory Architecture)", () => { it("should resolve milestone by title when milestone_number is not provided", async () => { const { main } = require("./assign_milestone.cjs"); - const handlerWithTitle = await main({ max: 10 }); + const handlerWithTitle = await main({ max: 10, target: "*" }); mockPaginateWith([ { number: 5, title: "v1.0" }, @@ -311,7 +312,7 @@ describe("assign_milestone (Handler Factory Architecture)", () => { it("should auto-create milestone when auto_create is true and title not found", async () => { const { main } = require("./assign_milestone.cjs"); - const handlerAutoCreate = await main({ max: 10, auto_create: true }); + const handlerAutoCreate = await main({ max: 10, auto_create: true, target: "*" }); mockPaginateWith([]); mockGithub.rest.issues.createMilestone.mockResolvedValue({ diff --git a/actions/setup/js/assign_to_agent.cjs b/actions/setup/js/assign_to_agent.cjs index 83e203d4ab0..84aa6d79749 100644 --- a/actions/setup/js/assign_to_agent.cjs +++ b/actions/setup/js/assign_to_agent.cjs @@ -269,7 +269,8 @@ async function main(config = {}) { const customInstructions = defaultCustomInstructions || null; // Validate that both issue_number and pull_number are not specified simultaneously - if (message.issue_number != null && message.pull_number != null) { + // (only relevant when the model-provided identifiers are actually used, i.e. target: "*") + if (targetConfig === "*" && message.issue_number != null && message.pull_number != null) { const error = "Cannot specify both issue_number and pull_number in the same assign_to_agent item"; core.error(error); allResults.push({ issue_number: message.issue_number, pull_number: message.pull_number, agent: agentName, owner: null, repo: null, success: false, error }); @@ -278,7 +279,7 @@ async function main(config = {}) { // Defer if issue_number is a temporary ID that hasn't been resolved yet // Strip leading '#' so both 'aw_abc1' and '#aw_abc1' (canonical validator form) are handled - if (message.issue_number != null) { + if (targetConfig === "*" && message.issue_number != null) { const issueNumStr = String(message.issue_number).trim(); if (isTemporaryId(issueNumStr)) { const normalized = normalizeTemporaryId(issueNumStr); @@ -301,7 +302,7 @@ async function main(config = {}) { let itemForTarget = message; // Resolve temporary ID in issue_number to real issue number - if (message.issue_number != null) { + if (targetConfig === "*" && message.issue_number != null) { const resolvedTarget = resolveRepoIssueTarget(message.issue_number, temporaryIdMap, effectiveOwner, effectiveRepo); if (!resolvedTarget.resolved) { const error = resolvedTarget.errorMessage || `Failed to resolve issue target: ${message.issue_number}`; @@ -317,10 +318,6 @@ async function main(config = {}) { } } - // Determine effective target configuration - const hasExplicitTarget = itemForTarget.issue_number != null || itemForTarget.pull_number != null; - const effectiveTarget = hasExplicitTarget ? "*" : targetConfig; - const basePullRequestRepoSlug = pullRequestOwner && pullRequestRepo ? `${pullRequestOwner}/${pullRequestRepo}` : `${effectiveOwner}/${effectiveRepo}`; // Handle per-item pull_request_repo override @@ -364,7 +361,7 @@ async function main(config = {}) { // Resolve the target issue or pull request number from context const targetResult = resolveTarget({ - targetConfig: effectiveTarget, + targetConfig, item: itemForTarget, context, itemType: "assign_to_agent", diff --git a/actions/setup/js/assign_to_agent.test.cjs b/actions/setup/js/assign_to_agent.test.cjs index 182dd216b03..c2cff4bc9b5 100644 --- a/actions/setup/js/assign_to_agent.test.cjs +++ b/actions/setup/js/assign_to_agent.test.cjs @@ -57,7 +57,7 @@ describe("assign_to_agent", () => { // This mirrors the production flow without requiring any backward-compat changes in // assign_to_agent.cjs itself. const STANDALONE_RUNNER = ` - const _config = {}; + const _config = { target: "*" }; if (process.env.GH_AW_AGENT_DEFAULT?.trim()) _config.name = process.env.GH_AW_AGENT_DEFAULT.trim(); if (process.env.GH_AW_AGENT_MODEL?.trim()) _config.model = process.env.GH_AW_AGENT_MODEL.trim(); if (process.env.GH_AW_AGENT_MAX_COUNT?.trim()) _config.max = process.env.GH_AW_AGENT_MAX_COUNT.trim(); @@ -360,7 +360,7 @@ describe("assign_to_agent", () => { // Call main() factory then invoke the handler directly so we can inspect the deferred result const deferred = await eval(`(async () => { ${assignToAgentScript}; - const _handler = await main({}); + const _handler = await main({ target: "*" }); const { loadTemporaryIdMap } = require("./temporary_id.cjs"); const _map = loadTemporaryIdMap(); return _handler({ type: "assign_to_agent", issue_number: "#aw_abc123", agent: "copilot" }, {}, _map); @@ -893,7 +893,46 @@ describe("assign_to_agent", () => { expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Failed to assign 1 agent(s)")); }); + it("should ignore model-provided issue_number/pull_number conflict when target is not '*'", async () => { + process.env.GH_AW_AGENT_TARGET = "triggering"; + mockContext.eventName = "issues"; + mockContext.payload = { + issue: { number: 123 }, + }; + mockContext.repo = { + owner: "test-owner", + repo: "test-repo", + }; + + setAgentOutput({ + items: [ + { + type: "assign_to_agent", + issue_number: 42, + pull_number: 99, + agent: "copilot", + }, + ], + errors: [], + }); + + mockGithub.rest.issues.checkUserCanBeAssigned.mockResolvedValueOnce({}); + mockGithub.rest.users.getByUsername.mockResolvedValueOnce({ data: { id: 99999 } }); + mockGithub.rest.issues.get.mockResolvedValueOnce({ + data: { id: 12345, number: 123, assignees: [], html_url: "", title: "", body: "" }, + }); + mockGithub.request.mockResolvedValueOnce({ data: { id: "task-123" } }); + + await eval(`(async () => { ${assignToAgentScript}; ${STANDALONE_RUNNER} })()`); + + // The mutual-exclusivity check should be skipped since target isn't "*", so the + // triggering issue #123 should be used instead of the ignored issue_number/pull_number. + expect(mockCore.error).not.toHaveBeenCalledWith("Cannot specify both issue_number and pull_number in the same assign_to_agent item"); + expect(mockCore.setFailed).not.toHaveBeenCalled(); + }); + it("should auto-resolve issue number from context when not provided (triggering target)", async () => { + process.env.GH_AW_AGENT_TARGET = "triggering"; // Set up context to simulate an issue event mockContext.eventName = "issues"; mockContext.payload = { @@ -935,6 +974,7 @@ describe("assign_to_agent", () => { }); it("should skip when context doesn't match triggering target", async () => { + process.env.GH_AW_AGENT_TARGET = "triggering"; // Set up context that doesn't support triggering target (e.g., push event) mockContext.eventName = "push"; @@ -1413,7 +1453,7 @@ describe("assign_to_agent", () => { const result = await eval(`(async () => { ${assignToAgentScript}; - const _handler = await main({ max: "1", name: "copilot" }); + const _handler = await main({ max: "1", name: "copilot", target: "*" }); const _invalid = await _handler({ type: "assign_to_agent", issue_number: 1, pull_number: 2, agent: "copilot" }, {}, new Map()); const _valid = await _handler({ type: "assign_to_agent", issue_number: 3, agent: "copilot" }, {}, new Map()); return { @@ -1446,7 +1486,7 @@ describe("assign_to_agent", () => { const result = await eval(`(async () => { ${assignToAgentScript}; - const _handler = await main({ max: "2", name: "copilot" }); + const _handler = await main({ max: "2", name: "copilot", target: "*" }); await _handler({ type: "assign_to_agent", issue_number: 1, agent: "copilot" }, {}, new Map()); return { second: _handler({ type: "assign_to_agent", issue_number: 2, agent: "copilot" }, {}, new Map()), @@ -1472,8 +1512,8 @@ describe("assign_to_agent", () => { const result = await eval(`(async () => { ${assignToAgentScript}; - const _handlerA = await main({ max: "5", name: "copilot" }); - const _handlerB = await main({ max: "5", name: "copilot" }); + const _handlerA = await main({ max: "5", name: "copilot", target: "*" }); + const _handlerB = await main({ max: "5", name: "copilot", target: "*" }); await _handlerA({ type: "assign_to_agent", issue_number: 11, agent: "copilot" }, {}, new Map()); return { assignedA: getAssignToAgentAssigned(_handlerA), diff --git a/actions/setup/js/assign_to_user.cjs b/actions/setup/js/assign_to_user.cjs index 718a5e38db5..eb5424b153b 100644 --- a/actions/setup/js/assign_to_user.cjs +++ b/actions/setup/js/assign_to_user.cjs @@ -8,7 +8,7 @@ const { processItems } = require("./safe_output_processor.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); -const { resolveIssueNumber, extractAssignees, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { resolveTarget, extractAssignees, checkRequiredFilter } = require("./safe_output_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { parseBoolTemplatable } = require("./templatable.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); @@ -29,6 +29,7 @@ const main = createCountGatedHandler({ // Extract configuration const allowedAssignees = config.allowed ?? []; const blockedAssignees = config.blocked ?? []; + const targetConfig = config.target || "triggering"; const unassignFirst = parseBoolTemplatable(config.unassign_first, false); const issueIntentEnabled = config.issue_intent !== false; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); @@ -72,16 +73,21 @@ const main = createCountGatedHandler({ const assignItem = message; const intentMetadata = issueIntentEnabled ? normalizeIssueIntentMetadata(assignItem) : {}; - // Determine issue number using shared helper - const issueResult = resolveIssueNumber(assignItem); - if (!issueResult.success) { - core.warning(`Skipping assign_to_user: ${issueResult.error}`); + const targetResult = resolveTarget({ + targetConfig, + item: assignItem, + context, + itemType: HANDLER_TYPE, + supportsIssue: true, + }); + if (!targetResult.success) { + core.warning(`Skipping assign_to_user: ${targetResult.error}`); return { success: false, - error: issueResult.error, + error: targetResult.error, }; } - const issueNumber = issueResult.issueNumber; + const issueNumber = targetResult.number; const filterResult = await checkRequiredFilter(githubClient, repoParts, issueNumber, requiredLabels, requiredTitlePrefix, HANDLER_TYPE); if (filterResult) return filterResult; diff --git a/actions/setup/js/assign_to_user.test.cjs b/actions/setup/js/assign_to_user.test.cjs index 6bb28f84e82..82ec4dd5e20 100644 --- a/actions/setup/js/assign_to_user.test.cjs +++ b/actions/setup/js/assign_to_user.test.cjs @@ -150,6 +150,8 @@ describe("assign_to_user (Handler Factory Architecture)", () => { }); it("should use explicit issue number from message", async () => { + const { main } = require("./assign_to_user.cjs"); + handler = await main({ max: 10, allowed: ["user1"], target: "*" }); mockGithub.rest.issues.addAssignees.mockResolvedValue({}); const message = { @@ -234,7 +236,7 @@ describe("assign_to_user (Handler Factory Architecture)", () => { const result = await handler(message, {}); expect(result.success).toBe(false); - expect(result.error).toContain("No issue number available"); + expect(result.error).toContain("not running in issue context"); expect(mockGithub.rest.issues.addAssignees).not.toHaveBeenCalled(); // Restore context @@ -295,6 +297,7 @@ describe("assign_to_user (Handler Factory Architecture)", () => { const { main } = require("./assign_to_user.cjs"); const targetRepoHandler = await main({ max: 10, + target: "*", "target-repo": "external-org/external-repo", }); const addAssigneesCalls = []; @@ -322,6 +325,7 @@ describe("assign_to_user (Handler Factory Architecture)", () => { const { main } = require("./assign_to_user.cjs"); const crossRepoHandler = await main({ max: 10, + target: "*", "target-repo": "default-org/default-repo", allowed_repos: ["cross-org/cross-repo"], }); @@ -373,6 +377,7 @@ describe("assign_to_user (Handler Factory Architecture)", () => { const { main } = require("./assign_to_user.cjs"); const handler = await main({ max: 10, + target: "*", "target-repo": "github/default-repo", allowed_repos: ["github/gh-aw"], }); diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index c4b0e6f9650..2092e35d10d 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -12,6 +12,7 @@ const { createCloseEntityHandler, buildCommentBody, ISSUE_CONFIG } = require("./ const { loadTemporaryIdMapFromResolved, resolveRepoIssueTarget } = require("./temporary_id.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); +const { resolveTarget } = require("./safe_output_helpers.cjs"); /** * Parse a `duplicate_of` value into { owner, repo, issueNumber }. @@ -246,15 +247,16 @@ async function main(config = {}) { } const { repo: entityRepo, repoParts } = repoResult; - // Determine issue number - either from explicit field or from context - if (item.issue_number !== undefined) { + const targetConfig = config.target || "triggering"; + let targetItem = item; + if (targetConfig === "*" && item.issue_number !== undefined) { // Try to resolve as temporary ID first, then fall back to integer parsing const tempIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds); const resolvedTarget = resolveRepoIssueTarget(item.issue_number, tempIdMap, repoParts.owner, repoParts.repo); if (resolvedTarget.wasTemporaryId && resolvedTarget.resolved) { const issueNumber = resolvedTarget.resolved.number; core.info(`Resolved temporary ID '${item.issue_number}' to #${issueNumber}`); - return { success: true, entityNumber: issueNumber, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; + targetItem = { ...item, issue_number: issueNumber }; } else if (resolvedTarget.wasTemporaryId && !resolvedTarget.resolved) { return { success: false, @@ -262,21 +264,19 @@ async function main(config = {}) { error: resolvedTarget.errorMessage || `Unresolved temporary ID: ${item.issue_number}`, }; } - - // Not a temporary ID - parse as integer - const issueNumber = parseInt(String(item.issue_number), 10); - if (Number.isNaN(issueNumber)) { - return { success: false, error: `Invalid issue number: ${item.issue_number}` }; - } - return { success: true, entityNumber: issueNumber, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; } - // Fall back to context issue number - const contextIssue = context.payload?.issue?.number; - if (!contextIssue) { - return { success: false, error: "No issue number available" }; + const numberResult = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: ISSUE_CONFIG.itemType, + supportsIssue: true, + }); + if (!numberResult.success) { + return { success: false, error: numberResult.error }; } - return { success: true, entityNumber: contextIssue, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; + return { success: true, entityNumber: numberResult.number, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; }, getDetails: getIssueDetails, diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index b31405ce66c..6279d55a33d 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -1,6 +1,7 @@ // @ts-check import { describe, it, expect, beforeEach } from "vitest"; -const { main, parseDuplicateOf } = require("./close_issue.cjs"); +const { main: createHandler, parseDuplicateOf } = require("./close_issue.cjs"); +const main = (config = {}) => createHandler({ target: "*", ...config }); describe("close_issue", () => { let mockCore; @@ -65,6 +66,7 @@ describe("close_issue", () => { }; mockContext = { + eventName: "issues", repo: { owner: "test-owner", repo: "test-repo", @@ -205,7 +207,7 @@ describe("close_issue", () => { }); it("should close an issue from context when issue_number not provided", async () => { - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const updateCalls = []; mockGithub.rest.issues.update = async params => { @@ -239,18 +241,18 @@ describe("close_issue", () => { ); expect(result.success).toBe(false); - expect(result.error).toContain("Invalid issue number"); + expect(result.error).toContain("Invalid item_number/issue_number"); }); it("should handle missing issue_number and no context", async () => { mockContext.payload = {}; - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const result = await handler({ body: "Trying to close" }, {}); expect(result.success).toBe(false); - expect(result.error).toContain("No issue number available"); + expect(result.error).toContain("no issue found"); }); it("should respect max count limit", async () => { diff --git a/actions/setup/js/close_pull_request.cjs b/actions/setup/js/close_pull_request.cjs index 8dbc351dd4a..d24d8560e53 100644 --- a/actions/setup/js/close_pull_request.cjs +++ b/actions/setup/js/close_pull_request.cjs @@ -5,6 +5,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { createCloseEntityHandler, checkLabelFilter, buildCommentBody, PULL_REQUEST_CONFIG } = require("./close_entity_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); +const { resolveTarget } = require("./safe_output_helpers.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -107,20 +108,16 @@ async function main(config = {}) { } const { repo: entityRepo, repoParts } = repoResult; - let prNumber; - if (item.pull_request_number !== undefined) { - prNumber = parseInt(String(item.pull_request_number), 10); - if (Number.isNaN(prNumber)) { - return { success: false, error: `Invalid pull request number: ${item.pull_request_number}` }; - } - } else { - const contextPR = context.payload?.pull_request?.number; - if (!contextPR) { - return { success: false, error: "No pull_request_number provided and not in pull request context" }; - } - prNumber = contextPR; + const targetResult = resolveTarget({ + targetConfig: config.target || "triggering", + item, + context, + itemType: PULL_REQUEST_CONFIG.itemType, + }); + if (!targetResult.success) { + return { success: false, error: targetResult.error }; } - return { success: true, entityNumber: prNumber, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; + return { success: true, entityNumber: targetResult.number, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; }, getDetails: getPullRequestDetails, diff --git a/actions/setup/js/close_pull_request.test.cjs b/actions/setup/js/close_pull_request.test.cjs index 3ff92a49f7b..4a274ee37b2 100644 --- a/actions/setup/js/close_pull_request.test.cjs +++ b/actions/setup/js/close_pull_request.test.cjs @@ -1,6 +1,7 @@ // @ts-check import { describe, it, expect, beforeEach } from "vitest"; -const { main } = require("./close_pull_request.cjs"); +const { main: createHandler } = require("./close_pull_request.cjs"); +const main = (config = {}) => createHandler({ target: "*", ...config }); describe("close_pull_request", () => { let mockCore; @@ -65,6 +66,7 @@ describe("close_pull_request", () => { }; mockContext = { + eventName: "pull_request", repo: { owner: "test-owner", repo: "test-repo", @@ -151,7 +153,7 @@ describe("close_pull_request", () => { }); it("should close a PR from context when pull_request_number not provided", async () => { - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const updateCalls = []; mockGithub.rest.pulls.update = async params => { @@ -185,18 +187,18 @@ describe("close_pull_request", () => { ); expect(result.success).toBe(false); - expect(result.error.includes("Invalid pull request number")).toBe(true); + expect(result.error).toContain("Invalid pull_request_number"); }); it("should handle missing pull_request_number and no context", async () => { mockContext.payload = {}; - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const result = await handler({ body: "Closing" }, {}); expect(result.success).toBe(false); - expect(result.error.includes("No pull_request_number provided")).toBe(true); + expect(result.error).toContain("no pull request found"); }); it("should respect max count limit", async () => { diff --git a/actions/setup/js/hide_comment.cjs b/actions/setup/js/hide_comment.cjs index c20ce4594c0..fdc44f96998 100644 --- a/actions/setup/js/hide_comment.cjs +++ b/actions/setup/js/hide_comment.cjs @@ -10,6 +10,8 @@ const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { ERR_VALIDATION, ERR_API } = require("./error_codes.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); +const { resolveInvocationContext } = require("./invocation_context_helpers.cjs"); /** * Type constant for handler identification @@ -48,7 +50,7 @@ async function hideCommentAPI(github, nodeId, reason = "spam") { * @param {any} github - GitHub client * @param {{owner?: string, repo?: string}|null|undefined} repoContext - Repository context * @param {string|number} commentId - GraphQL node ID or numeric REST comment ID - * @returns {Promise} GraphQL node ID + * @returns {Promise<{nodeId: string, itemNumber: number, repo: string, kind: "issue"|"discussion"}>} Resolved comment target */ async function resolveCommentNodeId(github, repoContext, commentId) { if (typeof commentId === "string") { @@ -57,9 +59,47 @@ async function resolveCommentNodeId(github, repoContext, commentId) { throw new Error(`${ERR_VALIDATION}: comment_id is required`); } - // GraphQL node IDs (e.g., IC_kwDOABCD123456) can be used directly. if (!/^\d+$/.test(trimmed)) { - return trimmed; + const query = /* GraphQL */ ` + query ($nodeId: ID!) { + node(id: $nodeId) { + __typename + ... on IssueComment { + issue { + number + repository { + nameWithOwner + } + } + } + ... on PullRequestReviewComment { + pullRequest { + number + repository { + nameWithOwner + } + } + } + ... on DiscussionComment { + discussion { + number + repository { + nameWithOwner + } + } + } + } + } + `; + const result = await github.graphql(query, { nodeId: trimmed }); + const parent = result?.node?.issue || result?.node?.pullRequest || result?.node?.discussion; + const itemNumber = parent?.number; + const repo = parent?.repository?.nameWithOwner; + if (!Number.isInteger(itemNumber) || itemNumber <= 0 || !repo) { + throw new Error(`${ERR_VALIDATION}: comment_id must reference a comment on an issue, pull request, or discussion`); + } + const kind = result?.node?.discussion ? "discussion" : "issue"; + return { nodeId: trimmed, itemNumber, repo, kind }; } commentId = Number.parseInt(trimmed, 10); @@ -79,12 +119,17 @@ async function resolveCommentNodeId(github, repoContext, commentId) { comment_id: commentId, }); - const nodeId = comment && comment.data ? comment.data.node_id : null; + const nodeId = comment?.data?.node_id; + const issueURL = comment?.data?.issue_url || ""; + const match = String(issueURL).match(/\/repos\/([^/]+)\/([^/]+)\/issues\/(\d+)(?:[#/?]|$)/); if (!nodeId || typeof nodeId !== "string") { throw new Error(`${ERR_API}: Failed to resolve GraphQL node ID for comment_id ${commentId}: comment not found or node_id unavailable`); } + if (!match) { + throw new Error(`${ERR_API}: Failed to resolve parent item for comment_id ${commentId}`); + } - return nodeId; + return { nodeId, itemNumber: Number(match[3]), repo: `${match[1]}/${match[2]}`, kind: "issue" }; } /** @@ -96,6 +141,8 @@ async function main(config = {}) { // Extract configuration const allowedReasons = config.allowed_reasons || []; const maxCount = config.max || 5; + const targetConfig = config.target || "triggering"; + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); // Check if we're in staged mode @@ -136,6 +183,13 @@ async function main(config = {}) { error: "comment_id is required", }; } + const isNumericCommentId = typeof commentId === "number" || (typeof commentId === "string" && /^\d+$/.test(commentId.trim())); + if (isNumericCommentId && (!context?.repo?.owner || !context?.repo?.repo)) { + return { + success: false, + error: "Unable to resolve numeric comment_id: repository context (owner/repo) is not available", + }; + } // Normalize reason to uppercase for GitHub API const normalizedReason = (message.reason || "SPAM").toUpperCase(); @@ -152,6 +206,58 @@ async function main(config = {}) { } } + const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "comment"); + if (!repoResult.success) { + return { success: false, error: repoResult.error }; + } + const selectedRepo = repoResult.repo; + const resolvedComment = await resolveCommentNodeId(githubClient, repoResult.repoParts, commentId); + if (resolvedComment.repo.toLowerCase() !== selectedRepo.toLowerCase()) { + return { + success: false, + error: `Comment belongs to repository ${resolvedComment.repo}, but target repository is ${selectedRepo}`, + }; + } + + let expectedNumber; + let expectedKind; + if (targetConfig === "triggering") { + const invocationContext = resolveInvocationContext(context); + if (invocationContext.eventPayload?.discussion?.number) { + expectedNumber = invocationContext.eventPayload.discussion.number; + expectedKind = "discussion"; + } else { + expectedNumber = invocationContext.eventPayload?.issue?.number ?? invocationContext.eventPayload?.pull_request?.number; + expectedKind = "issue"; + } + if (!expectedNumber) { + return { + success: false, + error: 'Target is "triggering" but not running in issue, pull request, or discussion context', + }; + } + } else if (targetConfig !== "*") { + expectedNumber = Number(targetConfig); + if (!Number.isInteger(expectedNumber) || expectedNumber <= 0) { + return { + success: false, + error: `Invalid target configuration: ${targetConfig}`, + }; + } + } + if (expectedNumber && resolvedComment.itemNumber !== expectedNumber) { + return { + success: false, + error: `Comment belongs to item #${resolvedComment.itemNumber}, but target is #${expectedNumber}`, + }; + } + if (expectedKind && resolvedComment.kind !== expectedKind) { + return { + success: false, + error: `Comment belongs to a ${resolvedComment.kind === "discussion" ? "discussion" : "issue/pull request"}, but the triggering item is a ${expectedKind === "discussion" ? "discussion" : "issue/pull request"} (#${expectedNumber})`, + }; + } + core.info(`Hiding comment: ${commentId} (reason: ${normalizedReason})`); // If in staged mode, preview without executing @@ -162,19 +268,20 @@ async function main(config = {}) { staged: true, previewInfo: { commentId, + itemNumber: resolvedComment.itemNumber, + repo: resolvedComment.repo, reason: normalizedReason, }, }; } - const resolvedNodeId = await resolveCommentNodeId(githubClient, context && context.repo ? context.repo : null, commentId); - const hideResult = await hideCommentAPI(githubClient, resolvedNodeId, normalizedReason); + const hideResult = await hideCommentAPI(githubClient, resolvedComment.nodeId, normalizedReason); if (hideResult.isMinimized) { - core.info(`Successfully hidden comment: ${resolvedNodeId}`); + core.info(`Successfully hidden comment: ${resolvedComment.nodeId}`); return { success: true, - comment_id: resolvedNodeId, + comment_id: resolvedComment.nodeId, is_hidden: true, }; } else { diff --git a/actions/setup/js/hide_comment.test.cjs b/actions/setup/js/hide_comment.test.cjs index 40e1c0f0a14..5244680be09 100644 --- a/actions/setup/js/hide_comment.test.cjs +++ b/actions/setup/js/hide_comment.test.cjs @@ -38,12 +38,27 @@ describe("hide_comment.cjs", () => { vi.resetAllMocks(); delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; - // Default successful graphql mock - mockGithub.graphql.mockResolvedValue({ - minimizeComment: { minimizedComment: { isMinimized: true } }, + mockGithub.graphql.mockImplementation(query => { + if (query.includes("query ($nodeId")) { + return Promise.resolve({ + node: { + __typename: "IssueComment", + issue: { + number: 42, + repository: { nameWithOwner: "testowner/testrepo" }, + }, + }, + }); + } + return Promise.resolve({ + minimizeComment: { minimizedComment: { isMinimized: true } }, + }); }); mockGithub.rest.issues.getComment.mockResolvedValue({ - data: { node_id: "IC_kwDOABCD123456" }, + data: { + node_id: "IC_kwDOABCD123456", + issue_url: "https://api.github.com/repos/testowner/testrepo/issues/42", + }, }); }); @@ -196,6 +211,35 @@ describe("hide_comment.cjs", () => { expect(mockGithub.graphql).not.toHaveBeenCalled(); }); + it("should reject a discussion comment for target: triggering when triggered by an issue with the same number", async () => { + // Issue #42 triggered the run, but the resolved comment belongs to discussion #42 + // (discussions use a separate numbering sequence from issues/PRs). + mockGithub.graphql.mockImplementation(query => { + if (query.includes("query ($nodeId")) { + return Promise.resolve({ + node: { + __typename: "DiscussionComment", + discussion: { + number: 42, + repository: { nameWithOwner: "testowner/testrepo" }, + }, + }, + }); + } + return Promise.resolve({ + minimizeComment: { minimizedComment: { isMinimized: true } }, + }); + }); + + const { main } = await loadModule(); + const handler = await main({ target: "triggering" }); + + const result = await handler({ comment_id: "IC_someDiscussionCommentId", reason: "SPAM" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("discussion"); + }); + it("should enforce max count limit", async () => { const { main } = await loadModule(); const handler = await main({ max: 2 }); @@ -206,7 +250,7 @@ describe("hide_comment.cjs", () => { expect(result.success).toBe(false); expect(result.error).toContain("Max count"); - expect(mockGithub.graphql).toHaveBeenCalledTimes(2); + expect(mockGithub.graphql).toHaveBeenCalledTimes(4); }); it("should reject reason not in allowed-reasons list", async () => { @@ -243,8 +287,21 @@ describe("hide_comment.cjs", () => { it("should return failure when comment is not minimized", async () => { const { main } = await loadModule(); - mockGithub.graphql.mockResolvedValue({ - minimizeComment: { minimizedComment: { isMinimized: false } }, + mockGithub.graphql.mockImplementation(query => { + if (query.includes("query ($nodeId")) { + return Promise.resolve({ + node: { + __typename: "IssueComment", + issue: { + number: 42, + repository: { nameWithOwner: "testowner/testrepo" }, + }, + }, + }); + } + return Promise.resolve({ + minimizeComment: { minimizedComment: { isMinimized: false } }, + }); }); const handler = await main(); @@ -265,7 +322,8 @@ describe("hide_comment.cjs", () => { expect(result.staged).toBe(true); expect(result.previewInfo?.commentId).toBe("IC_kwDOABCD123456"); expect(result.previewInfo?.reason).toBe("ABUSE"); - expect(mockGithub.graphql).not.toHaveBeenCalled(); + expect(mockGithub.graphql).toHaveBeenCalledTimes(1); + expect(mockGithub.graphql).not.toHaveBeenCalledWith(expect.stringContaining("minimizeComment"), expect.any(Object)); }); }); }); diff --git a/actions/setup/js/link_sub_issue.cjs b/actions/setup/js/link_sub_issue.cjs index 259dfd458ac..ee65ecf3438 100644 --- a/actions/setup/js/link_sub_issue.cjs +++ b/actions/setup/js/link_sub_issue.cjs @@ -7,6 +7,9 @@ const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); +const { resolveTarget } = require("./safe_output_helpers.cjs"); + +const HANDLER_TYPE = "link_sub_issue"; const { linkSubIssue } = require("./sub_issue_helpers.cjs"); /** @@ -22,6 +25,7 @@ async function main(config = {}) { const subRequiredLabels = config.sub_required_labels || []; const subTitlePrefix = config.sub_title_prefix || ""; const maxCount = config.max || 5; + const targetConfig = config.target || "triggering"; const githubClient = await createAuthenticatedGitHubClient(config); // Check if we're in staged mode @@ -94,13 +98,20 @@ async function main(config = {}) { // Convert resolvedTemporaryIds to a normalized Map for resolveIssueNumber const temporaryIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds); - // Resolve issue numbers, supporting temporary IDs from create_issue job - const parentResolved = resolveRepoIssueTarget(item.parent_issue_number, temporaryIdMap, itemOwner, itemRepo); + // Resolve the sub-issue from model output. The configured target controls the parent. const subResolved = resolveRepoIssueTarget(item.sub_issue_number, temporaryIdMap, itemOwner, itemRepo); + let parentResolved; + let targetItem = item; + if (targetConfig === "*") { + parentResolved = resolveRepoIssueTarget(item.parent_issue_number, temporaryIdMap, itemOwner, itemRepo); + if (parentResolved.resolved) { + targetItem = { ...item, issue_number: parentResolved.resolved.number }; + } + } // Check if either parent or sub issue is an unresolved temporary ID // If so, defer the operation to allow for resolution later - const hasUnresolvedParent = parentResolved.wasTemporaryId && !parentResolved.resolved; + const hasUnresolvedParent = parentResolved?.wasTemporaryId && !parentResolved.resolved; const hasUnresolvedSub = subResolved.wasTemporaryId && !subResolved.resolved; if (hasUnresolvedParent || hasUnresolvedSub) { @@ -124,7 +135,7 @@ async function main(config = {}) { } // Check for other resolution errors (non-temporary ID issues) - if (parentResolved.errorMessage) { + if (parentResolved?.errorMessage) { core.warning(`Failed to resolve parent issue: ${parentResolved.errorMessage}`); return { parent_issue_number: item.parent_issue_number, @@ -144,8 +155,23 @@ async function main(config = {}) { }; } - const parentIssueNumber = parentResolved.resolved?.number; const subIssueNumber = subResolved.resolved?.number; + const parentTarget = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: HANDLER_TYPE, + supportsIssue: true, + }); + if (!parentTarget.success) { + return { + parent_issue_number: item.parent_issue_number, + sub_issue_number: item.sub_issue_number, + success: false, + error: parentTarget.error, + }; + } + const parentIssueNumber = parentTarget.number; if (!parentIssueNumber || !subIssueNumber) { core.error("Internal error: Issue numbers are undefined after successful resolution"); @@ -157,16 +183,19 @@ async function main(config = {}) { }; } - if (parentResolved.wasTemporaryId && parentResolved.resolved) { + if (parentResolved?.wasTemporaryId && parentResolved.resolved) { core.info(`Resolved parent temporary ID '${item.parent_issue_number}' to ${parentResolved.resolved.owner}/${parentResolved.resolved.repo}#${parentIssueNumber}`); } if (subResolved.wasTemporaryId && subResolved.resolved) { core.info(`Resolved sub-issue temporary ID '${item.sub_issue_number}' to ${subResolved.resolved.owner}/${subResolved.resolved.repo}#${subIssueNumber}`); } + const owner = parentResolved?.resolved?.owner || itemOwner; + const repo = parentResolved?.resolved?.repo || itemRepo; + // Sub-issue linking is only supported within the same repository. - if (parentResolved.resolved && subResolved.resolved) { - const parentRepoSlug = `${parentResolved.resolved.owner}/${parentResolved.resolved.repo}`; + if (subResolved.resolved) { + const parentRepoSlug = `${owner}/${repo}`; const subRepoSlug = `${subResolved.resolved.owner}/${subResolved.resolved.repo}`; if (parentRepoSlug !== subRepoSlug) { const error = `Parent and sub-issue must be in the same repository for link_sub_issue (got ${parentRepoSlug} and ${subRepoSlug})`; @@ -178,10 +207,20 @@ async function main(config = {}) { error, }; } - } - const owner = parentResolved.resolved?.owner || itemOwner; - const repo = parentResolved.resolved?.repo || itemRepo; + // Validate the effective parent/sub-issue pair (after applying `target`), not the + // raw model-provided values which may have been overridden by a fixed/triggering target. + if (parentRepoSlug === subRepoSlug && parentIssueNumber === subIssueNumber) { + const error = `Parent and sub-issue must be different (both resolve to #${parentIssueNumber})`; + core.warning(error); + return { + parent_issue_number: item.parent_issue_number, + sub_issue_number: item.sub_issue_number, + success: false, + error, + }; + } + } // Fetch parent issue to validate filters let parentIssue; diff --git a/actions/setup/js/link_sub_issue.test.cjs b/actions/setup/js/link_sub_issue.test.cjs index 7ba753ce2f9..489af321204 100644 --- a/actions/setup/js/link_sub_issue.test.cjs +++ b/actions/setup/js/link_sub_issue.test.cjs @@ -44,6 +44,7 @@ const mockCore = { // Create handler with default config handler = await main({ max: 5, + target: "*", parent_required_labels: [], parent_title_prefix: "", sub_required_labels: [], @@ -137,7 +138,7 @@ const mockCore = { }); it("should handle max count limit", async () => { // Create handler with max=1 - const limitedHandler = await require(path.join(process.cwd(), "link_sub_issue.cjs")).main({ max: 1 }); + const limitedHandler = await require(path.join(process.cwd(), "link_sub_issue.cjs")).main({ max: 1, target: "*" }); const message1 = { type: "link_sub_issue", parent_issue_number: 100, sub_issue_number: 50 }; const message2 = { type: "link_sub_issue", parent_issue_number: 100, sub_issue_number: 51 }; @@ -212,11 +213,41 @@ const mockCore = { expect(mockGithub.graphql).not.toHaveBeenCalled(); }); + it("should reject a cross-repository sub-issue for a fixed parent target", async () => { + const { main } = require(path.join(process.cwd(), "link_sub_issue.cjs")); + const fixedTargetHandler = await main({ max: 5, target: "100" }); + const resolvedIds = { + aw_456789ab: { repo: "other-org/other-repo", number: 50 }, + }; + + const result = await fixedTargetHandler({ type: "link_sub_issue", parent_issue_number: 999, sub_issue_number: "aw_456789ab" }, resolvedIds); + + expect(result.success).toBe(false); + expect(result.error).toContain("must be in the same repository"); + expect(mockGithub.rest.issues.get).not.toHaveBeenCalled(); + expect(mockGithub.graphql).not.toHaveBeenCalled(); + }); + + it("should reject when a fixed parent target equals the sub-issue, even though the raw model parent differs", async () => { + const { main } = require(path.join(process.cwd(), "link_sub_issue.cjs")); + // The configured target (#50) is the same as the requested sub-issue (#50), so applying + // the target would create a self-link even though the model-provided parent (999) differs. + const fixedTargetHandler = await main({ max: 5, target: "50" }); + + const result = await fixedTargetHandler({ type: "link_sub_issue", parent_issue_number: 999, sub_issue_number: 50 }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("must be different"); + expect(mockGithub.rest.issues.get).not.toHaveBeenCalled(); + expect(mockGithub.graphql).not.toHaveBeenCalled(); + }); + it("should use target-repo config as default for issue resolution", async () => { const { main } = require(path.join(process.cwd(), "link_sub_issue.cjs")); const handlerWithTarget = await main({ max: 5, "target-repo": "external-org/external-repo", + target: "*", }); mockGithub.rest.issues.get diff --git a/actions/setup/js/mark_pull_request_as_ready_for_review.cjs b/actions/setup/js/mark_pull_request_as_ready_for_review.cjs index 23b4e7cf1cc..d83c04d8c0e 100644 --- a/actions/setup/js/mark_pull_request_as_ready_for_review.cjs +++ b/actions/setup/js/mark_pull_request_as_ready_for_review.cjs @@ -9,7 +9,7 @@ const { generateFooterWithMessages, getDetectionCautionAlert } = require("./mess const { sanitizeContent } = require("./sanitize_content.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); -const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); @@ -102,6 +102,7 @@ async function markPullRequestAsReadyForReview(github, pullRequestNodeId) { async function main(config = {}) { // Extract configuration const maxCount = config.max || 10; + const targetConfig = config.target || "triggering"; const githubClient = await createAuthenticatedGitHubClient(config); // Check if we're in staged mode @@ -150,29 +151,20 @@ async function main(config = {}) { const prOwner = repoResult.repoParts.owner; const prRepo = repoResult.repoParts.repo; - // Determine PR number - let prNumber; - if (item.pull_request_number !== undefined) { - prNumber = parseInt(String(item.pull_request_number), 10); - if (Number.isNaN(prNumber)) { - core.warning(`Invalid pull_request_number: ${item.pull_request_number}`); - return { - success: false, - error: `Invalid pull_request_number: ${item.pull_request_number}`, - }; - } - } else { - // Use context PR if available - const contextPR = context.payload?.pull_request?.number; - if (!contextPR) { - core.warning("No pull_request_number provided and not in pull request context"); - return { - success: false, - error: "No pull request number available", - }; - } - prNumber = contextPR; + const targetResult = resolveTarget({ + targetConfig, + item, + context, + itemType: HANDLER_TYPE, + }); + if (!targetResult.success) { + core.warning(targetResult.error); + return { + success: false, + error: targetResult.error, + }; } + const prNumber = targetResult.number; const repoParts = { owner: prOwner, repo: prRepo }; const filterResult = await checkRequiredFilter(githubClient, repoParts, prNumber, requiredLabels, requiredTitlePrefix, "mark_pull_request_as_ready_for_review"); diff --git a/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs b/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs index f08ef5a874e..ba651c38bfa 100644 --- a/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs +++ b/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs @@ -115,7 +115,7 @@ describe("mark_pull_request_as_ready_for_review", () => { describe("handleMarkPullRequestAsReadyForReview", () => { it("should use GraphQL mutation to mark a draft PR as ready for review", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "All tests passing" }, {}); @@ -127,7 +127,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should NOT call REST pulls.update (the broken endpoint)", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); await handler({ pull_request_number: 42, reason: "Ready!" }, {}); @@ -136,7 +136,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should use context PR number when pull_request_number is not provided", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const result = await handler({ reason: "Ready for review" }, {}); @@ -146,7 +146,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should return success with correct fields on success", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "LGTM" }, {}); @@ -160,7 +160,7 @@ describe("mark_pull_request_as_ready_for_review", () => { mockRestPullsGet.mockResolvedValue({ data: makePR(42, { draft: false }) }); const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "Already ready" }, {}); @@ -183,7 +183,7 @@ describe("mark_pull_request_as_ready_for_review", () => { }); const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "Ready" }, {}); @@ -193,7 +193,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should add a comment after successfully marking ready for review", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); await handler({ pull_request_number: 42, reason: "All checks passing" }, {}); @@ -209,7 +209,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should return failure for invalid pull_request_number", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: "not-a-number", reason: "Ready" }, {}); @@ -222,19 +222,19 @@ describe("mark_pull_request_as_ready_for_review", () => { global.context.payload = { repository: { html_url: "https://github.com/test-owner/test-repo" } }; const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "triggering" }); const result = await handler({ reason: "Ready" }, {}); expect(result.success).toBe(false); - expect(result.error).toContain("No pull request number available"); + expect(result.error).toContain("no pull request found"); global.context.payload = originalPayload; }); it("should return failure when reason is missing", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42 }, {}); @@ -244,7 +244,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should return failure when reason is empty string", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "" }, {}); @@ -254,7 +254,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should return failure when reason is whitespace only", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: " " }, {}); @@ -264,7 +264,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should respect max count limit", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 2 }); + const handler = await main({ max: 2, target: "*" }); // First two succeed const r1 = await handler({ pull_request_number: 42, reason: "Ready 1" }, {}); @@ -282,7 +282,7 @@ describe("mark_pull_request_as_ready_for_review", () => { mockGraphql.mockRejectedValue(new Error("GraphQL request failed: Resource not accessible by integration")); const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "Ready" }, {}); @@ -294,7 +294,7 @@ describe("mark_pull_request_as_ready_for_review", () => { mockRestPullsGet.mockRejectedValue(new Error("Not Found")); const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "Ready" }, {}); @@ -308,7 +308,7 @@ describe("mark_pull_request_as_ready_for_review", () => { }); const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); await handler({ pull_request_number: 42, reason: "Ready" }, {}); @@ -317,7 +317,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should use staged mode without executing GraphQL mutation", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10, staged: true }); + const handler = await main({ max: 10, staged: true, target: "*" }); const result = await handler({ pull_request_number: 42, reason: "Staged test" }, {}); @@ -333,6 +333,7 @@ describe("mark_pull_request_as_ready_for_review", () => { const handler = await main({ max: 10, "target-repo": "external-org/external-repo", + target: "*", }); setupDefaultMocks(42); @@ -346,7 +347,7 @@ describe("mark_pull_request_as_ready_for_review", () => { it("should use context.repo as default when no target-repo configured", async () => { const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); - const handler = await main({ max: 10 }); + const handler = await main({ max: 10, target: "*" }); setupDefaultMocks(42); @@ -362,6 +363,7 @@ describe("mark_pull_request_as_ready_for_review", () => { max: 10, "target-repo": "default-org/default-repo", allowed_repos: ["cross-org/cross-repo"], + target: "*", }); setupDefaultMocks(42); diff --git a/actions/setup/js/merge_pull_request.cjs b/actions/setup/js/merge_pull_request.cjs index 841e12b937f..15f55737392 100644 --- a/actions/setup/js/merge_pull_request.cjs +++ b/actions/setup/js/merge_pull_request.cjs @@ -5,7 +5,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { globPatternToRegex } = require("./glob_pattern_helpers.cjs"); -const { isStagedMode } = require("./safe_output_helpers.cjs"); +const { isStagedMode, resolveTarget } = require("./safe_output_helpers.cjs"); const { selectLatestRelevantChecks } = require("./check_runs_helpers.cjs"); const { withRetry, isTransientError } = require("./error_recovery.cjs"); const { normalizeBranchName } = require("./normalize_branch_name.cjs"); @@ -361,6 +361,7 @@ async function main(config = {}) { const githubClient = await createAuthenticatedGitHubClient(config); const isStaged = isStagedMode(config); const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + const targetConfig = config.target || "triggering"; const maxCount = Number(config.max || 1); const requiredLabels = Array.isArray(config.required_labels) ? config.required_labels : []; const requiredTitlePrefix = config.required_title_prefix || ""; @@ -389,15 +390,30 @@ async function main(config = {}) { const { owner, repo } = repoResult.repoParts; core.info(`Resolved target repository: ${owner}/${repo}`); - const pullNumberResolution = resolvePullRequestNumber(message, resolvedTemporaryIds); - if (!pullNumberResolution.success) { - core.error(pullNumberResolution.error); - return { success: false, error: pullNumberResolution.error }; + let targetItem = message; + if (targetConfig === "*" && message?.pull_request_number != null) { + const pullNumberResolution = resolvePullRequestNumber(message, resolvedTemporaryIds); + if (!pullNumberResolution.success) { + core.error(pullNumberResolution.error); + return { success: false, error: pullNumberResolution.error }; + } + targetItem = { ...message, pull_request_number: pullNumberResolution.pullNumber }; + if (pullNumberResolution.fromTemporaryId) { + core.info(`Resolved temporary ID '${String(message.pull_request_number)}' to pull request #${pullNumberResolution.pullNumber}`); + } } - const pullNumber = pullNumberResolution.pullNumber; - if (pullNumberResolution.fromTemporaryId) { - core.info(`Resolved temporary ID '${String(message?.pull_request_number)}' to pull request #${pullNumber}`); + + const targetResult = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: "merge_pull_request", + }); + if (!targetResult.success) { + core.error(targetResult.error); + return { success: false, error: targetResult.error }; } + const pullNumber = targetResult.number; core.info(`Target PR number: ${pullNumber}`); /** @type {Array<{code: string, message: string, details?: any}>} */ diff --git a/actions/setup/js/merge_pull_request.test.cjs b/actions/setup/js/merge_pull_request.test.cjs index 1a3e014373b..29b7cad1eb9 100644 --- a/actions/setup/js/merge_pull_request.test.cjs +++ b/actions/setup/js/merge_pull_request.test.cjs @@ -227,7 +227,7 @@ describe("merge_pull_request branch validation", () => { }; const { main } = await import("./merge_pull_request.cjs"); - const handler = await main({ "target-repo": "github/gh-aw" }); + const handler = await main({ "target-repo": "github/gh-aw", target: "*" }); const result = await handler({ pull_request_number: 100 }, {}); expect(result.success).toBe(false); diff --git a/actions/setup/js/resolve_pr_review_thread.cjs b/actions/setup/js/resolve_pr_review_thread.cjs index 1ff0968bee3..641d32bbc79 100644 --- a/actions/setup/js/resolve_pr_review_thread.cjs +++ b/actions/setup/js/resolve_pr_review_thread.cjs @@ -11,6 +11,7 @@ const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { resolveTargetRepoConfig, validateTargetRepo } = require("./repo_helpers.cjs"); +const { resolveInvocationContext } = require("./invocation_context_helpers.cjs"); /** * Type constant for handler identification @@ -322,8 +323,11 @@ async function main(config = {}) { const githubClient = await createAuthenticatedGitHubClient(config); - // Determine the triggering PR number from context - const triggeringPRNumber = getPRNumber(context.payload); + // Determine the triggering PR number from context, resolving forwarded invocations + // (workflow_dispatch inputs/aw_context, repository_dispatch.client_payload) the same + // way resolveTarget does. + const invocationContext = resolveInvocationContext(context); + const triggeringPRNumber = getPRNumber(invocationContext.eventPayload); // Check if we're in staged mode const isStaged = isStagedMode(config); @@ -419,99 +423,53 @@ async function main(config = {}) { core.info(`Resolved review comment ${threadId} to review thread ${resolvedThreadId}`); } - // When the user explicitly configured target-repo or allowed-repos, validate the thread's - // repository using validateTargetRepo (supports wildcards like "*", "org/*"). - // Otherwise, fall back to the legacy behavior of scoping to the triggering PR only. - if (hasExplicitTargetConfig) { - // Cross-repo mode: validate thread repo against configured repos (fail closed if missing) - if (!threadRepo) { - core.warning(`Could not determine repository for thread ${resolvedThreadId}`); - return { - success: false, - error: `Could not determine the repository for thread ${resolvedThreadId}`, - }; - } - const repoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); - if (!repoValidation.valid) { - core.warning(`Thread ${resolvedThreadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); + if (!threadRepo) { + core.warning(`Could not determine repository for thread ${resolvedThreadId}`); + return { + success: false, + error: `Could not determine the repository for thread ${resolvedThreadId}`, + }; + } + const repoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); + if (!repoValidation.valid) { + core.warning(`Thread ${resolvedThreadId} belongs to repo ${threadRepo}, which is not allowed`); + return { + success: false, + skipped: !hasExplicitTargetConfig, + thread_id: resolvedThreadId, + error: repoValidation.error, + }; + } + + if (resolveTarget === "triggering") { + if (!triggeringPRNumber) { + core.warning("Cannot resolve review thread: not running in a pull request context"); return { success: false, - error: repoValidation.error, + error: "Cannot resolve review threads outside of a pull request context", }; } - - // Determine target PR number based on target config - if (resolveTarget === "triggering") { - if (!triggeringPRNumber) { - core.warning("Cannot resolve review thread: not running in a pull request context"); - return { - success: false, - error: "Cannot resolve review threads outside of a pull request context", - }; - } - if (threadPRNumber !== triggeringPRNumber) { - core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); - return { - success: false, - error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, - }; - } - } else if (resolveTarget !== "*") { - // Explicit PR number target - const targetPRNumber = parseInt(resolveTarget, 10); - if (Number.isNaN(targetPRNumber) || targetPRNumber <= 0) { - core.warning(`Invalid target PR number: '${resolveTarget}'`); - return { - success: false, - error: `Invalid target: '${resolveTarget}' - must be 'triggering', '*', or a positive integer`, - }; - } - if (threadPRNumber !== targetPRNumber) { - core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); - return { - success: false, - error: `Thread belongs to PR #${threadPRNumber}, but target is PR #${targetPRNumber}`, - }; - } - } - // resolveTarget === "*": any PR in allowed repos — no further PR number check needed - } else { - // Default (legacy) mode: always validate thread repo against defaultTargetRepo to stay - // least-privilege, even when there is no triggering PR (e.g. schedule/workflow_dispatch). - if (!threadRepo) { - core.warning(`Unable to determine repository for review thread ${resolvedThreadId}; refusing to resolve in legacy mode`); + if (threadPRNumber !== triggeringPRNumber) { + core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); return { success: false, - error: `Unable to determine repository for review thread ${resolvedThreadId}`, + error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, }; } - - const legacyRepoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); - if (!legacyRepoValidation.valid) { - // In legacy mode, no cross-repo behavior was ever configured, so a thread_id resolving - // to an unrelated repository almost always indicates a stale or malformed ID (e.g. a - // hallucinated GraphQL node ID) rather than a genuine cross-repo access attempt. Treat - // this the same as an already-resolved/stale thread (skipped) so a single bad ID does - // not fail the entire safe_outputs job, while still refusing to perform the action. - core.warning(`Thread ${resolvedThreadId} repository ${threadRepo} is not allowed in legacy mode; skipping`); + } else if (resolveTarget !== "*") { + const targetPRNumber = parseInt(resolveTarget, 10); + if (Number.isNaN(targetPRNumber) || targetPRNumber <= 0) { + core.warning(`Invalid target PR number: '${resolveTarget}'`); return { success: false, - skipped: true, - thread_id: resolvedThreadId, - error: legacyRepoValidation.error || `Repository ${threadRepo} is not allowed for this handler`, + error: `Invalid target: '${resolveTarget}' - must be 'triggering', '*', or a positive integer`, }; } - - // Scope to triggering PR only when a triggering PR exists - if (!triggeringPRNumber) { - // No triggering PR (e.g. schedule/workflow_dispatch trigger), but the thread has been - // resolved to a specific allowed repository via the API — allow the resolution to proceed - core.info(`No triggering PR context; resolving thread ${resolvedThreadId} via explicit thread_id (PR #${threadPRNumber} in ${threadRepo})`); - } else if (threadPRNumber !== triggeringPRNumber) { - core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); + if (threadPRNumber !== targetPRNumber) { + core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); return { success: false, - error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, + error: `Thread belongs to PR #${threadPRNumber}, but target is PR #${targetPRNumber}`, }; } } diff --git a/actions/setup/js/resolve_pr_review_thread.test.cjs b/actions/setup/js/resolve_pr_review_thread.test.cjs index 5b2e83137cc..42579d39d5f 100644 --- a/actions/setup/js/resolve_pr_review_thread.test.cjs +++ b/actions/setup/js/resolve_pr_review_thread.test.cjs @@ -128,6 +128,41 @@ describe("resolve_pr_review_thread", () => { expect(result.error).toContain("triggering PR #42"); }); + it("should resolve the triggering PR from a forwarded workflow_dispatch aw_context invocation", async () => { + // Thread belongs to PR #42, and the run was forwarded via workflow_dispatch with + // aw_context describing PR #42 as the triggering item, rather than context.payload.pull_request. + mockGraphqlForThread(42); + + global.context = { + ...mockContext, + eventName: "workflow_dispatch", + payload: { + inputs: { + aw_context: JSON.stringify({ + event_type: "issue_comment", + item_type: "pull_request", + item_number: "42", + repo: "test-owner/test-repo", + }), + }, + }, + }; + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ max: 10 }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOForwardedThread", + }; + + const result = await freshHandler(message, {}); + + global.context = mockContext; + + expect(result.success).toBe(true); + }); + it("should succeed as a no-op when thread is not found (stale or already resolved)", async () => { mockGraphql.mockImplementation(query => { if (query.includes("resolveReviewThread")) { @@ -518,7 +553,7 @@ describe("resolve_pr_review_thread", () => { expect(mockGraphql).toHaveBeenCalledTimes(1); }); - it("should succeed when not in a pull request context but explicit thread_id is provided", async () => { + it("should reject an explicit thread when triggering PR context is unavailable", async () => { // Override context to non-PR event (afterEach restores the original payload) global.context.payload = { repository: { html_url: "https://github.com/test-owner/test-repo" }, @@ -534,13 +569,11 @@ describe("resolve_pr_review_thread", () => { const result = await freshHandler(message, {}); - // Should succeed: thread_id was explicitly provided and resolved to a PR via the API - expect(result.success).toBe(true); - expect(result.thread_id).toBe("PRRT_kwDOABCD123456"); - expect(result.is_resolved).toBe(true); + expect(result.success).toBe(false); + expect(result.error).toContain("outside of a pull request context"); }); - it("should succeed when triggered by a schedule event (no PR context) with explicit thread_id", async () => { + it("should reject a schedule-triggered thread when target defaults to triggering", async () => { // Simulate a schedule-triggered workflow (no pull_request in payload) global.context.payload = {}; @@ -556,12 +589,9 @@ describe("resolve_pr_review_thread", () => { const result = await freshHandler(message, {}); - expect(result.success).toBe(true); - expect(result.thread_id).toBe("PRRT_kwDOSchedule77"); - expect(result.is_resolved).toBe(true); - // Should have made two GraphQL calls: thread lookup + resolve mutation - expect(mockGraphql).toHaveBeenCalledTimes(2); - expect(mockGraphql).toHaveBeenCalledWith(expect.stringContaining("resolveReviewThread"), expect.objectContaining({ threadId: "PRRT_kwDOSchedule77" })); + expect(result.success).toBe(false); + expect(result.error).toContain("outside of a pull request context"); + expect(mockGraphql).toHaveBeenCalledTimes(1); }); it("should fail in legacy mode when schedule-triggered and thread repo cannot be determined", async () => { @@ -589,7 +619,7 @@ describe("resolve_pr_review_thread", () => { const result = await freshHandler(message, {}); expect(result.success).toBe(false); - expect(result.error).toContain("Unable to determine repository"); + expect(result.error).toContain("determine the repository"); }); it("should skip (not fail fatally) in legacy mode when schedule-triggered and thread belongs to a different repo", async () => { diff --git a/actions/setup/js/safe_output_helpers.cjs b/actions/setup/js/safe_output_helpers.cjs index 2b99e3236c2..353b196a828 100644 --- a/actions/setup/js/safe_output_helpers.cjs +++ b/actions/setup/js/safe_output_helpers.cjs @@ -63,8 +63,8 @@ function parseMaxCount(envValue, defaultValue = 3) { * @param {any} params.item - Safe output item with optional item_number, issue_number, or pull_request_number * @param {any} params.context - GitHub Actions context * @param {string} params.itemType - Type of item being processed (for error messages) - * @param {boolean} params.supportsPR - When true, handler supports BOTH issues and PRs (e.g., add_labels) - * When false, handler supports PRs ONLY (e.g., add_reviewers) + * @param {boolean} [params.supportsPR] - When true, handler supports BOTH issues and PRs (e.g., add_labels) + * When false, handler supports PRs ONLY (e.g., add_reviewers) * @param {boolean} [params.supportsIssue] - When true, handler supports issues ONLY (e.g., update_issue) * Optional; defaults to false. * @returns {{success: true, number: number, contextType: string} | {success: false, error: string, shouldFail: boolean}} Resolution result @@ -128,6 +128,7 @@ function resolveTarget(params) { // Resolve target number let itemNumber; let contextType; + let authorizedTargetNumber; if (target === "*") { // Use item_number, issue_number, or pull_request_number (aliases: pr_number, pr, pull_number) from item @@ -211,12 +212,14 @@ function resolveTarget(params) { shouldFail: true, }; } + authorizedTargetNumber = itemNumber; contextType = supportsPR || supportsIssue ? "issue" : "pull request"; } else { // Use triggering context if (isIssueContext) { if (effectivePayload.issue) { itemNumber = effectivePayload.issue.number; + authorizedTargetNumber = itemNumber; contextType = "issue"; } else { return { @@ -228,9 +231,11 @@ function resolveTarget(params) { } else if (isPRContext) { if (effectivePayload.pull_request) { itemNumber = effectivePayload.pull_request.number; + authorizedTargetNumber = itemNumber; contextType = "pull request"; } else if (isIssueCommentOnPR) { itemNumber = effectivePayload.issue.number; + authorizedTargetNumber = itemNumber; contextType = "pull request"; } else { return { @@ -251,6 +256,16 @@ function resolveTarget(params) { }; } + const authorizationInvariant = assertTargetAuthorizationInvariant({ + targetConfig: target, + resolvedNumber: itemNumber, + authorizedNumber: authorizedTargetNumber, + itemType, + }); + if (!authorizationInvariant.success) { + return authorizationInvariant; + } + return { success: true, number: itemNumber, @@ -258,6 +273,44 @@ function resolveTarget(params) { }; } +/** + * Fail closed when configured target authorization and resolved target diverge. + * For non-wildcard targets, the authorized target must come from trusted + * configuration or triggering context, never from agent-supplied item fields. + * + * @param {Object} params + * @param {string|undefined} params.targetConfig + * @param {number|undefined} params.resolvedNumber + * @param {number|undefined} params.authorizedNumber + * @param {string} params.itemType + * @returns {{success: true} | {success: false, error: string, shouldFail: boolean}} + */ +function assertTargetAuthorizationInvariant(params) { + const { targetConfig, resolvedNumber, authorizedNumber, itemType } = params; + const target = targetConfig || "triggering"; + if (target === "*") { + return { success: true }; + } + + if (typeof authorizedNumber !== "number" || !Number.isInteger(authorizedNumber) || authorizedNumber <= 0) { + return { + success: false, + error: `ERR_TARGET_AUTHORIZATION: could not determine authorized target for ${itemType}`, + shouldFail: true, + }; + } + + if (resolvedNumber !== authorizedNumber) { + return { + success: false, + error: `ERR_TARGET_AUTHORIZATION: resolved target #${resolvedNumber} does not match authorized target #${authorizedNumber} for ${itemType}`, + shouldFail: true, + }; + } + + return { success: true }; +} + /** * Load custom safe output job types from environment variable * These are job names defined in safe-outputs.jobs that are processed by custom jobs @@ -485,6 +538,7 @@ module.exports = { parseAllowedItems, parseMaxCount, resolveTarget, + assertTargetAuthorizationInvariant, loadCustomSafeOutputJobTypes, loadCustomSafeOutputScriptHandlers, loadCustomSafeOutputActionHandlers, diff --git a/actions/setup/js/safe_output_helpers.test.cjs b/actions/setup/js/safe_output_helpers.test.cjs index 188a62eb56d..b8deece2b9e 100644 --- a/actions/setup/js/safe_output_helpers.test.cjs +++ b/actions/setup/js/safe_output_helpers.test.cjs @@ -95,6 +95,16 @@ describe("safe_output_helpers", () => { expect(result.contextType).toBe("issue"); }); + it("should ignore agent-supplied issue numbers for triggering targets", () => { + const result = helpers.resolveTarget({ + ...baseParams, + item: { item_number: 999, issue_number: 999 }, + }); + expect(result.success).toBe(true); + expect(result.number).toBe(123); + expect(result.contextType).toBe("issue"); + }); + it("should resolve workflow_dispatch issue context from aw_context", () => { const result = helpers.resolveTarget({ ...baseParams, @@ -154,6 +164,17 @@ describe("safe_output_helpers", () => { expect(result.contextType).toBe("issue"); }); + it("should ignore conflicting agent-supplied issue numbers for fixed targets", () => { + const result = helpers.resolveTarget({ + ...baseParams, + targetConfig: "999", + item: { item_number: 123, issue_number: 123 }, + }); + expect(result.success).toBe(true); + expect(result.number).toBe(999); + expect(result.contextType).toBe("issue"); + }); + it("should fail for invalid explicit number", () => { const result = helpers.resolveTarget({ ...baseParams, @@ -282,6 +303,16 @@ describe("safe_output_helpers", () => { expect(result.contextType).toBe("pull request"); }); + it("should ignore agent-supplied PR numbers for triggering targets", () => { + const result = helpers.resolveTarget({ + ...baseParams, + item: { pull_request_number: 999, pr_number: 999, pr: 999, pull_number: 999 }, + }); + expect(result.success).toBe(true); + expect(result.number).toBe(123); + expect(result.contextType).toBe("pull request"); + }); + it("should resolve triggering pull_request_target context", () => { const result = helpers.resolveTarget({ ...baseParams, @@ -336,6 +367,17 @@ describe("safe_output_helpers", () => { expect(result.contextType).toBe("pull request"); }); + it("should ignore conflicting agent-supplied PR numbers for fixed targets", () => { + const result = helpers.resolveTarget({ + ...baseParams, + targetConfig: "456", + item: { pull_request_number: 123, pr_number: 123, pr: 123, pull_number: 123 }, + }); + expect(result.success).toBe(true); + expect(result.number).toBe(456); + expect(result.contextType).toBe("pull request"); + }); + it("should resolve wildcard with pull_request_number", () => { const result = helpers.resolveTarget({ ...baseParams, @@ -392,6 +434,30 @@ describe("safe_output_helpers", () => { }); }); + describe("target authorization invariants", () => { + it("should fail closed when a non-wildcard resolved target diverges from the authorized target", () => { + const result = helpers.assertTargetAuthorizationInvariant({ + targetConfig: "triggering", + resolvedNumber: 999, + authorizedNumber: 123, + itemType: "test operation", + }); + expect(result.success).toBe(false); + expect(result.error).toContain("ERR_TARGET_AUTHORIZATION"); + expect(result.shouldFail).toBe(true); + }); + + it("should allow wildcard targets because they are explicitly agent-selected", () => { + const result = helpers.assertTargetAuthorizationInvariant({ + targetConfig: "*", + resolvedNumber: 999, + authorizedNumber: undefined, + itemType: "test operation", + }); + expect(result.success).toBe(true); + }); + }); + describe("default target handling", () => { it("should use 'triggering' when targetConfig is undefined", () => { const result = helpers.resolveTarget({ diff --git a/actions/setup/js/set_issue_field.cjs b/actions/setup/js/set_issue_field.cjs index 5644e4637ad..83259defceb 100644 --- a/actions/setup/js/set_issue_field.cjs +++ b/actions/setup/js/set_issue_field.cjs @@ -8,7 +8,7 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); -const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { parseAllowedIssueFields, validateAllowedIssueFieldName, BUILTIN_ISSUE_FIELD_NAMES } = require("./allowed_issue_fields.cjs"); const { resolveSafeOutputIssueTarget } = require("./temporary_id.cjs"); @@ -160,6 +160,7 @@ async function setIssueFieldValue(githubClient, issueNodeId, fieldUpdate) { */ async function main(config = {}) { const maxCount = config.max || 5; + const targetConfig = config.target || "triggering"; const allowedIssueFields = parseAllowedIssueFields(config.allowed_fields); const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); @@ -205,23 +206,24 @@ async function main(config = {}) { const { repo: itemRepo, repoParts } = repoResult; core.info(`Target repository: ${itemRepo}`); - const targetResult = resolveSafeOutputIssueTarget({ message: item, resolvedTemporaryIds, repoParts, handlerType: "set_issue_field", aliases: ["issue_number"] }); - if (!targetResult.success) return targetResult; - let issueNumber; - if (targetResult.number !== null) { - issueNumber = targetResult.number; - core.info(`Resolved issue number: #${issueNumber}`); - } else { - const contextIssueNumber = context.payload?.issue?.number; - if (!contextIssueNumber) { - core.warning("No issue_number provided and not in issue context"); - return { - success: false, - error: "No issue number available", - }; + let targetItem = item; + if (targetConfig === "*") { + const explicitTarget = resolveSafeOutputIssueTarget({ message: item, resolvedTemporaryIds, repoParts, handlerType: "set_issue_field", aliases: ["issue_number"] }); + if (!explicitTarget.success) return explicitTarget; + if (explicitTarget.number !== null) { + targetItem = { ...item, issue_number: explicitTarget.number }; } - issueNumber = contextIssueNumber; } + const targetResult = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: "set_issue_field", + supportsIssue: true, + }); + if (!targetResult.success) return { success: false, error: targetResult.error }; + const issueNumber = targetResult.number; + core.info(`Resolved issue number: #${issueNumber}`); const filterResult = await checkRequiredFilter(githubClient, repoParts, issueNumber, requiredLabels, requiredTitlePrefix, "set_issue_field"); if (filterResult) return filterResult; diff --git a/actions/setup/js/set_issue_field.test.cjs b/actions/setup/js/set_issue_field.test.cjs index d615009c2ee..036c827e81b 100644 --- a/actions/setup/js/set_issue_field.test.cjs +++ b/actions/setup/js/set_issue_field.test.cjs @@ -90,7 +90,7 @@ describe("set_issue_field (Handler Factory Architecture)", () => { }); const { main } = require("./set_issue_field.cjs"); - handler = await main({ max: 5, issue_intent: true }); + handler = await main({ max: 5, issue_intent: true, target: "*" }); }); it("should return a function from main()", async () => { diff --git a/actions/setup/js/set_issue_type.cjs b/actions/setup/js/set_issue_type.cjs index 41f00eb2c1d..512999307a3 100644 --- a/actions/setup/js/set_issue_type.cjs +++ b/actions/setup/js/set_issue_type.cjs @@ -9,7 +9,7 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); -const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { resolveSafeOutputIssueTarget } = require("./temporary_id.cjs"); const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); @@ -172,6 +172,7 @@ async function main(config = {}) { // Extract configuration const allowedTypes = config.allowed || []; const maxCount = config.max || 5; + const targetConfig = config.target || "triggering"; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); @@ -227,24 +228,24 @@ async function main(config = {}) { const { repo: itemRepo, repoParts } = repoResult; core.info(`Target repository: ${itemRepo}`); - // Determine target issue number, with temporary ID support - const targetResult = resolveSafeOutputIssueTarget({ message: item, resolvedTemporaryIds, repoParts, handlerType: HANDLER_TYPE, aliases: ["issue_number"] }); - if (!targetResult.success) return targetResult; - let issueNumber; - if (targetResult.number !== null) { - issueNumber = targetResult.number; - core.info(`Resolved issue number: #${issueNumber}`); - } else { - const contextIssueNumber = context.payload?.issue?.number; - if (!contextIssueNumber) { - core.warning("No issue_number provided and not in issue context"); - return { - success: false, - error: "No issue number available", - }; + let targetItem = item; + if (targetConfig === "*") { + const explicitTarget = resolveSafeOutputIssueTarget({ message: item, resolvedTemporaryIds, repoParts, handlerType: HANDLER_TYPE, aliases: ["issue_number"] }); + if (!explicitTarget.success) return explicitTarget; + if (explicitTarget.number !== null) { + targetItem = { ...item, issue_number: explicitTarget.number }; } - issueNumber = contextIssueNumber; } + const targetResult = resolveTarget({ + targetConfig, + item: targetItem, + context, + itemType: HANDLER_TYPE, + supportsIssue: true, + }); + if (!targetResult.success) return { success: false, error: targetResult.error }; + const issueNumber = targetResult.number; + core.info(`Resolved issue number: #${issueNumber}`); const filterResult = await checkRequiredFilter(githubClient, repoParts, issueNumber, requiredLabels, requiredTitlePrefix, HANDLER_TYPE); if (filterResult) return filterResult; diff --git a/actions/setup/js/set_issue_type.test.cjs b/actions/setup/js/set_issue_type.test.cjs index d1c225ceee2..2c7f2baf97c 100644 --- a/actions/setup/js/set_issue_type.test.cjs +++ b/actions/setup/js/set_issue_type.test.cjs @@ -77,7 +77,7 @@ describe("set_issue_type (Handler Factory Architecture)", () => { }); const { main } = require("./set_issue_type.cjs"); - handler = await main({ max: 5, issue_intent: true }); + handler = await main({ max: 5, issue_intent: true, target: "*" }); }); it("should return a function from main()", async () => { @@ -130,6 +130,8 @@ describe("set_issue_type (Handler Factory Architecture)", () => { }); it("should use context issue number when issue_number not provided", async () => { + const { main } = require("./set_issue_type.cjs"); + handler = await main({ max: 5, issue_intent: true, target: "triggering" }); const message = { type: "set_issue_type", issue_type: "Bug", @@ -306,7 +308,7 @@ describe("set_issue_type (Handler Factory Architecture)", () => { try { const { main } = require("./set_issue_type.cjs"); - const stagedHandler = await main({ max: 5 }); + const stagedHandler = await main({ max: 5, target: "*" }); const message = { type: "set_issue_type", @@ -473,7 +475,7 @@ describe("set_issue_type (Handler Factory Architecture)", () => { it("should use legacy REST issue type update when issue_intent is disabled", async () => { const { main } = require("./set_issue_type.cjs"); - const handlerWithoutIntent = await main({ max: 5, issue_intent: false }); + const handlerWithoutIntent = await main({ max: 5, issue_intent: false, target: "*" }); const result = await handlerWithoutIntent( { diff --git a/actions/setup/js/unassign_from_user.cjs b/actions/setup/js/unassign_from_user.cjs index 844d2ed1251..785aeee0335 100644 --- a/actions/setup/js/unassign_from_user.cjs +++ b/actions/setup/js/unassign_from_user.cjs @@ -8,7 +8,7 @@ const { processItems } = require("./safe_output_processor.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); -const { resolveIssueNumber, extractAssignees, checkRequiredFilter } = require("./safe_output_helpers.cjs"); +const { resolveTarget, extractAssignees, checkRequiredFilter } = require("./safe_output_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { createCountGatedHandler } = require("./handler_scaffold.cjs"); @@ -27,6 +27,7 @@ const main = createCountGatedHandler({ // Extract configuration const allowedAssignees = config.allowed || []; const blockedAssignees = config.blocked || []; + const targetConfig = config.target || "triggering"; // Resolve target repository configuration const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); @@ -57,16 +58,22 @@ const main = createCountGatedHandler({ return async function handleUnassignFromUser(message, resolvedTemporaryIds) { const unassignItem = message; - // Determine issue number using shared helper - const issueResult = resolveIssueNumber(unassignItem); - if (!issueResult.success) { - core.warning(`Skipping unassign_from_user: ${issueResult.error}`); + const targetResult = resolveTarget({ + targetConfig, + item: unassignItem, + context, + itemType: HANDLER_TYPE, + // supportsPR=true means both issues and PRs in resolveTarget(). + supportsPR: true, + }); + if (!targetResult.success) { + core.warning(`Skipping unassign_from_user: ${targetResult.error}`); return { success: false, - error: issueResult.error, + error: targetResult.error, }; } - const issueNumber = issueResult.issueNumber; + const issueNumber = targetResult.number; // Extract assignees using shared helper const requestedAssignees = extractAssignees(unassignItem); diff --git a/actions/setup/js/unassign_from_user.test.cjs b/actions/setup/js/unassign_from_user.test.cjs index af12277978c..a39cb256582 100644 --- a/actions/setup/js/unassign_from_user.test.cjs +++ b/actions/setup/js/unassign_from_user.test.cjs @@ -101,6 +101,8 @@ describe("unassign_from_user (Handler Factory Architecture)", () => { }); it("should use explicit issue number from message", async () => { + const { main } = require("./unassign_from_user.cjs"); + handler = await main({ max: 10, allowed: ["user1"], target: "*" }); mockGithub.rest.issues.removeAssignees.mockResolvedValue({}); const message = { @@ -185,13 +187,49 @@ describe("unassign_from_user (Handler Factory Architecture)", () => { const result = await handler(message, {}); expect(result.success).toBe(false); - expect(result.error).toContain("No issue number available"); + expect(result.error).toContain("not running in issue or pull request context"); expect(mockGithub.rest.issues.removeAssignees).not.toHaveBeenCalled(); // Restore context global.context = mockContext; }); + it("should support triggering pull request context", async () => { + global.context = { + repo: { + owner: "test-owner", + repo: "test-repo", + }, + eventName: "pull_request", + payload: { + pull_request: { + number: 456, + }, + }, + }; + + mockGithub.rest.issues.removeAssignees.mockResolvedValue({}); + + const message = { + type: "unassign_from_user", + assignees: ["user1"], + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(result.issueNumber).toBe(456); + expect(mockGithub.rest.issues.removeAssignees).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + issue_number: 456, + assignees: ["user1"], + }); + + // Restore context + global.context = mockContext; + }); + it("should handle API errors gracefully", async () => { const apiError = new Error("API error"); mockGithub.rest.issues.removeAssignees.mockRejectedValue(apiError); @@ -247,6 +285,7 @@ describe("unassign_from_user (Handler Factory Architecture)", () => { max: 10, allowed: ["user1"], allowed_repos: ["test-owner/other-repo"], + target: "*", }); mockGithub.rest.issues.removeAssignees.mockResolvedValue({}); diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 5cba5b85369..afc1d3441e7 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -7,9 +7,9 @@ sidebar: # Safe Outputs MCP Gateway Specification -**Version**: 1.29.3
+**Version**: 1.29.4
**Status**: Working Draft
-**Publication Date**: 2026-09-12
+**Publication Date**: 2026-09-21
**Editor**: GitHub Agentic Workflows Team
**This Version**: [safe-outputs-specification](/gh-aw/specs/safe-outputs-specification/)
**Latest Published Version**: This document @@ -191,6 +191,7 @@ An implementation satisfying ALL normative requirements (MUST, SHALL, REQUIRED s - Complete security architecture implementation (privilege separation, threat mitigations) - Support for all mandatory safe output types (defined in Section 7) - Universal feature implementation (max limits, staged mode, footers, sanitization) +- Runtime target authorization for every safe output type that accepts a configured `target` - Protocol exchange pattern adherence (MCP stdio container transport, NDJSON persistence) - Content integrity mechanism enforcement (schema validation, domain filtering) - Execution guarantee provision (atomicity, ordering, idempotency) @@ -260,7 +261,11 @@ Verification that configuration parsing, validation, and enforcement match speci - Inheritance rules (type-specific overriding global) - Default value application -*Note*: A normative conformance test suite is RECOMMENDED for future specification versions but not currently provided. +**Method M5: Target Authorization Regression Testing** + +Validation that configured targets are enforced at runtime before privileged API calls. Target authorization tests MUST verify that omitted and explicit `target: "triggering"` configurations use trusted invocation context, fixed numeric targets use the configured number, only `target: "*"` may select agent-supplied target identifiers, and fail-safe runtime assertions abort if a non-wildcard resolved target diverges from the authorized target. + +*Note*: Implementations SHOULD maintain an automated conformance suite that maps each target-authorized safe output type to its specification section, runtime handler, and regression tests. --- @@ -2413,6 +2418,18 @@ safe-outputs: - Cross-repository commenting requires appropriate permissions in target repository - When `safe-outputs.add-comment.target` is `"*"`, requests MUST include at least one of `item_number`, `pr_number`, or `pr`; `item_number` is the canonical field. +**Target Authorization**: + +**AC-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**AC-002**: For `target: "triggering"`, the processor MUST use only the issue, pull request, or discussion number from trusted triggering-event context and MUST ignore any agent-supplied `item_number`, `pr_number`, or `pr`. + +**AC-003**: For a fixed numeric `target`, the processor MUST use the configured item number and MUST ignore any conflicting agent-supplied identifier. + +**AC-004**: Only `target: "*"` MAY select an item from the agent-supplied identifier. + +**AC-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + --- #### Type: create_pull_request @@ -2727,6 +2744,8 @@ This section provides complete definitions for all remaining safe output types. - Cross-repository targets MUST be validated against the `allowed-repos` allowlist - Issue number MUST be validated as a positive integer belonging to the target repository +**Target Authorization**: + **UI-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. **UI-002**: For `target: "triggering"`, the processor MUST use only the issue number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`. @@ -2873,6 +2892,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Configuration Parameters**: - `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed issue number - `target-repo`: Cross-repository target - `allowed-repos`: Cross-repo allowlist - `footer`: Footer override @@ -2887,6 +2907,18 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - The handler MUST verify the caller has `issues: write` permission before executing - The `duplicate_of` canonical issue reference MUST be resolved and validated before the GraphQL mutation is called; an unparseable value MUST be logged and skipped rather than causing an error +**Target Authorization**: + +**CI-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**CI-002**: For `target: "triggering"`, the processor MUST use only the issue number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`. + +**CI-003**: For a fixed numeric `target`, the processor MUST use the configured issue number and MUST ignore any conflicting agent-supplied `issue_number`. + +**CI-004**: Only `target: "*"` MAY select an issue from the agent-supplied `issue_number`. + +**CI-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -2943,6 +2975,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Configuration Parameters**: - `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed issue number. Controls the **parent** issue; `sub_issue_number` is always agent-supplied. - `staged`: Staged mode override **Security Requirements**: @@ -2951,6 +2984,19 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - Both issue numbers MUST exist in the target repository before modification - The handler MUST enforce the maximum sub-issue count limit to prevent unbounded growth - Cross-repository operations MUST be rejected; only same-repository linking is permitted +- The effective parent and sub-issue numbers, after applying `target`, MUST be validated as different; implementations MUST NOT rely solely on comparing the raw agent-supplied `parent_issue_number` to `sub_issue_number` before `target` is applied + +**Target Authorization**: + +**LSI-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**LSI-002**: For `target: "triggering"`, the processor MUST use only the parent issue number from trusted triggering-event context and MUST ignore any agent-supplied `parent_issue_number`. + +**LSI-003**: For a fixed numeric `target`, the processor MUST use the configured parent issue number and MUST ignore any conflicting agent-supplied `parent_issue_number`. + +**LSI-004**: Only `target: "*"` MAY select the parent issue from the agent-supplied `parent_issue_number`. + +**LSI-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. **Required Permissions**: @@ -3219,6 +3265,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 10) +- `target`: `"triggering"` (default), `"*"`, or a fixed pull request number +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**CPR-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**CPR-002**: For `target: "triggering"`, the processor MUST use only the pull request number from trusted triggering-event context and MUST ignore any agent-supplied `pull_request_number`. + +**CPR-003**: For a fixed numeric `target`, the processor MUST use the configured pull request number and MUST ignore any conflicting agent-supplied `pull_request_number`. + +**CPR-004**: Only `target: "*"` MAY select a pull request from the agent-supplied `pull_request_number`. + +**CPR-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3363,6 +3428,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Configuration Parameters**: - `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed pull request number - `required-labels`: Labels that must ALL be present on the pull request - `required-title-prefix`: Title prefix the pull request must start with - `allowed-branches`: Source branch glob patterns; the PR's branch must match at least one @@ -3370,6 +3436,18 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - `allowed-repos`: Cross-repository allowlist - `staged`: Staged mode override +**Target Authorization**: + +**MPR-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**MPR-002**: For `target: "triggering"`, the processor MUST use only the pull request number from trusted triggering-event context and MUST ignore any agent-supplied `pull_request_number`. + +**MPR-003**: For a fixed numeric `target`, the processor MUST use the configured pull request number and MUST ignore any conflicting agent-supplied `pull_request_number`. + +**MPR-004**: Only `target: "*"` MAY select a pull request from the agent-supplied `pull_request_number`. + +**MPR-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3402,6 +3480,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed pull request number +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**MRR-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**MRR-002**: For `target: "triggering"`, the processor MUST use only the pull request number from trusted triggering-event context and MUST ignore any agent-supplied `pull_request_number`. + +**MRR-003**: For a fixed numeric `target`, the processor MUST use the configured pull request number and MUST ignore any conflicting agent-supplied `pull_request_number`. + +**MRR-004**: Only `target: "*"` MAY select a pull request from the agent-supplied `pull_request_number`. + +**MRR-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3533,6 +3630,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 10) +- `target`: `"triggering"` (default), `"*"`, or a fixed pull request number. Constrains which PR's review threads may be resolved; `thread_id` (or `comment_id`) always identifies the specific thread. +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**RPT-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**RPT-002**: For `target: "triggering"`, the processor MUST resolve the invocation context (including forwarded `workflow_dispatch`/`repository_dispatch` invocations) and reject the operation unless the resolved thread's pull request matches the triggering pull request. + +**RPT-003**: For a fixed numeric `target`, the processor MUST reject the operation unless the resolved thread's pull request matches the configured pull request number. + +**RPT-004**: Only `target: "*"` MAY resolve threads on any pull request within the allowed repositories, without matching a specific triggering or fixed pull request. + +**RPT-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. This requirement applies uniformly, including in legacy same-repository mode. + **Required Permissions**: *GitHub Actions Token*: @@ -3605,7 +3721,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - Requires both `issues: write` and `pull-requests: write` to support labeling both entity types - Labels must exist in repository; non-existent labels generate warnings -**Target Authorization Requirements**: +**Target Authorization**: - **AL-001**: An omitted `target` configuration MUST be interpreted as `target: "triggering"`. - **AL-002**: With `target: "triggering"`, the handler MUST use only the issue or pull request number from trusted triggering-event context. It MUST ignore agent-supplied `item_number` and equivalent aliases. @@ -3641,7 +3757,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - Same permissions as `add_labels` - Missing labels are silently ignored (no error) -**Target Authorization Requirements**: +**Target Authorization**: - **RML-001**: An omitted `target` configuration MUST be interpreted as `target: "triggering"`. - **RML-002**: With `target: "triggering"`, the handler MUST use only the issue or pull request number from trusted triggering-event context. It MUST ignore agent-supplied `item_number` and equivalent aliases. @@ -3659,6 +3775,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 3) +- `target`: `"triggering"` (default), `"*"`, or a fixed pull request number +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**ARV-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**ARV-002**: For `target: "triggering"`, the processor MUST use only the pull request number from trusted triggering-event context and MUST ignore any agent-supplied `pull_request_number`. + +**ARV-003**: For a fixed numeric `target`, the processor MUST use the configured pull request number and MUST ignore any conflicting agent-supplied `pull_request_number`. + +**ARV-004**: Only `target: "*"` MAY select a pull request from the agent-supplied `pull_request_number`. + +**ARV-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3703,6 +3838,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement | `allowed` | `string[]` | `[]` | Restrict assignments to milestones with these titles | | `auto_create` | `boolean` | `false` | Auto-create milestones from the `allowed` list if they don't exist | | `max` | `number` | `1` | Maximum number of assignments | +| `target` | `string` | `"triggering"` | `"triggering"`, `"*"`, or a fixed issue number | | `target-repo` | `string` | — | Cross-repository target (`owner/repo`) | | `github-token` | `string` | — | Custom token for elevated permissions | @@ -3716,6 +3852,18 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement \* At least one of `milestone_number` or `milestone_title` must be provided. +**Target Authorization**: + +**AM-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**AM-002**: For `target: "triggering"`, the processor MUST use only the issue number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`. + +**AM-003**: For a fixed numeric `target`, the processor MUST use the configured issue number and MUST ignore any conflicting agent-supplied `issue_number`. + +**AM-004**: Only `target: "*"` MAY select an issue from the agent-supplied `issue_number`. + +**AM-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Notes**: - Milestones must exist in the repository unless `auto_create: true` is set @@ -3733,6 +3881,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed issue/pull request number +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**ATA-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**ATA-002**: For `target: "triggering"`, the processor MUST use only the issue or pull request number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`/`pull_number`. + +**ATA-003**: For a fixed numeric `target`, the processor MUST use the configured issue or pull request number and MUST ignore any conflicting agent-supplied `issue_number`/`pull_number`. + +**ATA-004**: Only `target: "*"` MAY select an issue or pull request from the agent-supplied `issue_number`/`pull_number`; only in this mode MUST the processor reject a message that specifies both fields as mutually exclusive. + +**ATA-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3762,6 +3929,19 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Configuration Options**: - `unassign-first` (boolean, default: false): If true, unassigns all current assignees before assigning new ones. Useful for reassigning issues from one user to another. +- `target`: `"triggering"` (default), `"*"`, or a fixed issue/pull request number + +**Target Authorization**: + +**ATU-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**ATU-002**: For `target: "triggering"`, the processor MUST use only the issue or pull request number from trusted triggering-event context and MUST ignore any agent-supplied identifier. + +**ATU-003**: For a fixed numeric `target`, the processor MUST use the configured issue or pull request number and MUST ignore any conflicting agent-supplied identifier. + +**ATU-004**: Only `target: "*"` MAY select an issue or pull request from the agent-supplied identifier. + +**ATU-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. **Required Permissions**: @@ -3791,6 +3971,25 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Cross-Repository Support**: Yes **Mandatory**: No +**Configuration Parameters**: + +- `max`: Operation limit (default: 1) +- `target`: `"triggering"` (default), `"*"`, or a fixed issue/pull request number +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repo allowlist + +**Target Authorization**: + +**UFU-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**UFU-002**: For `target: "triggering"`, the processor MUST use only the issue or pull request number from trusted triggering-event context and MUST ignore any agent-supplied identifier. This applies equally when the triggering context is an issue or a pull request. + +**UFU-003**: For a fixed numeric `target`, the processor MUST use the configured issue or pull request number and MUST ignore any conflicting agent-supplied identifier. + +**UFU-004**: Only `target: "*"` MAY select an issue or pull request from the agent-supplied identifier. + +**UFU-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + **Required Permissions**: *GitHub Actions Token*: @@ -3804,6 +4003,92 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement --- +#### Type: set_issue_type + +**Purpose**: Set or clear the type of an issue. + +**Default Max**: 5 +**Cross-Repository Support**: Yes +**Mandatory**: No + +**Configuration Parameters**: + +- `max`: Operation limit (default: 5) +- `allowed`: Optional allowlist of issue type names +- `target`: `"triggering"` (default), `"*"`, or a fixed issue number +- `required-labels`: Labels that must all be present on the issue +- `required-title-prefix`: Title prefix the issue must start with +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repository allowlist + +**Target Authorization**: + +**SIT-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**SIT-002**: For `target: "triggering"`, the processor MUST use only the issue number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`. + +**SIT-003**: For a fixed numeric `target`, the processor MUST use the configured issue number and MUST ignore any conflicting agent-supplied `issue_number`. + +**SIT-004**: Only `target: "*"` MAY select an issue from the agent-supplied `issue_number`. + +**SIT-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + +**Required Permissions**: + +*GitHub Actions Token*: + +- `issues: write` - Issue type operations + +*GitHub App*: + +- `issues: write` - Issue type operations +- `metadata: read` - Repository metadata (automatically granted) + +--- + +#### Type: set_issue_field + +**Purpose**: Set one custom issue field value. + +**Default Max**: 5 +**Cross-Repository Support**: Yes +**Mandatory**: No + +**Configuration Parameters**: + +- `max`: Operation limit (default: 5) +- `allowed-fields`: Optional allowlist of custom issue field names +- `target`: `"triggering"` (default), `"*"`, or a fixed issue number +- `required-labels`: Labels that must all be present on the issue +- `required-title-prefix`: Title prefix the issue must start with +- `target-repo`: Cross-repository target +- `allowed-repos`: Cross-repository allowlist + +**Target Authorization**: + +**SIF-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**SIF-002**: For `target: "triggering"`, the processor MUST use only the issue number from trusted triggering-event context and MUST ignore any agent-supplied `issue_number`. + +**SIF-003**: For a fixed numeric `target`, the processor MUST use the configured issue number and MUST ignore any conflicting agent-supplied `issue_number`. + +**SIF-004**: Only `target: "*"` MAY select an issue from the agent-supplied `issue_number`. + +**SIF-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. Agent-supplied target identifiers, including unresolved temporary IDs, MUST be ignored unless `target` is `"*"`. + +**Required Permissions**: + +*GitHub Actions Token*: + +- `issues: write` - Custom issue field operations + +*GitHub App*: + +- `issues: write` - Custom issue field operations +- `metadata: read` - Repository metadata (automatically granted) + +--- + #### Type: hide_comment **Purpose**: Hide (minimize) comments on issues, pull requests, or discussions. @@ -3815,11 +4100,24 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement **Configuration Parameters**: - `max`: Operation limit (default: 5) +- `target`: `"triggering"` (default), `"*"`, or a fixed issue/pull request/discussion number. Constrains the **parent item** that the resolved `comment_id` must belong to. - `discussions`: Control `discussions:write` permission (default: false) - `target-repo`: Cross-repository target - `allowed-repos`: Cross-repo allowlist - `allowed-reasons`: Allowed reasons for hiding comments +**Target Authorization**: + +**HC-001**: If `target` is omitted, the processor MUST interpret it as `target: "triggering"`. + +**HC-002**: For `target: "triggering"`, the processor MUST resolve the comment's parent item and reject the operation unless both the parent item's number and kind (issue/pull request vs. discussion) match the triggering context. Matching only the numeric ID and repository MUST NOT be treated as sufficient, since issue/pull request numbers and discussion numbers are independent sequences that can collide. + +**HC-003**: For a fixed numeric `target`, the processor MUST reject the operation unless the resolved comment's parent item number matches the configured target. + +**HC-004**: Only `target: "*"` MAY hide a comment on any parent item within the allowed repositories, without matching a specific triggering or fixed item. + +**HC-005**: Schema shaping, prompt instructions, and temporary-ID resolution MUST NOT replace or precede runtime target authorization. + **Required Permissions**: *GitHub Actions Token*: @@ -5634,6 +5932,11 @@ This specification revision aligns with directly relevant `CHANGELOG.md` entries - **Earlier changelog entry**: status comments were decoupled from default AI reaction behavior; explicit `on.status-comment` configuration is required when status comments are desired. - **Earlier changelog entry**: `command` trigger was renamed to `slash_command` with deprecation compatibility. +**Version 1.29.4** (2026-09-21): + +- **Added**: Conformance verification requirements for systematic target authorization regression testing and fail-safe runtime assertions. +- **Updated**: Publication metadata to 1.29.4. + **Version 1.29.3** (2026-09-12): - **Specified**: Runtime target authorization for `update_issue`, including wildcard-only resolution of agent-supplied temporary issue IDs. diff --git a/pkg/workflow/link_sub_issue_handler_config_test.go b/pkg/workflow/link_sub_issue_handler_config_test.go new file mode 100644 index 00000000000..2839b6922ed --- /dev/null +++ b/pkg/workflow/link_sub_issue_handler_config_test.go @@ -0,0 +1,45 @@ +//go:build !integration + +package workflow + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLinkSubIssueHandlerConfigIncludesTarget(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + LinkSubIssue: &LinkSubIssueConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "123"}, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + + for _, step := range steps { + if !strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + continue + } + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + require.Len(t, parts, 2) + jsonStr := strings.Trim(strings.TrimSpace(parts[1]), "\"") + jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") + + var config map[string]map[string]any + require.NoError(t, json.Unmarshal([]byte(jsonStr), &config)) + assert.Equal(t, "123", config["link_sub_issue"]["target"]) + return + } + + t.Fatal("GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG was not generated") +} diff --git a/pkg/workflow/safe_outputs_handler_registry_issues.go b/pkg/workflow/safe_outputs_handler_registry_issues.go index 2a6a807b7d0..21d33898e97 100644 --- a/pkg/workflow/safe_outputs_handler_registry_issues.go +++ b/pkg/workflow/safe_outputs_handler_registry_issues.go @@ -88,6 +88,7 @@ var issueHandlerRegistry = map[string]handlerBuilder{ c := cfg.LinkSubIssue return newHandlerConfigBuilder(). AddTemplatableInt("max", c.Max). + AddIfNotEmpty("target", c.Target). AddStringSlice("parent_required_labels", c.ParentRequiredLabels). AddIfNotEmpty("parent_title_prefix", c.ParentTitlePrefix). AddStringSlice("sub_required_labels", c.SubRequiredLabels). diff --git a/pkg/workflow/safe_outputs_specification_add_comment_test.go b/pkg/workflow/safe_outputs_specification_add_comment_test.go new file mode 100644 index 00000000000..41791ae9f02 --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_add_comment_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsAddCommentTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "add_comment") + + assert.Contains(t, section, "**AC-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**AC-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**AC-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**AC-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**AC-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_add_reviewer_test.go b/pkg/workflow/safe_outputs_specification_add_reviewer_test.go new file mode 100644 index 00000000000..781a1fe6a1a --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_add_reviewer_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsAddReviewerTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "add_reviewer") + + assert.Contains(t, section, "**ARV-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**ARV-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**ARV-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**ARV-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**ARV-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_assign_milestone_test.go b/pkg/workflow/safe_outputs_specification_assign_milestone_test.go new file mode 100644 index 00000000000..a88b4711ea3 --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_assign_milestone_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsAssignMilestoneTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "assign_milestone") + + assert.Contains(t, section, "**AM-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**AM-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**AM-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**AM-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**AM-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_assign_to_agent_test.go b/pkg/workflow/safe_outputs_specification_assign_to_agent_test.go new file mode 100644 index 00000000000..bffa8410903 --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_assign_to_agent_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsAssignToAgentTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "assign_to_agent") + + assert.Contains(t, section, "**ATA-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**ATA-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**ATA-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**ATA-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**ATA-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_assign_to_user_test.go b/pkg/workflow/safe_outputs_specification_assign_to_user_test.go new file mode 100644 index 00000000000..8be5a10e97d --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_assign_to_user_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsAssignToUserTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "assign_to_user") + + assert.Contains(t, section, "**ATU-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**ATU-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**ATU-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**ATU-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**ATU-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_close_issue_test.go b/pkg/workflow/safe_outputs_specification_close_issue_test.go new file mode 100644 index 00000000000..108a2c14bed --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_close_issue_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsCloseIssueTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "close_issue") + + assert.Contains(t, section, "**CI-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**CI-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**CI-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**CI-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**CI-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_close_pull_request_test.go b/pkg/workflow/safe_outputs_specification_close_pull_request_test.go new file mode 100644 index 00000000000..7bc46fd6274 --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_close_pull_request_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsClosePullRequestTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "close_pull_request") + + assert.Contains(t, section, "**CPR-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**CPR-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**CPR-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**CPR-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**CPR-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_hide_comment_test.go b/pkg/workflow/safe_outputs_specification_hide_comment_test.go new file mode 100644 index 00000000000..5edf25af9cb --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_hide_comment_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsHideCommentTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "hide_comment") + + assert.Contains(t, section, "**HC-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**HC-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**HC-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**HC-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**HC-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_link_sub_issue_test.go b/pkg/workflow/safe_outputs_specification_link_sub_issue_test.go new file mode 100644 index 00000000000..f68e8a3a5ca --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_link_sub_issue_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsLinkSubIssueTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "link_sub_issue") + + assert.Contains(t, section, "**LSI-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**LSI-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**LSI-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**LSI-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**LSI-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_mark_pull_request_as_ready_for_review_test.go b/pkg/workflow/safe_outputs_specification_mark_pull_request_as_ready_for_review_test.go new file mode 100644 index 00000000000..cab5b4ffc0d --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_mark_pull_request_as_ready_for_review_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsMarkPullRequestAsReadyForReviewTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "mark_pull_request_as_ready_for_review") + + assert.Contains(t, section, "**MRR-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**MRR-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**MRR-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**MRR-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**MRR-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_merge_pull_request_test.go b/pkg/workflow/safe_outputs_specification_merge_pull_request_test.go index f6ec170c2fd..f3e4db7fb07 100644 --- a/pkg/workflow/safe_outputs_specification_merge_pull_request_test.go +++ b/pkg/workflow/safe_outputs_specification_merge_pull_request_test.go @@ -38,6 +38,22 @@ func TestSafeOutputsSpecificationDocumentsMergePullRequest(t *testing.T) { "spec should document temporary ID support for merge_pull_request pull_request_number") } +func TestSafeOutputsSpecificationDocumentsMergePullRequestTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "merge_pull_request") + + assert.Contains(t, section, "**MPR-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**MPR-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**MPR-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**MPR-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**MPR-005**", "spec should require runtime enforcement") +} + func extractSpecTypeSection(t *testing.T, spec, typeName string) string { t.Helper() diff --git a/pkg/workflow/safe_outputs_specification_resolve_pull_request_review_thread_test.go b/pkg/workflow/safe_outputs_specification_resolve_pull_request_review_thread_test.go new file mode 100644 index 00000000000..8428a89364a --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_resolve_pull_request_review_thread_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsResolvePullRequestReviewThreadTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "resolve_pull_request_review_thread") + + assert.Contains(t, section, "**RPT-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**RPT-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**RPT-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**RPT-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**RPT-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_set_issue_field_test.go b/pkg/workflow/safe_outputs_specification_set_issue_field_test.go new file mode 100644 index 00000000000..d9be0cdeb5b --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_set_issue_field_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsSetIssueFieldTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "set_issue_field") + + assert.Contains(t, section, "**SIF-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**SIF-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**SIF-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**SIF-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**SIF-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_set_issue_type_test.go b/pkg/workflow/safe_outputs_specification_set_issue_type_test.go new file mode 100644 index 00000000000..ce1eb281090 --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_set_issue_type_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsSetIssueTypeTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "set_issue_type") + + assert.Contains(t, section, "**SIT-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**SIT-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**SIT-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**SIT-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**SIT-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_specification_unassign_from_user_test.go b/pkg/workflow/safe_outputs_specification_unassign_from_user_test.go new file mode 100644 index 00000000000..d345996f0ca --- /dev/null +++ b/pkg/workflow/safe_outputs_specification_unassign_from_user_test.go @@ -0,0 +1,28 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeOutputsSpecificationDocumentsUnassignFromUserTargetAuthorization(t *testing.T) { + specPath := findRepoFile(t, filepath.Join("docs", "src", "content", "docs", "specs", "safe-outputs-specification.md")) + specBytes, err := os.ReadFile(specPath) + require.NoError(t, err, "should read safe outputs specification") + + section := extractSpecTypeSection(t, string(specBytes), "unassign_from_user") + + assert.Contains(t, section, "**UFU-001**", "spec should define the omitted target default") + assert.Contains(t, section, "interpret it as `target: \"triggering\"`", "spec should default omitted targets to triggering") + assert.Contains(t, section, "**UFU-002**", "spec should define triggering target authorization") + assert.Contains(t, section, "**UFU-003**", "spec should define fixed target authorization") + assert.Contains(t, section, "**UFU-004**", "spec should define wildcard target authorization") + assert.Contains(t, section, "Only `target: \"*\"`", "spec should reserve agent-selected targets for wildcard mode") + assert.Contains(t, section, "**UFU-005**", "spec should require runtime enforcement") +} diff --git a/pkg/workflow/safe_outputs_target_validation_test.go b/pkg/workflow/safe_outputs_target_validation_test.go index 8893c71890c..3c74bfc6b66 100644 --- a/pkg/workflow/safe_outputs_target_validation_test.go +++ b/pkg/workflow/safe_outputs_target_validation_test.go @@ -283,6 +283,46 @@ func TestValidateSafeOutputsTarget(t *testing.T) { wantErr: true, errText: "invalid target value for submit-pull-request-review: \"invalid-value\"", }, + { + name: "invalid target for unassign-from-user", + config: &SafeOutputsConfig{ + UnassignFromUser: &UnassignFromUserConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "invalid-value"}, + }, + }, + wantErr: true, + errText: "invalid target value for unassign-from-user: \"invalid-value\"", + }, + { + name: "invalid target for set-issue-type", + config: &SafeOutputsConfig{ + SetIssueType: &SetIssueTypeConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "invalid-value"}, + }, + }, + wantErr: true, + errText: "invalid target value for set-issue-type: \"invalid-value\"", + }, + { + name: "invalid target for set-issue-field", + config: &SafeOutputsConfig{ + SetIssueField: &SetIssueFieldConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "invalid-value"}, + }, + }, + wantErr: true, + errText: "invalid target value for set-issue-field: \"invalid-value\"", + }, + { + name: "invalid target for resolve-pull-request-review-thread", + config: &SafeOutputsConfig{ + ResolvePullRequestReviewThread: &ResolvePullRequestReviewThreadConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "invalid-value"}, + }, + }, + wantErr: true, + errText: "invalid target value for resolve-pull-request-review-thread: \"invalid-value\"", + }, } for _, tt := range tests { diff --git a/pkg/workflow/safe_outputs_validation.go b/pkg/workflow/safe_outputs_validation.go index cb69fa32894..5692d5cee14 100644 --- a/pkg/workflow/safe_outputs_validation.go +++ b/pkg/workflow/safe_outputs_validation.go @@ -76,6 +76,11 @@ func (c *Compiler) validateSafeOutputsAllowedDomains(config *SafeOutputsConfig) var safeOutputsTargetValidationLog = logger.New("workflow:safe_outputs_target_validation") +type safeOutputTargetConfig struct { + name string + target string +} + // validateSafeOutputsTarget validates target fields in all safe-outputs configurations // Valid target values: // - "" (empty/default) - uses "triggering" behavior @@ -90,93 +95,108 @@ func validateSafeOutputsTarget(config *SafeOutputsConfig) error { safeOutputsTargetValidationLog.Print("Validating safe-outputs target fields") - // List of configs to validate - each with a name for error messages - type targetConfig struct { - name string - target string + configs := collectIssueTargetConfigs(config) + configs = append(configs, collectPullRequestTargetConfigs(config)...) + + for _, cfg := range configs { + if err := validateTargetValue(cfg.name, cfg.target); err != nil { + return err + } } - var configs []targetConfig + safeOutputsTargetValidationLog.Printf("Validated %d target fields", len(configs)) + return nil +} - // Collect all target fields from various safe-output configurations +func collectIssueTargetConfigs(config *SafeOutputsConfig) []safeOutputTargetConfig { + var configs []safeOutputTargetConfig if config.UpdateIssues != nil { - configs = append(configs, targetConfig{"update-issue", config.UpdateIssues.Target}) + configs = append(configs, safeOutputTargetConfig{"update-issue", config.UpdateIssues.Target}) } if config.UpdateDiscussions != nil { - configs = append(configs, targetConfig{"update-discussion", config.UpdateDiscussions.Target}) - } - if config.UpdatePullRequests != nil { - configs = append(configs, targetConfig{"update-pull-request", config.UpdatePullRequests.Target}) + configs = append(configs, safeOutputTargetConfig{"update-discussion", config.UpdateDiscussions.Target}) } if config.CloseIssues != nil { - configs = append(configs, targetConfig{"close-issue", config.CloseIssues.Target}) + configs = append(configs, safeOutputTargetConfig{"close-issue", config.CloseIssues.Target}) } if config.CloseDiscussions != nil { - configs = append(configs, targetConfig{"close-discussion", config.CloseDiscussions.Target}) - } - if config.ClosePullRequests != nil { - configs = append(configs, targetConfig{"close-pull-request", config.ClosePullRequests.Target}) + configs = append(configs, safeOutputTargetConfig{"close-discussion", config.CloseDiscussions.Target}) } if config.AddLabels != nil { - configs = append(configs, targetConfig{"add-labels", config.AddLabels.Target}) + configs = append(configs, safeOutputTargetConfig{"add-labels", config.AddLabels.Target}) } if config.RemoveLabels != nil { - configs = append(configs, targetConfig{"remove-labels", config.RemoveLabels.Target}) + configs = append(configs, safeOutputTargetConfig{"remove-labels", config.RemoveLabels.Target}) } if config.ReplaceLabel != nil { - configs = append(configs, targetConfig{"replace-label", config.ReplaceLabel.Target}) - } - if config.AddReviewer != nil { - configs = append(configs, targetConfig{"add-reviewer", config.AddReviewer.Target}) + configs = append(configs, safeOutputTargetConfig{"replace-label", config.ReplaceLabel.Target}) } if config.AssignMilestone != nil { - configs = append(configs, targetConfig{"assign-milestone", config.AssignMilestone.Target}) + configs = append(configs, safeOutputTargetConfig{"assign-milestone", config.AssignMilestone.Target}) } if config.AssignToAgent != nil { - configs = append(configs, targetConfig{"assign-to-agent", config.AssignToAgent.Target}) + configs = append(configs, safeOutputTargetConfig{"assign-to-agent", config.AssignToAgent.Target}) } if config.AssignToUser != nil { - configs = append(configs, targetConfig{"assign-to-user", config.AssignToUser.Target}) + configs = append(configs, safeOutputTargetConfig{"assign-to-user", config.AssignToUser.Target}) + } + if config.UnassignFromUser != nil { + configs = append(configs, safeOutputTargetConfig{"unassign-from-user", config.UnassignFromUser.Target}) + } + if config.SetIssueType != nil { + configs = append(configs, safeOutputTargetConfig{"set-issue-type", config.SetIssueType.Target}) + } + if config.SetIssueField != nil { + configs = append(configs, safeOutputTargetConfig{"set-issue-field", config.SetIssueField.Target}) } if config.LinkSubIssue != nil { - configs = append(configs, targetConfig{"link-sub-issue", config.LinkSubIssue.Target}) + configs = append(configs, safeOutputTargetConfig{"link-sub-issue", config.LinkSubIssue.Target}) } if config.HideComment != nil { - configs = append(configs, targetConfig{"hide-comment", config.HideComment.Target}) + configs = append(configs, safeOutputTargetConfig{"hide-comment", config.HideComment.Target}) + } + return configs +} + +func collectPullRequestTargetConfigs(config *SafeOutputsConfig) []safeOutputTargetConfig { + var configs []safeOutputTargetConfig + if config.UpdatePullRequests != nil { + configs = append(configs, safeOutputTargetConfig{"update-pull-request", config.UpdatePullRequests.Target}) + } + if config.ClosePullRequests != nil { + configs = append(configs, safeOutputTargetConfig{"close-pull-request", config.ClosePullRequests.Target}) + } + if config.AddReviewer != nil { + configs = append(configs, safeOutputTargetConfig{"add-reviewer", config.AddReviewer.Target}) } if config.MarkPullRequestAsReadyForReview != nil { - configs = append(configs, targetConfig{"mark-pull-request-as-ready-for-review", config.MarkPullRequestAsReadyForReview.Target}) + configs = append(configs, safeOutputTargetConfig{"mark-pull-request-as-ready-for-review", config.MarkPullRequestAsReadyForReview.Target}) } if config.DismissPullRequestReview != nil { - configs = append(configs, targetConfig{"dismiss-pull-request-review", config.DismissPullRequestReview.Target}) + configs = append(configs, safeOutputTargetConfig{"dismiss-pull-request-review", config.DismissPullRequestReview.Target}) + } + if config.ResolvePullRequestReviewThread != nil { + configs = append(configs, safeOutputTargetConfig{"resolve-pull-request-review-thread", config.ResolvePullRequestReviewThread.Target}) } if config.AddComments != nil { - configs = append(configs, targetConfig{"add-comment", config.AddComments.Target}) + configs = append(configs, safeOutputTargetConfig{"add-comment", config.AddComments.Target}) } if config.CreatePullRequestReviewComments != nil { - configs = append(configs, targetConfig{"create-pull-request-review-comment", config.CreatePullRequestReviewComments.Target}) + configs = append(configs, safeOutputTargetConfig{"create-pull-request-review-comment", config.CreatePullRequestReviewComments.Target}) } if config.SubmitPullRequestReview != nil { - configs = append(configs, targetConfig{"submit-pull-request-review", config.SubmitPullRequestReview.Target}) + configs = append(configs, safeOutputTargetConfig{"submit-pull-request-review", config.SubmitPullRequestReview.Target}) } if config.ReplyToPullRequestReviewComment != nil { - configs = append(configs, targetConfig{"reply-to-pull-request-review-comment", config.ReplyToPullRequestReviewComment.Target}) + configs = append(configs, safeOutputTargetConfig{"reply-to-pull-request-review-comment", config.ReplyToPullRequestReviewComment.Target}) } if config.PushToPullRequestBranch != nil { - configs = append(configs, targetConfig{"push-to-pull-request-branch", config.PushToPullRequestBranch.Target}) + configs = append(configs, safeOutputTargetConfig{"push-to-pull-request-branch", config.PushToPullRequestBranch.Target}) } if config.MergePullRequest != nil { - configs = append(configs, targetConfig{"merge-pull-request", config.MergePullRequest.Target}) + configs = append(configs, safeOutputTargetConfig{"merge-pull-request", config.MergePullRequest.Target}) } - // Validate each target field - for _, cfg := range configs { - if err := validateTargetValue(cfg.name, cfg.target); err != nil { - return err - } - } - - safeOutputsTargetValidationLog.Printf("Validated %d target fields", len(configs)) - return nil + return configs } // validateTargetValue validates a single target value diff --git a/scripts/check-safe-outputs-conformance.sh b/scripts/check-safe-outputs-conformance.sh index a28d8bff18b..e132de94372 100755 --- a/scripts/check-safe-outputs-conformance.sh +++ b/scripts/check-safe-outputs-conformance.sh @@ -540,6 +540,125 @@ PY } check_safe_output_config_schema_coverage +# IMP-005: Safe Output Target Authorization Coverage +check_safe_output_target_authorization_coverage() { + local findings + + echo "Running IMP-005: Safe Output Target Authorization Coverage..." + + findings=$(python3 - <<'PY' +import re +from pathlib import Path + +spec_path = Path("docs/src/content/docs/specs/safe-outputs-specification.md") +spec = spec_path.read_text() + +target_authorized_types = { + "add_comment": ("actions/setup/js/add_comment.cjs", "resolveTarget"), + "update_issue": ("actions/setup/js/update_issue.cjs", "resolveTarget"), + "close_issue": ("actions/setup/js/close_issue.cjs", "resolveTarget"), + "add_labels": ("actions/setup/js/add_labels.cjs", "resolveTarget"), + "remove_labels": ("actions/setup/js/remove_labels.cjs", "resolveTarget"), + "link_sub_issue": ("actions/setup/js/link_sub_issue.cjs", "resolveTarget"), + "close_pull_request": ("actions/setup/js/close_pull_request.cjs", "resolveTarget"), + "merge_pull_request": ("actions/setup/js/merge_pull_request.cjs", "resolveTarget"), + "mark_pull_request_as_ready_for_review": ("actions/setup/js/mark_pull_request_as_ready_for_review.cjs", "resolveTarget"), + "resolve_pull_request_review_thread": ("actions/setup/js/resolve_pr_review_thread.cjs", "reviewThreadTargetChecks"), + "add_reviewer": ("actions/setup/js/add_reviewer.cjs", "resolveTarget"), + "assign_milestone": ("actions/setup/js/assign_milestone.cjs", "resolveTarget"), + "assign_to_agent": ("actions/setup/js/assign_to_agent.cjs", "resolveTarget"), + "assign_to_user": ("actions/setup/js/assign_to_user.cjs", "resolveTarget"), + "unassign_from_user": ("actions/setup/js/unassign_from_user.cjs", "resolveTarget"), + "set_issue_type": ("actions/setup/js/set_issue_type.cjs", "resolveTarget"), + "set_issue_field": ("actions/setup/js/set_issue_field.cjs", "resolveTarget"), + "hide_comment": ("actions/setup/js/hide_comment.cjs", "commentParentTargetChecks"), +} + + +def type_section(type_name): + header = f"#### Type: {type_name}" + start = spec.find(header) + if start == -1: + return "" + rest = spec[start + len(header):] + next_section = rest.find("\n#### Type: ") + if next_section == -1: + return spec[start:] + return spec[start:start + len(header) + next_section] + + +messages = [] +for type_name, (handler_path, enforcement_mode) in target_authorized_types.items(): + section = type_section(type_name) + if not section: + messages.append(f"missing specification section for {type_name}") + continue + if "**Target Authorization**" not in section: + messages.append(f"missing Target Authorization section for {type_name}") + continue + + prefix_match = re.search(r"\*\*([A-Z]+)-001\*\*", section) + if not prefix_match: + messages.append(f"missing numbered target authorization requirements for {type_name}") + else: + prefix = prefix_match.group(1) + for suffix in ("001", "002", "003", "004", "005"): + if f"**{prefix}-{suffix}**" not in section: + messages.append(f"missing {prefix}-{suffix} in {type_name} target authorization requirements") + + for needle in ('target: "triggering"', 'target: "*"'): + if needle not in section: + messages.append(f"missing {needle} semantics in {type_name} target authorization requirements") + + test_path = Path(f"pkg/workflow/safe_outputs_specification_{type_name}_test.go") + if not test_path.exists(): + messages.append(f"missing specification regression test for {type_name}") + + handler = Path(handler_path) + if not handler.exists(): + messages.append(f"missing target-authorized handler {handler_path}") + continue + handler_text = handler.read_text() + if enforcement_mode == "resolveTarget": + if "resolveTarget" not in handler_text: + messages.append(f"{handler_path} does not use resolveTarget for runtime target authorization") + elif enforcement_mode == "reviewThreadTargetChecks": + for needle in ("resolveInvocationContext", "threadPRNumber", "triggeringPRNumber"): + if needle not in handler_text: + messages.append(f"{handler_path} missing {needle} review-thread target boundary check") + elif enforcement_mode == "commentParentTargetChecks": + for needle in ("resolvedComment.itemNumber", "resolvedComment.kind", "expectedKind"): + if needle not in handler_text: + messages.append(f"{handler_path} missing {needle} comment parent target boundary check") + +helper = Path("actions/setup/js/safe_output_helpers.cjs").read_text() +helper_test = Path("actions/setup/js/safe_output_helpers.test.cjs").read_text() +if "assertTargetAuthorizationInvariant" not in helper: + messages.append("safe_output_helpers.cjs missing fail-safe target authorization invariant assertion") +for needle in ( + "should ignore agent-supplied issue numbers for triggering targets", + "should ignore conflicting agent-supplied issue numbers for fixed targets", + "should ignore agent-supplied PR numbers for triggering targets", + "should ignore conflicting agent-supplied PR numbers for fixed targets", + "ERR_TARGET_AUTHORIZATION", +): + if needle not in helper_test: + messages.append(f"safe_output_helpers.test.cjs missing regression coverage: {needle}") + +print("\n".join(sorted(set(messages)))) +PY +) + + if [ -n "$findings" ]; then + while IFS= read -r finding; do + log_high "IMP-005: Safe output target authorization conformance gap: $finding" + done <<< "$findings" + else + log_pass "IMP-005: Safe output target authorization is specified, tested, and enforced" + fi +} +check_safe_output_target_authorization_coverage + # MCE-001: Tool Description Constraint Disclosure (Section 8.3 MCE2) echo "Running MCE-001: Tool Description Constraint Disclosure..." check_mce_constraint_disclosure() { diff --git a/scripts/check-safe-outputs-conformance_test.sh b/scripts/check-safe-outputs-conformance_test.sh index 553be40a266..e5680599695 100755 --- a/scripts/check-safe-outputs-conformance_test.sh +++ b/scripts/check-safe-outputs-conformance_test.sh @@ -9,6 +9,7 @@ trap 'rm -f "$OUTPUT"' EXIT (cd "$REPO_ROOT" && bash "$SCRIPT_DIR/check-safe-outputs-conformance.sh" >"$OUTPUT" 2>&1) || true mapfile -t findings < <(grep "IMP-004: Safe output config property is missing" "$OUTPUT" || true) +mapfile -t target_findings < <(grep "IMP-005: Safe output target authorization conformance gap" "$OUTPUT" || true) if [[ ${#findings[@]} -ne 0 ]]; then echo "FAIL: Expected no safe-output config schema gaps" @@ -16,9 +17,20 @@ if [[ ${#findings[@]} -ne 0 ]]; then exit 1 fi +if [[ ${#target_findings[@]} -ne 0 ]]; then + echo "FAIL: Expected no safe-output target authorization conformance gaps" + printf ' %s\n' "${target_findings[@]}" + exit 1 +fi + if ! grep -q "IMP-004: All safe output config properties are declared in the schema" "$OUTPUT"; then echo "FAIL: Expected IMP-004 complete schema coverage result" exit 1 fi -echo "PASS: IMP-004 resolves referenced safe-output schemas" +if ! grep -q "IMP-005: Safe output target authorization is specified, tested, and enforced" "$OUTPUT"; then + echo "FAIL: Expected IMP-005 target authorization coverage result" + exit 1 +fi + +echo "PASS: IMP-004 resolves referenced safe-output schemas and IMP-005 enforces target authorization coverage"