Plan 348: stricter Rust lints and a rustdoc check

openspec/changes/348-rust-lints-strict/.run/plan.jsonl

@@ -0,0 +1 @@
+{"type":"plan_ready","timestamp":"2026-11-03T16:12:44.238Z","data":{"hash":"sha256:51fa09bba539062eea7b1b5017b3cc7f0bf3d9c3b4e8be565499ee07db118180","notices":[{"id":"rules_path","severity":"red","folded":false},{"id":"many_tasks","severity":"amber","folded":false},{"id":"large_scope","severity":"amber","folded":false},{"id":"assumptions","severity":"amber","folded":false},{"id":"tests_modify","severity":"grey","folded":false}]}}

openspec/changes/348-rust-lints-strict/proposal.md

@@ -0,0 +1,99 @@
+---
+title: Rust lints deny unwrap, reasonless allows, debug leftovers and broken doc links, so agents can't slip them past verify
+depends_on: []
+verify: sh scripts/verify-rust.sh
+check: pnpm verify
+features:
+  reads: [rust-harnesses, rust-storage, rust-vcs, rust-loop, rust-sync, rust-archive, rust-doctor, rust-inbox, rust-lint, rust-report, rust-approve, rust-land, rust-manual, rust-plan, rust-retry, rust-run, rust-show, rust-cli]
+---
+## Goal
+
+osq's Rust lints are already strict: pedantic, denied docs on every item,
+forbidden `unsafe` and an 80-line function cap. But an agent can still write
+`.unwrap()`, put `#[allow(dead_code)]` on a module with no reason, leave a
+`dbg!` or `todo!`, or write a doc link that points nowhere, and verify stays
+green. Most of osq's Rust is written by agents, and a rule an agent can break
+without the compiler noticing gets broken and then copied.
+
+This change adds eight clippy restriction lints to `[workspace.lints]` at
+deny and a rustdoc step to `scripts/verify-rust.sh`, and fixes every place
+that breaks them today, so the workspace stays green under the new table. It
+is Tier 1 of the 2026-11-03 lint audit and item 10, `rust-lints-google`, of
+the Google Rust guidelines report, narrowed to the lints with few hits.
+
+Measured on main at 7015dd2f with Rust 1.99.1, the new lints fire at:
+
+| Lint or check | Crate: hits |
+|---|---|
+| `unwrap_used`, with `allow-unwrap-in-tests` | harnesses src: 4. Test helpers outside `#[test]` functions: storage 136, vcs 93, inbox 61, archive 27, report 25, doctor 5, loop 4, lint 1 |
+| `allow_attributes_without_reason` | run 8, approve 2, plan 2, land 1, lint 1, loop 1, manual 1, report 1, retry 1, sync 1. All `allow(dead_code)` in tests |
+| `string_add` | show 7, run 4, harnesses 1, manual 1. All in tests |
+| `dbg_macro`, `todo`, `unimplemented`, `print_stdout`, `print_stderr` | none |
+| `RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps` | core 1, report 2, cli 1 |
+
+## Verify
+
+`sh scripts/verify-rust.sh`
+
+The script runs the format check, clippy with `-D warnings`, and after task 23 the rustdoc check, then every test, then cargo-mutants on the
+lines the change touched. Each fix task keeps it green, since the lints turn on
+only in task 22 and the rustdoc step only in task 23, after every hit is
+fixed. The `check` runs `pnpm verify` at archive and land,
+which covers the two new TypeScript tests and the changed verify script test.
+
+## Non-goals
+
+- `expect_used`, `indexing_slicing`, `panic` or `clippy::nursery`. `expect`
+  with a reason is the house style, and the others fire hundreds of times,
+  mostly in tests.
+- Tier 2 of the audit: `mod_module_files`, `as_conversions`,
+  `rust_2018_idioms`, `unused_qualifications`, `let_underscore_must_use` and
+  `missing_assert_message`. Each is its own change.
+- Tier 3: `max-fn-params-bools`, a Rust file line cap and `cargo-deny`.
+- `allow_attributes`, which would force `#[expect]`. A test support module is
+  compiled into several test binaries, and a binary that uses every item
+  would fail an `#[expect(dead_code)]` as unfulfilled.
+- The TypeScript side: `tsconfig.json` and `biome.json` do not change.
+- Rewriting AGENTS.md. The lints now say what it would.
+
+## Surface
+
+- Added: `[workspace.lints.clippy]` in `Cargo.toml` denies `unwrap_used`, `allow_attributes_without_reason`, `dbg_macro`, `todo`, `unimplemented`, `print_stdout`, `print_stderr` and `string_add`.
+- Added: `allow-unwrap-in-tests = true` in `clippy.toml`.
+- Added: `scripts/verify-rust.sh` runs `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` between clippy and the tests.
+
+## Decisions
+
+- ADR 023: this is an additive amendment. Section 4 says the lint table "includes at least" a set, and task 22 records the new lints and the rustdoc step in a dated amendment. No lint is loosened.
+- ADR 022: each fix task stays in one crate. No crate gains a dependency.
+- ADR 024: every public function keeps its `Implements:` tag, and verify still runs cargo-mutants on the changed lines. Harnesses' `unwrap` to `expect` lines must stay covered.
+- ADR 025: nothing under `openspec/` or `.run/` changes shape.
+- ADR 026: the specs kernel and its parity job do not change. Its tests have no hits.
+- ADR 027: scheduling does not change. The fix tasks run in a chain so each crate's clippy check sees harnesses already fixed.
+- ADR 028: no `Feature` implementation changes. Report's `run` keeps its code and only its doc text changes.
+- ADR 029: no config key changes. Lint levels live in `Cargo.toml` and `clippy.toml`, not osq's config.
+- ADR 030: no event or reaction changes.
+
+## Assumptions
+
+- The audit doc's first count of `unwrap` was 4. That was wrong: `allow-unwrap-in-tests` covers only `#[test]` functions, so 352 `unwrap`s in test helpers also need `expect`. This plan converts them all.
+- Clippy lints a workspace crate's library when it builds a crate that depends on it. So only harnesses' `src` hits block other crates, and task 2 comes before every other fix task.
+- ADR 023 is amended in place with a dated section, as 207 edited it before, and not superseded.
+- `cargo doc --workspace --no-deps` adds under a minute to each verify on this machine.
+- Tasks 2 to 21 edit test files and docs that rust-harnesses, rust-storage and 16 other capabilities own, with no delta for them: their behaviour and specs do not change. `osq lint` warns once per such file. Splitting the sweep into one change per capability would clear the warnings at the cost of 18 changes.
+
+## Human steps
+
+### Before approval
+
+None
+
+### After landing
+
+None
+
+## Delta
+
+- `specs/rust-core/spec.md`: ADDED "Workspace lint table" and "Verify script rustdoc check". MODIFIED "Code ownership" (gains `clippy.toml` and the two new lint tests), "Verify script mutation step" and "Verify script toolchain check" (the rustdoc command joins the cargo command list).
+
+`clippy.toml` is written by task 1 only; task 22 reads it. No file is shared between tasks.

