## CRITICAL: Test Suite Failing (6 failures) The PR changes to `config/tinker/mcp_permissions.yml` removed `transition_ticket` from worker allowed tools, causing 4 test failures: - `spec/requests/api/v1/mcp_spec.rb:11` - GET /api/v1/mcp/tools expects transition_ticket - `spec/requests/api/v1/mcp_spec.rb:206` - POST transition_ticket returns 403 forbidden - `spec/requests/api/v1/mcp_spec.rb:218` - POST transition_ticket for draft returns 403 forbidden - `spec/requests/api/v1/mcp_spec.rb:232` - POST transition_ticket error handling broken This is an **unintended side effect** of the MCP permissions refactoring. The YAML anchor/alias refactoring removed `transition_ticket` from worker's allowed list. **Fix needed:** Add `transition_ticket` back to worker's allowed tools or update tests if this was intentional. --- ## CRITICAL: Missing Test Coverage This PR adds significant new functionality with virtually no test coverage: ### No Controller Tests (189 lines of code) `app/controllers/proposals_controller.rb` has ZERO tests: - No index action tests - No approve/reject action tests - No batch_action tests - No filter/sort tests - No error handling tests ### No Helper Tests (143 lines of code) `app/helpers/proposals_helper.rb` has ZERO tests: - No evidence_links rendering tests - No badge rendering tests - No edge case tests (missing data, malformed JSON) ### No View Tests `app/views/proposals/` has ZERO tests: - No feature specs for the proposals UI - No system tests for batch operations - No tests for rejection modal flow ### Only 6 Lines Added to Model Spec The PR adds only 2 minor tests to `spec/models/proposal_spec.rb` (likely just for rejection_reason column). According to project pattern (memory #58), new implementations require comprehensive test coverage including success cases, error cases, permission checks, and edge cases. --- ## CRITICAL: No Authorization on ProposalsController `app/controllers/proposals_controller.rb` has **NO AUTHORIZATION CHECKS**: ```ruby class ProposalsController < ApplicationController before_action :set_proposal, only: %i[approve reject show] before_action :set_metrics, only: %i[index] # NO AUTHENTICATION/AUTHORIZATION BEFORE_ACTIONS ``` **Security Issue:** Anyone who can access the web UI can approve/reject proposals. This should be restricted to: - Humans only (via authentication) - Possibly orchestrator role **Recommendation:** Add `before_action :authenticate_user!` and authorization checks (e.g., Pundit policy or similar). --- ## HIGH: SQL Injection Risk Fragility `app/controllers/proposals_controller.rb:173-178`: ```ruby def apply_sorting(query) priority_order = { high: 0, medium: 1, low: 2 } confidence_order = { high: 0, medium: 1, low: 2 } query.order(Arel.sql("ARRAY[#{priority_order[:high]}, #{priority_order[:medium]}, #{priority_order[:low]}]::int[] [priority::text::int] ASC")) .order(Arel.sql("ARRAY[#{confidence_order[:high]}, #{confidence_order[:medium]}, #{confidence_order[:low]}]::int[] [confidence::text::int] ASC")) ``` While the hardcoded values are currently safe (integers), this pattern is fragile. Consider: - Using a CASE statement in SQL - Using PostgreSQL's custom enum ordering - Extracting to a scope --- ## MEDIUM: Code Quality Issues ### 1. Manual Pagination Instead of Using Kaminari The PR adds `kaminari` as a dependency but implements manual pagination: ```ruby # Manual implementation in controller page = (params[:page] || 1).to_i per_page = 25 @proposals = @proposals.offset((page - 1) * per_page).limit(per_page) ``` **Should use:** `@proposals = @proposals.page(params[:page]).per(25)` ### 2. Inline JavaScript in View `app/views/proposals/index.html.haml` contains inline JavaScript at the bottom: ```haml :javascript function showRejectModal(selectedIds) { ... ``` **Should be:** In `app/javascript/controllers/` or a dedicated JS file following Rails 7 conventions. ### 3. Controller Does Too Much Metrics calculations in controller should be extracted to a query object or service: - `calculate_approval_rate` - `calculate_actions_today` ### 4. Hardcoded Values Duplication Priority, confidence, and status values are hardcoded across: - Controller sorting logic - Helper badge methods - Model validations **Should be:** Centralized as constants or enums. ### 5. Silent Error Handling Batch actions catch all exceptions silently: ```ruby rescue => e failed_count += 1 end ``` **Should:** Log the errors for debugging. --- ## LOW: Schema Drift in db/schema.rb `db/schema.rb` shows 145 deletions alongside the `rejection_reason` addition. This may indicate: - Unrelated schema changes included in this PR - Schema not properly regenerated Please verify all schema changes are intentional. --- ## Summary **Must Fix Before Approval:** 1. Fix failing tests (MCP permissions regression) 2. Add controller tests 3. Add authorization to ProposalsController 4. Add view/feature tests for the UI **Should Fix:** 5. Refactor SQL sorting to be more maintainable 6. Use Kaminari for pagination instead of manual implementation 7. Extract inline JavaScript 8. Extract metrics calculations from controller