Establish ESLint Linter Factory for actions/setup/js and add dedicated ESLint daily workflows#41938
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR adds an ESLint linter factory (actions/setup/js/eslint-factory/) and migrates daily linter workflow configs, but contains no *_test.go, *.test.cjs, or *.test.js files. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41938 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 introduces a dedicated TypeScript-based ESLint “factory” under actions/setup/js and retargets the existing daily linter workflow chain (miner/monster/refiner) to focus on generating, running, and refining custom ESLint rules for actions/setup/js.
Changes:
- Added a standalone ESLint plugin project (
actions/setup/js/eslint-factory) with build wiring, config, and an initial custom rule (require-json-parse-try-catch). - Migrated daily workflows to ESLint-focused roles (ESLint Miner / ESLint Monster / ESLint Refiner) and recompiled lockfiles.
- Updated docs to reflect the new workflow names/intent.
Show a summary per file
| File | Description |
|---|---|
docs/src/content/docs/agent-factory-status.mdx |
Renames workflow display names in the factory status table to ESLint-focused names. |
actions/setup/js/eslint-factory/tsconfig.json |
Adds TypeScript compiler configuration for the ESLint factory project. |
actions/setup/js/eslint-factory/src/rules/require-json-parse-try-catch.ts |
Implements the initial custom rule to flag unguarded JSON.parse(...). |
actions/setup/js/eslint-factory/src/index.ts |
Registers the rule and exports the factory plugin entrypoint. |
actions/setup/js/eslint-factory/README.md |
Documents the factory purpose and npm commands. |
actions/setup/js/eslint-factory/package.json |
Adds build + lint scripts and dev dependencies for the factory. |
actions/setup/js/eslint-factory/package-lock.json |
Locks npm dependencies for reproducible installs. |
actions/setup/js/eslint-factory/eslint.config.cjs |
Configures ESLint to load the built plugin and enable the custom rule. |
.github/workflows/sergo.md |
Repurposes the “sergo” workflow content to act as an ESLint Refiner. |
.github/workflows/sergo.lock.yml |
Recompiled lockfile reflecting the updated ESLint Refiner workflow. |
.github/workflows/linter-miner.md |
Repurposes Linter Miner into ESLint Miner focused on rule generation for actions/setup/js. |
.github/workflows/linter-miner.lock.yml |
Recompiled lockfile reflecting the updated ESLint Miner workflow. |
.github/workflows/lint-monster.md |
Repurposes LintMonster into ESLint Monster running the factory and orchestrating remediation. |
.github/workflows/lint-monster.lock.yml |
Recompiled lockfile reflecting the updated ESLint Monster workflow. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Files not reviewed (1)
- actions/setup/js/eslint-factory/package-lock.json: Generated file
- Files reviewed: 13/14 changed files
- Comments generated: 3
- Review effort level: Low
|
|
||
| find .github/skills -maxdepth 6 -name 'SKILL.md' | sort > /tmp/gh-aw/agent/skill-index.txt |
| if make golint-custom > /tmp/gh-aw/agent/golint-custom.log 2>&1; then | ||
| : > /tmp/gh-aw/agent/lint-diagnostics.txt | ||
| cd actions/setup/js/eslint-factory | ||
| npm install > /tmp/gh-aw/agent/eslint-factory.log 2>&1 |
PR Review Summary — ESLint Linter Factory (#41938)Verdict: REQUEST_CHANGES — one critical blocking bug plus two structural gaps. 🚨 Critical (must fix before merge)
ESLint exits 0 when only warnings are found. Fix: change to
|
![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 /improve-codebase-architecture, /tdd, and /zoom-out — requesting changes on one critical wiring bug and two structural gaps before this ships.
📋 Key Themes & Highlights
Key Themes
- Critical wiring bug: Rule severity
"warn"makesnpm run lint:setup-jsalways exit 0 — the monster workflow will never detect violations and will always callnoop. Change to"error"(or pass--max-warnings 0). - Missing test infrastructure: No
RuleTestertests forrequire-json-parse-try-catch, notestscript inpackage.json. The ESLint Miner is designed to autonomously add more rules — they need a validation harness to land safely. - Self-linting glob:
**/*.cjsfromactions/setup/jsincludeseslint-factory/eslint.config.cjs. Add{ ignores: ["eslint-factory/**"] }to the flat config.
Positive Highlights
- ✅ Clean plugin architecture in
src/index.ts— easy to extend with new rules - ✅
isInsideTryBlockcorrectly distinguishes try-body from catch/finally by range comparison - ✅
ESLintUtils.RuleCreatorused properly — rule URLs are auto-generated and typed - ✅ Workflow renaming is consistent end-to-end (markdown, lock files, tracker IDs, OTEL attributes)
- ✅ Scope is intentionally narrow — only
actions/setup/js, which reduces blast radius while the factory matures
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 113.2 AIC · ⌖ 14.1 AIC · ⊞ 6.6K
| "gh-aw-custom": plugin, | ||
| }, | ||
| rules: { | ||
| "gh-aw-custom/require-json-parse-try-catch": "warn", |
There was a problem hiding this comment.
[/improve-codebase-architecture] This "warn" severity is a critical wiring bug: ESLint exits with code 0 when only warnings are found, so the pre-check step's if npm run lint:setup-js condition is always true — lint-clean.flag gets set unconditionally and the monster workflow never triggers remediation.
💡 Fix
Change severity to "error" so ESLint exits 1 when violations are found:
rules: {
"gh-aw-custom/require-json-parse-try-catch": "error",
},Alternatively, keep "warn" and add --max-warnings 0 to force a non-zero exit:
"lint:setup-js": "npm run build && cd .. && ./eslint-factory/node_modules/.bin/eslint --max-warnings 0 --config eslint-factory/eslint.config.cjs '*.cjs' '**/*.cjs'"@copilot please address this.
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/actions/setup/js/eslint-factory#${name}`); | ||
|
|
||
| export const requireJsonParseTryCatchRule = createRule({ |
There was a problem hiding this comment.
[/tdd] No unit tests for this rule. The ESLint Miner workflow will autonomously add more rules to this factory — without a RuleTester suite, there's no safety net to verify each rule's correctness or catch regressions.
💡 Suggested test skeleton
Add src/rules/require-json-parse-try-catch.test.ts (using @typescript-eslint/rule-tester):
import { RuleTester } from "`@typescript-eslint/rule-tester`";
import { requireJsonParseTryCatchRule } from "./require-json-parse-try-catch";
const ruleTester = new RuleTester();
ruleTester.run("require-json-parse-try-catch", requireJsonParseTryCatchRule, {
valid: [
// guarded by try/catch
`try { const x = JSON.parse(s); } catch (e) {}`,
// helper that is itself guarded
`function safeJson(s) { try { return JSON.parse(s); } catch(e) { return null; } }`,
],
invalid: [
{
code: `const x = JSON.parse(s);`,
errors: [{ messageId: "requireTryCatch" }],
},
{
// inside catch clause — not guarded by the outer try
code: `try { doThing(); } catch (e) { JSON.parse(e.message); }`,
errors: [{ messageId: "requireTryCatch" }],
},
],
});Add a test script to package.json and wire it in CI.
@copilot please address this.
| "description": "Custom ESLint rule factory for actions/setup/js", | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", | ||
| "scripts": { |
There was a problem hiding this comment.
[/tdd] No test script defined, and no engines field. The Miner workflow will autonomously write new rules into this package — if the miner's validation step can't run npm test, rule correctness is never verified. The engines field is also important since the workflow pins node-version: '24' but the package doesn't express this constraint.
💡 Suggested additions
"engines": { "node": ">=24" },
"scripts": {
"build": "tsc -p tsconfig.json",
"clean": "rm -rf dist",
"test": "node --test src/**/*.test.ts",
"lint:setup-js": "...",
"lint:setup-js:changed": "..."
}Use Node 22/24's built-in node:test runner or add vitest as a dev dependency — either works well with @typescript-eslint/rule-tester.
@copilot please address this.
|
|
||
| module.exports = [ | ||
| { | ||
| files: ["*.cjs", "**/*.cjs"], |
There was a problem hiding this comment.
[/zoom-out] The "**/*.cjs" glob is evaluated from actions/setup/js (after cd .. in the lint script), which means it recursively includes eslint-factory/eslint.config.cjs itself as a lint target. As the rule set grows this could produce confusing self-referential diagnostics.
💡 Add a global ignore for the factory directory
// At the top of module.exports array, before the rules config:
{ ignores: ["eslint-factory/**"] },This uses ESLint v9 flat config's global ignore pattern (an object with only ignores, no files) to unconditionally exclude the factory's own files from being analyzed.
@copilot please address this.
| if make golint-custom > /tmp/gh-aw/agent/golint-custom.log 2>&1; then | ||
| : > /tmp/gh-aw/agent/lint-diagnostics.txt | ||
| cd actions/setup/js/eslint-factory | ||
| npm install > /tmp/gh-aw/agent/eslint-factory.log 2>&1 |
There was a problem hiding this comment.
[/zoom-out] npm install is used here rather than npm ci. On repeated daily runs this resolves and potentially upgrades packages within semver ranges, which can silently change rule behavior across runs. npm ci is deterministic — it installs exactly what's in package-lock.json.
💡 Switch to npm ci
npm ci --prefer-offline > /tmp/gh-aw/agent/eslint-factory.log 2>&1--prefer-offline speeds up cold starts by using the npm cache when available, without risking stale network resolves.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one step-level bug that breaks the Monster workflow in its primary use case, plus three correctness/consistency issues that should be resolved before the factory is live.
Blocking issues
Pre-check step breaks on lint failure
lint-monster.md line 53: find .github/skills runs from actions/setup/js/eslint-factory (after cd on line 37 is never undone). That path does not exist from there. find exits 1; with set -euo pipefail the step fails and skill-index.txt is never written. The ESLint Monster workflow will error every time lint actually has issues — which is exactly its raison d'être. Fix: cd "${GITHUB_WORKSPACE}" before the find.
Tool allowlist vs. prompt contract mismatch
lint-monster.md lines 25–28: the bash allowlist lets the agent cat three files. The agent prompt says to check /tmp/gh-aw/agent/lint-clean.flag first (step 1 of required flow), but no allowed command can test the flag's existence. Either expose the flag to the allowlist or remove it from the prompt.
Lint script scope vs. workflow description
package.json line 11: lint:setup-js only passes *.cjs globs. The ESLint Miner mission explicitly names .js and .ts targets; any rule written for those types will go unenforced by the Monster and Refiner.
eslint.config.cjs test-file config
Line 22: sourceType: "module" on a block matching **/*.test.cjs is a type contradiction — .cjs is always CommonJS.
Non-blocking observation
require-json-parse-try-catch.ts line 46: JSON["parse"](data) escapes the rule because the property guard only accepts Identifier nodes, not Literal nodes. Worth fixing for completeness.
🔎 Code quality review by PR Code Quality Reviewer · 120.9 AIC · ⌖ 7.01 AIC · ⊞ 5.2K
| grep -E '^[[:space:]]*[^[:space:]].*$' /tmp/gh-aw/agent/eslint-factory.log | head -n 80 > /tmp/gh-aw/agent/eslint-diagnostics.txt || true | ||
| fi | ||
|
|
||
| find .github/skills -maxdepth 6 -name 'SKILL.md' | sort > /tmp/gh-aw/agent/skill-index.txt |
There was a problem hiding this comment.
Step fails in the failure path: find .github/skills runs with CWD actions/setup/js/eslint-factory (after the cd on line 37), so .github/skills does not exist there — find exits 1, set -euo pipefail aborts the step, and skill-index.txt is never written. The workflow breaks precisely when lint is failing and the agent has real work to do.
💡 Suggested fix
Return to the workspace root before the find:
cd "${GITHUB_WORKSPACE}"
find .github/skills -maxdepth 6 -name 'SKILL.md' | sort > /tmp/gh-aw/agent/skill-index.txtThe cd actions/setup/js/eslint-factory on line 37 is never undone; there is no cd - or cd "${GITHUB_WORKSPACE}" before this line.
| - "cat /tmp/gh-aw/agent/lint-diagnostics.txt" | ||
| - "cat /tmp/gh-aw/agent/eslint-factory.log" | ||
| - "cat /tmp/gh-aw/agent/eslint-diagnostics.txt" | ||
| - "cat /tmp/gh-aw/agent/skill-index.txt" |
There was a problem hiding this comment.
lint-clean.flag is unreachable by the agent: the agent prompt (line 105) says to check for /tmp/gh-aw/agent/lint-clean.flag as step 1, but the bash tool allowlist here only permits cat on three specific paths — none of which is lint-clean.flag. The agent cannot test the file's existence and must fall back to inferring cleanness from an empty eslint-diagnostics.txt.
💡 Suggested fix
Either add the flag file to the bash allowlist:
bash:
- "cat /tmp/gh-aw/agent/eslint-factory.log"
- "cat /tmp/gh-aw/agent/eslint-diagnostics.txt"
- "cat /tmp/gh-aw/agent/skill-index.txt"
- "test -f /tmp/gh-aw/agent/lint-clean.flag"or remove the lint-clean.flag mechanism from the prompt and rely solely on whether eslint-diagnostics.txt is empty (which already works — the step writes an empty file on success).
| files: ["**/*.test.cjs", "**/*.test.js"], | ||
| languageOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module", |
There was a problem hiding this comment.
Wrong sourceType for .cjs test files: the test-file config block sets sourceType: "module" but .cjs files are CommonJS by definition — the file extension is the explicit contract for that. ESLint will parse them in ESM mode, causing bogus import/export parse errors or masking real issues depending on whether the test files use any CJS-only syntax.
💡 Suggested fix
{
files: ["**/*.test.cjs", "**/*.test.js"],
languageOptions: {
ecmaVersion: "latest",
sourceType: "commonjs", // .cjs files are always CommonJS
},
},If some .test.js files genuinely use ES module syntax, split the block into two: one for **/*.test.cjs (sourceType: "commonjs") and one for **/*.test.js (sourceType: "module").
| "scripts": { | ||
| "build": "tsc -p tsconfig.json", | ||
| "clean": "rm -rf dist", | ||
| "lint:setup-js": "npm run build && cd .. && ./eslint-factory/node_modules/.bin/eslint --config eslint-factory/eslint.config.cjs '*.cjs' '**/*.cjs'", |
There was a problem hiding this comment.
Lint script only covers .cjs but workflows claim .js/.ts coverage: the ESLint Miner description says it mines patterns in actions/setup/js/**/*.cjs, **/*.js, and **/*.ts, and the Monster workflow remediates them — but lint:setup-js passes only '*.cjs' '**/*.cjs' to ESLint, and the eslint.config.cjs files pattern matches only *.cjs. Any rule the Miner produces for .js or .ts targets will silently go unenforced.
💡 Suggested fix
Either extend the script and config to include the claimed types:
"lint:setup-js": "npm run build && cd .. && ./eslint-factory/node_modules/.bin/eslint --config eslint-factory/eslint.config.cjs '*.cjs' '**/*.cjs' '*.js' '**/*.js'"and add a matching files entry in eslint.config.cjs, or narrow the workflow descriptions and Miner mission to explicitly say .cjs only.
| return; | ||
| } | ||
|
|
||
| if (node.callee.property.type !== "Identifier") { |
There was a problem hiding this comment.
Computed-property access silently bypasses the rule: JSON["parse"](data) is a valid, semantically identical way to call JSON.parse, but the guard node.callee.property.type !== "Identifier" short-circuits and skips it. The rule's stated goal is to catch all unguarded JSON.parse calls; this is a false negative that can be accidentally written or deliberately used to suppress the warning.
💡 Suggested fix
Handle both Identifier and Literal property nodes:
const prop = node.callee.property;
const isParseName =
(prop.type === "Identifier" && prop.name === "parse") ||
(prop.type === "Literal" && prop.value === "parse");
if (!isParseName) {
return;
}Replace the two separate if checks (lines 46–52) with the above block.
|
@copilot keep the existing agentic workflows, create new ones for eslint |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot use lsp support for copilot to make the agentic workflows more efficientZ. See lsp.md |
|
Great work standing up the ESLint linter factory and cleanly migrating the miner/monster/refiner workflow chain to an ESLint-first focus! The description is thorough and the scope is nicely constrained to
If you'd like a hand adding test coverage, you can assign this prompt to your coding agent:
|
Adds `lsp.typescript` to the eslint-miner frontmatter so the Copilot engine has TypeScript language-server support when creating and editing ESLint rules in `actions/setup/js/eslint-factory`. Recompiles lock files accordingly. The `node` network entry already present in the miner satisfies the npm-install requirement for the typescript-language-server package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
This change establishes a dedicated TypeScript-based ESLint linter factory for
actions/setup/jsand adds new ESLint-focused daily workflows alongside the existing Go-based linter chain. Scope is intentionally constrained to Actions Setup JS codepaths.Factory foundation (
actions/setup/js/eslint-factory)actions/setup/js.Initial custom ESLint rule
require-json-parse-try-catchas a first factory rule to flag unguardedJSON.parse(...)usage.New ESLint-focused daily workflows
eslint-miner.md) — mines issues/discussions for recurring JS/TS patterns and implements new custom ESLint rules in the factory.eslint-monster.md) — runs the ESLint factory againstactions/setup/js, groups findings, and launches Copilot sessions to remediate them.eslint-refiner.md) — reviews and refines ESLint rules proposed by the miner.linter-miner,lint-monster, andsergoworkflows are kept unchanged.eslint-monster.mdusesnpm cifor deterministic installs and capturesREPO_ROOTbeforecdto correctly resolve.github/skillspaths.lsp.typescript) for language-aware rule authoring.Repository metadata/docs alignment