openspec/changes/348-rust-lints-strict/specs/rust-core/spec.md

@@ -0,0 +1,123 @@
+# Spec Delta: rust-core
+
+## ADDED Requirements
+
+### Requirement: Workspace lint table
+The root `Cargo.toml`'s `[workspace.lints]` tables SHALL set every lint
+level below, so `unwrap`, a bare `#[allow]`, a debug macro, a placeholder, a
+print or `+` on strings fails clippy. `clippy.toml` SHALL set the settings
+below. `allow-unwrap-in-tests` allows `unwrap` inside `#[test]` functions
+only; helpers and `src/` use `expect` with a reason.
+
+#### Scenario: Workspace lint levels
+- **WHEN** the root `Cargo.toml` is read
+- **THEN** its `[workspace.lints]` tables set each lint to the level given:
+
+| Table | Lint | Level |
+|---|---|---|
+| `[workspace.lints.rust]` | `missing_docs` | `"deny"` |
+| `[workspace.lints.rust]` | `unsafe_code` | `"forbid"` |
+| `[workspace.lints.clippy]` | `pedantic` | `{ level = "warn", priority = -1 }` |
+| `[workspace.lints.clippy]` | `missing_docs_in_private_items` | `"deny"` |
+| `[workspace.lints.clippy]` | `too_many_lines` | `"deny"` |
+| `[workspace.lints.clippy]` | `unwrap_used` | `"deny"` |
+| `[workspace.lints.clippy]` | `allow_attributes_without_reason` | `"deny"` |
+| `[workspace.lints.clippy]` | `dbg_macro` | `"deny"` |
+| `[workspace.lints.clippy]` | `todo` | `"deny"` |
+| `[workspace.lints.clippy]` | `unimplemented` | `"deny"` |
+| `[workspace.lints.clippy]` | `print_stdout` | `"deny"` |
+| `[workspace.lints.clippy]` | `print_stderr` | `"deny"` |
+| `[workspace.lints.clippy]` | `string_add` | `"deny"` |
+
+#### Scenario: Clippy settings
+- **WHEN** `clippy.toml` is read
+- **THEN** it sets each key to the value given:
+
+| Key | Value |
+|---|---|
+| `too-many-lines-threshold` | `80` |
+| `allow-unwrap-in-tests` | `true` |
+
+### Requirement: Verify script rustdoc check
+`scripts/verify-rust.sh` SHALL build the workspace's docs with
+`cargo doc --workspace --no-deps` and `RUSTDOCFLAGS` set to `-D warnings`,
+after clippy and before the tests, so a broken intra-doc link, a redundant
+link target or a public doc that links a private item fails the verify.
+Clippy does not run rustdoc, so without this step those docs pass.
+
+#### Scenario: Verify script denies rustdoc warnings
+- **WHEN** `sh scripts/verify-rust.sh` runs with no `OSQ_DIFF` and a `cargo` on `PATH` that prints `cargo 1.99.1 (stub)` for `--version`, records its other arguments, and records the value of `RUSTDOCFLAGS` it was given when its first argument is `doc`
+- **THEN** the script exits 0, records `doc --workspace --no-deps` once, as the third command after `--version`, and the recorded `RUSTDOCFLAGS` is `-D warnings`
+
+## MODIFIED Requirements
+
+### Requirement: Code ownership
+<!-- source: crates/core/**, tests/rust-adrs.test.ts, tests/rust-ci.test.ts, tests/rust-verify-script.test.ts, scripts/verify-rust.sh, scripts/mutants-rust.sh, .cargo/mutants.toml, .github/workflows/rust.yml, rust-toolchain.toml, clippy.toml, tests/rust-clippy-settings.test.ts, tests/rust-workspace-lints.test.ts -->
+The rust-core capability SHALL own the `osq-core` crate, the Rust decisions
+test, and the Rust workspace's tooling files and their tests.
+
+#### Scenario: Codebase ownership boundaries
+- **WHEN** file ownership is resolved for the `Event` enum, the folds or the change typestate
+- **THEN** system maps `crates/core/**` to rust-core
+
+#### Scenario: Rust decisions test ownership
+- **WHEN** file ownership is resolved for `tests/rust-adrs.test.ts`
+- **THEN** system maps it to rust-core
+
+#### Scenario: Rust tooling ownership
+- **WHEN** file ownership is resolved for `scripts/verify-rust.sh`, `scripts/mutants-rust.sh`, `.cargo/mutants.toml`, `.github/workflows/rust.yml` and `tests/rust-ci.test.ts`
+- **THEN** system maps each to rust-core
+
+#### Scenario: Verify script test ownership
+- **WHEN** file ownership is resolved for `tests/rust-verify-script.test.ts`
+- **THEN** system maps it to rust-core
+
+#### Scenario: Toolchain file ownership
+- **WHEN** file ownership is resolved for `rust-toolchain.toml`
+- **THEN** system maps it to rust-core
+
+#### Scenario: Lint settings ownership
+- **WHEN** file ownership is resolved for `clippy.toml`, `tests/rust-clippy-settings.test.ts` and `tests/rust-workspace-lints.test.ts`
+- **THEN** system maps each to rust-core
+
+### Requirement: Verify script mutation step
+`scripts/verify-rust.sh` SHALL run the format check, clippy, the rustdoc
+check and the workspace tests with ignored corpus tests included, then, when
+`OSQ_DIFF` is set and not empty, run `sh scripts/mutants-rust.sh "$OSQ_DIFF"`
+and exit with its status, so a missed mutant or timeout in the changed lines
+fails the verify. When `OSQ_DIFF` is unset or empty, it SHALL print
+`mutation check not run: OSQ_DIFF is not set` and exit 0 after the tests.
+
+#### Scenario: Verify script steps
+- **WHEN** `sh scripts/verify-rust.sh` runs with a `cargo` on `PATH` that prints `cargo 1.99.1 (stub)` for `--version`, records its other arguments and exits with the status of its row for `mutants` and 0 otherwise
+- **THEN** the script runs the cargo commands and exits as given:
+
+| `OSQ_DIFF` | `mutants` status | cargo commands in order | Exit |
+|---|---|---|---|
+| a diff file | 0 | `fmt --all --check`, `clippy --workspace --all-targets -- -D warnings`, `doc --workspace --no-deps`, `test --workspace -- --include-ignored`, then `mutants` with `--in-diff` and the diff file's path | 0 |
+| a diff file | 2 | the same five | 2 |
+| unset | 0 | `fmt --all --check`, `clippy --workspace --all-targets -- -D warnings`, `doc --workspace --no-deps`, `test --workspace -- --include-ignored` | 0 |
+| empty | 0 | the same four | 0 |
+
+#### Scenario: Verify script names the missing diff
+- **WHEN** `sh scripts/verify-rust.sh` runs with that recording `cargo` and no `OSQ_DIFF`
+- **THEN** its output holds `mutation check not run: OSQ_DIFF is not set`
+
+### Requirement: Verify script toolchain check
+Before any other cargo command, `scripts/verify-rust.sh` SHALL compare
+the version that `cargo --version` prints, its second word, with the
+`channel` of `rust-toolchain.toml`. When they differ, it SHALL print
+`cargo <found> is on PATH, but rust-toolchain.toml pins <pinned>; put rustup's cargo proxy first on PATH`
+to standard error and exit 1 without running another cargo command, so a
+`cargo` that ignores the toolchain file cannot lint with a different
+clippy.
+
+#### Scenario: Verify script refuses another toolchain
+- **WHEN** `sh scripts/verify-rust.sh` runs with no `OSQ_DIFF` and a `cargo` on `PATH` that prints its row's text for `--version`, records its other arguments and exits 0
+- **THEN** the script exits, prints and runs the cargo commands as given:
+
+| `cargo --version` prints | Exit | Output holds | cargo commands after `--version` |
+|---|---|---|---|
+| `cargo 1.99.1 (stub)` | 0 | `mutation check not run: OSQ_DIFF is not set` | `fmt --all --check`, `clippy --workspace --all-targets -- -D warnings`, `doc --workspace --no-deps`, `test --workspace -- --include-ignored` |
+| `cargo 1.99.0 (stub)` | 1 | `cargo 1.99.0 is on PATH, but rust-toolchain.toml pins 1.99.1; put rustup's cargo proxy first on PATH` | none |
+| `cargo 1.97.0 (stub)` | 1 | `cargo 1.97.0 is on PATH, but rust-toolchain.toml pins 1.99.1; put rustup's cargo proxy first on PATH` | none |

openspec/changes/348-rust-lints-strict/tasks.md

@@ -0,0 +1,36 @@
+# Tasks
+
+## 1. Clippy settings
+
+- [ ] 1. When clippy checks a #[test] function, unwrap is allowed there and nowhere else
+
+## 2. Fix every clippy hit, one crate at a time
+
+- [ ] 2. When the harnesses kernel takes a piped child's stdout or stderr, it says why the pipe is there instead of unwrapping
+- [ ] 3. When storage kernel test helpers fail a setup step, they panic with expect and a reason instead of unwrap
+- [ ] 4. When vcs kernel test helpers fail a git setup step, they panic with expect and a reason instead of unwrap
+- [ ] 5. When loop kernel tests share a support module or fail a setup step, the allow and the panic both say why
+- [ ] 6. When sync kernel tests share a support module, its dead_code allow says why
+- [ ] 7. When archive feature test helpers fail a setup step, they panic with expect and a reason instead of unwrap
+- [ ] 8. When doctor feature test helpers fail a setup step, they panic with expect and a reason instead of unwrap
+- [ ] 9. When inbox feature test helpers fail a setup step, they panic with expect and a reason instead of unwrap
+- [ ] 10. When lint feature tests share a support module or fail a setup step, the allow and the panic both say why
+- [ ] 11. When report feature tests share a support module or fail a setup step, the allow and the panic both say why
+- [ ] 12. When approve feature tests share a support module, its dead_code allow says why
+- [ ] 13. When land feature tests share a support module, its dead_code allow says why
+- [ ] 14. When manual feature tests share a support module, its allow says why and it builds strings with format
+- [ ] 15. When plan feature tests include a shared module, its dead_code allow says why
+- [ ] 16. When retry feature tests share a support module, its dead_code allow says why
+- [ ] 17. When run feature tests share support modules or build prompt text, every allow says why and strings use format
+- [ ] 18. When show feature land tests build expected text, they use format instead of adding strings
+
+## 3. Fix every rustdoc warning
+
+- [ ] 19. When the report feature's docs build, its public run doc links no private item
+- [ ] 20. When core's docs build, the event kind module's link to Event::kind resolves
+- [ ] 21. When the CLI app's docs build, its crate doc has no redundant link target
+
+## 4. Turn the checks on
+
+- [ ] 22. When clippy checks the workspace, the eight new restriction lints are denied and ADR 023 says why
+- [ ] 23. When the Rust workspace is verified, a rustdoc warning fails it

openspec/changes/348-rust-lints-strict/tasks/1.md

@@ -0,0 +1,28 @@
+---
+title: When clippy checks a #[test] function, unwrap is allowed there and nowhere else
+verify: node --import tsx --import ./tests/git-test-env.ts --test tests/rust-clippy-settings.test.ts
+verify_starts: red
+scope:
+  - clippy.toml
+  - tests/rust-clippy-settings.test.ts
+entry:
+  - clippy.toml
+  - tests/rust-ci.test.ts
+skills: []
+---
+## Acceptance
+
+- [ ] `clippy.toml` keeps its comment and `too-many-lines-threshold = 80`, and gains `allow-unwrap-in-tests = true` with a one-line comment saying `unwrap` stays allowed inside `#[test]` functions only, and test helpers use `expect`.
+- [ ] New `tests/rust-clippy-settings.test.ts` holds `describe('rust-core: Clippy settings', ...)` with one case per table row of that scenario. It reads `clippy.toml` with plain text matching, one `key = value` line per row; add no TOML dependency.
+- [ ] `pnpm lint` reports nothing in `tests/rust-clippy-settings.test.ts`.
+
+## Details
+
+Follow the shape of `tests/rust-ci.test.ts`, which reads
+`rust-toolchain.toml` the same way: `repoRoot` from `import.meta.url`,
+`node:test` and `node:assert/strict`. The lint itself turns on in task 22,
+so clippy's output does not change in this task.
+
+## Scenarios
+
+- rust-core: Clippy settings

openspec/changes/348-rust-lints-strict/tasks/10.md

@@ -0,0 +1,20 @@
+---
+title: When lint feature tests share a support module or fail a setup step, the allow and the panic both say why
+verify: cargo clippy -p osq-feature-lint --all-targets -- -D warnings -D clippy::unwrap_used -D clippy::allow_attributes_without_reason -D clippy::string_add
+verify_starts: red
+depends_on: [9]
+tests:
+  modify: true
+scope:
+  - crates/features/lint/tests/lint_entry_git.rs
+  - crates/features/lint/tests/support/mod.rs
+entry:
+  - crates/features/lint/tests/support/mod.rs
+skills: []
+---
+## Acceptance
+
+- [ ] The `.unwrap()` that clippy's `unwrap_used` reports in `tests/lint_entry_git.rs` becomes `.expect("...")` with a message naming the step.
+- [ ] `#![allow(dead_code)]` in `tests/support/mod.rs` becomes `#![allow(dead_code, reason = "...")]`, the reason saying several test binaries include the module and each uses only some of its items. It stays `allow`, not `expect`.
+- [ ] No test asserts anything different, and no test is added, removed or renamed.
+- [ ] The task's `verify` passes.

openspec/changes/348-rust-lints-strict/tasks/11.md

@@ -0,0 +1,22 @@
+---
+title: When report feature tests share a support module or fail a setup step, the allow and the panic both say why
+verify: cargo clippy -p osq-feature-report --all-targets -- -D warnings -D clippy::unwrap_used -D clippy::allow_attributes_without_reason -D clippy::string_add
+verify_starts: red
+depends_on: [10]
+tests:
+  modify: true
+scope:
+  - crates/features/report/tests/query_json.rs
+  - crates/features/report/tests/query_run.rs
+  - crates/features/report/tests/support/mod.rs
+  - crates/features/report/tests/summary_support/mod.rs
+entry:
+  - crates/features/report/tests/support/mod.rs
+skills: []
+---
+## Acceptance
+
+- [ ] Every `.unwrap()` under `crates/features/report/tests/` that clippy's `unwrap_used` reports, 25 today, becomes `.expect("...")` with a message naming the step. An `unwrap` inside a `#[test]` function may stay.
+- [ ] `#![allow(dead_code)]` in `tests/summary_support/mod.rs` becomes `#![allow(dead_code, reason = "...")]`, the reason saying several test binaries include the module and each uses only some of its items. It stays `allow`, not `expect`.
+- [ ] No test asserts anything different, and no test is added, removed or renamed.
+- [ ] The task's `verify` passes.