diff --git a/app/api/api_root.rb b/app/api/api_root.rb index da3bd45fa..2817baa83 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -103,6 +103,7 @@ class ApiRoot < Grape::API mount WebcalPublicApi mount MarkingSessionsApi mount DiscussionPromptsApi + mount OverseerStepsApi mount Feedback::FeedbackChipApi @@ -152,6 +153,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to Feedback::FeedbackChipApi AuthenticationHelpers.add_auth_to MarkingSessionsApi AuthenticationHelpers.add_auth_to DiscussionPromptsApi + AuthenticationHelpers.add_auth_to OverseerStepsApi add_swagger_documentation \ base_path: nil, diff --git a/app/api/entities/overseer_assessment_entity.rb b/app/api/entities/overseer_assessment_entity.rb index b8402dcf5..2d26ab47c 100644 --- a/app/api/entities/overseer_assessment_entity.rb +++ b/app/api/entities/overseer_assessment_entity.rb @@ -7,5 +7,8 @@ class OverseerAssessmentEntity < Grape::Entity expose :status expose :created_at expose :updated_at + + expose :total_steps + expose :passed_steps end end diff --git a/app/api/entities/overseer_step_entity.rb b/app/api/entities/overseer_step_entity.rb new file mode 100644 index 000000000..b230b577c --- /dev/null +++ b/app/api/entities/overseer_step_entity.rb @@ -0,0 +1,47 @@ +module Entities + class OverseerStepEntity < Grape::Entity + expose :id + expose :task_definition_id + + def staff?(my_role) + Role.teaching_staff_ids.include?(my_role.id) unless my_role.nil? + end + + expose :name, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :description, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :display_name + expose :display_description + + expose :run_command, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :timeout, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :sort_order, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :step_type + expose :partial_output_diff, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :stdin_input_file, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :expected_output_file, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :feedback_message, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :status_on_success, + if: ->(_obj, options) { staff?(options[:my_role]) } do |overseer_step| + TaskStatus.find_by(id: overseer_step.status_on_success_id)&.status_key || 'no_change' + end + + expose :status_on_failure, + if: ->(_obj, options) { staff?(options[:my_role]) } do |overseer_step| + TaskStatus.find_by(id: overseer_step.status_on_failure_id)&.status_key || 'no_change' + end + + expose :halt_on_success, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :halt_on_failure, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :show_expected_output, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :show_stdin, if: ->(_unit, options) { staff?(options[:my_role]) } + expose :show_stdout, if: ->(_unit, options) { staff?(options[:my_role]) } + + expose :enabled + end +end diff --git a/app/api/entities/overseer_step_result_entity.rb b/app/api/entities/overseer_step_result_entity.rb new file mode 100644 index 000000000..eeebd0caa --- /dev/null +++ b/app/api/entities/overseer_step_result_entity.rb @@ -0,0 +1,30 @@ +module Entities + class OverseerStepResultEntity < Grape::Entity + + def staff?(my_role) + Role.teaching_staff_ids.include?(my_role.id) unless my_role.nil? + end + + expose :id + expose :overseer_step_id + expose :exit_status + expose :pass + expose :feedback_message + + expose :stdout, if: lambda { |result, options| + staff?(options[:my_role]) || result.overseer_step&.show_stdout + } + + expose :stdin, if: lambda { |result, options| + staff?(options[:my_role]) || result.overseer_step&.show_stdin + } + + expose :expected_output, if: lambda { |result, options| + staff?(options[:my_role]) || result.overseer_step&.show_expected_output + } + + expose :stdout_sha256 + expose :stdin_sha256 + expose :expected_output_sha256 + end +end diff --git a/app/api/entities/task_definition_entity.rb b/app/api/entities/task_definition_entity.rb index 3aa2afd6a..fda5e31ce 100644 --- a/app/api/entities/task_definition_entity.rb +++ b/app/api/entities/task_definition_entity.rb @@ -50,7 +50,8 @@ def staff?(my_role) expose :is_graded expose :max_quality_pts expose :overseer_image_id, if: ->(unit, options) { staff?(options[:my_role]) }, expose_nil: false - expose :assessment_enabled, if: ->(unit, options) { staff?(options[:my_role]) } + # expose :assessment_enabled, if: ->(unit, options) { staff?(options[:my_role]) } + expose :assessment_enabled expose :similarity_language, if: ->(unit, options) { staff?(options[:my_role]) }, expose_nil: false expose :assess_in_portfolio_only expose :use_resources_for_jplag_base_code, if: ->(unit, options) { staff?(options[:my_role]) } @@ -61,5 +62,12 @@ def staff?(my_role) expose :discussion_prompts_count do |task_def| task_def.discussion_prompts.size end + + # expose :overseer_steps, using: OverseerStepEntity, if: ->(unit, options) { staff?(options[:my_role]) } + expose :overseer_steps, using: OverseerStepEntity do |task_def, options| + task_def.overseer_steps # options[:my_role] is still available inside the entity + end + expose :overseer_resource_files, if: ->(task_def, options) { staff?(options[:my_role]) } + end end diff --git a/app/api/overseer_steps_api.rb b/app/api/overseer_steps_api.rb new file mode 100644 index 000000000..9b695b6dd --- /dev/null +++ b/app/api/overseer_steps_api.rb @@ -0,0 +1,200 @@ +require 'grape' + +class OverseerStepsApi < Grape::API + helpers AuthenticationHelpers + helpers AuthorisationHelpers + helpers SidekiqHelper + + before do + authenticated? + end + + desc 'Add an overseer step' + params do + requires :overseer_step, type: Hash do + requires :name, type: String + optional :description, type: String + optional :display_name, type: String + optional :display_description, type: String + optional :run_command, type: String + optional :timeout, type: Integer + # TODO: rename to execution_order || exec_order? + optional :sort_order, type: Integer + optional :partial_output_diff, type: Boolean + requires :step_type, type: String + optional :stdin_input_file, type: String + optional :expected_output_file, type: String + optional :feedback_message, type: String + optional :status_on_success, type: String + optional :status_on_failure, type: String + optional :halt_on_success, type: Boolean + optional :halt_on_failure, type: Boolean + optional :show_expected_output, type: Boolean + optional :show_stdin, type: Boolean + optional :show_stdout, type: Boolean + optional :enabled, type: Boolean + end + requires :task_def_id, type: Integer + end + post '/units/:unit_id/task_definitions/:task_def_id/overseer_steps' do + unless Doubtfire::Application.config.overseer_enabled + error!({ error: 'Overseer is not enabled. Enable Overseer before updating settings.' }, 403) + end + + task_definition = TaskDefinition.find(params[:task_def_id]) + + unless authorise? current_user, task_definition, :manage_overseer_steps + error!({ error: 'Not authorised to manage overseer for this task definition' }, 403) + end + + status_on_success_param = params[:overseer_step][:status_on_success] + status_on_failure_param = params[:overseer_step][:status_on_failure] + + status_on_success_id = status_on_success_param.present? && status_on_success_param != 'no_change' ? TaskStatus.status_for_name(status_on_success_param)&.id : nil + status_on_failure_id = status_on_failure_param.present? && status_on_failure_param != 'no_change' ? TaskStatus.status_for_name(status_on_failure_param)&.id : nil + + overseer_step_params = ActionController::Parameters.new(params) + .require(:overseer_step) + .permit( + :name, + :description, + :display_name, + :display_description, + :run_command, + :timeout, + :sort_order, + :step_type, + :partial_output_diff, + :stdin_input_file, + :expected_output_file, + :feedback_message, + :status_on_success_id, + :status_on_failure_id, + :halt_on_success, + :halt_on_failure, + :show_expected_output, + :show_stdin, + :show_stdout, + :enabled + ) + .merge(task_definition_id: task_definition.id, + status_on_success_id: status_on_success_id, + status_on_failure_id: status_on_failure_id) + + result = OverseerStep.create!(overseer_step_params) + + if result.nil? + error!({ error: 'No overseer step added' }, 403) + else + present result, with: Entities::OverseerStepEntity + end + end + + desc 'Update an overseer step' + params do + requires :overseer_step, type: Hash do + optional :name, type: String + optional :description, type: String + optional :display_name, type: String + optional :display_description, type: String + optional :run_command, type: String + optional :timeout, type: Integer + optional :sort_order, type: Integer + optional :step_type, type: String + optional :partial_output_diff, type: Boolean + optional :stdin_input_file, type: String + optional :expected_output_file, type: String + optional :feedback_message, type: String + optional :status_on_success, type: String + optional :status_on_failure, type: String + optional :halt_on_success, type: Boolean + optional :halt_on_failure, type: Boolean + optional :show_expected_output, type: Boolean + optional :show_stdin, type: Boolean + optional :show_stdout, type: Boolean + optional :enabled, type: Boolean + end + requires :task_def_id, type: Integer + end + put '/units/:unit_id/task_definitions/:task_def_id/overseer_steps/:id' do + unless Doubtfire::Application.config.overseer_enabled + error!({ error: 'Overseer is not enabled. Enable Overseer before updating settings.' }, 403) + end + + unit = Unit.find(params[:unit_id]) + task_definition = unit.task_definitions.find(params[:task_def_id]) + overseer_step = task_definition.overseer_steps.find(params[:id]) + + unless authorise? current_user, overseer_step.task_definition, :manage_overseer_steps + error!({ error: 'Not authorised to manage overseer for this task definition' }, 403) + end + + status_on_success_param = params[:overseer_step][:status_on_success] + status_on_failure_param = params[:overseer_step][:status_on_failure] + + status_on_success_id = status_on_success_param.present? && status_on_success_param != 'no_change' ? TaskStatus.status_for_name(status_on_success_param)&.id : nil + status_on_failure_id = status_on_failure_param.present? && status_on_failure_param != 'no_change' ? TaskStatus.status_for_name(status_on_failure_param)&.id : nil + + overseer_step_params = ActionController::Parameters.new(params) + .require(:overseer_step) + .permit( + :name, + :description, + :display_name, + :display_description, + :run_command, + :timeout, + :sort_order, + :step_type, + :partial_output_diff, + :stdin_input_file, + :expected_output_file, + :feedback_message, + :status_on_success_id, + :status_on_failure_id, + :halt_on_success, + :halt_on_failure, + :show_expected_output, + :show_stdin, + :show_stdout, + :enabled + ) + .merge( + status_on_success_id: status_on_success_id, + status_on_failure_id: status_on_failure_id + ) + + overseer_step.update!(overseer_step_params) + + present overseer_step, with: Entities::OverseerStepEntity + end + + desc 'Delete an overseer step' + delete '/overseer_steps/:id' do + overseer_step = OverseerStep.find(params[:id]) + + unless authorise? current_user, overseer_step.task_definition, :manage_overseer_steps + error!({ error: 'Not authorised to manage overseer for this task definition' }, 403) + end + + overseer_step.destroy! + + error!({ error: overseer_step.errors.full_messages.last }, 403) unless overseer_step.destroyed? + + present overseer_step.destroyed?, with: Grape::Presenters::Presenter + end + + desc 'Get test results for an overseer assessment' + get '/projects/:project_id/task_definitions/:task_def_id/overseer_assessments_results/:id' do + project = Project.find(params[:project_id]) + + unless authorise? current_user, project, :get_submission + error!({ error: 'Not authorised to view this project' }, 403) + end + + unit = project.unit + + overseer_assessment = OverseerAssessment.find(params[:id]) + present overseer_assessment.overseer_step_results, with: Entities::OverseerStepResultEntity, my_role: unit.role_for(current_user) + end +end diff --git a/app/api/task_definitions_api.rb b/app/api/task_definitions_api.rb index c5e031279..b4fd9a127 100644 --- a/app/api/task_definitions_api.rb +++ b/app/api/task_definitions_api.rb @@ -335,17 +335,17 @@ class TaskDefinitionsApi < Grape::API upload_reqs = task.upload_requirements # Copy files to be PDFed - task.accept_submission(current_user, scoop_files(params, upload_reqs), self, nil, 'ready_for_feedback', nil, accepted_tii_eula: false) + task.accept_submission(current_user, scoop_files(params, upload_reqs), self, nil, 'ready_for_feedback', nil, accepted_tii_eula: false, test_submission: true) - logger.info '********* - about to perform overseer submission' - overseer_assessment = OverseerAssessment.create_for(task) - if overseer_assessment.present? - overseer_assessment.send_to_overseer + # logger.info '********* - about to perform overseer submission' + # overseer_assessment = OverseerAssessment.create_for(task) + # if overseer_assessment.present? + # overseer_assessment.send_to_overseer - logger.info "Overseer assessment for task_def_id: #{task_definition.id} task_id: #{task.id} was performed" - else - logger.info "Overseer assessment for task_def_id: #{task_definition.id} task_id: #{task.id} was not performed" - end + # logger.info "Overseer assessment for task_def_id: #{task_definition.id} task_id: #{task.id} was performed" + # else + # logger.info "Overseer assessment for task_def_id: #{task_definition.id} task_id: #{task.id} was not performed" + # end # todo: Do we need to return additional details here? e.g. the comment, and project? present task, with: Entities::TaskEntity, include_other_projects: true, update_only: true @@ -880,45 +880,45 @@ class TaskDefinitionsApi < Grape::API present job, with: Entities::SidekiqJobEntity end - desc 'Retrieve the contents of the overseer execution script' - params do - requires :unit_id, type: Integer, desc: 'The unit that has the task definition' - requires :task_def_id, type: Integer, desc: 'The task definition to download submissions for' - end - get '/units/:unit_id/task_definitions/:task_def_id/overseer_script' do - unit = Unit.find(params[:unit_id]) - unless authorise? current_user, unit, :add_task_def - error!({ error: 'Not authorised to edit task details of unit' }, 403) - end + # desc 'Retrieve the contents of the overseer execution script' + # params do + # requires :unit_id, type: Integer, desc: 'The unit that has the task definition' + # requires :task_def_id, type: Integer, desc: 'The task definition to download submissions for' + # end + # get '/units/:unit_id/task_definitions/:task_def_id/overseer_script' do + # unit = Unit.find(params[:unit_id]) + # unless authorise? current_user, unit, :add_task_def + # error!({ error: 'Not authorised to edit task details of unit' }, 403) + # end - td = unit.task_definitions.find(params[:task_def_id]) + # td = unit.task_definitions.find(params[:task_def_id]) - script_path = td.task_assessment_script + # script_path = td.task_assessment_script - content = File.read(script_path) - content - end + # content = File.read(script_path) + # content + # end - desc 'Update the contents of the overseer execution script' - params do - requires :unit_id, type: Integer, desc: 'The unit that has the task definition' - requires :task_def_id, type: Integer, desc: 'The task definition to download submissions for' - requires :script_content, type: String, desc: 'Content of the overseer execution script' - end - put '/units/:unit_id/task_definitions/:task_def_id/overseer_script' do - unit = Unit.find(params[:unit_id]) - unless authorise? current_user, unit, :add_task_def - error!({ error: 'Not authorised to edit task details of unit' }, 403) - end + # desc 'Update the contents of the overseer execution script' + # params do + # requires :unit_id, type: Integer, desc: 'The unit that has the task definition' + # requires :task_def_id, type: Integer, desc: 'The task definition to download submissions for' + # requires :script_content, type: String, desc: 'Content of the overseer execution script' + # end + # put '/units/:unit_id/task_definitions/:task_def_id/overseer_script' do + # unit = Unit.find(params[:unit_id]) + # unless authorise? current_user, unit, :add_task_def + # error!({ error: 'Not authorised to edit task details of unit' }, 403) + # end - td = unit.task_definitions.find(params[:task_def_id]) + # td = unit.task_definitions.find(params[:task_def_id]) - script_path = td.task_assessment_script + # script_path = td.task_assessment_script - decoded = Base64.urlsafe_decode64(params[:script_content]) + # decoded = Base64.urlsafe_decode64(params[:script_content]) - File.write(script_path, decoded) - status 200 - end + # File.write(script_path, decoded) + # status 200 + # end end diff --git a/app/models/comments/assessment_comment.rb b/app/models/comments/assessment_comment.rb index 5c281d32c..1b614b41f 100644 --- a/app/models/comments/assessment_comment.rb +++ b/app/models/comments/assessment_comment.rb @@ -6,6 +6,9 @@ class AssessmentComment < TaskComment def serialize(user) json = super(user) json[:overseer_assessment_id] = self.commentable_id + json[:overseer_total_steps] = self.commentable.total_steps + json[:overseer_passed_steps] = self.commentable.passed_steps + json[:overseer_status] = self.commentable.status json end end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index f15063505..5fd5fe80c 100644 --- a/app/models/overseer_assessment.rb +++ b/app/models/overseer_assessment.rb @@ -4,6 +4,7 @@ class OverseerAssessment < ApplicationRecord has_one :project, through: :task has_many :assessment_comments, as: :commentable, dependent: :destroy + has_many :overseer_step_results, dependent: :destroy validates :status, presence: true validates :task_id, presence: true @@ -11,12 +12,15 @@ class OverseerAssessment < ApplicationRecord validates :submission_timestamp, uniqueness: { scope: :task_id } - enum :status, { pre_queued: 0, queued: 1, queue_failed: 2, done: 3 } + enum :status, { pre_queued: 0, passed: 1, failed: 2 } after_destroy :delete_associated_files + # TODO: track how many tests ran, and how many tests total at the time + # TODO: we might not have an overseerStepResult because a new test was added later + # Creates an OverseerAssessment object for a new submission - def self.create_for(task) + def self.create_for(task, test_submission) # Create only if: # unit's assessment is enabled && # task's assessment is enabled && @@ -26,7 +30,7 @@ def self.create_for(task) task_definition = task.task_definition unit = task_definition.unit - return nil unless task.overseer_enabled? + return nil unless task.overseer_enabled? || test_submission docker_image_name_tag = task_definition.docker_image_name_tag || unit.docker_image_name_tag # assessment_resources_path = task_definition.task_assessment_resources @@ -118,7 +122,7 @@ def update_assessment_comment(text) add_assessment_comment text end - def send_to_overseer() + def send_to_overseer(test_submission: false) return { error: "Your task is already queued for processing. Pleasse wait until you receive a response before queueing your task again." } if self.status == :queued # TODO: Check status and do not queue if already queued @@ -140,10 +144,10 @@ def send_to_overseer() assessment_resources_path = task_definition.task_assessment_resources - unless unit.assessment_enabled && - task_definition.assessment_enabled && - task_definition.has_task_assessment_script? && - (task.has_new_files? || task.has_done_file?) + unless unit.assessment_enabled && + (task_definition.assessment_enabled || test_submission) && + # task_definition.has_task_assessment_script? && + (task.has_new_files? || task.has_done_file?) puts "ERROR: Assessment is no longer configured for overseer assessment. Unable to send - OverseerAssessment #{id}" return { error: "This assessment is no longer setup for automated feedback. Automated feedback is turned off at either the unit or task level, or the task does not have the scripts needed to automate assessment." } @@ -259,5 +263,10 @@ def delete_associated_files def base64?(value) value.is_a?(String) && Base64.strict_encode64(Base64.decode64(value)) == value end + + + def passed_steps + overseer_step_results.select(&:pass).size + end end # rubocop:enable Rails/Output diff --git a/app/models/overseer_step.rb b/app/models/overseer_step.rb new file mode 100644 index 000000000..6273f0db0 --- /dev/null +++ b/app/models/overseer_step.rb @@ -0,0 +1,6 @@ +class OverseerStep < ApplicationRecord + belongs_to :task_definition, optional: false + + validates :timeout, presence: true, numericality: { greater_than_or_equal_to: 1, less_than_or_equal_to: 300, message: 'must be between 1 and 300' } + validates :sort_order, presence: true, numericality: { greater_than_or_equal_to: 0 } +end diff --git a/app/models/overseer_step_result.rb b/app/models/overseer_step_result.rb new file mode 100644 index 000000000..21e482158 --- /dev/null +++ b/app/models/overseer_step_result.rb @@ -0,0 +1,5 @@ +class OverseerStepResult < ApplicationRecord + belongs_to :overseer_assessment, optional: false + belongs_to :overseer_step, optional: false + +end diff --git a/app/models/task.rb b/app/models/task.rb index 92644d42e..e6a166fc6 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -1403,7 +1403,7 @@ def create_submission_and_trigger_state_change(user, propagate = true, contribut # # Checks to make sure that the files match what we expect # - def accept_submission(current_user, files, ui, contributions, trigger, alignments, accepted_tii_eula: false) + def accept_submission(current_user, files, ui, contributions, trigger, alignments, accepted_tii_eula: false, test_submission: false) # Ensure there is not a submission already in process if processing_pdf? ui.error!({ 'error' => 'A submission is already being processed. Please wait for the current submission process to complete.' }, 403) @@ -1502,7 +1502,7 @@ def accept_submission(current_user, files, ui, contributions, trigger, alignment logger.info "Submission accepted! Status for task #{id} is now #{trigger}" # Trigger processing of new submission - async - AcceptSubmissionJob.perform_async(id, current_user.id, accepted_tii_eula) + AcceptSubmissionJob.perform_async(id, current_user.id, accepted_tii_eula, test_submission) end # The name that should be used for the uploaded file (based on index of upload requirements) @@ -1573,7 +1573,7 @@ def archive_submission def overseer_enabled? return unit.assessment_enabled && task_definition.assessment_enabled && - task_definition.has_task_assessment_script? && + # task_definition.has_task_assessment_script? && (has_new_files? || has_done_file?) end diff --git a/app/models/task_definition.rb b/app/models/task_definition.rb index 652d1b4ce..7902ca942 100644 --- a/app/models/task_definition.rb +++ b/app/models/task_definition.rb @@ -15,7 +15,8 @@ def self.permissions :get_los, :create_task_prerequisite, :get_discussion_prompt, - :create_discussion_prompt + :create_discussion_prompt, + :manage_overseer_steps ] admin_role_permissions = [ @@ -26,7 +27,8 @@ def self.permissions :get_los, :create_task_prerequisite, :get_discussion_prompt, - :create_discussion_prompt + :create_discussion_prompt, + :manage_overseer_steps ] tutor_role_permissions = [ @@ -71,6 +73,7 @@ def self.permissions has_many :tasks, dependent: :destroy # Destroying a task definition will also nuke any instances has_many :group_submissions, dependent: :destroy # Destroying a task definition will also nuke any group submissions has_many :learning_outcomes, as: :context, dependent: :destroy + has_many :overseer_steps, -> { order(:sort_order) }, inverse_of: :task_definition, dependent: :destroy has_many :tii_group_attachments, dependent: :destroy # destroy uploaded files to tii - after the tasks has_many :tii_actions, as: :entity, dependent: :destroy @@ -302,7 +305,7 @@ def check_upload_requirements_format end # Check the name matches a valid filename format - unless req['name'].match?(/^[a-zA-Z0-9_\- \.]+$/) + unless req['name'].match?(/^[a-zA-Z0-9_\- .]+$/) errors.add(:upload_requirements, "the name for item #{i + 1} does not seem to be a valid filename --> #{req['name']}.") end @@ -733,6 +736,24 @@ def task_assessment_resources(create = true) task_assessment_resources_with_abbreviation(abbreviation, create) end + def overseer_resource_files + return [] unless File.exist?(task_assessment_resources) + + files = [] + Zip::File.open(task_assessment_resources) do |zip_file| + zip_file.each do |entry| + next if entry.directory? + # skip macOS metadata files and hidden files + next if File.basename(entry.name).start_with?('._', '.') + + # remove top-level folder + parts = entry.name.split('/', 2) + files << "/#{parts.last}" unless parts.empty? + end + end + files + end + def task_assessment_script(create = true) task_assessment_script_with_abbreviation(abbreviation, create) end diff --git a/app/sidekiq/accept_overseer_job.rb b/app/sidekiq/accept_overseer_job.rb index f812efa5f..c48800868 100644 --- a/app/sidekiq/accept_overseer_job.rb +++ b/app/sidekiq/accept_overseer_job.rb @@ -1,4 +1,5 @@ require 'yaml' +require 'open3' class AcceptOverseerJob include Sidekiq::Job @@ -20,96 +21,238 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment total(1) task = Task.find(task_id) + task_definition = task.task_definition + + raise "PDF is still compiling" if task.processing_pdf? || !task.has_done_file? + + raise "Submission file not found: #{submission}" unless File.exist?(submission) + + active_overseer_steps = task_definition.overseer_steps.select(&:enabled) + + raise "Task definition has no enabled overseer steps #{task.unit.detailed_name} #{task_definition.abbreviation}" if active_overseer_steps.empty? + + oa = OverseerAssessment.find(overseer_assessment_id) + oa.update!( + total_steps: active_overseer_steps.size + ) work_dir_name = "#{task.id}-#{overseer_assessment_id}" work_dir = Rails.root.join("tmp", "overseer", work_dir_name) FileUtils.mkdir_p(work_dir) - raise "PDF is still compiling" if task.processing_pdf? || !task.has_done_file? + extract_student_submission_files(task, submission, work_dir) + extract_overseer_resource_files(assessment, work_dir) - raise "Submission file not found: #{submission}" unless File.exist?(submission) + success_status = nil + failure_status = nil - # Extract submission files, removing any parent folders - Zip::File.open(submission) do |zip_file| - zip_file.each do |entry| - next if entry.name_is_directory? + oa.add_assessment_comment("Tests in progress") - parts = entry.name.split('/')[1..] - next unless parts.length >= 1 + steps_attempted = 0 + steps_passed = 0 - file_name = parts.first - index = file_name.to_i + assessment_pass = true - file = task.upload_requirements[index] - final_name = file['name'] + active_overseer_steps.each do |step| + result = run_overseer_step( + step: step, + work_dir: work_dir, + work_dir_name: work_dir_name, + task_id: task_id, + timestamp: timestamp, + docker_image_name_tag: docker_image_name_tag, + overseer_assessment_id: overseer_assessment_id + ) - dest_path = File.join(work_dir, final_name) - FileUtils.mkdir_p(File.dirname(dest_path)) - zip_file.extract(entry, dest_path) { true } + steps_attempted += 1 + + if result.valid? && result.pass + steps_passed += 1 + if step.halt_on_success && step.status_on_success_id + success_status = TaskStatus.find(step.status_on_success_id) + break + end + elsif step.halt_on_failure + failure_status = TaskStatus.find(step.status_on_failure_id) if step.status_on_failure_id + assessment_pass = false + break end end - # Extract optional assessment resources - if File.exist?(assessment) - Zip::File.open(assessment) do |zip_file| - zip_file.each do |entry| - dest_path = File.join(work_dir, entry.name) - FileUtils.mkdir_p(File.dirname(dest_path)) - zip_file.extract(entry, dest_path) { true } # overwrite if exists - end + oa.update_assessment_comment("Tests complete: #{steps_passed} / #{active_overseer_steps.count}") + + if steps_attempted == steps_passed && assessment_pass + oa.update!(status: :passed) + unless success_status.nil? + # TODO: have an override status setting for the step? eg. if the task is overdue, let it remain overdue, otherwise use this task status + task.update!(task_status: success_status) + task.add_status_comment(task.project.tutor_for(task.task_definition), success_status) + + oa.update!(result_task_status: success_status.status_key.to_s) end + else + oa.update!(status: :failed) + unless failure_status.nil? + # TODO: have an override status setting for the step? eg. if the task is overdue, let it remain overdue, otherwise use this task status + task.update!(task_status: failure_status) + task.add_status_comment(task.project.tutor_for(task.task_definition), failure_status) + oa.update!(result_task_status: failure_status.status_key.to_s) + end + task.add_text_comment(task.project.tutor_for(task.task_definition), "**Automated comment**: Some tests did not pass for this submission. Please review the Overseer report, verify your output, and resubmit.") end - # Extract execution script - script_path = task.task_definition.task_assessment_script - raise "No execution script found" unless File.exist?(script_path) + logger.info "Completed overseer job" + rescue StandardError => e + logger.error e + raise e + end - script_contents = File.read(script_path) + def run_overseer_step(step:, work_dir:, work_dir_name:, task_id:, timestamp:, docker_image_name_tag:, overseer_assessment_id:) + script_contents = step.run_command raise "Execution script is empty" if script_contents.blank? + # Create script run_sh_path = File.join(work_dir, 'run.sh') File.write(run_sh_path, script_contents) - system("chmod +x #{work_dir}/run.sh") + + # Ensure script is executable + system("chmod +x #{run_sh_path}") mount = Doubtfire::Application.config.overseer_workdir_volume_mount - volume_mount = if mount.nil? - # Fallback for development only — mounts the entire overseer container volume, - # allowing all task work directories to be accessible. This should never be - # used in production, as it breaks isolation between tasks. - "--volumes-from #{Doubtfire::Application.config.overseer_fallback_volume_container}" - else - "-v #{mount}/#{work_dir_name}:/overseer/work-dir/#{work_dir_name}" - end + volume_mount = + if mount.nil? + # Fallback for development only — mounts the entire overseer container volume, + # allowing all task work directories to be accessible. This should never be + # used in production, as it breaks isolation between tasks. + "--volumes-from #{Doubtfire::Application.config.overseer_fallback_volume_container}" + else + # Absolute path on the hosting server to the shared mount + "-v #{mount}/#{work_dir_name}:/overseer/work-dir/#{work_dir_name}" + end + + # Max runtime (seconds) before force-killing the step (exit status 124) + timeout = step.timeout + timeout = 30 if timeout.nil? || timeout.negative? container_name = "overseer-#{task_id}-#{timestamp}" command = %( - timeout 300 docker run --rm \ + timeout #{timeout} docker run --rm -i \ --cpus 1 \ --network none \ #{volume_mount} \ --name #{container_name} \ #{docker_image_name_tag} \ - bash -c "cd /overseer/work-dir/#{work_dir_name} && ./run.sh" + bash -c "cd /overseer/work-dir/#{work_dir_name} && timeout #{timeout} ./run.sh" ) - system(command) + stdin_input_file = nil + expected_output_file = nil - yaml_path = File.join(work_dir, 'output.yaml') + # Retrieve names of input/output files + if step.step_type == 'output_diff' + stdin_input_file = step.stdin_input_file.present? ? File.join(work_dir, step.stdin_input_file) : nil + expected_output_file = step.expected_output_file.present? ? File.join(work_dir, step.expected_output_file) : nil + end - oa = OverseerAssessment.find(overseer_assessment_id) + output = "" + status = nil + stdin_contents = nil + + # Execute script and capture output + Open3.popen2e(command) do |stdin, stdout_err, wait_thr| + # If input file exists, pass it as standard input + if stdin_input_file && File.exist?(stdin_input_file) + File.open(stdin_input_file, 'rb') { |f| IO.copy_stream(f, stdin) } + stdin_contents = File.read(stdin_input_file) + stdin.close + end - oa.update_from_output(work_dir) - if File.exist?(yaml_path) - path = FileHelper.task_submission_identifier_path_with_timestamp(:done, task, timestamp) - FileUtils.cp(yaml_path, path) - FileUtils.rm_rf(work_dir) + stdout_err.each { |line| output << line } + status = wait_thr.value end - logger.info "Completed overseer job" - rescue StandardError => e - logger.error e - raise e + output = output.chomp + pass = status.exitstatus == 0 + + expected_output_contents = nil + + # If step type is comparing output, retrieve expected output file contents + if step.step_type == 'output_diff' + expected_output_contents = + if expected_output_file && File.exist?(expected_output_file) + File.read(expected_output_file) + else + '' + end + matches_output = if step.partial_output_diff + output.include?(expected_output_contents) + else + output == expected_output_contents + end + + pass = false unless matches_output + end + + feedback_message = + if step.feedback_message.blank? + if step.step_type == 'output_diff' + "Your output did not match the expected result." + else + "This test did not complete successfully. Check the output for any errors." + end + else + step.feedback_message + end + + OverseerStepResult.create!( + overseer_assessment_id: overseer_assessment_id, + overseer_step: step, + exit_status: status.exitstatus, + pass: pass, + feedback_message: feedback_message, + stdout: output, + stdin: stdin_contents, + expected_output: expected_output_contents, + stdout_sha256: Digest::SHA256.hexdigest(output), + stdin_sha256: stdin_contents && Digest::SHA256.hexdigest(stdin_contents), + expected_output_sha256: expected_output_contents && Digest::SHA256.hexdigest(expected_output_contents) + ) + end + + def extract_student_submission_files(task, submission, work_dir) + # Extract submission files, removing any parent folders + Zip::File.open(submission) do |zip_file| + zip_file.each do |entry| + next if entry.name_is_directory? + + parts = entry.name.split('/')[1..] + next unless parts.length >= 1 + + file_name = parts.first + index = file_name.to_i + + file = task.upload_requirements[index] + final_name = file['name'] + + dest_path = File.join(work_dir, final_name) + FileUtils.mkdir_p(File.dirname(dest_path)) + zip_file.extract(entry, dest_path) { true } + end + end + end + + def extract_overseer_resource_files(assessment, work_dir) + # Extract optional assessment resources + if File.exist?(assessment) + Zip::File.open(assessment) do |zip_file| + zip_file.each do |entry| + dest_path = File.join(work_dir, entry.name) + FileUtils.mkdir_p(File.dirname(dest_path)) + zip_file.extract(entry, dest_path) { true } # overwrite if exists + end + end + end end end diff --git a/app/sidekiq/accept_submission_job.rb b/app/sidekiq/accept_submission_job.rb index 90c20026d..a8a44bf7f 100644 --- a/app/sidekiq/accept_submission_job.rb +++ b/app/sidekiq/accept_submission_job.rb @@ -2,7 +2,7 @@ class AcceptSubmissionJob include Sidekiq::Job include LogHelper - def perform(task_id, user_id, accepted_tii_eula) + def perform(task_id, user_id, accepted_tii_eula, test_submission) begin # Ensure cwd is valid... FileUtils.cd(Rails.root) @@ -50,12 +50,12 @@ def perform(task_id, user_id, accepted_tii_eula) task.send_documents_to_tii(user, accepted_tii_eula: accepted_tii_eula) end - if task.overseer_enabled? - overseer_assessment = OverseerAssessment.create_for(task) + if task.overseer_enabled? || test_submission + overseer_assessment = OverseerAssessment.create_for(task, test_submission) if overseer_assessment.present? logger.info "Launching Overseer assessment for task_def_id: #{task.task_definition.id} task_id: #{task.id}" - overseer_assessment.send_to_overseer + overseer_assessment.send_to_overseer(test_submission: test_submission) else logger.info "Overseer assessment for task_def_id: #{task.task_definition.id} task_id: #{task.id} was not performed #{overseer_assessment.inspect}" diff --git a/db/migrate/20251218031455_create_overseer_steps.rb b/db/migrate/20251218031455_create_overseer_steps.rb new file mode 100644 index 000000000..eab2bafe4 --- /dev/null +++ b/db/migrate/20251218031455_create_overseer_steps.rb @@ -0,0 +1,70 @@ +class CreateOverseerSteps < ActiveRecord::Migration[8.0] + def change + create_table :overseer_steps do |t| + t.references :task_definition, null: false + + # Staff only + t.string :name, null: false + t.text :description + + # Shown to the student + t.string :display_name, null: false + t.string :display_description + + t.text :run_command + + t.integer :timeout, default: 30, null: false + t.integer :sort_order, default: 0, null: false + + t.string :step_type, null: false # "status_check", "output_diff", etc. + t.boolean :partial_output_diff + + t.string :stdin_input_file # Name of file (or path) in assessment resources + t.string :expected_output_file # Name of file in (or path) assessment resources + + t.text :feedback_message + + t.references :status_on_success + t.references :status_on_failure + + t.boolean :halt_on_success + t.boolean :halt_on_failure + + t.boolean :show_expected_output + t.boolean :show_stdin + t.boolean :show_stdout + + t.boolean :enabled, default: true + + t.timestamps + end + + create_table :overseer_step_results do |t| + t.references :overseer_assessment, null: false + t.references :overseer_step, null: false + + t.integer :exit_status, null: false, default: -1 + t.boolean :pass, null: false, default: false + + t.text :feedback_message + + # The output from the overseer script and student's submission + t.text :stdout + + # The original input/output files, in case they have since been changed + t.text :stdin + t.text :expected_output + + # We may want to discard the original_stdin, expected_output, and stdout when archiving a unit. + # Storing hashes will allow us to confirm if the original outputs matched + t.string :stdout_sha256 + t.string :stdin_sha256 + t.string :expected_output_sha256 + + t.timestamps + end + + # Track the number of available steps at the time of assessment + add_column :overseer_assessments, :total_steps, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index d2a328042..689f92ad0 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.0].define(version: 2025_12_12_010033) do +ActiveRecord::Schema[8.0].define(version: 2025_12_18_031455) do create_table "activity_types", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false @@ -215,6 +215,7 @@ t.integer "status", default: 0, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "total_steps" t.index ["task_id", "submission_timestamp"], name: "index_overseer_assessments_on_task_id_and_submission_timestamp", unique: true t.index ["task_id"], name: "index_overseer_assessments_on_task_id" end @@ -231,6 +232,53 @@ t.index ["tag"], name: "index_overseer_images_on_tag", unique: true end + create_table "overseer_step_results", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "overseer_assessment_id", null: false + t.bigint "overseer_step_id", null: false + t.integer "exit_status", default: -1, null: false + t.boolean "pass", default: false, null: false + t.text "feedback_message" + t.text "stdout" + t.text "stdin" + t.text "expected_output" + t.string "stdout_sha256" + t.string "stdin_sha256" + t.string "expected_output_sha256" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["overseer_assessment_id"], name: "index_overseer_step_results_on_overseer_assessment_id" + t.index ["overseer_step_id"], name: "index_overseer_step_results_on_overseer_step_id" + end + + create_table "overseer_steps", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "task_definition_id", null: false + t.string "name", null: false + t.text "description" + t.string "display_name", null: false + t.string "display_description" + t.text "run_command" + t.integer "timeout", default: 30, null: false + t.integer "sort_order", default: 0, null: false + t.string "step_type", null: false + t.boolean "partial_output_diff" + t.string "stdin_input_file" + t.string "expected_output_file" + t.text "feedback_message" + t.bigint "status_on_success_id" + t.bigint "status_on_failure_id" + t.boolean "halt_on_success" + t.boolean "halt_on_failure" + t.boolean "show_expected_output" + t.boolean "show_stdin" + t.boolean "show_stdout" + t.boolean "enabled", default: true + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["status_on_failure_id"], name: "index_overseer_steps_on_status_on_failure_id" + t.index ["status_on_success_id"], name: "index_overseer_steps_on_status_on_success_id" + t.index ["task_definition_id"], name: "index_overseer_steps_on_task_definition_id" + end + create_table "projects", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "unit_id" t.string "project_role" diff --git a/test/factories/overseer_steps.rb b/test/factories/overseer_steps.rb new file mode 100644 index 000000000..284b2e6b1 --- /dev/null +++ b/test/factories/overseer_steps.rb @@ -0,0 +1,5 @@ +FactoryBot.define do + factory :overseer_step do + + end +end diff --git a/test/models/overseer_step_test.rb b/test/models/overseer_step_test.rb new file mode 100644 index 000000000..6cce7d6fa --- /dev/null +++ b/test/models/overseer_step_test.rb @@ -0,0 +1,7 @@ +require "test_helper" + +describe OverseerStep do + # it "does a thing" do + # value(1+1).must_equal 2 + # end +end