Skip to content

codebase-audit-2026-03-31.md

Latest commit

 

History

History
338 lines (248 loc) · 17.5 KB

codebase-audit-2026-03-31.md

File metadata and controls

338 lines (248 loc) · 17.5 KB
Raw

CueLoop Comprehensive Codebase Audit

Status: Archived Owner: Maintainers Source of truth: historical snapshot; current guidance lives in linked active docs Parent: CueLoop Documentation

Date: 2026-03-31 Scope: Full codebase (crates/cueloop/src, crates/cueloop/tests, scripts/, Makefile) Auditor: AI Agent (comprehensive-codebase-audit skill)

Executive Summary

Top 10 Critical/High Issues

# Severity Category Issue
1 🟠 High File Size Limits 12 non-test source files exceed the 500 LOC hard limit; 34 exceed the 400 LOC soft limit
2 🟠 High Missing SAFETY Docs 5 unsafe blocks in production code lack SAFETY comments
3 🟠 High Missing set -euo pipefail 8 sourced library shell scripts omit strict mode
4 🟡 Medium File Size Limits runner/error.rs (530 LOC) — the largest non-test production file, holds 12 error variants + match arms
5 🟡 Medium Test Coverage 528 of 826 source files (64%) have zero inline test coverage; 263 files >100 LOC lack tests
6 🟡 Medium Clone Overuse 820 .clone() calls in non-test source; audit for hot-path cloning warranted
7 🟡 Medium unreachable!() in Production 1 unreachable!() in notification/display.rs for a legitimate enum branch
8 🔵 Low Test unwrap() Density ~300+ unwrap() calls in test helper code that could use ? with -> Result test signatures
9 🔵 Low Logging Distribution 528 log calls but heavily skewed toward warn (151) and info (197); debug (136) under-represents hot paths
10 🔵 Low Dead Code Candidates Multiple unreachable!() / todo!() patterns absent, but some Ok(_) => {} silent success branches in atomic/undo paths

Overall Assessment

