Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions actions/setup/js/approve_workflow_run.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const { SAFE_OUTPUT_E007 } = require("./error_codes.cjs");
const path = require("node:path");
const { isStagedMode } = require("./safe_output_helpers.cjs");
const { logStagedPreviewInfo } = require("./staged_preview.cjs");
const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs");
const { checkFileProtectionPostApply, POLICY_FILE_PROTECTION_DENIED_REASON_CODE } = require("./manifest_file_helpers.cjs");
const { getRunStartedMessage } = require("./messages_run_status.cjs");
const { generateFooterWithMessages } = require("./messages_footer.cjs");
const { sanitizeContent } = require("./sanitize_content.cjs");
Expand Down Expand Up @@ -369,7 +369,7 @@ async function main(config = {}) {
if (protection.action !== "allow") {
const error = `Workflow run ${runId} cannot be approved because pull request #${pullRequestNumber} modifies protected files (${protection.files.join(", ")})`;
core.warning(error);
return { success: false, error };
return { success: false, skipped: true, reasonCode: POLICY_FILE_PROTECTION_DENIED_REASON_CODE, error };
}
}

Expand Down
2 changes: 2 additions & 0 deletions actions/setup/js/approve_workflow_run.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,8 @@ describe("approve_workflow_run", () => {
const result = await handler({ run_id: 123 }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("modifies protected files");
expect(mockApproveWorkflowRun).not.toHaveBeenCalled();
});
Expand Down
21 changes: 20 additions & 1 deletion actions/setup/js/manifest_file_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
/** @typedef {import('./types/handler-factory').HandlerConfig} HandlerConfig */
const { extractDiffGitHeaderEntries } = require("./patch_path_helpers.cjs");

/**
* Shared reasonCode for handler results that are skipped because a policy-driven
* file-protection check (allowed-files allowlist or protected-files policy) denied
* the operation. Used by push_to_pull_request_branch.cjs and approve_workflow_run.cjs
* so that these declines are surfaced consistently as skips, not hard failures.
*/
const POLICY_FILE_PROTECTION_DENIED_REASON_CODE = "POLICY_FILE_PROTECTION_DENIED";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot is there already a centralized place for policy codes, reuse

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes — error_codes.cjs is the existing centralized registry for these codes (it already holds CONFIG_HASH_MISMATCH, RATE_LIMIT_EXCEEDED, etc.). Moved POLICY_FILE_PROTECTION_DENIED_REASON_CODE there and updated push_to_pull_request_branch.cjs/approve_workflow_run.cjs to import it from error_codes.cjs instead of manifest_file_helpers.cjs (dfd change just pushed).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correction: the commit hash is f460f00.


function normalizeDotFolderExcludes(excludes) {
return new Set((Array.isArray(excludes) ? excludes : []).map(exclude => String(exclude || "").replace(/\/+$/, "")).filter(Boolean));
}
Expand Down Expand Up @@ -340,4 +348,15 @@ function checkFileProtectionPostApply(actualFiles, config) {
return { action: "deny", source: "protected", files: allProtected };
}

module.exports = { extractFilenamesFromPatch, extractPathsFromPatch, checkForManifestFiles, checkForProtectedPaths, checkForTopLevelDotFolders, checkAllowedFiles, checkExcludedFiles, checkFileProtection, checkFileProtectionPostApply };
module.exports = {
extractFilenamesFromPatch,
extractPathsFromPatch,
checkForManifestFiles,
checkForProtectedPaths,
checkForTopLevelDotFolders,
checkAllowedFiles,
checkExcludedFiles,
checkFileProtection,
checkFileProtectionPostApply,
POLICY_FILE_PROTECTION_DENIED_REASON_CODE,
};
10 changes: 5 additions & 5 deletions actions/setup/js/push_to_pull_request_branch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs");
const { detectForkPR, checkBranchPushable } = require("./pr_helpers.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { checkFileProtection, checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs");
const { checkFileProtection, checkFileProtectionPostApply, POLICY_FILE_PROTECTION_DENIED_REASON_CODE } = require("./manifest_file_helpers.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs");
const { withGitHubHostToken } = require("./git_auth_helpers.cjs");
Expand Down Expand Up @@ -524,8 +524,8 @@ async function main(config = {}) {
protection.source === "allowlist"
? `Cannot push to pull request branch: patch modifies files outside the allowed-files list (${filesStr}). Add the files to the allowed-files configuration field or remove them from the patch.`
: `Cannot push to pull request branch: patch modifies protected files (${filesStr}). Add them to the allowed-files configuration field or set protected-files: fallback-to-issue to create a review issue instead.`;
core.error(msg);
return { success: false, error: msg };
core.warning(msg);
return { success: false, skipped: true, reasonCode: POLICY_FILE_PROTECTION_DENIED_REASON_CODE, error: msg };
}
if (protection.action === "fallback") {
protectedFilesForFallback = protection.files;
Expand Down Expand Up @@ -1118,8 +1118,8 @@ async function main(config = {}) {
bundleProtection.source === "post-apply"
? `Cannot push to pull request branch: bundle modifies files outside the allowed-files list (${filesStr}). Add the files to the allowed-files configuration field or remove them from the bundle.`
: `Cannot push to pull request branch: bundle modifies protected files (${filesStr}). Add them to the allowed-files configuration field or set protected-files: fallback-to-issue to create a review issue instead.`;
core.error(msg);
return { success: false, error: msg };
core.warning(msg);
return { success: false, skipped: true, reasonCode: POLICY_FILE_PROTECTION_DENIED_REASON_CODE, error: msg };
}
if (bundleProtection.action === "fallback") {
core.warning(`Protected file protection triggered (fallback-to-issue): ${bundleProtection.files.join(", ")}. Will create review issue instead of pushing.`);
Expand Down
10 changes: 10 additions & 0 deletions actions/setup/js/push_to_pull_request_branch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3161,6 +3161,8 @@ ${diffs}
const result = await handler({ branch: "should-reject-files-outside-the-allowed-files-allowlist" }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("outside the allowed-files list");
expect(result.error).toContain("src/index.js");
});
Expand Down Expand Up @@ -3195,6 +3197,8 @@ ${diffs}
const result = await handler({ branch: "should-still-block-a-protected-file-when-it-is-in-the-allowl" }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("protected files");
expect(result.error).toContain("package.json");
});
Expand Down Expand Up @@ -3362,6 +3366,8 @@ ${diffs}
const result = await handler({ branch: "should-block-a-protected-file-when-no-allowed-files-list-is-" }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("protected files");
expect(result.error).toContain("package.json");
});
Expand All @@ -3374,6 +3380,8 @@ ${diffs}
const result = await handler({ branch: "should-reject-a-mixed-patch-where-at-least-one-file-is-outsi" }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("outside the allowed-files list");
expect(result.error).toContain("src/index.js");
expect(result.error).not.toContain(".changeset/my-fix.md");
Expand Down Expand Up @@ -3438,6 +3446,8 @@ ${diffs}
const result = await handler({ branch: "should-still-block-non-ignored-files-that-violate-the-allowe" }, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reasonCode).toBe("POLICY_FILE_PROTECTION_DENIED");
expect(result.error).toContain("outside the allowed-files list");
expect(result.error).toContain("other/file.txt");
expect(result.error).not.toContain("src/index.js");
Expand Down
Loading