igmarin/review-engine

Review Engine

Use this skill when the task is to review an existing Rails engine or propose improvements.

Quick Reference

| Review Area | Key Checks | |-------------|------------| | Namespace | isolate_namespace used; clear boundaries; no host constant leakage | | Host integration | Configuration seams, adapters; no direct host model access | | Init | No side effects at load time; reload-safe hooks in config.to_prepare | | Migrations | Documented, copied via generator; no implicit or destructive steps | | Dummy app | Present in spec/; used for integration tests; exercises real mount and config |

HARD-GATE

Before writing findings, confirm every row in the Quick Reference table has been addressed:
- [ ] Namespace isolation verified
- [ ] Host integration points checked
- [ ] `engine.rb` initializer blocks inspected
- [ ] Migration/generator flow confirmed
- [ ] Dummy app presence and usage confirmed
- [ ] Integration tests exercise real mount

If any box cannot be checked (e.g., file not provided), record it as an open assumption.

Core Process

1. Identify the engine type and purpose. Read lib/<engine_name>/engine.rb and lib/<engine_name>/railtie.rb. Confirm isolated vs plain.
2. Inspect the namespace and public API surface. Check for isolate_namespace and unqualified top-level constant references.
3. Check host-app integration points. Flag direct host constant references. Verify host dependencies flow through config seams.
4. Check initialization and reload behavior. Inspect initializer, config.to_prepare, and ActiveSupport.on_load. Flag anything that mutates global state at require time outside an initializer block.
5. Check migrations, generators, and install flow. Confirm migrations are copied via a generator. Check for destructive or irreversible migrations.
6. Check dummy-app and integration tests. Confirm spec/dummy/ exists and exercises the mount point.
7. Summarize findings by severity. Flag High findings first. Do not surface Low findings before architecture issues.

High-severity finding example (engine reaching into host):

# Bad: engine assumes host model
class MyEngine::SomeService
  def call
    User.find(current_user_id)  # User is host app; engine is coupled
  end
end

Fix: Introduce config (MyEngine.config.user_finder = ->(id) { User.find(id) }) and use that.

Good (configuration seam):

# Good: engine uses configured dependency
class MyEngine::SomeService
  def call
    MyEngine.config.user_finder.call(current_user_id)
  end
end

Bad (require-time patching — not reload-safe):

# Bad: patches at require time — double-includes on code reload
ActionController::Base.include(MyEngine::ControllerHelpers)

Good (lazy-loaded with ActiveSupport.on_load):

# Good: patches only when the framework component is ready, reload-safe
ActiveSupport.on_load(:action_controller) do
  include MyEngine::ControllerHelpers
end

Output Style

When asked to review an engine, your output answer.md MUST comply with:

1. Findings First: Write the findings section (ordered HIGH -> MEDIUM -> LOW) as the very first section in the concrete artifact answer.md (or immediately after the short plan if a plan is requested). For each finding include severity, affected file/area, risk, and smallest credible fix.
2. Verification Commands: Include verification commands used, including grep -r "isolate_namespace" lib/ for namespace isolation, a migration audit such as grep -R "remove_column\|drop_table\|change_column" db/migrate lib/**/db/migrate for destructive or irreversible changes, and grep -r "ActiveSupport.on_load" lib/ or grep -r "initializer" lib/ to verify initialization reload safety.
3. Include open assumptions and recommended next changes.
4. If no meaningful findings exist, explicitly state so and mention residual testing gaps.
5. Language — Must be in English unless explicitly requested otherwise.

Integration

| Skill | When to chain | |-------|---------------| | create-engine | When implementing suggested fixes or refactoring the engine | | test-engine | When adding missing dummy-app or integration coverage | | upgrade-engine | When assessing Rails/Ruby version support or deprecation impact |

---

Extended Reference

Supplementary detail — consult after completing the Core Process.

Severity Tiers

• High — causes production failures or breaks host integration (e.g., direct host constant coupling, unsafe boot-time side effects, irreversible migrations without a down method).
• Medium — degrades maintainability or makes the engine fragile across host apps (e.g., undocumented configuration seams, missing install generator, no dummy app).
• Low — style or minor clarity issues; do not surface before architecture findings.

Common Mistakes

• Reviewing code style before architecture.
• Missing dummy app coverage check (dummy app must exist and be used).
• Ignoring engine.rb (often contains boot-time side effects).

Extended Resources (Progressive Disclosure)

Load these files only when their specific content is needed:

• FINDINGS.md — Use when you need sample findings format and severity classification examples
• assets/examples.md — Use when you need complete engine review examples with findings and recommendations
• assets/finding-schema.json — Use when you need the structured schema for engine review findings output