diff --git a/AGENTS.md b/AGENTS.md index 167b914..61ba5e4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,7 +9,7 @@ A Rails app for engineering design doc review, purpose-built for AI-assisted pla Most of the application logic lives in the **CoPlan Rails engine** (`engine/`), packaged as the `coplan` gem (path-based, in `Gemfile`). The top-level Rails app is a **thin host** that provides deployment configuration, ActiveAdmin, and app-specific glue. ### Engine (`engine/`) — where the code lives -- **Models** — all domain models live in `engine/app/models/coplan/` (Plan, PlanVersion, User, Comment, CommentThread, EditLease, EditSession, ApiToken, AutomatedPlanReviewer, PlanCollaborator) +- **Models** — all domain models live in `engine/app/models/coplan/` (Plan, PlanVersion, User, Comment, CommentThread, EditLease, EditSession, ApiToken, PlanCollaborator) - **Controllers** — web UI and API controllers in `engine/app/controllers/coplan/`, including `api/v1/` for the REST API - **Services** — all service objects in `engine/app/services/coplan/` (Plans::Create, Plans::ApplyOperations, AI providers, etc.) - **Policies** — authorization policies in `engine/app/policies/coplan/` @@ -49,7 +49,7 @@ Most of the application logic lives in the **CoPlan Rails engine** (`engine/`), ## Model Conventions -- Define valid values as **frozen constants** on the model (e.g., `Plan::STATUSES`, `AutomatedPlanReviewer::AI_PROVIDERS`) +- Define valid values as **frozen constants** on the model (e.g., `Plan::STATUSES`, `Comment::AUTHOR_TYPES`) - Use `inclusion:` validations against those constants - Use `after_initialize` for defaults on JSON array columns (e.g., `self.tags ||= []`) - Service objects live in `app/services/` namespaced by model (e.g., `Plans::Create`) @@ -80,7 +80,6 @@ Most of the application logic lives in the **CoPlan Rails engine** (`engine/`), - `db/seeds.rb` must be **idempotent** — use `find_or_create_by!` or guard with count checks - Seeds should provide enough data to demo features from a fresh checkout -- `AutomatedPlanReviewer.create_defaults_for(org)` handles reviewer seeding ## Code Review @@ -98,7 +97,6 @@ Most of the application logic lives in the **CoPlan Rails engine** (`engine/`), - **Brainstorm** plans are private; **considering+** are published to the org - **Editing model**: humans comment, AI agents apply edits via semantic operations (`replace_exact`, `insert_under_heading`, `delete_paragraph_containing`) - **Edit leases**: one agent edits at a time, enforced by a lease with TTL -- **Cloud Personas** (AutomatedPlanReviewers): server-side prompt templates that run as SolidQueue jobs - **Versions are immutable** — every edit creates a new PlanVersion with full provenance ## Comment & Review UX diff --git a/app/admin/automated_plan_reviewers.rb b/app/admin/automated_plan_reviewers.rb deleted file mode 100644 index 21c70a2..0000000 --- a/app/admin/automated_plan_reviewers.rb +++ /dev/null @@ -1,31 +0,0 @@ -ActiveAdmin.register CoPlan::AutomatedPlanReviewer, as: "AutomatedPlanReviewer" do - permit_params :key, :name, :prompt_text, :enabled, :ai_provider, :ai_model, trigger_statuses: [] - - index do - selectable_column - id_column - column :key - column :name - column :enabled - column :ai_provider - column :ai_model - column :trigger_statuses - column :created_at - actions - end - - show do - attributes_table do - row :id - row :key - row :name - row :prompt_text - row :enabled - row :ai_provider - row :ai_model - row :trigger_statuses - row :created_at - row :updated_at - end - end -end diff --git a/db/schema.rb b/db/schema.rb index 1a0c1cc..4d8f534 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_06_01_202711) do +ActiveRecord::Schema[8.1].define(version: 2026_06_02_170000) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" @@ -39,19 +39,6 @@ t.index ["user_id"], name: "index_coplan_api_tokens_on_user_id" end - create_table "coplan_automated_plan_reviewers", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| - t.string "ai_model", null: false - t.string "ai_provider", default: "openai", null: false - t.datetime "created_at", null: false - t.boolean "enabled", default: true, null: false - t.string "key", null: false - t.string "name", null: false - t.text "prompt_text", null: false - t.json "trigger_statuses", null: false - t.datetime "updated_at", null: false - t.index ["key"], name: "index_coplan_automated_plan_reviewers_on_key", unique: true - end - create_table "coplan_comment_threads", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "addressed_in_plan_version_id", limit: 36 t.text "anchor_context" diff --git a/db/seeds.rb b/db/seeds.rb index 4d72805..d510d7f 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -125,7 +125,4 @@ q3 = CoPlan::Plan.find_by(title: "Q3 Product Roadmap") q3.tag_names = ["roadmap", "product"] if q3 && q3.tags.empty? -puts "Seeding automated plan reviewers..." -CoPlan::AutomatedPlanReviewer.create_defaults - -puts "Done! #{CoPlan::User.count} users, #{CoPlan::Plan.count} plans, #{CoPlan::CommentThread.count} threads, #{CoPlan::Comment.count} comments, #{CoPlan::ApiToken.count} API tokens, #{CoPlan::AutomatedPlanReviewer.count} reviewers." +puts "Done! #{CoPlan::User.count} users, #{CoPlan::Plan.count} plans, #{CoPlan::CommentThread.count} threads, #{CoPlan::Comment.count} comments, #{CoPlan::ApiToken.count} API tokens." diff --git a/engine/app/controllers/coplan/api/v1/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index ae2a077..6875a4e 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -98,7 +98,6 @@ def update before: old_status, after: @plan.status, actor_type: api_author_type, actor_id: api_actor_id ) - Plans::TriggerAutomatedReviews.call(plan: @plan, new_status: permitted[:status], triggered_by: current_user) if @plan.status == "considering" && old_status != "considering" CoPlan::Analytics.track( "plan_published", diff --git a/engine/app/controllers/coplan/automated_reviews_controller.rb b/engine/app/controllers/coplan/automated_reviews_controller.rb deleted file mode 100644 index ecf68e8..0000000 --- a/engine/app/controllers/coplan/automated_reviews_controller.rb +++ /dev/null @@ -1,29 +0,0 @@ -module CoPlan - class AutomatedReviewsController < ApplicationController - before_action :set_plan - before_action :set_reviewer, only: [:create] - - def create - authorize!(@plan, :update?) - - AutomatedReviewJob.perform_later( - plan_id: @plan.id, - reviewer_id: @reviewer.id, - plan_version_id: @plan.current_plan_version_id, - triggered_by: current_user - ) - - redirect_to plan_path(@plan), notice: "#{@reviewer.name} review queued." - end - - private - - def set_plan - @plan = Plan.find(params[:plan_id]) - end - - def set_reviewer - @reviewer = AutomatedPlanReviewer.enabled.find(params[:reviewer_id]) - end - end -end diff --git a/engine/app/controllers/coplan/plans_controller.rb b/engine/app/controllers/coplan/plans_controller.rb index 09b5aee..10498ce 100644 --- a/engine/app/controllers/coplan/plans_controller.rb +++ b/engine/app/controllers/coplan/plans_controller.rb @@ -103,7 +103,6 @@ def update_status before: old_status, after: new_status ) - Plans::TriggerAutomatedReviews.call(plan: @plan, new_status: new_status, triggered_by: current_user) if new_status == "considering" && old_status != "considering" CoPlan::Analytics.track( "plan_published", diff --git a/engine/app/helpers/coplan/comments_helper.rb b/engine/app/helpers/coplan/comments_helper.rb index 98c597a..ff808b5 100644 --- a/engine/app/helpers/coplan/comments_helper.rb +++ b/engine/app/helpers/coplan/comments_helper.rb @@ -2,12 +2,7 @@ module CoPlan module CommentsHelper def comment_author_name(comment) user = comment_author_user(comment) - user_name = user&.name - user_name ||= if comment.author_type == "cloud_persona" - AutomatedPlanReviewer.find_by(id: comment.author_id)&.name || "Reviewer" - else - comment.author_type - end + user_name = user&.name || comment.author_type comment.agent_name.present? ? "#{comment.agent_name} (via #{user_name})" : user_name end diff --git a/engine/app/jobs/coplan/automated_review_job.rb b/engine/app/jobs/coplan/automated_review_job.rb deleted file mode 100644 index 4b235d0..0000000 --- a/engine/app/jobs/coplan/automated_review_job.rb +++ /dev/null @@ -1,71 +0,0 @@ -module CoPlan - class AutomatedReviewJob < ApplicationJob - queue_as :default - - discard_on AiProviders::OpenAi::Error - discard_on AiProviders::Anthropic::Error - - def perform(plan_id:, reviewer_id:, plan_version_id:, triggered_by: nil) - plan = Plan.find(plan_id) - reviewer = AutomatedPlanReviewer.find(reviewer_id) - version = PlanVersion.find(plan_version_id) - - return unless reviewer.enabled? - - response = call_ai_provider(reviewer, version.content_markdown) - feedback_items = Plans::ReviewResponseParser.call(response, plan_content: version.content_markdown) - - create_review_comments(plan, version, reviewer, feedback_items, triggered_by) - end - - private - - def call_ai_provider(reviewer, content) - system_prompt = Plans::ReviewPromptFormatter.call(reviewer_prompt: reviewer.prompt_text) - provider_class = resolve_provider(reviewer.ai_provider) - provider_class.call( - system_prompt: system_prompt, - user_content: content, - model: reviewer.ai_model - ) - end - - def resolve_provider(provider_name) - case provider_name - when "openai" then AiProviders::OpenAi - when "anthropic" then AiProviders::Anthropic - else raise ArgumentError, "Unknown AI provider: #{provider_name}" - end - end - - def create_review_comments(plan, version, reviewer, feedback_items, triggered_by) - created_by = triggered_by || plan.created_by_user - - feedback_items.each do |item| - thread = plan.comment_threads.create!( - plan_version: version, - created_by_user: created_by, - anchor_text: item[:anchor_text], - status: "pending" - ) - - thread.comments.create!( - author_type: AutomatedPlanReviewer::ACTOR_TYPE, - author_id: reviewer.id, - body_markdown: item[:comment] - ) - - broadcast_new_thread(plan, thread) - end - end - - def broadcast_new_thread(plan, thread) - Broadcaster.append_to( - plan, - target: "plan-threads", - partial: "coplan/comment_threads/thread_popover", - locals: { thread: thread, plan: plan } - ) - end - end -end diff --git a/engine/app/models/coplan/automated_plan_reviewer.rb b/engine/app/models/coplan/automated_plan_reviewer.rb deleted file mode 100644 index 574ea57..0000000 --- a/engine/app/models/coplan/automated_plan_reviewer.rb +++ /dev/null @@ -1,62 +0,0 @@ -module CoPlan - class AutomatedPlanReviewer < ApplicationRecord - ACTOR_TYPE = "cloud_persona" - AI_PROVIDERS = %w[openai anthropic].freeze - - DEFAULT_REVIEWERS = [ - { key: "security-reviewer", name: "Security Reviewer", prompt_file: "prompts/reviewers/security.md", - trigger_statuses: [ "considering" ], ai_model: "gpt-4o" }, - { key: "scalability-reviewer", name: "Scalability Reviewer", prompt_file: "prompts/reviewers/scalability.md", - trigger_statuses: [ "considering", "developing" ], ai_model: "gpt-4o" }, - { key: "routing-reviewer", name: "Routing Reviewer", prompt_file: "prompts/reviewers/routing.md", - trigger_statuses: [], ai_model: "gpt-4o" } - ].freeze - - after_initialize { self.trigger_statuses ||= [] } - - validates :key, presence: true, - format: { with: /\A[a-z0-9-]+\z/, message: "only allows lowercase letters, numbers, and hyphens" } - validates :key, uniqueness: true - validates :name, presence: true - validates :prompt_text, presence: true - validates :ai_provider, presence: true, inclusion: { in: AI_PROVIDERS } - validates :ai_model, presence: true - validate :validate_trigger_statuses - - scope :enabled, -> { where(enabled: true) } - - def self.create_defaults - DEFAULT_REVIEWERS.each do |template| - find_or_create_by!(key: template[:key]) do |r| - r.name = template[:name] - r.prompt_text = File.read(CoPlan::Engine.root.join(template[:prompt_file])) - r.trigger_statuses = template[:trigger_statuses] - r.ai_model = template[:ai_model] - end - end - end - - def self.ransackable_attributes(auth_object = nil) - %w[id key name enabled ai_provider ai_model created_at updated_at] - end - - def self.ransackable_associations(auth_object = nil) - %w[] - end - - def triggers_on_status?(status) - trigger_statuses.include?(status.to_s) - end - - private - - def validate_trigger_statuses - return if trigger_statuses.blank? - - invalid = trigger_statuses - Plan::STATUSES - if invalid.any? - errors.add(:trigger_statuses, "contains invalid status: #{invalid.join(', ')}. Valid statuses are: #{Plan::STATUSES.join(', ')}") - end - end - end -end diff --git a/engine/app/services/coplan/ai.rb b/engine/app/services/coplan/ai.rb index 4ce9d6a..a26ecf2 100644 --- a/engine/app/services/coplan/ai.rb +++ b/engine/app/services/coplan/ai.rb @@ -3,11 +3,6 @@ module CoPlan # which underlying provider runs the prompt. Use this from any place # that just wants "an AI" (e.g. SummarizePlanJob). # - # Provider-specific jobs that need to pin a model or provider per call - # (e.g. AutomatedReviewJob, where each reviewer is configured with its - # own provider+model) should keep calling AiProviders::OpenAi / - # AiProviders::Anthropic directly. - # # The provider chosen here is an implementation detail; swap it without # touching callers. Raises CoPlan::Ai::Error on provider failure so # callers can `discard_on` without knowing which provider is in use. diff --git a/engine/app/services/coplan/ai_providers/anthropic.rb b/engine/app/services/coplan/ai_providers/anthropic.rb deleted file mode 100644 index 820e18e..0000000 --- a/engine/app/services/coplan/ai_providers/anthropic.rb +++ /dev/null @@ -1,21 +0,0 @@ -module CoPlan - module AiProviders - class Anthropic - def self.call(system_prompt:, user_content:, model: "claude-sonnet-4-20250514") - new(system_prompt:, user_content:, model:).call - end - - def initialize(system_prompt:, user_content:, model:) - @system_prompt = system_prompt - @user_content = user_content - @model = model - end - - def call - raise Error, "Anthropic provider not yet implemented. Use OpenAI for now." - end - - class Error < StandardError; end - end - end -end diff --git a/engine/app/services/coplan/plans/review_prompt_formatter.rb b/engine/app/services/coplan/plans/review_prompt_formatter.rb deleted file mode 100644 index 6aca601..0000000 --- a/engine/app/services/coplan/plans/review_prompt_formatter.rb +++ /dev/null @@ -1,25 +0,0 @@ -module CoPlan - module Plans - class ReviewPromptFormatter - RESPONSE_FORMAT_INSTRUCTIONS = <<~INSTRUCTIONS.freeze - You MUST respond with a JSON array of feedback items. Each item is an object with two keys: - - "anchor_text": An exact substring copied verbatim from the plan document that this feedback applies to. Keep it short (a phrase or single sentence). Must match the plan text exactly. Use null for general feedback not tied to specific text. - - "comment": Your feedback in Markdown. Be concise and actionable. - - Example response: - ```json - [ - {"anchor_text": "API tokens scoped to a user", "comment": "Consider adding token expiration by default. Long-lived tokens without expiry are a common security risk."}, - {"anchor_text": null, "comment": "Overall the plan looks solid. One general concern: there's no mention of audit logging for administrative actions."} - ] - ``` - - Return ONLY the JSON array. No other text before or after it. - INSTRUCTIONS - - def self.call(reviewer_prompt:) - "#{reviewer_prompt}\n\n#{RESPONSE_FORMAT_INSTRUCTIONS}" - end - end - end -end diff --git a/engine/app/services/coplan/plans/review_response_parser.rb b/engine/app/services/coplan/plans/review_response_parser.rb deleted file mode 100644 index d5125cf..0000000 --- a/engine/app/services/coplan/plans/review_response_parser.rb +++ /dev/null @@ -1,65 +0,0 @@ -module CoPlan - module Plans - class ReviewResponseParser - def self.call(response_text, plan_content:) - new(response_text, plan_content:).call - end - - def initialize(response_text, plan_content:) - @response_text = response_text - @plan_content = plan_content - end - - def call - items = parse_json - items.filter_map { |item| normalize_item(item) } - end - - private - - def parse_json - json_text = extract_json_from_response - parsed = JSON.parse(json_text) - - unless parsed.is_a?(Array) - return fallback_single_comment - end - - parsed - rescue JSON::ParserError - fallback_single_comment - end - - def extract_json_from_response - # Strip markdown code fences if present - text = @response_text.strip - if text.start_with?("```") - text = text.sub(/\A```(?:json)?\s*\n?/, "").sub(/\n?```\s*\z/, "") - end - text - end - - def normalize_item(item) - return nil unless item.is_a?(Hash) - - anchor = item["anchor_text"].presence - comment = item["comment"].to_s.strip - - return nil if comment.blank? - - # Verify anchor_text actually exists in the plan content - if anchor && !@plan_content.include?(anchor) - # Anchor doesn't match — demote to unanchored with the quote in the comment - comment = "> #{anchor}\n\n#{comment}" - anchor = nil - end - - { anchor_text: anchor, comment: comment } - end - - def fallback_single_comment - [{ "anchor_text" => nil, "comment" => @response_text }] - end - end - end -end diff --git a/engine/app/services/coplan/plans/trigger_automated_reviews.rb b/engine/app/services/coplan/plans/trigger_automated_reviews.rb deleted file mode 100644 index 8730a29..0000000 --- a/engine/app/services/coplan/plans/trigger_automated_reviews.rb +++ /dev/null @@ -1,33 +0,0 @@ -module CoPlan - module Plans - class TriggerAutomatedReviews - def self.call(plan:, new_status:, triggered_by:) - new(plan:, new_status:, triggered_by:).call - end - - def initialize(plan:, new_status:, triggered_by:) - @plan = plan - @new_status = new_status - @triggered_by = triggered_by - end - - def call - version_id = @plan.current_plan_version_id - return unless version_id - - reviewers = CoPlan::AutomatedPlanReviewer.enabled - - reviewers.each do |reviewer| - next unless reviewer.triggers_on_status?(@new_status) - - AutomatedReviewJob.perform_later( - plan_id: @plan.id, - reviewer_id: reviewer.id, - plan_version_id: version_id, - triggered_by: @triggered_by - ) - end - end - end - end -end diff --git a/engine/app/views/coplan/plans/_header.html.erb b/engine/app/views/coplan/plans/_header.html.erb index 663fdea..8e56518 100644 --- a/engine/app/views/coplan/plans/_header.html.erb +++ b/engine/app/views/coplan/plans/_header.html.erb @@ -13,20 +13,5 @@
<%= render partial: "coplan/plans/viewers", locals: { viewers: CoPlan::PlanViewer.active_viewers_for(plan), current_user: current_user } %> - <% if CoPlan::AutomatedPlanReviewer.enabled.any? %> - - <% end %>
diff --git a/engine/config/routes.rb b/engine/config/routes.rb index d17d1c7..d99b94b 100644 --- a/engine/config/routes.rb +++ b/engine/config/routes.rb @@ -7,7 +7,6 @@ get :diff, on: :member end resources :references, controller: "references", only: [:create, :destroy] - resources :automated_reviews, only: [:create] resources :comment_threads, only: [:create] do member do patch :resolve diff --git a/engine/db/migrate/20260602170000_drop_coplan_automated_plan_reviewers.rb b/engine/db/migrate/20260602170000_drop_coplan_automated_plan_reviewers.rb new file mode 100644 index 0000000..1a3ea91 --- /dev/null +++ b/engine/db/migrate/20260602170000_drop_coplan_automated_plan_reviewers.rb @@ -0,0 +1,20 @@ +class DropCoplanAutomatedPlanReviewers < ActiveRecord::Migration[8.0] + def up + drop_table :coplan_automated_plan_reviewers + end + + def down + create_table :coplan_automated_plan_reviewers, id: { type: :string, limit: 36 } do |t| + t.string :key, null: false + t.string :name, null: false + t.text :prompt_text, null: false + t.string :ai_provider, default: "openai", null: false + t.string :ai_model, null: false + t.json :trigger_statuses, null: false + t.boolean :enabled, default: true, null: false + t.timestamps + end + + add_index :coplan_automated_plan_reviewers, :key, unique: true + end +end diff --git a/engine/prompts/reviewers/routing.md b/engine/prompts/reviewers/routing.md deleted file mode 100644 index 4ad7b7a..0000000 --- a/engine/prompts/reviewers/routing.md +++ /dev/null @@ -1,14 +0,0 @@ -You are an architecture reviewer focused on system routing and API design. - -Review the following plan for routing and API design concerns. Focus on: - -- REST conventions and resource naming -- URL structure and consistency -- API versioning strategy -- Request/response payload design -- Error handling patterns -- Middleware and filter chains - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/engine/prompts/reviewers/scalability.md b/engine/prompts/reviewers/scalability.md deleted file mode 100644 index 1c23ee5..0000000 --- a/engine/prompts/reviewers/scalability.md +++ /dev/null @@ -1,14 +0,0 @@ -You are a scalability reviewer for technical plans and proposals. - -Review the following plan for scalability concerns. Focus on: - -- Database query patterns and indexing strategies -- Caching opportunities and cache invalidation -- Background job volume and queue contention -- API rate limits and throughput bottlenecks -- Data growth projections and storage implications -- Horizontal vs vertical scaling considerations - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/engine/prompts/reviewers/security.md b/engine/prompts/reviewers/security.md deleted file mode 100644 index dcd9a82..0000000 --- a/engine/prompts/reviewers/security.md +++ /dev/null @@ -1,14 +0,0 @@ -You are a security reviewer for technical plans and proposals. - -Review the following plan for security concerns. Focus on: - -- Authentication and authorization gaps -- Data exposure or leakage risks -- Input validation and injection vulnerabilities -- Secrets management issues -- Network and infrastructure security -- Compliance considerations - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/prompts/reviewers/routing.md b/prompts/reviewers/routing.md deleted file mode 100644 index 4ad7b7a..0000000 --- a/prompts/reviewers/routing.md +++ /dev/null @@ -1,14 +0,0 @@ -You are an architecture reviewer focused on system routing and API design. - -Review the following plan for routing and API design concerns. Focus on: - -- REST conventions and resource naming -- URL structure and consistency -- API versioning strategy -- Request/response payload design -- Error handling patterns -- Middleware and filter chains - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/prompts/reviewers/scalability.md b/prompts/reviewers/scalability.md deleted file mode 100644 index 1c23ee5..0000000 --- a/prompts/reviewers/scalability.md +++ /dev/null @@ -1,14 +0,0 @@ -You are a scalability reviewer for technical plans and proposals. - -Review the following plan for scalability concerns. Focus on: - -- Database query patterns and indexing strategies -- Caching opportunities and cache invalidation -- Background job volume and queue contention -- API rate limits and throughput bottlenecks -- Data growth projections and storage implications -- Horizontal vs vertical scaling considerations - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/prompts/reviewers/security.md b/prompts/reviewers/security.md deleted file mode 100644 index dcd9a82..0000000 --- a/prompts/reviewers/security.md +++ /dev/null @@ -1,14 +0,0 @@ -You are a security reviewer for technical plans and proposals. - -Review the following plan for security concerns. Focus on: - -- Authentication and authorization gaps -- Data exposure or leakage risks -- Input validation and injection vulnerabilities -- Secrets management issues -- Network and infrastructure security -- Compliance considerations - -Be specific. Reference the relevant sections of the plan. Suggest concrete improvements where possible. - -Respond in Markdown. Keep your review concise and actionable. diff --git a/spec/factories/automated_plan_reviewers.rb b/spec/factories/automated_plan_reviewers.rb deleted file mode 100644 index eef8bd5..0000000 --- a/spec/factories/automated_plan_reviewers.rb +++ /dev/null @@ -1,11 +0,0 @@ -FactoryBot.define do - factory :automated_plan_reviewer, class: "CoPlan::AutomatedPlanReviewer" do - sequence(:key) { |n| "reviewer-#{n}" } - sequence(:name) { |n| "Reviewer #{n}" } - prompt_text { "You are a reviewer. Review the plan." } - enabled { true } - trigger_statuses { [] } - ai_provider { "openai" } - ai_model { "gpt-4o" } - end -end diff --git a/spec/jobs/automated_review_job_spec.rb b/spec/jobs/automated_review_job_spec.rb deleted file mode 100644 index 0262382..0000000 --- a/spec/jobs/automated_review_job_spec.rb +++ /dev/null @@ -1,115 +0,0 @@ -require "rails_helper" - -RSpec.describe CoPlan::AutomatedReviewJob, type: :job do - let(:plan) { create(:plan, :considering) } - let(:user) { plan.created_by_user } - let(:reviewer) { create(:automated_plan_reviewer) } - let(:version) { plan.current_plan_version } - let(:plan_content) { version.content_markdown } - - let(:structured_response) do - '[ - {"anchor_text": "Plan Content", "comment": "The title should be more specific."}, - {"anchor_text": "Some content here", "comment": "Expand on this section."}, - {"anchor_text": null, "comment": "Overall looks good."} - ]' - end - - let(:perform_args) { { plan_id: plan.id, reviewer_id: reviewer.id, plan_version_id: version.id, triggered_by: user } } - - before do - allow(CoPlan::AiProviders::OpenAi).to receive(:call).and_return(structured_response) - end - - describe "#perform" do - it "creates one comment thread per feedback item" do - expect { - described_class.perform_now(**perform_args) - }.to change(CoPlan::CommentThread, :count).by(3) - .and change(CoPlan::Comment, :count).by(3) - end - - it "anchors threads to matching plan text" do - described_class.perform_now(**perform_args) - - threads = CoPlan::CommentThread.order(:created_at).last(3) - expect(threads[0].anchor_text).to eq("Plan Content") - expect(threads[1].anchor_text).to eq("Some content here") - expect(threads[2].anchor_text).to be_nil - end - - it "attaches threads to the pinned version, not the current one" do - old_version = version - new_version = create(:plan_version, plan: plan, revision: 2) - plan.update!(current_plan_version: new_version, current_revision: 2) - - described_class.perform_now(plan_id: plan.id, reviewer_id: reviewer.id, plan_version_id: old_version.id, triggered_by: user) - - CoPlan::CommentThread.last(3).each do |thread| - expect(thread.plan_version).to eq(old_version) - end - end - - it "creates comments with cloud_persona author type" do - described_class.perform_now(**perform_args) - - CoPlan::Comment.last(3).each do |comment| - expect(comment.author_type).to eq("cloud_persona") - expect(comment.author_id).to eq(reviewer.id) - end - end - - it "sets the triggered_by user as the thread creator" do - other_user = create(:coplan_user) - described_class.perform_now(**perform_args.merge(triggered_by: other_user)) - - CoPlan::CommentThread.last(3).each do |thread| - expect(thread.created_by_user).to eq(other_user) - end - end - - it "falls back to plan author when triggered_by is nil" do - described_class.perform_now(**perform_args.except(:triggered_by)) - - CoPlan::CommentThread.last(3).each do |thread| - expect(thread.created_by_user).to eq(plan.created_by_user) - end - end - - it "passes the formatted prompt to the AI provider" do - described_class.perform_now(**perform_args) - - expect(CoPlan::AiProviders::OpenAi).to have_received(:call).with( - system_prompt: CoPlan::Plans::ReviewPromptFormatter.call(reviewer_prompt: reviewer.prompt_text), - user_content: plan_content, - model: reviewer.ai_model - ) - end - - it "falls back to a single unanchored comment for non-JSON AI responses" do - allow(CoPlan::AiProviders::OpenAi).to receive(:call).and_return("Plain text review with no JSON.") - - expect { - described_class.perform_now(**perform_args) - }.to change(CoPlan::CommentThread, :count).by(1) - - thread = CoPlan::CommentThread.last - expect(thread.anchor_text).to be_nil - expect(thread.comments.first.body_markdown).to eq("Plain text review with no JSON.") - end - - it "does nothing if the reviewer is disabled" do - reviewer.update!(enabled: false) - - expect { - described_class.perform_now(**perform_args) - }.not_to change(CoPlan::CommentThread, :count) - end - - it "enqueues on the default queue" do - expect { - described_class.perform_later(**perform_args) - }.to have_enqueued_job(described_class).on_queue("default") - end - end -end diff --git a/spec/models/automated_plan_reviewer_spec.rb b/spec/models/automated_plan_reviewer_spec.rb deleted file mode 100644 index 9da5a83..0000000 --- a/spec/models/automated_plan_reviewer_spec.rb +++ /dev/null @@ -1,146 +0,0 @@ -require "rails_helper" - -RSpec.describe CoPlan::AutomatedPlanReviewer, type: :model do - it "is valid with valid attributes" do - reviewer = create(:automated_plan_reviewer) - expect(reviewer).to be_valid - end - - it "requires key" do - reviewer = build(:automated_plan_reviewer, key: nil) - expect(reviewer).not_to be_valid - expect(reviewer.errors[:key]).to include("can't be blank") - end - - it "requires name" do - reviewer = build(:automated_plan_reviewer, name: nil) - expect(reviewer).not_to be_valid - expect(reviewer.errors[:name]).to include("can't be blank") - end - - it "requires prompt_text" do - reviewer = build(:automated_plan_reviewer, prompt_text: nil) - expect(reviewer).not_to be_valid - expect(reviewer.errors[:prompt_text]).to include("can't be blank") - end - - it "requires ai_model" do - reviewer = build(:automated_plan_reviewer, ai_model: nil) - expect(reviewer).not_to be_valid - expect(reviewer.errors[:ai_model]).to include("can't be blank") - end - - it "validates key uniqueness globally" do - existing = create(:automated_plan_reviewer, key: "unique-key") - duplicate = build(:automated_plan_reviewer, key: "unique-key") - expect(duplicate).not_to be_valid - expect(duplicate.errors[:key]).to include("has already been taken") - end - - it "validates key format" do - reviewer = build(:automated_plan_reviewer, key: "Invalid Key!") - expect(reviewer).not_to be_valid - expect(reviewer.errors[:key]).to include("only allows lowercase letters, numbers, and hyphens") - end - - it "validates key uniqueness" do - existing = create(:automated_plan_reviewer) - duplicate = build(:automated_plan_reviewer, key: existing.key) - expect(duplicate).not_to be_valid - expect(duplicate.errors[:key]).to include("has already been taken") - end - - it "rejects duplicate key globally" do - create(:automated_plan_reviewer, key: "same-key") - reviewer = build(:automated_plan_reviewer, key: "same-key") - expect(reviewer).not_to be_valid - expect(reviewer.errors[:key]).to include("has already been taken") - end - - it "stores prompt text" do - reviewer = create(:automated_plan_reviewer, prompt_text: "Review for security issues") - expect(reviewer.prompt_text).to include("security") - end - - it "triggers_on_status? returns true for matching status" do - reviewer = create(:automated_plan_reviewer, trigger_statuses: ["considering"]) - expect(reviewer.triggers_on_status?("considering")).to be true - end - - it "triggers_on_status? returns false for non-matching status" do - reviewer = create(:automated_plan_reviewer, trigger_statuses: ["considering"]) - expect(reviewer.triggers_on_status?("brainstorm")).to be false - end - - it "enabled scope returns only enabled reviewers" do - enabled_reviewer = create(:automated_plan_reviewer, enabled: true) - disabled_reviewer = create(:automated_plan_reviewer, enabled: false) - enabled = CoPlan::AutomatedPlanReviewer.enabled - expect(enabled).to include(enabled_reviewer) - expect(enabled).not_to include(disabled_reviewer) - end - - it "validates ai_provider inclusion" do - reviewer = build(:automated_plan_reviewer, ai_provider: "unknown-provider") - expect(reviewer).not_to be_valid - expect(reviewer.errors[:ai_provider]).to include("is not included in the list") - end - - it "accepts valid ai_providers" do - CoPlan::AutomatedPlanReviewer::AI_PROVIDERS.each do |provider| - reviewer = build(:automated_plan_reviewer, ai_provider: provider) - expect(reviewer).to be_valid, "Expected #{provider} to be valid" - end - end - - it "defaults ai_provider to openai" do - reviewer = CoPlan::AutomatedPlanReviewer.new - expect(reviewer.ai_provider).to eq("openai") - end - - it "validates trigger_statuses against Plan::STATUSES" do - reviewer = build(:automated_plan_reviewer, trigger_statuses: ["considering", "invalid-status"]) - expect(reviewer).not_to be_valid - expect(reviewer.errors[:trigger_statuses].any? { |e| e.include?("invalid-status") }).to be true - end - - it "accepts valid trigger_statuses" do - reviewer = build(:automated_plan_reviewer, trigger_statuses: CoPlan::Plan::STATUSES.dup) - expect(reviewer).to be_valid - end - - it "accepts empty trigger_statuses" do - reviewer = build(:automated_plan_reviewer, trigger_statuses: []) - expect(reviewer).to be_valid - end - - it "defaults trigger_statuses to empty array" do - reviewer = CoPlan::AutomatedPlanReviewer.new - expect(reviewer.trigger_statuses).to eq([]) - end - - it "defaults enabled to true" do - reviewer = CoPlan::AutomatedPlanReviewer.new - expect(reviewer.enabled).to be true - end - - it "create_defaults creates default reviewers" do - CoPlan::AutomatedPlanReviewer.destroy_all - CoPlan::AutomatedPlanReviewer.create_defaults - reviewers = CoPlan::AutomatedPlanReviewer.all - expect(reviewers.count).to eq(3) - expect(reviewers.pluck(:key).sort).to eq(%w[routing-reviewer scalability-reviewer security-reviewer]) - reviewers.each do |r| - expect(r.prompt_text).to be_present - expect(r.ai_model).to be_present - end - end - - it "create_defaults is idempotent" do - CoPlan::AutomatedPlanReviewer.destroy_all - CoPlan::AutomatedPlanReviewer.create_defaults - initial_count = CoPlan::AutomatedPlanReviewer.count - CoPlan::AutomatedPlanReviewer.create_defaults - expect(CoPlan::AutomatedPlanReviewer.count).to eq(initial_count) - end -end diff --git a/spec/requests/automated_reviews_spec.rb b/spec/requests/automated_reviews_spec.rb deleted file mode 100644 index f47f4b8..0000000 --- a/spec/requests/automated_reviews_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -require "rails_helper" - -RSpec.describe "AutomatedReviews", type: :request do - let(:user) { create(:coplan_user, :admin) } - let(:plan) { create(:plan, :considering, created_by_user: user) } - let!(:reviewer) { create(:automated_plan_reviewer, enabled: true) } - - before { sign_in_as(user) } - - describe "POST #create" do - it "enqueues an AutomatedReviewJob" do - expect { - post plan_automated_reviews_path(plan), params: { reviewer_id: reviewer.id } - }.to have_enqueued_job(CoPlan::AutomatedReviewJob).with( - plan_id: plan.id, - reviewer_id: reviewer.id, - plan_version_id: plan.current_plan_version_id, - triggered_by: user - ) - end - - it "redirects to the plan with a notice" do - post plan_automated_reviews_path(plan), params: { reviewer_id: reviewer.id } - expect(response).to redirect_to(plan_path(plan)) - expect(flash[:notice]).to include(reviewer.name) - end - - it "returns 404 for disabled reviewers" do - reviewer.update!(enabled: false) - post plan_automated_reviews_path(plan), params: { reviewer_id: reviewer.id } - expect(response).to have_http_status(:not_found) - end - end -end diff --git a/spec/services/plans/review_prompt_formatter_spec.rb b/spec/services/plans/review_prompt_formatter_spec.rb deleted file mode 100644 index e506863..0000000 --- a/spec/services/plans/review_prompt_formatter_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require "rails_helper" - -RSpec.describe CoPlan::Plans::ReviewPromptFormatter do - describe ".call" do - it "appends JSON response format instructions to the reviewer prompt" do - result = described_class.call(reviewer_prompt: "Review for security issues.") - - expect(result).to start_with("Review for security issues.") - expect(result).to include("JSON array") - expect(result).to include("anchor_text") - expect(result).to include("comment") - end - - it "preserves the original reviewer prompt" do - original = "You are a scalability reviewer.\n\nFocus on database queries." - result = described_class.call(reviewer_prompt: original) - - expect(result).to include(original) - end - end -end diff --git a/spec/services/plans/review_response_parser_spec.rb b/spec/services/plans/review_response_parser_spec.rb deleted file mode 100644 index aacf4a3..0000000 --- a/spec/services/plans/review_response_parser_spec.rb +++ /dev/null @@ -1,100 +0,0 @@ -require "rails_helper" - -RSpec.describe CoPlan::Plans::ReviewResponseParser do - let(:plan_content) { "# My Plan\n\nWe use API tokens scoped to a user.\n\nNo rate limiting yet." } - - describe ".call" do - it "parses a valid JSON array response into feedback items" do - response = '[ - {"anchor_text": "API tokens scoped to a user", "comment": "Add token expiration."}, - {"anchor_text": "No rate limiting yet", "comment": "Add rate limiting before launch."} - ]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items).to eq([ - { anchor_text: "API tokens scoped to a user", comment: "Add token expiration." }, - { anchor_text: "No rate limiting yet", comment: "Add rate limiting before launch." } - ]) - end - - it "handles JSON wrapped in markdown code fences" do - response = "```json\n[{\"anchor_text\": null, \"comment\": \"Looks good.\"}]\n```" - - items = described_class.call(response, plan_content: plan_content) - - expect(items).to eq([{ anchor_text: nil, comment: "Looks good." }]) - end - - it "handles null anchor_text for general feedback" do - response = '[{"anchor_text": null, "comment": "Overall solid plan."}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items.first[:anchor_text]).to be_nil - expect(items.first[:comment]).to eq("Overall solid plan.") - end - - it "demotes anchor_text that does not match plan content to unanchored" do - response = '[{"anchor_text": "text that does not exist in the plan", "comment": "Some feedback."}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items.first[:anchor_text]).to be_nil - expect(items.first[:comment]).to include("> text that does not exist in the plan") - expect(items.first[:comment]).to include("Some feedback.") - end - - it "falls back to a single unanchored comment for non-JSON responses" do - response = "Here is my review in plain text.\n\nSome concerns about security." - - items = described_class.call(response, plan_content: plan_content) - - expect(items.length).to eq(1) - expect(items.first[:anchor_text]).to be_nil - expect(items.first[:comment]).to eq(response) - end - - it "falls back to a single comment when JSON is not an array" do - response = '{"anchor_text": "something", "comment": "not an array"}' - - items = described_class.call(response, plan_content: plan_content) - - expect(items.length).to eq(1) - expect(items.first[:anchor_text]).to be_nil - expect(items.first[:comment]).to eq(response) - end - - it "handles empty anchor_text string as nil" do - response = '[{"anchor_text": "", "comment": "General note."}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items.first[:anchor_text]).to be_nil - end - - it "skips non-hash items in the array" do - response = '["Looks good", {"anchor_text": null, "comment": "Real feedback."}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items).to eq([{ anchor_text: nil, comment: "Real feedback." }]) - end - - it "skips items with blank comments" do - response = '[{"anchor_text": null, "comment": ""}, {"anchor_text": null, "comment": "Valid."}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items).to eq([{ anchor_text: nil, comment: "Valid." }]) - end - - it "skips items with null comments" do - response = '[{"anchor_text": null, "comment": null}]' - - items = described_class.call(response, plan_content: plan_content) - - expect(items).to be_empty - end - end -end diff --git a/spec/services/plans/trigger_automated_reviews_spec.rb b/spec/services/plans/trigger_automated_reviews_spec.rb deleted file mode 100644 index 2735d3f..0000000 --- a/spec/services/plans/trigger_automated_reviews_spec.rb +++ /dev/null @@ -1,74 +0,0 @@ -require "rails_helper" - -RSpec.describe CoPlan::Plans::TriggerAutomatedReviews do - let!(:plan) { create(:plan, :considering) } - let(:user) { plan.created_by_user } - - before do - # Clear any existing reviewers - CoPlan::AutomatedPlanReviewer.destroy_all - end - - describe ".call" do - it "enqueues jobs for reviewers that trigger on the given status" do - reviewer = create(:automated_plan_reviewer, - trigger_statuses: ["considering"], - enabled: true - ) - - expect { - described_class.call(plan: plan, new_status: "considering", triggered_by: user) - }.to have_enqueued_job(CoPlan::AutomatedReviewJob).with( - plan_id: plan.id, - reviewer_id: reviewer.id, - plan_version_id: plan.current_plan_version_id, - triggered_by: user - ) - end - - it "does not enqueue jobs for reviewers that do not trigger on the status" do - create(:automated_plan_reviewer, - trigger_statuses: ["developing"], - enabled: true - ) - - expect { - described_class.call(plan: plan, new_status: "considering", triggered_by: user) - }.not_to have_enqueued_job(CoPlan::AutomatedReviewJob) - end - - it "does not enqueue jobs for disabled reviewers" do - create(:automated_plan_reviewer, - trigger_statuses: ["considering"], - enabled: false - ) - - expect { - described_class.call(plan: plan, new_status: "considering", triggered_by: user) - }.not_to have_enqueued_job(CoPlan::AutomatedReviewJob) - end - - it "enqueues multiple jobs when multiple reviewers match" do - create(:automated_plan_reviewer, - trigger_statuses: ["considering"], - enabled: true, - key: "reviewer-a" - ) - create(:automated_plan_reviewer, - trigger_statuses: ["considering"], - enabled: true, - key: "reviewer-b" - ) - - expect { - described_class.call(plan: plan, new_status: "considering", triggered_by: user) - }.to have_enqueued_job(CoPlan::AutomatedReviewJob).exactly(2).times - end - - it "does nothing when no reviewers exist" do - expect { - described_class.call(plan: plan, new_status: "considering", triggered_by: user) - }.not_to have_enqueued_job(CoPlan::AutomatedReviewJob) - end - end -end