feat: #105 skim pkg — package manager parsers

[dean0x]

Summary

• Adds skim pkg subcommand with four package manager parsers: npm, pnpm, pip, cargo audit
• Each parser follows the three-tier degradation pattern (JSON -> regex -> passthrough), consistent with existing test/build/git parsers
• Adds 17 rewrite rules for automatic command interception via skim rewrite

Changes

New Types

• PkgResult / PkgOperation canonical types in output/canonical.rs (Install, Audit, Outdated, Check, List operations)
• CommandType::Pkg variant in analytics module

Parsers (4 tools, 13 subcommands)

• npm: install, audit, outdated, ls — JSON tier 1 + regex tier 2
• pnpm: install (regex only), audit (JSON), outdated (JSON)
• pip: install (regex only), check (regex), list --outdated (JSON + regex)
• cargo: audit (JSON + regex)

Rewrite Rules (17 new rules)

• npm audit/install/i/ci/outdated/ls -> skim pkg npm ...
• pnpm audit/install/outdated -> skim pkg pnpm ...
• pip install/check/list -> skim pkg pip ...
• pip3 install/check/list -> skim pkg pip ...
• cargo audit -> skim pkg cargo audit
• Skip flags: --json, --format prevent rewrite when user already uses structured output

Tests

• 41 unit tests across all four parser modules
• 22 E2E pkg parser tests (tier coverage for each tool)
• 18 E2E rewrite tests (match/skip/alias coverage)
• 8 canonical type tests (display, serde roundtrip, ensure_rendered)
• 15 fixture files with realistic tool output

Test plan

• All existing tests pass (0 failures across 37 test suites)
• cargo clippy -- -D warnings clean
• cargo fmt -- --check clean
• Each parser tested at all applicable degradation tiers
• Rewrite skip flags verified (--json, --format)
• Help text displayed for skim pkg, skim pkg npm, etc.
• Unknown tool/subcommand produces clear error message

Closes #105

[dean0x]

Code Review: Architecture & Complexity Issues

1. Duplicated combine_output Helper (90% confidence)

Severity: BLOCKING - DRY Violation
Files: npm.rs:580, pip.rs:344, pnpm.rs:319, cargo.rs:277

The exact same combine_output function is copy-pasted across all 4 parser modules. Moreover, the existing pytest.rs:210 uses Cow<'_, str> to avoid unnecessary cloning when stderr is empty.

Fix: Extract a shared helper to pkg/mod.rs:

pub(super) fn combine_output(output: &CommandOutput) -> Cow<'_, str> {
    if output.stderr.is_empty() {
        Cow::Borrowed(&output.stdout)
    } else {
        Cow::Owned(format!("{}\n{}", output.stdout, output.stderr))
    }
}

Then import as super::combine_output in each submodule.

---

2. npm.rs Exceeds 700-Line Threshold (90% confidence)

Severity: BLOCKING - Complexity
File: npm.rs:1-789

At 789 lines, this file covers 4 subcommands (install, audit, outdated, ls) with 3 tiers each. It will grow further and become hard to maintain.

Fix: Split into:

• npm/mod.rs (dispatch)
• npm/install.rs (~120 lines)
• npm/audit.rs (~150 lines)
• npm/outdated.rs (~120 lines)
• npm/ls.rs (~80 lines)

---

3. Inconsistent Module Visibility (80% confidence)

Severity: SHOULD-FIX - Convention Deviation
File: pkg/mod.rs:6-9

The pkg module uses mod cargo (private) while test and build use pub(crate) mod. This deviates from established convention.

Fix:

pub(crate) mod cargo;
pub(crate) mod npm;
pub(crate) mod pip;
pub(crate) mod pnpm;

[dean0x]

Code Review: Correctness & Regression Issues

1. pip check Silently Drops User-Supplied Arguments (88% confidence)

Severity: BLOCKING - Correctness
File: pip.rs:146-147

The comment says "pip check takes no meaningful args, but pass them through" — yet the code contradicts this by never extending cmd_args with user arguments. Every other run_* function correctly does this.

Current code:

