con un clic
code-review
// Perform a structured code review of staged or recently changed Warpine source files, covering correctness, security, test coverage, clean-room compliance, and documentation hygiene.
// Perform a structured code review of staged or recently changed Warpine source files, covering correctness, security, test coverage, clean-room compliance, and documentation hygiene.
| name | code-review |
| description | Perform a structured code review of staged or recently changed Warpine source files, covering correctness, security, test coverage, clean-room compliance, and documentation hygiene. |
When performing a code review, follow these steps in order:
Determine which files to review:
git diff HEAD (unstaged + staged changes) or git diff main...HEAD (branch vs main) to enumerate changed files.target/, $OUT_DIR, font_unifont*.bin) and vendored third-party code.cargo test 2>&1 # All unit + integration tests must pass
cargo clippy -- -D warnings 2>&1 # Zero warnings required
Report any failures immediately. Do not proceed to manual review until both are clean.
For each changed source file, check:
doc/ documentation. Check that error codes match the OS/2 spec (e.g., ERROR_INVALID_HANDLE = 6, not a Linux errno).APIRET (u32) in RAX. Confirm the correct value is placed in regs.rax on success and on each error path.targets/os2api.def and src/loader/api_registry.rs. Run cargo run --bin gen_api -- check and confirm it reports clean.guest_mem helpers must use the bounds-checked API; no raw pointer arithmetic from guest-supplied values.read_guest_string (which calls cp_decode with the active codepage). Direct from_utf8_lossy on raw guest bytes is wrong.Warpine's threat model is hypervisor escape from a malicious OS/2 guest. Check every changed unsafe block and every API handler:
guest_mem.Vec::with_capacity(guest_len) from an untrusted length is a DoS / OOM vector.../ traversal outside the mapped drive root, no absolute host paths derived from guest input.unwrap() on a KVM result is a crash vector.MAGIC_API_BASE INT 3 breakpoints must only be honoured when the guest is in ring 3 (CPL=3). Check that vcpu.rs verifies CPL before dispatching.Flag any finding with a [SECURITY] prefix and its severity (Critical / High / Medium / Low).
samples/.test_dos_alloc_mem_exceeds_limit, not test1).unwrap() on results that could legitimately fail — use expect() with a message or proper assertions.file vendor/**/* samples/**/* 2>/dev/null | grep -vE 'text|directory|makefile|script|source|ELF|PE32|data' and flag anything unexpected.constants.rs for existing definitions before adding new ones.unsafe blocks must have a // SAFETY: comment explaining the invariant being upheld.[STUB] message and return a reasonable error code.println! in production paths — use the structured logging/trace macros already in the codebase.doscalls.rs, pm_win.rs, etc.), not in mod.rs or vcpu.rs.Look for structural problems that are not bugs today but become bugs or maintenance burdens tomorrow. Flag each with a [SMELL] prefix.
Dead and redundant code
use imports that are never referenced — prefer outright deletion over #[allow(dead_code)].#[allow(...)] suppressions that paper over a real problem; prefer fixing the root cause.Complexity and readability
do_thing(true)) — prefer an enum or two named functions.is_not_valid where is_invalid would be clearer).Error handling
unwrap() / expect() on values that originate from guest input or external I/O — must use ? or explicit error handling.let _ = ... or an empty Err(_) => {} arm when the failure is meaningful.APIRET) with Rust Result without a clear conversion boundary.Warpine-specific smells
SharedState sub-locks in the documented order (memory → handles → dll → window → socket) to prevent deadlock; flag any deviation.struct field beyond the duration of a single API call — a stored raw guest address is stale after any guest reallocation.Arc::clone on a manager inside a hot VMEXIT path without a clear need — prefer passing a borrow.regs.rax written more than once in a single handler — only the last write counts; earlier ones are dead.Abstraction and coupling
mod.rs / vcpu.rs growing new business logic — it belongs in a domain module.super::super:: path — signals missing abstraction boundary.SharedState fields added without a corresponding Arc<Mutex<...>> wrapper when the field is accessed from multiple threads.CLAUDE.md — Update the API entry point count and any architecture notes that changed.doc/TODOs.md — Remove completed items; add any newly discovered gaps.CHANGELOG.md — Confirm there is an entry for the change under [Unreleased].doc/developer_guide.md — Update test counts if new tests were added.Present findings as a structured list grouped by category. For each finding include:
[SECURITY: Critical/High/Medium/Low]Conclude with a summary table:
| Category | Issues Found |
|---|---|
| Correctness | N |
| Security | N |
| Test coverage | N |
| Clean-room | N |
| Style | N |
| Code smells | N |
| Documentation | N |
If no issues are found in a category, write ✓ Clean.
Do not make any edits during the review pass. Present the full report first and wait for the user to confirm which findings to act on.
Audit and update all project documentation to stay in sync with the current development status.
Stage, commit, and push changes to the remote repository with a well-formed commit message.
Manage the full software release process, including version bumps, changelogs, Git tags, and GitHub releases.
Perform project-wide security audits focused on KVM hypervisor escape, guest memory safety, and dependency vulnerabilities.