Skip to content

fix(security): resolve RUSTSEC-2026-0098/0099/0097, remove dead deps #826

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.

Sign up for GitHub

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
AlexMikhalev merged 4 commits into from
Apr 21, 2026
Merged

fix(security): resolve RUSTSEC-2026-0098/0099/0097, remove dead deps #826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cb7b68
fix(security): resolve RUSTSEC-2026-0098/0099/0097, remove dead mcp-c…
Apr 21, 2026
fb29ebf
fix(clippy): suppress dead_code warnings for future-use functions
Apr 21, 2026
fea8723
style(fmt): fix import ordering Refs #826
AlexMikhalev Apr 21, 2026
0c8bb3c
feat(edm-scanner): create terraphim_negative_contribution crate (#626)
Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .cargo/audit.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,29 @@ ignore = [
# - Mitigation: discord feature disabled by default, patch attempted in Cargo.toml [patch.crates-io]
# - Resolution: Remove once serenity 0.13+ releases with rustls 0.23+ support
"RUSTSEC-2026-0049",
# RUSTSEC-2026-0098: rustls-webpki name constraints for URI names incorrectly accepted
# - Package: rustls-webpki 0.102.8 (via serenity 0.12.5 chain, same as RUSTSEC-2026-0049)
# - serenity is OPTIONAL (discord feature), NOT in default features
# - semver-incompatible with 0.103.x fix; blocked on serenity 0.13+
# - Resolution: Remove once serenity 0.13+ releases
"RUSTSEC-2026-0098",
# RUSTSEC-2026-0099: rustls-webpki wildcard name constraints incorrectly accepted
# - Package: rustls-webpki 0.102.8 (via serenity 0.12.5 chain, same as RUSTSEC-2026-0049/0098)
# - serenity is OPTIONAL (discord feature), NOT in default features
# - semver-incompatible with 0.103.x fix; blocked on serenity 0.13+
# - Resolution: Remove once serenity 0.13+ releases
"RUSTSEC-2026-0099",
# RUSTSEC-2026-0097: rand unsound with custom logger using rand::rng()
# - Affects rand 0.8.5 (transitive via tungstenite/sqlx/portpicker/mcp-client)
# - and rand 0.9.2 (transitive via ulid/tokio-tungstenite/redis/rmcp)
# - Our codebase does not use custom loggers that call rand::rng()
# - No practical exploit path in our usage
# - Resolution: Transitive deps will update naturally
"RUSTSEC-2026-0097",
# RUSTSEC-2025-0141: bincode 1.3.3 unmaintained
# - Used by terraphim_automata and heed-types (via fff-search)
# - Migration to bincode 2.0 or postcard planned as separate task
# - No known vulnerabilities, just unmaintained
# - Resolution: Migrate to bincode 2.0 or postcard (tracked in backlog)
"RUSTSEC-2025-0141",
]
163 changes: 163 additions & 0 deletions .docs/design-edm-scanner-626.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
# Design & Implementation Plan: EDM Scanner -- terraphim_negative_contribution Crate (#626)

## 1. Summary of Target Behavior

A new workspace crate `terraphim_negative_contribution` that statically detects Explicit Deferral Markers (EDMs) -- `todo!()`, `unimplemented!()`, and panic stubs -- in Rust source files. The scanner uses Aho-Corasick for O(n) multi-pattern matching, excludes non-production files, respects inline suppressions, and returns findings as `Vec<ReviewFinding>` for direct consumption by the compound review pipeline.

## 2. Key Invariants and Acceptance Criteria

### Invariants
- Aho-Corasick automaton built once at construction, never rebuilt
- `NegativeContributionScanner` is `Clone` + `Send + Sync` (Arc-safe, no interior mutability)
- Tier 1 patterns only -- no Tier 2 (`// FIXME`, `// HACK`)
- Single-pass byte scanning for all patterns simultaneously

### Acceptance Criteria (from issue #626)
- [ ] `todo!()` detected as High severity finding
- [ ] `unimplemented!()` detected as High severity finding
- [ ] `panic!("not implemented")` and `panic!("TODO")` detected as High severity
- [ ] `todo!("` and `unimplemented!("` (with message) detected
- [ ] `// terraphim: allow(stub)` suppresses finding on same line
- [ ] Files under `/tests/`, `/examples/`, `/benches/` excluded by path
- [ ] Files named `build.rs` excluded by path
- [ ] Files with suffix `_test.rs` excluded by path
- [ ] Files containing `#[test]` or `#[cfg(test)]` in full content excluded
- [ ] `// FIXME` and `// HACK` NOT flagged (Tier 2, out of scope)
- [ ] `cargo build -p terraphim_negative_contribution` succeeds
- [ ] `cargo test -p terraphim_negative_contribution` passes
- [ ] `cargo clippy -p terraphim_negative_contribution -- -D warnings` passes
- [ ] `cargo fmt --check` passes

## 3. High-Level Design and Boundaries

```
┌─────────────────────────────────────────────────┐
│ terraphim_negative_contribution │
│ │
│ ┌──────────┐ ┌───────────┐ ┌──────────────┐│
│ │patterns │──>│ scanner │──>│ReviewFinding ││
│ │(Tier 1) │ │(AC-based) │ │(from types) ││
│ └──────────┘ └───────────┘ └──────────────┘│
│ │ │ │
│ │ ┌────┴─────┐ │
│ │ │exclusion │ │
│ │ │logic │ │
│ │ └──────────┘ │
└─────────────────────────────────────────────────┘
│ │
v v
aho-corasick terraphim_types
(1.0.2) (ReviewFinding etc.)
```

### Module Layout

| Module | Responsibility |
|--------|---------------|
| `src/lib.rs` | Crate root, re-exports, `NegativeContributionScanner` struct |
| `src/patterns.rs` | Tier 1 pattern definitions, severity mapping |
| `src/scanner.rs` | Aho-Corasick scanning, byte-offset-to-line conversion, suppression check |
| `src/exclusion.rs` | `is_non_production()` path and content exclusion logic |

### Boundaries
- **Inside**: Pattern definitions, scanning logic, exclusion logic, finding construction
- **Outside**: File I/O (caller provides content), diff parsing (future), LSP integration (future)
- **No dependency on `terraphim_automata`**: Our patterns are compile-time constants, not user thesauruses

## 4. File/Module-Level Change Plan

| File | Action | Before | After | Dependencies |
|------|--------|--------|-------|--------------|
| `crates/terraphim_negative_contribution/Cargo.toml` | Create | - | Crate manifest with aho-corasick, terraphim_types deps | - |
| `crates/terraphim_negative_contribution/src/lib.rs` | Create | - | Crate root, re-exports, `NegativeContributionScanner` struct | patterns, scanner, exclusion |
| `crates/terraphim_negative_contribution/src/patterns.rs` | Create | - | `NEGATIVE_PATTERNS_TIER1`, `PatternInfo` with severity | - |
| `crates/terraphim_negative_contribution/src/scanner.rs` | Create | - | `scan_content()` method, byte-offset-to-line, suppression | patterns, terraphim_types |
| `crates/terraphim_negative_contribution/src/exclusion.rs` | Create | - | `is_non_production(path, content)` | - |

No existing files modified. Pure addition.

## 5. Step-by-Step Implementation Sequence

### Step 1: Cargo.toml + empty lib.rs (~5 min)
- Create crate directory
- `Cargo.toml` with `aho-corasick = "1.0.2"`, `terraphim_types` path dep
- Empty `lib.rs` with crate-level docs
- **Deployable**: `cargo check -p terraphim_negative_contribution` passes

### Step 2: patterns.rs (~10 min)
- Define `NEGATIVE_PATTERNS_TIER1: &[&str]` with the 6 patterns
- Define `PatternInfo` struct mapping pattern index to severity and description
- Unit test: verify pattern count and content
- **Deployable**: compiles, pattern unit test passes

### Step 3: exclusion.rs (~15 min)
- Implement `is_non_production(path: &str, full_content: &str) -> bool`
- Path checks: `/tests/`, `/examples/`, `/benches/`, `build.rs`, `_test.rs` suffix
- Content checks: `#[test]` or `#[cfg(test)]` anywhere in file
- Unit tests for each exclusion rule (positive and negative)
- **Deployable**: exclusion tests pass

### Step 4: scanner.rs (~30 min)
- `NegativeContributionScanner` struct with `AhoCorasick` field + `PatternInfo` vec
- `new()` constructor: build automaton from `NEGATIVE_PATTERNS_TIER1`
- `scan_file(path, content) -> Vec<ReviewFinding>`:
1. Check `is_non_production()` -> return empty if excluded
2. Build line offset map (Vec<usize> of `\n` positions) for byte-to-line conversion
3. Run `automaton.find_iter(content)` over byte slices
4. For each match: get line number, check suppression, construct `ReviewFinding`
- `scan_files(files: &[(String, String)]) -> Vec<ReviewFinding>`: iterate + flatten
- Unit tests for: basic detection, suppression, exclusion, multi-pattern, zero findings
- **Deployable**: all scanner tests pass

### Step 5: lib.rs integration + derive (~10 min)
- `NegativeContributionScanner` derives `Clone` (AhoCorasick is Clone)
- Re-export public API
- Wire everything together
- **Deployable**: full crate compiles and tests pass

### Step 6: Clippy + fmt + final verification (~5 min)
- `cargo clippy -p terraphim_negative_contribution -- -D warnings`
- `cargo fmt -p terraphim_negative_contribution -- --check`
- `cargo test -p terraphim_negative_contribution`
- **Deployable**: all quality gates green

## 6. Testing & Verification Strategy

| Acceptance Criteria | Test Type | Test Location |
|---------------------|-----------|---------------|
| `todo!()` detected | Unit | `scanner.rs` `test_detect_todo` |
| `unimplemented!()` detected | Unit | `scanner.rs` `test_detect_unimplemented` |
| `panic!("not implemented")` detected | Unit | `scanner.rs` `test_detect_panic_not_implemented` |
| `panic!("TODO")` detected | Unit | `scanner.rs` `test_detect_panic_todo` |
| `todo!("with message")` detected | Unit | `scanner.rs` `test_detect_todo_with_message` |
| `// terraphim: allow(stub)` suppresses | Unit | `scanner.rs` `test_suppression` |
| `/tests/` path excluded | Unit | `exclusion.rs` `test_tests_dir_excluded` |
| `/examples/` path excluded | Unit | `exclusion.rs` `test_examples_dir_excluded` |
| `/benches/` path excluded | Unit | `exclusion.rs` `test_benches_dir_excluded` |
| `build.rs` excluded | Unit | `exclusion.rs` `test_build_rs_excluded` |
| `_test.rs` suffix excluded | Unit | `exclusion.rs` `test_test_suffix_excluded` |
| `#[test]` in content excluded | Unit | `exclusion.rs` `test_inline_test_excluded` |
| `#[cfg(test)]` in content excluded | Unit | `exclusion.rs` `test_cfg_test_excluded` |
| `// FIXME` NOT flagged | Unit | `scanner.rs` `test_fixme_not_flagged` |
| `// HACK` NOT flagged | Unit | `scanner.rs` `test_hack_not_flagged` |
| Multiple patterns in one file | Unit | `scanner.rs` `test_multiple_patterns` |
| Clean file returns empty | Unit | `scanner.rs` `test_clean_file` |

## 7. Risk & Complexity Review

| Risk | Mitigation | Residual Risk |
|------|------------|---------------|
| Aho-Corasick version conflict | Pin `aho-corasick = "1.0.2"` matching terraphim_automata | None |
| False positive in doc comments | Patterns include `()` which don't appear in doc comments | Very low |
| `Clone` derive on AhoCorasick | AhoCorasick implements Clone natively | None |
| Line number off-by-one | Use 0-based `\n` counting, convert to 1-indexed | Test thoroughly |

## 8. Open Questions / Decisions for Human Reviewer

1. **`diffy` dependency**: I recommend NOT adding `diffy` for Step 1. We scan full file content, not diffs. Diff-based scanning can be added in Step 3 (compound review wiring) when needed.

2. **Scanner return type**: I recommend returning `Vec<ReviewFinding>` directly rather than a custom signal type. This gives immediate compatibility with the compound review pipeline.

3. **`is_non_production` visibility**: I recommend making it `pub` so the orchestrator can pre-filter file lists before calling the scanner.

4. **Confidence value**: I recommend `0.95` for exact macro matches since they are deterministic.
127 changes: 127 additions & 0 deletions .docs/research-edm-scanner-626.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# Research Document: EDM Scanner -- terraphim_negative_contribution Crate

## 1. Problem Restatement and Scope

**Problem**: LLM-generated code frequently contains deferral macros (`todo!()`, `unimplemented!()`, panic stubs) as scaffolding. These "negative contributions" slip through code review and accumulate as latent bugs. A zero-LLM static scanner can detect these before expensive LLM-based review runs.

**In scope** (#626 only):
- New crate `terraphim_negative_contribution` with Tier 1 pattern matching
- Aho-Corasick automaton for multi-pattern byte scanning
- File exclusion logic (tests, examples, benches, build.rs, inline test modules)
- Inline suppression via `// terraphim: allow(stub)`
- Output as `Vec<ReviewFinding>` compatible with existing compound review pipeline
- Unit tests covering all specified cases

**Out of scope** (deferred to later steps):
- Compound review wiring (`run_static_precheck()` in orchestrator) -- #627
- Integration test baseline against codebase -- #628
- LSP server crate (`terraphim_lsp`) -- #629
- Tier 2 patterns (`// FIXME`, `// HACK`)
- CoverageSignal analysis (eliminated by via-negativa)

## 2. User & Business Outcomes

- Compound review detects stubs before spending LLM tokens on them
- Review agents spend time on real issues, not scaffolding detection
- Foundational crate enables future LSP integration for real-time editor feedback
- Zero LLM cost for this class of detection

## 3. System Elements and Dependencies

### New Crate
| Element | Location | Role |
|---------|----------|------|
| `terraphim_negative_contribution` | `crates/terraphim_negative_contribution/` | Core EDM scanner |

### Existing Dependencies (incoming)
| Crate | Version | What We Use |
|-------|---------|-------------|
| `terraphim_types` | 1.15+ (workspace) | `ReviewFinding`, `FindingSeverity`, `FindingCategory`, `ReviewAgentOutput`, `deduplicate_findings()` |
| `aho-corasick` | 1.0.2 (via terraphim_automata) | Multi-pattern byte scanning. Direct dep needed since we don't use Thesaurus/find_matches. |

### Downstream Consumers (outgoing, future)
| Crate | How They'll Use It |
|-------|-------------------|
| `terraphim_orchestrator` | `Arc<NegativeContributionScanner>` in compound review pre-check (#627) |
| `terraphim_lsp` | Scanner reuse for editor diagnostics (#629) |

### Key Type Mappings
```
aho_corasick::AhoCorasick -> built once at new(), shared via &self
pattern index -> NEGATIVE_PATTERNS_TIER1[index] -> pattern name
byte offset -> count \n before offset -> line number
line content -> contains "terraphim: allow(stub)" -> suppress
file path -> is_non_production() -> skip entirely
```

### Review Types (from terraphim_types::review)
```rust
ReviewFinding {
file: String, // relative path
line: u32, // 1-indexed line number
severity: FindingSeverity, // High for todo!/unimplemented!()
category: FindingCategory, // Quality
finding: String, // human-readable description
suggestion: Option<String>, // "Replace with implementation"
confidence: f64, // 0.95 for exact macro match
}

ReviewAgentOutput {
agent: String, // "edm-scanner"
findings: Vec<ReviewFinding>,
summary: String,
pass: bool, // true if 0 findings
}
```

## 4. Constraints and Their Implications

| Constraint | Why It Matters | Implication |
|------------|----------------|-------------|
| Aho-Corasick built once at `new()` | Avoids rebuilding per file | Scanner must be `Clone` + `Arc`-safe, no interior mutability |
| `is_non_production` checks full content | `#[test]` can appear anywhere, not just top 20 lines | Must scan entire file content for test markers -- acceptable since we're already scanning for patterns |
| Tier 1 patterns only | Tier 2 (`// FIXME`) has too many false positives | Only exact macro invocations: `todo!()`, `unimplemented!()`, panic variants |
| Inline suppression | Some stubs are intentional (e.g., trait defaults) | Check line content for `// terraphim: allow(stub)` before creating finding |
| No `diff.rs` | Use `diffy` crate | Don't reinvent diff parsing. But for Step 1, we scan file content, not diffs |
| Edition 2024 | Workspace standard | `use` statements must follow 2024 edition rules |
| Must not depend on `terraphim_automata` Thesaurus | Our patterns are fixed strings, not user-configurable thesaurus entries | Use `aho-corasick` directly, not `find_matches()` |

## 5. Risks, Unknowns, and Assumptions

### Risks
| Risk | Impact | Likelihood | Mitigation |
|------|--------|------------|------------|
| False positives in macro-heavy crates | Medium | Low | Inline suppression mechanism |
| `aho-corasick` version conflict with `terraphim_automata` | Build failure | Low | Pin same version (1.0.2) |
| Scanning large files slowly | Performance | Low | Aho-Corasick is O(n); single pass |

### Unknowns
- Whether `diffy` is needed for Step 1 (likely no -- we scan full file content, not diffs)
- Whether `ReviewAgentOutput` needs to be produced by the scanner itself or by the orchestrator wrapper

### Assumptions
- The scanner operates on file content strings, not git diffs (diff-based scanning is future work)
- `ReviewAgentOutput` is constructed by the caller, not the scanner itself
- Confidence for exact macro matches is 0.95 (near-certain)
- File paths are relative to repo root

## 6. Context Complexity vs. Simplicity Opportunities

### Sources of Complexity
- Two exclusion mechanisms (path-based AND content-based)
- Suppression syntax parsing

### Simplifications
1. **Direct `aho-corasick` instead of `terraphim_automata`**: Our patterns are fixed compile-time constants. No need for Thesaurus builder, NormalizedTerm, or any KG machinery.
2. **Single-pass scanning**: Aho-Corasick scans for all patterns simultaneously. No need for multiple passes.
3. **Separate `is_non_production` from scanning**: Clean separation of concerns. Scanner only scans; caller decides what files to feed it.

## 7. Questions for Human Reviewer

1. **Should `diffy` be a dependency of this crate, or only added when diff-based scanning is needed in Steps 2-3?** I recommend omitting it for Step 1 since we scan full file content.

2. **Should the scanner return `Vec<ReviewFinding>` directly, or a custom `NegativeContributionSignal` type that the caller converts?** I recommend `Vec<ReviewFinding>` directly for simplicity and immediate compatibility.

3. **Is `confidence: 0.95` appropriate for exact `todo!()` macro matches?** These are deterministic string matches, not probabilistic.

4. **Should `is_non_production()` be a public function or internal to the scanner?** Making it public allows the orchestrator to pre-filter file lists before calling the scanner.
Loading
Loading