let mut cmd_args: Vec<String> = vec!["check".to_string()];
// Missing: cmd_args.extend(args.iter().cloned());

Fix:

let mut cmd_args: Vec<String> = vec!["check".to_string()];
cmd_args.extend(args.iter().cloned());  // Add this line
// pip check takes no meaningful args, but pass them through

---

2. Missing Rewrite Rules for npm list and pnpm i Aliases (82% confidence)

Severity: BLOCKING - Regression
File: cmd/rewrite.rs:276-320

The npm parser accepts both "ls" and "list" subcommands (npm.rs:63), and pnpm accepts both "install" and "i" (pnpm.rs:1920). However, the rewrite rules only register npm ls and pnpm install. This creates an asymmetry—npm has rules for i and ci aliases but pnpm doesn't have one for i.

Users running npm list or pnpm i won't get rewrites, breaking the tool's consistency.

Fix: Add two additional rules:

RewriteRule {
    prefix: &["npm", "list"],
    rewrite_to: &["skim", "pkg", "npm", "ls"],
    skip_if_flag_prefix: &["--json"],
    category: RewriteCategory::Pkg,
},
RewriteRule {
    prefix: &["pnpm", "i"],
    rewrite_to: &["skim", "pkg", "pnpm", "install"],
    skip_if_flag_prefix: &[],
    category: RewriteCategory::Pkg,
},

---

3. Vulnerability Totals Can Undercount Unknown Severity Levels (85% confidence)

Severity: BLOCKING - Data Correctness
Files: npm.rs:268-274, cargo.rs:184-190

The match severity arm has a catch-all _ => {} that silently skips unrecognized severity values (e.g., "info", "informational"). The total is computed as the sum of critical + high + moderate + low, meaning vulnerabilities with unknown severity are added to details but not counted in total. This causes a mismatch where details.len() exceeds total.

Fix: Compute total from details length instead of summing buckets:

let total = details.len();

Or add a catch-all counter:

let mut info: usize = 0;
// in match:
_ => info += 1,
// final total:
let total = critical + high + moderate + low + info;

[dean0x]

Code Review: Test Coverage & Performance Issues

1. Missing Unit Tests for 16 New Rewrite Rules (90% confidence)

Severity: BLOCKING - Test Coverage Gap
File: cmd/rewrite.rs:140-323 (test section at line 1554)

The PR adds 16 new RewriteCategory::Pkg rules but the #[cfg(test)] mod tests block has zero unit-level try_rewrite() tests for them. Every other rule category (Test, Build, Git, Read) has corresponding unit tests. The E2E tests cover 15/16 rules but the pnpm i alias has no test coverage at any level.

The comment was updated from "all 15 rules" to "all 32 rules" but no matching unit tests were added—this is misleading.

Fix: Add unit tests following the existing pattern:

#[test]
fn test_npm_audit() {
    let result = try_rewrite(&["npm", "audit"]).unwrap();
    assert_eq!(result.tokens, vec!["skim", "pkg", "npm", "audit"]);
}

#[test]
fn test_npm_i() {
    let result = try_rewrite(&["npm", "i", "express"]).unwrap();
    assert_eq!(result.tokens, vec!["skim", "pkg", "npm", "install", "express"]);
}

#[test]
fn test_npm_list() {
    let result = try_rewrite(&["npm", "list"]).unwrap();
    assert_eq!(result.tokens, vec!["skim", "pkg", "npm", "ls"]);
}

#[test]
fn test_pnpm_i() {
    let result = try_rewrite(&["pnpm", "i"]).unwrap();
    assert_eq!(result.tokens, vec!["skim", "pkg", "pnpm", "install"]);
}

// Add similar tests for: npm ci, npm install, npm outdated, pnpm audit, pnpm outdated,
// pip install, pip check, pip list, pip3 install, pip3 check, pip3 list, cargo audit

Also add category validation:

#[test]
fn test_pkg_category_for_npm_audit() {
    let result = try_rewrite(&["npm", "audit"]).unwrap();
    assert_eq!(result.category, RewriteCategory::Pkg);
}

---

2. Cargo Audit Unnecessarily Clones Entire Vulnerability List (82% confidence)

