diff --git a/.github/agents/Reviewer.agent.md b/.github/agents/Reviewer.agent.md index 2b6b41dd..f5a3cdb3 100644 --- a/.github/agents/Reviewer.agent.md +++ b/.github/agents/Reviewer.agent.md @@ -35,6 +35,10 @@ Automated reviews consistently miss: - Thread safety issues with shared state - JSONRPC protocol violations (stdout contamination) - Performance regressions from spawning Python processes +- Path-keyed caches that normalize keys inconsistently or return stale caller-facing paths +- Unicode-unsafe byte indexing and unnecessary hot-path allocations +- Tests that execute code without proving the claimed I/O or allocation reduction +- Duplicate warnings, reports, or telemetry emitted from overlapping paths --- @@ -44,6 +48,9 @@ Automated reviews consistently miss: Before reading code: +- If any changed file is Rust, load and apply `rust-coding-skill`. +- Load `rust-locator-patterns` only when locator ordering, discovery, identification, path/symlink handling, or locator state is in scope. + - What issue does this change claim to fix? - Which locator/crate is affected? - Does it touch identification logic (`try_from`) or discovery logic (`find`)? @@ -162,6 +169,35 @@ let mut environments = self.environments - No deadlock potential from nested locks - Consider using `thread::scope` for structured concurrency +### General Rust Correctness and Performance + +Apply `rust-coding-skill` to every Rust review, not only locator changes. + +**Path-keyed state:** + +- Are lookup, insert, remove, prune, and sync keys normalized consistently? +- Does a cache hit preserve the current caller's user-facing path rather than leaking the first cached spelling? +- Is there Windows coverage for equivalent casing or separators? + +**Parsing:** + +- Are byte offsets computed from the same string being sliced? +- If markers are ASCII, does the code use byte-stable ASCII matching and checked `str::get` slicing? +- Is there a non-ASCII regression case around paths or names? + +**Hot paths:** + +- Does the implementation read each metadata file once per logical operation? +- Did the author trace root/base and manager call paths, not just the common environment path? +- Are borrowed snapshots passed through instead of reopening files or cloning strings? +- Can `rfind`, `find_map`, or streaming output replace an intermediate `Vec` or `String`? + +**Side effects and tests:** + +- Can a warning, notification, manager, or environment be emitted twice by pre-check and worker paths? +- Does the test assert the claimed invariant (read count, cache hit, event count), not just the final value? +- Are classification boundaries covered (`**` path segment vs `foo**bar`, valid vs malformed markers)? + ### Platform-Specific Code **Use the correct conditional compilation:** diff --git a/.github/skills/rust-coding-skill/SKILL.md b/.github/skills/rust-coding-skill/SKILL.md new file mode 100644 index 00000000..ebce90e5 --- /dev/null +++ b/.github/skills/rust-coding-skill/SKILL.md @@ -0,0 +1,88 @@ +--- +name: "rust-coding-skill" +description: "Use whenever editing Rust in PET to write allocation-aware, cross-platform, byte-safe code with behavior-proving tests." +--- + +# PET Rust Coding Skill + +Use this alongside `rust-locator-patterns`. Priority order: + +1. Readable code with explicit invariants +2. Correct cross-platform and concurrent behavior +3. Measured performance improvements without duplicate work + +## Path Identity and Caches + +Use `Path`/`PathBuf` for paths. Preserve the caller-facing path in reported values, but normalize cache and comparison keys with existing PET helpers such as `norm_case`. + +A normalized key does not imply the cached value can expose the first caller's spelling: + +```rust +let key = norm_case(path); +let mut cached = cache.get(&key)?.clone(); +cached.prefix = Some(path.to_path_buf()); +``` + +When adding or reviewing a path-keyed cache, check lookup, insert, remove, retain/prune, and state-sync paths. Add Windows coverage using equivalent separators or casing; do not test only the happy-path spelling. + +## Byte-Safe Parsing + +Never calculate byte offsets from a transformed Unicode string and apply them to the original. Unicode case conversion can change byte length. + +For ASCII wire/file markers, use byte-stable ASCII-insensitive matching and checked slicing: + +```rust +let marker = b"# cmd:"; +let start = line + .as_bytes() + .windows(marker.len()) + .position(|window| window.eq_ignore_ascii_case(marker))? + + marker.len(); +let value = line.get(start..)?.trim(); +``` + +Use `to_ascii_lowercase` rather than `to_lowercase` when the format is defined as ASCII. Add a non-ASCII path regression test whenever offsets are derived from textual markers. + +## Hot-Path I/O and Allocations + +Discovery runs frequently and in parallel. Before adding a cache, prove the repeated work and define invalidation. Within one operation, read immutable metadata once and pass borrowed snapshots through parsers. + +- Prefer `&str`/`&[u8]` over cloning content between parsers. +- Prefer `rfind`/iterator operations over collecting an intermediate `Vec` just to select one item. +- Avoid `format!` and Unicode case conversion in per-line loops when ASCII matching or direct writes suffice. +- Do not claim an optimization is complete until every relevant call path is traced, including base/root environments and manager lookup. +- Do not emit the same warning, telemetry event, or report from both a pre-check and the worker path. + +## Error Handling and Locks + +Library code should preserve typed information where repository APIs permit it. Prefer `?`, `let-else`, and `if let` over broad fallbacks. Do not swallow filesystem errors when doing so can make stale cache data look valid. + +Use contextual `expect` for poisoned locks in production code, matching the surrounding crate. Keep lock scopes short and never perform filesystem I/O or callbacks while holding a shared-state lock unless the design explicitly requires it. + +## Cross-Platform Semantics + +- Use `#[cfg(...)]` for platform-only code; `cfg!` does not prevent compilation. +- Avoid `canonicalize` for Windows junction identity; use PET path helpers. +- Treat both `/` and `\` as separators when parsing user patterns, but only classify `**` as recursive when it is a complete path segment. `foo**bar` is not a recursive segment. +- Preserve original user-facing paths after normalized comparisons. +- Prefer raw string literals for regexes and backslash-heavy path examples to avoid malformed escapes. +- Before documenting or logging a recommended config value, trace how the consumer uses it. For example, `environmentDirectories` contains directories that hold environments, not environment folders themselves. + +## Tests Must Prove the Change + +Tests should demonstrate the behavior or performance invariant, not merely execute new lines. + +For optimizations, instrument the dependency boundary and assert the operation count: + +```rust +let reads = AtomicUsize::new(0); +parse_with_reader(path, |_| { + reads.fetch_add(1, Ordering::Relaxed); + Some(history.clone()) +}); +assert_eq!(reads.load(Ordering::Relaxed), 1); +``` + +For parser helpers, include malformed input, non-ASCII surrounding data, and case variations. For diagnostics, test pattern classification and expansion filtering separately. Keep temp paths unique with `tempfile` or process/counter-based names. + +Before every Rust commit, run targeted tests and invoke the `rust-precommit` skill. Keep that skill as the single source of truth for required format and Clippy commands. \ No newline at end of file