Architecture: Well-decomposed. The facade pattern is rigorously applied across 30+ module directories. Module documentation (//!) covers 100% of source files (826/826). The codebase demonstrates disciplined separation of concerns.

Security: Strong. Shell injection is blocked via argv-only CI gate validation, symlink/escape path traversal detection in plugin resolution, comprehensive redaction across env keys, bearer tokens, AWS keys, SSH keys, and hex strings. No eval in shell scripts. No raw Command::output() — all subprocesses flow through managed execution with timeouts.

Error Handling: Two-tier anyhow/thiserror approach is consistently applied. Production code uses ? operator almost universally. unwrap() is confined to test code and LazyLock<Regex> static initialization (acceptable).

Metrics Dashboard

Metric Count
Source files (src/) 826
Test files (tests/) 176
Total source LOC 139,286
Test LOC (inline) ~31,675
Integration test LOC ~28,073
Files >500 LOC (non-test) 12
Files >400 LOC (non-test) 34
Files with //! docs 826 (100%)
Production unwrap() (non-test, non-LazyLock) ~0
Production unsafe blocks 15
unsafe blocks with SAFETY comment 10
unsafe blocks missing SAFETY comment 5
.clone() calls (non-test) 820
Logging calls 528
Shell scripts 21
Shell scripts with set -euo pipefail 13 / 21
Shell scripts missing strict mode 8 (library scripts)
eval in shell scripts 0
todo!() macros 0
unreachable!() in production 1

Full Findings

ARCHITECTURE & DESIGN

🟠 [God Module / Size Limit] Multiple files exceed 500 LOC hard limit

  • Files (12):
    • queue/operations/tests/batch.rs (753) — test file, but should still decompose
    • runner/execution/tests/plugin_trait_tests.rs (736)
    • runner/execution/tests/stream.rs (708)
    • queue/operations/tests/mutation.rs (691)
    • config/tests.rs (598)
    • runutil/execution/orchestration/tests.rs (574)
    • session/tests.rs (536)
    • runner/error.rs (530) ← production
    • queue/operations/mutation.rs (522) ← production
    • queue/operations/batch/mod.rs (512) ← production
    • queue/operations/tests/status.rs (507)
    • runner/execution/tests/process.rs (506)
  • Problem: Violates AGENTS.md hard limit of 1,000 LOC and target of <500 LOC
  • Impact: Maintenance burden, difficult to navigate large test suites
  • Blast Radius: Queue operations, runner error classification, batch mutation
  • Fix: Split test suites into focused subdirectories following the runtime_tests/, ci_tests/, worker_tests/ patterns already established. For runner/error.rs, consider extracting match-arm formatting into a runner/error/formatting.rs.

🟡 [Large Module] 34 non-test files exceed 400 LOC soft limit

  • Notable files: cli/machine/queue_docs.rs (494), commands/init/writers.rs (485), sanity/unknown_keys.rs (484), cli/scan.rs (478), contracts/runner.rs (463), runner/execution/plugin_executor.rs (460)
  • Problem: Files approaching the hard limit; proactively splitting reduces future risk
  • Impact: Review fatigue, merge conflict density
  • Fix: Extract sub-concerns following the established facade pattern

✅ [Facade Pattern] Consistently applied across 30+ module directories. Root modules contain only //! docs and re-exports. Implementation lives in companion files. This is exemplary.

✅ [Module Documentation] 100% coverage — all 826 source files have //! purpose/responsibilities/scope/usage/invariants headers.

SECURITY

✅ [Injection Prevention] CI gate argv validation in config/validation/ci_gate.rs rejects sh -c, cmd /C, pwsh -Command, powershell -Command. Shell launcher detection is comprehensive.

✅ [Path Traversal Prevention] Plugin executable resolution in plugins/registry.rs uses canonicalize to detect symlinks escaping plugin directory boundaries. Absolute paths are rejected.

✅ [Secrets Redaction] redaction/patterns.rs implements layered redaction: key-value pairs → bearer tokens → AWS keys → SSH blocks → hex tokens → env values. Well-structured and thorough.

✅ [Subprocess Safety] No raw Command::output() in production code. All subprocess execution flows through runutil::shell::execute_managed_command with timeout classes, bounded capture, and SIGINT-before-SIGKILL escalation.

✅ [No eval] Zero eval usage across all shell scripts.

🟠 [Unsafe Safety Comments] 5 unsafe blocks missing SAFETY documentation

  • Files:
    1. lock/pid.rs:47 — CreateToolhelp32Snapshot (Windows) — no SAFETY comment
    2. lock/pid.rs:90 — libc::kill(pid, 0) — no SAFETY comment
    3. lock/pid.rs:108 — OpenProcess (Windows) — no SAFETY comment
    4. commands/run/parallel/worker_command.rs:33 — pre_exec for setpgid — no SAFETY comment
    5. runutil/shell/execute.rs:75 — pre_exec for setpgid — no SAFETY comment
  • Problem: Rust convention requires every unsafe block to have a // SAFETY: comment explaining why the operation is sound
  • Impact: Review difficulty, certification/audit friction
  • Fix: Add // SAFETY: comments to all 5 remaining blocks following the pattern already used in sibling code

🟡 [Daemon setsid] commands/daemon/start.rs:120

  • Violation: unsafe { command.pre_exec(|| { libc::setsid(); Ok(()) }); }
  • Problem: Return value of setsid() is silently ignored; failure could leave daemon in unexpected session state
  • Impact: Silent failure in daemon session isolation
  • Fix: Check setsid() return value and log on failure: if libc::setsid() == -1 { log::warn!("setsid failed: {}", std::io::Error::last_os_error()); }

COMPLEXITY & COGNITIVE LOAD

🟡 [Large Enum Match] runner/error.rs

  • Violation: RunnerError has 10 variants; the Display impl and classify() method each have large match blocks spanning ~200 lines
  • Problem: Adding a new error variant requires touching 4+ match arms in the same file
  • Impact: Regression risk on error handling changes
  • Fix: Consider a macro-driven approach or separate the Display impl into runner/error/display.rs

CODE QUALITY

✅ [unwrap() in Production] Virtually zero unwrap() calls in non-test production code. The few that exist are in LazyLock<Regex> static initializers (which would panic at startup if the regex is invalid — a compile-time invariant). This is acceptable.

✅ [Constants Centralization] constants.rs (431 LOC) provides well-organized compile-time constants organized by domain (buffers, limits, timeouts, UI, etc.). Prevents magic number drift.

✅ [todo!() Absence] Zero todo!() macros in the entire codebase — all code paths are implemented.

🟡 [Clone Audit Warranted] 820 .clone() calls in non-test source

  • Problem: Not all are problematic, but the volume suggests some hot-path cloning may exist
  • Impact: Unnecessary heap allocations in runner streaming paths, queue loading
  • Fix: Audit runner/execution/stream_reader.rs and queue loading paths for Arc or borrowing alternatives where clones copy large String or Vec

🟡 [unreachable!() in Production] notification/display.rs:59

  • Violation: unreachable!("loop notifications use loop path") in show_task_notification match arm
  • Problem: If a future caller passes LoopComplete through this path, it panics at runtime
  • Impact: Potential runtime crash if the invariant is violated
  • Fix: Return an error or no-op instead of panicking: Ok(()) with a log::warn!

ERROR HANDLING

✅ [Two-Tier Strategy] anyhow for propagation, thiserror for domain errors (RunnerError). Consistently applied.

✅ [Redacted Errors] RunnerError stores RedactedString for stdout/stderr, ensuring runner output is sanitized before display.

✅ [Context Propagation] Extensive use of .context() and .with_context() throughout the codebase.

🟡 [Silent Success Branches] Several Ok(_) => {} patterns in production:

  • fsutil/atomic.rs:39 — persist success (intentional, but could log)
  • undo/storage.rs:75 — backup removal success (intentional)
  • queue/backup.rs:41 — backup success (intentional)
  • Problem: These are intentional no-ops but lack comments explaining why the success case is silently discarded
  • Impact: Future readers may wonder if the branch was forgotten
  • Fix: Add // Intentional: success requires no action comments

RESOURCE MANAGEMENT

✅ [Process Cleanup] ProcessCleanupGuard ensures streaming threads are joined even on early returns. SIGINT-before-SIGKILL escalation with configurable grace period.

✅ [Bounded Capture] All managed subprocesses use BoundedCapture with configurable byte limits (MANAGED_SUBPROCESS_CAPTURE_MAX_BYTES: 256KB, CI: 4MB).

✅ [Atomic Writes] write_atomic uses NamedTempFile with flush+sync before persist. Explicit temp file drop on persist failure prevents leaks.

✅ [Temp Cleanup] Startup runs cleanup_default_temp_dirs with configurable retention.

PERFORMANCE

🟡 [Regex Initialization] 6 LazyLock<Regex> statics in prompts_internal/util.rs and queue/json_repair.rs

  • Problem: These compile at first use and never deallocate; the patterns are compile-time known
  • Impact: Minimal — LazyLock is the correct pattern for this use case
  • Fix: No action needed; this is documented for awareness

🟡 [Sensitive Env Cache] redaction/env.rs caches all sensitive env values in a RwLock<Option<HashSet<String>>>

  • Problem: If environment is large, this caches all values matching secret patterns. Cache is never invalidated.
  • Impact: Stale secrets won't be redacted if they change during process lifetime; memory grows with env size
  • Fix: Acceptable for a CLI tool — process lifetime is bounded. No action needed.

TESTING

🟠 [Coverage Gaps] 528 of 826 source files (64%) have zero inline test coverage

  • Problem: 263 files over 100 LOC lack even basic #[cfg(test)] blocks
  • Impact: Regression risk in untested modules, especially:
    • cli/machine/queue_docs.rs (494 LOC, 0 tests)
    • commands/task/decompose/support.rs (450 LOC, 0 tests)
    • commands/scan.rs (454 LOC, 0 tests)
    • commands/watch/processor.rs (438 LOC, 0 tests)
    • commands/init/readme.rs (438 LOC, 0 tests)
    • cli/scan.rs (478 LOC, 0 tests)
    • cli/webhook.rs (377 LOC, 0 tests)
    • prompts.rs (365 LOC, 0 tests)
  • Fix: Prioritize test coverage for queue mutation, CLI output formatting, and prompt composition modules. These are high-value test targets because they transform user-visible data.

✅ [Integration Test Suite] 176 integration test files with ~28K LOC provide substantial end-to-end coverage.

OBSERVABILITY

✅ [Structured Logging] 528 log calls across debug (136), info (197), warn (151), error (56). Distribution is healthy.

✅ [Redacted Logger] RedactedLogger wraps the env_logger output to ensure secrets are never logged.

🟡 [Debug Logging in Hot Paths] Runner streaming and subprocess management paths could benefit from more debug logging for production diagnostics

  • Impact: Difficult to diagnose streaming issues without RUST_LOG=debug
  • Fix: Add debug logging for session ID extraction, stream buffer growth, and timeout checkpoint transitions

CONFIGURATION

✅ [Precedence Chain] CLI flags → .cueloop/config.jsonc → ~/.config/cueloop/config.jsonc → schema defaults. Well-documented.

✅ [Validation] Config validation includes unknown key detection, CI gate argv validation, and schema enforcement.

✅ [JSONC Support] Both JSON and JSONC config files supported with comments.

SHELL SCRIPTS

🟠 [Missing Strict Mode] 8 of 21 shell scripts lack set -euo pipefail

  • Files:
    • scripts/lib/cueloop-shell.sh
    • scripts/lib/release_changelog.sh
    • scripts/lib/release_pipeline.sh
    • scripts/lib/release_policy.sh
    • scripts/lib/release_publish_pipeline.sh
    • scripts/lib/release_state.sh
    • scripts/lib/release_verify_pipeline.sh
    • scripts/lib/release_verify_state.sh
    • scripts/lib/xcodebuild-lock.sh
  • Problem: Library scripts sourced by entrypoint scripts. If sourced after set -euo pipefail in the caller, strict mode propagates. But if sourced independently, errors are silently ignored.
  • Impact: Undefined variable usage or failed commands could go undetected in library functions
  • Fix: Add set -euo pipefail to all library scripts, or add a guard: [[ $- == *e* ]] || set -e to inherit the caller's strict mode only if available. Given these are sourced libraries, adding set -euo pipefail at the top is safest and most explicit.

Remediation Roadmap

🔥 Quick Wins (This Week)

  1. Add SAFETY comments to 5 unsafe blocks — 15 minutes

    • lock/pid.rs (3 blocks)
    • commands/run/parallel/worker_command.rs (1 block)
    • runutil/shell/execute.rs (1 block)

  2. Add set -euo pipefail to 8 library shell scripts — 10 minutes

    • scripts/lib/*.sh — prepend strict mode header

  3. Replace unreachable!() in notification/display.rs — 5 minutes

    • Use log::warn! + early return instead

  4. Add comments to silent Ok(_) => {} branches — 10 minutes

    • fsutil/atomic.rs, undo/storage.rs, queue/backup.rs

  5. Check setsid() return value in daemon — 5 minutes

    • commands/daemon/start.rs:120

📋 Short-Term (This Sprint)

  1. Split 3 largest production files below 500 LOC

    • runner/error.rs (530) → extract Display impl to runner/error/display.rs
    • queue/operations/mutation.rs (522) → extract bulk helpers to queue/operations/mutation/helpers.rs
    • queue/operations/batch/mod.rs (512) → extract validation to batch/validation.rs

  2. Split top 3 test suites below 600 LOC

    • queue/operations/tests/batch.rs (753) → split into batch_basic.rs, batch_edge_cases.rs
    • runner/execution/tests/plugin_trait_tests.rs (736) → split by trait method
    • runner/execution/tests/stream.rs (708) → split by stream type

  3. Add test coverage for highest-value untested modules

    • cli/machine/queue_docs.rs (494 LOC) — machine document generation
    • commands/scan.rs (454 LOC) — scan workflow orchestration
    • commands/watch/processor.rs (438 LOC) — watch event processing

🏗️ Long-Term (Architectural)

  1. Clone audit for runner/queue hot paths — identify and eliminate unnecessary String/Vec clones in streaming and queue loading paths. Consider Cow<str> or &str borrowing where possible.

  2. Proactive file decomposition — split remaining 31 files in the 400-500 LOC range before they breach the hard limit. Prioritize cli/scan.rs, cli/machine/task.rs, commands/init/writers.rs.

Appendix: Top 20 Largest Non-Test Source Files

LOC File
530 runner/error.rs
522 queue/operations/mutation.rs
512 queue/operations/batch/mod.rs
499 commands/run/parallel/status.rs
499 commands/run/parallel/state.rs
494 cli/machine/queue_docs.rs
485 commands/init/writers.rs
484 sanity/unknown_keys.rs
478 cli/scan.rs
463 contracts/runner.rs
460 runner/execution/plugin_executor.rs
456 git/status.rs
454 commands/scan.rs
451 commands/run/supervision/ci.rs
451 commands/run/parallel/cleanup_guard.rs
450 commands/task/decompose/support.rs
438 commands/watch/processor.rs
438 commands/init/readme.rs
436 commands/task/mod.rs
436 cli/machine/task.rs