fix(runtime_import): resolve /plan context substitution failures for slash-command runs#31849
Conversation
…ion findings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes runtime-import prompt rendering for slash-command (workflow_dispatch) runs by ensuring GitHub event fields (issue/discussion/PR numbers, comment id) resolve correctly and {{#if ...}} blocks don’t become erroneously truthy due to placeholder strings.
Changes:
- Add
aw_contextfallback resolution inevaluateExpression()forgithub.event.*fields when the event payload lacks issue/discussion/PR objects. - Change
wrapExpressionsInTemplateConditionals()to inline-evaluate conditions instead of emitting always-truthy placeholder strings. - Update/add unit tests to cover
aw_contextresolution and the new conditional-wrapping behavior.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Promotes gopkg.in/yaml.v3 from indirect to direct dependency. |
| actions/setup/js/runtime_import.cjs | Implements aw_context fallback in expression evaluation; changes conditional wrapping to inline evaluation. |
| actions/setup/js/runtime_import.test.cjs | Adds/updates tests for aw_context expression resolution and updated conditional wrapping output. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| @@ -803,8 +835,21 @@ function wrapExpressionsInTemplateConditionals(content) { | |||
| return match; | |||
| } | |||
| // Evaluate the condition inline so that the template renderer (renderMarkdownTemplate / | ||
| // isTruthy) receives the actual runtime value rather than an __GH_AW__ placeholder string. | ||
| // | ||
| // If we had instead produced `{{#if __GH_AW_GITHUB_EVENT_ISSUE_NUMBER__ }}`, the | ||
| // placeholder literal (a non-empty string) would always be truthy when evaluated by | ||
| // isTruthy() because substitute_placeholders.cjs runs *after* template rendering. | ||
| // Evaluating inline here ensures: | ||
| // • a known number → truthy (block kept when issue is present) | ||
| // • "" or undefined → falsy (block removed when there is no issue/discussion) | ||
| const evaluated = evaluateExpression(trimmed); | ||
| if (evaluated.startsWith("${{")) { | ||
| // Expression could not be resolved — treat as falsy (empty condition) | ||
| return `{{#if }}`; | ||
| } | ||
| return `{{#if ${evaluated} }}`; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — this is a targeted bug fix with accompanying tests, which maps squarely to those two skills.
Key Themes
- Observability gap: When
wrapExpressionsInTemplateConditionalscannot resolve an expression it now silently produces a falsy{{#if }}block. This is correct for the reported bug, but future regressions will be invisible in logs. Acore.warningat that branch would make them immediately visible. ([/diagnose]) - Test state hygiene: The new
aw_context fallbackdescribe block mutatesglobal.context.payload.inputswithout anafterEachcleanup, which can cause order-dependent test failures. ([/tdd]) - Inconsistent test setup: The "multiple conditionals" test uses an inline try/finally while the surrounding suite uses
beforeEach/afterEach. ([/tdd]) - Static object in hot path:
fieldResolversis rebuilt on everyevaluateExpressioncall; it should be a module-level constant. ([/diagnose]) - Unrelated
go.modchange:gopkg.in/yaml.v3is promoted from indirect to direct without explanation; project conventions prefergo.yaml.in/yaml/v3for direct dependencies. ([/diagnose])
Positive Highlights
- ✅ Root cause is well-diagnosed and clearly explained in the PR description — both bugs documented with before/after reasoning
- ✅ The aw_context fallback correctly guards against invalid JSON with a silent catch-and-continue
- ✅ 8 new tests cover the happy path, mismatched
item_type, absent context, and invalid JSON — solid coverage of the new code path - ✅ The inline-evaluation approach in
wrapExpressionsInTemplateConditionalsis architecturally cleaner than the old placeholder-based design
Verdict
No blocking issues — the fix is correct and well-tested. The comments above are improvements worth addressing before merge, with the test state leakage being the highest priority.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.2M
🧪 Test Quality Sentinel ReportTest Quality Score: 83/100✅ Excellent
Test Classification DetailsView all 18 test classifications
Test Inflation Note
No action required — this is a well-motivated case of intentional scenario coverage. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25776603690
|
|
@copilot review all comments |
…apExpressionsInTemplateConditionals
- Only process expressions whose root matches a known GitHub Actions
namespace (github., needs., steps., env., inputs.) to prevent
non-GH dotted identifiers like experiments.foo from being collapsed
to falsy {{#if }} (addresses review comment on lines 826-836)
- Emit {{#if true}} / {{#if }} boolean sentinels instead of inlining
the raw resolved value to prevent template tag injection via values
containing '}}' (addresses review comment on lines 838-852)
- Import isTruthy from is_truthy.cjs to correctly classify resolved
values (handles edge cases like "false", "0", "null")
- Update tests to expect boolean sentinels and add experiments.foo
regression cases
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both review comments are addressed in the latest commits: Comment 1 (lines 826-836) — Comment 2 (lines 838-852) — raw value injection via Two regression tests for |
|
@copilot lint js, fix wasm |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit:
|
Bug Fix
What was the bug?
Audited
plan.lock.yml(lock file looks correct) and the last run (ID25774562076). The activation job flagged 6 unresolved${{ }}expressions —github.event.issue.numberandgithub.event.discussion.numberappeared literally in the prompt body, and both the issue and discussion conditional blocks were always included simultaneously.Two distinct bugs in
runtime_import.cjs:Bug 1 — expressions not resolved in body text
evaluateExpressiononly checkscontext.payload.issue.number, which isundefinedforworkflow_dispatch(how slash commands execute). No fallback existed for the aw_context input.Bug 2 —
{{#if github.event.issue.number}}always truthywrapExpressionsInTemplateConditionalswrapped conditions as{{#if __GH_AW_GITHUB_EVENT_ISSUE_NUMBER__ }}.isTruthy()treated the non-empty placeholder string as truthy.substitute_placeholders.cjsruns after template rendering, so the substitution was already too late.How did you fix it?
evaluateExpression— adds an aw_context fallback: whencontext.payload.issueis absent, parsesinputs.aw_contextJSON and resolves the correct field byitem_type:wrapExpressionsInTemplateConditionals— instead of wrapping in${{ }}to produce an always-truthy placeholder, now callsevaluateExpression()inline. The template renderer receives the actual value ("123"→ truthy,""→ falsy):Testing
wrapExpressionsInTemplateConditionalsunit tests to assert inline-evaluated output instead of${{ }}wrappingevaluateExpressiontests covering aw_context resolution: correctitem_type, mismatcheditem_type, absent context, and invalid JSON