Skip to content

chore: add code-review skill and strengthen existing skills#104

Merged
mohamedmansour merged 1 commit intomainfrom
akroshg/strengthen-review-skills
Mar 14, 2026
Merged

chore: add code-review skill and strengthen existing skills#104
mohamedmansour merged 1 commit intomainfrom
akroshg/strengthen-review-skills

Conversation

@akroshg
Copy link
Copy Markdown
Contributor

@akroshg akroshg commented Mar 13, 2026

Summary

Create a dedicated code-review skill and strengthen three existing skills (handler-perf, FFI, PR) based on findings from a full-project audit that produced 63 issues across all 12 Rust crates and 2 TypeScript packages.

The existing skills had a 30% catch rate — these changes close the gaps so the same classes of issues are flagged before PRs are opened.

Changes

New: .github/skills/code-review/SKILL.md

10-section framework code review checklist:

  1. Cross-layer contract parity — encoding, matching, spec-vs-code, binding surface, coercion rules, literal grammar, fallback contracts
  2. Determinism — HashMap iteration, tie-breaking, reproducibility
  3. Concurrency & race safety — lock scope, snapshots, catch_unwind on FFI, WASM panic hooks, callback error propagation, file watcher recovery, cache write races, Node event loop blocking
  4. Allocation & clone discipline — 17 Rust + 4 TS anti-patterns with fixes
  5. Data structure selection — access-pattern-to-structure mapping
  6. API surface & deduplication — duplicate logic, caching, public surface
  7. Type safety & correctness idioms#[must_use], silent fallbacks, error precision, byte/char confusion, numeric precision, build-time validation, UTF-8 safety
  8. Style & documentation — unused deps, placeholder APIs, tests-that-lock-in-bugs
  9. Server runtime & discovery — HTTP semantics, cache invalidation, symlinks, file-size guardrails, HMR injection
  10. Protocol & schema hygiene — versioning, backward compatibility

Updated: .github/skills/handler-perf/SKILL.md

Added 5 rules from handler audit:

  • No String::from(ch) in escape loops — use encode_utf8
  • No string clones for read-only escaping — use as_str()
  • No per-request route template re-parsing — pre-parse at load time
  • No format! in hex parsing — direct arithmetic
  • No silent unwrap_or defaults — propagate errors

Updated: .github/skills/ffi/SKILL.md

Expanded panic safety rules:

  • Explicit catch_unwind requirement on all FFI function bodies
  • No unwrap()/expect() in any FFI code path (including helpers)

Updated: .github/skills/pr/SKILL.md

  • Removed misplaced 60-line engineering checklist (wrong home for it)
  • Added cross-reference to the new code-review skill

Updated: .github/copilot-instructions.md

  • Registered code-review skill in the skills directory listing

Create a dedicated code-review skill with 10-section framework review
checklist covering correctness, performance, concurrency, design, and
style. Derived from a full-project audit (63 findings across 12 crates
and 2 TS packages) that exposed coverage gaps in existing guidance.

Changes:
- Add .github/skills/code-review/SKILL.md (288 lines, 10 sections)
- Update handler-perf skill with 5 new hot-path rules (String::from(ch),
  template caching, hex parsing, silent unwrap_or, clone discipline)
- Update FFI skill with explicit catch_unwind requirement and no
  unwrap/expect rule for panic safety across the boundary
- Replace misplaced engineering checklist in PR skill with a
  cross-reference to the new code-review skill
- Register code-review skill in copilot-instructions.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mohamedmansour mohamedmansour merged commit a084cfa into main Mar 14, 2026
15 checks passed
@mohamedmansour mohamedmansour deleted the akroshg/strengthen-review-skills branch March 14, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants