Implementation Plan: Fix Pre-Existing Clippy Errors Blocking PR #2971

Status: Draft Research Doc: docs/research/pr-2971-merge-block-research.md Author: OpenCode (Terraphim AI agent) Date: 2026-06-25 Estimated Effort: 1–2 hours (implementation + verification + merge coordination)

Overview

Summary

Fix four pre-existing Clippy lint errors in three workspace crates so that cargo clippy --workspace --all-targets -- -D warnings passes. Once the fixes land on main, rebase PR #2971 so its required native-ci / build (push) status turns green and the PR can merge.

Approach

Apply minimal, semantically equivalent fixes for each lint error:

Convert runtime assert!s on constants into compile-time const { assert!(...) } blocks in terraphim_merge_coordinator.
Move build_router_for_tests in terraphim_server to appear before the mod tests block.
Remove the vacuous assert!(true) in terraphim_validation while keeping the test that exercises ValidationSystem::new().

Scope

In Scope:

Fix clippy::assertions_on_constants in crates/terraphim_merge_coordinator/src/gitea.rs.
Fix clippy::items_after_test_module in terraphim_server/src/lib.rs.
Fix clippy::assertions_on_constants in crates/terraphim_validation/src/lib.rs.
Local verification with cargo fmt, cargo clippy, and cargo build.
Rebase PR #2971 onto the fixed main.
Confirm PR #2971's native CI run succeeds.

Out of Scope:

Changes to the native-ci.yml workflow.
Fixing the rch::hook path-normalisation warning.
Broader lint or refactor work across the workspace.
Changes to ADF review agents or their status checks.

Avoid At All Cost (from 5/25 analysis):

Forcing a merge through admin override while the required check is red.
Silencing Clippy globally with #![allow(...)] instead of fixing the root cause.
Expanding the PR into unrelated refactoring.

Architecture

Component Diagram

+----------------------------+
|  terraphim_merge_coordinator |
|  (const assertions)         |
+----------------------------+
           |
+----------------------------+
|  terraphim_server           |
|  (test helper reordering)   |
+----------------------------+
           |
+----------------------------+
|  terraphim_validation       |
|  (remove vacuous assert)    |
+----------------------------+
           |
+----------------------------+
|  native CI (Gitea Actions)  |
|  native-ci / build (push)   |
+----------------------------+
           |
+----------------------------+
|  PR #2971 merge gate        |
+----------------------------+

Data Flow

Developer creates fix branch from main.
Apply the three code changes.
Run local format/lint/build verification.
Push fix branch, open PR, merge to main.
Rebase PR #2971 branch onto updated main.
Native CI runs automatically on the rebased push.
PR #2971 becomes mergeable.

Key Design Decisions

| Decision | Rationale | Alternatives Rejected | |----------|-----------|----------------------| | Use const { assert!(...) } for const invariants | Preserves the intent (compile-time failure if the constant violates the rule) and satisfies Clippy. | static_assertions::const_assert! — adds a dependency when std syntax is sufficient. | | Move build_router_for_tests above mod tests | Directly addresses clippy::items_after_test_module without adding #[allow(...)]. | #[allow(clippy::items_after_test_module)] — hides the lint rather than fixing structure. | | Remove assert!(true) but keep the test body | The test still exercises ValidationSystem::new().unwrap(); the assertion added no value. | assert!(!system.orchestrator.is_none()) — no such method exists and would be speculative. |

Eliminated Options (Essentialism)

| Option Rejected | Why Rejected | Risk of Including | |-----------------|--------------|-------------------| | Admin override merge of PR #2971 | Violates branch protection and sets a bad precedent. | Erodes CI authority; future PRs may expect the same override. | | #![allow(clippy::assertions_on_constants)] at crate level | Masks real issues and contradicts the -D warnings policy. | Technical debt accumulates; other const-assert mistakes go undetected. | | #[allow(clippy::items_after_test_module)] on build_router_for_tests | Hides structural issue; test helpers should live outside the test module. | Makes the file harder to navigate and maintain. | | Broad cargo clippy --fix across workspace | Could introduce unrelated changes and extend scope. | Risk of hidden regressions and a larger review surface. |

Simplicity Check

"Minimum code that solves the problem. Nothing speculative."

What if this could be easy? The easiest fix is four small edits: two const { assert!(...) } blocks, one function move, and one deleted line. No new dependencies, no workflow changes, no admin overrides.

Senior Engineer Test: A senior engineer would recognise these as straightforward lint fixes, not over-engineering.

Nothing Speculative Checklist:

[x] No features the user didn't request
[x] No abstractions "in case we need them later"
[x] No flexibility "just in case"
[x] No error handling for scenarios that cannot occur
[x] No premature optimization

File Changes

Modified Files

| File | Changes | |------|---------| | crates/terraphim_merge_coordinator/src/gitea.rs | Replace two runtime assert!s on OPEN_PRS_LIMIT with const { assert!(...) } blocks. | | terraphim_server/src/lib.rs | Move build_router_for_tests to before mod tests. | | crates/terraphim_validation/src/lib.rs | Remove assert!(true); while retaining ValidationSystem::new().unwrap(). |

No New or Deleted Files

No new modules or deleted files are required.

API Design

No public API changes. All edits are internal to test code or const validation.

Test Strategy

Verification Commands

| Test | Command | Purpose | |------|---------|---------| | Format check | cargo fmt --all -- --check | Ensures no formatting regressions. | | Lint check | cargo clippy --workspace --all-targets -- -D warnings | Confirms the four errors are gone and no new ones appear. | | Build check | cargo build --workspace | Confirms the workspace still compiles. | | Unit tests (workspace lib) | cargo test --workspace --lib --no-fail-fast | Confirms the changed tests still pass. | | Gitea-runner tests | cargo test -p terraphim_gitea_runner --no-fail-fast | Matches the native CI test step. |

Expected Outcomes

All five commands above exit 0.
The two terraphim_merge_coordinator const assertions still fail at compile time if OPEN_PRS_LIMIT is changed to an invalid value.
terraphim_server integration tests that use build_router_for_tests continue to compile and pass.
terraphim_validation::tests::test_validation_system_creation continues to exercise ValidationSystem::new().

Implementation Steps

Step 1: Create Fix Branch

Files: N/A Description: Create a branch from latest main for the lint fixes. Command: git checkout -b task/fix-clippy-errors-blocking-ci Tests: N/A Estimated: 2 minutes

Step 2: Fix Const Assertions in `terraphim_merge_coordinator`

Files: crates/terraphim_merge_coordinator/src/gitea.rs Description: Replace the two runtime assert!s with compile-time const { assert!(...) } blocks. Tests: cargo clippy -p terraphim_merge_coordinator --all-targets -- -D warnings Estimated: 10 minutes

// Before
#[test]
fn open_prs_limit_exceeds_gitea_default_of_50() {
    assert!(
        OPEN_PRS_LIMIT > 50,
        "OPEN_PRS_LIMIT must exceed 50 so PRs beyond position 50 are not silently dropped"
    );
}

#[test]
fn open_prs_limit_within_gitea_max_page_size() {
    assert!(
        OPEN_PRS_LIMIT <= 300,
        "Gitea max page size is 300; limit must not exceed it"
    );
}

// After
#[test]
fn open_prs_limit_exceeds_gitea_default_of_50() {
    const {
        assert!(
            OPEN_PRS_LIMIT > 50,
            "OPEN_PRS_LIMIT must exceed 50 so PRs beyond position 50 are not silently dropped"
        );
    }
}

#[test]
fn open_prs_limit_within_gitea_max_page_size() {
    const {
        assert!(
            OPEN_PRS_LIMIT <= 300,
            "Gitea max page size is 300; limit must not exceed it"
        );
    }
}

Step 3: Reorder Test Helper in `terraphim_server`

Files: terraphim_server/src/lib.rs Description: Move build_router_for_tests and its doc comment from after mod tests to before it. Tests: cargo clippy -p terraphim_server --all-targets -- -D warnings Estimated: 15 minutes

Key move:

/// Constructs a minimal Axum router suitable for integration tests.
pub async fn build_router_for_tests() -> Router {
    // ... existing body ...
}

#[cfg(test)]
mod tests {
    use super::*;
    // ... existing tests ...
}

Step 4: Remove Vacuous Assertion in `terraphim_validation`

Files: crates/terraphim_validation/src/lib.rs Description: Remove assert!(true); while keeping the call to ValidationSystem::new().unwrap(). Tests: cargo clippy -p terraphim_validation --all-targets -- -D warnings Estimated: 5 minutes

// Before
#[tokio::test]
async fn test_validation_system_creation() {
    let system = ValidationSystem::new().unwrap();
    assert!(true); // Basic creation test
}

// After
#[tokio::test]
async fn test_validation_system_creation() {
    let _system = ValidationSystem::new().unwrap();
}

Step 5: Local Verification

Files: All touched crates Description: Run the exact commands used by native CI to confirm the workspace is clean. Commands:

cargo fmt --all -- --check
cargo clippy --workspace --all-targets -- -D warnings
cargo build --workspace
cargo test --workspace --lib --no-fail-fast
cargo test -p terraphim_gitea_runner --no-fail-fast

Estimated: 20–40 minutes (build/test time dominates)

Step 6: Commit and Push Fix Branch

Files: N/A Description: Commit the changes with a clear message referencing the CI unblock. Command: git commit -am "fix(ci): resolve clippy errors blocking native-ci build Refs #2492" Estimated: 5 minutes

Step 7: Open and Merge Fix PR

Files: N/A Description: Create a PR on Gitea, wait for native CI to pass, then merge to main. Command: gtr create-pull --owner terraphim --repo terraphim-ai --title "fix(ci): resolve clippy errors blocking native-ci build" --base main --head task/fix-clippy-errors-blocking-ci Estimated: 15 minutes (mostly CI wait)

Step 8: Rebase PR #2971

Files: N/A Description: Update PR #2971's branch so it includes the fixed main. Commands:

git fetch origin
git checkout task/2492-tinyclaw-safety-comment-echo-pr2
git rebase origin/main
git push --force-with-lease origin task/2492-tinyclaw-safety-comment-echo-pr2

Estimated: 5 minutes

Step 9: Confirm PR #2971 CI Passes and Merge

Files: N/A Description: Wait for the new native CI run on PR #2971, confirm native-ci / build (push) is green, then merge. Command: gtr merge-pull --owner terraphim --repo terraphim-ai --index 2971 Estimated: 10 minutes (CI wait)

Rollback Plan

If any step causes unexpected failures:

Revert the fix PR on main using git revert <merge-commit>.
Push the revert to both remotes (origin then gitea).
Re-open PR #2971 if it was merged prematurely.

There are no feature flags involved.

Dependencies

New Dependencies

None.

Dependency Updates