Updated generate_tar to reflect changes in export course tar file#2318
Closed
melissajinn wants to merge 29 commits intomasterfrom
Closed
Updated generate_tar to reflect changes in export course tar file#2318melissajinn wants to merge 29 commits intomasterfrom
melissajinn wants to merge 29 commits intomasterfrom
Conversation
…and disable access key fields if not enabled
This reverts commit fc35ace.
Add g4dn.xlarge, g4dn.2xlarge, and g5.xlarge instance type options
merged if-else branches
Contributor
📝 WalkthroughWalkthroughThis PR introduces EC2 SSH support for autograders, enabling AWS credential management with encrypted storage and instance type selection. It adds conditional UI tabs, updates model validations, implements job state derivation logic, and removes the ScoresController while enhancing scoreboard nil-handling. Database schema updates include new encrypted credential columns and a feature flag system. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Controller as AutogradersController
participant Model as Autograder Model
participant Helper as AssessmentAutogradeCore
participant TangoClient
Browser->>Controller: POST/PATCH form (with use_access_key flag)
Controller->>Controller: Normalize use_access_key to boolean<br/>Clear credentials if disabled
Controller->>Model: update(params)
Model->>Model: Validate instance_type<br/>Validate credentials if use_access_key?<br/>Encrypt access_key/access_key_id
Model-->>Controller: ✓ saved
rect rgba(0, 150, 200, 0.5)
Note over Controller,TangoClient: EC2 SSH Submission Flow
Controller->>Helper: tango_add_job(...)
alt EC2 SSH Enabled
Helper->>Helper: Build job_properties<br/>with ec2Vmms=true<br/>instanceType=t3.micro
alt Use Access Key
Helper->>Helper: Add accessKey<br/>Add accessKeyId
end
else EC2 SSH Disabled
Helper->>Helper: Build base job_properties only
end
Helper->>TangoClient: addjob(JSON payload)
TangoClient-->>Helper: job_id
end
Helper-->>Controller: ✓ queued
Controller-->>Browser: success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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: 13
🧹 Nitpick comments (7)
app/views/autograders/_form.html.erb (2)
3-15: Consider hiding tab bar when only one tab exists.When EC2 SSH is disabled, the tab bar shows only a single "Basic" tab, which may look unnecessary. Consider conditionally rendering the tab navigation only when multiple tabs exist.
💡 Suggested improvement
+<% show_tabs = Rails.configuration.x.ec2_ssh.presence %> +<% if show_tabs %> <div class="row"> <div class="col s12"> <ul id="tabs" class="tabs tabs-fixed-width"> - <% list = ["basic"] %> - <% if Rails.configuration.x.ec2_ssh.presence %> - <% list.append("ec2") %> - <% end %> + <% list = ["basic", "ec2"] %> <% list.each do |tab_name| %> ... <% end %> </ul> </div> </div> +<% end %>🤖 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 3 - 15, The tab bar is always rendered even when only the "basic" tab exists; change the ERB to build the list (currently via list = ["basic"] and optionally list.append("ec2")) and then conditionally render the <ul id="tabs" ...> block only when list.length > 1 so the tabs are hidden for a single tab. Keep the existing logic that iterates over list and uses params[:active_tab] to mark the active tab (the tag.li/link_to block) but wrap that rendering in an if list.size > 1 check so the tabs bar is omitted when there's only one entry.
21-23: Active tab is not preserved on form submission.Unlike the assessments edit form (which appends
active_tabto the form URL), this form always submits to the same endpoint without preserving the active tab. On validation failure, users will be redirected back to the default tab instead of staying on the EC2 tab where they may have entered invalid data.💡 Suggested fix to preserve active tab
- <%= form_for `@autograder`, url: course_assessment_autograder_path(`@course`, `@assessment`, `@autograder`), + <%= form_for `@autograder`, url: "#{course_assessment_autograder_path(`@course`, `@assessment`, `@autograder`)}?active_tab=#{params[:active_tab] || 'basic'}", builder: FormBuilderWithDateTimeInput, html: { multipart: true } do |f| %>And in
AutogradersController#update, redirect with the active_tab parameter:redirect_to(edit_course_assessment_autograder_path(`@course`, `@assessment`, active_tab: params[:active_tab] || 'basic'))🤖 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 21 - 23, The form is not preserving the active tab on submit; update the form_for call (the form_for `@autograder` with url: course_assessment_autograder_path(...)) to include the current active_tab (either by adding active_tab: params[:active_tab] to the URL options or by emitting a hidden input named "active_tab" with params[:active_tab]) so the param is sent on submit, and modify AutogradersController#update to redirect back with that param (use edit_course_assessment_autograder_path(`@course`, `@assessment`, active_tab: params[:active_tab] || 'basic')) so validation failures return the user to the same tab.spec/models/autograder_spec.rb (1)
14-18: Non-deterministic test may cause flaky failures.Using
Autograder::INSTANCE_TYPES.samplemakes this test non-deterministic. While unlikely, if the constant were ever empty, this would fail unexpectedly. Consider using a fixed value or the first element for consistent test behavior.💡 Suggested fix for deterministic test
it "allows listed instance types" do - autograder.instance_type = Autograder::INSTANCE_TYPES.sample + autograder.instance_type = Autograder::INSTANCE_TYPES.first expect(autograder).to be_valid end🤖 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 in spec/models/autograder_spec.rb uses Autograder::INSTANCE_TYPES.sample making it non-deterministic; change the assignment in the example block that sets autograder.instance_type = Autograder::INSTANCE_TYPES.sample to use a stable value (e.g., Autograder::INSTANCE_TYPES.first or a specific known-good string) so the example "allows listed instance types" reliably passes and cannot fail if the constant is empty or ordering changes.db/migrate/20251021034813_encrypt_autograder_credentials.rb (1)
24-42: Down migration should also use guards for consistency.The
downmethod should includecolumn_exists?guards to handle cases where the migration may have been partially run or the schema is in an unexpected state.💡 Suggested fix with guards
def down # Add back plaintext columns - add_column :autograders, :access_key, :string, default: "" - add_column :autograders, :access_key_id, :string, default: "" + add_column :autograders, :access_key, :string, default: "" unless column_exists?(:autograders, :access_key) + add_column :autograders, :access_key_id, :string, default: "" unless column_exists?(:autograders, :access_key_id) # Migrate encrypted data back to plaintext columns - Autograder.reset_column_information - Autograder.find_each do |autograder| - if autograder.access_key_ciphertext.present? - autograder.update_column(:access_key, autograder.access_key || "") - end - if autograder.access_key_id_ciphertext.present? - autograder.update_column(:access_key_id, autograder.access_key_id || "") - end - end + if column_exists?(:autograders, :access_key_ciphertext) + Autograder.reset_column_information + Autograder.find_each do |autograder| + if autograder.access_key_ciphertext.present? + autograder.update_column(:access_key, autograder.access_key || "") + end + if autograder.access_key_id_ciphertext.present? + autograder.update_column(:access_key_id, autograder.access_key_id || "") + end + end + end # Remove encrypted columns - remove_column :autograders, :access_key_ciphertext - remove_column :autograders, :access_key_id_ciphertext + remove_column :autograders, :access_key_ciphertext if column_exists?(:autograders, :access_key_ciphertext) + remove_column :autograders, :access_key_id_ciphertext if column_exists?(:autograders, :access_key_id_ciphertext) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251021034813_encrypt_autograder_credentials.rb` around lines 24 - 42, Update the down method to guard every schema operation with column_exists? checks: before add_column calls for :access_key and :access_key_id, wrap them in column_exists?(:autograders, :access_key) / column_exists?(:autograders, :access_key_id) logic (or the inverse via unless) so you don't error if they already exist; after calling Autograder.reset_column_information and inside the Autograder.find_each loop, check column_exists?(:autograders, :access_key_ciphertext) and column_exists?(:autograders, :access_key_id_ciphertext) before referencing autograder.access_key_ciphertext / access_key_id_ciphertext; and before remove_column calls for :access_key_ciphertext and :access_key_id_ciphertext, ensure those columns exist with column_exists?(:autograders, ...) to avoid failures when removing missing columns.app/models/autograder.rb (1)
21-24: Redundant validation:inclusionduplicatesnumericalityconstraints.The
numericalityvalidation on lines 22-23 already enforces>= 10and<= 900. Theinclusionvalidation on line 24 is redundant.♻️ Proposed fix
# Validations validates :autograde_timeout, numericality: { greater_than_or_equal_to: 10, less_than_or_equal_to: 900 } - validates :autograde_timeout, inclusion: { in: 10..900 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/autograder.rb` around lines 21 - 24, The validations for autograde_timeout are duplicated: the numericality check on :autograde_timeout in the Autograder model already enforces >=10 and <=900, so remove the redundant validates :autograde_timeout, inclusion: { in: 10..900 } line; keep the existing numericality validation in the Autograder class to enforce the limits.app/views/autograders/_basic_settings.html.erb (1)
30-35: Remove unnecessaryfile_pathparameter from download links.The
download_fileaction (perapp/controllers/autograders_controller.rb:104-121) only usesfile_keyto look up the file path from an allowlist. Thefile_pathparameter is ignored server-side and unnecessarily exposes internal filesystem paths in URLs.♻️ Proposed fix
<% unless `@makefile_exists.nil`? %> - <%= 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(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" %> + <%= link_to "Download Tar", download_file_course_assessment_autograder_path(file_key: 'tar'), class: "btn" %> <% end %>🤖 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 links currently include an unnecessary and sensitive file_path query param; update the two link_to calls in _basic_settings.html.erb that call download_file_course_assessment_autograder_path (the ones guarded by `@makefile_exists` and `@tar_exists`) to stop passing file_path and only pass file_key and other needed options (e.g., class: "btn"), so the URL contains only file_key used by the controller's download_file action.spec/helpers/assessment_autograde_core_spec.rb (1)
31-39: Consider using a local variable instead of instance variable for captured payload.Using
@captured_payload_jsonas an instance variable works but is fragile. A more idiomatic RSpec approach would useletwith a memoized reference or a dedicated capture mechanism.♻️ Alternative approach using let with a wrapper
let(:captured_payloads) { [] } before do helper_instance.instance_variable_set(:`@autograde_prop`, autograder) allow(TangoClient).to receive(:addjob) do |_queue, payload| captured_payloads << payload { "jobId" => 1 } end end # Then in tests: let(:captured_payload) { JSON.parse(captured_payloads.last) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/helpers/assessment_autograde_core_spec.rb` around lines 31 - 39, The test currently stores the stubbed TangoClient.addjob payload in an instance variable `@captured_payload_json` which is brittle; replace it with a local memoized capture: define let(:captured_payloads) { [] } and in the before block (where helper_instance.instance_variable_set(:`@autograde_prop`, autograder) and allow(TangoClient).to receive(:addjob) do |_queue, payload| ...) push payload into captured_payloads (e.g. captured_payloads << payload) and return the job hash; then redefine let(:captured_payload) to parse the last captured_payloads entry (JSON.parse(captured_payloads.last)) so tests read from the let-based capture instead of `@captured_payload_json`.
🤖 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/assets/javascripts/autograder.js`:
- Around line 30-33: The hover handler on the option elements
('#autograder_instance_type option' with .hover) is unreliable because most
browsers don't fire hover on <option>; remove this block from autograder.js and
either implement a custom dropdown component for hover highlighting or attach
hover handlers to a custom list/replica element instead; locate the selector
'#autograder_instance_type option' and the anonymous hover callbacks and delete
or replace them with handlers on a custom element that supports
mouseenter/mouseleave.
In `@app/controllers/jobs_controller.rb`:
- Around line 250-255: The last-trace check in jobs_controller.rb is too strict
(it only checks for "Error") so messages like "failed" or lowercase "error"
become "Unknown State"; update the conditional that reads rjob["trace"][-1] to
perform a case-insensitive match for either "fail" or "error" (e.g. downcase
include checks or a /fail|error/i regex) and set job[:state] = "Failed" when
either is present so failed/failed-like terminal messages are detected
consistently with tango_plot_data.
In `@app/controllers/scoreboards_controller.rb`:
- Around line 313-337: The current fix pads short parsed["scoreboard"] rows with
nil which makes them non-empty and bypasses the invalid-row fallback used by
scoreboardOrderSubmissions; instead of padding, detect when
parsed["scoreboard"].length < expected_length, log the warning, and mark the
entry as invalid so the existing invalid-row handling runs (for example clear
parsed["scoreboard"] or set a boolean flag on parsed like
parsed["_invalid_scoreboard"] = true) so later code (scoreboardOrderSubmissions
and the view rendering error_icon) treats the row as malformed and does not
include it in sorting; remove the fill/padding logic and ensure the rest of the
code checks the new invalid marker or empty value.
In `@app/helpers/assessment_autograde_core.rb`:
- Around line 183-186: The job_properties hash is being JSON-serialized before
calling TangoClient.addjob, but addjob currently posts the string without an
explicit Content-Type header; update either the call site or TangoClient.addjob
so the request sends the correct header: either pass the raw job_properties hash
to TangoClient.addjob (remove to_json in assessment_autograde_core.rb so
HTTParty can serialize and set headers) or modify TangoClient.addjob to accept
headers and set "Content-Type" => "application/json" (consistent with how
upload_file and upload_docker_image set headers). Ensure the change references
the job_properties usage in assessment_autograde_core.rb and the
TangoClient.addjob method.
In `@app/models/assessment.rb`:
- Around line 97-99: The current asmt_yaml_path method always returns
"assignment_config.yml" which breaks loading of legacy tarballs; update
load_yaml to try the new asmt_yaml_path first but fall back to the legacy
per-assessment filename (the original "<assignment>.yml" used before the rename)
if the new file is missing, while still writing/saving to asmt_yaml_path; modify
load_yaml to check both paths (asmt_yaml_path and the legacy filename) and use
whichever exists.
In `@app/models/autograder.rb`:
- Around line 8-9: In the Autograder model replace the Rails 7 `encrypts` calls
with Lockbox-compatible `has_encrypted` declarations: change the two occurrences
of `encrypts :access_key` and `encrypts :access_key_id` in the Autograder class
to use `has_encrypted :access_key` and `has_encrypted :access_key_id` (follow
the same pattern used in GithubIntegration model `has_encrypted` declarations).
In `@app/models/course.rb`:
- Around line 368-371: The export is currently writing the entire user record
via cud.user.attributes (in the course_user_data.each block where tar.add_file
is called), which leaks sensitive auth/PII fields; replace this by whitelisting
only required user fields for re-import/identity mapping (e.g. id,
login/username, name/display_name, email, and any other minimal mapping attrs)
instead of dumping all attributes. Implement this by defining an allowlist
(constant or method) and writing only user.slice(*ALLOWED_EXPORT_USER_FIELDS) or
a dedicated to_export_hash on User, then use that filtered hash (or its YAML)
when calling tar_file.write in the loop. Ensure no auth-related fields like
encrypted_password, reset/confirmation tokens, sign-in metadata, or admin flags
are included.
- Around line 387-405: The export currently writes flat assessment and
submission YAML from assessment.attributes.to_yaml, which loses the assignment
bundle and produces YAML that Assessment#load_yaml can't consume; change the
export to restore the per-assignment directory layout (create a directory per
assessment, e.g. "assessments/assessment_<id>/") and copy the full assignment
bundle files into that directory (assignment_config.yml,
"#{assessment.name}.rb", log.txt, writeup/autograde assets,
problems/autograder/scoreboard files) instead of emitting only
assessment.attributes; serialize assessment data using the same format expected
by Assessment#load_yaml (use the existing exporter/dump routine or build the
"general/dates/..." structure rather than assessment.attributes.to_yaml), and
when exporting submissions iterate assessment.submissions and include the actual
submission artifact files (submission files/artifacts) into the corresponding
assignment directory (or a parallel submission directory per assessment) rather
than writing submission.attributes.to_yaml.
In `@app/views/autograders/_basic_settings.html.erb`:
- Around line 26-28: Fix the typo in the confirmation message used in the
link_to "Delete Autograder" helper: update the data confirm string on the
link_to (the call that currently contains "Are you sure you want to delete the
Autograder for this assesssment?") to spell "assessment" correctly so the
message reads "Are you sure you want to delete the Autograder for this
assessment?".
In `@app/views/autograders/_ec2_settings.html.erb`:
- Around line 107-109: Fix the typo in the confirmation message of the "Delete
Autograder" link: update the data confirm string in the link_to call that uses
course_assessment_autograder_path(`@course`, `@assessment`) so "assesssment" is
corrected to "assessment" in the confirmation prompt.
- Around line 64-70: The view raises ArgumentError for an invalid
selected_instance_type which causes a 500; instead, detect invalid values from
the canonical Autograder::INSTANCE_TYPES (do not duplicate ec2_instance_options
logic), and either default selected_instance_type to a safe value (e.g., nil or
the first valid option) or set a visible error message to render in the
template; update the block where selected_instance_type is computed (currently
using ec2_instance_options and the local selected_instance_type) to validate
against Autograder::INSTANCE_TYPES and handle invalid cases gracefully (default
or set flash/inline error) rather than raising.
In `@app/views/scoreboards/show.html.erb`:
- Around line 80-89: The row collapses because the template only iterates
existing elements of grade[:entry]; when the controller falls back to
grade[:entry] = [] after createScoreboardEntry fails the view renders fewer
<td>s. Fix by ensuring the view renders a fixed number of cells: in
show.html.erb, replace the current conditional iteration over grade[:entry] with
a loop over the expected column count (calculate from the header or a known
constant) and for each index render <td><%= column_value %></td> if
grade[:entry][i] present else render the 'error_icon' placeholder; alternatively
ensure createScoreboardEntry always returns an array of the expected length
(e.g., Array.new(expected_count, nil)) so the existing per-index rendering still
produces placeholder cells. Ensure references to grade[:entry],
createScoreboardEntry, and the 'error_icon' partial are used to locate the code
to change.
In `@db/migrate/20251021034813_encrypt_autograder_credentials.rb`:
- Around line 1-22: The migration EncryptAutograderCredentials currently
unconditionally adds/removes columns and reads plaintext columns that may
already have been added/removed by a prior migration; update the up method to
guard each schema operation with column_exists? checks (use unless
column_exists?(:autograders, :access_key_ciphertext) / unless
column_exists?(:autograders, :access_key_id_ciphertext) before add_column, and
wrap remove_column calls with if column_exists?(:autograders, :access_key) / if
column_exists?(:autograders, :access_key_id)), and when iterating
Autograder.find_each only read and copy plaintext attributes if the plaintext
columns actually exist (guard reads of :access_key and :access_key_id),
preserving the existing save(validate: false) behavior and calling
Autograder.reset_column_information appropriately after any schema changes.
---
Nitpick comments:
In `@app/models/autograder.rb`:
- Around line 21-24: The validations for autograde_timeout are duplicated: the
numericality check on :autograde_timeout in the Autograder model already
enforces >=10 and <=900, so remove the redundant validates :autograde_timeout,
inclusion: { in: 10..900 } line; keep the existing numericality validation in
the Autograder class to enforce the limits.
In `@app/views/autograders/_basic_settings.html.erb`:
- Around line 30-35: The download links currently include an unnecessary and
sensitive file_path query param; update the two link_to calls in
_basic_settings.html.erb that call
download_file_course_assessment_autograder_path (the ones guarded by
`@makefile_exists` and `@tar_exists`) to stop passing file_path and only pass
file_key and other needed options (e.g., class: "btn"), so the URL contains only
file_key used by the controller's download_file action.
In `@app/views/autograders/_form.html.erb`:
- Around line 3-15: The tab bar is always rendered even when only the "basic"
tab exists; change the ERB to build the list (currently via list = ["basic"] and
optionally list.append("ec2")) and then conditionally render the <ul id="tabs"
...> block only when list.length > 1 so the tabs are hidden for a single tab.
Keep the existing logic that iterates over list and uses params[:active_tab] to
mark the active tab (the tag.li/link_to block) but wrap that rendering in an if
list.size > 1 check so the tabs bar is omitted when there's only one entry.
- Around line 21-23: The form is not preserving the active tab on submit; update
the form_for call (the form_for `@autograder` with url:
course_assessment_autograder_path(...)) to include the current active_tab
(either by adding active_tab: params[:active_tab] to the URL options or by
emitting a hidden input named "active_tab" with params[:active_tab]) so the
param is sent on submit, and modify AutogradersController#update to redirect
back with that param (use edit_course_assessment_autograder_path(`@course`,
`@assessment`, active_tab: params[:active_tab] || 'basic')) so validation failures
return the user to the same tab.
In `@db/migrate/20251021034813_encrypt_autograder_credentials.rb`:
- Around line 24-42: Update the down method to guard every schema operation with
column_exists? checks: before add_column calls for :access_key and
:access_key_id, wrap them in column_exists?(:autograders, :access_key) /
column_exists?(:autograders, :access_key_id) logic (or the inverse via unless)
so you don't error if they already exist; after calling
Autograder.reset_column_information and inside the Autograder.find_each loop,
check column_exists?(:autograders, :access_key_ciphertext) and
column_exists?(:autograders, :access_key_id_ciphertext) before referencing
autograder.access_key_ciphertext / access_key_id_ciphertext; and before
remove_column calls for :access_key_ciphertext and :access_key_id_ciphertext,
ensure those columns exist with column_exists?(:autograders, ...) to avoid
failures when removing missing columns.
In `@spec/helpers/assessment_autograde_core_spec.rb`:
- Around line 31-39: The test currently stores the stubbed TangoClient.addjob
payload in an instance variable `@captured_payload_json` which is brittle; replace
it with a local memoized capture: define let(:captured_payloads) { [] } and in
the before block (where helper_instance.instance_variable_set(:`@autograde_prop`,
autograder) and allow(TangoClient).to receive(:addjob) do |_queue, payload| ...)
push payload into captured_payloads (e.g. captured_payloads << payload) and
return the job hash; then redefine let(:captured_payload) to parse the last
captured_payloads entry (JSON.parse(captured_payloads.last)) so tests read from
the let-based capture instead of `@captured_payload_json`.
In `@spec/models/autograder_spec.rb`:
- Around line 14-18: The test in spec/models/autograder_spec.rb uses
Autograder::INSTANCE_TYPES.sample making it non-deterministic; change the
assignment in the example block that sets autograder.instance_type =
Autograder::INSTANCE_TYPES.sample to use a stable value (e.g.,
Autograder::INSTANCE_TYPES.first or a specific known-good string) so the example
"allows listed instance types" reliably passes and cannot fail if the constant
is empty or ordering changes.
🪄 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: 9442a7d7-b30b-4146-afa6-cc255ab04f14
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (27)
app/assets/javascripts/autograder.jsapp/controllers/announcements_controller.rbapp/controllers/assessments_controller.rbapp/controllers/autograders_controller.rbapp/controllers/jobs_controller.rbapp/controllers/scoreboards_controller.rbapp/controllers/scores_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.rbconfig/routes.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/controllers/scores_controller_spec.rbspec/factories/autograders.rbspec/helpers/assessment_autograde_core_spec.rbspec/models/autograder_spec.rb
💤 Files with no reviewable changes (3)
- spec/controllers/scores_controller_spec.rb
- config/routes.rb
- app/controllers/scores_controller.rb
Comment on lines
+30
to
+33
| $('#autograder_instance_type option').hover( | ||
| function() { $(this).addClass('highlighted-option'); }, | ||
| function() { $(this).removeClass('highlighted-option'); } | ||
| ); |
Contributor
There was a problem hiding this comment.
hover events on <option> elements are not reliably supported.
Most browsers do not fire hover events on <option> elements inside <select> dropdowns. This code will effectively be dead in Chrome, Firefox, and Safari. If visual feedback on hover is desired, consider using a custom dropdown component or simply remove this code.
🗑️ Suggested removal of unsupported code
- $('#autograder_instance_type option').hover(
- function() { $(this).addClass('highlighted-option'); },
- function() { $(this).removeClass('highlighted-option'); }
- );📝 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
| $('#autograder_instance_type option').hover( | |
| function() { $(this).addClass('highlighted-option'); }, | |
| function() { $(this).removeClass('highlighted-option'); } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/assets/javascripts/autograder.js` around lines 30 - 33, The hover handler
on the option elements ('#autograder_instance_type option' with .hover) is
unreliable because most browsers don't fire hover on <option>; remove this block
from autograder.js and either implement a custom dropdown component for hover
highlighting or attach hover handlers to a custom list/replica element instead;
locate the selector '#autograder_instance_type option' and the anonymous hover
callbacks and delete or replace them with handlers on a custom element that
supports mouseenter/mouseleave.
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.
Don't narrow failed-job detection to exact Error in the last trace field.
This is stricter than the existing heuristics in tango_plot_data, which look for lowercase fail / error. Dead jobs with terminal messages like failed or error will now fall through to Unknown State, and app/views/jobs/index.html.erb only highlights rows when the state is Failed, so those failures become easy to miss.
Suggested fix
- 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"
- end
+ else
+ last_trace = rjob["trace"][-1].to_s
+
+ job[:state] =
+ if last_trace.include?("Autodriver returned normally")
+ "Succeeded"
+ elsif last_trace.match?(/error|fail/i)
+ "Failed"
+ else
+ "Unknown State"
+ end
+ 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 last-trace
check in jobs_controller.rb is too strict (it only checks for "Error") so
messages like "failed" or lowercase "error" become "Unknown State"; update the
conditional that reads rjob["trace"][-1] to perform a case-insensitive match for
either "fail" or "error" (e.g. downcase include checks or a /fail|error/i regex)
and set job[:state] = "Failed" when either is present so failed/failed-like
terminal messages are detected consistently with tango_plot_data.
Comment on lines
+313
to
+337
| # ADDED: Ensure the scoreboard entries align with colspec expectations | ||
| if @scoreboard && @scoreboard.colspec.present? | ||
| begin | ||
| colspec_parsed = ActiveSupport::JSON.decode(@scoreboard.colspec) | ||
| if colspec_parsed && colspec_parsed["scoreboard"] && | ||
| colspec_parsed["scoreboard"].is_a?(Array) | ||
| expected_length = colspec_parsed["scoreboard"].length | ||
| # Ensure the entry has enough elements, pad with nil if needed | ||
| if parsed["scoreboard"].length < expected_length | ||
| Rails.logger.warn( | ||
| "Scoreboard entry too short (#{parsed['scoreboard'].length}) " \ | ||
| "for user, expected #{expected_length}" | ||
| ) | ||
| # Pad the array to match expected length | ||
| parsed["scoreboard"] = parsed["scoreboard"].fill( | ||
| nil, | ||
| parsed["scoreboard"].length, | ||
| expected_length - parsed["scoreboard"].length | ||
| ) | ||
| end | ||
| end | ||
| rescue StandardError => e | ||
| Rails.logger.error("Error validating scoreboard entry against colspec: #{e.message}") | ||
| end | ||
| end |
Contributor
There was a problem hiding this comment.
Don't turn malformed scoreboard rows into sortable data.
Padding a short entry with nil makes it non-empty, so it skips the invalid-row fallback at Lines 112-113 and still goes through scoreboardOrderSubmissions at Lines 393-401. That changes ranking behavior for broken autograder output instead of treating the row as invalid. Since the view now renders error_icon for missing cells, keep short rows marked invalid for sorting rather than padding them here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/scoreboards_controller.rb` around lines 313 - 337, The
current fix pads short parsed["scoreboard"] rows with nil which makes them
non-empty and bypasses the invalid-row fallback used by
scoreboardOrderSubmissions; instead of padding, detect when
parsed["scoreboard"].length < expected_length, log the warning, and mark the
entry as invalid so the existing invalid-row handling runs (for example clear
parsed["scoreboard"] or set a boolean flag on parsed like
parsed["_invalid_scoreboard"] = true) so later code (scoreboardOrderSubmissions
and the view rendering error_icon) treats the row as malformed and does not
include it in sorting; remove the fill/padding logic and ensure the rest of the
code checks the new invalid marker or empty value.
Comment on lines
+183
to
186
| job_properties = job_properties.to_json | ||
|
|
||
| begin | ||
| response = TangoClient.addjob("#{course.name}-#{assessment.name}", job_properties) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how TangoClient.addjob handles the body parameter and if headers are set
rg -n -A 10 'def self\.addjob' lib/tango_client.rb
# Check if there's any content-type header configuration
rg -n 'content.type|Content-Type|headers' lib/tango_client.rbRepository: autolab/Autolab
Length of output: 729
🏁 Script executed:
#!/bin/bash
# Check HTTParty configuration in TangoClient
rg -n -B 5 -A 5 'class TangoClient' lib/tango_client.rb
# Check if there are global headers configured
rg -n 'base_uri|default_params|headers' lib/tango_client.rb | head -20
# Look at the context in assessment_autograde_core.rb around line 183
sed -n '170,190p' app/helpers/assessment_autograde_core.rbRepository: autolab/Autolab
Length of output: 1137
🏁 Script executed:
#!/bin/bash
# Search for all calls to TangoClient.addjob
rg -n 'TangoClient\.addjob|\.addjob' --type ruby
# Check git history to see if this was recently changed
git log -n 5 --oneline -- app/helpers/assessment_autograde_core.rb
# Look for any documentation or comments about addjob format
rg -n -B 3 'def self\.addjob' lib/tango_client.rbRepository: autolab/Autolab
Length of output: 344
🏁 Script executed:
#!/bin/bash
# Check git diff for the recent changes to assessment_autograde_core.rb
git log --oneline -n 10 -- app/helpers/assessment_autograde_core.rb
# Look at the specific method/function around line 160-190 to understand the refactoring
git show HEAD:app/helpers/assessment_autograde_core.rb | sed -n '160,190p'
# Also check if there's a previous commit that might show the original implementation
git log -p --all -S 'addjob' -- app/helpers/assessment_autograde_core.rb | head -100Repository: autolab/Autolab
Length of output: 1275
🏁 Script executed:
#!/bin/bash
# Use git blame to see when the .to_json line was added
git blame app/helpers/assessment_autograde_core.rb | grep -A 2 -B 2 'to_json'
# Check if there's a way to see the real history
git log --all --oneline -- app/helpers/assessment_autograde_core.rb | head -20
# Let's also check the actual TangoClient documentation or comments about what format it expects
rg -n -B 5 -A 15 'def self\.addjob' lib/tango_client.rbRepository: autolab/Autolab
Length of output: 2231
🏁 Script executed:
#!/bin/bash
# Show the parent commit of ce48033
git show ce48033^:app/helpers/assessment_autograde_core.rb | sed -n '160,195p'
# Also check what changed in ce48033
git show ce48033 -- app/helpers/assessment_autograde_core.rb | head -150Repository: autolab/Autolab
Length of output: 5358
Verify that TangoClient.addjob receives the correct Content-Type header for JSON data.
The job_properties hash is serialized to JSON at line 183 before passing to TangoClient.addjob. However, TangoClient.addjob passes this string directly to HTTParty.post(url, body: options) without setting an explicit Content-Type: application/json header. HTTParty will send the JSON string as-is without this header.
Compare this with other methods in TangoClient: upload_file explicitly sets headers (headers: { "filename" => filename }), and upload_docker_image sets headers: { "Content-Type": "application/octet-stream" }.
Either modify TangoClient.addjob to accept and set a Content-Type header, or pass the job_properties hash directly instead of pre-serializing it to JSON and let HTTParty handle the serialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/helpers/assessment_autograde_core.rb` around lines 183 - 186, The
job_properties hash is being JSON-serialized before calling TangoClient.addjob,
but addjob currently posts the string without an explicit Content-Type header;
update either the call site or TangoClient.addjob so the request sends the
correct header: either pass the raw job_properties hash to TangoClient.addjob
(remove to_json in assessment_autograde_core.rb so HTTParty can serialize and
set headers) or modify TangoClient.addjob to accept headers and set
"Content-Type" => "application/json" (consistent with how upload_file and
upload_docker_image set headers). Ensure the change references the
job_properties usage in assessment_autograde_core.rb and the TangoClient.addjob
method.
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.
Keep the legacy assessment YAML filename as a read fallback.
load_yaml reads through asmt_yaml_path too, so this rename makes pre-existing course folders / tarballs that still contain <assignment>.yml fail with ENOENT. Keep assignment_config.yml as the new write path, but fall back to the legacy filename when loading.
🧩 Compatibility fallback
def load_yaml
return unless new_record?
- props = YAML.safe_load(File.open(asmt_yaml_path, "r", &:read))
+ yaml_path = [asmt_yaml_path, path("#{name}.yml")].find { |candidate| File.exist?(candidate) }
+ raise Errno::ENOENT, asmt_yaml_path unless yaml_path
+
+ props = YAML.safe_load(File.read(yaml_path))
validate_yaml(props)
backwards_compatibility(props)
deserialize(props)
end🤖 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 current asmt_yaml_path
method always returns "assignment_config.yml" which breaks loading of legacy
tarballs; update load_yaml to try the new asmt_yaml_path first but fall back to
the legacy per-assessment filename (the original "<assignment>.yml" used before
the rename) if the new file is missing, while still writing/saving to
asmt_yaml_path; modify load_yaml to check both paths (asmt_yaml_path and the
legacy filename) and use whichever exists.
Comment on lines
+26
to
+28
| <%= link_to "Delete Autograder", course_assessment_autograder_path(@course, @assessment), | ||
| method: :delete, class: "btn danger", | ||
| data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> |
Contributor
There was a problem hiding this comment.
Typo in confirmation message: "assesssment" → "assessment".
✏️ Proposed fix
<%= link_to "Delete Autograder", course_assessment_autograder_path(`@course`, `@assessment`),
method: :delete, class: "btn danger",
- data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %>
+ data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>🤖 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 26 - 28, Fix the
typo in the confirmation message used in the link_to "Delete Autograder" helper:
update the data confirm string on the link_to (the call that currently contains
"Are you sure you want to delete the Autograder for this assesssment?") to spell
"assessment" correctly so the message reads "Are you sure you want to delete the
Autograder for this assessment?".
Comment on lines
+64
to
+70
| <% | ||
| 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 | ||
| %> |
Contributor
There was a problem hiding this comment.
Raising ArgumentError in a view causes a 500 error instead of a user-friendly message.
If an invalid instance_type is somehow stored (e.g., after changing the allowed list), this will crash the page. Consider rendering an error message or defaulting to a safe value instead.
Additionally, this validation duplicates the model's INSTANCE_TYPES constant. Consider referencing Autograder::INSTANCE_TYPES to ensure consistency.
🛠️ Proposed 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}"
+ if selected_instance_type.present? && !Autograder::INSTANCE_TYPES.include?(selected_instance_type)
+ # Fall back to blank selection and let the user pick a valid type
+ selected_instance_type = nil
+ flash.now[:error] ||= "Previously saved instance type is no longer valid. Please select a new one."
end
%>Then update line 75:
<%= f.select :instance_type,
- grouped_options_for_select(ec2_instance_options, `@autograder.instance_type`),
+ grouped_options_for_select(ec2_instance_options, selected_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 - 70, The view
raises ArgumentError for an invalid selected_instance_type which causes a 500;
instead, detect invalid values from the canonical Autograder::INSTANCE_TYPES (do
not duplicate ec2_instance_options logic), and either default
selected_instance_type to a safe value (e.g., nil or the first valid option) or
set a visible error message to render in the template; update the block where
selected_instance_type is computed (currently using ec2_instance_options and the
local selected_instance_type) to validate against Autograder::INSTANCE_TYPES and
handle invalid cases gracefully (default or set flash/inline error) rather than
raising.
Comment on lines
+107
to
+109
| <%= link_to "Delete Autograder", course_assessment_autograder_path(@course, @assessment), | ||
| method: :delete, class: "btn danger", | ||
| data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> |
Contributor
There was a problem hiding this comment.
Same typo: "assesssment" → "assessment".
✏️ Proposed fix
<%= link_to "Delete Autograder", course_assessment_autograder_path(`@course`, `@assessment`),
method: :delete, class: "btn danger",
- data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %>
+ data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>📝 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
| <%= link_to "Delete Autograder", course_assessment_autograder_path(@course, @assessment), | |
| method: :delete, class: "btn danger", | |
| data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> | |
| <%= link_to "Delete Autograder", course_assessment_autograder_path(`@course`, `@assessment`), | |
| method: :delete, class: "btn danger", | |
| data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %> |
🤖 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 107 - 109, Fix the
typo in the confirmation message of the "Delete Autograder" link: update the
data confirm string in the link_to call that uses
course_assessment_autograder_path(`@course`, `@assessment`) so "assesssment" is
corrected to "assessment" in the confirmation prompt.
Comment on lines
80
to
89
| <% if grade[:entry].is_a?(Array) %> | ||
| <% grade[:entry].each do |column| %> | ||
| <td><%= column %></td> | ||
| <% if !column.nil? %> | ||
| <td><%= column %></td> | ||
| <% else %> | ||
| <%= render 'error_icon' %> | ||
| <% end %> | ||
| <% end %> | ||
| <% else %> | ||
| <%= render 'error_icon' %> |
Contributor
There was a problem hiding this comment.
Default-scoreboard rows can still collapse when the entry is short.
This branch only iterates the cells that are present. When the controller falls back to grade[:entry] = [] after a createScoreboardEntry error (app/controllers/scoreboards_controller.rb Lines 83-87), the row stops after the Time column and the table becomes misaligned instead of showing placeholders for Total/problem columns.
Possible fix
- <% if grade[:entry].is_a?(Array) %>
- <% grade[:entry].each do |column| %>
- <% if !column.nil? %>
- <td><%= column %></td>
- <% else %>
- <%= render 'error_icon' %>
- <% end %>
- <% end %>
- <% else %>
- <%= render 'error_icon' %>
- <% end %>
+ <% expected_columns = 1 + `@assessment.problems.length` %>
+ <% expected_columns.times do |idx| %>
+ <% column = grade[:entry].is_a?(Array) ? grade[:entry][idx] : nil %>
+ <% if column.nil? %>
+ <%= render 'error_icon' %>
+ <% else %>
+ <td><%= column %></td>
+ <% end %>
+ <% end %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/scoreboards/show.html.erb` around lines 80 - 89, The row collapses
because the template only iterates existing elements of grade[:entry]; when the
controller falls back to grade[:entry] = [] after createScoreboardEntry fails
the view renders fewer <td>s. Fix by ensuring the view renders a fixed number of
cells: in show.html.erb, replace the current conditional iteration over
grade[:entry] with a loop over the expected column count (calculate from the
header or a known constant) and for each index render <td><%= column_value
%></td> if grade[:entry][i] present else render the 'error_icon' placeholder;
alternatively ensure createScoreboardEntry always returns an array of the
expected length (e.g., Array.new(expected_count, nil)) so the existing per-index
rendering still produces placeholder cells. Ensure references to grade[:entry],
createScoreboardEntry, and the 'error_icon' partial are used to locate the code
to change.
Comment on lines
+1
to
+22
| class EncryptAutograderCredentials < ActiveRecord::Migration[6.1] | ||
| def up | ||
| # Add encrypted columns for access keys | ||
| add_column :autograders, :access_key_ciphertext, :text | ||
| add_column :autograders, :access_key_id_ciphertext, :text | ||
|
|
||
| # Migrate existing plaintext data to encrypted columns | ||
| Autograder.reset_column_information | ||
| Autograder.find_each do |autograder| | ||
| if autograder.read_attribute(:access_key).present? | ||
| autograder.access_key = autograder.read_attribute(:access_key) | ||
| end | ||
| if autograder.read_attribute(:access_key_id).present? | ||
| autograder.access_key_id = autograder.read_attribute(:access_key_id) | ||
| end | ||
| autograder.save(validate: false) # Skip validation during migration | ||
| end | ||
|
|
||
| # Remove plaintext columns after migration | ||
| remove_column :autograders, :access_key | ||
| remove_column :autograders, :access_key_id | ||
| end |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the existing migration and potential conflicts
echo "=== Checking existing migration 20241205233214 ==="
cat db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb
echo ""
echo "=== Checking schema for current column state ==="
rg -n 'access_key' db/schema.rb || echo "schema.rb not found or no matches"Repository: autolab/Autolab
Length of output: 1495
🏁 Script executed:
cat db/migrate/20251021034813_encrypt_autograder_credentials.rbRepository: autolab/Autolab
Length of output: 1706
Migration conflicts with existing migration 20241205233214_add_ec2_ssh_fields_to_autograders.rb.
This migration will fail when run after 20241205233214 due to duplicate operations:
-
Duplicate column additions: Migration
20241205233214already addsaccess_key_ciphertextandaccess_key_id_ciphertext. Lines 4-5 will fail with "column already exists" errors since they lack guards. -
Missing source columns: Migration
20241205233214removes the plaintextaccess_keyandaccess_key_idcolumns. Lines 10 and 13 attempt to read from these columns, which no longer exist. -
Duplicate column removals: Lines 20-21 attempt to remove columns already removed by
20241205233214.
Add column_exists? guards throughout the migration to safely handle both initial runs and conditional scenarios:
Suggested fix
def up
# Add encrypted columns for access keys
- add_column :autograders, :access_key_ciphertext, :text
- add_column :autograders, :access_key_id_ciphertext, :text
+ 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)
# Migrate existing plaintext data to encrypted columns
+ if column_exists?(:autograders, :access_key) && column_exists?(:autograders, :access_key_id)
Autograder.reset_column_information
Autograder.find_each do |autograder|
if autograder.read_attribute(:access_key).present?
autograder.access_key = autograder.read_attribute(:access_key)
end
if autograder.read_attribute(:access_key_id).present?
autograder.access_key_id = autograder.read_attribute(:access_key_id)
end
autograder.save(validate: false) # Skip validation during migration
end
+ end
# Remove plaintext columns after migration
- remove_column :autograders, :access_key
- remove_column :autograders, :access_key_id
+ 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
def down
add_column :autograders, :access_key, :string, default: "" unless column_exists?(:autograders, :access_key)
add_column :autograders, :access_key_id, :string, default: "" unless column_exists?(:autograders, :access_key_id)
if column_exists?(:autograders, :access_key_ciphertext) && column_exists?(:autograders, :access_key_id_ciphertext)
Autograder.reset_column_information
Autograder.find_each do |autograder|
if autograder.access_key_ciphertext.present?
autograder.update_column(:access_key, autograder.access_key || "")
end
if autograder.access_key_id_ciphertext.present?
autograder.update_column(:access_key_id, autograder.access_key_id || "")
end
end
end
remove_column :autograders, :access_key_ciphertext if column_exists?(:autograders, :access_key_ciphertext)
remove_column :autograders, :access_key_id_ciphertext if column_exists?(:autograders, :access_key_id_ciphertext)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20251021034813_encrypt_autograder_credentials.rb` around lines 1 -
22, The migration EncryptAutograderCredentials currently unconditionally
adds/removes columns and reads plaintext columns that may already have been
added/removed by a prior migration; update the up method to guard each schema
operation with column_exists? checks (use unless column_exists?(:autograders,
:access_key_ciphertext) / unless column_exists?(:autograders,
:access_key_id_ciphertext) before add_column, and wrap remove_column calls with
if column_exists?(:autograders, :access_key) / if column_exists?(:autograders,
:access_key_id)), and when iterating Autograder.find_each only read and copy
plaintext attributes if the plaintext columns actually exist (guard reads of
:access_key and :access_key_id), preserving the existing save(validate: false)
behavior and calling Autograder.reset_column_information appropriately after any
schema changes.
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
8 participants
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.
New tar format example:
test-course
test-course/course.rb
test-course/test-course.yml
test-course/llomac-lab/assignment_config.yml
test-course/llomac-lab/llomac-lab.rb
test-course/llomac-lab/llomac-lab.yml
test-course/llomac-lab/log.txt
test-course/hello_python/autograde-Makefile
test-course/hello_python/autograde.tar
test-course/hello_python/hello.py
test-course/hello_python/hello_python.rb
test-course/hello_python/hello_python.yml
test-course/hello_python/log.txt
test-course/hello_python/writeup
test-course/hello_python/writeup/writeup.html