Skip to content

Commit 9b71bb4

Browse files
authored
Merge branch 'main' into fix-org-page-stack
2 parents c13afec + 46bfea9 commit 9b71bb4

16 files changed

Lines changed: 1795 additions & 148 deletions
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
# ADR-29257: Expression-Controlled Threat Detection for `workflow_call` Reuse
2+
3+
**Date**: 2026-04-30
4+
**Status**: Draft
5+
**Deciders**: pelikhan
6+
7+
---
8+
9+
## Part 1 — Narrative (Human-Friendly)
10+
11+
### Context
12+
13+
The `safe-outputs.threat-detection` field previously accepted only compile-time boolean or object values. This made it impossible for reusable `workflow_call` workflows to expose threat detection on/off control to callers via `inputs` — every combination required a separate workflow file. The same limitation applied to the `enabled` and `continue-on-error` sub-fields of the object form. The codebase had already established a pattern of accepting GitHub Actions expression strings for other parameterizable fields (PR #29212 for list constraints; PR #29230 for `protected-files` and `patch-format`); this PR applies the same pattern to the threat detection subsystem. Unlike those prior cases, threat detection is compiled as a separate GitHub Actions job rather than a handler configuration value, so allowing runtime control requires changes to the `if:` conditions on the `detection` job and on all downstream jobs that depend on its result (`safe_outputs`, `safe_jobs`, `upload_assets`).
14+
15+
### Decision
16+
17+
We will allow `safe-outputs.threat-detection` to accept a GitHub Actions expression string (matching `${{...}}`), and will allow the `enabled` and `continue-on-error` sub-fields of the object form to accept `templatable_boolean` (bool literal or expression string). When an expression is detected at parse time, the compiler stores it in `EnabledExpr` / `ContinueOnErrorExpr` fields of `ThreatDetectionConfig` and always emits the `detection` job (never skips compilation). The detection job's `if:` condition is extended with the raw caller expression so GitHub Actions evaluates it at runtime and skips the job when the expression resolves to `false`. Because a skipped `detection` job would cause downstream jobs that depend on it with a strict success condition to also be skipped silently, the `safe_outputs`, `safe_jobs`, and `upload_assets` jobs switch to `always() && (success || skipped)` semantics (`buildDetectionPassedCondition`) whenever the detection configuration is expression-controlled.
18+
19+
### Alternatives Considered
20+
21+
#### Alternative 1: Separate Workflow Files per Detection Mode
22+
23+
Callers could maintain distinct workflow copies for each threat detection configuration. This was the status quo before this PR. It was rejected for the same reason as in ADR-29230: the duplication does not scale, and every structural change to the base workflow must be propagated to all variants, which teams routinely fail to do.
24+
25+
#### Alternative 2: Add a New Dedicated `threat-detection-expression` Field
26+
27+
A separate field (e.g., `threat-detection-enabled-expression`) could accept only expression strings while the existing field remained bool-only. This avoids changing the type of `threat-detection` but doubles the surface area and requires callers to know which field to use in which context. It was rejected as unnecessarily complex given that the `${{...}}`-pattern detection approach is already established in the codebase and yields a single, self-describing field.
28+
29+
#### Alternative 3: Relax Gate Conditions on All Downstream Jobs Unconditionally
30+
31+
Instead of conditionally switching downstream jobs to `always() && (success || skipped)` only when detection is expression-controlled, the gate condition could be relaxed for all workflows regardless of detection configuration. This was not chosen because it would weaken the strict-mode guarantee for workflows with static detection configurations, where a failed detection job must still block `safe_outputs`.
32+
33+
### Consequences
34+
35+
#### Positive
36+
- A single reusable `workflow_call` workflow can expose threat detection as a caller-controlled input without duplication.
37+
- Literal boolean and object forms remain fully backward-compatible; existing workflows are unaffected.
38+
- The `detection` job is always compiled when an expression is provided, ensuring the compiled lock file is static regardless of which runtime value the expression resolves to.
39+
- The fail-closed default (`always()` + `success || skipped` on downstream jobs) ensures that a misconfigured expression cannot silently skip safe-output handling.
40+
41+
#### Negative
42+
- Expression values are only validated at runtime. A typo in a `workflow_call` input default (e.g., `default: ${{ inputs.typo }}`) will not be caught until the workflow executes.
43+
- All three downstream job builders (`buildConsolidatedSafeOutputsJob`, `buildSafeJobs`, `buildUploadAssetsJob`) must independently check `IsConditionalDetection()` and apply the extended condition, adding three separate call sites to keep in sync.
44+
- The `continue-on-error` step emitter (`buildDetectionConclusionStep`) must distinguish between a literal-true, literal-false, and expression value, adding a third branch to a function that previously had two.
45+
46+
#### Neutral
47+
- The `extractRawExpression` helper was added to strip `${{` / `}}` wrappers from expression strings before embedding them in the YAML `if:` expression tree; this is a small pure utility with no broader side effects.
48+
- The `IsConditionalDetection` package-level helper provides a single authoritative check that is reused across all three affected compiler functions.
49+
- The `templatable_boolean` JSON schema `$ref` was already defined in the schema for other fields; this PR reuses it for `enabled` and `continue-on-error` without introducing a new definition.
50+
51+
---
52+
53+
## Part 2 — Normative Specification (RFC 2119)
54+
55+
> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
56+
57+
### Schema and Parsing
58+
59+
1. The `safe-outputs.threat-detection` field **MUST** accept either a boolean literal, an object form, or a GitHub Actions expression string matching `^\$\{\{.*\}\}$`.
60+
2. The `enabled` and `continue-on-error` sub-fields of the object form **MUST** accept either a boolean literal or a GitHub Actions expression string (i.e., they **MUST** be typed as `templatable_boolean`).
61+
3. When `threat-detection` is a boolean literal `false`, or the object form sets `enabled: false`, the parser **MUST** return `nil` (detection disabled).
62+
4. When `threat-detection` is an expression string, the parser **MUST** store the full `${{...}}` string in `ThreatDetectionConfig.EnabledExpr` and **MUST NOT** treat the expression as a disabled state.
63+
5. When `continue-on-error` is an expression string, the parser **MUST** store it in `ThreatDetectionConfig.ContinueOnErrorExpr` and **MUST NOT** set `ContinueOnError` (the literal bool field) for that configuration.
64+
65+
### Detection Job Compilation
66+
67+
1. When `ThreatDetectionConfig.EnabledExpr` is non-nil, the compiler **MUST** always emit a `detection` job (it **MUST NOT** skip compiling the job based on the expression value).
68+
2. The `detection` job's `if:` condition **MUST** include the raw caller expression (the `${{...}}` wrappers stripped via `extractRawExpression`) appended with `&&` to the existing content guard condition.
69+
3. The `continue-on-error` step field on the detection conclusion step **MUST** emit the expression string verbatim (unquoted) when `ContinueOnErrorExpr` is non-nil, rather than a `"true"` or `"false"` literal.
70+
4. The `GH_AW_DETECTION_CONTINUE_ON_ERROR` environment variable on the detection conclusion step **MUST** emit the expression string verbatim when `ContinueOnErrorExpr` is non-nil.
71+
72+
### Downstream Job Conditions
73+
74+
1. When `IsConditionalDetection(data.SafeOutputs)` returns `true`, the `safe_outputs` job's `if:` condition **MUST** use `always() && <agent-not-skipped> && buildDetectionPassedCondition()` (accepting both `success` and `skipped` results from the detection job).
75+
2. When `IsConditionalDetection(data.SafeOutputs)` returns `true`, every `safe_jobs` custom job's base condition **MUST** be wrapped with `always() && <safe-output-type-check> && buildDetectionPassedCondition()`.
76+
3. When `IsConditionalDetection(data.SafeOutputs)` returns `true`, the `upload_assets` job's `if:` condition **MUST** use `always() && <upload-asset-check> && buildDetectionPassedCondition()`.
77+
4. When `IsConditionalDetection(data.SafeOutputs)` returns `false` and detection is statically enabled, downstream jobs **MUST** continue to use the strict `needs.detection.result == 'success'` gate (via `buildDetectionSuccessCondition()`).
78+
79+
### Conformance
80+
81+
An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement — in particular: skipping the detection job compilation for an expression-controlled config, failing to wrap downstream jobs with `always()` when detection is conditional, or emitting a quoted literal instead of the raw expression in step fields — constitutes non-conformance.
82+
83+
---
84+
85+
*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25170616496) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/logs_formatting_test.go

