feat: wave/7 gap fixes — all 9 Shepherd audit items

[dean0x]

Summary

• P0: Zero-stderr fix — Redirect check_hook_version_mismatch() from eprintln! to hook_log::log_hook_warning(), preserving the zero-stderr invariant in hook mode
• P0: HookProtocol dispatch — Wire protocol_for_agent() factory so all 6 agents (Claude, Cursor, Gemini, Copilot, Codex, OpenCode) use their protocol's parse_input()/format_response() instead of hardcoded Claude JSON
• P1: SHA-256 integrity — skim agents now uses verify_script_integrity() for hook status (ok/tampered/missing/unknown)
• P1: Hook timeout — 5-second watchdog thread calls process::exit(0) to prevent agent hangs
• P1: Collision detection — Init warns when other Bash PreToolUse hooks exist (informational, not blocking)
• P1: Multi-agent init — skim init --agent <name> with per-agent config dir resolution, script generation, and agent-aware uninstall
• P2: Session dedup — dedup_invocations() by (command_key, timestamp) across agents
• P2: Awareness tracking — SHA-256 hash manifest for awareness files, --force required if user modified
• P2: Agent-not-found — Helpful error with install hints when target agent isn't installed

Test plan

• cargo test --all-features — all pass (28 new tests)
• cargo clippy -- -D warnings — zero warnings
• cargo fmt -- --check — clean
• Manual: echo '{"tool_input":{"command":"cargo test"}}' | skim rewrite --hook → Claude JSON, empty stderr
• Manual: echo '{"tool_input":{"command":"cargo test"}}' | skim rewrite --hook --agent gemini → Gemini JSON, empty stderr
• Manual: echo '{"command":"cargo test"}' | skim rewrite --hook --agent cursor → Cursor JSON, empty stderr
• Manual: skim init --agent codex --yes → awareness-only message
• Manual: skim init --yes --dry-run → Claude backward compat
• Manual: skim agents --json → integrity field uses SHA-256

[dean0x]

Performance: Dedup allocates 2 strings per invocation unnecessarily (85% confidence)

dedup_invocations creates a format!("bash:{command}") String and clones a timestamp String for every invocation just to use as a HashSet key. For large session histories with thousands of invocations, this creates 2 heap allocations per invocation.

When only one provider is active (the common case), deduplication is unnecessary since cross-agent duplicates are impossible.

Fix: Short-circuit dedup for single-provider case:

if providers.len() > 1 {
    dedup_invocations(&mut all_invocations);
}

[dean0x]

Consistency: cache_dir() duplicated across two modules (92% confidence)

The cache_dir() function is identically duplicated in rewrite.rs:1307 and hook_log.rs:79. Both check SKIM_CACHE_DIR env then fall back to dirs::cache_dir()/skim.

This violates the DRY pattern seen elsewhere where shared helpers are centralized (e.g., has_skim_hook_entry is in init/state.rs and re-exported).

Fix: Make the hook_log.rs version pub(super) and import it from rewrite.rs, or extract to cmd/mod.rs.

[dean0x]

Consistency: MAX_SETTINGS_SIZE duplicated in two modules (90% confidence)

MAX_SETTINGS_SIZE: u64 = 10 * 1024 * 1024 is defined independently in agents.rs:364 and init/state.rs:10. The agents.rs doc comment even states "consistent with the guard in init/state.rs" yet defines its own copy.

Within init/, helpers.rs correctly references super::state::MAX_SETTINGS_SIZE, showing the project pattern for sharing constants.

Fix: Widen init/state.rs::MAX_SETTINGS_SIZE from pub(super) to pub(crate) and import it in agents.rs.

[dean0x]

Complexity: looks_like_error has 10 boolean conditions in final expression (85% confidence)

The return statement chains 10 OR-conditions including nested has_error (6 sub-conditions) and has_failed (with negated inner condition). Each check is simple but the composite requires careful reading. Adding new patterns is error-prone.

Fix: Refactor into named predicates or data-driven pattern table:

