Skip to content

Commit 3ffab74

Browse files
AlexMikhalevAlexMikhalev
authored
fix(security): resolve RUSTSEC-2026-0098/0099/0097, remove dead deps
Refs #826
1 parent e832f15 commit 3ffab74

13 files changed

Lines changed: 1209 additions & 494 deletions

File tree

‎.cargo/audit.toml‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,29 @@ ignore = [
2323
# - Mitigation: discord feature disabled by default, patch attempted in Cargo.toml [patch.crates-io]
2424
# - Resolution: Remove once serenity 0.13+ releases with rustls 0.23+ support
2525
"RUSTSEC-2026-0049",
26+
# RUSTSEC-2026-0098: rustls-webpki name constraints for URI names incorrectly accepted
27+
# - Package: rustls-webpki 0.102.8 (via serenity 0.12.5 chain, same as RUSTSEC-2026-0049)
28+
# - serenity is OPTIONAL (discord feature), NOT in default features
29+
# - semver-incompatible with 0.103.x fix; blocked on serenity 0.13+
30+
# - Resolution: Remove once serenity 0.13+ releases
31+
"RUSTSEC-2026-0098",
32+
# RUSTSEC-2026-0099: rustls-webpki wildcard name constraints incorrectly accepted
33+
# - Package: rustls-webpki 0.102.8 (via serenity 0.12.5 chain, same as RUSTSEC-2026-0049/0098)
34+
# - serenity is OPTIONAL (discord feature), NOT in default features
35+
# - semver-incompatible with 0.103.x fix; blocked on serenity 0.13+
36+
# - Resolution: Remove once serenity 0.13+ releases
37+
"RUSTSEC-2026-0099",
38+
# RUSTSEC-2026-0097: rand unsound with custom logger using rand::rng()
39+
# - Affects rand 0.8.5 (transitive via tungstenite/sqlx/portpicker/mcp-client)
40+
# - and rand 0.9.2 (transitive via ulid/tokio-tungstenite/redis/rmcp)
41+
# - Our codebase does not use custom loggers that call rand::rng()
42+
# - No practical exploit path in our usage
43+
# - Resolution: Transitive deps will update naturally
44+
"RUSTSEC-2026-0097",
45+
# RUSTSEC-2025-0141: bincode 1.3.3 unmaintained
46+
# - Used by terraphim_automata and heed-types (via fff-search)
47+
# - Migration to bincode 2.0 or postcard planned as separate task
48+
# - No known vulnerabilities, just unmaintained
49+
# - Resolution: Migrate to bincode 2.0 or postcard (tracked in backlog)
50+
"RUSTSEC-2025-0141",
2651
]

