fix(eslint): detect dropped await on local core.summary aliases#43404
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ummary-write rule Extends the rule to detect dropped `await` on local variable aliases of `core.summary`, resolving the false negative described in #43324. Two alias patterns are now recognised: - `const/let summary = core.summary; summary.write()` - `const { summary } = core; summary.addRaw(x).write()` Re-assigned `let` bindings are conservatively rejected (their post-reassignment source is unknown). Non-core initialisers (e.g. `fs.summary`) are also ignored. Tests added: 5 new invalid cases (both corpus patterns + rename destructure + non-async + let variant) and 3 new valid cases (awaited alias, re-assigned let, non-core init). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…eview Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage — Run §28715668077
Detects dropped await on local core.summary aliases. +80/-2 TS rule, +40 test. Low risk eslint bugfix.
|
There was a problem hiding this comment.
Pull request overview
Fixes a false negative in the require-await-core-summary-write ESLint rule by detecting core.summary calls when core.summary is first assigned/destructured into a local alias, ensuring dropped await on summary.write() is correctly flagged across common corpus patterns.
Changes:
- Added scope-based alias resolution (
isCoreSummaryAlias) to identify single-assignment locals bound tocore.summary/{ summary } = core. - Extended root-tracing logic (
rootsSummaryOrAlias) to recognize alias-rooted chains likesummary.addRaw(x).write(). - Added test coverage for alias and destructuring patterns, including suggestion output behavior.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-await-core-summary-write.ts | Adds scope-aware alias detection and extends summary-root tracing to include local aliases. |
| eslint-factory/src/rules/require-await-core-summary-write.test.ts | Adds new valid/invalid cases covering aliasing, reassignment, destructuring, and suggestion behavior. |
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: 2
- Review effort level: Low
| // Pattern 1: const/let summaryVar = core.summary; | ||
| // rootsSummary covers both `core.summary` and chained inits like | ||
| // `core.summary.addRaw(x)` (which still returns a Summary object). | ||
| if (declarator.id.type === "Identifier") { | ||
| return rootsSummary(declarator.init); | ||
| } |
There was a problem hiding this comment.
Fixed in 72d11d2. Pattern 1 now explicitly rejects .write() call-expressions before calling rootsSummary: const s = core.summary.write() is no longer treated as a Summary alias. A valid test was added to codify this: const s = core.summary.write(); s.write() is not flagged.
| // Destructure with rename: const { summary: s } = core; s.write(); | ||
| { | ||
| code: `async function f() { const { summary: s } = core; s.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const { summary: s } = core; await s.write(); }` }] }], | ||
| }, | ||
| // Outside async: flagged, no suggestion |
There was a problem hiding this comment.
Fixed in 72d11d2. Added two new invalid test cases for coreObj: direct alias (const summary = coreObj.summary; summary.write()) and destructuring (const { summary } = coreObj; summary.write()).
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions: 0). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report
📊 Metrics (4 new tests)
Note: Test file is TypeScript (
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Scope-walk test gap: The
while (currentScope.upper)path inisCoreSummaryAliasis never exercised by the new tests — all test cases use single-scope setups. A cross-scope alias (alias declared in outer function, used in inner) should be added. - Unbounded recursion in
rootsSummaryOrAlias: The recursive call on deeply chainedCallExpressions has no depth guard. Not a practical risk, but a depth cap or iterative rewrite would remove the theoretical concern. - Undocumented chained-init subtlety:
isCoreSummaryAliascallsrootsSummary(declarator.init), which also matchescore.summary.addRaw(x)as valid initialisers. This is intentional but undocumented, making it easy to misread on future maintenance. - Missing valid-case test for alias without
.write():summary.addRaw(x)with no chained.write()should not be flagged; no test guards against a future false positive here.
Positive Highlights
- ✅ Excellent PR description: root cause, two corpus patterns, and zero-false-positive guarantee all clearly documented.
- ✅ Conservative re-assignment guard (
ref.isWrite() && !ref.init) is precise and well-commented. - ✅ Good use of the fast-path in
rootsSummaryOrAlias— existing behaviour unchanged, new code is purely additive. - ✅ Test structure cleanly separates valid and invalid cases for each new pattern.
- ✅
defs.length !== 1guard prevents false positives on merged/duplicate bindings.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 65.1 AIC · ⌖ 5.77 AIC · ⊞ 6.6K
Comment /matt to run again
| code: `function f() { const summary = core.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [] }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/tdd] No test for alias used across a nested function scope — the scope-walk (currentScope.upper) path in isCoreSummaryAlias is never exercised.
💡 Suggested test case to add inside the valid/invalid blocks
// Cross-scope: alias declared in outer, used in inner async — should be flagged
{
code: `async function outer() {
const summary = core.summary;
return (async function inner() { summary.write(); })();
}`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function outer() {
const summary = core.summary;
return (async function inner() { await summary.write(); })();
}` }] }],
},Without this, the while (currentScope !== null) { currentScope = currentScope.upper; } loop is only tested with a single-scope setup and a regression here would go undetected.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 72d11d2. Added a cross-scope invalid test: alias declared in the outer async function, write() called inside a nested inner async function. The scope-walk in isCoreSummaryAlias finds the variable via currentScope.upper and the call is correctly flagged with the await suggestion.
|
|
||
| // Chained alias: `summary.addRaw(x).write()` — callee.object is a CallExpression | ||
| // rooted on the alias identifier. | ||
| if (node.type === "CallExpression" && node.callee.type === "MemberExpression") { |
There was a problem hiding this comment.
[/diagnosing-bugs] rootsSummaryOrAlias has unbounded recursion on deeply nested CallExpression chains — e.g. summary.addRaw(a).addRaw(b).addRaw(c).write() calls itself once per call-chain depth.
💡 Note on risk and mitigation
In practice, ESLint AST depths for real source code are shallow (< ~20 levels), so a stack overflow is not a real-world risk. However:
- The recursive call passes the same function, so there is no progress guarantee.
- A dedicated
while-loop or a depth guard (e.g.depth > 32 → return false) would remove the theoretical concern and make the intent explicit.
function rootsSummaryOrAlias(node: TSESTree.Node, depth = 0): boolean {
if (depth > 32) return false; // guard against extreme nesting
if (rootsSummary(node)) return true;
if (node.type === "Identifier") return isCoreSummaryAlias(node);
if (node.type === "CallExpression" && node.callee.type === "MemberExpression") {
return rootsSummaryOrAlias(node.callee.object, depth + 1);
}
return false;
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 72d11d2. rootsSummaryOrAlias now accepts a depth parameter (default 0) and returns false when depth > 32. The recursive call passes depth + 1. Real source code is well below this limit; the guard removes the theoretical concern.
| // rootsSummary covers both `core.summary` and chained inits like | ||
| // `core.summary.addRaw(x)` (which still returns a Summary object). | ||
| if (declarator.id.type === "Identifier") { | ||
| return rootsSummary(declarator.init); |
There was a problem hiding this comment.
[/diagnosing-bugs] Pattern 1 in isCoreSummaryAlias calls rootsSummary(declarator.init), which accepts chained initialisers like core.summary.addRaw(x). That means const s = core.summary.addRaw(x) would be treated as a core.summary alias — but addRaw returns Summary, not a Promise, so this is intentional and correct. Consider adding a brief comment or a test case to make the intent explicit, since this subtlety is easy to miss in future maintenance.
💡 Suggested clarifying comment + optional test
Add to the existing comment block:
// Note: rootsSummary also accepts chained initialisers like `core.summary.addRaw(x)`
// because addRaw returns Summary — the alias still wraps a Summary object.Optional extra valid-case test:
`async function f() { const s = core.summary.addRaw("x"); await s.write(); }`@copilot please address this.
There was a problem hiding this comment.
Added in 72d11d2. Pattern 1 now has a comment block explicitly documenting that addRaw returns Summary so chained inits are valid, and that .write() is explicitly rejected because it returns Promise<Summary>. A valid test const s = core.summary.addRaw("x"); await s.write() was added to codify the chained-init intent.
| code: `function f() { const summary = core.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [] }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/tdd] No test verifies what happens when the destructured alias is called directly without .write() — e.g. summary.addRaw(x) alone (no chained .write()). This is a valid case and should be in the valid block to prevent future false positives.
💡 Suggested test
// Using alias for addRaw only (no write) — must NOT be flagged
`async function f() { const { summary } = core; summary.addRaw("x"); }`@copilot please address this.
There was a problem hiding this comment.
Fixed in 72d11d2. Added a valid test: async function f() { const { summary } = core; summary.addRaw("x"); } — using the alias for addRaw only (no .write()) is not flagged.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 77.1 AIC · ⌖ 5.87 AIC · ⊞ 4.8K
| errors: [{ messageId: "requireAwait", suggestions: [] }], | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
The new test block covers core.* aliases but not the coreObj variant. Since coreObj is in CORE_ALIASES and the fix advertises coverage for any CORE_ALIASES member, a test case like:
{
code: `async function f() { const summary = coreObj.summary; summary.write(); }`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const summary = coreObj.summary; await summary.write(); }` }] }],
},would guard against a future regression there.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 72d11d2. Added an invalid test for the coreObj direct-alias pattern: const summary = coreObj.summary; summary.write() is now flagged with the await suggestion.
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=0; skipped_last_comment_from_sous_chef=4; skipped_cooldown=1; nudged=0; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
|
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Reject .write() call-expressions as alias initializers in Pattern 1
(const s = core.summary.write() returns Promise, not Summary)
- Add depth guard (> 32) to rootsSummaryOrAlias to prevent unbounded
recursion on deeply chained CallExpression trees
- Add clarifying comment explaining addRaw chained inits are intentionally
accepted (addRaw returns Summary, not Promise)
- Tests: alias without .write() (no false positive)
- Tests: chained init core.summary.addRaw(x) — awaited alias is valid
- Tests: .write() as initializer is rejected — not treated as alias
- Tests: coreObj alias const summary = coreObj.summary; summary.write()
- Tests: coreObj destructuring const { summary } = coreObj; summary.write()
- Tests: cross-scope alias (declared in outer, used in inner async)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
require-await-core-summary-writerule had a false negative whencore.summarywas aliased to a local variable —getRootIdentifieronsummary.write()returned"summary", which failed theCORE_ALIASEScheck and silently skipped the call. This pattern is live in two corpus files.Changes
isCoreSummaryAlias(identifier)— new helper insidecreate()that uses ESLint scope analysis to check whether an identifier is a single-assignment local bound tocore.summary. Two patterns are tracked:const/let summary = core.summaryconst { summary } = core/const { summary: alias } = coreRe-assigned
letbindings are rejected viaref.isWrite() && !ref.init; non-core initialisers are ignored.rootsSummaryOrAlias(node)— replaces the directrootsSummarycall inExpressionStatement. Delegates torootsSummaryfirst (fast path), then resolvesIdentifiernodes through alias lookup and handles chained calls (summary.addRaw(x).write()).Now flagged
Still not flagged (zero false positives)