-
Notifications
You must be signed in to change notification settings - Fork 280
Description
Summary
The safe-outputs.create-pull-request.draft configuration setting can be overridden by the AI agent, defeating the purpose of having a configuration guardrail. When draft: false is configured, PRs should NEVER be created as drafts, regardless of what the agent requests.
Analysis
Current behavior (Line 613 in actions/setup/js/create_pull_request.cjs):
const draft = pullRequestItem.draft !== undefined ? pullRequestItem.draft : draftDefault;This prioritizes the agent's message over the configuration:
- ✅ If agent specifies
draft: true, PR is created as draft (WRONG - bypasses config) - ✅ If agent specifies
draft: false, PR is not draft - ✅ If agent doesn't specify, uses config default
Expected behavior:
Configuration should be enforced as a policy, not used as a fallback. Similar to how autoMerge and allowEmpty work.
Security impact:
This violates the four-layer security architecture documented in scratchpad/dev.md:
Layer 1: Frontmatter Configuration - Workflow authors declare
safe-outputs:in YAML frontmatter. Defines operation limits, permissions, and constraints. No runtime modification possible.
Evidence
Configuration shows draft: false
safe-outputs:
create-pull-request:
draft: false # Explicitly configured as falseLog shows draft default set to false, but Draft: true in output
Draft default: false
...
Draft: true # Agent overrode the configuration!
Agent output specifies "draft": true
{"type": "create_pull_request", "draft": true, ...}Root Cause
The draft setting is treated as a "fallback" value rather than a policy enforcement:
- Agent can include
"draft": true/falsein its message - If present, agent's value is used (line 613)
- Configuration is only consulted if agent doesn't specify
This is inconsistent with other config-only settings:
autoMerge- Config only, not overridable ✅allowEmpty- Config only, not overridable ✅draft- Agent can override ❌ (BUG)
Implementation Plan
1. Change Configuration Semantics
The draft configuration should become a policy enforcement mechanism, not a fallback:
Option A: Strict Mode (Recommended)
- When
draftis explicitly configured, use ONLY the config value - Ignore any
draftfield in the agent message - Log a warning if agent tries to override
Option B: Config as Default with Warning
- Keep current fallback behavior
- Add warning when agent overrides config
- This maintains backward compatibility but still allows overrides
Recommendation: Use Option A (strict enforcement) because:
- Aligns with security architecture (Layer 1 configuration)
- Consistent with
autoMergeandallowEmptybehavior - Configuration exists specifically to constrain agent behavior
2. Update create_pull_request.cjs
File: actions/setup/js/create_pull_request.cjs
Change at line 613:
// BEFORE (incorrect - agent can override):
const draft = pullRequestItem.draft !== undefined ? pullRequestItem.draft : draftDefault;
// AFTER (correct - config is enforced):
const draft = draftDefault;
// Log warning if agent attempted to override
if (pullRequestItem.draft !== undefined && pullRequestItem.draft !== draftDefault) {
core.warning(
`Agent requested draft: ${pullRequestItem.draft}, but configuration enforces draft: ${draftDefault}. ` +
`Configuration takes precedence for security. To change this, update safe-outputs.create-pull-request.draft in the workflow file.`
);
}3. Update Tests
File: actions/setup/js/create_pull_request.test.cjs (if exists) or create new test file
Add test cases:
describe("draft configuration enforcement", () => {
test("should enforce draft: false from config even when agent requests draft: true", async () => {
const config = { draft: false };
const message = {
title: "Test PR",
body: "Test",
draft: true // Agent tries to override
};
// Mock GitHub API to capture the draft value
const createPullMock = jest.fn().mockResolvedValue({ data: { number: 1 } });
// ... test execution ...
// Assert that PR was created with draft: false (from config)
expect(createPullMock).toHaveBeenCalledWith(
expect.objectContaining({ draft: false })
);
});
test("should enforce draft: true from config even when agent requests draft: false", async () => {
const config = { draft: true };
const message = {
title: "Test PR",
body: "Test",
draft: false // Agent tries to override
};
// ... test execution ...
// Assert that PR was created with draft: true (from config)
expect(createPullMock).toHaveBeenCalledWith(
expect.objectContaining({ draft: true })
);
});
test("should log warning when agent attempts to override draft config", async () => {
const config = { draft: false };
const message = { draft: true };
const warningMock = jest.spyOn(core, 'warning');
// ... test execution ...
expect(warningMock).toHaveBeenCalledWith(
expect.stringContaining("Agent requested draft: true, but configuration enforces draft: false")
);
});
});4. Update Documentation
File: Documentation files that describe safe-outputs.create-pull-request.draft
Clarify that:
draftis a policy setting, not a default- Agent messages cannot override this configuration
- This is consistent with other policy settings like
auto-mergeandallow-empty
5. Validation Checklist
Before completing:
- Run
make fmt- Format JavaScript files - Run
make lint-cjs- Validate JavaScript changes - Run tests to ensure no regressions
- Run
make agent-finish- Complete validation - Verify that draft configuration is now enforced in both directions (false→false, true→true)
- Confirm warning is logged when agent attempts override
Expected Outcomes
After fix:
- ✅
draft: falsein config → PR is never created as draft - ✅
draft: truein config → PR is always created as draft - ✅ Agent cannot bypass this security guardrail
- ✅ Warning logged when agent attempts to override
- ✅ Consistent with
autoMergeandallowEmptybehavior - ✅ Aligns with Layer 1 security architecture
Breaking Changes
Potential impact:
- Workflows relying on agent overriding
draftconfig will change behavior - However, this is a security fix - agent should not be able to bypass configuration guardrails
- The configuration already exists specifically to constrain agent behavior
Migration:
- Users who want dynamic draft control should remove
draftfrom their config (then fallback to agent decision) - Users who configured
draftexplicitly were expecting it to be enforced (this fixes that expectation)
Suggested labels: bug, actions, priority-high