changes to export course tar file#2319
Open
melissajinn wants to merge 10 commits intoec2-testingfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces EC2/AWS autograder support featuring encrypted credential storage, instance type selection, and conditional job payload augmentation. Changes span frontend UI, controller logic, model validations, database schema, configuration flags, and comprehensive test coverage for the new authentication and provisioning workflow. Changes
Sequence DiagramsequenceDiagram
actor User
participant Form as Autograder Form
participant Controller as AutogradersController
participant Model as Autograder Model
participant Helper as AssessmentAutogradeCore
participant TangoClient
User->>Form: Submit EC2 settings (use_access_key, instanceType, credentials)
Form->>Controller: POST /autograders/:id (use_access_key, access_key, access_key_id, instance_type)
Controller->>Controller: Transform params based on use_access_key flag
Controller->>Model: update(transformed_params)
Model->>Model: Validate instance_type, access_key format (if use_access_key enabled)
Model-->>Model: Encrypt access_key & access_key_id
Model-->>Controller: Autograder saved
Controller-->>User: Success response
rect rgb(200, 150, 100, 0.5)
Note over Helper,TangoClient: When EC2 SSH is enabled and job is submitted
end
User->>Helper: Submit job for grading
Helper->>Helper: Build job_properties hash
alt ec2_ssh enabled
Helper->>Model: Read instance_type, use_access_key, access_key, access_key_id
Model-->>Helper: EC2 configuration
Helper->>Helper: Augment payload with ec2Vmms, instanceType, accessKey, accessKeyId
end
Helper->>Helper: JSON encode job_properties
Helper->>TangoClient: addjob(job_properties)
TangoClient-->>Helper: Job queued with EC2 provisioning
Helper-->>User: Job submitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
app/controllers/scoreboards_controller.rb (1)
310-310: Use a descriptive error message when raising.
raise StandardErrorwithout a message results in an empty or unhelpfule.messagewhen caught at line 340. Consider raising with a meaningful message for better logging.♻️ Proposed fix
- raise StandardError + raise StandardError, "Scoreboard result is not an array"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/scoreboards_controller.rb` at line 310, Replace the bare raise StandardError with a descriptive message so the rescue that logs e.message (the handler near the rescue block) gets useful context; specifically update the raise in scoreboards_controller (the raise StandardError occurrence) to raise StandardError, "Descriptive message about what failed (e.g. 'Failed to update scoreboard: missing params or invalid state')" or raise a more specific custom error class if available, ensuring the message explains the failure and includes relevant variables (like scoreboard id or params) for better logging.app/models/course.rb (1)
380-389: Consider explicit attribute selection for consistency.The users export (lines 369-372) explicitly selects attributes, but here
cud.attributes.to_yamlexports all attributes including timestamps and foreign keys. While this works for a full backup, consider using explicit attribute selection for consistency and to avoid exporting potentially unnecessary internal fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/course.rb` around lines 380 - 389, The export of course user data currently uses cud.attributes.to_yaml which dumps all fields (including timestamps and foreign keys); change the export to explicitly select the same set of attributes used by the users export to ensure consistency — locate the loop over course_user_data in Course (the course_user_data.each block) and replace cud.attributes.to_yaml with a YAML serialization of a filtered hash (e.g., build a hash via cud.slice(...) or manually select attribute names) containing only the intended fields (IDs, user-facing fields, and any specific metadata) before writing to the tar file.spec/models/autograder_spec.rb (1)
14-18: Consider using a deterministic test value.Using
.samplemakes the test non-deterministic. While it will pass, a specific value would make failures easier to reproduce.Suggested change
it "allows listed instance types" do - autograder.instance_type = Autograder::INSTANCE_TYPES.sample + autograder.instance_type = "t3.micro" expect(autograder).to be_valid endAlternatively, if you want to test all types, consider using
Autograder::INSTANCE_TYPES.eachwith a shared example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/autograder_spec.rb` around lines 14 - 18, The test uses Autograder::INSTANCE_TYPES.sample which makes it non-deterministic; change the spec in spec/models/autograder_spec.rb to set autograder.instance_type to a deterministic value (e.g., Autograder::INSTANCE_TYPES.first or a specific known instance-type string) so the example "allows listed instance types" is repeatable, or alternatively expand the spec to iterate Autograder::INSTANCE_TYPES.each and use a shared example to assert validity for every type.app/views/autograders/_form.html.erb (1)
5-7: Consider simplifying boolean check.Using
.presenceon a boolean (ec2_ssh) is unconventional. While it works (returnsnilforfalse,truefortrue), a direct boolean check is clearer.Suggested simplification
- <% if Rails.configuration.x.ec2_ssh.presence %> + <% if Rails.configuration.x.ec2_ssh %>Same applies to line 27.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/autograders/_form.html.erb` around lines 5 - 7, The conditional uses Rails.configuration.x.ec2_ssh.presence which is unconventional for a boolean; change the checks that reference Rails.configuration.x.ec2_ssh (both the occurrence that calls list.append("ec2") and the similar occurrence around line 27) to a direct boolean conditional using Rails.configuration.x.ec2_ssh so the intent is clear and idiomatic.app/views/autograders/_ec2_settings.html.erb (1)
23-61: Keep the instance catalog in one place.This list duplicates
Autograder::INSTANCE_TYPESinapp/models/autograder.rb, while the defaultt3.microis also hard-coded inapp/controllers/autograders_controller.rbandapp/helpers/assessment_autograde_core.rb. The next instance-type change now has multiple places to keep in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/autograders/_ec2_settings.html.erb` around lines 23 - 61, The view duplicates instance-type data that already belongs in the model; replace the hard-coded ec2_instance_options in _ec2_settings.html.erb with a reference to Autograder::INSTANCE_TYPES (or move the grouped catalog into Autograder::INSTANCE_TYPES if it isn’t already), and remove/hard-coded defaults elsewhere by centralizing the default instance (e.g., add Autograder::DEFAULT_INSTANCE or derive it from Autograder::INSTANCE_TYPES) then update autograders_controller (where t3.micro is set) and assessment_autograde_core to use that centralized constant; ensure all code references the model constant (Autograder::INSTANCE_TYPES / Autograder::DEFAULT_INSTANCE) so the catalog and default live in one place.db/schema.rb (2)
62-63: Timestamp type has 2038 year limit.
TIMESTAMPcolumns in MySQL are limited to dates up to '2038-01-19 03:14:07' UTC. Forstart_date/end_dateon announcements, this is likely acceptable for typical use cases, but worth noting. The benefit is automatic UTC timezone normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 62 - 63, The TIMESTAMP columns start_date and end_date in db/schema.rb can hit the 2038 limit; change their type to datetime (or Rails' t.datetime) to support a wider range and avoid overflow, and add a short migration to alter the announcements table columns from timestamp to datetime (preserve nullability/defaults and set precision if needed) so existing data and UTC handling remain correct; update any model/schema references to use start_date and end_date as datetimes.
445-457: Foreign key type mismatch with referenced tables.
watchlist_instancesusesbigintforcourse_user_datum_idandcourse_id, but the referenced tables (course_user_data,courses) haveintegerprimary keys. MySQL handles this implicitly, but for consistency, consider aligning types. This also applies towatchlist_configurations.course_id(line 438).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 445 - 457, The watchlist tables have column type mismatches: change watchlist_instances.course_user_datum_id and watchlist_instances.course_id (and watchlist_configurations.course_id) from bigint to integer to match the referenced primary keys on course_user_data and courses; implement this by adding a migration that alters those columns to :integer (or recreate them with correct type), update any associated indexes and foreign key constraints to use the new type, and run schema dump so schema.rb reflects the integer type for course_user_datum_id, course_id and the watchlist_configurations.course_id columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/jobs_controller.rb`:
- Around line 250-255: The else branch in the JobsController terminal-state
logic currently sets job[:state] = "Unknown State" which surfaces ambiguous
values; change that fallback to a stable terminal state such as "Completed" (or
another agreed explicit terminal label) so any dead job that doesn't match the
two string checks shows a safe terminal state; update the else branch where
rjob["trace"][-1] is inspected (the block that sets job[:state] for "Autodriver
returned normally" and "Error") to assign "Completed" instead of "Unknown
State".
In `@app/models/assessment.rb`:
- Around line 97-99: The change to asmt_yaml_path returning a fixed
"assignment_config.yml" breaks compatibility with existing <name>.yml files;
update load_yaml to first try File.open(asmt_yaml_path) and if that file does
not exist fallback to File.open("#{name}.yml") (where name is the assessment
name used previously), and likewise update dump_yaml to write both the new
assignment_config.yml and (optionally) update/rename the old "#{name}.yml" to
avoid orphaning; also adjust the tar import/filtering logic that uses
asmt_yaml_path (in the import path) to accept either "assignment_config.yml" or
"#{name}.yml" during extraction so older tarballs still import.
In `@app/models/course.rb`:
- Around line 406-417: The current loop loads submissions per assessment causing
N+1 queries and duplicates iterating assessments; fix by loading submissions in
bulk (either eager load via assessments.includes(:submissions) before the export
block or, better, use the existing has_many :submissions, through: :assessments
and iterate course.submissions.find_each) and write each submission once to
File.join(course_dir, "submissions", "submission_#{submission.id}.yml") using
tar.add_file; ensure you remove the per-assessment submissions.each loop and the
duplicate assessments iteration so submissions are streamed efficiently with
find_each to reduce memory and DB queries.
In `@app/views/autograders/_basic_settings.html.erb`:
- Around line 30-35: The download link helpers in the view use
download_file_course_assessment_autograder_path incorrectly: they must be called
with the nested route positional objects (`@course`, `@assessment`) and should not
expose server file system paths via file_path; update the calls that reference
`@makefile_exists` and `@tar_exists` to pass `@course` and `@assessment` as the first
two positional arguments and only pass file_key (e.g., 'makefile' or 'tar') as a
named param expected by AutogradersController#download_file, removing file_path
from the public URL generation so the controller continues to reconstruct the
real path from file_key.
In `@app/views/autograders/_ec2_settings.html.erb`:
- Around line 64-76: The template raises ArgumentError when
`@autograder.instance_type` is stale; remove that raise and instead coerce the
selection to a safe fallback: compute selected_instance_type from
`@autograder.instance_type` but if it is not included in valid_instance_types
(derived from ec2_instance_options) set selected_instance_type to nil or a
default valid value, then pass that coerced selected_instance_type into
grouped_options_for_select (keep references to selected_instance_type,
valid_instance_types, ec2_instance_options and f.select) so the view renders
without throwing and allows instructors to correct the record.
In `@db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb`:
- Around line 3-8: The migration currently removes plaintext columns access_key
and access_key_id before the encrypting migration runs, which can delete the
only existing secrets; modify this migration
(add_ec2_ssh_fields_to_autograders.rb) to NOT remove the plaintext columns
(remove or comment out the remove_column lines for access_key and access_key_id)
so that the encryption migration
(db/migrate/20251021034813_encrypt_autograder_credentials.rb) can copy values
into access_key_ciphertext and access_key_id_ciphertext first; keep adding the
new columns instance_type, access_key_ciphertext, and access_key_id_ciphertext
as-is and only drop the plaintext columns after the
encrypt_autograder_credentials migration has successfully migrated data.
In `@db/migrate/20251021034813_encrypt_autograder_credentials.rb`:
- Around line 4-5: The migration unconditionally calls add_column for
access_key_ciphertext and access_key_id_ciphertext which will conflict with the
earlier migration
db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb; guard the column
additions by checking for their existence (use column_exists?(:autograders,
:access_key_ciphertext) and column_exists?(:autograders,
:access_key_id_ciphertext)) and only call add_column when the column is missing
so the migration becomes safe to run in either order.
---
Nitpick comments:
In `@app/controllers/scoreboards_controller.rb`:
- Line 310: Replace the bare raise StandardError with a descriptive message so
the rescue that logs e.message (the handler near the rescue block) gets useful
context; specifically update the raise in scoreboards_controller (the raise
StandardError occurrence) to raise StandardError, "Descriptive message about
what failed (e.g. 'Failed to update scoreboard: missing params or invalid
state')" or raise a more specific custom error class if available, ensuring the
message explains the failure and includes relevant variables (like scoreboard id
or params) for better logging.
In `@app/models/course.rb`:
- Around line 380-389: The export of course user data currently uses
cud.attributes.to_yaml which dumps all fields (including timestamps and foreign
keys); change the export to explicitly select the same set of attributes used by
the users export to ensure consistency — locate the loop over course_user_data
in Course (the course_user_data.each block) and replace cud.attributes.to_yaml
with a YAML serialization of a filtered hash (e.g., build a hash via
cud.slice(...) or manually select attribute names) containing only the intended
fields (IDs, user-facing fields, and any specific metadata) before writing to
the tar file.
In `@app/views/autograders/_ec2_settings.html.erb`:
- Around line 23-61: The view duplicates instance-type data that already belongs
in the model; replace the hard-coded ec2_instance_options in
_ec2_settings.html.erb with a reference to Autograder::INSTANCE_TYPES (or move
the grouped catalog into Autograder::INSTANCE_TYPES if it isn’t already), and
remove/hard-coded defaults elsewhere by centralizing the default instance (e.g.,
add Autograder::DEFAULT_INSTANCE or derive it from Autograder::INSTANCE_TYPES)
then update autograders_controller (where t3.micro is set) and
assessment_autograde_core to use that centralized constant; ensure all code
references the model constant (Autograder::INSTANCE_TYPES /
Autograder::DEFAULT_INSTANCE) so the catalog and default live in one place.
In `@app/views/autograders/_form.html.erb`:
- Around line 5-7: The conditional uses Rails.configuration.x.ec2_ssh.presence
which is unconventional for a boolean; change the checks that reference
Rails.configuration.x.ec2_ssh (both the occurrence that calls list.append("ec2")
and the similar occurrence around line 27) to a direct boolean conditional using
Rails.configuration.x.ec2_ssh so the intent is clear and idiomatic.
In `@db/schema.rb`:
- Around line 62-63: The TIMESTAMP columns start_date and end_date in
db/schema.rb can hit the 2038 limit; change their type to datetime (or Rails'
t.datetime) to support a wider range and avoid overflow, and add a short
migration to alter the announcements table columns from timestamp to datetime
(preserve nullability/defaults and set precision if needed) so existing data and
UTC handling remain correct; update any model/schema references to use
start_date and end_date as datetimes.
- Around line 445-457: The watchlist tables have column type mismatches: change
watchlist_instances.course_user_datum_id and watchlist_instances.course_id (and
watchlist_configurations.course_id) from bigint to integer to match the
referenced primary keys on course_user_data and courses; implement this by
adding a migration that alters those columns to :integer (or recreate them with
correct type), update any associated indexes and foreign key constraints to use
the new type, and run schema dump so schema.rb reflects the integer type for
course_user_datum_id, course_id and the watchlist_configurations.course_id
columns.
In `@spec/models/autograder_spec.rb`:
- Around line 14-18: The test uses Autograder::INSTANCE_TYPES.sample which makes
it non-deterministic; change the spec in spec/models/autograder_spec.rb to set
autograder.instance_type to a deterministic value (e.g.,
Autograder::INSTANCE_TYPES.first or a specific known instance-type string) so
the example "allows listed instance types" is repeatable, or alternatively
expand the spec to iterate Autograder::INSTANCE_TYPES.each and use a shared
example to assert validity for every type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5d36992-15b1-42d3-8596-d46681b2942a
📒 Files selected for processing (22)
app/assets/javascripts/autograder.jsapp/controllers/autograders_controller.rbapp/controllers/jobs_controller.rbapp/controllers/scoreboards_controller.rbapp/helpers/assessment_autograde_core.rbapp/models/assessment.rbapp/models/autograder.rbapp/models/course.rbapp/views/autograders/_basic_settings.html.erbapp/views/autograders/_ec2_settings.html.erbapp/views/autograders/_form.html.erbapp/views/scoreboards/show.html.erbconfig/environments/development.rbconfig/environments/production.rb.templateconfig/initializers/filter_parameter_logging.rbdb/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rbdb/migrate/20241211042124_add_use_access_key_to_autograder.rbdb/migrate/20251021034813_encrypt_autograder_credentials.rbdb/schema.rbspec/factories/autograders.rbspec/helpers/assessment_autograde_core_spec.rbspec/models/autograder_spec.rb
Comment on lines
+250
to
+255
| elsif rjob["trace"][-1].include?("Autodriver returned normally") | ||
| job[:state] = "Succeeded" | ||
| elsif rjob["trace"][-1].split("|")[1].include? "Error" | ||
| job[:state] = "Failed" | ||
| else | ||
| job[:state] = "Completed" | ||
| job[:state] = "Failed" if rjob["trace"][-1].split("|")[1].include? "Error" | ||
| job[:state] = "Unknown State" |
Contributor
There was a problem hiding this comment.
Use a stable fallback state instead of "Unknown State" for terminal jobs.
Line 255 now emits "Unknown State" for any dead job that misses the two string checks, and that value is rendered directly in list/detail views. This can surface normal terminal jobs as ambiguous to users. Prefer a safe terminal fallback (e.g., "Completed") or an explicit exhaustive mapping.
Proposed minimal fix
- elsif rjob["trace"][-1].include?("Autodriver returned normally")
- job[:state] = "Succeeded"
- elsif rjob["trace"][-1].split("|")[1].include? "Error"
+ elsif rjob["trace"][-1].include?("Autodriver returned normally")
+ job[:state] = "Succeeded"
+ elsif rjob["trace"][-1].split("|")[1].include?("Error")
job[:state] = "Failed"
else
- job[:state] = "Unknown State"
+ job[:state] = "Completed"
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| elsif rjob["trace"][-1].include?("Autodriver returned normally") | |
| job[:state] = "Succeeded" | |
| elsif rjob["trace"][-1].split("|")[1].include? "Error" | |
| job[:state] = "Failed" | |
| else | |
| job[:state] = "Completed" | |
| job[:state] = "Failed" if rjob["trace"][-1].split("|")[1].include? "Error" | |
| job[:state] = "Unknown State" | |
| elsif rjob["trace"][-1].include?("Autodriver returned normally") | |
| job[:state] = "Succeeded" | |
| elsif rjob["trace"][-1].split("|")[1].include?("Error") | |
| job[:state] = "Failed" | |
| else | |
| job[:state] = "Completed" | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/jobs_controller.rb` around lines 250 - 255, The else branch
in the JobsController terminal-state logic currently sets job[:state] = "Unknown
State" which surfaces ambiguous values; change that fallback to a stable
terminal state such as "Completed" (or another agreed explicit terminal label)
so any dead job that doesn't match the two string checks shows a safe terminal
state; update the else branch where rjob["trace"][-1] is inspected (the block
that sets job[:state] for "Autodriver returned normally" and "Error") to assign
"Completed" instead of "Unknown State".
Comment on lines
97
to
99
| def asmt_yaml_path | ||
| path "#{name}.yml" | ||
| path "assignment_config.yml" | ||
| end |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find assessment YAML files in course directories
echo "=== Searching for assessment YAML files ==="
fd -t f '\.yml$' courses/ 2>/dev/null | head -30
echo ""
echo "=== Checking for assignment_config.yml files ==="
fd -t f 'assignment_config.yml' courses/ 2>/dev/null | head -20
echo ""
echo "=== Checking tar import logic for yaml path comparison ==="
rg -n 'asmt_yaml_path' --type ruby -C 3Repository: autolab/Autolab
Length of output: 2059
🏁 Script executed:
#!/bin/bash
# Check the git history to see what the previous implementation was
echo "=== Git diff for asmt_yaml_path method ==="
git show HEAD:app/models/assessment.rb 2>/dev/null | sed -n '90,110p' || echo "Git not available, checking current implementation context"
echo ""
echo "=== Checking backwards_compatibility method ==="
rg -n 'backwards_compatibility' app/models/assessment.rb -A 10
echo ""
echo "=== Checking if there's any fallback logic in load_yaml ==="
rg -n 'def load_yaml' app/models/assessment.rb -A 15
echo ""
echo "=== Search for any reference to old filename patterns ==="
rg '\.yml' app/models/assessment.rb -nRepository: autolab/Autolab
Length of output: 2036
🏁 Script executed:
#!/bin/bash
# Check git log for asmt_yaml_path changes
echo "=== Recent commits touching asmt_yaml_path ==="
git log --oneline -n 30 -- app/models/assessment.rb 2>/dev/null | head -15
echo ""
echo "=== Diff of recent changes to assessment.rb ==="
git diff HEAD~5..HEAD -- app/models/assessment.rb 2>/dev/null | grep -A 10 -B 5 'asmt_yaml_path' || echo "Unable to get diff"
echo ""
echo "=== Check for any migration files or upgrade logic ==="
fd -t f 'migrate' --extension rb | xargs rg -l 'asmt_yaml_path|assignment_config' 2>/dev/null
echo ""
echo "=== Check for file reading with name-derived path ==="
rg '#{.*name.*}.*yml' app/models/assessment.rbRepository: autolab/Autolab
Length of output: 496
🏁 Script executed:
#!/bin/bash
# Check the specific commit
echo "=== Commit details for export tar updates ==="
git show b3f1bb9 --stat 2>/dev/null
echo ""
echo "=== Full diff of asmt_yaml_path change ==="
git show b3f1bb9 -- app/models/assessment.rb 2>/dev/null | grep -A 20 -B 20 'asmt_yaml_path' | head -50
echo ""
echo "=== Check for name-based yml references ==="
rg 'name.*yml|yml.*name' app/models/assessment.rb -n
echo ""
echo "=== Look for any file existence checks or fallbacks ==="
rg -n 'File\.exist|FileUtils|File\.open' app/models/assessment.rb | grep -i yml -A 2 -B 2Repository: autolab/Autolab
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# Check if load_yaml has any error handling for missing files
echo "=== Full load_yaml implementation ==="
rg -n 'def load_yaml' app/models/assessment.rb -A 20
echo ""
echo "=== Check for File.exist? checks near asmt_yaml_path ==="
rg -n 'File\.exist|asmt_yaml_path' app/models/assessment.rb -B 2 -A 2
echo ""
echo "=== Look for rescue or error handling in dump/load yaml methods ==="
rg -n 'def (dump_yaml|load_yaml)' app/models/assessment.rb -A 25 | head -60
echo ""
echo "=== Check unique_source_config_file_path referenced in tar extraction ==="
rg -n 'unique_source_config_file_path' app/models/assessment.rb -A 5 -B 5Repository: autolab/Autolab
Length of output: 5305
🏁 Script executed:
#!/bin/bash
# Check how load_yaml is called - is it used in tar import?
echo "=== Where is load_yaml called? ==="
rg -n 'load_yaml' app/ -t ruby --type-add 'ruby:*.rb'
echo ""
echo "=== Check tar extraction and assessment creation logic ==="
rg -n 'extract.*tar|new_assessment|Assessment\.new' app/controllers/assessments_controller.rb -B 3 -A 3 | head -80
echo ""
echo "=== Check if there's any backward compatibility for yaml file location ==="
rg -n 'assignment_config\.yml|\.yml' app/models/assessment.rb | grep -v '^\s*#'Repository: autolab/Autolab
Length of output: 4185
🏁 Script executed:
#!/bin/bash
# Check what happens before load_yaml is called - any existence checks?
echo "=== Code around load_yaml call in assessments_controller ==="
sed -n '295,315p' app/controllers/assessments_controller.rb
echo ""
echo "=== Check if there's any fallback filename logic in dump_yaml or load_yaml ==="
sed -n '295,345p' app/models/assessment.rb | grep -E 'def (dump_yaml|load_yaml)|File\.|asmt_yaml_path|\.yml|exist'
echo ""
echo "=== Check if name-based yml path is used anywhere else ==="
rg "#{name}" app/models/assessment.rb | grep -i ymlRepository: autolab/Autolab
Length of output: 1655
🏁 Script executed:
#!/bin/bash
# Verify when asmt_yaml_path was introduced and what the previous implementation was
echo "=== Check git log for asmt_yaml_path history ==="
git log --oneline -p -- app/models/assessment.rb 2>/dev/null | grep -A 10 -B 10 'asmt_yaml_path' | head -50
echo ""
echo "=== Check if there are any other references to assignment_config.yml in codebase ==="
rg 'assignment_config' app/ --type ruby
echo ""
echo "=== Check tar extraction filtering logic more carefully ==="
sed -n '165,180p' app/controllers/assessments_controller.rbRepository: autolab/Autolab
Length of output: 2653
Breaking change: Fixed filename breaks backwards compatibility with existing assessments.
Changing from a name-derived path to a fixed "assignment_config.yml" creates compatibility issues:
- Existing assessments: Code comments reference the old pattern (
homework01.yml,homework02.yml). Current assessments have configs at<assessment_name>.yml. After this change,load_yamlanddump_yamlreference a different file. - Tar import/export: Old assessment tarballs contain
<name>.yml, butload_yaml(line 340) opensasmt_yaml_pathdirectly with no fallback—it will fail withFileNotFound. - Orphan files:
dump_yamlcreatesassignment_config.ymlwhile leaving the old<name>.ymlfile orphaned. - Tar extraction filtering: The tar import logic in
assessments_controller.rb(line 171) filters byasmt_yaml_path—mismatched paths during import could cause issues.
Add fallback logic in load_yaml to check for the old filename if the new one doesn't exist, or implement a migration strategy for existing assessments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/assessment.rb` around lines 97 - 99, The change to asmt_yaml_path
returning a fixed "assignment_config.yml" breaks compatibility with existing
<name>.yml files; update load_yaml to first try File.open(asmt_yaml_path) and if
that file does not exist fallback to File.open("#{name}.yml") (where name is the
assessment name used previously), and likewise update dump_yaml to write both
the new assignment_config.yml and (optionally) update/rename the old
"#{name}.yml" to avoid orphaning; also adjust the tar import/filtering logic
that uses asmt_yaml_path (in the import path) to accept either
"assignment_config.yml" or "#{name}.yml" during extraction so older tarballs
still import.
Comment on lines
+406
to
417
| # save submissions | ||
| if export_configs&.include?('submissions') | ||
| tar.mkdir File.join(course_dir, "submissions"), mode | ||
|
|
||
| assessments.each do |assessment| | ||
| asmt_dir = assessment.name | ||
| assessment.dump_yaml | ||
| filter = [assessment.handin_directory_path] | ||
| assessment.load_dir_to_tar(base_path, asmt_dir, tar, filter, course_dir) | ||
| assessment.submissions.each do |submission| | ||
| tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file| | ||
| tar_file.write(submission.attributes.to_yaml) | ||
| end | ||
| end | ||
| end | ||
| end |
Contributor
There was a problem hiding this comment.
N+1 query issue: submissions are loaded separately for each assessment.
The current implementation triggers a separate database query for each assessment's submissions. For a course with many assessments, this causes significant performance degradation.
Additionally, assessments is iterated twice (lines 395 and 410) when both export options are enabled.
🔧 Proposed fix using eager loading or the through relationship
Option 1: Use eager loading
# save submissions
if export_configs&.include?('submissions')
tar.mkdir File.join(course_dir, "submissions"), mode
- assessments.each do |assessment|
- assessment.submissions.each do |submission|
+ assessments.includes(:submissions).each do |assessment|
+ assessment.submissions.each do |submission|
tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file|
tar_file.write(submission.attributes.to_yaml)
end
end
end
endOption 2: Use the existing has_many :submissions, through: :assessments relationship
# save submissions
if export_configs&.include?('submissions')
tar.mkdir File.join(course_dir, "submissions"), mode
- assessments.each do |assessment|
- assessment.submissions.each do |submission|
- tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file|
- tar_file.write(submission.attributes.to_yaml)
- end
+ submissions.find_each do |submission|
+ tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file|
+ tar_file.write(submission.attributes.to_yaml)
end
end
endOption 2 also uses find_each for better memory efficiency with large datasets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # save submissions | |
| if export_configs&.include?('submissions') | |
| tar.mkdir File.join(course_dir, "submissions"), mode | |
| assessments.each do |assessment| | |
| asmt_dir = assessment.name | |
| assessment.dump_yaml | |
| filter = [assessment.handin_directory_path] | |
| assessment.load_dir_to_tar(base_path, asmt_dir, tar, filter, course_dir) | |
| assessment.submissions.each do |submission| | |
| tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file| | |
| tar_file.write(submission.attributes.to_yaml) | |
| end | |
| end | |
| end | |
| end | |
| # save submissions | |
| if export_configs&.include?('submissions') | |
| tar.mkdir File.join(course_dir, "submissions"), mode | |
| assessments.includes(:submissions).each do |assessment| | |
| assessment.submissions.each do |submission| | |
| tar.add_file File.join(course_dir, "submissions", "submission_#{submission.id}.yml"), mode do |tar_file| | |
| tar_file.write(submission.attributes.to_yaml) | |
| end | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/course.rb` around lines 406 - 417, The current loop loads
submissions per assessment causing N+1 queries and duplicates iterating
assessments; fix by loading submissions in bulk (either eager load via
assessments.includes(:submissions) before the export block or, better, use the
existing has_many :submissions, through: :assessments and iterate
course.submissions.find_each) and write each submission once to
File.join(course_dir, "submissions", "submission_#{submission.id}.yml") using
tar.add_file; ensure you remove the per-assessment submissions.each loop and the
duplicate assessments iteration so submissions are streamed efficiently with
find_each to reduce memory and DB queries.
Comment on lines
+30
to
+35
| <% unless @makefile_exists.nil? %> | ||
| <%= link_to "Download Makefile", download_file_course_assessment_autograder_path(file_path: @makefile_exists, file_key: 'makefile'), class: "btn" %> | ||
| <% end %> | ||
|
|
||
| <% unless @tar_exists.nil? %> | ||
| <%= link_to "Download Tar", download_file_course_assessment_autograder_path(file_path: @tar_exists, file_key: 'tar'), class: "btn" %> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd routes.rb . -x rg -n 'download_file|autograder' {}
rg -n 'download_file_course_assessment_autograder_path' app/views -C2Repository: autolab/Autolab
Length of output: 921
🏁 Script executed:
cat config/routes.rb | grep -n -B15 -A5 "resource :autograder"Repository: autolab/Autolab
Length of output: 720
🏁 Script executed:
fd -e rb app/controllers | xargs grep -l "autograders_controller" | head -1 | xargs cat | grep -n -A20 "def download_file"Repository: autolab/Autolab
Length of output: 424
🏁 Script executed:
fd . app/controllers -name "*autograders*"Repository: autolab/Autolab
Length of output: 289
🏁 Script executed:
fd . app/controllers -name "*autograders*" -x cat {} | grep -n -A20 "def download_file"Repository: autolab/Autolab
Length of output: 289
🏁 Script executed:
rg -n "def download_file" app/controllers/Repository: autolab/Autolab
Length of output: 302
🏁 Script executed:
rg -n "autograders_controller" app/controllers/ | head -1Repository: autolab/Autolab
Length of output: 41
🏁 Script executed:
cat app/controllers/autograders_controller.rb | head -80Repository: autolab/Autolab
Length of output: 3056
🏁 Script executed:
sed -n '105,150p' app/controllers/autograders_controller.rbRepository: autolab/Autolab
Length of output: 1130
🏁 Script executed:
rg -n "@course" app/views/autograders/_basic_settings.html.erbRepository: autolab/Autolab
Length of output: 151
🏁 Script executed:
rg -n "def edit" app/controllers/autograders_controller.rb -A30 | head -40Repository: autolab/Autolab
Length of output: 1447
Build the download links from the nested route with proper positional parameters.
Lines 31 and 35 call download_file_course_assessment_autograder_path without passing @course and @assessment as positional route parameters, and they append file_path even though AutogradersController#download_file ignores that param and reconstructs the path from file_key instead. This breaks proper URL generation and exposes server-side paths in the rendered HTML.
💡 Suggested fix
- <%= link_to "Download Makefile", download_file_course_assessment_autograder_path(file_path: `@makefile_exists`, file_key: 'makefile'), class: "btn" %>
+ <%= link_to "Download Makefile", download_file_course_assessment_autograder_path(`@course`, `@assessment`, file_key: "makefile"), class: "btn" %>- <%= link_to "Download Tar", download_file_course_assessment_autograder_path(file_path: `@tar_exists`, file_key: 'tar'), class: "btn" %>
+ <%= link_to "Download Tar", download_file_course_assessment_autograder_path(`@course`, `@assessment`, file_key: "tar"), class: "btn" %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/autograders/_basic_settings.html.erb` around lines 30 - 35, The
download link helpers in the view use
download_file_course_assessment_autograder_path incorrectly: they must be called
with the nested route positional objects (`@course`, `@assessment`) and should not
expose server file system paths via file_path; update the calls that reference
`@makefile_exists` and `@tar_exists` to pass `@course` and `@assessment` as the first
two positional arguments and only pass file_key (e.g., 'makefile' or 'tar') as a
named param expected by AutogradersController#download_file, removing file_path
from the public URL generation so the controller continues to reconstruct the
real path from file_key.
Comment on lines
+64
to
+76
| <% | ||
| selected_instance_type = @autograder.instance_type | ||
| valid_instance_types = ec2_instance_options.flat_map { |_group, instances| instances.map(&:last) } | ||
| if selected_instance_type.present? && !valid_instance_types.include?(selected_instance_type) | ||
| raise ArgumentError, "Invalid EC2 instance type: #{selected_instance_type}" | ||
| end | ||
| %> | ||
|
|
||
| <div style="margin-bottom: 5px;"><strong>EC2 Instance Type</strong></div> | ||
|
|
||
| <%= f.select :instance_type, | ||
| grouped_options_for_select(ec2_instance_options, @autograder.instance_type), | ||
| { include_blank: false, label: "EC2 Instance Type" }, |
Contributor
There was a problem hiding this comment.
Don’t raise from the template for stale instance_type values.
Lines 67-68 turn an invalid persisted value into a 500 on the edit page, which also blocks instructors from fixing the record. The model already rejects invalid saves; the view should fall back to a safe selection instead of crashing.
💡 Suggested fix
selected_instance_type = `@autograder.instance_type`
valid_instance_types = ec2_instance_options.flat_map { |_group, instances| instances.map(&:last) }
- if selected_instance_type.present? && !valid_instance_types.include?(selected_instance_type)
- raise ArgumentError, "Invalid EC2 instance type: #{selected_instance_type}"
- end
+ selected_instance_type = "t3.micro" unless valid_instance_types.include?(selected_instance_type)
%>
@@
<%= f.select :instance_type,
- grouped_options_for_select(ec2_instance_options, `@autograder.instance_type`),
+ grouped_options_for_select(ec2_instance_options, selected_instance_type),
{ include_blank: false, label: "EC2 Instance Type" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/autograders/_ec2_settings.html.erb` around lines 64 - 76, The
template raises ArgumentError when `@autograder.instance_type` is stale; remove
that raise and instead coerce the selection to a safe fallback: compute
selected_instance_type from `@autograder.instance_type` but if it is not included
in valid_instance_types (derived from ec2_instance_options) set
selected_instance_type to nil or a default valid value, then pass that coerced
selected_instance_type into grouped_options_for_select (keep references to
selected_instance_type, valid_instance_types, ec2_instance_options and f.select)
so the view renders without throwing and allows instructors to correct the
record.
Comment on lines
+3
to
+8
| add_column :autograders, :instance_type, :string, default: "" unless column_exists?(:autograders, :instance_type) | ||
| add_column :autograders, :access_key_ciphertext, :text unless column_exists?(:autograders, :access_key_ciphertext) | ||
| add_column :autograders, :access_key_id_ciphertext, :text unless column_exists?(:autograders, :access_key_id_ciphertext) | ||
|
|
||
| remove_column :autograders, :access_key if column_exists?(:autograders, :access_key) | ||
| remove_column :autograders, :access_key_id if column_exists?(:autograders, :access_key_id) |
Contributor
There was a problem hiding this comment.
Don’t drop the plaintext credential columns before the copy happens.
Lines 7-8 remove access_key and access_key_id, but db/migrate/20251021034813_encrypt_autograder_credentials.rb is the migration that actually copies those values into encrypted storage. On an upgrade with existing secrets, this migration deletes the only source data before it is migrated.
💡 Minimal fix
def up
add_column :autograders, :instance_type, :string, default: "" unless column_exists?(:autograders, :instance_type)
add_column :autograders, :access_key_ciphertext, :text unless column_exists?(:autograders, :access_key_ciphertext)
add_column :autograders, :access_key_id_ciphertext, :text unless column_exists?(:autograders, :access_key_id_ciphertext)
-
- remove_column :autograders, :access_key if column_exists?(:autograders, :access_key)
- remove_column :autograders, :access_key_id if column_exists?(:autograders, :access_key_id)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| add_column :autograders, :instance_type, :string, default: "" unless column_exists?(:autograders, :instance_type) | |
| add_column :autograders, :access_key_ciphertext, :text unless column_exists?(:autograders, :access_key_ciphertext) | |
| add_column :autograders, :access_key_id_ciphertext, :text unless column_exists?(:autograders, :access_key_id_ciphertext) | |
| remove_column :autograders, :access_key if column_exists?(:autograders, :access_key) | |
| remove_column :autograders, :access_key_id if column_exists?(:autograders, :access_key_id) | |
| add_column :autograders, :instance_type, :string, default: "" unless column_exists?(:autograders, :instance_type) | |
| add_column :autograders, :access_key_ciphertext, :text unless column_exists?(:autograders, :access_key_ciphertext) | |
| add_column :autograders, :access_key_id_ciphertext, :text unless column_exists?(:autograders, :access_key_id_ciphertext) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb` around lines
3 - 8, The migration currently removes plaintext columns access_key and
access_key_id before the encrypting migration runs, which can delete the only
existing secrets; modify this migration (add_ec2_ssh_fields_to_autograders.rb)
to NOT remove the plaintext columns (remove or comment out the remove_column
lines for access_key and access_key_id) so that the encryption migration
(db/migrate/20251021034813_encrypt_autograder_credentials.rb) can copy values
into access_key_ciphertext and access_key_id_ciphertext first; keep adding the
new columns instance_type, access_key_ciphertext, and access_key_id_ciphertext
as-is and only drop the plaintext columns after the
encrypt_autograder_credentials migration has successfully migrated data.
coder6583
requested review from
a team and
mmilunicmobile
and removed request for
a team
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…o exportCourse
…o exportCourse
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
yaml is dump from table with assessments, users, cuds, and submissions, and new tar format in general