const ERROR_PATTERNS: &[&str] = &[
    "not found", "no such file", "permission denied",
    "command not found",
];

fn looks_like_error(content: &str) -> bool {
    let check = truncate_utf8(content, 1024);
    let lower = check.to_lowercase();
    has_error_prefix(&lower)
        || has_non_zero_failure(&lower)
        || ERROR_PATTERNS.iter().any(|p| lower.contains(p))
}```

[dean0x]

Complexity: run_hook_mode mixes 6 responsibilities (85% confidence)

This 116-line function handles: watchdog spawning, protocol dispatch, integrity checks, version checks, stdin reading, JSON parsing, command extraction, compound detection, rewriting, and response formatting. It has 6 return Ok(ExitCode::SUCCESS) exit paths and complex nesting.

Fix: Extract into phases:

fn run_hook_mode(agent: Option<AgentKind>) -> anyhow::Result<ExitCode> {
    spawn_watchdog();
    let (agent_kind, protocol) = resolve_protocol(agent)?;
    run_pre_hook_checks(agent_kind);
    let command = read_hook_command(&protocol)?;
    let rewritten = try_rewrite_command(&command);
    emit_hook_response(&protocol, &command, rewritten)
}```

[dean0x]

Rust: unwrap_or_default() silently swallows JSON parse errors (82% confidence)

serde_json::from_str(arguments_str).unwrap_or_default() silently creates Value::Null if JSON is malformed. If a tool call has corrupted arguments, the invocation will have empty fields, producing confusing results in discover/learn analysis.

Fix: Skip tool calls with malformed arguments:

let arguments: serde_json::Value = match serde_json::from_str(arguments_str) {
    Ok(v) => v,
    Err(_) => continue, // skip tool calls with malformed arguments
};

[dean0x]

Rust: Blanket #[allow(dead_code)] masks actual dead code (85% confidence)

The #[allow(dead_code)] annotation covers the entire HookProtocol trait with a comment explaining which methods are test-only vs production. This blanket suppression masks truly unused methods from compiler analysis.

The methods parse_input, format_response, and hook_support are called in production via protocol_for_agent() -- they are NOT dead code. Only agent_kind, generate_script, install, and uninstall are test-only.

Fix: Move #[allow(dead_code)] to individual methods that are genuinely only called in tests.

[dean0x]

Performance: Copilot hooks directory scan has no bound (83% confidence)

To detect whether skim is installed for Copilot, the code reads every .json file in .github/hooks/ without limit. There's no bound on the number of files scanned. A repository with many hook files could cause unbounded file I/O in the agents subcommand.

Fix: Add a bound and short-circuit on first match:

let has_skim_hook = std::fs::read_dir(hooks_dir).ok().is_some_and(|entries| {
    entries.flatten().take(50).any(|e| {
        e.path().extension().is_some_and(|ext| ext == "json")
            && std::fs::read_to_string(e.path())
                .ok()
                .is_some_and(|c| c.contains("skim"))
    })
});

[dean0x]

Tests: Hardcoded agent count assertion is fragile (90% confidence)

test_agents_json_has_six_entries asserts agents.len() == 6 with a magic number. When a 7th agent is added, this test fails with a cryptic error. The assertion couples the test to a count rather than testing meaningful behavior.

Furthermore, test_agents_lists_all_supported (line 131) already asserts the presence of all 6 agent names, making this count-only test redundant.

Fix: Either remove this test entirely (the existing test provides stronger coverage) or make it self-documenting:

let expected = ["claude-code", "cursor", "codex", "gemini", "copilot", "opencode"];
assert_eq!(agents.len(), expected.len(), "unexpected agent count change");

[dean0x]

Regression: skim_hook_version field changed from hardcoded to dynamic (85% confidence)

The skim_hook_version field in --suggest JSON output was hardcoded to "1.0.0" and is now env!("CARGO_PKG_VERSION"). This is arguably a bug fix (the hardcoded version was wrong), but it is a behavioral change in JSON output.