‎.docs/design-edm-scanner-626.md‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
# Design & Implementation Plan: EDM Scanner -- terraphim_negative_contribution Crate (#626)
2+
3+
## 1. Summary of Target Behavior
4+
5+
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.
6+
7+
## 2. Key Invariants and Acceptance Criteria
8+
9+
### Invariants
10+
- Aho-Corasick automaton built once at construction, never rebuilt
11+
- `NegativeContributionScanner` is `Clone` + `Send + Sync` (Arc-safe, no interior mutability)
12+
- Tier 1 patterns only -- no Tier 2 (`// FIXME`, `// HACK`)
13+
- Single-pass byte scanning for all patterns simultaneously
14+
15+
### Acceptance Criteria (from issue #626)
16+
- [ ] `todo!()` detected as High severity finding
17+
- [ ] `unimplemented!()` detected as High severity finding
18+
- [ ] `panic!("not implemented")` and `panic!("TODO")` detected as High severity
19+
- [ ] `todo!("` and `unimplemented!("` (with message) detected
20+
- [ ] `// terraphim: allow(stub)` suppresses finding on same line
21+
- [ ] Files under `/tests/`, `/examples/`, `/benches/` excluded by path
22+
- [ ] Files named `build.rs` excluded by path
23+
- [ ] Files with suffix `_test.rs` excluded by path
24+
- [ ] Files containing `#[test]` or `#[cfg(test)]` in full content excluded
25+
- [ ] `// FIXME` and `// HACK` NOT flagged (Tier 2, out of scope)
26+
- [ ] `cargo build -p terraphim_negative_contribution` succeeds
27+
- [ ] `cargo test -p terraphim_negative_contribution` passes
28+
- [ ] `cargo clippy -p terraphim_negative_contribution -- -D warnings` passes
29+
- [ ] `cargo fmt --check` passes
30+
31+
## 3. High-Level Design and Boundaries
32+
33+
```
34+
┌─────────────────────────────────────────────────┐
35+
│ terraphim_negative_contribution │
36+
│ │
37+
│ ┌──────────┐ ┌───────────┐ ┌──────────────┐│
38+
│ │patterns │──>│ scanner │──>│ReviewFinding ││
39+
│ │(Tier 1) │ │(AC-based) │ │(from types) ││
40+
│ └──────────┘ └───────────┘ └──────────────┘│
41+
│ │ │ │
42+
│ │ ┌────┴─────┐ │
43+
│ │ │exclusion │ │
44+
│ │ │logic │ │
45+
│ │ └──────────┘ │
46+
└─────────────────────────────────────────────────┘
47+
│ │
48+
v v
49+
aho-corasick terraphim_types
50+
(1.0.2) (ReviewFinding etc.)
51+
```
52+
53+
### Module Layout
54+
55+
| Module | Responsibility |
56+
|--------|---------------|
57+
| `src/lib.rs` | Crate root, re-exports, `NegativeContributionScanner` struct |
58+
| `src/patterns.rs` | Tier 1 pattern definitions, severity mapping |
59+
| `src/scanner.rs` | Aho-Corasick scanning, byte-offset-to-line conversion, suppression check |
60+
| `src/exclusion.rs` | `is_non_production()` path and content exclusion logic |
61+
62+
### Boundaries
63+
- **Inside**: Pattern definitions, scanning logic, exclusion logic, finding construction
64+
- **Outside**: File I/O (caller provides content), diff parsing (future), LSP integration (future)
65+
- **No dependency on `terraphim_automata`**: Our patterns are compile-time constants, not user thesauruses
66+
67+
## 4. File/Module-Level Change Plan
68+
69+
| File | Action | Before | After | Dependencies |
70+
|------|--------|--------|-------|--------------|
71+
| `crates/terraphim_negative_contribution/Cargo.toml` | Create | - | Crate manifest with aho-corasick, terraphim_types deps | - |
72+
| `crates/terraphim_negative_contribution/src/lib.rs` | Create | - | Crate root, re-exports, `NegativeContributionScanner` struct | patterns, scanner, exclusion |
73+
| `crates/terraphim_negative_contribution/src/patterns.rs` | Create | - | `NEGATIVE_PATTERNS_TIER1`, `PatternInfo` with severity | - |
74+
| `crates/terraphim_negative_contribution/src/scanner.rs` | Create | - | `scan_content()` method, byte-offset-to-line, suppression | patterns, terraphim_types |
75+
| `crates/terraphim_negative_contribution/src/exclusion.rs` | Create | - | `is_non_production(path, content)` | - |
76+
77+
No existing files modified. Pure addition.
78+
79+
## 5. Step-by-Step Implementation Sequence
80+
81+
### Step 1: Cargo.toml + empty lib.rs (~5 min)
82+
- Create crate directory
83+
- `Cargo.toml` with `aho-corasick = "1.0.2"`, `terraphim_types` path dep
84+
- Empty `lib.rs` with crate-level docs
85+
- **Deployable**: `cargo check -p terraphim_negative_contribution` passes
86+
87+
### Step 2: patterns.rs (~10 min)
88+
- Define `NEGATIVE_PATTERNS_TIER1: &[&str]` with the 6 patterns
89+
- Define `PatternInfo` struct mapping pattern index to severity and description
90+
- Unit test: verify pattern count and content
91+
- **Deployable**: compiles, pattern unit test passes
92+
93+
### Step 3: exclusion.rs (~15 min)
94+
- Implement `is_non_production(path: &str, full_content: &str) -> bool`
95+
- Path checks: `/tests/`, `/examples/`, `/benches/`, `build.rs`, `_test.rs` suffix
96+
- Content checks: `#[test]` or `#[cfg(test)]` anywhere in file
97+
- Unit tests for each exclusion rule (positive and negative)
98+
- **Deployable**: exclusion tests pass
99+
100+
### Step 4: scanner.rs (~30 min)
101+
- `NegativeContributionScanner` struct with `AhoCorasick` field + `PatternInfo` vec
102+
- `new()` constructor: build automaton from `NEGATIVE_PATTERNS_TIER1`
103+
- `scan_file(path, content) -> Vec<ReviewFinding>`:
104+
1. Check `is_non_production()` -> return empty if excluded
105+
2. Build line offset map (Vec<usize> of `\n` positions) for byte-to-line conversion
106+
3. Run `automaton.find_iter(content)` over byte slices
107+
4. For each match: get line number, check suppression, construct `ReviewFinding`
108+
- `scan_files(files: &[(String, String)]) -> Vec<ReviewFinding>`: iterate + flatten
109+
- Unit tests for: basic detection, suppression, exclusion, multi-pattern, zero findings
110+
- **Deployable**: all scanner tests pass
111+
112+
### Step 5: lib.rs integration + derive (~10 min)
113+
- `NegativeContributionScanner` derives `Clone` (AhoCorasick is Clone)
114+
- Re-export public API
115+
- Wire everything together
116+
- **Deployable**: full crate compiles and tests pass
117+
118+
### Step 6: Clippy + fmt + final verification (~5 min)
119+
- `cargo clippy -p terraphim_negative_contribution -- -D warnings`
120+
- `cargo fmt -p terraphim_negative_contribution -- --check`
121+
- `cargo test -p terraphim_negative_contribution`
122+
- **Deployable**: all quality gates green
123+
124+
## 6. Testing & Verification Strategy
125+
126+
| Acceptance Criteria | Test Type | Test Location |
127+
|---------------------|-----------|---------------|
128+
| `todo!()` detected | Unit | `scanner.rs` `test_detect_todo` |
129+
| `unimplemented!()` detected | Unit | `scanner.rs` `test_detect_unimplemented` |
130+
| `panic!("not implemented")` detected | Unit | `scanner.rs` `test_detect_panic_not_implemented` |
131+
| `panic!("TODO")` detected | Unit | `scanner.rs` `test_detect_panic_todo` |
132+
| `todo!("with message")` detected | Unit | `scanner.rs` `test_detect_todo_with_message` |
133+
| `// terraphim: allow(stub)` suppresses | Unit | `scanner.rs` `test_suppression` |
134+
| `/tests/` path excluded | Unit | `exclusion.rs` `test_tests_dir_excluded` |
135+
| `/examples/` path excluded | Unit | `exclusion.rs` `test_examples_dir_excluded` |
136+
| `/benches/` path excluded | Unit | `exclusion.rs` `test_benches_dir_excluded` |
137+
| `build.rs` excluded | Unit | `exclusion.rs` `test_build_rs_excluded` |
138+
| `_test.rs` suffix excluded | Unit | `exclusion.rs` `test_test_suffix_excluded` |
139+
| `#[test]` in content excluded | Unit | `exclusion.rs` `test_inline_test_excluded` |
140+
| `#[cfg(test)]` in content excluded | Unit | `exclusion.rs` `test_cfg_test_excluded` |
141+
| `// FIXME` NOT flagged | Unit | `scanner.rs` `test_fixme_not_flagged` |
142+
| `// HACK` NOT flagged | Unit | `scanner.rs` `test_hack_not_flagged` |
143+
| Multiple patterns in one file | Unit | `scanner.rs` `test_multiple_patterns` |
144+
| Clean file returns empty | Unit | `scanner.rs` `test_clean_file` |
145+
146+
## 7. Risk & Complexity Review
147+
148+
| Risk | Mitigation | Residual Risk |
149+
|------|------------|---------------|
150+
| Aho-Corasick version conflict | Pin `aho-corasick = "1.0.2"` matching terraphim_automata | None |
151+
| False positive in doc comments | Patterns include `()` which don't appear in doc comments | Very low |
152+
| `Clone` derive on AhoCorasick | AhoCorasick implements Clone natively | None |
153+
| Line number off-by-one | Use 0-based `\n` counting, convert to 1-indexed | Test thoroughly |
154+
155+
## 8. Open Questions / Decisions for Human Reviewer
156+
157+
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.
158+
159+
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.
160+
161+
3. **`is_non_production` visibility**: I recommend making it `pub` so the orchestrator can pre-filter file lists before calling the scanner.
162+
163+
4. **Confidence value**: I recommend `0.95` for exact macro matches since they are deterministic.