Lines changed: 43 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,65 +6,68 @@ import (
66
"testing"
77

88
"github.com/github/gh-aw/pkg/console"
9+
"github.com/stretchr/testify/assert"
910
)
1011

1112
func TestFormatNumber(t *testing.T) {
1213
tests := []struct {
14+
name string
1315
input int
1416
expected string
1517
}{
16-
{0, "0"},
17-
{5, "5"},
18-
{42, "42"},
19-
{999, "999"},
20-
{1000, "1.00k"},
21-
{1200, "1.20k"},
22-
{1234, "1.23k"},
23-
{12000, "12.0k"},
24-
{12300, "12.3k"},
25-
{123000, "123k"},
26-
{999999, "1000k"},
27-
{1000000, "1.00M"},
28-
{1200000, "1.20M"},
29-
{1234567, "1.23M"},
30-
{12000000, "12.0M"},
31-
{12300000, "12.3M"},
32-
{123000000, "123M"},
33-
{999999999, "1000M"},
34-
{1000000000, "1.00B"},
35-
{1200000000, "1.20B"},
36-
{1234567890, "1.23B"},
37-
{12000000000, "12.0B"},
38-
{123000000000, "123B"},
18+
{"zero", 0, "0"},
19+
{"single digit", 5, "5"},
20+
{"two digits", 42, "42"},
21+
{"three digits", 999, "999"},
22+
{"one thousand", 1000, "1.00k"},
23+
{"1.2k", 1200, "1.20k"},
24+
{"1.23k", 1234, "1.23k"},
25+
{"12k", 12000, "12.0k"},
26+
{"12.3k", 12300, "12.3k"},
27+
{"123k", 123000, "123k"},
28+
{"999.999k rounds to 1000k", 999999, "1000k"},
29+
{"one million", 1000000, "1.00M"},
30+
{"1.2M", 1200000, "1.20M"},
31+
{"1.23M", 1234567, "1.23M"},
32+
{"12M", 12000000, "12.0M"},
33+
{"12.3M", 12300000, "12.3M"},
34+
{"123M", 123000000, "123M"},
35+
{"999.999M rounds to 1000M", 999999999, "1000M"},
36+
{"one billion", 1000000000, "1.00B"},
37+
{"1.2B", 1200000000, "1.20B"},
38+
{"1.23B", 1234567890, "1.23B"},
39+
{"12B", 12000000000, "12.0B"},
40+
{"123B", 123000000000, "123B"},
3941
}
4042

41-
for _, test := range tests {
42-
result := console.FormatNumber(test.input)
43-
if result != test.expected {
44-
t.Errorf("console.FormatNumber(%d) = %s, expected %s", test.input, result, test.expected)
45-
}
43+
for _, tt := range tests {
44+
t.Run(tt.name, func(t *testing.T) {
45+
result := console.FormatNumber(tt.input)
46+
assert.Equal(t, tt.expected, result, "FormatNumber(%d)", tt.input)
47+
})
4648
}
4749
}
4850

4951
func TestFormatFileSize(t *testing.T) {
5052
tests := []struct {
53+
name string
5154
size int64
5255
expected string
5356
}{
54-
{0, "0 B"},
55-
{100, "100 B"},
56-
{1024, "1.0 KB"},
57-
{1536, "1.5 KB"}, // 1.5 * 1024
58-
{1048576, "1.0 MB"}, // 1024 * 1024
59-
{2097152, "2.0 MB"}, // 2 * 1024 * 1024
60-
{1073741824, "1.0 GB"}, // 1024^3
61-
{1099511627776, "1.0 TB"}, // 1024^4
57+
{"zero bytes", 0, "0 B"},
58+
{"100 bytes", 100, "100 B"},
59+
{"one kilobyte", 1024, "1.0 KB"},
60+
{"1.5 KB", 1536, "1.5 KB"}, // 1.5 * 1024
61+
{"one megabyte", 1048576, "1.0 MB"}, // 1024 * 1024
62+
{"two megabytes", 2097152, "2.0 MB"}, // 2 * 1024 * 1024
63+
{"one gigabyte", 1073741824, "1.0 GB"}, // 1024^3
64+
{"one terabyte", 1099511627776, "1.0 TB"}, // 1024^4
6265
}
6366

6467
for _, tt := range tests {
65-
result := console.FormatFileSize(tt.size)
66-
if result != tt.expected {
67-
t.Errorf("console.FormatFileSize(%d) = %q, expected %q", tt.size, result, tt.expected)
68-
}
68+
t.Run(tt.name, func(t *testing.T) {
69+
result := console.FormatFileSize(tt.size)
70+
assert.Equal(t, tt.expected, result, "FormatFileSize(%d)", tt.size)
71+
})
6972
}
7073
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
description: Test workflow combining cache-memory with expression-controlled threat detection
3+
on:
4+
workflow_call:
5+
inputs:
6+
enable-threat-detection:
7+
description: 'Whether to enable threat detection at runtime'
8+
type: boolean
9+
default: true
10+
task:
11+
description: 'Task to store in cache'
12+
type: string
13+
default: 'Cache this result'
14+
15+
permissions: read-all
16+
17+
engine: copilot
18+
19+
tools:
20+
cache-memory:
21+
retention-days: 7
22+
github:
23+
allowed: [get_repository]
24+
25+
safe-outputs:
26+
create-issue:
27+
title-prefix: "[bot] "
28+
labels: [automated]
29+
max: 1
30+
threat-detection: ${{ inputs.enable-threat-detection }}
31+
32+
timeout-minutes: 10
33+
---
34+
35+
# Test Cache Memory with Expression-Controlled Threat Detection
36+
37+
This workflow demonstrates `cache-memory` combined with expression-controlled threat detection.
38+
The caller controls whether detection runs by passing `enable-threat-detection`.
39+
40+
The compiled output must contain:
41+
- `detection` job with `if:` referencing `inputs.enable-threat-detection`
42+
- `actions/cache/restore` in the agent job (detection is present at compile time)
43+
- `update_cache_memory` job depending on `detection`
44+
- `update_cache_memory` condition using `always()` and accepting detection `skipped`
45+
so cache is saved even when detection is skipped at runtime
46+
47+
Steps:
48+
1. Check existing files in `/tmp/gh-aw/cache-memory/`
49+
2. Store a new entry: "Run ${{ github.run_number }}: ${{ inputs.task }}"
50+
3. Report a summary in a new issue
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
---
2+
description: Test workflow combining cache-memory with threat detection enabled
3+
on:
4+
workflow_dispatch:
5+
inputs:
6+
task:
7+
description: 'Task to store in cache'
8+
required: true
9+
default: 'Cache this result'
10+
11+
permissions: read-all
12+
13+
engine: copilot
14+
15+
tools:
16+
cache-memory:
17+
retention-days: 7
18+
github:
19+
allowed: [get_repository]
20+
21+
safe-outputs:
22+
create-issue:
23+
title-prefix: "[bot] "
24+
labels: [automated]
25+
max: 1
26+
threat-detection: true
27+
28+
timeout-minutes: 10
29+
---
30+
31+
# Test Cache Memory with Threat Detection Enabled
32+
33+
This workflow demonstrates `cache-memory` combined with standard threat detection.
34+
When detection is enabled the compiled output must contain:
35+
- `actions/cache/restore` (instead of `actions/cache`) in the agent job
36+
- An `update_cache_memory` job that depends on `detection`
37+
- `update_cache_memory` condition using `always()` and accepting detection `skipped`
38+
39+
Steps:
40+
1. Check what files exist in `/tmp/gh-aw/cache-memory/` from previous runs
41+
2. Store a new entry: "Run ${{ github.run_number }}: ${{ inputs.task }}"
42+
3. Get basic repository information with the GitHub tool
43+
4. Report a summary in a new issue
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
description: Test workflow combining repo-memory with expression-controlled threat detection
3+
on:
4+
workflow_call:
5+
inputs:
6+
enable-threat-detection:
7+
description: 'Whether to enable threat detection at runtime'
8+
type: boolean
9+
default: true
10+
task:
11+
description: 'Task to store in memory'
12+
type: string
13+
default: 'Record this run'
14+
15+
permissions: read-all
16+
17+
engine: copilot
18+
19+
tools:
20+
repo-memory:
21+
branch-name: memory/test-threat-detection-expr
22+
description: "Test repo-memory with expression-controlled threat detection"
23+
max-file-size: 524288
24+
github:
25+
allowed: [get_repository]
26+
27+
safe-outputs:
28+
create-issue:
29+
title-prefix: "[bot] "
30+
labels: [automated]
31+
max: 1
32+
threat-detection: ${{ inputs.enable-threat-detection }}
33+
34+
timeout-minutes: 10
35+
---
36+
37+
# Test Repo Memory with Expression-Controlled Threat Detection
38+
39+
This workflow demonstrates `repo-memory` combined with expression-controlled threat detection.
40+
The caller controls whether detection runs by passing `enable-threat-detection`.
41+
42+
The compiled output must contain:
43+
- `detection` job with `if:` referencing `inputs.enable-threat-detection`
44+
- `push_repo_memory` job depending on `detection`
45+
- `push_repo_memory` condition using `always()` and accepting detection `skipped`
46+
so that the memory is persisted even when detection is skipped at runtime
47+
48+
Steps:
49+
1. Check existing files in `/tmp/gh-aw/repo-memory-default/memory/default/`
50+
2. Append a new entry: "Run ${{ github.run_number }}: ${{ inputs.task }}"
51+
3. Report a summary in a new issue
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
description: Test workflow combining repo-memory with threat detection enabled
3+
on:
4+
workflow_dispatch:
5+
inputs:
6+
task:
7+
description: 'Task to store in memory'
8+
required: true
9+
default: 'Record this run'
10+
11+
permissions: read-all
12+
13+
engine: copilot
14+
15+
tools:
16+
repo-memory:
17+
branch-name: memory/test-threat-detection
18+
description: "Test repo-memory with threat detection"
19+
max-file-size: 524288
20+
github:
21+
allowed: [get_repository]
22+
23+
safe-outputs:
24+
create-issue:
25+
title-prefix: "[bot] "
26+
labels: [automated]
27+
max: 1
28+
threat-detection: true
29+
30+
timeout-minutes: 10
31+
---
32+
33+
# Test Repo Memory with Threat Detection Enabled
34+
35+
This workflow demonstrates `repo-memory` combined with threat detection enabled.
36+
The compiled output must contain:
37+
- `push_repo_memory` job depending on `detection`
38+
- `push_repo_memory` job condition using `always()` and accepting detection `skipped`
39+
40+
Steps:
41+
1. Check what files exist in `/tmp/gh-aw/repo-memory-default/memory/default/` from prior runs
42+
2. Append a new entry: "Run ${{ github.run_number }}: ${{ inputs.task }}"
43+
3. Get basic repository information with the GitHub tool
44+
4. Report a summary in a new issue

0 commit comments

Comments
 (0)