Action: This is correct behavior. Requires explicit sign-off that this is an intentional bug fix. Document in release notes.

[dean0x]

Regression: Stamp file path changed -- orphaned legacy file persists (82% confidence)

The version mismatch stamp file path changed from .hook-version-warned to .hook-version-warned-{agent_name}. Existing users who upgrade will have an orphaned .hook-version-warned stamp file that is never read or cleaned up.

This is not a functional regression (users get one extra warning on upgrade day), but the old file persists indefinitely.

Fix: Add one-time cleanup of the legacy stamp file:

// In check_hook_version_mismatch(), after computing stamp_path:
let legacy_stamp = stamp_path.with_file_name(".hook-version-warned");
let _ = std::fs::remove_file(&legacy_stamp);

[dean0x]

Wave/7 Code Review Summary

Reviewed 10 reports across 42 changed files (+10,768 / -1,244 lines). This PR introduces multi-agent support (6 agents) with session providers, hook protocols, integrity verification, and shell redirect support.

Overall Assessment

✅ APPROVED WITH CONDITIONS - All findings are non-blocking (no CRITICAL issues). The architecture is sound, test coverage is comprehensive, and the refactoring of the monolithic 1,047-line init.rs into focused sub-modules is exemplary.

Review Findings Consolidated

Blocking Issues (≥80% confidence):

• 17 inline comments created covering high-confidence findings across security, architecture, complexity, performance, consistency, and regression categories
• All findings are addressed with concrete, actionable fixes

High-Confidence Issue Counts:

Category	Issues	Confidence
Security	3	82-85%
Architecture	4	85-92%
Performance	2	83-85%
Complexity	3	85-92%
Consistency	2	90-92%
Rust	3	80-85%
Tests	1	90%
Regression	2	82-85%

Key Strengths

1. ✅ Zero unsafe code - Entire diff uses safe Rust exclusively
2. ✅ Error handling - All production code uses Result/? pattern, no unwrap() in business logic
3. ✅ Security mindset - Multiple layers of defense (size guards, symlink checks, bounded I/O, rate limiting)
4. ✅ Test coverage - 400+ tests passing with excellent behavior-focused integration tests
5. ✅ Module decomposition - init.rs refactor from 1,047 lines into 5 focused modules is exemplary
6. ✅ Strategy Pattern - HookProtocol and SessionProvider traits provide clean multi-agent abstraction
7. ✅ Performance - Meets <50ms target (14.6ms for 3000-line files, 3x faster than target)

Lower-Confidence Issues (60-79%)

The following findings from review reports are below the 80% threshold for inline comments but should be noted:

• Hash manifest written without restricted permissions (68%) - consider chmod 0600 on .sha256 files
• Copilot YAML deserialization without limits (62%) - serde_yaml_ng may lack DoS protections
• Agent CLI name unsanitized in path construction (65%) - add debug_assert on path separators
• Levenshtein watchdog thread spawns on every invocation (70%) - consider lazy spawn after stdin read
• Multiple test edge cases (65-70%) - provider neutralization duplication, test naming precision, rotation testing gaps

Backward Compatibility

✅ All major regressions verified as safe:

• skim init defaults to ClaudeCode (SAFE)
• skim rewrite --hook defaults to ClaudeCode (SAFE)
• skim learn migration logic included (cli-corrections.md → skim-corrections.md)
• Session dedup is additive (SAFE for single-agent deployments)

Note: Three behavioral changes require release notes documentation:

1. Hook warnings now route to ~/.cache/skim/hook.log (was stderr)
2. skim_hook_version now reports actual version (was hardcoded "1.0.0")
3. Rules filename changed to skim-corrections.md

Recommendation

Merge with condition: Address the 17 inline comments (concrete fixes provided for each). These are mechanical improvements that strengthen code quality without architectural changes. None block merge.

---

Review: Security (8/10), Architecture (7/10), Rust (8/10), Performance (8/10), Complexity (6/10), Consistency (8/10), Tests (8/10), Regression (8/10)

Generated by Claude Code review agent