-
Notifications
You must be signed in to change notification settings - Fork 52
docs(issues): add specs for #1786 and #1787, move closed specs to closed/ #1788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
josecelano
merged 3 commits into
torrust:develop
from
josecelano:add-new-issue-specs-for-clippy-andmsrv-review
May 15, 2026
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
4 changes: 2 additions & 2 deletions
4
...open/1778-migrate-to-rust-edition-2024.md → ...osed/1778-migrate-to-rust-edition-2024.md
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
4 changes: 2 additions & 2 deletions
4
...-push-checks-performance-and-verbosity.md → ...-push-checks-performance-and-verbosity.md
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| --- | ||
| doc-type: issue | ||
| issue-type: task | ||
| status: planned | ||
| priority: p2 | ||
| github-issue: 1786 | ||
| spec-path: docs/issues/open/1786-tighten-lint-config.md | ||
| branch: "1786-tighten-lint-config" | ||
| related-pr: 1784 | ||
| last-updated-utc: 2026-05-15 08:00 | ||
| semantic-links: | ||
| skill-links: | ||
| - create-issue | ||
| related-artifacts: | ||
| - Cargo.toml | ||
| - .cargo/config.toml | ||
| --- | ||
|
|
||
| <!-- skill-link: create-issue --> | ||
|
|
||
| # Issue #1786 - Migrate lint configuration to `[workspace.lints]` in Cargo.toml | ||
|
|
||
| ## Goal | ||
|
|
||
| Replace the ad-hoc lint configuration spread across `.cargo/config.toml` RUSTFLAGS and | ||
| `torrust-linting` command-line arguments with a single authoritative `[workspace.lints]` | ||
| section in `Cargo.toml`, following the idiomatic Cargo approach used in `torrust-index`. | ||
|
|
||
| ## Background | ||
|
|
||
| Lint enforcement is currently split across three places: | ||
|
|
||
| 1. **`.cargo/config.toml` RUSTFLAGS** — carries rust-group denials (`-D warnings`, | ||
| `-D future-incompatible`, `-D rust-2018-idioms`, etc.). These apply to every cargo | ||
| invocation (build, test, check) but are invisible without reading the config file. | ||
|
|
||
| 2. **`torrust-linting` clippy runner** — passes `-D clippy::correctness`, | ||
| `-D clippy::suspicious`, `-D clippy::complexity`, `-D clippy::perf`, | ||
| `-D clippy::style`, `-D clippy::pedantic` on the command line. These are only | ||
| active when the linter tool runs; `cargo clippy` invoked directly does not | ||
| apply them. | ||
|
|
||
| 3. **`[lints.clippy]` on the root `[package]`** — the root `Cargo.toml` already has a | ||
| `[lints.clippy]` section for the main binary package only; this is _not_ a | ||
| `[workspace.lints]` and does not propagate to other workspace members. It also | ||
| contains `needless_return = "allow"` with a `# temp allow this lint` comment, | ||
| suggesting it was added as a temporary workaround rather than a deliberate policy | ||
| decision. The original reason and whether the underlying callsites have since been | ||
| fixed is unknown; this must be investigated before the section is migrated or removed. | ||
|
|
||
| This fragmentation was raised in PR #1784 review by @da2ce7, who referenced the | ||
| `torrust-index` configuration as the target state. | ||
|
|
||
| Cargo 1.64+ supports `[workspace.lints]`, the idiomatic way to declare workspace-wide | ||
| lint policy in a single, visible, version-controlled location. | ||
|
|
||
| ## Scope | ||
|
|
||
| ### In Scope | ||
|
|
||
| - Add `[workspace.lints.rust]` to the root `Cargo.toml` with the lint groups currently | ||
| expressed as RUSTFLAGS. | ||
| - Add `[workspace.lints.clippy]` to the root `Cargo.toml` with the clippy groups | ||
| currently passed by `torrust-linting`, plus `nursery = "warn"` as suggested in the | ||
| PR review. | ||
| - Remove the now-redundant lint entries from `RUSTFLAGS` in `.cargo/config.toml`. | ||
| - Remove the root `[lints.clippy]` package-level section (superseded by workspace lints). | ||
| - Fix any new warnings or errors that surface once `nursery = "warn"` and | ||
| `all = "deny"` take effect (expected to be small; most lints are already enforced). | ||
| - Investigate the `needless_return = "allow"` entry (see T7 below) and resolve it. | ||
| - Coordinate with `torrust-linting`: either remove the redundant `-D clippy::X` flags | ||
| from the clippy runner (cleaner) or document that they are intentional redundancy | ||
| (safety net). A follow-up PR to `torrust-linting` may be needed. | ||
|
|
||
| ### Out of Scope | ||
|
|
||
| - Changes to any other lint policy beyond migrating the existing set. | ||
| - Enabling additional deny-level lints beyond what is listed in the Background section. | ||
| - Changes to `torrust-linting` beyond removing the now-redundant clippy group flags. | ||
| - MSRV changes (tracked separately in #1787). | ||
|
|
||
| ## Implementation Plan | ||
|
|
||
| Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | ||
|
|
||
| | ID | Status | Task | Notes / Expected Output | | ||
| | --- | ------ | ------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------ | | ||
| | T1 | TODO | Add `[workspace.lints.rust]` to root `Cargo.toml` | Mirrors current RUSTFLAGS entries; `rust-2024-compatibility` added | | ||
| | T2 | TODO | Add `[workspace.lints.clippy]` to root `Cargo.toml` | Matches torrust-index config; `nursery = "warn"`, `all = "deny"` | | ||
| | T3 | TODO | Remove redundant RUSTFLAGS lint entries from `.cargo/config.toml` | Only lint-related entries removed; other rustflags (e.g. `-D unused`) migrated too | | ||
| | T4 | TODO | Remove root `[lints.clippy]` package section from `Cargo.toml` | Superseded by `[workspace.lints.clippy]` | | ||
| | T5 | TODO | Fix any new lint failures from `nursery = "warn"` / `all = "deny"` | `cargo clippy --workspace --all-targets --all-features` must pass cleanly | | ||
| | T6 | TODO | Update `torrust-linting` to remove redundant `-D clippy::X` flags | Open a separate PR in `torrust-linting`; document decision if deferred | | ||
| | T7 | TODO | Investigate and resolve `needless_return = "allow"` in `Cargo.toml` | See Background; decide: fix callsites and remove the allow, or keep it with documented rationale | | ||
| | T8 | TODO | Verify all quality gates pass | `linter all`, doc tests, full test suite, pre-push hook | | ||
|
|
||
| ## Progress Tracking | ||
|
|
||
| ### Workflow Checkpoints | ||
|
|
||
| - [ ] Spec drafted in `docs/issues/drafts/` | ||
| - [x] Spec reviewed and approved by user/maintainer | ||
| - [x] GitHub issue created and issue number added to this spec | ||
| - [ ] Implementation completed | ||
| - [ ] Automatic verification completed (`linter all`, relevant tests, and pre-push checks) | ||
| - [ ] Manual verification scenarios executed and recorded (status + evidence) | ||
| - [ ] Acceptance criteria reviewed after implementation and updated with evidence | ||
| - [ ] Reviewer validated acceptance criteria and updated checkboxes | ||
| - [ ] Committer verified spec progress is up to date before commit | ||
| - [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` | ||
|
|
||
| ### Progress Log | ||
|
|
||
| - 2026-05-15 07:00 UTC - Agent - Spec drafted, triggered by @da2ce7 review comment on PR #1784 | ||
| - 2026-05-15 08:00 UTC - Agent - GitHub issue #1786 created; spec moved from drafts/ to open/ | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - [ ] AC1: `[workspace.lints.rust]` in `Cargo.toml` covers all groups previously in RUSTFLAGS | ||
| - [ ] AC2: `[workspace.lints.clippy]` in `Cargo.toml` covers all groups previously passed by `torrust-linting`, plus `nursery = "warn"` and `all = "deny"` | ||
| - [ ] AC3: `.cargo/config.toml` no longer contains lint-related RUSTFLAGS entries | ||
| - [ ] AC4: The root package `[lints.clippy]` section is removed | ||
| - [ ] AC5: `cargo clippy --workspace --all-targets --all-features` exits `0` with no warnings | ||
| - [ ] AC6: `linter all` exits `0` | ||
| - [ ] AC7: All tests pass (`cargo test --workspace --all-targets --all-features`) | ||
| - [ ] AC8: Pre-push hook passes | ||
| - [ ] AC9: The `needless_return` allow is either removed (callsites fixed) or kept with a documented rationale replacing the `# temp allow this lint` comment | ||
| - [ ] AC10: Manual verification scenarios are executed and documented (status + evidence) | ||
| - [ ] AC11: Acceptance criteria are re-reviewed after implementation and reflect actual behavior | ||
|
|
||
| ## Verification Plan | ||
|
|
||
| ### Automatic Checks | ||
|
|
||
| - `linter all` | ||
| - `cargo clippy --workspace --all-targets --all-features` | ||
| - `cargo test --doc --workspace` | ||
| - `cargo test --tests --benches --examples --workspace --all-targets --all-features` | ||
| - Pre-push hook (full gate) | ||
|
|
||
| ### Manual Verification Scenarios | ||
|
|
||
| Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. | ||
|
|
||
| | ID | Scenario | Command/Steps | Expected Result | Status | Evidence | | ||
| | --- | ------------------------------------------------------------- | ------------------------------------------------------------ | ------------------------------------------- | ------ | -------- | | ||
| | M1 | Direct `cargo clippy` enforces workspace lints without linter | `cargo clippy --workspace --all-targets --all-features` | Exits 0; pedantic/nursery lints applied | TODO | | | ||
| | M2 | `cargo build` no longer picks up redundant lint RUSTFLAGS | `cargo build --workspace` (inspect output for lint warnings) | No spurious warnings from removed RUSTFLAGS | TODO | | | ||
| | M3 | `linter all` still passes with the new configuration | `linter all` | Exits 0 | TODO | | | ||
|
|
||
| ### Acceptance Verification | ||
|
|
||
| | AC ID | Status (`TODO`/`DONE`) | Evidence | | ||
| | ----- | ---------------------- | -------- | | ||
| | AC1 | TODO | | | ||
| | AC2 | TODO | | | ||
| | AC3 | TODO | | | ||
| | AC4 | TODO | | | ||
| | AC5 | TODO | | | ||
| | AC6 | TODO | | | ||
| | AC7 | TODO | | | ||
| | AC8 | TODO | | | ||
| | AC9 | TODO | | | ||
| | AC10 | TODO | | | ||
| | AC11 | TODO | | | ||
|
|
||
| ## Risks and Trade-offs | ||
|
|
||
| - **`nursery = "warn"` may surface many warnings**: nursery lints are experimental and | ||
| can be noisy. Fixing them is not mandatory for CI to pass (warn, not deny), but a | ||
| large warning count degrades signal quality. Monitor after enabling. | ||
| - **`torrust-linting` coordination**: if the redundant `-D` flags are left in the linter | ||
| after workspace lints are added, they remain harmless (idempotent) but add confusion. | ||
| Cleaning them up requires a separate PR to `torrust-linting`. | ||
|
|
||
| ## References | ||
|
|
||
| - Related PRs: #1784 | ||
| - Suggested by: @da2ce7 in PR #1784 review | ||
| - Reference config: `torrust-index` workspace `Cargo.toml` | ||
| - Related issue: #1787 (evaluate MSRV bump) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| --- | ||
| doc-type: issue | ||
| issue-type: task | ||
| status: blocked | ||
| priority: p2 | ||
| github-issue: 1787 | ||
| spec-path: docs/issues/open/1787-evaluate-msrv-bump.md | ||
| branch: "1787-evaluate-msrv-bump" | ||
| related-pr: 1784 | ||
| last-updated-utc: 2026-05-15 08:00 | ||
| blocked-by: "#1669 (package restructuring)" | ||
| semantic-links: | ||
| skill-links: | ||
| - create-issue | ||
| related-artifacts: | ||
| - Cargo.toml | ||
| - AGENTS.md | ||
| - .github/skills/dev/maintenance/setup-dev-environment/SKILL.md | ||
| --- | ||
|
|
||
| <!-- skill-link: create-issue --> | ||
|
|
||
| # Issue #1787 - Evaluate and update workspace MSRV above 1.85 | ||
|
|
||
| ## Goal | ||
|
|
||
| Decide on the appropriate Minimum Supported Rust Version (MSRV) for the workspace | ||
| given the project's trajectory (planned extraction of `bittorrent-*` crates as | ||
| independent libraries) and update `rust-version` in `Cargo.toml` accordingly. | ||
|
|
||
| ## Background | ||
|
|
||
| PR #1784 set `rust-version = "1.85"` — the strict minimum required to compile | ||
| Rust edition 2024. This was correct as the conservative baseline for the migration, | ||
| but 1.85 is now several releases behind the current stable toolchain. | ||
|
|
||
| Two classes of crate coexist in this workspace: | ||
|
|
||
| 1. **Application layer** (`torrust-tracker-*` crates and the main binary) — not | ||
| consumed as a library by external projects; MSRV currently has no downstream | ||
| impact. All workspace packages carry `publish.workspace = true` but none have | ||
| been published to crates.io yet. Which packages will actually be released, | ||
| under what names, and whether some will move to their own repositories is | ||
| being decided in #1669. | ||
|
|
||
| 2. **Protocol/domain layer** (`bittorrent-*` crates: `bittorrent-peer-id`, | ||
| `bittorrent-http-tracker-protocol`, `bittorrent-udp-tracker-protocol`, | ||
| `bittorrent-tracker-core`, `bittorrent-http-tracker-core`, | ||
| `bittorrent-udp-tracker-core`, `bittorrent-tracker-client`) — planned for | ||
| extraction into independent repositories and publication to crates.io, where | ||
| they will be consumed by other BitTorrent projects. | ||
|
|
||
| This dual nature creates a tension: | ||
|
|
||
| - **For the application layer**: there is no reason to stay on an old MSRV; tracking | ||
| a recent stable is better (access to new APIs, better diagnostics). | ||
| - **For the future libraries**: a conservative MSRV (e.g. latest stable minus two | ||
| releases, or a deliberate policy) is appropriate once they are published. | ||
|
|
||
| Until the `bittorrent-*` crates are extracted, a single workspace MSRV applies to | ||
| both classes, so the decision must be made with the extraction timeline in mind. | ||
|
|
||
| **This issue is currently blocked on #1669** (ongoing package restructuring). | ||
| Several decisions that directly affect the MSRV strategy have not yet been made: | ||
|
|
||
| - Which packages will be extracted as independent crates.io libraries. | ||
| - Final names for those packages. | ||
| - Which packages will share a versioning lifecycle with the main tracker and which | ||
| will evolve independently. | ||
| - Publication targets and minimum toolchain expectations for downstream consumers. | ||
|
|
||
| The MSRV evaluation should be re-opened only after #1669 has settled these questions. | ||
| Opening it sooner risks choosing a policy that becomes invalid once extraction scope | ||
| is defined. | ||
|
|
||
| ## Scope | ||
|
|
||
| ### In Scope | ||
|
|
||
| - Evaluate the appropriate MSRV policy for this workspace given the two crate classes. | ||
| - Define a policy: track latest stable, pin to a specific recent release, or maintain | ||
| a conservative floor. | ||
| - Update `rust-version` in `Cargo.toml` to the agreed value. | ||
| - Update all documentation that references the MSRV: | ||
| - `AGENTS.md` (line referencing `MSRV 1.85`) | ||
| - `.github/skills/dev/maintenance/setup-dev-environment/SKILL.md` | ||
| - Verify CI passes with the new MSRV value. | ||
|
|
||
| ### Out of Scope | ||
|
|
||
| - Extracting `bittorrent-*` crates to independent repositories (separate epic). | ||
| - Setting per-crate MSRV values (only the workspace `rust-version` is in scope here). | ||
| - Adding a MSRV CI job (may be proposed as a follow-up if a conservative MSRV is chosen). | ||
|
|
||
| ## Blockers | ||
|
|
||
| - **#1669 — Package restructuring**: names, extraction scope, versioning lifecycle, and | ||
| publication targets for the `bittorrent-*` crates must be decided before a rational | ||
| MSRV policy can be set. Track that issue and re-evaluate when it closes. | ||
|
|
||
| ## Implementation Plan | ||
|
|
||
| Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | ||
|
|
||
| | ID | Status | Task | Notes / Expected Output | | ||
| | --- | ------ | ------------------------------------------------------------------- | ------------------------------------------------------ | | ||
| | T1 | TODO | Decide MSRV policy (track latest stable vs. pin conservative floor) | Document the rationale in this spec before proceeding | | ||
| | T2 | TODO | Update `rust-version` in root `Cargo.toml` | Change from `"1.85"` to the agreed value | | ||
| | T3 | TODO | Update `AGENTS.md` MSRV reference | Keep in sync with `Cargo.toml` | | ||
| | T4 | TODO | Update setup-dev-environment SKILL.md MSRV reference | Keep in sync with `Cargo.toml` | | ||
| | T5 | TODO | Verify CI passes | Full quality gate (`linter all`, tests, pre-push hook) | | ||
|
|
||
| ## Progress Tracking | ||
|
|
||
| ### Workflow Checkpoints | ||
|
|
||
| - [ ] Spec drafted in `docs/issues/drafts/` | ||
| - [x] Spec reviewed and approved by user/maintainer | ||
| - [x] GitHub issue created and issue number added to this spec | ||
| - [ ] Implementation completed | ||
| - [ ] Automatic verification completed (`linter all`, relevant tests, and pre-push checks) | ||
| - [ ] Manual verification scenarios executed and recorded (status + evidence) | ||
| - [ ] Acceptance criteria reviewed after implementation and updated with evidence | ||
| - [ ] Reviewer validated acceptance criteria and updated checkboxes | ||
| - [ ] Committer verified spec progress is up to date before commit | ||
| - [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` | ||
|
|
||
| ### Progress Log | ||
|
|
||
| - 2026-05-15 07:00 UTC - Agent - Spec drafted, follow-up from PR #1784 (Rust edition 2024 migration, MSRV set to 1.85) | ||
| - 2026-05-15 07:30 UTC - Jose Celano - Marked blocked on #1669 (package restructuring); MSRV policy requires knowing extraction scope, names, and versioning lifecycle | ||
| - 2026-05-15 08:00 UTC - Agent - GitHub issue #1787 created; spec moved to docs/issues/open/ | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - [ ] AC1: A MSRV policy decision is recorded in this spec with rationale | ||
| - [ ] AC2: `rust-version` in `Cargo.toml` reflects the agreed value | ||
| - [ ] AC3: `AGENTS.md` MSRV reference is in sync with `Cargo.toml` | ||
| - [ ] AC4: `setup-dev-environment` SKILL.md MSRV reference is in sync with `Cargo.toml` | ||
| - [ ] AC5: `linter all` exits `0` | ||
| - [ ] AC6: All tests pass | ||
| - [ ] AC7: Pre-push hook passes | ||
|
|
||
| ## Verification Plan | ||
|
|
||
| ### Automatic Checks | ||
|
|
||
| - `linter all` | ||
| - `cargo check --workspace --all-targets --all-features` | ||
| - `cargo test --doc --workspace` | ||
| - Pre-push hook (full gate) | ||
|
|
||
| ### Manual Verification Scenarios | ||
|
|
||
| Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. | ||
|
|
||
| | ID | Scenario | Command/Steps | Expected Result | Status | Evidence | | ||
| | --- | -------------------------------------------------- | ------------------------------------------------ | ---------------------------------------- | ------ | -------- | | ||
| | M1 | `rust-version` in Cargo.toml matches documentation | Compare `Cargo.toml`, `AGENTS.md`, SKILL.md | All three reference the same MSRV string | TODO | | | ||
| | M2 | Workspace builds cleanly on the new MSRV toolchain | `rustup install <msrv>; cargo +<msrv> check ...` | Exit 0 with no errors | TODO | | | ||
|
|
||
| ### Acceptance Verification | ||
|
|
||
| | AC ID | Status (`TODO`/`DONE`) | Evidence | | ||
| | ----- | ---------------------- | -------- | | ||
| | AC1 | TODO | | | ||
| | AC2 | TODO | | | ||
| | AC3 | TODO | | | ||
| | AC4 | TODO | | | ||
| | AC5 | TODO | | | ||
| | AC6 | TODO | | | ||
| | AC7 | TODO | | | ||
|
|
||
| ## Risks and Trade-offs | ||
|
|
||
| - **Too high a MSRV before crate extraction**: if `bittorrent-*` crates are extracted | ||
| carrying a high MSRV, downstream BitTorrent projects may be forced to upgrade their | ||
| toolchain. Setting a modest floor now (e.g. current stable minus two releases) gives | ||
| the extracted crates a clean, defensible starting point. | ||
| - **Too low a MSRV after extraction**: the application layer has no reason to stay | ||
| conservative; a low MSRV denies developers access to new stable APIs and better | ||
| compiler diagnostics. | ||
| - **Drift without a MSRV CI job**: a stated MSRV is only trustworthy if CI verifies it. | ||
| If a conservative MSRV is chosen, a MSRV CI job should be added. | ||
|
|
||
| ## References | ||
|
|
||
| - Related PRs: #1784 | ||
| - Related issue: #1786 (tighten lint config) | ||
| - Blocked by: https://github.com/torrust/torrust-tracker/issues/1669 (package restructuring) | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.