ci: run eslint and a format check on every pull request - #250
Conversation
e631d1f to
55d17bd
Compare
CPK-8215 Review PR #250 — eslint flat config and CI lint/format gates
PR #250 closes outpost#141. The repo has been unlinted since the ESLint 9 bump — ESLint 9 will not read
Worth knowing in reviewThe rule set is carried over unchanged, and **One deliberate exception: **
Formatting is checked against the diff327 files fail
UnblocksCPK-8042 (lint guard banning bare Verification: |
Closes #141. The repo has been unlinted since the ESLint 9 bump. ESLint 9 will not read `.eslintrc.*` without `ESLINT_USE_FLAT_CONFIG=false`, and that env var does not survive turbo's environment sanitization, so `eslint src/` failed outright in seven packages — which is why the CI job named "Lint, Typecheck & Test" ran no lint step. `eslint.config.mjs` replaces the eslintrc. The rule set is carried over unchanged. `@eslint/js` is pinned to the same major as eslint itself so that stays true: on `^10` against eslint 9 it supplies v10's recommended set, 64 rules rather than 61, adding `preserve-caught-error`, `no-useless-assignment` and `no-unassigned-vars`. That is a rule change smuggled in under a format migration, and a latent hard failure the first time that set names a rule eslint 9 does not ship. One deliberate exception: `no-explicit-any` drops to a warning in test files, where mocks of Prisma clients and SDK responses make an accurate type large, brittle, and worth nothing the assertions do not already pin. It keeps its `error` in production code. `apps/web` keeps linting through `next lint`, which supplies the `react-hooks` and `@next/next` plugins the root config does not carry. Its `.eslintrc.cjs` gains `root: true` and restates `argsIgnorePattern: '^_'` and `consistent-type-imports`: it had no `root: true`, so it inherited those from the repo-root eslintrc through the cascade, and deleting that file would have silently changed how the app lints — `_request` and `_ticket` began reporting as unused. Verified at parity: 44 warnings before and after, none of them `_`-prefixed. `apps/worker` gains a real lint script. It was `echo 'no eslint config for worker yet'`, so a production service reported green while being entirely unlinted, and the reason for the stub is what this change removes. ## Formatting is checked against the diff, not the repo 327 files fail `prettier --check` on `main`. Reformatting them in one commit would collide with all six open PRs and bury the change that mattered, so CI checks only the files a PR touches, read from the API rather than from git — `actions/checkout` runs with `persist-credentials: false` and a shallow clone, so a `git fetch` of the base commit fails with "could not read Username for 'https://github.com'". Asking the API also gives the PR's own file list rather than a two-dot diff against a moving base tip, which would fail a PR for legacy files it never touched. `.prettierignore` excludes `/templates/`, anchored. Those markdown files are loaded at runtime by `shared/src/templates/loader.ts` and sent by the email transport, and prettier reflows markdown, so formatting them changes what lands in a customer's inbox. Unanchored the pattern matched a `templates` directory at any depth and swallowed 11 tracked source files under `packages/outpost/shared/src/templates/` and `apps/web/src/app/**/templates/`. ## turbo The root config is now an input to the lint task, which the existing NOTE in turbo.json asked for once lint could actually run: editing a rule has to invalidate the cached result, or a stricter config reports the previous run's clean output. Verified rather than assumed — a warm `turbo run lint` reports 10/10 cached, and touching `eslint.config.mjs` drops it to 1/10. Verification: `turbo run lint` 10/10 with 0 errors (46 warnings, all pre-existing and now visible), typecheck 10/10, test 10/10.
Found by Jerel reviewing #241. `pull_request` was filtered to `branches: [main, staging]`, so a PR based on another PR's branch got no CI at all. `Static analysis (zizmor)` uses a bare `pull_request:` trigger because it is a required status check, so it still reported and still passed — and GitHub then read the PR as CLEAN and mergeable. #249 sat exactly that way, verified before changing anything: `gh pr checks 249` returned one check, zizmor, passing; `mergeStateStatus=CLEAN`. So 618 lines of the draft linter presented as green with a workflow linter as their only signal and no test run behind them. A check that reports success without evaluating anything is worse than no check, and it is the same shape as the defects this stack has been about. The filter is gone from `pull_request`. `push` keeps its filter. That is where the original reasoning applies: `main` and `staging` deploy, and Railway's deploy triggers wait for a check suite on the pushed commit. No other branch deploys, so no other branch needs a push-triggered suite — its pull-request run covers it. Retargeting #249 at `main` would have fixed that one PR. This fixes the next stack too, which was Jerel's preference and is the better trade for one dropped line. Verified the workflow parses and keeps all 14 steps: `on.pull_request` is now null, `on.push.branches` is unchanged.
959e3bd to
bc6480b
Compare
jerelvelarde
left a comment
There was a problem hiding this comment.
Verified by execution rather than by reading — this one is mostly claims about tooling behaviour, which is the kind that reads fine and behaves otherwise.
The trigger fix is right, and the comment is better than the fix. Dropping the filter from pull_request while keeping it on push is exactly the shape: Railway needs a push suite on the two deploying branches and nothing else does, since a PR run covers it. Recording #249 as the worked example in the comment means the next person to consider re-adding a filter has to argue with the incident rather than the style.
pnpm lint passes 10/10. Warnings only, apps/worker now genuinely linting instead of echo.
The turbo cache wiring works — with one correction to the claim. Editing the config's contents gives 1 cached, 10 total, matching your number. But touch alone gives 10 cached, 10 total, because turbo hashes contents, not mtimes — worth knowing if anyone re-verifies this and concludes the wiring is broken.
The more useful correction: the one cached task is @copilotkit/outpost:build, not a lint task. All nine lint tasks miss, @copilotkit/outpost-web included — and web's lint doesn't read eslint.config.mjs at all, since it runs next lint against its own .eslintrc.cjs. So the invalidation is over-broad by one package. Costs a redundant next lint on every root-config edit and nothing else, so I would leave it rather than add a per-package inputs override, but the turbo.json comment currently asserts the config "is an input to every package's lint task" as though that were precise, and for web it is true only accidentally.
.prettierignore anchoring — tested both directions. /templates/ with the leading slash: templates/invite.md is skipped, and packages/outpost/shared/src/templates/loader.ts is still checked (it reports style issues, i.e. it is part of the 311-file backlog rather than exempted). Unanchored it would have silently exempted 11 tracked source files. That is a detail almost nobody gets right first time.
The format-check pipeline survives a space in a path. Ran the actual tr '\n' '\0' | xargs -0 line against a file under a directory with a space; prettier names the file intact. The reasoning in the comment about xargs -d being GNU-only is also correct, and matters here because half the team is on macOS.
apps/web/.eslintrc.cjs with root: true is the best catch in the PR. Deleting the repo-root .eslintrc.cjs silently changed how the app lints, because argsIgnorePattern: '^_' and consistent-type-imports were being inherited through the cascade — so _request / _ticket would have started reporting. Restating the rules and stopping the cascade explicitly, rather than relying on a parent that no longer exists, is the right fix and would have been easy to miss until someone's editor disagreed with CI.
Approving.
Two things, neither blocking
More CI minutes, deliberately. Removing the base filter means every PR to any branch now runs the full suite with its Postgres service — renovate PRs, spike branches, stacks. That is the trade the PR is making and it is the right one, but it is a real cost increase and it is not mentioned.
One connection worth recording. The .prettierignore comment justifies the exclusion on the grounds that these files are "loaded at runtime by shared/src/templates/loader.ts and sent by the email transport". That reasoning is sound and the exclusion should stay — but per #253, templates/ never reaches the production image, so today nothing is sent from them at all. Not this PR's problem; noting it so the two facts are recorded next to each other rather than discovered separately.
The format check added in #250 caught these on its first run against a real PR — five of this PR's own files were unformatted. Prettier preserves the comments in tsconfig.build.json, and the build config still parses: a clean build emits 18 JS files with 0 test files. Worth noting the gate did exactly what it was added for, on the first opportunity, against the person who added it.
#250 added a format check over the files a PR touches, and three of these were already unformatted on main. Cosmetic only — collapsed single-item type re-exports onto one line and wrapped long template literals. Verified: 1099 tests passing, unchanged from before.
#250's format check covers the files a PR touches, and these four were already unformatted on main. Cosmetic only — re-export lists collapsed or wrapped, long calls broken across lines. Verified: 1145 tests passing in packages/outpost, unchanged.
#250 added a format check over the files a PR touches, and three of these were already unformatted on main. Cosmetic only — collapsed single-item type re-exports onto one line and wrapped long template literals. Verified: 1099 tests passing, unchanged from before.
The format check added by #250 runs against the files a PR touches, which is the right shape -- but two kinds of file in this repo get worse when prettier formats them, and both are now failing PRs that have nothing wrong with them. .claude/skills/*.md are hand-maintained spec prose carrying wide reference tables. Prettier pads every cell out to the widest one, so a one-word edit rewrites the whole table and the real change disappears into the diff. On #238 that is 305 changed lines across two SKILL.md files. docs/community-signal/reddit-pulse-seen.json is operational dedup state, appended by the weekly-report routine rather than written by hand, and 2-space by construction. The repo's tabWidth is 4, so formatting it reindents all 412 lines -- and the next run rewrites it 2-space again, so the check would break again every week. Both patterns are anchored with a leading slash, for the reason the /templates/ entry above them records: unanchored, a gitignore-style pattern matches a directory of that name at any depth. Verified against the CI step's own command over #238's three changed files: fails before, passes after. Repo-wide `prettier --check .` is unchanged at 334 pre-existing warnings, and .claude/settings.json stays covered -- the only tracked files this exempts are the 13 markdown skill definitions.
… orphaned `f25ec6f` wrapped two long signatures in PrismaLike across lines, which moved `args: any` one line further down than its `eslint-disable-next-line`. The directive then covered `create: (` instead, so #250's new Lint step reported two no-explicit-any errors and two unused-directive warnings on a file the formatting was supposed to leave alone. Directives now sit on the `args: any` line itself. Verified against a local merge with main (where eslint.config.mjs exists): `pnpm lint` 10/10, and prettier is still clean on every file this branch touches.
ESLint runs on every pull request, and formatting is checked on the files a change touches. Closes #141.
What was wrong
The repo has been unlinted since the ESLint 9 bump. ESLint 9 will not read
.eslintrc.*withoutESLINT_USE_FLAT_CONFIG=false, and that env var does not survive turbo's environment sanitization, soeslint src/failed outright in seven packages:That is why the CI job named "Lint, Typecheck & Test" ran no lint step —
ci.yml:314documented the reason and deferred the migration to its own PR. This is that PR.eslint.config.mjsreplaces the eslintrc andturbo run lintnow passes 10/10.The rule set is carried over, with two stated exceptions
This is a config format migration, not a tightening. Running ESLint for the first time surfaced 95 problems, 9 of them errors, and two groups needed a decision rather than a silent fix:
preserve-caught-errorandno-useless-assignmentare rules ESLint 9 added to its recommended set — not part of the config being migrated, and the repo has never run under a linter that enforced them. They fire on existing error handling and control flow inqueue/src/handlers/(3 occurrences, each listed in the config with its file and line). Turning a config migration into a rewrite of somebody else'scatchsemantics is the wrong trade, so they report as warnings: visible in the output, not gating the build, fixable in a pass of their own.no-explicit-anydrops to a warning in test files. All 6 errors were(args: any)on Prisma mocks inpending-response-sweep.test.ts. Tests mock external shapes where an accurate type is large, brittle, and buys nothing the assertions do not already pin. The rule keeps itserrorin production code.Happy to take either of those the other way if you would rather the errors be fixed in this PR.
apps/webis deliberately untouched. It runsnext lintagainst its own.eslintrc.cjs, which supplies thereact-hooksand@next/nextplugins this config does not carry — linting it here would reportreact-hooks/exhaustive-depsas an unknown rule wherever the app disables it inline.next lintis deprecated in Next 15 and removed in 16, so that migration wants its own change and its own verification.Formatting is checked against the diff, not the repo
327 files fail
prettier --checkonmain. Reformatting them in one commit would collide with all six open PRs and bury the change that mattered, so CI checks only the files the PR touches. That enforces the convention from here on and leaves the backlog to be cleared on its own schedule. The step ispull_request-only, since a push build has no base to diff against..prettierignoreexcludestemplates/. Those markdown files are loaded at runtime byshared/src/templates/loader.tsand sent by the email transport, and prettier reflows markdown — wrapping lines, renumbering lists, normalising emphasis. Formatting them changes what lands in a customer's inbox: a content change wearing a whitespace diff.turbo cache wiring
turbo.jsoncarried a NOTE asking for the root config to be wired as a lint input "with the lint enablement in #141, where the cache behavior is observable" — a failing task is never cached, so it could not be tested before now. Editing a rule has to invalidate the cached result, or a stricter config reports the previous run's clean output.Verified rather than assumed:
It sits on the lint task rather than in
globalDependenciesso a config change invalidates lint alone, not build, test and typecheck with it.Verification
turbo run lint10/10, 0 errors, 49 warnings now visible.turbo run typecheck10/10.turbo run test10/10.@eslint/jsadded as a devDependency for the core recommended set — it was previously only reachable as a transitive dependency of eslint itself. pnpm also dropped an empty"dependencies": {}frompackage.jsonwhile writing the lockfile.