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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions actions/setup/js/create_pr_review_comment.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_help
const { sanitizeContent } = require("./sanitize_content.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
const { isStagedMode, logStagedPreviewInfo, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { isTemplatableTrue, isStagedMode, logStagedPreviewInfo, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs");

/** @type {string} Safe output type handled by this module */
Expand Down Expand Up @@ -59,7 +59,7 @@ async function main(config = {}) {
}

// Propagate per-handler staged flag to the shared PR review buffer
if (config.staged === true) {
if (isTemplatableTrue(config.staged)) {
buffer.setStaged(true);
}
if (isStagedMode(config)) {
Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/dispatch_repository.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ async function main(config = {}) {
core.info(`dispatch_repository: dispatching event_type="${eventType}" to ${targetRepoSlug} (workflow: ${toolConfig.workflow || "unspecified"})`);

// If in staged mode, preview without executing
if (isStaged || toolConfig.staged) {
if (isStaged || isStagedMode(toolConfig)) {
logStagedPreviewInfo(`Would dispatch repository_dispatch event: event_type="${eventType}" to ${targetRepoSlug}, client_payload=${JSON.stringify(clientPayload)}`);
dispatchCounts[toolName] = currentCount + 1;
return {
Expand Down
11 changes: 11 additions & 0 deletions actions/setup/js/dispatch_repository.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,17 @@ describe("dispatch_repository", () => {
expect(dispatchEventCalls.length).toBe(0);
});

it('should not treat string "false" in tool staged config as staged mode', async () => {
const handler = await main({
tools: { deploy: { event_type: "deploy", repository: "test-owner/test-repo", max: 5, staged: "false" } },
});

const result = await handler({ tool_name: "deploy" }, {});
expect(result.success).toBe(true);
expect(result.staged).toBeUndefined();
expect(dispatchEventCalls.length).toBe(1);
});

it("should parse numeric max from string config", async () => {
const handler = await main({
tools: { deploy: { event_type: "deploy", repository: "test-owner/test-repo", max: "3" } },
Expand Down
28 changes: 28 additions & 0 deletions actions/setup/js/handler_scaffold.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,34 @@ describe("handler_scaffold", () => {
expect(setupSpy).toHaveBeenCalledWith(config, 5, true);
});

it('should pass isStaged as true when config.staged is the string "true"', async () => {
const setupSpy = vi.fn().mockResolvedValue(async () => ({ success: true }));

const factory = createCountGatedHandler({
handlerType: "test_handler",
setup: setupSpy,
});

const config = { max: 5, staged: "true" };
await factory(config);

expect(setupSpy).toHaveBeenCalledWith(config, 5, true);
});

it("should treat unresolved staged expressions as false until runtime resolves them", async () => {
const setupSpy = vi.fn().mockResolvedValue(async () => ({ success: true }));

const factory = createCountGatedHandler({
handlerType: "test_handler",
setup: setupSpy,
});

const config = { max: 5, staged: "${{ inputs.staged }}" };
await factory(config);

expect(setupSpy).toHaveBeenCalledWith(config, 5, false);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The new tests cover staged: "true" (resolves to true) and staged: "${{ inputs.staged }}" (unresolved, stays false) — but not staged: "false" (explicitly disabled as a resolved string). Given that isTemplatableTrue is the central safety gate for staged mode, a test asserting isTemplatableTrue("false") === false would make the contract complete and guard against future string-coercion mistakes.

💡 Suggested test case
it('should pass isStaged as false when config.staged is the string "false"', async () => {
  const setupSpy = vi.fn().mockResolvedValue(async () => ({ success: true }));
  const factory = createCountGatedHandler({
    handlerType: 'test_handler',
    setup: setupSpy,
  });
  const config = { max: 5, staged: 'false' };
  await factory(config);
  expect(setupSpy).toHaveBeenCalledWith(config, 5, false);
});


it("should delegate to handleItem when under the limit", async () => {
const handleItem = vi.fn().mockResolvedValue({ success: true, data: "result" });

Expand Down
18 changes: 15 additions & 3 deletions actions/setup/js/safe_output_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -420,16 +420,27 @@ function loadCustomSafeOutputActionHandlers() {
}
}

/**
* Parse a templatable boolean value that may arrive as a boolean literal or as
* the string result of a resolved GitHub Actions expression. Undefined,
* null, false, and unresolved expression strings return false.
* @param {any} value - Candidate templatable boolean value.
* @returns {boolean}
*/
function isTemplatableTrue(value) {
return value === true || value === "true";
}

/**
* Returns true when the current execution is in staged mode.
* Staged mode is active when either the global GH_AW_SAFE_OUTPUTS_STAGED
* environment variable is "true" or when the per-handler config has staged: true.
* environment variable is "true" or when the per-handler config resolves to true.
* Use this helper in all handlers to ensure consistent staged mode detection.
* @param {Object} [config] - Handler configuration object (may have staged: true)
* @param {Object} [config] - Handler configuration object (may have staged: true/"true")
* @returns {boolean}
*/
function isStagedMode(config) {
return process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true" || (config != null && config.staged === true);
return process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true" || (config != null && isTemplatableTrue(config.staged));
}

/**
Expand Down Expand Up @@ -481,6 +492,7 @@ module.exports = {
extractAssignees,
matchesBlockedPattern,
isUsernameBlocked,
isTemplatableTrue,
isStagedMode,
logStagedPreviewInfo,
checkRequiredFilter,
Expand Down
4 changes: 2 additions & 2 deletions actions/setup/js/submit_pr_review.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction
*/

const { resolveTarget, isStagedMode, logStagedPreviewInfo, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { resolveTarget, isTemplatableTrue, isStagedMode, logStagedPreviewInfo, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
Expand Down Expand Up @@ -60,7 +60,7 @@ async function main(config = {}) {
}

// Propagate per-handler staged flag to the shared PR review buffer
if (config.staged === true) {
if (isTemplatableTrue(config.staged)) {
buffer.setStaged(true);
}
if (isStagedMode(config)) {
Expand Down
4 changes: 3 additions & 1 deletion actions/setup/js/upload_artifact.cjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// @ts-check
/// <reference types="@actions/github-script" />

const { isStagedMode } = require("./safe_output_helpers.cjs");

/**
* upload_artifact handler
*
Expand Down Expand Up @@ -449,7 +451,7 @@ async function main(config = {}) {
const allowedPaths = Array.isArray(config["allowed-paths"]) ? config["allowed-paths"] : [];
const filtersInclude = Array.isArray(config["filters-include"]) ? config["filters-include"] : [];
const filtersExclude = Array.isArray(config["filters-exclude"]) ? config["filters-exclude"] : [];
const isStaged = config["staged"] === true || process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";
const isStaged = isStagedMode(config);

core.info(`upload_artifact handler: max_uploads=${maxUploads}, retention_days=${retentionDays}, skip_archive=${skipArchive}`);
core.info(`Allowed paths: ${allowedPaths.length > 0 ? allowedPaths.join(", ") : "(none – all staging files allowed)"}`);
Expand Down
47 changes: 47 additions & 0 deletions docs/adr/41296-templatable-safe-outputs-staged.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# ADR-41296: Make `safe-outputs.staged` a Templatable Boolean

**Date**: 2026-06-24
**Status**: Draft
**Deciders**: pelikhan, copilot-swe-agent [TODO: verify human deciders]

---

### Context

The `safe-outputs.staged` flag toggles preview mode, where safe-output handlers emit step-summary messages instead of making real GitHub API calls. It was modeled as a plain `boolean` in the JSON schema and as a Go `bool` in the workflow types. This prevented expression-based configuration: a workflow author could not write `staged: ${{ inputs.staged }}` or `staged: ${{ github.event_name != 'push' }}`, because the schema rejected non-literal strings and the compiler had no way to carry an unresolved expression through to the runtime. As a result, dynamic, context-driven preview toggling was impossible without duplicating workflow definitions.

### Decision

We will make `safe-outputs.staged` a **templatable boolean** end to end: the schema accepts `#/$defs/templatable_boolean` (literal booleans or GitHub expression strings), config parsing accepts both forms at the top level and per handler, the compiler carries the value as `*TemplatableBool` instead of `bool`, and the JavaScript handlers treat a resolved expression value (`"true"`) the same as a literal `true` via a shared `isTemplatableTrue` helper. Centralizing resolution keeps literal `true`, literal `false`, and expressions handled consistently across all handlers.

### Alternatives Considered

#### Alternative 1: Keep `staged` boolean-only

Leave the flag as a literal boolean and require authors who want conditional preview mode to maintain separate workflow files or rely on the global `GH_AW_SAFE_OUTPUTS_STAGED` environment variable. Rejected because it forces workflow duplication and cannot express per-handler conditional staging driven by inputs or event context.

#### Alternative 2: Add a separate `staged-expression` field

Introduce a parallel field (e.g., `staged-when`) accepting only expressions, leaving `staged` as a literal boolean. Rejected because it splits one concept across two fields, complicates precedence rules, and produces a less intuitive schema than a single templatable field that accepts either form.

#### Alternative 3: Resolve expressions only in JS, no type changes

Keep the Go type as `bool` and push all expression handling into the JavaScript runtime. Rejected because the compiler would have nowhere to store an unresolved expression, schema validation would still reject expression strings, and the value could not flow cleanly from frontmatter through config-JSON generation to env vars.

### Consequences

#### Positive
- Authors can drive preview mode dynamically with GitHub expressions (`${{ inputs.staged }}`, event-based conditions) at both global and per-handler scope.
- A single shared `isTemplatableTrue` helper gives all handlers consistent truthiness semantics, replacing ad hoc `config.staged === true` checks.

#### Negative
- Converting workflow types from `bool` to `*TemplatableBool` introduces pointer/nil and string-resolution handling that callers must respect, increasing surface for nil-dereference or mis-typed comparisons.
- Unresolved expression strings (e.g., `"${{ inputs.staged }}"` that never resolves) evaluate to `false`, so a misconfigured expression silently runs in live mode rather than preview — a fail-open behavior for staging that authors must be aware of.

#### Neutral
- The schema change touches many `staged` definitions (every safe-output type) but is mechanical, swapping `"type": "boolean"` for `"$ref": "#/$defs/templatable_boolean"`.
- Existing literal-boolean configurations remain valid and behave identically; this is a backward-compatible widening of accepted values.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
golang.org/x/term v0.44.0
golang.org/x/tools v0.46.0
golang.org/x/vuln v1.4.0
gopkg.in/yaml.v3 v3.0.1
)

require (
Expand Down Expand Up @@ -117,5 +118,4 @@ require (
google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect
google.golang.org/grpc v1.81.1 // indirect
google.golang.org/protobuf v1.36.11 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
20 changes: 20 additions & 0 deletions pkg/parser/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,7 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_ThreatDetectionMax
},
},
}

err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(validFrontmatter, "/tmp/gh-aw/threat-detection-max-ai-credits-"+name+"-test.md")
if err != nil {
t.Fatalf("expected safe-outputs.threat-detection.max-ai-credits=%v (%s) to pass schema validation, got: %v", raw, name, err)
Expand All @@ -586,6 +587,25 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_ThreatDetectionMax
}
}

func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_SafeOutputsStagedExpressionAccepted(t *testing.T) {
t.Parallel()

validFrontmatter := map[string]any{
"on": "push",
"safe-outputs": map[string]any{
"staged": "${{ inputs.staged }}",
"create-issue": map[string]any{
"staged": "${{ inputs['issue-staged'] }}",
},
},
Comment thread
Copilot marked this conversation as resolved.
}

err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(validFrontmatter, "/tmp/gh-aw/safe-outputs-staged-expression-test.md")
if err != nil {
t.Fatalf("expected safe-outputs staged expressions to pass schema validation, got: %v", err)
}
}

func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_MaxAICreditsZeroInvalid(t *testing.T) {
t.Parallel()

Expand Down
Loading
Loading