## Code Review: PR #85 - Bare HTTP Status Check Enforcer ### Summary: **FAIL AUDIT** - Multiple Issues The PR attempts to implement runtime enforcement against bare HTTP status checks, but has several critical issues. --- ### Critical Issues **1. Unrelated Migration File (Blocks Merge)** - `db/migrate/20251231014126_add_last_activity_at_to_agent_sessions.rb` - Adds `last_activity_at` column - `db/schema.rb` - Updated schema - **These changes have nothing to do with ticket #140** (forbidding bare HTTP status checks) - This is a different feature that should be in its own PR/ticket **2. Stale Reference to Non-Existent File** - `spec/support/response_matchers.rb:8-9` references: ```ruby # See config/initializers/patch_rspec_for_better_test_errors.rb for the # monkeypatch that makes bare have_http_status fail at runtime. ``` - This file does not exist - The PR creates `spec/support/bare_status_check_enforcer.rb` instead - This comment needs updating to point to the correct file **3. Monkeypatch Effectiveness Unclear** - The implementation patches `RSpec::Rails::Matchers#have_http_status` as an instance method - RSpec's matcher system is complex - `have_http_status` is defined at module level but invoked through the matcher DSL - The approach may not actually work as intended - **No test demonstrates that the warning is shown** when a bare status check is used --- ### Acceptance Criteria Status | Criterion | Status | Notes | |-----------|--------|-------| | Monkeypatch warns on bare `have_http_status` | ⚠️ UNCLEAR | Implementation added but effectiveness unverified | | Warning message shows better patterns | ✅ PASS | Clear helpful message | | Custom matcher `have_successful_response` | ✅ EXISTS | Already in `response_matchers.rb` (not added by this PR) | | Helper `expect_api_success` | ✅ EXISTS | Already in `response_matchers.rb` (not added by this PR) | | Memory stored | ✅ PASS | Memory #61 contains the instruction | | RuboCop cop | N/A | Marked optional | --- ### What's Good - ✅ Warning message is well-formatted and helpful - ✅ Memory stored (ID #61) - ✅ Test suite passes (2 pre-existing failures unrelated to this PR) - ✅ Cleanup in `spec/features/ticket_workflow_spec.rb` removes dead code --- ### Required Actions Before Pass 1. **Remove unrelated migration changes** from this PR 2. **Update stale comment** in `response_matchers.rb` to reference correct file 3. **Add a test** demonstrating that bare status checks trigger the warning 4. **Verify monkeypatch actually works** - may need different approach --- ### Recommendation **FAIL AUDIT** - The PR should not be merged until: 1. Unrelated migration file is removed (or moved to a separate ticket) 2. The enforcer's effectiveness is verified with a test 3. Stale comments are updated