Prevent GH_AW_TOOLS_META_JSON expression-size overflows in compiled workflows#53965
GH_AW_TOOLS_META_JSON expression-size overflows in compiled workflows#53965Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
GH_AW_TOOLS_META_JSON expression-size overflows in compiled workflows
|
@copilot add tests |
There was a problem hiding this comment.
Pull request overview
Prevents oversized expression-bearing tools metadata from exceeding GitHub Actions limits.
Changes:
- Replaces embedded expressions with runtime environment placeholders.
- Resolves placeholders before generating tools metadata.
- Adds Go and JavaScript regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_safe_outputs.go |
Sanitizes tools metadata expressions. |
pkg/workflow/mcp_setup_safe_outputs_test.go |
Tests expression extraction. |
actions/setup/js/generate_safe_outputs_tools.cjs |
Restores runtime placeholders. |
actions/setup/js/generate_safe_outputs_tools.test.cjs |
Tests placeholder resolution. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| * @returns {string} | ||
| */ | ||
| function resolveEnvStringPlaceholders(value) { | ||
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match); |
There was a problem hiding this comment.
Fixed in e403f67: resolveEnvStringPlaceholders now JSON-escapes the resolved env value (JSON.stringify(value).slice(1, -1)) before splicing it into the JSON text, so quotes/backslashes/newlines in the resolved value no longer corrupt tools_meta.json. Added regression tests covering these characters.
| sanitizedToolsMeta := toolsMetaJSON | ||
| for _, expr := range sliceutil.SortedKeys(expressionEnvVars) { | ||
| envName := expressionEnvVars[expr] | ||
| envValues[envName] = expr |
There was a problem hiding this comment.
Fixed in e403f67: buildToolsMetaRuntimeData now decodes the matched JSON string fragment via decodeJSONStringFragment (reversing encoding/json's HTML-escaping of <, >, &) before storing it as the step env value, while still replacing the encoded form in sanitizedToolsMeta. Added TestBuildToolsMetaRuntimeDataDecodesHTMLEscapedExpression to cover this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added tests in 3e2b5f6 (approx, see latest commit): Go tests for
Added tests in 8b6b0ba: Go tests for |
|
@copilot Please address the remaining maintainer-visible blockers on this PR in one pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please address the remaining maintainer-visible blockers on this PR in one pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed both in e403f67:
Added regression tests for both. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR Triage
Compiler fix: prevents GH_AW_TOOLS_META_JSON from exceeding GitHub Actions 21KB expression limit, a real compile-blocking regression.
|
|
@copilot This PR looks close, but it still needs a maintainer-facing finish pass.
|
Verified the branch is up to date and both prior review threads (JSON-safe placeholder escaping in Local validation on HEAD (900b17f):
I can't resolve the two GitHub review-thread conversations from this environment (no thread-resolve capability available to me), but their underlying issues are fixed in code as noted above. CI on this HEAD hasn't re-run since my push and needs a maintainer re-trigger (close/reopen, Remaining: awaiting maintainer review/approval (mergeable_state is |
PR TriageCategory: bug · Risk: high · Score: 70/100 (impact 35 + urgency 25 + quality 10) Critical compiler correctness fix (expression-size overflow) with CI green; review history has churn (dismissed review) — highest score, needs prompt human attention. Automated triage — see run report for full details.
|
|
@copilot This PR looks close, but it still still needs a maintainer-facing finish pass. Please do one final forward-progress sweep:
|
Replied on-thread to both previously unresolved review comments (r3812371390, r3812371435) confirming the fixes landed in e403f67, with commit references and test names. I don't have a thread-resolve capability in this environment, so the threads remain open pending a maintainer/reviewer marking them resolved. Branch base is unchanged (still on Local validation on HEAD (900b17f), re-verified:
Remaining risk/blocker: |
|
@copilot This PR looks close, but it still needs a maintainer-facing finish pass. Please do one final forward-progress sweep:
Open review threads (newest first):
If both are already fixed on the current head, reply on-thread with the exact commit/test evidence so reviewers can resolve them quickly.
|
Both threads already have on-thread replies with commit/test evidence from the prior pass: r3816286526 and r3816287113. No code changed since then; HEAD is still 900b17f, branch is not behind |
The daily compatibility audit surfaced a compiler-side regression: large, compiler-generated
GH_AW_TOOLS_META_JSONblocks could fail compilation when they contained${{ }}expressions and crossed GitHub Actions’ 21KB expression-bearing limit. This failure mode affected otherwise valid workflows with large tool metadata.Compiler path: sanitize tools-meta expression payload
pkg/workflow/mcp_setup_safe_outputs.go, added tools-meta runtime data preparation that:${{ ... }}in generated tools meta JSON,${GH_AW_*}placeholders in the emitted YAML block scalar,Runtime path: resolve placeholders before materializing
tools_meta.jsonactions/setup/js/generate_safe_outputs_tools.cjs, added placeholder resolution forGH_AW_TOOLS_META_JSONprior to writingtools_meta.json.Regression coverage
GH_AW_TOOLS_META_JSONis provided via env.Generated by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/32249206937> Generated by 👨🍳 PR Sous Chef · gpt54 · 23.7 AIC · ⌖ 8.26 AIC · ⊞ 9.3K · ◷