From 54e52cb4b36ab0d382c9813237b623220613b98e Mon Sep 17 00:00:00 2001 From: wan9chi Date: Sat, 20 Jun 2026 10:32:06 +0800 Subject: [PATCH] docs: add codebase assessment --- CODEBASE_ASSESSMENT.md | 294 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 294 insertions(+) create mode 100644 CODEBASE_ASSESSMENT.md diff --git a/CODEBASE_ASSESSMENT.md b/CODEBASE_ASSESSMENT.md new file mode 100644 index 00000000..0089c3d4 --- /dev/null +++ b/CODEBASE_ASSESSMENT.md @@ -0,0 +1,294 @@ +# Codebase Assessment + +## Executive summary + +The architecture is fundamentally strong: workspace discovery, task graph construction, execution planning, and runtime scheduling/cache are cleanly layered. Typed paths and graph invariants reduce invalid states, and the repository has substantial plan and end-to-end snapshot coverage. + +The largest risks are concentrated in filesystem tracing, cache correctness, nested concurrency, cache durability, and cross-platform filesystem semantics. The highest-value work is to tighten these existing boundaries rather than rewrite the architecture. + +## Highest-priority improvements + +### 1. Fix variadic `exec*` argument construction immediately + +The heap branch allocates `argc` pointers but writes the null terminator at `out[argc]`. The stack branch passes uninitialized elements through a typed slice. This can abort intercepted processes and invokes undefined behavior. + +Allocate `argc + 1`, expose only the initialized `[..=argc]` region, and add tests around the 31/32-argument boundary under sanitizers or Miri. + +Reference: `crates/fspy_preload_unix/src/interceptions/spawn/exec/with_argv.rs:28-58` + +### 2. Make external executable changes invalidate cache entries + +Programs outside the workspace are fingerprinted only by basename. Changing `PATH` or upgrading `node`, a compiler, package manager, or another global tool can therefore reuse output generated by a different executable. + +Include the resolved executable path and a session-cached content or stable metadata fingerprint. Include interpreter chains for scripts. + +Reference: `crates/vite_task_plan/src/cache_metadata.rs:122-130` + +### 3. Define concurrency as global or per nested graph + +Documentation presents the concurrency limit as an upper bound, but every nested execution graph creates an independent semaphore. Total leaf-process concurrency can consequently exceed the configured limit. + +Prefer one shared execution budget across the expanded execution tree. Add end-to-end tests for nested runs with limits 1, 2, the default, explicit nested limits, and unlimited mode. If per-level concurrency is intentional, document it prominently and avoid describing the setting as a global upper bound. + +References: + +- `docs/concurrency.md:3-16` +- `crates/vite_task/src/session/execute/scheduler.rs:67-85` +- `crates/vite_task/src/session/execute/scheduler.rs:149-168` + +### 4. Bound IPC frame sizes + +A child can submit a `u32` frame length that causes the runner to attempt a multi-gigabyte allocation. + +Add a small protocol maximum before resizing, reject zero and oversized frames, add malformed-frame tests, and introduce protocol version or capability negotiation. + +Reference: `crates/vite_task_server/src/lib.rs:481-488` + +### 5. Correct metadata and symlink-sensitive cache tracking + +`stat`, `lstat`, and no-follow accesses collapse into ordinary content reads. Tasks observing timestamps, permissions, ownership, executable bits, or symlink identity can receive incorrect cache hits. + +Preserve access semantics through fspy and fingerprint the observable metadata or link target appropriate to each operation. + +References: + +- `crates/fspy_shared/src/ipc/mod.rs:34-38` +- `crates/fspy_preload_unix/src/interceptions/stat.rs:19-30` +- `crates/vite_task/src/session/execute/fingerprint.rs:388-438` + +### 6. Remove the unconditional `dist` fingerprint exclusion + +Directory fingerprints ignore any entry named `dist`, even when it is a legitimate task input. A task that enumerates a directory can therefore receive a stale cache hit when that entry is added or removed. + +Derive exclusions from resolved task configuration or explicit runner-aware ignore requests instead of a global filename rule. + +Reference: `crates/vite_task/src/session/execute/fingerprint.rs:383-386` + +## Cache durability and fidelity + +### 7. Make cache publication transactional and self-healing + +Recommended changes: + +- Write archives to temporary files and atomically rename them after successful completion. +- Wrap related cache-entry and task-key updates in one SQLite transaction. +- On a missing, truncated, or corrupt archive, invalidate the entry and execute the task instead of failing the invocation. +- Reclaim orphaned archives with bounded cleanup. +- Add multi-process stress and fault-injection tests around every publication boundary. + +### 8. Preserve the complete output filesystem state + +Archive creation follows symlinks, omits directories and non-regular nodes, and cache restoration does not remove stale outputs. A cache hit can therefore produce a filesystem state different from a successful uncached run. + +Persist a complete output manifest, use `symlink_metadata`, reproduce symlinks and empty directories, and safely remove stale matching outputs before extraction. Prevent traversal outside the workspace. + +Reference: `crates/vite_task/src/session/cache/archive.rs:15-62` + +### 9. Bound captured stdout and stderr + +Cached task output is retained in memory and later stored in SQLite without a size limit. A verbose task can exhaust runner memory or create very large database rows. + +Use bounded capture or spool output to files. Disable cache replay beyond a configurable threshold while preserving normal live reporting. + +### 10. Move blocking cache work off async execution threads + +SQLite queries, input hashing, tar creation, and zstd compression execute synchronously inside async orchestration. Slow storage can stall unrelated task progress, output draining, and IPC handling. + +Use a dedicated cache worker or bounded `spawn_blocking` pool. Instrument planning, hashing, database, archive, and child-execution timings separately. + +### 11. Treat fingerprint I/O errors accurately + +Only actual `NotFound` errors should become missing-path fingerprints. Permission and transient I/O errors should prevent cache use or update with a diagnostic rather than masquerading as absence. + +Reference: `crates/vite_task/src/session/execute/fingerprint.rs:388-419` + +### 12. Bound descendant drain behavior + +After the direct child exits, surviving descendants can retain runner-aware IPC or fspy handles indefinitely and prevent task completion. + +Apply a bounded drain timeout or cancellation policy and discard late descendant reports after the direct child exits. + +## Workspace and task-graph correctness + +### 13. Support local dependencies beyond `workspace:` + +Ordinary local semver ranges, `file:`, `link:`, aliases, and pnpm `link-workspace-packages` dependencies are discarded before graph resolution. This can produce incomplete package graphs and incorrect topological execution. + +Preserve dependency names and specifiers, then resolve them against known workspace packages using package-manager-aware rules. Emit diagnostics for unsupported forms rather than silently omitting edges. + +Reference: `crates/vite_workspace/src/package.rs:47-69` + +### 14. Do not silently discard workspace traversal failures + +Propagate permission and I/O errors with the affected glob and path. Ignore only expected disappearance races. + +Reference: `crates/vite_workspace/src/lib.rs:112-130` + +### 15. Add runtime coverage for complex dependency graphs + +Existing plan snapshots are strong, but runtime coverage should include: + +- object-form `dependsOn` ordering; +- diamond graphs and shared dependency deduplication; +- mixed explicit and topological dependencies; +- cycles formed across multiple edge types; +- packages missing the requested task; +- `--ignore-depends-on` execution behavior; +- extra arguments reaching only explicitly requested tasks. + +### 16. Track shebang interpreters consistently + +The preload execution path records shebang interpreter reads, but the seccomp/static-executable path records only the script. Reuse the shared parser with a bounded kernel-compatible recursion limit and test both paths. + +References: + +- `crates/fspy/src/unix/syscall_handler/execve.rs:8-9` +- `crates/fspy_shared_unix/src/exec/mod.rs:135-140` + +## Architecture and maintainability + +### 17. Split `vite_task` along existing seams + +The crate currently owns CLI types, session orchestration, scheduling, process execution, cache storage, reporting, fspy integration, IPC, and Node addon materialization. Natural boundaries are: + +- `vite_task_exec`: scheduling, cancellation, and process lifecycle; +- `vite_task_cache`: fingerprints, persistence, archives, and cache schema; +- `vite_task_report`: reporter interfaces and presentation; +- `vite_task`: composition and session facade. + +Move cache-domain types together; they are currently split between planning and runtime crates. + +### 18. Remove test-only milestone infrastructure from production + +`vite_task` directly depends on `pty_terminal_test_client` and emits test milestones from production selector code. + +Replace this with a feature-gated test hook, injected observer, or synchronization based on externally observable screen state. + +References: + +- `crates/vite_task/Cargo.toml:23-27` +- `crates/vite_task/src/session/mod.rs:536-543` + +### 19. Decouple Node integration from generic execution + +Model runner-aware IPC and language bridges as execution observers that provide child environment additions, a concurrently polled driver, and post-run reports. Lazy-materialize the N-API addon only when requested. + +### 20. Decide the status of `vite_tui` + +The TUI is disconnected from task execution, hard-codes system commands, disables tests, and broadens the dependency graph. + +Exclude it from normal workspace validation until active development resumes, or integrate it through a supported execution-event API and move blocking PTY work to dedicated threads. + +### 21. Improve workspace loading scalability + +Enumerate and sort package paths first, then read and parse package files and user configuration concurrently. Deterministically assemble graphs afterward to preserve snapshot stability. + +### 22. Consolidate duplicated semantics + +High-value consolidation opportunities include: + +- environment prefix/query matching shared by IPC serving and cache validation; +- structurally identical input/output glob configuration resolution; +- repeated reporter lifecycle plumbing; +- summary persistence, statistics, and formatting currently combined in one large module. + +## Testing and tooling + +### 23. Add focused scheduler tests + +Use deterministic fake leaf executions to cover: + +- chains, diamonds, fan-in, fan-out, and disconnected components; +- exact concurrency bounds; +- each node executing exactly once; +- dependency completion before dependent start; +- cancellation before scheduling and while waiting for permits; +- fast-fail with queued and running tasks; +- nested graph failure propagation; +- empty graphs and maximum permit clamping. + +### 24. Add direct archive and cache tests + +Cover: + +- nested and empty output sets; +- files disappearing during collection; +- executable-bit and read-only restoration; +- symlinks, empty directories, and non-regular nodes; +- malformed and truncated archives; +- extraction traversal safety; +- missing and corrupt database values; +- concurrent processes publishing the same and different entries; +- interruption at each archive/database publication boundary. + +### 25. Expand cross-platform filesystem fixtures + +Add equivalent platform-paired cases for: + +- symlinks and Windows junctions; +- hardlinks; +- case sensitivity; +- non-UTF-8 Unix filenames; +- Unicode normalization on macOS; +- permissions and executable bits; +- long and extended-length Windows paths. + +### 26. Add Linux arm64 and cross-target lint coverage + +The runtime matrix covers Linux x64, macOS arm64/x64, Windows x64, and Linux musl, but not Linux arm64. Add Linux arm64 compile/runtime coverage where practical, plus Windows and Linux cross-target Clippy jobs already represented by local `just` recipes. + +### 27. Introduce measured coverage + +Add `cargo llvm-cov` for default and ignored tests. Initially publish reports without a global percentage gate. Focus changed-line or per-file expectations later on scheduler, cache, task graph query, planning, and workspace loading modules. + +### 28. Enforce dependency policy + +The repository has a detailed `deny.toml`, but CI does not enforce it. Add pinned `cargo deny check` runs when manifests, the lockfile, or policy change, and on a schedule for newly disclosed advisories. + +### 29. Add direct JavaScript client tests + +Use a fake addon module to test no-runner behavior, missing or throwing addons, one-time loading, argument forwarding, fallback values, API version mismatch, and paths containing spaces. Gate publishing on these tests. + +## Documentation and developer experience + +### 30. Correct the E2E harness README + +The README documents shell-string commands and pipes, while the harness expects argument arrays and permits only `vt` and `vtt`. Document the real schema, supported interactions, platform filters, ignored tests, snapshot formatting, and timeout behavior. + +Reference: `crates/vite_task_bin/tests/e2e_snapshots/README.md:14-47` + +### 31. Correct the input configuration guide + +The guide repeatedly documents `inputs`, while the configuration field and fixtures use singular `input`. Validate documentation JSON examples against the real deserializer to prevent future schema drift. + +Reference: `docs/inputs.md:1-30` + +### 32. Align setup and version documentation + +`CONTRIBUTING.md` requires pnpm 10.x, while `package.json` pins pnpm 11.1.2. It also says `just init` bootstraps Node tooling, but that recipe installs only Rust tools. + +Either expand the setup recipe to install locked Node dependencies and initialize submodules, or correct the documentation. Reconcile the documented Rust MSRV with the manifest and pinned development toolchain. + +References: + +- `CONTRIBUTING.md:3-17` +- `package.json:17-20` +- `justfile:11-12` + +## Recommended implementation sequence + +1. Fix `exec*` undefined behavior, IPC allocation limits, and external executable fingerprints. +2. Resolve nested concurrency semantics and add focused scheduler tests. +3. Correct metadata, symlink, shebang interpreter, and `dist` fingerprinting. +4. Make cache publication transactional, bounded, self-healing, and filesystem-faithful. +5. Add archive, fingerprint, corruption, and multi-process fault tests. +6. Expand workspace dependency syntax support and runtime graph coverage. +7. Isolate blocking work and split runtime responsibilities along existing seams. +8. Improve platform CI, coverage, dependency-policy enforcement, and JavaScript client tests. +9. Correct contributor, E2E harness, concurrency, and input documentation. + +## Overall assessment + +The codebase has a sound conceptual architecture and unusually strong integration coverage for a cross-platform task runner. The main opportunities are correctness hardening at operating-system boundaries and making runtime/cache semantics explicit and testable. Addressing the first six findings would materially improve reliability without requiring a broad redesign. + +This assessment was produced through a read-only static review of source, tests, fixtures, manifests, workflows, and documentation. No compilation or test execution was performed as part of the review.