Merge pull request #94829 from Expensify/claude-onyx-connect-bypass

[No QA] Block eslint-disable bypasses of the Onyx.connect() ban

Author: cristipaval

scripts/checkOnyxConnectBypass.ts

@@ -0,0 +1,107 @@
+#!/usr/bin/env ts-node
+
+/**
+ * Fails the lint run when a new inline `eslint-disable` bypasses the Onyx.connect() ban.
+ *
+ * The ban (`rulesdir/no-onyx-connect`, shipped by eslint-config-expensify) is a normal lint rule,
+ * so an inline disable can silence it. The ESLint CLI neither surfaces nor fails on such suppressed
+ * violations, so this runner re-elevates them: it lints with only the ban enabled, reads the
+ * suppressed violations off the results, and exits non-zero on any that are not grandfathered.
+ * Because it works from ESLint's suppressed-message data, no disable directive can reach it.
+ *
+ * A real bypass requires a file to contain both an `Onyx.connect` reference and an `eslint-disable`
+ * directive, so we first narrow the targets to files matching both (via git grep) and only run
+ * ESLint on those — keeping the check fast even on a whole-repo lint. The `Onyx.connect` match
+ * deliberately omits the `(` so it stays a superset of the AST rule (e.g. whitespace or a comment
+ * before the paren); extra matches like `Onyx.connectWithoutView` are harmless, as the rule ignores them.
+ */
+import tsParser from '@typescript-eslint/parser';
+import {ESLint} from 'eslint';
+import type {Rule} from 'eslint';
+import {execFileSync} from 'node:child_process';
+import {createRequire} from 'node:module';
+import path from 'node:path';
+import {BANNED_RULE_ID, collectSuppressedBans, findNewBypasses} from './onyxConnectBypass';
+
+const projectRoot = path.resolve(__dirname, '..');
+
+/** The ban's rule name as registered under the `rulesdir` plugin (i.e. `BANNED_RULE_ID` without the prefix). */
+const RULE_NAME = 'no-onyx-connect';
+
+function isRuleModule(value: unknown): value is Rule.RuleModule {
+    return typeof value === 'object' && value !== null && 'create' in value && typeof value.create === 'function';
+}
+
+/** Dynamically import the shipped `no-onyx-connect` rule, which is ESM with relative imports. */
+async function loadNoOnyxConnectRule(): Promise<Rule.RuleModule> {
+    const require = createRequire(__filename);
+    const expensifyConfigDirectory = path.dirname(require.resolve('eslint-config-expensify/package.json'));
+    const ruleFile = path.join(expensifyConfigDirectory, 'eslint-plugin-expensify', 'no-onyx-connect.js');
+    const imported: unknown = await import(ruleFile);
+    if (isRuleModule(imported)) {
+        return imported;
+    }
+    if (typeof imported === 'object' && imported !== null && 'default' in imported && isRuleModule(imported.default)) {
+        return imported.default;
+    }
+    throw new Error(`Could not load the no-onyx-connect rule from ${ruleFile}`);
+}
+
+/** Files among the lint targets that contain both an Onyx.connect() call and an eslint-disable. */
+function findCandidateFiles(targets: string[]): string[] {
+    const pathSpecs = targets.length > 0 ? targets : ['.'];
+    try {
+        const output = execFileSync('git', ['grep', '-lI', '-F', '--all-match', '--untracked', '--no-recurse-submodules', '-e', 'Onyx.connect', '-e', 'eslint-disable', '--', ...pathSpecs], {
+            cwd: projectRoot,
+            encoding: 'utf8',
+        });
+        return output.split('\n').filter(Boolean);
+    } catch (error: unknown) {
+        // git grep exits 1 when nothing matches; anything else is a real failure.
+        if (typeof error === 'object' && error !== null && 'status' in error && error.status === 1) {
+            return [];
+        }
+        throw error;
+    }
+}
+
+async function run(): Promise<void> {
+    const candidates = findCandidateFiles(process.argv.slice(2));
+    if (candidates.length === 0) {
+        return;
+    }
+
+    const rule = await loadNoOnyxConnectRule();
+    const eslint = new ESLint({
+        cwd: projectRoot,
+        warnIgnored: false,
+        errorOnUnmatchedPattern: false,
+        overrideConfigFile: true,
+        overrideConfig: [
+            {
+                files: ['**/*.{js,jsx,ts,tsx,mjs,cjs}'],
+                languageOptions: {parser: tsParser},
+                plugins: {rulesdir: {rules: {[RULE_NAME]: rule}}},
+                rules: {[BANNED_RULE_ID]: 'error'},
+            },
+        ],
+    });
+
+    const results = await eslint.lintFiles(candidates);
+    const newBypasses = findNewBypasses(collectSuppressedBans(results, projectRoot));
+    if (newBypasses.length === 0) {
+        return;
+    }
+
+    console.error('Onyx.connect() is banned and the ban cannot be bypassed with eslint-disable. Use the useOnyx() hook to read Onyx data instead.');
+    console.error('New bypasses found:');
+    for (const bypass of newBypasses) {
+        console.error(`  ${bypass.file}:${bypass.line}`);
+    }
+    process.exitCode = 1;
+}
+
+run().catch((error: unknown) => {
+    console.error(error instanceof Error ? error.message : error);
+    process.exitCode = 1;
+});

