feat(eslint): add require-await-core-summary-write rule#43170
Conversation
…tions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…expression test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
|
🔬 Test Quality Sentinel is analyzing test quality on this pull request... |
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
There was a problem hiding this comment.
Pull request overview
This PR adds a custom ESLint rule to prevent un-awaited core.summary.write() calls (which return a Promise<Summary> and can be dropped/truncated if the process exits too early), and updates existing workflow helper scripts to await those summary writes.
Changes:
- Added a new ESLint rule
require-await-core-summary-write(with tests) that flags bare expression-statement.summary.write()calls and offers anawaitsuggestion in async contexts. - Registered and enabled the rule at
"warn"in the eslint-factory plugin/config. - Updated multiple
actions/setup/js/*.cjsscripts toawaitsummary writes.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-await-core-summary-write.ts | Implements the new rule that detects un-awaited .summary.write() calls on summary chains and offers an await suggestion in async functions. |
| eslint-factory/src/rules/require-await-core-summary-write.test.ts | Adds test coverage for valid/invalid patterns, chaining, computed access, aliasing, and suggestion behavior. |
| eslint-factory/src/index.ts | Registers the new rule in the eslint-factory plugin export. |
| eslint-factory/eslint.config.cjs | Enables the new rule at warning level. |
| actions/setup/js/upload_assets.cjs | Awaits core.summary.write() to ensure summary output is flushed reliably. |
| actions/setup/js/parse_mcp_scripts_logs.cjs | Awaits core.summary.addRaw(...).write() to avoid dropping the Promise. |
| actions/setup/js/parse_mcp_gateway_log.cjs | Attempts to await summary writes, but currently introduces a syntax error due to await in a non-async function. |
| actions/setup/js/parse_firewall_logs.cjs | Awaits core.summary.addRaw(...).write() to avoid dropping the Promise. |
| actions/setup/js/log_parser_bootstrap.cjs | Awaits core.summary.addRaw(...).write() at both call sites. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Low
| coreObj.summary.addRaw(timelineMd); | ||
| } | ||
|
|
||
| coreObj.summary.write(); | ||
| await coreObj.summary.write(); | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (9 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(eslint): add require-await-core-summary-write rule
The new ESLint rule and test suite are well-structured — the rule correctly targets bare ExpressionStatement call sites, propagated usages (return/assign) are rightly exempted, and the suggestion correctly guards itself behind isInsideAsyncFunction. The five caller-site fixes in the .cjs action scripts are also correct in principle.
However, there is one blocking correctness bug introduced by this PR.
🔴 Blocking: await inside a non-async function in parse_mcp_gateway_log.cjs
writeStepSummaryWithTokenUsage at line 204 is not declared async, so the added await coreObj.summary.write() at line 236 is a SyntaxError and will crash the process before any step summary is written. This is a regression — the pre-PR code at least executed without crashing.
See the inline comment for the two-part fix needed (mark the function async, and await its three call sites in main()).
Non-blocking observations
- Rule coverage: the rule only detects patterns rooted in a
.summarymember whose property name issummary. If a caller passes asummaryobject directly (e.g.writeStepSummary(core.summary)and then callscoreObj.write()inside), the rule won't fire — but that's an acceptable limitation and is consistent with the PR description. rootsSummarytraversal depth: the recursive walk handlescore.summary.addX().addY().write()chains of arbitrary depth, which is correct and future-proof.- Test duplication: the "suggestion: inserts 'await' before the expression" test suite repeats cases already covered by earlier
itblocks. Deduplication is minor but keeps the test file lean.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer
| } | ||
|
|
||
| coreObj.summary.write(); | ||
| await coreObj.summary.write(); |
There was a problem hiding this comment.
Bug: await used inside a non-async function — this will throw a SyntaxError at runtime.
writeStepSummaryWithTokenUsage is declared as a plain (non-async) function at line 204:
function writeStepSummaryWithTokenUsage(coreObj) {Adding await coreObj.summary.write() inside it produces SyntaxError: await is only valid in async functions at parse time. The process will fail before writing the step summary.
Two changes are required:
- Mark the function
async:async function writeStepSummaryWithTokenUsage(coreObj) {
- Await all three call sites in
main()(~lines 907, 938, 977):await writeStepSummaryWithTokenUsage(core);
Without both changes the bug the rule was meant to fix (unawaited core.summary.write()) is traded for an outright syntax crash.
@copilot please address this.
🤖 PR Triage
Score Breakdown
Ready for human review once CI completes.
|
There was a problem hiding this comment.
❌ REQUEST_CHANGES — one crash-level bug must be fixed before merge
The ESLint rule itself is well-structured and the intent is correct, but the manual fix in parse_mcp_gateway_log.cjs introduces a runtime SyntaxError that will make the module unloadable. That alone blocks merge.
📋 Findings summary
🔴 Critical (1)
awaitin non-async function (parse_mcp_gateway_log.cjs:236):writeStepSummaryWithTokenUsageis a plain synchronous function.awaithere is aSyntaxError— Node.js will refuse to load the module. All three call sites inmain()(lines 907, 938, 977) break. The ESLint rule deliberately withholds itsawait-insert suggestion in non-async contexts; this fix was applied manually, bypassing that guard. Fix: make the functionasyncandawaitall three callers.
🟠 High (1)
rootsSummaryfalse positives (require-await-core-summary-write.ts:27): The predicate matches any object with a.summary.write()method, not just@actions/core.db.summary.write(),logger.summary.write()etc. would all trigger the rule. Root identifier should be constrained to knowncorealiases.
🟡 Medium (2)
- Unused
nodeparameter inisInsideAsyncFunction— dead parameter implies false intent; prefix_nodeor remove it. - No invalid/suggestion test for
asyncarrow or function expression —ArrowFunctionExpressionandFunctionExpressionare inASYNC_FUNCTION_TYPESbut untested in theinvalidpath; a silent regression removing them would not be caught.
🔎 Code quality review by PR Code Quality Reviewer
Comment /review to run again
| } | ||
|
|
||
| coreObj.summary.write(); | ||
| await coreObj.summary.write(); |
There was a problem hiding this comment.
SyntaxError: await inside a non-async function — this module will crash at require-time.
Line 204 declares function writeStepSummaryWithTokenUsage(coreObj) — a regular synchronous function. Using await inside a non-async function is a SyntaxError in Node.js CJS; the module will fail to load entirely, breaking all three callers at lines 907, 938, and 977 in main().
💡 Suggested fix
Make the function async and update all three call sites to await it:
async function writeStepSummaryWithTokenUsage(coreObj) {
// ... existing body ...
await coreObj.summary.write();
}
// In main() — all three call sites (lines 907, 938, 977):
await writeStepSummaryWithTokenUsage(core);Verified with Node.js:
SyntaxError: await is only valid in async functions and the top level bodies of modules
The new ESLint rule correctly refuses to offer the await suggestion when outside an async function. The manual fix was applied anyway, bypassing that guard.
| * - `core.summary.addHeading(...).addRaw(x)` | ||
| * - `coreObj.summary` (any identifier alias) | ||
| */ | ||
| function rootsSummary(node: TSESTree.Node): boolean { |
There was a problem hiding this comment.
rootsSummary matches any object with .summary.write() — not just @actions/core — producing false positives on unrelated code.
The function returns true for any MemberExpression whose property is summary, regardless of the root identifier. This means db.summary.write(), logger.summary.write(), document.summary.write() would all be flagged even though they have nothing to do with GitHub Actions.
💡 Suggested fix
Constrain the root to a known core-like identifier. One approach: check that the deepest object in the chain is an Identifier whose name matches a configurable set (default: ["core", "coreObj"]):
const DEFAULT_CORE_NAMES = new Set(["core", "coreObj"]);
function getRootIdentifier(node: TSESTree.Node): string | null {
if (node.type === "Identifier") return node.name;
if (node.type === "MemberExpression") return getRootIdentifier(node.object);
if (node.type === "CallExpression") return getRootIdentifier(node.callee);
return null;
}
function rootsSummary(node: TSESTree.Node, coreNames: Set<string>): boolean {
const root = getRootIdentifier(node);
if (root === null || !coreNames.has(root)) return false;
// ... existing summary property check ...
}Alternatively, add a rule option coreNames: string[] (schema) so projects using non-standard import aliases can configure it.
Without this constraint, any library using .summary.write() as a non-Promise method will get spurious warnings.
| * whether `await` is currently valid — the suggestion is only safe to apply | ||
| * in an async context. | ||
| */ | ||
| function isInsideAsyncFunction(node: TSESTree.Node, ancestors: TSESTree.Node[]): boolean { |
There was a problem hiding this comment.
isInsideAsyncFunction first parameter node is unused dead code.
The function body only ever iterates ancestors; node is never read. This creates a misleading API that implies node participates in the scope calculation.
💡 Suggested fix
Either remove the parameter entirely or prefix it with _ to signal intentional discard (matching TypeScript/ESLint conventions):
// Option A — remove it
function isInsideAsyncFunction(ancestors: TSESTree.Node[]): boolean {
// Option B — mark as intentionally unused
function isInsideAsyncFunction(_node: TSESTree.Node, ancestors: TSESTree.Node[]): boolean {Update the one call site accordingly:
const suggest = isInsideAsyncFunction(ancestors) ? [...] : [];| }); | ||
| }); | ||
|
|
||
| it("invalid: bare core.summary.write() is flagged", () => { |
There was a problem hiding this comment.
No test for async arrow function or async function expression in the invalid path — a regression in ASYNC_FUNCTION_TYPES would go undetected.
Every invalid test case uses async function f() {...} (FunctionDeclaration only). ArrowFunctionExpression and FunctionExpression are in ASYNC_FUNCTION_TYPES and assumed to work, but removing either would not break any test.
💡 Suggested fix
Add at least these two invalid cases (one per missing function form):
// async arrow function
{
code: `const f = async () => { core.summary.write(); };`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `const f = async () => { await core.summary.write(); };` }] }],
},
// async function expression
{
code: `const f = async function() { core.summary.write(); };`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `const f = async function() { await core.summary.write(); };` }] }],
},Also worth adding: a non-async arrow const f = () => { core.summary.write(); }; should be flagged with no suggestion (same as the non-async FunctionDeclaration case already tested).
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd — requesting changes on one correctness bug and two test-quality issues.
📋 Key Themes & Highlights
Key Themes
- Critical correctness bug:
awaitis inserted intowriteStepSummaryWithTokenUsageinparse_mcp_gateway_log.cjs, which is not anasyncfunction — this is a syntax error that will crash the Action. - False positive surface:
rootsSummaryflags any.summary.write()call regardless of the receiver object, which will produce false positives on non-core objects. - Test fixture issues: one fixture uses an undeclared variable (
p = core.summary.write()), and an async IIFE pattern is not exercised.
Positive Highlights
- ✅ Excellent motivation: the PR description clearly explains the root cause and the truncation risk.
- ✅ The suggestion logic is safe — it correctly withholds the
awaitfix outside async contexts. - ✅ Comprehensive test coverage across chained calls, computed access, and alias patterns.
- ✅ Five real violations fixed in a single PR — proves the rule finds real bugs immediately.
- ✅ Clean integration into the existing plugin structure (
index.ts,eslint.config.cjs).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
Comment /matt to run again
| } | ||
|
|
||
| coreObj.summary.write(); | ||
| await coreObj.summary.write(); |
There was a problem hiding this comment.
[/diagnosing-bugs] await is added inside writeStepSummaryWithTokenUsage which is not an async function — this is a syntax error in a CJS module and will crash the Action at runtime.
💡 Correct fix
The ESLint rule's own suggestion logic correctly withholds the await fix outside async contexts; this change should do the same. You need to either make the function async (and propagate the change to all callers), or return the promise:
// Option A — make the function async
async function writeStepSummaryWithTokenUsage(coreObj) {
// ...
await coreObj.summary.write();
}
// then await every call site
// Option B — return the Promise (no change to callers needed)
function writeStepSummaryWithTokenUsage(coreObj) {
// ...
return coreObj.summary.write();
}The other four fixes (log_parser_bootstrap.cjs, parse_firewall_logs.cjs, parse_mcp_scripts_logs.cjs, upload_assets.cjs) are all inside async function main() and are correct.
@copilot please address this.
| if (node.type === "CallExpression" && node.callee.type === "MemberExpression") { | ||
| return rootsSummary(node.callee.object); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
[/tdd] rootsSummary accepts any identifier with a .summary property — fs.summary.write(), db.summary.write(), etc. would all be flagged as false positives.
💡 Tighten the check + add tests
The check should also verify the receiver of .summary is a known alias for @actions/core. One safe approach: look for an Identifier whose name ends with core (or is in a configurable allow-list), instead of allowing any identifier:
// check the root of the chain is a known core identifier
function isCoreReceiver(node: TSESTree.Node): boolean {
return node.type === 'Identifier' && /core/i.test(node.name);
}And add test cases:
// should NOT be flagged
'fs.summary.write();',
'db.summary.write();',
'foo.bar.summary.write();', // nested but no core ancestor@copilot please address this.
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { await core.summary.addRaw(summary).write(); }` }] }], | ||
| }, | ||
| { | ||
| code: `async function f() { core.summary.addHeading("Title").addRaw(body).write(); }`, |
There was a problem hiding this comment.
[/tdd] The valid: returned and assigned calls are not flagged test includes p = core.summary.write() but the test file declares p nowhere — this would throw a ReferenceError in strict mode and is an invalid test fixture.
💡 Fix the fixture
Either declare p first or use the var keyword so the test fixture is itself valid JS:
// declare p before assignment
`async function f() { let p; p = core.summary.write(); }`
// or use a VariableDeclaration to keep it simple
`async function f() { const p = core.summary.write(); }`This also means the assignment-expression path is only tested by the const p = case in the same group, which already has full coverage. The undeclared-variable case can be dropped.
@copilot please address this.
| }); | ||
|
|
||
| it("invalid: coreObj alias and computed access are flagged", () => { | ||
| cjsRuleTester.run("require-await-core-summary-write", requireAwaitCoreSummaryWriteRule, { |
There was a problem hiding this comment.
[/tdd] Missing edge-case test: bare core.summary.write() inside a top-level async IIFE (async () => { ... })() — the suggestion should still be offered since the enclosing function is async.
💡 Add the test case
{
code: `(async () => { core.summary.write(); })()`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `(async () => { await core.summary.write(); })()` }] }],
},isInsideAsyncFunction already walks ancestors so this should work out of the box, but an explicit test guards against future regressions.
@copilot please address this.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 28ffa74. I fixed the non-async |
|
🎉 This pull request is included in a new release. Release: |
core.summary.write()returnsPromise<Summary>. Calling it withoutawaitsilently discards the promise — if the Node.js process exits before the microtask queue drains, the step summary is truncated or missing entirely. Five existing violations inactions/setup/jsdemonstrate the pattern.New ESLint rule:
require-await-core-summary-writeExpressionStatementcalls to.summary.write()— returned/assigned calls propagate the promise and are not flaggedcore.summary.write(),core.summary.addRaw(x).write(), deep chains likecore.summary.addHeading(...).addRaw(x).write(), anycoreAlias.summary.write(), and computed accesscore.summary["write"]()awaitbefore the expression, only offered when inside anasyncfunction (applying it outside would produce a syntax error)eslint-factory/src/index.tsand enabled at"warn"ineslint.config.cjsExisting violations fixed
Added
awaitto all 5 un-awaited call sites acrossparse_mcp_scripts_logs.cjs,log_parser_bootstrap.cjs(×2),parse_firewall_logs.cjs,parse_mcp_gateway_log.cjs, andupload_assets.cjs.