Handle workflow_dispatch PR targeting in deterministic safe-output sample replay - #48398
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Expands deterministic safe-output replay to derive pull request branches from dispatch payloads and configured targets.
Changes:
- Adds PR-number fallback resolution for dispatch events and config targets.
- Adds workflow-dispatch regression coverage.
- Env-backed config targets remain unresolved because replay does not forward their environment variables.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/apply_samples.cjs |
Expands PR-number derivation and target parsing. |
actions/setup/js/apply_samples.test.cjs |
Tests workflow-dispatch input derivation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| const envMatch = /^\$\{([A-Za-z_][A-Za-z0-9_]*)\}$/.exec(trimmed); | ||
| if (envMatch) { | ||
| return toPositivePullRequestNumber(process.env[envMatch[1]]); |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48398 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fallback chain for deriving pull_request_number is well-structured and correctly ordered. toPositivePullRequestNumber handles all edge cases (non-numeric, NaN, zero, negative). The config-target path gracefully returns null on missing env vars or parse errors rather than crashing replay. Test coverage for the workflow_dispatch path is focused and correctly validates the fetch URL. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.8 AIC · ⌖ 5.58 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
✅ Quality NotesStrengths:
Observations:
Context: Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — COMMENT with three suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Duplicated config-parsing logic:
readConfiguredTargetPullRequestNumberandreadConfiguredTargetRepoboth inline the sameObject.fromEntrieskey-normalisation block — extracting a shared helper would reduce maintenance risk. - Silent failure on bad env-var placeholder: when a
${ENV_VAR}target resolves to a non-integer, the function returnsnullwithout logging anything, making misconfiguration invisible in CI. - Test coverage gap: the new fallback chain (priority order + config-target path + invalid values) is not yet exercised — the single regression test only covers the
workflow_dispatchhappy path.
Positive Highlights
- ✅
toPositivePullRequestNumberis a clean, well-named utility — reusing it consistently across all four sources is exactly the right call. - ✅ Fallback chain is clearly documented in the JSDoc and in the PR description.
- ✅ The regression test is well-structured (Arrange / Act / Assert, explicit env-var cleanup in
finally). - ✅ Root cause properly addressed: the fix expands the resolution strategy rather than patching a single missing branch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.2 AIC · ⌖ 4.98 AIC · ⊞ 6.7K
Comment /matt to run again
| try { | ||
| const raw = fs.readFileSync(configPath, "utf8"); | ||
| const parsed = JSON.parse(raw); | ||
| const config = parsed && typeof parsed === "object" ? Object.fromEntries(Object.entries(parsed).map(([k, v]) => [String(k).replace(/-/g, "_"), v])) : {}; |
There was a problem hiding this comment.
[/codebase-design] Duplicated config-parsing logic — readConfiguredTargetPullRequestNumber repeats the same Object.fromEntries key-normalisation block already in readConfiguredTargetRepo. This dense one-liner is harder to maintain and test in isolation.
💡 Suggested refactor
Extract a shared helper:
function parseNormalisedConfig(configPath) {
const raw = fs.readFileSync(configPath, "utf8");
const parsed = JSON.parse(raw);
if (!parsed || typeof parsed !== "object") return {};
return Object.fromEntries(
Object.entries(parsed).map(([k, v]) => [String(k).replace(/-/g, "_"), v])
);
}Both readers call parseNormalisedConfig(configPath) — cuts duplication and makes each function a single-purpose.
@copilot please address this.
| const trimmed = target.trim(); | ||
| const envMatch = /^\$\{([A-Za-z_][A-Za-z0-9_]*)\}$/.exec(trimmed); | ||
| if (envMatch) { | ||
| return toPositivePullRequestNumber(process.env[envMatch[1]]); |
There was a problem hiding this comment.
[/diagnosing-bugs] ${ENV_VAR} placeholders that expand to an empty string or non-numeric value silently return null and skip all remaining fallbacks — there is no warning logged. If a CI operator misconfigures the env var they will see a silent no-op with no indication of why the branch derivation failed.
💡 Suggestion
Add a core.debug or core.warning when the env var is set but doesn't resolve to a positive integer:
if (envMatch) {
const resolved = toPositivePullRequestNumber(process.env[envMatch[1]]);
if (!resolved) {
core.debug(`apply_samples: env var ${envMatch[1]} did not resolve to a PR number (value: ${process.env[envMatch[1]]})`);
}
return resolved;
}This makes misconfiguration visible in the Actions debug log without changing behaviour.
@copilot please address this.
| if (prevEvent === undefined) delete process.env.GITHUB_EVENT_PATH; | ||
| else process.env.GITHUB_EVENT_PATH = prevEvent; | ||
| if (prevRepo === undefined) delete process.env.GITHUB_REPOSITORY; | ||
| else process.env.GITHUB_REPOSITORY = prevRepo; |
There was a problem hiding this comment.
[/tdd] The test covers only the happy path (valid numeric string "123"). There are no tests for the new fallback priority order or edge cases in readConfiguredTargetPullRequestNumber.
💡 Missing coverage to consider
- Priority order:
arguments.pull_request_numbershould win overinputs.pull_request_number— a test with both present would confirm this. - Config-target path: A test where neither arguments nor event inputs carry a PR number, but the safe-outputs config
targetdoes (both numeric literal and${ENV_VAR}form). - Invalid / zero values:
pull_request_number: "0","abc"and""should all return null and not cause an unhandled branch.
These are the exact edge cases where the original bug could silently regress.
@copilot please address this.
There was a problem hiding this comment.
Changes needed before merge
Three issues require fixes; one additional concern is worth addressing.
Blocking issues
-
toPositivePullRequestNumberaccepts floats (line 272) —"1.5"or1.9passes the guard and reaches the GitHub API as an invalid PR number. AddNumber.isInteger(n). -
client_payload.pull_request_numbertrusted without bounds (line 258) —client_payloadis fully caller-controlled inrepository_dispatch. Placing it at the same priority tier as sample arguments andworkflow_dispatchinputs creates a potential misdirection path. Either document the trust assumption explicitly or deprioritize it below the config-target fallback. -
Config file re-read on every call (line 329) —
fs.readFileSync+JSON.parseruns synchronously per invocation. Cache at module scope.
Non-blocking
- Silent
debugswallow of parse errors (line 347) — usecore.warningwhen the file exists but fails to parse, so misconfigured setups are visible in the action log.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 32.8 AIC · ⌖ 5.02 AIC · ⊞ 5.7K
Comment /review to run again
| */ | ||
| function toPositivePullRequestNumber(value) { | ||
| const n = Number(value); | ||
| return Number.isFinite(n) && n > 0 ? n : null; |
There was a problem hiding this comment.
toPositivePullRequestNumber accepts non-integer floats: Number("1.5") is 1.5, which passes the isFinite && > 0 guard and gets forwarded as a PR number — an invalid value that will cause the GitHub API call to fail.
💡 Suggested fix
function toPositivePullRequestNumber(value) {
const n = Number(value);
return Number.isFinite(n) && Number.isInteger(n) && n > 0 ? n : null;
}Rejects "1.5", 1.9, etc. before they reach the GitHub API.
| toPositivePullRequestNumber(payload?.client_payload?.pull_request_number) || | ||
| readConfiguredTargetPullRequestNumber(entry.tool); | ||
| if (pullNumber) { | ||
| const ref = await fetchPullRequestHeadRef({ owner, repo, pullNumber }); |
There was a problem hiding this comment.
client_payload.pull_request_number is user-controlled and accepted without authorization checks: repository_dispatch events let any actor with write access (or a workflow that forwards external input) set client_payload freely. Accepting it here as a trusted PR number source can steer the replay to a different PR branch than intended.
💡 Details
payload?.client_payload?.pull_request_number is on the same priority level as the explicit sample arguments and workflow_dispatch inputs. Unlike workflow_dispatch.inputs, client_payload is entirely caller-controlled and there is no structural guarantee it comes from a trusted workflow step. If an attacker can send a repository_dispatch, they could redirect sample replay to an arbitrary PR head ref.
Consider documenting the trust boundary clearly, or move the client_payload fallback behind the config-target fallback so it is the last resort — or require an explicit opt-in flag before trusting it.
| const toolConfig = toolKey && config && typeof config === "object" ? config[toolKey] : null; | ||
| const target = toolConfig && typeof toolConfig === "object" ? toolConfig.target : null; | ||
|
|
||
| if (typeof target === "number") { |
There was a problem hiding this comment.
Config file is re-read and re-parsed on every call: fs.readFileSync + JSON.parse runs synchronously each time readConfiguredTargetPullRequestNumber is called. In a batch run over many entries this will hammer the filesystem for no reason.
💡 Suggested fix
Cache the parsed result at module scope, similar to readConfiguredTargetRepo:
let _configCache = undefined; // undefined = not yet loaded; null = failed/missing
function _loadConfig() {
if (_configCache !== undefined) return _configCache;
const configPath = process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH;
if (!configPath?.trim()) return (_configCache = null);
try {
const parsed = JSON.parse(fs.readFileSync(configPath, "utf8"));
_configCache = parsed && typeof parsed === "object" ? parsed : null;
} catch {
_configCache = null;
}
return _configCache;
}Then readConfiguredTargetPullRequestNumber just calls _loadConfig() instead of re-reading the file.
| } | ||
|
|
||
| /** | ||
| * Resolve the on-disk working directory in which a sample's patch should be |
There was a problem hiding this comment.
Config parse errors are silently swallowed at debug level: a malformed or truncated GH_AW_SAFE_OUTPUTS_CONFIG_PATH file causes readConfiguredTargetPullRequestNumber to return null with only a core.debug log — indistinguishable at runtime from "config not set". Operators will have no idea their config is broken.
💡 Suggested fix
Emit at least core.warning (not core.debug) for JSON.parse failures so the problem surfaces in the action log without blocking execution:
} catch (err) {
core.warning(`apply_samples: could not read target from ${configPath}: ${getErrorMessage(err)}`);
}Keep debug for the "file not found" / env-var-not-set path, but use warning when the file exists and is unparseable.
|
Thanks for the fix! This PR looks great and ready for review.
This follows the project's agentic development process. 🤖
|
🤖 PR Triage
Rationale: Fixes deterministic replay failure for
|
The
gh-aw-testrun failed inReplay safe-outputs samples (deterministic)when replayingpush_to_pull_request_branchsamples fromworkflow_dispatch. In that path, replay could not derive the PR head branch if the sample omittedarguments.pull_request_number, even though the workflow/config already carried PR target information.PR-branch derivation fallback expansion
apply_samples.cjsbranch derivation forpush_to_pull_request_branchto resolve PR number from multiple sources, in order:arguments.pull_request_numberevent.inputs.pull_request_number(workflow_dispatch)event.client_payload.pull_request_number(repository_dispatch)target(including${ENV_VAR}placeholder resolution)Config-target numeric resolution
targetinto a positive PR number.Regression coverage for dispatch path
preStagePatchderives the PR head ref fromworkflow_dispatchinput when sample arguments omit PR number.pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/30293408015
Run: https://github.com/github/gh-aw/actions/runs/30302221322