scripts/lint.sh

@@ -58,4 +58,8 @@ ESLINT_ARGS+=(
 # Run ESLint with the repo's default memory ceiling and seatbelt behavior.
 NODE_OPTIONS="${NODE_OPTIONS:---max_old_space_size=8192}" \
 SEATBELT_FROZEN="${SEATBELT_FROZEN:-0}" \
-    exec npx eslint "${ESLINT_ARGS[@]}"
+    npx eslint "${ESLINT_ARGS[@]}"
+
+# Fail if a new inline eslint-disable bypasses the Onyx.connect() ban (rulesdir/no-onyx-connect),
+# checking the same targets as ESLint above. Reached only when ESLint itself passes (set -e).
+exec npx ts-node scripts/checkOnyxConnectBypass.ts "${PASSTHROUGH_ARGS[@]}"

scripts/onyxConnectBypass.ts

@@ -0,0 +1,72 @@
+/**
+ * Detection logic for new `eslint-disable` bypasses of the Onyx.connect() ban.
+ *
+ * `rulesdir/no-onyx-connect` (shipped by eslint-config-expensify) is a normal lint rule, so an
+ * inline `eslint-disable` can silence it. ESLint records such silenced violations as "suppressed
+ * messages". This module finds suppressed `rulesdir/no-onyx-connect` violations and flags any that
+ * go beyond the disables already present on `main`, so a new bypass can be re-elevated to an error
+ * at the runner level — where no disable directive can reach it.
+ */
+import type {ESLint} from 'eslint';
+import path from 'node:path';
+
+/** Rule id of the Onyx.connect() ban, as exposed through eslint-plugin-rulesdir. */
+const BANNED_RULE_ID = 'rulesdir/no-onyx-connect';
+
+/**
+ * Disables of the ban that already exist on `main`, keyed by repo-relative path with the number of
+ * occurrences in each file. Migrating these call sites to useOnyx() is already in progress; any
+ * suppressed violation beyond these counts is treated as a new bypass.
+ */
+const GRANDFATHERED_BYPASSES = new Map<string, number>([
+    ['src/libs/NextStepUtils.ts', 1],
+    ['src/libs/ReportNameUtils.ts', 2],
+]);
+
+/** A `no-onyx-connect` violation that an inline disable directive silenced. */
+type SuppressedBan = {
+    file: string;
+    line: number;
+};
+
+/** The fields of an ESLint result this module reads; real `ESLint.LintResult`s satisfy it. */
+type ResultWithSuppressed = Pick<ESLint.LintResult, 'filePath' | 'suppressedMessages'>;
+
+/** Pull suppressed `no-onyx-connect` violations out of ESLint results, keyed by repo-relative path. */
+function collectSuppressedBans(results: readonly ResultWithSuppressed[], projectRoot: string): SuppressedBan[] {
+    const bans: SuppressedBan[] = [];
+    for (const result of results) {
+        for (const message of result.suppressedMessages ?? []) {
+            if (message.ruleId !== BANNED_RULE_ID) {
+                continue;
+            }
+            const file = path.relative(projectRoot, result.filePath).split(path.sep).join('/');
+            bans.push({file, line: message.line});
+        }
+    }
+    return bans;
+}
+
+/** Return the suppressed bans that exceed the grandfathered allowance for their file. */
+function findNewBypasses(suppressedBans: readonly SuppressedBan[]): SuppressedBan[] {
+    const byFile = new Map<string, SuppressedBan[]>();
+    for (const ban of suppressedBans) {
+        const list = byFile.get(ban.file) ?? [];
+        list.push(ban);
+        byFile.set(ban.file, list);
+    }
+
+    const newBypasses: SuppressedBan[] = [];
+    for (const [file, bans] of byFile) {
+        const allowed = GRANDFATHERED_BYPASSES.get(file) ?? 0;
+        if (bans.length <= allowed) {
+            continue;
+        }
+        const sortedByLine = [...bans].sort((a, b) => a.line - b.line);
+        newBypasses.push(...sortedByLine.slice(allowed));
+    }
+    return newBypasses;
+}
+
+export {BANNED_RULE_ID, GRANDFATHERED_BYPASSES, collectSuppressedBans, findNewBypasses};
+export type {SuppressedBan, ResultWithSuppressed};

tests/unit/OnyxConnectBypassTest.ts

@@ -0,0 +1,56 @@
+import type {ESLint} from 'eslint';
+import {BANNED_RULE_ID, collectSuppressedBans, findNewBypasses} from '../../scripts/onyxConnectBypass';
+import type {ResultWithSuppressed} from '../../scripts/onyxConnectBypass';
+
+const PROJECT_ROOT = '/repo';
+
+function makeResult(relativePath: string, suppressedMessages: ESLint.LintResult['suppressedMessages']): ResultWithSuppressed {
+    return {filePath: `${PROJECT_ROOT}/${relativePath}`, suppressedMessages};
+}
+
+function suppressed(ruleId: string, line: number): ESLint.LintResult['suppressedMessages'][number] {
+    return {ruleId, line, column: 1, message: 'x', severity: 2, suppressions: [{kind: 'directive', justification: ''}]};
+}
+
+describe('collectSuppressedBans', () => {
+    it('keeps only suppressed no-onyx-connect violations and relativizes their paths', () => {
+        const results = [makeResult('src/libs/Foo.ts', [suppressed(BANNED_RULE_ID, 12), suppressed('no-console', 3)]), makeResult('src/libs/Bar.ts', [suppressed(BANNED_RULE_ID, 7)])];
+
+        expect(collectSuppressedBans(results, PROJECT_ROOT)).toEqual([
+            {file: 'src/libs/Foo.ts', line: 12},
+            {file: 'src/libs/Bar.ts', line: 7},
+        ]);
+    });
+
+    it('returns nothing when there are no suppressed messages', () => {
+        expect(collectSuppressedBans([makeResult('src/libs/Foo.ts', [])], PROJECT_ROOT)).toEqual([]);
+    });
+});
+
+describe('findNewBypasses', () => {
+    it('flags a bypass in a file with no grandfathered allowance', () => {
+        expect(findNewBypasses([{file: 'src/libs/CurrencyUtils.ts', line: 5}])).toEqual([{file: 'src/libs/CurrencyUtils.ts', line: 5}]);
+    });
+
+    it('allows grandfathered disables up to their recorded count', () => {
+        const bans = [
+            {file: 'src/libs/ReportNameUtils.ts', line: 192},
+            {file: 'src/libs/ReportNameUtils.ts', line: 201},
+            {file: 'src/libs/NextStepUtils.ts', line: 33},
+        ];
+        expect(findNewBypasses(bans)).toEqual([]);
+    });
+
+    it('flags only the overflow when a grandfathered file gains an extra disable', () => {
+        const bans = [
+            {file: 'src/libs/ReportNameUtils.ts', line: 192},
+            {file: 'src/libs/ReportNameUtils.ts', line: 201},
+            {file: 'src/libs/ReportNameUtils.ts', line: 300},
+        ];
+        expect(findNewBypasses(bans)).toEqual([{file: 'src/libs/ReportNameUtils.ts', line: 300}]);
+    });
+
+    it('returns nothing for an empty input', () => {
+        expect(findNewBypasses([])).toEqual([]);
+    });
+});