Severity: BLOCKING - Performance
File: cargo.rs:150

The code does .and_then(|v| v.as_array()).cloned().unwrap_or_default() which deep-clones the entire Vec<serde_json::Value> of vulnerabilities. For large audit reports, this is wasteful.

Current code:

let list = vulns
    .get("list")
    .and_then(|v| v.as_array())
    .cloned()
    .unwrap_or_default();

Fix: Use a reference instead:

let empty = vec![];
let list = vulns
    .get("list")
    .and_then(|v| v.as_array())
    .unwrap_or(&empty);

---

3. Dead Code: split_first() Guard After is_empty() Check (92% confidence)

Severity: SHOULD-FIX - Code Quality
Files: cargo.rs:48-51, npm.rs:55-58, pip.rs:43-46, pnpm.rs:40-43

Every run() function first checks args.is_empty() and returns. Then immediately: let Some((subcmd, subcmd_args)) = args.split_first() else { ... }. Since the empty case was already handled, this else block is unreachable dead code.

Current pattern:

if args.is_empty() {
    return Ok(ExitCode::FAILURE);
}
let Some((subcmd, subcmd_args)) = args.split_first() else {
    eprintln!("..."); // This is unreachable!
    return Ok(ExitCode::FAILURE);
};

Fix: Remove the redundant guard or restructure to single check:

let Some((subcmd, subcmd_args)) = args.split_first() else {
    eprintln!("...");
    return Ok(ExitCode::FAILURE);
};

---

4. combine_output Allocation Inefficiency (85% confidence)

Severity: SHOULD-FIX - Performance
Files: npm.rs:582, pip.rs:346, pnpm.rs:321, cargo.rs:279

Each combine_output function unconditionally clones output.stdout when stderr is empty. The common path wastes an allocation. This is already solved in pytest.rs:210 with Cow<'_, str>.

