self review changes

config/locales/en.yml

@@ -297,8 +297,21 @@ en:
       pdf_format_error: 'must be a PDF file'
       pdf_size_error: 'must be less than 10MB'
       pdf_upload_failed: 'Failed to upload PDF to OpenAI'
+      pdf_upload_success: 'PDF uploaded successfully with file_id: %{file_id}'
       pdf_processing_failed: 'Failed to process PDF document %{document_id}: %{error}'
       pdf_processing_success: 'Successfully processed PDF document %{document_id}'
+      faq_generation_complete: 'FAQ generation complete. Total FAQs created: %{count}'
+      using_paginated_faq: 'Using paginated FAQ generation for document %{document_id}'
+      using_standard_faq: 'Using standard FAQ generation for document %{document_id}'
+      response_creation_error: 'Error in creating response document: %{error}'
+      missing_openai_file_id: 'Document must have openai_file_id for paginated processing'
+      openai_api_error: 'OpenAI API Error: %{error}'
+      starting_paginated_faq: 'Starting paginated FAQ generation (%{pages_per_chunk} pages per chunk)'
+      stopping_faq_generation: 'Stopping processing. Reason: %{reason}'
+      paginated_faq_complete: 'Paginated generation complete. Total FAQs: %{total_faqs}, Pages processed: %{pages_processed}'
+      processing_pages: 'Processing pages %{start}-%{end} (iteration %{iteration})'
+      chunk_generated: 'Chunk generated %{chunk_faqs} FAQs. Total so far: %{total_faqs}'
+      page_processing_error: 'Error processing pages %{start}-%{end}: %{error}'
   public_portal:
     search:
       search_placeholder: Search for article by title or body...

enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb

@@ -2,15 +2,16 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
   before_action :current_account
   before_action -> { check_authorization(Captain::Assistant) }
 
+  before_action :set_current_page, only: [:index]
+  before_action :set_documents, except: [:create]
   before_action :set_document, only: [:show, :destroy]
-
+  before_action :set_assistant, only: [:create]
   RESULTS_PER_PAGE = 25
 
   def index
-    base_query = account_documents.includes(:assistant)
+    base_query = @documents
     base_query = base_query.where(assistant_id: permitted_params[:assistant_id]) if permitted_params[:assistant_id].present?
 
-    @current_page = (permitted_params[:page] || 1).to_i
     @documents_count = base_query.count
     @documents = base_query.page(@current_page).per(RESULTS_PER_PAGE)
   end
@@ -18,7 +19,9 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
   def show; end
 
   def create
-    @document = account_documents.build(document_params)
+    return render_could_not_create_error('Missing Assistant') if @assistant.nil?
+
+    @document = @assistant.documents.build(document_params)
     @document.save!
   rescue Captain::Document::LimitExceededError => e
     render_could_not_create_error(e.message)
@@ -27,18 +30,26 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
   end
 
   def destroy
-    @document.destroy!
+    @document.destroy
     head :no_content
   end
 
   private
 
-  def account_documents
-    @account_documents ||= Current.account.captain_documents.ordered
+  def set_documents
+    @documents = Current.account.captain_documents.includes(:assistant).ordered
   end
 
   def set_document
-    @document = account_documents.find(permitted_params[:id])
+    @document = @documents.find(permitted_params[:id])
+  end
+
+  def set_assistant
+    @assistant = Current.account.captain_assistants.find_by(id: document_params[:assistant_id])
+  end
+
+  def set_current_page
+    @current_page = permitted_params[:page] || 1
   end
 
   def permitted_params

enterprise/app/jobs/captain/documents/crawl_job.rb

@@ -16,10 +16,8 @@ class Captain::Documents::CrawlJob < ApplicationJob
   include Captain::FirecrawlHelper
 
   def perform_pdf_processing(document)
-    pdf_processor = Captain::Llm::PdfProcessingService.new(document)
-    pdf_processor.process
+    Captain::Llm::PdfProcessingService.new(document).process
     document.update!(status: :available)
-    Rails.logger.info I18n.t('captain.documents.pdf_processing_success', document_id: document.id)
   rescue StandardError => e
     Rails.logger.error I18n.t('captain.documents.pdf_processing_failed', document_id: document.id, error: e.message)
     raise # Re-raise to let job framework handle retry logic

enterprise/app/jobs/captain/documents/response_builder_job.rb

@@ -6,10 +6,10 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
 
     faqs = generate_faqs(document, options)
     create_responses_from_faqs(faqs, document)
-
-    Rails.logger.info "FAQ generation complete. Total FAQs created: #{faqs.size}"
   end
 
+  private
+
   def generate_faqs(document, options)
     if should_use_pagination?(document)
       generate_paginated_faqs(document, options)
@@ -19,7 +19,6 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
   end
 
   def generate_paginated_faqs(document, options)
-    Rails.logger.info "Using paginated FAQ generation for document #{document.id}"
     service = build_paginated_service(document, options)
     faqs = service.generate
     store_paginated_metadata(document, service)
@@ -27,7 +26,6 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
   end
 
   def generate_standard_faqs(document)
-    Rails.logger.info "Using standard FAQ generation for document #{document.id}"
     Captain::Llm::FaqGeneratorService.new(document.content).generate
   end
 
@@ -56,8 +54,6 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
     faqs.each { |faq| create_response(faq, document) }
   end
 
-  private
-
   def should_use_pagination?(document)
     # Auto-detect when to use pagination
     # For now, use pagination for PDFs with OpenAI file ID
@@ -76,6 +72,6 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
       documentable: document
     )
   rescue ActiveRecord::RecordInvalid => e
-    Rails.logger.error "Error in creating response document: #{e.message}"
+    Rails.logger.error I18n.t('captain.documents.response_creation_error', error: e.message)
   end
 end

enterprise/app/models/captain/document.rb

@@ -54,7 +54,9 @@ class Captain::Document < ApplicationRecord
   scope :for_assistant, ->(assistant_id) { where(assistant_id: assistant_id) }
 
   def pdf_document?
-    (external_link&.ends_with?('.pdf')) || (pdf_file.attached? && pdf_file.blob.content_type == 'application/pdf')
+    return true if pdf_file.attached? && pdf_file.blob.content_type == 'application/pdf'
+
+    external_link&.ends_with?('.pdf')
   end
 
   def content_type

enterprise/app/services/captain/llm/paginated_faq_generator_service.rb

@@ -16,7 +16,7 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
   end
 
   def generate
-    raise 'Document must have openai_file_id for paginated processing' if @document&.openai_file_id.blank?
+    raise CustomExceptions::PdfFaqGenerationError, I18n.t('captain.documents.missing_openai_file_id') if @document&.openai_file_id.blank?
 
     generate_paginated_faqs
   end
@@ -45,13 +45,11 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
     response = @client.chat(parameters: standard_chat_parameters)
     parse_response(response)
   rescue OpenAI::Error => e
-    Rails.logger.error "OpenAI API Error: #{e.message}"
+    Rails.logger.error I18n.t('captain.documents.openai_api_error', error: e.message)
     []
   end
 
   def generate_paginated_faqs
-    Rails.logger.info "Starting paginated FAQ generation (#{@pages_per_chunk} pages per chunk)"
-
     all_faqs = []
     current_page = 1
 
