Pwrl Work Review

pwrl-work-review — Code Simplification & System Review

Interaction Method

• Use platform's ask_user_question, ask_user, ask_user_input, vscode/askQuestions or any available extension/tool for user interaction for all decisions
• Ask one question at a time
• Use multiple-choice questions when possible
• If input is empty, ask: "Should the implementation be reviewed? Confirm scope match, test coverage, and code quality."
• Provide clear recovery suggestions when errors occur

Purpose: Review code after execution to consolidate duplication, extract shared helpers, run system-wide consistency checks, compare UI implementations to design specs (when applicable), and determine readiness for shipping.

Input

Accepts execution results from pwrl-work-execute:

mode: serial
taskCount: 5
tasksCompleted: 5
results:
  - unit-id: S1
    taskFile: docs/tasks/for-review/...
    status: for-review
    testsPassed: true
    files: [src/utils.ts, src/api.ts]

Optional: design spec references for UI work (e.g., Figma links, docs/design/ files).

Output: Review Summary

duplicationFound: true
duplicationsConsolidated: 3
helpersExtracted: 2
helperNames: [isValidEmail, parseUserInput]
LOCReduced: 120
systemChecks:
  eventTriggering: pass
  mockCoverage: warning
  idempotency: pass
  alternateEntryPoints: pass
designSpecDeltas: 0
refactoringScope: controlled
readyForShipping: true
recommendations:
  - "Add 2 integration tests for mock coverage"

Workflow

1. Identify Modified Files

After every 2-3 related tasks:

1. Collect files affected by the completed tasks:
   • From each task's files field in execution results
   • From git diff against base branch: git diff --name-only <base>...HEAD
2. Build a consolidated list of files modified by these tasks
3. Focus review ONLY on these files (scope control)

2. Detect Duplication

Scan modified files for code duplication:

Approach:

1. For each modified file, look for:

   • Identical code blocks (same logic, same structure)
   • Similar patterns with different variable names
   • Copy-paste code (identical comments, same ordering)

2. Cross-file duplication:

   • Compare files modified by different tasks
   • If two tasks modified different files with similar logic → candidate

3. For each candidate, show:

   • Location (file:lines)
   • Code snippet (before)
   • Proposed consolidation (after)
   • LOC reduction estimate

Example:

Found duplication between src/user.ts:45 and src/signup.ts:72:
  Both validate email with same regex: /^[^@]+@[^@]+\.[^@]+$/
  Proposed: Extract isValidEmail() to src/validation.ts
  LOC reduction: 12 lines

Decision:

• Ask user: "Extract helper? (yes/no/skip)"
• If yes: proceed to Step 3 (Helper Extraction)
• If no or skip: log as noted for future refactoring

3. Extract Helpers

For confirmed candidates, extract shared logic:

When to extract:

• Code appears 2+ times across modified files
• Extraction improves readability (function name self-documents)
• Extraction improves testability

When NOT to extract (YAGNI):

• Code appears only once
• Extract adds indirection without benefit
• Extract would create complex API (too many parameters)

Extraction process:

1. Design helper:

   • Name: clear verb/noun describing what it does
   • Parameters: minimal set needed
   • Return value: typed and documented

2. Create helper file (or add to existing shared module):

   • If no shared module exists: create one (e.g., src/utils.ts, src/validation.ts)
   • If shared module exists: add helper to it

3. Write tests for helper (test-first):

   • Test normal case
   • Test edge cases (empty, null, boundary)
   • Test error handling

4. Replace call sites:

   • Replace duplicate code blocks with helper call
   • Update all references

5. Run tests:

   • Verify all tests still pass
   • Log: "Extracted [helperName] → [file], replaced [N] call sites, -[M] LOC"

4. Run System Checks

After simplifying, verify system consistency:

Check 1: Event/Observer/Callback Triggering

• Are events emitted correctly? (e.g., user.created after signup)
• Are listeners registered and invoked?
• ✅ Pass if: Tests cover real event flow (not just mock assertions)
• ⚠️ Warn if: All event tests use mocks (real listener not tested)

Check 2: Mock vs. Real Interaction Balance

• What proportion of tests use mocked dependencies vs. real implementations?
• ✅ Pass if: ≥1 integration test per feature covers real interactions
• ⚠️ Warn if: All tests use mocks (no integration coverage)

Check 3: Idempotency & Cleanup Safety

• Can operations be retried safely?
• Are resources cleaned up after failure? (finally blocks, teardown)
• ✅ Pass if: Retry doesn't cause double effects; cleanup always runs
• ⚠️ Warn if: Failure leaves partial state or resource leaks

Check 4: Alternate Entry Points

• Is behavior consistent across different access methods?
• ✅ Pass if: All entry points tested with same behavior expectations
• ⚠️ Warn if: Only one entry point tested, or behavior diverges

Log results:

System Check Results:
  [✓] Event triggering: POST /user fires 'user.created'
  [⚠] Mock coverage: 8/10 tests use mocks → add 2 integration tests
  [✓] Idempotency: DELETE /user safe to retry
  [✓] Alternate entry points: API + CLI tested, consistent behavior

5. Compare to Design Specs (UI Work Only)

For tasks involving UI/component changes:

If design specs exist:

• Read from docs/design/ or linked Figma file
• Compare: layout, spacing, colors, typography, responsive behavior
• Classify each delta:

If no design specs:

• Skip this step entirely
• Log: "No design specs found; skipping visual comparison"

6. Control Refactoring Scope

Golden Rule: Only simplify code that was changed by executed tasks.

Do:

• Simplify code you just wrote
• Extract helpers from duplications you introduced
• Fix obvious bugs in modified code
• Improve tests for changed behavior

Do NOT:

• Refactor unrelated modules
• Do "while we're here" cleanup
• Change naming/style for consistency in untouched code

Enforcement:

• Build list of affected files from task execution results
• If scope check finds a change to an unrelated file:
  • Warn: "File [path] was modified but is not in any task's files list"
  • Ask: "Remove this unrelated change before shipping? (yes/no)"

7. Produce Review Summary

duplicationFound: true
duplicationsConsolidated: 2
helpersExtracted: 1
helperNames: [isValidEmail]
LOCReduced: 34
systemChecks:
  eventTriggering: pass
  mockCoverage: warning
  idempotency: pass
  alternateEntryPoints: pass
refactoringScope: controlled
readyForShipping: true
recommendations:
  - "Add 2 integration tests with real DB for user module"

If readyForShipping is true, work is ready for pull request creation.

Optional Deep Review Mode

For complex or high-risk changes, offer deep review:

/pwrl-work-review docs/tasks/for-review/ --deep

Deep review adds:

• Performance: Any obvious regressions? (N+1 queries, memory leaks)
• Security: New vulnerabilities? (XSS, injection, auth bypass)
• Accessibility: UI changes only (ARIA labels, keyboard nav, contrast)
• API contracts: Breaking changes? (new required params, removed fields)
• DB migrations: Safe? (backward compatible, rollback plan)

For detailed analysis, use the dedicated /pwrl-review skill.

Error Handling

Dependencies

• pwrl-work-execute (S5) — Consumes executed task results
• Git — For diff analysis
• File system — For reading/writing modified files
• Test framework — For verifying helpers and consolidations

References

• S1 Analysis: docs/analysis/2026-06-05-pwrl-work-structure-analysis.md
• Source Phase 3: Installed pwrl-work skill (Phase 3: Simplify & Review, lines 173-185)