(This overlaps with the duplication issue—once consolidated into a single helper, use Cow for the return type as shown in comment #1.)

[dean0x]

Summary: Code Review Findings

Blocking Issues (Must Fix Before Merge)

Issue	Confidence	Count	Category
Duplicated combine_output helper	90%	4 files	Architecture/DRY
pip check drops user arguments	88%	1 location	Correctness
Missing rewrite rule aliases	82%	2 rules	Regression
Vulnerability total undercounts	85%	2 locations	Data Correctness
npm.rs file length (789 lines)	90%	1 file	Complexity
Missing unit tests for 16 rewrite rules	90%	rewrite.rs tests	Test Coverage

Total Blocking: 6 issues across architecture, correctness, regression, and test coverage.

---

Should-Fix Issues (Recommended)

Issue	Confidence	Impact
Cargo audit clones vulnerability list unnecessarily	82%	Performance regression
Dead code guards (split_first() after is_empty())	92%	Code clarity (4 locations)
Module visibility deviates from convention	80%	Internal consistency

---

Lower-Confidence Suggestions (60-79%)

These appeared in only 1-2 review reports and can be addressed separately or in a follow-up:

• npm via array can contain strings (not just objects) — graceful degradation works but could be more robust
• Double iteration in regex fallback parsers (npm outdated, pip check/list) — O(2n) instead of O(n), negligible on typical output
• --json flag handling inconsistency in pkg dispatcher vs test/build — architectural consistency question
• Rule count comment may drift — consider compile-time assertion

---

Consolidated Blockers by Priority

Priority 1 (Data Correctness & API Contracts):

1. ✅ Fix pip check dropped args (1 line change)
2. ✅ Fix vulnerability total undercount (1-2 lines, multiple locations)
3. ✅ Add missing rewrite rule aliases (4 lines total)

Priority 2 (Code Quality & Maintenance):
4. ✅ Extract combine_output to pkg/mod.rs (5 lines, 4 deletions)
5. ✅ Split npm.rs into submodules (reorganization)
6. ✅ Add 16 unit tests for rewrite rules (~50 lines in tests block)

Priority 3 (Recommended Cleanup):
7. Remove dead code guards in run() functions (4 locations)
8. Change module visibility to pub(crate) (4 lines)

---

Review Coverage

• Architecture: 6 independent reviews ✓
• Security: Clean (9/10) - no secrets, proper input caps
• Performance: 8/10 - minor allocations in common paths
• Complexity: 6/10 - file organization needed (npm.rs split)
• Consistency: 7/10 - good patterns, minor deviations
• Regression: 9/10 - no existing APIs broken, 2 new gaps in feature parity
• Tests: 7/10 - unit tests added for parsers, but rewrite rules untested
• Rust: 7/10 - good idioms, data correctness issues in edge cases

Overall Recommendation: CHANGES_REQUESTED

The three-tier degradation pattern, rewrite rule integration, and canonical output types are well-architected. The issues are fixable: dropped arguments, undercounting totals, missing aliases, code duplication, and test gaps. None are design problems—just implementation details.

---

Suggested Merge Checklist

• Extracted combine_output to pkg/mod.rs (with Cow optimization)
• Fixed pip check to extend user args
• Fixed vulnerability total to not undercount unknown severity
• Added missing npm list and pnpm i rewrite aliases
• Split npm.rs into npm/{mod,install,audit,outdated,ls}.rs
• Added unit tests for all 16 new rewrite rules + category tests
• Removed dead code guards in run() functions (optional but recommended)
• Changed pkg submodule visibility to pub(crate) (optional but consistent)

Review completed with 8 independent report analysis and 100% deduplication of findings.

[dean0x]

Type Honesty Issue: The function signature promises Option<(String, &str)> but always returns Some(...) (85% confidence).

All code paths end with Some(...) and the doc comment correctly notes "this always returns Some in practice". The Option return type is misleading.

Suggested fix: Change return type to (String, &str) since it cannot fail:

fn extract_vuln_detail(vuln: &serde_json::Value) -> (String, &str) {
    // ... same body ...
    (detail, severity)
}

Update the call site to a simple let (detail, severity) = extract_vuln_detail(vuln);

---

Comment from code review (claude.ai/code)

[dean0x]

Unsafe Type Conversion: Three lines cast u64 to usize via as usize (82% confidence).

On 32-bit targets, usize is 32 bits and values above u32::MAX would silently truncate. While package counts this large are unrealistic, the as cast is a canonical Rust footgun for silent data loss.

Suggested fix: Use try_from with fallback:

let added = value.get("added")
    .and_then(|v| v.as_u64())
    .and_then(|v| usize::try_from(v).ok())
    .unwrap_or(0);

Apply the same pattern to all three u64 -> usize conversions (lines 74-76).

---

Comment from code review (claude.ai/code)

[dean0x]

Security: Terminal Escape Injection: User-controlled input is echoed directly to stderr without sanitization (80% confidence).

When an unrecognized subcommand is provided, the tool name is echoed via eprintln!("... unknown tool '{tool}' ..."). A malicious argument containing ANSI escape sequences could be used for terminal injection (e.g., change terminal title, clear screen on vulnerable emulators).

This is limited risk for a local CLI tool, but violates defense-in-depth principles.

Suggested fix: Sanitize non-printable characters before echoing:

let sanitized: String = tool.chars()
    .map(|c| if c.is_ascii_graphic() || c == ' ' { c } else { '?' })
    .collect();
eprintln!("skim pkg: unknown tool '{sanitized}'");

Apply to all unknown-subcommand error messages in this file and related dispatchers.

---

Comment from code review (claude.ai/code)

[dean0x]

Code Review Summary: feat-105-pkg-parsers

Overview

This PR adds comprehensive package manager support to skim (skim pkg) with four tools (npm, pnpm, pip, cargo audit) using a clean three-tier degradation pattern. Excellent architecture and thorough test coverage. Three high-confidence issues and several medium-confidence suggestions below.

---

High-Confidence Blockers (≥80%)

1. Missing unit tests for extract_json_flag and combine_output (Tests: 85%)

• Location: crates/rskim/src/cmd/pkg/mod.rs:58-77
• Issue: Two critical helper functions have no dedicated unit tests. extract_json_flag controls argument routing across all four package managers; combine_output has two code paths (Cow::Borrowed vs Cow::Owned).
• Fix: Add unit tests covering both functions, including edge cases for flag positioning and stderr merging.

2. Missing three-tier integration tests for npm outdated/ls (Tests: 82%)

• Location: crates/rskim/src/cmd/pkg/npm/{outdated,ls}.rs
• Issue: All other parsers have explicit three-tier tests validating the dispatch between JSON/regex/passthrough tiers. npm outdated and npm ls are missing these integration tests.
• Fix: Add three-tier tests following the same pattern as npm audit/install.

3. Cargo env var inconsistency (Consistency: 82%)

• Location: crates/rskim/src/cmd/pkg/cargo.rs:95
• Issue: Uses ("NO_COLOR", "1") but existing cargo invocations in test/cargo.rs and build/cargo.rs use ("CARGO_TERM_COLOR", "never").
• Fix: Change to ("CARGO_TERM_COLOR", "never") for consistency.

4. Repetitive run_* boilerplate (Complexity: 85%)

• Location: All 10 run_* functions across npm/pip/pnpm/cargo
• Issue: Every function follows identical 15-20 line pattern: build cmd_args, inject JSON flag, determine use_stdin, call run_parsed_command_with_mode. Makes adding new tools cumbersome.
• Fix: Extract a shared run_pkg_subcommand() helper in pkg/mod.rs that encapsulates this pattern, reducing each call site from 20 lines to 5.

5. Cargo audit triple-regex-pass (Performance: 82%)

• Location: crates/rskim/src/cmd/pkg/cargo.rs:239-250
• Issue: try_parse_audit_regex runs three separate captures_iter passes over the same input (RE_CRATE, RE_ADVISORY_ID, RE_TITLE), then zips by index. This scans the input 3x and has a fragile alignment assumption.
• Fix: Single-pass block-splitting approach: split on "Crate:" markers and extract all fields from each block in one pass.

6. Inconsistent module structure (Architecture: 82%)

• Location: crates/rskim/src/cmd/pkg/
• Issue: npm is split into submodules (audit.rs, install.rs, ls.rs, outdated.rs with a dispatching mod.rs) while cargo (419 lines), pip (472 lines), and pnpm (427 lines) use monolithic files. npm split provides better concerns separation but creates two patterns in one feature.
• Fix: Split pip and pnpm into submodules matching the npm pattern. This is non-blocking but should be addressed before the module grows further.

---

Medium-Confidence Findings (60-79%)

Test Helpers Duplication: The fixture_path() and load_fixture() helpers are copy-pasted identically in 7 test modules. Consider extracting to a shared #[cfg(test)] mod test_helpers in pkg/mod.rs.

Missing Edge Case Tests:

• npm audit JSON with unknown severity values (should count in total, not in any bucket)
• pnpm audit passthrough tier (only has JSON + passthrough, no regex tier)
• Full coverage for E2E negative-path subcommand dispatchers

Pre-Existing Issues (informational):

• rewrite.rs now 2,938 lines (was 2,641) — well past 500-line threshold
• canonical.rs now 781 lines (was 534) — past 500-line threshold
• Two error handling patterns for unknown subcommands across dispatchers
• Module visibility inconsistency (pkg keeps submodules private vs test/build use pub(crate))

---

Strengths

✅ Excellent architecture: Strategy pattern with consistent dispatcher design across all four tools
✅ Three-tier degradation: Well-implemented fallback chain (JSON → regex → passthrough)
✅ Comprehensive test coverage: 63 new tests (34 unit + 7 canonical + 22 E2E) with good fixture variety
✅ Canonical types: Follows existing PkgResult/PkgOperation patterns from TestResult/GitResult
✅ Dependency direction: Clean inward-pointing dependencies, no circular imports
✅ Rewrite rule integration: 19 new rules properly integrated with longest-prefix-first ordering preserved

---

Recommendation

APPROVED WITH CONDITIONS: Address the 6 high-confidence findings before merge. Most are straightforward fixes (unit tests, consistency updates, helper extraction). The architecture is solid and follows established patterns well.

---

Review generated by claude.ai/code

[dean0x]

Merged via consolidation PR #108