@@ -59,15 +57,11 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
       end_page = calculate_end_page(current_page)
       chunk_result = process_chunk_and_update_state(current_page, end_page, all_faqs)
 
-      unless should_continue_processing?(chunk_result)
-        Rails.logger.info "Stopping processing. Reason: #{determine_stop_reason(chunk_result)}"
-        break
-      end
+      break unless should_continue_processing?(chunk_result)
 
       current_page = end_page + 1
     end
 
-    Rails.logger.info "Paginated generation complete. Total FAQs: #{all_faqs.size}, Pages processed: #{@total_pages_processed}"
     deduplicate_faqs(all_faqs)
   end
 
@@ -77,8 +71,6 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
   end
 
   def process_chunk_and_update_state(current_page, end_page, all_faqs)
-    Rails.logger.info "Processing pages #{current_page}-#{end_page} (iteration #{@iterations_completed + 1})"
-
     chunk_result = process_page_chunk(current_page, end_page)
     chunk_faqs = chunk_result[:faqs]
 
@@ -86,7 +78,6 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
     @total_pages_processed = end_page
     @iterations_completed += 1
 
-    Rails.logger.info "Chunk generated #{chunk_faqs.size} FAQs. Total so far: #{all_faqs.size}"
     chunk_result
   end
 
@@ -96,7 +87,7 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::BaseOpenAiService
     result = parse_chunk_response(response)
     { faqs: result['faqs'] || [], has_content: result['has_content'] != false }
   rescue OpenAI::Error => e
-    Rails.logger.error "Error processing pages #{start_page}-#{end_page}: #{e.message}"
+    Rails.logger.error I18n.t('captain.documents.page_processing_error', start: start_page, end: end_page, error: e.message)
     { faqs: [], has_content: false }
   end
 

enterprise/app/services/captain/llm/pdf_processing_service.rb

@@ -1,11 +1,16 @@
 class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
   def initialize(document)
-    @document = document
     super()
+    @document = document
   end
 
   def process
-    process_for_pagination
+    return if document.openai_file_id.present?
+
+    file_id = upload_pdf_to_openai
+    raise CustomExceptions::PdfUploadError, I18n.t('captain.documents.pdf_upload_failed') if file_id.blank?
+
+    document.store_openai_file_id(file_id)
   end
 
   private
@@ -13,34 +18,23 @@ class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
   attr_reader :document
 
   def upload_pdf_to_openai
-    pdf_file = document.pdf_file
-
-    Tempfile.create(['pdf_upload', '.pdf'], binmode: true) do |temp_file|
-      temp_file.write(pdf_file.download)
-      temp_file.close
-
-      File.open(temp_file.path, 'rb') do |file|
-        @client.files.upload(
-          parameters: {
-            file: file,
-            purpose: 'assistants'
-          }
-        )
-      end
+    with_tempfile do |temp_file|
+      response = @client.files.upload(
+        parameters: {
+          file: temp_file,
+          purpose: 'assistants'
+        }
+      )
+      response['id']
     end
   end
 
-  def process_for_pagination
-    return 'PDF ready for paginated processing' if document.openai_file_id.present?
+  def with_tempfile(&)
+    Tempfile.create(['pdf_upload', '.pdf'], binmode: true) do |temp_file|
+      temp_file.write(document.pdf_file.download)
+      temp_file.close
 
-    openai_response = upload_pdf_to_openai
-    file_id = openai_response['id']
-
-    raise I18n.t('captain.documents.pdf_upload_failed') if file_id.blank?
-
-    document.store_openai_file_id(file_id)
-    Rails.logger.info "PDF uploaded successfully with file_id: #{file_id}"
-
-    "PDF ready for paginated processing (file_id: #{file_id})"
+      File.open(temp_file.path, 'rb', &)
+    end
   end
 end

lib/custom_exceptions/pdf_processing_error.rb

@@ -1,23 +1,25 @@
-class PdfProcessingError < CustomExceptions::Base
-  def initialize(message = 'PDF processing failed')
-    super(message)
+module CustomExceptions
+  class PdfProcessingError < Base
+    def initialize(message = 'PDF processing failed')
+      super(message)
+    end
   end
-end
 
-class PdfUploadError < PdfProcessingError
-  def initialize(message = 'PDF upload failed')
-    super(message)
+  class PdfUploadError < PdfProcessingError
+    def initialize(message = 'PDF upload failed')
+      super(message)
+    end
   end
-end
 
-class PdfValidationError < PdfProcessingError
-  def initialize(message = 'PDF validation failed')
-    super(message)
+  class PdfValidationError < PdfProcessingError
+    def initialize(message = 'PDF validation failed')
+      super(message)
+    end
   end
-end
 
-class PdfFaqGenerationError < PdfProcessingError
-  def initialize(message = 'PDF FAQ generation failed')
-    super(message)
+  class PdfFaqGenerationError < PdfProcessingError
+    def initialize(message = 'PDF FAQ generation failed')
+      super(message)
+    end
   end
 end

spec/enterprise/jobs/captain/documents/crawl_job_spec.rb

@@ -1,157 +1,132 @@
 require 'rails_helper'
 
 RSpec.describe Captain::Documents::CrawlJob, type: :job do
-  let(:account) { create(:account) }
-  let(:assistant) { create(:captain_assistant, account: account) }
-  let(:url_document) { create(:captain_document, assistant: assistant, account: account, external_link: 'https://example.com') }
-  let(:pdf_document) do
-    doc = create(:captain_document, assistant: assistant, account: account)
-    doc.pdf_file.attach(
-      io: File.open(Rails.root.join('spec/fixtures/files/sample.pdf')),
-      filename: 'sample.pdf',
-      content_type: 'application/pdf'
-    )
-    doc
-  end
+  let(:document) { create(:captain_document, external_link: 'https://example.com/page') }
+  let(:assistant_id) { document.assistant_id }
+  let(:webhook_url) { Rails.application.routes.url_helpers.enterprise_webhooks_firecrawl_url }
 
   describe '#perform' do
-    context 'when document has external link' do
-      it 'performs simple crawling when no firecrawl API key' do
-        allow(InstallationConfig).to receive(:find_by).and_return(nil)
-        job = described_class.new
-        expect(job).to receive(:perform_simple_crawl).with(url_document)
-        job.perform(url_document)
+    context 'when CAPTAIN_FIRECRAWL_API_KEY is configured' do
+      let(:firecrawl_service) { instance_double(Captain::Tools::FirecrawlService) }
+      let(:account) { document.account }
+      let(:token) { Digest::SHA256.hexdigest("-key#{document.assistant_id}#{document.account_id}") }
+
+      before do
+        allow(Captain::Tools::FirecrawlService).to receive(:new).and_return(firecrawl_service)
+        allow(firecrawl_service).to receive(:perform)
+        create(:installation_config, name: 'CAPTAIN_FIRECRAWL_API_KEY', value: 'test-key')
       end
 
-      it 'does not perform PDF processing' do
-        allow(InstallationConfig).to receive(:find_by).and_return(nil)
-        # Mock the HTTP request made by SimplePageCrawlService
-        stub_request(:get, 'https://example.com/').to_return(status: 200, body: '<html></html>', headers: {})
+      context 'with account usage limits' do
+        before do
+          allow(account).to receive(:usage_limits).and_return({ captain: { documents: { current_available: 20 } } })
+        end
 
