From 2ce9eb47aafbf4030eb418207bec57c916d4f9bc Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 5 Aug 2026 12:58:19 -0700 Subject: [PATCH 1/5] docs: strengthen PET Rust review guidance (Fixes #496) Capture recurring path identity, Unicode parsing, hot-path I/O, side-effect, and behavior-test checks from recent PR reviews. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/Reviewer.agent.md | 35 ++++++++++ .github/skills/rust-coding-skill/SKILL.md | 83 +++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 .github/skills/rust-coding-skill/SKILL.md diff --git a/.github/agents/Reviewer.agent.md b/.github/agents/Reviewer.agent.md index 2b6b41dd..69eb8a5a 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,8 @@ Automated reviews consistently miss: Before reading code: +- If any changed file is Rust, load and apply both `.github/skills/rust-coding-skill/SKILL.md` and `.github/skills/rust-locator-patterns/SKILL.md`. + - 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 +168,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 `.github/skills/rust-coding-skill/SKILL.md` 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..d378c8c8 --- /dev/null +++ b/.github/skills/rust-coding-skill/SKILL.md @@ -0,0 +1,83 @@ +--- +name: "rust-coding-skill" +description: "Use whenever editing Rust in PET to write allocation-aware, cross-platform, byte-safe code with behavior-proving tests." +user-invocable: true +--- + +# 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 start = find_ascii_case_insensitive(line, "# cmd:")? + "# cmd:".len(); +let end = find_ascii_case_insensitive(line, " create -")?; +let value = line.get(start..end)?.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. + +## 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 = Cell::new(0); +parse_with_reader(path, |_| { + reads.set(reads.get() + 1); + Some(history.clone()) +}); +assert_eq!(reads.get(), 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 the targeted tests plus `scripts/rust-precommit.ps1` (or `.sh`). Do not suppress Clippy warnings to land a change. \ No newline at end of file From c4f83c90312df276e63cac170471fbcd067ca48b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 5 Aug 2026 14:10:12 -0700 Subject: [PATCH 2/5] docs: address Rust skill review feedback (PR #497) Use supported frontmatter, standard-library parsing examples, atomic counters, and the existing pre-commit skill as the single source of truth. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/skills/rust-coding-skill/SKILL.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/.github/skills/rust-coding-skill/SKILL.md b/.github/skills/rust-coding-skill/SKILL.md index d378c8c8..84293c90 100644 --- a/.github/skills/rust-coding-skill/SKILL.md +++ b/.github/skills/rust-coding-skill/SKILL.md @@ -1,7 +1,6 @@ --- name: "rust-coding-skill" description: "Use whenever editing Rust in PET to write allocation-aware, cross-platform, byte-safe code with behavior-proving tests." -user-invocable: true --- # PET Rust Coding Skill @@ -33,9 +32,13 @@ Never calculate byte offsets from a transformed Unicode string and apply them to For ASCII wire/file markers, use byte-stable ASCII-insensitive matching and checked slicing: ```rust -let start = find_ascii_case_insensitive(line, "# cmd:")? + "# cmd:".len(); -let end = find_ascii_case_insensitive(line, " create -")?; -let value = line.get(start..end)?.trim(); +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. @@ -70,14 +73,14 @@ Tests should demonstrate the behavior or performance invariant, not merely execu For optimizations, instrument the dependency boundary and assert the operation count: ```rust -let reads = Cell::new(0); +let reads = AtomicUsize::new(0); parse_with_reader(path, |_| { - reads.set(reads.get() + 1); + reads.fetch_add(1, Ordering::Relaxed); Some(history.clone()) }); -assert_eq!(reads.get(), 1); +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 the targeted tests plus `scripts/rust-precommit.ps1` (or `.sh`). Do not suppress Clippy warnings to land a change. \ No newline at end of file +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 From 9f190ff1533d109208d698b781761f3e85598fde Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 5 Aug 2026 14:14:44 -0700 Subject: [PATCH 3/5] docs: capture config semantics review lesson (PR #497) Add raw-literal and downstream config-consumption checks from the latest glob diagnostics review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/skills/rust-coding-skill/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/skills/rust-coding-skill/SKILL.md b/.github/skills/rust-coding-skill/SKILL.md index 84293c90..ebce90e5 100644 --- a/.github/skills/rust-coding-skill/SKILL.md +++ b/.github/skills/rust-coding-skill/SKILL.md @@ -65,6 +65,8 @@ Use contextual `expect` for poisoned locks in production code, matching the surr - 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 From fba43a6788d67c8cfffd702e6282a429b07d3c4a Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 5 Aug 2026 14:25:58 -0700 Subject: [PATCH 4/5] docs: reference Rust skills by name (PR #497) Keep reviewer instructions stable and directly actionable through skill invocation names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/Reviewer.agent.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/agents/Reviewer.agent.md b/.github/agents/Reviewer.agent.md index 69eb8a5a..cef66e07 100644 --- a/.github/agents/Reviewer.agent.md +++ b/.github/agents/Reviewer.agent.md @@ -48,7 +48,7 @@ Automated reviews consistently miss: Before reading code: -- If any changed file is Rust, load and apply both `.github/skills/rust-coding-skill/SKILL.md` and `.github/skills/rust-locator-patterns/SKILL.md`. +- If any changed file is Rust, load and apply both the `rust-coding-skill` and `rust-locator-patterns` skills. - What issue does this change claim to fix? - Which locator/crate is affected? @@ -170,7 +170,7 @@ let mut environments = self.environments ### General Rust Correctness and Performance -Apply `.github/skills/rust-coding-skill/SKILL.md` to every Rust review, not only locator changes. +Apply the `rust-coding-skill` skill to every Rust review, not only locator changes. **Path-keyed state:** From f0dccc3fe858aee3c814abd1bf68021e535ec08e Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 5 Aug 2026 15:29:55 -0700 Subject: [PATCH 5/5] docs: scope locator review guidance (PR #497) Always apply the general Rust skill and load locator-specific guidance only when locator behavior is in scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/Reviewer.agent.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/agents/Reviewer.agent.md b/.github/agents/Reviewer.agent.md index cef66e07..f5a3cdb3 100644 --- a/.github/agents/Reviewer.agent.md +++ b/.github/agents/Reviewer.agent.md @@ -48,7 +48,8 @@ Automated reviews consistently miss: Before reading code: -- If any changed file is Rust, load and apply both the `rust-coding-skill` and `rust-locator-patterns` skills. +- 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? @@ -170,7 +171,7 @@ let mut environments = self.environments ### General Rust Correctness and Performance -Apply the `rust-coding-skill` skill to every Rust review, not only locator changes. +Apply `rust-coding-skill` to every Rust review, not only locator changes. **Path-keyed state:**