‎.docs/research-edm-scanner-626.md‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
# Research Document: EDM Scanner -- terraphim_negative_contribution Crate
2+
3+
## 1. Problem Restatement and Scope
4+
5+
**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.
6+
7+
**In scope** (#626 only):
8+
- New crate `terraphim_negative_contribution` with Tier 1 pattern matching
9+
- Aho-Corasick automaton for multi-pattern byte scanning
10+
- File exclusion logic (tests, examples, benches, build.rs, inline test modules)
11+
- Inline suppression via `// terraphim: allow(stub)`
12+
- Output as `Vec<ReviewFinding>` compatible with existing compound review pipeline
13+
- Unit tests covering all specified cases
14+
15+
**Out of scope** (deferred to later steps):
16+
- Compound review wiring (`run_static_precheck()` in orchestrator) -- #627
17+
- Integration test baseline against codebase -- #628
18+
- LSP server crate (`terraphim_lsp`) -- #629
19+
- Tier 2 patterns (`// FIXME`, `// HACK`)
20+
- CoverageSignal analysis (eliminated by via-negativa)
21+
22+
## 2. User & Business Outcomes
23+
24+
- Compound review detects stubs before spending LLM tokens on them
25+
- Review agents spend time on real issues, not scaffolding detection
26+
- Foundational crate enables future LSP integration for real-time editor feedback
27+
- Zero LLM cost for this class of detection
28+
29+
## 3. System Elements and Dependencies
30+
31+
### New Crate
32+
| Element | Location | Role |
33+
|---------|----------|------|
34+
| `terraphim_negative_contribution` | `crates/terraphim_negative_contribution/` | Core EDM scanner |
35+
36+
### Existing Dependencies (incoming)
37+
| Crate | Version | What We Use |
38+
|-------|---------|-------------|
39+
| `terraphim_types` | 1.15+ (workspace) | `ReviewFinding`, `FindingSeverity`, `FindingCategory`, `ReviewAgentOutput`, `deduplicate_findings()` |
40+
| `aho-corasick` | 1.0.2 (via terraphim_automata) | Multi-pattern byte scanning. Direct dep needed since we don't use Thesaurus/find_matches. |
41+
42+
### Downstream Consumers (outgoing, future)
43+
| Crate | How They'll Use It |
44+
|-------|-------------------|
45+
| `terraphim_orchestrator` | `Arc<NegativeContributionScanner>` in compound review pre-check (#627) |
46+
| `terraphim_lsp` | Scanner reuse for editor diagnostics (#629) |
47+
48+
### Key Type Mappings
49+
```
50+
aho_corasick::AhoCorasick -> built once at new(), shared via &self
51+
pattern index -> NEGATIVE_PATTERNS_TIER1[index] -> pattern name
52+
byte offset -> count \n before offset -> line number
53+
line content -> contains "terraphim: allow(stub)" -> suppress
54+
file path -> is_non_production() -> skip entirely
55+
```
56+
57+
### Review Types (from terraphim_types::review)
58+
```rust
59+
ReviewFinding {
60+
file: String, // relative path
61+
line: u32, // 1-indexed line number
62+
severity: FindingSeverity, // High for todo!/unimplemented!()
63+
category: FindingCategory, // Quality
64+
finding: String, // human-readable description
65+
suggestion: Option<String>, // "Replace with implementation"
66+
confidence: f64, // 0.95 for exact macro match
67+
}
68+
69+
ReviewAgentOutput {
70+
agent: String, // "edm-scanner"
71+
findings: Vec<ReviewFinding>,
72+
summary: String,
73+
pass: bool, // true if 0 findings
74+
}
75+
```
76+
77+
## 4. Constraints and Their Implications
78+
79+
| Constraint | Why It Matters | Implication |
80+
|------------|----------------|-------------|
81+
| Aho-Corasick built once at `new()` | Avoids rebuilding per file | Scanner must be `Clone` + `Arc`-safe, no interior mutability |
82+
| `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 |
83+
| Tier 1 patterns only | Tier 2 (`// FIXME`) has too many false positives | Only exact macro invocations: `todo!()`, `unimplemented!()`, panic variants |
84+
| Inline suppression | Some stubs are intentional (e.g., trait defaults) | Check line content for `// terraphim: allow(stub)` before creating finding |
85+
| No `diff.rs` | Use `diffy` crate | Don't reinvent diff parsing. But for Step 1, we scan file content, not diffs |
86+
| Edition 2024 | Workspace standard | `use` statements must follow 2024 edition rules |
87+
| Must not depend on `terraphim_automata` Thesaurus | Our patterns are fixed strings, not user-configurable thesaurus entries | Use `aho-corasick` directly, not `find_matches()` |
88+
89+
## 5. Risks, Unknowns, and Assumptions
90+
91+
### Risks
92+
| Risk | Impact | Likelihood | Mitigation |
93+
|------|--------|------------|------------|
94+
| False positives in macro-heavy crates | Medium | Low | Inline suppression mechanism |
95+
| `aho-corasick` version conflict with `terraphim_automata` | Build failure | Low | Pin same version (1.0.2) |
96+
| Scanning large files slowly | Performance | Low | Aho-Corasick is O(n); single pass |
97+
98+
### Unknowns
99+
- Whether `diffy` is needed for Step 1 (likely no -- we scan full file content, not diffs)
100+
- Whether `ReviewAgentOutput` needs to be produced by the scanner itself or by the orchestrator wrapper
101+
102+
### Assumptions
103+
- The scanner operates on file content strings, not git diffs (diff-based scanning is future work)
104+
- `ReviewAgentOutput` is constructed by the caller, not the scanner itself
105+
- Confidence for exact macro matches is 0.95 (near-certain)
106+
- File paths are relative to repo root
107+
108+
## 6. Context Complexity vs. Simplicity Opportunities
109+
110+
### Sources of Complexity
111+
- Two exclusion mechanisms (path-based AND content-based)
112+
- Suppression syntax parsing
113+
114+
### Simplifications
115+
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.
116+
2. **Single-pass scanning**: Aho-Corasick scans for all patterns simultaneously. No need for multiple passes.
117+
3. **Separate `is_non_production` from scanning**: Clean separation of concerns. Scanner only scans; caller decides what files to feed it.
118+
119+
## 7. Questions for Human Reviewer
120+
121+
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.
122+
123+
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.
124+
125+
3. **Is `confidence: 0.95` appropriate for exact `todo!()` macro matches?** These are deterministic string matches, not probabilistic.
126+
127+
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.

0 commit comments

Comments
 (0)