Harden setup JavaScript input and I/O boundaries - #54691
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54691 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This patch is mostly defensive hardening, but one change in the check-run selection helper regresses behavior: a previously selected run with an invalid started_at now blocks any later valid run with the same name from replacing it, which can surface the wrong check as the “latest” result.
Blocking theme
actions/setup/js/check_runs_helpers.cjs: invalid existing timestamps now pin stale entries inlatestByNameinstead of letting a valid newer run take over.
I did not find other changed lines here that clearly rise to blocking severity beyond that regression.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.03 AIC · ⌖ 8.93 AIC · ⊞ 4.6K
Comment /review to run again
| if (!Number.isFinite(runStartedAt)) { | ||
| continue; | ||
| } | ||
| if (!Number.isFinite(existingStartedAt)) { |
There was a problem hiding this comment.
Replacing the old comparison with an unconditional continue when existing.started_at is invalid changes behavior in a bad way: once a malformed older run is already stored, every newer valid run with the same name is ignored forever, so this helper can report a stale check instead of the actual latest result.
💡 Why this matters and how to fix it
The previous code still allowed a valid newer run to replace an entry whose timestamp parsed badly because new Date(invalid) > new Date(valid) simply evaluated false for the bad candidate, not for the already-selected one. The new branch at line 52 turns that into a hard stop.
A safer fallback is to treat an invalid existing.started_at as older than any valid candidate:
if (!Number.isFinite(runStartedAt)) {
continue;
}
if (!Number.isFinite(existingStartedAt) || runStartedAt > existingStartedAt) {
latestByName.set(run.name, run);
}That preserves the goal of skipping malformed incoming runs without pinning the map to a bad existing entry.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd — requesting changes on a correctness regression and missing test coverage.
📋 Key Themes & Highlights
Key Issues
- Behavior regression (
check_runs_helpers.cjsline 52): whenexistingStartedAtisNaN, the new code keepsexistingunconditionally — the original code fell back to epoch, letting any valid new run win. This silently changes which check run is selected as "latest". - Inconsistent error surface (
push_repo_memory.cjs): validation usescore.setFailed()+returnwhile every other guard in this PR throws. Downstream callers getundefinedand outertry/catchblocks miss it. - Duplicated helpers: six fs-wrapper functions in
memory_custom_validation.cjsduplicate the pattern fromartifact_client.cjs; extracting to a shared module would prevent divergence. - No test coverage: numeric-range guards are entirely untested.
Positive Highlights
- ✅ Comprehensive fetch-wrapping with
causechaining acrossexchange_otlp_workload_identity.cjsandartifact_client.cjs - ✅ Hardened date parsing throughout:
Date.parse+Number.isFiniteis the right idiom - ✅ Regex escaping fix in
fuzz_template_substitution_harness.cjsaddresses a real injection risk - ✅ Timeout added to workload identity requests — good defense against hung OIDC flows
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 67.2 AIC · ⌖ 10.3 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/check_runs_helpers.cjs:52
[/diagnosing-bugs] Behavior regression: when existingStartedAt is NaN, the code silently continues, keeping existing even when the incoming run has a valid timestamp. The original code used new Date(existing.started_at ?? 0) which fell back to epoch, so a valid new run would always win.
<details>
<summary>💡 Suggested fix</summary>
if (!Number.isFinite(existingStartedAt)) {
// existing has no valid timestamp; let the new run replace it
latestByName.set(run.name, run)…
</details>
<details><summary>actions/setup/js/push_repo_memory.cjs:67</summary>
**[/codebase-design]** Inconsistent error surface: every other validation guard in this PR throws an `Error`, but this block calls `core.setFailed()` + `return`. Downstream callers get `undefined` instead of an exception, and an outer `try/catch` won't capture it.
<details>
<summary>💡 Suggested fix</summary>
```js
throw new Error(`${ERR_VALIDATION}: Memory size, count, patch size, and validation timeout limits must be positive integers`);Pick one pattern for the file. Throwing is consi…
actions/setup/js/memory_custom_validation.cjs:452
[/codebase-design] Duplicated fs-wrapper pattern: this file defines six private helpers (removePath, makeDirectory, writeFile, readFile, readDirectory, makeTempDirectory) that mirror makeTempDir, readResponseText, and readResponseJSON added in artifact_client.cjs in the same PR. Two independent abstractions for the same concern will diverge in error formatting over time.
<details>
<summary>💡 Suggested approach</summary>
Extract into a shared fs_helpers.cjs (or `io_h…
actions/setup/js/exchange_otlp_workload_identity.cjs:329
[/codebase-design] The 30-second fetch timeout is hardcoded with no operator override. In high-latency or throttled environments there is no recourse without patching the source.
<details>
<summary>💡 Suggested fix</summary>
const timeoutMs = parseInt(process.env.GH_AW_OTLP_WIF_TIMEOUT_MS || "30000", 10);
// ...
signal: AbortSignal.timeout(Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : 30_000),Apply the same env var to both the STS and impersonation fetch calls for …
actions/setup/js/data_schema_normalizer.cjs:275
[/diagnosing-bugs] The error message drops the original parse failure text. Operators debugging a malformed schema will only see the path, not what the JSON parser rejected.
<details>
<summary>💡 Suggested fix</summary>
throw new Error(`${path}: invalid JSON schema: ${getErrorMessage(error)}`, { cause: error });cause is already preserved — adding getErrorMessage(error) to the message string makes it readable in logs without requiring callers to unwrap the cause chain.
</d…
actions/setup/js/push_repo_memory.cjs:67
[/tdd] None of the new numeric-validation guards (push_repo_memory, check_rate_limit, notify_comment_error, safe-outputs-mcp-server) are covered by tests. A miswired env var name or wrong default could silently pass CI.
<details>
<summary>💡 What to test</summary>
For each validation block, add unit tests that:
- Pass an invalid value (
"abc","-1","0") and assert the expected error /setFailedcall. - Pass the boundary value (e.g.,
"1") and assert normal execution con…
There was a problem hiding this comment.
Ponytail review: mostly clean error-handling/validation hardening, wrapped in appropriately small try/catches. Two spots split single boolean checks into multiple ifs unnecessarily.
net: -5 lines possible.
Generated by ✂️ Ponytail Reviewer for #54691 · auto · 32.6 AIC · ⌖ 4.07 AIC · ⊞ 7.3K
Comment /ponytail to run again
| if (!Number.isFinite(existingStartedAt)) { | ||
| continue; | ||
| } | ||
| if (runStartedAt > existingStartedAt) { |
There was a problem hiding this comment.
L49-55: shrink: two separate if (!Number.isFinite(...)) blocks. if (!Number.isFinite(runStartedAt) || !Number.isFinite(existingStartedAt)) continue;, then if (runStartedAt > existingStartedAt) ..., 2 lines saved.
| if (!Number.isFinite(b)) { | ||
| return false; | ||
| } | ||
| return a >= b; |
There was a problem hiding this comment.
L1225-1231: shrink: split into two ifs from a single combined check. if (!Number.isFinite(a) || !Number.isFinite(b)) return false;, 3 lines saved.
There was a problem hiding this comment.
Good hardening sweep — wrapping raw I/O calls, adding AbortSignal.timeout, propagating { cause: error }, and validating integer env-vars before use are all solid improvements.
Two issues to address:
-
check_runs_helpers.cjs— logic inversion (blocking): when the stored entry has an invalid date but the incoming run has a valid one, the new guard discards the valid run instead of upgrading to it. See inline comment. -
check_rate_limit.cjs— unreachable guard (non-blocking): theNumber.isNaN(thresholdTime.getTime())check can never fire after the upstream positive-integer validation. Remove or document it. See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 42.1 AIC · ⌖ 9.2 AIC · ⊞ 6.2K
| const existingStartedAt = Date.parse(existing.started_at ?? ""); | ||
| if (!Number.isFinite(runStartedAt)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Bug: valid-timestamp run is silently dropped when existing entry has an invalid date
When existingStartedAt is NaN (the stored entry has a malformed started_at) but runStartedAt is a valid timestamp, the current guard:
if (!Number.isFinite(existingStartedAt)) {
continue; // ← keeps the invalid entry, discards the valid one
}silently discards the valid incoming run and retains the entry with the unusable date. The correct behaviour is to prefer the valid-date run:
if (!Number.isFinite(runStartedAt)) {
continue; // incoming date is bad — keep existing regardless
}
if (!Number.isFinite(existingStartedAt)) {
latestByName.set(run.name, run); // upgrade: incoming is valid, existing is not
continue;
}
if (runStartedAt > existingStartedAt) {
latestByName.set(run.name, run);
}@copilot please address this.
| // Calculate time threshold | ||
| const windowMs = windowMinutes * 60 * 1000; | ||
| const thresholdTime = new Date(Date.now() - windowMs); | ||
| if (Number.isNaN(thresholdTime.getTime())) { |
There was a problem hiding this comment.
Dead code: Number.isNaN(thresholdTime.getTime()) guard is unreachable
After the new validation at line ~178:
if (!Number.isFinite(maxRuns) || maxRuns <= 0 || !Number.isFinite(windowMinutes) || windowMinutes <= 0) {
throw new Error("Rate limit maximum and window must be positive integers");
}windowMinutes is guaranteed to be a finite, positive number. Therefore windowMs = windowMinutes * 60 * 1000 is always a finite, positive number and new Date(Date.now() - windowMs) always produces a valid Date. The Number.isNaN(thresholdTime.getTime()) check added a few lines later can never be true and adds maintenance noise. Remove it, or add a comment explaining what scenario it guards against.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Hardens setup JavaScript input parsing and I/O error reporting for #54636.
Changes:
- Validates numeric, date, JSON, URL, and template inputs.
- Adds contextual filesystem and response-body errors.
- Adds workload identity request timeouts and fetch error handling.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe-outputs-mcp-server.cjs |
Validates the server port. |
actions/setup/js/push_repo_memory.cjs |
Validates memory limits and timeout. |
actions/setup/js/notify_comment_error.cjs |
Validates assignment error counts. |
actions/setup/js/memory_custom_validation.cjs |
Wraps synchronous filesystem operations. |
actions/setup/js/handle_noop_message.cjs |
Normalizes the no-op item limit. |
actions/setup/js/generate_usage_activity_summary.cjs |
Adds manifest read context. |
actions/setup/js/fuzz_template_substitution_harness.cjs |
Escapes variable names and wraps temp-directory creation. |
actions/setup/js/exchange_otlp_workload_identity.cjs |
Adds request timeouts and contextual failures. |
actions/setup/js/evaluate_outcomes.cjs |
Explicitly guards invalid dates. |
actions/setup/js/data_schema_normalizer.cjs |
Wraps malformed schema JSON errors. |
actions/setup/js/create_prompt.cjs |
Preserves causes for prompt file failures. |
actions/setup/js/check_runs_helpers.cjs |
Guards check-run timestamp comparisons. |
actions/setup/js/check_rate_limit.cjs |
Validates limits and run timestamps. |
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Adds parsing diagnostics and temp-directory context. |
actions/setup/js/awf_reflect.cjs |
Wraps invalid model URLs. |
actions/setup/js/artifact_client.cjs |
Wraps response parsing and temp-directory failures. |
actions/setup/js/add_workflow_run_comment.cjs |
Validates discussion endpoints. |
actions/setup/js/add_reaction_and_edit_comment.cjs |
Validates discussion endpoint numbers. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
actions/setup/js/add_workflow_run_comment.cjs:437
- The
discussion_commentformat is not actually validated: a missing/invalid comment ID, extra segments, a partial discussion number, or a non-positive number passes this finite check. Match the complete documented shape and require a positive safe discussion number.
const discussionNumber = parseInt(endpoint.split(":")[1], 10);
if (!Number.isFinite(discussionNumber)) {
throw new Error(`${ERR_VALIDATION}: Invalid discussion endpoint: ${endpoint}`);
}
- Files reviewed: 18/18 changed files
- Comments generated: 8
- Review effort level: Balanced
| @@ -54,6 +54,19 @@ async function main() { | |||
| const formatJSON = process.env.FORMAT_JSON === "true"; | |||
| const port = parseInt(process.env.GH_AW_SAFE_OUTPUTS_PORT || "3001", 10); | ||
| if (!Number.isFinite(port) || port <= 0 || port > 65535) { | ||
| throw new Error("GH_AW_SAFE_OUTPUTS_PORT must be a valid port number"); | ||
| } |
| const assignToAgentErrorCount = parseInt(process.env.GH_AW_ASSIGNMENT_ERROR_COUNT || "0", 10); | ||
| if (!Number.isFinite(assignToAgentErrorCount) || assignToAgentErrorCount < 0) { | ||
| throw new Error(`${ERR_VALIDATION}: GH_AW_ASSIGNMENT_ERROR_COUNT must be a non-negative integer`); | ||
| } |
| const maxCount = parseInt(process.env.GH_AW_NOOP_MAX || "0", 10); | ||
| const limitedMaxCount = Number.isFinite(maxCount) && maxCount > 0 ? maxCount : 0; | ||
| const allNoopItems = (result.items || []).filter(/** @param {any} item */ item => item.type === "noop"); | ||
| const noopItems = maxCount > 0 ? allNoopItems.slice(0, maxCount) : allNoopItems; | ||
| const noopItems = limitedMaxCount > 0 ? allNoopItems.slice(0, limitedMaxCount) : allNoopItems; |
| const maxRuns = parseInt(process.env.GH_AW_RATE_LIMIT_MAX?.trim() || "5", 10); | ||
| const windowMinutes = parseInt(process.env.GH_AW_RATE_LIMIT_WINDOW?.trim() || "60", 10); | ||
| if (!Number.isFinite(maxRuns) || maxRuns <= 0 || !Number.isFinite(windowMinutes) || windowMinutes <= 0) { | ||
| throw new Error("Rate limit maximum and window must be positive integers"); | ||
| } |
| const discussionNumber = parseInt(endpoint.split(":")[1], 10); | ||
| if (!Number.isFinite(discussionNumber)) { | ||
| throw new Error(`${ERR_VALIDATION}: Invalid discussion endpoint: ${endpoint}`); | ||
| } |
| const discussionNumber = parseInt(endpoint.split(":")[1], 10); | ||
| if (!Number.isFinite(discussionNumber)) { | ||
| throw new Error(`${ERR_VALIDATION}: Invalid discussion endpoint: ${endpoint}`); | ||
| } |
| const runStartedAt = Date.parse(run.started_at ?? ""); | ||
| const existingStartedAt = Date.parse(existing.started_at ?? ""); | ||
| if (!Number.isFinite(runStartedAt)) { | ||
| continue; | ||
| } | ||
| if (!Number.isFinite(existingStartedAt)) { | ||
| continue; | ||
| } | ||
| if (runStartedAt > existingStartedAt) { | ||
| latestByName.set(run.name, run); | ||
| } |
|
@copilot this PR still needs forward progress before maintainer review.
Failed checks:
Run: https://github.com/github/gh-aw/actions/runs/32552825826
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
@copilot resolve the merge conflicts on this branch. |
|
@copilot this PR is merge-conflicted and still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32573431024
|
…r-input-parsing-validation # Conflicts: # actions/setup/js/check_daily_aic_workflow_guardrail.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved and pushed in merge commit |
…r-input-parsing-validation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Resolved and pushed in |
|
@copilot this PR still needs forward progress before maintainer review. Failed checks to address:
There is also open review feedback from
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…r-input-parsing-validation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
@copilot this PR still needs forward progress before maintainer review. There is still open review feedback to resolve, and the branch was just refreshed by maintenance. Please address the remaining blocking review items, confirm the branch is up to date, rerun the relevant validations, and finish with the Run: https://github.com/github/gh-aw/actions/runs/32578113366
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…r-input-parsing-validation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed through current |
Malformed numeric, date, JSON, and URL inputs could silently propagate, while network and filesystem failures could escape without actionable context.
Run: https://github.com/github/gh-aw/actions/runs/32552086004> Generated by 👨🍳 PR Sous Chef · gpt54 · 28.1 AIC · ⌖ 8.19 AIC · ⊞ 9.5K · ◷
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32552825826> Generated by 👨🍳 PR Sous Chef · gpt54 · 24.2 AIC · ⌖ 8.28 AIC · ⊞ 9.5K · ◷