-        simple_crawl_service = instance_double(Captain::Tools::SimplePageCrawlService)
-        allow(Captain::Tools::SimplePageCrawlService).to receive(:new).and_return(simple_crawl_service)
-        allow(simple_crawl_service).to receive(:page_links).and_return([])
+        it 'uses FirecrawlService with the correct crawl limit' do
+          expect(firecrawl_service).to receive(:perform).with(
+            document.external_link,
+            "#{webhook_url}?assistant_id=#{assistant_id}&token=#{token}",
+            20
+          )
 
-        job = described_class.new
-        expect(job).not_to receive(:perform_pdf_processing)
-        job.perform(url_document)
-      end
-    end
-
-    context 'when document has PDF file' do
-      it 'performs PDF processing' do
-        job = described_class.new
-        expect(job).to receive(:perform_pdf_processing).with(pdf_document)
-        job.perform(pdf_document)
+          described_class.perform_now(document)
+        end
       end
 
-      it 'does not perform web crawling' do
-        job = described_class.new
-        allow(job).to receive(:perform_pdf_processing)
-        expect(job).not_to receive(:perform_simple_crawl)
-        expect(job).not_to receive(:perform_firecrawl_crawl)
-        job.perform(pdf_document)
+      context 'when crawl limit exceeds maximum' do
+        before do
+          allow(account).to receive(:usage_limits).and_return({ captain: { documents: { current_available: 1000 } } })
+        end
+
+        it 'caps the crawl limit at 500' do
+          expect(firecrawl_service).to receive(:perform).with(
+            document.external_link,
+            "#{webhook_url}?assistant_id=#{assistant_id}&token=#{token}",
+            500
+          )
+
+          described_class.perform_now(document)
+        end
       end
 
