[TheGlitching]

What & Why

Found sqlfluff lint as a missed opportunity using rtk discover on a
production dbt project — commands unhandled. This adds rtk sqlfluff lint
following the same pattern as rtk ruff.

How it works

• Injects --format json automatically so output is structured
• Groups violations by rule (top 10) and by file with per-file breakdown
• Shows fixable count and suggests sqlfluff fix when relevant
• fix, format, version subcommands pass through unchanged
• Preserves exit codes for CI/CD
• dbt-aware path compaction: keeps models/, macros/, seeds/ prefixes

Real-world discovery

While testing, I found that sqlfluff v3+ uses start_line_no/start_line_pos
in its JSON output (not line_no/line_pos as older docs suggest). Fixed and
covered with a regression test using actual command output.

Tests

13 tests: happy path, edge cases, JSON parse fallback, real schema
regression fixture, and token savings assertion (≥60% verified).

Example output

Before (sqlfluff lint models/):
~3,000 tokens of raw text with per-line violations

After (rtk sqlfluff lint models/):
SQLFluff: 10 violations in 3 files (2 fixable)
═══════════════════════════════════════
Top rules:
RF01 (4x)
LT09 (3x)
LT12 (2x)

Top files:
models/staging/stg_customers.sql (5 violations)
RF01 (2)
LT09 (2)
LT12 (1)
...

💡 Run sqlfluff fix to auto-fix 2 violations

[FlorianBruniaux]

Hi @TheGlitching! PR #241 just landed. Could you rebase this on latest master? The sqlfluff support looks clean, we'd like to review it properly once it's current.

[TheGlitching]

Hi @TheGlitching! PR #241 just landed. Could you rebase this on latest master? The sqlfluff support looks clean, we'd like to review it properly once it's current.

@FlorianBruniaux Rebase done! ready for your review

[CLAassistant]

All committers have signed the CLA.

[pszymkowiak]

Hi! Two things needed before we can review:

1. Retarget to develop — this PR targets master, but all PRs should target develop. You can change the base branch in the PR settings (right sidebar).
2. Sign the CLA — if not already done, please sign at https://cla-assistant.io/rtk-ai/rtk

Thanks!

[aeppling]

Hey

We are cleaning up the codebase and improving the project structure for better onboarding. As part of this effort, PR #826 reorganizes src/ from a flat layout into subfolders.

No logic changes — only file moves and import path updates.

What you need to do

Rebase your branch on develop when receiving this comment:

git fetch origin && git rebase origin/develop

Git detects renames automatically. If you get import conflicts, update the paths:

use crate::git;        // now: use crate::cmds::git::git;
use crate::tracking;   // now: use crate::core::tracking;
use crate::config;     // now: use crate::core::config;
use crate::init;       // now: use crate::hooks::init;
use crate::gain;       // now: use crate::analytics::gain;

Need help rebasing? Tag @aeppling

[TheGlitching]

Hi @pszymkowiak and @aeppling ! Thanks for the guidance.

