Set up Researcher agent: infrastructure, MCP tools, and guardrails

## Code Review: PR #82 (Updated - Re-review) ### Spec Pattern Search (REQUIRED) - Searched for existing spec patterns: `find spec -name "*mcp*" -o -name "*proposal*"` - Found existing spec files: - spec/requests/api/v1/mcp_spec.rb (MCP tool integration tests) - spec/models/proposal_spec.rb (Proposal model specs) - spec/models/mcp_synchronization_spec.rb (MCP role config validation) - spec/factories/agents.rb (now includes :researcher trait) ### Test Results - Test suite run: `bundle exec rspec` - Results: **629 examples, 0 failures, 10 pending** (pending unrelated to this PR) - MCP tests specifically: **57 examples, 0 failures** - Agent + MCP sync tests: **61 examples, 0 failures** ### Spec Coverage Check - Files changed in PR (7 files): 1. agents.rb (system prompt) - No tests needed (configuration) 2. app/models/agent.rb (researcher type, has_many :proposals) - Covered by agent_spec ✓ 3. app/controllers/api/v1/mcp_controller.rb (create_proposal, list_proposals tools + handlers) - **NOW TESTED** ✓ 4. config/tinker/mcp_permissions.yml (researcher permissions) - Covered by mcp_synchronization_spec ✓ 5. spec/factories/agents.rb (researcher trait) - Test support ✓ 6. spec/models/mcp_synchronization_spec.rb (updated known_roles) - ✓ 7. spec/requests/api/v1/mcp_spec.rb (**NEW TESTS ADDED**) - ✓ ### New Test Coverage (11 examples added) **create_proposal tests (4 examples):** - ✓ Creates a proposal via MCP (researcher only) - ✓ Returns access denied for workers attempting to create proposals - ✓ Validates required parameters - ✓ Applies default values for confidence and priority **list_proposals tests (7 examples):** - ✓ Lists proposals scoped to agent's project - ✓ Filters by status - ✓ Filters by proposal_type - ✓ Filters by confidence - ✓ Filters by priority - ✓ Respects limit parameter - ✓ Includes proposal details in response ### Findings - **Code quality:** Clean implementation, proper project-scoped queries, well-structured system prompt - **Security:** Permissions correctly configured (researcher is read-only + create_proposal) - **Implementation:** Matches ticket #134 requirements - **Test coverage:** Comprehensive - all new MCP tools have full integration tests ### Decision **PASS - All tests pass, comprehensive test coverage added** The previous review feedback has been fully addressed. The PR now includes: 1. 11 new integration tests for create_proposal and list_proposals MCP tools 2. Researcher factory trait for test support 3. All 629 tests passing with 0 failures

## Code Review: PR #82 ### What Was Reviewed This PR implements the Researcher agent infrastructure as specified in ticket #134: - Added `researcher` agent type with system prompt - Added `create_proposal` and `list_proposals` MCP tools - Configured permissions in `mcp_permissions.yml` - Added `has_many :proposals` association to Agent model ### Code Quality: PASS ✓ - Clean implementation following existing patterns - Proper project-scoped query in `handle_list_proposals` (proposals scoped to agent's project) - System prompt is well-structured with clear role boundaries and guardrails - Proper defaults for `confidence` and `priority` parameters ### Security: PASS ✓ - Researcher role is strictly read-only except for `create_proposal` - Properly blocked from: create_ticket, update_ticket, transition_ticket, ask_for_memory_deletion, send_message_to_agent, mark_busy, mark_idle - Guardrails in system prompt are comprehensive (0% noise target, clear forbidden actions) ### Test Coverage: FAIL ✗ **Critical Issue:** No tests for the new MCP tools in `spec/requests/api/v1/mcp_spec.rb` The PR adds two new tools (`create_proposal`, `list_proposals`) but has zero integration tests covering: - Success cases (creating proposals, listing with filters) - Error cases (invalid parameters, validation failures) - Permission checks (non-researcher agents blocked) - Edge cases (agent without project, empty results) The only test change was adding `researcher` to `known_roles` in `mcp_synchronization_spec.rb`. Every other MCP tool in the codebase has comprehensive test coverage (see `create_ticket`, `update_ticket`, `add_comment`, `list_members` tests). Following the same pattern, `create_proposal` and `list_proposals` should have similar coverage. ### Required Changes Add test coverage in `spec/requests/api/v1/mcp_spec.rb` following the existing pattern: ```ruby describe "create_proposal" do let(:researcher) { create(:agent, :researcher, project: project) } it "creates a proposal via MCP" it "returns access denied for workers attempting to create proposals" it "validates required parameters" end describe "list_proposals" do it "lists proposals scoped to agent's project" it "filters by status, type, confidence, priority" it "returns error when agent has no project" end ``` ### Test Suite Results All 629 examples passed, 10 pending (unrelated to this PR). ### Decision: FAIL_AUDIT The implementation is solid, but missing test coverage for new MCP tools is a blocker. Please add integration tests for `create_proposal` and `list_proposals` before this can be approved.

