require-await-core-summary-write: tighten isCoreLikeIdentifier to exact alias set#43382
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…wn alias set Replace the broad `/^core/i` prefix heuristic with an exact allow-list (CORE_ALIASES = new Set(["core", "coreObj"])) to prevent false positives from unrelated objects like coreCache, coreData, or coreference. Add a doc comment explaining the decision and listing the two verified @actions/core binding names in the corpus. Update the rootsSummary JSDoc to reference CORE_ALIASES instead of claiming "any identifier alias". Add a valid test block asserting that coreCache, coreData, and coreference are NOT flagged, codifying the tightened boundary. Closes #43325 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Hey This looks clean and ready for review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43382 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR tightens the require-await-core-summary-write ESLint rule’s detection of @actions/core summary chains by replacing a broad ^core prefix match with an exact allow-list of known aliases, preventing false positives on unrelated identifiers.
Changes:
- Replace the
isCoreLikeIdentifier/^core/iheuristic with aSet-based allow-list (CORE_ALIASES) for exact alias matching. - Update rule documentation to clarify the alias constraint.
- Add tests ensuring
coreCache,coreData, andcoreferenceare not incorrectly flagged.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-await-core-summary-write.ts | Replaces prefix-based “core” matching with an exact allow-list for known @actions/core aliases. |
| eslint-factory/src/rules/require-await-core-summary-write.test.ts | Adds regression coverage to ensure non-alias identifiers starting with core are not flagged. |
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: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change is correct and well-executed. A few observations:\n\n- Regex → Set: swapping /^core/i for Set.has() eliminates the false-positive class cleanly. The implicit narrowing from case-insensitive to case-sensitive matching is intentional and safe (no real @actions/core binding uses mixed/uppercase).\n- Comment quality: the block comment above CORE_ALIASES documents corpus provenance and gives future maintainers a clear extension point — no guesswork needed.\n- Test coverage: the new valid block with coreCache, coreData, and coreference directly codifies the bug that was fixed and will catch regressions.\n\nNo actionable issues. 🟢
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 20.8 AIC · ⌖ 5.87 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Undocumented behaviour change: the regex-to-
Setswap silently removes case-insensitivity (see inline comment onrequire-await-core-summary-write.ts:32). Worth a one-line note in the comment block so future maintainers know it was intentional. - Test description couples to internals: the new
it("valid: identifiers outside CORE_ALIASES...")name references the constant rather than the observable behaviour (see inline comment on the test file).
Positive Highlights
- ✅ Root cause properly addressed — the old prefix regex was the right thing to fix, not just the test expectations.
- ✅ Regression test added alongside the fix — exactly the right TDD move.
- ✅ Comment block is thorough and provides a clear extension point for new aliases.
- ✅ Diff is surgical: only the matching heuristic and its JSDoc were touched; nothing else shifted.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36 AIC · ⌖ 7 AIC · ⊞ 6.6K
Comment /matt to run again
| // Known aliases (extend here when a new verified binding is introduced): | ||
| // "core" — conventional require("@actions/core") name in github-script steps | ||
| // "coreObj" — alias used in parse_mcp_gateway_log.cjs | ||
| const CORE_ALIASES = new Set(["core", "coreObj"]); |
There was a problem hiding this comment.
[/diagnosing-bugs] The switch from /^core/i to CORE_ALIASES.has() silently drops case-insensitivity — Core.summary.write() and CORE.summary.write() were flagged before and are now silently passed. This is almost certainly the correct decision, but the comment block doesn't call it out, leaving a future maintainer to wonder if it was accidental.
💡 Suggested addition to the comment block
// Known aliases (extend here when a new verified binding is introduced):
// "core" — conventional require("`@actions/core`") name in github-script steps
// "coreObj" — alias used in parse_mcp_gateway_log.cjs
//
// Note: matching is intentionally case-sensitive; no uppercase aliases are known
// in the corpus, and the prior /^core/i was over-broad.
const CORE_ALIASES = new Set(["core", "coreObj"]);@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("valid: identifiers outside CORE_ALIASES are not flagged even with .summary.write() chain", () => { |
There was a problem hiding this comment.
[/tdd] The test name couples to the internal constant name CORE_ALIASES. If the constant is ever renamed, the test description becomes stale. Prefer a behaviour-description name that does not reference internals.
💡 Suggested rename
// Before:
it("valid: identifiers outside CORE_ALIASES are not flagged even with .summary.write() chain", ...
// After:
it("valid: identifiers that only start with 'core' but are not known `@actions/core` aliases are not flagged", ...This reads as a specification — the reader understands the intent without needing to know the constant name.
@copilot please address this.
There was a problem hiding this comment.
No blocking issues
This is a well-executed, surgical narrowing. The Set-based allow-list eliminates the false-positive class precisely, the block comment documents corpus provenance and gives a clear extension point, and the new valid test block codifies the regression boundary. The implicit case-sensitivity change (no longer matching Core, CORE) is intentional and safe — no real @actions/core binding uses mixed/uppercase.
Sub-agent and independent analysis both found no actionable issues.
🔎 Code quality review by PR Code Quality Reviewer · 103.5 AIC · ⌖ 6.29 AIC · ⊞ 5.4K
Comment /review to run again
isCoreLikeIdentifierused/^core/i, which over-matches any identifier starting withcore—coreCache,coreData,coreference, etc. — none of which are@actions/corebindings.Changes
require-await-core-summary-write.ts: Replace the broad regex with an exactSet-based allow-list containing the two verified corpus aliases:Comment documents the decision and instructs maintainers where to add new aliases.
rootsSummaryJSDoc: UpdatedcoreObj.summary (any identifier alias)→coreObj.summary (known @actions/core alias — see CORE_ALIASES).require-await-core-summary-write.test.ts: Newvalidtest block assertingcoreCache,coreData, andcoreferenceare not flagged, codifying the tightened boundary against future regressions.