• Retargeted the PR to develop ✅
• CLA signed ✅
• Rebased on origin/develop — resolved conflicts from PR feat(refacto-codebase-onboarding): partie 1 - folders and technical docs #826's restructure:
  • Moved sqlfluff_cmd.rs from src/ to src/cmds/python/
  • Updated imports (crate::tracking → crate::core::tracking, crate::utils → crate::core::utils)
  • Registered module in src/cmds/python/mod.rs
  • Resolved README.md conflicts (kept develop's structure, added sqlfluff entries)
• All 1136 tests pass, build is clean

Ready for review!

[pszymkowiak]

@TheGlitching — picked this back up to bench it. Token-savings claim
holds and overshoots on real projects:

Sample	Raw	rtk	Savings	Exit parity
jaffle_shop (10 violations)	1058 B	449 B	57.6%	✅
dbt-labs/dbt-utils (800 violations, 108 files)	82645 B	1317 B	98.4%	✅

Output is clean: group-by-rule + by-file, dbt path compaction
(macros/sql/union.sql), fixable count + sqlfluff fix hint.

Three blockers before merge — all from develop evolving since
2026-03-31:

1. Rebase needed. Conflicts in README.md (Supported Agents
   section was collapsed into a single link, Commands Rewritten table
   removed), src/cmds/python/mod.rs (now uses automod::dir!), and
   src/main.rs (use line + match arm style).

2. run must return Result<i32>, not Result<()>. PR refacto(truncations): Set global CAPS for truncation #1998
   standardized exit-code propagation — main.rs does it now, so
   commands return the code via Result instead of calling
   std::process::exit() mid-function. Compile error today:

   error[E0308]: `match` arms have incompatible types
     --> src/main.rs:2167:40
   2167 | Commands::Sqlfluff { args } => sqlfluff_cmd::run(&args, cli.verbose)?,
        |                                expected `i32`, found `()`
   

   Minimal fix: change signature + replace the trailing
   if !output.status.success() { exit(...) } Ok(()) with
   Ok(output.status.code().unwrap_or(0)).

   Cleaner option (recommended): port to crate::core::runner::run_filtered(...)
   — the new helper that handles exit propagation, tee, tracking, and
   stdout/stderr unification in one call. See src/cmds/python/ruff_cmd.rs:76-91
   for the canonical pattern, very close to your sqlfluff use case.

3. One clippy lint (-D warnings is enforced by CI now):

   error: consider using `sort_by_key`
      --> src/cmds/python/sqlfluff_cmd.rs:147:5
   147 | file_counts.sort_by(|a, b| b.1.cmp(&a.1));
       | -> file_counts.sort_by_key(|f| std::cmp::Reverse(f.1));
   

With those three, cargo fmt && cargo clippy --all-targets -- -D warnings && cargo test --all is green (verified locally — 1995 tests pass incl.
your 13). CI re-runs and this can land.

Thanks for the patience — solid work, just got buried. Ping when ready,
happy to re-bench.

[TheGlitching]

Thank you for the detailed review, @pszymkowiak. All three items have been addressed:

1. Rebased on develop — resolved conflicts in README.md, src/cmds/python/mod.rs (now automod::dir!), and src/main.rs. Added Sqlfluff to is_operational_command().
2. run now returns Result<i32> — ported to crate::core::runner::run_filtered(...) per the ruff_cmd.rs pattern, delegating exit propagation, tee, and tracking to the shared helper.
3. Clippy lint — replaced with sort_by_key(|f| std::cmp::Reverse(f.1)).

cargo fmt && cargo clippy --all-targets -- -D warnings && cargo test --all passes locally (1995 tests). History is linear; the diff against develop is 3 files (+447/−1).

Ready for re-review.

[TheGlitching]

Conflict resolved, kept mvn_cmd from develop and sqlfluff_cmd from this branch. Ready for review @pszymkowiak

[KuSh]

Reviewed via /code-review high. Overall the filter logic and the 13-test suite (including the real-schema regression for start_line_no) are solid, but there are a few functional bugs and one integration gap that undercut the PR's own token-savings claim, plus some registrations in other files that this PR doesn't touch and so can't be inline-commented on directly — flagging them here instead:

• src/discover/rules.rs (no matching entry yet — see the ruff RtkRule around line 458 for the pattern to follow): no RtkRule was added for sqlfluff, so the Claude Code hook never auto-rewrites raw sqlfluff lint ... to rtk sqlfluff lint .... Without this, the new filter and its ~75% savings are never applied transparently — unlike ruff/pytest/mypy/pip, which all have matching RULES entries. This also is step 3 of the checklist in src/cmds/README.md#adding-a-new-command-filter. This one seems important to fix since it's the mechanism that makes the filter actually get used.
• src/core/tracking.rs:1223: categorize_command's match arm ("pytest" | "ruff" | "mypy" | "pip" => "python") doesn't include "sqlfluff", so it falls into the _ => "other" bucket and rtk gain's ecosystem breakdown undercounts Python tooling for anyone using this command.
• src/cmds/system/pipe_cmd.rs (around line 27, next to ruff-check/ruff-format): no sqlfluff-lint entry in resolve_filter, so ... | rtk pipe sqlfluff-lint (e.g. piping saved CI logs through the filter) isn't supported, unlike the equivalent ruff/pytest/mypy workflows.
• src/core/toml_filter.rs (RUST_HANDLED_COMMANDS, around line 293 next to "pip"): "sqlfluff" isn't in this list, so a user's custom .rtk/filters/*.toml filter targeting sqlfluff would silently never fire, with no shadow-warning printed (unlike every other Rust-handled command).

Requesting changes for the rules.rs hook registration (defeats the point of the filter if it never fires transparently) and the functional bugs called out inline: dialect/dialects typo, --format json disabling the filter, uncapped passthrough output, the production .expect(), the missing line/column data that forces a raw re-run to actually act on any violation, and the bareword-path heuristic — see the line-56 comment for a contained fix that doesn't require the shared tokenizer work this would otherwise need.

[pszymkowiak]

Hey @TheGlitching, circling back on this one. The filter itself has proven its worth (the benchmarks earlier in this thread still hold), so it would be a shame to let it stall now.

Two things are standing between this PR and a merge:

1. @KuSh left a detailed review on August 29 that hasn't been addressed yet. The most important item is the missing RtkRule in src/discover/rules.rs — without it the hook never rewrites raw sqlfluff lint calls, so the filter never fires transparently. The inline comments (dialect/dialects typo, --format json disabling the filter, the production .expect(), line/column data being dropped) are all concrete and come with suggested fixes.

2. The branch has picked up a conflict in src/main.rs after recent changes on develop. A rebase on origin/develop should be quick since your last merge was already close.

You've been responsive throughout this PR's life, so I'm confident this can land once those are sorted. Happy to re-bench after the update.

[TheGlitching]

Hi @KuSh and @pszymkowiak,

Thank you both for the detailed reviews and guidance! All items have been addressed:

1. Hook and Integration Registrations

• src/discover/rules.rs: Added RtkRule for sqlfluff lint under Python (75% savings target). Added test_classify_sqlfluff_lint and test_rewrite_sqlfluff_lint in src/discover/registry.rs.
• src/core/tracking.rs: Mapped "sqlfluff" to the "python" ecosystem in categorize_command().
• src/cmds/system/pipe_cmd.rs: Added "sqlfluff-lint" to resolve_filter, updated help text, and added unit test test_resolve_filter_sqlfluff_lint.
• src/core/toml_filter.rs: Added "sqlfluff" to RUST_HANDLED_COMMANDS.
• src/main.rs: Added "sqlfluff" to the PASSTHROUGH list in test_every_subcommand_is_classified.

2. Bug Fixes in sqlfluff_cmd.rs

• Location data surfaced: SqlfluffViolation now captures start_line_no with #[serde(alias = "line_no")] to support both v3+ and legacy outputs. Top rules now display sample locations (e.g. LT09 (5x, first at models/staging/stg_customers.sql:1)), and per-file rule lines show line numbers without repeating the file path (LT09 (1, line 1)).
• Subcommand routing: Dropped the exclude list. Only explicit lint (or empty args) routes to the filter; all other subcommands (dialects, parse, rules, etc.) and unknown barewords pass through untouched so sqlfluff handles them directly.
• --format json handling: Matches ruff_cmd.rs pattern; handles both --format and -f flags (including -f json and --format=json), keeping filtering enabled when JSON is explicitly requested while bailing on other formats.
• Path guessing removed: Dropped the bareword path heuristic; no artificial . is appended, preventing flags like --dialect postgres from misbehaving.
• Passthrough cap: Passthrough output is now bounded by config::limits().passthrough_max_chars.
• No production .expect(): The filter now sorts files once with sort_by_key and iterates directly, eliminating the redundant lookup and panic surface.
• Shared constants: Bound truncation and listing limits to CAP_WARNINGS and named local constants. Added .tee("sqlfluff") to RunOptions per the checklist.

3. Verification

• Rebased on latest origin/develop.
• cargo fmt --all -- --check clean.
• cargo clippy --all-targets -- -D warnings clean (0 warnings).
• All 17 sqlfluff and discover tests pass; token reduction remains ≥60%.

Ready for re-review!

[KuSh]

Re-reviewed the latest push (0455d7c) via /code-review high. The previous round's 7 findings are all fixed (verified independently and resolved on the earlier review) — thanks for the quick turn.

This pass surfaced 7 more findings, all inline. One more that can't be inline-commented since this PR doesn't touch that file:

• src/cmds/js/lint_cmd.rs (around lines 56, 118, 198, next to the ruff/mypy/pylint/flake8 entries): the generic rtk lint <linter> dispatcher's Python-linter allowlist and its format-flag/filter-dispatch match arms weren't extended for sqlfluff. rtk lint sqlfluff models/ silently falls through to the generic text-scraping filter instead of using the new structured filter_sqlfluff_lint_json — so the compact grouped report this PR adds is unreachable through that entry point, unlike the analogous ruff/mypy paths. Requesting this be wired up alongside the direct rtk sqlfluff entry point, for parity with the other Python linters.

Requesting changes for the 3 correctness bugs (JSON-parse-failure fallback pattern violation, the "first at" location bug, and non-deterministic tie ordering) plus the lint_cmd.rs gap above. The remaining inline comments are non-blocking — documentation/deferral for the joined-short-flag gap (tied to #3681), and quality/duplication notes.

[TheGlitching]

Thanks for the detailed re-review, @KuSh - and @pszymkowiak for the nudge. All findings from this round are addressed in 70e778e, rebased onto latest origin/develop.

Blocking

• Fallback pattern - filter_sqlfluff_lint_json now returns the raw sqlfluff output verbatim on JSON parse failure and warns on stderr (rtk: filter warning: ...), per the mandated pattern in rust-patterns.md. No more truncated hybrid. I noticed ruff_cmd.rs carries the same pre-existing deviation and deliberately did not replicate it into new code; that shared path deserves its own follow-up.
• "first at" location - rule first-locations are now computed from the raw sqlfluff JSON order (pre-sort) instead of the worst-first file list, so the sample points at the true earliest occurrence in the original output.
• Deterministic ordering - all three report sorts (rules, per-file rules, files) now break ties alphabetically, so identical input always yields an identical report.

Integration gap

• rtk lint sqlfluff now routes through the same structured filter_sqlfluff_lint_json, with subcommand routing and format-flag handling mirroring sqlfluff_cmd::run (non-lint subcommands pass through untouched; a user-chosen non-JSON format keeps truncated raw output).

Non-blocking

• Documented the joined short-flag gap (-fhuman) as a known limitation, tracked via the arg-tokenizer migration (feat(core): add arg_tokenizer and migrate git/grep/dotnet/golangci arg parsing onto it #3681).
• The compact_path and grouping-helper suggestions cover duplication already present across the six linter modules; a shared helper is a worthwhile follow-up but touches files outside this PR's scope, so I've kept it out here.

Verification - cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test --all are all clean locally (3073 tests, 8 new regression tests). CI is queued and awaiting the usual fork-PR approval.

Ready for a re-bench whenever you have time.

[KuSh]

Re-reviewed at 70e778e3 via /code-review high, in a dedicated worktree.

Round 2 is fully cleared — verified independently and all 7 threads resolved:

• Parse-failure path now eprintln!s and returns raw output unchanged, per the non-negotiable fallback rule.
• by_rule now iterates the pre-sort parsed vec, so "first at" is a genuine first occurrence.
• All three sorts carry deterministic tiebreakers.
• lint_cmd.rs wires sqlfluff through is_python_linter, format detection, JSON injection and dispatch.
• The joined-short-flag gap is documented in place with a #3681 pointer, as agreed.

Quality gate in the worktree is clean: cargo fmt --check, cargo clippy --all-targets (zero warnings), cargo test --all all pass, and the new regression tests land directly on the round-2 findings. Merged against current upstream/develop it's conflict-free and still fully green. Nice turnaround.

This pass found 7 more, all inline. The two I'd most like addressed are the pair where a failed sqlfluff run is reported as success: through rtk lint sqlfluff it prints SQLFluff: No issues found and exits 2, and through rtk sqlfluff it prints a single blank line and exits 2. Both were reproduced with a stub sqlfluff on PATH. For a proxy the caller doesn't know is in the loop, silently converting a failure into a clean-lint message is the most costly thing a filter can do.

The third blocking one is about the report keeping enough detail to act on — see that comment for the note that this filter is budgeting against a stale 60% savings floor when CONTRIBUTING.md's authoritative floor is 20%, which leaves plenty of room for a ruff-style per-violation section.

The remaining four are non-blocking: two missing truncation markers, one latent serde alias issue, and a documentation note about bare-path handling.

Also checked and clean, for the record: module registration and every sibling registration point, no OOB on the effective_args[1] accesses, arg construction across 11 invocation shapes (no double lint, no double --format, and -f json / --format=json both detected), and exit-code propagation.

[TheGlitching]

All 8 round-3 review items are addressed in 0db62bd. Summary:

Blocking:

• F1: lint_cmd.rs — "No issues found" now gated on result.success(); failure surfaces stderr
• F2: sqlfluff_cmd.rs — .early_exit_on_failure() so non-zero exit prints raw stdout+stderr
• F3: sqlfluff_cmd.rs — added description field + Violations: section (50-line cap, path:line:col CODE description)

Non-blocking:

• F4: ... +N more rules overflow marker after top-rules list
• F5: ... +N more rules overflow marker in per-file breakdown
• F6: dual-field start_line_no/line_no + start_line_pos/line_pos deserialization (independent Option<u64> fields with .or() helpers)
• F7: README bare-path example added
• F8: pointer-only (lockstep comment already in lint_cmd.rs)

Verification: cargo fmt / clippy / test all pass. Test fixture updated to include realistic sqlfluff fields (end_line_no, end_line_pos, fixable) for a faithful savings measurement.

[KuSh]

Round 4, reviewed at 0db62bd5 in a dedicated worktree, verified against real sqlfluff 4.3.0 in Docker (a second pass independently confirmed on 3.2.5 and 2.3.5). cargo fmt --check and cargo clippy --all-targets are both clean — none of what follows is visible to CI, which is part of the problem.

Round 3 is fully addressed in intent: the Violations: section, both ... +N more markers, the split line_no/start_line_no fields, the README note and the savings bound are all in. Piped directly (rtk pipe -f sqlfluff-lint) the filter now produces 83.2% savings with full path:line:col CODE description detail on real output. The filter itself is in good shape.

But this push introduced two regressions that invert the tool's purpose. Both come from treating a non-zero exit as failure, when sqlfluff — like every linter — exits 1 to mean violations found:

• rtk sqlfluff lint now emits 13,621 bytes where the previous head emitted 966, and where plain sqlfluff lint emits 1,724. That is 7.9x more output than not using rtk at all, and rtk gain scores it 0.0%.
• rtk lint sqlfluff prints SQLFluff: failed (exit 1) and discards every violation.

A correction I owe you: both regressions are downstream of a mistake in my round-3 review. I claimed sqlfluff writes fatal errors to stderr with an empty stdout; I had reproduced that against a stub rather than the real tool. Real sqlfluff writes Error: Unknown dialect 'NOPE' to stdout with stderr empty. The previous head already surfaced that correctly through the round-2 raw passthrough, so the two findings I filed were largely artifacts — and the fix they prompted is what broke the normal path. My apologies for the detour.

The third blocking item is a truncation-ordering bug: the new Violations: section is built from sqlfluff's emission order rather than the ranked order used everywhere above it, so it drops exactly the worst file. On a 61-file run the file ranked #1 under Top files: contributed zero of the 50 lines shown.

The pattern worth naming: that ordering bug is the third instance of one confusion — two vecs holding the same data in different orders, with the wrong one picked. Round 2 took "first at" from the sorted vec when it needed original order; this is the mirror image. And the exit-code logic now exists in two copies that this push edited differently, which is why the same command behaves in two incompatible ways depending on entry point. The comment on lint_cmd.rs:117 proposes extracting sqlfluff_cmd::render(stdout, exit_code) so both fixes land once and become unit-testable without a process.

One test would close the class fleet-wide. The suite has 18 tests, all calling filter_sqlfluff_lint_json directly and none exercising run(), which is why a wiring regression sails through. The integration harness already exists (env!("CARGO_BIN_EXE_rtk"), used by 10 tests). One invariant covers every filter: for a producer exiting 1 with parseable output, rtk's stdout must be smaller than the producer's.

Requesting changes on the three blocking items; the rest are notes, including two pre-existing ones (path collapsing, 2.x fixes) that this push makes more visible rather than causes.

[KuSh]

Round 5 (landing) — APPROVED

Claim: add rtk sqlfluff lint with a JSON filter that condenses sqlfluff's output.
Scope: accept.

Ran: real sqlfluff 3.2.5 in Docker — wrapper fidelity checked first (exit 1 with violations; exit 2 with the fatal on stdout, stderr empty) — across a 4-file and a 51-file corpus, both entry points, 0db62bd5 against ebf4cbf5.

All three round-4 blockers are fixed

blocker	0db62bd5	ebf4cbf5
sqlfluff_cmd.rs:112 — filter disabled on the normal path	18,561 B of raw JSON	2,637 B (plain sqlfluff lint: 2,626)
lint_cmd.rs:254 — violations discarded	83 B, SQLFluff: failed (exit 1)	2,633 B, full detail
sqlfluff_cmd.rs:264 — worst file hidden	0 of zzz_worst.sql's violations in the section	leads the section, all 18 present

rtk gain went from 0.0% to 96.2% on the 51-file corpus. Savings against the baseline rtk tracks: 85.8% on 4 files, 96.2% on 51, against the 20% floor in CONTRIBUTING.md.

Sharing one decision between the two entry points was the right shape, and it is what made the exit-code semantics unit-testable instead of dependent on the runner. Err(_) if exit_code != 0 rather than an exit-code taxonomy is the part that will still be correct after sqlfluff renumbers something.

Checked and correct — no need to re-verify

Overflow markers (+41 more files, … +68 more); description present in the detail lines; zero fabricated :0:0 coordinates; compact_path distinguishes migrations/orders.sql from reports/orders.sql; fixes: Option<Vec<_>> gated by fixes_reported; four independent line_no / start_line_no / line_pos / start_line_pos fields. cargo fmt --check and cargo clippy --all-targets clean, 3,246 tests on the branch; merged onto the current develop tip (22 commits ahead of this branch's base) it is a clean merge, clean gates, 3,278 tests.

One thing I checked and dropped

I flagged that the two entry points were not byte-identical as you claimed. They differ by 4 bytes, and it is only the tee log filename — …_sqlfluff.log against …_lint.log — which has to name the invoking command. Your claim was right; ignore that.

Your question

The second reads better to me, since Top rules currently gives a code with no indication of what it means. But it reshapes something you settled in ruff_cmd, so I have left it alone here. Say the word and I will do either in the next push.

Leaving it alone was the right call, and I do not want either change in this PR. Two reasons. The Top rules reshape changes something shared with ruff_cmd, and a cross-filter design decision should not be settled inside a sqlfluff PR — it should be decided for both filters at once. And this PR is five rounds in on a three-round budget; a reshape buys another round for something that is not blocking merge, when against the baseline rtk actually measures you are at 85.8–96.2%.

I have filed it as #3920 with both of your measurements and the ruff_cmd coupling written down, so the decision gets made once rather than drifting between filters. Your numbers are the substance of that issue — flagging it rather than settling it yourself was the right instinct.

Next

Nothing. Approving. All 17 open threads are resolved in this round: 14 fixed and verified by mechanism, 2 superseded by the plan / render refactor, and 1 no longer relevant now that early_exit_on_failure is gone.

Thanks for the patience across five rounds — and for the round-4 measurements, which found the regression's shape faster than I would have.