Add SARIF-based lint regression ratchet - #287
Draft
gtbuchanan wants to merge 21 commits into
Draft
Conversation
Lay the CLI groundwork for the #38 lint regression gate: - lint:eslint loads a bundled wrapper formatter that writes dist/eslint.sarif (via @microsoft/eslint-formatter-sarif) and returns compact console output; the file is a turbo task output so cached runs restore it - gtb lint:eslint:compare walks each lint cwd, runs sarif-multitool match-results-forward against dist/eslint-base.sarif, and fails only on results the baseliner classifies as new — fingerprint and content-based matching absorbs line shifts without any git-diff offset math - a pnpm override drops the formatter package''s wrongly-declared eslint v8 dependency, whose bin otherwise shadows the workspace ESLint inside packages/cli CI wiring (base-SHA lint via turbo remote cache, PR comment, label override) is follow-up work tracked in #38. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PRs now fail only on lint violations that are new relative to their merge base; pre-existing (accepted) violations never block. - lint:eslint drops --max-warnings=0 and becomes a reporter: warnings never fail it (fatals still do), so every commit produces a cacheable SARIF baseline. Local changed-file enforcement stays in the hk pre-commit eslint step and the IDE. - gtb lint:eslint:compare gains --base <ref>: it lints the merge base of that ref and HEAD in a throwaway git worktree to produce the baselines itself, so the same invocation works locally and in CI. A failing base lint is tolerated (pre-ratchet gtb wrote the SARIF log before exiting non-zero), and base commits that predate SARIF output just skip, which makes rollout graceful. - New lint-regression.yml reusable workflow wired into pr.yml: lints HEAD, compares against the merge base, posts an idempotent PR comment on failure, and honors a maintainer-applied override label (dismissed on every new push) as a run-but-neutral acceptance. - E2E coverage runs the real multitool against fixture SARIF logs. Per-test fixtures, not the file-scoped one: two compares sharing a project dir race on the matched-output write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The compare machinery never inspects anything ESLint-specific, so stop naming it that way, and stop scattering loose artifacts in dist/. - SARIF artifacts move to a dedicated dist/sarif/ tree: <tool>.sarif from reporters, base/<tool>.sarif baselines, matched/<tool>.sarif multitool output, and base.ref (the merge-base stamp, root only). The compare pairs every dist/sarif/*.sarif with its baseline by filename, so a future reporter is gated by dropping a file there. - Commands become a `gtb sarif` hub (compare/baseline), reserving colon names for turbo tasks. `sarif compare --base` skips baseline production when the base.ref stamp already records the merge base — repeat local runs drop from ~35s to multitool time, and CI restores the stamped baselines from an actions/cache keyed on the merge-base SHA. release.yml seeds that cache cross-PR from each main commit via `gtb sarif baseline` (merge base == HEAD, so the baseline is the commit's own reporter output; pure file copy). - A missing baseline is an empty baseline, not a pass: every finding in an unbaselined log is new and needs acceptance, so the bootstrap PR and newly added reporters can't slip findings in silently. Findings carrying in-source suppressions are exempt — a reasoned suppression is already the accepted mechanism — but stay in the logs for visibility. - The multitool won't create its output file's parent directory; ensure dist/sarif/matched/ exists before matching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the full-history checkout (fetch-depth: 0) with the merge-ref first-parent technique: on the PR merge commit both CI providers check out, HEAD^1 is the target branch head — which is the merge base of the merged checkout — so a fetch-depth: 2 checkout resolves it with no branch fetch and no merge-base computation. A guard fails fast if the job is not on a two-parent merge ref, where HEAD^1 would silently name the wrong commit. - `gtb sarif compare` gains --base-sha <sha> (exact commit, mutually exclusive with --base); CI passes `git rev-parse HEAD^1`. Local runs keep --base <ref> merge-base semantics, where full history is free. - Baseline production now ensures the commit exists locally, fetching it by SHA at depth 1 when absent — which also fixes local shallow clones. - Cache keys become base-tip SHAs, matching exactly what the default-branch seed job stamps, so cross-PR baseline cache hits stop depending on the branch being up to date with main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Enforce step read labels from the event payload, but a re-run replays the original payload — so a label applied after the failure could never take effect, and no automatic path re-evaluated the check either (label events don't trigger the pipeline, deliberately: that would rebuild everything on any label change). Accept flow is now: apply the label, re-run the failed job. Enforce queries the live label state, so the stale payload doesn't matter. Dismissal only runs on the first attempt of a synchronize — a re-run of the replayed push event must not strip a label applied after it — and skips fork PRs, whose read-only token can't edit labels (their labels are still honored on re-runs, which only read). The PR comment now spells out the label + re-run ceremony. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub has no per-label permission rules — anyone with triage can apply any label, and a PR author with write could self-accept their own regressions. The Enforce step now resolves the label's most recent applier from the issue events API and honors it only when their repository role meets the new `override-role` input (default `maintain`, so ordinary committers can't self-accept); unresolvable appliers or roles fail closed. Also reframe the docs and PR comment: the routine response to new findings is fixing or suppressing them in-source (suppressed findings are already gate-exempt); the label is the escape hatch for bulk introductions like a dependency bump shipping a new rule, where per-instance suppression is noise and disabling the rule would let new violations creep in while it's off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Labels survive PR closure, and pushes to a closed PR emit no synchronize events — so a reopened head may have moved under a stale label with nothing dismissing it. Treat reopened like synchronize in both the dismissal step and Enforce: the label is dropped and discounted on the first attempt, so acceptance always postdates the code it vouches for. Also record the known residual: cancelling a run before its dismissal step and re-running preserves a stale label, which needs write access and deliberate timing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency ReviewThe following issues were found:
License Issuespnpm-lock.yaml
.github/workflows/lint-regression.yml
packages/cli/package.json
OpenSSF ScorecardScorecard details
Scanned Files
|
Two packaging consequences of the new SARIF dependencies surfaced on the first PR run: - pnpm blocks the multitool platform packages' build scripts (they chmod the bundled self-contained binary), failing every job at install on Linux. Allow all three; entries are consulted only when a script exists, so the scriptless win32 one is inert. - jschardet (LGPL-2.1-or-later) rides in via the formatter's SARIF_ESLINT_EMBED feature, which the lint pipeline never enables. Every consumer of @gtbuchanan/cli inherits it, so the name-scoped exception belongs in the shared dependency-review config — same pattern as the lightningcss MPL carve-out. Retire it if sarif-sdk makes the embed deps optional. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
actionlint's shellcheck pass flags `${{ }}` inside single-quoted
script strings (SC2016) — only on Linux, since the shared preset
disables shellcheck under actionlint on Windows, which is why local
pre-commit missed it. Env-var passing is the safer pattern anyway:
the label value never touches shell parsing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov flagged the real I/O layer as untested: the unit suites inject stubs and the e2e suites collect no source coverage by convention. Export the default deps and test each closure against a temp sandbox (list filtering, recursive removal, mkdir-before-write, multitool resolution, single-package workspace fallback), and test the formatter's SARIF write end to end. The formatter now resolves its output under the ESLint-provided context cwd instead of the process cwd — what it should have done anyway, and what makes the write testable without cwd games. The citty command wrappers stay uncovered like every other command file: they only delegate to the tested executors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The changeset recapped the full architecture; the deep detail lives in AGENTS.md and the README. Keep only what a consumer reading the CHANGELOG needs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With both modes in lint-regression.yml behind a seed input, every PR run showed a permanently skipped Lint Baseline Seed job and every release run a skipped Lint Regression job. A dedicated lint-baseline reusable removes the skipped peers; release.yml calls it directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The PR-comment report could go stale after a green re-run (nothing edited it) and gh's --edit-last targets the bot's last comment, which another workflow (dependency review) may own. A job summary is pinned to its run, so neither failure mode exists, and it needs no write token, so fork PRs get the full report too. Also fail the merge-ref guard with an explicit ::error:: instead of git's bare "Needed a single revision", and remove the seed job now that lint-baseline.yml owns seeding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The duplicated 'internal' literal covers ESLint's null ruleId (fatal parse/config errors); hoist it into a shared, documented module. Also record why formatConsole is hand-rolled (ESLint v9 exposes no importable formatter, and the line shape matches the compare report) and that the baseline worktree bootstrap assumes a node-only lint graph (mise never activates outside the repo). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ratchet documentation lived only in AGENTS.md while the README -- the consumer-facing surface -- never mentioned the new workflows. README gains the Lint jobs in both pipeline examples, the reusables table rows, and a lint regression section (ratchet semantics, override label flow, inputs, permissions). AGENTS.md keeps a condensed section scoped to the invariants agents need and points to the README for usage; its workflow bullets now reflect the job-summary report and the lint-baseline split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only pnpm-lock.yaml conflicted; regenerated via pnpm install (which merges the lockfile from the combined manifests). Everything else auto-merged; build and gtb verify pass on the result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The lint-regression and lint-baseline reusables ran turbo without the remote-cache shim every other CI job gets from the turbo-run composite, so the dedup their comments promised against the CI workflow's lint run never happened. Route the lint through turbo-run, then pnpm-tasks for the gtb steps (mirroring cd.yml). This also leaves lint-baseline with a single gtb-prefix ternary. Also compute the override label's freshness once in its own step: the dismissal `if:` and the Enforce bash previously encoded the same predicate twice (once inverted) and could silently drift apart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cleanups from a reuse/simplification/efficiency/altitude review of the feature branch: - Split the pure SARIF log parsing/extraction layer into lib/sarif-log.ts, leaving sarif-compare.ts as orchestration only. - Share the finding line shape via lib/finding-line.ts so the ESLint formatter's console output and the compare's new-findings report stay identical structurally, not by comment. - Derive sarif-paths entries from one POSIX-form constant and expose sarifLogPath() so turbo-config stops hardcoding the eslint SARIF output glob (generated turbo.json is unchanged). - Move planTurboInvocation into lib/turbo-invocation.ts and route produceBaseline's base-worktree lint through it, keeping the Android (Termux) escape hatch working for gtb sarif compare --base. - Resolve the head workspace once per command instead of repeating the findUp/YAML/glob walk, and run the per-file multitool matches concurrently (independent outputs; report order stays sorted). - Fold the copy-pasted extract pipelines into one extractFindings, drop the dead sarifPaths re-export, un-export hasCurrentBaseline, and dedupe the lint-cwd set into lintDirs(). - Reuse captureLogger() and createTempDir() from the test helpers instead of hand-rolled equivalents. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ESLint accepts a single --format, which forced the SARIF write into a side-effecting formatter and a hand-rolled compact console report. Running the programmatic API instead lets one lint run feed both the SARIF log (with rulesMeta) and the console output, with explicit reporter-not-gate exit semantics: the SARIF log is written before the error exit. The new finding-report renderer produces the ESLint stylish layout and is shared by lint output and the compare's new-findings report, so lint output and the CI gate read identically. Both color via util.styleText (TTY/NO_COLOR/FORCE_COLOR aware); the lint-regression job summary strips ANSI since its code fence renders escapes literally. eslint becomes an optional peer of @gtbuchanan/cli, resolved to the consumer's copy; hk-only adopters are unaffected. lint:eslint now accepts only its supported surface (patterns, --fix, --ignore-pattern) and errors on anything else instead of passing it through. The ./eslint-sarif-formatter subpath export is gone.
A one-file config change that introduces violations only in files the commit never touches — the exact gap changed-file pre-commit linting cannot see. The Lint Regression gate should fail this push and report both findings in its job summary. To be reverted after the demo.
This reverts commit 31e1819.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #38.
Implements the lint regression gate with SARIF baselining (the standard +
sarif-multitoolthe issue's follow-up comment pointed at) instead of a hand-rolled cross-ref diff: fingerprint/content-based matching absorbs line shifts natively, so there's no git-diff offset math anywhere.Design
Lint enforcement becomes a ratchet — PRs may not introduce new findings, while pre-existing (accepted) ones never block:
lint:eslintdrops--max-warnings=0and writesdist/sarif/eslint.sarifvia a bundled formatter (compact console output preserved). Warnings never fail it; fatals still do. The layout is tool-agnostic: any reporter that drops a<tool>.sarifintodist/sarif/is gated with no extra wiring.gtb sarif compareis the gate. Pairs everydist/sarif/*.sarifwithdist/sarif/base/<name>.sarifand classifies viasarif-multitool match-results-forward, failing only onnewresults. A missing baseline counts as an empty baseline (all findings new — a new reporter or the bootstrap PR can't slip findings in silently), and findings with reasoned in-source suppressions are exempt (the suppression is already the accepted mechanism).--base <ref>(local: merge-base semantics) or--base-sha <sha>(CI: the PR merge ref's first parent, which is the merge base of the merged checkout — no history fetch needed,fetch-depth: 2suffices) lints the baseline commit in a throwaway git worktree. A stamp (dist/sarif/base.ref) skips production when baselines are current; missing commits are fetched by SHA at depth 1, so shallow clones work.actions/cachekeyed on the baseline SHA;release.ymlseeds the cache cross-PR from every main commit viagtb sarif baseline(merge base == HEAD there, so it's a pure file copy of the commit's own reporter output).gtb sarif compare --base origin/maingives the full ratchet verdict locally (~seconds when the stamp is warm).Acceptance model
Routine response to new findings: fix them, or suppress in-source with a reason (gate-exempt, stays visible in the logs). The
accepted-lint-regressionlabel is the escape hatch for bulk introductions (e.g. a dependency bump shipping a new rule, where per-instance suppression is noise and disabling the rule would let new violations creep in while off): apply the label, re-run the failed job. Labels are read live (re-runs replay stale payloads), dismissed on every push and on reopen, and honored only when applied by someone with theoverride-role(defaultmaintain) — GitHub has no per-label permission rules, so the workflow implements its own.Rollout (post-merge)
accepted-lint-regressionlabelLint Regression(PR) to required checks;Lint Baseline Seedruns on main pushesFollow-ups (non-blocking)
gtb-build-pipelineskill + README coverage of thesarifcommand family--base/--base-shaworktree flows (unit-covered today)🤖 Generated with Claude Code