## Code Review: FAIL AUDIT ### CRITICAL BLOCKER: PR Not Mergeable **PR #79 is CLOSED with merge conflicts (CONFLICTING)** - The PR was closed without being merged and cannot be merged in its current state due to conflicts. ``` state: CLOSED mergeStateStatus: DIRTY mergedAt: null ``` The branch `feature/ticket-134-researcher-agent-infrastructure` needs to be rebased onto main and the conflicts resolved before this can be reviewed properly. --- ### Security Issues Found #### 1. Path Traversal Vulnerability in `read_code` (mcp_controller.rb) **Location:** `app/controllers/api/v1/mcp_controller.rb:947` **Issue:** The security check happens BEFORE `File.expand_path`, allowing bypass with `../` sequences. ```ruby # VULNERABLE CODE: rails_root = Rails.root.to_s unless file_path.start_with?(rails_root) # Check happens first return { error: "Access denied..." } end expanded_path = File.expand_path(file_path) # Expand happens AFTER ``` **Attack Example:** `/rails/app/../../etc/passwd` would pass the check but expand to `/etc/passwd`. **Fix:** Expand the path FIRST, then check: ```ruby expanded_path = File.expand_path(file_path) rails_root = Rails.root.to_s unless expanded_path.start_with?(rails_root) return { error: "Access denied..." } end ``` #### 2. SQL Injection Risk in Migration (insert_researcher_agent.rb) **Location:** `db/migrate/20251229035535_insert_researcher_agent.rb:18` **Issue:** Raw SQL with unsanitized `project.name.parameterize`: ```ruby VALUES ( '#{project.name.parameterize}-researcher', # Unsafe interpolation ``` While `parameterize` provides some sanitization, it's safer to use parameterized queries or sanitize explicitly. --- ### Missing Test Coverage The following new code has NO test coverage: 1. **`handle_create_proposal` handler** - No specs in `spec/requests/api/v1/mcp_spec.rb` 2. **`handle_read_code` handler** - No specs testing: - Normal file reading - Path traversal security - File size limits - Error handling 3. **Migration tests** - No specs for `InsertResearcherAgent` **Required tests:** ```ruby # spec/requests/api/v1/mcp_spec.rb describe "create_proposal" do it "creates a proposal for researcher agents" it "blocks non-researcher agents from creating proposals" it "validates required parameters" end describe "read_code" do it "reads files within Rails directory" it "blocks path traversal attacks" it "enforces file size limits" it "returns error for non-existent files" end ``` --- ### Positive Findings 1. **MCP Permissions (mcp_permissions.yml):** Properly configured - researcher has read-only access with appropriate blocks 2. **Agent Model:** Enum and capabilities correctly added 3. **TypeScript definitions:** Properly match Ruby MCP tools 4. **Guardrails:** Permission-level restrictions prevent unauthorized modifications --- ### Action Required 1. **Rebase the PR** onto main to resolve merge conflicts 2. **Fix path traversal vulnerability** in `read_code` 3. **Fix SQL injection risk** in migration 4. **Add comprehensive test coverage** for new MCP handlers 5. **Re-open PR** and request re-review