-      it 'calls PDF processing service' do
-        pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-        expect(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-        expect(pdf_processing_service).to receive(:process)
+      context 'with no usage limits configured' do
+        before do
+          allow(account).to receive(:usage_limits).and_return({})
+        end
 
-        described_class.perform_now(pdf_document)
-      end
+        it 'uses default crawl limit of 10' do
+          expect(firecrawl_service).to receive(:perform).with(
+            document.external_link,
+            "#{webhook_url}?assistant_id=#{assistant_id}&token=#{token}",
+            10
+          )
 
-      it 'marks document as available after processing' do
-        pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-        allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-        allow(pdf_processing_service).to receive(:process)
-
-        expect { described_class.perform_now(pdf_document) }
-          .to change { pdf_document.reload.status }.to('available')
-      end
-
-      it 'logs successful PDF processing' do
-        pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-        allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-        allow(pdf_processing_service).to receive(:process)
-        allow(Rails.logger).to receive(:info)
-
-        described_class.perform_now(pdf_document)
-
-        expect(Rails.logger).to have_received(:info).with(I18n.t('captain.documents.pdf_processing_success', document_id: pdf_document.id))
-      end
-
-      context 'when PDF processing fails' do
-        it 'logs error and re-raises' do
-          pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-          allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-          allow(pdf_processing_service).to receive(:process).and_raise(StandardError, 'Processing failed')
-
-          allow(Rails.logger).to receive(:error)
-
-          expect { described_class.perform_now(pdf_document) }
-            .to raise_error(StandardError, 'Processing failed')
-
-          expect(Rails.logger).to have_received(:error).with(I18n.t('captain.documents.pdf_processing_failed', document_id: pdf_document.id,
-                                                                                                               error: 'Processing failed'))
+          described_class.perform_now(document)
         end
       end
     end
 
-    context 'when document is nil' do
-      it 'raises NoMethodError' do
-        expect { described_class.perform_now(nil) }
-          .to raise_error(NoMethodError)
+    context 'when CAPTAIN_FIRECRAWL_API_KEY is not configured' do
+      let(:page_links) { ['https://example.com/page1', 'https://example.com/page2'] }
+      let(:simple_crawler) { instance_double(Captain::Tools::SimplePageCrawlService) }
+
+      before do
+        allow(Captain::Tools::SimplePageCrawlService)
+          .to receive(:new)
+          .with(document.external_link)
+          .and_return(simple_crawler)
+
+        allow(simple_crawler).to receive(:page_links).and_return(page_links)
+      end
+
+      it 'enqueues SimplePageCrawlParserJob for each discovered link' do
+        page_links.each do |link|
+          expect(Captain::Tools::SimplePageCrawlParserJob)
+            .to receive(:perform_later)
+            .with(
+              assistant_id: assistant_id,
+              page_link: link
+            )
+        end
+
+        # Should also crawl the original link
+        expect(Captain::Tools::SimplePageCrawlParserJob)
+          .to receive(:perform_later)
+          .with(
+            assistant_id: assistant_id,
+            page_link: document.external_link
+          )
+
+        described_class.perform_now(document)
+      end
+
+      it 'uses SimplePageCrawlService to discover page links' do
+        expect(simple_crawler).to receive(:page_links)
+        described_class.perform_now(document)
       end
     end
 
-    context 'when document has external link but no PDF file' do
-      let(:web_document) { create(:captain_document, assistant: assistant, account: account, external_link: 'https://example.com') }
-
-      it 'performs simple crawl when no firecrawl API key' do
-        allow(InstallationConfig).to receive(:find_by).and_return(nil)
-        job = described_class.new
-        expect(job).to receive(:perform_simple_crawl).with(web_document)
-        expect(job).not_to receive(:perform_pdf_processing)
-
-        job.perform(web_document)
+    context 'when document is a PDF' do
+      let(:pdf_document) do
+        doc = create(:captain_document, external_link: 'https://example.com/document')
+        allow(doc).to receive(:pdf_document?).and_return(true)
+        allow(doc).to receive(:update!).and_return(true)
+        doc
       end
-    end
-  end
 
-  describe '#perform_pdf_processing' do
-    let(:job_instance) { described_class.new }
+      it 'processes PDF using PdfProcessingService' do
+        pdf_service = instance_double(Captain::Llm::PdfProcessingService)
+        expect(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_service)
+        expect(pdf_service).to receive(:process)
+        expect(pdf_document).to receive(:update!).with(status: :available)
 
-    it 'creates PDF processing service with correct parameters' do
-      pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-      expect(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-      expect(pdf_processing_service).to receive(:process)
+        described_class.perform_now(pdf_document)
+      end
 
-      job_instance.send(:perform_pdf_processing, pdf_document)
-    end
+      it 'handles PDF processing errors' do
+        allow(Captain::Llm::PdfProcessingService).to receive(:new).and_raise(StandardError, 'Processing failed')
 
-    it 'marks document as available after processing' do
-      pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-      allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-      allow(pdf_processing_service).to receive(:process)
-
-      expect { job_instance.send(:perform_pdf_processing, pdf_document) }
-        .to change { pdf_document.reload.status }.to('available')
-    end
-
-    context 'when processing fails' do
-      it 'logs error and re-raises' do
-        pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
-        allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
-        allow(pdf_processing_service).to receive(:process).and_raise(StandardError, 'Test error')
-
-        allow(Rails.logger).to receive(:error)
-
-        expect { job_instance.send(:perform_pdf_processing, pdf_document) }
-          .to raise_error(StandardError, 'Test error')
-
-        expect(Rails.logger).to have_received(:error).with(I18n.t('captain.documents.pdf_processing_failed', document_id: pdf_document.id,
-                                                                                                             error: 'Test error'))
+        expect { described_class.perform_now(pdf_document) }.to raise_error(StandardError, 'Processing failed')
       end
     end
   end

spec/enterprise/jobs/captain/documents/response_builder_job_spec.rb

@@ -1,229 +1,83 @@
 require 'rails_helper'
 
 RSpec.describe Captain::Documents::ResponseBuilderJob, type: :job do
-  let(:account) { create(:account) }
-  let(:assistant) { create(:captain_assistant, account: account) }
-  let(:document) { create(:captain_document, assistant: assistant, account: account) }
+  let(:assistant) { create(:captain_assistant) }
+  let(:document) { create(:captain_document, assistant: assistant) }
+  let(:faq_generator) { instance_double(Captain::Llm::FaqGeneratorService) }
+  let(:faqs) do
+    [
+      { 'question' => 'What is Ruby?', 'answer' => 'A programming language' },
+      { 'question' => 'What is Rails?', 'answer' => 'A web framework' }
+    ]
+  end
+
+  before do
+    allow(Captain::Llm::FaqGeneratorService).to receive(:new)
+      .with(document.content)
+      .and_return(faq_generator)
+    allow(faq_generator).to receive(:generate).and_return(faqs)
+  end
 
   describe '#perform' do
-    before do
-      # Mock the InstallationConfig for services that need it
-      allow(InstallationConfig).to receive(:find_by!)
-        .with(name: 'CAPTAIN_OPEN_AI_API_KEY')
-        .and_return(instance_double(InstallationConfig, value: 'test-api-key'))
+    context 'when processing a document' do
+      it 'deletes previous responses' do
+        existing_response = create(:captain_assistant_response, documentable: document)
+
+        described_class.new.perform(document)
+
+        expect { existing_response.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'creates new responses for each FAQ' do
+        expect do
+          described_class.new.perform(document)
+        end.to change(Captain::AssistantResponse, :count).by(2)
+
+        responses = document.responses.reload
+        expect(responses.count).to eq(2)
+
+        first_response = responses.first
+        expect(first_response.question).to eq('What is Ruby?')
+        expect(first_response.answer).to eq('A programming language')
+        expect(first_response.assistant).to eq(assistant)
+        expect(first_response.documentable).to eq(document)
+      end
     end
 
-    context 'when document requires pagination' do
+    context 'when processing a PDF document' do
+      let(:pdf_document) do
+        doc = create(:captain_document, assistant: assistant)
+        allow(doc).to receive(:pdf_document?).and_return(true)
+        allow(doc).to receive(:openai_file_id).and_return('file-123')
+        allow(doc).to receive(:update!).and_return(true)
+        allow(doc).to receive(:metadata).and_return({})
+        doc
+      end
+      let(:paginated_service) { instance_double(Captain::Llm::PaginatedFaqGeneratorService) }
+      let(:pdf_faqs) do
+        [{ 'question' => 'What is in the PDF?', 'answer' => 'Important content' }]
+      end
+
       before do
-        allow(document).to receive(:pdf_document?).and_return(true)
-        allow(document).to receive(:openai_file_id).and_return('file-123')
-        allow(document).to receive(:update!).and_return(true)
-        allow(document).to receive(:metadata).and_return({})
-      end
-
-      it 'uses paginated FAQ generator service' do
-        paginated_service = instance_double(Captain::Llm::PaginatedFaqGeneratorService)
-
-        expect(Captain::Llm::PaginatedFaqGeneratorService).to receive(:new)
-          .with(document, hash_including(:pages_per_chunk, :max_pages))
-          .and_return(paginated_service)
-
-        expect(paginated_service).to receive(:generate).and_return([])
-        expect(paginated_service).to receive(:total_pages_processed).and_return(10)
-        expect(paginated_service).to receive(:iterations_completed).and_return(1)
-
-        described_class.perform_now(document)
-      end
-
-      it 'accepts pagination options' do
-        options = { pages_per_chunk: 5, max_pages: 20 }
-        paginated_service = instance_double(Captain::Llm::PaginatedFaqGeneratorService)
-
-        expect(Captain::Llm::PaginatedFaqGeneratorService).to receive(:new)
-          .with(document, pages_per_chunk: 5, max_pages: 20)
-          .and_return(paginated_service)
-
-        expect(paginated_service).to receive(:generate).and_return([])
-        expect(paginated_service).to receive(:total_pages_processed).and_return(20)
-        expect(paginated_service).to receive(:iterations_completed).and_return(4)
-
-        described_class.perform_now(document, options)
-      end
-
-      it 'stores metadata about paginated generation' do
-        paginated_service = instance_double(Captain::Llm::PaginatedFaqGeneratorService)
-
         allow(Captain::Llm::PaginatedFaqGeneratorService).to receive(:new)
+          .with(pdf_document, anything)
           .and_return(paginated_service)
-        allow(paginated_service).to receive(:generate).and_return([])
-        allow(paginated_service).to receive(:total_pages_processed).and_return(15)
-        allow(paginated_service).to receive(:iterations_completed).and_return(2)
-
-        expect(document).to receive(:update!).with(
-          metadata: hash_including(
-            'faq_generation' => hash_including(
-              'method' => 'paginated',
-              'pages_processed' => 15,
-              'iterations' => 2
-            )
-          )
-        )
-
-        described_class.perform_now(document)
-      end
-    end
-
-    context 'when document uses standard generation' do
-      before do
-        allow(document).to receive(:pdf_document?).and_return(false)
-        allow(document).to receive(:content).and_return('Document content')
+        allow(paginated_service).to receive(:generate).and_return(pdf_faqs)
+        allow(paginated_service).to receive(:total_pages_processed).and_return(10)
+        allow(paginated_service).to receive(:iterations_completed).and_return(1)
       end
 
-      it 'uses standard FAQ generator service' do
-        standard_service = instance_double(Captain::Llm::FaqGeneratorService)
+      it 'uses paginated FAQ generator for PDFs' do
+        expect(Captain::Llm::PaginatedFaqGeneratorService).to receive(:new).with(pdf_document, anything)
 
-        expect(Captain::Llm::FaqGeneratorService).to receive(:new)
-          .with('Document content')
-          .and_return(standard_service)
-
-        expect(standard_service).to receive(:generate).and_return([])
-
-        described_class.perform_now(document)
-      end
-    end
-
-    context 'when FAQs are generated successfully' do
-      let(:faqs) do
-        [
-          { 'question' => 'What is this?', 'answer' => 'This is a test' },
-          { 'question' => 'How does it work?', 'answer' => 'It works well' }
-        ]
+        described_class.new.perform(pdf_document)
       end
 
-      before do
-        allow(document).to receive(:pdf_document?).and_return(false)
-        allow(document).to receive(:content).and_return('Document content')
+      it 'stores pagination metadata' do
+        expect(pdf_document).to receive(:update!).with(hash_including(metadata: hash_including('faq_generation')))
 
-        standard_service = instance_double(Captain::Llm::FaqGeneratorService)
-        allow(Captain::Llm::FaqGeneratorService).to receive(:new).and_return(standard_service)
-        allow(standard_service).to receive(:generate).and_return(faqs)
+        described_class.new.perform(pdf_document)
       end
-
-      it 'creates response records from FAQs' do
-        expect { described_class.perform_now(document) }
-          .to change { document.responses.count }.by(2)
-      end
-
-      it 'creates responses with correct attributes' do
-        described_class.perform_now(document)
-
-        response = document.responses.first
-        expect(response.question).to eq('What is this?')
-        expect(response.answer).to eq('This is a test')
-        expect(response.assistant).to eq(assistant)
-        expect(response.documentable).to eq(document)
-      end
-
-      it 'removes previous responses before creating new ones' do
-        # Create existing responses
-        document.responses.create!(
-          question: 'Old question',
-          answer: 'Old answer',
-          assistant: assistant,
-          documentable: document
-        )
-
-        expect(document.responses.count).to eq(1)
-
-        described_class.perform_now(document)
-
-        expect(document.responses.count).to eq(2)
-        expect(document.responses.pluck(:question)).not_to include('Old question')
-      end
-    end
-
-    context 'when FAQ generation fails' do
-      before do
-        allow(document).to receive(:pdf_document?).and_return(false)
-        allow(document).to receive(:content).and_return('Document content')
-      end
-
-      it 'propagates the error' do
-        standard_service = instance_double(Captain::Llm::FaqGeneratorService)
-        allow(Captain::Llm::FaqGeneratorService).to receive(:new).and_return(standard_service)
-        allow(standard_service).to receive(:generate).and_raise(StandardError, 'Generation failed')
-
-        expect { described_class.perform_now(document) }
-          .to raise_error(StandardError, 'Generation failed')
-      end
-    end
-
-    context 'when creating response fails' do
-      let(:faqs) do
-        [
-          { 'question' => nil, 'answer' => 'Invalid FAQ' } # Invalid due to nil question
-        ]
-      end
-
-      before do
-        allow(document).to receive(:pdf_document?).and_return(false)
-        allow(document).to receive(:content).and_return('Document content')
-
-        standard_service = instance_double(Captain::Llm::FaqGeneratorService)
-        allow(Captain::Llm::FaqGeneratorService).to receive(:new).and_return(standard_service)
-        allow(standard_service).to receive(:generate).and_return(faqs)
-      end
-
-      it 'logs the error and continues' do
-        expect(Rails.logger).to receive(:error).with(/Error in creating response document/)
-
-        expect { described_class.perform_now(document) }
-          .not_to raise_error
-      end
-    end
-  end
-
-  describe 'job queuing' do
-    it 'is enqueued in the low queue' do
-      expect do
-        described_class.perform_later(document)
-      end.to have_enqueued_job(described_class).on_queue('low')
-    end
-
-    it 'can be scheduled for later execution' do
-      expect do
-        described_class.set(wait: 5.minutes).perform_later(document)
-      end.to have_enqueued_job(described_class).at(a_value_within(1.second).of(5.minutes.from_now))
-    end
-  end
-
-  describe '#should_use_pagination?' do
-    subject { described_class.new.send(:should_use_pagination?, document) }
-
-    context 'when document is a PDF with openai_file_id' do
-      before do
-        allow(document).to receive(:pdf_document?).and_return(true)
-        allow(document).to receive(:openai_file_id).and_return('file-123')
-      end
-
-      it { is_expected.to be true }
-    end
-
-    context 'when document is a PDF without openai_file_id' do
-      before do
-        allow(document).to receive(:pdf_document?).and_return(true)
-        allow(document).to receive(:openai_file_id).and_return(nil)
-      end
-
-      it { is_expected.to be false }
-    end
-
-    context 'when document is not a PDF' do
-      before do
-        allow(document).to receive(:pdf_document?).and_return(false)
-      end
-
-      it { is_expected.to be false }
     end
   end
 end

spec/enterprise/models/captain/document_spec.rb

@@ -4,240 +4,81 @@ RSpec.describe Captain::Document, type: :model do
   let(:account) { create(:account) }
   let(:assistant) { create(:captain_assistant, account: account) }
 
-  describe 'validations' do
-    context 'with PDF file' do
-      let(:document) { build(:captain_document, assistant: assistant, account: account) }
-
-      before do
-        document.pdf_file.attach(
-          io: StringIO.new('PDF content'),
-          filename: 'test.pdf',
-          content_type: 'application/pdf'
-        )
-      end
-
-      it 'is valid with PDF file and no external link' do
-        document.external_link = nil
-        expect(document).to be_valid
-      end
-
-      # NOTE: Current implementation allows both PDF and external link
-      it 'is valid with both PDF file and external link' do
-        document.external_link = 'https://example.com'
-        expect(document).to be_valid
-      end
-
-      it 'validates PDF file format' do
-        document.pdf_file.purge
-        document.pdf_file.attach(
-          io: StringIO.new('Text content'),
-          filename: 'test.txt',
-          content_type: 'text/plain'
-        )
-        # The validation actually checks if content_type is application/pdf
-        # Since pdf_document? returns true for attached files with pdf content type,
-        # and validate_pdf_format only runs if pdf_document? is true,
-        # this test shows current behavior
-        expect(document).to be_valid  # Because it has external_link and pdf_document? returns false
-      end
-    end
-
-    context 'without PDF file' do
-      let(:document) { build(:captain_document, assistant: assistant, account: account, external_link: 'https://example.com') }
-
-      it 'is valid with external link and no PDF file' do
-        expect(document).to be_valid
-      end
-
-      it 'is invalid without both PDF file and external link' do
-        document.external_link = nil
-        expect(document).not_to be_valid
-        expect(document.errors[:external_link]).to include("can't be blank")
-      end
-    end
-  end
-
-  describe '#pdf_document?' do
-    let(:document) { create(:captain_document, assistant: assistant, account: account) }
-
-    context 'when document has PDF file attached' do
-      before do
-        document.pdf_file.attach(
-          io: StringIO.new('PDF content'),
-          filename: 'test.pdf',
-          content_type: 'application/pdf'
-        )
-      end
-
-      it 'returns true' do
-        expect(document.pdf_document?).to be true
-      end
-    end
-
-    context 'when document has no PDF file' do
-      it 'returns false' do
-        expect(document.pdf_document?).to be false
-      end
-    end
-  end
-
-  describe '#content_type' do
-    let(:document) { create(:captain_document, assistant: assistant, account: account) }
-
-    context 'with PDF file' do
-      before do
-        document.pdf_file.attach(
-          io: StringIO.new('PDF content'),
-          filename: 'test.pdf',
-          content_type: 'application/pdf'
-        )
-      end
-
-      it 'returns PDF content type' do
-        expect(document.content_type).to eq('application/pdf')
-      end
-    end
-
-    context 'without PDF file' do
-      it 'returns nil' do
-        expect(document.content_type).to be_nil
-      end
-    end
-  end
-
-  describe '#file_size' do
-    let(:document) { create(:captain_document, assistant: assistant, account: account) }
-
-    context 'with PDF file' do
-      let(:file_content) { 'PDF content with some size' }
-
-      before do
-        document.pdf_file.attach(
-          io: StringIO.new(file_content),
-          filename: 'test.pdf',
-          content_type: 'application/pdf'
-        )
-      end
-
-      it 'returns the file size in bytes' do
-        expect(document.file_size).to eq(file_content.bytesize)
-      end
-    end
-
-    context 'without PDF file' do
-      it 'returns nil' do
-        expect(document.file_size).to be_nil
-      end
-    end
-  end
-
-  describe '#set_external_link_for_pdf' do
-    let(:document) { build(:captain_document, assistant: assistant, account: account, external_link: nil) }
-
-    context 'when called before save' do
-      before do
-        document.pdf_file.attach(
-          io: StringIO.new('PDF content'),
-          filename: 'test.pdf',
-          content_type: 'application/pdf'
-        )
-      end
-
-      it 'generates a unique external link' do
-        document.save!
-        expect(document.external_link).to be_present
-        expect(document.external_link).to match(/^PDF: /)
-      end
-
-      it 'generates different links for different documents' do
-        document.save!
-        another_document = build(:captain_document, assistant: assistant, account: account, external_link: nil)
-        another_document.pdf_file.attach(
-          io: StringIO.new('Different PDF'),
-          filename: 'another.pdf',
-          content_type: 'application/pdf'
-        )
-        another_document.save!
-
-        expect(document.external_link).not_to eq(another_document.external_link)
-      end
-    end
-
-    context 'when document already has external link' do
-      before do
-        document.external_link = 'https://example.com'
-      end
-
-      it 'does not override existing external link' do
-        document.save!
-        expect(document.external_link).to eq('https://example.com')
-      end
-    end
-
-    context 'when document has no PDF file' do
-      it 'does not set external link' do
-        expect { document.save! }.to raise_error(ActiveRecord::RecordInvalid)
-      end
-    end
-  end
-
-  describe 'metadata column' do
-    let(:document) { create(:captain_document, assistant: assistant, account: account) }
-
-    it 'has metadata column with default empty hash' do
-      expect(document.metadata).to eq({})
-    end
-
-    it 'can store OpenAI file ID' do
-      document.metadata['openai_file_id'] = 'file-123'
-      document.save!
-      document.reload
-      expect(document.metadata['openai_file_id']).to eq('file-123')
-    end
-
-    it 'can store FAQ generation metadata' do
-      document.metadata['faq_generation'] = {
-        'faqs_generated' => 10,
-        'pages_processed' => 3,
-        'generation_time' => 5.2
-      }
-      document.save!
-      document.reload
-      expect(document.metadata['faq_generation']).to include(
-        'faqs_generated' => 10,
-        'pages_processed' => 3,
-        'generation_time' => 5.2
+  describe 'PDF support' do
+    let(:pdf_document) do
+      doc = build(:captain_document, assistant: assistant, account: account)
+      doc.pdf_file.attach(
+        io: StringIO.new('PDF content'),
+        filename: 'test.pdf',
+        content_type: 'application/pdf'
       )
+      doc
     end
 
-    it 'preserves existing metadata when adding new keys' do
-      document.metadata['openai_file_id'] = 'file-123'
-      document.save!
+    describe 'validations' do
+      it 'allows PDF file without external link' do
+        pdf_document.external_link = nil
+        expect(pdf_document).to be_valid
+      end
 
-      document.metadata['faq_generation'] = { 'count' => 5 }
-      document.save!
-      document.reload
-
-      expect(document.metadata).to include(
-        'openai_file_id' => 'file-123',
-        'faq_generation' => { 'count' => 5 }
-      )
-    end
-  end
-
-  describe 'callbacks' do
-    context 'when before_validation callback runs' do
-      let(:document) { build(:captain_document, assistant: assistant, account: account, external_link: nil) }
-
-      it 'sets external link for PDF documents' do
-        document.pdf_file.attach(
-          io: StringIO.new('PDF content'),
-          filename: 'test.pdf',
+      it 'validates PDF file size' do
+        doc = build(:captain_document, assistant: assistant, account: account)
+        doc.pdf_file.attach(
+          io: StringIO.new('x' * 11.megabytes),
+          filename: 'large.pdf',
           content_type: 'application/pdf'
         )
+        doc.external_link = nil
+        expect(doc).not_to be_valid
+        expect(doc.errors[:pdf_file]).to include(I18n.t('captain.documents.pdf_size_error'))
+      end
+    end
 
-        expect { document.valid? }.to change(document, :external_link).from(nil)
-        expect(document.external_link).to match(/^PDF: /)
+    describe '#pdf_document?' do
+      it 'returns true for attached PDF' do
+        expect(pdf_document.pdf_document?).to be true
+      end
+
+      it 'returns true for .pdf external links' do
+        doc = build(:captain_document, external_link: 'https://example.com/document.pdf')
+        expect(doc.pdf_document?).to be true
+      end
+
+      it 'returns false for non-PDF documents' do
+        doc = build(:captain_document, external_link: 'https://example.com')
+        expect(doc.pdf_document?).to be false
+      end
+    end
+
+    describe '#display_url' do
+      it 'returns Rails blob URL for attached PDFs' do
+        pdf_document.save!
+        # The display_url method calls rails_blob_url which returns a URL containing 'rails/active_storage'
+        url = pdf_document.display_url
+        expect(url).to be_present
+      end
+
+      it 'returns external_link for web documents' do
+        doc = create(:captain_document, external_link: 'https://example.com')
+        expect(doc.display_url).to eq('https://example.com')
+      end
+    end
+
+    describe '#store_openai_file_id' do
+      it 'stores the file ID in metadata' do
+        pdf_document.save!
+        pdf_document.store_openai_file_id('file-abc123')
+
+        expect(pdf_document.reload.openai_file_id).to eq('file-abc123')
+      end
+    end
+
+    describe 'automatic external_link generation' do
+      it 'generates unique external_link for PDFs' do
+        pdf_document.external_link = nil
+        pdf_document.save!
+
+        expect(pdf_document.external_link).to start_with('PDF: test_')
       end
     end
   end

spec/enterprise/services/captain/llm/paginated_faq_generator_service_spec.rb

@@ -1,315 +1,106 @@
 require 'rails_helper'
+require 'custom_exceptions/pdf_processing_error'
 
 RSpec.describe Captain::Llm::PaginatedFaqGeneratorService do
-  let(:account) { create(:account) }
-  let(:service) do
-    described_class.new(document, { pages_per_chunk: 10 })
-  end
-  let(:assistant) { create(:captain_assistant, account: account) }
-  let(:document) { create(:captain_document, assistant: assistant, account: account) }
+  let(:document) { create(:captain_document) }
+  let(:service) { described_class.new(document, pages_per_chunk: 5) }
   let(:openai_client) { instance_double(OpenAI::Client) }
 
   before do
-    # Mock the InstallationConfig for base class initialization
-    allow(InstallationConfig).to receive(:find_by!).with(name: 'CAPTAIN_OPEN_AI_API_KEY')
-                                                   .and_return(instance_double(InstallationConfig, value: 'test-api-key'))
-    # Allow any find_by calls to return nil by default
-    allow(InstallationConfig).to receive(:find_by).and_return(nil)
+    # Mock OpenAI configuration
+    installation_config = instance_double(InstallationConfig, value: 'test-api-key')
+    allow(InstallationConfig).to receive(:find_by!)
+      .with(name: 'CAPTAIN_OPEN_AI_API_KEY')
+      .and_return(installation_config)
+
     allow(OpenAI::Client).to receive(:new).and_return(openai_client)
   end
 
-  describe '#initialize' do
-    it 'initializes with document and options' do
-      expect(service.instance_variable_get(:@document)).to eq(document)
-      expect(service.instance_variable_get(:@pages_per_chunk)).to eq(10)
-    end
-
-    it 'uses default pages per chunk when not specified' do
-      default_service = described_class.new(document)
-      expect(default_service.instance_variable_get(:@pages_per_chunk)).to eq(10)
-    end
-
-    it 'accepts max_pages option' do
-      service_with_max = described_class.new(document, { max_pages: 50 })
-      expect(service_with_max.instance_variable_get(:@max_pages)).to eq(50)
-    end
-  end
-
   describe '#generate' do
-    context 'when document has no openai_file_id' do
+    context 'when document lacks OpenAI file ID' do
       before do
         allow(document).to receive(:openai_file_id).and_return(nil)
       end
 
       it 'raises an error' do
-        expect { service.generate }.to raise_error('Document must have openai_file_id for paginated processing')
+        expect { service.generate }.to raise_error(CustomExceptions::PdfFaqGenerationError)
       end
     end
 
-    context 'when document has openai_file_id' do
+    context 'when generating FAQs from PDF pages' do
+      let(:faq_response) do
+        {
+          'choices' => [{
+            'message' => {
+              'content' => JSON.generate({
+                                           'faqs' => [
+                                             { 'question' => 'What is this document about?', 'answer' => 'It explains key concepts.' }
+                                           ],
+                                           'has_content' => true
+                                         })
+            }
+          }]
+        }
+      end
+
+      let(:empty_response) do
+        {
+          'choices' => [{
+            'message' => {
+              'content' => JSON.generate({
+                                           'faqs' => [],
+                                           'has_content' => false
+                                         })
+            }
+          }]
+        }
+      end
+
       before do
         allow(document).to receive(:openai_file_id).and_return('file-123')
       end
 
-      context 'when generating FAQs successfully' do
-        let(:faq_response) do
-          {
-            'choices' => [
-              {
-                'message' => {
-                  'content' => JSON.generate({
-                                               'faqs' => [
-                                                 { 'question' => 'What is the product?', 'answer' => 'It is a great product.' },
-                                                 { 'question' => 'How does it work?', 'answer' => 'It works seamlessly.' }
-                                               ],
-                                               'has_content' => true
-                                             })
-                }
-              }
-            ]
-          }
-        end
+      it 'generates FAQs from paginated content' do
+        allow(openai_client).to receive(:chat).and_return(faq_response, empty_response)
 
-        let(:empty_response) do
-          {
-            'choices' => [
-              {
-                'message' => {
-                  'content' => JSON.generate({
-                                               'faqs' => [],
-                                               'has_content' => false
-                                             })
-                }
-              }
-            ]
-          }
-        end
+        faqs = service.generate
 
-        it 'generates FAQs from document' do
-          allow(openai_client).to receive(:chat).and_return(faq_response, empty_response)
-
-          result = service.generate
-          expect(result).to be_an(Array)
-          expect(result.first).to include('question', 'answer')
-        end
-
-        it 'processes multiple chunks until no more content' do
-          allow(openai_client).to receive(:chat).and_return(faq_response, faq_response, empty_response)
-
-          result = service.generate
-          expect(result.count).to eq(2) # Deduplication removes duplicates
-        end
-
-        it 'deduplicates similar FAQs' do
-          duplicate_response = {
-            'choices' => [
-              {
-                'message' => {
-                  'content' => JSON.generate({
-                                               'faqs' => [
-                                                 { 'question' => 'What is the product?', 'answer' => 'It is a great product.' },
-                                                 { 'question' => 'What is the product?', 'answer' => 'Different answer.' }
-                                               ],
-                                               'has_content' => false
-                                             })
-                }
-              }
-            ]
-          }
-
-          allow(openai_client).to receive(:chat).and_return(duplicate_response)
-
-          result = service.generate
-          expect(result.count).to eq(1)
-        end
-
-        it 'tracks total pages processed' do
-          allow(openai_client).to receive(:chat).and_return(faq_response, faq_response, empty_response)
-
-          service.generate
-          expect(service.total_pages_processed).to eq(30) # 3 chunks of 10 pages each
-        end
-
-        it 'tracks iterations completed' do
-          allow(openai_client).to receive(:chat).and_return(faq_response, faq_response, empty_response)
-
-          service.generate
-          expect(service.iterations_completed).to eq(3)
-        end
+        expect(faqs).to have_attributes(size: 1)
+        expect(faqs.first['question']).to eq('What is this document about?')
       end
 
-      context 'when API returns error' do
-        before do
-          allow(openai_client).to receive(:chat).and_raise(OpenAI::Error, 'API error')
-        end
+      it 'stops when no more content' do
+        allow(openai_client).to receive(:chat).and_return(empty_response)
 
-        it 'returns empty array and logs error' do
-          expect(Rails.logger).to receive(:error).with(/Error processing pages/)
+        faqs = service.generate
 
-          result = service.generate
-          expect(result).to eq([])
-        end
+        expect(faqs).to be_empty
       end
 
-      context 'when response is malformed' do
-        let(:malformed_response) do
-          {
-            'choices' => [
-              {
-                'message' => {
-                  'content' => 'Invalid JSON'
-                }
-              }
-            ]
-          }
-        end
+      it 'respects max iterations limit' do
+        allow(openai_client).to receive(:chat).and_return(faq_response)
 
-        before do
-          allow(openai_client).to receive(:chat).and_return(malformed_response)
-        end
+        # Force max iterations
+        service.instance_variable_set(:@iterations_completed, 19)
 
-        it 'handles JSON parsing errors gracefully' do
-          expect(Rails.logger).to receive(:error).with(/Error parsing chunk response/)
-
-          result = service.generate
-          expect(result).to eq([])
-        end
+        service.generate
+        expect(service.iterations_completed).to eq(20)
       end
     end
   end
 
   describe '#should_continue_processing?' do
-    let(:chunk_result_with_faqs) { { faqs: [{ 'question' => 'Q1', 'answer' => 'A1' }], has_content: true } }
-    let(:chunk_result_empty) { { faqs: [], has_content: false } }
-
-    it 'returns false when max iterations reached' do
+    it 'stops at max iterations' do
       service.instance_variable_set(:@iterations_completed, 20)
-      expect(service.should_continue_processing?(chunk_result_with_faqs)).to be false
+      expect(service.should_continue_processing?(faqs: ['faq'], has_content: true)).to be false
     end
 
-    it 'returns false when max pages reached' do
-      service.instance_variable_set(:@max_pages, 30)
-      service.instance_variable_set(:@total_pages_processed, 30)
-      expect(service.should_continue_processing?(chunk_result_with_faqs)).to be false
+    it 'stops when no FAQs returned' do
+      expect(service.should_continue_processing?(faqs: [], has_content: true)).to be false
     end
 
-    it 'returns false when no FAQs returned' do
-      expect(service.should_continue_processing?({ faqs: [], has_content: true })).to be false
-    end
-
-    it 'returns false when has_content is false' do
-      expect(service.should_continue_processing?({ faqs: ['faq'], has_content: false })).to be false
-    end
-
-    it 'returns true when should continue' do
-      expect(service.should_continue_processing?(chunk_result_with_faqs)).to be true
-    end
-  end
-
-  describe 'private methods' do
-    describe '#similarity_score' do
-      it 'returns 1 for identical strings' do
-        score = service.send(:similarity_score, 'Hello world', 'Hello world')
-        expect(score).to eq(1.0)
-      end
-
-      it 'returns 0 for completely different strings' do
-        score = service.send(:similarity_score, 'Hello', 'Goodbye')
-        expect(score).to eq(0.0)
-      end
-
-      it 'calculates partial similarity correctly' do
-        score = service.send(:similarity_score, 'Hello world', 'Hello there')
-        expect(score).to be_between(0.3, 0.4)
-      end
-    end
-
-    describe '#deduplicate_faqs' do
-      let(:faqs_with_duplicates) do
-        [
-          { 'question' => 'What is the product?', 'answer' => 'Answer 1' },
-          { 'question' => 'What is the product?', 'answer' => 'Answer 2' },
-          { 'question' => 'What is the product really?', 'answer' => 'Answer 3' },
-          { 'question' => 'How does it work?', 'answer' => 'Answer 4' }
-        ]
-      end
-
-      it 'removes exact duplicates' do
-        result = service.send(:deduplicate_faqs, faqs_with_duplicates)
-        questions = result.map { |f| f['question'] }
-        expect(questions.count('What is the product?')).to eq(1)
-      end
-
-      it 'removes similar questions' do
-        result = service.send(:deduplicate_faqs, faqs_with_duplicates)
-        expect(result.count).to be < faqs_with_duplicates.count
-      end
-    end
-
-    describe '#determine_stop_reason' do
-      it 'identifies max iterations reason' do
-        service.instance_variable_set(:@iterations_completed, 20)
-        reason = service.send(:determine_stop_reason, { faqs: ['faq'] })
-        expect(reason).to eq('Maximum iterations reached')
-      end
-
-      it 'identifies max pages reason' do
-        service.instance_variable_set(:@max_pages, 30)
-        service.instance_variable_set(:@total_pages_processed, 30)
-        reason = service.send(:determine_stop_reason, { faqs: ['faq'] })
-        expect(reason).to eq('Maximum pages processed')
-      end
-
-      it 'identifies no content reason' do
-        reason = service.send(:determine_stop_reason, { faqs: [] })
-        expect(reason).to eq('No content found in last chunk')
-      end
-
-      it 'identifies end of document reason' do
-        reason = service.send(:determine_stop_reason, { faqs: ['faq'], has_content: false })
-        expect(reason).to eq('End of document reached')
-      end
-    end
-
-    describe '#calculate_end_page' do
-      it 'calculates end page correctly' do
-        service.instance_variable_set(:@pages_per_chunk, 10)
-        expect(service.send(:calculate_end_page, 1)).to eq(10)
-        expect(service.send(:calculate_end_page, 11)).to eq(20)
-      end
-
-      it 'respects max_pages limit' do
-        service.instance_variable_set(:@pages_per_chunk, 10)
-        service.instance_variable_set(:@max_pages, 15)
-        expect(service.send(:calculate_end_page, 11)).to eq(15)
-      end
-    end
-
-    describe '#build_chunk_parameters' do
-      before do
-        allow(document).to receive(:openai_file_id).and_return('file-123')
-        allow(Captain::Llm::SystemPromptsService).to receive(:paginated_faq_generator)
-          .and_return('Generate FAQs from pages')
-      end
-
-      it 'builds correct parameters structure' do
-        params = service.send(:build_chunk_parameters, 1, 10)
-
-        expect(params[:model]).to eq('gpt-4.1-mini')
-        expect(params[:response_format]).to eq({ type: 'json_object' })
-        expect(params[:messages]).to be_an(Array)
-        expect(params[:messages].first[:role]).to eq('user')
-      end
-
-      it 'includes file reference and prompt' do
-        params = service.send(:build_chunk_parameters, 1, 10)
-        user_content = params[:messages].first[:content]
-
-        expect(user_content).to be_an(Array)
-        expect(user_content.first[:type]).to eq('file')
-        expect(user_content.first[:file][:file_id]).to eq('file-123')
-        expect(user_content.last[:type]).to eq('text')
-      end
+    it 'continues when FAQs exist and under limits' do
+      expect(service.should_continue_processing?(faqs: ['faq'], has_content: true)).to be true
     end
   end
 end

spec/enterprise/services/captain/llm/pdf_processing_service_spec.rb

@@ -1,13 +1,12 @@
 require 'rails_helper'
+require 'custom_exceptions/pdf_processing_error'
 
 RSpec.describe Captain::Llm::PdfProcessingService do
-  let(:account) { create(:account) }
-  let(:assistant) { create(:captain_assistant, account: account) }
-  let(:document) { create(:captain_document, account: account, assistant: assistant) }
+  let(:document) { create(:captain_document) }
   let(:service) { described_class.new(document) }
 
   before do
-    # Mock the base class dependencies
+    # Mock OpenAI configuration
     installation_config = instance_double(InstallationConfig, value: 'test-api-key')
     allow(InstallationConfig).to receive(:find_by!)
       .with(name: 'CAPTAIN_OPEN_AI_API_KEY')
@@ -15,43 +14,44 @@ RSpec.describe Captain::Llm::PdfProcessingService do
   end
 
   describe '#process' do
-    context 'when document already has openai_file_id' do
+    context 'when document already has OpenAI file ID' do
       before do
         allow(document).to receive(:openai_file_id).and_return('existing-file-id')
       end
 
-      it 'returns success message without uploading' do
-        result = service.process
-        expect(result).to eq('PDF ready for paginated processing')
+      it 'skips upload' do
+        expect(document).not_to receive(:store_openai_file_id)
+        service.process
       end
     end
 
-    context 'when document needs upload' do
+    context 'when uploading PDF to OpenAI' do
       let(:mock_client) { instance_double(OpenAI::Client) }
+      let(:pdf_content) { 'PDF content' }
 
       before do
         allow(document).to receive(:openai_file_id).and_return(nil)
 
-        # Create a mock for pdf_file that responds to download
-        pdf_file = Struct.new(:download).new('pdf content')
+        # Use a simple double for ActiveStorage since it's a complex Rails object
+        pdf_file = double('pdf_file', download: pdf_content) # rubocop:disable RSpec/VerifiedDoubles
         allow(document).to receive(:pdf_file).and_return(pdf_file)
 
-        # Mock OpenAI client
         allow(OpenAI::Client).to receive(:new).and_return(mock_client)
-        files_api = Object.new
-        files_api.define_singleton_method(:upload) { |_params| { 'id' => 'file-abc123' } }
+        # Use a simple double for OpenAI::Files as it may not be loaded
+        files_api = double('files_api') # rubocop:disable RSpec/VerifiedDoubles
+        allow(files_api).to receive(:upload).and_return({ 'id' => 'file-abc123' })
         allow(mock_client).to receive(:files).and_return(files_api)
-
-        allow(document).to receive(:store_openai_file_id)
       end
 
-      it 'uploads PDF and returns success message' do
+      it 'uploads PDF and stores file ID' do
         expect(document).to receive(:store_openai_file_id).with('file-abc123')
+        service.process
+      end
 
-        result = service.process
+      it 'raises error when upload fails' do
+        allow(mock_client.files).to receive(:upload).and_return({ 'id' => nil })
 
-        expect(result).to include('file-abc123')
-        expect(result).to include('PDF ready for paginated processing')
+        expect { service.process }.to raise_error(CustomExceptions::PdfUploadError)
       end
     end
   end