# Lincyaw Workbuddy Code Review Standards > Code Review Standards - Skill: `tomevault-io/lincyaw-workbuddy-code-review-standards` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/lincyaw-workbuddy-code-review-standards` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/lincyaw-workbuddy-code-review-standards/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/lincyaw-workbuddy-code-review-standards --- # Code Review Standards Review verifies two things: the code fulfills the spec, and the code is maintainable. In that order — correct but messy beats clean but wrong. ## Review process ### Step 1: Run the tests Before reading any code, run the test suite (specifically the tests generated from the spec): ```bash pytest tests/test_.py -v ``` Record per-test pass/fail. This is the ground truth — everything else in the review is judgment layered on top. If tests don't exist (test-gen agent didn't run, or spec didn't have ACs), flag this immediately: review cannot verify correctness without tests. Send back to the developing state. ### Step 2: Per-AC verdict For each acceptance criterion, produce a structured verdict: | AC | Test Result | Code Inspection | Verdict | |----|-------------|-----------------|---------| | AC-1 | test_ac1 PASS | Implementation matches spec interface | **PASS** | | AC-2 | test_ac2 PASS | Edge case handled correctly | **PASS** | | AC-3 | test_ac3 FAIL | Time handling uses `time.time()` not injected clock | **FAIL** | | AC-4 | test_ac4 PASS | Reset clears counter dict | **PASS** | | AC-5 | (no test) | Concurrent access not addressed | **CANNOT JUDGE** | Three verdicts: - **PASS**: test passes AND code inspection confirms the AC is met (not just the test — the test might have gaps) - **FAIL**: test fails, OR test passes but code inspection reveals the implementation doesn't actually satisfy the AC (test gap) - **CANNOT JUDGE**: no test exists for this AC, or the AC requires manual verification (e.g., UI appearance) ### Step 3: Code quality findings After the per-AC verdict, note code quality issues. Categorize by severity: **Critical** — must fix before merge: - Security vulnerabilities (injection, auth bypass, secret exposure) - Data corruption risks (race conditions on shared state, missing transactions) - Correctness bugs not caught by existing tests **Major** — should fix, but doesn't block merge if ACs pass: - Missing error handling for documented error cases - Performance issues in hot paths (unbounded allocations, O(n²) where O(n) is straightforward) - Breaking changes to existing public interfaces without migration **Minor** — nice to fix, never blocks merge: - Naming inconsistencies with project conventions - Missing docstrings on public functions - Unused imports or variables ### Step 4: Verdict decision - **All ACs PASS, no critical findings** → approve, transition to done - **Any AC FAIL** → reject, transition to developing. The verdict comment must list every failing AC with: - What the AC requires - What the implementation does instead - The specific test failure message (if available) - A suggestion for how to fix (optional but helpful) - **CANNOT JUDGE on any AC** → flag but don't block if all testable ACs pass. Note what couldn't be verified and why. ## Selecting between parallel implementations When multiple dev sessions produce implementations that all pass tests, rank by: 1. **Correctness** — any AC FAIL disqualifies 2. **Simplicity** — fewer lines of code, fewer abstractions, fewer dependencies. The implementation that a new contributor would understand fastest. 3. **Idiomaticity** — follows the project's existing patterns. If the codebase uses dataclasses, don't introduce attrs. If it uses `fmt.Errorf`, don't introduce a custom error package. 4. **Test coverage beyond spec** — did the dev agent add extra tests for edge cases not in the ACs? Bonus, not required. 5. **Performance** — only as a tiebreaker, and only if measured. "Looks faster" is not a valid criterion. When selecting, name the winner and briefly explain why: ``` Selected: rollout-2 Reason: All 7 ACs pass. Rollout-2 implements the rate limiter as a single function with a dict + timestamp, while rollout-1 uses a class hierarchy with 3 abstract base classes. Rollout-2 is 40 lines vs 120 lines with identical behavior. Rollout-3 fails AC-3. ``` ## Structured verdict format The review comment MUST follow this format so the orchestrator can parse it and provide structured feedback to the dev agent: ```markdown ## Review Verdict ### Test Results - Total: 7 | Pass: 5 | Fail: 2 - Failing: test_ac3_window_expiry, test_ac6_zero_max_requests ### Per-AC Verdict | AC | Verdict | Evidence | |----|---------|----------| | AC-1 | PASS | test_ac1 passes; implementation uses counter dict | | AC-2 | PASS | test_ac2 passes; boundary check correct | | AC-3 | FAIL | test_ac3 fails: time.time() not mockable, flaky | | AC-4 | PASS | test_ac4 passes; reset clears entry | | AC-5 | PASS | test_ac5 passes; separate dict entries per client | | AC-6 | FAIL | test_ac6 fails: ValueError not raised for 0 | | AC-7 | PASS | test_ac7 passes; ValueError raised for negative | ### Code Quality - [critical] AC-6: constructor accepts max_requests=0 without raising ValueError (missing validation) - [minor] rate_limiter.py: unused import `collections` ### Decision REJECTED — 2 ACs fail. See failing ACs above for required fixes. ``` The `` marker is required on the first line. The orchestrator uses it to locate the most recent verdict when assembling the next dev cycle's prompt. ## Common review mistakes **Reviewing style when correctness is unclear.** Don't bikeshed variable names while tests are failing. Fix correctness first. **Trusting tests blindly.** Tests pass ≠ implementation is correct. Tests can have gaps. A test that checks `assert result is not None` passes even if the result is wrong. Read the code. **Rejecting for style alone.** If all ACs pass and there are no critical findings, the implementation is acceptable. Style issues are minor findings, not rejection reasons. **Vague rejection.** "Needs work" is not actionable. The dev agent needs: which AC fails, what error message, what's wrong with the implementation, what would fix it. **Reviewing without test results.** If you haven't run the tests, you're guessing at correctness. Run them first. Always. --- > Source: [Lincyaw/workbuddy](https://github.com/Lincyaw/workbuddy) — distributed by [TomeVault](https://tomevault.io).