Skip to content

fix(golangci-lint): restore run wrapper and preserve global-flag rewrites#798

Merged
FlorianBruniaux merged 3 commits intortk-ai:developfrom
mgierok:fix/golangci-lint-run-wrapper-develop
Apr 13, 2026
Merged

fix(golangci-lint): restore run wrapper and preserve global-flag rewrites#798
FlorianBruniaux merged 3 commits intortk-ai:developfrom
mgierok:fix/golangci-lint-run-wrapper-develop

Conversation

@mgierok
Copy link
Copy Markdown
Contributor

@mgierok mgierok commented Mar 24, 2026

Summary

This PR fixes a regression in the golangci-lint wrapper and restores consistency between runtime behavior, discover/rewrite, and the documented compact path.

It includes exactly these 3 commits:

  • fix(golangci-lint): restore run wrapper and align guidance
  • fix(discover): preserve golangci-lint flags in rewrite
  • fix(golangci-lint): support inline global flags before run

What changed

  • restored rtk golangci-lint as a single wrapper entrypoint with passthrough for non-run invocations
  • kept compact filtering only for rtk golangci-lint run ...
  • fixed discover / rewrite so global flags before run are preserved
  • fixed inline --flag=value handling in both runtime parsing and rewrite/discovery
  • updated guidance/docs so the compact path is consistently advertised as rtk golangci-lint run ...

Validation

Targeted checks passed:

  • rtk cargo test golangci_cmd -- --nocapture
  • rtk cargo test discover::registry -- --nocapture
  • rtk cargo run -- rewrite 'golangci-lint --color=never run ./...'

Local full gate status:

  • rtk cargo fmt --all --check passed
  • rtk cargo clippy --all-targets reported one existing warning in src/rake_cmd.rs
  • rtk cargo test in this environment still hits pre-existing tracking DB file failures unrelated to this PR

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 24, 2026

CLA assistant check
All committers have signed the CLA.

@mgierok
Copy link
Copy Markdown
Contributor Author

mgierok commented Mar 24, 2026

This PR fixes a regression in the golangci-lint wrapper path and closes the follow-up parser gaps around rewrite/discovery.

Problem

The original regression came from shifting golangci-lint toward a nested Clap subcommand flow instead of treating it as a single wrapper entrypoint.

That created two distinct problems.

1. Runtime wrapper regression

rtk golangci-lint run ... could build an incorrect child command with duplicated run, because run was effectively being interpreted both by wrapper routing and as an upstream argument.

At the same time, bare rtk golangci-lint stopped behaving like a true passthrough entrypoint and was treated too much like the compact run path.

That made the public interface inconsistent with the intended contract:

  • compact filtering should apply to rtk golangci-lint run ...
  • bare rtk golangci-lint and other upstream subcommands should remain passthrough

2. Rewrite / discovery regressions

After narrowing the compact path to golangci-lint run, the discovery layer became too strict and stopped recognizing valid invocations with global flags before run, for example:

golangci-lint -v run ./...
golangci-lint --color never run ./...

That meant hook users could silently fall back to the raw verbose command even though the runtime wrapper explicitly supports those forms.

A second bug affected the standard inline syntax:

golangci-lint --color=never run ./...
golangci-lint --config=foo.yml run ./...

Both the runtime parser and the rewrite/discovery parser normalized --flag=value down to the flag name, but still skipped the next token as if the value were separate. In practice, that jumped past run, so these commands no longer took the compact path.

Fix

This PR is intentionally split into 3 commits, each tightening one part of the behavior:

1. Restore the wrapper contract

The wrapper keeps a single public entrypoint for rtk golangci-lint and moves the behavioral split into golangci_cmd.

Only invocations whose first real upstream subcommand is run take the compact filtered path. Bare rtk golangci-lint and non-run subcommands remain passthrough.

In the filtered path, run is consumed logically only once, so the child process receives exactly one run.

2. Restore rewrite/discovery support for pre-run global flags

discover and rewrite now recognize supported global flags that appear before run and preserve them in rewritten commands.

Examples that now work correctly again:

golangci-lint -v run ./...
golangci-lint --color never run ./...

and rewrite to:

rtk golangci-lint -v run ./...
rtk golangci-lint --color never run ./...

3. Support inline --flag=value forms consistently

Both the runtime parser and the rewrite/discovery parser now distinguish between:

  • --flag value
  • --flag=value

So these forms also stay on the supported compact path:

golangci-lint --color=never run ./...
golangci-lint --config=foo.yml run ./...

and rewrite to:

rtk golangci-lint --color=never run ./...
rtk golangci-lint --config=foo.yml run ./...

Resulting behavior

After this PR, the contract is consistent again:

  • compact path:
    • rtk golangci-lint run [args...]
    • rtk golangci-lint -v run [args...]
    • rtk golangci-lint --color never run [args...]
    • rtk golangci-lint --color=never run [args...]
  • passthrough path:
    • rtk golangci-lint
    • rtk golangci-lint version
    • rtk golangci-lint linters

Tests

This PR adds or updates regression coverage for:

  • runtime dispatch for run
  • runtime dispatch with global flags before run
  • runtime dispatch with inline --flag=value
  • discover classification for golangci-lint run
  • discover classification/rewrite for -v, --color never, --color=never, and --config=...
  • env-prefixed rewrite cases
  • helper normalization for global flags before run

Targeted validation run locally:

  • rtk cargo test golangci_cmd -- --nocapture
  • rtk cargo test discover::registry -- --nocapture
  • rtk cargo run -- rewrite 'golangci-lint --color=never run ./...'

The local full gate still reports unrelated tracking DB failures in this environment, which appear to be pre-existing and not caused by these 3 commits.

@pszymkowiak pszymkowiak added bug Something isn't working effort-medium 1-2 jours, quelques fichiers filter-quality Filter produces incorrect/truncated signal labels Mar 24, 2026
@pszymkowiak
Copy link
Copy Markdown
Collaborator

[w] wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Restores the golangci-lint run wrapper so that only rtk golangci-lint run triggers compact filtering, while bare rtk golangci-lint passes through. Fixes discover/rewrite to preserve global flags (e.g. --color, --config) that appear before the run subcommand, and updates all documentation to reflect the correct compact path.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Analyzed automatically by wshm · This is an automated analysis, not a human review.

@aeppling
Copy link
Copy Markdown
Contributor

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

@mgierok mgierok force-pushed the fix/golangci-lint-run-wrapper-develop branch from c0e8ea8 to abe0158 Compare March 27, 2026 10:45
@mgierok
Copy link
Copy Markdown
Contributor Author

mgierok commented Mar 27, 2026

Rebased on develop — resolved one conflict in CLAUDE.md (kept the slim version from PR #826, dropped the inline module table that was replaced by references to ARCHITECTURE.md / CONTRIBUTING.md).

All checks pass: fmt, clippy (0 errors), 1147 tests green. No import path changes were needed — git detected the renames automatically and applied our commits to the new folder structure.

@mgierok mgierok force-pushed the fix/golangci-lint-run-wrapper-develop branch from abe0158 to 69f909c Compare March 31, 2026 07:10
mgierok added 3 commits April 12, 2026 23:14
Keep bare golangci-lint invocations as passthrough while preserving compact filtering for golangci-lint run.
Update discover/rewrite rules, regression tests, and docs to advertise only the supported compact run path.
Normalize golangci-lint global flags before run during classification and keep them in rewritten commands.
Add regression coverage for classify_command and rewrite_command with pre-run global flags.
Handle --flag=value forms consistently in both the runtime parser and discover rewrite logic.
Add regression coverage for classify and rewrite paths using inline global flag values before run.
@pszymkowiak pszymkowiak force-pushed the fix/golangci-lint-run-wrapper-develop branch from 69f909c to 24f2ada Compare April 12, 2026 21:17
Copy link
Copy Markdown
Collaborator

@pszymkowiak pszymkowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased the branch onto develop since it had drifted 2+ weeks. Had to drop the doc changes to resolve conflicts:

  • docs/TROUBLESHOOTING.md — deleted on develop (moved to docs/guide/troubleshooting.md by the docs refacto). Your content tweak was just replacing "golangci-lint" with "golangci-lint run" in one line — trivial to re-apply if needed.
  • docs/FEATURES.md → now at docs/usage/FEATURES.md — same kind of trivial text tweak dropped.
  • README.md — took develop's version; the PR change was the same text tweak.

Code changes intact across all 3 commits. Tested locally with a mock golangci-lint:

Before After
rtk golangci-lint versionrun --out-format=json version (mangled) version (correct passthrough)
rtk golangci-lint lintersrun --out-format=json linters (mangled) linters (correct passthrough)
rtk golangci-lint --versionrun --out-format=json --version (mangled) --version (correct passthrough)
rtk golangci-lint run → works still works
rtk golangci-lint --color=never run ./... → ? --color=never run ... ./... (flag preserved)

1408 tests pass, fmt + clippy clean. LGTM.

@pszymkowiak pszymkowiak requested a review from aeppling April 12, 2026 21:21
@FlorianBruniaux FlorianBruniaux merged commit 24875a2 into rtk-ai:develop Apr 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort-medium 1-2 jours, quelques fichiers filter-quality Filter produces incorrect/truncated signal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants